-
Notifications
You must be signed in to change notification settings - Fork 36
feat: remote (and local) build caching #48
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
Conversation
packages/plugin-platform-android/src/lib/commands/runAndroid/runAndroid.ts
Outdated
Show resolved
Hide resolved
0f0320c to
0f73681
Compare
0f73681 to
46298ac
Compare
packages/plugin-platform-android/src/lib/commands/listAndroidTasks.ts
Outdated
Show resolved
Hide resolved
packages/plugin-platform-android/src/lib/commands/runAndroid/runAndroid.ts
Show resolved
Hide resolved
templates/rnef-template-default/.github/actions/rnef-native-fingerprint/index.mjs
Show resolved
Hide resolved
| import path from 'node:path'; | ||
| import { getProjectRoot } from '../project.js'; | ||
|
|
||
| export const LOCAL_BUILD_CACHE_DIRECTORY = '.rnef-build-cache'; |
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.
Can you use cacheManager or helpers from there? We have logic that will store the cache in:
- home/user/.cache/rnef on Linux
- /Users/User/Library/Caches/rnef on MacOS
- C:\Users\User\AppData\Local\Temp\rnef on Windows
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.
I wonder it it would not be a better idea to have the cache folder under project path, otherwise there is a risk for collision between project, etc. That would be only benefitial if there is something that can be cached BETWEEN different projects. @thymikee wdyt?
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.
I would rather suggests defining 2 types of RNEF cache:
- global: shared between RNEF projects (current
cacheManager) - project: related to given project, kept under project root
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.
Currently cache manager is project-bound (so no collisions) and I would prefer to keep it this way unless there’s clear benefit to have global cache. Most tools keep their caches outside of the project and there’s no clear winner (some in ~/.cache, some in system caches, some in os tmpdir, some even in node_modules). I’d keep it where it is right now and stick to one directory
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.
From what you pasted, the cache manager is global:
- /home/user/.cache/rnef on Linux
- /Users/User/Library/Caches/rnef on MacOS
- C:\Users\User\AppData\Local\Temp\rnef on Window
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.
Seems like this is work fairly manual, with project-specific cache keys constructed at cacheManager.get call-sites. The existing solution looks error-prone, as it uses names from package.json which might be duplicate and relies on caller constructing such project-unique key. IMO it would be better idea to keep these caches under project directory, e.g. <project-root>/.rnef/cache.
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.
Ok, let's go with .rnef/ for now
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.
Ok, changed to:
◇ Downloaded cached build: .rnef/cache/remote-build/rnef-android-debug-01a6d8b8bbda48a412cbf19738475df9efd56155/app-debug.apk
@thymikee Let me know if I should refactor cacheManager to put stuff in .rnef/cache instead of system temp directory.
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.
Let's keep cache in one place please, so yeah refactor :D
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.
I'll do it in a separate PR.
packages/plugin-platform-android/src/lib/commands/runAndroid/fetchCachedBuild.ts
Outdated
Show resolved
Hide resolved
packages/plugin-platform-android/src/lib/commands/runAndroid/fetchCachedBuild.ts
Show resolved
Hide resolved
packages/plugin-platform-apple/src/lib/commands/run/fetchCachedBuild.ts
Outdated
Show resolved
Hide resolved
# Conflicts: # packages/cli/src/lib/commands/fingerprint.ts # packages/tools/src/lib/logger.ts
# Conflicts: # eslint.config.js # pnpm-lock.yaml
# Conflicts: # packages/plugin-platform-android/src/lib/commands/listAndroidTasks.ts # packages/plugin-platform-android/src/lib/commands/runAndroid/runAndroid.ts
Summary
Scope:
--no-remote-build-cacheCLI optionUX:
Android:

iOS:

Test plan