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

Display notice when copyrighted file is copied or renamed #52

Merged

Conversation

KyleFromNVIDIA
Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA commented Aug 26, 2024

Also proposes requiring Python 3.10, since rapidsai/build-planning#88 is being rolled out.

Fixes #48

@KyleFromNVIDIA KyleFromNVIDIA requested a review from a team as a code owner August 26, 2024 19:41
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! But I don't think it's working as intended.

I pulled your branch to my checkout of pre-commit-hooks and installed it.

cd ~/repos/pre-commit-hooks
git remote add kyle git@github.com:KyleFromNVIDIA/pre-commit-hooks.git
git fetch kyle copyright-copy-rename-notice
git checkout copyright-copy-rename-notice
pip install --ignore-installed -e .

Then pulled the latest cudf and added this to its .pre-commit-config.yaml

  - repo: local
    hooks:
      - id: verify-copyright-local
        name: verify-copyright-local
        language: system
        entry: verify-copyright

Then ran the following from the root of the cudf repo (the same change that led to #48):

sed -i .bak 's/2023-2024/2024/g' ./python/libcudf/libcudf/_version.py
git add ./python
pre-commit run verify-copyright-local \
  --files ./python/libcudf/libcudf/_version.py

I don't see the new warning text about file changes. I see the exact same output I reported in #48.

verify-copyright-local...................................................Failed
- hook id: verify-copyright-local
- exit code: 1

In file python/libcudf/libcudf/_version.py:1:17:
 # Copyright (c) 2024, NVIDIA CORPORATION.
warning: copyright is not out of date and should not be updated

In file python/libcudf/libcudf/_version.py:1:3:
-# Copyright (c) 2024, NVIDIA CORPORATION.
+# Copyright (c) 2023-2024, NVIDIA CORPORATION.
note: suggested fix

Double-checked that my local hook was set up correctly by adding a raise RuntimeError in my checkout of the pre-commit-hooks code and repeating the steps above. I saw that error raised, so I'm fairly confident that my local testing setup here is using your branch.

Like I mentioned in #48 (comment), I think it'd be fine to just unconditionally include language in this particular warning saying that a copy/rename of a file can be one reason for this result, and that adding arbitrary content to the file is enough to get this hook to use only that file's own edit history for copyright years. That's an option I'd support if determining whether a particular change was a copy/rename and which source file the target file was copied/renamed from is too complex.

@KyleFromNVIDIA
Copy link
Contributor Author

Then pulled the latest cudf

This is the problem. You've done this after rapidsai/cudf#15483 was already merged, so the hook will no longer think _version.py was copied. Try resetting your branch-24.10 to rapidsai/cudf@91f304e and see if you get the same result.

@KyleFromNVIDIA
Copy link
Contributor Author

I see the problem. I just tried copying a random file and changing the copyright range. I put the message on "update copyright" but not on "don't update copyright". I'll fix it.

@KyleFromNVIDIA
Copy link
Contributor Author

Try it again. This time, do the following:

sed 's/2023-2024/2024/g' python/cudf/cudf/_version.py > python/cudf/cudf/_version2.py
git add python/cudf/cudf/_version2.py
pre-commit run -a verify-copyright

And you'll see the note about the file being copied.

@jameslamb jameslamb self-requested a review August 27, 2024 14:53
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok great, tested as you suggested and I do see the note now.

verify-copyright-local...................................................Failed
- hook id: verify-copyright-local
- exit code: 1

In file python/cudf/cudf/_version2.py:1:17:
 # Copyright (c) 2024, NVIDIA CORPORATION.
warning: copyright is not out of date and should not be updated

In file python/cudf/cudf/_version2.py:1:1:
 # Copyright (c) 2024, NVIDIA CORPORATION.
note: file was copied from '_version.py'

In file python/cudf/cudf/_version2.py:1:3:
-# Copyright (c) 2024, NVIDIA CORPORATION.
+# Copyright (c) 2023-2024, NVIDIA CORPORATION.
note: suggested fix

I do still think, as I suggested in #48 (comment), that it'd be helpful to explicitly say something like the following instead:

note: file was copied from '_version.py', and is assumed to share copyright history with it. Change the content of 'python/cudf/cudf/_version2.py' if you want its copyright dates to only be determined by its own edit history.

If I'd seen that when I initially encountered the behavior that led to #48, I would have immediately known why it was happening and how to resolve it.

But if you really feel strongly about not having that, I'm not going to block the PR until you agree, so marking this approved.

@KyleFromNVIDIA KyleFromNVIDIA merged commit 80e3e1b into rapidsai:main Aug 27, 2024
2 checks passed
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 this pull request may close these issues.

verify-copyright disallows an update that makes years match file history for a new file
3 participants