From 62747aa714a2bd61cc091761d8654f920493a10a Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Sun, 20 Sep 2026 13:08:08 +0800 Subject: [PATCH] process: forward TMPDIR, TMP and TEMP to build commands Build commands got PATH alone on POSIX, so a compiler running under a sandbox whose /tmp and /var/tmp are read-only walked libiberty's search (TMPDIR, TMP, TEMP, then fixed directories, then cwd) to its last entry, and cwd is the Tupfile's source directory. GCC deletes those files on a normal exit and not on SIGKILL, so an interrupted -j8 build left one ccXXXXXX.s per in-flight job in the source tree; link steps leave collect2's .res and .cdtor.* the same way, which -pipe cannot prevent. Reproduced here under a read-only /tmp: strace on the old binary shows g++ opening ./cc8Z8hOV.s beside x.cc, and on the new one $TMPDIR/ccepYMwl.s. base_child_env now appends TMPDIR, TMP and TEMP when putup's own environment sets them to a non-empty value and nothing otherwise; an empty value is dropped because tools that test only the pointer (sort, tar) would use "" as the directory where before they saw no variable. The three are the names gcc 14's compile and collect2's link both read, measured by strace with each set alone; TEMPDIR is not forwarded because gcc ignores it and clang, which reads it, falls back to a fixed /tmp rather than cwd. Windows is unchanged: its keep list already carries TEMP and TMP, which the platform always sets ahead of any cwd fallback, so a Windows user with only TMPDIR degrades to TMP rather than to the source tree. Letting putup choose a directory under the build dir was rejected: it overrides a value the user set for a reason (LTO temporaries are large enough that the disk is a choice), adds a mkdir and its failure path, and leaves nobody to delete the directory. The forwarded value stays out of command identity. Only exported variables get a sticky edge, and `export TMPDIR` would fold the path into every command, so a sandbox that hands each session a fresh TMPDIR would rebuild the world every session; that is why "export it" was not the fix. A rule whose text reads $TMPDIR therefore keeps stale output across a TMPDIR change, and the new no-op scenario asserts exactly that as the fence against a later "fix" that adds the edge. That requirement (REQ-ENV-TEMPDIR-IDENTITY) is conditioned on the Tupfile not exporting or importing the variable, since export folds it in by REQ-ENV-SUBPROCESS; it is vacuously true before this change; its scenario goes red on the forwarding assertion, not on identity. `export TMPDIR` in a Tupfile emits the name twice with the same value, as `export PATH` already does. docs/reference.md claimed the minimal environment keeps builds hermetic against ambient shell state. It did not before this change: a rule reading $PATH keeps its output across a PATH change and putup reports nothing to do. The paragraph now states which variables are forwarded, that only exported ones are recorded, and that `export` is the lever when a value must decide staleness. PATH's exclusion from identity has no requirement of its own; tup, whose default list also carries HOME, re-runs every command when a default variable changes, and putup's divergence there is noted in the new requirement's reference rather than settled here. Ref: https://github.com/typeless/putup/issues/478 Co-Authored-By: Claude Fable 5.1 --- docs/reference.md | 25 ++++-- include/pup/platform/process.hpp | 9 +- spec/requirements/command-record.ears.md | 26 +++++- src/platform/process-posix.cpp | 12 +++ .../fixtures/tmpdir_forwarded/Tupfile.fixture | 1 + test/unit/test_e2e.cpp | 57 +++++++++++++ test/unit/test_platform_process.cpp | 83 +++++++++++++++++++ 7 files changed, 201 insertions(+), 12 deletions(-) create mode 100644 test/e2e/fixtures/tmpdir_forwarded/Tupfile.fixture 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 +}