-
Couldn't load subscription status.
- Fork 9.1k
HADOOP-19605. Upgrade Protobuf 3.25.5 for docker images #7780
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
base: trunk
Are you sure you want to change the base?
Conversation
|
(!) A patch to the testing environment has been detected. |
| make install && | ||
| cmake -S . -B build -Dprotobuf_BUILD_TESTS=OFF && | ||
| cmake --build build --parallel $(nproc) && | ||
| cmake --install build --prefix /opt/protobuf && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protocolbuffers/protobuf#10132 (since protobuf 3.22.0) removes autotools support, the supported toolchains are bazel and cmake.
| { | ||
| "name": "protobuf", | ||
| "version": "3.21.12" | ||
| "version": "3.25.5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this works, but I don't have Windows env to verify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for PR @pan3793. Please let me know once you're done with all the changes and I can verify it on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GauthamBanasandra Now it passes on all Linux containers. Due to my limited CPP experience, my change might be dirty ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GauthamBanasandra do you remember why we should install libprotobuf-dev and libprotoc-dev through APT? seems we can remove it and always use the manually installed protobuf instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These libraries are needed for C/C++ code that uses protobuf. Regarding installing these from apt v/s manual installation - I would prefer the former wherever possible as it keeps the environment clean. It's easy to remove or upgrade the libraries when installed through apt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GauthamBanasandra, but it has already been installed install-protobuf.sh. I think the major advantage of manual installation is to make the protobuf version consistent with Java and across all Linux distributions.
Anyway, duplicated installation is not a major issue. Could you please review this PR? I tested it in all current Dockerfiles, but my changes in CMake files might be dirty (sorry, I'm not familiar with cpp toolchains)
|
💔 -1 overall
This message was automatically generated. |
| tar xzf /opt/protobuf.tar.gz --strip-components 1 -C /opt/protobuf-src && | ||
| mkdir -p /opt/abseil-cpp-src && | ||
| curl -L -s -S \ | ||
| https://github.com/abseil/abseil-cpp/archive/refs/tags/20230802.1.tar.gz \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recent protobuf versions have hard dependency on abseil, the version is defined at
https://github.com/protocolbuffers/protobuf/blob/v3.25.5/protobuf_deps.bzl#L46
|
(!) A patch to the testing environment has been detected. |
| $ cmake -S . -B build -Dprotobuf_BUILD_TESTS=OFF | ||
| $ cmake --build build --parallel $(nproc) | ||
| $ cmake --install build | ||
| $ protoc --version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now it returns 25.5 (seems protobuf changed version policy recently)
chengpan@a1847ca27697:~/hadoop$ protoc --version
libprotoc 25.5
|
💔 -1 overall
This message was automatically generated. |
|
CentOS 7 CI failure caused by GLIBC version too low, which is already tracked by YARN-11794 and HADOOP-19489 Update: CentOS 7 was removed from trunk. |
|
hadoop-hdfs-native-client building fails, further fix is required. convert to draft now, will investigate later Update: I fixed it |
1be1417 to
eb4b43e
Compare
|
(!) A patch to the testing environment has been detected. |
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
79886df to
8434cdd
Compare
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
8434cdd to
cf6a61b
Compare
|
(!) A patch to the testing environment has been detected. |
|
💔 -1 overall
This message was automatically generated. |
|
@steveloughran protobuf upgrading is done here, I made it work at least on the Linux platform, but I'm not very familiar with cpp toolchains, the CMake files changes might need to be reviewed by a domain expert. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
- If we hit problems, we can revert.
- Needs a backport PR for branch-3.4 and its release
- abseil is just google's C++ version of guava isn't it? code their stuff depends on with no stability guarantees
Description of PR
HADOOP-19289 upgraded protobuf-java 3.25.5, we should use same version for protobuf installed in docker images.
How was this patch tested?
Changes are covered by CI, also manually tested in local
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?