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 component remove error by component name with explicit target triple #3601

Merged
merged 3 commits into from
Apr 23, 2024

Conversation

ongchi
Copy link
Contributor

@ongchi ongchi commented Dec 24, 2023

Fix #3166

Follow by #3467, another approach to fix the error.

Rationale

The basic point of this PR is to improve the error message when Rustup somehow cannot recognize a component's full name by extracting the component name first from the manifest when parsing rust-std-x-y-z, so that at least x-y-z is intended to be a target triple. This prevents Rustup from falling back on the default target triple, producing an error message that is not very helpful (see the quote below).

The direct cause of #3166 is that wasm32-unknown-unknown hasn't been hardcoded into our codebase:

static LIST_ARCHS: &[&str] = &[
"i386",
"i586",
"i686",
"x86_64",
"arm",
"armv7",
"armv7s",
"aarch64",
"mips",
"mipsel",
"mips64",
"mips64el",
"powerpc",
"powerpc64",
"powerpc64le",
"riscv64gc",
"s390x",
"loongarch64",
];
static LIST_OSES: &[&str] = &[
"pc-windows",
"unknown-linux",
"apple-darwin",
"unknown-netbsd",
"apple-ios",
"linux",
"rumprun-netbsd",
"unknown-freebsd",
"unknown-illumos",
];

... but that's another issue (later split into #3783), i.e. we really should not hardcode target triples in our artifact, as the complete list changes quite regularly.

The old component_(add|remove)() functions rely on the new_with_target() function; on the other hand, the new implementation of those two functions in this PR is based on distributable and fallback_target, which relies on detecting the component names (fetched from the distributable manifest) first, so will be more robust implementation-wise.

For the case in #3166:

rustup component remove rust-std-wasm32-unknown-unknown

Because of rustup cannot parse wasm32-unknown-unknown as a valid target, the rust-std-wasm32-unknown-unknown here is treated as a component name, and the target inferred to be x86_64-apple-darwin for this case. The error message as following:

error: toolchain 'nightly-x86_64-apple-darwin' does not contain component 'rust-std-wasm32-unknown-unknown' for target 'x86_64-apple-darwin'
note: not all platforms have the standard library pre-compiled: https://doc.rust-lang.org/nightly/rustc/platform-support.html

The actual component and target should be rust-std and wasm32-unknown-unknown in this error message respectively.
This is subtle because component name may also contains a '-' character (rust-std in this case), the component name is determined by user input subtract from possible target suffix currently (TryFrom<PartialTargetTriple>).

#3467 (comment)

@rami3l rami3l self-requested a review March 18, 2024 12:59
Copy link
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

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

LGTM, and really sorry for the long wait! I personally would like to get this into Rustup v1.28.0.

src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
@rami3l rami3l requested a review from djc March 26, 2024 08:47
@ongchi ongchi requested a review from rami3l April 14, 2024 14:09
Copy link
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

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

Nice work :)

Copy link
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

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

Thanks a lot! I think this PR is good to go.

@rami3l
Copy link
Member

rami3l commented Apr 15, 2024

@djc @rbtcollins I guess we can ship this with v1.27.1 as well? As I see it, only rustup component (add|remove) is affected here and it's rather unlikely for this PR to introduce a regression somewhere given all our existing tests combined with the new ones...

@rami3l rami3l added this to the 1.27.1 milestone Apr 15, 2024
@rami3l rami3l requested a review from djc April 23, 2024 05:19
@rami3l rami3l added this pull request to the merge queue Apr 23, 2024
Merged via the queue into rust-lang:master with commit 7677fea Apr 23, 2024
23 checks passed
@ongchi ongchi deleted the fix-3166 branch April 26, 2024 14:49
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.

rustup component remove component-NOT-TOOLCHAIN-TARGET errors
3 participants