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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

In D138470#3943208 <https://reviews.llvm.org/D138470#3943208>, @gchatelet wrote:

> FTR D119547 <https://reviews.llvm.org/D119547> uses `external` as the default policy which builds from source.

Made it the default for this too, then.



================
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),
     ],
----------------
GMNGeoffrey wrote:
> This formatting looks wrong to me. These are kwargs
Weird, buildifier did this.

Since starlark is sort-of-but-not-really-python this might actually be the expected format. I'll just undo it for now though.


================
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`.
----------------
GMNGeoffrey wrote:
> 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)
I personally think it makes sense to have this apply transitively. This is basically a shim config target that either says "I have pfm" or "I am missing pfm", so advertising that pervasively is an intended goal. If this causes a build target downstream to behave differently, it's because it has a "#ifdef HAVE_LIBPFM" somewhere, and it should be depending on this anyway.

I think the warning that this applies transitively and recommending that one use local defines is in case you build one target with "-UNDEBUG" and then accidentally make all your dependencies run slower because debug mode is enabled everywhere. That's good general advice but not what's happening here.

If we don't do it this way, then we have to:
1. Set copts on "Exegesis"
2. Set copts and linkopts on "llvm-exegesis"
3. Set copts and linkopts on "llvm_exegesis_tests"

Not a whole lot to do if we go down that road, but I think this is simpler.


================
Comment at: utils/bazel/llvm-project-overlay/llvm/BUILD.bazel:2707
+
 cc_library(
     name = "Exegesis",
----------------
gchatelet wrote:
> GMNGeoffrey wrote:
> > 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
> No strong opinion here, I'm fine with the status quo.
I'm leaning toward leaving it like this too just because that's what we currently have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138470