A python_binary_create task, to bring pex-building under goal.

Review Request #470 — Created June 9, 2014 and submitted

benjyw
pants
pants-reviews
jsirois, patricklaw, wickman
This task supports 'compatibiity' specs in python_binary targets, to restrict the interpreters the pex machinery can select from.

Old functionality via 'command' is unchanged.

Decided, on balance, not to backport the compatibility support into the old command pathway. This is for convenience (PythonTask already has machinery for choosing interpreters), and to minimize disruption at Twitter due to introduced bugs.  The only demand for this feature right now is from us at Foursquare, and we can easily switch to goal-based python binary building. And anyway, 'command' will go away "soon".
Manually ran the new echo_interpreter_version binary with various different compatibility settings. 

Ran all unittests, for good measure.
  • 1
  • 0
  • 0
  • 0
  • 1
Description From Last Updated
Not sure what this means exactly. Is this a problem with InterpreterCache? Or am I using it wrong? BE benjyw
PA
  1. Exciting!
  2. Fixup import spacing
  3. 
      
BE
JS
  1. 
      
  2. The pants.ini still contains a stie-wide platforms config, it seems like that should be the fallback if binary.platforms is blank.
  3. Since you have all 13 binaries active in the context at hand in execute, it would be good to fail-fast on dup binary.names.
  4. Fwict this leaves chroot tmpdirs laying about even on success.  I think you need:
    try:
      env.build(filename)
    finally:
      env.chroot.delete()
  5. This is problematic IIRC because it will find all pythons and fail when it hits python<=2.4 or 3.0<=python<3.3 - the 3 range due to a zig-zag in how 3 series handled unicode before settling in 3.3+.  Travis-ci will tell, that's where we hit this before and its why the crazy constraints exist in the ci.sh.
  6. tests/python/pants_test/python/BUILD (Diff revision 1)
     
     
    Please stick to the style or change it wholesale
  7. tests/python/pants_test/python/BUILD (Diff revision 1)
     
     
    How about a real test?  Tejal started a precedent with the lone :integration and I think it would be easy enough to use that to subshell `./pants ...` in concert with a check for 2.6 and 2.7 and a test for each available via dedicated 2.6 and 2.7 binary targets here.
  8. 
      
BE
  1. Addressed all comments. In particular, added a test as you suggested. 
    
    However I'm not clear on what one of your comments meant, if you could clarify that would be great.
    
    See below for details.
  2. That's done later, in resolver.py, which is the only thing that uses platforms: 
    
    https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/python/resolver.py#L81
  3. Not sure what this means exactly. Is this a problem with InterpreterCache? Or am I using it wrong?
    1. InterpreterCache will find all pythons on the PATH.  Say one of those is python 3.2.x (Travis CI slaves have these) - then this code used to trigger in python commons:
      PythonInterpreter.from_binary: https://github.com/twitter/commons/blob/master/src/python/twitter/common/python/interpreter.py#L284
        -> PythonInterpreter._from_binary_external: https://github.com/twitter/commons/blob/master/src/python/twitter/common/python/interpreter.py#L241
          -> (uses) https://github.com/twitter/commons/blob/master/src/python/twitter/common/python/interpreter.py#L31
      
      That would trigger a unicode incompat and systemically fail for 3.2.x even when you never wanted 3.2.x setup in the 1st place per constraint filters.
      
      That tour through the code suggests the code path has changed - we don't call PythonInterpreter.from_binary any longer - so you may be safe.  If there is an issue a travis-ci `./pants goal binary src/python/...` would blow up.
      
    2. And this failed for exactly this reason in travis-ci: https://travis-ci.org/pantsbuild/pants/jobs/27357695
      Please revert, circle back, and fix.
  4. tests/python/pants_test/python/BUILD (Diff revision 1)
     
     
    Done.
  5. tests/python/pants_test/python/BUILD (Diff revision 1)
     
     
    I wanted to write a proper test but wasn't sure how to deal with the fact that the system you run the test on might not have the right python interpreters on it, so you wouldn't know if the test was actually testing anything. 
    
    But yeah, making it an integration test that doesn't run under :all makes sense so I've done so. 
    
    Note that I had to change the pants runner a little bit so that the subprocess doesn't deadlock waiting for the pants workspace lock. This only surfaced when running the tests in the new python test goal.
  6. 
      
BE
  1. 
      
  2. 
      
BE
JS
  1. LGTM if travis-ci works.
  2. I had a change in yesterday this will conflict with pretty nicely.
  3. Consider just adding 'pants_workdir' to the config and then simplifying/uniformizing the ini string construction.
  4. You get the print data for free via skip - just run with -v.  I'm not blocker-opposed, but I am generally against print in unit tests.
    
    Also - you might just lift the helpers to top-level functions and:
    @pytest.mark.skipif("not have_version('2.6'")
    def test_select_26(self):
      self.do_test_version('2.6')
    ...
    
  5. 
      
BE
  1. 
      
  2. Done, but with code to ensure that the DEFAULT section is emitted first.
  3. This one I will push back on, as I'm happy to have (modest) print statements in unittests. Anything to help you not waste time figuring out what went wrong. 
    
    And re using decorators - I prefer not to, for similar reasons.
  4. 
      
BE
Review request changed

Status: Closed (submitted)

Loading...