[engine] fix address node creation in v2 build graph; fix filedeps

Review Request #4250 — Created Sept. 20, 2016 and updated

Information
ity
pants
3752, 3891
Reviewers
pants-reviews
kwlzn, nhoward_tw, peiyu, stuhood, yujiec 
- Preserve build file when mapping to be able to create BuildFileAddress when required.

- Re-use already parsed Addresses in addressables.

- Add project_tree attribute to AddressFamily for completion.

- Add types for BuildFileAddress to validate against during BuildGraph creation.

- Working on this change made it obvious that Address/BuildFileAddress can really be the same thing. They do not benefit from inheritance as such at the moment and I do not see any future subclassing required for these. In fact, they do add to the complexity of parsing, memoizing, retriving. To keep this change simple, split the refactor to another rb.
Locally, ./pants --enable-v2-engine filedeps xx:: was failing before this change, passing now.

Also, did simple functional testing against v1 engine outout.
  • 0
  • 0
  • 5
  • 0
  • 5
Description From Last Updated
ST
stuhood 
  1. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      src/python/pants/bin/engine_initializer.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    Objects "in" the ProductGraph should not have direct access to the filesystem (ie, the ProjectTree), as that would allow them to bypass filesystem invalidation.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      makes sense. thanks - still learning my way around the new engine. moved out to create_graph_tasks()
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      src/python/pants/engine/addressable.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    BuildFileAddress extends Address, so this is redundant.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      src/python/pants/engine/addressable.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    To be correct, this should still normalize the address as the other branch does, but should pass the additional arguments needed by the BuildFileAddress constructor.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      this one was interesting, I was a little confused by this -- any idea why we create a new Address here instead of modifying the existing one?
      
 
      

      2. 


 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      src/python/pants/engine/graph.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Rather than adding the AddressMapper reference to the AddressFamily, you should probably request it below in any functions that need it.
    

    It looks like only resolve_unhydrated_struct uses it, so to get the additional argument there, you would simply add a SelectLiteral(address_mapper, AddressMapper) argument to the definition of resolve_unhydrated_struct.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      apologies, this shouldnt have been here -- just needed the project_tree
      
 
      

      2. 


 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      src/python/pants/engine/mapper.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    I don't like this dependency.  The BuildFile and BuildFileAddressMapper APIs are the ones we're attempting to replace, so we should be trying to reduce our dependence on them.
    

    Could we change BuildFileAddress to hold the path of a BuildFile, rather than an object?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      just a quick look shows thats an involved change, which I am happy to make. although, to unblock this I modified BuildFileAddress to to have the dependence on BuildFile instead of this dependence. See if thats something we can live with, until I circle back with the path instead of BuildFile change? 
      
If it works, then I will update this RB with unit tests fixed (left them unchanged for now, so travis will fail)
      
If not, then I would like to make that change separately since it will bloat up this review.
      
 
      

      2. 


 
    


    

 
    

    7. 




  7. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 




  

 
















 



 

  

   





 IT





   

   

  

 



 















 





 

  

   





 ST





   

   







  

 



 

  


   

   

    
 
stuhood


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Thanks, closer!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      src/python/pants/engine/graph.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    This check should probably move to the top with the if not struct check, so that those can just be accomplished together.
    

    Falling through to the other case would be a fatal case, because the AddressFamily should always contain BuildFileAddress objects, right?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      ...but also, I'm not sure that pre-creating the BuildFileAddress objects is actually necessary, rather than just-recreating them each time. They're cheap.
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      src/python/pants/engine/graph.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    It's not enforced anywhere yet, but as mentioned before: functions in the graph should not directly access the file system, or the ProjectTree.
    

    So (ideally) we'd remove the ProjectTree reference from BuildFileAddress entirely. Similar to how an Address object doesn't know which build_root it is relative to, a BuildFileAddress need not have a ProjectTree.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 





 

  

   





 YU





   

   





  

 



 

  


   

   

    
 
yujiec


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ity,
    
you may want to add some integration tests with ensure_engine decorator to verify filedeps works in both v1 and v2 engine and produce same output.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   





 IT





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  








 
Change Summary:

 
Addressed Stu & Yujie's comments.





 



  

   
Diff:

   
    
 Revision 3 (+137 -50)
Show changes
 
    contrib/buildgen/src/python/pants/contrib/buildgen/build_file_manipulator.py 
    src/python/pants/backend/project_info/tasks/filedeps.py 
    src/python/pants/backend/project_info/tasks/ide_gen.py 
    src/python/pants/build_graph/address.py 
    src/python/pants/engine/addressable.py 
    src/python/pants/engine/graph.py 
    src/python/pants/engine/mapper.py 
    src/python/pants/engine/scheduler.py 
  6 more
 

   		
  



 










  

 
















 





 

  

   





 ST





   

   







  

 



 

  


   

   

    
 
stuhood


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Thanks Ity. It still looks possible to completely remove the BuildFile object from BuildFileAddress, but it doesn't seem like a blocker. Maybe a new ticket?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      src/python/pants/backend/project_info/tasks/ide_gen.py
      


       (Diff revision 3)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    This will be broken in the v2 engine, because a BuildFile will not exist.
    

    Rather than getting the ProjectTree from the BuildFile, this Task could probably get it from self.context, which would mean that it would only need the rel_path of the BuildFileAddress, rather than an actual BuildFile object.
    

    If that change were to be made, it would likely be possible to remove BuildFileAddress._build_file entirely, which would be very nice.
    

    But, not a blocker... would be good to land this rather than changing too much more.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      src/python/pants/engine/mapper.py
      


       (Diff revision 3)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    This is a pretty generic argument... if you could either avoid adding it, or carefully document it, that would be best.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 





 

  

   





 YU





   

   







  

 



 

  


   

   

    
 
yujiec


    
   


  

  

  



    




  1. 
 
    
  
    
   
    LGTM!
    
I like that we don't use BuildFile objects in v2 engine!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
    This is unused.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 









     

    

   



  

  

   
   Loading...