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 requirement for root build file in `changed` #5134

Merged
merged 2 commits into from Nov 27, 2017

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

stuhood commented Nov 26, 2017

Problem

The changed options as implemented in the v2 engine use the AscendantAddresses spec type to find "all addresses above a directory". As described in #4833 and #4904, this currently introduces a requirement that at least one BUILD file exists between a changed file and the root of the repo.

Solution

Remove the requirement for a non-empty set of AddressFamily instances when expanding AscendantAddresses, and add a covering integration test.

Result

AscendantAddress instances that don't encounter BUILD files will not cause errors.

Fixes #4833 and #4904.

@stuhood stuhood added this to the 1.3.1 milestone Nov 26, 2017

@stuhood stuhood requested a review from kwlzn Nov 26, 2017

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks reasonable to me, but I don't have much context

@kwlzn

kwlzn approved these changes Nov 27, 2017

Copy link
Member

kwlzn left a comment

lgtm

write_path = os.path.join(worktree, path)
with safe_open(write_path, 'w') as f:
f.write(dedent(content))
return write_path

This comment has been minimized.

@kwlzn

kwlzn Nov 27, 2017

Member

could probably re-use BaseTest.{create_file,create_workdir_file} here instead?

This comment has been minimized.

@stuhood

stuhood Nov 27, 2017

Member

The workdir in that case is different from the one in use here, unfortunately.

@stuhood stuhood merged commit fb9224b into pantsbuild:master Nov 27, 2017

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/remove-requirement-for-root-BUILD-file branch Nov 27, 2017

stuhood added a commit that referenced this pull request Nov 27, 2017

Remove requirement for root build file in `changed` (#5134)
The `changed` options as implemented in the v2 engine use the `AscendantAddresses` spec type to find "all addresses above a directory". As described in #4833 and #4904, this currently introduces a requirement that at least one `BUILD` file exists between a changed file and the root of the repo.

Remove the requirement for a non-empty set of `AddressFamily` instances when expanding `AscendantAddresses`, and add a covering integration test.

`AscendantAddress` instances that don't encounter `BUILD` files will not cause errors.

Fixes #4833 and #4904.

stuhood added a commit to twitter/pants that referenced this pull request Dec 15, 2017

Remove requirement for root build file in `changed` (pantsbuild#5134)
### Problem

The `changed` options as implemented in the v2 engine use the `AscendantAddresses` spec type to find "all addresses above a directory". As described in pantsbuild#4833 and pantsbuild#4904, this currently introduces a requirement that at least one `BUILD` file exists between a changed file and the root of the repo.

### Solution

Remove the requirement for a non-empty set of `AddressFamily` instances when expanding `AscendantAddresses`, and add a covering integration test.

### Result

`AscendantAddress` instances that don't encounter `BUILD` files will not cause errors.

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