Skip to content

Conversation

@EricccTaiwan
Copy link
Contributor

@EricccTaiwan EricccTaiwan commented Jul 21, 2025

Which issue does this PR close?

None.

Rationale for this change

Without a fixed toolchain, contributors may use different Rust versions, leading to inconsistent Clippy lints or build errors (e.g., from renamed lints). This change ensures that the project builds consistently across environments.

What changes are included in this PR?

  • Add a rust-toolchain.toml file to pin the Rust toolchain to the stable channel.

Are these changes tested?

This change affects toolchain configuration and does not modify runtime code. Behavior is implicitly tested via CI builds, which will now consistently use the pinned toolchain.

Are there any user-facing changes?

No.

@alamb
Copy link
Contributor

alamb commented Jul 21, 2025

THANK YOU @EricccTaiwan -- this is super helpful

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of adding a specific release here like https://github.com/apache/datafusion/blob/70e7eb3aaef221c8fefe2eee6198e9a61d68bc0f/rust-toolchain.toml#L22

1.88

That will prevent random failure when new stable releases are released

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of adding a specific release here like https://github.com/apache/datafusion/blob/70e7eb3aaef221c8fefe2eee6198e9a61d68bc0f/rust-toolchain.toml#L22

1.88

That will prevent random failure when new stable releases are released

Makes sense. Using 1.88 avoids random breakage, though it means running rustup manually.
I'm okay with both. What do you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is preferable to have to manually bump it to avoid CI randomly breaking when a new rust version is released.

@EricccTaiwan
Copy link
Contributor Author

By the way, would it make sense to also add components = ["rustfmt", "clippy"] like in DataFusion?
https://github.com/apache/datafusion/blob/70e7eb3aaef221c8fefe2eee6198e9a61d68bc0f/rust-toolchain.toml#L23

@alamb
Copy link
Contributor

alamb commented Jul 22, 2025

By the way, would it make sense to also add components = ["rustfmt", "clippy"] like in DataFusion? https://github.com/apache/datafusion/blob/70e7eb3aaef221c8fefe2eee6198e9a61d68bc0f/rust-toolchain.toml#L23

I am not sure

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @EricccTaiwan -- think this looks great to me

@EricccTaiwan
Copy link
Contributor Author

By the way, would it make sense to also add components = ["rustfmt", "clippy"] like in DataFusion? https://github.com/apache/datafusion/blob/70e7eb3aaef221c8fefe2eee6198e9a61d68bc0f/rust-toolchain.toml#L23

I am not sure

Just thought it might be helpful for consistency, and possibly more contributor-friendly too -- especially for folks who rely on cargo fmt and cargo clippy out of the box.
But I’m good either way. 👍

@alamb
Copy link
Contributor

alamb commented Jul 23, 2025

Hmm, looks like the CI jobs will need to be updated to account for the new toolchain somehow

@EricccTaiwan
Copy link
Contributor Author

Hmm, looks like the CI jobs will need to be updated to account for the new toolchain somehow

Yes, I see -- I can give it a try to modify the CI part.
By the way, is there a way for me to test the CI on my own fork? Or does it only run on the upstream repo?

@alamb
Copy link
Contributor

alamb commented Jul 23, 2025

Hmm, looks like the CI jobs will need to be updated to account for the new toolchain somehow

Yes, I see -- I can give it a try to modify the CI part. By the way, is there a way for me to test the CI on my own fork? Or does it only run on the upstream repo?

I think can make (another) PR into your own fork to test the CI

This file ensures that contributors and CI use the same Rust toolchain,
preventing version mismatch issues and improving reproducibility.

Signed-off-by: Cheng-Yang Chou <[email protected]>
@EricccTaiwan
Copy link
Contributor Author

EricccTaiwan commented Jul 26, 2025

Hmm, looks like the CI jobs will need to be updated to account for the new toolchain somehow

Yes, I see -- I can give it a try to modify the CI part. By the way, is there a way for me to test the CI on my own fork? Or does it only run on the upstream repo?

I think can make (another) PR into your own fork to test the CI

I'm a bit confused — on my fork, CI only seems to run on the main branch. When I sync with upstream, it works fine, but creating a new branch (like this PR) doesn’t trigger CI at all.
And just to confirm: on the upstream repo, do CI jobs always require approval before they start running?

Edit: Cuz I need CI to show me what needs changing. 😢

@alamb
Copy link
Contributor

alamb commented Jul 28, 2025

I'm a bit confused — on my fork, CI only seems to run on the main branch. When I sync with upstream, it works fine, but creating a new branch (like this PR) doesn’t trigger CI at all.
And just to confirm: on the upstream repo, do CI jobs always require approval before they start running?

CI jobs on this (the upstream) repo need approval for first time contributors. Once we have merged a PR from you then the CI starts automatically on subsequent PRs

@alamb
Copy link
Contributor

alamb commented Aug 7, 2025

Here is an example of what happens without this PR:

  1. New clippy is released, and CI tests start failing
  2. First person to notice needs to make a PR to fix it
  3. Fix new clippy lints from Rust 1.89 #8078

@alamb
Copy link
Contributor

alamb commented Aug 7, 2025

Since the new upgrade just caused me annoyance I am going to try and push this PR through (It will be easier for me as I can trigger CI). Thanks you @EricccTaiwan

@alamb alamb added the development-process Related to development process of arrow-rs label Aug 7, 2025
@EricccTaiwan
Copy link
Contributor Author

Since the new upgrade just caused me annoyance I am going to try and push this PR through (It will be easier for me as I can trigger CI). Thanks you @EricccTaiwan

Hi @alamb , I’ve been a bit busy lately, and this change looks a bit more involved than I can tackle right now.
I’m not sure how to move it forward at the moment, but I’m happy to help if there’s anything specific you need. Thanks!

@alamb
Copy link
Contributor

alamb commented Aug 7, 2025

Since the new upgrade just caused me annoyance I am going to try and push this PR through (It will be easier for me as I can trigger CI). Thanks you @EricccTaiwan

Hi @alamb , I’ve been a bit busy lately, and this change looks a bit more involved than I can tackle right now. I’m not sure how to move it forward at the moment, but I’m happy to help if there’s anything specific you need. Thanks!

No worries -- thank you for the help. I will get this one over the line. Than you for all your help so far


name: Prepare Rust Builder
description: 'Prepare Rust Build Environment'
inputs:
Copy link
Contributor

Choose a reason for hiding this comment

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

I removed these parameters as almost all uses installed stable

target: wasm32-unknown-unknown,wasm32-wasip1
- name: Install wasm32 targets
run: |
rustup target add wasm32-unknown-unknown
Copy link
Contributor

Choose a reason for hiding this comment

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

This adds 4 seconds to the job:

Screenshot 2025-08-07 at 11 19 10 AM

docs:
name: Rustdocs are clean
runs-on: ubuntu-latest
strategy:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is let over from a previous time when we checked docs on different configurations -- at the moment there is a single pair that is used, so I unparameterized this and made the version used explicit

Copy link
Contributor

@alamb alamb Aug 7, 2025

Choose a reason for hiding this comment

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

This adds 9s to the job

Screenshot 2025-08-07 at 11 23 24 AM

@alamb
Copy link
Contributor

alamb commented Aug 12, 2025

Thanks again @EricccTaiwan -- since there has been no more comments on this PR I will merge it in

@alamb alamb merged commit c628435 into apache:main Aug 12, 2025
31 checks passed
@EricccTaiwan
Copy link
Contributor Author

Thanks again @EricccTaiwan -- since there has been no more comments on this PR I will merge it in

Thanks @alamb, for doing most of the work, lol!

@EricccTaiwan EricccTaiwan deleted the add-rust-toolchain branch August 13, 2025 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

development-process Related to development process of arrow-rs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants