From 30969221fb64cc28986e9aa1220653049f43b13e Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 26 Mar 2024 14:35:49 -0700 Subject: [PATCH 01/10] do not pass conditional env vars to createNullDelimitedEnvMap --- src/env_loader.zig | 62 +++++++++++++++++++++++++++++++++------- test/cli/run/env.test.ts | 25 ++++++++++++++++ 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/src/env_loader.zig b/src/env_loader.zig index 1e776a38f9d20..99e877b7d3f03 100644 --- a/src/env_loader.zig +++ b/src/env_loader.zig @@ -1079,30 +1079,70 @@ pub const Map = struct { map: HashTable, - pub fn createNullDelimitedEnvMap(this: *Map, arena: std.mem.Allocator) ![:null]?[*:0]u8 { + /// Creates a environment block for use in Posix Spawn APIs. + /// As in, a sentinel slice of sentinel string pointers. + pub fn createNullDelimitedEnvMap(this: *Map, alloc: std.mem.Allocator) ![:null]?[*:0]u8 { var env_map = &this.map; - const envp_count = env_map.count(); - const envp_buf = try arena.allocSentinel(?[*:0]u8, envp_count, null); + var envp_count: usize = 0; + var total_bytes: usize = 0; + { + var it = env_map.iterator(); + while (it.next()) |pair| { + // Allow var from .env.development or .env.production to be loaded again + if (!pair.value_ptr.conditional) { + envp_count += 1; + // env line is 'KEY=VALUE\x00' + total_bytes += (pair.key_ptr.len + pair.value_ptr.value.len + "=\x00".len); + } + } + } + total_bytes += (envp_count + 1) * @sizeOf(?[*:0]u8); // +1 for the null ptr after the pointer list + + const buf = try alloc.alignedAlloc(u8, @alignOf(?[*:0]u8), total_bytes); + + const envp = envp: { + const p: [*]?[*:0]u8 = @ptrCast(buf.ptr); + p[envp_count] = null; + break :envp p[0..envp_count :null]; + }; + + var fba = std.heap.FixedBufferAllocator.init(buf[@sizeOf(usize) * (envp_count + 1) ..]); + const string_alloc = fba.allocator(); + { var it = env_map.iterator(); var i: usize = 0; - while (it.next()) |pair| : (i += 1) { - const env_buf = try arena.allocSentinel(u8, pair.key_ptr.len + pair.value_ptr.value.len + 1, 0); - bun.copy(u8, env_buf, pair.key_ptr.*); - env_buf[pair.key_ptr.len] = '='; - bun.copy(u8, env_buf[pair.key_ptr.len + 1 ..], pair.value_ptr.value); - envp_buf[i] = env_buf.ptr; + while (it.next()) |pair| { + // Allow var from .env.development or .env.production to be loaded again + if (!pair.value_ptr.conditional) { + const variable_buf = string_alloc.allocSentinel( + u8, + pair.key_ptr.len + pair.value_ptr.value.len + 1, + 0, + ) catch unreachable; // all bytes were pre-allocated. + @memcpy(variable_buf[0..pair.key_ptr.len], pair.key_ptr.*); + variable_buf[pair.key_ptr.len] = '='; + @memcpy(variable_buf[pair.key_ptr.len + 1 ..], pair.value_ptr.value); + envp[i] = variable_buf.ptr; + i += 1; + } } if (comptime Environment.allow_assert) std.debug.assert(i == envp_count); } - return envp_buf; + + if (comptime Environment.allow_assert) { + std.debug.assert(fba.end_index == fba.buffer.len); // incorrect counting above. every pointer should be byte-aligned + } + + return envp; } /// Returns a wrapper around the std.process.EnvMap that does not duplicate the memory of /// the keys and values, but instead points into the memory of the bun env map. /// - /// To prevent + /// To prevent mutation, the return value is a wrapper struct that can only + /// return a *const std.process.EnvMap. pub fn stdEnvMap(this: *Map, allocator: std.mem.Allocator) !StdEnvMapWrapper { var env_map = std.process.EnvMap.init(allocator); diff --git a/test/cli/run/env.test.ts b/test/cli/run/env.test.ts index c1fa2fd1a3d39..d535ca18f00ec 100644 --- a/test/cli/run/env.test.ts +++ b/test/cli/run/env.test.ts @@ -710,6 +710,31 @@ test("NODE_ENV default is not propogated in bun run", () => { expect(bunRunAsScript(tmp, "show-env", {}).stdout).toBe(""); }); +test("the script runner does not pass variables from .env files into scripts", () => { + const tmp = tempDirWithFiles("script-runner-env", { + "package.json": '{"scripts":{"show-env":"echo $ENV_FILE_NAME"}}', + + ".env": "ENV_FILE_NAME=.env", + ".env.development": "ENV_FILE_NAME=.env.development", + ".env.production": "ENV_FILE_NAME=.env.production", + ".env.test": "ENV_FILE_NAME=.env.test", + }); + expect(bunRunAsScript(tmp, "show-env", {}).stdout).toBe(""); +}); + +test("the script runner does pass .env.local in", () => { + const tmp = tempDirWithFiles("script-runner-env", { + "package.json": '{"scripts":{"show-env":"echo $ENV_FILE_NAME"}}', + + ".env": "ENV_FILE_NAME=.env", + ".env.development": "ENV_FILE_NAME=.env.development", + ".env.production": "ENV_FILE_NAME=.env.production", + ".env.test": "ENV_FILE_NAME=.env.test", + ".env.local": "ENV_FILE_NAME=.env.local", + }); + expect(bunRunAsScript(tmp, "show-env", {}).stdout).toBe(".env.local"); +}); + const todoOnPosix = process.platform !== "win32" ? test.todo : test; todoOnPosix("setting process.env coerces the value to a string", () => { // @ts-expect-error From c2beb127c5d118de4f9dfe22563e423277f90207 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 26 Mar 2024 14:49:05 -0700 Subject: [PATCH 02/10] a --- src/env_loader.zig | 5 +++++ test/cli/run/env.test.ts | 37 +++++++++++++++++++++++++++++++++---- test/harness.ts | 4 ++-- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/src/env_loader.zig b/src/env_loader.zig index 99e877b7d3f03..5d8e53107dab0 100644 --- a/src/env_loader.zig +++ b/src/env_loader.zig @@ -1081,6 +1081,11 @@ pub const Map = struct { /// Creates a environment block for use in Posix Spawn APIs. /// As in, a sentinel slice of sentinel string pointers. + /// + /// TODO: the returned value of this is very hard to cleanup manually, and currently must be + /// backed by an arena allocator to avoid a memory leak. We use this in many places without + /// an area, so those usages are leaking here. Solution would be to make this return a struct + /// with a deinit that contained the allocation length. pub fn createNullDelimitedEnvMap(this: *Map, alloc: std.mem.Allocator) ![:null]?[*:0]u8 { var env_map = &this.map; diff --git a/test/cli/run/env.test.ts b/test/cli/run/env.test.ts index d535ca18f00ec..f9c36b3dd9596 100644 --- a/test/cli/run/env.test.ts +++ b/test/cli/run/env.test.ts @@ -710,7 +710,36 @@ test("NODE_ENV default is not propogated in bun run", () => { expect(bunRunAsScript(tmp, "show-env", {}).stdout).toBe(""); }); -test("the script runner does not pass variables from .env files into scripts", () => { +test("the script runner (system) does not pass variables from .env files into scripts", () => { + const script = isWindows ? "echo ENV_FILE_NAME=%ENV_FILE_NAME%" : "echo ENV_FILE_NAME=$ENV_FILE_NAME"; + const tmp = tempDirWithFiles("script-runner-env", { + "package.json": '{"scripts":{"show-env":"' + script + '"}}', + + ".env": "ENV_FILE_NAME=.env", + ".env.development": "ENV_FILE_NAME=.env.development", + ".env.production": "ENV_FILE_NAME=.env.production", + ".env.test": "ENV_FILE_NAME=.env.test", + }); + // Cannot be empty as windows system shell will print "ECHO is on/off." if echo has no arguments + expect(bunRunAsScript(tmp, "show-env", {}, ["--shell=system"]).stdout).toBe("ENV_FILE_NAME="); +}); + +test("the script runner (system) does pass .env.local in", () => { + const script = isWindows ? "echo ENV_FILE_NAME=%ENV_FILE_NAME%" : "echo ENV_FILE_NAME=$ENV_FILE_NAME"; + const tmp = tempDirWithFiles("script-runner-env", { + "package.json": '{"scripts":{"show-env":"' + script + '"}}', + + ".env": "ENV_FILE_NAME=.env", + ".env.development": "ENV_FILE_NAME=.env.development", + ".env.production": "ENV_FILE_NAME=.env.production", + ".env.test": "ENV_FILE_NAME=.env.test", + ".env.local": "ENV_FILE_NAME=.env.local", + }); + expect(bunRunAsScript(tmp, "show-env", {}, ["--shell=system"]).stdout).toBe("ENV_FILE_NAME=.env.local"); +}); + +// TODO(@paperdave): fix this +test.todo("the script runner (bun shell) does not pass variables from .env files into scripts", () => { const tmp = tempDirWithFiles("script-runner-env", { "package.json": '{"scripts":{"show-env":"echo $ENV_FILE_NAME"}}', @@ -719,10 +748,10 @@ test("the script runner does not pass variables from .env files into scripts", ( ".env.production": "ENV_FILE_NAME=.env.production", ".env.test": "ENV_FILE_NAME=.env.test", }); - expect(bunRunAsScript(tmp, "show-env", {}).stdout).toBe(""); + expect(bunRunAsScript(tmp, "show-env", {}, ["--shell=bun"]).stdout).toBe(""); }); -test("the script runner does pass .env.local in", () => { +test("the script runner (bun shell) does pass .env.local in", () => { const tmp = tempDirWithFiles("script-runner-env", { "package.json": '{"scripts":{"show-env":"echo $ENV_FILE_NAME"}}', @@ -732,7 +761,7 @@ test("the script runner does pass .env.local in", () => { ".env.test": "ENV_FILE_NAME=.env.test", ".env.local": "ENV_FILE_NAME=.env.local", }); - expect(bunRunAsScript(tmp, "show-env", {}).stdout).toBe(".env.local"); + expect(bunRunAsScript(tmp, "show-env", {}, ["--shell=bun"]).stdout).toBe(".env.local"); }); const todoOnPosix = process.platform !== "win32" ? test.todo : test; diff --git a/test/harness.ts b/test/harness.ts index ff3e49f677e32..7aa4ecf0a6913 100644 --- a/test/harness.ts +++ b/test/harness.ts @@ -168,8 +168,8 @@ export function bunTest(file: string, env?: Record) { }; } -export function bunRunAsScript(dir: string, script: string, env?: Record) { - const result = Bun.spawnSync([bunExe(), `run`, `${script}`], { +export function bunRunAsScript(dir: string, script: string, env?: Record, execArgv?: string[]) { + const result = Bun.spawnSync([bunExe(), ...(execArgv ?? []), `run`, `${script}`], { cwd: dir, env: { ...bunEnv, From ff9e7571afcb68721e7c5100594450700743b146 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 26 Mar 2024 16:14:15 -0700 Subject: [PATCH 03/10] =?UTF-8?q?fix:=20=F0=9F=92=A5=20to=20.conditional?= =?UTF-8?q?=20env=20vars.=20see=20pr=20description?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/bun_js.zig | 5 +- src/bundler.zig | 10 +-- src/cli/run_command.zig | 32 ++++---- src/cli/test_command.zig | 1 - src/env_loader.zig | 153 ++++++++++++++++----------------------- src/install/install.zig | 3 +- src/install/lockfile.zig | 2 +- test/cli/run/env.test.ts | 106 +++++++++++++++------------ test/harness.ts | 7 +- 9 files changed, 152 insertions(+), 167 deletions(-) diff --git a/src/bun_js.zig b/src/bun_js.zig index 02419c4cd4f27..9bd0050f7d4f3 100644 --- a/src/bun_js.zig +++ b/src/bun_js.zig @@ -133,7 +133,7 @@ pub const Run = struct { try @import("./bun.js/config.zig").configureTransformOptionsForBunVM(ctx.allocator, ctx.args), null, ); - try bundle.runEnvLoader(); + try bundle.runEnvLoader(false); const mini = JSC.MiniEventLoop.initGlobal(bundle.env); mini.top_level_dir = ctx.args.absolute_working_dir orelse ""; return try bun.shell.Interpreter.initAndRunFromFile(mini, entry_path); @@ -147,7 +147,7 @@ pub const Run = struct { try bun.CLI.Arguments.loadConfigPath(ctx.allocator, true, "bunfig.toml", &ctx, .RunCommand); } - if (strings.endsWithComptime(entry_path, comptime if (Environment.isWindows) ".sh" else ".bun.sh")) { + if (strings.endsWithComptime(entry_path, ".sh")) { const exit_code = try bootBunShell(&ctx, entry_path); Global.exitWide(exit_code); return; @@ -228,7 +228,6 @@ pub const Run = struct { node_env_entry.key_ptr.* = try b.env.allocator.dupe(u8, node_env_entry.key_ptr.*); node_env_entry.value_ptr.* = .{ .value = try b.env.allocator.dupe(u8, "development"), - .conditional = false, }; } diff --git a/src/bundler.zig b/src/bundler.zig index cdbde565764f2..4300235821fb3 100644 --- a/src/bundler.zig +++ b/src/bundler.zig @@ -520,7 +520,7 @@ pub const Bundler = struct { bundler.configureLinkerWithAutoJSX(true); } - pub fn runEnvLoader(this: *Bundler) !void { + pub fn runEnvLoader(this: *Bundler, is_script_runner: bool) !void { switch (this.options.env.behavior) { .prefix, .load_all, .load_all_without_inlining => { // Step 1. Load the project root. @@ -541,11 +541,11 @@ pub const Bundler = struct { } if (this.options.isTest() or this.env.isTest()) { - try this.env.load(dir, this.options.env.files, .@"test"); + try this.env.load(dir, this.options.env.files, .@"test", is_script_runner); } else if (this.options.production) { - try this.env.load(dir, this.options.env.files, .production); + try this.env.load(dir, this.options.env.files, .production, is_script_runner); } else { - try this.env.load(dir, this.options.env.files, .development); + try this.env.load(dir, this.options.env.files, .development, is_script_runner); } }, .disable => { @@ -584,7 +584,7 @@ pub const Bundler = struct { this.options.env.prefix = "BUN_"; } - try this.runEnvLoader(); + try this.runEnvLoader(false); this.options.jsx.setProduction(this.env.isProduction()); diff --git a/src/cli/run_command.zig b/src/cli/run_command.zig index ee21120203255..99e534afbf13c 100644 --- a/src/cli/run_command.zig +++ b/src/cli/run_command.zig @@ -850,7 +850,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 {}; @@ -876,26 +876,20 @@ 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; - } - } + if (this_bundler.env.isProduction()) + 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 {}; + this_bundler.runEnvLoader(true) 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 {}; - } - } + // sus + // 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, true) catch {}; + // } else { + // this_bundler.env.load(dir, this_bundler.options.env.files, .development, true) catch {}; + // } + // } } this_bundler.env.map.putDefault("npm_config_local_prefix", this_bundler.fs.top_level_dir) catch unreachable; diff --git a/src/cli/test_command.zig b/src/cli/test_command.zig index 9021d66482793..7e4b7362b3bad 100644 --- a/src/cli/test_command.zig +++ b/src/cli/test_command.zig @@ -665,7 +665,6 @@ pub const TestCommand = struct { node_env_entry.key_ptr.* = try env_loader.allocator.dupe(u8, node_env_entry.key_ptr.*); node_env_entry.value_ptr.* = .{ .value = try env_loader.allocator.dupe(u8, "test"), - .conditional = false, }; } diff --git a/src/env_loader.zig b/src/env_loader.zig index 5d8e53107dab0..7519e585fa517 100644 --- a/src/env_loader.zig +++ b/src/env_loader.zig @@ -189,7 +189,6 @@ pub const Loader = struct { } fn loadCCachePathImpl(this: *Loader, fs: *Fs.FileSystem) !void { - // if they have ccache installed, put it in env variable `CMAKE_CXX_COMPILER_LAUNCHER` so // cmake can use it to hopefully speed things up var buf: [bun.MAX_PATH_BYTES]u8 = undefined; @@ -206,7 +205,6 @@ pub const Loader = struct { cxx_gop.key_ptr.* = try this.allocator.dupe(u8, cxx_gop.key_ptr.*); cxx_gop.value_ptr.* = .{ .value = try this.allocator.dupe(u8, ccache_path), - .conditional = false, }; } const c_gop = try this.map.getOrPutWithoutValue("CMAKE_C_COMPILER_LAUNCHER"); @@ -214,7 +212,6 @@ pub const Loader = struct { c_gop.key_ptr.* = try this.allocator.dupe(u8, c_gop.key_ptr.*); c_gop.value_ptr.* = .{ .value = try this.allocator.dupe(u8, ccache_path), - .conditional = false, }; } } @@ -468,13 +465,14 @@ pub const Loader = struct { dir: *Fs.FileSystem.DirEntry, env_files: []const []const u8, comptime suffix: DotEnvFileSuffix, + is_script_runner: bool, ) !void { const start = std.time.nanoTimestamp(); if (env_files.len > 0) { try this.loadExplicitFiles(env_files); } else { - try this.loadDefaultFiles(dir, suffix); + try this.loadDefaultFiles(dir, suffix, is_script_runner); } if (!this.quiet) this.printLoaded(start); @@ -492,8 +490,7 @@ pub const Loader = struct { var iter = std.mem.splitBackwardsScalar(u8, arg_value, ','); while (iter.next()) |file_path| { if (file_path.len > 0) { - try this.loadEnvFileDynamic(file_path, false, true); - Analytics.Features.dotenv = true; + try this.loadEnvFileDynamic(file_path, false); } } } @@ -508,61 +505,61 @@ pub const Loader = struct { this: *Loader, dir: *Fs.FileSystem.DirEntry, comptime suffix: DotEnvFileSuffix, + /// Only '.env' and '.env.local' are loaded when running a script + /// mostly for backwards compatibility with older behavior of bun. + /// See https://github.com/oven-sh/bun/pull/9642 + is_script_runner: bool, ) !void { const dir_handle: std.fs.Dir = std.fs.cwd(); - switch (comptime suffix) { - .development => { - if (dir.hasComptimeQuery(".env.development.local")) { - try this.loadEnvFile(dir_handle, ".env.development.local", false, true); - Analytics.Features.dotenv = true; - } - }, - .production => { - if (dir.hasComptimeQuery(".env.production.local")) { - try this.loadEnvFile(dir_handle, ".env.production.local", false, true); - Analytics.Features.dotenv = true; - } - }, - .@"test" => { - if (dir.hasComptimeQuery(".env.test.local")) { - try this.loadEnvFile(dir_handle, ".env.test.local", false, true); - Analytics.Features.dotenv = true; - } - }, + if (!is_script_runner) { + switch (comptime suffix) { + .development => { + if (dir.hasComptimeQuery(".env.development.local")) { + try this.loadEnvFile(dir_handle, ".env.development.local", false); + } + }, + .production => { + if (dir.hasComptimeQuery(".env.production.local")) { + try this.loadEnvFile(dir_handle, ".env.production.local", false); + } + }, + .@"test" => { + if (dir.hasComptimeQuery(".env.test.local")) { + try this.loadEnvFile(dir_handle, ".env.test.local", false); + } + }, + } } if (comptime suffix != .@"test") { if (dir.hasComptimeQuery(".env.local")) { - try this.loadEnvFile(dir_handle, ".env.local", false, false); - Analytics.Features.dotenv = true; + try this.loadEnvFile(dir_handle, ".env.local", false); } } - switch (comptime suffix) { - .development => { - if (dir.hasComptimeQuery(".env.development")) { - try this.loadEnvFile(dir_handle, ".env.development", false, true); - Analytics.Features.dotenv = true; - } - }, - .production => { - if (dir.hasComptimeQuery(".env.production")) { - try this.loadEnvFile(dir_handle, ".env.production", false, true); - Analytics.Features.dotenv = true; - } - }, - .@"test" => { - if (dir.hasComptimeQuery(".env.test")) { - try this.loadEnvFile(dir_handle, ".env.test", false, true); - Analytics.Features.dotenv = true; - } - }, - } + if (!is_script_runner) { + switch (comptime suffix) { + .development => { + if (dir.hasComptimeQuery(".env.development")) { + try this.loadEnvFile(dir_handle, ".env.development", false); + } + }, + .production => { + if (dir.hasComptimeQuery(".env.production")) { + try this.loadEnvFile(dir_handle, ".env.production", false); + } + }, + .@"test" => { + if (dir.hasComptimeQuery(".env.test")) { + try this.loadEnvFile(dir_handle, ".env.test", false); + } + }, + } - if (dir.hasComptimeQuery(".env")) { - try this.loadEnvFile(dir_handle, ".env", false, false); - Analytics.Features.dotenv = true; + if (dir.hasComptimeQuery(".env")) { + try this.loadEnvFile(dir_handle, ".env", false); + } } } @@ -636,7 +633,6 @@ pub const Loader = struct { dir: std.fs.Dir, comptime base: string, comptime override: bool, - comptime conditional: bool, ) !void { if (@field(this, base) != null) { return; @@ -714,7 +710,6 @@ pub const Loader = struct { this.map, override, false, - conditional, ); @field(this, base) = source; @@ -724,7 +719,6 @@ pub const Loader = struct { this: *Loader, file_path: []const u8, comptime override: bool, - comptime conditional: bool, ) !void { if (this.custom_files_loaded.contains(file_path)) { return; @@ -786,7 +780,6 @@ pub const Loader = struct { this.map, override, false, - conditional, ); try this.custom_files_loaded.put(file_path, source); @@ -1011,8 +1004,9 @@ const Parser = struct { map: *Map, comptime override: bool, comptime is_process: bool, - comptime conditional: bool, ) void { + Analytics.Features.dotenv = true; + var count = map.map.count(); while (this.pos < this.src.len) { const key = this.parseKey(true) orelse { @@ -1032,7 +1026,6 @@ const Parser = struct { } entry.value_ptr.* = .{ .value = allocator.dupe(u8, value) catch unreachable, - .conditional = conditional, }; } if (comptime !is_process) { @@ -1044,7 +1037,6 @@ const Parser = struct { allocator.free(entry.value_ptr.value); entry.value_ptr.* = .{ .value = allocator.dupe(u8, value) catch unreachable, - .conditional = conditional, }; } } @@ -1057,17 +1049,15 @@ const Parser = struct { map: *Map, comptime override: bool, comptime is_process: bool, - comptime conditional: bool, ) void { var parser = Parser{ .src = source.contents }; - parser._parse(allocator, map, override, is_process, conditional); + parser._parse(allocator, map, override, is_process); } }; pub const Map = struct { const HashTableValue = struct { value: string, - conditional: bool, }; // On Windows, environment variables are case-insensitive. So we use a case-insensitive hash map. // An issue with this exact implementation is unicode characters can technically appear in these @@ -1094,12 +1084,9 @@ pub const Map = struct { { var it = env_map.iterator(); while (it.next()) |pair| { - // Allow var from .env.development or .env.production to be loaded again - if (!pair.value_ptr.conditional) { - envp_count += 1; - // env line is 'KEY=VALUE\x00' - total_bytes += (pair.key_ptr.len + pair.value_ptr.value.len + "=\x00".len); - } + envp_count += 1; + // env line is 'KEY=VALUE\x00' + total_bytes += (pair.key_ptr.len + pair.value_ptr.value.len + "=\x00".len); } } total_bytes += (envp_count + 1) * @sizeOf(?[*:0]u8); // +1 for the null ptr after the pointer list @@ -1119,19 +1106,16 @@ pub const Map = struct { var it = env_map.iterator(); var i: usize = 0; while (it.next()) |pair| { - // Allow var from .env.development or .env.production to be loaded again - if (!pair.value_ptr.conditional) { - const variable_buf = string_alloc.allocSentinel( - u8, - pair.key_ptr.len + pair.value_ptr.value.len + 1, - 0, - ) catch unreachable; // all bytes were pre-allocated. - @memcpy(variable_buf[0..pair.key_ptr.len], pair.key_ptr.*); - variable_buf[pair.key_ptr.len] = '='; - @memcpy(variable_buf[pair.key_ptr.len + 1 ..], pair.value_ptr.value); - envp[i] = variable_buf.ptr; - i += 1; - } + const variable_buf = string_alloc.allocSentinel( + u8, + pair.key_ptr.len + pair.value_ptr.value.len + 1, + 0, + ) catch unreachable; // all bytes were pre-allocated. + @memcpy(variable_buf[0..pair.key_ptr.len], pair.key_ptr.*); + variable_buf[pair.key_ptr.len] = '='; + @memcpy(variable_buf[pair.key_ptr.len + 1 ..], pair.value_ptr.value); + envp[i] = variable_buf.ptr; + i += 1; } if (comptime Environment.allow_assert) std.debug.assert(i == envp_count); } @@ -1153,10 +1137,7 @@ pub const Map = struct { var iter = this.map.iterator(); while (iter.next()) |entry| { - // Allow var from .env.development or .env.production to be loaded again - if (!entry.value_ptr.conditional) { - try env_map.hash_map.put(entry.key_ptr.*, entry.value_ptr.value); - } + try env_map.hash_map.put(entry.key_ptr.*, entry.value_ptr.value); } return .{ .unsafe_map = env_map }; @@ -1215,7 +1196,6 @@ pub const Map = struct { } try this.map.put(key, .{ .value = value, - .conditional = false, }); } @@ -1223,7 +1203,6 @@ pub const Map = struct { const gop = try this.map.getOrPut(key); gop.value_ptr.* = .{ .value = try allocator.dupe(u8, value), - .conditional = false, }; if (!gop.found_existing) { gop.key_ptr.* = try allocator.dupe(u8, key); @@ -1234,7 +1213,6 @@ pub const Map = struct { const gop = try this.map.getOrPut(key); gop.value_ptr.* = .{ .value = value, - .conditional = false, }; if (!gop.found_existing) { gop.key_ptr.* = try allocator.dupe(u8, key); @@ -1244,7 +1222,6 @@ pub const Map = struct { pub inline fn putAllocValue(this: *Map, allocator: std.mem.Allocator, key: string, value: string) !void { try this.map.put(key, .{ .value = try allocator.dupe(u8, value), - .conditional = false, }); } @@ -1283,14 +1260,12 @@ pub const Map = struct { pub inline fn putDefault(this: *Map, key: string, value: string) !void { _ = try this.map.getOrPutValue(key, .{ .value = value, - .conditional = false, }); } pub inline fn getOrPut(this: *Map, key: string, value: string) !void { _ = try this.map.getOrPutValue(key, .{ .value = value, - .conditional = false, }); } diff --git a/src/install/install.zig b/src/install/install.zig index 5e5b6092cd1a1..d27304dc3bc94 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -2114,7 +2114,6 @@ pub const PackageManager = struct { init_cwd_gop.key_ptr.* = try ctx.allocator.dupe(u8, init_cwd_gop.key_ptr.*); init_cwd_gop.value_ptr.* = .{ .value = try ctx.allocator.dupe(u8, FileSystem.instance.top_level_dir), - .conditional = false, }; } @@ -6672,7 +6671,7 @@ pub const PackageManager = struct { }; env.loadProcess(); - try env.load(entries_option.entries, &[_][]u8{}, .production); + try env.load(entries_option.entries, &[_][]u8{}, .production, false); var cpu_count = @as(u32, @truncate(((try std.Thread.getCpuCount()) + 1))); diff --git a/src/install/lockfile.zig b/src/install/lockfile.zig index 6dfffc99a0d42..30a4f85e3e94c 100644 --- a/src/install/lockfile.zig +++ b/src/install/lockfile.zig @@ -1112,7 +1112,7 @@ pub const Printer = struct { }; env_loader.loadProcess(); - try env_loader.load(entries_option.entries, &[_][]u8{}, .production); + try env_loader.load(entries_option.entries, &[_][]u8{}, .production, false); var log = logger.Log.init(allocator); try options.load( allocator, diff --git a/test/cli/run/env.test.ts b/test/cli/run/env.test.ts index f9c36b3dd9596..af2455821972c 100644 --- a/test/cli/run/env.test.ts +++ b/test/cli/run/env.test.ts @@ -710,59 +710,73 @@ test("NODE_ENV default is not propogated in bun run", () => { expect(bunRunAsScript(tmp, "show-env", {}).stdout).toBe(""); }); -test("the script runner (system) does not pass variables from .env files into scripts", () => { - const script = isWindows ? "echo ENV_FILE_NAME=%ENV_FILE_NAME%" : "echo ENV_FILE_NAME=$ENV_FILE_NAME"; - const tmp = tempDirWithFiles("script-runner-env", { - "package.json": '{"scripts":{"show-env":"' + script + '"}}', - - ".env": "ENV_FILE_NAME=.env", - ".env.development": "ENV_FILE_NAME=.env.development", - ".env.production": "ENV_FILE_NAME=.env.production", - ".env.test": "ENV_FILE_NAME=.env.test", - }); - // Cannot be empty as windows system shell will print "ECHO is on/off." if echo has no arguments - expect(bunRunAsScript(tmp, "show-env", {}, ["--shell=system"]).stdout).toBe("ENV_FILE_NAME="); -}); +for (const shell of ["system", "bun"]) { + const isWindowsCMD = isWindows && shell === "system"; -test("the script runner (system) does pass .env.local in", () => { - const script = isWindows ? "echo ENV_FILE_NAME=%ENV_FILE_NAME%" : "echo ENV_FILE_NAME=$ENV_FILE_NAME"; - const tmp = tempDirWithFiles("script-runner-env", { - "package.json": '{"scripts":{"show-env":"' + script + '"}}', + const show_env_script = isWindowsCMD // + ? "echo ENV_FILE_NAME=%ENV_FILE_NAME%, NODE_ENV=%NODE_ENV%" + : "echo ENV_FILE_NAME=$ENV_FILE_NAME, NODE_ENV=$NODE_ENV"; - ".env": "ENV_FILE_NAME=.env", - ".env.development": "ENV_FILE_NAME=.env.development", - ".env.production": "ENV_FILE_NAME=.env.production", - ".env.test": "ENV_FILE_NAME=.env.test", - ".env.local": "ENV_FILE_NAME=.env.local", - }); - expect(bunRunAsScript(tmp, "show-env", {}, ["--shell=system"]).stdout).toBe("ENV_FILE_NAME=.env.local"); -}); + describe(`script runner with ${shell} shell`, () => { + test("does not pass variables from .env files into scripts", () => { + const tmp = tempDirWithFiles("script-runner-env", { + "package.json": '{"scripts":{"show-env":"' + show_env_script + '"}}', -// TODO(@paperdave): fix this -test.todo("the script runner (bun shell) does not pass variables from .env files into scripts", () => { - const tmp = tempDirWithFiles("script-runner-env", { - "package.json": '{"scripts":{"show-env":"echo $ENV_FILE_NAME"}}', + ".env.development": "ENV_FILE_NAME=.env.development", + ".env.production": "ENV_FILE_NAME=.env.production", + ".env.test": "ENV_FILE_NAME=.env.test", + ".env": "ENV_FILE_NAME=.env", + }); - ".env": "ENV_FILE_NAME=.env", - ".env.development": "ENV_FILE_NAME=.env.development", - ".env.production": "ENV_FILE_NAME=.env.production", - ".env.test": "ENV_FILE_NAME=.env.test", - }); - expect(bunRunAsScript(tmp, "show-env", {}, ["--shell=bun"]).stdout).toBe(""); -}); + expect(bunRunAsScript(tmp, "show-env", {}, ["--shell=" + shell]).stdout).toBe("ENV_FILE_NAME=, NODE_ENV="); + }); -test("the script runner (bun shell) does pass .env.local in", () => { - const tmp = tempDirWithFiles("script-runner-env", { - "package.json": '{"scripts":{"show-env":"echo $ENV_FILE_NAME"}}', + test(".env.local is loaded by the script runner", () => { + const tmp = tempDirWithFiles("script-runner-env", { + "package.json": '{"scripts":{"show-env":"' + show_env_script + '"}}', + + ".env.local": "ENV_FILE_NAME=.env.local", + ".env.development": "ENV_FILE_NAME=.env.development", + ".env.production": "ENV_FILE_NAME=.env.production", + ".env.test": "ENV_FILE_NAME=.env.test", + ".env": "ENV_FILE_NAME=.env", + }); + expect(bunRunAsScript(tmp, "show-env", {}, ["--shell=" + shell]).stdout).toBe( + "ENV_FILE_NAME=.env.local, NODE_ENV=", + ); + }); - ".env": "ENV_FILE_NAME=.env", - ".env.development": "ENV_FILE_NAME=.env.development", - ".env.production": "ENV_FILE_NAME=.env.production", - ".env.test": "ENV_FILE_NAME=.env.test", - ".env.local": "ENV_FILE_NAME=.env.local", + for (const { NODE_ENV, expected } of [ + { + NODE_ENV: "production", + expected: "production", + }, + { + NODE_ENV: "development", + expected: "development", + }, + { + NODE_ENV: undefined, + expected: "", + }, + ]) { + test("explicit NODE_ENV=" + NODE_ENV, () => { + const tmp = tempDirWithFiles("script-runner-env", { + "package.json": '{"scripts":{"show-env":"' + show_env_script + '"}}', + + ".env.development": "ENV_FILE_NAME=.env.development", + ".env.production": "ENV_FILE_NAME=.env.production", + ".env.test": "ENV_FILE_NAME=.env.test", + ".env": "ENV_FILE_NAME=.env", + }); + + expect(bunRunAsScript(tmp, "show-env", { NODE_ENV }, ["--shell=" + shell]).stdout).toBe( + "ENV_FILE_NAME=, NODE_ENV=" + expected, + ); + }); + } }); - expect(bunRunAsScript(tmp, "show-env", {}, ["--shell=bun"]).stdout).toBe(".env.local"); -}); +} const todoOnPosix = process.platform !== "win32" ? test.todo : test; todoOnPosix("setting process.env coerces the value to a string", () => { diff --git a/test/harness.ts b/test/harness.ts index 7aa4ecf0a6913..f8382e4a71cf3 100644 --- a/test/harness.ts +++ b/test/harness.ts @@ -168,7 +168,12 @@ export function bunTest(file: string, env?: Record) { }; } -export function bunRunAsScript(dir: string, script: string, env?: Record, execArgv?: string[]) { +export function bunRunAsScript( + dir: string, + script: string, + env?: Record, + execArgv?: string[], +) { const result = Bun.spawnSync([bunExe(), ...(execArgv ?? []), `run`, `${script}`], { cwd: dir, env: { From 45286ef5e1bc622b852a04c97a98e4e0fb360862 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 26 Mar 2024 16:21:11 -0700 Subject: [PATCH 04/10] absord 9448's tests --- test/cli/run/env.test.ts | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/test/cli/run/env.test.ts b/test/cli/run/env.test.ts index af2455821972c..cd56f2c97d335 100644 --- a/test/cli/run/env.test.ts +++ b/test/cli/run/env.test.ts @@ -746,18 +746,21 @@ for (const shell of ["system", "bun"]) { ); }); - for (const { NODE_ENV, expected } of [ + for (const { NODE_ENV, expected, env_file } of [ { NODE_ENV: "production", expected: "production", + env_file: ".env.production", }, { NODE_ENV: "development", expected: "development", + env_file: ".env.development", }, { NODE_ENV: undefined, expected: "", + env_file: ".env.development", }, ]) { test("explicit NODE_ENV=" + NODE_ENV, () => { @@ -774,6 +777,28 @@ for (const shell of ["system", "bun"]) { "ENV_FILE_NAME=, NODE_ENV=" + expected, ); }); + + // This is already covered in isolation by the '.env file is loaded' describe + // but it is nice to have just a couple e2e tests combining script runner AND the runtime. + test("e2e NODE_ENV=" + NODE_ENV, () => { + const run_index_script = isWindowsCMD + ? `set NODE_ENV=${NODE_ENV} && bun run index.ts` + : `NODE_ENV=${NODE_ENV} bun run index.ts`; + + const tmp = tempDirWithFiles("script-runner-env", { + "package.json": '{"scripts":{"start":"' + run_index_script + '"}}', + "index.ts": "console.log(`ENV_FILE_NAME=${process.env.ENV_FILE_NAME}, NODE_ENV=${process.env.NODE_ENV}`);", + + ".env.development": "ENV_FILE_NAME=.env.development", + ".env.production": "ENV_FILE_NAME=.env.production", + ".env.test": "ENV_FILE_NAME=.env.test", + ".env": "ENV_FILE_NAME=.env", + }); + + expect(bunRunAsScript(tmp, "start", {}, ["--shell=" + shell]).stdout).toBe( + "ENV_FILE_NAME=" + env_file + ", NODE_ENV=" + NODE_ENV, + ); + }); } }); } From be9db9dd3d47f73a29301242984d04afc408d1d0 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 26 Mar 2024 16:41:25 -0700 Subject: [PATCH 05/10] HashMapValue -> string this structure had a single field in it --- src/bun.js/javascript.zig | 2 +- src/bun_js.zig | 4 +-- src/cli/test_command.zig | 4 +-- src/env_loader.zig | 68 +++++++++++++-------------------------- src/install/install.zig | 4 +-- src/shell/interpreter.zig | 2 +- 6 files changed, 27 insertions(+), 57 deletions(-) diff --git a/src/bun.js/javascript.zig b/src/bun.js/javascript.zig index c474b76c66e14..ea3473b6df52f 100644 --- a/src/bun.js/javascript.zig +++ b/src/bun.js/javascript.zig @@ -770,7 +770,7 @@ pub const VirtualMachine = struct { if (map.map.fetchSwapRemove("BUN_INTERNAL_IPC_FD")) |kv| { if (Environment.isWindows) { this.initIPCInstance(kv.value.value); - } else if (std.fmt.parseInt(i32, kv.value.value, 10) catch null) |fd| { + } else if (std.fmt.parseInt(i32, kv.value, 10) catch null) |fd| { this.initIPCInstance(bun.toFD(fd)); } else { Output.printErrorln("Failed to parse BUN_INTERNAL_IPC_FD", .{}); diff --git a/src/bun_js.zig b/src/bun_js.zig index 9bd0050f7d4f3..cb89573033154 100644 --- a/src/bun_js.zig +++ b/src/bun_js.zig @@ -226,9 +226,7 @@ pub const Run = struct { const node_env_entry = try b.env.map.getOrPutWithoutValue("NODE_ENV"); if (!node_env_entry.found_existing) { node_env_entry.key_ptr.* = try b.env.allocator.dupe(u8, node_env_entry.key_ptr.*); - node_env_entry.value_ptr.* = .{ - .value = try b.env.allocator.dupe(u8, "development"), - }; + node_env_entry.value_ptr.* = try b.env.allocator.dupe(u8, "development"); } b.configureRouter(false) catch { diff --git a/src/cli/test_command.zig b/src/cli/test_command.zig index 7e4b7362b3bad..e36c0bfc09283 100644 --- a/src/cli/test_command.zig +++ b/src/cli/test_command.zig @@ -663,9 +663,7 @@ pub const TestCommand = struct { const node_env_entry = try env_loader.map.getOrPutWithoutValue("NODE_ENV"); if (!node_env_entry.found_existing) { node_env_entry.key_ptr.* = try env_loader.allocator.dupe(u8, node_env_entry.key_ptr.*); - node_env_entry.value_ptr.* = .{ - .value = try env_loader.allocator.dupe(u8, "test"), - }; + node_env_entry.value_ptr.* = try env_loader.allocator.dupe(u8, "test"); } try vm.bundler.configureDefines(); diff --git a/src/env_loader.zig b/src/env_loader.zig index 7519e585fa517..2fd747c9b73f0 100644 --- a/src/env_loader.zig +++ b/src/env_loader.zig @@ -203,16 +203,12 @@ pub const Loader = struct { const cxx_gop = try this.map.getOrPutWithoutValue("CMAKE_CXX_COMPILER_LAUNCHER"); if (!cxx_gop.found_existing) { cxx_gop.key_ptr.* = try this.allocator.dupe(u8, cxx_gop.key_ptr.*); - cxx_gop.value_ptr.* = .{ - .value = try this.allocator.dupe(u8, ccache_path), - }; + cxx_gop.value_ptr.* = try this.allocator.dupe(u8, ccache_path); } const c_gop = try this.map.getOrPutWithoutValue("CMAKE_C_COMPILER_LAUNCHER"); if (!c_gop.found_existing) { c_gop.key_ptr.* = try this.allocator.dupe(u8, c_gop.key_ptr.*); - c_gop.value_ptr.* = .{ - .value = try this.allocator.dupe(u8, ccache_path), - }; + c_gop.value_ptr.* = try this.allocator.dupe(u8, ccache_path); } } } @@ -332,7 +328,7 @@ pub const Loader = struct { if (behavior == .prefix) { while (iter.next()) |entry| { - const value: string = entry.value_ptr.value; + const value: string = entry.value_ptr.*; if (strings.startsWith(entry.key_ptr.*, prefix)) { const key_str = std.fmt.allocPrint(key_allocator, "process.env.{s}", .{entry.key_ptr.*}) catch unreachable; @@ -383,12 +379,11 @@ pub const Loader = struct { } } else { while (iter.next()) |entry| { - const value: string = entry.value_ptr.value; const key = std.fmt.allocPrint(key_allocator, "process.env.{s}", .{entry.key_ptr.*}) catch unreachable; e_strings[0] = js_ast.E.String{ - .data = if (entry.value_ptr.value.len > 0) - @as([*]u8, @ptrFromInt(@intFromPtr(entry.value_ptr.value.ptr)))[0..value.len] + .data = if (entry.value_ptr.len > 0) + entry.value_ptr.* else &[_]u8{}, }; @@ -1021,23 +1016,19 @@ const Parser = struct { // https://github.com/oven-sh/bun/issues/1262 if (comptime !override) continue; } else { - allocator.free(entry.value_ptr.value); + allocator.free(entry.value_ptr.*); } } - entry.value_ptr.* = .{ - .value = allocator.dupe(u8, value) catch unreachable, - }; + entry.value_ptr.* = allocator.dupe(u8, value) catch bun.outOfMemory(); } if (comptime !is_process) { var it = map.iterator(); while (it.next()) |entry| { if (count > 0) { count -= 1; - } else if (expandValue(map, entry.value_ptr.value)) |value| { - allocator.free(entry.value_ptr.value); - entry.value_ptr.* = .{ - .value = allocator.dupe(u8, value) catch unreachable, - }; + } else if (expandValue(map, entry.value_ptr.*)) |value| { + allocator.free(entry.value_ptr.*); + entry.value_ptr.* = allocator.dupe(u8, value) catch bun.outOfMemory(); } } } @@ -1056,14 +1047,11 @@ const Parser = struct { }; pub const Map = struct { - const HashTableValue = struct { - value: string, - }; // On Windows, environment variables are case-insensitive. So we use a case-insensitive hash map. // An issue with this exact implementation is unicode characters can technically appear in these // keys, and we use a simple toLowercase function that only applies to ascii, so this will make // some strings collide. - const HashTable = (if (Environment.isWindows) bun.CaseInsensitiveASCIIStringArrayHashMap else bun.StringArrayHashMap)(HashTableValue); + const HashTable = (if (Environment.isWindows) bun.CaseInsensitiveASCIIStringArrayHashMap else bun.StringArrayHashMap)(string); const GetOrPutResult = HashTable.GetOrPutResult; @@ -1086,7 +1074,7 @@ pub const Map = struct { while (it.next()) |pair| { envp_count += 1; // env line is 'KEY=VALUE\x00' - total_bytes += (pair.key_ptr.len + pair.value_ptr.value.len + "=\x00".len); + total_bytes += (pair.key_ptr.len + pair.value_ptr.len + "=\x00".len); } } total_bytes += (envp_count + 1) * @sizeOf(?[*:0]u8); // +1 for the null ptr after the pointer list @@ -1108,12 +1096,12 @@ pub const Map = struct { while (it.next()) |pair| { const variable_buf = string_alloc.allocSentinel( u8, - pair.key_ptr.len + pair.value_ptr.value.len + 1, + pair.key_ptr.len + pair.value_ptr.len + 1, 0, ) catch unreachable; // all bytes were pre-allocated. @memcpy(variable_buf[0..pair.key_ptr.len], pair.key_ptr.*); variable_buf[pair.key_ptr.len] = '='; - @memcpy(variable_buf[pair.key_ptr.len + 1 ..], pair.value_ptr.value); + @memcpy(variable_buf[pair.key_ptr.len + 1 ..], pair.value_ptr.*); envp[i] = variable_buf.ptr; i += 1; } @@ -1137,7 +1125,7 @@ pub const Map = struct { var iter = this.map.iterator(); while (iter.next()) |entry| { - try env_map.hash_map.put(entry.key_ptr.*, entry.value_ptr.value); + try env_map.hash_map.put(entry.key_ptr.*, entry.value_ptr.*); } return .{ .unsafe_map = env_map }; @@ -1194,16 +1182,12 @@ pub const Map = struct { if (Environment.isWindows and Environment.allow_assert) { std.debug.assert(bun.strings.indexOfChar(key, '\x00') == null); } - try this.map.put(key, .{ - .value = value, - }); + try this.map.put(key, value); } pub inline fn putAllocKeyAndValue(this: *Map, allocator: std.mem.Allocator, key: string, value: string) !void { const gop = try this.map.getOrPut(key); - gop.value_ptr.* = .{ - .value = try allocator.dupe(u8, value), - }; + gop.value_ptr.* = try allocator.dupe(u8, value); if (!gop.found_existing) { gop.key_ptr.* = try allocator.dupe(u8, key); } @@ -1211,18 +1195,14 @@ pub const Map = struct { pub inline fn putAllocKey(this: *Map, allocator: std.mem.Allocator, key: string, value: string) !void { const gop = try this.map.getOrPut(key); - gop.value_ptr.* = .{ - .value = value, - }; + gop.value_ptr.* = value; if (!gop.found_existing) { gop.key_ptr.* = try allocator.dupe(u8, key); } } pub inline fn putAllocValue(this: *Map, allocator: std.mem.Allocator, key: string, value: string) !void { - try this.map.put(key, .{ - .value = try allocator.dupe(u8, value), - }); + try this.map.put(key, try allocator.dupe(u8, value)); } pub inline fn getOrPutWithoutValue(this: *Map, key: string) !GetOrPutResult { @@ -1254,19 +1234,15 @@ pub const Map = struct { this: *const Map, key: string, ) ?string { - return if (this.map.get(key)) |entry| entry.value else null; + return this.map.get(key); } pub inline fn putDefault(this: *Map, key: string, value: string) !void { - _ = try this.map.getOrPutValue(key, .{ - .value = value, - }); + _ = try this.map.getOrPutValue(key, value); } pub inline fn getOrPut(this: *Map, key: string, value: string) !void { - _ = try this.map.getOrPutValue(key, .{ - .value = value, - }); + _ = try this.map.getOrPutValue(key, value); } pub fn remove(this: *Map, key: string) void { diff --git a/src/install/install.zig b/src/install/install.zig index d27304dc3bc94..244144376424f 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -2112,9 +2112,7 @@ pub const PackageManager = struct { const init_cwd_gop = try this.env.map.getOrPutWithoutValue("INIT_CWD"); if (!init_cwd_gop.found_existing) { init_cwd_gop.key_ptr.* = try ctx.allocator.dupe(u8, init_cwd_gop.key_ptr.*); - init_cwd_gop.value_ptr.* = .{ - .value = try ctx.allocator.dupe(u8, FileSystem.instance.top_level_dir), - }; + init_cwd_gop.value_ptr.* = try ctx.allocator.dupe(u8, FileSystem.instance.top_level_dir); } this.env.loadCCachePath(this_bundler.fs); diff --git a/src/shell/interpreter.zig b/src/shell/interpreter.zig index 667e46f7420c3..15642507b5b15 100644 --- a/src/shell/interpreter.zig +++ b/src/shell/interpreter.zig @@ -1043,7 +1043,7 @@ pub const Interpreter = struct { var iter = env_loader.iterator(); while (iter.next()) |entry| { - const value = EnvStr.initSlice(entry.value_ptr.value); + const value = EnvStr.initSlice(entry.value_ptr.*); const key = EnvStr.initSlice(entry.key_ptr.*); export_env.insert(key, value); } From a02a374ef1e066758bcbb20e01951a9607dfc302 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 26 Mar 2024 18:41:05 -0700 Subject: [PATCH 06/10] fix window build --- src/bun.js/javascript.zig | 2 +- src/env_loader.zig | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bun.js/javascript.zig b/src/bun.js/javascript.zig index ea3473b6df52f..d9edb54adb42e 100644 --- a/src/bun.js/javascript.zig +++ b/src/bun.js/javascript.zig @@ -769,7 +769,7 @@ pub const VirtualMachine = struct { if (map.map.fetchSwapRemove("BUN_INTERNAL_IPC_FD")) |kv| { if (Environment.isWindows) { - this.initIPCInstance(kv.value.value); + this.initIPCInstance(kv.value); } else if (std.fmt.parseInt(i32, kv.value, 10) catch null) |fd| { this.initIPCInstance(bun.toFD(fd)); } else { diff --git a/src/env_loader.zig b/src/env_loader.zig index 2fd747c9b73f0..b94df564fddc4 100644 --- a/src/env_loader.zig +++ b/src/env_loader.zig @@ -1153,7 +1153,7 @@ pub const Map = struct { if (i + 7 >= result.len) return error.TooManyEnvironmentVariables; result[i] = '='; i += 1; - i += bun.strings.convertUTF8toUTF16InBuffer(result[i..], pair.value_ptr.*.value).len; + i += bun.strings.convertUTF8toUTF16InBuffer(result[i..], pair.value_ptr.*).len; if (i + 5 >= result.len) return error.TooManyEnvironmentVariables; result[i] = 0; i += 1; From c5fe3a0a4497a85cf2a35d70444a64c0cd056ae1 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Wed, 27 Mar 2024 15:15:54 -0700 Subject: [PATCH 07/10] make it possible to deinit createNullDelimitedEnvMap --- src/bun.js/api/bun/subprocess.zig | 8 +-- src/bun.js/event_loop.zig | 2 +- src/cli/bunx_command.zig | 5 +- src/cli/run_command.zig | 31 +++------- src/env_loader.zig | 82 ++++++++++++++++++++----- src/install/install.zig | 7 +-- src/install/lifecycle_script_runner.zig | 9 +-- src/shell/interpreter.zig | 4 +- src/shell/subproc.zig | 62 ++++++++++++------- 9 files changed, 134 insertions(+), 76 deletions(-) diff --git a/src/bun.js/api/bun/subprocess.zig b/src/bun.js/api/bun/subprocess.zig index f4aa21aae9363..7fc9fe40f6fc8 100644 --- a/src/bun.js/api/bun/subprocess.zig +++ b/src/bun.js/api/bun/subprocess.zig @@ -1688,7 +1688,6 @@ pub const Subprocess = struct { } if (args != .zero and args.isObject()) { - // This must run before the stdio parsing happens if (args.getTruthy(globalThis, "ipc")) |val| { if (val.isCell() and val.isCallable(globalThis.vm())) { @@ -1848,9 +1847,8 @@ pub const Subprocess = struct { } if (!override_env and env_array.items.len == 0) { - env_array.items = jsc_vm.bundler.env.map.createNullDelimitedEnvMap(allocator) catch |err| + env_array = jsc_vm.bundler.env.map.createEnvArrayList(allocator) catch |err| return globalThis.handleError(err, "in Bun.spawn"); - env_array.capacity = env_array.items.len; } inline for (0..stdio.len) |fd_index| { @@ -1949,8 +1947,8 @@ pub const Subprocess = struct { var spawned = switch (bun.spawn.spawnProcess( &spawn_options, - @ptrCast(argv.items.ptr), - @ptrCast(env_array.items.ptr), + @ptrCast(argv.items[0 .. argv.items.len - 1 :null].ptr), + @ptrCast(env_array.items[0 .. env_array.items.len - 1 :null].ptr), ) catch |err| { process_allocator.destroy(subprocess); spawn_options.deinit(); diff --git a/src/bun.js/event_loop.zig b/src/bun.js/event_loop.zig index b6a164e13b4f5..17885bc19df53 100644 --- a/src/bun.js/event_loop.zig +++ b/src/bun.js/event_loop.zig @@ -2172,7 +2172,7 @@ pub const EventLoopHandle = union(enum) { this.loop().unref(); } - pub inline fn createNullDelimitedEnvMap(this: @This(), alloc: Allocator) ![:null]?[*:0]u8 { + pub inline fn createNullDelimitedEnvMap(this: @This(), alloc: Allocator) !bun.DotEnv.Map.NullDelimitedEnvMap { return switch (this) { .js => this.js.virtual_machine.bundler.env.map.createNullDelimitedEnvMap(alloc), .mini => this.mini.env.?.map.createNullDelimitedEnvMap(alloc), diff --git a/src/cli/bunx_command.zig b/src/cli/bunx_command.zig index 8f2794a6b7875..6664a3d04c0d2 100644 --- a/src/cli/bunx_command.zig +++ b/src/cli/bunx_command.zig @@ -602,10 +602,13 @@ pub const BunxCommand = struct { debug("installing package: {s}", .{bun.fmt.fmtSlice(argv_to_use, " ")}); this_bundler.env.map.put("BUN_INTERNAL_BUNX_INSTALL", "true") catch bun.outOfMemory(); + const envp = try this_bundler.env.map.createNullDelimitedEnvMap(bun.default_allocator); + defer envp.deinit(bun.default_allocator); + const spawn_result = switch ((bun.spawnSync(&.{ .argv = argv_to_use, - .envp = try this_bundler.env.map.createNullDelimitedEnvMap(bun.default_allocator), + .envp = envp.envp, .cwd = bunx_cache_dir, .stderr = .inherit, diff --git a/src/cli/run_command.zig b/src/cli/run_command.zig index 99e534afbf13c..18775cfe96dc3 100644 --- a/src/cli/run_command.zig +++ b/src/cli/run_command.zig @@ -347,13 +347,14 @@ pub const RunCommand = struct { Output.flush(); } + const envp = try env.map.createNullDelimitedEnvMap(bun.default_allocator); + defer envp.deinit(bun.default_allocator); + const spawn_result = switch ((bun.spawnSync(&.{ .argv = &argv, .argv0 = shell_bin.ptr, - // TODO: remember to free this when we add --filter or --concurrent - // in the meantime we don't need to free it. - .envp = try env.map.createNullDelimitedEnvMap(bun.default_allocator), + .envp = envp.envp, .cwd = cwd, .stderr = .inherit, @@ -513,14 +514,15 @@ pub const RunCommand = struct { argv = try array_list.toOwnedSlice(); } + const envp = try env.map.createNullDelimitedEnvMap(bun.default_allocator); + defer envp.deinit(bun.default_allocator); + const silent = ctx.debug.silent; const spawn_result = bun.spawnSync(&.{ .argv = argv, .argv0 = executableZ, - // TODO: remember to free this when we add --filter or --concurrent - // in the meantime we don't need to free it. - .envp = try env.map.createNullDelimitedEnvMap(bun.default_allocator), + .envp = envp.envp, .cwd = cwd, .stderr = .inherit, @@ -880,16 +882,6 @@ pub const RunCommand = struct { this_bundler.options.production = true; this_bundler.runEnvLoader(true) catch {}; - - // sus - // 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, true) catch {}; - // } else { - // this_bundler.env.load(dir, this_bundler.options.env.files, .development, true) catch {}; - // } - // } } this_bundler.env.map.putDefault("npm_config_local_prefix", this_bundler.fs.top_level_dir) catch unreachable; @@ -1049,11 +1041,8 @@ 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/src/env_loader.zig b/src/env_loader.zig index b94df564fddc4..9c10b4feb3970 100644 --- a/src/env_loader.zig +++ b/src/env_loader.zig @@ -1058,13 +1058,9 @@ pub const Map = struct { map: HashTable, /// Creates a environment block for use in Posix Spawn APIs. - /// As in, a sentinel slice of sentinel string pointers. - /// - /// TODO: the returned value of this is very hard to cleanup manually, and currently must be - /// backed by an arena allocator to avoid a memory leak. We use this in many places without - /// an area, so those usages are leaking here. Solution would be to make this return a struct - /// with a deinit that contained the allocation length. - pub fn createNullDelimitedEnvMap(this: *Map, alloc: std.mem.Allocator) ![:null]?[*:0]u8 { + /// The return value is a struct with a field '.envp' which can be passed to a posix api. + /// Call .deinit with the same allocator to free the memory. + pub fn createNullDelimitedEnvMap(this: *Map, alloc: std.mem.Allocator) !NullDelimitedEnvMap { var env_map = &this.map; var envp_count: usize = 0; @@ -1077,12 +1073,17 @@ pub const Map = struct { total_bytes += (pair.key_ptr.len + pair.value_ptr.len + "=\x00".len); } } - total_bytes += (envp_count + 1) * @sizeOf(?[*:0]u8); // +1 for the null ptr after the pointer list + total_bytes += (envp_count + 1) * @sizeOf(?[*:0]const u8); // +1 for the null ptr after the pointer list - const buf = try alloc.alignedAlloc(u8, @alignOf(?[*:0]u8), total_bytes); + // Instead of creating separate allocations for each string, let's allocate + // enough bytes for everything. The buffer contains the pointers, and then + // all the string bytes afterwards. For this to pass the ptrCast a few + // lines later, we have to make a pointer-aligned allocation. + const buf = try alloc.alignedAlloc(u8, @alignOf(?[*:0]const u8), total_bytes); + comptime std.debug.assert(@TypeOf(buf.ptr) == NullDelimitedEnvMap.BufPtrType); const envp = envp: { - const p: [*]?[*:0]u8 = @ptrCast(buf.ptr); + const p: [*]?[*:0]const u8 = @ptrCast(buf.ptr); p[envp_count] = null; break :envp p[0..envp_count :null]; }; @@ -1110,11 +1111,66 @@ pub const Map = struct { if (comptime Environment.allow_assert) { std.debug.assert(fba.end_index == fba.buffer.len); // incorrect counting above. every pointer should be byte-aligned + std.debug.assert(@intFromPtr(envp.ptr) == @intFromPtr(buf.ptr)); } - return envp; + return .{ + .envp = envp, + // In order to properly free this, the Zig allocator must be passed + // the original slice it was given. The pointer is technically not + // enough information. + .allocation_size = total_bytes, + }; } + /// Creates a environment block for use in Posix Spawn APIs. It is assumed that the caller + /// will want to add other strings to this block, **so there is no null terminator** in the + /// return value. + /// + /// If passing this into a Posix Spawn API, you will need to append a `null`, and then + /// use `list.items[0.. list.items.len - 1 :null]` to get a null-terminated array. + /// + /// Unlike `createNullDelimitedEnvMap`, this does a separate allocation for each string, + /// meaning you likely want to use an arena allocator for the input so you can quickly + /// free everything. + pub fn createEnvArrayList(this: *Map, alloc: std.mem.Allocator) !std.ArrayListUnmanaged(?[*:0]const u8) { + const envp_count = this.map.count(); + // Allocate an extra entry so a caller that wants a null pointer does not have to resize the ArrayList. + var list = try std.ArrayListUnmanaged(?[*:0]const u8).initCapacity(alloc, envp_count + 1); + errdefer { + for (list.items) |item| { + alloc.free(bun.span( + item orelse unreachable, // no null values are added + )); + } + list.deinit(alloc); + } + + var it = this.map.iterator(); + while (it.next()) |pair| { + const variable_buf = try alloc.allocSentinel(u8, pair.key_ptr.len + pair.value_ptr.len + 1, 0); + @memcpy(variable_buf[0..pair.key_ptr.len], pair.key_ptr.*); + variable_buf[pair.key_ptr.len] = '='; + @memcpy(variable_buf[pair.key_ptr.len + 1 ..], pair.value_ptr.*); + list.appendAssumeCapacity(variable_buf.ptr); + } + + return list; + } + + pub const NullDelimitedEnvMap = struct { + envp: [:null]?[*:0]const u8, + allocation_size: usize, + + const BufPtrType = [*]align(@alignOf(?[*:0]const u8)) u8; + + pub inline fn deinit(this: NullDelimitedEnvMap, alloc: std.mem.Allocator) void { + alloc.free( + @as(BufPtrType, @ptrCast(this.envp.ptr))[0..this.allocation_size], + ); + } + }; + /// Returns a wrapper around the std.process.EnvMap that does not duplicate the memory of /// the keys and values, but instead points into the memory of the bun env map. /// @@ -1241,10 +1297,6 @@ pub const Map = struct { _ = try this.map.getOrPutValue(key, value); } - pub inline fn getOrPut(this: *Map, key: string, value: string) !void { - _ = try this.map.getOrPutValue(key, value); - } - pub fn remove(this: *Map, key: string) void { this.map.remove(key); } diff --git a/src/install/install.zig b/src/install/install.zig index 244144376424f..f7e1262385feb 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -10321,13 +10321,12 @@ pub const PackageManager = struct { try PATH.appendSlice(original_path); } - this_bundler.env.map.put("PATH", PATH.items) catch unreachable; - - const envp = try this_bundler.env.map.createNullDelimitedEnvMap(this.allocator); + this_bundler.env.map.put("PATH", PATH.items) catch bun.outOfMemory(); + const env = try this_bundler.env.map.createNullDelimitedEnvMap(this.allocator); try this_bundler.env.map.put("PATH", original_path); PATH.deinit(); - try LifecycleScriptSubprocess.spawnPackageScripts(this, list, envp, log_level); + try LifecycleScriptSubprocess.spawnPackageScripts(this, list, env, log_level); } }; diff --git a/src/install/lifecycle_script_runner.zig b/src/install/lifecycle_script_runner.zig index 6295d852eb4f3..d79de1ab8a630 100644 --- a/src/install/lifecycle_script_runner.zig +++ b/src/install/lifecycle_script_runner.zig @@ -25,7 +25,7 @@ pub const LifecycleScriptSubprocess = struct { stderr: OutputReader = OutputReader.init(@This()), has_called_process_exit: bool = false, manager: *PackageManager, - envp: [:null]?[*:0]u8, + env: bun.DotEnv.Map.NullDelimitedEnvMap, timer: ?Timer = null, @@ -174,7 +174,7 @@ pub const LifecycleScriptSubprocess = struct { }; this.remaining_fds = 0; - var spawned = try (try bun.spawn.spawnProcess(&spawn_options, @ptrCast(&argv), this.envp)).unwrap(); + var spawned = try (try bun.spawn.spawnProcess(&spawn_options, @ptrCast(&argv), this.env.envp)).unwrap(); if (comptime Environment.isPosix) { if (spawned.stdout) |stdout| { @@ -391,18 +391,19 @@ pub const LifecycleScriptSubprocess = struct { this.stderr.deinit(); } + this.env.deinit(this.manager.allocator); this.destroy(); } pub fn spawnPackageScripts( manager: *PackageManager, list: Lockfile.Package.Scripts.List, - envp: [:null]?[*:0]u8, + env: bun.DotEnv.Map.NullDelimitedEnvMap, comptime log_level: PackageManager.Options.LogLevel, ) !void { var lifecycle_subprocess = LifecycleScriptSubprocess.new(.{ .manager = manager, - .envp = envp, + .env = env, .scripts = list, .package_name = list.package_name, }); diff --git a/src/shell/interpreter.zig b/src/shell/interpreter.zig index 15642507b5b15..00b7f0b3dcb0e 100644 --- a/src/shell/interpreter.zig +++ b/src/shell/interpreter.zig @@ -3423,8 +3423,6 @@ pub const Interpreter = struct { var arena = &this.spawn_arena; var arena_allocator = arena.allocator(); var spawn_args = Subprocess.SpawnArgs.default(arena, this.base.interpreter.event_loop, false); - - spawn_args.argv = std.ArrayListUnmanaged(?[*:0]const u8){}; spawn_args.cmd_parent = this; spawn_args.cwd = this.base.shell.cwdZ(); @@ -3608,7 +3606,7 @@ pub const Interpreter = struct { .child = undefined, .buffered_closed = buffered_closed, } }; - const subproc = switch (Subprocess.spawnAsync(this.base.eventLoop(), &shellio, spawn_args, &this.exec.subproc.child)) { + const subproc = switch (Subprocess.spawnAsync(this.base.eventLoop(), &shellio, &spawn_args, &this.exec.subproc.child)) { .result => this.exec.subproc.child, .err => |*e| { this.base.throw(e); diff --git a/src/shell/subproc.zig b/src/shell/subproc.zig index ef3941433496d..8446cf7ef2391 100644 --- a/src/shell/subproc.zig +++ b/src/shell/subproc.zig @@ -610,10 +610,7 @@ pub const ShellSubprocess = struct { cmd_parent: ?*ShellCmd = null, override_env: bool = false, - env_array: std.ArrayListUnmanaged(?[*:0]const u8) = .{ - .items = &.{}, - .capacity = 0, - }, + env_array: std.ArrayListUnmanaged(?[*:0]const u8) = .{}, cwd: []const u8, stdio: [3]Stdio = .{ .ignore, @@ -686,10 +683,7 @@ pub const ShellSubprocess = struct { .arena = arena, .override_env = false, - .env_array = .{ - .items = &.{}, - .capacity = 0, - }, + .env_array = .{}, .cwd = event_loop.topLevelDir(), .stdio = .{ .{ .ignore = {} }, @@ -698,7 +692,7 @@ pub const ShellSubprocess = struct { }, .lazy = false, .PATH = event_loop.env().get("PATH") orelse "", - .argv = undefined, + .argv = .{}, .detached = false, // .ipc_mode = IPCMode.none, // .ipc_callback = .zero, @@ -708,6 +702,7 @@ pub const ShellSubprocess = struct { out.stdio[1] = .{ .pipe = {} }; out.stdio[2] = .{ .pipe = {} }; } + return out; } @@ -752,21 +747,19 @@ pub const ShellSubprocess = struct { pub fn spawnAsync( event_loop: JSC.EventLoopHandle, shellio: *ShellIO, - spawn_args_: SpawnArgs, + spawn_args: *SpawnArgs, out: **@This(), ) bun.shell.Result(void) { var arena = @import("root").bun.ArenaAllocator.init(bun.default_allocator); defer arena.deinit(); - var spawn_args = spawn_args_; - _ = switch (spawnMaybeSyncImpl( .{ .is_sync = false, }, event_loop, arena.allocator(), - &spawn_args, + spawn_args, shellio, out, )) { @@ -789,11 +782,36 @@ pub const ShellSubprocess = struct { ) bun.shell.Result(*@This()) { const is_sync = config.is_sync; - if (!spawn_args.override_env and spawn_args.env_array.items.len == 0) { - // spawn_args.env_array.items = jsc_vm.bundler.env.map.createNullDelimitedEnvMap(allocator) catch bun.outOfMemory(); - spawn_args.env_array.items = event_loop.createNullDelimitedEnvMap(allocator) catch bun.outOfMemory(); - spawn_args.env_array.capacity = spawn_args.env_array.items.len; - } + const override_env = !spawn_args.override_env and spawn_args.env_array.items.len == 0; + const env: bun.DotEnv.Map.NullDelimitedEnvMap = brk: { + if (override_env) { + break :brk event_loop.createNullDelimitedEnvMap(allocator) catch { + return .{ .err = .{ + .custom = bun.default_allocator.dupe(u8, "out of memory") catch bun.outOfMemory(), + } }; + }; + } else { + // Make sure this is a null-terminated + if (spawn_args.env_array.items.len == 0 or + spawn_args.env_array.getLast() != null) + { + spawn_args.argv.append(allocator, null) catch { + return .{ .err = .{ + .custom = bun.default_allocator.dupe(u8, "out of memory") catch bun.outOfMemory(), + } }; + }; + } + + break :brk .{ + .envp = spawn_args.env_array.items[0 .. spawn_args.env_array.items.len - 1 :null], + // undefined is fine because `env.deinit` is only called if the `override_env` path is taken. + .allocation_size = undefined, + }; + } + }; + + // var overridden_env: bun + // defer if (override_env) var should_close_memfd = Environment.isLinux; @@ -845,14 +863,14 @@ pub const ShellSubprocess = struct { return .{ .err = .{ .custom = bun.default_allocator.dupe(u8, "out of memory") catch bun.outOfMemory() } }; }; - spawn_args.env_array.append(allocator, null) catch { - return .{ .err = .{ .custom = bun.default_allocator.dupe(u8, "out of memory") catch bun.outOfMemory() } }; - }; + // spawn_args.env_array.append(allocator, null) catch { + // return .{ .err = .{ .custom = bun.default_allocator.dupe(u8, "out of memory") catch bun.outOfMemory() } }; + // }; var spawn_result = switch (bun.spawn.spawnProcess( &spawn_options, @ptrCast(spawn_args.argv.items.ptr), - @ptrCast(spawn_args.env_array.items.ptr), + env.envp, ) catch |err| { return .{ .err = .{ .custom = std.fmt.allocPrint(bun.default_allocator, "Failed to spawn process: {s}", .{@errorName(err)}) catch bun.outOfMemory() } }; }) { From 96bde2dee15105c40e5661bb9bbd431544de8420 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Wed, 27 Mar 2024 15:51:38 -0700 Subject: [PATCH 08/10] remove comment --- src/shell/subproc.zig | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/shell/subproc.zig b/src/shell/subproc.zig index 8446cf7ef2391..a31cb849d2c9d 100644 --- a/src/shell/subproc.zig +++ b/src/shell/subproc.zig @@ -863,10 +863,6 @@ pub const ShellSubprocess = struct { return .{ .err = .{ .custom = bun.default_allocator.dupe(u8, "out of memory") catch bun.outOfMemory() } }; }; - // spawn_args.env_array.append(allocator, null) catch { - // return .{ .err = .{ .custom = bun.default_allocator.dupe(u8, "out of memory") catch bun.outOfMemory() } }; - // }; - var spawn_result = switch (bun.spawn.spawnProcess( &spawn_options, @ptrCast(spawn_args.argv.items.ptr), From 5c9270dbc25cf5c7ce754aac3da6be66c7109d59 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Wed, 27 Mar 2024 17:22:56 -0700 Subject: [PATCH 09/10] ok --- src/output.zig | 50 +++++++++++++++++++++++++++---------------- src/shell/subproc.zig | 2 +- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/src/output.zig b/src/output.zig index 41b01dbb58100..71887b305ab9d 100644 --- a/src/output.zig +++ b/src/output.zig @@ -567,17 +567,21 @@ pub noinline fn print(comptime fmt: string, args: anytype) callconv(std.builtin. /// BUN_DEBUG_foo=1 /// To enable all logs, set the environment variable /// BUN_DEBUG_ALL=1 -const _log_fn = fn (comptime fmt: string, args: anytype) void; -pub fn scoped(comptime tag: anytype, comptime disabled: bool) _log_fn { +const LogFunction = fn (comptime fmt: string, args: anytype) void; + +pub fn createScope(comptime tag: anytype, comptime disabled: bool) type { const tagname = switch (@TypeOf(tag)) { @Type(.EnumLiteral) => @tagName(tag), []const u8 => tag, - else => @compileError("Output.scoped expected @Type(.EnumLiteral) or []const u8, you gave: " ++ @typeName(@Type(tag))), + else => @compileError("Output.scoped expected @Type(.EnumLiteral) or []const u8, you gave: " ++ @typeName(@TypeOf(tag))), }; if (comptime !Environment.isDebug or !Environment.isNative) { return struct { + pub fn isVisible() bool { + return false; + } pub fn log(comptime _: string, _: anytype) void {} - }.log; + }; } return struct { @@ -589,6 +593,21 @@ pub fn scoped(comptime tag: anytype, comptime disabled: bool) _log_fn { var evaluated_disable = false; var lock = std.Thread.Mutex{}; + pub fn isVisible() bool { + if (!evaluated_disable) { + evaluated_disable = true; + if (bun.getenvZ("BUN_DEBUG_ALL") != null or + bun.getenvZ("BUN_DEBUG_" ++ tagname) != null) + { + really_disable = false; + } else if (bun.getenvZ("BUN_DEBUG_QUIET_LOGS")) |val| { + really_disable = really_disable or !strings.eqlComptime(val, "0"); + } + } + + return !really_disable; + } + /// Debug-only logs which should not appear in release mode /// To enable a specific log at runtime, set the environment variable /// BUN_DEBUG_${TAG} to 1 @@ -605,18 +624,7 @@ pub fn scoped(comptime tag: anytype, comptime disabled: bool) _log_fn { return; } - if (!evaluated_disable) { - evaluated_disable = true; - if (bun.getenvZ("BUN_DEBUG_ALL") != null or - bun.getenvZ("BUN_DEBUG_" ++ tagname) != null) - { - really_disable = false; - } else if (bun.getenvZ("BUN_DEBUG_QUIET_LOGS")) |val| { - really_disable = really_disable or !strings.eqlComptime(val, "0"); - } - } - - if (really_disable) + if (!isVisible()) return; if (!out_set) { @@ -649,7 +657,11 @@ pub fn scoped(comptime tag: anytype, comptime disabled: bool) _log_fn { }; } } - }.log; + }; +} + +pub fn scoped(comptime tag: anytype, comptime disabled: bool) LogFunction { + return createScope(tag, disabled).log; } // Valid "colors": @@ -867,9 +879,11 @@ pub inline fn warn(comptime fmt: []const u8, args: anytype) void { prettyErrorln("warn: " ++ fmt, args); } +const debug_scope = Output.createScope(.debug_warn, false); + /// Print a yellow warning message, only in debug mode pub inline fn debugWarn(comptime fmt: []const u8, args: anytype) void { - if (Environment.isDebug) { + if (Environment.isDebug and debug_scope.isVisible()) { prettyErrorln("debug warn: " ++ fmt, args); flush(); } diff --git a/src/shell/subproc.zig b/src/shell/subproc.zig index a31cb849d2c9d..8615ecd803058 100644 --- a/src/shell/subproc.zig +++ b/src/shell/subproc.zig @@ -795,7 +795,7 @@ pub const ShellSubprocess = struct { if (spawn_args.env_array.items.len == 0 or spawn_args.env_array.getLast() != null) { - spawn_args.argv.append(allocator, null) catch { + spawn_args.env_array.append(allocator, null) catch { return .{ .err = .{ .custom = bun.default_allocator.dupe(u8, "out of memory") catch bun.outOfMemory(), } }; From d934e3ef1206d70add5971d92fa7dad7c3c1d1e8 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Wed, 27 Mar 2024 20:02:15 -0700 Subject: [PATCH 10/10] tweak --- src/bun.js/WebKit | 2 +- src/bun.js/module_loader.zig | 6 +++--- src/shell/interpreter.zig | 2 +- src/shell/subproc.zig | 9 ++++----- 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/bun.js/WebKit b/src/bun.js/WebKit index 64b2fa7da7c07..c3712c13dcdc0 160000 --- a/src/bun.js/WebKit +++ b/src/bun.js/WebKit @@ -1 +1 @@ -Subproject commit 64b2fa7da7c077a3ac7f258d45281f25ac2ef250 +Subproject commit c3712c13dcdc091cfe4c7cb8f2c1fd16472e6f92 diff --git a/src/bun.js/module_loader.zig b/src/bun.js/module_loader.zig index 0432380861e99..a612647fac610 100644 --- a/src/bun.js/module_loader.zig +++ b/src/bun.js/module_loader.zig @@ -183,7 +183,7 @@ fn dumpSourceString(specifier: string, written: []const u8) void { }, }; const dir = std.fs.cwd().makeOpenPath(base_name, .{}) catch |e| { - Output.debug("Failed to dump source string: {}", .{e}); + Output.debugWarn("Failed to dump source string: {}", .{e}); return; }; BunDebugHolder.dir = dir; @@ -196,12 +196,12 @@ fn dumpSourceString(specifier: string, written: []const u8) void { .windows => bun.path.windowsFilesystemRoot(dir_path).len, }; var parent = dir.makeOpenPath(dir_path[root_len..], .{}) catch |e| { - Output.debug("Failed to dump source string: makeOpenPath({s}[{d}..]) {}", .{ dir_path, root_len, e }); + Output.debugWarn("Failed to dump source string: makeOpenPath({s}[{d}..]) {}", .{ dir_path, root_len, e }); return; }; defer parent.close(); parent.writeFile(std.fs.path.basename(specifier), written) catch |e| { - Output.debug("Failed to dump source string: writeFile {}", .{e}); + Output.debugWarn("Failed to dump source string: writeFile {}", .{e}); return; }; } else { diff --git a/src/shell/interpreter.zig b/src/shell/interpreter.zig index da0deccf337e1..e3b416daa55d7 100644 --- a/src/shell/interpreter.zig +++ b/src/shell/interpreter.zig @@ -3698,7 +3698,7 @@ pub const Interpreter = struct { .child = undefined, .buffered_closed = buffered_closed, } }; - const subproc = switch (Subprocess.spawnAsync(this.base.eventLoop(), &shellio, &spawn_args, &this.exec.subproc.child)) { + const subproc = switch (Subprocess.spawnAsync(this.base.eventLoop(), &shellio, spawn_args, &this.exec.subproc.child)) { .result => this.exec.subproc.child, .err => |*e| { this.base.throw(e); diff --git a/src/shell/subproc.zig b/src/shell/subproc.zig index 2fa55553cbbf4..27c9d5bbb97ce 100644 --- a/src/shell/subproc.zig +++ b/src/shell/subproc.zig @@ -747,9 +747,10 @@ pub const ShellSubprocess = struct { pub fn spawnAsync( event_loop: JSC.EventLoopHandle, shellio: *ShellIO, - spawn_args: *SpawnArgs, + spawn_args: SpawnArgs, out: **@This(), ) bun.shell.Result(void) { + var cloned_spawn_args = spawn_args; var arena = @import("root").bun.ArenaAllocator.init(bun.default_allocator); defer arena.deinit(); @@ -759,7 +760,7 @@ pub const ShellSubprocess = struct { }, event_loop, arena.allocator(), - spawn_args, + &cloned_spawn_args, shellio, out, )) { @@ -809,9 +810,7 @@ pub const ShellSubprocess = struct { }; } }; - - // var overridden_env: bun - // defer if (override_env) + defer if (override_env) env.deinit(allocator); var should_close_memfd = Environment.isLinux;