support dependees in v2 engine

Review Request #4202 — Created Sept. 1, 2016 and submitted

yujiec
pants
3809, 3826
pants-reviews
benjyw, kwlzn, nhoward_tw, stuhood

The goal "dependees" is not supported in v2 engine currently because scan_build_files is not implemented for v2 address_mapper.
If we use scan_addresses() instead of scan_build_files, we can make dependees work in both v1 and v2 engine.

This review does:
1. Modify "dependees" logic using scan_addresses() instead of scan_build_files().
2. Sort the output if output format is in "json".
3. Add integration tests for "dependees"
4. (unrelated to dependees) Raise an error when bundle(fileset=...) does not match any files.

ci green:
https://travis-ci.org/pantsbuild/pants/builds/157238701

YU
YU
YU
KW
  1. lgtm!

  2. more idiomatic:

    for bundle_counter, bundle in enumerate(app.bundles):
      ...
    

    (and no need to manually increment bundle_counter)

    1. neat! thanks. done

  3. 
      
YU
IT
  1. Ship It!
  2. 
      
NH
  1. Looks pretty good!

    I've got a few small comments.

  2. If we want the output sorted, maybe we should pass sort_keys=True?

    1. good point! done.

  3. Thanks for the integration tests!

  4. tests/python/pants_test/engine/legacy/test_dependees_integration.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    I think these should not just assert set equality. It'd be useful to ensure that the ordering remains the same.

    1. hmm, why would the ordering matter/need to stay the same?

    2. I just ensure the ordering when output format is json. The main motivation is easier to compare output, since in json format, using "set" is not that ideal. The tests highlighted by @Nick is not json format, thus no ordering is enforced. As for @Kris's point, I don't actually have a strong opinion to maintain the ordering in json.

    3. One reason would be to ensure we're testing that the ordering is deterministic. As a user, it might be annoying if running dependees two times resulted in two different outputs--particularly if the list was large. Another would be that different orderings between v1, v2 might point to behavior we care about being the same being different.

      In some ways it's less of an issue for json because it isn't directly user facing.

      That said, I don't feel really strongly about it.

    4. OK so if we don't have strong opinion then I think I will keep the non-ordering of text output, since it aligns with most other console tasks (like "list"), and users can easily use "sort" shell command for strict ordering.

      As for json, I will still keep it sorted, because in our unit tests and integration tests, ordering matters for json, but not for text output (we use "set" with text output, while for json, we have to make sure outputs are exact match). Enforcing ordering on json output can avoid the trouble of modifying test cases when running on different engines.

  5. 
      
YU
YU
YU
Review request changed

Status: Closed (submitted)

Change Summary:

in https://github.com/pantsbuild/pants/commit/dcdb55f96217e4f888460c384d6e5a3bf8ecbf29.
Thanks Kris, Ity and Nick!

Loading...