diff --git a/LICENSE b/LICENSE index 6a66aea..2a7cf70 100644 --- a/LICENSE +++ b/LICENSE @@ -1,4 +1,4 @@ -Copyright (c) 2009 The Go Authors. All rights reserved. +Copyright 2009 The Go Authors. Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following conditions are @@ -10,7 +10,7 @@ notice, this list of conditions and the following disclaimer. copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution. - * Neither the name of Google Inc. nor the names of its + * Neither the name of Google LLC nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission. diff --git a/README.md b/README.md index 94da72d..4bbeeaa 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ That is, it is for direct manipulation of Go modules themselves. It is NOT about supporting general development tools that need to do things like load packages in module mode. That use case, where modules are incidental rather than the focus, -should remain in [x/tools](https://pkg.go.dev/golang/org/x/tools), +should remain in [x/tools](https://pkg.go.dev/golang.org/x/tools), specifically [x/tools/go/packages](https://pkg.go.dev/golang.org/x/tools/go/packages). The specific case of loading packages should still be done by diff --git a/go.mod b/go.mod index dec228a..c71ba17 100644 --- a/go.mod +++ b/go.mod @@ -1,5 +1,5 @@ module golang.org/x/mod -go 1.18 +go 1.23.0 require golang.org/x/tools v0.13.0 // tagx:ignore diff --git a/modfile/read.go b/modfile/read.go index 2205682..2d74868 100644 --- a/modfile/read.go +++ b/modfile/read.go @@ -226,8 +226,9 @@ func (x *FileSyntax) Cleanup() { continue } if ww == 1 && len(stmt.RParen.Comments.Before) == 0 { - // Collapse block into single line. - line := &Line{ + // Collapse block into single line but keep the Line reference used by the + // parsed File structure. + *stmt.Line[0] = Line{ Comments: Comments{ Before: commentsAdd(stmt.Before, stmt.Line[0].Before), Suffix: commentsAdd(stmt.Line[0].Suffix, stmt.Suffix), @@ -235,7 +236,7 @@ func (x *FileSyntax) Cleanup() { }, Token: stringsAdd(stmt.Token, stmt.Line[0].Token), } - x.Stmt[w] = line + x.Stmt[w] = stmt.Line[0] w++ continue } @@ -876,6 +877,11 @@ func (in *input) parseLineBlock(start Position, token []string, lparen token) *L in.Error(fmt.Sprintf("syntax error (unterminated block started at %s:%d:%d)", in.filename, x.Start.Line, x.Start.LineRune)) case ')': rparen := in.lex() + // Don't preserve blank lines (denoted by a single empty comment, added above) + // at the end of the block. + if len(comments) == 1 && comments[0] == (Comment{}) { + comments = nil + } x.RParen.Before = comments x.RParen.Pos = rparen.pos if !in.peek().isEOL() { diff --git a/modfile/read_test.go b/modfile/read_test.go index efc75e1..8f17eea 100644 --- a/modfile/read_test.go +++ b/modfile/read_test.go @@ -603,3 +603,134 @@ comments before "// k" }) } } + +func TestCleanup(t *testing.T) { + for _, test := range []struct { + desc string + want string + input []Expr + }{ + { + desc: "simple_lines", + want: `line: module a +line: require b v1.0.0 +`, + input: []Expr{ + &Line{ + Token: []string{"module", "a"}, + }, + &Line{ + Token: []string{"require", "b", "v1.0.0"}, + }, + &Line{ + Token: nil, + }, + }, + }, { + desc: "line_block", + want: `line: module a +block: require +blockline: b v1.0.0 +blockline: c v1.0.0 +`, + input: []Expr{ + &Line{ + Token: []string{"module", "a"}, + }, + &LineBlock{ + Token: []string{"require"}, + Line: []*Line{ + { + Token: []string{"b", "v1.0.0"}, + InBlock: true, + }, + { + Token: nil, + InBlock: true, + }, + { + Token: []string{"c", "v1.0.0"}, + InBlock: true, + }, + }, + }, + }, + }, { + desc: "collapse", + want: `line: module a +line: require b v1.0.0 +`, + input: []Expr{ + &Line{ + Token: []string{"module", "a"}, + }, + &LineBlock{ + Token: []string{"require"}, + Line: []*Line{ + { + Token: []string{"b", "v1.0.0"}, + InBlock: true, + }, + { + Token: nil, + InBlock: true, + }, + }, + }, + }, + }, + } { + t.Run(test.desc, func(t *testing.T) { + syntax := &FileSyntax{ + Stmt: test.input, + } + syntax.Cleanup() + + buf := &bytes.Buffer{} + for _, stmt := range syntax.Stmt { + switch stmt := stmt.(type) { + case *Line: + fmt.Fprintf(buf, "line: %v\n", strings.Join(stmt.Token, " ")) + case *LineBlock: + fmt.Fprintf(buf, "block: %v\n", strings.Join(stmt.Token, " ")) + for _, line := range stmt.Line { + fmt.Fprintf(buf, "blockline: %v\n", strings.Join(line.Token, " ")) + } + } + } + + got := strings.TrimSpace(buf.String()) + want := strings.TrimSpace(test.want) + if got != want { + t.Errorf("got:\n%s\nwant:\n%s", got, want) + } + }) + } +} + +// Issue 45130: File.Cleanup breaks references so future edits do nothing +func TestCleanupMaintainsRefs(t *testing.T) { + lineB := &Line{ + Token: []string{"b", "v1.0.0"}, + InBlock: true, + } + syntax := &FileSyntax{ + Stmt: []Expr{ + &LineBlock{ + Token: []string{"require"}, + Line: []*Line{ + lineB, + { + Token: nil, + InBlock: true, + }, + }, + }, + }, + } + syntax.Cleanup() + + if syntax.Stmt[0] != lineB { + t.Errorf("got:\n%v\nwant:\n%v", syntax.Stmt[0], lineB) + } +} diff --git a/modfile/rule.go b/modfile/rule.go index 66dcaf9..3e4a1d0 100644 --- a/modfile/rule.go +++ b/modfile/rule.go @@ -43,6 +43,7 @@ type File struct { Exclude []*Exclude Replace []*Replace Retract []*Retract + Tool []*Tool Syntax *FileSyntax } @@ -93,6 +94,12 @@ type Retract struct { Syntax *Line } +// A Tool is a single tool statement. +type Tool struct { + Path string + Syntax *Line +} + // A VersionInterval represents a range of versions with upper and lower bounds. // Intervals are closed: both bounds are included. When Low is equal to High, // the interval may refer to a single version ('v1.2.3') or an interval @@ -297,7 +304,7 @@ func parseToFile(file string, data []byte, fix VersionFixer, strict bool) (parse }) } continue - case "module", "godebug", "require", "exclude", "replace", "retract": + case "module", "godebug", "require", "exclude", "replace", "retract", "tool": for _, l := range x.Line { f.add(&errs, x, l, x.Token[0], l.Token, fix, strict) } @@ -509,6 +516,21 @@ func (f *File) add(errs *ErrorList, block *LineBlock, line *Line, verb string, a Syntax: line, } f.Retract = append(f.Retract, retract) + + case "tool": + if len(args) != 1 { + errorf("tool directive expects exactly one argument") + return + } + s, err := parseString(&args[0]) + if err != nil { + errorf("invalid quoted string: %v", err) + return + } + f.Tool = append(f.Tool, &Tool{ + Path: s, + Syntax: line, + }) } } @@ -1567,6 +1589,36 @@ func (f *File) DropRetract(vi VersionInterval) error { return nil } +// AddTool adds a new tool directive with the given path. +// It does nothing if the tool line already exists. +func (f *File) AddTool(path string) error { + for _, t := range f.Tool { + if t.Path == path { + return nil + } + } + + f.Tool = append(f.Tool, &Tool{ + Path: path, + Syntax: f.Syntax.addLine(nil, "tool", path), + }) + + f.SortBlocks() + return nil +} + +// RemoveTool removes a tool directive with the given path. +// It does nothing if no such tool directive exists. +func (f *File) DropTool(path string) error { + for _, t := range f.Tool { + if t.Path == path { + t.Syntax.markRemoved() + *t = Tool{} + } + } + return nil +} + func (f *File) SortBlocks() { f.removeDups() // otherwise sorting is unsafe @@ -1593,9 +1645,9 @@ func (f *File) SortBlocks() { } } -// removeDups removes duplicate exclude and replace directives. +// removeDups removes duplicate exclude, replace and tool directives. // -// Earlier exclude directives take priority. +// Earlier exclude and tool directives take priority. // // Later replace directives take priority. // @@ -1605,10 +1657,10 @@ func (f *File) SortBlocks() { // retract directives are not de-duplicated since comments are // meaningful, and versions may be retracted multiple times. func (f *File) removeDups() { - removeDups(f.Syntax, &f.Exclude, &f.Replace) + removeDups(f.Syntax, &f.Exclude, &f.Replace, &f.Tool) } -func removeDups(syntax *FileSyntax, exclude *[]*Exclude, replace *[]*Replace) { +func removeDups(syntax *FileSyntax, exclude *[]*Exclude, replace *[]*Replace, tool *[]*Tool) { kill := make(map[*Line]bool) // Remove duplicate excludes. @@ -1649,6 +1701,24 @@ func removeDups(syntax *FileSyntax, exclude *[]*Exclude, replace *[]*Replace) { } *replace = repl + if tool != nil { + haveTool := make(map[string]bool) + for _, t := range *tool { + if haveTool[t.Path] { + kill[t.Syntax] = true + continue + } + haveTool[t.Path] = true + } + var newTool []*Tool + for _, t := range *tool { + if !kill[t.Syntax] { + newTool = append(newTool, t) + } + } + *tool = newTool + } + // Duplicate require and retract directives are not removed. // Drop killed statements from the syntax tree. diff --git a/modfile/rule_test.go b/modfile/rule_test.go index 4d0d12a..c75a77a 100644 --- a/modfile/rule_test.go +++ b/modfile/rule_test.go @@ -1714,6 +1714,72 @@ var dropGodebugTests = []struct { }, } +var addToolTests = []struct { + desc, in, path, want string +}{ + { + `add_first`, + `module example.com/m`, + `example.com/tool/v1`, + `module example.com/m + tool example.com/tool/v1`, + }, + { + `sorted_correctly`, + `module example.com/m + tool example.com/tool2 + `, + `example.com/tool1`, + `module example.com/m + tool ( + example.com/tool1 + example.com/tool2 + )`, + }, + { + `duplicates_ignored`, + `module example.com/m + tool example.com/tool1 + `, + `example.com/tool1`, + `module example.com/m + tool example.com/tool1`, + }, +} + +var dropToolTests = []struct { + desc, in, path, want string +}{ + { + `only`, + `module example.com/m + tool example.com/tool1`, + `example.com/tool1`, + `module example.com/m`, + }, + { + `parenthesized`, + `module example.com/m + tool ( + example.com/tool1 + example.com/tool2 + )`, + `example.com/tool1`, + `module example.com/m + tool example.com/tool2`, + }, + { + `missing`, + `module example.com/m + tool ( + example.com/tool2 + )`, + `example.com/tool1`, + `module example.com/m + tool example.com/tool2`, + }, +} + func fixV(path, version string) (string, error) { if path != "example.com/m" { return "", fmt.Errorf("module path must be example.com/m") @@ -2051,6 +2117,7 @@ func TestAddOnEmptyFile(t *testing.T) { t.Fatal(err) } got, err := f.Format() + if err != nil { t.Fatal(err) } @@ -2061,3 +2128,65 @@ func TestAddOnEmptyFile(t *testing.T) { }) } } + +func TestAddTool(t *testing.T) { + for _, tt := range addToolTests { + t.Run(tt.desc, func(t *testing.T) { + inFile, err := Parse("in", []byte(tt.in), nil) + if err != nil { + t.Fatal(err) + } + if err := inFile.AddTool(tt.path); err != nil { + t.Fatal(err) + } + inFile.Cleanup() + got, err := inFile.Format() + if err != nil { + t.Fatal(err) + } + + outFile, err := Parse("out", []byte(tt.want), nil) + if err != nil { + t.Fatal(err) + } + want, err := outFile.Format() + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(got, want) { + t.Fatalf("got:\n%s\nwant:\n%s", got, want) + } + }) + } +} + +func TestDropTool(t *testing.T) { + for _, tt := range dropToolTests { + t.Run(tt.desc, func(t *testing.T) { + inFile, err := Parse("in", []byte(tt.in), nil) + if err != nil { + t.Fatal(err) + } + if err := inFile.DropTool(tt.path); err != nil { + t.Fatal(err) + } + inFile.Cleanup() + got, err := inFile.Format() + if err != nil { + t.Fatal(err) + } + + outFile, err := Parse("out", []byte(tt.want), nil) + if err != nil { + t.Fatal(err) + } + want, err := outFile.Format() + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(got, want) { + t.Fatalf("got:\n%s\nwant:\n%s", got, want) + } + }) + } +} diff --git a/modfile/testdata/issue70632.golden b/modfile/testdata/issue70632.golden new file mode 100644 index 0000000..f559879 --- /dev/null +++ b/modfile/testdata/issue70632.golden @@ -0,0 +1,7 @@ +module tidy + +go 1.23.0 + +require ( + golang.org/x/time v0.8.0 +) diff --git a/modfile/testdata/issue70632.in b/modfile/testdata/issue70632.in new file mode 100644 index 0000000..e6e6b93 --- /dev/null +++ b/modfile/testdata/issue70632.in @@ -0,0 +1,12 @@ +module tidy + +go 1.23.0 + +require ( + + "golang.org/x/time" v0.8.0 + + + + +) diff --git a/modfile/work.go b/modfile/work.go index 8f54897..5387d0c 100644 --- a/modfile/work.go +++ b/modfile/work.go @@ -331,5 +331,5 @@ func (f *WorkFile) SortBlocks() { // retract directives are not de-duplicated since comments are // meaningful, and versions may be retracted multiple times. func (f *WorkFile) removeDups() { - removeDups(f.Syntax, nil, &f.Replace) + removeDups(f.Syntax, nil, &f.Replace, nil) } 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 diff --git a/zip/testdata/check_dir/various.txt b/zip/testdata/check_dir/various.txt index ee843be..0ded5ac 100644 --- a/zip/testdata/check_dir/various.txt +++ b/zip/testdata/check_dir/various.txt @@ -1,10 +1,12 @@ -- want -- valid: $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/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 @@ -14,7 +16,9 @@ $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 -- -- .git/x -- +-- pkg/vendor/vendor.go -- diff --git a/zip/testdata/check_dir/various_go123.txt b/zip/testdata/check_dir/various_go123.txt new file mode 100644 index 0000000..69102ac --- /dev/null +++ b/zip/testdata/check_dir/various_go123.txt @@ -0,0 +1,25 @@ +-- 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/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 + +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 -- +-- pkg/vendor/vendor.go -- diff --git a/zip/testdata/check_dir/various_go124.txt b/zip/testdata/check_dir/various_go124.txt new file mode 100644 index 0000000..43bed2e --- /dev/null +++ b/zip/testdata/check_dir/various_go124.txt @@ -0,0 +1,26 @@ +-- want -- +valid: +$work/go.mod +$work/pkg/vendor/vendor.go +$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 -- +-- pkg/vendor/vendor.go -- diff --git a/zip/testdata/check_files/various.txt b/zip/testdata/check_files/various.txt index a704a8a..06ec791 100644 --- a/zip/testdata/check_files/various.txt +++ b/zip/testdata/check_files/various.txt @@ -1,11 +1,13 @@ -- want -- valid: valid.go +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 +pkg/vendor/vendor.go: file is in vendor directory invalid: not/../clean: file path is not clean @@ -17,9 +19,11 @@ 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 -- 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 new file mode 100644 index 0000000..a28be93 --- /dev/null +++ b/zip/testdata/check_files/various_go123.txt @@ -0,0 +1,30 @@ +-- 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 +pkg/vendor/vendor.go: file is in vendor directory + +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 +-- pkg/vendor/vendor.go -- diff --git a/zip/testdata/check_files/various_go124.txt b/zip/testdata/check_files/various_go124.txt new file mode 100644 index 0000000..94d4bfd --- /dev/null +++ b/zip/testdata/check_files/various_go124.txt @@ -0,0 +1,31 @@ +-- want -- +valid: +valid.go +go.mod +pkg/vendor/vendor.go + +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 +-- 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 new file mode 100644 index 0000000..940df51 --- /dev/null +++ b/zip/testdata/create/exclude_vendor_go124.txt @@ -0,0 +1,18 @@ +path=example.com/m +version=v1.0.0 +hash=h1:mJR1q75yiMK6+CDw6DuhMS4v4hNoLXqkyk2Cph4VS8Q= +-- 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 +-- 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 new file mode 100644 index 0000000..940df51 --- /dev/null +++ b/zip/testdata/create_from_dir/exclude_vendor_go124.txt @@ -0,0 +1,18 @@ +path=example.com/m +version=v1.0.0 +hash=h1:mJR1q75yiMK6+CDw6DuhMS4v4hNoLXqkyk2Cph4VS8Q= +-- 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 +-- 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 5eb9535..9deb192 100644 --- a/zip/vendor_test.go +++ b/zip/vendor_test.go @@ -6,33 +6,51 @@ 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", +} + +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? + path string + want bool + 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: 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}, - {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: 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}, } { - 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) - if tc.falsePositive { - t.Logf("(Expected a false-positive due to https://golang.org/issue/37397.)") + for _, v := range tc.versions { + got := isVendoredPackage(tc.path, v) + want := tc.want + if got != want { + t.Errorf("isVendoredPackage(%q, %s) = %t; want %t", tc.path, v, got, tc.want) } } } diff --git a/zip/zip.go b/zip/zip.go index 574f83f..3673db4 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 @@ -573,7 +603,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. @@ -754,20 +786,42 @@ 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". -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/") } 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. + // + // 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. + // + // This bug affected how vendored packages were identified in cases like: // - // i = j + len("/vendor/") + // - "pkg/vendor/vendor.go" (incorrectly identified as vendored in pre-1.24) + // - "pkg/vendor/foo/foo.go" (correctly identified as vendored) // - // (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/") + // 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 } @@ -892,6 +946,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 @@ -902,11 +962,10 @@ 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) { + if isVendoredPackage(slashPath, vers) { omitted = append(omitted, FileError{Path: slashPath, Err: errVendored}) return nil } 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