diff --git a/modules/buildmcpp/src/directives.cppm b/modules/buildmcpp/src/directives.cppm index aff2044c..54044235 100644 --- a/modules/buildmcpp/src/directives.cppm +++ b/modules/buildmcpp/src/directives.cppm @@ -657,13 +657,27 @@ std::string target_directive_error(const mcpp::manifest::Manifest& m, const Dire // both the directive and the declaring package, like `target_directive_error`. std::string deploy_directive_error(const mcpp::manifest::Manifest& m, const Directives& d); +// Own only the files created for the prepare-time source scan. They must be +// gone before ninja checks the outputs: a new placeholder is newer than an +// action's inputs and can otherwise hide a missing generated source (#778). +// Scope ownership also cleans up a failed or configure-only prepare. +struct ActionPlaceholders { + ActionPlaceholders() = default; + ActionPlaceholders(const ActionPlaceholders&) = delete; + ActionPlaceholders& operator=(const ActionPlaceholders&) = delete; + ~ActionPlaceholders(); + void clear() noexcept; + + std::vector files; +}; + // Resolve an action's paths against `pkgRoot` and make its Source outputs // exist, so the ordinary source scan can see them. // // A placeholder rather than a synthesised CompileUnit, because that reuses // every existing mechanism: the glob finds it, the scanner reads it, the plan -// gives it an object path, and ninja overwrites it with the real content -// before the compile edge runs (the compile depends on the action's output). +// gives it an object path. The owner removes the placeholder after scanning, +// so ninja sees a missing output and runs the generator before compiling it. // // For a module interface the placeholder carries the DECLARED interface — // `export module X;` plus its imports — so the prepare-time scan agrees with @@ -685,7 +699,8 @@ std::string deploy_directive_error(const mcpp::manifest::Manifest& m, const Dire // generator runs before anything reads it either way. void prepare_actions(std::vector& actions, const std::filesystem::path& pkgRoot, - const mcpp::ExtensionTable& extensions); + const mcpp::ExtensionTable& extensions, + ActionPlaceholders& placeholders); // Does this action output belong in the COMPILE set? // @@ -1447,9 +1462,20 @@ bool is_compilable_output(const fs::path& p, const mcpp::ExtensionTable& t) { return kind != mcpp::SourceKind::Header && kind != mcpp::SourceKind::Other; } +ActionPlaceholders::~ActionPlaceholders() { clear(); } + +void ActionPlaceholders::clear() noexcept { + for (const auto& file : files) { + std::error_code ec; + fs::remove(file, ec); + } + files.clear(); +} + void prepare_actions(std::vector& actions, const fs::path& pkgRoot, - const mcpp::ExtensionTable& extensions) { + const mcpp::ExtensionTable& extensions, + ActionPlaceholders& placeholders) { for (auto& a : actions) { auto absolutize = [&](std::vector& v) { for (auto& p : v) { @@ -1494,6 +1520,7 @@ void prepare_actions(std::vector& actions, fs::path p(o); if (fs::exists(p, ec)) continue; // real content already there fs::create_directories(p.parent_path(), ec); + placeholders.files.push_back(p); std::ofstream os(p, std::ios::trunc); if (!os) continue; if (!a.provides.empty()) { diff --git a/modules/buildmcpp/tests/test_action_placeholders.cpp b/modules/buildmcpp/tests/test_action_placeholders.cpp new file mode 100644 index 00000000..07997eae --- /dev/null +++ b/modules/buildmcpp/tests/test_action_placeholders.cpp @@ -0,0 +1,93 @@ +#include + +import std; +import mcpp.build.directives; +import mcpp.manifest; +import mcpp.source_kind; + +namespace dirs = mcpp::build::directives; + +namespace { + +struct ActionPlaceholders : testing::Test { + std::filesystem::path root; + + void SetUp() override { + root = std::filesystem::temp_directory_path() / "mcpp_action_placeholders"; + std::filesystem::remove_all(root); + std::filesystem::create_directories(root); + } + + void TearDown() override { + std::error_code ec; + std::filesystem::remove_all(root, ec); + } + + auto actions(std::vector outputs) { + mcpp::manifest::BuildAction a; + a.id = "generate"; + a.role = mcpp::manifest::BuildAction::Role::Source; + a.outputs = std::move(outputs); + return std::vector{std::move(a)}; + } + + void prepare(std::vector& a, + dirs::ActionPlaceholders& owner) { + dirs::prepare_actions(a, root, mcpp::extension_table_for({}, {}), owner); + } +}; + +} // namespace + +TEST_F(ActionPlaceholders, MissingSourcesExistOnlyDuringScanning) { + auto a = actions({"gen/answer.cpp", "gen/answer.h"}); + { + dirs::ActionPlaceholders owner; + prepare(a, owner); + EXPECT_TRUE(std::filesystem::exists(root / "gen/answer.cpp")); + EXPECT_FALSE(std::filesystem::exists(root / "gen/answer.h")); + // Re-adopting an output must not acquire a second owner or erase it. + prepare(a, owner); + EXPECT_TRUE(std::filesystem::exists(root / "gen/answer.cpp")); + owner.clear(); + EXPECT_FALSE(std::filesystem::exists(root / "gen/answer.cpp")); + } + EXPECT_FALSE(std::filesystem::exists(root / "gen/answer.cpp")); +} + +TEST_F(ActionPlaceholders, ExistingOutputsIncludingEmptySourcesArePreserved) { + { + std::ofstream(root / "empty.cpp"); + std::ofstream(root / "answer.cpp") << "int answer() { return 42; }\n"; + } + const auto emptyTime = std::filesystem::last_write_time(root / "empty.cpp"); + const auto answerTime = std::filesystem::last_write_time(root / "answer.cpp"); + auto a = actions({"empty.cpp", "answer.cpp", "missing.cpp"}); + { + dirs::ActionPlaceholders owner; + prepare(a, owner); + } + EXPECT_TRUE(std::filesystem::exists(root / "empty.cpp")); + EXPECT_EQ(std::filesystem::file_size(root / "empty.cpp"), 0u); + EXPECT_EQ(std::filesystem::last_write_time(root / "empty.cpp"), emptyTime); + EXPECT_EQ(std::filesystem::last_write_time(root / "answer.cpp"), answerTime); + EXPECT_GT(std::filesystem::file_size(root / "answer.cpp"), 0u); + EXPECT_FALSE(std::filesystem::exists(root / "missing.cpp")); +} + +TEST_F(ActionPlaceholders, ModuleDeclarationsAreAvailableUntilScopeExit) { + auto a = actions({"generated.cppm"}); + a[0].provides = {"generated"}; + a[0].imports = {"std"}; + EXPECT_THROW({ + dirs::ActionPlaceholders owner; + prepare(a, owner); + std::ifstream file(root / "generated.cppm"); + const std::string content(std::istreambuf_iterator(file), + std::istreambuf_iterator{}); + EXPECT_NE(content.find("import std;"), std::string::npos); + EXPECT_NE(content.find("export module generated;"), std::string::npos); + throw std::runtime_error("prepare failed after adopting the output"); + }, std::runtime_error); + EXPECT_FALSE(std::filesystem::exists(root / "generated.cppm")); +} diff --git a/src/build/prepare/driver.cpp b/src/build/prepare/driver.cpp index d4871e4a..da3fce69 100644 --- a/src/build/prepare/driver.cpp +++ b/src/build/prepare/driver.cpp @@ -151,6 +151,7 @@ prepare_build_pass(bool print_fingerprint, if (auto r = timed("scan", [&] { return phase11_scan(state); }); !r) return fail(r.error()); + state.actionPlaceholders.clear(); g_notesOnFailure.clear(); return timed("finish", [&] { return phase13_finish(state); }); } diff --git a/src/build/prepare/graph.cpp b/src/build/prepare/graph.cpp index 4df4cb58..417d5705 100644 --- a/src/build/prepare/graph.cpp +++ b/src/build/prepare/graph.cpp @@ -2156,10 +2156,10 @@ step4b_define_provisioning_closures(PrepareState& state) { // A declared build-graph node's Source outputs must be visible to the // scan, so they are materialized as placeholders and joined to the source // set here — the same two lists `generated=` feeds, for the same reason - // (the scanner walks the legacy modules.sources mirror). ninja overwrites - // the placeholder before the compile edge runs, because that compile - // depends on the action's output. - state.adoptActionOutputs = [](mcpp::manifest::Manifest& mm, + // (the scanner walks the legacy modules.sources mirror). Prepare owns + // these temporary files until the scan finishes; ninja must see the + // original missing output rather than a newer placeholder (#778). + state.adoptActionOutputs = [&state](mcpp::manifest::Manifest& mm, const std::filesystem::path& pkgRoot, std::size_t firstNewAction) { if (firstNewAction >= mm.buildConfig.actions.size()) return; @@ -2175,7 +2175,8 @@ step4b_define_provisioning_closures(PrepareState& state) { const auto pkgExtTable = mcpp::extension_table_for(mm.buildConfig.moduleExtensions, mm.buildConfig.deviceExtensions); - mcpp::build::directives::prepare_actions(fresh, pkgRoot, pkgExtTable); + mcpp::build::directives::prepare_actions(fresh, pkgRoot, pkgExtTable, + state.actionPlaceholders); std::copy(fresh.begin(), fresh.end(), mm.buildConfig.actions.begin() + static_cast(firstNewAction)); diff --git a/src/build/prepare/state.cppm b/src/build/prepare/state.cppm index 7e71be3b..2d9ce9d7 100644 --- a/src/build/prepare/state.cppm +++ b/src/build/prepare/state.cppm @@ -619,6 +619,7 @@ struct PrepareState { std::pair, std::vector>, std::string>()> graph_xlings_split; std::function computeUsageRequirements; + mcpp::build::directives::ActionPlaceholders actionPlaceholders; std::function adoptActionOutputs; std::function mcpp.toml <<'EOF' +[workspace] +members = ["app"] +EOF +cat > app/mcpp.toml <<'EOF' +[package] +name = "app" +namespace = "repro" +version = "0.1.0" +[build] +sources = [] +[targets.resource_test] +kind = "bin" +main = "src/main.cpp" +EOF +cat > app/src/main.cpp <<'EOF' +int generated_value(); +int main() { return generated_value() == 42 ? 0 : 1; } +EOF +echo 'int generated_value() { return 42; }' > app/value.cpp.in +cat > app/generate.py <<'EOF' +from pathlib import Path +import sys + +source, output, counter = map(Path, sys.argv[1:]) +output.parent.mkdir(parents=True, exist_ok=True) +output.write_bytes(source.read_bytes()) +count = int(counter.read_text()) if counter.exists() else 0 +counter.write_text(str(count + 1)) +EOF +cat > app/build.mcpp <<'EOF' +import std; +import mcpp; +int main() { + mcpp::rerun_if_env_changed("ACTION_PYTHON"); + const std::string root = mcpp::manifest_dir(); + const std::string out = std::string(mcpp::out_dir()) + "/generated.cpp"; + mcpp::action a; + a.id = "generate:value"; + a.role = mcpp::roles::source; + a.arg(std::getenv("ACTION_PYTHON")) + .arg((root + "/generate.py").c_str()) + .arg((root + "/value.cpp.in").c_str()).arg(out.c_str()) + .arg((root + "/generator-count.txt").c_str()) + .input((root + "/generate.py").c_str()) + .input((root + "/value.cpp.in").c_str()).output(out.c_str()).submit(); +} +EOF + +build() { "$MCPP" build -p app --release > "$1" 2>&1 || fail "the build failed" "$1"; } +count_is() { [ "$(cat app/generator-count.txt)" = "$1" ] || fail "expected $1 generator calls"; } +generated="app/target/.build-mcpp/out/generated.cpp" + +# A real cold build establishes the generated source and the retained log. +build cold.log +cmp app/value.cpp.in "$generated" || fail "the generated source differs from its input" +count_is 1 +log=$(find target -name .ninja_log | head -1) +[ -n "$log" ] && [ -s "$log" ] || fail "the workspace has no ninja build record" + +build warm.log +count_is 1 +touch app/build.mcpp +build reprepare.log +count_is 1 +echo "ok: warm builds and re-prepare preserve the real output" + +# Retain target/.ninja_log while removing only the member's output tree. +mv app/target saved-app-target +build member-clean.log +cmp app/value.cpp.in "$generated" || fail "the cleared source was replaced by a placeholder" +count_is 2 +echo "ok: clearing a member's target reruns its generator" + +# A configure-only pass must not leave a newer fake output behind. +mv app/target saved-app-target-2 +"$MCPP" build -p app --release --configure-only > configure.log 2>&1 || fail "configure-only failed" configure.log +[ ! -e "$generated" ] || fail "configure-only left a scan placeholder behind" +count_is 2 +build after-configure.log +cmp app/value.cpp.in "$generated" || fail "the build after configure-only did not generate its source" +count_is 3 +build final-warm.log +count_is 3 +echo "ok: configure-only leaves the missing output visible to the next build" + +# Generated module interfaces still need their declared provider in the scan, +# even though the temporary interface disappears before the actual build. +cat > app/src/main.cpp <<'EOF' +import generated; +int main() { return generated_value() == 42 ? 0 : 1; } +EOF +printf 'export module generated;\nexport int generated_value() { return 42; }\n' > app/value.cpp.in +python3 <<'PY' +from pathlib import Path +p = Path("app/build.mcpp") +p.write_text(p.read_text().replace('"/generated.cpp"', '"/generated.cppm"') + .replace('a.role = mcpp::roles::source;', + 'a.role = mcpp::roles::source; a.provides("generated");')) +PY +generated="app/target/.build-mcpp/out/generated.cppm" +build module.log +count_is 4 +cmp app/value.cpp.in "$generated" || fail "the module interface was not generated" +mv app/target saved-app-target-3 +build module-clean.log +count_is 5 +cmp app/value.cpp.in "$generated" || fail "the cleared module interface was not regenerated" +echo "ok: generated module interfaces retain their scan declarations and regenerate" +echo "PASS: 890_source_placeholders_do_not_hide_missing_outputs"