Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Several scanner and plugin related refactorings #7572

Merged
merged 13 commits into from
Sep 26, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions plugins/commands/scanner/src/main/kotlin/ScannerCommand.kt
Original file line number Diff line number Diff line change
Expand Up @@ -179,10 +179,10 @@ class ScannerCommand : OrtCommand(
): OrtResult {
val packageScannerWrappers = scannerWrapperFactories
.takeIf { PackageType.PACKAGE in packageTypes }.orEmpty()
.map { it.create(ortConfig.scanner, ortConfig.downloader) }
.map { it.create(ortConfig.scanner) }
val projectScannerWrappers = projectScannerWrapperFactories
.takeIf { PackageType.PROJECT in packageTypes }.orEmpty()
.map { it.create(ortConfig.scanner, ortConfig.downloader) }
.map { it.create(ortConfig.scanner) }

if (projectScannerWrappers.isNotEmpty()) {
echo("Scanning projects with:")
Expand Down
8 changes: 4 additions & 4 deletions plugins/scanners/askalono/src/main/kotlin/Askalono.kt
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import org.ossreviewtoolkit.model.LicenseFinding
import org.ossreviewtoolkit.model.ScanSummary
import org.ossreviewtoolkit.model.Severity
import org.ossreviewtoolkit.model.TextLocation
import org.ossreviewtoolkit.model.config.DownloaderConfiguration
import org.ossreviewtoolkit.model.config.ScannerConfiguration
import org.ossreviewtoolkit.scanner.AbstractScannerWrapperFactory
import org.ossreviewtoolkit.scanner.CommandLinePathScannerWrapper
Expand All @@ -52,13 +51,14 @@ class Askalono internal constructor(
private companion object : Logging

class Factory : AbstractScannerWrapperFactory<Askalono>("Askalono") {
override fun create(scannerConfig: ScannerConfiguration, downloaderConfig: DownloaderConfiguration) =
Askalono(type, scannerConfig)
override fun create(scannerConfig: ScannerConfiguration) = Askalono(type, scannerConfig)
}

override val configuration = ""

override val criteria by lazy { ScannerCriteria.fromConfig(details, scannerConfig) }
override val criteria by lazy {
ScannerCriteria.create(details, scannerConfig.options?.get(details.name).orEmpty())
}

override fun command(workingDir: File?) =
listOfNotNull(workingDir, if (Os.isWindows) "askalono.exe" else "askalono").joinToString(File.separator)
Expand Down
8 changes: 4 additions & 4 deletions plugins/scanners/boyterlc/src/main/kotlin/BoyterLc.kt
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import org.ossreviewtoolkit.model.LicenseFinding
import org.ossreviewtoolkit.model.ScanSummary
import org.ossreviewtoolkit.model.Severity
import org.ossreviewtoolkit.model.TextLocation
import org.ossreviewtoolkit.model.config.DownloaderConfiguration
import org.ossreviewtoolkit.model.config.ScannerConfiguration
import org.ossreviewtoolkit.scanner.AbstractScannerWrapperFactory
import org.ossreviewtoolkit.scanner.CommandLinePathScannerWrapper
Expand All @@ -56,13 +55,14 @@ class BoyterLc internal constructor(
}

class Factory : AbstractScannerWrapperFactory<BoyterLc>("BoyterLc") {
override fun create(scannerConfig: ScannerConfiguration, downloaderConfig: DownloaderConfiguration) =
BoyterLc(type, scannerConfig)
override fun create(scannerConfig: ScannerConfiguration) = BoyterLc(type, scannerConfig)
}

override val configuration = CONFIGURATION_OPTIONS.joinToString(" ")

override val criteria by lazy { ScannerCriteria.fromConfig(details, scannerConfig) }
override val criteria by lazy {
ScannerCriteria.create(details, scannerConfig.options?.get(details.name).orEmpty())
}

override fun command(workingDir: File?) =
listOfNotNull(workingDir, if (Os.isWindows) "lc.exe" else "lc").joinToString(File.separator)
Expand Down
3 changes: 1 addition & 2 deletions plugins/scanners/fossid/src/main/kotlin/FossId.kt
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ import org.ossreviewtoolkit.model.ScanSummary
import org.ossreviewtoolkit.model.Severity
import org.ossreviewtoolkit.model.UnknownProvenance
import org.ossreviewtoolkit.model.VcsType
import org.ossreviewtoolkit.model.config.DownloaderConfiguration
import org.ossreviewtoolkit.model.config.Options
import org.ossreviewtoolkit.model.config.ScannerConfiguration
import org.ossreviewtoolkit.model.createAndLogIssue
Expand Down Expand Up @@ -171,7 +170,7 @@ class FossId internal constructor(
}

class Factory : AbstractScannerWrapperFactory<FossId>("FossId") {
override fun create(scannerConfig: ScannerConfiguration, downloaderConfig: DownloaderConfiguration) =
override fun create(scannerConfig: ScannerConfiguration) =
FossId(type, scannerConfig, FossIdConfig.create(scannerConfig))
}

Expand Down
8 changes: 4 additions & 4 deletions plugins/scanners/licensee/src/main/kotlin/Licensee.kt
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import org.ossreviewtoolkit.model.LicenseFinding
import org.ossreviewtoolkit.model.ScanSummary
import org.ossreviewtoolkit.model.Severity
import org.ossreviewtoolkit.model.TextLocation
import org.ossreviewtoolkit.model.config.DownloaderConfiguration
import org.ossreviewtoolkit.model.config.ScannerConfiguration
import org.ossreviewtoolkit.scanner.AbstractScannerWrapperFactory
import org.ossreviewtoolkit.scanner.CommandLinePathScannerWrapper
Expand All @@ -55,13 +54,14 @@ class Licensee internal constructor(
}

class Factory : AbstractScannerWrapperFactory<Licensee>("Licensee") {
override fun create(scannerConfig: ScannerConfiguration, downloaderConfig: DownloaderConfiguration) =
Licensee(type, scannerConfig)
override fun create(scannerConfig: ScannerConfiguration) = Licensee(type, scannerConfig)
}

override val configuration = CONFIGURATION_OPTIONS.joinToString(" ")

override val criteria by lazy { ScannerCriteria.fromConfig(details, scannerConfig) }
override val criteria by lazy {
ScannerCriteria.create(details, scannerConfig.options?.get(details.name).orEmpty())
}

override fun command(workingDir: File?) =
listOfNotNull(workingDir, if (Os.isWindows) "licensee.bat" else "licensee").joinToString(File.separator)
Expand Down
8 changes: 4 additions & 4 deletions plugins/scanners/scancode/src/main/kotlin/ScanCode.kt
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import org.apache.logging.log4j.kotlin.Logging

import org.ossreviewtoolkit.model.ScanSummary
import org.ossreviewtoolkit.model.ScannerDetails
import org.ossreviewtoolkit.model.config.DownloaderConfiguration
import org.ossreviewtoolkit.model.config.ScannerConfiguration
import org.ossreviewtoolkit.scanner.AbstractScannerWrapperFactory
import org.ossreviewtoolkit.scanner.CommandLinePathScannerWrapper
Expand Down Expand Up @@ -95,11 +94,12 @@ class ScanCode internal constructor(
}

class Factory : AbstractScannerWrapperFactory<ScanCode>(SCANNER_NAME) {
override fun create(scannerConfig: ScannerConfiguration, downloaderConfig: DownloaderConfiguration) =
ScanCode(type, scannerConfig)
override fun create(scannerConfig: ScannerConfiguration) = ScanCode(type, scannerConfig)
}

override val criteria by lazy { ScannerCriteria.fromConfig(details, scannerConfig) }
override val criteria by lazy {
ScannerCriteria.create(details, scannerConfig.options?.get(details.name).orEmpty())
}

override val configuration by lazy {
buildList {
Expand Down
8 changes: 4 additions & 4 deletions plugins/scanners/scanoss/src/main/kotlin/ScanOss.kt
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import org.apache.logging.log4j.kotlin.Logging

import org.ossreviewtoolkit.clients.scanoss.ScanOssService
import org.ossreviewtoolkit.model.ScanSummary
import org.ossreviewtoolkit.model.config.DownloaderConfiguration
import org.ossreviewtoolkit.model.config.ScannerConfiguration
import org.ossreviewtoolkit.scanner.AbstractScannerWrapperFactory
import org.ossreviewtoolkit.scanner.PathScannerWrapper
Expand All @@ -57,8 +56,7 @@ class ScanOss internal constructor(
private companion object : Logging

class Factory : AbstractScannerWrapperFactory<ScanOss>("SCANOSS") {
override fun create(scannerConfig: ScannerConfiguration, downloaderConfig: DownloaderConfiguration) =
ScanOss(type, scannerConfig)
override fun create(scannerConfig: ScannerConfiguration) = ScanOss(type, scannerConfig)
}

private val config = ScanOssConfig.create(scannerConfig).also {
Expand All @@ -76,7 +74,9 @@ class ScanOss internal constructor(

override val configuration = ""

override val criteria by lazy { ScannerCriteria.fromConfig(details, scannerConfig) }
override val criteria by lazy {
ScannerCriteria.create(details, scannerConfig.options?.get(details.name).orEmpty())
}

/**
* The name of the file corresponding to the fingerprints can be sent to SCANOSS for more precise matches.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ import org.ossreviewtoolkit.model.SnippetFinding
import org.ossreviewtoolkit.model.TextLocation
import org.ossreviewtoolkit.model.VcsInfo
import org.ossreviewtoolkit.model.VcsType
import org.ossreviewtoolkit.model.config.DownloaderConfiguration
import org.ossreviewtoolkit.model.config.ScannerConfiguration
import org.ossreviewtoolkit.scanner.ScanContext
import org.ossreviewtoolkit.utils.spdx.SpdxExpression
Expand All @@ -65,7 +64,7 @@ class ScanOssScannerDirectoryTest : StringSpec({
server.start()
val scannerOptions = mapOf(ScanOssConfig.API_URL_PROPERTY to "http://localhost:${server.port()}")
val configuration = ScannerConfiguration(options = mapOf("ScanOss" to scannerOptions))
scanner = spyk(ScanOss.Factory().create(configuration, DownloaderConfiguration()))
scanner = spyk(ScanOss.Factory().create(configuration))
}

afterSpec {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import java.util.UUID
import org.ossreviewtoolkit.model.LicenseFinding
import org.ossreviewtoolkit.model.PackageType
import org.ossreviewtoolkit.model.TextLocation
import org.ossreviewtoolkit.model.config.DownloaderConfiguration
import org.ossreviewtoolkit.model.config.ScannerConfiguration
import org.ossreviewtoolkit.scanner.ScanContext

Expand All @@ -58,7 +57,7 @@ class ScanOssScannerFileTest : StringSpec({
server.start()
val scannerOptions = mapOf(ScanOssConfig.API_URL_PROPERTY to "http://localhost:${server.port()}")
val configuration = ScannerConfiguration(options = mapOf("ScanOss" to scannerOptions))
scanner = spyk(ScanOss.Factory().create(configuration, DownloaderConfiguration()))
scanner = spyk(ScanOss.Factory().create(configuration))
}

afterSpec {
Expand Down
50 changes: 16 additions & 34 deletions scanner/src/main/kotlin/ScannerCriteria.kt
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,11 @@
package org.ossreviewtoolkit.scanner

import org.ossreviewtoolkit.model.ScannerDetails
import org.ossreviewtoolkit.model.config.ScannerConfiguration
import org.ossreviewtoolkit.model.config.Options

import org.semver4j.Semver
import org.semver4j.Semver.VersionDiff

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the feature was not taken into use since its introduction three years ago,

Just FYI, this reminds me of @oheger-bosch's #3372, which unfortunately we never managed to merge. But it's probably right by now to start anew and rethink the scanner configuration matching.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I wanted to close the PR after this is merged. I think in the beginning we expected that we would change the ScanCode options more often but over time it turned out that we barely ever change them and the current approach works well enough. So to me the complexity of the proposed solution is not appropriate to the relevance of the problem.

* Definition of a predicate to check whether the configuration of a scanner is compatible with the requirements
* specified by a [ScannerCriteria].
*
* When testing whether a scan result is compatible with specific criteria this function is invoked on the
* scanner configuration data stored in the result. By having different, scanner-specific matcher functions, this
* compatibility check can be made very flexible.
*
* TODO: Switch to a more advanced type than String to represent the scanner configuration.
*/
typealias ScannerConfigMatcher = (String) -> Boolean

/**
* A data class defining selection criteria for scanners.
*
Expand Down Expand Up @@ -69,17 +57,12 @@ data class ScannerCriteria(
val maxVersion: Semver,

/**
* A function to check whether the configuration of a scanner is compatible with this [ScannerCriteria].
* Criterion to match the [configuration][ScannerDetails.configuration] of the scanner. If `null`, all
* configurations are matched.
*/
val configMatcher: ScannerConfigMatcher
val configuration: String?
) {
companion object {
/**
* A matcher for scanner configurations that accepts all configurations passed in. This function can be
* used if the concrete configuration of a scanner is irrelevant.
*/
val ALL_CONFIG_MATCHER: ScannerConfigMatcher = { true }

/**
* The name of the property defining the regular expression for the scanner name as part of [ScannerCriteria].
* The [scanner details][ScannerDetails] of the corresponding scanner must match the criteria.
Expand All @@ -99,10 +82,10 @@ data class ScannerCriteria(
const val PROP_CRITERIA_MAX_VERSION = "maxVersion"

/**
* A matcher for scanner configurations that accepts only exact matches of the [originalConfig]. This
* function can be used by scanners that are extremely sensitive about their configuration.
* The name of the property defining the configuration of the scanner as part of [ScannerCriteria]. The
* [scanner details][ScannerDetails] of the corresponding scanner must match the criteria.
*/
fun exactConfigMatcher(originalConfig: String): ScannerConfigMatcher = { config -> originalConfig == config }
const val PROP_CRITERIA_CONFIGURATION = "configuration"

/**
* Generate a [ScannerCriteria] instance that is compatible with the given [details] and versions that differ
Expand All @@ -122,27 +105,25 @@ data class ScannerCriteria(
regScannerName = details.name,
minVersion = minVersion,
maxVersion = maxVersion,
configMatcher = exactConfigMatcher(details.configuration)
configuration = details.configuration
)
}

/**
* Return a [ScannerCriteria] instance that is to be used when looking up existing scan results from a
* [ScanResultsStorage]. By default, the properties of this instance are initialized to match the scanner
* [details]. These default can be overridden by the [ScannerConfiguration.options] property in the provided
* [config]: Use properties of the form `scannerName.property`, where `scannerName` is the name of the scanner
* the configuration applies to, and `property` is the name of a property of the [ScannerCriteria] class. For
* instance, to specify that a specific minimum version of ScanCode is allowed, set this property:
* `options.ScanCode.minVersion=3.0.2`.
* [details]. These defaults can be overridden by the provided [options]. The keys of the option map must match
* names of the [ScannerCriteria] class. For example, to specify that a specific minimum version of the scanner
* is allowed, set this option: `minVersion=3.0.2`.
*/
fun fromConfig(details: ScannerDetails, config: ScannerConfiguration): ScannerCriteria {
val options = config.options?.get(details.name).orEmpty()
fun create(details: ScannerDetails, options: Options = emptyMap()): ScannerCriteria {
val scannerVersion = Semver(normalizeVersion(details.version))
val minVersion = parseVersion(options[PROP_CRITERIA_MIN_VERSION]) ?: scannerVersion
val maxVersion = parseVersion(options[PROP_CRITERIA_MAX_VERSION]) ?: minVersion.nextMinor()
val name = options[PROP_CRITERIA_NAME] ?: details.name
val configuration = options[PROP_CRITERIA_CONFIGURATION] ?: details.configuration
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mnonnenmacher, I just realized a general problem with this: ScannerCriteria.configuration is nullable to allow matching all configurations, but we can never set it to null via a scanner's Options as this will then fall back to ScannerDetails.configuration which is non-nullable.


return ScannerCriteria(name, minVersion, maxVersion, exactConfigMatcher(details.configuration))
return ScannerCriteria(name, minVersion, maxVersion, configuration)
}
}

Expand All @@ -163,7 +144,8 @@ data class ScannerCriteria(
if (!nameRegex.matches(details.name)) return false

val version = Semver(details.version)
return minVersion <= version && version < maxVersion && configMatcher(details.configuration)
return minVersion <= version && version < maxVersion &&
(configuration == null || configuration == details.configuration)
}
}

Expand Down
7 changes: 3 additions & 4 deletions scanner/src/main/kotlin/ScannerWrapperFactory.kt
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ package org.ossreviewtoolkit.scanner

import java.util.ServiceLoader

import org.ossreviewtoolkit.model.config.DownloaderConfiguration
import org.ossreviewtoolkit.model.config.ScannerConfiguration
import org.ossreviewtoolkit.utils.common.Plugin

Expand All @@ -30,9 +29,9 @@ import org.ossreviewtoolkit.utils.common.Plugin
*/
interface ScannerWrapperFactory : Plugin {
/**
* Create a [ScannerWrapper] using the specified [scannerConfig] and [downloaderConfig].
* Create a [ScannerWrapper] using the specified [scannerConfig].
*/
fun create(scannerConfig: ScannerConfiguration, downloaderConfig: DownloaderConfiguration): ScannerWrapper
fun create(scannerConfig: ScannerConfiguration): ScannerWrapper
}

/**
Expand All @@ -41,7 +40,7 @@ interface ScannerWrapperFactory : Plugin {
abstract class AbstractScannerWrapperFactory<out T : ScannerWrapper>(
override val type: String
) : ScannerWrapperFactory {
abstract override fun create(scannerConfig: ScannerConfiguration, downloaderConfig: DownloaderConfiguration): T
abstract override fun create(scannerConfig: ScannerConfiguration): T

/**
* Return the scanner wrapper's name here to allow Clikt to display something meaningful when listing the scanners
Expand Down
3 changes: 1 addition & 2 deletions scanner/src/main/kotlin/storages/ClearlyDefinedStorage.kt
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ import org.ossreviewtoolkit.model.UnknownProvenance
import org.ossreviewtoolkit.model.VcsInfo
import org.ossreviewtoolkit.model.VcsType
import org.ossreviewtoolkit.model.config.ClearlyDefinedStorageConfiguration
import org.ossreviewtoolkit.model.config.DownloaderConfiguration
import org.ossreviewtoolkit.model.config.ScannerConfiguration
import org.ossreviewtoolkit.model.jsonMapper
import org.ossreviewtoolkit.model.utils.toClearlyDefinedCoordinates
Expand Down Expand Up @@ -126,7 +125,7 @@ class ClearlyDefinedStorage(
val supportedScanners = toolVersionsByName.mapNotNull { (name, versions) ->
// For the ClearlyDefined tool names see https://github.com/clearlydefined/service#tool-name-registry.
ScannerWrapper.ALL[name]?.let { factory ->
val scanner = factory.create(ScannerConfiguration(), DownloaderConfiguration())
val scanner = factory.create(ScannerConfiguration())
(scanner as? CommandLinePathScannerWrapper)?.let { cliScanner -> cliScanner to versions.last() }
}.also {
if (it == null) logger.debug { "Unsupported tool '$name' for coordinates '$coordinates'." }
Expand Down
Loading