[tools] Add test_alt_binder option to drake_py_test#24767
Conversation
jwnimmer-tri
left a comment
There was a problem hiding this comment.
+a:@rpoyner-tri for feature review, please.
@jwnimmer-tri reviewed 17 files and all commit messages, and made 3 comments.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers (waiting on jwnimmer-tri and rpoyner-tri).
a discussion (no related file):
Working
Possible merge-correctness race with #24766. Don't merge them both at the same time without rebasing and re-testing first.
bindings/pydrake/BUILD.bazel line 71 at r1 (raw file):
}), visibility = ["//visibility:public"], deps = select({
FYI While this change is not strictly necessary for the correctness of this PR, I'm including it here to maintain the invariant that the deps here on pydrake_pybind.h should exactly match the binding library of a pybind_py_library.
rpoyner-tri
left a comment
There was a problem hiding this comment.
@rpoyner-tri reviewed 17 files and all commit messages, and made 2 comments.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers (waiting on jwnimmer-tri).
tools/flags/internal/multi_config.bzl line 11 at r1 (raw file):
# of the same `bazel test` command as tests with the default binder setting. # # python_binder_reset is like an alias() rule except that the clears the
typo? grammar?
Suggestion:
that it clears7095f42 to
9160e1d
Compare
jwnimmer-tri
left a comment
There was a problem hiding this comment.
+a:@xuchen-han for platform review per schedule, please.
@jwnimmer-tri reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchen-han(platform) (waiting on xuchen-han).
rpoyner-tri
left a comment
There was a problem hiding this comment.
@rpoyner-tri reviewed 1 file and all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchen-han(platform) (waiting on xuchen-han).
xuchen-han
left a comment
There was a problem hiding this comment.
@xuchen-han reviewed 17 files and all commit messages, and made 5 comments.
Reviewable status: 5 unresolved discussions (waiting on jwnimmer-tri).
tools/flags/internal/multi_config.bzl line 8 at r2 (raw file):
# py_test_with_alt_binder is equivalent to py_test except that it forces its # dependencies to be built with --@drake//tools/flags:python_binder=nanobind. # This allows us to run the a test under the non-default binder setting as part
typo
Suggestion:
# This allows us to run the test under the non-default binder setting as parttools/skylark/drake_cc.bzl line 785 at r2 (raw file):
) if type(deps) in (type([]), type(())):
nit not immediately clear to me why this spelling is need.
tools/skylark/drake_py.bzl line 211 at r2 (raw file):
if test_alt_binder: alt_target_compatible_with, _ = combine_conditions( name = name,
IIUC, this could result in name collision with targets generate a few lines above (line 189).
Code quote:
alt_target_compatible_with, _ = combine_conditions(
name = name,tools/skylark/README.md line 100 at r2 (raw file):
**test_alt_binder** Can be either True, False, or "auto" (defaults to "auto").
nit It's not clear from reading this what auto does.
Towards #21572.
Though not strictly required here (since the default of
"auto"is nerfed), we still marktest_alt_binder = Falsea bunch of tests where"auto"resolving toTruewould end up being incorrect.This change is