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] Modernize Bazel spelling for selecting compiler flags #22335

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Dec 19, 2024

Deprecate the @cc external.

Towards #20731. Keeping this repository rule intact with bzlmod is certainly possible, but is somewhat of a pain point and hassle. Better to just use what rules_cc already offers for compiler selection as best we can.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: low release notes: newly deprecated This pull request contains new deprecations labels Dec 19, 2024
jwnimmer-tri

This comment was marked as resolved.

jwnimmer-tri

This comment was marked as resolved.

jwnimmer-tri

This comment was marked as duplicate.

@jwnimmer-tri jwnimmer-tri changed the title [tools] Modernize Bazel spelling for selecting compiler flags [build] Modernize Bazel spelling for selecting compiler flags Dec 20, 2024
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: needs platform reviewer assigned, needs at least two assigned reviewers


tools/workspace/fcl_internal/package.BUILD.bazel line 128 at r3 (raw file):

    copts = [
        "-fvisibility=hidden",
        "-w",

FYI This -w flag is not required / affected by this patch. While locally testing permutations of options, I noticed that there were spurious warnings coming from FCL builds, so this suppresses them. The spurious warning were also there on master.

@jwnimmer-tri
Copy link
Collaborator Author

+@xuchenhan-tri for feature review, please.

Copy link
Contributor

@xuchenhan-tri xuchenhan-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:

Reviewed 4 of 9 files at r1, 6 of 7 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers


tools/workspace/default.bzl line 162 at r3 (raw file):

        buildifier_repository(name = "buildifier", mirrors = mirrors)
    if "cc" not in excludes:
        # Deprecated 2025-04-01.

Btw this is less than 3 months if we care


tools/cc_toolchain/BUILD.bazel line 110 at r3 (raw file):

int_flag(
    name = "compiler_major",
    build_setting_default = 0,

Btw is the default never used here?

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.

+@SeanCurtis-TRI for platform review per schedule, but if today is too busy feel free to spill it to Grant.

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform)


tools/cc_toolchain/BUILD.bazel line 110 at r3 (raw file):

Previously, xuchenhan-tri wrote…

Btw is the default never used here?

The default would be used in case the user is building Drake on an unsupported platform, or if the user is building Drake as a bazel module external.


tools/workspace/default.bzl line 162 at r3 (raw file):

Previously, xuchenhan-tri wrote…

Btw this is less than 3 months if we care

Oops, I'd bumped the date out to May in the warning printout but missed this comment.

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:

Reviewed 3 of 9 files at r1, 5 of 7 files at r2, 4 of 4 files at r4, all commit messages.
Reviewable status: 3 unresolved discussions


tools/skylark/drake_cc.bzl line 115 at r4 (raw file):

def _platform_copts(rule_copts, rule_gcc_copts, rule_clang_copts, cc_test = 0):
    """Returns the concatenation of Drake's platform-specific BASE_COPTS,
    plus the passed-in rule_copts, plus the passed-in plus rule_{cc}_copts iff

nit: vestigial "plus"?

"..., plus the passed-in plus rule_{cc}_copts iff..."

Code quote:

plus

tools/skylark/drake_cc.bzl line 119 at r4 (raw file):

    When cc_test=1, the GCC_CC_TEST_FLAGS will also be added. It should only be
    used from cc_test rules or rules that are boil down to cc_test rules.

nit: grammar

Suggestion:

rules that boil down 

Deprecate the 'cc' external. Keeping this repository rule intact with
bzlmod is certainly possible, but is somewhat of a hassle. Better to
use what rules_cc already offers for compiler selection.

The command line option --define=DRAKE_WERROR=ON no longer has any
effect. (That flag was intended to be internal use only so hopefully
nobody will care. It's replacement is now explicitly documented to be
internal use only.)

Remove some vestigial cc_toolchain diagnostic programs.

Also fix some warning Noble GCC spam from FCL, noticed while debugging.
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.

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

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 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),xuchenhan-tri(platform)

@jwnimmer-tri jwnimmer-tri merged commit d0f2b13 into RobotLocomotion:master Jan 3, 2025
9 checks passed
@jwnimmer-tri jwnimmer-tri deleted the select-copts branch January 3, 2025 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: newly deprecated This pull request contains new deprecations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants