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

cmd/go: override semantics break e.g. GOFLAGS=-ldflags #38522

Open
stapelberg opened this issue Apr 19, 2020 · 11 comments
Open

cmd/go: override semantics break e.g. GOFLAGS=-ldflags #38522

stapelberg opened this issue Apr 19, 2020 · 11 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@stapelberg
Copy link
Contributor

I was asked to post this as a separate issue in #26849 (comment), so here goes:

Comment #26849 (comment) already touches upon the override semantics issue (-ldflags=-w -ldflags=-s results in only -s), but since that example would likely be fixed by quoting changes, I’d like to stress another nuance not addressed by quoting: the current override behavior makes setting -ldflags in GOFLAGS impossible when the go tool command lines contain an explicit -ldflags flag.

For my use-case, I want to set the ELF interpreter in my build system, so I figured I’d set the GOFLAGS environment variable like so:

GOFLAGS="-ldflags=-extldflags=-Wl,--dynamic-linker=/ro/glibc-amd64-2.31-4/out/lib/ld-linux-x86-64.so.2"

This works in general, but breaks as soon as the software in question specifies its own ldflags on the command line, like e.g. containerd does in its Makefile.

It’s easy to verify this:

GOFLAGS=-ldflags=-invalid go build # fails
GOFLAGS=-ldflags=-invalid go build -ldflags=-s # does not fail

Notably, from C build systems, I’m used to LDFLAGS generally being extended, never overridden.

For my use-case, I’m currently setting -Wl,--dynamic-linker in CGO_LDFLAGS instead. Not entirely sure how reliable that is longer-term, but it seems to work with the packages I currently care about.

@mvdan
Copy link
Member

mvdan commented Apr 19, 2020

I think @myitcv also raised a similar point about -tags. It's impossible to set one tag via GOFLAGS, and some extra tags via command line flags, as the latter will simply overwrite the former.

-ldflags already has semantics that allow merging values in some cases, so I think it's reasonable to extend that to support this use case. The alternative would be to redesign GOFLAGS somehow, or to not support this use case at all, both of which seem like worse options to me.

For example, here's one sample idea to extend the current syntax. Allow pattern+=flags, where += means that the flags should be merged with the resulting list of flags for a pattern, instead of substituting (or being substituted by) other flags. This way, @stapelberg could use GOFLAGS="-ldflags=all+=-extldflags=-Wl,--dynamic-linker=/ro/glibc-amd64-2.31-4/out/lib/ld-linux-x86-64.so.2".

Edit: I realise that all=<flags> and <flags> mean different things today, but I think Michael wants to use all= for his use case anyway, since they are global flags for an entire build.

CC @bcmills @jayconrod

@bcmills
Copy link
Contributor

bcmills commented Apr 20, 2020

CC @matloob

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 20, 2020
@bcmills bcmills added this to the Backlog milestone Apr 20, 2020
@rsc
Copy link
Contributor

rsc commented Apr 21, 2020

Yes, changing the default behavior is entirely reasonable except when it isn't. For example, what if I set

GOFLAGS=-ldflags=-w

to turn off DWARF by default, but then I want to override that, with go build -ldflags= to get a DWARF binary?

@rsc
Copy link
Contributor

rsc commented Apr 21, 2020

In general the possible can of complexity we could open here is quite large, and I am wary of anything that sounds "straightforward". For example, supposing we had the += syntax, using it in GOFLAGS would not be enough, because GOFLAGS applies before the command line. The Makefile command line would still need to be changed too. At that point why not make the Makefile accept from a given environment variable the things you want to inject?

@bcmills bcmills modified the milestones: Backlog, Unplanned Apr 21, 2020
@thanm
Copy link
Contributor

thanm commented Apr 21, 2020

@rsc in your example if ldflags were indeed "additive", you could enabled a DWARF binary by just adding in -ldflags=-w=0, I think.

@thanm
Copy link
Contributor

thanm commented Apr 21, 2020

It is pretty common with other compilers/linkers to have the concept of "rightmost setting wins", e.g. if you say "-w -w=0 -w=1 -w=0" you get -w=0.

@ianlancetaylor
Copy link
Contributor

I agree with the rightmost setting wins rule, which is what the linker implements anyhow.

That is how #cgo LDFLAGS: works today with respect to -extldflags. When invoking the external linker we pass the aggregated #cgo LDFLAGS values, then the -extldflags values.

johnboyes added a commit to agilepathway/label-checker that referenced this issue Jun 27, 2020
This commit removes the DWARF debugging information[1]

Decided not to try to also turn off generation of the Go symbol table
using the `-s` flag, as:
1. the GOFLAGS functionality currently only allows 1 ldflag to be set[2]
   (NB this may be fixed in Go 1.15[3])
2. The improvements to the binary size in Go 1.15 [4] mean that the `-s`
   flag will no longer really do anything[5]

Will also try shrinking the binary size further using upx[6] in a
separate pull request.

[1] https://stackoverflow.com/a/22276273
[2] golang/go#38522
[3] golang/go#26849 (comment)
[4] See #64
[5] https://twitter.com/bradfitz/status/1256989624590712834
[6] https://upx.github.io/
johnboyes added a commit to agilepathway/label-checker that referenced this issue Jun 27, 2020
This commit removes the DWARF debugging information[1], resulting in the
Docker image size shrinking from 4.76MB to 2.88MB.

Decided not to try to also turn off generation of the Go symbol table
using the `-s` flag, as:
1. the GOFLAGS functionality currently only allows 1 ldflag to be set[2]
   (NB this may be fixed in Go 1.15[3])
2. The improvements to the binary size in Go 1.15 [4] mean that the `-s`
   flag will no longer really do anything[5]

Will also try shrinking the binary size further using upx[6] in a
separate pull request.

[1] https://stackoverflow.com/a/22276273
[2] golang/go#38522
[3] golang/go#26849 (comment)
[4] See #64
[5] https://twitter.com/bradfitz/status/1256989624590712834
[6] https://upx.github.io/
johnboyes added a commit to agilepathway/label-checker that referenced this issue Jun 27, 2020
This commit removes the DWARF debugging information[1], resulting in the
Docker image size shrinking from 4.76MB to 2.88MB.

Decided not to try to also turn off generation of the Go symbol table
using the `-s` flag, as:
1. the GOFLAGS functionality currently only allows 1 ldflag to be set[2]
   (NB this may be fixed in Go 1.15[3])
2. The improvements to the binary size in Go 1.15 [4] mean that the `-s`
   flag will no longer really do anything[5]

Will also try shrinking the binary size further using upx[6] in a
separate pull request.

[1] https://stackoverflow.com/a/22276273
[2] golang/go#38522
[3] golang/go#26849 (comment)
[4] See #64
[5] https://twitter.com/bradfitz/status/1256989624590712834
[6] https://upx.github.io/
@rsc
Copy link
Contributor

rsc commented Sep 18, 2020

This discussion never fully resolved, but it sounds like the rough consensus is that the current behavior is working as intended.

@stapelberg
Copy link
Contributor Author

I can get behind declaring the semantics as “rightmost setting wins”.

However, the larger issue is “working as intended” only if we declare my use-case invalid :).

Thinking about this again, maybe it would be helpful for this discussion to frame things in terms of default options and override options? Currently, GOFLAGS allows setting defaults (overridden by the command-line), but perhaps the best way to enable my use-case is to add an environment variable with override semantics?

@rsc
Copy link
Contributor

rsc commented Oct 1, 2020

@stapelberg, I wouldn't want to add an environment variable that overrides the command line. That seems very wrong to me. Above I wrote:

At that point why not make the Makefile accept from a given environment variable the things you want to inject?

That still seems like the right approach to me. It's not the go command's responsibility to work around inadequate makefiles.

@stapelberg
Copy link
Contributor Author

That’s fair. I agree that a command-line flag is more direct than an environment variable, and it makes sense that the environment variable supplies defaults which the command-line overrides.


More as a note to myself: the take-away is that GOFLAGS can and should still be set in my build system, but will only help with packages that are built with the go tool directly. For packages that are built indirectly, e.g. via a Makefile, package-specific care must be taken to pass through build-system-wide policy flags (such as the ELF interpreter).


I see that https://golang.org/cmd/go/ currently already documents the override semantics:

GOFLAGS
A space-separated list of -flag=value settings to apply
to go commands by default, when the given flag is known by
the current command. Each entry must be a standalone flag.
Because the entries are space-separated, flag values must
not contain spaces. Flags listed on the command line
are applied after this list and therefore override it.

Still left to do: the “rightmost setting wins” semantics should also be documented here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants