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: Binaries are not stripped by default. Proposal to add -s -w ldflags by default #177698

Open
azahi opened this issue Jun 14, 2022 · 13 comments

Comments

@azahi
Copy link
Member

azahi commented Jun 14, 2022

I was wondering, is there a specific reason for -s and -w ldflags not being in the buildGoModule's buildFlagsArray=() and shouldn't we enable them by default or, for instance, when the ldflags attribute is not explicitly defined by package's derivation?

As outlined in documentation for cmd/link, flag description is as follows:

-s
	Omit the symbol table and debug information.
-w
	Omit the DWARF symbol table.

Right now, if these flags are not included, binaries are left unstripped and consequently bigger in size. Basically, to include these flags would mean to mimic what strip command does in stdenv. Or we could just add strip itself after the build if it's not already somehow inherited from stdenv.

CC @NixOS/golang

@zowoq
Copy link
Contributor

zowoq commented Jun 15, 2022

#157207 (comment)
I've looked at doing it a couple of times previously but it wasn't a good default to set. Sometimes caused weird issues or outright breakage and was generally just a PITA for the amount of derivations that would need to opt out. Homebrew and a couple of other distros also looked at making it a default but decided against it.

I haven't tried rebuilding all of nixpkgs with it since 1.16 1.15, once we're on 1.18 (hopefully soon) I'll take another look at it.

@NickCao NickCao mentioned this issue Jul 8, 2022
13 tasks
@azahi
Copy link
Member Author

azahi commented Jul 22, 2022

Since #179622 is merged now, @zowoq any chance you can look into this again? I'm willing to help but I currently don't have enough horse power for mass rebuilds.

/cc @SuperSandro2000

@zowoq
Copy link
Contributor

zowoq commented Jul 22, 2022

I'll follow up on this in the next couple of weeks.

@SuperSandro2000
Copy link
Member

IMO we should enable this by default and disable it for the few packages that do not support it.

@zowoq
Copy link
Contributor

zowoq commented Jul 24, 2022

Here is the PR: #182743

I still need to recheck what other distros are doing in case there are reasons not to do this and check for breakage in packages.

@zowoq zowoq linked a pull request Jul 25, 2022 that will close this issue
13 tasks
@zowoq
Copy link
Contributor

zowoq commented Sep 1, 2022

Alpine have recently removed -s -w ldflags from all packages as it was already handled by the strip in their build tooling. Rather than setting these flags we should find out why ours doesn't cover this and fix it. This would also be the more complete fix as setting default ldflags in buildGo{Module,Package} would only have worked for packages that don't override the default buildPhase.

@winterqt
Copy link
Member

winterqt commented Sep 1, 2022

we should find out why ours doesn't cover this and fix it.

Just tested this -- our strip produces the same llvm-dwarf-dump --all output that a Go-stripped binary does.

Did our strip at one point not work fully, or is that speculation? Did we all (as in, Alpine and us) just add the flags for completeness, when they were actually extraneous since we strip at the end? Or were they needed at some point to fully strip debug info?

@zowoq
Copy link
Contributor

zowoq commented Sep 1, 2022

our strip produces the same llvm-dwarf-dump --all output that a Go-stripped binary does.

Assuming that this was on linux, it isn't the same on x86_64-darwin.

Did our strip at one point not work fully, or is that speculation? Did we all (as in, Alpine and us) just add the flags for completeness, when they were actually extraneous since we strip at the end? Or were they needed at some point to fully strip debug info?

IIRC these flags did make a difference on linux at one point. Not sure why these flags were sometimes problematic in the past if the stdenv strip will produce the same result anyway.

@winterqt
Copy link
Member

winterqt commented Sep 1, 2022

it isn't the same on x86_64-darwin

I (naively) only tested Linux, correct.

IIRC these flags did make a difference on linux at one point. Not sure why these flags were sometimes problematic in the past if the stdenv strip will produce the same result anyway.

Do you remember how they were problematic?

@zowoq
Copy link
Contributor

zowoq commented Sep 1, 2022

if these flags are not included, binaries are left unstripped and consequently bigger in size.

Removing these flags still seems to cause bigger binaries on linux?

@winterqt
Copy link
Member

winterqt commented Sep 1, 2022

Removing these flags still seems to cause bigger binaries on linux?

If I'm correctly understanding the point you're trying to make: yes, that's to be expected, as we only strip debug symbols by default (as opposed to stripping all symbols).

If you manually run strip on a copy of a binary made without those ldflags, the size should be closer (if not the same) to the binaries made with them.

We can set stripDebugFlags to -s (note the lowercase) by default in buildGo{Module,Package} to get parity with setting the flags.

@azahi azahi mentioned this issue Sep 11, 2022
13 tasks
@zowoq
Copy link
Contributor

zowoq commented Sep 21, 2022

We can set stripDebugFlags to -s (note the lowercase) by default in buildGo{Module,Package} to get parity with setting the flags.

stripAllFlags=" " # the Darwin "strip" command doesn't know "-s"

Doesn't seem like we have a way of doing this consistently with strip. Guess that bring us back to where we started, setting defaultLdflags even though it will only work for packages using the default buildPhase.

@qbit
Copy link
Contributor

qbit commented Jan 24, 2023

Doesn't seem like we have a way of doing this consistently with strip. Guess that bring us back to where we started, setting defaultLdflags even though it will only work for packages using the default buildPhase.

fwiw, that's what we do in OpenBSD land: https://github.com/openbsd/ports/blob/master/lang/go/go.port.mk#L111

Maybe we can create an example buildPhase in the docs or something that will encourage the usage of defaultLdflags with other flags appended/prepended?

@katexochen katexochen moved this to Backlog in Go team Oct 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

5 participants