-
Notifications
You must be signed in to change notification settings - Fork 52
Do more at once to make image smaller #8
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
Conversation
woops, I forgot to remove the block following what I did. I'll fix it if I can get a +1 from a maint (and apply the change to the other builds.) |
&& TEST_JOBS=$(nproc) make test_harness \ | ||
&& make install \ | ||
&& cd /root | ||
&& rm -fr /usr/src/perl |
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.
You probably want to remove the succeeding RUN
line if this were the case.
Alternatively, I think we should ought to put the command chain on this RUN
into its own script and just put in arguments to what perl to build and how many procs, like e.g.
COPY build-perl.sh /tmp/build-perl.sh
RUN sh -x /tmp/build-perl.sh https://cpan.metacpan.org/authors/id/S/SH/SHAY/perl-5.20.1.tar.bz2 $(nproc)
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.
COPY has depressing Docker cache behavior, so I'd recommend sticking to the
multiline RUN even though it's not exactly pretty
I like the idea of bundling it into a build script instead of mixing it together |
Err, running between tasks, that was a little briefer than I meant. Anyway, +1 to getting rid of build debris in whatever way's best |
Ok, I'll send a fresh set of changes tomorrow with a single script called by RUNsent from a rotary phone, pardon my brevity
|
I really, really recommend you don't, especially since there's already a Line 109 in c7af0df
If you make a separate script just to build Perl, you'll have to copy it |
I would also recommend against a file that contains these commands as that would obfuscate the build for a user who pulls the image and does a |
Ok, I'll tweak the script that generates them. sent from a rotary phone, pardon my brevity
|
Maybe outside this PR but @tianon can you shed more light on that COPY cache semantics issue? @yosifkit as you said it, the file is in the image, so the user could just do a |
Ok, I refactored this to be done in the right place. Again, as far as I can tell this saves about ~100M of space. I am a little bummed it's not more, but buildpack-deps is pretty fat as is. |
+1 ❤️ |
As for fatness, we've actually got several images that have a "slim" variant for people who don't want as much included by default: https://github.com/joyent/docker-node/blob/master/0.10/slim/Dockerfile The basic idea being that it's roughly the same as the default |
Yeah, I'm of two minds about slimmer variants. On the one hand, it means that if you only use perl it's nice and small, but on the other hand, if everyone uses buildpack-deps, it's a shared root that can save some space. But I've seen enough churn in buildpack-deps that I don't think it's really worth it. |
@frioux There should not be any more churn in buildpack-deps aside from security updates in the base debian images. We have it at a nice spot. |
Do more at once to make image smaller
This is just a PR to get a conversation going. When I made this change my build reduced by more than 100M. Is this something that we can do? Is there a reason to leave the old stuff behind?