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

Hard to deal with shared library #153

Closed
kerim371 opened this issue Aug 2, 2021 · 2 comments
Closed

Hard to deal with shared library #153

kerim371 opened this issue Aug 2, 2021 · 2 comments

Comments

@kerim371
Copy link
Contributor

kerim371 commented Aug 2, 2021

Hi,

I just started units library and I have found some problematic points when dealing with shared units library.

I successively installed shared units library with (Windows 10, MSVC 2019 x64):

  • cd C:/units
  • mkdir d
  • cd d
  • cmake .. -DUNITS_BUILD_SHARED_LIBRARY=ON -DUNITS_BUILD_STATIC_LIBRARY=OFF -DUNITS_BUILD_WEB_SERVER=OFF -DUNITS_ENABLE_TESTS=OFF
  • cmake --build . --config Debug
  • cmake --install . --prefix ../install_d --config Debug

Then I create new cmake project units_test where I want to check units functionality. Here is the simple CMakeLists.txt of my units_test program:

cmake_minimum_required(VERSION 3.5)

project(units_test LANGUAGES CXX)

set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

add_executable(units_test main.cpp)

find_package(units REQUIRED)

target_link_libraries(units_test PUBLIC units-shared)

and I pass -Dunits_DIR=/path/to/units/install_d/lib/cmake argument when configuring my units_test project.

First of all why you named differently units-shared target instead of simply units? I have seen this once in another library and this confuses me because that makes me to hardcode my CMake script to allow linking to third party library if it is built in static or shared mode.

Then in your examples you include headers as #include <units/units.hpp> but if I installed units then I can see the folder structure is:

/install
    /bin
    /include
    /lib
        /cmake

thus to include installed units I can only do #include <units.hpp>. This adds difficulties when somebody wil use my library (that depends on units) and link units via add_subdirectory(...). My #include <units.hpp> will give error because the right way in this case would be #include <units/units.hpp>

Also when trying to link to installed shared library I get error that units_decl.hpp can't find units/units_export.h. So I have to manually create folder units inside install_d/include/ and copy units_export.h (I guess it is automatically generated by CMake) there.

I propose:

  1. rename units-shared cmake target to standard units (do not make difference between shared/static/interface (header only) target`;
  2. when doing install we should install header files in install/include/units folder;
  3. copy units_export.h when doing installation.

P.S. You made a great library and I can see that it will help me a lot! Thank you!

@kerim371 kerim371 changed the title Hard to deal shared library Hard to deal with shared library Aug 2, 2021
@phlptp
Copy link
Collaborator

phlptp commented Aug 2, 2021

Thanks for the suggestions. None of our use cases ever use the shared library or install it, so it really hasn't gotten much attention.
We pretty much static link it as a submodule or just include the code directly. C++ shared libraries are always a bit dicey in my opinion. But regardless seems like it could be done better.

Would you propose disabling the ability to make both the static and shared libraries at the same time? If not how would you distinguish between them?

@kerim371
Copy link
Contributor Author

kerim371 commented Aug 2, 2021

@phlptp thank you for a quick reply,

Would you propose disabling the ability to make both the static and shared libraries at the same time? If not how would you distinguish between them?

In my experience there were no necessarity to create both static and shared libs at the same time. But I have seen such option few times even in quite big libraries (as I remember VTK provides this but I may be mistaken)

I think that units can be built static by default (i.e. when UNITS_BUILD_SHARED_LIBRARY=OFF) and only if UNITS_BUILD_SHARED_LIBRARY=ON then units should be built in shared mode.

BUT regarding how UNITS_HEADER_ONLY var is working the above proposition can be modified in the following way:

if I understand correct UNITS_HEADER_ONLY makes units to build and install headers only (something like copy include directories to install folder). In this case I think if UNITS_BUILD_SHARED_LIBRARY=ON then there is no need to process UNITS_HEADER_ONLY var because include folder will be copied anyway.
If UNITS_HEADER_ONLY=ON and UNITS_BUILD_SHARED_LIBRARY=OFF then we don't want to build neither static nor shared library and we install only include folder.
If UNITS_HEADER_ONLY=OFF and UNITS_BUILD_SHARED_LIBRARY=OFF then we build static library because there is nothinh else user can expect from building.

Also I expect that if a library provides all the three header only/shared/static mode then I don't want to distinguish my cmake code and C++ code with thoughts like: "What if my user wants to build my library and link to shared units library then I need... Or if he wants to link to static units...". This is super annoying :)

phlptp added a commit that referenced this issue Aug 12, 2021
Add `units_export.h` to `install/insclude/units` dir #153
@phlptp phlptp closed this as completed Aug 16, 2021
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

No branches or pull requests

2 participants