build: remove dev_dependency = true from google_benchmark

[akonradi]

Commit Message:
envoy_cc_benchmark_binary produces targets that link //test/benchmark:main, which depends on the @benchmark (google_benchmark) rule. This macro is part of Envoy's public Bazel build API and is used by downstream repositories that consume Envoy as a bzlmod dependency (e.g. a git submodule) rather than as the build's root module.

Under bzlmod, a bazel_dep declared with dev_dependency=True is dropped from the dependency graph whenever the declaring module is not the root module of the current build. So any downstream repo building an envoy_cc_benchmark_binary target fails:

ERROR: no such package '@@[unknown repo 'benchmark' requested from @@envoy+]//': The repository '@@[unknown repo 'benchmark' requested from @@envoy+]' could not be resolved: No repository visible as '@benchmark' from repository '@@envoy'

Dropping dev_dependency lets @benchmark resolve regardless of which module is root, matching how the macro is actually consumed.

Additional Description:
I found this when attempting to define my own benchmark targets in a project that consumes Envoy as a submodule.

Risk Level: low
Testing: built targets in consuming project
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #47497 was opened by akonradi.

see: more, trace.

[phlax]

no real objection to this - but wonder if its something that would be better placed in downstream mods

fwiw - i pondered similar wrt the llvm toolchain - this is something that is not generally exposed as a non-build dep - in our case we kinda support a specific toolchain so i made some effort to workaround that

in the case of test deps im more inclined to think its something that downstreams should add if they wish to test it

[phlax]

ah - ok - just saw mention of macro - perhaps we could fix the macro to take a label - we have done that several other places for this purpose

[akonradi]

Note that while invoking the macro in my code is sufficient to demonstrate the issue, it isn't necessary. I'm also seeing similar errors when trying to run Envoy benchmarks from within my project:

bazel run -c opt @envoy//test/common/stats:recent_lookups_benchmark
...
ERROR: no such package '@@[unknown repo 'benchmark' requested from @@envoy+]//': The repository '@@[unknown repo 'benchmark' requested from @@envoy+]' could not be resolved: No repository visible as '@benchmark' from repository '@@envoy+'
ERROR: /home/coder/.cache/bazel/_bazel_abakon/355d0dec2e3175fe6c76e1aadc5e2c98/external/envoy+/test/common/stats/BUILD:95:26: no such package '@@[unknown repo 'benchmark' requested from @@envoy+]//': The repository '@@[unknown repo 'benchmark' requested from @@envoy+]' could not be resolved: No repository visible as '@benchmark' from repository '@@envoy+' and referenced by '@@envoy+//test/common/stats:recent_lookups_benchmark'
ERROR: Analysis of target '@@envoy+//test/common/stats:recent_lookups_benchmark' failed; build aborted: Analysis failed

I realized after writing the PR description that there was another error line (I omitted it because it looked duplicative):

ERROR: /home/coder/.cache/bazel/_bazel_abakon/355d0dec2e3175fe6c76e1aadc5e2c98/external/envoy+/test/benchmark/BUILD:11:22: no such package '@@[unknown repo 'benchmark' requested from @@envoy+]//': The repository '@@[unknown repo 'benchmark' requested from @@envoy+]' could not be resolved: No repository visible as '@benchmark' from repository '@@envoy+' and referenced by '@@envoy+//test/benchmark:main'

FWIW, the macro invocation in my repo looks like this - it sets repository already:

envoy_cc_benchmark_binary(
    name = "<benchmark_name>",
    srcs = ["<benchmark_name>.cc"],
    repository = "@envoy",
    deps = [ ... ],
)

The macro itself produces a rule that references an Envoy target, which I would expect to be able to see the @benchmark defined in that module, but that's not the case. I have no issues building and running its sibling test targets that don't depend on @benchmark.

[phlax]

setting repository wont help with bzlmod - we should probably strip any code like this out to reduce confusion

there is a pattern that can make this work i think involving Label that we have used elsewhere with macros - pretty sure it ccan be used here

im ooo rn - but will follow up as soon as i get chance

[akonradi]

Sounds good, I'm happy for this to get done the Right Way. I'd be happy to test any patches against my project if that's helpful.

[phlax]

cool, thanks

I have a PR here #47556 that should fix this properly (switches the macros to Label() so they resolve against @envoy regardless of which module is root, and adds external-consumer tests).

If you're up for testing it that would be amazing. Note: in the end I made it so the macro doesn't inject the @benchmark dep, so downstream you need to:

• add to your MODULE.bazel (the repo_name = "benchmark" is important since //test/benchmark:main.cc includes benchmark/benchmark.h):

  bazel_dep(name = "google_benchmark", version = "1.9.5", repo_name = "benchmark")
  bazel_dep(name = "tclap", version = "1.2.5")

• add "@benchmark" to deps of each envoy_cc_benchmark_binary target

@tclap is picked up by the macro from your repo mapping, so it only needs the bazel_dep. There's a working example in bazel/tests/external/ (BUILD + MODULE.bazel) on that branch.

im wondering whether we can make @benchmark work same as @tclap - it got split while i was testing/devving it - but probs we can add the (non-label) string here - ill follow up on this

[phlax]

i remembered why @benchmark was handled separately - it duplicates if you put it in the macro - and not easy to dedupe due to select handling somewhere - probs resolvable but that is why we currently need to add it everywhere

[akonradi]

Yep, that worked for me, and it looks like it's merged. Thanks!