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

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

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 Dec 16, 2019
Copy link
Contributor

@nh2 nh2 left a comment

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 veprbl commented Dec 17, 2019

One more thing: This PR should be targeting staging.

@FRidh FRidh changed the base branch from master to staging Dec 19, 2019
@FRidh FRidh added this to WIP in Staging via automation Dec 19, 2019
@ofborg ofborg bot requested a review from veprbl Dec 19, 2019
@tobim tobim force-pushed the tobim:pkgsStatic/arrow branch from 0c315c5 to ffb4791 Dec 19, 2019
@ofborg ofborg bot requested a review from ttuegel Dec 19, 2019
Copy link
Contributor Author

@tobim tobim left a comment

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 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 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 (#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 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.

@tobim tobim force-pushed the tobim:pkgsStatic/arrow branch from ffb4791 to e2c1471 Dec 20, 2019
Copy link
Member

@veprbl veprbl left a comment

Changes to the arrow-cpp LGTM

@tobim tobim force-pushed the tobim:pkgsStatic/arrow branch from e2c1471 to a939f01 Dec 28, 2019
@ofborg ofborg bot requested a review from veprbl Dec 28, 2019
@veprbl veprbl dismissed nh2’s stale review Dec 29, 2019

This was addressed

@tobim tobim force-pushed the tobim:pkgsStatic/arrow branch from 2820dce to 0d39262 Dec 29, 2019
@tobim
Copy link
Contributor Author

@tobim tobim commented Dec 29, 2019

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

@tobim
Copy link
Contributor Author

@tobim 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 Dec 29, 2019
@veprbl
Copy link
Member

@veprbl veprbl commented Dec 30, 2019

@GrahamcOfBorg build thrift
@GrahamcOfBorg build pkgsStatic.thrift

@tobim tobim force-pushed the tobim:pkgsStatic/arrow branch from 0d39262 to db5d3cd Dec 31, 2019
@ofborg ofborg bot requested a review from veprbl Dec 31, 2019
@veprbl
Copy link
Member

@veprbl veprbl commented Dec 31, 2019

@GrahamcOfBorg build thrift
@GrahamcOfBorg build pkgsStatic.thrift

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Dec 31, 2019

@GrahamcOfBorg build pkgsCross.mingwW64.zstd

@veprbl
Copy link
Member

@veprbl 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
3 of 10 tasks complete
+if(NOT BUILD_SHARED_LIBS)
+ set(_S "STATIC_")
+endif()
+set(BROTLIDEC_LIBRARIES ${PC_BROTLIDEC_${_S}LIBRARIES})

This comment has been minimized.

@veprbl

veprbl Jan 3, 2020
Member

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
24 checks passed
24 checks passed
arrow-cpp, double-conversion, gflags, glog, gtest, openblas, snappy, thrift, woff2, zstd on aarch64-linux Failure
Details
arrow-cpp, double-conversion, gflags, glog, gtest, openblas, snappy, thrift, woff2, zstd on x86_64-linux Failure
Details
pkgsCross.mingwW64.zstd on x86_64-darwin Timed out, unknown build status
Details
pkgsStatic.thrift on x86_64-darwin Timed out, unknown build status
Details
thrift on x86_64-darwin Timed out, unknown build status
Details
Evaluation Performance Report Evaluator Performance Report
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
pkgsCross.mingwW64.zstd on aarch64-linux Success
Details
pkgsCross.mingwW64.zstd on x86_64-linux Success
Details
pkgsStatic.thrift on aarch64-linux Success
Details
pkgsStatic.thrift on x86_64-linux Success
Details
thrift on aarch64-linux Success
Details
thrift on x86_64-linux Success
Details
Staging automation moved this from Needs review to Done Jan 3, 2020
@FRidh
Copy link
Member

@FRidh FRidh commented Jan 3, 2020

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

@nh2
Copy link
Contributor

@nh2 nh2 commented Jan 10, 2020

Nice job! 👍

@veprbl
Copy link
Member

@veprbl 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 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
Projects
Staging
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

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