-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add Command for Running Emacs in Docker #53
Conversation
In principle this looks interesting. However, I'm unable to even run it as it is:
As someone who has close to zero experience with Docker, I have no idea how to proceed from here. I also noticed that local dependencies don't work at all, as I had Ideally, I think, it would work as
would fetch Docker image and run Also, would be nice if there was a way to make local dependencies work. If this is not possible, maybe Eldev in Docker could receive a flag "ignore all local dependencies and print a warning if any are used". |
Thanks for the feedback.
My guess is that you haven't
I think the last commit should cover this, I cloned datetime and extmap and tried it out with an
Seems to work ok.
Yeah, that makes sense. Hopefully the last commit covers this too, so the command in the example is:
As an aside, is it possible to run |
From this point on it looks generally useful, so I agree to merge this into Eldev. If you wanted to just present a showcase, I can continue from here. Or if you feel like it, you can continue improving this PR yourself. Things to improve and investigate:
Apparently not. I don't see a way in Emacs to forward |
Nice, I've pushed a few commits adding some documentation and a couple of tests (the tests take ~20 secs each due to the docker invocations so I've held off adding more for now - any thoughts on this?) and also addressing some of the ^. I've made the docker command customizable along with allowing a custom function to be set to determine the docker image to use from an Emacs version. Some thoughts on some of the other points:
Yeah I found obtaining the file paths for the mounts to be a bit tricky given you can use relative paths in
See https://unix.stackexchange.com/questions/196677/what-is-tmp-x11-unix. It appears to be fairly standard for X, but yeah would break with Windows. I'm happy to continue working on some of the above. I've un-drafted this PR feel free to merge it if you'd prefer they'd be done in seperate PRs (and you're happy with the new changes + doc!) - I think this would make the most sense if you wanted to investigate some of the above in tandem. Continuing on this PR is fine by me otherwise 🙂 . |
Have no time to review and investigate everything yet, sorry. Will reply again later, maybe tomorrow.
Move them to integration tests (we integrate with remote-provided images here). Or maybe even to their own test type, due to slowness.
Thank you. Please continue on your own for now, I will tell you when I want to merge. I will request you to squash all the commits then. Also, please rebase on |
No problem, whenever you have the time - no rush 🙂
Will do. |
71e0797
to
a2eac2f
Compare
eldev.el
Outdated
(defvar eldev-docker-executable "docker" | ||
"Executable to call when executing docker commands.") | ||
|
||
(defvar eldev-docker-img-fn #'eldev--docker-img-fn |
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 rather had in mind that if user specifies a (Emacs) version, then it gets transformed to image name. And if it already contains '/' or whatever else marks a proper image name from Docker's point of view, it is left untouched. E.g. eldev docker 25 ...
-> use silex/emacs:25-ci-eldev
, eldev docker me/image-1
-> use me/image-1
.
eldev.el
Outdated
(defun eldev--emacs-docker-local-dep-mounts () | ||
"Return bind mount arguments of local dependencies for docker run." | ||
(mapcan (lambda (local-dep) | ||
(let* ((dir (cadddr local-dep)) |
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.
AFAIR, cadddr
is too new, we should avoid such things. Use nth
.
eldev.el
Outdated
"Return bind mount arguments of local dependencies for docker run." | ||
(mapcan (lambda (local-dep) | ||
(let* ((dir (cadddr local-dep)) | ||
(home (getenv "HOME")) |
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.
As far as I see from Emacs source code, (expand-file-name "~")
is the "standard" way to compute home directory.
eldev.el
Outdated
(let* ((dir (cadddr local-dep)) | ||
(home (getenv "HOME")) | ||
(container-dir | ||
(if (string-prefix-p home dir) |
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 still find it somewhat dirty to use string functions directly here, maybe because I'm mostly a Java programmer. Apparently, Elisp doesn't provide a lot of functions for this. But e.g. in eldev-filter-files
I used a combination of file-relative-name
and home-grown eldev-external-or-absolute-filename
.
eldev.el
Outdated
:custom-parsing t | ||
(unless (car parameters) | ||
(signal 'eldev-wrong-command-usage `(t "version not specified"))) | ||
(unless (executable-find eldev-docker-executable) |
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.
Please create also a function with this name, like e.g. function eldev-git-executable
. The function would verify presence, cache and so on.
eldev.el
Outdated
"Return an appropriate ci-eldev image based on TARGET-VERSION." | ||
(format "silex/emacs:%s-ci-eldev" target-version)) | ||
|
||
(defun eldev--emacs-docker-local-dep-mounts () |
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.
Rename to just docker
everywhere, not only the command.
test/docker.el
Outdated
|
||
(defvar eldev--emacs-docker-version "27.2") | ||
|
||
(ert-deftest eldev-emacs-docker-emacs-1 () |
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.
Since these tests are so expensive, drop this one, second is enough. Maybe instead add a test for a non-trivial Eldev command, like e.g. test
in project-c
(two tests, all must pass, even when run inside Docker).
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 switched to running test
in project-c
as suggested. It succeeds for Emacs >= 26, but fails below atm unfortunately, still debugging why.
Edit: FYI here is the error https://github.com/LaurenceWarne/eldev/runs/4019397102?check_suite_focus=true
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.
Update: eldev clean all
previous to running the test, appears to fix this problem. This points to some kind of problem with mounting the global cache when the host Emacs is not the same as the container Emacs, what do you think?
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.
Global cache is not cleared with eldev clean all
, you have to explicitly invoke eldev clean global-cache
. So, this likely means that global cache is irrelevant. Also, it includes non-compiled packages, so Emacs version mismatch shouldn't play any role. Maybe it is another instance of tests subtly depending on previous tests (which is, of course, an error, but one difficult to hunt down), see if eldev clean test-caches
"solves" the problem.
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 sorry, I saw Run all available cleaners
and my eyes glazed over 😄.
see if eldev clean test-caches "solves" the problem.
It appears no?: https://github.com/LaurenceWarne/eldev/runs/4044954229?check_suite_focus=true
test/docker.el
Outdated
"--batch" | ||
"--eval" | ||
`(prin1 (+ 1 2))) | ||
(should (string= (substring (string-trim-right stdout "\n*") -1) "3")) |
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 the way, I think it would be better (if possible) to send Docker's own output to stderr, so that stdout contents would be the same regardless if run under Docker or not. Or have I misunderstood what you are doing here?
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.
It's probably me misunderstanding something, but isn't the response 3
output by docker's stdout anyway? - So there's no way to differentiate it?
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 forgot that "*" is a regexp operator here, not a verbatim character. Still, it looks like you are putting too much effort into discarding whitespace here. E.g. without using Docker, with simple eldev eval "(+ 1 2)"
, I always get predictable "3\n" on stdout. Can this be guaranteed with Docker too?
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.
Something is adding an extra newline somewhere I'll try and see where. This is slightly nicer in a recent commit since I found (string-trim-right stdout "\n*")
is equivalent to (string-trim-right stdout)
, the former is not Emacs 24 compatible either!
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.
Think I've figured it out, eldev-output
is called both in the container and on the host, adding one newline too many. So precipitating :nolf
on the docker commands fixes it. The only difference in the stdout now is Bootstrapping Eldev for Emacs ...
, which doesn't appear to respect --quiet
? Though I guess this will be not be present once the tests use the Eldev being tested.
a2eac2f
to
98da69f
Compare
What's currently TODO:
QQ: Is there a standard way to install Eldev from my local git repo? - atm I'm just copying files to the relevant place in |
|
||
(setq eldev-docker-run-extra-args | ||
`("-v" ,(concat (expand-file-name "../package-archive-a/") | ||
":/package-archive-a"))) |
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've added a new variable that can be used to provide custom args to docker run
, using it here to mount a required package archive.
The path of the archive is available to eldev at runtime through package-archives
, but the path this provides is not useful in this case since the container will expect it to be in ../package-archive-a
-> /package-archive-a
. But we only get the absolute path at runtime (/foo/bar/eldev/test/package-archive-a
) from which we can't deduce the expected directory of ../package-archive-a
, I hope that makes sense.
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.
Seems fine. As you said, we need something like this for tests, but it probably should be public from the beginning, because I could imagine legitimate uses for it we cannot cover automatically.
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'll expand on the doc, adding this and other stuff which probably should be added.
Yeah, I already ran into this too. However, could there be a better solution? As I understand, if we do it with |
Not sure if it would be enough, but have a look at |
Some more thoughts:
|
I created antoher image: LaurenceWarne@aadfcd7, it works by creating another user with the same uid on the container as the host, and running with that user on the container (inspired by http://www.inanzzz.com/index.php/post/dna6/unning-docker-container-with-a-non-root-user-and-fixing-shared-volume-permissions-with-gosu and https://github.com/daniccan/docker-gosu). It lgtm testing, in particular it solves the permissions errors on the test runs. lmk what you think of this - I'm not sure where it would live (I don't think it should be part of the default |
6260c6b
to
d109a8c
Compare
Uh, is there no easier way? I also searched for this on the Internet briefly, and haven't found anything simple. But is it really not possible to go without a specialized image? This would effectively remove the option to use user's own image (i.e. what we discussed before, either VER -> silex's image, or IMAGE-NAME, for a custom one), or at least require that they base on "our" image somehow...
Yeah, I thought about negotiating with silex to include this into his images, but now I see that there are quite a few things going in it. |
I've just thought of a horrific hack to get away with using the same image (or any image with an Emacs installation): the problem with using
This appear s to work, though seems a bit fragile, and we'd have to mount the global cache and config inside this new home, etc. I don't know if I'd call this an "easier way", but wdyt?
We'd have to install docker on the images also 😄 |
If you have looked at Eldev code enough, you must know that I'm not against hacks. The source code is full of hacks and workarounds. Whatever works, unless it really works "by side effect only" or is likely to break with any small future change in the program we are integrating with here (i.e. Docker).
As it is looks fine to me. I'm only not sure I understand the meaning of "mount the global cache and config inside this new home, etc.". This fake root should be placed somewhere inside
It is easier if we don't have to create and maintain (remember about rebuilding when a new Emacs version comes) extra images. And even better if it allows users to use their own images. I don't care if it results in 10, 30 or 100 more lines of Elisp code.
Crap, didn't think about that. Oh well ;) |
Cool, it looks to have fixed the permissions on CI (lmk if you encounter any more).
Just that we have to change the target mount points on the container, e.g. As suggested, the "fake home" on the host is created in Edit: Also we don't use eldev on the eldev-ci images so we can use plain old Emacs images. TODO
|
It can stay if it contains anything useful that could speed up future Docker runs. Otherwise delete it after all runs. However, future runs must not fail if the directory didn't get deleted for whatever reason. Also, no need to rename it with a dot, because cache directory is already hidden (it is that
Agreed. Also wanted to mention, but I'm too late.
This is very important. Basically, when testing, I want to use Eldev that I'm developing right now for all tests. If I break
This we can forget because of the hack. I guess it would be even better to add an explicit test for |
Looks like CI is broken again, this time on macOS. Let's wait for a while. |
I've set it up so it's deleted after runs. But there should be no problem if it happens to exist prior to a run.
Cool the last few commits should handle this.
If you're ok with that, I've disabled the tests for windows. Though I've just noticed that on GH actions, mac os appears not to have docker installed 😞 |
eldev-util.el
Outdated
@@ -1091,16 +1102,17 @@ Also, eat up several options from BODY if present: | |||
(signal 'eldev-error (list "%s exited with error code %d" (eldev-message-upcase-first description) exit-code)))))) | |||
,@(or body '(exit-code))))))) | |||
|
|||
(defun eldev--forward-process-output (&optional header-message header-if-empty-output only-when-verbose) | |||
(defun eldev--forward-process-output (&optional header-message header-if-empty-output only-when-verbose nolf) |
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.
Actually, it should always work as if nolf
was specified, even for existing invocations. Drop the parameter, just change the function. For eldev-verbose
it should stay as it is, just in case the invoked process doesn't print ending newline. But for eldev-output
we should just pass generated output without changes, not even supplying trailing lf
.
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 just to double check, the function should look like this? :
(defun eldev--forward-process-output (&optional header-message header-if-empty-output only-when-verbose)
(if (= (point-min) (point-max))
(when header-if-empty-output
(eldev-verbose header-if-empty-output))
(when header-message
(eldev-verbose header-message))
(if only-when-verbose
(eldev-verbose "%s" (buffer-string))
(eldev-output :nolf "%s" (buffer-string)))))
In which case, am I ok to fix the existing tests which expect a newline from this fn, e.g. eldev-emacs-2
?
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.
Yes. Adjust all tests that depend on this accordingly, they are not meant to be set in stone.
eldev.el
Outdated
@@ -111,6 +111,9 @@ instead.") | |||
"Name of Eldev cache subdirectory, `.eldev'. | |||
See also function `eldev-cache-dir'.") | |||
|
|||
(defconst eldev-global-cache-directory-name "global-cache" |
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.
Just use -dir
instead of -directory-name
, for consistency with e.g. eldev-cache-dir
. I don't like to abbreviate too much, but sometimes names simply become too long otherwise.
"OS %s is not currently supported by \"eldev docker\"" | ||
"Error message format string if the os is not supported.") | ||
|
||
(defun eldev--container-bootstrap-cmd-fn (eldev-args) |
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.
Can we replace bootstrapping with copying the script file (it is now always bundled with Eldev, so simple locate-file
call should do)? While this sort of implies that we depend on extact target's OS, it's not like bootstrapping commands are cross-platform either. As an added benefit, we'd have "test the source code, not something published" here too, even if for bootstrap scripts it rarely matters.
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.
👍 should be done in the new commit.
(eldev--container-bootstrap-cmd-fn | ||
(if (getenv "ELDEV_LOCAL") | ||
(apply-partially | ||
#'eldev--container-eldev-source-install-cmd "/eldev") |
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.
Is this correct? I don't see the result of (getenv "ELDEV_LOCAL")
being used anywhere. Maybe I just don't follow the logic.
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.
The function eldev--conatainer-eldev-source-install-cmd
is only concerned with the location of the eldev source on the container and assumes that it's mounted in /eldev
. In this commit, I was using (getenv "ELDEV_LOCAL")
as an indicator and calling it again when the mounts are determined, but this is lazy, the most recent changes have it so that (getenv "ELDEV_LOCAL")
is called only once and its value passed through.
Yes, fine. Also fine to skip tests when Docker is not found, and as I understand it, your code does just that already. Please rebase on |
Please address the latest review (four points) and we can finally merge this. Before merging, please squash everything into one commit. Or, if you want, break out stuff that is not related to Docker per se, e.g. there could be a separate commit that adds Also, please remove changes to |
Ah, and also please rebase on |
2693c53
to
e701b6a
Compare
(should (string-suffix-p "3" stdout)) | ||
(should (= exit-code 0))))) | ||
|
||
(ert-deftest eldev-docker-emacs-2 () |
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've added a new test against Emacs 24
, I thought it might be useful to test against the oldest supported version. Let me know if you think this is overkill and I'll remove it.
"--batch" | ||
"--eval" | ||
`(prin1 (+ 1 2))) | ||
(should (string-suffix-p "3" stdout)) |
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.
The stdout is exactly the same as the command w/o docker if the user has the image before hand, but on CI the "not found locally, pulling bla..." will be on the start of stdout. If you don't like this I can see what can be done, maybe filtering it out in the forwarded output or something.
Done! I've been lazy and stuck everything all in one 🙂 Thanks for your patience.
No problem, I can PR with the doc changes I have locally if you like. Edit: also on the mac os stuff, I think (unable to test) that the commands might work if Emacs isn't launched as a GUI, but mac os doesn't use X (?) so I'm guessing the GUI stuff won't work. |
Hm, the automated tests on GitHub servers pass, but I have problems running anything locally. Neither from Eldev project itself:
nor from another, with a different error:
Am I doing something wrong? |
Ah, apparently it's caused by Also, after working around this I still get yet another error:
|
Add the 'docker' command for linux systems. It takes as arguments an Emacs version along with an eldev command and its arguments, and runs the eldev command in a container running that Emacs version.
e701b6a
to
a12d3eb
Compare
Weird, I was using
For some reason I got it into my head that |
Thank you, now it works, so merged! I massaged it a bit before committing, mostly the docstring of the new command. I will release 0.10 with this new command and some testing support improvements (most important are already committed) next week.
Rather thank you. All the time I was afraid you'd say: "Fuck it with all the requests, I'm done here" ;). Then I'm not sure if I would come up with the clever hack for user mismatch and "file created by root" problem caused by it. |
Awesome! Thanks again for your time and quick responses.
This made me laugh 😄 |
Hi! I've been playing around a bit for a way to run my projects with different Emacs versions without having to have multiple Emacs installations and/or something like evm.
I thought it might be useful to take advantage of the existing eldev docker images on https://hub.docker.com/r/silex/emacs to do this. Demo:
emacs-docker-demo.mp4
Essentially the command just runs an appropriate docker image and then executes
eldev emacs
with the rest of the passed args. Would you be interested in this? - No problem if you think this would be more appropriate as a plugin or something.I've left out automated testing and documentation, happy to add them (to this PR) if you're interested in this change.