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.14.0 #64836

Closed
wants to merge 29 commits into from
Closed

protobuf 3.14.0 #64836

wants to merge 29 commits into from

Conversation

jeroen
Copy link
Contributor

@jeroen jeroen commented Nov 15, 2020

Created with brew bump-formula-pr.

resource blocks may require updates.

@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Nov 15, 2020
@chenrui333 chenrui333 removed the automerge-skip `brew pr-automerge` will skip this pull request label Nov 15, 2020
@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Nov 15, 2020
@chenrui333 chenrui333 removed the automerge-skip `brew pr-automerge` will skip this pull request label Nov 15, 2020
@jeroen
Copy link
Contributor Author

jeroen commented Nov 16, 2020

The grpc linking error is unrelated to protobuf. This is because one of the dependencies of grpc has changed to c++17.
grpc problems solved in: #64943

@jeroen jeroen mentioned this pull request Nov 16, 2020
@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Nov 16, 2020
@jeroen
Copy link
Contributor Author

jeroen commented Nov 16, 2020

Rebased for the updated grpc...

@SMillerDev
Copy link
Member

brew fetch --retry libphonenumber --build-bottle --force
brew install --verbose --build-bottle libphonenumber
brew test --retry --verbose torchvision

@jeroen
Copy link
Contributor Author

jeroen commented Nov 17, 2020

We now see 2 errors in reverse dependencies which are unrelated protobuf. libphonenumber seems to have moved a tag:

==> Downloading https://github.com/google/libphonenumber/archive/v8.12.13.tar.gz
Error: SHA256 mismatch
Expected: 1d0dab61ab0d4ffa9d86fd93a46b0f4df7ab159bf8b8f08416dc211a753747b2
  Actual: 4d202b3d20c545d39a9a0bbaf0f7f26e64a8875429bfadf36f7015adb9bada37
 Archive: /Users/brew/Library/Caches/Homebrew/downloads/27743676f863120797162238737b4e922e38442c316df1848d20bb017ac70e7a--libphonenumber-8.12.13.tar.gz
To retry an incomplete download, remove the file above.
Already downloaded: /Users/brew/Library/Caches/Homebrew/downloads/27743676f863120797162238737b4e922e38442c316df1848d20bb017ac70e7a--libphonenumber-8.12.13.tar.gz

And it looks like libtorchvision has been compiled agains xQuartz (which is in /opt/X11) which is no longer available?

==> Testing torchvision
/usr/bin/sandbox-exec -f /private/tmp/homebrew20201117-3508-1m51uot.sb ruby -W0 -I $LOAD_PATH -- /usr/local/Homebrew/Library/Homebrew/test.rb /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/torchvision.rb --verbose --retry
==> /usr/bin/clang++ -std=c++14 main.cpp -o test -I/usr/local/opt/libtorch/include -I/usr/local/opt/libtorch/include/torch/csrc/api/include -L/usr/local/opt/libtorch/lib -ltorch -ltorch_cpu -lc10 -L/usr/local/Cellar/torchvision/0.8.1/lib -ltorchvision
==> ./test
dyld: Library not loaded: /opt/X11/lib/libpng16.16.dylib
  Referenced from: /usr/local/opt/torchvision/lib/libtorchvision.dylib
  Reason: image not found

Should I bump these as well, and rebuild again?

@SMillerDev
Copy link
Member

For libphonenumber we should ask upstream if it was intentional, for torchvision we should figure out how to make it not do that. @gromgit might be able to help there.

@gromgit
Copy link
Contributor

gromgit commented Nov 17, 2020

@jeroen, based on my local tests, torchvision just needs a depends_on "libpng" and a revision bump.

@chenrui333 chenrui333 mentioned this pull request Nov 18, 2020
5 tasks
@jeroen
Copy link
Contributor Author

jeroen commented Nov 18, 2020

@chenrui333 I think this is ready to merge. The only remaining error is:

==> FAILED
Error: 1 problem in 1 formula detected
libphonenumber:
  * stable sha256 changed without the url/version also changing; please create an issue upstream to rule out malicious circumstances and to find out why the file changed.

The upstream git repository doesn't have an issue tracker, but they seem to be creating and moving the latest tag as part of the release process to test release candidates.

@chenrui333
Copy link
Member

yeah, let's move forward with the change.

chenrui333
chenrui333 previously approved these changes Nov 18, 2020
@BrewTestBot
Copy link
Member

:shipit: @chenrui333 has triggered a merge.

@BrewTestBot
Copy link
Member

⚠️ @chenrui333 bottle publish failed.

@BrewTestBot BrewTestBot dismissed chenrui333’s stale review November 18, 2020 23:29

bottle publish failed

@chenrui333
Copy link
Member

Auto-merging Formula/opencv.rb
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

@BrewTestBot BrewTestBot removed the automerge-skip `brew pr-automerge` will skip this pull request label Nov 19, 2020
@jeroen
Copy link
Contributor Author

jeroen commented Nov 19, 2020

Is it because opencv was updated between when yesterday and now? So we need to rebump the revision, even one more?

@chenrui333
Copy link
Member

yeah, that sounds about right, unfortunately. I just rebased and have another run before the merge.

@chenrui333 chenrui333 added the ready to merge PR can be merged once CI is green label Nov 19, 2020
@jeroen
Copy link
Contributor Author

jeroen commented Nov 19, 2020

Hmm OK, I think the PR now no longer contains a bump to opencv, so it will probably fail for that...

@chenrui333
Copy link
Member

Hmm OK, I think the PR now no longer contains a bump to opencv, so it will probably fail for that...

my bad

@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Nov 19, 2020
chenrui333 and others added 7 commits November 22, 2020 14:20
@BrewTestBot BrewTestBot removed the automerge-skip `brew pr-automerge` will skip this pull request label Nov 22, 2020
@chenrui333
Copy link
Member

merged the cmake fix and rebased the PR. 🤞

@chenrui333
Copy link
Member

rerun the build because 10.14 build somehow got skipped previously.

@chenrui333
Copy link
Member

I will do the merge around lunch today.

@SMillerDev
Copy link
Member

The upstream git repository doesn't have an issue tracker, but they seem to be creating and moving the latest tag as part of the release process to test release candidates.

Can you try and contact them regardless? Maybe tagging some maintainers here? The git manual says re-tagging is "the insane thing" to do and it just causes a lot of trouble for everyone.

chenrui333
chenrui333 previously approved these changes Nov 23, 2020
@BrewTestBot
Copy link
Member

:shipit: @chenrui333 has triggered a merge.

@chenrui333
Copy link
Member

Hi @rohininidhi or maybe @penmetsaa, 👋 we run into some libphonenumber audit check issue (since it got re-released?). Because there is no way for us to create an issue to track down on your side, I will tag you for clarifying the re-release is intentional for our better understanding the release process in general. Any feedback or comments are appreciated. Thanks!!

@BrewTestBot
Copy link
Member

⚠️ @chenrui333 bottle publish failed.

@BrewTestBot BrewTestBot dismissed chenrui333’s stale review November 23, 2020 17:24

bottle publish failed

@chenrui333
Copy link
Member

curl: (35) OpenSSL SSL_connect: SSL_ERROR_SYSCALL in connection to api.bintray.com:443

@BrewTestBot
Copy link
Member

:shipit: @chenrui333 has triggered a merge.

@chenrui333
Copy link
Member

Thanks @jeroen!!

@jeroen jeroen deleted the bump-protobuf-3.14.0 branch November 23, 2020 18:26
@jeroen
Copy link
Contributor Author

jeroen commented Nov 23, 2020

Thanks for your help! I think this issue shows that there is some room for improvement in the homebrew build process.

Perhaps the CI could automatically bump the required bottles, such that the PR is not constantly outdated/blocked due to other things changing at the same time.

@scpeters scpeters mentioned this pull request Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants