On 2012/10/07 11:18:39, dfc wrote: > Hello mailto:russross@gmail.com (cc: mailto:golang-dev@googlegroups.com), > > I'd like you ...
11 years, 6 months ago
(2012-10-07 11:20:33 UTC)
#2
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.
Thank you. I think this topic deserves more discussion, but there is no reason not ...
11 years, 6 months ago
(2012-10-07 11:30:01 UTC)
#4
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/
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 ...
11 years, 6 months ago
(2012-10-07 14:12:08 UTC)
#5
> I've tested this in appengine 1.7.2 dev_server by copying > code.google.com/p/go.crypto into the helloworld ...
11 years, 6 months ago
(2012-10-07 23:50:12 UTC)
#6
> 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.
On 2012/10/07 23:50:12, russross wrote: > > I've tested this in appengine 1.7.2 dev_server by ...
11 years, 6 months ago
(2012-10-07 23:59:02 UTC)
#7
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.
On Sun, Oct 7, 2012 at 5:59 PM, <dave@cheney.net> wrote: > On 2012/10/07 23:50:12, russross ...
11 years, 6 months ago
(2012-10-08 00:03:27 UTC)
#8
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
Hi Russ, I think I can fix those remaining issues as well, I'll rename this ...
11 years, 6 months ago
(2012-10-08 03:19:50 UTC)
#9
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
11 years, 6 months ago
(2012-10-08 16:37:48 UTC)
#15
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!
Issue 6623053: code review 6623053: go.crypto: various: fix appengine compatibility
(Closed)
Created 11 years, 6 months ago by dave_cheney.net
Modified 10 years, 6 months ago
Reviewers:
Base URL:
Comments: 3