Convert two existing concepts (RunTracker and the jar tool) to be subsystems.

Review Request #2081 — Created April 15, 2015 and submitted

benjyw
pants
90da36f...
pants-reviews
patricklaw, stuhood, zundel

- The RunTracker change is straightforward.

- There is now a JarTool subsystem. But implementing this neatly
required some changes to the JVM tool machinery:

+ The old JvmToolTaskMixin is now JvmToolMixin. I.e., you don't have
to be a task to use JVM tools. Indeed, subsystems can mix this in
in order to register and bootstrap JVM tools.

+ JvmToolTaskMixin is now a thin subclass of JvmToolMixin, adapting
it for ease of use by tasks. In the future we might make all
JVM tools (compilers etc.) into subsystems and then not need
JvmToolTaskMixin at all.

+ The new JarTool subsystem does two things:
1) Bootstraps the jar tool and handles its options.
2) Actually runs the jar tool with given args in a given runner.
This interface is slightly awkward perhaps, but very reusable.
And we already pass runners around as arguments elsewhere anyway.

- Removed some clunky code in JvmToolTaskTestBase that registers options
for BootstrapJvmTools, which is not the task under test but is needed
to bootstrap tools for the task under test. Instead overrides context() to
add BootstrapJvmTools to the list of tasks the base class already clunkily
registers options for (in addition to the task under test)...

- TODO: JarTool is now tested indirectly in test_jar_task.py,
but we might want to also test the now-standalone subsystem directly.

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

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
ZU
  1. 
      
  2. I didn't know you could implement @abstractproperty by just creating a class variable? Seems like it would be safer to declare an @property to make it read-only.

    1. Yeah, it's all dynamic dispatch, so you just need self.scope_qualifier to resolve to something at runtime.

      TBH I don't actually like that it's an @abstractproperty because it's really a property of the class, not the instance. So now that I revisit, it makes more sense to make this a @classmethod that must be overridden. Does this address your concern?

      In theory a subclass could create an instance method that would override the classmethod, and mess with it that way, but that's python for you...

    2. Making it @classproperty sounds good to me if you think that better models it. I know this abstract class/method/property idea is not first class python feature so I understand it isn't perfect.

    3. I made it a @classmethod. There is no such thing as @classproperty, alas.

  3. change from 'jar-tool' literal to self.scope_qualifier?

    1. Technically these are different. One is a scope qualifier, the other is a logical tool name. They just happen to use the same string in this particular file. For example, you could imagine registering multiple tools here.

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

    again, maybe use @property instead

  5. 
      
ST
  1. 
      
  2. src/python/pants/goal/run_tracker.py (Diff revision 1)
     
     

    This directory will be renamed from runs to run-tracker... intentional?

    1. Yes. 'runs' was just an arbitrary string, and could in theory collide with the options scope of some other task (tasks use pants_workdir/goal_scope/task_scope as their workdir). Always using scope names as directory names under pants_workdir is safe, because we already verify that those don't collide.

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

Status: Closed (submitted)

Change Summary:

Submitted as 8a656213dceaceb0588fc61c8f9d5ffd633990bf.

BE
  1. Submitted as 8a656213dceaceb0588fc61c8f9d5ffd633990bf. Thanks for the reviews!

  2. 
      
Loading...