[rush] deferredInstallationScripts & --ignore-scripts #3537

chengcyber · 2022-07-14T07:36:29Z

Summary

This PR implements "two stage install" proposal.
close #3530

Details

This feature is now behind a flag in experiments.json: "deferredInstallationScripts": true

Workflow now:

Dividing "rush install/update" into two stage install when using pnpm & "useWorkspaces" on implicitly.
During two stage install, '--ignore-scripts' is pushed as a cli parameter to "pnpm install".
After "pnpm install" successfully, if "--ignore-scripts" is not specified explicitly by user, run "pnpm rebuild --pending"

Goal: faster retries for install lifecycle scripts:
If step3 fails, such as encountering an error in a "postinstall" scripts, rerun "rush install" will skip step2, which gets a faster retry on failed scripts.

How it was tested

Use rushstack repo for example

rush --debug install -t rush

two stage install works :)

rush --debug install -t rush-lib

Because -t rush-lib is a subset of -t rush, stage1(pnpm install w/ --ignore-scripts) was skipped, and stage2(pnpm rebuild --pending) was run. At the same time, there is no pending rebuild scripts to run. rebuild finished in seconds.

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

libraries/rush-lib/src/api/LastInstallFlag.ts

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

libraries/rush-lib/src/api/base/BaseFlag.ts

libraries/rush-lib/src/api/LastInstallFlag.ts

dmichon-msft · 2022-07-15T22:00:17Z

libraries/rush-lib/src/api/LastInstallFlag.ts

+          } else {
+            // full install
+            if (newState.selectedProjectNames === oldState.selectedProjectNames) {
+              return true;


It's bad for maintenance to have return true statements short circuiting the "does this object match" checker. The only early returns should be return false, and return true is the result when all the individual checks have passed.

I agree with you. but i have no idea how to do it here. Could you give some suggestions on how to change this part of code for better maintenance.

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

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

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

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

D4N14L · 2022-07-16T00:48:14Z

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

@@ -372,6 +376,10 @@ export class WorkspaceInstallManager extends BaseInstallManager {
  }

  protected async postInstallAsync(): Promise<void> {
+    if (this.twoStageInstall) {
+      this.commonTempInstallFlag.saveIfModified();


The use of this flag file feels like it's getting muddled by this change. It used to mean "everything in the repo is installed and ready to use", now it means "some subset of the repo is installed and possibly the post-install scripts have been run".

It would make sense to use this file to do more specific state tracking (I like that you're including the scope in the file now). We should also care about running the post-install scripts though since not running them could leave your repo in a less-usable state. Maybe we should check this file, and if ignoreScripts is true and this.options.ignoreScripts is false, we execute this stage, then write ignoreScripts: false in the flag file?

It kind of complicated. Say we have run rush install --ignore-scripts --to rush, the flag file records:

"selectedProjects": ["<other_deps>", ..., "rush-lib", "rush"] "ignoreScripts": true

Now, if i run rush install --to rush-lib, the stage 1 pnpm install --ignore-scripts should be skipped, since --to rush-lib is a subset of --to rush. And Rush just run stage 2 pnpm rebuild --filter rush-lib.... Now we can not mark ignoreScripts: false in the flag file. Since the selectedProjects contains "rush", which is out of range of --filter rush-lib...

One possible solution maybe is to record more information in one selected project in the flag file, use the above case as an example, the flag file will be

"selectedProjects": [ // ... other deps { packageName: 'rush-lib', ignoreScripts: false, }, { packageName: 'rush', ignoreScripts: true, // <--- because we run "pnpm rebuild --pending --filter rush-lib..." which excludes "rush" } ]

In this way, in postinstallAsync we need calculate ignoreScripts based on all selectedProjects to figure out whether we can skip "pnpm rebuild --pending"...

Do you think it's worth the efforts to implement this logic? For now, i choose an easy implementation which run "pnpm rebuild --pending [--filter <project...>]" everytime. --pending makes pnpm to decide whether there is any script we need to run.

Good question. I think keeping more detailed info on the selected projects does make sense... it allows you to be very specific about the work you're performing and when you want to perform that work. Another option IMO would be to execute the install from scratch again (ie. running pnpm install --ignore-scripts ... then pnpm rebuild ...) whenever non-scoping arguments to the install have changed.

Basically, I feel like we either need to commit to tracking each project install on a per-project basis, or we need to treat all other options as something that could affect all projects and start from scratch.

D4N14L · 2022-07-16T00:52:04Z

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

+        throw new Error(`Encounter an error when running install lifecycle scripts. error: ${err.message}`);
+      } finally {
+        // Always save after pnpm rebuild to update timestamp of last install flag file.
+        this.commonTempInstallFlag.create();


Previously, this flag would only exist if there was a successful install. Use should be updated here to keep in line with what I describe above (update the flag for ignoreScripts in the case that the post-install scripts run + succeed, or leave as-is and use it to skip the install but use to execute post-install scripts.

The reason why invoking creating last install flag here: canSkipInstall compares a timestamp(mtime of this flag file) with mtime of node_modules folder. And no matter pnpm rebuild --pending runs successfully or not, node_modules folder is touched, i.e. mtime updated. So we need to update the mtime of this flag file to make the canSkipInstall works again.

Right, but skipping an install in the event of an error doesn't make sense. It should be updated only after the command is successfully executed. If the command fails, then the state of the repo is unknown and the last-install flag should probably be deleted.

It's kindof fight against the main point of the proposal, faster retries on postinstall scripts.

Imagine

pnpm install --ignore-scripts --filter <a...>

👆 success

pnpm rebuild --pending --filter <a...>

👆 fail

In this case, if Rush.js doesn't update the last-install.flag. The "canSkipInstall" will never return true in this case. which triggers a "pnpm install --ignore-scripts --filter <a...>" again in next "rush install/update"...

but, the main point is to make "canSkipInstall" returns true in this case, and only the stage 2 command("pnpm rebuild") runs, which is the "faster retires" part...

Then in that case it should write the flag file twice, once after performing the install (with ignoreScripts: true), then once after running the post-install scripts (with ignoreScripts: false). We shouldn't be saying "yes, everything is installed correctly" regardless of success or failure, that defeats the entire purpose of the flag file.

in that case it should write the flag file twice, once after performing the install

+1 Rush install is meant to be transactionally safe. It is a common optimization for CI agents to reuse the same git clone working folder from a previous job. And it is very common for the previous rush install to have been terminated because someone pushed an update to their PR branch, which causes the previous CI job to get immediately cancelled and restarted.

iclanton · 2022-07-27T20:39:15Z

libraries/rush-lib/assets/rush-init/common/config/rush/experiments.json

+   * If true, rush install or rush update implicitly specify --ignore-scripts during pnpm install,
+   * and run install lifecycle scripts by pnpm rebuild --pending after pnpm install successfully.
+   */
+  /*[LINE "HYPOTHETICAL"]*/ "deferredInstallationScripts": true


Will this eventually be our recommended approach?

Yes, I think so

libraries/rush-lib/src/api/LastInstallFlag.ts

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

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

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

…npm repos

chengcyber · 2022-08-07T16:17:39Z

Updates 2022/8/7:

rebase latest main branch
rename pushRebuildArgs to pushPnpmRebuildCommandArgs
add a comment on Stage 1
throw error when specifying --ignore-scripts in non-pnpm repos
refactor last-install flag and add a individual isSelectedProjectInstalled method

…nstall-failures

chengcyber · 2022-12-02T08:33:00Z

Update: Sync this branch with the latest main branch

iclanton · 2022-12-09T06:01:01Z

common/changes/@microsoft/rush/feat-mitigation-postinstall-failures_2022-07-14-07-36.json

+  "changes": [
+    {
+      "packageName": "@microsoft/rush",
+      "comment": "Supports deferredInstallationScripts behind a experimental flag, which makes faster retires for install lifecycle scripts",


Suggested change

"comment": "Supports deferredInstallationScripts behind a experimental flag, which makes faster retires for install lifecycle scripts",

"comment": "Add support for a `--deferredInstallationScripts` CLI flag behind an experimental flag, which allows for faster retires of dependency install lifecycle scripts.",

Actually, there is no --defereredInstallScripts CLI flag. When turn on the experimental flag, post install scripts are invoked after pnpm install during "rush install/update"...

Dose "Add support for deferred running installation scripts behind an experimental flag, which allows for faster retires of dependency install lifecycle scripts." work for you?

…nstall-failures

chengcyber · 2022-12-16T03:57:24Z

Update:

merge the latest main branch
update change log comment

chengcyber marked this pull request as ready for review July 14, 2022 07:38

chengcyber requested review from iclanton, octogonz, apostolisms and D4N14L as code owners July 14, 2022 07:38

octogonz reviewed Jul 15, 2022

View reviewed changes

libraries/rush-lib/src/logic/base/BaseInstallManager.ts Outdated Show resolved Hide resolved

octogonz reviewed Jul 15, 2022

View reviewed changes

libraries/rush-lib/src/api/LastInstallFlag.ts Outdated Show resolved Hide resolved

octogonz reviewed Jul 15, 2022

View reviewed changes

libraries/rush-lib/src/api/LastInstallFlag.ts Outdated Show resolved Hide resolved

octogonz reviewed Jul 15, 2022

View reviewed changes

libraries/rush-lib/src/logic/installManager/WorkspaceInstallManager.ts Outdated Show resolved Hide resolved

dmichon-msft reviewed Jul 15, 2022

View reviewed changes

libraries/rush-lib/src/api/base/BaseFlag.ts Outdated Show resolved Hide resolved

dmichon-msft reviewed Jul 15, 2022

View reviewed changes

libraries/rush-lib/src/api/LastInstallFlag.ts Outdated Show resolved Hide resolved

dmichon-msft reviewed Jul 15, 2022

View reviewed changes

libraries/rush-lib/src/cli/actions/InstallAction.ts Outdated Show resolved Hide resolved

D4N14L reviewed Jul 16, 2022

View reviewed changes

chengcyber changed the title ~~feat(rush-lib): two stage install & --ignore-scripts~~ feat(rush-lib): deferredInstallationScripts & --ignore-scripts Jul 16, 2022

chengcyber changed the title ~~feat(rush-lib): deferredInstallationScripts & --ignore-scripts~~ [rush] deferredInstallationScripts & --ignore-scripts Jul 16, 2022

chengcyber requested review from octogonz, D4N14L and dmichon-msft July 16, 2022 13:48

iclanton added this to In Progress in Bug Triage Jul 18, 2022

chengcyber force-pushed the feat-mitigation-postinstall-failures branch from 0392f2e to d1529e9 Compare July 19, 2022 09:54

iclanton reviewed Jul 27, 2022

View reviewed changes

chengcyber added 7 commits August 8, 2022 00:10

feat(rush-lib): two stage install & --ignore-scripts

65e8349

chore: rush change

5231307

chore: update test snapshot

a4da1ce

feat: BaseFlag class & ts type for LastInstallFlag

e8081a2

chore: rename twoStageInstall to deferredInstallationScripts

d39b5a2

chore: generic state type for flag file

954642e

chore: logout pnpm rebuild --pending

a2d6503

chengcyber added 11 commits August 8, 2022 00:11

fix: check valid logic when full install

1a2c6c2

feat: getPnpmFilterArgumentsAsync returns selectedProjects as well

1849e57

chore: internal error

b234e3c

chore: better description for --ignore-scripts

841dde5

chore: a common method for pushing cli parameters for pnpm rebuild

d357652

chore: remove static private variables

0686ab7

chore: update snapshots

f7d7ea3

chore: rename pushRebuildArgs to pushPnpmRebuildCommandArgs

bce2545

chore: add a comment for stage 1 installation

58f62ae

feat(rush-lib): throw error when specifying --ignore-scripts in non-p…

34439ff

…npm repos

feat(rush-lib): last install flag records installed projects

7fa7ee2

chengcyber force-pushed the feat-mitigation-postinstall-failures branch from d1529e9 to 7fa7ee2 Compare August 7, 2022 16:15

Merge remote-tracking branch 'origin/main' into feat-mitigation-posti…

6094d5a

…nstall-failures

iclanton reviewed Dec 9, 2022

View reviewed changes

chengcyber added 2 commits December 16, 2022 11:54

Merge remote-tracking branch 'origin/main' into feat-mitigation-posti…

22ea049

…nstall-failures

chore: update change comment

9fd1ccd

william2958 mentioned this pull request Dec 10, 2023

[rush:subspace] Add BaseFlag and --ignore-scripts #4448

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rush] deferredInstallationScripts & --ignore-scripts #3537

[rush] deferredInstallationScripts & --ignore-scripts #3537

chengcyber commented Jul 14, 2022 •

edited

dmichon-msft Jul 15, 2022

chengcyber Jul 16, 2022

D4N14L Jul 16, 2022

chengcyber Jul 16, 2022 •

edited

D4N14L Jul 28, 2022

D4N14L Jul 16, 2022

chengcyber Jul 16, 2022

D4N14L Jul 28, 2022

chengcyber Jul 29, 2022 •

edited

D4N14L Jul 29, 2022

octogonz Jul 30, 2022

iclanton Jul 27, 2022

octogonz Jul 27, 2022

chengcyber commented Aug 7, 2022

chengcyber commented Dec 2, 2022

iclanton Dec 9, 2022

chengcyber Dec 15, 2022 •

edited

chengcyber commented Dec 16, 2022

	"comment": "Supports deferredInstallationScripts behind a experimental flag, which makes faster retires for install lifecycle scripts",
	"comment": "Add support for a `--deferredInstallationScripts` CLI flag behind an experimental flag, which allows for faster retires of dependency install lifecycle scripts.",

[rush] deferredInstallationScripts & --ignore-scripts #3537

Are you sure you want to change the base?

[rush] deferredInstallationScripts & --ignore-scripts #3537

Conversation

chengcyber commented Jul 14, 2022 • edited

Summary

Details

How it was tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chengcyber Jul 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chengcyber Jul 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chengcyber commented Aug 7, 2022

chengcyber commented Dec 2, 2022

Choose a reason for hiding this comment

chengcyber Dec 15, 2022 • edited

Choose a reason for hiding this comment

chengcyber commented Dec 16, 2022

chengcyber commented Jul 14, 2022 •

edited

chengcyber Jul 16, 2022 •

edited

chengcyber Jul 29, 2022 •

edited

chengcyber Dec 15, 2022 •

edited