Skip to content

Commit 0b82803

Browse files
authored
refactor: platform abstraction layer + fix macOS stdin hang (#55)
* feat: add msvc.cppm — robust Visual Studio / MSVC STL discovery New module mcpp.toolchain.msvc with three discovery strategies: 1. vswhere.exe (Microsoft's official VS locator) — most reliable 2. Environment variables (VSINSTALLDIR, VS*COMNTOOLS) 3. Well-known paths (VS 2017-2025, all editions + BuildTools) Replaces the hardcoded VS2022 path in clang.cppm with msvc::find_std_module_source(). Fixes "no std module source" error on machines with non-standard VS installations. Also provides find_cl() for future MSVC toolchain support. * refactor: extract platform abstraction layer into src/platform/ Create a unified platform abstraction layer that consolidates all platform-specific code into dedicated modules under src/platform/: - common.cppm: compile-time constants, platform detection, naming - process.cppm: unified subprocess execution (auto-closes stdin on POSIX — fixes macOS first-run hang where xcrun/xcode- select would block waiting for user input) - fs.cppm: self_exe_path(), which(), RAII FileLock - shell.cppm: platform-aware shell argument quoting - env.cppm: environment variable operations - macos.cppm: Xcode CLT detection, xcrun SDK discovery - linux.cppm: LD_LIBRARY_PATH construction, runtime lib dirs - windows.cppm: PATH manipulation - platform.cppm: facade that re-exports all sub-modules Migrated consumers: - config.cppm: use platform::fs::self_exe_path(), which() - xlings.cppm: use platform::shell, process, env (all raw popen/system calls removed) - probe.cppm: use platform::fs::which(), macos::sdk_path(), linux_::build_ld_library_path_prefix() - bmi_cache.cppm: use platform::fs::FileLock (RAII) - ninja_backend.cppm: use platform::fs::self_exe_path() - clang.cppm: use platform::exe_suffix for tool paths - cli.cppm: use platform::name constant Deleted: - src/platform.cppm: replaced by src/platform/common.cppm - src/process.cppm: absorbed by src/platform/process.cppm + shell.cppm Net result: ~550 lines of scattered #ifdef blocks removed, all subprocess calls now go through platform::process which auto-redirects stdin from /dev/null on POSIX. * fix: add missing #include <cstdio> in xlings.cppm for stderr The previous commit removed <cstdio> during cleanup but stderr is still used in ensure_init(), ensure_patchelf(), and ensure_ninja() warning messages. * 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/. * ci: trigger workflow
1 parent b362894 commit 0b82803

28 files changed

Lines changed: 1370 additions & 916 deletions
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
# Platform Abstraction Layer — Architecture & Implementation Plan
2+
3+
## 1. Problem Statement
4+
5+
mcpp has ~45+ `#if defined(...)` blocks scattered across 10+ source files for platform-specific logic. This causes:
6+
- **stdin leakage**: subprocess calls on macOS don't close stdin → first-run hangs
7+
- **Code duplication**: `self_exe_path()` duplicated in `config.cppm` and `ninja_backend.cppm`
8+
- **Inconsistent abstractions**: `process.cppm` wraps some calls, but many files still use raw `std::system()`/`popen()`
9+
- **Maintenance burden**: adding platform support requires editing 10+ files
10+
11+
## 2. Target Architecture
12+
13+
```
14+
src/platform/
15+
├── platform.cppm # Unified facade (re-exports all platform capabilities)
16+
├── common.cppm # Cross-platform constants + compile-time detection
17+
├── process.cppm # Unified process execution (auto-closes stdin on POSIX)
18+
├── fs.cppm # Platform filesystem ops (exe path, file lock, which)
19+
├── env.cppm # Environment variable operations
20+
├── shell.cppm # Shell quoting + command building
21+
├── macos.cppm # macOS: xcrun, SDK discovery, Xcode CLT detection
22+
├── linux.cppm # Linux: /proc, LD_LIBRARY_PATH, patchelf
23+
└── windows.cppm # Windows: vswhere, MSVC, _putenv_s, registry
24+
```
25+
26+
## 3. Module Responsibilities
27+
28+
### common.cppm — Compile-time constants & platform detection
29+
- Platform booleans: `is_windows`, `is_macos`, `is_linux`
30+
- Binary naming: `exe_suffix`, `static_lib_ext`, `shared_lib_ext`, `lib_prefix`
31+
- Platform name string
32+
- Null redirect string
33+
34+
### process.cppm — Unified process execution (CORE FIX)
35+
- `capture()` — run command, capture stdout (auto `</dev/null` on POSIX)
36+
- `run_silent()` — run without caring about output
37+
- `run_streaming()` — stream stdout line-by-line via callback
38+
- All subprocess calls go through here → stdin leak impossible
39+
40+
### fs.cppm — Platform filesystem operations
41+
- `self_exe_path()` — current executable path (Win: GetModuleFileName, macOS: _NSGetExecutablePath, Linux: /proc/self/exe)
42+
- `FileLock` — RAII exclusive file lock (Win: LockFileEx, POSIX: flock)
43+
- `which()` — find executable (Win: `where`, POSIX: `command -v`)
44+
45+
### env.cppm — Environment variable operations
46+
- `set()` / `get()` — platform-aware env var manipulation
47+
- `build_env_prefix()` — construct env prefix for commands
48+
49+
### shell.cppm — Shell quoting
50+
- `quote()` — platform-aware shell argument quoting
51+
- `silent_redirect` — full silent redirect string
52+
53+
### macos.cppm — macOS-specific
54+
- `has_xcode_clt()` — check Xcode Command Line Tools
55+
- `sdk_path()` — xcrun SDK discovery
56+
- `runtime_lib_dirs()` — macOS library search paths
57+
58+
### linux.cppm — Linux-specific
59+
- `build_ld_library_path()` — LD_LIBRARY_PATH construction
60+
- `runtime_lib_dirs()` — Linux library search paths
61+
62+
### windows.cppm — Windows-specific
63+
- `find_visual_studio()` — VS discovery via vswhere/env/paths
64+
- `find_std_module_source()` — MSVC STL module source
65+
- `prepend_path()` — Windows PATH manipulation
66+
67+
### platform.cppm — Unified facade
68+
- Re-exports all common modules
69+
- Conditionally exports platform-specific modules
70+
71+
## 4. Migration Impact
72+
73+
| Source File | Changes | Effort |
74+
|-------------|---------|--------|
75+
| `config.cppm` | Use `fs::self_exe_path()`, `fs::which()`, `process::run_silent()` | Medium |
76+
| `xlings.cppm` | Use `shell::quote()`, `process::run_streaming()`, `env::*` | Large |
77+
| `process.cppm` | DELETE — absorbed by `platform/process.cppm` + `platform/shell.cppm` | Delete |
78+
| `probe.cppm` | Use `fs::which()`, `macos::sdk_path()` | Medium |
79+
| `bmi_cache.cppm` | Use `fs::FileLock` | Small |
80+
| `ninja_backend.cppm` | Use `fs::self_exe_path()` | Small |
81+
| `flags.cppm` | Use `common` constants | Small |
82+
| `clang.cppm` | Use `windows::find_std_module_source()` | Small |
83+
| `msvc.cppm` | Discovery logic moves to `platform/windows.cppm` | Medium |
84+
| `cli.cppm` | Use `common::name` | Small |
85+
86+
## 5. Implementation Phases
87+
88+
### Phase 1: Skeleton (parallel-safe)
89+
Create directory + `common.cppm` (move from existing `platform.cppm`)
90+
91+
### Phase 2: Core modules (parallelizable)
92+
- 2a: `process.cppm` — includes stdin fix
93+
- 2b: `fs.cppm` — self_exe_path + FileLock + which
94+
- 2c: `env.cppm` + `shell.cppm`
95+
- 2d: `macos.cppm` + `linux.cppm` + `windows.cppm`
96+
- 2e: `platform.cppm` facade
97+
98+
### Phase 3: Consumer migration
99+
Migrate all files to use new platform modules, remove old `process.cppm`
100+
101+
### Phase 4: Build config + CI verification
102+
Update `mcpp.toml`, verify on all platforms
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/bmi_cache.cppm

Lines changed: 3 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -15,18 +15,11 @@
1515
// dep cache without trashing manifest.txt (docs/26 §5.4 V2).
1616

1717
module;
18-
#if defined(_WIN32)
19-
#include <io.h>
20-
#include <windows.h>
21-
#else
22-
#include <fcntl.h>
23-
#include <sys/file.h>
24-
#include <unistd.h>
25-
#endif
2618

2719
export module mcpp.bmi_cache;
2820

2921
import std;
22+
import mcpp.platform;
3023

3124
export namespace mcpp::bmi_cache {
3225

@@ -188,56 +181,6 @@ stage_into(const CacheKey& key,
188181
}
189182

190183
namespace {
191-
192-
// Acquire an exclusive non-blocking lock on <dir>/.lock. Returns a handle
193-
// on success, or -1/INVALID_HANDLE if another mcpp is already populating.
194-
#if defined(_WIN32)
195-
// Windows: use LockFileEx on a file handle
196-
HANDLE try_lock_dir(const std::filesystem::path& dir) {
197-
std::error_code ec;
198-
std::filesystem::create_directories(dir, ec);
199-
auto lockPath = dir / ".lock";
200-
HANDLE h = CreateFileW(lockPath.wstring().c_str(),
201-
GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE,
202-
NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
203-
if (h == INVALID_HANDLE_VALUE) return h;
204-
OVERLAPPED ov = {};
205-
if (!LockFileEx(h, LOCKFILE_EXCLUSIVE_LOCK | LOCKFILE_FAIL_IMMEDIATELY,
206-
0, 1, 0, &ov)) {
207-
CloseHandle(h);
208-
return INVALID_HANDLE_VALUE;
209-
}
210-
return h;
211-
}
212-
213-
void release_lock(HANDLE h) {
214-
if (h == INVALID_HANDLE_VALUE) return;
215-
OVERLAPPED ov = {};
216-
UnlockFileEx(h, 0, 1, 0, &ov);
217-
CloseHandle(h);
218-
}
219-
#else
220-
// POSIX: use flock(2)
221-
int try_lock_dir(const std::filesystem::path& dir) {
222-
std::error_code ec;
223-
std::filesystem::create_directories(dir, ec);
224-
auto lockPath = dir / ".lock";
225-
int fd = ::open(lockPath.c_str(), O_CREAT | O_RDWR | O_CLOEXEC, 0644);
226-
if (fd < 0) return -1;
227-
if (::flock(fd, LOCK_EX | LOCK_NB) != 0) {
228-
::close(fd);
229-
return -1;
230-
}
231-
return fd;
232-
}
233-
234-
void release_lock(int fd) {
235-
if (fd < 0) return;
236-
::flock(fd, LOCK_UN);
237-
::close(fd);
238-
}
239-
#endif
240-
241184
} // namespace
242185

243186
std::expected<void, std::string>
@@ -246,26 +189,11 @@ populate_from(const CacheKey& key,
246189
const DepArtifacts& arts)
247190
{
248191
auto cacheDir = key.dir();
249-
#if defined(_WIN32)
250-
HANDLE lockHandle = try_lock_dir(cacheDir);
251-
if (lockHandle == INVALID_HANDLE_VALUE) {
252-
return {};
253-
}
254-
struct LockGuard {
255-
HANDLE h;
256-
~LockGuard() { release_lock(h); }
257-
} guard{ lockHandle };
258-
#else
259-
int lockFd = try_lock_dir(cacheDir);
260-
if (lockFd < 0) {
192+
auto lock = mcpp::platform::fs::FileLock::try_acquire(cacheDir);
193+
if (!lock) {
261194
// Another writer holds the lock; treat as success (they'll do it).
262195
return {};
263196
}
264-
struct LockGuard {
265-
int fd;
266-
~LockGuard() { release_lock(fd); }
267-
} guard{ lockFd };
268-
#endif
269197

270198
auto cacheBmi = key.bmiDir();
271199
auto cacheObj = key.objDir();

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
}

0 commit comments

Comments
 (0)