Introduce standardized license identifiers #3699

Biscgit · 2024-11-20T13:29:39Z

Introduce standardized license identifiers

With this PR we are moving to use standardized licenses (as listed in SPDX license list) for all records. I also added a test and GH Action to ensure that new records will have the license identifiers set correctly before merging.

Once this has been merged, the compatibility implementations in the portal can be removed safely.

Warning

Only merge this AFTER cernopendata/cernopendata-portal#87 has been deployed to production! Otherwise, it might break the current implementation of displaying licenses.

Edit: This has been deployed by now

Important

Since not all licenses could be clearly transfered to which they concretly refer to (for example which `BSD-X-Clause`), please especially check the changes in the following comments.

Closes #3111

Biscgit · 2024-11-20T13:32:56Z

data/records/cms-tools-ispy.json

@@ -115,7 +115,7 @@
      "CMS"
    ],
    "license": {
-      "attribution": "BSD License"
+      "attribution": "BSD-3-Clause"


No further information on which exact BSD license is used. Assuming most commonly used.

Looking into the source code repository linked in that record: https://github.com/cms-outreach/ispy-webgl/blob/master/LICENSE I see that the license is actually MIT now, not BSD.

Biscgit · 2024-11-20T13:35:09Z

data/records/cms-tools-ana-Run2011A.json

@@ -129,7 +129,7 @@
      "CMS"
    ],
    "license": {
-      "attribution": "GNU General Public License (GPL) version 3"
+      "attribution": "GPL-3.0-only"


No further information on if only the current GPLv3 license should be used, or also apply to changes in upcoming versions. Assuming only the current license should be valid.

Yes, this should be OK. However if we want to be very precise, you can check the license body that exists in the linked repository: https://github.com/cms-opendata-analyses/pattuples2011/blob/master/COPYING

I think it is better to leave it with -only. It should be much easier to expand that sometime in the future if needed, than later revert the changes.

tiborsimko · 2024-12-12T15:25:06Z

scripts/check_licenses.py

@@ -0,0 +1,92 @@
+"""Check if license fields are valid in all records."""


todo: ‏Can you please introduce leading shebang?

#!/usr/bin/env python

And make this script executable by itself?

chmod u+x scripts/check_licenses.py

This will be good for consistency with the other helper scripts.

tiborsimko · 2024-12-12T15:25:51Z

scripts/check_licenses.py

+import pathlib
+import time
+
+VALID_LICENSE_IDENTIFIERS = [


praise: ‏I appreciate that you included the checker tool together with fixture changes 👍

tiborsimko · 2024-12-12T15:27:45Z

run-tests.sh

@@ -99,13 +99,18 @@ check_isort () {
    isort -rc -c -df --profile black -- **/*.py
 }

+check_licenses () {
+    python3 scripts/check_licenses.py


nitpick (non-blocking): ‏After chmod-ding the check license script, see the other comment, you can remove reading python3 here.

tiborsimko · 2024-12-12T15:29:47Z

scripts/check_licenses.py

+
+            if attr not in VALID_LICENSE_IDENTIFIERS:
+                recid = record.get("recid", "UNSET")
+                message = f"Invalid license `{attr}` identifier in file {path.name} with recid {recid}! "


todo: ‏The order "invalid license FOO identifier" could be inverted for readability: "invalid license identifier FOO".

todo: Instead of "with recid RECID", we should say "for recid RECID".

tiborsimko · 2024-12-12T15:31:03Z

scripts/check_licenses.py

+        logging.error(
+            f"Validation completed with {errors} errors!\n"
+            f"\tPlease ensure the licenses are one of the following: {VALID_LICENSE_IDENTIFIERS}.\n"
+            f"\tIn case it is a valid license, please contact the Open Data Team."


todo: ‏The last phrase may not be clear to users. Perhaps say: "If you use a valid SPDX license string that is not in the above list, please contact [email protected]".

tiborsimko · 2024-12-12T15:33:30Z

data/records/cms-tools-ispy.json

@@ -115,7 +115,7 @@
      "CMS"
    ],
    "license": {
-      "attribution": "BSD License"
+      "attribution": "BSD-3-Clause"


Looking into the source code repository linked in that record: https://github.com/cms-outreach/ispy-webgl/blob/master/LICENSE I see that the license is actually MIT now, not BSD.

tiborsimko · 2024-12-12T15:36:39Z

data/records/cms-tools-ana-Run2011A.json

@@ -129,7 +129,7 @@
      "CMS"
    ],
    "license": {
-      "attribution": "GNU General Public License (GPL) version 3"
+      "attribution": "GPL-3.0-only"


Yes, this should be OK. However if we want to be very precise, you can check the license body that exists in the linked repository: https://github.com/cms-opendata-analyses/pattuples2011/blob/master/COPYING

tiborsimko · 2024-12-12T15:46:00Z

todo: ‏I'd also have some commit log cosmetics comments. The current status looks like:

Licenses: Rebased on main branch and adjusted licenses for new delphi datasets
checks: improved final log messages
license: changed licenses current ids to standardized format
checks: fixed missing error exit
checks: created new action for running the license check
checks: applied linter changes
checks: created check for upcoming license changes

It is not necessary to have all these commits separate, since chances are we won't start any branch from those "intermediate" commits that are not really necessary and that may only pollute git commit history. (Which could be counterproductive for later bisecting). This is just cosmetics for the pull request at hand, but may be important for big feature changes for the application. It's good to use the same principles here, for consistency.

You may want to squash everything into basically two different commits:

ci(check-licenses): add licence value checker
feat(records): switch to SPDX license attribution values

Note also the usage of Conventional Commits style for the commit messages. It's a nice wide-spread standard, we use it for REANA or for cernopendata-client, and I started to use it lately for the open data content repository as well. It make things tidier when one inspects the commit headers of a repository.

tiborsimko · 2024-12-12T15:50:16Z

todo: ‏Also please add yourself to the AUTHORS.rst file so that your contributions are nicely and visibly acknowledged 😄

Biscgit · 2024-12-13T10:17:45Z

I merged today's introduced atlas records into this branch as well 👍

Closes #3111

Biscgit linked an issue Nov 20, 2024 that may be closed by this pull request

records: standardise license strings #3111

Closed

Biscgit commented Nov 20, 2024

View reviewed changes

Biscgit requested a review from tiborsimko November 20, 2024 13:35

Biscgit changed the title ~~Standardize license ids~~ Introduce standardized license identifiers Nov 20, 2024

Biscgit force-pushed the standardize-license-ids branch from cc4df0e to 996a6f7 Compare November 20, 2024 14:00

Biscgit assigned tiborsimko Dec 10, 2024

Biscgit added Topic: records Type: enhancement Status: ready for work labels Dec 10, 2024

Biscgit force-pushed the standardize-license-ids branch 2 times, most recently from fe2aab2 to 4810ff3 Compare December 12, 2024 13:48

Biscgit added Status: in review and removed Status: ready for work labels Dec 12, 2024

tiborsimko reviewed Dec 12, 2024

View reviewed changes

Biscgit force-pushed the standardize-license-ids branch 2 times, most recently from 36c9766 to 7d1843b Compare December 13, 2024 10:16

tiborsimko force-pushed the standardize-license-ids branch from 7d1843b to 1feff5a Compare December 17, 2024 12:48

feat(records): switch to SPDX license attribution values

29183a0

Closes #3111

tiborsimko force-pushed the standardize-license-ids branch 2 times, most recently from 3843794 to 1a79680 Compare December 17, 2024 13:45

ci(check-licenses): add license value checker

69b149e

tiborsimko force-pushed the standardize-license-ids branch from 1a79680 to 69b149e Compare December 17, 2024 13:51

tiborsimko merged commit 69b149e into master Dec 17, 2024
14 checks passed

tiborsimko approved these changes Dec 17, 2024

View reviewed changes

tiborsimko deleted the standardize-license-ids branch December 17, 2024 14:29

tiborsimko mentioned this pull request Jan 6, 2025

ci: generalise license checker for other controlled-vocabulary fields #3714

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce standardized license identifiers #3699

Introduce standardized license identifiers #3699

Biscgit commented Nov 20, 2024 •

edited by tiborsimko

Loading

Biscgit Nov 20, 2024 •

edited

Loading

tiborsimko Dec 12, 2024

Biscgit Nov 20, 2024 •

edited

Loading

tiborsimko Dec 12, 2024

Biscgit Dec 13, 2024

tiborsimko Dec 12, 2024

tiborsimko Dec 12, 2024

tiborsimko Dec 12, 2024

tiborsimko Dec 12, 2024

tiborsimko Dec 12, 2024

tiborsimko Dec 12, 2024

tiborsimko Dec 12, 2024

tiborsimko commented Dec 12, 2024

tiborsimko commented Dec 12, 2024

Biscgit commented Dec 13, 2024

		@@ -0,0 +1,92 @@
		"""Check if license fields are valid in all records."""

Introduce standardized license identifiers #3699

Introduce standardized license identifiers #3699

Conversation

Biscgit commented Nov 20, 2024 • edited by tiborsimko Loading