[rush] Using pnpm, an unexpected version of a transitive dependency is installed

[elliottsj]

See https://github.com/elliottsj/rush-pnpm-bug

In summary, the latest version of transitive dependency react-focus-lock is being installed, despite having pinned an older version in shrinkwrap.yaml.

For now, the workaround I'm using is specifying the older version in common-versions.json:

{
  "preferredVersions": {
    "react-focus-lock": "1.17.7"
  }
}

[acoates-ms]

Seeing similar issues in our repo. Basically means that shrinkwrap is useless... which is very worrying.

[octogonz]

@christiango this seems like it could be the same behavior you described in #1288. Let's use this thread to continue the discussion, since it has an actual repro we can talk about.

After rush install, I see a diff between common/temp/shrinkwrap-preinstall.yaml and shrinkwrap.yaml. This is Rush's optimization that was explained in #1273 (comment):

The intended design is that common/temp/shinkwrap.yaml will be (1) a correct solution of the version ranges, and it will be (2) a deterministic outcome (i.e. unaffected by new versions being published to the registry). It is /not/ intended to be (3) exactly the same as common/config/rush/shrinkwrap.yaml. This relaxation avoids unnecessary modifications to the file tracked by Git, which can cause merge conflicts, which are a significant concern in very active monorepos.
. . .
The temp shrinkwrap file is supposed to be a deterministic function of the package.json files, so it should not change unless the commit includes a change to a package.json file somewhere.

And here's a summary the implementation from #1180 (comment):

One thing to keep in mind is the file copying performed by rush install and rush update. There are 3 files:

A. common/config/rush/shrinkwrap.yaml - the "official" version that is tracked by Git
B. common/temp/shrinkwrap.yaml - the version that PNPM sees when invoked in common/temp
C. common/temp/shrinkwrap-preinstall.yaml - a backup copy of shrinkwrap.yaml, for diagnostic purposes to compare with B

rush install reads A, makes some fixes to it, and writes out B and C before invoking pnpm install

pnpm install reads B and might rewrite it with some changes, making it different from C. (But by design, rush install does not copy these changes back to A; the deviations are allowable because they are supposed to be fully deterministic. The reason is to minimize churn that causes Git merge conflicts.)

rush update is like rush install but it does copy B -> A, but only when Rush determines that the changes are interesting.

rush update --full deletes B, then runs pnpm install to create B, then copies B -> A

Some observations:

• Rush's installation optimization should be better documented. I can see why people are confused by it. I'm sure it's especially mystifying if you're trying to investigate a problem without understanding that this is happening.

• There should be a way to disable this optimization. In fact, creating a named setting in rush.json is a pretty effective way to document it. In most cases (e.g. issue [rush] with pnpm installs patch versions newer than 'locked' versions #1273) I expect people will be okay with the optimization as long as they understand what it's doing. But your https://github.com/elliottsj/rush-pnpm-bug repro brings up an interesting counterexample: In the Git history, you seem to be manually tinkering with the shrinkwrap file to adjust the installation plan. Although that's a strongly discouraged practice, there are valid cases where people need to do that (e.g. applying a hotfix for an old release branch). In those situations you'd want to be editing the real installation plan, not an input to a function that produces it.

• This might be related to the --resolution-strategy issue. There was a longstanding debate about how PNPM solved versions for indirect dependencies (see Deduping algorithm not working properly pnpm/pnpm#1187 for background). It was finally solved by introducing resolutionStrategy=fewer-dependencies in Rush 5.7.x. (BTW Yarn has the same problem and it is still unsolved.) The fix corresponds to this new setting in rush.json:

   /**
     * Configures the strategy used to select versions during installation.
     *
     * This feature requires PNPM version 3.1 or newer.  It corresponds to the "--resolution-strategy" command-line
     * option for PNPM.  Possible values are "fast" and "fewer-dependencies".  PNPM's default is "fast", but this may
     * be incompatible with certain packages, for example the "@types" packages from DefinitelyTyped.  Rush's default
     * is "fewer-dependencies", which causes PNPM to avoid installing a newer version if an already installed version
     * can be reused; this is more similar to NPM's algorithm.
     */
    // "resolutionStrategy": "fast"

Maybe we should upgrade Rush/PNPM for this repro and see if that solves it.

CC @iclanton @matteralus @HarryGifford

[octogonz]

Here's a proposal for disabling the installation optimization: #1300

If you want to simulate the effect of this feature manually, it's pretty easy:

• Run rush update
• Copy common/temp/shrinkwrap.yaml --> common/config/rush/shrinkwrap.yaml
• Commit the result to Git

[octogonz]

Maybe we should upgrade Rush/PNPM for this repro and see if that solves it.

In PNPM 3.x the shrinkwrap file is renamed, and its file format has also changed. The changes are described here:

https://github.com/pnpm/spec/blob/master/lockfile/5.md#changes

I suppose it would be possible to manually convert the file, but @elliottsj do you have an easy way to recreate your repro using PNPM 3.x?

[elliottsj]

@octogonz Done; I've updated the repro using pnpm@3: elliottsj/rush-pnpm-bug@53660df

I updated the pnpm-lock.yaml file by deleting the old shrinkwrap.yaml and running rush update, then manually omitted some of the lockfile changes so it still includes the old react-focus-lock: 1.17.7 and integrity hashes for react-focus-lock.

I tested it again, found the same results and updated the README.md.

[octogonz]

Thank you!

[mikeharder]

@octogonz: If I understand this issue correctly, I believe I have created a standalone repro here: https://github.com/mikeharder/rush-dependency-versions

[mikeharder]

@octogonz: I believe the root cause is rush passes the --no-prefer-frozen-shrinkwrap option:

> rush install --debug-package-manager

Invoking package manager: pnpm install ... --no-prefer-frozen-shrinkwrap

I created a similar repro with just pnpm (no rush), and this issue repros when I use pnpm install --no-prefer-frozen-shrinkwrap, but I get the expected behavior when I use pnpm install.

[octogonz]

What does --no-prefer-frozen-shrinkwrap do? I can't find the docs for it

[mikeharder]

I believe --no-prefer-frozen-shrinkwrap is equivalent to --prefer-frozen-lockfile=false:

https://pnpm.js.org/docs/en/pnpm-install.html#prefer-frozen-lockfile

Here's my standalone pnpm repro:

https://github.com/mikeharder/pnpm-dependency-versions

[mikeharder]

Related PRs

• [rush] Add workaround for issue where shrinkwrap.yaml was sometimes out of sync #807
• [rush] Add support for PNPM's --strict-peer-dependencies feature #817

Related issues

• When a package references a tarball, and the tarball's dependencies change, the change isn't reflected in the shrinkwrap. pnpm/pnpm#1342
• (part 2) When a package references a tarball, and the tarball's dependencies change, the change isn't reflected in the shrinkwrap pnpm/pnpm#1362

[octogonz]

I believe the root cause is rush passes the --no-prefer-frozen-shrinkwrap option:

> rush install --debug-package-manager

Invoking package manager: pnpm install ... --no-prefer-frozen-shrinkwrap

@mikeharder I think you're right! Thanks a ton for investigating this. I asked the PNPM developers about --no-prefer-frozen-shrinkwrap, and they explained that a "headless installation" blindly installs whatever installation plan is found in the shrinkwrap file, without any validation. (The rationale is that it's faster, assuming the shrinkwrap file can be trusted to be correct.) This is definitely incompatible with Rush's optimization.

Could someone create a PR to remove this option, and see if it solves the problem? If so we should merge+publish that ASAP.

@iclanton

[mikeharder]

I don't know much about either Rush or PNPM, but from the history I see that --no-prefer-frozen-shrinkwrap was added to address a previous issue, so is it really safe to completely remove this parameter now? Instead, maybe Rush needs to expose its own command-line parameter, so customers can choose between the two PNPM behaviors?

[octogonz]

from the history I see that --no-prefer-frozen-shrinkwrap was added to address a previous issue, so is it really safe to completely remove this parameter now?

Where are you seeing that? I assumed we added it because it seemed like it would reduce churn, without considering its interaction with the installation optimization.

[mikeharder]

@octogonz: If you feel it's safe to remove --no-prefer-frozen-shrinkwrap, it's fine with us. For our usage, it would almost certainly be preferable to the current behavior. However, my reading of pnpm/pnpm#1342 and pnpm/pnpm#1362 was that the option was added for an important reason.

[Lincoded]

Using the repo in the first post, simply removing --no-prefer-frozen-shrinkwrap in doesn't appear to address this.

[sachinjoseph]

• Using the repo here: https://github.com/sachinjoseph/rush-pnpm-bug

• Before all runs below, pnpm-lock.yaml has react-focus-lock's version as 1.17.7.

PNPM

PNPM behavior seems a bit weird; it has a different behavior on 3.4.1 and 3.5.1.

In (repo-root)/pnpm-only/

$ cd pnpm-only/

pnpm version	$ pnpm install --frozen-shrinkwrap	$ pnpm install	$ pnpm install --no-prefer-frozen-shrinkwrap
3.4.1	installs react-focus-lock version 1.17.7, does NOT update pnpm-lock.yaml	installs react-focus-lock version 1.17.7, does NOT update pnpm-lock.yaml	🔴 installs react-focus-lock version 1.19.1, updates pnpm-lock.yaml
3.5.1	installs react-focus-lock version 1.17.7, does NOT update pnpm-lock.yaml	🔴 installs react-focus-lock version 1.19.1, updates pnpm-lock.yaml	🔴 installs react-focus-lock version 1.19.1, updates pnpm-lock.yaml

In (repo-root)/common/temp

$ cd common/temp/

pnpm version	$ pnpm install --frozen-shrinkwrap	$ pnpm install	$ pnpm install --no-prefer-frozen-shrinkwrap
3.4.1	installs react-focus-lock version 1.17.7, does NOT update pnpm-lock.yaml	🔴 installs react-focus-lock version 1.19.1, updates pnpm-lock.yaml	🔴 installs react-focus-lock version 1.19.1, updates pnpm-lock.yaml
3.5.1	installs react-focus-lock version 1.17.7, does NOT update pnpm-lock.yaml	🔴 installs react-focus-lock version 1.19.1, updates pnpm-lock.yaml	🔴 installs react-focus-lock version 1.19.1, updates pnpm-lock.yaml

Rush with PNPM

Assuming that the PNPM behavior is accurate, rush is NOT syncing common/config/rush/pnpm-lock.yaml with the updated lockfile common/temp/pnpm-lock.yaml. This seems to the bug.

Rush 5.9.1 behavior: rush install passes --no-prefer-frozen-shrinkwrap to pnpm. I tweaked this flag for the runs below.

Repo state after running rush install:

rush version	pnpm version	react-focus-lock version	common/config/rush/pnpm-lock.yaml updated?	common/temp/pnpm-lock.yaml updated?
5.9.1	3.4.1	1.19.1	🔴 NO (not in sync)	YES
5.9.1 minus --no-prefer-frozen-shrinkwrap	3.4.1	1.19.1	🔴 NO (not in sync)	YES
5.9.1 plus --frozen-shrinkwrap	3.4.1	1.17.7	NO	NO
5.9.1	3.5.1	1.19.1	🔴 NO (not in sync)	YES
5.9.1 minus --no-prefer-frozen-shrinkwrap	3.5.1	1.19.1	🔴 NO (not in sync)	YES
5.9.1 plus --frozen-shrinkwrap	3.5.1	1.17.7	NO	NO

(Pnpm flags have been updated to replace the word shrinkwrap to lockfile, e.g. from --frozen-shrinkwrap to --frozen-lockfile.)

• Is common/config/rush/pnpm-lock.yaml expected to be in sync with common/temp/pnpm-lock.yaml? If pnpm install is indeed working the way it's supposed to work (see the table above), then rush needs to sync the updated temp lockfile back to common/config/rush.
• Should new flags such as --frozen-lockfile be introduced to rush install? @mikeharder suggested the same thing above.

[sachinjoseph]

Assuming that the PNPM behavior is accurate, rush is NOT syncing common/config/rush/pnpm-lock.yaml with the updated lockfile common/temp/pnpm-lock.yaml. This seems to the bug.

According to Pete's post, this is not a bug. In that case, common/config/rush/pnpm-lock.yaml is NOT meant to represent the installation plan. The idea of maintaining a lockfile that does not represent the installation plan is confusing.

[sachinjoseph]

(The words pnpm-lock/shrinkwrap/lockfile are used interchangeably in this section.)

File	Description
common/config/rush/pnpm-lock.yaml	lockfile tracked by git
common/temp/pnpm-lock.yaml	the transformed lockfile based on common/config/rush/pnpm-lock.yaml used internally by Rush

1. rush install runs pnpm install in common/temp/ with the flag --no-prefer-frozen-lockfile
2. This could cause common/temp/pnpm-lock.yaml to be regenerated and thus to differ from common/config/rush/pnpm-lock.yaml
3. Rush, by design, does not guarantee that common/config/rush/pnpm-lock.yaml and common/temp/pnpm-lock.yaml will be in sync. This helps reduce the churn of the tracked lockfile leading to fewer merge conflicts, especially on very active monorepos.
4. While this Rush behavior can help reduce merge conflicts, it is also pretty confusing for developers especially those who are accustomed to the PNPM behavior.

Proposals

1. rush install to not pass --no-prefer-frozen-lockfile to pnpm install anymore

pnpm install flag	Default
prefer-frozen-lockfile	true

Rush adds --no-prefer-frozen-lockfile to pnpm install as a workaround but the issue has been fixed in PNPM 2.15.1. Looks like this flag is no longer required - I will verify this. If this flag is removed, then pnpm will modify the lockfile only when it is necessary.

2. Add a new switch to rush install to pass --frozen-lockfile to pnpm install?

3. Keep common/config/rush/pnpm-lock.yaml and common/temp/pnpm-lock.yaml in sync all the time (i.e., revert to the original Rush behavior).
   This will reduce the confusion but will potentially increase merge conflicts. Lockfile churns would slightly reduce if we implement Proposal 1.

pnpm install seems to have some inconsistent behavior - I think this should be sorted out before we make changes to Rush. See pnpm/pnpm#1878.

Locking versions of transitive dependencies

❌ DO NOT USE pnpm-lock.yaml to lock versions of transitive dependencies.

pnpm install may regenerate the lockfile unless unless the flag --frozen-lockfile is used. There's no way currently to pass this flag to pnpm install via Rush. Also, pnpm install will upgrade all dependencies if the lockfile version needs an upgrade - see pnpm/pnpm#1876.

✅ USE preferred versions to lock versions of transitive dependencies.

[sachinjoseph]

Update on the proposals

1. rush install to guarantee to install what's precisely in the shrinkwrap, fail if the shrinkwrap is missing or outdated

This is precisely what the documentation says. See https://rushjs.io/pages/commands/rush_install/.
The current rush install behavior does NOT satisfy this guarantee.

For pnpm, this means that rush install should always pass --frozen-lockfile to pnpm install

If the package.jsons cannot be satisfied by the existing shrinkwrap then rush install would break, as it should.

Open/ToDo:

1. This PNPM bug is a blocker on this proposal: When indirect dependencies change, "pnpm install --frozen-lockfile" fails only if node_modules and pnpm-store don't exist pnpm/pnpm#1882
2. Prevent transformations to shrinkwrap for rush install
   To guarantee that rush install always installs what's described by shrinkwrap, there should not be any transformations. This means that rush install should error out if it determines that the shrinkwrap is out of date. Shrinkwrap could be out of date for numerous reasons, including changes to preferred versions. => changes to preferred versions will require a rush update to update the shrinkwrap. All shrinkwrap updates should come through rush update.
3. Ensure equivalent behavior on yarn and npm.

2. Keep common/config/rush/pnpm-lock.yaml and common/temp/pnpm-lock.yaml always in sync.

If rush install does not transform the shrinkwrap then these files will always be in sync for a rush install. rush update's behavior needs to be updated to always sync these files.

Thoughts?

[sachinjoseph]

@octogonz @nickpape

[octogonz]

Someone should follow up and see what's the status with pnpm/pnpm#1876

[sachinjoseph]

@octogonz From this response, it seems Zoltan is not prioritizing the ask to maintain dependency versions when upgrading the file format.

[octogonz]

From pnpm/pnpm#1876 (comment):

We can only promise full determinism in scope of one pnpm version + lockfile. So if you have a lockfile and you use the same version of pnpm, it should be deterministic.

If I understand correctly, when the lockfile is incompatible with the PNPM version, he's saying that PNPM makes a "best effort" to follow the installation plan. The result is inaccurate and not even deterministic.

If so, that should really be an error. The whole point of a lockfile is to guarantee a deterministic outcome.

I opened pnpm/pnpm#1883 suggesting that PNPM report an error in this case: For example, if rush install failed and said something like "Hey buddy, this lockfile was created using a different version of PNPM; you need to do a rush update --full or else downgrade PNPM." ...I think people would find that to be understandable and reasonable.

I'm not sure whether this fully explains the incorrect installations described in this issue (and #1288 and #1273), but it would definitely eliminate a troublesome variable from the equation.

[octogonz]

Wow, that was fast! The fix for pnpm/pnpm#1883 was already published as PNPM 3.5.3. It also includes the fix for pnpm/pnpm#1882. 😊

[octogonz]

@mikeharder regarding your repro: https://github.com/mikeharder/rush-dependency-versions

...when I upgrade its rush.json to use "rushVersion": "5.9.1" and "pnpmVersion": "3.5.3", it seems to be fixed now. (Hurray!) Let us know if not.

[octogonz]

@sachinjoseph regarding your repro: https://github.com/sachinjoseph/rush-pnpm-bug

...when I upgrade its rush.json to use "rushVersion": "5.9.1" and "pnpmVersion": "3.5.3", it seems to be fixed as well. react-focus-lock 1.17.7 is installed as expected. Let us know if not.

BTW it seems that the fix for pnpm/pnpm#1883 was not to report an error; instead he simply made it so newer versions of PNPM correctly install the older lockfile format.

[octogonz]

Rush adds --no-prefer-frozen-lockfile to pnpm install as a workaround but the issue has been fixed in PNPM 2.15.1. Looks like this flag is no longer required - I will verify this. If this flag is removed, then pnpm will modify the lockfile only when it is necessary.

@sachinjoseph This proposal seems okay. However Rush would need to either (1) drop support for PNPM versions <2.15.1, or else (2) only omit that flag in newer versions.

[octogonz]

1. rush install to guarantee to install what's precisely in the shrinkwrap, fail if the shrinkwrap is missing or outdated
2. Keep common/config/rush/pnpm-lock.yaml and common/temp/pnpm-lock.yaml always in sync.

@sachinjoseph This is the same behavior described in #1300. But that issue proposes that the "minimizeShrinkwrapChurn" optimization should /not/ be removed entirely, but merely made opt-in. Defining a setting in rush.json would give the feature a name and provide a natural place to write detailed comments explaining how it works. I agree that today this optimization is not documented anywhere, and that's quite confusing.

I'm hesitant to remove this optimization entirely. In the sp-client monorepo, we were receiving constant complaints that would go like this:

"My project2 is already using cool-library version 1.2.3. I simply want to add that dependency to project3 as well. Why are you forcing me to regenerate the shrinkwrap file? My project3 wants the exact same version of cool-library, and there it is in Rush's common folder!"

Why people want to avoid regenerating: Any PR that diffs the shrinkwrap file will cause a Git merge conflict with all other PRs that diff the shrinkwrap file. The merge conflict can only be resolved by running rush update. Our .gitattributes explicitly designates shrinkwrap files as "binary" because Git cannot safely merge them. (The pnpm-lockfile.yaml stores a directed-acyclic-graph data structure whose internal consistency can be easily corrupted by a simpleminded Git merge.)

Thus, the shrinkwrap file becomes a sort of mutex: All PRs that touch it will have to go in one at a time, waiting their turn. And it's psychologically the worst kind of lineup, because a Skinner-box gamble occurs at each step: After one PR merges into master, now there's a race to see which person can rush update, push their change, and merge their PR next. This situation is practically nonexistent in smaller monorepos. They will be fine with the optimization disabled. But in a large busy monorepo it can happen often, particularly during a stampede before a ship deadline. Rush's "minimizeShrinkwrapChurn" optimization significantly mitigated this pain by significantly reducing the number of PRs that need to diff the shrinkwrap file at all.

Now, there is another better solution: For PNPM at least, Zoltan created a pnpm/merge-driver package that enables a CI job to intelligently merge shrinkwrap diffs without having to run rush update. Yeah!

Unfortunately, we were not able to figure out how to use pnpm/merge-driver, because of a seeming limitation of GitHub and Azure DevOps: After the CI job clones the PR branch, it locally merges master into the working folder, to ensure the thing being built is as current as possible. This is a hardcoded build step in the CI system. (It sort of makes sense, as the web page UI wants to display whether the merge is blocked or not, so it needs to manage that operation.) Anyway, immediately before the merging happens, we would need the CI job to (1) install the pnpm/merge-driver software and (2) configure Git to use it. We could not figure out a way to do that; however, I wouldn't say we investigated it very deeply.

Also, that was years ago. If someone can figure out how to use pnpm/merge-driver, then perhaps we could indeed eliminate Rush's "minimizeShrinkwrapChurn" optimization entirely.

[sachinjoseph]

Thanks @octogonz for explaining this in detail. I like the idea of turning off shrinkwrap churn optimization as an opt-in for Rush 5.x. Let me investigate further if there's a way to use merge drivers on the server side for PRs. I have also asked this on Stack Overflow here.

I think we could also come up with a CI job (workaround?) that helps with the shrinkwrap merge conflicts. The job outline would look like this:

1. Checkout the PR branch to the agent (shallow clone?)
2. Configure the merge driver
3. Run a git pull. This would now use the merge driver.
4. If there are no conflicts then git push the local branch to update the PR branch.

Maybe we could also configure this job to automatically kick in if the merge conflict in a PR is only on the shrinkwrap file.

On a different note, I have a question about maintaining the shrinkwrap file: If rush install is not guaranteed to use the committed shrinkwrap file, is there a reason for maintaining it in the repo in the first place? Why not make pnpm regenerate it all the time? (Versions of transitive dependencies could be locked using preferredVersions which would then be copied by Rush to common/temp/package.json.)

[sachinjoseph]

The fix for pnpm/pnpm#1883 was already published as PNPM 3.5.3. It also includes the fix for pnpm/pnpm#1882. 😊

@octogonz pnpm/pnpm#1882 still repros for me on pnpm 3.5.3. Zoltan has reopened it and created a new issue at pnpm/pnpm#1889.

@sachinjoseph regarding your repro: https://github.com/sachinjoseph/rush-pnpm-bug
...when I upgrade its rush.json to use "rushVersion": "5.9.1" and "pnpmVersion": "3.5.3", it seems to be fixed as well. react-focus-lock 1.17.7 is installed as expected. Let us know if not.

Verified that react-focus-lock 1.17.7 is installed on Rush 5.9.1/pnpm3.5.3 on my end as well. Thank you. :-)

[sachinjoseph]

@octogonz The fix to pnpm/pnpm#1876 also means that rush update is now broken. Neither rush install nor rush update will upgrade react-focus-lock to 1.19.1 with pnpm 3.5.3+, because both these rush verbs internally call pnpm install.

Rush needs to be updated in order to make rush update call pnpm update.

[octogonz]

On a different note, I have a question about maintaining the shrinkwrap file: If rush install is not guaranteed to use the committed shrinkwrap file, is there a reason for maintaining it in the repo in the first place? Why not make pnpm regenerate it all the time? (Versions of transitive dependencies could be locked using preferredVersions which would then be copied by Rush to common/temp/package.json.)

rush install /is/ guaranteed to use it, and is supposed to produce a deterministic outcome. The optimization simply allows that Rush is not required to follow the installation plan exactly.

For example, adding cool-library to project3 might shuffle its tree a bit. But how it gets shuffled should not be impacted by new versions getting published to the NPM registry, so the result is still deterministic.

To see the exact changes, you can diff common/temp/pnpm-lockfile.yaml with pnpm-lockfile-preinstall.yaml.

[sachinjoseph]

Yes, that makes sense to me.

[sachinjoseph]

The state of this issue

As discussed above, the original issue (see comment) should be unblocked (but there's a regression, see the WARNING below!) by using pnpm v3.5.3 and Rush v5.9.1. @elliottsj @mikeharder @acoates-ms Can you confirm?

WARNING: DO NOT use Rush with pnpm v3.5.3 in production! pnpm v3.5.3 has broken rush update; this would be addressed in an immediate patch release.

Closing this issue

Once a new Rush release addresses the rush update regression, I think this issue could be closed. PR: #1340

Continuing the discussion

The discussion needs to continue to address the other issues though. I will summarize them here; we could open new issues if it makes more sense.

Pins

For now let's put a pin in the following:
1. Always sync common/config/rush/pnpm-lock.yaml and common/temp/pnpm-lock.yaml. See #1300 for a detailed proposal.
2. Simplify shrinkwrap merge conflicts/enable pnpm merge driver for shrinkwrap files on the server side.

That leaves us with proposals for rush install, rush update, and rush update --full:

rush install

Outline of current flow

1. Copy common/config/rush/pnpm-lock.yaml to common/temp/pnpm-lock.yaml
2. Apply some minor transformations to common/temp/pnpm-lock.yaml.
3. Run pnpm install --no-prefer-frozen-shrinkwrap in common/temp.

Proposed change

For pnpm 3.5.3+, change the install command in Step 3 from pnpm install --no-prefer-frozen-shrinkwrap to pnpm install --frozen-lockfile

Rationale for this proposal

Prevent the underlying package manager from causing changes to the lockfile so that rush install can be used by CI systems for installing the exact plan as depicted by the shrinkwrap.

Blockers on this proposal:

1. When indirect dependencies change, "pnpm install --frozen-lockfile" fails only if node_modules and pnpm-store don't exist pnpm/pnpm#1882 / pnpm i --frozen-lockfile with mutated local tarballs should fail pnpm/pnpm#1889
2. Transformations on common/temp/pnpm-lock.yaml are often incomplete: integrity hashes are not updated when a dependency is added or a version is updated. This means that pnpm install --frozen-lockfile would fail.

TODO

1. For Blocker 1, follow up on When indirect dependencies change, "pnpm install --frozen-lockfile" fails only if node_modules and pnpm-store don't exist pnpm/pnpm#1882 / pnpm i --frozen-lockfile with mutated local tarballs should fail pnpm/pnpm#1889.
2. For Blocker 2, improve the transformations so that pnpm install --frozen-lockfile can work. Start with fixing the integrity hash.
3. Ensure that rush install behaves the same way on npm and yarn.

rush update

Outline of current flow

The rush update behavior is more or less the same as rush install's described above. In addition, rush update syncs common/temp/pnpm-lock.yaml and common/config/rush/pnpm-lock.yaml if it finds the lockfile update interesting.

1. Copy common/config/rush/pnpm-lock.yaml to common/temp/pnpm-lock.yaml
2. Apply some minor transformations to common/temp/pnpm-lock.yaml.
3. Run pnpm install --no-prefer-frozen-shrinkwrap in common/temp.
4. If the lockfile change is interesting, then sync it back from common/temp/pnpm-lock.yaml to common/config/rush/pnpm-lock.yaml

Proposed change

For 3.5.3+, change the install command in Step 3 from pnpm install --no-prefer-frozen-shrinkwrap to pnpm install

Rationale for this proposal

pnpm install (with no arguments regarding the lockfile) takes a conservative approach and minimizes changes to the lockfile by not updating versions unless it's necessary. This is what rush update is expected to do. See https://rushjs.io/pages/commands/rush_update/.

Blockers on this proposal:

None.

TODO:

1. Ensure that rush update behaves the same way on npm and yarn.

rush update --full

Outline of current flow

1. Delete common/temp/pnpm-lock.yaml (and common/temp/node_modules/pnpm-lock.yaml, see [rush] Workaround for PNPM Issue #1890 #1340)
2. Run pnpm install --no-prefer-frozen-shrinkwrap in common/temp. This generates a new shrinkwrap with all dependencies updated to their latest SemVer-compatible version.
3. Sync the lockfile back from common/temp/pnpm-lock.yaml to common/config/rush/pnpm-lock.yaml

Proposed change

For 3.5.3+, change the install command in Step 3 from pnpm install --no-prefer-frozen-shrinkwrap to pnpm install

Rationale for this proposal

While the flag --no-prefer-frozen-shrinkwrap is harmless when the shrinkwrap file doesn't exist, it should not be passed at all to pnpm in the first place for this rush scenario.

Assumption: pnpm update and pnpm install work the same way in the absence of shrinkwrap file. (To confirm, I have asked this here pnpm/pnpm#1890 (comment).)

Blockers on this proposal:

None.

TODO:

1. Follow up on When "pnpm-lock.yaml" doesn't exist, "pnpm install" should not respect "node_modules/.pnpm-lock.yaml"  pnpm/pnpm#1890 (comment).
2. Ensure that rush update --full behaves the same way on npm and yarn.

[octogonz]

integrity hashes are not updated when a dependency is added or a version is updated

Note that Rush makes modifications to the shrinkwrap file (again as part of the install optimization):

https://github.com/microsoft/web-build-tools/blob/9cf4692209f0dc090dbb80364817e5c898fcf9f8/apps/rush-lib/src/logic/pnpm/PnpmShrinkwrapFile.ts#L202-L208

If tarball hashes are a problem, we could simply delete them as we do with Yarn:
https://github.com/microsoft/web-build-tools/blob/9cf4692209f0dc090dbb80364817e5c898fcf9f8/apps/rush-lib/src/logic/yarn/YarnShrinkwrapFile.ts#L211-L226

That's usually sufficient to prevent the package manager from complaining.

[octogonz]

Rationale for this proposal

Prevent the underlying package manager from causing changes to the lockfile so that rush install can be used by CI systems for installing the exact plan as depicted by the shrinkwrap.

You are essentially proposing to permanently disable the installation optimization without any switch to turn it back on again. Some considerations:

• If we do that, there's a more code that participates in this optimization that will need to be updated/removed
• If we later decide we want the optimization after all, the change would need to be reverted. Might be safer to (1) do the merge-driver investigation first so we have a backup plan or (2) keep the optimization around behind a setting as I had suggested.

The optimization is admittedly fairly complex code and clearly brittle when PNPM changes its algorithm and file formats. It would be better to remove it. But it did work correctly when it was originally introduced, and the problem it handles is real.

[octogonz]

(BTW I will mention one other option: PNPM now has the ability to do monorepo installations itself including symlinking local projects and consolidating a common shrinkwrap file. For a long time I have wanted to get rush install out of the business of installing packages and defer this role to PNPM. Then we could ask PNPM to tackle the installation optimization problem directly, since Zoltan is in a much better position to deal with these complexities and he wants PNPM to scale. The Rush work for this is not trivial but it's not huge either. Strategically it would improve the PNPM/Rush relationship, since we would no longer be a weird tarbally scenario, and we would become compatible with all the special PNPM features like --shamefully-flatten. The Yarn/NPM backends would probably have keep the old design, though. Just something to think about. If you want to pursue it, we should open a separate GitHub issue.)

[sachinjoseph]

Prevent the underlying package manager from causing changes to the lockfile so that rush install can be used by CI systems for installing the exact plan as depicted by the shrinkwrap.

You are essentially proposing to permanently disable the installation optimization without any switch to turn it back on again. Some considerations:

No, I am not proposing that we disable the optimization. I am proposing that we prevent the underlying package manager (pnpm or yarn or npm) from making any changes to the lockfile; let Rush and only Rush deal with the transformations/optimization/modifications on the shrinkwrap. Sorry my wording wasn't clear about this.

Rationale to allow only Rush perform modifications on the shrinkwrap: Rush knows what it's doing to transform/optimize, but if Rush lets pnpm also modify the shrinkwrap then the outcome might not be deterministic.

What do you think?

[octogonz]

After Rush's modifications the shrinkwrap file may be in an inconsistent state that the package manager is expected to fix. (Not sure if this applies to PNPM but it was true for NPM)

[sachinjoseph]

If we could get the modifications to leave the shrinkwrap file in a consistent state, then we could use the --frozen-lockfile flag. If we fix (or leave out) the integrity field, other inconsistent properties, etc. - do you think the shrinkwrap would be in a consistent state? I could look into the npm scenario.

Since not using the --frozen-lockfile flag might potentially lead to a non-deterministic installation, I am strongly inclined towards using this flag in the rush install scenario.

[mikeharder]

@mikeharder regarding your repro: https://github.com/mikeharder/rush-dependency-versions

...when I upgrade its rush.json to use "rushVersion": "5.9.1" and "pnpmVersion": "3.5.3", it seems to be fixed now. (Hurray!) Let us know if not.

@octogonz: Confirmed 👍

[octogonz]

If we could get the modifications to leave the shrinkwrap file in a consistent state, then we could use the --frozen-lockfile flag. If we fix (or leave out) the integrity field, other inconsistent properties, etc. - do you think the shrinkwrap would be in a consistent state? I could look into the npm scenario.

Since not using the --frozen-lockfile flag might potentially lead to a non-deterministic installation, I am strongly inclined towards using this flag in the rush install scenario.

As long as we invoke PNPM in a mode that will report an error if the shrinkwrap is in an inconsistent state, we'll know if it's not working. Maybe we should just give it a try.

Implementing #1300 is also a really good idea. It's not a whole lot of work. I tagged it as "Effort: Medium" only because it will touches a number of files and requires some basic familiarity with the installation logic.

[sachinjoseph]

This issue occurs due to bugs in PNPM and Rush which were addressed since this issue was opened. I verified that the repro provided no longer repros on Rush v5.10.0 and PNPM v3.5.3.

I will open a new issue to discuss the additional proposals in this thread. Closing this issue.