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

buildGoModule: refactor stripping #346380

Open
5 tasks
katexochen opened this issue Oct 4, 2024 · 3 comments
Open
5 tasks

buildGoModule: refactor stripping #346380

katexochen opened this issue Oct 4, 2024 · 3 comments
Assignees

Comments

@katexochen
Copy link
Contributor

katexochen commented Oct 4, 2024

Current situation

  • Many Go packages in nixpkgs set ldflags = [ "-s" "-w" ];
  • nix-init sets ldflags = [ "-s" "-w" ]; for Go packages
  • stdenv has it's own stripping mechanism, which is turned on by default and can be disabled with dontStrip = true;
  • For debugging, people want to override individual packages with dontStrip = true; and get the expected debug symbols, which isn't currently working for many packages that set -s -w on a package level (see Golang: add support for the gcflags attribute #277609)
  • At the same time, the stripping done by stdenv isn't removing all symbols, so binaries aren't as small as they could be (see buildGoModule: Binaries are not stripped by default. Proposal to add -s -w ldflags by default #177698 and table below)
  • There have been repeated discussions in PR review about whether these flags should be added or not

History

About -w

From the Go v1.22 release notes:

The -w flag suppresses DWARF debug information generation. The -s flag suppresses symbol table generation. The -s flag also implies the -w flag

Setting -w in combination with -s has no effect.

Suggested action items:

  • Update nix-init to not set -w flag
  • Remove all occurrences of -w in nixpkgs

About -s

Go's native stripping works far better than the stripping done by stdenv (see table below for comparison), binaries are smaller and recognized by file as stripped.

In my opinion, we should use Go's native stripping mechanism by default in buildGoModule. This would mean that we:

  1. Add -s to the ldflags inside of buildGoModule
  2. Don't add -s to the ldflags in case dontStrip is set
  3. Throw a warning in case -s is set in ldflags

We can discuss whether we want to always set dontStrip on the underlying derivation (maybe to fasten up build time) or if we keep it active to prevent confusion and for mixed language derivations (I'd opt for keeping it for now).

Suggested action items:

  • Update nix-init to not set -s flag
  • Update buildGoModule to strip with -s flag by default, to drop -s when dontStrip = true, throw warning if flag is parsed from outside
  • Remove all occurrences of -s in nixpkgs

Example: stripping statistics for uplosi on x86_64-linux

CGO_ENABLED dontStrip ldflags size file
1 false 73871744 (71MiB) dynamically linked, not stripped,
1 true 89904674 (86MiB) dynamically linked, with debug_info, not stripped,
1 false -w 73871744 (71MiB) dynamically linked, not stripped,
1 true -w 73880107 (71MiB) dynamically linked, not stripped,
1 false -s 64374008 (62MiB) dynamically linked, stripped,
1 true -s 64372979 (62MiB) dynamically linked, stripped,
1 false -s -w 64374008 (62MiB) dynamically linked, stripped,
1 true -s -w 64372979 (62MiB) dynamically linked, stripped,
0 false 73753128 (71MiB) statically linked, not stripped,
0 true 89767921 (86MiB) statically linked, with debug_info, not stripped,
0 false -w 73753128 (71MiB) statically linked, not stripped,
0 true -w 73762114 (71MiB) statically linked, not stripped,
0 false -s 64269808 (62MiB) statically linked, stripped,
0 true -s 64270471 (62MiB) statically linked, stripped,
0 false -s -w 64269808 (62MiB) statically linked, stripped,
0 true -s -w 64270471 (62MiB) statically linked, stripped,

Created with katexochen@169e31b, which can be used to check results for other packages.

@zowoq
Copy link
Contributor

zowoq commented Oct 5, 2024

I looked at this a while ago: #177698


  1. Add -s to the ldflags inside of buildGoModule
  2. Don't add -s to the ldflags in case dontStrip is set
  3. Throw a warning in case -s is set in ldflags

Agree.

We can discuss whether we want to always set dontStrip on the underlying derivation (maybe to fasten up build time) or if we keep it active to prevent confusion and for mixed language derivations (I'd opt for keeping it for now).

Agree with keeping it for now.

@katexochen

This comment was marked as resolved.

@katexochen
Copy link
Contributor Author

I've updated the issue description with the additional pointers to the previous work. For me, there are two points from the previous discussion we should note:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

No branches or pull requests

4 participants