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

pkgsStatic: set BUILD_SHARED_LIBS=OFF for cmake #76659

Merged
merged 5 commits into from Jan 6, 2020

Conversation

veprbl
Copy link
Member

@veprbl veprbl commented Dec 29, 2019

Motivation for this change

While reviewing #75798 I've noticed a repetitive pattern of setting BUILD_SHARED_LIBS=OFF for many of the cmake builds. It makes sense to just set it for all builds.

Background

CMake has a BUILD_SHARED_LIBS option which governs the behaviour of the add_library() function in case when the library type is not specified. Its default value is "OFF" meaning default to STATIC. It is not uncommon to explicitly redefine the option:

option(BUILD_SHARED_LIBS "Build shared instead of static library" OFF)

This adds the BUILD_SHARED_LIBS option to the list of options in the CMake gui. There is a different variation of the redefinition where the default value is changed to "ON".

Some packages allow to provide both shared and static libraries. Additional custom options are defined to facilitate this. These options may or may not break the default convention for the BUILD_SHARED_LIBS.

In Nixpkgs

Here are a few data points for nixpkgs:

# grep -r -iE "\\-DBUILD_SHARED_LIBS=(ON|TRUE|1)" pkgs/ | wc -l
39
# grep -r -iE "\\-DBUILD_SHARED_LIBS=(OFF|FALSE|0)" pkgs/ | wc -l
2
# grep -r "\\-DBUILD_SHARED_LIBS" . | wc -l
46
# grep -r "\\-DBUILD_STATIC_LIBS" . | wc -l
4
# grep -r "\\-DBUILD_" . | grep SHARED | wc -l
48
# grep -r "\\-DBUILD_" . | grep STATIC | wc -l
9

This PR should help with 39 cases where we enabled building of shared libraries. It will also help in cases when the default value of the BUILD_STATIC_LIBS option was changed to "ON"

In upstream

Things done

fmt

# ls -1 $(nix-build . -A fmt)/lib
libfmt.so
libfmt.so.6
libfmt.so.6.0.0
# ls -1 $(nix-build . -A pkgsStatic.fmt)/lib
libfmt.a

gtest

# ls -1 $(nix-build . -A gtest)/lib
libgmock_main.so
libgmock.so
libgtest_main.so
libgtest.so
# ls -1 $(nix-build . -A pkgsStatic.gtest)/lib
libgmock.a
libgmock_main.a
libgtest.a
libgtest_main.a

Copy link
Contributor

@tobim tobim left a comment

Choose a reason for hiding this comment

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

Does this make any existing pkg overrides obsolete? If so, you should remove those from the explicit set in static.nix.

@veprbl
Copy link
Member Author

veprbl commented Dec 29, 2019

@tobim Yes, it does, but only two or three cases. I will make sure to address those.

@FRidh FRidh moved this from Needs review to WIP in Staging Dec 31, 2019
@ofborg ofborg bot requested a review from jeroendehaas January 1, 2020 16:56
Copy link
Contributor

@jeroendehaas jeroendehaas left a comment

Choose a reason for hiding this comment

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

Looks fine to me!

@matthewbauer
Copy link
Member

See @orivej comments on this:

CMake does not have BUILD_STATIC_LIBS option: its meaning is project-dependent and by default it's unset. I don't think we should disable it here.

My impression is that smaller (single library) projects indeed depend on BUILD_SHARED_LIBS to select between static and shared build, as the new variable dontDisableStatic implies, but the effect of BUILD_SHARED_LIBS on larger projects is completely different.

from #56391 (comment)

It may still be okay to just enable it in the static overlay though. The above PR was doing this for everything.

@veprbl
Copy link
Member Author

veprbl commented Jan 2, 2020

See @orivej comments on this:

CMake does not have BUILD_STATIC_LIBS option: its meaning is project-dependent and by default it's unset. I don't think we should disable it here.

My impression is that smaller (single library) projects indeed depend on BUILD_SHARED_LIBS to select between static and shared build, as the new variable dontDisableStatic implies, but the effect of BUILD_SHARED_LIBS on larger projects is completely different.

from #56391 (comment)

It may still be okay to just enable it in the static overlay though. The above PR was doing this for everything.

Yes, I was considering doing that. I've also seen other other custom options like BUILD_STATIC_AND_SHARED, BUILD_BOTH_STATIC_SHARED_LIBS and BUILD_STATIC, BUILD_LIB_STATIC. But it seems like the majority of custom options use an explicit prefix (like foobar_BUILD_SHARED), so those will need to be addressed individually anyway.

@veprbl veprbl force-pushed the pr/cmake_static_adapter branch 2 times, most recently from ff397be to 3128ed0 Compare January 3, 2020 00:17
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Some more packages need to be changed since #75798 was merged.

Staging automation moved this from WIP to Ready Jan 3, 2020
Static libraries are to be provided by the pkgsStatic.fmt package.
pkgsStatic.gtest already has CMAKE_BUILD_SHARED set to OFF.
pkgsStatic.glog already has CMAKE_BUILD_SHARED set to OFF.
pkgsStatic.double-conversion already has CMAKE_BUILD_SHARED set to OFF.
@FRidh FRidh merged commit f4b4ef1 into NixOS:staging Jan 6, 2020
Staging automation moved this from Ready to Done Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants