Skip to content

Add conlyopts and cxxopts attributes to cc rules#23792

Closed
keith wants to merge 2 commits intobazelbuild:masterfrom
keith:ks/add-conlyopts-and-cxxopts-attributes-to-cc-rules
Closed

Add conlyopts and cxxopts attributes to cc rules#23792
keith wants to merge 2 commits intobazelbuild:masterfrom
keith:ks/add-conlyopts-and-cxxopts-attributes-to-cc-rules

Conversation

@keith
Copy link
Copy Markdown
Member

@keith keith commented Sep 27, 2024 •
edited
Loading

The inability to pass C or C++ specific compiler flags to targets that
contain a mix of those sources is a common sticking point for new users.
These mirror the global --conlyopt and --cxxopt flags but at the
target level.

Fixes #22041

RELNOTES: Add conlyopts and cxxopts attributes to cc rules

@keith
Add conlyopts and cxxopts attributes to cc rules
f3154c2 
The inability to pass C or C++ specific compiler flags to targets that
contain a mix of those sources is a common sticking point for new users.
These mirror the global `--conlyopt` and `--cxxopt` flags but at the
target level.

Fixes bazelbuild#22041
@github-actions github-actions bot added team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels Sep 27, 2024
@keith keith mentioned this pull request Sep 27, 2024
Closed
@davidben
Copy link
Copy Markdown

davidben commented Sep 30, 2024

Thank you! I'm not familiar enough with Bazel to review this, but this would solve a ton of problems with Bazel that prevent BoringSSL from using it effectively. We support many build systems, and issue alone, due to all the problems it causes down the line, is probably singlehandedly the reason why it is the most expensive build system for us to support right now. E.g. we'd also be able to enable parse_headers because #23460 would be moot. If this could be merged and then cherry-picked into every release branch that can take it, that would be lovely.

@Wyverald Wyverald requested review from comius and pzembrod and removed request for oquenchil September 30, 2024 20:57
@comius comius requested review from trybka and removed request for lberki and pzembrod October 2, 2024 07:21
comius
comius approved these changes Oct 2, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@comius comius left a comment

Choose a reason for hiding this comment

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

LGTM from Blaze perspective.

I want LGTM also from @trybka, before merging.

@fmeum
Copy link
Copy Markdown
Collaborator

fmeum commented Oct 2, 2024

@bazel-io fork 7.4.0

@bazel-io bazel-io mentioned this pull request Oct 2, 2024
Closed
@trybka
Copy link
Copy Markdown
Contributor

trybka commented Oct 2, 2024

LGTM. I am extremely supportive.

@iancha1992 iancha1992 added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Oct 3, 2024
@Wyverald
Copy link
Copy Markdown
Member

Wyverald commented Oct 3, 2024

@keith: could you add a RELNOTES line to the PR description? I'll attach it during import.

@keith
Copy link
Copy Markdown
Member Author

keith commented Oct 3, 2024

done!

@Wyverald
Copy link
Copy Markdown
Member

Wyverald commented Oct 3, 2024

FTR, this is breaking Google-internal tests because some other code is calling cc_helper.get_copts, and is not happy about the new param. What makes it worse is that this code is not in @_builtins, so there's version skew.

Could you perhaps make the new param optional?

@keith keith requested a review from pzembrod as a code owner October 3, 2024 23:03
@keith
Copy link
Copy Markdown
Member Author

keith commented Oct 3, 2024

made it optional

@copybara-service copybara-service bot closed this in 5854788 Oct 4, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Oct 4, 2024
@keith keith deleted the ks/add-conlyopts-and-cxxopts-attributes-to-cc-rules branch October 8, 2024 21:58
@pzembrod pzembrod mentioned this pull request Nov 18, 2024
Closed
davidben added a commit to google/boringssl that referenced this pull request Nov 10, 2025
@davidben
Remove workaround for Bazel C/C++ issue
3a6dd1a 
bazelbuild/bazel#23792 is available in Bazel
7.4.0 or later. [0] has long set a minimum Bazel version of Bazel 7 and,
as of [1], Envoy has since caught up. We should be able to rely on this
now, and
remove the workaround.

This doesn't enable parse_headers, but clears the Bazel blocker.

Update-Note: BoringSSL now requires Bazel 7.4.0 or later.

[0] https://github.com/google/oss-policies-info/blob/main/foundational-cxx-support-matrix.md
[1] envoyproxy/envoy#41209

Bug: 399387924
Change-Id: Icf2278d9fe8f0f38f353ce87dd227bd4bf0ac95c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/76787
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@comius comius comius approved these changes

@trybka trybka Awaiting requested review from trybka trybka is a code owner

@pzembrod pzembrod Awaiting requested review from pzembrod pzembrod is a code owner

Assignees

No one assigned

Labels

team-Rules-CPP Issues for C++ rules

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cxxopts and conlyopts parameters to cc_library

7 participants

@keith @davidben @fmeum @trybka @Wyverald @comius @iancha1992