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

SDL2: Keep .a files on dontDisableStatic; don't move them to $dev; prune .la #72736

Merged
merged 1 commit into from Nov 10, 2019
Merged

SDL2: Keep .a files on dontDisableStatic; don't move them to $dev; prune .la #72736

merged 1 commit into from Nov 10, 2019

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented Nov 3, 2019

Motivation for this change

Most other packages don't move .a files to "$dev", and that is because
it makes the pkg-config .pc file wrong (the libdir is the non-dev one).

Keeping them in the main output makes static linking of SDL2 work.

See added comment about pruning of .la files.

Tested in nh2/static-haskell-nix#65.

Things done
  • 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 @cpages

FYI @matthewbauer

@nh2 nh2 requested review from viric and cpages November 3, 2019 19:06
@nh2
Copy link
Contributor Author

nh2 commented Nov 3, 2019

Relevant history of the SDL2 package and the moveToOutput in postInstall:

commit b81caba5fbb49012a5cabefb672f3b83bd524d1e
Author: Carles Pagès <page@cubata.homelinux.net>
Date:   Wed Nov 6 23:17:46 2013 +0100

   SDL2: some improvements to the expression.

commit da70d3da0f11b22eac77756b39b349215e06b2e3
Author: Linus Heckemann <git@sphalerite.org>
Date:   Sun Dec 25 01:11:07 2016 +0000

   SDL2: split derivation

commit 0c42efd9d70774eafb47212967caada630335731
Author: Lluís Batlle i Rossell <viric@viric.name>
Date:   Thu Feb 16 22:24:40 2017 +0100

   SDL2: fix creation of libSDL2main.a
   
   It's required by a trigger rally update I will commit next.
   And other games use that too.

commit 1e7da9ec78d11f586fc74747d77b17f91969a7ed
Author: Lluís Batlle i Rossell <viric@viric.name>
Date:   Thu Feb 16 22:25:23 2017 +0100

   trigger: update to 0.6.5

What the libtool problem looks like when static libs are enabled (this PR fixes that):

libtool: link: gcc -shared  .libs/SDL_ttf.o   -Wl,-rpath -Wl,/nix/store/ggafj30pr2sv0hj7jgfywvqv1yz88mws-freetype-2.10.1/lib -Wl,-rpath -Wl,/nix/store/mdxphvay0gaxrrm7hpanfc76j198bbvj-bzip2-1.0.6.0.1/lib -Wl,-rpath -Wl,/nix/store/q33gb957w94mqdqkq4pwil8hix34krwv-libpng-apng-1.6.37/lib -Wl,-rpath -Wl,/nix/store/x3szqg2c1rav8ja944583r1a1m6vmq86-SDL2-2.0.10/lib -Wl,-rpath -Wl,/nix/store/ggafj30pr2sv0hj7jgfywvqv1yz88mws-freetype-2.10.1/lib -Wl,-rpath -Wl,/nix/store/mdxphvay0gaxrrm7hpanfc76j198bbvj-bzip2-1.0.6.0.1/lib -Wl,-rpath -Wl,/nix/store/q33gb957w94mqdqkq4pwil8hix34krwv-libpng-apng-1.6.37/lib -Wl,-rpath -Wl,/nix/store/x3szqg2c1rav8ja944583r1a1m6vmq86-SDL2-2.0.10/lib -L/nix/store/ggafj30pr2sv0hj7jgfywvqv1yz88mws-freetype-2.10.1/lib /nix/store/ggafj30pr2sv0hj7jgfywvqv1yz88mws-freetype-2.10.1/lib/libfreetype.so -L/nix/store/2fd7k3j5vmmkzw14ywwjykw8sa4h0vdd-zlib-1.2.11/lib -L/nix/store/mdxphvay0gaxrrm7hpanfc76j198bbvj-bzip2-1.0.6.0.1/lib -L/nix/store/q33gb957w94mqdqkq4pwil8hix34krwv-libpng-apng-1.6.37/lib /nix/store/mdxphvay0gaxrrm7hpanfc76j198bbvj-bzip2-1.0.6.0.1/lib/libbz2.so /nix/store/q33gb957w94mqdqkq4pwil8hix34krwv-libpng-apng-1.6.37/lib/libpng16.so -lz -L/nix/store/x3szqg2c1rav8ja944583r1a1m6vmq86-SDL2-2.0.10/lib /nix/store/x3szqg2c1rav8ja944583r1a1m6vmq86-SDL2-2.0.10/lib/libSDL2.so -lm -lX11 -lXext -lXcursor -lXinerama -lXi -lXrandr -lXss -lXxf86vm -lpthread -lrt  -Wl,-rpath -Wl,/nix/store/x3szqg2c1rav8ja944583r1a1m6vmq86-SDL2-2.0.10/lib -Wl,--enable-new-dtags   -Wl,-soname -Wl,libSDL2_ttf-2.0.so.0 -o .libs/libSDL2_ttf-2.0.so.0.14.1
checking if g++ static flag -static works... yes
/nix/store/f1v9hcm7pxdrgmmarf5ldss9xc480l0n-binutils-2.31.1/bin/ld: cannot find -lXext
/nix/store/f1v9hcm7pxdrgmmarf5ldss9xc480l0n-binutils-2.31.1/bin/ld: cannot find -lXcursor
/nix/store/f1v9hcm7pxdrgmmarf5ldss9xc480l0n-binutils-2.31.1/bin/ld: cannot find -lXinerama
/nix/store/f1v9hcm7pxdrgmmarf5ldss9xc480l0n-binutils-2.31.1/bin/ld: cannot find -lXi
/nix/store/f1v9hcm7pxdrgmmarf5ldss9xc480l0n-binutils-2.31.1/bin/ld: cannot find -lXrandr
/nix/store/f1v9hcm7pxdrgmmarf5ldss9xc480l0n-binutils-2.31.1/bin/ld: cannot find -lXss
/nix/store/f1v9hcm7pxdrgmmarf5ldss9xc480l0n-binutils-2.31.1/bin/ld: cannot find -lXxf86vm
collect2: error: ld returned 1 exit status
make: *** [Makefile:517: libSDL2_ttf.la] Error 1

builder for '/nix/store/x1gslck2ab1r09by2jgpm6qzcn24d90a-SDL2_ttf-2.0.15.drv' failed with exit code 2

@nh2 nh2 mentioned this pull request Nov 3, 2019
Merged
@nh2
Copy link
Contributor Author

nh2 commented Nov 3, 2019

10.rebuild-linux: 501-1000 -- I'll switch it to staging instead.

@nh2 nh2 changed the base branch from master to staging November 3, 2019 20:44
@nh2 nh2 mentioned this pull request Nov 3, 2019
Open
60 tasks
matthewbauer
matthewbauer reviewed Nov 4, 2019
View reviewed changes
pkgs/development/libraries/SDL2/default.nix
postInstall = ''
moveToOutput lib/libSDL2main.a "$dev"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this file (and other .a) still be removed when dontDisableStatic is true? Otherwise we get .a files in the SDL closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right.

It seems SDL2 does not quite respect --disable-static; it still generates libSDL2main.a and libSDL2_test.a.

I've force-pushed an else branch that deletes them.

It also generates libSDL2main.la and libSDL2_test.la which are prett useless given that the .a files they mention are nonexistent. Should I delete them, or do we not care?

Also, with what comand can I rebuild everything that depends on SDL2, so that I can check if anything used the libSDL2main.a that before was in the $dev output?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the 'else' branch to keep removing the .a libs. Your force push failed? Otherwise looks good to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your force push failed?

Oops, you are right. Fixed now, sorry.

@nh2
Copy link
Contributor Author

nh2 commented Nov 4, 2019

@GrahamcOfBorg eval

@FRidh FRidh added this to WIP in Staging via automation Nov 5, 2019
@FRidh FRidh moved this from WIP to Needs review in Staging Nov 5, 2019
@nh2
Copy link
Contributor Author

nh2 commented Nov 7, 2019

@matthewbauer Good to go with the incorporated feedback?

@nh2
SDL2: Keep .a files on dontDisableStatic; don't move them to $dev; …
57908c1 
…prune .la.

Most other packages don't move `.a` files to "$dev", and that is because
it makes the pkg-config `.pc` file wrong (the `libdir` is the non-dev one).

Keeping them in the main output makes static linking of SDL2 work.

See added comment about pruning of `.la` files.
@nh2
Copy link
Contributor Author

nh2 commented Nov 9, 2019

The ofborg build times out on building GCC 8 after 1 hour.

@cpages cpages merged commit 539e940 into NixOS:staging Nov 10, 2019
Staging automation moved this from Needs review to Done Nov 10, 2019
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

@viric viric Awaiting requested review from viric

@cpages cpages Awaiting requested review from cpages

Assignees
No one assigned
Labels
10.rebuild-darwin: 101-500 10.rebuild-linux: 501-1000 10.rebuild-linux: 501+
Projects
Staging
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nh2 @matthewbauer @cpages