diff options
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) |