New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uniform handling of subsystem discovery #5575

Merged
merged 12 commits into from Mar 10, 2018

Conversation

Projects
None yet
3 participants
@benjyw
Copy link
Contributor

benjyw commented Mar 7, 2018

Previously we had two related/overlapping ways to discover all subsystems that require initialization. We had Subsystem.closure() for discovering the unscoped subsystem types, and then known_scope_infos() for discovering the possible scopes they can be found in.

Options initialization required a somewhat delicate dance involving separate treatment for different kind of Optionables (tasks, subsystems, global options). And yet, due to the naive implementation of known_scope_infos(), the init sequence still didn't support cases like "a subsystem scoped to a subsystem scoped to a task".

This change re-implements known_scope_infos() using a similar algorithm to the one used by Subsystem.closure(), and removes the latter (since ScopeInfo includes the type of the Optionable at each scope anyway). This new implementation is no longer naive, and can handle elaborate cases. Handling the different kinds of Optionable in the initialization sequence is now uniform - since they all define known_scope_infos().

benjyw added some commits Mar 5, 2018

WIP
WIP
WIP
WIP
WIP
WIP
WIP

@benjyw benjyw changed the title Simplify optionables [WIP] Uniform handling of subsystem discovery Mar 8, 2018

@benjyw benjyw requested review from jsirois , cosmicexplorer and stuhood Mar 8, 2018

@cosmicexplorer
Copy link
Contributor

cosmicexplorer left a comment

Pretty great, just unclear about the need for SubsystemClientMixin's existence as mentioned.

known_scope_infos = set()
optionables_path = OrderedSet() # To check for cycles at the Optionable level, ignoring scope.

def collect_scope_infos(optionable_cls, scoped_to):

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Mar 9, 2018

Contributor

With the call collect_scope_infos(cls, GLOBAL_SCOPE) at the bottom, it looks like we are assuming the cls argument is necessarily an Optionable as well, which it doesn't seem is guaranteed. From a quick grep, it seems that every consumer of SubsystemClientMixin is also an Optionable, so it won't cause an error, but at that point is there a reason the contents of this mixin can't just be merged into Optionable? That's the approach I took in a very old pr that had similar motivations and I can't immediately see why it wouldn't apply here.

This comment has been minimized.

@benjyw

benjyw Mar 9, 2018

Contributor

There's a comment on SubsystemClientMixin: Must be mixed in to an Optionable.

The mixin isn't merged into Optionable because not every Optionable is a SubsystemClientMixin, although I suppose they could be. More to the point, the subsystem package depends on the option package, so that move would require merging the packages themselves, which may not be what we want. Currently Optionable is blissfully unaware of the existence of subsystems. Although granted, the subsystem package is tiny, and may not deserve independent existence.

Still, my point is, that's for a future change, this one is already substantial enough.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Mar 9, 2018

Contributor

Ooh, I definitely should have seen that comment -- I just asked because I wasn't sure if it had occurred to you. I definitely didn't realize the package dependency situation either, although I suspected something was fishy. Thanks for the thorough explanation!

@@ -76,36 +66,6 @@ def get_scope_info(cls, subscope=None):
else:
return ScopeInfo(cls.subscope(subscope), ScopeInfo.SUBSYSTEM, cls)

@classmethod
def closure(cls, subsystem_types):

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Mar 9, 2018

Contributor

+1 to killing this, I tried to in my first pants PR but didn't go far enough.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Mar 10, 2018

ping? This will conflict with @stuhood's BaseTest change, FYI.

@stuhood
Copy link
Member

stuhood left a comment

Thanks Benjy!

"""Subsystems used outside of any task."""
"""Subsystems used outside of any task.
TODO: Should these simply be dependencies of the GlobalOptions optionable?

This comment has been minimized.

@stuhood

stuhood Mar 10, 2018

Member

Placing them somewhere more specific rather than less would be my preference... so here is better for now.

This comment has been minimized.

@benjyw

benjyw Mar 10, 2018

Contributor

Fair, I guess we don't want GlobalOptions to have all those dependencies. Will remove the TODO.

@benjyw benjyw merged commit 327a8d9 into pantsbuild:master Mar 10, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@benjyw benjyw deleted the benjyw:simplify_optionables branch Mar 10, 2018

@benjyw benjyw referenced this pull request Mar 10, 2018

Merged

Port BaseTest to v2 engine #4867

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment