code review 6458091: go/build: correct shouldBuild bug reading whole content...

go/build: correct shouldBuild bug reading whole contents of file.

It was caused by bytes.TrimSpace being able to return a nil
slice.

Fixes issue 3914.

[remyoudompheng, 2012-08-06 20:19:51]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
remy@archlinux.org),

I'd like you to review this change to
https://go.googlecode.com/hg/

[remyoudompheng, 2012-08-06 20:20:34]

Not using backquote strings in the test is a convenience to avoid the test
skipping itself due to the bug it is testing.

[remyoudompheng, 2012-08-07 22:21:49]

ping

[r, 2012-08-09 20:32:56]

LGTM
although i think this code could use a rewrite, this looks like a correct fix.
independently verified.

[remyoudompheng, 2012-08-09 21:23:06]

*** Submitted as http://code.google.com/p/go/source/detail?r=307fafbc2d6f ***

go/build: correct shouldBuild bug reading whole contents of file.

It was caused by bytes.TrimSpace being able to return a nil
slice.

Fixes issue 3914.

R=golang-dev, r
CC=golang-dev, remy
http://codereview.appspot.com/6458091

[dfc, 2012-08-09 22:38:41]

This commit has broken all the amd64 go.crypto/curve25519 builds.

For some reason the line

// +build !amd64

is no longer working.

http://build.golang.org/log/722cdb774cd6862886c36b2c8a293f326075578d

On Fri, Aug 10, 2012 at 7:23 AM,  <remyoudompheng@gmail.com> wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=307fafbc2d6f ***
>
>
> go/build: correct shouldBuild bug reading whole contents of file.
>
> It was caused by bytes.TrimSpace being able to return a nil
> slice.
>
> Fixes issue 3914.
>
> R=golang-dev, r
> CC=golang-dev, remy
> http://codereview.appspot.com/6458091
>
>
> http://codereview.appspot.com/6458091/

[rsc, 2012-08-09 23:36:00]

On Thu, Aug 9, 2012 at 6:38 PM, Dave Cheney <dave@cheney.net> wrote:
> This commit has broken all the amd64 go.crypto/curve25519 builds.
>
> For some reason the line
>
> // +build !amd64
>
> is no longer working.

The +build line must be in the leading sequence of comments and blank
lines that ends in a blank line.
So the file should say

// Copyright 2012 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

// We have a implementation in amd64 assembly so this code is only run on
// non-amd64 platforms.
// +build !amd64

package curve25519

[dfc, 2012-08-09 23:36:40]

I'll propose a fix now.

On Fri, Aug 10, 2012 at 9:35 AM, Russ Cox <rsc@golang.org> wrote:
> On Thu, Aug 9, 2012 at 6:38 PM, Dave Cheney <dave@cheney.net> wrote:
>> This commit has broken all the amd64 go.crypto/curve25519 builds.
>>
>> For some reason the line
>>
>> // +build !amd64
>>
>> is no longer working.
>
> The +build line must be in the leading sequence of comments and blank
> lines that ends in a blank line.
> So the file should say
>
> // Copyright 2012 The Go Authors. All rights reserved.
> // Use of this source code is governed by a BSD-style
> // license that can be found in the LICENSE file.
>
> // We have a implementation in amd64 assembly so this code is only run on
> // non-amd64 platforms.
> // +build !amd64
>
> package curve25519