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

Bump golang to 1.11.0 #37358

Merged
merged 7 commits into from Sep 7, 2018
Merged

Bump golang to 1.11.0 #37358

merged 7 commits into from Sep 7, 2018

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jun 27, 2018

It's that time of year again! Go 1.11 is released, let's use it.

This PR also:

Gopher
image credit: https://www.maxpixel.net/Animal-Mammal-Nora-Nature-Gopher-Meadow-Spring-2196633

@cpuguy83
Copy link
Member

golang:1.11beta1 not found

@kolyshkin
Copy link
Contributor Author

It worked on my laptop before I pushed it. Perhaps CI uses some old(er) private copy of dockerhub that needs to be updated?

@kolyshkin
Copy link
Contributor Author

PPC CI, looks like I chose a bad moment to re-push:

13:09:05 Could not resolve 'cdn-fastly.deb.debian.org'

Windows, apparently

13:11:05 # github.com/docker/docker/vendor/github.com/google/certificate-transparency/go/x509
13:11:05 ....\vendor\github.com\google\certificate-transparency\go\x509\root_windows.go:112:3: cannot use uintptr(unsafe.Pointer(sslPara)) (type uintptr) as type syscall.Pointer in field value

is caused by golang commit golang/go@4869ec0

@thaJeztah
Copy link
Member

/cc @cyli for the certificate-transparency issue (FYI)

@kolyshkin
Copy link
Contributor Author

certificate-transparency requires a one-line patch but it will break 1.10 compatibility. I think a backward-compatible fix can be made, too, although it'd be more than one liner.

@thaJeztah
Copy link
Member

I think it's important we report it (both in the Go issue tracker, and in certificate-transparency); don't think there's been a single version of go that didn't break something

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Jul 4, 2018

I think it's important we report it

Not quite sure yet if I want to report this one to golang/go (as the issue they solved is quite complicated) but I left a comment in golang/go#21376 (comment)

@thaJeztah
Copy link
Member

Thanks @kolyshkin those look good; at least we reported upstream (so that they're aware of the issue), and we have a possible way forward 👍

@thaJeztah
Copy link
Member

Opened #37390 to bump certificate-transparency to match what's currently in SwarmKit (so that a later bump will be smaller)

@kolyshkin
Copy link
Contributor Author

Not sure where this is coming from

13:27:55 --- FAIL: TestSessionCreate (0.01s)
13:27:55 session_test.go:23: assertion failed: error is not nil: Post http://%2Fgo%2Fsrc%2Fgithub.com%2Fdocker%2Fdocker%2Fbundles%2Ftest-integration%2Fdocker.sock/session: net/http: HTTP/1.x transport connection broken: malformed HTTP status code "*"

@thaJeztah
Copy link
Member

ping @tonistiigi could you have a look at that session failure? IIRC, that endpoint does an upgrade?

@kolyshkin
Copy link
Contributor Author

janky fail:

11:29:17 WARNING: failed to resolve symlinks in copy.go: lstat /go/src/github.com/docker/docker/copy.go: no such file or directory
11:29:17 WARNING: failed to make copy.go a relative path: Rel: can't make copy.go relative to /go/src/github.com/docker/docker
11:29:17 WARNING: failed to resolve symlinks in copy.go: lstat /go/src/github.com/docker/docker/copy.go: no such file or directory
11:29:17 WARNING: failed to make copy.go a relative path: Rel: can't make copy.go relative to /go/src/github.com/docker/docker
11:29:17 WARNING: failed to resolve symlinks in copy.go: lstat /go/src/github.com/docker/docker/copy.go: no such file or directory
11:29:17 WARNING: failed to make copy.go a relative path: Rel: can't make copy.go relative to /go/src/github.com/docker/docker
11:29:17 WARNING: failed to resolve symlinks in copy.go: lstat /go/src/github.com/docker/docker/copy.go: no such file or directory
11:29:17 WARNING: failed to make copy.go a relative path: Rel: can't make copy.go relative to /go/src/github.com/docker/docker
11:29:17 copy.go:241::warning: unnecessary conversion (unconvert)
11:29:17 copy.go:240::warning: unnecessary conversion (unconvert)
11:29:17 copy.go:240::warning: unnecessary conversion (unconvert)
11:29:17 copy.go:241::warning: unnecessary conversion (unconvert)
11:29:17 Build step 'Execute shell' marked build as failure

This is something going wrong with gometalinter. The file is daemon/graphdriver/copy/copy.go but somehow gometalinter (or maybe one of linters) fails to figure out its path, and probably because of this it fails to ignore the // nolint: unconvert directive added by commit 21b2c27.

@dnephin @alecthomas ^^^ can you PTAL?

@thaJeztah
Copy link
Member

Should we try updating gometalinter? Looks like the version hasn't been updated in a while;

GOMETALINTER_COMMIT=bfcc1d6942136fd86eb6f1a6fb328de8398fbd80

alecthomas/gometalinter@bfcc1d6...master (version v2.0.6 was tagged a few days ago https://github.com/alecthomas/gometalinter/releases/tag/v2.0.6)

@kolyshkin
Copy link
Contributor Author

Should we try updating gometalinter?

Indeed; let's see if it helps

@dnephin
Copy link
Member

dnephin commented Jul 4, 2018

Doesn't look like it helped. I'll try to look into it further soon. I took a quick look and my best guess so far is that something started returning basename instead of a full path to a file. I'm not sure why that is. Maybe a regression in the go beta release?

To get the rest of Ci running maybe add an || true to the validate step for now?

@thaJeztah
Copy link
Member

thaJeztah commented Jul 4, 2018

@dnephin looks like there's more breaking changes in Go 1.11; I opened alecthomas/gometalinter#499 to test gometalinter on 1.11, and golang/go#26222 upstream (output being printed twice)

quite some changes in this file since go 1.10; https://github.com/golang/go/commits/4ba55273713bebfbfe0bed9ce196e845c0c69567/src/cmd/vet/print.go diff

UPDATE:

my issue was marked as a duplicate of golang/go#26125 (which is labeled "release blocker", so should be fixed before Go 1.11 final)

@@ -29,8 +29,8 @@ func TestStrSliceMarshalJSON(t *testing.T) {

func TestStrSliceUnmarshalJSON(t *testing.T) {
parts := map[string][]string{
"": {"default", "values"},
"[]": {},
"": {"default", "values"},
Copy link
Member

Choose a reason for hiding this comment

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

As a workaround, we can add a blank line between the short and long keys to make it work for both 1.10 and 1.11 (see containerd/containerd#2436) but it's really a workaround

I opened a ticket upstream for the gofmt change; golang/go#26228

Copy link
Member

Choose a reason for hiding this comment

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

Ok, this was marked as a "won't fix", "documentation only" change golang/go#26228 (comment), so if we want to support multiple versions of Go, we need to make some changes to CI and/or the contributing guide;

This change was done on purpose in golang/go@542ea5a.
As to why gofmt changes every release - see @griesemer's comment in golang/go#25161 (comment).
However, I agree that this change should be documented in the release notes. I can't see it in https://tip.golang.org/doc/go1.11.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this looks better (in most cases), thanks for suggestion!

Copy link
Member

Choose a reason for hiding this comment

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

Discussing possible enhancements on the ticket upstream to ease migrating between Go versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In most cases, if I remember correctly, once rebasing to a newer Go version we are losing compatibility with older ones either directly (i.e. some code in moby/moby starts using some feature from the new Go version) or indirectly (i.e. some code in vendored or otherwise used packages relies on a feature from the new Go version). I am about 84% sure current moby can't be compiled with Go 1.9, for example.

Therefore, chances are slim we'll be supporting Go 1.10 when Go 1.11 will become the current stable version of Go. Having said that, I am all for maintaining compatibility with older versions unless it requires some tremendous efforts.

@kolyshkin kolyshkin force-pushed the go111 branch 2 times, most recently from 7f2084c to 298e445 Compare July 5, 2018 18:13
@kolyshkin
Copy link
Contributor Author

Finally something new for a change! On janky, powerpc, and z:

12:05:57 --- FAIL: TestMountInit (0.00s)
12:05:57 mount_test.go:40: operation not supported
12:05:57 --- FAIL: TestMountChanges (0.00s)
12:05:57 mount_test.go:152: operation not supported
12:05:57 FAIL

On Windows:

12:09:14 --- FAIL: TestTarSums (0.05s)
12:09:14 tarsum_test.go:393: expecting [tarsum+sha256:8bf12d7e67c51ee2e8306cba569398b1b9f419969521a12ffb9d8875e8836738], but got [tarsum+sha256:75258b2c5dcd9adfe24ce71eeca5fc5019c7e669912f15703ede92b1a60cb11f]
12:09:14 tarsum_test.go:393: expecting [tarsum+md5:0d7529ec7a8360155b48134b8e599f53], but got [tarsum+md5:3a6cdb475d90459ac0d3280703d17be2]
12:09:14 tarsum_test.go:393: expecting [tarsum+sha1:f1fee39c5925807ff75ef1925e7a23be444ba4df], but got [tarsum+sha1:14b5e0d12a0c50a4281e86e92153fa06d55d00c6]
12:09:14 tarsum_test.go:393: expecting [tarsum+sha224:6319390c0b061d639085d8748b14cd55f697cf9313805218b21cf61c], but got [tarsum+sha224:dd8925b7a4c71b13f3a68a0f9428a757c76b93752c398f272a9062d5]
12:09:14 tarsum_test.go:393: expecting [tarsum+sha384:a578ce3ce29a2ae03b8ed7c26f47d0f75b4fc849557c62454be4b5ffd66ba021e713b48ce71e947b43aab57afd5a7636], but got [tarsum+sha384:e39e82f40005134bed13fb632d1a5f2aa4675c9ddb4a136fbcec202797e68d2f635e1200dee2e3a8d7f69d54d3f2fd27]
12:09:14 tarsum_test.go:393: expecting [tarsum+sha512:e9bfb90ca5a4dfc93c46ee061a5cf9837de6d2fdf82544d6460d3147290aecfabf7b5e415b9b6e72db9b8941f149d5d69fb17a394cbfaf2eac523bd9eae21855], but got [tarsum+sha512:7c56de40b2d1ed3863ff25d83b59cdc8f53e67d1c01c3ee8f201f8e4dec3107da976d0c0ec9109c962a152b32699fe329b2dab13966020e400c32878a0761a7e]
12:09:14 FAIL

To include the following backported fix:

kolyshkin/ugorji-go@1cf431c13dec46596

which should fix this:

> 13:40:53 vendor/github.com/ugorji/go/codec/gen-helper.generated.go:1:
> possible malformed +build comment%!(EXTRA []interface {}=[])

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is to include the Go 1.11 fix
(containerd/continuity#120).
Again (see c64a244).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Instead of installing golang from sources, it's easier to use
golang image which is based on Debian Stretch.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
We would like to use a version with .0 suffix (like 1.11.0) in
Dockerfile, so that once a .1 version is out (like 1.11.1) we
won't accidentally switch to it.

Unfortunately it's not possible to use .0 suffix currently
as it breaks the check in make.ps1. This patch fixes that.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It's that time of year again! Go 1.11 is released, time to use it.

This commit also

* removes our archive/tar fork, since upstream archive/tar
  is fixed for static builds, and osusergo build tag is set.

* removes ENV GO_VERSION from Dockerfile as it's not needed
  anymore since PR moby#37592 is merged.

[v2: switch to beta2]
[v3: switch to beta3]
[v4: rc1]
[v5: remove ENV GO_VERSION as PR moby#37592 is now merged]
[v6: rc2]
[v7: final!]
[v8: use 1.11.0]
[v9: back to 1.11]
[v8: use 1.11.0]

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
We can do that now as we're no longer carrying archive/tar.
Note that latest vndr removes vendor/ subdir so we don't have to,
thus the change in hack/validate/vendor.

While at it, re-run a new vndr version to make sure everything
that should be there is.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

To allow for FROM golang:1.11.0, we need to remove .0 suffix from the version number.

To that effect:

Once the CI PR Is merged, windows CI needs to be restarted, and once green, we're good to go!

@lowenna
Copy link
Member

lowenna commented Sep 7, 2018

  • CI Script updated merged in my repo

  • Jenkins Windows RS1 job updated with revised script

  • rebuild/WindowsRS1

  • I also restarted experimental failing for an unrelated reason

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, and all green now!

@@ -57,7 +57,8 @@ github.com/samuel/go-zookeeper d0e0d8e11f318e000a8cc434616d69e329edc374
github.com/deckarep/golang-set ef32fa3046d9f249d399f98ebaf9be944430fd1d
github.com/coreos/etcd v3.2.1
github.com/coreos/go-semver v0.2.0
github.com/ugorji/go f1f1a805ed361a0e078bb537e4ea78cd37dcf065
# fix for go vet (https://github.com/kolyshkin/ugorji-go/commit/1cf431c13dec46596)
Copy link
Member

Choose a reason for hiding this comment

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

we should create a tracking issue for this (ie to get rid of the fork)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, #37804

@thaJeztah thaJeztah merged commit ec99bd6 into moby:master Sep 7, 2018
bhipple added a commit to bhipple/nixpkgs that referenced this pull request Sep 9, 2018
The linked PR has been merged, and in fact dockerTools has upgraded to the
latest `go1.11` compiler:

moby/moby#35739
moby/moby#37358
AkihiroSuda added a commit to AkihiroSuda/buildkit_poc that referenced this pull request Sep 14, 2018
For consistency with Moby (moby/moby#37358)

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
AkihiroSuda added a commit to AkihiroSuda/buildkit_poc that referenced this pull request Sep 15, 2018
For consistency with Moby (moby/moby#37358)

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
AkihiroSuda added a commit to AkihiroSuda/buildkit_poc that referenced this pull request Sep 15, 2018
For consistency with Moby (moby/moby#37358)

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
AkihiroSuda added a commit to AkihiroSuda/buildkit_poc that referenced this pull request Sep 15, 2018
For consistency with Moby (moby/moby#37358)

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
AkihiroSuda added a commit to AkihiroSuda/buildkit_poc that referenced this pull request Sep 15, 2018
For consistency with Moby (moby/moby#37358)

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
andir pushed a commit to andir/nixpkgs that referenced this pull request Jan 30, 2019
The linked PR has been merged, and in fact dockerTools has upgraded to the
latest `go1.11` compiler:

moby/moby#35739
moby/moby#37358
(cherry picked from commit 4d8bb9a)
crazy-max pushed a commit to crazy-max/dockerfile that referenced this pull request Jan 8, 2022
For consistency with Moby (moby/moby#37358)

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
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

8 participants