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"); } } }