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.

Sign up for GitHub

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

Jump to bottom

Update protobuf to 3.7.1 #7681

Closed
wants to merge 7 commits into from
Closed

Update protobuf to 3.7.1 #7681

wants to merge 7 commits into from

Conversation

davido
Copy link
Contributor

@davido davido commented Mar 9, 2019

No description provided.

@davido
Copy link
Contributor Author

davido commented Mar 9, 2019

//CC @drigz @laurentlb @meteorcloudy.

Note, that 3.7.1 wasn't tagged yet on the protobuf side. I used the code from this commit on 3.7.x branch: protocolbuffers/protobuf@4b9a5df. This was the backport of my fix from master: [1].

[1] protocolbuffers/protobuf#5811

davido added 3 commits March 9, 2019 14:05
@davido
Update protobuf to 3.7.1 (PR 1)
026afdb 
Closes bazelbuild#7611.

Change-Id: Ib6edee142ed3cf92806f2b63d119ae11ebe51a6f
@davido
Update protobuf to 3.7.1 (PR 2)
1397b69 
Closes bazelbuild#7611.

Change-Id: I3bae96836d9a25f48fed96338e7f4bf59a19d42c
@davido
Update protobuf to 3.7.1 (PR 3)
d6e6e77 
Closes bazelbuild#7611.

Change-Id: I95f61293971ce7dcf4557bf7cf2c8890544c7d31
@davido davido force-pushed the update-protobuf-to-3.7.1 branch from 498023f to 32b911d Compare March 9, 2019 13:29
@davido
Copy link
Contributor Author

davido commented Mar 9, 2019 •
edited
Loading

It seems that Protobuf is using bazel_skylib now. Tests are failing:

ERROR: /home/davido/.cache/bazel/_bazel_davido/4596c3304267e4c44bca87d41e038f29/sandbox/linux-sandbox/1669/execroot/io_bazel/_tmp/69b4d07d7f58456ce79df1b29fbdc766/workspace/test_modify_execution_info_various_types/BUILD:24:1: every rule of type proto_library implicitly depends upon the target '@com_google_protobuf//:protoc', but this target could not be found because of: error loading package '@com_google_protobuf//': in /home/davido/.cache/bazel/_bazel_davido/4596c3304267e4c44bca87d41e038f29/sandbox/linux-sandbox/1669/execroot/io_bazel/_tmp/69b4d07d7f58456ce79df1b29fbdc766/root/cccd58d5baf92601474e7dca6142d199/external/com_google_protobuf/protobuf.bzl: Unable to load package for '@bazel_skylib//lib:versions.bzl': The repository '@bazel_skylib' could not be resolved
ERROR: Analysis of target '//test_modify_execution_info_various_types:proto' failed; build aborted: error loading package '@com_google_protobuf//': in /home/davido/.cache/bazel/_bazel_davido/4596c3304267e4c44bca87d41e038f29/sandbox/linux-sandbox/1669/execroot/io_bazel/_tmp/69b4d07d7f58456ce79df1b29fbdc766/root/cccd58d5baf92601474e7dca6142d199/external/com_google_protobuf/protobuf.bzl: Unable to load package for '@bazel_skylib//lib:versions.bzl': The repository '@bazel_skylib' could not be resolved

In previous version, 3.6.1 Protobuf was using native.bazel_version version check. This was changed in this commit: protocolbuffers/protobuf@b2a1908. I don't think we want to bring bazel_skylib dependency to Bazel itself to bootstrap protobuf. So that the way forward could be to revert that commit in Bazel Protobuf fork and remove dependency on bazel_skylib, and check for the minimum required Bazel version in the same way, as it was done in older Protobuf versions:

  expected = apple_common.dotted_version("0.5.4")
  current = apple_common.dotted_version(native.bazel_version)
  if current.compare_to(expected) < 0:
    fail("Bazel must be newer than 0.5.4")

@davido
Update protobuf to 3.7.1 (PR 4)
32b911d 
Closes bazelbuild#7611.

Change-Id: I7557fddc7148d3933c1bf2223bab450f58c5e621
@aiuto aiuto added the area-EngProd Bazel CI, infrastructure, bootstrapping, release, and distribution tooling label Mar 13, 2019
@aiuto
Copy link
Contributor

aiuto commented Mar 13, 2019

Note to reviewers: I added people who had done this update before. Could one of you please volunteer to follow it by de-assigning the others, or fully assigning it to yourselves. We don't need 3 people on it.

@davido
Protobuf: Avoid dependency on bazel_skylib
9ae8ddd 
Revert protocolbuffers/protobuf@b2a1908 to avoid dependency on
bazel_skylib in Bazel core.
@laszlocsomor
Copy link
Contributor

David, what's to motivation for this PR, what is 3.6.1 (and 3.7.0) missing?

@davido
Copy link
Contributor Author

davido commented Mar 15, 2019

David, what's to motivation for this PR, what is 3.6.1 (and 3.7.0) missing?

Upgrade to the recent protobuf release (3.7.0) is needed to make Bazel build tool chain forward compatible (incompatible options).

Upgrade to 3.7.x branch is needed, because 3.7.0 is missing this fix: [1].

[1] protocolbuffers/protobuf#5811

@laszlocsomor
Copy link
Contributor

laszlocsomor commented Mar 15, 2019 •
edited
Loading

Thank you.

Please send out PR 1 alone first, and PRs 2..4 after PR 1 is merged.

Rationale: separately, they are easier to review and roll back.

@davido
Copy link
Contributor Author

davido commented Mar 17, 2019

@laszlocsomor

Please send out PR 1 alone first, and PRs 2..4 after PR 1 is merged.

OK, will do once I figured out how to fix test breakages. There are two complications compared to previous protobuf version upgrades:

  • Dependency on bazel_skylib
  • Dependency on zlib

@davido
Remove examples/third_party/zlib.BUILD to satisfy srcs test
bc145f0 
Remove symbolic link to make the tests pass:

third_party/protobuf/3.7.1/examples/third_party/zlib.BUILD to
../../third_party/zlib.BUILD
@laszlocsomor
Copy link
Contributor

@davido : Thanks. Let me know if you need help.

@davido
Copy link
Contributor Author

davido commented Mar 19, 2019 •
edited
Loading

@laszlocsomor Most of the test jobs are passing now. But some (flaky) are still failing. Do you have any clou, what is going on here? Thanks!

@laszlocsomor
Copy link
Contributor

I'll take a look.

@laszlocsomor
Copy link
Contributor

@davido: I'll create a separate PR of your PR 1 (026afdb). It must be merged directly to https://bazel.googlesource.com/bazel/+/master, and I don't know if you have permission to merge there. Does that sound good?

@davido
Copy link
Contributor Author

davido commented Mar 20, 2019

@laszlocsomor

Does that sound good?

Thank you. Your help is very much appreciated!

@laszlocsomor laszlocsomor mentioned this pull request Mar 20, 2019
Closed
@davido davido mentioned this pull request Mar 20, 2019
Closed
@iirina
Copy link
Contributor

iirina commented Mar 21, 2019

@laszlocsomor was this PR merged? Could you please close the PR when/if it's closed? Thanks!

@laszlocsomor laszlocsomor self-assigned this Mar 21, 2019
@laszlocsomor
Copy link
Contributor

laszlocsomor commented Mar 21, 2019 •
edited
Loading

@iirina : It wasn't, we're debating whether it's required. Will close afterwards.

See #7775

@davido davido mentioned this pull request Apr 5, 2019
Closed
@jin
Copy link
Member

jin commented Oct 2, 2019

@laszlocsomor can this be closed if #7775 is closed?

@laszlocsomor
Copy link
Contributor

Yes.

@philwo philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laszlocsomor laszlocsomor Awaiting requested review from laszlocsomor

Assignees

@laszlocsomor laszlocsomor

Labels
area-EngProd Bazel CI, infrastructure, bootstrapping, release, and distribution tooling cla: yes team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@davido @aiuto @laszlocsomor @iirina @jin @philwo @googlebot