docker-library / ruby

Docker Official Image packaging for Ruby
http://www.ruby-lang.org/
BSD 2-Clause "Simplified" License
590 stars 334 forks source link

Update 3.2.0-rc to 3.2.0 #399

Closed frederikspang closed 1 year ago

frederikspang commented 1 year ago

https://www.ruby-lang.org/en/news/2022/12/25/ruby-3-2-0-released/

stillhart commented 1 year ago

Isn't rust as a dependency missing? Otherwise we can't use YJIT.

sdwolfz commented 1 year ago

@stillhart It's included already:

yjit=; \
    apkArch="$(apk --print-arch)"; \
    case "$apkArch" in \
        x86_64 | aarch64) yjit=1 ;; \
esac; \

Then further down:

${yjit:+rust} \
nickjj commented 1 year ago

@sdwolfz is there a path forward where the Debian based images can have YJIT support?

At the moment it looks like Alpine is the only option due to Debian not having a more modern version of Rust in its apt repo. The next Debian stable release is still quite a ways off (at least 6-8 months based on their roadmap and prior release history).

Perhaps the Debian base image could be altered to install a custom version of Rust instead of apt installing it? The official Rust docs include installers for a bunch of different systems at https://forge.rust-lang.org/infra/other-installation-methods.html#standalone-installers. There's also a shortcut script to auto-detect the platform at https://forge.rust-lang.org/infra/other-installation-methods.html#other-ways-to-install-rustup.

frederikspang commented 1 year ago

@nickjj See also https://github.com/docker-library/ruby/issues/390 and https://github.com/docker-library/ruby/pull/398

This pull request does not introduce new features, other than updating the Dockerfiles for 3.2.0 release from 3.2.0-rc1.

tianon commented 1 year ago

Would you mind also updating https://github.com/docker-library/ruby/blob/9b27face61a92f7e77c069fae18abeb55aca3a90/generate-stackbrew-library.sh#L5 to [3.2]='3 latest'? (I'll get to it eventually if you don't :bow:)

tianon commented 1 year ago

CI is almost done - I'll correct that post-merge so we don't have to reset CI :+1:

tianon commented 1 year ago

Thank you!

nickjj commented 1 year ago

@frederikspang Thanks, I did see those issues and PRs previously. It's why I suggested installing a non-apt version of Rust to avoid waiting until Bookworm is available.

dbackeus commented 1 year ago

At what point do the new images become available at https://hub.docker.com/_/ruby/tags?

(sorry if I'm being impatient 😅)

frederikspang commented 1 year ago

@dbackeus Should be there now!

invalidusrname commented 1 year ago

Looks like the images become available when https://github.com/docker-library/official-images/pull/13806 gets merged

sdwolfz commented 1 year ago

@sdwolfz is there a path forward where the Debian based images can have YJIT support?

Only @tianon can realistically answer this for you.

ianks commented 1 year ago

Not including yjit would be a huge mistake here. We’ve seen 10% improvements in response times across the board on Shopifys biggest production apps. It’s the killer feature of 3.2.

YJIT can build with rustc >= 1.58 which is in many distros at this point. However, I think installing a temporary rust toolchain (with either stable or 1.66) is the way to go for more consistency across builds.

Happy to help get this integrated if needed.

frederikspang commented 1 year ago

@ianks I agree.

wget -O- https://sh.rustup.rs | sh -s -- -y
. "$HOME/.cargo/env"

<install stuff here>

rustup self uninstall -y

would almost do it for debian builds, no?

nickjj commented 1 year ago

Based on the docs at https://rust-lang.github.io/rustup/installation/other.html there's:
curl https://sh.rustup.rs -sSf | sh -s -- --profile minimal --component rustc -y

This finishes installing in about 1 second and rustc is available on your PATH, it installs the latest stable version which you can verify with:

$ rustc --version
rustc 1.66.0 (69f9c33d7 2022-12-12)

I never installed or used Rust directly but does the YJIT only need rustc? https://rust-lang.github.io/rustup/concepts/profiles.html mentions what the minimal profile does. It includes rustc and a few other things (rust-std and cargo).

Even with --component rustc the cargo binary is available. Maybe additional cleanup can be done in a single RUN layer to only leave rustc available if that's all we need?

frederikspang commented 1 year ago

Something along these lines? https://github.com/docker-library/ruby/commit/66facd305aca0db3f4642cb911163cc0e46bb3cc

Obviously with some adjustments, and not for all ruby versions etc - And the condition of using is_debian isn't super great when should_yjit exists already.

But overall I mean.

If the maintainers (@tianon) is up for it, I can take a look at preparing a PR for this within the next few days! (New Years etc. first 😄 )

sdwolfz commented 1 year ago
wget -O- https://sh.rustup.rs | sh -s -- -y

Unfortunately something like this will never be accepted as it's a catastrophic security issue. You need to have a sha256sum check of anything you download form the internet in any automated pipeline.

The current docker images do this for the ruby downloads:

wget -O ruby.tar.xz "https://cache.ruby-lang.org/pub/ruby/${RUBY_MAJOR%-rc}/ruby-$RUBY_VERSION.tar.xz"; \
echo "$RUBY_DOWNLOAD_SHA256 *ruby.tar.xz" | sha256sum --check --strict; \

It might be accepted if sh.rustup.rs will provide a versoned file, that file never changes, so the sha256sum doesn't, and you check the sha during image build.

For reference, here's a recent proof of the security issue:

And in the future, please stay away from anything that does wget/curl into sh/bash as you're not only compromising your own machine, but potentially the intellectual property of the company you're working for.

ianks commented 1 year ago

It might be accepted if sh.rustup.rs will provide a versoned file, that file never changes, so the sha256sum doesn't, and you check the sha during image build.

Good news, this is possible today! So something like the below should work.

export CARGO_HOME="/tmp/cargo"; \
wget -O rustup-init https://static.rust-lang.org/rustup/archive/1.25.1/x86_64-unknown-linux-gnu/rustup-init; \
echo "5cc9ffd1026e82e7fb2eec2121ad71f4b0f044e88bca39207b3f6b769aaa799c *rustup-init" | sha256sum --check --strict; \
chmod +x rustup-init; \
./rustup-init --no-modify-path --profile minimal -y --default-toolchain 1.66; \
export PATH="$CARGO_HOME/bin:$PATH"; \
cargo --version;

One thing to keep in mind is that since this is a precompiled binary, we'll have to either:

  1. Keep a map of docker-platform => (rust-target, rustup-sha256)
  2. Rely on binfmt_misc with amd64 emulation being available during the build so we can just always download the x86_64-unknown-linux-gnu version, and run that. I think this should work, but haven't tested myself yet.

cc @frederikspang

sdwolfz commented 1 year ago

@ianks You're half way there. Now the rustup-init has to respect the same rule: the packages downloaded and installed by it need to either do a sha check, or gpg signature verification, or equivalent check to make sure it's installing rust in a secure way.