diff options
| author | rillig <rillig@pkgsrc.org> | 2019-11-23 23:35:55 +0000 |
|---|---|---|
| committer | rillig <rillig@pkgsrc.org> | 2019-11-23 23:35:55 +0000 |
| commit | 01b069e112e0d2f3857c1e06de01534b862e8bfc (patch) | |
| tree | 5049ee41a7af990eedcc0994a4da61f58da48fe7 /pkgtools | |
| parent | 98b8b1cdb72657dd290a615e0c33b6f7a814519e (diff) | |
| download | pkgsrc-01b069e112e0d2f3857c1e06de01534b862e8bfc.tar.gz | |
pkgtools/pkglint: update to 19.3.10
Changes since 19.3.9:
In diagnostics for suggested package updates, the exact line of doc/TODO
is mentioned. If a suggested update has an additional comment, the
brackets around that comment are not output anymore.
The check for defined but not used variables has been improved for the
edge case of defining a variable in the package Makefile and using it
in the buildlink3.mk file of the same package, which just doesn't work.
Makefile fragments in patches/ directories are now completely ignored.
It was a hypothetical case anyway.
Comparing PKGSRC_COMPILER using the == or != operators is now considered
an error instead of a warning. The common cases can be autofixed.
Diffstat (limited to 'pkgtools')
55 files changed, 1613 insertions, 758 deletions
diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index b459c2f70bf..c3f3bce7964 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.609 2019/11/19 06:51:38 rillig Exp $ +# $NetBSD: Makefile,v 1.610 2019/11/23 23:35:55 rillig Exp $ -PKGNAME= pkglint-19.3.9 +PKGNAME= pkglint-19.3.10 CATEGORIES= pkgtools DISTNAME= tools MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/} diff --git a/pkgtools/pkglint/PLIST b/pkgtools/pkglint/PLIST index 076c8201878..c3dc5211938 100644 --- a/pkgtools/pkglint/PLIST +++ b/pkgtools/pkglint/PLIST @@ -1,4 +1,4 @@ -@comment $NetBSD: PLIST,v 1.16 2019/11/17 01:26:25 rillig Exp $ +@comment $NetBSD: PLIST,v 1.17 2019/11/23 23:35:55 rillig Exp $ bin/pkglint gopkg/pkg/${GO_PLATFORM}/netbsd.org/pkglint.a gopkg/pkg/${GO_PLATFORM}/netbsd.org/pkglint/getopt.a @@ -77,6 +77,8 @@ gopkg/src/netbsd.org/pkglint/paragraph.go gopkg/src/netbsd.org/pkglint/paragraph_test.go gopkg/src/netbsd.org/pkglint/patches.go gopkg/src/netbsd.org/pkglint/patches_test.go +gopkg/src/netbsd.org/pkglint/path.go +gopkg/src/netbsd.org/pkglint/path_test.go gopkg/src/netbsd.org/pkglint/pkglint.0 gopkg/src/netbsd.org/pkglint/pkglint.1 gopkg/src/netbsd.org/pkglint/pkglint.go diff --git a/pkgtools/pkglint/files/alternatives.go b/pkgtools/pkglint/files/alternatives.go index 13e01958d1b..7c16b97b43f 100644 --- a/pkgtools/pkglint/files/alternatives.go +++ b/pkgtools/pkglint/files/alternatives.go @@ -5,7 +5,7 @@ import ( "strings" ) -func CheckFileAlternatives(filename string) { +func CheckFileAlternatives(filename Path) { lines := Load(filename, NotEmpty|LogErrors) if lines == nil { return @@ -16,7 +16,7 @@ func CheckFileAlternatives(filename string) { plist = G.Pkg.Plist } - checkPlistWrapper := func(line *Line, wrapper string) { + checkPlistWrapper := func(line *Line, wrapper Path) { if plist.Files[wrapper] != nil { line.Errorf("Alternative wrapper %q must not appear in the PLIST.", wrapper) } @@ -25,10 +25,10 @@ func CheckFileAlternatives(filename string) { checkPlistAlternative := func(line *Line, alternative string) { relImplementation := strings.Replace(alternative, "@PREFIX@/", "", 1) plistName := replaceAll(relImplementation, `@(\w+)@`, "${$1}") - if plist.Files[plistName] != nil || G.Pkg.vars.IsDefined("ALTERNATIVES_SRC") { + if plist.Files[NewPath(plistName)] != nil || G.Pkg.vars.IsDefined("ALTERNATIVES_SRC") { return } - if plist.Files[strings.Replace(plistName, "${PKGMANDIR}", "man", 1)] != nil { + if plist.Files[NewPath(strings.Replace(plistName, "${PKGMANDIR}", "man", 1))] != nil { return } @@ -57,7 +57,7 @@ func CheckFileAlternatives(filename string) { } if plist.Files != nil { - checkPlistWrapper(line, wrapper) + checkPlistWrapper(line, NewPath(wrapper)) checkPlistAlternative(line, alternative) } diff --git a/pkgtools/pkglint/files/autofix.go b/pkgtools/pkglint/files/autofix.go index 62a50315f49..0ab132f3e28 100644 --- a/pkgtools/pkglint/files/autofix.go +++ b/pkgtools/pkglint/files/autofix.go @@ -1,10 +1,8 @@ package pkglint import ( - "io/ioutil" "netbsd.org/pkglint/regex" "os" - "path/filepath" "strconv" "strings" ) @@ -455,12 +453,12 @@ func SaveAutofixChanges(lines *Lines) (autofixed bool) { if G.Testing { abs := abspath(lines.Filename) - absTmp := abspath(filepath.ToSlash(os.TempDir())) - assertf(hasPrefix(abs, absTmp), "%q must be inside %q", abs, absTmp) + absTmp := abspath(NewPathSlash(os.TempDir())) + assertf(abs.HasPrefixPath(absTmp), "%q must be inside %q", abs, absTmp) } - changes := make(map[string][]string) - changed := make(map[string]bool) + changes := make(map[Path][]string) + changed := make(map[Path]bool) for _, line := range lines.Lines { chlines := changes[line.Filename] if fix := line.autofix; fix != nil { @@ -488,12 +486,12 @@ func SaveAutofixChanges(lines *Lines) (autofixed bool) { for _, changedLine := range changedLines { text.WriteString(changedLine) } - err := ioutil.WriteFile(tmpName, []byte(text.String()), 0666) + err := tmpName.WriteString(text.String()) if err != nil { G.Logger.Errorf(tmpName, "Cannot write: %s", err) continue } - err = os.Rename(tmpName, filename) + err = tmpName.Rename(filename) if err != nil { G.Logger.Errorf(tmpName, "Cannot overwrite with autofixed content: %s", err) continue diff --git a/pkgtools/pkglint/files/autofix_test.go b/pkgtools/pkglint/files/autofix_test.go index b29aee0e8da..cd292c36350 100644 --- a/pkgtools/pkglint/files/autofix_test.go +++ b/pkgtools/pkglint/files/autofix_test.go @@ -1282,7 +1282,7 @@ func (s *Suite) Test_SaveAutofixChanges__file_removed(c *check.C) { t.SetUpCommandLine("--autofix") lines := t.SetUpFileLines("subdir/file.txt", "line 1") - _ = os.RemoveAll(t.File("subdir")) + _ = os.RemoveAll(t.File("subdir").String()) fix := lines.Lines[0].Autofix() fix.Warnf("Should start with an uppercase letter.") @@ -1308,7 +1308,7 @@ func (s *Suite) Test_SaveAutofixChanges__file_busy_Windows(c *check.C) { "line 1") // As long as the file is kept open, it cannot be overwritten or deleted. - openFile, err := os.OpenFile(t.File("subdir/file.txt"), 0, 0666) + openFile, err := os.OpenFile(t.File("subdir/file.txt").String(), 0, 0666) // TODO: replace with Path.Open defer func() { assertNil(openFile.Close(), "") }() c.Check(err, check.IsNil) @@ -1336,8 +1336,8 @@ func (s *Suite) Test_SaveAutofixChanges__cannot_overwrite(c *check.C) { lines := t.SetUpFileLines("file.txt", "line 1") - c.Check(os.RemoveAll(t.File("file.txt")), check.IsNil) - c.Check(os.MkdirAll(t.File("file.txt"), 0777), check.IsNil) + c.Check(os.RemoveAll(t.File("file.txt").String()), check.IsNil) + c.Check(os.MkdirAll(t.File("file.txt").String(), 0777), check.IsNil) fix := lines.Lines[0].Autofix() fix.Warnf("Should start with an uppercase letter.") diff --git a/pkgtools/pkglint/files/buildlink3.go b/pkgtools/pkglint/files/buildlink3.go index c8ee205dd27..a260b3c323a 100644 --- a/pkgtools/pkglint/files/buildlink3.go +++ b/pkgtools/pkglint/files/buildlink3.go @@ -2,7 +2,6 @@ package pkglint import ( "netbsd.org/pkglint/pkgver" - "path" "strings" ) @@ -21,7 +20,7 @@ func CheckLinesBuildlink3Mk(mklines *MkLines) { func (ck *Buildlink3Checker) Check() { mklines := ck.mklines if trace.Tracing { - defer trace.Call1(mklines.lines.Filename)() + defer trace.Call(mklines.lines.Filename)() } mklines.Check() @@ -40,7 +39,7 @@ func (ck *Buildlink3Checker) Check() { if llex.SkipRegexp(`^BUILDLINK_DEPMETHOD\.([^\t ]+)\?=.*$`) { llex.PreviousLine().Warnf("This line belongs inside the .ifdef block.") - for llex.SkipString("") { + for llex.SkipText("") { } } @@ -100,7 +99,7 @@ func (ck *Buildlink3Checker) checkUniquePkgbase(pkgbase string, mkline *MkLine) return } - base, name := trimCommon(pkgbase, path.Base(path.Dir(mkline.Filename))) + base, name := trimCommon(pkgbase, mkline.Filename.Dir().Base()) if base == "" && matches(name, `^(\d*|-cvs|-fossil|-git|-hg|-svn|-devel|-snapshot)$`) { return } diff --git a/pkgtools/pkglint/files/buildlink3_test.go b/pkgtools/pkglint/files/buildlink3_test.go index 89e618c1a70..0eddb084648 100644 --- a/pkgtools/pkglint/files/buildlink3_test.go +++ b/pkgtools/pkglint/files/buildlink3_test.go @@ -599,7 +599,7 @@ func (s *Suite) Test_Buildlink3Checker_checkUniquePkgbase(c *check.C) { G.InterPackage.Enable() - test := func(pkgbase, pkgpath string, diagnostics ...string) { + test := func(pkgbase string, pkgpath Path, diagnostics ...string) { mkline := t.NewMkLine(t.File(pkgpath+"/buildlink3.mk"), 123, "") (*Buildlink3Checker).checkUniquePkgbase(nil, pkgbase, mkline) diff --git a/pkgtools/pkglint/files/category.go b/pkgtools/pkglint/files/category.go index 5d1a66d5cf5..77eca30a4f3 100644 --- a/pkgtools/pkglint/files/category.go +++ b/pkgtools/pkglint/files/category.go @@ -6,12 +6,12 @@ import ( "strings" ) -func CheckdirCategory(dir string) { +func CheckdirCategory(dir Path) { if trace.Tracing { - defer trace.Call1(dir)() + defer trace.Call(dir)() } - mklines := LoadMk(dir+"/Makefile", NotEmpty|LogErrors) + mklines := LoadMk(dir.JoinNoClean("Makefile"), NotEmpty|LogErrors) // TODO: Remove the "./" here already if mklines == nil { return } @@ -49,7 +49,7 @@ func CheckdirCategory(dir string) { mlex.SkipEmptyOrNote() type subdir struct { - name string + name Path line *MkLine } @@ -67,7 +67,7 @@ func CheckdirCategory(dir string) { if (mkline.IsVarassignMaybeCommented()) && mkline.Varname() == "SUBDIR" { mlex.Skip() - name := mkline.Value() + name := mkline.Value() // TODO: Maybe NewPath here already if mkline.IsCommentedVarassign() && !mkline.HasComment() { mkline.Warnf("%q commented out without giving a reason.", name) } @@ -78,12 +78,12 @@ func CheckdirCategory(dir string) { seen[name] = mkline if len(mSubdirs) > 0 { - if prev := mSubdirs[len(mSubdirs)-1].name; name < prev { + if prev := mSubdirs[len(mSubdirs)-1].name; name < prev.String() { mkline.Warnf("%q should come before %q.", name, prev) } } - mSubdirs = append(mSubdirs, subdir{name, mkline}) + mSubdirs = append(mSubdirs, subdir{NewPath(name), mkline}) } else { if !mkline.IsEmpty() { @@ -96,8 +96,8 @@ func CheckdirCategory(dir string) { // To prevent unnecessary warnings about subdirectories that are // in one list but not in the other, generate the sets of // subdirs of each list. - fCheck := make(map[string]bool) - mCheck := make(map[string]bool) + fCheck := make(map[Path]bool) + mCheck := make(map[Path]bool) for _, fsub := range fSubdirs { fCheck[fsub] = true } @@ -122,7 +122,7 @@ func CheckdirCategory(dir string) { fix := line.Autofix() fix.Errorf("%q exists in the file system but not in the Makefile.", fCurrent) - fix.InsertBefore("SUBDIR+=\t" + fCurrent) + fix.InsertBefore("SUBDIR+=\t" + fCurrent.String()) fix.Apply() } fRest = fRest[1:] @@ -155,7 +155,7 @@ func CheckdirCategory(dir string) { mklines.SaveAutofixChanges() if G.Opts.Recursive { - var recurseInto []string + var recurseInto []Path for _, msub := range mSubdirs { if !msub.line.IsCommentedVarassign() { recurseInto = append(recurseInto, joinPath(dir, msub.name)) diff --git a/pkgtools/pkglint/files/category_test.go b/pkgtools/pkglint/files/category_test.go index 6d6a8a10499..553373ad7b0 100644 --- a/pkgtools/pkglint/files/category_test.go +++ b/pkgtools/pkglint/files/category_test.go @@ -208,12 +208,12 @@ func (s *Suite) Test_CheckdirCategory__recursive(c *check.C) { // It is only removed in Pkglint.Main, therefore it stays there even // after the call to CheckdirCategory. This is a bit unrealistic, // but close enough for this test. - t.CheckDeepEquals(G.Todo.entries, []string{"."}) + t.CheckDeepEquals(G.Todo.entries, []Path{"."}) CheckdirCategory(".") t.CheckOutputEmpty() - t.CheckDeepEquals(G.Todo.entries, []string{"./package", "."}) + t.CheckDeepEquals(G.Todo.entries, []Path{"./package", "."}) } // Ensures that a directory in the file system can be added at the very diff --git a/pkgtools/pkglint/files/check_test.go b/pkgtools/pkglint/files/check_test.go index b0f1d69c1af..6f000552703 100644 --- a/pkgtools/pkglint/files/check_test.go +++ b/pkgtools/pkglint/files/check_test.go @@ -7,8 +7,6 @@ import ( "io/ioutil" "netbsd.org/pkglint/regex" "os" - "path" - "path/filepath" "regexp" "strings" "testing" @@ -128,9 +126,9 @@ type Tester struct { stdout bytes.Buffer stderr bytes.Buffer - tmpdir string + tmpdir Path prevdir string // The current working directory before the test started - relCwd string // See Tester.Chdir + relCwd Path // See Tester.Chdir seenSetUpCommandLine bool seenSetupPkgsrc int @@ -206,7 +204,7 @@ func (t *Tester) SetUpTool(name, varname string, validity Validity) *Tool { // The file is then read in, without interpreting line continuations. // // See SetUpFileMkLines for loading a Makefile fragment. -func (t *Tester) SetUpFileLines(relativeFileName string, lines ...string) *Lines { +func (t *Tester) SetUpFileLines(relativeFileName Path, lines ...string) *Lines { filename := t.CreateFileLines(relativeFileName, lines...) return Load(filename, MustSucceed) } @@ -215,7 +213,7 @@ func (t *Tester) SetUpFileLines(relativeFileName string, lines ...string) *Lines // The file is then read in, handling line continuations for Makefiles. // // See SetUpFileLines for loading an ordinary file. -func (t *Tester) SetUpFileMkLines(relativeFileName string, lines ...string) *MkLines { +func (t *Tester) SetUpFileMkLines(relativeFileName Path, lines ...string) *MkLines { filename := t.CreateFileLines(relativeFileName, lines...) return LoadMk(filename, MustSucceed) } @@ -224,15 +222,15 @@ func (t *Tester) SetUpFileMkLines(relativeFileName string, lines ...string) *MkL // merging all the lines into a single MkLines object. // // This is useful for testing code related to Package.readMakefile. -func (t *Tester) LoadMkInclude(relativeFileName string) *MkLines { +func (t *Tester) LoadMkInclude(relativeFileName Path) *MkLines { var lines []*Line // TODO: Include files with multiple-inclusion guard only once. // TODO: Include files without multiple-inclusion guard as often as needed. // TODO: Set an upper limit, to prevent denial of service. - var load func(filename string) - load = func(filename string) { + var load func(filename Path) + load = func(filename Path) { for _, mkline := range NewMkLines(Load(filename, MustSucceed)).mklines { lines = append(lines, mkline.Line) @@ -332,11 +330,12 @@ func (t *Tester) SetUpPkgsrc() { // SetUpCategory makes the given category valid by creating a dummy Makefile. // After that, it can be mentioned in the CATEGORIES variable of a package. -func (t *Tester) SetUpCategory(name string) { - assert(!contains(name, "/")) // Category must not contain a slash. +func (t *Tester) SetUpCategory(name Path) { + assert(name.Count() == 1) - if _, err := os.Stat(t.File(name + "/Makefile")); os.IsNotExist(err) { - t.CreateFileLines(name+"/Makefile", + makefile := name.JoinNoClean("Makefile") + if !t.File(makefile).IsFile() { + t.CreateFileLines(makefile, MkCvsID) } } @@ -352,11 +351,11 @@ func (t *Tester) SetUpCategory(name string) { // After calling this method, individual files can be overwritten as necessary. // At the end of the setup phase, t.FinishSetUp() must be called to load all // the files. -func (t *Tester) SetUpPackage(pkgpath string, makefileLines ...string) string { - assertf(matches(pkgpath, `^[^/]+/[^/]+$`), "pkgpath %q must have the form \"category/package\"", pkgpath) +func (t *Tester) SetUpPackage(pkgpath Path, makefileLines ...string) Path { + assertf(matches(pkgpath.String(), `^[^/]+/[^/]+$`), "pkgpath %q must have the form \"category/package\"", pkgpath) - distname := path.Base(pkgpath) - category := path.Dir(pkgpath) + distname := pkgpath.Base() + category := pkgpath.Dir() if category == "wip" { // To avoid boilerplate CATEGORIES definitions for wip packages. category = "local" @@ -365,9 +364,9 @@ func (t *Tester) SetUpPackage(pkgpath string, makefileLines ...string) string { t.SetUpPkgsrc() t.SetUpCategory(category) - t.CreateFileLines(pkgpath+"/DESCR", + t.CreateFileLines(pkgpath.JoinNoClean("DESCR"), "Package description") - t.CreateFileLines(pkgpath+"/PLIST", + t.CreateFileLines(pkgpath.JoinNoClean("PLIST"), PlistCvsID, "bin/program") @@ -377,12 +376,12 @@ func (t *Tester) SetUpPackage(pkgpath string, makefileLines ...string) string { // correct position. To prevent the tests from having to mention the // unrelated warnings about the variable order, that check is suppressed // here. - t.CreateFileLines(pkgpath+"/suppress-varorder.mk", + t.CreateFileLines(pkgpath.JoinNoClean("suppress-varorder.mk"), MkCvsID) // This distinfo file contains dummy hashes since pkglint cannot check the // distfiles hashes anyway. It can only check the hashes for the patches. - t.CreateFileLines(pkgpath+"/distinfo", + t.CreateFileLines(pkgpath.JoinNoClean("distinfo"), CvsID, "", "SHA1 (distfile-1.0.tar.gz) = 12341234", @@ -395,7 +394,7 @@ func (t *Tester) SetUpPackage(pkgpath string, makefileLines ...string) string { "", "DISTNAME=\t" + distname + "-1.0", "#PKGNAME=\tpackage-1.0", - "CATEGORIES=\t" + category, + "CATEGORIES=\t" + category.String(), "MASTER_SITES=\t# none", "", "MAINTAINER=\tpkgsrc-users@NetBSD.org", @@ -433,7 +432,7 @@ line: "", ".include \"../../mk/bsd.pkg.mk\"") - t.CreateFileLines(pkgpath+"/Makefile", + t.CreateFileLines(pkgpath.JoinNoClean("Makefile"), mlines...) return t.File(pkgpath) @@ -443,18 +442,18 @@ line: // given lines to it. // // It returns the full path to the created file. -func (t *Tester) CreateFileLines(relativeFileName string, lines ...string) (filename string) { - var content bytes.Buffer +func (t *Tester) CreateFileLines(relativeFileName Path, lines ...string) (filename Path) { + var content strings.Builder for _, line := range lines { content.WriteString(line) content.WriteString("\n") } filename = t.File(relativeFileName) - err := os.MkdirAll(path.Dir(filename), 0777) + err := os.MkdirAll(filename.Dir().String(), 0777) t.c.Assert(err, check.IsNil) - err = ioutil.WriteFile(filename, content.Bytes(), 0666) + err = filename.WriteString(content.String()) t.c.Assert(err, check.IsNil) G.fileCache.Evict(filename) @@ -464,7 +463,7 @@ func (t *Tester) CreateFileLines(relativeFileName string, lines ...string) (file // CreateFileDummyPatch creates a patch file with the given name in the // temporary directory. -func (t *Tester) CreateFileDummyPatch(relativeFileName string) { +func (t *Tester) CreateFileDummyPatch(relativeFileName Path) { t.CreateFileLines(relativeFileName, CvsID, "", @@ -477,9 +476,10 @@ func (t *Tester) CreateFileDummyPatch(relativeFileName string) { "+new") } -func (t *Tester) CreateFileDummyBuildlink3(relativeFileName string, customLines ...string) { - dir := path.Dir(relativeFileName) - lower := path.Base(dir) +func (t *Tester) CreateFileDummyBuildlink3(relativeFileName Path, customLines ...string) { + assert(relativeFileName.Count() == 3) + dir := relativeFileName.Dir() + lower := dir.Base() // see pkgtools/createbuildlink/files/createbuildlink, "package specific variables" upper := strings.Replace(strings.ToUpper(lower), "-", "_", -1) @@ -519,26 +519,26 @@ func (t *Tester) CreateFileDummyBuildlink3(relativeFileName string, customLines // File returns the absolute path to the given file in the // temporary directory. It doesn't check whether that file exists. // Calls to Tester.Chdir change the base directory for the relative filename. -func (t *Tester) File(relativeFileName string) string { +func (t *Tester) File(relativeFileName Path) Path { if t.tmpdir == "" { - t.tmpdir = filepath.ToSlash(t.c.MkDir()) + t.tmpdir = NewPathSlash(t.c.MkDir()) } if t.relCwd != "" { - return path.Clean(relativeFileName) + return relativeFileName.Clean() } - return path.Clean(joinPath(t.tmpdir, relativeFileName)) + return t.tmpdir.JoinClean(relativeFileName) } // Copy copies a file inside the temporary directory. -func (t *Tester) Copy(relativeSrc, relativeDst string) { +func (t *Tester) Copy(relativeSrc, relativeDst Path) { src := t.File(relativeSrc) dst := t.File(relativeDst) - data, err := ioutil.ReadFile(src) + data, err := src.ReadString() assertNil(err, "Copy.Read") - err = os.MkdirAll(path.Dir(dst), 0777) + err = os.MkdirAll(dst.Dir().String(), 0777) assertNil(err, "Copy.MkdirAll") - err = ioutil.WriteFile(dst, data, 0777) + err = dst.WriteString(data) assertNil(err, "Copy.Write") } @@ -554,7 +554,7 @@ func (t *Tester) Copy(relativeSrc, relativeDst string) { // // As long as this method is not called in a test, the current working // directory is indeterminate. -func (t *Tester) Chdir(relativeDirName string) { +func (t *Tester) Chdir(relativeDirName Path) { if t.relCwd != "" { // When multiple calls of Chdir are mixed with calls to CreateFileLines, // the resulting Lines and MkLines variables will use relative filenames, @@ -564,17 +564,17 @@ func (t *Tester) Chdir(relativeDirName string) { } absDirName := t.File(relativeDirName) - assertNil(os.MkdirAll(absDirName, 0700), "MkDirAll") - assertNil(os.Chdir(absDirName), "Chdir") + assertNil(os.MkdirAll(absDirName.String(), 0700), "MkDirAll") + assertNil(os.Chdir(absDirName.String()), "Chdir") t.relCwd = relativeDirName G.cwd = absDirName } // Remove removes the file or directory from the temporary directory. // The file or directory must exist. -func (t *Tester) Remove(relativeFileName string) { +func (t *Tester) Remove(relativeFileName Path) { filename := t.File(relativeFileName) - err := os.Remove(filename) + err := os.Remove(filename.String()) t.c.Assert(err, check.IsNil) G.fileCache.Evict(filename) } @@ -604,12 +604,12 @@ func (t *Tester) Remove(relativeFileName string) { // subdir/module.mk includes subdir/version.mk, the include line is just: // .include "version.mk" func (t *Tester) SetUpHierarchy() ( - include func(filename string, args ...interface{}) *MkLines, - get func(string) *MkLines) { + include func(filename Path, args ...interface{}) *MkLines, + get func(Path) *MkLines) { - files := map[string]*MkLines{} + files := map[Path]*MkLines{} - include = func(filename string, args ...interface{}) *MkLines { + include = func(filename Path, args ...interface{}) *MkLines { var lines []*Line lineno := 1 @@ -623,7 +623,7 @@ func (t *Tester) SetUpHierarchy() ( case string: addLine(arg) case *MkLines: - text := sprintf(".include %q", relpath(path.Dir(filename), arg.lines.Filename)) + text := sprintf(".include %q", relpath(filename.Dir(), arg.lines.Filename)) addLine(text) lines = append(lines, arg.lines.Lines...) default: @@ -637,7 +637,7 @@ func (t *Tester) SetUpHierarchy() ( return mklines } - get = func(filename string) *MkLines { + get = func(filename Path) *MkLines { assertf(files[filename] != nil, "MkLines with name %q doesn't exist.", filename) return files[filename] } @@ -715,10 +715,9 @@ func (t *Tester) Main(args ...string) int { argv := []string{"pkglint"} for _, arg := range args { - fileArg := t.File(arg) - _, err := os.Lstat(fileArg) - if err == nil { - argv = append(argv, fileArg) + fileArg := t.File(NewPath(arg)) + if fileArg.Exists() { + argv = append(argv, fileArg.String()) } else { argv = append(argv, arg) } @@ -845,15 +844,15 @@ func (t *Tester) NewRawLines(args ...interface{}) []*RawLine { // NewLine creates an in-memory line with the given text. // This line does not correspond to any line in a file. -func (t *Tester) NewLine(filename string, lineno int, text string) *Line { +func (t *Tester) NewLine(filename Path, lineno int, text string) *Line { textnl := text + "\n" rawLine := RawLine{lineno, textnl, textnl} return NewLine(filename, lineno, text, &rawLine) } // NewMkLine creates an in-memory line in the Makefile format with the given text. -func (t *Tester) NewMkLine(filename string, lineno int, text string) *MkLine { - basename := path.Base(filename) +func (t *Tester) NewMkLine(filename Path, lineno int, text string) *MkLine { + basename := filename.Base() assertf( hasSuffix(basename, ".mk") || basename == "Makefile" || @@ -872,14 +871,14 @@ func (t *Tester) NewShellLineChecker(text string) *ShellLineChecker { // NewLines returns a list of simple lines that belong together. // // To work with line continuations like in Makefiles, use SetUpFileMkLines. -func (t *Tester) NewLines(filename string, lines ...string) *Lines { +func (t *Tester) NewLines(filename Path, lines ...string) *Lines { return t.NewLinesAt(filename, 1, lines...) } // NewLinesAt returns a list of simple lines that belong together. // // To work with line continuations like in Makefiles, use SetUpFileMkLines. -func (t *Tester) NewLinesAt(filename string, firstLine int, texts ...string) *Lines { +func (t *Tester) NewLinesAt(filename Path, firstLine int, texts ...string) *Lines { lines := make([]*Line, len(texts)) for i, text := range texts { lines[i] = t.NewLine(filename, i+firstLine, text) @@ -893,8 +892,8 @@ func (t *Tester) NewLinesAt(filename string, firstLine int, texts ...string) *Li // // No actual file is created for the lines; // see SetUpFileMkLines for loading Makefile fragments with line continuations. -func (t *Tester) NewMkLines(filename string, lines ...string) *MkLines { - basename := path.Base(filename) +func (t *Tester) NewMkLines(filename Path, lines ...string) *MkLines { + basename := filename.Base() assertf( hasSuffix(basename, ".mk") || basename == "Makefile" || hasPrefix(basename, "Makefile."), "filename %q must be realistic, otherwise the variable permissions are wrong", filename) @@ -924,7 +923,7 @@ func (t *Tester) Output() string { } assertf(t.tmpdir != "", "Tester must be initialized before checking the output.") - return strings.Replace(stdout+stderr, t.tmpdir, "~", -1) + return strings.Replace(stdout+stderr, t.tmpdir.String(), "~", -1) } // CheckOutputEmpty ensures that the output up to now is empty. @@ -983,14 +982,14 @@ func (t *Tester) CheckOutputLinesIgnoreSpace(expectedLines ...string) { t.CheckDeepEquals(actual, expected) } -func (t *Tester) compareOutputIgnoreSpace(rawOutput string, expectedLines []string, tmpdir string) ([]string, []string) { +func (t *Tester) compareOutputIgnoreSpace(rawOutput string, expectedLines []string, tmpdir Path) ([]string, []string) { whitespace := regexp.MustCompile(`\s+`) // Replace all occurrences of tmpdir in the raw output with a tilde, // also covering cases where tmpdir is wrapped into multiple lines. output := func() string { var tmpdirPattern strings.Builder - for i, part := range whitespace.Split(tmpdir, -1) { + for i, part := range whitespace.Split(tmpdir.String(), -1) { if i > 0 { tmpdirPattern.WriteString("\\s+") } @@ -1017,7 +1016,7 @@ func (s *Suite) Test_Tester_compareOutputIgnoreSpace(c *check.C) { t := s.Init(c) lines := func(lines ...string) []string { return lines } - test := func(rawOutput string, expectedLines []string, tmpdir string, eq bool) { + test := func(rawOutput string, expectedLines []string, tmpdir Path, eq bool) { actual, expected := t.compareOutputIgnoreSpace(rawOutput, expectedLines, tmpdir) t.CheckEquals(actual == nil && expected == nil, eq) } @@ -1137,10 +1136,10 @@ func (t *Tester) DisableTracing() { // CheckFileLines loads the lines from the temporary file and checks that // they equal the given lines. -func (t *Tester) CheckFileLines(relativeFileName string, lines ...string) { - content, err := ioutil.ReadFile(t.File(relativeFileName)) +func (t *Tester) CheckFileLines(relativeFileName Path, lines ...string) { + content, err := t.File(relativeFileName).ReadString() t.c.Assert(err, check.IsNil) - actualLines := strings.Split(string(content), "\n") + actualLines := strings.Split(content, "\n") actualLines = actualLines[:len(actualLines)-1] t.CheckDeepEquals(emptyToNil(actualLines), emptyToNil(lines)) } @@ -1149,7 +1148,7 @@ func (t *Tester) CheckFileLines(relativeFileName string, lines ...string) { // that they equal the given lines. The loaded file may use tabs or spaces // for indentation, while the lines in the code use spaces exclusively, // in order to make the depth of the indentation clearly visible in the test code. -func (t *Tester) CheckFileLinesDetab(relativeFileName string, lines ...string) { +func (t *Tester) CheckFileLinesDetab(relativeFileName Path, lines ...string) { actualLines := Load(t.File(relativeFileName), MustSucceed) var detabbedLines []string @@ -1167,11 +1166,11 @@ func (t *Tester) CheckFileLinesDetab(relativeFileName string, lines ...string) { // development. func (t *Tester) Use(...interface{}) {} -func (t *Tester) Shquote(format string, rels ...string) string { +func (t *Tester) Shquote(format string, rels ...Path) string { var subs []interface{} for _, rel := range rels { - quoted := shquote(path.Join(t.tmpdir, rel)) - subs = append(subs, strings.Replace(quoted, t.tmpdir, "~", -1)) + quoted := shquote(t.tmpdir.JoinClean(rel).String()) + subs = append(subs, strings.Replace(quoted, t.tmpdir.String(), "~", -1)) } return sprintf(format, subs...) } diff --git a/pkgtools/pkglint/files/distinfo.go b/pkgtools/pkglint/files/distinfo.go index b15776a365f..b93b9552cf2 100644 --- a/pkgtools/pkglint/files/distinfo.go +++ b/pkgtools/pkglint/files/distinfo.go @@ -8,29 +8,27 @@ import ( "golang.org/x/crypto/ripemd160" "hash" "io" - "io/ioutil" - "os" "strings" ) func CheckLinesDistinfo(pkg *Package, lines *Lines) { if trace.Tracing { - defer trace.Call1(lines.Filename)() + defer trace.Call(lines.Filename)() } filename := lines.Filename - patchdir := "patches" - if pkg != nil && dirExists(pkg.File(pkg.Patchdir)) { + patchdir := NewPath("patches") + if pkg != nil && pkg.File(pkg.Patchdir).IsDir() { patchdir = pkg.Patchdir } if trace.Tracing { - trace.Step1("patchdir=%q", patchdir) + trace.Stepf("patchdir=%q", patchdir) } distinfoIsCommitted := isCommitted(filename) ck := distinfoLinesChecker{ pkg, lines, patchdir, distinfoIsCommitted, - nil, make(map[string]distinfoFileInfo)} + nil, make(map[Path]distinfoFileInfo)} ck.parse() ck.check() CheckLinesTrailingEmptyLines(lines) @@ -42,11 +40,11 @@ func CheckLinesDistinfo(pkg *Package, lines *Lines) { type distinfoLinesChecker struct { pkg *Package lines *Lines - patchdir string // Relative to pkg + patchdir Path // Relative to pkg distinfoIsCommitted bool - filenames []string // For keeping the order from top to bottom - infos map[string]distinfoFileInfo + filenames []Path // For keeping the order from top to bottom + infos map[Path]distinfoFileInfo } func (ck *distinfoLinesChecker) parse() { @@ -58,16 +56,16 @@ func (ck *distinfoLinesChecker) parse() { } llex.SkipEmptyOrNote() - prevFilename := "" + prevFilename := NewPath("") var hashes []distinfoHash isPatch := func() YesNoUnknown { switch { - case !hasPrefix(prevFilename, "patch-"): + case !prevFilename.HasPrefixText("patch-"): return no case ck.pkg == nil: return unknown - case fileExists(ck.pkg.File(joinPath(ck.patchdir, prevFilename))): + case ck.pkg.File(ck.patchdir.JoinNoClean(prevFilename)).IsFile(): return yes default: return no @@ -84,7 +82,8 @@ func (ck *distinfoLinesChecker) parse() { line := llex.CurrentLine() llex.Skip() - m, alg, filename, hash := match3(line.Text, `^(\w+) \((\w[^)]*)\) = (\S+(?: bytes)?)$`) + m, alg, file, hash := match3(line.Text, `^(\w+) \((\w[^)]*)\) = (\S+(?: bytes)?)$`) + filename := NewPath(file) if !m { line.Errorf("Invalid line: %s", line.Text) continue @@ -146,7 +145,7 @@ func (ck *distinfoLinesChecker) checkAlgorithms(info distinfoFileInfo) { // At this point, the file is either a missing patch file or a distfile. - case hasPrefix(filename, "patch-") && algorithms == "SHA1": + case filename.HasPrefixText("patch-") && algorithms == "SHA1": if ck.pkg.IgnoreMissingPatches { break } @@ -196,7 +195,7 @@ func (ck *distinfoLinesChecker) checkAlgorithmsDistfile(info distinfoFileInfo) { distdir := G.Pkgsrc.File("distfiles") distfile := cleanpath(joinPath(distdir, info.filename())) - if !fileExists(distfile) { + if !distfile.IsFile() { // It's a rare situation that the explanation is generated // this far from the corresponding diagnostic. @@ -225,7 +224,7 @@ func (ck *distinfoLinesChecker) checkAlgorithmsDistfile(info distinfoFileInfo) { } computeHash := func(hasher hash.Hash) string { - f, err := os.Open(distfile) + f, err := distfile.Open() assertNil(err, "Opening distfile") // Don't load the distfile into memory since some of them @@ -250,7 +249,7 @@ func (ck *distinfoLinesChecker) checkAlgorithmsDistfile(info distinfoFileInfo) { case "SHA512": return computeHash(sha512.New()) default: - fileInfo, err := os.Lstat(distfile) + fileInfo, err := distfile.Lstat() assertNil(err, "Inaccessible distfile info") return sprintf("%d bytes", fileInfo.Size()) } @@ -302,7 +301,7 @@ func (ck *distinfoLinesChecker) checkUnrecordedPatches() { if ck.pkg == nil { return } - patchFiles, err := ioutil.ReadDir(ck.pkg.File(ck.patchdir)) + patchFiles, err := ck.pkg.File(ck.patchdir).ReadDir() if err != nil { if trace.Tracing { trace.Stepf("Cannot read patchdir %q: %s", ck.patchdir, err) @@ -311,11 +310,11 @@ func (ck *distinfoLinesChecker) checkUnrecordedPatches() { } for _, file := range patchFiles { - patchName := file.Name() - if file.Mode().IsRegular() && ck.infos[patchName].isPatch != yes && hasPrefix(patchName, "patch-") { + patchName := NewPath(file.Name()) + if file.Mode().IsRegular() && ck.infos[patchName].isPatch != yes && patchName.HasPrefixText("patch-") { line := NewLineWhole(ck.lines.Filename) line.Errorf("Patch %q is not recorded. Run %q.", - line.PathToFile(ck.pkg.File(joinPath(ck.patchdir, patchName))), + line.PathToFile(ck.pkg.File(ck.patchdir.JoinNoClean(patchName))), bmake("makepatchsum")) } } @@ -336,7 +335,7 @@ func (ck *distinfoLinesChecker) checkGlobalDistfileMismatch(info distinfoHash) { // Intentionally checking the filename instead of ck.isPatch. // Missing the few distfiles that actually start with patch-* // is more convenient than having lots of false positive mismatches. - if hasPrefix(filename, "patch-") { + if filename.HasPrefixText("patch-") { return } @@ -381,7 +380,7 @@ func (ck *distinfoLinesChecker) checkUncommittedPatch(info distinfoHash) { } } -func (ck *distinfoLinesChecker) checkPatchSha1(line *Line, patchFileName, distinfoSha1Hex string) { +func (ck *distinfoLinesChecker) checkPatchSha1(line *Line, patchFileName Path, distinfoSha1Hex string) { lines := Load(ck.pkg.File(patchFileName), 0) if lines == nil { line.Errorf("Patch %s does not exist.", patchFileName) @@ -409,8 +408,8 @@ type distinfoFileInfo struct { hashes []distinfoHash } -func (info *distinfoFileInfo) filename() string { return info.hashes[0].filename } -func (info *distinfoFileInfo) line() *Line { return info.hashes[0].line } +func (info *distinfoFileInfo) filename() Path { return info.hashes[0].filename } +func (info *distinfoFileInfo) line() *Line { return info.hashes[0].line } func (info *distinfoFileInfo) algorithms() string { var algs []string @@ -422,7 +421,7 @@ func (info *distinfoFileInfo) algorithms() string { type distinfoHash struct { line *Line - filename string + filename Path algorithm string hash string } diff --git a/pkgtools/pkglint/files/files.go b/pkgtools/pkglint/files/files.go index b54dde877c8..d4eb93d6114 100644 --- a/pkgtools/pkglint/files/files.go +++ b/pkgtools/pkglint/files/files.go @@ -1,9 +1,7 @@ package pkglint import ( - "io/ioutil" "netbsd.org/pkglint/textproc" - "path" "strings" ) @@ -16,7 +14,7 @@ const ( LogErrors // ) -func LoadMk(filename string, options LoadOptions) *MkLines { +func LoadMk(filename Path, options LoadOptions) *MkLines { lines := Load(filename, options|Makefile) if lines == nil { return nil @@ -24,12 +22,12 @@ func LoadMk(filename string, options LoadOptions) *MkLines { return NewMkLines(lines) } -func Load(filename string, options LoadOptions) *Lines { +func Load(filename Path, options LoadOptions) *Lines { if fromCache := G.fileCache.Get(filename, options); fromCache != nil { return fromCache } - rawBytes, err := ioutil.ReadFile(filename) + rawText, err := filename.ReadString() if err != nil { switch { case options&MustSucceed != 0: @@ -40,7 +38,6 @@ func Load(filename string, options LoadOptions) *Lines { return nil } - rawText := string(rawBytes) if rawText == "" && options&NotEmpty != 0 { switch { case options&MustSucceed != 0: @@ -52,17 +49,17 @@ func Load(filename string, options LoadOptions) *Lines { } if G.Opts.Profiling { - G.loaded.Add(path.Clean(filename), 1) + G.loaded.Add(filename.Clean().String(), 1) } result := convertToLogicalLines(filename, rawText, options&Makefile != 0) - if hasSuffix(filename, ".mk") { + if filename.HasSuffixText(".mk") { G.fileCache.Put(filename, options, result) } return result } -func convertToLogicalLines(filename string, rawText string, joinBackslashLines bool) *Lines { +func convertToLogicalLines(filename Path, rawText string, joinBackslashLines bool) *Lines { var rawLines []*RawLine for lineno, rawLine := range strings.SplitAfter(rawText, "\n") { if rawLine != "" { @@ -92,7 +89,7 @@ func convertToLogicalLines(filename string, rawText string, joinBackslashLines b return NewLines(filename, loglines) } -func nextLogicalLine(filename string, rawLines []*RawLine, index int) (*Line, int) { +func nextLogicalLine(filename Path, rawLines []*RawLine, index int) (*Line, int) { { // Handle the common case efficiently rawLine := rawLines[index] textnl := rawLine.textnl diff --git a/pkgtools/pkglint/files/fuzzer_test.go b/pkgtools/pkglint/files/fuzzer_test.go index d35bd2b5e0b..6a995bae2de 100644 --- a/pkgtools/pkglint/files/fuzzer_test.go +++ b/pkgtools/pkglint/files/fuzzer_test.go @@ -3,6 +3,7 @@ package pkglint import ( "gopkg.in/check.v1" "math/rand" + "time" ) // Fuzzer generates random strings. @@ -27,7 +28,7 @@ func NewFuzzer(seed ...int64) *Fuzzer { if len(seed) > 0 { actualSeed = seed[0] } else { - actualSeed = rand.Int63() + actualSeed = time.Now().UnixNano() } return &Fuzzer{seed: actualSeed, rnd: rand.New(rand.NewSource(actualSeed))} } diff --git a/pkgtools/pkglint/files/licenses.go b/pkgtools/pkglint/files/licenses.go index 0e6602d74fc..9b155649017 100644 --- a/pkgtools/pkglint/files/licenses.go +++ b/pkgtools/pkglint/files/licenses.go @@ -24,18 +24,18 @@ func (lc *LicenseChecker) Check(value string, op MkOperator) { } func (lc *LicenseChecker) checkName(license string) { - licenseFile := "" + licenseFile := NewPath("") if G.Pkg != nil { if mkline := G.Pkg.vars.FirstDefinition("LICENSE_FILE"); mkline != nil { - licenseFile = G.Pkg.File(mkline.ResolveVarsInRelativePath(mkline.Value())) + licenseFile = G.Pkg.File(mkline.ResolveVarsInRelativePath(NewPath(mkline.Value()))) } } if licenseFile == "" { - licenseFile = G.Pkgsrc.File("licenses/" + license) + licenseFile = G.Pkgsrc.File("licenses").JoinNoClean(NewPath(license)) G.InterPackage.UseLicense(license) } - if !fileExists(licenseFile) { + if !licenseFile.IsFile() { lc.MkLine.Warnf("License file %s does not exist.", cleanpath(licenseFile)) } diff --git a/pkgtools/pkglint/files/licenses_test.go b/pkgtools/pkglint/files/licenses_test.go index 612c706359e..62ffcf46246 100644 --- a/pkgtools/pkglint/files/licenses_test.go +++ b/pkgtools/pkglint/files/licenses_test.go @@ -54,7 +54,7 @@ func (s *Suite) Test_LicenseChecker_checkName__LICENSE_FILE(c *check.C) { t.CreateFileLines("category/package/my-license", "An individual license file.") - t.Main(t.File("category/package")) + t.Main("category/package") // There is no warning about the unusual file name in the package directory. // If it were not mentioned in LICENSE_FILE, the file named my-license diff --git a/pkgtools/pkglint/files/line.go b/pkgtools/pkglint/files/line.go index c669166d163..03bd76718fd 100644 --- a/pkgtools/pkglint/files/line.go +++ b/pkgtools/pkglint/files/line.go @@ -15,7 +15,6 @@ package pkglint import ( "netbsd.org/pkglint/regex" - "path" "strconv" ) @@ -38,16 +37,16 @@ type RawLine struct { func (rline *RawLine) String() string { return sprintf("%d:%s", rline.Lineno, rline.textnl) } type Location struct { - Filename string // uses / as directory separator on all platforms - firstLine int32 // zero means the whole file, -1 means EOF - lastLine int32 // usually the same as firstLine, may differ in Makefiles + Filename Path + firstLine int32 // zero means the whole file, -1 means EOF + lastLine int32 // usually the same as firstLine, may differ in Makefiles } func (loc *Location) String() string { - return loc.Filename + ":" + loc.Linenos() + return loc.Filename.String() + ":" + loc.Linenos() } -func NewLocation(filename string, firstLine, lastLine int) Location { +func NewLocation(filename Path, firstLine, lastLine int) Location { return Location{filename, int32(firstLine), int32(lastLine)} } @@ -83,23 +82,23 @@ type Line struct { // XXX: Filename and Basename could be replaced with a pointer to a Lines object. } -func NewLine(filename string, lineno int, text string, rawLine *RawLine) *Line { +func NewLine(filename Path, lineno int, text string, rawLine *RawLine) *Line { assert(rawLine != nil) // Use NewLineMulti for creating a Line with no RawLine attached to it. return NewLineMulti(filename, lineno, lineno, text, []*RawLine{rawLine}) } // NewLineMulti is for logical Makefile lines that end with backslash. -func NewLineMulti(filename string, firstLine, lastLine int, text string, rawLines []*RawLine) *Line { - return &Line{NewLocation(filename, firstLine, lastLine), path.Base(filename), text, rawLines, nil, Once{}} +func NewLineMulti(filename Path, firstLine, lastLine int, text string, rawLines []*RawLine) *Line { + return &Line{NewLocation(filename, firstLine, lastLine), filename.Base(), text, rawLines, nil, Once{}} } // NewLineEOF creates a dummy line for logging, with the "line number" EOF. -func NewLineEOF(filename string) *Line { +func NewLineEOF(filename Path) *Line { return NewLineMulti(filename, -1, 0, "", nil) } // NewLineWhole creates a dummy line for logging messages that affect a file as a whole. -func NewLineWhole(filename string) *Line { +func NewLineWhole(filename Path) *Line { return NewLineMulti(filename, 0, 0, "", nil) } @@ -111,7 +110,7 @@ func (line *Line) RefTo(other *Line) string { func (line *Line) RefToLocation(other Location) string { if line.Filename != other.Filename { - return line.PathToFile(other.Filename) + ":" + other.Linenos() + return line.PathToFile(other.Filename).String() + ":" + other.Linenos() } return "line " + other.Linenos() } @@ -119,8 +118,8 @@ func (line *Line) RefToLocation(other Location) string { // PathToFile returns the relative path from this line to the given file path. // This is typically used for arguments in diagnostics, which should always be // relative to the line with which the diagnostic is associated. -func (line *Line) PathToFile(filePath string) string { - return relpath(path.Dir(line.Filename), filePath) +func (line *Line) PathToFile(filePath Path) Path { + return relpath(line.Filename.Dir(), filePath) } func (line *Line) IsMultiline() bool { diff --git a/pkgtools/pkglint/files/linelexer.go b/pkgtools/pkglint/files/linelexer.go index 9c3f10caa02..6c43942e92a 100644 --- a/pkgtools/pkglint/files/linelexer.go +++ b/pkgtools/pkglint/files/linelexer.go @@ -4,19 +4,22 @@ import "netbsd.org/pkglint/regex" // LinesLexer records the state when checking a list of lines from top to bottom. type LinesLexer struct { - lines *Lines + line *Line index int + lines *Lines } func NewLinesLexer(lines *Lines) *LinesLexer { - return &LinesLexer{lines, 0} + llex := LinesLexer{nil, 0, lines} + llex.setIndex(0) + return &llex } // CurrentLine returns the line that the lexer is currently looking at. -// If it is at the end of file, the line number of the line is EOF. +// For the EOF, a virtual line with line number "EOF" is returned. func (llex *LinesLexer) CurrentLine() *Line { - if llex.index < llex.lines.Len() { - return llex.lines.Lines[llex.index] + if llex.line != nil { + return llex.line } return NewLineEOF(llex.lines.Filename) } @@ -26,20 +29,20 @@ func (llex *LinesLexer) PreviousLine() *Line { } func (llex *LinesLexer) EOF() bool { - return !(llex.index < llex.lines.Len()) + return llex.line == nil } -// Skip skips the current line and returns true. +// Skip skips the current line. func (llex *LinesLexer) Skip() bool { if llex.EOF() { return false } - llex.index++ + llex.next() return true } func (llex *LinesLexer) Undo() { - llex.index-- + llex.setIndex(llex.index - 1) } func (llex *LinesLexer) NextRegexp(re regex.Pattern) []string { @@ -48,8 +51,8 @@ func (llex *LinesLexer) NextRegexp(re regex.Pattern) []string { } if !llex.EOF() { - if m := match(llex.lines.Lines[llex.index].Text, re); m != nil { - llex.index++ + if m := match(llex.line.Text, re); m != nil { + llex.next() return m } } @@ -65,19 +68,19 @@ func (llex *LinesLexer) SkipPrefix(prefix string) bool { defer trace.Call2(llex.CurrentLine().Text, prefix)() } - if !llex.EOF() && hasPrefix(llex.lines.Lines[llex.index].Text, prefix) { - llex.Skip() + if !llex.EOF() && hasPrefix(llex.line.Text, prefix) { + llex.next() return true } return false } -func (llex *LinesLexer) SkipString(text string) bool { +func (llex *LinesLexer) SkipText(text string) bool { if trace.Tracing { defer trace.Call2(llex.CurrentLine().Text, text)() } - if !llex.EOF() && llex.lines.Lines[llex.index].Text == text { + if !llex.EOF() && llex.line.Text == text { llex.Skip() return true } @@ -85,7 +88,7 @@ func (llex *LinesLexer) SkipString(text string) bool { } func (llex *LinesLexer) SkipEmptyOrNote() bool { - if llex.SkipString("") { + if llex.SkipText("") { return true } @@ -106,13 +109,24 @@ func (llex *LinesLexer) SkipEmptyOrNote() bool { } func (llex *LinesLexer) SkipContainsOrWarn(text string) bool { - result := llex.SkipString(text) + result := llex.SkipText(text) if !result { llex.CurrentLine().Warnf("This line should contain the following text: %s", text) } return result } +func (llex *LinesLexer) setIndex(index int) { + llex.index = index + if index < llex.lines.Len() { + llex.line = llex.lines.Lines[index] + } else { + llex.line = nil + } +} + +func (llex *LinesLexer) next() { llex.setIndex(llex.index + 1) } + // MkLinesLexer records the state when checking a list of Makefile lines from top to bottom. type MkLinesLexer struct { mklines *MkLines @@ -133,7 +147,7 @@ func (mlex *MkLinesLexer) CurrentMkLine() *MkLine { func (mlex *MkLinesLexer) SkipIf(pred func(mkline *MkLine) bool) bool { if !mlex.EOF() && pred(mlex.CurrentMkLine()) { - mlex.Skip() + mlex.next() return true } return false diff --git a/pkgtools/pkglint/files/lines.go b/pkgtools/pkglint/files/lines.go index 339c2f6e48a..3d7e565b070 100644 --- a/pkgtools/pkglint/files/lines.go +++ b/pkgtools/pkglint/files/lines.go @@ -2,17 +2,16 @@ package pkglint import ( "netbsd.org/pkglint/regex" - "path" ) type Lines struct { - Filename string - BaseName string + Filename Path + BaseName string // TODO: consider converting to Path Lines []*Line } -func NewLines(filename string, lines []*Line) *Lines { - return &Lines{filename, path.Base(filename), lines} +func NewLines(filename Path, lines []*Line) *Lines { + return &Lines{filename, filename.Base(), lines} } func (ls *Lines) Len() int { return len(ls.Lines) } diff --git a/pkgtools/pkglint/files/logging.go b/pkgtools/pkglint/files/logging.go index 0d25f3ef73f..f6e54ca2fbc 100644 --- a/pkgtools/pkglint/files/logging.go +++ b/pkgtools/pkglint/files/logging.go @@ -5,7 +5,6 @@ import ( "io" "netbsd.org/pkglint/histogram" "netbsd.org/pkglint/textproc" - "path" "strings" ) @@ -126,12 +125,12 @@ func (l *Logger) Diag(line *Line, level *LogLevel, format string, args ...interf l.Logf(level, filename, linenos, format, msg) } -func (l *Logger) FirstTime(filename, linenos, msg string) bool { +func (l *Logger) FirstTime(filename Path, linenos, msg string) bool { if l.Opts.LogVerbose { return true } - if !l.logged.FirstTimeSlice(path.Clean(filename), linenos, msg) { + if !l.logged.FirstTimeSlice(filename.Clean().String(), linenos, msg) { l.suppressDiag = true l.suppressExpl = true return false @@ -230,7 +229,7 @@ func (l *Logger) showSource(line *Line) { // IsAutofix returns whether one of the --show-autofix or --autofix options is active. func (l *Logger) IsAutofix() bool { return l.Opts.Autofix || l.Opts.ShowAutofix } -func (l *Logger) Logf(level *LogLevel, filename, lineno, format, msg string) { +func (l *Logger) Logf(level *LogLevel, filename Path, lineno, format, msg string) { if l.suppressDiag { l.suppressDiag = false return @@ -290,7 +289,7 @@ func (l *Logger) Logf(level *LogLevel, filename, lineno, format, msg string) { // Location.Filename. It may be followed by the usual ":123" for line numbers. // // For diagnostics, use Logf instead. -func (l *Logger) Errorf(location string, format string, args ...interface{}) { +func (l *Logger) Errorf(location Path, format string, args ...interface{}) { msg := sprintf(format, args...) var diag string if l.Opts.GccOutput { diff --git a/pkgtools/pkglint/files/mkline.go b/pkgtools/pkglint/files/mkline.go index 83b1c35ffb7..bad8e5c106a 100644 --- a/pkgtools/pkglint/files/mkline.go +++ b/pkgtools/pkglint/files/mkline.go @@ -4,7 +4,6 @@ import ( "fmt" "netbsd.org/pkglint/regex" "netbsd.org/pkglint/textproc" - "path" "strings" ) @@ -60,7 +59,7 @@ type mkLineInclude struct { mustExist bool // for .sinclude, nonexistent files are ignored sys bool // whether the include uses <file.mk> (very rare) instead of "file.mk" indent string // the space between the leading "." and the directive - includedFile string // the text between the <brackets> or "quotes" + includedFile Path // the text between the <brackets> or "quotes" conditionalVars []string // variables on which this inclusion depends (filled in later, as needed) } @@ -277,12 +276,12 @@ func (mkline *MkLine) SetHasElseBranch(elseLine *MkLine) { func (mkline *MkLine) MustExist() bool { return mkline.data.(*mkLineInclude).mustExist } -func (mkline *MkLine) IncludedFile() string { return mkline.data.(*mkLineInclude).includedFile } +func (mkline *MkLine) IncludedFile() Path { return mkline.data.(*mkLineInclude).includedFile } // IncludedFileFull returns the path to the included file, relative to the // current working directory. -func (mkline *MkLine) IncludedFileFull() string { - return cleanpath(path.Join(path.Dir(mkline.Filename), mkline.IncludedFile())) +func (mkline *MkLine) IncludedFileFull() Path { + return cleanpath(mkline.Filename.Dir().JoinClean(mkline.IncludedFile())) // FIXME: JoinNoClean? } func (mkline *MkLine) Targets() string { return mkline.data.(mkLineDependency).targets } @@ -554,31 +553,31 @@ func (*MkLine) WithoutMakeVariables(value string) string { return valueNovar.String() } -func (mkline *MkLine) ResolveVarsInRelativePath(relativePath string) string { - if !contains(relativePath, "$") { +func (mkline *MkLine) ResolveVarsInRelativePath(relativePath Path) Path { + if !containsVarRef(relativePath.String()) { return cleanpath(relativePath) } - var basedir string + var basedir Path if G.Pkg != nil { basedir = G.Pkg.File(".") } else { - basedir = path.Dir(mkline.Filename) + basedir = mkline.Filename.Dir() } tmp := relativePath - if contains(tmp, "PKGSRCDIR") { + if tmp.ContainsText("PKGSRCDIR") { pkgsrcdir := relpath(basedir, G.Pkgsrc.File(".")) if G.Testing { // Relative pkgsrc paths usually only contain two or three levels. // A possible reason for reaching this assertion is a pkglint unit test // that uses t.NewMkLines instead of the correct t.SetUpFileMkLines. - assertf(!contains(pkgsrcdir, "../../../../.."), + assertf(!pkgsrcdir.ContainsPath("../../../../.."), "Relative path %q for %q is too deep below the pkgsrc root %q.", pkgsrcdir, basedir, G.Pkgsrc.File(".")) } - tmp = strings.Replace(tmp, "${PKGSRCDIR}", pkgsrcdir, -1) + tmp = tmp.Replace("${PKGSRCDIR}", pkgsrcdir.String()) } // Strictly speaking, the .CURDIR should be replaced with the basedir. @@ -586,7 +585,7 @@ func (mkline *MkLine) ResolveVarsInRelativePath(relativePath string) string { // path, this would produce diagnostics that "this relative path must not // be absolute". Since ${.CURDIR} is usually used in package Makefiles and // followed by "../.." anyway, the exact directory doesn't matter. - tmp = strings.Replace(tmp, "${.CURDIR}", ".", -1) + tmp = tmp.Replace("${.CURDIR}", ".") // TODO: Add test for exists(${.PARSEDIR}/file). // TODO: Add test for evaluating ${.PARSEDIR} in an included package. @@ -595,12 +594,12 @@ func (mkline *MkLine) ResolveVarsInRelativePath(relativePath string) string { // This is the only practically relevant use case since the category // directories don't contain any *.mk files that could be included. // TODO: Add test that suggests ${.PARSEDIR} in .include to be omitted. - tmp = strings.Replace(tmp, "${.PARSEDIR}", ".", -1) + tmp = tmp.Replace("${.PARSEDIR}", ".") - replaceLatest := func(varuse, category string, pattern regex.Pattern, replacement string) { - if contains(tmp, varuse) { + replaceLatest := func(varuse string, category Path, pattern regex.Pattern, replacement string) { + if tmp.ContainsText(varuse) { latest := G.Pkgsrc.Latest(category, pattern, replacement) - tmp = strings.Replace(tmp, varuse, latest, -1) + tmp = tmp.Replace(varuse, latest) } } @@ -617,14 +616,14 @@ func (mkline *MkLine) ResolveVarsInRelativePath(relativePath string) string { // XXX: Even if these variables are defined indirectly, // pkglint should be able to resolve them properly. // There is already G.Pkg.Value, maybe that can be used here. - tmp = strings.Replace(tmp, "${FILESDIR}", G.Pkg.Filesdir, -1) - tmp = strings.Replace(tmp, "${PKGDIR}", G.Pkg.Pkgdir, -1) + tmp = tmp.Replace("${FILESDIR}", G.Pkg.Filesdir.String()) + tmp = tmp.Replace("${PKGDIR}", G.Pkg.Pkgdir.String()) } tmp = cleanpath(tmp) if trace.Tracing && relativePath != tmp { - trace.Step2("resolveVarsInRelativePath: %q => %q", relativePath, tmp) + trace.Stepf("resolveVarsInRelativePath: %q => %q", relativePath, tmp) } return tmp } @@ -820,7 +819,7 @@ func (mkline *MkLine) ForEachUsed(action func(varUse *MkVarUse, time VucTime)) { searchIn(mkline.Sources(), VucLoadTime) case mkline.IsInclude(): - searchIn(mkline.IncludedFile(), VucLoadTime) + searchIn(mkline.IncludedFile().String(), VucLoadTime) } } @@ -1053,7 +1052,10 @@ type indentationLevel struct { // pkglint will happily accept .include "fname" in both the then and // the else branch. This is ok since the primary job of this file list // is to prevent wrong pkglint warnings about missing files. - checkedFiles []string + checkedFiles []Path + + // whether the line is a multiple-inclusion guard + guard bool } func (ind *Indentation) IsEmpty() bool { @@ -1085,9 +1087,9 @@ func (ind *Indentation) Pop() { ind.levels = ind.levels[:len(ind.levels)-1] } -func (ind *Indentation) Push(mkline *MkLine, indent int, condition string) { +func (ind *Indentation) Push(mkline *MkLine, indent int, args string, guard bool) { assert(mkline.IsDirective()) - ind.levels = append(ind.levels, indentationLevel{mkline, indent, condition, nil, nil}) + ind.levels = append(ind.levels, indentationLevel{mkline, indent, args, nil, nil, guard}) } // AddVar remembers that the current indentation depends on the given variable, @@ -1121,12 +1123,12 @@ func (ind *Indentation) DependsOn(varname string) bool { } // IsConditional returns whether the current line depends on evaluating -// any variable in an .if or .elif expression or from a .for loop. +// any .if or .elif expression, or is inside a .for loop. // // Variables named *_MK are excluded since they are usually not interesting. func (ind *Indentation) IsConditional() bool { for _, level := range ind.levels { - if len(level.conditionalVars) > 0 { + if !level.guard { return true } } @@ -1155,14 +1157,14 @@ func (ind *Indentation) Args() string { return ind.top().args } -func (ind *Indentation) AddCheckedFile(filename string) { +func (ind *Indentation) AddCheckedFile(filename Path) { top := ind.top() top.checkedFiles = append(top.checkedFiles, filename) } // HasExists returns whether the given filename has been tested in an // exists(filename) condition and thus may or may not exist. -func (ind *Indentation) HasExists(filename string) bool { +func (ind *Indentation) HasExists(filename Path) bool { for _, level := range ind.levels { for _, levelFilename := range level.checkedFiles { if filename == levelFilename { @@ -1177,13 +1179,16 @@ func (ind *Indentation) TrackBefore(mkline *MkLine) { if !mkline.IsDirective() { return } - if trace.Tracing { - trace.Stepf("Indentation before line %s: %s", mkline.Linenos(), ind) - } - switch mkline.Directive() { + directive := mkline.Directive() + switch directive { case "for", "if", "ifdef", "ifndef": - ind.Push(mkline, ind.Depth(mkline.Directive()), mkline.Args()) + guard := false + if directive == "if" { + cond := mkline.Cond() + guard = cond != nil && cond.Not != nil && hasSuffix(cond.Not.Defined, "_MK") + } + ind.Push(mkline, ind.Depth(directive), mkline.Args(), guard) } } @@ -1197,11 +1202,8 @@ func (ind *Indentation) TrackAfter(mkline *MkLine) { switch directive { case "if": - cond := mkline.Cond() - // For multiple-inclusion guards, the indentation stays at the same level. - guard := cond != nil && cond.Not != nil && hasSuffix(cond.Not.Defined, "_MK") - if !guard { + if !ind.top().guard { ind.top().depth += 2 } @@ -1237,17 +1239,13 @@ func (ind *Indentation) TrackAfter(mkline *MkLine) { cond.Walk(&MkCondCallback{ Call: func(name string, arg string) { if name == "exists" { - ind.AddCheckedFile(arg) + ind.AddCheckedFile(NewPath(arg)) } }}) } - - if trace.Tracing { - trace.Stepf("Indentation after line %s: %s", mkline.Linenos(), ind) - } } -func (ind *Indentation) CheckFinish(filename string) { +func (ind *Indentation) CheckFinish(filename Path) { if ind.IsEmpty() { return } @@ -1277,7 +1275,7 @@ var ( VarparamBytes = textproc.NewByteSet("A-Za-z_0-9#*+---./[") ) -func MatchMkInclude(text string) (m bool, indentation, directive, filename string) { +func MatchMkInclude(text string) (m bool, indentation, directive string, filename Path) { lexer := textproc.NewLexer(text) if lexer.SkipByte('.') { indentation = lexer.NextHspace() @@ -1291,7 +1289,7 @@ func MatchMkInclude(text string) (m bool, indentation, directive, filename strin // Note: strictly speaking, the full MkVarUse would have to be parsed // here. But since these usually don't contain double quotes, it has // worked fine up to now. - filename = lexer.NextBytesFunc(func(c byte) bool { return c != '"' }) + filename = NewPath(lexer.NextBytesFunc(func(c byte) bool { return c != '"' })) if filename != "" && lexer.SkipByte('"') { lexer.NextHspace() if lexer.EOF() { diff --git a/pkgtools/pkglint/files/mkline_test.go b/pkgtools/pkglint/files/mkline_test.go index b13ac378a94..b913f7ed26b 100644 --- a/pkgtools/pkglint/files/mkline_test.go +++ b/pkgtools/pkglint/files/mkline_test.go @@ -503,7 +503,7 @@ func (s *Suite) Test_MkLine_ResolveVarsInRelativePath(c *check.C) { MkCvsID) mkline := mklines.mklines[0] - test := func(before string, after string) { + test := func(before Path, after Path) { t.CheckEquals(mkline.ResolveVarsInRelativePath(before), after) } @@ -1129,7 +1129,7 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__uncovered_cases(c *check.C) { "") // Just for branch coverage. - trace.Tracing = false + t.DisableTracing() MkLineChecker{mklines, mklines.mklines[2]}.Check() t.CheckOutputEmpty() @@ -1265,7 +1265,7 @@ func (s *Suite) Test_Indentation(c *check.C) { t.CheckEquals(ind.Depth("if"), 0) t.CheckEquals(ind.DependsOn("VARNAME"), false) - ind.Push(mkline, 2, "") + ind.Push(mkline, 2, "", false) t.CheckEquals(ind.Depth("if"), 2) t.CheckEquals(ind.Depth("endfor"), 0) @@ -1280,7 +1280,7 @@ func (s *Suite) Test_Indentation(c *check.C) { t.CheckEquals(ind.DependsOn("LEVEL1.VAR1"), true) t.CheckEquals(ind.DependsOn("OTHER_VAR"), false) - ind.Push(mkline, 2, "") + ind.Push(mkline, 2, "", false) ind.AddVar("LEVEL2.VAR") @@ -1308,7 +1308,7 @@ func (s *Suite) Test_Indentation__realistic(c *check.C) { ".if 1", ". if !defined(GUARD_MK)", ". for var in 1 2 3", - ". if !defined(GUARD_MK)", + ". if !defined(GUARD_MK)", // well, not entirely realistic ". if 3", ". endif", ". endif", @@ -1322,43 +1322,29 @@ func (s *Suite) Test_Indentation__realistic(c *check.C) { ". endfor", ".endif") - t.EnableTracingToLog() - mklines.ForEach(func(mkline *MkLine) {}) - t.CheckOutputLinesMatching(`Indentation`, - "TRACE: Indentation before line 3: []", - "TRACE: Indentation after line 3: [2]", - "TRACE: Indentation before line 4: [2]", - "TRACE: Indentation after line 4: [2 2]", - "TRACE: Indentation before line 5: [2 2]", - "TRACE: Indentation after line 5: [2 2 4]", - "TRACE: Indentation before line 6: [2 2 4]", - "TRACE: Indentation after line 6: [2 2 4 4]", - "TRACE: Indentation before line 7: [2 2 4 4]", - "TRACE: Indentation after line 7: [2 2 4 4 6]", - "TRACE: Indentation before line 8: [2 2 4 4 6]", - "TRACE: Indentation after line 8: [2 2 4 4]", - "TRACE: Indentation before line 9: [2 2 4 4]", - "TRACE: Indentation after line 9: [2 2 4]", - "TRACE: Indentation before line 10: [2 2 4]", - "TRACE: Indentation after line 10: [2 2]", - "TRACE: Indentation before line 11: [2 2]", - "TRACE: Indentation after line 11: [2]", - "TRACE: Indentation before line 12: [2]", - "TRACE: Indentation after line 12: [2]", - "TRACE: Indentation before line 13: [2]", - "TRACE: Indentation after line 13: [2 4]", - "TRACE: Indentation before line 14: [2 4]", - "TRACE: Indentation after line 14: [2]", - "TRACE: Indentation before line 15: [2]", - "TRACE: Indentation after line 15: [2]", - "TRACE: Indentation before line 16: [2]", - "TRACE: Indentation after line 16: [2 4]", - "TRACE: Indentation before line 17: [2 4]", - "TRACE: Indentation after line 17: [2]", - "TRACE: Indentation before line 18: [2]", - "TRACE: Indentation after line 18: []") + t.CheckOutputEmpty() +} + +func (s *Suite) Test_Indentation_String(c *check.C) { + t := s.Init(c) + + mklines := t.NewMkLines("filename.mk", + ".if exists(/bin)", + "# probably POSIX", + ".endif") + var str string + + mklines.ForEach(func(mkline *MkLine) { + if mkline.IsComment() { + t.CheckEquals(mklines.indentation.IsConditional(), true) + t.Check(mklines.indentation.Varnames(), check.IsNil) + str = mklines.indentation.String() + } + }) + + t.CheckEquals(str, "[2]") } func (s *Suite) Test_Indentation_RememberUsedVariables(c *check.C) { @@ -1440,7 +1426,7 @@ func (s *Suite) Test_Indentation_TrackAfter__lonely_else(c *check.C) { func (s *Suite) Test_MatchMkInclude(c *check.C) { t := s.Init(c) - test := func(input, expectedIndent, expectedDirective, expectedFilename, expectedComment string) { + test := func(input, expectedIndent, expectedDirective string, expectedFilename Path, expectedComment string) { splitResult := NewMkLineParser().split(nil, input, true) m, indent, directive, args := MatchMkInclude(splitResult.main) t.CheckDeepEquals( diff --git a/pkgtools/pkglint/files/mklinechecker.go b/pkgtools/pkglint/files/mklinechecker.go index b58040d4b79..1243658f342 100644 --- a/pkgtools/pkglint/files/mklinechecker.go +++ b/pkgtools/pkglint/files/mklinechecker.go @@ -3,9 +3,6 @@ package pkglint import ( "netbsd.org/pkglint/regex" "netbsd.org/pkglint/textproc" - "os" - "path" - "path/filepath" "strconv" "strings" ) @@ -1089,9 +1086,9 @@ func (ck MkLineChecker) checkVarassignRightCategory() { categories := mkline.ValueFields(mkline.Value()) actual := categories[0] - expected := path.Base(path.Dir(path.Dir(mkline.Filename))) + expected := mkline.Filename.Dir().Dir().Base() if expected == "." { - expected = path.Base(path.Dir(path.Dir(G.Pkgsrc.ToRel(mkline.Filename)))) + expected = G.Pkgsrc.ToRel(mkline.Filename).Dir().Dir().Base() } if expected == "wip" || actual == expected { return @@ -1194,7 +1191,7 @@ func (ck MkLineChecker) checkVarassignMiscRedundantInstallationDirs() { } for _, dir := range mkline.ValueFields(mkline.Value()) { - if G.Pkg.Plist.Dirs[dir] != nil { + if G.Pkg.Plist.Dirs[NewPath(dir)] != nil { mkline.Notef("The directory %q is redundant in %s.", dir, varname) mkline.Explain( "This package defines AUTO_MKDIR, and the directory is contained in the PLIST.", @@ -1308,12 +1305,12 @@ func (ck MkLineChecker) checkInclude() { includedFile := mkline.IncludedFile() mustExist := mkline.MustExist() if trace.Tracing { - trace.Step2("includingFile=%s includedFile=%s", mkline.Filename, includedFile) + trace.Stepf("includingFile=%s includedFile=%s", mkline.Filename, includedFile) } ck.CheckRelativePath(includedFile, mustExist) switch { - case hasSuffix(includedFile, "/Makefile"): + case includedFile.HasBase("Makefile"): mkline.Errorf("Other Makefiles must not be included directly.") mkline.Explain( "To include portions of another Makefile, extract the common parts", @@ -1329,25 +1326,25 @@ func (ck MkLineChecker) checkInclude() { fix.Apply() } - case hasSuffix(includedFile, "pkgtools/x11-links/buildlink3.mk"): + case includedFile.HasSuffixPath("pkgtools/x11-links/buildlink3.mk"): fix := mkline.Autofix() fix.Errorf("%s must not be included directly. Include \"../../mk/x11.buildlink3.mk\" instead.", includedFile) fix.Replace("pkgtools/x11-links/buildlink3.mk", "mk/x11.buildlink3.mk") fix.Apply() - case hasSuffix(includedFile, "graphics/jpeg/buildlink3.mk"): + case includedFile.HasSuffixPath("graphics/jpeg/buildlink3.mk"): fix := mkline.Autofix() fix.Errorf("%s must not be included directly. Include \"../../mk/jpeg.buildlink3.mk\" instead.", includedFile) fix.Replace("graphics/jpeg/buildlink3.mk", "mk/jpeg.buildlink3.mk") fix.Apply() - case hasSuffix(includedFile, "/intltool/buildlink3.mk"): + case includedFile.HasSuffixPath("intltool/buildlink3.mk"): mkline.Warnf("Please write \"USE_TOOLS+= intltool\" instead of this line.") - case hasSuffix(includedFile, "/builtin.mk"): + case includedFile.HasSuffixText("/builtin.mk"): // TODO: maybe HasSuffixPath if mkline.Basename != "hacks.mk" && !mkline.HasRationale() { fix := mkline.Autofix() - fix.Errorf("%s must not be included directly. Include \"%s/buildlink3.mk\" instead.", includedFile, path.Dir(includedFile)) + fix.Errorf("%s must not be included directly. Include \"%s/buildlink3.mk\" instead.", includedFile, includedFile.Dir()) fix.Replace("builtin.mk", "buildlink3.mk") fix.Apply() } @@ -1370,28 +1367,28 @@ func (ck MkLineChecker) checkDirectiveIndentation(expectedDepth int) { // CheckRelativePath checks a relative path that leads to the directory of another package // or to a subdirectory thereof or a file within there. -func (ck MkLineChecker) CheckRelativePath(relativePath string, mustExist bool) { +func (ck MkLineChecker) CheckRelativePath(relativePath Path, mustExist bool) { if trace.Tracing { defer trace.Call(relativePath, mustExist)() } mkline := ck.MkLine - if !G.Wip && contains(relativePath, "/wip/") { + if !G.Wip && relativePath.ContainsPath("wip") { mkline.Errorf("A main pkgsrc package must not depend on a pkgsrc-wip package.") } resolvedPath := mkline.ResolveVarsInRelativePath(relativePath) - if containsVarRef(resolvedPath) { + if containsVarRef(resolvedPath.String()) { return } - if filepath.IsAbs(resolvedPath) { + if resolvedPath.IsAbs() { mkline.Errorf("The path %q must be relative.", resolvedPath) return } - abs := joinPath(path.Dir(mkline.Filename), resolvedPath) - if _, err := os.Stat(abs); err != nil { + abs := mkline.Filename.Dir().JoinNoClean(resolvedPath) + if !abs.Exists() { if mustExist && !ck.MkLines.indentation.HasExists(resolvedPath) { mkline.Errorf("Relative path %q does not exist.", resolvedPath) } @@ -1399,21 +1396,21 @@ func (ck MkLineChecker) CheckRelativePath(relativePath string, mustExist bool) { } switch { - case !hasPrefix(resolvedPath, "../"): + case !resolvedPath.HasPrefixPath(".."): break - case hasPrefix(resolvedPath, "../../mk/"): + case resolvedPath.HasPrefixPath("../../mk"): // From a package to the infrastructure. - case matches(resolvedPath, `^\.\./\.\./[^./][^/]*/[^/]`): + case matches(resolvedPath.String(), `^\.\./\.\./[^./][^/]*/[^/]`): // From a package to another package. - case hasPrefix(resolvedPath, "../mk/") && relpath(path.Dir(mkline.Filename), G.Pkgsrc.File(".")) == "..": + case resolvedPath.HasPrefixPath("../mk") && G.Pkgsrc.ToRel(mkline.Filename).Count() == 2: // For category Makefiles. // TODO: Or from a pkgsrc wip package to wip/mk. - case matches(resolvedPath, `^\.\./[^./][^/]*/[^/]`): - if G.Wip && contains(resolvedPath, "/mk/") { + case matches(resolvedPath.String(), `^\.\./[^./][^/]*/[^/]`): + if G.Wip && resolvedPath.ContainsPath("mk") { mkline.Warnf("References to the pkgsrc-wip infrastructure should look like \"../../wip/mk\", not \"../mk\".") } else { mkline.Warnf("References to other packages should look like \"../../category/package\", not \"../package\".") @@ -1433,16 +1430,16 @@ func (ck MkLineChecker) CheckRelativePath(relativePath string, mustExist bool) { // // When used in .include directives, the relative package directories must be written // with the leading ../.. anyway, so the benefit might not be too big at all. -func (ck MkLineChecker) CheckRelativePkgdir(pkgdir string) { +func (ck MkLineChecker) CheckRelativePkgdir(pkgdir Path) { if trace.Tracing { - defer trace.Call1(pkgdir)() + defer trace.Call(pkgdir)() } mkline := ck.MkLine ck.CheckRelativePath(pkgdir+"/Makefile", true) pkgdir = mkline.ResolveVarsInRelativePath(pkgdir) - if !matches(pkgdir, `^\.\./\.\./([^./][^/]*/[^./][^/]*)$`) && !containsVarRef(pkgdir) { + if !matches(pkgdir.String(), `^\.\./\.\./([^./][^/]*/[^./][^/]*)$`) && !containsVarRef(pkgdir.String()) { mkline.Warnf("%q is not a valid relative package directory.", pkgdir) mkline.Explain( "A relative pathname always starts with \"../../\", followed", @@ -1722,11 +1719,30 @@ func (ck MkLineChecker) checkCompareVarStr(varname, op, value string) { ck.checkVartype(varname, opUseCompare, value, "") if varname == "PKGSRC_COMPILER" { - ck.MkLine.Warnf("Use ${PKGSRC_COMPILER:%s%s} instead of the %s operator.", condStr(op == "==", "M", "N"), value, op) - ck.MkLine.Explain( - "The PKGSRC_COMPILER can be a list of chained compilers, e.g. \"ccache distcc clang\".", - "Therefore, comparing it using == or != leads to wrong results in these cases.") + ck.checkCompareVarStrCompiler(op, value) + } +} + +func (ck MkLineChecker) checkCompareVarStrCompiler(op string, value string) { + if !matches(value, `^\w+$`) { + return } + + // It would be nice if original text of the whole comparison expression + // were available at this point, to avoid guessing how much whitespace + // the package author really used. + + matchOp := condStr(op == "==", "M", "N") + + fix := ck.MkLine.Autofix() + fix.Errorf("Use ${PKGSRC_COMPILER:%s%s} instead of the %s operator.", matchOp, value, op) + fix.Explain( + "The PKGSRC_COMPILER can be a list of chained compilers, e.g. \"ccache distcc clang\".", + "Therefore, comparing it using == or != leads to wrong results in these cases.") + fix.Replace("${PKGSRC_COMPILER} "+op+" "+value, "${PKGSRC_COMPILER:"+matchOp+value+"}") + fix.Replace("${PKGSRC_COMPILER} "+op+" \""+value+"\"", "${PKGSRC_COMPILER:"+matchOp+value+"}") + fix.Anyway() + fix.Apply() } func (ck MkLineChecker) checkDirectiveFor(forVars map[string]bool, indentation *Indentation) { diff --git a/pkgtools/pkglint/files/mklinechecker_test.go b/pkgtools/pkglint/files/mklinechecker_test.go index 7698b5e4b03..28382422a75 100644 --- a/pkgtools/pkglint/files/mklinechecker_test.go +++ b/pkgtools/pkglint/files/mklinechecker_test.go @@ -102,6 +102,9 @@ func (s *Suite) Test_MkLineChecker_Check__varuse_modifier_L(c *check.C) { t.CheckOutputLines( "WARN: x11/xkeyboard-config/Makefile:3: "+ "Invalid part \"/xkbcomp\" after variable name \"${XKBBASE}\".", + // TODO: Avoid this duplicate diagnostic. + "WARN: x11/xkeyboard-config/Makefile:3: "+ + "Invalid part \"/xkbcomp\" after variable name \"${XKBBASE}\".", "WARN: x11/xkeyboard-config/Makefile:3: XKBBASE is used but not defined.") } @@ -2413,7 +2416,7 @@ func (s *Suite) Test_MkLineChecker_CheckRelativePkgdir(c *check.C) { t.CreateFileLines("other/package/Makefile") - test := func(relativePkgdir string, diagnostics ...string) { + test := func(relativePkgdir Path, diagnostics ...string) { // Must be in the filesystem because of directory references. mklines := t.SetUpFileMkLines("category/package/Makefile", "# dummy") @@ -2614,7 +2617,7 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveCond(c *check.C) { test(".if ${PKGSRC_COMPILER} == \"msvc\"", "WARN: filename.mk:1: \"msvc\" is not valid for PKGSRC_COMPILER. "+ "Use one of { ccache ccc clang distcc f2c gcc hp icc ido mipspro mipspro-ucode pcc sunpro xlc } instead.", - "WARN: filename.mk:1: Use ${PKGSRC_COMPILER:Mmsvc} instead of the == operator.") + "ERROR: filename.mk:1: Use ${PKGSRC_COMPILER:Mmsvc} instead of the == operator.") test(".if ${PKG_LIBTOOL:Mlibtool}", "NOTE: filename.mk:1: PKG_LIBTOOL should be compared using == instead of matching against \":Mlibtool\".", @@ -2668,10 +2671,11 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveCond(c *check.C) { "WARN: filename.mk:1: VAR is used but not defined.") test(".if ${MASTER_SITES:Mftp://*} == \"ftp://netbsd.org/\"", + // FIXME: duplicate diagnostic, see MkParser.MkCond. + "WARN: filename.mk:1: Invalid variable modifier \"//*\" for \"MASTER_SITES\".", "WARN: filename.mk:1: Invalid variable modifier \"//*\" for \"MASTER_SITES\".", "WARN: filename.mk:1: \"ftp\" is not a valid URL.", - "WARN: filename.mk:1: MASTER_SITES should not be used at load time in any file.", - "WARN: filename.mk:1: Invalid variable modifier \"//*\" for \"MASTER_SITES\".") + "WARN: filename.mk:1: MASTER_SITES should not be used at load time in any file.") } func (s *Suite) Test_MkLineChecker_checkDirectiveCond__tracing(c *check.C) { @@ -2703,7 +2707,7 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveCond__comparison_with_shell_com // Don't warn about unknown shell command "cc". t.CheckOutputLines( - "WARN: security/openssl/Makefile:2: Use ${PKGSRC_COMPILER:Mgcc} instead of the == operator.") + "ERROR: security/openssl/Makefile:2: Use ${PKGSRC_COMPILER:Mgcc} instead of the == operator.") } // The :N modifier filters unwanted values. After this filter, any variable value @@ -2746,8 +2750,8 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveCond__comparing_PKGSRC_COMPILER mklines.Check() t.CheckOutputLines( - "WARN: Makefile:2: Use ${PKGSRC_COMPILER:Mclang} instead of the == operator.", - "WARN: Makefile:3: Use ${PKGSRC_COMPILER:Ngcc} instead of the != operator.") + "ERROR: Makefile:2: Use ${PKGSRC_COMPILER:Mclang} instead of the == operator.", + "ERROR: Makefile:3: Use ${PKGSRC_COMPILER:Ngcc} instead of the != operator.") } func (s *Suite) Test_MkLineChecker_checkDirectiveCondEmpty(c *check.C) { @@ -3062,6 +3066,62 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveCondCompareVarStr__no_tracing(c t.CheckOutputEmpty() } +func (s *Suite) Test_MkLineChecker_checkCompareVarStrCompiler(c *check.C) { + t := s.Init(c) + + t.SetUpVartypes() + t.Chdir(".") + + test := func(cond string, diagnostics ...string) { + mklines := t.SetUpFileMkLines("filename.mk", + MkCvsID, + "", + ".if "+cond, + ".endif") + + t.SetUpCommandLine("-Wall") + mklines.Check() + t.SetUpCommandLine("-Wall", "--autofix") + mklines.Check() + + t.CheckOutput(diagnostics) + } + + test( + "${PKGSRC_COMPILER} == gcc", + + "ERROR: filename.mk:3: "+ + "Use ${PKGSRC_COMPILER:Mgcc} instead of the == operator.", + "AUTOFIX: filename.mk:3: "+ + "Replacing \"${PKGSRC_COMPILER} == gcc\" "+ + "with \"${PKGSRC_COMPILER:Mgcc}\".") + + // No autofix because of missing whitespace. + // TODO: Provide the autofix regardless of the whitespace. + test( + "${PKGSRC_COMPILER}==gcc", + + "ERROR: filename.mk:3: "+ + "Use ${PKGSRC_COMPILER:Mgcc} instead of the == operator.") + + // The comparison value can be with or without quotes. + test( + "${PKGSRC_COMPILER} == \"gcc\"", + + "ERROR: filename.mk:3: "+ + "Use ${PKGSRC_COMPILER:Mgcc} instead of the == operator.", + "AUTOFIX: filename.mk:3: "+ + "Replacing \"${PKGSRC_COMPILER} == \\\"gcc\\\"\" "+ + "with \"${PKGSRC_COMPILER:Mgcc}\".") + + // No warning because it is not obvious what is meant here. + // This case probably doesn't occur in practice. + test( + "${PKGSRC_COMPILER} == \"distcc gcc\"", + + nil...) +} + func (s *Suite) Test_MkLineChecker_checkDirectiveFor(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/mklineparser.go b/pkgtools/pkglint/files/mklineparser.go index 4d638f9c3b3..94c911550f6 100644 --- a/pkgtools/pkglint/files/mklineparser.go +++ b/pkgtools/pkglint/files/mklineparser.go @@ -247,7 +247,7 @@ func (p MkLineParser) parseSysinclude(line *Line, splitResult mkLineSplitResult) return nil } - return &MkLine{line, splitResult, &mkLineInclude{directive == "include", true, indent, includedFile, nil}} + return &MkLine{line, splitResult, &mkLineInclude{directive == "include", true, indent, NewPath(includedFile), nil}} } func (p MkLineParser) parseDependency(line *Line, splitResult mkLineSplitResult) *MkLine { diff --git a/pkgtools/pkglint/files/mklineparser_test.go b/pkgtools/pkglint/files/mklineparser_test.go index b3b3b183817..a9bb8413fc5 100644 --- a/pkgtools/pkglint/files/mklineparser_test.go +++ b/pkgtools/pkglint/files/mklineparser_test.go @@ -543,7 +543,7 @@ func (s *Suite) Test_MkLineParser_parseInclude(c *check.C) { t.CheckEquals(mkline.IsInclude(), true) t.CheckEquals(mkline.Indent(), " ") t.CheckEquals(mkline.MustExist(), true) - t.CheckEquals(mkline.IncludedFile(), "../../mk/bsd.prefs.mk") + t.CheckEquals(mkline.IncludedFile(), NewPath("../../mk/bsd.prefs.mk")) t.CheckEquals(mkline.IsSysinclude(), false) } @@ -557,7 +557,7 @@ func (s *Suite) Test_MkLineParser_parseSysinclude(c *check.C) { t.CheckEquals(mkline.IsSysinclude(), true) t.CheckEquals(mkline.Indent(), " ") t.CheckEquals(mkline.MustExist(), true) - t.CheckEquals(mkline.IncludedFile(), "subdir.mk") + t.CheckEquals(mkline.IncludedFile(), NewPath("subdir.mk")) t.CheckEquals(mkline.IsInclude(), false) } diff --git a/pkgtools/pkglint/files/mklines.go b/pkgtools/pkglint/files/mklines.go index 486eae68661..ad518ff4cdc 100644 --- a/pkgtools/pkglint/files/mklines.go +++ b/pkgtools/pkglint/files/mklines.go @@ -70,7 +70,7 @@ func NewMkLines(lines *Lines) *MkLines { func (mklines *MkLines) Check() { if trace.Tracing { - defer trace.Call1(mklines.lines.Filename)() + defer trace.Call(mklines.lines.Filename)() } // In the first pass, all additions to BUILD_DEFS and USE_TOOLS @@ -440,7 +440,7 @@ func (mklines *MkLines) checkVarassignPlist(mkline *MkLine) { // CheckUsedBy checks that this file (a Makefile.common) has the given // relativeName in one of the "# used by" comments at the beginning of the file. -func (mklines *MkLines) CheckUsedBy(relativeName string) { +func (mklines *MkLines) CheckUsedBy(relativeName Path) { lines := mklines.lines if lines.Len() < 3 { return @@ -448,7 +448,7 @@ func (mklines *MkLines) CheckUsedBy(relativeName string) { paras := mklines.SplitToParagraphs() - expected := "# used by " + relativeName + expected := "# used by " + relativeName.String() found := false var usedParas []*Paragraph diff --git a/pkgtools/pkglint/files/mklines_test.go b/pkgtools/pkglint/files/mklines_test.go index 04bd725426f..883d225eeb5 100644 --- a/pkgtools/pkglint/files/mklines_test.go +++ b/pkgtools/pkglint/files/mklines_test.go @@ -1069,7 +1069,7 @@ func (s *Suite) Test_MkLines_CheckUsedBy__show_autofix(c *check.C) { t.SetUpCommandLine("--show-autofix") - test := func(pkgpath string, lines []string, diagnostics []string) { + test := func(pkgpath Path, lines []string, diagnostics []string) { mklines := t.NewMkLines("Makefile.common", lines...) mklines.CheckUsedBy(pkgpath) @@ -1155,7 +1155,7 @@ func (s *Suite) Test_MkLines_CheckUsedBy__show_autofix(c *check.C) { func (s *Suite) Test_MkLines_CheckUsedBy(c *check.C) { t := s.Init(c) - test := func(pkgpath string, lines []string, diagnostics []string) { + test := func(pkgpath Path, lines []string, diagnostics []string) { mklines := t.NewMkLines("Makefile.common", lines...) mklines.CheckUsedBy(pkgpath) diff --git a/pkgtools/pkglint/files/mkparser.go b/pkgtools/pkglint/files/mkparser.go index 19414ba22f8..4305248f094 100644 --- a/pkgtools/pkglint/files/mkparser.go +++ b/pkgtools/pkglint/files/mkparser.go @@ -410,6 +410,8 @@ func (p *MkParser) varUseModifierAt(lexer *textproc.Lexer, varname string) bool // MkCond parses a condition like ${OPSYS} == "NetBSD". // // See devel/bmake/files/cond.c. +// +// FIXME: Move over to MkTokensParser func (p *MkParser) MkCond() *MkCond { and := p.mkCondAnd() if and == nil { diff --git a/pkgtools/pkglint/files/package.go b/pkgtools/pkglint/files/package.go index 7029931e883..ec9e3175b97 100644 --- a/pkgtools/pkglint/files/package.go +++ b/pkgtools/pkglint/files/package.go @@ -3,7 +3,6 @@ package pkglint import ( "netbsd.org/pkglint/pkgver" "os" - "path" "strconv" "strings" ) @@ -17,12 +16,12 @@ const rePkgname = `^([\w\-.+]+)-(\d[.0-9A-Z_a-z]*)$` // This is necessary because variables in Makefiles may be used before they are defined, // and such dependencies often span multiple files that are included indirectly. type Package struct { - dir string // The directory of the package, for resolving files - Pkgpath string // e.g. "category/pkgdir" - Pkgdir string // PKGDIR from the package Makefile - Filesdir string // FILESDIR from the package Makefile - Patchdir string // PATCHDIR from the package Makefile - DistinfoFile string // DISTINFO_FILE from the package Makefile + dir Path // The directory of the package, for resolving files + Pkgpath Path // e.g. "category/pkgdir" + Pkgdir Path // PKGDIR from the package Makefile + Filesdir Path // FILESDIR from the package Makefile + Patchdir Path // PATCHDIR from the package Makefile + DistinfoFile Path // DISTINFO_FILE from the package Makefile EffectivePkgname string // PKGNAME or DISTNAME from the package Makefile, including nb13, can be empty EffectivePkgbase string // EffectivePkgname without the version EffectivePkgversion string // The version part of the effective PKGNAME, excluding nb13 @@ -32,7 +31,7 @@ type Package struct { vars Scope redundant *RedundantScope - bl3 map[string]*MkLine // buildlink3.mk name => line; contains only buildlink3.mk files that are directly included. + bl3 map[Path]*MkLine // buildlink3.mk name => line; contains only buildlink3.mk files that are directly included. // Remembers the Makefile fragments that have already been included. // The key to the map is the filename relative to the package directory. @@ -50,21 +49,24 @@ type Package struct { // Files from .include lines that are nested inside .if. // They often depend on OPSYS or on the existence of files in the build environment. - conditionalIncludes map[string]*MkLine + conditionalIncludes map[Path]*MkLine // Files from .include lines that are not nested. // These are cross-checked with buildlink3.mk whether they are unconditional there, too. - unconditionalIncludes map[string]*MkLine + unconditionalIncludes map[Path]*MkLine IgnoreMissingPatches bool // In distinfo, don't warn about patches that cannot be found. Once Once } -func NewPackage(dir string) *Package { +func NewPackage(dir Path) *Package { pkgpath := G.Pkgsrc.ToRel(dir) // Package directory must be two subdirectories below the pkgsrc root. - assert(strings.Count(pkgpath, "/") == 1) + // As of November 2019, it is technically possible to create packages + // on different levels, but that is not used at all. Therefore all + // relative directories are in the form "../../category/package". + assert(pkgpath.Count() == 2) pkg := Package{ dir: dir, @@ -75,10 +77,10 @@ func NewPackage(dir string) *Package { DistinfoFile: "${PKGDIR}/distinfo", // TODO: Redundant, see the vars.Fallback below. Plist: NewPlistContent(), vars: NewScope(), - bl3: make(map[string]*MkLine), + bl3: make(map[Path]*MkLine), included: Once{}, - conditionalIncludes: make(map[string]*MkLine), - unconditionalIncludes: make(map[string]*MkLine), + conditionalIncludes: make(map[Path]*MkLine), + unconditionalIncludes: make(map[Path]*MkLine), } pkg.vars.DefineAll(G.Pkgsrc.UserDefinedVars) @@ -97,7 +99,7 @@ func NewPackage(dir string) *Package { return &pkg } -func (pkg *Package) load() ([]string, *MkLines, *MkLines) { +func (pkg *Package) load() ([]Path, *MkLines, *MkLines) { // Load the package Makefile and all included files, // to collect all used and defined variables and similar data. mklines, allLines := pkg.loadPackageMakefile() @@ -110,21 +112,30 @@ func (pkg *Package) load() ([]string, *MkLines, *MkLines) { files = append(files, dirglob(pkg.File(pkg.Pkgdir))...) } files = append(files, dirglob(pkg.File(pkg.Patchdir))...) - if pkg.DistinfoFile != pkg.vars.fallback["DISTINFO_FILE"] { + if pkg.DistinfoFile != NewPath(pkg.vars.fallback["DISTINFO_FILE"]) { files = append(files, pkg.File(pkg.DistinfoFile)) } + isRelevantMk := func(filename Path, basename string) bool { + if !hasPrefix(basename, "Makefile.") && !filename.HasSuffixText(".mk") { + return false + } + if filename.Dir().Base() == "patches" { + return false + } + if pkg.Pkgdir == "." { + return true + } + return !filename.ContainsPath(pkg.Pkgdir) + } + // Determine the used variables and PLIST directories before checking any of the Makefile fragments. // TODO: Why is this code necessary? What effect does it have? pkg.collectConditionalIncludes(mklines) for _, filename := range files { - basename := path.Base(filename) - if (hasPrefix(basename, "Makefile.") || hasSuffix(filename, ".mk")) && - !matches(filename, `patch-`) && - !contains(filename, pkg.Pkgdir+"/") && - !contains(filename, pkg.Filesdir+"/") { + basename := filename.Base() + if isRelevantMk(filename, basename) { fragmentMklines := LoadMk(filename, MustSucceed) - fragmentMklines.collectUsedVariables() pkg.collectConditionalIncludes(fragmentMklines) } if hasPrefix(basename, "PLIST") { @@ -138,7 +149,7 @@ func (pkg *Package) load() ([]string, *MkLines, *MkLines) { func (pkg *Package) loadPackageMakefile() (*MkLines, *MkLines) { filename := pkg.File("Makefile") if trace.Tracing { - defer trace.Call1(filename)() + defer trace.Call(filename)() } mainLines := LoadMk(filename, NotEmpty|LogErrors) @@ -170,10 +181,10 @@ func (pkg *Package) loadPackageMakefile() (*MkLines, *MkLines) { allLines.collectUsedVariables() - pkg.Pkgdir = pkg.vars.LastValue("PKGDIR") - pkg.DistinfoFile = pkg.vars.LastValue("DISTINFO_FILE") - pkg.Filesdir = pkg.vars.LastValue("FILESDIR") - pkg.Patchdir = pkg.vars.LastValue("PATCHDIR") + pkg.Pkgdir = NewPath(pkg.vars.LastValue("PKGDIR")) + pkg.DistinfoFile = NewPath(pkg.vars.LastValue("DISTINFO_FILE")) + pkg.Filesdir = NewPath(pkg.vars.LastValue("FILESDIR")) + pkg.Patchdir = NewPath(pkg.vars.LastValue("PATCHDIR")) // See lang/php/ext.mk if pkg.vars.IsDefinedSimilar("PHPEXT_MK") { @@ -191,19 +202,19 @@ func (pkg *Package) loadPackageMakefile() (*MkLines, *MkLines) { } if trace.Tracing { - trace.Step1("DISTINFO_FILE=%s", pkg.DistinfoFile) - trace.Step1("FILESDIR=%s", pkg.Filesdir) - trace.Step1("PATCHDIR=%s", pkg.Patchdir) - trace.Step1("PKGDIR=%s", pkg.Pkgdir) + trace.Stepf("DISTINFO_FILE=%s", pkg.DistinfoFile) + trace.Stepf("FILESDIR=%s", pkg.Filesdir) + trace.Stepf("PATCHDIR=%s", pkg.Patchdir) + trace.Stepf("PKGDIR=%s", pkg.Pkgdir) } return mainLines, allLines } // TODO: What is allLines used for, is it still necessary? Would it be better as a field in Package? -func (pkg *Package) parse(mklines *MkLines, allLines *MkLines, includingFileForUsedCheck string) bool { +func (pkg *Package) parse(mklines *MkLines, allLines *MkLines, includingFileForUsedCheck Path) bool { if trace.Tracing { - defer trace.Call1(mklines.lines.Filename)() + defer trace.Call(mklines.lines.Filename)() } result := mklines.ForEachEnd( @@ -217,10 +228,10 @@ func (pkg *Package) parse(mklines *MkLines, allLines *MkLines, includingFileForU // For every included buildlink3.mk, include the corresponding builtin.mk // automatically since the pkgsrc infrastructure does the same. filename := mklines.lines.Filename - if path.Base(filename) == "buildlink3.mk" { - builtin := cleanpath(path.Dir(filename) + "/builtin.mk") + if filename.Base() == "buildlink3.mk" { + builtin := cleanpath(filename.Dir().JoinNoClean("builtin.mk")) builtinRel := relpath(pkg.dir, builtin) - if pkg.included.FirstTime(builtinRel) && fileExists(builtin) { + if pkg.included.FirstTime(builtinRel.String()) && builtin.IsFile() { builtinMkLines := LoadMk(builtin, MustSucceed|LogErrors) pkg.parse(builtinMkLines, allLines, "") } @@ -246,8 +257,8 @@ func (pkg *Package) parseLine(mklines *MkLines, mkline *MkLine, allLines *MkLine return false } - filenameForUsedCheck := "" - dir, base := path.Split(includedFile) + filenameForUsedCheck := NewPath("") + dir, base := includedFile.Split() if dir != "" && base == "Makefile.common" && dir != "../../"+pkg.Pkgpath+"/" { filenameForUsedCheck = includingFile } @@ -276,14 +287,14 @@ func (pkg *Package) parseLine(mklines *MkLines, mkline *MkLine, allLines *MkLine // the included file is not processed further for whatever reason. But if // skip is false, the file could not be read and an appropriate error message // has already been logged. -func (pkg *Package) loadIncluded(mkline *MkLine, includingFile string) (includedMklines *MkLines, skip bool) { +func (pkg *Package) loadIncluded(mkline *MkLine, includingFile Path) (includedMklines *MkLines, skip bool) { includedFile := pkg.resolveIncludedFile(mkline, includingFile) if includedFile == "" { return nil, true } - dirname, _ := path.Split(includingFile) + dirname, _ := includingFile.Split() dirname = cleanpath(dirname) fullIncluded := joinPath(dirname, includedFile) relIncludedFile := relpath(pkg.dir, fullIncluded) @@ -292,14 +303,14 @@ func (pkg *Package) loadIncluded(mkline *MkLine, includingFile string) (included return nil, true } - if !pkg.included.FirstTime(relIncludedFile) { + if !pkg.included.FirstTime(relIncludedFile.String()) { return nil, true } pkg.collectSeenInclude(mkline, includedFile) if trace.Tracing { - trace.Step1("Including %q.", fullIncluded) + trace.Stepf("Including %q.", fullIncluded) } includedMklines = LoadMk(fullIncluded, 0) if includedMklines != nil { @@ -330,7 +341,7 @@ func (pkg *Package) loadIncluded(mkline *MkLine, includingFile string) (included } mkline.Notef("The path to the included file should be %q.", - relpath(path.Dir(mkline.Filename), fullIncludedFallback)) + mkline.PathToFile(fullIncludedFallback)) mkline.Explain( "The .include directive first searches the file relative to the including file.", "And if that doesn't exist, falls back to the current directory, which in the", @@ -344,13 +355,15 @@ func (pkg *Package) loadIncluded(mkline *MkLine, includingFile string) (included // resolveIncludedFile resolves Makefile variables such as ${PKGPATH} to // their actual values. -func (pkg *Package) resolveIncludedFile(mkline *MkLine, includingFilename string) string { +func (pkg *Package) resolveIncludedFile(mkline *MkLine, includingFilename Path) Path { // TODO: resolveVariableRefs uses G.Pkg implicitly. It should be made explicit. // TODO: Try to combine resolveVariableRefs and ResolveVarsInRelativePath. - includedFile := resolveVariableRefs(nil /* XXX: or maybe some mklines? */, mkline.ResolveVarsInRelativePath(mkline.IncludedFile())) - if containsVarRef(includedFile) { - if trace.Tracing && !contains(includingFilename, "/mk/") { + resolved := mkline.ResolveVarsInRelativePath(mkline.IncludedFile()) + includedText := resolveVariableRefs(nil /* XXX: or maybe some mklines? */, resolved.String()) + includedFile := NewPath(includedText) + if containsVarRef(includedText) { + if trace.Tracing && !includingFilename.ContainsPath("mk") { trace.Stepf("%s:%s: Skipping unresolvable include file %q.", mkline.Filename, mkline.Linenos(), includedFile) } @@ -358,10 +371,10 @@ func (pkg *Package) resolveIncludedFile(mkline *MkLine, includingFilename string } if mkline.Basename != "buildlink3.mk" { - if hasSuffix(includedFile, "/buildlink3.mk") { + if includedFile.HasSuffixText("/buildlink3.mk") { pkg.bl3[includedFile] = mkline if trace.Tracing { - trace.Step1("Buildlink3 file in package: %q", includedFile) + trace.Step1("Buildlink3 file in package: %q", includedText) } } } @@ -373,28 +386,31 @@ func (pkg *Package) resolveIncludedFile(mkline *MkLine, includingFilename string // // The includingFile is relative to the current working directory, // the includedFile is taken directly from the .include directive. -func (*Package) shouldDiveInto(includingFile string, includedFile string) bool { +func (*Package) shouldDiveInto(includingFile, includedFile Path) bool { - if hasSuffix(includedFile, "/bsd.pkg.mk") || IsPrefs(includedFile) { + if includedFile.HasSuffixPath("bsd.pkg.mk") || IsPrefs(includedFile) { return false } - if contains(includingFile, "/mk/") && !hasPrefix(G.Pkgsrc.ToRel(includingFile), "wip/mk") { - return hasSuffix(includingFile, "buildlink3.mk") && hasSuffix(includedFile, "builtin.mk") + // FIXME: includingFile may be "../../mk/../devel/readline/buildlink.mk" and thus contain "mk" + // even though the resolved file is not part of the pkgsrc infrastructure. + if includingFile.ContainsPath("mk") && !G.Pkgsrc.ToRel(includingFile).HasPrefixPath("wip/mk") { + // TODO: try ".buildlink.mk", ".builtin.mk" instead, see wip/clfswm. + return includingFile.HasSuffixText("buildlink3.mk") && includedFile.HasSuffixText("builtin.mk") } return true } -func (pkg *Package) collectSeenInclude(mkline *MkLine, includedFile string) { +func (pkg *Package) collectSeenInclude(mkline *MkLine, includedFile Path) { if mkline.Basename != "Makefile" { return } - incDir, incBase := path.Split(includedFile) + incDir, incBase := includedFile.Split() switch { case - hasPrefix(incDir, "../../mk/"), + incDir.HasPrefixPath("../../mk"), incBase == "buildlink3.mk", incBase == "builtin.mk", incBase == "options.mk": @@ -402,7 +418,7 @@ func (pkg *Package) collectSeenInclude(mkline *MkLine, includedFile string) { } if trace.Tracing { - trace.Step1("Including %q sets seenInclude.", includedFile) + trace.Stepf("Including %q sets seenInclude.", includedFile) } pkg.seenInclude = true } @@ -412,22 +428,22 @@ func (pkg *Package) collectConditionalIncludes(mklines *MkLines) { if mkline.IsInclude() { mkline.SetConditionalVars(mklines.indentation.Varnames()) - key := pkg.Rel(mkline.IncludedFileFull()) + includedFile := pkg.Rel(mkline.IncludedFileFull()) if mklines.indentation.IsConditional() { - pkg.conditionalIncludes[key] = mkline + pkg.conditionalIncludes[includedFile] = mkline } else { - pkg.unconditionalIncludes[key] = mkline + pkg.unconditionalIncludes[includedFile] = mkline } } }) } -func (pkg *Package) loadPlistDirs(plistFilename string) { +func (pkg *Package) loadPlistDirs(plistFilename Path) { lines := Load(plistFilename, MustSucceed) ck := PlistChecker{ pkg, - make(map[string]*PlistLine), - make(map[string]*PlistLine), + make(map[Path]*PlistLine), + make(map[Path]*PlistLine), "", Once{}, false} @@ -441,19 +457,19 @@ func (pkg *Package) loadPlistDirs(plistFilename string) { } } -func (pkg *Package) check(filenames []string, mklines, allLines *MkLines) { +func (pkg *Package) check(filenames []Path, mklines, allLines *MkLines) { haveDistinfo := false havePatches := false for _, filename := range filenames { - if containsVarRef(filename) { + if containsVarRef(filename.String()) { if trace.Tracing { trace.Stepf("Skipping file %q because the name contains an unresolved variable.", filename) } continue } - st, err := os.Lstat(filename) + st, err := filename.Lstat() switch { case err != nil: // For a missing custom distinfo file, an error message is already generated @@ -463,7 +479,7 @@ func (pkg *Package) check(filenames []string, mklines, allLines *MkLines) { // since all those files come from calls to dirglob. break - case path.Base(filename) == "Makefile" && strings.Count(G.Pkgsrc.ToRel(filename), "/") == 2: + case filename.HasBase("Makefile") && G.Pkgsrc.ToRel(filename).Count() == 3: G.checkExecutable(filename, st.Mode()) pkg.checkfilePackageMakefile(filename, mklines, allLines) @@ -471,9 +487,9 @@ func (pkg *Package) check(filenames []string, mklines, allLines *MkLines) { pkg.checkDirent(filename, st.Mode()) } - if contains(filename, "/patches/patch-") { + if filename.ContainsText("/patches/patch-") { havePatches = true - } else if hasSuffix(filename, "/distinfo") { + } else if filename.HasSuffixPath("distinfo") { haveDistinfo = true } pkg.checkOwnerMaintainer(filename) @@ -491,9 +507,9 @@ func (pkg *Package) check(filenames []string, mklines, allLines *MkLines) { } } -func (pkg *Package) checkfilePackageMakefile(filename string, mklines *MkLines, allLines *MkLines) { +func (pkg *Package) checkfilePackageMakefile(filename Path, mklines *MkLines, allLines *MkLines) { if trace.Tracing { - defer trace.Call1(filename)() + defer trace.Call(filename)() } vars := pkg.vars @@ -506,12 +522,12 @@ func (pkg *Package) checkfilePackageMakefile(filename string, mklines *MkLines, if !want { distinfoFile := pkg.File(pkg.DistinfoFile) - if fileExists(distinfoFile) { + if distinfoFile.IsFile() { NewLineWhole(distinfoFile).Warnf("This file should not exist since NO_CHECKSUM or META_PACKAGE is set.") } } else { distinfoFile := pkg.File(pkg.DistinfoFile) - if !containsVarRef(distinfoFile) && !fileExists(distinfoFile) { + if !containsVarRef(distinfoFile.String()) && !distinfoFile.IsFile() { line := NewLineWhole(distinfoFile) line.Warnf("A package that downloads files should have a distinfo file.") line.Explain( @@ -554,7 +570,7 @@ func (pkg *Package) checkfilePackageMakefile(filename string, mklines *MkLines, if imake := vars.FirstDefinition("USE_IMAKE"); imake != nil { if x11 := vars.FirstDefinition("USE_X11"); x11 != nil { - if !hasSuffix(x11.Filename, "/mk/x11.buildlink3.mk") { + if !x11.Filename.HasSuffixPath("mk/x11.buildlink3.mk") { imake.Notef("USE_IMAKE makes USE_X11 in %s redundant.", imake.RefTo(x11)) } } @@ -592,8 +608,8 @@ func (pkg *Package) checkPlist() { } needsPlist, line := pkg.needsPlist() - hasPlist := fileExists(pkg.File(pkg.Pkgdir+"/PLIST")) || - fileExists(pkg.File(pkg.Pkgdir+"/PLIST.common")) + hasPlist := pkg.File(pkg.Pkgdir.JoinNoClean("PLIST")).IsFile() || + pkg.File(pkg.Pkgdir.JoinNoClean("/PLIST.common")).IsFile() if needsPlist && !hasPlist { line.Warnf("This package should have a PLIST file.") @@ -966,7 +982,7 @@ func (pkg *Package) checkUseLanguagesCompilerMk(mklines *MkLines) { } handleInclude := func(mkline *MkLine) { - _ = seen.FirstTime(pkg.Rel(mkline.IncludedFileFull())) + _ = seen.FirstTime(pkg.Rel(mkline.IncludedFileFull()).String()) } mklines.ForEach(func(mkline *MkLine) { @@ -1103,7 +1119,7 @@ func (pkg *Package) checkPossibleDowngrade() { change := G.Pkgsrc.LastChange[pkg.Pkgpath] if change == nil { if trace.Tracing { - trace.Step1("No change log for package %q", pkg.Pkgpath) + trace.Stepf("No change log for package %q", pkg.Pkgpath) } return } @@ -1147,44 +1163,51 @@ func (pkg *Package) checkUpdate() { } suggver, comment := sugg.Version, sugg.Comment - if comment != "" { - comment = " (" + comment + ")" + + commentSuffix := func() string { + if comment != "" { + return " (" + comment + ")" + } + return "" } - pkgnameLine := pkg.EffectivePkgnameLine + mkline := pkg.EffectivePkgnameLine cmp := pkgver.Compare(pkg.EffectivePkgversion, suggver) + ref := mkline.RefToLocation(sugg.Line) switch { case cmp < 0: - pkgnameLine.Warnf("This package should be updated to %s%s.", - sugg.Version, comment) - pkgnameLine.Explain( - "The wishlist for package updates in doc/TODO mentions that a newer", - "version of this package is available.") + if comment != "" { + mkline.Warnf("This package should be updated to %s (%s; see %s).", + sugg.Version, comment, ref) + } else { + mkline.Warnf("This package should be updated to %s (see %s).", + sugg.Version, ref) + } case cmp > 0: - pkgnameLine.Notef("This package is newer than the update request to %s%s.", - suggver, comment) + mkline.Notef("This package is newer than the update request to %s%s from %s.", + suggver, commentSuffix(), ref) default: - pkgnameLine.Notef("The update request to %s from doc/TODO%s has been done.", - suggver, comment) + mkline.Notef("The update request to %s%s from %s has been done.", + suggver, commentSuffix(), ref) } } } // checkDirent checks a directory entry based on its filename and its mode // (regular file, directory, symlink). -func (pkg *Package) checkDirent(dirent string, mode os.FileMode) { +func (pkg *Package) checkDirent(dirent Path, mode os.FileMode) { // TODO: merge duplicate code in Pkglint.checkMode - basename := path.Base(dirent) + basename := dirent.Base() switch { case mode.IsRegular(): pkgsrcRel := G.Pkgsrc.ToRel(dirent) - depth := strings.Count(pkgsrcRel, "/") + depth := pkgsrcRel.Count() - 1 // FIXME G.checkReg(dirent, basename, depth) case hasPrefix(basename, "work"): @@ -1197,7 +1220,7 @@ func (pkg *Package) checkDirent(dirent string, mode os.FileMode) { switch { case basename == "files", basename == "patches", - matches(dirent, `(?:^|/)files/[^/]*$`), + dirent.Dir().Base() == "files", isEmptyDir(dirent): break @@ -1219,7 +1242,7 @@ func (pkg *Package) checkDirent(dirent string, mode os.FileMode) { // // Pkglint assumes that the local username is the same as the NetBSD // username, which fits most scenarios. -func (pkg *Package) checkOwnerMaintainer(filename string) { +func (pkg *Package) checkOwnerMaintainer(filename Path) { if trace.Tracing { defer trace.Call(filename)() } @@ -1261,7 +1284,7 @@ func (pkg *Package) checkOwnerMaintainer(filename string) { "keyword \"maintainer\", for more information.") } -func (pkg *Package) checkFreeze(filename string) { +func (pkg *Package) checkFreeze(filename Path) { freezeStart := G.Pkgsrc.LastFreezeStart if freezeStart == "" || G.Pkgsrc.LastFreezeEnd != "" { return @@ -1278,8 +1301,8 @@ func (pkg *Package) checkFreeze(filename string) { "See https://www.NetBSD.org/developers/pkgsrc/ for the exact rules.") } -func (pkg *Package) checkFileMakefileExt(filename string) { - base := path.Base(filename) +func (pkg *Package) checkFileMakefileExt(filename Path) { + base := filename.Base() if !hasPrefix(base, "Makefile.") || base == "Makefile.common" { return } @@ -1313,11 +1336,11 @@ func (pkg *Package) checkLinesBuildlink3Inclusion(mklines *MkLines) { } // Collect all the included buildlink3.mk files from the file. - includedFiles := make(map[string]*MkLine) + includedFiles := make(map[Path]*MkLine) for _, mkline := range mklines.mklines { if mkline.IsInclude() { includedFile := mkline.IncludedFile() - if hasSuffix(includedFile, "/buildlink3.mk") { + if includedFile.HasSuffixPath("buildlink3.mk") { includedFiles[includedFile] = mkline if pkg.bl3[includedFile] == nil { mkline.Warnf("%s is included by this file but not by the package.", includedFile) @@ -1329,7 +1352,7 @@ func (pkg *Package) checkLinesBuildlink3Inclusion(mklines *MkLines) { if trace.Tracing { for packageBl3 := range pkg.bl3 { if includedFiles[packageBl3] == nil { - trace.Step1("%s is included by the package but not by the buildlink3.mk file.", packageBl3) + trace.Stepf("%s is included by the package but not by the buildlink3.mk file.", packageBl3) } } } @@ -1405,8 +1428,10 @@ func (pkg *Package) AutofixDistinfo(oldSha1, newSha1 string) { // File returns the (possibly absolute) path to relativeFileName, // as resolved from the package's directory. // Variables that are known in the package are resolved, e.g. ${PKGDIR}. -func (pkg *Package) File(relativeFileName string) string { - return cleanpath(resolveVariableRefs(nil /* XXX: or maybe some mklines? */, joinPath(pkg.dir, relativeFileName))) +func (pkg *Package) File(relativeFileName Path) Path { + joined := pkg.dir.JoinNoClean(relativeFileName) + resolved := resolveVariableRefs(nil /* XXX: or maybe some mklines? */, joined.String()) + return cleanpath(NewPath(resolved)) } // Rel returns the path by which the given filename (as seen from the @@ -1415,13 +1440,13 @@ func (pkg *Package) File(relativeFileName string) string { // // Example: // NewPackage("category/package").Rel("other/package") == "../../other/package" -func (pkg *Package) Rel(filename string) string { +func (pkg *Package) Rel(filename Path) Path { return relpath(pkg.dir, filename) } // Returns whether the given file (relative to the package directory) // is included somewhere in the package, either directly or indirectly. -func (pkg *Package) Includes(filename string) bool { +func (pkg *Package) Includes(filename Path) bool { return pkg.unconditionalIncludes[filename] != nil || pkg.conditionalIncludes[filename] != nil } @@ -1435,12 +1460,12 @@ func (pkg *Package) Includes(filename string) bool { // 2. Ensure that the entries mentioned in the ALTERNATIVES file // also appear in the PLIST files. type PlistContent struct { - Dirs map[string]*PlistLine - Files map[string]*PlistLine + Dirs map[Path]*PlistLine + Files map[Path]*PlistLine } func NewPlistContent() PlistContent { return PlistContent{ - make(map[string]*PlistLine), - make(map[string]*PlistLine)} + make(map[Path]*PlistLine), + make(map[Path]*PlistLine)} } diff --git a/pkgtools/pkglint/files/package_test.go b/pkgtools/pkglint/files/package_test.go index 5285b635d9b..a24048a445a 100644 --- a/pkgtools/pkglint/files/package_test.go +++ b/pkgtools/pkglint/files/package_test.go @@ -311,7 +311,7 @@ func (s *Suite) Test_Package__case_insensitive(c *check.C) { t.FinishSetUp() // this test is only interesting on a case-insensitive filesystem - if !fileExists(t.File("mk/BSD.PKG.MK")) { + if !t.File("mk/BSD.PKG.MK").IsFile() { return } @@ -332,6 +332,64 @@ func (s *Suite) Test_NewPackage(c *check.C) { t.ExpectAssert(func() { NewPackage("category") }) } +func (s *Suite) Test_Package_load__variable_from_Makefile_used_in_builtin_mk(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("devel/binutils", + "BINUTILS_PREFIX=\t${PREFIX}/${MACHINE_GNU_PLATFORM}") + t.CreateFileLines("devel/binutils/builtin.mk", + MkCvsID, + ".include \"../../mk/bsd.prefs.mk\"", + "BINUTILS_PREFIX?=\t/usr", + "", + "BUILTIN_FIND_FILES.BINUTILS_FILES:=\t${BINUTILS_PREFIX}/include/bfd.h") + t.FinishSetUp() + + G.Check(t.File("devel/binutils")) + + // The BINUTILS_PREFIX from the Makefile is not used since the + // builtin.mk file is only parsed inside of buildlink3.mk, and + // that doesn't happen for the package itself, but only for those + // packages that depend on this package. + t.CheckOutputLines( + "WARN: ~/devel/binutils/Makefile:20: " + + "BINUTILS_PREFIX is defined but not used.") +} + +func (s *Suite) Test_Package_load__buildlink3_mk_includes_other_mk(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("-Wall", "--explain") + t.SetUpPackage("multimedia/libav") + t.CreateFileDummyBuildlink3("multimedia/libav/buildlink3.mk", + ".include \"available.mk\"") + t.CreateFileLines("multimedia/libav/available.mk", + MkCvsID, + "", + "LIBAV_AVAILABLE=\tno") + t.FinishSetUp() + + G.Check(t.File("multimedia/libav")) + + // From looking at the file available.mk alone, this variable looks + // unused indeed, but its intention is to be used by other packages. + // The explanation has a large paragraph covering exactly this case, + // therefore the warning is ok. + t.CheckOutputLines( + "WARN: ~/multimedia/libav/available.mk:3: "+ + "LIBAV_AVAILABLE is defined but not used.", + "", + "\tThis might be a simple typo.", + "", + "\tIf a package provides a file containing several related variables", + "\t(such as module.mk, app.mk, extension.mk), that file may define", + "\tvariables that look unused since they are only used by other", + "\tpackages. These variables should be documented at the head of the", + "\tfile; see mk/subst.mk for an example of such a documentation", + "\tcomment.", + "") +} + // Demonstrates that Makefile fragments are handled differently, // depending on the directory they are in. func (s *Suite) Test_Package_load__extra_files(c *check.C) { @@ -357,7 +415,7 @@ func (s *Suite) Test_Package_load__extra_files(c *check.C) { "@@ -1,1 +1,1 @@", "- old", "+ new") - t.CreateFileLines("patches/readme.mk", + t.CreateFileLines("patches/readme.mk", // Is ignored "This is not a BSD-style Makefile.") t.Copy("gnu-style.mk", "files/gnu-style.mk") t.Copy("gnu-style.mk", "../../category/other/gnu-style.mk") @@ -374,10 +432,6 @@ func (s *Suite) Test_Package_load__extra_files(c *check.C) { "ERROR: gnu-style.mk:3: Unknown Makefile line format: \"else\".", "ERROR: gnu-style.mk:5: Unknown Makefile line format: \"endif\".", - // Since the patches directory should contain only patches, - // each other file is treated as a file belonging to pkgsrc, - // therefore *.mk is interpreted as a Makefile fragment. - "ERROR: patches/readme.mk:1: Unknown Makefile line format: \"This is not a BSD-style Makefile.\".", "ERROR: distinfo: Patch \"patches/patch-Makefile.mk\" is not recorded. Run \""+confMake+" makepatchsum\".", // The following diagnostics are duplicated because the files from @@ -1011,7 +1065,7 @@ func (s *Suite) Test_Package_shouldDiveInto(c *check.C) { t := s.Init(c) t.Chdir(".") - test := func(including, included string, expected bool) { + test := func(including, included Path, expected bool) { actual := (*Package)(nil).shouldDiveInto(including, included) t.CheckEquals(actual, expected) } @@ -1105,13 +1159,13 @@ func (s *Suite) Test_Package_loadPlistDirs__empty(c *check.C) { pkg := NewPackage(t.File("category/package")) pkg.load() - var dirs []string + var dirs []Path for dir := range pkg.Plist.Dirs { dirs = append(dirs, dir) } - sort.Strings(dirs) + sort.Slice(dirs, func(i, j int) bool { return dirs[i] < dirs[j] }) - t.CheckDeepEquals(dirs, []string{"bin"}) + t.CheckDeepEquals(dirs, []Path{"bin"}) } func (s *Suite) Test_Package_loadPlistDirs(c *check.C) { @@ -1128,13 +1182,13 @@ func (s *Suite) Test_Package_loadPlistDirs(c *check.C) { pkg := NewPackage(t.File("category/package")) pkg.load() - var dirs []string + var dirs []Path for dir := range pkg.Plist.Dirs { dirs = append(dirs, dir) } - sort.Strings(dirs) + sort.Slice(dirs, func(i, j int) bool { return dirs[i] < dirs[j] }) - t.CheckDeepEquals(dirs, []string{"bin", "dir", "dir/subdir"}) + t.CheckDeepEquals(dirs, []Path{"bin", "dir", "dir/subdir"}) } func (s *Suite) Test_Package_check__files_Makefile(c *check.C) { @@ -2456,6 +2510,8 @@ func (s *Suite) Test_Package_checkPossibleDowngrade__locally_modified_update(c * func (s *Suite) Test_Package_checkUpdate(c *check.C) { t := s.Init(c) + // The package names intentionally differ from the package directories + // to ensure that the check uses the package name. t.SetUpPackage("category/pkg1", "PKGNAME= package1-1.0") t.SetUpPackage("category/pkg2", @@ -2463,27 +2519,29 @@ func (s *Suite) Test_Package_checkUpdate(c *check.C) { t.SetUpPackage("category/pkg3", "PKGNAME= package3-5.0") t.CreateFileLines("doc/TODO", + CvsID, "Suggested package updates", + "=========================", + "For possible Perl packages updates, see http://www.NetBSD.org/~wiz/perl.html.", "", - "", - "\t"+"O wrong bullet", - "\t"+"o package-without-version", "\t"+"o package1-1.0", + "\t"+"o package1-1.0 [with comment]", + "\t"+"o package2-2.0", "\t"+"o package2-2.0 [nice new features]", + "\t"+"o package3-3.0", "\t"+"o package3-3.0 [security update]") t.Chdir(".") t.Main("-Wall,no-space", "category/pkg1", "category/pkg2", "category/pkg3") t.CheckOutputLines( - "WARN: category/pkg1/../../doc/TODO:3: Invalid line format \"\".", - "WARN: category/pkg1/../../doc/TODO:4: Invalid line format \"\\tO wrong bullet\".", - "WARN: category/pkg1/../../doc/TODO:5: Invalid package name \"package-without-version\".", - "NOTE: category/pkg1/Makefile:4: The update request to 1.0 from doc/TODO has been done.", - "WARN: category/pkg2/Makefile:4: This package should be updated to 2.0 ([nice new features]).", - "NOTE: category/pkg3/Makefile:4: This package is newer than the update request to 3.0 ([security update]).", - "4 warnings and 2 notes found.", - "(Run \"pkglint -e -Wall,no-space category/pkg1 category/pkg2 category/pkg3\" to show explanations.)") + "NOTE: category/pkg1/Makefile:4: The update request to 1.0 from ../../doc/TODO:6 has been done.", + "NOTE: category/pkg1/Makefile:4: The update request to 1.0 (with comment) from ../../doc/TODO:7 has been done.", + "WARN: category/pkg2/Makefile:4: This package should be updated to 2.0 (see ../../doc/TODO:8).", + "WARN: category/pkg2/Makefile:4: This package should be updated to 2.0 (nice new features; see ../../doc/TODO:9).", + "NOTE: category/pkg3/Makefile:4: This package is newer than the update request to 3.0 from ../../doc/TODO:10.", + "NOTE: category/pkg3/Makefile:4: This package is newer than the update request to 3.0 (security update) from ../../doc/TODO:11.", + "2 warnings and 4 notes found.") } func (s *Suite) Test_Package_checkDirent__errors(c *check.C) { @@ -3162,8 +3220,12 @@ func (s *Suite) Test_Package_Includes(c *check.C) { t.CheckEquals(pkg.Includes("conditionally.mk"), true) t.CheckEquals(pkg.Includes("other.mk"), false) - // TODO: Strictly speaking, never.mk should be in conditionalIncludes. - // This is an edge case though. See collectConditionalIncludes and - // Indentation.IsConditional for the current implementation. - t.CheckEquals(pkg.conditionalIncludes["never.mk"], (*MkLine)(nil)) + // The file never.mk is in conditionalIncludes since pkglint only + // analyzes on the syntactical level. It doesn't evaluate the + // condition from the .if to see whether it is satisfiable. + // + // See Package.collectConditionalIncludes and Indentation.IsConditional. + t.CheckEquals( + pkg.conditionalIncludes["never.mk"].Location, + NewLocation(t.File("category/package/Makefile"), 22, 22)) } diff --git a/pkgtools/pkglint/files/patches.go b/pkgtools/pkglint/files/patches.go index df64453d1d8..2c46595191d 100644 --- a/pkgtools/pkglint/files/patches.go +++ b/pkgtools/pkglint/files/patches.go @@ -79,7 +79,7 @@ func (ck *PatchChecker) Check() { } } - if patchedFiles > 1 && !matches(ck.lines.Filename, `\bCVE\b`) { + if patchedFiles > 1 && !matches(ck.lines.Filename.String(), `\bCVE\b`) { ck.lines.Whole().Warnf("Contains patches for %d files, should be only one.", patchedFiles) } else if patchedFiles == 0 { ck.lines.Whole().Errorf("Contains no patch.") diff --git a/pkgtools/pkglint/files/path.go b/pkgtools/pkglint/files/path.go new file mode 100644 index 00000000000..746c5572fc7 --- /dev/null +++ b/pkgtools/pkglint/files/path.go @@ -0,0 +1,162 @@ +package pkglint + +import ( + "io/ioutil" + "os" + "path" + "path/filepath" + "strings" +) + +// Path is a slash-separated path in the filesystem. +// It may be absolute or relative. +// Some paths may contain placeholders like @VAR@ or ${VAR}. +// The base directory of relative paths depends on the context +// in which the path is used. +type Path string + +func NewPath(name string) Path { return Path(name) } + +func NewPathSlash(name string) Path { return Path(filepath.ToSlash(name)) } + +func (p Path) String() string { return string(p) } + +func (p Path) GoString() string { return sprintf("%q", string(p)) } + +func (p Path) Dir() Path { return Path(path.Dir(string(p))) } + +func (p Path) Base() string { return path.Base(string(p)) } + +func (p Path) Split() (dir Path, base string) { + strDir, strBase := path.Split(string(p)) + return Path(strDir), strBase +} + +func (p Path) Parts() []string { + return strings.FieldsFunc(string(p), func(r rune) bool { return r == '/' }) +} + +func (p Path) Count() int { return len(p.Parts()) } + +func (p Path) HasPrefixText(prefix string) bool { + return hasPrefix(string(p), prefix) +} + +// HasPrefixPath returns whether the path starts with the given prefix. +// The basic unit of comparison is a path component, not a character. +func (p Path) HasPrefixPath(prefix Path) bool { + return hasPrefix(string(p), string(prefix)) && + (len(p) == len(prefix) || p[len(prefix)] == '/') +} + +// TODO: Check each call whether ContainsPath is more appropriate; add tests +func (p Path) ContainsText(contained string) bool { + return contains(string(p), contained) +} + +// ContainsPath returns whether the sub path is part of the path. +// The basic unit of comparison is a path component, not a character. +// +// Note that the paths used in pkglint may contains seemingly unnecessary +// components, like "../../wip/mk/../../devel/gettext-lib". To ignore these +// components, use ContainsPathCanonical instead. +func (p Path) ContainsPath(sub Path) bool { + limit := len(p) - len(sub) + for i := 0; i <= limit; i++ { + if (i == 0 || p[i-1] == '/') && p[i:].HasPrefixPath(sub) { + return true + } + } + return sub == "." +} + +func (p Path) ContainsPathCanonical(sub Path) bool { + cleaned := cleanpath(p) + return cleaned.ContainsPath(sub) +} + +// TODO: Check each call whether HasSuffixPath is more appropriate; add tests +func (p Path) HasSuffixText(suffix string) bool { + return hasSuffix(string(p), suffix) +} + +// HasSuffixPath returns whether the path ends with the given prefix. +// The basic unit of comparison is a path component, not a character. +func (p Path) HasSuffixPath(suffix Path) bool { + return hasSuffix(string(p), string(suffix)) && + (len(p) == len(suffix) || p[len(p)-len(suffix)-1] == '/') +} + +func (p Path) HasBase(base string) bool { return p.Base() == base } + +func (p Path) TrimSuffix(suffix string) Path { + return Path(strings.TrimSuffix(string(p), suffix)) +} + +func (p Path) Replace(from, to string) Path { + return Path(strings.Replace(string(p), from, to, -1)) +} + +func (p Path) JoinClean(s Path) Path { + return Path(path.Join(string(p), string(s))) +} + +func (p Path) JoinNoClean(s Path) Path { + return Path(string(p) + "/" + string(s)) +} + +func (p Path) Clean() Path { return NewPath(path.Clean(string(p))) } + +func (p Path) IsAbs() bool { + return p.HasPrefixText("/") || filepath.IsAbs(filepath.FromSlash(string(p))) +} + +func (p Path) Rel(other Path) Path { + fp := filepath.FromSlash(p.String()) + fpOther := filepath.FromSlash(other.String()) + rel, err := filepath.Rel(fp, fpOther) + assertNil(err, "relpath from %q to %q", p, other) + return NewPath(filepath.ToSlash(rel)) +} + +func (p Path) Rename(newName Path) error { + return os.Rename(string(p), string(newName)) +} + +func (p Path) Lstat() (os.FileInfo, error) { return os.Lstat(string(p)) } + +func (p Path) Stat() (os.FileInfo, error) { return os.Stat(string(p)) } + +func (p Path) Exists() bool { + _, err := p.Lstat() + return err == nil +} + +func (p Path) IsFile() bool { + info, err := p.Lstat() + return err == nil && info.Mode().IsRegular() +} + +func (p Path) IsDir() bool { + info, err := p.Lstat() + return err == nil && info.IsDir() +} + +func (p Path) Chmod(mode os.FileMode) error { + return os.Chmod(string(p), mode) +} + +func (p Path) ReadDir() ([]os.FileInfo, error) { + return ioutil.ReadDir(string(p)) +} + +func (p Path) Open() (*os.File, error) { return os.Open(string(p)) } + +func (p Path) ReadString() (string, error) { + bytes, err := ioutil.ReadFile(string(p)) + return string(bytes), err +} + +func (p Path) WriteString(s string) error { + return ioutil.WriteFile(string(p), []byte(s), 0666) +} diff --git a/pkgtools/pkglint/files/path_test.go b/pkgtools/pkglint/files/path_test.go new file mode 100644 index 00000000000..51b9aec1232 --- /dev/null +++ b/pkgtools/pkglint/files/path_test.go @@ -0,0 +1,535 @@ +package pkglint + +import ( + "gopkg.in/check.v1" + "io" + "os" + "runtime" + "strings" +) + +func (s *Suite) Test_NewPath(c *check.C) { + t := s.Init(c) + + t.CheckEquals(NewPath("filename"), NewPath("filename")) + t.CheckEquals(NewPath("\\"), NewPath("\\")) + c.Check(NewPath("\\"), check.Not(check.Equals), NewPath("/")) +} + +func (s *Suite) Test_NewPathSlash(c *check.C) { + t := s.Init(c) + + t.CheckEquals(NewPathSlash("filename"), NewPathSlash("filename")) + t.CheckEquals(NewPathSlash("\\"), NewPathSlash("\\")) + + t.CheckEquals( + NewPathSlash("\\"), + NewPathSlash(condStr(runtime.GOOS == "windows", "/", "\\"))) +} + +func (s *Suite) Test_Path_String(c *check.C) { + t := s.Init(c) + + for _, p := range []string{"", "filename", "a/b", "c\\d"} { + t.CheckEquals(NewPath(p).String(), p) + } +} + +func (s *Suite) Test_Path_GoString(c *check.C) { + t := s.Init(c) + + test := func(p Path, s string) { + t.CheckEquals(p.GoString(), s) + } + + test("", "\"\"") + test("filename", "\"filename\"") + test("a/b", "\"a/b\"") + test("c\\d", "\"c\\\\d\"") +} + +func (s *Suite) Test_Path_Dir(c *check.C) { + t := s.Init(c) + + test := func(p, dir Path) { + t.CheckEquals(p.Dir(), dir) + } + + test("", ".") + test("././././", ".") + test("/root", "/") + test("filename", ".") + test("dir/filename", "dir") + test("dir/filename\\with\\backslash", "dir") +} + +func (s *Suite) Test_Path_Base(c *check.C) { + t := s.Init(c) + + test := func(p Path, base string) { + t.CheckEquals(p.Base(), base) + } + + test("", ".") // That's a bit surprising + test("././././", ".") + test("/root", "root") + test("filename", "filename") + test("dir/filename", "filename") + test("dir/filename\\with\\backslash", "filename\\with\\backslash") +} + +func (s *Suite) Test_Path_Split(c *check.C) { + t := s.Init(c) + + test := func(p, dir, base string) { + actualDir, actualBase := NewPath(p).Split() + + t.CheckDeepEquals( + []string{actualDir.String(), actualBase}, + []string{dir, base}) + } + + test("", "", "") + test("././././", "././././", "") + test("/root", "/", "root") + test("filename", "", "filename") + test("dir/filename", "dir/", "filename") + test("dir/filename\\with\\backslash", "dir/", "filename\\with\\backslash") +} + +func (s *Suite) Test_Path_Parts(c *check.C) { + t := s.Init(c) + + test := func(p string, parts ...string) { + t.CheckDeepEquals(NewPath(p).Parts(), parts) + } + + test("", []string{}...) + test("././././", ".", ".", ".", ".") // No trailing "" + test("/root", "root") // No leading "" + test("filename", "filename") + test("dir/filename", "dir", "filename") + test("dir/filename\\with\\backslash", "dir", "filename\\with\\backslash") +} + +func (s *Suite) Test_Path_Count(c *check.C) { + t := s.Init(c) + + test := func(p string, count int) { + t.CheckEquals(NewPath(p).Count(), count) + } + + test("", 0) // FIXME + test("././././", 4) + test("/root", 1) // FIXME + test("filename", 1) + test("dir/filename", 2) + test("dir/filename\\with\\backslash", 2) +} + +func (s *Suite) Test_Path_HasPrefixText(c *check.C) { + t := s.Init(c) + + test := func(p, prefix string, hasPrefix bool) { + t.CheckEquals(NewPath(p).HasPrefixText(prefix), hasPrefix) + } + + test("", "", true) + test("filename", "", true) + test("", "x", false) + test("/root", "/r", true) + test("/root", "/root", true) + test("/root", "/root/", false) + test("/root", "root/", false) +} + +func (s *Suite) Test_Path_HasPrefixPath(c *check.C) { + t := s.Init(c) + + test := func(p, prefix Path, hasPrefix bool) { + t.CheckEquals(p.HasPrefixPath(prefix), hasPrefix) + } + + test("", "", true) + test("filename", "", false) + test("", "x", false) + test("/root", "/r", false) + test("/root", "/root", true) + test("/root", "/root/", false) + test("/root/", "/root", true) + test("/root/subdir", "/root", true) +} + +func (s *Suite) Test_Path_ContainsText(c *check.C) { + t := s.Init(c) + + test := func(p Path, text string, contains bool) { + t.CheckEquals(p.ContainsText(text), contains) + } + + test("", "", true) + test("filename", "", true) + test("filename", ".", false) + test("a.b", ".", true) + test("..", ".", true) + test("", "x", false) + test("/root", "/r", true) + test("/root", "/root", true) + test("/root", "/root/", false) + test("/root", "root/", false) + test("/root", "ro", true) + test("/root", "ot", true) +} + +func (s *Suite) Test_Path_ContainsPath(c *check.C) { + t := s.Init(c) + + test := func(p, sub Path, contains bool) { + t.CheckEquals(p.ContainsPath(sub), contains) + } + + test("", "", true) // It doesn't make sense to search for empty paths. + test(".", "", false) // It doesn't make sense to search for empty paths. + test("filename", ".", true) // Every path contains "." implicitly at the beginning + test("a.b", ".", true) + test("..", ".", true) + test("filename", "", false) + test("filename", "filename", true) + test("a/b/c", "a", true) + test("a/b/c", "b", true) + test("a/b/c", "c", true) + test("a/b/c", "a/b", true) + test("a/b/c", "b/c", true) + test("a/b/c", "a/b/c", true) + test("aa/b/c", "a", false) + test("a/bb/c", "b", false) + test("a/bb/c", "b/c", false) + test("mk/fetch/fetch.mk", "mk", true) + test("category/package/../../wip/mk/../..", "mk", true) +} + +func (s *Suite) Test_Path_ContainsPathCanonical(c *check.C) { + t := s.Init(c) + + test := func(p, sub Path, contains bool) { + t.CheckEquals(p.ContainsPathCanonical(sub), contains) + } + + test("", "", false) + test(".", "", false) + test("filename", "", false) + test("filename", "filename", true) + test("a/b/c", "a", true) + test("a/b/c", "b", true) + test("a/b/c", "c", true) + test("a/b/c", "a/b", true) + test("a/b/c", "b/c", true) + test("a/b/c", "a/b/c", true) + test("aa/b/c", "a", false) + test("a/bb/c", "b", false) + test("a/bb/c", "b/c", false) + test("mk/fetch/fetch.mk", "mk", true) + test("category/package/../../wip/mk", "mk", true) + test("category/package/../../wip/mk/..", "mk", true) // FIXME + test("category/package/../../wip/mk/../..", "mk", false) +} + +func (s *Suite) Test_Path_HasSuffixText(c *check.C) { + t := s.Init(c) + + test := func(p Path, suffix string, has bool) { + t.CheckEquals(p.HasSuffixText(suffix), has) + } + + test("", "", true) + test("a/bb/c", "", true) + test("a/bb/c", "c", true) + test("a/bb/c", "/c", true) + test("a/bb/c", "b/c", true) + test("aa/b/c", "bb", false) +} + +func (s *Suite) Test_Path_HasSuffixPath(c *check.C) { + t := s.Init(c) + + test := func(p, suffix Path, has bool) { + t.CheckEquals(p.HasSuffixPath(suffix), has) + } + + test("", "", true) + test("a/bb/c", "", false) + test("a/bb/c", "c", true) + test("a/bb/c", "/c", false) + test("a/bb/c", "b/c", false) + test("aa/b/c", "bb", false) +} + +func (s *Suite) Test_Path_HasBase(c *check.C) { + t := s.Init(c) + + test := func(p Path, suffix string, hasBase bool) { + t.CheckEquals(p.HasBase(suffix), hasBase) + } + + test("dir/file", "e", false) + test("dir/file", "file", true) + test("dir/file", "file.ext", false) + test("dir/file", "/file", false) + test("dir/file", "dir/file", false) +} + +func (s *Suite) Test_Path_TrimSuffix(c *check.C) { + t := s.Init(c) + + test := func(p Path, suffix string, result Path) { + t.CheckEquals(p.TrimSuffix(suffix), result) + } + + test("dir/file", "e", "dir/fil") + test("dir/file", "file", "dir/") + test("dir/file", "/file", "dir") + test("dir/file", "dir/file", "") + test("dir/file", "subdir/file", "dir/file") +} + +func (s *Suite) Test_Path_Replace(c *check.C) { + t := s.Init(c) + + test := func(p Path, from, to string, result Path) { + t.CheckEquals(p.Replace(from, to), result) + } + + test("dir/file", "dir", "other", "other/file") + test("dir/file", "r", "sk", "disk/file") + test("aaa/file", "a", "sub/", "sub/sub/sub//file") +} + +func (s *Suite) Test_Path_JoinClean(c *check.C) { + t := s.Init(c) + + test := func(p Path, suffix Path, result Path) { + t.CheckEquals(p.JoinClean(suffix), result) + } + + test("dir", "file", "dir/file") + test("dir", "///file", "dir/file") + test("dir/./../dir/", "///file", "dir/file") + test("dir", "..", ".") +} + +func (s *Suite) Test_Path_JoinNoClean(c *check.C) { + t := s.Init(c) + + test := func(p, suffix Path, result Path) { + t.CheckEquals(p.JoinNoClean(suffix), result) + } + + test("dir", "file", "dir/file") + test("dir", "///file", "dir////file") + test("dir/./../dir/", "///file", "dir/./../dir/////file") + test("dir", "..", "dir/..") +} + +func (s *Suite) Test_Path_Clean(c *check.C) { + t := s.Init(c) + + test := func(p, result Path) { + t.CheckEquals(p.Clean(), result) + } + + test("", ".") + test(".", ".") + test("./././", ".") + test("a/bb///../c", "a/c") +} + +func (s *Suite) Test_Path_IsAbs(c *check.C) { + t := s.Init(c) + + test := func(p Path, abs bool) { + t.CheckEquals(p.IsAbs(), abs) + } + + test("", false) + test(".", false) + test("a/b", false) + test("/a", true) + test("C:/", runtime.GOOS == "windows") + test("c:/", runtime.GOOS == "windows") +} + +func (s *Suite) Test_Path_Rel(c *check.C) { + t := s.Init(c) + + base := NewPath(".") + abc := NewPath("a/b/c") + defFile := NewPath("d/e/f/file") + + t.CheckEquals(abc.Rel(defFile), NewPath("../../../d/e/f/file")) + t.CheckEquals(base.Rel(base), NewPath(".")) + t.CheckEquals(abc.Rel(base), NewPath("../../../.")) +} + +func (s *Suite) Test_Path_Rename(c *check.C) { + t := s.Init(c) + + f := t.CreateFileLines("filename.old", + "line 1") + t.CheckEquals(f.IsFile(), true) + dst := NewPath(f.TrimSuffix(".old").String() + ".new") + + err := f.Rename(dst) + + assertNil(err, "Rename") + t.CheckEquals(f.IsFile(), false) + t.CheckFileLines("filename.new", + "line 1") +} + +func (s *Suite) Test_Path_Lstat(c *check.C) { + t := s.Init(c) + + testDir := func(f Path, isDir bool) { + st, err := f.Lstat() + assertNil(err, "Lstat") + t.CheckEquals(st.Mode()&os.ModeDir != 0, isDir) + } + + t.CreateFileLines("subdir/file") + t.CreateFileLines("file") + + testDir(t.File("subdir"), true) + testDir(t.File("file"), false) +} + +func (s *Suite) Test_Path_Stat(c *check.C) { + t := s.Init(c) + + testDir := func(f Path, isDir bool) { + st, err := f.Stat() + assertNil(err, "Stat") + t.CheckEquals(st.Mode()&os.ModeDir != 0, isDir) + } + + t.CreateFileLines("subdir/file") + t.CreateFileLines("file") + + testDir(t.File("subdir"), true) + testDir(t.File("file"), false) +} + +func (s *Suite) Test_Path_Exists(c *check.C) { + t := s.Init(c) + + test := func(f Path, exists bool) { + t.CheckEquals(f.Exists(), exists) + } + + t.CreateFileLines("subdir/file") + t.CreateFileLines("file") + + test(t.File("subdir"), true) + test(t.File("file"), true) + test(t.File("enoent"), false) +} + +func (s *Suite) Test_Path_IsFile(c *check.C) { + t := s.Init(c) + + t.CreateFileLines("dir/file") + + t.CheckEquals(t.File("nonexistent").IsFile(), false) + t.CheckEquals(t.File("dir").IsFile(), false) + t.CheckEquals(t.File("dir/nonexistent").IsFile(), false) + t.CheckEquals(t.File("dir/file").IsFile(), true) +} + +func (s *Suite) Test_Path_IsDir(c *check.C) { + t := s.Init(c) + + t.CreateFileLines("dir/file") + + t.CheckEquals(t.File("nonexistent").IsDir(), false) + t.CheckEquals(t.File("dir").IsDir(), true) + t.CheckEquals(t.File("dir/nonexistent").IsDir(), false) + t.CheckEquals(t.File("dir/file").IsDir(), false) +} + +func (s *Suite) Test_Path_Chmod(c *check.C) { + t := s.Init(c) + + testWritable := func(f Path, writable bool) { + lstat, err := f.Lstat() + assertNil(err, "Lstat") + t.CheckEquals(lstat.Mode().Perm()&0200 != 0, writable) + } + + f := t.CreateFileLines("file") + testWritable(f, true) + + err := f.Chmod(0444) + assertNil(err, "Chmod") + + testWritable(f, false) +} + +func (s *Suite) Test_Path_ReadDir(c *check.C) { + t := s.Init(c) + + t.CreateFileLines("subdir/file") + t.CreateFileLines("file") + t.CreateFileLines("CVS/Entries") + t.CreateFileLines(".git/info/exclude") + + infos, err := t.File(".").ReadDir() + + assertNil(err, "ReadDir") + var names []string + for _, info := range infos { + names = append(names, info.Name()) + } + + t.CheckDeepEquals(names, []string{".git", "CVS", "file", "subdir"}) +} + +func (s *Suite) Test_Path_Open(c *check.C) { + t := s.Init(c) + + t.CreateFileLines("filename", + "line 1", + "line 2") + + f, err := t.File("filename").Open() + + assertNil(err, "Open") + defer func() { assertNil(f.Close(), "Close") }() + var sb strings.Builder + n, err := io.Copy(&sb, f) + assertNil(err, "Copy") + t.CheckEquals(n, int64(14)) + t.CheckEquals(sb.String(), "line 1\nline 2\n") +} + +func (s *Suite) Test_Path_ReadString(c *check.C) { + t := s.Init(c) + + t.CreateFileLines("filename", + "line 1", + "line 2") + + text, err := t.File("filename").ReadString() + + assertNil(err, "ReadString") + t.CheckEquals(text, "line 1\nline 2\n") +} + +func (s *Suite) Test_Path_WriteString(c *check.C) { + t := s.Init(c) + + err := t.File("filename").WriteString("line 1\nline 2\n") + + assertNil(err, "WriteString") + t.CheckFileLines("filename", + "line 1", + "line 2") +} diff --git a/pkgtools/pkglint/files/pkglint.go b/pkgtools/pkglint/files/pkglint.go index 1b9f0888329..8a4ff984465 100644 --- a/pkgtools/pkglint/files/pkglint.go +++ b/pkgtools/pkglint/files/pkglint.go @@ -10,7 +10,6 @@ import ( "os" "os/user" "path" - "path/filepath" "runtime" "runtime/debug" "runtime/pprof" @@ -26,14 +25,14 @@ type Pkglint struct { Pkgsrc Pkgsrc // Global data, mostly extracted from mk/*. Pkg *Package // The package that is currently checked, or nil. - Todo StringQueue // The files or directories that still need to be checked. - Wip bool // Is the currently checked file or package from pkgsrc-wip? - Infrastructure bool // Is the currently checked file from the pkgsrc infrastructure? - Testing bool // Is pkglint in self-testing mode (only during development)? - Experimental bool // For experimental features, only enabled individually in tests - Username string // For checking against OWNER and MAINTAINER + Todo PathQueue // The files or directories that still need to be checked. + Wip bool // Is the currently checked file or package from pkgsrc-wip? + Infrastructure bool // Is the currently checked file from the pkgsrc infrastructure? + Testing bool // Is pkglint in self-testing mode (only during development)? + Experimental bool // For experimental features, only enabled individually in tests + Username string // For checking against OWNER and MAINTAINER - cvsEntriesDir string // Cached to avoid I/O + cvsEntriesDir Path // Cached to avoid I/O cvsEntries map[string]CvsEntry Logger Logger @@ -43,10 +42,10 @@ type Pkglint struct { fileCache *FileCache interner StringInterner - // cwd is the slash-separated absolute path to the current working + // cwd is the absolute path to the current working // directory. It is used for speeding up relpath and abspath. // There is no other use for it. - cwd string + cwd Path InterPackage InterPackage } @@ -58,7 +57,7 @@ func NewPkglint(stdout io.Writer, stderr io.Writer) Pkglint { p := Pkglint{ res: regex.NewRegistry(), fileCache: NewFileCache(200), - cwd: filepath.ToSlash(cwd), + cwd: NewPathSlash(cwd), interner: NewStringInterner()} p.Logger.out = NewSeparatorWriter(stdout) p.Logger.err = NewSeparatorWriter(stderr) @@ -208,8 +207,8 @@ func (pkglint *Pkglint) setUpProfiling() func() { func (pkglint *Pkglint) prepareMainLoop() { firstDir := pkglint.Todo.Front() - if fileExists(firstDir) { - firstDir = path.Dir(firstDir) + if firstDir.IsFile() { + firstDir = firstDir.Dir() } relTopdir := findPkgsrcTopdir(firstDir) @@ -222,7 +221,7 @@ func (pkglint *Pkglint) prepareMainLoop() { } pkglint.Pkgsrc = NewPkgsrc(joinPath(firstDir, relTopdir)) - pkglint.Wip = matches(pkglint.Pkgsrc.ToRel(firstDir), `^wip(/|$)`) // Same as in Pkglint.Check. + pkglint.Wip = pkglint.Pkgsrc.ToRel(firstDir).HasPrefixPath("wip") // Same as in Pkglint.Check. pkglint.Pkgsrc.LoadInfrastructure() currentUser, err := user.Current() @@ -283,7 +282,7 @@ func (pkglint *Pkglint) ParseCommandLine(args []string) int { } for _, arg := range pkglint.Opts.args { - pkglint.Todo.Push(filepath.ToSlash(arg)) + pkglint.Todo.Push(NewPathSlash(arg)) } if pkglint.Todo.IsEmpty() { pkglint.Todo.Push(".") @@ -299,12 +298,12 @@ func (pkglint *Pkglint) ParseCommandLine(args []string) int { // // It sets up all the global state (infrastructure, wip) for accurately // classifying the entry. -func (pkglint *Pkglint) Check(dirent string) { +func (pkglint *Pkglint) Check(dirent Path) { if trace.Tracing { - defer trace.Call1(dirent)() + defer trace.Call(dirent)() } - st, err := os.Lstat(dirent) + st, err := dirent.Lstat() if err != nil { NewLineWhole(dirent).Errorf("No such file or directory.") return @@ -313,7 +312,7 @@ func (pkglint *Pkglint) Check(dirent string) { pkglint.checkMode(dirent, st.Mode()) } -func (pkglint *Pkglint) checkMode(dirent string, mode os.FileMode) { +func (pkglint *Pkglint) checkMode(dirent Path, mode os.FileMode) { // TODO: merge duplicate code in Package.checkDirent isDir := mode.IsDir() isReg := mode.IsRegular() @@ -324,14 +323,14 @@ func (pkglint *Pkglint) checkMode(dirent string, mode os.FileMode) { dir := dirent if !isDir { - dir = path.Dir(dirent) + dir = dirent.Dir() } - basename := path.Base(dirent) + basename := dirent.Base() pkgsrcRel := pkglint.Pkgsrc.ToRel(dirent) - pkglint.Wip = matches(pkgsrcRel, `^wip(/|$)`) - pkglint.Infrastructure = matches(pkgsrcRel, `^mk(/|$)`) + pkglint.Wip = pkgsrcRel.HasPrefixPath("wip") + pkglint.Infrastructure = pkgsrcRel.HasPrefixPath("mk") pkgsrcdir := findPkgsrcTopdir(dir) if pkgsrcdir == "" { NewLineWhole(dirent).Errorf("Cannot determine the pkgsrc root directory for %q.", cleanpath(dir)) @@ -339,7 +338,7 @@ func (pkglint *Pkglint) checkMode(dirent string, mode os.FileMode) { } if isReg { - depth := strings.Count(pkgsrcRel, "/") + depth := pkgsrcRel.Count() - 1 // FIXME pkglint.checkExecutable(dirent, mode) pkglint.checkReg(dirent, basename, depth) return @@ -363,9 +362,9 @@ func (pkglint *Pkglint) checkMode(dirent string, mode os.FileMode) { // checkdirPackage checks a complete pkgsrc package, including each // of the files individually, and also when seen in combination. -func (pkglint *Pkglint) checkdirPackage(dir string) { +func (pkglint *Pkglint) checkdirPackage(dir Path) { if trace.Tracing { - defer trace.Call1(dir)() + defer trace.Call(dir)() } pkglint.Pkg = NewPackage(dir) @@ -377,9 +376,9 @@ func (pkglint *Pkglint) checkdirPackage(dir string) { } // Returns the pkgsrc top-level directory, relative to the given directory. -func findPkgsrcTopdir(dirname string) string { - for _, dir := range [...]string{".", "..", "../..", "../../.."} { - if fileExists(joinPath(dirname, dir, "mk/bsd.pkg.mk")) { +func findPkgsrcTopdir(dirname Path) Path { + for _, dir := range [...]Path{".", "..", "../..", "../../.."} { + if joinPath(dirname, dir, "mk/bsd.pkg.mk").IsFile() { return dir } } @@ -427,9 +426,9 @@ func resolveVariableRefs(mklines *MkLines, text string) (resolved string) { } } -func CheckFileOther(filename string) { +func CheckFileOther(filename Path) { if trace.Tracing { - defer trace.Call1(filename)() + defer trace.Call(filename)() } if lines := Load(filename, NotEmpty|LogErrors); lines != nil { @@ -439,7 +438,7 @@ func CheckFileOther(filename string) { func CheckLinesDescr(lines *Lines) { if trace.Tracing { - defer trace.Call1(lines.Filename)() + defer trace.Call(lines.Filename)() } for _, line := range lines.Lines { @@ -473,7 +472,7 @@ func CheckLinesDescr(lines *Lines) { func CheckLinesMessage(lines *Lines) { if trace.Tracing { - defer trace.Call1(lines.Filename)() + defer trace.Call(lines.Filename)() } // For now, skip all checks when the MESSAGE may be built from multiple @@ -529,9 +528,9 @@ func CheckLinesMessage(lines *Lines) { SaveAutofixChanges(lines) } -func CheckFileMk(filename string) { +func CheckFileMk(filename Path) { if trace.Tracing { - defer trace.Call1(filename)() + defer trace.Call(filename)() } mklines := LoadMk(filename, NotEmpty|LogErrors) @@ -547,7 +546,7 @@ func CheckFileMk(filename string) { mklines.SaveAutofixChanges() } -func (pkglint *Pkglint) checkReg(filename, basename string, depth int) { +func (pkglint *Pkglint) checkReg(filename Path, basename string, depth int) { if depth == 2 && !pkglint.Wip { if contains(basename, "README") || contains(basename, "TODO") { @@ -606,16 +605,16 @@ func (pkglint *Pkglint) checkReg(filename, basename string, depth int) { CheckLinesPatch(lines) } - case matches(filename, `(?:^|/)patches/manual[^/]*$`): + case matches(filename.String(), `(?:^|/)patches/manual[^/]*$`): if trace.Tracing { - trace.Step1("Unchecked file %q.", filename) + trace.Stepf("Unchecked file %q.", filename) } - case matches(filename, `(?:^|/)patches/[^/]*$`): + case filename.Dir().Base() == "patches": NewLineWhole(filename).Warnf("Patch files should be named \"patch-\", followed by letters, '-', '_', '.', and digits only.") case (hasPrefix(basename, "Makefile") || hasSuffix(basename, ".mk")) && - !pathContainsDir(filename, "files"): + !pathContainsDir(filename, "files"): // FIXME: G.Pkgsrc.Rel(filename) instead of filename CheckFileMk(filename) case hasPrefix(basename, "PLIST"): @@ -627,11 +626,11 @@ func (pkglint *Pkglint) checkReg(filename, basename string, depth int) { // This only checks the file but doesn't register the changes globally. _ = pkglint.Pkgsrc.loadDocChangesFromFile(filename) - case matches(filename, `(?:^|/)files/[^/]*$`): + case filename.Dir().Base() == "files": // Skip files directly in the files/ directory, but not those further down. case basename == "spec": - if !hasPrefix(pkglint.Pkgsrc.ToRel(filename), "regress/") { + if !pkglint.Pkgsrc.ToRel(filename).HasPrefixPath("regress") { NewLineWhole(filename).Warnf("Only packages in regress/ may have spec files.") } @@ -652,7 +651,7 @@ func (pkglint *Pkglint) matchesLicenseFile(basename string) bool { return basename == path.Base(licenseFile) } -func (pkglint *Pkglint) checkExecutable(filename string, mode os.FileMode) { +func (pkglint *Pkglint) checkExecutable(filename Path, mode os.FileMode) { if mode.Perm()&0111 == 0 { // Not executable at all. return @@ -677,7 +676,7 @@ func (pkglint *Pkglint) checkExecutable(filename string, mode os.FileMode) { fix.Custom(func(showAutofix, autofix bool) { fix.Describef(0, "Clearing executable bits") if autofix { - if err := os.Chmod(filename, mode&^0111); err != nil { + if err := filename.Chmod(mode &^ 0111); err != nil { G.Logger.Errorf(cleanpath(filename), "Cannot clear executable bits: %s", err) } } @@ -732,8 +731,8 @@ func (pkglint *Pkglint) tools(mklines *MkLines) *Tools { } } -func (pkglint *Pkglint) loadCvsEntries(filename string) map[string]CvsEntry { - dir := path.Dir(filename) +func (pkglint *Pkglint) loadCvsEntries(filename Path) map[string]CvsEntry { + dir := filename.Dir() if dir == pkglint.cvsEntriesDir { return pkglint.cvsEntries } @@ -798,8 +797,8 @@ func (ip *InterPackage) Enable() { func (ip *InterPackage) Enabled() bool { return ip.hashes != nil } -func (ip *InterPackage) Hash(alg, filename string, hashBytes []byte, loc *Location) *Hash { - key := alg + ":" + filename +func (ip *InterPackage) Hash(alg string, filename Path, hashBytes []byte, loc *Location) *Hash { + key := alg + ":" + filename.String() if otherHash := ip.hashes[key]; otherHash != nil { return otherHash } diff --git a/pkgtools/pkglint/files/pkglint_test.go b/pkgtools/pkglint/files/pkglint_test.go index f07dccadc88..ee2d6a87e48 100644 --- a/pkgtools/pkglint/files/pkglint_test.go +++ b/pkgtools/pkglint/files/pkglint_test.go @@ -14,9 +14,9 @@ func (pkglint *Pkglint) isUsable() bool { return pkglint.fileCache != nil } func (s *Suite) Test_Pkglint_Main(c *check.C) { t := s.Init(c) - out, err := os.Create(t.CreateFileLines("out")) + out, err := os.Create(t.CreateFileLines("out").String()) c.Check(err, check.IsNil) - outProfiling, err := os.Create(t.CreateFileLines("out.profiling")) + outProfiling, err := os.Create(t.CreateFileLines("out.profiling").String()) c.Check(err, check.IsNil) t.SetUpPackage("category/package") @@ -238,7 +238,7 @@ func (s *Suite) Test_Pkglint_Main__complete_package(c *check.C) { "Package version \"1.11\" is greater than the latest \"1.10\" "+ "from ../../doc/CHANGES-2018:5.", "WARN: ~/sysutils/checkperms/Makefile:3: "+ - "This package should be updated to 1.13 ([supports more file formats]).", + "This package should be updated to 1.13 (supports more file formats; see ../../doc/TODO:5).", "ERROR: ~/sysutils/checkperms/Makefile:4: Invalid category \"tools\".", "ERROR: ~/sysutils/checkperms/README: Packages in main pkgsrc must not have a README file.", "ERROR: ~/sysutils/checkperms/TODO: Packages in main pkgsrc must not have a TODO file.", @@ -259,7 +259,7 @@ func (s *Suite) Test_Pkglint_Main__autofix_exitcode(c *check.C) { t.CreateFileLines("filename.mk", "") - exitcode := t.Main("-Wall", "--autofix", t.File("filename.mk")) + exitcode := t.Main("-Wall", "--autofix", "filename.mk") t.CheckOutputLines( "AUTOFIX: ~/filename.mk:1: Inserting a line \"" + MkCvsID + "\" before this line.") @@ -309,7 +309,7 @@ func (s *Suite) Test_Pkglint_Main__profiling(c *check.C) { // Pkglint always writes the profiling data into the current directory. // TODO: Make the location of the profiling log a mandatory parameter. - t.CheckEquals(fileExists("pkglint.pprof"), true) + t.CheckEquals(NewPath("pkglint.pprof").IsFile(), true) err := os.Remove("pkglint.pprof") c.Check(err, check.IsNil) @@ -860,7 +860,7 @@ func (s *Suite) Test_Pkglint_checkReg__alternatives(c *check.C) { lines := t.SetUpFileLines("category/package/ALTERNATIVES", "bin/tar bin/gnu-tar") - t.Main(lines.Filename) + t.Main(lines.Filename.String()) t.CheckOutputLines( "ERROR: ~/category/package/ALTERNATIVES:1: Alternative implementation \"bin/gnu-tar\" must be an absolute path.", @@ -961,7 +961,7 @@ func (s *Suite) Test_Pkglint_checkReg__readme_and_todo(c *check.C) { // Copy category/package/** to wip/package. err := filepath.Walk( - t.File("category/package"), + t.File("category/package").String(), func(pathname string, info os.FileInfo, err error) error { if info.Mode().IsRegular() { src := filepath.ToSlash(pathname) @@ -1047,7 +1047,7 @@ func (s *Suite) Test_Pkglint_checkExecutable(c *check.C) { t := s.Init(c) filename := t.CreateFileLines("file.mk") - err := os.Chmod(filename, 0555) + err := os.Chmod(filename.String(), 0555) assertNil(err, "") G.checkExecutable(filename, 0555) @@ -1065,7 +1065,7 @@ func (s *Suite) Test_Pkglint_checkExecutable(c *check.C) { // On Windows, this is effectively a no-op test since there is no // execute-bit. The only relevant permissions bit is whether a // file is readonly or not. - st, err := os.Lstat(filename) + st, err := filename.Lstat() if t.Check(err, check.IsNil) { t.CheckEquals(st.Mode()&0111, os.FileMode(0)) } diff --git a/pkgtools/pkglint/files/pkgsrc.go b/pkgtools/pkglint/files/pkgsrc.go index 750ca1d917e..3e3dd1e3a5c 100644 --- a/pkgtools/pkglint/files/pkgsrc.go +++ b/pkgtools/pkglint/files/pkgsrc.go @@ -1,7 +1,6 @@ package pkglint import ( - "io/ioutil" "netbsd.org/pkglint/regex" "netbsd.org/pkglint/textproc" "os" @@ -20,7 +19,7 @@ import ( type Pkgsrc struct { // The top directory (PKGSRCDIR), either absolute or relative to // the current working directory. - topdir string + topdir Path // The set of user-defined variables that are added to BUILD_DEFS // within the bsd.pkg.mk file. @@ -36,7 +35,7 @@ type Pkgsrc struct { suggestedUpdates []SuggestedUpdate suggestedWipUpdates []SuggestedUpdate - LastChange map[string]*Change + LastChange map[Path]*Change LastFreezeStart string // e.g. "2018-01-01", or "" LastFreezeEnd string // e.g. "2018-01-01", or "" @@ -53,7 +52,7 @@ type Pkgsrc struct { vartypes VarTypeRegistry } -func NewPkgsrc(dir string) Pkgsrc { +func NewPkgsrc(dir Path) Pkgsrc { return Pkgsrc{ dir, make(map[string]bool), @@ -63,7 +62,7 @@ func NewPkgsrc(dir string) Pkgsrc { make(map[string]string), nil, nil, - make(map[string]*Change), + make(map[Path]*Change), "", "", make(map[string][]string), @@ -149,15 +148,15 @@ func (src *Pkgsrc) loadDocChanges() { NewLineWhole(docDir).Fatalf("Cannot be read for loading the package changes.") } - var filenames []string + var filenames []Path for _, file := range files { filename := file.Name() if matches(filename, `^CHANGES-20\d\d$`) && filename >= "CHANGES-2011" { // TODO: Why 2011? - filenames = append(filenames, filename) + filenames = append(filenames, NewPath(filename)) } } - src.LastChange = make(map[string]*Change) + src.LastChange = make(map[Path]*Change) for _, filename := range filenames { changes := src.loadDocChangesFromFile(joinPath(docDir, filename)) for _, change := range changes { @@ -171,7 +170,7 @@ func (src *Pkgsrc) loadDocChanges() { src.checkRemovedAfterLastFreeze() } -func (src *Pkgsrc) loadDocChangesFromFile(filename string) []*Change { +func (src *Pkgsrc) loadDocChangesFromFile(filename Path) []*Change { warn := G.Opts.CheckGlobal && !G.Wip @@ -179,7 +178,7 @@ func (src *Pkgsrc) loadDocChangesFromFile(filename string) []*Change { // This check has been added in 2018. // For years earlier than 2018 pkglint doesn't care because it's not a big issue anyway. year := "" - if _, yyyy := match1(filename, `-(\d\d\d\d)$`); yyyy >= "2018" { + if _, yyyy := match1(filename.Base(), `-(\d\d\d\d)$`); yyyy >= "2018" { year = yyyy } @@ -313,7 +312,7 @@ func (*Pkgsrc) parseDocChange(line *Line, warn bool) *Change { return &Change{ Location: line.Location, Action: action, - Pkgpath: intern(pkgpath), + Pkgpath: NewPath(intern(pkgpath)), target: intern(condStr(n == 6, f[3], "")), Author: intern(author), Date: intern(date), @@ -332,7 +331,7 @@ func (src *Pkgsrc) checkRemovedAfterLastFreeze() { for pkgpath, change := range src.LastChange { switch change.Action { case Added, Updated, Downgraded: - if !dirExists(src.File(pkgpath)) { + if !src.File(pkgpath).IsDir() { wrong = append(wrong, change) } } @@ -360,32 +359,33 @@ func (src *Pkgsrc) parseSuggestedUpdates(lines *Lines) []SuggestedUpdate { } var updates []SuggestedUpdate - state := 0 - for _, line := range lines.Lines { - text := line.Text - // TODO: Replace this state transition scheme with explicit code, - // hoping that the code will be easier to understand. - if state == 0 && text == "Suggested package updates" { - state = 1 - } else if state == 1 && text == "" { - state = 2 - } else if state == 2 { - state = 3 - } else if state == 3 && text == "" { - state = 4 - } + llex := NewLinesLexer(lines) + for !llex.EOF() && !llex.SkipText("Suggested package updates") { + llex.Skip() + } + for !llex.EOF() && !llex.SkipText("") { + llex.Skip() + } + for llex.SkipText("") { + } - if state == 3 { - if m, pkgname, comment := match2(text, `^\to[\t ]([^\t ]+)(?:[\t ]*(.+))?$`); m { - if m, pkgbase, pkgversion := match2(pkgname, rePkgname); m { - updates = append(updates, SuggestedUpdate{line.Location, intern(pkgbase), intern(pkgversion), intern(comment)}) - } else { - line.Warnf("Invalid package name %q.", pkgname) + for !llex.EOF() && !llex.SkipText("") { + line := llex.CurrentLine() + text := line.Text + llex.Skip() + + if m, pkgname, comment := match2(text, `^\to[\t ]([^\t ]+)(?:[\t ]*(.+))?$`); m { + if m, pkgbase, pkgversion := match2(pkgname, rePkgname); m { + if hasPrefix(comment, "[") && hasSuffix(comment, "]") { + comment = comment[1 : len(comment)-1] } + updates = append(updates, SuggestedUpdate{line.Location, intern(pkgbase), intern(pkgversion), intern(comment)}) } else { - line.Warnf("Invalid line format %q.", text) + line.Warnf("Invalid package name %q.", pkgname) } + } else { + line.Warnf("Invalid line format %q.", text) } } return updates @@ -405,14 +405,14 @@ func (src *Pkgsrc) loadUserDefinedVars() { func (src *Pkgsrc) loadTools() { tools := src.Tools - toolFiles := []string{"defaults.mk"} + toolFiles := []Path{"defaults.mk"} { toc := src.File("mk/tools/bsd.tools.mk") mklines := LoadMk(toc, MustSucceed|NotEmpty) for _, mkline := range mklines.mklines { if mkline.IsInclude() { includedFile := mkline.IncludedFile() - if !contains(includedFile, "/") { + if !includedFile.ContainsText("/") { toolFiles = append(toolFiles, includedFile) } } @@ -436,7 +436,7 @@ func (src *Pkgsrc) loadTools() { }) } - for _, relativeName := range [...]string{"mk/bsd.prefs.mk", "mk/bsd.pkg.mk"} { + for _, relativeName := range [...]Path{"mk/bsd.prefs.mk", "mk/bsd.pkg.mk"} { mklines := src.LoadMk(relativeName, MustSucceed|NotEmpty) mklines.ForEach(func(mkline *MkLine) { @@ -670,7 +670,7 @@ func (src *Pkgsrc) loadUntypedVars() { } } - handleMkFile := func(path string) { + handleMkFile := func(path Path) { mklines := LoadMk(path, MustSucceed) mklines.collectVariables() mklines.collectUsedVariables() @@ -686,12 +686,12 @@ func (src *Pkgsrc) loadUntypedVars() { assertNil(err, "handleFile %q", pathName) baseName := info.Name() if info.Mode().IsRegular() && (hasSuffix(baseName, ".mk") || baseName == "mk.conf") { - handleMkFile(filepath.ToSlash(pathName)) + handleMkFile(NewPath(filepath.ToSlash(pathName))) // FIXME: This is too deep to handle os-specific paths } return nil } - err := filepath.Walk(src.File("mk"), handleFile) + err := filepath.Walk(src.File("mk").String(), handleFile) assertNil(err, "Walk error in pkgsrc infrastructure") } @@ -775,7 +775,7 @@ func (src *Pkgsrc) loadDefaultBuildDefs() { // Example: // Latest("lang", `^php[0-9]+$`, "../../lang/$0") // => "../../lang/php72" -func (src *Pkgsrc) Latest(category string, re regex.Pattern, repl string) string { +func (src *Pkgsrc) Latest(category Path, re regex.Pattern, repl string) string { versions := src.ListVersions(category, re, repl, true) if len(versions) > 0 { @@ -791,7 +791,7 @@ func (src *Pkgsrc) Latest(category string, re regex.Pattern, repl string) string // Example: // ListVersions("lang", `^php[0-9]+$`, "php-$0") // => {"php-53", "php-56", "php-73"} -func (src *Pkgsrc) ListVersions(category string, re regex.Pattern, repl string, errorIfEmpty bool) []string { +func (src *Pkgsrc) ListVersions(category Path, re regex.Pattern, repl string, errorIfEmpty bool) []string { if G.Testing { // Regular expression must be anchored at both ends, to avoid typos. assert(hasPrefix(string(re), "^")) @@ -799,13 +799,11 @@ func (src *Pkgsrc) ListVersions(category string, re regex.Pattern, repl string, } // TODO: Maybe convert cache key to a struct, to save allocations. - cacheKey := category + "/" + string(re) + " => " + repl + cacheKey := category.String() + "/" + string(re) + " => " + repl if latest, found := src.listVersions[cacheKey]; found { return latest } - categoryDir := src.File(category) - var names []string for _, fileInfo := range src.ReadDir(category) { name := fileInfo.Name() @@ -815,7 +813,7 @@ func (src *Pkgsrc) ListVersions(category string, re regex.Pattern, repl string, } if len(names) == 0 { if errorIfEmpty { - dummyLine.Errorf("Cannot find package versions of %q in %q.", re, categoryDir) + dummyLine.Errorf("Cannot find package versions of %q in %q.", re, src.File(category)) } src.listVersions[cacheKey] = nil return nil @@ -973,7 +971,7 @@ func (src *Pkgsrc) checkToplevelUnusedLicenses() { for _, licenseFile := range src.ReadDir("licenses") { licenseName := licenseFile.Name() if !G.InterPackage.IsLicenseUsed(licenseName) { - licensePath := joinPath(licensesDir, licenseName) + licensePath := joinPath(licensesDir, NewPath(licenseName)) NewLineWhole(licensePath).Warnf("This license seems to be unused.") } } @@ -997,9 +995,9 @@ func (src *Pkgsrc) IsBuildDef(varname string) bool { // ReadDir lists the files and subdirectories from the given directory // (relative to the pkgsrc root), filtering out any ignored files (CVS/*) // and empty directories. -func (src *Pkgsrc) ReadDir(dirName string) []os.FileInfo { +func (src *Pkgsrc) ReadDir(dirName Path) []os.FileInfo { dir := src.File(dirName) - files, err := ioutil.ReadDir(dir) + files, err := dir.ReadDir() if err != nil { return nil } @@ -1007,7 +1005,7 @@ func (src *Pkgsrc) ReadDir(dirName string) []os.FileInfo { var relevantFiles []os.FileInfo for _, dirent := range files { name := dirent.Name() - if !dirent.IsDir() || !isIgnoredFilename(name) && !isEmptyDir(joinPath(dir, name)) { + if !dirent.IsDir() || !isIgnoredFilename(name) && !isEmptyDir(dir.JoinNoClean(NewPath(name))) { relevantFiles = append(relevantFiles, dirent) } } @@ -1015,22 +1013,25 @@ func (src *Pkgsrc) ReadDir(dirName string) []os.FileInfo { return relevantFiles } -func (src *Pkgsrc) LoadMkInfra(filename string, options LoadOptions) *MkLines { - if G.Testing { - // During testing, the infrastructure files don't have to exist. - // They are often emulated by setting their data structures manually. - options &^= MustSucceed +// LoadMkExisting loads a file that must exist. +// +// During pkglint testing, these files often don't exist, as they are +// emulated by setting their data structures manually. +func (src *Pkgsrc) LoadMkExisting(filename Path) *MkLines { + options := NotEmpty + if !G.Testing { + options |= MustSucceed } return src.LoadMk(filename, options) } // LoadMk loads the Makefile relative to the pkgsrc top directory. -func (src *Pkgsrc) LoadMk(filename string, options LoadOptions) *MkLines { +func (src *Pkgsrc) LoadMk(filename Path, options LoadOptions) *MkLines { return LoadMk(src.File(filename), options) } // Load loads the file relative to the pkgsrc top directory. -func (src *Pkgsrc) Load(filename string, options LoadOptions) *Lines { +func (src *Pkgsrc) Load(filename Path, options LoadOptions) *Lines { return Load(src.File(filename), options) } @@ -1038,7 +1039,7 @@ func (src *Pkgsrc) Load(filename string, options LoadOptions) *Lines { // // Example: // NewPkgsrc("/usr/pkgsrc").File("distfiles") => "/usr/pkgsrc/distfiles" -func (src *Pkgsrc) File(relativeName string) string { +func (src *Pkgsrc) File(relativeName Path) Path { // TODO: Package.File resolves variables, Pkgsrc.File doesn't. They should behave the same. return cleanpath(joinPath(src.topdir, relativeName)) } @@ -1047,23 +1048,23 @@ func (src *Pkgsrc) File(relativeName string) string { // // Example: // NewPkgsrc("/usr/pkgsrc").ToRel("/usr/pkgsrc/distfiles") => "distfiles" -func (src *Pkgsrc) ToRel(filename string) string { +func (src *Pkgsrc) ToRel(filename Path) Path { return relpath(src.topdir, filename) } // IsInfra returns whether the given filename (relative to the pkglint // working directory) is part of the pkgsrc infrastructure. -func (src *Pkgsrc) IsInfra(filename string) bool { +func (src *Pkgsrc) IsInfra(filename Path) bool { rel := src.ToRel(filename) - return hasPrefix(rel, "mk/") || hasPrefix(rel, "wip/mk/") + return rel.HasPrefixPath("mk") || rel.HasPrefixPath("wip/mk") } // Change describes a modification to a single package, from the doc/CHANGES-* files. type Change struct { Location Location Action ChangeAction // Added, Updated, Downgraded, Renamed, Moved, Removed - Pkgpath string // For renamed or moved packages, the previous PKGPATH - target string + Pkgpath Path // For renamed or moved packages, the previous PKGPATH + target string // The path or version number, depending on the action Author string Date string } @@ -1075,9 +1076,9 @@ func (ch *Change) Version() string { } // Target returns the target PKGPATH for a Renamed or Moved package. -func (ch *Change) Target() string { +func (ch *Change) Target() Path { assert(ch.Action == Renamed || ch.Action == Moved) - return ch.target + return NewPath(ch.target) } // Successor returns the successor for a Removed package. diff --git a/pkgtools/pkglint/files/pkgsrc_test.go b/pkgtools/pkglint/files/pkgsrc_test.go index 7487cdbebee..ed1073ff32c 100644 --- a/pkgtools/pkglint/files/pkgsrc_test.go +++ b/pkgtools/pkglint/files/pkgsrc_test.go @@ -292,7 +292,7 @@ func (s *Suite) Test_Pkgsrc_loadDocChangesFromFile__infrastructure(c *check.C) { "\t\tdistfile directly from GitHub [rillig 2018-01-01]", "\tmk/bsd.pkg.mk: Another infrastructure change [rillig 2018-01-02]") - t.Main(t.File("category/package")) + t.Main("category/package") // For pkglint's purpose, the infrastructure entries are simply ignored // since they do not belong to a single package. @@ -543,6 +543,7 @@ func (s *Suite) Test_Pkgsrc_parseSuggestedUpdates(c *check.C) { t := s.Init(c) lines := t.NewLines("doc/TODO", + CvsID, "", "Suggested package updates", "==============", @@ -556,8 +557,8 @@ func (s *Suite) Test_Pkgsrc_parseSuggestedUpdates(c *check.C) { todo := G.Pkgsrc.parseSuggestedUpdates(lines) t.CheckDeepEquals(todo, []SuggestedUpdate{ - {lines.Lines[5].Location, "CSP", "0.34", ""}, - {lines.Lines[6].Location, "freeciv-client", "2.5.0", "(urgent)"}}) + {lines.Lines[6].Location, "CSP", "0.34", ""}, + {lines.Lines[7].Location, "freeciv-client", "2.5.0", "(urgent)"}}) } func (s *Suite) Test_Pkgsrc_parseSuggestedUpdates__wip(c *check.C) { @@ -577,7 +578,36 @@ func (s *Suite) Test_Pkgsrc_parseSuggestedUpdates__wip(c *check.C) { t.CheckOutputLines( "WARN: ~/wip/package/Makefile:3: " + - "This package should be updated to 1.13 ([cool new features]).") + "This package should be updated to 1.13 (cool new features; see ../../wip/TODO:5).") +} + +func (s *Suite) Test_Pkgsrc_parseSuggestedUpdates__parse_errors(c *check.C) { + t := s.Init(c) + + lines := t.NewLines("doc/TODO", + "", // missing CvsID + "Suggested package updates", + "==============", // usually this line is a bit longer + "", + "", // usually there's only a single empty line + "\t"+"O wrong bullet", + "\t"+"o package-without-version", + "\t"+"o CSP-0.34", + "\t"+"o freeciv-client-2.5.0 (urgent)", // missing [brackets] + "\t"+"o mix-2.5.0 [urgent)", // bracket + parenthesis + "", + "\t"+"o ignored-0.0") + + todo := G.Pkgsrc.parseSuggestedUpdates(lines) + + t.CheckDeepEquals(todo, []SuggestedUpdate{ + {lines.Lines[7].Location, "CSP", "0.34", ""}, + {lines.Lines[8].Location, "freeciv-client", "2.5.0", "(urgent)"}, + {lines.Lines[9].Location, "mix", "2.5.0", "[urgent)"}}) + + t.CheckOutputLines( + "WARN: doc/TODO:6: Invalid line format \"\\tO wrong bullet\".", + "WARN: doc/TODO:7: Invalid package name \"package-without-version\".") } func (s *Suite) Test_Pkgsrc_loadTools(c *check.C) { @@ -1025,12 +1055,12 @@ func (s *Suite) Test_Pkgsrc_VariableType__from_mk(c *check.C) { "PKGSRC_MAKE_ENV?=\t# none", "CPPPATH?=\tcpp", "OSNAME.Linux?=\tLinux") - pkg := t.SetUpPackage("category/package", + t.SetUpPackage("category/package", "PKGSRC_MAKE_ENV+=\tCPP=${CPPPATH:Q}", "PKGSRC_UNKNOWN_ENV+=\tCPP=${ABCPATH:Q}", "OSNAME.SunOS=\t\t${OSNAME.Other}") - t.Main("-Wall", pkg) + t.Main("-Wall", "category/package") if typ := G.Pkgsrc.VariableType(nil, "PKGSRC_MAKE_ENV"); c.Check(typ, check.NotNil) { t.CheckEquals(typ.String(), "ShellWord (list, guessed)") @@ -1110,7 +1140,7 @@ func (s *Suite) Test_Pkgsrc_checkToplevelUnusedLicenses(c *check.C) { t.SetUpPackage("category/package2", "LICENSE=\tmissing") - t.Main("-r", "-Cglobal", t.File(".")) + t.Main("-r", "-Cglobal", ".") t.CheckOutputLines( "WARN: ~/category/package2/Makefile:11: License file ~/licenses/missing does not exist.", @@ -1162,8 +1192,8 @@ func (s *Suite) Test_Change_Target(c *check.C) { moved := Change{loc, Moved, "category/path", "category/other", "author", "2019-01-01"} downgraded := Change{loc, Downgraded, "category/path", "1.0", "author", "2019-01-01"} - t.CheckEquals(renamed.Target(), "category/other") - t.CheckEquals(moved.Target(), "category/other") + t.CheckEquals(renamed.Target(), NewPath("category/other")) + t.CheckEquals(moved.Target(), NewPath("category/other")) t.ExpectAssert(func() { downgraded.Target() }) } diff --git a/pkgtools/pkglint/files/plist.go b/pkgtools/pkglint/files/plist.go index 93b55d7890e..51b5f860804 100644 --- a/pkgtools/pkglint/files/plist.go +++ b/pkgtools/pkglint/files/plist.go @@ -9,7 +9,7 @@ import ( func CheckLinesPlist(pkg *Package, lines *Lines) { if trace.Tracing { - defer trace.Call1(lines.Filename)() + defer trace.Call(lines.Filename)() } idOk := lines.CheckCvsID(0, `@comment `, "@comment ") @@ -31,8 +31,8 @@ func CheckLinesPlist(pkg *Package, lines *Lines) { ck := PlistChecker{ pkg, - make(map[string]*PlistLine), - make(map[string]*PlistLine), + make(map[Path]*PlistLine), + make(map[Path]*PlistLine), "", Once{}, false} @@ -41,8 +41,8 @@ func CheckLinesPlist(pkg *Package, lines *Lines) { type PlistChecker struct { pkg *Package - allFiles map[string]*PlistLine - allDirs map[string]*PlistLine + allFiles map[Path]*PlistLine + allDirs map[Path]*PlistLine lastFname string once Once nonAsciiAllowed bool @@ -53,7 +53,7 @@ func (ck *PlistChecker) Load(lines *Lines) []*PlistLine { ck.collectFilesAndDirs(plines) if lines.BaseName == "PLIST.common_end" { - commonLines := Load(strings.TrimSuffix(lines.Filename, "_end"), NotEmpty) + commonLines := Load(lines.Filename.TrimSuffix("_end"), NotEmpty) if commonLines != nil { ck.collectFilesAndDirs(ck.NewLines(commonLines)) } @@ -107,15 +107,16 @@ func (ck *PlistChecker) collectFilesAndDirs(plines []*PlistLine) { first := text[0] switch { case plistLineStart.Contains(first): - if prev := ck.allFiles[text]; prev == nil || stringSliceLess(pline.conditions, prev.conditions) { - ck.allFiles[text] = pline + path := NewPath(text) + if prev := ck.allFiles[path]; prev == nil || stringSliceLess(pline.conditions, prev.conditions) { + ck.allFiles[path] = pline } - for dir := path.Dir(text); dir != "."; dir = path.Dir(dir) { + for dir := path.Dir(); dir != "."; dir = dir.Dir() { ck.allDirs[dir] = pline } case first == '@': if m, dirname := match1(text, `^@exec \$\{MKDIR\} %D/(.*)$`); m { - for dir := dirname; dir != "."; dir = path.Dir(dir) { + for dir := NewPath(dirname); dir != "."; dir = dir.Dir() { ck.allDirs[dir] = pline } } @@ -259,7 +260,7 @@ func (ck *PlistChecker) checkDuplicate(pline *PlistLine) { return } - prev := ck.allFiles[text] + prev := ck.allFiles[NewPath(text)] if prev == pline || len(prev.conditions) > 0 { return } @@ -317,7 +318,7 @@ func (ck *PlistChecker) checkPathLib(pline *PlistLine, dirname, basename string) if contains(basename, ".a") || contains(basename, ".so") { if m, noext := match1(pline.text, `^(.*)(?:\.a|\.so[0-9.]*)$`); m { - if laLine := ck.allFiles[noext+".la"]; laLine != nil { + if laLine := ck.allFiles[NewPath(noext+".la")]; laLine != nil { pline.Warnf("Redundant library found. The libtool library is in %s.", pline.RefTo(laLine.Line)) } } @@ -350,7 +351,7 @@ func (ck *PlistChecker) checkPathMan(pline *PlistLine) { pline.Warnf("Unknown section %q for manual page.", section) } - if catOrMan == "cat" && ck.allFiles["man/man"+section+"/"+manpage+"."+section] == nil { + if catOrMan == "cat" && ck.allFiles[NewPath("man/man"+section+"/"+manpage+"."+section)] == nil { pline.Warnf("Preformatted manual page without unformatted one.") } diff --git a/pkgtools/pkglint/files/redundantscope.go b/pkgtools/pkglint/files/redundantscope.go index ee272c42e94..49006019e7d 100644 --- a/pkgtools/pkglint/files/redundantscope.go +++ b/pkgtools/pkglint/files/redundantscope.go @@ -223,14 +223,14 @@ func (s *RedundantScope) onOverwrite(overwritten *MkLine, by *MkLine) { // one of two variable assignments is redundant. Two assignments can // only be redundant if one location includes the other. type includePath struct { - files []string + files []Path } -func (p *includePath) push(filename string) { +func (p *includePath) push(filename Path) { p.files = append(p.files, filename) } -func (p *includePath) popUntil(filename string) { +func (p *includePath) popUntil(filename Path) { for p.files[len(p.files)-1] != filename { p.files = p.files[:len(p.files)-1] } @@ -276,5 +276,5 @@ func (p *includePath) equals(other includePath) bool { } func (p *includePath) copy() includePath { - return includePath{append([]string(nil), p.files...)} + return includePath{append([]Path(nil), p.files...)} } diff --git a/pkgtools/pkglint/files/redundantscope_test.go b/pkgtools/pkglint/files/redundantscope_test.go index 00877d58103..f76606deb9c 100644 --- a/pkgtools/pkglint/files/redundantscope_test.go +++ b/pkgtools/pkglint/files/redundantscope_test.go @@ -1525,7 +1525,7 @@ func (s *Suite) Test_RedundantScope_handleVarassign__commented_variable_assignme func (s *Suite) Test_includePath_includes(c *check.C) { t := s.Init(c) - path := func(locations ...string) includePath { + path := func(locations ...Path) includePath { return includePath{locations} } @@ -1550,7 +1550,7 @@ func (s *Suite) Test_includePath_includes(c *check.C) { func (s *Suite) Test_includePath_equals(c *check.C) { t := s.Init(c) - path := func(locations ...string) includePath { + path := func(locations ...Path) includePath { return includePath{locations} } diff --git a/pkgtools/pkglint/files/shell.go b/pkgtools/pkglint/files/shell.go index 739b96e5b6b..fd4f1c39f7a 100644 --- a/pkgtools/pkglint/files/shell.go +++ b/pkgtools/pkglint/files/shell.go @@ -250,7 +250,7 @@ func (scc *SimpleCommandChecker) checkAutoMkdirs() { if m, dirname := match1(arg, `^(?:\$\{DESTDIR\})?\$\{PREFIX(?:|:Q)\}/(.*)`); m { autoMkdirs := false if G.Pkg != nil { - plistLine := G.Pkg.Plist.Dirs[dirname] + plistLine := G.Pkg.Plist.Dirs[NewPath(dirname)] if plistLine != nil && !containsVarRef(plistLine.Text) { autoMkdirs = true } @@ -760,7 +760,7 @@ func (ck *ShellLineChecker) CheckWord(token string, checkQuoting bool, time Tool } func (ck *ShellLineChecker) checkWordQuoting(token string, checkQuoting bool, time ToolTime) { - tok := NewShTokenizer(ck.mkline.Line, token, false) + tok := NewShTokenizer(ck.mkline.Line, token, true) atoms := tok.ShAtoms() quoting := shqPlain diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go index 8a2bbb01ac8..36dc8a06534 100644 --- a/pkgtools/pkglint/files/shell_test.go +++ b/pkgtools/pkglint/files/shell_test.go @@ -912,7 +912,10 @@ func (s *Suite) Test_ShellLineChecker_CheckShellCommandLine__shell_variables(c * "WARN: Makefile:3: $f is ambiguous. Use ${f} if you mean a Make variable or $$f if you mean a shell variable.", "WARN: Makefile:3: $f is ambiguous. Use ${f} if you mean a Make variable or $$f if you mean a shell variable.", "NOTE: Makefile:3: Please use the SUBST framework instead of ${SED} and ${MV}.", - "WARN: Makefile:3: f is used but not defined.") + "WARN: Makefile:3: f is used but not defined.", + // TODO: Avoid these duplicate diagnostics. + "WARN: Makefile:3: $f is ambiguous. Use ${f} if you mean a Make variable or $$f if you mean a shell variable.", + "WARN: Makefile:3: $f is ambiguous. Use ${f} if you mean a Make variable or $$f if you mean a shell variable.") ck.CheckShellCommandLine("install -c manpage.1 ${PREFIX}/man/man1/manpage.1") diff --git a/pkgtools/pkglint/files/shtokenizer.go b/pkgtools/pkglint/files/shtokenizer.go index 0a50521a600..531cfccb16c 100644 --- a/pkgtools/pkglint/files/shtokenizer.go +++ b/pkgtools/pkglint/files/shtokenizer.go @@ -8,6 +8,7 @@ type ShTokenizer struct { func NewShTokenizer(line *Line, text string, emitWarnings bool) *ShTokenizer { // TODO: Switching to NewMkParser is nontrivial since emitWarnings must equal (line != nil). + // assert((line != nil) == emitWarnings) p := MkParser{line, textproc.NewLexer(text), emitWarnings} return &ShTokenizer{&p} } diff --git a/pkgtools/pkglint/files/testnames_test.go b/pkgtools/pkglint/files/testnames_test.go index aafc0a6154f..0354d53ab0f 100644 --- a/pkgtools/pkglint/files/testnames_test.go +++ b/pkgtools/pkglint/files/testnames_test.go @@ -11,6 +11,7 @@ import ( func (s *Suite) Test__test_names(c *check.C) { ck := intqa.NewTestNameChecker(c.Errorf) ck.Configure("*", "*", "*", -intqa.EMissingTest) + ck.Configure("path.go", "*", "*", +intqa.EMissingTest) ck.Configure("*yacc.go", "*", "*", intqa.ENone) ck.Check() } diff --git a/pkgtools/pkglint/files/textproc/lexer.go b/pkgtools/pkglint/files/textproc/lexer.go index 8df3d9eb400..54359beba7c 100644 --- a/pkgtools/pkglint/files/textproc/lexer.go +++ b/pkgtools/pkglint/files/textproc/lexer.go @@ -75,7 +75,7 @@ func (l *Lexer) NextString(prefix string) string { return "" } -// SkipString skips over the given string, if the remaining string starts +// SkipText skips over the given string, if the remaining string starts // with it. It returns whether it actually skipped. func (l *Lexer) SkipString(prefix string) bool { skipped := strings.HasPrefix(l.rest, prefix) diff --git a/pkgtools/pkglint/files/tools.go b/pkgtools/pkglint/files/tools.go index 6eb01c21bbc..f7423c23222 100644 --- a/pkgtools/pkglint/files/tools.go +++ b/pkgtools/pkglint/files/tools.go @@ -345,7 +345,7 @@ func (tr *Tools) implicitTools(toolName string) []string { func (tr *Tools) validity(basename string, useTools bool) Validity { switch { - case IsPrefs(basename): // IsPrefs is not 100% accurate here but good enough + case IsPrefs(NewPath(basename)): // IsPrefs is not 100% accurate here but good enough return AfterPrefsMk case basename == "Makefile" && !tr.SeenPrefs: return AfterPrefsMk diff --git a/pkgtools/pkglint/files/tools_test.go b/pkgtools/pkglint/files/tools_test.go index dea93d3afbc..b02ab709808 100644 --- a/pkgtools/pkglint/files/tools_test.go +++ b/pkgtools/pkglint/files/tools_test.go @@ -46,7 +46,7 @@ func (s *Suite) Test_Tools__USE_TOOLS_predefined_sed(c *check.C) { "\t${SED} < input > output", "\t${AWK} < input > output") - t.Main("-Wall", t.File("module.mk")) + t.Main("-Wall", "module.mk") // Since this test doesn't load the usual tool definitions via // G.Pkgsrc.loadTools, AWK is not known at all. diff --git a/pkgtools/pkglint/files/toplevel.go b/pkgtools/pkglint/files/toplevel.go index a11d1dff856..7937e23653f 100644 --- a/pkgtools/pkglint/files/toplevel.go +++ b/pkgtools/pkglint/files/toplevel.go @@ -1,14 +1,14 @@ package pkglint type Toplevel struct { - dir string - previousSubdir string - subdirs []string + dir Path + previousSubdir Path + subdirs []Path } -func CheckdirToplevel(dir string) { +func CheckdirToplevel(dir Path) { if trace.Tracing { - defer trace.Call1(dir)() + defer trace.Call(dir)() } ctx := Toplevel{dir, "", nil} @@ -36,7 +36,7 @@ func CheckdirToplevel(dir string) { } func (ctx *Toplevel) checkSubdir(mkline *MkLine) { - subdir := mkline.Value() + subdir := NewPath(mkline.Value()) if mkline.IsCommentedVarassign() { if !mkline.HasComment() || mkline.Comment() == "" { @@ -44,7 +44,7 @@ func (ctx *Toplevel) checkSubdir(mkline *MkLine) { } } - if containsVarRef(subdir) || !fileExists(joinPath(ctx.dir, subdir, "Makefile")) { + if containsVarRef(subdir.String()) || !ctx.dir.JoinNoClean(subdir).JoinNoClean("Makefile").IsFile() { return } diff --git a/pkgtools/pkglint/files/trace/tracing.go b/pkgtools/pkglint/files/trace/tracing.go index 926344cbdf5..2c6a29a4d16 100644 --- a/pkgtools/pkglint/files/trace/tracing.go +++ b/pkgtools/pkglint/files/trace/tracing.go @@ -92,7 +92,9 @@ func argsStr(args []interface{}) string { if rv.Len() > 0 { rv.WriteString(", ") } - if str, ok := arg.(fmt.Stringer); ok && !isNil(str) { + if str, ok := arg.(fmt.GoStringer); ok && !isNil(str) { + rv.WriteString(str.GoString()) + } else if str, ok := arg.(fmt.Stringer); ok && !isNil(str) { rv.WriteString(str.String()) } else { _, _ = fmt.Fprintf(&rv, "%#v", arg) diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go index 0c832a21679..b9648c6580d 100644 --- a/pkgtools/pkglint/files/util.go +++ b/pkgtools/pkglint/files/util.go @@ -3,12 +3,9 @@ package pkglint import ( "fmt" "hash/crc64" - "io/ioutil" "netbsd.org/pkglint/regex" "netbsd.org/pkglint/textproc" - "os" "path" - "path/filepath" "reflect" "regexp" "sort" @@ -30,6 +27,7 @@ func (ynu YesNoUnknown) String() string { } // Short names for commonly used functions. + func contains(s, substr string) bool { return strings.Contains(s, substr) } @@ -240,12 +238,12 @@ func assertf(cond bool, format string, args ...interface{}) { } } -func isEmptyDir(filename string) bool { - if hasSuffix(filename, "/CVS") { +func isEmptyDir(filename Path) bool { + if filename.HasSuffixText("/CVS") { return true } - dirents, err := ioutil.ReadDir(filename) + dirents, err := filename.ReadDir() if err != nil { return true // XXX: Why not false? } @@ -255,7 +253,7 @@ func isEmptyDir(filename string) bool { if isIgnoredFilename(name) { continue } - if dirent.IsDir() && isEmptyDir(joinPath(filename, name)) { + if dirent.IsDir() && isEmptyDir(filename.JoinNoClean(NewPath(name))) { continue } return false @@ -263,17 +261,17 @@ func isEmptyDir(filename string) bool { return true } -func getSubdirs(filename string) []string { - dirents, err := ioutil.ReadDir(filename) +func getSubdirs(filename Path) []Path { + dirents, err := filename.ReadDir() if err != nil { NewLineWhole(filename).Fatalf("Cannot be read: %s", err) } - var subdirs []string + var subdirs []Path for _, dirent := range dirents { name := dirent.Name() - if dirent.IsDir() && !isIgnoredFilename(name) && !isEmptyDir(joinPath(filename, name)) { - subdirs = append(subdirs, name) + if dirent.IsDir() && !isIgnoredFilename(name) && !isEmptyDir(filename.JoinNoClean(NewPath(name))) { + subdirs = append(subdirs, NewPath(name)) } } return subdirs @@ -287,24 +285,24 @@ func isIgnoredFilename(filename string) bool { return hasPrefix(filename, ".#") } -func dirglob(dirname string) []string { - infos, err := ioutil.ReadDir(dirname) +func dirglob(dirname Path) []Path { + infos, err := dirname.ReadDir() if err != nil { return nil } - var filenames []string + var filenames []Path for _, info := range infos { if !(isIgnoredFilename(info.Name())) { - filenames = append(filenames, cleanpath(joinPath(dirname, info.Name()))) + filenames = append(filenames, cleanpath(dirname.JoinNoClean(NewPath(info.Name())))) } } return filenames } // Checks whether a file is already committed to the CVS repository. -func isCommitted(filename string) bool { +func isCommitted(filename Path) bool { entries := G.loadCvsEntries(filename) - _, found := entries[path.Base(filename)] + _, found := entries[filename.Base()] return found } @@ -313,14 +311,14 @@ func isCommitted(filename string) bool { // // There is no corresponding test for Git (as used by pkgsrc-wip) since that // is more difficult to implement than simply reading a CVS/Entries file. -func isLocallyModified(filename string) bool { +func isLocallyModified(filename Path) bool { entries := G.loadCvsEntries(filename) - entry, found := entries[path.Base(filename)] + entry, found := entries[filename.Base()] if !found { return false } - st, err := os.Stat(filename) + st, err := filename.Stat() if err != nil { return true } @@ -438,16 +436,6 @@ func varnameParam(varname string) string { return "" } -func fileExists(filename string) bool { - st, err := os.Stat(filename) - return err == nil && st.Mode().IsRegular() -} - -func dirExists(filename string) bool { - st, err := os.Stat(filename) - return err == nil && st.Mode().IsDir() -} - func toInt(s string, def int) int { if n, err := strconv.Atoi(s); err == nil { return n @@ -469,11 +457,15 @@ func mkopSubst(s string, left bool, from string, right bool, to string, flags st }) } -func joinPath(a, b string, others ...string) string { +func joinPath(a, b Path, others ...Path) Path { if len(others) == 0 { return a + "/" + b } - return a + "/" + b + "/" + strings.Join(others, "/") + parts := []string{a.String(), b.String()} + for _, part := range others { + parts = append(parts, part.String()) + } + return NewPath(strings.Join(parts, "/")) } // relpath returns the relative path from the directory "from" @@ -493,7 +485,7 @@ func joinPath(a, b string, others ...string) string { // // TODO: Invent data types for all kinds of relative paths that occur in pkgsrc // and pkglint. Make sure that these paths cannot be accidentally mixed. -func relpath(from, to string) (result string) { +func relpath(from, to Path) (result Path) { if trace.Tracing { defer trace.Call(from, to, trace.Result(&result))() @@ -507,34 +499,31 @@ func relpath(from, to string) (result string) { } // Take a shortcut for the common case from "dir" to "dir/subdir/...". - if hasPrefix(cto, cfrom) && hasPrefix(cto[len(cfrom):], "/") { + if cto.HasPrefixPath(cfrom) { return cleanpath(cto[len(cfrom)+1:]) } // Take a shortcut for the common case from "category/package" to ".". // This is the most common variant in a complete pkgsrc scan. if cto == "." { - fromParts := strings.FieldsFunc(cfrom, func(r rune) bool { return r == '/' }) + fromParts := cfrom.Parts() if len(fromParts) == 2 && !hasPrefix(fromParts[0], ".") && !hasPrefix(fromParts[1], ".") { return "../.." } } - if cfrom == "." && !filepath.IsAbs(cto) { - return path.Clean(cto) + if cfrom == "." && !cto.IsAbs() { + return cto.Clean() } absFrom := abspath(cfrom) absTopdir := abspath(G.Pkgsrc.topdir) absTo := abspath(cto) - toTop, err := filepath.Rel(absFrom, absTopdir) - assertNil(err, "relpath from %q to topdir %q", absFrom, absTopdir) - - fromTop, err := filepath.Rel(absTopdir, absTo) - assertNil(err, "relpath from topdir %q to %q", absTopdir, absTo) + toTop := absFrom.Rel(absTopdir) + fromTop := absTopdir.Rel(absTo) - result = cleanpath(joinPath(filepath.ToSlash(toTop), filepath.ToSlash(fromTop))) + result = cleanpath(toTop.JoinNoClean(fromTop)) if trace.Tracing { trace.Stepf("relpath from %q to %q = %q", cfrom, cto, result) @@ -542,20 +531,20 @@ func relpath(from, to string) (result string) { return } -func abspath(filename string) string { +func abspath(filename Path) Path { abs := filename - if !filepath.IsAbs(filename) { + if !filename.IsAbs() { abs = joinPath(G.cwd, abs) } - return path.Clean(abs) + return abs.Clean() } // Differs from path.Clean in that only "../../" is replaced, not "../". // Also, the initial directory is always kept. // This is to provide the package path as context in recursive invocations of pkglint. -func cleanpath(filename string) string { +func cleanpath(filename Path) Path { parts := make([]string, 0, 5) - lex := textproc.NewLexer(filename) + lex := textproc.NewLexer(filename.String()) for lex.SkipString("./") { } @@ -585,7 +574,7 @@ func cleanpath(filename string) string { if len(parts) == 0 { return "." } - return strings.Join(parts, "/") + return NewPath(strings.Join(parts, "/")) } func pathContains(haystack, needle string) bool { @@ -602,10 +591,10 @@ func pathContains(haystack, needle string) bool { return false } -func pathContainsDir(haystack, needle string) bool { +func pathContainsDir(haystack, needle Path) bool { n0 := needle[0] for i := 0; i < 1+len(haystack)-len(needle); i++ { - if haystack[i] == n0 && hasPrefix(haystack[i:], needle) { + if haystack[i] == n0 && hasPrefix(haystack.String()[i:], needle.String()) { if i == 0 || haystack[i-1] == '/' { if i+len(needle) < len(haystack) && haystack[i+len(needle)] == '/' { return true @@ -1010,8 +999,8 @@ func naturalLess(str1, str2 string) bool { // IsPrefs returns whether the given file, when included, loads the user // preferences. -func IsPrefs(filename string) bool { - switch path.Base(filename) { +func IsPrefs(filename Path) bool { + switch filename.Base() { case // See https://github.com/golang/go/issues/28057 "bsd.prefs.mk", // in mk/ "bsd.fast.prefs.mk", // in mk/ @@ -1047,7 +1036,7 @@ func NewFileCache(size int) *FileCache { 0} } -func (c *FileCache) Put(filename string, options LoadOptions, lines *Lines) { +func (c *FileCache) Put(filename Path, options LoadOptions, lines *Lines) { key := c.key(filename) entry := c.mapping[key] @@ -1101,7 +1090,7 @@ func (c *FileCache) removeOldEntries() { } } -func (c *FileCache) Get(filename string, options LoadOptions) *Lines { +func (c *FileCache) Get(filename Path, options LoadOptions) *Lines { key := c.key(filename) entry, found := c.mapping[key] if found && entry.options == options { @@ -1118,7 +1107,7 @@ func (c *FileCache) Get(filename string, options LoadOptions) *Lines { return nil } -func (c *FileCache) Evict(filename string) { +func (c *FileCache) Evict(filename Path) { key := c.key(filename) entry, found := c.mapping[key] if !found { @@ -1136,9 +1125,7 @@ func (c *FileCache) Evict(filename string) { } } -func (c *FileCache) key(filename string) string { - return path.Clean(filename) -} +func (c *FileCache) key(filename Path) string { return filename.Clean().String() } func bmakeHelp(topic string) string { return bmake("help topic=" + topic) } @@ -1398,27 +1385,27 @@ func pathMatches(pattern, s string) bool { return err == nil && matched } -type StringQueue struct { - entries []string +type PathQueue struct { + entries []Path } -func (q *StringQueue) PushFront(entries ...string) { - q.entries = append(append([]string(nil), entries...), q.entries...) +func (q *PathQueue) PushFront(entries ...Path) { + q.entries = append(append([]Path(nil), entries...), q.entries...) } -func (q *StringQueue) Push(entries ...string) { +func (q *PathQueue) Push(entries ...Path) { q.entries = append(q.entries, entries...) } -func (q *StringQueue) IsEmpty() bool { +func (q *PathQueue) IsEmpty() bool { return len(q.entries) == 0 } -func (q *StringQueue) Front() string { +func (q *PathQueue) Front() Path { return q.entries[0] } -func (q *StringQueue) Pop() string { +func (q *PathQueue) Pop() Path { front := q.entries[0] q.entries = q.entries[1:] return front diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go index 95f6c5f6795..391583a8e35 100644 --- a/pkgtools/pkglint/files/util_test.go +++ b/pkgtools/pkglint/files/util_test.go @@ -99,12 +99,12 @@ func (s *Suite) Test_isEmptyDir__and_getSubdirs(c *check.C) { if dir := t.File("."); true { t.CheckEquals(isEmptyDir(dir), true) - t.CheckDeepEquals(getSubdirs(dir), []string(nil)) + t.CheckDeepEquals(getSubdirs(dir), []Path(nil)) t.CreateFileLines("somedir/file") t.CheckEquals(isEmptyDir(dir), false) - t.CheckDeepEquals(getSubdirs(dir), []string{"somedir"}) + t.CheckDeepEquals(getSubdirs(dir), []Path{"somedir"}) } if absent := t.File("nonexistent"); true { @@ -122,9 +122,9 @@ func (s *Suite) Test_getSubdirs(c *check.C) { t.CreateFileLines("subdir/file") t.CreateFileLines("empty/file") - c.Check(os.Remove(t.File("empty/file")), check.IsNil) + c.Check(os.Remove(t.File("empty/file").String()), check.IsNil) - t.CheckDeepEquals(getSubdirs(t.File(".")), []string{"subdir"}) + t.CheckDeepEquals(getSubdirs(t.File(".")), []Path{"subdir"}) } func (s *Suite) Test_isLocallyModified(c *check.C) { @@ -133,10 +133,10 @@ func (s *Suite) Test_isLocallyModified(c *check.C) { unmodified := t.CreateFileLines("unmodified") modTime := time.Unix(1136239445, 0).UTC() - err := os.Chtimes(unmodified, modTime, modTime) + err := os.Chtimes(unmodified.String(), modTime, modTime) c.Check(err, check.IsNil) - st, err := os.Lstat(unmodified) + st, err := os.Lstat(unmodified.String()) c.Check(err, check.IsNil) // Make sure that the file system has second precision and accuracy. @@ -284,28 +284,6 @@ func (s *Suite) Test_varnameParam(c *check.C) { t.CheckEquals(varnameParam(".CURDIR"), "") } -func (s *Suite) Test_fileExists(c *check.C) { - t := s.Init(c) - - t.CreateFileLines("dir/file") - - t.CheckEquals(fileExists(t.File("nonexistent")), false) - t.CheckEquals(fileExists(t.File("dir")), false) - t.CheckEquals(fileExists(t.File("dir/nonexistent")), false) - t.CheckEquals(fileExists(t.File("dir/file")), true) -} - -func (s *Suite) Test_dirExists(c *check.C) { - t := s.Init(c) - - t.CreateFileLines("dir/file") - - t.CheckEquals(dirExists(t.File("nonexistent")), false) - t.CheckEquals(dirExists(t.File("dir")), true) - t.CheckEquals(dirExists(t.File("dir/nonexistent")), false) - t.CheckEquals(dirExists(t.File("dir/file")), false) -} - func (s *Suite) Test_mkopSubst__middle(c *check.C) { t := s.Init(c) @@ -360,7 +338,7 @@ func (s *Suite) Test_relpath(c *check.C) { t.Chdir(".") t.CheckEquals(G.Pkgsrc.topdir, t.tmpdir) - test := func(from, to, result string) { + test := func(from, to Path, result Path) { t.CheckEquals(relpath(from, to), result) } @@ -396,7 +374,7 @@ func (s *Suite) Test_relpath(c *check.C) { func (s *Suite) Test_relpath__quick(c *check.C) { t := s.Init(c) - test := func(from, to, result string) { + test := func(from, to Path, result Path) { t.CheckEquals(relpath(from, to), result) } @@ -411,7 +389,7 @@ func (s *Suite) Test_relpath__quick(c *check.C) { func (s *Suite) Test_cleanpath(c *check.C) { t := s.Init(c) - test := func(from, to string) { + test := func(from, to Path) { t.CheckEquals(cleanpath(from), to) } @@ -498,12 +476,12 @@ func (s *Suite) Test_pathContains(c *check.C) { func (s *Suite) Test_pathContainsDir(c *check.C) { t := s.Init(c) - test := func(haystack, needle string, expected bool) { + test := func(haystack, needle Path, expected bool) { actual := pathContainsDir(haystack, needle) t.CheckEquals(actual, expected) } - testPanic := func(haystack, needle string) { + testPanic := func(haystack, needle Path) { t.c.Check( func() { _ = pathContainsDir(haystack, needle) }, check.PanicMatches, @@ -850,15 +828,15 @@ func (s *Suite) Test_FileCache(c *check.C) { c.Check(cache.Get("Makefile", MustSucceed|LogErrors), check.IsNil) // Wrong LoadOptions. linesFromCache := cache.Get("Makefile", 0) - t.CheckEquals(linesFromCache.Filename, "Makefile") + t.CheckEquals(linesFromCache.Filename, NewPath("Makefile")) c.Check(linesFromCache.Lines, check.HasLen, 2) - t.CheckEquals(linesFromCache.Lines[0].Filename, "Makefile") + t.CheckEquals(linesFromCache.Lines[0].Filename, NewPath("Makefile")) // Cache keys are normalized using path.Clean. linesFromCache2 := cache.Get("./Makefile", 0) - t.CheckEquals(linesFromCache2.Filename, "./Makefile") + t.CheckEquals(linesFromCache2.Filename, NewPath("./Makefile")) c.Check(linesFromCache2.Lines, check.HasLen, 2) - t.CheckEquals(linesFromCache2.Lines[0].Filename, "./Makefile") + t.CheckEquals(linesFromCache2.Lines[0].Filename, NewPath("./Makefile")) cache.Put("file1.mk", 0, lines) cache.Put("file2.mk", 0, lines) diff --git a/pkgtools/pkglint/files/vardefs.go b/pkgtools/pkglint/files/vardefs.go index cf312c606c2..11344c06a3c 100644 --- a/pkgtools/pkglint/files/vardefs.go +++ b/pkgtools/pkglint/files/vardefs.go @@ -2,7 +2,6 @@ package pkglint import ( "netbsd.org/pkglint/regex" - "path" "strings" ) @@ -331,7 +330,7 @@ func (reg *VarTypeRegistry) infralist(varname string, basicType *BasicType) { // compilerLanguages reads the available languages that are typically // bundled in a single compiler framework, such as GCC or Clang. func (reg *VarTypeRegistry) compilerLanguages(src *Pkgsrc) *BasicType { - mklines := src.LoadMkInfra("mk/compiler.mk", NotEmpty|MustSucceed) + mklines := src.LoadMkExisting("mk/compiler.mk") languages := make(map[string]bool) if mklines != nil { @@ -371,8 +370,8 @@ func (reg *VarTypeRegistry) compilerLanguages(src *Pkgsrc) *BasicType { // // If the file cannot be found, the allowed values are taken from // defval. This is mostly useful when testing pkglint. -func (reg *VarTypeRegistry) enumFrom(pkgsrc *Pkgsrc, filename string, defval string, varcanons ...string) *BasicType { - mklines := LoadMk(pkgsrc.File(filename), NotEmpty) +func (reg *VarTypeRegistry) enumFrom(pkgsrc *Pkgsrc, filename Path, defval string, varcanons ...string) *BasicType { + mklines := pkgsrc.LoadMkExisting(filename) if mklines == nil { return enum(defval) } @@ -419,7 +418,7 @@ func (reg *VarTypeRegistry) enumFrom(pkgsrc *Pkgsrc, filename string, defval str // // If the directories cannot be found, the allowed values are taken // from defval. This is mostly useful when testing pkglint. -func (reg *VarTypeRegistry) enumFromDirs(pkgsrc *Pkgsrc, category string, re regex.Pattern, repl string, defval string) *BasicType { +func (reg *VarTypeRegistry) enumFromDirs(pkgsrc *Pkgsrc, category Path, re regex.Pattern, repl string, defval string) *BasicType { versions := pkgsrc.ListVersions(category, re, repl, false) if len(versions) == 0 { return enum(defval) @@ -432,10 +431,10 @@ func (reg *VarTypeRegistry) enumFromDirs(pkgsrc *Pkgsrc, category string, re reg // // If no files are found, the allowed values are taken // from defval. This should only happen in the pkglint tests. -func (reg *VarTypeRegistry) enumFromFiles(basedir string, re regex.Pattern, repl string, defval string) *BasicType { +func (reg *VarTypeRegistry) enumFromFiles(basedir Path, re regex.Pattern, repl string, defval string) *BasicType { var relevant []string for _, filename := range dirglob(G.Pkgsrc.File(basedir)) { - basename := path.Base(filename) + basename := filename.Base() if matches(basename, re) { relevant = append(relevant, replaceAll(basename, re, repl)) } @@ -1249,8 +1248,8 @@ func (reg *VarTypeRegistry) Init(src *Pkgsrc) { reg.sys("MANOWN", BtUserGroupName) reg.pkglist("MASTER_SITES", BtFetchURL) - for _, filename := range []string{"mk/fetch/sites.mk", "mk/fetch/fetch.mk"} { - sitesMk := src.LoadMkInfra(filename, NotEmpty|MustSucceed) + for _, filename := range []Path{"mk/fetch/sites.mk", "mk/fetch/fetch.mk"} { + sitesMk := src.LoadMkExisting(filename) if sitesMk != nil { sitesMk.ForEach(func(mkline *MkLine) { if mkline.IsVarassign() && hasPrefix(mkline.Varname(), "MASTER_SITE_") { diff --git a/pkgtools/pkglint/files/vardefs_test.go b/pkgtools/pkglint/files/vardefs_test.go index 7d523959985..06a342ff46c 100644 --- a/pkgtools/pkglint/files/vardefs_test.go +++ b/pkgtools/pkglint/files/vardefs_test.go @@ -178,7 +178,7 @@ func (s *Suite) Test_VarTypeRegistry_Init__no_testing(c *check.C) { G.Testing = false t.ExpectFatal( t.FinishSetUp, - "FATAL: ~/mk/fetch/sites.mk: Cannot be read.") + "FATAL: ~/editors/emacs/modules.mk: Cannot be read.") } func (s *Suite) Test_VarTypeRegistry_Init__MASTER_SITES(c *check.C) { diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go index 47a637cf080..0233555cae7 100644 --- a/pkgtools/pkglint/files/vartypecheck.go +++ b/pkgtools/pkglint/files/vartypecheck.go @@ -225,7 +225,7 @@ func (cv *VartypeCheck) BuildlinkDepmethod() { } func (cv *VartypeCheck) Category() { - if cv.Value != "wip" && fileExists(G.Pkgsrc.File(cv.Value+"/Makefile")) { + if cv.Value != "wip" && G.Pkgsrc.File(NewPath(cv.Value).JoinNoClean("Makefile")).IsFile() { return } @@ -452,7 +452,7 @@ func (cv *VartypeCheck) DependencyWithPath() { } if !containsVarRef(relpath) { - MkLineChecker{cv.MkLines, cv.MkLine}.CheckRelativePkgdir(relpath) + MkLineChecker{cv.MkLines, cv.MkLine}.CheckRelativePkgdir(NewPath(relpath)) } switch pkg { @@ -1073,22 +1073,23 @@ func (cv *VartypeCheck) PkgOptionsVar() { func (cv *VartypeCheck) Pkgpath() { cv.Pathname() - pkgpath := cv.Value - if pkgpath != cv.ValueNoVar || cv.Op == opUseMatch { + value := cv.Value + if value != cv.ValueNoVar || cv.Op == opUseMatch { return } - if !G.Wip && hasPrefix(pkgpath, "wip/") { + pkgpath := NewPath(value) + if !G.Wip && pkgpath.HasPrefixPath("wip") { cv.MkLine.Errorf("A main pkgsrc package must not depend on a pkgsrc-wip package.") } - if !fileExists(G.Pkgsrc.File(joinPath(pkgpath, "Makefile"))) { + if !G.Pkgsrc.File(pkgpath.JoinNoClean("Makefile")).IsFile() { cv.MkLine.Errorf("There is no package in %q.", - relpath(path.Dir(cv.MkLine.Filename), G.Pkgsrc.File(pkgpath))) + cv.MkLine.PathToFile(G.Pkgsrc.File(pkgpath))) return } - if !matches(pkgpath, `^([^./][^/]*/[^./][^/]*)$`) { + if !matches(value, `^([^./][^/]*/[^./][^/]*)$`) { cv.MkLine.Errorf("%q is not a valid path to a package.", pkgpath) cv.MkLine.Explain( "A path to a package has the form \"category/pkgbase\".", @@ -1164,7 +1165,7 @@ func (cv *VartypeCheck) RPkgVer() { // RelativePkgDir refers to a package directory, e.g. ../../category/pkgbase. func (cv *VartypeCheck) RelativePkgDir() { - MkLineChecker{cv.MkLines, cv.MkLine}.CheckRelativePkgdir(cv.Value) + MkLineChecker{cv.MkLines, cv.MkLine}.CheckRelativePkgdir(NewPath(cv.Value)) } // RelativePkgPath refers to a file or directory, e.g. ../../category/pkgbase, @@ -1172,7 +1173,7 @@ func (cv *VartypeCheck) RelativePkgDir() { // // See RelativePkgDir, which requires a directory, not a file. func (cv *VartypeCheck) RelativePkgPath() { - MkLineChecker{cv.MkLines, cv.MkLine}.CheckRelativePath(cv.Value, true) + MkLineChecker{cv.MkLines, cv.MkLine}.CheckRelativePath(NewPath(cv.Value), true) } func (cv *VartypeCheck) Restricted() { diff --git a/pkgtools/pkglint/files/vartypecheck_test.go b/pkgtools/pkglint/files/vartypecheck_test.go index 92b6e304a2c..64b1da229e8 100644 --- a/pkgtools/pkglint/files/vartypecheck_test.go +++ b/pkgtools/pkglint/files/vartypecheck_test.go @@ -498,7 +498,7 @@ func (s *Suite) Test_VartypeCheck_Enum__use_match(c *check.C) { "\tlike ^, *, $. In such a case, using the :M or :N modifiers is useful", "\tand preferred.", "", - "WARN: module.mk:5: Use ${PKGSRC_COMPILER:Mclang} instead of the == operator.", + "ERROR: module.mk:5: Use ${PKGSRC_COMPILER:Mclang} instead of the == operator.", "", "\tThe PKGSRC_COMPILER can be a list of chained compilers, e.g. \"ccache", "\tdistcc clang\". Therefore, comparing it using == or != leads to wrong", @@ -1792,7 +1792,7 @@ func (s *Suite) Test_VartypeCheck_YesNoIndirectly(c *check.C) { type VartypeCheckTester struct { tester *Tester basicType *BasicType - filename string + filename Path lineno int varname string op MkOperator @@ -1820,7 +1820,7 @@ func (vt *VartypeCheckTester) Varname(varname string) { vt.nextSection() } -func (vt *VartypeCheckTester) File(filename string) { +func (vt *VartypeCheckTester) File(filename Path) { vt.filename = filename vt.lineno = 1 } |