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

Protobuf 21.2 #105712

Closed
wants to merge 29 commits into from
Closed

Protobuf 21.2 #105712

wants to merge 29 commits into from

Conversation

ezzieyguywuf
Copy link
Contributor

Note: Starting with
v21.0-rc1,
protobuf is using a new versioning scheme.

Signed-off-by: Wolfgang Sanyer [email protected]

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

This is a duplicate of #105661, where I accidentally deleted my branch b/c I
thought the PR was merged - or rather, I accidentally deleted my branch and then
the PR was closed and then I thought the PR was merged.

@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Jul 13, 2022
@carlocab carlocab added CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. long build Set a long timeout for formula testing labels Jul 13, 2022
@carlocab
Copy link
Member

Btw, the extra commits are not unrelated. We need them to avoid breaking dependents when we ship the new version of protobuf.

@ezzieyguywuf
Copy link
Contributor Author

Btw, the extra commits are not unrelated. We need them to avoid breaking dependents when we ship the new version of protobuf.

ah hah, yea I sort of deduced that but thanks for clarifying that for me.

carlocab added a commit to carlocab/homebrew-core that referenced this pull request Jul 13, 2022
Let's try this to get Homebrew#105712 over the line.
@carlocab carlocab added the CI-linux-self-hosted Build on Linux self-hosted runner label Jul 13, 2022
@carlocab
Copy link
Member

Rebased again. Forgot the CI-linux-self-hosted label.

carlocab added a commit that referenced this pull request Jul 13, 2022
Let's try this to get #105712 over the line.
@cho-m
Copy link
Member

cho-m commented Jul 14, 2022

With new version scheme, do we know how upstream will update protobuf-all tarball when only a single language runtime is updated? Is it going to be modified in-place and trigger checksum issues in Homebrew?

If I understand correctly, the upstream's version scheme is to allow them to keep the same version (e.g. 21.2) while independently modifying a language version (e.g. update Python from 4.21.2 to 5.21.2).


On side note, this will hit same issue we saw in other PR #99843 (comment).

Upstream has closed issues related to this so might need to manually pass NDEBUG to dependents that use the particular header:

@ezzieyguywuf
Copy link
Contributor Author

With new version scheme, do we know how upstream will update protobuf-all tarball when only a single language runtime is updated? Is it going to be modified in-place and trigger checksum issues in Homebrew?

I imagine this would trigger a minor or patch version bump - from the news item

The minor and patch versions, however, will remain coupled

so even if only one language has a small tweak, all languages get a new release with a new sha.

You can see this (sort of) with the changes from v21.0 to v21.1. Note that although only the C++ and Python languages received changes (so two languages, not one like in your question), all languages received a version bump.

If I understand correctly, the upstream's version scheme is to allow them to keep the same version (e.g. 21.2) while independently modifying a language version (e.g. update Python from 4.21.2 to 5.21.2).

Yes I think you're understanding this correctly.

@fxcoudert fxcoudert mentioned this pull request Jul 14, 2022
@carlocab carlocab removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Jul 14, 2022
@carlocab
Copy link
Member

Will need rebasing after #105756 is merged (leave that to me).

Failures in formulae modified here (let's ignore dependents for now):

All macOS

brew install --verbose --build-bottle protobuf-c
brew install --verbose --build-bottle bloaty
brew install --verbose --build-bottle libphonenumber
brew install --verbose --build-bottle mosh
brew install --build-from-source protobuf-c
brew install --only-dependencies --verbose --build-bottle netdata
brew install --verbose --build-bottle netdata
brew install --verbose --build-bottle percona-xtrabackup
brew test --verbose protoc-gen-grpc-web

And on Monterey (both ARM and Intel)

brew test --verbose ola

Linux

brew install --verbose --build-bottle libphonenumber
brew install --verbose --build-bottle percona-xtrabackup
brew test --verbose protoc-gen-grpc-web

percona-xtrabackup is a bug in the formula; it's installing files that conflict with percona-server. @Bo98, did you mention that you knew how to fix this?

Less sure about the rest. Presumably, a number of the --build-bottle failures will be fixed by passing -DNDEBUG.

@carlocab
Copy link
Member

Formulae affected by the NDEBUG-dependent definition of ~InternalMetadata:

  • protobuf-c
  • bloaty
  • mosh
  • netdata
  • ola

libphonenumber is erroring out with

  /tmp/libphonenumber-20220713-64831-eg9ry8/libphonenumber-8.12.44/_deps/abseil-cpp-src/absl/base/policy_checks.h:79:2: error: "C++ versions less than C++14 are not supported."
  #error "C++ versions less than C++14 are not supported."
   ^

Not sure what's going on with protoc-gen-grpc-web:

  ==>Testing protoc-gen-grpc-web
  ==>protoc test.proto --plugin=/usr/local/Cellar/protoc-gen-grpc-web/1.3.1_1/bin/protoc-gen-grpc-web --js_out=import_style=commonjs:. --grpc-web_out=import_style=typescript,mode=grpcwebtext:.
  protoc-gen-js: program not found or is not executable
  Please specify a program using absolute path or make sure the program is available in your PATH system variable
  --js_out: protoc-gen-js: Plugin failed with status code 1.

@cho-m
Copy link
Member

cho-m commented Jul 14, 2022

Not sure what's going on with protoc-gen-grpc-web:

  ==>Testing protoc-gen-grpc-web
  ==>protoc test.proto --plugin=/usr/local/Cellar/protoc-gen-grpc-web/1.3.1_1/bin/protoc-gen-grpc-web --js_out=import_style=commonjs:. --grpc-web_out=import_style=typescript,mode=grpcwebtext:.
  protoc-gen-js: program not found or is not executable
  Please specify a program using absolute path or make sure the program is available in your PATH system variable
  --js_out: protoc-gen-js: Plugin failed with status code 1.

From grpc/grpc-web#1251, looks like protoc-gen-grpc-web may not work with current protobuf as upstream says to use 3.20.1 instead.

This PR may be blocked until we get a release for https://github.com/protocolbuffers/protobuf-javascript

@carlocab
Copy link
Member

carlocab commented Jul 14, 2022

grpc/grpc-web#1251

This PR may be blocked until we get a release for https://github.com/protocolbuffers/protobuf-javascript

Looks like 3.20.1 isn't affected by this problem. We might want to consider updating to that in the meantime. I think that should be enough to unblock mobile-shell/mosh#1210, which seems to be what prompted this PR.

Or, does it make sense to add https://github.com/protocolbuffers/protobuf-javascript as a resource to protoc-gen-grpc-web? Or maybe even to protobuf?

@cho-m
Copy link
Member

cho-m commented Jul 14, 2022

Looks like 3.20.1 isn't affected by this problem. We might want to consider updating to that in the meantime. I think that should be enough to unblock mobile-shell/mosh#1210, which seems to be what prompted this PR.

I guess one option is to finish #99843 (3.20.1) and then rebase this PR to continue 21.2 update. Most of failures in that PR are probably the same as what we see here.


Or, does it make sense to add https://github.com/protocolbuffers/protobuf-javascript as a resource to protoc-gen-grpc-web? Or maybe even to protobuf?

One related question is how many users would be impacted by missing Javascript functionality (as seen by users commenting in protobuf repo issue: protocolbuffers/protobuf-javascript#127).

New Javascript runtime repo is still showing version as 3.20.1 https://github.com/protocolbuffers/protobuf-javascript/blob/main/package.json#L3. Haven't actually checked on how it is supposed to be built/used.

@cho-m
Copy link
Member

cho-m commented Jul 14, 2022

Based on last commit in repo (PR: protocolbuffers/protobuf-javascript#1), it doesn't seem possible to build yet.

@carlocab
Copy link
Member

carlocab commented Jul 14, 2022

Based on last commit in repo (PR: protocolbuffers/protobuf-javascript#1), it doesn't seem possible to build yet.

...oh. That's unfortunate.

@SMillerDev SMillerDev mentioned this pull request Jul 14, 2022
carlocab added 15 commits July 15, 2022 02:26
Also, set `-DNDEBUG` to ensure that we have a proper destructor for
`InternalMetadata`.
Also, set `-DNDEBUG` to ensure we have a proper destructor for
`InternalMetadata`.
Also, set `-DNDEBUG` to ensure we have a proper destructor for
`InternalMetadata`.
Conflict with `percona-xtrabackup` to avoid a `brew install` failure in
CI.

This needs a proper fix, but we use this for now to unblock the
`protobuf` version bump.
Conflict with `percona-server` to avoid a `brew install` failure in CI.

This needs a proper fix, but we use this for now to unblock the
`protobuf` version bump.
Also, set `-DNDEBUG` to ensure that we have a proper destructor for
`InternalMetadata`.
This doesn't work with the latest `protobuf`, so depend on `protobuf@3`,
which provides Protobuf 3.20.1.
@carlocab
Copy link
Member

ARM builds are green.

Also, we may want to migrate this to using CMake at some point:

protocolbuffers/protobuf#10209 (comment)
protocolbuffers/protobuf#9392 (comment)

@carlocab
Copy link
Member

carlocab commented Jul 15, 2022

All builds are 🍏. Only dependent tests running on 11 and 12.

system "cmake", ".", *std_cmake_args
system "make", "install"
# https://github.com/protocolbuffers/protobuf/issues/9947
ENV.append_to_cflags "-DNDEBUG"
Copy link
Member

Choose a reason for hiding this comment

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

It's a little weird that we need this, since I always thought that CMAKE_BUILD_TYPE=Release already added -DNDEBUG to C[XX]FLAGS.

It turns out that this is only true for CMAKE_C_FLAGS:

❯ cat CMakeLists.txt
cmake_minimum_required(VERSION 3.20)
project(flags C)
message(STATUS "CMAKE_C_FLAGS_RELEASE = ${CMAKE_C_FLAGS_RELEASE}")
message(STATUS "CMAKE_CXX_FLAGS_RELEASE = ${CMAKE_CXX_FLAGS_RELEASE}")
❯ cmake .
-- The C compiler identification is AppleClang 13.1.6.13160021
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- CMAKE_C_FLAGS_RELEASE = -O3 -DNDEBUG
-- CMAKE_CXX_FLAGS_RELEASE =
-- Configuring done
-- Generating done
-- Build files have been written to: /var/folders/kb/pf4wbfc512z47l56g5072l900000gn/T/tmp.KkoU3ADrCO

Copy link
Member

Choose a reason for hiding this comment

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

Ah, no, it's definitely there:

❯ cat CMakeLists.txt
cmake_minimum_required(VERSION 3.20)
project(flags C CXX)
message(STATUS "CMAKE_C_FLAGS_RELEASE = ${CMAKE_C_FLAGS_RELEASE}")
message(STATUS "CMAKE_CXX_FLAGS_RELEASE = ${CMAKE_CXX_FLAGS_RELEASE}")
❯ cmake .
-- The C compiler identification is AppleClang 13.1.6.13160021
-- The CXX compiler identification is AppleClang 13.1.6.13160021
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /Library/Developer/CommandLineTools/usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- CMAKE_C_FLAGS_RELEASE = -O3 -DNDEBUG
-- CMAKE_CXX_FLAGS_RELEASE = -O3 -DNDEBUG
-- Configuring done
-- Generating done
-- Build files have been written to: /var/folders/kb/pf4wbfc512z47l56g5072l900000gn/T/tmp.KkoU3ADrCO

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

carlocab added a commit to carlocab/homebrew-core that referenced this pull request Jul 15, 2022
We're close to merging Homebrew#105712, so we can remove the lines that were
added in Homebrew#105715.
MikeMcQuaid pushed a commit that referenced this pull request Jul 15, 2022
We're close to merging #105712, so we can remove the lines that were
added in #105715.
@ezzieyguywuf
Copy link
Contributor Author

Thank you everyone who helped with this!

@github-actions github-actions bot added the outdated PR was locked due to age label Sep 3, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request CI-linux-self-hosted Build on Linux self-hosted runner CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. long build Set a long timeout for formula testing outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants