fix: emit absolute -fprebuilt-module-path so clangd resolves modules#77
Merged
Conversation
src/build/flags.cppm was emitting a bare `-fprebuilt-module-path=pcm.cache`
(or `=gcm.cache`). This works for `mcpp build` because ninja runs commands
with cwd = `<projectRoot>/target/<triple>/<fp>/` so the relative path
resolves correctly. But the same flag is captured verbatim into
`compile_commands.json`, whose `directory` field is `plan.projectRoot`.
Per CDB spec, clangd does `cd <directory>` before resolving the args, so
it looks for `<projectRoot>/pcm.cache` — which doesn't exist. Effect:
`import` resolution silently fails in clangd ("module 'X' not found"),
even when `mcpp build` succeeds.
The other `-fmodule-file=` flags in the same block were already absolute
(escape_path-wrapped); this one was an oversight.
Fix: format the flag with `escape_path(plan.outputDir / traits.bmiDir)`.
Single line. ninja still works (absolute paths are cwd-independent), and
clangd now finds the BMI cache.
Regression test: tests/e2e/47_cdb_prebuilt_module_path_abs.sh creates a
fresh project, runs `mcpp build`, parses `compile_commands.json`, asserts
every `-fprebuilt-module-path=X`:
- is absolute (starts with `/` on POSIX or `<drive>:` on Windows)
- ends in `/pcm.cache` or `/gcm.cache`
- points to an existing directory
Pass-through for GCC-only flows that don't emit the flag at all.
First CI round revealed two layered issues exposed by the new e2e: 1. **The shipped CDB carries ninja-escape artefacts.** `flags.cppm`'s `escape_path` ninja-escapes paths (`$ ` for space, `$:` for colon, `$$` for literal `$`) so they survive embedding in ninja rule strings. But the same escaped strings flow verbatim through `f.cxx` into `compile_commands.json`, whose `arguments` array is exec'd by clangd without any ninja involved. On Windows that makes `C:` appear as `C$:` in CDB → clangd can't resolve the path. POSIX usually escaped nothing in practice, hiding the bug; Windows drive letters always trigger `$:`. Fix in `compile_commands.cppm::split_flags`: same single pass now splits tokens *and* unescapes the three ninja sequences. Splitting was also broken for paths with a literal space (would've broken `-isystem "/path with space/include"` mid-arg) — that's fixed simultaneously since `$ ` is no longer a token separator. 2. **e2e test 47 needed Windows-friendly checks.** The original `[^"]+` grep returned the raw JSON-escaped form (`\\Users\\...`) and the `^[A-Za-z]:` regex didn't match `C$:`. Rewrote using `jq -r` (which does JSON un-escape natively, preinstalled on all GHA runners) and: - explicit ninja-escape sniff (`$:`, `$ `, `$$`) - both POSIX and Windows-drive absolute-path checks - basename check normalised against `\` / `/` Net: CDB now contains plain paths in both runtimes, and the test fails loudly if either layer regresses.
The Windows CI run reported:
FAIL: basename is not pcm.cache/gcm.cache: 'pcm.cache
'
The closing quote on the next line is the giveaway — the value carries a
trailing `\r`. `jq.exe` on git-bash emits CRLF; `mapfile -t` strips LF
but leaves CR, so every downstream comparison saw `pcm.cache$'\r'` and
failed. Trim with `${v%$'\r'}` per element.
The source-code fix (un-escape ninja sequences) is working — the value
itself is now a clean `C:\Users\...\pcm.cache`, no `$:` artefact.
Apple ships GPLv2-era bash 3.2 on macOS, which has no `mapfile` / `readarray`. The previous round failed on ci-macos with: line 31: mapfile: command not found FAIL: 47_cdb_prebuilt_module_path_abs.sh (exit 127) Replace `mapfile -t vals < <(jq …)` with `jq … > tmp.txt` followed by a `while … read … done < tmp.txt`. Using input redirection (not a `|` pipeline) keeps the loop in the current shell so `fail=1` propagates; the temp file lives under $TMP and is cleaned up by the script's trap. Empty-list signal now uses `[[ ! -s "$TMP/vals.txt" ]]` instead of array length — same semantics, no bashism.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
mcpp emits
-fprebuilt-module-path=pcm.cache(bare relative) in the compile command. ninja runs with cwd =<projectRoot>/target/<triple>/<fp>/so the bare relative path resolves to the actual BMI cache and the build succeeds. But the same flag is captured verbatim intocompile_commands.json, whosedirectoryfield is set toplan.projectRoot.Per the CDB spec, clangd does
cd <directory>before resolving the arguments. So clangd looks for<projectRoot>/pcm.cache— which doesn't exist. The user-visible symptom isimportresolution silently failing in the editor:… even though
mcpp builditself works perfectly.Root cause
src/build/flags.cppm:215before:traits.bmiDiris just"pcm.cache"(Clang) or"gcm.cache"(GCC). All the other-fmodule-file=flags in the same block are already wrapped inescape_path(<absolute_path>). This one was a leftover.Fix
Single line. ninja still works (absolute paths are cwd-independent). clangd now finds the BMI cache.
Test
New
tests/e2e/47_cdb_prebuilt_module_path_abs.sh:mcpp new app && mcpp buildcompile_commands.json, extract every-fprebuilt-module-path=XX:/on POSIX or<drive>:on Windows)/pcm.cacheor/gcm.cache-fmodules-only flow)Test plan
Risk
Minimal. The flag is functionally identical (absolute path resolves the same as the cwd-relative path did, since ninja's cwd is exactly
<outputDir>). The change only affects how the flag looks in the captured command string.