abedra / libvault

A C++ library for Hashicorp Vault
MIT License
34 stars 25 forks source link

Proposal: only install libs when shared #92

Closed lucasgonze closed 2 years ago

lucasgonze commented 2 years ago

Only Install Libs When Shared

Problem

The ability to compile as a static library is helpful. However, the library is still installed at the system level. It would prevent system sprawl to only install when compiled as a shared library.

Solution

Limit installation when compiling static

diff --git a/CMakeLists.txt b/CMakeLists.txt
index dce948c..0944a22 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -41,7 +41,7 @@ if (ENABLE_COVERAGE)
   target_link_libraries(vault gcov)
 endif ()

-if (UNIX)
+if (UNIX AND BUILD_SHARED_LIBS)
   install(TARGETS vault
       EXPORT libvaultTargets
       LIBRARY DESTINATION ${CMAKE_INSTALL_DIR}
abedra commented 2 years ago

Hi @lucasgonze. I think I understand your position and intention here, but is it really correct to not install targets if the library is built statically? Wouldn't it be better to just set the appropriate cmake options when running cmake initially so that things are installed to somewhere other than system folders?

Blzut3 commented 2 years ago

I'm guessing the OP is vendoring libvault. Built alone it would indeed be correct to install the static libraries since they would be referenced by the CMake config (for use with find_package). Usually the pattern I see is for libraries to provide an option to disable all install rules for the vendoring use case.

VanL commented 2 years ago

Hi @abedra, I am working with @lucasgonze on this. The end goal here is to be able to use libvault as a subproject (sometimes vendored, as @Blzut3 guessed) that is compiled into the target binary. However, the current makefile always tries to install the library, even when it is built statically. There is no real option to build it in-place and use it as a subtarget.

This can be done by changing the cmake to: if (UNIX AND BUILD_SHARED_LIBS) and updating the matching endif, but that may not be what is wanted here.

If it was possible to make that change, we could put together a PR. Alternatively, there could could be an option

option(INSTALL_LIBS "Install built library" ON)

that could be used to turn that capability off.

Would you include this functionality? Would one of these ways of doing it be better for you?

abedra commented 2 years ago

Thanks for clarifying the use case @VanL. I'm happy to offer a way to solve this. Understanding what you are trying to do here definitely helps. To clarify, you are looking to use the add_subdirectory CMake functionality, or just vendor the project in general?

abedra commented 2 years ago

@VanL / @lucasgonze please take a look at #94 and make sure that suits your needs. I gave it a try and found that it does not generate any install targets, so it should be good to go.

lucasgonze commented 2 years ago

@abedra That does the job. I appreciate very much what you did here.

abedra commented 2 years ago

This is now available in https://github.com/abedra/libvault/releases/tag/0.48.0