[ENG-9500] Implement save-cache and restore-cache functions #289

khamilowicz · 2023-11-14T11:11:47Z

Why

User should be able to save and restore files in their custom builds as steps.

This is part 1 of this feature. It provides two new custom build functions: eas/save-cache and eas/restore-cache.

build:
  name: Foobar
  steps:
    - eas/checkout
    - eas/restore-cache:
      inputs:
        key: cache-key
        paths: node_modules
    - eas/install_node_module
    - eas/save-cache:
        inputs:
          key: cache-key
          paths: node_modules

Future PR will allow passing lists of files to paths parameter and define dynamic cache-key.

How

This PR implements eas/reastore-cache and eas/save-cache functions and lays foundations for future PRs.
Cache type is enhanced with downloadUrls attribute which will store mappings between cache keys and signed urls with cached files. Field will be populated by launcher.
CacheManager now accepts two parameters: build context (which is either BuildStepContext or BuildContext<Job>) and Cache. Implementation of CacheManager using dynamic cache urls will be provided in future PRs. Until this happens, for compatibility, build context is using any type.

Test Plan

These changes are not meant to provide working functionality, but to provide foundation for new GCSCacheManager implementation.

linear · 2023-11-14T11:11:49Z

ENG-9500 Caching for custom builds

codecov · 2023-11-14T11:13:43Z

Codecov Report

Attention: Patch coverage is 90.72165% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 91.43%. Comparing base (74d8c65) to head (db0ebe4).
Report is 2 commits behind head on main.

❗ Current head db0ebe4 differs from pull request most recent head fdb4f77. Consider uploading reports for the commit fdb4f77 to get more accurate results

Files	Patch %	Lines
packages/steps/src/cli/cli.ts	0.00%	3 Missing ⚠️
packages/steps/src/inputFunctions.ts	86.37%	3 Missing ⚠️
packages/steps/src/BuildStepInput.ts	93.11%	2 Missing ⚠️
packages/steps/src/utils/template.ts	96.43%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #289      +/-   ##
==========================================
+ Coverage   88.51%   91.43%   +2.92%     
==========================================
  Files          23       23              
  Lines        1114     1085      -29     
  Branches      247      229      -18     
==========================================
+ Hits          986      992       +6     
+ Misses        128       93      -35

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sjchmiela · 2024-01-08T15:56:13Z

packages/steps/src/cacheUtils.ts

+  saveCache(ctx: any, cache?: Cache): Promise<boolean | void>;
+  // TOOD: Change any to CacheableContext
+  restoreCache(ctx: any, cache: Cache): Promise<boolean | void>;


When does it return boolean and when does it return void? Suggestion: make it either one or the other.

Why saveCache doesn't require the cache argument?

void is here only to keep compatibility with GCSCacheManager in turtle (https://github.com/expo/turtle-v2/blob/9e6fd370c862869fec034d7e1be1074f7d3e3105/src/services/worker/src/CacheManager.ts#L18) and will be changed when changes to turtle land

My suggestion to make development of this feature easier will be to do as follows:

make all necessary changes in eas-build. They can break API. We shouldn't try to deploy turtle-v2 with new cache changes (bumped dependencies), but without your adjusting changes (making GCSCacheManager conform to new restoreCache.

submit a pull request with upgraded dependencies and updated turtle-v2 code.

This is the flow I used when developing generic artifacts:

made a couple breaking changes to eas-build (Refactor uploading artifacts #306, Change uploadArtifacts into uploadArtifact #309, Add support for generic artifacts #307)

updated dependencies and adjusted turtle-v2 in one PR (https://github.com/expo/turtle-v2/pull/1609).

This removes the complexity of trying to maintain two interfaces at the same time. What do you think, do you foresee any problems?

Suggestion also applies to other comments like #289 (comment).

sjchmiela · 2024-01-08T15:56:31Z

packages/steps/src/cacheUtils.ts

+  saveCache(ctx: any, cache?: Cache): Promise<boolean | void>;
+  // TOOD: Change any to CacheableContext
+  restoreCache(ctx: any, cache: Cache): Promise<boolean | void>;
+  generateUrls?: boolean;


What does this do? Is this like a feature-enabling flag?

Yes, it will tell new GCSCacheManager whether to use pregenerated upload/download urls or to generate new ones.

packages/steps/src/BuildStepContext.ts

packages/eas-build-job/src/common.ts

packages/build-tools/src/steps/functions/cache.ts

sjchmiela · 2024-01-08T16:05:20Z

packages/steps/src/cacheUtils.ts

+  disabled: boolean;
+  clear: boolean;


What do these do?

Unresolving discussion. It remains unclear what does disabled or clear mean in this context.

packages/steps/src/cacheUtils.ts

sjchmiela · 2024-01-08T16:06:07Z

packages/steps/src/cacheUtils.ts

@@ -0,0 +1,31 @@
+import { bunyan } from '@expo/logger';
+
+export interface Cache {


Do we need to have two Caches, one here and another one in eas-build-job?

It is part of the transition process: in order to avoid circular dependencies Cache should be defined in steps, but to keep compatibility I kept it in both places. It will be consolidated after changes to turtle

Is this still true after recent changes to Turtle? Why would we get a circular dependency if we used Cache from eas-build-job?

packages/build-tools/src/steps/functions/cache.ts

sjchmiela · 2024-01-30T21:10:13Z

packages/build-tools/src/customBuildContext.ts

@@ -56,6 +59,9 @@ export class CustomBuildContext implements ExternalBuildContextProvider {
    this.runtimeApi = {
      uploadArtifacts: (...args) => buildCtx['uploadArtifacts'](...args),
    };
+    this.cacheManager = buildCtx.cacheManager;


We could consider rolling cacheManager into runtimeApi for simplicity. Like, how many functions may we put into runtimeApi? I don't think the list will be too long.

khamilowicz · 2024-03-18T14:34:32Z

@sjchmiela

sjchmiela

Very cool, some questions and suggestions!

sjchmiela · 2024-05-01T10:19:42Z

packages/build-tools/src/steps/functions/cache.ts

+    ],
+    outputProviders: [
+      BuildStepOutput.createProvider({
+        id: 'cache_key',


nit: I think key should be sufficient, same for paths.

Or, we could not add it because however users are providing us key and paths, they should be able to use the same technique anywhere else? This is not a strong opinion.

sjchmiela · 2024-05-01T10:24:56Z

packages/build-tools/src/steps/functions/cache.ts

+        return;
+      }
+
+      const job = stepsCtx.global.staticContext.job as BuildJob;


If the function only makes sense for a BuildJob let's define argument to this function like

eas-build/packages/build-tools/src/steps/functions/resolveBuildConfig.ts

Lines 9 to 11 in e15bbd6

export function createResolveBuildConfigBuildFunction(

ctx: CustomBuildContext<BuildJob>

): BuildFunction {

so types require a BuildJob and we don't need to cast.

However, I think the bigger thing to update here is that I think this doesn't take into consideration updates to https://github.com/expo/turtle-v2/pull/1715, e.g. that cache download URLs are to be requested dynamically from the launcher?

However, I think the bigger thing to update here is that I think this doesn't take into consideration updates to https://github.com/expo/turtle-v2/pull/1715, e.g. that cache download URLs are to be requested dynamically from the launcher?

What makes you think so?

call to restoreCache depends on job.cache existence

restoreCache receives job.cache

I thought dynamic caches are completely separate from job.cache.

sjchmiela · 2024-05-01T10:26:16Z

packages/steps/build.sh

@@ -11,6 +11,7 @@ if [[ "$npm_lifecycle_event" == "prepack" ]]; then
  echo 'Removing "dist_commonjs" and "dist_esm" folders...'
  rm -rf dist_commonjs dist_esm
 fi
+rm -rf dist_commonjs dist_esm


Why did you need to add this? Can we remove it or submit a different pull request explaining the reason for this addition?

packages/steps/src/__tests__/BuildConfig-test.ts

packages/steps/package.json

sjchmiela · 2024-05-01T10:43:33Z

packages/steps/src/cacheUtils.ts

+  disabled: boolean;
+  clear: boolean;


Unresolving discussion. It remains unclear what does disabled or clear mean in this context.

packages/steps/src/__tests__/fixtures/build.yml

packages/steps/src/__tests__/BuildWorkflowValidator-test.ts

packages/steps/src/BuildWorkflowValidator.ts

packages/steps/src/BuildStepInput.ts

khamilowicz changed the title ~~Piotrekszeremeta/eng 9500 caching for custom builds~~ [ENG-9500] Caching for custom builds Nov 20, 2023

khamilowicz force-pushed the piotrekszeremeta/eng-9500-caching-for-custom-builds branch 11 times, most recently from 155ed7f to 569cd61 Compare November 21, 2023 12:04

khamilowicz force-pushed the piotrekszeremeta/eng-9500-caching-for-custom-builds branch 5 times, most recently from 4c39620 to 9685659 Compare January 3, 2024 13:11

khamilowicz changed the title ~~[ENG-9500] Caching for custom builds~~ [ENG-9500] Implement save-cache and restore-cache functions Jan 3, 2024

khamilowicz force-pushed the piotrekszeremeta/eng-9500-caching-for-custom-builds branch from 9685659 to 9eb71f1 Compare January 3, 2024 14:20

sjchmiela reviewed Jan 8, 2024

View reviewed changes

khamilowicz marked this pull request as ready for review January 30, 2024 13:32

khamilowicz requested a review from szdziedzic as a code owner January 30, 2024 13:32

sjchmiela reviewed Jan 30, 2024

View reviewed changes

khamilowicz force-pushed the piotrekszeremeta/eng-9500-caching-for-custom-builds branch 2 times, most recently from 6615c81 to db0ebe4 Compare March 5, 2024 17:45

khamilowicz force-pushed the piotrekszeremeta/eng-9500-caching-for-custom-builds branch 2 times, most recently from b119c69 to 9800877 Compare April 16, 2024 14:51

sjchmiela requested changes May 1, 2024

View reviewed changes

khamilowicz added 3 commits May 13, 2024 14:26

[ENG-9500] Add save and restore cache functions

01fba1e

[ENG-9500] Allow calling functions with arguments in template files

712ac20

[ENG-9500] Don't mix cacheManager with dynamic cache manager

7ff63d7

khamilowicz force-pushed the piotrekszeremeta/eng-9500-caching-for-custom-builds branch from 9800877 to a3dcacf Compare May 13, 2024 12:30

Apply review suggestions

fdb4f77

khamilowicz force-pushed the piotrekszeremeta/eng-9500-caching-for-custom-builds branch from a3dcacf to fdb4f77 Compare May 13, 2024 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENG-9500] Implement save-cache and restore-cache functions #289

[ENG-9500] Implement save-cache and restore-cache functions #289

khamilowicz commented Nov 14, 2023 •

edited

Loading

linear bot commented Nov 14, 2023

codecov bot commented Nov 14, 2023 •

edited

Loading

sjchmiela Jan 8, 2024

khamilowicz Jan 30, 2024 •

edited

Loading

sjchmiela Jan 30, 2024 •

edited

Loading

sjchmiela Jan 8, 2024

khamilowicz Jan 30, 2024

sjchmiela Jan 8, 2024

sjchmiela May 1, 2024

sjchmiela Jan 8, 2024

khamilowicz Jan 30, 2024

sjchmiela May 1, 2024

sjchmiela Jan 30, 2024

khamilowicz commented Mar 18, 2024

sjchmiela left a comment

sjchmiela May 1, 2024

sjchmiela May 1, 2024

khamilowicz May 8, 2024

sjchmiela May 8, 2024

sjchmiela May 1, 2024

sjchmiela May 1, 2024

		@@ -0,0 +1,31 @@
		import { bunyan } from '@expo/logger';

		export interface Cache {

	export function createResolveBuildConfigBuildFunction(
	ctx: CustomBuildContext<BuildJob>
	): BuildFunction {

[ENG-9500] Implement save-cache and restore-cache functions #289

Are you sure you want to change the base?

[ENG-9500] Implement save-cache and restore-cache functions #289

Conversation

khamilowicz commented Nov 14, 2023 • edited Loading

Why

How

Test Plan

linear bot commented Nov 14, 2023

codecov bot commented Nov 14, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

khamilowicz Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

sjchmiela Jan 30, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khamilowicz commented Mar 18, 2024

sjchmiela 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khamilowicz commented Nov 14, 2023 •

edited

Loading

codecov bot commented Nov 14, 2023 •

edited

Loading

khamilowicz Jan 30, 2024 •

edited

Loading

sjchmiela Jan 30, 2024 •

edited

Loading