Remove home-rolled yarn caching #48237
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D67140004 |
NickGerleman
added a commit
to NickGerleman/react-native
that referenced
this pull request
Dec 12, 2024
Summary: Noticed this when trying to diagnose a stale caching issue, that might be related. D59917944 added logic to only do yarn caching on main, but it has some correctness issues: 1. We cache `node_modules` instead of the yarn cache, which may contain e.g. build artifacts. We really want to be caching the yarn cache, which has pristine packages before install 2. We key the cache on root `package.json`, which is missing a lot of information (both provided by the other `package.json` in the repo, but mostly, the lockfile resolution). This removes the custom logic, but only enables GHA Yarn caching on the main branch. Changelog: [Internal] Differential Revision: D67140004
NickGerleman
force-pushed
the
export-D67140004
branch
from
718ee03 to
2ff8091
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67140004 |
NickGerleman
added a commit
to NickGerleman/react-native
that referenced
this pull request
Dec 12, 2024
Summary: Noticed this when trying to diagnose what seemed like a stale caching issue. It effectively reverts D59917944. D59917944 added logic to only do yarn caching on main, but it has some correctness issues: 1. We cache `node_modules` instead of the yarn cache, which may contain e.g. build artifacts. We really want to be caching the yarn cache, which has pristine packages before install 2. We key the cache on root `package.json`, which is missing a lot of information (both provided by the other `package.json` in the repo, but mostly, the lockfile resolution). We only save cache when we're on `refs/heads/main` (so continuous builds against main), and supposedly, builds against base branch should be able to restore against those, but a recent job, with a cache key that hasn't changed in a while, failed to restore cache. So for now, I'm going back to previous method of using GHA built-in caching for this, even if it has the potential to generate more artifacts than before. Since I'm not sure this cache taking up 10-20% is unreasonable, stale cache issues are pretty insidious, and cache underuse is also expensive. Changelog: [Internal] Differential Revision: D67140004
NickGerleman
force-pushed
the
export-D67140004
branch
from
2ff8091 to
a837448
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67140004 |
Summary: Noticed this when trying to diagnose what seemed like a stale caching issue. It effectively reverts D59917944. D59917944 added logic to only do yarn caching on main, but it has some correctness issues: 1. We cache `node_modules` instead of the yarn cache, which may contain e.g. build artifacts. We really want to be caching the yarn cache, which has pristine packages before install 2. We key the cache on root `package.json`, which is missing a lot of information (both provided by the other `package.json` in the repo, but mostly, the lockfile resolution). We only save cache when we're on `refs/heads/main` (so continuous builds against main), and supposedly, builds against base branch should be able to restore against those, but recent PR jobs I have seen, where `package.json` has not changed, all have `Cache not found for input keys: node-modules-068350889e87919c1c6c2c220c8d2d92db13f38820bf2efb315d1274b97bc367` Because of the potential correctness issues, and that the strategy for limiting to main isn't correctly working, this goes back to previous solution, which may store more artifacts (but working cache should also reduce cost by making jobs run faster). Changelog: [Internal] Differential Revision: D67140004
NickGerleman
force-pushed
the
export-D67140004
branch
from
a837448 to
9fd2173
Compare
|
This pull request was exported from Phabricator. Differential Revision: D67140004 |
|
This pull request has been merged in a28867f. |
|
This pull request was successfully merged by @NickGerleman in a28867f When will my fix make it into a release? | How to file a pick request? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary: Changelog: [Internal]
Differential Revision: D67140004