Just strip everything by default, fully tested version :-) #15339

layus · 2016-05-09T22:59:47Z

This is the same PR as #15087, but fully tested and modified to take into account the discovered bugs.

Stdenv now supports the dontStripPath attribute which is the black-list version of the former stripDebugList and stripAllList (maintained for compatibility).

These three variables now support bash globbing, as explained in the updated manual.
stripDebugList = [ "bin" "lib/libpretty.so" "modules/*.so" ] are three valid expressions, expanded under $prefix/.

It appears that this PR can break stuff. Sometimes strip will fail with slightly different error messages than the expected ones. It seems to come from its structure, where the file type is recognised and handled by a custom handler, which in turn may fail with custom errors that we cannot (and dont want to) catch. For these cases, we will need to update the derivation wit a dontStripList argument. The only example of this I have encountered is swig. Some template text file presumably looks like one of the supported binary formats, but truncated. The error message is quite different from the basic truncation error.

A similar issue occurred with go which ships sample binaries for testing purposes. These are allegedly ELF's, but not to strip's liking.

Finally, I could not resist the pleasure of removing some superfluous stripDebugList's :-).

The build is successful for nix-build -A samba -A go -A flashplayer -A mesa_noglu -A apacheHttpd -A syslinux -A avrgcclibc, which is already a good panel of derivations. (Well, at least it takes a few hours on my quad-core, and at least one hour for gcc alone.) (BTW, the build is not successful for nss, but it's not my fault, it comes from d71a485#commitcomment-17408979, which in turn calls for a refactoring of my stripDirs function into a generic wrapper for find, but let's leave this for later.)

Closes #14821, #14822, #14823, #14824.

mention-bot · 2016-05-09T22:59:49Z

By analyzing the blame information on this pull request, we identified @edolstra, @peti and @ambrop72 to be potential reviewers

domenkozar · 2016-05-10T11:08:15Z

pkgs/development/misc/avr-gcc-with-avr-libc/default.nix

@@ -26,8 +26,8 @@ stdenv.mkDerivation {

  buildInputs = [ gmp mpfr libmpc zlib ];

-  # Make sure we don't strip the libraries in lib/gcc/avr.
-  stripDebugList= [ "bin" "avr/bin" "libexec" ];
+  # Make sure we don't strip the libraries in lib/gcc/avr and in avr/lib.


Could we have a comment that explains why they shouldn't be stripped? Currently it just says the same as the definition below does

Because if stripped linking with them tends to fail, presumably because there are some functionally relevant symbols there. Can't say more.

I guess @domenkozar's point was that the comment should say that. As it is, the comment just repeats what's written below it anyway.

My experience is that *.o files in avr/lib make strip fail because their format is not recognised.
Only avr-strip could possibly recognise these. I will update the patch to reflect this.
Later, it may be a good idea to strip these files with avr-strip.
@ambrop72, does it make sense to you ?

Anyway, I will update the comment.

@layus IIRC, I wrote that original dontStripList, not because it failed to strip, but because to broke the toolchain. Actually my experience is that "strip" doesn't particularly care about the CPU architecture, it can strip symbols from "other" architectures.

domenkozar · 2016-05-10T11:08:29Z

cc @edolstra

vcunat · 2016-05-20T08:37:47Z

I've got a problem in linux-config-4.4.10; I'll continue rebuilding my system to see if there are more. (I'm testing with #15405 included.)

vcunat · 2016-05-20T08:42:31Z

Ah, I think it has problems when an output is just a file directly, not a directory as usual. It tries to strip that non-ELF file and fails.

layus · 2016-05-20T08:59:48Z

Hmm, this is the intended behavior, except for the failing part obviously. What is the error produced by strip on that file? Also, are you sure it does not fail because of patchElf?

This is the main drawback of this patch. It tries to strip everything but may fail. It replaces false positives by false negatives...

vcunat · 2016-05-20T09:13:14Z

It might be something else. No error is printed.

vcunat · 2016-05-20T09:14:23Z

Example log:

post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/6c348l0k96kxybv91xd2j75vw81f0rnn-linux-config-4.4.10
Stripping (with flags -S) the file /nix/store/6c348l0k96kxybv91xd2j75vw81f0rnn-linux-config-4.4.10.
builder for ‘/nix/store/x0fb8bzc5905bnn27v6zxx9lmafm9gb9-linux-config-4.4.10.drv’ failed with exit code 1

layus · 2016-05-20T09:21:58Z

I have found that adding "set -x" to the preFixup hook of the derivation is very useful, but I guess you already know that.

But the ultimate trick is

{
   # ...
   preFixup = ''
      set -x
      export PS4='+(''${BASH_SOURCE}:''${LINENO}): ''${FUNCNAME[0]:+''${FUNCNAME[0]}(): }'
   '';
}

(source : http://stackoverflow.com/questions/17804007/how-to-show-line-number-when-executing-bash-script)

vcunat · 2016-05-20T10:07:06Z

Yes, I use set -x often, too. Thanks anyway. See #15405 (comment)

layus · 2016-05-20T12:12:00Z

Ok, I just pushed a fixup for the wrong return usage, and also added a null byte at the end of the file, because the input to stripAll should be null-terminated. Null-separated is not enough.

I wont be available this weekend, and the fix has not been tested. Feel free to use it or just wait till Monday for me to test it properly.

Thanks for looking into this anyway.

vcunat · 2016-05-20T14:25:23Z

This way of adding null byte seems not to work, judging from while read -r -d $'\0' f; do echo "$f"; done <<< "foo"$'\0'.

layus · 2016-05-26T07:34:02Z

I fixed the remaining issues:

does not fail when the prefix is a single file (i.e. use return 0 instead of bare return)
documented avr-gcc dontStripList.
really print a null-terminated string in bash (ouch!).

Could it be the last iteration? I hope so :-).

layus · 2016-06-08T15:02:33Z

@vcunat, could you give it another try ?

vcunat · 2016-06-09T10:43:39Z

pkgs/build-support/setup-hooks/strip.sh

-        if [ -n "$stripAllList" ]; then
-            stripDirs "$stripAllList" "${stripAllFlags:--s}"
+        if [ -n "${stripAllList:-}" ]; then
+            stripDirs "${stripAllList:-}" "${stripAllFlags:--s}"
        fi
    fi
 }


Nitpick: in _doStrip I would change all :- to -, so that people can explicitly define those variables as empty without being overridden back to the default values. (Yes, one could define them as " ", but it seems nicer to allow empty string as well.)

Note that otherwise e.g. the first if will always fire, as * is used whenever the expansion could be empty.

What does an empty string mean ? Strip everything or nothing ?
For now, passing an empty string to stripDirs strips nothing because it is interpreted as a gob pattern. This is why "" is replaced by "*".

If you want to disable stripping, why not use dontStrip ?

My fear is that if the variable is defined for some reason, or remains (say, from another output), the result may not be expected by the user.

I may however remove the useless if. Would that suit you ?

I removed the if in bda09f6.

layus · 2016-08-09T14:04:19Z

@vcunat I just rebased everything and fixed some merge conflict. I know this is getting tedious, but could you give it another try ?

vcunat · 2016-08-09T18:49:42Z

Yeah, I'm sorry for that. This summer I've got way too many things to do and slight health problems atop.

langston-barrett · 2016-10-03T21:34:39Z

@layus @vcunat How's this going?

wmertens · 2016-12-18T20:08:13Z

How about not stripping everything by default, and instead calculating dependencies on stripped binaries but keeping the originals?

So build time dependencies will not be part of the dependency graph that way, hopefully.

vcunat · 2016-12-18T21:30:52Z

That's something completely different than what's addressed by this PR, as I remember it.

I'm sorry, I've been swamped by other nixpkgs issues, neglecting especially more work-intensive tickets like this one.

layus · 2017-01-08T14:48:56Z

How about not stripping everything by default, and instead calculating dependencies on stripped binaries but keeping the originals?

So build time dependencies will not be part of the dependency graph that way, hopefully.

@wmertens Not a bad idea, but this is a drastic change in the assumptions on the nix store. Dependencies are defined as the references to the nix-store contained in the package. Changing that is way beyond the scope of this PR.

I'm sorry, I've been swamped by other nixpkgs issues, neglecting especially more work-intensive tickets like this one.

@vcunat This PR is cpu-intensive, not really work-intensive :-). Would you mind staging it again ? or dedicating a separate hydra branch to this one. I have tested as much as I could with my limited tools. I am still trying to install hydra on my server. Its tougher than one would expect :-).

The issue is still present. On my current system, I get

$ blame () {comm -12 <(nix-store -qR /run/current-system | sort) <(nix-store -q --referrers "$1" | sort); }
$ blame /nix/store/njdkyj7lk0ghyaiyd73xm045vzddv1db-gcc-5.4.0
/nix/store/j5plvf0jn9ywd7amwb4f2zsddkqdq8j3-php-7.0.14          
/nix/store/k2087nwdxr4v9ychx8cick7g2cis801a-apr-1.5.2-dev
/nix/store/n5s3zqfnf10xg227yam0zmqwpjbg1xy5-libtool-2.4.
/nix/store/qbca0n2m92c5lx8iisgkjaxgwzyrdbix-apache-httpd-2.4.23
/nix/store/yrxm2jz2ki37f2zl80ln4higlygk1l5i-hydra-2016-04-15
/nix/store/njdkyj7lk0ghyaiyd73xm045vzddv1db-gcc-5.4.0
/nix/store/w303yxk4fs7adw6mq8jn5hj8bh0r47i3-gcc-wrapper-5.4.0
$ blame /nix/store/w303yxk4fs7adw6mq8jn5hj8bh0r47i3-gcc-wrapper-5.4.0
/nix/store/bzlniad3yydk7w7b51hk7qddy31sdyf2-rpm-4.13.0-rc1
/nix/store/k2087nwdxr4v9ychx8cick7g2cis801a-apr-1.5.2-dev
/nix/store/n5s3zqfnf10xg227yam0zmqwpjbg1xy5-libtool-2.4.6
/nix/store/vn240njwiw71sr7v2j5i2d5wsv3xb6ji-mariadb-client-10.1.19-bin
/nix/store/w303yxk4fs7adw6mq8jn5hj8bh0r47i3-gcc-wrapper-5.4.0

At least mariadb, php and apache should not depend on the gcc command/wrapper.

Run strip of each file and discard expected failure types. Also default to stripping the entire output.

vcunat · 2017-01-08T17:23:41Z

@wmertens: such things are doable without any change to nix. There's a PR actually #15539, but whatever way you implement such a feature, it would always decrease purity, so it's not easy to get consensus on such a thing around NixOS.org.

We did this for 2.2 (cc61d31) but lost this for 2.4. This reduces the Apache closure size from 312 MiB to 102 MiB (primarily by getting rid of -dev outputs).

FRidh · 2017-07-15T16:32:07Z

What's the status of this PR?

vcunat · 2017-07-15T16:40:54Z

I believe the status is that... I've been swamped and this "cosmetic" change (improving "only" logs) got left behind; I just haven't managed to properly review it.

Ekleog · 2018-10-13T18:37:59Z

(triage) Is anyone still working or willing to work on this PR? I'd guess other people than @vcunat are able to review it?

danbst · 2019-01-08T11:50:17Z

@FRidh @Ekleog see discussion #23648

A way to figure out dontStripList automagically is needed. Otherwise it adds a bit of complication to package.
To minimize whole world rebuilds, apply suggestion from Binaries are not stripped #21667 (comment) about --strip-all for files in libexec bin sbin
Separate Hydra job to rebuild everything with this PR. Fix packages when needed
After this is merged, stripDirs: Silence annoying 'File format not recognized' errors #23648 should be reverted
strip: --strip-all in libexec, bin, sbin #31025 would become unneeded

So, it isn't an easy merge, requires time and patience. cc @infinisil

layus · 2019-08-13T10:25:56Z

Just closing old PRs that are too work-intensive. I guess #23648 (comment) got it quite right ;-).

layus mentioned this pull request May 9, 2016

Just strip everything by default #15087

Closed

domenkozar reviewed May 10, 2016
View reviewed changes

layus mentioned this pull request May 12, 2016

setup-hooks: do not pass missing dirs to find #15405

Closed

vcunat self-assigned this May 19, 2016

bjornfor mentioned this pull request May 22, 2016

strip: File format not recognized #14842

Open

layus force-pushed the fix-strip branch 3 times, most recently from e2e3876 to b2e5072 Compare May 26, 2016 07:24

vcunat reviewed Jun 9, 2016
View reviewed changes

vcunat mentioned this pull request Jun 23, 2016

glibc, gcc: closure-size fixes for ARM targets #15867

Merged

layus force-pushed the fix-strip branch 2 times, most recently from 085b004 to bda09f6 Compare June 28, 2016 09:14

layus force-pushed the fix-strip branch from bda09f6 to 641301f Compare June 28, 2016 15:42

This was referenced Jul 20, 2016

apache: strip extra dirs. #14823

Closed

ldb: strip non-standard binaries #14821

Closed

php: add a dev ouptut & strip all the binaries #14822

Closed

go: strip binaries in /share #14824

Closed

layus force-pushed the fix-strip branch from 641301f to a8dbc63 Compare August 9, 2016 14:03

vcunat mentioned this pull request Jan 8, 2017

Binaries are not stripped #21667

Open

layus added 4 commits January 8, 2017 15:53

Just strip everything by default

c1d2b27

Run strip of each file and discard expected failure types. Also default to stripping the entire output.

swig: stripping share/ fails, don't

6dcb5ab

Remove superfluous stripDebugList

0c718ed

go: do not strip upstream elf test data

1f93a82

layus force-pushed the fix-strip branch from a8dbc63 to 1f93a82 Compare January 9, 2017 07:43

vcunat mentioned this pull request Feb 7, 2017

Darwin strip is evil and can modify files even if they aren't valid Mach-O #22520

Closed

layus mentioned this pull request Mar 9, 2017

stripDirs: Silence annoying 'File format not recognized' errors #23648

Merged

layus referenced this pull request Mar 22, 2017

apache-httpd: Strip modules

4e54611

We did this for 2.2 (cc61d31) but lost this for 2.4. This reduces the Apache closure size from 312 MiB to 102 MiB (primarily by getting rid of -dev outputs).

FRidh added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 15, 2017

illegalprime mentioned this pull request Apr 23, 2019

Add toradex apalis imx6 board configs illegalprime/nixos-on-arm#2

Merged

layus closed this Aug 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Just strip everything by default, fully tested version :-) #15339

Just strip everything by default, fully tested version :-) #15339

layus commented May 9, 2016 •

edited

Loading

mention-bot commented May 9, 2016

domenkozar May 10, 2016 •

edited

Loading

ambrop72 May 10, 2016

peti May 10, 2016

layus May 10, 2016

ambrop72 May 10, 2016 •

edited

Loading

domenkozar commented May 10, 2016

vcunat commented May 20, 2016

vcunat commented May 20, 2016

layus commented May 20, 2016

vcunat commented May 20, 2016

vcunat commented May 20, 2016

layus commented May 20, 2016

vcunat commented May 20, 2016

layus commented May 20, 2016

vcunat commented May 20, 2016

layus commented May 26, 2016 •

edited

Loading

layus commented Jun 8, 2016

vcunat Jun 9, 2016 •

edited

Loading

layus Jun 27, 2016

layus Jun 28, 2016

layus commented Aug 9, 2016

vcunat commented Aug 9, 2016

langston-barrett commented Oct 3, 2016

wmertens commented Dec 18, 2016

vcunat commented Dec 18, 2016 •

edited

Loading

layus commented Jan 8, 2017

vcunat commented Jan 8, 2017 •

edited

Loading

FRidh commented Jul 15, 2017

vcunat commented Jul 15, 2017

Ekleog commented Oct 13, 2018

danbst commented Jan 8, 2019

layus commented Aug 13, 2019

Just strip everything by default, fully tested version :-) #15339

Just strip everything by default, fully tested version :-) #15339

Conversation

layus commented May 9, 2016 • edited Loading

mention-bot commented May 9, 2016

domenkozar May 10, 2016 • edited Loading

Choose a reason for hiding this comment

ambrop72 May 10, 2016

Choose a reason for hiding this comment

peti May 10, 2016

Choose a reason for hiding this comment

layus May 10, 2016

Choose a reason for hiding this comment

ambrop72 May 10, 2016 • edited Loading

Choose a reason for hiding this comment

domenkozar commented May 10, 2016

vcunat commented May 20, 2016

vcunat commented May 20, 2016

layus commented May 20, 2016

vcunat commented May 20, 2016

vcunat commented May 20, 2016

layus commented May 20, 2016

vcunat commented May 20, 2016

layus commented May 20, 2016

vcunat commented May 20, 2016

layus commented May 26, 2016 • edited Loading

layus commented Jun 8, 2016

vcunat Jun 9, 2016 • edited Loading

Choose a reason for hiding this comment

layus Jun 27, 2016

Choose a reason for hiding this comment

layus Jun 28, 2016

Choose a reason for hiding this comment

layus commented Aug 9, 2016

vcunat commented Aug 9, 2016

langston-barrett commented Oct 3, 2016

wmertens commented Dec 18, 2016

vcunat commented Dec 18, 2016 • edited Loading

layus commented Jan 8, 2017

vcunat commented Jan 8, 2017 • edited Loading

FRidh commented Jul 15, 2017

vcunat commented Jul 15, 2017

Ekleog commented Oct 13, 2018

danbst commented Jan 8, 2019

layus commented Aug 13, 2019

layus commented May 9, 2016 •

edited

Loading

domenkozar May 10, 2016 •

edited

Loading

ambrop72 May 10, 2016 •

edited

Loading

layus commented May 26, 2016 •

edited

Loading

vcunat Jun 9, 2016 •

edited

Loading

vcunat commented Dec 18, 2016 •

edited

Loading

vcunat commented Jan 8, 2017 •

edited

Loading