From 1c760dc17c9a3ecc04c62b26746102a7cf6d9320 Mon Sep 17 00:00:00 2001 From: Meghan Denny Date: Fri, 15 Mar 2024 15:59:25 -0700 Subject: [PATCH 1/2] shell: make sure proper .env is loaded in script runner --- src/bundler.zig | 18 +++++---- src/cli/run_command.zig | 31 ++------------- test/cli/install/dummy.registry.ts | 2 +- test/js/bun/shell/bunshell.test.ts | 63 +++++++++++++++++++++++++++++- 4 files changed, 76 insertions(+), 38 deletions(-) diff --git a/src/bundler.zig b/src/bundler.zig index cdbde565764f2..0a811f233268d 100644 --- a/src/bundler.zig +++ b/src/bundler.zig @@ -540,13 +540,7 @@ pub const Bundler = struct { this.options.setProduction(true); } - if (this.options.isTest() or this.env.isTest()) { - try this.env.load(dir, this.options.env.files, .@"test"); - } else if (this.options.production) { - try this.env.load(dir, this.options.env.files, .production); - } else { - try this.env.load(dir, this.options.env.files, .development); - } + try this.runEnvLoaderLoad(dir); }, .disable => { this.env.loadProcess(); @@ -573,6 +567,16 @@ pub const Bundler = struct { Analytics.disabled = Analytics.disabled or this.env.get("HYPERFINE_RANDOMIZED_ENVIRONMENT_OFFSET") != null; } + pub fn runEnvLoaderLoad(this: *Bundler, dir: *Fs.FileSystem.DirEntry) !void { + if (this.options.isTest() or this.env.isTest()) { + try this.env.load(dir, this.options.env.files, .@"test"); + } else if (this.options.production) { + try this.env.load(dir, this.options.env.files, .production); + } else { + try this.env.load(dir, this.options.env.files, .development); + } + } + // This must be run after a framework is configured, if a framework is enabled pub fn configureDefines(this: *Bundler) !void { if (this.options.defines_loaded) { diff --git a/src/cli/run_command.zig b/src/cli/run_command.zig index 0d33219976f3a..54b61bce0e506 100644 --- a/src/cli/run_command.zig +++ b/src/cli/run_command.zig @@ -756,7 +756,7 @@ pub const RunCommand = struct { this_bundler.configureLinker(); - var root_dir_info = this_bundler.resolver.readDirInfo(this_bundler.fs.top_level_dir) catch |err| { + const root_dir_info = this_bundler.resolver.readDirInfo(this_bundler.fs.top_level_dir) catch |err| { if (!log_errors) return error.CouldntReadCurrentDirectory; if (Output.enable_ansi_colors) { ctx.log.printForLogLevelWithEnableAnsiColors(Output.errorWriter(), true) catch {}; @@ -781,27 +781,7 @@ pub const RunCommand = struct { if (env == null) { this_bundler.env.loadProcess(); - - if (this_bundler.env.get("NODE_ENV")) |node_env| { - if (strings.eqlComptime(node_env, "production")) { - this_bundler.options.production = true; - } - } - - // TODO: evaluate if we can skip running this in nested calls to bun run - // The reason why it's unclear: - // - Some scripts may do NODE_ENV=production bun run foo - // This would cause potentially a different .env file to be loaded - this_bundler.runEnvLoader() catch {}; - - if (root_dir_info.getEntries(0)) |dir| { - // Run .env again if it exists in a parent dir - if (this_bundler.options.production) { - this_bundler.env.load(dir, this_bundler.options.env.files, .production) catch {}; - } else { - this_bundler.env.load(dir, this_bundler.options.env.files, .development) catch {}; - } - } + if (this_bundler.env.isProduction()) this_bundler.options.production = true; } this_bundler.env.map.putDefault("npm_config_local_prefix", this_bundler.fs.top_level_dir) catch unreachable; @@ -960,12 +940,7 @@ pub const RunCommand = struct { { this_bundler.env.loadProcess(); - - if (this_bundler.env.get("NODE_ENV")) |node_env| { - if (strings.eqlComptime(node_env, "production")) { - this_bundler.options.production = true; - } - } + if (this_bundler.env.isProduction()) this_bundler.options.production = true; } const ResultList = bun.StringArrayHashMap(void); diff --git a/test/cli/install/dummy.registry.ts b/test/cli/install/dummy.registry.ts index 67d021c068c4a..61f61684eada1 100644 --- a/test/cli/install/dummy.registry.ts +++ b/test/cli/install/dummy.registry.ts @@ -102,7 +102,7 @@ export function dummyAfterAll() { server.stop(); } -let packageDirGetter: () => string = () => { +export let packageDirGetter: () => string = () => { return tmpdirSync("bun-install-test-" + testCounter++ + "--"); }; export async function dummyBeforeEach() { diff --git a/test/js/bun/shell/bunshell.test.ts b/test/js/bun/shell/bunshell.test.ts index a6d7067eddf26..17d10fd82142c 100644 --- a/test/js/bun/shell/bunshell.test.ts +++ b/test/js/bun/shell/bunshell.test.ts @@ -7,11 +7,12 @@ */ import { $ } from "bun"; import { afterAll, beforeAll, describe, expect, test } from "bun:test"; -import { mkdir, mkdtemp, realpath, rm } from "fs/promises"; -import { bunEnv, runWithErrorPromise, tempDirWithFiles } from "harness"; +import { mkdir, mkdtemp, realpath, rm, writeFile } from "fs/promises"; +import { bunEnv, bunExe, runWithErrorPromise, tempDirWithFiles } from "harness"; import { tmpdir } from "os"; import { join, sep } from "path"; import { TestBuilder, sortedShellOutput } from "./util"; +import { packageDirGetter } from "./../../../cli/install/dummy.registry"; $.env(bunEnv); $.cwd(process.cwd()); @@ -815,3 +816,61 @@ function sentinelByte(buf: Uint8Array): number { } throw new Error("No sentinel byte"); } + +test("shell as script runner propagates env vars: bun run ", async () => { + const package_dir = packageDirGetter(); + await writeFile( + join(package_dir, "package.json"), + JSON.stringify({ scripts: { prod: `NODE_ENV=production ${bunExe()} index.ts` } }), + ); + await writeFile( + join(package_dir, "index.ts"), + `console.log(process.env.NODE_ENV); console.log(process.env.AWESOME);`, + ); + await writeFile(join(package_dir, ".env.production"), `AWESOME=production`); + await writeFile(join(package_dir, ".env.development"), `AWESOME=development`); + + const { stdout, stderr, exited } = Bun.spawn({ + cmd: [bunExe(), "run", "prod"], + cwd: package_dir, + stdio: ["pipe", "pipe", "pipe"], + env: bunEnv, + }); + expect(stderr).toBeDefined(); + const err = await new Response(stderr).text(); + expect(err).not.toContain("error:"); + expect(err).not.toContain("panic:"); + expect(stdout).toBeDefined(); + const out = await new Response(stdout).text(); + expect(out.trim().split("\n")).toEqual(["production", "production"]); + expect(await exited).toBe(0); +}); + +test("shell as script runner propagates env vars: bun ", async () => { + const package_dir = packageDirGetter(); + await writeFile( + join(package_dir, "package.json"), + JSON.stringify({ scripts: { prod: `NODE_ENV=production ${bunExe()} index.ts` } }), + ); + await writeFile( + join(package_dir, "index.ts"), + `console.log(process.env.NODE_ENV); console.log(process.env.AWESOME);`, + ); + await writeFile(join(package_dir, ".env.production"), `AWESOME=production`); + await writeFile(join(package_dir, ".env.development"), `AWESOME=development`); + + const { stdout, stderr, exited } = Bun.spawn({ + cmd: [bunExe(), "prod"], + cwd: package_dir, + stdio: ["pipe", "pipe", "pipe"], + env: bunEnv, + }); + expect(stderr).toBeDefined(); + const err = await new Response(stderr).text(); + expect(err).not.toContain("error:"); + expect(err).not.toContain("panic:"); + expect(stdout).toBeDefined(); + const out = await new Response(stdout).text(); + expect(out.trim().split("\n")).toEqual(["production", "production"]); + expect(await exited).toBe(0); +}); From a0ad160781c41666066af4c6950811eeee70f024 Mon Sep 17 00:00:00 2001 From: Zack Radisic Date: Tue, 19 Mar 2024 12:51:22 -0700 Subject: [PATCH 2/2] Escape bun exe --- test/js/bun/shell/bunshell.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/js/bun/shell/bunshell.test.ts b/test/js/bun/shell/bunshell.test.ts index 4d3c75ebdde54..ddbcd6c472f3c 100644 --- a/test/js/bun/shell/bunshell.test.ts +++ b/test/js/bun/shell/bunshell.test.ts @@ -7,7 +7,7 @@ */ import { $ } from "bun"; import { afterAll, beforeAll, describe, expect, test } from "bun:test"; -import { mkdir, mkdtemp, realpath, rm, writeFile } from "fs/promises"; +import { mkdir, mkdtemp, realpath, rm, writeFile, readFile } from "fs/promises"; import { bunEnv, bunExe, runWithErrorPromise, tempDirWithFiles } from "harness"; import { tmpdir } from "os"; import { join, sep } from "path"; @@ -837,7 +837,7 @@ test("shell as script runner propagates env vars: bun run ", async () => { const package_dir = packageDirGetter(); await writeFile( join(package_dir, "package.json"), - JSON.stringify({ scripts: { prod: `NODE_ENV=production ${bunExe()} index.ts` } }), + JSON.stringify({ scripts: { prod: `NODE_ENV=production ${$.escape(bunExe())} index.ts` } }), ); await writeFile( join(package_dir, "index.ts"), @@ -866,7 +866,7 @@ test("shell as script runner propagates env vars: bun ", async () => { const package_dir = packageDirGetter(); await writeFile( join(package_dir, "package.json"), - JSON.stringify({ scripts: { prod: `NODE_ENV=production ${bunExe()} index.ts` } }), + JSON.stringify({ scripts: { prod: `NODE_ENV=production ${$.escape(bunExe())} index.ts` } }), ); await writeFile( join(package_dir, "index.ts"),