Updated SPDX related files in light of 2.0 release of SPDX specification #4208
Conversation
|
@alcohol this method is parsing strings like |
|
and failing tests are because some of the licenses used by the tests are now removed from the SPDX list (see the diff on the JSON file) |
|
Yeah I figured parts of it out. I was thinking, do we still want to add the deprecated licenses to the current list and add a I fixed the tests btw but it doesn't support the new specification fully yet, need to add additional tests for that. |
|
I would say no. We should rather support the new SPDX way to provide such licenses. |
|
Btw the notation of |
|
I don't remember whether it was part of SPDX 1 or no. But it is part of SPDX 2 for sure (look at the spec for license expressions, and you will see it covers them) |
| @@ -220,6 +221,7 @@ private function isValidLicenseString($license) | |||
| $require = 0; | |||
| break; | |||
| case 'ws': | |||
| case 'pl': | |||
There was a problem hiding this comment.
pl should not be allowed anywhere. It can only follow a license identifier
|
Just a thought; would you guys be at all opposed to the idea of extracting the SPDX related stuff into package by itself? |
| @@ -144,8 +144,9 @@ private function isValidLicenseString($license) | |||
| 'op' => '(?:or|and)', | |||
| 'lix' => '(?:NONE|NOASSERTION)', | |||
| 'lir' => 'LicenseRef-\d+', | |||
| 'lic' => '[-+_.a-zA-Z0-9]{3,}', | |||
| 'lic' => '[-_.a-zA-Z0-9]{3,}', | |||
There was a problem hiding this comment.
Maybe just having the \+? here at the end would be fine and then rtrim it in isValidLicenseIdentifier?
|
@alcohol I thought about doing exactly that when reviewing this PR |
alcohol
force-pushed
the
update-spdx-updater
branch
from
ad81fa9
to
3ddd805
Compare
|
This covers it all and makes it relatively easy to extract. However, it's not incredibly elegant.. so yeah, feedback/thoughts/suggestions? |
alcohol
force-pushed
the
update-spdx-updater
branch
from
ee1bdc5
to
67dec28
Compare
| */ | ||
| private function isValidLicenseString($license) | ||
| { | ||
| $tokens = array( | ||
| 'po' => '\(', | ||
| 'pc' => '\)', | ||
| 'op' => '(?:or|and)', | ||
| 'op' => '(?:OR|AND)', |
There was a problem hiding this comment.
I suggest allowing the lower case versions too, to keep BC
There was a problem hiding this comment.
Well the tests don't allow mixed casing. So just using a case insensitive modifier wouldn't work. It would then be or|OR|and|AND. But the specs explicitly only use uppercase. You sure you want to keep BC for this?
alcohol
force-pushed
the
update-spdx-updater
branch
from
739bde1
to
fdc25c2
Compare
|
Looks good to me, are you done done @alcohol? :) |
| array("MIT (MIT and MIT)"), | ||
| array("(MIT and MIT) MIT"), | ||
| array(array("LGPL-2.0", "The system pwns you")), | ||
| array("and GPL-3.0+"), | ||
| array("EUDatagrid and GPL-3.0+"), |
There was a problem hiding this comment.
is this one actually valid now ?
There was a problem hiding this comment.
It always was valid actually. I don't know why it was not marked as valid.
I mean, it should have been considered valid. I don't know why it didn't pass before.
There was a problem hiding this comment.
The regex does match a string such as "MIT AND MIT" though. So I guess it's not good enough :-/
|
Reverted it so it stays available in history, if someone ever wants to revisit it. |
|
Edit: ok so the specs say that composite license expressions SHOULD be encapsulated by parentheses to facilitate expression parsing (not MUST be by the way). That's probably why the tokenizer approach failed them (on purpose). The regex does not accommodate that either. |
alcohol
force-pushed
the
update-spdx-updater
branch
from
fbdfe93
to
4019f7b
Compare
|
Rebased it for now. /cc @Seldaek Ready to merge and then extract to self contained package. |
Updated SPDX related files in light of 2.0 release of SPDX specification
|
Cool thanks |
Not sure if desired or not, but figured it was time for an update.