diff --git a/docs/reference.md b/docs/reference.md index 3400ef4b..2748d692 100644 --- a/docs/reference.md +++ b/docs/reference.md @@ -1142,14 +1142,23 @@ export PKG_CONFIG_PATH : foo.c |> $(CC) -c %f -o %o |> foo.o ``` -Commands run with a **minimal environment**: on POSIX only `PATH` is passed -through (Windows adds the system set: `SystemRoot`, `ComSpec`, `PATHEXT`, -`TEMP`, `TMP`, `windir`). Every other variable a command reads must be -`export`ed — an unexported variable is simply absent from the child -environment. This keeps builds hermetic: ambient shell state can't silently -change outputs. Exported variables are folded into command identity, so -changing an exported variable's **value** re-runs the affected commands even -when the command text is unchanged. +Commands run with a **minimal environment**: on POSIX `PATH`, plus `TMPDIR`, +`TMP` and `TEMP` when putup's own environment sets them to a non-empty value +(Windows passes `PATH` plus the system set: `SystemRoot`, `ComSpec`, `PATHEXT`, `TEMP`, `TMP`, `windir`). The +temporary-directory variables are forwarded because a command runs with its +Tupfile's directory as cwd, and a tool that finds no writable temporary +directory falls back to cwd, which puts its scratch files in the **source** +tree. Every other variable a command reads must be `export`ed — an unexported +variable is simply absent from the child environment. + +Only `export`ed variables are folded into command identity, so changing an +exported variable's **value** re-runs the affected commands even when the +command text is unchanged. The forwarded set above is **not** recorded: a +command whose text reads `$PATH` or `$TMPDIR` sees whatever the invoking shell +had, and changing that value alone re-runs nothing, except that a bare name in +`CONFIG_TRACKED_TOOLS` (§6.1) is resolved through `PATH`, so a `PATH` change +that resolves it to a different binary re-runs every command. `export` the +variable if its value must decide whether a command is out of date. **`import`** - Import an environment variable into the Tupfile namespace: ```tup diff --git a/include/pup/platform/process.hpp b/include/pup/platform/process.hpp index 3e74f961..be2e8cd8 100644 --- a/include/pup/platform/process.hpp +++ b/include/pup/platform/process.hpp @@ -60,9 +60,12 @@ auto build_env_strings( bool inherit_env ) -> Vec; -/// Minimal "VAR=value" environment for build commands: only the variables a -/// child needs to run tools at all (POSIX: PATH; Windows adds the system set). -/// Everything else must be passed explicitly via `export`. +/// Minimal "VAR=value" environment for build commands: the variables a child +/// needs to run tools at all, and the ones its toolchain searches to place +/// temporaries (POSIX: PATH, plus TMPDIR, TMP and TEMP when set; Windows: the +/// system set, whose TEMP and TMP serve that role). A variable here is +/// forwarded, not recorded: its value is absent from command identity. +/// Everything else must be passed via `export`. [[nodiscard]] auto base_child_env() -> Vec; diff --git a/spec/requirements/command-record.ears.md b/spec/requirements/command-record.ears.md index 6435de67..c5714b0e 100644 --- a/spec/requirements/command-record.ears.md +++ b/spec/requirements/command-record.ears.md @@ -651,7 +651,8 @@ conditional branch, putup shall schedule no command. ## Group: env-values -The configuration and environment values a command's identity depends on. +The configuration and environment values a command's identity depends on, and the environment +its subprocess receives. ### REQ-ENV-RECORD @@ -722,3 +723,26 @@ the commands the newly taken branch declares rather than reporting the build up While a variable a previous build imported is absent from the environment, putup shall use the value it recorded for that variable rather than an empty one. + +### REQ-ENV-TEMPDIR + +- leg: invariant +- conformance: deliberate-deviation +- reference: upstream's per-command default environment is `default_env[]` in environ.c (`PATH` and `HOME` on every platform; under `_WIN32` also `SYSTEMROOT`, `TEMP`, `TMP` and a set of Visual Studio variables), collected into each Tupfile by `environ_add_defaults` and emitted into the subprocess block by `tup_db_get_environ` (db.c), so on POSIX tup forwards no temporary-directory variable and a child falls back to `/tmp`; putup forwards the three GCC's `choose_tmpdir` (libiberty) reads ahead of `/tmp` and cwd because a putup command's cwd is its Tupfile's source directory, so with `/tmp` unwritable the fallback writes scratch files into the source tree (issue #478); putup's Windows system set already carries `TEMP` and `TMP`, which the platform always sets ahead of any cwd fallback, so `TMPDIR` is not added there; the same upstream list carries `HOME`, which putup still does not forward, unchanged by this requirement +- discharge: test "base_child_env forwards the temporary-directory variables the tools read" +- discharge: test "Scenario: TMPDIR set for putup reaches every build command" + +When a temporary-directory variable its platform's tools read (`TMPDIR`, `TMP` or `TEMP` on +POSIX; `TEMP` or `TMP` on Windows) is set to a non-empty value in putup's own environment, putup +shall give every command's subprocess that variable with that value, and shall give it no such +variable that is unset or empty. + +### REQ-ENV-TEMPDIR-IDENTITY + +- leg: invariant +- conformance: deliberate-deviation +- reference: upstream's `default_env[]` (environ.c) carries `TEMP` and `TMP` only under `_WIN32` and never `TMPDIR`; there `environ_add_defaults` makes each a sticky env node and `tup_db_check_env` (db.c) marks it modified when `getenv` disagrees with the stored `VAR=value`, so on Windows tup re-runs every command when `TEMP` or `TMP` changes while putup's Windows system set forwards them unrecorded, and on POSIX upstream forwards none of the three so both agree that a change re-runs nothing; putup records only exported variables (REQ-ENV-SUBPROCESS), and a temporary-directory value says where scratch files live rather than what a command produces, so a sandbox that hands each session a fresh `TMPDIR` would otherwise re-run the whole build every session +- discharge: test "Scenario: Changing TMPDIR alone re-runs no command" + +While a Tupfile neither exports nor imports a temporary-directory variable putup forwards, putup +shall exclude that variable's value from the identity of every command it declares. diff --git a/src/platform/process-posix.cpp b/src/platform/process-posix.cpp index 53f66a1f..202858d7 100644 --- a/src/platform/process-posix.cpp +++ b/src/platform/process-posix.cpp @@ -60,6 +60,18 @@ auto base_child_env() -> Vec buf.append(std::string_view { "/usr/bin:/bin" }); } result.push_back(buf.intern(pool)); + static constexpr char const* forwarded_when_set[] = { "TMPDIR", "TMP", "TEMP" }; + for (auto const* name : forwarded_when_set) { + auto const* value = sys::getenv(name); + if (value == nullptr || *value == '\0') { + continue; + } + auto var = Buf {}; + var.append(std::string_view { name }); + var.append('='); + var.append(std::string_view { value }); + result.push_back(var.intern(pool)); + } return result; } diff --git a/test/e2e/fixtures/tmpdir_forwarded/Tupfile.fixture b/test/e2e/fixtures/tmpdir_forwarded/Tupfile.fixture new file mode 100644 index 00000000..5c293a78 --- /dev/null +++ b/test/e2e/fixtures/tmpdir_forwarded/Tupfile.fixture @@ -0,0 +1 @@ +: |> echo "TMPDIR=$TMPDIR" > %o |> out.txt diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index f33d1a9d..3806c37c 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -9214,6 +9214,63 @@ SCENARIO("Exported env var consumed via subprocess environment triggers rebuild" } } +SCENARIO("TMPDIR set for putup reaches every build command", "[e2e][envdep]") +{ + GIVEN("a rule that prints the TMPDIR its shell sees") + { + auto f = E2EFixture { "tmpdir_forwarded" }; + f.mkdir("scratch"); + auto scratch = (f.workdir() / "scratch").string(); + REQUIRE(f.init().success()); + + WHEN("putup runs with TMPDIR set in its own environment") + { + auto env = EnvGuard { "TMPDIR", scratch }; + auto result = f.build(); + + THEN("the command's environment carries the same value") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE(result.success()); + REQUIRE(f.read_file("out.txt") == "TMPDIR=" + scratch + "\n"); + } + } + } +} + +SCENARIO("Changing TMPDIR alone re-runs no command", "[e2e][envdep]") +{ + GIVEN("a project built with one TMPDIR by a rule whose text reads it") + { + auto f = E2EFixture { "tmpdir_forwarded" }; + f.mkdir("first"); + f.mkdir("second"); + auto first = (f.workdir() / "first").string(); + auto second = (f.workdir() / "second").string(); + REQUIRE(f.init().success()); + { + auto env = EnvGuard { "TMPDIR", first }; + REQUIRE(f.build().success()); + REQUIRE(f.read_file("out.txt") == "TMPDIR=" + first + "\n"); + } + + WHEN("putup runs again with a different TMPDIR and nothing else changed") + { + auto env = EnvGuard { "TMPDIR", second }; + auto result = f.build(); + + THEN("the build is a no-op and the output keeps the first value") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE(result.is_noop()); + REQUIRE(f.read_file("out.txt") == "TMPDIR=" + first + "\n"); + } + } + } +} + SCENARIO("Tracked tool binaries fold into command identity", "[e2e][incremental]") { GIVEN("a project whose config tracks a tool that is not a rule input") diff --git a/test/unit/test_platform_process.cpp b/test/unit/test_platform_process.cpp index 24f76f14..05c163b3 100644 --- a/test/unit/test_platform_process.cpp +++ b/test/unit/test_platform_process.cpp @@ -2,18 +2,21 @@ // Copyright (c) 2024 Putup authors #include "catch_amalgamated.hpp" +#include "e2e_fixture.hpp" #include "temp_root.hpp" #include "pup/core/global_pool.hpp" #include "pup/core/string_pool.hpp" #include "pup/platform/process.hpp" +#include #include #include using namespace pup::platform; using pup::StringId; using pup::global_pool; +using pup::test::EnvGuard; namespace { @@ -226,3 +229,83 @@ TEST_CASE("build_env_strings constructs environment list", "[platform][process]" REQUIRE(has_extra); } } + +TEST_CASE("base_child_env forwards the temporary-directory variables the tools read", "[platform][process]") +{ + auto has_name = [](pup::Vec const& env, std::string_view name) { + for (auto var : env) { + auto entry = sv(var); + if (entry.find('=') != name.size()) { + continue; + } + auto same = true; + for (std::size_t i = 0; i < name.size(); ++i) { +#ifdef _WIN32 + same = std::toupper(static_cast(entry[i])) == name[i]; +#else + same = entry[i] == name[i]; +#endif + if (!same) { + break; + } + } + if (same) { + return true; + } + } + return false; + }; + +#ifdef _WIN32 + SECTION("the Windows keep list carries TEMP and TMP and not TMPDIR") + { + auto tmpdir = EnvGuard { "TMPDIR", "C:\\pup-probe\\tmpdir" }; + auto env = base_child_env(); + + REQUIRE(has_name(env, "TEMP")); + REQUIRE(has_name(env, "TMP")); + REQUIRE_FALSE(has_name(env, "TMPDIR")); + } +#else + auto contains = [](pup::Vec const& env, std::string_view entry) { + for (auto var : env) { + if (sv(var) == entry) { + return true; + } + } + return false; + }; + + SECTION("TMPDIR, TMP and TEMP reach the child when set") + { + auto tmpdir = EnvGuard { "TMPDIR", "/pup-probe/tmpdir" }; + auto tmp = EnvGuard { "TMP", "/pup-probe/tmp" }; + auto temp = EnvGuard { "TEMP", "/pup-probe/temp" }; + auto env = base_child_env(); + + REQUIRE(contains(env, "TMPDIR=/pup-probe/tmpdir")); + REQUIRE(contains(env, "TMP=/pup-probe/tmp")); + REQUIRE(contains(env, "TEMP=/pup-probe/temp")); + REQUIRE(has_name(env, "PATH")); + } + + SECTION("an unset TMPDIR is absent rather than empty") + { + auto restore = EnvGuard { "TMPDIR", "/pup-probe/tmpdir" }; + pup::platform::unset_env("TMPDIR"); + auto env = base_child_env(); + + REQUIRE_FALSE(has_name(env, "TMPDIR")); + REQUIRE(has_name(env, "PATH")); + } + + SECTION("a set-but-empty TMPDIR is absent rather than empty") + { + auto empty = EnvGuard { "TMPDIR", "" }; + auto env = base_child_env(); + + REQUIRE_FALSE(has_name(env, "TMPDIR")); + REQUIRE(has_name(env, "PATH")); + } +#endif +}