-
Notifications
You must be signed in to change notification settings - Fork 311
Add automated testing of license XML #513
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
Conversation
Signed-off-by: Gary O'Neall <[email protected]>
Signed-off-by: Gary O'Neall <[email protected]>
Signed-off-by: Gary O'Neall <[email protected]>
Signed-off-by: Gary O'Neall <[email protected]>
Signed-off-by: Gary O'Neall <[email protected]>
Signed-off-by: Gary O'Neall <[email protected]>
BTW - just noticed the commits should be squashed when merging the pull requests |
I'm concerned about how opaque this is to folks unfamiliar with spdx/tools (like me ;). I'd be happier if we filled in license-test-files and pulled a specific version of license-test-files to test against. So in
Would that be possible, or is that too far from where spdx/tools is now? |
Yes - I should add this to Travis. As it turns out, it is not currently testing against any of the license test files which is an oversight on my part.
Prefer to wget the tools from the released binary location - which is currently in the travis.yml
Agree - I'll update the PR after testing the new approach |
…riginal license text Signed-off-by: Gary O'Neall <[email protected]>
@wking Updated the PR to include the test files eliminating the need to pull from the test repository |
I'd rather keep the test files separate (as we're discussing in spdx/license-test-files#6). If we go with the separate-test-repo approach, you could recreate your current
or alternatively (if you need per-commit granularity):
|
Signed-off-by: Gary O'Neall <[email protected]>
Signed-off-by: Gary O'Neall <[email protected]>
Signed-off-by: Gary O'Neall <[email protected]>
.travis.yml
Outdated
@@ -2,4 +2,4 @@ language: node_js | |||
node_js: | |||
- "node" | |||
before_script: | |||
- npm run preinstall | |||
- npm run preinstall |
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.
3bc8691 removes the trailing newline here, which I think we want to keep.
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.
Added to the later commit
Signed-off-by: Gary O'Neall <[email protected]>
Signed-off-by: Gary O'Neall <[email protected]>
Signed-off-by: Gary O'Neall <[email protected]>
Based on the legal workgroup all on 21 Dec 2017, we agreed to add this testing and use the test directory in the same repository. Will merge in after the release. |
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'd still rather have the test content in an external repository. Besides the reasons given in spdx/license-test-files#6, this addition is increasing the working-directory size of this repository by ~40% (adding 3.7 MB to master's 4.9 MB). And the provenance of the test content is unclear, which makes review hard (what do I compare to to determine if the content is accurate?).
test/original/RPL-1.5.txt
Outdated
|
||
3.4 Licensor reserves the right to release new versions of the Software with different features, specifications, capabilities, functions, licensing terms, general availability or other characteristics. Title, ownership rights, and intellectual property rights in and to the Licensed Software shall remain in Licensor and/or its Contributors. | ||
|
||
4.0 Grant of License From Contributor. By application of the provisions in Section 6 below, each Contributor hereby grants You a world-wide, royalty- free, non-exclusive license, subject to said Contributor's intellectual property rights, and any third party intellectual property claims derived from the Licensed Software under this License, to do the following: |
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.
Where is this content from? I see we have the SPDX-specific “royalty- free” typo here (fixed in #431).
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.
Where is this content from?
All of the license texts for any licenses existing prior to the 3.0 update are are from the original license list repository (https://github.com/spdx/license-list). If there were compare errors, I researched the origin and if needed updated the text.
If there are errors in the license-list text files, they will be replicated here unless it caused a comparison failure.
I'll fix this specific error.
Please post if you see any other errors.
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.
In fixing the above referenced issue, I found another apparent copy/paste error with "non-exclusive". I'll fix that in the update as well.
test/original/Adobe-Glyph.txt
Outdated
|
||
Permission is hereby granted, free of charge, to any person obtaining a copy of this documentation file, to create their own derivative works from the content of this document to use, copy, publish, distribute, sublicense, and/or sell the derivative works, and to permit others to do the same, provided that the derived work is not represented as being a copy or version of this document. | ||
|
||
Adobe shall not be liable to any party for any loss of revenue or profit or for indirect, incidental, special, consequential, or other similar damages, whether based on tort (including without limitation negligence or strict liability), contract or other legal or equitable grounds even if Adobe has been advised or had reason to know of the possibility of such damages.Ê The Adobe materials are provided on an "AS IS" basis.Ê Adobe specifically disclaims all express, statutory, or implied warranties relating to the Adobe materials, including but not limited to those concerning merchantability or fitness for a particular purpose or non-infringement of any third party rights regarding the Adobe materials. |
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 “Ê” (fixed in #429) is another instance of questionable provenance. I agree that we want to match this form, because it matches the preferred form from earlier SPDX releases. But I doubt upstream ever included the Ê, and we (since #429) no longer recommend the Ê, so I'd rather not include it in this “original” file.
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.
@wking Good catch, I agree. Removing it from the original text, will push an update to the PR.
Understood - and the size is a good consideration. Even weighing the additional repository size, the added complexity for new license submissions makes the inclusion of the test files in the license-list-XML repository a better option for most contributors.
For any new submissions when there have been comparison failures, I've been comparing the text to the license text referenced in the URL's included in the license XML file. |
Signed-off-by: Gary O'Neall <[email protected]>
Signed-off-by: Gary O'Neall <[email protected]>
Signed-off-by: Gary O'Neall <[email protected]>
Temporarily closing this PR while I test a new version |
Signed-off-by: Gary O'Neall <[email protected]>
Resolves issues #502 and #503