Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[build] Use modern spelling for Bazel config options #22393

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Jan 4, 2025

Towards #20731.

See https://bazel.build/configure/attributes for background.

This just gets us started with the solvers flags. More flags are on the way in the future (eigen, fmt, spdlog).


This change is Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

+@SeanCurtis-TRI for feature review, please.

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:LGTM:

+@ggould-tri for platform review, please.

Reviewed 22 of 22 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee ggould-tri(platform)


solvers/BUILD.bazel line 1382 at r1 (raw file):

    name = "disabled_solvers_test",
    deps = [
        ":clarabel_solver_disabled",

BTW It may not be important, but without reading defs.bzl documentaiton, it is more or less impossible to figure out what these targets are. When I see a target I'm not familiar with, it's typically easy to find. These targets defy that. Perhaps a note on where to find these (particularly as this seems to be the only site these targets are referenced).

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:; documentation notes.

Reviewed 22 of 22 files at r1, all commit messages.
Reviewable status: 6 unresolved discussions


solvers/defs.bzl line 48 at r1 (raw file):

    Additionally, declares a library {name}_disabled with the disabled flavor,
    intended for use by unit tests checks of stub implementation even when the

typo?

Suggestion:

unit test checks

tools/flags/BUILD.bazel line 14 at r1 (raw file):

#
# bazelrc example:
#   common --@drake//tools/flags:with_mosek=True

minor: These seem like they should also appear in https://drake.mit.edu/bazel.html#developing-drake-using-bazel

Code quote:

# Command line example:
#   bazel build //:something --@drake//tools/flags:with_mosek=True
#
# bazelrc example:
#   common --@drake//tools/flags:with_mosek=True

tools/workspace/gurobi/BUILD.bazel line 19 at r1 (raw file):

config_setting(
    name = "enabled_via_define",
    # N.B. This is the legacy spelling. Users shouldn't use this in new code.

minor: This is still the documented spelling in the from_source.md docs, though.


tools/workspace/snopt/BUILD.bazel line 20 at r1 (raw file):

config_setting(
    name = "enabled_via_define",
    # N.B. This is the legacy spelling. Users shouldn't use this in new code.

minor: As above.


tools/workspace/mosek/BUILD.bazel line 19 at r1 (raw file):

config_setting(
    name = "enabled_via_define",
    # N.B. This is the legacy spelling. Users shouldn't use this in new code.

minor: As above.

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),SeanCurtis-TRI(platform)


tools/flags/BUILD.bazel line 14 at r1 (raw file):

Previously, ggould-tri wrote…

minor: These seem like they should also appear in https://drake.mit.edu/bazel.html#developing-drake-using-bazel

I am repaving that entire file in #22055. I've added a reminder there to add more docs.


tools/workspace/gurobi/BUILD.bazel line 19 at r1 (raw file):

Previously, ggould-tri wrote…

minor: This is still the documented spelling in the from_source.md docs, though.

The mention in from_source.md is referring the CMake option, which remains unchanged by this PR.

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),SeanCurtis-TRI(platform)

@jwnimmer-tri jwnimmer-tri merged commit 8df4db9 into RobotLocomotion:master Jan 7, 2025
8 of 9 checks passed
@jwnimmer-tri jwnimmer-tri deleted the bzlmod-flags branch January 7, 2025 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants