Skip to content

Commit fc65564

Browse files
committed
fix(workspace): [workspace.build] applies to a member reached as a dependency
Found by asking the completeness question the feature invites and not by a failure: does `[workspace.build]` reach a member that is compiled as ANOTHER member's `path` dependency? Measured, it did not. mcpp build -p appb appb -DWS_FLAG=1 -std=c++26 liba -std=c++26 <- the sibling Inheritance ran only where the command's own manifest is loaded, so a workspace where members depend on each other — the ordinary shape, not an exotic one — got the flags on one package and not on the others, in the same command. `[workspace.package] standard` hid it. The standard is imposed graph-wide from the root for BMI-compatibility reasons, so it reached the sibling anyway; the gap only became reachable once a `[build]` key was inheritable too. Two halves, applied where each one's consumer reads it: - `[workspace.package]` at the dependency LOAD site, because a member may legally omit `package.version` when the workspace supplies it, and the parser would otherwise refuse it for a field the workspace does provide — naming the member's manifest rather than the table that answers. - `[workspace.build]` in `makePackageRoot`, the one funnel both dependency-assembly sites go through, and the point at which the include directories are captured from the manifest. A later mutation would reach the flags and silently not the include dirs. `is_workspace_member` asks the workspace's own `members` list rather than "is this path inside the tree": a vendored copy or an example living under the workspace is not a member, and a member's flags are exactly what it must not acquire. 321 asserts both directions in one fixture, because a fix that inherited to every `path` dependency would pass the positive alone. `inherit_workspace_config` is now the composition of the two halves, so the three call sites cannot drift.
1 parent 7735387 commit fc65564

3 files changed

Lines changed: 178 additions & 35 deletions

File tree

src/build/prepare.cppm

Lines changed: 62 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4443,8 +4443,38 @@ prepare_build(bool print_fingerprint,
44434443

44444444
auto makePackageRoot =
44454445
[&](const std::filesystem::path& packageRoot,
4446-
const mcpp::manifest::Manifest& manifest)
4446+
const mcpp::manifest::Manifest& manifestIn)
44474447
{
4448+
// `[workspace.build]` APPLIES TO EVERY MEMBER, INCLUDING ONE REACHED AS
4449+
// ANOTHER MEMBER'S `path` DEPENDENCY — WHICH IS THE ORDINARY SHAPE.
4450+
//
4451+
// Inheritance runs where the command's own manifest is loaded, so
4452+
// `mcpp build -p appb` gave `appb` the workspace flags and gave `liba`
4453+
// none, even though `liba` is a member of the same workspace and is
4454+
// being compiled by the same command. Measured before this: `-DWS_FLAG`
4455+
// on the consumer's TUs and not on the sibling's.
4456+
//
4457+
// `[workspace.package] standard` did not have the problem, because the
4458+
// standard is imposed graph-wide from the root for BMI-compatibility
4459+
// reasons — which is exactly why the gap was invisible until a
4460+
// `[build]` flag was inheritable too.
4461+
//
4462+
// Applied HERE because this is the one funnel both dependency-assembly
4463+
// sites go through, and because the include directories a few lines
4464+
// below are captured from the manifest at this moment: a later mutation
4465+
// would reach the flags and silently not the include dirs.
4466+
//
4467+
// Only for MEMBERS. An index or git dependency is not part of the
4468+
// workspace and must not acquire its flags.
4469+
mcpp::manifest::Manifest manifest = manifestIn;
4470+
if (wsManifest && !runtimeWorkspaceRoot.empty()
4471+
&& mcpp::project::is_workspace_member(*wsManifest,
4472+
runtimeWorkspaceRoot,
4473+
packageRoot)) {
4474+
mcpp::project::inherit_workspace_build(manifest, *wsManifest,
4475+
runtimeWorkspaceRoot);
4476+
}
4477+
44484478
mcpp::modgraph::PackageRoot pkg;
44494479
pkg.root = packageRoot;
44504480
pkg.manifest = manifest;
@@ -5442,13 +5472,43 @@ prepare_build(bool print_fingerprint,
54425472
"{} dependency '{}' (at '{}') has no mcpp.toml",
54435473
spec.isGit() ? "git" : "path", name, dep_root.string()));
54445474
}
5445-
auto dm = mcpp::manifest::load(dep_root / "mcpp.toml");
5475+
// A MEMBER IS A MEMBER HOWEVER IT IS REACHED.
5476+
//
5477+
// A workspace member that omits `package.version` because
5478+
// `[workspace.package]` supplies it is legal — and it is reached
5479+
// here as a sibling's `path` dependency, which is the ordinary
5480+
// shape rather than an exotic one. Loading it as an anonymous path
5481+
// dependency would refuse it for a field the workspace does
5482+
// provide, and the message would name the member's manifest rather
5483+
// than the table that answers.
5484+
//
5485+
// `is_workspace_member` asks the workspace's own `members` list, so
5486+
// a vendored copy or an example living inside the tree is still
5487+
// refused for a missing version, exactly as before.
5488+
const bool depIsMember =
5489+
wsManifest && !runtimeWorkspaceRoot.empty()
5490+
&& mcpp::project::is_workspace_member(
5491+
*wsManifest, runtimeWorkspaceRoot, dep_root);
5492+
auto dm = mcpp::manifest::load(
5493+
dep_root / "mcpp.toml",
5494+
{.insideWorkspace = depIsMember});
54465495
if (!dm) {
54475496
return std::unexpected(std::format(
54485497
"dependency '{}' (at '{}'): {}",
54495498
name, dep_root.string(), dm.error().format()));
54505499
}
54515500
dep_manifest = std::move(*dm);
5501+
// The metadata half of the inheritance. The `[build]` half runs in
5502+
// `makePackageRoot`, where the include directories are captured;
5503+
// splitting them is what keeps each one at the point its consumer
5504+
// reads it.
5505+
if (depIsMember) {
5506+
mcpp::project::inherit_workspace_package(
5507+
*dep_manifest, *wsManifest);
5508+
if (auto bad = mcpp::project::workspace_inheritance_error(
5509+
*dep_manifest, dep_root))
5510+
return std::unexpected(*bad);
5511+
}
54525512
// #229: path/git-dep half of the L1 cfg funnel — mirrors the
54535513
// loadVersionDep call site above (loadFrom's L1 cfg merge, ~1740
54545514
// lines up). Before this fix, path/git deps never ran this merge

src/project.cppm

Lines changed: 80 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,59 @@ export void inherit_workspace_indices(mcpp::manifest::Manifest& member,
128128
}
129129
}
130130

131+
// Is `candidate` one of this workspace's declared members?
132+
//
133+
// The membership test is the workspace's OWN `members` list resolved against
134+
// the workspace root — not "is this path under the workspace directory". A
135+
// `path` dependency can live inside the tree without being a member (a vendored
136+
// copy, an example, a scratch package), and a member's flags are exactly what
137+
// it must not acquire.
138+
export bool is_workspace_member(const mcpp::manifest::Manifest& workspace,
139+
const std::filesystem::path& wsRoot,
140+
const std::filesystem::path& candidate) {
141+
if (!workspace.workspace.present) return false;
142+
std::error_code ec;
143+
auto want = std::filesystem::weakly_canonical(candidate, ec);
144+
if (ec) { ec.clear(); want = candidate.lexically_normal(); }
145+
for (auto const& m : workspace.workspace.members) {
146+
auto member = std::filesystem::weakly_canonical(wsRoot / m, ec);
147+
if (ec) { ec.clear(); member = (wsRoot / m).lexically_normal(); }
148+
if (member == want) return true;
149+
}
150+
return false;
151+
}
152+
153+
export void inherit_workspace_build(mcpp::manifest::Manifest& member,
154+
const mcpp::manifest::Manifest& workspace,
155+
const std::filesystem::path& wsRoot);
156+
157+
// The `[workspace.package]` half on its own — metadata, no paths, so no anchor
158+
// argument. Second caller: a member reached as a sibling's `path` dependency,
159+
// which may legally omit `version` because this table supplies it.
160+
export void inherit_workspace_package(mcpp::manifest::Manifest& member,
161+
const mcpp::manifest::Manifest& workspace) {
162+
const auto& inh = workspace.workspace.inherited;
163+
// `standardDeclared` and not `standard != "c++23"`: a member that
164+
// deliberately pins c++23 under a c++26 workspace must keep it, and that is
165+
// indistinguishable from the default without the bit.
166+
if (inh.standardDeclared && !member.package.standardDeclared) {
167+
member.package.standard = inh.standard;
168+
member.language.standard = inh.standard;
169+
member.package.standardDeclared = true;
170+
// `cppStandard` was normalised by the parser from the member's own
171+
// value; it has to be re-derived, or the inherited spelling would sit
172+
// in `package.standard` while every build surface kept reading the
173+
// default out of the normalised copy.
174+
if (auto cfg = mcpp::manifest::normalize_cpp_standard(inh.standard))
175+
member.cppStandard = *cfg;
176+
}
177+
if (member.package.version.empty()) member.package.version = inh.version;
178+
if (member.package.license.empty()) member.package.license = inh.license;
179+
if (member.package.description.empty()) member.package.description = inh.description;
180+
if (member.package.repo.empty()) member.package.repo = inh.repo;
181+
if (member.package.authors.empty()) member.package.authors = inh.authors;
182+
}
183+
131184
// EVERYTHING A MEMBER INHERITS FROM ITS WORKSPACE ROOT, IN ONE FUNCTION.
132185
//
133186
// There are two inheritance SITES in prepare_build — the command issued at the
@@ -158,40 +211,34 @@ export void inherit_workspace_config(mcpp::manifest::Manifest& member,
158211
member.targetOverrides[triple] = entry;
159212
inherit_workspace_indices(member, workspace, wsRoot);
160213

161-
const auto& inh = workspace.workspace.inherited;
162-
163-
// `[workspace.package]`. The standard is the load-bearing one: a C++ module
164-
// graph has ONE standard, so a workspace that states it once is how a
165-
// monorepo stops depending on every member remembering to.
166-
//
167-
// `standardDeclared` and not `standard != "c++23"`: a member that
168-
// deliberately pins c++23 under a c++26 workspace must keep it, and that is
169-
// indistinguishable from the default without the bit.
170-
if (inh.standardDeclared && !member.package.standardDeclared) {
171-
member.package.standard = inh.standard;
172-
member.language.standard = inh.standard;
173-
member.package.standardDeclared = true;
174-
// `cppStandard` was normalised by the parser from the member's own
175-
// value; it has to be re-derived, or the inherited spelling would sit
176-
// in `package.standard` while every build surface kept reading the
177-
// default out of the normalised copy. Same class of defect as a
178-
// recorded field with no reader, one struct over.
179-
if (auto cfg = mcpp::manifest::normalize_cpp_standard(inh.standard))
180-
member.cppStandard = *cfg;
181-
}
182-
if (member.package.version.empty()) member.package.version = inh.version;
183-
// (the "still missing after inheritance" refusal is in
184-
// `workspace_inheritance_error` below — one predicate, both call sites)
185-
if (member.package.license.empty()) member.package.license = inh.license;
186-
if (member.package.description.empty()) member.package.description = inh.description;
187-
if (member.package.repo.empty()) member.package.repo = inh.repo;
188-
if (member.package.authors.empty()) member.package.authors = inh.authors;
214+
// The two halves, each with a second caller of its own: a member reached
215+
// as a sibling.s `path` dependency needs both, at two different points.
216+
// (The "still missing after inheritance" refusal is
217+
// `workspace_inheritance_error`, called by each site.)
218+
inherit_workspace_package(member, workspace);
219+
inherit_workspace_build(member, workspace, wsRoot);
220+
}
189221

190-
// `[workspace.build]`. Vectors append workspace-FIRST so a member's own
191-
// flag lands later on the command line, where the compiler lets it win.
192-
if (inh.buildPresent) {
193-
auto& b = member.buildConfig;
194-
const auto& w = inh.build;
222+
// The `[workspace.build]` half on its own.
223+
//
224+
// SEPARATE BECAUSE IT HAS A SECOND CALLER. `inherit_workspace_config` runs for
225+
// the manifest the command names; this runs additionally for every OTHER member
226+
// pulled in as a `path` dependency — which is what workspace members are to each
227+
// other, and therefore the ordinary case rather than an exotic one. Without the
228+
// second call, `mcpp build -p appb` gave `appb` the workspace flags and gave the
229+
// sibling `liba` none, while compiling both in the same command.
230+
//
231+
// `[workspace.package] standard` needs no second call: the standard is imposed
232+
// graph-wide from the root for BMI-compatibility reasons, which is precisely
233+
// why this gap stayed invisible until a `[build]` key became inheritable too.
234+
export void inherit_workspace_build(mcpp::manifest::Manifest& member,
235+
const mcpp::manifest::Manifest& workspace,
236+
const std::filesystem::path& wsRoot) {
237+
const auto& inh = workspace.workspace.inherited;
238+
if (!inh.buildPresent) return;
239+
auto& b = member.buildConfig;
240+
const auto& w = inh.build;
241+
{
195242
auto prepend = [](auto& dst, const auto& src) {
196243
if (src.empty()) return;
197244
dst.insert(dst.begin(), src.begin(), src.end());

tests/e2e/321_workspace_inheritance.sh

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,4 +149,40 @@ grep -q "package.version" novers.log || {
149149
exit 1
150150
}
151151

152+
# ── a SIBLING member reached as a `path` dependency inherits too ────────────
153+
#
154+
# THE ORDINARY WORKSPACE SHAPE, AND THE ONE THAT WAS MISSED. Inheritance runs
155+
# where the command's own manifest is loaded, so `mcpp build -p consumer` gave
156+
# the consumer the workspace flags and gave the sibling none — while compiling
157+
# both in the same command. `[workspace.package] standard` hid the gap, because
158+
# the standard is imposed graph-wide from the root for BMI compatibility and
159+
# reached the sibling anyway.
160+
#
161+
# The negative is in the same fixture on purpose: a `path` dependency that is
162+
# NOT a member (a vendored copy, an example) must not acquire the flags, and a
163+
# fix that inherited to every path dependency would pass the positive alone.
164+
cat > mcpp.toml <<'EOF'
165+
[workspace]
166+
members = ["silent", "pinned", "adds", "consumer"]
167+
168+
[workspace.package]
169+
standard = 26
170+
version = "0.4.2"
171+
172+
[workspace.build]
173+
cxxflags = ["-DFROM_WORKSPACE=1"]
174+
EOF
175+
mkdir -p consumer/src vendored/src
176+
printf '[package]\nname = "consumer"\n\n[dependencies]\nadds = { path = "../adds" }\nvend = { path = "../vendored" }\n' > consumer/mcpp.toml
177+
printf 'import addslib;\nimport vend;\nint main(){ return addslib()+vend_v()==3 ? 0 : 1; }\n' > consumer/src/main.cpp
178+
# `adds` becomes a library so it can be imported; its own source asserts it saw
179+
# the workspace flag while being built as somebody else's dependency.
180+
printf '[package]\nname = "adds"\n\n[targets.adds]\nkind = "lib"\n\n[build]\ncxxflags = ["-DFROM_MEMBER=1"]\n' > adds/mcpp.toml
181+
rm -f adds/src/main.cpp
182+
printf '#ifndef FROM_WORKSPACE\n#error "a SIBLING member built as a path dependency did not inherit"\n#endif\nexport module addslib;\nexport int addslib(){ return 1; }\n' > adds/src/addslib.cppm
183+
printf '[package]\nname = "vend"\nversion = "0.1.0"\n\n[targets.vend]\nkind = "lib"\n' > vendored/mcpp.toml
184+
printf '#ifdef FROM_WORKSPACE\n#error "a NON-member path dependency must not acquire workspace flags"\n#endif\nexport module vend;\nexport int vend_v(){ return 2; }\n' > vendored/src/vend.cppm
185+
186+
"$MCPP" build -p consumer > sibling.log 2>&1 || { cat sibling.log; exit 1; }
187+
152188
echo "PASS: 321_workspace_inheritance"

0 commit comments

Comments
 (0)