Minimize classpath pants passes to java for running tests

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

ddavydov
commons
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:
./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
JS
  1. Many apologies for the thrash.  But this code is dead [1].  Can you apply this to pantsbuild/pants and re-post?  The layout has changed there:
    src/python/twitter/pants -> src/python/pants
    
    [1] https://github.com/twitter/commons/pull/281
  2. 
      
DD
Review request changed

Status: Closed (submitted)

Change Summary:

commit aa4a419785e1d45e778292ddc67df90420f60185
Author: Travis Crawford <travis@twitter.com>
Date:   Fri Apr 18 14:22:52 2014 -0700

    Minimize the classpath pants passes to java for running tests.
    
    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.
    
    By Dan Davydov at https://rbcommons.com/s/twitter/r/205/
Loading...