code review 147690043: go/build: do not consider "android.go" to be android-sp...

go/build: do not consider "android.go" to be android-specific
A file name must have a non-empty underscore-separated
prefix before its suffix matches GOOS. This is what the
documentation already said but is not what the code did.

Fixes issue 8838.

This needs to be called out in the release notes.
The he single affected file
        code.google.com/p/go.text/collate/tools/colcmp/darwin.go
could use a renaming but works because it has a build tag inside.

[r, 2014-10-06 21:45:52]

Hello golang-codereviews@googlegroups.com,

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

[adg, 2014-10-06 21:48:25]

LGTM

[rsc, 2014-10-06 21:50:05]

Lgtm

On Monday, October 6, 2014, Andrew Gerrand <adg@golang.org> wrote:

> LGTM
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com
>
<javascript:_e(%7B%7D,'cvml','golang-codereviews%2Bunsubscribe@googlegroups.com');>
> .
> For more options, visit https://groups.google.com/d/optout.
>

[r, 2014-10-06 21:50:37]

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

go/build: do not consider "android.go" to be android-specific
A file name must have a non-empty underscore-separated
prefix before its suffix matches GOOS. This is what the
documentation already said but is not what the code did.

Fixes issue 8838.

This needs to be called out in the release notes.
The he single affected file
        code.google.com/p/go.text/collate/tools/colcmp/darwin.go
could use a renaming but works because it has a build tag inside.

LGTM=adg, rsc
R=golang-codereviews, adg, rsc
CC=golang-codereviews
https://codereview.appspot.com/147690043

[minux, 2014-10-06 22:48:08]

I have two questions regarding this change:
1. the go1.4.txt only talks about GOOS, however, looking at the code, it
seems it equally affects GOARCH. I, for example, have a few packages with
amd64.s (the packages have a small API interface, so it makes sense to just
name the assembly implementation as GOARCH.s)

2. that brings us to the 2nd problem: even the old go/build docs mentions
"windows.go" as an example so I think this change is breaking the Go 1 API.

I do understand this CL aims to not breaking user's code after more
goarches and GOOSes are introduced. But should we also aim to preserve
existing semantics on existing GOOS/GOARCHes? I know this will complicate
things a lot, but it at least conform to our go1 API gurantee.

Thanks.

[adg, 2014-10-06 22:56:51]

On 7 October 2014 09:48, minux <minux@golang.org> wrote:

> I think this change is breaking the Go 1 API.


I think the docs have always said the file should end in _GOOS.go, so it's
really an implementation bug that the behaviour was supported in the first
place.

[rsc, 2014-10-06 23:22:05]

On Mon, Oct 6, 2014 at 6:48 PM, minux <minux@golang.org> wrote:

> I have two questions regarding this change:
> 1. the go1.4.txt only talks about GOOS, however, looking at the code, it
> seems it equally affects GOARCH. I, for example, have a few packages with
> amd64.s (the packages have a small API interface, so it makes sense to just
> name the assembly implementation as GOARCH.s)
>
> 2. that brings us to the 2nd problem: even the old go/build docs mentions
> "windows.go" as an example so I think this change is breaking the Go 1 API.
>
> I do understand this CL aims to not breaking user's code after more
> goarches and GOOSes are introduced. But should we also aim to preserve
> existing semantics on existing GOOS/GOARCHes? I know this will complicate
> things a lot, but it at least conform to our go1 API gurantee.
>
There is no guarantee for the tools, only for the language. This change
will break some packages, and we will document that, but the fix is trivial
and it makes it far easier to future-proof against new GOOS/GOARCH that
come along.

[gobot, 2014-10-07 01:35:47]

This CL appears to have broken the linux-amd64-noopt builder.
See http://build.golang.org/log/3d004d5a73050609b5df66cc047bc484b69bca77

[r, 2014-10-07 02:13:44]

The docs mentioned the old behavior as a special case. They do not now.

Yes it will break stuff, but the fix is easy and it is a tool change,
not a language change.

-rob


On Mon, Oct 6, 2014 at 6:35 PM,  <gobot@golang.org> wrote:
> This CL appears to have broken the linux-amd64-noopt builder.
> See http://build.golang.org/log/3d004d5a73050609b5df66cc047bc484b69bca77
>
> https://codereview.appspot.com/147690043/

[gobot, 2014-10-07 04:28:04]

This changed caused perf changes on windows-amd64-perf:


garbage-4                 old          new      delta
sys-stack              131072        98304     -25.00

garbage-8                 old          new      delta
sys-stack              163840       131072     -20.00

garbage-16                old          new      delta
sys-stack              131072        98304     -25.00

http://build.golang.org/perfdetail?commit=222ddd760fc1d40aba7a9e77c51ff388d97...



—gobot

[dave_cheney.net, 2014-10-07 04:31:12]

This sounds improbable. 



> On 7 Oct 2014, at 15:28, gobot@golang.org wrote:
> 
> This changed caused perf changes on windows-amd64-perf:
> 
> 
> garbage-4                 old          new      delta
> sys-stack              131072        98304     -25.00
> 
> garbage-8                 old          new      delta
> sys-stack              163840       131072     -20.00
> 
> garbage-16                old          new      delta
> sys-stack              131072        98304     -25.00
> 
>
http://build.golang.org/perfdetail?commit=222ddd760fc1d40aba7a9e77c51ff388d97...
> 
> 
> 
> --gobot
> 
> 
> https://codereview.appspot.com/147690043/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[shurcooL, 2015-05-10 01:51:18]

There's a typo in the comments:

// sytems, such as android, to arrive without breaking existing code with

"sytems" should be "systems".

On 2014/10/07 04:31:12, dfc wrote:
> This sounds improbable. 
> 
> 
> 
> > On 7 Oct 2014, at 15:28, mailto:gobot@golang.org wrote:
> > 
> > This changed caused perf changes on windows-amd64-perf:
> > 
> > 
> > garbage-4                 old          new      delta
> > sys-stack              131072        98304     -25.00
> > 
> > garbage-8                 old          new      delta
> > sys-stack              163840       131072     -20.00
> > 
> > garbage-16                old          new      delta
> > sys-stack              131072        98304     -25.00
> > 
> >
>
http://build.golang.org/perfdetail?commit=222ddd760fc1d40aba7a9e77c51ff388d97...
> > 
> > 
> > 
> > --gobot
> > 
> > 
> > https://codereview.appspot.com/147690043/
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email to mailto:golang-codereviews+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.