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

Open
bcmills opened this issue Aug 7, 2018 · 13 comments
Assignees
Milestone

Comments

@bcmills
Copy link
Member

@bcmills 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 this to the Go1.11 milestone Aug 7, 2018
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Aug 8, 2018

@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

This comment has been minimized.

Copy link
Contributor

@rsc 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 Aug 9, 2018
@tianon

This comment has been minimized.

Copy link
Contributor

@tianon 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

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 3, 2018

@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

This comment has been minimized.

Copy link
Member

@andybons andybons commented Dec 5, 2018

Ping @rsc @bcmills.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc 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

This comment has been minimized.

Copy link
Member

@myitcv 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

This comment has been minimized.

Copy link
Member

@mvdan 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

This comment has been minimized.

Copy link
Member

@myitcv 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

This comment has been minimized.

Copy link

@ianthehat ianthehat commented Apr 23, 2019

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Apr 23, 2019

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

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan 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
@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Sep 22, 2019

Don't mean to bump the thread, but - is this planned for 1.14, or is it likely to be moved to 1.15 if noone works on it?

If so, I really want a fix for this problem, so I can work on a fix for 1.14. We'd just need to agree on either of these solutions:

  1. Support basic forms of quoting in GOFLAGS, as per @bcmills
  2. Support commas in -ldflags and -gcflags, similar to the recent change in -tags, as per @ianthehat

I personally think option 2 is the most consistent, and probably the simplest. Like with -tags, we can continue to allow spaces as a separator for now, since it can't be part of a valid flag argument anyway.

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.