-
Notifications
You must be signed in to change notification settings - Fork 32
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
Accept all single SPDX license identifiers #52
Conversation
Current limitations: - Doesn't accept compound licenses (AND, OR) - Doesn't accept WITH (e.g. WITH LLVM-exception) Scraped spdx.org on 2021-10-20 with https://github.com/conda-forge/conda-smithy/blob/adb09bbed8136976db3caa139ebcd6fef19f2e8e/extract_licenses.bash
Thanks for taking care of this @jdblischak. I think this solves my immediate issues (#51). For the future I think it would be useful to add replacement text for the most common licenses in CRAN, similar to what #49 did for "GPL-2" -> "GPL-2.0-or-later". However I am a little uncertain about that particular conversion; I would typically interpret CRAN's "GPL-2" to mean "GPL-2.0-only" if I was making a package, and reserve "GPL-2.0-or-later" for packages where CRAN says "GPL (>=2)". |
@brendanf Agreed. But this is a more complicated task than is typically performed by this helper script (typically simple regex's to find/replace). I'm thinking it may finally be time to create a more robust solution. I recently shared my thoughts in #7 (comment) |
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.
Looks good @jdblischak ... just 2 optional comments.
Thanks!
run.py
Outdated
'MIT AND BSD-2-Clause', 'PSF-2.0'} | ||
with open('spdx-licenses.txt') as f: | ||
SPDX_licenses = f.readlines() | ||
SPDX_licenses = [x.strip() for x in SPDX_licenses] |
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.
SPDX_licenses = [x.strip() for x in open(f)]
Should do the trick, imho.
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! I simplified the code. Though I still kept the with open(...
construct, as recommended from this SO thread.
run.py
Outdated
msg = '"{}" license not valid. See {}' | ||
raise ValueError(msg.format(license, SPDX_url)) | ||
msg = 'Warning: "{}" license not valid. See {}\n' | ||
sys.stderr.write(msg.format(license, SPDX_url)) |
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 could use f-strings here.
sys.stderr.write( f'Warning: "{license}" license not valid. See {SPDX_url}\n' )
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! I updated the entire script to use f-strings
This PR expands the acceptable valid licenses to include all the SPDX license identifiers. I downloaded the list of identifiers using the same script used by the conda-forge linter. I also changed the error to a warning when an invalid license is detected. It's better to have users submit incomplete recipes for feedback than to fail locally.
This is meant as a start. There is plenty of room for improvement, e.g.:
Please let me know what you think about this PR in particular as well as plans for the next steps. Thanks!
xref: #49, #51
cc: @brendanf @nick-youngblut @cbrueffer @bgruening