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

Bazel parse_headers feature is incompatible with C copts #23460

Closed
davidben opened this issue Aug 28, 2024 · 3 comments
Closed

Bazel parse_headers feature is incompatible with C copts #23460

davidben opened this issue Aug 28, 2024 · 3 comments
Labels
team-Rules-CPP Issues for C++ rules type: bug

Comments

@davidben
Copy link

davidben commented Aug 28, 2024

Description of the bug:

When the parse_headers feature is enabled, Bazel tries to build headers standalone. It seems to always do so in C++ mode. While C headers are broadly expected to build as C++, Bazel's other bugs mean that a Bazel target may not be able to build with C++ mode.

In particular, C and C++ copts values are not the same. Flags like -std=c11 only work in C mode, and some warnings like -Wmissing-prototypes do not work in C++. Passing those to C++ mode causes the build to fail. Unfortunately, because Bazel lacks conlyopts at the build rule level, Bazel projects that involve C are forced to carefully split their projects into C and C++ targets, so that the C-specific copts go to the C targets. See #22041 for details.

parse_headers breaks this workaround because there is no such thing as a C target in Bazel. Once that feature is enabled, every target contains some C++ by virtue of needing to parse the headers.

Which category does this issue belong to?

C++ Rules

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

$ cat MODULE.bazel
module(name = "test")
$ cat BUILD.bazel 
package(features = ["parse_headers"])

cc_library(
    name = "foo",
    srcs = ["foo.c"],
    hdrs = ["foo.h"],
    copts = [
        "-Wall",
        "-Werror",
        "-std=c11",
    ],
)
$ cat foo.h
#ifndef FOO_H_
#define FOO_H_
void foo(void);
#endif
$ cat foo.c
#include "foo.h"
void foo(void) {}
$ bazelisk build ...
[...]
cc1plus: error: command-line option '-std=c11' is valid for C/ObjC but not for C++ [-Werror]
cc1plus: all warnings being treated as errors
[...]
ERROR: Build did NOT complete successfully

Which operating system are you running Bazel on?

Linux

What is the output of bazel info release?

release 7.3.1

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

n/a

If this is a regression, please try to identify the Bazel commit where the bug was introduced with bazelisk --bisect.

While not introduced by this, #21561 made the issue more visible. (Before, it seems we thought we were enabling parse_headers but actually were not.)

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@fmeum
Copy link
Collaborator

fmeum commented Aug 28, 2024

I started to work on a fix in #22971 a while back. If anybody wants to pick it up and complete it, I'm happy to help.

@davidben
Copy link
Author

TBH the ideal for us would be if #22041 were solved. I spent much of the day chasing after weird build issues that all stemmed from the compounding effects of us needing to work around that core Bazel problem. All other build systems that BoringSSL has to support at Google recognize that C and C++ are different languages and configure their flags separately.

davidben added a commit to google/boringssl that referenced this issue Aug 29, 2024
Bazel has a parse_headers feature which expects headers to be
independently buildable. While a nice way to partially enforce IWYU,
it's broken. See bazelbuild/bazel#23460.

Until the Bazel issue is fixed, we'll need to turn that off. In
particular, after bazelbuild/bazel#22369,
parse_headers is no longer silently inactive.

This does not remove the need to do something about the fips_fragment
filenames. Those come from the layering_check feature, rather than the
parse_headers feature. We also have a number of headers that don't
actually work standalone and, by the style guide, probably should be
named .inc:
https://google.github.io/styleguide/cppguide.html#Self_contained_Headers

But since the feature does not work anyway, just turn it off for now.

Bug: 362664827
Change-Id: I9646d722a59f92be81848cf5a586738cc5f3dac4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/70687
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: Bob Beck <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
@pzembrod
Copy link
Contributor

Closing this since #23792 was merged fixing #22041.

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 type: bug
Projects
None yet
Development

No branches or pull requests

6 participants