Skip to content

Commit 586e865

Browse files
committed
refactor: eliminate remaining platform macros (P0-P6)
P0: Remove all #define popen/_popen macros from 6 files — migrate every raw popen/pclose/std::system call to platform::process APIs. Files: cli, ninja_backend, msvc, stdmod, pack, publisher, p1689. Add run_passthrough() and extract_exit_code() to platform::process. P1: Replace all manual shell quoting #ifdefs in cli.cppm with platform::shell::quote(). Covers git clone, ninja -C, cmd_run, cmd_test (5 locations). P2: Create platform/terminal.cppm — absorbs is_tty() and terminal_cols() from ui.cppm. Eliminates #ifdef __unix__ blocks. P3: Move kXpkgPlatform constant to platform::common::xpkg_platform. resolver.cppm now uses it without #if defined blocks. P4: Add link strategy constants (supports_full_static, supports_rpath, needs_explicit_libcxx) to platform::common. flags.cppm now uses if constexpr instead of #if defined blocks. P5: Replace #if defined(_WIN32) in xlings.cppm build_command_prefix, install_with_progress, ensure_init with if constexpr. P6: Eliminate all #if defined blocks in ninja_backend.cppm rule generation. Introduce constexpr $toolenv prefix variable, use if constexpr for platform-specific rules (cp_bmi, scan_deps). Net: ~440 lines of #ifdef removed, 315 lines of clean constexpr/ platform API code added. Zero raw popen calls remain outside src/platform/.
1 parent 8add877 commit 586e865

16 files changed

Lines changed: 416 additions & 440 deletions
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
# Remaining Platform Macros Outside src/platform/ — Analysis Report
2+
3+
## Summary
4+
5+
14 files, 39 occurrences of platform-specific macros/conditionals outside `src/platform/`.
6+
7+
## P0: Eliminate `#define popen _popen` (6 files, ~16 occurrences)
8+
9+
Migrate all raw popen/pclose calls to `platform::process::capture()` / `run_streaming()` / `run_silent()`.
10+
11+
Files: cli.cppm, ninja_backend.cppm, msvc.cppm, stdmod.cppm, pack/pack.cppm, pm/publisher.cppm, modgraph/p1689.cppm
12+
13+
## P1: Shell quoting in cli.cppm (5 occurrences)
14+
15+
Replace manual `'...'` vs `"..."` with `platform::shell::quote()`.
16+
17+
Lines: 1954, 1965, 2527, 2620, 3178
18+
19+
## P2: Terminal abstraction — ui.cppm (3 occurrences)
20+
21+
Create `platform/terminal.cppm` for isatty, terminal width detection.
22+
23+
Lines: 8-9, 149, 329
24+
25+
## P3: kXpkgPlatform in resolver.cppm (1 occurrence)
26+
27+
Move to `platform::common` as a constant.
28+
29+
Line: 30
30+
31+
## P4: Link strategy constants in flags.cppm (3 occurrences)
32+
33+
Add `supports_full_static`, `supports_rpath`, `needs_explicit_libcxx` to `platform::common`.
34+
35+
Lines: 162, 175, 183
36+
37+
## P5: xlings.cppm build_command_prefix (3 occurrences)
38+
39+
Abstract Windows env-setting vs POSIX shell-prefix into platform layer.
40+
41+
Lines: 401, 612, 739
42+
43+
## P6: ninja_backend.cppm rule generation (10 occurrences)
44+
45+
Abstract `$toolenv` prefix and shell syntax differences.
46+
47+
Lines: 178, 207, 231, 243, 254, 287, 297, 548, 560
48+
49+
## Priority: P0+P1 = 54% elimination with minimal risk.

src/build/flags.cppm

Lines changed: 16 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ export module mcpp.build.flags;
1010

1111
import std;
1212
import mcpp.build.plan;
13+
import mcpp.platform;
1314
import mcpp.toolchain.clang;
1415
import mcpp.toolchain.detect;
1516
import mcpp.toolchain.provider;
@@ -159,46 +160,23 @@ CompileFlags compute_flags(const BuildPlan& plan) {
159160
// Link flags
160161
f.staticStdlib = plan.manifest.buildConfig.staticStdlib;
161162
f.linkage = plan.manifest.buildConfig.linkage;
162-
#if defined(_WIN32)
163-
// Windows: MSVC linker handles static/dynamic linking differently
164-
std::string full_static;
165-
std::string static_stdlib;
166-
#elif defined(__APPLE__)
167-
// macOS does not support full static linking (libSystem must be dynamic)
168-
std::string full_static;
169-
std::string static_stdlib = (f.staticStdlib && !isClang) ? " -static-libstdc++" : "";
170-
#else
171-
std::string full_static = (f.linkage == "static") ? " -static" : "";
172-
std::string static_stdlib = (f.staticStdlib && !isClang) ? " -static-libstdc++" : "";
173-
#endif
163+
std::string full_static = (mcpp::platform::supports_full_static && f.linkage == "static") ? " -static" : "";
164+
std::string static_stdlib = (f.staticStdlib && !isClang && !mcpp::platform::is_windows) ? " -static-libstdc++" : "";
174165
std::string runtime_dirs;
175-
#if !defined(_WIN32)
176-
// -L and -rpath are ELF/Mach-O linker flags; MSVC linker doesn't use them.
177-
for (auto& dir : plan.toolchain.linkRuntimeDirs) {
178-
runtime_dirs += " -L" + escape_path(dir);
179-
runtime_dirs += " -Wl,-rpath," + escape_path(dir);
166+
if constexpr (mcpp::platform::supports_rpath) {
167+
for (auto& dir : plan.toolchain.linkRuntimeDirs) {
168+
runtime_dirs += " -L" + escape_path(dir);
169+
runtime_dirs += " -Wl,-rpath," + escape_path(dir);
170+
}
171+
}
172+
173+
if constexpr (mcpp::platform::is_windows) {
174+
f.ld = "";
175+
} else if constexpr (mcpp::platform::needs_explicit_libcxx) {
176+
f.ld = std::format("{}{}{} -lc++", full_static, static_stdlib, b_flag);
177+
} else {
178+
f.ld = std::format("{}{}{}{}{}", full_static, static_stdlib, sysroot_flag, b_flag, runtime_dirs);
180179
}
181-
#endif
182-
183-
#if defined(_WIN32)
184-
// Windows: Clang targeting MSVC links against MSVC runtime automatically.
185-
// No -L/-rpath/-static flags needed.
186-
f.ld = "";
187-
#elif defined(__APPLE__)
188-
// macOS linking strategy:
189-
// - No --sysroot: SDK .tbd stubs miss libc++abi exports.
190-
// - No -L<llvm>/lib: xlings LLVM's libc++.dylib doesn't pull in
191-
// libc++abi. System /usr/lib/libc++ does (and is ABI-compatible
192-
// with LLVM 20 headers since macOS ships a recent libc++).
193-
// - No -rpath for LLVM lib: binary should use system libc++ at runtime.
194-
// - Explicit -lc++: clang++.cfg's -nostdinc++ suppresses implicit linkage.
195-
// Result: compile with LLVM headers, link with system libc++ + libc++abi.
196-
f.ld = std::format("{}{}{} -lc++", full_static, static_stdlib, b_flag);
197-
#else
198-
// Linux: sysroot + runtime dirs needed (glibc/libc++ live in sandbox)
199-
f.ld = std::format("{}{}{}{}{}", full_static, static_stdlib, sysroot_flag, b_flag,
200-
runtime_dirs);
201-
#endif
202180

203181
return f;
204182
}

src/build/ninja_backend.cppm

Lines changed: 62 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,6 @@
1414
module;
1515
#include <cstdio>
1616
#include <cstdlib>
17-
#if defined(_WIN32)
18-
#define popen _popen
19-
#define pclose _pclose
20-
#endif
2117

2218
export module mcpp.build.ninja;
2319

@@ -90,20 +86,14 @@ void write_file(const std::filesystem::path& p, std::string_view content) {
9086
os << content;
9187
}
9288

93-
bool run(const std::string& cmd, std::string& output_capture, bool capture = true) {
94-
std::array<char, 8192> buf{};
89+
bool run(const std::string& cmd, std::string& output_capture, bool capture_output = true) {
9590
output_capture.clear();
96-
std::FILE* fp = ::popen(cmd.c_str(), "r");
97-
if (!fp)
98-
return false;
99-
while (std::fgets(buf.data(), buf.size(), fp) != nullptr) {
100-
if (capture)
101-
output_capture += buf.data();
102-
else
103-
std::fputs(buf.data(), stdout);
91+
if (capture_output) {
92+
auto r = mcpp::platform::process::capture(cmd);
93+
output_capture = r.output;
94+
return r.exit_code == 0;
10495
}
105-
int rc = ::pclose(fp);
106-
return rc == 0;
96+
return mcpp::platform::process::run_passthrough(cmd) == 0;
10797
}
10898

10999
bool dyndep_mode_enabled() {
@@ -175,13 +165,13 @@ std::string emit_ninja_string(const BuildPlan& plan) {
175165
append("\n");
176166

177167
append("rule cp_bmi\n");
178-
#if defined(_WIN32)
179-
// Use PowerShell Copy-Item which handles both forward and back slashes.
180-
// cmd.exe `copy` breaks on forward-slash paths from ninja.
181-
append(" command = powershell -NoProfile -Command \"Copy-Item -Force '$in' -Destination '$out'\"\n");
182-
#else
183-
append(" command = mkdir -p $$(dirname $out) && cp -f $in $out\n");
184-
#endif
168+
if constexpr (mcpp::platform::is_windows) {
169+
// Use PowerShell Copy-Item which handles both forward and back slashes.
170+
// cmd.exe `copy` breaks on forward-slash paths from ninja.
171+
append(" command = powershell -NoProfile -Command \"Copy-Item -Force '$in' -Destination '$out'\"\n");
172+
} else {
173+
append(" command = mkdir -p $$(dirname $out) && cp -f $in $out\n");
174+
}
185175
append(" description = STAGE $out\n\n");
186176

187177
// P1: per-file dyndep rule. Converts one .ddi → .dd independently.
@@ -201,81 +191,63 @@ std::string emit_ninja_string(const BuildPlan& plan) {
201191
//
202192
// $bmi_out is set per build edge to the BMI path (gcm.cache/<module>.gcm).
203193
// If $bmi_out is empty (no module provided), we just compile normally.
194+
// On POSIX, $toolenv prefixes commands with LD_LIBRARY_PATH etc.
195+
// On Windows, $toolenv is empty and its leading space breaks CreateProcess,
196+
// so we omit it entirely.
197+
constexpr std::string_view te = mcpp::platform::is_windows ? "" : "$toolenv ";
198+
204199
std::string module_output_flag = traits.needsExplicitModuleOutput
205200
? " -fmodule-output=$bmi_out" : "";
206201
append("rule cxx_module\n");
207-
#if defined(_WIN32)
208-
// Windows: skip BMI restat optimization (requires POSIX shell).
209-
// No $toolenv (empty on Windows; its leading space breaks CreateProcess).
210-
append(std::format(" command = "
211-
"$cxx $cxxflags{} -c $in -o $out\n", module_output_flag));
212-
#else
213-
append(std::format(" command = "
214-
"if [ -n \"$bmi_out\" ] && [ -f \"$bmi_out\" ]; then "
215-
"cp -p \"$bmi_out\" \"$bmi_out.bak\"; "
216-
"fi && "
217-
"$toolenv $cxx $cxxflags{} -c $in -o $out && "
218-
"if [ -n \"$bmi_out\" ] && [ -f \"$bmi_out.bak\" ] && "
219-
"cmp -s \"$bmi_out\" \"$bmi_out.bak\"; then "
220-
"mv \"$bmi_out.bak\" \"$bmi_out\"; "
221-
"else "
222-
"rm -f \"$bmi_out.bak\"; "
223-
"fi\n", module_output_flag));
224-
#endif
202+
if constexpr (mcpp::platform::is_windows) {
203+
// Windows: skip BMI restat optimization (requires POSIX shell).
204+
append(std::format(" command = "
205+
"$cxx $cxxflags{} -c $in -o $out\n", module_output_flag));
206+
} else {
207+
append(std::format(" command = "
208+
"if [ -n \"$bmi_out\" ] && [ -f \"$bmi_out\" ]; then "
209+
"cp -p \"$bmi_out\" \"$bmi_out.bak\"; "
210+
"fi && "
211+
"{}$cxx $cxxflags{} -c $in -o $out && "
212+
"if [ -n \"$bmi_out\" ] && [ -f \"$bmi_out.bak\" ] && "
213+
"cmp -s \"$bmi_out\" \"$bmi_out.bak\"; then "
214+
"mv \"$bmi_out.bak\" \"$bmi_out\"; "
215+
"else "
216+
"rm -f \"$bmi_out.bak\"; "
217+
"fi\n", te, module_output_flag));
218+
}
225219
append(" description = MOD $out\n");
226220
if (dyndep)
227221
append(" restat = 1\n");
228222
append("\n");
229223

230224
append("rule cxx_object\n");
231-
#if defined(_WIN32)
232-
append(" command = $cxx $cxxflags -c $in -o $out\n");
233-
#else
234-
append(" command = $toolenv $cxx $cxxflags -c $in -o $out\n");
235-
#endif
225+
append(std::format(" command = {}$cxx $cxxflags -c $in -o $out\n", te));
236226
append(" description = OBJ $out\n");
237227
if (dyndep)
238228
append(" restat = 1\n");
239229
append("\n");
240230

241231
if (need_c_rule) {
242232
append("rule c_object\n");
243-
#if defined(_WIN32)
244-
append(" command = $cc $cflags -c $in -o $out\n");
245-
#else
246-
append(" command = $toolenv $cc $cflags -c $in -o $out\n");
247-
#endif
233+
append(std::format(" command = {}$cc $cflags -c $in -o $out\n", te));
248234
append(" description = CC $out\n");
249235
if (dyndep)
250236
append(" restat = 1\n");
251237
append("\n");
252238
}
253239

254-
#if defined(_WIN32)
255240
append("rule cxx_link\n");
256-
append(" command = $cxx $in -o $out $ldflags\n");
241+
append(std::format(" command = {}$cxx $in -o $out $ldflags\n", te));
257242
append(" description = LINK $out\n\n");
258243

259244
append("rule cxx_archive\n");
260-
append(" command = $ar rcs $out $in\n");
245+
append(std::format(" command = {}$ar rcs $out $in\n", te));
261246
append(" description = AR $out\n\n");
262247

263248
append("rule cxx_shared\n");
264-
append(" command = $cxx -shared $in -o $out $ldflags\n");
249+
append(std::format(" command = {}$cxx -shared $in -o $out $ldflags\n", te));
265250
append(" description = SHARED $out\n\n");
266-
#else
267-
append("rule cxx_link\n");
268-
append(" command = $toolenv $cxx $in -o $out $ldflags\n");
269-
append(" description = LINK $out\n\n");
270-
271-
append("rule cxx_archive\n");
272-
append(" command = $toolenv $ar rcs $out $in\n");
273-
append(" description = AR $out\n\n");
274-
275-
append("rule cxx_shared\n");
276-
append(" command = $toolenv $cxx -shared $in -o $out $ldflags\n");
277-
append(" description = SHARED $out\n\n");
278-
#endif
279251

280252
if (dyndep) {
281253
// Scan rule: produce P1689 .ddi for one TU.
@@ -284,25 +256,21 @@ std::string emit_ninja_string(const BuildPlan& plan) {
284256
append("rule cxx_scan\n");
285257
if (plan.scanDepsPath.empty()) {
286258
// GCC path: compiler-integrated P1689 scanning.
287-
#if defined(_WIN32)
288-
append(" command = $cxx $cxxflags -fmodules "
289-
#else
290-
append(" command = $toolenv $cxx $cxxflags -fmodules "
291-
#endif
259+
append(std::format(" command = {}$cxx $cxxflags -fmodules "
292260
"-fdeps-format=p1689r5 "
293261
"-fdeps-file=$out -fdeps-target=$compile_target "
294-
"-M -MM -MF $out.dep -E $in -o $compile_target\n");
262+
"-M -MM -MF $out.dep -E $in -o $compile_target\n", te));
295263
} else {
296264
// Clang path: clang-scan-deps produces P1689 JSON to stdout.
297-
#if defined(_WIN32)
298-
// Wrap in cmd /c for shell redirection (ninja on Windows uses
299-
// CreateProcess which doesn't interpret > as redirect).
300-
append(" command = cmd /c \"$scan_deps -format=p1689 -- "
301-
"$cxx $cxxflags -c $in -o $compile_target > $out\"\n");
302-
#else
303-
append(" command = $toolenv $scan_deps -format=p1689 -- "
304-
"$cxx $cxxflags -c $in -o $compile_target > $out\n");
305-
#endif
265+
if constexpr (mcpp::platform::is_windows) {
266+
// Wrap in cmd /c for shell redirection (ninja on Windows uses
267+
// CreateProcess which doesn't interpret > as redirect).
268+
append(" command = cmd /c \"$scan_deps -format=p1689 -- "
269+
"$cxx $cxxflags -c $in -o $compile_target > $out\"\n");
270+
} else {
271+
append(std::format(" command = {}$scan_deps -format=p1689 -- "
272+
"$cxx $cxxflags -c $in -o $compile_target > $out\n", te));
273+
}
306274
}
307275
append(" description = SCAN $out\n\n");
308276

@@ -545,27 +513,21 @@ std::expected<BuildResult, BuildError> NinjaBackend::build(const BuildPlan& plan
545513
// -B<binutils-bin> flag we emit into cxxflags/ldflags (see
546514
// emit_ninja_string). No PATH injection needed here.
547515
std::filesystem::path ninjaBin;
548-
#if defined(_WIN32)
516+
auto ninja_name = std::string("ninja") + std::string(mcpp::platform::exe_suffix);
549517
if (auto nb = mcpp::xlings::paths::find_sibling_binary(
550-
plan.toolchain.binaryPath, "ninja", "ninja.exe")) {
518+
plan.toolchain.binaryPath, "ninja", ninja_name)) {
551519
ninjaBin = *nb;
552520
}
553-
#else
554-
if (auto nb = mcpp::xlings::paths::find_sibling_binary(
555-
plan.toolchain.binaryPath, "ninja", "ninja")) {
556-
ninjaBin = *nb;
521+
522+
std::string ninjaProgram;
523+
if (!ninjaBin.empty()) {
524+
if constexpr (mcpp::platform::is_windows)
525+
ninjaProgram = ninjaBin.string();
526+
else
527+
ninjaProgram = mcpp::platform::shell::quote(ninjaBin.string());
528+
} else {
529+
ninjaProgram = "ninja";
557530
}
558-
#endif
559-
560-
#if defined(_WIN32)
561-
// Windows: no quotes on first token (cmd.exe strips leading quotes),
562-
// use shq only for the -C argument which may contain spaces.
563-
std::string ninjaProgram =
564-
!ninjaBin.empty() ? ninjaBin.string() : std::string{"ninja"};
565-
#else
566-
std::string ninjaProgram =
567-
!ninjaBin.empty() ? mcpp::xlings::shq(ninjaBin.string()) : std::string{"ninja"};
568-
#endif
569531

570532
// Record ninja binary for P0 fast-path cache.
571533
BuildResult r;

0 commit comments

Comments
 (0)