code review 6623053: go.crypto: various: fix appengine compatibility

go.crypto: various: fix appengine compatibility

Fixes issue 4102.

[dave_cheney.net, 2012-10-07 11:18:39]

Hello russross@gmail.com (cc: golang-dev@googlegroups.com),

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

[dave_cheney.net, 2012-10-07 11:20:33]

On 2012/10/07 11:18:39, dfc wrote:
> Hello mailto:russross@gmail.com (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go.crypto

I've tested this in appengine 1.7.2 dev_server by copying
code.google.com/p/go.crypto into the helloworld demo app.

[minux1, 2012-10-07 11:25:46]

LGTM.

[dave_cheney.net, 2012-10-07 11:30:01]

Thank you. I think this topic deserves more discussion, but there is
no reason not to fix Russ' problem right now.

On Sun, Oct 7, 2012 at 10:25 PM,  <minux.ma@gmail.com> wrote:
> LGTM.
>
> http://codereview.appspot.com/6623053/

[rsc, 2012-10-07 14:12:08]

LGTM

https://codereview.appspot.com/6623053/diff/5001/ssh/terminal/util.go
File ssh/terminal/util.go (right):

https://codereview.appspot.com/6623053/diff/5001/ssh/terminal/util.go#newcode5
ssh/terminal/util.go:5: // +build linux,!appengine
If these functions are used from other packages in go.crypto, then you'll want a
separate appengine.go that has // +build appengine that provides stubs.

[russross, 2012-10-07 23:50:12]

> I've tested this in appengine 1.7.2 dev_server by copying
> code.google.com/p/go.crypto into the helloworld demo app.

I copied a fresh clone of code.google.com/p/go.crypto (changeset
79:33051f713476) into my app and applied the patch to ssh/terminal/util.go.  I
didn't see any errors related to util.go, but I did get:

2012/10/07 17:28:09 /.../code.google.com/p/go.crypto/ssh/client.go:415:10:
composite struct literal net.TCPAddr with untagged fields

I changed the offending line from:
	return &net.TCPAddr{ip, int(port)}, b, true
to:
	return &net.TCPAddr{IP: ip, Port: int(port)}, b, true
and tried again.  This time I got:

... ERROR Compile error:
syscall.errors:
code.google.com/p/go.crypto/salsa20/salsa.salsa2020XORKeyStream(0): not defined

I should note that I am using the amd64 version of the appengine SDK.  The
salsa20/salsa package appears to define a reference version and an assembly
version for amd64.  A build directive in salsa20/salsa/salsa20_ref.go:5 disables
building it for amd64, and I'm assuming the app engine SDK disables building the
_amd64.* files.

If I delete the _amd64.* files from that directory, it breaks salsa20.  If I
delete that (but leave salsa20/salsa/salsa208.go alone, because scrypt depends
on it and that is my actual dependency), it breaks the go.crypto/nacl package. 
If I delete that, it finally works.

So that's the long way of saying the patch looks good, but the overall problem
is still unresolved.  As far as I can tell, it would be fully resolved by fixing
the untagged composite literal fields in ssh/client.go, and convincing the build
system to use salsa20_ref.go on amd64 appengine.

All this has been from the development environment; I have not tried deploying.

[dave_cheney.net, 2012-10-07 23:59:02]

On 2012/10/07 23:50:12, russross wrote:
> > I've tested this in appengine 1.7.2 dev_server by copying
> > code.google.com/p/go.crypto into the helloworld demo app.
> 
> I copied a fresh clone of code.google.com/p/go.crypto (changeset
> 79:33051f713476) into my app and applied the patch to ssh/terminal/util.go.  I
> didn't see any errors related to util.go, but I did get:
> 
> 2012/10/07 17:28:09 /.../code.google.com/p/go.crypto/ssh/client.go:415:10:
> composite struct literal net.TCPAddr with untagged fields
> 
> I changed the offending line from:
> 	return &net.TCPAddr{ip, int(port)}, b, true
> to:
> 	return &net.TCPAddr{IP: ip, Port: int(port)}, b, true
> and tried again.  This time I got:
> 
> ... ERROR Compile error:
> syscall.errors:
> code.google.com/p/go.crypto/salsa20/salsa.salsa2020XORKeyStream(0): not
defined
> 
> I should note that I am using the amd64 version of the appengine SDK.  The
> salsa20/salsa package appears to define a reference version and an assembly
> version for amd64.  A build directive in salsa20/salsa/salsa20_ref.go:5
disables
> building it for amd64, and I'm assuming the app engine SDK disables building
the
> _amd64.* files.
> 
> If I delete the _amd64.* files from that directory, it breaks salsa20.  If I
> delete that (but leave salsa20/salsa/salsa208.go alone, because scrypt depends
> on it and that is my actual dependency), it breaks the go.crypto/nacl package.

> If I delete that, it finally works.
> 
> So that's the long way of saying the patch looks good, but the overall problem
> is still unresolved.  As far as I can tell, it would be fully resolved by
fixing
> the untagged composite literal fields in ssh/client.go, and convincing the
build
> system to use salsa20_ref.go on amd64 appengine.
> 
> All this has been from the development environment; I have not tried
deploying.

Thanks Russ. As a meta comment, the overarching issue is you used go get to get
something in the go.crypto package and because of the way the repo is
structured, it bought in a bunch of other code you didn't need. It is this code,
which is not compatible with appengine, that is upsetting the dev server.

[russross, 2012-10-08 00:03:27]

On Sun, Oct 7, 2012 at 5:59 PM,  <dave@cheney.net> wrote:
> On 2012/10/07 23:50:12, russross wrote:
>>
>> > I've tested this in appengine 1.7.2 dev_server by copying
>> > code.google.com/p/go.crypto into the helloworld demo app.
>
>
>> I copied a fresh clone of code.google.com/p/go.crypto (changeset
>> 79:33051f713476) into my app and applied the patch to
>
> ssh/terminal/util.go.  I
>>
>> didn't see any errors related to util.go, but I did get:
>
>
>> 2012/10/07 17:28:09
>
> /.../code.google.com/p/go.crypto/ssh/client.go:415:10:
>>
>> composite struct literal net.TCPAddr with untagged fields
>
>
>> I changed the offending line from:
>>         return &net.TCPAddr{ip, int(port)}, b, true
>> to:
>>         return &net.TCPAddr{IP: ip, Port: int(port)}, b, true
>> and tried again.  This time I got:
>
>
>> ... ERROR Compile error:
>> syscall.errors:
>> code.google.com/p/go.crypto/salsa20/salsa.salsa2020XORKeyStream(0):
>
> not defined
>
>> I should note that I am using the amd64 version of the appengine SDK.
>
> The
>>
>> salsa20/salsa package appears to define a reference version and an
>
> assembly
>>
>> version for amd64.  A build directive in
>
> salsa20/salsa/salsa20_ref.go:5 disables
>>
>> building it for amd64, and I'm assuming the app engine SDK disables
>
> building the
>>
>> _amd64.* files.
>
>
>> If I delete the _amd64.* files from that directory, it breaks salsa20.
>
>  If I
>>
>> delete that (but leave salsa20/salsa/salsa208.go alone, because scrypt
>
> depends
>>
>> on it and that is my actual dependency), it breaks the go.crypto/nacl
>
> package.
>>
>> If I delete that, it finally works.
>
>
>> So that's the long way of saying the patch looks good, but the overall
>
> problem
>>
>> is still unresolved.  As far as I can tell, it would be fully resolved
>
> by fixing
>>
>> the untagged composite literal fields in ssh/client.go, and convincing
>
> the build
>>
>> system to use salsa20_ref.go on amd64 appengine.
>
>
>> All this has been from the development environment; I have not tried
>
> deploying.
>
> Thanks Russ. As a meta comment, the overarching issue is you used go get
> to get something in the go.crypto package and because of the way the
> repo is structured, it bought in a bunch of other code you didn't need.
> It is this code, which is not compatible with appengine, that is
> upsetting the dev server.

That is true; I was using an older version of scrypt without problems
before it was merged into go.crypto.  The issue with amd64 in the
go.crypto/salsa package is in my direct line of dependencies, however,
so cherry picking packages would not help me there.

Thanks,

Russ

[dave_cheney.net, 2012-10-08 03:19:50]

Hi Russ,

I think I can fix those remaining issues as well, I'll rename this CL,
and roll those fixes in as well (not sure about the go vet one :/)

Cheers

Dave

On Mon, Oct 8, 2012 at 11:02 AM, Russ Ross <russross@gmail.com> wrote:
> On Sun, Oct 7, 2012 at 5:59 PM,  <dave@cheney.net> wrote:
>> On 2012/10/07 23:50:12, russross wrote:
>>>
>>> > I've tested this in appengine 1.7.2 dev_server by copying
>>> > code.google.com/p/go.crypto into the helloworld demo app.
>>
>>
>>> I copied a fresh clone of code.google.com/p/go.crypto (changeset
>>> 79:33051f713476) into my app and applied the patch to
>>
>> ssh/terminal/util.go.  I
>>>
>>> didn't see any errors related to util.go, but I did get:
>>
>>
>>> 2012/10/07 17:28:09
>>
>> /.../code.google.com/p/go.crypto/ssh/client.go:415:10:
>>>
>>> composite struct literal net.TCPAddr with untagged fields
>>
>>
>>> I changed the offending line from:
>>>         return &net.TCPAddr{ip, int(port)}, b, true
>>> to:
>>>         return &net.TCPAddr{IP: ip, Port: int(port)}, b, true
>>> and tried again.  This time I got:
>>
>>
>>> ... ERROR Compile error:
>>> syscall.errors:
>>> code.google.com/p/go.crypto/salsa20/salsa.salsa2020XORKeyStream(0):
>>
>> not defined
>>
>>> I should note that I am using the amd64 version of the appengine SDK.
>>
>> The
>>>
>>> salsa20/salsa package appears to define a reference version and an
>>
>> assembly
>>>
>>> version for amd64.  A build directive in
>>
>> salsa20/salsa/salsa20_ref.go:5 disables
>>>
>>> building it for amd64, and I'm assuming the app engine SDK disables
>>
>> building the
>>>
>>> _amd64.* files.
>>
>>
>>> If I delete the _amd64.* files from that directory, it breaks salsa20.
>>
>>  If I
>>>
>>> delete that (but leave salsa20/salsa/salsa208.go alone, because scrypt
>>
>> depends
>>>
>>> on it and that is my actual dependency), it breaks the go.crypto/nacl
>>
>> package.
>>>
>>> If I delete that, it finally works.
>>
>>
>>> So that's the long way of saying the patch looks good, but the overall
>>
>> problem
>>>
>>> is still unresolved.  As far as I can tell, it would be fully resolved
>>
>> by fixing
>>>
>>> the untagged composite literal fields in ssh/client.go, and convincing
>>
>> the build
>>>
>>> system to use salsa20_ref.go on amd64 appengine.
>>
>>
>>> All this has been from the development environment; I have not tried
>>
>> deploying.
>>
>> Thanks Russ. As a meta comment, the overarching issue is you used go get
>> to get something in the go.crypto package and because of the way the
>> repo is structured, it bought in a bunch of other code you didn't need.
>> It is this code, which is not compatible with appengine, that is
>> upsetting the dev server.
>
> That is true; I was using an older version of scrypt without problems
> before it was merged into go.crypto.  The issue with amd64 in the
> go.crypto/salsa package is in my direct line of dependencies, however,
> so cherry picking packages would not help me there.
>
> Thanks,
>
> Russ

[dave_cheney.net, 2012-10-08 07:09:54]

Hello russross@gmail.com, minux.ma@gmail.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[minux1, 2012-10-08 08:06:01]

https://codereview.appspot.com/6623053/diff/2005/salsa20/salsa/salsa20_ref.go
File salsa20/salsa/salsa20_ref.go (right):

https://codereview.appspot.com/6623053/diff/2005/salsa20/salsa/salsa20_ref.go...
salsa20/salsa/salsa20_ref.go:5: // +build !amd64, appengine
i think you want // +build !amd64 appengine

[dave_cheney.net, 2012-10-08 09:57:43]

https://codereview.appspot.com/6623053/diff/2005/salsa20/salsa/salsa20_ref.go
File salsa20/salsa/salsa20_ref.go (right):

https://codereview.appspot.com/6623053/diff/2005/salsa20/salsa/salsa20_ref.go...
salsa20/salsa/salsa20_ref.go:5: // +build !amd64, appengine
On 2012/10/08 08:06:02, minux wrote:
> i think you want // +build !amd64 appengine

Yes, thank you.

[dave_cheney.net, 2012-10-08 09:58:11]

Hello russross@gmail.com, minux.ma@gmail.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[agl1, 2012-10-08 14:31:51]

LGTM

[russross, 2012-10-08 16:37:48]

On 2012/10/08 09:58:11, dfc wrote:
> Hello mailto:russross@gmail.com, mailto:minux.ma@gmail.com,
mailto:rsc@golang.org (cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

This one works great for me.

Thanks!

[dave_cheney.net, 2012-10-09 02:15:48]

*** Submitted as
http://code.google.com/p/go/source/detail?r=29e4e1ec65e5&repo=crypto ***

go.crypto: various: fix appengine compatibility

Fixes issue 4102.

R=russross, minux.ma, rsc, agl
CC=golang-dev
http://codereview.appspot.com/6623053