diff options
| author | rillig <rillig@pkgsrc.org> | 2019-12-08 22:03:37 +0000 |
|---|---|---|
| committer | rillig <rillig@pkgsrc.org> | 2019-12-08 22:03:37 +0000 |
| commit | 2411eff2d1c769d9506a59a806aabc4247a22609 (patch) | |
| tree | fcd9b8354fed69c94bd85987cec26b940bb8e87b /pkgtools | |
| parent | a4478eb271afee37db5026d147e23429eb33f636 (diff) | |
| download | pkgsrc-2411eff2d1c769d9506a59a806aabc4247a22609.tar.gz | |
pkgtools/pkglint: update pkglint to 19.3.15
Changes since 19.3.14:
Invalid lines in PLIST files are now reported as errors instead of
warnings. If pkglint doesn't know about it, it must be an error.
In PLIST files, all paths are validated to be canonical. That is, no
dotdot components, no absolute paths, no extra slashes, no intermediate
dot components.
Fewer notes for unexpanded variable expressions in DESCR files. Before,
the text $@ was reported as possible Makefile variable even though it
was just a Perl expression.
README files are allowed again in pkgsrc package directories. There was
no convincing argument why these should be forbidden.
A few diagnostics have been changed from NOTE to WARNING or from WARNING
to ERROR, to match their wording.
When pkglint suggests to replace :M with ==, the wording is now "can be
made" instead of "should".
Diffstat (limited to 'pkgtools')
49 files changed, 1473 insertions, 730 deletions
diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index 533afeb42e9..e3ff5dd27fb 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.614 2019/12/08 00:06:37 rillig Exp $ +# $NetBSD: Makefile,v 1.615 2019/12/08 22:03:37 rillig Exp $ -PKGNAME= pkglint-19.3.14 +PKGNAME= pkglint-19.3.15 CATEGORIES= pkgtools DISTNAME= tools MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/} diff --git a/pkgtools/pkglint/files/autofix_test.go b/pkgtools/pkglint/files/autofix_test.go index 6e9c2d9283b..7d0ede66118 100644 --- a/pkgtools/pkglint/files/autofix_test.go +++ b/pkgtools/pkglint/files/autofix_test.go @@ -164,7 +164,8 @@ func (s *Suite) Test_Autofix__lonely_source(c *check.C) { t.CheckOutputLines( ">\tPRE_XORGPROTO_LIST_MISSING =\tapplewmproto", - "NOTE: x11/xorgproto/builtin.mk:5: Unnecessary space after variable name \"PRE_XORGPROTO_LIST_MISSING\".") + "NOTE: x11/xorg-cf-files/../../x11/xorgproto/builtin.mk:5: "+ + "Unnecessary space after variable name \"PRE_XORGPROTO_LIST_MISSING\".") } // Up to 2018-11-26, pkglint in some cases logged only the source without @@ -557,7 +558,7 @@ func (s *Suite) Test_Autofix_ReplaceAt(c *check.C) { lines := func(lines ...string) []string { return lines } test := func(texts []string, rawIndex int, column int, from, to string, diagnostics ...string) { - mainPart := func() { + mainPart := func(autofix bool) { mklines := t.NewMkLines("filename.mk", texts...) assert(len(mklines.mklines) == 1) mkline := mklines.mklines[0] diff --git a/pkgtools/pkglint/files/buildlink3.go b/pkgtools/pkglint/files/buildlink3.go index 63a4e5b7802..a59df037849 100644 --- a/pkgtools/pkglint/files/buildlink3.go +++ b/pkgtools/pkglint/files/buildlink3.go @@ -99,7 +99,7 @@ func (ck *Buildlink3Checker) checkUniquePkgbase(pkgbase string, mkline *MkLine) return } - dirname := G.Pkgsrc.ToRel(mkline.Filename.DirNoClean()).Base() + dirname := G.Pkgsrc.Rel(mkline.Filename.DirNoClean()).Base() base, name := trimCommon(pkgbase, dirname) if base == "" && matches(name, `^(\d*|-cvs|-fossil|-git|-hg|-svn|-devel|-snapshot)$`) { return diff --git a/pkgtools/pkglint/files/category.go b/pkgtools/pkglint/files/category.go index ee7662e4e45..0f8e540f7ee 100644 --- a/pkgtools/pkglint/files/category.go +++ b/pkgtools/pkglint/files/category.go @@ -148,7 +148,7 @@ func CheckdirCategory(dir CurrPath) { mlex.SkipEmptyOrNote() mlex.SkipContainsOrWarn(".include \"../mk/misc/category.mk\"") if !mlex.EOF() { - mlex.CurrentLine().Errorf("The file should end here.") + mlex.CurrentLine().Errorf("The file must end here.") } } diff --git a/pkgtools/pkglint/files/category_test.go b/pkgtools/pkglint/files/category_test.go index 96b15d6dfac..262ab2358a9 100644 --- a/pkgtools/pkglint/files/category_test.go +++ b/pkgtools/pkglint/files/category_test.go @@ -33,7 +33,7 @@ func (s *Suite) Test_CheckdirCategory__totally_broken(c *check.C) { "ERROR: ~/archivers/Makefile:3: \"aaaaa\" exists in the Makefile but not in the file system.", "NOTE: ~/archivers/Makefile:3: Empty line expected after this line.", "WARN: ~/archivers/Makefile:4: This line should contain the following text: .include \"../mk/misc/category.mk\"", - "ERROR: ~/archivers/Makefile:4: The file should end here.") + "ERROR: ~/archivers/Makefile:4: The file must end here.") } func (s *Suite) Test_CheckdirCategory__invalid_comment(c *check.C) { @@ -305,7 +305,7 @@ func (s *Suite) Test_CheckdirCategory__comment_at_the_top(c *check.C) { "ERROR: ~/category/Makefile:3: \"package\" exists in the file system but not in the Makefile.", "NOTE: ~/category/Makefile:2: Empty line expected after this line.", "WARN: ~/category/Makefile:3: This line should contain the following text: .include \"../mk/misc/category.mk\"", - "ERROR: ~/category/Makefile:3: The file should end here.") + "ERROR: ~/category/Makefile:3: The file must end here.") } func (s *Suite) Test_CheckdirCategory__unexpected_EOF_while_reading_SUBDIR(c *check.C) { diff --git a/pkgtools/pkglint/files/check_test.go b/pkgtools/pkglint/files/check_test.go index 9f2438f8c0d..91b13e4c0bf 100644 --- a/pkgtools/pkglint/files/check_test.go +++ b/pkgtools/pkglint/files/check_test.go @@ -102,8 +102,8 @@ func (s *Suite) TearDownTest(c *check.C) { // Ensures that all test names follow a common naming scheme: // // Test_${Type}_${Method}__${description_using_underscores} -func (s *Suite) Test__qa(c *check.C) { - ck := intqa.NewQAChecker(c.Errorf) +func Test__qa(t *testing.T) { + ck := intqa.NewQAChecker(t.Errorf) ck.Configure("autofix.go", "*", "*", -intqa.EMissingTest) // TODO ck.Configure("buildlink3.go", "*", "*", -intqa.EMissingTest) // TODO @@ -131,7 +131,6 @@ func (s *Suite) Test__qa(c *check.C) { ck.Configure("patches.go", "*", "*", -intqa.EMissingTest) // TODO ck.Configure("pkglint.go", "*", "*", -intqa.EMissingTest) // TODO ck.Configure("pkgsrc.go", "*", "*", -intqa.EMissingTest) // TODO - ck.Configure("plist.go", "*", "*", -intqa.EMissingTest) // TODO ck.Configure("redundantscope.go", "*", "*", -intqa.EMissingTest) // TODO ck.Configure("shell.go", "*", "*", -intqa.EMissingTest) // TODO ck.Configure("shtokenizer.go", "*", "*", -intqa.EMissingTest) // TODO @@ -144,7 +143,6 @@ func (s *Suite) Test__qa(c *check.C) { ck.Configure("vardefs.go", "*", "*", -intqa.EMissingTest) // TODO ck.Configure("vargroups.go", "*", "*", -intqa.EMissingTest) // TODO ck.Configure("vartype.go", "*", "*", -intqa.EMissingTest) // TODO - ck.Configure("vartypecheck.go", "*", "*", -intqa.EMissingTest) // TODO // For now, don't require tests for all the test code. // Having good coverage for the main code is more important. @@ -262,6 +260,27 @@ func (t *Tester) SetUpTool(name, varname string, validity Validity) *Tool { return G.Pkgsrc.Tools.def(name, varname, false, validity, nil) } +// SetUpType defines a variable to have a certain type and access permissions, +// like in the type definitions in vardefs.go. +// +// Example: +// SetUpType("PKGPATH", BtPkgpath, DefinedIfInScope|NonemptyIfDefined, +// "Makefile, *.mk: default, set, append, use, use-loadtime") +func (t *Tester) SetUpType(varname string, basicType *BasicType, + options vartypeOptions, aclEntries ...string) { + + if len(aclEntries) == 0 { + aclEntries = []string{"Makefile, *.mk: default, set, append, use, use-loadtime"} + } + + G.Pkgsrc.vartypes.acl(varname, basicType, options, aclEntries...) + + // Make sure that registering the type succeeds. + // This is necessary for BtUnknown and guessed types. + vartype := G.Pkgsrc.VariableType(nil, varname) + t.c.Assert(vartype.basicType, check.Equals, basicType) +} + // SetUpFileLines creates a temporary file and writes the given lines to it. // The file is then read in, without interpreting line continuations. // @@ -399,7 +418,7 @@ func (t *Tester) SetUpPkgsrc() { // SetUpCategory makes the given category valid by creating a dummy Makefile. // After that, it can be mentioned in the CATEGORIES variable of a package. func (t *Tester) SetUpCategory(name RelPath) { - assert(G.Pkgsrc.ToRel(t.File(name)).Count() == 1) + assert(G.Pkgsrc.Rel(t.File(name)).Count() == 1) makefile := name.JoinNoClean("Makefile") if !t.File(makefile).IsFile() { @@ -535,7 +554,7 @@ func (t *Tester) CreateFileLines(filename RelPath, lines ...string) CurrPath { // temporary directory. func (t *Tester) CreateFileDummyPatch(filename RelPath) { // Patch files only make sense in category/package/patches directories. - assert(G.Pkgsrc.ToRel(t.File(filename)).Count() == 4) + assert(G.Pkgsrc.Rel(t.File(filename)).Count() == 4) t.CreateFileLines(filename, CvsID, @@ -551,7 +570,7 @@ func (t *Tester) CreateFileDummyPatch(filename RelPath) { func (t *Tester) CreateFileBuildlink3(filename RelPath, customLines ...string) { // Buildlink3.mk files only make sense in category/package directories. - assert(G.Pkgsrc.ToRel(t.File(filename)).Count() == 3) + assert(G.Pkgsrc.Rel(t.File(filename)).Count() == 3) dir := filename.DirClean() lower := dir.Base() @@ -924,12 +943,12 @@ func (t *Tester) ExpectAssert(action func()) { // ExpectDiagnosticsAutofix first runs the given action with -Wall, and // then another time with -Wall --autofix. -func (t *Tester) ExpectDiagnosticsAutofix(action func(), diagnostics ...string) { +func (t *Tester) ExpectDiagnosticsAutofix(action func(autofix bool), diagnostics ...string) { t.SetUpCommandLine("-Wall") - action() + action(false) t.SetUpCommandLine("-Wall", "--autofix") - action() + action(true) t.CheckOutput(diagnostics) } diff --git a/pkgtools/pkglint/files/logging.go b/pkgtools/pkglint/files/logging.go index c0d5cab1346..43fc43e64f1 100644 --- a/pkgtools/pkglint/files/logging.go +++ b/pkgtools/pkglint/files/logging.go @@ -255,6 +255,15 @@ func (l *Logger) Logf(level *LogLevel, filename CurrPath, lineno, format, msg st return } + if G.Testing { + if level != Error { + assertf(!contains(format, "must"), "Must must only appear in errors: %s", format) + } + if level != Warn && level != Note { + assertf(!contains(format, "should"), "Should must only appear in warnings: %s", format) + } + } + if G.Testing && format != AutofixFormat { if textproc.Alpha.Contains(format[0]) { assertf( diff --git a/pkgtools/pkglint/files/logging_test.go b/pkgtools/pkglint/files/logging_test.go index c346f468c1c..72022cc5167 100644 --- a/pkgtools/pkglint/files/logging_test.go +++ b/pkgtools/pkglint/files/logging_test.go @@ -172,11 +172,11 @@ func (s *Suite) Test_Logger_Diag__duplicates(c *check.C) { logger := Logger{out: NewSeparatorWriter(&sw)} line := t.NewLine("filename", 3, "Text") - logger.Diag(line, Error, "Blue should be %s.", "orange") - logger.Diag(line, Error, "Blue should be %s.", "orange") + logger.Diag(line, Error, "Blue must be %s.", "orange") + logger.Diag(line, Error, "Blue must be %s.", "orange") t.CheckEquals(sw.String(), ""+ - "ERROR: filename:3: Blue should be orange.\n") + "ERROR: filename:3: Blue must be orange.\n") } // Explanations are associated with their diagnostics. Therefore, when one @@ -189,22 +189,22 @@ func (s *Suite) Test_Logger_Diag__explanation(c *check.C) { logger.Opts.Explain = true line := t.NewLine("filename", 3, "Text") - logger.Diag(line, Error, "Blue should be %s.", "orange") + logger.Diag(line, Error, "Blue must be %s.", "orange") logger.Explain( "The colors have changed.") - logger.Diag(line, Error, "Blue should be %s.", "orange") + logger.Diag(line, Error, "Blue must be %s.", "orange") logger.Explain( "The colors have changed.") // Even when the text of the explanation is not the same, it is still // suppressed since it belongs to the diagnostic. - logger.Diag(line, Error, "Blue should be %s.", "orange") + logger.Diag(line, Error, "Blue must be %s.", "orange") logger.Explain( "The colors have further changed.") t.CheckEquals(sw.String(), ""+ - "ERROR: filename:3: Blue should be orange.\n"+ + "ERROR: filename:3: Blue must be orange.\n"+ "\n"+ "\tThe colors have changed.\n"+ "\n") @@ -571,10 +571,10 @@ func (s *Suite) Test_Logger_Logf(c *check.C) { var sw strings.Builder logger := Logger{out: NewSeparatorWriter(&sw)} - logger.Logf(Error, "filename", "3", "Blue should be %s.", "Blue should be orange.") + logger.Logf(Error, "filename", "3", "Blue must be %s.", "Blue must be orange.") t.CheckEquals(sw.String(), ""+ - "ERROR: filename:3: Blue should be orange.\n") + "ERROR: filename:3: Blue must be orange.\n") } // Logf doesn't filter duplicates, but Diag does. @@ -584,12 +584,12 @@ func (s *Suite) Test_Logger_Logf__duplicates(c *check.C) { var sw strings.Builder logger := Logger{out: NewSeparatorWriter(&sw)} - logger.Logf(Error, "filename", "3", "Blue should be %s.", "Blue should be orange.") - logger.Logf(Error, "filename", "3", "Blue should be %s.", "Blue should be orange.") + logger.Logf(Error, "filename", "3", "Blue must be %s.", "Blue must be orange.") + logger.Logf(Error, "filename", "3", "Blue must be %s.", "Blue must be orange.") t.CheckEquals(sw.String(), ""+ - "ERROR: filename:3: Blue should be orange.\n"+ - "ERROR: filename:3: Blue should be orange.\n") + "ERROR: filename:3: Blue must be orange.\n"+ + "ERROR: filename:3: Blue must be orange.\n") } // Ensure that suppressing a diagnostic doesn't influence later calls to Logf. diff --git a/pkgtools/pkglint/files/mkassignchecker.go b/pkgtools/pkglint/files/mkassignchecker.go index 00bbca246a5..505427601d5 100644 --- a/pkgtools/pkglint/files/mkassignchecker.go +++ b/pkgtools/pkglint/files/mkassignchecker.go @@ -56,7 +56,7 @@ func (ck *MkAssignChecker) checkVarassignLeftNotUsed() { return } - if ck.MkLines.vars.IsUsedSimilar(varname) { + if ck.MkLines.allVars.IsUsedSimilar(varname) { return } @@ -384,7 +384,7 @@ func (ck *MkAssignChecker) checkVarassignRightCategory() { categories := mkline.ValueFields(mkline.Value()) actual := categories[0] - expected := G.Pkgsrc.ToRel(mkline.Filename).DirNoClean().DirNoClean().Base() + expected := G.Pkgsrc.Rel(mkline.Filename).DirNoClean().DirNoClean().Base() if expected == "wip" || actual == expected { return diff --git a/pkgtools/pkglint/files/mkassignchecker_test.go b/pkgtools/pkglint/files/mkassignchecker_test.go index 48cea7149cc..b85d6ef9b21 100644 --- a/pkgtools/pkglint/files/mkassignchecker_test.go +++ b/pkgtools/pkglint/files/mkassignchecker_test.go @@ -89,7 +89,7 @@ func (s *Suite) Test_MkAssignChecker_checkVarassignLeft(c *check.C) { MkCvsID, "_VARNAME=\tvalue") // Only to prevent "defined but not used". - mklines.vars.Use("_VARNAME", mklines.mklines[1], VucRunTime) + mklines.allVars.Use("_VARNAME", mklines.mklines[1], VucRunTime) mklines.Check() @@ -380,7 +380,7 @@ func (s *Suite) Test_MkAssignChecker_checkVarassignLeftUserSettable__after_prefs func (s *Suite) Test_MkAssignChecker_checkVarassignLeftUserSettable__vartype_nil(c *check.C) { t := s.Init(c) - t.CreateFileLines("category/package/vars.mk", + t.CreateFileLines("category/package/allVars.mk", MkCvsID, "#", "# User-settable variables:", @@ -651,7 +651,7 @@ func (s *Suite) Test_MkAssignChecker_checkVarassignOpShell(c *check.C) { "", "MUST_BE_EARLY!=\techo 123 # must be evaluated early", "", - "show-package-vars: .PHONY", + "show-package-allVars: .PHONY", "\techo OS_NAME=${OS_NAME:Q}", "\techo MUST_BE_EARLY=${MUST_BE_EARLY:Q}") t.FinishSetUp() diff --git a/pkgtools/pkglint/files/mkcondchecker.go b/pkgtools/pkglint/files/mkcondchecker.go index 50d0562d17e..0f3de4cb53a 100644 --- a/pkgtools/pkglint/files/mkcondchecker.go +++ b/pkgtools/pkglint/files/mkcondchecker.go @@ -14,7 +14,7 @@ func NewMkCondChecker(mkLine *MkLine, mkLines *MkLines) *MkCondChecker { return &MkCondChecker{MkLine: mkLine, MkLines: mkLines} } -func (ck *MkCondChecker) checkDirectiveCond() { +func (ck *MkCondChecker) Check() { mkline := ck.MkLine if trace.Tracing { defer trace.Call1(mkline.Args())() @@ -39,26 +39,26 @@ func (ck *MkCondChecker) checkDirectiveCond() { checkNotEmpty := func(not *MkCond) { empty := not.Empty if empty != nil { - ck.checkDirectiveCondEmpty(empty, true, true) + ck.checkEmpty(empty, true, true) done[empty] = true } if not.Term != nil && not.Term.Var != nil { varUse := not.Term.Var - ck.checkDirectiveCondEmpty(varUse, false, false) + ck.checkEmpty(varUse, false, false) done[varUse] = true } } checkEmpty := func(empty *MkVarUse) { if !done[empty] { - ck.checkDirectiveCondEmpty(empty, true, false) + ck.checkEmpty(empty, true, false) } } checkVar := func(varUse *MkVarUse) { if !done[varUse] { - ck.checkDirectiveCondEmpty(varUse, false, true) + ck.checkEmpty(varUse, false, true) } } @@ -66,19 +66,19 @@ func (ck *MkCondChecker) checkDirectiveCond() { Not: checkNotEmpty, Empty: checkEmpty, Var: checkVar, - Compare: ck.checkDirectiveCondCompare, + Compare: ck.checkCompare, VarUse: checkVarUse}) } -// checkDirectiveCondEmpty checks a condition of the form empty(VAR), +// checkEmpty checks a condition of the form empty(VAR), // empty(VAR:Mpattern) or ${VAR:Mpattern} in an .if directive. -func (ck *MkCondChecker) checkDirectiveCondEmpty(varuse *MkVarUse, fromEmpty bool, neg bool) { - ck.checkDirectiveCondEmptyExpr(varuse) - ck.checkDirectiveCondEmptyType(varuse) - ck.simplifyCondition(varuse, fromEmpty, neg) +func (ck *MkCondChecker) checkEmpty(varuse *MkVarUse, fromEmpty bool, neg bool) { + ck.checkEmptyExpr(varuse) + ck.checkEmptyType(varuse) + ck.simplify(varuse, fromEmpty, neg) } -func (ck *MkCondChecker) checkDirectiveCondEmptyExpr(varuse *MkVarUse) { +func (ck *MkCondChecker) checkEmptyExpr(varuse *MkVarUse) { if !matches(varuse.varname, `^\$.*:[MN]`) { return } @@ -97,7 +97,7 @@ func (ck *MkCondChecker) checkDirectiveCondEmptyExpr(varuse *MkVarUse) { "\t${VARNAME:Mpattern}") } -func (ck *MkCondChecker) checkDirectiveCondEmptyType(varuse *MkVarUse) { +func (ck *MkCondChecker) checkEmptyType(varuse *MkVarUse) { for _, modifier := range varuse.modifiers { ok, _, pattern, _ := modifier.MatchMatch() if ok { @@ -123,14 +123,14 @@ var mkCondStringLiteralUnquoted = textproc.NewByteSet("+---./0-9@A-Z_a-z") // that are interpreted literally in the :M and :N modifiers. var mkCondModifierPatternLiteral = textproc.NewByteSet("+---./0-9<=>@A-Z_a-z") -// simplifyCondition replaces an unnecessarily complex condition with +// simplify replaces an unnecessarily complex condition with // a simpler condition that's still equivalent. // // * fromEmpty is true for the form empty(VAR...), and false for ${VAR...}. // // * neg is true for the form !empty(VAR...), and false for empty(VAR...). // It also applies to the ${VAR} form. -func (ck *MkCondChecker) simplifyCondition(varuse *MkVarUse, fromEmpty bool, neg bool) { +func (ck *MkCondChecker) simplify(varuse *MkVarUse, fromEmpty bool, neg bool) { varname := varuse.varname mods := varuse.modifiers modifiers := mods @@ -147,7 +147,8 @@ func (ck *MkCondChecker) simplifyCondition(varuse *MkVarUse, fromEmpty bool, neg return true } - if ck.MkLines.vars.IsDefined(varname) { + // TODO: Use ck.MkLines.loadVars instead. + if ck.MkLines.allVars.IsDefined(varname) { return true } @@ -218,7 +219,8 @@ func (ck *MkCondChecker) simplifyCondition(varuse *MkVarUse, fromEmpty bool, neg } fix := ck.MkLine.Autofix() - fix.Notef("%s should be compared using \"%s\" instead of matching against %q.", + fix.Notef("%s can be compared using the simpler \"%s\" "+ + "instead of matching against %q.", varname, to, ":"+modifier.Text) fix.Explain( "This variable has a single value, not a list of values.", @@ -232,19 +234,24 @@ func (ck *MkCondChecker) simplifyCondition(varuse *MkVarUse, fromEmpty bool, neg fix.Apply() } -func (ck *MkCondChecker) checkDirectiveCondCompare(left *MkCondTerm, op string, right *MkCondTerm) { +func (ck *MkCondChecker) checkCompare(left *MkCondTerm, op string, right *MkCondTerm) { switch { case left.Var != nil && right.Var == nil && right.Num == "": - ck.checkDirectiveCondCompareVarStr(left.Var, op, right.Str) + ck.checkCompareVarStr(left.Var, op, right.Str) } } -func (ck *MkCondChecker) checkDirectiveCondCompareVarStr(varuse *MkVarUse, op string, str string) { +func (ck *MkCondChecker) checkCompareVarStr(varuse *MkVarUse, op string, str string) { varname := varuse.varname varmods := varuse.modifiers switch len(varmods) { case 0: - ck.checkCompareVarStr(varname, op, str) + mkLineChecker := NewMkLineChecker(ck.MkLines, ck.MkLine) + mkLineChecker.checkVartype(varname, opUseCompare, str, "") + + if varname == "PKGSRC_COMPILER" { + ck.checkCompareVarStrCompiler(op, str) + } case 1: if m, _, pattern, _ := varmods[0].MatchMatch(); m { @@ -272,15 +279,6 @@ func (ck *MkCondChecker) checkDirectiveCondCompareVarStr(varuse *MkVarUse, op st } } -func (ck *MkCondChecker) checkCompareVarStr(varname, op, value string) { - mkLineChecker := NewMkLineChecker(ck.MkLines, ck.MkLine) - mkLineChecker.checkVartype(varname, opUseCompare, value, "") - - if varname == "PKGSRC_COMPILER" { - ck.checkCompareVarStrCompiler(op, value) - } -} - func (ck *MkCondChecker) checkCompareVarStrCompiler(op string, value string) { if !matches(value, `^\w+$`) { return diff --git a/pkgtools/pkglint/files/mkcondchecker_test.go b/pkgtools/pkglint/files/mkcondchecker_test.go index 1c11e78b7e0..6b55aaaa898 100644 --- a/pkgtools/pkglint/files/mkcondchecker_test.go +++ b/pkgtools/pkglint/files/mkcondchecker_test.go @@ -14,7 +14,7 @@ func (s *Suite) Test_NewMkCondChecker(c *check.C) { t.CheckEquals(ck.MkLines, mklines) } -func (s *Suite) Test_MkCondChecker_checkDirectiveCond(c *check.C) { +func (s *Suite) Test_MkCondChecker_Check(c *check.C) { t := s.Init(c) t.SetUpPkgsrc() @@ -65,8 +65,8 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCond(c *check.C) { // PKG_LIBTOOL is only available after including bsd.pkg.mk, // therefore the :U and the subsequent warning. test(".if ${PKG_LIBTOOL:U:Mlibtool}", - "NOTE: filename.mk:4: PKG_LIBTOOL "+ - "should be compared using \"${PKG_LIBTOOL:U} == libtool\" "+ + "NOTE: filename.mk:4: PKG_LIBTOOL can be "+ + "compared using the simpler \"${PKG_LIBTOOL:U} == libtool\" "+ "instead of matching against \":Mlibtool\".", "WARN: filename.mk:4: PKG_LIBTOOL should not be used at load time in any file.") @@ -84,8 +84,8 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCond(c *check.C) { "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.", - "NOTE: filename.mk:4: MACHINE_ARCH "+ - "should be compared using \"${MACHINE_ARCH} == x86\" "+ + "NOTE: filename.mk:4: MACHINE_ARCH can be "+ + "compared using the simpler \"${MACHINE_ARCH} == x86\" "+ "instead of matching against \":Mx86\".") // Doesn't occur in practice since it is surprising that the ! applies @@ -127,7 +127,7 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCond(c *check.C) { "WARN: filename.mk:4: MASTER_SITES should not be used at load time in any file.") } -func (s *Suite) Test_MkCondChecker_checkDirectiveCond__tracing(c *check.C) { +func (s *Suite) Test_MkCondChecker_Check__tracing(c *check.C) { t := s.Init(c) t.EnableTracingToLog() @@ -143,7 +143,7 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCond__tracing(c *check.C) { "WARN: filename.mk:2: VAR is used but not defined.") } -func (s *Suite) Test_MkCondChecker_checkDirectiveCond__comparison_with_shell_command(c *check.C) { +func (s *Suite) Test_MkCondChecker_Check__comparison_with_shell_command(c *check.C) { t := s.Init(c) t.SetUpPkgsrc() @@ -167,7 +167,7 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCond__comparison_with_shell_com // The :N modifier filters unwanted values. After this filter, any variable value // may be compared with the empty string, regardless of the variable type. // Effectively, the :N modifier changes the type from T to Option(T). -func (s *Suite) Test_MkCondChecker_checkDirectiveCond__compare_pattern_with_empty(c *check.C) { +func (s *Suite) Test_MkCondChecker_Check__compare_pattern_with_empty(c *check.C) { t := s.Init(c) t.SetUpPkgsrc() @@ -196,7 +196,7 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCond__compare_pattern_with_empt "WARN: filename.mk:8: The pathname \"*\" contains the invalid character \"*\".") } -func (s *Suite) Test_MkCondChecker_checkDirectiveCond__comparing_PKGSRC_COMPILER_with_eqeq(c *check.C) { +func (s *Suite) Test_MkCondChecker_Check__comparing_PKGSRC_COMPILER_with_eqeq(c *check.C) { t := s.Init(c) t.SetUpPkgsrc() @@ -218,7 +218,7 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCond__comparing_PKGSRC_COMPILER "ERROR: Makefile:6: Use ${PKGSRC_COMPILER:Ngcc} instead of the != operator.") } -func (s *Suite) Test_MkCondChecker_checkDirectiveCondEmpty(c *check.C) { +func (s *Suite) Test_MkCondChecker_checkEmpty(c *check.C) { t := s.Init(c) t.SetUpPackage("category/package") @@ -236,7 +236,7 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCondEmpty(c *check.C) { ".endif") t.ExpectDiagnosticsAutofix( - mklines.Check, + func(autofix bool) { mklines.Check() }, diagnostics...) afterMklines := LoadMk(t.File("filename.mk"), MustSucceed) @@ -249,8 +249,8 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCondEmpty(c *check.C) { "WARN: filename.mk:3: The pattern \"Unknown\" cannot match any of "+ "{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS } for OPSYS.", - "NOTE: filename.mk:3: OPSYS should be "+ - "compared using \"${OPSYS:U} == Unknown\" "+ + "NOTE: filename.mk:3: OPSYS can be "+ + "compared using the simpler \"${OPSYS:U} == Unknown\" "+ "instead of matching against \":MUnknown\".", // TODO: Turn the bsd.prefs.mk warning into an error, // once pkglint is confident enough to get this check right. @@ -271,7 +271,7 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCondEmpty(c *check.C) { ".include \"../../mk/bsd.prefs.mk\" first.") } -func (s *Suite) Test_MkCondChecker_checkDirectiveCondEmptyExpr(c *check.C) { +func (s *Suite) Test_MkCondChecker_checkEmptyExpr(c *check.C) { t := s.Init(c) test := func(use *MkVarUse, diagnostics ...string) { @@ -279,7 +279,7 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCondEmptyExpr(c *check.C) { "# dummy") ck := NewMkCondChecker(mklines.mklines[0], mklines) - ck.checkDirectiveCondEmptyExpr(use) + ck.checkEmptyExpr(use) t.CheckOutput(diagnostics) } @@ -305,7 +305,7 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCondEmptyExpr(c *check.C) { "name as parameter, not a variable expression.") } -func (s *Suite) Test_MkCondChecker_checkDirectiveCondEmptyType(c *check.C) { +func (s *Suite) Test_MkCondChecker_checkEmptyType(c *check.C) { t := s.Init(c) t.SetUpPackage("category/package") @@ -322,7 +322,7 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCondEmptyType(c *check.C) { mklines.ForEach(func(mkline *MkLine) { ck := NewMkCondChecker(mkline, mklines) mkline.ForEachUsed(func(varUse *MkVarUse, time VucTime) { - ck.checkDirectiveCondEmptyType(varUse) + ck.checkEmptyType(varUse) }) }) @@ -364,226 +364,366 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCondEmptyType(c *check.C) { nil...) } -func (s *Suite) Test_MkCondChecker_simplifyCondition(c *check.C) { +func (s *Suite) Test_MkCondChecker_simplify(c *check.C) { t := s.Init(c) - t.SetUpPackage("category/package") + t.CreateFileLines("mk/bsd.prefs.mk") t.Chdir("category/package") - t.FinishSetUp() + + // The Anything type suppresses the warnings from type checking. + // BtUnknown would not work, see Pkgsrc.VariableType. + btAnything := &BasicType{"Anything", func(cv *VartypeCheck) {}} + + // For simplifying the expressions, it is necessary to know whether + // a variable can be undefined. Undefined variables need the + // :U modifier, otherwise bmake will complain about "malformed + // conditions", which is not entirely precise since the expression + // is syntactically valid, it's just the evaluation that fails. + // + // Some variables such as MACHINE_ARCH are in scope from the very + // beginning. + // + // Some variables such as PKGPATH are in scope after bsd.prefs.mk + // has been included. + // + // Some variables such as PREFIX (as of December 2019) are only in + // scope after bsd.pkg.mk has been included. These cannot be used + // in .if conditions at all. + // + // Even when they are in scope, some variables such as PKGREVISION + // or MAKE_JOBS may be undefined. + + t.SetUpType("IN_SCOPE_DEFINED", btAnything, AlwaysInScope|DefinedIfInScope, + "*.mk: use, use-loadtime") + t.SetUpType("IN_SCOPE", btAnything, AlwaysInScope, + "*.mk: use, use-loadtime") + t.SetUpType("PREFS_DEFINED", btAnything, DefinedIfInScope, + "*.mk: use, use-loadtime") + t.SetUpType("PREFS", btAnything, NoVartypeOptions, + "*.mk: use, use-loadtime") + t.SetUpType("LATER_DEFINED", btAnything, DefinedIfInScope, + "*.mk: use") + t.SetUpType("LATER", btAnything, NoVartypeOptions, + "*.mk: use") + // UNDEFINED is also used in the following tests, but is obviously + // not defined here. // prefs: whether to include bsd.prefs.mk before // before: the directive before the condition is simplified // after: the directive after the condition is simplified - test := func(prefs bool, before, after string, diagnostics ...string) { + doTest := func(prefs bool, before, after string, diagnostics ...string) { + if !matches(before, `IN_SCOPE|PREFS|LATER|UNDEFINED`) { + c.Errorf("Condition %q must include one of the above variable names.", before) + } mklines := t.SetUpFileMkLines("filename.mk", MkCvsID, condStr(prefs, ".include \"../../mk/bsd.prefs.mk\"", ""), before, ".endif") - // The high-level call MkLines.Check is used here to - // get MkLines.Tools.SeenPrefs correct, which decides - // whether the :U modifier is necessary. - // - // TODO: Replace MkLines.Check this with a more specific method. + action := func(autofix bool) { + mklines.ForEach(func(mkline *MkLine) { + // Sets mklines.Tools.SeenPrefs, which decides whether the :U modifier + // is necessary; see MkLines.checkLine. + mklines.Tools.ParseToolLine(mklines, mkline, false, false) + + if mkline.IsDirective() && mkline.Directive() != "endif" { + // TODO: Replace Check with a more + // specific method that does not do the type checks. + NewMkCondChecker(mkline, mklines).Check() + } + }) - t.ExpectDiagnosticsAutofix( - mklines.Check, - diagnostics...) + if autofix { + afterMklines := LoadMk(t.File("filename.mk"), MustSucceed) + t.CheckEquals(afterMklines.mklines[2].Text, after) + } + } - // TODO: Move this assertion above the assertion about the diagnostics. - afterMklines := LoadMk(t.File("filename.mk"), MustSucceed) - t.CheckEquals(afterMklines.mklines[2].Text, after) + t.ExpectDiagnosticsAutofix(action, diagnostics...) + } + + testBeforePrefs := func(before, after string, diagnostics ...string) { + doTest(false, before, after, diagnostics...) } testAfterPrefs := func(before, after string, diagnostics ...string) { - test(true, before, after, diagnostics...) + doTest(true, before, after, diagnostics...) + } + testBeforeAndAfterPrefs := func(before, after string, diagnostics ...string) { + doTest(true, before, after, diagnostics...) } - test( - false, - ".if ${PKGPATH:Mpattern}", - ".if ${PKGPATH:U} == pattern", + testBeforeAndAfterPrefs( + ".if ${IN_SCOPE_DEFINED:Mpattern}", + ".if ${IN_SCOPE_DEFINED} == pattern", + + "NOTE: filename.mk:3: IN_SCOPE_DEFINED can be "+ + "compared using the simpler \"${IN_SCOPE_DEFINED} == pattern\" "+ + "instead of matching against \":Mpattern\".", + "AUTOFIX: filename.mk:3: Replacing \"${IN_SCOPE_DEFINED:Mpattern}\" "+ + "with \"${IN_SCOPE_DEFINED} == pattern\".") + + testBeforeAndAfterPrefs( + ".if ${IN_SCOPE:Mpattern}", + ".if ${IN_SCOPE:U} == pattern", - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH:U} == pattern\" "+ + "NOTE: filename.mk:3: IN_SCOPE can be "+ + "compared using the simpler \"${IN_SCOPE:U} == pattern\" "+ + "instead of matching against \":Mpattern\".", + "AUTOFIX: filename.mk:3: Replacing \"${IN_SCOPE:Mpattern}\" "+ + "with \"${IN_SCOPE:U} == pattern\".") + + // Even though PREFS_DEFINED is declared as DefinedIfInScope, + // it is not in scope yet. Therefore it needs the :U modifier. + // The warning that this variable is not yet in scope comes from + // a different part of pkglint. + testBeforePrefs( + ".if ${PREFS_DEFINED:Mpattern}", + ".if ${PREFS_DEFINED:U} == pattern", + + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED:U} == pattern\" "+ "instead of matching against \":Mpattern\".", - "WARN: filename.mk:3: To use PKGPATH at load time, "+ + "WARN: filename.mk:3: To use PREFS_DEFINED at load time, "+ ".include \"../../mk/bsd.prefs.mk\" first.", - "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Mpattern}\" "+ - "with \"${PKGPATH:U} == pattern\".") + "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Mpattern}\" "+ + "with \"${PREFS_DEFINED:U} == pattern\".") testAfterPrefs( - ".if ${PKGPATH:Mpattern}", - ".if ${PKGPATH} == pattern", + ".if ${PREFS_DEFINED:Mpattern}", + ".if ${PREFS_DEFINED} == pattern", - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH} == pattern\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} == pattern\" "+ "instead of matching against \":Mpattern\".", - "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Mpattern}\" "+ - "with \"${PKGPATH} == pattern\".") + "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Mpattern}\" "+ + "with \"${PREFS_DEFINED} == pattern\".") + + testBeforePrefs( + ".if ${PREFS:Mpattern}", + ".if ${PREFS:U} == pattern", + + "NOTE: filename.mk:3: PREFS can be "+ + "compared using the simpler \"${PREFS:U} == pattern\" "+ + "instead of matching against \":Mpattern\".", + "WARN: filename.mk:3: To use PREFS at load time, "+ + ".include \"../../mk/bsd.prefs.mk\" first.", + "AUTOFIX: filename.mk:3: Replacing \"${PREFS:Mpattern}\" "+ + "with \"${PREFS:U} == pattern\".") + + // Preferences that may be undefined always need the :U modifier, + // even when they are in scope. + testAfterPrefs( + ".if ${PREFS:Mpattern}", + ".if ${PREFS:U} == pattern", + + "NOTE: filename.mk:3: PREFS can be "+ + "compared using the simpler \"${PREFS:U} == pattern\" "+ + "instead of matching against \":Mpattern\".", + "AUTOFIX: filename.mk:3: Replacing \"${PREFS:Mpattern}\" "+ + "with \"${PREFS:U} == pattern\".") + + // Variables that are defined later always need the :U modifier. + // It is probably a mistake to use them in conditions at all. + testBeforeAndAfterPrefs( + ".if ${LATER_DEFINED:Mpattern}", + ".if ${LATER_DEFINED:U} == pattern", + + "NOTE: filename.mk:3: LATER_DEFINED can be "+ + "compared using the simpler \"${LATER_DEFINED:U} == pattern\" "+ + "instead of matching against \":Mpattern\".", + "WARN: filename.mk:3: "+ + "LATER_DEFINED should not be used at load time in any file.", + "AUTOFIX: filename.mk:3: Replacing \"${LATER_DEFINED:Mpattern}\" "+ + "with \"${LATER_DEFINED:U} == pattern\".") + + // Variables that are defined later always need the :U modifier. + // It is probably a mistake to use them in conditions at all. + testBeforeAndAfterPrefs( + ".if ${LATER:Mpattern}", + ".if ${LATER:U} == pattern", + + "NOTE: filename.mk:3: LATER can be "+ + "compared using the simpler \"${LATER:U} == pattern\" "+ + "instead of matching against \":Mpattern\".", + "WARN: filename.mk:3: "+ + "LATER should not be used at load time in any file.", + "AUTOFIX: filename.mk:3: Replacing \"${LATER:Mpattern}\" "+ + "with \"${LATER:U} == pattern\".") + + testBeforeAndAfterPrefs( + ".if ${UNDEFINED:Mpattern}", + ".if ${UNDEFINED:Mpattern}", + + "WARN: filename.mk:3: UNDEFINED is used but not defined.") // When the pattern contains placeholders, it cannot be converted to == or !=. testAfterPrefs( - ".if ${PKGPATH:Mpa*n}", - ".if ${PKGPATH:Mpa*n}", + ".if ${PREFS_DEFINED:Mpa*n}", + ".if ${PREFS_DEFINED:Mpa*n}", nil...) // When deciding whether to replace the expression, only the // last modifier is inspected. All the others are copied. testAfterPrefs( - ".if ${PKGPATH:tl:Mpattern}", - ".if ${PKGPATH:tl} == pattern", + ".if ${PREFS_DEFINED:tl:Mpattern}", + ".if ${PREFS_DEFINED:tl} == pattern", - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH:tl} == pattern\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED:tl} == pattern\" "+ "instead of matching against \":Mpattern\".", - "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:tl:Mpattern}\" "+ - "with \"${PKGPATH:tl} == pattern\".") + "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:tl:Mpattern}\" "+ + "with \"${PREFS_DEFINED:tl} == pattern\".") // Negated pattern matches are supported as well, // as long as the variable is guaranteed to be nonempty. + // TODO: Actually implement this. + // As of December 2019, IsNonemptyIfDefined is not used anywhere. testAfterPrefs( - ".if ${PKGPATH:Ncategory/package}", - ".if ${PKGPATH} != category/package", - - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH} != category/package\" "+ - "instead of matching against \":Ncategory/package\".", - "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Ncategory/package}\" "+ - "with \"${PKGPATH} != category/package\".") - - // ${PKGPATH:None:Ntwo} is a short variant of ${PKGPATH} != "one" && - // ${PKGPATH} != "two". Applying the transformation would make the - // condition longer than before, therefore nothing is done here. + ".if ${PREFS_DEFINED:Npattern}", + ".if ${PREFS_DEFINED} != pattern", + + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} != pattern\" "+ + "instead of matching against \":Npattern\".", + "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Npattern}\" "+ + "with \"${PREFS_DEFINED} != pattern\".") + + // ${PREFS_DEFINED:None:Ntwo} is a short variant of + // ${PREFS_DEFINED} != "one" && ${PREFS_DEFINED} != "two". + // Applying the transformation would make the condition longer + // than before, therefore nothing can be simplified here, + // even though all patterns are exact matches. testAfterPrefs( - ".if ${PKGPATH:None:Ntwo}", - ".if ${PKGPATH:None:Ntwo}", + ".if ${PREFS_DEFINED:None:Ntwo}", + ".if ${PREFS_DEFINED:None:Ntwo}", nil...) // Note: this combination doesn't make sense since the patterns // "one" and "two" don't overlap. + // Nevertheless it is possible and valid to simplify the condition. testAfterPrefs( - ".if ${PKGPATH:Mone:Mtwo}", - ".if ${PKGPATH:Mone} == two", + ".if ${PREFS_DEFINED:Mone:Mtwo}", + ".if ${PREFS_DEFINED:Mone} == two", - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH:Mone} == two\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED:Mone} == two\" "+ "instead of matching against \":Mtwo\".", - "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Mone:Mtwo}\" "+ - "with \"${PKGPATH:Mone} == two\".") - - testAfterPrefs( - ".if !empty(PKGPATH:Mpattern)", - ".if ${PKGPATH} == pattern", - - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH} == pattern\" "+ - "instead of matching against \":Mpattern\".", - "AUTOFIX: filename.mk:3: Replacing \"!empty(PKGPATH:Mpattern)\" "+ - "with \"${PKGPATH} == pattern\".") + "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Mone:Mtwo}\" "+ + "with \"${PREFS_DEFINED:Mone} == two\".") + // There is no ! before the empty, which is easy to miss. + // Because of this missing negation, the comparison operator is !=. testAfterPrefs( - ".if empty(PKGPATH:Mpattern)", - ".if ${PKGPATH} != pattern", + ".if empty(PREFS_DEFINED:Mpattern)", + ".if ${PREFS_DEFINED} != pattern", - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH} != pattern\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} != pattern\" "+ "instead of matching against \":Mpattern\".", - "AUTOFIX: filename.mk:3: Replacing \"empty(PKGPATH:Mpattern)\" "+ - "with \"${PKGPATH} != pattern\".") + "AUTOFIX: filename.mk:3: Replacing \"empty(PREFS_DEFINED:Mpattern)\" "+ + "with \"${PREFS_DEFINED} != pattern\".") testAfterPrefs( - ".if !!empty(PKGPATH:Mpattern)", + ".if !!empty(PREFS_DEFINED:Mpattern)", // TODO: The ! and == could be combined into a !=. // Luckily the !! pattern doesn't occur in practice. - ".if !${PKGPATH} == pattern", + ".if !${PREFS_DEFINED} == pattern", // TODO: When taking all the ! into account, this is actually a // test for emptiness, therefore the diagnostics should suggest // the != operator instead of ==. - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH} == pattern\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} == pattern\" "+ "instead of matching against \":Mpattern\".", - "AUTOFIX: filename.mk:3: Replacing \"!empty(PKGPATH:Mpattern)\" "+ - "with \"${PKGPATH} == pattern\".") + "AUTOFIX: filename.mk:3: Replacing \"!empty(PREFS_DEFINED:Mpattern)\" "+ + "with \"${PREFS_DEFINED} == pattern\".") - testAfterPrefs(".if empty(PKGPATH:Mpattern) || 0", - ".if ${PKGPATH} != pattern || 0", + // Simplifying the condition also works in complex expressions. + testAfterPrefs(".if empty(PREFS_DEFINED:Mpattern) || 0", + ".if ${PREFS_DEFINED} != pattern || 0", - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH} != pattern\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} != pattern\" "+ "instead of matching against \":Mpattern\".", - "AUTOFIX: filename.mk:3: Replacing \"empty(PKGPATH:Mpattern)\" "+ - "with \"${PKGPATH} != pattern\".") + "AUTOFIX: filename.mk:3: Replacing \"empty(PREFS_DEFINED:Mpattern)\" "+ + "with \"${PREFS_DEFINED} != pattern\".") // No note in this case since there is no implicit !empty around the varUse. + // There is no obvious way of writing this expression in a simpler form. testAfterPrefs( - ".if ${PKGPATH:Mpattern} != ${OTHER}", - ".if ${PKGPATH:Mpattern} != ${OTHER}", + ".if ${PREFS_DEFINED:Mpattern} != ${OTHER}", + ".if ${PREFS_DEFINED:Mpattern} != ${OTHER}", "WARN: filename.mk:3: OTHER is used but not defined.") + // The condition is also simplified if it doesn't use the !empty + // form but the implicit conversion to boolean. testAfterPrefs( - ".if ${PKGPATH:Mpattern}", - ".if ${PKGPATH} == pattern", + ".if ${PREFS_DEFINED:Mpattern}", + ".if ${PREFS_DEFINED} == pattern", - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH} == pattern\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} == pattern\" "+ "instead of matching against \":Mpattern\".", - "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Mpattern}\" "+ - "with \"${PKGPATH} == pattern\".") + "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Mpattern}\" "+ + "with \"${PREFS_DEFINED} == pattern\".") + // A single negation outside the implicit conversion is taken into + // account when simplifying the condition. testAfterPrefs( - ".if !${PKGPATH:Mpattern}", - ".if ${PKGPATH} != pattern", + ".if !${PREFS_DEFINED:Mpattern}", + ".if ${PREFS_DEFINED} != pattern", - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH} != pattern\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} != pattern\" "+ "instead of matching against \":Mpattern\".", - "AUTOFIX: filename.mk:3: Replacing \"!${PKGPATH:Mpattern}\" "+ - "with \"${PKGPATH} != pattern\".") + "AUTOFIX: filename.mk:3: Replacing \"!${PREFS_DEFINED:Mpattern}\" "+ + "with \"${PREFS_DEFINED} != pattern\".") // TODO: Merge the double negation into the comparison operator. testAfterPrefs( - ".if !!${PKGPATH:Mpattern}", - ".if !${PKGPATH} != pattern", + ".if !!${PREFS_DEFINED:Mpattern}", + ".if !${PREFS_DEFINED} != pattern", - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH} != pattern\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} != pattern\" "+ "instead of matching against \":Mpattern\".", - "AUTOFIX: filename.mk:3: Replacing \"!${PKGPATH:Mpattern}\" "+ - "with \"${PKGPATH} != pattern\".") + "AUTOFIX: filename.mk:3: Replacing \"!${PREFS_DEFINED:Mpattern}\" "+ + "with \"${PREFS_DEFINED} != pattern\".") // This pattern with spaces doesn't make sense at all in the :M // modifier since it can never match. // Or can it, if the PKGPATH contains quotes? - // How exactly does bmake apply the matching here, are both values unquoted? - testAfterPrefs( - ".if ${PKGPATH:Mpattern with spaces}", - ".if ${PKGPATH:Mpattern with spaces}", + // TODO: How exactly does bmake apply the matching here, + // are both values unquoted first? Probably not, but who knows. + testBeforeAndAfterPrefs( + ".if ${IN_SCOPE_DEFINED:Mpattern with spaces}", + ".if ${IN_SCOPE_DEFINED:Mpattern with spaces}", - "WARN: filename.mk:3: The pathname pattern \"pattern with spaces\" "+ - "contains the invalid characters \" \".") + nil...) // TODO: ".if ${PKGPATH} == \"pattern with spaces\"") - testAfterPrefs( - ".if ${PKGPATH:M'pattern with spaces'}", - ".if ${PKGPATH:M'pattern with spaces'}", + testBeforeAndAfterPrefs( + ".if ${IN_SCOPE_DEFINED:M'pattern with spaces'}", + ".if ${IN_SCOPE_DEFINED:M'pattern with spaces'}", - "WARN: filename.mk:3: The pathname pattern \"'pattern with spaces'\" "+ - "contains the invalid characters \"' '\".") + nil...) // TODO: ".if ${PKGPATH} == 'pattern with spaces'") - testAfterPrefs( - ".if ${PKGPATH:M&&}", - ".if ${PKGPATH:M&&}", + testBeforeAndAfterPrefs( + ".if ${IN_SCOPE_DEFINED:M&&}", + ".if ${IN_SCOPE_DEFINED:M&&}", - "WARN: filename.mk:3: The pathname pattern \"&&\" "+ - "contains the invalid characters \"&&\".") + nil...) // TODO: ".if ${PKGPATH} == '&&'") + // The :N modifier involves another negation and is therefore more + // difficult to understand. That's even more reason to use the + // well-known == and != comparison operators instead. + // // If PKGPATH is "", the condition is false. // If PKGPATH is "negative-pattern", the condition is false. // In all other cases, the condition is true. @@ -596,222 +736,156 @@ func (s *Suite) Test_MkCondChecker_simplifyCondition(c *check.C) { // such as OPSYS or PKGPATH, this replacement is valid. // These variables are only guaranteed to be defined after bsd.prefs.mk // has been included, like everywhere else. + // + // TODO: This is where NonemptyIfDefined comes into play. testAfterPrefs( - ".if ${PKGPATH:Nnegative-pattern}", - ".if ${PKGPATH} != negative-pattern", + ".if ${PREFS_DEFINED:Nnegative-pattern}", + ".if ${PREFS_DEFINED} != negative-pattern", - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH} != negative-pattern\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} != negative-pattern\" "+ "instead of matching against \":Nnegative-pattern\".", - "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Nnegative-pattern}\" "+ - "with \"${PKGPATH} != negative-pattern\".") + "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Nnegative-pattern}\" "+ + "with \"${PREFS_DEFINED} != negative-pattern\".") - // Since UNKNOWN is not a well-known system-provided variable that is + // Since UNDEFINED is not a well-known variable that is // guaranteed to be non-empty (see the previous example), it is not // transformed at all. - test( - false, - ".if ${UNKNOWN:Nnegative-pattern}", - ".if ${UNKNOWN:Nnegative-pattern}", + testBeforePrefs( + ".if ${UNDEFINED:Nnegative-pattern}", + ".if ${UNDEFINED:Nnegative-pattern}", - "WARN: filename.mk:3: UNKNOWN is used but not defined.") + "WARN: filename.mk:3: UNDEFINED is used but not defined.") - test( - true, - ".if ${UNKNOWN:Nnegative-pattern}", - ".if ${UNKNOWN:Nnegative-pattern}", + testAfterPrefs( + ".if ${UNDEFINED:Nnegative-pattern}", + ".if ${UNDEFINED:Nnegative-pattern}", - "WARN: filename.mk:3: UNKNOWN is used but not defined.") + "WARN: filename.mk:3: UNDEFINED is used but not defined.") + // A complex condition may contain several simple conditions + // that are each simplified independently, in the same go. testAfterPrefs( - ".if ${PKGPATH:Mpath1} || ${PKGPATH:Mpath2}", - ".if ${PKGPATH} == path1 || ${PKGPATH} == path2", + ".if ${PREFS_DEFINED:Mpath1} || ${PREFS_DEFINED:Mpath2}", + ".if ${PREFS_DEFINED} == path1 || ${PREFS_DEFINED} == path2", - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH} == path1\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} == path1\" "+ "instead of matching against \":Mpath1\".", - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH} == path2\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} == path2\" "+ "instead of matching against \":Mpath2\".", - "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Mpath1}\" "+ - "with \"${PKGPATH} == path1\".", - "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Mpath2}\" "+ - "with \"${PKGPATH} == path2\".") - + "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Mpath1}\" "+ + "with \"${PREFS_DEFINED} == path1\".", + "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Mpath2}\" "+ + "with \"${PREFS_DEFINED} == path2\".") + + // Removing redundant parentheses may be useful one day but is + // not urgent. + // Simplifying the inner expression keeps all parentheses as-is. testAfterPrefs( - ".if (((((${PKGPATH:Mpath})))))", - ".if (((((${PKGPATH} == path)))))", + ".if (((((${PREFS_DEFINED:Mpath})))))", + ".if (((((${PREFS_DEFINED} == path)))))", - "NOTE: filename.mk:3: PKGPATH "+ - "should be compared using \"${PKGPATH} == path\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} == path\" "+ "instead of matching against \":Mpath\".", - "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Mpath}\" "+ - "with \"${PKGPATH} == path\".") - - // MACHINE_ARCH is built-in into bmake and is always available. - // Therefore it doesn't matter whether bsd.prefs.mk is included or not. - test( - false, - ".if ${MACHINE_ARCH:Mx86_64}", - ".if ${MACHINE_ARCH} == x86_64", - - "NOTE: filename.mk:3: MACHINE_ARCH "+ - "should be compared using \"${MACHINE_ARCH} == x86_64\" "+ - "instead of matching against \":Mx86_64\".", - "AUTOFIX: filename.mk:3: Replacing \"${MACHINE_ARCH:Mx86_64}\" "+ - "with \"${MACHINE_ARCH} == x86_64\".") - - // MACHINE_ARCH is built-in into bmake and is always available. - // Therefore it doesn't matter whether bsd.prefs.mk is included or not. - test( - true, - ".if ${MACHINE_ARCH:Mx86_64}", - ".if ${MACHINE_ARCH} == x86_64", - - "NOTE: filename.mk:3: MACHINE_ARCH "+ - "should be compared using \"${MACHINE_ARCH} == x86_64\" "+ - "instead of matching against \":Mx86_64\".", - "AUTOFIX: filename.mk:3: Replacing \"${MACHINE_ARCH:Mx86_64}\" "+ - "with \"${MACHINE_ARCH} == x86_64\".") - - test( - false, - ".if !empty(OPSYS:MUnknown)", - ".if ${OPSYS:U} == Unknown", - - // FIXME: This warning is not the job of simplifyCondition. - // Therefore don't test it here. - "WARN: filename.mk:3: The pattern \"Unknown\" cannot match any of "+ - "{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS } for OPSYS.", - "NOTE: filename.mk:3: OPSYS should be "+ - "compared using \"${OPSYS:U} == Unknown\" "+ - "instead of matching against \":MUnknown\".", - "WARN: filename.mk:3: To use OPSYS at load time, "+ - ".include \"../../mk/bsd.prefs.mk\" first.", - "AUTOFIX: filename.mk:3: Replacing \"!empty(OPSYS:MUnknown)\" "+ - "with \"${OPSYS:U} == Unknown\".") + "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Mpath}\" "+ + "with \"${PREFS_DEFINED} == path\".") + // Several modifiers like :S and :C may change the variable value. + // Whether the condition can be simplified or not only depends on the + // last modifier in the chain. testAfterPrefs( - ".if !empty(OPSYS:S,NetBSD,ok,:Mok)", - ".if ${OPSYS:S,NetBSD,ok,} == ok", + ".if !empty(PREFS_DEFINED:S,NetBSD,ok,:Mok)", + ".if ${PREFS_DEFINED:S,NetBSD,ok,} == ok", - "NOTE: filename.mk:3: OPSYS should be "+ - "compared using \"${OPSYS:S,NetBSD,ok,} == ok\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED:S,NetBSD,ok,} == ok\" "+ "instead of matching against \":Mok\".", - "AUTOFIX: filename.mk:3: Replacing \"!empty(OPSYS:S,NetBSD,ok,:Mok)\" "+ - "with \"${OPSYS:S,NetBSD,ok,} == ok\".") + "AUTOFIX: filename.mk:3: Replacing \"!empty(PREFS_DEFINED:S,NetBSD,ok,:Mok)\" "+ + "with \"${PREFS_DEFINED:S,NetBSD,ok,} == ok\".") testAfterPrefs( - ".if empty(OPSYS:tl:Msunos)", - ".if ${OPSYS:tl} != sunos", + ".if empty(PREFS_DEFINED:tl:Msunos)", + ".if ${PREFS_DEFINED:tl} != sunos", - "NOTE: filename.mk:3: OPSYS should be "+ - "compared using \"${OPSYS:tl} != sunos\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED:tl} != sunos\" "+ "instead of matching against \":Msunos\".", - "AUTOFIX: filename.mk:3: Replacing \"empty(OPSYS:tl:Msunos)\" "+ - "with \"${OPSYS:tl} != sunos\".") + "AUTOFIX: filename.mk:3: Replacing \"empty(PREFS_DEFINED:tl:Msunos)\" "+ + "with \"${PREFS_DEFINED:tl} != sunos\".") + // The condition can only be simplified if the :M or :N modifier + // appears at the end of the chain. testAfterPrefs( - ".if !empty(OPSYS:O:MUnknown:S,a,b,)", - ".if !empty(OPSYS:O:MUnknown:S,a,b,)", + ".if !empty(PREFS_DEFINED:O:MUnknown:S,a,b,)", + ".if !empty(PREFS_DEFINED:O:MUnknown:S,a,b,)", - "WARN: filename.mk:3: The pattern \"Unknown\" cannot match any of "+ - "{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS } for OPSYS.") + nil...) - // The dot is just an ordinary character. - // It's only special when used in number literals. + // The dot is just an ordinary character in a pattern. + // In comparisons, an unquoted 1.2 is interpreted as a number though. testAfterPrefs( - ".if !empty(PKGPATH:Mcategory/package1.2)", - ".if ${PKGPATH} == category/package1.2", + ".if !empty(PREFS_DEFINED:Mpackage1.2)", + ".if ${PREFS_DEFINED} == package1.2", - "NOTE: filename.mk:3: PKGPATH should be "+ - "compared using \"${PKGPATH} == category/package1.2\" "+ - "instead of matching against \":Mcategory/package1.2\".", - "AUTOFIX: filename.mk:3: Replacing \"!empty(PKGPATH:Mcategory/package1.2)\" "+ - "with \"${PKGPATH} == category/package1.2\".") + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} == package1.2\" "+ + "instead of matching against \":Mpackage1.2\".", + "AUTOFIX: filename.mk:3: Replacing \"!empty(PREFS_DEFINED:Mpackage1.2)\" "+ + "with \"${PREFS_DEFINED} == package1.2\".") // Numbers must be enclosed in quotes, otherwise they are compared - // as numbers, not as strings. The :M and :N modifiers always compare - // strings. + // as numbers, not as strings. + // The :M and :N modifiers always compare strings. testAfterPrefs( - ".if empty(ABI:U:M64)", - ".if ${ABI:U} != \"64\"", + ".if empty(PREFS:U:M64)", + ".if ${PREFS:U} != \"64\"", - "NOTE: filename.mk:3: ABI should be compared using \"${ABI:U} != \"64\"\" "+ + "NOTE: filename.mk:3: PREFS can be "+ + "compared using the simpler \"${PREFS:U} != \"64\"\" "+ "instead of matching against \":M64\".", - "AUTOFIX: filename.mk:3: Replacing \"empty(ABI:U:M64)\" "+ - "with \"${ABI:U} != \\\"64\\\"\".") + "AUTOFIX: filename.mk:3: Replacing \"empty(PREFS:U:M64)\" "+ + "with \"${PREFS:U} != \\\"64\\\"\".") // Fractional numbers must also be enclosed in quotes. testAfterPrefs( - ".if empty(PKGVERSION_NOREV:U:M19.12)", - ".if ${PKGVERSION_NOREV:U} != \"19.12\"", + ".if empty(PREFS:U:M19.12)", + ".if ${PREFS:U} != \"19.12\"", - "NOTE: filename.mk:3: PKGVERSION_NOREV should be "+ - "compared using \"${PKGVERSION_NOREV:U} != \"19.12\"\" "+ + "NOTE: filename.mk:3: PREFS can be "+ + "compared using the simpler \"${PREFS:U} != \"19.12\"\" "+ "instead of matching against \":M19.12\".", - "WARN: filename.mk:3: PKGVERSION_NOREV should not be used at load time in any file.", - "AUTOFIX: filename.mk:3: Replacing \"empty(PKGVERSION_NOREV:U:M19.12)\" "+ - "with \"${PKGVERSION_NOREV:U} != \\\"19.12\\\"\".") + "AUTOFIX: filename.mk:3: Replacing \"empty(PREFS:U:M19.12)\" "+ + "with \"${PREFS:U} != \\\"19.12\\\"\".") testAfterPrefs( - ".if !empty(PKG_INFO:Mpkg_info)", - ".if ${PKG_INFO} == pkg_info", - - "NOTE: filename.mk:3: PKG_INFO should be "+ - "compared using \"${PKG_INFO} == pkg_info\" "+ - "instead of matching against \":Mpkg_info\".", - "AUTOFIX: filename.mk:3: "+ - "Replacing \"!empty(PKG_INFO:Mpkg_info)\" "+ - "with \"${PKG_INFO} == pkg_info\".") - - t.CheckEquals( - G.Pkgsrc.VariableType(nil, "PKG_LIBTOOL"). - Union().Contains(aclpUseLoadtime), - false) - testAfterPrefs( - ".if !empty(PKG_LIBTOOL:Npattern)", - ".if !empty(PKG_LIBTOOL:Npattern)", + ".if !empty(LATER:Npattern)", + ".if !empty(LATER:Npattern)", // No diagnostics about the :N modifier yet, - // see MkLineChecker.simplifyCondition.replace. - "WARN: filename.mk:3: PKG_LIBTOOL should not be used "+ + // see MkCondChecker.simplify.replace. + "WARN: filename.mk:3: LATER should not be used "+ "at load time in any file.") // TODO: Add a note that the :U is unnecessary, and explain why. testAfterPrefs( - ".if ${PKGPATH:U:Mcategory/package}", - ".if ${PKGPATH:U} == category/package", - - "NOTE: filename.mk:3: PKGPATH should be "+ - "compared using \"${PKGPATH:U} == category/package\" "+ - "instead of matching against \":Mcategory/package\".", - "AUTOFIX: filename.mk:3: "+ - "Replacing \"${PKGPATH:U:Mcategory/package}\" "+ - "with \"${PKGPATH:U} == category/package\".") + ".if ${PREFS_DEFINED:U:Mpattern}", + ".if ${PREFS_DEFINED:U} == pattern", - testAfterPrefs( - ".if ${UNKNOWN:Mpattern}", - ".if ${UNKNOWN:Mpattern}", - - "WARN: filename.mk:3: UNKNOWN is used but not defined.") - - // MAKE is AlwaysInScope and DefinedIfInScope and NonemptyIfDefined. - testAfterPrefs( - ".if ${MAKE:Mpattern}", - ".if ${MAKE} == pattern", - - "NOTE: filename.mk:3: MAKE should be "+ - "compared using \"${MAKE} == pattern\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED:U} == pattern\" "+ "instead of matching against \":Mpattern\".", "AUTOFIX: filename.mk:3: "+ - "Replacing \"${MAKE:Mpattern}\" "+ - "with \"${MAKE} == pattern\".") + "Replacing \"${PREFS_DEFINED:U:Mpattern}\" "+ + "with \"${PREFS_DEFINED:U} == pattern\".") - // VarUse without any modifiers is skipped. - testAfterPrefs( - ".if ${MAKE}", - ".if ${MAKE}", + // Conditions without any modifiers cannot be simplified + // and are therefore skipped. + testBeforeAndAfterPrefs( + ".if ${IN_SCOPE_DEFINED}", + ".if ${IN_SCOPE_DEFINED}", nil...) @@ -821,53 +895,55 @@ func (s *Suite) Test_MkCondChecker_simplifyCondition(c *check.C) { // replaced automatically, see mkCondLiteralChars. // TODO: Add tests for all characters that are special in string literals or patterns. // TODO: Then, extend the set of characters for which the expressions are simplified. - testAfterPrefs( - ".if ${FETCH_CMD:M&&}", - ".if ${FETCH_CMD:M&&}", + testBeforeAndAfterPrefs( + ".if ${PREFS_DEFINED:M&&}", + ".if ${PREFS_DEFINED:M&&}", + + nil...) + + testBeforeAndAfterPrefs( + ".if ${PREFS:M&&}", + ".if ${PREFS:M&&}", + // TODO: Warn that the :U is missing. nil...) - // The + is contained in mkCondStringLiteralUnquoted. - // The + is contained in mkCondModifierPatternLiteral. + // The + is contained in both mkCondStringLiteralUnquoted and + // mkCondModifierPatternLiteral, therefore it is copied verbatim. testAfterPrefs( - ".if ${PKGPATH:Mcategory/gtk+}", - ".if ${PKGPATH} == category/gtk+", + ".if ${PREFS_DEFINED:Mcategory/gtk+}", + ".if ${PREFS_DEFINED} == category/gtk+", - "NOTE: filename.mk:3: PKGPATH should be "+ - "compared using \"${PKGPATH} == category/gtk+\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} == category/gtk+\" "+ "instead of matching against \":Mcategory/gtk+\".", "AUTOFIX: filename.mk:3: "+ - "Replacing \"${PKGPATH:Mcategory/gtk+}\" "+ - "with \"${PKGPATH} == category/gtk+\".") + "Replacing \"${PREFS_DEFINED:Mcategory/gtk+}\" "+ + "with \"${PREFS_DEFINED} == category/gtk+\".") // The characters <=> may be used unescaped in :M and :N patterns // but not in .if conditions. There they must be enclosed in quotes. - testAfterPrefs( - ".if ${PKGPATH:M<=>}", - ".if ${PKGPATH} == \"<=>\"", + testBeforeAndAfterPrefs( + ".if ${PREFS_DEFINED:M<=>}", + ".if ${PREFS_DEFINED} == \"<=>\"", - "WARN: filename.mk:3: The pathname pattern \"<=>\" "+ - "contains the invalid characters \"<=>\".", - "NOTE: filename.mk:3: PKGPATH should be "+ - "compared using \"${PKGPATH} == \"<=>\"\" "+ + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED} == \"<=>\"\" "+ "instead of matching against \":M<=>\".", "AUTOFIX: filename.mk:3: "+ - "Replacing \"${PKGPATH:M<=>}\" "+ - "with \"${PKGPATH} == \\\"<=>\\\"\".") + "Replacing \"${PREFS_DEFINED:M<=>}\" "+ + "with \"${PREFS_DEFINED} == \\\"<=>\\\"\".") // If pkglint replaces this particular pattern, the resulting string // literal must be escaped properly. - testAfterPrefs( - ".if ${PKGPATH:M\"}", - ".if ${PKGPATH:M\"}", + testBeforeAndAfterPrefs( + ".if ${IN_SCOPE_DEFINED:M\"}", + ".if ${IN_SCOPE_DEFINED:M\"}", - // TODO: Find a better variable than PKGPATH, - // to get rid of this unrelated warning. - "WARN: filename.mk:3: The pathname pattern \"\\\"\" "+ - "contains the invalid character \"\\\"\".") + nil...) } -func (s *Suite) Test_MkCondChecker_simplifyCondition__defined_in_same_file(c *check.C) { +func (s *Suite) Test_MkCondChecker_simplify__defined_in_same_file(c *check.C) { t := s.Init(c) t.SetUpPackage("category/package") @@ -894,7 +970,7 @@ func (s *Suite) Test_MkCondChecker_simplifyCondition__defined_in_same_file(c *ch // TODO: Replace MkLines.Check this with a more specific method. t.ExpectDiagnosticsAutofix( - mklines.Check, + func(autofix bool) { mklines.Check() }, diagnostics...) // TODO: Move this assertion above the assertion about the diagnostics. @@ -924,8 +1000,8 @@ func (s *Suite) Test_MkCondChecker_simplifyCondition__defined_in_same_file(c *ch ".if ${OK_DIR:Mpattern}", ".if ${OK_DIR} == pattern", - "NOTE: filename.mk:4: OK_DIR should be "+ - "compared using \"${OK_DIR} == pattern\" "+ + "NOTE: filename.mk:4: OK_DIR can be "+ + "compared using the simpler \"${OK_DIR} == pattern\" "+ "instead of matching against \":Mpattern\".", "AUTOFIX: filename.mk:4: "+ "Replacing \"${OK_DIR:Mpattern}\" "+ @@ -939,15 +1015,15 @@ func (s *Suite) Test_MkCondChecker_simplifyCondition__defined_in_same_file(c *ch // FIXME: Warn that LATER_DIR is used before it is defined. // FIXME: Add :U modifier since LATER_DIR is not yet defined. - "NOTE: filename.mk:4: LATER_DIR should be "+ - "compared using \"${LATER_DIR} == pattern\" "+ + "NOTE: filename.mk:4: LATER_DIR can be "+ + "compared using the simpler \"${LATER_DIR} == pattern\" "+ "instead of matching against \":Mpattern\".", "AUTOFIX: filename.mk:4: "+ "Replacing \"${LATER_DIR:Mpattern}\" "+ "with \"${LATER_DIR} == pattern\".") } -func (s *Suite) Test_MkCondChecker_checkDirectiveCondCompare(c *check.C) { +func (s *Suite) Test_MkCondChecker_checkCompare(c *check.C) { t := s.Init(c) t.SetUpVartypes() @@ -956,7 +1032,7 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCondCompare(c *check.C) { mklines := t.NewMkLines("filename.mk", cond) mklines.ForEach(func(mkline *MkLine) { - NewMkCondChecker(mkline, mklines).checkDirectiveCond() + NewMkCondChecker(mkline, mklines).Check() }) t.CheckOutput(output) } @@ -995,7 +1071,7 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCondCompare(c *check.C) { "WARN: filename.mk:1: Invalid condition, unrecognized part: \"empty{VAR}\".") } -func (s *Suite) Test_MkCondChecker_checkDirectiveCondCompareVarStr__no_tracing(c *check.C) { +func (s *Suite) Test_MkCondChecker_checkCompareVarStr__no_tracing(c *check.C) { t := s.Init(c) b := NewMkTokenBuilder() @@ -1007,22 +1083,11 @@ func (s *Suite) Test_MkCondChecker_checkDirectiveCondCompareVarStr__no_tracing(c ck := NewMkCondChecker(mklines.mklines[0], mklines) varUse := b.VarUse("DISTFILES", "Mpattern", "O", "u") // TODO: mklines.ForEach - ck.checkDirectiveCondCompareVarStr(varUse, "==", "distfile-1.0.tar.gz") + ck.checkCompareVarStr(varUse, "==", "distfile-1.0.tar.gz") t.CheckOutputEmpty() } -func (s *Suite) Test_MkCondChecker_checkCompareVarStr(c *check.C) { - t := s.Init(c) - - test := func() { - // FIXME - t.CheckEquals(true, true) - } - - test() -} - func (s *Suite) Test_MkCondChecker_checkCompareVarStrCompiler(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/mklexer.go b/pkgtools/pkglint/files/mklexer.go index 527a139f43b..c1008260698 100644 --- a/pkgtools/pkglint/files/mklexer.go +++ b/pkgtools/pkglint/files/mklexer.go @@ -73,7 +73,7 @@ func (p *MkLexer) VarUse() *MkVarUse { // This is an escaped dollar character and not a variable use. return nil - case '@', '<', ' ': + case '>', '!', '<', '%', '?', '*', '@', ' ': // These variable names are known to exist. // // Many others are also possible but not used in practice. @@ -81,6 +81,13 @@ func (p *MkLexer) VarUse() *MkVarUse { // the $ must not be interpreted as a variable name, // even when it looks like $/ could refer to the "/" variable. // + // Example: + // ${:U }= space + // ${:U"}= quot + // + // all: + // @echo ${ } $ d, ${"} $"ed # space spaced, quote quoted + // // TODO: Find out whether $" is a variable use when it appears in the :M modifier. p.lexer.Skip(2) return NewMkVarUse(rest[1:2]) diff --git a/pkgtools/pkglint/files/mklexer_test.go b/pkgtools/pkglint/files/mklexer_test.go index ebc50d4e7df..4ddce96248d 100644 --- a/pkgtools/pkglint/files/mklexer_test.go +++ b/pkgtools/pkglint/files/mklexer_test.go @@ -404,7 +404,7 @@ func (s *Suite) Test_MkLexer_VarUse(c *check.C) { func (s *Suite) Test_MkLexer_varUseBrace__autofix_parentheses(c *check.C) { t := s.Init(c) - test := func() { + test := func(autofix bool) { mklines := t.SetUpFileMkLines("Makefile", MkCvsID, "COMMENT=\t$(P1) $(P2)) $(P3:Q) ${BRACES} $(A.$(B.$(C))) $(A:M\\#)", @@ -1248,7 +1248,7 @@ func (s *Suite) Test_MkLexer_Explain(c *check.C) { func (s *Suite) Test_MkLexer_Autofix(c *check.C) { t := s.Init(c) - test := func() { + test := func(autofix bool) { mklines := t.SetUpFileMkLines("filename.mk", "# before") lex := NewMkLexer("", mklines.lines.Lines[0]) diff --git a/pkgtools/pkglint/files/mkline.go b/pkgtools/pkglint/files/mkline.go index 7f9f1699725..8d476611d69 100644 --- a/pkgtools/pkglint/files/mkline.go +++ b/pkgtools/pkglint/files/mkline.go @@ -1239,7 +1239,7 @@ func (ind *Indentation) TrackAfter(mkline *MkLine) { cond.Walk(&MkCondCallback{ Call: func(name string, arg string) { if name == "exists" && !NewPath(arg).IsAbs() { - rel := G.Pkgsrc.ToRel(mkline.File(NewRelPathString(arg))) + rel := G.Pkgsrc.Rel(mkline.File(NewRelPathString(arg))) ind.AddCheckedFile(rel) } }}) diff --git a/pkgtools/pkglint/files/mklinechecker.go b/pkgtools/pkglint/files/mklinechecker.go index ca119a68d76..82537080270 100644 --- a/pkgtools/pkglint/files/mklinechecker.go +++ b/pkgtools/pkglint/files/mklinechecker.go @@ -310,7 +310,7 @@ func (ck MkLineChecker) CheckRelativePath(relativePath RelPath, mustExist bool) abs := mkline.Filename.DirNoClean().JoinNoClean(resolvedPath) if !abs.Exists() { - pkgsrcPath := G.Pkgsrc.ToRel(ck.MkLine.File(resolvedPath)) + pkgsrcPath := G.Pkgsrc.Rel(ck.MkLine.File(resolvedPath)) if mustExist && !ck.MkLines.indentation.HasExists(pkgsrcPath) { mkline.Errorf("Relative path %q does not exist.", resolvedPath) } @@ -327,7 +327,7 @@ func (ck MkLineChecker) CheckRelativePath(relativePath RelPath, mustExist bool) case matches(resolvedPath.String(), `^\.\./\.\./[^./][^/]*/[^/]`): // From a package to another package. - case resolvedPath.HasPrefixPath("../mk") && G.Pkgsrc.ToRel(mkline.Filename).Count() == 2: + case resolvedPath.HasPrefixPath("../mk") && G.Pkgsrc.Rel(mkline.Filename).Count() == 2: // For category Makefiles. // TODO: Or from a pkgsrc wip package to wip/mk. @@ -406,7 +406,7 @@ func (ck MkLineChecker) checkDirective(forVars map[string]bool, ind *Indentation case directive == "if" || directive == "elif": mkCondChecker := NewMkCondChecker(mkline, ck.MkLines) - mkCondChecker.checkDirectiveCond() + mkCondChecker.Check() case directive == "ifdef" || directive == "ifndef": mkline.Warnf("The \".%s\" directive is deprecated. Please use \".if %sdefined(%s)\" instead.", diff --git a/pkgtools/pkglint/files/mklinechecker_test.go b/pkgtools/pkglint/files/mklinechecker_test.go index e647c884ba3..def38881aef 100644 --- a/pkgtools/pkglint/files/mklinechecker_test.go +++ b/pkgtools/pkglint/files/mklinechecker_test.go @@ -209,7 +209,7 @@ func (s *Suite) Test_MkLineChecker_checkVartype(c *check.C) { MkCvsID, "DISTNAME=\tgcc-${GCC_VERSION}") - mklines.vars.Define("GCC_VERSION", mklines.mklines[1]) + mklines.allVars.Define("GCC_VERSION", mklines.mklines[1]) mklines.Check() t.CheckOutputEmpty() diff --git a/pkgtools/pkglint/files/mklineparser_test.go b/pkgtools/pkglint/files/mklineparser_test.go index 0d3f66bdd22..cac89e8b511 100644 --- a/pkgtools/pkglint/files/mklineparser_test.go +++ b/pkgtools/pkglint/files/mklineparser_test.go @@ -913,7 +913,7 @@ func (s *Suite) Test_MkLineParser_split(c *check.C) { comment: " comment after spaces", }) - // FIXME: This theoretical edge case is interpreted differently + // XXX: This theoretical edge case is interpreted differently // between bmake and pkglint. Pkglint treats the # as a comment, // while bmake interprets it as a regular character. test("\\[#", diff --git a/pkgtools/pkglint/files/mklines.go b/pkgtools/pkglint/files/mklines.go index 95fc1b88135..434ced7d880 100644 --- a/pkgtools/pkglint/files/mklines.go +++ b/pkgtools/pkglint/files/mklines.go @@ -7,7 +7,7 @@ type MkLines struct { mklines []*MkLine lines *Lines target string // Current make(1) target; only available during checkAll - vars Scope // + allVars Scope // The variables after loading the complete file 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. @@ -124,7 +124,7 @@ func (mklines *MkLines) collectUsedVariables() { // UseVar remembers that the given variable is used in the given line. // This controls the "defined but not used" warning. func (mklines *MkLines) UseVar(mkline *MkLine, varname string, time VucTime) { - mklines.vars.Use(varname, mkline, time) + mklines.allVars.Use(varname, mkline, time) if G.Pkg != nil { G.Pkg.vars.Use(varname, mkline, time) } @@ -148,8 +148,8 @@ func (mklines *MkLines) collectDocumentedVariables() { // leaving 2 of these 3 lines for the actual documentation. if commentLines >= 3 && relevant { for varname, mkline := range scope.used { - mklines.vars.Define(varname, mkline) - mklines.vars.Use(varname, mkline, VucRunTime) + mklines.allVars.Define(varname, mkline) + mklines.allVars.Use(varname, mkline, VucRunTime) } } @@ -231,7 +231,7 @@ func (mklines *MkLines) collectVariables() { "BUILTIN_FIND_FILES_VAR", "BUILTIN_FIND_HEADERS_VAR": for _, varname := range mkline.Fields() { - mklines.vars.Define(varname, mkline) + mklines.allVars.Define(varname, mkline) } case "PLIST_VARS": @@ -284,6 +284,9 @@ func (mklines *MkLines) ForEachEnd(action func(mkline *MkLine) bool, atEnd func( // Multiple of these line checkers could be run in parallel, so that // the diagnostics appear in the correct order, from top to bottom. + // ForEachEnd must not be called within itself. + assert(mklines.indentation == nil) + mklines.indentation = NewIndentation() mklines.Tools.SeenPrefs = false @@ -307,7 +310,7 @@ func (mklines *MkLines) ForEachEnd(action func(mkline *MkLine) bool, atEnd func( // defineVar marks a variable as defined in both the current package and the current file. func (mklines *MkLines) defineVar(pkg *Package, mkline *MkLine, varname string) { - mklines.vars.Define(varname, mkline) + mklines.allVars.Define(varname, mkline) if pkg != nil { pkg.vars.Define(varname, mkline) } diff --git a/pkgtools/pkglint/files/mklines_test.go b/pkgtools/pkglint/files/mklines_test.go index 69b4e5a2632..cf5a1eb4aa4 100644 --- a/pkgtools/pkglint/files/mklines_test.go +++ b/pkgtools/pkglint/files/mklines_test.go @@ -496,8 +496,8 @@ func (s *Suite) Test_MkLines_collectUsedVariables__simple(c *check.C) { mklines.collectUsedVariables() - t.CheckDeepEquals(mklines.vars.used, map[string]*MkLine{"VAR": mkline}) - t.CheckEquals(mklines.vars.FirstUse("VAR"), mkline) + t.CheckDeepEquals(mklines.allVars.used, map[string]*MkLine{"VAR": mkline}) + t.CheckEquals(mklines.allVars.FirstUse("VAR"), mkline) } func (s *Suite) Test_MkLines_collectUsedVariables__nested(c *check.C) { @@ -515,12 +515,12 @@ func (s *Suite) Test_MkLines_collectUsedVariables__nested(c *check.C) { mklines.collectUsedVariables() - t.CheckEquals(len(mklines.vars.used), 5) - t.CheckEquals(mklines.vars.FirstUse("lparam"), assignMkline) - t.CheckEquals(mklines.vars.FirstUse("rparam"), assignMkline) - t.CheckEquals(mklines.vars.FirstUse("inner"), shellMkline) - t.CheckEquals(mklines.vars.FirstUse("outer.*"), shellMkline) - t.CheckEquals(mklines.vars.FirstUse("outer.${inner}"), shellMkline) + t.CheckEquals(len(mklines.allVars.used), 5) + t.CheckEquals(mklines.allVars.FirstUse("lparam"), assignMkline) + t.CheckEquals(mklines.allVars.FirstUse("rparam"), assignMkline) + t.CheckEquals(mklines.allVars.FirstUse("inner"), shellMkline) + t.CheckEquals(mklines.allVars.FirstUse("outer.*"), shellMkline) + t.CheckEquals(mklines.allVars.FirstUse("outer.${inner}"), shellMkline) } func (s *Suite) Test_MkLines_collectDocumentedVariables(c *check.C) { @@ -572,7 +572,7 @@ func (s *Suite) Test_MkLines_collectDocumentedVariables(c *check.C) { mklines.collectDocumentedVariables() var varnames []string - for varname, mkline := range mklines.vars.used { + for varname, mkline := range mklines.allVars.used { varnames = append(varnames, sprintf("%s (line %s)", varname, mkline.Linenos())) } sort.Strings(varnames) @@ -696,7 +696,7 @@ func (s *Suite) Test_MkLines_collectVariables__find_files_and_headers(c *check.C mklines.Check() t.CheckDeepEquals( - keys(mklines.vars.firstDef), + keys(mklines.allVars.firstDef), []string{ "BUILTIN_FIND_FILES_VAR", "BUILTIN_FIND_HEADERS_VAR", diff --git a/pkgtools/pkglint/files/mkparser_test.go b/pkgtools/pkglint/files/mkparser_test.go index 48148f6cdb2..c6c5154b3b0 100644 --- a/pkgtools/pkglint/files/mkparser_test.go +++ b/pkgtools/pkglint/files/mkparser_test.go @@ -12,7 +12,7 @@ func (s *Suite) Test_MkParser_MkCond(c *check.C) { testRest := func(input string, expectedTree *MkCond, expectedRest string) { // As of July 2019 p.MkCond does not emit warnings; - // this is left to MkLineChecker.checkDirectiveCond. + // this is left to MkCondChecker.Check. line := t.NewLine("filename.mk", 1, ".if "+input) p := NewMkParser(line, input) actualTree := p.MkCond() diff --git a/pkgtools/pkglint/files/mktypes_test.go b/pkgtools/pkglint/files/mktypes_test.go index 1d059bfc356..d51eac8eaf6 100644 --- a/pkgtools/pkglint/files/mktypes_test.go +++ b/pkgtools/pkglint/files/mktypes_test.go @@ -192,7 +192,9 @@ func (s *Suite) Test_MkVarUseModifier_ChangesList(c *check.C) { test("E", false) test("H", false) - // FIXME: The :M and :N modifiers obviously change the number of words. + // The :M and :N modifiers may reduce the number of words in a + // variable, but they don't change the interpretation from a list + // to a non-list. test("Mpattern", false) test("Npattern", false) diff --git a/pkgtools/pkglint/files/mkvarusechecker.go b/pkgtools/pkglint/files/mkvarusechecker.go index 18a4c17ac16..734a670b185 100644 --- a/pkgtools/pkglint/files/mkvarusechecker.go +++ b/pkgtools/pkglint/files/mkvarusechecker.go @@ -45,9 +45,10 @@ func (ck *MkVarUseChecker) checkUndefined() { case !G.Opts.WarnExtra, // Well-known variables are probably defined by the infrastructure. vartype != nil && !vartype.IsGuessed(), - ck.MkLines.vars.IsDefinedSimilar(varname), + // TODO: At load time, check ck.MkLines.loadVars instead of allVars. + ck.MkLines.allVars.IsDefinedSimilar(varname), ck.MkLines.forVars[varname], - ck.MkLines.vars.Mentioned(varname) != nil, + ck.MkLines.allVars.Mentioned(varname) != nil, G.Pkg != nil && G.Pkg.vars.IsDefinedSimilar(varname), containsVarRef(varname), G.Pkgsrc.vartypes.IsDefinedCanon(varname), @@ -61,9 +62,7 @@ func (ck *MkVarUseChecker) checkUndefined() { } func (ck *MkVarUseChecker) checkModifiers() { - varuse := ck.use - mods := varuse.modifiers - if len(mods) == 0 { + if len(ck.use.modifiers) == 0 { return } diff --git a/pkgtools/pkglint/files/mkvarusechecker_test.go b/pkgtools/pkglint/files/mkvarusechecker_test.go index 4a0dfffd540..be9d5dfe83a 100644 --- a/pkgtools/pkglint/files/mkvarusechecker_test.go +++ b/pkgtools/pkglint/files/mkvarusechecker_test.go @@ -275,7 +275,25 @@ func (s *Suite) Test_MkVarUseChecker_checkUndefined__documented(c *check.C) { } func (s *Suite) Test_MkVarUseChecker_checkModifiers(c *check.C) { - // FIXME + t := s.Init(c) + + test := func(text string, diagnostics ...string) { + mklines := t.NewMkLines("filename.mk", + text) + mklines.ForEach(func(mkline *MkLine) { + mkline.ForEachUsed(func(varUse *MkVarUse, time VucTime) { + ck := NewMkVarUseChecker(varUse, mklines, mkline) + ck.checkModifiers() + }) + }) + t.CheckOutput(diagnostics) + } + + test("\t${VAR}", + nil...) + + test("\t${VAR:from=to:Q}", + "WARN: filename.mk:1: The text \":Q\" looks like a modifier but isn't.") } func (s *Suite) Test_MkVarUseChecker_checkModifiersSuffix(c *check.C) { @@ -1075,7 +1093,7 @@ func (s *Suite) Test_MkVarUseChecker_fixQuotingModifiers(c *check.C) { t.SetUpVartypes() - test := func() { + test := func(autofix bool) { mklines := t.SetUpFileMkLines("filename.mk", MkCvsID, "", diff --git a/pkgtools/pkglint/files/options.go b/pkgtools/pkglint/files/options.go index 9777fa8ff9b..49044342988 100755 --- a/pkgtools/pkglint/files/options.go +++ b/pkgtools/pkglint/files/options.go @@ -184,7 +184,8 @@ func (ck *OptionsLinesChecker) handleLowerCondition(mkline *MkLine, cond *MkCond Var: recordVarUse}) if cond.Empty != nil && cond.Empty.varname == "PKG_OPTIONS" && mkline.HasElseBranch() { - mkline.Notef("The positive branch of the .if/.else should be the one where the option is set.") + mkline.Warnf("The positive branch of the .if/.else " + + "should be the one where the option is set.") mkline.Explain( "For consistency among packages, the upper branch of this", ".if/.else statement should always handle the case where the", diff --git a/pkgtools/pkglint/files/options_test.go b/pkgtools/pkglint/files/options_test.go index f0f245a1b8c..bd3fd304b46 100755 --- a/pkgtools/pkglint/files/options_test.go +++ b/pkgtools/pkglint/files/options_test.go @@ -305,7 +305,7 @@ func (s *Suite) Test_CheckLinesOptionsMk(c *check.C) { t.CheckOutputLines( "WARN: ~/category/package/options.mk:6: l is used but not defined.", "WARN: ~/category/package/options.mk:18: Unknown option \"undeclared\".", - "NOTE: ~/category/package/options.mk:21: "+ + "WARN: ~/category/package/options.mk:21: "+ "The positive branch of the .if/.else should be the one where the option is set.", // TODO: The diagnostics should appear in the correct order. "WARN: ~/category/package/options.mk:6: "+ diff --git a/pkgtools/pkglint/files/package.go b/pkgtools/pkglint/files/package.go index 45b6a4b1b03..a542fe0693b 100644 --- a/pkgtools/pkglint/files/package.go +++ b/pkgtools/pkglint/files/package.go @@ -78,7 +78,7 @@ type Package struct { } func NewPackage(dir CurrPath) *Package { - pkgpath := G.Pkgsrc.ToRel(dir) + pkgpath := G.Pkgsrc.Rel(dir) // Package directory must be two subdirectories below the pkgsrc root. // As of November 2019, it is technically possible to create packages @@ -143,8 +143,7 @@ func (pkg *Package) load() ([]CurrPath, *MkLines, *MkLines) { if !hasPrefix(basename, "Makefile.") && !filename.HasSuffixText(".mk") { return false } - // FIXME: consider DirNoClean - if filename.DirClean().Base() == "patches" { + if filename.DirNoClean().Base() == "patches" { return false } if pkg.Pkgdir == "." { @@ -247,15 +246,14 @@ func (pkg *Package) parse(mklines *MkLines, allLines *MkLines, includingFileForU func(mkline *MkLine) {}) if includingFileForUsedCheck != "" { - mklines.CheckUsedBy(G.Pkgsrc.ToRel(includingFileForUsedCheck)) + mklines.CheckUsedBy(G.Pkgsrc.Rel(includingFileForUsedCheck)) } // For every included buildlink3.mk, include the corresponding builtin.mk // automatically since the pkgsrc infrastructure does the same. filename := mklines.lines.Filename if filename.Base() == "buildlink3.mk" { - // FIXME: consider DirNoClean - builtin := filename.DirClean().JoinNoClean("builtin.mk").CleanPath() + builtin := filename.DirNoClean().JoinNoClean("builtin.mk").CleanPath() builtinRel := G.Pkgsrc.Relpath(pkg.dir, builtin) if pkg.included.FirstTime(builtinRel.String()) && builtin.IsFile() { builtinMkLines := LoadMk(builtin, MustSucceed|LogErrors) @@ -276,7 +274,7 @@ func (pkg *Package) parseLine(mklines *MkLines, mkline *MkLine, allLines *MkLine includedMkLines, skip := pkg.loadIncluded(mkline, includingFile) if includedMkLines == nil { - pkgsrcPath := G.Pkgsrc.ToRel(mkline.File(includedFile)) + pkgsrcPath := G.Pkgsrc.Rel(mkline.File(includedFile)) if skip || mklines.indentation.HasExists(pkgsrcPath) { return true // See https://github.com/rillig/pkglint/issues/1 } @@ -372,7 +370,7 @@ func (pkg *Package) loadIncluded(mkline *MkLine, includingFile CurrPath) (includ return nil, false } - mkline.Notef("The path to the included file should be %q.", + mkline.Warnf("The path to the included file should be %q.", mkline.Rel(fullIncludedFallback)) mkline.Explain( "The .include directive first searches the file relative to the including file.", @@ -389,7 +387,7 @@ func (pkg *Package) loadIncluded(mkline *MkLine, includingFile CurrPath) (includ // their actual values. func (pkg *Package) resolveIncludedFile(mkline *MkLine, includingFilename CurrPath) RelPath { - // FIXME: resolveVariableRefs uses G.Pkg implicitly. It should be made explicit. + // XXX: resolveVariableRefs uses G.Pkg implicitly. It should be made explicit. // TODO: Try to combine resolveVariableRefs and ResolveVarsInRelativePath. resolved := mkline.ResolveVarsInRelativePath(mkline.IncludedFile()) includedText := resolveVariableRefs(nil /* XXX: or maybe some mklines? */, resolved.String()) @@ -515,7 +513,7 @@ func (pkg *Package) check(filenames []CurrPath, mklines, allLines *MkLines) { // since all those files come from calls to dirglob. break - case filename.HasBase("Makefile") && G.Pkgsrc.ToRel(filename).Count() == 3: + case filename.HasBase("Makefile") && G.Pkgsrc.Rel(filename).Count() == 3: G.checkExecutable(filename, st.Mode()) pkg.checkfilePackageMakefile(filename, mklines, allLines) @@ -593,10 +591,13 @@ func (pkg *Package) checkfilePackageMakefile(filename CurrPath, mklines *MkLines pkg.redundant = NewRedundantScope() pkg.redundant.IsRelevant = func(mkline *MkLine) bool { - if !G.Infrastructure && !G.Opts.CheckGlobal { - return !G.Pkgsrc.IsInfra(mkline.Filename) - } - return true + // As of December 2019, the RedundantScope is only used for + // checking a whole package. Therefore, G.Infrastructure can + // never be true and there is no point testing it. + // + // If the RedundantScope is applied also to individual files, + // it would have to be added here. + return G.Opts.CheckGlobal || !G.Pkgsrc.IsInfra(mkline.Filename) } pkg.redundant.Check(allLines) // Updates the variables in the scope pkg.checkCategories() @@ -941,7 +942,7 @@ func (pkg *Package) checkCategories() { return } - // FIXME: Decide what exactly this map means. + // XXX: Decide what exactly this map means. // Is it "this category has been seen somewhere", // or is it "this category has definitely been added"? seen := map[string]*MkLine{} @@ -949,7 +950,7 @@ func (pkg *Package) checkCategories() { switch mkline.Op() { case opAssignDefault: for _, category := range mkline.ValueFields(mkline.Value()) { - // FIXME: This looks wrong. It should probably be replaced by + // XXX: This looks wrong. It should probably be replaced by // an "if len(seen) == 0" outside the for loop. if seen[category] == nil { seen[category] = mkline @@ -1267,7 +1268,7 @@ func (pkg *Package) checkDirent(dirent CurrPath, mode os.FileMode) { switch { case mode.IsRegular(): - G.checkReg(dirent, basename, G.Pkgsrc.ToRel(dirent).Count()) + G.checkReg(dirent, basename, G.Pkgsrc.Rel(dirent).Count()) case hasPrefix(basename, "work"): if G.Opts.Import { @@ -1279,8 +1280,7 @@ func (pkg *Package) checkDirent(dirent CurrPath, mode os.FileMode) { switch { case basename == "files", basename == "patches", - // FIXME: consider DirNoClean - dirent.DirClean().Base() == "files", + dirent.DirNoClean().Base() == "files", isEmptyDir(dirent): break diff --git a/pkgtools/pkglint/files/package_test.go b/pkgtools/pkglint/files/package_test.go index 4684ccb5817..f7582cc6e6d 100644 --- a/pkgtools/pkglint/files/package_test.go +++ b/pkgtools/pkglint/files/package_test.go @@ -316,7 +316,7 @@ func (s *Suite) Test_Package__case_insensitive(c *check.C) { G.Check(t.File("category/package")) - // FIXME: On a case-sensitive filesystem, p5-net-dns would not be found. + // TODO: On a case-sensitive filesystem, p5-net-dns would not be found. t.CheckOutputEmpty() } @@ -1005,7 +1005,7 @@ func (s *Suite) Test_Package_parse__fallback_lookup_in_package_directory(c *chec G.Check(t.File("category/package")) t.CheckOutputLines( - "NOTE: ~/mk/pthread.buildlink3.mk:2: " + + "WARN: ~/mk/pthread.buildlink3.mk:2: " + "The path to the included file should be \"pthread.builtin.mk\".") } @@ -1503,6 +1503,38 @@ func (s *Suite) Test_Package_checkfilePackageMakefile__prefs_indirect(c *check.C t.CheckOutputEmpty() } +func (s *Suite) Test_Package_checkfilePackageMakefile__redundancy_in_infra(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package", + ".include \"../../mk/redundant.mk\"", + ".include \"redundant.mk\"") + t.CreateFileLines("mk/redundant.mk", + MkCvsID, + "INFRA_REDUNDANT:=\t# empty", + "INFRA_REDUNDANT=\t# empty") + t.CreateFileLines("category/package/redundant.mk", + MkCvsID, + "PKG_REDUNDANT:=\t# empty", + "PKG_REDUNDANT=\t# empty") + t.Chdir(".") + t.FinishSetUp() + + G.checkdirPackage("category/package") + + t.CheckOutputLines( + "NOTE: category/package/redundant.mk:3: "+ + "Definition of PKG_REDUNDANT is redundant because of line 2.", + "WARN: category/package/redundant.mk:2: "+ + "PKG_REDUNDANT is defined but not used.") + + G.Check("mk/redundant.mk") + + // The redundancy check is only performed when a whole package + // is checked. Therefore nothing is reported here. + t.CheckOutputEmpty() +} + // When a package defines PLIST_SRC, it may or may not use the // PLIST file from the package directory. Therefore the check // is skipped completely. diff --git a/pkgtools/pkglint/files/patches_test.go b/pkgtools/pkglint/files/patches_test.go index 9ac0d0dc630..fe3ed787122 100644 --- a/pkgtools/pkglint/files/patches_test.go +++ b/pkgtools/pkglint/files/patches_test.go @@ -590,7 +590,7 @@ func (s *Suite) Test_PatchChecker_Check__absolute_path(c *check.C) { CheckLinesPatch(lines) - // FIXME: Patches must not apply to absolute paths. + // XXX: Patches must not apply to absolute paths. // The only allowed exception is /dev/null. // ^(---|\+\+\+) /(?!dev/null) t.CheckOutputEmpty() diff --git a/pkgtools/pkglint/files/path.go b/pkgtools/pkglint/files/path.go index 58ee2ade6cc..fcf07d6c86d 100644 --- a/pkgtools/pkglint/files/path.go +++ b/pkgtools/pkglint/files/path.go @@ -408,8 +408,7 @@ func NewPackagePath(p RelPath) PackagePath { } func NewPackagePathString(p string) PackagePath { - _ = NewRelPathString(p) - return PackagePath(p) + return PackagePath(NewRelPathString(p)) } func (p PackagePath) AsPath() Path { return Path(p) } @@ -434,8 +433,7 @@ func NewRelPath(p Path) RelPath { } func NewRelPathString(p string) RelPath { - assert(!NewPath(p).IsAbs()) - return RelPath(p) + return NewRelPath(NewPath(p)) } func (p RelPath) AsPath() Path { return NewPath(string(p)) } diff --git a/pkgtools/pkglint/files/path_test.go b/pkgtools/pkglint/files/path_test.go index 6d09730150f..c1efe6d3e4c 100644 --- a/pkgtools/pkglint/files/path_test.go +++ b/pkgtools/pkglint/files/path_test.go @@ -87,6 +87,8 @@ func (s *Suite) Test_Path_DirNoClean(c *check.C) { test("filename", ".") test("dir/filename", "dir") test("dir/filename\\with\\backslash", "dir") + test("dir/./file", "dir") + test("./file", ".") } func (s *Suite) Test_Path_Base(c *check.C) { @@ -427,6 +429,7 @@ func (s *Suite) Test_Path_CleanDot(c *check.C) { test("", "") test(".", ".") test("./././", ".") + test("dir/", "dir/") // TODO: Or maybe "dir/."? test("a/bb///../c", "a/bb/../c") test("./filename", "filename") test("/absolute", "/absolute") diff --git a/pkgtools/pkglint/files/pkglint.1 b/pkgtools/pkglint/files/pkglint.1 index a5fadc0e49c..97b1ac283d5 100644 --- a/pkgtools/pkglint/files/pkglint.1 +++ b/pkgtools/pkglint/files/pkglint.1 @@ -1,4 +1,4 @@ -.\" $NetBSD: pkglint.1,v 1.60 2019/12/08 00:06:38 rillig Exp $ +.\" $NetBSD: pkglint.1,v 1.61 2019/12/08 22:03:38 rillig Exp $ .\" From FreeBSD: portlint.1,v 1.8 1997/11/25 14:53:14 itojun Exp .\" .\" Copyright (c) 1997 by Jun-ichiro Itoh <itojun@itojun.org>. diff --git a/pkgtools/pkglint/files/pkglint.go b/pkgtools/pkglint/files/pkglint.go index e2356bcd090..3c9998243d6 100644 --- a/pkgtools/pkglint/files/pkglint.go +++ b/pkgtools/pkglint/files/pkglint.go @@ -201,8 +201,7 @@ func (pkglint *Pkglint) setUpProfiling() func() { func (pkglint *Pkglint) prepareMainLoop() { firstDir := pkglint.Todo.Front() if firstDir.IsFile() { - // FIXME: consider DirNoClean - firstDir = firstDir.DirClean() + firstDir = firstDir.DirNoClean() } relTopdir := findPkgsrcTopdir(firstDir) @@ -318,7 +317,7 @@ func (pkglint *Pkglint) checkMode(dirent CurrPath, mode os.FileMode) { } basename := dirent.Base() - pkgsrcRel := pkglint.Pkgsrc.ToRel(dirent) + pkgsrcRel := pkglint.Pkgsrc.Rel(dirent) pkglint.Wip = pkgsrcRel.HasPrefixPath("wip") pkglint.Infrastructure = pkgsrcRel.HasPrefixPath("mk") @@ -393,7 +392,8 @@ func resolveVariableRefs(mklines *MkLines, text string) (resolved string) { } } if mklines != nil { - if value, ok := mklines.vars.LastValueFound(varname); ok { + // TODO: At load time, use mklines.loadVars instead. + if value, ok := mklines.allVars.LastValueFound(varname); ok { return value } } @@ -430,20 +430,26 @@ func CheckLinesDescr(lines *Lines) { defer trace.Call(lines.Filename)() } + checkVarRefs := func(line *Line) { + tokens, _ := NewMkLexer(line.Text, nil).MkTokens() + for _, token := range tokens { + switch { + case token.Varuse == nil, + !hasPrefix(token.Text, "${"), + G.Pkgsrc.VariableType(nil, token.Varuse.varname) == nil: + default: + line.Notef("Variables like %q are not expanded in the DESCR file.", + token.Text) + } + } + } + for _, line := range lines.Lines { ck := LineChecker{line} ck.CheckLength(80) ck.CheckTrailingWhitespace() ck.CheckValidCharacters() - - if containsVarRef(line.Text) { - tokens, _ := NewMkLexer(line.Text, nil).MkTokens() - for _, token := range tokens { - if token.Varuse != nil && G.Pkgsrc.VariableType(nil, token.Varuse.varname) != nil { - line.Notef("Variables are not expanded in the DESCR file.") - } - } - } + checkVarRefs(line) } CheckLinesTrailingEmptyLines(lines) @@ -542,8 +548,7 @@ func CheckFileMk(filename CurrPath) { func (pkglint *Pkglint) checkReg(filename CurrPath, basename string, depth int) { if depth == 3 && !pkglint.Wip { - // FIXME: There's no good reason for prohibiting a README file. - if contains(basename, "README") || contains(basename, "TODO") { + if contains(basename, "TODO") { NewLineWhole(filename).Errorf("Packages in main pkgsrc must not have a %s file.", basename) // TODO: Add a convincing explanation. return @@ -554,7 +559,6 @@ func (pkglint *Pkglint) checkReg(filename CurrPath, basename string, depth int) case hasSuffix(basename, "~"), hasSuffix(basename, ".orig"), hasSuffix(basename, ".rej"), - contains(basename, "README") && depth == 3, contains(basename, "TODO") && depth == 3: if pkglint.Opts.Import { NewLineWhole(filename).Errorf("Must be cleaned up before committing the package.") @@ -599,20 +603,16 @@ func (pkglint *Pkglint) checkReg(filename CurrPath, basename string, depth int) CheckLinesPatch(lines) } - // FIXME: consider DirNoClean - case filename.DirClean().Base() == "patches" && matches(filename.Base(), `^manual[^/]*$`): + case filename.DirNoClean().Base() == "patches" && matches(filename.Base(), `^manual[^/]*$`): if trace.Tracing { trace.Stepf("Unchecked file %q.", filename) } - // FIXME: consider DirNoClean - case filename.DirClean().Base() == "patches": + case filename.DirNoClean().Base() == "patches": NewLineWhole(filename).Warnf("Patch files should be named \"patch-\", followed by letters, '-', '_', '.', and digits only.") case (hasPrefix(basename, "Makefile") || hasSuffix(basename, ".mk")) && - // FIXME: consider DirNoClean - // FIXME: G.Pkgsrc.Rel(filename) instead of filename - !filename.DirClean().ContainsPath("files"): + !G.Pkgsrc.Rel(filename).AsPath().ContainsPath("files"): CheckFileMk(filename) case hasPrefix(basename, "PLIST"): @@ -620,16 +620,18 @@ func (pkglint *Pkglint) checkReg(filename CurrPath, basename string, depth int) CheckLinesPlist(G.Pkg, lines) } + case contains(basename, "README"): + break + case hasPrefix(basename, "CHANGES-"): // This only checks the file but doesn't register the changes globally. _ = pkglint.Pkgsrc.loadDocChangesFromFile(filename) - // FIXME: consider DirNoClean - case filename.DirClean().Base() == "files": + case filename.DirNoClean().Base() == "files": // Skip files directly in the files/ directory, but not those further down. case basename == "spec": - if !pkglint.Pkgsrc.ToRel(filename).HasPrefixPath("regress") { + if !pkglint.Pkgsrc.Rel(filename).HasPrefixPath("regress") { NewLineWhole(filename).Warnf("Only packages in regress/ may have spec files.") } @@ -731,7 +733,6 @@ func (pkglint *Pkglint) tools(mklines *MkLines) *Tools { } func (pkglint *Pkglint) loadCvsEntries(filename CurrPath) map[string]CvsEntry { - // FIXME: consider DirNoClean dir := filename.DirClean() if dir == pkglint.cvsEntriesDir { return pkglint.cvsEntries diff --git a/pkgtools/pkglint/files/pkglint_test.go b/pkgtools/pkglint/files/pkglint_test.go index fcecc850554..f60390ccede 100644 --- a/pkgtools/pkglint/files/pkglint_test.go +++ b/pkgtools/pkglint/files/pkglint_test.go @@ -237,13 +237,12 @@ func (s *Suite) Test_Pkglint_Main__complete_package(c *check.C) { "WARN: ~/sysutils/checkperms/Makefile:3: "+ "This package should be updated to 1.13 (supports more file formats; see ../../doc/TODO:5).", "ERROR: ~/sysutils/checkperms/Makefile:4: Invalid category \"tools\".", - "ERROR: ~/sysutils/checkperms/README: Packages in main pkgsrc must not have a README file.", "ERROR: ~/sysutils/checkperms/TODO: Packages in main pkgsrc must not have a TODO file.", "ERROR: ~/sysutils/checkperms/distinfo:7: SHA1 hash of patches/patch-checkperms.c differs "+ "(distinfo has asdfasdf, patch file has e775969de639ec703866c0336c4c8e0fdd96309c).", "WARN: ~/sysutils/checkperms/patches/patch-checkperms.c:12: Premature end of patch hunk "+ "(expected 1 lines to be deleted and 0 lines to be added).", - "4 errors, 2 warnings and 1 note found.", + "3 errors, 2 warnings and 1 note found.", t.Shquote("(Run \"pkglint -e -Wall -Call %s\" to show explanations.)", "sysutils/checkperms"), t.Shquote("(Run \"pkglint -fs -Wall -Call %s\" to show what can be fixed automatically.)", "sysutils/checkperms"), t.Shquote("(Run \"pkglint -F -Wall -Call %s\" to automatically fix some issues.)", "sysutils/checkperms")) @@ -754,10 +753,46 @@ func (s *Suite) Test_CheckLinesDescr(c *check.C) { // in devel/go-properties/DESCR. t.CheckOutputLines( "WARN: DESCR:1: Line too long (should be no more than 80 characters).", - "NOTE: DESCR:11: Variables are not expanded in the DESCR file.", + "NOTE: DESCR:11: Variables like \"${PREFIX}\" are not expanded in the DESCR file.", "WARN: DESCR:25: File too long (should be no more than 24 lines).") } +// The package author may think that variables like ${PREFIX} +// are expanded in DESCR files too, but that doesn't happen. +func (s *Suite) Test_CheckLinesDescr__variables(c *check.C) { + t := s.Init(c) + + t.SetUpVartypes() + + test := func(text string, diagnostics ...string) { + lines := t.NewLines("DESCR", + text) + + CheckLinesDescr(lines) + + t.CheckOutput(diagnostics) + } + + test("${PREFIX}", + + "NOTE: DESCR:1: Variables like \"${PREFIX}\" "+ + "are not expanded in the DESCR file.") + + // Variables in parentheses are unusual in pkgsrc. + // Therefore they are not worth being mentioned. + test("$(PREFIX)", nil...) + + // Variables that are not well-known in pkgsrc are not warned + // about since these are probably legitimate examples, as seen + // in devel/go-properties/DESCR. + test("${UNDEFINED}", nil...) + + test("$<", nil...) + + // This one occurs in a few Perl packages. + test("$@", nil...) +} + func (s *Suite) Test_CheckLinesMessage__one_line_of_text(c *check.C) { t := s.Init(c) @@ -990,18 +1025,15 @@ func (s *Suite) Test_Pkglint_checkReg__readme_and_todo(c *check.C) { t.Main("category/package", "wip/package") t.CheckOutputLines( - "ERROR: category/package/README: Packages in main pkgsrc must not have a README file.", "ERROR: category/package/TODO: Packages in main pkgsrc must not have a TODO file.", - "2 errors found.") + "1 error found.") t.Main("--import", "category/package", "wip/package") t.CheckOutputLines( - "ERROR: category/package/README: Packages in main pkgsrc must not have a README file.", "ERROR: category/package/TODO: Packages in main pkgsrc must not have a TODO file.", - "ERROR: wip/package/README: Must be cleaned up before committing the package.", "ERROR: wip/package/TODO: Must be cleaned up before committing the package.", - "4 errors found.") + "2 errors found.") } func (s *Suite) Test_Pkglint_checkReg__unknown_file_in_patches(c *check.C) { diff --git a/pkgtools/pkglint/files/pkgsrc.go b/pkgtools/pkglint/files/pkgsrc.go index a6f5b974742..8ecdf139224 100644 --- a/pkgtools/pkglint/files/pkgsrc.go +++ b/pkgtools/pkglint/files/pkgsrc.go @@ -150,8 +150,8 @@ func (src *Pkgsrc) loadDocChanges() { var filenames []RelPath for _, file := range files { filename := file.Name() - if matches(filename, `^CHANGES-20\d\d$`) && filename >= "CHANGES-2011" { // TODO: Why 2011? - filenames = append(filenames, NewRelPathString(filename)) // FIXME: low-level API + if matches(filename, `^CHANGES-20\d\d$`) && filename >= "CHANGES-2011" { // XXX: Why 2011? + filenames = append(filenames, NewRelPathString(filename)) // XXX: low-level API } } @@ -676,10 +676,10 @@ func (src *Pkgsrc) loadUntypedVars() { mklines := LoadMk(path, MustSucceed) mklines.collectVariables() mklines.collectUsedVariables() - for varname, mkline := range mklines.vars.firstDef { + for varname, mkline := range mklines.allVars.firstDef { define(varnameCanon(varname), mkline) } - for varname, mkline := range mklines.vars.used { + for varname, mkline := range mklines.allVars.used { define(varnameCanon(varname), mkline) } } @@ -688,7 +688,7 @@ func (src *Pkgsrc) loadUntypedVars() { assertNil(err, "handleFile %q", pathName) baseName := info.Name() if info.Mode().IsRegular() && (hasSuffix(baseName, ".mk") || baseName == "mk.conf") { - handleMkFile(NewCurrPathSlash(pathName)) // FIXME: This is too deep to handle os-specific paths + handleMkFile(NewCurrPathSlash(pathName)) // XXX: This is too deep to handle os-specific paths } return nil } @@ -1117,29 +1117,28 @@ func (src *Pkgsrc) File(relativeName PkgsrcPath) CurrPath { return src.topdir.JoinNoClean(cleaned).CleanDot() } -// ToRel returns the path of `filename`, relative to the pkgsrc top directory. +// Rel returns the path of `filename`, relative to the pkgsrc top directory. // // Example: -// NewPkgsrc("/usr/pkgsrc").ToRel("/usr/pkgsrc/distfiles") => "distfiles" -// FIXME: Rename to Rel. -func (src *Pkgsrc) ToRel(filename CurrPath) PkgsrcPath { +// NewPkgsrc("/usr/pkgsrc").Rel("/usr/pkgsrc/distfiles") => "distfiles" +func (src *Pkgsrc) Rel(filename CurrPath) PkgsrcPath { return NewPkgsrcPath(src.Relpath(src.topdir, filename).AsPath()) } // IsInfra returns whether the given filename is part of the pkgsrc // infrastructure. func (src *Pkgsrc) IsInfra(filename CurrPath) bool { - rel := src.ToRel(filename) + rel := src.Rel(filename) return rel.HasPrefixPath("mk") || rel.HasPrefixPath("wip/mk") } func (src *Pkgsrc) IsInfraMain(filename CurrPath) bool { - rel := src.ToRel(filename) + rel := src.Rel(filename) return rel.HasPrefixPath("mk") } func (src *Pkgsrc) IsWip(filename CurrPath) bool { - rel := src.ToRel(filename) + rel := src.Rel(filename) return rel.HasPrefixPath("wip") } diff --git a/pkgtools/pkglint/files/plist.go b/pkgtools/pkglint/files/plist.go index ebe8cc07a05..04e4f4fdcf1 100644 --- a/pkgtools/pkglint/files/plist.go +++ b/pkgtools/pkglint/files/plist.go @@ -2,7 +2,6 @@ package pkglint import ( "netbsd.org/pkglint/textproc" - "path" "sort" "strings" ) @@ -29,13 +28,7 @@ func CheckLinesPlist(pkg *Package, lines *Lines) { return } - ck := PlistChecker{ - pkg, - make(map[RelPath]*PlistLine), - make(map[RelPath]*PlistLine), - "", - Once{}, - false} + ck := NewPlistChecker(pkg) ck.Check(lines) } @@ -43,19 +36,29 @@ type PlistChecker struct { pkg *Package allFiles map[RelPath]*PlistLine allDirs map[RelPath]*PlistLine - lastFname string + lastFname RelPath once Once nonAsciiAllowed bool } +func NewPlistChecker(pkg *Package) *PlistChecker { + return &PlistChecker{ + pkg, + make(map[RelPath]*PlistLine), + make(map[RelPath]*PlistLine), + "", + Once{}, + false} +} + func (ck *PlistChecker) Load(lines *Lines) []*PlistLine { - plines := ck.NewLines(lines) + plines := ck.newLines(lines) ck.collectFilesAndDirs(plines) if lines.BaseName == "PLIST.common_end" { commonLines := Load(lines.Filename.TrimSuffix("_end"), NotEmpty) if commonLines != nil { - ck.collectFilesAndDirs(ck.NewLines(commonLines)) + ck.collectFilesAndDirs(ck.newLines(commonLines)) } } @@ -78,7 +81,7 @@ func (ck *PlistChecker) Check(plainLines *Lines) { } } -func (ck *PlistChecker) NewLines(lines *Lines) []*PlistLine { +func (*PlistChecker) newLines(lines *Lines) []*PlistLine { plines := make([]*PlistLine, lines.Len()) for i, line := range lines.Lines { var conditions []string @@ -103,33 +106,41 @@ var plistLineStart = textproc.NewByteSet("$0-9A-Za-z") func (ck *PlistChecker) collectFilesAndDirs(plines []*PlistLine) { for _, pline := range plines { - if text := pline.text; len(text) > 0 { - first := text[0] - switch { - case plistLineStart.Contains(first): - // FIXME: Add test for absolute path. - path := NewRelPathString(text) - if prev := ck.allFiles[path]; prev == nil || stringSliceLess(pline.conditions, prev.conditions) { - ck.allFiles[path] = pline - } - // FIXME: consider DirNoClean - // FIXME: consider DirNoClean - for dir := path.DirClean(); dir != "."; dir = dir.DirClean() { - ck.allDirs[dir] = pline - } - case first == '@': - if m, dirname := match1(text, `^@exec \$\{MKDIR\} %D/(.*)$`); m { - // FIXME: consider DirNoClean - // FIXME: Add test for absolute path. - for dir := NewRelPathString(dirname); dir != "."; dir = dir.DirClean() { - ck.allDirs[dir] = pline - } - } - } + text := pline.text + switch { + case text == "": + break + case plistLineStart.Contains(text[0]): + ck.collectPath(NewRelPathString(text), pline) + case text[0] == '@': + ck.collectDirective(pline) } } } +func (ck *PlistChecker) collectPath(rel RelPath, pline *PlistLine) { + + // TODO: What about paths containing variables? + // Are they intended to be collected as well? + + if prev := ck.allFiles[rel]; prev == nil || stringSliceLess(pline.conditions, prev.conditions) { + ck.allFiles[rel] = pline + } + for dir := rel.DirNoClean(); dir != "."; dir = dir.DirNoClean() { + ck.allDirs[dir] = pline + } +} + +func (ck *PlistChecker) collectDirective(pline *PlistLine) { + m, dirname := match1(pline.text, `^@exec \$\{MKDIR\} %D/(.*)$`) + if !m || NewPath(dirname).IsAbs() { + return + } + for dir := NewRelPathString(dirname); dir != "."; dir = dir.DirNoClean() { + ck.allDirs[dir] = pline + } +} + func (ck *PlistChecker) checkLine(pline *PlistLine) { text := pline.text @@ -139,32 +150,30 @@ func (ck *PlistChecker) checkLine(pline *PlistLine) { fix.Delete() fix.Apply() - } else if textproc.AlnumU.Contains(text[0]) || text[0] == '$' { - ck.checkPath(pline) + } else if plistLineStart.Contains(text[0]) { + ck.checkPath(pline, pline.Path()) } else if m, cmd, arg := match2(text, `^@([a-z-]+)[\t ]*(.*)`); m { pline.CheckDirective(cmd, arg) - ck.nonAsciiAllowed = pline.firstLine > 1 + if cmd == "comment" && pline.firstLine > 1 { + ck.nonAsciiAllowed = true + } } else { - pline.Warnf("Invalid line type: %s", pline.Line.Text) + pline.Errorf("Invalid line type: %s", pline.Line.Text) } } -func (ck *PlistChecker) checkPath(pline *PlistLine) { - text := pline.text - dirSlash, basename := path.Split(text) - dirname := strings.TrimSuffix(dirSlash, "/") - +func (ck *PlistChecker) checkPath(pline *PlistLine, rel RelPath) { ck.checkPathNonAscii(pline) ck.checkSorted(pline) ck.checkDuplicate(pline) - if contains(basename, "${IMAKE_MANNEWSUFFIX}") { + if contains(rel.Base(), "${IMAKE_MANNEWSUFFIX}") { pline.warnImakeMannewsuffix() } - if hasPrefix(text, "${PKGMANDIR}/") { + if rel.HasPrefixPath("${PKGMANDIR}") { fix := pline.Autofix() fix.Notef("PLIST files should use \"man/\" instead of \"${PKGMANDIR}\".") fix.Explain( @@ -177,44 +186,53 @@ func (ck *PlistChecker) checkPath(pline *PlistLine) { pline.text = strings.Replace(pline.text, "${PKGMANDIR}/", "man/", 1) } - topdir := strings.SplitN(text, "/", 2)[0] + topdir := rel.Parts()[0] switch topdir { case "bin": - ck.checkPathBin(pline, dirname, basename) + ck.checkPathBin(pline, rel) case "doc": pline.Errorf("Documentation must be installed under share/doc, not doc.") case "etc": - ck.checkPathEtc(pline, dirname, basename) + ck.checkPathEtc(pline) case "info": - ck.checkPathInfo(pline, dirname, basename) + ck.checkPathInfo(pline) case "lib": - ck.checkPathLib(pline, dirname, basename) + ck.checkPathLib(pline, rel) case "man": ck.checkPathMan(pline) case "share": ck.checkPathShare(pline) } - if contains(text, "${PKGLOCALEDIR}") && ck.pkg != nil && !ck.pkg.vars.IsDefined("USE_PKGLOCALEDIR") { + ck.checkPathMisc(rel, pline) +} + +func (ck *PlistChecker) checkPathMisc(rel RelPath, pline *PlistLine) { + if rel.ContainsText("${PKGLOCALEDIR}") && ck.pkg != nil && !ck.pkg.vars.IsDefined("USE_PKGLOCALEDIR") { pline.Warnf("PLIST contains ${PKGLOCALEDIR}, but USE_PKGLOCALEDIR is not set in the package Makefile.") } - if contains(text, "/CVS/") { + if rel.ContainsPath("CVS") { pline.Warnf("CVS files should not be in the PLIST.") } - if hasSuffix(text, ".orig") { + if rel.HasSuffixText(".orig") { pline.Warnf(".orig files should not be in the PLIST.") } - if hasSuffix(text, "/perllocal.pod") { + if rel.HasBase("perllocal.pod") { pline.Warnf("The perllocal.pod file should not be in the PLIST.") pline.Explain( "This file is handled automatically by the INSTALL/DEINSTALL scripts", "since its contents depends on more than one package.") } - if contains(text, ".egg-info/") { + if rel.ContainsText(".egg-info/") { pline.Warnf("Include \"../../lang/python/egg.mk\" instead of listing .egg-info files directly.") } + if rel.ContainsPath("..") { + pline.Errorf("Paths in PLIST files must not contain \"..\".") + } else if canonical := rel.Clean(); canonical != rel { + pline.Errorf("Paths in PLIST files must be canonical (%s).", canonical) + } } func (ck *PlistChecker) checkPathNonAscii(pline *PlistLine) { @@ -246,38 +264,38 @@ func (ck *PlistChecker) checkPathNonAscii(pline *PlistLine) { } func (ck *PlistChecker) checkSorted(pline *PlistLine) { - if text := pline.text; hasAlnumPrefix(text) && !containsVarRef(text) { - if ck.lastFname != "" { - if ck.lastFname > text && !G.Logger.Opts.Autofix { - pline.Warnf("%q should be sorted before %q.", text, ck.lastFname) - pline.Explain( - "The files in the PLIST should be sorted alphabetically.", - "This allows human readers to quickly see whether a file is included or not.") - } - } - ck.lastFname = text + if !pline.HasPlainPath() { + return + } + + rel := pline.Path() + if ck.lastFname != "" && ck.lastFname > rel && !G.Logger.Opts.Autofix { + pline.Warnf("%q should be sorted before %q.", rel.String(), ck.lastFname.String()) + pline.Explain( + "The files in the PLIST should be sorted alphabetically.", + "This allows human readers to quickly see whether a file is included or not.") } + ck.lastFname = rel } func (ck *PlistChecker) checkDuplicate(pline *PlistLine) { - text := pline.text - if !hasAlnumPrefix(text) || containsVarRef(text) { + if !pline.HasPlainPath() { return } - prev := ck.allFiles[NewRelPathString(text)] + prev := ck.allFiles[pline.Path()] if prev == pline || len(prev.conditions) > 0 { return } fix := pline.Autofix() - fix.Errorf("Duplicate filename %q, already appeared in %s.", text, pline.RelLine(prev.Line)) + fix.Errorf("Duplicate filename %q, already appeared in %s.", pline.text, pline.RelLine(prev.Line)) fix.Delete() fix.Apply() } -func (ck *PlistChecker) checkPathBin(pline *PlistLine, dirname, basename string) { - if contains(dirname, "/") { +func (ck *PlistChecker) checkPathBin(pline *PlistLine, rel RelPath) { + if rel.Count() > 2 { pline.Warnf("The bin/ directory should not have subdirectories.") pline.Explain( "The programs in bin/ are collected there to be executable by the", @@ -288,7 +306,7 @@ func (ck *PlistChecker) checkPathBin(pline *PlistLine, dirname, basename string) } } -func (ck *PlistChecker) checkPathEtc(pline *PlistLine, dirname, basename string) { +func (ck *PlistChecker) checkPathEtc(pline *PlistLine) { if hasPrefix(pline.text, "etc/rc.d/") { pline.Errorf("RCD_SCRIPTS must not be registered in the PLIST.") pline.Explain( @@ -301,7 +319,7 @@ func (ck *PlistChecker) checkPathEtc(pline *PlistLine, dirname, basename string) "Please use the CONF_FILES framework, which is described in mk/pkginstall/bsd.pkginstall.mk.") } -func (ck *PlistChecker) checkPathInfo(pline *PlistLine, dirname, basename string) { +func (ck *PlistChecker) checkPathInfo(pline *PlistLine) { if pline.text == "info/dir" { pline.Errorf("\"info/dir\" must not be listed. Use install-info to add/remove an entry.") return @@ -312,20 +330,23 @@ func (ck *PlistChecker) checkPathInfo(pline *PlistLine, dirname, basename string } } -func (ck *PlistChecker) checkPathLib(pline *PlistLine, dirname, basename string) { +func (ck *PlistChecker) checkPathLib(pline *PlistLine, rel RelPath) { switch { - case hasPrefix(pline.text, "lib/locale/"): + case rel.HasPrefixPath("lib/locale"): pline.Errorf("\"lib/locale\" must not be listed. Use ${PKGLOCALEDIR}/locale and set USE_PKGLOCALEDIR instead.") return } + basename := rel.Base() if contains(basename, ".a") || contains(basename, ".so") { - if m, noext := match1(pline.text, `^(.*)(?:\.a|\.so[0-9.]*)$`); m { - // FIXME: Add test for absolute path. - if laLine := ck.allFiles[NewRelPathString(noext+".la")]; laLine != nil { - pline.Warnf("Redundant library found. The libtool library is in %s.", pline.RelLine(laLine.Line)) + la := replaceAll(pline.text, `(\.a|\.so[0-9.]*)$`, ".la") + if la != pline.text { + laLine := ck.allFiles[NewRelPathString(la)] + if laLine != nil { + pline.Warnf("Redundant library found. The libtool library is in %s.", + pline.RelLine(laLine.Line)) } } } @@ -445,6 +466,15 @@ type PlistLine struct { text string // Line.Text without any conditions of the form ${PLIST.cond} } +func (pline *PlistLine) Path() RelPath { return NewRelPathString(pline.text) } + +func (pline *PlistLine) HasPlainPath() bool { + text := pline.text + return text != "" && + plistLineStart.Contains(text[0]) && + !containsVarRef(text) +} + func (pline *PlistLine) CheckTrailingWhitespace() { if hasSuffix(pline.text, " ") || hasSuffix(pline.text, "\t") { pline.Errorf("Pkgsrc does not support filenames ending in whitespace.") @@ -549,6 +579,7 @@ func NewPlistLineSorter(plines []*PlistLine) *plistLineSorter { func (s *plistLineSorter) Sort() { if line := s.unsortable; line != nil { if G.Logger.IsAutofix() { + // FIXME: Missing trace.Enabled trace.Stepf("%s: This line prevents pkglint from sorting the PLIST automatically.", line) } return diff --git a/pkgtools/pkglint/files/plist_test.go b/pkgtools/pkglint/files/plist_test.go index 9a00cc75987..8dfc3f51e02 100644 --- a/pkgtools/pkglint/files/plist_test.go +++ b/pkgtools/pkglint/files/plist_test.go @@ -24,7 +24,8 @@ func (s *Suite) Test_CheckLinesPlist(c *check.C) { "share/icons/hicolor/icon1.png", "share/icons/hicolor/icon2.png", // No additional error for hicolor-icon-theme. "share/tzinfo", - "share/tzinfo") + "share/tzinfo", + "/absolute") CheckLinesPlist(G.Pkg, lines) @@ -45,7 +46,8 @@ func (s *Suite) Test_CheckLinesPlist(c *check.C) { "WARN: PLIST:14: Packages that install icon theme files should set ICON_THEMES.", "ERROR: PLIST:15: Packages that install hicolor icons "+ "must include \"../../graphics/hicolor-icon-theme/buildlink3.mk\" in the Makefile.", - "ERROR: PLIST:18: Duplicate filename \"share/tzinfo\", already appeared in line 17.") + "ERROR: PLIST:18: Duplicate filename \"share/tzinfo\", already appeared in line 17.", + "ERROR: PLIST:19: Invalid line type: /absolute") } func (s *Suite) Test_CheckLinesPlist__single_file_no_comment(c *check.C) { @@ -336,11 +338,11 @@ func (s *Suite) Test_PlistChecker__invalid_line_type(c *check.C) { CheckLinesPlist(nil, lines) t.CheckOutputLines( - "WARN: ~/PLIST:2: Invalid line type: ---invalid", - "WARN: ~/PLIST:3: Invalid line type: +++invalid", - "WARN: ~/PLIST:4: Invalid line type: <<<<<<<< merge conflict", - "WARN: ~/PLIST:5: Invalid line type: ======== merge conflict", - "WARN: ~/PLIST:6: Invalid line type: >>>>>>>> merge conflict") + "ERROR: ~/PLIST:2: Invalid line type: ---invalid", + "ERROR: ~/PLIST:3: Invalid line type: +++invalid", + "ERROR: ~/PLIST:4: Invalid line type: <<<<<<<< merge conflict", + "ERROR: ~/PLIST:5: Invalid line type: ======== merge conflict", + "ERROR: ~/PLIST:6: Invalid line type: >>>>>>>> merge conflict") } func (s *Suite) Test_PlistChecker__doc(c *check.C) { @@ -400,6 +402,153 @@ func (s *Suite) Test_PlistChecker__PKGLOCALEDIR_without_package(c *check.C) { t.CheckOutputEmpty() } +func (s *Suite) Test_NewPlistChecker(c *check.C) { + t := s.Init(c) + + pkg := NewPackage(t.File("category/package")) + + ck := NewPlistChecker(pkg) + + t.CheckEquals(ck.pkg, pkg) + t.Check(ck.allDirs, check.NotNil) + t.Check(ck.allFiles, check.NotNil) +} + +func (s *Suite) Test_PlistChecker_Load__common_end(c *check.C) { + t := s.Init(c) + + t.Chdir(".") + t.CreateFileLines("PLIST", + PlistCvsID, + "bin/plist") + t.CreateFileLines("PLIST.common", + PlistCvsID, + "bin/plist_common") + t.CreateFileLines("PLIST.common_end", + PlistCvsID, + "bin/plist_common_end") + + ck := NewPlistChecker(nil) + + plistLines := ck.Load(Load(t.File("PLIST.common_end"), MustSucceed)) + + // The corresponding PLIST.common is loaded if possible. + // Its lines are not appended to plistLines since they + // are checked separately. + t.Check(plistLines, check.HasLen, 2) + + // But the files and directories from PLIST.common are registered, + // to check for duplicates and to make these lists available to + // the package being checked, for cross-validation. + t.Check(ck.allFiles["bin/plist"], check.IsNil) + t.CheckEquals( + ck.allFiles["bin/plist_common"].String(), + "PLIST.common:2: bin/plist_common") + t.CheckEquals( + ck.allFiles["bin/plist_common_end"].String(), + "PLIST.common_end:2: bin/plist_common_end") +} + +func (s *Suite) Test_PlistChecker_Check(c *check.C) { + t := s.Init(c) + + lines := t.NewLines("PLIST", + "bin/subdir/program") + ck := NewPlistChecker(nil) + + ck.Check(lines) + + t.CheckOutputLines( + "WARN: PLIST:1: The bin/ directory should not have subdirectories.") +} + +func (s *Suite) Test_PlistChecker_newLines(c *check.C) { + t := s.Init(c) + + lines := t.NewLines("PLIST", + "bin/program", + "${PLIST.cond}bin/conditional", + "${PLIST.abs}${PLIST.abs2}/bin/conditional-absolute", + "${PLIST.mod:Q}invalid") + + plistLines := (*PlistChecker)(nil).newLines(lines) + + // The invalid condition in line 4 is silently skipped when the + // lines are parsed. The actual check happens later. + + t.Check(plistLines, check.HasLen, 4) + t.CheckEquals(plistLines[0].text, "bin/program") + t.CheckEquals(plistLines[1].text, "bin/conditional") + t.CheckEquals(plistLines[2].text, "/bin/conditional-absolute") + t.CheckEquals(plistLines[3].text, "${PLIST.mod:Q}invalid") + + t.Check(plistLines[0].conditions, check.HasLen, 0) + t.CheckDeepEquals(plistLines[1].conditions, []string{"PLIST.cond"}) + t.CheckDeepEquals(plistLines[2].conditions, []string{"PLIST.abs", "PLIST.abs2"}) + t.Check(plistLines[3].conditions, check.HasLen, 0) +} + +func (s *Suite) Test_PlistChecker_collectFilesAndDirs(c *check.C) { + t := s.Init(c) + + lines := t.NewLines("PLIST", + PlistCvsID, + "bin/program", + "man/man1/program.1", + "/absolute", + "${PLIST.cond}/absolute", + "@exec ${MKDIR} %D//absolute") + ck := NewPlistChecker(nil) + plistLines := ck.newLines(lines) + + ck.collectFilesAndDirs(plistLines) + + t.CheckDeepEquals(keys(ck.allDirs), + []string{"bin", "man", "man/man1"}) + t.CheckDeepEquals(keys(ck.allFiles), + []string{"bin/program", "man/man1/program.1"}) +} + +func (s *Suite) Test_PlistChecker_collectPath(c *check.C) { + t := s.Init(c) + + line := t.NewLine("PLIST", 1, "a/b/c/program") + ck := NewPlistChecker(nil) + + ck.collectPath("a/b/c/program", &PlistLine{line, nil, line.Text}) + + t.CheckDeepEquals(keys(ck.allDirs), + []string{"a", "a/b", "a/b/c"}) + t.CheckDeepEquals(keys(ck.allFiles), + []string{"a/b/c/program"}) +} + +func (s *Suite) Test_PlistChecker_collectDirective(c *check.C) { + t := s.Init(c) + + test := func(directive string, dirs ...string) { + line := t.NewLine("PLIST", 1, directive) + ck := NewPlistChecker(nil) + + ck.collectDirective(&PlistLine{line, nil, line.Text}) + + t.CheckDeepEquals(keys(ck.allDirs), dirs) + t.Check(keys(ck.allFiles), check.HasLen, 0) + } + + test("@exec ${MKDIR} %D/a/b/c", + "a", "a/b", "a/b/c") + + test("@exec echo hello", + nil...) + + test("@exec ${MKDIR} %D//absolute", + nil...) + + test("@exec ${MKDIR} %D/a/../../../breakout", + "a", "a/..", "a/../..", "a/../../..", "a/../../../breakout") +} + func (s *Suite) Test_PlistChecker_checkLine(c *check.C) { t := s.Init(c) @@ -430,7 +579,7 @@ func (s *Suite) Test_PlistChecker_checkLine(c *check.C) { "WARN: PLIST:4: \"bin/arm-linux-only\" should be sorted before \"bin/conditional-program\".", "WARN: PLIST:10: PLISTs should not contain empty lines.", "WARN: PLIST:11: PLISTs should not contain empty lines.", - "WARN: PLIST:14: Invalid line type: <<<<<<<<< merge conflict") + "ERROR: PLIST:14: Invalid line type: <<<<<<<<< merge conflict") } func (s *Suite) Test_PlistChecker_checkPath__PKGMANDIR(c *check.C) { @@ -446,7 +595,7 @@ func (s *Suite) Test_PlistChecker_checkPath__PKGMANDIR(c *check.C) { "NOTE: PLIST:2: PLIST files should use \"man/\" instead of \"${PKGMANDIR}\".") } -func (s *Suite) Test_PlistChecker_checkPath__python_egg(c *check.C) { +func (s *Suite) Test_PlistChecker_checkPathMisc__python_egg(c *check.C) { t := s.Init(c) lines := t.NewLines("PLIST", @@ -459,21 +608,38 @@ func (s *Suite) Test_PlistChecker_checkPath__python_egg(c *check.C) { "WARN: PLIST:2: Include \"../../lang/python/egg.mk\" instead of listing .egg-info files directly.") } -func (s *Suite) Test_PlistChecker_checkPath__unwanted_entries(c *check.C) { +func (s *Suite) Test_PlistChecker_checkPathMisc__unwanted_entries(c *check.C) { t := s.Init(c) lines := t.SetUpFileLines("PLIST", PlistCvsID, "share/perllocal.pod", "share/pkgbase/CVS/Entries", - "share/pkgbase/Makefile.orig") + "share/pkgbase/Makefile.orig", + "../breakout", + "t/../../breakout", + "t/../../breakout/${VAR}", + "t/./non-canonical", + "t///non-canonical", + "t///non-canonical/${VAR}", + "t///non-canonical${VAR}", + "t/non-canonical/", + "t/ok/${VAR}") CheckLinesPlist(nil, lines) t.CheckOutputLines( "WARN: ~/PLIST:2: The perllocal.pod file should not be in the PLIST.", "WARN: ~/PLIST:3: CVS files should not be in the PLIST.", - "WARN: ~/PLIST:4: .orig files should not be in the PLIST.") + "WARN: ~/PLIST:4: .orig files should not be in the PLIST.", + "ERROR: ~/PLIST:5: Invalid line type: ../breakout", + "ERROR: ~/PLIST:6: Paths in PLIST files must not contain \"..\".", + "ERROR: ~/PLIST:7: Paths in PLIST files must not contain \"..\".", + "ERROR: ~/PLIST:8: Paths in PLIST files must be canonical (t/non-canonical).", + "ERROR: ~/PLIST:9: Paths in PLIST files must be canonical (t/non-canonical).", + "ERROR: ~/PLIST:10: Paths in PLIST files must be canonical (t/non-canonical/${VAR}).", + "ERROR: ~/PLIST:11: Paths in PLIST files must be canonical (t/non-canonical${VAR}).", + "ERROR: ~/PLIST:12: Paths in PLIST files must be canonical (t/non-canonical).") } func (s *Suite) Test_PlistChecker_checkPathNonAscii(c *check.C) { @@ -508,6 +674,11 @@ func (s *Suite) Test_PlistChecker_checkPathNonAscii(c *check.C) { "sbin/iconv", "sbin/\U0001F603", // Smiling face with open mouth + + // Directives other than comments do not allow non-ASCII. + "unicode/00FC/reset", + "@exec true", + "unicode/00FC/\u00FC", // u-umlaut ) CheckLinesPlist(nil, lines) @@ -526,7 +697,65 @@ func (s *Suite) Test_PlistChecker_checkPathNonAscii(c *check.C) { "\tcontain non-ASCII filenames.", "", "WARN: PLIST:5: Non-ASCII filename \"dir2/<U+0633><U+0644><U+0627><U+0645>\".", - "WARN: PLIST:11: Non-ASCII filename \"sbin/<U+1F603>\".") + "WARN: PLIST:11: Non-ASCII filename \"sbin/<U+1F603>\".", + "WARN: PLIST:14: Non-ASCII filename \"unicode/00FC/<U+00FC>\".") +} + +func (s *Suite) Test_PlistChecker_checkSorted(c *check.C) { + t := s.Init(c) + + lines := t.NewLines("PLIST", + PlistCvsID, + "bin/program2", + "bin/program1") + + CheckLinesPlist(nil, lines) + + t.CheckOutputLines( + "WARN: PLIST:3: \"bin/program1\" should be " + + "sorted before \"bin/program2\".") +} + +func (s *Suite) Test_PlistChecker_checkDuplicate(c *check.C) { + t := s.Init(c) + + lines := t.NewLines("PLIST", + PlistCvsID, + "bin/program", + "bin/program") + + CheckLinesPlist(nil, lines) + + t.CheckOutputLines( + "ERROR: PLIST:3: Duplicate filename \"bin/program\", " + + "already appeared in line 2.") +} + +func (s *Suite) Test_PlistChecker_checkPathBin(c *check.C) { + t := s.Init(c) + + lines := t.NewLines("PLIST", + PlistCvsID, + "bin", + "bin/subdir/program") + + CheckLinesPlist(nil, lines) + + t.CheckOutputLines( + "WARN: PLIST:3: The bin/ directory should not have subdirectories.") +} + +func (s *Suite) Test_PlistChecker_checkPathEtc(c *check.C) { + t := s.Init(c) + + lines := t.NewLines("PLIST", + PlistCvsID, + "etc/config") + + CheckLinesPlist(nil, lines) + + t.CheckOutputLines( + "ERROR: PLIST:2: Configuration files must not be registered in the PLIST.") } func (s *Suite) Test_PlistChecker_checkPathInfo(c *check.C) { @@ -775,6 +1004,38 @@ func (s *Suite) Test_PlistChecker_checkPathShareIcons__hicolor_ok(c *check.C) { t.CheckOutputEmpty() } +func (s *Suite) Test_PlistLine_Path(c *check.C) { + t := s.Init(c) + + t.CheckEquals( + (&PlistLine{text: "relative"}).Path(), + NewRelPathString("relative")) + + t.ExpectAssert( + func() { (&PlistLine{text: "/absolute"}).Path() }) +} + +func (s *Suite) Test_PlistLine_HasPlainPath(c *check.C) { + t := s.Init(c) + + test := func(text string, hasPlainPath bool) { + t.CheckEquals((&PlistLine{text: text}).HasPlainPath(), hasPlainPath) + } + + test("abc", true) + test("9plan", true) + test("bin/program", true) + + test("", false) + test("@", false) + test(":", false) + test("/absolute", false) + test("-rf", false) + test("\\", false) + test("bin/$<", false) + test("bin/${VAR}", false) +} + func (s *Suite) Test_PlistLine_CheckTrailingWhitespace(c *check.C) { t := s.Init(c) @@ -880,7 +1141,7 @@ func (s *Suite) Test_plistLineSorter_Sort(c *check.C) { "lib/after.la", "@exec echo \"after lib/after.la\"") ck := PlistChecker{nil, nil, nil, "", Once{}, false} - plines := ck.NewLines(lines) + plines := ck.newLines(lines) sorter1 := NewPlistLineSorter(plines) t.CheckEquals(sorter1.unsortable, lines.Lines[5]) @@ -888,7 +1149,7 @@ func (s *Suite) Test_plistLineSorter_Sort(c *check.C) { cleanedLines := append(append(lines.Lines[0:5], lines.Lines[6:8]...), lines.Lines[9:]...) // Remove ${UNKNOWN} and @exec sorter2 := NewPlistLineSorter((&PlistChecker{nil, nil, nil, "", Once{}, false}). - NewLines(NewLines(lines.Filename, cleanedLines))) + newLines(NewLines(lines.Filename, cleanedLines))) c.Check(sorter2.unsortable, check.IsNil) diff --git a/pkgtools/pkglint/files/redundantscope.go b/pkgtools/pkglint/files/redundantscope.go index ae29e7bd0d0..b7303a12693 100644 --- a/pkgtools/pkglint/files/redundantscope.go +++ b/pkgtools/pkglint/files/redundantscope.go @@ -97,7 +97,7 @@ func (s *RedundantScope) handleVarassign(mkline *MkLine, ind *Indentation) { effOp = opAssign } - // FIXME: Skip the whole redundancy check if the value is not known to be constant. + // TODO: Skip the whole redundancy check if the value is not known to be constant. if effOp == opAssign && info.vari.Value() == value { effOp = opAssignDefault } diff --git a/pkgtools/pkglint/files/shell.go b/pkgtools/pkglint/files/shell.go index ae1eed4ef34..c0c0338029d 100644 --- a/pkgtools/pkglint/files/shell.go +++ b/pkgtools/pkglint/files/shell.go @@ -128,7 +128,7 @@ func (scc *SimpleCommandChecker) handleCommandVariable() bool { // When the package author has explicitly defined a command // variable, assume it to be valid. - if scc.MkLines.vars.IsDefinedSimilar(varname) { + if scc.MkLines.allVars.IsDefinedSimilar(varname) { return true } @@ -531,7 +531,7 @@ func (ck *ShellLineChecker) checkPipeExitcode(pipeline *MkShPipeline) { var shellCommandsType = NewVartype(BtShellCommands, NoVartypeOptions, NewACLEntry("*", aclpAllRuntime)) -// FIXME: Why is this called shell_Word_Vuc and not shell_Commands_Vuc? +// XXX: Why is this called shell_Word_Vuc and not shell_Commands_Vuc? var shellWordVuc = &VarUseContext{shellCommandsType, VucUnknownTime, VucQuotPlain, false} func NewShellLineChecker(mklines *MkLines, mkline *MkLine) *ShellLineChecker { @@ -657,7 +657,7 @@ func (ck *ShellLineChecker) CheckShellCommand(shellcmd string, pSetE *bool, time line := ck.mkline.Line program, err := parseShellProgram(line, shellcmd) - // FIXME: This code is duplicated in checkWordQuoting. + // XXX: This code is duplicated in checkWordQuoting. if err != nil && contains(shellcmd, "$$(") { // Hack until the shell parser can handle subshells. line.Warnf("Invoking subshells via $(...) is not portable enough.") return diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go index 27e56801127..6fbbbb76e96 100644 --- a/pkgtools/pkglint/files/shell_test.go +++ b/pkgtools/pkglint/files/shell_test.go @@ -131,7 +131,7 @@ func (s *Suite) Test_SimpleCommandChecker_handleCommandVariable__followed_by_lit // On the contrary, when pkglint checks a single .mk file, these // commands are not guaranteed to be defined, not even when the // .mk file includes the file defining the command. -// FIXME: This paragraph sounds wrong. All commands from included files should be valid. +// TODO: This paragraph sounds wrong. All commands from included files should be valid. // // The PYTHON_BIN variable below must not be called *_CMD, or another code path is taken. func (s *Suite) Test_SimpleCommandChecker_handleCommandVariable__from_package(c *check.C) { @@ -1222,9 +1222,9 @@ func (s *Suite) Test_ShellLineChecker_CheckShellCommand__subshell(c *check.C) { mklines.Check() - // FIXME: Fix the parse errors (nested subshells). - // FIXME: Fix the duplicate diagnostic in line 6. - // FIXME: "(" is not a shell command, it's an operator. + // XXX: Fix the parse errors (nested subshells). + // XXX: Fix the duplicate diagnostic in line 6. + // TODO: "(" is not a shell command, it's an operator. t.CheckOutputLines( "WARN: Makefile:4: The shell command \"(\" should not be hidden.", "WARN: Makefile:5: Internal pkglint error in ShTokenizer.ShAtom at \"$$(echo 1024))\" (quoting=S).", @@ -1350,7 +1350,7 @@ func (s *Suite) Test_ShellLineChecker_CheckWord__dquot_dollar(c *check.C) { ck.CheckWord(ck.mkline.ShellCommand(), false, RunTime) - // FIXME: Make consumes the dollar silently. + // XXX: Make consumes the dollar silently. // This could be worth another pkglint warning. t.CheckOutputEmpty() } diff --git a/pkgtools/pkglint/files/shtokenizer_test.go b/pkgtools/pkglint/files/shtokenizer_test.go index 8f0c0e9f610..81ae1f580a6 100644 --- a/pkgtools/pkglint/files/shtokenizer_test.go +++ b/pkgtools/pkglint/files/shtokenizer_test.go @@ -41,7 +41,7 @@ func (s *Suite) Test_ShTokenizer__examples_from_fuzzing(c *check.C) { "WARN: filename.mk:2: Pkglint ShellLine.CheckShellCommand: splitIntoShellTokens couldn't parse \"\\\"`'`y\"") // Covers shAtomDquotBackt: return nil - // FIXME: Pkglint must parse unescaped dollar in the same way, everywhere. + // XXX: Pkglint should parse unescaped dollar in the same way, everywhere. test( "\"`$|", "WARN: filename.mk:2: Internal pkglint error in ShTokenizer.ShAtom at \"$|\" (quoting=db).", @@ -49,7 +49,7 @@ func (s *Suite) Test_ShTokenizer__examples_from_fuzzing(c *check.C) { "WARN: filename.mk:2: Internal pkglint error in MkLine.Tokenize at \"$|\".") // Covers shAtomDquotBacktDquot: return nil - // FIXME: Pkglint must support unlimited nesting. + // XXX: Pkglint should support unlimited nesting. test( "\"`\"`", "WARN: filename.mk:2: Internal pkglint error in ShTokenizer.ShAtom at \"`\" (quoting=dbd).", diff --git a/pkgtools/pkglint/files/substcontext.go b/pkgtools/pkglint/files/substcontext.go index 2137b0a3f03..9b73c1aa52b 100644 --- a/pkgtools/pkglint/files/substcontext.go +++ b/pkgtools/pkglint/files/substcontext.go @@ -142,7 +142,7 @@ func (ctx *SubstContext) Varassign(mkline *MkLine) { fix.Replace("pre-patch", "post-extract") fix.Replace("post-patch", "pre-configure") fix.Apply() - // FIXME: Add test that has "SUBST_STAGE.id=pre-patch # or rather post-patch?" + // XXX: Add test that has "SUBST_STAGE.id=pre-patch # or rather post-patch?" } if G.Pkg != nil && (value == "pre-configure" || value == "post-configure") { diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go index 424a4884fff..39ae8dcc902 100644 --- a/pkgtools/pkglint/files/util.go +++ b/pkgtools/pkglint/files/util.go @@ -449,7 +449,17 @@ func mkopSubst(s string, left bool, from string, right bool, to string, flags st } func containsVarRef(s string) bool { - return contains(s, "${") || contains(s, "$(") + if !contains(s, "$") { + return false + } + lex := NewMkLexer(s, nil) + tokens, _ := lex.MkTokens() + for _, token := range tokens { + if token.Varuse != nil { + return true + } + } + return false } func hasAlnumPrefix(s string) bool { return s != "" && textproc.AlnumU.Contains(s[0]) } diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go index f21f2a829f6..eb2e1b4f460 100644 --- a/pkgtools/pkglint/files/util_test.go +++ b/pkgtools/pkglint/files/util_test.go @@ -436,24 +436,24 @@ func (s *Suite) Test_containsVarRef(c *check.C) { test("$", false) // A syntax error. // See the bmake manual page. - test("$>", false) // FIXME: true; .ALLSRC - test("$!", false) // FIXME: true; .ARCHIVE - test("$<", false) // FIXME: true; .IMPSRC - test("$%", false) // FIXME: true; .MEMBER - test("$?", false) // FIXME: true; .OODATE - test("$*", false) // FIXME: true; .PREFIX - test("$@", false) // FIXME: true; .TARGET - - test("$V", false) // FIXME: true - test("$v", false) // FIXME: true + test("$>", true) // .ALLSRC + test("$!", true) // .ARCHIVE + test("$<", true) // .IMPSRC + test("$%", true) // .MEMBER + test("$?", true) // .OODATE + test("$*", true) // .PREFIX + test("$@", true) // .TARGET + + test("$V", true) + test("$v", true) test("${Var}", true) test("${VAR.${param}}", true) test("$(VAR)", true) - test("$$", false) // An escaped dollar character. - test("$$(VAR)", true) // FIXME: false; An escaped dollar character; probably a subshell. - test("$${VAR}", true) // FIXME: false; An escaped dollar character; probably a shell variable. - test("$$VAR", false) // An escaped dollar character. + test("$$", false) // An escaped dollar character. + test("$$(VAR)", false) // An escaped dollar character; probably a subshell. + test("$${VAR}", false) // An escaped dollar character; probably a shell variable. + test("$$VAR", false) // An escaped dollar character. } func (s *Suite) Test_hasAlnumPrefix(c *check.C) { @@ -645,7 +645,8 @@ func (s *Suite) Test_Scope_LastValue(c *check.C) { mklines.Check() - t.CheckEquals(mklines.vars.LastValue("VAR"), "third (conditional)") + // TODO: At load time, use loadVars instead of allVars. + t.CheckEquals(mklines.allVars.LastValue("VAR"), "third (conditional)") t.CheckOutputLines( "WARN: file.mk:2: VAR is defined but not used.") diff --git a/pkgtools/pkglint/files/vardefs.go b/pkgtools/pkglint/files/vardefs.go index 62315a425c9..8de0b307716 100644 --- a/pkgtools/pkglint/files/vardefs.go +++ b/pkgtools/pkglint/files/vardefs.go @@ -1282,13 +1282,13 @@ func (reg *VarTypeRegistry) Init(src *Pkgsrc) { reg.pkglist("LTCONFIG_OVERRIDE", BtPathPattern) // See devel/bmake/files/main.c:/Var_Set."MACHINE_ARCH"/. - reg.sysload("MACHINE_ARCH", enumMachineArch, AlwaysInScope|DefinedIfInScope|NonemptyIfDefined) + reg.sysload("MACHINE_ARCH", BtMachineArch, AlwaysInScope|DefinedIfInScope|NonemptyIfDefined) // From mk/endian.mk, determined by a shell program that compiles // a C program. That's just too much for pkglint to analyze. reg.sysload("MACHINE_ENDIAN", enum("big little unknown"), DefinedIfInScope|NonemptyIfDefined) - reg.sysload("MACHINE_GNU_ARCH", enumMachineGnuArch, DefinedIfInScope|NonemptyIfDefined) + reg.sysload("MACHINE_GNU_ARCH", BtMachineGnuArch, DefinedIfInScope|NonemptyIfDefined) reg.sysload("MACHINE_GNU_PLATFORM", BtMachineGnuPlatform, DefinedIfInScope|NonemptyIfDefined) reg.sysload("MACHINE_PLATFORM", BtMachinePlatform, DefinedIfInScope|NonemptyIfDefined) reg.pkg("MAINTAINER", BtMailAddress) diff --git a/pkgtools/pkglint/files/vargroups.go b/pkgtools/pkglint/files/vargroups.go index f39a48b1a83..d385df6764f 100644 --- a/pkgtools/pkglint/files/vargroups.go +++ b/pkgtools/pkglint/files/vargroups.go @@ -39,7 +39,7 @@ func NewVargroupsChecker(mklines *MkLines) *VargroupsChecker { func (ck *VargroupsChecker) init() { mklines := ck.mklines - scope := mklines.vars + scope := mklines.allVars if !scope.IsDefined("_VARGROUPS") { ck.skip = true return diff --git a/pkgtools/pkglint/files/vartype.go b/pkgtools/pkglint/files/vartype.go index 2797e2058b5..991e9776504 100644 --- a/pkgtools/pkglint/files/vartype.go +++ b/pkgtools/pkglint/files/vartype.go @@ -2,6 +2,7 @@ package pkglint import ( "path" + "sort" "strings" ) @@ -45,7 +46,7 @@ const ( // This variable is provided by either the pkgsrc infrastructure in // mk/*, or by <sys.mk>, which is included at the very beginning. // - // FIXME: Clearly distinguish between: + // TODO: Clearly distinguish between: // * sys.mk // * bsd.prefs.mk // * bsd.pkg.mk @@ -199,7 +200,7 @@ func (vt *Vartype) NeedsRationale() bool { return vt.options&NeedsRationa func (vt *Vartype) IsOnePerLine() bool { return vt.options&OnePerLine != 0 } func (vt *Vartype) IsAlwaysInScope() bool { return vt.options&AlwaysInScope != 0 } func (vt *Vartype) IsDefinedIfInScope() bool { return vt.options&DefinedIfInScope != 0 } -func (vt *Vartype) IsNonemptyIfInScope() bool { return vt.options&NonemptyIfDefined != 0 } +func (vt *Vartype) IsNonemptyIfDefined() bool { return vt.options&NonemptyIfDefined != 0 } func (vt *Vartype) EffectivePermissions(basename string) ACLPermissions { for _, aclEntry := range vt.aclEntries { @@ -464,6 +465,13 @@ var ( BtYesNo = &BasicType{"YesNo", (*VartypeCheck).YesNo} BtYesNoIndirectly = &BasicType{"YesNoIndirectly", (*VartypeCheck).YesNoIndirectly} + BtMachineOpsys = enumFromValues(machineOpsysValues) + BtMachineArch = enumFromValues(machineArchValues) + BtMachineGnuArch = enumFromValues(machineGnuArchValues) + BtEmulOpsys = enumFromValues(emulOpsysValues) + BtEmulArch = enumFromValues(machineArchValues) // Just a wild guess. + BtMachineGnuPlatformOpsys = BtEmulOpsys + btCond = &BasicType{".if condition", nil /* never called */} btForLoop = &BasicType{".for loop", nil /* never called */} ) @@ -477,3 +485,51 @@ func init() { BtShellCommands.checker = (*VartypeCheck).ShellCommands BtShellWord.checker = (*VartypeCheck).ShellWord } + +// TODO: Move these values to VarTypeRegistry.Init and read them from the +// pkgsrc infrastructure files, as far as possible. +const ( + machineOpsysValues = "" + // See mk/platform + "AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD " + + "HPUX Haiku IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare" + + // See mk/emulator/emulator-vars.mk. + emulOpsysValues = "" + + "bitrig bsdos cygwin darwin dragonfly freebsd " + + "haiku hpux interix irix linux mirbsd netbsd openbsd osf1 solaris sunos" + + // Hardware architectures having the same name in bsd.own.mk and the GNU world. + // These are best-effort guesses, since they depend on the operating system. + archValues = "" + + "aarch64 alpha amd64 arc arm cobalt convex dreamcast i386 " + + "hpcmips hpcsh hppa hppa64 ia64 " + + "m68k m88k mips mips64 mips64el mipseb mipsel mipsn32 mlrisc " + + "ns32k pc532 pmax powerpc powerpc64 rs6000 s390 sparc sparc64 vax x86_64" + + // See mk/bsd.prefs.mk:/^GNU_ARCH\./ + machineArchValues = "" + + archValues + " " + + "aarch64eb amd64 arm26 arm32 coldfire earm earmeb earmhf earmhfeb earmv4 earmv4eb earmv5 " + + "earmv5eb earmv6 earmv6eb earmv6hf earmv6hfeb earmv7 earmv7eb earmv7hf earmv7hfeb evbarm " + + "i386 i586 i686 m68000 mips mips64eb sh3eb sh3el" + + // See mk/bsd.prefs.mk:/^GNU_ARCH\./ + machineGnuArchValues = "" + + archValues + " " + + "aarch64_be arm armeb armv4 armv4eb armv6 armv6eb armv7 armv7eb " + + "i486 m5407 m68010 mips64 mipsel sh shle x86_64" +) + +func enumFromValues(spaceSeparated string) *BasicType { + values := strings.Fields(spaceSeparated) + sort.Strings(values) + seen := make(map[string]bool) + var unique []string + for _, value := range values { + if !seen[value] { + seen[value] = true + unique = append(unique, value) + } + } + return enum(strings.Join(unique, " ")) +} diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go index 7cbfc45a342..16d92e57376 100644 --- a/pkgtools/pkglint/files/vartypecheck.go +++ b/pkgtools/pkglint/files/vartypecheck.go @@ -4,7 +4,6 @@ import ( "netbsd.org/pkglint/regex" "netbsd.org/pkglint/textproc" "path" - "sort" "strings" ) @@ -81,7 +80,7 @@ func (cv *VartypeCheck) WithVarnameValue(varname, value string) *VartypeCheck { // and the value. // // This is typically used when checking parts of composite types, -// especially patterns. +// such as the patterns from ONLY_FOR_PLATFORM. func (cv *VartypeCheck) WithVarnameValueMatch(varname, value string) *VartypeCheck { newVc := *cv newVc.Varname = varname @@ -91,61 +90,6 @@ func (cv *VartypeCheck) WithVarnameValueMatch(varname, value string) *VartypeChe return &newVc } -const ( - machineOpsysValues = "" + // See mk/platform - "AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD " + - "HPUX Haiku IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare" - - // See mk/emulator/emulator-vars.mk. - emulOpsysValues = "" + - "bitrig bsdos cygwin darwin dragonfly freebsd " + - "haiku hpux interix irix linux mirbsd netbsd openbsd osf1 solaris sunos" - - // Hardware architectures having the same name in bsd.own.mk and the GNU world. - // These are best-effort guesses, since they depend on the operating system. - archValues = "" + - "aarch64 alpha amd64 arc arm cobalt convex dreamcast i386 " + - "hpcmips hpcsh hppa hppa64 ia64 " + - "m68k m88k mips mips64 mips64el mipseb mipsel mipsn32 mlrisc " + - "ns32k pc532 pmax powerpc powerpc64 rs6000 s390 sparc sparc64 vax x86_64" - - // See mk/bsd.prefs.mk:/^GNU_ARCH\./ - machineArchValues = "" + - archValues + " " + - "aarch64eb amd64 arm26 arm32 coldfire earm earmeb earmhf earmhfeb earmv4 earmv4eb earmv5 " + - "earmv5eb earmv6 earmv6eb earmv6hf earmv6hfeb earmv7 earmv7eb earmv7hf earmv7hfeb evbarm " + - "i386 i586 i686 m68000 mips mips64eb sh3eb sh3el" - - // See mk/bsd.prefs.mk:/^GNU_ARCH\./ - machineGnuArchValues = "" + - archValues + " " + - "aarch64_be arm armeb armv4 armv4eb armv6 armv6eb armv7 armv7eb " + - "i486 m5407 m68010 mips64 mipsel sh shle x86_64" -) - -func enumFromValues(spaceSeparated string) *BasicType { - values := strings.Fields(spaceSeparated) - sort.Strings(values) - seen := make(map[string]bool) - var unique []string - for _, value := range values { - if !seen[value] { - seen[value] = true - unique = append(unique, value) - } - } - return enum(strings.Join(unique, " ")) -} - -var ( - enumMachineOpsys = enumFromValues(machineOpsysValues) - enumMachineArch = enumFromValues(machineArchValues) - enumMachineGnuArch = enumFromValues(machineGnuArchValues) - enumEmulOpsys = enumFromValues(emulOpsysValues) - enumEmulArch = enumFromValues(machineArchValues) // Just a wild guess. - enumMachineGnuPlatformOpsys = enumEmulOpsys -) - func (cv *VartypeCheck) AwkCommand() { if trace.Tracing { trace.Step1("Unchecked AWK command: %q", cv.Value) @@ -497,10 +441,10 @@ func (cv *VartypeCheck) EmulPlatform() { const rePair = `^(` + rePart + `)-(` + rePart + `)$` if m, opsysPattern, archPattern := match2(cv.Value, rePair); m { opsysCv := cv.WithVarnameValue("the operating system part of "+cv.Varname, opsysPattern) - enumEmulOpsys.checker(opsysCv) + BtEmulOpsys.checker(opsysCv) archCv := cv.WithVarnameValue("the hardware architecture part of "+cv.Varname, archPattern) - enumEmulArch.checker(archCv) + BtEmulArch.checker(archCv) } else { cv.Warnf("%q is not a valid emulation platform.", cv.Value) cv.Explain( @@ -806,14 +750,14 @@ func (cv *VartypeCheck) MachineGnuPlatform() { archCv := cv.WithVarnameValueMatch( "the hardware architecture part of "+cv.Varname, archPattern) - enumMachineGnuArch.checker(archCv) + BtMachineGnuArch.checker(archCv) _ = vendorPattern opsysCv := cv.WithVarnameValueMatch( "the operating system part of "+cv.Varname, opsysPattern) - enumMachineGnuPlatformOpsys.checker(opsysCv) + BtMachineGnuPlatformOpsys.checker(opsysCv) } else { cv.Warnf("%q is not a valid platform pattern.", cv.Value) @@ -848,13 +792,13 @@ func (cv *VartypeCheck) MachinePlatformPattern() { if m, opsysPattern, versionPattern, archPattern := match3(pattern, reTriple); m { opsysCv := cv.WithVarnameValueMatch("the operating system part of "+cv.Varname, opsysPattern) - enumMachineOpsys.checker(opsysCv) + BtMachineOpsys.checker(opsysCv) versionCv := cv.WithVarnameValueMatch("the version part of "+cv.Varname, versionPattern) versionCv.Version() archCv := cv.WithVarnameValueMatch("the hardware architecture part of "+cv.Varname, archPattern) - enumMachineArch.checker(archCv) + BtMachineArch.checker(archCv) } else { cv.Warnf("%q is not a valid platform pattern.", cv.Value) @@ -1099,7 +1043,7 @@ func (cv *VartypeCheck) Pkgpath() { func (cv *VartypeCheck) Pkgrevision() { if !matches(cv.Value, `^[1-9]\d*$`) { - cv.Warnf("%s must be a positive integer number.", cv.Varname) + cv.Errorf("%s must be a positive integer number.", cv.Varname) } if cv.MkLine.Basename != "Makefile" { cv.Errorf("%s only makes sense directly in the package Makefile.", cv.Varname) @@ -1253,7 +1197,7 @@ func (cv *VartypeCheck) SedCommands() { i++ ncommands++ if ncommands > 1 { - cv.Notef("Each sed command should appear in an assignment of its own.") + cv.Warnf("Each sed command should appear in an assignment of its own.") cv.Explain( "For example, instead of", " SUBST_SED.foo+= -e s,command1,, -e s,command2,,", diff --git a/pkgtools/pkglint/files/vartypecheck_test.go b/pkgtools/pkglint/files/vartypecheck_test.go index 2527e3863bd..20d93b1649a 100644 --- a/pkgtools/pkglint/files/vartypecheck_test.go +++ b/pkgtools/pkglint/files/vartypecheck_test.go @@ -4,6 +4,126 @@ import ( "gopkg.in/check.v1" ) +func (s *Suite) Test_VartypeCheck_Errorf(c *check.C) { + t := s.Init(c) + + mkline := t.NewMkLine("filename.mk", 123, "") + cv := VartypeCheck{MkLine: mkline} + + cv.Errorf("Error %q.", "message") + + t.CheckOutputLines( + "ERROR: filename.mk:123: Error \"message\".") +} + +func (s *Suite) Test_VartypeCheck_Warnf(c *check.C) { + t := s.Init(c) + + mkline := t.NewMkLine("filename.mk", 123, "") + cv := VartypeCheck{MkLine: mkline} + + cv.Warnf("Warning %q.", "message") + + t.CheckOutputLines( + "WARN: filename.mk:123: Warning \"message\".") +} + +func (s *Suite) Test_VartypeCheck_Notef(c *check.C) { + t := s.Init(c) + + mkline := t.NewMkLine("filename.mk", 123, "") + cv := VartypeCheck{MkLine: mkline} + + cv.Notef("Note %q.", "message") + + t.CheckOutputLines( + "NOTE: filename.mk:123: Note \"message\".") +} + +func (s *Suite) Test_VartypeCheck_Explain(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--explain") + mkline := t.NewMkLine("filename.mk", 123, "") + cv := VartypeCheck{MkLine: mkline} + + cv.Notef("Note %q.", "message") + cv.Explain("Explanation.") + + t.CheckOutputLines( + "NOTE: filename.mk:123: Note \"message\".", + "", + "\tExplanation.", + "") +} + +func (s *Suite) Test_VartypeCheck_Autofix(c *check.C) { + t := s.Init(c) + + mkline := t.NewMkLine("filename.mk", 123, "") + cv := VartypeCheck{MkLine: mkline} + + t.CheckEquals(cv.Autofix(), mkline.Autofix()) +} + +func (s *Suite) Test_VartypeCheck_WithValue(c *check.C) { + t := s.Init(c) + + cv := VartypeCheck{ + Varname: "OLD", + Value: "oldValue${VAR}", + ValueNoVar: "oldValue", + } + + copied := cv.WithValue("newValue${NEW_VAR}") + + t.CheckEquals(copied.Varname, "OLD") + t.CheckEquals(copied.Value, "newValue${NEW_VAR}") + t.CheckEquals(copied.ValueNoVar, "newValue") + t.CheckEquals(cv.Value, "oldValue${VAR}") + t.CheckEquals(cv.ValueNoVar, "oldValue") +} + +func (s *Suite) Test_VartypeCheck_WithVarnameValue(c *check.C) { + t := s.Init(c) + + cv := VartypeCheck{ + Varname: "OLD", + Value: "oldValue${VAR}", + ValueNoVar: "oldValue", + } + + copied := cv.WithVarnameValue("NEW", "newValue${NEW_VAR}") + + t.CheckEquals(copied.Varname, "NEW") + t.CheckEquals(copied.Value, "newValue${NEW_VAR}") + t.CheckEquals(copied.ValueNoVar, "newValue") + t.CheckEquals(cv.Value, "oldValue${VAR}") + t.CheckEquals(cv.ValueNoVar, "oldValue") +} + +func (s *Suite) Test_VartypeCheck_WithVarnameValueMatch(c *check.C) { + t := s.Init(c) + + cv := VartypeCheck{ + Varname: "OLD", + Op: opAssign, + Value: "oldValue${VAR}", + ValueNoVar: "oldValue", + } + + copied := cv.WithVarnameValueMatch("NEW", "newValue${NEW_VAR}") + + t.CheckEquals(copied.Varname, "NEW") + t.CheckEquals(copied.Op, opUseMatch) + t.CheckEquals(copied.Value, "newValue${NEW_VAR}") + t.CheckEquals(copied.ValueNoVar, "newValue") + t.CheckEquals(cv.Varname, "OLD") + t.CheckEquals(cv.Op, opAssign) + t.CheckEquals(cv.Value, "oldValue${VAR}") + t.CheckEquals(cv.ValueNoVar, "oldValue") +} + func (s *Suite) Test_VartypeCheck_AwkCommand(c *check.C) { t := s.Init(c) vt := NewVartypeCheckTester(t, BtAwkCommand) @@ -516,8 +636,8 @@ func (s *Suite) Test_VartypeCheck_Enum__use_match(c *check.C) { mklines.Check() t.CheckOutputLines( - "NOTE: module.mk:5: MACHINE_ARCH "+ - "should be compared using \"${MACHINE_ARCH} == i386\" "+ + "NOTE: module.mk:5: MACHINE_ARCH can be "+ + "compared using the simpler \"${MACHINE_ARCH} == i386\" "+ "instead of matching against \":Mi386\".", "", "\tThis variable has a single value, not a list of values. Therefore it", @@ -626,15 +746,17 @@ func (s *Suite) Test_VartypeCheck_FetchURL(c *check.C) { vt.Values( "https://example.org/pub", - "https://example.org/$@", // Doesn't make sense at all. + "https://example.org/$@", "https://example.org/?f=", "https://example.org/download:", - "https://example.org/download?") + "https://example.org/download?", + "https://example.org/$$") vt.Output( "WARN: filename.mk:71: The fetch URL \"https://example.org/pub\" should end with a slash.", - "WARN: filename.mk:72: \"https://example.org/$@\" is not a valid URL.", - "WARN: filename.mk:75: The fetch URL \"https://example.org/download?\" should end with a slash.") + "WARN: filename.mk:75: The fetch URL \"https://example.org/download?\" should end with a slash.", + "WARN: filename.mk:76: \"https://example.org/$$\" is not a valid URL.", + "WARN: filename.mk:76: The fetch URL \"https://example.org/$$\" should end with a slash.") // The transport protocol doesn't matter for matching the MASTER_SITEs. // See url2pkg.py, function adjust_site_from_sites_mk. @@ -659,6 +781,15 @@ func (s *Suite) Test_VartypeCheck_FetchURL(c *check.C) { "instead of \"-ftp://ftp.gnu.org/pub/gnu/bash/bash-5.0.tar.gz\".", "WARN: filename.mk:86: Please use ${MASTER_SITE_GNU:S,^,-,:=bash/bash-5.0.tar.gz} "+ "instead of \"-https://ftp.gnu.org/pub/gnu/bash/bash-5.0.tar.gz\".") + + // The ${.TARGET} variable doesn't make sense at all in a URL. + // Other variables might, and there could be checks for them. + // As of December 2019 these are skipped completely, + // see containsVarRef in VartypeCheck.URL. + vt.Values( + "https://example.org/$@") + + vt.OutputEmpty() } func (s *Suite) Test_VartypeCheck_FetchURL__without_package(c *check.C) { @@ -992,6 +1123,60 @@ func (s *Suite) Test_VartypeCheck_MachineGnuPlatform(c *check.C) { "WARN: filename.mk:6: \"x86_64-pc\" is not a valid platform pattern.") } +func (s *Suite) Test_VartypeCheck_MachinePlatform(c *check.C) { + vt := NewVartypeCheckTester(s.Init(c), BtMachinePlatform) + + // There is no need to test the assignment operators since the + // only variable of this type is read-only. + + vt.Varname("MACHINE_PLATFORM") + vt.Op(opUseMatch) + vt.Values( + "linux-i386", + "nextbsd-5.0-8087", + "netbsd-7.0-l*", + "NetBSD-1.6.2-i386", + "FreeBSD*", + "FreeBSD-*", + "${LINUX}", + "NetBSD-[0-1]*-*") + + vt.Output( + "WARN: filename.mk:1: \"linux-i386\" is not a valid platform pattern.", + "WARN: filename.mk:2: The pattern \"nextbsd\" cannot match any of "+ + "{ AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD HPUX Haiku "+ + "IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare "+ + "} for the operating system part of MACHINE_PLATFORM.", + "WARN: filename.mk:2: The pattern \"8087\" 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 MACHINE_PLATFORM.", + "WARN: filename.mk:3: The pattern \"netbsd\" cannot match any of "+ + "{ AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD HPUX Haiku "+ + "IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare "+ + "} for the operating system part of MACHINE_PLATFORM.", + "WARN: filename.mk:3: The pattern \"l*\" 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 MACHINE_PLATFORM.", + "WARN: filename.mk:5: \"FreeBSD*\" is not a valid platform pattern.", + "WARN: filename.mk:8: Please use \"[0-1].*\" instead of \"[0-1]*\" as the version pattern.") +} + func (s *Suite) Test_VartypeCheck_MachinePlatformPattern(c *check.C) { vt := NewVartypeCheckTester(s.Init(c), BtMachinePlatformPattern) @@ -1271,7 +1456,7 @@ func (s *Suite) Test_VartypeCheck_Pkgrevision(c *check.C) { "3a") vt.Output( - "WARN: filename.mk:1: PKGREVISION must be a positive integer number.", + "ERROR: filename.mk:1: PKGREVISION must be a positive integer number.", "ERROR: filename.mk:1: PKGREVISION only makes sense directly in the package Makefile.") vt.File("Makefile") @@ -1354,6 +1539,31 @@ func (s *Suite) Test_VartypeCheck_RPkgVer(c *check.C) { vt.OutputEmpty() } +func (s *Suite) Test_VartypeCheck_RelativePkgDir(c *check.C) { + t := s.Init(c) + vt := NewVartypeCheckTester(t, BtRelativePkgDir) + + t.CreateFileLines("category/other-package/Makefile") + t.Chdir("category/package") + + vt.Varname("PKGDIR") + vt.Values( + "category/other-package", + "../../category/other-package", + "${OTHER_VAR}", + "invalid", + "../../invalid/relative", + "/absolute") + + vt.Output( + "ERROR: filename.mk:1: Relative path \"category/other-package/Makefile\" does not exist.", + "WARN: filename.mk:1: \"category/other-package\" is not a valid relative package directory.", + "ERROR: filename.mk:4: Relative path \"invalid/Makefile\" does not exist.", + "WARN: filename.mk:4: \"invalid\" is not a valid relative package directory.", + "ERROR: filename.mk:5: Relative path \"../../invalid/relative/Makefile\" does not exist.", + "ERROR: filename.mk:6: The path \"/absolute\" must be relative.") +} + func (s *Suite) Test_VartypeCheck_RelativePkgPath(c *check.C) { t := s.Init(c) vt := NewVartypeCheckTester(t, BtRelativePkgPath) @@ -1367,12 +1577,14 @@ func (s *Suite) Test_VartypeCheck_RelativePkgPath(c *check.C) { "../../category/other-package", "${OTHER_VAR}", "invalid", - "../../invalid/relative") + "../../invalid/relative", + "/absolute") vt.Output( "ERROR: filename.mk:1: Relative path \"category/other-package\" does not exist.", "ERROR: filename.mk:4: Relative path \"invalid\" does not exist.", - "ERROR: filename.mk:5: Relative path \"../../invalid/relative\" does not exist.") + "ERROR: filename.mk:5: Relative path \"../../invalid/relative\" does not exist.", + "ERROR: filename.mk:6: The path \"/absolute\" must be relative.") } func (s *Suite) Test_VartypeCheck_Restricted(c *check.C) { @@ -1405,7 +1617,7 @@ func (s *Suite) Test_VartypeCheck_SedCommands(c *check.C) { vt.Output( "NOTE: filename.mk:1: Please always use \"-e\" in sed commands, even if there is only one substitution.", - "NOTE: filename.mk:2: Each sed command should appear in an assignment of its own.", + "WARN: filename.mk:2: Each sed command should appear in an assignment of its own.", "WARN: filename.mk:3: The # character starts a Makefile comment.", "ERROR: filename.mk:3: Invalid shell words \"\\\"s,\" in sed commands.", "WARN: filename.mk:8: Unknown sed command \"1d\".", @@ -1590,6 +1802,23 @@ func (s *Suite) Test_VartypeCheck_Tool(c *check.C) { vt.OutputEmpty() } +func (s *Suite) Test_VartypeCheck_Unknown(c *check.C) { + t := s.Init(c) + vt := NewVartypeCheckTester(t, BtUnknown) + + vt.Varname("BDB185_DEFAULT") + vt.Values( + "# empty", + "Something", + "'quotes are ok'", + "!\"#$%&/()*+,-./0-9:;<=>?@A-Z[\\]^_a-z{|}~") + + // This warning is produced as a side effect of parsing the lines. + // It is not specific to the BtUnknown type. + vt.Output( + "WARN: filename.mk:4: The # character starts a Makefile comment.") +} + func (s *Suite) Test_VartypeCheck_URL(c *check.C) { t := s.Init(c) vt := NewVartypeCheckTester(t, BtURL) @@ -1749,6 +1978,30 @@ func (s *Suite) Test_VartypeCheck_WrapperTransform(c *check.C) { "WARN: filename.mk:8: Unknown wrapper transform command \"unknown\".") } +func (s *Suite) Test_VartypeCheck_WrkdirSubdirectory(c *check.C) { + vt := NewVartypeCheckTester(s.Init(c), BtWrkdirSubdirectory) + + vt.Varname("WRKSRC") + vt.Op(opAssign) + vt.Values( + "${WRKDIR}", + "${WRKDIR}/", + "${WRKDIR}/.", + "${WRKDIR}/subdir", + ".", + "${DISTNAME}", + "${PKGNAME_NOREV}", + "two words", + "../other", + "${WRKSRC}", // Recursive definition. + "${PKGDIR}/files") + + // XXX: Many more consistency checks are possible here. + vt.Output( + "WARN: filename.mk:8: The pathname \"two words\" " + + "contains the invalid character \" \".") +} + func (s *Suite) Test_VartypeCheck_WrksrcSubdirectory(c *check.C) { vt := NewVartypeCheckTester(s.Init(c), BtWrksrcSubdirectory) |