-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[License] update license SPDX in all package.json, source file headers #12584
Conversation
@@ -19,16 +19,16 @@ Eclipse Theia is an extensible framework to develop full-fledged multi-language | |||
|
|||
</div> | |||
|
|||
- [**Website**](#website) |
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 was not an intended change. When I try to change it back locally, the stars **
are automatically removed. I am guessing that using them is against some config we have somewhere. I think I would just leave out these stars, unless there are objections.
439aed7
to
bb5a08f
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.
The changes look good to me 👍
- the splitting looks correct
- the new
header
snippet works as intended - the updated eslint rule correctly identifies errors with the header if missing, or mistyped
Note: I have a little doubt now, that the SPDX might have been correct. I checked on our EF project page, in the Legal Documents Generator page, and the license SPDX that we use is as proposed there. I am not sure if this might be an old way of referring to that license, but I did find several other matches, when searching online. I am not sure why the I have added a related comment on the IP ticket - let's see what the IP Team thinks |
The current form is actually more correct than the changes in this PR. The The documentation generator is correct. It should be Note that this is different from what you're currently using. The FWIW, there are two choices for the GPL-2.0: So... the SPDX-License-Identifier should be Note that while changing |
Agreed.
Understood, thanks @waynebeaton! |
6369470
to
bf2e5df
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.
I re-affirm my previous review 👍
@msujew I wanted to mention that I tested the GitHub detection of our licenses, using my fork (the only consistent way I found was to merge to master and observe how it affected the detection). Doing so, I have confirmed that even with tweaked SPDX, the licenses were not detected correctly when I tried going-back to a single license file. So, I think your split of the single license file, into two files, was the correct move! |
Thanks for the review @vince-fugnitto . I will squash the commits locally and tidy-up the commit message. |
I have just rebased this PR locally to confirm that linting still passes (in case a PR had been merged in the meantime, that used the old SPDX in source header. So, there should be no surprises when we merge. It's possible that there are PRs under review that will need to update new headers - the safest will be to rebase on latest master once this one here is in. |
1) as per Wayne's suggestion, it would be clearer to use the SPDX that reflects the fact that our secondary license is of the "GPL-2.0-ONLY" type 2) rename LICENSE-GPL to more precisely reflect our secondary license 3) As per SPDX[1], it seems that our secondary license text has accidently deviated from stock, during a recent refactoring. Let's restore it to what it should be. [1]: https://spdx.org/licenses/GPL-2.0-with-classpath-exception.html Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
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 for the work on this @marcdumais-work, looks good to me as well 👍
What it does
update: following Wayne's feedback below, I have updated the description and content of this PR
This PR tweaks the secondary license SPDX short identifier, to use the most precise version that applies. It also restores the original text of secondary license, that was accidentally changed during the recent file split.
Making these changes should make our packages easier to deal-with, from an IP PoV, for adopters and extenders, by making our licenses easier to auto-detect.
For more details see the corresponding issue
Note:
Since this PR changes our copyright headers and the stated license in variouspackage.json
files, I have created an IP ticket, for the Foundation IP Team to sign-off on these changes: ticket. We should probably wait (timeout: 3 weeks?) for feedback before merging.The feedback was received and acted-upon. This PR updates the license headers to use the more precise, suggested SPDX:
Before:
SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
After:
SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0
Fixes #12583
How to test
This PR makes changes to almost 2000 files. As such I have experienced slow load/update times, working in the GitHub PR diff view (Files Changed tab). It might be preferable to review the changes locally. I have split things in separate commits, so that important changes do not go unnoticed, among the mass of license header changes. Please consider looking at the PR commit-by-commit. We can squash into a single commit at the end, before merging.
Review checklist
Reminder for reviewers