Migrate link-arg, link-dedup and issue-26092 run-make tests to rmake format

[Oneirical]

Part of #121876 and the associated Google Summer of Code project.

All of these tests check if rustc's output contains (or does not) contain certain strings. Does that mean these could be better suited to becoming UI/codegen tests?

try-job: x86_64-msvc

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[jieyouxu]

Yes, I think it would be good to try converting them to UI tests

[jieyouxu]

@rustbot author

[Oneirical]

Unfortunately, porting these tests to UI tests turned out to be non-trivial:

• link-dedup is not only looking for strings in the output, but precisely that strings appear in a specific order and exactly once.
• link-arg is passing a flag to the compiler twice in a row.
• no-panic-blank-output is reliant on passing a weird flag to the compiler (blank output) which can interfere with the default flags used in UI tests.

It's very possible that some existing UI test features would allow supporting these tests in UI form, but it seems to me that there is a reason they were run-make tests in the first place.

Of all 3, link-arg looks the most susceptible of being UI-ified, but I am not sure if it's worth trying to force the conversion. Tell me what you think.

@rustbot review

[rustbot]

Some changes occurred in run-make tests.

cc @jieyouxu

[Oneirical]

Some strange test failures here. Perhaps link-dedup was not truly equivalent to the original test in the final line:

$(RUSTC) empty.rs 2>&1 | $(CGREP) -v '"-ltesta" "-ltesta" "-ltesta"'

let output = String::from_utf8(rustc().input("empty.rs").command_output().stderr).unwrap();
assert_eq!(output.matches("-ltesta").count(), 1);

I have tried replacing it with an inefficient alternative just to see if it makes the test pass, though this is likely a bad implementation.

let output = rustc().input("empty.rs").command_output().stderr;
let pattern = b"\"-ltesta\" \"-ltesta\" \"-ltesta\"";
for window in output.windows(pattern.len()) {
    if window == pattern {
        panic!("ltesta was repeated too many times");
    }
}

As for the other error, it's quite strange - the rewrite seems really equivalent to the original Makefile. It could be interesting to debug and see what the output is actually printing.

[jieyouxu]

@Oneirical becareful with the grep -v flag:

grep -v "foo" takes input line by line, and outputs only the lines in which "foo" does not appear.

So if the input to grep -v "foo" contains "foo", then it will exit with status 1 failing the test. I think this is checking that there is in fact, no matches. Needs double checking though.

[jieyouxu]

@rustbot author

[Oneirical]

@Oneirical becareful with the grep -v flag:

grep -v "foo" takes input line by line, and outputs only the lines in which "foo" does not appear.

So if the input to grep -v "foo" contains "foo", then it will exit with status 1 failing the test. I think this is checking that there is in fact, no matches. Needs double checking though.

That is what I thought too, hence the assert!(!output.contains("-ltestb"));.

However, the last one is the confusing one:

$(CGREP) -v '"-ltesta" "-ltesta" "-ltesta"'

This is testing that the string "-ltesta" "-ltesta" "-ltesta" never appears, with the quotation marks as part of the output, I suppose... Let me try it with assert!(!output.contains("\"-ltesta\" \"-ltesta\" \"-ltesta\""));

EDIT: Actually, most of this logic was really overkill, let's try something much simpler.

[jieyouxu]

Thanks, looks good in general, just have a few nits.

[jieyouxu]

Suggestion: I think we really want to expand these asserts to verbosely log the stdout and the expected/unexpected, but that can be done in a follow-up PR.

[Oneirical]

Actually, that's already the case (this PR was not rebased fully, I don't know why it didn't complain about merge conflicts). Only the assert_x_equals need verbose logging.

[Oneirical]

I chose to add it right away, as it's fairly trivial.

[jieyouxu]

Question: where did these formatting diffs from come?

[Oneirical]

Just me waving around autoformatting a little too liberally, I have restored it.

[jieyouxu]

@rustbot author

[Oneirical]

@rustbot review

[jieyouxu]

Thanks, just a few minor nits left, then r=me.

[jieyouxu]

I found some more nits... Sorry!

[jieyouxu]

Question: I think this needs an .arg("-")

[Oneirical]

Testing it locally did not result in any problems.

[jieyouxu]

Cool 😎

[jieyouxu]

After fixing the nits, can you try to run ./x doc src/tools/run-make-support to check if the intra-doc link passes? Otherwise it fails very late into the CI pipeline.

[Oneirical]

After fixing the nits, can you try to run ./x doc src/tools/run-make-support to check if the intra-doc link passes? Otherwise it fails very late into the CI pipeline.

Indeed, it failed, because the correct way to write it is /// Prefer to use [Self::run] and [Self::run_fail]. (with backticks) Fixed.

[Oneirical]

Thanks, just a few minor nits left, then r=me.

@bors r=@jieyouxu

[bors]

📌 Commit 6228b3e has been approved by jieyouxu

It is now in the queue for this repository.

[bors]

⌛ Testing commit 6228b3e with merge 59e2c01...

[bors]

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 59e2c01 to master...

[rust-timer]

Finished benchmarking commit (59e2c01): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.3%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-0.4%, -0.3%]	6

Max RSS (memory usage)

Results (primary 0.9%, secondary -3.4%)

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)	0.9%	[0.9%, 0.9%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-3.4%, -3.4%]	1
All ❌✅ (primary)	0.9%	[0.9%, 0.9%]	1

Cycles

Results (secondary -0.9%)

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)	-	-	0
Regressions ❌ (secondary)	4.5%	[3.4%, 6.3%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.2%	[-3.9%, -2.6%]	7
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 669.558s -> 670.994s (0.21%)
Artifact size: 320.53 MiB -> 320.50 MiB (-0.01%)