allow plugins to provide "auxiliary" goals (and refactor BSP into backends) #20913

tdyas · 2024-05-13T15:37:14Z

Allow plugins to provide "auxiliary" goals via the newly-introduced auxiliary_goals function in the plugin registration module. This function returns an iterable of AuxiliaryGoal subclasses representing auxiliary goals which are run outside of the engine.

The first user for this mechanism is the Build Server Protocol support which previously required the experimental-bsp goal to be a BuiltinGoal and thus live in the Pants core rules. With the new auxiliary goal support, the BSP rules can now live in several new optional backends and not in the core rules.

See related discussion in #20931.

The BuiltinGoal class intentionally remains for the "builtin goals" in the core like the help system. AuxiliaryGoal uses a context class instead of function parameters like BuiltinGoal for its run method in order to be more extensible in the future.

sureshjoshi · 2024-05-15T03:45:25Z

What's the use case for a plug in providing "built-in goals" rather than regular goal rules?

Feels like BuiltinGoal would no-longer be the correct term for these.

tdyas · 2024-05-15T04:18:55Z

What's the use case for a plug in providing "built-in goals" rather than regular goal rules?

Feels like BuiltinGoal would no-longer be the correct term for these.

BuiltinGoal is probably misnamed since it is for goals which run without full option parsing (e.g., the help system). For BSPGoal, it needs to take over stdin and stdout when the Pants BSP implementation runs in "server" mode.

EarlyGoal is probably a better name.

I was not trying to rename BuiltinGoal with this PR though. Should I be?

sureshjoshi · 2024-05-15T12:06:06Z

I was not trying to rename BuiltinGoal with this PR though. Should I be?

Probably not.

But we should probably carve out a ticket/discussion about this - as once this lands, BuiltIn goal becomes part of the usable public API, need docs around naming, why to use this vs goal_rule, etc etc blah blah.

tdyas · 2024-05-15T17:40:31Z

Opened discussion: #20931

huonw · 2024-06-05T05:48:42Z

We've just branched for 2.22, so merging this pull request now will come out in 2.23, please move the release notes updates to docs/notes/2.23.x.md. Thank you!

sureshjoshi · 2024-06-24T03:16:14Z

@tdyas Just to loop back on this. From the discussion, it doesn't seem like there is any pushback to the feature - just a bike shed on Builtin for something that a plugin provides...

Does the bike shed need to get figured out? Or is that something we can punt on?

tdyas · 2024-06-26T03:56:47Z

@tdyas Just to loop back on this. From the discussion, it doesn't seem like there is any pushback to the feature - just a bike shed on Builtin for something that a plugin provides...

Does the bike shed need to get figured out? Or is that something we can punt on?

I pushed my current progress on a DaemonGoal API and retaining BuiltinGoal for the help goals. I will appreciate people's feedback.

Some points:

I switched the API to use a context dataclass instead of passing in parameters. I believe this should provide better support for DaemonGoal.run implementations supporting multiple Pants versions by being able to check for attributes before accessing them if need be. It also allows us to implement complex getters on DaemonGoalContext if the need arises for compatibility reasons.
I have not yet modeled passing stdin/stdout to the DaemonGoal. So for now, BSP still deals with sys.stdin / sys.stdout.

sureshjoshi · 2024-07-02T12:48:33Z

Does DaemonGoal here imply that the usage is a long-running "server-ish" mechanism? Like a BSP or LSP?

Or can I still run a one-off command (e.g. pants plugin init --foo --bar or pants migrate --from 2.17 --to 2.21) - both of which might need to happen before goal_rule?

tdyas · 2024-07-24T23:12:50Z

Does DaemonGoal here imply that the usage is a long-running "server-ish" mechanism? Like a BSP or LSP?

Or can I still run a one-off command (e.g. pants plugin init --foo --bar or pants migrate --from 2.17 --to 2.21) - both of which might need to happen before goal_rule?

It could still run one-off commands like the ones you suggest. DaemonGoal is what I thought had been suggested in the GitHub discussion; although I see you made this point in #20931 (reply in thread).

Any suggestions of a better name?

tdyas · 2024-07-24T23:29:58Z

I ended up talking to the Claude AI and it gave me some nice suggestions: #20931 (reply in thread)

tdyas · 2024-07-25T01:28:23Z

Renamed to AuxillaryGoal. I liked what the AI suggested.

cognifloyd

You've got a typo in auxiliary.

s/uxillary/uxiliary/

And a couple comments still mention "daemon".

Do we want auxiliary and builtin goals to have the same precedence?

src/python/pants/goal/auxillary_goal.py

cognifloyd · 2024-07-25T17:01:48Z

src/python/pants/goal/auxillary_goal.py

+    When multiple auxillary goals are presented, the first auxillary goal will be used unless there is a
+    auxillary goal that begin with a hyphen (`-`), in which case the last such "option goal" will be
+    prioritized. This is to support things like `./pants some-builtin-goal --help`.


Should auxiliary goals support --opt "option goals"? I lean towards no to keep the exposed API simpler...

I had not intended on changing the BuiltinGoal API for AuxiliaryGoal. I pretty much did not have to disturb any of the existing option parsing logic for this PR thankfully. While the suggestion would simplify the public API, it would entail modifying the existing logic which has been undisturbed thus far and risks introducing bugs. Thoughts?

src/python/pants/goal/auxillary_goal.py

tdyas · 2024-07-25T20:23:18Z

Do we want auxiliary and builtin goals to have the same precedence?

Good question. I would expect them to be disjoint. An auxiliary goal should not be able to shadow a builtin goal. In that sense, builtin goals have precedence.

tdyas · 2024-07-25T23:20:09Z

Latest commits should fix the typo and the remaining instances of "daemon" goal in comments.

Issues addressed

src/python/pants/goal/auxiliary_goal.py

tdyas · 2024-08-06T01:38:21Z

The PR now actually invokes auxiliary goals (and proves that fact with an integration test).

src/python/pants/bin/BUILD

Co-authored-by: Jacob Floyd <cognifloyd@gmail.com>

The refactor in #20913 to move BSP out of the core rules (via the new `AuxiliaryGoal` support) did not move the registration of BSP-related `QueryRule`s out of the core rules. This PR remedies that oversight by moving the `QueryRule` registration to the applicable BSP backends.

tdyas added the category:new feature label May 13, 2024

tdyas force-pushed the builtin_goals_in_plugins branch from 6f48967 to 9aba8db Compare May 13, 2024 17:12

tdyas added 3 commits May 22, 2024 13:32

:allow plugins to provide builtin goals

0896dfa

release notes

9dee824

switch back commented code

a501625

tdyas added 5 commits June 19, 2024 22:09

Merge branch 'main' into builtin_goals_in_plugins

6a168cf

move release notes to 2.23.x

5b6adef

Merge branch 'main' into builtin_goals_in_plugins

c39baee

checkpoint

d44ac54

checkpoint

392e1bc

tdyas added 2 commits June 25, 2024 23:38

Merge branch 'main' into builtin_goals_in_plugins

46c626a

more progress on DaemonGoal API

e50f128

tdyas force-pushed the builtin_goals_in_plugins branch from 9aba8db to e50f128 Compare June 26, 2024 03:50

tdyas added 2 commits July 24, 2024 20:06

Merge branch 'main' into builtin_goals_in_plugins

6accd00

rename to AuxillaryGoal

bb2ebfd

tdyas changed the title ~~allow plugins to provide builtin goals and refactor BSP into backends~~ allow plugins to provide "auxillary"goals (and refactor BSP into backends) Jul 25, 2024

tdyas marked this pull request as ready for review July 25, 2024 01:36

tdyas requested review from cognifloyd and huonw July 25, 2024 01:36

tdyas requested a review from sureshjoshi July 25, 2024 01:37

tdyas changed the title ~~allow plugins to provide "auxillary"goals (and refactor BSP into backends)~~ allow plugins to provide "auxillary" goals (and refactor BSP into backends) Jul 25, 2024

fix comment

638a421

cognifloyd previously requested changes Jul 25, 2024

View reviewed changes

tdyas added 3 commits July 25, 2024 18:21

Merge branch 'main' into builtin_goals_in_plugins

51d62a5

rename auxillary (typo!) to auxiliary

1d31ea5

cleanup some references to "daemon" goals

4ab5bd2

kaos changed the title ~~allow plugins to provide "auxillary" goals (and refactor BSP into backends)~~ allow plugins to provide "auxiliary" goals (and refactor BSP into backends) Jul 26, 2024

tdyas requested a review from cognifloyd July 26, 2024 12:52

cognifloyd reviewed Jul 27, 2024

View reviewed changes

src/python/pants/goal/auxiliary_goal.py Outdated Show resolved Hide resolved

src/python/pants/goal/auxiliary_goal.py Show resolved Hide resolved

Merge branch 'main' into builtin_goals_in_plugins

1844e25

tdyas marked this pull request as draft July 27, 2024 02:11

tdyas added 2 commits August 5, 2024 21:36

actually invoke auxiliary goals + integration test

c9b96d3

Merge branch 'main' into builtin_goals_in_plugins

de15570

tdyas marked this pull request as ready for review August 6, 2024 01:37

tdyas requested a review from cognifloyd August 6, 2024 01:55

cognifloyd reviewed Aug 6, 2024

View reviewed changes

src/python/pants/bin/BUILD Outdated Show resolved Hide resolved

tdyas added 2 commits August 5, 2024 22:27

integration test

738285e

add backends for BSP to docs

31fdb33

cognifloyd approved these changes Aug 6, 2024

View reviewed changes

tdyas and others added 2 commits August 5, 2024 22:45

Update src/python/pants/goal/auxiliary_goal.py

ef3e98c

Co-authored-by: Jacob Floyd <cognifloyd@gmail.com>

fmt

6dc5d7d

tdyas merged commit 0baf008 into pantsbuild:main Aug 6, 2024
25 checks passed

tdyas deleted the builtin_goals_in_plugins branch August 6, 2024 04:37

tdyas mentioned this pull request Aug 15, 2024

move BSP QueryRule registration out of core rules #21311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow plugins to provide "auxiliary" goals (and refactor BSP into backends) #20913

allow plugins to provide "auxiliary" goals (and refactor BSP into backends) #20913

tdyas commented May 13, 2024 •

edited

Loading

sureshjoshi commented May 15, 2024

tdyas commented May 15, 2024

sureshjoshi commented May 15, 2024 •

edited

Loading

tdyas commented May 15, 2024

huonw commented Jun 5, 2024

sureshjoshi commented Jun 24, 2024

tdyas commented Jun 26, 2024

sureshjoshi commented Jul 2, 2024

tdyas commented Jul 24, 2024

tdyas commented Jul 24, 2024

tdyas commented Jul 25, 2024

cognifloyd left a comment •

edited

Loading

cognifloyd Jul 25, 2024

tdyas Jul 25, 2024 •

edited

Loading

tdyas commented Jul 25, 2024

tdyas commented Jul 25, 2024

tdyas commented Aug 6, 2024

allow plugins to provide "auxiliary" goals (and refactor BSP into backends) #20913

allow plugins to provide "auxiliary" goals (and refactor BSP into backends) #20913

Conversation

tdyas commented May 13, 2024 • edited Loading

sureshjoshi commented May 15, 2024

tdyas commented May 15, 2024

sureshjoshi commented May 15, 2024 • edited Loading

tdyas commented May 15, 2024

huonw commented Jun 5, 2024

sureshjoshi commented Jun 24, 2024

tdyas commented Jun 26, 2024

sureshjoshi commented Jul 2, 2024

tdyas commented Jul 24, 2024

tdyas commented Jul 24, 2024

tdyas commented Jul 25, 2024

cognifloyd left a comment • edited Loading

Choose a reason for hiding this comment

cognifloyd Jul 25, 2024

Choose a reason for hiding this comment

tdyas Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

tdyas commented Jul 25, 2024

tdyas commented Jul 25, 2024

tdyas commented Aug 6, 2024

tdyas commented May 13, 2024 •

edited

Loading

sureshjoshi commented May 15, 2024 •

edited

Loading

cognifloyd left a comment •

edited

Loading

tdyas Jul 25, 2024 •

edited

Loading