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

Fix copyright linter #4221

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix copyright linter #4221

wants to merge 2 commits into from

Conversation

ma-ilsi
Copy link

@ma-ilsi ma-ilsi commented Sep 24, 2023

Resolved issues:

Resolves #2298. Resolves #4078.

Description of changes:

Describe s2n’s current behavior and how your code changes that behavior. If there are no issues this PR is resolving, explain why this change is necessary.

Currently, the linter misses 200+ files that have copyright headers and are not being checked. This commit contains a complete linter that is:

  • Up to date with every single file that has a copyright header (without any excess nor decrease; an exact number), all added in easy to write regex patterns.
  • Able to unregister a file in case it gets grouped with a regex pattern accidentally and does not need to be checked. Unregistered file patterns will be ignored.
  • Supplied with a -scan flag that checks every file in the project that has not been registered or unregistered with the linter, in order to find new copyright headers. This solves the problem of the linter becoming outdated and needing a file list overhaul every year or so...
  • Using numbers to give quantitative results.

The effectiveness of the new copyright linter is easily seen as it detects 9 files that are missing copyright headers including Rust files that were expressed in #4078, .yml files which have siblings in a directory all of which are copyrighted yet these ones are not, and finds an empty python file. All the files reported seem concerning and should be looked in to.

s2n-tls-copyright-lint.mp4
Copyright Check Failed:
s2n-tls/bindings/rust/bench/benches/resumption.rs

Copyright Check Failed:
s2n-tls/bindings/rust/bench/src/bin/graph_memory.rs

Copyright Check Failed:
s2n-tls/codebuild/spec/buildspec_amazonlinux2.yml

Copyright Check Failed:
s2n-tls/codebuild/spec/buildspec_ktls.yml

Copyright Check Failed:
s2n-tls/codebuild/spec/buildspec_ubuntu_integv2criterion.yml

Copyright Check Failed:
s2n-tls/codebuild/spec/buildspec_ubuntu_integv2criterion_baseline.yml

Copyright Check Failed:
s2n-tls/tests/cbmc/proofs/lib/__init__.py

Copyright Check Failed:
s2n-tls/tests/saw/HMAC/spec/modthm.cry

Copyright Check Failed:
s2n-tls/tests/saw/verify_drbg.saw

1103/1112 Files Passing
FAILED Copyright Check

Feel free to try out the -scan feature by removing a registered pattern; the linter will complain about those files and report them to be added when run with the -scan flag. Currently no new files are found using -scan as the current registered files list is complete.

Call-outs:

Files copyrighted by Galois are also detected/complained/scanned. This is because the pattern (adopted from the previous linter) is to search for the single word Copyright in the first 3 lines.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

No tests have been added.

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dougch
Copy link
Contributor

dougch commented Sep 25, 2023

Thanks for the submission; I have concerns about the long list of regex's; let me review our requirements and dig into it.

@ma-ilsi
Copy link
Author

ma-ilsi commented Sep 26, 2023

On my own curious accord, I decided to just simplify the Makefiles regex: .*s2n-tls/.*Makefile$ and found something interesting.

The linter reported 100+ Makefiles missing Copyright Amazon.com almost all of them being in tests/cmbc/proofs. These are valid reports. The files have a comment for Apache 2.0 license but no comment/Copyright header. See examples:

https://github.com/aws/s2n-tls/blob/main/tests/cbmc/proofs/s2n_stuffer_resize/Makefile
https://github.com/aws/s2n-tls/blob/main/tests/cbmc/proofs/s2n_stuffer_recv_from_fd/Makefile

...and a huge bulk more.

Simplifying this regex collapses ~30 patterns in to a single pattern, correctly detected more files (which I originally thought were intentionally lacking copyright headers).

I'll wait for you to get back before anymore steps in the dark; I'm guessing this still is not in sync with, and in optimal agreement of, the requirements (which are beyond just making the list of patterns shorter).

@dougch dougch requested a review from kagarmoe October 3, 2023 23:05
@dougch
Copy link
Contributor

dougch commented Oct 3, 2023

I'll wait for you to get back before anymore steps in the dark; I'm guessing this still is not in sync with, and in optimal agreement of, the requirements (which are beyond just making the list of patterns shorter).

Thanks, this is useful data. We're looking for more clarity on what's required here.

Linter now has a complete list of files that were once missing. More files can easily be added with a regex pattern. -scan flag allows finding new files that have not been registered.
Copy link

github-actions bot commented Dec 5, 2023

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Previous increment had chances of failing on Linux systems. This increment is more robust and covers older machines like macos10.
Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

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

Successfully merging this pull request may close these issues.

Copyright linter misses rust files, python files, etc. ci: Check for copyright in Python scripts
2 participants