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] When using cobuilds, no-op operations should not be treated as uncacheable #4660
[rush] When using cobuilds, no-op operations should not be treated as uncacheable #4660
Conversation
libraries/rush-lib/src/logic/operations/CacheableOperationPlugin.ts
Outdated
Show resolved
Hide resolved
Example output with
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mechanically this seems fine, though it will result in the Rush build cache engine doing considerably more work for noops than it used to, unless we still have logic to skip the actual cache reads/writes for noops.
libraries/rush-lib/src/logic/operations/CacheableOperationPlugin.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/CacheableOperationPlugin.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/CacheableOperationPlugin.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/CacheableOperationPlugin.ts
Outdated
Show resolved
Hide resolved
@dmichon-msft I think no-ops should still be skipped bc the null operation runner has |
Co-authored-by: David Michon <dmichon@microsoft.com>
b9a70cf
to
3ecc6cc
Compare
@iclanton @dmichon-msft Are we ready to merge this? This PR has been open for nearly a month. |
libraries/rush-lib/src/logic/operations/CacheableOperationPlugin.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/test/BuildPlanPlugin.test.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/test/BuildPlanPlugin.test.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/test/BuildPlanPlugin.test.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/test/BuildPlanPlugin.test.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/test/__snapshots__/BuildPlanPlugin.test.ts.snap
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/test/__snapshots__/BuildPlanPlugin.test.ts.snap
Show resolved
Hide resolved
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
libraries/rush-lib/src/logic/operations/test/BuildPlanPlugin.test.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/test/BuildPlanPlugin.test.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/test/BuildPlanPlugin.test.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/test/BuildPlanPlugin.test.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/test/BuildPlanPlugin.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
…est.ts Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
libraries/rush-lib/src/logic/operations/test/BuildPlanPlugin.test.ts
Outdated
Show resolved
Hide resolved
…est.ts Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
7f2f698
to
399bd21
Compare
399bd21
to
ffb6427
Compare
Summary
This PR does 2 things,
ignoreMissingScript
ormissingScriptBehavior: silent
are treated as disabling the build cache causing all operations that depend on that operation to cluster. This causes some pretty extreme slow down for projects that use central dependencies that don't have build/test steps.Details
No-op operations now no longer play into build cache cluster calculations. I'm also adding some visibility to the output for cobuilds so users can understand when this is happening. This solves the initial issue I was having, but additional operations may still have problems. While this is a bug fix, there may be projects that are incorrectly set up to build with cobuilds. This change may expose that and cause additional work by repo owners. However, I expect that they thought they were cobuilding all along. The bug fix should improve performance, the extra logging may decrease improvement and may be best to set behind verbose logging.
How it was tested
Tested a few different ways,
rm -rf common/temp/build-cache && RUSH_COBUILD_CONTEXT_ID=foo REDIS_PASS=redis123 RUSH_COBUILD_RUNNER_ID=runner1 node ../../lib/runRush.js cobuild -p 10
and checking the output plan.disableBuildCacheForProject: true
to rush-project.json caused the expected drop in clusters, adding it to thee
project in thesharded-repo
project dropped the number of clusters from 227 to 127 as expected.