[engine] Iterative improvements for`changed` and friends.

Review Request #4269 — Created Sept. 28, 2016 and submitted

kwlzn
pants
3798, 3899
pants-reviews
jsirois, nhoward_tw, stuhood, yujiec
  • Kill implied :: on no targets passed in the v2 TargetRoots path.
  • Make ./pants --enable-v2-engine list a functional no-op to match the behavior of ./pants --enable-v2-engine --changed-parent=HEAD list w/ no changes.
  • Misc GoalRunner refactoring.
  • Kill TargetRoots.as_string_specs().
  • Add FilesetWithSpec.iter_relative_paths(), a test and utilization in EngineSourceMapper.
  • Add a changed integration test for unclaimed source files.
  • Add a list integration test for ./pants --enable-v2-engine list.

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

ST
  1. Thanks Kris.

  2. Can we add a warning here indicating that this behaviour will be removed when v2 becomes the default?

    I can't see a good way to trigger an error here in the future, but if you can (perhaps based on differentiating between goals that consume targets, and goals that don't, and then blowing up in TargetRoots?) it would be great to add it.

    1. Also, if we're changing list's behavior, should we also do the same for filemap and fmt.isort's?

    2. @Stu - sure, done. in the v2 future case where TargetRoots is first class, I think at least one way that I see would probably be to plumb the TargetRoots object directly as context.target_roots and map TargetRoots.__iter__ -> TargetRoots.as_specs() or other extraction convention vs doing the earlier TargetRoots.as_specs() conversion in GoalRunner. then, tasks can do their own isinstance(context.target_roots, LiteralTargetRoots|ChangedTargetRoots) type checks and act accordingly.

      @Nick - probably, factored out a method on TaskBase and adjusted usage accordingly.

  3. src/python/pants/bin/goal_runner.py (Diff revision 1)
     
     

    Probably clearer/more-efficient to use the list constructor here.

  4. src/python/pants/bin/target_roots.py (Diff revision 1)
     
     
     
     

    This is duped in goal_runner _parse_specs... it's a small bit of code, so not a huge issue, but might hint at a larger duplicated codepath I'm not seeing.

    1. this should be it - factored this out into a TargetRoots @classmethod instead.

  5. Offtopic for today, but it occurs to me that as part of our native support for filesystems operations in the graph, a cleaner way to support this might be to add a native ProductGraph method to "find all subjects of types (X, Y, Z) below some Node/Selector".

    This sortof conflicts with what I've said about the public API of the ProductGraph and Engine (ie: you shouldn't investigate the structure of the graph, as that is an implementation detail). But on the other hand, I think we can safely say that quickly/easily/accurately determining Filesystem/Subsystem dependencies is the entire reason the Engine exists in the first place, so we should do whatever it takes to make this type of usecase easy.

    1. seems reasonable.

  6. src/python/pants/source/wrapped_globs.py (Diff revision 1)
     
     
     
     
     

    It would be nice to use this in target.py or https://github.com/pantsbuild/pants/blob/master/src/python/pants/source/payload_fields.py#L66 , to be doubly sure that it doesn't go out of sync.

  7. 
      
YU
  1. Looks good to me! One minor comment.

  2. src/python/pants/bin/goal_runner.py (Diff revision 1)
     
     

    I prefer to keep this in __init__, since this call is not part of setup below.

    1. sure - reverted.

  3. 
      
NH
  1. Ship It!
  2. 
      
KW
KW
KW
Review request changed

Status: Closed (submitted)

Change Summary:

thanks Stu, Yujie & Nick! submitted @ https://github.com/pantsbuild/pants/commit/a5906b597813ef1b4ba8eefb609a8c38976c04d4

Loading...