Update SPDX Expression Parsing #719

febuiles · 2024-03-21T18:17:51Z

Context

Since we introduced SPDX licenses around 2022, we've had issues dealing with SPDX expression validations due to the library we use for checking whether one expression satisfies another one.

Folks reached out to the maintainer in 2022 to fix some of these changes, but set a clear direction that does not fit our purposes anymore. The @onebeyond/spdx-license-satisfies is a fork of the original project, created by people who encountered the same issues as us.

Changes

This PR moves the Action away from spdx-satisfies.js and uses @onebeyond/spdx-license-satisfies instead to check whether an SPDX license satisfies an expression or not: The MIT license satisfies the expression MIT OR GPL-2.0, but it does not satisfy MIT AND GPL-2.0.

In the process of making these changes I:

Moved all the SPDX-related logic to spdx.ts.
Added a few TODOs to spdx.ts noting the things we still need to support.
Updated the tests that were using invalid SPDX identifiers.
Removed unnecessary stubs for SPDX license validation.
Updated tsconfig.json to fix a duplicate entry in the compiler options.

src/config.ts

__tests__/external-config.test.ts

__tests__/spdx.test.ts

src/spdx.ts

elireisman · 2024-03-27T15:44:35Z

Couple questions, but this is looking good so far, thanks! 🍻

elireisman · 2024-06-06T21:42:27Z

👋 I just pushed a pretty major overhaul of this PR. It should be ready for review now.

Highlights:

Simplified usage (call sites) of the new SPDX expression eval library
Changed how illegal inputs are handled to make the logic more robust and unit tests better
Removed mocks of the SPDX license eval calls in module tests to ensure we exercise the library
Added lots of new unit tests

That said, there could be surprises here and I'm no TypeScript expert 😂 so please do review this carefully 🙇 cc @hmaurer @bteng22 @mrysav

…w spdx pkg - TODO: update tests

__tests__/spdx.test.ts

elireisman · 2024-06-10T16:58:45Z

package.json

@@ -41,6 +41,7 @@
    "zod": "^3.22.3"
  },
  "devDependencies": {
+    "@types/jest": "^29.5.12",
    "@types/node": "^20",
    "@types/spdx-expression-parse": "^3.0.4",
    "@types/spdx-satisfies": "^0.1.1",


moved Jest back into dev dependencies where I think it belongs 👍

elireisman · 2024-06-10T17:00:38Z

src/licenses.ts

+            validityCache.set(license, found)
+          } else {
+            invalidLicenseChanges.unresolved.push(change)
+          }


☝️ the library is very strict about invalid SPDX inputs, so instead of just bucketing these as "unresolved" based on an error bubble up, we'll now check validity specifically as a pre-step when classifying and resolving licenses 👍

🤔 due to this change, we may be able to ditch the try...catch blocks entirely

src/licenses.ts

src/spdx.ts

elireisman · 2024-06-10T23:12:30Z

__tests__/spdx.test.ts

+    const got: boolean = spdx.satisfiesAny(unit.candidate, unit.licenses)
+    test(`should return ${unit.expected} for ("${unit.candidate}", "${unit.licenses}")`, () => {
+      expect(got).toBe(unit.expected)
+    })
  }


oh nice change! 😍

src/licenses.ts

__tests__/licenses.test.ts

juxtin · 2024-06-12T23:52:49Z

From a code review/testing perspective, I'm pretty happy with this right now. My plan is to do a little bit of manual testing to validate that the linked issues really are resolved, and then I'll get this approved, merged, and released.

juxtin · 2024-06-13T23:01:52Z

From a code review/testing perspective, I'm pretty happy with this right now. My plan is to do a little bit of manual testing to validate that the linked issues really are resolved, and then I'll get this approved, merged, and released.

I'm glad I didn't rush through this, because this PR doesn't resolve any of the issues that were linked above. That's not to say it's a bad PR or that we shouldn't merge it, but it doesn't address the fundamental causes of the behaviors that people have run into.

I'll break it down issue by issue:

Properly resolve licenses with "OR" expressions #670
- problem: Apache-2.0 OR BSL-1.0 should be allowed when only Apache-2.0 is denied. One of its acceptable licenses is implicitly allowed, so it should be fine.
- reality: as of this PR, we use spdx.satisfiesAny('Apache-2.0 OR BSL-1.0', 'Apache-2.0') to evaluate this. It returns true, because 'Apache-2.0 OR BSL-1.0' does satisfy 'Apache-2.0'. This just isn't the semantics that we want.
"Invalid SPDX License" after upgrading JSTS package #575
- problem: EPL-1.0 OR NOASSERTION OR (EPL-1.0 AND NOASSERTION) is considered an Invalid SPDX License.
- reality: as of this PR, NOASSERTION is still considered an invalid license.
deny-licenses mistakenly blocking LGPL-3.0 license #635
- problem: LGPL-3.0 license is denied when the deny list only contains LGPL-2.0, LGPLLR
- reality: for reasons I haven't totally figured out yet, this is still the behavior as of this PR. For what it's worth, internally we appear to have a license of GPL-3.0 AND GPL-3.0-only AND GPL-3.0-or-later AND LGPL-3.0-only for pygithub, but it is displayed in the action summary as LGPL-3.0.

juxtin

Despite the fact that this doesn't fix any outstanding bugs, I do still think it moves us in the right direction. The remaining bugs will have to be addressed by other work in the future.

…113) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [actions/dependency-review-action](https://togithub.com/actions/dependency-review-action) | action | patch | `v4.3.3` -> `v4.3.4` | --- ### Release Notes <details> <summary>actions/dependency-review-action (actions/dependency-review-action)</summary> ### [`v4.3.4`](https://togithub.com/actions/dependency-review-action/releases/tag/v4.3.4) [Compare Source](https://togithub.com/actions/dependency-review-action/compare/v4.3.3...v4.3.4) #### What's Changed - Include all added dependencies in scorecard entries by [@elireisman](https://togithub.com/elireisman) in [https://github.com/actions/dependency-review-action/pull/783](https://togithub.com/actions/dependency-review-action/pull/783) - Update SPDX Expression Parsing by [@febuiles](https://togithub.com/febuiles) in [https://github.com/actions/dependency-review-action/pull/719](https://togithub.com/actions/dependency-review-action/pull/719) - This PR is a significant refactor of SPDX expression parsing that *may* fix some bugs, but unfortunately there are several related known issues that remain unresolved as of this version. **Full Changelog**: actions/dependency-review-action@v4.3.3...v4.3.4 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate).  Signed-off-by: Renovate Bot <[email protected]> Co-authored-by: renovate-gsuquet[bot] <173481049+renovate-gsuquet[bot]@users.noreply.github.com>

…#688) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [actions/dependency-review-action](https://togithub.com/actions/dependency-review-action) | action | patch | `v4.3.3` -> `v4.3.4` | --- ### Release Notes <details> <summary>actions/dependency-review-action (actions/dependency-review-action)</summary> ### [`v4.3.4`](https://togithub.com/actions/dependency-review-action/releases/tag/v4.3.4) [Compare Source](https://togithub.com/actions/dependency-review-action/compare/v4.3.3...v4.3.4) #### What's Changed - Include all added dependencies in scorecard entries by [@elireisman](https://togithub.com/elireisman) in [https://github.com/actions/dependency-review-action/pull/783](https://togithub.com/actions/dependency-review-action/pull/783) - Update SPDX Expression Parsing by [@febuiles](https://togithub.com/febuiles) in [https://github.com/actions/dependency-review-action/pull/719](https://togithub.com/actions/dependency-review-action/pull/719) - This PR is a significant refactor of SPDX expression parsing that *may* fix some bugs, but unfortunately there are several related known issues that remain unresolved as of this version. **Full Changelog**: actions/dependency-review-action@v4.3.3...v4.3.4 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View the [repository job log](https://developer.mend.io/github/xmldom/xmldom).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

febuiles self-assigned this Mar 21, 2024

febuiles marked this pull request as ready for review March 22, 2024 13:37

febuiles requested a review from a team as a code owner March 22, 2024 13:37

mrysav reviewed Mar 22, 2024

View reviewed changes

src/config.ts Outdated Show resolved Hide resolved

mrysav reviewed Mar 22, 2024

View reviewed changes

src/config.ts Outdated Show resolved Hide resolved

src/config.ts Outdated Show resolved Hide resolved

elireisman reviewed Mar 27, 2024

View reviewed changes

__tests__/external-config.test.ts Outdated Show resolved Hide resolved

elireisman reviewed Mar 27, 2024

View reviewed changes

__tests__/spdx.test.ts Show resolved Hide resolved

elireisman reviewed Mar 27, 2024

View reviewed changes

src/spdx.ts Outdated Show resolved Hide resolved

febuiles mentioned this pull request Mar 30, 2024

Properly resolve licenses with "OR" expressions #670

Open

elireisman force-pushed the change-spdx-parser branch 3 times, most recently from 22b3072 to 310ae5f Compare June 6, 2024 21:35

chriswk mentioned this pull request Jun 7, 2024

chore: upgrade to yarn v4 Unleash/unleash#7230

Merged

elireisman added 7 commits June 10, 2024 09:51

add @onebeyond/spdx-license-satisfies to DR Action project

154c150

move jest to dev dependencies

bc5b235

register spdx lib as ES Module, start converting call sites to use ne…

ecd706f

…w spdx pkg - TODO: update tests

complete test suite conversions; simplify fn name

2e4eaa4

update licenses pkg and tests

bbed6f3

more SPDX unit tests to illustrate matching behavior

ed624db

npm run package

f60d593

elireisman force-pushed the change-spdx-parser branch from 310ae5f to f60d593 Compare June 10, 2024 16:52

elireisman reviewed Jun 10, 2024

View reviewed changes

__tests__/spdx.test.ts Outdated Show resolved Hide resolved

elireisman reviewed Jun 10, 2024

View reviewed changes

src/licenses.ts Show resolved Hide resolved

elireisman reviewed Jun 10, 2024

View reviewed changes

src/spdx.ts Show resolved Hide resolved

elireisman and others added 2 commits June 10, 2024 10:11

remove redundant declaration from TS types module registration

d85edeb

Properly display test failures using jest

b4ae47c

elireisman reviewed Jun 10, 2024

View reviewed changes

elireisman reviewed Jun 12, 2024

View reviewed changes

src/licenses.ts Show resolved Hide resolved

juxtin reviewed Jun 12, 2024

View reviewed changes

__tests__/licenses.test.ts Show resolved Hide resolved

juxtin mentioned this pull request Jun 12, 2024

Support for packages with multiple licenses #263

Open

juxtin approved these changes Jul 10, 2024

View reviewed changes

juxtin merged commit 28743f8 into main Jul 10, 2024
6 checks passed

juxtin deleted the change-spdx-parser branch July 10, 2024 17:06

jonjanego mentioned this pull request Aug 9, 2024

Packages being flagged incorrectly with invalid SPDX license definitions #809

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update SPDX Expression Parsing #719

Update SPDX Expression Parsing #719

febuiles commented Mar 21, 2024 •

edited by juxtin

Loading

elireisman commented Mar 27, 2024

elireisman commented Jun 6, 2024 •

edited

Loading

elireisman Jun 10, 2024

elireisman Jun 10, 2024

elireisman Jun 12, 2024

elireisman Jun 10, 2024

juxtin commented Jun 12, 2024

juxtin commented Jun 13, 2024

juxtin left a comment

Update SPDX Expression Parsing #719

Update SPDX Expression Parsing #719

Conversation

febuiles commented Mar 21, 2024 • edited by juxtin Loading

Context

Changes

elireisman commented Mar 27, 2024

elireisman commented Jun 6, 2024 • edited Loading

elireisman Jun 10, 2024

Choose a reason for hiding this comment

elireisman Jun 10, 2024

Choose a reason for hiding this comment

elireisman Jun 12, 2024

Choose a reason for hiding this comment

elireisman Jun 10, 2024

Choose a reason for hiding this comment

juxtin commented Jun 12, 2024

juxtin commented Jun 13, 2024

juxtin left a comment

Choose a reason for hiding this comment

febuiles commented Mar 21, 2024 •

edited by juxtin

Loading

elireisman commented Jun 6, 2024 •

edited

Loading