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

Wed Nov 10 09:49:45 PST 2021

phawkins added inline comments.

================
Comment at: utils/bazel/llvm-project-overlay/mlir/BUILD.bazel:315
+
+CAPI_IR_HEADERS = [
+    "include/mlir-c/AffineExpr.h",
----------------
GMNGeoffrey wrote:
> Could this be a glob with an exclude for each of the things in a separate library? I think that would be easier to maintain
It could be, but note that's not something this PR changes: here we just move the list around. If we want to do that, can we do it in another PR?

================
Comment at: utils/bazel/llvm-project-overlay/mlir/BUILD.bazel:605
 cc_library(
     name = "CAPIExecutionEngine",
     srcs = ["lib/CAPI/ExecutionEngine/ExecutionEngine.cpp"],
----------------
GMNGeoffrey wrote:
> Why isn't this one also split?
It's not split because I'm personally not using it. But done.

================
Comment at: utils/bazel/llvm-project-overlay/mlir/BUILD.bazel:774
+    ],
+    alwayslink = True,
 )
----------------
mehdi_amini wrote:
> GMNGeoffrey wrote:
> > 😢 alwayslink is a scourge. Is it really the only way?
> Same concern here: why is this needed?
I removed it from this target, where I don't think it's needed. In general it *is* needed on all the libraries that implement the exported C API, because otherwise Bazel will strip the C API out.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113565