From 0f941ae832370e79ad801a6e0511ef5937d5fee3 Mon Sep 17 00:00:00 2001
From: Frank Viernau <frank_viernau@epam.com>
Date: Tue, 22 Oct 2024 14:56:29 +0200
Subject: [PATCH 1/2] refactor(npm): Remove unused parallelization constructs
The code looks like it would run things in parallel, but it executes
`getRemotePackageDetails()` strictly sequentially. The pull request
which had introduced these constructs [1] also introduced the caching.
So, the speed-up observed back then was probably solely due to the
added cache. Remove the parallelization constructs to not mislead the
reader and to simplify.
[1]: https://github.com/oss-review-toolkit/ort/pull/6009
Signed-off-by: Frank Viernau <frank_viernau@epam.com>
---
.../node/src/main/kotlin/Npm.kt | 27 +++++--------------
.../main/kotlin/utils/NpmDependencyHandler.kt | 5 +---
2 files changed, 8 insertions(+), 24 deletions(-)
diff --git a/plugins/package-managers/node/src/main/kotlin/Npm.kt b/plugins/package-managers/node/src/main/kotlin/Npm.kt
index ac0b3c27f23a7..c1a34edd5337d 100644
--- a/plugins/package-managers/node/src/main/kotlin/Npm.kt
+++ b/plugins/package-managers/node/src/main/kotlin/Npm.kt
@@ -24,11 +24,6 @@ package org.ossreviewtoolkit.plugins.packagemanagers.node
import java.io.File
import java.util.concurrent.ConcurrentHashMap
-import kotlinx.coroutines.Deferred
-import kotlinx.coroutines.Dispatchers
-import kotlinx.coroutines.async
-import kotlinx.coroutines.withContext
-
import org.apache.logging.log4j.kotlin.logger
import org.ossreviewtoolkit.analyzer.AbstractPackageManagerFactory
@@ -120,7 +115,7 @@ open class Npm(
private val graphBuilder by lazy { DependencyGraphBuilder(NpmDependencyHandler(this)) }
- private val npmViewCache = ConcurrentHashMap<String, Deferred<PackageJson>>()
+ private val npmViewCache = mutableMapOf<String, PackageJson>()
protected open fun hasLockfile(projectDir: File) = NodePackageManager.NPM.hasLockfile(projectDir)
@@ -249,7 +244,7 @@ open class Npm(
* Construct a [Package] by parsing its _package.json_ file and - if applicable - querying additional
* content via the `npm view` command. The result is a [Pair] with the raw identifier and the new package.
*/
- internal suspend fun parsePackage(workingDir: File, packageJsonFile: File): Package {
+ internal fun parsePackage(workingDir: File, packageJsonFile: File): Package {
val packageDir = packageJsonFile.parentFile
logger.debug { "Found a 'package.json' file in '$packageDir'." }
@@ -289,7 +284,7 @@ open class Npm(
if (hasIncompleteData) {
runCatching {
- getRemotePackageDetailsAsync(workingDir, "$rawName@$version").await()
+ getRemotePackageDetails(workingDir, "$rawName@$version")
}.onSuccess { details ->
if (description.isEmpty()) description = details.description.orEmpty()
if (homepageUrl.isEmpty()) homepageUrl = details.homepage.orEmpty()
@@ -342,20 +337,12 @@ open class Npm(
return module
}
- private suspend fun getRemotePackageDetailsAsync(workingDir: File, packageName: String): Deferred<PackageJson> =
- withContext(Dispatchers.IO) {
- npmViewCache.getOrPut(packageName) {
- async {
- getRemotePackageDetails(workingDir, packageName)
- }
- }
+ protected open fun getRemotePackageDetails(workingDir: File, packageName: String): PackageJson =
+ npmViewCache.getOrPut(packageName) {
+ val process = run(workingDir, "info", "--json", packageName)
+ parsePackageJson(process.stdout)
}
- protected open fun getRemotePackageDetails(workingDir: File, packageName: String): PackageJson {
- val process = run(workingDir, "info", "--json", packageName)
- return parsePackageJson(process.stdout)
- }
-
/** Cache for submodules identified by its moduleDir absolutePath */
private val submodulesCache = ConcurrentHashMap<String, Set<File>>()
diff --git a/plugins/package-managers/node/src/main/kotlin/utils/NpmDependencyHandler.kt b/plugins/package-managers/node/src/main/kotlin/utils/NpmDependencyHandler.kt
index a18c62be7086f..af874762b97c4 100644
--- a/plugins/package-managers/node/src/main/kotlin/utils/NpmDependencyHandler.kt
+++ b/plugins/package-managers/node/src/main/kotlin/utils/NpmDependencyHandler.kt
@@ -28,7 +28,6 @@ import org.ossreviewtoolkit.model.PackageLinkage
import org.ossreviewtoolkit.model.Project
import org.ossreviewtoolkit.model.utils.DependencyHandler
import org.ossreviewtoolkit.plugins.packagemanagers.node.Npm
-import org.ossreviewtoolkit.utils.ort.runBlocking
/**
* A data class storing information about a specific NPM module and its dependencies.
@@ -76,7 +75,5 @@ internal class NpmDependencyHandler(private val npm: Npm) : DependencyHandler<Np
PackageLinkage.DYNAMIC.takeUnless { dependency.isProject } ?: PackageLinkage.PROJECT_DYNAMIC
override fun createPackage(dependency: NpmModuleInfo, issues: MutableCollection<Issue>): Package? =
- runBlocking {
- npm.takeUnless { dependency.isProject }?.parsePackage(dependency.workingDir, dependency.packageFile)
- }
+ npm.takeUnless { dependency.isProject }?.parsePackage(dependency.workingDir, dependency.packageFile)
}
From 697903b0a6121f6d76f63de0d77ea59429d97a89 Mon Sep 17 00:00:00 2001
From: Frank Viernau <frank_viernau@epam.com>
Date: Tue, 22 Oct 2024 15:20:09 +0200
Subject: [PATCH 2/2] refactor(npm): Drop a slightly misleading log output
The log message sounds as if the function would have searched the
'package.json' and now found one. However, the function does not search
anything. Simply remove that log output to simplify an upcoming change.
Signed-off-by: Frank Viernau <frank_viernau@epam.com>
---
plugins/package-managers/node/src/main/kotlin/Npm.kt | 4 ----
1 file changed, 4 deletions(-)
diff --git a/plugins/package-managers/node/src/main/kotlin/Npm.kt b/plugins/package-managers/node/src/main/kotlin/Npm.kt
index c1a34edd5337d..e4e479df79a2c 100644
--- a/plugins/package-managers/node/src/main/kotlin/Npm.kt
+++ b/plugins/package-managers/node/src/main/kotlin/Npm.kt
@@ -245,10 +245,6 @@ open class Npm(
* content via the `npm view` command. The result is a [Pair] with the raw identifier and the new package.
*/
internal fun parsePackage(workingDir: File, packageJsonFile: File): Package {
- val packageDir = packageJsonFile.parentFile
-
- logger.debug { "Found a 'package.json' file in '$packageDir'." }
-
val packageJson = parsePackageJson(packageJsonFile)
// The "name" and "version" fields are only required if the package is going to be published, otherwise they are