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

build: interpret all protoc warnings as errors #20339

Merged
merged 2 commits into from
Nov 30, 2017

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Nov 29, 2017

No description provided.

@benesch benesch requested review from dt and a team November 29, 2017 23:06
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mberhault
Copy link
Contributor

LGTM. I'm sad to see there is no -Werror for protoc.

@benesch
Copy link
Contributor Author

benesch commented Nov 29, 2017

Indeed. If I had to guess, the problem is that protoc has no concept of warnings, so gogoproto prints directly to stderr: https://github.com/gogo/protobuf/blob/a11c89fbb0ad4acfa8abc4a4d5f7e27c477169b1/plugin/defaultcheck/defaultcheck.go#L102

I'll file a bug there. Thanks for the review!

Copy link
Member

@dt dt left a comment

Choose a reason for hiding this comment

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

Thanks!

@benesch
Copy link
Contributor Author

benesch commented Nov 30, 2017

uggg, blocked on gogo/protobuf#207. Nothing is ever easy.

Fix the following protobuf warning:

    WARNING: field BackupDescriptor.CompleteDbs is a repeated non-nullable
    native type, nullable=false has no effect
Keep protoc warnings from getting lost in the noise, like in dd588f58.
@benesch
Copy link
Contributor Author

benesch commented Nov 30, 2017

Well, that was a stupid amount of yak shaving. I've submitted a request for a -Werror into the upstream void (protocolbuffers/protobuf#3980), but this should do for now.

TFTRs!

@benesch benesch merged commit f95f9fb into cockroachdb:master Nov 30, 2017
@benesch benesch deleted the protoc-warnings branch November 30, 2017 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants