Revert "Disable clang warning in order to mitigate protocolbuffers/protobuf#9181" #1210
Conversation
…otobuf#9181" This reverts commit dbe419d. Since protocolbuffers/protobuf#9181 hase been resolved, this workaround should no longer be needed.
|
I was going to ask if the fixed protobuf package is available everywhere it needs to be, but it seems that our new github CI has answered that question for us 😃 |
|
CI is failing due to an old protobuf version in homebrew. See Homebrew/homebrew-core#105661 for an open PR that will update the version. |
|
My fear here would be that, because Homebrew doesn't (or didn't historically) track dependencies after installation, even after Homebrew updates the upstream version of protobuf, we still can't take this PR because it will break Mosh for every Homebrew user who has installed an old version of protobuf at any point. But I haven't looked at this in a long time. Can Homebrew set it up so if the user updates Mosh, it will also update the protobuf package? |
I don't know, but I will look into this. In most package managers that I've interacted with, it's been possible to set a minimum version for a given dependency. Therefore, I'd think we could set a minimum version for the next mosh version in homebrew and this would be resolved. I'll update this PR as I learn more. |
|
What about other clang-based platforms? Has the latest protobuf release been out long enough to make it into all of the necessary package repos? I don't necessarily mind breaking mosh builds if the answer is "run this command to get the latest protobuf". But if the answer is "wait a few more months for your distribution's package repo to get updated and then run this command to get the latest protobuf", then that's not good. Edit: I should clarify that breaking mosh builds in general is not good, so even if homebrew accepts your patch today, it may be prudent to wait some period of time to naturally let that updated protobuf package become more widely used, thus less likely to cause mosh build errors |
This is fixed since v3.20, which hit on march 25th, 2022 I can take a look at repology to see which versions are being distributed on various Linux distributions |
Yikes ,the badges (see below) sure shows a lot of red. This is somewhat misleading for a few reasons:
Here's an abbreviated summary:
So overall I guess it's not looking too good. I'm curious why the macos CI is the only one to fail though, I guess maybe b/c we're using gcc in the other environments? Either way, I guess it's still too soon for this PR - I'm happy to close it, or just let it sit here until the new protobuf has percolated into enough package managers. |
|
Thanks for putting together that summary
I think that's right. My reading of the protobuf issue is that it's specific to clang. I don't honestly know how common clang is (as the default compiler) on systems like Debian (but I did ask a friend who said it is "approximately never"). So Homebrew is obviously the primary concern here, and we definitely need to wait for that. Let's park this PR for now and revisit later. I don't think there's an urgent need to revert this patch. |
|
|
Homebrew is now on 21.2. Thanks @ezzieyguywuf for getting the ball rolling on it! |
|
Let's close this out for now. We should not merge this before the 1.4 release, but we can revisit after. |
Sgtm. I'm surprised that last CI run failed, it should be using the updated protoc Edit: interesting - i took a look and the errors looks different. Still protobuf related, but this time due to unused parameters. Should be fixable but i agree that this can wait till after 1.4 |
|
I think that's because of protocolbuffers/protobuf#10357, I have #1215 out to fix that. |
This reverts commit dbe419d.
Since protocolbuffers/protobuf#9181 hase been
resolved, this workaround should no longer be needed.