From 27c2b511d9aec6095a1b94c16fce2e41fa73db30 Mon Sep 17 00:00:00 2001 From: Prakhar Maheshwari Date: Sun, 30 Aug 2026 18:00:37 +0530 Subject: [PATCH] fix(ebuild): escape ninja-special characters in generated build paths ## Problem `ebuild build` fails for any project whose path contains a space -- on Windows that includes most home directories: ninja: error: build\build.ninja:24: expected build command name build C:\Users\Prakhar Maheshwari\AppData\...\obj\app\main.o: cc main.c ^ near here Paths are interpolated into build statements as-is. Ninja ends the output list at the first unescaped `:` and splits on unescaped spaces, so neither a Windows drive letter nor a directory with a space survives. The failure mode is worse than the error above. Escaping only the colon makes the manifest parse, and `ninja -t targets all` then reports: C:/Users/Prakhar: touchit Maheshwari/AppData/.../dir: touchit with: touchit space/out.o: touchit Four targets where one was meant, no error, no warning. The build then fails later with a message pointing nowhere near the cause, or quietly builds something other than what was asked for. ## Fix Add `escape_ninja_path()` and apply it to every path written into a build statement: compile edge outputs and sources, link inputs, dependency archives, and executable/library outputs. `$` is replaced first. The space and colon replacements introduce `$` characters, and escaping those again would yield `$$ ` -- a literal dollar followed by a separator -- rather than an escaped space. This matches what CMake's Ninja generator does (`cmGlobalNinjaGenerator::EncodePath`). Deliberately *not* escaped: - `compile_commands.json`. It is JSON consumed by clang tooling, not a Ninja manifest; `$:` in a path there would be corruption. A test pins this. - Command lines. Verified with `ninja -v` that Ninja shell-quotes `$in`/`$out` itself, so an escaped path still reaches the compiler as one argument: python argv.py -o "C:\Users\Prakhar Maheshwari\...\out.o" ARG[0]=-o ARG[1]=C:\Users\Prakhar Maheshwari\...\out.o ## Testing The escaping rules were established empirically against ninja 1.13, not from the manual: unescaped and space-only-escaped manifests fail to parse, colon-only escaping parses into four wrong targets, and only colon + space escaping yields the single intended target. Six tests added as `TestNinjaPathEscaping`: - three on the helper -- `$`/space/`:` are escaped, `$` is escaped before the others, ordinary paths are untouched; - `test_build_dir_with_a_space_is_escaped_in_the_manifest` -- no raw space survives in a build statement's output list; - `test_ninja_parses_a_spacey_path_as_one_target` -- runs real `ninja -t targets all` and asserts 2 targets, not one per path fragment. This is the check a string assertion cannot make: an unescaped path parses cleanly and is simply wrong; - `test_escaping_does_not_leak_into_compile_commands` -- guards the boundary above. Verified the tests detect the bug: with escaping replaced by a passthrough, 5 of the 6 fail, along with the end-to-end `test_editing_a_header_triggers_a_rebuild`. With the fix, all pass. `test_editing_a_header_triggers_a_rebuild` now passes for the first time. It had been skipping because the declared `ninja` dependency was absent from the environment, and failing once installed -- its `tmp_path` contains a space. Full suite: 212 passed, 5 failed (was 6). The 5 remaining are pre-existing and unrelated: 3 initramfs, 2 shared-library link rule. ## Known, not addressed here Flag values are still interpolated unquoted: `cflags = ... -I` and `cc = `. A compiler path or include directory containing a space will split into two shell arguments, and a `$` in one would be read as a Ninja variable reference. That is shell quoting of user-supplied values rather than manifest-path escaping -- a different mechanism, sent separately. Signed-off-by: Prakhar Maheshwari Signed-off-by: Prakhar Maheshwari --- CHANGELOG.md | 7 ++ ebuild/build/ninja_backend.py | 31 +++++++-- tests/unit/test_ninja_backend.py | 114 ++++++++++++++++++++++++++++++- 3 files changed, 146 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 46d39f08..9f744dd1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,13 @@ ## [Unreleased] ### Fixed +- **A path containing a space produced a silently wrong `build.ninja`.** Paths + were written into build statements unescaped, but Ninja ends the output list + at the first unescaped `:` and splits on unescaped spaces. A build directory + under `C:\Users\Jane Doe\` parsed into four targets instead of one, so + `ebuild build` failed with `expected build command name` or built the wrong + thing. `$`, spaces and `:` are now escaped in generated build statements + (`ebuild/build/ninja_backend.py`). - **`NinjaBackend` could not generate anything.** `_object_path()` is called by both `_write_ninja()` and `_write_compile_commands()`, but the method itself was dropped in a merge, so every `generate()` raised diff --git a/ebuild/build/ninja_backend.py b/ebuild/build/ninja_backend.py index 4ef73ce4..3cffcef5 100644 --- a/ebuild/build/ninja_backend.py +++ b/ebuild/build/ninja_backend.py @@ -30,6 +30,25 @@ class PackagePaths: "-fno-pie", "-fno-PIE"} +def escape_ninja_path(path) -> str: + """Escape a path for use inside a Ninja build statement. + + Ninja's lexer ends the output list at the first unescaped ``:`` and splits + on unescaped spaces, so a Windows drive letter or a directory containing a + space does not raise an error -- it silently parses into several wrong + targets. ``C:\\Users\\Jane Doe\\build\\app.o`` becomes four targets + (``C``, ``...\\Jane``, ``Doe``, ``build\\app.o``) instead of one. + + ``$`` is replaced first: the space and colon replacements introduce ``$`` + characters that must not be escaped again. + + Only the manifest needs this. Ninja shell-quotes ``$in``/``$out`` itself + when it builds the command line, so an escaped path still reaches the + compiler as a single argument. + """ + return str(path).replace("$", "$$").replace(" ", "$ ").replace(":", "$:") + + class NinjaBackend: """Generate build.ninja from a ProjectConfig and resolved toolchain. @@ -156,10 +175,10 @@ def _write_ninja(self) -> None: obj_files = [] for src in target.sources: - obj = str(self._object_path(target, src)) + obj = escape_ninja_path(self._object_path(target, src)) obj_files.append(obj) lines.append( - f"build {obj}: cc {src}" + f"build {obj}: cc {escape_ninja_path(src)}" ) if cflags: lines.append(f" cflags = {' '.join(cflags)}") @@ -181,10 +200,12 @@ def _write_ninja(self) -> None: for dep_name in target.depends: for dep_target in self.config.targets: if dep_target.name == dep_name and dep_target.target_type == "static_library": - dep_archives.append(str(self.build_dir / f"lib{dep_name}.a")) + dep_archives.append( + escape_ninja_path(self.build_dir / f"lib{dep_name}.a") + ) link_inputs = obj_files + dep_archives - out = str(self.build_dir / target.name) + out = escape_ninja_path(self.build_dir / target.name) lines.append( f"build {out}: link {' '.join(link_inputs)}" ) @@ -201,7 +222,7 @@ def _write_ninja(self) -> None: ext = ".dylib" else: ext = ".so" - out = str(self.build_dir / f"lib{target.name}{ext}") + out = escape_ninja_path(self.build_dir / f"lib{target.name}{ext}") if target.target_type == "static_library": lines.append(f"build {out}: ar_rule {' '.join(obj_files)}") diff --git a/tests/unit/test_ninja_backend.py b/tests/unit/test_ninja_backend.py index d5e09ac7..fd136d53 100644 --- a/tests/unit/test_ninja_backend.py +++ b/tests/unit/test_ninja_backend.py @@ -13,7 +13,7 @@ import pytest -from ebuild.build.ninja_backend import NinjaBackend +from ebuild.build.ninja_backend import NinjaBackend, escape_ninja_path from ebuild.build.toolchain import ResolvedToolchain from ebuild.core.config import ProjectConfig, TargetConfig @@ -193,5 +193,117 @@ def test_compile_commands_distinguishes_shared_source_entries(self, tmp_path): assert len(set(objects)) == 2, f"entries name the same object: {objects}" +@pytest.mark.ebuild +class TestNinjaPathEscaping: + """Paths in build statements must be escaped for Ninja's lexer. + + Ninja ends the output list at the first unescaped ``:`` and splits on + unescaped spaces. Neither is an error -- a Windows drive letter or a + directory with a space parses into several wrong targets instead of one + right one, so the manifest either fails to build the requested target or + fails much later with a confusing message. + """ + + def test_escapes_space_colon_and_dollar(self): + assert escape_ninja_path("a b") == "a$ b" + assert escape_ninja_path("C:/x") == "C$:/x" + assert escape_ninja_path("a$b") == "a$$b" + + def test_dollar_is_escaped_before_the_others(self): + """Order matters. Escaping the space first would leave a ``$`` that + the dollar pass then doubles, yielding ``$$ `` -- a literal dollar + followed by a separator rather than an escaped space.""" + assert escape_ninja_path("a b:c") == "a$ b$:c" + + def test_leaves_ordinary_paths_untouched(self): + assert escape_ninja_path("src/main.c") == "src/main.c" + + @staticmethod + def _config_in(build_dir, source_dir): + return ProjectConfig( + name="spacey", + version="1.0.0", + targets=[ + TargetConfig( + name="app", + target_type="executable", + sources=["main.c"], + ) + ], + source_dir=source_dir, + ) + + def test_build_dir_with_a_space_is_escaped_in_the_manifest(self, tmp_path): + build_dir = tmp_path / "dir with space" / "_build" + NinjaBackend( + self._config_in(build_dir, tmp_path), build_dir, ResolvedToolchain() + ).generate() + + manifest = (build_dir / "build.ninja").read_text(encoding="utf-8") + build_lines = [ln for ln in manifest.splitlines() if ln.startswith("build ")] + assert build_lines + + for line in build_lines: + # The path is escaped, so no raw space survives before the rule + # separator, and the drive colon does not terminate the outputs. + outputs = line[len("build "):].split(": ", 1)[0] + assert " " not in outputs.replace("$ ", ""), line + assert "$ " in outputs, line + + def test_ninja_parses_a_spacey_path_as_one_target(self, tmp_path): + """The check that matters: real ninja must resolve one target, not four. + + A string assertion cannot catch this -- an unescaped path parses + cleanly and is simply wrong. + """ + pytest.importorskip("ninja", reason="ninja package not installed") + + source_dir = tmp_path / "dir with space" + source_dir.mkdir() + (source_dir / "main.c").write_text("int main(void){return 0;}\n", encoding="utf-8") + + build_dir = source_dir / "_build" + NinjaBackend( + self._config_in(build_dir, source_dir), build_dir, ResolvedToolchain() + ).generate() + + result = subprocess.run( + [ + sys.executable, "-m", "ninja", + "-f", str(build_dir / "build.ninja"), + "-t", "targets", "all", + ], + cwd=str(source_dir), + capture_output=True, + text=True, + ) + assert result.returncode == 0, ( + f"ninja rejected the manifest:\n{result.stdout}\n{result.stderr}" + ) + + # One compile edge and one link edge -- not one per path fragment. + targets = [ln for ln in result.stdout.strip().splitlines() if ln.strip()] + assert len(targets) == 2, f"expected 2 targets, got {targets}" + assert all("dir with space" in t for t in targets), targets + + def test_escaping_does_not_leak_into_compile_commands(self, tmp_path): + """compile_commands.json is JSON consumed by clang tooling, not a + Ninja manifest. A ``$:`` or ``$ `` there would be a corrupt path.""" + source_dir = tmp_path / "dir with space" + source_dir.mkdir() + build_dir = source_dir / "_build" + NinjaBackend( + self._config_in(build_dir, source_dir), build_dir, ResolvedToolchain() + ).generate() + + cc_data = json.loads( + (build_dir / "compile_commands.json").read_text(encoding="utf-8") + ) + assert cc_data + for entry in cc_data: + assert "$:" not in entry["command"] + assert "$ " not in entry["command"] + + if __name__ == "__main__": unittest.main()