tidy: only run eslint on JS files if specified with "--extra-checks"

[yotamofek]

npx already obviates the need to have the package installed locally, we can just ask npx to run the pre-configured version of eslint

This should reduce some friction for rustdoc contributors who are using tidy.
The one downside I can think of is that this will make skipping the eslint check more difficult to do, but not sure there's a good use-case for that.

r? @GuillaumeGomez since you added the eslint check to tidy :)

[GuillaumeGomez]

Seems good to me, thanks! r=me once CI pass.

@bors delegate+

[bors]

✌️ @yotamofek, you can now approve this pull request!

If @GuillaumeGomez told you to "r=me" after making some further change, please make that change, then do @bors r=@GuillaumeGomez

[yotamofek]

@bors r=@GuillaumeGomez rollup

[bors]

📌 Commit 986b270 has been approved by GuillaumeGomez

It is now in the queue for this repository.

[workingjubilee]

The one downside I can think of is that this will make skipping the eslint check more difficult to do, but not sure there's a good use-case for that.

...I am not modifying the JS files in this repository.

[workingjubilee]

I am going to uninstall Node.js from my machines, I guess.

[Mark-Simulacrum]

@bors r-

How much extra time does this add for the tidy execution for folks who do have npx? We've had numerous attempts to optimize this flow, and I think the majority of contributors aren't editing JavaScript so this is just wasted time for them.

Maybe we can make this an opt-in in bootstrap.toml or perhaps detect changed files or similar?

[yotamofek]

The one downside I can think of is that this will make skipping the eslint check more difficult to do, but not sure there's a good use-case for that.

...I am not modifying the JS files in this repository.

Yeah, sorry, didn't phrase that correctly at all. I meant that not having eslint globally (or locally) installed isn't really a proper way to opt out of this check, since it's a kinda arbitrary thing that might be dependent on whether you by chance have some other JS project you're working on. NVM.

Your modified title isn't quite right, this PR doesn't change the way eslint is invoked (which was already via npx), just the check that is performed before.

[yotamofek]

@bors r-

How much extra time does this add for the tidy execution for folks who do have npx? We've had numerous attempts to optimize this flow, and I think the majority of contributors aren't editing JavaScript so this is just wasted time for them.

Maybe we can make this an opt-in in bootstrap.toml or perhaps detect changed files or similar?

This PR actually seems to make tidy run faster on my machine, I think asking npm to generate the list of installed packages was quite expensive.

But yeah, making the eslint opt-in (or only run when actually relevant) is probably a good idea.

[workingjubilee]

Your modified title isn't quite right, this PR doesn't change the way eslint is invoked (which was already via npx), just the check that is performed before.

Oh, well, yes, but the step that is npm install is replaced by simply directly running npx eslint, it just is replacing it like...

(if (npm install package) then (npx package)) => (npx package)

The original title just seemed slightly vague. Feel free to modify the PR title further or even revert it to your original if you disagree, I won't touch it again.

[jieyouxu]

Hi, can we please gate the eslint stuff behind --extra-checks, that is why the extra checks exist, because most contributors to this repository won't need or want to run eslint because they don't work on js files, even if they have node or npx locally for other reasons.

You can see e.g.

rust/src/tools/tidy/src/ext_tool_checks.rs

Lines 62 to 66 in d4e1159

	// Split comma-separated args up
	let lint_args = match extra_checks {
	Some(s) => s.strip_prefix("--extra-checks=").unwrap().split(',').collect(),
	None => vec![],
	};

, which also similarly gates python linting or C++ code formatting behind the --extra-checks flag.

Invocation wise, I would expect it be sth like ./x test tidy --extra-checks=js:lint. We can document that in the rustdoc contributing chapter in rustc-dev-guide.

In one of the CI mingw-check-* jobs, we should also incl. --extra-checks=js:lint.

[jieyouxu]

Doing this as part of --extra-checks also has an additional benefit -- I would expect ./x test tidy --extra-checks=js:lint to hard-error if it cannot find a suitable npx or eslint locally¹, because that is now an opt-in flow the contributor explicitly indicated their intent for, whereas currently we skip w/ a warning everyone will see.

Footnotes

1. If possible, also note what versions are expected because there's too many versions :D ↩

[jieyouxu]

Also hold on, will this try to use any eslint version, and not a pinned version?

[workingjubilee]

@jieyouxu It should always try to use this version but I am not sure what the dependency resolution process for npx is from here, e.g. if it will use a compatible version that isn't the specified one.

[jieyouxu]

Ah right... yeah.

[yotamofek]

It will always resolve to that exact version specified, there's no implicit semver operator here like cargo does :)

[yotamofek]

Haha. Weird bug. I love it when GH contradicts itself 😁

[yotamofek]

@jieyouxu mind taking a look please?
Moved the invocation of eslint to be gated by "--extra-checks"

[GuillaumeGomez]

Please don't. I don't want to gate eslint checks behind an option, too easy to miss.

[yotamofek]

Please don't. I don't want to gate eslint checks behind an option, too easy to miss.

I agree it's easy to miss, but I also agree with what other people were saying about it being a bit of a time sink (though pretty minimal) for the vast majority of people who aren't working on the rustdoc frontend.
Maybe the best option is to gate it on the detection of modified JS files? That sounds like the sweet spot to me.

Anyways, these are the options I see:

• just close this PR, leave things as-is (eslint is run through npx only if the correct version is already installed)
• revert PR to my original intent (directly run npx without checking for an existing installation of eslint)
• leave PR in current state (gate eslint on "--extra-checks" flag)
• run eslint only if we detect that a JS file was modified

Whatever is decided, I'm happy to make the needed modifications, but seems to me like there needs to be some consensus first?

[jieyouxu]

I don't feel strongly enough to block running this in the default flow (I guess I could just uninstall node from my env too), but I would prefer if we gate eslint

• Behind --extra-checks=js (like is currently done), or
• Behind a bootstrap option, or
• Behind modified js files somehow.

Otherwise, you bear the linting cost for having node installed even if you don't work on the js files...

[GuillaumeGomez]

Then let's keep it as it is currently? If eslint is not installed at the right version, then nothing happens. It's longer and a bit subpar but it prevents all listed issues from here.

[yotamofek]

Then let's keep it as it is currently? If eslint is not installed at the right version, then nothing happens. It's longer and a bit subpar but it prevents all listed issues from here.

Sounds good.
I'm contemplating between simply closing this PR and adding some comment in the code about why we're requiring eslint to be installed (e.g. "it's a poor man's opting-out mechanism") to prevent the next person here from getting nerd sniped like me.

[GuillaumeGomez]

Adding comments and even a link to this PR would be a very good idea!

[Kobzol]

I agree with @jieyouxu, detecting the version is not a good approach. It is not intuitive, to disable the check, you have to uninstall eslint, which you might want to have for other reasons, and if you have a different version, it fails tidy. The Python linting in tidy instead manages a virtualenv for you and installs the right version of packages, which is what we should do. Also, if you get a linting error once on CI, and then you install the correct eslint version locally, you'll then get worse tidy performance going forward.

We already have strong precedent for gating Python/C++ checks behind an optional flag. I don't see why we should do an exception for JS, it's in a similar category. For people who work on JS often, you can just invoke tidy with the extra check or write a bash script that will do it for you. With the opt-in, tidy can actually error out when eslint is missing on CI, which it currently doesn't; if eslint disappears from the CI image, we will silently stop running the linter now, which is not great. We will also stop checking the installed version of eslint, which is a hack anyway, and it makes running tidy slower.

[GuillaumeGomez]

Interesting point: you have the opposite approach but it also makes sense. My worry is that if we need to add an extra argument to tidy, it's very likely that except for CI checks, no one will remember it and will run it. Maybe we should instead detect if a change happened in rustdoc JS files and run tidy only in that case if we're not in CI?

[yotamofek]

I agree that running it if we detected changed JS files is the ideal option: for non-JS-contributors it will minimize tidy's run time, for the few people actually working on the rustdoc frontend, it will run eslint without necessitating they remember to add a flag every time they run tidy.

Question is, is the "changed file detection" mechanism trustworthy?
Up until a while ago, can't remember exactly when, whenever I ran tidy I would see ~5k files detected as changed (and a warning saying something like "if this number seems high, try git clean" or something).

[GuillaumeGomez]

Well, kinda. We already use this mechanism in stuff like #142677

[Kobzol]

The 5k message was mostly a UX failure. The detection should be fairly robust now.

If the check for modified files is fast, I wouldn't mind it. Although it is not 100% to just check if *.js files have changed, you still might have changed in the lockfile, the version of eslint, the code of tidy itself, etc. But it would get you 99% there.

[lolbinarycat]

Up until a while ago, can't remember exactly when, whenever I ran tidy I would see ~5k files detected as changed (and a warning saying something like "if this number seems high, try git clean" or something).

Pretty sure I wrote that warning! The issue was if origin/master was stale, I believe. No idea about the further improvements though, does origin/master get automatically updated by default in git pull somehow now?

Anyways, I'm gonna take a crack at this, as well as adding tsc as an extra check, and then either adding a bootstrap.toml option for default extra checks, or adding a warning to tidy for if a file is modified but the extra checks for it haven't been run (will also be nice for anyone editing python scripts).

UPDATE: fairly sure none of the warnings show unless you run with --no-capture? Unless I've somehow just missed all of them.

UPDATE: also wanna throw es-check in there too

[Kobzol]

No idea about the further improvements though, does origin/master get automatically updated by default in git pull somehow now?

We actually completely removed the remote handling, because it was unreliable and requires CI hacks. Now we just find the most recent commit authored by bors, that seems to be much more robust.

[lolbinarycat]

We actually completely removed the remote handling, because it was unreliable and requires CI hacks. Now we just find the most recent commit authored by bors, that seems to be much more robust.

just got fmt: formatted 6117 modified files, though I am unsure if this is because I modified tidy or something.

[Kobzol]

To clarify, I think that the remote handling was used both for detecting download-ci-XXX commits and also the rustfmt check. The download-ci checking is now working well, but I don't remember how exactly the rustfmt check works.

[lolbinarycat]

Here's my attempt: #142924