-
-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make emasql-sqlite errors more informative, fix CI, add MS-Windows test workflow #116
Conversation
Thanks! It will probably be at least a few days until I get around to looking at this. |
7567b40
to
4a01f3e
Compare
Why are you adding a dependency on nix and eldev? Does this fix some issue? Does this enable something that wasn't possible before? I am not familiar with either and fear that once things break, I won't be able to fix it. (In fact this has happened to me before, and because I couldn't get the original contributor of the extended nix based ci to help, I had to remove it again.) |
nix CI dependencyThe dependency to nix is implicit because The emacs-sqlite-api's sqlite3-api.so lib requires the libsqlite3.so as a runtime dependency. on the CI linux runners, the sqlite-api.so dependencies resolve at runtime to
Trying to load the above sqlite3-api module dependencies from Emacs (say 28.2) deployed with nix, results to conflict against the libc version Emacs was built with
This is because Emacs was compiled with a different libc library version which is not compatible with the runner system's one
and thus it fails. One might ask, why is it then working with Emacs 29.1? It is because the latter comes with libsql3.so bundled as part of the built in sql3 support, and thus it does not have to load /lib/x86_64-linux-gnu/libsqlite3.so.0
As such if we were to load a libsqlite3.so, we have to make sure that it is compatible with the libc version that was used to build Emacs (the nix libc). The way to do so is to ask nix to create a development environment with libsqlite3 pre-installed (the - name: 'Build Sqlite3'
working-directory: sqlite3
run: nix-shell -p sqlite.dev --run "make all"
- name: 'Build Emacsql'
run: nix-shell -p sqlite.dev --run "make all" The same technique should be used when compiling emacs-sqlite-api in compile.yml compile:
name: Compile
uses: emacscollective/workflows/.github/workflows/compile.yml@main This will compile sqlite3-api.so, but we need it to do the ifdef NIX_PATH
ci-sqlite3-nix-fix:
SQLITE3-EL-PATH = ../sqlite3/sqlite3.el
ifneq (,$(wildcard $(SQLITE3-EL-PATH)))
# try to change the sqlite3 build command based on past and present
# known patterns found in ../sqlite3/sqlite3.el ...
ci-sqlite3-nix-fix:
@echo patching $(SQLITE3-EL-PATH) to build with nix...
sed -i 's\("make" "all")\("nix-shell" "-p" "sqlite.dev" "--run" "make all")\g' $(SQLITE3-EL-PATH)
sed -i 's/"make all"/"nix-shell -p sqlite.dev --run \\"make all\\""/g' $(SQLITE3-EL-PATH)
endif
endif
lisp: ci-sqlite3-nix-fix $(ELCS) loaddefs I can't think of linking with the correct libsqlite3.so and Emacs's libc.so library, other than invoking the build command with EldevWe can't use the same CI actions (test and compile) for Emacs on MS-Windows, because it only support pipelines on ubuntu and macos. There is a much simplified way to do so, by using the Eldev building tool. Eldev is a tool that simplifies developing Emacs packages. From specifying package dependencies, creating isolated Emacs environments, testing, linting, packaging and publishing to ELPAs. All thought a simple to use I'm using it to setup a GH workflow for testing the package on MS-Windows. The only thing it requires is an Eldev files specyfing the Emacs package dependencies for testing ( (eldev-add-extra-dependencies 'test 'sqlite3 'pg) and what files required to create the package (setf eldev-files-to-package
`(:or ,eldev-files-to-package
'("./sqlite/Makefile" "./sqlite/emacsql.c" "./sqlite/sqlite3.c" "./sqlite3.h"))) The tests are then simply invoked by using the following command in the CI ( - name: Test
run: |
# Update PATH to include first to the new msys2 dev
# environment.
$env:Path = "D:\msys64\mingw64\bin;" + $env:Path
#
eldev -p -dtTC test It works out of the box. It's not easy to create an Emacs CI workflow on Windows, without going into much difficulty with managing package dependencies. I would have hoped that if you were acquitted with Eldev, you'd used it for the whole development cycle, it is that good.
Indeed. I will be happy to support these two aspect if something breaks. It will also be easy to revert if there are unforeseen troubles:
Thanks |
lisp: $(ELCS) loaddefs | ||
# Emacs in GitHub Actions was built with nix; consequently, we must | ||
# also utilize nix to build its modules. This ensures proper linking | ||
# with the correct library paths. | ||
.PHONY: ci-sqlite3-nix-fix | ||
ci-sqlite3-nix-fix: | ||
|
||
ifdef NIX_PATH | ||
ci-sqlite3-nix-fix: | ||
SQLITE3-EL-PATH = ../sqlite3/sqlite3.el | ||
ifneq (,$(wildcard $(SQLITE3-EL-PATH))) | ||
# try to change the sqlite3 build command based on past and present | ||
# known patterns found in ../sqlite3/sqlite3.el ... | ||
ci-sqlite3-nix-fix: | ||
@echo patching $(SQLITE3-EL-PATH) to build with nix... | ||
sed -i 's\("make" "all")\("nix-shell" "-p" "sqlite.dev" "--run" "make all")\g' $(SQLITE3-EL-PATH) | ||
sed -i 's/"make all"/"nix-shell -p sqlite.dev --run \\"make all\\""/g' $(SQLITE3-EL-PATH) | ||
endif | ||
endif | ||
|
||
lisp: ci-sqlite3-nix-fix $(ELCS) loaddefs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... I could do without this.
A nix user should know that they have to run nix-shell -p sqlite.dev --run make all
. Alternatively, doesn't nix have something like per-project guix.scm
?
I would rather not add distribution-specific things to the Makefile. A nix.something
file would be okay though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, wait you are patching ../sqlite3/sqlite3.el
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what about cd ../sqlite3/; nix-shell -p sqlite.dev --run make all
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By that I mean that we don't need this for ci, just for extra convenience of nix users who are building locally. If we need this for ci, that is fine and helpful, but I don't want to add kludges to support nix, eldev and whatever comes next. But a "how to build with ..." page on the wiki would be helpful and appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this is the only hack I can think of, of having https://github.com/magit/emacsql/blob/main/.github/workflows/compile.yml workflow work, and invoke nix-shell -p sqlite.dev --run make all
for sqlite3, by patching sqlite3.el. It is not specifically targeting nix
users, it is just fixing the CI break for compile.yml.
some more info
The entry point of this workflow is the Makefile
s lisp:
target
In this workflow, we don't have the luxury ofexecuting any code. Thus I hacked the lisp:
target entry point. Whenever this action is executed and make lisp
is invoked, I added an additional target to replace sqlite3.el
make all
command
# Emacs in GitHub Actions was built with nix; consequently, we must
# also utilize nix to build its modules. This ensures proper linking
# with the correct library paths.
.PHONY: ci-sqlite3-nix-fix
ci-sqlite3-nix-fix:
ifdef NIX_PATH
ci-sqlite3-nix-fix:
SQLITE3-EL-PATH = ../sqlite3/sqlite3.el
ifneq (,$(wildcard $(SQLITE3-EL-PATH)))
# try to change the sqlite3 build command based on past and present
# known patterns found in ../sqlite3/sqlite3.el ...
ci-sqlite3-nix-fix:
@echo patching $(SQLITE3-EL-PATH) to build with nix...
sed -i 's\("make" "all")\("nix-shell" "-p" "sqlite.dev" "--run" "make all")\g' $(SQLITE3-EL-PATH)
sed -i 's/"make all"/"nix-shell -p sqlite.dev --run \\"make all\\""/g' $(SQLITE3-EL-PATH)
endif
endif
Thanks
7ceee63
to
4a9085a
Compare
I've just released v4.0.0. This includes new, more reliable sqlite back-ends. This older sqlite back-end is still included in this release, but will be removed in the future. I don't intend to fix any issues of this back-end on its way out. |
Hi,
could you please review patch to provide a more informative message when there messages are corrupted or getting out of sync with emacsql-sqlite program. It addresses #115.
The way this is done is for the executable to return a non-zero exit status that it is then examined by the process sentinel, and raises an error with the last comms from the executable. A test is also written to exercise the condition by sending a malformed message.
In addition to this, I've taken the liberty to correctly build the executable with
nix
for binary compatibility with Emacs used in the CI builds with the second commit, and removed the hacks inemacsql-external-tests.el
to work around it. There is also a hack in Makefile to build thesqilte3
module usingnix
(unfortunately I couldn't find any other way to do it other than patching the sqlite3 file).Moreover, I've added an new workflow file to run the tests with a much simpler configuration using https://github.com/emacs-eldev/eldev on MS-Windows. This simplifies the testing quite significantly.
Happy to separate the CI update into another commit if you thinks it is necessary.
Thanks