rustup component (add|remove) should not rely on hardcoded target triples

[rami3l]

Verification

• I searched for recent similar issues at https://github.com/rust-lang/rustup/issues?q=is%3Aissue+is%3Aopen%2Cclosed and found no duplicates.
• I am on the latest version of Rustup according to https://github.com/rust-lang/rustup/tags and am still able to reproduce my issue.

Problem

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

rustup/src/dist/triple.rs

Lines 7 to 37 in a6c9fae

	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",
	];

[...] we really should not hardcode target triples in our artifact, as the complete list changes quite regularly.

#3601 (comment)

Rustup version

rustup 1.27.0+74 (a6c9fae91 2024-04-21) dirty 1 modification
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.77.2 (25ef9e3d8 2024-04-09)`

[djc]

I think a good approach here would be to add an integration test that can download a recent manifest, parse it, and regenerate a source file that contains current definitions of these (and fails the test if they're outdated).

[rami3l]

I think a good approach here would be to add an integration test that can download a recent manifest, parse it, and regenerate a source file that contains current definitions of these (and fails the test if they're outdated).

@djc The current model of target triples in Rustup requires some non-code knowledge to correctly generate the list:

{ arch: aarch64, os: linux, env: android }
{ arch: aarch64, os: unknown-freebsd}

Looking at the original file, I think a reasonable (but possibly breaking) solution would be parsing the triples as follows:

// for `x-y`
{ arch: x, os: y }

// special case for `x-y-w` where `y` is `none` or `linux`
// e.g. `thumbv4t-none-eabi`, `i686-linux-android`
// (should've been called `x-unknown-y-w`, but alas)
{ arch: x, os: y, env: w }

// for `x-y-z`
{ arch: x, os: y-z }

// for `x-y-z-w`
{ arch: x, os: y-z, env: w }

Then we can get a list of triples by using things like rustc --print target-list (I assume we cannot add/remove a component without even having rustc installed, right?), or by reading the manifest (either at comptime or at runtime) and deduplicating.

[djc]

Yeah, splitting is definitely not completely straightforward, but having an automated test that checks our internal code matches any targets in current manifests still sounds good.

[rami3l]

@djc After some investigation I think https://docs.rs/platforms could potentially be a better alternative, also I like its glob patterns which might greatly simplify our Implementation.

I'm not sure how this might interact with our triple shorthands though...

rustup/src/dist/triple.rs

Lines 107 to 119 in 2a5a69e

	let success_cases = vec![
	("", (None, None, None)),
	("i386", (Some("i386"), None, None)),
	("pc-windows", (None, Some("pc-windows"), None)),
	("gnu", (None, None, Some("gnu"))),
	("i386-gnu", (Some("i386"), None, Some("gnu"))),
	("pc-windows-gnu", (None, Some("pc-windows"), Some("gnu"))),
	("i386-pc-windows", (Some("i386"), Some("pc-windows"), None)),
	(
	"i386-pc-windows-gnu",
	(Some("i386"), Some("pc-windows"), Some("gnu")),
	),
	];

Also we need to ensure the existence of an escape hatch for new/custom triples.

[djc]

Ah yes, that looks great!

[rami3l]

So here's my plan for the experiment: I'll try to generate a source file by CS 101-level string templating according to the rules in #3783 (comment), and include!() it in src/dist/triple.rs. Or, better yet, I can add this into rustup-macros as a proc_macro.

The only concern I have is the following snippet in build.rs. We almost definitely need to decouple src/dist/triple.rs from this one since now it's build.rs' responsibility to generate the list of triple components.

rustup/build.rs

Lines 3 to 27 in 2a5a69e

	include!("src/dist/triple.rs");
	fn from_build() -> Result<PartialTargetTriple, String> {
	let triple = if let Ok(triple) = env::var("RUSTUP_OVERRIDE_BUILD_TRIPLE") {
	triple
	} else {
	env::var("TARGET").unwrap()
	};
	PartialTargetTriple::new(&triple).ok_or(triple)
	}
	fn main() {
	println!("cargo:rerun-if-env-changed=RUSTUP_OVERRIDE_BUILD_TRIPLE");
	println!("cargo:rerun-if-env-changed=TARGET");
	match from_build() {
	Ok(triple) => eprintln!("Computed build based partial target triple: {triple:#?}"),
	Err(s) => {
	eprintln!("Unable to parse target '{s}' as a PartialTargetTriple");
	eprintln!(
	"If you are attempting to bootstrap a new target you may need to adjust the\n\
	permitted values found in src/dist/triple.rs"
	);
	std::process::abort();
	}
	}

However I don't know too much about this undocumented environment variable and how it's supposed to be used. Maybe @kinnison can provide more context on this one?

I mean, by using platforms as a build dependency, we at least know whether RUSTUP_OVERRIDE_BUILD_TRIPLE has been set to a valid full triple (instead of a partial triple), and that regressing change, albeit breaking, should not cause too much damage.

Update: Grepping the whole repo showed that RUSTUP_OVERRIDE_BUILD_TRIPLE has been used only once in the actual business logic, and exactly as a full triple, so this partial triple thing in build.rs is probably not very useful after all?

rustup/src/dist/dist.rs

Line 261 in 2a5a69e

impl TargetTriple {

rustup/src/dist/dist.rs

Lines 266 to 268 in 2a5a69e

	pub(crate) fn from_build() -> Self {
	if let Some(triple) = option_env!("RUSTUP_OVERRIDE_BUILD_TRIPLE") {
	Self::new(triple)

[djc]

Instead of using include!() or a procedural macro, IMO it would be better to have an integration test that writes into a source file inside src and can be imported by the usual means. The integration test can then panic if the expected (newly generated) contents of the file are not the same as the "current" contents (on disk).

[rami3l]

Instead of using include!() or a procedural macro, IMO it would be better to have an integration test that writes into a source file inside src and can be imported by the usual means. The integration test can then panic if the expected (newly generated) contents of the file are not the same as the "current" contents (on disk).

@djc I see your point of keeping those lists clearly visible in the codebase, but will this cause PR authors to chase a moving target, similar to what we have now WRT clippy lints? My original point is that outsourcing the changes to an external dependency means updating the list is always a version bump away.

[djc]

Potentially -- but (a) fixing it should be very straightforward and (b) don't know how much the target moves. If you think using platforms is easier, I'd go with that first, we can see which downsides we run into over time.