From 27452582919966bc52e5b9a50b5b5a79c80c73a0 Mon Sep 17 00:00:00 2001 From: rillig Date: Sun, 12 Aug 2018 16:31:56 +0000 Subject: pkgtools/pkglint: update to 5.6.0 Changes since 5.5.16: * Check for negated shell commands (if ! test -z "foo"); they are not supported by Solaris. * Don't check variable permissions for infrastructure files. A warning like "may not be set by any package" doesn't make sense for them. * Check that PLIST_VARS matches PLIST.*, which is especially useful in options.mk files. * Improve checks for options.mk files (for PKG_OPTIONS_SET). * Prefer options handling with !empty() over checking empty() first. * Prefer ${MACHINE_ARCH} == i386 over !empty(MACHINE_ARCH:Mi386), for single-valued variables. --- pkgtools/pkglint/Makefile | 4 +- pkgtools/pkglint/files/alternatives.go | 22 +-- pkgtools/pkglint/files/alternatives_test.go | 2 +- pkgtools/pkglint/files/autofix_test.go | 49 ++++-- pkgtools/pkglint/files/buildlink3_test.go | 20 +-- pkgtools/pkglint/files/category.go | 10 +- pkgtools/pkglint/files/category_test.go | 7 +- pkgtools/pkglint/files/check_test.go | 84 +++++++--- pkgtools/pkglint/files/codewalk.md | 63 ++++++++ pkgtools/pkglint/files/distinfo.go | 117 +++++++------- pkgtools/pkglint/files/distinfo_test.go | 117 +++++++++++--- pkgtools/pkglint/files/files.go | 4 + pkgtools/pkglint/files/histogram/histogram.go | 2 +- pkgtools/pkglint/files/licenses.go | 2 +- pkgtools/pkglint/files/licenses_test.go | 3 +- pkgtools/pkglint/files/mkline.go | 123 ++++++++++----- pkgtools/pkglint/files/mkline_test.go | 10 +- pkgtools/pkglint/files/mklinechecker.go | 56 ++++--- pkgtools/pkglint/files/mklinechecker_test.go | 37 +++-- pkgtools/pkglint/files/mklines.go | 79 ++++++++-- pkgtools/pkglint/files/mklines_test.go | 179 ++++++++++++++++++++- pkgtools/pkglint/files/mkparser.go | 132 +++++++++++++--- pkgtools/pkglint/files/mkparser_test.go | 89 ++++++----- pkgtools/pkglint/files/mkshwalker.go | 214 ++++++++++++++++---------- pkgtools/pkglint/files/mkshwalker_test.go | 154 ++++++++++++++++-- pkgtools/pkglint/files/options.go | 41 +++-- pkgtools/pkglint/files/options_test.go | 63 ++++++-- pkgtools/pkglint/files/package.go | 59 ++++--- pkgtools/pkglint/files/package_test.go | 68 +++----- pkgtools/pkglint/files/patches_test.go | 47 +++++- pkgtools/pkglint/files/pkglint.go | 24 +-- pkgtools/pkglint/files/pkglint_test.go | 97 ++++++------ pkgtools/pkglint/files/pkgsrc.go | 2 +- pkgtools/pkglint/files/pkgsrc_test.go | 6 +- pkgtools/pkglint/files/pkgver/vercmp_test.go | 31 ++-- pkgtools/pkglint/files/shell.go | 67 ++++---- pkgtools/pkglint/files/shell_test.go | 30 ++++ pkgtools/pkglint/files/tools_test.go | 3 +- pkgtools/pkglint/files/toplevel.go | 13 +- pkgtools/pkglint/files/toplevel_test.go | 3 +- pkgtools/pkglint/files/util.go | 16 +- pkgtools/pkglint/files/util_test.go | 24 ++- pkgtools/pkglint/files/vardefs_test.go | 21 +++ pkgtools/pkglint/files/vartypecheck.go | 5 +- pkgtools/pkglint/files/vartypecheck_test.go | 71 ++++++--- 45 files changed, 1635 insertions(+), 635 deletions(-) create mode 100644 pkgtools/pkglint/files/vardefs_test.go (limited to 'pkgtools') diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index 56ea36117ec..7917ff5a463 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.545 2018/08/09 20:21:42 rillig Exp $ +# $NetBSD: Makefile,v 1.546 2018/08/12 16:31:56 rillig Exp $ -PKGNAME= pkglint-5.5.16 +PKGNAME= pkglint-5.6.0 DISTFILES= # none CATEGORIES= pkgtools diff --git a/pkgtools/pkglint/files/alternatives.go b/pkgtools/pkglint/files/alternatives.go index c09fd6124f4..6da77a32bff 100644 --- a/pkgtools/pkglint/files/alternatives.go +++ b/pkgtools/pkglint/files/alternatives.go @@ -13,17 +13,19 @@ func CheckfileAlternatives(filename string, plistFiles map[string]bool) { for _, line := range lines { if m, wrapper, space, implementation := match3(line.Text, `^(\S+)([ \t]+)(\S+)`); m { - if plistFiles[wrapper] { - line.Errorf("Alternative wrapper %q must not appear in the PLIST.", wrapper) - } + if plistFiles != nil { + if plistFiles[wrapper] { + line.Errorf("Alternative wrapper %q must not appear in the PLIST.", wrapper) + } - relImplementation := strings.Replace(implementation, "@PREFIX@/", "", 1) - plistName := regex.Compile(`@(\w+)@`).ReplaceAllString(relImplementation, "${$1}") - if !plistFiles[plistName] && !G.Pkg.vars.Defined("ALTERNATIVES_SRC") { - if plistName != implementation { - line.Errorf("Alternative implementation %q must appear in the PLIST as %q.", implementation, plistName) - } else { - line.Errorf("Alternative implementation %q must appear in the PLIST.", implementation) + relImplementation := strings.Replace(implementation, "@PREFIX@/", "", 1) + plistName := regex.Compile(`@(\w+)@`).ReplaceAllString(relImplementation, "${$1}") + if !plistFiles[plistName] && !G.Pkg.vars.Defined("ALTERNATIVES_SRC") { + if plistName != implementation { + line.Errorf("Alternative implementation %q must appear in the PLIST as %q.", implementation, plistName) + } else { + line.Errorf("Alternative implementation %q must appear in the PLIST.", implementation) + } } } diff --git a/pkgtools/pkglint/files/alternatives_test.go b/pkgtools/pkglint/files/alternatives_test.go index 6a678a8f808..1a2762cf09c 100644 --- a/pkgtools/pkglint/files/alternatives_test.go +++ b/pkgtools/pkglint/files/alternatives_test.go @@ -16,7 +16,7 @@ func (s *Suite) Test_Alternatives_PLIST(c *check.C) { G.Pkg.PlistFiles["bin/vim"] = true G.Pkg.PlistFiles["sbin/sendmail.exim${EXIMVER}"] = true - CheckfileAlternatives(t.TempFilename("ALTERNATIVES"), G.Pkg.PlistFiles) + CheckfileAlternatives(t.File("ALTERNATIVES"), G.Pkg.PlistFiles) t.CheckOutputLines( "ERROR: ~/ALTERNATIVES:1: Alternative implementation \"@PREFIX@/sbin/sendmail.postfix@POSTFIXVER@\" must appear in the PLIST as \"sbin/sendmail.postfix${POSTFIXVER}\".", diff --git a/pkgtools/pkglint/files/autofix_test.go b/pkgtools/pkglint/files/autofix_test.go index e9d00ef809f..7df2f637817 100644 --- a/pkgtools/pkglint/files/autofix_test.go +++ b/pkgtools/pkglint/files/autofix_test.go @@ -113,13 +113,13 @@ func (s *Suite) Test_autofix_MkLines(c *check.C) { t := s.Init(c) t.SetupCommandLine("--autofix") - t.SetupFileLines("Makefile", + t.SetupFileLines("category/basename/Makefile", "line1 := value1", "line2 := value2", "line3 := value3") pkg := NewPackage("category/basename") G.Pkg = pkg - mklines := pkg.loadPackageMakefile(t.TempFilename("Makefile")) + mklines := pkg.loadPackageMakefile() G.Pkg = nil fix := mklines.mklines[1].Autofix() @@ -135,19 +135,19 @@ func (s *Suite) Test_autofix_MkLines(c *check.C) { SaveAutofixChanges(mklines.lines) t.CheckOutputLines( - "AUTOFIX: ~/Makefile:2: Replacing \"lin\" with \"XXX\".", - "AUTOFIX: ~/Makefile:2: Replacing \"e2 \" with \"XXX\".", - "AUTOFIX: ~/Makefile:2: Replacing \":= \" with \"XXX\".", - "AUTOFIX: ~/Makefile:2: Replacing \"val\" with \"XXX\".", - "AUTOFIX: ~/Makefile:2: Replacing \"ue2\" with \"XXX\".", - "AUTOFIX: ~/Makefile:3: Replacing \"lin\" with \"XXX\".") - t.CheckFileLines("Makefile", + "AUTOFIX: ~/category/basename/Makefile:2: Replacing \"lin\" with \"XXX\".", + "AUTOFIX: ~/category/basename/Makefile:2: Replacing \"e2 \" with \"XXX\".", + "AUTOFIX: ~/category/basename/Makefile:2: Replacing \":= \" with \"XXX\".", + "AUTOFIX: ~/category/basename/Makefile:2: Replacing \"val\" with \"XXX\".", + "AUTOFIX: ~/category/basename/Makefile:2: Replacing \"ue2\" with \"XXX\".", + "AUTOFIX: ~/category/basename/Makefile:3: Replacing \"lin\" with \"XXX\".") + t.CheckFileLines("category/basename/Makefile", "line1 := value1", "XXXXXXXXXXXXXXX", "XXXe3 := value3") } -func (s *Suite) Test_Autofix_multiple_modifications(c *check.C) { +func (s *Suite) Test_Autofix__multiple_modifications(c *check.C) { t := s.Init(c) t.SetupCommandLine("--show-autofix", "--explain") @@ -449,3 +449,32 @@ func (s *Suite) Test_Autofix_Explain(c *check.C) { "WARN: Makefile:74: Please write row instead of line.") c.Check(G.explanationsAvailable, equals, true) } + +// Since the diagnostic doesn't contain the string "few", nothing happens. +func (s *Suite) Test_Autofix__skip(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("--only", "few", "--autofix") + + lines := t.SetupFileLines("fname", + "111 222 333 444 555") + + fix := lines[0].Autofix() + fix.Warnf("Many.") + fix.Explain( + "Explanation.") + fix.Replace("111", "___") + fix.ReplaceAfter(" ", "222", "___") + fix.ReplaceRegex(`\d+`, "___", 1) + fix.InsertBefore("before") + fix.InsertAfter("after") + fix.Delete() + fix.Apply() + + SaveAutofixChanges(lines) + + t.CheckOutputEmpty() + t.CheckFileLines("fname", + "111 222 333 444 555") + c.Check(lines[0].raw[0].textnl, equals, "111 222 333 444 555\n") +} diff --git a/pkgtools/pkglint/files/buildlink3_test.go b/pkgtools/pkglint/files/buildlink3_test.go index 414bab092f2..17f34cb8222 100644 --- a/pkgtools/pkglint/files/buildlink3_test.go +++ b/pkgtools/pkglint/files/buildlink3_test.go @@ -6,7 +6,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk(c *check.C) { t := s.Init(c) t.SetupVartypes() - mklines := t.NewMkLines("buildlink3.mk", + mklines := t.SetupFileMkLines("buildlink3.mk", MkRcsID, "# XXX This file was created automatically using createbuildlink-@PKGVERSION@", "", @@ -28,10 +28,10 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk(c *check.C) { ChecklinesBuildlink3Mk(mklines) t.CheckOutputLines( - "ERROR: buildlink3.mk:12: \"/x11/Xbae\" does not exist.", - "ERROR: buildlink3.mk:12: There is no package in \"x11/Xbae\".", - "ERROR: buildlink3.mk:14: \"/mk/motif.buildlink3.mk\" does not exist.", - "ERROR: buildlink3.mk:2: This comment indicates unfinished work (url2pkg).") + "ERROR: ~/buildlink3.mk:12: \"x11/Xbae\" does not exist.", + "ERROR: ~/buildlink3.mk:12: There is no package in \"x11/Xbae\".", + "ERROR: ~/buildlink3.mk:14: \"mk/motif.buildlink3.mk\" does not exist.", + "ERROR: ~/buildlink3.mk:2: This comment indicates unfinished work (url2pkg).") } // Before version 5.3, pkglint wrongly warned here. @@ -315,7 +315,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk_indentation(c *check.C) { t.SetupCommandLine("-Wall") t.SetupVartypes() - mklines := t.NewMkLines("buildlink3.mk", + mklines := t.SetupFileMkLines("buildlink3.mk", MkRcsID, "", ".if ${VAAPI_AVAILABLE} == \"yes\"", @@ -340,8 +340,8 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk_indentation(c *check.C) { // No warning about the indentation of the .include lines. t.CheckOutputLines( - "ERROR: buildlink3.mk:11: \"/multimedia/libva\" does not exist.", - "ERROR: buildlink3.mk:11: There is no package in \"multimedia/libva\".", - "ERROR: buildlink3.mk:13: \"/x11/libX11/buildlink3.mk\" does not exist.", - "WARN: buildlink3.mk:3: Expected a BUILDLINK_TREE line.") + "ERROR: ~/buildlink3.mk:11: \"multimedia/libva\" does not exist.", + "ERROR: ~/buildlink3.mk:11: There is no package in \"multimedia/libva\".", + "ERROR: ~/buildlink3.mk:13: \"x11/libX11/buildlink3.mk\" does not exist.", + "WARN: ~/buildlink3.mk:3: Expected a BUILDLINK_TREE line.") } diff --git a/pkgtools/pkglint/files/category.go b/pkgtools/pkglint/files/category.go index 34af8dc8a29..3f38ff30c6c 100644 --- a/pkgtools/pkglint/files/category.go +++ b/pkgtools/pkglint/files/category.go @@ -5,12 +5,12 @@ import ( "sort" ) -func CheckdirCategory() { +func CheckdirCategory(dir string) { if trace.Tracing { - defer trace.Call1(G.CurrentDir)() + defer trace.Call1(dir)() } - lines := LoadNonemptyLines(G.CurrentDir+"/Makefile", true) + lines := LoadNonemptyLines(dir+"/Makefile", true) if lines == nil { return } @@ -40,7 +40,7 @@ func CheckdirCategory() { // the (hopefully) sorted list of SUBDIRs. The first step is to // collect the SUBDIRs in the Makefile and in the file system. - fSubdirs := getSubdirs(G.CurrentDir) + fSubdirs := getSubdirs(dir) sort.Strings(fSubdirs) var mSubdirs []subdir @@ -147,7 +147,7 @@ func CheckdirCategory() { fNeednext = true mNeednext = true if mActive { - subdirs = append(subdirs, G.CurrentDir+"/"+mCurrent) + subdirs = append(subdirs, dir+"/"+mCurrent) } } } diff --git a/pkgtools/pkglint/files/category_test.go b/pkgtools/pkglint/files/category_test.go index 5fcb3735b35..203bf580d46 100644 --- a/pkgtools/pkglint/files/category_test.go +++ b/pkgtools/pkglint/files/category_test.go @@ -13,9 +13,8 @@ func (s *Suite) Test_CheckdirCategory_totally_broken(c *check.C) { "SUBDIR-=unknown #doesn\u2019t work", "", ".include \"../mk/category.mk\"") - G.CurrentDir = t.TempFilename("archivers") - CheckdirCategory() + CheckdirCategory(t.File("archivers")) t.CheckOutputLines( "ERROR: ~/archivers/Makefile:1: Expected \"# $"+"NetBSD$\".", @@ -48,10 +47,8 @@ func (s *Suite) Test_CheckdirCategory_invalid_comment(c *check.C) { "# dummy") t.SetupFileLines("mk/misc/category.mk", "# dummy") - G.CurrentDir = t.TempFilename("archivers") - G.CurPkgsrcdir = ".." - CheckdirCategory() + CheckdirCategory(t.File("archivers")) t.CheckOutputLines( "WARN: ~/archivers/Makefile:2: COMMENT contains invalid characters (U+005C U+0024 U+0024 U+0024 U+0024 U+0022).") diff --git a/pkgtools/pkglint/files/check_test.go b/pkgtools/pkglint/files/check_test.go index 90ed786126b..c275e3f5d24 100644 --- a/pkgtools/pkglint/files/check_test.go +++ b/pkgtools/pkglint/files/check_test.go @@ -51,7 +51,7 @@ func (s *Suite) SetUpTest(c *check.C) { G.logOut = NewSeparatorWriter(&t.stdout) G.logErr = NewSeparatorWriter(&t.stderr) trace.Out = &t.stdout - G.Pkgsrc = NewPkgsrc(t.TmpDir()) + G.Pkgsrc = NewPkgsrc(t.File(".")) t.checkC = c t.SetupCommandLine( /* no arguments */ ) @@ -160,13 +160,57 @@ func (t *Tester) SetupFileMkLines(relativeFilename string, lines ...string) *MkL return NewMkLines(plainLines) } +// SetupPkgsrc sets up a minimal but complete pkgsrc installation in the +// temporary folder, so that pkglint runs without any errors. +// Individual files may be overwritten by calling other Setup* methods. +// This setup is especially interesting for testing Pkglint.Main. +func (t *Tester) SetupPkgsrc() { + + // This file is needed to locate the pkgsrc root directory. + // See findPkgsrcTopdir. + t.CreateFileLines("mk/bsd.pkg.mk", + MkRcsID) + + // See Pkgsrc.loadDocChanges. + t.CreateFileLines("doc/CHANGES-2018", + RcsID) + + // See Pkgsrc.loadSuggestedUpdates. + t.CreateFileLines("doc/TODO", + RcsID) + + // The MASTER_SITES in the package Makefile are searched here. + // See Pkgsrc.loadMasterSites. + t.CreateFileLines("mk/fetch/sites.mk", + MkRcsID) + + // The options for the PKG_OPTIONS framework must be readable. + // See Pkgsrc.loadPkgOptions. + t.CreateFileLines("mk/defaults/options.description") + + // The user-defined variables are read in to check for missing + // BUILD_DEFS declarations in the package Makefile. + t.CreateFileLines("mk/defaults/mk.conf", + MkRcsID) + + // The tool definitions are read in to check for missing + // USE_TOOLS declarations in the package Makefile. + // They spread over several files from the pkgsrc infrastructure. + t.CreateFileLines("mk/tools/bsd.tools.mk", + ".include \"defaults.mk\"") + t.CreateFileLines("mk/tools/defaults.mk", + MkRcsID) + t.CreateFileLines("mk/bsd.prefs.mk", // Some tools are defined here. + MkRcsID) +} + func (t *Tester) CreateFileLines(relativeFilename string, lines ...string) (filename string) { content := "" for _, line := range lines { content += line + "\n" } - filename = t.TempFilename(relativeFilename) + filename = t.File(relativeFilename) err := os.MkdirAll(path.Dir(filename), 0777) t.c().Assert(err, check.IsNil) @@ -176,31 +220,23 @@ func (t *Tester) CreateFileLines(relativeFilename string, lines ...string) (file return filename } -func (t *Tester) LoadTmpFile(relFname string) (absFname string) { - bytes, err := ioutil.ReadFile(t.TmpDir() + "/" + relFname) - t.c().Assert(err, check.IsNil) - return string(bytes) -} - -func (t *Tester) TmpDir() string { +// File returns the absolute path to the given file in the +// temporary directory. It doesn't check whether that file exists. +func (t *Tester) File(relativeFilename string) string { if t.tmpdir == "" { t.tmpdir = filepath.ToSlash(t.c().MkDir()) } - return t.tmpdir -} - -// TempFilename returns the absolute path to the given file in the -// temporary directory. It doesn't check whether that file exists. -func (t *Tester) TempFilename(relativeFilename string) string { - return t.TmpDir() + "/" + relativeFilename + return t.tmpdir + "/" + relativeFilename } +// ExpectFatalError, when run in a defer statement, runs the action +// if the current function panics with a pkglintFatal +// (typically from line.Fatalf). func (t *Tester) ExpectFatalError(action func()) { - if r := recover(); r != nil { - if _, ok := r.(pkglintFatal); ok { - action() - return - } + r := recover() + if _, ok := r.(pkglintFatal); ok { + action() + } else { panic(r) } } @@ -319,7 +355,9 @@ 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) { - text := t.LoadTmpFile(relativeFileName) + content, err := ioutil.ReadFile(t.File(relativeFileName)) + t.c().Assert(err, check.IsNil) + text := string(content) actualLines := strings.Split(text, "\n") actualLines = actualLines[:len(actualLines)-1] t.c().Check(emptyToNil(actualLines), deepEquals, emptyToNil(lines)) @@ -330,7 +368,7 @@ func (t *Tester) CheckFileLines(relativeFileName string, lines ...string) { // for indentation, while the lines in the code use spaces exclusively, // in order to make the depth of the indentation clearly visible. func (t *Tester) CheckFileLinesDetab(relativeFileName string, lines ...string) { - actualLines, err := readLines(t.TempFilename(relativeFileName), false) + actualLines, err := readLines(t.File(relativeFileName), false) if !t.c().Check(err, check.IsNil) { return } diff --git a/pkgtools/pkglint/files/codewalk.md b/pkgtools/pkglint/files/codewalk.md index 5f79f39f7ac..baf446309a0 100755 --- a/pkgtools/pkglint/files/codewalk.md +++ b/pkgtools/pkglint/files/codewalk.md @@ -222,3 +222,66 @@ file shell.go start ^type ShellLine struct end ^\} ``` + +## Testing pkglint + +### Standard shape of a test + +```go +func (s *Suite) Test_Type_Method__description(c *check.C) { + t := s.Init(c) // Every test needs this. + + t.Setup…(…) // Set up the testing environment. + + lines := t.New…(…) // Set up the test data. + + CodeToBeTested() // The code to be tested. + + t.Check…(…) // Check the result (typically diagnostics). +} +``` + +The `t` variable is the center of most tests. +It is of type `Tester` and provides a high-level interface +for setting up tests and checking the results. + +```codewalk +file check_test.go +start /^type Tester/ upwhile /^\/\// +end ^\} +``` + +The `s` variable is not used in tests. +The only purpose of its type `Suite` is to group the tests so they are all run together. + +The `c` variable comes from [gocheck](https://godoc.org/gopkg.in/check.v1), +which is the underlying testing framework. +Most pkglint tests don't need this variable. +Low-level tests call `c.Check` to compare their results to the expected values. + +```codewalk +file util_test.go +start ^func .* Test_tabLength +end ^\} +``` + +### Logging detailed information during tests + +When testing complicated code, it sometimes helps to have a detailed trace +of the code that is run. This is done via these two methods: + +```go +t.EnableTracing() +t.DisableTracing() +``` + +### Setting up a realistic pkgsrc environment + +To see how to setup complicated tests, have a look at the following test, +which sets up a realistic environment to run the tests in. + +```codewalk +file pkglint_test.go +start ^func .* Test_Pkglint_Main__complete_package +end ^\} +``` diff --git a/pkgtools/pkglint/files/distinfo.go b/pkgtools/pkglint/files/distinfo.go index 41e17e2a98e..4b734ce134d 100644 --- a/pkgtools/pkglint/files/distinfo.go +++ b/pkgtools/pkglint/files/distinfo.go @@ -6,6 +6,7 @@ import ( "fmt" "io/ioutil" "netbsd.org/pkglint/trace" + "path" "strings" ) @@ -15,25 +16,18 @@ func ChecklinesDistinfo(lines []Line) { } fname := lines[0].Filename - patchesDir := "patches" - patchesDirSet := false - if G.Pkg != nil && contains(fname, "lang/php") { - phpdir := G.Pkgsrc.Latest("lang", `^php[0-9]+$`, "/lang/$0") - if hasSuffix(fname, phpdir+"/distinfo") { - patchesDir = G.CurPkgsrcdir + phpdir + "/patches" - patchesDirSet = true - } - } - if G.Pkg != nil && !patchesDirSet && dirExists(G.CurrentDir+"/"+G.Pkg.Patchdir) { - patchesDir = G.Pkg.Patchdir + patchdir := "patches" + if G.Pkg != nil && dirExists(G.Pkg.File(G.Pkg.Patchdir)) { + patchdir = G.Pkg.Patchdir } if trace.Tracing { - trace.Step1("patchesDir=%q", patchesDir) + trace.Step1("patchdir=%q", patchdir) } + distinfoIsCommitted := isCommitted(fname) ck := &distinfoLinesChecker{ - fname, patchesDir, isCommitted(fname), - make(map[string]bool), nil, "", false, nil} + fname, patchdir, distinfoIsCommitted, + make(map[string]bool), nil, "", unknown, nil} ck.checkLines(lines) ChecklinesTrailingEmptyLines(lines) ck.checkUnrecordedPatches() @@ -42,13 +36,13 @@ func ChecklinesDistinfo(lines []Line) { type distinfoLinesChecker struct { distinfoFilename string - patchdir string // Relative to G.currentDir + patchdir string // Relative to G.Pkg distinfoIsCommitted bool patches map[string]bool // "patch-aa" => true currentFirstLine Line currentFilename string - isPatch bool + isPatch YesNoUnknown algorithms []string } @@ -83,12 +77,15 @@ func (ck *distinfoLinesChecker) onFilenameChange(line Line, nextFname string) { currentFname := ck.currentFilename if currentFname != "" { algorithms := strings.Join(ck.algorithms, ", ") - if ck.isPatch { + if ck.isPatch == yes { if algorithms != "SHA1" { line.Errorf("Expected SHA1 hash for %s, got %s.", currentFname, algorithms) } + } else if ck.isPatch == unknown { + } else if G.Pkg != nil && G.Pkg.IgnoreMissingPatches { } else if hasPrefix(currentFname, "patch-") && algorithms == "SHA1" { - ck.currentFirstLine.Warnf("Patch file %q does not exist in directory %q.", currentFname, cleanpath(ck.patchdir)) + pathToPatchdir := relpath(path.Dir(ck.currentFirstLine.Filename), G.Pkg.File(ck.patchdir)) + ck.currentFirstLine.Warnf("Patch file %q does not exist in directory %q.", currentFname, pathToPatchdir) Explain( "If the patches directory looks correct, the patch may have been", "removed without updating the distinfo file. In such a case please", @@ -100,46 +97,26 @@ func (ck *distinfoLinesChecker) onFilenameChange(line Line, nextFname string) { } } - ck.isPatch = hasPrefix(nextFname, "patch-") && fileExists(G.CurrentDir+"/"+ck.patchdir+"/"+nextFname) + if !hasPrefix(nextFname, "patch-") { + ck.isPatch = no + } else if G.Pkg == nil { + ck.isPatch = unknown + } else if fileExists(G.Pkg.File(ck.patchdir + "/" + nextFname)) { + ck.isPatch = yes + } else { + ck.isPatch = no + } + ck.currentFilename = nextFname ck.currentFirstLine = line ck.algorithms = nil } -// Same as in mk/checksum/distinfo.awk:/function patchsum/ -func computePatchSha1Hex(patchFilename string) (string, error) { - patchBytes, err := ioutil.ReadFile(patchFilename) - if err != nil { - return "", err - } - - hash := sha1.New() - netbsd := []byte("$" + "NetBSD") - for _, patchLine := range bytes.SplitAfter(patchBytes, []byte("\n")) { - if !bytes.Contains(patchLine, netbsd) { - hash.Write(patchLine) - } - } - return fmt.Sprintf("%x", hash.Sum(nil)), nil -} - -func (ck *distinfoLinesChecker) checkPatchSha1(line Line, patchFname, distinfoSha1Hex string) { - fileSha1Hex, err := computePatchSha1Hex(G.CurrentDir + "/" + patchFname) - if err != nil { - line.Errorf("%s does not exist.", patchFname) +func (ck *distinfoLinesChecker) checkUnrecordedPatches() { + if G.Pkg == nil { return } - if distinfoSha1Hex != fileSha1Hex { - fix := line.Autofix() - fix.Errorf("%s hash of %s differs (distinfo has %s, patch file has %s). Run \"%s makepatchsum\".", - "SHA1", patchFname, distinfoSha1Hex, fileSha1Hex, confMake) - fix.Replace(distinfoSha1Hex, fileSha1Hex) - fix.Apply() - } -} - -func (ck *distinfoLinesChecker) checkUnrecordedPatches() { - files, err := ioutil.ReadDir(G.CurrentDir + "/" + ck.patchdir) + files, err := ioutil.ReadDir(G.Pkg.File(ck.patchdir)) if err != nil { if trace.Tracing { trace.Stepf("Cannot read patchesDir %q: %s", ck.patchdir, err) @@ -173,9 +150,9 @@ func (ck *distinfoLinesChecker) checkGlobalMismatch(line Line, fname, alg, hash } func (ck *distinfoLinesChecker) checkUncommittedPatch(line Line, patchName, sha1Hash string) { - if ck.isPatch { + if ck.isPatch == yes { patchFname := ck.patchdir + "/" + patchName - if ck.distinfoIsCommitted && !isCommitted(G.CurrentDir+"/"+patchFname) { + if ck.distinfoIsCommitted && !isCommitted(G.Pkg.File(patchFname)) { line.Warnf("%s is registered in distinfo but not added to CVS.", patchFname) } ck.checkPatchSha1(line, patchFname, sha1Hash) @@ -183,8 +160,40 @@ func (ck *distinfoLinesChecker) checkUncommittedPatch(line Line, patchName, sha1 } } +func (ck *distinfoLinesChecker) checkPatchSha1(line Line, patchFname, distinfoSha1Hex string) { + fileSha1Hex, err := computePatchSha1Hex(G.Pkg.File(patchFname)) + if err != nil { + line.Errorf("%s does not exist.", patchFname) + return + } + if distinfoSha1Hex != fileSha1Hex { + fix := line.Autofix() + fix.Errorf("%s hash of %s differs (distinfo has %s, patch file has %s). Run \"%s makepatchsum\".", + "SHA1", patchFname, distinfoSha1Hex, fileSha1Hex, confMake) + fix.Replace(distinfoSha1Hex, fileSha1Hex) + fix.Apply() + } +} + +// Same as in mk/checksum/distinfo.awk:/function patchsum/ +func computePatchSha1Hex(patchFilename string) (string, error) { + patchBytes, err := ioutil.ReadFile(patchFilename) + if err != nil { + return "", err + } + + hash := sha1.New() + netbsd := []byte("$" + "NetBSD") + for _, patchLine := range bytes.SplitAfter(patchBytes, []byte("\n")) { + if !bytes.Contains(patchLine, netbsd) { + hash.Write(patchLine) + } + } + return fmt.Sprintf("%x", hash.Sum(nil)), nil +} + func AutofixDistinfo(oldSha1, newSha1 string) { - distinfoFilename := G.CurrentDir + "/" + G.Pkg.DistinfoFile + distinfoFilename := G.Pkg.File(G.Pkg.DistinfoFile) if lines, err := readLines(distinfoFilename, false); err == nil { for _, line := range lines { fix := line.Autofix() diff --git a/pkgtools/pkglint/files/distinfo_test.go b/pkgtools/pkglint/files/distinfo_test.go index 7290e469fbf..3a42088858a 100644 --- a/pkgtools/pkglint/files/distinfo_test.go +++ b/pkgtools/pkglint/files/distinfo_test.go @@ -5,27 +5,28 @@ import "gopkg.in/check.v1" func (s *Suite) Test_ChecklinesDistinfo(c *check.C) { t := s.Init(c) - t.SetupFileLines("patches/patch-aa", + t.SetupFileLines("category/package/patches/patch-aa", "$"+"NetBSD$ line is ignored", "patch contents") - t.SetupFileLines("patches/patch-ab", + t.SetupFileLines("category/package/patches/patch-ab", "patch contents") - G.CurrentDir = t.TmpDir() - - ChecklinesDistinfo(t.NewLines("distinfo", + lines := t.SetupFileLines("category/package/distinfo", "should be the RCS ID", "should be empty", "MD5 (distfile.tar.gz) = 12345678901234567890123456789012", "SHA1 (distfile.tar.gz) = 1234567890123456789012345678901234567890", "SHA1 (patch-aa) = 6b98dd609f85a9eb9c4c1e4e7055a6aaa62b7cc7", "SHA1 (patch-ab) = 6b98dd609f85a9eb9c4c1e4e7055a6aaa62b7cc7", - "SHA1 (patch-nonexistent) = 1234")) + "SHA1 (patch-nonexistent) = 1234") + G.Pkg = NewPackage("category/package") + + ChecklinesDistinfo(lines) t.CheckOutputLines( - "ERROR: distinfo:1: Expected \"$"+"NetBSD$\".", - "NOTE: distinfo:2: Empty line expected.", - "ERROR: distinfo:5: Expected SHA1, RMD160, SHA512, Size checksums for \"distfile.tar.gz\", got MD5, SHA1.", - "WARN: distinfo:7: Patch file \"patch-nonexistent\" does not exist in directory \"patches\".") + "ERROR: ~/category/package/distinfo:1: Expected \"$"+"NetBSD$\".", + "NOTE: ~/category/package/distinfo:2: Empty line expected.", + "ERROR: ~/category/package/distinfo:5: Expected SHA1, RMD160, SHA512, Size checksums for \"distfile.tar.gz\", got MD5, SHA1.", + "WARN: ~/category/package/distinfo:7: Patch file \"patch-nonexistent\" does not exist in directory \"patches\".") } func (s *Suite) Test_ChecklinesDistinfo_global_hash_mismatch(c *check.C) { @@ -48,7 +49,7 @@ func (s *Suite) Test_ChecklinesDistinfo_global_hash_mismatch(c *check.C) { func (s *Suite) Test_ChecklinesDistinfo_uncommitted_patch(c *check.C) { t := s.Init(c) - t.SetupFileLines("patches/patch-aa", + t.SetupFileLines("category/package/patches/patch-aa", RcsID, "", "--- oldfile", @@ -56,54 +57,122 @@ func (s *Suite) Test_ChecklinesDistinfo_uncommitted_patch(c *check.C) { "@@ -1,1 +1,1 @@", "-old", "+new") - t.SetupFileLines("CVS/Entries", + t.SetupFileLines("category/package/CVS/Entries", "/distinfo/...") - lines := t.SetupFileLines("distinfo", + lines := t.SetupFileLines("category/package/distinfo", RcsID, "", "SHA1 (patch-aa) = 5ad1fb9b3c328fff5caa1a23e8f330e707dd50c0") - G.CurrentDir = t.TmpDir() + G.Pkg = NewPackage("category/package") ChecklinesDistinfo(lines) t.CheckOutputLines( - "WARN: ~/distinfo:3: patches/patch-aa is registered in distinfo but not added to CVS.") + "WARN: ~/category/package/distinfo:3: patches/patch-aa is registered in distinfo but not added to CVS.") } func (s *Suite) Test_ChecklinesDistinfo_unrecorded_patches(c *check.C) { t := s.Init(c) - t.SetupFileLines("patches/CVS/Entries") - t.SetupFileLines("patches/patch-aa") - t.SetupFileLines("patches/patch-src-Makefile") - lines := t.SetupFileLines("distinfo", + t.SetupFileLines("category/package/patches/CVS/Entries") + t.SetupFileLines("category/package/patches/patch-aa") + t.SetupFileLines("category/package/patches/patch-src-Makefile") + lines := t.SetupFileLines("category/package/distinfo", RcsID, "", "SHA1 (distfile.tar.gz) = ...", "RMD160 (distfile.tar.gz) = ...", "SHA512 (distfile.tar.gz) = ...", "Size (distfile.tar.gz) = 1024 bytes") - G.CurrentDir = t.TmpDir() + G.Pkg = NewPackage("category/package") ChecklinesDistinfo(lines) t.CheckOutputLines( - "ERROR: ~/distinfo: patch \"patches/patch-aa\" is not recorded. Run \""+confMake+" makepatchsum\".", - "ERROR: ~/distinfo: patch \"patches/patch-src-Makefile\" is not recorded. Run \""+confMake+" makepatchsum\".") + "ERROR: ~/category/package/distinfo: patch \"patches/patch-aa\" is not recorded. Run \""+confMake+" makepatchsum\".", + "ERROR: ~/category/package/distinfo: patch \"patches/patch-src-Makefile\" is not recorded. Run \""+confMake+" makepatchsum\".") } func (s *Suite) Test_ChecklinesDistinfo_manual_patches(c *check.C) { t := s.Init(c) - t.SetupFileLines("patches/manual-libtool.m4") + t.CreateFileLines("patches/manual-libtool.m4") lines := t.SetupFileLines("distinfo", RcsID, "", "SHA1 (patch-aa) = ...") - G.CurrentDir = t.TmpDir() ChecklinesDistinfo(lines) + // When a distinfo file is checked on its own, without belonging to a package, + // the PATCHDIR is not known and therefore no diagnostics are logged. + t.CheckOutputEmpty() + + G.Pkg = NewPackage(".") + + ChecklinesDistinfo(lines) + + // When a distinfo file is checked in the context of a package, + // the PATCHDIR is known, therefore the checks are active. t.CheckOutputLines( "WARN: ~/distinfo:3: Patch file \"patch-aa\" does not exist in directory \"patches\".") } + +// PHP modules that are not PECL use the distinfo file from lang/php* but +// their own patches directory. Therefore the distinfo file refers to missing +// patches. Since this strange situation is caused by the pkgsrc +// infrastructure, there is nothing a package author can do about. +func (s *Suite) Test_ChecklinesDistinfo__missing_php_patches(c *check.C) { + t := s.Init(c) + + t.SetupPkgsrc() + t.SetupCommandLine("-Wall,no-space") + t.CreateFileLines("licenses/unknown-license") + t.CreateFileLines("lang/php/ext.mk", + MkRcsID, + "", + "PHPEXT_MK= # defined", + "PHPPKGSRCDIR= lang/php72", + "LICENSE?= unknown-license", + "COMMENT?= Some PHP package", + "GENERATE_PLIST+=# none", + "", + ".if !defined(PECL_VERSION)", + "DISTINFO_FILE= ${.CURDIR}/${PHPPKGSRCDIR}/distinfo", + ".endif", + ".if defined(USE_PHP_EXT_PATCHES)", + "PATCHDIR= ${.CURDIR}/${PHPPKGSRCDIR}/patches", + ".endif") + t.CreateFileLines("lang/php72/patches/patch-php72", + RcsID, + "", + "Documentation", + "", + "--- old file", + "+++ new file", + "@@ -1,1 +1,1 @@", + "-old", + "+new") + t.SetupFileLines("lang/php72/distinfo", + RcsID, + "", + "SHA1 (patch-php72) = c109b2089f5ddbc5372b2ab28115ff558ee4187d") + + t.CreateFileLines("archivers/php-bz2/Makefile", + MkRcsID, + "", + "USE_PHP_EXT_PATCHES= yes", + "", + ".include \"../../lang/php/ext.mk\"", + ".include \"../../mk/bsd.pkg.mk\"") + t.CreateFileLines("archivers/php-zlib/Makefile", + MkRcsID, + "", + ".include \"../../lang/php/ext.mk\"", + ".include \"../../mk/bsd.pkg.mk\"") + + G.CheckDirent(t.File("archivers/php-bz2")) + G.CheckDirent(t.File("archivers/php-zlib")) + + t.CheckOutputEmpty() +} diff --git a/pkgtools/pkglint/files/files.go b/pkgtools/pkglint/files/files.go index b51a075cfc3..9a8bf119c8c 100644 --- a/pkgtools/pkglint/files/files.go +++ b/pkgtools/pkglint/files/files.go @@ -2,6 +2,7 @@ package main import ( "io/ioutil" + "path" "strings" ) @@ -111,6 +112,9 @@ func readLines(fname string, joinBackslashLines bool) ([]Line, error) { return nil, err } + if G.opts.Profiling { + G.loaded.Add(path.Clean(fname), 1) + } return convertToLogicalLines(fname, string(rawText), joinBackslashLines), nil } diff --git a/pkgtools/pkglint/files/histogram/histogram.go b/pkgtools/pkglint/files/histogram/histogram.go index ad802d949a9..fdccfeae5f9 100644 --- a/pkgtools/pkglint/files/histogram/histogram.go +++ b/pkgtools/pkglint/files/histogram/histogram.go @@ -40,7 +40,7 @@ func (h *Histogram) PrintStats(caption string, out io.Writer, limit int) { for i, entry := range entries { fmt.Fprintf(out, "%s %6d %s\n", caption, entry.count, entry.s) - if limit > 0 && i >= limit { + if limit >= 0 && i >= limit { break } } diff --git a/pkgtools/pkglint/files/licenses.go b/pkgtools/pkglint/files/licenses.go index 8d63fe21b80..9a16e6f6292 100644 --- a/pkgtools/pkglint/files/licenses.go +++ b/pkgtools/pkglint/files/licenses.go @@ -48,7 +48,7 @@ func (lc *LicenseChecker) checkLicenseName(license string) { var licenseFile string if G.Pkg != nil { if licenseFileValue, ok := G.Pkg.varValue("LICENSE_FILE"); ok { - licenseFile = G.CurrentDir + "/" + lc.MkLine.ResolveVarsInRelativePath(licenseFileValue, false) + licenseFile = G.Pkg.File(lc.MkLine.ResolveVarsInRelativePath(licenseFileValue, false)) } } if licenseFile == "" { diff --git a/pkgtools/pkglint/files/licenses_test.go b/pkgtools/pkglint/files/licenses_test.go index 2f5b543e3df..e97f1328ee5 100644 --- a/pkgtools/pkglint/files/licenses_test.go +++ b/pkgtools/pkglint/files/licenses_test.go @@ -10,7 +10,6 @@ func (s *Suite) Test_checklineLicense(c *check.C) { t.SetupFileLines("licenses/gnu-gpl-v2", "Most software \u2026") mkline := t.NewMkLine("Makefile", 7, "LICENSE=dummy") - G.CurrentDir = t.TmpDir() licenseChecker := &LicenseChecker{mkline} licenseChecker.Check("gpl-v2", opAssign) @@ -85,7 +84,7 @@ func (s *Suite) Test_checkToplevelUnusedLicenses(c *check.C) { PlistRcsID, "bin/program") - G.Main("pkglint", "-r", "-Cglobal", t.TmpDir()) + G.Main("pkglint", "-r", "-Cglobal", t.File(".")) t.CheckOutputLines( "WARN: ~/licenses/gnu-gpl-v3: This license seems to be unused.", diff --git a/pkgtools/pkglint/files/mkline.go b/pkgtools/pkglint/files/mkline.go index 1814b65906c..b8882a82512 100644 --- a/pkgtools/pkglint/files/mkline.go +++ b/pkgtools/pkglint/files/mkline.go @@ -6,6 +6,7 @@ import ( "fmt" "netbsd.org/pkglint/regex" "netbsd.org/pkglint/trace" + "path" "strings" ) @@ -39,6 +40,7 @@ type mkLineConditional struct { directive string args string comment string + elseLine MkLine // (filled in later) } type mkLineInclude struct { mustExist bool @@ -114,7 +116,7 @@ func NewMkLine(line Line) *MkLineImpl { } if m, indent, directive, args, comment := matchMkCond(text); m { - return &MkLineImpl{line, mkLineConditional{indent, directive, args, comment}} + return &MkLineImpl{line, mkLineConditional{indent, directive, args, comment, nil}} } if m, indent, directive, includefile := MatchMkInclude(text); m { @@ -246,6 +248,12 @@ func (mkline *MkLineImpl) Args() string { return mkline.data.(mkLineConditi // CondComment is the trailing end-of-line comment, typically at a deeply nested .endif or .endfor. func (mkline *MkLineImpl) CondComment() string { return mkline.data.(mkLineConditional).comment } +func (mkline *MkLineImpl) HasElseBranch() bool { return mkline.data.(mkLineConditional).elseLine != nil } +func (mkline *MkLineImpl) SetHasElseBranch(elseLine MkLine) { + data := mkline.data.(mkLineConditional) + data.elseLine = elseLine + mkline.data = data +} func (mkline *MkLineImpl) MustExist() bool { return mkline.data.(mkLineInclude).mustExist } func (mkline *MkLineImpl) IncludeFile() string { return mkline.data.(mkLineInclude).includeFile } @@ -263,6 +271,11 @@ func (mkline *MkLineImpl) SetConditionVars(varnames string) { mkline.data = include } +// Tokenize extracts variable uses and other text from the string. +// +// Example: +// input: ${PREFIX}/bin abc +// output: [MkToken("${PREFIX}", MkVarUse("PREFIX")), MkToken("/bin abc")] func (mkline *MkLineImpl) Tokenize(s string) []*MkToken { if trace.Tracing { defer trace.Call(mkline, s)() @@ -280,6 +293,8 @@ func (mkline *MkLineImpl) Tokenize(s string) []*MkToken { // taking care of variable references. For example, when the value // "/bin:${PATH:S,::,::,}" is split at ":", it results in // {"/bin", "${PATH:S,::,::,}"}. +// +// If the separator is empty, splitting is done on whitespace. func (mkline *MkLineImpl) ValueSplit(value string, separator string) []string { tokens := mkline.Tokenize(value) var split []string @@ -288,7 +303,12 @@ func (mkline *MkLineImpl) ValueSplit(value string, separator string) []string { split = []string{""} } if token.Varuse == nil && contains(token.Text, separator) { - subs := strings.Split(token.Text, separator) + var subs []string + if separator == "" { + subs = splitOnSpace(token.Text) + } else { + subs = strings.Split(token.Text, separator) + } split[len(split)-1] += subs[0] split = append(split, subs[1:]...) } else { @@ -310,9 +330,18 @@ func (mkline *MkLineImpl) WithoutMakeVariables(value string) string { } } -func (mkline *MkLineImpl) ResolveVarsInRelativePath(relpath string, adjustDepth bool) string { - tmp := relpath - tmp = strings.Replace(tmp, "${PKGSRCDIR}", G.CurPkgsrcdir, -1) +func (mkline *MkLineImpl) ResolveVarsInRelativePath(relativePath string, adjustDepth bool) string { + + var basedir string + if G.Pkg != nil { + basedir = G.Pkg.File(".") + } else { + basedir = path.Dir(mkline.Filename) + } + pkgsrcdir := relpath(basedir, G.Pkgsrc.File(".")) + + tmp := relativePath + tmp = strings.Replace(tmp, "${PKGSRCDIR}", pkgsrcdir, -1) tmp = strings.Replace(tmp, "${.CURDIR}", ".", -1) tmp = strings.Replace(tmp, "${.PARSEDIR}", ".", -1) if contains(tmp, "${LUA_PKGSRCDIR}") { @@ -338,35 +367,36 @@ func (mkline *MkLineImpl) ResolveVarsInRelativePath(relpath string, adjustDepth if adjustDepth { if m, pkgpath := match1(tmp, `^\.\./\.\./([^.].*)$`); m { - tmp = G.CurPkgsrcdir + "/" + pkgpath + tmp = pkgsrcdir + "/" + pkgpath } } + tmp = cleanpath(tmp) + if trace.Tracing { - trace.Step2("resolveVarsInRelativePath: %q => %q", relpath, tmp) + trace.Step2("resolveVarsInRelativePath: %q => %q", relativePath, tmp) } return tmp } -func (ind *Indentation) RememberUsedVariables(cond *Tree) { - arg0varname := func(node *Tree) { - varname := node.args[0].(string) - ind.AddVar(varname) - } - arg0varuse := func(node *Tree) { - varuse := node.args[0].(MkVarUse) - ind.AddVar(varuse.varname) - } - arg2varuse := func(node *Tree) { - varuse := node.args[2].(MkVarUse) - ind.AddVar(varuse.varname) - } - cond.Visit("defined", arg0varname) - cond.Visit("empty", arg0varuse) - cond.Visit("compareVarNum", arg0varuse) - cond.Visit("compareVarStr", arg0varuse) - cond.Visit("compareVarVar", arg0varuse) - cond.Visit("compareVarVar", arg2varuse) +func (ind *Indentation) RememberUsedVariables(cond MkCond) { + NewMkCondWalker().Walk(cond, &MkCondCallback{ + Defined: func(varname string) { + ind.AddVar(varname) + }, + Empty: func(varuse *MkVarUse) { + ind.AddVar(varuse.varname) + }, + CompareVarNum: func(varuse *MkVarUse, op string, num string) { + ind.AddVar(varuse.varname) + }, + CompareVarStr: func(varuse *MkVarUse, op string, str string) { + ind.AddVar(varuse.varname) + }, + CompareVarVar: func(left *MkVarUse, op string, right *MkVarUse) { + ind.AddVar(left.varname) + ind.AddVar(right.varname) + }}) } func (mkline *MkLineImpl) ExplainRelativeDirs() { @@ -777,11 +807,23 @@ type Indentation struct { func NewIndentation() *Indentation { ind := &Indentation{} - ind.Push(0, "") // Dummy + ind.Push(nil, 0, "") // Dummy return ind } +func (ind *Indentation) String() string { + s := "" + for _, level := range ind.levels[1:] { + s += fmt.Sprintf(" %d", level.depth) + if len(level.conditionVars) != 0 { + s += " (" + strings.Join(level.conditionVars, " ") + ")" + } + } + return "[" + strings.TrimSpace(s) + "]" +} + type indentationLevel struct { + mkline MkLine // The line in which the indentation started; the .if/.for depth int // Number of space characters; always a multiple of 2 condition string // The corresponding condition from the .if or .elif conditionVars []string // Variables on which the current path depends @@ -814,8 +856,8 @@ func (ind *Indentation) Pop() { ind.levels = ind.levels[:ind.Len()-1] } -func (ind *Indentation) Push(indent int, condition string) { - ind.levels = append(ind.levels, indentationLevel{indent, condition, nil, nil}) +func (ind *Indentation) Push(mkline MkLine, indent int, condition string) { + ind.levels = append(ind.levels, indentationLevel{mkline, indent, condition, nil, nil}) } func (ind *Indentation) AddVar(varname string) { @@ -894,7 +936,7 @@ func (ind *Indentation) TrackBefore(mkline MkLine) { return } if trace.Tracing { - trace.Stepf("Indentation before line %s: %+v", mkline.Linenos(), ind.levels) + trace.Stepf("Indentation before line %s: %s", mkline.Linenos(), ind) } directive := mkline.Directive() @@ -902,7 +944,7 @@ func (ind *Indentation) TrackBefore(mkline MkLine) { switch directive { case "for", "if", "ifdef", "ifndef": - ind.Push(ind.top().depth, args) + ind.Push(mkline, ind.top().depth, args) } } @@ -928,9 +970,14 @@ func (ind *Indentation) TrackAfter(mkline MkLine) { if contains(args, "exists") { cond := NewMkParser(mkline.Line, args, false).MkCond() - cond.Visit("exists", func(node *Tree) { - ind.AddCheckedFile(node.args[0].(string)) - }) + if cond != nil { + NewMkCondWalker().Walk(cond, &MkCondCallback{ + Call: func(name string, arg string) { + if name == "exists" { + ind.AddCheckedFile(arg) + } + }}) + } } case "for", "ifdef", "ifndef": @@ -940,6 +987,12 @@ func (ind *Indentation) TrackAfter(mkline MkLine) { // Handled here instead of TrackAfter to allow the action to access the previous condition. ind.top().condition = args + case "else": + top := ind.top() + if top.mkline != nil { + top.mkline.SetHasElseBranch(mkline) + } + case "endfor", "endif": if ind.Len() > 1 { // Can only be false in unbalanced files. ind.Pop() @@ -947,7 +1000,7 @@ func (ind *Indentation) TrackAfter(mkline MkLine) { } if trace.Tracing { - trace.Stepf("Indentation after line %s: %+v", mkline.Linenos(), ind.levels) + trace.Stepf("Indentation after line %s: %s", mkline.Linenos(), ind) } } diff --git a/pkgtools/pkglint/files/mkline_test.go b/pkgtools/pkglint/files/mkline_test.go index 84e623e814a..602cae0f468 100644 --- a/pkgtools/pkglint/files/mkline_test.go +++ b/pkgtools/pkglint/files/mkline_test.go @@ -876,12 +876,16 @@ func (s *Suite) Test_MatchVarassign(c *check.C) { } func (s *Suite) Test_Indentation(c *check.C) { + t := s.Init(c) + ind := NewIndentation() + mkline := t.NewMkLine("dummy.mk", 5, ".if 0") + c.Check(ind.Depth("if"), equals, 0) c.Check(ind.DependsOn("VARNAME"), equals, false) - ind.Push(2, "") + ind.Push(mkline, 2, "") c.Check(ind.Depth("if"), equals, 0) // Because "if" is handled in MkLines.TrackBefore. c.Check(ind.Depth("endfor"), equals, 0) @@ -896,11 +900,12 @@ func (s *Suite) Test_Indentation(c *check.C) { c.Check(ind.DependsOn("LEVEL1.VAR1"), equals, true) c.Check(ind.DependsOn("OTHER_VAR"), equals, false) - ind.Push(2, "") + ind.Push(mkline, 2, "") ind.AddVar("LEVEL2.VAR") c.Check(ind.Varnames(), equals, "LEVEL1.VAR1, LEVEL1.VAR2, LEVEL2.VAR") + c.Check(ind.String(), equals, "[2 (LEVEL1.VAR1 LEVEL1.VAR2) 2 (LEVEL2.VAR)]") ind.Pop() @@ -911,4 +916,5 @@ func (s *Suite) Test_Indentation(c *check.C) { c.Check(ind.Varnames(), equals, "") c.Check(ind.IsConditional(), equals, false) + c.Check(ind.String(), equals, "[]") } diff --git a/pkgtools/pkglint/files/mklinechecker.go b/pkgtools/pkglint/files/mklinechecker.go index 8731a9cf1ac..c5ca6fd8efc 100644 --- a/pkgtools/pkglint/files/mklinechecker.go +++ b/pkgtools/pkglint/files/mklinechecker.go @@ -257,7 +257,7 @@ func (ck MkLineChecker) checkDependencyRule(allowedTargets map[string]bool) { } func (ck MkLineChecker) checkVarassignDefPermissions() { - if !G.opts.WarnPerm { + if !G.opts.WarnPerm || G.Infrastructure { return } if trace.Tracing { @@ -941,6 +941,18 @@ func (ck MkLineChecker) CheckVartype(varname string, op MkOperator, value, comme case vartype.kindOfList == lkNone: ck.CheckVartypePrimitive(varname, vartype.basicType, op, value, comment, vartype.guessed) + if op == opUseMatch && matches(value, `^[\w-/]+$`) && !vartype.IsConsideredList() { + mkline.Notef("%s should be compared using == instead of the :M or :N modifier without wildcards.", varname) + Explain( + "This variable has a single value, not a list of values. Therefore", + "it feels strange to apply list operators like :M and :N onto it.", + "A more direct approach is to use the == and != operators.", + "", + "An entirely different case is when the pattern contains wildcards", + "like ^, *, $. In such a case, using the :M or :N modifiers is", + "useful and preferred.") + } + case value == "": break @@ -1031,8 +1043,7 @@ func (ck MkLineChecker) CheckCond() { return } - cond.Visit("empty", func(node *Tree) { - varuse := node.args[0].(MkVarUse) + checkEmpty := func(varuse *MkVarUse) { varname := varuse.varname if matches(varname, `^\$.*:[MN]`) { mkline.Warnf("The empty() function takes a variable name as parameter, not a variable expression.") @@ -1054,19 +1065,21 @@ func (ck MkLineChecker) CheckCond() { ck.CheckVartype(varname, opUseMatch, modifier[1:], "") } } - }) + } - cond.Visit("compareVarStr", func(node *Tree) { - varuse := node.args[0].(MkVarUse) + checkCompareVarStr := func(varuse *MkVarUse, op string, value string) { varname := varuse.varname varmods := varuse.modifiers - value := node.args[2].(string) if len(varmods) == 0 { - ck.checkCompareVarStr(varname, node.args[1].(string), value) + ck.checkCompareVarStr(varname, op, value) } else if len(varmods) == 1 && matches(varmods[0], `^[MN]`) && value != "" { ck.CheckVartype(varname, opUseMatch, value, "") } - }) + } + + NewMkCondWalker().Walk(cond, &MkCondCallback{ + Empty: checkEmpty, + CompareVarStr: checkCompareVarStr}) if G.Mk != nil { G.Mk.indentation.RememberUsedVariables(cond) @@ -1120,24 +1133,24 @@ func (ck MkLineChecker) CheckRelativePkgdir(pkgdir string) { } } -func (ck MkLineChecker) CheckRelativePath(path string, mustExist bool) { +func (ck MkLineChecker) CheckRelativePath(relativePath string, mustExist bool) { if trace.Tracing { - defer trace.Call(path, mustExist)() + defer trace.Call(relativePath, mustExist)() } mkline := ck.MkLine - if !G.Wip && contains(path, "/wip/") { + if !G.Wip && contains(relativePath, "/wip/") { mkline.Errorf("A main pkgsrc package must not depend on a pkgsrc-wip package.") } - resolvedPath := mkline.ResolveVarsInRelativePath(path, true) + resolvedPath := mkline.ResolveVarsInRelativePath(relativePath, true) if containsVarRef(resolvedPath) { return } abs := resolvedPath if !hasPrefix(abs, "/") { - abs = G.CurrentDir + "/" + abs + abs = path.Dir(mkline.Filename) + "/" + abs } if _, err := os.Stat(abs); err != nil { if mustExist { @@ -1146,10 +1159,15 @@ func (ck MkLineChecker) CheckRelativePath(path string, mustExist bool) { return } - if hasPrefix(path, "../") && - !matches(path, `^\.\./\.\./[^/]+/[^/]`) && - !(G.CurPkgsrcdir == ".." && hasPrefix(path, "../mk/")) && // For category Makefiles. - !hasPrefix(path, "../../mk/") { - mkline.Warnf("Invalid relative path %q.", path) + switch { + case !hasPrefix(relativePath, "../"): + case matches(relativePath, `^\.\./\.\./[^/]+/[^/]`): + // From a package to another package. + case hasPrefix(relativePath, "../../mk/"): + // From a package to the infrastructure. + case hasPrefix(relativePath, "../mk/") && relpath(path.Dir(mkline.Filename), G.Pkgsrc.File(".")) == "..": + // For category Makefiles. + default: + mkline.Warnf("Invalid relative path %q.", relativePath) } } diff --git a/pkgtools/pkglint/files/mklinechecker_test.go b/pkgtools/pkglint/files/mklinechecker_test.go index 051b397aeb9..94612842bb3 100644 --- a/pkgtools/pkglint/files/mklinechecker_test.go +++ b/pkgtools/pkglint/files/mklinechecker_test.go @@ -103,14 +103,15 @@ func (s *Suite) Test_MkLineChecker_Check__conditions(c *check.C) { MkLineChecker{t.NewMkLine("fname", 1, ".if ${EMUL_PLATFORM:Mlinux-x386}")}.CheckCond() t.CheckOutputLines( - "WARN: fname:1: " + - "The pattern \"x386\" cannot match any of { aarch64 aarch64eb alpha amd64 arc arm arm26 " + - "arm32 cobalt coldfire convex dreamcast earm earmeb earmhf earmhfeb earmv4 earmv4eb " + - "earmv5 earmv5eb earmv6 earmv6eb earmv6hf earmv6hfeb earmv7 earmv7eb earmv7hf " + - "earmv7hfeb evbarm hpcmips hpcsh hppa hppa64 i386 i586 i686 ia64 m68000 m68k m88k " + - "mips mips64 mips64eb mips64el mipseb mipsel mipsn32 mlrisc ns32k pc532 pmax powerpc powerpc64 " + - "rs6000 s390 sh3eb sh3el sparc sparc64 vax x86_64 } " + - "for the hardware architecture part of EMUL_PLATFORM.") + "WARN: fname:1: "+ + "The pattern \"x386\" cannot match any of { aarch64 aarch64eb alpha amd64 arc arm arm26 "+ + "arm32 cobalt coldfire convex dreamcast earm earmeb earmhf earmhfeb earmv4 earmv4eb "+ + "earmv5 earmv5eb earmv6 earmv6eb earmv6hf earmv6hfeb earmv7 earmv7eb earmv7hf "+ + "earmv7hfeb evbarm hpcmips hpcsh hppa hppa64 i386 i586 i686 ia64 m68000 m68k m88k "+ + "mips mips64 mips64eb mips64el mipseb mipsel mipsn32 mlrisc ns32k pc532 pmax powerpc powerpc64 "+ + "rs6000 s390 sh3eb sh3el sparc sparc64 vax x86_64 } "+ + "for the hardware architecture part of EMUL_PLATFORM.", + "NOTE: fname:1: EMUL_PLATFORM should be compared using == instead of the :M or :N modifier without wildcards.") MkLineChecker{t.NewMkLine("fname", 98, ".if ${MACHINE_PLATFORM:MUnknownOS-*-*} || ${MACHINE_ARCH:Mx86}")}.CheckCond() @@ -127,7 +128,8 @@ func (s *Suite) Test_MkLineChecker_Check__conditions(c *check.C) { "earmv7 earmv7eb earmv7hf earmv7hfeb evbarm hpcmips hpcsh hppa hppa64 i386 i586 i686 ia64 "+ "m68000 m68k m88k mips mips64 mips64eb mips64el mipseb mipsel mipsn32 mlrisc ns32k pc532 pmax "+ "powerpc powerpc64 rs6000 s390 sh3eb sh3el sparc sparc64 vax x86_64 "+ - "} for MACHINE_ARCH.") + "} for MACHINE_ARCH.", + "NOTE: fname:98: MACHINE_ARCH should be compared using == instead of the :M or :N modifier without wildcards.") } func (s *Suite) Test_MkLineChecker_checkVarassign(c *check.C) { @@ -158,6 +160,23 @@ func (s *Suite) Test_MkLineChecker_checkVarassignDefPermissions(c *check.C) { "WARN: options.mk:2: The variable PKG_DEVELOPER may not be given a default value by any package.") } +// Don't check the permissions for infrastructure files since they have their own rules. +func (s *Suite) Test_MkLineChecker_checkVarassignDefPermissions__infrastructure(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wall") + t.SetupVartypes() + t.SetupFileMkLines("mk/infra.mk", + MkRcsID, + "", + "PKG_DEVELOPER?=\tyes") + t.SetupFileMkLines("mk/bsd.pkg.mk") + + G.CheckDirent(t.File("mk/infra.mk")) + + t.CheckOutputEmpty() +} + func (s *Suite) Test_MkLineChecker_CheckVarusePermissions(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/mklines.go b/pkgtools/pkglint/files/mklines.go index f2e599e4ed3..f935736eaff 100644 --- a/pkgtools/pkglint/files/mklines.go +++ b/pkgtools/pkglint/files/mklines.go @@ -13,10 +13,12 @@ type MkLines struct { forVars map[string]bool // The variables currently used in .for loops target string // Current make(1) target vars Scope - buildDefs map[string]bool // Variables that are registered in BUILD_DEFS, to ensure that all user-defined variables are added to it. - plistVars map[string]bool // Variables that are registered in PLIST_VARS, to ensure that all user-defined variables are added to it. - tools map[string]bool // Set of tools that are declared to be used. - toolRegistry ToolRegistry // Tools defined in file scope. + buildDefs map[string]bool // Variables that are registered in BUILD_DEFS, to ensure that all user-defined variables are added to it. + plistVarAdded map[string]MkLine // Identifiers that are added to PLIST_VARS. + plistVarSet map[string]MkLine // Identifiers for which PLIST.${id} is defined. + plistVarSkip bool // True if any of the PLIST_VARS identifiers refers to a variable. + tools map[string]bool // Set of tools that are declared to be used. + toolRegistry ToolRegistry // Tools defined in file scope. SeenBsdPrefsMk bool indentation *Indentation // Indentation depth of preprocessing directives; only available during MkLines.ForEach. Once @@ -42,7 +44,9 @@ func NewMkLines(lines []Line) *MkLines { "", NewScope(), make(map[string]bool), - make(map[string]bool), + make(map[string]MkLine), + make(map[string]MkLine), + false, tools, NewToolRegistry(), false, @@ -85,6 +89,8 @@ func (mklines *MkLines) Check() { // are collected to make the order of the definitions irrelevant. mklines.DetermineUsedVariables() mklines.DetermineDefinedVariables() + mklines.collectPlistVars() + mklines.collectElse() // In the second pass, the actual checks are done. @@ -105,9 +111,25 @@ func (mklines *MkLines) Check() { case mkline.IsVarassign(): mklines.target = "" - mkline.Tokenize(mkline.Value()) + mkline.Tokenize(mkline.Value()) // Just for the side-effect of the warning. substcontext.Varassign(mkline) + switch mkline.Varcanon() { + case "PLIST_VARS": + value := mkline.ValueSplit(resolveVariableRefs(mkline.Value()), "") + for _, id := range value { + if !mklines.plistVarSkip && mklines.plistVarSet[id] == nil { + mkline.Warnf("%q is added to PLIST_VARS, but PLIST.%s is not defined in this file.", id, id) + } + } + + case "PLIST.*": + id := mkline.Varparam() + if !mklines.plistVarSkip && mklines.plistVarAdded[id] == nil { + mkline.Warnf("PLIST.%s is defined, but %q is not added to PLIST_VARS in this file.", id, id) + } + } + case mkline.IsInclude(): mklines.target = "" switch path.Base(mkline.IncludeFile()) { @@ -177,6 +199,8 @@ func (mklines *MkLines) DetermineDefinedVariables() { continue } + defineVar(mkline, mkline.Varname()) + varcanon := mkline.Varcanon() switch varcanon { case "BUILD_DEFS", "PKG_GROUPS_VARS", "PKG_USERS_VARS": @@ -188,12 +212,17 @@ func (mklines *MkLines) DetermineDefinedVariables() { } case "PLIST_VARS": - for _, id := range splitOnSpace(mkline.Value()) { - mklines.plistVars["PLIST."+id] = true + value := mkline.ValueSplit(resolveVariableRefs(mkline.Value()), "") + for _, id := range value { if trace.Tracing { trace.Step1("PLIST.%s is added to PLIST_VARS.", id) } - mklines.UseVar(mkline, "PLIST."+id) + if containsVarRef(id) { + mklines.UseVar(mkline, "PLIST.*") + mklines.plistVarSkip = true + } else { + mklines.UseVar(mkline, "PLIST."+id) + } } case "USE_TOOLS": @@ -231,6 +260,38 @@ func (mklines *MkLines) DetermineDefinedVariables() { } } +func (mklines *MkLines) collectPlistVars() { + for _, mkline := range mklines.mklines { + if mkline.IsVarassign() { + switch mkline.Varcanon() { + case "PLIST_VARS": + value := mkline.ValueSplit(resolveVariableRefs(mkline.Value()), "") + for _, id := range value { + if containsVarRef(id) { + mklines.plistVarSkip = true + } else { + mklines.plistVarAdded[id] = mkline + } + } + case "PLIST.*": + id := mkline.Varparam() + if containsVarRef(id) { + mklines.plistVarSkip = true + } else { + mklines.plistVarSet[id] = mkline + } + } + } + } +} + +func (mklines *MkLines) collectElse() { + // Make a dry-run over the lines, which sets data.elseLine (in mkline.go) as a side-effect. + mklines.ForEach( + func(mkline MkLine) bool { return true }, + func(mkline MkLine) {}) +} + func (mklines *MkLines) DetermineUsedVariables() { for _, mkline := range mklines.mklines { varnames := mkline.DetermineUsedVariables() diff --git a/pkgtools/pkglint/files/mklines_test.go b/pkgtools/pkglint/files/mklines_test.go index 4a4729a8428..204d5cb945a 100644 --- a/pkgtools/pkglint/files/mklines_test.go +++ b/pkgtools/pkglint/files/mklines_test.go @@ -55,13 +55,13 @@ func (s *Suite) Test_MkLines_Check__unusual_target(c *check.C) { func (s *Suite) Test_MkLineChecker_checkInclude__Makefile(c *check.C) { t := s.Init(c) - mkline := t.NewMkLine("Makefile", 2, ".include \"../../other/package/Makefile\"") + mkline := t.NewMkLine(t.File("Makefile"), 2, ".include \"../../other/package/Makefile\"") MkLineChecker{mkline}.checkInclude() t.CheckOutputLines( - "ERROR: Makefile:2: \"/other/package/Makefile\" does not exist.", - "ERROR: Makefile:2: Other Makefiles must not be included directly.") + "ERROR: ~/Makefile:2: \"other/package/Makefile\" does not exist.", + "ERROR: ~/Makefile:2: Other Makefiles must not be included directly.") } func (s *Suite) Test_MkLines_quoting_LDFLAGS_for_GNU_configure(c *check.C) { @@ -453,14 +453,17 @@ func (s *Suite) Test_MkLines_Check__endif_comment(c *check.C) { "WARN: opsys.mk:20: Comment \"NetBSD\" does not match condition \"${OPSYS} == FreeBSD\".") } -// Demonstrates how to define your own make(1) targets. +// Demonstrates how to define your own make(1) targets for creating +// files in the current directory. The pkgsrc-wip category Makefile +// does this, while all other categories don't need any custom code. func (s *Suite) Test_MkLines_wip_category_Makefile(c *check.C) { t := s.Init(c) - t.SetupCommandLine("-Wall") + t.SetupCommandLine("-Wall", "--explain") t.SetupVartypes() t.SetupTool(&Tool{Name: "rm", Varname: "RM", Predefined: true}) - mklines := t.NewMkLines("Makefile", + t.CreateFileLines("mk/misc/category.mk") + mklines := t.SetupFileMkLines("wip/Makefile", MkRcsID, "", "COMMENT=\tWIP pkgsrc packages", @@ -474,12 +477,25 @@ func (s *Suite) Test_MkLines_wip_category_Makefile(c *check.C) { "${.CURDIR}/INDEX:", "\t${RM} -f ${.CURDIR}/INDEX", "", - ".include \"../../mk/misc/category.mk\"") + "clean-tmpdir:", + "\t${RUN} rm -rf tmpdir", + "", + ".include \"../mk/misc/category.mk\"") mklines.Check() t.CheckOutputLines( - "ERROR: Makefile:14: \"/mk/misc/category.mk\" does not exist.") + "WARN: ~/wip/Makefile:14: Unusual target \"clean-tmpdir\".", + "", + "\tIf you want to define your own target, declare it like this:", + "\t", + "\t\t.PHONY: my-target", + "\t", + "\tIn the rare case that you actually want a file-based make(1)", + "\ttarget, write it like this:", + "\t", + "\t\t${.CURDIR}/my-filename:", + "") } func (s *Suite) Test_MkLines_ExtractDocumentedVariables(c *check.C) { @@ -622,3 +638,150 @@ func (s *Suite) Test_MkLines_CheckRedundantVariables__procedure_call(c *check.C) t.CheckOutputEmpty() } + +func (s *Suite) Test_MkLines_Check__PLIST_VARS(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wno-space") + t.SetupVartypes() + t.SetupOption("both", "") + t.SetupOption("only-added", "") + t.SetupOption("only-defined", "") + + mklines := t.SetupFileMkLines("options.mk", + MkRcsID, + "", + "PKG_OPTIONS_VAR= PKG_OPTIONS.pkg", + "PKG_SUPPORTED_OPTIONS= both only-added only-defined", + "PKG_SUGGESTED_OPTIONS= # none", + "", + ".include \"../../mk/bsd.options.mk\"", + "", + "PLIST_VARS+= both only-added", + "", + ".if !empty(PKG_OPTIONS:Mboth)", + "PLIST.both= yes", + ".endif", + "", + ".if !empty(PKG_OPTIONS:Monly-defined)", + "PLIST.only-defined= yes", + ".endif") + + mklines.Check() + + t.CheckOutputLines( + "ERROR: ~/options.mk:7: \"mk/bsd.options.mk\" does not exist.", // Not relevant for this test. + "WARN: ~/options.mk:9: \"only-added\" is added to PLIST_VARS, but PLIST.only-added is not defined in this file.", + "WARN: ~/options.mk:16: PLIST.only-defined is defined, but \"only-defined\" is not added to PLIST_VARS in this file.") +} + +func (s *Suite) Test_MkLines_Check__PLIST_VARS_indirect(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wno-space") + t.SetupVartypes() + t.SetupOption("option1", "") + t.SetupOption("option2", "") + + mklines := t.SetupFileMkLines("module.mk", + MkRcsID, + "", + "MY_PLIST_VARS= option1 option2", + "PLIST_VARS+= ${MY_PLIST_VARS}", + ".for option in option3", + "PLIST_VARS+= ${option}", + ".endfor", + "", + ".if 0", + "PLIST.option1= yes", + ".endif", + "", + ".if 1", + "PLIST.option2= yes", + ".endif") + + mklines.Check() + + t.CheckOutputEmpty() +} + +func (s *Suite) Test_MkLines_Check__if_else(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wno-space") + t.SetupVartypes() + + mklines := t.NewMkLines("module.mk", + MkRcsID, + "", + ".if 0", + ".endif", + "", + ".if 0", + ".else", + ".endif", + "", + ".if 0", + ".elif 0", + ".endif") + + mklines.collectElse() + + c.Check(mklines.mklines[2].HasElseBranch(), equals, false) + c.Check(mklines.mklines[5].HasElseBranch(), equals, true) + c.Check(mklines.mklines[9].HasElseBranch(), equals, false) +} + +func (s *Suite) Test_MkLines_Check__defined_and_used_variables(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wno-space") + t.SetupVartypes() + + mklines := t.NewMkLines("module.mk", + MkRcsID, + "", + ".for lang in de fr", + "PLIST_VARS+= ${lang}", + ".endif", + "", + ".for language in de fr", + "PLIST.${language}= yes", + ".endif", + "", + "PLIST.other= yes") + + mklines.Check() + + // If there are variable involved in the definition of PLIST_VARS or PLIST.*, + // it becomes too difficult for pkglint to decide whether the IDs can still match. + // Therefore, in such a case, no diagnostics are logged at all. + t.CheckOutputEmpty() +} + +func (s *Suite) Test_MkLines_Check__indirect_PLIST_VARS(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wno-space") + t.SetupVartypes() + t.SetupOption("a", "") + t.SetupOption("b", "") + t.SetupOption("c", "") + + mklines := t.NewMkLines("module.mk", + MkRcsID, + "", + "PKG_SUPPORTED_OPTIONS= a b c", + "PLIST_VARS+= ${PKG_SUPPORTED_OPTIONS:S,a,,g}", + "", + "PLIST_VARS+= only-added", + "", + "PLIST.only-defined= yes") + + mklines.Check() + + // If the PLIST_VARS contain complex expressions that involve other variables, + // it becomes too difficult for pkglint to decide whether the IDs can still match. + // Therefore, in such a case, no diagnostics are logged at all. + t.CheckOutputEmpty() +} diff --git a/pkgtools/pkglint/files/mkparser.go b/pkgtools/pkglint/files/mkparser.go index dcfd2a77411..7ddffba3f29 100644 --- a/pkgtools/pkglint/files/mkparser.go +++ b/pkgtools/pkglint/files/mkparser.go @@ -203,17 +203,13 @@ func (p *MkParser) VarUseModifiers(varname, closing string) []string { // MkCond parses a condition like ${OPSYS} == "NetBSD". // See devel/bmake/files/cond.c. -func (p *MkParser) MkCond() *Tree { - return p.mkCondOr() -} - -func (p *MkParser) mkCondOr() *Tree { +func (p *MkParser) MkCond() MkCond { and := p.mkCondAnd() if and == nil { return nil } - ands := append([]interface{}(nil), and) + ands := []MkCond{and} for { mark := p.repl.Mark() if !p.repl.AdvanceRegexp(`^\s*\|\|\s*`) { @@ -229,16 +225,16 @@ func (p *MkParser) mkCondOr() *Tree { if len(ands) == 1 { return and } - return NewTree("or", ands...) + return &mkCond{Or: ands} } -func (p *MkParser) mkCondAnd() *Tree { +func (p *MkParser) mkCondAnd() MkCond { atom := p.mkCondAtom() if atom == nil { return nil } - atoms := append([]interface{}(nil), atom) + atoms := []MkCond{atom} for { mark := p.repl.Mark() if !p.repl.AdvanceRegexp(`^\s*&&\s*`) { @@ -254,10 +250,10 @@ func (p *MkParser) mkCondAnd() *Tree { if len(atoms) == 1 { return atom } - return NewTree("and", atoms...) + return &mkCond{And: atoms} } -func (p *MkParser) mkCondAtom() *Tree { +func (p *MkParser) mkCondAtom() MkCond { if trace.Tracing { defer trace.Call1(p.Rest())() } @@ -269,7 +265,7 @@ func (p *MkParser) mkCondAtom() *Tree { case repl.AdvanceStr("!"): cond := p.mkCondAtom() if cond != nil { - return NewTree("not", cond) + return &mkCond{Not: cond} } case repl.AdvanceStr("("): cond := p.MkCond() @@ -282,14 +278,14 @@ func (p *MkParser) mkCondAtom() *Tree { case repl.AdvanceRegexp(`^defined\s*\(`): if varname := p.Varname(); varname != "" { if repl.AdvanceStr(")") { - return NewTree("defined", varname) + return &mkCond{Defined: varname} } } case repl.AdvanceRegexp(`^empty\s*\(`): if varname := p.Varname(); varname != "" { modifiers := p.VarUseModifiers(varname, ")") if repl.AdvanceStr(")") { - return NewTree("empty", MkVarUse{varname, modifiers}) + return &mkCond{Empty: &MkVarUse{varname, modifiers}} } } case repl.AdvanceRegexp(`^(commands|exists|make|target)\s*\(`): @@ -299,7 +295,7 @@ func (p *MkParser) mkCondAtom() *Tree { } arg := repl.Since(argMark) if repl.AdvanceStr(")") { - return NewTree(funcname, arg) + return &mkCond{Call: &MkCondCall{funcname, arg}} } default: lhs := p.VarUse() @@ -313,33 +309,33 @@ func (p *MkParser) mkCondAtom() *Tree { } if lhs != nil { if repl.AdvanceRegexp(`^\s*(<|<=|==|!=|>=|>)\s*(\d+(?:\.\d+)?)`) { - return NewTree("compareVarNum", *lhs, repl.Group(1), repl.Group(2)) + return &mkCond{CompareVarNum: &MkCondCompareVarNum{lhs, repl.Group(1), repl.Group(2)}} } if repl.AdvanceRegexp(`^\s*(<|<=|==|!=|>=|>)\s*`) { op := repl.Group(1) if (op == "!=" || op == "==") && repl.AdvanceRegexp(`^"([^"\$\\]*)"`) { - return NewTree("compareVarStr", *lhs, op, repl.Group(1)) + return &mkCond{CompareVarStr: &MkCondCompareVarStr{lhs, op, repl.Group(1)}} } else if repl.AdvanceRegexp(`^\w+`) { - return NewTree("compareVarStr", *lhs, op, repl.Group(0)) + return &mkCond{CompareVarStr: &MkCondCompareVarStr{lhs, op, repl.Group(0)}} } else if rhs := p.VarUse(); rhs != nil { - return NewTree("compareVarVar", *lhs, op, *rhs) + return &mkCond{CompareVarVar: &MkCondCompareVarVar{lhs, op, rhs}} } else if repl.PeekByte() == '"' { mark := repl.Mark() if repl.AdvanceStr("\"") { if quotedRHS := p.VarUse(); quotedRHS != nil { if repl.AdvanceStr("\"") { - return NewTree("compareVarVar", *lhs, op, *quotedRHS) + return &mkCond{CompareVarVar: &MkCondCompareVarVar{lhs, op, quotedRHS}} } } } repl.Reset(mark) } } else { - return NewTree("not", NewTree("empty", *lhs)) // See devel/bmake/files/cond.c:/\* For \.if \$/ + return &mkCond{Not: &mkCond{Empty: lhs}} // See devel/bmake/files/cond.c:/\* For \.if \$/ } } if repl.AdvanceRegexp(`^\d+(?:\.\d+)?`) { - return NewTree("literalNum", repl.Group(0)) + return &mkCond{Num: repl.Group(0)} } } repl.Reset(mark) @@ -358,3 +354,95 @@ func (p *MkParser) Varname() string { } return repl.Since(mark) } + +type MkCond = *mkCond + +type mkCond struct { + Or []*mkCond + And []*mkCond + Not *mkCond + + Defined string + Empty *MkVarUse + CompareVarNum *MkCondCompareVarNum + CompareVarStr *MkCondCompareVarStr + CompareVarVar *MkCondCompareVarVar + Call *MkCondCall + Num string +} +type MkCondCompareVarNum struct { + Var *MkVarUse + Op string // One of <, <=, ==, !=, >=, >. + Num string +} +type MkCondCompareVarStr struct { + Var *MkVarUse + Op string // One of ==, !=. + Str string +} +type MkCondCompareVarVar struct { + Left *MkVarUse + Op string // One of <, <=, ==, !=, >=, >. + Right *MkVarUse +} +type MkCondCall struct { + Name string + Arg string +} + +type MkCondCallback struct { + Defined func(varname string) + Empty func(empty *MkVarUse) + CompareVarNum func(varuse *MkVarUse, op string, num string) + CompareVarStr func(varuse *MkVarUse, op string, str string) + CompareVarVar func(left *MkVarUse, op string, right *MkVarUse) + Call func(name string, arg string) +} + +type MkCondWalker struct{} + +func NewMkCondWalker() *MkCondWalker { return &MkCondWalker{} } + +func (w *MkCondWalker) Walk(cond MkCond, callback *MkCondCallback) { + switch { + case cond.Or != nil: + for _, or := range cond.Or { + w.Walk(or, callback) + } + case cond.And != nil: + for _, and := range cond.And { + w.Walk(and, callback) + } + case cond.Not != nil: + w.Walk(cond.Not, callback) + + case cond.Defined != "": + if callback.Defined != nil { + callback.Defined(cond.Defined) + } + case cond.Empty != nil: + if callback.Empty != nil { + callback.Empty(cond.Empty) + } + case cond.CompareVarVar != nil: + if callback.CompareVarVar != nil { + cvv := cond.CompareVarVar + callback.CompareVarVar(cvv.Left, cvv.Op, cvv.Right) + } + case cond.CompareVarStr != nil: + if callback.CompareVarStr != nil { + cvs := cond.CompareVarStr + callback.CompareVarStr(cvs.Var, cvs.Op, cvs.Str) + } + case cond.CompareVarNum != nil: + if callback.CompareVarNum != nil { + cvn := cond.CompareVarNum + callback.CompareVarNum(cvn.Var, cvn.Op, cvn.Num) + } + case cond.Call != nil: + if callback.Call != nil { + call := cond.Call + callback.Call(call.Name, call.Arg) + } + } +} diff --git a/pkgtools/pkglint/files/mkparser_test.go b/pkgtools/pkglint/files/mkparser_test.go index dd0f22a69a6..fbb46ef12b7 100644 --- a/pkgtools/pkglint/files/mkparser_test.go +++ b/pkgtools/pkglint/files/mkparser_test.go @@ -135,87 +135,92 @@ func (s *Suite) Test_MkParser_MkTokens(c *check.C) { } func (s *Suite) Test_MkParser_MkCond(c *check.C) { - checkRest := func(input string, expectedTree *Tree, expectedRest string) { + checkRest := func(input string, expectedTree MkCond, expectedRest string) { p := NewMkParser(dummyLine, input, false) actualTree := p.MkCond() c.Check(actualTree, deepEquals, expectedTree) c.Check(p.Rest(), equals, expectedRest) } - check := func(input string, expectedTree *Tree) { + check := func(input string, expectedTree MkCond) { checkRest(input, expectedTree, "") } - varuse := func(varname string, modifiers ...string) MkVarUse { - return MkVarUse{varname: varname, modifiers: modifiers} + varuse := func(varname string, modifiers ...string) *MkVarUse { + return &MkVarUse{varname: varname, modifiers: modifiers} } check("${OPSYS:MNetBSD}", - NewTree("not", NewTree("empty", varuse("OPSYS", "MNetBSD")))) + &mkCond{Not: &mkCond{Empty: varuse("OPSYS", "MNetBSD")}}) check("defined(VARNAME)", - NewTree("defined", "VARNAME")) + &mkCond{Defined: "VARNAME"}) check("empty(VARNAME)", - NewTree("empty", varuse("VARNAME"))) + &mkCond{Empty: varuse("VARNAME")}) check("!empty(VARNAME)", - NewTree("not", NewTree("empty", varuse("VARNAME")))) + &mkCond{Not: &mkCond{Empty: varuse("VARNAME")}}) check("!empty(VARNAME:M[yY][eE][sS])", - NewTree("not", NewTree("empty", varuse("VARNAME", "M[yY][eE][sS]")))) + &mkCond{Not: &mkCond{Empty: varuse("VARNAME", "M[yY][eE][sS]")}}) check("${VARNAME} != \"Value\"", - NewTree("compareVarStr", varuse("VARNAME"), "!=", "Value")) + &mkCond{CompareVarStr: &MkCondCompareVarStr{varuse("VARNAME"), "!=", "Value"}}) check("${VARNAME:Mi386} != \"Value\"", - NewTree("compareVarStr", varuse("VARNAME", "Mi386"), "!=", "Value")) + &mkCond{CompareVarStr: &MkCondCompareVarStr{varuse("VARNAME", "Mi386"), "!=", "Value"}}) check("${VARNAME} != Value", - NewTree("compareVarStr", varuse("VARNAME"), "!=", "Value")) + &mkCond{CompareVarStr: &MkCondCompareVarStr{varuse("VARNAME"), "!=", "Value"}}) check("\"${VARNAME}\" != Value", - NewTree("compareVarStr", varuse("VARNAME"), "!=", "Value")) + &mkCond{CompareVarStr: &MkCondCompareVarStr{varuse("VARNAME"), "!=", "Value"}}) check("${pkg} == \"${name}\"", - NewTree("compareVarVar", varuse("pkg"), "==", varuse("name"))) + &mkCond{CompareVarVar: &MkCondCompareVarVar{varuse("pkg"), "==", varuse("name")}}) check("\"${pkg}\" == \"${name}\"", - NewTree("compareVarVar", varuse("pkg"), "==", varuse("name"))) + &mkCond{CompareVarVar: &MkCondCompareVarVar{varuse("pkg"), "==", varuse("name")}}) check("(defined(VARNAME))", - NewTree("defined", "VARNAME")) + &mkCond{Defined: "VARNAME"}) check("exists(/etc/hosts)", - NewTree("exists", "/etc/hosts")) + &mkCond{Call: &MkCondCall{"exists", "/etc/hosts"}}) check("exists(${PREFIX}/var)", - NewTree("exists", "${PREFIX}/var")) + &mkCond{Call: &MkCondCall{"exists", "${PREFIX}/var"}}) check("${OPSYS} == \"NetBSD\" || ${OPSYS} == \"OpenBSD\"", - NewTree("or", - NewTree("compareVarStr", varuse("OPSYS"), "==", "NetBSD"), - NewTree("compareVarStr", varuse("OPSYS"), "==", "OpenBSD"))) + &mkCond{Or: []*mkCond{ + {CompareVarStr: &MkCondCompareVarStr{varuse("OPSYS"), "==", "NetBSD"}}, + {CompareVarStr: &MkCondCompareVarStr{varuse("OPSYS"), "==", "OpenBSD"}}}}) check("${OPSYS} == \"NetBSD\" && ${MACHINE_ARCH} == \"i386\"", - NewTree("and", - NewTree("compareVarStr", varuse("OPSYS"), "==", "NetBSD"), - NewTree("compareVarStr", varuse("MACHINE_ARCH"), "==", "i386"))) + &mkCond{And: []*mkCond{ + {CompareVarStr: &MkCondCompareVarStr{varuse("OPSYS"), "==", "NetBSD"}}, + {CompareVarStr: &MkCondCompareVarStr{varuse("MACHINE_ARCH"), "==", "i386"}}}}) check("defined(A) && defined(B) || defined(C) && defined(D)", - NewTree("or", - NewTree("and", - NewTree("defined", "A"), - NewTree("defined", "B")), - NewTree("and", - NewTree("defined", "C"), - NewTree("defined", "D")))) + &mkCond{Or: []*mkCond{ + {And: []*mkCond{ + {Defined: "A"}, + {Defined: "B"}}}, + {And: []*mkCond{ + {Defined: "C"}, + {Defined: "D"}}}}}) check("${MACHINE_ARCH:Mi386} || ${MACHINE_OPSYS:MNetBSD}", - NewTree("or", - NewTree("not", NewTree("empty", varuse("MACHINE_ARCH", "Mi386"))), - NewTree("not", NewTree("empty", varuse("MACHINE_OPSYS", "MNetBSD"))))) + &mkCond{Or: []*mkCond{ + {Not: &mkCond{Empty: varuse("MACHINE_ARCH", "Mi386")}}, + {Not: &mkCond{Empty: varuse("MACHINE_OPSYS", "MNetBSD")}}}}) // Exotic cases check("0", - NewTree("literalNum", "0")) + &mkCond{Num: "0"}) check("! ( defined(A) && empty(VARNAME) )", - NewTree("not", NewTree("and", NewTree("defined", "A"), NewTree("empty", varuse("VARNAME"))))) + &mkCond{Not: &mkCond{ + And: []*mkCond{ + {Defined: "A"}, + {Empty: varuse("VARNAME")}}}}) check("${REQD_MAJOR} > ${MAJOR}", - NewTree("compareVarVar", varuse("REQD_MAJOR"), ">", varuse("MAJOR"))) + &mkCond{CompareVarVar: &MkCondCompareVarVar{varuse("REQD_MAJOR"), ">", varuse("MAJOR")}}) check("${OS_VERSION} >= 6.5", - NewTree("compareVarNum", varuse("OS_VERSION"), ">=", "6.5")) + &mkCond{CompareVarNum: &MkCondCompareVarNum{varuse("OS_VERSION"), ">=", "6.5"}}) check("${OS_VERSION} == 5.3", - NewTree("compareVarNum", varuse("OS_VERSION"), "==", "5.3")) + &mkCond{CompareVarNum: &MkCondCompareVarNum{varuse("OS_VERSION"), "==", "5.3"}}) check("!empty(${OS_VARIANT:MIllumos})", // Probably not intended - NewTree("not", NewTree("empty", varuse("${OS_VARIANT:MIllumos}")))) + &mkCond{Not: &mkCond{Empty: varuse("${OS_VARIANT:MIllumos}")}}) check("defined (VARNAME)", // There may be whitespace before the parenthesis; see devel/bmake/files/cond.c:^compare_function. - NewTree("defined", "VARNAME")) + &mkCond{Defined: "VARNAME"}) + check("${\"${PKG_OPTIONS:Moption}\":?--enable-option:--disable-option}", + &mkCond{Not: &mkCond{Empty: varuse("\"${PKG_OPTIONS:Moption}\"", "?--enable-option:--disable-option")}}) // Errors checkRest("!empty(PKG_OPTIONS:Msndfile) || defined(PKG_OPTIONS:Msamplerate)", - NewTree("not", NewTree("empty", varuse("PKG_OPTIONS", "Msndfile"))), + &mkCond{Not: &mkCond{Empty: varuse("PKG_OPTIONS", "Msndfile")}}, " || defined(PKG_OPTIONS:Msamplerate)") } diff --git a/pkgtools/pkglint/files/mkshwalker.go b/pkgtools/pkglint/files/mkshwalker.go index 075cea2095f..d32a5406ff3 100644 --- a/pkgtools/pkglint/files/mkshwalker.go +++ b/pkgtools/pkglint/files/mkshwalker.go @@ -3,153 +3,209 @@ package main type MkShWalker struct { } -// Walk calls the given callback for each node of the parsed shell program. -// See the types in mkshtypes.go for possible node types. -func (w *MkShWalker) Walk(list *MkShList, callback func(node interface{})) { - for element := range w.iterate(list) { - callback(element) - } +func NewMkShWalker() *MkShWalker { + return &MkShWalker{} } -func (w *MkShWalker) iterate(list *MkShList) <-chan interface{} { - elements := make(chan interface{}) - - go func() { - w.walkList(list, elements) - close(elements) - }() - - return elements +// Walk calls the given callback for each node of the parsed shell program, +// in visiting order from large to small. +func (w *MkShWalker) Walk(list *MkShList, callback *MkShWalkCallback) { + w.walkList(list, callback) } -func (w *MkShWalker) walkList(list *MkShList, collector chan<- interface{}) { - collector <- list +func (w *MkShWalker) walkList(list *MkShList, callback *MkShWalkCallback) { + if callback.List != nil { + callback.List(list) + } for _, andor := range list.AndOrs { - w.walkAndOr(andor, collector) + w.walkAndOr(andor, callback) } } -func (w *MkShWalker) walkAndOr(andor *MkShAndOr, collector chan<- interface{}) { - collector <- andor +func (w *MkShWalker) walkAndOr(andor *MkShAndOr, callback *MkShWalkCallback) { + if callback.AndOr != nil { + callback.AndOr(andor) + } for _, pipeline := range andor.Pipes { - w.walkPipeline(pipeline, collector) + w.walkPipeline(pipeline, callback) } } -func (w *MkShWalker) walkPipeline(pipeline *MkShPipeline, collector chan<- interface{}) { - collector <- pipeline +func (w *MkShWalker) walkPipeline(pipeline *MkShPipeline, callback *MkShWalkCallback) { + if callback.Pipeline != nil { + callback.Pipeline(pipeline) + } for _, command := range pipeline.Cmds { - w.walkCommand(command, collector) + w.walkCommand(command, callback) } } -func (w *MkShWalker) walkCommand(command *MkShCommand, collector chan<- interface{}) { - collector <- command +func (w *MkShWalker) walkCommand(command *MkShCommand, callback *MkShWalkCallback) { + if callback.Command != nil { + callback.Command(command) + } switch { case command.Simple != nil: - w.walkSimpleCommand(command.Simple, collector) + w.walkSimpleCommand(command.Simple, callback) case command.Compound != nil: - w.walkCompoundCommand(command.Compound, collector) - w.walkRedirects(command.Redirects, collector) + w.walkCompoundCommand(command.Compound, callback) + w.walkRedirects(command.Redirects, callback) case command.FuncDef != nil: - w.walkFunctionDefinition(command.FuncDef, collector) - w.walkRedirects(command.Redirects, collector) + w.walkFunctionDefinition(command.FuncDef, callback) + w.walkRedirects(command.Redirects, callback) } } -func (w *MkShWalker) walkSimpleCommand(command *MkShSimpleCommand, collector chan<- interface{}) { - collector <- command +func (w *MkShWalker) walkSimpleCommand(command *MkShSimpleCommand, callback *MkShWalkCallback) { + if callback.SimpleCommand != nil { + callback.SimpleCommand(command) + } - w.walkWords(command.Assignments, collector) + w.walkWords(command.Assignments, callback) if command.Name != nil { - w.walkWord(command.Name, collector) + w.walkWord(command.Name, callback) } - w.walkWords(command.Args, collector) - w.walkRedirects(command.Redirections, collector) + w.walkWords(command.Args, callback) + w.walkRedirects(command.Redirections, callback) } -func (w *MkShWalker) walkCompoundCommand(command *MkShCompoundCommand, collector chan<- interface{}) { - collector <- command +func (w *MkShWalker) walkCompoundCommand(command *MkShCompoundCommand, callback *MkShWalkCallback) { + if callback.CompoundCommand != nil { + callback.CompoundCommand(command) + } switch { case command.Brace != nil: - w.walkList(command.Brace, collector) + w.walkList(command.Brace, callback) case command.Case != nil: - w.walkCase(command.Case, collector) + w.walkCase(command.Case, callback) case command.For != nil: - w.walkFor(command.For, collector) + w.walkFor(command.For, callback) case command.If != nil: - w.walkIf(command.If, collector) + w.walkIf(command.If, callback) case command.Loop != nil: - w.walkLoop(command.Loop, collector) + w.walkLoop(command.Loop, callback) case command.Subshell != nil: - w.walkList(command.Subshell, collector) + w.walkList(command.Subshell, callback) } } -func (w *MkShWalker) walkCase(caseClause *MkShCaseClause, collector chan<- interface{}) { - collector <- caseClause +func (w *MkShWalker) walkCase(caseClause *MkShCaseClause, callback *MkShWalkCallback) { + if callback.Case != nil { + callback.Case(caseClause) + } - w.walkWord(caseClause.Word, collector) + w.walkWord(caseClause.Word, callback) for _, caseItem := range caseClause.Cases { - collector <- caseItem - w.walkWords(caseItem.Patterns, collector) - w.walkList(caseItem.Action, collector) + if callback.CaseItem != nil { + callback.CaseItem(caseItem) + } + w.walkWords(caseItem.Patterns, callback) + w.walkList(caseItem.Action, callback) } } -func (w *MkShWalker) walkFunctionDefinition(funcdef *MkShFunctionDefinition, collector chan<- interface{}) { - collector <- funcdef +func (w *MkShWalker) walkFunctionDefinition(funcdef *MkShFunctionDefinition, callback *MkShWalkCallback) { + if callback.FunctionDefinition != nil { + callback.FunctionDefinition(funcdef) + } - w.walkCompoundCommand(funcdef.Body, collector) + w.walkCompoundCommand(funcdef.Body, callback) } -func (w *MkShWalker) walkIf(ifClause *MkShIfClause, collector chan<- interface{}) { - collector <- ifClause +func (w *MkShWalker) walkIf(ifClause *MkShIfClause, callback *MkShWalkCallback) { + if callback.If != nil { + callback.If(ifClause) + } + for i, cond := range ifClause.Conds { - w.walkList(cond, collector) - w.walkList(ifClause.Actions[i], collector) + w.walkList(cond, callback) + w.walkList(ifClause.Actions[i], callback) } if ifClause.Else != nil { - w.walkList(ifClause.Else, collector) + w.walkList(ifClause.Else, callback) } } -func (w *MkShWalker) walkLoop(loop *MkShLoopClause, collector chan<- interface{}) { - collector <- loop - w.walkList(loop.Cond, collector) - w.walkList(loop.Action, collector) +func (w *MkShWalker) walkLoop(loop *MkShLoopClause, callback *MkShWalkCallback) { + if callback.Loop != nil { + callback.Loop(loop) + } + + w.walkList(loop.Cond, callback) + w.walkList(loop.Action, callback) } -func (w *MkShWalker) walkWords(words []*ShToken, collector chan<- interface{}) { - collector <- words +func (w *MkShWalker) walkWords(words []*ShToken, callback *MkShWalkCallback) { + if len(words) != 0 { + if callback.Words != nil { + callback.Words(words) + } - for _, word := range words { - w.walkWord(word, collector) + for _, word := range words { + w.walkWord(word, callback) + } } } -func (w *MkShWalker) walkWord(word *ShToken, collector chan<- interface{}) { - collector <- word +func (w *MkShWalker) walkWord(word *ShToken, callback *MkShWalkCallback) { + if callback.Word != nil { + callback.Word(word) + } } -func (w *MkShWalker) walkRedirects(redirects []*MkShRedirection, collector chan<- interface{}) { - collector <- redirects +func (w *MkShWalker) walkRedirects(redirects []*MkShRedirection, callback *MkShWalkCallback) { + if len(redirects) != 0 { + if callback.Redirects != nil { + callback.Redirects(redirects) + } + + for _, redirect := range redirects { + if callback.Redirect != nil { + callback.Redirect(redirect) + } - for _, redirect := range redirects { - collector <- redirect - w.walkWord(redirect.Target, collector) + w.walkWord(redirect.Target, callback) + } } } -func (w *MkShWalker) walkFor(forClause *MkShForClause, collector chan<- interface{}) { - collector <- forClause +func (w *MkShWalker) walkFor(forClause *MkShForClause, callback *MkShWalkCallback) { + if callback.For != nil { + callback.For(forClause) + } + if callback.Varname != nil { + callback.Varname(forClause.Varname) + } - collector <- forClause.Varname - w.walkWords(forClause.Values, collector) - w.walkList(forClause.Body, collector) + w.walkWords(forClause.Values, callback) + w.walkList(forClause.Body, callback) +} + +type MkShWalkCallback struct { + List func(list *MkShList) + AndOr func(andor *MkShAndOr) + Pipeline func(pipeline *MkShPipeline) + Command func(command *MkShCommand) + SimpleCommand func(command *MkShSimpleCommand) + CompoundCommand func(command *MkShCompoundCommand) + Case func(caseClause *MkShCaseClause) + CaseItem func(caseItem *MkShCaseItem) + FunctionDefinition func(funcdef *MkShFunctionDefinition) + If func(ifClause *MkShIfClause) + Loop func(loop *MkShLoopClause) + Words func(words []*ShToken) + Word func(word *ShToken) + Redirects func(redirects []*MkShRedirection) + Redirect func(redirect *MkShRedirection) + For func(forClause *MkShForClause) + Varname func(varname string) +} + +func NewMkShWalkCallback() *MkShWalkCallback { + return &MkShWalkCallback{} } diff --git a/pkgtools/pkglint/files/mkshwalker_test.go b/pkgtools/pkglint/files/mkshwalker_test.go index 5169772596f..ca486d59ba4 100644 --- a/pkgtools/pkglint/files/mkshwalker_test.go +++ b/pkgtools/pkglint/files/mkshwalker_test.go @@ -1,33 +1,159 @@ package main import ( + "fmt" "gopkg.in/check.v1" ) func (s *Suite) Test_MkShWalker_Walk(c *check.C) { list, err := parseShellProgram(dummyLine, ""+ "if condition; then action; else case selector in pattern) case-item-action ;; esac; fi; "+ - "set -e; cd ${WRKSRC}/locale; "+ + "set -e; "+ + "cd ${WRKSRC}/locale; "+ "for lang in *.po; do "+ " [ \"$${lang}\" = \"wxstd.po\" ] && continue; "+ " ${TOOLS_PATH.msgfmt} -c -o \"$${lang%.po}.mo\" \"$${lang}\"; "+ - "done") + "done; "+ + "while :; do fun() { :; } 1>&2; done") if c.Check(err, check.IsNil) && c.Check(list, check.NotNil) { var commands []string - (*MkShWalker).Walk(nil, list, func(node interface{}) { - if cmd, ok := node.(*MkShSimpleCommand); ok { - commands = append(commands, NewStrCommand(cmd).String()) + add := func(kind string, format string, args ...interface{}) { + if format != "" && !contains(format, "%") { + panic(format) } - }) + detail := fmt.Sprintf(format, args...) + commands = append(commands, fmt.Sprintf("%16s %s", kind, detail)) + } + + callback := NewMkShWalkCallback() + callback.List = func(list *MkShList) { add("List", "with %d andOrs", len(list.AndOrs)) } + callback.AndOr = func(andor *MkShAndOr) { add("AndOr", "with %d pipelines", len(andor.Pipes)) } + callback.Pipeline = func(pipeline *MkShPipeline) { add("Pipeline", "with %d commands", len(pipeline.Cmds)) } + callback.Command = func(command *MkShCommand) { add("Command", "") } + callback.SimpleCommand = func(command *MkShSimpleCommand) { add("SimpleCommand", "%s", NewStrCommand(command).String()) } + callback.CompoundCommand = func(command *MkShCompoundCommand) { add("CompoundCommand", "") } + callback.Case = func(caseClause *MkShCaseClause) { add("Case", "with %d items", len(caseClause.Cases)) } + callback.CaseItem = func(caseItem *MkShCaseItem) { add("CaseItem", "with %d patterns", len(caseItem.Patterns)) } + callback.FunctionDefinition = func(funcdef *MkShFunctionDefinition) { add("FunctionDef", "for %s", funcdef.Name) } + callback.If = func(ifClause *MkShIfClause) { add("If", "with %d then-branches", len(ifClause.Conds)) } + callback.Loop = func(loop *MkShLoopClause) { add("Loop", "") } + callback.Words = func(words []*ShToken) { add("Words", "with %d words", len(words)) } + callback.Word = func(word *ShToken) { add("Word", "%s", word.MkText) } + callback.Redirects = func(redirects []*MkShRedirection) { add("Redirects", "with %d redirects", len(redirects)) } + callback.Redirect = func(redirect *MkShRedirection) { add("Redirect", "%s", redirect.Op) } + callback.For = func(forClause *MkShForClause) { add("For", "variable %s", forClause.Varname) } + callback.Varname = func(varname string) { add("Varname", "%s", varname) } + + NewMkShWalker().Walk(list, callback) + c.Check(commands, deepEquals, []string{ - "[] condition []", - "[] action []", - "[] case-item-action []", - "[] set [-e]", - "[] cd [${WRKSRC}/locale]", - "[] [ [\"$${lang}\" = \"wxstd.po\" ]]", - "[] continue []", - "[] ${TOOLS_PATH.msgfmt} [-c -o \"$${lang%.po}.mo\" \"$${lang}\"]"}) + " List with 5 andOrs", + " AndOr with 1 pipelines", + " Pipeline with 1 commands", + " Command ", + " CompoundCommand ", + " If with 1 then-branches", + " List with 1 andOrs", + " AndOr with 1 pipelines", + " Pipeline with 1 commands", + " Command ", + " SimpleCommand [] condition []", + " Word condition", + " List with 1 andOrs", + " AndOr with 1 pipelines", + " Pipeline with 1 commands", + " Command ", + " SimpleCommand [] action []", + " Word action", + " List with 1 andOrs", + " AndOr with 1 pipelines", + " Pipeline with 1 commands", + " Command ", + " CompoundCommand ", + " Case with 1 items", + " Word selector", + " CaseItem with 1 patterns", + " Words with 1 words", + " Word pattern", + " List with 1 andOrs", + " AndOr with 1 pipelines", + " Pipeline with 1 commands", + " Command ", + " SimpleCommand [] case-item-action []", + " Word case-item-action", + " AndOr with 1 pipelines", + " Pipeline with 1 commands", + " Command ", + " SimpleCommand [] set [-e]", + " Word set", + " Words with 1 words", + " Word -e", + " AndOr with 1 pipelines", + " Pipeline with 1 commands", + " Command ", + " SimpleCommand [] cd [${WRKSRC}/locale]", + " Word cd", + " Words with 1 words", + " Word ${WRKSRC}/locale", + " AndOr with 1 pipelines", + " Pipeline with 1 commands", + " Command ", + " CompoundCommand ", + " For variable lang", + " Varname lang", + " Words with 1 words", + " Word *.po", + " List with 2 andOrs", + " AndOr with 2 pipelines", + " Pipeline with 1 commands", + " Command ", + " SimpleCommand [] [ [\"$${lang}\" = \"wxstd.po\" ]]", + " Word [", + " Words with 4 words", + " Word \"$${lang}\"", + " Word =", + " Word \"wxstd.po\"", + " Word ]", + " Pipeline with 1 commands", + " Command ", + " SimpleCommand [] continue []", + " Word continue", + " AndOr with 1 pipelines", + " Pipeline with 1 commands", + " Command ", + " SimpleCommand [] ${TOOLS_PATH.msgfmt} [-c -o \"$${lang%.po}.mo\" \"$${lang}\"]", + " Word ${TOOLS_PATH.msgfmt}", + " Words with 4 words", + " Word -c", + " Word -o", + " Word \"$${lang%.po}.mo\"", + " Word \"$${lang}\"", + " AndOr with 1 pipelines", + " Pipeline with 1 commands", + " Command ", + " CompoundCommand ", + " Loop ", + " List with 1 andOrs", + " AndOr with 1 pipelines", + " Pipeline with 1 commands", + " Command ", + " SimpleCommand [] : []", + " Word :", + " List with 1 andOrs", + " AndOr with 1 pipelines", + " Pipeline with 1 commands", + " Command ", + " FunctionDef for fun", + " CompoundCommand ", + " List with 1 andOrs", + " AndOr with 1 pipelines", + " Pipeline with 1 commands", + " Command ", + " SimpleCommand [] : []", + " Word :", + " Redirects with 1 redirects", + " Redirect >&", + " Word 2"}) } } diff --git a/pkgtools/pkglint/files/options.go b/pkgtools/pkglint/files/options.go index 37f8b6e9018..996ae05a4da 100755 --- a/pkgtools/pkglint/files/options.go +++ b/pkgtools/pkglint/files/options.go @@ -27,24 +27,27 @@ func ChecklinesOptionsMk(mklines *MkLines) { handledOptions := make(map[string]MkLine) var optionsInDeclarationOrder []string - // The conditionals are typically for OPSYS and MACHINE_ARCH. loop: for ; !exp.EOF(); exp.Advance() { mkline := exp.CurrentMkLine() switch { case mkline.IsComment(): case mkline.IsEmpty(): + case mkline.IsVarassign(): - varname := mkline.Varname() - if varname == "PKG_SUPPORTED_OPTIONS" || hasPrefix(varname, "PKG_OPTIONS_GROUP.") { + switch mkline.Varcanon() { + case "PKG_SUPPORTED_OPTIONS", "PKG_OPTIONS_GROUP.*", "PKG_OPTIONS_SET.*": for _, option := range splitOnSpace(mkline.Value()) { - if option == mkline.WithoutMakeVariables(option) { + if !containsVarRef(option) { declaredOptions[option] = mkline optionsInDeclarationOrder = append(optionsInDeclarationOrder, option) } } } + case mkline.IsCond(): + // The conditionals are typically for OPSYS and MACHINE_ARCH. + case mkline.IsInclude(): includedFile := mkline.IncludeFile() switch { @@ -57,6 +60,7 @@ loop: exp.Advance() break loop } + default: exp.CurrentLine().Warnf("Expected inclusion of \"../../mk/bsd.options.mk\".") Explain( @@ -70,17 +74,30 @@ loop: for ; !exp.EOF(); exp.Advance() { mkline := exp.CurrentMkLine() - if mkline.IsCond() && mkline.Directive() == "if" { + if mkline.IsCond() && (mkline.Directive() == "if" || mkline.Directive() == "elif") { cond := NewMkParser(mkline.Line, mkline.Args(), false).MkCond() - if cond != nil { - cond.Visit("empty", func(t *Tree) { - varuse := t.args[0].(MkVarUse) + if cond == nil { + continue + } + + NewMkCondWalker().Walk(cond, &MkCondCallback{ + Empty: func(varuse *MkVarUse) { if varuse.varname == "PKG_OPTIONS" && len(varuse.modifiers) == 1 && hasPrefix(varuse.modifiers[0], "M") { option := varuse.modifiers[0][1:] - handledOptions[option] = mkline - optionsInDeclarationOrder = append(optionsInDeclarationOrder, option) + if !containsVarRef(option) { + handledOptions[option] = mkline + optionsInDeclarationOrder = append(optionsInDeclarationOrder, option) + } } - }) + }}) + + if cond.Empty != nil && mkline.HasElseBranch() { + mkline.Notef("The positive branch of the .if/.else should be the one where the option is set.") + Explain( + "For consistency among packages, the upper branch of this", + ".if/.else statement should always handle the case where the", + "option is activated. A missing exclamation mark at this", + "point can easily be overlooked.") } } } @@ -95,7 +112,7 @@ loop: "typo, or the option does not have any effect.") } if declared == nil && handled != nil { - handled.Warnf("Option %q is handled but not declared above.", option) + handled.Warnf("Option %q is handled but not added to PKG_SUPPORTED_OPTIONS.", option) Explain( "This block of code will never be run since PKG_OPTIONS cannot", "contain this value. This is most probably a typo.") diff --git a/pkgtools/pkglint/files/options_test.go b/pkgtools/pkglint/files/options_test.go index 4306e8dde38..9213f2fc712 100755 --- a/pkgtools/pkglint/files/options_test.go +++ b/pkgtools/pkglint/files/options_test.go @@ -8,8 +8,11 @@ func (s *Suite) Test_ChecklinesOptionsMk(c *check.C) { t.SetupCommandLine("-Wno-space") t.SetupVartypes() t.SetupOption("mc-charset", "") + t.SetupOption("mysql", "") t.SetupOption("ncurses", "") + t.SetupOption("negative", "Demonstrates negated .if/.else") t.SetupOption("slang", "") + t.SetupOption("sqlite", "") t.SetupOption("x11", "") t.SetupFileMkLines("mk/bsd.options.mk", @@ -21,8 +24,10 @@ func (s *Suite) Test_ChecklinesOptionsMk(c *check.C) { "PKG_OPTIONS_VAR= PKG_OPTIONS.mc", "PKG_OPTIONS_REQUIRED_GROUPS= screen", "PKG_OPTIONS_GROUP.screen= ncurses slang", - "PKG_SUPPORTED_OPTIONS= mc-charset x11 lang-${l}", + "PKG_SUPPORTED_OPTIONS= mc-charset x11 lang-${l} negative", "PKG_SUGGESTED_OPTIONS= mc-charset slang", + "PKG_OPTIONS_NONEMPTY_SETS+= db", + "PKG_OPTIONS_SET.db= mysql sqlite", "", ".include \"../../mk/bsd.options.mk\"", "", @@ -30,19 +35,27 @@ func (s *Suite) Test_ChecklinesOptionsMk(c *check.C) { ".endif", "", ".if !empty(PKG_OPTIONS:Mundeclared)", + ".endif", + "", + ".if empty(PKG_OPTIONS:Mnegative)", + ".else", + ".endif", + "", + ".if !empty(PKG_OPTIONS:Mncurses)", + ".elif !empty(PKG_OPTIONS:Mslang)", + ".endif", + "", + ".if !empty(PKG_OPTIONS:Mmysql)", + ".elif !empty(PKG_OPTIONS:Msqlite)", ".endif") - G.CurrentDir = t.TmpDir() - G.CurPkgsrcdir = "." - ChecklinesOptionsMk(mklines) t.CheckOutputLines( - "WARN: ~/category/package/options.mk:14: Unknown option \"undeclared\".", - "WARN: ~/category/package/options.mk:5: Option \"ncurses\" should be handled below in an .if block.", - "WARN: ~/category/package/options.mk:5: Option \"slang\" should be handled below in an .if block.", + "WARN: ~/category/package/options.mk:16: Unknown option \"undeclared\".", + "NOTE: ~/category/package/options.mk:19: The positive branch of the .if/.else should be the one where the option is set.", "WARN: ~/category/package/options.mk:6: Option \"mc-charset\" should be handled below in an .if block.", - "WARN: ~/category/package/options.mk:14: Option \"undeclared\" is handled but not declared above.") + "WARN: ~/category/package/options.mk:16: Option \"undeclared\" is handled but not added to PKG_SUPPORTED_OPTIONS.") } func (s *Suite) Test_ChecklinesOptionsMk__unexpected_line(c *check.C) { @@ -68,11 +81,39 @@ func (s *Suite) Test_ChecklinesOptionsMk__unexpected_line(c *check.C) { "pre-configure:", "\techo \"In the pre-configure stage.\"") - G.CurrentDir = t.TmpDir() - G.CurPkgsrcdir = "." - ChecklinesOptionsMk(mklines) t.CheckOutputLines( "WARN: ~/category/package/options.mk:7: Expected inclusion of \"../../mk/bsd.options.mk\".") } + +func (s *Suite) Test_ChecklinesOptionsMk__malformed_conditional(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wno-space") + t.SetupVartypes() + t.SetupOption("mc-charset", "") + t.SetupOption("ncurses", "") + t.SetupOption("slang", "") + t.SetupOption("x11", "") + + t.SetupFileMkLines("mk/bsd.options.mk", + MkRcsID) + + mklines := t.SetupFileMkLines("category/package/options.mk", + MkRcsID, + "", + "PKG_OPTIONS_VAR= PKG_OPTIONS.mc", + "PKG_SUPPORTED_OPTIONS= # none", + "PKG_SUGGESTED_OPTIONS= # none", + "", + ".include \"../../mk/bsd.options.mk\"", + "", + ".if ${OPSYS} == 'Darwin'", + ".endif") + + ChecklinesOptionsMk(mklines) + + t.CheckOutputLines( + "WARN: ~/category/package/options.mk:9: Invalid conditional \"${OPSYS} == 'Darwin'\".") +} diff --git a/pkgtools/pkglint/files/package.go b/pkgtools/pkglint/files/package.go index 3ff5650f4fa..0e46b47375f 100644 --- a/pkgtools/pkglint/files/package.go +++ b/pkgtools/pkglint/files/package.go @@ -37,11 +37,15 @@ type Package struct { conditionalIncludes map[string]MkLine unconditionalIncludes map[string]MkLine once Once + IgnoreMissingPatches bool // In distinfo, don't warn about patches that cannot be found. } func NewPackage(pkgpath string) *Package { pkg := &Package{ Pkgpath: pkgpath, + Pkgdir: ".", + Filesdir: "files", + Patchdir: "patches", PlistDirs: make(map[string]bool), PlistFiles: make(map[string]bool), vars: NewScope(), @@ -58,6 +62,12 @@ func NewPackage(pkgpath string) *Package { return pkg } +// File returns the (possibly absolute) path to relativeFilename, +// as resolved from the package's directory. +func (pkg *Package) File(relativeFilename string) string { + return G.Pkgsrc.File(pkg.Pkgpath + "/" + relativeFilename) +} + func (pkg *Package) varValue(varname string) (string, bool) { switch varname { case "KRB5_TYPE": @@ -151,26 +161,30 @@ func (pkglint *Pkglint) checkdirPackage(pkgpath string) { defer trace.Call1(pkgpath)() } + if strings.Count(pkgpath, "/") != 1 { + dummyLine.Fatalf("Internal pkglint error: Wrong pkgpath %q.", pkgpath) + } + G.Pkg = NewPackage(pkgpath) defer func() { G.Pkg = nil }() pkg := G.Pkg // we need to handle the Makefile first to get some variables - lines := pkg.loadPackageMakefile(G.CurrentDir + "/Makefile") + lines := pkg.loadPackageMakefile() if lines == nil { return } - files := dirglob(G.CurrentDir) + files := dirglob(pkg.File(".")) if pkg.Pkgdir != "." { - files = append(files, dirglob(G.CurrentDir+"/"+pkg.Pkgdir)...) + files = append(files, dirglob(pkg.File(pkg.Pkgdir))...) } if G.opts.CheckExtra { - files = append(files, dirglob(G.CurrentDir+"/"+pkg.Filesdir)...) + files = append(files, dirglob(pkg.File(pkg.Filesdir))...) } - files = append(files, dirglob(G.CurrentDir+"/"+pkg.Patchdir)...) + files = append(files, dirglob(pkg.File(pkg.Patchdir))...) if pkg.DistinfoFile != "distinfo" && pkg.DistinfoFile != "./distinfo" { - files = append(files, G.CurrentDir+"/"+pkg.DistinfoFile) + files = append(files, pkg.File(pkg.DistinfoFile)) } haveDistinfo := false havePatches := false @@ -194,7 +208,7 @@ func (pkglint *Pkglint) checkdirPackage(pkgpath string) { if containsVarRef(fname) { continue } - if fname == G.CurrentDir+"/Makefile" { + if fname == pkg.File("Makefile") { if st, err := os.Lstat(fname); err == nil { pkglint.checkExecutable(st, fname) } @@ -214,7 +228,7 @@ func (pkglint *Pkglint) checkdirPackage(pkgpath string) { if G.opts.CheckDistinfo && G.opts.CheckPatches { if havePatches && !haveDistinfo { - NewLineWhole(G.CurrentDir+"/"+pkg.DistinfoFile).Warnf("File not found. Please run \"%s makepatchsum\".", confMake) + NewLineWhole(pkg.File(pkg.DistinfoFile)).Warnf("File not found. Please run \"%s makepatchsum\".", confMake) } } @@ -226,12 +240,13 @@ func (pkglint *Pkglint) checkdirPackage(pkgpath string) { } } - if !isEmptyDir(G.CurrentDir + "/scripts") { - NewLineWhole(G.CurrentDir + "/scripts").Warnf("This directory and its contents are deprecated! Please call the script(s) explicitly from the corresponding target(s) in the pkg's Makefile.") + if !isEmptyDir(pkg.File("scripts")) { + NewLineWhole(pkg.File("scripts")).Warnf("This directory and its contents are deprecated! Please call the script(s) explicitly from the corresponding target(s) in the pkg's Makefile.") } } -func (pkg *Package) loadPackageMakefile(fname string) *MkLines { +func (pkg *Package) loadPackageMakefile() *MkLines { + fname := pkg.File("Makefile") if trace.Tracing { defer trace.Call1(fname)() } @@ -256,13 +271,19 @@ func (pkg *Package) loadPackageMakefile(fname string) *MkLines { pkg.Filesdir = pkg.expandVariableWithDefault("FILESDIR", "files") pkg.Patchdir = pkg.expandVariableWithDefault("PATCHDIR", "patches") + // See lang/php/ext.mk if varIsDefined("PHPEXT_MK") { if !varIsDefined("USE_PHP_EXT_PATCHES") { pkg.Patchdir = "patches" } if varIsDefined("PECL_VERSION") { pkg.DistinfoFile = "distinfo" + } else { + pkg.IgnoreMissingPatches = true } + + // For PHP modules that are not PECL, this combination means that + // the patches in the distinfo cannot be found in PATCHDIR. } if trace.Tracing { @@ -349,8 +370,8 @@ func (pkg *Package) readMakefile(fname string, mainLines *MkLines, allLines *MkL if fileMklines.indentation.IsCheckedFile(includeFile) { return true // See https://github.com/rillig/pkglint/issues/1 - } else if dirname != G.CurrentDir { // Prevent unnecessary syscalls - dirname = G.CurrentDir + } else if dirname != pkg.File(".") { // Prevent unnecessary syscalls + dirname = pkg.File(".") if !fileExists(dirname + "/" + includeFile) { mkline.Errorf("Cannot read %q.", dirname+"/"+includeFile) result = false @@ -402,17 +423,17 @@ func (pkg *Package) checkfilePackageMakefile(fname string, mklines *MkLines) { if !vars.Defined("PLIST_SRC") && !vars.Defined("GENERATE_PLIST") && !vars.Defined("META_PACKAGE") && - !fileExists(G.CurrentDir+"/"+pkg.Pkgdir+"/PLIST") && - !fileExists(G.CurrentDir+"/"+pkg.Pkgdir+"/PLIST.common") { + !fileExists(pkg.File(pkg.Pkgdir+"/PLIST")) && + !fileExists(pkg.File(pkg.Pkgdir+"/PLIST.common")) { NewLineWhole(fname).Warnf("Neither PLIST nor PLIST.common exist, and PLIST_SRC is unset.") } - if (vars.Defined("NO_CHECKSUM") || vars.Defined("META_PACKAGE")) && isEmptyDir(G.CurrentDir+"/"+pkg.Patchdir) { - if distinfoFile := G.CurrentDir + "/" + pkg.DistinfoFile; fileExists(distinfoFile) { + if (vars.Defined("NO_CHECKSUM") || vars.Defined("META_PACKAGE")) && isEmptyDir(pkg.File(pkg.Patchdir)) { + if distinfoFile := pkg.File(pkg.DistinfoFile); fileExists(distinfoFile) { NewLineWhole(distinfoFile).Warnf("This file should not exist if NO_CHECKSUM or META_PACKAGE is set.") } } else { - if distinfoFile := G.CurrentDir + "/" + pkg.DistinfoFile; !containsVarRef(distinfoFile) && !fileExists(distinfoFile) { + if distinfoFile := pkg.File(pkg.DistinfoFile); !containsVarRef(distinfoFile) && !fileExists(distinfoFile) { NewLineWhole(distinfoFile).Warnf("File not found. Please run \"%s makesum\" or define NO_CHECKSUM=yes in the package Makefile.", confMake) } } @@ -901,7 +922,7 @@ func (pkg *Package) CheckInclude(mkline MkLine, indentation *Indentation) { mkline.SetConditionVars(conditionVars) } - if path.Dir(abspath(mkline.Filename)) == abspath(G.CurrentDir) { + if path.Dir(abspath(mkline.Filename)) == abspath(pkg.File(".")) { includefile := mkline.IncludeFile() if indentation.IsConditional() { diff --git a/pkgtools/pkglint/files/package_test.go b/pkgtools/pkglint/files/package_test.go index 64abc7d3255..b4582c241d5 100644 --- a/pkgtools/pkglint/files/package_test.go +++ b/pkgtools/pkglint/files/package_test.go @@ -159,9 +159,8 @@ func (s *Suite) Test_Package_varorder_license(c *check.C) { ".include \"../../mk/bsd.pkg.mk\"") t.SetupVartypes() - G.CurrentDir = t.TmpDir() - G.CheckDirent(t.TmpDir() + "/x11/9term") + G.CheckDirent(t.File("x11/9term")) // Since the error is grave enough, the warning about the correct position is suppressed. t.CheckOutputLines( @@ -281,7 +280,6 @@ func (s *Suite) Test_Package_checkPossibleDowngrade(c *check.C) { t := s.Init(c) G.Pkg = NewPackage("category/pkgbase") - G.CurPkgsrcdir = "../.." G.Pkg.EffectivePkgname = "package-1.0nb15" G.Pkg.EffectivePkgnameLine = t.NewMkLine("category/pkgbase/Makefile", 5, "PKGNAME=dummy") G.Pkgsrc.LastChange = map[string]*Change{ @@ -307,61 +305,44 @@ func (s *Suite) Test_Package_checkPossibleDowngrade(c *check.C) { func (s *Suite) Test_checkdirPackage(c *check.C) { t := s.Init(c) - t.SetupFileLines("Makefile", + t.SetupFileLines("category/package/Makefile", MkRcsID) - G.CurrentDir = t.TmpDir() - G.checkdirPackage(t.TmpDir()) + G.checkdirPackage("category/package") t.CheckOutputLines( - "WARN: ~/Makefile: Neither PLIST nor PLIST.common exist, and PLIST_SRC is unset.", - "WARN: ~/distinfo: File not found. Please run \""+confMake+" makesum\" or define NO_CHECKSUM=yes in the package Makefile.", - "ERROR: ~/Makefile: Each package must define its LICENSE.", - "WARN: ~/Makefile: No COMMENT given.") + "WARN: ~/category/package/Makefile: Neither PLIST nor PLIST.common exist, and PLIST_SRC is unset.", + "WARN: ~/category/package/distinfo: File not found. Please run \""+confMake+" makesum\" or define NO_CHECKSUM=yes in the package Makefile.", + "ERROR: ~/category/package/Makefile: Each package must define its LICENSE.", + "WARN: ~/category/package/Makefile: No COMMENT given.") } -func (s *Suite) Test_checkdirPackage__meta_package_without_license(c *check.C) { +func (s *Suite) Test_Pkglint_checkdirPackage__meta_package_without_license(c *check.C) { t := s.Init(c) - t.CreateFileLines("Makefile", + t.CreateFileLines("category/package/Makefile", MkRcsID, "", "META_PACKAGE=\tyes") - G.CurrentDir = t.TmpDir() t.SetupVartypes() - G.checkdirPackage(t.TmpDir()) + G.checkdirPackage("category/package") t.CheckOutputLines( - "WARN: ~/Makefile: No COMMENT given.") // No error about missing LICENSE. + "WARN: ~/category/package/Makefile: No COMMENT given.") // No error about missing LICENSE. } func (s *Suite) Test_Package__varuse_at_load_time(c *check.C) { t := s.Init(c) - t.CreateFileLines("doc/CHANGES-2016", - "# dummy") - t.CreateFileLines("doc/TODO", - "# dummy") + t.SetupPkgsrc() t.CreateFileLines("licenses/bsd-2", "# dummy") - t.CreateFileLines("mk/fetch/sites.mk", - "# dummy") - t.CreateFileLines("mk/bsd.pkg.mk", - "# dummy") - t.CreateFileLines("mk/defaults/options.description", - "option Description") - t.CreateFileLines("mk/defaults/mk.conf", - "# dummy") - t.CreateFileLines("mk/tools/bsd.tools.mk", - ".include \"defaults.mk\"") t.CreateFileLines("mk/tools/defaults.mk", "TOOLS_CREATE+=false", "TOOLS_CREATE+=nice", "TOOLS_CREATE+=true", "_TOOLS_VARNAME.nice=NICE") - t.CreateFileLines("mk/bsd.prefs.mk", - "# dummy") t.CreateFileLines("category/pkgbase/Makefile", MkRcsID, @@ -391,7 +372,7 @@ func (s *Suite) Test_Package__varuse_at_load_time(c *check.C) { t.CreateFileLines("category/pkgbase/distinfo", RcsID) - G.Main("pkglint", "-q", "-Wperm", t.TmpDir()+"/category/pkgbase") + G.Main("pkglint", "-q", "-Wperm", t.File("category/pkgbase")) t.CheckOutputLines( "WARN: ~/category/pkgbase/Makefile:8: To use the tool \"FALSE\" at load time, bsd.prefs.mk has to be included before.", @@ -411,11 +392,9 @@ func (s *Suite) Test_Package_loadPackageMakefile(c *check.C) { "DISTNAME=distfile_1_67", ".include \"../../category/package/Makefile\"") pkg := NewPackage("category/package") - G.CurrentDir = t.TempFilename("category/package") - G.CurPkgsrcdir = "../.." G.Pkg = pkg - pkg.loadPackageMakefile(t.TempFilename("category/package/Makefile")) + pkg.loadPackageMakefile() // Including a package Makefile directly is an error (in the last line), // but that is checked later. @@ -459,8 +438,6 @@ func (s *Suite) Test_Package_conditionalAndUnconditionalInclude(c *check.C) { t.CreateFileLines("sysutils/coreutils/buildlink3.mk", "") pkg := NewPackage("category/package") - G.CurrentDir = t.TmpDir() + "/category/package" - G.CurPkgsrcdir = "../.." G.Pkg = pkg G.checkdirPackage("category/package") @@ -488,8 +465,7 @@ func (s *Suite) Test_Package_includeWithoutExists(c *check.C) { "", ".include \"../../mk/bsd.pkg.mk\"") - G.CurrentDir = t.TempFilename("category/package") - G.checkdirPackage(G.CurrentDir) + G.checkdirPackage("category/package") t.CheckOutputLines( "ERROR: ~/category/package/options.mk: Cannot be read.") @@ -510,16 +486,14 @@ func (s *Suite) Test_Package_includeAfterExists(c *check.C) { "", ".include \"../../mk/bsd.pkg.mk\"") - G.CurrentDir = t.TempFilename("category/package") - G.checkdirPackage(G.CurrentDir) + G.checkdirPackage("category/package") t.CheckOutputLines( "WARN: ~/category/package/Makefile: Neither PLIST nor PLIST.common exist, and PLIST_SRC is unset.", "WARN: ~/category/package/distinfo: File not found. Please run \""+confMake+" makesum\" or define NO_CHECKSUM=yes in the package Makefile.", "ERROR: ~/category/package/Makefile: Each package must define its LICENSE.", "WARN: ~/category/package/Makefile: No COMMENT given.", - "ERROR: ~/category/package/Makefile:4: \"options.mk\" does not exist.", - "ERROR: ~/category/package/Makefile:7: \"/mk/bsd.pkg.mk\" does not exist.") + "ERROR: ~/category/package/Makefile:4: \"options.mk\" does not exist.") } // See https://github.com/rillig/pkglint/issues/1 @@ -537,8 +511,7 @@ func (s *Suite) Test_Package_includeOtherAfterExists(c *check.C) { "", ".include \"../../mk/bsd.pkg.mk\"") - G.CurrentDir = t.TempFilename("category/package") - G.checkdirPackage(G.CurrentDir) + G.checkdirPackage("category/package") t.CheckOutputLines( "ERROR: ~/category/package/another.mk: Cannot be read.") @@ -572,12 +545,9 @@ func (s *Suite) Test_Package__redundant_master_sites(c *check.C) { ".include \"../../math/R/Makefile.extension\"", ".include \"../../mk/bsd.pkg.mk\"") - G.CurrentDir = t.TempFilename("math/R-date") - G.CurPkgsrcdir = "../.." - // See Package.checkfilePackageMakefile // See Scope.uncond - G.checkdirPackage(G.CurrentDir) + G.checkdirPackage("math/R-date") t.CheckOutputLines( "NOTE: ~/math/R-date/Makefile:6: Definition of MASTER_SITES is redundant because of ../R/Makefile.extension:4.") diff --git a/pkgtools/pkglint/files/patches_test.go b/pkgtools/pkglint/files/patches_test.go index 85705b871e9..f140d8baa8b 100644 --- a/pkgtools/pkglint/files/patches_test.go +++ b/pkgtools/pkglint/files/patches_test.go @@ -48,7 +48,6 @@ func (s *Suite) Test_ChecklinesPatch__without_empty_line__autofix(c *check.C) { "SHA1 (some patch) = 49abd735b7e706ea9ed6671628bb54e91f7f5ffb") t.SetupCommandLine("-Wall", "--autofix") - G.CurrentDir = t.TmpDir() G.Pkg = &Package{DistinfoFile: "distinfo"} ChecklinesPatch(patchLines) @@ -479,3 +478,49 @@ func (s *Suite) Test_ChecklinesPatch__autofix_long_empty_patch(c *check.C) { t.CheckOutputEmpty() } + +func (s *Suite) Test_ChecklinesPatch__crlf(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wall", "--autofix") + lines := t.SetupFileLines("patch-aa", + RcsID, + "", + "Documentation", + "", + "--- oldfile", + "+++ newfile", + "@@ -1,1 +1,1 @@\r", + "-old line", + "+new line") + + ChecklinesPatch(lines) + + t.CheckOutputLines( + "AUTOFIX: ~/patch-aa:7: Replacing \"\\r\\n\" with \"\\n\".") +} + +func (s *Suite) Test_ChecklinesPatch__autogenerated(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wall") + lines := t.SetupFileLines("patch-aa", + RcsID, + "", + "Documentation", + "", + "--- configure.orig", + "+++ configure", + "@@ -1,1 +1,1 @@", + "-old line", + "+: Avoid regenerating within pkgsrc") + + ChecklinesPatch(lines) + + t.CheckOutputLines( + "ERROR: ~/patch-aa:9: This code must not be included in patches.") +} + +func (s *Suite) Test_FileType_String(c *check.C) { + c.Check(ftUnknown.String(), equals, "unknown") +} diff --git a/pkgtools/pkglint/files/pkglint.go b/pkgtools/pkglint/files/pkglint.go index 9ad5fbd3d7b..fff56fecfcc 100644 --- a/pkgtools/pkglint/files/pkglint.go +++ b/pkgtools/pkglint/files/pkglint.go @@ -32,9 +32,7 @@ type Pkglint struct { Mk *MkLines // The Makefile (or fragment) that is currently checked. Todo []string // The files or directories that still need to be checked. - CurrentDir string // The currently checked directory, relative to the cwd - CurPkgsrcdir string // The pkgsrc directory, relative to currentDir - Wip bool // Is the currently checked directory from pkgsrc-wip? + Wip bool // Is the currently checked item from pkgsrc-wip? Infrastructure bool // Is the currently checked item from the pkgsrc infrastructure? Testing bool // Is pkglint in self-testing mode (only during development)? CurrentUsername string // For checking against OWNER and MAINTAINER @@ -52,6 +50,7 @@ type Pkglint struct { logErr *SeparatorWriter loghisto *histogram.Histogram + loaded *histogram.Histogram } type CmdOpts struct { @@ -143,10 +142,12 @@ func (pkglint *Pkglint) Main(argv ...string) (exitcode int) { regex.Profiling = true pkglint.loghisto = histogram.New() + pkglint.loaded = histogram.New() defer func() { pkglint.logOut.Write("") - pkglint.loghisto.PrintStats("loghisto", pkglint.logOut.out, 0) + pkglint.loghisto.PrintStats("loghisto", pkglint.logOut.out, -1) regex.PrintStats() + pkglint.loaded.PrintStats("loaded", pkglint.logOut.out, 50) }() } @@ -296,13 +297,12 @@ func (pkglint *Pkglint) CheckDirent(fname string) { isReg := st.Mode().IsRegular() currentDir := ifelseStr(isReg, path.Dir(fname), fname) - pkglint.CurrentDir = currentDir absCurrentDir := abspath(currentDir) pkglint.Wip = !pkglint.opts.Import && matches(absCurrentDir, `/wip/|/wip$`) pkglint.Infrastructure = matches(absCurrentDir, `/mk/|/mk$`) - pkglint.CurPkgsrcdir = findPkgsrcTopdir(currentDir) - if pkglint.CurPkgsrcdir == "" { - NewLineWhole(fname).Errorf("Cannot determine the pkgsrc root directory for %q.", currentDir) + pkgsrcdir := findPkgsrcTopdir(currentDir) + if pkgsrcdir == "" { + NewLineWhole(fname).Errorf("Cannot determine the pkgsrc root directory for %q.", cleanpath(currentDir)) return } @@ -314,13 +314,13 @@ func (pkglint *Pkglint) CheckDirent(fname string) { return } - switch pkglint.CurPkgsrcdir { + switch pkgsrcdir { case "../..": pkglint.checkdirPackage(pkglint.Pkgsrc.ToRel(currentDir)) case "..": - CheckdirCategory() + CheckdirCategory(currentDir) case ".": - CheckdirToplevel() + CheckdirToplevel(currentDir) default: NewLineWhole(fname).Errorf("Cannot check directories outside a pkgsrc tree.") } @@ -509,7 +509,7 @@ func (pkglint *Pkglint) Checkfile(fname string) { case basename == "ALTERNATIVES": if pkglint.opts.CheckAlternatives { - CheckfileExtra(fname) + CheckfileAlternatives(fname, nil) } case basename == "buildlink3.mk": diff --git a/pkgtools/pkglint/files/pkglint_test.go b/pkgtools/pkglint/files/pkglint_test.go index b9625888f71..9a4538e2f77 100644 --- a/pkgtools/pkglint/files/pkglint_test.go +++ b/pkgtools/pkglint/files/pkglint_test.go @@ -124,15 +124,11 @@ func (s *Suite) Test_Pkglint_Main__unknown_option(c *check.C) { func (s *Suite) Test_Pkglint_Main__complete_package(c *check.C) { t := s.Init(c) - // This file is needed to locate the pkgsrc root directory. - // See findPkgsrcTopdir. - t.CreateFileLines("mk/bsd.pkg.mk", - "# dummy") + t.SetupPkgsrc() - // See Pkgsrc.loadDocChanges. // FIXME: pkglint should warn that the latest version in this file // (1.10) doesn't match the current version in the package (1.11). - t.CreateFileLines("doc/CHANGES-2018", + t.SetupFileLines("doc/CHANGES-2018", RcsID, "", "Changes to the packages collection and infrastructure in 2018:", @@ -140,7 +136,7 @@ func (s *Suite) Test_Pkglint_Main__complete_package(c *check.C) { "\tUpdated sysutils/checkperms to 1.10 [rillig 2018-01-05]") // See Pkgsrc.loadSuggestedUpdates. - t.CreateFileLines("doc/TODO", + t.SetupFileLines("doc/TODO", RcsID, "", "Suggested package updates", @@ -148,47 +144,27 @@ func (s *Suite) Test_Pkglint_Main__complete_package(c *check.C) { "\to checkperms-1.13 [supports more file formats]") // The LICENSE in the package Makefile is searched here. - t.CreateFileLines("licenses/bsd-2", + t.SetupFileLines("licenses/bsd-2", "# dummy") // The MASTER_SITES in the package Makefile are searched here. // See Pkgsrc.loadMasterSites. - t.CreateFileLines("mk/fetch/sites.mk", + t.SetupFileMkLines("mk/fetch/sites.mk", MkRcsID, "", "MASTER_SITE_GITHUB+=\thttps://github.com/") - // The options for the PKG_OPTIONS framework must be readable. - // See Pkgsrc.loadPkgOptions. - t.CreateFileLines("mk/defaults/options.description", - "option Description") - - // The user-defined variables are read in to check for missing - // BUILD_DEFS declarations in the package Makefile. - t.CreateFileLines("mk/defaults/mk.conf", - "# dummy") - - // The tool definitions are read in to check for missing - // USE_TOOLS declarations in the package Makefile. - // They spread over several files from the pkgsrc infrastructure. - t.CreateFileLines("mk/tools/bsd.tools.mk", - ".include \"defaults.mk\"") - t.CreateFileLines("mk/tools/defaults.mk", - "# dummy") - t.CreateFileLines("mk/bsd.prefs.mk", - "# dummy") - // The existence of this file makes the category "sysutils" valid. // The category "tools" on the other hand is not valid. - t.CreateFileLines("sysutils/Makefile", - "# dummy") + t.SetupFileMkLines("sysutils/Makefile", + MkRcsID) // The package Makefile is quite simple, containing just the // standard variable definitions. The data for checking the variable // values is partly defined in the pkgsrc infrastructure files // (as defined in the previous lines), and partly in the pkglint // code directly. Many details can be found in vartypecheck.go. - t.CreateFileLines("sysutils/checkperms/Makefile", + t.SetupFileMkLines("sysutils/checkperms/Makefile", MkRcsID, "", "DISTNAME=\tcheckperms-1.11", @@ -202,7 +178,7 @@ func (s *Suite) Test_Pkglint_Main__complete_package(c *check.C) { "", ".include \"../../mk/bsd.pkg.mk\"") - t.CreateFileLines("sysutils/checkperms/MESSAGE", + t.SetupFileLines("sysutils/checkperms/MESSAGE", "===========================================================================", RcsID, "", @@ -210,18 +186,18 @@ func (s *Suite) Test_Pkglint_Main__complete_package(c *check.C) { "", "===========================================================================") - t.CreateFileLines("sysutils/checkperms/PLIST", + t.SetupFileLines("sysutils/checkperms/PLIST", PlistRcsID, "bin/checkperms", "man/man1/checkperms.1") - t.CreateFileLines("sysutils/checkperms/README", + t.SetupFileLines("sysutils/checkperms/README", "When updating this package, test the pkgsrc bootstrap.") - t.CreateFileLines("sysutils/checkperms/TODO", + t.SetupFileLines("sysutils/checkperms/TODO", "Make the package work on MS-DOS") - t.CreateFileLines("sysutils/checkperms/patches/patch-checkperms.c", + t.SetupFileLines("sysutils/checkperms/patches/patch-checkperms.c", RcsID, "", "A simple patch demonstrating that pkglint checks for missing", @@ -234,7 +210,7 @@ func (s *Suite) Test_Pkglint_Main__complete_package(c *check.C) { "+// Header 1", "+// Header 2", "+// Header 3") - t.CreateFileLines("sysutils/checkperms/distinfo", + t.SetupFileLines("sysutils/checkperms/distinfo", RcsID, "", "SHA1 (checkperms-1.12.tar.gz) = 34c084b4d06bcd7a8bba922ff57677e651eeced5", @@ -243,7 +219,7 @@ func (s *Suite) Test_Pkglint_Main__complete_package(c *check.C) { "Size (checkperms-1.12.tar.gz) = 6621 bytes", "SHA1 (patch-checkperms.c) = asdfasdf") // Invalid SHA-1 checksum - G.Main("pkglint", "-Wall", "-Call", t.TempFilename("sysutils/checkperms")) + G.Main("pkglint", "-Wall", "-Call", t.File("sysutils/checkperms")) t.CheckOutputLines( "WARN: ~/sysutils/checkperms/Makefile:3: This package should be updated to 1.13 ([supports more file formats]).", @@ -276,7 +252,7 @@ func (s *Suite) Test_Pkglint_CheckDirent__outside(c *check.C) { t.SetupFileLines("empty") - G.CheckDirent(t.TmpDir()) + G.CheckDirent(t.File(".")) t.CheckOutputLines( "ERROR: ~: Cannot determine the pkgsrc root directory for \"~\".") @@ -290,22 +266,22 @@ func (s *Suite) Test_Pkglint_CheckDirent(c *check.C) { t.SetupFileLines("category/Makefile") t.SetupFileLines("Makefile") - G.CheckDirent(t.TmpDir()) + G.CheckDirent(t.File(".")) t.CheckOutputLines( "ERROR: ~/Makefile: Must not be empty.") - G.CheckDirent(t.TempFilename("category")) + G.CheckDirent(t.File("category")) t.CheckOutputLines( "ERROR: ~/category/Makefile: Must not be empty.") - G.CheckDirent(t.TempFilename("category/package")) + G.CheckDirent(t.File("category/package")) t.CheckOutputLines( "ERROR: ~/category/package/Makefile: Must not be empty.") - G.CheckDirent(t.TempFilename("category/package/nonexistent")) + G.CheckDirent(t.File("category/package/nonexistent")) t.CheckOutputLines( "ERROR: ~/category/package/nonexistent: No such file or directory.") @@ -437,9 +413,9 @@ func (s *Suite) Test_ChecklinesMessage__autofix(c *check.C) { func (s *Suite) Test_Pkgsrc_Latest_no_basedir(c *check.C) { t := s.Init(c) - latest1 := G.Pkgsrc.Latest("lang", `^python[0-9]+$`, "../../lang/$0") + latest := G.Pkgsrc.Latest("lang", `^python[0-9]+$`, "../../lang/$0") - c.Check(latest1, equals, "") + c.Check(latest, equals, "") t.CheckOutputLines( "ERROR: Cannot find latest version of \"^python[0-9]+$\" in \"~/lang\".") } @@ -449,9 +425,9 @@ func (s *Suite) Test_Pkgsrc_Latest_no_subdirs(c *check.C) { t.SetupFileLines("lang/Makefile") - latest2 := G.Pkgsrc.Latest("lang", `^python[0-9]+$`, "../../lang/$0") + latest := G.Pkgsrc.Latest("lang", `^python[0-9]+$`, "../../lang/$0") - c.Check(latest2, equals, "") + c.Check(latest, equals, "") t.CheckOutputLines( "ERROR: Cannot find latest version of \"^python[0-9]+$\" in \"~/lang\".") } @@ -462,9 +438,9 @@ func (s *Suite) Test_Pkgsrc_Latest_single(c *check.C) { t.SetupFileLines("lang/Makefile") t.SetupFileLines("lang/python27/Makefile") - latest3 := G.Pkgsrc.Latest("lang", `^python[0-9]+$`, "../../lang/$0") + latest := G.Pkgsrc.Latest("lang", `^python[0-9]+$`, "../../lang/$0") - c.Check(latest3, equals, "../../lang/python27") + c.Check(latest, equals, "../../lang/python27") } func (s *Suite) Test_Pkgsrc_Latest_multi(c *check.C) { @@ -474,9 +450,9 @@ func (s *Suite) Test_Pkgsrc_Latest_multi(c *check.C) { t.SetupFileLines("lang/python27/Makefile") t.SetupFileLines("lang/python35/Makefile") - latest4 := G.Pkgsrc.Latest("lang", `^python[0-9]+$`, "../../lang/$0") + latest := G.Pkgsrc.Latest("lang", `^python[0-9]+$`, "../../lang/$0") - c.Check(latest4, equals, "../../lang/python35") + c.Check(latest, equals, "../../lang/python35") } func (s *Suite) Test_Pkgsrc_Latest_numeric(c *check.C) { @@ -491,3 +467,20 @@ func (s *Suite) Test_Pkgsrc_Latest_numeric(c *check.C) { c.Check(latest, equals, "postgresql104") } + +// Demonstrates that an ALTERNATIVES file can be tested individually, +// without any dependencies on a whole package or a PLIST file. +func (s *Suite) Test_Pkglint_Checkfile__alternatives(c *check.C) { + t := s.Init(c) + + t.SetupPkgsrc() + lines := t.SetupFileLines("category/package/ALTERNATIVES", + "bin/tar @PREFIX@/bin/gnu-tar") + + G.Main("pkglint", lines[0].Filename) + + t.CheckOutputLines( + "NOTE: ~/category/package/ALTERNATIVES:1: @PREFIX@/ can be omitted from the file name.", + "Looks fine.", + "(Run \"pkglint -e\" to show explanations.)") +} diff --git a/pkgtools/pkglint/files/pkgsrc.go b/pkgtools/pkglint/files/pkgsrc.go index 46a6c214b04..47de3b5c55a 100644 --- a/pkgtools/pkglint/files/pkgsrc.go +++ b/pkgtools/pkglint/files/pkgsrc.go @@ -552,7 +552,7 @@ func (src *Pkgsrc) LoadExistingLines(fileName string, joinBackslashLines bool) [ // Example: // NewPkgsrc("/usr/pkgsrc").File("distfiles") => "/usr/pkgsrc/distfiles" func (src *Pkgsrc) File(relativeName string) string { - return src.topdir + "/" + relativeName + return cleanpath(src.topdir + "/" + relativeName) } // ToRel returns the path of `fileName`, relative to the pkgsrc top directory. diff --git a/pkgtools/pkglint/files/pkgsrc_test.go b/pkgtools/pkglint/files/pkgsrc_test.go index 981706e5558..14a389e00ef 100644 --- a/pkgtools/pkglint/files/pkgsrc_test.go +++ b/pkgtools/pkglint/files/pkgsrc_test.go @@ -33,7 +33,7 @@ func (s *Suite) Test_Pkgsrc_loadMasterSites(c *check.C) { func (s *Suite) Test_Pkgsrc_InitVartypes(c *check.C) { t := s.Init(c) - src := NewPkgsrc(t.TmpDir()) + src := NewPkgsrc(t.File(".")) src.InitVartypes() c.Check(src.vartypes["BSD_MAKE_ENV"].basicType.name, equals, "ShellWord") @@ -96,8 +96,6 @@ func (s *Suite) Test_Pkgsrc_loadTools(c *check.C) { "USE_TOOLS+=\tm4:pkgsrc") t.SetupFileLines("mk/bsd.pkg.mk", "USE_TOOLS+=\tmv") - G.CurrentDir = t.TmpDir() - G.CurPkgsrcdir = "." G.Pkgsrc.loadTools() @@ -135,7 +133,7 @@ func (s *Suite) Test_Pkgsrc_loadDocChangesFromFile(c *check.C) { "\tRemoved category/package successor category/package2 [author6 2018-01-06]", "\tDowngraded category/package to 1.2 [author7 2018-01-07]") - changes := G.Pkgsrc.loadDocChangesFromFile(t.TmpDir() + "/doc/CHANGES-2018") + changes := G.Pkgsrc.loadDocChangesFromFile(t.File("doc/CHANGES-2018")) c.Assert(len(changes), equals, 7) c.Check(*changes[0], equals, Change{changes[0].Line, "Added", "category/package", "1.0", "author1", "2015-01-01"}) diff --git a/pkgtools/pkglint/files/pkgver/vercmp_test.go b/pkgtools/pkglint/files/pkgver/vercmp_test.go index 0b04a628f0b..d0c8df07b9c 100644 --- a/pkgtools/pkglint/files/pkgver/vercmp_test.go +++ b/pkgtools/pkglint/files/pkgver/vercmp_test.go @@ -17,6 +17,7 @@ func (s *Suite) Test_newVersion(c *check.C) { c.Check(newVersion("5.0nb5"), check.DeepEquals, &version{[]int{5, 0, 0}, 5}) c.Check(newVersion("0.0.1-SNAPSHOT"), check.DeepEquals, &version{[]int{0, 0, 0, 0, 1, 19, 14, 1, 16, 19, 8, 15, 20}, 0}) c.Check(newVersion("1.0alpha3"), check.DeepEquals, &version{[]int{1, 0, 0, -3, 3}, 0}) + c.Check(newVersion("1_0alpha3"), check.DeepEquals, &version{[]int{1, 0, 0, -3, 3}, 0}) c.Check(newVersion("2.5beta"), check.DeepEquals, &version{[]int{2, 0, 5, -2}, 0}) c.Check(newVersion("20151110"), check.DeepEquals, &version{[]int{20151110}, 0}) c.Check(newVersion("0"), check.DeepEquals, &version{[]int{0}, 0}) @@ -37,7 +38,7 @@ func (s *Suite) Test_Compare(c *check.C) { {"1.0alpha3"}, {"1", "1.0", "1.0.0"}, {"1.0nb1"}, - {"1.0nb2"}, + {"1.0nb2", "1_0nb2"}, {"1.0.1a", "1.0.a1", "1.0.aa"}, {"1.0.1z"}, {"1.0.11", "1.0.k"}, @@ -52,23 +53,25 @@ func (s *Suite) Test_Compare(c *check.C) { {"20151110"}, } + checkVersion := func(i int, iversion string, j int, jversion string) { + actual := Compare(iversion, jversion) + switch { + case i < j && !(actual < 0): + c.Check([]interface{}{i, iversion, j, jversion, actual}, check.DeepEquals, []interface{}{i, iversion, j, jversion, "<0"}) + case i == j && !(actual == 0): + c.Check([]interface{}{i, iversion, j, jversion, actual}, check.DeepEquals, []interface{}{i, iversion, j, jversion, "==0"}) + case i > j && !(actual > 0): + c.Check([]interface{}{i, iversion, j, jversion, actual}, check.DeepEquals, []interface{}{i, iversion, j, jversion, ">0"}) + } + } + for i, iversions := range versions { - for _, iversion := range iversions { - for j, jversions := range versions { + for j, jversions := range versions { + for _, iversion := range iversions { for _, jversion := range jversions { - actual := Compare(iversion, jversion) - if i < j && !(actual < 0) { - c.Check([]interface{}{i, iversion, j, jversion, "<0"}, check.DeepEquals, []interface{}{i, iversion, j, jversion, actual}) - } - if i == j && !(actual == 0) { - c.Check([]interface{}{i, iversion, j, jversion, "==0"}, check.DeepEquals, []interface{}{i, iversion, j, jversion, actual}) - } - if i > j && !(actual > 0) { - c.Check([]interface{}{i, iversion, j, jversion, ">0"}, check.DeepEquals, []interface{}{i, iversion, j, jversion, actual}) - } + checkVersion(i, iversion, j, jversion) } } - } } } diff --git a/pkgtools/pkglint/files/shell.go b/pkgtools/pkglint/files/shell.go index 379cef7ca48..acb1b5bf553 100644 --- a/pkgtools/pkglint/files/shell.go +++ b/pkgtools/pkglint/files/shell.go @@ -338,27 +338,25 @@ func (shline *ShellLine) CheckShellCommand(shellcmd string, pSetE *bool) { spc := &ShellProgramChecker{shline} spc.checkConditionalCd(program) - (*MkShWalker).Walk(nil, program, func(node interface{}) { - if cmd, ok := node.(*MkShSimpleCommand); ok { - scc := NewSimpleCommandChecker(shline, cmd) - scc.Check() - if scc.strcmd.Name == "set" && scc.strcmd.AnyArgMatches(`^-.*e`) { - *pSetE = true - } - } - - if cmd, ok := node.(*MkShList); ok { - spc.checkSetE(cmd, pSetE) - } - - if cmd, ok := node.(*MkShPipeline); ok { - spc.checkPipeExitcode(line, cmd) + callback := NewMkShWalkCallback() + callback.SimpleCommand = func(command *MkShSimpleCommand) { + scc := NewSimpleCommandChecker(shline, command) + scc.Check() + if scc.strcmd.Name == "set" && scc.strcmd.AnyArgMatches(`^-.*e`) { + *pSetE = true } + } + callback.List = func(list *MkShList) { + spc.checkSetE(list, pSetE) + } + callback.Pipeline = func(pipeline *MkShPipeline) { + spc.checkPipeExitcode(line, pipeline) + } + callback.Word = func(word *ShToken) { + spc.checkWord(word, false) + } - if word, ok := node.(*ShToken); ok { - spc.checkWord(word, false) - } - }) + NewMkShWalker().Walk(program, callback) } func (shline *ShellLine) CheckShellCommands(shellcmds string) { @@ -743,20 +741,29 @@ func (spc *ShellProgramChecker) checkConditionalCd(list *MkShList) { } } - (*MkShWalker).Walk(nil, list, func(node interface{}) { - if cmd, ok := node.(*MkShIfClause); ok { - for _, cond := range cmd.Conds { - if simple := getSimple(cond); simple != nil { - checkConditionalCd(simple) - } - } - } - if cmd, ok := node.(*MkShLoopClause); ok { - if simple := getSimple(cmd.Cond); simple != nil { + callback := NewMkShWalkCallback() + callback.If = func(ifClause *MkShIfClause) { + for _, cond := range ifClause.Conds { + if simple := getSimple(cond); simple != nil { checkConditionalCd(simple) } } - }) + } + callback.Loop = func(loop *MkShLoopClause) { + if simple := getSimple(loop.Cond); simple != nil { + checkConditionalCd(simple) + } + } + callback.Pipeline = func(pipeline *MkShPipeline) { + if pipeline.Negated { + spc.shline.mkline.Warnf("The Solaris /bin/sh does not support negation of shell commands.") + Explain( + "The GNU Autoconf manual has many more details of what shell", + "features to avoid for portable programs. It can be read at:", + "https://www.gnu.org/software/autoconf/manual/autoconf.html#Limitations-of-Builtins") + } + } + NewMkShWalker().Walk(list, callback) } func (spc *ShellProgramChecker) checkWords(words []*ShToken, checkQuoting bool) { diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go index d8d5bb96231..896dffd284b 100644 --- a/pkgtools/pkglint/files/shell_test.go +++ b/pkgtools/pkglint/files/shell_test.go @@ -680,3 +680,33 @@ func (s *Suite) Test_ShellLine__variable_outside_quotes(c *check.C) { "WARN: dummy.mk:2: Unquoted shell variable \"comment\".", "WARN: dummy.mk:2: ECHO should not be evaluated indirectly at load time.") } + +func (s *Suite) Test_ShellLine_CheckShellCommand__cd_inside_if(c *check.C) { + t := s.Init(c) + + mklines := t.NewMkLines("Makefile", + MkRcsID, + "", + "\t${RUN} if cd /bin; then echo \"/bin exists.\"; fi") + + mklines.Check() + + t.CheckOutputLines( + "ERROR: Makefile:3: The Solaris /bin/sh cannot handle \"cd\" inside conditionals.", + "WARN: Makefile:3: Found absolute pathname: /bin") +} + +func (s *Suite) Test_ShellLine_CheckShellCommand__negated_pipe(c *check.C) { + t := s.Init(c) + + mklines := t.NewMkLines("Makefile", + MkRcsID, + "", + "\t${RUN} if ! test -f /etc/passwd; then echo \"passwd is missing.\"; fi") + + mklines.Check() + + t.CheckOutputLines( + "WARN: Makefile:3: The Solaris /bin/sh does not support negation of shell commands.", + "WARN: Makefile:3: Found absolute pathname: /etc/passwd") +} diff --git a/pkgtools/pkglint/files/tools_test.go b/pkgtools/pkglint/files/tools_test.go index 645f390222f..5511ab3b349 100644 --- a/pkgtools/pkglint/files/tools_test.go +++ b/pkgtools/pkglint/files/tools_test.go @@ -12,8 +12,7 @@ func (s *Suite) Test_ToolRegistry_ParseToolLine(c *check.C) { "", "USE_TOOLS.NetBSD+=\ttool1") - G.CurrentDir = t.TmpDir() - CheckdirToplevel() + CheckdirToplevel(t.File(".")) // No error about "Unknown tool \"NetBSD\"." t.CheckOutputEmpty() diff --git a/pkgtools/pkglint/files/toplevel.go b/pkgtools/pkglint/files/toplevel.go index 893dd47a343..5c43da10fd6 100644 --- a/pkgtools/pkglint/files/toplevel.go +++ b/pkgtools/pkglint/files/toplevel.go @@ -5,17 +5,18 @@ import ( ) type Toplevel struct { + dir string previousSubdir string subdirs []string } -func CheckdirToplevel() { +func CheckdirToplevel(dir string) { if trace.Tracing { - defer trace.Call1(G.CurrentDir)() + defer trace.Call1(dir)() } - ctx := new(Toplevel) - fname := G.CurrentDir + "/Makefile" + ctx := &Toplevel{dir, "", nil} + fname := dir + "/Makefile" lines := LoadNonemptyLines(fname, true) if lines == nil { @@ -48,7 +49,7 @@ func (ctx *Toplevel) checkSubdir(line Line, commentedOut bool, indentation, subd line.Warnf("Indentation should be a single tab character.") } - if contains(subdir, "$") || !fileExists(G.CurrentDir+"/"+subdir+"/Makefile") { + if contains(subdir, "$") || !fileExists(ctx.dir+"/"+subdir+"/Makefile") { return } @@ -66,6 +67,6 @@ func (ctx *Toplevel) checkSubdir(line Line, commentedOut bool, indentation, subd ctx.previousSubdir = subdir if !commentedOut { - ctx.subdirs = append(ctx.subdirs, G.CurrentDir+"/"+subdir) + ctx.subdirs = append(ctx.subdirs, ctx.dir+"/"+subdir) } } diff --git a/pkgtools/pkglint/files/toplevel_test.go b/pkgtools/pkglint/files/toplevel_test.go index 026fbf0ab53..1d9114cc146 100644 --- a/pkgtools/pkglint/files/toplevel_test.go +++ b/pkgtools/pkglint/files/toplevel_test.go @@ -21,8 +21,7 @@ func (s *Suite) Test_CheckdirToplevel(c *check.C) { t.SetupFileLines("x11/Makefile") t.SetupVartypes() - G.CurrentDir = t.TmpDir() - CheckdirToplevel() + CheckdirToplevel(t.File(".")) t.CheckOutputLines( "WARN: ~/Makefile:3: Indentation should be a single tab character.", diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go index 1ea5f6315b7..8e7afab5ed4 100644 --- a/pkgtools/pkglint/files/util.go +++ b/pkgtools/pkglint/files/util.go @@ -15,6 +15,18 @@ import ( "time" ) +type YesNoUnknown uint8 + +const ( + no YesNoUnknown = iota + yes + unknown +) + +func (ynu YesNoUnknown) String() string { + return [...]string{"no", "yes", "unknown"}[ynu] +} + // Short names for commonly used functions. func contains(s, substr string) bool { return strings.Contains(s, substr) @@ -313,7 +325,6 @@ func mkopSubst(s string, left bool, from string, right bool, to string, flags st } // relpath returns the relative path from `from` to `to`. -// If `to` is not within `from`, it panics. func relpath(from, to string) string { absFrom, err1 := filepath.Abs(from) absTo, err2 := filepath.Abs(to) @@ -348,6 +359,9 @@ func cleanpath(fname string) string { for contains(tmp, "/./") { tmp = strings.Replace(tmp, "/./", "/", -1) } + if len(tmp) > 2 && hasSuffix(tmp, "/.") { + tmp = tmp[:len(tmp)-2] + } for contains(tmp, "//") { tmp = strings.Replace(tmp, "//", "/", -1) } diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go index 91be65d52f2..0c9fe149761 100644 --- a/pkgtools/pkglint/files/util_test.go +++ b/pkgtools/pkglint/files/util_test.go @@ -7,6 +7,12 @@ import ( "testing" ) +func (s *Suite) Test_YesNoUnknown_String(c *check.C) { + c.Check(yes.String(), equals, "yes") + c.Check(no.String(), equals, "no") + c.Check(unknown.String(), equals, "unknown") +} + func (s *Suite) Test_MkopSubst__middle(c *check.C) { c.Check(mkopSubst("pkgname", false, "kgna", false, "ri", ""), equals, "prime") c.Check(mkopSubst("pkgname", false, "pkgname", false, "replacement", ""), equals, "replacement") @@ -68,20 +74,22 @@ func (s *Suite) Test_isEmptyDir_and_getSubdirs(c *check.C) { t.SetupFileLines("CVS/Entries", "dummy") - c.Check(isEmptyDir(t.TmpDir()), equals, true) - c.Check(getSubdirs(t.TmpDir()), check.DeepEquals, []string(nil)) + if dir := t.File("."); true { + c.Check(isEmptyDir(dir), equals, true) + c.Check(getSubdirs(dir), check.DeepEquals, []string(nil)) - t.SetupFileLines("somedir/file") + t.SetupFileLines("somedir/file") - c.Check(isEmptyDir(t.TmpDir()), equals, false) - c.Check(getSubdirs(t.TmpDir()), check.DeepEquals, []string{"somedir"}) + c.Check(isEmptyDir(dir), equals, false) + c.Check(getSubdirs(dir), check.DeepEquals, []string{"somedir"}) + } - if nodir := t.TmpDir() + "/nonexistent"; true { - c.Check(isEmptyDir(nodir), equals, true) // Counts as empty. + if absent := t.File("nonexistent"); true { + c.Check(isEmptyDir(absent), equals, true) // Counts as empty. defer t.ExpectFatalError(func() { c.Check(t.Output(), check.Matches, `FATAL: (.+): Cannot be read: open (.+): (.+)\n`) }) - c.Check(getSubdirs(nodir), check.DeepEquals, []string(nil)) + getSubdirs(absent) // Panics with a pkglintFatal. c.FailNow() } } diff --git a/pkgtools/pkglint/files/vardefs_test.go b/pkgtools/pkglint/files/vardefs_test.go new file mode 100644 index 00000000000..7cc38d70307 --- /dev/null +++ b/pkgtools/pkglint/files/vardefs_test.go @@ -0,0 +1,21 @@ +package main + +import "gopkg.in/check.v1" + +func (s *Suite) Test_InitVartypes__enumFrom(c *check.C) { + t := s.Init(c) + + t.SetupFileMkLines("editors/emacs/modules.mk", + MkRcsID, + "", + "_EMACS_VERSIONS_ALL=\temacs31", + "_EMACS_VERSIONS_ALL=\tignored") + mklines := t.SetupFileMkLines("Makefile", + MkRcsID, + "") + + t.SetupVartypes() + vartype := mklines.mklines[1].VariableType("EMACS_VERSIONS_ACCEPTED") + + c.Check(vartype.String(), equals, "ShellList of enum: emacs31 ") +} diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go index 40f4d81cd09..5bdf12381cf 100644 --- a/pkgtools/pkglint/files/vartypecheck.go +++ b/pkgtools/pkglint/files/vartypecheck.go @@ -139,7 +139,7 @@ func (cv *VartypeCheck) BuildlinkDepmethod() { } func (cv *VartypeCheck) Category() { - if cv.Value != "wip" && fileExists(G.CurrentDir+"/"+G.CurPkgsrcdir+"/"+cv.Value+"/Makefile") { + if cv.Value != "wip" && fileExists(G.Pkgsrc.File(cv.Value+"/Makefile")) { return } switch cv.Value { @@ -757,7 +757,8 @@ func (cv *VartypeCheck) PkgOptionsVar() { // A directory name relative to the top-level pkgsrc directory. // Despite its name, it is more similar to RelativePkgDir than to RelativePkgPath. func (cv *VartypeCheck) PkgPath() { - MkLineChecker{cv.MkLine}.CheckRelativePkgdir(G.CurPkgsrcdir + "/" + cv.Value) + pkgsrcdir := relpath(path.Dir(cv.MkLine.Filename), G.Pkgsrc.File(".")) + MkLineChecker{cv.MkLine}.CheckRelativePkgdir(pkgsrcdir + "/" + cv.Value) } func (cv *VartypeCheck) PkgRevision() { diff --git a/pkgtools/pkglint/files/vartypecheck_test.go b/pkgtools/pkglint/files/vartypecheck_test.go index cfa0160d793..267504cb536 100644 --- a/pkgtools/pkglint/files/vartypecheck_test.go +++ b/pkgtools/pkglint/files/vartypecheck_test.go @@ -46,8 +46,6 @@ func (s *Suite) Test_VartypeCheck_Category(c *check.C) { "# empty") t.SetupFileLines("wip/Makefile", "# empty") - G.CurrentDir = t.TmpDir() - G.CurPkgsrcdir = "." runVartypeChecks(t, "CATEGORIES", opAssign, (*VartypeCheck).Category, "chinese", @@ -168,14 +166,23 @@ func (s *Suite) Test_VartypeCheck_Dependency(c *check.C) { func (s *Suite) Test_VartypeCheck_DependencyWithPath(c *check.C) { t := s.Init(c) - t.SetupFileLines("x11/alacarte/Makefile", - "# empty") - t.SetupFileLines("category/package/Makefile", - "# empty") - G.CurrentDir = t.TmpDir() + "/category/package" - G.CurPkgsrcdir = "../.." + t.CreateFileLines("x11/alacarte/Makefile") + t.CreateFileLines("category/package/Makefile") + G.Pkg = NewPackage("category/package") + + // Since this test involves relative paths, the filename of the line must be realistic. + // Therefore this custom implementation of runVartypeChecks. + runChecks := func(values ...string) { + for i, value := range values { + mkline := t.NewMkLine(t.File("category/package/fname.mk"), i+1, "DEPENDS+=\t"+value) + mkline.Tokenize(mkline.Value()) + valueNovar := mkline.WithoutMakeVariables(mkline.Value()) + vc := &VartypeCheck{mkline, mkline.Line, mkline.Varname(), mkline.Op(), mkline.Value(), valueNovar, "", false} + (*VartypeCheck).DependencyWithPath(vc) + } + } - runVartypeChecks(t, "DEPENDS", opAssignAppend, (*VartypeCheck).DependencyWithPath, + runChecks( "Perl", "perl5>=5.22:../perl5", "perl5>=5.24:../../lang/perl5", @@ -190,19 +197,19 @@ func (s *Suite) Test_VartypeCheck_DependencyWithPath(c *check.C) { "gtk2+>=2.16:../../x11/alacarte") t.CheckOutputLines( - "WARN: fname:1: Unknown dependency pattern with path \"Perl\".", - "WARN: fname:2: Dependencies should have the form \"../../category/package\".", - "ERROR: fname:3: \"../../lang/perl5\" does not exist.", - "ERROR: fname:3: There is no package in \"lang/perl5\".", - "WARN: fname:3: Please use USE_TOOLS+=perl:run instead of this dependency.", - "WARN: fname:4: Unknown dependency pattern \"broken0.12.1\".", - "WARN: fname:5: Unknown dependency pattern \"broken[0-9]*\".", - "WARN: fname:6: Unknown dependency pattern with path \"broken[0-9]*../../x11/alacarte\".", - "WARN: fname:7: Unknown dependency pattern \"broken>=\".", - "WARN: fname:8: Unknown dependency pattern \"broken=0\".", - "WARN: fname:9: Unknown dependency pattern \"broken=\".", - "WARN: fname:10: Unknown dependency pattern \"broken-\".", - "WARN: fname:11: Unknown dependency pattern \"broken>\".") + "WARN: ~/category/package/fname.mk:1: Unknown dependency pattern with path \"Perl\".", + "WARN: ~/category/package/fname.mk:2: Dependencies should have the form \"../../category/package\".", + "ERROR: ~/category/package/fname.mk:3: \"../../lang/perl5\" does not exist.", + "ERROR: ~/category/package/fname.mk:3: There is no package in \"lang/perl5\".", + "WARN: ~/category/package/fname.mk:3: Please use USE_TOOLS+=perl:run instead of this dependency.", + "WARN: ~/category/package/fname.mk:4: Unknown dependency pattern \"broken0.12.1\".", + "WARN: ~/category/package/fname.mk:5: Unknown dependency pattern \"broken[0-9]*\".", + "WARN: ~/category/package/fname.mk:6: Unknown dependency pattern with path \"broken[0-9]*../../x11/alacarte\".", + "WARN: ~/category/package/fname.mk:7: Unknown dependency pattern \"broken>=\".", + "WARN: ~/category/package/fname.mk:8: Unknown dependency pattern \"broken=0\".", + "WARN: ~/category/package/fname.mk:9: Unknown dependency pattern \"broken=\".", + "WARN: ~/category/package/fname.mk:10: Unknown dependency pattern \"broken-\".", + "WARN: ~/category/package/fname.mk:11: Unknown dependency pattern \"broken>\".") } func (s *Suite) Test_VartypeCheck_DistSuffix(c *check.C) { @@ -254,6 +261,26 @@ func (s *Suite) Test_VartypeCheck_Enum(c *check.C) { "WARN: fname:3: The pattern \"sun-jdk*\" cannot match any of { jdk1 jdk2 jdk4 } for JDK.") } +func (s *Suite) Test_VartypeCheck_Enum__use_match(c *check.C) { + t := s.Init(c) + + t.SetupVartypes() + + mklines := t.NewMkLines("module.mk", + MkRcsID, + "", + ".if !empty(MACHINE_ARCH:Mi386) || ${MACHINE_ARCH} == i386", + ".endif", + ".if !empty(PKGSRC_COMPILER:Mclang) || ${PKGSRC_COMPILER} == clang", + ".endif") + + mklines.Check() + + t.CheckOutputLines( + "NOTE: module.mk:3: MACHINE_ARCH should be compared using == instead of the :M or :N modifier without wildcards.", + "WARN: module.mk:5: Use ${PKGSRC_COMPILER:Mclang} instead of the == operator.") +} + func (s *Suite) Test_VartypeCheck_FetchURL(c *check.C) { t := s.Init(c) -- cgit v1.2.3