Fix the SetupPy target ownership check.

Review Request #4315 — Created Oct. 15, 2016 and submitted

jsirois
pants
jsirois/python/publish/fix_ownership_check
3968, 3969
3a022b9...
pants-reviews
benjyw, kwlzn
Previously the check was too restrictive and considered targets that did
not own files when the single publish ownership check is only intended
to prevent publishing the same file in more than one package.

 src/python/pants/backend/python/tasks/setup_py.py             | 14 ++++++------
 tests/python/pants_test/backend/python/tasks/test_setup_py.py | 58 ++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 64 insertions(+), 8 deletions(-)

Tested this over in Aurora ad-hoc and it solved the publish problem
there as well as passing the new test emulating the Aurora prep_command
arrangement.

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

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
  1. 
      
  2. I don't understand this sentence. What do dependent targets have to do with it? The implementation of this method below doesn't reference dependencies at all.

    And are dependents the ones that depend or the ones that are depended on? The word "dependent" means the former on your tax returns, but often the latter in our codebase and elsewhere. Natural language sucks.

    1. See what you think of the re-word.
    2. Great, I get it now. Thanks.

  3. 
      
  1. Ship It!
  2. 
      
  1. 
      
  2. If you have time, would be awesome to see validation of the remote_sources case as well.

    1. The DeferredSourcesMapper is linked into the product graph ad-hoc right now - only for jvm stuff and protobuf (but not thrift, go, python etc).  I'd like to pass on this as pretty far out of scope as a result, but I've filed this issue to track elevating remote/deferred sources processing to 1st class: https://github.com/pantsbuild/pants/issues/3994
  3. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Now on master:

git log -1 origin/master
commit 0abade1c40309ad63d22d17cef9e301aa9b8f8c6
Author: John Sirois <john.sirois@gmail.com>
Date:   Sat Oct 22 16:23:10 2016 -0400

    Fix the SetupPy target ownership check.
    
    Previously the check was too restrictive and considered targets that did
    not own files when the single publish ownership check is only intended
    to prevent publishing the same file in more than one package.
    
    Testing Done:
    Tested this over in Aurora ad-hoc and it solved the publish problem
    there as well as passing the new test emulating the Aurora `prep_command`
    arrangement.
    
    CI went green here:
      https://travis-ci.org/pantsbuild/pants/builds/169610485
    
    Bugs closed: 3968, 3969
    
    Reviewed at https://rbcommons.com/s/twitter/r/4315/
Loading...