https://pantsbuild.org/ logo
Join Slack
Powered by
We’re running into an issue where our `console_rul...
# development
a
We’re running into an issue where our 
console_rule
named 
fmt
conflicts with the scope of the v1 goal named 
fmt
- we can’t work out how we fixed this for 
test
, which doesn’t appear to do any magic to avoid this… Anyone know/remember?
a
--v2 --no-v1
should do it?
a
It doesn’t
a
ah!
is there a branch i can check out?
i have some time right now to debug
w
Is there a fmt Goal installed?
a
i don't know off the top of my head what would cause it
^
a
There is a 
fmt
v1 
Goal
installed
But the same is true for the 
test
v1 `Goal`…
w
no
Copy code
$ rg 'Goal.*fmt'
src/python/pants/core_tasks/register.py
46:  Goal.register('fmt', 'Autoformat source code.', FmtTaskMixin.goal_options_registrar_cls)
a
Oh. We renamed the 
test
goal to 
legacy
.
And stubbed it
Ok
w
Copy code
$ rg 'Goal.*test'
tests/python/pants_test/console_rule_test_base.py
26:    """Subclasses must return the Goal type to test.

tests/python/pants_test/task/echo_plugin/register.py
31:  Goal.register('echo', 'test tasks that echo their target set',
a
We can do that!
a
(registering all the ceremony for `@console_rule`s isn't super clear yet, thank you for forging through it!)
a
w
the v1 Goal doesn't actually need to exist (iirc?): the text will come from the v2 goal
👍 1
☝️ 1
a
Cool, we will just unregister the 
test
v1 
Goal
🙂
w
hm
a
Oh, except the fmt 
Goal
uses a custom options registrar class…
w
see the first line: that's what registers a goal
mm. yes. that will be... hairy
i think that that is the only instance of that API
(...that and 
lint
)
a
Any idea how we can fix that? It appears to be the only thing blocking us at the moment 🙂
w
in the medium term, i think we need to nuke that facility in place of a scoped Subsystem
in the short term... might need to use another goal name =/
unclear though? there might be another short term solution. but the goal registrar stuff is confusing
Oh. Hm
...mm. no.
Some sort of hack to allow the v1 Tasks to get the recursive option definitions from the v2 Goal?
(iirc, it's things like 
--transitive
, etc)
👍 1
f
Will raise PR with fmt_v2 as the name for now to try and get somewhere during hackweek 🙂
And we can figure out the minutia in a follow-up PR
a
Sooooo… There’s an interesting long-term question here which is “How do we want to model per-task options in a v2 world?”
w
That's mostly nailed down: the Goal has options
a
Right now we’ve done away with tasks, and we’re doing the per-task equivalent implementations using 
@union
, but that omits the “there’s some per-union-type options you may want to propagate”
w
And everything else is a Subsystem
a
So we’d have a subsystem per language per console_rule, emulating tasks?
w
Per... whatever. As many option scopes as you need
a
How will they compose?
w
If your formatter needs to compile something first, then you've got compiler options
Etc
@average-vr-56795: what do you mean?
Subsystems are already pretty widely used. For the case above with "recursive options" you can use a scoped instance of a subsystem to get 
--jvm-options=..
vs 
--jvm-mytool-options=
applied recursively
(so Goal registrars should become Subsystems probably)
a
If I want to be able to say: 
--fmt-python-skip --no-fmt-java-skip
will they both be injected into the console_rule (so the console_rule needs to be aware of it), or will they be injected into the language-specific rules (so we need every rule to decide whether it should noop, and to return some “I noop’d” value up to the console_rule)
w
A thing that you lose between v1 and v2 is an "obvious name" for each rule that might run as part of 
fmt
, but they can choose their own scopes for Subsystems
For something like 
--skip
it could go either way: either implemented once, or implemented by a @rule that consumes a scoped subsystem to compute the targets to execute on: 
yield Get(TargetsToFormat, Scope('black'))
a
It can’t be on the 
console_rule
, right? Because if it is, the 
console_rule
needs to know about custom plugins’ Subsystems
w
You're mixing them I think
The option would be in one place or the other, rather than both
For something like 
--transitive
, it'd probably make sense on the console_rule
To emulate the current behaviour of 
--skip
, would need to be a Subsystem though I think
a
So 
--transitive
we can fix by just deprecating the per-task option and making it a goal option
w
Yea
a
What would the scope name of the subsystem containing skip be?
w
Good question. That's the primary difference between "Goal registrars" and Subsystems: the former end up inlined
And the latter need names.
I don't have a good answer... naming is hard. But would want to look at what other behaviour we might want there
I think it also relates to other kinds of filtering... tags and such.
I pushed @wide-energy-11069 a bit more to make that API subsystem friendly
a
So right now we’ve removed a concept (“Task”) which does a number of things (“Allow decomposition” and “Add option scopes” among them), and introduced a partial replacement (“UnionRule”) which covers the “Allow decomposition” piece, but skips the “Add option scopes” piece…
w
From that perspective, Subsystem == Task
They're both Optionable
So, rather, we removed one Optionable subclass (Task -> ), and replaced another (Goal -> Goal)
a
And the API between Goal and Task is that Task can do whatever it wants and no-oping is fine, whereas the API in 
console_rule
kind of requires that information is passed in a structured way from a task-equivalent to a Goal
(Which I think is probably only really relevant for 
--skip
, at this point…)
w
Yea. Which is where the "--skip" question comes in.
...that looks like a good place for "skip", maybe.
TargetFilter.scoped_instance(self)
so... that one actually ended up ready for v2.
(it's named "target-filter" currently, but ... naming)
a
So we could just deprecate the per-task 
--skip
too, and recommend 
target-filter
(with a new “language” or something option) as the replacement, which works really nicely for v2, but would probably be hard to shoehorn into v1 because it would need to implicitly add a subsystem dep to all the tasks in the goal?
w
it's already implicitly a dep if you opt in
Alex followed up on my feedback from the first one: https://github.com/pantsbuild/pants/pull/7283
so yea... i think my current take is: fmt/lint "Goal registrar" stuff should move over there, and it should get a better name
a
That still requires opting in… Which I guess is a breaking change we can just make?
w
not sure it needs to be breaking. but could be if folks agreed it needed to happen sooner
... buuut. i think that "scoped Subsystem" options are not currently quite working? they should via something like 
optionable_rule(Subsystem.scoped('blah'))
... but i remember thinking that that might not just work
can't remember why.
not a near term issue, but likely an issue when we're swapping wholesale from python v1 test to v2 test by default, for example
alright, back to my non-determinism. have a good night UK folks!
a
Hurrah! I will file some issues 🙂
Thanks for your thoughts!
👍 1
2 Views
Open in Slack
PreviousNext
Powered by