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

go/build: commas in build tag lists should error #18800

Closed
mvdan opened this issue Jan 26, 2017 · 19 comments
Closed

go/build: commas in build tag lists should error #18800

mvdan opened this issue Jan 26, 2017 · 19 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Jan 26, 2017

I just realised that multiple build tags are separated by spaces, not commas. I had been doing go install -tags 'foo,bar' instead of go install -tags 'foo bar' for days without noticing, because the former meant a single build tag that did nothing at all.

Two other people in my team went through the very same issue weeks ago when learning build tags, so surely it's not just my misunderstanding.

I suggest that we either make the docs clearer about this (edit: done by @ALTree below), or that we make commas illegal in build tags so that -tags 'foo,bar' would error instead of silently ignoring both tags.

@cznic
Copy link
Contributor

cznic commented Jan 26, 2017

It seems to be documented properly.

$ go help build
usage: go build [-o output] [-i] [build flags] [packages]
...
	-tags 'tag list'
		a list of build tags to consider satisfied during the build.
		For more information about build tags, see the description of
		build constraints in the documentation for the go/build package.
...
$ 

Note, it's 'tag list', not 'tag,list'.

@mvdan
Copy link
Member Author

mvdan commented Jan 26, 2017

I realise that - but even though we all read that line, we still used commas. Note that lists are usually comma-separated, and 'tag list' doesn't particularly say to me that tag and list are elements. It seems more like a description.

@ianlancetaylor ianlancetaylor added this to the Go1.9 milestone Jan 26, 2017
@josharian
Copy link
Contributor

Does vet complain about this? If not, perhaps it should. There is already a build tags check.

@mvdan
Copy link
Member Author

mvdan commented Jan 26, 2017

@josharian what go vet command, exactly? I can't get it to complain.

Given the documentation tag added, I assume being more explicit in the docs is preferred. Either of the two fixes sounds fine by me, but a check would be nice.

@cznic
Copy link
Contributor

cznic commented Jan 26, 2017

Does vet complain about this? If not, perhaps it should. There is already a build tags check.

@josharian This is, I believe, about a command option tag list, not about source code.

It seems more like a description.

@mvdan Well, if the documentation shows a description/descriptive example, I wonder what the documentation is missing.

@mvdan
Copy link
Member Author

mvdan commented Jan 26, 2017

What I said above is that I don't see the help line as an example. Sure it's a description, but definitely not an example since tag and list are not obvious build tag names.

@ALTree
Copy link
Member

ALTree commented Jan 26, 2017

I agree with mvdan, what I understand when I read -tags 'tag list' is that tags takes the usual comma-separated list. I could expect the command to also accept space-separated values, but I would be surprised to see that a comma-separated list is actually rejected.

Slightly better would be: -tags 'tag1 tag2'.

Even better: clarify it using English words in the following description: "... accepts a space separated list of tags ..."

@mvdan
Copy link
Member Author

mvdan commented Jan 26, 2017

@ALTree thanks for the suggestions, those sound more self-explanatory.

@josharian
Copy link
Contributor

@josharian This is, I believe, about a command option tag list, not about source code.

Haha, indeed. Given that no build tag could ever include a comma, maybe cmd/go could fail with an error when given a tags flag containing a comma. We should fix the docs too, of course.

@gopherbot
Copy link

CL https://golang.org/cl/35951 mentions this issue.

@mvdan
Copy link
Member Author

mvdan commented Jan 28, 2017

Even though I'm happy with the doc fix CL above, I'd still like to see the go tools error if commas appear in space-separated lists. @josharian said they aren't allowed as tags, so at least for that option it makes sense.

@ALTree
Copy link
Member

ALTree commented Jan 28, 2017

@mvdan sure I'll change the CL from fixes to updates and let you decide when this is fixed.

I think you'll have to maybe change the issue title (and remove the documentation label) though, because now we're talking about a more invasive (code) change.

@mvdan
Copy link
Member Author

mvdan commented Jan 28, 2017

@ALTree I didn't add the error directly in the title because I wasn't sure it would be possible (maybe commas were valid parts of a build tag), and because I thought the documentation bit was more important to fix first.

Since you're talking care of that, I'll update the issue. Thanks!

@mvdan mvdan changed the title go/build: not clear that build tags are space separated go/build: commas in build tags should error Jan 28, 2017
@mvdan mvdan changed the title go/build: commas in build tags should error go/build: commas in build tag lists should error Jan 28, 2017
gopherbot pushed a commit that referenced this issue Feb 8, 2017
Apparently the current documentation is confusing users that
quickly skim the flags list at the top. Make very clear that
build tags are space-separated.

Updates #18800

Change-Id: I473552c5a2b70ca03d8bbbd2c76805f7f82b49a2
Reviewed-on: https://go-review.googlesource.com/35951
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Minux Ma <minux@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 7, 2017
@mvdan
Copy link
Member Author

mvdan commented Apr 26, 2017

Does anyone disagree with commas in build tags erroring? If noone speaks up in the next week, I'll work on a patch. Not sure if this is soon enough to get into 1.9, though.

@mvdan mvdan self-assigned this Apr 26, 2017
@cznic
Copy link
Contributor

cznic commented Apr 26, 2017

Would it harm anything if strings.Replace(",", " ", -1) is just performed on the "list" first?

@mvdan
Copy link
Member Author

mvdan commented Apr 26, 2017

If the 'go' help says it accepts a space-separated list, I would prefer that it error on any commas rather than silently converting it to spaces. Otherwise the help should be changed to "space and/or comma separated", which I would find unnecessarily confusing.

@josharian
Copy link
Contributor

Not sure if this is soon enough to get into 1.9, though.

If you mail the CL before May 1, it can at least be considered for 1.9

If the 'go' help says it accepts a space-separated list, I would prefer that it error on any commas rather than silently converting it to spaces.

I concur.

@mvdan
Copy link
Member Author

mvdan commented Apr 26, 2017

If you mail the CL before May 1, it can at least be considered for 1.9

Thanks - will try to get a patch ready by tomorrow then. Anyone strongly against it can speak up there instead.

@gopherbot
Copy link

CL https://golang.org/cl/41951 mentions this issue.

@golang golang locked and limited conversation to collaborators Apr 28, 2018
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>
@rsc rsc unassigned mvdan Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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

6 participants