Helpers to generate python lockfile rules by lilatomic · Pull Request #17480 · pantsbuild/pants

lilatomic · 2022-11-07T03:36:14Z

Breakout from #16412

Adds helpers to generate common python lockfile rules. includes:

tool that can just use GeneratePythonLockfile.from_tool
tool that needs to gather interpreter constraints
tool that needs to install 1st-party plugins

I threw together some documentation, but I'm not sure where these would go or even if they're what we want.
I'm also not sure how we want to fail generating rules if the fields we need aren't present. For example, for tools which need interpreter constraints, their FieldSet needs to have an InterpreterConstraintsField attribute; how should we warn if that field isn't present?

Talking things over with @thejcannon , we think that we'd need to do some prework to support a design similar to the other rule generators. Having a declarative field on the class would be ideal. However, we would need to tether a tool to its fieldset to support usecases 2 and 3, which is where the prework would come in.
This MR now deals only with the first case. This is basically 60% of usecases and probably most of the uses in plugins.

thejcannon · 2023-01-09T15:50:48Z

Is there a chance I could convince you to rewrite history here so commits could be reviewed in order?
Something like:

First commit makes the core change
Second commit changes the core tests
Third commit is all the replacements out in the world

That'd make it much easier to review 😄

lilatomic · 2023-01-10T03:55:40Z

sure, makes sense. full history available here

benjyw · 2023-01-10T11:58:16Z

Will try and review properly today, but I love the idea so far! Take that, boilerplate.

lilatomic · 2023-01-11T19:05:16Z

I think that CI failure isn't related to this MR? (failure is in Scala)

stuhood · 2023-01-11T19:15:34Z

I think that CI failure isn't related to this MR? (failure is in Scala)

Yea, looks like a flake. Have added a category label and restarted this.

lilatomic · 2023-01-11T20:15:06Z

I'm not sure which test failed? there's 100% pass but it says it failed?

kaos · 2023-01-11T20:22:06Z

I'm not sure which test failed? there's 100% pass but it says it failed?

looks like another flake.. kicked it again.

benjyw

This looks great! And the proof of the pudding is in the eating. If all the tests pass, I'm happy pretty much by definition...

benjyw · 2023-01-10T11:58:56Z

docs/markdown/Writing Plugins/common-plugin-tasks/plugins-helpers.md

+    ...
+
+
+def rules():


Thanks for writing up these great docs!

benjyw · 2023-01-12T20:37:15Z

Throwing a couple more reviewers on, for perspective.

lilatomic · 2023-01-14T21:38:19Z

If all the tests pass, I'm happy pretty much by definition...

There weren't that many tests for this with the other tools. I guess since most of this was boilerplate, people didn't feel the need to test it? But with the tests for Cowsay and Flake8 I think we're well-covered.

benjyw · 2023-01-14T23:19:17Z

And presumably you've manually tested generate-lockfiles and so on?

lilatomic · 2023-01-15T01:03:34Z

I've run ./pants generate-lockfiles and then ./pants fmt test check lint --changed-since==main and none of the tools exploded.

benjyw · 2023-01-15T11:53:08Z

@thejcannon @kaos I'll merge this on Tuesday if no comments from you before then. Thanks!!

thejcannon · 2023-01-15T12:43:33Z

Yeah sorry, been in the hospital since my last comment. Tuesday is my first day back at work, so I'll try and squeeze in this PR.

benjyw · 2023-01-15T14:36:56Z

Not urgent!!

kaos

lgtm!

src/python/pants/backend/python/util_rules/lockfile.py

thejcannon

We're close, I can taste it. I can't thank you enough for getting this together, I'm VERY excited at the prospect of more boilerplate-reducers.

src/python/pants/backend/python/util_rules/lockfile.py

src/python/pants/backend/python/util_rules/lockfile_test.py

src/python/pants/testutil/rule_runner.py

src/python/pants/backend/python/util_rules/lockfile.py

src/python/pants/backend/python/util_rules/lockfile_test.py

thejcannon · 2023-02-15T19:17:22Z

src/python/pants/backend/python/util_rules/lockfile.py

+        if self == LockfileRules.NONE:
+            return
+
+        yield from lockfile.rules()


I still don't think this is necessary. I understand it's useful for tests, but let's not pollute the code and confuse authors (and removing this removes the need for None).

I thought so too. They're registered in the Python backend so they should be included. But the core doesn't register them, because it doesn't pull in the full python backend; it selectively pulls in YAPF and Black for updating build files. These would have then pulled in the lockfile.rules().
I've pulled this up to the root register in core, hopefully that gets the tests passing.

In that case, I think the black/yapf backends should be re-registering these rules.

they're included upstream, only need to be added for tests

TODO: port to generic tool

helps with debugging

helps for opt-in and also doesn't generate rules for unsupported types

These rules were transitively included through core/register.py -> core/goals/update_build_files.py -> backend/python/lint/yapf/subsystem.py

update_build_files goal is the one that uses them, so it should register them there.

…o_lockfile_request

resolves runtime import

They were already transitively imported through their subsystems. This MR pulls them up explicitly, so we need to allow that in the visibility rules

lilatomic · 2023-03-09T04:23:37Z

I think that's a flaky test? It seems to work fine locally...

benjyw · 2023-03-09T05:55:14Z

Yeah, that test is flaky, I will look into fixing or kililng it. Meanwhile I've restarted that shard.

benjyw · 2023-03-09T08:07:52Z

Merged, finally! Thanks again for the contribution and for your patience!

#17480 and #18431 collided, such that a new file was not re-formatted by the new version of `black`.

lilatomic force-pushed the feature/helper-lockfiles branch from 485e4c4 to 1773456 Compare November 7, 2022 06:20

lilatomic force-pushed the feature/helper-lockfiles branch from 5bf81e7 to cac4900 Compare November 17, 2022 05:24

lilatomic force-pushed the feature/helper-lockfiles branch from 55e8fe7 to 951ab9f Compare January 7, 2023 20:12

lilatomic marked this pull request as ready for review January 7, 2023 22:58

lilatomic force-pushed the feature/helper-lockfiles branch from b9a2e7f to 574f1f2 Compare January 10, 2023 03:51

lilatomic force-pushed the feature/helper-lockfiles branch from 574f1f2 to 3af161e Compare January 10, 2023 04:06

stuhood added the category:internal CI, fixes for not-yet-released features, etc. label Jan 11, 2023

benjyw approved these changes Jan 12, 2023

View reviewed changes

benjyw requested review from kaos and thejcannon January 12, 2023 20:36

kaos approved these changes Jan 15, 2023

View reviewed changes

src/python/pants/backend/python/util_rules/lockfile.py Outdated Show resolved Hide resolved

lilatomic force-pushed the feature/helper-lockfiles branch from 3af161e to 609bed2 Compare January 17, 2023 04:54

thejcannon requested changes Jan 17, 2023

View reviewed changes

huonw mentioned this pull request Jan 18, 2023

Simplify remote store load_bytes API #18034

Merged

lilatomic force-pushed the feature/helper-lockfiles branch from f17c103 to 39033ea Compare February 15, 2023 19:07

thejcannon reviewed Feb 15, 2023

View reviewed changes

lilatomic added 18 commits March 8, 2023 18:47

wip: port simple python lockfile from previous

2fe49ca

fix import cycles with TYPE_CHECKING imports

d876c11

remove lockfile.rules from helper

d079166

they're included upstream, only need to be added for tests

rollout

b7dd096

rename PYTHON->SIMPLE

6983a6f

fix Ruff test

b41f90f

TODO: port to generic tool

rename generated sentinel

02739d8

helps with debugging

change default behaviour to CUSTOM

d4d6ed0

helps for opt-in and also doesn't generate rules for unsupported types

add python lockfile rules to core

e7d99bc

These rules were transitively included through core/register.py -> core/goals/update_build_files.py -> backend/python/lint/yapf/subsystem.py

fix: actually use LockfileRules in test

4399477

push registering python lockfile rules closer to use

c96130c

update_build_files goal is the one that uses them, so it should register them there.

break import cycle

821c44d

pull relevant doc segment

c8f514c

move GeneratePythonLockfile.from_tool -> PythonToolRequirementsBase.t…

1cd33de

…o_lockfile_request

move lockfile helper types together with util rules

66c5ab3

resolves runtime import

fix import of Protocol in python3.7

4089817

remove outdated doc reference

0c46448

fill out other branches of default_rules matching

d120118

lilatomic force-pushed the feature/helper-lockfiles branch 2 times, most recently from 4164ab6 to a98e001 Compare March 9, 2023 03:14

allow lockfile rules to be explicitly imported

cd88e04

They were already transitively imported through their subsystems. This MR pulls them up explicitly, so we need to allow that in the visibility rules

lilatomic force-pushed the feature/helper-lockfiles branch from a98e001 to cd88e04 Compare March 9, 2023 03:20

benjyw merged commit 6b1b304 into pantsbuild:main Mar 9, 2023

stuhood mentioned this pull request Mar 9, 2023

Fix commit collision between #17480 and #18431. #18459

Merged

stuhood added a commit that referenced this pull request Mar 9, 2023

Fix commit collision between #17480 and #18431. (#18459)

9fb1e6c

#17480 and #18431 collided, such that a new file was not re-formatted by the new version of `black`.

Eric-Arellano mentioned this pull request Mar 11, 2023

Prepare 2.17.0.dev0 #18470

Merged

huonw mentioned this pull request May 5, 2023

Add lint backend to run semgrep #18593

Merged

Uh oh!

Conversation

lilatomic commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thejcannon commented Jan 9, 2023

Uh oh!

lilatomic commented Jan 10, 2023

Uh oh!

benjyw commented Jan 10, 2023

Uh oh!

lilatomic commented Jan 11, 2023

Uh oh!

stuhood commented Jan 11, 2023

Uh oh!

lilatomic commented Jan 11, 2023

Uh oh!

kaos commented Jan 11, 2023

Uh oh!

benjyw left a comment

Choose a reason for hiding this comment

Uh oh!

benjyw Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

benjyw commented Jan 12, 2023

Uh oh!

lilatomic commented Jan 14, 2023

Uh oh!

benjyw commented Jan 14, 2023

Uh oh!

lilatomic commented Jan 15, 2023

Uh oh!

benjyw commented Jan 15, 2023

Uh oh!

thejcannon commented Jan 15, 2023

Uh oh!

benjyw commented Jan 15, 2023

Uh oh!

kaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thejcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thejcannon Feb 15, 2023

Choose a reason for hiding this comment

Uh oh!

lilatomic Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

thejcannon Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

lilatomic commented Mar 9, 2023

Uh oh!

benjyw commented Mar 9, 2023

Uh oh!

benjyw commented Mar 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lilatomic commented Nov 7, 2022 •

edited

Loading