code review 75820046: cmd/go: let build -x output describe CGO_LDFLAGS env va...

cmd/go: let build -x output describe CGO_LDFLAGS env variable

Fixes issue 7249

[atom, 2014-03-14 14:49:25]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[atom, 2014-03-16 09:25:22]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[iant, 2014-03-18 18:45:28]

https://codereview.appspot.com/75820046/diff/70001/src/cmd/go/build.go
File src/cmd/go/build.go (right):

https://codereview.appspot.com/75820046/diff/70001/src/cmd/go/build.go#newcod...
src/cmd/go/build.go:1456: value = strings.Replace(value, "\"", "", -1)
I guess I don't understand why you are removing double-quote characters.  At
this point they are part of the value that will be passed in the environment, so
it seems to me that we ought to display them.  For that matter, I don't see why
you call TrimSpace either.

https://codereview.appspot.com/75820046/diff/70001/src/cmd/go/build.go#newcod...
src/cmd/go/build.go:1458: value = "\"" + value + "\""
Use raw strings rather than backslash quoting.
    value = `"` + value + `"`

[atom, 2014-03-18 19:28:30]

https://codereview.appspot.com/75820046/diff/70001/src/cmd/go/build.go
File src/cmd/go/build.go (right):

https://codereview.appspot.com/75820046/diff/70001/src/cmd/go/build.go#newcod...
src/cmd/go/build.go:1456: value = strings.Replace(value, "\"", "", -1)
On 2014/03/18 18:45:29, iant wrote:
> I guess I don't understand why you are removing double-quote characters.  At
> this point they are part of the value that will be passed in the environment,
so
> it seems to me that we ought to display them.  For that matter, I don't see
why
> you call TrimSpace either.

If CGO_LDFLAGS is `"X Y Z"` the value will be `"X" "Y" "Z"`. cmd/go somewhere
changes the single string into 3 strings. If the latter isn't an issue I can
remove the envFormat function.

I removed TrimSpace.

https://codereview.appspot.com/75820046/diff/70001/src/cmd/go/build.go#newcod...
src/cmd/go/build.go:1458: value = "\"" + value + "\""
On 2014/03/18 18:45:29, iant wrote:
> Use raw strings rather than backslash quoting.
>     value = `"` + value + `"`

Done.

[iant, 2014-03-18 20:01:14]

On 2014/03/18 19:28:30, atom wrote:
> https://codereview.appspot.com/75820046/diff/70001/src/cmd/go/build.go
> File src/cmd/go/build.go (right):
> 
>
https://codereview.appspot.com/75820046/diff/70001/src/cmd/go/build.go#newcod...
> src/cmd/go/build.go:1456: value = strings.Replace(value, "\"", "", -1)
> On 2014/03/18 18:45:29, iant wrote:
> > I guess I don't understand why you are removing double-quote characters.  At
> > this point they are part of the value that will be passed in the
environment,
> so
> > it seems to me that we ought to display them.  For that matter, I don't see
> why
> > you call TrimSpace either.
> 
> If CGO_LDFLAGS is `"X Y Z"` the value will be `"X" "Y" "Z"`. cmd/go somewhere
> changes the single string into 3 strings. If the latter isn't an issue I can
> remove the envFormat function.

As far as I can see, at this point in the code, any quote characters in value
will wind up in the environment variable.  I added a
fmt.Println(os.Getenv("CGO_LDFLAGS")) to the start of cgo, and when I set
CGO_LDFLAGS="X Y Z" it prints "\"X" "Y" "Z\"" "-lpthread".  So I think the
quotes are real, and they should be printed here.

Ian

[atom, 2014-03-18 20:09:38]

I removed the envFormat function. Please take another look.

[iant, 2014-03-18 20:45:15]

LGTM

Thanks.

[atom, 2014-03-18 20:47:36]

*** Submitted as https://code.google.com/p/go/source/detail?r=b05a4dcfc73a ***

cmd/go: let build -x output describe CGO_LDFLAGS env variable

Fixes issue 7249

LGTM=iant
R=golang-codereviews, iant
CC=golang-codereviews
https://codereview.appspot.com/75820046

[gobot, 2014-03-18 21:08:06]

This CL appears to have broken the openbsd-386-rootbsd builder.
See http://build.golang.org/log/a023989f1fc4572ac820018a24b226b719e3d338