Skip to content

setup-hooks/separate-debug-info.sh: don't inhibit strip hook#185537

Merged
trofi merged 1 commit intoNixOS:stagingfrom
trofi:decouple-strip-and-separate-debug
Aug 11, 2022
Merged

setup-hooks/separate-debug-info.sh: don't inhibit strip hook#185537
trofi merged 1 commit intoNixOS:stagingfrom
trofi:decouple-strip-and-separate-debug

Conversation

@trofi
Copy link
Contributor

@trofi trofi commented Aug 7, 2022 •
edited
Loading

Before the change separate-debug-info.sh did the stripping itself.
This scheme has a few problems:

  1. Stripping happens only on ELF files. *.a and *.o files are skipped.
    Derivations have to do it manually. Usually incorrectly
    as they don't run $RANLIB (true for glibc and musl).
  2. Stripping happens on all paths. Ideally only stripDebugList paths
    should be considered.
  3. Host strip is called on Target files.

This change offloads stripping logic to strip hook. This strips more
files for glibc and musl. Now we can remove most $STRIP calls
from individual derivations.

To avoid strip hook from stomping on results of separate-debug-info
hook we skip lib/debug directory from stripping.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Aug 7, 2022
SuperSandro2000
SuperSandro2000 reviewed Aug 7, 2022
View reviewed changes
@trofi @SuperSandro2000
setup-hooks/separate-debug-info.sh: don't inhibit strip hook
b3b672d 
Before the change separate-debug-info.sh did the stripping itself.
This scheme has a few problems:
1. Stripping happens only on ELF files. *.a and *.o files are skipped.
   Derivations have to do it manually. Usually incorrectly
   as they don't run $RANLIB (true for `glibc` and `musl`).
2. Stripping happens on all paths. Ideally only `stripDebugList` paths
   should be considered.
3. Host strip is called on Target files.

This change offloads stripping logic to strip hook. This strips more
files for `glibc` and `musl`. Now we can remove most $STRIP calls
from individual derivations.

Co-authored-by: Sandro <[email protected]>
@trofi trofi force-pushed the decouple-strip-and-separate-debug branch from 14cf4ae to b3b672d Compare August 7, 2022 11:49
@trofi trofi requested a review from SuperSandro2000 August 7, 2022 12:10
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Aug 7, 2022
@mweinelt mweinelt requested a review from oxalica August 7, 2022 19:59
@mweinelt
Copy link
Member

mweinelt commented Aug 7, 2022

@oxalica PTAL at the firefox common.nix change.

@risicle
Copy link
Contributor

risicle commented Aug 7, 2022

See #164520

@risicle
Copy link
Contributor

risicle commented Aug 7, 2022

Sorry let me expand on that - specifically https://github.com/NixOS/nixpkgs/pull/164520/files#r846774388.

It would be really good to handle .a files, because otherwise adding separateDebugInfo has the danger of being a bomb that leaves unstripped .a files in the regular output, massively bloating it.

@trofi
Copy link
Contributor Author

trofi commented Aug 7, 2022

Sorry let me expand on that - specifically https://github.com/NixOS/nixpkgs/pull/164520/files#r846774388.

It would be really good to handle .a files, because otherwise adding separateDebugInfo has the danger of being a bomb that leaves unstripped .a files in the regular output, massively bloating it.

Note that this PR drops dontStrip=1 from separateDebugInfo to enable automatic *.a (and *.o) file handling. This makes strip hook and separate-debug hooks both execute and do their job.

I started writing this change as I noticed that glibc is not fully stripping crt*.o and libm.a (and more in cross) files today. It should be fully handling it now.

@trofi trofi mentioned this pull request Aug 8, 2022
Merged
13 tasks
@Mindavi
Copy link
Contributor

Mindavi commented Aug 9, 2022

Less stripping hacks sounds like a good idea to me. I looked through the PR and didn't see any obvious issues, but haven't tested a build myself.

@trofi
Copy link
Contributor Author

trofi commented Aug 11, 2022

I tested a few cross-compilers and native compilers: pkgsCross.mingw32.re2c, pkgsCross.ppc64.re2c, pkgsMusl.re2c, pkgsLLVM.re2c`. Did a light build of my whole system.

Let's give it a try. Worst case we can revert it.

@trofi trofi merged commit 3ceb8a5 into NixOS:staging Aug 11, 2022
@trofi trofi deleted the decouple-strip-and-separate-debug branch August 11, 2022 07:53
@risicle risicle mentioned this pull request Dec 23, 2022
Closed
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mweinelt mweinelt Awaiting requested review from mweinelt

@FRidh FRidh Awaiting requested review from FRidh

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@oxalica oxalica Awaiting requested review from oxalica

Assignees

No one assigned

Labels

6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@trofi @mweinelt @risicle @Mindavi @SuperSandro2000