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

cc_proto_library should support cc_library parameters (like copts, linkopts, etc) #4446

Open
pmuetschard opened this issue Jan 12, 2018 · 19 comments
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: feature request

Comments

@pmuetschard
Copy link
Contributor

Description of the problem / feature request:

The cc_proto_library is, in essence a cc_library rule and should thus honor the cc_library parameters, such as copts.

Feature requests: what underlying problem are you trying to solve with this feature?

Large protos may require the -Wa,-mbig-obj copt on Windows, but it is not always possible to use that flag globally (e.g. if also compiling go code).

There are other reason why people may want to control the underlaying cc_library rule, of course.

What operating system are you running Bazel on?

Linux, OSX, Windows

What's the output of bazel info release?

0.9.0

@RNabel
Copy link
Contributor

RNabel commented Jun 19, 2018

Hey @jin (sorry to rope you into this directly): is there some progress or a known workaround? I'm on bazel 0.14.0 and protobuf 3.4.0, my CROSSTOOL compiles with "-Wall" - the generated code breaks with this flag due to unused-parameters and extended-offsetof.

I tried to wrap cc_proto_library with a cc_library with copts = ["-w"] but the compilation still fails, stating it cannot build the underlying proto_library. i.e.:

proto_library(
    name = "A", 
    srcs = "some.proto"
    # in my case also depends on googleapis.
)

cc_proto_library(
    name = "B",
    deps = ":A",
)

cc_library(
    name = "C",
    deps = [":B"],
    copts = ["-w"]
)

Error:

C++ compilation of rule '//:A' failed (Exit 1) etc. etc.

bazel-out/darwin-opt-clang/genfiles/external/com_github_googleapis_googleapis/google/api/http.pb.cc:79:3: error: using extended field designator is an extension [-Werror,-Wextended-offsetof]
  GOOGLE_PROTOBUF_GENERATED_MESSAGE_FIELD_OFFSET(HttpRule, _oneof_case_[0]),
  ^                                                                    ~~~
external/protobuf/src/google/protobuf/generated_message_util.h:92:3: note: expanded from macro 'GOOGLE_PROTOBUF_GENERATED_MESSAGE_FIELD_OFFSET'
  __builtin_offsetof(TYPE, FIELD)                                    \
  ^                        ~~~~~
bazel-out/darwin-opt-clang/genfiles/external/com_github_googleapis_googleapis/google/api/http.pb.cc:254:57: error: unused parameter 'arena' [-Werror,-Wunused-parameter]
void Http::RegisterArenaDtor(::google::protobuf::Arena* arena) {

See related protobuf ticket: protocolbuffers/protobuf#4628

@jin
Copy link
Member

jin commented Jun 19, 2018

@RNabel I have little contextual knowledge around cc_{proto_}_library, adding in @mhlopko who works on the cc_* rules.

@RNabel
Copy link
Contributor

RNabel commented Jun 20, 2018

I got something hacky to work, maybe this helps someone with the same problem.

  1. extract the cc_proto_library's compile_flags manually by using a custom rule, I used:
def _cc_proto_lib_impl(ctx):
  p = ctx.attr.cc_proto_libraries[0].cc
  print("=" * 50)
  print("compile_flags", p.compile_flags) # <---- these flags.
  print("=" * 50)
  print("defines", p.defines)
  print("=" * 50)
  print("include_directories", p.include_directories)
  print("=" * 50)
  print("libs", p.libs)
  print("=" * 50)
  print("link_flags", p.link_flags)
  print("=" * 50)
  print("quote_include_directories", p.quote_include_directories)
  print("=" * 50)
  print("system_include_directories", p.system_include_directories)
  print("=" * 50)
  print("transitive_headers", p.transitive_headers)
  print("=" * 50)

wrapped_cc_proto_library = rule(
    implementation = _cc_proto_lib_impl,
    attrs = {
        "cc_proto_libraries": attr.label_list(mandatory = True, allow_empty = False),
    },
)

This rule is then imported and used within a BUILD file like so:

wrapped_cc_proto_library(name = "whatever", cc_proto_libraries = [":your_cc_proto_library_target"])

which will print the contents of all fields in the cc provider of the cc_proto_library.

  1. Create a cc_library in which you copy-paste the compile_flags into the copts + add additional copts. I added "-w" to suppress all warnings. It would looks roughly like this:
cc_library(
    name = "some_general_cc_target",
    srcs = [
        ":some_cc_proto_library", # The main cc_proto_library library you're compiling. Yes, passed to srcs. Ugh. 
        ":transitive_dep_cc_proto_library", # You need to include all transitive dependencies' cc_proto_libraries.
    ],
    copts = [
        # Compile flags extracted using the custom rule here.
        "-isystem external/com_google_protobuf/src", etc. etc.

        # Custom copts.
        "-w",
    ]
)

@hlopko
Copy link
Member

hlopko commented Jun 28, 2018

@RNabel I'd advise you not to spend too much time on that, since #4570 is being actively worked on and that will allow you to do exactly what you want - create the exactly same actions as C++ rules would.

@hlopko hlopko self-assigned this Jun 28, 2018
@hlopko hlopko added the P2 We'll consider working on this in future. (Assignee optional) label Jun 28, 2018
@RNabel
Copy link
Contributor

RNabel commented Jun 29, 2018

@mhlopko Nice, thanks for the heads up. Is there a rough timeline?

@hlopko
Copy link
Member

hlopko commented Jun 29, 2018

The roadmap says it will be done in July, which will be a challenge but it's possible. So it will be in released Bazel in August or in September. If all goes well.

@hlopko hlopko added team-Rules-CPP Issues for C++ rules and removed category: rules > C++ labels Oct 11, 2018
@hlopko hlopko assigned dbabkin and unassigned hlopko Jul 4, 2019
@dbabkin dbabkin removed their assignment Oct 11, 2019
@meisterT
Copy link
Member

@oquenchil what's the current state?

@oquenchil oquenchil added help wanted Someone outside the Bazel team could own this P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed P2 We'll consider working on this in future. (Assignee optional) labels May 15, 2020
@oquenchil
Copy link
Contributor

No plans to work on this in the short term. I updated priorities.

@c-mita c-mita added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) help wanted Someone outside the Bazel team could own this labels Nov 23, 2020
@TheKaur
Copy link

TheKaur commented Dec 1, 2020

Any further update on this? This would be a pretty useful feature for us.

@jonathan-enf
Copy link

Sadly, cc_common does not seem to contain a cc_proto_library rule, requiring that the user build their own by reverse-engineering the old cc_proto_library rule.

@yeswalrus
Copy link

Ping, any word on updating cc_proto_library with this support?

@oquenchil
Copy link
Contributor

No plans in the immediate future.

@github-actions
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Aug 26, 2023
Copy link

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please reach out to the triage team (@bazelbuild/triage). Thanks!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 24, 2023
@keith keith removed the stale Issues or PRs that are stale (no activity for 30 days) label May 7, 2024
@keith keith reopened this May 7, 2024
@chandlerc
Copy link

I'd really like this to not just be a "feature request" if my issue is going to be de-duped to it, only being un-staled in 2023.

Without this, it is not possible to build protobufs in many configurations. Sadly, copts aren't really an "optional" or "nice-to-have" feature given the complexity of the C++ compiler and flag space. =[

@fmeum
Copy link
Collaborator

fmeum commented Jun 4, 2024

@chandlerc As a workaround, you could probably define a toolchain feature with the required flags and then use the features attribute of cc_proto_library to enable it.

@comius comius added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed P3 We're not considering working on this, but happy to review a PR. (No assignee) labels Jun 4, 2024
@comius
Copy link
Contributor

comius commented Jun 4, 2024

Adding copts or other parameters to cc_proto_library doesn't work well, because of "transitivity". Imagine you have pl2 -> pl1. Then if cc_pl1 -> pl1 sets some parameters, second cc_proto_library: cc_pl2 -> pl2, will contain the code for pl1, but won't set the same parameters as cc_pl1.

@keith
Copy link
Member

keith commented Jun 4, 2024

note one bad workaround you can use today is --per_file_copt in your .bazelrc can apply to cc_proto_library targets, I'm not sure if that's intended but it does work!

github-merge-queue bot pushed a commit to carbon-language/carbon-lang that referenced this issue Jun 4, 2024
Most of these are places where we failed to include a header file and
simply never got an error about this. The fix is to include the header
file.

Most other cases are functions that should have been marked `static` but
were not. Finding all of these was a main motivation for me enabling the
warning despite how much work it is.

One complicating factor was that we weren't including the `handle.h` for
all the state-based handler functions. While this isn't a tiny amount of
code, it is just declarations and doesn't add any extra dependencies. It
also lets us have the checking for which functions need to be `static`
and which don't. For the `parse` library I had to add the `handle.h`
header as well, I tried to match the design of it in `check`.

I have also had to work around a bug in the warning, but given the value
it seems to be providing, that seems reasonable. I've filed the bug
upstream: llvm/llvm-project#94138

I also had to use some hacks to work around limitations of Bazel rules
that wrap `cc_library` rules and don't expose `copts`. I filed a bug for
`cc_proto_library` specifically:
~bazelbuild/bazel#22610 
bazelbuild/bazel#4446
keith added a commit to keith/protobuf that referenced this issue Dec 18, 2024
This new `cc_proto_aspect_hint` allows users to specify `copts` that are
then used when compiling the generated C++ code.

This is a potential solution to bazelbuild/bazel#4446

In that discussion the primary concern was that if you have multiple
`cc_proto_library` targets that depend on the same `proto_library`
targets, if the `copts` was part of `cc_proto_library` you'd have to be
sure to duplicate that to all `cc_proto_library` targets in the
dependency tree. Using the relatively new `aspect_hints` functionality
in bazel we can instead attach that info to the `proto_library` target
itself, without adding C++ specific attributes to the `proto_library`
rule.
@keith
Copy link
Member

keith commented Dec 18, 2024

I submitted an option for this to protobuf which uses aspect_hints to avoid the issue @comius mentioned above, PTAL protocolbuffers/protobuf#19706

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: feature request
Projects
None yet
Development

No branches or pull requests