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

Binaries are not stripped #21667

Open
peterhoeg opened this issue Jan 5, 2017 · 17 comments
Open

Binaries are not stripped #21667

peterhoeg opened this issue Jan 5, 2017 · 17 comments
Assignees
Labels
1.severity: mass-darwin-rebuild This PR causes a large number of packages to rebuild on Darwin 1.severity: mass-rebuild This PR causes a large number of packages to rebuild 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: closure size The final size of a derivation, including its dependencies

Comments

@peterhoeg
Copy link
Member

Issue description

I don't know if I'm missing something obvious, but the binaries are not stripped and according to the manual they should be:

dontStrip

    If set, libraries and executables are not stripped. By default, they are.

hello doesn't set dontStrip but still the binary is not stripped:

/home/peter/.nix-profile/bin/hello: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/8lbpq1vmajrbnc96xhv84r87fa4wvfds-glibc-2.24/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, not stripped

I checked on nixos-unstable and nixos-16.09.

Steps to reproduce

nix-env -i hello
file -L $(which hello)

Technical details

  • NixOS: 17.03.git.6375328bc0M (Gorilla)
  • Nix version: nix-env (Nix) 1.11.4
  • nixpkgs: 17.03.git.6375328bc0M
@peterhoeg peterhoeg changed the title Binaries are stripped Binaries are not stripped Jan 5, 2017
@vcunat
Copy link
Member

vcunat commented Jan 6, 2017

I believe this is only a problem/confusion in the output from file. Stripping debug symbols, i.e. running strip -S, does not make the "not stripped" text go away.

@peterhoeg
Copy link
Member Author

@vcunat, no, those binaries are not stripped. Using hello as the example again.

#!/usr/bin/env nix-shell
#!nix-shell -i bash -p binutils file hello

cp -L $(which hello) .
chmod 755 hello
du -h hello
file hello
strip hello
du -h hello
file hello

And this is the output:

32K     hello
hello: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/8lbpq1vmajrbnc96xhv84r87fa4wvfds-glibc-2.24/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, not stripped
24K     hello
hello: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /nix/store/8lbpq1vmajrbnc96xhv84r87fa4wvfds-glibc-2.24/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, stripped

Seems to be the case with all the binaries - not just hello.

I just looked at syncthing as an example and manually stripping it makes it drop from 89MB to 80MB. Looks like there are some closure size savings here!

@vcunat
Copy link
Member

vcunat commented Jan 7, 2017

Possibly. So far we've only been stripping debugging symbols (-S), not other things, at least by default. Perhaps --strip-unneeded will be better. I don't know enough about such stuff...

For standalone executables a full strip might work, but libraries would get unusable by that AFAIK. See https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/setup-hooks/strip.sh

@peterhoeg
Copy link
Member Author

I should have looked at the source first....

Maybe @peti remembers why the "strip all" commit was reverted?

Offhand it seems like running --strip-all on bin and sbin should be safe, but I'll try it out and report back.

@peti
Copy link
Member

peti commented Jan 7, 2017

The history of the previous patch is in #15087.

@Mic92
Copy link
Member

Mic92 commented Jan 7, 2017

makepkg from archlinux has a robust implementation to strip binaries:

https://git.archlinux.org/pacman.git/tree/scripts/libmakepkg/tidy/strip.sh.in

It also take care of libraries and static binaries.

@peterhoeg
Copy link
Member Author

I have just tried doing a rebuild of xfce with the following patch applied and it launches nicely in a VM:

diff --git a/pkgs/build-support/setup-hooks/strip.sh b/pkgs/build-support/setup-hooks/strip.sh
index 6860c9b9cb..7e0098a36c 100644
--- a/pkgs/build-support/setup-hooks/strip.sh
+++ b/pkgs/build-support/setup-hooks/strip.sh
@@ -4,14 +4,14 @@ fixupOutputHooks+=(_doStrip)

 _doStrip() {
     if [ -z "$dontStrip" ]; then
-        stripDebugList=${stripDebugList:-lib lib32 lib64 libexec bin sbin}
+        stripDebugList=${stripDebugList:-lib lib32 lib64}
         if [ -n "$stripDebugList" ]; then
-            stripDirs "$stripDebugList" "${stripDebugFlags:--S}"
+            stripDirs "$stripDebugList" "${stripDebugFlags:---strip-debug --strip-unneeded}"
         fi

-        stripAllList=${stripAllList:-}
+        stripAllList=${stripAllList:-libexec bin sbin}
         if [ -n "$stripAllList" ]; then
-            stripDirs "$stripAllList" "${stripAllFlags:--s}"
+            stripDirs "$stripAllList" "${stripAllFlags:---strip-all}"
         fi
     fi
 }

@Mic92
Copy link
Member

Mic92 commented Jan 7, 2017

I would use file to inspect the elf type instead on relying on the path to check if a package is an executable or a shared library.

@peterhoeg
Copy link
Member Author

You could also argue that executables belong in bin and libraries in lib and if that's not the case, that's a packaging bug.

Unless I rebuild the entire repo I obviously cannot be sure, but isn't getting xfce running enough to show that it works? And we save quite some space (10% for the binaries in my limited testing).

@vcunat
Copy link
Member

vcunat commented Jan 8, 2017

There's a PR for improving how we apply strip. #15339.

@vcunat
Copy link
Member

vcunat commented Jan 8, 2017

--strip-unneeded does sound good to me. It would be nice to find some reference, e.g. how other distros do that (and why) or more detailed implications of the options, but even so... I suppose we could stage such a change and fixup individual problematic packages later (if any).

@vcunat vcunat added 1.severity: mass-darwin-rebuild This PR causes a large number of packages to rebuild on Darwin 1.severity: mass-rebuild This PR causes a large number of packages to rebuild 6.topic: closure size The final size of a derivation, including its dependencies labels Jan 8, 2017
@peterhoeg
Copy link
Member Author

@layus has clearly put in a lot more effort than me but what's your preferred approach @vcunat?

This is truly a minor thing, but in any kind of shell scripting I generally use the long form of parameters passed to programs - it's far easier to remember what's going on when you're looking at it 6 months later.

@layus
Copy link
Member

layus commented Jan 8, 2017

This issue and #15339 are somewhat orthogonal. #15339 ensures that we strip every file without restriction. This issue proposes to change the way we strip the current files. They are both useful, and should even be merged together to avoid two mass rebuilds.

Yours is a concern of package size, and mine of closure size. Nix uses stripping to remove references to build inputs. From that point of view, stripping with -S is enough. AFAIK, that's the historical reason why binaries are stripped. The idea was to remove these unwanted references. Size was not really a concern. My observation on the other hand is that many "strippable" objects reside outside of the given paths "lib lib32 lib64 libexec bin sbin". I just want to strip everything by default, and let packagers explicitly exclude files or directories if need be.

As long as you do not --strip-all the shared objects, all should be fine. That being said, you will find most of the annoying packages on a server, not on a desktop.

@vcunat
Copy link
Member

vcunat commented Jan 8, 2017

My preferred approach? I liked that strip-everything PR, I think, but I always feel compelled to review things thoroughly and unfortunately I like many more things than I'm able to catch (timely and at all). EDIT: and yes, I also thought these to be orthogonal :-) (I originally thought this is thread of that old PR.)

@cartazio
Copy link

What’s the current state of play of this and related issues ?

@stale
Copy link

stale bot commented Jun 3, 2020

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse.
  3. Ask on the #nixos channel on irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 3, 2020
@peterhoeg peterhoeg self-assigned this Jun 3, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 3, 2020
@stale
Copy link

stale bot commented Nov 30, 2020

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.severity: mass-darwin-rebuild This PR causes a large number of packages to rebuild on Darwin 1.severity: mass-rebuild This PR causes a large number of packages to rebuild 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: closure size The final size of a derivation, including its dependencies
Projects
None yet
Development

No branches or pull requests

6 participants