Skip to content
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

fix: update Rust if rust in sysreqs #4

Closed
wants to merge 3 commits into from

Conversation

eitsupi
Copy link
Contributor

@eitsupi eitsupi commented Mar 20, 2025

As I posted at #2 (comment), since Rust in containers becomes outdated over time, it would make sense to update Rust for packages that need it.

@jeroen
Copy link
Member

jeroen commented Mar 20, 2025

Would it be more efficient if we updated it every time we rebuild this container (and do this regularly)?

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 20, 2025

Would it be more efficient if we updated it every time we rebuild this container (and do this regularly)?

Given the small number of packages that have dependencies on Rust, I imagined that it would be more efficient to update each time.

If the update is done at container build time, the older R version of the container (now R 4.3) would also need to be built each time.

@jeroen jeroen closed this in 9f51bdf Mar 20, 2025
@jeroen
Copy link
Member

jeroen commented Mar 20, 2025

If the update is done at container build time, the older R version of the container (now R 4.3) would also need to be built each time.

Ah right, that is still a problem.

@jeroen jeroen reopened this Mar 20, 2025
@jeroen
Copy link
Member

jeroen commented Mar 20, 2025

Hmm why does this not run: https://github.com/r-universe-org/build-wasm/actions/runs/13969838927/job/39108621933
Is rustup not on the path?

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 20, 2025

I think it is just a syntax error in the Dockerfile, I recommend using Heredocs.
https://jedevc.com/blog/dockerfile-heredocs-intro/

@jeroen jeroen closed this in 7762166 Mar 20, 2025
jeroen added a commit that referenced this pull request Mar 20, 2025
jeroen added a commit that referenced this pull request Mar 20, 2025
@jeroen
Copy link
Member

jeroen commented Mar 20, 2025

Now running into this: rust-lang/rustup#2417
See https://github.com/r-universe-org/build-wasm/actions/runs/13970138434/job/39109590767

jeroen added a commit that referenced this pull request Mar 20, 2025
jeroen added a commit that referenced this pull request Mar 20, 2025
@eitsupi eitsupi deleted the ensure-update-rust branch March 20, 2025 13:28
@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 20, 2025

In my opinion, doing a daily Docker Build just for Rust is a waste of energy and storage. Wouldn't once a week/month be enough?

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 20, 2025

Another question is that once the nightly toolchain is removed, will it ruin the default toolchain that was set to nightly in the base image?

@jeroen
Copy link
Member

jeroen commented Mar 20, 2025

I think daily is fine, that way it is clearer to see which day it started failing when there is a problem.

Can you check if the current solution works for you for r-release? I don't think we'll be updating the 4.3 image anymore, it will go out of use in a few weeks anyway.

@jeroen
Copy link
Member

jeroen commented Mar 20, 2025

Another question is that once the nightly toolchain is removed, will it ruin the default toolchain that was set to nightly in the base image?

I am not sure, could you test this? Sadly it seems that we cannot just update the nightly due to #4 (comment)

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 20, 2025

it will go out of use in a few weeks anyway.

Since webR does not quickly follow R releases on regular machines, I think it is important to have packages that work with older versions.
In other words, I don't expect the R 4.5.0 version of webR to be used immediately after the R 4.5.0 release.

In fact, the webR JupyterLite kernel is still based on R 4.3.
https://github.com/r-wasm/jupyterlite-webr-kernel

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 20, 2025

could you test this?

It seems failed.
https://github.com/r-universe/prql/actions/runs/13971539228/job/39114612837

error: "/usr/local/rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/Cargo.lock" does not exist, unable to build with the standard library, try:
        rustup component add rust-src --toolchain nightly-x86_64-unknown-linux-gnu

What about updating only the stable toolchain without updating the nightly toolchain?

My main problem is that the first build (regular linux target, not emscripten) fails, so it is possible that the nightly toolchain is unrelated.

@jeroen
Copy link
Member

jeroen commented Mar 20, 2025

What about updating only the stable toolchain without updating the nightly toolchain?

Yes we can try this. Is there any reason you need nightly?

jeroen added a commit that referenced this pull request Mar 20, 2025
@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 20, 2025

Is there any reason you need nightly?

No, there is not.

To begin with, I had to set up an arbitrary toolchain in the configure script to make the build succeed even with the r-wasm/actions/build-rwasm action, so for Emscripten target builds, as long as rustup is installed/
https://github.com/eitsupi/neo-r-polars/blob/021172e581cfa48f434fc88e1eadbf4444e2f3b4/configure#L3-L16

The problem with R-universe is that the first build for the normal target fails and subsequent Emscripten target builds are not executed because the stable toolchain used for nomal target is out of date.

@jeroen
Copy link
Member

jeroen commented Mar 20, 2025

The problem with R-universe is that the first build for the normal target fails and subsequent Emscripten target builds are not executed because the stable toolchain used for nomal target is out of date.

I'm sorry to hear. Can we just update both the stable and emscripten toolchain in the image?

@jeroen
Copy link
Member

jeroen commented Mar 20, 2025

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 20, 2025

In the case of my neopolars, I think it is very likely that a modification of 32f307f will suffice.

Certain features in the nightly toolchain may be removed in the future, so the latest nightly toolchain may not necessarily be suitable.
So for polars that have code that depends on nightly toolchain channel, they will need to fix the date of the nightly toolchain in some way, so they will need to install it themselves (it happened to work until now, but it stopped working the other day I've changed it to install via configure script)

@jeroen
Copy link
Member

jeroen commented Mar 20, 2025

It seems neopolars still does not build: https://github.com/r-universe/eitsupi/actions/runs/13904598903/job/39123880162

I am confused why it does work on r-oldrel which has a much older version of rust?

@eitsupi
Copy link
Contributor Author

eitsupi commented Mar 20, 2025

Sorry, I forgot that the default toolchain needs to be changed to stable on the first build.
After the first build, you need to change it back to the nightly toolchain.

I think that in the R 4.3 job, when the first build failed, pak did not correctly fail the job with an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants