Several scanner and plugin related refactorings #7572
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mnonnenmacher
requested review from
MarcelBochtler,
oheger-bosch and
a team
as code owners
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7572 +/- ##
=========================================
Coverage 68.03% 68.03%
Complexity 2023 2023
=========================================
Files 344 344
Lines 16727 16727
Branches 2371 2371
=========================================
Hits 11381 11381
Misses 4363 4363
Partials 983 983
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
sschuberth
approved these changes
Sep 26, 2023
| @@ -25,18 +25,6 @@ import org.ossreviewtoolkit.model.config.Options | |||
| import org.semver4j.Semver | |||
| import org.semver4j.Semver.VersionDiff | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Remove the unused `downloaderConfig` parameter from the `create` function of the `ScannerWrapperFactory`. The `downloaderConfig` was required by the old scanner API which was removed in c71628d, because there the code to download the source code was not separated from the scanner plugins. Signed-off-by: Martin Nonnenmacher <[email protected]>
Use a generic name for the factory function as the previous name will not match anymore after an upcoming change. Signed-off-by: Martin Nonnenmacher <[email protected]>
Signed-off-by: Martin Nonnenmacher <[email protected]>
Provide only the scanner specific options to the `ScannerCriteria` factory function. This improves the separation of concerns as the function only has to deal with the relevant options. Provide an empty map as default value for the options to simplify usage in tests. Signed-off-by: Martin Nonnenmacher <[email protected]>
The `ScannerConfigMatcher` was intended to support cases where the `ScannerDetails.configuration` must not be matched exactly. As the feature was not taken into use since its introduction three years ago, replace it with a simpler approach to match either the whole configuration or ignore the configuration to simplify code. Signed-off-by: Martin Nonnenmacher <[email protected]>
Remove the `ScannerCriteria.forDetails()` function as it was only used in tests and usage can be replaced by calling the `create()` function instead. The functionality to create the `maxVersion` based on a `VersionDiff` was never made configurable and was always exclusively used by tests. Signed-off-by: Martin Nonnenmacher <[email protected]>
Add the detected license mapping from the `ScannerConfiguration` to the `ScanContext` to make it available to `ScannerWrapper`s. This is currently only required by the `FossId` implementation because FossID can return arbitrary strings as licenses. These strings can sometimes not be parsed as SPDX expressions and can therefore not be returned as part of a `LicenseFinding` in the `ScanSummary`. A better solution could be to automatically convert all license strings returned by FossID to a form that can be parsed as SPDX expression, then the mapping could be applied globally. However, as this would be a breaking configuration change it is not implemented now and only added as a TODO comment instead. Signed-off-by: Martin Nonnenmacher <[email protected]>
Provide only the scanner specific `Options` instead of the `ScannerConfiguration` to the factory method in `ScannerWrapperFactory`. This simplifies the implementation of the scanners as they do not have to find the relevant configuration on their own, and improves separation of concerns as scanners do not have access to the configuration of other scanners and the global scanner configuration anymore. Signed-off-by: Martin Nonnenmacher <[email protected]>
Extract an interface from the duplicated code in `PackageCurationProviderFactory` and `PackageConfigurationProviderFactory` to make it reusable by other plugins. Signed-off-by: Martin Nonnenmacher <[email protected]>
Merge the `ScannerWrapperFactory` interface and the `AbstractScannerWrapperFactory` abstract class into a single abstract class to simplify code. All implementations already inherited from the abstract class, and the `toString` implementation from the abstract class should also be supported by all scanner wrapper factories. Signed-off-by: Martin Nonnenmacher <[email protected]>
Move the `Options` typealias from model to common-utils to make it available to the plugin API. This allows to increase consistency as many plugin configurations already use the typealias. Signed-off-by: Martin Nonnenmacher <[email protected]>
Use the `Options` typealias in the `PluginManager` and rename the `parseConfig` function of `TypedConfigurablePluginFactory` to `parseOptions` accordingly. Signed-off-by: Martin Nonnenmacher <[email protected]>
Signed-off-by: Martin Nonnenmacher <[email protected]>
mnonnenmacher
force-pushed
the
refactor-scanner
branch
from
3a80f24 to
e626462
Compare
|
Merged as the failing |
sschuberth
reviewed
Sep 27, 2023
| @@ -138,8 +121,9 @@ data class ScannerCriteria( | |||
| 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 | |||
There was a problem hiding this comment.
@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.
|
@mnonnenmacher: please mark this PR as breaking change, as it changes the plugins interface. |
Done, thanks for the reminder. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clean-up the scanner API to reduce complexity and adapt the plugin API to prepare for using the API for configurable plugins for more plugins, especially the scanner wrappers. I planned to include this in the same PR, but as this PR has already become very large I will create another PR afterwards. Please see the commit messages for details.
Breaking API change:
The API for
ScannerWrapper,PackageCurationProvider, andPackageConfigurationProviderplugins has changed. For details see the PR diff.