onnx and ORT mismatch in their handling of Protobuf_USE_STATIC_LIBS and ORT can produce incorrect warnings and failures #12867
Comments
sstamenova
changed the title
|
You are right. Neither ORT nor protobuf use this Protobuf_USE_STATIC_LIBS variable. If you search it in https://github.com/protocolbuffers/protobuf, you will not find anything. So, in normal usages you should not need to worry about this macro. |
Protobuf_USE_STATIC_LIBS seems to only be used in a status message in the onnx cmake setup. https://github.com/onnx/onnx/search?q=Protobuf_USE_STATIC_LIBS. It doesn't seem to have any influence over how protobuf is built. Based on that, I'm not following how not setting that value would affect a build and reproduce the issue. |
CMake uses the value to determine which libs to use for protobuf: https://github.com/Kitware/CMake/blob/master/Modules/FindProtobuf.cmake I recommend trying it locally to see it reproduce. |
|
Oh ok. I didn't realize it was a value used internally by cmake. Could you please share the full command you're using to build? We generally build using build.py, and there's only one place that sets Protobuf_USE_STATIC_LIBS, and that requires additional options for it to be hit. In my typical Windows build I don't use those additional options so Protobuf_USE_STATIC_LIBS is not explicitly set and I have no issues. e.g. this is the cmake command build.py generates |
|
Our team includes ort through our cmake file like so: and passing You can reproduce it by invoking cmake directly: Or using |
|
Got it. build.py does not set the experimental onnxruntime_PREFER_SYSTEM_LIB, and it has this comment in the cmake file. #The onnxruntime_PREFER_SYSTEM_LIB is mainly designed for package managers like apt/yum/vcpkg.
#Please note, by default Protobuf_USE_STATIC_LIBS is OFF but it's recommended to turn it ON on Windows. You should set it properly when onnxruntime_PREFER_SYSTEM_LIB is ON otherwise you'll hit linkage errors.
#If you have already installed protobuf(or the others) in your system at the default system paths(like /usr/include), then it's better to set onnxruntime_PREFER_SYSTEM_LIB ON. Otherwise onnxruntime may see two different protobuf versions and we won't know which one will be used, the worst case could be onnxruntime picked up header files from one of them but the binaries from the other one.
#It's default OFF because it's experimental now.
option(onnxruntime_PREFER_SYSTEM_LIB "Experimental: Build with the preinstalled libraries in your system" OFF)My initial thought would be we could change this to make the ONNX build consistent. onnxruntime/cmake/CMakeLists.txt Lines 788 to 811 in 1e34440 by changing it to also set ONNX_USE_PROTOBUF_SHARED_LIBS # We have a check here but most of the cmake users don't know the Protobuf_USE_STATIC_LIBS
# variable exists and may leave it in a wrong state.
# Also need to make onnx build params consistent as (currently) that uses the value of ONNX_USE_PROTOBUF_SHARED_LIBS
# to set Protobuf_USE_STATIC_LIBS. This is somewhat fragile as changes in the ONNX cmake setup may break this.
if(NOT Protobuf_USE_STATIC_LIBS)
# ONNX Runtime itself can work in such a setting but it may cause compatibility issues
# when ONNX Runtime is integrated with the other ONNX ecosystem softwares.
message(WARNING "Use Protobuf_USE_STATIC_LIBS to ensure compatibility with other ONNX ecosystem components.")
set(ONNX_USE_PROTOBUF_SHARED_LIBS ON)
else()
set(ONNX_USE_PROTOBUF_SHARED_LIBS OFF)
endif()However this is only in a branch that is taken when onnxruntime_PREFER_SYSTEM_LIB is set. @snnn do we need to make sure ONNX_USE_PROTOBUF_SHARED_LIBS is consistent with Protobuf_USE_STATIC_LIBS for all builds? |
|
Setting It would be better, I think, to expose something like |
|
According to https://cmake.org/cmake/help/latest/module/FindProtobuf.html the default of Protobuf_USE_STATIC_LIBS is OFF. So if that is not set or OFF wouldn't it be using the shared protobuf libs and ONNX_USE_PROTOBUF_SHARED_LIBS should be ON to make the onnx build consistent? With that setup, is it not the case that If you wish to use static libs you can set Protobuf_USE_STATIC_LIBS. What value would adding ORT_USE_PROTOBUF_SHARED_LIBS provide apart from having a different default? |
The variable Protobuf_USE_STATIC_LIBS takes effect only when onnxruntime_PREFER_SYSTEM_LIB is ON. And the onnxruntime_PREFER_SYSTEM_LIB variable was added only for Linux package managers. Linux only. Most of the code was added by the community and maintained by the community. So, everyone is welcomed to make it better. Also, everyone is welcomed to review such changes. On the other hand, Microsoft teams should use https://github.com/microsoft/vcpkg . Besides, the var Protobuf_USE_STATIC_LIBS was added and used by the cmake community, not Google staffs. I think from the very beginning it shouldn't exist. The dev team of protobuf team do not suggest using protobuf as a dynamical library. There are tons of issues related to it and simply nobody won't fix them. Or, won't be able to fix them. In my opinion, nobody have the need to dynamically link to protobuf. If someone does it, they are wrong. In some very limited usage cases, it may still work. But, if it doesn't work, don't report it Google and don't report it to us. One such case is: If you want to use the full version of protobuf(onnxruntime_USE_FULL_PROTOBUF=ON), you should only use the static version of protobuf. In ORT's CMakeLists.txt we have: if (onnxruntime_USE_FULL_PROTOBUF)
set(PROTOBUF_LIB protobuf::libprotobuf)
#We have a check here but most of the cmake users don't know the Protobuf_USE_STATIC_LIBS
# variable exists and may leave it in a wrong state.
if (NOT Protobuf_USE_STATIC_LIBS)
# ONNX Runtime itself can work in such a setting but it may cause compatibility issues
# when ONNX Runtime is integrated with the other ONNX ecosystem softwares.
message(WARNING "Use Protobuf_USE_STATIC_LIBS to ensure compatibility with other ONNX ecosystem components.")
endif()
endif()But initially it wasn't a warning. It was a fatal error. Then a 3rd-party user asked me to relax the check because he believed he could make it work in his use cases. So I let him do it. But I would not help fix any problem coming from it. Because I know there are many blockers there. Even the dev team of protobuf wouldn't want to touch them, why would I? And what value does it add? |
|
@snnn a couple of things:
I think it's a great idea to enforce that it is explicitly set to However, even if |
|
If we step back a bit and assume there was just one library consuming libprotobuf, then who should set this Protobuf_USE_STATIC_LIBS variable? I think it should be the user who invoke the cmake command. Only they know whether the preinstalled protobuf lib is static or dynamic. For example, if I wrote a simple C/C++ program which just read a ONNX model and print out all the names of all the ops, I can do it by directly interact with protobuf, without the need of using onnx or onnxruntime. And that simple program could either use a static protobuf library or a dynamic one. Both work good enough. So I believe the program's CMakeFile.txt should not try to set a default value for Protobuf_USE_STATIC_LIBS . I think the name "Protobuf_USE_STATIC_LIBS " is a bit misleading. It is not an ON/OFF control option. When you set it to ON, it doesn't mean cmake will help you find a static library of protobuf. Conversely, it is the way to let cmake know the lib cmake found is static or not. If you don't tell it, cmake would assume you have installed a dynamic one. So, my opinion is whenever you use https://github.com/Kitware/CMake/blob/master/Modules/FindProtobuf.cmake you should always manually specify Then back to ONNX: the var ONNX_USE_PROTOBUF_SHARED_LIBS is just another name of Protobuf_USE_STATIC_LIBS . ONNX expect you using ONNX_USE_PROTOBUF_SHARED_LIBS instead of Protobuf_USE_STATIC_LIBS. I don't know the reason. Probably because they want to support the case that you have both static protobuf lib and dynamic protobuf in the same application. For example, in ORT while the core framework prefer to static link to protobuf, an ORT EP could still either statically link to protobuf or dynamically. Then it might be good to have two different variables to control the two different things. But, it is just my guessing. I don't know why ONNX decided to introduce a new variable to control this. |
|
If you set onnxruntime_USE_CUDA = ON, you build ONNX Runtime with CUDA. If you set onnxruntime_USE_CUDA = OFF, you build ONNX Runtime without CUDA. But Protobuf_USE_STATIC_LIBS doesn't work in that way. On Windows You should set Protobuf_USE_STATIC_LIBS to ON if you know the protobuf lib cmake will find will definitely be a static lib. And you set it to OFF if you know it would be definitely dynamic. If you are not sure, you run cmake and see what it find, then do it again and set Protobuf_USE_STATIC_LIBS correctly according to the previous run's result. It is not intuitive but it is how this cmake module works. |
Describe the bug
onnx supports setting
ONNX_USE_PROTOBUF_SHARED_LIBStoONorOFFon Windows and by extension settingProtobuf_USE_STATIC_LIBStoONorOFF. The default isONNX_USE_PROTOBUF_SHARED_LIBSset toOFFandProtobuf_USE_STATIC_LIBSset toON.onnxruntime, on the other hand, does not change
Protobuf_USE_STATIC_LIBS, so it is set toOFFby default in the onnxruntime build and set toONby default in the onnx build within onnxruntime resulting in build failures.The onnxruntime cmake also produces a warning if
Protobuf_USE_STATIC_LIBSis not set toONregardless of what it is set to foronnx, so it will produce a warning even if it is intended to be set toOFFfor both onnx and onnxruntime.Instead, onnxruntime should use the same defaults as onnx, or possibly even set
ONNX_USE_PROTOBUF_SHARED_LIBSitself.For a related issue in onnx, which has been fixed, see: onnx/onnx#3345.
Urgency
None
System information
To Reproduce
Build ORT and onnx together on Windows without setting
Protobuf_USE_STATIC_LIBStoONExpected behavior
The build succeeds without any protobuf related issues or warnings.
Instead we get warnings and failures like:
The text was updated successfully, but these errors were encountered: