Add support for tidy linting via external tools for non-rust files #112482

tgross35 · 2023-06-10T03:12:18Z

This change adds the flag --check-extras to tidy. It accepts a comma separated list of any of the options:

py (test everything applicable for python files)
py:lint (lint python files using ruff)
py:fmt (check formatting for python files using black)
shell or shell:lint (lint shell files using shellcheck)

Specific files to check can also be specified via positional args. Examples:

./x test tidy --check-extras=shell,py
./x test tidy --check-extras=py:fmt -- src/bootstrap/bootstrap.py
./x test tidy --check-extras=shell -- src/ci/*.sh
Python formatting can be applied with bless: ./x test tidy --ckeck-extras=py:fmt --bless

ruff and black need to be installed via pip; this tool manages these within a virtual environment at build/venv. shellcheck needs to be installed on the system already.

This PR doesn't fix any of the errors that show up (I will likely go through those at some point) and it doesn't enforce anything new in CI. Relevant zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/Other.20linters.20in.20CI

rustbot · 2023-06-10T03:12:26Z

r? @Mark-Simulacrum

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

pietroalbini · 2023-06-10T07:41:10Z

src/tools/tidy/src/ext_tool_checks.rs

+        *pip_checked = true;
+    }
+
+    let status = Command::new(py_path).args(["-m", "pip", "install", &bin_vers]).status()?;


Installing any version could lead to differences between users. What I recommend is:

Add a requirements.txt somewhere in the source tree, pinning the versions of the tools we use (ideally with pip-compile --generate-hashes from a requirements.in)

When creating a virtualenv, install the requirements.txt file and copy the file itself into the virtualenv

In any future tidy invocation, check if the requirements.txt in the source tree matches the one copied into the virtualenv, otherwise discard the whole virtualenv and create a new one from scratch.

I do like your process and will update to do that so versions aren't hard coded. But what differences are possible as-is? bin_vers is a pinned version e.g. ruff==0.0.272, and the version is verified before running https://github.com/rust-lang/rust/blob/1d05b0dbb4ce2e0afa82d63518443ba96aaf7888/src/tools/tidy/src/ext_tool_checks.rs#L248-L251

Before we do a bunch of work with pinning requirements, I think it's useful to take a step back and ask whether we want this checking at all. I'm not convinced I want to put contributors on the hook for fixing lints and formatting in the majority of our Python and shell code. If it's not checked in CI then I'm not sure it adds enough value for us to then take on the burden of maintaining this code and getting PRs trying to fix issues these linters will warn about.

tgross35 · 2023-06-10T18:26:47Z

(following up with @Mark-Simulacrum's comment at top level)

That of course makes sense. I think it would be pretty reasonable to enforce ruff linting in CI: correct code should generally have very few errors, the things that do show up can potentially be serious errors, and even if you're only reading CI output it's usually pretty easy to figure out what to fix. #112499 fixes all the current errors, it's a fairly small patchset and it finds some real gotchas (fn_arg={}acting as globals, exceptions ignoring ctrl+c, lazy loop bindings).

Enforcement of black could make sense either way, since you do need this one installed. I think this PR does a pretty good job of lowering the bar for what setup is needed ("just works" as long as you have some python+pip+venv available - likely true for anyone working on python files), and means it's no more burden then running rustfmt via tidy like you do for any changes. I see it more of a convenience of being able to format entire files without thinking, rather than a burdon of needing to do it - but of course defer to you here.

shellcheck.... there are a lot of errors. Maybe it would make sense to enforce in CI but only error or error+warning levels? I do intend to start a PR to fix at correct the issues of those levels.

It is your call for what you do and don't want to enforce, but I am happy to put in the legwork to make that happen.

tgross35 · 2023-06-10T18:46:35Z

All three tools can actually output diffs of their suggested changes - if CI enforcement is a yes, I'll add a way to print those on error when running on GH.

Kobzol · 2023-06-11T07:12:09Z

"just works" as long as you have some python+pip+venv available

Just note that when you pin a version of a package, you basically also pin the version of (C)Python, version support sometimes moves fast and it's not always easy to install a set of pinned packages on a different version of Python than the one on which the versions were pinned.

tgross35 · 2023-06-11T07:26:29Z

That is a good point. I think it's a fairly unlikely issue since black checks multiple versions in CI and ruff is probably pretty version-independent (written in Rust), but we can keep an eye out for problems.

This patch makes sure it's using python 3.7+ which is supported by both tools (and prefers newer versions), but 3.7 is going EOL in a couple weeks....

pietroalbini · 2023-06-11T09:32:13Z

I think at least for black+ruff it would make sense to enforce them in CI. They both provide hint in the CI output on how to fix them, and if someone changes so much Python code that applying the suggestions get bothersome, they should have a working Python toolchain locally.

If we get lots of people complaining about this in a month or so we can reconsider, but this is also a change that's really quick to revert.

Mark-Simulacrum · 2023-06-11T13:06:50Z

@pietroalbini What value do you see in enforcing formatting for our Python? My sense is that lints may make sense (looking at #112499, there's quite a bit more than I expected, though it's hard for me to gauge severity with my knowledge of Python), but I don't see as much value in formatting the limited set of code. I don't think we've ever had problems with formatting and needing to re-push after X minutes just because you had a slightly different idea of formatting is painful in my experience.

I don't think we can expect folks to have Python toolchains (with pip etc.) locally, even if they are modifying a good chunk of Python code.

pietroalbini · 2023-06-18T11:05:18Z

What value do you see in enforcing formatting for our Python?

On one side, having a consistent style across the codebase is good, like we do for Rust. The more important point for me though is being able to run autoformatting in your editor without having to disable it for rust-lang/rust.

pietroalbini · 2023-07-06T07:45:36Z

@bors rollup=never

src/tools/tidy/src/ext_tool_checks.rs

tgross35 · 2023-07-06T09:10:26Z

Thanks @pietroalbini! Feel free to ping me with whatever might be decided at that point

I just updated a few things to get this to pass with the latest master so I think I need your r+ again. Only changes were removing the comments that @klensy mentioned, and fixing the mutable default args in bootstrap and bootstrap_test 64b2f00

tgross35 · 2023-07-11T19:24:20Z

Looks like bors didn't correct the labels when this got bumped

@rustbot ready

tgross35 · 2023-07-11T19:24:52Z

Probably won't let me do this one but we'll try...

@rustbot label -S-waiting-on-bors

tgross35 · 2023-07-17T16:38:57Z

@pietroalbini would you be able to redo your r+?

Since you had previously approved, I only needed to reapply the new rustfmt options, and fix a few lints that had happened since #112499 - but no serious changes.

bors · 2023-07-31T17:08:32Z

☔ The latest upstream changes (presumably #113592) made this pull request unmergeable. Please resolve the merge conflicts.

This change adds the flag `--check-extras` to `tidy`. It accepts a comma separated list of any of the options: - py (test everything applicable for python files) - py:lint (lint python files using `ruff`) - py:fmt (check formatting for python files using `black`) - shell or shell:lint (lint shell files using `shellcheck`) Specific files to check can also be specified via positional args. Examples: - `./x test tidy --check-extras=shell,py` - `./x test tidy --check-extras=py:fmt -- src/bootstrap/bootstrap.py` - `./x test tidy --check-extras=shell -- src/ci/*.sh` - Python formatting can be applied with bless: `./x test tidy --ckeck-extras=py:fmt --bless` `ruff` and `black` need to be installed via pip; this tool manages these within a virtual environment at `build/venv`. `shellcheck` needs to be installed on the system already.

- Remove unneeded imports in 'fuscia-test-runner.py' - Add explicit stacklevel to 'x.py' - Fix mutable types as default args in `bootstrap.py` and `bootstrap_test.py`

pietroalbini · 2023-08-10T10:44:31Z

@bors r+

Sorry for the delay!

bors · 2023-08-10T10:44:33Z

📌 Commit 9df0f5d has been approved by pietroalbini

It is now in the queue for this repository.

bors · 2023-08-10T13:07:22Z

⌛ Testing commit 9df0f5d with merge 9fa6bdd...

bors · 2023-08-10T15:46:31Z

☀️ Test successful - checks-actions
Approved by: pietroalbini
Pushing 9fa6bdd to master...

rust-timer · 2023-08-10T17:08:43Z

Finished benchmarking commit (9fa6bdd): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-4.1%, -2.8%]	2
All ❌✅ (primary)	2.4%	[2.4%, 2.4%]	1

Cycles

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

Binary size

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

Bootstrap: 632.004s -> 632.25s (0.04%)

Fix python linting errors These were flagged by `ruff`, run using the config in rust-lang/rust#112482

rustbot assigned Mark-Simulacrum Jun 10, 2023

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 Jun 10, 2023

This comment has been minimized.

Sign in to view

tgross35 force-pushed the ci-non-rust-linters branch from a044fc4 to 1d05b0d Compare June 10, 2023 04:04

pietroalbini reviewed Jun 10, 2023

View reviewed changes

tgross35 mentioned this pull request Jun 10, 2023

Fix python linting errors #112499

Merged

tgross35 force-pushed the ci-non-rust-linters branch from 1d05b0d to a57d303 Compare June 10, 2023 20:03

Mark-Simulacrum assigned pietroalbini and unassigned Mark-Simulacrum Jun 17, 2023

tgross35 force-pushed the ci-non-rust-linters branch 2 times, most recently from 318187f to 44cea52 Compare June 17, 2023 08:23

rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Jun 17, 2023

This comment has been minimized.

Sign in to view

tgross35 force-pushed the ci-non-rust-linters branch from 44cea52 to d2b5e1f Compare June 17, 2023 08:33

This comment has been minimized.

Sign in to view

tgross35 force-pushed the ci-non-rust-linters branch from d2b5e1f to ac59754 Compare June 17, 2023 08:45

This comment has been minimized.

Sign in to view

tgross35 force-pushed the ci-non-rust-linters branch 2 times, most recently from db53f24 to a35950a Compare June 19, 2023 05:01

This comment has been minimized.

Sign in to view

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 Jul 6, 2023

klensy reviewed Jul 6, 2023

View reviewed changes

src/tools/tidy/src/ext_tool_checks.rs Outdated Show resolved Hide resolved

klensy reviewed Jul 6, 2023

View reviewed changes

src/tools/tidy/src/ext_tool_checks.rs Outdated Show resolved Hide resolved

tgross35 force-pushed the ci-non-rust-linters branch from b48ec03 to 64b2f00 Compare July 6, 2023 09:03

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 11, 2023

rustbot removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 11, 2023

tgross35 force-pushed the ci-non-rust-linters branch from 64b2f00 to 91590df Compare July 17, 2023 16:16

tgross35 added 2 commits August 2, 2023 04:40

Fix recent python linting errors

9df0f5d

- Remove unneeded imports in 'fuscia-test-runner.py' - Add explicit stacklevel to 'x.py' - Fix mutable types as default args in `bootstrap.py` and `bootstrap_test.py`

tgross35 force-pushed the ci-non-rust-linters branch from 91590df to 9df0f5d Compare August 2, 2023 08:41

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 Aug 10, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 10, 2023

bors merged commit 9fa6bdd into rust-lang:master Aug 10, 2023

rustbot added this to the 1.73.0 milestone Aug 10, 2023

tgross35 deleted the ci-non-rust-linters branch August 10, 2023 17:52

antoyo pushed a commit to rust-lang/rustc_codegen_gcc that referenced this pull request Aug 13, 2023

Rollup merge of #112499 - tgross35:py-ruff-fixes, r=Mark-Simulacrum

ab1e4bb

Fix python linting errors These were flagged by `ruff`, run using the config in rust-lang/rust#112482

thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Aug 24, 2023

Rollup merge of #112499 - tgross35:py-ruff-fixes, r=Mark-Simulacrum

2d73c4b

Fix python linting errors These were flagged by `ruff`, run using the config in rust-lang/rust#112482

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for tidy linting via external tools for non-rust files #112482

Add support for tidy linting via external tools for non-rust files #112482

tgross35 commented Jun 10, 2023 •

edited

Loading

rustbot commented Jun 10, 2023

This comment has been minimized.

pietroalbini Jun 10, 2023

tgross35 Jun 10, 2023

Mark-Simulacrum Jun 10, 2023

tgross35 commented Jun 10, 2023

tgross35 commented Jun 10, 2023 •

edited

Loading

Kobzol commented Jun 11, 2023

tgross35 commented Jun 11, 2023

pietroalbini commented Jun 11, 2023

Mark-Simulacrum commented Jun 11, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pietroalbini commented Jun 18, 2023

This comment has been minimized.

pietroalbini commented Jul 6, 2023

tgross35 commented Jul 6, 2023 •

edited

Loading

tgross35 commented Jul 11, 2023

tgross35 commented Jul 11, 2023

tgross35 commented Jul 17, 2023

bors commented Jul 31, 2023

pietroalbini commented Aug 10, 2023

bors commented Aug 10, 2023

bors commented Aug 10, 2023

bors commented Aug 10, 2023

rust-timer commented Aug 10, 2023

Add support for tidy linting via external tools for non-rust files #112482

Add support for tidy linting via external tools for non-rust files #112482

Conversation

tgross35 commented Jun 10, 2023 • edited Loading

rustbot commented Jun 10, 2023

This comment has been minimized.

pietroalbini Jun 10, 2023

Choose a reason for hiding this comment

tgross35 Jun 10, 2023

Choose a reason for hiding this comment

Mark-Simulacrum Jun 10, 2023

Choose a reason for hiding this comment

tgross35 commented Jun 10, 2023

tgross35 commented Jun 10, 2023 • edited Loading

Kobzol commented Jun 11, 2023

tgross35 commented Jun 11, 2023

pietroalbini commented Jun 11, 2023

Mark-Simulacrum commented Jun 11, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pietroalbini commented Jun 18, 2023

This comment has been minimized.

pietroalbini commented Jul 6, 2023

tgross35 commented Jul 6, 2023 • edited Loading

tgross35 commented Jul 11, 2023

tgross35 commented Jul 11, 2023

tgross35 commented Jul 17, 2023

bors commented Jul 31, 2023

pietroalbini commented Aug 10, 2023

bors commented Aug 10, 2023

bors commented Aug 10, 2023

bors commented Aug 10, 2023

rust-timer commented Aug 10, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

tgross35 commented Jun 10, 2023 •

edited

Loading

tgross35 commented Jun 10, 2023 •

edited

Loading

tgross35 commented Jul 6, 2023 •

edited

Loading