Optionally assert targets are unread before replacing target_roots

Review Request #1543 — Created Dec. 26, 2014 and submitted

davidt
pants
891
f80a52b...
pants-reviews
jsirois, patricklaw, zundel

Allow callers of Context.replace_targets to first ensure they are not violating other callers assumptions, specifically by asserting that targets have not previously been read by another task.

The check is opt-in for now, but is easily switched to opt-out later if it seems like a good idea.

Added tests for Context.replace_targets and Context.targets_retrieved.

./pants test tests/python/pants_test/tasks:context

https://travis-ci.org/pantsbuild/pants/builds/45157436 passed.

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
DA
DA
PA
  1. lgtm sans naming nits

  2. src/python/pants/goal/context.py (Diff revision 2)
     
     

    Not a big fan of this name. It sounds like it's going to return the targets that have been retrieved.

    1. targets_were_retrieved ? targets_field_accessed?

  3. src/python/pants/goal/error.py (Diff revision 2)
     
     

    Maybe TargetRootReplacementError? Out of context it's not clear at all which "Root" is being replaced.

  4. 
      
DA
JS
  1. This looks good, but also the only user is here: https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/ide_gen.py#L212
    It would be better to just kill the functionality, but it needs to skate along until the current round engine work settles and supports acting on arbitrary universes of targets natively.

    Does Foursquare have code that uses this?

    1. Yup, in several places. And we're in the process of open sourcing and pluginizing some of those users.

    2. Yes, we do.

      We have tasks that, during prepare, find their own targets on which they wish the round to operate (usually by consulting SCM, but not always) and then call this.

      We've tried a couple other approaches for how to implment these kinds of tasks -- leading contenders were a new lifecycle method on Task that could return alternate roots, or a separate thing that tasks could register with goal runner, eg a TargetDiscoveryStrategy, with the default one just searching command line for target specs.

      After playing with a few of these ideas, I've come to believe that just calling context.replace_targets in prepare is the least disruptive / complex approach, at least for now.

      If we introduced a separate method on task, we'd need to first construct all tasks for the round, then see if at-most-one returned alternate target roots. But to know the tasks that will be scheduled in a round, we need to iteratively call prepare to get their dependencies. There are prepare methods that set up state using context.targets, so we'd need to have already correctly set targets before before those run, introducing a chicken-egg situation. A more ambitious change might be to remove targets from Context altogether, making them an arguemnt to Task.execute(), with a new lifecycle method between prepare and execute, but that seemed much more invasive than this approach.

      The biggest advantage I saw of these other approaches was that I would have strict control over when targets were swapped, specifically that I could ensure it happened before anything read or used contex's target roots. However I think the assertion added in this changeset can give me most of the same confidence without the substantial added complexity in goal runner / round engine / Task / etc.

    3. As this change seem to be look okay to people for now, I'll go ahead and submit it. If there's a broader discussion of how tasks that change target roots should work, that is probably better for pants-devel -- can easily revert this specific change, or remove it completely, if we change come up with a different approach for changing target roots.

  2. 
      
DA
Review request changed

Status: Closed (submitted)

Change Summary:

21c47db0197b330361624ac179494d79d5a671d7
Loading...