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

Jump to bottom

arrow-cpp: add pkgsStatic support #75798

Merged
merged 11 commits into from Jan 3, 2020
Merged

arrow-cpp: add pkgsStatic support #75798

merged 11 commits into from Jan 3, 2020

Conversation

tobim
Copy link
Contributor

@tobim tobim commented Dec 16, 2019 •
edited

Motivation for this change
Things done

Added a static option to zstd, double-conversion, gflags, glog, gtest, woff2, snappy, thrift, and finally arrow-cpp.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@tobim tobim changed the title Pkgs static/arrow arrow-cpp: add pkgsStatic support Dec 16, 2019
@ofborg ofborg bot requested review from hrdinka, veprbl and bjornfor December 16, 2019 21:58
nh2
nh2 previously requested changes Dec 16, 2019
View reviewed changes
Copy link
Contributor

@nh2 nh2 left a comment

Choose a reason for hiding this comment

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

Great stuff!

Overall requests:

  • For packages that allow configuring the building of static and shared libs separately (thus, also allow building both of them), can we used the argument names enableStatic ? false and enableShared ? true instead of a single static ? false argument?
    That makes it a lot more obvious to figure out what exactly the option does. So far in nixpkgs, the semantics of just static is quite inconsistent: For some packages it additionally enables static libs, for some others it turns off shared libs and builds only static, and for a few (like zlib) it puts static libs into a separate .static output.
  • For the few packages that (usually for not-so-great reasons / bad upstream packaging reasons) allow only static XOR shared libs, could you point that out with a comment?

Thanks!

pkgs/development/libraries/arrow-cpp/default.nix Outdated Show resolved Hide resolved
@veprbl
Copy link
Member

veprbl commented Dec 17, 2019

One more thing: This PR should be targeting staging.

@FRidh FRidh changed the base branch from master to staging December 19, 2019 08:30
@FRidh FRidh added this to WIP in Staging via automation Dec 19, 2019
@ofborg ofborg bot requested a review from veprbl December 19, 2019 08:50
@ofborg ofborg bot requested a review from ttuegel December 19, 2019 22:16
tobim
tobim commented Dec 19, 2019
View reviewed changes
Copy link
Contributor Author

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

For packages that allow configuring the building of static and shared libs separately (thus, also allow building both of them), can we used the argument names enableStatic ? false and enableShared ? true instead of a single static ? false argument?
That makes it a lot more obvious to figure out what exactly the option does. So far in nixpkgs, the semantics of just static is quite inconsistent: For some packages it additionally enables static libs, for some others it turns off shared libs and builds only static, and for a few (like zlib) it puts static libs into a separate .static output.

For the few packages that (usually for not-so-great reasons / bad upstream packaging reasons) allow only static XOR shared libs, could you point that out with a comment?

In my opinion it is the other way around. Allowing the option to build static and shared libs in the same build tree is handing a foot gun to the libraries users. Upstream projects often do it anyway to either be more flexible in their development or for packaging for traditional distributions where everything goes to /usr/lib and the packager has no information about how the library will be used. But in nixpkgs exactly that information is available through that option, and with that any reason to put both into a build is gone.

Consequently, I think static is the correct name for that option and it would be better to change the existing packages that currently use a different interpretation for static.

@tobim
Copy link
Contributor Author

tobim commented Dec 20, 2019

I wasn't able to remove the python restriction. The actual culprit is openblas, which currently does not build with the static option due the omp.h not being found.
That file exists in regular gcc outputs but not in the musl-stage-final one.

@nh2
Copy link
Contributor

nh2 commented Dec 20, 2019

But in nixpkgs exactly that information is available through that option, and with that any reason to put both into a build is gone.

@tobim I have tried your approach in the past (for static-haskell-nix) and and found it forbiddingly much effort:

  • Some use cases need both static and dynamic libs. Static cally linking Haskell projects that also require TemplateHaskell (Provide middle-ground overlay between pkgsMusl and pkgsStatic #61575) is an example.
  • pkg-config support does not always work correctly with split outputs, making it much harder to build things.
  • Both autoconf and meson organise their build options such that you can turn on/off static/shared dependently of each other, thus mapping naturally to enableShared/enableStatic.

@tobim
Copy link
Contributor Author

tobim commented Dec 20, 2019

@tobim I have tried your approach in the past (for static-haskell-nix) and and found it forbiddingly much effort:

* Some use cases need both static and dynamic libs. Static cally linking Haskell projects that also require TemplateHaskell (#61575) is an example.

I admit I did not consider TemplateHaskell while formulating my argument.

* `pkg-config` support does not always work correctly with split outputs, making it much harder to build things.

I'm not arguing for zlib-style split outputs though. I think static libs should live in the pkgsStatic attribute set and not in the top level nixpkgs. Then there won't be a problem with pkg-config. There are of course some libraries that always need to be static such as some unit testing frameworks, but they are normally hardcoded to that in their build scripts so that should not be an issue.

* Both `autoconf` and `meson` organise their build options such that you can turn on/off static/shared dependently of each other, thus mapping naturally to `enableShared`/`enableStatic`.

From the point of view of a nixpkgs user these options are implementation details that should be confined to the package expression, but abstracted away in the interface.

veprbl
veprbl previously approved these changes Dec 21, 2019
View reviewed changes
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

Changes to the arrow-cpp LGTM

@ofborg ofborg bot requested a review from veprbl December 28, 2019 17:47
@veprbl

This comment has been minimized.

Sign in to view
@veprbl veprbl dismissed nh2’s stale review December 29, 2019 22:16

This was addressed

@tobim
Copy link
Contributor Author

tobim commented Dec 29, 2019

I applied that without changes. But lets merge #76659 first.

@tobim
Copy link
Contributor Author

tobim commented Dec 29, 2019

Regarding upstreaming the patch to woff2: I don't have high hopes that it would be successful, look at google/woff2#112, which tries to address the same issue (but did not work for me).

I opened https://gitlab.kitware.com/cmake/cmake/issues/20136 instead, that might at least get us a cleaner way for interacting with pkgconfig in a static context.

@ofborg ofborg bot requested a review from veprbl December 29, 2019 23:11
@veprbl
Copy link
Member

veprbl commented Dec 30, 2019

@GrahamcOfBorg build thrift
@GrahamcOfBorg build pkgsStatic.thrift

@ofborg ofborg bot requested a review from veprbl December 31, 2019 14:24
@veprbl
Copy link
Member

veprbl commented Dec 31, 2019

@GrahamcOfBorg build thrift
@GrahamcOfBorg build pkgsStatic.thrift

@Ericson2314
Copy link
Member

@GrahamcOfBorg build pkgsCross.mingwW64.zstd

@veprbl
Copy link
Member

veprbl commented Jan 2, 2020

gflags change also comes in #76833
pkgsStatic.openblas could be fixed by #76832

@veprbl veprbl mentioned this pull request Jan 2, 2020
Merged
10 tasks
veprbl
veprbl reviewed Jan 3, 2020
View reviewed changes
pkgs/development/web/woff2/brotli-static.patch
+if(NOT BUILD_SHARED_LIBS)
+ set(_S "STATIC_")
+endif()
+set(BROTLIDEC_LIBRARIES ${PC_BROTLIDEC_${_S}LIBRARIES})
Copy link
Member

Choose a reason for hiding this comment

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

It seems like the only reason why we need this is because we are applying google/brotli#655 which is not upstream yet. The problem is that other than fixing BUILD_SHARED_LIBS it also sneakily does this:

-target_link_libraries(brotlidec-static brotlicommon-static)
-target_link_libraries(brotlienc-static brotlicommon-static)
-

I think the 7ebd599 needs to be reworked to not use unsupported patch. Instead, we could remove the shared libraries and symlink the static ones in their place (also suggested in google/brotli#655 (comment)).
cc @matthewbauer @domenkozar

@FRidh FRidh merged commit 960c24a into NixOS:staging Jan 3, 2020
Staging automation moved this from Needs review to Done Jan 3, 2020
@FRidh
Copy link
Member

FRidh commented Jan 3, 2020

This looks good to me. In case of regressions we can always revert commits.

This was referenced Jan 3, 2020
Closed
Merged
@nh2
Copy link
Contributor

nh2 commented Jan 10, 2020

Nice job! 👍

@veprbl
Copy link
Member

veprbl commented Jan 10, 2020

In case anyone is looking, this broke glog on darwin. The fix is to be provided by #77451

@veprbl
Copy link
Member

veprbl commented Jan 10, 2020

Fix for the thrift build: #77467

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer left review comments

@veprbl veprbl veprbl left review comments

@nh2 nh2 nh2 left review comments

@hrdinka hrdinka Awaiting requested review from hrdinka

@bjornfor bjornfor Awaiting requested review from bjornfor

@ttuegel ttuegel Awaiting requested review from ttuegel

Assignees
No one assigned
Labels
6.topic: static 10.rebuild-darwin: 501+ 10.rebuild-darwin: 2501-5000 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
Staging
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tobim @veprbl @nh2 @Ericson2314 @FRidh @matthewbauer