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

Throw error when multiple versions specified #122

Merged
merged 6 commits into from
May 6, 2024
Merged

Throw error when multiple versions specified #122

merged 6 commits into from
May 6, 2024

Conversation

karlhorky
Copy link
Contributor

@karlhorky karlhorky commented Apr 19, 2024

Followup to #33 and #35 by @Jack-Works

Throws error if multiple versions of pnpm are specified, both in:

  • in the GitHub Action config with the key "version"
  • in the package.json with the key "packageManager"

This will avoid ERR_PNPM_BAD_PM_VERSION errors as documented here:

cc @KSXGitHub

src/install-pnpm/run.ts Outdated Show resolved Hide resolved
src/install-pnpm/run.ts Outdated Show resolved Hide resolved
@KSXGitHub KSXGitHub requested a review from zkochan April 19, 2024 10:59
Comment on lines 48 to 50
if (GITHUB_WORKSPACE) {
({ packageManager } = JSON.parse(await readFile(path.join(GITHUB_WORKSPACE, packageJsonFile), 'utf8')))
}
Copy link
Collaborator

@KSXGitHub KSXGitHub Apr 19, 2024

Choose a reason for hiding this comment

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

If there is no package.json at root, this will result in error. Please fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, this error was also there before, wasn't it? Should that be a separate PR?

Happy to wrap it in a try/catch to swallow the error if you think it belongs in this PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, this error was also there before, wasn't it? Should that be a separate PR?

Before, if user already specifies version in the action, no error would actually happen.

Happy to wrap it in a try/catch to swallow the error if you think it belongs in this PR

If the file doesn't exist (error.code === 'ENOENT'), assign undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

packageManager is already set to undefined

I added a condition to swallow the error if error.code === 'ENOENT':

Comment on lines 52 to 58
if (version) {
if (typeof packageManager === 'string') {
throw new Error(`Multiple versions of pnpm specified:
- version ${version} in the GitHub Action config with the key "version"
- version ${packageManager} in the package.json with the key "packageManager"
Remove one of these versions to avoid version mismatch errors like ERR_PNPM_BAD_PM_VERSION`)
}
Copy link
Member

Choose a reason for hiding this comment

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

but why should it fail if the versions match?

Copy link
Contributor Author

@karlhorky karlhorky Apr 19, 2024

Choose a reason for hiding this comment

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

Added a condition to skip if versions match:

@karlhorky
Copy link
Contributor Author

@KSXGitHub @zkochan is there anything left to resolve here? Can this be merged?

@KSXGitHub KSXGitHub requested a review from zkochan May 6, 2024 16:15
@zkochan zkochan merged commit bee1f09 into pnpm:master May 6, 2024
25 of 27 checks passed
@zkochan
Copy link
Member

zkochan commented May 6, 2024

I guess this is a breaking change

@karlhorky
Copy link
Contributor Author

Thanks for the review, the commit improving the robustness of the error condition and merge!

I would also agree that this is a breaking change - I will keep an eye on the releases for v4:

@karlhorky karlhorky deleted the patch-1 branch May 7, 2024 09:31
if (version) {
if (
typeof packageManager === 'string' &&
packageManager.replace('pnpm@', '') !== version
Copy link

Choose a reason for hiding this comment

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

this isn't enough, there can be a hash at end. see recent corepack release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry for any breakage! Can you provide an example of the hash + delimiter? I can open a new PR.

Copy link
Contributor Author

@karlhorky karlhorky May 17, 2024

Choose a reason for hiding this comment

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

I saw this behavior in Corepack just today, the packageManager specifier looks like this:

   "packageManager": "[email protected]+sha512.14e915759c11f77eac07faba4d019c193ec8637229e62ec99eefb7cf3c3b75c64447882b7c485142451ee3a6b408059cdfb7b7fa0341b975f12d0f7629c71195"

Looks like it was released in [email protected] in the following PR:

Copy link
Contributor Author

@karlhorky karlhorky May 17, 2024

Choose a reason for hiding this comment

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

@viceice but maybe this is actually desirable behavior, since:

  • the version input on pnpm/action-setup supports a specifier with a Corepack hash (pnpm/action-setup uses pnpm.cjs install, see output of pnpm install below)
  • technically it could be argued that 9.1.1 is not the same as 9.1.1+sha512.14e915759c11f77eac07faba4d019c193ec8637229e62ec99eefb7cf3c3b75c64447882b7c485142451ee3a6b408059cdfb7b7fa0341b975f12d0f7629c71195, so stripping the hash is maybe not wanted
$ pnpm install [email protected]+sha512.14e915759c11f77eac07faba4d019c193ec8637229e62ec99eefb7cf3c3b75c64447882b7c485142451ee3a6b408059cdfb7b7fa0341b975f12d0f7629c71195

Packages: +1
+
Progress: resolved 1, reused 0, downloaded 1, added 1, done

dependencies:
+ pnpm 9.1.1

Done in 1.3s

Copy link

Choose a reason for hiding this comment

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

Thanks, but does it also work with the standalone version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it, yep:

$ pnpm install @pnpm/[email protected]+sha512.14e915759c11f77eac07faba4d019c193ec8637229e62ec99eefb7cf3c3b75c64447882b7c485142451ee3a6b408059cdfb7b7fa0341b975f12d0f7629c71195
Packages: +2
++
Downloading @pnpm/[email protected]: 22.84 MB/22.84 MB, done
Progress: resolved 7, reused 1, downloaded 2, added 2, done
node_modules/.pnpm/@[email protected]/node_modules/@pnpm/exe: Running preinstall script, done in 46ms

dependencies:
+ @pnpm/exe 9.1.1

Done in 8.6s

jmikedupont2 pushed a commit to meta-introspector/action-setup that referenced this pull request Sep 12, 2024
* Throw error when multiple versions specified

* fix: fmt

* fix: fmt

* Swallow error on ENOENT

* Match versions

* refactor: install pnpm

---------

Co-authored-by: Khải <[email protected]>
Co-authored-by: Zoltan Kochan <[email protected]>
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.

4 participants