Merge `validate` goal with `lint` goal #14102

Eric-Arellano · 2022-01-07T02:44:49Z

This adds generic support for lint implementations that do not deal with targets. That allows us to merge validate into lint, which is much cleaner.

CLI specs

As before with the validate goal, it's not very intuitive how to get Pants to run on files not owned by targets, which you want for validate. :: only matches files owned by targets, whereas ** matches all files regardless of targets.

So, users of regex-lint should typically use ./pants lint '**' rather than ./pants lint ::, which is not intuitive

https://docs.google.com/document/d/1WWQM-X6kHoSCKwItqf61NiKFWNSlpnTC5QNu3ul9RDk/edit#heading=h.1h4j0d5mazhu proposes changing :: to match all files, so you can simply use ./pants lint ::. I don't think we need to block on this proposal? This is still forward progress, and also validate/regex-lint is not used very much fwict.

Batching

We don't yet batch per #14186, although it would be trivial for us to hook up. I'm only waiting to do it till we can better reason about if it makes sense to apply here too.

The `fmt` goal

Note that we need more design for fmt before we can apply this same change there. fmt is tricky because we run each formatter for a certain language sequentially so that they don't overwrite each other; but we run distinct languages in parallel. We would need some way to know which "language" target-less files are for.

"Inferred targets"

A related technology would be inferred targets, where you don't need a BUILD flie but we still have a target: #14074.

This is a complementary technology. The main difference here is that we can operate on files that will never have an owning target, such as a BUILD file itself.

[ci skip-rust]
[ci skip-build-wheels]

Closes #12189. This also will allow us in #14102 to know whether to run `validate` as part of the `lint` goal or not. If it isn't configured, we'll skip it. [ci skip-rust] [ci skip-build-wheels]

…on].detail_level` (#14103) Prework for #14102. The `GoalSubsystem` will be going away so the option should live on the relevant subsystem instead. I'm not sure if `sourcefile-validation` is the best options scope for #14102, but I'm not sure what else we would call it. [ci skip-rust] [ci skip-build-wheels]

…k`, and `test` (#14303) Before: ``` ❯ ./pants --black-skip fmt src/python/pants/util/strutil.py 07:22:21.51 [INFO] Completed: Format with Autoflake - autoflake made no changes. 07:22:22.52 [INFO] Completed: Format with docformatter - Docformatter made no changes. 07:22:22.54 [INFO] Completed: Format with isort - isort made no changes. - Black skipped. ✓ Docformatter made no changes. ✓ autoflake made no changes. ✓ isort made no changes. ``` After: ``` ❯ ./pants --black-skip fmt src/python/pants/util/strutil.py 07:22:01.24 [INFO] Completed: Format with Autoflake - autoflake made no changes. 07:22:01.41 [INFO] Completed: Format with docformatter - Docformatter made no changes. 07:22:01.70 [INFO] Completed: Format with isort - isort made no changes. ✓ Docformatter made no changes. ✓ autoflake made no changes. ✓ isort made no changes. ``` As we've added more tools, rendering skipping is somewhat noisy. More importantly, it causes a problem when tools want to disable themselves via some complex logic that requires the Rules API. For example, with Go, we don't know if a `go_package` has tests in it or not until compiling the code and running our analyzer: https://github.com/pantsbuild/pants/blob/0488605f54ef3602797d68a5c4797dea9d706b14/src/python/pants/backend/go/goals/test.py#L192-L193 With the upcoming `regex-lint` (formerly `validate`), we want to only enable the linter if `[regex-lint].config` is set up: #14102. In both these cases, it is annoying & noisy to render the skipped tools in the summary. I wasn't anticipating these use cases when designing this summary in 2020: #9710. We will still log that the tool is skipped at `-ldebug`, which helps if users are confused why something isn't showing up. [ci skip-rust] [ci skip-build-wheels]

[ci skip-rust] [ci skip-build-wheels]

stuhood

Thanks! Even with the gotchas, I think that this is a step in the right direction, as it will let us incrementally improve.

stuhood · 2022-01-29T18:40:19Z

src/python/pants/backend/project_info/regex_lint.py

+            "The `regex-lint` check run by the `validate` goal is now run as part of the "
+            "`lint` goal. So long as you have set up `[regex-lint].config` "
+            "(or the deprecated `[sourcefile-validation].config`), this checker will run.\n\n"
+            "Note that if you were running `validate '**'` in order to check files without "
+            "owning targets (e.g. to check BUILD file contents), then you will need to run "
+            "`lint '**'` rather than `lint ::`. Also, `--changed-since=<sha>` will not cause "
+            "`regex-lint` to run, same as it how `validate` worked. We are exploring how to remove "
+            "both these gotchas."


Is it worth referring to a ticket in the help as part of the explanation of the gotchas? Improving the --changed behavior is possibly related to #13757 (i.e., the need to refactor Specs calculation to move it into a single call to the engine).

I think the most relevant thing is my design doc from November. Should I reference that? Fwit, that remains one of the most pressing projects I personally want to work on.

I think it's fine as is. We've always had these gotchas since adding validate in 2020.

stuhood · 2022-01-29T19:23:48Z

src/python/pants/core/goals/lint.py

@@ -199,43 +212,63 @@ async def lint(
    console: Console,
    workspace: Workspace,
    targets: Targets,
+    specs_snapshot: SpecsSnapshot,
    lint_subsystem: LintSubsystem,


Commenting in the wrong place, but: is it possible to update required_union_implementations for this case? Seems like it would need both AND and OR perhaps.

This has been a problem for generate-lockfiles, too. We want to OR whereas it's implemented as AND.

stuhood

Thanks! Even with the gotchas, I think that this is a step in the right direction, as it will let us incrementally improve.

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

…-merge [ci skip-build-wheels]

[ci skip-rust] [ci skip-build-wheels]

#14102 added `LintFilesRequest`, so now `LintRequest` is poorly named. I don't yet rename `FmtRequest` to `FmtTargetsRequest` because that API is still in flux until we figure out how to safely have target-less formatters. No need to rename `CheckRequest` because we need dependencies information there, so we will need targets (possibly inferred). [ci skip-rust] [ci skip-build-wheels]

This was referenced Jan 7, 2022

Deprecate [validate].detail_level in favor of [sourcefile-validation].detail_level #14103

Merged

Fix validate goal crashing if not configured #14104

Merged

Eric-Arellano mentioned this pull request Jan 14, 2022

Plugin development tool to check names, such as Field class names. #13799

Open

Eric-Arellano force-pushed the lint-validate-merge branch from b7ed49e to edfd648 Compare January 15, 2022 00:10

Eric-Arellano mentioned this pull request Jan 29, 2022

Add SpecsPaths #14301

Closed

Eric-Arellano force-pushed the lint-validate-merge branch from edfd648 to 206c91b Compare January 29, 2022 00:56

Eric-Arellano mentioned this pull request Jan 29, 2022

Don't render skipped tools in final summary with fmt, lint, check, and test #14303

Merged

Eric-Arellano force-pushed the lint-validate-merge branch from 206c91b to 588a05a Compare January 29, 2022 17:02

Eric-Arellano changed the title ~~[wip] Merge validate goal with lint goal~~ Merge validate goal with lint goal Jan 29, 2022

Eric-Arellano marked this pull request as ready for review January 29, 2022 17:06

Merge validate goal with lint goal

796094c

[ci skip-rust] [ci skip-build-wheels]

Eric-Arellano force-pushed the lint-validate-merge branch from 588a05a to 796094c Compare January 29, 2022 17:10

Update CI to not use validate

d32b136

Eric-Arellano requested review from stuhood, kaos and benjyw January 29, 2022 17:41

stuhood approved these changes Jan 29, 2022

View reviewed changes

Eric-Arellano enabled auto-merge (squash) January 29, 2022 19:46

Fix test

ba4244d

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano force-pushed the lint-validate-merge branch from b7e7b5f to ba4244d Compare January 29, 2022 19:55

Eric-Arellano mentioned this pull request Jan 29, 2022

Add fmt --style-only to avoid semantic changes like PyUpgrade and Autoflake #14309

Closed

Eric-Arellano added 2 commits January 29, 2022 14:11

Merge branch 'main' of github.com:pantsbuild/pants into lint-validate…

8f941c7

…-merge [ci skip-build-wheels]

Actually fix test

c9cc709

[ci skip-rust] [ci skip-build-wheels]

Eric-Arellano merged commit 2127fb1 into pantsbuild:main Jan 29, 2022

Eric-Arellano deleted the lint-validate-merge branch January 29, 2022 21:59

Eric-Arellano mentioned this pull request Jan 29, 2022

Rename LintRequest to LintTargetsRequest #14312

Merged

Eric-Arellano mentioned this pull request May 18, 2022

--owners-not-found-behavior no longer makes sense with target-less goals #15529

Closed

Eric-Arellano mentioned this pull request Aug 11, 2022

Add support for targetless formatters #16480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge `validate` goal with `lint` goal #14102

Merge `validate` goal with `lint` goal #14102

Eric-Arellano commented Jan 7, 2022 •

edited

Loading

stuhood left a comment

stuhood Jan 29, 2022

Eric-Arellano Jan 29, 2022

Eric-Arellano Jan 29, 2022

stuhood Jan 29, 2022

Eric-Arellano Jan 29, 2022

stuhood left a comment

Merge validate goal with lint goal #14102

Merge validate goal with lint goal #14102

Conversation

Eric-Arellano commented Jan 7, 2022 • edited Loading

CLI specs

Batching

The fmt goal

"Inferred targets"

stuhood left a comment

Choose a reason for hiding this comment

stuhood Jan 29, 2022

Choose a reason for hiding this comment

Eric-Arellano Jan 29, 2022

Choose a reason for hiding this comment

Eric-Arellano Jan 29, 2022

Choose a reason for hiding this comment

stuhood Jan 29, 2022

Choose a reason for hiding this comment

Eric-Arellano Jan 29, 2022

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Merge `validate` goal with `lint` goal #14102

Merge `validate` goal with `lint` goal #14102

Eric-Arellano commented Jan 7, 2022 •

edited

Loading

The `fmt` goal