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

Remove Address/BuildFileAddress ambiguity and fix list-owners #4399

Merged
merged 4 commits into from Mar 29, 2017

Conversation

Projects
None yet
3 participants
@stuhood
Member

stuhood commented Mar 29, 2017

Problem

Because many rules accept "Exactly(Address, BuildFileAddress)", they will run when the subject is either an Address or a BuildFileAddress: the result being that they might run multiple times for the same spec path due to multiple instances of the rule running for different subjects.

In a related vein, list-owners was broken for particular usecases due to looking for a field that will soon be deprecated.

Solution

Remove the type union, to explicitly require one or the other type for each rule, and convert from one type to the other where necessary (apparently only in transitive_hydrated_targets).

Additionally, fix list-owners by using the rel_path property of the BuildFileAddress, rather than the build_file, which is only set in the v1 engine.

Result

Fixes #4376 and #3925.

@stuhood stuhood requested review from wisechengyi, ity and peiyuwang Mar 29, 2017

@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

stuhood Mar 29, 2017

Member

Will add a test tonight.

Member

stuhood commented Mar 29, 2017

Will add a test tonight.

@kwlzn

kwlzn approved these changes Mar 29, 2017

lgtm! thanks!

@wisechengyi

Thanks Stu! Just a nit

# see: https://github.com/pantsbuild/pants/issues/4401
def get_target_set(self, std_out):
return sorted([l for l in std_out.split('\n') if l])

This comment has been minimized.

@wisechengyi

wisechengyi Mar 29, 2017

Contributor

nit: sorted(l for l in std_out.split('\n') if l) should do

@wisechengyi

wisechengyi Mar 29, 2017

Contributor

nit: sorted(l for l in std_out.split('\n') if l) should do

This comment has been minimized.

@stuhood

stuhood Mar 29, 2017

Member

If CI wasn't green I would fix this... but since it is I'll wait! Sorry.

@stuhood

stuhood Mar 29, 2017

Member

If CI wasn't green I would fix this... but since it is I'll wait! Sorry.

@stuhood stuhood merged commit 1dd699c into pantsbuild:master Mar 29, 2017

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/remove-address-ambiguity branch Mar 29, 2017

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Remove Address/BuildFileAddress ambiguity and fix list-owners (#4399)
### Problem

Because many rules accept "Exactly(Address, BuildFileAddress)", they will run when the subject is _either_ an `Address` or a `BuildFileAddress`: the result being that they might run multiple times for the same spec path due to multiple instances of the rule running for different subjects.

In a related vein, `list-owners` was broken for particular usecases due to looking for a field that will soon be deprecated.

### Solution

Remove the type union, to explicitly require one or the other type for each rule, and convert from one type to the other where necessary (apparently only in `transitive_hydrated_targets`).

Additionally, fix `list-owners` by using the `rel_path` property of the `BuildFileAddress`, rather than the `build_file`, which is only set in the v1 engine.

### Result

Fixes #4376 and #3925.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment