[perlun]

New attempt at #4336, building on top of the nice groundwork already provided by @benesch (appreciated!). Hoping we can get it to an approvable state this time. 😄

Because this builds on top of an existing MR, I decided to keep it as two separate commits - one for the rebased commit by @benesch and another one with my fixes. (There were some rebase conflicts that were amended into the original commit. One particular rebase conflict that I'm a bit hesitant about is https://github.com/protocolbuffers/protobuf/pull/4336/files#diff-cd3d05a33a7e35e0eeeff7b01f65d8744f20f08b1b179bb7ea3dc7e36e202d08R794; the constructor call looked differently here now so I removed that field setting altogether. Unsure if this is correct or not so please do advise.)

The newly added test runs fine locally with CLion but I'm not fully sure on how to run all the tests, so I'm hoping for the CI to tell me if the other tests pass or not with this change now. 🙂

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[benesch]

@googlebot I consent.

[perlun]

@benesch Do you happen to know what the process is for getting someone to review this? It doesn't seem to be moving incredibly fast at the moment. 😅

[benesch]

The process is: find someone you know at Google, and ask them to internally ping someone on the protobuf team 😄. Only a little bit kidding.

[acozzette]

It looks like the CommandLineInterfaceTest.InvalidErrorFormat test case is failing here:

protobuf/src/google/protobuf/compiler/command_line_interface_unittest.cc

Line 2315 in fe47e99

ExpectErrorText("Unknown error format: invalid\n");

[perlun]

Thanks @acozzette, will give it a look. 👍

[perlun]

@acozzette There we go, I edited the test in question like this:

diff --git src/google/protobuf/compiler/command_line_interface_unittest.cc src/google/protobuf/compiler/command_line_interface_unittest.cc
index 15c9170a3..d1d14b81a 100644
--- src/google/protobuf/compiler/command_line_interface_unittest.cc
+++ src/google/protobuf/compiler/command_line_interface_unittest.cc
@@ -2330,7 +2330,7 @@ TEST_F(CommandLineInterfaceTest, InvalidErrorFormat) {
 
   Run("protocol_compiler --test_out=$tmpdir --proto_path=$tmpdir foo.proto");
 
-  ExpectErrorText("Unknown error format: invalid\n");
+  ExpectErrorText("foo.proto:2:1: Expected top-level statement (e.g. \"message\").\n");
 }
 
 TEST_F(CommandLineInterfaceTest, Warnings) {

This is admittedly quite naïve, I don't really understand why the semantics for this error changes because of the (seemingly unrelated) changes in this MR. If you understand it better, please let me know.

(I couldn't access the kokoro build logs for some reason, so I'm unsure if there's any more failures than this single one.)

[perlun]

This is admittedly quite naïve, I don't really understand why the semantics for this error changes because of the (seemingly unrelated) changes in this MR. If you understand it better, please let me know.

I think I got it now. The test was changed in the original commit by @benesch (by mistake/intentionally? Hard to tell). I reverted that change back now and the original test assertion now succeeds, so we can minimize the impact of this PR (and hopefully get it merged soon). @acozzette - please trigger a new kokoro run so we can see if this is now (finally!) CI clean.

[perlun]

Thanks; there still seems to be some build failures (but the logs are still inaccessible to me, so slightly hard to see what they are). 😅

[acozzette]

@perlun Sorry for the trouble, something's wrong with the permissions on our test logs. Here is the relevant error, though:

[ RUN      ] CommandLineInterfaceTest.InputsFromDescriptorSetInAndFileSystem_UnusedImportIsReported
google/protobuf/compiler/command_line_interface_unittest.cc:441: Failure
Expected equality of these values:
  0
  return_code_
    Which is: 1
[  FAILED  ] CommandLineInterfaceTest.InputsFromDescriptorSetInAndFileSystem_UnusedImportIsReported (16 ms)

[perlun]

@perlun Sorry for the trouble, something's wrong with the permissions on our test logs. Here is the relevant error, though:

No probs. 👍 Thanks for that. Interestingly enough, I don't think I'm able to reproduce this locally (not 100% sure since I started editing the code a bit too fast...). Anyhow, I made the new field default to false now more explicitly + swapped the order of the assertions in CommandLineInterfaceTest::ExpectWarningSubstring - if it fails again, we should hopefully get the invocation output in the log and not just the 1 != 0 part.

@acozzette please get the CI started one more time, maybe we're good to go this time. 🤞 😄

[acozzette]

The Python errors look to be unrelated, so I will go ahead and merge this. Thanks, @perlun!

[perlun]

Thanks for the merge @acozzette, greatly appreciated! 🙏 👍

[benesch]

Woohoo! Thanks very much for the persistence @perlun!