[steps] Expand legacy command/path local functions to a single build step

Author: sswrk

packages/build-tools/src/steps/__tests__/compositeFunctions.test.ts

@@ -1,3 +1,5 @@
+import { isLegacyFunctionConfig } from '@expo/eas-build-job';
+import assert from 'assert';
 import { promises as fs } from 'fs';
 import os from 'os';
 import path from 'path';
@@ -43,6 +45,7 @@ describe(buildCompositeFunctionCatalogAsync, () => {
 
     expect(Object.keys(catalog)).toEqual(['./.eas/functions/setup']);
     const action = catalog['./.eas/functions/setup'];
+    assert(!isLegacyFunctionConfig(action));
     expect(action.name).toBe('Setup');
     expect(action.runs.steps).toHaveLength(1);
     expect(action.outputs?.version.value).toBe('${{ steps.read.outputs.version }}');

packages/eas-build-job/src/__tests__/compositeFunction.test.ts

@@ -127,9 +127,11 @@ describe('LegacyFunctionConfigZ', () => {
   });
 
   it('rejects a config declaring both command and path', () => {
-    expect(parseErrorMessages(LegacyFunctionConfigZ, { command: 'echo hi', path: './fn' })).toEqual([
-      'A local function must declare either "command" (a shell script) or "path" (a JavaScript function module), not both.',
-    ]);
+    expect(parseErrorMessages(LegacyFunctionConfigZ, { command: 'echo hi', path: './fn' })).toEqual(
+      [
+        'A local function must declare either "command" (a shell script) or "path" (a JavaScript function module), not both.',
+      ]
+    );
   });
 
   it('rejects a config declaring neither command nor path', () => {

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

@@ -288,4 +288,5 @@ export function isLegacyFunctionConfig(
   return config.runs === undefined;
 }
 
-export type CompositeFunctionCatalog = Record<string, CompositeFunctionConfig>;
+/** Local functions of either shape, keyed by their normalized `uses:` path. */
+export type CompositeFunctionCatalog = Record<string, LocalFunctionConfig>;

packages/steps/src/CompositeFunctionExpander.ts

@@ -1,23 +1,26 @@
 /**
- * Expands local composite functions (`uses: ./path/to/function`) into a
- * {@link CompositeBuildStep} tree at parse time.
+ * Expands local functions (`uses: ./path/to/function`) into build steps at parse time.
  *
- * Each call becomes a node with prefixed child ids (`caller__inner`); the parser
- * flattens the tree into the workflow. Expanded steps carry a
- * {@link BuildStepCompositeFunctionScope} so `${{ steps.* }}` and `${{ inputs.* }}`
- * resolve against composite-function-local names.
+ * A composite function call becomes a {@link CompositeBuildStep} node with prefixed child ids
+ * (`caller__inner`); the parser flattens the tree into the workflow. Expanded steps carry a
+ * {@link BuildStepCompositeFunctionScope} so `${{ steps.* }}` and `${{ inputs.* }}` resolve
+ * against composite-function-local names. A single-step (`command`/`path`) function call becomes
+ * one ordinary build step keeping the caller's own id.
  */
 import {
   CompositeFunctionCatalog,
   CompositeFunctionConfig,
   FunctionStep,
+  LegacyFunctionConfig,
+  LocalFunctionConfig,
   ShellStep,
   Step,
+  isLegacyFunctionConfig,
   isStepFunctionStep,
   isStepShellStep,
 } from '@expo/eas-build-job';
 
-import { BuildFunctionById } from './BuildFunction';
+import { BuildFunction, BuildFunctionById } from './BuildFunction';
 import { BuildFunctionGroupById } from './BuildFunctionGroup';
 import { BuildStep, BuildStepOutputAccessor } from './BuildStep';
 import { BuildStepCompositeFunctionScope } from './BuildStepCompositeFunctionScope';
@@ -31,6 +34,7 @@ import {
 import { CompositeBuildStep } from './CompositeBuildStep';
 import { BuildConfigError } from './errors';
 import { duplicates } from './utils/expodash/duplicates';
+import { createBuildFunctionFromLegacyFunctionConfig } from './utils/legacyFunction';
 import {
   getLocalCompositeFunctionCallWorkingDirectoryError,
   isLocalCompositeFunctionPath,
@@ -45,12 +49,16 @@ export type FunctionMaps = {
   buildFunctionGroupById: BuildFunctionGroupById;
 };
 
-type CompositeFunctionCall = {
+export type FunctionMapsWithExpander = FunctionMaps & {
+  compositeFunctionExpander: CompositeFunctionExpander;
+};
+
+type LocalFunctionCall = {
   compositeFunctionPath: string;
-  /** Caller-assigned id used as prefix for all inner step ids. */
+  /** Caller-assigned id; for a composite function also the prefix of all inner step ids. */
   syntheticStepId: string;
   name?: string;
-  /** Caller-provided input values, consumed when composite function inputs are interpolated. */
+  /** Caller-provided input values, consumed when function inputs are interpolated. */
   callWith?: Record<string, unknown>;
   callIf?: string;
   parentScope?: BuildStepCompositeFunctionScope;
@@ -64,6 +72,9 @@ type StepOverrides = {
 };
 
 export class CompositeFunctionExpander {
+  /** Cached per path: a `BuildFunction` is stateless, and one path may be called many times. */
+  private readonly legacyFunctionByPath = new Map<string, BuildFunction>();
+
   constructor(
     private readonly ctx: BuildStepGlobalContext,
     private readonly compositeFunctionCatalog: CompositeFunctionCatalog,
@@ -78,15 +89,16 @@ export class CompositeFunctionExpander {
     return this.functionMaps.buildFunctionGroupById;
   }
 
-  public expandCompositeFunctionStep(
+  /** Steps a local function call expands to: many for a composite function, one otherwise. */
+  public expandLocalFunctionStep(
     step: FunctionStep,
-    compositeFunctionPath: string,
+    functionPath: string,
     syntheticStepId: string
-  ): CompositeBuildStep {
-    this.rejectCompositeFunctionCallWorkingDirectory(step);
-    return this.expand(
+  ): BuildStep[] {
+    this.rejectLocalFunctionCallWorkingDirectory(step);
+    const expanded = this.expandCall(
       {
-        compositeFunctionPath,
+        compositeFunctionPath: functionPath,
         syntheticStepId,
         name: step.name,
         callWith: step.with,
@@ -95,21 +107,58 @@ export class CompositeFunctionExpander {
       },
       new Set<string>()
     );
+    return expanded instanceof CompositeBuildStep ? expanded.getFlattenedSteps() : [expanded];
   }
 
   // The call step expands away; `working_directory` on it would never apply.
-  private rejectCompositeFunctionCallWorkingDirectory(step: FunctionStep): void {
+  private rejectLocalFunctionCallWorkingDirectory(step: FunctionStep): void {
     if (step.working_directory !== undefined) {
       throw new BuildConfigError(getLocalCompositeFunctionCallWorkingDirectoryError(step.uses));
     }
   }
 
+  private expandCall(call: LocalFunctionCall, visited: ReadonlySet<string>): BuildStep {
+    const localFunction = this.lookupLocalFunction(call.compositeFunctionPath);
+    if (isLegacyFunctionConfig(localFunction)) {
+      return this.createLegacyFunctionStep(call, localFunction);
+    }
+    return this.expand(call, localFunction, visited);
+  }
+
+  /**
+   * A single-step function keeps the caller's id and takes the call-site `if` as its own
+   * condition; there is no expansion scope to hang it on.
+   */
+  private createLegacyFunctionStep(
+    call: LocalFunctionCall,
+    config: LegacyFunctionConfig
+  ): BuildStep {
+    const { compositeFunctionPath } = call;
+    let buildFunction = this.legacyFunctionByPath.get(compositeFunctionPath);
+    if (!buildFunction) {
+      buildFunction = createBuildFunctionFromLegacyFunctionConfig(compositeFunctionPath, config);
+      this.legacyFunctionByPath.set(compositeFunctionPath, buildFunction);
+    }
+    return buildFunction.createBuildStepFromFunctionCall(this.ctx, {
+      id: call.syntheticStepId,
+      name: call.name,
+      callInputs: call.callWith,
+      shell: config.shell,
+      env: call.inheritedEnv,
+      ifCondition: call.callIf,
+      compositeFunctionScope: call.parentScope,
+    });
+  }
+
   // `visited.size` is the current composite function nesting depth.
-  private expand(call: CompositeFunctionCall, visited: ReadonlySet<string>): CompositeBuildStep {
+  private expand(
+    call: LocalFunctionCall,
+    compositeFunction: CompositeFunctionConfig,
+    visited: ReadonlySet<string>
+  ): CompositeBuildStep {
     const { compositeFunctionPath, syntheticStepId } = call;
     this.guardAgainstRunawayRecursion(compositeFunctionPath, visited);
 
-    const compositeFunction = this.lookupCompositeFunction(compositeFunctionPath);
     const compositeFunctionDisplayName =
       call.name ?? compositeFunction.name ?? compositeFunctionPath;
     const innerSteps = compositeFunction.runs.steps;
@@ -184,14 +233,14 @@ export class CompositeFunctionExpander {
     }
   }
 
-  private lookupCompositeFunction(compositeFunctionPath: string): CompositeFunctionConfig {
-    const compositeFunction = this.compositeFunctionCatalog[compositeFunctionPath];
-    if (!compositeFunction) {
+  private lookupLocalFunction(compositeFunctionPath: string): LocalFunctionConfig {
+    const localFunction = this.compositeFunctionCatalog[compositeFunctionPath];
+    if (!localFunction) {
       throw new BuildConfigError(
-        `Local composite function "${compositeFunctionPath}" does not exist. Expected a "function.yml" (or "function.yaml") file at "${compositeFunctionPath}" relative to the EAS project root (convention: ".eas/functions/<name>").`
+        `Local function "${compositeFunctionPath}" does not exist. Expected a "function.yml" (or "function.yaml") file at "${compositeFunctionPath}" relative to the EAS project root (convention: ".eas/functions/<name>").`
       );
     }
-    return compositeFunction;
+    return localFunction;
   }
 
   private expandInnerStep(
@@ -211,8 +260,8 @@ export class CompositeFunctionExpander {
 
     if (isStepFunctionStep(innerStep)) {
       if (isLocalCompositeFunctionPath(innerStep.uses)) {
-        this.rejectCompositeFunctionCallWorkingDirectory(innerStep);
-        return this.expandNestedCompositeFunctionCall(innerStep, {
+        this.rejectLocalFunctionCallWorkingDirectory(innerStep);
+        return this.expandNestedLocalFunctionCall(innerStep, {
           compositeFunctionPath: parseLocalCompositeFunctionPath(innerStep.uses),
           newId,
           overrides,
@@ -243,7 +292,7 @@ export class CompositeFunctionExpander {
     };
   }
 
-  private expandNestedCompositeFunctionCall(
+  private expandNestedLocalFunctionCall(
     innerStep: FunctionStep,
     {
       compositeFunctionPath,
@@ -258,8 +307,8 @@ export class CompositeFunctionExpander {
       scope: BuildStepCompositeFunctionScope;
       visited: ReadonlySet<string>;
     }
-  ): CompositeBuildStep {
-    return this.expand(
+  ): BuildStep {
+    return this.expandCall(
       {
         compositeFunctionPath,
         syntheticStepId: newId,

packages/steps/src/StepsConfigParser.ts

@@ -285,13 +285,11 @@ export class StepsConfigParser extends AbstractConfigParser {
     compositeFunctionExpander: CompositeFunctionExpander
   ): BuildStep[] {
     if (isLocalCompositeFunctionPath(step.uses)) {
-      return compositeFunctionExpander
-        .expandCompositeFunctionStep(
-          step,
-          parseLocalCompositeFunctionPath(step.uses),
-          BuildStep.getNewId(step.id)
-        )
-        .getFlattenedSteps();
+      return compositeFunctionExpander.expandLocalFunctionStep(
+        step,
+        parseLocalCompositeFunctionPath(step.uses),
+        BuildStep.getNewId(step.id)
+      );
     }
 
     const buildFunction = compositeFunctionExpander.buildFunctionById[step.uses];

packages/steps/src/__tests__/StepsConfigParser-composite-functions-expansion-test.ts

@@ -163,7 +163,7 @@ describe('StepsConfigParser local composite functions', () => {
         parseCompositeFunctions({
           steps: [{ uses: './.eas/functions/missing', id: 'x' }],
         })
-      ).rejects.toThrow(/Local composite function ".\/.eas\/functions\/missing"/);
+      ).rejects.toThrow(/Local function ".\/.eas\/functions\/missing" does not exist/);
     });
 
     it.each([

packages/steps/src/__tests__/StepsConfigParser-composite-functions-test-utils.ts

@@ -1,4 +1,4 @@
-import { CompositeFunctionCatalog, CompositeFunctionConfigZ, Step } from '@expo/eas-build-job';
+import { CompositeFunctionCatalog, LocalFunctionConfigZ, Step } from '@expo/eas-build-job';
 
 import { createGlobalContextMock } from './utils/context';
 import { BuildFunction } from '../BuildFunction';
@@ -13,7 +13,7 @@ export const SETUP = './.eas/functions/setup';
 export function makeCatalog(entries: Record<string, unknown>): CompositeFunctionCatalog {
   const catalog: CompositeFunctionCatalog = {};
   for (const [compositeFunctionPath, raw] of Object.entries(entries)) {
-    catalog[compositeFunctionPath] = CompositeFunctionConfigZ.parse(raw);
+    catalog[compositeFunctionPath] = LocalFunctionConfigZ.parse(raw);
   }
   return catalog;
 }

packages/steps/src/__tests__/StepsConfigParser-hooks-test.ts

@@ -633,6 +633,31 @@ describe('StepsConfigParser hooks with composite functions', () => {
     expect(workflow.buildSteps.map(step => step.displayName)).toEqual(['Install node modules']);
   });
 
+  it('parses a single-step function hook step into one entry with one step', async () => {
+    const workflow = await parseWorkflowAsync({
+      ctx,
+      steps: [{ uses: 'eas/install_node_modules' }],
+      hooks: {
+        before_install_node_modules: [
+          { uses: './.eas/functions/say-hi', id: 'greet', if: '${{ always() }}' },
+        ],
+      },
+      compositeFunctionCatalog: makeCatalog({
+        './.eas/functions/say-hi': { name: 'Say hi', command: 'echo hi' },
+      }),
+    });
+    const anchorHooks = [...workflow.hooksByAnchorStep.values()][0];
+    expect(anchorHooks.before).toHaveLength(1);
+    const [hookStep] = anchorHooks.before[0].steps;
+    expect(anchorHooks.before[0].steps).toHaveLength(1);
+    expect(hookStep.id).toBe('greet');
+    expect(hookStep.displayName).toBe('Say hi');
+    expect(hookStep.command).toBe('echo hi');
+    // Unlike a composite call, the authored if: lands on the step itself.
+    expect(hookStep.ifCondition).toBe('${{ always() }}');
+    expect(workflow.buildSteps.map(step => step.displayName)).toEqual(['Install node modules']);
+  });
+
   it('does not copy the if condition of a composite hook step onto the entry', async () => {
     // The authored if: is applied inside the expansion scope, not on the entry.
     const workflow = await parseWorkflowAsync({