Introducing target aliases in BUILD files.

Review Request #3939 — Created May 27, 2016 and submitted

gmalmquist
pants
gmalmquist/aliased-targets
3513
pants-reviews
benjyw, jsirois, nhoward_tw, patricklaw, stuhood, zundel
This introduces a new object for use in build files: `alias()`.

It can be used like so: `alias('foo', ':bar')` to allow referencing
`':bar'` by the name `':foo'`. This provides both practical and
aesthetic benefits. Aesthetically, it cleans up common constructs:

Eg, in a maven repo:
```
target(name='foobar',
  dependencies=[
    'foobar/src/main/java:lib',
  ],
)
```
becomes simply:
```
alias('foobar', 'foobar/src/main/java:lib')
```

Aliases also behave slightly differently than normal wrapper
targets. The substitution task which processes target aliases
injects alias's dependencies directly into their dependees. This
means that intransitive targets can be referred to via aliases
without breaking their dependencies.

We've been using this extensively at Square as an internal plugin
for a while now, but it seems likely that other folks could benefit
from it.

Added passing tests.

Build went green: https://travis-ci.org/pantsbuild/pants/builds/135101452

  • 0
  • 0
  • 5
  • 1
  • 6
Description From Last Updated
ZU
  1. Here's a concrete description of our use case.

    We used to have build files structured like this:

    foo/BUILD

      jvm_binary(name='foo',
        ...
      )
    

    Where foo was the default target. Folks are used to running ./pants binary foo , ./pants run foo, ...

    But then we wanted to introduce a new kind of target and also introduce a convention for the jvm_binary targets that would be eaiser to get to work in our scripts.

    foo/BUILD

      target('foo', dependencies=[':bin'])
    
      square_deployable(name='artifact',
        ...
        dependencies=[':bin'],
      )
    
      jvm_binary(name='bin',
        ... same as before..
      )
    

    Now our CI scripts are cleaner, and most commands like ./pants compile foo continued to work, but ./pants run foo and some similar stopped working. Those task operate on target roots.

    We went around about this and decided to implement alias:

    foo/BUILD

      alias('foo', ':bin')
    
      square_deployable(name='artifact',
         ...
         dependencies=[':bin'],
      )
    
      jvm_binary(name='bin',
        ...
      )
    
    1. Yeah, I should also mention that if an aliased target is in target_roots, the actual target it references gets injected into the target_roots, which matters for some tasks.

    2. Out of curiosity, in this example, why wasn't this sufficient?

      jvm_binary(name='foo',
        ...
      )
      
      square_deployable(name='artifact',
        ...
        dependencies=[':foo'],
      )
      

      I'm fine with this change, but I do think that we should "just do the right thing" in cases like this, where it's obvious what the user meant, without forcing them to change BUILD files on a technicality. In this case - E.g., if the target root of ./pants run is a scaffolding target that points to a binary, then just run that. But that's a bigger change for another day.

    3. In this case we name the shaded binaries ':shaded-jar', and we try to maintain the flexibility to easily change around what the default target points to.

      Some of it has to do with how we generate BUILD files from pom.xml's -- we expect particular patterns of directory structures and spec names.

  2. 
      
ZU
  1. Ship It!
  2. 
      
BE
  1. 
      
  2. As a matter of clarity and grammatical pedantry, shouldn't this should be called AliasTarget or something like that? AFAICT this represents the name, not the aliased destination (which is in this things deps).

    1. Aliased made more sense to me, but I have no qualms with renaming it to Alias; this isn't user-facing either way.

  3. This is a neat way to implement this functionality!

  4. This would be slightly more readable if the test were inverted:

    if isinstance(...):
    ...
    else:
    ...

    It prevents the reader from having to grok the double-negative implied by the else.

  5. 
      
GM
NH
  1. 
      
  2. Rather than registering this target type, have you considered creating a TargetMacro.Factory? That way you could drop the aliased_target registration and the associated _direct checking.

  3. nit: s/target:/address:/.

  4. I think these arguments should be required since it's an error if they are missing.

    1. They effectively are, but doing it this way yields better error messaging.

    2. But I think with the optional args, if alias were called like alias(target='some:address'), there would be no error, it would just silently fail to register a target.

  5. src/python/pants/build_graph/aliased_target.py (Diff revision 1)
     
     
     
     
     
     

    How about raising the error earlier, and losing the inline if else expression?

    1. I wanted to be able to raise a target definition exception to be maximally user-friendly, and I can't do that without a target instance :-P

    2. If that's important behavior, could you exercise it with a test?

  6. src/python/pants/core_tasks/register.py (Diff revision 1)
     
     

    It might be worth using first=True here. Since this is modifying the root of all build graph ops, it's very global. If something else that interacts with targets were to be registered, or run earlier, it could have unintended consequences.

  7. You could use context._replace_targets here, which would make searching for target root modifications easier later. From the comment on it, it seems like it exists for this kind of thing.

    Doing that would also mean that the changes to target roots would happen all in one go rather than incrementally, so if there were a thread running that read target_roots, they wouldn't be empty, or in a partial state.

  8. Should this also work for address related inputs that aren't in the targets dependencies, eg deferred sources, or would you say that's just unsupported? Can you think of a way to document this so that the distinction is clear?

    1. Hmm; it would be nice if it worked with deferred sources. Maybe I'll just add a note in the docstring and a TODO.

  9. Maybe the aliased targets ought to stay in the target roots some how?

    Removing them means that aliases won't show up in the results of ./pants list calls since these are removed early.

    But, the eliding behavior isn't consistent, because if the alias also exists somewhere else in the graph, you'll see it there for tasks that do traversals.

    For instance, ./pants paths will show both the aliased path and the unaliased path.

    1. Yeah, I worry about unintended consequences if we leave them in the graph though. It seems cleaner to remove them.

  10. 
      
GM
NH
  1. Ship It!
  2. 
      
GM
BE
  1. Ship It!
  2. 
      
GM
GM
Review request changed

Status: Closed (submitted)

Change Summary:

In b0836bf07dced36fd9a7872ce3c6d2a1f59bb774. Thanks Eric, Nick, and Benjy for the reviews (and Stu for the v2 engine fix)!

Loading...