[ament_copyright] Fix file exclusion behavior #327

aprotyas · 2021-10-17T08:02:49Z

This PR fixes the file exclusion behavior reported in
#326.

Specifically, the exclusion list is matched against traversed
files in the crawler module.

Changes inspired by #299.

Signed-off-by: Abrar Rahman Protyasha [email protected]

This commit fixes the faulty file exclusion behavior reported in ament#326. Specifically, the exclusion list is matched against traversed files in the `crawler` module. Changes inspired by ament#299. Signed-off-by: Abrar Rahman Protyasha <[email protected]>

Since file names are not indiscriminately matched throughout the search tree anymore, the excluded files listed in the copyright tests need to be updated relative to the root of the package. Signed-off-by: Abrar Rahman Protyasha <[email protected]>

aprotyas · 2021-10-17T09:09:37Z

Note that wildcard patterns are supported following this PR, because of the nature of glob.glob's pattern expansion. If this is not desirable, the expansion can be suppressed.

aprotyas · 2021-10-17T09:14:52Z

For reviewers: note that the Rpr job failed for the initial commit (f9f7e56) because copyright tests in some other lint packages could not exclude files like before - i.e. ament_copyright was not indiscriminately matching filenames to exclude from the search tree. This necessitated b10782d, in which the excluded files listed in the copyright tests were updated relative to the root of the package.

I recognize that this is not ideal, and maybe the desired behavior is for ament_copyright to be excluding the way it previously had been doing. However, for reasons listed in #326, I don't believe that is the "correct" way to be excluding files. Also, these changes should make file exclusion simpler for more sophisticated packages (namely tf2), where I hope to add common linters using their respective CMake functions.

aprotyas · 2021-10-17T09:22:13Z

CI with linter tests only (--ctest-args -L linter)

Linux
Linux-aarch64
macOS
Windows

audrow

Looks good with green CI.

It would also be nice to add a test for this.

aprotyas · 2021-10-19T22:22:10Z

It would also be nice to add a test for this.

Thanks for the review! I will do that shortly.

Specifically, these tests check for: - Incorrect exclusion of single filenames. - Correct exclusion of relatively/absolutely addressed filenames. - Correct exclusion of wildcarded paths. Signed-off-by: Abrar Rahman Protyasha <[email protected]>

These unit tests make sure both search and exclusion behaviors are correctly demonstrated by the `ament_copyright.crawler` module. Signed-off-by: Abrar Rahman Protyasha <[email protected]>

aprotyas · 2021-10-20T13:20:19Z

@audrow I've added test cases to test file exclusion in cbed0b9 and added unit tests for the crawler module - these tests for thoroughly testing the search/exclusion behavior - in 5a6177a.

I recognize that this PR is super long now! My bad. If you want, I can revert 5a6177a and submit that as a separate PR, as the script-level exclusion behavior ament_copyright should demonstrate is tested entirely in cbed0b9.

aprotyas · 2021-10-20T13:23:12Z

Running CI again with linter tests only (--ctest-args -L linter)

Linux
Linux-aarch64
macOS
Windows

aprotyas · 2021-10-20T19:03:46Z

For reviewers' benefit, here's a summary of the exclusion behavior after this PR.

.
├── a.cpp
├── b.cpp
└── dir
   ├── a.cpp
   └── b.cpp

ament_copyright --exclude=a.cpp will exclude ./a.cpp.
ament_copyright --exclude=* will exclude ./a.cpp and ./b.cpp.
ament_copyright --exclude=dir/a.cpp will exclude ./dir/a.cpp.
ament_copyright --exclude=dir/* will exclude ./dir/a.cpp and ./dir/b.cpp.
ament_copyright --exclude=. will not exclude anything.
ament_copyright --exclude=dir will not exclude anything.

Let me know if this is desired/good behavior for exclusion, particularly in regard to the last two points. I can easily see provision of a directory to mean "exclude all items in that directory (and all its subdirectories)". If so, I can amend that.

audrow

Looks good, thanks for adding tests. It would probably be good to run CI so that these new tests run, in addition to the linters.

aprotyas · 2021-10-21T22:41:58Z

Thanks for the review again @audrow. Yes, the last CI run did not invoke these tests, so let me run CI again.

CI (build/test: --packages-above ament_copyright)

Linux
Linux-aarch64
macOS
Windows

aprotyas · 2021-10-21T23:07:41Z

Not sure why colcon build choked on all platforms because things build fine locally. Trying CI again.

CI (build/test: --packages-above ament_copyright)

Linux
Linux-aarch64
macOS
Windows

Any clue what the problem is?

This PR fixes the file exclusion behavior reported in Specifically, the exclusion list is matched against files/directories as the search path is traversed. Tries to maintain consistency with ament#327. Signed-off-by: Abrar Rahman Protyasha <[email protected]>

This PR fixes the file exclusion behavior reported in ament#326. Specifically, the exclusion list is matched against files/directories as the search path is traversed. Tries to maintain consistency with ament#327. Signed-off-by: Abrar Rahman Protyasha <[email protected]>

audrow · 2021-10-27T21:02:05Z

It seems that CI was being built with --packages-above ament_copyright, when it should be --packages-above-and-dependencies ament_copyright, so it was building packages above ament_copyright without building any of ament_copyright's dependencies.

Here is another run of CI.

Linux
Linux-aarch64
macOS
Windows

aprotyas · 2021-10-28T01:01:14Z

Thank you for running CI correctly @audrow. It looks like there are some qt_dotgraph test failures - not sure what to make of these really. They also failed in a separate full CI run for a launch_ros PR earlier today: https://ci.ros2.org/job/ci_linux/15497/

audrow · 2021-10-29T23:04:45Z

I think because you've found the issue elsewhere, it is safe to merge. Here's a new round of CI anyway:

Linux
Linux-aarch64
macOS
Windows (re-ran, again)

audrow · 2021-11-03T17:16:29Z

@aprotyas, CI is green! Do you have merge access? Otherwise, I can merge this in.

aprotyas · 2021-11-03T21:32:49Z

@audrow Thanks for re-running CI (again)! I don't have merge access, so I'll ask you to merge this in.

This PR fixes the file exclusion behavior reported in ament#326. Specifically, the exclusion list is matched against files/directories as the search path is traversed. Tries to maintain consistency with ament#327. Signed-off-by: Abrar Rahman Protyasha <[email protected]>

* [ament_uncrustify] Fix file exclusion behavior This PR fixes the file exclusion behavior reported in #326. Specifically, the exclusion list is matched against files/directories as the search path is traversed. Tries to maintain consistency with #327. Signed-off-by: Abrar Rahman Protyasha <[email protected]> * [ament_uncrustify] Add file exclusion tests Signed-off-by: Abrar Rahman Protyasha <[email protected]> * [ament_uncrustify] Remove erroneous pytest marker Signed-off-by: Abrar Rahman Protyasha <[email protected]>

* [ament_uncrustify] Fix file exclusion behavior This PR fixes the file exclusion behavior reported in ament#326. Specifically, the exclusion list is matched against files/directories as the search path is traversed. Tries to maintain consistency with ament#327. Signed-off-by: Abrar Rahman Protyasha <[email protected]> * [ament_uncrustify] Add file exclusion tests Signed-off-by: Abrar Rahman Protyasha <[email protected]> * [ament_uncrustify] Remove erroneous pytest marker Signed-off-by: Abrar Rahman Protyasha <[email protected]>

aprotyas added 2 commits October 17, 2021 04:58

aprotyas force-pushed the aprotyas/ament_copyright_fix_file_exclusion branch from f9f7e56 to b10782d Compare October 17, 2021 09:01

This was referenced Oct 17, 2021

tf2: Enable common linter tests ros2/geometry2#469

Merged

[ament_copyright, ament_uncrustify] Unexpected file exclusion behavior #326

Closed

audrow approved these changes Oct 19, 2021

View reviewed changes

aprotyas added 2 commits October 20, 2021 09:10

Add unit tests for crawler module

5a6177a

These unit tests make sure both search and exclusion behaviors are correctly demonstrated by the `ament_copyright.crawler` module. Signed-off-by: Abrar Rahman Protyasha <[email protected]>

audrow approved these changes Oct 21, 2021

View reviewed changes

aprotyas mentioned this pull request Oct 25, 2021

[ament_uncrustify] Fix file exclusion behavior #334

Merged

audrow merged commit 6f7165e into ament:master Nov 4, 2021

aprotyas deleted the aprotyas/ament_copyright_fix_file_exclusion branch November 30, 2021 22:56

orensbruli mentioned this pull request Sep 9, 2022

[ament_uncrustify] Fix file exclusion behavior (#334) #409

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ament_copyright] Fix file exclusion behavior #327

[ament_copyright] Fix file exclusion behavior #327

aprotyas commented Oct 17, 2021 •

edited

Loading

aprotyas commented Oct 17, 2021

aprotyas commented Oct 17, 2021 •

edited

Loading

aprotyas commented Oct 17, 2021

audrow left a comment •

edited

Loading

aprotyas commented Oct 19, 2021

aprotyas commented Oct 20, 2021

aprotyas commented Oct 20, 2021

aprotyas commented Oct 20, 2021 •

edited

Loading

audrow left a comment •

edited

Loading

aprotyas commented Oct 21, 2021 •

edited

Loading

aprotyas commented Oct 21, 2021 •

edited

Loading

audrow commented Oct 27, 2021

aprotyas commented Oct 28, 2021 •

edited

Loading

audrow commented Oct 29, 2021 •

edited

Loading

audrow commented Nov 3, 2021

aprotyas commented Nov 3, 2021

[ament_copyright] Fix file exclusion behavior #327

[ament_copyright] Fix file exclusion behavior #327

Conversation

aprotyas commented Oct 17, 2021 • edited Loading

aprotyas commented Oct 17, 2021

aprotyas commented Oct 17, 2021 • edited Loading

aprotyas commented Oct 17, 2021

audrow left a comment • edited Loading

Choose a reason for hiding this comment

aprotyas commented Oct 19, 2021

aprotyas commented Oct 20, 2021

aprotyas commented Oct 20, 2021

aprotyas commented Oct 20, 2021 • edited Loading

audrow left a comment • edited Loading

Choose a reason for hiding this comment

aprotyas commented Oct 21, 2021 • edited Loading

aprotyas commented Oct 21, 2021 • edited Loading

audrow commented Oct 27, 2021

aprotyas commented Oct 28, 2021 • edited Loading

audrow commented Oct 29, 2021 • edited Loading

audrow commented Nov 3, 2021

aprotyas commented Nov 3, 2021

aprotyas commented Oct 17, 2021 •

edited

Loading

aprotyas commented Oct 17, 2021 •

edited

Loading

audrow left a comment •

edited

Loading

aprotyas commented Oct 20, 2021 •

edited

Loading

audrow left a comment •

edited

Loading

aprotyas commented Oct 21, 2021 •

edited

Loading

aprotyas commented Oct 21, 2021 •

edited

Loading

aprotyas commented Oct 28, 2021 •

edited

Loading

audrow commented Oct 29, 2021 •

edited

Loading