-
Notifications
You must be signed in to change notification settings - Fork 20.1k
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
build: use -trimpath flag when building executables #21374
Conversation
Trimpath if compiling with Go >= 1.13
build/ci.go
Outdated
@@ -268,14 +273,11 @@ func doInstall(cmdline []string) { | |||
|
|||
func buildFlags(env build.Environment) (flags []string) { | |||
var ld []string | |||
ld = append(ld, "-s", "-w") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a good change. I'm not even sure why it's disabled on darwin? (cc @fjl) We need the debug symbols to interpret crash logs, otherwise we'll get useless bugreports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently Go is a bit smarter and still keeps a symbol stub for crash dumps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The symbol table is mostly for external debuggers. Without it external debugging with tools like 'delve' is not possible. I can agree with adding -trimpath
because it helps with reproducible builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fjl Any particular reason that -s was only added for macos, not the rest? I've just tried it on Linux and it does cut off 10MB of junk + stack traces and pprof still work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was added for macOS in 1cf2ee4 to fix some cgo issue. We can probably remove it again because there have been two major macOS releases and multiple go releases since then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know -s makes the binary smaller, but as I said, stripping the debug information has downsides for debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, all in all we should leave the debug symbols as they are currently. Given the huge dataset Ethereum operates on, seems to be pointless to try and save a few MBs in exchange of less debuggability. Please revert that part.
The path trimming is a good change, but you can just leave it enabled for all code paths, not check for Go 1.13, we already have that as a minimum requirement due to the special error handling.
Instead of special casing .13 for trim, please change the minor < 11
check to 13.
@karalabe Comments addressed in latest commit. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
* Disable symbol table and DWARF generation by default. Trimpath if compiling with Go >= 1.13 * Set Go to minimum version 1.13. Revert debug symbol changes.
Disable symbol table and DWARF generation by default.
Trimpath if compiling with Go >= 1.13
On latest release 1.9.17, this reduces build size by 10MB+
Before:
After:
Trimpath also enhances privacy by stripping the full path of the source code location.
To see what I mean, run this on the binary: