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: revert to 3.12.2 #58104

Closed
wants to merge 1 commit into from

Conversation

AustinShalit
Copy link
Contributor

@AustinShalit AustinShalit commented Jul 16, 2020

This reverts commit a4511d0.
This reverts commit 13b1c5f.

  • Have you followed the guidelines for contributing?
  • 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 <formula>)?

There are multiple issues with the 3.12.3 release of protobuf that the maintainers said would be fixed with a 3.12.4 release but that update has not arrived. These issues are with the release itself and not particular bugs in the library.

protocolbuffers/protobuf#7600
protocolbuffers/protobuf#7632
protocolbuffers/protobuf#7637
protocolbuffers/protobuf#7627

@AustinShalit AustinShalit changed the title protobuf: revert to 3.12.3 protobuf: revert to 3.12.2 Jul 16, 2020
@Bo98
Copy link
Member

Bo98 commented Jul 16, 2020

Perhaps we should switch to using protobuf-all-3.12.3.tar.gz rather than the tag? I believe the tarballs don't have the issue.

@AustinShalit
Copy link
Contributor Author

Maybe?

The backstory here is I am on a project whose build process requires all protobuf versions (native, Java, Python) to be the same. Protobuf v3.12.3 was never pushed to Maven or PyPI (presumably because it was tagged incorrectly) leading to the direct issue that I am trying to solve.

@AustinShalit
Copy link
Contributor Author

Note that CI will fail because it thinks this is a downgrade from a stable version. I guess the argument that I am trying to make with this PR is that 3.12.3 is not a stable version and was accidentally released.

@Bo98
Copy link
Member

Bo98 commented Jul 16, 2020

was accidentally released

Upstream seem to disagree on that. In fact, the PHP version of 3.12.3 was published to PECL yesterday.

@peternewman
Copy link
Contributor

peternewman commented Jul 23, 2020

The backstory here is I am on a project whose build process requires all protobuf versions (native, Java, Python) to be the same. Protobuf v3.12.3 was never pushed to Maven or PyPI (presumably because it was tagged incorrectly) leading to the direct issue that I am trying to solve.

Likewise here!

Upstream seem to disagree on that. In fact, the PHP version of 3.12.3 was published to PECL yesterday.

Maybe, although no comment on their issue and still no Java:
https://search.maven.org/artifact/com.google.protobuf/protobuf-java
Or Python:
https://pypi.org/project/protobuf/#history
releases.

Although they have admitted a likely error here:
protocolbuffers/protobuf#7632 (comment)

This is causing issues for others too:
vgteam/vg@928ad64

This reverts commit 13b1c5f.
This reverts commit a4511d0.
This reverts commit f3bbd3e.
This reverts commit 74f7123
@AustinShalit
Copy link
Contributor Author

I've rebased the PR to also revert the build from tar because building from tar will not help the version mismatch issue or issues as described by the comment here: protocolbuffers/protobuf#7632 (comment)

@AustinShalit
Copy link
Contributor Author

It looks like they are going to cut a release to 3.12.4 soon "because 3.12.3 was also mistakenly tagged at the wrong commit"

protocolbuffers/protobuf#7600 (comment)

@kevinburke1
Copy link

Commit f3bbd3e changes this to use the protobuf-all tarball. I think this is a better fix.

I had previously modified our build process to compile a deb package from the Git tag, to match Homebrew, now I don't have a good way of installing the latest protoc commit.

@iMichka
Copy link
Member

iMichka commented Jul 28, 2020

If we really want to do this, we need to use version_scheme 1 to tell the formula that the number is starting at 0 again, to force an update on everybody. We would like to avoid this though if it is not necessary.

When is the next release planned?

@SMillerDev
Copy link
Member

They are clearly just having issues with their releases and homebrew doesn't generally revert a version because upstream broke it. It's not up to us to be quality control for everything we ship.

@AustinShalit
Copy link
Contributor Author

Looks like they are pushing 3.12.4 today. Do we want to revert the build from tar when we bump the version?

protocolbuffers/protobuf#7600 (comment)

@SeekingMeaning
Copy link
Contributor

Do we want to revert the build from tar when we bump the version?

Yes please

@AustinShalit AustinShalit mentioned this pull request Jul 29, 2020
5 tasks
@Bo98
Copy link
Member

Bo98 commented Jul 29, 2020 via email

@kevinburke1
Copy link

The advantage of a git tag is that you can easily reproduce what's in the Homebrew release by cloning the repo and checking out the given git tag. It is unclear how they are building tarballs, and they have at least once put incorrect contents in it.

@AustinShalit
Copy link
Contributor Author

AustinShalit commented Jul 29, 2020

It was working great with the git tag method before 3.12.3 and switching to the tarball method did not fix the issue that this PR was trying to fix. Seems like the burden should be on those who want to use the tarball method rather than those who want to use the git clone method.

As an aside, this conversation would've been great to have in a pull request to switch to the tarball method rather than after a direct commit to the repo. But that is water under the bridge at this point; just something to learn from.

@Bo98
Copy link
Member

Bo98 commented Jul 29, 2020

switching to the tarball method did not fix the issue that this PR was trying to fix

No, it was not intended to fix the issue of upstream not uploading artifacts for other languages to PECL etc - only upstream can fix that. It was intended to revert the breaking changes that were introduced from the misplaced git tag, which broke dependents like caffe. It did that job successfully.

It is unclear how they are building tarballs

We've always preferred distribution tarballs, where available. I'm fine with using git tags if there's a strong reason for protobuf to be different than everything else.

they have at least once put incorrect contents in it.

That would indeed be an issue. Do you know when that was? I'm not finding anything on the issue tracker.

this conversation would've been great to have in a pull request to switch to the tarball method rather than after a direct commit to the repo

It was not a direct commit to the repo. The pull request was open for around 12 hours (with no objections, and none afterwards), and would have been longer if the git tag wasn't causing actual real-world issues like #58210. It's unfortunate that the breaking change issue with the git tag wasn't caught earlier, but it ultimately makes the transition to 3.12.4 easier as 3.12.4 would have otherwise been another breaking change moving the tag to the correct branch - no revision bumps should be required to dependents as a result.

@SeekingMeaning
Copy link
Contributor

Sorry for the misunderstanding everyone 😰

For reference, this is the PR that switched to using tarball: #58229

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants