Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client: transport: fix tlsconfig Clone() on different Golang versions #26406

Merged
merged 1 commit into from Sep 8, 2016

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Sep 8, 2016

Fix #26410

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

@AkihiroSuda
Copy link
Member Author

@AkihiroSuda AkihiroSuda force-pushed the fix-make-manpages branch 2 times, most recently from bd93b8a to fecca8a Compare September 8, 2016 05:18
@feilengcui008
Copy link

@AkihiroSuda actually it seems that go1.7 has tls.Config.clone, so can we use this directly and use tls.Config.Clone for go version >=1.8, and for go version <=1.6, we can use the code in tlsconfig_clone_go17.go,

@AkihiroSuda
Copy link
Member Author

AkihiroSuda commented Sep 8, 2016

@feilengcui008 clone can be called only from tls package itself

@feilengcui008
Copy link

@AkihiroSuda sorry, I didn't notice this...another thing is can we use the !go1.6 and !go1.7 in tlsconfig_clone.go instead of go1.8 since we can avoid continuing adding go1.9... if go upstream doesn't change this interface

@justincormack
Copy link
Contributor

Why not use golang:alpine as the base image for the man page build (or explicitly golang:1.7-alpine) as this has Go 1.7 already? (Would also need to check the other archs).

@AkihiroSuda
Copy link
Member Author

@justincormack yes, I was just actually thinking that 😃
So I'm going to replace this PR with #26416

@AkihiroSuda AkihiroSuda closed this Sep 8, 2016
@runcom
Copy link
Member

runcom commented Sep 8, 2016

I feel this is still needed though since I'm not able to compile anymore and #26410

@runcom runcom reopened this Sep 8, 2016
@runcom runcom changed the title client: revive Go 1.6 support for make manpages client: transport: fix tlsconfig Clone() on different Golang versions Sep 8, 2016
@runcom
Copy link
Member

runcom commented Sep 8, 2016

ping @justincormack @crosbymichael

@runcom
Copy link
Member

runcom commented Sep 8, 2016

@AkihiroSuda could you please edit the commit message?

@runcom
Copy link
Member

runcom commented Sep 8, 2016

@justincormack @crosbymichael Golang in Fedora 23 is still at Go 1.5.4 (http://koji.fedoraproject.org/koji/packageinfo?packageID=16224) for us still and in F24 we're at Go 1.6.x

@AkihiroSuda
Copy link
Member Author

@runcom done

@feilengcui008 go1.8 will continue to work for Go 1.9`.

poc: go1.6 work for Go 1.7

main.go:

package main

import "runtime"

func main() { println("hello " + runtime.Version()) }

go16.go (and suppose that there are go17.go and go18.go correspondingly)

// +build go1.6

package main

func init() { println("hello from +build go1.6") }

demo

$ go version
go version go1.7 linux/amd64
$ ls
go16.go  go17.go  go18.go  main.go
$ go build
$ ./demo
hello from +build go1.6
hello from +build go1.7
hello go1.7

@@ -0,0 +1,13 @@
// +build go1.6
// +build !go1.7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you move these build tags to the same line? or why is this needed at all? isn't it enough to say only build for go1.6 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crosbymichael go1.6 actually means go1.6+

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line means go1.6 && !go1.7.
If we split the line it would become go1.6 || !go1.7
https://golang.org/pkg/go/build/

And even on Go 1.7, the go1.6 tag is true, that's why we need !go1.7

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in one line it would be go1.6,!go1.7

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, updated

@AkihiroSuda
Copy link
Member Author

@justincormack

Why not use golang:alpine as the base image for the man page build (or explicitly golang:1.7-alpine) as this has Go 1.7 already? (Would also need to check the other archs).

Can it be delayed until Ubuntu 16.10 Yakkety Yak that provides Go 1.7 is released (for other archs)?
https://launchpad.net/ubuntu/+source/golang-1.7/1.7-2ubuntu1

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@@ -1,4 +1,4 @@
// +build !go1.7,!windows
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why there was !windows here, Clone() seems supporting windows.
golang/go@d24f446

@crosbymichael
Copy link
Contributor

LGTM

@tonistiigi
Copy link
Member

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants