Merge pull request #4707 from iclanton/fix-rush-install-followups

[rush] Followups to #4701.

.github/workflows/ci.yml

@@ -28,6 +28,13 @@ jobs:
     steps:
       - name: Create ~/.rush-user/settings.json
         shell: pwsh
+        # Create a .rush-user/settings.json file that looks like this:
+        #
+        # { "buildCacheFolder": "/<runner working directory>/rush-cache" }
+        #
+        # This configures the local cache to be shared between all Rush repos. This allows us to run a build in
+        # one clone of the repo (repo-a), and restore from the cache in another clone of the repo (repo-b) to test
+        # the build cache.
         run: |
           mkdir -p $HOME/.rush-user
           @{ buildCacheFolder = Join-Path ${{ github.workspace }} rush-cache } | ConvertTo-Json > $HOME/.rush-user/settings.json

common/changes/@microsoft/rush/fix-rush-install-followups_2024-05-14-23-49.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "comment": "",
+      "type": "none",
+      "packageName": "@microsoft/rush"
+    }
+  ],
+  "packageName": "@microsoft/rush",
+  "email": "iclanton@users.noreply.github.com"
+}

libraries/rush-lib/src/cli/RushPnpmCommandLineParser.ts

@@ -510,7 +510,7 @@ export class RushPnpmCommandLineParser {
       offline: false,
       collectLogFile: false,
       maxInstallAttempts: RushConstants.defaultMaxInstallAttempts,
-      pnpmFilterArguments: [],
+      pnpmFilterArgumentValues: [],
       selectedProjects: new Set(this._rushConfiguration.projects),
       checkOnly: false,
       // TODO: Support subspaces

libraries/rush-lib/src/cli/actions/BaseInstallAction.ts

@@ -24,9 +24,12 @@ import type { SelectionParameterSet } from '../parsing/SelectionParameterSet';
 import type { RushConfigurationProject } from '../../api/RushConfigurationProject';
 import type { Subspace } from '../../api/Subspace';
 
-interface ISubspaceSelectedProjectsMetadata {
+/**
+ * Temporary data structure used by `BaseInstallAction.runAsync()`
+ */
+interface ISubspaceInstallationData {
   selectedProjects: Set<RushConfigurationProject>;
-  pnpmFilterArguments: string[];
+  pnpmFilterArgumentValues: string[];
   alwaysFullInstall: boolean;
 }
 
@@ -149,35 +152,35 @@ export abstract class BaseInstallAction extends BaseRushAction {
 
     // If we are doing a filtered install and subspaces is enabled, we need to find the affected subspaces and install for all of them.
     let selectedSubspaces: Set<Subspace> | undefined;
-    const selectedProjectsMetadataBySubspace: Map<Subspace, ISubspaceSelectedProjectsMetadata> = new Map();
+    const subspaceInstallationDataBySubspace: Map<Subspace, ISubspaceInstallationData> = new Map();
     if (this.rushConfiguration.subspacesFeatureEnabled) {
       selectedSubspaces = new Set();
       const { selectedProjects } = installManagerOptions;
       if (selectedProjects.size !== this.rushConfiguration.projects.length) {
         // This is a filtered install. Go through each project, add its subspace's pnpm filter arguments
         for (const project of selectedProjects) {
           const { subspace: projectSubspace } = project;
-          let subspaceSelectedProjectsMetadata: ISubspaceSelectedProjectsMetadata | undefined =
-            selectedProjectsMetadataBySubspace.get(projectSubspace);
-          if (!subspaceSelectedProjectsMetadata) {
+          let subspaceInstallationData: ISubspaceInstallationData | undefined =
+            subspaceInstallationDataBySubspace.get(projectSubspace);
+          if (!subspaceInstallationData) {
             selectedSubspaces.add(projectSubspace);
             const alwaysFullInstall: boolean = projectSubspace.getPnpmOptions()?.alwaysFullInstall ?? false;
-            subspaceSelectedProjectsMetadata = {
+            subspaceInstallationData = {
               selectedProjects: new Set(alwaysFullInstall ? projectSubspace.getProjects() : undefined),
-              pnpmFilterArguments: [],
+              pnpmFilterArgumentValues: [],
               alwaysFullInstall
             };
-            selectedProjectsMetadataBySubspace.set(projectSubspace, subspaceSelectedProjectsMetadata);
+            subspaceInstallationDataBySubspace.set(projectSubspace, subspaceInstallationData);
           }
 
           const {
-            pnpmFilterArguments,
+            pnpmFilterArgumentValues,
             selectedProjects: subspaceSelectedProjects,
             alwaysFullInstall
-          } = subspaceSelectedProjectsMetadata;
+          } = subspaceInstallationData;
           if (!alwaysFullInstall) {
             subspaceSelectedProjects.add(project);
-            pnpmFilterArguments.push('--filter', project.packageName);
+            pnpmFilterArgumentValues.push(project.packageName);
           }
         }
       } else if (this._subspaceParameter.value) {
@@ -263,17 +266,27 @@ export abstract class BaseInstallAction extends BaseRushAction {
       if (selectedSubspaces) {
         // Run the install for each affected subspace
         for (const subspace of selectedSubspaces) {
-          const selectedProjectsMetadata: ISubspaceSelectedProjectsMetadata | undefined =
-            selectedProjectsMetadataBySubspace.get(subspace);
+          const subspaceInstallationData: ISubspaceInstallationData | undefined =
+            subspaceInstallationDataBySubspace.get(subspace);
           // eslint-disable-next-line no-console
           console.log(Colorize.green(`Installing for subspace: ${subspace.subspaceName}`));
           let installManagerOptionsForInstall: IInstallManagerOptions;
-          if (selectedProjectsMetadata) {
-            const { selectedProjects, pnpmFilterArguments } = selectedProjectsMetadata;
+          if (subspaceInstallationData) {
+            const { selectedProjects, pnpmFilterArgumentValues } = subspaceInstallationData;
             installManagerOptionsForInstall = {
               ...installManagerOptions,
               selectedProjects,
-              pnpmFilterArguments,
+              // IMPORTANT: SelectionParameterSet.getPnpmFilterArgumentValuesAsync() already calculated
+              // installManagerOptions.pnpmFilterArgumentValues using PNPM CLI operators such as "...my-app".
+              // But with subspaces, "pnpm install" can only see the subset of projects in subspace's temp workspace,
+              // therefore an operator like "--filter ...my-app" will malfunction.  As a workaround, here we are
+              // overwriting installManagerOptions.pnpmFilterArgumentValues with a flat last of project names that
+              // were calculated by Rush.
+              //
+              // TODO: If the flat list produces too many "--filter" arguments, invoking "pnpm install" will exceed
+              // the maximum command length and fail on Windows OS.  Once this is solved, we can eliminate the
+              // redundant logic from SelectionParameterSet.getPnpmFilterArgumentValuesAsync().
+              pnpmFilterArgumentValues,
               subspace
             };
           } else {

libraries/rush-lib/src/cli/actions/InstallAction.ts

@@ -65,8 +65,8 @@ export class InstallAction extends BaseInstallAction {
       maxInstallAttempts: this._maxInstallAttempts.value!,
       // These are derived independently of the selection for command line brevity
       selectedProjects,
-      pnpmFilterArguments:
-        (await this._selectionParameters?.getPnpmFilterArgumentsAsync(this._terminal)) ?? [],
+      pnpmFilterArgumentValues:
+        (await this._selectionParameters?.getPnpmFilterArgumentValuesAsync(this._terminal)) ?? [],
       checkOnly: this._checkOnlyParameter.value,
       subspace: this.getTargetSubspace(),
       beforeInstallAsync: () => this.rushSession.hooks.beforeInstall.promise(this),

libraries/rush-lib/src/cli/actions/UpdateAction.ts

@@ -96,8 +96,8 @@ export class UpdateAction extends BaseInstallAction {
       maxInstallAttempts: this._maxInstallAttempts.value!,
       // These are derived independently of the selection for command line brevity
       selectedProjects,
-      pnpmFilterArguments:
-        (await this._selectionParameters?.getPnpmFilterArgumentsAsync(this._terminal)) ?? [],
+      pnpmFilterArgumentValues:
+        (await this._selectionParameters?.getPnpmFilterArgumentValuesAsync(this._terminal)) ?? [],
       checkOnly: false,
       subspace: this.getTargetSubspace(),
 

libraries/rush-lib/src/cli/parsing/SelectionParameterSet.ts

@@ -264,16 +264,21 @@ export class SelectionParameterSet {
    * Represents the selection as `--filter` parameters to pnpm.
    *
    * @remarks
-   * This is a separate from the selection to allow the filters to be represented more concisely.
    *
-   * @see https://pnpm.js.org/en/filtering
+   * IMPORTANT: This function produces PNPM CLI operators that select projects from PNPM's temp workspace.
+   * If Rush subspaces are enabled, PNPM cannot see the complete Rush workspace, and therefore these operators
+   * would malfunction. In the current implementation, we calculate them anyway, then `BaseInstallAction.runAsync()`
+   * will overwrite `pnpmFilterArgumentValues` with a flat list of project names.  In the future, these
+   * two code paths will be combined into a single general solution.
+   *
+   * @see https://pnpm.io/filtering
    */
-  public async getPnpmFilterArgumentsAsync(terminal: ITerminal): Promise<string[]> {
+  public async getPnpmFilterArgumentValuesAsync(terminal: ITerminal): Promise<string[]> {
     const args: string[] = [];
 
     // Include exactly these projects (--only)
     for (const project of await this._evaluateProjectParameterAsync(this._onlyProject, terminal)) {
-      args.push('--filter', project.packageName);
+      args.push(project.packageName);
     }
 
     // Include all projects that depend on these projects, and all dependencies thereof
@@ -289,19 +294,19 @@ export class SelectionParameterSet {
       // --from / --from-version-policy
       Selection.expandAllConsumers(fromProjects)
     )) {
-      args.push('--filter', `${project.packageName}...`);
+      args.push(`${project.packageName}...`);
     }
 
     // --to-except
     // All projects that the project directly or indirectly declares as a dependency
     for (const project of await this._evaluateProjectParameterAsync(this._toExceptProject, terminal)) {
-      args.push('--filter', `${project.packageName}^...`);
+      args.push(`${project.packageName}^...`);
     }
 
     // --impacted-by
     // The project and all projects directly or indirectly declare it as a dependency
     for (const project of await this._evaluateProjectParameterAsync(this._impactedByProject, terminal)) {
-      args.push('--filter', `...${project.packageName}`);
+      args.push(`...${project.packageName}`);
     }
 
     // --impacted-by-except
@@ -310,7 +315,7 @@ export class SelectionParameterSet {
       this._impactedByExceptProject,
       terminal
     )) {
-      args.push('--filter', `...^${project.packageName}`);
+      args.push(`...^${project.packageName}`);
     }
 
     return args;

libraries/rush-lib/src/logic/PackageJsonUpdater.ts

@@ -290,7 +290,7 @@ export class PackageJsonUpdater {
       offline: false,
       collectLogFile: false,
       maxInstallAttempts: RushConstants.defaultMaxInstallAttempts,
-      pnpmFilterArguments: [],
+      pnpmFilterArgumentValues: [],
       selectedProjects: new Set(this._rushConfiguration.projects),
       checkOnly: false,
       subspace: subspace,

libraries/rush-lib/src/logic/base/BaseInstallManager.ts

@@ -103,8 +103,8 @@ export abstract class BaseInstallManager {
   }
 
   public async doInstallAsync(): Promise<void> {
-    const { allowShrinkwrapUpdates, selectedProjects, pnpmFilterArguments } = this.options;
-    const isFilteredInstall: boolean = pnpmFilterArguments.length > 0;
+    const { allowShrinkwrapUpdates, selectedProjects, pnpmFilterArgumentValues } = this.options;
+    const isFilteredInstall: boolean = pnpmFilterArgumentValues.length > 0;
     const useWorkspaces: boolean =
       this.rushConfiguration.pnpmOptions && this.rushConfiguration.pnpmOptions.useWorkspaces;
     // Prevent filtered installs when workspaces is disabled
@@ -660,7 +660,7 @@ ${gitLfsHookHandling}
     const {
       offline,
       collectLogFile,
-      pnpmFilterArguments,
+      pnpmFilterArgumentValues,
       onlyShrinkwrap,
       networkConcurrency,
       allowShrinkwrapUpdates
@@ -723,7 +723,7 @@ ${gitLfsHookHandling}
         args.push('--frozen-lockfile');
 
         if (
-          pnpmFilterArguments.length > 0 &&
+          pnpmFilterArgumentValues.length > 0 &&
           Number.parseInt(this.rushConfiguration.packageManagerToolVersion, 10) >= 8 // PNPM Major version 8+
         ) {
           // On pnpm@8, disable the "dedupe-peer-dependents" feature when doing a filtered CI install so that filters take effect.

libraries/rush-lib/src/logic/base/BaseInstallManagerTypes.ts

@@ -82,15 +82,17 @@ export interface IInstallManagerOptions {
   maxInstallAttempts: number;
 
   /**
-   * Filters to be passed to PNPM during installation, if applicable.
-   * These restrict the scope of a workspace installation.
+   * An array of `--filter` argument values. For example, if the array is ["a", "b"] then Rush would invoke
+   * `pnpm install --filter a --filter b` which restricts the install/update to dependencies of
+   * workspace projects "a" and "b". If the array is empty, then an unfiltered install
+   * is performed. Filtered installs have some limitations such as less comprehensive version analysis.
    *
    * @remarks
    * Note that PNPM may arbitrarily ignore `--filter` (producing an unfiltered install) in certain situations,
    * for example when `config.dedupe-peer-dependents=true` with PNPM 8.  Rush tries to circumvent this, under the
    * assumption that a user who invokes a filtered install cares more about lockfile stability than duplication.
    */
-  pnpmFilterArguments: string[];
+  pnpmFilterArgumentValues: string[];
 
   /**
    * The set of projects for which installation should be performed.

libraries/rush-lib/src/logic/installManager/WorkspaceInstallManager.ts

@@ -724,8 +724,8 @@ export class WorkspaceInstallManager extends BaseInstallManager {
         }
       }
 
-      for (const arg of this.options.pnpmFilterArguments) {
-        args.push(arg);
+      for (const arg of this.options.pnpmFilterArgumentValues) {
+        args.push('--filter', arg);
       }
     }
   }

libraries/rush-lib/src/logic/installManager/doBasicInstallAsync.ts

@@ -40,7 +40,7 @@ export async function doBasicInstallAsync(options: IRunInstallOptions): Promise<
       recheckShrinkwrap: false,
       offline: false,
       collectLogFile: false,
-      pnpmFilterArguments: [],
+      pnpmFilterArgumentValues: [],
       selectedProjects: new Set(rushConfiguration.projects),
       maxInstallAttempts: 1,
       networkConcurrency: undefined,