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 3.20.1 #99843

Closed
wants to merge 18 commits into from
Closed

protobuf 3.20.1 #99843

wants to merge 18 commits into from

Conversation

owine
Copy link
Sponsor Contributor

@owine owine commented Apr 22, 2022

Created by brew bump


Created with brew bump-formula-pr.

@BrewTestBot BrewTestBot added automerge-skip `brew pr-automerge` will skip this pull request bump-formula-pr PR was created using `brew bump-formula-pr` labels Apr 22, 2022
@scpeters scpeters added long build Set a long timeout for formula testing CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Apr 22, 2022
@cho-m cho-m added CI-linux-self-hosted Build on Linux self-hosted runner and removed CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. labels Apr 23, 2022
@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. To keep this pull request open, add a help wanted or in progress label.

@github-actions github-actions bot added the stale No recent activity label Apr 25, 2022
@cho-m cho-m added in progress Stale bot should stay away and removed stale No recent activity labels Apr 26, 2022
@chenrui333 chenrui333 added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Apr 29, 2022
@cho-m cho-m removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Apr 30, 2022
@SMillerDev SMillerDev mentioned this pull request May 16, 2022
6 tasks
@chenrui333
Copy link
Member

chenrui333 commented May 28, 2022

protobuf changed the version scheme, here is the doc for it

Versioning
We changed our versioning scheme to enable more-nimble updates to language-specific parts of Protocol Buffers. In the new scheme, each language has its own major version that can be incremented independently of other languages, as covered later in this topic with the Python release. The minor and patch versions, however, will remain coupled. This allows us to introduce breaking changes into some languages without requiring a bump of the major version in languages that do not experience a breaking change.

The first instance of this new versioning scheme is the new version of the Python API, 4.21.0, which follows the preceding version, 3.20.1. Other language APIs will be released as 3.21.0.

it might be good shipping this release before bumping to 21.1

@owine
Copy link
Sponsor Contributor Author

owine commented May 31, 2022

Sorry, all - I have taken this one as far as I am capable. I would appreciate an assist in keeping this one moving or I can close in favor of another attempt.

@chenrui333 chenrui333 added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label May 31, 2022
@fxcoudert fxcoudert removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Jun 1, 2022
@cho-m
Copy link
Member

cho-m commented Jun 3, 2022

For error:

Undefined symbols for architecture arm64:
    "google::protobuf::internal::InternalMetadata::~InternalMetadata()", referenced from:

The issue is that destructor should be defined in header when using NDEBUG https://github.com/protocolbuffers/protobuf/blob/v3.20.1/src/google/protobuf/metadata_lite.h#L73-L81

#if defined(NDEBUG) || defined(_MSC_VER)
  ~InternalMetadata() {
    if (HasMessageOwnedArenaTag()) {
      delete reinterpret_cast<Arena*>(ptr_ - kMessageOwnedArenaTagMask);
    }
  }
#else
  ~InternalMetadata();
#endif

However, this information isn't propagated to dependent builds, so they end up trying to use #else (assuming they don't set NDEBUG) and this fails since Homebrew's protobuf was compiled with NDEBUG.


One possibility is to get upstream to add flag as part of pkgconfig file https://github.com/protocolbuffers/protobuf/blob/v3.20.1/protobuf.pc.in when library is built with it.

This would then propagate the -DNDEBUG to all dependents that use pkg-config.

EDIT: though not sure how this may impact projects also uses (N)DEBUG and user wants to compile with DEBUG but gets NDEBUG due to pkg-config flags. In this case, protobuf would need a more specific solution to avoid impact of commonly used build variables.

The workarounds available is to manually append -DNDEBUG to CXXFLAGS for every dependent that is hitting this issue.

@SMillerDev
Copy link
Member

Closing in favor of #105712

@SMillerDev SMillerDev closed this Jul 14, 2022
@carlocab
Copy link
Member

@owine, don't delete your branch yet. It seems like we might still want this update, as #105712 is blocked.

@pitrou
Copy link

pitrou commented Jul 19, 2022

@cho-m For the record, the ABI issue should be fixed in protobuf 21.3: protocolbuffers/protobuf#10273

@carlocab
Copy link
Member

Thanks for the fix @pitrou!

@owine owine deleted the bump-protobuf-3.20.1 branch July 26, 2022 13:17
@github-actions github-actions bot added the outdated PR was locked due to age label Aug 26, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 26, 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 bump-formula-pr PR was created using `brew bump-formula-pr` CI-linux-self-hosted Build on Linux self-hosted runner CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. in progress Stale bot should stay away 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.

9 participants