tidy: move rustdoc js stuff into a tidy extra check #142924
Conversation
rustbot
added
A-testsuite
|
This PR modifies If appropriate, please update There are changes to the cc @jieyouxu This PR modifies If appropriate, please update |
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #142929) made this pull request unmergeable. Please resolve the merge conflicts. |
| @@ -382,7 +382,7 @@ pub enum Subcommand { | |||
| bless: bool, | |||
| #[arg(long)] | |||
| /// comma-separated list of other files types to check (accepts py, py:lint, | |||
| /// py:fmt, shell) | |||
| /// py:fmt, shell, cpp, cpp:fmt, js, js:lint, js:typecheck, js:es-check) | |||
There was a problem hiding this comment.
Do we need to separate type checking and eslint? It's essentially the same category of "checking code" to me, at least in the world of JavaScript. Unless it runs for too long, I'd just collapse both into "lint" or "check". When you develop on JS, you'd set your bootstrap.toml file to contain both anyway, I imagine.
There was a problem hiding this comment.
tsc generally checks correctness, while eslint mostly checks style... thought i suppose if you want fast typechecking, it would be best to run tsc directly...
also calling an extra check "check" feels unhelpful.
| # Runs checks to ensure that there are no issues in our JS code. | ||
| es-check es2019 ../src/librustdoc/html/static/js/*.js && \ | ||
| tsc --project ../src/librustdoc/html/static/js/tsconfig.json | ||
| src/tools/tidy --extra-checks=js |
There was a problem hiding this comment.
Since this is now part of tidy, I think that we can move it out of this CI runner and just put it into mingw-check-tidy, to have centralized execution of all tidy checks on one place.
| @@ -224,6 +248,18 @@ fn check_impl( | |||
| shellcheck_runner(&merge_args(&cfg_args, &file_args_shc))?; | |||
| } | |||
|
|
|||
| if js_lint { | |||
| rustdoc_js::lint(librustdoc_path, tools_path, src_path)?; | |||
There was a problem hiding this comment.
Didn't we say that the checks would be run if the JS files were modified too? Having them never run unless an extra arguments is passed is very inconvenient.
There was a problem hiding this comment.
it wasn't too long ago that they were never run locally.
i was gonna have a warning for modifying files and not enabling their extra checks....
i suppose i could add a new "auto" value to --extra-checks, and make that the default.
i would prefer a design which still lets people manually disable these checks if they want.
There was a problem hiding this comment.
Well, if you modify JS files, seems like you do want these checks by default, no?
There was a problem hiding this comment.
What if you don't have the tools installed, and have limited or no network connectivity, but still want to run tidy?
I agree that running the tools based on modification is a good default, but there's very little cost to letting people disable it.
I think I would want to add 3 new special values, actually:
all, run everything, possibly used in CI (currently shellcheck isn't run in CI, so we would need to do that first).auto, run checks for modified filesnone, run no extra checks, same as the empty string
This would give a robust system that would be easy to add any future checks into, and would also help improve the existing checks, instead of just improving js checks.
| @@ -171,7 +170,17 @@ fn main() { | |||
| }; | |||
| check!(unstable_book, &src_path, collected); | |||
|
|
|||
| check!(ext_tool_checks, &root_path, &output_directory, bless, extra_checks, pos_args); | |||
| check!( | |||
| ext_tool_checks, | |||
There was a problem hiding this comment.
Why is this being added to a single check module? AFAICS ext_tools is running external programs serially, so the wall-time of all those tools adds up. The check! macro exists to run things in threads so they can run in parallel to cut down on walltime.
There was a problem hiding this comment.
It's in ext_tools because that's where --extra-checks is parsed. I definitely agree that the parsing logic and stuff could use a big upgrade, and that tools should be run concurrently, but I think that can be done within ext_tools. It doesn't even technically need threads, just to spawn processes then wait for them all simultaneously. I could get working on a system for that if you want. It would probably involve making a struct or trait to represent an extra-check instead of having it be ad-hoc. This would also be a good opportunity to refactor the parsing.
There was a problem hiding this comment.
Well, the macro also limits concurrency to obey -j, so having thread per check is the easiest way to get that.
There was a problem hiding this comment.
So, should I inline every --extra-check as its own call to check!, probably doubling the size of main() in the process, or do you have a better idea?
There was a problem hiding this comment.
Well, before doing more work I think it'd make sense to check how long it takes to run all the commands and compare that to the overall runtime of tidy. If it's the longest chain then parallelizing makes sense.
lolbinarycat
force-pushed
the
tidy-js-extra-checks
branch
from
575be8c to
b023137
Compare
Most of these were factored out of CI scripts, but
eslintin particular was previously implemented with its own special cased logic.A new option has been added to bootstrap,
build.tidy-extra-checks, which serves as a default value for the--extra-checksflag. This is mostly for the benefit of rustdoc js maintainers, but should also help bootstrap py maintainers.Additionally,
--extra-checks=cpphas been documented.I'm not super happy with how long the extra check names are in comparison to the others (in particular
typecheck), but I couldn't think of anything better (I didn't want to name ittscon the off chance we want to switch to a different typechecking engine in the future).It would be nice to convert the extra checks arg into a proper enum, both for warning on unknown values and to provide better shell completion.
r? @GuillaumeGomez