From 0eb1de1a9e6d99e64be08f36d185bcf879aa027d Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Fri, 14 Aug 2026 10:38:47 +0800 Subject: [PATCH 1/2] Give the test suite one temp root instead of fourteen ambient guesses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rule commands run under base_child_env(), which passes PATH and nothing else, so a test the build launches sees no TMPDIR and std::filesystem::temp_directory_path() falls back to bare /tmp. Where /tmp is read-only the scratch write fails, the REQUIRE fires, and Catch2 — built -fno-exceptions — aborts the process. That abort is what the issue recorded as a second, unrelated SIGABRT, and the shard that reported it varied between runs, which is what made it look like a 32-way race. It is neither: one failure, one cause, reproducible in a single sequential process under env -u TMPDIR, with the failed REQUIRE and the SIGABRT in one output. Every test that needs scratch now asks pup::test::temp_root(), which resolves one writable base per process — the ambient temp directory when it works, then /var/tmp, then /dev/shm — and hands out unique paths under it. Each candidate is probed by actually creating a directory rather than assumed. The hardcoded "/tmp/claude" fallback in the e2e fixture is deleted rather than generalized: it was an earlier repair of this same failure applied in the one file where it was noticed, which fixed that instance and left the class alive to be rediscovered here. The class was wider than the grep that found it. Two sites reached for an ambient path in spellings that temp_directory_path() does not match — a getenv("TMPDIR") with a bare "/tmp" literal fed to mkdtemp, and "/tmp" used as a working directory — and the first of them was the only test still failing after every known site had been converted. Scratch has to stay outside any putup project tree, not merely be writable. A working-directory fallback passed every per-test run and then failed the sharded runner, where cwd is test/runner — itself a putup project — with "Attempting to create files the build does not own". putup was right to refuse it; the constraint is now recorded where the resolution happens. Two sites are deliberately left alone: a realpath comparison that needs a real system path to exist rather than to be writable, and the e2e fixture data. The helper is header-only so test/unit/Tupfile keeps its explicit source list and the build graph is unchanged; no bootstrap regeneration is needed. Known and recorded rather than fixed: /dev/shm can be mounted noexec, and e2e fixtures compile and execute binaries in their scratch tree, so a host where both earlier candidates are unwritable and /dev/shm is noexec would fail confusingly. Dropping /dev/shm would abort the suite on the machine this issue is about, where it is the candidate that works, and an honest exec probe means fork+exec at startup in every test process because access(X_OK) does not reflect a noexec mount. Verified: make test exit 0 (170219 assertions, 835 cases, 32 shards); the full suite green in one process under env -u TMPDIR; five formerly failing tests quoted red before the change and green after; format, format-check, tidy, iwyu and spec-check all exit 0. Pair-reviewed by a non-Opus partner: no blockers. Fixes #397 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014wx1bWwRf23eFT82y9D641 --- test/unit/e2e_fixture.cpp | 27 ++-------- test/unit/temp_root.hpp | 79 +++++++++++++++++++++++++++++ test/unit/test_builder.cpp | 6 +-- test/unit/test_exec.cpp | 17 ++++--- test/unit/test_glob.cpp | 12 ++--- test/unit/test_index.cpp | 20 ++++---- test/unit/test_layout.cpp | 15 ++---- test/unit/test_platform_file_io.cpp | 5 +- test/unit/test_platform_process.cpp | 4 +- test/unit/test_strict_checks.cpp | 5 +- test/unit/test_sys.cpp | 10 ++-- test/unit/test_target.cpp | 5 +- 12 files changed, 130 insertions(+), 75 deletions(-) create mode 100644 test/unit/temp_root.hpp diff --git a/test/unit/e2e_fixture.cpp b/test/unit/e2e_fixture.cpp index f5ec3458..4b96d5d6 100644 --- a/test/unit/e2e_fixture.cpp +++ b/test/unit/e2e_fixture.cpp @@ -3,12 +3,13 @@ #include "e2e_fixture.hpp" +#include "temp_root.hpp" + #include "pup/core/global_pool.hpp" #include "pup/core/string_pool.hpp" #include #include -#include #include @@ -24,26 +25,6 @@ auto intern(std::string_view s) -> StringId { return global_pool().intern(s); } namespace { -auto generate_temp_dir() -> fs::path -{ - auto const* tmpdir = std::getenv("TMPDIR"); - if (!tmpdir) - tmpdir = "/tmp/claude"; - - auto base = fs::path { tmpdir }; - if (!fs::exists(base)) - fs::create_directories(base); - - // Generate random suffix - auto rng = std::random_device {}; - auto dist = std::uniform_int_distribution { 0, 0xFFFFFFFF }; - auto suffix = std::to_string(dist(rng)); - - auto result = base / ("e2e_" + suffix); - fs::create_directories(result); - return result; -} - auto copy_fixture(fs::path const& src, fs::path const& dst) -> void { for (auto const& entry : fs::recursive_directory_iterator(src)) { @@ -73,7 +54,7 @@ auto copy_fixture(fs::path const& src, fs::path const& dst) -> void E2EFixture::E2EFixture(std::string_view name) : m_name { name } - , m_workdir { generate_temp_dir() } + , m_workdir { temp_dir("e2e") } , m_fixture_dir { get_fixtures_dir() / name } , m_pup_binary { get_pup_binary() } { @@ -337,7 +318,7 @@ auto run_shell_fixture(std::string_view name) -> ProcessResult } // Create temp directory - auto workdir = generate_temp_dir(); + auto workdir = temp_dir("e2e"); copy_fixture(fixture_dir, workdir); // Copy test.sh too for shell fixtures diff --git a/test/unit/temp_root.hpp b/test/unit/temp_root.hpp new file mode 100644 index 00000000..346abee1 --- /dev/null +++ b/test/unit/temp_root.hpp @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2024 Putup authors + +#pragma once + +#include +#include +#include +#include +#include +#include +#include + +namespace pup::test { + +namespace detail { + +inline auto claim_root(std::filesystem::path const& base) -> std::filesystem::path +{ + auto rng = std::random_device {}; + auto dist = std::uniform_int_distribution { 0, 0xFFFFFFFF }; + for (auto attempt = 0; attempt < 64; ++attempt) { + auto candidate = base / ("pup_test_" + std::to_string(dist(rng))); + auto ec = std::error_code {}; + if (std::filesystem::create_directories(candidate, ec) && !ec) { + return candidate; + } + } + return {}; +} + +inline auto resolve_root() -> std::filesystem::path +{ + // Scratch must be outside any putup project tree: a project rejects files under it that no rule owns. + auto ec = std::error_code {}; + if (auto ambient = std::filesystem::temp_directory_path(ec); !ec && !ambient.empty()) { + if (auto root = claim_root(ambient); !root.empty()) { + return root; + } + } + +#ifndef _WIN32 + // e2e fixtures exec binaries from scratch, so a host relying on /dev/shm needs it mounted exec. + for (auto const* candidate : { "/var/tmp", "/dev/shm" }) { + if (auto root = claim_root(candidate); !root.empty()) { + return root; + } + } +#endif + + std::fprintf(stderr, "temp_root: no writable scratch directory\n"); + std::abort(); +} + +} // namespace detail + +/// Writable scratch directory for this process, resolved once. +inline auto temp_root() -> std::filesystem::path const& +{ + static auto const root = detail::resolve_root(); + return root; +} + +/// A path under this process's scratch directory; the caller creates whatever it needs there. +inline auto temp_path(std::string_view stem) -> std::filesystem::path +{ + return temp_root() / std::string { stem }; +} + +/// A created directory, unique within this process and across concurrent shards. +inline auto temp_dir(std::string_view stem) -> std::filesystem::path +{ + static auto counter = std::atomic { 0 }; + auto path = temp_root() / (std::string { stem } + "_" + std::to_string(counter.fetch_add(1))); + std::filesystem::create_directories(path); + return path; +} + +} // namespace pup::test diff --git a/test/unit/test_builder.cpp b/test/unit/test_builder.cpp index d23c4289..dd99d638 100644 --- a/test/unit/test_builder.cpp +++ b/test/unit/test_builder.cpp @@ -2,6 +2,8 @@ // Copyright (c) 2024 Putup authors #include "catch_amalgamated.hpp" +#include "temp_root.hpp" + #include "pup/core/global_pool.hpp" #include "pup/core/string_pool.hpp" #include "pup/graph/builder.hpp" @@ -83,9 +85,7 @@ class BuilderTestFixture { public: BuilderTestFixture() { - test_root_ = fs::temp_directory_path() - / ("pup_test_builder_" + std::to_string(std::random_device {}())); - fs::create_directories(test_root_); + test_root_ = pup::test::temp_dir("pup_test_builder"); fs::create_directories(test_root_ / "src"); fs::create_directories(test_root_ / "include"); fs::create_directories(test_root_ / "include" / "generated"); diff --git a/test/unit/test_exec.cpp b/test/unit/test_exec.cpp index 064f8e6a..73fb1180 100644 --- a/test/unit/test_exec.cpp +++ b/test/unit/test_exec.cpp @@ -2,6 +2,8 @@ // Copyright (c) 2024 Putup authors #include "catch_amalgamated.hpp" +#include "temp_root.hpp" + #include "pup/core/global_pool.hpp" #include "pup/core/string_pool.hpp" #include "pup/exec/progress_display.hpp" @@ -12,6 +14,7 @@ #include #include +#include #include using namespace pup; @@ -59,12 +62,14 @@ TEST_CASE("CommandRunner basic execution", "[exec]") SECTION("working directory") { - auto opts = RunOptions { .working_dir = intern("/tmp") }; + auto const workdir = pup::test::temp_dir("pup_exec_wd").string(); + auto opts = RunOptions { .working_dir = intern(workdir) }; auto result = runner.run("pwd", opts); REQUIRE(result.has_value()); REQUIRE(result->exit_code == 0); - // May have trailing newline and/or resolve to /private/tmp on macOS - REQUIRE(sv(result->stdout_output).find("tmp") != std::string_view::npos); + // The path may come back resolved (/tmp is /private/tmp on macOS), so match the leaf. + auto const leaf = std::filesystem::path { workdir }.filename().string(); + REQUIRE(sv(result->stdout_output).find(leaf) != std::string_view::npos); } SECTION("environment variable") @@ -408,7 +413,7 @@ TEST_CASE("Scheduler exported_vars", "[exec]") auto output_id = graph::add_file_node(bs.graph, graph::FileNode { .type = NodeType::Generated, - .name = intern("/tmp/test_output.txt"), + .name = intern(pup::test::temp_path("test_output.txt").string()), }); (void)graph::add_edge(bs.graph, *input_id, *cmd_id); @@ -458,7 +463,7 @@ TEST_CASE("Scheduler exported_vars", "[exec]") auto output_id = graph::add_file_node(bs.graph, graph::FileNode { .type = NodeType::Generated, - .name = intern("/tmp/test_export_order.txt"), + .name = intern(pup::test::temp_path("test_export_order.txt").string()), }); (void)graph::add_edge(bs.graph, *input_id, *cmd_id); @@ -501,7 +506,7 @@ TEST_CASE("Scheduler exported_vars", "[exec]") auto output_id = graph::add_file_node(bs.graph, graph::FileNode { .type = NodeType::Generated, - .name = intern("/tmp/test_output2.txt"), + .name = intern(pup::test::temp_path("test_output2.txt").string()), }); (void)graph::add_edge(bs.graph, *input_id, *cmd_id); diff --git a/test/unit/test_glob.cpp b/test/unit/test_glob.cpp index 4286239e..6af772ae 100644 --- a/test/unit/test_glob.cpp +++ b/test/unit/test_glob.cpp @@ -2,6 +2,8 @@ // Copyright (c) 2024 Putup authors #include "catch_amalgamated.hpp" +#include "temp_root.hpp" + #include "pup/core/global_pool.hpp" #include "pup/core/string_pool.hpp" #include "pup/parser/glob.hpp" @@ -28,15 +30,7 @@ class TempDir { // Test shards run as concurrent processes, so the name must be unique across them. TempDir() { - auto rng = std::random_device {}; - auto dist = std::uniform_int_distribution { 0, 0xFFFFFFFF }; - for (;;) { - auto candidate = fs::temp_directory_path() / ("pup_glob_" + std::to_string(dist(rng))); - if (fs::create_directory(candidate)) { - path_ = candidate; - return; - } - } + path_ = pup::test::temp_dir("pup_glob"); } ~TempDir() diff --git a/test/unit/test_index.cpp b/test/unit/test_index.cpp index fa35c6a5..82fb4b49 100644 --- a/test/unit/test_index.cpp +++ b/test/unit/test_index.cpp @@ -2,6 +2,8 @@ // Copyright (c) 2024 Putup authors #include "catch_amalgamated.hpp" +#include "temp_root.hpp" + #include "pup/cli/index_serialize.hpp" #include "pup/core/global_pool.hpp" #include "pup/core/hash.hpp" @@ -479,7 +481,7 @@ TEST_CASE("Index serialization roundtrip", "[e2e][index]") REQUIRE(data->size() > sizeof(RawHeader) + sizeof(RawFooter)); // Write to temp file and read back - auto temp_path = (std::filesystem::temp_directory_path() / "pup_test_index").string(); + auto temp_path = pup::test::temp_path("pup_test_index").string(); auto write_result = write_index(temp_path, index); REQUIRE(write_result.has_value()); @@ -615,7 +617,7 @@ TEST_CASE("Index ID contiguity requirement", "[e2e][index]") }); // Serialize and read back - auto temp_path = (std::filesystem::temp_directory_path() / "pup_test_contiguous").string(); + auto temp_path = pup::test::temp_path("pup_test_contiguous").string(); auto write_result = write_index(temp_path, index); REQUIRE(write_result.has_value()); @@ -665,7 +667,7 @@ TEST_CASE("A record whose declared layout does not fit the file is refused", "[e auto const data = serialize_index(index); REQUIRE(data.has_value()); - auto const path = (std::filesystem::temp_directory_path() / "pup_malicious_test").string(); + auto const path = pup::test::temp_path("pup_malicious_test").string(); auto const past_end = static_cast(data->size()) + 1000; auto const overflowing_count = static_cast(data->size() / sizeof(RawFileEntry)) + 100; @@ -764,7 +766,7 @@ TEST_CASE("A command longer than the string table's entry limit is still recorde .display = intern(oversized.view()), }); - auto temp_path = (std::filesystem::temp_directory_path() / "pup_oversized_command_test").string(); + auto temp_path = pup::test::temp_path("pup_oversized_command_test").string(); REQUIRE(write_index(temp_path, index).has_value()); auto opened = open_index(temp_path); @@ -785,7 +787,7 @@ TEST_CASE("A command longer than the string table's entry limit is still recorde exact.add_file(FileEntry { .id = 1, .parent_id = 0, .type = NodeType::Directory, .name = intern("dir") }); exact.add_command(CommandEntry { .id = cmd_id, .dir_id = 1, .instruction_pattern = intern(at_limit.view()) }); - auto exact_path = (std::filesystem::temp_directory_path() / "pup_at_limit_command_test").string(); + auto exact_path = pup::test::temp_path("pup_at_limit_command_test").string(); REQUIRE(write_index(exact_path, exact).has_value()); auto exact_opened = open_index(exact_path); @@ -864,7 +866,7 @@ TEST_CASE("A command with more than 255 operands records all of them", "[index]" .outputs = outputs, }); - auto temp_path = (std::filesystem::temp_directory_path() / "pup_many_operands_test").string(); + auto temp_path = pup::test::temp_path("pup_many_operands_test").string(); REQUIRE(write_index(temp_path, index).has_value()); auto opened = open_index(temp_path); @@ -1232,7 +1234,7 @@ TEST_CASE("v8 roundtrip with operand sections", "[e2e][index][v8]") REQUIRE(data.has_value()); // Write and read back - auto temp_path = (std::filesystem::temp_directory_path() / "pup_v8_roundtrip_test").string(); + auto temp_path = pup::test::temp_path("pup_v8_roundtrip_test").string(); auto write_result = write_index(temp_path, index); REQUIRE(write_result.has_value()); @@ -1356,7 +1358,7 @@ auto require_graph_index_roundtrip(pup::graph::BuildGraph const& bs, std::string } } - auto temp_path = (std::filesystem::temp_directory_path() / file_tag).string(); + auto temp_path = pup::test::temp_path(file_tag).string(); auto write_result = write_index(temp_path, index); REQUIRE(write_result.has_value()); @@ -1524,7 +1526,7 @@ auto write_bytes(std::string const& path, Vec const& bytes) -> void auto temp_index_path(std::string_view stem) -> std::string { - return (std::filesystem::temp_directory_path() / std::string { stem }).string(); + return pup::test::temp_path(std::string { stem }).string(); } /// A record with one file of every type, laid out under a directory chain so the recovered diff --git a/test/unit/test_layout.cpp b/test/unit/test_layout.cpp index e6a418cf..e884f237 100644 --- a/test/unit/test_layout.cpp +++ b/test/unit/test_layout.cpp @@ -2,6 +2,8 @@ // Copyright (c) 2024 Putup authors #include "catch_amalgamated.hpp" +#include "temp_root.hpp" + #include "pup/core/global_pool.hpp" #include "pup/core/layout.hpp" #include "pup/core/string_pool.hpp" @@ -22,20 +24,9 @@ namespace { /// RAII helper to create a temporary directory tree for testing class TempDir { public: - // Shards run concurrently as separate processes, so the name must be unique - // across processes: std::rand() is unseeded and yields the same sequence in - // every one of them. TempDir() + : path_ { pup::test::temp_dir("pup_test") } { - auto rng = std::random_device {}; - auto dist = std::uniform_int_distribution { 0, 0xFFFFFFFF }; - for (;;) { - auto candidate = fs::temp_directory_path() / ("pup_test_" + std::to_string(dist(rng))); - if (fs::create_directory(candidate)) { - path_ = candidate; - return; - } - } } ~TempDir() diff --git a/test/unit/test_platform_file_io.cpp b/test/unit/test_platform_file_io.cpp index 9e9d9521..0f2e6b23 100644 --- a/test/unit/test_platform_file_io.cpp +++ b/test/unit/test_platform_file_io.cpp @@ -3,6 +3,7 @@ #include "catch_amalgamated.hpp" #include "e2e_fixture.hpp" +#include "temp_root.hpp" #include "pup/core/global_pool.hpp" #include "pup/core/path.hpp" #include "pup/core/string_pool.hpp" @@ -63,7 +64,7 @@ SCENARIO("MappedFile handles missing files", "[platform][file_io]") { GIVEN("a path to a non-existent file") { - auto path = std::string { "/tmp/pup_test_nonexistent_12345.bin" }; + auto path = pup::test::temp_path("pup_test_nonexistent_12345.bin").string(); WHEN("attempting to memory-map the file") { @@ -181,7 +182,7 @@ namespace { class TempDir { public: explicit TempDir(std::string_view name) - : m_path { std::filesystem::temp_directory_path() / (std::string { "pup_walk_" } + std::string { name }) } + : m_path { pup::test::temp_path(std::string { "pup_walk_" } + std::string { name }) } { auto ec = std::error_code {}; std::filesystem::remove_all(m_path, ec); diff --git a/test/unit/test_platform_process.cpp b/test/unit/test_platform_process.cpp index 74eeb572..24f76f14 100644 --- a/test/unit/test_platform_process.cpp +++ b/test/unit/test_platform_process.cpp @@ -2,6 +2,8 @@ // Copyright (c) 2024 Putup authors #include "catch_amalgamated.hpp" +#include "temp_root.hpp" + #include "pup/core/global_pool.hpp" #include "pup/core/string_pool.hpp" #include "pup/platform/process.hpp" @@ -97,8 +99,8 @@ SCENARIO("run_process respects working directory", "[platform][process]") { // A plain temp directory is all this needs — no E2EFixture (which // requires a putup binary and aborts on runners that lack one). + auto workdir = pup::test::temp_dir("pup_run_process_wd") / "subdir"; auto ec = std::error_code {}; - auto workdir = std::filesystem::temp_directory_path(ec) / "pup_run_process_wd" / "subdir"; std::filesystem::create_directories(workdir, ec); auto opts = make_opts(PWD_CMD); diff --git a/test/unit/test_strict_checks.cpp b/test/unit/test_strict_checks.cpp index 227f8562..18e6cf80 100644 --- a/test/unit/test_strict_checks.cpp +++ b/test/unit/test_strict_checks.cpp @@ -2,6 +2,8 @@ // Copyright (c) 2024 Putup authors #include "catch_amalgamated.hpp" +#include "temp_root.hpp" + #include "pup/cli/strict_checks.hpp" #include "pup/core/global_pool.hpp" #include "pup/core/string_pool.hpp" @@ -142,7 +144,8 @@ TEST_CASE("check_assignment: non-Tuprules file — no diagnostic", "[strict]") TEST_CASE("check_component_dirs: missing Tupfile.ini — warning", "[strict]") { - auto dirs = pup::Vec { "/tmp/nonexistent_dir_for_test" }; + auto const missing = pup::test::temp_path("nonexistent_dir_for_test").string(); + auto dirs = pup::Vec { missing }; auto diags = check_component_dirs(dirs); REQUIRE(diags.size() == 1); REQUIRE(diags[0].severity == Diagnostic::Warning); diff --git a/test/unit/test_sys.cpp b/test/unit/test_sys.cpp index a8b3694b..9d67f0f3 100644 --- a/test/unit/test_sys.cpp +++ b/test/unit/test_sys.cpp @@ -4,6 +4,8 @@ #ifndef _WIN32 # include "catch_amalgamated.hpp" +# include "temp_root.hpp" + # include "pup/platform/sys.hpp" # include @@ -45,14 +47,8 @@ struct TempDir { std::string path; TempDir() + : path { pup::test::temp_dir("pup_sys_test").string() } { - auto const* base = std::getenv("TMPDIR"); - auto tmpl = std::string { base != nullptr ? base : "/tmp" } + "/pup_sys_test_XXXXXX"; - auto buf = std::vector(tmpl.begin(), tmpl.end()); - buf.push_back('\0'); - auto* created = mkdtemp(buf.data()); - REQUIRE(created != nullptr); - path = created; } ~TempDir() diff --git a/test/unit/test_target.cpp b/test/unit/test_target.cpp index 745a687f..266f572b 100644 --- a/test/unit/test_target.cpp +++ b/test/unit/test_target.cpp @@ -2,6 +2,8 @@ // Copyright (c) 2024 Putup authors #include "catch_amalgamated.hpp" +#include "temp_root.hpp" + #include "pup/cli/target.hpp" #include "pup/core/global_pool.hpp" #include "pup/core/string_pool.hpp" @@ -19,9 +21,8 @@ auto sv(pup::StringId sid) -> std::string_view { return pup::global_pool().get(s class TempDir { public: TempDir() - : path_(fs::temp_directory_path() / ("pup_target_test_" + std::to_string(std::rand()))) + : path_(pup::test::temp_dir("pup_target_test")) { - fs::create_directories(path_); } ~TempDir() From 8027ca835bc20ca27b49ef543147d328e4f0c27d Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Fri, 14 Aug 2026 10:38:56 +0800 Subject: [PATCH 2/2] Ignore the test runner's generated tup.config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test/runner is a nested putup project and `make test` configures it, so test/runner/tup.config appears after any test run — on main as much as on a branch. It was neither tracked nor ignored, which makes every git status report a modification nobody made and trains the reader to skim the untracked list. Anchored to the one path so it cannot silently ignore a future tracked tup.config elsewhere in the tree. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014wx1bWwRf23eFT82y9D641 --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index 0d698fdf..68245cea 100644 --- a/.gitignore +++ b/.gitignore @@ -16,6 +16,9 @@ cc*.s /build/ /build-*/ +# Nested test-runner project's generated config +/test/runner/tup.config + # Tup database .tup/