Replace / by OR in the license field of Rust crates #2990

gferon · 2020-08-26T15:11:55Z

cargo supports arbitrary formats to specify multiple licenses, but / should be interpreted as OR.

It looks like the consensus is that crates.io should only accept SPDX 2.1 license expressions, but this is not the reality for all crates as it's not validated.

Related issue in cargo: rust-lang/cargo#2039

sschuberth · 2020-08-26T15:34:56Z

Thanks for this @gferon, but can you please put the text from your comment in the commit message body?

gferon · 2020-08-26T16:14:50Z

Thanks for this @gferon, but can you please put the text from your comment in the commit message body?

done. it looks like there's an unrelated test failure in CI (in the Sbt analyzer).

sschuberth · 2020-08-26T16:46:30Z

analyzer/src/main/kotlin/managers/Cargo.kt

-            .toSortedSet()
+        sortedSetOf<String>().apply {
+            add(node["license"].textValueOrEmpty().replace("/", " OR ").trim())
+        }


This could theoretically be written as

sortedSetOf(node["license"].textValueOrEmpty().replace("/", " OR ").trim())

but I actually liked the trimming of the individual elements in the original code.

Anyway, I believe we have a more fundamental API problem here: The return value of this function is used to populate the declaredLicenses field, and that's defined as val declaredLicenses: SortedSet<String>. Strictly speaking we somewhat have the same problem with the individual entries in the set: It's unclear whether set entries have an AND or OR relation among each other.

IIRC we deliberately did not want to chose any relation here, but declaredLicenses should literally take whatever string is used in the package manager's meta data. And as some package managers have multiple license fields, declaredLicenses is a collection / set instead of a single string.

Long story short, I'm not sure if even the original split by / was correct, as that's already not literally taking the meta data. I believe this needs some discussion.

Well, what is there to discuss? I just read the whole story and in the Rust ecosystem the / was used as a OR in a SPDX license expression, which is what my commit tries to address.

If you believe the toolkit should get the raw value, and it can handle it, I can change it so we don't process anything here. In the meantime, I'd argue that it's better to cover more cases (correctly) over anything else.

Well, what is there to discuss?

Put very briefly, whether a string in the declaredLicenses set should be allowed to be an SPDX expression or not. There was a time when we treated every single string as a single license, i.e. something like "Apache-2.0 OR MIT" was interpreted as a literal single license name, not as a license expression. But that should indeed be handled now by the introduction of declaredLicensesProcessed.

It's unclear whether set entries have an AND or OR relation among each other.

Actually, looking again at how we generate declaredLicensesProcessed, I realize we do relate set entries always with AND.

To conclude my thinking-out-aloud: I'd be fine with the intent of this change, but I'd implement it as

private fun extractDeclaredLicenses(node: JsonNode) = sortedSetOf( node["license"].textValueOrEmpty().split('/') .map { it.trim() } .filter { it.isNotEmpty() } .joinToString(" OR ") )

Let's see what others say.

Basically I agree with interpreting the / as or.
I wonder how the and operator looks like in Rust - any idea?

@fviernau it doesn't exist, which is why the recommendation was to start using SPDX expressions instead.

@sschuberth I have applied your suggestion.

sschuberth · 2020-08-27T09:47:38Z

Looks like we have test failures as probably expected results now need to be updated. Could you please amend you commit with the necessary changes, @gferon?

sschuberth · 2020-08-27T10:51:29Z

Thanks for the update @gferon, but please squash your commits as we aim for each commit in a PR to pass tests on its own.

gferon · 2020-08-27T11:01:49Z

Thanks for the update @gferon, but please squash your commits as we aim for each commit in a PR to pass tests on its own.

why not switch the contribution model to squash on merge then?

EDIT: done

sschuberth · 2020-08-27T11:05:33Z

why not switch the contribution model to squash on merge then?

Because we do want to keep individual commits in PRs and the final history (as long as they pass tests individually) for better / more fine grained traceability of the development process, and to improve running tools like git bisect.

sschuberth · 2020-08-27T11:08:06Z

EDIT: done

Well, you of course need to adjust your commit message then to not contain two sign-offs... that is, just remove all of the second's commit message, and stick with only the message of the first commit.

And while at it, please also prefix the commit message with "Cargo: " to highlight the scope of modified files.

gferon · 2020-08-31T13:59:26Z

@sschuberth I just updated the test files, they should pass now. I also had to change the code a little, feel free to suggest something else, I don't really know Kotlin.

sschuberth · 2020-08-31T14:15:56Z

analyzer/src/main/kotlin/managers/Cargo.kt

            .map { it.trim() }
            .filter { it.isNotEmpty() }
-            .toSortedSet()
+
+        return if(licenses.isEmpty()) sortedSetOf<String>() else sortedSetOf(licenses.joinToString(" OR "))


This should be (note the space after if, and the removed explicit type):

if (licenses.isEmpty()) sortedSetOf() else sortedSetOf(licenses.joinToString(" OR "))

Is the case distinction actually needed, e.g. why not just sortedSetOf(licenses.joinToString(" OR "))?

Because that would create a SortedSet containing an empty string if licenses is empty, and not the wanted empty set.

Oh right, fully missed that.

if (licenses.isEmpty()) sortedSetOf() else sortedSetOf(licenses.joinToString(" OR "))

When I tried that, it didn't compile, I can try again, though.

I also just tried, for me it compiles fine without the explicit type.

fviernau · 2020-08-31T14:50:08Z

analyzer/src/main/kotlin/managers/Cargo.kt

            .map { it.trim() }
            .filter { it.isNotEmpty() }
-            .toSortedSet()
+
+        return if(licenses.isEmpty()) sortedSetOf<String>() else sortedSetOf(licenses.joinToString(" OR "))


Is the case distinction actually needed, e.g. why not just sortedSetOf(licenses.joinToString(" OR "))?

cargo supports the legacy '/' format to specify multiple licenses and the consensus is that crates.io should only accept SPDX 2.1 license expressions, but this is not enforced as of yet. see: rust-lang/cargo#2039 Signed-off-by: Gabriel Féron <gabriel.feron@here.com>

gferon · 2020-09-01T08:21:30Z

took a while, but tests are finally passing 🎈

Misunderstandng clarified.

gferon requested review from fviernau, mnonnenmacher and sschuberth as code owners August 26, 2020 15:11

sschuberth reviewed Aug 26, 2020

View reviewed changes

sschuberth previously approved these changes Aug 27, 2020

View reviewed changes

gferon dismissed sschuberth’s stale review via 5752165 August 27, 2020 10:47

gferon requested a review from sschuberth August 27, 2020 11:03

sschuberth requested changes Aug 31, 2020

View reviewed changes

fviernau previously requested changes Aug 31, 2020

View reviewed changes

sschuberth approved these changes Sep 1, 2020

View reviewed changes

sschuberth merged commit c343abe into oss-review-toolkit:master Sep 1, 2020

gferon deleted the rust-spdx-or branch September 1, 2020 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace / by OR in the license field of Rust crates #2990

Replace / by OR in the license field of Rust crates #2990

gferon commented Aug 26, 2020

sschuberth commented Aug 26, 2020

gferon commented Aug 26, 2020

sschuberth Aug 26, 2020 •

edited

Loading

gferon Aug 26, 2020

sschuberth Aug 26, 2020 •

edited

Loading

sschuberth Aug 26, 2020

fviernau Aug 27, 2020

gferon Aug 27, 2020

gferon Aug 27, 2020

sschuberth commented Aug 27, 2020

sschuberth commented Aug 27, 2020

gferon commented Aug 27, 2020 •

edited

Loading

sschuberth commented Aug 27, 2020

sschuberth commented Aug 27, 2020

gferon commented Aug 31, 2020

sschuberth Aug 31, 2020

fviernau Aug 31, 2020

sschuberth Aug 31, 2020

fviernau Aug 31, 2020

gferon Aug 31, 2020

sschuberth Aug 31, 2020

gferon Aug 31, 2020

fviernau Aug 31, 2020

gferon commented Sep 1, 2020

Replace / by OR in the license field of Rust crates #2990

Replace / by OR in the license field of Rust crates #2990

Conversation

gferon commented Aug 26, 2020

sschuberth commented Aug 26, 2020

gferon commented Aug 26, 2020

sschuberth Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth commented Aug 27, 2020

sschuberth commented Aug 27, 2020

gferon commented Aug 27, 2020 • edited Loading

sschuberth commented Aug 27, 2020

sschuberth commented Aug 27, 2020

gferon commented Aug 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gferon commented Sep 1, 2020

sschuberth Aug 26, 2020 •

edited

Loading

sschuberth Aug 26, 2020 •

edited

Loading

gferon commented Aug 27, 2020 •

edited

Loading