Cleanup `BinaryUtil`.

Review Request #4108 — Created July 21, 2016 and submitted

jsirois
pants
jsirois/binary_util/cleanups
3710
d0b64a2...
pants-reviews
gmalmquist, zundel
This simplifies some logic and presents more directly useful error
messages as well as firming up the `osutil.get_os_id` contract with
docs and a now-uniform return protocol.

The `test_support_url_multi` test is also simplified to remove the
ineffective dedup test and is sped up and stabilized by switching a real
bintray protoc fetch out for a local file url fetch of a small test
file.

 src/python/pants/binaries/binary_util.py             | 44 +++++++++++++++++---------------------
 src/python/pants/util/osutil.py                      |  8 ++++++-
 tests/python/pants_test/binaries/BUILD               |  2 ++
 tests/python/pants_test/binaries/test_binary_util.py | 57 +++++++++++++++++++++++++++++---------------------
 4 files changed, 61 insertions(+), 50 deletions(-)

Now locally green on every run vs ~15% of the time and much faster:
./pants test tests/python/pants_test/binaries/:

CI went green here:
https://travis-ci.org/pantsbuild/pants/builds/146512692

ST
  1. Thanks John!

  2. 
      
JS
IT
  1. Ship It!
  2. 
      
ZU
  1. FYI @gmalmquist is out until next week.

  2. I think the point of the 2 lines that you deleted from the origin was to have a path to protoc in two places and make sure it picked up the one from the desired order in the baseurls list. Having said that, I'm not sure what the desired order is.

    Admittedly, it wasn't a great test. I think you could improve this logic by writting a file named protoc to invalid_local_files and making sure that we picked up the "right" protoc.

    1. I read it as the comment noted, an attempt to confirm duplicate urls were not probed.  For that probe to have worked, the count mechanism needed to work and did not since there was only ever 1 url returned from the contextmanager by design.  Since the dup test was completely broken with no obvious easy way to implement/fix, I skipped/nuked.
      
      I believe the test I have achieves your aim wrt invalid_local_files, since the only way to get that url to be skipped is to not have a protoc file at the right path.  As I have it - there are 0 paths under invalid_local_files, which seems just as good a test as touching, say `binary_path + '_not_actually' under invalid_local_files.
  3. 
      
JS
Review request changed

Status: Closed (submitted)

Change Summary:

Now on master:

git log -1 origin/master
commit 783feee3639953f11654cdd9284d6b8b0d6b5a26
Author: John Sirois <john.sirois@gmail.com>
Date:   Sat Jul 23 16:45:16 2016 -0600

    Cleanup `BinaryUtil`.
    
    This simplifies some logic and presents more directly useful error
    messages as well as firming up the `osutil.get_os_id` contract with
    docs and a now-uniform return protocol.
    
    The `test_support_url_multi` test is also simplified to remove the
    ineffective dedup test and is sped up and stabilized by switching a real
    bintray protoc fetch out for a local file url fetch of a small test
    file.
    
    Testing Done:
    Now locally green on every run vs ~15% of the time and much faster:
    `./pants test tests/python/pants_test/binaries/:`
    
    CI went green here:
      https://travis-ci.org/pantsbuild/pants/builds/146512692
    
    Bugs closed: 3710
    
    Reviewed at https://rbcommons.com/s/twitter/r/4108/
Loading...