Skip to content

Remove validation for custom toolchains when reading rust-toolchain.toml #4250

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

Merged

Conversation

wesleywiser
Copy link
Member

@wesleywiser wesleywiser commented Mar 12, 2025

This validation was introduced in e1306b3 However, it breaks a number of scenarios that previously worked without issue such as #4248. As requested by rustup maintainers in that thread, I'm taking out the extra validation added in that commit, leaving the rest of the code as-is.

Testing confirms this fixes #4248.

Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Will wait for @rami3l to have a look.

I guess alternatively we could show a warning, and then store a setting that we've shown the warning, or something? (Or, store the date when we've last shown the warning and only show it again after x days?)

@wesleywiser wesleywiser force-pushed the revert_custom_toolchain_validation branch from 1b80189 to 0c3d6eb Compare March 12, 2025 17:00
@rami3l
Copy link
Member

rami3l commented Mar 13, 2025

@wesleywiser Thanks a lot for your comprehension and your quick patch!

I was not able to figure out a way to write an automated test for this scenario since the rustup CLI needs the custom toolchain to actually exist and unit testing looked infeasible due to use of the Cfg type.

Sorry for the back and forth, but I would still like to see an automated test for this scenario. Is it possible to trick rustup into believing the existence of that toolchain by installing e.g. stable with the components you want, and rename it directly under $RUSTUP_HOME/toolchains (written as cx.config.rustupdir.join("toolchains") in our test suites)?

@wesleywiser wesleywiser force-pushed the revert_custom_toolchain_validation branch 2 times, most recently from e0a8a6b to 21716f8 Compare March 13, 2025 19:33
@wesleywiser
Copy link
Member Author

Thanks @rami3l for that suggestion! I've added a test that covers this 🙂

@rami3l
Copy link
Member

rami3l commented Mar 14, 2025

@wesleywiser About the message that @djc has mentioned, I think it will probably be worthwhile to add a warning clarifying that we are not doing any checks, as demonstrated in the new test. That said, it will definitely not be a blocker anyway. Thanks again for your contribution!

@djc I'm afraid a "last emitted warning" check might not be enough now that we are considering some workstation with multiple repos/toolchain combinations...

@wesleywiser wesleywiser force-pushed the revert_custom_toolchain_validation branch from 21716f8 to 1f446ea Compare March 14, 2025 00:23
This validation was introduced in e1306b3
However, it breaks a number of scenarios that previously worked without
issue such as rust-lang#4248. As requested by rustup maintainers in that thread,
I'm taking out the extra validation added in that commit, leaving the
rest of the code as-is.

Testing confirms this fixes rust-lang#4248.
@wesleywiser wesleywiser force-pushed the revert_custom_toolchain_validation branch from 1f446ea to 0585f41 Compare March 14, 2025 00:25
@rami3l rami3l added this pull request to the merge queue Mar 14, 2025
Merged via the queue into rust-lang:master with commit ec156b4 Mar 14, 2025
29 checks passed
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.

Reconsider the "toolchain options are ignored for a custom toolchain" error
3 participants