Skip to content

Make packagemanager PNPM configurable #11200

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.

Sign up for GitHub

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

Open
thomasscheer wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Make packagemanager PNPM configurable #11200

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0044d77
spdx-utils: Make PNPM package manager configurable
thomasscheer Dec 8, 2025
ac51bc0
Merge branch 'oss-review-toolkit:main' into main
thomasscheer Dec 8, 2025
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
30 changes: 27 additions & 3 deletions plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.ossreviewtoolkit.model.config.Excludes
import org.ossreviewtoolkit.model.utils.DependencyGraphBuilder
import org.ossreviewtoolkit.plugins.api.OrtPlugin
import org.ossreviewtoolkit.plugins.api.OrtPluginOption
import org.ossreviewtoolkit.plugins.api.PluginDescriptor
import org.ossreviewtoolkit.plugins.packagemanagers.node.ModuleInfoResolver
import org.ossreviewtoolkit.plugins.packagemanagers.node.NodePackageManager
Expand All @@ -44,6 +45,27 @@
import org.semver4j.range.RangeList
import org.semver4j.range.RangeListFactory

data class PnPMConfig(
/**
* The version of PNPM to use when analyzing projects. Defaults to the version 10.24.0
*/
val pnpmVersion: String?,
Copy link
Member

@fviernau fviernau Dec 8, 2025

Choose a reason for hiding this comment

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

This seems to be unused.


/**
* Ignore "pnpmfile" when analyzing projects. Defaults to "--ignore-pnpmfile"
Copy link
Member

@fviernau fviernau Dec 8, 2025

Choose a reason for hiding this comment

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

If the KDoc mention the default value, it should imho use the Boolean type.

*/
@OrtPluginOption(defaultValue = "true")
val ignore_pnpmfile: Boolean,

Check notice on line 58 in plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt

View workflow job for this annotation

GitHub Actions / qodana-scan

Property naming convention 

Property name `ignore_pnpmfile` should not contain underscores
Copy link
Member

@sschuberth sschuberth Dec 8, 2025

Choose a reason for hiding this comment

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

Please stick to camel-case option names.

Comment on lines +54 to +58

Check warning

Code scanning / detekt

Constructor parameter names should follow the naming convention set in the projects configuration. Warning

Constructor parameter names should match the pattern: [a-z][A-Za-z0-9]*

Check notice

Code scanning / QDJVMC

Property naming convention Note

Property name ignore_pnpmfile should not contain underscores

/**
* Ignore "pnpmfile" when analyzing projects. Defaults to ""--ignore-scripts"
*/
@OrtPluginOption(defaultValue = "true")
val ignore_scripts: Boolean,

Check notice on line 64 in plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt

View workflow job for this annotation

GitHub Actions / qodana-scan

Property naming convention 

Property name `ignore_scripts` should not contain underscores
Copy link
Member

@fviernau fviernau Dec 8, 2025

Choose a reason for hiding this comment

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

Would you have a rationale, why this needs to be configurable?
In particular, in which cases would one want to set these to false?

Copy link
Member

@sschuberth sschuberth Dec 8, 2025

Choose a reason for hiding this comment

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

In particular, in which cases would one want to set these to false?

I've seen cases where scripts generate files for other package managers in a multi-package-manager-setup.

Copy link
Member

@fviernau fviernau Dec 8, 2025

Choose a reason for hiding this comment

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

Can you please share one such case?

And also, are you sure this realted to --ignore-scripts, which to my understanding is for post installation, which in turn is for dependencies, not for (ORT) projects. And multi-package-manager-setup to me sounds like a project thing.

Copy link
Member

@sschuberth sschuberth Dec 9, 2025

Choose a reason for hiding this comment

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

Can you please share one such case?

Unfortunately not, it has been a private repository. But this project has a similar concept where react-native integration is run from scripts.

And also, are you sure this realted to --ignore-scripts, which to my understanding is for post installation,

I'm not sure, but the docs are very generic and say "Do not execute any scripts defined in the project package.json and its dependencies" (emphasis mine), so it sounds not to be limited to post-install scripts.

And multi-package-manager-setup to me sounds like a project thing.

I'm not sure what that means. There certainly are project setups where you need to know in which order to build things to get the right results, i.e. use ORT's mustRunAfter functionality.

What's the problem with making this configurable?

Copy link
Author

@thomasscheer thomasscheer Dec 9, 2025

Choose a reason for hiding this comment

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

In our case, the pnpmfile-mechanism is used to generate code during the build. The pnpm install won't work with --ignore-pnpmfile.

I added the second variable only for completness.

Copy link
Member

@fviernau fviernau Dec 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What's the problem with making this configurable?

If it's needed to make it configurable, there is no problem at all.
I just wanted to understand the need. If there is no need I believe then it shouldn't be done.
But for ignore-scripts I found further evidence that configuration makes sense, see https://www.npmjs.com/package/can-i-ignore-scripts.

Copy link
Member

@fviernau fviernau Dec 9, 2025

Choose a reason for hiding this comment

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

How is the need for ignore_pnpmfile? Does this have to be configurable, or could it always be ignored?

Copy link
Member

@sschuberth sschuberth Dec 9, 2025

Choose a reason for hiding this comment

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

I guess we generally should make things configurable if they deviate from the package manager's default. PNPM's default is to not ignore scripts, and if we'd hard-code to do ignore script, that might be different from what users expect.

Copy link
Member

@fviernau fviernau Dec 9, 2025

Choose a reason for hiding this comment

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

I guess we generally should make things configurable if they deviate from the package manager's default.

I believe we used to follow the rule to make things configurable only if needed, and not if it deviates from default.
It would be relatively easy to come up with a list of counter examples, invalidating this rule. For exmaple, we do set the node-linker and this should not be configurable.

Comment on lines +60 to +64

Check warning

Code scanning / detekt

Constructor parameter names should follow the naming convention set in the projects configuration. Warning

Constructor parameter names should match the pattern: [a-z][A-Za-z0-9]*

Check warning

Code scanning / detekt

Rule to mandate/forbid trailing commas at declaration sites Warning

Unnecessary trailing comma before ")"

Check notice

Code scanning / QDJVMC

Property naming convention Note

Property name ignore_scripts should not contain underscores


Check warning

Code scanning / detekt

Reports consecutive blank lines Warning

Needless blank line(s)

Check warning

Code scanning / detekt

Detects trailing spaces Warning

Trailing space(s)
)

internal object PnpmCommand : CommandLineTool {
override fun command(workingDir: File?) = if (Os.isWindows) "pnpm.cmd" else "pnpm"

Expand All @@ -62,8 +84,9 @@
description = "The PNPM package manager for Node.js.",
factory = PackageManagerFactory::class
)
class Pnpm(override val descriptor: PluginDescriptor = PnpmFactory.descriptor) :
class Pnpm(override val descriptor: PluginDescriptor = PnpmFactory.descriptor, private val config: PnPMConfig) :

Check warning

Code scanning / detekt

Detects trailing spaces Warning

Trailing space(s)
NodePackageManager(NodePackageManagerType.PNPM) {

Check warning

Code scanning / detekt

Detects trailing spaces Warning

Trailing space(s)
override val globsForDefinitionFiles = listOf(NodePackageManagerType.DEFINITION_FILE)

private val moduleInfoResolver = ModuleInfoResolver.create { workingDir, moduleId ->
Expand Down Expand Up @@ -146,10 +169,11 @@
}

private fun installDependencies(workingDir: File, scopes: Collection<Scope>) {

Check warning

Code scanning / detekt

Reports methods that have an empty first line. Warning

First line in a method block should not be empty

Check warning

Code scanning / detekt

Detects trailing spaces Warning

Trailing space(s)
val args = listOfNotNull(
"install",
"--ignore-pnpmfile",
"--ignore-scripts",
"--ignore-pnpmfile".takeIf { config.ignore_pnpmfile },
"--ignore-scripts".takeIf { config.ignore_scripts },
"--frozen-lockfile", // Use the existing lockfile instead of updating an outdated one.
"--prod".takeUnless { Scope.DEV_DEPENDENCIES in scopes }
)
Expand Down
Loading