Skip to content

Commit e36ecda

Browse files
committed
fix(pack): one Error per module, and route the named target both ways
Two problems the first push found, one from CI and one from running the previous release side by side. `mcpp.pack.library` exported a `mcpp::pack::Error` and `mcpp.pack` already had one. A name attaches to exactly one module, and mcpp.pack.library_pipeline imports both — clang refuses outright ("cannot be attached to other modules"), GCC accepted it. Every Windows and macOS job failed on it while the Linux ones were green, which is the whole argument for the three-platform matrix. `mcpp pack` in a workspace root stopped working. Routing on `[targets.<n>].kind` means something reads the manifest before the build does, and a workspace root has no targets of its own — a virtual one has no `[package]` either — so the new router read an empty list and concluded there was nothing to pack. Found by running the previous release against examples/04-workspace and comparing; e2e 249 is that comparison made permanent. And the positional was accepted but never reached the application pipeline, so a project with two `bin` targets would take `mcpp pack app2` and bundle app1 — succeeding with the wrong answer. e2e 250 pins both directions plus the refusal for an unknown name.
1 parent 84e6520 commit e36ecda

6 files changed

Lines changed: 181 additions & 14 deletions

File tree

src/cli/cmd_publish.cppm

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ export int cmd_pack(const mcpplibs::cmdline::ParsedArgs& parsed) {
8888
"bundle wraps one executable, and one executable has one target.");
8989
return 2;
9090
}
91-
return mcpp::pack::build_and_pack(std::move(opts), modeFromUser);
91+
return mcpp::pack::build_and_pack(std::move(opts), modeFromUser, route->targetName);
9292
}
9393

9494
} // namespace mcpp::cli

src/pack/library.cppm

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -79,28 +79,33 @@ struct LibraryPackPlan {
7979
std::vector<LibraryLeg> legs;
8080
};
8181

82-
struct Error { std::string message; };
82+
// NOT `Error`. `mcpp.pack` already exports a `mcpp::pack::Error`, and a name
83+
// can only be attached to one module — clang rejects the second outright
84+
// ("cannot be attached to other modules"), and mcpp.pack.library_pipeline
85+
// imports both. GCC accepted it, which is exactly why the Windows leg of CI
86+
// is the one that found this.
87+
struct LibraryPackError { std::string message; };
8388

8489
// Stage, drop, describe, archive. Returns the path a caller should report.
8590
//
8691
// The digests it records come from mcpp.pack.digest, which the CONSUMER also
8792
// uses — one derivation, verified from both ends.
88-
std::expected<std::filesystem::path, Error> run_library_pack(const LibraryPackPlan& plan);
93+
std::expected<std::filesystem::path, LibraryPackError> run_library_pack(const LibraryPackPlan& plan);
8994

9095
} // namespace mcpp::pack
9196

9297
namespace mcpp::pack {
9398

9499
namespace {
95100

96-
std::expected<void, Error> copy_into(const std::filesystem::path& src,
101+
std::expected<void, LibraryPackError> copy_into(const std::filesystem::path& src,
97102
const std::filesystem::path& dst)
98103
{
99104
std::error_code ec;
100105
std::filesystem::create_directories(dst.parent_path(), ec);
101106
std::filesystem::copy_file(src, dst,
102107
std::filesystem::copy_options::overwrite_existing, ec);
103-
if (ec) return std::unexpected(Error{ std::format(
108+
if (ec) return std::unexpected(LibraryPackError{ std::format(
104109
"cannot copy '{}' -> '{}': {}", src.string(), dst.string(), ec.message()) });
105110
return {};
106111
}
@@ -119,13 +124,13 @@ std::vector<std::filesystem::path> walk(const std::filesystem::path& root) {
119124

120125
} // namespace
121126

122-
std::expected<std::filesystem::path, Error>
127+
std::expected<std::filesystem::path, LibraryPackError>
123128
run_library_pack(const LibraryPackPlan& plan)
124129
{
125130
std::error_code ec;
126131
std::filesystem::remove_all(plan.stagingRoot, ec);
127132
std::filesystem::create_directories(plan.stagingRoot, ec);
128-
if (ec) return std::unexpected(Error{ std::format(
133+
if (ec) return std::unexpected(LibraryPackError{ std::format(
129134
"cannot create staging dir '{}': {}", plan.stagingRoot.string(), ec.message()) });
130135

131136
// ── interface/ ────────────────────────────────────────────────────
@@ -141,7 +146,7 @@ run_library_pack(const LibraryPackPlan& plan)
141146
for (auto const& src : plan.interfaceSources) {
142147
auto name = src.filename().string();
143148
if (auto it = seen.find(name); it != seen.end()) {
144-
return std::unexpected(Error{ std::format(
149+
return std::unexpected(LibraryPackError{ std::format(
145150
"two interface units are both called '{}':\n"
146151
" {}\n {}\n"
147152
"A package's interface is published flat, so their names must differ.",
@@ -171,7 +176,7 @@ run_library_pack(const LibraryPackPlan& plan)
171176
std::vector<PackageLeg> docLegs;
172177
for (auto const& leg : plan.legs) {
173178
if (!std::filesystem::exists(leg.artifact, ec)) {
174-
return std::unexpected(Error{ std::format(
179+
return std::unexpected(LibraryPackError{ std::format(
175180
"the build for '{}' produced no artifact at '{}'",
176181
leg.triple, leg.artifact.string()) });
177182
}
@@ -189,7 +194,7 @@ run_library_pack(const LibraryPackPlan& plan)
189194
cmd += " " + mcpp::platform::shell::quote(m);
190195
auto r = mcpp::platform::process::capture(cmd + " 2>&1");
191196
if (r.exit_code != 0) {
192-
return std::unexpected(Error{ std::format(
197+
return std::unexpected(LibraryPackError{ std::format(
193198
"cannot drop published interface objects from '{}' (rc={}): {}",
194199
dst.string(), r.exit_code, r.output) });
195200
}
@@ -240,7 +245,7 @@ run_library_pack(const LibraryPackPlan& plan)
240245
doc.dependencies = plan.dependencies;
241246

242247
std::ofstream os(plan.stagingRoot / "mcpp.toml", std::ios::binary);
243-
if (!os) return std::unexpected(Error{ std::format(
248+
if (!os) return std::unexpected(LibraryPackError{ std::format(
244249
"cannot write '{}'", (plan.stagingRoot / "mcpp.toml").string()) });
245250
os << emit_package_manifest(doc);
246251
}
@@ -259,15 +264,15 @@ run_library_pack(const LibraryPackPlan& plan)
259264
});
260265
}
261266
if (auto r = zip::write(plan.archivePath, entries); !r)
262-
return std::unexpected(Error{ r.error() });
267+
return std::unexpected(LibraryPackError{ r.error() });
263268
} else {
264269
auto cmd = std::format("tar -czf {} -C {} {}",
265270
mcpp::platform::shell::quote(plan.archivePath.string()),
266271
mcpp::platform::shell::quote(plan.stagingRoot.parent_path().string()),
267272
mcpp::platform::shell::quote(plan.stagingRoot.filename().string()));
268273
auto r = mcpp::platform::process::capture(cmd + " 2>&1");
269274
if (r.exit_code != 0)
270-
return std::unexpected(Error{ std::format(
275+
return std::unexpected(LibraryPackError{ std::format(
271276
"tar failed (rc={}): {}", r.exit_code, r.output) });
272277
}
273278
return plan.archivePath;

src/pack/pipeline.cppm

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,15 @@ import mcpp.ui;
2323
namespace mcpp::pack {
2424

2525
// Everything after CLI option parsing for `mcpp pack`.
26-
export int build_and_pack(Options opts, bool modeFromUser) {
26+
//
27+
// `wantTarget` is the target NAME the user asked for, empty when they did not.
28+
// It exists because `mcpp pack <name>` now routes on `[targets.<name>].kind`:
29+
// a name that resolves to a program has to reach the binary selection below,
30+
// or a project with two `bin` targets would accept `mcpp pack app2` and
31+
// silently bundle app1 — the shape where the command succeeds and the answer
32+
// is wrong.
33+
export int build_and_pack(Options opts, bool modeFromUser,
34+
const std::string& wantTarget = {}) {
2735
// `--target *-linux-musl` without an explicit `--mode` implies
2836
// `--mode static` — packaging a musl-static ELF as bundle-project
2937
// would feed patchelf a static binary and crash. The docs treat
@@ -83,8 +91,26 @@ export int build_and_pack(Options opts, bool modeFromUser) {
8391
}
8492

8593
// ─── Pick the main binary target ─────────────────────────────────
94+
//
95+
// An explicitly named target wins over the package-name convention: the
96+
// user said which one, and guessing past that is how `mcpp pack app2`
97+
// would produce app1's bundle under app2's name.
8698
std::filesystem::path mainBinary;
99+
if (!wantTarget.empty()) {
100+
for (auto& lu : ctx->plan.linkUnits) {
101+
if (lu.kind == mcpp::build::LinkUnit::Binary && lu.targetName == wantTarget) {
102+
mainBinary = ctx->outputDir / lu.output;
103+
break;
104+
}
105+
}
106+
if (mainBinary.empty()) {
107+
mcpp::ui::error(std::format(
108+
"target '{}' is not a program in this build", wantTarget));
109+
return 2;
110+
}
111+
}
87112
for (auto& lu : ctx->plan.linkUnits) {
113+
if (!mainBinary.empty()) break;
88114
if (lu.kind == mcpp::build::LinkUnit::Binary
89115
&& lu.targetName == ctx->manifest.package.name)
90116
{

src/pack/route.cppm

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,17 @@ std::expected<PackRoute, std::string> route_pack_target(std::string_view request
7878
list.empty() ? "" : "; this package declares: ", list));
7979
}
8080

81+
// A WORKSPACE ROOT has no targets of its own — a virtual one has no
82+
// `[package]` at all. `mcpp pack` there has always meant "pack the member",
83+
// and the application pipeline is what resolves which member that is. So
84+
// hand it straight through rather than reading the root's (empty) target
85+
// list and concluding there is nothing to pack.
86+
//
87+
// Found by running the old binary and the new one against
88+
// examples/04-workspace: the routing added here turned a working command
89+
// into "this package declares no program and no library to pack".
90+
if (m->targets.empty() && m->workspace.present) return PackRoute{ {}, false };
91+
8192
// Nothing requested. A program is still the default — `mcpp pack` has
8293
// always meant "bundle this application" and a project that has one is
8394
// asking for that.
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
#!/usr/bin/env bash
2+
# requires: gcc
3+
# 249_pack_workspace_root_unchanged.sh — `mcpp pack` in a workspace root still
4+
# packs the member's program.
5+
#
6+
# Routing `mcpp pack` through `[targets.<n>].kind` means something has to read
7+
# the manifest before the build does. A workspace root has no targets of its
8+
# own — a virtual one has no `[package]` either — so the first version of that
9+
# routing read the root's empty target list and concluded there was nothing to
10+
# pack, turning a working command into an error.
11+
#
12+
# Caught by running the previous release against examples/04-workspace and
13+
# comparing. This test is that comparison, made permanent.
14+
set -e
15+
16+
TMP=$(mktemp -d)
17+
trap "rm -rf $TMP" EXIT
18+
cd "$TMP"
19+
20+
mkdir -p ws/apps/hello/src ws/libs/mathcore/src
21+
22+
cat > ws/mcpp.toml <<'EOF'
23+
[workspace]
24+
members = ["libs/mathcore", "apps/hello"]
25+
EOF
26+
27+
cat > ws/libs/mathcore/mcpp.toml <<'EOF'
28+
[package]
29+
name = "mathcore"
30+
version = "0.1.0"
31+
EOF
32+
cat > ws/libs/mathcore/src/mathcore.cppm <<'EOF'
33+
export module mathcore;
34+
export int core_answer() { return 42; }
35+
EOF
36+
37+
cat > ws/apps/hello/mcpp.toml <<'EOF'
38+
[package]
39+
name = "hello"
40+
version = "0.1.0"
41+
[dependencies]
42+
mathcore = { path = "../../libs/mathcore" }
43+
EOF
44+
cat > ws/apps/hello/src/main.cpp <<'EOF'
45+
#include <cstdio>
46+
import mathcore;
47+
int main() { std::printf("ok=%d\n", core_answer()); return 0; }
48+
EOF
49+
50+
cd ws
51+
"$MCPP" pack --mode system > pack.log 2>&1 || {
52+
cat pack.log
53+
echo "FAIL: packing from a virtual workspace root stopped working"
54+
exit 1
55+
}
56+
grep -q 'Packed' pack.log || { cat pack.log; echo "no archive reported"; exit 1; }
57+
[[ -n "$(find . -name 'hello-0.1.0-*.tar.gz' | head -1)" ]] || {
58+
cat pack.log; echo "the member's archive was not produced"; find . -name '*.tar.gz'; exit 1; }
59+
60+
echo "PASS: a workspace root still packs its member's program"
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
#!/usr/bin/env bash
2+
# requires: gcc
3+
# 250_pack_names_the_target.sh — `mcpp pack <name>` packs the target it was
4+
# given, and refuses a name it cannot pack.
5+
#
6+
# The positional is what decides application-bundle vs library-package, so it
7+
# has to reach BOTH pipelines. It did not reach the application one at first:
8+
# a project with two `bin` targets accepted `mcpp pack app2` and bundled app1,
9+
# which is the shape where the command succeeds and the answer is wrong.
10+
set -e
11+
12+
TMP=$(mktemp -d)
13+
trap "rm -rf $TMP" EXIT
14+
cd "$TMP"
15+
16+
mkdir -p two/src
17+
cat > two/src/alpha.cpp <<'EOF'
18+
#include <cstdio>
19+
int main() { std::printf("alpha\n"); return 0; }
20+
EOF
21+
cat > two/src/beta.cpp <<'EOF'
22+
#include <cstdio>
23+
int main() { std::printf("beta\n"); return 0; }
24+
EOF
25+
cat > two/mcpp.toml <<'EOF'
26+
[package]
27+
name = "two"
28+
version = "0.1.0"
29+
30+
[targets.alpha]
31+
kind = "bin"
32+
main = "src/alpha.cpp"
33+
34+
[targets.beta]
35+
kind = "bin"
36+
main = "src/beta.cpp"
37+
EOF
38+
39+
cd two
40+
41+
# ── the named program is the one that gets bundled ─────────────────────
42+
"$MCPP" pack beta --mode system > beta.log 2>&1 || { cat beta.log; echo "pack beta failed"; exit 1; }
43+
staged="$(find target/dist -maxdepth 1 -type d -name 'two-0.1.0*' | head -1)"
44+
[[ -n "$staged" ]] || { cat beta.log; echo "no staging dir"; exit 1; }
45+
find "$staged" -type f -name 'beta*' | grep -q . || {
46+
echo "FAIL: 'mcpp pack beta' did not bundle beta"; find "$staged" -type f; exit 1; }
47+
find "$staged" -type f -name 'alpha*' | grep -q . && {
48+
echo "FAIL: 'mcpp pack beta' bundled alpha instead"; find "$staged" -type f; exit 1; }
49+
50+
# ── and the other one, to prove the first result was not the default ───
51+
rm -rf target/dist
52+
"$MCPP" pack alpha --mode system > alpha.log 2>&1 || { cat alpha.log; echo "pack alpha failed"; exit 1; }
53+
staged="$(find target/dist -maxdepth 1 -type d -name 'two-0.1.0*' | head -1)"
54+
find "$staged" -type f -name 'alpha*' | grep -q . || {
55+
echo "FAIL: 'mcpp pack alpha' did not bundle alpha"; find "$staged" -type f; exit 1; }
56+
57+
# ── an unknown name is refused, and says what there is ─────────────────
58+
if "$MCPP" pack nosuch --mode system > bad.log 2>&1; then
59+
cat bad.log; echo "FAIL: an unknown target name was accepted"; exit 1
60+
fi
61+
grep -q "no target named 'nosuch'" bad.log || { cat bad.log; echo "wrong refusal"; exit 1; }
62+
grep -q 'alpha' bad.log && grep -q 'beta' bad.log || {
63+
cat bad.log; echo "the refusal did not list the available targets"; exit 1; }
64+
65+
echo "PASS: mcpp pack packs the target it is given"

0 commit comments

Comments
 (0)