Page MenuHomeFreeBSD

Mk/Uses/go.mk: Omit symbol table and debug information by default
ClosedPublic

Authored by dmgk on Sep 20 2019, 2:58 PM.

Details

Summary

Mk/Uses/go.mk: Add linker flags to omit symbol table and debug information
unless WITH_DEBUG is defined or port sets its own -ldflags.

PR: 233335

Test Plan

Tested with:

poudriere testport devel/awless (doesn't set custom -ldflags)
poudriere testport ports-mgmt/modules2tuple (sets custom -ldflags, but without "-s -w")

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26592
Build 24976: arc lint + arc unit

Event Timeline

dmgk created this revision.Sep 20 2019, 2:58 PM
0mp added a subscriber: pizzamig.Sep 20 2019, 3:25 PM

Seems good!

tobik accepted this revision.Sep 20 2019, 4:11 PM
dmgk added a comment.Sep 20 2019, 4:27 PM

I did some more testing and it turns out that this patch actually breaks a few ports (www/gohugo, devel/gitaly, mail/aerc) due to GO_BUILDFLAGS being expanded too early.
It should be possible to pass linker flags in GOFLAGS though, I'll look into that and update the patch.

tobik requested changes to this revision.Sep 20 2019, 4:45 PM
tobik added inline comments.
Mk/Uses/go.mk
79

Maybe use

GO_BUILDFLAGS+=	-ldflags=-s -ldflags=-w

instead to not break mail/aerc and others.

This revision now requires changes to proceed.Sep 20 2019, 4:45 PM
dmgk added inline comments.Sep 20 2019, 4:52 PM
Mk/Uses/go.mk
79

No, the issue is that empty(GO_BUILDFLAGS:M-ldflags*) expands GO_BUILDFLAGS and in case of e.g. www/gohugo it breaks the port because DISTINFO_FILE used in the shell command is not yet defined. I didn't look closely at other ports breakages but I guess it's the same issue.

And multiple -ldflags syntax doesn't work anyway unfortunately because second -ldflags overrides the first.

dmgk marked an inline comment as done.Sep 21 2019, 12:11 PM
dmgk added inline comments.
Mk/Uses/go.mk
79

Actually you were right about -ldflags - surprisingly, GOFLAGS -flag=value syntax doesn't allow spaces in value [1]. Just -s (Omit the symbol table and debug information) seems like a good compromise for now, it produces smaller executables and is enough to silence the strip warning.

[1] https://github.com/golang/go/issues/29096

dmgk updated this revision to Diff 62375.Sep 21 2019, 12:28 PM

Pass just -s because GOFLAGS doesn't support spaces in values.

This change depends on https://reviews.freebsd.org/D21735.

Tested with poudriere bulk over all Go ports.

emaste added a subscriber: emaste.Sep 22 2019, 11:49 AM
tobik accepted this revision.Sep 23 2019, 4:56 AM

IIUC this potentially changes all Go binaries, so all USES=go ports that do not set ldflags themselves probably need a PORTREVISION bump afterwards to relink them?

Mk/Uses/go.mk
79

Yeah, -s seems to be enough.

I misunderstood how -ldflags works. go help build is pretty clear that "the latest match on the command line wins." The semantics of -ldflags are weird to me. If it would append to previous values (like with rustc -Clink-arg) integrating this into ports would be much easier.

dmgk added a comment.Sep 23 2019, 9:55 PM

IIUC this potentially changes all Go binaries, so all USES=go ports that do not set ldflags themselves probably need a PORTREVISION bump afterwards to relink them?

A quick search

for f in $(find /usr/ports -name Makefile | xargs grep -l 'USES=.*\bgo\b'); do grep -q '\bldflags\b' $f || echo $f; done | wc -l

shows that it would require bumping PORTREVISION of 198 ports. Recompiling will make binaries about 25% smaller on average but there are no other functional changes. I can prepare a separate review if we're OK with this.

In D21730#475087, @dmgk wrote:

IIUC this potentially changes all Go binaries, so all USES=go ports that do not set ldflags themselves probably need a PORTREVISION bump afterwards to relink them?

A quick search

for f in $(find /usr/ports -name Makefile | xargs grep -l 'USES=.*\bgo\b'); do grep -q '\bldflags\b' $f || echo $f; done | wc -l

shows that it would require bumping PORTREVISION of 198 ports. Recompiling will make binaries about 25% smaller on average but there are no other functional changes. I can prepare a separate review if we're OK with this.

Please do so, seems very beneficial!
Thanks for all this work.

araujo accepted this revision.Sep 24 2019, 2:02 AM

Is it worth do an exp-run on this one too?

dmgk added a reviewer: mat.Sep 24 2019, 12:21 PM
dmgk added inline comments.
Mk/Uses/go.mk
78

@mat Is there a way to avoid early evaluation by empty here?

dmgk marked an inline comment as not done.Sep 30 2019, 10:25 PM

Please do so, seems very beneficial!

Review for PORTREVISION bump of ports without explicit -ldflags: https://reviews.freebsd.org/D21854

dmgk added a comment.Nov 15 2019, 2:15 PM

The exp-run (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=240986) doesn't seem to be having any progress, I'm going to do a local test build of this change along with https://reviews.freebsd.org/D21854 and commit.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 16 2019, 12:39 PM
This revision was automatically updated to reflect the committed changes.