Apply license choices specified for a package defined in the repository configuration file #3713

MarcelBochtler · 2021-03-03T14:15:00Z

Apply license choices specified for a package defined in the repository configuration file.

The choices are used in the licenseRule and the StaticHtmlReporter and applied in the effectiveLicense function of ResolveLicenseInfo.

Related to #3396.

CC: @neubs-bsi

model/src/main/kotlin/config/LicenseChoices.kt

docs/config-file-ort-yml.md

oheger-bosch

Some minor remarks. Thumbs-up for the thorough testing!

oheger-bosch · 2021-03-12T06:07:37Z

spdx-utils/src/test/kotlin/SpdxExpressionTest.kt

+                val result = expression.applyChoices(choices)
+
+                result shouldBe "b OR d".toSpdx()
+            }


Could we have a test case where a single choice can be applied multiple times? E.g. we have something like (a OR b) AND (c OR d) AND (a OR e). When applying the choice a to the whole expression I assume the result should be a AND (c OR d) AND a, and this can probably be simplified to a AND (c OR d). Is this assumption correct and should this be tested as well?

Your assumption is incorrect.
a is not a valid choice for (a OR b) AND (c OR d) AND (a OR e), because the choice is not just a OR b but always need to take the licenses from the AND expressions into consideration.

To get your wanted result the user would need to configure something like this:

- given: a OR b choice: a - given: a OR e choice: a

I added a test for this.

model/src/main/kotlin/licenses/ResolvedLicenseInfo.kt

model/src/main/kotlin/config/RepositoryConfiguration.kt

model/src/test/kotlin/config/RepositoryConfigurationTest.kt

spdx-utils/src/main/kotlin/model/LicenseChoice.kt

model/src/main/kotlin/config/RepositoryConfiguration.kt

spdx-utils/src/main/kotlin/SpdxExpression.kt

fviernau · 2021-03-15T10:13:31Z

evaluator/src/main/kotlin/PackageRule.kt

@@ -135,7 +135,8 @@ open class PackageRule(
     * A DSL function to configure a [LicenseRule] and add it to this rule.
     */
    fun licenseRule(name: String, licenseView: LicenseView, block: LicenseRule.() -> Unit) {
-        resolvedLicenseInfo.filter(licenseView, filterSources = true).forEach { resolvedLicense ->
+        resolvedLicenseInfo.filter(licenseView, filterSources = true)
+            .filter(ruleSet.ortResult.getLicenseChoices(pkg.id)).forEach { resolvedLicense ->


Could this impact the performance of the evaluator for larger inputs?

We don't think that this impacts the performance of the evaluator.
We renamed the function from filter to applyLicense as the functionality is different from the existing filters.

The peformance impact is basically to build the effective license and decompose it again which will lead to n additional loops for n Single SPDX licenses and if required to apply the choices.

If its clear that there is not performance impact fine, otherwise it'd make sense IMO to run a quick test with a large project.

fviernau · 2021-03-15T10:19:54Z

model/src/main/kotlin/OrtResult.kt

+    /**
+     * Return all [LicenseChoice]s for the [Package] with [id].
+     */
+    fun getLicenseChoices(id: Identifier): List<LicenseChoice> =


I assumed that in future we'll have license choices also defined elsewhere, so the license choices do not only come from the OrtResult. This is why I looked up how path excludes and license finding curations are retrieved.

I found that they get injected into DefaultLicenseInfoProvider to be used by the LicenseInfoResolver.
Have you considered processing license choices in a similar / analog way?

Would it be ok for you to refactor this when other license choice configurations are added?

I'd actually like to also have @mnonnenmacher opinion on that as he has written the parts relevant here before diving deeper into this. I fear that it could be quite a bit of tech debt, but let's see.

I agree, the design idea is that the LicenseInfoResolver gets all required information from the LicenseInfo and therefore it would make sense to also add the choices there. But I would be fine with changing this separately, because I currently cannot decide which would be the best way to integrate the choices there.

fviernau · 2021-03-15T10:29:50Z

model/src/main/kotlin/licenses/ResolvedLicenseInfo.kt

@@ -65,6 +66,23 @@ data class ResolvedLicenseInfo(
 ) : Iterable<ResolvedLicense> by licenses {
    operator fun get(license: SpdxSingleLicenseExpression): ResolvedLicense? = find { it.license == license }

+    /**


IIUC this resolved license info class has all configuration relevant for computing the needed information in its attributes. So, what's the reason for not adding an attribute for the applicable license choices to the data model?

For ResolvedLicenseLocation there is appliedCuration and matchingPathExcludes there is the curations and path excludes already applied, and additionally there is the properties for what was applied. The analog thing for choices would be that choices are already applied and there is an applied choices property?

Note: I'm haven't myself yet fully understood this area of the code, so I rather want to get the discussion going, but don't intent to say it's wrong as-is.

Yes we thought about this.
But the license choice should only be applied to the licenses used in the evaluator and the reporter, and therefore make use of the specified LicenseViews.

This implementation decision was a direct result of a discussion in the developers meeting on 2021-03-04, we developed it in pair programming with @mnonnenmacher.

Exactly, the problem is that the license choice can only be applied if the sub-expression matches, and if it does can depend on the used LicenseView. So the available license choices could be provided by the LicenseInfo as mentioned in another comment, but they can only be applied in combination with a LicenseView, for example in the evaluator rules.

mnonnenmacher

Please also rebase you PR to make the new static analysis checks work.

model/src/test/kotlin/licenses/ResolvedLicenseInfoTest.kt

evaluator/src/test/kotlin/RuleSetTest.kt

docs/config-file-ort-yml.md

This specifies the structure for license choices by packages in the repository configuration. Signed-off-by: Marcel Bochtler <marcel.bochtler@bosch.io>

Signed-off-by: Marcel Bochtler <marcel.bochtler@bosch.io>

The effective license filters all licenses from all sources by the LicenseView with the LicenseChoices applied. Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch.io>

For later use. Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch.io>

The new filter applies the license choices from the ORT result to all licenses in the ResolvedLicenseInfo when using a licenseRule. Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch.io>

Signed-off-by: Marcel Bochtler <marcel.bochtler@bosch.io>

Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch.io>

Comments addressed.

MarcelBochtler requested a review from oheger-bosch March 3, 2021 14:15

MarcelBochtler requested review from fviernau, mnonnenmacher, sschuberth and tsteenbe as code owners March 3, 2021 14:15

MarcelBochtler assigned mnonnenmacher Mar 3, 2021

MarcelBochtler mentioned this pull request Mar 3, 2021

Add functionality to handle license choices #3396

Closed

oheger-bosch reviewed Mar 4, 2021

View reviewed changes

model/src/main/kotlin/config/LicenseChoices.kt Outdated Show resolved Hide resolved

model/src/main/kotlin/config/LicenseChoices.kt Outdated Show resolved Hide resolved

docs/config-file-ort-yml.md Outdated Show resolved Hide resolved

docs/config-file-ort-yml.md Outdated Show resolved Hide resolved

MarcelBochtler force-pushed the license-choice branch 2 times, most recently from e252711 to b81fd6c Compare March 4, 2021 08:20

MarcelBochtler requested a review from oheger-bosch March 4, 2021 08:20

oheger-bosch previously approved these changes Mar 4, 2021

View reviewed changes

MarcelBochtler marked this pull request as draft March 4, 2021 10:31

MarcelBochtler dismissed oheger-bosch’s stale review via bdd054c March 9, 2021 12:53

MarcelBochtler force-pushed the license-choice branch 2 times, most recently from bdd054c to 59722ad Compare March 11, 2021 09:00

neubs-bsi force-pushed the license-choice branch from 59722ad to 688e2f6 Compare March 11, 2021 13:39

MarcelBochtler force-pushed the license-choice branch 2 times, most recently from 8e67f11 to 2b12afa Compare March 11, 2021 14:25

MarcelBochtler marked this pull request as ready for review March 11, 2021 14:32

MarcelBochtler requested a review from oheger-bosch March 11, 2021 14:33

MarcelBochtler force-pushed the license-choice branch from 2b12afa to 62ba93f Compare March 11, 2021 14:57

MarcelBochtler marked this pull request as draft March 11, 2021 15:51

oheger-bosch reviewed Mar 12, 2021

View reviewed changes

MarcelBochtler force-pushed the license-choice branch 5 times, most recently from a7910fb to a4d08f0 Compare March 15, 2021 10:29

fviernau previously requested changes Mar 15, 2021

View reviewed changes

MarcelBochtler force-pushed the license-choice branch 4 times, most recently from 6fd7c65 to 4cddcb1 Compare March 16, 2021 13:53

MarcelBochtler requested review from fviernau and oheger-bosch March 16, 2021 13:54

MarcelBochtler marked this pull request as ready for review March 16, 2021 15:17

MarcelBochtler marked this pull request as draft March 16, 2021 15:48

MarcelBochtler force-pushed the license-choice branch 2 times, most recently from 3d6b543 to 294245e Compare March 17, 2021 08:47

MarcelBochtler marked this pull request as ready for review March 17, 2021 08:49

mnonnenmacher requested changes Mar 19, 2021

View reviewed changes

model/src/test/kotlin/licenses/ResolvedLicenseInfoTest.kt Outdated Show resolved Hide resolved

evaluator/src/test/kotlin/RuleSetTest.kt Outdated Show resolved Hide resolved

docs/config-file-ort-yml.md Outdated Show resolved Hide resolved

MarcelBochtler and others added 3 commits March 19, 2021 21:13

RepositoryConfiguration: Add data model for license choice

711179d

This specifies the structure for license choices by packages in the repository configuration. Signed-off-by: Marcel Bochtler <marcel.bochtler@bosch.io>

SpdxExpression: Add functionality to apply multiple license choices

cdcec0d

Signed-off-by: Marcel Bochtler <marcel.bochtler@bosch.io>

ResolvedLicenseInfo: Add effective license function

e004460

The effective license filters all licenses from all sources by the LicenseView with the LicenseChoices applied. Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch.io>

MarcelBochtler force-pushed the license-choice branch from 294245e to 00a9507 Compare March 19, 2021 20:21

neubs-bsi and others added 5 commits March 19, 2021 21:23

ResolvedLicenseInfo: add applyChoices function for license choices

0f5de06

For later use. Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch.io>

PackageRule: Add filtering by licenseChoices to licenseRule

fbd6ba7

The new filter applies the license choices from the ORT result to all licenses in the ResolvedLicenseInfo when using a licenseRule. Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch.io>

StaticHtmlReporter: Add effective license

ffad00d

Signed-off-by: Marcel Bochtler <marcel.bochtler@bosch.io>

LicenseChoice: Add init function to check valid choice

ddff361

Signed-off-by: Marcel Bochtler <marcel.bochtler@bosch.io>

config-file-ort-yml: Add documentation for license choices

9a0f405

Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch.io>

MarcelBochtler force-pushed the license-choice branch from 00a9507 to 9a0f405 Compare March 19, 2021 20:35

MarcelBochtler requested a review from mnonnenmacher March 19, 2021 20:35

mnonnenmacher approved these changes Mar 22, 2021

View reviewed changes

MarcelBochtler merged commit b43f388 into oss-review-toolkit:master Mar 22, 2021

MarcelBochtler deleted the license-choice branch March 22, 2021 10:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply license choices specified for a package defined in the repository configuration file #3713

Apply license choices specified for a package defined in the repository configuration file #3713

MarcelBochtler commented Mar 3, 2021 •

edited

Loading

oheger-bosch left a comment

oheger-bosch Mar 12, 2021

MarcelBochtler Mar 15, 2021

fviernau Mar 15, 2021

MarcelBochtler Mar 16, 2021

fviernau Mar 16, 2021

fviernau Mar 15, 2021

MarcelBochtler Mar 16, 2021

fviernau Mar 16, 2021

mnonnenmacher Mar 19, 2021

fviernau Mar 15, 2021

MarcelBochtler Mar 16, 2021

mnonnenmacher Mar 19, 2021

mnonnenmacher left a comment

Apply license choices specified for a package defined in the repository configuration file #3713

Apply license choices specified for a package defined in the repository configuration file #3713

Conversation

MarcelBochtler commented Mar 3, 2021 • edited Loading

oheger-bosch left a 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

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

mnonnenmacher left a comment

Choose a reason for hiding this comment

MarcelBochtler commented Mar 3, 2021 •

edited

Loading