Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rush] feat(buildcache): improve access to operation build cache #5058

Merged
merged 11 commits into from
Jan 8, 2025

Conversation

aramissennyeydd
Copy link
Contributor

@aramissennyeydd aramissennyeydd commented Dec 26, 2024

Summary

Work towards #3994. This adds a new method to ProjectBuildCache, forOperation that generates a build cache accessor from an operation and a few additional classes, but aims to reduce the amount of boilerplate it takes to interact with the build cache.

Details

  • Moves stateHash calculation to Operation, and only sets after a calculate method is called. I was trying to mirror this as best I could to what we had before, the big issue with moving it to, say, the constructor is we'd have to add more options to allow us to calculate it. This way, we calculate both an operation and its dependencies and can cache the hash result easily on each operation itself.
  • Creates a new static method forOperation on ProjectBuildCache that should now be easier to call from rush-sdk code, assuming you've called the calculateStateHash method.

How it was tested

I ran rush build a few times to make sure that caching isn't in place on changes and is in place when nothing has changed. Are there any more in depth checks I should do?

Impacted documentation

@aramissennyeydd aramissennyeydd changed the title feat: improve access to operation build cache [rush] feat(buildcache): improve access to operation build cache Dec 26, 2024
Signed-off-by: Aramis Sennyey <[email protected]>
Copy link
Contributor

@dmichon-msft dmichon-msft left a comment

Choose a reason for hiding this comment

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

I like the idea, but using the wrong object to store the state.

libraries/rush-lib/src/logic/operations/Operation.ts Outdated Show resolved Hide resolved
libraries/rush-lib/src/logic/operations/Operation.ts Outdated Show resolved Hide resolved
libraries/rush-lib/src/logic/operations/Operation.ts Outdated Show resolved Hide resolved
libraries/rush-lib/src/logic/operations/Operation.ts Outdated Show resolved Hide resolved
libraries/rush-lib/src/logic/operations/Operation.ts Outdated Show resolved Hide resolved
libraries/rush-lib/src/logic/operations/Operation.ts Outdated Show resolved Hide resolved
@aramissennyeydd
Copy link
Contributor Author

@dmichon-msft This should be ready for another round 🙇 !

Copy link
Contributor

@dmichon-msft dmichon-msft left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just a couple of nits.

@dmichon-msft dmichon-msft merged commit 698fe5c into microsoft:main Jan 8, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

2 participants