@happy-kitchen-89482: hey: so re: Subsystems consuming https://github.com/pantsbuild/pants/pull/6872

i'd love to be able to use the Subsystem interface verbatim, but there are two issues that i can see: 1) the

cls._options

field, and 2) the static/cls requests for Subsystem instances aren't concurrency safe

so i expect that we'll have to move to injecting everything at construction time

i could imagine that afterward, it would look something like

...oof. not quite ready to imagine yet.

well, yes i am. without any additional magic it might look like:

class ListOptions(Subsystem):
  """Lists all targets matching the target specs.

  If no targets are specified, lists all targets in the workspace.
  """

  options_scope = 'list'

  @classmethod
  def register_options(cls, register):
+---  9 lines: super(ListTargets, cls).register_options(register)-----------

  def __init__(self, options):
    super(ListTargets, self).__init__(*args, **kwargs)
    self._provides = options.provides
    self._provides_columns = options.provides_columns
    self._documented = options.documented


@rule(ListOptions, [])
def list_options():
  scoped_options = yield Get(ScopedOptions, Scope(str(ListOptions.options_scope)))
  return ListOptions(scoped_options)

in that case, the rule that constructs the subsystem looks like boilerplate. but if the subsystem had subsystem deps, you'd want to inject those into the constructor as well

so i think that what the Subsystem interface provides for here is 1) a way to register the

scope

as a known scope, 2) the register function, 3) the object to hold the results

1 and 2 are necessary... but i think it actually does a poor job of 3, because the subsystem ends up holding an OptionsValueContainer containing all of the options, rather than just the ones it might have wanted

which will make having useful equality for Subsystems (which is important for caching) challenging

given the above, i'm thinking of maybe adding a new abstract subclass of

Optionable

for this usecase

we could potentially still call it Subsystem, although that might be confusing.

ok, i think i see how to preserve Subsystem then... can give

global_instance

and

scoped_instance

an

options

parameter that bypasses subsystem initialization and singletonism. cc @happy-kitchen-89482

@witty-crayon-22786 just seeing this now, will read properly and get back to you

thanks. In the last two days of hackweek, I've gone from well understood ground into actual hacks =)

latest on this is that i think that we can leave Subsystem approximately as is, and then construct them directly in the engine context

it's also possible that during a migration period we could move to constructing all Subsystem instances via requests to the engine

and deprecate the

global_instance

and

scoped_instance

methods

although that's further down the line i expect.

yea, this is coming together

i think an abstract class for subsystems with the new interface is a fantastic idea

i would just make all my new subsystems that way

OK, finally read and understood https://github.com/pantsbuild/pants/pull/6872 (awesome work btw), so now I can address your discussion above…

I like the idea of constructing all Subsystem instances via the engine, and deprecating the global_instance and scoped_instance methods.

@happy-kitchen-89482: ok, this is going to be slightly broken, because the

--no-v1 --v2

split isn't quite working, but it's hackweek. this is a complete replacement for the ListTargets goal: https://github.com/pantsbuild/pants/pull/6880

@console_rule('list', [Select(Console), Select(ListOptions), Select(Specs)])

this is **********ing absurd

pretty dag happy with how that turned out =D

it's easy like oh what do i want to do? a new console rule named

list

? what do i need to do that? it's all right there

can totally remove the

Select

wrapper at this point too

yeah after the coroutines brainblast