-
Notifications
You must be signed in to change notification settings - Fork 31
Install rust and enable yjit #40
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
Dockerfile
Outdated
$(cat /tmp/ruby_build_deps.txt) && \ | ||
apt-get clean && rm -r /var/lib/apt/lists/* | ||
|
||
RUN set -ex && \ | ||
useradd -ms /bin/bash ubuntu | ||
|
||
RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y |
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.
Two things:
- I think YJIT doesn't require a particularly recent Rust, so we might be able to install the relevant debian package instead of using rust up.
- Also rustup is like 1.5GiB, so this will blow up the image size significantly. It would be best to remove it once Ruby is installed like it's done for
/tmp/ruby_build_deps.txt
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 intentionally stuck to Rust >= 1.58.1 to make it easier to build YJIT with prepackaged Rust installs 👍
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 took this line directly from ruby/ruby here https://github.com/ruby/ruby/blob/e896b338605914df70c1c34274f5147dff6f71a3/.cirrus.yml#L110. From @maximecb's comment I can't tell if you're agreeing I should change it or that it should stay as-is.
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 don't remember why I used rustup for cirrus CI, I'll have to look at whether the rustc from the repositories is recent enough, but it's not a big deal there because it's ephemeral.
And yeah @maximecb is agreeing that it should be changed.
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 have not been able to get a working version of this with the apt 1.59.0 version of Rust. It refuses to build Ruby with when running rake docker:build ruby_version=master:b3d8dddee7a9ea0bc9c278a5c9faa4df81afd57e
or rake docker:build arch=arm64 ruby_version=master:b3d8dddee7a9ea0bc9c278a5c9faa4df81afd57e
. I get the following error when compiling.
#13 126.1 error[E0433]: failed to resolve: could not find `is_aarch64_feature_detected` in `arch`
#13 126.1 --> /usr/src/ruby/yjit/src/options.rs:157:28
#13 126.1 |
#13 126.1 157 | if !std::arch::is_aarch64_feature_detected!("lse") {
#13 126.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ could not find `is_aarch64_feature_detected` in `arch`
#13 126.1
It seems like something installed the wrong architecture but I don't understand why this worked fine with rustup
. For reference I run my M1 terminal in rosetta so I don't have to switch back and forth.
If I set DOCKER_DEFAULT_PLATFORM=linux/amd64
with rake docker:build ruby_version=master:b3d8dddee7a9ea0bc9c278a5c9faa4df81afd57e
it will build but I when I try to run Ruby like ruby --enable-yjit -v
inside the container I get a segv. I also tried with arch64 and had the same result:
ruby: [BUG] Couldn't make JIT page (0x0000ffff7789b000, 0 bytes) executable, errno: Cannot allocate memory
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.
In general I'm for this PR.
About the implementation, even though you uninstall rust as rustup self uninstall -y
, once you finish RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
, it will consume 1.5GB.
It's a sort of docker image size optimization.
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.
even though you uninstall rust as rustup self uninstall -y, once you finish RUN
Yes, deleting in another layer won't save space, unless the layers are squashed. Since that pattern was already done for ruby_build_deps.txt
I assumed the layers were squashed.
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 I'm at a loss then, I really don't know what to do since there's no CI here running to tell me whether the apt version works and I can't get it to build correctly (without compilation failure OR segv) locally on my mac via docker.
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.
@eileencodes Maxime said that YJIT not compiling with 1.59 is a regression that she'll fix.
We should make sure ARM CI use the minimum rustc version we wish to support to avoid a similar regression in the future.
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.
Right so I tried testing that but after her changes were merged I was unable to build anything (even without my yjit changes). So Ruby master was also broken.
d5eabe9
to
49eb5a0
Compare
cc @nurse is this something we can peek your interest on? |
This PR installs Rust in the Dockerfile and enables yjit in the images. The change here is useful for using and testing yjit without being required to manually configure Ruby. One usecase is to add a build to rails/rails to test our code against yjit. I tested this locally and was able to build the docker image and verified it built correctly and I could use yjit by running the following command: ``` $ RUBY_YJIT_ENABLE=1 ruby -e 'p RubyVM::YJIT.enabled?' => true ```
49eb5a0
to
266cb90
Compare
They need to be installed and removed in the same layer to not be committed in the image.
…-slim Don't commit build dependencies
Ok I was finally able to get I've also merged Jean's changes to install and remove Rust as part of the Ruby install so the image size stays small. |
Thanks all ❤️ |
Nicely done. Thanks Eileen! |
This PR installs Rust in the Dockerfile and enables yjit in the images. The change here is useful for using and testing yjit without being required to manually configure Ruby. One usecase is to add a build to rails/rails to test our code against yjit.
I tested this locally and was able to build the docker image and verified it built correctly and I could use yjit by running the following command: