evaluator: Introduce the option to evaluate a licenseRule for all license sources at once #11154
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -202,33 +202,44 @@ open class PackageRule( | |
| /** | ||
| * A DSL function to configure a [LicenseRule] and add it to this rule. | ||
| */ | ||
| fun licenseRule( | ||
| name: String, | ||
| licenseView: LicenseView, | ||
| separateEvaluationPerSource: Boolean = true, | ||
|
Member
There was a problem hiding this comment. Do we even need this configurability? Why not make the new the default, and require rule users to manually create multiple violations from this rule (one violation per source) if needed?
Member
Author
There was a problem hiding this comment. This is needed for:
|
||
| block: LicenseRule.() -> Unit | ||
| ) { | ||
| val effectiveResolvedLicenseInfo = resolvedLicenseInfo.filter(licenseView, filterSources = true) | ||
| .applyChoices(ruleSet.ortResult.getPackageLicenseChoices(pkg.metadata.id), licenseView) | ||
| .applyChoices(ruleSet.ortResult.getRepositoryLicenseChoices(), licenseView) | ||
|
|
||
| effectiveResolvedLicenseInfo.forEach { resolvedLicense -> | ||
| if (separateEvaluationPerSource) { | ||
| resolvedLicense.sources.forEach { licenseSource -> | ||
| licenseRules += LicenseRule(name, resolvedLicense, setOf(licenseSource)).apply(block) | ||
| } | ||
| } else { | ||
| licenseRules += LicenseRule(name, resolvedLicense, resolvedLicense.sources).apply(block) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun issue(severity: Severity, message: String, howToFix: String) = | ||
| issue(severity, pkg.metadata.id, null, emptySet(), message, howToFix) | ||
|
|
||
| /** | ||
| * Add a [hint][Severity.HINT] to the list of [violations]. | ||
| */ | ||
| fun hint(message: String, howToFix: String) = hint(pkg.metadata.id, null, emptySet(), message, howToFix) | ||
|
|
||
| /** | ||
| * Add a [warning][Severity.WARNING] to the list of [violations]. | ||
| */ | ||
| fun warning(message: String, howToFix: String) = warning(pkg.metadata.id, null, emptySet(), message, howToFix) | ||
|
|
||
| /** | ||
| * Add an [error][Severity.ERROR] to the list of [violations]. | ||
| */ | ||
| fun error(message: String, howToFix: String) = error(pkg.metadata.id, null, emptySet(), message, howToFix) | ||
|
|
||
| /** | ||
| * A [Rule] to check a single license of the [package][pkg]. | ||
|
|
@@ -242,10 +253,31 @@ open class PackageRule( | |
| val resolvedLicense: ResolvedLicense, | ||
|
|
||
| /** | ||
| * The license sources to evaluate the license for. Must not be empty and contained in the [resolvedLicense]. | ||
|
Member
There was a problem hiding this comment. Nit: I'm unsure about the "to evaluate the license for" part. No one forces the rule to actually look at / evaluate the license sources. I think it should rather say "The sources where this particular license was found".
Member
Author
There was a problem hiding this comment. I believe "The sources where this particular license was found" is mis-leading:
Does this make more sense now?
Member
There was a problem hiding this comment. I'm still confused about the difference / different purpose of So, why / under what circumstances can |
||
| */ | ||
| val licenseSources: Set<LicenseSource> | ||
| ) : Rule(ruleSet, name) { | ||
| init { | ||
| require(licenseSources.isNotEmpty()) { | ||
| "The given license sources must not be empty." | ||
| } | ||
|
|
||
| val invalidLicenseSources = (licenseSources - resolvedLicense.sources) | ||
| require(invalidLicenseSources.isEmpty()) { | ||
| "The license sources $invalidLicenseSources are not part of the resolved license." | ||
| } | ||
| } | ||
|
|
||
| /** Backwards compatibility */ | ||
|
Member
There was a problem hiding this comment. Personally, I would be fine with omitting this and having a breaking change, and clearly documenting the migration path as part of the release notes.
Member
Author
There was a problem hiding this comment. Do you mean only this property, or the entire commit? (Would you also be ok, with marking it as deprected now, and removing it later? This would allow a smooth transition outside of the critical path of upgrading ORT.)
Member
There was a problem hiding this comment.
I was referring to the entire commit.
Yes. I was just trying to say that this additional work would not be required from my side.
Member
Author
There was a problem hiding this comment. I ended up not deprecating this property, because the separate per source evaluation / single source per license rule or violation is still a valid (and in my opinion even the preferred) use case. In that case using that property is a bit simpler.
Member
There was a problem hiding this comment. Agree with @fviernau we don't want to get rid of per license source evaluator per rule we just want to offer ORT user more power/flexibility to define rules that match their organization's open source policies. |
||
| @Suppress("unused") // This is intended to be used by rule implementations. | ||
| val licenseSource by lazy { | ||
|
||
| require(licenseSources.size == 1) { | ||
| "The license source is ambiguous. Please use the licenseSources property instead." | ||
| } | ||
|
|
||
| licenseSources.single() | ||
| } | ||
|
|
||
| /** | ||
| * A shortcut for the [license][ResolvedLicense.license] in [resolvedLicense]. | ||
| */ | ||
|
|
@@ -257,11 +289,11 @@ open class PackageRule( | |
| */ | ||
| fun pkg() = pkg | ||
|
|
||
| override val description = "\tEvaluating license rule '$name' for $licenseSources license " + | ||
| "'${resolvedLicense.license}'." | ||
|
|
||
| override fun issueSource() = | ||
| "$name - ${pkg.metadata.id.toCoordinates()} - ${resolvedLicense.license} ($licenseSources})" | ||
|
|
||
| /** | ||
| * A [RuleMatcher] that checks if a [detected][LicenseSource.DETECTED] license is | ||
|
|
@@ -271,7 +303,8 @@ open class PackageRule( | |
| object : RuleMatcher { | ||
| override val description = "isDetectedExcluded($license)" | ||
|
|
||
| override fun matches() = | ||
| licenseSources.singleOrNull() == LicenseSource.DETECTED && resolvedLicense.isDetectedExcluded | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -290,22 +323,23 @@ open class PackageRule( | |
| } | ||
|
|
||
| fun issue(severity: Severity, message: String, howToFix: String) = | ||
| issue(severity, pkg.metadata.id, license, licenseSources, message, howToFix) | ||
|
|
||
| /** | ||
| * Add a [hint][Severity.HINT] to the list of [violations]. | ||
| */ | ||
| fun hint(message: String, howToFix: String) = hint(pkg.metadata.id, license, licenseSources, message, howToFix) | ||
|
|
||
| /** | ||
| * Add a [warning][Severity.WARNING] to the list of [violations]. | ||
| */ | ||
| fun warning(message: String, howToFix: String) = | ||
| warning(pkg.metadata.id, license, licenseSources, message, howToFix) | ||
|
|
||
| /** | ||
| * Add an [error][Severity.ERROR] to the list of [violations]. | ||
| */ | ||
| fun error(message: String, howToFix: String) = | ||
| error(pkg.metadata.id, license, licenseSources, message, howToFix) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit message nit: "for all sources"