bazel_to_cmake: Simplify proto aspect building slightly.

As part of fixing proto aspect builds, the last change added an additional CMake interface library. This removes that by adding the generated .h files to the
common aspect library.

This also removes btc_protobuf.cmake use, as the proto compile calls are now
emitted directly without using the older macros.

PiperOrigin-RevId: 676555369
Change-Id: I0da06ad30ba6143a4311d31c849ce1997c1adf79

CMakeLists.txt

@@ -78,7 +78,6 @@ if(MSVC AND CMAKE_CXX_COMPILER_LAUNCHER MATCHES "sccache")
 endif()
 
 include(bazel_to_cmake)
-include(btc_protobuf)
 
 bazel_to_cmake(
   --cmake-project-name tensorstore

tools/cmake/bazel_to_cmake/bzl_library/grpc_generate_cc.py

@@ -15,7 +15,6 @@
 
 # pylint: disable=invalid-name,missing-function-docstring,relative-beyond-top-level,g-long-lambda
 import io
-import pathlib
 from typing import Any, List, Optional, cast
 
 from ..cmake_builder import CMakeBuilder
@@ -25,6 +24,7 @@
 from ..native_aspect_proto import btc_protobuf
 from ..native_aspect_proto import plugin_generated_files
 from ..native_aspect_proto import PluginSettings
+from ..native_aspect_proto import maybe_augment_output_dir
 from ..starlark.bazel_globals import BazelGlobals
 from ..starlark.bazel_globals import register_bzl_library
 from ..starlark.bazel_target import RepositoryId
@@ -166,16 +166,11 @@ def _generate_grpc_cc_impl(
   state.collect_deps(UPB_PLUGIN.aspectdeps(resolved_srcs[0]))
 
   # Augment output with strip import prefix
-  output_dir = repo.cmake_binary_dir
-  if proto_library_provider.strip_import_prefix:
-    include_path = str(
-        pathlib.PurePosixPath(_context.caller_package_id.package_name).joinpath(
-            proto_library_provider.strip_import_prefix
-        )
-    )
-    if include_path[0] == "/":
-      include_path = include_path[1:]
-    output_dir = output_dir.joinpath(include_path)
+  output_dir = repo.replace_with_cmake_macro_dirs([
+      maybe_augment_output_dir(
+          _context, proto_library_provider, repo.cmake_binary_dir
+      )
+  ])[0]
 
   # Emit.
   out = io.StringIO()

tools/cmake/bazel_to_cmake/emit_cc.py

@@ -68,6 +68,7 @@ def _emit_cc_common_options(
     extra_public_compile_options: Optional[Iterable[str]] = None,
     interface_only: bool = False,
     srcs: Optional[Iterable[str]] = None,
+    public_srcs: Optional[Iterable[str]] = None,
     **kwargs,
 ):
   """Emits CMake rules for common C++ target options."""
@@ -134,18 +135,31 @@ def _emit_cc_common_options(
   )
   if add_dependencies:
     out.write(
-        f"add_dependencies({target_name} {quote_list(sorted(add_dependencies))})\n"
+        f"add_dependencies({target_name} "
+        f"{quote_list(sorted(add_dependencies))})\n"
     )
   if extra_public_compile_options:
     out.write(
-        f"target_compile_options({target_name} {public_context} {quote_list(extra_public_compile_options)})\n"
+        f"target_compile_options({target_name} {public_context} "
+        f"{quote_list(extra_public_compile_options)})\n"
     )
-  if srcs:
+
+  if srcs or public_srcs:
     non_header_srcs = partition_by(srcs, pattern=_HEADER_SRC_PATTERN)[1]
-    out.write(
-        f"target_sources({target_name} PRIVATE{_SEP}{quote_path_list(non_header_srcs , separator=_SEP)})\n"
-    )
-    asm_srcs = partition_by(non_header_srcs, pattern=_ASM_SRC_PATTERN)[0]
+    asm_srcs = partition_by(
+        itertools.chain(non_header_srcs, public_srcs or []),
+        pattern=_ASM_SRC_PATTERN,
+    )[0]
+    if public_srcs:
+      out.write(
+          f"target_sources({target_name} PUBLIC{_SEP}"
+          f"{quote_path_list(public_srcs , separator=_SEP)})\n"
+      )
+    if non_header_srcs:
+      out.write(
+          f"target_sources({target_name} PRIVATE{_SEP}"
+          f"{quote_path_list(non_header_srcs , separator=_SEP)})\n"
+      )
     if asm_srcs:
       if asm_dialect is None:
         raise ValueError(
@@ -491,7 +505,7 @@ def emit_cc_test(
   out.write(
       f"add_test(NAME {target_name}\n"
       f"      COMMAND {target_name}{args_suffix}\n"
-      f"      WORKING_DIRECTORY ${{CMAKE_CURRENT_SOURCE_DIR}})\n"
+      "      WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})\n"
   )
   if properties:
     out.write(f"set_tests_properties({target_name} PROPERTIES\n")

tools/cmake/bazel_to_cmake/native_aspect_proto.py

@@ -50,7 +50,6 @@
 from .cmake_target import CMakeTarget
 from .emit_cc import emit_cc_library
 from .emit_cc import handle_cc_common_options
-from .emit_filegroup import emit_filegroup
 from .evaluation import EvaluationState
 from .provider_util import ProviderCollection
 from .starlark.bazel_target import RepositoryId
@@ -181,6 +180,24 @@ def _assert_is_proto(src: TargetId):
   ), f"{src.as_label()} must end in .proto"
 
 
+def maybe_augment_output_dir(
+    _context: InvocationContext,
+    proto_library_provider: ProtoLibraryProvider,
+    output_dir: pathlib.PurePath,
+):
+  """If necessary, augment the output_dir with the strip_import_prefix."""
+  if proto_library_provider.strip_import_prefix:
+    include_path = str(
+        pathlib.PurePosixPath(_context.caller_package_id.package_name).joinpath(
+            proto_library_provider.strip_import_prefix
+        )
+    )
+    if include_path[0] == "/":
+      include_path = include_path[1:]
+    return output_dir.joinpath(include_path)
+  return output_dir
+
+
 def singleproto_aspect_target(
     src: TargetId,
     plugin_settings: PluginSettings,
@@ -269,27 +286,10 @@ def aspect_genproto_singleproto(
     )
     generated_files.append(t[1])
 
-  _context.add_analyzed_target(
-      aspect_target,
-      TargetInfo(
-          *default_providers(cmake_target_pair),
-          FilesProvider(generated_files),
-      ),
+  output_dir = maybe_augment_output_dir(
+      _context, proto_library_provider, output_dir
   )
 
-  # Augment output with strip import prefix
-  if proto_library_provider.strip_import_prefix:
-    include_path = str(
-        pathlib.PurePosixPath(_context.caller_package_id.package_name).joinpath(
-            proto_library_provider.strip_import_prefix
-        )
-    )
-    if include_path[0] == "/":
-      include_path = include_path[1:]
-    output_dir = output_dir.joinpath(include_path)
-
-  relative_includes = repo.replace_with_cmake_macro_dirs([output_dir])
-
   # Emit.
   out = io.StringIO()
   out.write(
@@ -304,34 +304,31 @@ def aspect_genproto_singleproto(
       proto_src=proto_src_files[0],
       generated_files=generated_files,
       import_targets=import_targets,
-      cmake_depends=src_collector.add_dependencies(),
-      output_dir=relative_includes[0],
+      cmake_depends=sorted(src_collector.add_dependencies()),
+      output_dir=repo.replace_with_cmake_macro_dirs([output_dir])[0],
   )
 
   sep = "\n    "
   quoted_outputs = quote_path_list(
       repo.replace_with_cmake_macro_dirs(generated_files), sep
   )
   out.write(
-      f"add_custom_target(genrule_{cmake_target_pair.target} DEPENDS{sep}{quoted_outputs})\n"
-  )
-  # NOTE: We probably don't need this file group once the aspect target
-  # includes .h files as INTERFACE sources to ensure that they are generated.
-  # Without that, CMake had trouble finding the files properly, but that
-  # may have been by attempted includes across directories (.pb.h):
-  # https://gitlab.kitware.com/cmake/cmake/-/issues/18399
-  emit_filegroup(
-      out,
-      cmake_name=cmake_target_pair.target,
-      filegroup_files=generated_files,
-      source_directory=repo.source_directory,
-      cmake_binary_dir=output_dir,
-      add_dependencies=[CMakeTarget("genrule__" + cmake_target_pair.target)],
-      includes=relative_includes,
+      f"add_custom_target({cmake_target_pair.target} DEPENDS{sep}{quoted_outputs})\n"
   )
-
   _context.access(CMakeBuilder).addtext(out.getvalue())
 
+  _context.add_analyzed_target(
+      aspect_target,
+      TargetInfo(
+          *make_providers(
+              cmake_target_pair,
+              CMakeAddDependenciesProvider,
+              CMakePackageDepsProvider,
+          ),
+          FilesProvider(generated_files),
+      ),
+  )
+
 
 ##############################################################################
 
@@ -406,6 +403,10 @@ def aspect_genproto_library_target(
         output_dir=output_dir,
     )
 
+  aspect_dir = repo.replace_with_cmake_macro_dirs(
+      [maybe_augment_output_dir(_context, proto_library_provider, output_dir)]
+  )[0]
+
   # Now emit a cc_library for each proto_library
   srcs_collector = state.collect_targets(aspect_srcs)
   split_srcs = partition_by(
@@ -427,6 +428,9 @@ def aspect_genproto_library_target(
   )
   del common_options["private_includes"]
 
+  if aspect_dir not in common_options["includes"]:
+    common_options["includes"].append(aspect_dir)
+
   out = io.StringIO()
   out.write(
       f"\n# {target.as_label()}"
@@ -436,6 +440,7 @@ def aspect_genproto_library_target(
       out,
       cmake_target_pair,
       hdrs=split_srcs[0],
+      public_srcs=repo.replace_with_cmake_macro_dirs(sorted(split_srcs[0])),
       **common_options,
   )
   _context.access(CMakeBuilder).addtext(out.getvalue())

tools/cmake/bazel_to_cmake/testdata/grpc_generate_cc/golden/_srcdir/build_rules.cmake

@@ -30,16 +30,9 @@ COMMENT "Running protoc upb on ${TEST_SRCDIR}/c.proto"
 COMMAND_EXPAND_LISTS
 VERBATIM
 )
-add_custom_target(genrule_CMakeProject_aspect_upb__71950a86 DEPENDS
+add_custom_target(CMakeProject_aspect_upb__71950a86 DEPENDS
     "${PROJECT_BINARY_DIR}/_gen_upb/c.upb.h"
     "${PROJECT_BINARY_DIR}/_gen_upb/c.upb.c")
-add_library(CMakeProject_aspect_upb__71950a86 INTERFACE)
-target_sources(CMakeProject_aspect_upb__71950a86 INTERFACE
-    "${TEST_BINDIR}/_gen_upb/c.upb.c"
-    "${TEST_BINDIR}/_gen_upb/c.upb.h")
-target_include_directories(CMakeProject_aspect_upb__71950a86 INTERFACE
-    "${PROJECT_BINARY_DIR}/_gen_upb")
-add_dependencies(CMakeProject_aspect_upb__71950a86 genrule__CMakeProject_aspect_upb__71950a86)
 
 # @grpc_generate_cc_test_repo//:aspect_upb_minitable__71950a86
 # genproto upb_minitable @grpc_generate_cc_test_repo//:c.proto
@@ -55,23 +48,16 @@ COMMAND $<TARGET_FILE:protobuf::protoc>
     "--upb_minitable_out=${PROJECT_BINARY_DIR}/_gen_upb_minitable"
     "${TEST_SRCDIR}/c.proto"
 DEPENDS
+    "protobuf::protoc_gen_upb_minitable_stage1"
     "${TEST_SRCDIR}/c.proto"
     "protobuf::protoc"
-    "protobuf::protoc_gen_upb_minitable_stage1"
 COMMENT "Running protoc upb_minitable on ${TEST_SRCDIR}/c.proto"
 COMMAND_EXPAND_LISTS
 VERBATIM
 )
-add_custom_target(genrule_CMakeProject_aspect_upb_minitable__71950a86 DEPENDS
+add_custom_target(CMakeProject_aspect_upb_minitable__71950a86 DEPENDS
     "${PROJECT_BINARY_DIR}/_gen_upb_minitable/c.upb_minitable.h"
     "${PROJECT_BINARY_DIR}/_gen_upb_minitable/c.upb_minitable.c")
-add_library(CMakeProject_aspect_upb_minitable__71950a86 INTERFACE)
-target_sources(CMakeProject_aspect_upb_minitable__71950a86 INTERFACE
-    "${TEST_BINDIR}/_gen_upb_minitable/c.upb_minitable.c"
-    "${TEST_BINDIR}/_gen_upb_minitable/c.upb_minitable.h")
-target_include_directories(CMakeProject_aspect_upb_minitable__71950a86 INTERFACE
-    "${PROJECT_BINARY_DIR}/_gen_upb_minitable")
-add_dependencies(CMakeProject_aspect_upb_minitable__71950a86 genrule__CMakeProject_aspect_upb_minitable__71950a86)
 
 # @grpc_generate_cc_test_repo//:c_proto__minitable_library
 # aspect upb_minitable @grpc_generate_cc_test_repo//:c_proto
@@ -84,6 +70,9 @@ target_link_libraries(CMakeProject_c_proto__minitable_library PUBLIC
 target_include_directories(CMakeProject_c_proto__minitable_library PUBLIC
         "$<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/_gen_upb_minitable>")
 target_compile_features(CMakeProject_c_proto__minitable_library PUBLIC cxx_std_17)
+add_dependencies(CMakeProject_c_proto__minitable_library "CMakeProject_aspect_upb_minitable__71950a86")
+target_sources(CMakeProject_c_proto__minitable_library PUBLIC
+        "${PROJECT_BINARY_DIR}/_gen_upb_minitable/c.upb_minitable.h")
 target_sources(CMakeProject_c_proto__minitable_library PRIVATE
         "${PROJECT_BINARY_DIR}/_gen_upb_minitable/c.upb_minitable.c")
 add_library(CMakeProject::c_proto__minitable_library ALIAS CMakeProject_c_proto__minitable_library)
@@ -100,6 +89,9 @@ target_link_libraries(CMakeProject_c_proto__upb_library PUBLIC
 target_include_directories(CMakeProject_c_proto__upb_library PUBLIC
         "$<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/_gen_upb>")
 target_compile_features(CMakeProject_c_proto__upb_library PUBLIC cxx_std_17)
+add_dependencies(CMakeProject_c_proto__upb_library "CMakeProject_aspect_upb__71950a86")
+target_sources(CMakeProject_c_proto__upb_library PUBLIC
+        "${PROJECT_BINARY_DIR}/_gen_upb/c.upb.h")
 target_sources(CMakeProject_c_proto__upb_library PRIVATE
         "${PROJECT_BINARY_DIR}/_gen_upb/c.upb.c")
 add_library(CMakeProject::c_proto__upb_library ALIAS CMakeProject_c_proto__upb_library)
@@ -116,9 +108,9 @@ COMMAND $<TARGET_FILE:protobuf::protoc>
     "--grpc_out=services_namespace=grpc_gen:${PROJECT_BINARY_DIR}"
     "${TEST_SRCDIR}/c.proto"
 DEPENDS
+    "gRPC::grpc_cpp_plugin"
     "${TEST_SRCDIR}/c.proto"
     "protobuf::protoc"
-    "gRPC::grpc_cpp_plugin"
 COMMENT "Running protoc grpc on ${TEST_SRCDIR}/c.proto"
 COMMAND_EXPAND_LISTS
 VERBATIM
@@ -176,16 +168,9 @@ COMMENT "Running protoc cpp on ${TEST_SRCDIR}/c.proto"
 COMMAND_EXPAND_LISTS
 VERBATIM
 )
-add_custom_target(genrule_CMakeProject_aspect_cpp__71950a86 DEPENDS
+add_custom_target(CMakeProject_aspect_cpp__71950a86 DEPENDS
     "${PROJECT_BINARY_DIR}/_gen_cpp/c.pb.h"
     "${PROJECT_BINARY_DIR}/_gen_cpp/c.pb.cc")
-add_library(CMakeProject_aspect_cpp__71950a86 INTERFACE)
-target_sources(CMakeProject_aspect_cpp__71950a86 INTERFACE
-    "${TEST_BINDIR}/_gen_cpp/c.pb.cc"
-    "${TEST_BINDIR}/_gen_cpp/c.pb.h")
-target_include_directories(CMakeProject_aspect_cpp__71950a86 INTERFACE
-    "${PROJECT_BINARY_DIR}/_gen_cpp")
-add_dependencies(CMakeProject_aspect_cpp__71950a86 genrule__CMakeProject_aspect_cpp__71950a86)
 
 # @grpc_generate_cc_test_repo//:c_proto__cpp_library
 # aspect cpp @grpc_generate_cc_test_repo//:c_proto
@@ -198,6 +183,9 @@ target_link_libraries(CMakeProject_c_proto__cpp_library PUBLIC
 target_include_directories(CMakeProject_c_proto__cpp_library PUBLIC
         "$<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/_gen_cpp>")
 target_compile_features(CMakeProject_c_proto__cpp_library PUBLIC cxx_std_17)
+add_dependencies(CMakeProject_c_proto__cpp_library "CMakeProject_aspect_cpp__71950a86")
+target_sources(CMakeProject_c_proto__cpp_library PUBLIC
+        "${PROJECT_BINARY_DIR}/_gen_cpp/c.pb.h")
 target_sources(CMakeProject_c_proto__cpp_library PRIVATE
         "${PROJECT_BINARY_DIR}/_gen_cpp/c.pb.cc")
 add_library(CMakeProject::c_proto__cpp_library ALIAS CMakeProject_c_proto__cpp_library)
@@ -216,23 +204,16 @@ COMMAND $<TARGET_FILE:protobuf::protoc>
     "--upbdefs_out=${PROJECT_BINARY_DIR}/_gen_upbdefs"
     "${TEST_SRCDIR}/c.proto"
 DEPENDS
-    "${TEST_SRCDIR}/c.proto"
     "protobuf::protoc_gen_upbdefs"
+    "${TEST_SRCDIR}/c.proto"
     "protobuf::protoc"
 COMMENT "Running protoc upbdefs on ${TEST_SRCDIR}/c.proto"
 COMMAND_EXPAND_LISTS
 VERBATIM
 )
-add_custom_target(genrule_CMakeProject_aspect_upbdefs__71950a86 DEPENDS
+add_custom_target(CMakeProject_aspect_upbdefs__71950a86 DEPENDS
     "${PROJECT_BINARY_DIR}/_gen_upbdefs/c.upbdefs.h"
     "${PROJECT_BINARY_DIR}/_gen_upbdefs/c.upbdefs.c")
-add_library(CMakeProject_aspect_upbdefs__71950a86 INTERFACE)
-target_sources(CMakeProject_aspect_upbdefs__71950a86 INTERFACE
-    "${TEST_BINDIR}/_gen_upbdefs/c.upbdefs.c"
-    "${TEST_BINDIR}/_gen_upbdefs/c.upbdefs.h")
-target_include_directories(CMakeProject_aspect_upbdefs__71950a86 INTERFACE
-    "${PROJECT_BINARY_DIR}/_gen_upbdefs")
-add_dependencies(CMakeProject_aspect_upbdefs__71950a86 genrule__CMakeProject_aspect_upbdefs__71950a86)
 
 # @grpc_generate_cc_test_repo//:c_proto__upbdefs_library
 # aspect upbdefs @grpc_generate_cc_test_repo//:c_proto
@@ -247,6 +228,9 @@ target_link_libraries(CMakeProject_c_proto__upbdefs_library PUBLIC
 target_include_directories(CMakeProject_c_proto__upbdefs_library PUBLIC
         "$<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/_gen_upbdefs>")
 target_compile_features(CMakeProject_c_proto__upbdefs_library PUBLIC cxx_std_17)
+add_dependencies(CMakeProject_c_proto__upbdefs_library "CMakeProject_aspect_upbdefs__71950a86")
+target_sources(CMakeProject_c_proto__upbdefs_library PUBLIC
+        "${PROJECT_BINARY_DIR}/_gen_upbdefs/c.upbdefs.h")
 target_sources(CMakeProject_c_proto__upbdefs_library PRIVATE
         "${PROJECT_BINARY_DIR}/_gen_upbdefs/c.upbdefs.c")
 add_library(CMakeProject::c_proto__upbdefs_library ALIAS CMakeProject_c_proto__upbdefs_library)