microsoft / rushstack

Monorepo for tools developed by the Rush Stack community
https://rushstack.io/
Other
5.85k stars 594 forks source link

[rush] v5.104.0 Breaks Incremental Build #4394

Closed hhanesand closed 11 months ago

hhanesand commented 11 months ago

Summary

Rush v5.104.0 breaks incremental building, at least in our project setup. On and above that version, every rush build rebuilds all packages, regardless of changes on the filesystem. On v5.103.0 and below, incremental builds work as expected.

Repro steps

Details

We use the build caching feature, but it's only enabled on CI. That means each package has a rush-project.json file with settings for the build command. I tested deleting these files, but it's not a fix.

Not sure what other information is relevant, happy to share more details.

Standard questions

Please answer these questions to help us investigate your issue more quickly:

Question Answer
@microsoft/rush globally installed version? 5.99.0
rushVersion from rush.json? 5.104.0
useWorkspaces from rush.json? true
Operating system? mac
Would you consider contributing a PR? yes
Node.js version (node -v)? v18.16.0
octogonz commented 11 months ago

Thanks for reporting this. We'll investigate asap

hhanesand commented 11 months ago

This is in a private repo, but happy to screenshare or try to provide some more details.

octogonz commented 11 months ago

It is night in Seattle's timezone right now. But tomorrow maybe the first step will be to examine the diff between these two releases and see what might have changed to explain this bug.

iclanton commented 11 months ago

@dmichon-msft - you recently changed some of the logic here I believe.

dmichon-msft commented 11 months ago

Version 5.104.0 is when cobuilds merged; we released a patch to fix at least one issue. This release included separating the build cache incremental flow from the legacy skip-based incremental flow.

Could you clarify what you mean by "build cache is only enabled in CI"? Do you mean that the CI writes to a cloud cache but everything else is only local cache? Or do you mean that the config to enable the build cache feature at all is somehow only present during CI pipelines?

hhanesand commented 11 months ago

The build cache is disabled by default:

{
  "$schema": "https://developer.microsoft.com/json-schemas/rush/v5/build-cache.schema.json",
  "buildCacheEnabled": false,
  "cacheProvider": "amazon-s3",
  "amazonS3Configuration": {
    "isCacheWriteAllowed": false,
    "s3Bucket": "xxx",
    "s3Region": "xxx"
  }
}

And the CI turns it on through the RUSH_BUILD_CACHE_ENABLED flag. Thought it might be related since enabling build caching disables incremental, but there's no logging that indicates caching is enabled 🤷

I double checked in a second monorepo in our org, and was able to reproduce there as well, but if it's a configuration thing they're bound to have quite similar setups.

I also don't have RUSH_BUILD_CACHE_ENABLED set to anything in the terminal I'm testing this in.

vjau commented 11 months ago

I was having the same problem and was trying to do a repro. My findings shows that the problem happens even with a very simple config with only one project in the repo. I can confirm that downgrading to rush 5.103 fixes the problem. I have a repro here https://github.com/vjau/rush-always-full-rebuild Latest commit is with 5.103 (working). Previous one is with 5.109.1 (not working).

bcluyse commented 11 months ago

In our project we have the same problem, the incremental builds works for 5.103. However, from 5.104 onwards it rebuilds all packages every single time.

dmichon-msft commented 11 months ago

Ok, so the issue is that the check to determine when to use the legacy skip behavior is overly strict. This condition:

https://github.com/microsoft/rushstack/blob/e5c3b2aa90b90e44d09e7964a5989c2b1c0cdb82/libraries/rush-lib/src/cli/scriptActions/PhasedScriptAction.ts#L354-L354

Should be changed thusly:

-      if (buildCacheConfiguration) {
+      if (buildCacheConfiguration?.buildCacheEnabled) {

I'll put out a PR.

octogonz commented 11 months ago

Based on some chats about "legacy skip behavior", we've also updated the docs to provide better terminology https://github.com/microsoft/rushstack-websites/pull/208

hhanesand commented 11 months ago

Nice! Thanks all ❤️

octogonz commented 11 months ago

🚀 Released with Rush 5.109.2