From 9d89b58b777bbba7e5a0f09f63f535ce4b51d2aa Mon Sep 17 00:00:00 2001 From: Matthieu Patou Date: Tue, 5 May 2026 15:59:50 +0000 Subject: [PATCH 1/7] Better parse custom commands used for generating files Sometime you might have multiple command that generates different files in the same group of command and previous implementation was not picking all the commands leaving some files ungenerated. --- build.py | 207 +++++++++++++++++++++++++++++++++++++ build_visitor.py | 10 +- ninjabuild.py | 96 +++++++++++------ test/test_build.py | 145 ++++++++++++++++++++++++++ test/test_build_visitor.py | 30 +++++- test/test_ninjabuild.py | 124 ++++++++++++++++++++++ 6 files changed, 579 insertions(+), 33 deletions(-) diff --git a/build.py b/build.py index 6598e53..eae3503 100644 --- a/build.py +++ b/build.py @@ -452,6 +452,13 @@ def __init__( self.vars: Dict[str, str] = {} + def detach(self) -> None: + for target in list(self._inputs) + list(self.depends): + target.usedbybuilds = [build for build in target.usedbybuilds if build is not self] + for output in self.outputs: + if output.producedby is self: + output.producedby = None + def getInputs(self) -> List[BuildTarget]: return self._inputs @@ -1677,6 +1684,206 @@ def getCoreCommand(self) -> Optional[Tuple[str, Optional[str]]]: return (cmd, runDir) return None + def getGeneratorCommands(self) -> List[Tuple[str, Optional[str]]]: + commands = [] + for group in self.getGeneratorCommandGroups(): + commands.extend(group) + return commands + + def getGeneratorCommandsForTarget( + self, target: BuildTarget + ) -> List[Tuple[str, Optional[str]]]: + command_groups = self.getGeneratorCommandGroups() + if len(command_groups) == 0: + return [] + + workDir = self.vars.get("cmake_ninja_workdir", "") + for command_group in command_groups: + if self._commandGroupMentionsTarget(command_group, target, workDir): + return command_group + return [] + + def getGeneratorCommandGroups(self) -> List[List[Tuple[str, Optional[str]]]]: + command = self.rulename.vars.get("command") + if command is None: + return [] + command = self._resolveName(command, ["in", "out", "TARGET_FILE"]) + workDir = self.vars.get("cmake_ninja_workdir", "") + runDir = None + command_groups = [] + pending_setup_commands = [] + + for raw_cmd in command.split("&&"): + cmd = raw_cmd.strip() + if cmd == "": + continue + if cmd.startswith("cd "): + runDir = self._getRunDir(cmd[3:].strip(), workDir) + continue + if self._isExcludedGeneratorCommand(cmd): + pending_setup_commands = [] + continue + if self._isGeneratorSetupCommand(cmd): + pending_setup_commands.append((cmd, runDir)) + continue + if not self._isGeneratorCommand(cmd, workDir): + continue + command_group = pending_setup_commands + [(cmd, runDir)] + pending_setup_commands = [] + command_groups.append(command_group) + return command_groups + + def splitByGeneratorCommands(self) -> List["Build"]: + if self.rulename.name != "CUSTOM_COMMAND" or len(self.outputs) <= 1: + return [self] + + command_groups = self.getGeneratorCommandGroups() + if len(command_groups) <= 1: + return [self] + + workDir = self.vars.get("cmake_ninja_workdir", "") + remaining_outputs = set(self.outputs) + assignments: List[Tuple[List[Tuple[str, Optional[str]]], List[BuildTarget]]] = [] + + for command_group in command_groups: + candidate_outputs = [ + output + for output in self.outputs + if output in remaining_outputs + and self._commandGroupMentionsTarget(command_group, output, workDir) + ] + if len(candidate_outputs) == 0: + continue + for output in candidate_outputs: + remaining_outputs.remove(output) + assignments.append((command_group, candidate_outputs)) + + if len(remaining_outputs) != 0 or len(assignments) <= 1: + return [self] + + split_builds = [] + for command_group, outputs in assignments: + split_rule = Rule(self.rulename.name) + split_rule.vars = dict(self.rulename.vars) + split_rule.vars["command"] = self._formatGeneratorCommandGroup( + command_group, workDir + ) + + split_inputs = list(self._inputs) + for output in self.outputs: + if output in outputs: + continue + if ( + self._commandGroupMentionsTarget(command_group, output, workDir) + and output not in split_inputs + ): + split_inputs.append(output) + + split_build = Build(outputs, split_rule, split_inputs, self.depends) + split_build.vars.update(self.vars) + split_build.vars["COMMAND"] = split_rule.vars["command"] + split_builds.append(split_build) + + return split_builds + + def _getRunDir(self, path: str, workDir: str) -> str: + if workDir != "": + normalizedWorkDir = workDir + if not normalizedWorkDir.endswith(os.path.sep): + normalizedWorkDir = f"{normalizedWorkDir}{os.path.sep}" + if path.startswith(normalizedWorkDir): + return path[len(normalizedWorkDir) :] + if path == workDir.rstrip(os.path.sep): + return "" + return path + + def _isGeneratorSetupCommand(self, cmd: str) -> bool: + if cmd.startswith("mkdir "): + return True + return "cmake -E make_directory" in cmd + + def _isExcludedGeneratorCommand(self, cmd: str) -> bool: + if "/bin/cmake" in cmd: + return True + try: + parts = shlex.split(cmd) + except ValueError: + return False + if len(parts) == 0: + return False + return os.path.basename(parts[0]) == "cp" + + def _isGeneratorCommand(self, cmd: str, workDir: str) -> bool: + if self.rulename.name != "CUSTOM_COMMAND": + return False + for target in list(self._inputs) + list(self.outputs): + if self._commandMentionsTarget(cmd, target, workDir): + return True + return False + + def _commandGroupMentionsTarget( + self, + command_group: List[Tuple[str, Optional[str]]], + target: BuildTarget, + workDir: str, + ) -> bool: + return any( + self._commandMentionsTarget(cmd, target, workDir) + for cmd, _ in command_group + ) + + def _commandMentionsTarget(self, cmd: str, target: BuildTarget, workDir: str) -> bool: + candidates = set(self._targetCommandCandidates(target, workDir)) + return any(value in candidates for value in self._commandTargetValues(cmd)) + + def _commandTargetValues(self, cmd: str) -> List[str]: + try: + parts = shlex.split(cmd) + except ValueError: + parts = cmd.split() + + values = [] + for part in parts: + values.append(part) + if "=" in part: + values.append(part.split("=", 1)[1]) + return values + + def _targetCommandCandidates(self, target: BuildTarget, workDir: str) -> List[str]: + candidates = [target.name, target.shortName] + if workDir != "": + normalizedWorkDir = workDir + if not normalizedWorkDir.endswith(os.path.sep): + normalizedWorkDir = f"{normalizedWorkDir}{os.path.sep}" + if target.name.startswith(normalizedWorkDir): + candidates.append(target.name[len(normalizedWorkDir) :]) + for candidate in list(candidates): + if not candidate.startswith(os.path.sep): + candidates.append(f"{normalizedWorkDir}{candidate}") + return candidates + + def _formatGeneratorCommandGroup( + self, + command_group: List[Tuple[str, Optional[str]]], + workDir: str, + ) -> str: + parts = [] + current_run_dir = None + for cmd, runDir in command_group: + if runDir != current_run_dir: + current_run_dir = runDir + if runDir is not None: + parts.append(f"cd {self._expandRunDir(runDir, workDir)}") + parts.append(cmd) + return " && ".join(parts) + + def _expandRunDir(self, runDir: str, workDir: str) -> str: + if runDir == "": + return workDir.rstrip(os.path.sep) + if os.path.isabs(runDir) or workDir == "": + return runDir + return os.path.join(workDir, runDir) + def _resolveName(self, name: str, exceptVars: Optional[List[str]] = None) -> str: regex = r"\$\{?([\w+]+)\}?" diff --git a/build_visitor.py b/build_visitor.py index 01fd9e6..71c31f7 100644 --- a/build_visitor.py +++ b/build_visitor.py @@ -32,7 +32,15 @@ def visitProduced( f"Skipping non phony top level target that is not used by anything: {el}" ) return False - rawCmd = build.getCoreCommand() + rawCmd = None + if build.rulename.name == "CUSTOM_COMMAND": + generator_commands = build.getGeneratorCommandsForTarget(el) + if len(generator_commands) > 0: + rawCmd = generator_commands[-1] + else: + rawCmd = build.getCoreCommand() + else: + rawCmd = build.getCoreCommand() if rawCmd is None and build.rulename.name != "CUSTOM_COMMAND": logging.warning(f"{el} has no valid command {build.getInputs()}") diff --git a/ninjabuild.py b/ninjabuild.py index cd3ee46..ddf08c3 100644 --- a/ninjabuild.py +++ b/ninjabuild.py @@ -2,6 +2,7 @@ import logging import os import re +import shlex import shutil import subprocess import sys @@ -422,7 +423,10 @@ def _handleBuild(self, arr: List[str], vars: Dict[str, str]): build.vars.update(build.rulename.vars) build.vars.update(vars) - self.buildEdges.append(build) + split_builds = build.splitByGeneratorCommands() + if len(split_builds) != 1 or split_builds[0] is not build: + build.detach() + self.buildEdges.extend(split_builds) def handleVariable(self, name: str, value: str): self.vars[self.currentContext][name] = value @@ -443,13 +447,18 @@ def executeGenerator(self, build: Build, target: BuildTarget): cacheDirBase = f"{os.environ['HOME']}/.cache/ninja2bazel/{subDir}" os.makedirs(cacheDirBase, exist_ok=True) - coreRet = build.getCoreCommand() outputs = set() workDir = build.vars.get("cmake_ninja_workdir", "") for o in build.outputs: outputs.add(o.name.replace(workDir, "")) - if coreRet is None: + generatorCommands = build.getGeneratorCommandsForTarget(target) + if len(generatorCommands) == 0: + coreRet = build.getCoreCommand() + if coreRet is not None: + generatorCommands = [coreRet] + + if len(generatorCommands) == 0: if " cp " in build.vars.get( "COMMAND", "" ) or "/bin/cmake" in build.vars.get("COMMAND", ""): @@ -457,17 +466,18 @@ def executeGenerator(self, build: Build, target: BuildTarget): f'Command for {target.name}: {build.vars.get("COMMAND")} is not a "core" one' ) return - cmd, runDir = coreRet - cmd = cmd.strip() - if cmd.startswith("cp "): + if all(cmd.strip().startswith("cp ") for cmd, _ in generatorCommands): return - cmd = cmd.strip() os.environ["PYTHONPATH"] = ( os.environ.get("PYTHONPATH", "") + ":" + self.codeRootDir ) - exe = cmd.split(" ") - if exe[0].endswith("/mono"): + generatorCommands = [ + (self._normalizeGeneratorCommand(cmd), runDir) + for cmd, runDir in generatorCommands + ] + exes = [shlex.split(cmd)[0] for cmd, _ in generatorCommands if cmd.strip()] + if len(exes) > 0 and all(exe.endswith("/mono") for exe in exes): for f in outputs: self.generatedFiles[f] = ( None, @@ -477,47 +487,71 @@ def executeGenerator(self, build: Build, target: BuildTarget): # skip mono all togother # Should generate empty files return - if exe[0].endswith("/protoc"): + if len(exes) > 0 and all(exe.endswith("/protoc") for exe in exes): for f in outputs: self.generatedFiles[f] = (build, None) # Should generate empty files # skip protoc return - if exe[0].endswith(".py"): - cmd = f"python3 {cmd}" + cmd = self._makeGeneratorShellCommand(generatorCommands) + if cmd == "": + return if (cmd, workDir) in self.ran: return else: self.ran.add((cmd, workDir)) - cwd = os.getcwd() - os.chdir(tempDir) - - if runDir is not None: - cmd = f"mkdir -p {runDir} && cd {runDir} && {cmd}" - sha1cmd = hashlib.sha1() sha1cmd.update(cmd.encode()) sha1 = sha1cmd.hexdigest() # We want to hash first before replacing workdir by tempdir - cmd = re.sub(rf"{workDir}", f"{tempDir}/", cmd) + if workDir != "": + cmd = re.sub(rf"{workDir}", f"{tempDir}/", cmd) cacheDir = f"{cacheDirBase}/{sha1}" - if os.path.exists(cacheDir): - logging.info(f"Using cache for {cmd} SHA1:{sha1}") - _copyFilesBackNForth(cacheDir, tempDir) - else: - logging.info(f"Running in {tempDir} {cmd} SHA1:{sha1}") - res = subprocess.run(cmd, shell=True) - if res.returncode != 0: - logging.warn(f"Got an exception when trying to run {cmd} in {tempDir}") - return + cwd = os.getcwd() + os.chdir(tempDir) + try: + for output in outputs: + outputDir = os.path.dirname(output) + if outputDir != "": + os.makedirs(outputDir, exist_ok=True) + if os.path.exists(cacheDir): + logging.info(f"Using cache for {cmd} SHA1:{sha1}") + _copyFilesBackNForth(cacheDir, tempDir) + else: + logging.info(f"Running in {tempDir} {cmd} SHA1:{sha1}") + res = subprocess.run(cmd, shell=True) + if res.returncode != 0: + logging.warn(f"Got an exception when trying to run {cmd} in {tempDir}") + return + + _copyFilesBackNForth(tempDir, cacheDir) + finally: + os.chdir(cwd) + return tempDir - _copyFilesBackNForth(tempDir, cacheDir) + def _normalizeGeneratorCommand(self, cmd: str) -> str: + parts = shlex.split(cmd) + if len(parts) > 0 and parts[0].endswith(".py"): + return f"python3 {cmd}" + return cmd - os.chdir(cwd) - return tempDir + def _makeGeneratorShellCommand( + self, generatorCommands: List[Tuple[str, Optional[str]]] + ) -> str: + parts = [] + for cmd, runDir in generatorCommands: + cmd = cmd.strip() + if cmd == "": + continue + if runDir is not None and runDir != "": + quotedRunDir = shlex.quote(runDir) + parts.append(f"(mkdir -p {quotedRunDir} && cd {quotedRunDir} && {cmd})") + else: + parts.append(f"({cmd})") + return " && ".join(parts) def getCCImportForExternalDep(self, target: BuildTarget) -> Optional[BuildTarget]: logging.debug(f"Checking {target.name} as part of CCimport") diff --git a/test/test_build.py b/test/test_build.py index adaede2..9daa775 100644 --- a/test/test_build.py +++ b/test/test_build.py @@ -172,6 +172,151 @@ def test_get_core_command_extracts_run_directory(self) -> None: self.assertTrue(cmd.strip().startswith("gcc -c")) self.assertEqual(run_dir, "/build") + def test_get_generator_commands_extracts_all_commands_with_run_directory(self) -> None: + with tempfile.TemporaryDirectory() as td: + tmp_path = Path(td) + work_dir = tmp_path / "build" + script = tmp_path / "protocol_version.py" + source = tmp_path / "ProtocolVersions.cmake" + template = tmp_path / "ProtocolVersion.h.template" + out = BuildTarget("flow/include/flow/ProtocolVersion.h", ("ProtocolVersion.h", None)) + rule = Rule("CUSTOM_COMMAND") + build = Build( + [out], + rule, + [ + BuildTarget(str(script), (script.name, None)), + BuildTarget(str(template), (template.name, None)), + BuildTarget(str(source), (source.name, None)), + ], + [], + ) + build.vars["cmake_ninja_workdir"] = f"{work_dir}/" + rule.vars["command"] = ( + f"cd {work_dir}/flow && " + f"python3 {script} --source {source} --generator cpp " + f"--output {work_dir}/flow/include/flow/ProtocolVersion.h && " + f"python3 {script} --source {source} --generator java " + f"--output {work_dir}/flow/include/flow/ProtocolVersion.java && " + f"python3 {script} --source {source} --generator python " + f"--output {work_dir}/flow/include/flow/protocol_version.py" + ) + + commands = build.getGeneratorCommands() + + self.assertEqual(3, len(commands)) + self.assertEqual(["flow", "flow", "flow"], [run_dir for _, run_dir in commands]) + self.assertTrue(all(str(script) in cmd for cmd, _ in commands)) + + def test_get_generator_commands_for_target_selects_matching_command(self) -> None: + work_dir = "/tmp/build/" + out_a = BuildTarget("generated/a.txt", ("a.txt", None)) + out_b = BuildTarget("generated/b.txt", ("b.txt", None)) + rule = Rule("CUSTOM_COMMAND") + build = Build([out_a, out_b], rule, [], []) + build.vars["cmake_ninja_workdir"] = work_dir + rule.vars["command"] = ( + "python3 gen.py --output /tmp/build/generated/a.txt && " + "python3 gen.py --generated-file=/tmp/build/generated/b.txt" + ) + + commands = build.getGeneratorCommandsForTarget(out_b) + + self.assertEqual( + [("python3 gen.py --generated-file=/tmp/build/generated/b.txt", None)], + commands, + ) + + def test_get_generator_commands_for_target_ignores_template_basename(self) -> None: + work_dir = "/tmp/build/" + template = BuildTarget( + "/src/flow/protocolversion/ProtocolVersion.h.template", + ("ProtocolVersion.h.template", None), + ).markAsFile() + source = BuildTarget( + "/src/flow/ProtocolVersions.cmake", + ("ProtocolVersions.cmake", None), + ).markAsFile() + out = BuildTarget("flow/include/flow/ProtocolVersion.h", ("ProtocolVersion.h", None)) + rule = Rule("CUSTOM_COMMAND") + build = Build([out], rule, [template, source], []) + build.vars["cmake_ninja_workdir"] = work_dir + rule.vars["command"] = ( + "cd /tmp/build/flow && " + "python3 /src/flow/protocolversion/protocol_version.py " + "--source /src/flow/ProtocolVersions.cmake --generator cpp " + "--output /tmp/build/flow/include/flow/ProtocolVersion.h && " + "python3 /src/flow/protocolversion/protocol_version.py " + "/src/flow/protocolversion/ProtocolVersion.h.template " + "--source /src/flow/ProtocolVersions.cmake --generator python " + "--output /tmp/build/flow/include/flow/protocol_version.py" + ) + + commands = build.getGeneratorCommandsForTarget(out) + + self.assertEqual(1, len(commands)) + self.assertIn("--generator cpp", commands[0][0]) + self.assertIn("ProtocolVersion.h", commands[0][0]) + self.assertNotIn("--generator python", commands[0][0]) + self.assertNotIn("--output /tmp/build/flow/include/flow/protocol_version.py", commands[0][0]) + + def test_split_by_generator_commands_assigns_outputs_to_commands(self) -> None: + work_dir = "/tmp/build/" + source = BuildTarget("input.txt", ("input.txt", None)).markAsFile() + out_a = BuildTarget("generated/a.txt", ("a.txt", None)) + out_b = BuildTarget("generated/b.txt", ("b.txt", None)) + rule = Rule("CUSTOM_COMMAND") + build = Build([out_a, out_b], rule, [source], []) + build.vars["cmake_ninja_workdir"] = work_dir + rule.vars["command"] = ( + "python3 gen.py input.txt --output /tmp/build/generated/a.txt && " + "python3 gen.py /tmp/build/generated/a.txt --output /tmp/build/generated/b.txt" + ) + + split_builds = build.splitByGeneratorCommands() + + self.assertEqual( + [["generated/a.txt"], ["generated/b.txt"]], + [[o.name for o in b.outputs] for b in split_builds], + ) + self.assertEqual( + "python3 gen.py /tmp/build/generated/a.txt --output /tmp/build/generated/b.txt", + split_builds[1].rulename.vars["command"], + ) + self.assertIn(out_a, split_builds[1].getInputs()) + + def test_get_generator_commands_excludes_copy_and_cmake_commands(self) -> None: + cases = [ + "cp input.txt generated.txt", + "/bin/cp input.txt generated.txt", + "/usr/bin/cmake -E copy input.txt generated.txt", + "/opt/bin/cmake -E copy input.txt generated.txt", + ] + for command in cases: + with self.subTest(command=command): + inp = BuildTarget("input.txt", ("input.txt", None)).markAsFile() + out = BuildTarget("generated.txt", ("generated.txt", None)) + rule = Rule("CUSTOM_COMMAND") + build = Build([out], rule, [inp], []) + rule.vars["command"] = command + + self.assertEqual([], build.getGeneratorCommands()) + + def test_get_generator_commands_does_not_reuse_setup_for_excluded_command(self) -> None: + inp = BuildTarget("input.txt", ("input.txt", None)).markAsFile() + out = BuildTarget("generated.txt", ("generated.txt", None)) + rule = Rule("CUSTOM_COMMAND") + build = Build([out], rule, [inp], []) + rule.vars["command"] = ( + "mkdir copied && cp input.txt copied/generated.txt && " + "python3 gen.py input.txt generated.txt" + ) + + self.assertEqual( + [("python3 gen.py input.txt generated.txt", None)], + build.getGeneratorCommands(), + ) + def test_is_cpp_command(self) -> None: cases = [ ("clang -c foo.c", True), diff --git a/test/test_build_visitor.py b/test/test_build_visitor.py index f962a85..e963538 100644 --- a/test/test_build_visitor.py +++ b/test/test_build_visitor.py @@ -1,6 +1,6 @@ import unittest -from bazel import BazelBuild, BazelTarget, bazelcache +from bazel import BazelBuild, BazelGenRuleTarget, BazelTarget, bazelcache from build import BazelBuildVisitorContext, Build, BuildTarget, Rule from build_visitor import BuildVisitor @@ -34,3 +34,31 @@ def test_visit_produced_missing_command(self) -> None: out = BuildTarget("out.o", ("out.o", None)) build = Build([out], Rule("CXX"), [], []) self.assertFalse(BuildVisitor.visitProduced(ctx, out, build)) + + def test_visit_custom_command_uses_target_specific_generator_command(self) -> None: + bb = BazelBuild("") + ctx = BazelBuildVisitorContext(False, "/root", bb, [], prefix="") + ctx.current = BazelTarget("cc_library", "lib", "") + source = BuildTarget("flow/ProtocolVersions.cmake", ("ProtocolVersions.cmake", None)).markAsFile() + template = BuildTarget( + "flow/protocolversion/ProtocolVersion.h.template", + ("ProtocolVersion.h.template", None), + ).markAsFile() + out_a = BuildTarget("generated/a.txt", ("a.txt", None)) + out_b = BuildTarget("generated/b.txt", ("b.txt", None)) + rule = Rule("CUSTOM_COMMAND") + build = Build([out_a, out_b], rule, [source, template], []) + rule.vars["command"] = ( + "tool flow/ProtocolVersions.cmake --output generated/a.txt && " + "tool flow/ProtocolVersions.cmake --output generated/b.txt" + ) + split_builds = build.splitByGeneratorCommands() + target_build = split_builds[1] + + self.assertTrue(BuildVisitor.visitProduced(ctx, out_b, target_build)) + + gen = target_build.associatedBazelTarget + self.assertIsInstance(gen, BazelGenRuleTarget) + self.assertEqual({"b.txt"}, {out.name for out in gen.outs}) + self.assertIn("b.txt", gen.cmd) + self.assertNotIn("generated/a.txt", gen.cmd) diff --git a/test/test_ninjabuild.py b/test/test_ninjabuild.py index 321ca7e..3a55325 100644 --- a/test/test_ninjabuild.py +++ b/test/test_ninjabuild.py @@ -1,4 +1,5 @@ import os +import sys import tempfile import unittest from pathlib import Path @@ -118,3 +119,126 @@ def test_resolve_name_prefers_additional_vars(self) -> None: parser.vars["ctx"]["cmake_ninja_workdir"] = "/root" resolved = parser._resolveName("${FOO}", {"FOO": "override"}) self.assertEqual(resolved, "override") + + def test_parser_splits_custom_command_outputs_by_generator_command(self) -> None: + with tempfile.TemporaryDirectory() as td: + build_file = Path(td) / "build.ninja" + build_file.write_text( + "rule CUSTOM_COMMAND\n" + " command = $COMMAND\n" + "\n" + "build generated/a.txt generated/b.txt: CUSTOM_COMMAND\n" + " COMMAND = python3 gen.py --output generated/a.txt && " + "python3 gen.py generated/a.txt --output generated/b.txt\n" + ) + + parser = NinjaParser(td) + parser.setManuallyGeneratedTargets({}) + parser.setContext("ctx") + parser.vars["ctx"]["cmake_ninja_workdir"] = f"{td}/" + parser.setRemapPath({}) + parser.setCompilerIncludes([]) + parser.setCCImports([]) + parser.parse(build_file.read_text().splitlines(), td) + parser.markDone() + parser.endContext("ctx") + + producer_a = parser.all_outputs["generated/a.txt"].producedby + producer_b = parser.all_outputs["generated/b.txt"].producedby + + self.assertIsNot(producer_a, producer_b) + self.assertEqual(["generated/a.txt"], [output.name for output in producer_a.outputs]) + self.assertEqual(["generated/b.txt"], [output.name for output in producer_b.outputs]) + self.assertIn(parser.all_outputs["generated/a.txt"], producer_b.getInputs()) + + def test_execute_generator_runs_only_target_generator_command_after_cd(self) -> None: + with tempfile.TemporaryDirectory() as td: + tmp_path = Path(td) + work_dir = tmp_path / "build" + source = tmp_path / "ProtocolVersions.cmake" + source.write_text("version") + script = tmp_path / "protocol_version.py" + script.write_text( + "import argparse\n" + "from pathlib import Path\n" + "parser = argparse.ArgumentParser()\n" + "parser.add_argument('--source')\n" + "parser.add_argument('--generator')\n" + "parser.add_argument('--output')\n" + "args = parser.parse_args()\n" + "output = Path(args.output)\n" + "output.parent.mkdir(parents=True, exist_ok=True)\n" + "output.write_text(args.generator)\n" + ) + template = tmp_path / "ProtocolVersion.h.template" + template.write_text("template") + + parser = NinjaParser(str(tmp_path)) + rule = Rule("CUSTOM_COMMAND") + output = BuildTarget( + "flow/include/flow/ProtocolVersion.h", + ("ProtocolVersion.h", None), + ) + build = Build( + [output], + rule, + [ + BuildTarget(str(script), (script.name, None)), + BuildTarget(str(template), (template.name, None)), + BuildTarget(str(source), (source.name, None)), + ], + [], + ) + build.vars["cmake_ninja_workdir"] = f"{work_dir}/" + rule.vars["command"] = ( + f"cd {work_dir}/flow && " + f"{sys.executable} {script} --source {source} --generator cpp " + f"--output {work_dir}/flow/include/flow/ProtocolVersion.h && " + f"{sys.executable} {script} --source {source} --generator java " + f"--output {work_dir}/flow/include/flow/ProtocolVersion.java" + ) + + temp_dir = Path(parser.executeGenerator(build, output)) + + self.assertEqual( + "cpp", + (temp_dir / "flow/include/flow/ProtocolVersion.h").read_text(), + ) + self.assertFalse( + (temp_dir / "flow/include/flow/ProtocolVersion.java").exists(), + ) + + def test_execute_generator_for_split_build_runs_only_its_command(self) -> None: + with tempfile.TemporaryDirectory() as td: + tmp_path = Path(td) + work_dir = tmp_path / "build" + script = tmp_path / "gen.py" + script.write_text( + "import argparse\n" + "from pathlib import Path\n" + "parser = argparse.ArgumentParser()\n" + "parser.add_argument('--value')\n" + "parser.add_argument('--output')\n" + "args = parser.parse_args()\n" + "output = Path(args.output)\n" + "output.parent.mkdir(parents=True, exist_ok=True)\n" + "output.write_text(args.value)\n" + ) + + parser = NinjaParser(str(tmp_path)) + rule = Rule("CUSTOM_COMMAND") + out_a = BuildTarget("generated/a.txt", ("a.txt", None)) + out_b = BuildTarget("generated/b.txt", ("b.txt", None)) + build = Build([out_a, out_b], rule, [], []) + build.vars["cmake_ninja_workdir"] = f"{work_dir}/" + rule.vars["command"] = ( + f"{sys.executable} {script} --value a --output {work_dir}/generated/a.txt && " + f"{sys.executable} {script} --value b --output {work_dir}/generated/b.txt" + ) + split_builds = build.splitByGeneratorCommands() + target_build = split_builds[1] + + temp_dir = Path(parser.executeGenerator(target_build, out_b)) + + self.assertFalse((temp_dir / "generated/a.txt").exists()) + self.assertEqual("b", (temp_dir / "generated/b.txt").read_text()) From c9f63cffaa27084d66c5d955198f3b0cefda1cf4 Mon Sep 17 00:00:00 2001 From: Matthieu Patou Date: Tue, 5 May 2026 16:14:07 +0000 Subject: [PATCH 2/7] Fix visiting protobuf and grpc libs After refactoring the way we parse commands for genrule we exposed a bug in the way some relation existed in the graph we have to fix them. --- build.py | 6 ++++-- test/test_build.py | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/build.py b/build.py index eae3503..03c4e06 100644 --- a/build.py +++ b/build.py @@ -1340,7 +1340,8 @@ def _handleGRPCCCProtobuf(self, ctx: BazelBuildVisitorContext, el: BuildTarget): for tgt in ctx.bazelbuild.bazelTargets: if tgt.name == f"{proto}_cc_proto": t.addDep(tgt) - ctx.current.addDep(t) + if not isinstance(ctx.current, (BazelCCProtoLibrary, BazelGRPCCCProtoLibrary)): + ctx.current.addDep(t) ctx.next_current = t ctx.current = t @@ -1355,7 +1356,8 @@ def _handleCCProtobuf(self, ctx: BazelBuildVisitorContext, el: BuildTarget): t: BaseBazelTarget = getObject( BazelCCProtoLibrary, f"{proto}_cc_proto", location ) - ctx.current.addDep(t) + if not isinstance(ctx.current, BazelCCProtoLibrary): + ctx.current.addDep(t) ctx.bazelbuild.bazelTargets.add(t) for tgt in ctx.bazelbuild.bazelTargets: if tgt.name == f"{proto}_cc_grpc": diff --git a/test/test_build.py b/test/test_build.py index 9daa775..c4d2943 100644 --- a/test/test_build.py +++ b/test/test_build.py @@ -5,7 +5,12 @@ from pathlib import Path sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) -from bazel import BazelCCImport, BazelGenRuleTarget +from bazel import ( + BazelCCImport, + BazelCCProtoLibrary, + BazelGenRuleTarget, + BazelGRPCCCProtoLibrary, +) from bazel import BazelTarget, BazelBuild, getObject, bazelcache from build import BazelBuildVisitorContext, Build, BuildTarget, Rule from ninjabuild import canBePruned @@ -503,6 +508,34 @@ def test_handle_protobuf_header_keeps_context(self) -> None: self.assertIs(ctx.next_current, kept) self.assertGreater(len(kept.deps), 0) # type: ignore + def test_handle_grpc_protobuf_header_keeps_current_grpc_context(self) -> None: + ctx = self._ctx() + current = BazelGRPCCCProtoLibrary("foo_cc_grpc", "proto") + ctx.current = current + out = BuildTarget("foo.grpc.pb.h", ("foo.grpc.pb.h", "proto")) + build = Build([out], Rule("CUSTOM_COMMAND"), [], []) + build.vars["COMMAND"] = "/usr/bin/bin/protoc something" + + self.assertTrue(build.handleRuleProducedForBazelGen(ctx, out, "cmd")) + + self.assertIs(ctx.current, current) + self.assertIs(ctx.next_current, current) + self.assertNotIn(current, current.deps) + + def test_handle_protobuf_header_keeps_current_cc_proto_context(self) -> None: + ctx = self._ctx() + current = BazelCCProtoLibrary("foo_cc_proto", "proto") + ctx.current = current + out = BuildTarget("foo.pb.h", ("foo.pb.h", "proto")) + build = Build([out], Rule("CUSTOM_COMMAND"), [], []) + build.vars["COMMAND"] = "/usr/bin/bin/protoc something" + + self.assertTrue(build.handleRuleProducedForBazelGen(ctx, out, "cmd")) + + self.assertIs(ctx.current, current) + self.assertIs(ctx.next_current, current) + self.assertNotIn(current, current.deps) + def test_revisit_shared_library_reuses_existing_target(self) -> None: ctx = self._ctx() out = BuildTarget("libfoo.so", ("libfoo.so", None)) From 0302f134028f255b10e1b15c36edd9b59776b145 Mon Sep 17 00:00:00 2001 From: Matthieu Patou Date: Tue, 5 May 2026 20:03:45 +0000 Subject: [PATCH 3/7] Don't rely on the order for properly populating deps or srcs of protobuf libs and grpc libs --- build.py | 170 ++++++++++++++++++++++++++++++++++++--------- test/test_build.py | 121 ++++++++++++++++++++++++++++++++ 2 files changed, 258 insertions(+), 33 deletions(-) diff --git a/build.py b/build.py index 03c4e06..bfbc464 100644 --- a/build.py +++ b/build.py @@ -1025,11 +1025,7 @@ def _handleProtobufForBazelGen( ctx.next_current = savedCurrent # Maybe we still want to continue ... tbd return True - location = TopLevelGroupingStrategy().getBuildFilenamePath( - el, ctx.current.location if ctx.current else ctx.prefix - ) - t = getObject(BazelProtoLibrary, f"{proto}_proto", location) - ctx.bazelbuild.bazelTargets.add(t) + t = self._getOrCreateProtoLibrary(ctx, el) self.setAssociatedBazelTarget(t) assert el.producedby is not None @@ -1286,6 +1282,33 @@ def _handleCustomCommandForBazelGen( @classmethod def _getProtoName(kls, element: BuildTarget) -> str: + name = kls._getRawProtoName(element) + if name in kls._protoNames: + return kls._protoNames[name] + + logging.debug(f"Getting proto name for {element.shortName} => {name}") + arr = name.split(os.path.sep) + filename = arr[-1] + existingNames = list(kls._protoNames.values()) + + for i in sorted(range(-len(arr), 0), reverse=True): + logging.debug( + f"Checking {name} {arr[i:]} i = {i} location = {element.location}" + ) + filename = kls._normalizeProtoNameParts(arr[i:]) + if filename == "": + continue + if filename not in existingNames: + kls._protoNames[name] = filename + return filename + assert False + + @classmethod + def _normalizeProtoNameParts(kls, parts: List[str]) -> str: + return "_".join(part for part in parts if part not in ("proto", "protos")) + + @classmethod + def _getRawProtoName(kls, element: BuildTarget) -> str: regex = r"(.*?)(\.grpc)?\.pb\.(cc|h|cc\.o)$" # clean extentions matches = re.match(regex, element.shortName) @@ -1304,31 +1327,17 @@ def _getProtoName(kls, element: BuildTarget) -> str: if element.location is not None and not name.startswith(element.location): # Protobuf files seems not have location (why ?) so it helps normalize the name - name = f"{element.location}{name}" - - if name in kls._protoNames: - return kls._protoNames[name] - - logging.debug(f"Getting proto name for {element.shortName} => {name}") - arr = name.split(os.path.sep) - filename = arr[-1] - existingNames = list(kls._protoNames.values()) + name = os.path.join(element.location, name) - for i in sorted(range(-len(arr), 0), reverse=True): - logging.debug( - f"Checking {name} {arr[i:]} i = {i} location = {element.location}" - ) - filename = "_".join(arr[i:]) - if filename not in existingNames: - kls._protoNames[name] = filename - return filename - assert False + return name def _handleGRPCCCProtobuf(self, ctx: BazelBuildVisitorContext, el: BuildTarget): assert ctx.current is not None # We can rely on self.associatedBazelTarget usually protobuf related target produces multiple files and multiple bazel targets # Now that we cache the associated bazel targets there is limited risk to "recreate" the same target - proto = self._getProtoName(el) + proto = self._getCanonicalProtoName(el) + proto_lib = self._getOrCreateProtoLibrary(ctx, el) + cc_proto = self._getOrCreateCCProtoLibrary(ctx, el, proto_lib) location = TopLevelGroupingStrategy().getBuildFilenamePath( el, ctx.current.location if ctx.current else ctx.prefix @@ -1336,27 +1345,29 @@ def _handleGRPCCCProtobuf(self, ctx: BazelBuildVisitorContext, el: BuildTarget): t: BaseBazelTarget = getObject( BazelGRPCCCProtoLibrary, f"{proto}_cc_grpc", location ) + assert isinstance(t, BazelGRPCCCProtoLibrary) ctx.bazelbuild.bazelTargets.add(t) - for tgt in ctx.bazelbuild.bazelTargets: - if tgt.name == f"{proto}_cc_proto": - t.addDep(tgt) - if not isinstance(ctx.current, (BazelCCProtoLibrary, BazelGRPCCCProtoLibrary)): + t.addSrc(proto_lib) + t.addDep(cc_proto) + if not isinstance( + ctx.current, + (BazelProtoLibrary, BazelCCProtoLibrary, BazelGRPCCCProtoLibrary), + ): ctx.current.addDep(t) ctx.next_current = t ctx.current = t def _handleCCProtobuf(self, ctx: BazelBuildVisitorContext, el: BuildTarget): assert ctx.current is not None - proto = self._getProtoName(el) + proto = self._getCanonicalProtoName(el) + proto_lib = self._getOrCreateProtoLibrary(ctx, el) + t = self._getOrCreateCCProtoLibrary(ctx, el, proto_lib) location = TopLevelGroupingStrategy().getBuildFilenamePath( el, ctx.current.location if ctx.current else ctx.prefix ) - t: BaseBazelTarget = getObject( - BazelCCProtoLibrary, f"{proto}_cc_proto", location - ) - if not isinstance(ctx.current, BazelCCProtoLibrary): + if not isinstance(ctx.current, (BazelProtoLibrary, BazelCCProtoLibrary)): ctx.current.addDep(t) ctx.bazelbuild.bazelTargets.add(t) for tgt in ctx.bazelbuild.bazelTargets: @@ -1366,6 +1377,99 @@ def _handleCCProtobuf(self, ctx: BazelBuildVisitorContext, el: BuildTarget): ctx.next_current = t ctx.current = t + def _getOrCreateProtoLibrary( + self, ctx: BazelBuildVisitorContext, el: BuildTarget + ) -> BazelProtoLibrary: + proto = self._getCanonicalProtoName(el) + proto_input = self._getPrimaryProtoInput(el) + location = TopLevelGroupingStrategy().getBuildFilenamePath( + el, ctx.current.location if ctx.current else ctx.prefix + ) + t = getObject(BazelProtoLibrary, f"{proto}_proto", location) + assert isinstance(t, BazelProtoLibrary) + ctx.bazelbuild.bazelTargets.add(t) + if proto_input is not None: + for paramName, paramValue in proto_input.bazelAdditionalParameters.items(): + logging.info( + f"Setting {paramName} to {paramValue} on {proto_input.name}" + ) + t.__setattr__(paramName, paramValue) + t.addSrc( + self._genExportedFile( + filename=_relpath_for_bazel(proto_input.name, ctx.rootdir), + locationCaller=t.location, + ctx=ctx, + ) + ) + return t + + def _getOrCreateCCProtoLibrary( + self, + ctx: BazelBuildVisitorContext, + el: BuildTarget, + proto_lib: BazelProtoLibrary, + ) -> BazelCCProtoLibrary: + proto = self._getCanonicalProtoName(el) + location = TopLevelGroupingStrategy().getBuildFilenamePath( + el, ctx.current.location if ctx.current else ctx.prefix + ) + t = getObject(BazelCCProtoLibrary, f"{proto}_cc_proto", location) + assert isinstance(t, BazelCCProtoLibrary) + t.addDep(proto_lib) + ctx.bazelbuild.bazelTargets.add(t) + return t + + def _getCanonicalProtoName(self, el: BuildTarget) -> str: + proto_input = self._getPrimaryProtoInput(el) + if proto_input is not None: + return self._getProtoName(proto_input) + return self._getProtoName(el) + + def _getPrimaryProtoInput(self, el: BuildTarget) -> Optional[BuildTarget]: + proto_inputs = self._getProtoInputs() + if len(proto_inputs) == 0: + return None + if len(proto_inputs) == 1: + return proto_inputs[0] + + generated_name = self._getRawProtoName(el) + matches = [ + proto_input + for proto_input in proto_inputs + if self._protoNamesMatch(generated_name, self._getProtoName(proto_input)) + ] + if len(matches) == 1: + return matches[0] + + logging.warning( + f"Could not identify a unique proto input for {el.name}; " + f"candidates are {[i.name for i in proto_inputs]}" + ) + return None + + def _protoNamesMatch(self, generated_name: str, proto_name: str) -> bool: + return generated_name == proto_name or generated_name.endswith(f"_{proto_name}") + + def _getProtoInputs(self) -> List[BuildTarget]: + proto_inputs = [i for i in self._inputs if i.name.endswith(".proto")] + for input_target in self._inputs: + if input_target.producedby is None: + continue + proto_inputs.extend( + i + for i in input_target.producedby._inputs + if i.name.endswith(".proto") + ) + + ret = [] + seen = set() + for proto_input in proto_inputs: + if proto_input.name in seen: + continue + seen.add(proto_input.name) + ret.append(proto_input) + return ret + def _handleCPPLinkExecutableCommand( self, el: BuildTarget, cmd: str, ctx: BazelBuildVisitorContext ) -> bool: diff --git a/test/test_build.py b/test/test_build.py index c4d2943..37dead2 100644 --- a/test/test_build.py +++ b/test/test_build.py @@ -10,6 +10,7 @@ BazelCCProtoLibrary, BazelGenRuleTarget, BazelGRPCCCProtoLibrary, + BazelProtoLibrary, ) from bazel import BazelTarget, BazelBuild, getObject, bazelcache from build import BazelBuildVisitorContext, Build, BuildTarget, Rule @@ -490,6 +491,7 @@ def test_static_archive_command(self) -> None: class TestBuildProtoAndLinkHandling(unittest.TestCase): def setUp(self) -> None: bazelcache.clear() + Build._protoNames.clear() def _ctx(self) -> BazelBuildVisitorContext: bb = BazelBuild("") @@ -508,6 +510,125 @@ def test_handle_protobuf_header_keeps_context(self) -> None: self.assertIs(ctx.next_current, kept) self.assertGreater(len(kept.deps), 0) # type: ignore + def test_handle_protobuf_header_creates_complete_cc_proto_stack(self) -> None: + ctx = self._ctx() + proto_input = BuildTarget("echo.proto", ("echo.proto", None)).markAsFile() + out = BuildTarget("echo.pb.h", ("echo.pb.h", None)) + build = Build([out], Rule("CUSTOM_COMMAND"), [proto_input], []) + build.vars["COMMAND"] = "/usr/bin/bin/protoc something" + parent = ctx.current + + self.assertTrue(build.handleRuleProducedForBazelGen(ctx, out, "cmd")) + + proto = next(t for t in ctx.bazelbuild.bazelTargets if t.name == "echo_proto") + cc_proto = next(t for t in ctx.bazelbuild.bazelTargets if t.name == "echo_cc_proto") + self.assertIsInstance(proto, BazelProtoLibrary) + self.assertIsInstance(cc_proto, BazelCCProtoLibrary) + self.assertIn(proto, cc_proto.deps) + self.assertIn(cc_proto, parent.deps) # type: ignore[union-attr] + self.assertEqual( + [":echo.proto"], + sorted(src.name for src in proto.srcs), # type: ignore[attr-defined] + ) + + def test_handle_grpc_protobuf_header_creates_complete_grpc_proto_stack(self) -> None: + ctx = self._ctx() + proto_input = BuildTarget("echo.proto", ("echo.proto", None)).markAsFile() + out = BuildTarget("echo.grpc.pb.h", ("echo.grpc.pb.h", None)) + build = Build([out], Rule("CUSTOM_COMMAND"), [proto_input], []) + build.vars["COMMAND"] = "/usr/bin/bin/protoc something" + parent = ctx.current + + self.assertTrue(build.handleRuleProducedForBazelGen(ctx, out, "cmd")) + + proto = next(t for t in ctx.bazelbuild.bazelTargets if t.name == "echo_proto") + cc_proto = next(t for t in ctx.bazelbuild.bazelTargets if t.name == "echo_cc_proto") + grpc = next(t for t in ctx.bazelbuild.bazelTargets if t.name == "echo_cc_grpc") + self.assertIsInstance(proto, BazelProtoLibrary) + self.assertIsInstance(cc_proto, BazelCCProtoLibrary) + self.assertIsInstance(grpc, BazelGRPCCCProtoLibrary) + self.assertIn(proto, cc_proto.deps) + self.assertIn(proto, grpc.srcs) + self.assertIn(cc_proto, grpc.deps) + self.assertIn(grpc, parent.deps) # type: ignore[union-attr] + + def test_grpc_protobuf_header_uses_proto_input_name(self) -> None: + ctx = self._ctx() + proto_input = BuildTarget("echo.proto", ("echo.proto", None)).markAsFile() + out = BuildTarget("test_echo.grpc.pb.h", ("test_echo.grpc.pb.h", None)) + build = Build([out], Rule("CUSTOM_COMMAND"), [proto_input], []) + build.vars["COMMAND"] = "/usr/bin/bin/protoc something" + parent = ctx.current + + self.assertTrue(build.handleRuleProducedForBazelGen(ctx, out, "cmd")) + + target_names = {t.name for t in ctx.bazelbuild.bazelTargets} + self.assertIn("echo_proto", target_names) + self.assertIn("echo_cc_proto", target_names) + self.assertIn("echo_cc_grpc", target_names) + self.assertNotIn("test_echo_proto", target_names) + self.assertNotIn("test_echo_cc_proto", target_names) + self.assertNotIn("test_echo_cc_grpc", target_names) + grpc = next(t for t in ctx.bazelbuild.bazelTargets if t.name == "echo_cc_grpc") + self.assertIn(grpc, parent.deps) # type: ignore[union-attr] + + def test_grpc_protobuf_header_strips_proto_path_components(self) -> None: + ctx = self._ctx() + proto_input = BuildTarget("protos/echo.proto", ("echo.proto", "protos")).markAsFile() + out = BuildTarget("protos/echo.grpc.pb.h", ("echo.grpc.pb.h", "protos")) + build = Build([out], Rule("CUSTOM_COMMAND"), [proto_input], []) + build.vars["COMMAND"] = "/usr/bin/bin/protoc something" + + self.assertTrue(build.handleRuleProducedForBazelGen(ctx, out, "cmd")) + + target_names = {t.name for t in ctx.bazelbuild.bazelTargets} + self.assertIn("echo_proto", target_names) + self.assertIn("echo_cc_proto", target_names) + self.assertIn("echo_cc_grpc", target_names) + self.assertNotIn("protos_echo_proto", target_names) + self.assertNotIn("protos_echo_cc_proto", target_names) + self.assertNotIn("protos_echo_cc_grpc", target_names) + + def test_grpc_protobuf_header_uses_one_primary_proto_input(self) -> None: + ctx = self._ctx() + other_input = BuildTarget("other.proto", ("other.proto", None)).markAsFile() + proto_input = BuildTarget("echo.proto", ("echo.proto", None)).markAsFile() + out = BuildTarget("test_echo.grpc.pb.h", ("test_echo.grpc.pb.h", None)) + build = Build([out], Rule("CUSTOM_COMMAND"), [other_input, proto_input], []) + build.vars["COMMAND"] = "/usr/bin/bin/protoc something" + + self.assertTrue(build.handleRuleProducedForBazelGen(ctx, out, "cmd")) + + proto = next(t for t in ctx.bazelbuild.bazelTargets if t.name == "echo_proto") + cc_proto = next(t for t in ctx.bazelbuild.bazelTargets if t.name == "echo_cc_proto") + grpc = next(t for t in ctx.bazelbuild.bazelTargets if t.name == "echo_cc_grpc") + self.assertEqual( + [":echo.proto"], + sorted(src.name for src in proto.srcs), # type: ignore[attr-defined] + ) + self.assertEqual({proto}, cc_proto.deps) + self.assertEqual({proto}, grpc.srcs) # type: ignore[attr-defined] + self.assertIn(cc_proto, grpc.deps) + + def test_protobuf_compile_uses_proto_input_from_generated_source(self) -> None: + ctx = self._ctx() + proto_input = BuildTarget("echo.proto", ("echo.proto", None)).markAsFile() + generated_cc = BuildTarget("test_echo.pb.cc", ("test_echo.pb.cc", None)) + Build([generated_cc], Rule("CUSTOM_COMMAND"), [proto_input], []) + out = BuildTarget("test_echo.pb.cc.o", ("test_echo.pb.cc.o", None)) + build = Build([out], Rule("CXX_COMPILER"), [generated_cc], []) + parent = ctx.current + + build._handleCCProtobuf(ctx, out) + + target_names = {t.name for t in ctx.bazelbuild.bazelTargets} + self.assertIn("echo_proto", target_names) + self.assertIn("echo_cc_proto", target_names) + self.assertNotIn("test_echo_proto", target_names) + self.assertNotIn("test_echo_cc_proto", target_names) + cc_proto = next(t for t in ctx.bazelbuild.bazelTargets if t.name == "echo_cc_proto") + self.assertIn(cc_proto, parent.deps) # type: ignore[union-attr] + def test_handle_grpc_protobuf_header_keeps_current_grpc_context(self) -> None: ctx = self._ctx() current = BazelGRPCCCProtoLibrary("foo_cc_grpc", "proto") From a5d5eb2ddce8c1cbbfb0a649c846054f07b89e75 Mon Sep 17 00:00:00 2001 From: Matthieu Patou Date: Tue, 5 May 2026 22:13:04 +0000 Subject: [PATCH 4/7] Don't forget to import cc_proto_library if needed --- bazel.py | 3 +++ test/test_bazel.py | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/bazel.py b/bazel.py index 1e2968f..ac39e35 100644 --- a/bazel.py +++ b/bazel.py @@ -988,6 +988,9 @@ class BazelCCProtoLibrary(BaseBazelTarget): def __init__(self, name: str, location: str): super().__init__("cc_proto_library", name, location) + def getGlobalImport(self) -> str: + return 'load("@com_google_protobuf//bazel:cc_proto_library.bzl", "cc_proto_library")' + def addDep(self, dep: Union[BaseBazelTarget, BazelCCImport]): assert isinstance(dep, BazelProtoLibrary) or isinstance(dep, BazelExternalDep) self.deps.add(dep) diff --git a/test/test_bazel.py b/test/test_bazel.py index 4a87a1e..780e9d6 100644 --- a/test/test_bazel.py +++ b/test/test_bazel.py @@ -55,6 +55,10 @@ def test_cc_targets_emit_explicit_rules_cc_load(self): BazelTarget("cc_binary", "app", "src").getGlobalImport(), 'load("@rules_cc//cc:defs.bzl", "cc_binary")', ) + self.assertEqual( + BazelCCProtoLibrary("proto_cc", "src").getGlobalImport(), + 'load("@com_google_protobuf//bazel:cc_proto_library.bzl", "cc_proto_library")', + ) def test_py_and_sh_targets_emit_explicit_loads(self): self.assertEqual( @@ -202,6 +206,10 @@ def test_gen_bazel_build_content_includes_various_targets(self) -> None: 'load("@rules_cc//cc:defs.bzl", "cc_library")', src_content, ) + self.assertIn( + 'load("@com_google_protobuf//bazel:cc_proto_library.bzl", "cc_proto_library")', + src_content, + ) self.assertNotIn('"cc_binary"', src_content) self.assertNotIn('"cc_shared_library"', src_content) self.assertIn( From 0105078f1538c9a42485bf00c26f651e6f0f6bd1 Mon Sep 17 00:00:00 2001 From: Matthieu Patou Date: Thu, 30 Apr 2026 05:28:02 +0000 Subject: [PATCH 5/7] Overall postprocess to work better with strings, load, rules * preserve load(...) lines understand loaded rule symbols * emit BUILD rules with more Bazel-like formatting, add ordered attribute printing, sorted list values * correct string formatting including triple-quoted strings * avoid emitting the same rule more than once * narrow the companion _hdrs rule behavior to cc_library rules * fixes a missing-dependency bookkeeping bug with missing.setdefault(...). --- postprocess | 214 ++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 174 insertions(+), 40 deletions(-) diff --git a/postprocess b/postprocess index b3b3248..00f3933 100755 --- a/postprocess +++ b/postprocess @@ -2,11 +2,58 @@ from __future__ import annotations import argparse +import re from dataclasses import dataclass, field from pathlib import Path from typing import Any, Callable, Dict, List, Optional +TRIPLE_QUOTED_STRINGS: set[str] = set() + +RULE_ATTR_ORDER: Dict[str, List[str]] = { + "cc_library": [ + "srcs", + "hdrs", + "copts", + "conlyopts", + "cxxopts", + "defines", + "data", + "includes", + "linkopts", + "deps", + "visibility", + ], + "cc_binary": [ + "srcs", + "copts", + "conlyopts", + "cxxopts", + "defines", + "data", + "includes", + "linkopts", + "deps", + "visibility", + ], + "cc_test": [ + "srcs", + "copts", + "conlyopts", + "cxxopts", + "defines", + "data", + "includes", + "linkopts", + "deps", + "visibility", + ], + "genrule": ["srcs", "outs", "cmd", "local", "tools"], + "py_binary": ["srcs", "main"], + "sh_binary": ["srcs"], +} + + @dataclass class RuleCall: rule: str @@ -60,6 +107,19 @@ def select(mapping, **kwargs): return {"__fn__": "select", "mapping": mapping, "kwargs": kwargs} +def load_lines(build_path: str) -> List[str]: + return [ + line + for line in Path(build_path).read_text(encoding="utf-8").splitlines() + if line.lstrip().startswith("load(") + ] + + +def remember_triple_quoted_strings(source: str) -> None: + TRIPLE_QUOTED_STRINGS.clear() + TRIPLE_QUOTED_STRINGS.update(re.findall(r'"""(.*?)"""', source, re.DOTALL)) + + def parse_build_with_starlark_pyo3( build_path: str, *, @@ -86,6 +146,7 @@ def parse_build_with_starlark_pyo3( ] source = Path(build_path).read_text(encoding="utf-8") + remember_triple_quoted_strings(source) # Dialect: standard Starlark; BUILD files usually need top-level statements. dialect = sl.Dialect.standard() @@ -97,6 +158,11 @@ def parse_build_with_starlark_pyo3( recorder = BuildRecorder() module = sl.Module() glb = sl.Globals.standard() + loaded_symbols = { + symbol + for load in ast.loads() + for symbol in load.symbols.values() + } # Provide rule functions that record what they were called with. for r in rule_names: @@ -104,13 +170,17 @@ def parse_build_with_starlark_pyo3( r, recorder.rule_fn(r) ) # :contentReference[oaicite:3]{index=3} + loaded_module = sl.Module() + for r in loaded_symbols: + loaded_module.add_callable(r, recorder.rule_fn(r)) + loaded_module = loaded_module.freeze() + # Provide common BUILD helpers (symbolic) module.add_callable("glob", glob) module.add_callable("select", select) # Evaluate the BUILD file; rule calls will be recorded. - # (If your BUILD uses load(), see note below.) - sl.eval(module, ast, glb) # :contentReference[oaicite:4]{index=4} + sl.eval(module, ast, glb, sl.FileLoader(lambda _: loaded_module)) # :contentReference[oaicite:4]{index=4} return recorder.calls @@ -146,9 +216,7 @@ def grafify_rules(raw_rules: List[RuleCall]) -> List[RuleCall]: c.deps.append(all_rules[dep_name]) all_rules[dep_name].usedBy.append(c) else: - if missing[dep_name] is None: - missing[dep_name] = [] - missing[dep_name].append(c) + missing.setdefault(dep_name, []).append(c) if len(remaining_deps) > 0: c.attrs["deps"] = remaining_deps elif deps is not None: @@ -174,60 +242,119 @@ def debug_graph(graph: List[RuleCall]): for e in graph: print_rule(e) -def print_bazel(rule: RuleCall) -> None: - print(f"{rule.rule}(") - print(f' name = "{rule.attrs.get("name")}",') - for attr,val in rule.attrs.items(): - if attr != "name": - if isinstance(val, list): - if len(val) == 0: - continue - print(f' {attr} = [') - for v in val: - print(f' "{v}",') +def format_string(val: Any) -> str: + if not isinstance(val, str): + return str(val) + if "\n" in val or val in TRIPLE_QUOTED_STRINGS: + return f'"""{val}"""' + return f'"{val}"' - print(' ],') - else: - print(f' {attr} = "{val}",') - print(")") - print(f"{rule.rule}(") - print(f' name = "{rule.attrs.get("name")}_hdrs",') - for attr,val in rule.attrs.items(): - if attr in ["visibility", "hdrs"]: - if isinstance(val, list): - if len(val) == 0: - continue - print(f' {attr} = [') - for v in val: - print(f' "{v}",') +def sorted_list_values(values: List[Any]) -> List[Any]: + return sorted(values, key=format_string) - print(' ],') - else: - print(f' {attr} = "{val}",') +def print_attr(attr: str, val: Any) -> None: + if isinstance(val, list): + if len(val) == 0: + return + print(f' {attr} = [') + for v in sorted_list_values(val): + print(f' {format_string(v)},') + + print(' ],') + else: + print(f' {attr} = {format_string(val)},') + + +def attrs_for_print(rule: RuleCall) -> Dict[str, Any]: + attrs = dict(rule.attrs) + deps = list(attrs.get("deps") or []) + for dep in rule.deps: + name = dep.attrs.get("name") + assert isinstance(name, str) + dep_label = f":{name}" + if dep_label not in deps: + deps.append(dep_label) + if deps: + attrs["deps"] = deps + return attrs + + +def print_rule_call(rule: RuleCall, attrs: Optional[List[str]] = None) -> None: + printable_attrs = attrs_for_print(rule) + print(f"{rule.rule}(") + print(f' name = {format_string(printable_attrs.get("name"))},') + attr_names = attrs or ordered_attr_names(rule, printable_attrs) + for attr in attr_names: + if attr == "name" or attr not in printable_attrs: + continue + print_attr(attr, printable_attrs[attr]) print(")") -def visit_rule(e: RuleCall, shouldCollapse: bool, callback: Callable[[RuleCall], None]): + +def ordered_attr_names(rule: RuleCall, attrs: Optional[Dict[str, Any]] = None) -> List[str]: + attrs = attrs or rule.attrs + preferred = RULE_ATTR_ORDER.get(rule.rule, []) + seen = set(preferred) + extra_attrs = sorted( + attr for attr in attrs if attr not in seen and attr != "name" + ) + return preferred + extra_attrs + + +def print_bazel(rule: RuleCall) -> None: + print_rule_call(rule) + if rule.rule == "cc_library": + print() + print(f"{rule.rule}(") + print(f' name = {format_string(rule.attrs.get("name") + "_hdrs")},') + for attr in ["hdrs", "visibility"]: + if attr in rule.attrs: + print_attr(attr, rule.attrs[attr]) + print(")") + print() + +def visit_rule( + e: RuleCall, + shouldCollapse: bool, + callback: Callable[[RuleCall], None], + emitted: Optional[set[int]] = None, +): # Here is the logic, # When we find a rule that has hidden symbols we start collecting all the srcs, hdrs, copts, ... from the # deps that also have hidden symbols hasHidden = e.hasHiddenSymbols + if emitted is None: + emitted = set() # Let's copy external deps new_deps: List[RuleCall] = [] for d in e.deps: - visit_rule(d, (e.rule == "cc_library" or hasHidden) and d.hasHiddenSymbols, callback) + visit_rule( + d, + (e.rule == "cc_library" or hasHidden) and d.hasHiddenSymbols, + callback, + emitted, + ) if (e.rule == "cc_library" or hasHidden) and d.hasHiddenSymbols: for attr in d.attrs: if attr in ["name", "visibility"]: continue current_attr = e.attrs.get(attr, []) assert isinstance(current_attr, list) - current_attr.extend(d.attrs.get(attr, [])) + if attr not in e.attrs: + e.attrs[attr] = current_attr + for val in d.attrs.get(attr, []): + if val not in current_attr: + current_attr.append(val) + for dep in d.deps: + if not any(existing is dep for existing in new_deps): + new_deps.append(dep) else: new_deps.append(d) e.deps = new_deps - if not shouldCollapse: + if not shouldCollapse and id(e) not in emitted: + emitted.add(id(e)) callback(e) @@ -247,8 +374,8 @@ parent: list attributes such as srcs, hdrs, copts, cxxopts, and deps are copied up, while name and visibility are left on the original rule. The collapsed dep edge is removed. -The rewritten BUILD content is printed to stdout. For each emitted rule, the -script also prints a companion rule named "_hdrs" containing only hdrs and +The rewritten BUILD content is printed to stdout. For each emitted cc_library, +the script also prints a companion rule named "_hdrs" containing only hdrs and visibility. This is useful when generated hidden-symbol libraries need to be flattened while still preserving a header-only target shape. """ @@ -282,9 +409,16 @@ def main() -> None: debug_graph(graph) return + loads = load_lines(args.build_file) + for line in loads: + print(line) + if loads: + print() + # Collapse hidden-symbol deps and print the rewritten BUILD content. + emitted: set[int] = set() for e in graph: - visit_rule(e, False, lambda e: print_bazel(e)) + visit_rule(e, False, lambda e: print_bazel(e), emitted) if __name__ == "__main__": From 0ed2f24e21007d5e93b212f6f39833258ebe39cf Mon Sep 17 00:00:00 2001 From: Matthieu Patou Date: Sun, 28 Jun 2026 08:27:02 -0700 Subject: [PATCH 6/7] Preserve multiline load blocks --- postprocess | 50 +++++++++++++++++++++++++++++++++---- test/test_postprocess.py | 54 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 5 deletions(-) create mode 100644 test/test_postprocess.py diff --git a/postprocess b/postprocess index 00f3933..4f1dae3 100755 --- a/postprocess +++ b/postprocess @@ -108,11 +108,51 @@ def select(mapping, **kwargs): def load_lines(build_path: str) -> List[str]: - return [ - line - for line in Path(build_path).read_text(encoding="utf-8").splitlines() - if line.lstrip().startswith("load(") - ] + loads: List[str] = [] + in_load = False + paren_depth = 0 + + for line in Path(build_path).read_text(encoding="utf-8").splitlines(): + if in_load: + loads.append(line) + paren_depth += paren_delta(line) + if paren_depth <= 0: + in_load = False + continue + + if line.lstrip().startswith("load("): + loads.append(line) + paren_depth = paren_delta(line) + in_load = paren_depth > 0 + + return loads + + +def paren_delta(line: str) -> int: + delta = 0 + quote: Optional[str] = None + escaped = False + + for char in line: + if quote is not None: + if escaped: + escaped = False + elif char == "\\": + escaped = True + elif char == quote: + quote = None + continue + + if char == "#": + break + if char in {'"', "'"}: + quote = char + elif char == "(": + delta += 1 + elif char == ")": + delta -= 1 + + return delta def remember_triple_quoted_strings(source: str) -> None: diff --git a/test/test_postprocess.py b/test/test_postprocess.py new file mode 100644 index 0000000..fd82181 --- /dev/null +++ b/test/test_postprocess.py @@ -0,0 +1,54 @@ +import importlib.machinery +import importlib.util +import sys +import tempfile +import unittest +from pathlib import Path + + +REPO_ROOT = Path(__file__).resolve().parent.parent +POSTPROCESS_PATH = REPO_ROOT / "postprocess" + + +loader = importlib.machinery.SourceFileLoader("postprocess", str(POSTPROCESS_PATH)) +spec = importlib.util.spec_from_loader(loader.name, loader) +postprocess = importlib.util.module_from_spec(spec) +sys.modules[loader.name] = postprocess +loader.exec_module(postprocess) + + +class TestPostprocess(unittest.TestCase): + def test_load_lines_preserves_multiline_load_blocks(self): + with tempfile.TemporaryDirectory() as tmpdir: + build_file = Path(tmpdir) / "BUILD.bazel" + build_file.write_text( + "\n".join( + [ + 'load("@rules_cc//cc:defs.bzl", "cc_binary")', + "load(", + ' "@com_google_protobuf//bazel:cc_proto_library.bzl",', + ' "cc_proto_library",', + ")", + "", + "cc_binary(", + ' name = "app",', + ")", + ] + ), + encoding="utf-8", + ) + + self.assertEqual( + postprocess.load_lines(str(build_file)), + [ + 'load("@rules_cc//cc:defs.bzl", "cc_binary")', + "load(", + ' "@com_google_protobuf//bazel:cc_proto_library.bzl",', + ' "cc_proto_library",', + ")", + ], + ) + + +if __name__ == "__main__": + unittest.main() From 2113e5abef2984942907c8f11425c07c632c6fa2 Mon Sep 17 00:00:00 2001 From: Matthieu Patou Date: Mon, 29 Jun 2026 02:36:14 -0700 Subject: [PATCH 7/7] Fix un-needed location --- build.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/build.py b/build.py index bfbc464..3935ca4 100644 --- a/build.py +++ b/build.py @@ -1363,10 +1363,6 @@ def _handleCCProtobuf(self, ctx: BazelBuildVisitorContext, el: BuildTarget): proto_lib = self._getOrCreateProtoLibrary(ctx, el) t = self._getOrCreateCCProtoLibrary(ctx, el, proto_lib) - location = TopLevelGroupingStrategy().getBuildFilenamePath( - el, ctx.current.location if ctx.current else ctx.prefix - ) - if not isinstance(ctx.current, (BazelProtoLibrary, BazelCCProtoLibrary)): ctx.current.addDep(t) ctx.bazelbuild.bazelTargets.add(t)