Skip to content

Add eslint as part of tidy run #141705

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 30, 2025
Merged

Add eslint as part of tidy run #141705

merged 5 commits into from
May 30, 2025

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented May 28, 2025 •
edited by aDotInTheVoid
Loading

Rustdoc uses eslint to run lints on the JS files. Currently you need to run it by hand since it's not part of any x.py command. This PR makes it part of test tidy. However, to prevent having all rust developers to install npm and eslint, I made it optional: if eslint is not installed, then the check is simply skipped (but will tell that it is being skipped).

The second commit removes the manual checks from the docker file since eslint is run as part of tidy.

cc @lolbinarycat, #t-rustdoc > eslint seems to only be run in CI

@rustbot
Copy link
Collaborator

rustbot commented May 28, 2025

r? @Kobzol

rustbot has assigned @Kobzol.
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 A-tidy Area: The tidy tool 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) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels May 28, 2025
aDotInTheVoid
aDotInTheVoid reviewed May 28, 2025
View reviewed changes
src/ci/docker/host-x86_64/mingw-check/Dockerfile
@@ -65,7 +65,4 @@ ENV SCRIPT \
python3 ../x.py test collect-license-metadata && \
# Runs checks to ensure that there are no issues in our JS code.
es-check es2019 ../src/librustdoc/html/static/js/*.js && \
Copy link
Member

Choose a reason for hiding this comment

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

Feels weird to run eslint locally, but not tsc/es-check. Is there a reason for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

it is kinda odd, and i wouldn't mind fixing it, but we are at least fully compatible with the latest version of tsc, which is why i only complained about eslint.

@aDotInTheVoid aDotInTheVoid added the T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. label May 28, 2025
@lolbinarycat
Copy link
Contributor

my main concern is what happens if there is a too new version installed globally, as well as the correct version installed via npm install, do we find the correct version? i haven't checked the code for this yet, i just saw we were doing version detection.

Kobzol
Kobzol reviewed May 29, 2025
View reviewed changes
Copy link
Contributor

@Kobzol Kobzol left a comment •
edited
Loading

Choose a reason for hiding this comment

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

So, I like the implementation. There's just one problem with it - the checks won't be executed on CI at all now, unless I'm mistaken xD We only run tidy on a single job (mingw-check-tidy), which only runs on PR CI, not on full merge builds (I know, it's a mess), and that job doesn't have npm/nodejs installed, AFAIK, so effectively the tests won't be executed anywhere right now.

I think that we should just run the tidy job also on full merges, I'll ask on Zulip.

src/tools/tidy/src/rustdoc_js.rs Outdated

const ESLINT_VERSION: &str = "8.6.0";

pub fn check(librustdoc_path: &Path, tools_path: &Path, bad: &mut bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It really feels like we should add a npm lockfile to tidy, same as we have requirements.txt. But that can be done later.

@Kobzol
Copy link
Contributor

Kobzol commented May 29, 2025

Anyway, that discussion is a bit orthogonal to this PR. In any case, we need to make sure that npm is available on the mingw-check-tidy job. Could you please add NodeJS and eslint there? By copy-pasting from the mingw-check job.

@GuillaumeGomez
Copy link
Member Author

Added the installation of eslint into mingw-check-tidy dockerfile. And to prevent having the eslint version tracked from multiple places, I reused the same strategy as for browser-ui-test: a .version alongside the dockerfile which is used by both docker and tidy.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Kobzol
Copy link
Contributor

Kobzol commented May 29, 2025

Thank you, looks great! You can r=me once CI is green.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

@bors r=Kobzol rollup

@bors
Copy link
Collaborator

bors commented May 29, 2025

📌 Commit 8645ef7 has been approved by Kobzol

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 May 29, 2025
@GuillaumeGomez GuillaumeGomez mentioned this pull request May 29, 2025
Merged
bors added a commit that referenced this pull request May 29, 2025
@bors
Auto merge of #141739 - GuillaumeGomez:rollup-ivboqwd, r=GuillaumeGomez
de6db15 
Rollup of 11 pull requests

Successful merges:

 - #137574 (Make `std/src/num` mirror `core/src/num`)
 - #141384 (Enable review queue tracking)
 - #141448 (A variety of improvements to the codegen backends)
 - #141636 (avoid some usages of `&mut P<T>` in AST visitors)
 - #141676 (float: Disable `total_cmp` sNaN tests for `f16`)
 - #141705 (Add eslint as part of `tidy` run)
 - #141715 (Add `loongarch64` with `d` feature to `f32::midpoint` fast path)
 - #141723 (Provide secrets to try builds with new bors)
 - #141728 (Fix false documentation of FnCtxt::diverges)
 - #141729 (resolve target-libdir directly from rustc)
 - #141732 (creader: Remove extraenous String::clone)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request May 29, 2025
@bors
Auto merge of #141739 - GuillaumeGomez:rollup-ivboqwd, r=GuillaumeGomez
1ac1950 
Rollup of 11 pull requests

Successful merges:

 - #137574 (Make `std/src/num` mirror `core/src/num`)
 - #141384 (Enable review queue tracking)
 - #141448 (A variety of improvements to the codegen backends)
 - #141636 (avoid some usages of `&mut P<T>` in AST visitors)
 - #141676 (float: Disable `total_cmp` sNaN tests for `f16`)
 - #141705 (Add eslint as part of `tidy` run)
 - #141715 (Add `loongarch64` with `d` feature to `f32::midpoint` fast path)
 - #141723 (Provide secrets to try builds with new bors)
 - #141728 (Fix false documentation of FnCtxt::diverges)
 - #141729 (resolve target-libdir directly from rustc)
 - #141732 (creader: Remove extraenous String::clone)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a547af9 into rust-lang:master May 30, 2025
9 checks passed
@rustbot rustbot added this to the 1.89.0 milestone May 30, 2025
rust-timer added a commit that referenced this pull request May 30, 2025
@rust-timer
Unrolled build for #141705
7a6e778 
Rollup merge of #141705 - GuillaumeGomez:eslint-tidy, r=Kobzol

Add eslint as part of `tidy` run

Rustdoc uses `eslint` to run lints on the JS files. Currently you need to run it by hand since it's not part of any `x.py` command. This PR makes it part of `test tidy`. However, to prevent having all rust developers to install `npm` and `eslint`, I made it optional: if `eslint` is not installed, then the check is simply skipped (but will tell that it is being skipped).

The second commit removes the manual checks from the docker file since `eslint` is run as part of tidy.

cc `@lolbinarycat,` [#t-rustdoc > eslint seems to only be run in CI](https://rust-lang.zulipchat.com/#narrow/channel/266220-t-rustdoc/topic/eslint.20seems.20to.20only.20be.20run.20in.20CI/with/520761477)
@GuillaumeGomez GuillaumeGomez deleted the eslint-tidy branch May 30, 2025 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kobzol Kobzol Kobzol left review comments

@lolbinarycat lolbinarycat lolbinarycat left review comments

@aDotInTheVoid aDotInTheVoid aDotInTheVoid left review comments

Assignees

@Kobzol Kobzol

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc A-tidy Area: The tidy tool 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) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Milestone
1.89.0
Development

Successfully merging this pull request may close these issues.
7 participants
@GuillaumeGomez @rustbot @lolbinarycat @Kobzol @rust-log-analyzer @bors @aDotInTheVoid