-
Notifications
You must be signed in to change notification settings - Fork 5
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 fetch URL for exception.json and update license files #54
base: main
Are you sure you want to change the base?
Conversation
@fjssilva please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement ( “Agreement” ) is agreed to by the party signing below ( “You” ), 1. Definitions. “Code” means the computer software code, whether in human-readable or machine-executable form, “Project” means any of the projects owned or managed by .NET Foundation and offered under a license “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any “Submission” means the Code and any other copyrightable material Submitted by You, including any 2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any 3. Originality of Work. You represent that each of Your Submissions is entirely Your 4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else 5. Licenses. a. Copyright License. You grant .NET Foundation, and those who receive the Submission directly b. Patent License. You grant .NET Foundation, and those who receive the Submission directly or c. Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement. 6. Representations and Warranties. You represent that You are legally entitled to grant the above 7. Notice to .NET Foundation. You agree to notify .NET Foundation in writing of any facts or 8. Information about Submissions. You agree that contributions to Projects and information about 9. Governing Law/Jurisdiction. This Agreement is governed by the laws of the State of Washington, and 10. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and .NET Foundation dedicates this Contribution License Agreement to the public domain according to the Creative Commons CC0 1. |
@@ -0,0 +1,7 @@ | |||
{ |
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.
Was the license already supported by us? Otherwise, we need to triage the case by case. Not sure what this license for? Same applies for others.
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.
I don't think this is a matter of licenses being supported or not. This repository keeps and maintains replicas of the licences and exceptions hold in https://spdx.org/licenses/ and https://spdx.org/licenses/exceptions-index.html. The script license-text-extractor.ps1 in this repository does all the cloning and updating work. It was only a matter of chance (bug) that this script was stuck on downloading the exceptions from an old commit in the SPDX GitHub repository. Once I fixed the script, all newer license exception files were downloaded and processed automatically.
Same applies to all new files added in this patch. Actually, you can replicate this fix quite easily: just fix the script license-text-extractor.ps1 as I did and run it.
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.
Let's scope to Universal-FOSS-exception-1.0 license
. We need approval for new licenses on a case-by-case basis. Let's solve one problem at the time, there're not simple documents, actual legal document, we need to be careful.
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.
Some license supported by https://spdx.org/licenses/ doesn't mean we should immediately accept it.
@@ -1,7 +0,0 @@ | |||
{ |
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.
Why this one was deleted?
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.
Same as before, the script license-text-extractor.ps1 did it, not me.
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.
But, digging into the details, the reason why it was deleted is because this license is now deprecated and the extractor script skips those https://github.com/NuGet/NuGet.Licenses/blob/main/src/NuGet.Licenses/license-text-extractor.ps1#L19.
Before my fix, the list of license exceptions was wrongly taken from https://raw.githubusercontent.com/spdx/license-list-data/45d10da0366f5fa931f60f3931fd23d5fb708de5/json/exceptions.json, where Nokia-Qt-exception-1.1
wasn't deprecated yet, and now it is deprecated in the current/latest https://raw.githubusercontent.com/spdx/license-list-data/main/json/exceptions.json.
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.
Considering we assume that packages never mutate once published, and I believe it extends to embedded data like license too. What happens if the package Nokia123
was published with the Nokia-Qt-exception-1.1.json
license 5 years ago and the license is deleted now? Suddenly, it turns into a package without any license. Now, any enterprise consuming that package is in limbo or has to constantly check if it still has a license. Instead, regardless of what happens, the package should always keep its original license. Please correct me if I'm wrong.
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.
I don't disagree with you. But then, INHO, the license extractor tool should be fixed to not exclude deprecated licenses. Not my call.
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.
We can't focus on extractor tool right now due to high priority work we have scheduled right now. This isn't ideal and you're welcome to contribute fixing the tool.
@@ -15,7 +15,7 @@ Write-Host "Found $($licenseIds.Count) licenses." | |||
# The SPDX version that client uses is here: https://github.com/NuGet/NuGet.Client/blob/release-6.4.x/src/NuGet.Core/NuGet.Packaging/Licenses/NuGetLicenseData.cs | |||
# Make note of the release label in the NuGet.Client URL above! This should match the version of NuGet.Packaging that NuGet.Licenses.csproj depends on. | |||
Write-Host "Fetching exception IDs..." | |||
$exceptionResponseData = Invoke-RestMethod "https://raw.githubusercontent.com/spdx/license-list-data/45d10da0366f5fa931f60f3931fd23d5fb708de5/json/exceptions.json" |
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.
I think we should use specific latest commit hash instead of just main.
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.
^ @NuGet/gallery-team
What do you think?
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.
Note that this change was made in line with https://github.com/NuGet/NuGet.Licenses/blob/main/src/NuGet.Licenses/license-text-extractor.ps1#L10, which also downloads the licenses list from main.
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.
I'll wait for @joelverhagen and @agr's feedback on this since they created the extractor tool.
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.
The comment starting at line 14 explains why specific version has to be used for exceptions. We can't just grab latest from main
, we have to align with the snapshot used by the version of the NuGet.Packaging
package referenced by the project.
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.
The comment just says "The expression analyzer is a bit sensitive for unknown license exceptions...", it doesn't explain what the issues actually are. Anyway, I'm trying to connect the dots here, so, according to what you just explained, the licences (exceptions) in this repository must match the license list version "bb0099c", which, AFAICT, come from the SPDX snapshot https://github.com/spdx/license-list-data/tree/38193d69ba3e0b9e84f94ebc23c6e3a2bd9b0bec. If so, then the URL to download the exceptions from must be https://raw.githubusercontent.com/spdx/license-list-data/38193d69ba3e0b9e84f94ebc23c6e3a2bd9b0bec/json/exceptions.json. Correct?
I have to ask, though: if this is the case, then why you don't do the same with the main licences file? Note how so many have changes since the last time the license files were updated in this repository and new ones were added too. Wouldn't this hit the same problem as with the exceptions?
Also, the (potential) issue related to the deprecated exception Nokia-Qt-exception-1.1
will remain because this exception is already deprecated in the license list version "bb0099c".
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.
We can't just blindly grab the latest version of exception data. It has to align with the NuGet.Packaging supported list.
Specifically, the concern is, if I remember correctly, that we shouldn't have any exceptions client doesn't know about. And it seems the package haven't updated its list of licenses/exceptions for a while (the snapshot referenced in the code haven't changed for a while). I'll ask the client folks if they can sync the list, then we can pull the changes here and update the list on our end.
However, it seems the package "knows" about the Universal-FOSS-exception-1.0
exception, so maybe, just changing the URL to use the bb0099c
snapshot, the client expects would be enough.
So, exceptions are the issue? No problem at all in having (main) licenses the client doesn't know about? BTW, all fine by me. I'm just pointing it out because it sounds strange to me, especially because NuGetLicenseData.cs does refer to a specific license list version...
Yes, it seems so. However, mind that running the extractor tool with this snapshot will still remove the exception |
I don't remember the details, but the API of the licenses service assumes that SPDX license expression is passed as the query path. If the expression contains more than one element, a page is displayed with links to individual elements (licenses or exceptions). However, standalone exception name is not a valid SPDX license expression (exceptions can only follow the The I also looked through all the exceptions specified in license expressions for packages, and it seems none use |
Thanks @agr. FYI, this PR is a consequence of the missing page in Licensing information for the MySQL.Data package. As you can see, the links are rendered properly but the Universal-FOSS-exception-1.0 links to nowhere. So, IIUC, what I should do next is to rework this patch so that it downloads the exceptions from the snapshot Just one remark: if I do so, the patch will also include all the new new licenses (not only exceptions) and changes in some of the existing license files, same as what you see already on this PR since the licenses are downloaded from |
Yes, please update the exception snapshot. Pulling new licenses is fine. For completeness, I sent the PR to update NuGet.Packaging to sync the license list to its today's state, so when the package update is published, we'd enable those to be actually used for new packages. This would require re-sync of exceptions, but we'll sort it out later. |
5078cb0
to
3563d0b
Compare
3563d0b
to
8ddbaa3
Compare
PR updated. The extractor tool now downloads from the snapshot |
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.
Thank you!
I verified that those are all the changes that are available in SPDX repo and spot checked several licenses that the changes brought in are indeed the license template maintenance.
@erdembayar I know that, but I can't sign the CLA without my company's (Oracle) acceptance, and for that I need to involve our legal team, which may lead to a long bureaucratic process. |
I don't know what the best way is for you. Is there anything owned by Oracle in this PR? |
@dotnet-policy-service agree company="Oracle America, Inc." |
@microsoft-github-policy-service agree company="Oracle America, Inc." |
The script
license-text-extractor.ps1
was wrongly fetching an old version of the fileexception.json
that only contained a small number of license exceptions.This fixes the NuGetGallery issue #10225. As stated in the issue, I only require the missing text for the exception
Universal-FOSS-exception-1.0
but, given how license files are updated in this repository, all missing licenses and other changes were automatically applied to this patch.