[PATCH] D107714: Simplify setting up LLVM as bazel external repo

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

GMNGeoffrey added a comment.

Nice! This is much better. I think some of the workspace munging came from when this rule was more general (overlay any directory over any other). I still think such functionality is useful, but not at the cost of making this API really hard to use.

A few nits though. Sorry I was out and couldn't review :-)



================
Comment at: utils/bazel/configure.bzl:36
 
-    overlay_script = repository_ctx.path(
-        repository_ctx.attr._overlay_script,
-    )
+    src_path = repository_ctx.path(Label("//:WORKSPACE")).dirname
+    bazel_path = src_path.get_child("utils").get_child("bazel")
----------------
I think this could just be `repository_ctx.path(".")`


================
Comment at: utils/bazel/examples/http_archive/WORKSPACE:12
 # Replace with the LLVM commit you want to use.
 LLVM_COMMIT = "09ac97ce350316b95b8cddb796d52f71b6f68296"
 
----------------
We should update the commit here so it's one that includes this change. For future reference, I think it would've been better to leave this example as-is, since it is still a working example for the given LLVM commit and then follow up with an update for your commit (since it's impossible to know the commit hash before it's committed)


================
Comment at: utils/bazel/examples/http_archive/WORKSPACE:13
-
-http_archive(
-    name = "bazel_skylib",
----------------
bazel_skylib is still a required dependency for tblgen, although we could go back to manual path manipulation to avoid the dependency


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107714