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

Don't skip all directories when tidy-checking #109440

Merged
merged 3 commits into from
Mar 28, 2023

Conversation

WaffleLapkin
Copy link
Member

This fixes a regression from #108772 which basically made it that tidy style checks only README.md and COMPILER_TESTS.md.

@rustbot
Copy link
Collaborator

rustbot commented Mar 21, 2023

r? @ozkanonur

(rustbot has picked a reviewer for you, use r? to override)

@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. labels Mar 21, 2023
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Whoops, sorry 😅 good catch.

I don't think we need to pass is_dir into every check though, that calls path.metadata a lot more than it used to. It should be enough to call path.is_dir in the style check only.

@WaffleLapkin
Copy link
Member Author

WaffleLapkin commented Mar 21, 2023

that calls path.metadata a lot more than it used to

The code never calls path.metadata... e.file_type().map(|ft| ft.is_dir()).unwrap_or(false) is all just field accesses, if I'm not missing something.

@jyn514
Copy link
Member

jyn514 commented Mar 21, 2023

Ah I see, walkdir has already called metadata() before us and it's passing us that info through the file type.

Ok, I still have a mild preference for the smaller change, but this is fine too - r=me with or without it.

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.

lgtm

@onur-ozkan
Copy link
Member

onur-ozkan commented Mar 21, 2023

I still have a mild preference for the smaller change, but this is fine too - r=me with or without it.

@WaffleLapkin Are you planning to make any change on the PR? I will queue the PR otherwise.

Also please squash the commits.

@WaffleLapkin
Copy link
Member Author

WaffleLapkin Are you planning to make any change on the PR? I will queue the PR otherwise.

@ozkanonur I don't... Maybe a better solution would be to pass the ...Entry everywhere, but maybe not, idk. Anyway don't really want to work on this atm.

@bors r=jyn514,ozkanonur

@bors
Copy link
Contributor

bors commented Mar 21, 2023

📌 Commit 285fec8 has been approved by jyn514,ozkanonur

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 21, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2023
…yn514,ozkanonur

Don't skip all directories when tidy-checking

This fixes a regression from rust-lang#108772 which basically made it that tidy style checks only `README.md` and `COMPILER_TESTS.md`.
@matthiaskrgr
Copy link
Member

@bors r- rollup=iffy
looks like more cleanup is needed now

fmt check
tidy check
tidy: Skipping binary file check, read-only filesystem
Error: tidy error: /checkout/compiler/rustc_error_codes/src/error_codes/E0080.md:19: line longer than 80 chars
Error: tidy error: /checkout/src/doc/unstable-book/src/compiler-flags/sanitizer.md:216: unexplained "```ignore" doctest; try one:

* make the test actually pass, by adding necessary imports and declarations, or
* use "```text", if the code is not Rust code, or
* use "```compile_fail,Ennnn", if the code is expected to fail at compile time, or
* use "```should_panic", if the code is expected to fail at run time, or
* use "```no_run", if the code should type-check but not necessary linkable/runnable, or
* explain it like "```ignore (cannot-test-this-because-xxxx)", if the annotation cannot be avoided.


Error: tidy error: /checkout/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs:571: \
Use a single space after dots in comments.
Error: tidy error: /checkout/tests/ui/const-generics/generic_const_exprs/typeid-equality-by-subtyping.rs:20: trailing whitespace

https://github.com/rust-lang/rust/actions/runs/4497868288/jobs/7913878216#step:26:1777

#109513

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 23, 2023
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 23, 2023
@WaffleLapkin
Copy link
Member Author

I've blessed tidy, but note that some PRs from #109513 are still not merged and conflict with this...

@bors r=jyn514,ozkanonur

@bors
Copy link
Contributor

bors commented Mar 23, 2023

📌 Commit 1b1410df127034f0c754350f54189d5c3d4db858 has been approved by jyn514,ozkanonur

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2023
@bors
Copy link
Contributor

bors commented Mar 24, 2023

⌛ Testing commit 1b1410df127034f0c754350f54189d5c3d4db858 with merge 0fa3f2220f21ffb8dce4e24ebc8895fc8ccf8997...

@bors
Copy link
Contributor

bors commented Mar 24, 2023

💔 Test failed - checks-actions

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 24, 2023
@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 27, 2023
@bors
Copy link
Contributor

bors commented Mar 27, 2023

⌛ Testing commit b7a0b06f6bc849011753ab7b2919d013240c9157 with merge 597f4b0cf55e5c04a0c62358142748d45f79d97f...

@bors
Copy link
Contributor

bors commented Mar 27, 2023

💔 Test failed - checks-actions

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

This comment has been minimized.

@WaffleLapkin
Copy link
Member Author

@bors retry

@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 27, 2023
@jyn514
Copy link
Member

jyn514 commented Mar 27, 2023

@bors r- r+

retry means something different :( not sure why it didn't print the commit it was going to retry

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 27, 2023
@bors
Copy link
Contributor

bors commented Mar 27, 2023

📌 Commit 904dd2c has been approved by jyn514

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 27, 2023
@bors
Copy link
Contributor

bors commented Mar 27, 2023

⌛ Testing commit 904dd2c with merge 5bf139e...

@bors
Copy link
Contributor

bors commented Mar 28, 2023

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 5bf139e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 28, 2023
@bors bors merged commit 5bf139e into rust-lang:master Mar 28, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 28, 2023
@jyn514
Copy link
Member

jyn514 commented Mar 28, 2023

yay!!! thank you @WaffleLapkin for sticking with this ❤️ i know it was frustrating

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5bf139e): 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.6% [-0.6%, -0.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.6% [-0.6%, -0.6%] 1

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
Regressions ❌
(secondary)
4.6% [4.6%, 4.6%] 1
Improvements ✅
(primary)
-1.2% [-1.2%, -1.2%] 1
Improvements ✅
(secondary)
-3.4% [-4.3%, -2.5%] 2
All ❌✅ (primary) -1.2% [-1.2%, -1.2%] 1

Cycles

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

@WaffleLapkin WaffleLapkin deleted the make_tidy_slower branch March 28, 2023 08:35
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 26, 2023
…jyn514

Change tidy error message for TODOs

Blocked on rust-lang#109440 (first few commits are from where)

IMO "deprecated" doesn't really explain anything, I've tried to highlight the actual reason we error on TODOs. The message is not at all perfect, maybe someone has ideas how to phrase it better?
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants