Copyright check captures changes on target branch

[vyasr]

#10253 added a check to make sure that when files are modified the copyrights are updated to include the current year (and are more generally formatted correctly). As a current example of this, we see the copyright check failing on #10319. The specific failure that we observe there is with respect to this file, which was modified in #10313. What appears to be happening is that the copyright is diffing our main branch (currently branch-22.04) directly against the source branch for the PR, rather than diffing the source branch against the merge base (git merge-base source target). As a result, changes that are merged into our main branch can cause failures of the copyright check on other branches until they merge the target branch back down.

The short term solution is for PR authors to simply merge the main branch back down into their branch, which should fix the failure. However, to avoid wasting unnecessary cycles on this we should address the underlying issue. The fix should be changing the changedFilesBetween function in gitutils to use the common merge base to find the changed files used in the copyright check rather than using the target branch directly. I am not 100% sure what impact this will have in the event that there are merge conflicts, but I don't think we need to worry about this since in those cases PR authors need to merge in the target branch to fix conflicts anyway.

[vyasr]

CC @shwina @bdice @galipremsagar

[github-actions]

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

[github-actions]

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

[vyasr]

As part of the long term fix here, we should probably also switch to using GitPython rather than our own homegrown git utils. In addition to reducing our maintenance burden, that switch should help us avoid issues like the one currently preventing our usage of the copyright check in pre-commit during CI.

[bdice]

@vyasr I'm not sure if we still need gitutils at all. If we refactored ci/checks/copyright.py to run as a pre-commit hook that accepts a list of changed file names (as determined by pre-commit) then we should be able to remove gitutils and a good portion of the logic in ci/checks/copyright.py dealing with which files to check.

The CLI would look like python ci/checks/copyright.py FILE1 FILE2 FILE3 ..., and pre-commit would supply the filenames.

I'm going to give this a try.

[vyasr]

If the copyright check only checks that the current year is included, but doesn't do any check of the start date (e.g. in a range like 2017-2022 will it check that 2017 is the right start date or no?) then I think you're probably right. It's been a while since I looked into this issue, so I don't remember if that's the case, but if so then it would be great to be able to remove it altogether.

[bdice]

I adapted my proposal and it is available for review in #11711.

It doesn't seem that the copyright check verified the start year (e.g. 2017) before or after my changes. It only verifies two error cases: start > end and thisYear < start or thisYear > end:

cudf/ci/checks/copyright.py

Line 133 in cfa0187

if start > end:

cudf/ci/checks/copyright.py

Line 142 in cfa0187

elif thisYear < start or thisYear > end:

[vyasr]

OK yeah that's what I suspected since some files were created in other repos and then migrated here, so the start date check via git may not even be valid.