Convert BundleAdaptor to BundleProps during JvmApp target creation

Review Request #4057 — Created July 9, 2016 and submitted

yujiec
pants
3641
pants-reviews
ity, kwlzn, stuhood

In v2 engine, TargetAdaptor serves as an intermediate state of a target and is eventually converted to its real target type, with one exception: BundleAdaptor.
BundleAdaptor never gets converted to its "traditional" counterpart (which is BundleProps).
BundleAdaptor structure and old BundleProps structure are not compatible, the hashing logic is based on BundleProps, thus exception will be thrown when applying the same logic onto BundleAdaptor

This review converts BundleAdaptor to BundleProps during the creation of JvmApp target.

ci green:
https://travis-ci.org/pantsbuild/pants/builds/144788292

  1. 
      
  2. src/python/pants/backend/jvm/targets/jvm_app.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     

    So far pants.engine.legacy.* is not imported anywhere except by the engine integration code... I think we're waiting until we begin the push to enable the v2 engine by default before we begin referring to it from Target definitions.

    So rather than doing this here, it would be preferable to change the caller in pants.engine.legacy to execute the necessary conversion.

    1. More concretely, I think we want to wait until https://github.com/pantsbuild/pants/issues/3560 is finished before we start changing Target implementations... right now there isn't really a strategy, and so we're trying to isolate changes inside the pants.engine.legacy package.

    2. ok. so we want to do the opposite, which is import backend stuff to engine.legacy, because it is isolated and will be thrown away eventually?

    3. Right, to the extent possible.

  3. 
      
  1. this lgtm w/ one thought (below) and one request: I'd also like to see an integration test that exercises the failure/fix so we don't regress this again in the future.

    see https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/engine/legacy/test_list_integration.py for some examples.

  2. src/python/pants/engine/legacy/graph.py (Diff revision 3)
     
     
     
     

    consider a list comprehension here - it's more concise/readable and potentially faster than appending to a list in a for loop:

    kwargs['bundles'] = [
      BundleProps.create_bundle_props(bundle.kwargs()['fileset'])
      for bundle in kwargs['bundles']
    ]
    
  3. 
      
  1. lgtm!

  2. src/python/pants/engine/legacy/graph.py (Diff revision 4)
     
     
     
     
     

    style nit: the closing bracket here should generally align with the column that the opening line starts on

    e.g.

    var = [
      ...
    ]
    

    (pants.ini is the only place where the "indented closing bracket style" should be needed, because it's .ini format vs python)

  3. 
      
  1. Thanks!

  2. src/python/pants/engine/legacy/graph.py (Diff revision 4)
     
     
     
     
     
     
     

    Might be better broken out as an _instantiate_jvm_app type method.

  3. Since the target has the same name as the directory (which is recommended), you don't need to repeat it. It's a "default" target.

    So just ./pants -q bundle testprojects/src/java/org/pantsbuild/testproject/bundle is sufficient.

  4. 
      
  1. 
      
  2. src/python/pants/engine/legacy/graph.py (Diff revisions 5 - 6)
     
     
     
     
     
     
     
     
     
     
     

    you shouldn't need the ** part of **kwargs in the call to or the signature of this new method - just passing the kwargs dictionary should be totally sufficient.

    1. ah, right. done. Thanks!

  3. 
      
  1. thanks Yujie! this is in master @ https://github.com/pantsbuild/pants/commit/bea6349be3be65dfc850ef03ef1accda87d5768b

    please mark this RB as submitted when you get a chance.

  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks Kris and Stu!

Loading...