diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cc3920f..536e83a0 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 ad8bb12b..f3cfc76d 100644 --- a/ebuild/build/ninja_backend.py +++ b/ebuild/build/ninja_backend.py @@ -31,42 +31,23 @@ class PackagePaths: "-fno-pie", "-fno-PIE"} -def _ninja_path(path) -> str: - """Escape *path* for use in a Ninja build statement. +def escape_ninja_path(path) -> str: + """Escape a path for use inside a Ninja build statement. - Ninja splits build statements on unescaped spaces and colons, so a Windows - absolute path writes a drive letter that Ninja reads as the output/rule - separator: + 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. - build C:\\...\\main.o: cc main.c - ^ "expected build command name" + ``$`` is replaced first: the space and colon replacements introduce ``$`` + characters that must not be escaped again. - `$` is escaped first so the escapes introduced below are not re-escaped. - Only build statements need this; variable values (cflags, ldflags) are read - to end of line and must not be escaped, or the flags reach the compiler - mangled. + 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. """ - text = str(path) - return text.replace("$", "$$").replace(":", "$:").replace(" ", "$ ") - - -def _ninja_path(path) -> str: - """Escape *path* for use in a Ninja build statement. - - Ninja splits build statements on unescaped spaces and colons, so a Windows - absolute path writes a drive letter that Ninja reads as the output/rule - separator: - - build C:\\...\\main.o: cc main.c - ^ "expected build command name" - - `$` is escaped first so the escapes introduced below are not re-escaped. - Only build statements need this; variable values (cflags, ldflags) are read - to end of line and must not be escaped, or the flags reach the compiler - mangled. - """ - text = str(path) - return text.replace("$", "$$").replace(":", "$:").replace(" ", "$ ") + return str(path).replace("$", "$$").replace(" ", "$ ").replace(":", "$:") class NinjaBackend: @@ -207,10 +188,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 {_ninja_path(obj)}: cc {_ninja_path(src)}" + f"build {obj}: cc {escape_ninja_path(src)}" ) if cflags: lines.append(f" cflags = {' '.join(cflags)}") @@ -232,10 +213,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 {_ninja_path(out)}: link " f"{' '.join(_ninja_path(x) for x in link_inputs)}" ) @@ -252,7 +235,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 {_ninja_path(out)}: ar_rule " f"{' '.join(_ninja_path(x) for x in obj_files)}") diff --git a/tests/unit/test_ninja_backend.py b/tests/unit/test_ninja_backend.py index b7375ede..e3c02b5d 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 @@ -244,5 +244,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()