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.

Sign up for GitHub

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

Jump to bottom

Add parse_headers support to the default Unix toolchain #21560

Closed
wants to merge 2 commits into from
Closed

Add parse_headers support to the default Unix toolchain #21560

wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Mar 4, 2024 •
edited
Loading

Also remove Ubuntu 16.04 workarounds from layering_check tests.

RELNOTES: The default Unix C++ toolchain now supports the parse_headers feature to validate header files with --process_headers_in_dependencies.

@github-actions github-actions bot added team-Rules-CPP Issues for C++ rules team-Documentation Documentation improvements that cannot be directly linked to other team labels awaiting-review PR is awaiting review from an assigned reviewer labels Mar 4, 2024
@fmeum fmeum removed the team-Documentation Documentation improvements that cannot be directly linked to other team labels label Mar 4, 2024
@fmeum fmeum requested review from comius and removed request for gregestren and fweikert March 4, 2024 20:04
keith
keith reviewed Mar 4, 2024
View reviewed changes
tools/cpp/unix_cc_toolchain_config.bzl
Comment on lines +114 to +117
# parsing failures for C headers that are not valid C++.
# For such headers, use features = ["-parse_headers"] to selectively
# disable parsing.
"-xc++-header",
Copy link
Member

Choose a reason for hiding this comment

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

looks like there is also -xc-header if this is a major issue

Copy link
Collaborator Author

@fmeum fmeum Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We could add a separate feature for that flag as a follow-up. The problem is that Bazel can't tell whether a given header should be treated as C or C++.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Making this more concrete: I could submit a follow-up PR that defines two more features, parse_headers_as_cpp (default) and parse_headers_as_c, that can be combined with parse_headers.

@fmeum
Copy link
Collaborator Author

fmeum commented Mar 4, 2024

@bazel-io fork 7.1.0

@bazel-io bazel-io mentioned this pull request Mar 4, 2024
Closed
@fmeum
Copy link
Collaborator Author

fmeum commented Mar 4, 2024

@iancha1992 This is not a hard blocker for the 7.1.0 release, but getting it in there would help maintaining C++ modules in the BCR (cc @meteorcloudy).

@iancha1992
Copy link
Member

@fmeum do you still want to merge this?

@fmeum
Copy link
Collaborator Author

fmeum commented Apr 10, 2024

@iancha1992 Yes, it's just waiting for a review.

@comius Friendly ping :⁠-⁠)

@fmeum fmeum requested review from pzembrod and removed request for comius April 22, 2024 18:51
@fmeum
Copy link
Collaborator Author

fmeum commented Apr 22, 2024

@pzembrod I unassigned Ivo and moved this over to you. Let me know if you have any questions!

@Wyverald
Copy link
Member

Wyverald commented May 8, 2024

@pzembrod Is this still on track for 7.2.0? We're aiming for RC1 next Monday.

@fmeum fmeum force-pushed the parse-headers branch 2 times, most recently from 22e2131 to 6c59953 Compare May 14, 2024 12:31
@fmeum
Add parse_headers support to the default Unix toolchain
7623c0f 
RELNOTES: The default Unix C++ toolchain now supports the
`parse_headers` to validate header files with
`--process_headers_in_dependencies`.
pzembrod
pzembrod reviewed May 14, 2024
View reviewed changes
site/en/docs/bazel-and-cpp.md Show resolved Hide resolved
src/test/shell/bazel/bazel_layering_check_test.sh Show resolved Hide resolved
tools/cpp/linux_cc_wrapper.sh.tpl Show resolved Hide resolved
tools/cpp/unix_cc_configure.bzl Show resolved Hide resolved
tools/cpp/unix_cc_configure.bzl Show resolved Hide resolved
pzembrod
pzembrod approved these changes May 14, 2024
View reviewed changes
site/en/docs/bazel-and-cpp.md Outdated Show resolved Hide resolved
@pzembrod pzembrod removed the awaiting-review PR is awaiting review from an assigned reviewer label May 14, 2024
@pzembrod pzembrod added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 14, 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 May 14, 2024
@fmeum fmeum deleted the parse-headers branch May 14, 2024 19:14
fmeum added a commit to fmeum/bazel that referenced this pull request May 14, 2024
@fmeum
Add parse_headers support to the default Unix toolchain
adfe3fe 
Also remove Ubuntu 16.04 workarounds from layering_check tests.

RELNOTES: The default Unix C++ toolchain now supports the `parse_headers` feature to validate header files with `--process_headers_in_dependencies`.

Closes bazelbuild#21560.

PiperOrigin-RevId: 633657012
Change-Id: Iaaa2623bcc86b219b02567eca1c7bf9e1a77ae7d
@fmeum fmeum mentioned this pull request May 14, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request May 15, 2024
@fmeum
[7.2.0] Add parse_headers support to the default Unix toolchain (#2…
a98d588 
…2369)

Also remove Ubuntu 16.04 workarounds from layering_check tests.

RELNOTES: The default Unix C++ toolchain now supports the
`parse_headers` feature to validate header files with
`--process_headers_in_dependencies`.

Closes #21560.

PiperOrigin-RevId: 633657012
Change-Id: Iaaa2623bcc86b219b02567eca1c7bf9e1a77ae7d

Fixes #22361

Commit
231dfc2
@iancha1992
Copy link
Member

The changes in this PR have been included in Bazel 7.2.0 RC1. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=7.2.0rc1. Thanks!

keith added a commit to bazelbuild/apple_support that referenced this pull request Jun 28, 2024
@keith
Add support for parse_headers
e7925d9 
This feature enables bazel to validate "C++" header files through clang.
Without this you only get this type of validating when you actually
include the library elsewhere.

Similar to layering_check we have to validate this outside of normal
bazel because we are expecting build failures and need to parse the logs

This mirrors bazelbuild/bazel#21560
@keith keith mentioned this pull request Jun 28, 2024
Merged
brentleyjones pushed a commit to bazelbuild/apple_support that referenced this pull request Jun 28, 2024
@keith
Add support for parse_headers (#337)
e8007bf 
This feature enables bazel to validate "C++" header files through clang.
Without this you only get this type of validating when you actually
include the library elsewhere.

Similar to layering_check we have to validate this outside of normal
bazel because we are expecting build failures and need to parse the logs

This mirrors bazelbuild/bazel#21560
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keith keith keith left review comments

@pzembrod pzembrod pzembrod approved these changes

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.
5 participants
@fmeum @iancha1992 @Wyverald @keith @pzembrod