https://pantsbuild.org/ logo
Powered by
a

average-vr-56795

09/26/2019, 4:05 PM
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

aloof-angle-91616

09/26/2019, 4:06 PM
--v2 --no-v1
should do it?
a

average-vr-56795

09/26/2019, 4:06 PM
It doesn’t
a

aloof-angle-91616

09/26/2019, 4:06 PM
ah!
is there a branch i can check out?
i have some time right now to debug
w

witty-crayon-22786

09/26/2019, 4:06 PM
Is there a fmt Goal installed?
a

aloof-angle-91616

09/26/2019, 4:06 PM
i don't know off the top of my head what would cause it
^
a

average-vr-56795

09/26/2019, 4:07 PM
There is a 
fmt
v1 
Goal
installed
But the same is true for the 
test
v1 `Goal`…
w

witty-crayon-22786

09/26/2019, 4:07 PM
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

average-vr-56795

09/26/2019, 4:08 PM
Oh. We renamed the 
test
goal to 
legacy
.
And stubbed it
Ok
w

witty-crayon-22786

09/26/2019, 4:08 PM
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

average-vr-56795

09/26/2019, 4:08 PM
We can do that!
a

aloof-angle-91616

09/26/2019, 4:08 PM
(registering all the ceremony for `@console_rule`s isn't super clear yet, thank you for forging through it!)
a

average-vr-56795

09/26/2019, 4:08 PM
w

witty-crayon-22786

09/26/2019, 4:08 PM
the v1 Goal doesn't actually need to exist (iirc?): the text will come from the v2 goal
👍 1
☝️ 1
a

average-vr-56795

09/26/2019, 4:09 PM
Cool, we will just unregister the 
test
v1 
Goal
🙂
w

witty-crayon-22786

09/26/2019, 4:09 PM
hm
a

average-vr-56795

09/26/2019, 4:09 PM
Oh, except the fmt 
Goal
uses a custom options registrar class…
w

witty-crayon-22786

09/26/2019, 4:09 PM
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

average-vr-56795

09/26/2019, 4:12 PM
Any idea how we can fix that? It appears to be the only thing blocking us at the moment 🙂
w

witty-crayon-22786

09/26/2019, 4:12 PM
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

fancy-motherboard-24956

09/26/2019, 4:21 PM
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

average-vr-56795

09/26/2019, 4:24 PM
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

witty-crayon-22786

09/26/2019, 4:24 PM
That's mostly nailed down: the Goal has options
a

average-vr-56795

09/26/2019, 4:24 PM
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

witty-crayon-22786

09/26/2019, 4:24 PM
And everything else is a Subsystem
a

average-vr-56795

09/26/2019, 4:25 PM
So we’d have a subsystem per language per console_rule, emulating tasks?
w

witty-crayon-22786

09/26/2019, 4:25 PM
Per... whatever. As many option scopes as you need
a

average-vr-56795

09/26/2019, 4:25 PM
How will they compose?
w

witty-crayon-22786

09/26/2019, 4:25 PM
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

average-vr-56795

09/26/2019, 4:31 PM
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

witty-crayon-22786

09/26/2019, 4:31 PM
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

average-vr-56795

09/26/2019, 4:35 PM
It can’t be on the 
console_rule
, right? Because if it is, the 
console_rule
needs to know about custom plugins’ Subsystems
w

witty-crayon-22786

09/26/2019, 4:36 PM
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

average-vr-56795

09/26/2019, 4:38 PM
So 
--transitive
we can fix by just deprecating the per-task option and making it a goal option
w

witty-crayon-22786

09/26/2019, 4:39 PM
Yea
a

average-vr-56795

09/26/2019, 4:39 PM
What would the scope name of the subsystem containing skip be?
w

witty-crayon-22786

09/26/2019, 4:39 PM
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

average-vr-56795

09/26/2019, 4:42 PM
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

witty-crayon-22786

09/26/2019, 4:43 PM
From that perspective, Subsystem == Task
They're both Optionable
So, rather, we removed one Optionable subclass (Task -> ), and replaced another (Goal -> Goal)
a

average-vr-56795

09/26/2019, 4:47 PM
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

witty-crayon-22786

09/26/2019, 4:48 PM
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

average-vr-56795

09/26/2019, 4:53 PM
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

witty-crayon-22786

09/26/2019, 4:54 PM
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

average-vr-56795

09/26/2019, 4:56 PM
That still requires opting in… Which I guess is a breaking change we can just make?
w

witty-crayon-22786

09/26/2019, 4:59 PM
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

average-vr-56795

09/26/2019, 5:05 PM
Hurrah! I will file some issues 🙂
Thanks for your thoughts!
👍 1
Powered by