Inconsistent behaviour of --experimental-licenses

[oliverchang]

The table output for --experimental-licenses has inconsistent behaviour:

--experimental-licenses

$ go run ./cmd/osv-scanner --experimental-licenses -L /tmp/pom.xml       
Scanning dir /tmp/pom.xml
Unable to parse git ignores: open /tmp/env.boot.1699391406.376213.6470.d: permission denied
Scanned /tmp/pom.xml file and found 4 packages
╭───────────────────┬───────────┬──────────────────────────────────────────────────┬─────────┬───────────────────────────────╮
│ LICENSE VIOLATION │ ECOSYSTEM │ PACKAGE                                          │ VERSION │ SOURCE                        │
├───────────────────┼───────────┼──────────────────────────────────────────────────┼─────────┼───────────────────────────────┤
│ Apache-2.0        │ Maven     │ org.apache.logging.log4j:log4j-api               │ 2.17.1  │ ../../../../../../tmp/pom.xml │
│ Apache-2.0        │ Maven     │ org.apache.logging.log4j:log4j-core              │ 2.17.1  │ ../../../../../../tmp/pom.xml │
│ Apache-2.0        │ Maven     │ org.springframework.boot:spring-boot-starter     │ 2.7.6   │ ../../../../../../tmp/pom.xml │
│ Apache-2.0        │ Maven     │ org.springframework.boot:spring-boot-starter-web │ 2.7.8   │ ../../../../../../tmp/pom.xml │
╰───────────────────┴───────────┴──────────────────────────────────────────────────┴─────────┴───────────────────────────────╯
exit status 4

The expected behaviour here seems to the same as --expeirmental-licenses=""? EDIT: this seems like it's just ambiguity with flag parsing per #671 (comment).

--experimental-licenses=""

go run ./cmd/osv-scanner --experimental-licenses="" -L /tmp/pom.xml
Scanned /tmp/pom.xml file and found 4 packages
╭────────────┬─────────────────────────╮
│ LICENSE    │ NO. OF PACKAGE VERSIONS │
├────────────┼─────────────────────────┤
│ Apache-2.0 │                       4 │
╰────────────┴─────────────────────────╯
exit status 4

The exit codes also seem wrong here (it should be 0)?

license_violations included in JSON with --experimental-licenses=""

go run ./cmd/osv-scanner --format json --experimental-licenses "" -L /tmp/pom.xml
Scanned /tmp/pom.xml file and found 4 packages
{
  "results": [
    {
      "source": {
        "path": "/tmp/pom.xml",
        "type": "lockfile"
      },
      "packages": [
        {
          "package": {
            "name": "org.apache.logging.log4j:log4j-api",
            "version": "2.17.1",
            "ecosystem": "Maven",
            "commit": ""
          },
          "licenses": [
            "Apache-2.0"
          ],
          "license_violations": [
            "Apache-2.0"
          ]
        }

@josieang could you please take a look?

[oliverchang]

CC @another-rex

[oliverchang]

Ah actually, I think the ambiguity from the first example above comes from --experimental-licenses -L in the first example. -L is getting interpreted as a license instead of the next flag.

If it's not possible to reliably distinguish between --experimental-licenses "value" vs --experimental-licenses --next-flag, we might need to consider breaking this out into two flags. Otherwise the behaviour here is a little confusing.

Alternatively, to be consistent with our --call-analysis flags in #513, we should just replace "" with "all" as the special value to print all licenses to make it more explicit and avoid confusion:

i.e.

osv-scanner --experimental-licenses=all # Print all licenses, don't check for license violations
osv-scanner --experimental-licenses="allowlist1,allowlist2" # Check for license 

@josieang @another-rex WDYT?

[josieang]

I remember this now, and I totally should have flagged it. I couldn't get --experimental-licenses --next-flag to work, so
the implementation I've done always take in an argument.

I'm all for consistency, so if --experimental-licenses=all is consistent with the call analysis flags that sounds like a good way to go. BUT if we can get --experimental-licenses --next-flag to work and make the call analysis flags work like this also I think that would provide the better user experience. I will chat to @another-rex about this tomorrow to see what we can do and report back here.

Thanks for looking into this Oliver!

[oliverchang]

Thanks @josieang !

Getting --experimental-licenses --next-flag perfect might be challenging. there's also the case of --experimental-licenses <dir-to-scan> to consider.

[josieang]

Rex and I talked about this and we've decided to introduce --experimental-licenses-summary as a bool flag. It will always return exit status 0. The reason for this is that in call analysis, 'all' means turn on every language, but here 'all' doesn't mean allowlist every language. A separate flag is clearer.

We also went through some edge cases:
--experimental-licenses="" is invalid (will error)
You can't specify both --experimental-licenses and --experimental-licenses-summary flags. (will error)

We should also verify the list of spdx licenses that they provide against https://spdx.org/licenses/. That will at least make it error in the case of --experimental-licenses --next-flag, as --next-flag is not an spdx license.

[josieang]

I will try send out a PR for it this week.

[oliverchang]

Thanks Josie!

[another-rex]

Fixed in #678