Skip to content

[rush] Rework ProjectChangeAnalyzer, operation hashes#3696

Closed
dmichon-msft wants to merge 4 commits into
microsoft:mainfrom
dmichon-msft:cache-hash-cleanup
Closed

[rush] Rework ProjectChangeAnalyzer, operation hashes#3696
dmichon-msft wants to merge 4 commits into
microsoft:mainfrom
dmichon-msft:cache-hash-cleanup

Conversation

@dmichon-msft
Copy link
Copy Markdown
Contributor

@dmichon-msft dmichon-msft commented Oct 11, 2022

Summary

Performs the following major architectural rework of Rush phased commands:

  1. Restores ProjectChangeAnalyzer to a mostly synchronous API, abstracting out the contribution from rush-project.json into a filter argument
  2. Changes the hashes used by the build cache to be based on the Operation graph, rather than the project graph
  3. Moves interaction with the build cache out of ShellOperationRunner and into a separate component, so that it can be used with other custom runners
  4. Reworks the terminal pipeline to be encapsulated in OperationExecutionRecord

Details

Currently missing:

  • Support for skip detection (it is currently completely removed)

How it was tested

The test suite, local builds on another repository

for (const item of record.consumers) {
if (blockCacheWrite) {
item.runner.isCacheWriteAllowed = false;
item.isCacheWriteAllowed = false;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In this context, "blockCacheWrite" might be true if the operation failed, right? In previous implementation this also meant no downstream operations could be written to cache. Does this change that behavior?

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.

Behavior here should be preserved.

@elliot-nelson
Copy link
Copy Markdown
Collaborator

Pulled down the branch and confirmed it seems to successfully read/write cache entries (in rushstack and our local monorepo).

The move of a couple flags from the runner down to the actual tasks doesn't seem to be accompanied by any new logic, so just curious if there is an expected change in behavior there.

@dmichon-msft
Copy link
Copy Markdown
Contributor Author

After discussion with @iclanton I'll be splitting this into a few different pieces to try to scope it more cleanly. I'm aware that the current iteration works, the main gap is that it has currently completely disabled skip detection (in preparation for making it be based on build cache hashes and file mtimes).
It also came up that the change to hashing to only include upstream operations is dangerous, because you could have a phase that depends on the source files of another workspace project without depending on a phase from said project, and that would allow for hash collisions with different inputs.
Granted, there are already ways to make this happen (access a workspace file via relative path, for example), but it becomes easier to shoot yourself in the foot if npm dependencies aren't included by default.

@dmichon-msft
Copy link
Copy Markdown
Contributor Author

This PR as currently authored is now impossible due to #3824, which prevents the full hash of an operation from being calculated until all of its dependencies have executed.

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

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants