x/mod: remove vendor/modules.txt from module download This fixes a bug where vendor/modules.txt was accidently included during a module download. This CL trims this file for 1.24 modules and beyond. We cannot remove this for earlier Go versions because this would alter checksums and cause a checksum failure. This CL also adds the ability to case on the Go version in the root's go.mod file, enabling future behavior changes if necessary. Fixes: golang/go#63395 Updates: golang/go#37397 Change-Id: I4a4f2174b0f5e79c7e5c516e0db3c91e7d2ae4d9 Reviewed-on: https://go-review.googlesource.com/c/mod/+/584635 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Michael Matloob <matloob@golang.org>
diff --git a/zip/testdata/check_dir/various.txt b/zip/testdata/check_dir/various.txt index ee843be..4dd757d 100644 --- a/zip/testdata/check_dir/various.txt +++ b/zip/testdata/check_dir/various.txt
@@ -1,6 +1,7 @@ -- want -- valid: $work/valid.go +$work/vendor/modules.txt omitted: $work/.hg_archival.txt: file is inserted by 'hg archive' and is always omitted @@ -14,6 +15,7 @@ -- valid.go -- -- GO.MOD -- -- invalid.go' -- +-- vendor/modules.txt -- -- vendor/x/y -- -- sub/go.mod -- -- .hg_archival.txt --
diff --git a/zip/testdata/check_dir/various_go123.txt b/zip/testdata/check_dir/various_go123.txt new file mode 100644 index 0000000..169c059 --- /dev/null +++ b/zip/testdata/check_dir/various_go123.txt
@@ -0,0 +1,23 @@ +-- want -- +valid: +$work/go.mod +$work/valid.go +$work/vendor/modules.txt + +omitted: +$work/.hg_archival.txt: file is inserted by 'hg archive' and is always omitted +$work/.git: directory is a version control repository +$work/sub: directory is in another module +$work/vendor/x/y: file is in vendor directory + +invalid: +$work/invalid.go': malformed file path "invalid.go'": invalid char '\'' +-- valid.go -- +-- go.mod -- +go 1.23 +-- invalid.go' -- +-- vendor/modules.txt -- +-- vendor/x/y -- +-- sub/go.mod -- +-- .hg_archival.txt -- +-- .git/x -- \ No newline at end of file
diff --git a/zip/testdata/check_dir/various_go124.txt b/zip/testdata/check_dir/various_go124.txt new file mode 100644 index 0000000..bea7bf4 --- /dev/null +++ b/zip/testdata/check_dir/various_go124.txt
@@ -0,0 +1,24 @@ +-- want -- +valid: +$work/go.mod +$work/valid.go + +omitted: +$work/.hg_archival.txt: file is inserted by 'hg archive' and is always omitted +$work/.git: directory is a version control repository +$work/sub: directory is in another module +$work/vendor/modules.txt: file is in vendor directory +$work/vendor/x/y: file is in vendor directory + +invalid: +$work/invalid.go': malformed file path "invalid.go'": invalid char '\'' +-- go.mod -- +go 1.24 +-- valid.go -- +-- invalid.go' -- +-- vendor/modules.txt -- +-- vendor/x/y -- +-- sub/go.mod -- +go 1.23 +-- .hg_archival.txt -- +-- .git/x --
diff --git a/zip/testdata/check_files/various.txt b/zip/testdata/check_files/various.txt index a704a8a..cb312bf 100644 --- a/zip/testdata/check_files/various.txt +++ b/zip/testdata/check_files/various.txt
@@ -1,6 +1,7 @@ -- want -- valid: valid.go +vendor/modules.txt omitted: vendor/x/y: file is in vendor directory @@ -17,6 +18,7 @@ -- GO.MOD -- -- invalid.go' -- -- vendor/x/y -- +-- vendor/modules.txt -- -- sub/go.mod -- -- .hg_archival.txt -- -- valid.go --
diff --git a/zip/testdata/check_files/various_go123.txt b/zip/testdata/check_files/various_go123.txt new file mode 100644 index 0000000..7a34168 --- /dev/null +++ b/zip/testdata/check_files/various_go123.txt
@@ -0,0 +1,28 @@ +-- want -- +valid: +valid.go +go.mod +vendor/modules.txt + +omitted: +vendor/x/y: file is in vendor directory +sub/go.mod: file is in another module +.hg_archival.txt: file is inserted by 'hg archive' and is always omitted + +invalid: +not/../clean: file path is not clean +invalid.go': malformed file path "invalid.go'": invalid char '\'' +valid.go: multiple entries for file "valid.go" +-- valid.go -- +-- not/../clean -- +-- go.mod -- +go 1.23 +-- invalid.go' -- +-- vendor/x/y -- +-- vendor/modules.txt -- +-- sub/go.mod -- +-- .hg_archival.txt -- +-- valid.go -- +duplicate +-- valid.go -- +another duplicate
diff --git a/zip/testdata/check_files/various_go124.txt b/zip/testdata/check_files/various_go124.txt new file mode 100644 index 0000000..3e8881d --- /dev/null +++ b/zip/testdata/check_files/various_go124.txt
@@ -0,0 +1,29 @@ +-- want -- +valid: +valid.go +go.mod + +omitted: +vendor/x/y: file is in vendor directory +vendor/modules.txt: file is in vendor directory +sub/go.mod: file is in another module +.hg_archival.txt: file is inserted by 'hg archive' and is always omitted + +invalid: +not/../clean: file path is not clean +invalid.go': malformed file path "invalid.go'": invalid char '\'' +valid.go: multiple entries for file "valid.go" +-- valid.go -- +-- not/../clean -- +-- go.mod -- +go 1.24 +-- invalid.go' -- +-- vendor/x/y -- +-- vendor/modules.txt -- +-- sub/go.mod -- +go 1.23 +-- .hg_archival.txt -- +-- valid.go -- +duplicate +-- valid.go -- +another duplicate
diff --git a/zip/testdata/create/exclude_vendor_go124.txt b/zip/testdata/create/exclude_vendor_go124.txt new file mode 100644 index 0000000..5559d66 --- /dev/null +++ b/zip/testdata/create/exclude_vendor_go124.txt
@@ -0,0 +1,15 @@ +path=example.com/m +version=v1.0.0 +hash=h1:WxQ7ERgHLILawPCiuSwXp5UHul7CUSRRftO8b7GTnV8= +-- go.mod -- +module example.com/m + +go 1.24 +modules.txt is excluded in 1.24+. See golang.org/issue/63395 +-- vendor/modules.txt -- +excluded +see comment in isVendoredPackage and golang.org/issue/31562. +-- vendor/example.com/x/x.go -- +excluded +-- sub/vendor/sub.txt -- +excluded
diff --git a/zip/testdata/create_from_dir/exclude_vendor_go124.txt b/zip/testdata/create_from_dir/exclude_vendor_go124.txt new file mode 100644 index 0000000..5559d66 --- /dev/null +++ b/zip/testdata/create_from_dir/exclude_vendor_go124.txt
@@ -0,0 +1,15 @@ +path=example.com/m +version=v1.0.0 +hash=h1:WxQ7ERgHLILawPCiuSwXp5UHul7CUSRRftO8b7GTnV8= +-- go.mod -- +module example.com/m + +go 1.24 +modules.txt is excluded in 1.24+. See golang.org/issue/63395 +-- vendor/modules.txt -- +excluded +see comment in isVendoredPackage and golang.org/issue/31562. +-- vendor/example.com/x/x.go -- +excluded +-- sub/vendor/sub.txt -- +excluded
diff --git a/zip/vendor_test.go b/zip/vendor_test.go index 5eb9535..e330401 100644 --- a/zip/vendor_test.go +++ b/zip/vendor_test.go
@@ -6,17 +6,36 @@ import "testing" +var pre124 []string = []string{ + "", + "go1.14", + "go1.21.0", + "go1.22.4", + "go1.23", + "go1.23.1", + "go1.2", + "go1.7", + "go1.9", +} + func TestIsVendoredPackage(t *testing.T) { for _, tc := range []struct { path string want bool falsePositive bool // is this case affected by https://golang.org/issue/37397? + versions []string }{ - {path: "vendor/foo/foo.go", want: true}, - {path: "pkg/vendor/foo/foo.go", want: true}, - {path: "longpackagename/vendor/foo/foo.go", want: true}, + {path: "vendor/foo/foo.go", want: true, versions: pre124}, + {path: "pkg/vendor/foo/foo.go", want: true, versions: pre124}, + {path: "longpackagename/vendor/foo/foo.go", want: true, versions: pre124}, + {path: "vendor/vendor.go", want: false, versions: pre124}, + {path: "vendor/foo/modules.txt", want: true, versions: pre124}, + {path: "modules.txt", want: false, versions: pre124}, + {path: "vendor/amodules.txt", want: false, versions: pre124}, - {path: "vendor/vendor.go", want: false}, + // These test cases were affected by https://golang.org/issue/63395 + {path: "vendor/modules.txt", want: false, versions: pre124}, + {path: "vendor/modules.txt", want: true, versions: []string{"go1.24.0", "go1.24", "go1.99.0"}}, // We ideally want these cases to be false, but they are affected by // https://golang.org/issue/37397, and if we fix them we will invalidate @@ -24,13 +43,15 @@ {path: "pkg/vendor/vendor.go", falsePositive: true}, {path: "longpackagename/vendor/vendor.go", falsePositive: true}, } { - got := isVendoredPackage(tc.path) - want := tc.want - if tc.falsePositive { - want = true - } - if got != want { - t.Errorf("isVendoredPackage(%q) = %t; want %t", tc.path, got, tc.want) + for _, v := range tc.versions { + got := isVendoredPackage(tc.path, v) + want := tc.want + if tc.falsePositive { + want = true + } + if got != want { + t.Errorf("isVendoredPackage(%q, %s) = %t; want %t", tc.path, v, got, tc.want) + } if tc.falsePositive { t.Logf("(Expected a false-positive due to https://golang.org/issue/37397.)") }
diff --git a/zip/zip.go b/zip/zip.go index 5aed6e2..9b351c5 100644 --- a/zip/zip.go +++ b/zip/zip.go
@@ -50,6 +50,7 @@ "bytes" "errors" "fmt" + "go/version" "io" "os" "os/exec" @@ -60,6 +61,7 @@ "unicode" "unicode/utf8" + "golang.org/x/mod/modfile" "golang.org/x/mod/module" ) @@ -193,6 +195,20 @@ return cf, cf.Err() } +// parseGoVers extracts the Go version specified in the given go.mod file. +// It returns an empty string if the version is not found or if an error +// occurs during file parsing. +// +// The version string is in Go toolchain name syntax, prefixed with "go". +// Examples: "go1.21", "go1.22rc2", "go1.23.0" +func parseGoVers(file string, data []byte) string { + mfile, err := modfile.ParseLax(file, data, nil) + if err != nil || mfile.Go == nil { + return "" + } + return "go" + mfile.Go.Version +} + // checkFiles implements CheckFiles and also returns lists of valid files and // their sizes, corresponding to cf.Valid. It omits files in submodules, files // in vendored packages, symlinked files, and various other unwanted files. @@ -217,6 +233,7 @@ // Files in these directories will be omitted. // These directories will not be included in the output zip. haveGoMod := make(map[string]bool) + var vers string for _, f := range files { p := f.Path() dir, base := path.Split(p) @@ -226,8 +243,21 @@ addError(p, false, err) continue } - if info.Mode().IsRegular() { - haveGoMod[dir] = true + if !info.Mode().IsRegular() { + continue + } + haveGoMod[dir] = true + // Extract the Go language version from the root "go.mod" file. + // This ensures we correctly interpret Go version-specific file omissions. + // We use f.Open() to handle potential custom Open() implementations + // that the underlying File type might have. + if base == "go.mod" && dir == "" { + if file, err := f.Open(); err == nil { + if data, err := io.ReadAll(file); err == nil { + vers = version.Lang(parseGoVers("go.mod", data)) + } + file.Close() + } } } } @@ -257,7 +287,7 @@ addError(p, false, errPathNotRelative) continue } - if isVendoredPackage(p) { + if isVendoredPackage(p, vers) { // Skip files in vendored packages. addError(p, true, errVendored) continue @@ -758,7 +788,17 @@ // // Unfortunately, isVendoredPackage reports false positives for files in any // non-top-level package whose import path ends in "vendor". -func isVendoredPackage(name string) bool { +// The 'vers' parameter specifies the Go version declared in the module's +// go.mod file and must be a valid Go version according to the +// go/version.IsValid function. +// Vendoring behavior has evolved across Go versions, so this function adapts +// its logic accordingly. +func isVendoredPackage(name string, vers string) bool { + // vendor/modules.txt is a vendored package but was included in 1.23 and earlier. + // Remove vendor/modules.txt only for 1.24 and beyond to preserve older checksums. + if version.Compare(vers, "go1.24") >= 0 && name == "vendor/modules.txt" { + return true + } var i int if strings.HasPrefix(name, "vendor/") { i += len("vendor/") @@ -894,6 +934,12 @@ // files, as well as a list of directories and files that were skipped (for // example, nested modules and symbolic links). func listFilesInDir(dir string) (files []File, omitted []FileError, err error) { + // Extract the Go language version from the root "go.mod" file. + // This ensures we correctly interpret Go version-specific file omissions. + var vers string + if data, err := os.ReadFile(filepath.Join(dir, "go.mod")); err == nil { + vers = version.Lang(parseGoVers("go.mod", data)) + } err = filepath.Walk(dir, func(filePath string, info os.FileInfo, err error) error { if err != nil { return err @@ -908,7 +954,7 @@ // golang.org/issue/31562, described in isVendoredPackage. // We would like Create and CreateFromDir to produce the same result // for a set of files, whether expressed as a directory tree or zip. - if isVendoredPackage(slashPath) { + if isVendoredPackage(slashPath, vers) { omitted = append(omitted, FileError{Path: slashPath, Err: errVendored}) return nil }