-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
tar: fix Go 1.10 breakage #4790
Conversation
Best would be to merge it before 0.4.14 is released as Go 1.10 fixes performance issues on MacOS. |
thirdparty/tar/extractor.go
Outdated
if err != nil { | ||
if err == io.EOF { | ||
return nil | ||
if n != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how many other places we're making this mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if i'm comfortable using go1.10 until we test on it a bit to make sure nothing else weird is happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have problem with Go 1.10 because they refactored tar reader. I don't think Go 1.10 has any more failures of this kind.
thirdparty/tar/extractor.go
Outdated
switch err { | ||
case io.EOF: | ||
fallthrough | ||
case nil: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seams to change the behavior so they will always be a single Read now, instead of reading contentiously until there is an EOF. Also from https://golang.org/pkg/io/#Reader
Callers should treat a return of 0 and nil as indicating that nothing happened; in particular it does not indicate EOF
Shouldn't this be:
switch err {
case nil:
// continue
case io.EOF:
return nil
default:
return err
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually write this as
if err != nil {
if err == EOF {
return nil
}
return err
}
I think it makes it clearer what is going on, but either way is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was my mistake. For some reason it sill works correctly :P
License: MIT Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu do you plan on also updating circleci and travis to test with go 1.10? For now I would recommend we test with both go 1.9 and 1.10 but I can understand if that will take too long. |
In my opinion we can leave them be for now on Go 1.9. |
Alright, since the change that broke this was specific to the tar code, i'm okay with us shipping an rc2 with this fix, and building the rc2 release with go1.10. |
Resolves #4781 #4785
Thanks to @schomatis for tracing it.