-
-
Notifications
You must be signed in to change notification settings - Fork 92
chore: Dockerfile use HereDoc syntax
#159
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
- Remove `ENV CARGO_REGISTRIES_CRATES_IO_PROTOCOL=sparse` it's the default since Rust 1.70.0 (June 2023) - Revise the rustup `RUN` and relocate relevant changes associated to it with additional PR references for maintainers as docs. - Handle `PATH` updates properly, remove any redundancy. - Remove `LD_LIBRARY_PATH`, it was misconfigured and seems redundant? - Improve the `protoc` + `sccache` bin retrieval + extraction `RUN` instructions. - Change `zlib` + `sqlite` steps into their own build stages. They each extract the source into their respective `WORKDIR`. No cleanup necessary due to separate stage.
Primary change is rustup `RUN` in it's own separate stage. This may not be as friendly to cache storage? Additionally breaking change that moved rustup and cargo install to `/opt` instead of in `/root`.
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.
Thank you so much for this. The tracing and documentation on all these flags that's been in here for ages is amazing.
I agree with pretty much all of this. Lots of clear errors on my behalf that's great to clear out and document. I was a bit confused by the benefit of the workstage separation, but I see it actually makes my local build able parallelize the stages. (41s build locally on my ryzen).
Yeah, when the image builder understands it can parallelize the build of multiple stages it will do so. It also has the benefit that updating one will not invalidate the other since they don't depend on each other.
I had some additional changes to tackle, but ran out of steam for that today. Seems you merged with the last commit failing the test suite 😓 I'll try tackle that tomorrow for you. |
|
All good, I did some minor follow-ups so CI has been a bit delayed due to early-cancels, but it looks like all previous failures have just been my failure to deal with logins correctly on PRs. EDIT: The most recent |
|
Looks like you tackled everything else I had queued up 💪 In this follow-up commit you replaced there marker from If there's another reason to prefer In this follow-up commit you changed from major pin to major.minor pin of the dockerfile syntax. This is discouraged in the official docs, as you are not setting a minimum AFAIK but locking on that minor series. Better to leave it on the v1 major. |
no, not really. just like the idea of using the common heredoc marker we typically encounter outside docker for some code familiarity. a search for
Ah, thanks! I didn't know that. |
as per note in #159 (comment) Signed-off-by: clux <sszynrae@gmail.com>
This PR updates the
Dockerfileto use HereDoc syntax, as suggested here. This allows forRUNstatements to drop the&& \noise and be easier to grok like a regular shell script.If you review the first commit that should be fairly straight-forward. I've since noticed other portions of the
Dockerfilethat could do with some maintenance, but lack time to stagger PRs for each individually, the bulk of those are in the follow-up commits (might be easier to review separately if you rely on a diff).Additionally:
ENV CARGO_REGISTRIES_CRATES_IO_PROTOCOL=sparsecan be dropped as this is the default since Rust 1.70.0.Variables use
${VAR_NAME}instead of$VARNAME. I've replaced someENVwithARGsince they're only intended for build-time usage and located them by each associatedRUNso that adjusting these won't invalidate all layers.In your recent PR you added some additional
LABEL(the existingmaintainerlabel is discouraged btw), one of these you attempt to set the value with a shell command, but these are string values for assignment. If you assumed you could run a subshell because of${VAR_NAME}usage, that's not a shell ENV but an equivalent feature supported by theDockerfileformat to reference and useENV/ARG. Thus this value literally stores that as a string:Likewise, no need for this when you have
--targetoption forrustup-init:SHELLis defined early on to change the default/bin/shused byRUNinstructions./bin/shin Debian/Ubuntu symlinks to/usr/bin/dash, I've chosen/usr/bin/bashhere which is usually what is expected. On Fedora/bin/shsymlinks to/usr/bin/bashand Alpine to/usr/bin/ashI've removed the comment for
protocversion in Jammy having missing features.protobuf-compileris 3.19.6 (Sep 2022) 🤷♂️PATHwas set twice, while referencing the previous set PATH which set duplicate paths. The final PATH value became:I've resolved that. You also switched from Debian to Ubuntu in 2016, where you prepended
/usr/local/binto PATH, but that should be unnecessary as it's already in the PATH.PREFIXis set in the same ENV instruction asLD_LIBRARY_PATHwhich references it. Thus PREFIX is empty and this likely fails to do what was intended.ENV PREFIXseparately before anENV LD_LIBRARY_PATH.ENV LD_LIBRARY_PATH, you should prefer to use that explicitly as a prefix to commands rather than a global env.LD_LIBRARY_PATHbeing useful, and given that it's been empty since then I've removed that too.In the 2nd commit
zlib+sqliteare now handled in separate stages. I also put rustup into it's own stage in the third commit and shifted your labels to the end (while switching the maintainer label for standardized authors one). This has a breaking change where I migrated cargo + rustup to/optinstead of storing them in/root. You don't need to give access to/roothome folder this way, but if any one used the image while cache mounting or similar with the/rootlocation hard-coded then this might be an unexpected change for them.