go/build: commas in build tag lists should error

[mvdan]

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]

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]

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.

[josharian]

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

[mvdan]

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

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]

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]

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]

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

[josharian]

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

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

[mvdan]

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]

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

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

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.

[cznic]

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

[mvdan]

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]

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]

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]

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