test-changed doesn't appear to respect exclude_target_regexp

[mateor]

I am testing 1.2.0rc1 in our repo and passing exclude_target_regexp to test-changed doesn't appear to be working.

I was able to hack it working with:

--- a/src/python/pants/task/changed_target_task.py
+++ b/src/python/pants/task/changed_target_task.py
@@ -49,7 +49,8 @@ class ChangedTargetTask(NoopExecTask):
   def alternate_target_roots(cls, options, address_mapper, build_graph):
     changed = Changed.Factory.global_instance().create(options)
     change_calculator = changed.change_calculator(build_graph=build_graph,
-                                                  address_mapper=address_mapper)
+                                                  address_mapper=address_mapper,
+                                                  exclude_target_regexp=options.exclude_target_regexp)
     changed_addresses = change_calculator.changed_target_addresses()
     readable = ''.join(sorted('\n\t* {}'.format(addr.reference()) for addr in changed_addresses))
     logger.info('Operating on changed {} target(s): {}'.format(len(changed_addresses), readable))

The rest of the options look to be gathered from the ChangedRequest.from_options(chained_options), it is possible the "correct" thing is to change the excludes to be also attached the ChangedRequest.

Here is a repro from Pants 1.2.0rc1 checkout:

mateo:pants mateo$ git st
HEAD detached at release_1.2.0rc1
    modified:   src/python/pants/core_tasks/what_changed.py

Before the above patch:

mateo:pants mateo$ ./pants test-changed
04:55:20 00:00     [parse]INFO] Operating on changed 1 target(s):
    * src/python/pants/core_tasks

mateo:pants mateo$ ./pants test-changed --test-changed-exclude-target-regexp="['src']"
04:55:35 00:00     [parse]INFO] Operating on changed 1 target(s):
    * src/python/pants/core_tasks

After the above patch was applied:

mateo:pants mateo$ ./pants test-changed
04:57:47 00:00     [parse]INFO] Operating on changed 1 target(s):
    * src/python/pants/core_tasks

mateo:pants mateo$ ./pants test-changed --test-changed-exclude-target-regexp="['src']"
04:58:46 00:00     [parse]INFO] Operating on changed 0 target(s):

[mateor]

Unit tests pass with the above. I don't think there is any coverage for the feature.

[JieGhost]

I think first exclude-target-regexp is a global option, so put it under scope test-changed may cause the normal handling get skipped.

And for the issue you mentioned, your hack definitely works, and I think in general it is how we handle exclude-target-regexp currently in changed related tasks, ie, pass this flag to change calculator and use it as a filter right before returning the results. https://github.com/pantsbuild/pants/blob/master/src/python/pants/scm/change_calculator.py#L109-L113

But one thing I noticed when browsing the code is that, changed calculator relies on source mapper which relies on address_mapper for its work. The exclude-target-regexp option is supposed to be used inside address_mapper. Clearly v1 address_mapper does not have correct implementation of this exclude logic. Only scan_specs() uses this flag to filter out excluded targets. V2 address_mapper should not have this problem, as the exclude logic is inside v2 engine. The v2 address_mapper is just a facade. You can verify if running test-changed with --enable-v2-engine gives you correct output.

My opinion is we don't need this flag in change calculator at all. We should fix v1 address_mapper to respect this flag in all its methods, and we will get correct results.

[mateor]

Your comment sounds reasonable to me - I didn't look at this for much more than a few moments in order to A) repro and b) workaround.

However, in the Pants repro posted above, passing --enable-v2-engine test-changed does not fix the behavior when the hack is not present.

Will follow up by emailing to the RC thread.

[JieGhost]

@mateor I modified a file in core_tasks/, and tried this command:

./pants --enable-v2-engine --exclude-target-regexp="['src']" test-changed

and it showed

14:04:49 00:00   [setup]
14:04:49 00:00     [parse]INFO] Operating on changed 0 target(s): 

Without --enable-v2-engine, this is what I get:

14:06:33 00:00   [setup]
14:06:33 00:00     [parse]INFO] Operating on changed 1 target(s): 
        * src/python/pants/core_tasks

[JieGhost]

merged.
Subsequent change on BuildFileAddressMapper will be addressed in #3983