[PATCH] D107414: [Bazel] Add support for lld

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

GMNGeoffrey added inline comments.


================
Comment at: utils/bazel/llvm-project-overlay/lld/BUILD.bazel:70
+        "ELF/*.cpp",
+        "ELF/*.h",
+        "ELF/Arch/*.cpp",
----------------
MaskRay wrote:
> Is `*.h` needed in `srcs`?
hdrs is only for headers that are exported for dependent libraries. Other headers should be declared in srcs:

```
Headers not meant to be included by a client of this library should be listed in the srcs attribute instead, even if they are included by a published header.
```

https://docs.bazel.build/versions/main/be/c-cpp.html#cc_library.hdrs

Actually I noticed that the MachO2 headers should move to srcs as well.


================
Comment at: utils/bazel/llvm-project-overlay/lld/BUILD.bazel:147
+        "//llvm:TransformUtils",
+        "//llvm:WindowsManifest",
+    ],
----------------
MaskRay wrote:
> In CMake, with `LLVM_ENABLE_LIBXML2` this dependency is essentially a no-op. Can Bazel allow optional builds?
I don't totally understand the question. What do you mean by optional builds? There's configurability based on select


================
Comment at: utils/bazel/llvm-project-overlay/lld/BUILD.bazel:239
+cc_library(
+    name = "MachO2",
+    srcs = glob(["MachO/**/*.cpp"]),
----------------
MaskRay wrote:
> Just `MachO`
> 
> The special name can go to the old port which will go away soon.
This matches the name in the CMake file (as do all the library names). I'd prefer not to diverge. Having these match makes it much easier to compare the Bazel and CMake rules. I'm happy to update this once the CMake name changes.


================
Comment at: utils/bazel/llvm-project-overlay/lld/BUILD.bazel:304
+    name = "wasm_options_inc_gen",
+    # Bazel seems to be broken when you try to include generated files from the
+    # same directory using only their basename, so we have to strip the prefix.
----------------
MaskRay wrote:
> This comment is repeated multiple times. Is there a Bazel bug or anything we can fix in this file?
Filed https://github.com/bazelbuild/bazel/issues/13803 and linked to it instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107414