From 9cd0e4c9f675aeac595a4cbb5ba1b46798ce0fdf Mon Sep 17 00:00:00 2001 From: Sam Thanawalla Date: Thu, 9 May 2024 20:20:01 +0000 Subject: [PATCH 1/3] 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 Reviewed-by: Michael Matloob --- zip/testdata/check_dir/various.txt | 2 + zip/testdata/check_dir/various_go123.txt | 23 ++++++++ zip/testdata/check_dir/various_go124.txt | 24 ++++++++ zip/testdata/check_files/various.txt | 2 + zip/testdata/check_files/various_go123.txt | 28 ++++++++++ zip/testdata/check_files/various_go124.txt | 29 ++++++++++ zip/testdata/create/exclude_vendor_go124.txt | 15 +++++ .../create_from_dir/exclude_vendor_go124.txt | 15 +++++ zip/vendor_test.go | 43 ++++++++++---- zip/zip.go | 56 +++++++++++++++++-- 10 files changed, 221 insertions(+), 16 deletions(-) create mode 100644 zip/testdata/check_dir/various_go123.txt create mode 100644 zip/testdata/check_dir/various_go124.txt create mode 100644 zip/testdata/check_files/various_go123.txt create mode 100644 zip/testdata/check_files/various_go124.txt create mode 100644 zip/testdata/create/exclude_vendor_go124.txt create mode 100644 zip/testdata/create_from_dir/exclude_vendor_go124.txt 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 @@ $work/invalid.go': malformed file path "invalid.go'": invalid char '\'' -- 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 @@ valid.go: multiple entries for file "valid.go" -- 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 @@ package zip 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 @@ func TestIsVendoredPackage(t *testing.T) { {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 @@ import ( "bytes" "errors" "fmt" + "go/version" "io" "os" "os/exec" @@ -60,6 +61,7 @@ import ( "unicode" "unicode/utf8" + "golang.org/x/mod/modfile" "golang.org/x/mod/module" ) @@ -193,6 +195,20 @@ func CheckFiles(files []File) (CheckedFiles, error) { 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 @@ func checkFiles(files []File) (cf CheckedFiles, validFiles []File, validSizes [] // 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 @@ func checkFiles(files []File) (cf CheckedFiles, validFiles []File, validSizes [] 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 @@ func checkFiles(files []File) (cf CheckedFiles, validFiles []File, validSizes [] 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 @@ func (fi dataFileInfo) Sys() interface{} { return nil } // // 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 @@ func (cc collisionChecker) check(p string, isDir bool) error { // 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 @@ func listFilesInDir(dir string) (files []File, omitted []FileError, err error) { // 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 } From c8a731972177c6ce4073699c705e55918ee7be09 Mon Sep 17 00:00:00 2001 From: Sam Thanawalla Date: Wed, 9 Oct 2024 20:29:36 +0000 Subject: [PATCH 2/3] x/mod: fix handling of vendored packages with '/vendor' in non-top-level paths This CL address a bug in the handling of vendored packages where the '/vendor' element appears in non-top level import paths within a module zip file. This issue arose from an incorrect offset calculation that caused non-vendored packages to be incorrectly identified as vendored. This CL corrects the offset calculation for Go 1.24 and beyond. Fixes golang/go#37397 Change-Id: Ibf1751057e8383c7b82f1622674204597e735b7c Reviewed-on: https://go-review.googlesource.com/c/mod/+/619175 LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Matloob --- zip/testdata/check_dir/various.txt | 2 + zip/testdata/check_dir/various_go123.txt | 4 +- zip/testdata/check_dir/various_go124.txt | 2 + zip/testdata/check_files/various.txt | 2 + zip/testdata/check_files/various_go123.txt | 2 + zip/testdata/check_files/various_go124.txt | 2 + zip/testdata/create/exclude_vendor.txt | 3 ++ zip/testdata/create/exclude_vendor_go124.txt | 5 ++- .../create_from_dir/exclude_vendor.txt | 3 ++ .../create_from_dir/exclude_vendor_go124.txt | 5 ++- zip/vendor_test.go | 43 +++++++++---------- zip/zip.go | 29 +++++++++---- 12 files changed, 67 insertions(+), 35 deletions(-) diff --git a/zip/testdata/check_dir/various.txt b/zip/testdata/check_dir/various.txt index 4dd757d..0ded5ac 100644 --- a/zip/testdata/check_dir/various.txt +++ b/zip/testdata/check_dir/various.txt @@ -6,6 +6,7 @@ $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/pkg/vendor/vendor.go: file is in vendor directory $work/sub: directory is in another module $work/vendor/x/y: file is in vendor directory @@ -20,3 +21,4 @@ $work/invalid.go': malformed file path "invalid.go'": invalid char '\'' -- sub/go.mod -- -- .hg_archival.txt -- -- .git/x -- +-- pkg/vendor/vendor.go -- diff --git a/zip/testdata/check_dir/various_go123.txt b/zip/testdata/check_dir/various_go123.txt index 169c059..69102ac 100644 --- a/zip/testdata/check_dir/various_go123.txt +++ b/zip/testdata/check_dir/various_go123.txt @@ -7,6 +7,7 @@ $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/pkg/vendor/vendor.go: file is in vendor directory $work/sub: directory is in another module $work/vendor/x/y: file is in vendor directory @@ -20,4 +21,5 @@ go 1.23 -- vendor/x/y -- -- sub/go.mod -- -- .hg_archival.txt -- --- .git/x -- \ No newline at end of file +-- .git/x -- +-- pkg/vendor/vendor.go -- diff --git a/zip/testdata/check_dir/various_go124.txt b/zip/testdata/check_dir/various_go124.txt index bea7bf4..43bed2e 100644 --- a/zip/testdata/check_dir/various_go124.txt +++ b/zip/testdata/check_dir/various_go124.txt @@ -1,6 +1,7 @@ -- want -- valid: $work/go.mod +$work/pkg/vendor/vendor.go $work/valid.go omitted: @@ -22,3 +23,4 @@ go 1.24 go 1.23 -- .hg_archival.txt -- -- .git/x -- +-- pkg/vendor/vendor.go -- diff --git a/zip/testdata/check_files/various.txt b/zip/testdata/check_files/various.txt index cb312bf..06ec791 100644 --- a/zip/testdata/check_files/various.txt +++ b/zip/testdata/check_files/various.txt @@ -7,6 +7,7 @@ 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 +pkg/vendor/vendor.go: file is in vendor directory invalid: not/../clean: file path is not clean @@ -25,3 +26,4 @@ valid.go: multiple entries for file "valid.go" duplicate -- valid.go -- another duplicate +-- pkg/vendor/vendor.go -- diff --git a/zip/testdata/check_files/various_go123.txt b/zip/testdata/check_files/various_go123.txt index 7a34168..a28be93 100644 --- a/zip/testdata/check_files/various_go123.txt +++ b/zip/testdata/check_files/various_go123.txt @@ -8,6 +8,7 @@ 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 +pkg/vendor/vendor.go: file is in vendor directory invalid: not/../clean: file path is not clean @@ -26,3 +27,4 @@ go 1.23 duplicate -- valid.go -- another duplicate +-- pkg/vendor/vendor.go -- diff --git a/zip/testdata/check_files/various_go124.txt b/zip/testdata/check_files/various_go124.txt index 3e8881d..94d4bfd 100644 --- a/zip/testdata/check_files/various_go124.txt +++ b/zip/testdata/check_files/various_go124.txt @@ -2,6 +2,7 @@ valid: valid.go go.mod +pkg/vendor/vendor.go omitted: vendor/x/y: file is in vendor directory @@ -27,3 +28,4 @@ go 1.23 duplicate -- valid.go -- another duplicate +-- pkg/vendor/vendor.go -- diff --git a/zip/testdata/create/exclude_vendor.txt b/zip/testdata/create/exclude_vendor.txt index 79b2c08..40ac449 100644 --- a/zip/testdata/create/exclude_vendor.txt +++ b/zip/testdata/create/exclude_vendor.txt @@ -12,3 +12,6 @@ see comment in isVendoredPackage and golang.org/issue/31562. excluded -- sub/vendor/sub.txt -- excluded +-- pkg/vendor/vendor.go -- +excluded +see comment in isVendoredPackage and golang.org/issue/37397 diff --git a/zip/testdata/create/exclude_vendor_go124.txt b/zip/testdata/create/exclude_vendor_go124.txt index 5559d66..940df51 100644 --- a/zip/testdata/create/exclude_vendor_go124.txt +++ b/zip/testdata/create/exclude_vendor_go124.txt @@ -1,6 +1,6 @@ path=example.com/m version=v1.0.0 -hash=h1:WxQ7ERgHLILawPCiuSwXp5UHul7CUSRRftO8b7GTnV8= +hash=h1:mJR1q75yiMK6+CDw6DuhMS4v4hNoLXqkyk2Cph4VS8Q= -- go.mod -- module example.com/m @@ -13,3 +13,6 @@ see comment in isVendoredPackage and golang.org/issue/31562. excluded -- sub/vendor/sub.txt -- excluded +-- pkg/vendor/vendor.go -- +included +see comment in isVendoredPackage and golang.org/issue/37397 diff --git a/zip/testdata/create_from_dir/exclude_vendor.txt b/zip/testdata/create_from_dir/exclude_vendor.txt index 79b2c08..40ac449 100644 --- a/zip/testdata/create_from_dir/exclude_vendor.txt +++ b/zip/testdata/create_from_dir/exclude_vendor.txt @@ -12,3 +12,6 @@ see comment in isVendoredPackage and golang.org/issue/31562. excluded -- sub/vendor/sub.txt -- excluded +-- pkg/vendor/vendor.go -- +excluded +see comment in isVendoredPackage and golang.org/issue/37397 diff --git a/zip/testdata/create_from_dir/exclude_vendor_go124.txt b/zip/testdata/create_from_dir/exclude_vendor_go124.txt index 5559d66..940df51 100644 --- a/zip/testdata/create_from_dir/exclude_vendor_go124.txt +++ b/zip/testdata/create_from_dir/exclude_vendor_go124.txt @@ -1,6 +1,6 @@ path=example.com/m version=v1.0.0 -hash=h1:WxQ7ERgHLILawPCiuSwXp5UHul7CUSRRftO8b7GTnV8= +hash=h1:mJR1q75yiMK6+CDw6DuhMS4v4hNoLXqkyk2Cph4VS8Q= -- go.mod -- module example.com/m @@ -13,3 +13,6 @@ see comment in isVendoredPackage and golang.org/issue/31562. excluded -- sub/vendor/sub.txt -- excluded +-- pkg/vendor/vendor.go -- +included +see comment in isVendoredPackage and golang.org/issue/37397 diff --git a/zip/vendor_test.go b/zip/vendor_test.go index e330401..9deb192 100644 --- a/zip/vendor_test.go +++ b/zip/vendor_test.go @@ -18,43 +18,40 @@ var pre124 []string = []string{ "go1.9", } +var after124 []string = []string{"go1.24.0", "go1.24", "go1.99.0"} + +var allVers []string = append(pre124, after124...) + 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 string + want bool + versions []string }{ - {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/foo/foo.go", want: true, versions: allVers}, + {path: "pkg/vendor/foo/foo.go", want: true, versions: allVers}, + {path: "longpackagename/vendor/foo/foo.go", want: true, versions: allVers}, + {path: "vendor/vendor.go", want: false, versions: allVers}, + {path: "vendor/foo/modules.txt", want: true, versions: allVers}, + {path: "modules.txt", want: false, versions: allVers}, + {path: "vendor/amodules.txt", want: false, versions: allVers}, // 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"}}, + {path: "vendor/modules.txt", want: true, versions: after124}, - // 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 - // existing module checksums. We must leave them as-is-for now. - {path: "pkg/vendor/vendor.go", falsePositive: true}, - {path: "longpackagename/vendor/vendor.go", falsePositive: true}, + // These test cases were affected by https://golang.org/issue/37397 + {path: "pkg/vendor/vendor.go", want: true, versions: pre124}, + {path: "pkg/vendor/vendor.go", want: false, versions: after124}, + {path: "longpackagename/vendor/vendor.go", want: true, versions: pre124}, + {path: "longpackagename/vendor/vendor.go", want: false, versions: after124}, } { 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 9b351c5..3673db4 100644 --- a/zip/zip.go +++ b/zip/zip.go @@ -786,8 +786,6 @@ func (fi dataFileInfo) Sys() interface{} { return nil } // in a package whose import path contains (but does not end with) the component // "vendor". // -// Unfortunately, isVendoredPackage reports false positives for files in any -// non-top-level package whose import path ends in "vendor". // 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. @@ -803,13 +801,27 @@ func isVendoredPackage(name string, vers string) bool { if strings.HasPrefix(name, "vendor/") { i += len("vendor/") } else if j := strings.Index(name, "/vendor/"); j >= 0 { - // This offset looks incorrect; this should probably be + // Calculate the correct starting position within the import path + // to determine if a package is vendored. // - // i = j + len("/vendor/") + // Due to a bug in Go versions before 1.24 + // (see https://golang.org/issue/37397), the "/vendor/" prefix within + // a package path was not always correctly interpreted. // - // (See https://golang.org/issue/31562 and https://golang.org/issue/37397.) - // Unfortunately, we can't fix it without invalidating module checksums. - i += len("/vendor/") + // This bug affected how vendored packages were identified in cases like: + // + // - "pkg/vendor/vendor.go" (incorrectly identified as vendored in pre-1.24) + // - "pkg/vendor/foo/foo.go" (correctly identified as vendored) + // + // To correct this, in Go 1.24 and later, we skip the entire "/vendor/" prefix + // when it's part of a nested package path (as in the first example above). + // In earlier versions, we only skipped the length of "/vendor/", leading + // to the incorrect behavior. + if version.Compare(vers, "go1.24") >= 0 { + i = j + len("/vendor/") + } else { + i += len("/vendor/") + } } else { return false } @@ -950,8 +962,7 @@ func listFilesInDir(dir string) (files []File, omitted []FileError, err error) { } slashPath := filepath.ToSlash(relPath) - // Skip some subdirectories inside vendor, but maintain bug - // golang.org/issue/31562, described in isVendoredPackage. + // Skip some subdirectories inside vendor. // 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, vers) { From dec0365065b75edd0e98b0306f6f9b0051710ed2 Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Sun, 6 Oct 2024 14:39:21 +0200 Subject: [PATCH 3/3] sumdb: make data tiles by Server compatible with sum.golang.org Make the format of sumdb.Server data tile responses compatible with those served by sum.golang.org: Like formatted records for the lookup endpoint, but without each record IDs. Updates documentation for sumdb/tlog.FormatRecord about data tiles. Server still calls FormatRecord to keep the validation, then removes the first line. For golang/go#69348 Change-Id: I1bea45b3343c58acc90982aaff5d41e32b06ae8c Reviewed-on: https://go-review.googlesource.com/c/mod/+/618135 LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Matloob Reviewed-by: Dmitri Shuralyov --- sumdb/server.go | 3 +++ sumdb/tlog/note.go | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/sumdb/server.go b/sumdb/server.go index 1e1779d..216a256 100644 --- a/sumdb/server.go +++ b/sumdb/server.go @@ -6,6 +6,7 @@ package sumdb import ( + "bytes" "context" "net/http" "os" @@ -150,6 +151,8 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { http.Error(w, err.Error(), http.StatusInternalServerError) return } + // Data tiles contain formatted records without the first line with record ID. + _, msg, _ = bytes.Cut(msg, []byte{'\n'}) data = append(data, msg...) } w.Header().Set("Content-Type", "text/plain; charset=UTF-8") diff --git a/sumdb/tlog/note.go b/sumdb/tlog/note.go index ce5353e..fc6d5fa 100644 --- a/sumdb/tlog/note.go +++ b/sumdb/tlog/note.go @@ -73,13 +73,16 @@ func ParseTree(text []byte) (tree Tree, err error) { var errMalformedRecord = errors.New("malformed record data") // FormatRecord formats a record for serving to a client -// in a lookup response or data tile. +// in a lookup response. // // The encoded form is the record ID as a single number, // then the text of the record, and then a terminating blank line. // Record text must be valid UTF-8 and must not contain any ASCII control // characters (those below U+0020) other than newline (U+000A). // It must end in a terminating newline and not contain any blank lines. +// +// Responses to data tiles consist of concatenated formatted records from each of +// which the first line, with the record ID, is removed. func FormatRecord(id int64, text []byte) (msg []byte, err error) { if !isValidRecordText(text) { return nil, errMalformedRecord