Symbols of onnx_proto target are not exposed correctly for Windows

[mbencer]

There is currently no way to export symbols from onnx_proto on Windows. Their visibility on other OSes is not a problem.

Problem scenario:
Target onnx_proto is used by more than one target (libraries).
onnx_proto (and onnx) is build as static lib.
libprotobuf is shared lib (ONNX_USE_PROTOBUF_SHARED_LIBS is set as ON).
When I run tests which use these two libraries which links to onnx_proto I see the error:

[libprotobuf ERROR /home/mbencer/workspace/openvino/build/_deps/ext_protobuf-src/src/google/protobuf/descriptor_database.cc:58] File already exists in database: onnx/onnx_ngraph_onnx-ml.proto
[libprotobuf FATAL /home/mbencer/workspace/openvino/build/_deps/ext_protobuf-src/src/google/protobuf/descriptor.cc:1370] CHECK failed: GeneratedDatabase()->Add(encoded_file_descriptor, size): 
terminate called after throwing an instance of 'google::protobuf::FatalException'
  what():  CHECK failed: GeneratedDatabase()->Add(encoded_file_descriptor, size): 
Aborted (core dumped)

The solution for me is build onnx_proto as shared lib (just by setting BUILD_SHARED_LIBS=ON).

Unfortunately, I encounter two other problems:
1. Symbols of onnx_proto target are not exposed correctly for Windows.
Linking attributes (such as dllimport, dllexport) for onnx_proto are set in onnx_pb.h file.

For Windows build it should be set as following:

• __declspec(dllexport) when we build onnx_proto target (it is set in cmake here
• __declspec(dllimport) for onnx_proto consumers (ONNX_BUILD_MAIN flag users is as I understand separate story) so in simply words for components which include onnx_pb.h header.

When we look at current onnx_pb.h implementation, there are no flags configuration which set __declspec(dllimport).
From my point of view __declspec(dllimport) should be set if ONNX_BUILD_SHARED_LIB=ON.

2. Target onnx should be build as static explicitly.
If I set BUILD_SHARED_LIBS=ON before start using onnx CmakeLists.txt (in my case by FetchContent), it causes that both onnx and onnx_proto are built as shared libraries.
There is a problem with dynamic onnx target, because component of onnx (classes, functions etc) have no linking decorators (such as dll_export) and generally I don't know cases when shared onnx target is needed.
I see two options:

• introduction cmake flags which allow to control if only onnx_proto is build as shared
• explicitly build onnx always as static (by changing line).

If there are no objections to such changes I can prepare PR but I am open to other ideas.

[ilya-lavrenov]

From my point of view __declspec(dllimport) should be set if ONNX_BUILD_SHARED_LIB=ON.

Usually, when BUILD_SHARED_LIBS is ON, export is defined. If we consume the library - import is defined.

Agree. For me, setting onnx as static seems a good solution.

[mbencer]

I've prepared PR with proposition of solution: #3335.
@ilya-lavrenov Please for review.

[snnn]

Have you read this: protocolbuffers/protobuf#1941 ?

[snnn]

First, I against doing this because protobuf's official document says:

"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."

It means you should not directly build onnx_proto and onnx as DLLs as what you are trying to do. It is not recommend by the authors of the protobuf library.

If you ask VC++ guys, usually they would say something similar. In general they recommend that you do not put STL containers(like std::vector) in DLL's exported symbols.

The second concern comes from protocolbuffers/protobuf#1941 . It happens when you dynamically link to libprotobuf. As Google guys suggested: "If A and B shares the same proto type, I believe you will need to make a separate shared lib C containing the shared proto and make it a dependency of A and B."

Here we assume:
A is ONNX.
B is another ONNX project like ngraph/ONNX Runtime.
C is another lib that should contain all the *.proto files. Such an approach works good for companies with a mono-repo, but not good for an open source project like ONNX. We wouldn't create a repo in https://github/onnx and ask all the companies put their protobuf definition files in. Let's say, I were from a startup company and I built a model server which use protobuf messages as the main RPC mechanism. And my RPC messages includes onnx.proto and use some of the types(like TensorProto). How would it work?

My opinion: ONNX should not be in the DLL business. I'd prefer keep it simple, clean and extensible. You can always build a DLL on top it in your place. If there is a gap we need to address, I'd love to provide help. But I don't think we should provide onnx.lib and onnx_proto.lib as DLLs.