[PATCH] D113565: Split headers from implementations in MLIR C API Bazel build.

[Peter Hawkins via Phabricator via llvm-commits]

phawkins added a comment.

Please take a look.



================
Comment at: utils/bazel/llvm-project-overlay/mlir/BUILD.bazel:587
+
+cc_library(
+    name = "MLIRBindingsPythonHeaders",
----------------
GMNGeoffrey wrote:
> Not necessary for this patch since the other target already exists, but we should be able to pull these targets together and use the macro here as well, right?
You could, but I'm not convinced it's a good idea in this case. Here we actually do *not* want alwayslink (this is just a normal library: the only important part is which variant of the C API we link to), so the macro is actually not helpful.


================
Comment at: utils/bazel/llvm-project-overlay/mlir/BUILD.bazel:666
+cc_library(
+    name = "MLIRBindingsPythonCAPIDeps",
+    tags = [
----------------
GMNGeoffrey wrote:
> I'm confused why this target exists. Are these ones meaningfully a different pattern than the others such that they can't use the same macro?
This exists to bundle together the C API deps needed by the Python bindings. A key point is that the deps are different in all three targets (header-only, deps, deps-for-shared-object), and we never need alwayslink. So the macro doesn't apply.


================
Comment at: utils/bazel/llvm-project-overlay/mlir/BUILD.bazel:684
+cc_library(
+    name = "MLIRBindingsPythonCAPIObjects",
+    tags = [
----------------
GMNGeoffrey wrote:
> You don't want alwayslink here?
It's not needed. It's sufficient to have it on the `cc_library` targets that define the symbols we want to make into the final linker command.


================
Comment at: utils/bazel/llvm-project-overlay/mlir/build_defs.bzl:34
+        header_deps = [],
+        **kw):
+    """Macro that generates three targets for MLIR C API libraries.
----------------
GMNGeoffrey wrote:
> Nit: I think **kwargs would be standard? Searching inside google `**kwargs` is a few orders of magnitude more common in bzl files (and an order of magnitude more common in py files)
I think both are pretty common, and my practice is always to prefer to save a few characters. But I don't feel strongly about it; done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113565/new/

https://reviews.llvm.org/D113565