[workspace] Upgrade googlebenchmark to v1.7.1 #18457
Conversation
|
@drake-jenkins-bot linux-focal-unprovisioned-clang-bazel-experimental-debug please. |
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-bazel-experimental-release please. |
There was a problem hiding this comment.
+@jwnimmer-tri for design review, at your leisure. I'm in no hurry.
+(priority: low)
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @jwnimmer-tri)
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @jwnimmer-tri)
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-bazel-experimental-release please.
@drake-jenkins-bot mac-x86-monterey-clang-bazel-experimental-release please.
So far, so good.
jwnimmer-tri
added
the
release notes: fix
There was a problem hiding this comment.
+(release notes: fix)
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: 6 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @rpoyner-tri)
a discussion (no related file):
nit The PR title (to be used as the subject when merging) needs improvement.
-- commits line 9 at r1:
The commits are not independent. This PR should just be 1 commit, so that (1) it's easier to revert the whole thing if necessary and (2) your reviewer doesn't need to manually test whether CI would have passed at the halfway point between the two commits. (For multi-commit PRs, it's incumbent upon the reviewer to ensure that all commits build on their own, unless the author has disclaimed that they won't build on their own e.g. when trying to preserve git history tracking.)
tools/workspace/libpfm/repository.bzl line 3 at r1 (raw file):
# -*- mode: python -*- # vi: set ft=python :
nit Either here, or in package.BUILD.bazel we should have a comment to explain what's going on. (That googlebenchmark needs a definition of libpfm at analysis-time, but does not use it at build-time, so we're stubbing it out with something that will load correctly, but would fail to build in case it were actually used.)
tools/workspace/libpfm/repository.bzl line 12 at r1 (raw file):
libpfm_repository = repository_rule( attrs = { "modname": attr.string(default = "libpfm"),
nit To my eye, "modname" here is dead code, and so the entire attrs stanza should be removed.
tools/workspace/libpfm/repository.bzl line 14 at r1 (raw file):
"modname": attr.string(default = "libpfm"), }, local = True,
nit "local=True" is wrong. There is no need to re-run this rule when the dependency graph changes. This line should be removed.
tools/workspace/libpfm/repository.bzl line 15 at r1 (raw file):
}, local = True, configure = True,
nit "configure=True" is wrong. This rule does not inspect the host system. This line should be removed.
Also add empty stub @libpfm, for googlebenchmark. Between v1.7.0 and v1.7.1, googlebenchmark added an optional dependency on @libpfm. This patch just satisfies the dependency graph at fetch time, without providing any actual code for @libpfm.
rpoyner-tri
force-pushed
the
benchmark-update-min
branch
from
0dab9af to
010c17c
Compare
rpoyner-tri
changed the title
There was a problem hiding this comment.
+@xuchenhan-tri for platform review, please (as a follow-up from #18447).
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform) (waiting on @xuchenhan-tri)
|
FYI, I’m out of office this week with very limited access to internet. Please expect some delay on the review. |
There was a problem hiding this comment.
platform
Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),xuchenhan-tri(platform) (waiting on @rpoyner-tri)
tools/workspace/libpfm/repository.bzl line 3 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Either here, or in
package.BUILD.bazelwe should have a comment to explain what's going on. (That googlebenchmark needs a definition of libpfm at analysis-time, but does not use it at build-time, so we're stubbing it out with something that will load correctly, but would fail to build in case it were actually used.)
TIL, thanks.
Closes #18435.
This change is