Skip to content

SuperTuxKart: Replaced some bundled libraries with system ones #91421

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.

Sign up for GitHub

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
Merged
teto merged 2 commits into from
Jun 27, 2020
Merged

SuperTuxKart: Replaced some bundled libraries with system ones #91421

teto merged 2 commits into from
Jun 27, 2020

Conversation

@SCOTT-HAMILTON
Copy link
Contributor

@SCOTT-HAMILTON SCOTT-HAMILTON commented Jun 24, 2020

Motivation for this change

Was packaging WiiUse when noticed
that SuperTuxKart was conflicting with it. So I came with
this partially unbundled build. This should also reduce the package size a bit.

Things done

Replaced some bundled libs with system ones.

  • 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 nixpkgs-review --run "nixpkgs-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.

@SCOTT-HAMILTON
Copy link
Contributor Author

SCOTT-HAMILTON commented Jun 24, 2020

Needs #91425, waiting for this PR to be merged.

teto
teto reviewed Jun 27, 2020
View reviewed changes
Copy link
Member

@teto teto left a comment

Choose a reason for hiding this comment

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

cool job using system libraries instead. Could you check the reduction in closure size.
Also I've merged your other PR in case you wanna rebase.

pkgs/games/super-tux-kart/CMakeLists-system-libs.patch Outdated
Copy link
Member

@teto teto Jun 27, 2020

Choose a reason for hiding this comment

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

why removing the check about USE_IPV6 ?

Copy link
Contributor Author

@SCOTT-HAMILTON SCOTT-HAMILTON Jun 27, 2020

Choose a reason for hiding this comment

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

libenet doesn't yet support ipv6 upstream lsalzman/enet#109.
So I either disable ipv6 or I use the bundled libenet (which has been fixed to support ipv6).

Copy link
Member

@teto teto Jun 27, 2020

Choose a reason for hiding this comment

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

ok but instead of patching, you could probably pass USE_IPV6=no https://notabug.org/Supertuxkart/stk-code/src/master/CMakeLists.txt#L32 ? also maybe we could keep using the bundled enet until the next release with ipv6 support otherwise the update removes feature.

Copy link
Contributor Author

@SCOTT-HAMILTON SCOTT-HAMILTON Jun 27, 2020

Choose a reason for hiding this comment

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

In terms of closuze size it's getting worse :
890.0M before against 898.2M now.

I don't really explain this. It doesn't seem to be static linking since all
libs are dynamically linked.

Copy link
Member

@teto teto Jun 27, 2020

Choose a reason for hiding this comment

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

maybe the system packages have a bigger closure because they are built with more options/stuff ?

Copy link
Member

@teto teto Jun 27, 2020

Choose a reason for hiding this comment

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

the difference is little and if one of the lib is reused, it becomes a gain (again :) ) so not an issue for me.

Copy link
Contributor Author

@SCOTT-HAMILTON SCOTT-HAMILTON Jun 27, 2020

Choose a reason for hiding this comment

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

Since the bundled libs are statically linked, this frees 2M on the final executable :
17M→15M

@SCOTT-HAMILTON
Copy link
Contributor Author

SCOTT-HAMILTON commented Jun 27, 2020

New rebasing : added WiiUse, removed CMakeLists.txt patch and added libenet to whitelist of bundled libs to keep.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jun 27, 2020
@ofborg ofborg bot requested review from PyroLagus and peterhoeg June 27, 2020 14:26
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jun 27, 2020
teto
teto reviewed Jun 27, 2020
View reviewed changes
pkgs/games/super-tux-kart/default.nix Outdated
Copy link
Member

@teto teto Jun 27, 2020

Choose a reason for hiding this comment

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

if we whitelist enet, then we don't use the system one

Copy link
Contributor Author

@SCOTT-HAMILTON SCOTT-HAMILTON Jun 27, 2020

Choose a reason for hiding this comment

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

Fixed

@teto
Copy link
Member

teto commented Jun 27, 2020

ran nixpkgs-review and... endud up to complete 2 laps with tux. thanks

@teto teto merged commit 91c9127 into NixOS:master Jun 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@teto teto teto left review comments

@peterhoeg peterhoeg Awaiting requested review from peterhoeg

@PyroLagus PyroLagus Awaiting requested review from PyroLagus

Assignees

No one assigned

Labels

8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SCOTT-HAMILTON @teto