From ca461f9e83b2c692442bb7e20c2f9e3670d2ef6a Mon Sep 17 00:00:00 2001 From: Dylan Conway <35280289+dylan-conway@users.noreply.github.com> Date: Mon, 11 Sep 2023 17:19:21 -0700 Subject: fix loading env from `.env.production` and friends (#4630) * reload conditional vars * test * change `get` and `put` methods * dont clone empty env variables --- Makefile | 2 +- src/bun.js/api/bun.zig | 2 +- src/bun.js/javascript.zig | 2 +- src/bundler.zig | 6 ++-- src/cli/run_command.zig | 4 +-- src/env_loader.zig | 89 +++++++++++++++++++++++++++++++---------------- src/install/install.zig | 2 +- src/install/lockfile.zig | 2 +- test/cli/run/env.test.ts | 43 ++++++++++++++++++++++- 9 files changed, 111 insertions(+), 41 deletions(-) diff --git a/Makefile b/Makefile index 49d6535a7..b3fe2d6bf 100644 --- a/Makefile +++ b/Makefile @@ -1880,7 +1880,7 @@ PACKAGE_MAP = --pkg-begin async_io $(BUN_DIR)/src/io/io_darwin.zig --pkg-begin b .PHONY: base64 base64: - cd $(BUN_DEPS_DIR)/base64 && make clean && cmake $(CMAKE_FLAGS) . && make + cd $(BUN_DEPS_DIR)/base64 && make clean && rm -rf CMakeCache.txt CMakeFiles && cmake $(CMAKE_FLAGS) . && make cp $(BUN_DEPS_DIR)/base64/libbase64.a $(BUN_DEPS_OUT_DIR)/libbase64.a .PHONY: cold-jsc-start diff --git a/src/bun.js/api/bun.zig b/src/bun.js/api/bun.zig index 36e52821f..966c82d38 100644 --- a/src/bun.js/api/bun.zig +++ b/src/bun.js/api/bun.zig @@ -4431,7 +4431,7 @@ pub const EnvironmentVariables = struct { var vm = globalObject.bunVM(); var sliced = name.toSlice(vm.allocator); defer sliced.deinit(); - const value = vm.bundler.env.map.map.get(sliced.slice()) orelse return null; + const value = vm.bundler.env.map.get(sliced.slice()) orelse return null; return ZigString.initUTF8(value); } }; diff --git a/src/bun.js/javascript.zig b/src/bun.js/javascript.zig index 0d73c31c5..752fcb3a4 100644 --- a/src/bun.js/javascript.zig +++ b/src/bun.js/javascript.zig @@ -670,7 +670,7 @@ pub const VirtualMachine = struct { } if (map.map.fetchSwapRemove("BUN_INTERNAL_IPC_FD")) |kv| { - if (std.fmt.parseInt(i32, kv.value, 10) catch null) |fd| { + if (std.fmt.parseInt(i32, kv.value.value, 10) catch null) |fd| { this.initIPCInstance(fd); } else { Output.printErrorln("Failed to parse BUN_INTERNAL_IPC_FD", .{}); diff --git a/src/bundler.zig b/src/bundler.zig index 66443d165..76fbc02ee 100644 --- a/src/bundler.zig +++ b/src/bundler.zig @@ -472,11 +472,11 @@ pub const Bundler = struct { } if (!has_production_env and this.options.isTest()) { - try this.env.load(&this.fs.fs, dir, .@"test"); + try this.env.load(dir, .@"test"); } else if (this.options.production) { - try this.env.load(&this.fs.fs, dir, .production); + try this.env.load(dir, .production); } else { - try this.env.load(&this.fs.fs, dir, .development); + try this.env.load(dir, .development); } }, .disable => { diff --git a/src/cli/run_command.zig b/src/cli/run_command.zig index f637bfe6c..dcb5312b1 100644 --- a/src/cli/run_command.zig +++ b/src/cli/run_command.zig @@ -525,9 +525,9 @@ pub const RunCommand = struct { 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(&this_bundler.fs.fs, dir, .production) catch {}; + this_bundler.env.load(dir, .production) catch {}; } else { - this_bundler.env.load(&this_bundler.fs.fs, dir, .development) catch {}; + this_bundler.env.load(dir, .development) catch {}; } } } diff --git a/src/env_loader.zig b/src/env_loader.zig index 7c2c38a99..7f0be98a7 100644 --- a/src/env_loader.zig +++ b/src/env_loader.zig @@ -231,7 +231,7 @@ pub const Loader = struct { if (behavior == .prefix) { while (iter.next()) |entry| { - const value: string = entry.value_ptr.*; + const value: string = entry.value_ptr.value; if (strings.startsWith(entry.key_ptr.*, prefix)) { const key_str = std.fmt.allocPrint(key_allocator, "process.env.{s}", .{entry.key_ptr.*}) catch unreachable; @@ -282,12 +282,12 @@ pub const Loader = struct { } } else { while (iter.next()) |entry| { - const value: string = if (entry.value_ptr.*.len == 0) empty_string_value else entry.value_ptr.*; + const value: string = if (entry.value_ptr.value.len == 0) empty_string_value else 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.*.len > 0) - @as([*]u8, @ptrFromInt(@intFromPtr(entry.value_ptr.*.ptr)))[0..value.len] + .data = if (entry.value_ptr.value.len > 0) + @as([*]u8, @ptrFromInt(@intFromPtr(entry.value_ptr.value.ptr)))[0..value.len] else &[_]u8{}, }; @@ -370,7 +370,6 @@ pub const Loader = struct { // .env goes last pub fn load( this: *Loader, - fs: *Fs.FileSystem.RealFS, dir: *Fs.FileSystem.DirEntry, comptime suffix: enum { development, production, @"test" }, ) !void { @@ -380,19 +379,19 @@ pub const Loader = struct { switch (comptime suffix) { .development => { if (dir.hasComptimeQuery(".env.development.local")) { - try this.loadEnvFile(fs, dir_handle, ".env.development.local", false); + try this.loadEnvFile(dir_handle, ".env.development.local", false, true); Analytics.Features.dotenv = true; } }, .production => { if (dir.hasComptimeQuery(".env.production.local")) { - try this.loadEnvFile(fs, dir_handle, ".env.production.local", false); + try this.loadEnvFile(dir_handle, ".env.production.local", false, true); Analytics.Features.dotenv = true; } }, .@"test" => { if (dir.hasComptimeQuery(".env.test.local")) { - try this.loadEnvFile(fs, dir_handle, ".env.test.local", false); + try this.loadEnvFile(dir_handle, ".env.test.local", false, true); Analytics.Features.dotenv = true; } }, @@ -400,7 +399,7 @@ pub const Loader = struct { if (comptime suffix != .@"test") { if (dir.hasComptimeQuery(".env.local")) { - try this.loadEnvFile(fs, dir_handle, ".env.local", false); + try this.loadEnvFile(dir_handle, ".env.local", false, false); Analytics.Features.dotenv = true; } } @@ -408,26 +407,26 @@ pub const Loader = struct { switch (comptime suffix) { .development => { if (dir.hasComptimeQuery(".env.development")) { - try this.loadEnvFile(fs, dir_handle, ".env.development", false); + try this.loadEnvFile(dir_handle, ".env.development", false, true); Analytics.Features.dotenv = true; } }, .production => { if (dir.hasComptimeQuery(".env.production")) { - try this.loadEnvFile(fs, dir_handle, ".env.production", false); + try this.loadEnvFile(dir_handle, ".env.production", false, true); Analytics.Features.dotenv = true; } }, .@"test" => { if (dir.hasComptimeQuery(".env.test")) { - try this.loadEnvFile(fs, dir_handle, ".env.test", false); + try this.loadEnvFile(dir_handle, ".env.test", false, true); Analytics.Features.dotenv = true; } }, } if (dir.hasComptimeQuery(".env")) { - try this.loadEnvFile(fs, dir_handle, ".env", false); + try this.loadEnvFile(dir_handle, ".env", false, false); Analytics.Features.dotenv = true; } @@ -487,8 +486,13 @@ pub const Loader = struct { Output.flush(); } - pub fn loadEnvFile(this: *Loader, fs: *Fs.FileSystem.RealFS, dir: std.fs.Dir, comptime base: string, comptime override: bool) !void { - _ = fs; + pub fn loadEnvFile( + this: *Loader, + dir: std.fs.Dir, + comptime base: string, + comptime override: bool, + comptime conditional: bool, + ) !void { if (@field(this, base) != null) { return; } @@ -565,6 +569,7 @@ pub const Loader = struct { this.map, override, false, + conditional, ); @field(this, base) = source; @@ -789,6 +794,7 @@ const Parser = struct { map: *Map, comptime override: bool, comptime is_process: bool, + comptime conditional: bool, ) void { var count = map.map.count(); while (this.pos < this.src.len) { @@ -804,19 +810,25 @@ const Parser = struct { // https://github.com/oven-sh/bun/issues/1262 if (comptime !override) continue; } else { - allocator.free(entry.value_ptr.*); + allocator.free(entry.value_ptr.value); } } - entry.value_ptr.* = allocator.dupe(u8, value) catch unreachable; + entry.value_ptr.* = .{ + .value = allocator.dupe(u8, value) catch unreachable, + .conditional = conditional, + }; } if (comptime !is_process) { var it = map.iter(); while (it.next()) |entry| { if (count > 0) { count -= 1; - } else if (expandValue(map, entry.value_ptr.*)) |value| { - allocator.free(entry.value_ptr.*); - entry.value_ptr.* = allocator.dupe(u8, value) catch unreachable; + } 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, + .conditional = conditional, + }; } } } @@ -828,14 +840,19 @@ 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); + parser._parse(allocator, map, override, is_process, conditional); } }; pub const Map = struct { - const HashTable = bun.StringArrayHashMap(string); + const HashTableValue = struct { + value: string, + conditional: bool, + }; + const HashTable = bun.StringArrayHashMap(HashTableValue); map: HashTable, @@ -848,10 +865,10 @@ pub const Map = struct { 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.len + 1, 0); + 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.*); + bun.copy(u8, env_buf[pair.key_ptr.len + 1 ..], pair.value_ptr.value); envp_buf[i] = env_buf.ptr; } std.debug.assert(i == envp_count); @@ -864,7 +881,10 @@ pub const Map = struct { var iter_ = this.map.iterator(); while (iter_.next()) |entry| { - try env_map.putMove(bun.constStrToU8(entry.key_ptr.*), bun.constStrToU8(entry.value_ptr.*)); + // Allow var from .env.development or .env.production to be loaded again. Also don't clone empty vars. + if (!entry.value_ptr.conditional and entry.value_ptr.value.len > 0) { + try env_map.putMove(bun.constStrToU8(entry.key_ptr.*), bun.constStrToU8(entry.value_ptr.value)); + } } return env_map; @@ -879,7 +899,10 @@ pub const Map = struct { } pub inline fn put(this: *Map, key: string, value: string) !void { - try this.map.put(key, value); + try this.map.put(key, .{ + .value = value, + .conditional = false, + }); } pub fn jsonStringify(self: *const @This(), writer: anytype) !void { @@ -907,22 +930,28 @@ pub const Map = struct { this: *const Map, key: string, ) ?string { - return this.map.get(key); + return if (this.map.get(key)) |entry| entry.value else null; } pub fn get_( this: *const Map, key: string, ) ?string { - return this.map.get(key); + return if (this.map.get(key)) |entry| entry.value else null; } pub inline fn putDefault(this: *Map, key: string, value: string) !void { - _ = try this.map.getOrPutValue(key, value); + _ = 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); + _ = try this.map.getOrPutValue(key, .{ + .value = value, + .conditional = false, + }); } }; diff --git a/src/install/install.zig b/src/install/install.zig index e8b95b820..bf6422cc7 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -5243,7 +5243,7 @@ pub const PackageManager = struct { }; env.loadProcess(); - try env.load(&fs.fs, entries_option.entries, .production); + try env.load(entries_option.entries, .production); if (env.map.get("BUN_INSTALL_VERBOSE") != null) { PackageManager.verbose_install = true; diff --git a/src/install/lockfile.zig b/src/install/lockfile.zig index 0b5e0d7bc..a9bee2d27 100644 --- a/src/install/lockfile.zig +++ b/src/install/lockfile.zig @@ -983,7 +983,7 @@ pub const Printer = struct { }; env_loader.loadProcess(); - try env_loader.load(&fs.fs, entries_option.entries, .production); + try env_loader.load(entries_option.entries, .production); 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 159793e27..3ed300477 100644 --- a/test/cli/run/env.test.ts +++ b/test/cli/run/env.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { bunRun, bunTest, tempDirWithFiles, bunExe, bunEnv } from "harness"; +import { bunRun, bunRunAsScript, bunTest, tempDirWithFiles, bunExe, bunEnv } from "harness"; import path from "path"; function bunRunWithoutTrim(file: string, env?: Record) { @@ -255,6 +255,47 @@ test(".env process variables no comments", () => { expect(stdout).toBe('test#1 "test#2"'); }); +describe("package scripts load from .env.production and .env.development", () => { + test("NODE_ENV=production", () => { + const dir = tempDirWithFiles("dotenv-package-script-prod", { + "index.ts": "console.log(process.env.TEST);", + "package.json": ` + { + "name": "foo", + "version": "2.0", + "scripts": { + "test": "NODE_ENV=production ${bunExe()} run index.ts", + } + } + `, + ".env.production": "TEST=prod", + ".env.development": "TEST=dev", + }); + + const { stdout } = bunRunAsScript(dir, "test"); + expect(stdout).toBe("prod"); + }); + test("NODE_ENV=development", () => { + const dir = tempDirWithFiles("dotenv-package-script-prod", { + "index.ts": "console.log(process.env.TEST);", + "package.json": ` + { + "name": "foo", + "version": "2.0", + "scripts": { + "test": "NODE_ENV=development ${bunExe()} run index.ts", + } + } + `, + ".env.production": "TEST=prod", + ".env.development": "TEST=dev", + }); + + const { stdout } = bunRunAsScript(dir, "test"); + expect(stdout).toBe("dev"); + }); +}); + test(".env escaped dollar sign", () => { const dir = tempDirWithFiles("dotenv-dollar", { ".env": "FOO=foo\nBAR=\\$FOO", -- cgit v1.2.3