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

Add conlyopts and cxxopts attributes to cc rules #23792

Conversation

keith
Copy link
Member

@keith keith commented Sep 27, 2024

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

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
@davidben
Copy link

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
Copy link
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
Collaborator

fmeum commented Oct 2, 2024

@bazel-io fork 7.4.0

@trybka
Copy link
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
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
Member Author

keith commented Oct 3, 2024

done!

@Wyverald
Copy link
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cxxopts and conlyopts parameters to cc_library
7 participants