-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[cmake] Using Protobuf v27 make python pybind11 test failed #4307
Comments
Custom Protobufcloning Protobuf locally and playing with SHA1/patch git clone [email protected]:protocolbuffers/protobuf.git
cd protobuf
# reset to the commit to test
git reset --hard d21425d334f965650c3c66362b0d827797f059db
# Change -dev version suffix using v27.0 commit otherwise at runtime protobuf check version suffix and refuse to start
# we use tig to select the commit to cherry pick
tig v27.0
# inside tig press `C` to cherrypick, `Y` to confirm, then quit with `q`
# fix patch using mergetool
git mergetool
# remove .orig
git clean -n
git clean -f
# Patch CMake stuff
# note: could be protobuf-v27.patch
git apply --3way ~/work/main/patches/protobuf-v26.patch modify or-tools diff --git a/cmake/dependencies/CMakeLists.txt b/cmake/dependencies/CMakeLists.txt
index a5cfdeee13..fbd75fe544 100644
--- a/cmake/dependencies/CMakeLists.txt
+++ b/cmake/dependencies/CMakeLists.txt
@@ -106,13 +106,21 @@ if(BUILD_Protobuf)
#set(protobuf_BUILD_LIBUPB ON)
FetchContent_Declare(
Protobuf
- GIT_REPOSITORY "https://github.com/protocolbuffers/protobuf.git"
+ SOURCE_DIR "${CMAKE_SOURCE_DIR}/pb"
- GIT_SHALLOW TRUE
+ #GIT_SHALLOW TRUE
GIT_SUBMODULES ""
- PATCH_COMMAND git apply --ignore-whitespace "${CMAKE_CURRENT_LIST_DIR}/../../patches/protobuf-v27.patch")
+ #PATCH_COMMAND git apply --ignore-whitespace "${CMAKE_CURRENT_LIST_DIR}/../../patches/protobuf-v27.patch"
+ #PATCH_COMMAND git apply --ignore-whitespace "${CMAKE_CURRENT_LIST_DIR}/../../patches/protobuf-v26.patch"
+ )
FetchContent_MakeAvailable(Protobuf)
list(POP_BACK CMAKE_MESSAGE_INDENT)
message(CHECK_PASS "fetched") note: I've use a symbolic link in the root dir of my or-tools local repo. In or-tools: # clean protobuf build
rm -rf build/_deps/protobuf-build build/_deps/protobuf-src build/_deps/protobuf-subbuild
# rebuild
cmake --build build -j 8
# test
source build/python/venv/bin/activate
python ortools/routing/python/model_test.py Dev NoteRename the 'includingDefaultValueWithoutPresenceFields' and 'always_print_without_presence_fields' to 'alwaysPrintFieldsWithNoPresence':You also, may need to revert this commit e0a4dcf diff --git a/ortools/util/file_util.cc b/ortools/util/file_util.cc
index db638ca8f9..fcb661fcf1 100644
--- a/ortools/util/file_util.cc
+++ b/ortools/util/file_util.cc
@@ -165,7 +165,7 @@ absl::Status WriteProtoToFile(absl::string_view filename,
case ProtoWriteFormat::kJson: {
google::protobuf::util::JsonPrintOptions options;
options.add_whitespace = true;
- options.always_print_fields_with_no_presence = true;
+ options.always_print_primitive_fields = true;
options.preserve_proto_field_names = true;
if (!google::protobuf::util::MessageToJsonString(proto, &output_string,
options) if you are before this commit: Fun Fact: While release See only commit between a and b[To only list commit [a, b[ you can use: tig a...b ResultsUsing a bisect approach to find the first borken commit(s)...
|
TLDR: using Protobuf (cmake based build) as a "Static" library will lead to an ODR violation, until now this ODR was without impact/side effect but the Protobuf change this behaviour and make it crash the runtime. |
Also seems to break
|
Tests impacted
here the list of impacted tests...
To test:
note: using Protobuf 58baeb4c3b664f8918d24cef5151083d9da9767c everything pass...
Regression seem to have been introduced by http://cl/602725967
Investigation
Testing one broken test
cmake -S. -Bbuild -DBUILD_PYTHON=ON -DBUILD_CXX_SAMPLES=OFF -DBUILD_CXX_EXAMPLES=OFF cmake --build build -j 8 source build/python/venv/bin/activate python ortools/routing/python/model_test.py
Trace:
or-tools/ortools/routing/python/model_test.py
Lines 648 to 651 in f9adb26
continue to investigate...
ref: https://github.com/protocolbuffers/protobuf/blob/63def39e881afa496502d9c410f4ea948e59490d/src/google/protobuf/generated_message_reflection.cc#L3616-L3620
The text was updated successfully, but these errors were encountered: