Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move to shared library format #177

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

harshavardhana
Copy link
Member

No description provided.

CMakeLists.txt Outdated Show resolved Hide resolved
@kobalicek
Copy link
Contributor

kobalicek commented Sep 19, 2024

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

harshavardhana commented Sep 20, 2024

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
Copy link
Contributor

kobalicek commented Sep 21, 2024

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
Copy link
Member Author

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.

Copy link
Contributor

@kobalicek kobalicek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved - although I think this is not usable without the API decorator and without removing other static libs from the public interface (I would not want to debug problems caused by this mix).

@balamurugana
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants