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

zlib: Properly clean up static/shared distinction #66490

Merged
merged 1 commit into from Aug 17, 2019
Merged

zlib: Properly clean up static/shared distinction #66490

merged 1 commit into from Aug 17, 2019

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented Aug 11, 2019

Includes #66486 (a simple comments change) which is for master, while this is for staging.

Motivation for this change

The previous logic was "grown" and has become pretty crazy to understand.

This improves what commit e999def (zlib: clean up static/shared distincion) described as "kind of a mess" and "confusing". And indeed it was confusing.

Now, the concept whether or not the .a file is moved to a split output is controlled by a clean variable.

The defaults remain unchanged.

The new approach also finally cleanly allows building statically but NOT using a split output, like all other autoconf-based projects in nixpkgs do (using the dontDisableStatic setting).
That is important for overlays that want to enable static libs for all packages in one go, without having to hand-patch idiosynchrasies like zlib had until now.

Until now, if you wanted the .a in the main output, the only way was to go via static=false, shared=true -- which made no sense, because you had to say static=false even though you want a static lib. That is fixed now.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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.
  • Tested all boolean combinations of shared, static and splitStaticOutput, checking that the files in the outputs are as expected.
Notify maintainers

cc @matthewbauer

@nh2 nh2 requested a review from matthewbauer August 11, 2019 20:20
@nh2
Copy link
Contributor Author

nh2 commented Aug 11, 2019

@GrahamcOfBorg build zlib

@nh2 nh2 force-pushed the zlib-static-shared-split-cleanup branch from b6e2ad4 to bd66819 Compare August 13, 2019 00:18
nh2 added a commit to nh2/static-haskell-nix that referenced this pull request Aug 13, 2019
@nh2
nixpkgs: Include zlib cleanup from NixOS/nixpkgs#66490
642a257
@matthewbauer
Copy link
Member

The comments look good, but I'm a little bit surprised this needs to be a mass rebuild. I didn't think dontDisableStatic would be needed at all, --disable-static is not being passed to zlib.

splitStaticOutput definitely makes things clearer, although for the same reasons you describe in .pc, I was hoping we could eventually phase those out altogether. There are a few things that rely on this output existing though. pkg-config assumes that all of your libraries are in the same directory.

Changing the order seems reasonable though, at least to make things more clear. Not sure why it has that behavior, but my understanding was that zlib just always assumed you wanted static libraries.

@nh2
Copy link
Contributor Author

nh2 commented Aug 13, 2019

I didn't think dontDisableStatic would be needed at all, --disable-static is not being passed to zlib.

@matthewbauer Can you elaborate a bit on that? I thought that's passed in general, is there an exception for zlib somehwere that I could refer to?

for the same reasons you describe in .pc, I was hoping we could eventually phase those out altogether. There are a few things that rely on this output existing though

I wasn't sure if there's anyone who wants the zlib.static output to exist. Do you know if such someone exists?

I didn't find many uses of it; what's the best way to find them all?

Changing the order seems reasonable though, at least to make things more clear. Not sure why it has that behavior

As far as I can tell, zlib has a hand-writen ./configure script, not autoconf, and I suspect that the logic is that by default it builds both, that they added a --static option to build only static, and that later somebody added a --shared flag so that you can "turn it back on", and that the order is simply because their command line argument parsing code "grew that way". But I haven't checked.

@nh2 nh2 force-pushed the zlib-static-shared-split-cleanup branch from bd66819 to 5647474 Compare August 13, 2019 18:42
@nh2
Copy link
Contributor Author

nh2 commented Aug 13, 2019

I thought that's passed in general, is there an exception for zlib somehwere that I could refer to?

Ah I see:

nixpkgs/pkgs/stdenv/generic/setup.sh

Lines 993 to 998 in 6490f9c

# By default, disable static builds.
if [ -z "${dontDisableStatic:-}" ]; then
if [ -f "$configureScript" ] && grep -q enable-static "$configureScript"; then
configureFlags="--disable-static $configureFlags"
fi
fi

This greps for enable-static in the ./configure file, and zlib's handwritten one doesn't have that.

I'm a little bit surprised this needs to be a mass rebuild

I've pushed a fix that should avoid mass rebuilds, with extra commentary for rationale.

@nh2
Copy link
Contributor Author

nh2 commented Aug 13, 2019

@GrahamcOfBorg eval

@GrahamcOfBorg build zlib

nh2 added a commit to nh2/static-haskell-nix that referenced this pull request Aug 13, 2019
@nh2
nixpkgs: Include zlib cleanup from NixOS/nixpkgs#66490
f984dfa
This was referenced Aug 14, 2019
Open
Closed
@nh2
zlib: Properly clean up static/shared distinction
3d87db9 
This improves what commit

    e999def zlib: clean up static/shared distincion

described as "kind of a mess" and "confusing". And indeed it was confusing.

Now, the concept whether or not the .a file is moved to a split output
is controlled by a clean variable.

The defaults remain unchanged.

The new approach also finally cleanly allows building statically but NOT
using a split output, like all other autoconf-based projects in nixpkgs do
(using the `dontDisableStatic` setting).
That is important for overlays that want to enable static libs for all
packages in one go, without having to hand-patch idiosynchrasies like zlib
had until now.

Until now, if you wanted the .a in the main output, the only way was to go via
`static=false, shared=true` -- which made no sense, because you had to say
`static=false` even though you want a static lib. That is fixed now.
@nh2 nh2 force-pushed the zlib-static-shared-split-cleanup branch from 5647474 to 3d87db9 Compare August 14, 2019 22:06
@nh2 nh2 requested a review from Ericson2314 as a code owner August 14, 2019 22:06
@Ericson2314
Copy link
Member

madler/zlib#394 may it someday be better upstream.

nh2 added a commit to nh2/static-haskell-nix that referenced this pull request Aug 14, 2019
@nh2
nixpkgs: Include zlib cleanup from NixOS/nixpkgs#66490
4f57531
This was referenced Aug 14, 2019
Closed
Open
Open
@nh2
Copy link
Contributor Author

nh2 commented Aug 15, 2019

splitStaticOutput definitely makes things clearer, although for the same reasons you describe in .pc, I was hoping we could eventually phase those out altogether. There are a few things that rely on this output existing though. pkg-config assumes that all of your libraries are in the same directory.

I have filed a separate issue for this task at #66658.

nh2 added a commit to nh2/static-haskell-nix that referenced this pull request Aug 15, 2019
@nh2
survey: Remove curl -lz, no longer necessary.
2c7967c 
Our submodule now includes the fix from
NixOS/nixpkgs#66490
which makes this unnecessary.
nh2 added a commit to nh2/static-haskell-nix that referenced this pull request Aug 16, 2019
@nh2
nixpkgs: Include zlib cleanup from NixOS/nixpkgs#66490
c8efdf3
nh2 added a commit to nh2/static-haskell-nix that referenced this pull request Aug 16, 2019
@nh2
survey: Remove curl -lz, no longer necessary.
5baf249 
Our submodule now includes the fix from
NixOS/nixpkgs#66490
which makes this unnecessary.
nh2 added a commit to nh2/static-haskell-nix that referenced this pull request Aug 16, 2019
@nh2
nixpkgs: Include zlib cleanup from NixOS/nixpkgs#66490
19186c1
nh2 added a commit to nh2/static-haskell-nix that referenced this pull request Aug 16, 2019
@nh2
survey: Remove curl -lz, no longer necessary.
cffe6bf 
Our submodule now includes the fix from
NixOS/nixpkgs#66490
which makes this unnecessary.
nh2 added a commit to nh2/static-haskell-nix that referenced this pull request Aug 16, 2019
@nh2
nixpkgs: Update submodule, add changes for zlib cleanup from NixOS/ni…
7296d34 
…xpkgs#66490
nh2 added a commit to nh2/static-haskell-nix that referenced this pull request Aug 16, 2019
@nh2
survey: Remove curl -lz, no longer necessary.
f9bdfa0 
Our submodule now includes the fix from
NixOS/nixpkgs#66490
which makes this unnecessary.
@nh2 nh2 merged commit aa99a26 into NixOS:master Aug 17, 2019
nh2 added a commit to nh2/static-haskell-nix that referenced this pull request Aug 17, 2019
@nh2
nixpkgs: Update submodule, add changes for zlib cleanup from NixOS/ni…
7a2d291 
…xpkgs#66490
nh2 added a commit to nh2/static-haskell-nix that referenced this pull request Aug 17, 2019
@nh2
survey: Remove curl -lz, no longer necessary.
2ed2d3a 
Our submodule now includes the fix from
NixOS/nixpkgs#66490
which makes this unnecessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrew-d andrew-d andrew-d left review comments

@Ericson2314 Ericson2314 Ericson2314 approved these changes

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@FRidh FRidh Awaiting requested review from FRidh

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nh2 @matthewbauer @Ericson2314 @andrew-d