Minimal target "tags" support

Review Request #1227 — Created Oct. 27, 2014 and submitted

davidt
pants
9da6017...
pants-reviews
patricklaw, zundel

This doesn't provide any addtional end-user functionality, but has been useful as a channel for custom tasks and BUILD files to communicate addtional information about targets.

We've been rebasing this change on top of upstream pants for releases at Foursquare for a few months now and we've found it useful in implmenting downstream custom tasks (like our graph rules), which need a place to put additional information in the build graph, while staying out of the internally meaningful things like labels.

https://travis-ci.org/pantsbuild/pants/builds/39211424

LA
  1. 
      
  2. src/python/pants/base/target.py (Diff revision 1)
     
     

    :param tags: You know, like delicious
    :type tags: ?iterable of strings or something?

    1. Wow, my tone sure was flippant. Sorry, I thought you were David Turner. blush I humbly suggest adding docstring for this new param.

    2. Heh. Added.

  3. 
      
DA
ZU
  1. 
      
  2. src/python/pants/base/target.py (Diff revision 2)
     
     

    Maybe we should call out in the doc string that this is intended for use by custom tasks. Since there are no referneces to it in the codebase, it is likely to get refactored out.

    Did you want to add it to the payload? I don't know what these custom tasks are doing, but if they change the way something is compiled, it might be useful. Also, you wouldn't need to add the tags property.

    1. Hm, I hadn't thought of including in payloads.

      As we are currently use them, I don't think tags should be in payloads, primarily since they are mutable and payloads should not be.

      In our current usage a) mutability is required and b) they're purely information about a target's role in the larger graph, for use by tasks that reason about the graph, and they do not change how any individual target is built.

      We rely on mutability right now: we use tags as place for separate tasks to pass information about tagets (one writes tags, the other reads them).

      Given this, I'm inclined leave tags out of payloads and add a comment saying tags must not be used to determine how a particular target is built and are only for providing information to tools like graph queries.

    2. A concrete example of our usage, in case it helps:

      Task "tag" uses path patterns defined in pants.ini to programmatically add tags to matching targets.
      "tag" runs before a "validate" task, which checks our build graph rules, written in terms of tags, to determine if the graph is valid or not.

      https://gist.github.com/dt/55b1a37c35cde9812d0e#file-tag-py

      An example pattern in pants.ini looks like [target-tags.by-prefix]…'lib/common': 'common', and then various rules can refer to common.

    3. Seems fine to leave it out of payload, I just thought I'd bring it up.

      I'd still like to see some kind if indication either in the docstring or in a comment to keep ups from inadvertently removing it in the future.

    4. Yeah, it is an interesting question, but for now I think it is easier to leave it out of payloads. If we found a way to make it immutable, I'd likely lean towards including it and allowing it to be used to change how something is built, but for now, I'll leave it as is with a disclaimer.

      Updated diff to include a comment about expected usage.

  3. 
      
DA
DA
ZU
  1. Ship It!

  2. 
      
PA
  1. Ship It!

  2. 
      
IT
  1. Ship It!

  2. 
      
DA
Review request changed

Status: Closed (submitted)

Change Summary:

d97b7b95881bbc14285a00de4610e59ff717d8b0
Loading...