Skip to content
Draft
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
96 changes: 94 additions & 2 deletions src/apphosting/localbuilds.spec.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,24 @@
import * as sinon from "sinon";
import { expect } from "chai";
import * as localBuildModule from "@apphosting/build";
import { localBuild } from "./localbuilds";
import { localBuild, runUniversalMaker } from "./localbuilds.js";
import * as secrets from "./secrets";
import { EnvMap } from "./yaml";
import * as childProcess from "child_process";
import * as fs from "fs";
import * as experiments from "../experiments";

describe("localBuild", () => {
beforeEach(() => {
sinon.stub(experiments, "isEnabled").returns(false);
});
afterEach(() => {
sinon.restore();
});

it("returns the expected output", async () => {
const bundleConfig = {
version: "v1" as const,

runConfig: {
runCommand: "npm run build:prod",
},
Expand Down Expand Up @@ -125,7 +131,7 @@
let confirmStub: sinon.SinonStub;

beforeEach(() => {
confirmStub = sinon.stub(require("../prompt"), "confirm");

Check warning on line 134 in src/apphosting/localbuilds.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Require statement not part of import statement
});

it("throws an error in non-interactive mode if build-available secrets are used without the bypass flag", async () => {
Expand Down Expand Up @@ -202,4 +208,90 @@
expect(confirmStub).to.have.been.calledOnce;
});
});

describe("runUniversalMaker", () => {
it("should successfully execute Universal Maker and parse output", () => {
process.env.UNIVERSAL_MAKER_BINARY = "/path/to/universal_maker";
const spawnResult: childProcess.SpawnSyncReturns<string> = {
pid: 12345,
output: [null, "", ""],
stdout: "",
stderr: "",
status: 0,
signal: null,
};
const spawnStub = sinon.stub(childProcess, "spawnSync").returns(spawnResult);
sinon.stub(fs, "existsSync").returns(true);
sinon.stub(fs, "readdirSync").returns(["bundle.yaml"] as unknown as any);

Check warning on line 225 in src/apphosting/localbuilds.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type

Check warning on line 225 in src/apphosting/localbuilds.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe argument of type `any` assigned to a parameter of type `Dirent<NonSharedBuffer>[]`
sinon.stub(fs, "renameSync");
sinon.stub(fs, "rmdirSync");
const readFileSyncStub = sinon.stub(fs, "readFileSync").returns(
JSON.stringify({
command: "npm",
args: ["run", "start"],
language: "nodejs",
runtime: "nodejs22",
envVars: { PORT: 3000 },
}),
);

const output = runUniversalMaker("./", "nextjs");

expect(output).to.deep.equal({
metadata: {
language: "nodejs",
runtime: "nodejs22",
framework: "nextjs",
},
runConfig: {
runCommand: "npm run start",
environmentVariables: [{ variable: "PORT", value: "3000", availability: ["RUNTIME"] }],
},
outputFiles: {
serverApp: {
include: [".apphosting"],
},
},
});

sinon.assert.calledOnce(spawnStub);
sinon.assert.calledWith(
spawnStub,
"/path/to/universal_maker",
["-application_dir", "./", "-output_dir", "./", "-output_format", "json"],
sinon.match({
env: sinon.match({
X_GOOGLE_TARGET_PLATFORM: "fah",
FIREBASE_OUTPUT_BUNDLE_DIR: "bundle_output",
}),
}),
);
sinon.assert.calledOnce(readFileSyncStub);

delete process.env.UNIVERSAL_MAKER_BINARY;
});

it("should raise clear FirebaseError when UNIVERSAL_MAKER_BINARY is undefined", () => {
delete process.env.UNIVERSAL_MAKER_BINARY;

expect(() => runUniversalMaker("./")).to.throw(
"Please specify the path to your Universal Maker binary by establishing the UNIVERSAL_MAKER_BINARY environment variable.",
);
});

it("should raise clear FirebaseError on permission errors within child execution", () => {
process.env.UNIVERSAL_MAKER_BINARY = "/path/to/universal_maker";
sinon.stub(childProcess, "spawnSync").callsFake(() => {
const err = new Error("EACCES exception") as NodeJS.ErrnoException;
err.code = "EACCES";

throw err;
});

expect(() => runUniversalMaker("./")).to.throw(
"Failed to execute the Universal Maker binary due to permission constraints. Please assure you have set chmod +x on your file.",
);
delete process.env.UNIVERSAL_MAKER_BINARY;
});
});
});
172 changes: 164 additions & 8 deletions src/apphosting/localbuilds.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,148 @@
import { BuildConfig, Env } from "../gcp/apphosting";
import * as childProcess from "child_process";
import * as fs from "fs";
import * as path from "path";
import { Availability, BuildConfig, Env } from "../gcp/apphosting";

import { localBuild as localAppHostingBuild } from "@apphosting/build";
import { EnvMap } from "./yaml";
import { loadSecret } from "./secrets";
import { confirm } from "../prompt";
import { FirebaseError } from "../error";
import * as experiments from "../experiments";
import { logger } from "../logger";
Comment thread
falahat marked this conversation as resolved.
import { wrappedSafeLoad } from "../utils";


Check failure on line 15 in src/apphosting/localbuilds.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Delete `⏎`

Check failure on line 15 in src/apphosting/localbuilds.ts

View workflow job for this annotation

GitHub Actions / unit (24)

Delete `⏎`

Check failure on line 15 in src/apphosting/localbuilds.ts

View workflow job for this annotation

GitHub Actions / unit (24)

Delete `⏎`

Check failure on line 15 in src/apphosting/localbuilds.ts

View workflow job for this annotation

GitHub Actions / unit (20)

Delete `⏎`
interface UniversalMakerOutput {
command: string;
args: string[];
language: string;
runtime: string;
envVars?: Record<string, string | number | boolean>;
}

/**
* Runs the Universal Maker binary to build the project.
*/
export function runUniversalMaker(projectRoot: string, framework?: string): AppHostingBuildOutput {
if (!process.env.UNIVERSAL_MAKER_BINARY) {
throw new FirebaseError(
"Please specify the path to your Universal Maker binary by establishing the UNIVERSAL_MAKER_BINARY environment variable.",
);
}

try {
childProcess.spawnSync(
process.env.UNIVERSAL_MAKER_BINARY,
["-application_dir", projectRoot, "-output_dir", projectRoot, "-output_format", "json"],
{
env: {
...process.env,
X_GOOGLE_TARGET_PLATFORM: "fah",
FIREBASE_OUTPUT_BUNDLE_DIR: "bundle_output",
NPM_CONFIG_REGISTRY: "https://registry.npmjs.org/",
},
stdio: "inherit",
},
);
Comment on lines +35 to +47
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The exit status of the spawnSync command should be checked. If the build fails, the process should stop and throw a FirebaseError to prevent the CLI from attempting to use incomplete or missing build artifacts. Additionally, consider removing the hardcoded NPM_CONFIG_REGISTRY as it may interfere with users' custom registry configurations.

    const result = childProcess.spawnSync(
      process.env.UNIVERSAL_MAKER_BINARY,
      ["-application_dir", projectRoot, "-output_dir", projectRoot, "-output_format", "json"],
      {
        env: {
          ...process.env,
          X_GOOGLE_TARGET_PLATFORM: "fah",
          FIREBASE_OUTPUT_BUNDLE_DIR: "bundle_output",
        },
        stdio: "inherit",
      },
    );

    if (result.status !== 0) {
      throw new FirebaseError(`Universal Maker build failed with status ${result.status}`, {
        exit: result.status ?? 1,
      });
    }


const bundleOutput = path.join(projectRoot, "bundle_output");
const targetAppHosting = path.join(projectRoot, ".apphosting");
if (fs.existsSync(bundleOutput)) {
if (!fs.existsSync(targetAppHosting)) {
fs.mkdirSync(targetAppHosting, { recursive: true });
}
const files = fs.readdirSync(bundleOutput);
for (const file of files) {
fs.renameSync(path.join(bundleOutput, file), path.join(targetAppHosting, file));
}
fs.rmdirSync(bundleOutput);
}
Comment on lines +53 to +62
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The .apphosting directory should be cleared before moving new artifacts to prevent stale files from previous builds from persisting. Also, using fs.rmSync with recursive: true is more robust than fs.rmdirSync for cleaning up the temporary output directory.

    if (fs.existsSync(bundleOutput)) {
      fs.rmSync(targetAppHosting, { recursive: true, force: true });
      fs.mkdirSync(targetAppHosting, { recursive: true });
      const files = fs.readdirSync(bundleOutput);
      for (const file of files) {
        fs.renameSync(path.join(bundleOutput, file), path.join(targetAppHosting, file));
      }
      fs.rmSync(bundleOutput, { recursive: true, force: true });
    }

} catch (e) {
if (e && typeof e === "object" && "code" in e && e.code === "EACCES") {
throw new FirebaseError(
"Failed to execute the Universal Maker binary due to permission constraints. Please assure you have set chmod +x on your file.",
);
}
throw e;
}

const outputFilePath = path.join(projectRoot, "build_output.json");
if (!fs.existsSync(outputFilePath)) {
throw new FirebaseError(
`Universal Maker did not produce the expected output file at ${outputFilePath}`,
);
}

const outputRaw = fs.readFileSync(outputFilePath, "utf-8");
let umOutput: UniversalMakerOutput;
try {
umOutput = JSON.parse(outputRaw) as UniversalMakerOutput;
} catch (e) {
throw new FirebaseError(`Failed to parse build_output.json: ${(e as Error).message}`);
}
Comment on lines +79 to +85
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The build_output.json file is a temporary metadata file and should be deleted after it has been read and parsed to keep the project directory clean.

Suggested change
const outputRaw = fs.readFileSync(outputFilePath, "utf-8");
let umOutput: UniversalMakerOutput;
try {
umOutput = JSON.parse(outputRaw) as UniversalMakerOutput;
} catch (e) {
throw new FirebaseError(`Failed to parse build_output.json: ${(e as Error).message}`);
}
const outputRaw = fs.readFileSync(outputFilePath, "utf-8");
let umOutput: UniversalMakerOutput;
try {
umOutput = JSON.parse(outputRaw) as UniversalMakerOutput;
} catch (e) {
throw new FirebaseError(`Failed to parse build_output.json: ${(e as Error).message}`);
} finally {
fs.rmSync(outputFilePath, { force: true });
}


let finalRunCommand = `${umOutput.command} ${umOutput.args.join(" ")}`;
const bundleYamlPath = path.join(projectRoot, ".apphosting", "bundle.yaml");
if (fs.existsSync(bundleYamlPath)) {
try {
const bundleRaw = fs.readFileSync(bundleYamlPath, "utf-8");
// Safely parse the YAML string
const bundleData = wrappedSafeLoad(bundleRaw);

Check warning on line 91 in src/apphosting/localbuilds.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe assignment of an `any` value

if (bundleData?.runConfig?.runCommand) {

Check warning on line 93 in src/apphosting/localbuilds.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .runConfig on an `any` value
finalRunCommand = bundleData.runConfig.runCommand;

Check warning on line 94 in src/apphosting/localbuilds.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .runConfig on an `any` value

Check warning on line 94 in src/apphosting/localbuilds.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe assignment of an `any` value
}
} catch (e) {
// Fall back gracefully if parser fails
}
}

return {
metadata: {
language: umOutput.language,
runtime: umOutput.runtime,
framework: framework || "nextjs",
},
runConfig: {
runCommand: finalRunCommand,
environmentVariables: Object.entries(umOutput.envVars || {}).map(([k, v]) => ({
variable: k,
value: String(v),
availability: ["RUNTIME"],
})),
},
outputFiles: {
serverApp: {
include: [".apphosting"],
},
},
};
}

export interface AppHostingBuildOutput {
metadata: Record<string, string | number | boolean>;

runConfig: {
runCommand?: string;
environmentVariables?: Array<{
variable: string;
value: string;
availability: string[];
}>;
};
outputFiles?: {
serverApp: {
include: string[];
};
};
}

/**
* Triggers a local build of your App Hosting codebase.
*
* This function orchestrates the build process using the App Hosting build adapter.
*
* It detects the framework (though currently defaults/assumes 'nextjs' in some contexts),
* generates the necessary build artifacts, and returns metadata about the build.
* @param projectId - The project ID to use for resolving secrets.
Expand Down Expand Up @@ -62,9 +196,26 @@
process.env[key] = value;
}

let apphostingBuildOutput;
let apphostingBuildOutput: AppHostingBuildOutput;
try {
apphostingBuildOutput = await localAppHostingBuild(projectRoot, framework);
if (experiments.isEnabled("universalMaker")) {
apphostingBuildOutput = runUniversalMaker(projectRoot, framework);
logger.debug(
`[apphosting] Universal Maker build outputFiles include: ${JSON.stringify(apphostingBuildOutput.outputFiles?.serverApp?.include ?? [])}`,
);
} else {
const buildResult = await localAppHostingBuild(projectRoot, framework);
apphostingBuildOutput = {
metadata: Object.fromEntries(
Object.entries(buildResult.metadata || {}).map(([k, v]) => [
k,
v as string | number | boolean,
]),
),
runConfig: buildResult.runConfig,
outputFiles: buildResult.outputFiles,
};
}
} finally {
for (const key in process.env) {
if (!(key in originalEnv)) {
Expand All @@ -82,11 +233,16 @@

const discoveredEnv: Env[] | undefined =
apphostingBuildOutput.runConfig.environmentVariables?.map(
({ variable, value, availability }) => ({
variable,
value,
availability,
}),
({ variable, value, availability }) => {
const validAvail = availability.filter(
(a): a is Availability => a === "BUILD" || a === "RUNTIME",
);
return {
variable,
value,
availability: validAvail,
};
},
);

return {
Expand Down
Loading
Loading