code review 222890043: go/cmd/go: always link external test packages first

go/cmd/go: always link external test packages first

When linking complex packages that use both internal and external tests as well as many dependencies it is critical that the link order be external test package, internal test package, everything else.

This change is a back (forward?) port of the same change that canonical have been maintaining on their fork of the go tool for gccgo. Now that gccgo uses the go tool from upstream, this patch should be applied both to the gofrontend and golang/go repos.

[dave_cheney.net, 2015-03-30 04:30:26]

Hello iant@golang.org, michael.hudson@canonical.com (cc:
gofrontend-dev@googlegroups.com),

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

[dave_cheney.net, 2015-03-30 04:31:59]

The reproduction for this issue is quite involved as you need to create a very
large package with intricate dependency interweaving.

A simple repo, which failed before this patch is

% /opt/gccgo/bin/go get github.com/juju/juju/...
% /opt/gccgo/bin/go test github.com/juju/juju/.../reboot

At least one of these packages would fail to link during testing.

[minux, 2015-03-30 04:34:02]

I think this needs to be applied to gc repository first.

[dave_cheney.net, 2015-03-30 04:44:20]

Sure, i'll do that now.

On Mon, Mar 30, 2015 at 3:33 PM, minux <minux@golang.org> wrote:
> I think this needs to be applied to gc repository first.

[dave_cheney.net, 2015-03-30 04:51:00]

On 2015/03/30 04:44:20, dfc wrote:
> Sure, i'll do that now.
> 
> On Mon, Mar 30, 2015 at 3:33 PM, minux <mailto:minux@golang.org> wrote:
> > I think this needs to be applied to gc repository first.

Actually, no I cannot do that. 

libgo/go/cmd/go is based on an older revision of the Go tool, this patch will
not apply and the version in golang/go is not current with gccgo's version.

[dave_cheney.net, 2015-03-30 05:08:32]

On 2015/03/30 04:51:00, dfc wrote:
> On 2015/03/30 04:44:20, dfc wrote:
> > Sure, i'll do that now.
> > 
> > On Mon, Mar 30, 2015 at 3:33 PM, minux <mailto:minux@golang.org> wrote:
> > > I think this needs to be applied to gc repository first.
> 
> Actually, no I cannot do that. 
> 
> libgo/go/cmd/go is based on an older revision of the Go tool, this patch will
> not apply and the version in golang/go is not current with gccgo's version.

I'm going to propose a patch on golang/go that treats gccgo's version + this
patch as canonical. This seems right as gccgo's version is the one that will be
used with the gccgoToolchain the most

[iant, 2015-03-30 18:06:26]

https://codereview.appspot.com/222890043/diff/40001/libgo/go/cmd/go/build.go
File libgo/go/cmd/go/build.go (right):

https://codereview.appspot.com/222890043/diff/40001/libgo/go/cmd/go/build.go#...
libgo/go/cmd/go/build.go:1944: xfiles = append([]string{a.target}, xfiles...)
For afiles, we needed to put the _test files first.  Here we don't.  This can be
    xfiles = append(xfiles, a.target)

[dave_cheney.net, 2015-03-31 08:30:44]

Hello iant@golang.org, michael.hudson@canonical.com, minux@golang.org (cc:
gofrontend-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2015-03-31 08:35:31]

https://codereview.appspot.com/222890043/diff/40001/libgo/go/cmd/go/build.go
File libgo/go/cmd/go/build.go (right):

https://codereview.appspot.com/222890043/diff/40001/libgo/go/cmd/go/build.go#...
libgo/go/cmd/go/build.go:1944: xfiles = append([]string{a.target}, xfiles...)
On 2015/03/30 18:06:25, iant wrote:
> For afiles, we needed to put the _test files first.  Here we don't.  This can
be
>     xfiles = append(xfiles, a.target)

Done.

[iant, 2015-03-31 17:51:40]

LGTM

[dave_cheney.net, 2015-03-31 17:53:32]

Ian, will you merge this into gofrontend and possible gcc/trunk ?

On Wed, Apr 1, 2015 at 4:51 AM,  <iant@golang.org> wrote:
> LGTM
>
> https://codereview.appspot.com/222890043/

[iant, 2015-03-31 17:53:47]

*** Submitted as
https://code.google.com/p/gofrontend/source/detail?r=15a24202fa42 ***

go/cmd/go: always link external test packages first

When linking complex packages that use both internal and external tests as well
as many dependencies it is critical that the link order be external test
package, internal test package, everything else.

This change is a back (forward?) port of the same change that canonical have
been maintaining on their fork of the go tool for gccgo. Now that gccgo uses the
go tool from upstream, this patch should be applied both to the gofrontend and
golang/go repos.

LGTM=iant
R=iant, michael.hudson, minux
CC=gofrontend-dev
https://codereview.appspot.com/222890043

Committer: Ian Lance Taylor <iant@golang.org>

[iant, 2015-03-31 17:57:25]

On Tue, Mar 31, 2015 at 10:53 AM, Dave Cheney <dave@cheney.net> wrote:
> Ian, will you merge this into gofrontend and possible gcc/trunk ?

Yes.

Ian

> On Wed, Apr 1, 2015 at 4:51 AM,  <iant@golang.org> wrote:
>> LGTM
>>
>> https://codereview.appspot.com/222890043/