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

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

Information
zundel
pants
zundel/dup-dependency-refactor
808
c075f8b...
Reviewers
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
zundel 
  1. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  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. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 





 

  

   





 PA





   

   







  

 



 

  


   

   

    
 
patricklaw


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    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. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   





 ZU





   

   

  

 



 















 





 

  

   





 PA





   

   







  

 



 

  


   

   

    
 
patricklaw


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   





 JO





   

   









  

 



 

  


   

   

    
 
johanoskarsson


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    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.
      
 
      

      2. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 





 

  

   





 IT





   

   







  

 



 

  


   

   

    
 
ity


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   





 ZU





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
commit 892ce88





 


 










  

 









     

    

   



  

  

   
   Loading...