Minimize the classpath pants passes to java for running tests

Review Request #205 — Created April 10, 2014 and submitted

ddavydov
pants
pants-reviews
jsirois, tejal
This change decreases the size of the classpath pants passes to java for running tests. It is necessary because this classpath can grow very large, and exceed the maximum argument length on some environments. 

Really the best place for this change was in executor.py:Executor:_create_command
But the proper solution would involve changing self._command to a function that returned a context manager (since a temporary jar would need to be created to be used for the new classpath) and this would require refactoring a considerable amount of code.
This fix is also a bandaid fix, as it does not ultimately solve the classpath length problem (it only removes jars from the classpath).
Because of the fact that this fix is hopefully temporary, and a refactor now would require another refactor when a better solution is implemented, I instead opted to make the classpath change in the client code rather than in the lower level java executor code.

I did not add tests/logging because I did not find any existing tests/logging in the source (it would also probably require heavy mocking). If you think the code needs some please let me know and I'll add it.
Manual Testing Done (in commons repo):
./build-support/bin/ci.sh
./pants src/python/twitter/pants && ./dist/pants.pex goal test tests/java/com/twitter/common:all -ldebug
./pants src/python/twitter/pants && ./dist/pants.pex goal test tests/java/com/twitter/common/objectsize -ldebug
Traced through code to make sure no jars on the classpath/only jars on the classpath would work
  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
TE
  1. 
      
  2. src/python/pants/tasks/junit_run.py (Diff revision 1)
     
     
    Please add tests.. 
    1. Originally running any set of unit tests did not run when I had an empty jar (complaining of a missing class), but when I filled the manifest the tests all passed. I can add some additional log statements provide you with the output if you wish but I already checked this myself when I ran the final set of tests.
    2. new classpath:['/tmp/tmpg3RJc0', u'/home/ddavydov/ws/my_pants/.pants.d/javac/classes', u'/home/ddavydov/ws/my_pants/.pants.d/javac/resources', u'/home/ddavydov/ws/my_pants/src/resources']
      
      old classpath:['/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/org.hamcrest/hamcrest-core/jars/hamcrest-core-1.2.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/junit/junit-dep/jars/junit-dep-4.10.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.twitter.common/junit-runner/jars/junit-runner-0.0.38.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.twitter.common/junit-runner-annotations/jars/junit-runner-annotations-0.0.5.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.twitter.common/junit-runner-withretry/jars/junit-runner-withretry-0.0.2.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/args4j/args4j/jars/args4j-2.0.16.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.google.guava/guava/bundles/guava-15.0.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.google.code.findbugs/jsr305/jars/jsr305-1.3.9.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/javax.inject/javax.inject/jars/javax.inject-1.jar', u'/home/ddavydov/ws/my_pants/.pants.d/javac/classes', u'/home/ddavydov/ws/my_pants/.pants.d/javac/resources', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.twitter.common/stats/jars/stats-0.0.90.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.twitter.common/stat-registry/jars/stat-registry-0.0.25.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.twitter.common/stat/jars/stat-0.0.27.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.twitter.common/stats-provider/jars/stats-provider-0.0.55.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.google.guava/guava/bundles/guava-16.0.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.google.code.findbugs/jsr305/jars/jsr305-2.0.1.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.twitter.common/application-action/jars/application-action-0.0.67.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/commons-lang/commons-lang/jars/commons-lang-2.6.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.google.inject/guice/jars/guice-3.0.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/javax.inject/javax.inject/jars/javax.inject-1.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/aopalliance/aopalliance/jars/aopalliance-1.0.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/org.sonatype.sisu.inject/cglib/jars/cglib-2.2.1-v20090111.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/asm/asm/jars/asm-3.1.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.twitter.common/util-sampler/jars/util-sampler-0.0.52.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.twitter.common/base/jars/base-0.0.85.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.twitter.common/quantity/jars/quantity-0.0.69.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.twitter.common/collections/jars/collections-0.0.72.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.twitter.common/util-system-mocks/jars/util-system-mocks-0.0.70.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/org.easymock/easymock/jars/easymock-3.2.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/cglib/cglib-nodep/jars/cglib-nodep-2.2.2.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/org.objenesis/objenesis/jars/objenesis-1.3.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/org.easymock/easymockclassextension/jars/easymockclassextension-3.2.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/junit/junit/jars/junit-4.11.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/org.hamcrest/hamcrest-core/jars/hamcrest-core-1.3.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.twitter.common/testing-easymock/jars/testing-easymock-0.0.3.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.google.testing/test-libraries-for-java/jars/test-libraries-for-java-1.1.1.jar', '/home/ddavydov/ws/my_pants/.pants.d/classpath/jars/com.sun.jersey/jersey-core/bundles/jersey-core-1.12.jar', u'/home/ddavydov/ws/my_pants/src/resources']
      
  3. src/python/pants/tasks/junit_run.py (Diff revision 1)
     
     
    You can use CommandUtil.execute instead of subprocess.call
  4. 
      
DD
JS
  1. The ripple to pushing this impl into Executor is actually pretty small.  I could 3 use sites that would need to switch from `runner = executor.runner(...)` to `with executor.runner(...) as runner:` if runner were to be a context manager.  I'd much prefer to see that change if you're game.  I can also put my money where my mouth is and pair a bit since I'm around if that seems too hairy.  I'd just like to cut down on conceptual sprawl early.
    1. I guess having to revert these changes when a more proper fix is thought of won't really be a big deal, so I'll go ahead and do this.
    2. I remember what I meant when I said it wasn't straightforward. There are a lot of places that don't use the runner in a purely functional way (i.e. they store it for later use). The current logic in some modules (e.g. ivy.py) would need a significant rework in order for this to work. I think it is fine to leave it the way I have now since it's a temporary solution, but if you see an easy way to make this change then please let me know.
    3. My analysis showed there were 2 locations that stored the runner for use 2 lines later.  I'll swing by.
  2. src/python/pants/tasks/junit_run.py (Diff revision 2)
     
     
    This is useful but doesn't actually say more than the code does.  The intersting bit is why this complexity is needed at all.  Thats the bit that will be noddled on by folks as time goes by - can I kill this craziness will be the primary question.  Please nod to the issue this solves in the docs.
    1. Good point, done.
  3. src/python/pants/tasks/junit_run.py (Diff revision 2)
     
     
    You might just de-generalize this and dispense with the bool map - 2 lists, loop and append to one or the other.
  4. src/python/pants/tasks/junit_run.py (Diff revision 2)
     
     
    Please use open_jar [1] - we try to control jre/jdk access via java/{Distribution,Executor} [1][2].
    
    [1] https://github.com/pantsbuild/pants/blob/master/src/python/pants/java/jar/__init__.py#L17
    [2] https://github.com/pantsbuild/pants/blob/master/src/python/pants/java/distribution/distribution.py
    [3] https://github.com/pantsbuild/pants/blob/master/src/python/pants/java/executor.py
  5. src/python/pants/tasks/junit_run.py (Diff revision 2)
     
     
    An interesting thing here is classpath instability between before this is introduced to after.  Classpaths _should_ be order independent though, so any breaks this causes are really legit pointers to some other issue with the test classpath.
    1. I could create one wrapper jar for each contiguous sequence of jars in the classpath, but ya the order should be independent anyways so that is probably overkill.
  6. src/python/pants/tasks/junit_run.py (Diff revision 2)
     
     
    Kill prints please.  Grab a logger and debug if you really want this to lay around.
    1. Good catch, I added this temporarily to give Tejal some proof that this worked that she requested.
  7. src/python/pants/tasks/junit_run.py (Diff revision 2)
     
     
    2 blank lines between top-level functions and classes.
  8. 
      
DD
DD
JS
  1. Thanks for pushing this into Executor Dan.
  2. 
      
DD
JS
  1. Ship It!
  2. 
      
DD
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks Dan!
https://github.com/pantsbuild/pants/commit/aa4a419785e1d45e778292ddc67df90420f60185
Loading...