From 7e9df2df633c95c41da0ec6374ba8557d290cbc0 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:08:39 +0800 Subject: [PATCH] Reject a rule output that would overwrite a source file (fixes #194) A rule whose output path was also a checked-in source file was accepted, and in an in-tree build putup overwrote the tracked file on disk: a/Tupfile: : gen.src |> cp %f %o |> x.dat (a/x.dat is checked in) before: FROMSRC after: FROMRULE <- the tracked file is gone ensure_file_node silently upgraded an existing File node to Generated (src/graph/dag.cpp:140-145), so the generated node simply won. Upstream tup rejects the project instead, in-tree and in variant builds alike, and the diagnostic here follows its wording including both remedies. The bisect on the issue found no first-bad commit: this reproduces at the earliest commit where a subdirectory rule can run at all, so there is nothing to revert and the check has simply never existed. What putup can decide, and what it cannot. The previous index is what tells a source file apart from our own output sitting in the source tree after an in-tree build -- the distinction tup's persistent node types carry. So the check fires on exactly two answerable questions: with no previous index nothing we produced can be on disk yet, so a file at an output's path is a source; with one, a path recorded as a source File is a source whatever is on disk now. A generated file the previous index does not mention is not decidable from here and is left alone -- that case is real, not hypothetical: a scoped build drops out-of-scope commands from the saved index, and the first draft of this check rejected the subsequent full build because of it. Deleting .pup while in-tree artifacts remain therefore errors until they are cleaned. That is not a regression against tup: deleting .tup in the same situation produces the same error and the same remedy, verified against the tup binary. Config outputs are exempt. The two-stage configure design has a rule produce the tup.config that the same build then reads as configuration, so that path is generated and read on purpose rather than shadowing anything. The [deviation]-tagged scenario added in #193 asserted the old shadow-wins behaviour and said it should be rewritten if this issue were fixed by rejecting the project. It now expects the rejection. The dedup #191 added for this shape is consequently unreachable through a legal project and stays only as a defence, which the test records. Verified: rejection in-tree and out-of-tree with the source file intact; an in-tree rebuild still succeeds with its own output on disk; scoped-then-full in-tree still succeeds; GCC BSP parses (24 Tupfiles, 1853 commands) and busybox parses (31 Tupfiles, 293 commands). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TLob4Ef3qyMd3Dnp9q3XmA --- src/cli/cmd_build.cpp | 78 ++++++++++++++++++++++++++++++++++++++++++ test/unit/test_e2e.cpp | 68 ++++++++++++++++++++++++++++++------ 2 files changed, 136 insertions(+), 10 deletions(-) diff --git a/src/cli/cmd_build.cpp b/src/cli/cmd_build.cpp index 0836e29..923bf4c 100644 --- a/src/cli/cmd_build.cpp +++ b/src/cli/cmd_build.cpp @@ -1425,6 +1425,79 @@ struct NewCommands { pup::Vec forced_cmds; }; +/// A rule may not produce a file that already exists as a source. Upstream tup rejects it +/// ("Attempting to insert '' as a generated node when it already exists as a different +/// type"); putup used to let the generated node win, and in-tree that overwrote the tracked +/// file on disk. The previous index is what tells a source file apart from our own output +/// sitting in the source tree after an in-tree build -- exactly the distinction tup's node +/// types carry. +auto reject_shadowed_sources( + pup::graph::BuildGraph const& state, + pup::index::Index const* old_index, + std::string_view source_root +) -> pup::Result +{ + auto const& g = state.graph; + auto& pool = pup::global_pool(); + auto build_root_name = pup::graph::get_build_root_name(g); + + // Two questions putup can answer for certain. With no previous index nothing we + // produced can be on disk yet, so anything sitting at an output's path is a source. + // With one, a path it recorded as a source File is a source whatever is on disk now. + // Everything else -- notably a generated file the previous index does not mention, + // which is what a scoped build leaves behind for out-of-scope outputs -- is not + // decidable from here and is left alone. + auto recorded_sources = pup::Vec {}; + if (old_index != nullptr) { + for (auto const& file : old_index->files()) { + if (file.type == pup::NodeType::File && !pup::is_empty(file.path)) { + recorded_sources.push_back(file.path); + } + } + std::sort(recorded_sources.begin(), recorded_sources.end(), pup::handle_less); + } + + for (auto id : pup::graph::all_nodes(g)) { + if (pup::node_id::is_command(id) || pup::graph::get(g, id) != pup::NodeType::Generated) { + continue; + } + auto full_path_sv = pup::graph::get_full_path(g, id, state.path_cache); + if (full_path_sv.empty()) { + continue; + } + auto rel_sv = pool.get(pup::strip_path_prefix(full_path_sv, build_root_name)); + // The two-stage configure design has a rule produce the tup.config that the same + // build then reads as configuration, so this one path is generated and read on + // purpose rather than shadowing anything. + if (rel_sv.ends_with("tup.config")) { + continue; + } + auto rel_id = pool.intern(rel_sv); + if (old_index != nullptr) { + if (!std::binary_search(recorded_sources.begin(), recorded_sources.end(), rel_id, pup::handle_less)) { + continue; + } + } else { + auto abs_sv = pool.get(pup::path::join(source_root, rel_sv)); + if (!pup::platform::exists(abs_sv)) { + continue; + } + } + + auto err = pup::Buf {}; + err.fmt( + "Attempting to create '{}' as a generated file when it already exists as a source " + "file. You can do one of two things to fix this:\n" + " 1) If this file is really supposed to be created from the command, delete the " + "file from the filesystem and try again.\n" + " 2) Change your rule in the Tupfile so you aren't trying to overwrite the file.", + rel_sv + ); + return pup::make_error(pup::ErrorCode::DuplicateNode, err.view()); + } + return {}; +} + /// Commands that must run because of what they are rather than because a file changed: /// no counterpart in the previous build, or a counterpart whose signature differs. Their /// outputs join the changed-file set; a command that contributes no output paths cannot @@ -1817,6 +1890,11 @@ auto build_single_variant( auto index_path = pup::global_pool().get(ctx.layout().index_path()); auto const* old_idx_ptr = ctx.old_index(); + + if (auto shadowed = reject_shadowed_sources(bs, old_idx_ptr, source_root_str); !shadowed) { + veprint(variant_name, "Error: {}\n", shadowed.error().msg()); + return EXIT_FAILURE; + } auto use_incremental = false; auto changed_files = pup::Vec {}; auto forced_cmds = pup::Vec {}; diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index 02004e7..8178cbc 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -4522,6 +4522,54 @@ SCENARIO("Editing a rule's recipe does not make it a different rule", "[e2e][ide } } +SCENARIO("A rule may not overwrite a checked-in source file", "[e2e][shadow]") +{ + GIVEN("a rule whose output path is also a source file in the tree") + { + auto f = E2EFixture { "glob_mixed_space" }; + f.write_file("Tupfile", ": gen.src |> cp %f %o |> x.dat\n"); + f.write_file("gen.src", "FROMRULE\n"); + f.write_file("x.dat", "FROMSRC\n"); + + WHEN("the project is built") + { + auto result = f.build(); + + THEN("it is rejected and the source file is untouched") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("x.dat") != std::string::npos); + REQUIRE(f.read_file("x.dat") == "FROMSRC\n"); + } + } + } +} + +SCENARIO("An in-tree rebuild does not mistake its own output for a source file", "[e2e][shadow]") +{ + GIVEN("an ordinary in-tree project built once, so its output is on disk beside the sources") + { + auto f = E2EFixture { "glob_mixed_space" }; + f.write_file("Tupfile", ": src.txt |> cp %f %o |> out.txt\n"); + f.write_file("src.txt", "ORIGINAL\n"); + REQUIRE(f.build().success()); + REQUIRE(f.exists("out.txt")); + + WHEN("it is built again") + { + auto again = f.build(); + + THEN("the rebuild succeeds") + { + INFO("stderr: " << again.stderr_output); + REQUIRE(again.success()); + } + } + } +} + SCENARIO("A glob's %f order does not depend on the build directory's name", "[e2e][glob][pathspace]") { GIVEN("a glob matching one generated and one source file, built out-of-tree") @@ -4557,12 +4605,11 @@ SCENARIO("A glob's %f order does not depend on the build directory's name", "[e2 } } -SCENARIO("A generated file shadowing a source file is one glob match", "[e2e][glob][pathspace][deviation]") +SCENARIO("A glob over a generated file that shadows a source is rejected", "[e2e][glob][pathspace]") { - // Upstream tup rejects this project outright ("Attempting to insert 'x.dat' as a generated - // node when it already exists as a different type"). putup accepts it and lets the generated - // node win; issue #194 tracks that deviation. This asserts only the dedup #191 is about -- - // one match rather than two -- and is not a claim that shadowing should be legal. + // Tagged [deviation] when filed: putup let the generated node win where tup rejects the + // project. Now that #194 rejects it too, the dedup #191 added for this shape is + // unreachable through a legal project and remains only as a defence. GIVEN("a generated file whose name also exists as a checked-in source") { auto f = E2EFixture { "glob_mixed_space" }; @@ -4575,13 +4622,14 @@ SCENARIO("A generated file shadowing a source file is one glob match", "[e2e][gl WHEN("built out-of-tree") { - REQUIRE(f.build({ "-B", "build" }).success()); + auto result = f.build({ "-B", "build" }); - THEN("it is consumed once, not twice") + THEN("the build is rejected and the source file survives") { - auto content = f.read_file("build/a/all.txt"); - INFO("all.txt: " << content); - REQUIRE(content == "FROMRULE\n"); + INFO("stderr: " << result.stderr_output); + REQUIRE_FALSE(result.success()); + REQUIRE(result.stderr_output.find("x.dat") != std::string::npos); + REQUIRE(f.read_file("a/x.dat") == "FROMSRC\n"); } } }