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

[dmgk]

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")

[0mp]

Seems good!

[dmgk]

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]

Mk/Uses/go.mk

79 ↗

(On Diff #62349)

Maybe use

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

instead to not break mail/aerc and others.

[dmgk]

Mk/Uses/go.mk

79 ↗

(On Diff #62349)

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]

Mk/Uses/go.mk

79 ↗

(On Diff #62349)

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]

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.

[tobik]

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 ↗

(On Diff #62349)

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]

In D21730#474823, @tobik 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.

[araujo]

In D21730#475087, @dmgk wrote:

In D21730#474823, @tobik 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]

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

[dmgk]

Mk/Uses/go.mk

78 ↗

(On Diff #62375)

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

[dmgk]

In D21730#475088, @araujo wrote:

Please do so, seems very beneficial!

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

[dmgk]

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.