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

Add more CMake flags #56391

Merged
merged 2 commits into from Mar 11, 2019
Merged

Add more CMake flags #56391

merged 2 commits into from Mar 11, 2019

Conversation

@matthewbauer
Copy link
Member

@matthewbauer matthewbauer commented Feb 26, 2019

Motivation for this change

Adds BUILD_TESTING, BUILD_SHARED_LIBS, and BUILD_STATIC_LIBS flags to Nixpkgs. BUILD_TESTING=OFF is set if doCheck = false. BUILD_SHARED_LIBS=ON, and BUILD_STATIC_LIBS=OFF to tell CMake to build the rights libs. These are reversed in static overlay.

This means we can avoid building test suites that will never be run.
Copy link
Contributor

@orivej orivej left a comment

I like the changes about BUILD_TESTING, but I doubt the changes about BUILD_SHARED_LIBS.

preCheck = if enableShared
then "export LD_LIBRARY_PATH=\"$PWD\""
else "";
preCheck = "export LD_LIBRARY_PATH=\"$PWD\"";
Copy link
Contributor

@orivej orivej Mar 11, 2019

Choose a reason for hiding this comment

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

This can be replaced by -DCMAKE_SKIP_BUILD_RPATH=OFF in cmakeFlags.

Overall, -DBUILD_SHARED_LIBS=ON has a high potential to break packages that use add_library without STATIC or SHARED for internal components and add_library(SHARED) (or STATIC with something like BUILD_STATIC_LIBS) for final artifacts. (This is a good practice for development.) One way they are likely to break is during testing due to our current default of -DCMAKE_SKIP_BUILD_RPATH=OFF. If -DBUILD_SHARED_LIBS=ON is to be the default, -DCMAKE_SKIP_BUILD_RPATH=OFF should probably be removed from the default.

Copy link
Member Author

@matthewbauer matthewbauer Mar 11, 2019

Choose a reason for hiding this comment

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

Yeah that makes sense to me. I'll leave of the BUILD_*_LIBS changes for now and just do the BUILD_TESTING change.

@@ -17,8 +17,6 @@ stdenv.mkDerivation rec {
preConfigure = "rm BUILD";

cmakeFlags = [
"-DBUILD_SHARED_LIBS=ON"
"-DBUILD_STATIC_LIBS=ON"
Copy link
Contributor

@orivej orivej Mar 11, 2019

Choose a reason for hiding this comment

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

Building the static library in all cases is intentional (needed by some packages that do not expect shared gflags).

@@ -60,6 +60,10 @@ cmakeConfigurePhase() {
# correctly detect our clang compiler
cmakeFlags="-DCMAKE_POLICY_DEFAULT_CMP0025=NEW $cmakeFlags"

if [ -z "$dontDisableStatic" ]; then
cmakeFlags="-DBUILD_SHARED_LIBS=ON -DBUILD_STATIC_LIBS=OFF $cmakeFlags"
Copy link
Contributor

@orivej orivej Mar 11, 2019

Choose a reason for hiding this comment

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

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.

This is now set by CMake
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants