Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Mar 28, 2024

Conversation

jieyouxu
Copy link
Member

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
Copy link
Collaborator

rustbot commented Mar 22, 2024

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

@rustbot 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
Comment on lines 12 to 14
let allowed_makefiles = include!("expected_run_make_makefiles.txt");
let is_sorted = allowed_makefiles.windows(2).all(|w| w[0] < w[1]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment explaining why include! is used.

src/tools/tidy/src/main.rs Outdated Show resolved Hide resolved
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

jieyouxu commented Mar 24, 2024

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
Copy link
Member

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
Copy link
Member Author

jieyouxu commented Mar 24, 2024

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 jieyouxu closed this Mar 24, 2024
@jieyouxu jieyouxu deleted the tidy-no-new-makefiles branch March 24, 2024 17:13
@jieyouxu jieyouxu restored the tidy-no-new-makefiles branch March 27, 2024 19:50
@jieyouxu jieyouxu reopened this Mar 27, 2024
@jieyouxu
Copy link
Member Author

jieyouxu commented Mar 27, 2024

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.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member Author

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

Copy link
Member

@albertlarsan68 albertlarsan68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -0,0 +1,344 @@
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@onur-ozkan
Copy link
Member

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
Copy link
Member Author

jieyouxu commented Mar 27, 2024

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
Copy link
Member

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
Copy link
Member

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

@onur-ozkan
Copy link
Member

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.

Comment on lines 80 to 85
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()
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also point to --bless here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the message to point to --bless

Comment on lines +52 to +58
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()
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@albertlarsan68
Copy link
Member

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
Copy link
Member

@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
Copy link
Member Author

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
Copy link
Member

onur-ozkan commented Mar 27, 2024

@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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

r=me when CI green

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

@albertlarsan68
Copy link
Member

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
Copy link
Contributor

bors commented Mar 28, 2024

📌 Commit 7a99be3 has been approved by albertlarsan68

It is now in the queue for this repository.

@bors 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
Copy link
Contributor

bors commented Mar 28, 2024

⌛ Testing commit 7a99be3 with merge d0e8cbb...

@bors
Copy link
Contributor

bors commented Mar 28, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 28, 2024
@bors bors merged commit d0e8cbb into rust-lang:master Mar 28, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 28, 2024
@rust-timer
Copy link
Collaborator

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%)

@jieyouxu jieyouxu deleted the tidy-no-new-makefiles branch April 1, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants