Split jvm_binary.py into jvm_binary.py and jvm_app.py.

Review Request #2006 — Created March 30, 2015 and submitted

zundel
pants
zundel/split-jvm-binary-jvm-app
1344
bda252f...
pants-reviews
jsirois, patricklaw
  • Moved test_jvm_app.py into the tests/python/pants_test/backend/jvm/targets directory
  • Added some explicit tests for the JvmBinary target.
  • Minor fix to jvm_binary.py to raise an error about the 'source' attribute before
    it gets passed to the super() constructor.

CI at https://travis-ci.org/pantsbuild/pants/builds/56504009

ZU
  1. I can see why jvm_binary and jvm_app were put in the same source file because jvm_app must live in conjunction with jvm_binary. But there is no shared code between the two targets and I want to add more code and tests to jvm_binary. It seems ripe for each living in their own source files.

    This change just moves code around for the most part:

    jvm_binary.py is split into jvm_app.py and jvm_binary.py
    test_jvm_app.py is moved to a new directory
    test_jvm_binary.py was added.

  2. 
      
PA
  1. Ship It!
  2. 
      
JS
  1. 
      
  2. The super call move you did makes sense from a fail-fast perspective, but TargetDefinitionException is (still) a tricky beast.  It will report 'address not yet set' in the error message since the super constructor has not been called yet.
    1. Here is the problem I was trying to fix. Below is message before this change. The line above never gets reached because it fails the sources field check (unfortunately, that exception message doesn't have the field name in it, but it does show the value which is helpful.)

      Exception message: Invalid target JvmBinary(address not yet set): Expected a list containing values of type (<type 'basestring'>,), instead got a value ['HelloMain.java'] of <type 'list'>
      

      Here is the method with the first diff:

      Invalid target JvmBinary(address not yet set): source must be a single relative file path
      

      Both of them have the problem you mention. Here it is after the second diff to force the address setting in early:

      Exception message: Invalid target JvmBinary(BuildFileAddress(/Users/zundel/Src/pants/examples/src/java/com/pants/examples/hello/main/BUILD, main-bin)): source must be a single relative file path
      

      I went ahead and patched up the problem in JvmTarget and Python Target as well.

      # remove square brackets from sources in 'address' target:
      $ PANTS_DEV=1 ./pants compile ./src/python/pants/base:address
      Exception message: Invalid target PythonLibrary(BuildFileAddress(/Users/zundel/Src/pants/src/python/pants/base/BUILD, address)): Expected an object of acceptable type (<type 'list'>, <class 'twitter.common.dirutil.fileset.Fileset'>, <class 'twitter.common.collections.orderedset.OrderedSet'>, <type 'set'>, <type 'tuple'>), received address.py instead
      
  3. 
      
ZU
JS
  1. 
      
  2. This is forward progress, but it seems even better to just force good behavior having TargetDefinitionException take an address instead of a Target.  I can follow up with that sort of global change if you agree,
    1. OK. That's not exactly a mechanical change but looks like there are only a couple dozen references to fix up.

  3. 
      
ZU
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks for the reviews Patrick and John. Commit de08424

Loading...