Skip to content

[rush] Add PNPM global virtual store support via RUSH_PNPM_ENABLE_GLOBAL_VIRTUAL_STORE - #6005

Open
Jérémy Dieuze (QuanticPotatoes) wants to merge 3 commits into
microsoft:mainfrom
QuanticPotatoes:feature/d-11160-pnpm-gvs-pr
Open

[rush] Add PNPM global virtual store support via RUSH_PNPM_ENABLE_GLOBAL_VIRTUAL_STORE#6005
Jérémy Dieuze (QuanticPotatoes) wants to merge 3 commits into
microsoft:mainfrom
QuanticPotatoes:feature/d-11160-pnpm-gvs-pr

Conversation

@QuanticPotatoes

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #5830 (currently conflicts with main). Original work by EscapeB (@EscapeB); this PR rebases it onto current main and keeps the env-var design from that review.

When RUSH_PNPM_ENABLE_GLOBAL_VIRTUAL_STORE=1, workspace installs set enableGlobalVirtualStore: true in the generated pnpm-workspace.yaml. Package instances live in the shared PNPM store instead of being recreated under each worktree's common/temp/node_modules/.pnpm.

The lockfile does not change. The package-manager install-lock skip stays in #5844.

Details

  • Opt-in via RUSH_PNPM_ENABLE_GLOBAL_VIRTUAL_STORE, not pnpm-config.json. A per-subspace config flag would allow mixed layouts in one repo (called out on [rush] Add PNPM global virtual store support for rush install #5830).
  • Requires a shared store: pnpmStore global, or RUSH_PNPM_STORE_PATH pointing outside the repo. A worktree-local store throws. A path override inside the repo warns and still enables the flag (store stays local, so no worktree win).
  • Requires PNPM >= 10.12.1.
  • Incompatible with usePnpmSyncForInjectedDependencies (pnpm-sync still assumes node_modules/.pnpm in the worktree).
  • Last-install flag records the setting so flipping it forces a reinstall.
  • Adapted to the current PnpmWorkspaceFile / EnvironmentConfiguration shape (public workspace fields, saveAsync, module-level env state, globalPnpmfile).

How it was tested

  • Existing unit tests from [rush] Add PNPM global virtual store support for rush install #5830, updated for the current APIs (PnpmWorkspaceFile.test.ts, WorkspaceInstallManager.test.ts, BaseInstallManager.test.ts, EnvironmentConfiguration.test.ts, LastInstallFlag.test.ts).
  • rush-package-manager-integration-test coverage from [rush] Add PNPM global virtual store support for rush install #5830 kept.
  • Dogfood on a 7-subspace Rush repo that already uses RUSH_PNPM_STORE_PATH (shared content store). The four installed subspaces still keep a worktree-local virtual store: 5.4 GB / 531k inodes under node_modules/.pnpm (api alone: 2.0 GB / 204k inodes / 2606 instances). With this PR and RUSH_PNPM_ENABLE_GLOBAL_VIRTUAL_STORE=1, rush install --subspace api wrote enableGlobalVirtualStore: true. Worktree common/temp/api/node_modules/.pnpm dropped to 1.5 MB / 2354 inodes (2 top-level entries). Package instances moved to ~/.rush-pnpm-store/v10/links (+2.0 GB). A second worktree on the same store installed in 6.2s; store size did not grow; .pnpm stayed 1.5 MB. Lockfile unchanged.

EscapeB (@EscapeB), happy to close this if you already have a rebase in flight. Otherwise this is meant to land #5830.

Fixes #5830

When RUSH_PNPM_ENABLE_GLOBAL_VIRTUAL_STORE=1, workspace installs set
enableGlobalVirtualStore in the generated pnpm-workspace.yaml so package
instances live in the shared PNPM store instead of each worktree's
node_modules/.pnpm.

Follow-up to microsoft#5830, rebased onto current main.

Co-authored-by: chenzhelong.sirius <EscapeB@users.noreply.github.com>
microsoft#5994 exported this regexp for TrimRushEnvironmentVariablesPlugin.
The GVS follow-up accidentally dropped it and inlined /^RUSH_/i.
* If true, enables PNPM's global virtual store during workspace installs.
* See {@link EnvironmentVariableNames.RUSH_PNPM_ENABLE_GLOBAL_VIRTUAL_STORE}
*/
public static get pnpmGlobalVirtualStore(): boolean {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
public static get pnpmGlobalVirtualStore(): boolean {
public static get enablePnpmGlobalVirtualStore(): boolean {

Along with renaming _pnpmGlobalVirtualStore. As-written, this sounds like it may be a path.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Renamed the getter to enablePnpmGlobalVirtualStore and the backing variable to _enablePnpmGlobalVirtualStore, and updated callers, tests, and the generated API report in bed4be5.

const oldPnpmGlobalVirtualStore: boolean = oldState.pnpmGlobalVirtualStore === true;
const newPnpmGlobalVirtualStore: boolean = newState.pnpmGlobalVirtualStore === true;
if (oldPnpmGlobalVirtualStore !== newPnpmGlobalVirtualStore) {
throw new Error(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pnpm can't automatically figure this out?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed that requiring an explicit purge was unnecessary. In bed4be5, switching modes now invalidates the install marker so Rush automatically uses its existing clean-install path. The early invalidation also prevents filtered-install shortcuts from accepting the old layout. Added tests for both directions, including filtered installs, and verified real PNPM 10.12.1 installs switching off and back on without --purge, with unchanged lockfile and working dependency links.

this.rushConfiguration.experimentsConfiguration.configuration?.usePnpmSyncForInjectedDependencies
});
if (globalVirtualStoreWarning) {
this._terminal.writeWarningLine(Colorize.yellow(globalVirtualStoreWarning));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Warnings are printed in yellow by default

Suggested change
this._terminal.writeWarningLine(Colorize.yellow(globalVirtualStoreWarning));
this._terminal.writeWarningLine(globalVirtualStoreWarning);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the explicit Colorize.yellow wrapper in bed4be5.

});

describe('prepareCommonTempAsync', () => {
const fixtureRepoPath: string = path.resolve(__dirname, 'repoWithSubspacesCatalogs');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
const fixtureRepoPath: string = path.resolve(__dirname, 'repoWithSubspacesCatalogs');
const fixtureRepoPath: string = `${__dirname}/repoWithSubspacesCatalogs`;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Applied the template-literal fixture path and removed the unused node:path import in bed4be5.

let originalPnpmGlobalVirtualStoreEnvValue: string | undefined;

beforeEach(() => {
originalPnpmStorePathEnvValue = process.env[EnvironmentVariableNames.RUSH_PNPM_STORE_PATH];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can process.env properties be mocked by Jest so you don't have to do this manual record+reset?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes: replaced the manual save/reset with jest.replaceProperty(process, 'env', { ...process.env }) and jest.restoreAllMocks() in bed4be5.

variant: undefined,
subspace: rushConfiguration.defaultSubspace,
terminal
} as unknown as IInstallManagerOptions;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the as unknown actually required?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removing only as unknown produces TS2352 because the fixture omits required options. In bed4be5, I filled in the required options and removed both assertions, so the fixture is now checked directly as IInstallManagerOptions.


function prepareFixtureRepo(options: { pnpmStore?: PnpmStoreLocation }): RushConfiguration {
const repoPath: string = `${tempFolderPath}/repo`;
FileSystem.copyFiles({

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use the async variants for filesystem operations.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Converted the fixture filesystem operations to their async variants, including setup, teardown, copy, JSON reads/writes, and workspace-file read, in bed4be5.

);
}

if (options.pnpmStore === 'local' && !options.pnpmStorePathOverride) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Destructure options.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Destructured the validation options at the start of the function in bed4be5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs triage

Development

Successfully merging this pull request may close these issues.

2 participants