google / benchmark

A microbenchmark support library
Apache License 2.0
8.61k stars 1.57k forks source link

Add a CI test for the new bzlmod integration #1617

Closed macandy13 closed 1 year ago

macandy13 commented 1 year ago

Enhanced the already existing bazel test to also run with the --enable_bzlmod flag.

Technically it shouldn't be necessary to run these additional test on all operating systems, but this will make the eventual switch to exclusively use bzlmod (slightly) easier.

The one thing that is unclear is if there should be another test for the --define pfm=1 flag, which uses more libraries. This is missing for the base case and is currently only exercises in the cmake CI workflows, so it might be worth a separate PR to introduce those tests (for both variants then).

Related issue: https://github.com/bazelbuild/bazel-central-registry/issues/384

dmah42 commented 1 year ago

agreed, we should probably test pfm, but not in this PR.

i wonder if there's a way to simplify this by having a matrix-level setting for bzlmod and enabling or disabling the flag based on that?

macandy13 commented 1 year ago

Good point, I will dig a bit deeper into the syntax and update the pr.

dominic @.***> schrieb am Fr., 30. Juni 2023, 10:59:

agreed, we should probably test pfm, but not in this PR.

i wonder if there's a way to simplify this by having a matrix-level setting for bzlmod and enabling or disabling the flag based on that?

— Reply to this email directly, view it on GitHub https://github.com/google/benchmark/pull/1617#issuecomment-1614354986, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHQC2P574R5BREJMCPVPPLLXN2IPJANCNFSM6AAAAAAZZNY7D4 . You are receiving this because you authored the thread.Message ID: @.***>

macandy13 commented 1 year ago

Ok, I think I have a nice solution now (although the "&& ||" trick for emulating a ternary condition is definitely debatable ;). Also tried to incorporate pfm but got hit by the failing test with pfm-enabled, so I'll probably keep that for another PR where the test would be fixed as well.

PTAL.

dmah42 commented 1 year ago

i need to update the "expected tests" to cover these too. merging without waiting for the non-existant tests.