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
@veprbl veprbl mentioned this pull request Dec 29, 2019
3 of 10 tasks complete
@veprbl veprbl added this to WIP in Staging via automation Dec 29, 2019
@veprbl veprbl moved this from WIP to Needs review in Staging Dec 29, 2019
Copy link
Contributor

@tobim tobim left a comment

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 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 Jan 1, 2020
Copy link
Contributor

@jeroendehaas jeroendehaas left a comment

Looks fine to me!

@knedlsepp knedlsepp mentioned this pull request Jan 2, 2020
3 of 10 tasks complete
@matthewbauer
Copy link
Member

@matthewbauer matthewbauer 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.

@veprbl
Copy link
Member Author

@veprbl 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 veprbl:pr/cmake_static_adapter branch 2 times, most recently from ff397be to 3128ed0 Jan 3, 2020
@FRidh
FRidh approved these changes Jan 3, 2020
Copy link
Member

@FRidh FRidh left a comment

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

Staging automation moved this from WIP to Ready Jan 3, 2020
veprbl added 5 commits Dec 29, 2019
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.
@veprbl veprbl force-pushed the veprbl:pr/cmake_static_adapter branch from 3128ed0 to bb890c4 Jan 3, 2020
@FRidh FRidh merged commit f4b4ef1 into NixOS:staging Jan 6, 2020
16 checks passed
16 checks passed
double-conversion, fmt, glog, gtest on x86_64-darwin Failure
Details
Evaluation Performance Report Evaluator Performance Report
Details
double-conversion, fmt, glog, gtest on aarch64-linux Success
Details
double-conversion, fmt, glog, gtest on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
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
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.