search

minio / minio-cpp

MinIO C++ Client SDK for Amazon S3 Compatible Cloud Storage
https://minio-cpp.min.io/
Apache License 2.0
139 stars 56 forks source link

move to shared library format #177

Closed harshavardhana closed 1 week ago

kobalicek commented 2 months ago

I'm not sure what the intention is, but this should be configurable and not hardcoded. If your whole stack is static libraries, why would you want minio-cpp shared?

Additionally, this doesn't even consider dependencies. Is shared library compiling all dependencies statically or also as shared libraries? Because vcpkg prefers static, so you can be linking a lot of stuff twice, which means unreliable exception handling.

I'm in favor of maintaining the existing behavior (static) and adding an option to build a shared library, but not hardcoding that decision.

harshavardhana commented 2 months ago

Additionally, this doesn't even consider dependencies. Is shared library compiling all dependencies statically or also as shared libraries? Because vcpkg prefers static, so you can be linking a lot of stuff twice, which means unreliable exception handling.

is that because of MSVC requirement? can you show me where vcpkg indicates that they prefer static?

kobalicek commented 1 month ago

vcpkg uses triplets, which contain the information about the shared/static choice. Unfortunately with vcpkg it's either ALL static or ALL shared (conan supports this option per library). Some libraries can only be built as static though, so the triplet specifies the default when there is a choice.

The configuration option in vcpkg is called VCPKG_LIBRARY_LINKAGE:

But I would either create a custom option (like MINIO_CPP_SHARED=TRUE|FALSE) or use the cmake default, which should be automatically setup by vcpkg.

Here are my reasons against this change:

  1. There should always be a choice. Forcing building a shared library when the rest of the libraries are static is noting but pain for end users.

  2. ABI compatibility and not linking libraries twice. Since minio-cpp has many dependencies it should not happen that the dependencies, which are exposed in its API/ABI are linked statically - this creates problems for users that you won't be able to debug. One example is a third-party exception thrown from minio-cpp (with a statically compiled dependency) while the user is trying to catch it with another copy of a third-party dependency. This is actually a nightmare to debug and basically unsolvable when the linkage is mixed.

  3. API decorators. I have just checked the minio-cpp source code and I'm not sure how it's supposed to work. Shared library requires API decorators - when compiling for Windows you have to use export/import attribute, when compiling for non-windows platform there is a visibility attribute, which must be used as most code is compiled with -fvisibility=hidden option (to decrease the symbol bloat). If the API attribute is not there you are basically going to break your client's code.

BTW there is a reason why this commit hardcodes static library for Windows, because the source code it's missing the API decorators and Windows won't export anything if the API is not decorated. Windows linkage is similar to -fvisibility=hidden option.

harshavardhana commented 1 month ago

BTW there is a reason why this commit hardcodes static library for Windows, because the source code it's missing the API decorators and Windows won't export anything if the API is not decorated. Windows linkage is similar to -fvisibility=hidden option.

This is a fine choice, the main reason I am avoiding static is for GPU Direct dependency since there the libraries are shipped as shared and static compilation didn't work properly.

Let me see how to make this a cleaner choice.

kobalicek commented 1 month ago

I'm linking the following article that should explain what I'm talking about:

In other words, you cannot provide a good support for a shared library without using the API/EXPORT macro. It's simply not possible, and if you decide to build a shared library, you should build ALL dependencies that are exposed in minio-cpp API as shared too.

In addition, there should always be a choice, otherwise you are just telling your users to go away as shared library is something that most people just don't want today.

harshavardhana commented 1 month ago

I'm linking the following article that should explain what I'm talking about:

In other words, you cannot provide a good support for a shared library without using the API/EXPORT macro. It's simply not possible, and if you decide to build a shared library, you should build ALL dependencies that are exposed in minio-cpp API as shared too.

In addition, there should always be a choice, otherwise you are just telling your users to go away as shared library is something that most people just don't want today.

Default is back to what it was, you need to now pass a flag to build shared libs.

balamurugana commented 1 month ago

@kobalicek If we use separate build scripts/system for shared library in GNU/Linux, would it solve the issue?