Update protobuf3 and use CMake #561
Conversation
dstoup
force-pushed
the
cmake-upgrade-protobuf3
branch
from
e31efc8 to
80fcb9e
Compare
|
Jenkins test this please |
|
@aaron-bray The dashboards passed but I think you'd mentioned something about kwiver's definitions needing to change as a result of pb3. Should I keep 2 as the default for now? Thinking that's the right thing anyway until we get buyin, but wanted to get a sense of the rest of the work left to do. |
CMake/External_Protobuf.cmake
Outdated
| ${COMMON_EP_ARGS} | ||
| ${COMMON_CMAKE_EP_ARGS} | ||
| # UPDATE_COMMAND | ||
| # COMMAND ${CMAKE_COMMAND} -DPULSE_IL2CPP_PATCH=${PULSE_IL2CPP_PATCH} -Dprotobuf_source=${protobuf_SRC} -Dprotobuf_patch=${protobuf_Patch} -P ${protobuf_Patch}/Patch.cmake |
There was a problem hiding this comment.
You can take this patch out. It was put in to work with the Unity IL2CPP compiler that converts C# to C++.
The IL2CPP compiler can be a bit too aggressive, so this patch was created
By the time you are supporting kwiver/fletch into C# and Unity, this will probably have changed
Just let me know then
There was a problem hiding this comment.
Thanks ... what are you thoughts about making protobuf3 the default? It certainly works building/testing kwiver on all platforms.
CMake/External_Protobuf.cmake
Outdated
| ${COMMON_CMAKE_ARGS} | ||
| -Dprotobuf_BUILD_TESTS:BOOL=OFF | ||
| -Dprotobuf_BUILD_EXAMPLES:BOOL=OFF | ||
| -Dprotobuf_BUILD_SHARED_LIBS:BOOL=${BUILD_SHARED_LIBS} |
There was a problem hiding this comment.
Please read this tid bit about protobuf linkage
https://github.com/protocolbuffers/protobuf/tree/master/cmake#dlls-vs-static-linking
I suggest we always build protobuf as static libs, and they should only be linked into the protobuf arrow and nothing else. Should work great in the kwiver architecture.
|
protobuf3 can read in and use protobuf2 files So you really don't need to move the kwiver proto files fro 2 to 3 immediately You should be able to remove protobuf2 altogether from fletch and just use 3 So I feel pretty confident we can nix proto2 from fletch all together and not sink any ships |
|
Perfect, thanks! I will keep protobuf3 as the default, leave protobuf2 for just a little while and look to remove it completely soon. I'm looking into the static thing as well. I suspect I will have to force PIC in those cases since it didn't build correctly out of the box that way (on Linux). I think VS was fine building static |
|
Note, |
dstoup
force-pushed
the
cmake-upgrade-protobuf3
branch
from
a69052e to
cafb37e
Compare
Currently this branch sets the default Protobuf version to 3 for testing purposes only. We have more work to do before version 3 can be the default.l If this branch passes dashboards, I will pull the version back to 2 and repush.