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

Reconsider the "toolchain options are ignored for a custom toolchain" error #4248

Closed
wesleywiser opened this issue Mar 11, 2025 · 8 comments · Fixed by #4250
Closed

Reconsider the "toolchain options are ignored for a custom toolchain" error #4248

wesleywiser opened this issue Mar 11, 2025 · 8 comments · Fixed by #4250

Comments

@wesleywiser
Copy link
Member

Problem you are trying to solve

The rustup 1.28 release introduced new validation for custom toolchains listed in rust-toolchain.toml files. At Microsoft, we use an internal build of the Rust compiler that is distributed as a custom toolchain (not managed by rustup but linked to from ~/.rustup/toolchains) so it's common for our developers to have rust-toolchain.toml files that look like:

[toolchain]
channel = "ms-1.84.1"
targets = [
    "aarch64-pc-windows-msvc",
    "x86_64-pc-windows-msvc",
]

We have tooling that handles installing the Rust build and appropriate targets. For our developers, this is nice as it aligns very closely to working with the standard Rust toolchain build, which we want to support as much as possible.

With the update to rustup 1.28 however, any invocation of rustc, cargo, etc is met with:

> cargo --version
error: toolchain options are ignored for a custom toolchain (ms-1.84.1)

even when the appropriate custom toolchain is already configured and ready to be used.

Solution you'd like

Ideally, it would be great if this error could only be shown in the case where required components or targets are missing from the custom toolchain!

I understand that is probably difficult to determine ahead of time so some other solutions might be:

  1. Revert the additional validation for custom toolchain options.
    • If this is acceptable, I would be happy to prepare a PR with this change.
  2. Allow the user to ignore this error entirely somehow (preferably via a configuration flag in ~/.rustup/settings.toml or an environment variable).
  3. Downgrade the error to a warning or informational message.
    • This is not highly preferred because it will still appear frequently to our developers while not being useful/actionable.

Thanks for your consideration and efforts to improve rustup!

Notes

No response

@djc
Copy link
Contributor

djc commented Mar 12, 2025

Thanks for the feedback, and sorry for the hiccups we caused for using Rust at MS!

Solution 1 sounds like a reasonable option to me from this list, would be good to review a PR in that direction.

wesleywiser added a commit to wesleywiser/rustup that referenced this issue Mar 12, 2025
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.

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.

Manual testing confirms this fixes rust-lang#4248.
@wesleywiser
Copy link
Member Author

Thanks so much for the quick response @djc! Occasional hiccups are expected and we're taking this opportunity to improve reliability on our side as well 🙂

wesleywiser added a commit to wesleywiser/rustup that referenced this issue Mar 12, 2025
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.

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.

Manual testing confirms this fixes rust-lang#4248.
@djc
Copy link
Contributor

djc commented Mar 12, 2025

Thanks so much for the quick response @djc! Occasional hiccups are expected and we're taking this opportunity to improve reliability on our side as well 🙂

So do I understand correctly that you have tooling that uses toolchain.targets key for something? Is part of your plan to shift that away to using a more custom key?

wesleywiser added a commit to wesleywiser/rustup that referenced this issue Mar 13, 2025
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 added a commit to wesleywiser/rustup that referenced this issue Mar 13, 2025
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 added a commit to wesleywiser/rustup that referenced this issue Mar 14, 2025
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 added a commit to wesleywiser/rustup that referenced this issue Mar 14, 2025
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.
github-merge-queue bot pushed a commit that referenced this issue Mar 14, 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.
@wesleywiser
Copy link
Member Author

So do I understand correctly that you have tooling that uses toolchain.targets key for something?

Yes, we use it for the same purpose that rustup does: listing the targets needed by the project which should be installed by our tooling.

Is part of your plan to shift that away to using a more custom key?

We've discussed that but given that the meaning is the same, we'd like to keep symmetry with rustup. We're currently exploring ways to make it so that rustup (and the cargo, rustc, etc redirectors) don't need to be run inside projects that use our internal build.

@wesleywiser
Copy link
Member Author

Is it possible for this fix to be included in the upcoming .2 release? I'm not familiar with your release process so just asking in case it needs another decision or backport 🙂

@rami3l
Copy link
Member

rami3l commented Mar 14, 2025

Is it possible for this fix to be included in the upcoming .2 release? I'm not familiar with your release process so just asking in case it needs another decision or backport 🙂

@wesleywiser Yes, it definitely will.

@wesleywiser
Copy link
Member Author

Awesome, thank you (and @djc as well) so much for the really fast responses and iterations on this issue! I can tell you that it's greatly appreciated by many people in Microsoft 🏅

@ChrisDenton
Copy link
Member

We're currently exploring ways to make it so that rustup (and the cargo, rustc, etc redirectors) don't need to be run inside projects that use our internal build.

Btw, you might want to look at the history of #3035 in case it's relevant; apparently there's a fair amount of stuff that expects the proxies. Though that may be less of an issue for your specific builds.

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

Successfully merging a pull request may close this issue.

4 participants