-
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
Refactor images #24
Refactor images #24
Conversation
247653e
to
5019b73
Compare
Looks good to me, I like all the changes so far and I don't think this will break any of my builds. Nice! :) |
Hum. I just realised multi-stage builds allow me to shave ~170MB from the image. Basically, remove the source from the layers! The trick is to copy the source in an early stage, build there and then copy the build artefacts. So, looks like we are back to |
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 left a few comments in the code... thanks
25.1/xenial/Dockerfile
Outdated
rm -rf /tmp/emacs | ||
|
||
#-------------------------------------------------------------------------------- | ||
|
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.
If you're going to do this, then perhaps you should keep the emacs sources in the first stage & tag that image separately for developers who want to inspect exactly how Emacs was compiled. The way docker manages layers there is no additional cost because, as you've discovered, the rm -rf /tmp/emacs
didn't actually cause the image to be smaller anyway.
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.
Interesting, do you think there's a need tho? I will already have quite a lot of tags... If things go well the -alpine
version will come, and if I follow your idea this will add another variant, so we'll have:
- dev (with /tmp/emacs)
- normal (without /tmp/emacs)
- slim (without /tmp/emacs and without
-dev
packages) - alpine (from alpine, without /tmp/emacs and without
-dev
packages)
If you look at https://github.com/Silex/docker-emacs/blob/refactor/README.md, the list is decently long... add 24.3
and 24.4
, then double that list because of -alpine
and -dev
. This becomes a wall of tags :-) But maybe that can be solved with better formatting.
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.
Well the other alternative is to encourage the “slim” builds, which are guaranteed to be even smaller than this. Just dev, slim and alpine.
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.
@gonewest818: I like your idea! tags 25.3-dev
, 25.3
and 25.3-alpine
25.1/xenial/slim/Dockerfile
Outdated
@@ -0,0 +1,74 @@ | |||
FROM ubuntu:16.04 | |||
|
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.
Assuming this first stage is identical to the first stage of the non-slim build, then you don't need this repetition. You can use a single Dockerfile and just run the build stages individually and tag them individually.
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.
True, but then you lose the "1 image = 1 dockerfile" spirit that makes the README a great way to quickly visualize what is going on.
Except if you meant COPY --from=silex/emacs:25.3 /opt/emacs /opt/emacs/
, but I don't think this is allowed. I need to try tho.
EDIT: woa, COPY --from=silex/emacs:25.3
is allowed! That's so nice!
That means we can build once and then derive all the other images from the root one. I'll need to think about 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.
Yes, that does work and in particular for the situation I just commented on, where the builder images are named differently to keep the namespace clean.
25.1/xenial/Dockerfile
Outdated
|
||
# Install Cask | ||
ENV PATH="/opt/emacs/bin:/root/.cask/bin:${PATH}" | ||
RUN curl -fsSL https://raw.githubusercontent.com/cask/cask/master/go | python |
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'd like to see a variant without Cask and python for people like me who don't want or need them in the image.
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.
That'd make 5 variants! :-) Wait no, 8 variants! Each of dev, normal, slim, alpine would need a -cask
variant.
Right now it feels a bit too much, especially since python/cask does not really add much MB or hurt when present.
Thank you for the feedback/review! I'll ask what other thinks about your points on the issue itself.
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 seen projects separate the build images and the “run” images into separate namespaces so that the naming isn’t so cluttered.
So for instance most users would never be concerned with the dev images:
Silex/docker-emacs-builder:25.3 (this is the “dev” image)
But they do select either the emacs-only images
Silex/docker-emacs:25.3 (this is “slim”)
Silex/docker-emacs:25.3-alpine (“slim+alpine”)
Or the ones with cask too
Silex/docker-emacs-cask:25.3 (this is “slim+cask”)
Silex/docker-emacs-cask:25.3-alpine (“slim+alpine+cask”)
You might find the Alpine images are small enough that adding python is significant.
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.
Again I like your idea! It makes sense. I'll start by creating all the images in this repo and then split the repo in 3 when the time is right.
Thank you a lot 👍
@gonewest818 suggest we have What do you think of these ideas? |
Okay, for now I see several possibilites. One namespaces, all possibilities:
Multiple namespaces, all possibilities:
One namespace, only the needed ones:
I think I prefer the last one for simplicity reasons. My rationale is that if you want the |
I still prefer separating the namespaces to help guide users to the right builds. Also, personally I don't care about the presentation of images on docker hub. I think github is a more effective way to promote the images, allow for better "discovery", provide more detailed documentation, etc. That's why in earlier commits I didn't worry about reorganizing the Dockerfiles and markdown in ways that wasn't perfectly compatible with the docker hub site. Ultimately though, I understand my priorities are different than yours, perhaps dramatically so, and fully acknowledge your approach is fine for normal use. About the dev-alpine variant -- I think it would be useful to have base images that contributors can use as the starting point for other build variations. Compiling |
I need to think more about it, because if it's all automated it's not really a problem to have 3 repos, especially since today I discovered I can automate the README update on the hub with a dummy branch.
Ah, good to know the portable dumper discussion is back from the dead... we'll see where that goes. Your dev-building-blocks idea is very interesting, it immediatly got me thinking into EDIT: hum, maybe |
What is the status of this PR? |
@DamienCassou: brewing. Most commits here will be rewritten. When it's done, we'll have "normal" (what is called I'm still debating wether alpine/cask images go in a repository on their own. At the moment I'm working on including a "patch" phase to the emacs source in order to properly build the alpine images, and how the final |
cac0b03
to
b6d561f
Compare
Alright. To solve the design issues I did some README-driven development 😄 I arrived at two possible ways:
Here are the tradeoffs for option 1:
Here are the tradeoffs for option 2:
I'm slightly in favor of option 1 because simplicity wins in my book. @gonewest818, @jgkamat, @DamienCassou: voice your opinions 😉 |
I personally prefer option 1, option 2 is too overloaded in my opinion (and confusing which one I'd want, too many choices :P). I don't think having missing In short, I don't think the little benefit gained from having every image possible is worth the build complexity and the information overload when picking images. If someone wanted that much specificity, they should just build their own image |
b4cc64d
to
af540ad
Compare
Okay, I'm almost merge-ready. Unfortunately, one of my plan failed miserably, which was to be cute and have docker-emacs/master/xenial/Dockerfile Line 27 in bba48df
I had to create a
Which basically means I did pretty much all this for nothing 😅 I'll keep this work for future refactoring in some branch, might be useful some day. I'll just keep the EDIT: actually |
92c69ce
to
1c8df90
Compare
Merging as soon as the travis-build is successful 😄 |
@gonewest818, @DamienCassou, @jgkamat
For your information, I'll merge this soon. The main changes that might concern you are:
WORKDIR
is gone./opt/
anymore.-slim
image are build differently.That was made so
-alpine
images will come soon.This PR is mainly so you can protest against a change for whatever reason you might have, and that we can find a solution together. Without any news from you in the next days I'll just go and merge it.