From b1d336cfca975f8b4b9c88e782fbe1911b2494b0 Mon Sep 17 00:00:00 2001 From: Michael Matloob Date: Thu, 15 Aug 2024 11:08:58 -0400 Subject: [PATCH 1/6] go.mod: update required go version to go1.22 Now that go1.23 has been released, versions of Go older than go1.22 are no longer supported. This will allow us to use the go/version package, which was introduced in Go 1.22. This change will force modules that depend on golang.org/x/mod, notably golang.org/x/tools, to update their Go version requirement to at least go1.22 when they update their requirement on golang.org/x/mod to a version after this commit. For golang/go#63395 Change-Id: I6f6b5bb9e43b5f9945cc5bc8c398628436d2e739 Reviewed-on: https://go-review.googlesource.com/c/mod/+/605796 LUCI-TryBot-Result: Go LUCI Reviewed-by: Dmitri Shuralyov Reviewed-by: Dmitri Shuralyov --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index dec228a..e765791 100644 --- a/go.mod +++ b/go.mod @@ -1,5 +1,5 @@ module golang.org/x/mod -go 1.18 +go 1.22 require golang.org/x/tools v0.13.0 // tagx:ignore From 3afcd4e90a74c23515a9543f1e8fb68f05ecc8e0 Mon Sep 17 00:00:00 2001 From: Michael Matloob Date: Thu, 15 Aug 2024 12:31:23 -0400 Subject: [PATCH 2/6] go.mod: set go version to 1.22.0 The go verison was set to 1.22 but on Go versions 1.21.0 up to 1.21.10, the toolchain upgrade logic will try to download the release "1.22", which doesn't exist. Go 1.21.11+ incorporates CL 580217 (cherry-picked in CL 583797) and will download 1.22.0, so it should be fine, but set 1.22.0 to allow the upgrade for users with older local toolchains. Change-Id: I9aafaaa389ded3200b15fd3e7bb676610fa958d8 Reviewed-on: https://go-review.googlesource.com/c/mod/+/605935 Reviewed-by: Dmitri Shuralyov Commit-Queue: Michael Matloob LUCI-TryBot-Result: Go LUCI Reviewed-by: Dmitri Shuralyov --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index e765791..0339736 100644 --- a/go.mod +++ b/go.mod @@ -1,5 +1,5 @@ module golang.org/x/mod -go 1.22 +go 1.22.0 require golang.org/x/tools v0.13.0 // tagx:ignore From 46a3137daeac7bd5e64dc5971191e4a7207e6d89 Mon Sep 17 00:00:00 2001 From: Michael Matloob Date: Thu, 15 Aug 2024 12:30:22 -0400 Subject: [PATCH 3/6] zip: set GIT_DIR in test when using bare repositories If git has safe.bareRepository=explicit set, operations on bare git repos will fail unless --git-dir or GIT_DIR is set. Set GIT_DIR in the parts of the zip test that use bare repos to allow the tests to pass in those circumstances. See CL 489915 for the change setting GIT_DIR for git operations on bare repositories in cmd/go. Change-Id: I1f8ae9ed2b687a58d533fa605ed9ad4b5cbb8549 Reviewed-on: https://go-review.googlesource.com/c/mod/+/605937 Auto-Submit: Michael Matloob Reviewed-by: Dmitri Shuralyov Reviewed-by: Dmitri Shuralyov LUCI-TryBot-Result: Go LUCI --- zip/zip.go | 4 +++- zip/zip_test.go | 20 +++++++++++++++----- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/zip/zip.go b/zip/zip.go index 574f83f..5aed6e2 100644 --- a/zip/zip.go +++ b/zip/zip.go @@ -573,7 +573,9 @@ func CreateFromDir(w io.Writer, m module.Version, dir string) (err error) { // VCS repository stored locally. The zip content is written to w. // // repoRoot must be an absolute path to the base of the repository, such as -// "/Users/some-user/some-repo". +// "/Users/some-user/some-repo". If the repository is a Git repository, +// this path is expected to point to its worktree: it can't be a bare git +// repo. // // revision is the revision of the repository to create the zip from. Examples // include HEAD or SHA sums for git repositories. diff --git a/zip/zip_test.go b/zip/zip_test.go index 606e7aa..106df80 100644 --- a/zip/zip_test.go +++ b/zip/zip_test.go @@ -1325,7 +1325,7 @@ func downloadVCSZip(t testing.TB, vcs, url, rev, subdir string) (repoDir string, switch vcs { case "git": // Create a repository and download the revision we want. - if _, err := run(t, repoDir, "git", "init", "--bare"); err != nil { + if _, err := runWithGitDir(t, repoDir, repoDir, "git", "init", "--bare"); err != nil { return "", nil, err } if err := os.MkdirAll(filepath.Join(repoDir, "info"), 0777); err != nil { @@ -1342,7 +1342,7 @@ func downloadVCSZip(t testing.TB, vcs, url, rev, subdir string) (repoDir string, if err := attrFile.Close(); err != nil { return "", nil, err } - if _, err := run(t, repoDir, "git", "remote", "add", "origin", "--", url); err != nil { + if _, err := runWithGitDir(t, repoDir, repoDir, "git", "remote", "add", "origin", "--", url); err != nil { return "", nil, err } var refSpec string @@ -1351,7 +1351,7 @@ func downloadVCSZip(t testing.TB, vcs, url, rev, subdir string) (repoDir string, } else { refSpec = fmt.Sprintf("%s:refs/dummy", rev) } - if _, err := run(t, repoDir, "git", "fetch", "-f", "--depth=1", "origin", refSpec); err != nil { + if _, err := runWithGitDir(t, repoDir, repoDir, "git", "fetch", "-f", "--depth=1", "origin", refSpec); err != nil { return "", nil, err } @@ -1368,6 +1368,7 @@ func downloadVCSZip(t testing.TB, vcs, url, rev, subdir string) (repoDir string, cmd := exec.Command("git", "-c", "core.autocrlf=input", "-c", "core.eol=lf", "archive", "--format=zip", "--prefix=prefix/", rev, "--", subdirArg) cmd.Dir = repoDir + cmd.Env = append(cmd.Environ(), "GIT_DIR="+repoDir) cmd.Stdout = tmpZipFile stderr := new(strings.Builder) cmd.Stderr = stderr @@ -1425,17 +1426,20 @@ func downloadVCSFile(t testing.TB, vcs, repo, rev, file string) ([]byte, error) t.Helper() switch vcs { case "git": - return run(t, repo, "git", "cat-file", "blob", rev+":"+file) + return runWithGitDir(t, repo, repo, "git", "cat-file", "blob", rev+":"+file) default: return nil, fmt.Errorf("vcs %q not supported", vcs) } } -func run(t testing.TB, dir string, name string, args ...string) ([]byte, error) { +func runWithGitDir(t testing.TB, gitDir, dir string, name string, args ...string) ([]byte, error) { t.Helper() cmd := exec.Command(name, args...) cmd.Dir = dir + if gitDir != "" { + cmd.Env = append(cmd.Environ(), "GIT_DIR="+gitDir) + } stderr := new(strings.Builder) cmd.Stderr = stderr @@ -1450,6 +1454,12 @@ func run(t testing.TB, dir string, name string, args ...string) ([]byte, error) return out, err } +func run(t testing.TB, dir string, name string, args ...string) ([]byte, error) { + t.Helper() + + return runWithGitDir(t, "", dir, name, args...) +} + type zipFile struct { name string f *zip.File From 9cd0e4c9f675aeac595a4cbb5ba1b46798ce0fdf Mon Sep 17 00:00:00 2001 From: Sam Thanawalla Date: Thu, 9 May 2024 20:20:01 +0000 Subject: [PATCH 4/6] 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 5/6] 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 6/6] 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