Support exclude_target_regexps and ignore_patterns in v2 engine

Review Request #4172 — Created Aug. 19, 2016 and submitted

3671, 3780, 3801
kwlzn, mateor, patricklaw, stuhood

This change does the followings:
1. Add support for exclude_target_regexps and ignore_patterns options in v2 engine.
2. Deprecate --ignore-patterns in version 1.3.0, replace with --build-ignore. If both are given, pants will explode.
3. Mark tests that failed without 1 in v2 engine with "ensure_engine" decorator.
4. Move from tests/python/pants_test/base to tests/python/pants_test/engine/legacy and rename to
5. Add test cases for 1.

for tests/python/pants_test/projects/, I split the original tests into 2 tests, one using v1 engine, the other using v2 engine. The reason I don't use @ensure_engine here is that, in travis-ci, it takes more than 10 minutes for that test to finish and travis-ci will error out if there is no output for more than 10 minutes. The alternative is to use travis_wait, but that causes another failure in ci, which I have no idea why it happens, and I have opened an issue in travis github. So for now, spliting the test is the best solution we have.

ci green:

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
  1. lgtm! handful of minor comments.

  2. src/python/pants/bin/ (Diff revision 2)

    nit: should avoid escaped newlines wherever possible.

    generally best to linebreak within parens which provides implicit line continuations, e.g.:

    graph_helper = graph_helper or EngineInitializer.setup_legacy_graph(
  3. src/python/pants/engine/ (Diff revision 2)

    this could avoid the len(list()) by just counting items in the iterable:

    ignored = sum(1 for _ in address_mapper.build_ignore_patterns.match_files([stat.path])) > 0
    1. look neat! thanks!

  4. src/python/pants/engine/ (Diff revision 2)

    entering micro-optimization territory here, but would it make sense to check whether the path is ignored earlier in the boolean expression before bothering to type check and fnmatch?


    return (not ignored) and type(stat) is File and fnmatch(...)
    1. yeah, that should be better.

  5. what's the motivation for moving this under engine/legacy? isn't it still relevant to base?

    1. in v2 engine, build_ignore logic is implemented in engine/, and this test is to make sure the legacy functionality works in v2 engine as well, thus I moved it under engine/legacy.

  6. tests/python/pants_test/engine/legacy/ (Diff revision 2)

    any reason to not break this into two separate tests?

    1. these 2 are closely related. I want to show that if BUILD of a dependency is ignored, then the error will be thrown. If that BUILD is not ignored, everything works fine.

  7. denpendency -> dependency

  1. lgtm - handful of comments/thoughts.

    this RB could probably benefit from a second pair of eyes too.

  2. src/python/pants/bin/ (Diff revision 4)

    should these two params actually be required vs defaulting to None?

    seems like it'd be problematic to not pass them given there's no defaulting case like there is for symbol_table_cls.

    1. I think they can be default to None. These 2 are passed to AddressMapper's __init__ method, and it can handle None case.

  3. s/exlude/exclude/ here and in callers.

  4. should this be pants vs paths? if not, maybe drop the pluralization (s/paths ignore patterns/path ignore patterns/).

    also, should this also get renamed to pants_ignore_patterns like was done elsewhere?

    1. Good catch! I should rename them. I think the description should be "A list of path ignore patterns...", which is clearer. Will drop the plualization.

  5. src/python/pants/bin/ (Diff revision 4)

    this is super awkward. why not just define a MutuallyExclusiveOptionError Exception subclass and raise that directly?

  6. src/python/pants/bin/ (Diff revision 4)

    you can avoid escaping the single quote here by using double quotes:



  7. src/python/pants/bin/ (Diff revision 4)

    is the RankedValue strictly necessary here? why do we care where the option came from?

    1. It is necessary not because we care about the rank, but because self._global_options is of type OptionValueContainer, and its setter method only takes RankedValue object.

  8. src/python/pants/option/ (Diff revision 4)

    rather than stating these as "Pants will ...", how about stating them more naturally/simply along the lines of:

    "Paths to ignore when identifying BUILD files. Patterns use the gitignore pattern syntax (...)."

    "Paths to ignore for all filesystem operations performed by pants (e.g. BUILD file scanning, glob matching, etc). Patterns use the gitignore pattern syntax (...). This currently only affects the v2 engine."

    care should also be taken to insert spaces between concatenated strings (should be able to preview this with ./pants help-advanced ...).

    1. Yeah this is much better. Thanks for the help with my writing!

  9. tests/python/pants_test/projects/ (Diff revision 4)

    strictly speaking, the way test_testprojects_v1_engine works is the default case.

    so, to achieve the same result you could leave tests_testprojects alone and let pytest run that as-is and then just define the test_testprojects_v2_engine method here to run that with the modified environment.

  1. Thanks Yujie!

  2. src/python/pants/engine/ (Diff revision 5)

    might also be able to do:

    any(true for _ in address_mapper.build_ignore_patterns.match_files([stat.path]))
  3. Should make sure Patrick or Mateo review this, as I think Foursquare are the only folks using the regex ignore.

  1. This doesn't appear to break our usage. This is a bit of a rubber stamp. I personally found it a bit of a challenge to differentiate between the various flags that are being threaded through. But I understand this is just an intermediate state and I like where it is going, so works for me.

  2. src/python/pants/engine/ (Diff revision 6)

    Could you pydoc this?

    I don't think someone constructing an AddressMapper will be able to understand the distinctions between this args.

    This class isn't marked API Public - but it probably should be, since anyone injecting a synthetic target has a use case for it.

    1. Thanks, Mateo. I have improved the doc string.

      As for API public, I checked it seems nothing in the v2 engine is marked API public yet. I think probably we want to wait till we are ready to ship v2 engine to do that.

Review request changed

Status: Closed (submitted)

Change Summary:

63ea982f2bb397e407c8f8ebd75876eedf5ed79e Thanks Kris, Stu, Yi and Mateo!