Skip to content

Commit bbd2eeb

Browse files
committed
fix(build): refuse to cache an index package whose upstream is local
A key covers an upstream package by folding in that package's KEY, and a local package's key covers its file list but not its file contents — nothing could, without hashing a tree that may change between the hash and the compile. So a cached downstream entry would keep looking valid after a local upstream's source was edited. No index descriptor can declare a path dependency today, which makes the shape unreachable in practice. Enforced structurally anyway: "unreachable today" is exactly how the transitive path-dep leak got in. Also asserts the invariant that cache-served units keep their compile_commands.json entries — they stay in the plan on purpose so clangd does not lose the dependency's sources.
1 parent 5daaacb commit bbd2eeb

2 files changed

Lines changed: 46 additions & 0 deletions

File tree

src/build/prepare.cppm

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3902,6 +3902,19 @@ prepare_build(bool print_fingerprint,
39023902
nlohmann::json::object());
39033903
std::vector<int> keyState(packages.size(), 0); // 0 new/1 busy/2 done
39043904
std::string keyCycleError;
3905+
// Does this package's own transitive upstream contain anything that is
3906+
// not an immutable index payload? If so it cannot be cached either, even
3907+
// when the package itself is an index package.
3908+
//
3909+
// A key covers an upstream package by folding in that package's KEY, and
3910+
// a local package's key covers its file list but not its file CONTENTS —
3911+
// nothing could, without hashing a tree that may change between the hash
3912+
// and the compile. So editing a local upstream's source would leave a
3913+
// downstream entry looking valid. No index descriptor can declare a path
3914+
// dependency today, which makes this shape unreachable in practice; it is
3915+
// enforced structurally anyway, because "unreachable today" is how the
3916+
// transitive path-dep leak got in.
3917+
std::vector<char> localTaint(packages.size(), 0);
39053918
auto compute_key = [&](auto&& self, std::size_t idx) -> const std::string& {
39063919
static const std::string kEmpty;
39073920
if (keyState[idx] == 2) return pkgKeys[idx];
@@ -3933,10 +3946,15 @@ prepare_build(bool print_fingerprint,
39333946
if (pa.version.empty()) pa.version = packages[idx].manifest.package.version;
39343947
ck::fill_package_config(pa, packages[idx], storeRoot);
39353948
pa.sources = pkgSources[idx];
3949+
const bool selfIsIndex = idx > 0
3950+
&& idx - 1 < dep_cache_identities.size()
3951+
&& dep_cache_identities[idx - 1].sourceKind == "version";
3952+
if (!selfIsIndex) localTaint[idx] = 1;
39363953
for (auto& e : dependencyEdges) {
39373954
if (e.consumerPackageIndex != idx) continue;
39383955
auto& up = self(self, e.dependencyPackageIndex);
39393956
if (!up.empty()) pa.upstreamKeys.push_back(up);
3957+
if (localTaint[e.dependencyPackageIndex]) localTaint[idx] = 1;
39403958
for (auto& f : e.requestedFeatures) pa.features.push_back(f);
39413959
}
39423960
std::ranges::sort(pa.upstreamKeys);
@@ -3979,6 +3997,8 @@ prepare_build(bool print_fingerprint,
39793997
// xpkgs store stays out, because the failure mode here is a
39803998
// silently wrong object rather than a rejected command.
39813999
if (!depIdent || depIdent->sourceKind != "version") continue;
4000+
// ...and neither may anything it was built against be local.
4001+
if (localTaint[i]) continue;
39824002

39834003
const auto& depName = depIdent->packageName;
39844004
const auto& depVer = depIdent->version.empty()

tests/unit/test_ninja_backend.cpp

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1040,3 +1040,29 @@ TEST(NinjaBackend, CachedUnitWithoutCachedObjectPathIsStillCompiled) {
10401040
EXPECT_EQ(ninja.find("stage_file \n"), std::string::npos) << ninja;
10411041
EXPECT_EQ(ninja.find("build obj/main.o : stage_file"), std::string::npos) << ninja;
10421042
}
1043+
1044+
// A cache-served unit keeps its compile_commands.json entry. The units stay in
1045+
// the plan (only their EDGE changes shape) precisely so clangd does not lose the
1046+
// dependency's sources — a cache that silently degraded IDE navigation would be
1047+
// a bad trade for build time.
1048+
TEST(NinjaBackend, CachedUnitsStillAppearInCompileCommands) {
1049+
auto plan = minimal_plan();
1050+
plan.compileUnits.push_back({
1051+
.source = "/store/dep/src/dep.c",
1052+
.object = "obj/dep.o",
1053+
.packageName = "dep",
1054+
.servedFromCache = true,
1055+
.cachedObject = "/bc/v1/pkg/idx/dep@1.0.0/key/obj/dep.o",
1056+
});
1057+
plan.compileUnits.push_back({
1058+
.source = "src/main.cpp",
1059+
.object = "obj/main.o",
1060+
.packageName = "objc_rule_test",
1061+
});
1062+
1063+
auto flags = compute_flags(plan);
1064+
auto cdb = emit_compile_commands(plan, flags);
1065+
1066+
EXPECT_NE(cdb.find("/store/dep/src/dep.c"), std::string::npos) << cdb;
1067+
EXPECT_NE(cdb.find("src/main.cpp"), std::string::npos) << cdb;
1068+
}

0 commit comments

Comments
 (0)