Bump to Replace golang 1.10 with 1.12 #1384
Conversation
|
Well so much for the quick and easy touch. 1.12 is failing with a vet on a docker/types.go. It doesn't like that we have in both the V2Image and V1Compatibility structs. I think we can change the one in the V1Compatibility struct (line 137) to That compiles and seems to run, but I didn't test it much.... Here's the full error: |
|
At a first glance this rather looks like a real bug; is there a more complex point to it that I am missing? |
|
@mtrmac Nope, the problem is this is in vendored code. |
|
@rhatdan, you can exclude the file from validation in the scripts, see https://github.com/containers/buildah/blob/master/tests/validate/gofmt.sh. |
How about just fixing the data structures? There’s no point in having a validation step if we are going to ignore its output. A
https://github.com/containers/buildah/blob/master/docker/types.go is not vendored; it’s a manually-made copy. |
|
@mtrmac What would you suggest we call it. |
This question does not make sense to me WRT the situation in |
|
☔ The latest upstream changes (presumably 8ceda28) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I am seeing a similar issue in containers/image with go 1.12. |
|
As mentioned above, we have the issue over at containers/image: https://github.com/containers/image/blob/master/manifest/docker_schema2.go#L151 However, when (un)marshalling, we would encounter an error (that we never saw) which gives me the impression we can rename the field to anything. |
|
Yes the golang 1.12 error is present in Master. I was just about to send an email to you and Nalin about it. |
|
@rhatdan I did this in c/image (not merged) containers/image@3c99f1f |
|
But this does not fix the golang 1.12 issue. |
|
The error does not occur in Travis (see https://travis-ci.org/containers/buildah/builds/516139048?utm_source=github_status&utm_medium=notification) which sets up everything. I think we should make the integration tests easier to reproduce and run on a dev machine. |
|
I am not seeing this, I can now create the error with a much simpler command. It seems to be tied to the vfs storage driver. Try |
|
Also tied to ubuntu image. fedora and alpine working. |
|
|
This is the error line that matters:
|
|
Seems to be related to VFS. Good catch @rhatdan. I only saw the red herring :^) |
|
Same for podman master. |
|
It's a character device that we are not catching. |
|
The (dirty) diff below makes it work locally on my machine. Can you try it out @rhatdan? diff --git a/vendor/github.com/containers/storage/drivers/copy/copy.go b/vendor/github.com/containers/storage/drivers/copy/copy.go
index bcbc612848df..449c2bd8a855 100644
--- a/vendor/github.com/containers/storage/drivers/copy/copy.go
+++ b/vendor/github.com/containers/storage/drivers/copy/copy.go
@@ -153,8 +153,8 @@ func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error {
isHardlink := false
- switch f.Mode() & os.ModeType {
- case 0: // Regular file
+ switch mode := f.Mode(); {
+ case mode.IsRegular(): // Regular file
id := fileID{dev: stat.Dev, ino: stat.Ino}
if copyMode == Hardlink {
isHardlink = true
@@ -172,12 +172,12 @@ func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error {
copiedFiles[id] = dstPath
}
- case os.ModeDir:
+ case mode.IsDir():
if err := os.Mkdir(dstPath, f.Mode()); err != nil && !os.IsExist(err) {
return err
}
- case os.ModeSymlink:
+ case mode&os.ModeSymlink != 0:
link, err := os.Readlink(srcPath)
if err != nil {
return err
@@ -187,14 +187,14 @@ func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error {
return err
}
- case os.ModeNamedPipe:
+ case mode&os.ModeNamedPipe != 0:
fallthrough
- case os.ModeSocket:
+ case mode&os.ModeSocket != 0:
if err := unix.Mkfifo(dstPath, stat.Mode); err != nil {
return err
}
- case os.ModeDevice:
+ case mode&os.ModeDevice!=0:
if rsystem.RunningInUserNS() {
// cannot create a device if running in user namespace
return nil
@@ -202,9 +202,11 @@ func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error {
if err := unix.Mknod(dstPath, stat.Mode, int(stat.Rdev)); err != nil {
return err
}
+ case mode&os.ModeCharDevice!=0:
+ panic("IT S A CHAR DEVICE")
default:
- return fmt.Errorf("unknown file type for %s", srcPath)
+ return fmt.Errorf("unknown file type (mode %s) for %s", f.Mode(), srcPath)
}
// Everything below is copying metadata from src to dst. All this metadata |
|
@rhatdan, here's a PR: containers/storage#315 |
|
The problem was that we were checking for full matches of the masks, so char devices were not covered by the |
rhatdan
changed the title
|
@vrothberg Worked for me locally will see if full tests works. |
(Is there a PR?) This does not really make sense to me; schema2 is a fixed format with a fixed specification; we can’t just introduce a new field name for an existing field like this. (If it actually exists[*])
If we can’t change the external formats, either our data structures just don’t match that format and should be fixed, or we are taking shortcuts (reusing data structures across formats that have different semantics in different formats) that we shouldn’t be taking, or, sure, the externally-defined formats may be inconsistently defined. (It seems to kind of be all three at the same time; docker/docker similarly inherits the [*] maybe that field does not “actually exist”, after all — but then we should remove it from the declaration, not rename it.) |
|
Removing the field cause compilation errors. |
Vendor in containers/storage v1.12.2 Signed-off-by: Daniel J Walsh <[email protected]>
rhatdan
changed the title
|
Ok I checked upstream in Moby and they still have the duplicate name. I am just going to tell go vet to not check the docker types. |
|
@TomSweeneyRedHat @vrothberg @giuseppe @nalind Lets get this merged PTAL. |
|
LGTM, we need a similar go-vet trick when we bump c/image. I think it's to bump our other repos to 1.12 as well do avoid surprises. |
|
📌 Commit 19f29cc has been approved by |
|
☀️ Test successful - status-papr, status-travis |
Signed-off-by: Daniel J Walsh [email protected]