-
Notifications
You must be signed in to change notification settings - Fork 192
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
Ubuntu 20.04 #120
Ubuntu 20.04 #120
Conversation
Running
As you also mention in #98. But I think that might be a red herring. Here's an output of another binary (randomly chosen from /usr/bin) which does seem to be not fully static as it needs a libc on the system:
Interwebz says that we should also check the output of
using-sqlx has no INTERP there. While …
… does have it, and you can even see a path encoded there. From https://greek0.net/elf.html:
Another way to check is to run And yet another option is to use scanelf from pax-utils ( I also ran an example app in different variations of busybox:
And there are no problems, which I would expect if it was not truly static. Even Fun fact: ldd on alpine/musl is the following shell script:
So I believe that this lib/executable is injecting itself into the ldd output. Last but not least I ran it under a
(debian's ldd is a slightly more elaborate script.) I adjusted the test-image script and the Dockerfiles of both static tests to use debian:stretch-slim for the better ldd checker. Under asaaki#1 you can see my travis runs, which are green for the proposed changes. |
I've tested this updated image to build the I wouldn't mind seeing this reviewed and merged (notable change: the updated EDIT: worth mentioning that perhaps openssl could also be safely upgraded to latest |
@jman-schief Updated to have 1.1.1k used. |
Also updates various dependencies and tools. Further changes (squashed commits): * Use COPY over ADD If you copy local files, then COPY is preferred. ADD should only be used in specifc circumstances like fetching files from URLs or unpacking archives. * Use a more reliable testing for ldd * Use openssl 1.1.1m, move cargo tools
Many thanks to @asaaki for having figured out the static link check was returning flase negatives with Alpine's `lld`. See: #120 (comment) The plan is to merge this when Rust 1.58.0 is released, so people who need 18.04 can stick with1.57.0. Closes #98. Closes #120.
Thank you to everyone for working on this, and to @asaaki for figuring out the static linking check! I've prepared a new PR inspired by this one but based off the latest I plan to release this when Rust 1.58.0 is released, to allowing existing users to easily stick with 1.57.0 if they want. |
For some of the tiny tweaks and consistency changes do you want me to open new PRs after you merged your update PR? (btw this one was already rebased to latest main, I can also undo some changes you don't like to make it into a mergeable state) |
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 for proposing more fixes! That would be very helpful.
To save you some time, I read through your PR, and I marked the fixes I'd definitely like to add to this project! In general, I'm happy to merge small, clean PRs that do a single thing and don't change anything unrelated. If there's even a single part of a PR that I don't want, it makes the whole process a lot more time-consuming for on my end.
In this particular case, please feel free to submit a single new PR with just the changes I've accepted below. Thank you very much for figuring out 20.04, and for the other fixes you've proposed.
# syntax=docker/dockerfile: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.
We can add this, since Docker uses it in all their latest examples.
# - `mdbook` is the standard Rust tool for making searchable HTML manuals. | ||
# - `mdbook-graphviz` allows using inline GraphViz drawing commands to add illustrations. | ||
# - `cargo-about` generates a giant license file for all dependencies. | ||
# - `cargo-audit` checks for security vulnerabilities. We include it for backwards compat. | ||
# - `cargo-deny` does everything `cargo-audit` does, plus check licenses & many other things. | ||
RUN curl -fLO https://github.com/rust-lang-nursery/mdBook/releases/download/v$MDBOOK_VERSION/mdbook-v$MDBOOK_VERSION-x86_64-unknown-linux-gnu.tar.gz && \ | ||
tar xf mdbook-v$MDBOOK_VERSION-x86_64-unknown-linux-gnu.tar.gz && \ | ||
mv mdbook /usr/local/bin/ && \ | ||
rm -f mdbook-v$MDBOOK_VERSION-x86_64-unknown-linux-gnu.tar.gz && \ |
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 leave this whole section at the top of the file. Thank you!
RUN apt-get update && \ | ||
export DEBIAN_FRONTEND=noninteractive && \ | ||
RUN export DEBIAN_FRONTEND=noninteractive && \ | ||
apt-get update && \ |
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 leave out any minor changes that don't change anything, unless the current version is clearly inelegant.
RUN echo "Building OpenSSL" && \ | ||
RUN echo "Building OpenSSL ${OPENSSL_VERSION}" && \ |
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 think it's a great idea to add the version numbers!
As for variable syntax, according to the spec:
Environment variables are notated in the Dockerfile either with
$variable_name or $ {variable_name}. They are treated equivalently and the brace syntax is typically used to address issues with variable names with no whitespace, like ${foo}_bar.
This project prefers $variable_name
when unambiguous, and only uses ${variable_name}
when the next character would otherwise be interpreted as part of the name. Let's use $variable_name
here and elsewhere.
ADD git-credential-ghtoken /usr/local/bin/ghtoken | ||
COPY git-credential-ghtoken /usr/local/bin/ghtoken |
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.
OK, it looks like COPY
is officially preferred, so we can use it when it works.
RUN curl https://sh.rustup.rs -sSf | \ | ||
RUN echo "Installing Rust (${TOOLCHAIN} toolchain)" && \ | ||
curl https://sh.rustup.rs -sSf | \ |
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.
👍 for logging the toolchain.
# if we also install other target's build dependencies (like tools, libs, and linkers) | ||
# rustup target add x86_64-unknown-linux-musl \ | ||
# rustup target add aarch64-unknown-linux-musl \ | ||
# rustup target add armv7-unknown-linux-musleabihf \ | ||
# rustup target add armv7-unknown-linux-musleabi |
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.
We are definitely not going to try to support multiple targets in a single image. And we won't be shipping any targets that can't link diesel
and sqlx
. I tried this in the past and it was maintenance headache and backwards compatibility guarantee I didn't want.
Honestly, people should use cross
unless they need C libraries that are not supported by cross
. I've already converted as many projects as I can, and it's great. (But it won't work for everything.)
|
||
# Now, we need to build our _real_ Docker container, copying in `linking-with-git2`. | ||
FROM debian:stretch-slim | ||
RUN apt-get update && apt-get install -y ca-certificates | ||
COPY --from=builder \ | ||
/home/rust/src/target/x86_64-unknown-linux-musl/debug/linking-with-git2 \ | ||
/usr/local/bin/ | ||
CMD /usr/local/bin/linking-with-git2 |
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.
This is probably unnecessary for libgit2
. We have other examples anyways.
FROM alpine:latest | ||
RUN apk --no-cache add ca-certificates | ||
FROM debian:stretch-slim | ||
RUN apt-get update && apt-get install -y ca-certificates |
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.
All images should continue to use Alpine, now that we do ldd
checks on the host.
Many thanks for figuring out ldd
, and for preparing this PR!
# Needs evaluation; | ||
# fails for libpq with: | ||
# "configure: error: library 'crypto' is required for OpenSSL" | ||
# ARG OPENSSL_VERSION=3.0.0 |
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.
Any support of OpenSSL 3.0.0 should happen in a separate PR, and it should wait until all the examples in the test suite support dynamic linking of OpenSSL. (They may do so already.)
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.
We'll also need to consider how many existing users this breaks in the short term. We try to avoid breaking working CI setups, though in a 6-year-old project, that occasionally happens to people who pin to :stable
instead of a version.
Use latest ubuntu LTS release (20.04)
Also:
Travis CI checks can be found under my PR of the fork: asaaki#1
If you want to use the image already:
FROM ghcr.io/asaaki/rust-musl-builder:latest
Closes #98