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 to Go 1.11.0 #2435

Merged
merged 1 commit into from Oct 26, 2018
Merged

Bump to Go 1.11.0 #2435

merged 1 commit into from Oct 26, 2018

Conversation

thaJeztah
Copy link
Member

Already saw some breakage in moby/moby#37358, so lets run it here as well

Changelog: https://tip.golang.org/doc/go1.11

@@ -367,6 +367,7 @@ func checkLabels(ctx context.Context, t *testing.T, cs content.Store) {
labels := map[string]string{
"k1": "v1",
"k2": "v2",

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@thaJeztah
Copy link
Member Author

temporarily cherry-picked #2420 to fix compilation on 1.11

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 5, 2018

Okay, we're getting closer; 3 out of four pass (on Linux);

There's a panic on Go 1.11beta1 with CGO_ENABLED=1; https://api.travis-ci.org/v3/job/400360537/log.txt

?   	github.com/containerd/containerd/cmd/containerd-shim	1.013s	coverage: 9.3% of statements [no test files]
panic: invalid argument

goroutine 1 [running]:
github.com/containerd/containerd/cmd/containerd-stress.init.0()
	/home/travis/gopath/src/github.com/containerd/containerd/cmd/containerd-stress/main.go:61 +0x73c
FAIL	github.com/containerd/containerd/cmd/containerd-stress	0.024s
make: *** [coverage] Error 1

Panic is done here;

// set higher ulimits
if err := setRlimit(); err != nil {
panic(err)
}

Originating from

func setRlimit() error {
rlimit := uint64(100000)
if rlimit > 0 {
var limit syscall.Rlimit
if err := syscall.Getrlimit(syscall.RLIMIT_NOFILE, &limit); err != nil {
return err
}
if limit.Cur < rlimit {
limit.Cur = rlimit
if err := syscall.Setrlimit(syscall.RLIMIT_NOFILE, &limit); err != nil {
return err
}
}
}
return nil
}

@mlaventure
Copy link
Contributor

syscall.Setrlimit(...) is the most likely culprit, they may have been a breaking change in the go code.

@mlaventure
Copy link
Contributor

Just checked, no changes to those syscalls. May be a regression in CGO, may want to print the value returned by Getrlimit() to ensure it's not corrupted somehow

@thaJeztah
Copy link
Member Author

Failure on Windows; looks like there were some more format errors;

?   	github.com/containerd/containerd/version	[no test files]
# github.com/containerd/containerd/windows
windows\process.go:124: Wrapf format %s has arg s of wrong type github.com/containerd/containerd/runtime.Status
windows\task.go:287: Wrapf format %d has arg id of wrong type string
windows\task.go:300: Wrapf format %d has arg t.id of wrong type string
Makefile:223: recipe for target 'coverage' failed
mingw32-make: *** [coverage] Error 2

@thaJeztah
Copy link
Member Author

Pushed a commit here, and as a separate PR; #2487

@codecov-io
Copy link

codecov-io commented Jul 23, 2018

Codecov Report

Merging #2435 into master will increase coverage by 1.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2435      +/-   ##
==========================================
+ Coverage   43.74%   44.76%   +1.01%     
==========================================
  Files         100       93       -7     
  Lines       10728     9555    -1173     
==========================================
- Hits         4693     4277     -416     
+ Misses       5305     4585     -720     
+ Partials      730      693      -37
Flag Coverage Δ
#linux 48.98% <ø> (+1.56%) ⬆️
#windows 41.04% <ø> (+0.13%) ⬆️
Impacted Files Coverage Δ
mount/lookup_unix.go 0% <0%> (-75%) ⬇️
oci/spec.go 40% <0%> (-53.72%) ⬇️
mount/mount_linux.go 0% <0%> (-31.02%) ⬇️
archive/compression/compression.go 43.93% <0%> (-14.76%) ⬇️
mount/mountinfo_linux.go 53.06% <0%> (-8.17%) ⬇️
content/local/writer.go 52.63% <0%> (-5.22%) ⬇️
metadata/content.go 39.32% <0%> (-4.49%) ⬇️
metadata/leases.go 40.6% <0%> (-3.68%) ⬇️
metadata/migrations.go 42.3% <0%> (-1.45%) ⬇️
archive/tar.go 43.11% <0%> (-0.03%) ⬇️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c444666...07237e3. Read the comment docs.

@thaJeztah
Copy link
Member Author

Yay! All green now!

Let me rebase to get rid of the extra commit

@thaJeztah thaJeztah changed the title [do not merge] Bump to Go 1.11 Bump to Go 1.11beta2 Jul 23, 2018
@crosbymichael
Copy link
Member

Why would we bump to a beta version?

@thaJeztah
Copy link
Member Author

@crosbymichael sorry, probably should've kept the [do not merge]; mainly opening this to catch possible regressions before it leaves "beta"

@thaJeztah thaJeztah changed the title Bump to Go 1.11beta2 [do not merge] Bump to Go 1.11beta2 Jul 25, 2018
@thaJeztah thaJeztah changed the title [do not merge] Bump to Go 1.11beta2 [do not merge] Bump to Go 1.11beta3 Aug 6, 2018
@thaJeztah thaJeztah changed the title [do not merge] Bump to Go 1.11beta3 [do not merge] Bump to Go 1.11rc1 Aug 15, 2018
.travis.yml Outdated
@@ -7,7 +7,7 @@ services:
language: go

go:
- "1.10.x"
- "1.11"
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be changed to 1.11.x once the first patch release for Go 1.11 is released

Copy link
Member

Choose a reason for hiding this comment

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

Can you update this now? Now that 1.2 is out I think if you change this to 1.11.x we can merge this.

@thaJeztah thaJeztah changed the title [do not merge] Bump to Go 1.11rc1 Bump to Go 1.11.0 Aug 28, 2018
@thaJeztah
Copy link
Member Author

Go 1.11 was released, so this is now no longer "WIP"

@crosbymichael
Copy link
Member

LGTM

@dmcgowan
Copy link
Member

dmcgowan commented Aug 28, 2018

Any reason or risk to changing this before 1.2? My preference would be to leave this open to test but merge after 1.2.0-rc.0.

@AkihiroSuda AkihiroSuda mentioned this pull request Oct 21, 2018
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

@estesp rebased and updated; thanks for the ping 👍

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

None yet

6 participants