[rush-lib] Ignore specified files when calculating project state hash #2643

elliot-nelson · 2021-04-24T15:10:54Z

Summary

Implementation of #2618. The intent is to allow each project (in its rush-project.json file) to optionally specify glob patterns that should be ignored when calculating project state, which in turn, will allow those files to be ignored for the purposes of incremental build watching, cloud build cache hits, etc.

The new option is called incrementalBuildIgnoredGlobs, and is interpreted as a dot-ignore file.

Details

PR needs a little cleanup, but it does work (tested locally on our monorepo here and does exactly what I was hoping). Hoping to get feedback before I finish up the PR, to make sure this approach makes sense.

The new model of ConfigurationFile, which helps define riggable / extendable configuration files, is all async-based and takes a Terminal for possible warning output; this means that anything above it that uses it also needs to be async and to accept a Terminal. This requires a little bit of hacking on several chains (like ProjectWatcher -> ProjectBuildCache -> PackageChangeAnalyzer etc.).

It's worth noting that ProjectBuildCache() always accepted a RushProjectConfiguration that was previously loaded, but, that wasn't that useful in this situation. That config is for the project for which a hash is being calculated; if, for example, A is dependent on B, if B specified disableBuildCache in its project configuration, that doesn't stop A from calculating the files in B as part of its hash. But for this new feature, we do need A to ignore files that B ignores, so we need to load the project configuration for every project in the repo essentially. (Or, possibly, load them on the fly as we hit projects we haven't loaded the project configuration for yet; I opted for the former approach for simplicity but we can enhance that.)

How it was tested

Currently tested in our current monorepo, for example, with project A depending on B:

Add ignoreChangedFiles: "*.md" to B's rush-project.json.
Tweak B/src/index.ts and rush build; rebuilds B and A as expected.
Tweak B/README.md and rush build; B and A get cache hits as expected.
Add ignoreChangedFiles: "*.png" to A's rush-project.json.
Tweak A/logo.png and rush build; A gets cache hit as expected.
Tweak B/logo.png and rush build; rebuilds B and A as expected (A is ignoring PNGs, but B is not)

octogonz · 2021-04-26T19:18:09Z

apps/rush-lib/src/api/RushProjectConfiguration.ts

+   * A list of file globs under the project root that will be ignored when
+   * determining whether this project has changed.
+   */
+  ignoreChangedFiles?: string[];


The phrase "changed files" sounds like it is somehow related to the rush change feature, which this isn't.

If they are globs, we should call them "globs". (Heft has some settings like "includeGlobs" and "excludeGlobs" for example.)

You suggested ignoreFilePatternsForBuildState, but that sounds limited to rush build, which this isn't. The incremental build analysis can be enabled for various Rush commands, which includes rush build but also custom commands. A user might define an incremental command like rush generate-docs and then enable this setting:

// /** // * If true then this command will be incremental like the built-in "build" command // */ // "incremental": false,

(We could provide separate ignore globs for each command, but that seems like an advanced feature that will get entangled with other design problems like phased commands. Note that phases will introduce "incremental phases" in addition to the "incremental commands" that we have today.)

My point is that "incremental" or "incremental build" is perhaps more clear than "build" by itself.

So based on all this, maybe the setting could be something like:

incrementalIgnoredGlobs

incrementalBuildIgnoredGlobs

incrementalCommandIgnoredGlobs

@iclanton what do you think?

I think incrementalBuildIgnoredGlobs is just about right. The phrase "incremental build" is the most common phrasing by far, in my opinion, so it makes sense to say "here's ignored globs for incremental build" (the fact that it applies to other commands potentially is OK, since those commands "act like incremental build" -- that's the way you'd describe them to someone).

octogonz · 2021-04-26T19:27:10Z

apps/rush-lib/src/schemas/rush-project.schema.json

+
+    "ignoreChangedFiles": {
+      "type": "array",
+      "description": "A list of file globs under the project root that will be ignored when determining whether this project has changed.",


Possible improved wording:

The incremental analyzer can skip Rush commands for projects whose input files have not changed since the last build. Normally, every Git-tracked file under the project folder is assumed to be an input. Use the incrementalBuildIgnoredGlobs setting to ignore specific file paths. The paths are specified as globs relative to the project folder.

octogonz · 2021-04-26T19:29:25Z

apps/rush-lib/src/logic/PackageChangeAnalyzer.ts

+
+      const projectConfiguration:
+        | RushProjectConfiguration
+        | undefined = await RushProjectConfiguration.tryLoadForProjectAsync(project, undefined, terminal);


@iclanton Can the rush-project.json settings be inherited from a rig? That seems useful

I can confirm that they can today, I have it working in our local monorepo with 5.43.0. 🎉

dmichon-msft · 2021-04-27T21:47:31Z

apps/rush-lib/src/logic/PackageChangeAnalyzer.ts

+
+      const projectConfiguration:
+        | RushProjectConfiguration
+        | undefined = await RushProjectConfiguration.tryLoadForProjectAsync(project, undefined, terminal);


We'd want to do an async parallel invocation if we are waiting for file reads in this loop

This function is very much critical path for all rush commands that do any work

Makes sense! I was hunting around the codebase and I just might not have found it yet -- is there a generic bottleneck / rate limiting / parallelism-limited async map somewhere in rush-lib that I can use?

(I'm pretty sure dropping a Promise.all(map) bomb is actually slower for 200+ files, so I guess I'd feel better if we could limit it to a parallelism of 16 or something.)

I was hunting around the codebase and I just might not have found it yet -- is there a generic bottleneck / rate limiting / parallelism-limited async map somewhere in rush-lib that I can use?

@elliot-nelson Weirdly no, I can't seem to find one. (Which is weird -- I'm sure someone encountered this before!) The idiomatic pattern would be an API something like:

import { Async } from '@rushstack/node-core-library'; // (or whatever the right API design is) Async.throttle(3, [ () => doAsyncThing(), () => doAsyncThing(), () => doAsyncThing(), () => doAsyncThing(), () => doAsyncThing(), () => doAsyncThing(), ] );

If you want to recommend a high quality throttling NPM package, we could use this PR to add a @beta API in @rushstack/node-core-library that wraps it.

I put up a PR with an alternate pretty quick option -- #2665.

dmichon-msft · 2021-04-27T21:49:22Z

apps/rush-lib/src/logic/PackageChangeAnalyzer.ts

@@ -92,7 +110,16 @@ export class PackageChangeAnalyzer {
        | RushConfigurationProject
        | undefined = this._rushConfiguration.findProjectForPosixRelativePath(filePath);
      if (owningProject) {
-        projectHashDeps.get(owningProject.packageName)!.set(filePath, fileHash);
+        const relativePath: string = path.relative(owningProject.projectRelativeFolder, filePath);


I'd recommend just slicing off the prefix, since owningProject will only be defined if filePath literally starts with projectRelativeFolder

Also, it keeps all paths normalized to POSIX slashes (which git returns, and which projectRelativeFolder has been normalized to).

dmichon-msft · 2021-04-27T21:51:14Z

apps/rush-lib/src/logic/PackageChangeAnalyzer.ts

@@ -92,7 +110,16 @@ export class PackageChangeAnalyzer {
        | RushConfigurationProject
        | undefined = this._rushConfiguration.findProjectForPosixRelativePath(filePath);
      if (owningProject) {
-        projectHashDeps.get(owningProject.packageName)!.set(filePath, fileHash);
+        const relativePath: string = path.relative(owningProject.projectRelativeFolder, filePath);
+        const ignoreMatchers: IMinimatch[] | undefined = ignorePatternsByProject.get(


We already have a dependency on ignore, which can run this like it is processing a .gitignore file and theoretically optimize a bit.

elliot-nelson · 2021-04-28T14:41:03Z

Updated PR title and body, and made following changes:

Renamed option to incrementalBuildIgnoredGlobs, and updated the description based on @octogonz's suggestion.
Replaced minimatch with ignore (a great change). Added a unit test and slight wording change to convey this in the description -- this actually makes the feature much more powerful, as it allows configurations like ['**', '!only-this-folder/*']. I don't know that we would necessarily highlight that fact, as a large monorepo with a bunch of fancy ignore logic is bound to start slowing down the incremental change logic.
Fixed up remaining unit tests and added rush change entry.

Took PR out of FEEDBACK state, as I think it is ready to go except for the suggested improvement of loading rush-project.json files in parallel. I can make that change if there's a good way in this codebase to load a limit of 10-16 or so files at a time.

…Globs

elliot-nelson · 2021-05-12T02:50:16Z

Latest cleanup commit -- load project configuration files in parallel, using the new Async.forEachAsync function that was ported over to the node core library.

At this point I believe all feedback has been addressed, let me know if any other concerns remain.

octogonz · 2021-05-12T23:39:55Z

apps/rush-lib/src/logic/PackageChangeAnalyzer.ts

@@ -29,9 +31,12 @@ export class PackageChangeAnalyzer {
    this._git = new Git(this._rushConfiguration);
  }

-  public getPackageDeps(projectName: string): Map<string, string> | undefined {
+  public async getPackageDeps(


This repo uses a suffix to indicate async methods, so generally a function like this should be renamed to getPackageDepsAsync() when converting. The same applies to other functions modified by this same PR.

octogonz · 2021-05-12T23:42:02Z

apps/rush-lib/src/logic/PackageChangeAnalyzer.ts

+  ): Promise<Ignore> {
+    const projectConfiguration:
+      | RushProjectConfiguration
+      | undefined = await RushProjectConfiguration.tryLoadForProjectAsync(project, undefined, terminal);


(Ideally this should be cached somewhere, but we don't need to solve that in this PR.)

I'm not sure we want to do that here; this is a wrapper for the heft-config-file library which does is own internal caching of all config files you load with it, and it seems like that's the best way to do it.

(I did file a related ticket, #2691, that will improve the caching further.)

octogonz · 2021-05-13T00:07:37Z

apps/rush-lib/src/logic/PackageChangeAnalyzer.ts

-        projectHashDeps.get(owningProject.packageName)!.set(filePath, fileHash);
+        const relativePath: string = filePath
+          .replace(owningProject.projectRelativeFolder, '')
+          .replace(/^\//, '');


Could you add a code comment explaining what transformation is happening here?

It might be helpful to include a realistic example of the input/output, something like:

// Example: // filePath = category/project/src/thing.ts // projectRelativeFolder = category/project // relativePath = src/thing.ts const relativePath: string = filePath .replace(owningProject.projectRelativeFolder, '') .replace(/^\//, '');

It seems obvious to me now, but I had to run this code in the debugger to figure out what it is doing. :-)

The correctness of replace() hinges on the assumption that filePath will always contain projectRelativeFolder, but it's difficult to prove that, since filePath and projectRelativeFolder come from fairly different subsystems.

Here's a couple ideas to protect against that:

Use path.relative() to operate on path objects rather than text strings; OR

First test whether filePath starts with projectRelativeFolder and throw an InternalError if that not. The assumption is probably safe, but if not it would be nice to hear about it so we can fix it.

(BTW I tried some experiments to see if I could break this logic, like altering the case of a project folder on Windows. But the LookupByPath API malfunctions first preventing this code from running. Maybe I'm being overly defensive -- it's just if replace() did fail to match, the behavior might be pretty difficult to understand. 🙂)

@dmichon-msft

The base logic here is that, since LookupByPath cannot return a matching project for a path that does not literally begin with projectRelativeFolder, all paths must necessarily begin with said string prefix. Personally I'd just blindly slice the expected length off of the start of the string, because that is how I prefer to do path manipulation (as quickly as possible, with a minimum of fuss).
Actually the .slice() call is safer here, because, contrary to what my memory had said, I only normalize the projectRelativeFolder to / when handing it off to LookupByPath, so you can technically (incorrectly) use \\ inside of your paths in rush.json and weird things will happen.

I've split the difference by adding a comment and converting it to use @dmichon-msft 's suggested logic.

👍 I am content that sufficient paranoia has been applied here heheh

dmichon-msft · 2021-05-19T02:35:14Z

In the repo I work on, this change has increased the run time of PackageChangeAnalyzer._getData() by an order of magnitude (from ~200 ms to 2.4 seconds). The bulk of this time appears to be in reading the rush-project.json files.

elliot-nelson requested review from apostolisms, D4N14L, halfnibble, iclanton, octogonz and sachinjoseph as code owners April 24, 2021 15:10

octogonz reviewed Apr 26, 2021

View reviewed changes

dmichon-msft reviewed Apr 27, 2021

View reviewed changes

elliot-nelson force-pushed the t3 branch from ab937ab to 9c3d626 Compare April 28, 2021 14:25

elliot-nelson requested a review from patmill as a code owner April 28, 2021 14:25

elliot-nelson force-pushed the t3 branch from 9c3d626 to eec0eda Compare April 28, 2021 14:32

elliot-nelson changed the title ~~[rush-lib] Ignore specified files when calculating project state hash (FEEDBACK ONLY)~~ [rush-lib] Ignore specified files when calculating project state hash Apr 28, 2021

[rush-lib] allow rush-project.json to specify incrementalBuildIgnored…

4f2842c

…Globs

elliot-nelson force-pushed the t3 branch from f4f86b6 to 4f2842c Compare May 12, 2021 02:36

elliot-nelson mentioned this pull request May 12, 2021

[rush] Improve heft-config-file to cache promises instead of return values #2691

Closed

octogonz reviewed May 12, 2021

View reviewed changes

octogonz reviewed May 13, 2021

View reviewed changes

Use slice() for file path operation

7f2afdd

octogonz approved these changes May 14, 2021

View reviewed changes

octogonz merged commit 9e51c9b into microsoft:master May 14, 2021

D4N14L mentioned this pull request Jul 15, 2021

[rush-lib] Fix incremental build state calculation during filtered installs #2814

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rush-lib] Ignore specified files when calculating project state hash #2643

[rush-lib] Ignore specified files when calculating project state hash #2643

elliot-nelson commented Apr 24, 2021 •

edited

Loading

octogonz Apr 26, 2021 •

edited

Loading

elliot-nelson Apr 28, 2021

octogonz Apr 26, 2021

octogonz Apr 26, 2021

elliot-nelson Apr 26, 2021

dmichon-msft Apr 27, 2021

dmichon-msft Apr 27, 2021

elliot-nelson Apr 28, 2021

octogonz Apr 30, 2021

elliot-nelson Apr 30, 2021

dmichon-msft Apr 27, 2021

dmichon-msft Apr 27, 2021

dmichon-msft Apr 27, 2021

elliot-nelson commented Apr 28, 2021

elliot-nelson commented May 12, 2021

octogonz May 12, 2021 •

edited

Loading

octogonz May 12, 2021

elliot-nelson May 13, 2021

octogonz May 13, 2021 •

edited

Loading

octogonz May 13, 2021

dmichon-msft May 13, 2021

elliot-nelson May 13, 2021

octogonz May 14, 2021

dmichon-msft commented May 19, 2021

[rush-lib] Ignore specified files when calculating project state hash #2643

[rush-lib] Ignore specified files when calculating project state hash #2643

Conversation

elliot-nelson commented Apr 24, 2021 • edited Loading

Summary

Details

How it was tested

octogonz Apr 26, 2021 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elliot-nelson commented Apr 28, 2021

elliot-nelson commented May 12, 2021

octogonz May 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octogonz May 13, 2021 • edited Loading

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

dmichon-msft commented May 19, 2021

elliot-nelson commented Apr 24, 2021 •

edited

Loading

octogonz Apr 26, 2021 •

edited

Loading

octogonz May 12, 2021 •

edited

Loading

octogonz May 13, 2021 •

edited

Loading