Skip to content

Commit d871cfb

Browse files
committed
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.
1 parent e57be57 commit d871cfb

18 files changed

Lines changed: 971 additions & 494 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

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/ninja_backend.cppm

Lines changed: 2 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,8 @@ module;
1515
#include <cstdio>
1616
#include <cstdlib>
1717
#if defined(_WIN32)
18-
#include <windows.h>
1918
#define popen _popen
2019
#define pclose _pclose
21-
#elif defined(__APPLE__)
22-
#include <mach-o/dyld.h> // _NSGetExecutablePath
2320
#endif
2421

2522
export module mcpp.build.ninja;
@@ -33,6 +30,7 @@ import mcpp.dyndep;
3330
import mcpp.toolchain.detect;
3431
import mcpp.toolchain.registry;
3532
import mcpp.xlings;
33+
import mcpp.platform;
3634

3735
export namespace mcpp::build {
3836

@@ -119,27 +117,7 @@ bool dyndep_mode_enabled() {
119117
}
120118

121119
std::filesystem::path mcpp_exe_path() {
122-
std::error_code ec;
123-
#if defined(_WIN32)
124-
char buf[MAX_PATH];
125-
DWORD len = GetModuleFileNameA(NULL, buf, MAX_PATH);
126-
if (len > 0 && len < MAX_PATH) {
127-
auto p = std::filesystem::canonical(buf, ec);
128-
if (!ec) return p;
129-
}
130-
#elif defined(__APPLE__)
131-
char buf[4096];
132-
uint32_t size = sizeof(buf);
133-
if (_NSGetExecutablePath(buf, &size) == 0) {
134-
auto p = std::filesystem::canonical(buf, ec);
135-
if (!ec) return p;
136-
}
137-
#else
138-
auto p = std::filesystem::read_symlink("/proc/self/exe", ec);
139-
if (!ec)
140-
return p;
141-
#endif
142-
return "mcpp"; // fall back to PATH lookup
120+
return mcpp::platform::fs::self_exe_path();
143121
}
144122

145123
bool is_c_source(const std::filesystem::path& src) {

src/cli.cppm

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ import mcpp.publish.xpkg_emit;
3636
import mcpp.pack;
3737
import mcpp.config;
3838
import mcpp.xlings;
39+
import mcpp.platform;
3940
import mcpp.fetcher;
4041
import mcpp.pm.resolver; // PR-R4: extracted from cli.cppm
4142
import mcpp.pm.commands; // PR-R5: cmd_add / cmd_remove / cmd_update live here now
@@ -1016,16 +1017,7 @@ prepare_build(bool print_fingerprint,
10161017
return &*cfg_opt;
10171018
};
10181019

1019-
constexpr std::string_view kCurrentPlatform =
1020-
#if defined(__linux__)
1021-
"linux";
1022-
#elif defined(__APPLE__)
1023-
"macos";
1024-
#elif defined(_WIN32)
1025-
"windows";
1026-
#else
1027-
"unknown";
1028-
#endif
1020+
constexpr std::string_view kCurrentPlatform = mcpp::platform::name;
10291021

10301022
// M5.5: toolchain resolution priority:
10311023
// 0. --target X / --static, looked up in [target.<triple>]

src/config.cppm

Lines changed: 13 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -14,22 +14,15 @@
1414
// the directory tree exists and seeds .xlings.json if missing.
1515

1616
module;
17-
#include <cstdio>
1817
#include <cstdlib>
19-
#if defined(_WIN32)
20-
#include <windows.h>
21-
#define popen _popen
22-
#define pclose _pclose
23-
#elif defined(__APPLE__)
24-
#include <mach-o/dyld.h> // _NSGetExecutablePath
25-
#endif
2618

2719
export module mcpp.config;
2820

2921
import std;
3022
import mcpp.libs.toml;
3123
import mcpp.pm.index_spec;
3224
import mcpp.xlings;
25+
import mcpp.platform;
3326

3427
export namespace mcpp::config {
3528

@@ -180,22 +173,8 @@ std::filesystem::path home_dir() {
180173
return std::filesystem::path(e);
181174

182175
std::error_code ec;
183-
#if defined(_WIN32)
184-
char _exe_buf[MAX_PATH];
185-
DWORD _exe_len = GetModuleFileNameA(NULL, _exe_buf, MAX_PATH);
186-
std::filesystem::path exe;
187-
if (_exe_len > 0 && _exe_len < MAX_PATH)
188-
exe = std::filesystem::canonical(_exe_buf, ec);
189-
#elif defined(__APPLE__)
190-
char _exe_buf[4096];
191-
uint32_t _exe_size = sizeof(_exe_buf);
192-
std::filesystem::path exe;
193-
if (_NSGetExecutablePath(_exe_buf, &_exe_size) == 0)
194-
exe = std::filesystem::canonical(_exe_buf, ec);
195-
#else
196-
auto exe = std::filesystem::canonical("/proc/self/exe", ec);
197-
#endif
198-
if (!ec && exe.parent_path().filename() == "bin") {
176+
auto exe = mcpp::platform::fs::self_exe_path();
177+
if (exe.has_parent_path() && exe.parent_path().filename() == "bin") {
199178
// Dev builds emit binaries at target/<triple>/<fp>/bin/<exe>,
200179
// matching the bin/ shape. Any ancestor literally named
201180
// "target" disqualifies self-contained mode and falls through
@@ -367,14 +346,10 @@ acquire_xlings_binary(const std::filesystem::path& destBin, bool quiet)
367346
}
368347

369348
// 2. Copy from system (`which xlings`)
370-
#if defined(_WIN32)
371-
auto sys = run_capture("where xlings.exe 2>nul");
372-
#else
373-
auto sys = run_capture("command -v xlings 2>/dev/null");
374-
#endif
375-
if (sys) {
376-
std::string p = *sys;
377-
while (!p.empty() && (p.back() == '\n' || p.back() == '\r')) p.pop_back();
349+
auto xlings_name = std::string("xlings") + std::string(mcpp::platform::exe_suffix);
350+
auto sysXlings = mcpp::platform::fs::which(xlings_name);
351+
if (sysXlings) {
352+
std::string p = sysXlings->string();
378353
if (!p.empty() && std::filesystem::exists(p)) {
379354
std::filesystem::copy_file(p, destBin,
380355
std::filesystem::copy_options::overwrite_existing, ec);
@@ -453,11 +428,8 @@ std::expected<GlobalConfig, ConfigError> load_or_init(
453428
// <XLINGS_HOME>/bin/xlings, which satisfies xlings's own shim-
454429
// creation guard (`if fs::exists(homeDir/"bin"/"xlings")`),
455430
// making ensure_sandbox_xlings_binary() a no-op.
456-
#if defined(_WIN32)
457-
cfg.xlingsBinary = cfg.registryDir / "bin" / "xlings.exe";
458-
#else
459-
cfg.xlingsBinary = cfg.registryDir / "bin" / "xlings";
460-
#endif
431+
cfg.xlingsBinary = cfg.registryDir / "bin" /
432+
(std::string("xlings") + std::string(mcpp::platform::exe_suffix));
461433
cfg.bmiCacheDir = cfg.mcppHome / "bmi";
462434
cfg.metaCacheDir = cfg.mcppHome / "cache";
463435
cfg.logDir = cfg.mcppHome / "log";
@@ -552,16 +524,11 @@ std::expected<GlobalConfig, ConfigError> load_or_init(
552524
auto xbin = acquire_xlings_binary(cfg.xlingsBinary, quiet);
553525
if (!xbin) return std::unexpected(ConfigError{xbin.error()});
554526
} else if (cfg.xlingsBinaryMode == "system") {
555-
#if defined(_WIN32)
556-
auto sys = run_capture("where xlings.exe 2>nul");
557-
#else
558-
auto sys = run_capture("command -v xlings 2>/dev/null");
559-
#endif
560-
if (!sys || sys->empty())
527+
auto sysPath = mcpp::platform::fs::which(
528+
std::string("xlings") + std::string(mcpp::platform::exe_suffix));
529+
if (!sysPath)
561530
return std::unexpected(ConfigError{"system xlings not found in PATH"});
562-
std::string p = *sys;
563-
while (!p.empty() && (p.back() == '\n' || p.back() == '\r')) p.pop_back();
564-
cfg.xlingsBinary = p;
531+
cfg.xlingsBinary = *sysPath;
565532
} else {
566533
cfg.xlingsBinary = cfg.xlingsBinaryMode;
567534
if (!std::filesystem::exists(cfg.xlingsBinary))

0 commit comments

Comments
 (0)