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: escape quoted strings in GOFLAGS #26849

Closed
bcmills opened this issue Aug 7, 2018 · 26 comments
Closed

cmd/go: escape quoted strings in GOFLAGS #26849

bcmills opened this issue Aug 7, 2018 · 26 comments
Assignees
Labels
FeatureRequest FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Aug 7, 2018

In https://golang.org/cl/126656, @rsc added the GOFLAGS environment variable.

The variable is space-separated, but it is sometimes useful to include spaces within flags passed to the go command (for example, as the final argument to list -f). It would be nice to recognize at least a subset of the usual quoting conventions when parsing GOFLAGS.

~/go/src/cmd/go$ go list -e -deps -f="{{if .Error}}{{.Error}}{{end}}"

~/go/src/cmd/go$ export GOFLAGS='-e -deps -f="{{if .Error}}{{.Error}}{{end}}"'

~/go/src/cmd/go$ go list
go: parsing $GOFLAGS: non-flag ".Error}}{{.Error}}{{end}}\""

@bcmills bcmills added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. FeatureRequest labels Aug 7, 2018
@bcmills bcmills added this to the Go1.11 milestone Aug 7, 2018
@alexbrainman
Copy link
Member

@bcmills if you want to support cmd.exe users on Windows, you should consider how quoting works in cmd.exe. For example, this http://daviddeley.com/autohotkey/parameters/parameters.htm#WINCMDRULES shows cmd.exe rules.

Alex

@rsc
Copy link
Contributor

rsc commented Aug 9, 2018

Sorry, but no. I looked at this when I added GOFLAGS, and we don't do any special quoting for the other environment variables (go help environment | grep FLAGS)

This means you cant use GOFLAGS for your example.
That's OK, GOFLAGS is for things like -ldflags=-w.

@rsc rsc closed this as completed Aug 9, 2018
@tianon
Copy link
Contributor

tianon commented Oct 3, 2018

That's OK, GOFLAGS is for things like -ldflags=-w.

My usage of -ldflags almost always also includes spaces -- I frequently use -d -s -w in order to get smaller static binaries (where size is the primary concern); combined with -tags netgo and CGO_ENABLED=0, this works pretty well.

I think -tags is another great problematic example -- multiple instances of -tags replace each other, and the argument to -tags is space separated, so -tags can't easily be used via GOFLAGS today except for the absolute simplest cases.

Respectfully, I would request that this be reopened and reconsidered, especially since the primary use case where this feature got me excited (building the same binary for a load of different architectures in a row) aren't actually helped by this feature today. ❤️ 😅

(I commented on the CL and was redirected here. ❤️)

@ianlancetaylor
Copy link
Contributor

@rsc I'm not sure I understand the argument for not permitting quoting in the environment variable given the special support we have for quoting in -gcflags and friends. It seems to me that there are clear uses for passing multiple options to all builds, such as the -l -N that delve uses.

@andybons
Copy link
Member

andybons commented Dec 5, 2018

Ping @rsc @bcmills.

@rsc
Copy link
Contributor

rsc commented Dec 6, 2018

Leaving for Go 1.13. GOFLAGS is relatively new and the workaround is to use the command line, like you had to do before Go 1.11.

@rsc rsc modified the milestones: Go1.12, Go1.13 Dec 6, 2018
@myitcv
Copy link
Member

myitcv commented Jan 15, 2019

In case we do loosen this constraint on GOFLAGS, I think we will also need to add support for accepting multiple -tags settings.

At the moment the last -tags setting "wins":

$ GOFLAGS="-tags=banana" go list -tags apple -f "{{with context}}{{.BuildTags}}{{end}}" runtime
[apple]

Given my use-case, this is relatively easy to work around by parsing the space-separated GOFLAGS for -tags=-prefixed strings and adding the results to the last command line flag.

But if we allow escape quoted strings in GOFLAGS, this parsing becomes more difficult.

I suspect allowing multiple instances of -tags would be useful in any case? I can always spin off another proposal/CL if needs be.

@mvdan
Copy link
Member

mvdan commented Jan 15, 2019

I suspect allowing multiple instances of -tags would be useful in any case?

Also note that the per-package flags like -ldflags also overwrite each other, but not always. For example, in -ldflags=-w -ldflags=-s the second wins, but in -ldflags=foo=-w -ldflags=bar=-s both apply (to different packages). In that flag, it makes sense as it allows to override a previous per-package flag instead of only appending to it.

Not saying that I disagree with allowing multiple -tags flags; I actually think what you're trying to do is reasonable. My point is that I think we should fix the GOFLAGS interaction with all these list-like flags in a consistent way.

@myitcv
Copy link
Member

myitcv commented Feb 4, 2019

Re-reading what I wrote in #26849 (comment) I realise it wasn't that clear.

At the moment, because GOFLAGS is:

A space-separated list of -flag=value settings

it can't contain a multi-value -tags flag value because -tags is:

a space-separated list of build tags to consider satisfied during the build

Add to that, cmd/go does not understand multiple -tags values; the last one wins.

Hence at the moment, it is not possible to specify a multi-value -tags via GOFLAGS. This becomes problematic in situations like #27898 (comment) where the GOFLAGS variable could otherwise be used as a means of passing build tags in the environment go generate sets for each directive it runs.

For my specific case there are at least two possible non-mutually exclusive solutions:

  • allowing quoted strings in GOFLAGS (this proposal)
  • having cmd/go accept -tags multiple times, i.e. space-joining the multiple -tags values

It is for this second option that I can create a separate issue if required.

@ianthehat
Copy link

Another alternative that would only work for -tags would be to accept comma separated tags. This feels more natural anyway, and would match the syntax used in the +build lines anyway.
This would be very helpful for people needing to set multiple tags and have them obeyed by tools invoked by their editors.

@gopherbot
Copy link

Change https://golang.org/cl/173438 mentions this issue: cmd/go: change -tags to a comma-separated list

@mvdan
Copy link
Member

mvdan commented Apr 23, 2019

Funnily enough, I thought that was how -tags worked years ago. That is why I filed #18800 and mailed a change for cmd/go to give a more obvious error. So I agree with @ianthehat; I'm not sure why that idea hadn't popped up again since 2017.

gopherbot pushed a commit that referenced this issue Apr 24, 2019
Using commas makes it possible to put multiple tags into GOFLAGS.
The space-separated form is still recognized and will be maintained.

Alleviates #26849 somewhat.
Fixes #18800 (again).

Change-Id: I6f4cf28ea31e53e21ccbdad6ef1a0aee63b007d7
Reviewed-on: https://go-review.googlesource.com/c/go/+/173438
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
jwgcarlson added a commit to lanikai/alohartc that referenced this issue Aug 27, 2019
* Stub out MP4 loader for production builds.
* Change alohacam `Makefile` to use a `buildcmd` function instead of a
  constant `BUILDCMD`. This is necessary because we want to specify
  *two* build tags (`production` and `v4l2`), but `GOFLAGS` does not
  allow setting multiple tags. (This should be fixed in 1.13, which is
  imminent. See golang/go#26849.)
thinkski pushed a commit to lanikai/alohartc that referenced this issue Sep 5, 2019
* Stub out MP4 loader for production builds.
* Change alohacam `Makefile` to use a `buildcmd` function instead of a
  constant `BUILDCMD`. This is necessary because we want to specify
  *two* build tags (`production` and `v4l2`), but `GOFLAGS` does not
  allow setting multiple tags. (This should be fixed in 1.13, which is
  imminent. See golang/go#26849.)
@mvdan
Copy link
Member

mvdan commented Apr 18, 2020

@stapelberg I think you raise a valid point, and I've thought about it as well, but I think that should be filed as a separate issue. That is, when cmd/go merges GOFLAGS with command-line flags, the behavior is simply that the flags are concatenated and then interpreted. So, any flags that override previous ones (like -tags, as per @myitcv above, or your example with -ldflags) aren't very usable with GOFLAGS.

@myitcv already mentioned raising this issue separately in #26849 (comment) for -tags. I think one such issue should be filed for all these flags at once, explaining the unfortunate interaction with GOFLAGS.

@mvdan
Copy link
Member

mvdan commented Apr 18, 2020

I didn't get much of a response on #26849 (comment) aside form a couple of reactions, so I didn't feel like there was enough consensus to try something out for 1.14.

This 1.15 cycle is being weird, but I'd like to give this a try in the two remaining weeks.

@bcmills do you still prefer adding simple quoting support to GOFLAGS?

@ianthehat do you still prefer adding support for flags like -ldflags to take commas, just like we did for -tags?

I used to prefer the second option, but I'm starting to think that simple quoting support is a better long-term fix. Commas are really only a short-term fix, don't support nesting of list parameters which also use commas, and IMO make complex list flags like -ldflags harder to read.

So, I now propose that we add support for two quoting syntaxes on all platforms equally:

  • single quotes, which end at the next single quote. no support for any form of escaping. very similar to single quotes in POSIX shell, and to raw string literals in Go.
  • double quotes, with Go syntax semantics. That is, they can contain nested double quotes like GOFLAGS=-ldflags="-foo=\"bar baz\"".

These should be fairly easy to implement. For example, the second would reuse https://golang.org/pkg/strconv/#Unquote for unquoting. If this generally seems like a good idea, I can send a CL this coming week and we can give it a try during the freeze.

@stapelberg
Copy link
Contributor

I think one such issue should be filed for all these flags at once, explaining the unfortunate interaction with GOFLAGS.

Filed #38522. Please amend as you see fit.

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/
@danderson
Copy link
Contributor

danderson commented Sep 25, 2021

Just hit this while trying to make a go wrapper that automatically injects arcane linker flags and other settings, so that we can use go build ./..., go test ./... and so forth without having to indirect through a differently-shaped tool to turn those invocations into arcane go invocations. AFAICT, GOFLAGS looks enticingly useful, but is actually unusable for any non-trivial addition of flags.

Specific problems encountered:

  • Lack of escaping, combined with the "rightmost wins" behavior of most Go tool flags, means I cannot specify multiple ldflags or extldflags.
  • Lack of escaping means I sometimes cannot even specify a single ldflag or extldflag, because some of those flags must be in two-word form, in violation of GOFLAG's naive parser. For example, -ldflags=-Xfoo=bar is rejected by the linker (interpreted as unknown flag -Xfoo), whereas -ldflags=-X foo=bar is rejected by the GOFLAGS parser.
    • This bites in particular when forced to interface with C toolchains, because they routinely forbid the -k=v form and instead require -k v - which cannot be expressed at all in GOFLAGS. For example, -isysroot on macOS disallows the = form.
  • Falling back to specifying things on the commandline is painful, because you lose GOFLAGS's behavior that flags are passed to any command that understands them, and otherwise are gracefully ignored. This means a wrapper tool must now know which flags are applicable where, and partially reimplement go's flag parsing to determine which set of flags are applicable.
  • As a further complication on the previous point, flags are position-sensitive in the Go command, so even if I determine that I need to pass -ldflags, I still need to carefully parse the user-provided command and injects -ldflags at the correct position. I cannot inject them unconditionally (some commands don't understand ldflags), at the beginning (-ldflags not allowed before the words build, which may also not be $1), or at the end (if the command being executed is go run, that'll pass -ldflags to the built binary, rather than use it during the build.

My conclusion so far is that making Go more pleasant to use in non-trivial build environments is not possible with GOFLAGS, and painfully difficult without. Neither is a happy outcome, and makes it harder to use a mostly-plain go driven development process in more complex codebases.

EDIT: sorry, minor correction. Looking at the source code of the go tool, there seem to be no "global" flags, so it appears currently safe to assume that $1 of a go invocation will be the desired subcommand. This makes it a little easier to implement a wrapper that injects "defaults" for go invocations.

half-duplex added a commit to half-duplex/aur-teleirc that referenced this issue Apr 26, 2022
This requires moving ldflags from goflags to the cmdline because of
golang/go#26849
half-duplex added a commit to half-duplex/aur-teleirc that referenced this issue Apr 26, 2022
This requires moving ldflags from goflags to the cmdline because of
golang/go#26849
@tacerus
Copy link

tacerus commented Aug 20, 2022

This is interesting as some Golang RPM packaging macros rely on the packager being able to use $GOFLAGS to add additional, software specific, build options. If something like export GOFLAGS="-mod=vendor -ldflags='-Xmain.commit=12345'" is not possible, we might need to append some custom variables, such as $LDFLAGS, to the go build and go install commands inside the macro.

@rsc
Copy link
Contributor

rsc commented Sep 8, 2022

export GOFLAGS="-mod=vendor -ldflags='-Xmain.commit=12345'"

For what it's worth, this isn't the right syntax. It should be

export GOFLAGS="-mod=vendor -ldflags=-X=main.commit=12345"

which doesn't require any additional quoting (also -mod=vendor has been implied by the presence of a vendor directory since Go 1.14). Of course, if you want two -X flags, now you need more quoting.

The hard part about all this is that there's no standard definition of quoting, nor how you escape inner quotes, and so on. It was much easier to say don't do that.

Unfortunately, this does seem to keep coming up, so maybe we need to make a change. In 2018 I wrote

we don't do any special quoting for the other environment variables

CL 341936 changed that. It would make sense to do the same for $GOFLAGS.

The specific definition of quoting we now use in other variables should be completely backwards compatible for use in $GOFLAGS: the quotes must go around the entire thing, not appear mid-word. That means for $GOFLAGS they must go around the -name=value, and to date any field not beginning with - would have been rejected. So no extant valid $GOFLAGS settings would change meaning.

@mvdan
Copy link
Member

mvdan commented Sep 8, 2022

Thinking aloud, it would be nice if the quoting was somewhat consistent between GOFLAGS and existing flags with support for quotes, like -ldflags. What you say about "not appear mid-word" seems in line with the existing docs, as it suggests surrounding an entire element in a space-separated list with quotes:

To embed spaces in an element in the list, surround it with either single or double quotes.

Then, one could imagine two levels of quoting as an extreme example:

export GOFLAGS="-mod=vendor '-ldflags=-X=main.commit=12345 '-X=main.hello=hello world' ' "

Though I admit that, even with the added spaces for readability, I still struggle a bit to follow what's going on.

An alternative that doesn't mean escaping quote characters (akin to POSIX shell or Go's double-quoted string literals) would be for GOFLAGS to support a multi-line form, where each line with any non-whitespace character is a Go flag. The example above could then be:

export GOFLAGS="
-mod=vendor
-ldflags=-X=main.commit=12345 '-X=main.hello=hello world'
"

We remove the need for the nested quotes, at least. Multi-line strings in interactive shells aren't super pretty, but I imagine such nested quoting scenarios will most commonly happen in moderately complex shell scripts or programs calling cmd/go, in which case the multi-line strings are generally not a problem at all - and more readable.

@rsc
Copy link
Contributor

rsc commented Sep 9, 2022

I suggested exactly that multiline approach to Bryan just before posting what I did. 😄
I like it, but it consistency with all the other environment variables is more compelling, even if it's not as clean.

The 'two levels of quoting' example does not behave as written in your post, because quotes can't nest (you can't tell left from right). If you want to pass two different ldflags, you have to use different quotes, as in (this is the actual argv value):

-ldflags=-X=main.commit=12345 '-X=main.hello=hello world'

and then to get that into GOFLAGS you have to put (different) quotes around it as in

GOFLAGS="-ldflags=-X=main.commit=12345 '-X=main.hello=hello world'"

or you can swap the quotes:

GOFLAGS='-ldflags=-X=main.commit=12345 "-X=main.hello=hello world"'

To get this into a shell you have to introduce a third level of quoting; in sh you use " so that you can escape things inside:

export GOFLAGS="'-ldflags=-X=main.commit=12345 \"-X=main.hello=hello world\"'"

All this complexity is exactly why I said "Sorry, but no" in 2018. But the giant improv scene that is software engineering plays on, and other players sent us down this path. Now the right thing to do is almost certainly to yes and ..., meaning do what makes the most sense given those earlier decisions, not try to back them out.

For the many parts of the toolchain that now accept possibly quoted space-separated argument lists, there is one consistent rule about how you add quotes. I think it serves us much better to keep to that convention, restoring the 2018 invariant that all environment variables are handled the same way, rather than introduce a separate convention just for GOFLAGS.

@rsc
Copy link
Contributor

rsc commented Sep 9, 2022

Also go env -w disallows newlines in values:

% go env -w GOFLAGS='a
	b'
go: invalid newline in GOFLAGS=... value
% 

@gopherbot
Copy link

Change https://go.dev/cl/443956 mentions this issue: cmd/go: split quotes in GOFLAGS same as in other env vars

@mvdan
Copy link
Member

mvdan commented Oct 26, 2022

Yeah, using newlines is a bit awkward, but the bigger problem is that it clashes with the go env -w file. What you did here sounds good.

romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Nov 3, 2022
GOFLAGS didn't split on quotes because no other env vars
(such as CC, CXX, ...) did either. This kept them all consistent.

CL 341936 changed everything but GOFLAGS, making them inconsistent.

Split GOFLAGS the same way as the other environment variables.

Fixes golang#26849.

Change-Id: I99bb450fe30cab949da48af133b6a36ff320532f
Reviewed-on: https://go-review.googlesource.com/c/go/+/443956
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
@golang golang locked and limited conversation to collaborators Oct 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests