From f840933dabd3c9d475f5455677f50b81d04bbacc Mon Sep 17 00:00:00 2001 From: JieGhost Date: Fri, 24 Feb 2017 14:19:08 -0500 Subject: [PATCH] Make "./pants changed" output correct results when BUILD files are modified (#4282) ### Problem #3933 Currently with v2 engine, if a dir has more than 1 BUILD file, changing only 1 of them will cause "./pants changed" output all targets in that dir, even though some targets are not defined in the changed BUILD file. The reason is here: https://github.com/pantsbuild/pants/blob/master/src/python/pants/engine/legacy/address_mapper.py#L53. In is_declaring_file method, since Address object does not have information about declaring BUILD file, the result is not accurate. ### Solution With https://github.com/pantsbuild/pants/commit/257a6223827d917107392092a78e06c46816c105, Now addresses passed to that method are BuildFileAddress objects, whose "rel_path" field is the path to its declaring BUILD file. I still kept the old logic in case an Address object is passed to this method. After #3925 is fixed, BuildFileAddress and Address will become 1 class, thus we can eliminate the extra logic. --- .../pants/engine/legacy/address_mapper.py | 20 ++++++++++++------- .../engine/legacy/test_address_mapper.py | 8 +++++++- .../engine/legacy/test_changed_integration.py | 11 ++++++++++ 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/src/python/pants/engine/legacy/address_mapper.py b/src/python/pants/engine/legacy/address_mapper.py index 24f3b6e55d1..37b1919ed6e 100644 --- a/src/python/pants/engine/legacy/address_mapper.py +++ b/src/python/pants/engine/legacy/address_mapper.py @@ -51,13 +51,19 @@ def scan_build_files(self, base_path): @staticmethod def is_declaring_file(address, file_path): - # NB: this will cause any BUILD file, whether it contains the address declaration or not to be - # considered the one that declared it. That's ok though, because the spec path should be enough - # information for debugging most of the time. - # - # We could call into the engine to ask for the file that declared the address. - return (os.path.dirname(file_path) == address.spec_path and - BuildFile._is_buildfile_name(os.path.basename(file_path))) + if not BuildFile._is_buildfile_name(os.path.basename(file_path)): + return False + + try: + # A precise check for BuildFileAddress + return address.rel_path == file_path + except AttributeError: + # NB: this will cause any BUILD file, whether it contains the address declaration or not to be + # considered the one that declared it. That's ok though, because the spec path should be enough + # information for debugging most of the time. + # + # TODO: remove this after https://github.com/pantsbuild/pants/issues/3925 lands + return os.path.dirname(file_path) == address.spec_path def addresses_in_spec_path(self, spec_path): return self.scan_specs([SiblingAddresses(spec_path)]) diff --git a/tests/python/pants_test/engine/legacy/test_address_mapper.py b/tests/python/pants_test/engine/legacy/test_address_mapper.py index c5b998d81d9..d04ee84e02e 100644 --- a/tests/python/pants_test/engine/legacy/test_address_mapper.py +++ b/tests/python/pants_test/engine/legacy/test_address_mapper.py @@ -12,7 +12,7 @@ from pants.base.specs import SiblingAddresses, SingleAddress from pants.bin.engine_initializer import EngineInitializer -from pants.build_graph.address import Address +from pants.build_graph.address import Address, BuildFileAddress from pants.build_graph.address_mapper import AddressMapper from pants.engine.legacy.address_mapper import LegacyAddressMapper from pants.util.contextutil import temporary_dir @@ -102,6 +102,12 @@ def test_is_declaring_file(self): self.assertTrue(mapper.is_declaring_file(Address('path', 'name'), 'path/BUILD.suffix')) self.assertFalse(mapper.is_declaring_file(Address('path', 'name'), 'path/not_a_build_file')) self.assertFalse(mapper.is_declaring_file(Address('path', 'name'), 'differing-path/BUILD')) + self.assertFalse(mapper.is_declaring_file( + BuildFileAddress(target_name='name', rel_path='path/BUILD.new'), + 'path/BUILD')) + self.assertTrue(mapper.is_declaring_file( + BuildFileAddress(target_name='name', rel_path='path/BUILD'), + 'path/BUILD')) def test_addresses_in_spec_path(self): with temporary_dir() as build_root: diff --git a/tests/python/pants_test/engine/legacy/test_changed_integration.py b/tests/python/pants_test/engine/legacy/test_changed_integration.py index fa86928db87..57e87a33a21 100644 --- a/tests/python/pants_test/engine/legacy/test_changed_integration.py +++ b/tests/python/pants_test/engine/legacy/test_changed_integration.py @@ -370,6 +370,17 @@ def test_changed_diffspec_and_files(self): 'src/python/python_targets/test_unclaimed_src.py'} ) + @ensure_engine + def test_changed_with_multiple_build_files(self): + new_build_file = 'src/python/python_targets/BUILD.new' + + with create_isolated_git_repo() as worktree: + touch(os.path.join(worktree, new_build_file)) + pants_run = self.run_pants(['changed']) + + self.assert_success(pants_run) + self.assertEqual(pants_run.stdout_data.strip(), '') + # Following 4 tests do not run in isolated repo because they don't mutate working copy. def test_changed(self): self.assert_changed_new_equals_old([])