[PATCH] D138470: [bazel] Restore libpfm as a conditional dependency for exegesis.

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

GMNGeoffrey accepted this revision.
GMNGeoffrey added inline comments.
This revision is now accepted and ready to land.


================
Comment at: utils/bazel/WORKSPACE:15-16
     urls = [
-        "https://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/releases/download/{version}/bazel-skylib-{version}.tar.gz".format(version=SKYLIB_VERSION),
-        "https://github.com/bazelbuild/bazel-skylib/releases/download/{version}/bazel-skylib-{version}.tar.gz".format(version=SKYLIB_VERSION),
+        "https://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/releases/download/{version}/bazel-skylib-{version}.tar.gz".format(version = SKYLIB_VERSION),
+        "https://github.com/bazelbuild/bazel-skylib/releases/download/{version}/bazel-skylib-{version}.tar.gz".format(version = SKYLIB_VERSION),
     ],
----------------
This formatting looks wrong to me. These are kwargs


================
Comment at: utils/bazel/llvm-project-overlay/llvm/BUILD.bazel:2693
+    name = "maybe_pfm",
+    # We want dependencies of this library to have -DHAVE_LIBPFM conditionally
+    # defined, so we set `defines` instead of `copts`.
----------------
Do we want all transitive dependencies to have that defined? It might be better to have the selects in more places and add it in `local_defines` (note not copts)


================
Comment at: utils/bazel/llvm-project-overlay/llvm/BUILD.bazel:2707
+
 cc_library(
     name = "Exegesis",
----------------
gchatelet wrote:
> AFAIU it doesn't make sense to build `Exegesis` without `libpfm` so I would do something similar to
> https://github.com/llvm/llvm-project/blob/ee82b864f2086f944f046bd00b03f30697403f8a/utils/bazel/llvm-project-overlay/libc/utils/MPFRWrapper/BUILD.bazel#L13-L16
> 
> This makes sure that the target is skipped when using wildcards (which is the case on CI).
I think it's fine to have it without? That's what we had before this patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138470