Allow adding entries to source_roots

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

dturner-tw
pants
fe9caa5...
pants-reviews
ity, jinfeng, jsirois, patricklaw, peiyu, zundel

Allow adding entries to source_roots

Ran source_roots tests.

ZU
  1. I won't oppose this change if others think it is useful, but in my environment, I would consider two source_root() decls on the same directory a configuration bug and would want to know. It isn't that big of a deal though.

    FYI, I think the original issue David is trying to work around is that maven_layout() is missing a target type he wants to add. I have similar issues with maven_layout() (doesn't include the right targets, doesn't include all the dirs I want) so I don't use it at all.

    1. One other suggestion. You could create a local plugin and define a new function similar to maven_layout() , e.g. tw_maven_layout() that includes the additional targets you want. then switch your maven_layout() references in your BUILD file to your new one.

    2. +1 to the general issue of having multiple source_root decls for a single root. Also, as mentioned I think in another email thread, at Foursquare we just don't enforce target types for source roots. The source root of a given target is just the closest ancestor directory that was declared as a source root.

      Re the second comment: Or straight up override the alias for maven_layout with your own implementation that sticks in more target roots. You don't have to change the name.

    3. We do override the alias for maven_layout. But without this patch, we would have to copy-paste the entire OSS pants maven_layout into ours (instead of simply calling the original). And then we would have to remember to keep it up-to-date as OSS pants changes maven_layout. That's why I wrote this patch.

    4. Ok, then changing maven_layout() to use a newly introduce append_source_root() instead of source_root() makes sense to me.

  2. 
      
DT
ZU
  1. Ship It!

  2. 
      
PA
  1. Ship It!

  2. 
      
DT
DT
IT
  1. lgtm!

  2. 
      
DT
Review request changed

Status: Closed (submitted)

Loading...