code review 5832043: goprotobuf: Use local imports for testdata package. (issue 5832043)

[dsym...@golang.org]

Reviewers: r,

Message:
Hello r (cc: golan...@googlegroups.com),

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

Description:
goprotobuf: Use local imports for testdata package.

Please review this at http://codereview.appspot.com/5832043/

Affected files:
M proto/all_test.go
M proto/clone_test.go
M proto/equal_test.go
M proto/text_parser_test.go
M proto/text_test.go

Index: proto/all_test.go
===================================================================
--- a/proto/all_test.go
+++ b/proto/all_test.go
@@ -43,8 +43,8 @@
"testing"
"time"

+ . "./testdata"
. "code.google.com/p/goprotobuf/proto"
- . "code.google.com/p/goprotobuf/proto/testdata"
)

var globalO *Buffer
Index: proto/clone_test.go
===================================================================
--- a/proto/clone_test.go
+++ b/proto/clone_test.go
@@ -37,7 +37,7 @@

"code.google.com/p/goprotobuf/proto"

- pb "code.google.com/p/goprotobuf/proto/testdata"
+ pb "./testdata"
)

var cloneTestMessage = &pb.MyMessage{
Index: proto/equal_test.go
===================================================================
--- a/proto/equal_test.go
+++ b/proto/equal_test.go
@@ -35,8 +35,8 @@
"log"
"testing"

+ pb "./testdata"
. "code.google.com/p/goprotobuf/proto"
- pb "code.google.com/p/goprotobuf/proto/testdata"
)

// Four identical base messages.
Index: proto/text_parser_test.go
===================================================================
--- a/proto/text_parser_test.go
+++ b/proto/text_parser_test.go
@@ -35,8 +35,8 @@
"reflect"
"testing"

+ . "./testdata"
. "code.google.com/p/goprotobuf/proto"
- . "code.google.com/p/goprotobuf/proto/testdata"
)

type UnmarshalTextTest struct {
Index: proto/text_test.go
===================================================================
--- a/proto/text_test.go
+++ b/proto/text_test.go
@@ -38,7 +38,7 @@

"code.google.com/p/goprotobuf/proto"

- pb "code.google.com/p/goprotobuf/proto/testdata"
+ pb "./testdata"
)

func newTestMessage() *pb.MyMessage {

[dsym...@golang.org]

*** Submitted as
http://code.google.com/p/goprotobuf/source/detail?r=07a39af1fd9f ***

goprotobuf: Use local imports for testdata package.

TBR=r
CC=golang-dev
http://codereview.appspot.com/5832043

http://codereview.appspot.com/5832043/

[Rob 'Commander' Pike]

LGTM

[Russ Cox]

LGTM

Nice use of local imports.

[Albert Strasheim]

Hello

On Thursday, March 15, 2012 6:10:30 AM UTC+2, David Symonds wrote:

*** Submitted as
http://code.google.com/p/goprotobuf/source/detail?r=07a39af1fd9f ***

goprotobuf: Use local imports for testdata package.

Sadly, this breaks the build on gccgo.

Is the plan to support local imports in gccgo for Go 1?

Regards

Albert 

[Ian Lance Taylor]

Albert Strasheim <ful...@gmail.com> writes:

> Sadly, this breaks the build on gccgo.
>
> Is the plan to support local imports in gccgo for Go 1?

I didn't know they were broken. What fails?

Ian

[Albert Strasheim]

I made a project with this layout:

src/code.google.com/p/go.crypto
src/code.google.com/p/go.net
src/code.google.com/p/go.image
src/code.google.com/p/goprotobuf

rm -rf src/code.google.com/p/go.crypto/curve25519

so avoid the assembler that doesn't build.

GOPATH=`pwd` go test -compiler=gccgo -v ./...
# code.google.com/p/goprotobuf/proto_test
src/code.google.com/p/goprotobuf/proto/all_test.go:46:2: error: import
file ‘./testdata’ not found
src/code.google.com/p/goprotobuf/proto/clone_test.go:40:2: error:
import file ‘./testdata’ not found

etc.

Works fine with normal go test.

gccgo (GCC) 4.8.0 20120316 (experimental)

go version weekly.2012-03-13 +a303acb0a5f2

I assumed it was a gccgo issue, but maybe it's a go issue.

Regards

Albert

[Russ Cox]

I changed the gc compilers recently to accept an option
-D rel (it doesn't stand for anything, I am running out of letters)
that makes the compiler interpret local imports relative to the
path rel. For example, 6g -D a/b/c seeing import "../file" will
treat the import as a/b/file. The go command uses this to resolve
relative imports in a position-independent way. Gccgo will
need similar functionality and then the go command needs to
invoke it. The gc change is http://codereview.appspot.com/5732045.

I do not expect the go command to work completely with gccgo
when Go 1 is launched. We're doing things too last minute and
these are substantial changes. It should work for a common
subset, though, like pure Go packages (no assembly or C)
that do not use local imports.

Russ

[Rob 'Commander' Pike]

On 17/03/2012, at 12:34 AM, Albert Strasheim wrote:

Hello

On Thursday, March 15, 2012 6:10:30 AM UTC+2, David Symonds wrote:

*** Submitted as
http://code.google.com/p/goprotobuf/source/detail?r=07a39af1fd9f ***

goprotobuf: Use local imports for testdata package.

Sadly, this breaks the build on gccgo.

Does it break the build?  Tests I can believe but surely the build is OK.

-rob