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

Fix Single Address Exclude #6366

Merged
merged 2 commits into from Aug 20, 2018

Conversation

Projects
None yet
5 participants
@CMLivingston
Copy link
Contributor

CMLivingston commented Aug 17, 2018

Problem

The git commit hook (and CI lint) will lint targets that own changed files. Transforming specs to single addresses is currently broken and causes a "did you mean error" to be thrown on specs that actually exist in an affected build file:

ResolveError: "main_py2" was not found in namespace "testprojects/src/python/interpreter_selection/python_3_selection_testing". Did you mean one of:
          :lib_py2
          :lib_py23
          :lib_py3
          :main_py2
          :main_py23
          :main_py3
          :test_py2
          :test_py23

This manifested after trying to make changes to a testprojects BUILD file, which is excluded in the CI/commit hook lint step:
./pants -q --exclude-target-regexp='testprojects/.*' --changed-parent=master lint

Solution

Edit build_files.py to aggregate excluded addresses in addition to included ones and check that excluded addresses align with exclude logic so that the "did you mean" error only occurs when the spec actually doesn't exist.

Result

Changes to files in testprojects will not throw any "did you mean" errors in CI/commit hook lint invoked by the following command:
./pants -q --exclude-target-regexp='testprojects/.*' --changed-parent=master lint

Chris Livingston

@stuhood stuhood requested review from ity and stuhood Aug 17, 2018

@@ -32,8 +34,8 @@ def test_empty(self):
"""Test that parsing an empty BUILD file results in an empty AddressFamily."""
address_mapper = AddressMapper(JsonParser(TestTable()))
af = run_rule(parse_address_family, address_mapper, Dir('/dev/null'), {
(Snapshot, PathGlobs): lambda _: Snapshot(DirectoryDigest('abc', 10), (File('/dev/null/BUILD'),)),
(FilesContent, DirectoryDigest): lambda _: FilesContent([FileContent('/dev/null/BUILD', b'')]),
(Snapshot, PathGlobs): lambda _: Snapshot(DirectoryDigest(text_type("abc"), 10), (File('/dev/null/BUILD'),)),

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 17, 2018

Contributor

I don't think text_type should be necessary here, because the string literal is already unicode in PY2 and str in Py3

(Snapshot, PathGlobs): lambda _: Snapshot(DirectoryDigest('abc', 10), (File('/dev/null/BUILD'),)),
(FilesContent, DirectoryDigest): lambda _: FilesContent([FileContent('/dev/null/BUILD', b'')]),
(Snapshot, PathGlobs): lambda _: Snapshot(DirectoryDigest(text_type("abc"), 10), (File('/dev/null/BUILD'),)),
(FilesContent, DirectoryDigest): lambda _: FilesContent([FileContent('/dev/null/BUILD', '')]),

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 17, 2018

Contributor

FileContent must be binary, did you mean to revert this?

"""Test that targets are filtered based on exclude patterns."""
spec = SingleAddress('root', 'not_me')
address_mapper = AddressMapper(JsonParser(TestTable()))
snapshot = Snapshot(DirectoryDigest(text_type('xx'), 2), (Path('root/BUILD', File('root/BUILD')),))

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 17, 2018

Contributor

Ditto on not needing text_type

@stuhood
Copy link
Member

stuhood left a comment

Looks great, thanks!

@@ -32,8 +34,8 @@ def test_empty(self):
"""Test that parsing an empty BUILD file results in an empty AddressFamily."""
address_mapper = AddressMapper(JsonParser(TestTable()))
af = run_rule(parse_address_family, address_mapper, Dir('/dev/null'), {
(Snapshot, PathGlobs): lambda _: Snapshot(DirectoryDigest('abc', 10), (File('/dev/null/BUILD'),)),
(FilesContent, DirectoryDigest): lambda _: FilesContent([FileContent('/dev/null/BUILD', b'')]),
(Snapshot, PathGlobs): lambda _: Snapshot(DirectoryDigest(text_type("abc"), 10), (File('/dev/null/BUILD'),)),

This comment has been minimized.

@stuhood

stuhood Aug 17, 2018

Member

Were these tests failing without these edits? Does this need a rebase to master?

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 17, 2018

Contributor

Py2 wasn’t failing, I don’t know about Py3. I had cleaned up the file because I realized we had more code than necessary.

Ah that makes sense it might simply need to merge master. I wasn’t sure if it was intentional to revert the changes I had made.

@@ -96,6 +95,23 @@ def test_exclude_pattern(self):
self.assertEqual(len(targets.dependencies), 1)
self.assertEqual(targets.dependencies[0].spec, 'root:not_me')

def test_exclude_pattern_with_single_address(self):
"""Test that targets are filtered based on exclude patterns."""

This comment has been minimized.

@stuhood

stuhood Aug 17, 2018

Member

Would be good to update this comment.

@CMLivingston

This comment has been minimized.

Copy link
Contributor

CMLivingston commented Aug 18, 2018

Thanks for the reviews all. This is strange, I rebased master before pushing these changes, and I didn’t make any of the 2/3 comparability or formatting changes that appear here. Is there a futurize or autofmt script running as part of the git hook or something? Otherwise this was definitely an accident and Ill revert those.

Chris Livingston

@CMLivingston CMLivingston force-pushed the CMLivingston:clivingston/fix-single-address-exclude branch from a314f06 to c17e335 Aug 20, 2018

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Aug 20, 2018

No, we don't have any futurize script running and all of those changes were manually made by me at the beginning of this migration and then manually reverted by me about a week ago. I don't know how the old version slipped in.

Thanks for fixing!

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Aug 20, 2018

@CMLivingston you're all green for merge.

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@stuhood stuhood merged commit a03144b into pantsbuild:master Aug 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ity
Copy link
Member

ity left a comment

thanks for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment