Reimplement the jvm tool registration mechanism via the options system.

Review Request #1521 — Created Dec. 18, 2014 and submitted

benjyw
pants
932118d...
pants-reviews
ity, jsirois, patricklaw, zundel

Instead of reaching in to config directly, the tool registration simply
registers an appropriately-named option, and then registers the fact
that this option is in fact a jvm tool.

The option value is a list of specs. We set sensible defaults in the code,
which are specs in a root-level BUILD.tools file.

There's one slight quirk, which is that we must the register function into
our calls to register_jvm_tool(). This is because the JvmToolTaskMixin can't
have its own register_options() method (because it comes after TaskBase
in the mro, and chained super-calls to register_options() stop at TaskBase).

NOTE: This is a larger change that I would have liked, but it's pretty easy to review, as almost every file it touches is made simpler.

CI passed other than one unrelated unittest failure. Another one baking, just in case.
Doing some ongoing manual testing to see that options work.

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
ZU
  1. This makes the implementation much cleaner! I patched it in to take a look.

    pants.ini needs to be updated: it still references the old configuration names. As far as I am concerned, we can just remove the ones that are redundant with the defaults.

    If you make an intentional misconfiguration, pants fails with an unhelpful message.

    [compile.java]
    jmake: ["//:jmakeBOGUS"]
    

    Here's what you get

    Exception message: 'NoneType' object has no attribute '__getitem__'
    

    Before, the message looked something like this:

     Failed to resolve target for bootstrap tool: //:jmakeBOGUS. You probably need to add this dep to your tools BUILD file(s), usually located in the root of the build. 
                           See pants.ini section: [java-compile] key: jmake-bootstrap-tools
                           Error: jmake00 was not found in BUILD file /Users/zundel/Src/pants/BUILD. Perhaps you meant one of: 
    
    1. Ooops, that was some stale code. Fixed.

  2. 
      
PA
  1. 
      
    1. Looks like this was broken in the old code, and I just copied the bad string over...

  2. All of this looks great, except the global nature of JvmToolTaskMixin._tool_keys. This kind of global registration always makes me vaguely uncomfortable.

    1. Yep, I don't love it either. Happy to hear other ideas.

      One that occurred to me was to have the options system itself understand the notion of "jvm tools" (or maybe even "tools" in general). E.g., instead of the option type being "list of strings" it could be "list of tool specs", and then BootstrapJvmTools could act on all options with that type. So instead of the registry being global, it's part of the Options registry. This might also make it easier to default to actual targets (not specs of targets) so we can get rid of the need to have a BUILD.tools.

      However I'd prefer to make that change separately. This change paves the way for it.

    2. Now that I'm past the release woods I'll be diving into the engine design/experiment/code work and although not settled, that should make this all moot; ie:

      class TaskThatNeedsJvmTool(Task):
        @classmethod
        def register_options(cls, register):
          register('--bootstrap-tools', type=Options.list, default=['//:my-tool-cp'])
      
        def prepare(self, round_manager):
          if self.get_options().bootstrap_tools:
            bootstrap_tools = self.context.resolve(self.get_options().bootstrap_tools)
          else:
            # Defaults - no pants.ini _or_ BUILD.tools needed
            bootstrap_tools = self.context.add_new_target(JarLibrary, ...)
          # This form of require where a product type (RuntimeClasspath) _and_ a set of targets
          # (bootstrap_tools) is passed schedules a sub round - in that sub round plain old IvyResolve
          # will end up populating a purpose built RuntimeClasspath for this task.
          round_manager.require_data(RuntimeClasspath, bootstrap_tools)
        ...
      

      So I wouldn't worry too much about aesthetics atm.

  3. 
      
BE
JS
  1. 
      
  2. True for 3, not for 4 - we could do better for 4 and just use the runtime jar here. I have a shelved RB that TODOs this, Might be good for you to note here though since that RB is shelved.

    1. I don't get it. Is this broken for 4 then?

    2. Technically yes - but its too many deps that are added, the compiler jar and the runtime jar. In the antlr3 series both bits were in 1 jar. So this is just a lingering trap-style issue with overstated deps and potential for un-needed dep conflict.

  3. 
      
BE
BE
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as 1dda3a05a832569ea6a5ed9ff5056947ab181b99.
Loading...