Revert "tidy: validate LLVM component names in tests"

[erikdesjardins]

This reverts #125472.

This has already caused a bit of trouble, and I was mistaken about the original motivation--incorrect component names will be detected by a full CI run.

I no longer think it pulls its weight.

r? @workingjubilee

[RalfJung]

Ideally we'd document this somewhere. Is there a place that we document the meaning of all the ui-test annotations? For @needs-llvm-component we could then mention COMPILETEST_REQUIRE_ALL_LLVM_COMPONENTS.

[jieyouxu]

Ideally we'd document this somewhere. Is there a place that we document the meaning of all the ui-test annotations? For @needs-llvm-component we could then mention COMPILETEST_REQUIRE_ALL_LLVM_COMPONENTS.

cc #125706

rustc-dev-guide has some non-exhaustive description of various UI test directives and annotations, but AFAIK we don't have an exhaustive list of descriptions.

[workingjubilee]

@Hoverbear what do you think? would you have wanted to wait until after your PR was r+'d?

[RalfJung]

Which other PR are you referring to?

[Hoverbear]

@workingjubilee I do not believe this impacts me, go for it. :)

[erikdesjardins]

Opened rust-lang/rustc-dev-guide#1990 to add COMPILETEST_REQUIRE_ALL_LLVM_COMPONENTS to the docs.

[workingjubilee]

hmm.

I realize this caused a bit of issues but I don't think it really doesn't pull its weight? I think what the troubles point to is a need for a more centralized way in the repo of describing "this is the list of components we want to build LLVM with, generally speaking".

[workingjubilee]

the troubles, after all, did also lead to us reenabling a test.

[RalfJung]

The test was disabled because of a confusing a error message in the "all components must be available" check, and because that check erroneously ran on a build of LLVM that does not have all components. Not sure if this is a good argument to keep the new tidy check around: the actually relevant point is that someone looked into whether there is a "csky" component.

Having to maintain yet another list in tidy better come with sufficient benefits for this extra paperwork. I am not convinced that is the case here.

[workingjubilee]

Having to maintain yet another list in tidy better come with sufficient benefits for this extra paperwork. I am not convinced that is the case here.

That's the thing that I think is the actual problem: tidy shouldn't have even one single list that is just "the list of things we're feeding into rustc's build anyways". That's a file that can be shared between crates, so it should be somewhere it can be shared.

[workingjubilee]

...like, why does every single test that specifies a --target have to have a needs-llvm-components directive anyways???

if we want to talk about duplication...

[Dylan-DPC]

@erikdesjardins@workingjubilee what's the status on this? thanks

[jieyouxu]

Could someone elaborate what the "root" thing the llvm component checks are trying to address?

rust/src/bootstrap/src/core/build_steps/test.rs

Lines 1943 to 1961 in df4ca44

	let llvm::LlvmResult{ llvm_config, .. } =
	builder.ensure(llvm::Llvm{target: builder.config.build});
	if !builder.config.dry_run(){
	let llvm_version =
	command(&llvm_config).arg("--version").run_capture_stdout(builder).stdout();
	let llvm_components =
	command(&llvm_config).arg("--components").run_capture_stdout(builder).stdout();
	// Remove trailing newline from llvm-config output.
	cmd.arg("--llvm-version")
	.arg(llvm_version.trim())
	.arg("--llvm-components")
	.arg(llvm_components.trim());
	llvm_components_passed = true;
	}
	if !builder.is_rust_llvm(target){
	// FIXME: missing Rust patches is not the same as being system llvm; we should rename the flag at some point.
	// Inspecting the tests with `// no-system-llvm` in src/test *looks* like this is doing the right thing, though.
	cmd.arg("--system-llvm");
	}

bootstrap knows which (local) llvm components are available via llvm-config --components, and this info is made available to compiletest in

rust/src/tools/compiletest/src/header.rs

Lines 1473 to 1491 in df4ca44

	ifletSome(needed_components) =
	config.parse_name_value_directive(line,"needs-llvm-components")
	{
	let components:HashSet<_> = config.llvm_components.split_whitespace().collect();
	ifletSome(missing_component) = needed_components
	.split_whitespace()
	.find(|needed_component| !components.contains(needed_component))
	{
	if env::var_os("COMPILETEST_REQUIRE_ALL_LLVM_COMPONENTS").is_some(){
	panic!(
	"missing LLVM component {}, and COMPILETEST_REQUIRE_ALL_LLVM_COMPONENTS is set",
	missing_component
	);
	}
	returnIgnoreDecision::Ignore{
	reason:format!("ignored when the {missing_component} LLVM component is missing"),
	};
	}
	}

But frankly, I don't quite understand what these things together are trying to check. In particular, what is COMPILETEST_REQUIRE_ALL_LLVM_COMPONENTS intended to do?

Note that just using a llvm-config --components might not be sufficient: our CI has a particular set of available llvm components. However, a user local custom llvm can report a different list of available llvm components. If we tried to use llvm-config --components as the ground truth on the user's system, this can cause a naively implemented unknown components check to fail.

[RalfJung]

In particular, what is COMPILETEST_REQUIRE_ALL_LLVM_COMPONENTS intended to do?

See #125710: it makes a missing LLVM component into a test failure, rather than ignoring the test.

[jieyouxu]

Ah, thanks for clarifying, that makes sense.

[jieyouxu]

So I'm inclined to drop this tidy check, in favor of something like proper //@ cross-compile-target that should infer a //@ needs-llvm-components based on the target triple. I.e. on the rationale that we should not need a separate //@ needs-llvm-components in the first place. @workingjubilee what do you think?

[workingjubilee]

Yeah, that was basically some of what I was thinking of

[jieyouxu]

Given that we want to move towards //@ cross-compile-target, I'm inclined to drop this tidy check.

[jieyouxu]

@bors r+

[jieyouxu]

@bors ping

[jieyouxu]

@bors r+

[bors]

📌 Commit 040929b has been approved by jieyouxu

It is now in the queue for this repository.

[bors]

⌛ Testing commit 040929b with merge 23e7ecb...

[bors]

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 23e7ecb to master...

[rust-timer]

Finished benchmarking commit (23e7ecb): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (primary 1.2%, secondary -2.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.2%	[1.2%, 1.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	1.2%	[1.2%, 1.2%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 787.86s -> 789.761s (0.24%)
Artifact size: 335.54 MiB -> 335.51 MiB (-0.01%)