Add tidy check to error on new `Makefile`s in `tests/run-make` #122898

jieyouxu · 2024-03-22T18:47:34Z

We would like to strongly encourage new run-make tests to be written in Rust and not add any new Makefile-based run-make tests.

rustbot · 2024-03-22T18:47:42Z

r? @albertlarsan68

rustbot has assigned @albertlarsan68.
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

Noratrieb · 2024-03-22T18:55:55Z

src/tools/tidy/src/run_make_tests.rs

+        let allowed_makefiles = include!("expected_run_make_makefiles.txt");
+        let is_sorted = allowed_makefiles.windows(2).all(|w| w[0] < w[1]);


i think you messed up something here, you arent actually parsing the file
can you just reuse the code from the issues test stuff?

oh, include! does rust syntax, please leave a comment about that lol

Added a comment explaining why include! is used.

src/tools/tidy/src/main.rs

onur-ozkan

How often will this check be useful (finding newly created Makefiles)? I think it's so rare. How about creating a shell script (which can be used in mingw-check-tidy pipeline) that handles this? Making additions on tidy for this check seems like an overkill to me.

jieyouxu · 2024-03-24T16:07:45Z

How often will this check be useful (finding newly created Makefiles)? I think it's so rare.

There's already 3 instances where people weren't aware of the new infrastructure and tried to introduce new Makefiles, e.g. #122757, #122663, #122840, so it does happen.

How about creating a shell script (which can be used in mingw-check-tidy pipeline)

That sounds reasonable, though I'm not very keen on writing shell scripts :3 If this seems unnecessary, it's fine if I just occasionally remind people who introduce new Makefiles to use rmake.rs instead (after all, the plan is to get rid of all the Makefiles under run-make), and I'd imagine it's going to happen less as more run-make tests become rmake.rs instead of Makefiles.

onur-ozkan · 2024-03-24T16:57:57Z

I'd imagine it's going to happen less as more run-make tests become rmake.rs instead of Makefiles.

I thought the same and that's why I suggest creating shell script instead of bloating tidy.

jieyouxu · 2024-03-24T17:13:07Z

I thought the same and that's why I suggest creating shell script instead of bloating tidy.

Now that I think about it, it's probably not even worth the shell script, because we're planning to migrate away from Makefiles anyway, and if I add a shell script it invariably is going to require some allowlist for the Makefiles, which then means anyone who's porting the run-make tests have additional friction needing to adjust that. Given that, I'm going to close this PR, thanks for the advice!

EDIT: the tidy version (this version) supports --bless so the person porting run-make tests do not need to manually edit the allowlist if a Makefile is removed.

jieyouxu · 2024-03-27T19:54:33Z

I'm actually going to re-open this because a new Makefile slipped through in #118644.

How about creating a shell script (which can be used in mingw-check-tidy pipeline) that handles this?

The logic for handling entries in the allowlist for run-make tests is not exactly trivial to write or maintain in a shellscript (this includes checking for sorted entries, moved/removed entries, walking the run-make tests directory, blessing).

Furthermore, I would think that it's nicer for people working on PRs to see the "don't add a new Makefile" tidy error locally so it can be caught early, without having to rely on PR CI to then immediately fail anyway. For the people working on actually porting a run-make test, they should only need to --bless it without having to manually update the allowlist themselves.

jieyouxu · 2024-03-27T20:00:13Z

(Failing as intended, it's the new Makefile that slipped through)

albertlarsan68

A few comments

albertlarsan68 · 2024-03-22T18:56:18Z

src/tools/tidy/src/main.rs

@@ -103,6 +103,7 @@ fn main() {
        check!(tests_revision_unpaired_stdout_stderr, &tests_path);
        check!(debug_artifacts, &tests_path);
        check!(ui_tests, &root_path, bless);
+        check!(run_make_tests, &root_path, bless);


Why not use tests_path here, instead of creating it later from the root path?

(This was previously changed to check!(run_make_tests, &tests_path, &src_path, bless);)

albertlarsan68 · 2024-03-27T20:17:00Z

src/tools/tidy/src/allowed_run_make_makefiles.txt

@@ -0,0 +1,344 @@
+/*


I'm not fond of this being a rust file, but marked as .txt

I could just change this to a simple line-separated plain text file and not rust. FWIW, this is also what the ui test tidy check does.

Changed the allowlist txt file to be actually a plain-text file.

onur-ozkan · 2024-03-27T21:00:31Z

I agree that this check is useful, but I'm not sure if it's worth adding this complexity on tidy. I still think we can handle this using shell scripting script to perform the check in CI.

That sounds reasonable, though I'm not very keen on writing shell scripts :3

I can make a P.o.C PR in couple days if you don't want to do that.

jieyouxu · 2024-03-27T21:10:14Z

I can make a P.o.C PR in couple days if you don't want to do that.

(That'd be great, it's moreso that I don't know how to write this in shell script lol)

Although, if it's not written in tidy, would people porting run-make tests need to manually edit some allowlist file (i.e. no ./x test tidy --bless?)? Or is it going to look more like ./x run src/ci/some-shell-script.sh --bless?

albertlarsan68 · 2024-03-27T21:16:02Z

I want to point out that we have a check in tidy that the tests aren't added to src/test, even though it isn't really needed anymore.

I prefer the rust version over a shell version, just because it is more portable, and that we have most of the infrastructure needed to run this kind of test.
It is in the name, tidy is used to keep the repo well... Tidy.

workingjubilee · 2024-03-27T21:22:22Z

We can also justifiably remove the tidy check when we drive out our foe (Makefiles).

onur-ozkan · 2024-03-27T21:23:42Z

I can make a P.o.C PR in couple days if you don't want to do that.

(That'd be great, it's moreso that I don't know how to write this in shell script lol)

Although, if it's not written in tidy, would people porting run-make tests need to manually edit some allowlist file (i.e. no ./x test tidy --bless?)? Or is it going to look more like ./x run src/ci/some-shell-script.sh --bless?

Manually editing. It should be easy to remove a couple of lines (we will check if those lines exist on the filesystem anyway).

I want to point out that we have a check in tidy that the tests aren't added to src/test, even though it isn't really needed anymore.

I prefer the rust version over a shell version, just because it is more portable, and that we have most of the infrastructure needed to run this kind of test. It is in the name, tidy is used to keep the repo well... Tidy.

tidy is probably the most invoked command by everyone. To keep it light and quick, it shouldn't be bloated unless absolutely necessary. Maybe I am missing something about the check we are discussing; as I don't really see it as necessary to add to tidy.

albertlarsan68 · 2024-03-27T21:22:15Z

src/tools/tidy/src/run_make_tests.rs

+            tidy_error!(
+                bad,
+                "Makefile `{}` no longer exists and should be removed from the exclusions in \
+                `src/tools/tidy/src/allowed_run_make_makefiles.txt`",
+                p.display()
+            );


Maybe also point to --bless here?

Updated the message to point to --bless

albertlarsan68 · 2024-03-27T21:23:28Z

src/tools/tidy/src/run_make_tests.rs

+                tidy_error!(
+                    bad,
+                    "found run-make Makefile not permitted in \
+                `src/tools/tidy/src/allowed_run_make_makefiles.txt`, please write new run-make \
+                tests with `rmake.rs` instead: {}",
+                    entry.path().display()
+                );


Am I right that this error is not fixed by running with --bless?

Yes, that is the intention: that new Makefiles not in the allowlist will error, even if you pass --bless.

albertlarsan68 · 2024-03-27T21:28:34Z

tidy is probably the most invoked command by everyone. To keep it light and quick, it shouldn't be bloated unless absolutely necessary. Maybe I am missing something about the check we are discussing; as I don't really see it as necessary to add to tidy.

The check will become lighter over time, and we can remove it once the run-make with Makefiles infrastructure is removed.
Do you mind leaving a FIXME somewhere in the run-make code @jieyouxu ?

workingjubilee · 2024-03-27T21:29:39Z

@onur-ozkan I am not sure I see the point between tidy and another shell script here.

Anything that people should be running every time they commit or push a PR is gonna be in that hot path, and this won't catch anything if it doesn't run in that hot path, script or no, but it will require many more hours for the back-and-forth of "this should be fixed" "oh" re: a new Makefile appearing in a PR.

jieyouxu · 2024-03-27T21:33:25Z

Do you mind leaving a FIXME somewhere in the run-make code

Left a FIXME to remind removing this check once Makefiles are no longer accepted.

onur-ozkan · 2024-03-27T21:42:59Z

@onur-ozkan I am not sure I see the point between tidy and another shell script here.

Anything that people should be running every time they commit or push a PR is gonna be in that hot path, and this won't catch anything if it doesn't run in that hot path, script or no, but it will require many more hours for the back-and-forth of "this should be fixed" "oh" re: a new Makefile appearing in a PR.

The catch is rare (only when writing tests in a Makefile) and one CI failure should be enough for anyone to understand "you shouldn't write tests in Makefiles". A simple script can easily perform this check and can be removed right away without changing the codebase once we are done with the Makefiles. I don't want to talk about this indefinitely; it could be just my personal preference. Since most people here prefer the current approach, you can ignore my comments.

jieyouxu · 2024-03-27T21:45:42Z

The catch is rare (only when writing tests in a Makefile). A simple script can easily perform this check and can be removed right away without changing the codebase once we are done with the Makefiles. I don't want to talk about this indefinitely; it could be just my personal preference. Since most people here prefer the current approach, you can ignore my comments.

I have no strong opinions on this, as I can understand that if tidy is slow it will be very annoying. I personally don't know how to write a shell script version of this, but if someone writes a shell script version of this instead that'll work too.

albertlarsan68 · 2024-03-27T21:46:04Z

As we will have to remove the makefile-handling code, we can at the same time remove the tidy part.

r=me when CI green

jieyouxu · 2024-03-27T23:24:13Z

r=me when CI green

(You will need to r+ since I don't have r+)

albertlarsan68 · 2024-03-28T07:13:22Z

Thanks for the PR!
@bors r+ p=1 (this PR may fail because of the checks it adds, but enforces a new policy)

The r=me is also a signal to other r+ people that they can approve the PR in my name, not only to the author of the PR.

bors · 2024-03-28T07:13:25Z

📌 Commit 7a99be3 has been approved by albertlarsan68

It is now in the queue for this repository.

bors · 2024-03-28T07:38:58Z

⌛ Testing commit 7a99be3 with merge d0e8cbb...

bors · 2024-03-28T09:50:33Z

☀️ Test successful - checks-actions
Approved by: albertlarsan68
Pushing d0e8cbb to master...

rust-timer · 2024-03-28T11:56:19Z

Finished benchmarking commit (d0e8cbb): comparison URL.

Overall result: ❌✅ regressions and 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)	4.4%	[4.4%, 4.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.4%	[-1.4%, -1.4%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

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.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.7%, -0.5%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-0.7%, 0.5%]	3

Cycles

Results

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

Binary size

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

Bootstrap: 668.256s -> 669.796s (0.23%)
Artifact size: 315.62 MiB -> 315.69 MiB (0.02%)

rustbot assigned albertlarsan68 Mar 22, 2024

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Mar 22, 2024

jieyouxu force-pushed the tidy-no-new-makefiles branch from f5dbd19 to 1761d9e Compare March 22, 2024 18:53

Noratrieb reviewed Mar 22, 2024

View reviewed changes

the8472 reviewed Mar 22, 2024

View reviewed changes

src/tools/tidy/src/main.rs Outdated Show resolved Hide resolved

onur-ozkan reviewed Mar 24, 2024

View reviewed changes

jieyouxu closed this Mar 24, 2024

jieyouxu deleted the tidy-no-new-makefiles branch March 24, 2024 17:13

jieyouxu restored the tidy-no-new-makefiles branch March 27, 2024 19:50

jieyouxu reopened this Mar 27, 2024

This comment has been minimized.

Sign in to view

jieyouxu added 5 commits March 27, 2024 20:00

Add tidy check to error on new Makefiles in tests/run-make

f85063c

Add comment for include!() usage

ec25a42

Pass tests_path and src_path separately

7cf69a8

Rename expected_run_make_makefiles -> allowed_run_make_makefiles

81e3af7

Add run-make/link-framework/Makefile to allowlist

f10cf7b

jieyouxu force-pushed the tidy-no-new-makefiles branch from 7c03576 to f10cf7b Compare March 27, 2024 20:03

albertlarsan68 reviewed Mar 27, 2024

View reviewed changes

Use a line-separated plain-text allowlist instead

5089717

jieyouxu force-pushed the tidy-no-new-makefiles branch from 69eb315 to 5089717 Compare March 27, 2024 20:31

albertlarsan68 reviewed Mar 27, 2024

View reviewed changes

jieyouxu added 2 commits March 27, 2024 21:30

Point out you can use --bless to update allow list for removed entries

0f1bbe3

Add FIXME to remind removing the check once all tests are ported over

7a99be3

albertlarsan68 approved these changes Mar 27, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 28, 2024

bors merged commit d0e8cbb into rust-lang:master Mar 28, 2024
12 checks passed

rustbot added this to the 1.79.0 milestone Mar 28, 2024

jieyouxu deleted the tidy-no-new-makefiles branch April 1, 2024 21:28

		let allowed_makefiles = include!("expected_run_make_makefiles.txt");
		let is_sorted = allowed_makefiles.windows(2).all(\|w\| w[0] < w[1]);

Add tidy check to error on new Makefiles in tests/run-make #122898

Add tidy check to error on new Makefiles in tests/run-make #122898

Conversation

jieyouxu commented Mar 22, 2024

rustbot commented Mar 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

jieyouxu commented Mar 24, 2024 • edited Loading

onur-ozkan commented Mar 24, 2024

jieyouxu commented Mar 24, 2024 • edited Loading

jieyouxu commented Mar 27, 2024 • edited Loading

This comment has been minimized.

jieyouxu commented Mar 27, 2024

albertlarsan68 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan commented Mar 27, 2024

jieyouxu commented Mar 27, 2024 • edited Loading

albertlarsan68 commented Mar 27, 2024

workingjubilee commented Mar 27, 2024

onur-ozkan commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertlarsan68 commented Mar 27, 2024

workingjubilee commented Mar 27, 2024

jieyouxu commented Mar 27, 2024

onur-ozkan commented Mar 27, 2024 • edited Loading

jieyouxu commented Mar 27, 2024

albertlarsan68 commented Mar 27, 2024

jieyouxu commented Mar 27, 2024

albertlarsan68 commented Mar 28, 2024

bors commented Mar 28, 2024

bors commented Mar 28, 2024

bors commented Mar 28, 2024

rust-timer commented Mar 28, 2024

Overall result: ❌✅ regressions and improvements - no action needed

Add tidy check to error on new `Makefile`s in `tests/run-make` #122898

Add tidy check to error on new `Makefile`s in `tests/run-make` #122898

jieyouxu commented Mar 24, 2024 •

edited

Loading

jieyouxu commented Mar 24, 2024 •

edited

Loading

jieyouxu commented Mar 27, 2024 •

edited

Loading

albertlarsan68 left a comment •

edited

Loading

jieyouxu commented Mar 27, 2024 •

edited

Loading

onur-ozkan commented Mar 27, 2024 •

edited

Loading