Fix build cache management in workspaces #675

filip131311 · 2024-10-29T23:41:13Z

This PR changes a way we manage build caches. Previous approach was dependent on current vscode workspace, which led to build caches not being accessible when switching from working in the root of the workspace to the root of the application (or doing the same in reverse). The new approach saves this data in global storage, and adds a appRoot identifier to the cache key.

This PR is a dependency for #663

How Has This Been Tested:

Open a project in the root of the workspace and run some application that is part of it in the IDE.
Open IDE in the root of that application and see if the build cache is loaded.
repeat the proces in reverse
open an application that had a build made before this change and check if migration process works as expected

Verify that the build cache migration work:

Build project without these changes
Open the same project with no changes but with the new version of extension – expect the cache hit for native build

vercel · 2024-10-29T23:41:17Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
radon-ide	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 16, 2024 10:02pm

km1chno · 2024-10-30T13:05:27Z

packages/vscode-extension/src/utilities/common.ts

@@ -94,6 +94,14 @@ export function getOrCreateAppDownloadsDir() {
  return downloadsDirLocation;
 }

+export function getOrCreateBuildCachesDir() {


what do you think about creating abstract utils for app root level cache management? I believe it would be enough to support storing json data? It would be nice to use something like that in #640 to store recently opened deep links

on high level it could look something like this:

// src/utilities/appCache.ts const readAppLevelCache = (key: string): Record<string, unknown> | null => { return JSON.parse(fs.read(join(appCacheDir), key))); } const writeAppLevelCache = (key: string, data: Record<string, unknown>) => { fs.write(join(appCacheDir, key), JSON.stringify(data)); }

You are right it would be great to it build like that, thank you

kmagiera · 2024-10-30T13:25:47Z

Is this really a scenario we want to optimize for? When I work with monorepo setups I never switch between opening the workspace from different places. I either have the app bit open, or I have the whole monorepo opened. I never switch between these two approaches for a given project. Do you have any insights whether this is what people tend to do?

filip131311 · 2024-10-30T13:58:59Z

@kmagiera This PR as I eluded in description is necessary to enable switching between applications in #663, so the fact that it will help some people that like to change set up is just a bonus, but to answer your question when I talked to @jakex7 about his monorepo set up he seemed interested in being able to use booth approaches.

packages/vscode-extension/src/utilities/appCaches.ts

km1chno · 2024-11-04T08:18:54Z

packages/vscode-extension/src/utilities/appCaches.ts

+  let oldCaches;
+  try {
+    oldCaches = JSON.parse(fs.readFileSync(appCachesPath).toString());
+  } catch (e) {
+    Logger.warn(`Error parsing old caches file for app ${getAppRootFolder()}`, e);
+    return;
+  }


this logic is repeated several times, you can extract it to new function getAppCaches() or something like that (without key, it returns the whole json object)

kmagiera · 2024-11-04T10:12:48Z

packages/vscode-extension/src/utilities/appCaches.ts

+}
+
+/**
+ * This function sets app level caches from caches storage.


What does that even mean?

kmagiera · 2024-11-04T10:15:18Z

packages/vscode-extension/src/builders/BuildManager.ts

@@ -135,7 +135,7 @@ export class BuildManager {
        );
      }

-      await buildCache.storeBuild(buildFingerprint, buildResult);
+      await buildCache.storeCache(buildFingerprint, buildResult);


I don't like the naming being so mixed up in this whole PR. We should have naming that better maps to the intuition what cache is rather than trying to provide a super-accurate naming.

Here, it sound very weird that we are "storing cache" using "build cache". Intuitively, what this line does is it stores some build metadata (build result) in a cache storage. IMO "storeBuild" just much better describes what this method does than "storeCache".

kmagiera · 2024-11-04T10:15:55Z

packages/vscode-extension/src/builders/PlatformBuildCache.ts

@@ -53,21 +54,34 @@ export class PlatformBuildCache {
  /**
   * Passed fingerprint should be calculated at the time build is started.
   */
-  public async storeBuild(buildFingerprint: string, build: BuildResult) {
+  public async storeCache(buildFingerprint: string, build: BuildResult) {


why has this been renamed? The method takes fingerprint and build result that gets stored. We don't store any cache here

kmagiera · 2024-11-04T10:17:02Z

packages/vscode-extension/src/builders/PlatformBuildCache.ts

      fingerprint: buildFingerprint,
      buildHash: appPath,
      buildResult: build,
    });
+
+    setAppCache(this.cacheKey, cache);


In contrast, this method actually stores stuff, so why is it called "set" – this is a frequently used prefix for setting class members and using it for method that writes to disk is very confusing.

kmagiera · 2024-11-04T10:20:20Z

packages/vscode-extension/src/utilities/appCaches.ts

+ * @param value
+ * @returns the new value that was set
+ */
+export function setAppCache(key: string, value: string) {


this whole module doesn't have a clear contract. It seem to assume that the values it saves or retrieves are strings, but internal implementation parses those files expecting this is actually a structured data

I stores strings for given key but the whole caches file is a json

kmagiera

After reviewing this I think it'd be best to restructure this a bit. Below are some things that I think we should do:

I don't think we need the whole logic for managing app caches directory etc. We've been using workspaceState, but if we cannot rely on this data on being workspace-specific, we should instead use globalState. For the keys, we could just use the app root directory instead of a hash which should make it more readable and allow for cleaning it up at later time.
we should settle on avoiding the use of getAppRoot going forward to make it easier to support cases where we want to switch app roots. The first step we can make could be by adding appRoot as constructor parameter to PlatformBuildCache – this way the build cache can reference the right place in the storage and we don't need to add more references to getAppRoot.

kmagiera · 2024-12-13T13:16:41Z

packages/vscode-extension/src/builders/BuildCache.ts

+    const platformKey =
+      this.platform === DevicePlatform.Android ? ANDROID_BUILD_CACHE_KEY : IOS_BUILD_CACHE_KEY;
+    const appRoot = this.appRoot;
+    return appRoot + platformKey;


would prefer to add some delimiter here, would be cleaner to read in case we actually need to access this in the future.

Also, because of that, it'd be better to have the path at the end, otherwise it would be harder to get the path as you couldn't tell where the file path actually ends.

Finally, lets make this private as it is not being accessed from the outside, and I don't think it needs to be exposed.

So I'd go for:

private get cacheKey() { const keyPrefix = this.platform === DevicePlatform.Android ? ANDROID_BUILD_CACHE_KEY : IOS_BUILD_CACHE_KEY; return `${keyPrefix}:{this.appRoot}` }

kmagiera · 2024-12-13T13:18:12Z

packages/vscode-extension/src/project/project.ts

@@ -671,7 +671,8 @@ export class Project
  private checkIfNativeChanged = throttleAsync(async () => {
    if (!this.isCachedBuildStale && this.projectState.selectedDevice) {
      const platform = this.projectState.selectedDevice.platform;
-      const isCacheStale = await PlatformBuildCache.forPlatform(platform).isCacheStale();
+      const buildCache = new BuildCache(platform, getAppRootFolder());


would it be possible to restructure the code in a way that we don't have to instantiate the new cache object here? This doesn't matter at this point because the class is stateless, but may be a source of errors in the future if we end up changing that.

kmagiera · 2024-12-15T12:54:27Z

packages/vscode-extension/src/builders/BuildCache.ts

+
+    extensionContext.globalState.update(appRoot + platformKey, cache);
+
+    // remove the old cache afterwords


Suggested change

// remove the old cache afterwords

// remove the old cache afterwards

…rprintManagment

kmagiera

vercel bot deployed to Preview October 29, 2024 23:50 View deployment

vercel bot deployed to Preview October 29, 2024 23:59 View deployment

filip131311 requested a review from kmagiera October 30, 2024 11:42

km1chno reviewed Oct 30, 2024

View reviewed changes

vercel bot deployed to Preview October 31, 2024 14:56 View deployment

vercel bot deployed to Preview October 31, 2024 15:07 View deployment

vercel bot deployed to Preview October 31, 2024 15:12 View deployment

filip131311 requested a review from km1chno October 31, 2024 15:17

km1chno reviewed Nov 4, 2024

View reviewed changes

km1chno mentioned this pull request Nov 4, 2024

Add Open Deep Link modal #640

Merged

kmagiera requested changes Nov 4, 2024

View reviewed changes

vercel bot deployed to Preview November 13, 2024 13:55 View deployment

vercel bot deployed to Preview November 13, 2024 14:08 View deployment

vercel bot deployed to Preview November 13, 2024 14:10 View deployment

filip131311 requested a review from kmagiera November 13, 2024 14:58

kmagiera requested changes Dec 9, 2024

View reviewed changes

filip131311 added 2 commits December 12, 2024 12:22

init

0a1d9bb

format

01ee387

filip131311 force-pushed the @Filip131311/FixFingerprintManagment branch from cd19698 to 01ee387 Compare December 12, 2024 11:24

vercel bot deployed to Preview December 12, 2024 11:26 View deployment

kmagiera requested changes Dec 13, 2024

View reviewed changes

CR

5ca477e

vercel bot deployed to Preview December 13, 2024 15:58 View deployment

kmagiera reviewed Dec 15, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into @Filip131311/FixFinge…

106f3c4

…rprintManagment

vercel bot deployed to Preview December 16, 2024 14:47 View deployment

Fix review comments

feb252d

vercel bot deployed to Preview December 16, 2024 14:59 View deployment

Make migration async

d265552

vercel bot deployed to Preview December 16, 2024 21:57 View deployment

Fix logic to work ok for Android and iOS

7a81b19

vercel bot deployed to Preview December 16, 2024 22:02 View deployment

kmagiera approved these changes Dec 16, 2024

View reviewed changes

kmagiera merged commit 4aa4897 into main Dec 16, 2024
4 checks passed

kmagiera deleted the @Filip131311/FixFingerprintManagment branch December 16, 2024 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix build cache management in workspaces #675

Fix build cache management in workspaces #675

filip131311 commented Oct 29, 2024 •

edited by kmagiera

Loading

vercel bot commented Oct 29, 2024 •

edited

Loading

km1chno Oct 30, 2024

filip131311 Oct 30, 2024

kmagiera commented Oct 30, 2024

filip131311 commented Oct 30, 2024

km1chno Nov 4, 2024

kmagiera Nov 4, 2024

kmagiera Nov 4, 2024

kmagiera Nov 4, 2024

kmagiera Nov 4, 2024

kmagiera Nov 4, 2024

filip131311 Nov 13, 2024

kmagiera left a comment

kmagiera Dec 13, 2024

kmagiera Dec 13, 2024

kmagiera Dec 15, 2024

kmagiera left a comment


		extensionContext.globalState.update(appRoot + platformKey, cache);

		// remove the old cache afterwords

	// remove the old cache afterwords
	// remove the old cache afterwards

Fix build cache management in workspaces #675

Fix build cache management in workspaces #675

Conversation

filip131311 commented Oct 29, 2024 • edited by kmagiera Loading

How Has This Been Tested:

vercel bot commented Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmagiera commented Oct 30, 2024

filip131311 commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmagiera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmagiera left a comment

Choose a reason for hiding this comment

filip131311 commented Oct 29, 2024 •

edited by kmagiera

Loading

vercel bot commented Oct 29, 2024 •

edited

Loading