Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add copyright hook #2

Closed
vyasr opened this issue Mar 24, 2023 · 1 comment · Fixed by #9
Closed

Add copyright hook #2

vyasr opened this issue Mar 24, 2023 · 1 comment · Fixed by #9
Assignees

Comments

@vyasr
Copy link
Contributor

vyasr commented Mar 24, 2023

The first hook that we would like to support is for checking copyright headers to ensure that they are up to date. The copyright script that we would like to adapt is already present in a few repos such as cudf. The purpose of this issue is to capture discussions that we have had recently regarding issues with this script, discuss potential workarounds, and determine a path forward.

The current script tends to work well for our style checks in CI, but it occasionally runs into issues on local runs where it incorrectly identifies modified files, resulting in updated copyrights for unmodified files. Here are some of the solutions suggested so far:

  • I proposed a modification to the logic that eschews "upstream branch" detection altogether in favor of relying on pre-commit to tell us which files were modified and then use git to query first and last modified times. This approach is far simpler and more robust, but @bdice pointed out that it has a potential weakness due to our usage of squash merges rewriting history and therefore having a different final year after a squash merge than before when a PR opened in one year is merged in the following year. Specifically, the problematic case is the following situation:
    • A PR is created in December, with files foo.cpp and foo.hpp modified and given the copyright year X
    • In January, foo.cpp is modified, resulting in an updated copyright year X + 1
    • The PR is squash-merged, with foo.cpp having the copyright year X + 1 but foo.hpp only having year X. However, due to the squash, the last modified time of foo.hpp is actually year X + 1 according to the git history.
    • All subsequent runs of the copyright hook with --all-files result in errors on foo.hpp
    • Note that the same issue is also present with the current copyright hook logic, but only in the much more limited scenario where a PR is created in December and then merged in January with no additional modifications to files. The primary difference is that with the current approach modifying any file in the PR is sufficient to update the copyright for all of the files in the diff with the main branch, whereas with the above proposal only the subset of actually modified files will be updated.
  • Another proposed alternative was modifying the RAPIDS /merge GH command to account for this discrepancy by performing a squash and copyright update prior to doing the squash merge on GH, but we rejected this solution as requiring too much engineering effort.

Given the above considerations, our current best path forward may be simply using the existing script and trying to identify its weak points. @ajschmidt8 and I considered the possibility of including further git logic into the copyright script to verify whether the relevant git target branches are sufficiently up-to-date. If we went this route, my inclination would be to throw errors rather than do any implicit git actions in the hook, but we may at least be able to provide more robust error modes in this way and avoid incorrect modifications.

@KyleFromNVIDIA
Copy link
Contributor

For the initial copyright year, I'm thinking we should simply enforce a "no-later-than" policy to handle the squashing issue. It would enforce the initial year to be either the year it was first committed or the initial year that's already listed, whichever is earlier. We don't know if the actual copyright year is earlier than the first commit, but we certainly do know that it's not later.

KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 19, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 19, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 19, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 19, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 19, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 19, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 19, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 20, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 20, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 22, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 22, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 22, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 22, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 22, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 22, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 22, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 22, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 22, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 22, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 22, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 22, 2024
KyleFromNVIDIA added a commit to KyleFromNVIDIA/pre-commit-hooks that referenced this issue Jan 22, 2024
@KyleFromNVIDIA KyleFromNVIDIA self-assigned this Jan 24, 2024
KyleFromNVIDIA added a commit that referenced this issue Jan 29, 2024
* Add copyright check

Fixes: #2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants