onnx_proto symbols visibility clean-up #3371
Conversation
Signed-off-by: mbencer <mateusz.bencer@intel.com>
mbencer
force-pushed
the
mbencer/OnnxProtoSymbolsVisibility
branch
from
59e7031
to
ef56321
Compare
Signed-off-by: mbencer <mateusz.bencer@intel.com>
mbencer
force-pushed
the
mbencer/OnnxProtoSymbolsVisibility
branch
from
c2a0383
to
700545e
Compare
|
@askhade Do you maybe know why |
|
@mbencer: Some of the cmake options are legacy and I am not sure why they were added. I need to take a thorough look at this PR before I can approve it. Will add my comments by today or tomorrow |
|
@askhade Do you have an update for this PR? |
| @@ -414,7 +429,8 @@ if(CMAKE_SYSTEM_NAME STREQUAL "AIX") | |||
| # So, create a object library | |||
| add_library(onnx OBJECT ${ONNX_SRCS}) | |||
| else() | |||
| add_library(onnx ${ONNX_SRCS}) | |||
| # onnx target doesn't export symbols and should be built as static lib | |||
| add_library(onnx STATIC ${ONNX_SRCS}) | |||
There was a problem hiding this comment.
can we keep this default meaning let BUILD_SHARED_LIBS control whether we use STATIC or SHARED
what is the problem with building shared lib when build_shared_lib is set?
There was a problem hiding this comment.
The features (classes/members/functions) from onnx library don't use decorators to control visibility.
It is especially required for Windows - __declspec.
So the idea behind it was to block building shared onnx if it is not supported.
| set(ONNX_API_DEFINE "-DONNX_API=__attribute__\(\(__visibility__\(\"default\"\)\)\)") | ||
| set_target_properties(onnx_proto PROPERTIES CXX_VISIBILITY_PRESET hidden) | ||
| set_target_properties(onnx_proto PROPERTIES VISIBILITY_INLINES_HIDDEN 1) | ||
| target_compile_definitions(onnx_proto PUBLIC "ONNX_API=__attribute__\(\(__visibility__\(\"default\"\)\)\)") |
There was a problem hiding this comment.
what is the difference between original set vs the PR version target_compile_definitions(onnx_proto ?
is set global ?
There was a problem hiding this comment.
Previous version set value on directory level - it will be visible in current directory scope.
In the current version, we match it only with the target which uses it (it will be visible for onnx_proto code and for targets which use onnx_proto).
So from a functional point of view, it's rather not a big difference.
CMakeLists.txt
Outdated
| # onnx_proto symbols visibility | ||
| if(MSVC) | ||
| if(ONNX_BUILD_MAIN_LIB) | ||
| # ONNX_BUILD_MAIN_LIB can be be set if onnx_proto is static, |
|
FYI - If your project is itself a DLL intended for use by third-party software, we recommend that you do NOT expose protocol buffer objects in your library's public interface, and that you statically link protocol buffers into your library. |
|
There is a long thread relating to this in a previous PR: please see: #1938 . Among other things, there is a comment towards the end of the thread that defining the preprocessors macros in the cmake breaks the pytorch build. |
|
I did not fully understand the context here. Is the goal to produce an ONNX dll that statically links onnx_proto as well as protobuf? Or, is it something else? |
The goal for us was to build: shared |
mbencer
force-pushed
the
mbencer/OnnxProtoSymbolsVisibility
branch
2 times, most recently
from
8f6c4cd
to
b514ae3
Compare
…SymbolsVisibility Signed-off-by: mbencer <mateusz.bencer@intel.com>
mbencer
force-pushed
the
mbencer/OnnxProtoSymbolsVisibility
branch
from
b514ae3
to
2781c41
Compare
The problem was described in #3319
In short, currently, there is no possibility for import
onnx_protosymbols on Windows (when we buildonnx_protoas shared lib).The basic solution was introduced in #3335, but there are many doubts around this topic.
The main problem seems to be specifying
onnx_protovisibility both in onnx_pb.h header and in cmake.My proposition is unification and one place to defining symbols visibility strategy - cmake file.
ONNX_BUILD_SHARED_LIBSflag seems to be unuseful - a way of building is determined byBUILD_SHARED_LIBSso it was removed, but please let me know if there are some scenarios to use it.