Make "./pants changed" output correct results when BUILD files are modified #4282

Merged
merged 4 commits into from Feb 24, 2017

Conversation

Projects
None yet
4 participants
@JieGhost
Contributor

JieGhost commented Feb 24, 2017

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 257a622,
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.

@JieGhost JieGhost requested review from baroquebobcat, stuhood, ity and kwlzn Feb 24, 2017

@kwlzn

kwlzn approved these changes Feb 24, 2017

lgtm!

@@ -1 +1 @@
-e5b11346a458240934cc6136e40faa62b75f79ff
+da39a3ee5e6b4b0d3255bfef95601890afd80709

This comment has been minimized.

@kwlzn

kwlzn Feb 24, 2017

Member

might be wrong, but I'd only expect this to get modified with rust code changes?

@kwlzn

kwlzn Feb 24, 2017

Member

might be wrong, but I'd only expect this to get modified with rust code changes?

This comment has been minimized.

@JieGhost

JieGhost Feb 24, 2017

Contributor

Yeah, I saw a lot of errors in CI. I believe they are due to this. I don't know why this got changed, but I think I should change it back, right?

@JieGhost

JieGhost Feb 24, 2017

Contributor

Yeah, I saw a lot of errors in CI. I believe they are due to this. I don't know why this got changed, but I think I should change it back, right?

This comment has been minimized.

@JieGhost

JieGhost Feb 24, 2017

Contributor

After I nuked ~/.cache/bin/native_engine and run a v2 engine task, the version is correct now.

@JieGhost

JieGhost Feb 24, 2017

Contributor

After I nuked ~/.cache/bin/native_engine and run a v2 engine task, the version is correct now.

@@ -56,8 +56,14 @@ def is_declaring_file(address, file_path):
# 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)):

This comment has been minimized.

@wisechengyi

wisechengyi Feb 24, 2017

Contributor

The comments above no longer applies, right?

@wisechengyi

wisechengyi Feb 24, 2017

Contributor

The comments above no longer applies, right?

This comment has been minimized.

@JieGhost

JieGhost Feb 24, 2017

Contributor

part of it still applies to "except" block, I will move it there and rephrase it.

@JieGhost

JieGhost Feb 24, 2017

Contributor

part of it still applies to "except" block, I will move it there and rephrase it.

@baroquebobcat

lgtm too!

@JieGhost JieGhost merged commit f840933 into pantsbuild:master Feb 24, 2017

1 check passed

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

@JieGhost JieGhost deleted the JieGhost:yujieproject/changed_investigation branch Feb 24, 2017

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

Make "./pants changed" output correct results when BUILD files are mo…
…dified (#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 pantsbuild@257a622,
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment