CMake: Allow using BUILD_SHARED_LIBS to choose static/shared libs

[aperezdc]

By convention projects using CMake which can build either static or shared libraries use a BUILD_SHARED_LIBS flag to allow selecting between both: the add_library() command automatically switches between both using this variable when the library kind is not passed to add_library(). It is also usual to expose the BUILD_SHARED_LIBS as an user-facing setting with the option() command.

This way, the following will both work as expected:

   % cmake -DBUILD_SHARED_LIBS=OFF ...
   % cmake -DBUILS_SHARED_LIBS=ON ...

This is helpful for distributions which need (or want) to build only static libraries.

---

The above being said, I understand that there might be some reason why it is desirable that both kinds of libraries are always built. If that is the case, let me know and I'll try to come up with some other way of allowing to only build static libraries, and installing them in a way that other projects trying to link with -lbrotli* (and/or using pkg-config) will find the static libraries.

[aperezdc]

I have just pushed an update which modifies the c/fuzz/test_fuzzer.sh script accordingly. This should make Travis-CI happy again.

[aperezdc]

For some more context (and an example), I am trying to address building for an Linux uCLibc target which uses only static libraries, like in this Buildroot build — and in general making it easier for getting only static libraries as a result of the build, with the same base names as the shared libraries.

[eustas]

Can't remember the full motivation, but we moved to static+shared build in #599.
Every output artifact is produced by a separate build target, so building just static libraries is easy:
make brotlidec-static brotlienc-static

[aperezdc]

@eustas: I saw the brotlidec-static and brotlienc-static targets, but using those means that every distributor needs to manually adapt build recipes to only build those two targets. Which I can agree would be acceptable.

But then distributors need to trust that renaming libraries (libbrotli-static.a → libbrotli.a, and so on) will keep working, and that the .pc file will be valid to find the renamed libraries, or generate their own. IMHO it would be desirable that the output from install target of the build system is directly useable — both for static and shared library builds.

Do you think it would be more acceptable to leave both targets for static and shared libraries, then when cmake -DBUILD_SHARED_LIBS=OFF is used:

• Disable the targets for shared libraries.
• Static libraries are built with their current names (lib*-static.a).
• The install adds symlinks lib*.a → lib*-static.a.

Once again, thank you for your time, and let me be clear that I just want to try to make it easier for packagers to adopt the library, and that I don't have anything against building both version by default 😉

[eustas]

Perhaps the change was done to fix #542. Going to dig in further. Conceptually, there are no "contra" against this shared/static switch, while all parties (developers/distros) are happy =)

[aperezdc]

In case it helps: I have been looking a bit at the woff2 CMake build definitions, and it does use BUILD_SHARED_LIBS as proposed in this PR.

[eustas]

Here is a commit that would allow disabling shared/static libraries build (or both =). WDYT?

[eustas]

I think the reason to have both shared and static is that brotli tool is build "static" (for grab-and-go scenario), but libraries are "shared", because it is what they are created for...

[AppVeyorBot]

✅ Build brotli 1.0.0#1398 completed (commit 86e54fca39 by @aperezdc)

[aperezdc]

@eustas Coming back to this topic... The solution proposed in fb139e4 works fine with BUILD_SHARED_LIBS=ON and BUILD_STATIC_LIBS=OFF:

• Only shared libraries are build.
• The .pc files are installed and using them results in linking against the shared libraried.

But using BUILD_SHARED_LIBS=OFF and BUILD_STATIC_LIBS=ON results in no .pc files being installed, so other packages which use them to determine how to link against the library won't build. Moreover, the names of the static libraries have a -static suffix, so even if they try the usual library names for linking without relying on having .pc files, likely they would use e.g. -lbrotlidec (and not -lbrotlidec-static).

I think the approach in fb139e4 would do if the .pc files were also installed for the static libraries, so pkg-config --static … can work.

[xhochy]

I would be interested in reactivating this PR. @eustas Would you be open to this approach or prefer to use your commit as stated in fb139e4 ?

My motivation is that in conda-forge, we as a distribution prefer dynamic linkage as this is manageable by our toolchain and static libraries aren't used but consume unnecessary space in the packages.

[aperezdc]

I have updated the patch to work with the current state of the tree, which also works for version 1.0.9—something like this is still desired for packaging in Buildroot so for now it will continue carrying this patch to be applied after unpacking the sources.

Please let me know if there is some other way in which I could improve these changes to have them merged, I would be more than happy to iterate over the patch and have it included in Brotli 😸

[aperezdc]

The CIFuzz failure is caused by the projects/brotli/build.sh script in the oss-fuzz repository hardcoding the brotlidec-static; it would need to be changed to pass -DBUILD_SHARED_LIBS=OFF to CMake.

[AppVeyorBot]

✅ Build brotli 1.0.0#1531 completed (commit 2dad627000 by @aperezdc)

[AppVeyorBot]

✅ Build brotli 1.0.0#1532 completed (commit e29904ab22 by @aperezdc)

[delroth]

I see #766 was merged and released in 1.0.8. Is this PR now obsolete?

[aperezdc]

I see #766 was merged and released in 1.0.8. Is this PR now obsolete?

@delroth It's not the same, note that #766 only allows to disable building shared libraries, but does not remove the -static part the the static library names, and does not make the pkg-config .pc files usable with the static libraries—this PR does.

[ILW000]

Hello. Are the old (libbrotli-static.a) and new binaries (libbrotli.a) the same?

[aperezdc]

Hello. Are the old (libbrotli-static.a) and new binaries (libbrotli.a) the same?

Yes, the lib*.a files built with this patch applied are bit-by-bit equal to the old lib*-static.a files:

% md5sum build/*.a 
2edb88652563ba78cd4f863e4d884723  build/libbrotlicommon.a
2edb88652563ba78cd4f863e4d884723  build/libbrotlicommon-static.a
d35dbe1d59060611de85fcf05693275b  build/libbrotlidec.a
d35dbe1d59060611de85fcf05693275b  build/libbrotlidec-static.a
ff8b3edac99cc8dacb2d9427d5dc5061  build/libbrotlienc.a
ff8b3edac99cc8dacb2d9427d5dc5061  build/libbrotlienc-static.a
%

[aperezdc]

Patch rebased, and [RFC] tag removed from PR title—we have been using this patch for a bit over three years in Buildroot without any issue. I am still convinced that doing this in the standard way using BUILD_SHARED_LIBS to let CMake determine what to build, is the correct way to do things, plus this patch also solves the issue of the .pc files not being installed.

[AppVeyorBot]

✅ Build brotli 1.0.0#1611 completed (commit 8b5919313a by @aperezdc)

[nh2]

Is it currently possible to build both shared and static libraries?

This would be the ideal use case for me: Being able to control their existence independently, so that you can also build both (with the same name, e.g. libbrotlienc.a, without -static.a suffix).

Some Linux distributions aim to build both flavours where possible, so that the end developer can choose between static and dynamic linking easily.

As in: https://stackoverflow.com/questions/2152077/is-it-possible-to-get-cmake-to-build-both-a-static-and-shared-library-at-the-sam

[kleisauke]

Thanks! I had to apply this patch as well:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -221,14 +221,12 @@ if(NOT BROTLI_BUNDLED_MODE)
     RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}"
   )
 
-  if(NOT BROTLI_EMSCRIPTEN)
-    install(
-      TARGETS ${BROTLI_LIBRARIES_CORE}
-      ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}"
-      LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}"
-      RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}"
-    )
-  endif()  # BROTLI_EMSCRIPTEN
+  install(
+    TARGETS ${BROTLI_LIBRARIES_CORE}
+    ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}"
+    LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}"
+    RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}"
+  )
 
   install(
     DIRECTORY ${BROTLI_INCLUDE_DIRS}/brotli

To ensure that the libraries are still installed when compiling with Emscripten.

[eustas]

LGTM modulo comments

[aperezdc]

Thanks! I had to apply this patch as well:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -221,14 +221,12 @@ if(NOT BROTLI_BUNDLED_MODE)
     RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}"
   )
 
-  if(NOT BROTLI_EMSCRIPTEN)
-    install(
-      TARGETS ${BROTLI_LIBRARIES_CORE}
-      ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}"
-      LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}"
-      RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}"
-    )
-  endif()  # BROTLI_EMSCRIPTEN
+  install(
+    TARGETS ${BROTLI_LIBRARIES_CORE}
+    ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}"
+    LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}"
+    RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}"
+  )
 
   install(
     DIRECTORY ${BROTLI_INCLUDE_DIRS}/brotli

To ensure that the libraries are still installed when compiling with Emscripten.

This PR does not change the existing logic for installation (which is: when targeting Emscripten, an install target for the libraries is not configured); if you want to suggest a change in that regard, that would be a separate issue/PR instead.

[aperezdc]

@eustas I have submitted an update with your suggestions, PTAL. Also the Bazel CI build is failing, but this patch only touches the CMake build—likely the Bazel build failure is caused by something completely different.

[kleisauke]

when targeting Emscripten, an install target for the libraries is not configured

FWIW, commit ce222e3 fixes that and this PR would cause a regression in that regard (i.e. it causes static libraries not to be installed when BROTLI_EMSCRIPTEN is defined).

that would be a separate issue/PR instead.

Sure, that comment was more FYI. I'll open a separate PR for that after this lands.

[eustas]

Thanks!