Give tidy the good news about C-SKY

[workingjubilee]

It seems this was overlooked in #125472 because we don't test C-SKY much yet.

Fixes #125697

r? @erikdesjardins

[rustbot]

Failed to set assignee to erikdesjardins: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

[workingjubilee]

:ferrisCluelesser:

r? bootstrap

[Kobzol]

r? @Kobzol

@bors r+

[bors]

📌 Commit 084f058 has been approved by Kobzol

It is now in the queue for this repository.

[lqd]

Can the test be re-enabled?

rust/tests/ui/abi/compatibility.rs

Lines 61 to 65 in 7516912

	// FIXME: disabled since it fails on CI saying the csky component is missing
	/* revisions: csky
	[csky] compile-flags: --target csky-unknown-linux-gnuabiv2
	[csky] needs-llvm-components: csky
	*/

[RalfJung]

Can you also enable csky in tests/ui/abi/compatibility.rs so we can tell whether this actually works? In the past there was some trouble with this component, but the details are lost to github deleting all our logfiles...

[RalfJung]

@bors r-

[workingjubilee]

I believe the problem with CSKY was that when we supported sufficiently early-enough versions of LLVM, those ones didn't have the actual component in them.

[workingjubilee]

Since the change to LLVM 18 and the dropping of an old LLVM version have happened since last November, the logfiles would have been misinforming you anyways.

[workingjubilee]

It passes locally, and I was able to make the test emit a compile error for specifically the csky revision, so it is in fact building it.

[RalfJung]

Yeah the test already passed locally back then (and I did verify that it was being run). But then somehow on CI the test failed.

I believe the problem with CSKY was that when we supported sufficiently early-enough versions of LLVM, those ones didn't have the actual component in them.

That doesn't explain it though -- the test had the needs-component line, so if the LLVM on CI does not have csky it should have skipped the test. Maybe our implementation of needs-component is buggy...

[workingjubilee]

Maybe our implementation of needs-component is buggy...

do you really think people would really do that? just go out on the internet and write bugs in their test code? :^)

[lqd]

LLVM component shenanigans and unclear CI requirements could mess a rollup, @bors r=Kobzol,lqd rollup=iffy

[bors]

📌 Commit 81bc4d0 has been approved by Kobzol,lqd

It is now in the queue for this repository.

[Kobzol]

There was some weird csky behavior in #125141. The x86_64-gnu-llvm-17 job was outputting

error: could not create LLVM TargetMachine for triple: csky-unknown-linux-gnuabiv2: No available targets are compatible with triple "csky-unknown-linux-gnuabiv2"

in tests/run-make/print-target-list. The test was obviously succeeding before this PR, but somehow unrelated changes from the PR somehow also broke csky (?). Or maybe the error is swallowed because of some test bug 🤷

[lqd]

We'll see what the PR builder says about this one.

We could also take a look at CI's record of tests results with their ignore reason, if we have it until that far back, and check for csky there.

[lqd]

heh

missing LLVM component: csky

[nikic]

We should unset COMPILETEST_NEEDS_ALL_LLVM_COMPONENTS for the llvm-N jobs.

[RalfJung]

Oh, so on CI we actually ignore the needs-llvm-component part and run the test anyway?
That should be documented and mentioned in the error message, it is very confusing.
But it is good to know that we actually enforce that the test runs somewhere. If we unset COMPILETEST_NEEDS_ALL_LLVM_COMPONENTS, will the test be run anywhere at all?

[nikic]

We don't run the test, but we check that all components are present in CI test runs. And yes, the test will get run on builders using our own LLVM. But we don't control which targets Ubuntu's LLVM includes, so we shouldn't use the option there.

[RalfJung]

We don't run the test, but we check that all components are present in CI test runs.

If the component is present, then we also run the test, no?
Doesn't this mean the new check in #125472 is unnecessary, we would already have noticed when someone adds a non-existing component?

#125710 should help clarify the error we are seeing here.

But we don't control which targets Ubuntu's LLVM includes, so we shouldn't use the option there.

Ah so there are still other runners where COMPILETEST_NEEDS_ALL_LLVM_COMPONENTS is set?

[nikic]

Doesn't this mean the new check in #125472 is unnecessary, we would already have noticed when someone adds a non-existing component?

Yes-ish. I believe CI would fail, but having it in tidy is probably still a nice convenience to catch the issue during local development and PR CI already.

Ah so there are still other runners where COMPILETEST_NEEDS_ALL_LLVM_COMPONENTS is set?

See

rust/src/ci/run.sh

Lines 172 to 176 in 5870f1c

	# Unless we're using an older version of LLVM, check that all LLVM components
	# used by tests are available.
	if [ "$IS_NOT_LATEST_LLVM" = "" ]; then
	export COMPILETEST_NEEDS_ALL_LLVM_COMPONENTS=1
	fi

. It's currently set unless IS_NOT_LATEST_LLVM which is only set for the llvm-17 job. It's enabled everywhere else.

[bors]

☔ The latest upstream changes (presumably #125695) made this pull request unmergeable. Please resolve the merge conflicts.

[workingjubilee]

I am simply going to move the logic under $EXTERNAL_LLVM, since the answer to "does this LLVM have the components we need?" is "well, did we build it with them?"

[nikic]

@bors r+

[bors]

📌 Commit 60c30f5 has been approved by nikic

It is now in the queue for this repository.

[bors]

⌛ Testing commit 60c30f5 with merge 23ea77b...

[bors]

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 23ea77b to master...

[rust-timer]

Finished benchmarking commit (23ea77b): 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)

Results (primary -0.5%)

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)	3.8%	[2.2%, 5.4%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.2%	[-3.1%, -1.4%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-3.1%, 5.4%]	7

Cycles

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

Binary size

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

Bootstrap: 666.699s -> 669.162s (0.37%)
Artifact size: 318.90 MiB -> 318.80 MiB (-0.03%)

[erikdesjardins]

Hmm, I didn't realize that this was already caught in (non-PR) CI.

I'm inclined to revert the original PR. It's a rare enough issue that it just needs to be caught somehow, and it shouldn't be a huge issue that it takes a bors run to do so.