Skip to content

Commit 0d871b3

Browse files
committed
review: verify the compatibility claim, and stop guessing at the archiver
Four things a deeper read of this PR turned up. **The old-client claim was unverified.** The PR body and docs/12 both say an mcpp that predates this feature still builds against these packages — that is the entire justification for adding no manifest section and no key, and nothing checked it. It is true (2026.8.15.3 consumes a package from 2026.8.17.2 and runs it), and e2e 252 now pins it in two halves: a portable static check that the generated manifest's sections are a subset of the pre-existing vocabulary, and the real thing against `$MCPP_BOOT`, the released binary each CI job bootstraps from. The three e2e workflows now export it, so the real half runs rather than noting itself out. **The packer spoke `ar` to whatever archiver it was handed.** `archive_tool` returns LIB.EXE for MSVC, which spells member removal `/REMOVE:<member>` — one flag per member, archive last — not `d <archive> <member>…`. On mcpp's Windows CI the archiver is clang's llvm-ar, which takes the GNU form, so the difference was invisible. How to speak to a tool is what mcpp.toolchain.dialect is for, so the removal spelling lives there now, next to `archiveCmd`, and the packer substitutes rather than assumes. The MSVC row is marked untested, and a failure reports the command it ran. **A duplicate partition provider now says so.** Two files declaring `module m:p;` used to be accepted silently; the scanner names both files. Those programs were always ill-formed, but it is a new failure path and belongs in the changelog with the other behaviour change (`sources = []`). **The module graph is moved into BuildContext, not copied.** Nothing reads `scan.graph` after that point.
1 parent e3e0276 commit 0d871b3

9 files changed

Lines changed: 201 additions & 7 deletions

File tree

.github/workflows/ci-linux-e2e.yml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,11 @@ jobs:
6363
# Point the e2e runner at the freshly-built binary, not the
6464
# bootstrap one. Tests cd into mktemp -d, so $MCPP must be
6565
# absolute or the relative path breaks under the temp cwd.
66+
# The RELEASED mcpp this job bootstrapped from, kept for e2e 252: the
67+
# claim that an older client can still build against a package the PR
68+
# produces is only worth making if something checks it against a real
69+
# old binary. Captured before $MCPP is repointed at the fresh build.
70+
export MCPP_BOOT="$MCPP"
6671
MCPP=$(realpath "$(find target -type f -name mcpp -printf '%T@ %p\n' | sort -rn | head -1 | cut -d' ' -f2)")
6772
test -x "$MCPP"
6873
export MCPP

.github/workflows/ci-macos-e2e.yml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,11 @@ jobs:
4141
# Per-test 600s timeout lives in run_all.sh.
4242
timeout-minutes: 25
4343
run: |
44+
# The RELEASED mcpp this job bootstrapped from, kept for e2e 252: the
45+
# claim that an older client can still build against a package the PR
46+
# produces is only worth making if something checks it against a real
47+
# old binary. Captured before $MCPP is repointed at the fresh build.
48+
export MCPP_BOOT="$MCPP"
4449
MCPP=$(find target -path "*/bin/mcpp" | head -1)
4550
MCPP=$(cd "$(dirname "$MCPP")" && pwd)/$(basename "$MCPP")
4651
test -x "$MCPP"

.github/workflows/ci-windows-e2e.yml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,11 @@ jobs:
6363
# Per-test 600s timeout lives in run_all.sh.
6464
timeout-minutes: 25
6565
run: |
66+
# The RELEASED mcpp this job bootstrapped from, kept for e2e 252: the
67+
# claim that an older client can still build against a package the PR
68+
# produces is only worth making if something checks it against a real
69+
# old binary. Captured before $MCPP is repointed at the fresh build.
70+
export MCPP_BOOT="${MCPP:-$MCPP_BOOT}"
6671
export MCPP="$MCPP_SELF"
6772
export MCPP_VENDORED_XLINGS="$XLINGS_BIN"
6873
export MCPP_E2E_TOOLCHAIN_MIRROR=GLOBAL

CHANGELOG.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,12 @@
6868
用到它才暴露出来。现在扫描器记 `provides = M:part` 并标 `providesInterface
6969
= false`;`import :part;` 按 TU 自己所属的模块名解析。
7070

71+
**⚠️ 两处行为变化**:①两个文件声明同一个分区(`module m:p;` × 2)现在会被
72+
**拒绝并点名两个文件**,此前是静默接受 —— 那种程序本来就 ill-formed,
73+
但它是一条新的失败路径;②`sources = []` 从「等于不写」变成「什么都不编」,
74+
一个真写了 `sources = []` 又依赖默认 glob 的工程会发现产物变空(此前无法表达
75+
「什么都不编」,所以这种写法只可能是误解)。
76+
7177
- **`[target.'<三元组>'.build]` 在没有 `--target` 时从不命中。**
7278

7379
同一个语句的两种拼写互相矛盾:`cfg(linux)` 在原生构建上命中,

src/build/prepare.cppm

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5225,7 +5225,7 @@ prepare_build(bool print_fingerprint,
52255225
// away — a CompileUnit records what to compile, not what it provides — so
52265226
// the packer would otherwise have to scan the tree a second time and could
52275227
// then disagree with the build about what the package even contains.
5228-
ctx.graph = scan.graph;
5228+
ctx.graph = std::move(scan.graph);
52295229
// mcpp#407. Both callers that produce a non-plain graph arrive here the
52305230
// same way: dev-dependencies enabled, synthetic test targets appended. The
52315231
// resulting `default` line names the test binaries and omits the package's

src/pack/library.cppm

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,12 @@ struct LibraryLeg {
5151
std::string abiTag;
5252
std::string buildKey;
5353
std::string linkName; // the -l argument, e.g. "mathkit"
54+
// How THIS leg's archiver spells "delete a member", from
55+
// mcpp.toolchain.dialect. `ar` and `llvm-ar` take `d <archive> <member>…`;
56+
// LIB.EXE takes `/REMOVE:<member>` per member with the archive last. The
57+
// packer must not pick one — see the dialect's note.
58+
std::string removeArg; // "d" | "/REMOVE:{}"
59+
bool removeArchiveFirst = true;
5460
// The SONAME the artifact declares, when it declares one. A shared library
5561
// is FOUND at run time by this name and LINKED by `lib<linkName>.so`, and
5662
// those are two different filenames — so a package that ships only the
@@ -225,15 +231,38 @@ run_library_pack(const LibraryPackPlan& plan)
225231
leg.triple, name) });
226232
}
227233
if (!leg.shared && !plan.dropObjects.empty()) {
228-
std::string cmd = mcpp::platform::shell::quote(leg.archiveTool.string())
229-
+ " d " + mcpp::platform::shell::quote(dst.string());
230-
for (auto const& m : plan.dropObjects)
231-
cmd += " " + mcpp::platform::shell::quote(m);
234+
std::string cmd = mcpp::platform::shell::quote(leg.archiveTool.string());
235+
auto member_words = [&] {
236+
std::string w;
237+
for (auto const& m : plan.dropObjects) {
238+
// `{}` means one flag per member (LIB.EXE); its absence
239+
// means one verb followed by every member (ar).
240+
auto pos = leg.removeArg.find("{}");
241+
if (pos == std::string::npos) { w += " " + mcpp::platform::shell::quote(m); continue; }
242+
auto arg = leg.removeArg;
243+
arg.replace(pos, 2, m);
244+
w += " " + mcpp::platform::shell::quote(arg);
245+
}
246+
return w;
247+
};
248+
if (leg.removeArchiveFirst) {
249+
if (leg.removeArg.find("{}") == std::string::npos)
250+
cmd += " " + leg.removeArg;
251+
cmd += " " + mcpp::platform::shell::quote(dst.string());
252+
cmd += member_words();
253+
} else {
254+
cmd += member_words();
255+
cmd += " " + mcpp::platform::shell::quote(dst.string());
256+
}
232257
auto r = mcpp::platform::process::capture(cmd + " 2>&1");
233258
if (r.exit_code != 0) {
234259
return std::unexpected(LibraryPackError{ std::format(
235-
"cannot drop published interface objects from '{}' (rc={}): {}",
236-
dst.string(), r.exit_code, r.output) });
260+
"cannot drop published interface objects from '{}' (rc={}).\n"
261+
" command: {}\n"
262+
" output : {}\n"
263+
" Leaving them in would give the consumer two definitions of "
264+
"each published module's initialiser.",
265+
dst.string(), r.exit_code, cmd, r.output) });
237266
}
238267
}
239268

src/pack/library_pipeline.cppm

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import mcpp.pack;
3131
import mcpp.pack.abi_tag;
3232
import mcpp.pack.interface;
3333
import mcpp.pack.library;
34+
import mcpp.toolchain.dialect;
3435
import mcpp.toolchain.registry;
3536
import mcpp.toolchain.triple;
3637
import mcpp.ui;
@@ -284,6 +285,10 @@ export int build_and_pack_library(const std::string& targetName,
284285
.abiTag = tag.str(),
285286
.buildKey = ctx->fp.hex,
286287
.linkName = targetName,
288+
.removeArg = std::string(
289+
mcpp::toolchain::dialect_for(ctx->tc).archiveRemoveArg),
290+
.removeArchiveFirst =
291+
mcpp::toolchain::dialect_for(ctx->tc).archiveRemoveTakesArchiveFirst,
287292
.soname = target->soname,
288293
.shared = shared,
289294
});

src/toolchain/dialect.cppm

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,23 @@ struct CommandDialect {
8989

9090
// Full ninja command template for static archives.
9191
std::string_view archiveCmd; // "$ar rcs $out $in" | "$ar /nologo /OUT:$out $in"
92+
93+
// How this archiver DELETES a member, as argv words before the member
94+
// names. `mcpp pack` needs it: a library package publishes its interface
95+
// units as source, so their objects have to come OUT of the archive, or the
96+
// consumer links two definitions of each published module's initialiser.
97+
//
98+
// It lives here rather than in the packer because "how do you speak to the
99+
// archiver" is exactly what this table is for. Hard-coding `ar`-style `d`
100+
// there would be right for GNU and llvm-ar and wrong for LIB.EXE, which
101+
// spells it `/REMOVE:<member>` and takes one flag per member — a difference
102+
// in ARITY as well as in spelling, which is why this is a template with a
103+
// placeholder rather than a prefix string.
104+
//
105+
// `{}` is substituted with the member name; the words are joined with
106+
// spaces after the archive path.
107+
std::string_view archiveRemoveArg; // "d" needs no {} | "/REMOVE:{}"
108+
bool archiveRemoveTakesArchiveFirst = true;
92109
};
93110

94111
// Dialect lookup. GCC / Clang / MinGW → gnu; MSVC → msvc.
@@ -189,6 +206,9 @@ constexpr CommandDialect kGnuDialect{
189206
.rspfileLink = false,
190207
.linkStyle = CommandDialect::LinkStyle::Driver,
191208
.archiveCmd = "$ar rcs $out $in",
209+
// `ar d <archive> <member>...` — one verb, then every member.
210+
.archiveRemoveArg = "d",
211+
.archiveRemoveTakesArchiveFirst = true,
192212
};
193213

194214
// Native cl.exe. Unreachable in builds until the MSVC backend lands
@@ -218,6 +238,12 @@ constexpr CommandDialect kMsvcDialect{
218238
.rspfileLink = true,
219239
.linkStyle = CommandDialect::LinkStyle::SeparateLinker,
220240
.archiveCmd = "$ar /nologo /OUT:$out $in",
241+
// `LIB /REMOVE:<member> … <archive>` — one flag PER member, and the
242+
// archive last. Untested against a real LIB.EXE (mcpp's Windows CI packs
243+
// with clang, whose llvm-ar takes the GNU form), so a failure here is
244+
// reported with the command that produced it rather than swallowed.
245+
.archiveRemoveArg = "/REMOVE:{}",
246+
.archiveRemoveTakesArchiveFirst = false,
221247
};
222248

223249
} // namespace
Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
#!/usr/bin/env bash
2+
# requires:
3+
# 252_pack_library_old_client.sh — an mcpp that predates library packaging must
4+
# still BUILD against a package produced by one that has it.
5+
#
6+
# That claim is the reason the generated manifest introduces no section and no
7+
# key: everything in it — `sources`, `include_dirs`, `[modules] exports`, a
8+
# `cfg(...)` block per leg, `[[runtime.artifacts]]` — was already parsed before
9+
# this feature existed. An older client reads the package and links it; what it
10+
# does not do is run the gates, because it has no way to know that
11+
# `provenance = "mcpp-pack …"` means anything.
12+
#
13+
# Two halves, because only one of them can run everywhere:
14+
#
15+
# 1. STATIC — the generated manifest's top-level sections are a subset of the
16+
# vocabulary that predates this feature. Portable, and it is the actual
17+
# invariant rather than a proxy for it.
18+
# 2. REAL — consume the package with $MCPP_BOOT, the released mcpp each CI job
19+
# bootstraps from. Skipped with a loud note when that is not available,
20+
# never silently.
21+
set -e
22+
source "$(dirname "$0")/_host_path.sh"
23+
24+
TMP=$(mktemp -d)
25+
trap "rm -rf $TMP" EXIT
26+
cd "$TMP"
27+
28+
mkdir -p mathkit/src
29+
cat > mathkit/src/mathkit.cppm <<'EOF'
30+
export module mathkit;
31+
export namespace mk { int answer(); }
32+
EOF
33+
cat > mathkit/src/impl.cpp <<'EOF'
34+
module mathkit;
35+
namespace mk { int answer() { return 42; } }
36+
EOF
37+
cat > mathkit/mcpp.toml <<'EOF'
38+
[package]
39+
name = "mathkit"
40+
version = "0.1.0"
41+
[build]
42+
sources = ["src/*.cppm", "src/*.cpp"]
43+
[targets.mathkit]
44+
kind = "lib"
45+
EOF
46+
47+
cd mathkit
48+
"$MCPP" pack mathkit > pack.log 2>&1 || { cat pack.log; echo "pack failed"; exit 1; }
49+
pkg="$TMP/mathkit/$(find target/dist -maxdepth 1 -type d -name 'mathkit-0.1.0-*' | head -1)"
50+
PKG_HOST="$(host_path "$pkg")"
51+
52+
# ── 1. no section outside the pre-existing vocabulary ──────────────────
53+
#
54+
# Listed literally rather than derived: the point is that this set was frozen
55+
# before the feature, so a new entry has to be added here deliberately — and
56+
# adding one is exactly the moment to ask whether older clients can still read
57+
# the package.
58+
# `\[\[?` covers both a table and an array-of-tables header: `[[runtime.artifacts]]`
59+
# is the same section as `[runtime]` for this purpose, and the first version of
60+
# this pattern matched only the single-bracket form — so it flagged the very
61+
# section the design deliberately reuses.
62+
known='^\[\[?(package|build|modules|targets\.|target\.|dependencies|dev-dependencies|runtime|profile\.|features|lib|pack|workspace|indices|resources|xlings|capabilities|tools)'
63+
bad="$(grep -E '^\[' "$pkg/mcpp.toml" | grep -Ev "$known" || true)"
64+
[[ -z "$bad" ]] || {
65+
echo "FAIL: the generated manifest uses sections an older mcpp cannot read:"
66+
printf '%s\n' "$bad"
67+
echo " Either express the fact with an existing key, or accept that packages"
68+
echo " need a version floor — and say so in docs/12."
69+
exit 1; }
70+
71+
# ── 2. the released client actually builds against it ──────────────────
72+
mkdir -p "$TMP/app/src"
73+
cat > "$TMP/app/src/main.cpp" <<'EOF'
74+
#include <cstdio>
75+
import mathkit;
76+
int main(){ std::printf("ok=%d\n", mk::answer()); return 0; }
77+
EOF
78+
cat > "$TMP/app/mcpp.toml" <<EOF
79+
[package]
80+
name = "app"
81+
version = "0.1.0"
82+
[dependencies]
83+
mathkit = { path = "$PKG_HOST" }
84+
[targets.app]
85+
kind = "bin"
86+
main = "src/main.cpp"
87+
EOF
88+
89+
# Baseline with the PR binary, so a failure below is attributable to the client
90+
# and not to the package.
91+
( cd "$TMP/app" && "$MCPP" run > new.log 2>&1 ) \
92+
|| { cat "$TMP/app/new.log"; echo "the PR binary could not consume its own package"; exit 1; }
93+
grep -q 'ok=42' "$TMP/app/new.log" || { cat "$TMP/app/new.log"; echo "wrong answer"; exit 1; }
94+
95+
if [[ -n "${MCPP_BOOT:-}" && -x "${MCPP_BOOT}" ]] \
96+
&& [[ "$("$MCPP_BOOT" --version 2>/dev/null)" != "$("$MCPP" --version 2>/dev/null)" ]]; then
97+
echo "old client: $("$MCPP_BOOT" --version)"
98+
rm -rf "$TMP/app/target"
99+
( cd "$TMP/app" && "$MCPP_BOOT" run > old.log 2>&1 ) || {
100+
cat "$TMP/app/old.log"
101+
echo "FAIL: the released mcpp cannot build against a package this one produced."
102+
echo " The compatibility claim in docs/12 is then false: such packages"
103+
echo " need a version floor, and publishing one without it bricks older"
104+
echo " clients rather than degrading them."
105+
exit 1; }
106+
grep -q 'ok=42' "$TMP/app/old.log" || {
107+
cat "$TMP/app/old.log"; echo "the old client built it but ran it wrong"; exit 1; }
108+
echo "PASS: a released mcpp builds and runs against a package from this one"
109+
else
110+
echo "NOTE: \$MCPP_BOOT is unset or identical to \$MCPP — the real old-client"
111+
echo " check did not run here. The static section-vocabulary check did."
112+
echo "PASS: the generated manifest introduces no section an older mcpp cannot read"
113+
fi

0 commit comments

Comments
 (0)