Simplify subsystem option scoping.

Review Request #2380 — Created June 16, 2015 and submitted

benjyw
pants
ef4dd70...
pants-reviews
nhoward_tw, patricklaw, zundel

This gets rid of the concept of "scope qualifiers", which was mostly just confusing.
Now subsystems just have scopes ('ivy', 'scala-platform' etc.) and task-specific
instances have options in the appropriate subscope (e.g., `cache.compile.java` instead of
`compile.java.cache` as it was previously). This has many advantages:
- Option value inheritance "just works": `cache.compile.java` will inherit values from
`cache`, in the usual way.
- Overriding values in the subscope will also work as it does today - by registering
the option as recursive=True.
- Code is simpler, with a net reduction in LoC: "82 insertions(+), 114 deletions(-)".
- Subsystems and tasks are now more uniform. For example, both just have an options_scope
property, instead of one having a scope_qualifier() method.

This change doesn't affect any existing pants.ini or flags because we don't have any
task-specific subsystem instances yet.

It does mean that the section name for a task-specific config would be
[cache.compile.java], not [compile.java.cache], but that doesn't seem like a big deal.

All unit tests pass locally.

CI passes here: https://travis-ci.org/pantsbuild/pants/builds/67110458.

BE
  1. BTW I'm looking into some metaclass magic for detecting when a class hasn't declared an options_scope, and throwing a sensible error, instead of letting None propagate. But that will be in a followup change. I don't want to turn options_scope into a method because we have dozens of tasks that declare it as a class variable, and I don't want to have to change that (not even for Subsystems, because I want those to be uniform with tasks).

  2. 
      
BE
ST
  1. Looks good! Will need to update the config/options migrator I suppose?

    1. Fortunately no, because we have no task-specific subsystems yet.

  2. src/python/pants/bin/goal_runner.py (Diff revision 1)
     
     

    "until we kill this subsystem outright" is there are ticket/issue/plan for that somewhere?

    1. No idea, I don't actually remember what this is about.

  3. src/python/pants/goal/goal.py (Diff revision 1)
     
     

    can drop the . after "own"

  4. src/python/pants/subsystem/subsystem.py (Diff revision 1)
     
     

    Can we imagine cases in which the global subsystem would need options that we wouldn't want inherited by the tasks?

    1. I don't think so? We haven't had any with tasks, and I don't see a difference here.

      There are definitely cases where we don't want the inner scope to be able to override the value from the outer scope. In fact these are the majority of cases - you must register the option with recursive=True to be able to override it.

      But you can always read the value set on the outer scope from your inner scope. This is what allows tasks and subsystems to do things like get_options().level, instead of "verbose code to get the global scope options".level, to get the log level.

  5. src/python/pants/subsystem/subsystem.py (Diff revision 1)
     
     
     

    s/setup/options/

    setup implies that initializing a subsystem has side-effects, which I don't think we have ambitions for?

  6. src/python/pants/subsystem/subsystem.py (Diff revision 1)
     
     

    What the "main" scope is isn't defined anywhere, so using that words might be filler here.

  7. 
      
ZU
  1. Can you create a test where it demonstrates the new behavior for setting an option, "task-specific
    instances have options in the appropriate subscope (e.g., cache.compile.java instead of
    compile.java.cache as it was previously)"

    1. Well, the previous behavior was never implemented (e.g., compile.java.cache never inherited from cache) - I had that complicated change to implement that but abandoned it to go the other way with this change.

      The new behavior is already tested in the options system's tests. There's nothing subsystem-specific about it. It falls out of the dotted naming.

  2. 
      
BE
ST
  1. Thanks Benjy!

  2. 
      
NH
  1. LGTM!

  2. 
      
ZU
  1. Ship It!
  2. 
      
BE
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as 3b38c063a3d7fe23bd0af9a4be9e1814eb294e77.

BE
  1. Submitted as 3b38c063a3d7fe23bd0af9a4be9e1814eb294e77. Thanks all!

  2. 
      
Loading...