Skip to content

Fix conversion from PartialTargetTriple to TargetTriple #3467

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

Conversation

ongchi
Copy link
Contributor

@ongchi ongchi commented Aug 25, 2023

Most of valid target triple have this structure:

arch-os-env

For instance, a windows system with gnu environment would be:

x86_64-pc-windows-gnu

But a triple without env could be a valid triple:

x86_64-apple-darwin

This PR makes convertion of PartialTargetTriple without an env field a valid TargetTriple and updates the list of possible targets of PartialTargetTriple.
This also makes Component::new_with_target able to create a valid target from a triple without env.

Fix: #3166

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.

This looks good, thanks!

"i586",
"i686",
"x86_64",
"aarch64",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check, where did you source these lists of current arches, OSes and envs?

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 copied all the targets from the Platform Support section of the rustc book and created the lists manually. I'm not sure if these lists already exist somewhere, so I created them manually.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, maybe add a docstring linking to a section in the book for each of these constants?

src/dist/dist.rs Outdated
}
let arch = value.arch.ok_or("Incomplete / bad target triple")?;
let os = value.os.ok_or("Incomplete / bad target triple")?;
let triple = match value.env {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny style nit: I would spell this Ok(Self(match value.env { .. })) directly, which avoids a low-value single use binding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, willing to do so.

@djc
Copy link
Contributor

djc commented Aug 28, 2023

Maybe it would be good to add some tests for the new behavior? Especially the high-level CLI behavior as discussed in #3166.

@rbtcollins
Copy link
Contributor

Thanks for the patch.

I have two questions.

  1. Its not clear to me that this fixes the linked bug, which is about removing a fully qualified target component rustup component remove rust-std-wasm32-unknown-unknown from a nightly-x86_64-apple-darwin toolchain. A unit test showing that rust-std-wasm32-unknown-unknown was failing in this codepath would be one way to show that more clearly.

  2. I have a suspicion that this is the wrong direction to take the fix.

One possibility is to hook in a call to resolve() with the context of the toolchain.

Another possibility is to look at what would happen if instead we defined x86_64-apple-darwin as arch=x86_64, os=apple, env=darwin ?

The problem I am worried about is that i686-pc-windows is not a valid TargetTriple - it has to be resolved resolve( - which confusingly actually goes from a string -> ToolchainDesc with a fallback context of the particular host that we're configured for (e.g. cfg.get_default_host_triple()).

So a missing env gets filled in - and I'm still used to the older machine-vendor-os triples from gcc - https://wiki.osdev.org/Target_Triplet / https://github.com/gcc-mirror/gcc/blob/master/config.sub#L1877C6-L1877C37 - where apple is a vendor and darwin is the os.

@ongchi
Copy link
Contributor Author

ongchi commented Sep 11, 2023

Trying to clarify this issue as best I can.

  1. I think add/remove components by name with target string is intended to be supported, the behavior of component_add and component_remove will trying to parse target from user input before refer to current toolchain. But I agree the failing message is not clear when input target is not valid with this patch, it would better to hint user that the target is nonexisted.

  2. Since the list of supported targets could be obtained from common::list_targets(), maybe we could just match from this list for target validation?

The rustup will try to extract target triple from the substring from the user input. If the substring of user input does not match the combination of archs, oses and envs in PartialTargetTriple, then the string will be treated as a component name without target suffix.

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>).

The valid component list could be obtained from unique values from common::list_components, this would be better than relying on pattern match and decrease maintaining efforts.

Most error messages should remain, except the input component is totally gibberish and does not match any valid component and target, then display a different message to address this error.

@ongchi ongchi closed this Dec 24, 2023
@ongchi ongchi deleted the fix-conversion-partialtargettriple-to-targettriple branch April 26, 2024 14:50
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
4 participants