Various fixups to warnings flags #9344
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benesch
added a commit
to MaterializeInc/rust-protobuf-native
that referenced
this pull request
Dec 25, 2021
benesch
added a commit
to MaterializeInc/rust-protobuf-native
that referenced
this pull request
Dec 25, 2021
benesch
added a commit
to MaterializeInc/rust-protobuf-native
that referenced
this pull request
Dec 26, 2021
benesch
added a commit
to MaterializeInc/rust-protobuf-native
that referenced
this pull request
Dec 26, 2021
|
@benesch Thank you for the pull request! This looks like a good change to me but I think there are some unit tests that need to be updated. Here is one test failure I see for example: |
|
@benesch I am going to close this since there's been no activity in a few weeks, but if you would like to keep working on this then please let me know and I can reopen it. |
|
Yes, I'll take a pass at fixing the test failures if you re-open. Hard to context switch back with the month-long latency is all, though now I'm guilty of that too! |
benesch
force-pushed
the
warnings-fixups
branch
2 times, most recently
from
e1dac2b to
07bd3b4
Compare
For some reason this warning was emitted as a log message rather than a structured warning. Convert it to use the AddWarning API so that it gets emitted with a file and line number by protoc, and is visible via the error collection interface during programmatic use.
|
I believe I've fixed the test failures. PTAL. |
benesch
force-pushed
the
warnings-fixups
branch
from
07bd3b4 to
a82c136
Compare
|
@benesch Thanks for taking another look! I still see one more test failure and I'm not sure what this is about: |
|
Oy, me neither! This one might be a bit beyond my capacity to debug. Any suggestions? |
CaptureTestStderr() and GetCapturedTestStderr() have to be paired with each other.
|
I think I fixed the problem in parser_unittest.cc, but that seems to have uncovered some other tests that need to be updated. |
|
Gotcha. I can take a look at those if you like? Thanks for fixing the
logging issue!
…On Fri, Apr 29, 2022 at 5:57 PM Adam Cozzette ***@***.***> wrote:
I think I fixed the problem in parser_unittest.cc, but that seems to have
uncovered some other tests that need to be updated.
—
Reply to this email directly, view it on GitHub
<#9344 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGXSIGJ6T5F2Z5PXZ47JHDVHRLOJANCNFSM5KYBIB4Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Sure, that would be great if you could take a look at the other failures. |
A few tests now produce warnings that they didn't before, but were expecting not to see any stderr output. Adjust the tests accordingly.
|
Ok, got them all passing locally. RFAL! |
acozzette
approved these changes
May 13, 2022
|
@benesch Thanks! |
|
Thanks for all the help getting this through CI! |
acozzette
added a commit
to acozzette/protobuf
that referenced
this pull request
May 24, 2022
This reverts commit 448d421. Unfortunately we have to revert this because we're finding that it introduces too much new build log spam for existing proto files that are out of compliance with the warnings. We might be able to roll it forward again if we can figure out a way to do so without so many new log messages.
acozzette
added a commit
that referenced
this pull request
May 24, 2022
This reverts commit 448d421. Unfortunately we have to revert this because we're finding that it introduces too much new build log spam for existing proto files that are out of compliance with the warnings. We might be able to roll it forward again if we can figure out a way to do so without so many new log messages.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Two commits in here that fix up some glitches in how libprotobuf/protoc handle warnings, described below.
Don't drop parser warnings on the floor
Fix #9343.
Convert "missing syntax" warning to an actual warning
For some reason this warning was emitted as a log message rather than a
structured warning. Convert it to use the AddWarning API so that it gets
emitted with a file and line number by protoc, and is visible via the
error collection interface during programmatic use.