Skip to content
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.

Sign up for GitHub

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

Fix build cache management in workspaces #675

Merged
merged 7 commits into from
Dec 16, 2024
Merged

Fix build cache management in workspaces #675

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0a1d9bb
init
filip131311 Dec 12, 2024
01ee387
format
filip131311 Dec 12, 2024
5ca477e
CR
filip131311 Dec 13, 2024
106f3c4
Merge remote-tracking branch 'origin/main' into @Filip131311/FixFinge…
kmagiera Dec 16, 2024
feb252d
Fix review comments
kmagiera Dec 16, 2024
d265552
Make migration async
kmagiera Dec 16, 2024
7a81b19
Fix logic to work ok for Android and iOS
kmagiera Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 29 additions & 23 deletions ...ension/src/builders/PlatformBuildCache.ts → ...code-extension/src/builders/BuildCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,44 +30,34 @@ export type BuildCacheInfo = {
buildResult: AndroidBuildResult | IOSBuildResult;
};

export class PlatformBuildCache {
static instances: Record<DevicePlatform, PlatformBuildCache | undefined> = {
[DevicePlatform.Android]: undefined,
[DevicePlatform.IOS]: undefined,
};

static forPlatform(platform: DevicePlatform): PlatformBuildCache {
if (!this.instances[platform]) {
this.instances[platform] = new PlatformBuildCache(platform);
}

return this.instances[platform];
}

private constructor(private readonly platform: DevicePlatform) {}
export class BuildCache {
constructor(private readonly platform: DevicePlatform, private readonly appRoot: string) {}

get cacheKey() {
return this.platform === DevicePlatform.Android ? ANDROID_BUILD_CACHE_KEY : IOS_BUILD_CACHE_KEY;
const platformKey =
this.platform === DevicePlatform.Android ? ANDROID_BUILD_CACHE_KEY : IOS_BUILD_CACHE_KEY;
const appRoot = this.appRoot;
return appRoot + platformKey;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}`
}

}

/**
* Passed fingerprint should be calculated at the time build is started.
*/
public async storeBuild(buildFingerprint: string, build: BuildResult) {
const appPath = await getAppHash(getAppPath(build));
await extensionContext.workspaceState.update(this.cacheKey, {
await extensionContext.globalState.update(this.cacheKey, {
fingerprint: buildFingerprint,
buildHash: appPath,
buildResult: build,
});
}

public async clearCache() {
await extensionContext.workspaceState.update(this.cacheKey, undefined);
await extensionContext.globalState.update(this.cacheKey, undefined);
}

public async getBuild(currentFingerprint: string) {
const cache = extensionContext.workspaceState.get<BuildCacheInfo>(this.cacheKey);
const cache = extensionContext.globalState.get<BuildCacheInfo>(this.cacheKey);
if (!cache) {
Logger.debug("No cached build found.");
return undefined;
Expand Down Expand Up @@ -105,8 +95,7 @@ export class PlatformBuildCache {

public async isCacheStale() {
const currentFingerprint = await this.calculateFingerprint();
const { fingerprint } =
extensionContext.workspaceState.get<BuildCacheInfo>(this.cacheKey) ?? {};
const { fingerprint } = extensionContext.globalState.get<BuildCacheInfo>(this.cacheKey) ?? {};

return currentFingerprint !== fingerprint;
}
Expand Down Expand Up @@ -145,10 +134,10 @@ export class PlatformBuildCache {
const fingerprint = await runfingerprintCommand(fingerprintCommand, env);

if (!fingerprint) {
throw new Error("Failed to generate workspace fingerprint using custom script.");
throw new Error("Failed to generate application fingerprint using custom script.");
}

Logger.debug("Workspace fingerprint", fingerprint);
Logger.debug("Application fingerprint", fingerprint);
return fingerprint;
}
}
Expand All @@ -160,3 +149,20 @@ function getAppPath(build: BuildResult) {
async function getAppHash(appPath: string) {
return (await calculateMD5(appPath)).digest("hex");
}

export function migrateOldBuildCachesToNewStorage() {
const platformKeys = [ANDROID_BUILD_CACHE_KEY, IOS_BUILD_CACHE_KEY];
const appRoot = getAppRootFolder();

platformKeys.forEach((platformKey) => {
const cache = extensionContext.workspaceState.get<BuildCacheInfo>(platformKey);
if (!cache) {
return;
}

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

// remove the old cache afterwords
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// remove the old cache afterwords
// remove the old cache afterwards

extensionContext.workspaceState.update(platformKey, undefined);
});
}
4 changes: 2 additions & 2 deletions packages/vscode-extension/src/builders/BuildManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Disposable, OutputChannel, window } from "vscode";
import { PlatformBuildCache } from "./PlatformBuildCache";
import { BuildCache } from "./BuildCache";
import { AndroidBuildResult, buildAndroid } from "./buildAndroid";
import { IOSBuildResult, buildIos } from "./buildIOS";
import { DeviceInfo, DevicePlatform } from "../common/DeviceManager";
Expand Down Expand Up @@ -53,7 +53,7 @@ export class BuildManager {
});

const cancelToken = new CancelToken();
const buildCache = PlatformBuildCache.forPlatform(platform);
const buildCache = new BuildCache(platform, getAppRootFolder());

const buildApp = async () => {
const currentFingerprint = await buildCache.calculateFingerprint();
Expand Down
4 changes: 4 additions & 0 deletions packages/vscode-extension/src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { getLaunchConfiguration } from "./utilities/launchConfiguration";
import { Project } from "./project/project";
import { findFilesInWorkspace, isWorkspaceRoot } from "./utilities/common";
import { Platform } from "./utilities/platform";
import { migrateOldBuildCachesToNewStorage } from "./builders/BuildCache";

const OPEN_PANEL_ON_ACTIVATION = "open_panel_on_activation";

Expand Down Expand Up @@ -245,6 +246,9 @@ export async function activate(context: ExtensionContext) {
}
}

// this needs to be run after app root is set
migrateOldBuildCachesToNewStorage();

extensionActivated();
}

Expand Down
7 changes: 4 additions & 3 deletions packages/vscode-extension/src/project/project.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {
import { Logger } from "../Logger";
import { DeviceInfo } from "../common/DeviceManager";
import { DeviceAlreadyUsedError, DeviceManager } from "../devices/DeviceManager";
import { extensionContext } from "../utilities/extensionContext";
import { extensionContext, getAppRootFolder } from "../utilities/extensionContext";
import { IosSimulatorDevice } from "../devices/IosSimulatorDevice";
import { AndroidEmulatorDevice } from "../devices/AndroidEmulatorDevice";
import { DependencyManager } from "../dependency/DependencyManager";
Expand All @@ -31,7 +31,7 @@ import { DebugSessionDelegate } from "../debugging/DebugSession";
import { Metro, MetroDelegate } from "./metro";
import { Devtools } from "./devtools";
import { AppEvent, DeviceSession, EventDelegate } from "./deviceSession";
import { PlatformBuildCache } from "../builders/PlatformBuildCache";
import { BuildCache } from "../builders/BuildCache";
import { PanelLocation } from "../common/WorkspaceConfig";
import { activateDevice, getLicenseToken } from "../utilities/license";

Expand Down Expand Up @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const isCacheStale = await buildCache.isCacheStale();

if (isCacheStale) {
this.isCachedBuildStale = true;
Expand Down
Loading