Skip to content
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.

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

feat(pnpm): Add support for PNPM 8.x #7633

Merged
merged 2 commits into from
Oct 6, 2023

Conversation

MarcelBochtler
Copy link
Member

@MarcelBochtler MarcelBochtler commented Oct 5, 2023

According to 1 the only breaking change in PNPM 8.0.0 is that the support for Node.js 14 was dropped, which is not relevant for ORT's PNPM implementation.

Breaking change
The upgrade to PNPM 8.8.0 in the Docker image will cause analyzing PNPM projects that were created using PNPM 7.x resulting in an error.
To fix this, one has to update the PNPM lockfile using version PNPM version 8.0.0 or newer.

@MarcelBochtler MarcelBochtler requested a review from a team as a code owner October 5, 2023 07:45
@@ -70,7 +70,7 @@ class Pnpm(

override fun command(workingDir: File?) = if (Os.isWindows) "pnpm.cmd" else "pnpm"

override fun getVersionRequirement(): RangesList = RangesListFactory.create("5.* - 7.*")
override fun getVersionRequirement(): RangesList = RangesListFactory.create("5.* - 8.*")
Copy link
Member

Choose a reason for hiding this comment

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

The change of the default resolution mode in the change log IIUC may lead to change in dependency resolution results. However, I believe if the lockfile practice is followed, it shouldn't have any effect.

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (89c626e) 68.03% compared to head (3320e0a) 68.03%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7633   +/-   ##
=========================================
  Coverage     68.03%   68.03%           
  Complexity     2023     2023           
=========================================
  Files           344      344           
  Lines         16727    16727           
  Branches       2372     2372           
=========================================
  Hits          11381    11381           
  Misses         4363     4363           
  Partials        983      983           
Flag Coverage Δ
funTest-non-docker 36.45% <ø> (ø)
test 35.54% <ø> (ø)

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.

fviernau
fviernau previously approved these changes Oct 5, 2023
@@ -14,7 +14,7 @@ NPM_VERSION=8.15.1
NUGET_INSPECTOR_VERSION=0.9.12
PHP_VERSION=8.1
PIPTOOL_VERSION=22.2.2
PNPM_VERSION=7.8.0
PNPM_VERSION=8.8.0
Copy link
Member

Choose a reason for hiding this comment

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

As this upgrade breaks the tests, I was wondering if updating lockfiles should be squashed. I'll leave it up to you. Without squashing I believe it makes sense to at least mention that tests break and lockfiles are updated in a following commit.

According to [1] the only breaking change in PNPM 8.0.0 is that the
support for Node.js 14 was dropped, which is not relevant for ORT's PNPM
implementation.

[1]: https://github.com/pnpm/pnpm/blob/main/pnpm/CHANGELOG.md#800

Signed-off-by: Marcel Bochtler <[email protected]>
See [1] for the changelog. The lockfiles of the synthetic FunTests are
no longer compatible with this PNPM version, therefore update them as
well to fix this error:
```
ERR_PNPM_FROZEN_LOCKFILE_WITH_OUTDATED_LOCKFILE
Cannot perform a frozen installation because the version of the lockfile
is incompatible with this version of pnpm

Try either:
1. Aligning the version of pnpm that generated the lockfile with the
version that installs from it, or
2. Migrating the lockfile so that it is compatible with the newer
version of pnpm, or
3. Using "pnpm install --no-frozen-lockfile"
Note that in CI environments, this setting is enabled by default.
```

[1]: https://github.com/pnpm/pnpm/blob/main/pnpm/CHANGELOG.md#880

Signed-off-by: Marcel Bochtler <[email protected]>
@MarcelBochtler MarcelBochtler merged commit b15dbb2 into oss-review-toolkit:main Oct 6, 2023
35 checks passed
@MarcelBochtler MarcelBochtler deleted the pnpm-8.x branch October 6, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants