feat(Yarn): Add basic support for Corepack [1]. #8703

oheger-bosch · 2024-05-27T15:50:18Z

According to the Yarn 2+ documentation, Corepack is the preferred way to install this package manager. If this method is used, the name of the executable has to be determined differently.

Please ensure that your pull request adheres to our contribution guidelines. Thank you!

analyzer/src/funTest/kotlin/Yarn2Test.kt

codecov · 2024-05-27T16:00:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.85%. Comparing base (1765a15) to head (b45d3a1).
Report is 21 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #8703   +/-   ##
=========================================
  Coverage     67.85%   67.85%           
  Complexity     1165     1165           
=========================================
  Files           244      244           
  Lines          7736     7736           
  Branches        865      865           
=========================================
  Hits           5249     5249           
  Misses         2128     2128           
  Partials        359      359

Flag	Coverage Δ
funTest-non-docker	`34.17% <ø> (ø)`

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.

analyzer/src/funTest/kotlin/Yarn2Test.kt

fviernau · 2024-05-27T16:38:27Z

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

+        /**
+         * The name of the option that determines whether Yarn 2+ was installed via Corepack.
+         */
+        const val OPTION_COREPACK_ENABLED = "corepackEnabled"


commit-message: Can you explain why you choose to implement this as an option as opposed to adding detection whether Corepack is enabled? Looking at [1] it says:

You can quickly check whether Corepack is enabled by running `yarn exec env`: if you get a path as output, Corepack is properly installed. If not, something may be messing with how the shims are installed.

^[1]: https://yarnpkg.com/corepack

See my other comment.

fviernau · 2024-05-27T16:40:55Z

analyzer/src/funTest/kotlin/Yarn2Test.kt

+import org.ossreviewtoolkit.model.config.RepositoryConfiguration
+import org.ossreviewtoolkit.plugins.packagemanagers.node.Yarn2
+
+class Yarn2Test : WordSpec({


The class should be named Yarn2FunTest.

Oops, IntelliJ has created the class at a completely wrong path. I moved it to the test folder of the Node package manager plugin. It is acutally a unit test.

analyzer/src/funTest/kotlin/Yarn2Test.kt

sschuberth · 2024-05-27T16:45:56Z

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

@@ -157,6 +171,8 @@ class Yarn2(
    override fun command(workingDir: File?): String {
        if (workingDir == null) return ""

+        if (options[OPTION_COREPACK_ENABLED].toBoolean()) return DEFAULT_EXECUTABLE_NAME


IMO it would be nicer if this could be (reliably) auto-detected. Does the yarn exec env approach mentioned over here work for that?

This is an interesting point, I was also thinking about this, but am not sure how to handle this in the best way. It would also be possible to even go a step further and design the Yarn2 class in a way that it - optionally - executes the corepack enable command, which is required to use Corepack.

My line of thinking was that ORT is rather low-level. It should focus on obtaining the dependencies from package managers and assume that the environment is properly set up. The setup of the environment falls in the responsibility of integrators, e.g. ORT Server. (This is also related to the discussion we had in the last ORT Community meeting.) Now, the integration environment typically knows whether Corepack should be used or not and can directly provide the information to Yarn2; so there is no need to do an additional check.

But I am open for other opinions.

Any other opinions or comments on that?

Coincidentally, I've read about Corepack today also in the context of #8715. Could we simply auto-enable the use of Corepack if package.json has the packageManager node?

Well, this does not really play well with the API: Whether Corepack is enabled or not affects the command function of Yarn2. However, this function has no obvious way to check the status of Corepack. It is called early in the analysis process to determine the tool version. IIUC, at that time, Yarn2 does not know about the definition files it will have to process.

Ah, yes, I forgot to reply to that earlier:

My line of thinking was that ORT is rather low-level. It should focus on obtaining the dependencies from package managers and assume that the environment is properly set up.

I'd agree to the second sentence, but not so much to the former. Whenever feasible to detect and apply, ORT should use the correct tools and versions automatically. Just out of pure pragmatism we deviated from this rule of thumb for ecosystems where there's no single defined way to declare the tool version, or bootstrapping the tool (on all supported platform) would be infeasible / too much effort.

Or phrased differently: Development teams that take ORT into use should not be required to configure their tools / version for ORT again if they already did so in their build system, as we'd want to avoid teams to maintain basically the same information in multiple different places. That thinking is also the motivation behind the already linked #8715.

However, whether the auto-detection implementation is feasible needs to be decided on a case-by-case basis. And for this particular Yarn / Corepack case, I'm not yet convinced that it is not feasible.

Then I propose the following: The default behavior is to auto-detect whether Corepack is enabled based on the content of the package.json file. But this behavior can be overridden with the configuration property; then this mechanism can be disabled if it is not needed.

Would this be okay?

I have changed the implementation accordingly.

Thanks. But I wonder whether we really needs this:

But this behavior can be overridden with the configuration property

IMO too many things in ORT are configurable already, not always with a proven use-case (anymore). So, you do already see a concrete case where one would opt-out of auto-detection, or was the configuration added "just in case"?

Yes, for ORT Server (at least our installation), we will probably enable Corepack by default. So, the result of the auto-detection must be overridden. The reason for this is that Corepack does not support other installation options; for instance, it refuses to work with a version of yarn that was installed via the package manager of the OS. This means in our case that if we want to support Corepack, we cannot provide default installations of certain package managers, but must rely on Corepack to do the installation.

sschuberth · 2024-06-03T11:52:26Z

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

+        /**
+         * The name of the manifest file used by Yarn 2+.
+         */
+        private const val MANIFEST = "package.json"


Maybe prefer MANIFEST_NAME or MANIFEST_FILENAME (but not MANIFEST_FILE_NAME, also see f9c5095).

PS: I was tempted to say that we should use ORT terminology here and call it DEFINITION_FILENAME, but Yarn indeed calls this a manifest (file), so let's stick to that.

Also, to make the introduction of this constant a bit more worthwhile, I'd propose to also use it in line 159.

And one more thing: While the trend has not been started with this PR, we should not continue adding private constants to a companion object. Using a companion object makes sense to scope public constants only.

IMO these should become private top-level constants (some applies to the function), but this can be addressed separately.

sschuberth · 2024-06-03T12:04:50Z

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

@@ -87,6 +88,9 @@ private enum class YarnDependencyType(val type: String) {
 * - *disableRegistryCertificateVerification*: If true, the `yarn npm info` commands called by this package manager will
 *   not verify the server certificate of the HTTPS connection to the NPM registry. This allows to replace the latter by
 *   a local one, e.g. for intercepting the requests or replaying them.
+ * - *corepackEnabled*: If true, this implementation assumes that Yarn has been installed via


IMO the name of the property (and also partly the description) is a bit misleading now, as it sounds as if Corepack would only be supported at all if corepackEnabled is set to true.

I'd rather propose to rename this e.g. to corepackOverride or forceCorepack and explain that Corepack support is usually auto-detected, but can be overridden / enforced using this property.

Renamed the property and reworded the description.

According to the Yarn 2+ documentation, Corepack [1] is the preferred way to install this package manager. If this method is used, the name of the executable has to be determined differently. [1]: https://yarnpkg.com/corepack Signed-off-by: Oliver Heger <oliver.heger@bosch.io>

Comments addressed.

sschuberth · 2024-06-03T14:05:45Z

Test failures are related to outdated Pub tests only, see #8722.

oheger-bosch requested a review from a team as a code owner May 27, 2024 15:50

github-advanced-security bot found potential problems May 27, 2024

View reviewed changes

analyzer/src/funTest/kotlin/Yarn2Test.kt Fixed Show fixed Hide fixed

analyzer/src/funTest/kotlin/Yarn2Test.kt Fixed Show fixed Hide fixed

analyzer/src/funTest/kotlin/Yarn2Test.kt Fixed Show fixed Hide fixed

oheger-bosch force-pushed the yarn_corepack_support branch 2 times, most recently from 2119800 to c664182 Compare May 27, 2024 15:58

fviernau previously requested changes May 27, 2024

View reviewed changes

sschuberth requested changes May 27, 2024

View reviewed changes

oheger-bosch force-pushed the yarn_corepack_support branch from c664182 to b513234 Compare May 28, 2024 08:04

oheger-bosch requested review from sschuberth and fviernau May 28, 2024 08:13

oheger-bosch force-pushed the yarn_corepack_support branch 2 times, most recently from 843b27a to 53abdfa Compare June 3, 2024 11:08

sschuberth reviewed Jun 3, 2024

View reviewed changes

oheger-bosch force-pushed the yarn_corepack_support branch from 53abdfa to d70c71f Compare June 3, 2024 13:35

oheger-bosch force-pushed the yarn_corepack_support branch from d70c71f to b45d3a1 Compare June 3, 2024 13:37

oheger-bosch requested a review from sschuberth June 3, 2024 13:38

sschuberth approved these changes Jun 3, 2024

View reviewed changes

sschuberth merged commit 3e9d8f4 into oss-review-toolkit:main Jun 3, 2024
18 of 20 checks passed

oheger-bosch deleted the yarn_corepack_support branch June 3, 2024 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Yarn): Add basic support for Corepack [1]. #8703

feat(Yarn): Add basic support for Corepack [1]. #8703

oheger-bosch commented May 27, 2024

codecov bot commented May 27, 2024 •

edited

Loading

fviernau May 27, 2024

oheger-bosch May 28, 2024

fviernau May 27, 2024

oheger-bosch May 28, 2024

sschuberth May 27, 2024

oheger-bosch May 28, 2024

oheger-bosch May 29, 2024

sschuberth May 29, 2024

oheger-bosch May 29, 2024

sschuberth May 29, 2024 •

edited

Loading

oheger-bosch Jun 3, 2024

oheger-bosch Jun 3, 2024

sschuberth Jun 3, 2024

oheger-bosch Jun 3, 2024

sschuberth Jun 3, 2024 •

edited

Loading

sschuberth Jun 3, 2024

sschuberth Jun 3, 2024

sschuberth Jun 3, 2024

oheger-bosch Jun 3, 2024

sschuberth commented Jun 3, 2024

feat(Yarn): Add basic support for Corepack [1]. #8703

feat(Yarn): Add basic support for Corepack [1]. #8703

Conversation

oheger-bosch commented May 27, 2024

codecov bot commented May 27, 2024 • edited Loading

Codecov Report

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

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 May 29, 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 Jun 3, 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 commented Jun 3, 2024

codecov bot commented May 27, 2024 •

edited

Loading

sschuberth May 29, 2024 •

edited

Loading

sschuberth Jun 3, 2024 •

edited

Loading