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

[Geoffrey Martin-Noble via Phabricator via llvm-commits]

GMNGeoffrey added a subscriber: stellaraccident.
GMNGeoffrey added inline comments.


================
Comment at: utils/bazel/llvm-project-overlay/mlir/BUILD.bazel:315
+
+CAPI_IR_HEADERS = [
+    "include/mlir-c/AffineExpr.h",
----------------
phawkins wrote:
> 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?
Separate patch SGTM. I only mentioned it because you were already moving this list around.


================
Comment at: utils/bazel/llvm-project-overlay/mlir/BUILD.bazel:774
+    ],
+    alwayslink = True,
 )
----------------
phawkins wrote:
> 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.
Why wasn't it needed before? We're using this internally without issue IIUC. We've worked hard in many places to scour this from the codebase, so I don't love seeing it return

@stellaraccident can you weigh in on these build rules?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113565