Fixup ScalaLibrary dep injection - do this at BuildGraph population time.

Review Request #581 — Created June 20, 2014 and submitted

jsirois
pants
jsirois/inject_scala_library
208, 256
pants-reviews
benjyw, ity, stuhood
commit e2eecade12a4a2ef503ed30d08998ca597794d1e
Author: John Sirois <jsirois@twitter.com>
Date:   Fri Jun 20 04:27:24 2014 -0600

    Fixup ScalaLibrary dep injection - do this at BuildGraph population time.
    
    This is effectively a restoration of 24681429b by Ryan Williams.
    
    Previously ScalaLibrary runtime deps were injected in the classpath by
    ScalaCompile at execute time.  This let downstream goals like test and
    run that just use an aggregate classpath work, but it failed for any goal
    needing correct deps at a per-target level like binary, bundle and publish.
    
    This change introduces a scala TargetPlatform class as a very rough start
    to centralizing the scala version to target and uses this in ScalaLibrary and
    ZincUtils.

 src/python/pants/backend/jvm/scala/BUILD                           | 10 ++++++++++
 src/python/pants/backend/jvm/scala/__init__.py                     |  0
 src/python/pants/backend/jvm/scala/target_platform.py              | 36 ++++++++++++++++++++++++++++++++++++
 src/python/pants/backend/jvm/targets/BUILD                         |  3 +++
 src/python/pants/backend/jvm/targets/jvm_target.py                 | 18 ++++++++++++++----
 src/python/pants/backend/jvm/targets/scala_library.py              | 18 ++++++++++++++++--
 src/python/pants/backend/jvm/tasks/jvm_compile/BUILD               |  1 +
 src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py      | 12 ------------
 src/python/pants/backend/jvm/tasks/jvm_compile/scala/zinc_utils.py |  4 ++--
 9 files changed, 82 insertions(+), 20 deletions(-)
Some of the python tests have issues due to TargetPlatform being singleton and getting a production Config loaded instead of a test build root pants.ini (blank) config.
Getting this out there to refresh Benjy's memory on a recent IM conversation about this problem and get a gut/double-check on this approach.
I'll circle back with a 2nd diff for this RB that fixes up the TargetPlatform Singleton test interaction issue.

Before:
=======
$ PANTS_DEV=1 ./pants goal bundle src/scala/com/pants/example/hello/exe
$ java -jar dist/exe-bundle/exe.jar 
Exception in thread "main" java.lang.NoClassDefFoundError: scala/ScalaObject
...
$ tree dist/exe-bundle/
dist/exe-bundle/
├── exe.jar
└── libs

1 directory, 1 file

After:
======
$ PANTS_DEV=1 ./pants goal bundle src/scala/com/pants/example/hello/exe
$ java -jar dist/exe-bundle/exe.jar
$ java -jar dist/exe-bundle/exe.jar
Num args passed: 0. Stand by for welcome...
Hello, Resource World!
$ tree dist/exe-bundle/
dist/exe-bundle/
├── exe.jar
└── libs
    └── org.scala-lang-scala-library-2.9.3.jar -> /home/jsirois/dev/3rdparty/jsirois-pantsbuild-pants/.pants.d/ivy/mapped-jars/src.scala.com.pants.example.hello.exe.exe/org.scala-lang/scala-library/default/org.scala-lang-scala-library-2.9.3.jar

1 directory, 2 files
ST
  1. Change looks like an improvement, but:
  2. src/python/pants/backend/jvm/scala/target_platform.py (Diff revision 1)
     
     
     
     
     
     
    One thing we discussed was having a platform act as a required (by scala_library in this case), "exclusive" dependency, which would handle enforcement of uniqueness.
    
    I think what that would look like in terms of implementation would be a first class "Platform" target type with a list of dependencies, which the scala_library target would look for... by name?
    
    Would that be more difficult than this singleton approach?
    1. Not really, but its also additive.  This change just gets a default platform plumbed to get things working like they used for folks (like Twitter), that don't specify a scala_library dep explicity in their dependencies.
      
      So I agree strongly that should be the next step, add an attribute to scala target types and java target types that allows picking the target platform.
      
      In either case the exclusivity bit seems best acheived with the exclusives mechanism that already exists.  We'd just need to mark different scala-library targets as mutual exclusives... I think.
  3. 
      
BE
  1. I think this is the right approach, although it might be worth double-checking with Patrick that traversable_dependency_specs() is how he envisions this sort of thing, and that it doesn't look like abuse to him. 
    
    Note that support for targets specifying their own scala versions falls out of this: If you explicitly depend on a scala version greater than the default one, ivy will do its thing and you should end up with the later version. 
    
    1. You probably don't want to have ivy decide what your scala version should be, because the failure case is that someone accidentally mixing a 2.11 dep into a 2.9 project would cause wacky compile errors rather than a failure due to exclusives. Exclusives being more explicit than ivy is one of their benefits, imo... even using ivy's `force` would do the wrong thing there by forcing 2.9 and causing a runtime error in the dep, rather than failing early.
    2. exclusives - which Twitter does not use yet - address this - I think.
    3. To be clear - they'd allow `compile target1 target2` where target1 needed 2.9 and target2 2.10 as long as the graphs did not intersect and it would blow up with a clear exclusives error if the did intersect.
  2. 
      
PA
  1. Ship It!
  2. 
      
JS
IT
  1. Ship It!
  2. 
      
JS
JS
JS
  1. Tests all pass locally now via tests/python:: - I'll wait for this guy to go green and submit: https://travis-ci.org/pantsbuild/pants/builds/28597217
    1. That went green.
      
      Thanks folks - submitted @ https://github.com/pantsbuild/pants/commit/87bef813b3e92946d127f6fc58e211e69cb8e8bc
  2. 
      
JS
Review request changed

Status: Closed (submitted)

JS
  1. Full disclosure - I pushed an extra commit un-reviewed after this hit master and broke ci distribution tests. https://github.com/pantsbuild/pants/commit/57810ec5d20dd0b8c90da6a480830db2d109c74f
    It was the missing empty __init__.py problem Mateo ran down with the ReviewBoard folks.
  2. 
      
Loading...