-
Notifications
You must be signed in to change notification settings - Fork 24
Add copyright checking and shell lint to lint
#424
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
base: main
Are you sure you want to change the base?
Conversation
545af13
to
9a1759f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @willieyz! Something is not yet working correctly, because in fact not all source files have the Apache-2.0 OR MIT OR ISC
license -- see e.g. https://github.com/pq-code-package/mldsa-native/blob/main/mldsa/native/aarch64/src/ntt.S. Somehow the script does not seem to catch this.
9a1759f
to
12e8f63
Compare
scripts/lint
Outdated
fi | ||
done | ||
# For source files in dev/* and mldsa/*, we enforce `Apache-2.0 OR ISC OR MIT` | ||
for file in $(git ls-files -- "*.[chsSi]" | grep "^dev/\|^mlkem/"); do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be mldsa
, not mlkem
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, @hanno-becker,
Thank you for your reviewing, and apologies for my careless mistake...
After correcting it to use mldsa
instead of mlkem
, the script correctly catch the SPDX + Copyright headers
error,
see following link:
https://github.com/pq-code-package/mldsa-native/actions/runs/17578845391/job/49930280691?pr=424
I have a question, for these two file(intt.S
, ntt.S
),
Should I update them to use Apache-2.0 OR ISC OR MIT as the script requires, instead of only MIT(same as mlkem-native
), or should I modify the script so that it passes if only one of the licenses is present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot just change licenses of code without asking the copyright owners - so that is not an option.
Let's temporarily whitelist these two files and add a TODO in the script it needs to be removed once licenses have been sorted out as a part of #381.
6cd2ee0
to
ce1ec0c
Compare
53c30aa
to
694fb17
Compare
Signed-off-by: willieyz <willie.zhao@chelpis.com>
694fb17
to
b37d47f
Compare
lint
#423lint
, also align the color format and syntax with mlkem-native