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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump Go to 1.9.2 #33892

Merged
merged 1 commit into from Nov 27, 2017

Conversation

@thaJeztah
Copy link
Member

commented Jun 30, 2017

let's see what fails (or not) 馃憤

fixes #35152

@AkihiroSuda

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

interesting

08:48:07 --- FAIL: TestHashFile (0.07s)
08:48:07 	tarsum_test.go:66: invalid checksum. expected 1149ab94af7be6cc1da1335e398f24ee1cf4926b720044d229969dfc248ae7ec, got 55dfeb344351ab27f59aa60ebb0ed12025a2f2f4677bf77d26ea7a671274a9ca
08:48:07 --- FAIL: TestHashSubdir (0.05s)
08:48:07 	tarsum_test.go:103: invalid checksum. expected d7f8d6353dee4816f9134f4156bf6a9d470fdadfb5d89213721f7e86744a4e69, got 74a3326b8e766ce63a8e5232f22e9dd895be647fb3ca7d337e5e0a9b3da8ef28
08:48:07 FAIL
08:48:07 coverage: 15.8% of statements
08:48:07 FAIL	github.com/docker/docker/builder/remotecontext	0.333s
@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Jun 30, 2017

@AkihiroSuda

This comment has been minimized.

Copy link
Member

commented Jul 4, 2017

git bisect says golang/go@66b5a2f is the first commit causing this failure:

commit 66b5a2f3f0b1d94f69763114a85a977f5bc0277a
Author: Lars Jeppesen <jeppesen.lars@gmail.com>                
Date:   Sat Apr 29 23:25:34 2017 +0200  
                                                                                            
    archive/tar: remove file type bits from mode field                         
                           
    When writing tar files by using the FileInfoHeader                                   
    the type bits was set in the mode field of the header
    This is not correct according to the standard (GNU/Posix) and 
    other implementations.       
        
    Fixed #20150
                                                
    Change-Id: I3be7d946a1923ad5827cf45c696546a5e287ebba
    Reviewed-on: https://go-review.googlesource.com/42093
    Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
    Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
    TryBot-Result: Gobot Gobot <gobot@golang.org>
@tonistiigi

This comment has been minimized.

Copy link
Member

commented Jul 4, 2017

@AkihiroSuda Thanks for the debug.

I'm not sure what's the best approach here. Do we want to break build cache for everyone or fork the archive.FileInfoHeader and keep using the wrong headers? Also related: moby/buildkit#38

@AkihiroSuda

This comment has been minimized.

Copy link
Member

commented Jul 4, 2017

Although build cache are not persistent data, people will get surprised if caches were broken by just updating Go compiler.
So my opinion is to keep using the wrong header until we integrate buildkit to the engine, but I don't have any strong opinion.
WDYT?

@AkihiroSuda

This comment has been minimized.

Copy link
Member

commented Jul 4, 2017

I'll try to open a separate PR for filling file bits

@AkihiroSuda

This comment has been minimized.

Copy link
Member

commented Jul 4, 2017

opened #33935

@tiborvass

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2017

Can we confirm that the impact is only on caching? What about tar layers?

@tonistiigi

This comment has been minimized.

Copy link
Member

commented Jul 5, 2017

@tiborvass Layers use tar-split copy of the original headers and don't regenerate them.

@thaJeztah thaJeztah force-pushed the thaJeztah:bump-golang-19 branch from 072667d to d7c2a4b Jul 6, 2017

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2017

Rebased, because #33935 was merged; let's see if it goes green this time

@tophj-ibm

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2017

z failure looks serious, can you take a look @michael-holzheu ?

from the log

SIGILL: illegal instruction
PC=0x283186 m=0 sigcode=2
@thaJeztah thaJeztah referenced this pull request Jul 12, 2017
@utzb

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2017

The z golang issue is understood and addressed. Thanks for attempting in the first place!

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Jul 14, 2017

Thanks @utzb - is that fix upstream (golang), or an issue fixed in this repository? (IOW, should a rebase fix it, or do we need to wait for the next Go beta/release)

@utzb

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2017

The fix will be in upstream golang. With a little luck, it will be in 1.9beta3, but for sure in the first rc.
I verified test-unit and test-integration-cli run smooth using a golang with an early version of the fix.

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2017

Thanks, good to hear!

@utzb

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2017

@thaJeztah thaJeztah force-pushed the thaJeztah:bump-golang-19 branch from d7c2a4b to ff43e35 Jul 25, 2017

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2017

bumped to 1.9rc1

@tiborvass

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2017

Since we are bumping Go versions, we should probably update archive/tar as well.
Here is the changelog for archive/tar: https://gist.github.com/tiborvass/f9303e496644c77bd11ba08ec00d9fa0

cc @tonistiigi @dmcgowan

@darstahl

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2017

I'm investigating a blocker on Windows that prevents pulling some images. Not sure how CI passed tbh.

Edit: Oh, CI just side loads the base images. I'm not sure how that bypasses the bug, but it's new information to help me track it down.

@thaJeztah thaJeztah force-pushed the thaJeztah:bump-golang-19 branch from a80d169 to 1aab7e8 Nov 8, 2017

@thaJeztah thaJeztah changed the title [do not merge] Bump Go to 1.9.2 Bump Go to 1.9.2 Nov 8, 2017

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Nov 8, 2017

Removing the [do not merge] I think we can review this now; the changes made for the behaviour-change in Windows are a bit quirky, and looking at those, perhaps it should be a docker/cli test, so I added a TODO for that https://github.com/moby/moby/pull/33892/files#diff-f77ac0f89cdcfa4259ac67bced3d418cR39

@darrenstahlmsft was there still something to look into from your side?

ping @AkihiroSuda @vdemeester @cpuguy83 @yongtang PTAL

@vdemeester
Copy link
Member

left a comment

LGTM 馃惍

@cpuguy83
Copy link
Contributor

left a comment

LGTM

@cpuguy83

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2017

馃憥 Checking error strings, but the current error handling for this endpoint is not good enough as a true "not found" error would be seen as the container not being found, so we need more data in the error message to differentiate a "file not found" from a "container not found".

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Nov 8, 2017

@cpuguy83 agree it's pretty dirty; I didn't see a better option directly, other than splitting the test into a unit test (docker/cli), and an API test here (and may require changes to the endpoint itself as you mentioned).

Do you want me to look into this in this PR, or should we open an issue for tracking that effort?

@cpuguy83

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2017

We don't need it for this PR.

@dnephin
dnephin approved these changes Nov 8, 2017
Copy link
Member

left a comment

LGTM

@unclejack
Copy link
Contributor

left a comment

LGTM

@darstahl

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2017

@thaJeztah yes, Windows needs microsoft/hcsshim#144

@darstahl

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2017

Please hold the merge until that is in

@thaJeztah thaJeztah changed the title Bump Go to 1.9.2 [WIP] Bump Go to 1.9.2 Nov 8, 2017

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Nov 8, 2017

Thanks for checking @darrenstahlmsft - temporarily changed back to WIP

salah-khan added a commit to salah-khan/moby that referenced this pull request Nov 15, 2017
Update runc to 0351df1c5a66838d0c392b4ac4cf9450de844e2d
This fix updates runc to 0351df1c5a66838d0c392b4ac4cf9450de844e2d

With this fix the warnings generated by netgo and dlopen by go 1.9
are addressed.

See
- opencontainers/runc#1577
- opencontainers/runc#1579

This fix is part of the efforts for go 1.9 (moby#33892)

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Bump Go to 1.9.2
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

@thaJeztah thaJeztah force-pushed the thaJeztah:bump-golang-19 branch from 1aab7e8 to d6e1cc3 Nov 21, 2017

@thaJeztah thaJeztah changed the title [WIP] Bump Go to 1.9.2 Bump Go to 1.9.2 Nov 21, 2017

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2017

#35554 was merged; rebased this PR, and removed "WIP" again; this should be ready to go now

@cpuguy83
Copy link
Contributor

left a comment

LGTM

@vieux
vieux approved these changes Nov 27, 2017
Copy link
Collaborator

left a comment

LGTM

@vieux vieux merged commit c672fbd into moby:master Nov 27, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37956 has succeeded
Details
janky Jenkins build Docker-PRs 46669 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7081 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18226 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6892 has succeeded
Details

@thaJeztah thaJeztah deleted the thaJeztah:bump-golang-19 branch Nov 27, 2017

@vdemeester vdemeester referenced this pull request Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.