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

Specify rust toolchain explicitly, document how to change it #14655

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Feb 13, 2025

Which issue does this PR close?

Rationale for this change

Currently DataFusion's CI runs with whatever the most recent release is. This ensures DataFusion always works with the latest rust release but causes some non trival churn:

Magically broken Releases

Currently whenever a new rust version is released we often scramble to fix the CI (for new clippy lints for example).

Some recent examples

Old releases don't pass CI cleanly

As @findepi observes on #14479, always using the most recent Rust release means that older releases no longer cleanly pass CI tests

What changes are included in this PR?

We can solve this by specifying a specific version of Rust using rust-toolchain.toml
https://rust-lang.github.io/rustup/overrides.html

We use this in influxdb_iox and it works well for avoiding surprise brekages

The downside is that to use a newer version of rust with DataFusion will require a PR to update the config file

Changes:

  1. Add a rust-toolchain.toml file
  2. Add instructions on how to update it

Are these changes tested?

by CI
(testing)

Are there any user-facing changes?

Hopefully a more controlled experience

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Feb 13, 2025
Comment on lines +22 to +26
## How to update the version of Rust used in CI tests

- Make a PR to update the [rust-toolchain] file in the root of the repository:

[rust-toolchain]: https://github.com/apache/datafusion/blob/main/rust-toolchain.toml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this apply to CI only, or is rust-toolchain read by cargo too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it applies by default if you just run cargo locally if that is what you are asking

So unless someone overrides the setting by default local development environments would use the toolchain version specified by this file

@alamb alamb marked this pull request as ready for review February 13, 2025 22:36
@alamb alamb mentioned this pull request Feb 14, 2025
@alamb
Copy link
Contributor Author

alamb commented Feb 17, 2025

I plan to merge this PR over the next day or two unless there are any additional comments

@alamb alamb merged commit 873b5f7 into apache:main Feb 18, 2025
24 of 25 checks passed
@alamb
Copy link
Contributor Author

alamb commented Feb 18, 2025

Let's give it a try

@alamb
Copy link
Contributor Author

alamb commented Feb 18, 2025

Thanks everyone for the reviews

@alamb alamb deleted the alamb/pin_toolchain branch February 18, 2025 12:48
@findepi
Copy link
Member

findepi commented Feb 19, 2025

Now cargo will auto-update my local toolchain.

$ cargo test --test sqllogictests  -- map.slt --complete
info: syncing channel updates for '1.84.1-aarch64-apple-darwin'
info: latest update on 2025-01-30, rust version 1.84.1 (e71f9a9a9 2025-01-27)
info: downloading component 'cargo'
info: downloading component 'clippy'
info: downloading component 'rust-docs'
info: downloading component 'rust-std'
info: downloading component 'rustc'
info: downloading component 'rustfmt'
info: installing component 'cargo'
info: installing component 'clippy'
info: installing component 'rust-docs'
info: installing component 'rust-std'
info: installing component 'rustc'
...

very cool!

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

Successfully merging this pull request may close these issues.

Buildable release builds
4 participants