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

[william2958]

Summary

This MR adds many useful changes introduced in the split-workspace PR (#3481) such as the creation of the BaseFlag class as well as the addition of the --ignore-scripts parameter, which allows for installation without having to run the post-install scripts.

Note that the --ignore-scripts feature is also introduced here: #3537 but the PR is a bit outdated but it was also referenced for this PR.

Both of these features will be very helpful for the installation implementation of subspaces, so I thought it would be easier to introduce them first.

Details

How it was tested

Impacted documentation

[iclanton]

Is there no schema for this file? Surprised this is allowed without upgrading Rush.

[octogonz]

It works because it's commented out. I'm going to temporarily delete this, since our convention is to introduce these lines when upgrading Rush itself. For this PR the appropriate place to add it is in the the rush-init template.

[octogonz]

Fixed

[iclanton]

Seems weird that this would be specified as an experiment.

[william2958]

Should this be added as an official feature instead?

[octogonz]

Yes, I think it can be an official feature. At the time when @chengcyber originally implemented it, it was an untested idea, but we have been using it successfully in our monorepo for over a year.

We can move this setting to pnpm-config.json

[octogonz]

Fixed

[iclanton]

Suggested change

	packageManager: PackageManagerName;
	packageManagerName: PackageManagerName;

[william2958]

This property maps to the packageManager field in the RushConfiguration class, so I think it would be confusing to call it packageManagerName here, and packageManager in rushConfig

[octogonz]

(see my other note)

[iclanton]

Suggested change

	rushJsonFolder: string;
	rushJsonFolderPath: string;

[william2958]

Same here, this maps to rushConfig's rushJsonFolder so I also think it would be confusing to call it differently in this class compared to rushconfig

[octogonz]

(see my other note)

[iclanton]

Async?

[iclanton]

This file should have an

  "additionalProperties": false,

property.

[william2958]

I believe it does at the bottom of the file, line 62

[octogonz]

Do we need this? It seems to be only used by this method of BaseFlag:

  /**
   * Merge new data into current state by "merge"
   */
  public mergeFromObject(data: JsonObject): void {
    if (isMatch(this._state, data)) {
      return;
    }
    merge(this._state, data);
    this._isModified = true;
  }

However I don't see anything calling mergeFromObject(), and the use case seems unclear. In general the last-install.flag file may have been written by a Rush release that is much older or much newer than the current Rush, which may write different fields to that file. Can we safely merge them?

[octogonz]

The constructor creates this.commonTempInstallFlag here, however further down in this file we have doInstallAsync() doing this:

    // This marker file indicates that the last "rush install" completed successfully.
    // Always perform a clean install if filter flags were provided. Additionally, if
    // "--purge" was specified, or if the last install was interrupted, then we will
    // need to perform a clean install.  Otherwise, we can do an incremental install.
    const commonTempInstallFlag: LastInstallFlag = LastInstallFlagFactory.getCommonTempFlag(
      this.rushConfiguration,
      { npmrcHash: npmrcHash || '<NO NPMRC>' }
    );

And then WorkspaceInstallManager.ts does this:

        // Always save after pnpm rebuild to update timestamp of last install flag file.
        this.commonTempInstallFlag.create();

The flag file is a synchronization mechanism -- if rush install gets killed halfway through an operation, we need to determine where we left off, and whether node_modules is in a usable state or not. That is determined by the presence/absence of the flag file, and whether its contents match Rush's current configuration. When dealing with synchronization, we can expect race conditions (code that "mostly" works but crashes randomly in situations that are very difficult to reproduce). Thus, the review of such code will generally focus on looking for guarantees of correctness, e.g. we delete the file before starting the operation, and our "finally" block guarantees its creation at the end, and the Rush commands that access this file use a reliable LockFile mutex to ensure two processes cannot ever be running simultaneously, etc.

This PR is changing the behavior of the flag file. We need some code comments explaining the new behavior of the flag file: How do we track whether install scripts have been run yet or not? Are we updating the flag file twice? I wasn't able to figure that out easily from looking at the GitHub diff.