refactor(package-managers): Prefer composition for CommandLineTools

Make more clear that package manager implementation *are* not CLIs, but eventually *use* CLIs. This is a better separation of concerns and reduces the sizes of the main classes. Do the refactoring for all but `Pub` and `Yarn2` for now as these require more work. Note that `CommandLineTool` implementations in plugins can at maximum be `internal` for the `ort requirements` command to work. Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
oss-review-toolkit · Dec 12, 2024 · a88a0f3 · a88a0f3
1 parent 04cd958
commit a88a0f3
Show file tree

Hide file tree

Showing 17 changed files with 261 additions and 236 deletions.
diff --git a/plugins/package-managers/bazel/src/main/kotlin/Bazel.kt b/plugins/package-managers/bazel/src/main/kotlin/Bazel.kt
@@ -74,12 +74,34 @@ private const val LOCKFILE_NAME = "MODULE.bazel.lock"
 private const val BUILDOZER_COMMAND = "buildozer"
 private const val BUILDOZER_MISSING_VALUE = "(missing)"
 
+internal object BazelCommand : CommandLineTool {
+    override fun command(workingDir: File?) = "bazel"
+
+    override fun run(vararg args: CharSequence, workingDir: File?, environment: Map<String, String>): ProcessCapture =
+        super.run(
+            args = args,
+            workingDir,
+            // Disable the optional wrapper script under `tools/bazel` only for the "--version" call.
+            environment + mapOf(
+                "BAZELISK_SKIP_WRAPPER" to "${args[0] == getVersionArguments()}",
+                "USE_BAZEL_FALLBACK_VERSION" to BAZEL_FALLBACK_VERSION
+            )
+        )
+
+    override fun transformVersion(output: String) = output.removePrefix("bazel ")
+
+    // Bazel 6.0 already supports bzlmod but it is not enabled by default.
+    // Supporting it would require adding the flag "--enable_bzlmod=true" at the correct position of all bazel
+    // invocations.
+    override fun getVersionRequirement(): RangesList = RangesListFactory.create(">=7.0")
+}
+
 class Bazel(
     name: String,
     analysisRoot: File,
     analyzerConfig: AnalyzerConfiguration,
     repoConfig: RepositoryConfiguration
-) : PackageManager(name, "Bazel", analysisRoot, analyzerConfig, repoConfig), CommandLineTool {
+) : PackageManager(name, "Bazel", analysisRoot, analyzerConfig, repoConfig) {
     class Factory : AbstractPackageManagerFactory<Bazel>("Bazel") {
         override val globsForDefinitionFiles = listOf("MODULE", "MODULE.bazel")
 
@@ -90,8 +112,6 @@ class Bazel(
         ) = Bazel(type, analysisRoot, analyzerConfig, repoConfig)
     }
 
-    override fun command(workingDir: File?) = "bazel"
-
     /**
      * To avoid processing the module files in a local registry as definition files, ignore them if they are aside a
      * source.json file and under a directory with a metadata.json file. This simple metric avoids parsing the .bazelrc
@@ -106,24 +126,6 @@ class Bazel(
             }
         }
 
-    override fun run(vararg args: CharSequence, workingDir: File?, environment: Map<String, String>): ProcessCapture =
-        super.run(
-            args = args,
-            workingDir = workingDir,
-            // Disable the optional wrapper script under `tools/bazel` only for the "--version" call.
-            environment = environment + mapOf(
-                "BAZELISK_SKIP_WRAPPER" to "${args[0] == getVersionArguments()}",
-                "USE_BAZEL_FALLBACK_VERSION" to BAZEL_FALLBACK_VERSION
-            )
-        )
-
-    override fun transformVersion(output: String) = output.removePrefix("bazel ")
-
-    // Bazel 6.0 already supports bzlmod but it is not enabled by default.
-    // Supporting it would require adding the flag "--enable_bzlmod=true" at the correct position of all bazel
-    // invocations.
-    override fun getVersionRequirement(): RangesList = RangesListFactory.create(">=7.0")
-
     override fun resolveDependencies(definitionFile: File, labels: Map<String, String>): List<ProjectAnalyzerResult> {
         val projectDir = definitionFile.parentFile
         val lockfile = projectDir.resolve(LOCKFILE_NAME)
@@ -388,7 +390,7 @@ class Bazel(
         depDirectives: Map<String, BazelDepDirective>,
         archiveOverrides: Map<String, ArchiveOverride>
     ): Set<Scope> {
-        val process = run(
+        val process = BazelCommand.run(
             "mod",
             "graph",
             "--output",

diff --git a/plugins/package-managers/bower/src/main/kotlin/Bower.kt b/plugins/package-managers/bower/src/main/kotlin/Bower.kt
@@ -46,6 +46,12 @@ import org.ossreviewtoolkit.utils.common.stashDirectories
 import org.semver4j.RangesList
 import org.semver4j.RangesListFactory
 
+internal object BowerCommand : CommandLineTool {
+    override fun command(workingDir: File?) = if (Os.isWindows) "bower.cmd" else "bower"
+
+    override fun getVersionRequirement(): RangesList = RangesListFactory.create(">=1.8.8")
+}
+
 /**
  * The [Bower](https://bower.io/) package manager for JavaScript.
  */
@@ -54,7 +60,7 @@ class Bower(
     analysisRoot: File,
     analyzerConfig: AnalyzerConfiguration,
     repoConfig: RepositoryConfiguration
-) : PackageManager(name, "Bower", analysisRoot, analyzerConfig, repoConfig), CommandLineTool {
+) : PackageManager(name, "Bower", analysisRoot, analyzerConfig, repoConfig) {
     class Factory : AbstractPackageManagerFactory<Bower>("Bower") {
         override val globsForDefinitionFiles = listOf("bower.json")
 
@@ -65,11 +71,7 @@ class Bower(
         ) = Bower(type, analysisRoot, analyzerConfig, repoConfig)
     }
 
-    override fun command(workingDir: File?) = if (Os.isWindows) "bower.cmd" else "bower"
-
-    override fun getVersionRequirement(): RangesList = RangesListFactory.create(">=1.8.8")
-
-    override fun beforeResolution(definitionFiles: List<File>) = checkVersion()
+    override fun beforeResolution(definitionFiles: List<File>) = BowerCommand.checkVersion()
 
     override fun resolveDependencies(definitionFile: File, labels: Map<String, String>): List<ProjectAnalyzerResult> {
         val workingDir = definitionFile.parentFile
@@ -99,8 +101,8 @@ class Bower(
     }
 
     private fun getProjectPackageInfo(workingDir: File): PackageInfo {
-        run(workingDir, "--allow-root", "install").requireSuccess()
-        val json = run(workingDir, "--allow-root", "list", "--json").requireSuccess().stdout
+        BowerCommand.run(workingDir, "--allow-root", "install").requireSuccess()
+        val json = BowerCommand.run(workingDir, "--allow-root", "list", "--json").requireSuccess().stdout
         return parsePackageInfoJson(json)
     }
 }

diff --git a/plugins/package-managers/cargo/src/main/kotlin/Cargo.kt b/plugins/package-managers/cargo/src/main/kotlin/Cargo.kt
@@ -55,6 +55,15 @@ private const val DEFAULT_KIND_NAME = "normal"
 private const val DEV_KIND_NAME = "dev"
 private const val BUILD_KIND_NAME = "build"
 
+internal object CargoCommand : CommandLineTool {
+    override fun command(workingDir: File?) = "cargo"
+
+    override fun transformVersion(output: String) =
+        // The version string can be something like:
+        // cargo 1.35.0 (6f3e9c367 2019-04-04)
+        output.removePrefix("cargo ").substringBefore(' ')
+}
+
 /**
  * The [Cargo](https://doc.rust-lang.org/cargo/) package manager for Rust.
  */
@@ -63,7 +72,7 @@ class Cargo(
     analysisRoot: File,
     analyzerConfig: AnalyzerConfiguration,
     repoConfig: RepositoryConfiguration
-) : PackageManager(name, "Cargo", analysisRoot, analyzerConfig, repoConfig), CommandLineTool {
+) : PackageManager(name, "Cargo", analysisRoot, analyzerConfig, repoConfig) {
     class Factory : AbstractPackageManagerFactory<Cargo>("Cargo") {
         override val globsForDefinitionFiles = listOf("Cargo.toml")
 
@@ -74,13 +83,6 @@ class Cargo(
         ) = Cargo(type, analysisRoot, analyzerConfig, repoConfig)
     }
 
-    override fun command(workingDir: File?) = "cargo"
-
-    override fun transformVersion(output: String) =
-        // The version string can be something like:
-        // cargo 1.35.0 (6f3e9c367 2019-04-04)
-        output.removePrefix("cargo ").substringBefore(' ')
-
     /**
      * Cargo.lock is located next to Cargo.toml or in one of the parent directories. The latter is the case when the
      * project is part of a workspace. Cargo.lock is then located next to the Cargo.toml file defining the workspace.
@@ -129,7 +131,7 @@ class Cargo(
 
     override fun resolveDependencies(definitionFile: File, labels: Map<String, String>): List<ProjectAnalyzerResult> {
         val workingDir = definitionFile.parentFile
-        val metadataProcess = run(workingDir, "metadata", "--format-version=1").requireSuccess()
+        val metadataProcess = CargoCommand.run(workingDir, "metadata", "--format-version=1").requireSuccess()
         val metadata = json.decodeFromString<CargoMetadata>(metadataProcess.stdout)
 
         val projectId = requireNotNull(metadata.resolve.root) {

diff --git a/plugins/package-managers/cocoapods/src/main/kotlin/CocoaPods.kt b/plugins/package-managers/cocoapods/src/main/kotlin/CocoaPods.kt
@@ -52,6 +52,14 @@ import org.ossreviewtoolkit.utils.common.stashDirectories
 import org.semver4j.RangesList
 import org.semver4j.RangesListFactory
 
+internal object CocoaPodsCommand : CommandLineTool {
+    override fun command(workingDir: File?) = if (Os.isWindows) "pod.bat" else "pod"
+
+    override fun getVersionRequirement(): RangesList = RangesListFactory.create(">=1.11.0")
+
+    override fun getVersionArguments() = "--version --allow-root"
+}
+
 /**
  * The [CocoaPods](https://cocoapods.org/) package manager for Objective-C.
  *
@@ -68,7 +76,7 @@ class CocoaPods(
     analysisRoot: File,
     analyzerConfig: AnalyzerConfiguration,
     repoConfig: RepositoryConfiguration
-) : PackageManager(name, "CocoaPods", analysisRoot, analyzerConfig, repoConfig), CommandLineTool {
+) : PackageManager(name, "CocoaPods", analysisRoot, analyzerConfig, repoConfig) {
     class Factory : AbstractPackageManagerFactory<CocoaPods>("CocoaPods") {
         override val globsForDefinitionFiles = listOf("Podfile")
 
@@ -81,18 +89,13 @@ class CocoaPods(
 
     private val podspecCache = mutableMapOf<String, Podspec>()
 
-    override fun command(workingDir: File?) = if (Os.isWindows) "pod.bat" else "pod"
-
-    override fun getVersionRequirement(): RangesList = RangesListFactory.create(">=1.11.0")
-
-    override fun getVersionArguments() = "--version --allow-root"
-
-    override fun beforeResolution(definitionFiles: List<File>) = checkVersion()
+    override fun beforeResolution(definitionFiles: List<File>) = CocoaPodsCommand.checkVersion()
 
     override fun resolveDependencies(definitionFile: File, labels: Map<String, String>): List<ProjectAnalyzerResult> =
         stashDirectories(Os.userHomeDirectory.resolve(".cocoapods/repos")).use {
             // Ensure to use the CDN instead of the monolithic specs repo.
-            run("repo", "add-cdn", "trunk", "https://cdn.cocoapods.org", "--allow-root").requireSuccess()
+            CocoaPodsCommand.run("repo", "add-cdn", "trunk", "https://cdn.cocoapods.org", "--allow-root")
+                .requireSuccess()
 
             try {
                 resolveDependenciesInternal(definitionFile)
@@ -182,7 +185,7 @@ class CocoaPods(
         val podspecName = id.name.substringBefore("/")
 
         val podspecCommand = runCatching {
-            run(
+            CocoaPodsCommand.run(
                 "spec", "which", "^$podspecName$",
                 "--version=${id.version}",
                 "--allow-root",

diff --git a/plugins/package-managers/composer/src/main/kotlin/Composer.kt b/plugins/package-managers/composer/src/main/kotlin/Composer.kt
@@ -59,25 +59,7 @@ private const val SCOPE_NAME_REQUIRE = "require"
 private const val SCOPE_NAME_REQUIRE_DEV = "require-dev"
 private val ALL_SCOPE_NAMES = setOf(SCOPE_NAME_REQUIRE, SCOPE_NAME_REQUIRE_DEV)
 
-/**
- * The [Composer](https://getcomposer.org/) package manager for PHP.
- */
-class Composer(
-    name: String,
-    analysisRoot: File,
-    analyzerConfig: AnalyzerConfiguration,
-    repoConfig: RepositoryConfiguration
-) : PackageManager(name, "Composer", analysisRoot, analyzerConfig, repoConfig), CommandLineTool {
-    class Factory : AbstractPackageManagerFactory<Composer>("Composer") {
-        override val globsForDefinitionFiles = listOf("composer.json")
-
-        override fun create(
-            analysisRoot: File,
-            analyzerConfig: AnalyzerConfiguration,
-            repoConfig: RepositoryConfiguration
-        ) = Composer(type, analysisRoot, analyzerConfig, repoConfig)
-    }
-
+internal object ComposerCommand : CommandLineTool {
     override fun command(workingDir: File?) =
         if (workingDir?.resolve(COMPOSER_PHAR_BINARY)?.isFile == true) {
             "php $COMPOSER_PHAR_BINARY"
@@ -98,6 +80,26 @@ class Composer(
         output.splitOnWhitespace().dropLast(2).last().removeSurrounding("(", ")")
 
     override fun getVersionRequirement(): RangesList = RangesListFactory.create(">=1.5")
+}
+
+/**
+ * The [Composer](https://getcomposer.org/) package manager for PHP.
+ */
+class Composer(
+    name: String,
+    analysisRoot: File,
+    analyzerConfig: AnalyzerConfiguration,
+    repoConfig: RepositoryConfiguration
+) : PackageManager(name, "Composer", analysisRoot, analyzerConfig, repoConfig) {
+    class Factory : AbstractPackageManagerFactory<Composer>("Composer") {
+        override val globsForDefinitionFiles = listOf("composer.json")
+
+        override fun create(
+            analysisRoot: File,
+            analyzerConfig: AnalyzerConfiguration,
+            repoConfig: RepositoryConfiguration
+        ) = Composer(type, analysisRoot, analyzerConfig, repoConfig)
+    }
 
     override fun beforeResolution(definitionFiles: List<File>) {
         // If all directories we are analyzing contain a composer.phar, no global installation of Composer is required
@@ -106,7 +108,7 @@ class Composer(
 
         // We do not actually depend on any features specific to a version of Composer, but we still want to stick to
         // fixed versions to be sure to get consistent results.
-        checkVersion()
+        ComposerCommand.checkVersion()
     }
 
     override fun mapDefinitionFiles(definitionFiles: List<File>): List<File> {
@@ -249,9 +251,9 @@ class Composer(
         if (hasLockfile) return lockfile
 
         // Ensure that the build is not configured to disallow the creation of lockfiles.
-        run(workingDir, "--no-interaction", "config", "--unset", "lock").requireSuccess()
+        ComposerCommand.run(workingDir, "--no-interaction", "config", "--unset", "lock").requireSuccess()
 
-        val composerVersion = Semver(getVersion(workingDir))
+        val composerVersion = Semver(ComposerCommand.getVersion(workingDir))
         val args = buildList {
             add("--no-interaction")
             add("update")
@@ -263,7 +265,7 @@ class Composer(
             }
         }
 
-        run(workingDir, *args.toTypedArray()).requireSuccess()
+        ComposerCommand.run(workingDir, *args.toTypedArray()).requireSuccess()
 
         return lockfile
     }