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

[rush] Rush fails to install under pnpm v5.0.0. #1892

Closed
1 of 2 tasks
eozu opened this issue May 28, 2020 · 16 comments
Closed
1 of 2 tasks

[rush] Rush fails to install under pnpm v5.0.0. #1892

eozu opened this issue May 28, 2020 · 16 comments

Comments

@eozu
Copy link

eozu commented May 28, 2020

Is this a feature or a bug?

  • Feature
  • Bug

Please describe the actual behavior.

pnpm v5.0.0 has removed --no-lock as a flag, which now raises an error if used. As a result, Rush will fail to install if a user configures their rush.json file to use pnpm v5.0.0.

I noticed that pnpm v5.0.0 came out of release candidacy, but may still be a pre-release version. In any case, if it is released in its current form and users upgrade, they'll encounter the error. v5 seems to have some great features, so I would expect eager teams to want to test.

Release Notes: pnpm v5.0.0 where it references the following:

We are not using directory locks anymore. So the --no-lock option will throw an error. Some users had issues with locking. We have confidence that pnpm will never leave either node_modules or the store in a broken state, so we removed locking.

An issue on pnpm/pnpm was raised, more so for awareness.

If the issue is a bug, how can we reproduce it? Please provide detailed steps and include a GitHub branch if applicable. Your issue will get resolved faster if you can make it easy to investigate.

npm install -g @microsoft/rush
git clone https://github.com/microsoft/rushstack
cd rushstack
# Configure rush.json to use pnpm 5.0.0
rush update

The error received is:

 ERROR  Unknown option 'lock'
For help, run: pnpm help install

The command failed:
 /Users/eozu/Sandbox/rushstack/common/temp/pnpm-local/node_modules/.bin/pnpm install 
--no-lock --no-prefer-frozen-lockfile --strict-peer-dependencies
ERROR: Error: The command failed with exit code 1

The fix seems relatively straightforward. Remove the --no-lock flag during the install process.

What is the expected behavior?

Rush should install the dependencies normally.

If this is a bug, please provide the tool version, Node.js version, and OS.

  • Tool: Rush
  • Tool Version: 5.23.4
  • Node Version: 12.17.0
    • Is this a LTS version? Yes
    • Have you tested on a LTS version? Yes
  • OS: macOS Catalina 10.15.5
@eozu eozu changed the title [rush] Rush fails to install under ppm v5.0.0. [rush] Rush fails to install under pnpm v5.0.0. May 28, 2020
@zkochan
Copy link
Contributor

zkochan commented May 28, 2020

I fixed the issue with the option but I think pnpm v5 will not work with current Rush.

I did some optimizations and skipped a workaround that was added for Rush: https://github.com/pnpm/pnpm/blob/acf3dbc570131359ce3a731f0178c3a0058c3137/packages/supi/test/install/local.ts#L167

pnpm install does not update local tarball dependencies in v5. I am not sure if I will return that workaround. It would be nice if Rush could either use pnpm's built-in monorepo support or generate the tarball files with some random names.

@danimas
Copy link

danimas commented May 28, 2020

With pnpm 5.0.1 rush projects works fine. You only get a warning.

@zkochan
Copy link
Contributor

zkochan commented May 28, 2020

what about, when you change the dependencies in one of the projects?

@octogonz
Copy link
Collaborator

pnpm install does not update local tarball dependencies in v5. I am not sure if I will return that workaround.

This seems like incorrect behavior for PNPM. (A file: dependency refers to files on disk, so if they change, PNPM should detect the change and update the lockfile. Otherwise the caching strategy is basically broken.) But maybe I am misunderstanding something, or maybe there is an easy workaround. We can chat on Gitter if that's a faster way to sort it out.

It would be nice if Rush could either use pnpm's built-in monorepo support

We are working on this in #1897 . But there are a number of steps before it will reach feature parity with the old way. And it will take time for all users to migrate to the new thing. In the meantime, it doesn't look good to be stranded on PNPM 4 permanently. We need get it fixed.

or generate the tarball files with some random names.

I don't see how this can work. But maybe I am misunderstanding something.

@zkochan
Copy link
Contributor

zkochan commented May 28, 2020

ok, it will work the same in v5

@sachinjoseph
Copy link
Member

@octogonz @zkochan We filed issues with tarball handling on pnpm repo last year: pnpm/pnpm#1882, pnpm/pnpm#1889

@eozu
Copy link
Author

eozu commented Jun 2, 2020

Just a quick update (I know things are being worked out and actively discussed):

  • PNPM v5.0.2 was installing using Rush with deprecation warnings.
  • PNPM v5.1.2 raises an error in Rush for the same issue raised in the original post.

PNPM does run the little “A newer version of PNPM is available, upgrade to 5.1.2,” script every so often.

@arikon
Copy link

arikon commented Jun 7, 2020

Got this error trying to update to [email protected]

Running "pnpm install" in /Users/arikon/projects/smart-oil/smart-oil-monorepo/common/temp

 ERROR  Unknown options: 'lock', 'prefer-frozen-lockfile', 'resolution-strategy'
For help, run: pnpm help add

The command failed:
 /Users/arikon/projects/smart-oil/smart-oil-monorepo/common/temp/pnpm-local/node_modules/.bin/pnpm install --store /Users/arikon/projects/smart-oil/smart-oil-monorepo/common/temp/pnpm-store --no-lock --no-prefer-frozen-lockfile --resolution-strategy fewer-dependencies
ERROR: Error: The command failed with exit code 1

zkochan added a commit to zoli-forks/rushstack that referenced this issue Jun 7, 2020
The --resolution-strategy option is deprecated in pnpm v5.
It doesn't fail the installation but it should be written as
--resolution-strategy=<value>. Otherwise, pnpm cannot tell that
the argument is the value of the deprecated option.

ref microsoft#1892
@octogonz
Copy link
Collaborator

octogonz commented Jun 8, 2020

Thanks @zkochan for making this fix! 👍👍

Please test Rush 5.24.4 and confirm that PNPM 5 is working with these changes. If nobody has further issues, we'll close this issue.

@sdalonzo
Copy link

sdalonzo commented Jun 8, 2020

@octogonz @zkochan Rush 5.24.4 and PNPM 5.1.5 confirmed working together! Thanks for this!

@RIP21
Copy link

RIP21 commented Jun 12, 2020

For our 3k+ dependencies, 40 packages, 400k+ loc project it works so far.
If any issues will be detected will report here.
Thanks! :)

@mikeharder
Copy link
Contributor

@octogonz: @microsoft/[email protected] and [email protected] appears to be working fine for https://github.com/Azure/azure-sdk-for-js. I do see a couple of warnings during rush install, is there any way to avoid these?

 WARN  Deprecated options: 'lock', 'resolution-strategy'

@eozu
Copy link
Author

eozu commented Jun 16, 2020

I agree with the above. If it cannot be quickly removed, we may need another ticket to have Rush not pass those flags when using PNPM >= v5.

@mikeharder
Copy link
Contributor

Issue for the warnings on pnpm v5: #1937

@babusatti
Copy link

Hi @ALL
How do I stop installation of pnpm while I run rush update. I have already installed same version of pnpm in my container and also how can I tell rush to refer the pnpm installation path in container.

Thanks in advance

@elliot-nelson
Copy link
Collaborator

Closing this ticket as the original issue has been fixed.

Regarding the additional question:

How do I stop installation of pnpm while I run rush update. I have already installed same version of pnpm in my container and also how can I tell rush to refer the pnpm installation path in container.

Rush always installs its own copy of PNPM, as specified in rush.json, regardless of what might be installed globally on the container or running machine. (You can point it at an existing PNPM Store, but not the pnpm binary.)

@iclanton iclanton moved this to Closed in Bug Triage Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests