Refactor duplicate dependency detection to be in the BUILD file parsing logic

Review Request #1362 — Created Nov. 19, 2014 and submitted

zundel
pants
zundel/dup-dependency-refactor
808
c075f8b...
pants-reviews
ity, johanoskarsson, patricklaw, stuhood

This is a refactoring of https://rbcommons.com/s/twitter/r/1332/. The intent of the original
change was to keep BUILD files from declaring the same target twice in a dependency[] attribute.
This refactor was done to avoid triggering this error due to implicit dependency injection.
For example, We don't really care if an explicit dependency is added to a BUILD file that
duplicates a dependency injected by a code generator like protobuf_gen.py.

Now the same case fails with:

Exception message: Invalid target TargetAddressable(..., name=greet, **kwargs=...): dependencies must be unique.  '3rdparty:guava' occurs more than once.
 while executing BUILD file /Users/zundel/Src/pants/examples/src/java/com/pants/examples/hello/greet/BUILD
 Loading addresses from 'examples/src/java/com/pants/examples/hello/greet' failed.
  referenced from examples/src/java/com/pants/examples/hello/main:main-bin
  referenced from examples/src/java/com/pants/examples/hello/main:main

CI is running.

  • 0
  • 0
  • 1
  • 1
  • 2
Description From Last Updated
ZU
  1. 
      
  2. src/python/pants/base/build_graph.py (Diff revision 1)
     
     

    Does anyone still care about this message? I tamped it down to a debug msg.

  3. 
      
PA
  1. 
      
  2. I'm not sure if this is the right place for this. It would be much better if you were looking at Addresses, which are normalized. For example, :foo and the/same/folder/foo:foo and //the/same/folder/foo ... etc could all refer to the same thing, and it would make sense to trigger this error.

    And these can't be normalized here, because an Addressable is (intentionally) not told what its address and relpath are.

    Probably this belongs at the point where an Addressable becomes a Target: BuildGraph.target_addressable_to_target.

  3. 
      
ZU
PA
  1. Ship It!

  2. 
      
JO
  1. 
      
  2. Shouldn't this be DuplicateAddressError?

    1. If you look at the way the exception catching logic works, Any subclass of AddressLookupError can be caught and re-thrown. Notice how the original exception doesn't mention the build file name or target, but the re-thrown message captures all that.

      A more targeted unit test might test just the method that is throwing the original DuplicateAddressException, but this test verifies that the intended error is firing, and that the chained exception handling is giving good feedback to the user.

  3. 
      
IT
  1. Ship It!

  2. 
      
ZU
Review request changed

Status: Closed (submitted)

Change Summary:

commit 892ce88
Loading...