Suppress all C++ warning when compiling generated protobuf code. #5467

RNabel · 2018-06-26T14:44:36Z

This PR is a suggested fix for #5466.

As long as a) one cannot specify copts for the compilation of generated C++ code, and b) protobuf generated code is warning-generating, we should suppress at least the errors protobuf code is known to generate (from my experience at least unused-parameter (as seen here: protocolbuffers/protobuf#4628) and invalid-offset and zero-as-null-pointer-constant).

a) is fixable, but has no timeline, b) will not be fixed.

Option 1: Suppress all warnings (currently chosen)

Since protobuf code is already assumed to be correct, suppressing all warnings both reduces log message spam and solves the -Werror problem.
Advantages: Will compile with any CROSSTOOL copts.
Disadvantages: Could suppress warnings of a broken C++ protobuf plugin.

Option 2: Suppress specific known errors

It is theoretically possible to suppress only the known errors, but I'm not a C++ expert so don't know how compatible these flags are between OSs.
Risks: Windows, different protobuf versions could have different errors.

Option 3: Pass `-Wno-error`

Warnings would still be shown but at least the compilation would succeed.
Advantages: least intrusive, builds complete.
Disadvantages: many un-actionable log messages.

I hope this is the correct format for this kind of discussion. Many thanks in advance for any comments :)

rongjiecomputer · 2018-06-26T23:36:18Z

It is theoretically possible to suppress only the known errors, but I'm not a C++ expert so don't know how compatible these flags are between OSs.
Risks: Windows, different protobuf versions could have different errors.

Context: I am C++ programmer on Windows and regularly deal with GCC, Clang and MSVC.

Adding only the flags needed for each compiler (the problem is specific to compiler, not OS) is feasible, but adding them in .java is kind of awkward. If cc_proto_library rule will be migrated to Skylark and flags can be added with Crosstool in Skylark, that will be easier for me.

zero-as-null-pointer-constant warning should be fixed in upstream protobuf in future version when they migrate from NULL to nullptr (they couldn't do it before 3.6.0 due to requirement to support pre-C++11 compilers).

lberki

Sorry for the late review. Unfortunately, this can't be merged because it would make MSVC users sad.

I don't think Bazel has the concept of "disable all warnings" yet, does it, @mhlopko ? I'd define a feature that does that then add it to the default generated CROSSTOOL file. Unfortunately, that's not a single-line change.

lberki · 2018-07-04T13:50:26Z

src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java

@@ -256,6 +256,7 @@ private CcCompilationHelper initializeCompilationHelper(
      }

      helper.addDeps(ruleContext.getPrerequisites("deps", TARGET));
+      helper.setCopts(ImmutableList.of("-w"));


This won't work: this assumes that the compiled is gcc/clang and leaves MSVC users out in the cold.

Thanks for the review! I'll try it on Windows just to be on the safe side.

The MSVC docs state Options are specified by either a forward slash (/) or a dash (–). (see the option entry). This in combination with the Warning levels page leads me to believe that it should work.

But then again, this is Windows we're talking about.

MSVC can accept -w to disable all warnings just fine, but if someone append -W3/-W4 etc. after -w, you will get D9025` warning that cannot be silenced at all, causing a waterfall of warnings.

lberki · 2018-07-04T14:53:04Z

Well, the review latency was embarrassing....

Even if it does work with MSVC, I'd like us to move away from hard-coded command line options not towards them. What do you think, @mhlopko ?

RNabel · 2018-07-04T15:15:24Z

@lberki I agree that having hard-coded command line options is non-optimal and that it would be far better to add a feature to the CROSSTOOLs which suppresses all warnings and enable that instead. Am I correct in assuming that this feature would need to be added to the general CROSSTOOL and the osx CROSSTOOL. How does the latter get edited? It states:

# This file was auto-generated by a script maintained internally by
# Google.

lberki · 2018-07-05T08:13:46Z

/cc @mhlopko for CROSSTOOL wisdom

hlopko · 2018-07-05T16:20:01Z

+1 what @lberki said, let's not hardcode any flags if we can help it. I propose to add "disable_errors" feature to requested features here.

Yes that means that we have to define the feature in all the crosstools. Well, we don't have to (bazel won't complain if the feature is not defined) but users will expect the feature to be there, so let's not let them down.

Osx crosstool is indeed generated by a tool that we didn't get to open source (and we will not because Crosstool in Skylark solves the problem better). Just edit one toolchain in that file and I'll take care of the generator once I import the PR.

RNabel · 2018-07-11T09:10:10Z

@mhlopko I just pushed a working example for macos and added the new feature to what I hope is the default Unix toolchain. I tested it on macos (sorry don't have a Linux machine handy right now).

Could you let me know where else this feature should go?

Finally, I'd propose to rename the feature "disable_warnings" or changing the flag from "-w" to "-Wno-error".

laurentlb · 2018-07-20T22:14:57Z

ping @mhlopko

RNabel · 2018-08-01T14:40:57Z

ping @lberki

hlopko · 2018-08-02T15:59:10Z

I like it, let me import it :) Thanks!

hlopko · 2018-08-03T15:22:38Z

(Asking protobuf team for approval internally, will take some time).

RNabel · 2018-08-03T15:41:03Z

@mhlopko Awesome, thanks for the update!

RNabel · 2018-08-28T17:49:05Z

@mhlopko Do you have a rough timeline for this to go in?

petemounce · 2018-09-07T17:06:21Z

Any update?

jin · 2018-12-10T22:18:04Z

What is this status of this PR?

lberki · 2018-12-11T13:44:30Z

Wow, this was a while ago... I imported this change and asked @mhlopko to review it. What's the plan for Windows?

jin · 2019-02-19T21:30:13Z

Is this still import-able?

hlopko · 2019-03-07T09:20:41Z

Hi all,

I'm very sorry for how long this PR took. So there were discussion with the protobuf team internally and the result is that protoc should not generate warning triggering code. I commented protocolbuffers/protobuf#4628 (comment) and I expect that protobuf team will reopen that issue and drive this effort further. @RNabel, @rongjiecomputer, can you check in on protocolbuffers/protobuf#4628 (comment) to help them understand the actual problems?

Thank you! And again, I apologize for this taking this long.

I'll now close this PR. But please shout if you disagree with what I wrote above and we'll continue the discussion here.

googlebot added the cla: yes label Jun 26, 2018

RNabel mentioned this pull request Jun 28, 2018

cc_proto_library unusable with -Werror in CROSSTOOL #5466

Closed

hlopko requested a review from lberki June 28, 2018 14:50

hlopko assigned lberki Jun 28, 2018

lberki reviewed Jul 4, 2018

View reviewed changes

hlopko self-requested a review July 5, 2018 16:20

hlopko assigned hlopko and unassigned lberki Jul 5, 2018

RNabel added 2 commits July 10, 2018 09:38

Add "disable_errors" to requestedFeatures.

0c03c42

Add feature: "disable_errors" to darwin toolchain.

a4d7ae1

RNabel force-pushed the feature/BazelCcProtoLibrary-add-copts-suppress-warnings branch from 357d63c to a4d7ae1 Compare July 11, 2018 08:55

Add disable_errors feature to default unix toolchain (hopefully).

6cc0579

hlopko assigned lberki Jul 24, 2018

jin added the team-Rules-Server Issues for serverside rules included with Bazel label Dec 10, 2018

hlopko mentioned this pull request Mar 7, 2019

Warning message when build protobuf lite protocolbuffers/protobuf#4628

Closed

hlopko closed this Mar 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppress all C++ warning when compiling generated protobuf code. #5467

Suppress all C++ warning when compiling generated protobuf code. #5467

RNabel commented Jun 26, 2018

rongjiecomputer commented Jun 26, 2018

lberki left a comment

lberki Jul 4, 2018

RNabel Jul 4, 2018 •

edited

Loading

rongjiecomputer Jul 4, 2018

lberki commented Jul 4, 2018

RNabel commented Jul 4, 2018 •

edited

Loading

lberki commented Jul 5, 2018

hlopko commented Jul 5, 2018

RNabel commented Jul 11, 2018

laurentlb commented Jul 20, 2018

RNabel commented Aug 1, 2018

hlopko commented Aug 2, 2018

hlopko commented Aug 3, 2018

RNabel commented Aug 3, 2018

RNabel commented Aug 28, 2018

petemounce commented Sep 7, 2018

jin commented Dec 10, 2018

lberki commented Dec 11, 2018

jin commented Feb 19, 2019 •

edited

Loading

hlopko commented Mar 7, 2019

Suppress all C++ warning when compiling generated protobuf code. #5467

Suppress all C++ warning when compiling generated protobuf code. #5467

Conversation

RNabel commented Jun 26, 2018

Option 1: Suppress all warnings (currently chosen)

Option 2: Suppress specific known errors

Option 3: Pass -Wno-error

rongjiecomputer commented Jun 26, 2018

lberki left a comment

Choose a reason for hiding this comment

lberki Jul 4, 2018

Choose a reason for hiding this comment

RNabel Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

rongjiecomputer Jul 4, 2018

Choose a reason for hiding this comment

lberki commented Jul 4, 2018

RNabel commented Jul 4, 2018 • edited Loading

lberki commented Jul 5, 2018

hlopko commented Jul 5, 2018

RNabel commented Jul 11, 2018

laurentlb commented Jul 20, 2018

RNabel commented Aug 1, 2018

hlopko commented Aug 2, 2018

hlopko commented Aug 3, 2018

RNabel commented Aug 3, 2018

RNabel commented Aug 28, 2018

petemounce commented Sep 7, 2018

jin commented Dec 10, 2018

lberki commented Dec 11, 2018

jin commented Feb 19, 2019 • edited Loading

hlopko commented Mar 7, 2019

Option 3: Pass `-Wno-error`

RNabel Jul 4, 2018 •

edited

Loading

RNabel commented Jul 4, 2018 •

edited

Loading

jin commented Feb 19, 2019 •

edited

Loading