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

Just strip everything by default #15087

Closed
wants to merge 1 commit into from
Closed

Conversation

layus
Copy link
Member

@layus layus commented Apr 29, 2016

Stripping binaries is of utmost importance to benefit from the closure-size changes.
To avoid missing custom paths (like /modules for php-like packages), strip the whole output.

This PR has the following advantages

  1. It does not produce pointless warning about unrecognized filetypes;
  2. It does strip everything that strip can strip;
  3. It does not ignore failures of strip (other than unrecognized filetypes).

I have tested it on some derivations (ldb, php) by fiddling around with makeSetupHook, but not on stdenv. That being said, the only way this patch could break stuff is by stripping something that should not be stripped and that resides outside of lib lib32 lib64 libexec bin sbin.

NB: This triggers a mass-rebuild.
Fixes #14821, #14822, #14823, #14824.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @edolstra to be a potential reviewer

Run strip of each file and discard expected failure types.
Also default to stripping the entire output.
@danbst
Copy link
Contributor

danbst commented Apr 30, 2016

When this is merged some dontStrip properties can be removed from expressions. I've used dontStrip exactly to shut up file format warnings

@edolstra
Copy link
Member

edolstra commented May 2, 2016

Looks good to me.

@vcunat vcunat added the 1.severity: mass-rebuild This PR causes a large number of packages to rebuild label May 2, 2016
@layus
Copy link
Member Author

layus commented May 3, 2016

Thanks for the reviews.
I have a few questions: Is staging the right branch to pull-request against for this impacting change? And what is the usual policy for merging mass-rebuild changes? I assume we try to group them and merge a bundle now and then, right?

@globin
Copy link
Member

globin commented May 3, 2016

It is the right branch, normally someone looks through the mass-rebuild changes and merges them about once a week.

@peti
Copy link
Member

peti commented May 3, 2016

Merged to staging in 2362891.

@peti peti closed this May 3, 2016
@peti
Copy link
Member

peti commented May 3, 2016

Hmm, the error https://travis-ci.org/NixOS/nixpkgs/builds/127459031#L1131 looks a little disconcerting. @layus, have you tested the bootstrapping process with your patch applied?

@layus
Copy link
Member Author

layus commented May 3, 2016

That was fast :-). No, as I explain above, I have done little testing on this (because I do not know how, except rebuilding my whole system). I actually started a nix-build -A stdenv and stopped it after some time because everything was going well. Not much of a test.

Now, I have no idea how a line like mkdir: cannot create directory '/nix-support': Permission denied can be related to this. I can however reproduce the bug locally with a simple nix-build -A zlib on branch staging.
Trying to investigate the issue...

@layus
Copy link
Member Author

layus commented May 3, 2016

More info:
The setup script has (/nix/store/2fcjgppcvy54l1j7mk4iirg46az9q7iw-stdenv-linux-boot/setup, line 774):

    if [ -n "$setupHook" ]; then 
        mkdir -p ${!outputDev}/nix-support"
        substituteAll "$setupHook" "${!outputDev}/nix-support/setup-hook"
    fi

which fails because

  1. paxctl had a setupHook, and
  2. somehow, ${!outputDev} is empty.

Looking further...

@layus
Copy link
Member Author

layus commented May 3, 2016

@peti, Look no further, the issue is obviously in the patch as it overrides $out

+            if out=$(strip $commonStripFlags $stripFlags "$f" 2>&1); then

I need to make my variables local!
I will submit a better version soon, but feel free to do it before me if you feel so.

@peti
Copy link
Member

peti commented May 3, 2016 via email

@peti
Copy link
Member

peti commented May 4, 2016

I reverted the patch in 397c75a.

@layus
Copy link
Member Author

layus commented May 4, 2016

Yes, seems better as the fix has complications.
Now that I am testing it properly, it appears that stripping an empty file for example is not safe.
if the file contains only a line break, then strip fails with a "File truncated" error.

So this patch also requires small adjustments in other derivations.

peti referenced this pull request May 4, 2016
Run strip of each file and discard expected failure types.
Also default to stripping the entire output.
@layus
Copy link
Member Author

layus commented May 9, 2016

Somehow, I lost track of this branch and GitHub won't let me push commits to it.
The new version is in #15339.

@peti, the new version has been fully tested. It does not break stdenv and updates the manual \o/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.severity: mass-rebuild This PR causes a large number of packages to rebuild
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants