node: Prepare for a larger `Npm` refactoring #9385

fviernau · 2024-11-06T10:53:04Z

Prepare for re-writing large parts of Npm to solely rely on the npm command output to construct the dependency tree,
which will remove the inheritance between Yarn and Npm. To allow doing that, invert the inheritance between Yarn and Npm as a "temporary" refactoring step.

Also do further minor preparations.

Part of: #9261.

Prepare for re-writing large parts of `Npm` to no more rely on the file hierarchy of `node_modules` when constructing the dependency tree. While the same is planned for `Yarn` too, this change is just a refactoring step which allows to do `Npm` first. Signed-off-by: Frank Viernau <[email protected]>

As of the recent refactoring of `Pnpm`, the is no more override of this function in ORT. So, make the function private. Signed-off-by: Frank Viernau <[email protected]>

codecov · 2024-11-06T11:20:22Z

Codecov Report

Attention: Patch coverage is 56.77083% with 83 lines in your changes missing coverage. Please review.

Project coverage is 66.72%. Comparing base (390a055) to head (92f0710).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...-managers/node/src/main/kotlin/utils/NpmSupport.kt	55.84%	8 Missing and 26 partials ⚠️
...package-managers/node/src/main/kotlin/yarn/Yarn.kt	35.84%	34 Missing ⚠️
...s/package-managers/node/src/main/kotlin/npm/Npm.kt	75.40%	6 Missing and 9 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9385      +/-   ##
============================================
- Coverage     67.67%   66.72%   -0.96%     
- Complexity     1223     1236      +13     
============================================
  Files           244      247       +3     
  Lines          8626     8962     +336     
  Branches        911      974      +63     
============================================
+ Hits           5838     5980     +142     
- Misses         2413     2567     +154     
- Partials        375      415      +40

Flag	Coverage Δ
funTest-non-docker	`33.74% <ø> (+0.17%)`	⬆️
test	`37.12% <56.77%> (+0.12%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

The functions `parseProject()` and `parsePackage()` are used by mutliple package managers. Move them to `NpmSupport` which contains common code to account for that. Signed-off-by: Frank Viernau <[email protected]>

Align with `pnpm` and `yarn2`. Signed-off-by: Frank Viernau <[email protected]>

Establish consistency and prepare for an upcoming refactoring of `Npm` which will add further classes under the `npm` directory. Signed-off-by: Frank Viernau <[email protected]>

Use an own logger instead of the one from `Npm`. Signed-off-by: Frank Viernau <[email protected]>

sschuberth · 2024-11-06T13:10:14Z

Merging despite the unrelated StackFunTest failure.

sschuberth · 2024-11-06T13:14:06Z

Merging despite the unrelated StackFunTest failure.

Oh, crap, sorry @oss-review-toolkit/core-devs, I was mixing this up with #9380 which I intended to merge!

Let me do a retrospective review to see if I need to revert things, or if we can live with the accidental merge.

Sorry again 😞

sschuberth

I've added a few retrospective comments. I don't think we need to revert the accidental merge, but getting relies to my questions would be nice for the sake of documenting things.

sschuberth · 2024-11-06T13:20:28Z

plugins/package-managers/node/src/main/kotlin/Npm.kt

@@ -22,68 +22,41 @@
 package org.ossreviewtoolkit.plugins.packagemanagers.node

 import java.io.File
-import java.util.concurrent.ConcurrentHashMap


Commit message:

While the same is planned for Yarn too, this change is just a refactoring step which allows to do Npm first.

As this refactoring moves around so much code which is hard to review, it would have been nice to explain why Npm is done first. Wouldn't it have been easier to do Yarn first?

Doing this for Npm is way more straight forward than doing this for Yarn.
I wanted to do the low hanging fruit first, which also allows to quickly add workspaces support for NPM.

How to best implement this in Yarn I'm not sure. I'm considering to ask Yarn community whether they are open to a contribution to the list command, so that it outputs something which is less complex to process for our purpose.

I'm considering to ask Yarn community whether they are open to a contribution to the list command

But then we would depend on (at least) that Yarn version, which is probably not feasible anyway.

Hm, why would this be problematic? I do remember similar thing for go mod.

Because projects might depend on a lower version of Yarn (for whatever reason), similar to like some projects depend on a lower Version of Gradle.

sschuberth · 2024-11-06T13:21:45Z

plugins/package-managers/node/src/main/kotlin/Npm.kt

@@ -26,32 +26,13 @@ import java.io.File
 import org.apache.logging.log4j.kotlin.logger

 import org.ossreviewtoolkit.analyzer.AbstractPackageManagerFactory
-import org.ossreviewtoolkit.analyzer.PackageManager.Companion.getFallbackProjectName


Commit message:

Typo in "mutliple".

sschuberth · 2024-11-06T13:24:03Z

plugins/package-managers/node/src/main/kotlin/npm/Npm.kt

@@ -94,7 +95,7 @@ class Npm(
        return runCatching {
            val process = run(workingDir, "info", "--json", packageName)

-            parsePackageJson(process.stdout)
+            org.ossreviewtoolkit.plugins.packagemanagers.node.parsePackageJson(process.stdout)


Why is this a FQN now?

Just a mistake. Haven't noticed it.

sschuberth · 2024-11-06T13:26:25Z

plugins/package-managers/node/src/main/kotlin/utils/NpmSupport.kt

@@ -247,8 +247,10 @@ internal fun parsePackage(
    return module
 }

+private val logger = loggerOf(MethodHandles.lookup().lookupClass())


I'd move this to the top to make more clear that all functions could make use of it.

fviernau · 2024-11-06T14:26:35Z

I've addressed your (@sschuberth) comments where possible with #9389.

fviernau requested a review from a team as a code owner November 6, 2024 10:53

fviernau force-pushed the prepare-for-refactoring-npm branch from 7a0bd6c to 0ede477 Compare November 6, 2024 11:03

fviernau added 2 commits November 6, 2024 12:05

refactor(yarn)!: Make loadWorkspaceSubmodules() private

d02a0f4

As of the recent refactoring of `Pnpm`, the is no more override of this function in ORT. So, make the function private. Signed-off-by: Frank Viernau <[email protected]>

fviernau force-pushed the prepare-for-refactoring-npm branch 2 times, most recently from 5497a72 to f015cc1 Compare November 6, 2024 11:06

fviernau enabled auto-merge (rebase) November 6, 2024 11:07

fviernau force-pushed the prepare-for-refactoring-npm branch from f015cc1 to d48900f Compare November 6, 2024 11:26

fviernau added 4 commits November 6, 2024 12:30

refactor(node): Move two model mapping functions to NpmSupport

4dd7283

The functions `parseProject()` and `parsePackage()` are used by mutliple package managers. Move them to `NpmSupport` which contains common code to account for that. Signed-off-by: Frank Viernau <[email protected]>

refactor(node)!: Move Yarn into its own dedicated directory

385b55f

Align with `pnpm` and `yarn2`. Signed-off-by: Frank Viernau <[email protected]>

refactor(node)!: Also move Npm into its own dedicated directory

0a85e82

Establish consistency and prepare for an upcoming refactoring of `Npm` which will add further classes under the `npm` directory. Signed-off-by: Frank Viernau <[email protected]>

refactor(node): Remove a dependency on Npm

92f0710

Use an own logger instead of the one from `Npm`. Signed-off-by: Frank Viernau <[email protected]>

fviernau force-pushed the prepare-for-refactoring-npm branch from d48900f to 92f0710 Compare November 6, 2024 11:30

sschuberth disabled auto-merge November 6, 2024 13:09

sschuberth merged commit ccdcad4 into main Nov 6, 2024
20 of 23 checks passed

sschuberth deleted the prepare-for-refactoring-npm branch November 6, 2024 13:10

sschuberth reviewed Nov 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node: Prepare for a larger `Npm` refactoring #9385

node: Prepare for a larger `Npm` refactoring #9385

fviernau commented Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 6, 2024 •

edited

Loading

sschuberth commented Nov 6, 2024

sschuberth commented Nov 6, 2024

sschuberth left a comment •

edited

Loading

sschuberth Nov 6, 2024

fviernau Nov 6, 2024

sschuberth Nov 6, 2024

fviernau Nov 6, 2024 •

edited

Loading

sschuberth Nov 6, 2024

sschuberth Nov 6, 2024

sschuberth Nov 6, 2024

fviernau Nov 6, 2024

sschuberth Nov 6, 2024 •

edited

Loading

fviernau Nov 6, 2024

fviernau commented Nov 6, 2024

node: Prepare for a larger Npm refactoring #9385

node: Prepare for a larger Npm refactoring #9385

Conversation

fviernau commented Nov 6, 2024 • edited Loading

codecov bot commented Nov 6, 2024 • edited Loading

Codecov Report

sschuberth commented Nov 6, 2024

sschuberth commented Nov 6, 2024

sschuberth left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fviernau Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fviernau commented Nov 6, 2024

node: Prepare for a larger `Npm` refactoring #9385

node: Prepare for a larger `Npm` refactoring #9385

fviernau commented Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 6, 2024 •

edited

Loading

sschuberth left a comment •

edited

Loading

fviernau Nov 6, 2024 •

edited

Loading

sschuberth Nov 6, 2024 •

edited

Loading