New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude only roots for exclude-target-regexp in v2 #4578

Merged
merged 5 commits into from May 12, 2017

Conversation

Projects
None yet
4 participants
@stuhood
Copy link
Member

stuhood commented May 11, 2017

Problem

The v2 engine implemented the exclude-target-regexp option differently from the v1 engine, in that it completely excluded targets immediately after parsing. While that arguably aligned with the option's docstring, it did not align with the implementation in the v1 engine, which had the effect of only filtering target roots.

Real usecases in the wild (see twitter/commons#451) were depending on the behaviour of filtering only roots, and the result of filtering targets completely out of the graph when other things are depending on them is always some sort of error.

Solution

Align the behaviour of v2 with v1 by only filtering root targets, clarify the doc string, and add a test that excluding a dependency still allows the dependee to be built.

Result

The behaviour of the v1 engine for this case is preserved in v2. Users who want to completely ignore a BUILD file can use the more granular --build-ignore option to do so. Fixes #4573

@stuhood stuhood added this to the 1.3.0 milestone May 11, 2017

@stuhood stuhood requested review from mateor , kwlzn and JieGhost May 12, 2017

@stuhood stuhood requested a review from gmalmquist May 12, 2017

@JieGhost
Copy link
Contributor

JieGhost left a comment

LGTM!

Select(_SPECS_CONSTRAINT)])
def addresses_from_address_families(address_families, spec):
def addresses_from_address_families(address_mapper, address_families, spec):
"""Given a list of AddressFamilies and a Spec, return matching Addresses.

This comment has been minimized.

@JieGhost

JieGhost May 12, 2017

Contributor

Maybe update the doc string a little?

@@ -243,13 +243,23 @@ def addresses_from_address_families(address_families, spec):
if not address_families:
raise ResolveError('Path "{}" contains no BUILD files.'.format(spec.directory))

def exclude_address(address):

This comment has been minimized.

@baroquebobcat

baroquebobcat May 12, 2017

Contributor

I feel like the exclusions should be done in parse_address_family rather than here. It already has the address_mapper, and addresses_from_address_families is not the only place that plucks addresses from address families. There's also resolve_unhydrated_struct.

This comment has been minimized.

@stuhood

stuhood May 12, 2017

Member

@baroquebobcat : That filters at the wrong level (it's how this was implemented before): we don't want to prevent the target from being loaded at all, we just want to filter it out of roots (Specs represent roots): see the description.

This comment has been minimized.

@baroquebobcat

baroquebobcat May 12, 2017

Contributor

Got it.

@mateor

mateor approved these changes May 12, 2017

@stuhood stuhood merged commit df19f57 into pantsbuild:master May 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/exclude-only-roots branch May 12, 2017

stuhood added a commit that referenced this pull request May 13, 2017

Exclude only roots for exclude-target-regexp in v2 (#4578)
### Problem

The `v2` engine implemented the `exclude-target-regexp` option differently from the `v1` engine, in that it completely excluded targets immediately after parsing. While that arguably aligned with the option's docstring, it did not align with the implementation in the `v1` engine, which had the effect of only filtering target roots.

Real usecases in the wild (see twitter/commons#451) were depending on the behaviour of filtering only roots, and the result of filtering targets completely out of the graph when other things are depending on them is always some sort of error.

### Solution

Align the behaviour of `v2` with `v1` by only filtering root targets, clarify the doc string, and add a test that excluding a dependency still allows the dependee to be built.

### Result

The behaviour of the `v1` engine for this case is preserved in `v2`. Users who want to completely ignore a BUILD file can use the more granular `--build-ignore` option to do so. Fixes #4573
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment