Skip to content

Commit ee52e7e

Browse files
committed
fix: limit generated source placeholders to the prepare-time scan
1 parent de9c290 commit ee52e7e

6 files changed

Lines changed: 258 additions & 9 deletions

File tree

‎modules/buildmcpp/src/directives.cppm‎

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -657,13 +657,27 @@ std::string target_directive_error(const mcpp::manifest::Manifest& m, const Dire
657657
// both the directive and the declaring package, like `target_directive_error`.
658658
std::string deploy_directive_error(const mcpp::manifest::Manifest& m, const Directives& d);
659659

660+
// Own only the files created for the prepare-time source scan. They must be
661+
// gone before ninja checks the outputs: a new placeholder is newer than an
662+
// action's inputs and can otherwise hide a missing generated source (#778).
663+
// Scope ownership also cleans up a failed or configure-only prepare.
664+
struct ActionPlaceholders {
665+
ActionPlaceholders() = default;
666+
ActionPlaceholders(const ActionPlaceholders&) = delete;
667+
ActionPlaceholders& operator=(const ActionPlaceholders&) = delete;
668+
~ActionPlaceholders();
669+
void clear() noexcept;
670+
671+
std::vector<std::filesystem::path> files;
672+
};
673+
660674
// Resolve an action's paths against `pkgRoot` and make its Source outputs
661675
// exist, so the ordinary source scan can see them.
662676
//
663677
// A placeholder rather than a synthesised CompileUnit, because that reuses
664678
// every existing mechanism: the glob finds it, the scanner reads it, the plan
665-
// gives it an object path, and ninja overwrites it with the real content
666-
// before the compile edge runs (the compile depends on the action's output).
679+
// gives it an object path. The owner removes the placeholder after scanning,
680+
// so ninja sees a missing output and runs the generator before compiling it.
667681
//
668682
// For a module interface the placeholder carries the DECLARED interface —
669683
// `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
685699
// generator runs before anything reads it either way.
686700
void prepare_actions(std::vector<mcpp::manifest::BuildAction>& actions,
687701
const std::filesystem::path& pkgRoot,
688-
const mcpp::ExtensionTable& extensions);
702+
const mcpp::ExtensionTable& extensions,
703+
ActionPlaceholders& placeholders);
689704

690705
// Does this action output belong in the COMPILE set?
691706
//
@@ -1447,9 +1462,20 @@ bool is_compilable_output(const fs::path& p, const mcpp::ExtensionTable& t) {
14471462
return kind != mcpp::SourceKind::Header && kind != mcpp::SourceKind::Other;
14481463
}
14491464

1465+
ActionPlaceholders::~ActionPlaceholders() { clear(); }
1466+
1467+
void ActionPlaceholders::clear() noexcept {
1468+
for (const auto& file : files) {
1469+
std::error_code ec;
1470+
fs::remove(file, ec);
1471+
}
1472+
files.clear();
1473+
}
1474+
14501475
void prepare_actions(std::vector<mcpp::manifest::BuildAction>& actions,
14511476
const fs::path& pkgRoot,
1452-
const mcpp::ExtensionTable& extensions) {
1477+
const mcpp::ExtensionTable& extensions,
1478+
ActionPlaceholders& placeholders) {
14531479
for (auto& a : actions) {
14541480
auto absolutize = [&](std::vector<std::string>& v) {
14551481
for (auto& p : v) {
@@ -1494,6 +1520,7 @@ void prepare_actions(std::vector<mcpp::manifest::BuildAction>& actions,
14941520
fs::path p(o);
14951521
if (fs::exists(p, ec)) continue; // real content already there
14961522
fs::create_directories(p.parent_path(), ec);
1523+
placeholders.files.push_back(p);
14971524
std::ofstream os(p, std::ios::trunc);
14981525
if (!os) continue;
14991526
if (!a.provides.empty()) {
Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
#include <gtest/gtest.h>
2+
3+
import std;
4+
import mcpp.build.directives;
5+
import mcpp.manifest;
6+
import mcpp.source_kind;
7+
8+
namespace dirs = mcpp::build::directives;
9+
10+
namespace {
11+
12+
struct ActionPlaceholders : testing::Test {
13+
std::filesystem::path root;
14+
15+
void SetUp() override {
16+
root = std::filesystem::temp_directory_path() / "mcpp_action_placeholders";
17+
std::filesystem::remove_all(root);
18+
std::filesystem::create_directories(root);
19+
}
20+
21+
void TearDown() override {
22+
std::error_code ec;
23+
std::filesystem::remove_all(root, ec);
24+
}
25+
26+
auto actions(std::vector<std::string> outputs) {
27+
mcpp::manifest::BuildAction a;
28+
a.id = "generate";
29+
a.role = mcpp::manifest::BuildAction::Role::Source;
30+
a.outputs = std::move(outputs);
31+
return std::vector{std::move(a)};
32+
}
33+
34+
void prepare(std::vector<mcpp::manifest::BuildAction>& a,
35+
dirs::ActionPlaceholders& owner) {
36+
dirs::prepare_actions(a, root, mcpp::extension_table_for({}, {}), owner);
37+
}
38+
};
39+
40+
} // namespace
41+
42+
TEST_F(ActionPlaceholders, MissingSourcesExistOnlyDuringScanning) {
43+
auto a = actions({"gen/answer.cpp", "gen/answer.h"});
44+
{
45+
dirs::ActionPlaceholders owner;
46+
prepare(a, owner);
47+
EXPECT_TRUE(std::filesystem::exists(root / "gen/answer.cpp"));
48+
EXPECT_FALSE(std::filesystem::exists(root / "gen/answer.h"));
49+
// Re-adopting an output must not acquire a second owner or erase it.
50+
prepare(a, owner);
51+
EXPECT_TRUE(std::filesystem::exists(root / "gen/answer.cpp"));
52+
owner.clear();
53+
EXPECT_FALSE(std::filesystem::exists(root / "gen/answer.cpp"));
54+
}
55+
EXPECT_FALSE(std::filesystem::exists(root / "gen/answer.cpp"));
56+
}
57+
58+
TEST_F(ActionPlaceholders, ExistingOutputsIncludingEmptySourcesArePreserved) {
59+
{
60+
std::ofstream(root / "empty.cpp");
61+
std::ofstream(root / "answer.cpp") << "int answer() { return 42; }\n";
62+
}
63+
const auto emptyTime = std::filesystem::last_write_time(root / "empty.cpp");
64+
const auto answerTime = std::filesystem::last_write_time(root / "answer.cpp");
65+
auto a = actions({"empty.cpp", "answer.cpp", "missing.cpp"});
66+
{
67+
dirs::ActionPlaceholders owner;
68+
prepare(a, owner);
69+
}
70+
EXPECT_TRUE(std::filesystem::exists(root / "empty.cpp"));
71+
EXPECT_EQ(std::filesystem::file_size(root / "empty.cpp"), 0u);
72+
EXPECT_EQ(std::filesystem::last_write_time(root / "empty.cpp"), emptyTime);
73+
EXPECT_EQ(std::filesystem::last_write_time(root / "answer.cpp"), answerTime);
74+
EXPECT_GT(std::filesystem::file_size(root / "answer.cpp"), 0u);
75+
EXPECT_FALSE(std::filesystem::exists(root / "missing.cpp"));
76+
}
77+
78+
TEST_F(ActionPlaceholders, ModuleDeclarationsAreAvailableUntilScopeExit) {
79+
auto a = actions({"generated.cppm"});
80+
a[0].provides = {"generated"};
81+
a[0].imports = {"std"};
82+
EXPECT_THROW({
83+
dirs::ActionPlaceholders owner;
84+
prepare(a, owner);
85+
std::ifstream file(root / "generated.cppm");
86+
const std::string content(std::istreambuf_iterator<char>(file),
87+
std::istreambuf_iterator<char>{});
88+
EXPECT_NE(content.find("import std;"), std::string::npos);
89+
EXPECT_NE(content.find("export module generated;"), std::string::npos);
90+
throw std::runtime_error("prepare failed after adopting the output");
91+
}, std::runtime_error);
92+
EXPECT_FALSE(std::filesystem::exists(root / "generated.cppm"));
93+
}

‎src/build/prepare/driver.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ prepare_build_pass(bool print_fingerprint,
151151
if (auto r = timed("scan", [&] { return phase11_scan(state); }); !r)
152152
return fail(r.error());
153153

154+
state.actionPlaceholders.clear();
154155
g_notesOnFailure.clear();
155156
return timed("finish", [&] { return phase13_finish(state); });
156157
}

‎src/build/prepare/graph.cpp‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2156,10 +2156,10 @@ step4b_define_provisioning_closures(PrepareState& state) {
21562156
// A declared build-graph node's Source outputs must be visible to the
21572157
// scan, so they are materialized as placeholders and joined to the source
21582158
// set here — the same two lists `generated=` feeds, for the same reason
2159-
// (the scanner walks the legacy modules.sources mirror). ninja overwrites
2160-
// the placeholder before the compile edge runs, because that compile
2161-
// depends on the action's output.
2162-
state.adoptActionOutputs = [](mcpp::manifest::Manifest& mm,
2159+
// (the scanner walks the legacy modules.sources mirror). Prepare owns
2160+
// these temporary files until the scan finishes; ninja must see the
2161+
// original missing output rather than a newer placeholder (#778).
2162+
state.adoptActionOutputs = [&state](mcpp::manifest::Manifest& mm,
21632163
const std::filesystem::path& pkgRoot,
21642164
std::size_t firstNewAction) {
21652165
if (firstNewAction >= mm.buildConfig.actions.size()) return;
@@ -2175,7 +2175,8 @@ step4b_define_provisioning_closures(PrepareState& state) {
21752175
const auto pkgExtTable =
21762176
mcpp::extension_table_for(mm.buildConfig.moduleExtensions,
21772177
mm.buildConfig.deviceExtensions);
2178-
mcpp::build::directives::prepare_actions(fresh, pkgRoot, pkgExtTable);
2178+
mcpp::build::directives::prepare_actions(fresh, pkgRoot, pkgExtTable,
2179+
state.actionPlaceholders);
21792180
std::copy(fresh.begin(), fresh.end(),
21802181
mm.buildConfig.actions.begin()
21812182
+ static_cast<std::ptrdiff_t>(firstNewAction));

‎src/build/prepare/state.cppm‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -619,6 +619,7 @@ struct PrepareState {
619619
std::pair<std::vector<std::string>, std::vector<std::string>>,
620620
std::string>()> graph_xlings_split;
621621
std::function<void()> computeUsageRequirements;
622+
mcpp::build::directives::ActionPlaceholders actionPlaceholders;
622623
std::function<void(mcpp::manifest::Manifest&, const std::filesystem::path&,
623624
std::size_t)> adoptActionOutputs;
624625
std::function<void(mcpp::build::BuildProgramEnv&, const mcpp::manifest::Manifest&,
Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
#!/usr/bin/env bash
2+
# requires: python3
3+
# #778: keep the workspace's ninja log, clear only a member's generated
4+
# outputs, and verify the generator runs again rather than compiling an empty
5+
# scan placeholder. Configure-only must leave no placeholder for a later run;
6+
# ordinary warm builds, including a re-prepare, must not rerun the generator.
7+
set -e
8+
9+
HERE=$(cd "$(dirname "$0")" && pwd)
10+
TMP=$(mktemp -d)
11+
trap 'rm -rf "$TMP"' EXIT
12+
fail() { echo "FAIL: $1"; [ -z "${2:-}" ] || cat "$2"; exit 1; }
13+
MCPP="${MCPP:-mcpp}"
14+
export ACTION_PYTHON=$(python3 -c 'import sys; print(sys.executable.replace(chr(92), "/"))')
15+
export MCPP_HOME="$TMP/mcpp-home"
16+
source "$HERE/_inherit_toolchain.sh"
17+
18+
mkdir -p "$TMP/ws/app/src"
19+
cd "$TMP/ws"
20+
cat > mcpp.toml <<'EOF'
21+
[workspace]
22+
members = ["app"]
23+
EOF
24+
cat > app/mcpp.toml <<'EOF'
25+
[package]
26+
name = "app"
27+
namespace = "repro"
28+
version = "0.1.0"
29+
[build]
30+
sources = []
31+
[targets.resource_test]
32+
kind = "bin"
33+
main = "src/main.cpp"
34+
EOF
35+
cat > app/src/main.cpp <<'EOF'
36+
int generated_value();
37+
int main() { return generated_value() == 42 ? 0 : 1; }
38+
EOF
39+
echo 'int generated_value() { return 42; }' > app/value.cpp.in
40+
cat > app/generate.py <<'EOF'
41+
from pathlib import Path
42+
import sys
43+
44+
source, output, counter = map(Path, sys.argv[1:])
45+
output.parent.mkdir(parents=True, exist_ok=True)
46+
output.write_bytes(source.read_bytes())
47+
count = int(counter.read_text()) if counter.exists() else 0
48+
counter.write_text(str(count + 1))
49+
EOF
50+
cat > app/build.mcpp <<'EOF'
51+
import std;
52+
import mcpp;
53+
int main() {
54+
mcpp::rerun_if_env_changed("ACTION_PYTHON");
55+
const std::string root = mcpp::manifest_dir();
56+
const std::string out = std::string(mcpp::out_dir()) + "/generated.cpp";
57+
mcpp::action a;
58+
a.id = "generate:value";
59+
a.role = mcpp::roles::source;
60+
a.arg(std::getenv("ACTION_PYTHON"))
61+
.arg((root + "/generate.py").c_str())
62+
.arg((root + "/value.cpp.in").c_str()).arg(out.c_str())
63+
.arg((root + "/generator-count.txt").c_str())
64+
.input((root + "/generate.py").c_str())
65+
.input((root + "/value.cpp.in").c_str()).output(out.c_str()).submit();
66+
}
67+
EOF
68+
69+
build() { "$MCPP" build -p app --release > "$1" 2>&1 || fail "the build failed" "$1"; }
70+
count_is() { [ "$(cat app/generator-count.txt)" = "$1" ] || fail "expected $1 generator calls"; }
71+
generated="app/target/.build-mcpp/out/generated.cpp"
72+
73+
# A real cold build establishes the generated source and the retained log.
74+
build cold.log
75+
cmp app/value.cpp.in "$generated" || fail "the generated source differs from its input"
76+
count_is 1
77+
log=$(find target -name .ninja_log | head -1)
78+
[ -n "$log" ] && [ -s "$log" ] || fail "the workspace has no ninja build record"
79+
80+
build warm.log
81+
count_is 1
82+
touch app/build.mcpp
83+
build reprepare.log
84+
count_is 1
85+
echo "ok: warm builds and re-prepare preserve the real output"
86+
87+
# Retain target/.ninja_log while removing only the member's output tree.
88+
mv app/target saved-app-target
89+
build member-clean.log
90+
cmp app/value.cpp.in "$generated" || fail "the cleared source was replaced by a placeholder"
91+
count_is 2
92+
echo "ok: clearing a member's target reruns its generator"
93+
94+
# A configure-only pass must not leave a newer fake output behind.
95+
mv app/target saved-app-target-2
96+
"$MCPP" build -p app --release --configure-only > configure.log 2>&1 || fail "configure-only failed" configure.log
97+
[ ! -e "$generated" ] || fail "configure-only left a scan placeholder behind"
98+
count_is 2
99+
build after-configure.log
100+
cmp app/value.cpp.in "$generated" || fail "the build after configure-only did not generate its source"
101+
count_is 3
102+
build final-warm.log
103+
count_is 3
104+
echo "ok: configure-only leaves the missing output visible to the next build"
105+
106+
# Generated module interfaces still need their declared provider in the scan,
107+
# even though the temporary interface disappears before the actual build.
108+
echo 'import generated; int main() { return generated_value() == 42 ? 0 : 1; }' > app/src/main.cpp
109+
printf 'export module generated;\nexport int generated_value() { return 42; }\n' > app/value.cpp.in
110+
python3 <<'PY'
111+
from pathlib import Path
112+
p = Path("app/build.mcpp")
113+
p.write_text(p.read_text().replace('"/generated.cpp"', '"/generated.cppm"')
114+
.replace('a.role = mcpp::roles::source;',
115+
'a.role = mcpp::roles::source; a.provides("generated");'))
116+
PY
117+
generated="app/target/.build-mcpp/out/generated.cppm"
118+
build module.log
119+
count_is 4
120+
cmp app/value.cpp.in "$generated" || fail "the module interface was not generated"
121+
mv app/target saved-app-target-3
122+
build module-clean.log
123+
count_is 5
124+
cmp app/value.cpp.in "$generated" || fail "the cleared module interface was not regenerated"
125+
echo "ok: generated module interfaces retain their scan declarations and regenerate"
126+
echo "PASS: 890_source_placeholders_do_not_hide_missing_outputs"

0 commit comments

Comments
 (0)