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] Support PNPM 6 and Removal of pnpm-lock.yaml Rewriting #2610

Merged
merged 9 commits into from
Apr 20, 2021

Conversation

D4N14L
Copy link
Member

@D4N14L D4N14L commented Apr 14, 2021

Summary

Adds support for PNPM 6 in Rush!

Details

  • Fixes [rush] Rush reports "Internal Error" after upgrading to PNPM 6 #2601 by using new path format to exploded tarball for local dependency
  • Removes pnpm-lock.yaml modification of tarball integrity hashes for local projects in non-workspaces PNPM installs and replaces it with validation prior to running PNPM install
  • Removes pnpm-lock.yaml shrinkwrap churn optimization in favor of built-in shrinkwrap churn optimization in PNPM
  • Fixes [rush] Inconsistent yaml serialization during rush update #2602 where a bug in Rush caused double-quotes to be added in the pnpm-lock.yaml in some scenarios
  • Only attempts to delete pnpm-workspace.yaml once instead of for each local package

pnpm-lock.yaml modification by Rush has been something that we have been attempting to remove for some time. With PNPM 6 using a new pnpm-lock.yaml format, this felt like a good time to remove this functionality instead of updating and special-casing writing of the file even further. There were two things that were being done with this manual modification:

  1. Shrinkwrap churn optimization on behalf of PNPM prior to running install in order to reduce the amount of churn in the pnpm-lock.yaml file when adding a dependency that may be satisfied by an existing instance of that package elsewhere in the monorepo. This optimization was confusing for devs ([rush] Using pnpm, an unexpected version of a transitive dependency is installed #1142), and the PNPM-side resolution strategy fewer-dependencies has been the default since v3 and is the only option as of v5. Additionally, this churn optimization would happen during normal rush install, which means that this feature was breaking the expectation that "install" would perform install with the lockfile in it's current state. We even had to disable this feature specifically when using the PNPM frozen-lockfile feature, even though this is not documented anywhere. In my opinion, PNPM has gotten much better at this, to the point that it doesn't make sense to keep maintaining the feature within Rush.
  2. Manual tarball integrity hash updates for local packages. These were performed in a narrow use-case of non-workspace installs using PNPM, with the usePnpmFrozenLockfileForRushInstall experiment set to true. This was implemented in order to smooth over issues with using the frozen-lockfile feature in PNPM that were found when monorepo packages had versions bumped. When this happens (or when the contents of a package.json is changed at all in a way that affects the temp package.json in common/temp, really), the lockfile integrity hash would become out of date, causing the install to fail. This is an artifact of how Rush uses tarballs for local package installs in non-workspace monorepos. I've replaced this with a validation check prior to running install that should clearly describe to the developer how to resolve the out-of-date hash (running rush update). Removing this will mean that people in this narrow use-case will need to run rush update more frequently.

The net result of these changes is that we no longer have any need to directly modify the pnpm-lock.yaml file, and can instead leave that entirely up to PNPM to handle. This goes hand-in-hand with the goal of decoupling Rush from specific package managers.

How it was tested

Ran installs against legacy Rush repo (tsdoc) as well as more recent Rush repo. Validated that install completed successfully and that builds could run as expected. Validated that changes to package versions in non-workspace monorepos with usePnpmFrozenLockfileForRushInstall set to true would result in integrity hash differences that require rush update to be run. Validated that rush update resolved the issue.

Copy link
Contributor

@dmichon-msft dmichon-msft left a comment

Choose a reason for hiding this comment

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

Couple nits, but looks good.

apps/rush-lib/src/logic/pnpm/PnpmLinkManager.ts Outdated Show resolved Hide resolved
@dmichon-msft
Copy link
Contributor

Does this fix #2602 as well?

@D4N14L
Copy link
Member Author

D4N14L commented Apr 15, 2021

Does this fix #2602 as well?

Yes it does. By removing all manual modification of the lockfile it should be consistently serialized by PNPM

@octogonz octogonz changed the title Support PNPM 6 and Removal of pnpm-lock.yaml Rewriting [rush] Support PNPM 6 and Removal of pnpm-lock.yaml Rewriting Apr 17, 2021
@wbern
Copy link
Contributor

wbern commented Apr 18, 2021

It's really impressive that you're polishing rush to the point of maintaining separation of concerns regarding things like this. Keep up the good work!

Utilities.syncFile(
this._rushConfiguration.getCommittedShrinkwrapFilename(this.options.variant),
this.rushConfiguration.tempShrinkwrapPreinstallFilename
);
Copy link
Collaborator

@octogonz octogonz Apr 19, 2021

Choose a reason for hiding this comment

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

When I invoke rush install using this PR, for the already-up-to-date state (where pnpm install is skipped), the console output looks like this:

Found files in the "common/git-hooks" folder.
Successfully installed these Git hook scripts: pre-commit

Trying to acquire lock for pnpm-5.18.9
Acquired lock for pnpm-5.18.9
Found pnpm version 5.18.9 in C:\Users\Owner\.rush\node-v12.20.1\pnpm-5.18.9

Symlinking "C:\Git\MyRepo\common\temp\pnpm-local"
  --> "C:\Users\Owner\.rush\node-v12.20.1\pnpm-5.18.9"
Copying C:\Git\MyRepo\common\config\rush\.npmrc --> C:\Git\MyRepo\common\temp\.npmrc
Updating C:\Git\MyRepo\common\temp\pnpmfile.js

Updating workspace files in C:\Git\MyRepo\common\temp
Finished creating workspace (0.16 seconds)
Updating C:\Git\MyRepo\common\temp\pnpm-lock.yaml
Updating C:\Git\MyRepo\common\temp\pnpm-lock-preinstall.yaml

Checking node_modules in C:\Git\MyRepo\common\temp


Rush install finished successfully. (0.58 seconds)

(The messages about pnpm-lock.yaml and pnpm-lock-preinstall.yaml are new in this PR.)

Some observations:

  • Symlinking shows the destination on a separate line from the source
  • Copying shows the destination on the same line as the source. (When I looked at the code, it seems that actually Utilities.copyAndTrimNpmrcFile() is not actually a simple "copy" operation, but rather transforms the file based on environment variables.)
  • Updating shows ONLY the destination. If the source file does not exist, then Utilities.syncFile() will instead say Deleting ${destinationPath}. (But this code path seems to be impossible for pnpm-lock.yaml.)
  • Some other common/temp files don't get mentioned at all: clientPnpmfile.js, current-variant.json, pnpmfileSettings.json, pnpm-workspace.yaml
  • The Checking node_modules in message is actually testing whether we can skip pnpm install or not.

🤔 Maybe this is fine? Some possible improvements:

  1. Change Utilities.copyAndTrimNpmrcFile() to say "Transforming" and print the --> on a separate line (like how "Symlinking" does)
  2. Change Utilities.syncFile() to say "Copying" since it is an actual file copy. The source is unimportant.
  3. We should print a newline before Updating C:\Git\MyRepo\common\temp\pnpm-lock.yaml since it is unrelated to Finished creating workspace (0.16 seconds)
  4. In the case where Checking node_modules in determines to skip pnpm install, maybe we should print a message like Installation is already up to date.

Copy link
Collaborator

Choose a reason for hiding this comment

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

3. We should print a newline before Updating C:\Git\MyRepo\common\temp\pnpm-lock.yaml since it is unrelated to Finished creating workspace (0.16 seconds)

(Alternatively, maybe we should eliminate the message Finished creating workspace (0.16 seconds) -- is it important to measure this? Have you ever seen it take a nontrivial amount of time?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I got all of these but you may want to try it out locally to see how it feels to you (but heads up my remote URL changed),

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
6 participants