diff options
Diffstat (limited to 'pkgtools')
21 files changed, 685 insertions, 352 deletions
diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index e3ff5dd27fb..34e158dd2c4 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.615 2019/12/08 22:03:37 rillig Exp $ +# $NetBSD: Makefile,v 1.616 2019/12/09 20:38:15 rillig Exp $ -PKGNAME= pkglint-19.3.15 +PKGNAME= pkglint-19.3.16 CATEGORIES= pkgtools DISTNAME= tools MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/} diff --git a/pkgtools/pkglint/files/logging.go b/pkgtools/pkglint/files/logging.go index 43fc43e64f1..698e4dcedb3 100644 --- a/pkgtools/pkglint/files/logging.go +++ b/pkgtools/pkglint/files/logging.go @@ -257,10 +257,14 @@ func (l *Logger) Logf(level *LogLevel, filename CurrPath, lineno, format, msg st if G.Testing { if level != Error { - assertf(!contains(format, "must"), "Must must only appear in errors: %s", format) + assertf( + !contains(format, "must"), + "The word \"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) + assertf( + !contains(format, "should"), + "The word \"should\" must only appear in warnings: %s", format) } } diff --git a/pkgtools/pkglint/files/logging_test.go b/pkgtools/pkglint/files/logging_test.go index 72022cc5167..a6b5cb85b07 100644 --- a/pkgtools/pkglint/files/logging_test.go +++ b/pkgtools/pkglint/files/logging_test.go @@ -805,6 +805,23 @@ func (s *Suite) Test_Logger_Logf__panic(c *check.C) { "Pkglint internal error: Diagnostic format \"No period\" must end in a period.") } +func (s *Suite) Test_Logger_Logf__wording(c *check.C) { + t := s.Init(c) + + t.ExpectPanic( + func() { G.Logger.Logf(Error, "filename", "13", "This should.", "This should.") }, + "Pkglint internal error: The word \"should\" must only appear in warnings: This should.") + + t.ExpectPanic( + func() { G.Logger.Logf(Warn, "filename", "13", "This must.", "This must.") }, + "Pkglint internal error: The word \"must\" must only appear in errors: This must.") + + G.Logger.Logf(Note, "filename", "13", "This should.", "This should.") + + t.CheckOutputLines( + "NOTE: filename:13: This should.") +} + func (s *Suite) Test_Logger_TechErrorf__gcc_format(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/mkline.go b/pkgtools/pkglint/files/mkline.go index 8d476611d69..7490bd8dfbe 100644 --- a/pkgtools/pkglint/files/mkline.go +++ b/pkgtools/pkglint/files/mkline.go @@ -1277,29 +1277,46 @@ var ( ) func MatchMkInclude(text string) (m bool, indentation, directive string, filename RelPath) { - lexer := textproc.NewLexer(text) - if lexer.SkipByte('.') { - indentation = lexer.NextHspace() - directive = lexer.NextString("include") - if directive == "" { - directive = lexer.NextString("sinclude") - } - if directive != "" { - lexer.NextHspace() - if lexer.SkipByte('"') { - // Note: strictly speaking, the full MkVarUse would have to be parsed - // here. But since these usually don't contain double quotes, it has - // worked fine up to now. - filename = NewRelPathString(lexer.NextBytesFunc(func(c byte) bool { return c != '"' })) - if !filename.IsEmpty() && lexer.SkipByte('"') { - lexer.NextHspace() - if lexer.EOF() { - m = true - return - } - } - } - } + tokens, rest := NewMkLexer(text, nil).MkTokens() + if rest != "" { + return false, "", "", "" + } + + lexer := NewMkTokensLexer(tokens) + if !lexer.SkipByte('.') { + return false, "", "", "" + } + + indentation = lexer.NextHspace() + + directive = lexer.NextString("include") + if directive == "" { + directive = lexer.NextString("sinclude") + } + if directive == "" { + return false, "", "", "" } - return false, "", "", "" + + lexer.SkipHspace() + if !lexer.SkipByte('"') { + return false, "", "", "" + } + + mark := lexer.Mark() + for lexer.NextBytesFunc(func(c byte) bool { return c != '"' && c != '$' }) != "" || + lexer.NextVarUse() != nil { + } + enclosed := NewPath(lexer.Since(mark)) + + if enclosed.IsEmpty() || enclosed.IsAbs() || !lexer.SkipByte('"') { + return false, "", "", "" + } + lexer.SkipHspace() + if !lexer.EOF() { + return false, "", "", "" + } + + filename = NewRelPath(enclosed) + m = true + return } diff --git a/pkgtools/pkglint/files/mkline_test.go b/pkgtools/pkglint/files/mkline_test.go index 470b65160c8..0c4cd50a919 100644 --- a/pkgtools/pkglint/files/mkline_test.go +++ b/pkgtools/pkglint/files/mkline_test.go @@ -1547,13 +1547,6 @@ func (s *Suite) Test_MatchMkInclude(c *check.C) { } } - testFail("") - testFail(".") - testFail(".include") - testFail(".include \"") - testFail(".include \"other.mk") - testFail(".include \"other.mk\" \"") - test(".include \"other.mk\"", "", "include", "other.mk", "") @@ -1566,5 +1559,25 @@ func (s *Suite) Test_MatchMkInclude(c *check.C) { test(".include \"other.mk\"\t# comment", "", "include", "other.mk", "# comment") - t.CheckOutputEmpty() + test(". include \"${DIR}/file.mk\"", + " ", "include", "${DIR}/file.mk", "") + + test(". include \"${DIR:S,\",',g}/file.mk\"", + " ", "include", "${DIR:S,\",',g}/file.mk", "") + + test(".include \"${}\"", + "", "include", "${}", "") + + testFail("") + testFail(".") + testFail(".include") + testFail(".include \"") + testFail(".include \"\"") + testFail(".include \"$\"") + testFail(".include \"$$\"") + testFail(".include \"other.mk") + testFail(".include \"other.mk\" \"") + testFail(".include \"/absolute\"") + testFail(".include \"/absolute\"rest") + testFail(".include \"/absolute\" rest") } diff --git a/pkgtools/pkglint/files/mklineparser.go b/pkgtools/pkglint/files/mklineparser.go index 12c07c20c4a..3a449de9e11 100644 --- a/pkgtools/pkglint/files/mklineparser.go +++ b/pkgtools/pkglint/files/mklineparser.go @@ -72,33 +72,8 @@ func (p MkLineParser) parseVarassign(line *Line, text string, splitResult mkLine return nil } - if a.spaceAfterVarname != "" { - varname := a.varname - op := a.op - switch { - case hasSuffix(varname, "+") && (op == opAssign || op == opAssignAppend): - break - case matches(varname, `^[a-z]`) && op == opAssignEval: - break - default: - fix := line.Autofix() - fix.Notef("Unnecessary space after variable name %q.", varname) - fix.Replace(varname+a.spaceAfterVarname+op.String(), varname+op.String()) - fix.Apply() - // FIXME: Preserve the alignment of the variable value. - } - } - - if splitResult.hasComment && a.value != "" && splitResult.spaceBeforeComment == "" { - line.Warnf("The # character starts a Makefile comment.") - line.Explain( - "In a variable assignment, an unescaped # starts a comment that", - "continues until the end of the line.", - "To escape the #, write \\#.", - "", - "If this # character intentionally starts a comment,", - "it should be preceded by a space in order to make it more visible.") - } + p.fixSpaceAfterVarname(line, a) + p.checkUnintendedComment(&splitResult, a, line) return &MkLine{line, splitResult, a} } @@ -181,6 +156,45 @@ func (p MkLineParser) MatchVarassign(line *Line, text string, splitResult *mkLin } } +func (p MkLineParser) fixSpaceAfterVarname(line *Line, a *mkLineAssign) { + if !(a.spaceAfterVarname != "") { + return + } + + varname := a.varname + op := a.op + switch { + case hasSuffix(varname, "+") && (op == opAssign || op == opAssignAppend): + break + case matches(varname, `^[a-z]`) && op == opAssignEval: + break + + default: + before := a.valueAlign + after := alignWith(varname+op.String(), before) + + fix := line.Autofix() + fix.Notef("Unnecessary space after variable name %q.", varname) + fix.Replace(before, after) + fix.Apply() + } +} + +func (p MkLineParser) checkUnintendedComment(splitResult *mkLineSplitResult, a *mkLineAssign, line *Line) { + if !(splitResult.hasComment && a.value != "" && splitResult.spaceBeforeComment == "") { + return + } + + line.Warnf("The # character starts a Makefile comment.") + line.Explain( + "In a variable assignment, an unescaped # starts a comment that", + "continues until the end of the line.", + "To escape the #, write \\#.", + "", + "If this # character intentionally starts a comment,", + "it should be preceded by a space in order to make it more visible.") +} + func (p MkLineParser) parseShellcmd(line *Line, splitResult mkLineSplitResult) *MkLine { return &MkLine{line, splitResult, mkLineShell{line.Text[1:]}} } diff --git a/pkgtools/pkglint/files/mklineparser_test.go b/pkgtools/pkglint/files/mklineparser_test.go index cac89e8b511..4496bc19f35 100644 --- a/pkgtools/pkglint/files/mklineparser_test.go +++ b/pkgtools/pkglint/files/mklineparser_test.go @@ -146,58 +146,6 @@ func (s *Suite) Test_MkLineParser_parseVarassign__leading_space(c *check.C) { "WARN: rubyversion.mk:427: Makefile lines should not start with space characters.") } -func (s *Suite) Test_MkLineParser_parseVarassign__space_around_operator(c *check.C) { - t := s.Init(c) - - t.SetUpCommandLine("--show-autofix", "--source") - t.NewMkLine("test.mk", 101, - "pkgbase = package") - - t.CheckOutputLines( - "NOTE: test.mk:101: Unnecessary space after variable name \"pkgbase\".", - "AUTOFIX: test.mk:101: Replacing \"pkgbase =\" with \"pkgbase=\".", - "-\tpkgbase = package", - "+\tpkgbase= package") -} - -func (s *Suite) Test_MkLineParser_parseVarassign__autofix_space_after_varname(c *check.C) { - t := s.Init(c) - - filename := t.CreateFileLines("Makefile", - MkCvsID, - "VARNAME +=\t${VARNAME}", - "VARNAME+ =\t${VARNAME+}", - "VARNAME+ +=\t${VARNAME+}", - "VARNAME+ ?=\t${VARNAME}", - "pkgbase := pkglint") - - CheckFileMk(filename) - - t.CheckOutputLines( - "NOTE: ~/Makefile:2: Unnecessary space after variable name \"VARNAME\".", - - // The assignment operators other than = and += cannot lead to ambiguities. - "NOTE: ~/Makefile:5: Unnecessary space after variable name \"VARNAME+\".", - - "WARN: ~/Makefile:5: "+ - "Please include \"../../mk/bsd.prefs.mk\" before using \"?=\".") - - t.SetUpCommandLine("-Wall", "--autofix") - - CheckFileMk(filename) - - t.CheckOutputLines( - "AUTOFIX: ~/Makefile:2: Replacing \"VARNAME +=\" with \"VARNAME+=\".", - "AUTOFIX: ~/Makefile:5: Replacing \"VARNAME+ ?=\" with \"VARNAME+?=\".") - t.CheckFileLines("Makefile", - MkCvsID+"", - "VARNAME+=\t${VARNAME}", - "VARNAME+ =\t${VARNAME+}", - "VARNAME+ +=\t${VARNAME+}", - "VARNAME+?=\t${VARNAME}", - "pkgbase := pkglint") -} - func (s *Suite) Test_MkLineParser_parseVarassign__append(c *check.C) { t := s.Init(c) @@ -458,6 +406,88 @@ func (s *Suite) Test_MkLineParser_MatchVarassign(c *check.C) { "") } +func (s *Suite) Test_MkLineParser_fixSpaceAfterVarname__show_autofix(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--show-autofix", "--source") + t.NewMkLine("test.mk", 101, + "pkgbase = package") + + t.CheckOutputLines( + "NOTE: test.mk:101: Unnecessary space after variable name \"pkgbase\".", + "AUTOFIX: test.mk:101: Replacing \"pkgbase = \" with \"pkgbase= \".", + "-\tpkgbase = package", + "+\tpkgbase= package") +} + +func (s *Suite) Test_MkLineParser_fixSpaceAfterVarname__autofix(c *check.C) { + t := s.Init(c) + + filename := t.CreateFileLines("Makefile", + MkCvsID, + "VARNAME +=\t${VARNAME}", + "VARNAME+ =\t${VARNAME+}", + "VARNAME+ +=\t${VARNAME+}", + "VARNAME+ ?=\t${VARNAME}", + "pkgbase := pkglint") + + CheckFileMk(filename) + + t.CheckOutputLines( + "NOTE: ~/Makefile:2: Unnecessary space after variable name \"VARNAME\".", + + // The assignment operators other than = and += cannot lead to ambiguities. + "NOTE: ~/Makefile:5: Unnecessary space after variable name \"VARNAME+\".", + + "WARN: ~/Makefile:5: "+ + "Please include \"../../mk/bsd.prefs.mk\" before using \"?=\".") + + t.SetUpCommandLine("-Wall", "--autofix") + + CheckFileMk(filename) + + t.CheckOutputLines( + "AUTOFIX: ~/Makefile:2: Replacing \"VARNAME +=\\t\" with \"VARNAME+=\\t\".", + "AUTOFIX: ~/Makefile:5: Replacing \"VARNAME+ ?=\\t\" with \"VARNAME+?=\\t\".") + t.CheckFileLines("Makefile", + MkCvsID+"", + "VARNAME+=\t${VARNAME}", + "VARNAME+ =\t${VARNAME+}", + "VARNAME+ +=\t${VARNAME+}", + "VARNAME+?=\t${VARNAME}", + "pkgbase := pkglint") +} + +func (s *Suite) Test_MkLineParser_fixSpaceAfterVarname__preserve_alignment(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--show-autofix") + + test := func(before, after string, diagnostics ...string) { + + doTest := func(autofix bool) { + mkline := t.NewMkLine("filename.mk", 123, before) + t.CheckEquals(mkline.Text, condStr(autofix, after, before)) + } + + t.ExpectDiagnosticsAutofix(doTest, diagnostics...) + } + + test( + "V += ${VARNAME}", + "V+=\t\t${VARNAME}", + + "NOTE: filename.mk:123: Unnecessary space after variable name \"V\".", + "AUTOFIX: filename.mk:123: Replacing \"V += \" with \"V+=\\t\\t\".") + + test( + "V += ${VARNAME}", + "V+=\t ${VARNAME}", + + "NOTE: filename.mk:123: Unnecessary space after variable name \"V\".", + "AUTOFIX: filename.mk:123: Replacing \"V += \" with \"V+=\\t \".") +} + func (s *Suite) Test_MkLineParser_parseShellcmd(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/mklines_test.go b/pkgtools/pkglint/files/mklines_test.go index cf5a1eb4aa4..cc4d6c47e9d 100644 --- a/pkgtools/pkglint/files/mklines_test.go +++ b/pkgtools/pkglint/files/mklines_test.go @@ -743,6 +743,28 @@ func (s *Suite) Test_MkLines_ForEach__conditional_variables(c *check.C) { t.CheckEquals(seenUsesGettext, true) } +func (s *Suite) Test_MkLines_ForEachEnd(c *check.C) { + t := s.Init(c) + + t.SetUpVartypes() + + mklines := t.NewMkLines("filename.mk", + MkCvsID) + + // Calls to MkLines.ForEach cannot nest since they modify fields + // in the MkLines type. As of December 2019 there is no separation + // between: + // - The MkLines as a collection of data + // - An iterator over the MkLines + // - The MkLinesChecker + t.ExpectAssert(func() { + mklines.ForEach(func(mkline *MkLine) { + mklines.ForEach(func(mkline *MkLine) { + }) + }) + }) +} + func (s *Suite) Test_MkLines_collectElse(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/mkvarusechecker.go b/pkgtools/pkglint/files/mkvarusechecker.go index 734a670b185..bb4f9700ed6 100644 --- a/pkgtools/pkglint/files/mkvarusechecker.go +++ b/pkgtools/pkglint/files/mkvarusechecker.go @@ -25,7 +25,7 @@ func (ck *MkVarUseChecker) Check(vuc *VarUseContext) { ck.checkUndefined() ck.checkPermissions(vuc) - ck.checkVarname() + ck.checkVarname(vuc.time) ck.checkModifiers() ck.checkQuoting(vuc) @@ -130,7 +130,7 @@ func (ck *MkVarUseChecker) checkModifiersRange() { fix.Apply() } -func (ck *MkVarUseChecker) checkVarname() { +func (ck *MkVarUseChecker) checkVarname(time VucTime) { varname := ck.use.varname if varname == "@" { ck.MkLine.Warnf("Please use %q instead of %q.", "${.TARGET}", "$@") @@ -139,7 +139,7 @@ func (ck *MkVarUseChecker) checkVarname() { "of the same name.") } - if varname == "LOCALBASE" && !G.Infrastructure { + if varname == "LOCALBASE" && !G.Infrastructure && time == VucRunTime { fix := ck.MkLine.Autofix() fix.Warnf("Please use PREFIX instead of LOCALBASE.") fix.ReplaceRegex(`\$\{LOCALBASE\b`, "${PREFIX", 1) @@ -380,12 +380,10 @@ func (ck *MkVarUseChecker) checkUseAtLoadTime(time VucTime) { return } - if ck.vartype.IsPackageSettable() && - basename != "Makefile" && basename != "options.mk" { - - // For package-settable variables, the explanation doesn't - // make sense since it talks about completely different - // types of variables. + if ck.vartype.IsPackageSettable() { + // For package-settable variables, the explanation below + // doesn't make sense since including bsd.prefs.mk won't + // define any package-settable variables. return } diff --git a/pkgtools/pkglint/files/mkvarusechecker_test.go b/pkgtools/pkglint/files/mkvarusechecker_test.go index be9d5dfe83a..6fee1c066a8 100644 --- a/pkgtools/pkglint/files/mkvarusechecker_test.go +++ b/pkgtools/pkglint/files/mkvarusechecker_test.go @@ -355,7 +355,24 @@ func (s *Suite) Test_MkVarUseChecker_checkModifiersRange(c *check.C) { } func (s *Suite) Test_MkVarUseChecker_checkVarname(c *check.C) { - // FIXME + t := s.Init(c) + + mklines := t.NewMkLines("filename.mk", + "\t: $@", + ".if ${LOCALBASE} == /usr/pkg", // Use at load time + "\t: ${LOCALBASE}", // Use at run time + ".endif") + + mklines.ForEach(func(mkline *MkLine) { + mkline.ForEachUsed(func(varUse *MkVarUse, time VucTime) { + ck := NewMkVarUseChecker(varUse, mklines, mkline) + ck.checkVarname(time) + }) + }) + + t.CheckOutputLines( + "WARN: filename.mk:1: Please use \"${.TARGET}\" instead of \"$@\".", + "WARN: filename.mk:3: Please use PREFIX instead of LOCALBASE.") } func (s *Suite) Test_MkVarUseChecker_checkPermissions(c *check.C) { @@ -430,33 +447,6 @@ func (s *Suite) Test_MkVarUseChecker_checkPermissions__explain(c *check.C) { "\ttech-pkg@NetBSD.org mailing list.", "") } -func (s *Suite) Test_MkVarUseChecker_checkPermissions__load_time(c *check.C) { - t := s.Init(c) - - t.SetUpPkgsrc() - t.Chdir("category/package") - t.FinishSetUp() - mklines := t.NewMkLines("options.mk", - MkCvsID, - "", - "# don't include bsd.prefs.mk here", - "", - "WRKSRC:=\t${.CURDIR}", - ".if ${PKG_SYSCONFDIR.gdm} != \"etc\"", - ".endif") - - mklines.Check() - - // The PKG_SYSCONFDIR.* depend on the directory layout that is - // specified in mk.conf, therefore bsd.prefs.mk must be included first. - // - // Evaluating .CURDIR at load time is definitely ok since it is defined - // internally by bmake to be AlwaysInScope. - t.CheckOutputLines( - "WARN: options.mk:6: To use PKG_SYSCONFDIR.gdm at load time, " + - ".include \"../../mk/bsd.prefs.mk\" first.") -} - func (s *Suite) Test_MkVarUseChecker_checkPermissions__load_time_in_condition(c *check.C) { t := s.Init(c) @@ -860,6 +850,52 @@ func (s *Suite) Test_MkVarUseChecker_checkUseAtLoadTime__other_mk(c *check.C) { ".include \"../../mk/bsd.prefs.mk\" first.") } +func (s *Suite) Test_MkVarUseChecker_checkUseAtLoadTime__PKG_SYSCONFDIR(c *check.C) { + t := s.Init(c) + + t.SetUpVartypes() + t.Chdir("category/package") + mklines := t.NewMkLines("options.mk", + MkCvsID, + ".if ${PKG_SYSCONFDIR.gdm} != \"etc\"", + ".endif") + + mklines.Check() + + // The PKG_SYSCONFDIR.* directories typically start with ${PREFIX}. + // Since PREFIX is not defined until bsd.pkg.mk, including bsd.prefs.mk + // wouldn't help, therefore pkglint doesn't suggest it. + t.CheckOutputEmpty() +} + +func (s *Suite) Test_MkVarUseChecker_checkUseAtLoadTime__package_settable(c *check.C) { + t := s.Init(c) + + btAnything := &BasicType{"Anything", func(cv *VartypeCheck) {}} + t.SetUpType("PKG", btAnything, PackageSettable) + t.Chdir("category/package") + + test := func(filename CurrPath, diagnostics ...string) { + mklines := t.NewMkLines(filename, + MkCvsID, + ".if ${PKG} != \"etc\"", + ".endif") + + mklines.Check() + + t.CheckOutput(diagnostics) + } + + test("Makefile", + nil...) + + test("options.mk", + nil...) + + test("other.mk", + nil...) +} + func (s *Suite) Test_MkVarUseChecker_warnToolLoadTime(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/package_test.go b/pkgtools/pkglint/files/package_test.go index f7582cc6e6d..3c49278afb2 100644 --- a/pkgtools/pkglint/files/package_test.go +++ b/pkgtools/pkglint/files/package_test.go @@ -1471,13 +1471,25 @@ func (s *Suite) Test_Package_checkfilePackageMakefile__options_mk(c *check.C) { t.CheckOutputEmpty() } +// When the lines of the package Makefile are checked, it is necessary +// to know whether bsd.prefs.mk has already been included. +// +// When the files are loaded recursively, Package.seenPrefs is set to +// true as soon as some file includes bsd.prefs.mk. After that, when +// loading reaches the main package Makefile again, Package.prefsLine +// is set to the line that had just been included. +// +// In this test case, the preferences are loaded indirectly by line 22, +// which includes common.mk, and that in turn includes bsd.prefs.mk. func (s *Suite) Test_Package_checkfilePackageMakefile__prefs_indirect(c *check.C) { t := s.Init(c) - t.SetUpOption("option", "An example option") + // FIXME: remove t.SetUpOption("option", "An example option") t.SetUpPackage("category/package", - ".include \"common.mk\"", - ".if ${OPSYS} == NetBSD", + ".if ${OPSYS} == NetBSD", // 20: OPSYS is not yet defined here. + ".endif", // 21 + ".include \"common.mk\"", // 22: OPSYS gets defined. + ".if ${OPSYS} == NetBSD", // 23: Now OPSYS is defined. ".endif") t.CreateFileLines("category/package/common.mk", MkCvsID, @@ -1491,16 +1503,19 @@ func (s *Suite) Test_Package_checkfilePackageMakefile__prefs_indirect(c *check.C files, mklines, allLines := G.Pkg.load() t.CheckEquals(G.Pkg.seenPrefs, false) - t.CheckEquals(G.Pkg.prefsLine, mklines.mklines[19]) + t.CheckEquals(G.Pkg.prefsLine, mklines.mklines[21]) G.Pkg.check(files, mklines, allLines) t.CheckEquals(G.Pkg.seenPrefs, true) - t.CheckEquals(G.Pkg.prefsLine, mklines.mklines[19]) + t.CheckEquals(G.Pkg.prefsLine, mklines.mklines[21]) // Since bsd.prefs.mk is included indirectly by common.mk, - // OPSYS may be used at load time in line 21. - t.CheckOutputEmpty() + // OPSYS may be used at load time in line 23, but not in line 20. + t.CheckOutputLines( + "WARN: ~/category/package/Makefile:20: " + + "To use OPSYS at load time, " + + ".include \"../../mk/bsd.prefs.mk\" first.") } func (s *Suite) Test_Package_checkfilePackageMakefile__redundancy_in_infra(c *check.C) { diff --git a/pkgtools/pkglint/files/path_test.go b/pkgtools/pkglint/files/path_test.go index c1efe6d3e4c..4cdc95e3ae2 100644 --- a/pkgtools/pkglint/files/path_test.go +++ b/pkgtools/pkglint/files/path_test.go @@ -525,8 +525,10 @@ func (s *Suite) Test_Path_Rel(c *check.C) { test("a/../b", "c/../d", "../d") test(".", "dir/file", "dir/file") - test(".", "dir/subdir/", "dir/subdir") // FIXME: missing /. at the end - test(".", "dir/subdir/.", "dir/subdir") // FIXME: missing /. at the end + // XXX: maybe the /. is missing at the end + test(".", "dir/subdir/", "dir/subdir") + // XXX: maybe the /. is missing at the end + test(".", "dir/subdir/.", "dir/subdir") } func (s *Suite) Test_NewCurrPath(c *check.C) { diff --git a/pkgtools/pkglint/files/pkglint.1 b/pkgtools/pkglint/files/pkglint.1 index 97b1ac283d5..918346c5449 100644 --- a/pkgtools/pkglint/files/pkglint.1 +++ b/pkgtools/pkglint/files/pkglint.1 @@ -1,4 +1,4 @@ -.\" $NetBSD: pkglint.1,v 1.61 2019/12/08 22:03:38 rillig Exp $ +.\" $NetBSD: pkglint.1,v 1.62 2019/12/09 20:38:16 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_test.go b/pkgtools/pkglint/files/pkglint_test.go index f60390ccede..8239505f8ed 100644 --- a/pkgtools/pkglint/files/pkglint_test.go +++ b/pkgtools/pkglint/files/pkglint_test.go @@ -275,12 +275,18 @@ func (s *Suite) Test_Pkglint_Main__autofix_exitcode(c *check.C) { // install https://github.com/rillig/gobco and adjust the following code: // // env \ -// PKGLINT_TESTDIR=C:/Users/rillig/git/pkgsrc \ -// PKGLINT_TESTCMDLINE="-r -Wall -Call -e" \ -// gobco -test.covermode=count \ -// -test.coverprofile=pkglint-pkgsrc.pprof \ -// -timeout=3600s -check.f '^Test_Pkglint__realistic' \ -// > pkglint-pkgsrc.out +// PKGLINT_TESTDIR="C:/Users/rillig/git/pkgsrc" \ +// PKGLINT_TESTCMDLINE="-r -Wall -Call -p -s -e" \ +// gobco \ +// -test=-test.covermode=count +// -test=-test.coverprofile="C:/Users/rillig/go/src/netbsd.org/pkglint/stats-go.txt" +// -test=-timeout=3600s \ +// -test=-check.f="^Test_Pkglint_Main__realistic" \ +// -stats="stats-gobco.json" \ +// > out +// +// Note that the path to -test.coverprofile must be absolute, since gobco +// runs "go test" in a temporary directory. // // See https://github.com/rillig/gobco for the tool to measure the branch coverage. func (s *Suite) Test_Pkglint_Main__realistic(c *check.C) { diff --git a/pkgtools/pkglint/files/plist.go b/pkgtools/pkglint/files/plist.go index 04e4f4fdcf1..bdbd9d08625 100644 --- a/pkgtools/pkglint/files/plist.go +++ b/pkgtools/pkglint/files/plist.go @@ -578,8 +578,7 @@ func NewPlistLineSorter(plines []*PlistLine) *plistLineSorter { func (s *plistLineSorter) Sort() { if line := s.unsortable; line != nil { - if G.Logger.IsAutofix() { - // FIXME: Missing trace.Enabled + if G.Logger.IsAutofix() && trace.Tracing { trace.Stepf("%s: This line prevents pkglint from sorting the PLIST automatically.", line) } return diff --git a/pkgtools/pkglint/files/shell.go b/pkgtools/pkglint/files/shell.go index c0c0338029d..203e6b0b102 100644 --- a/pkgtools/pkglint/files/shell.go +++ b/pkgtools/pkglint/files/shell.go @@ -9,15 +9,17 @@ import ( // Parsing and checking shell commands embedded in Makefiles type SimpleCommandChecker struct { - *ShellLineChecker cmd *MkShSimpleCommand strcmd *StrCommand time ToolTime + + mkline *MkLine + mklines *MkLines } -func NewSimpleCommandChecker(ck *ShellLineChecker, cmd *MkShSimpleCommand, time ToolTime) *SimpleCommandChecker { +func NewSimpleCommandChecker(cmd *MkShSimpleCommand, time ToolTime, mkline *MkLine, mklines *MkLines) *SimpleCommandChecker { strcmd := NewStrCommand(cmd) - return &SimpleCommandChecker{ck, cmd, strcmd, time} + return &SimpleCommandChecker{cmd, strcmd, time, mkline, mklines} } @@ -58,7 +60,7 @@ func (scc *SimpleCommandChecker) checkCommandStart() { case matches(shellword, `\$\{(PKGSRCDIR|PREFIX)(:Q)?\}`): break default: - if G.Opts.WarnExtra && !scc.MkLines.indentation.DependsOn("OPSYS") { + if G.Opts.WarnExtra && !scc.mklines.indentation.DependsOn("OPSYS") { scc.mkline.Warnf("Unknown shell command %q.", shellword) scc.mkline.Explain( "To make the package portable to all platforms that pkgsrc supports,", @@ -69,6 +71,51 @@ func (scc *SimpleCommandChecker) checkCommandStart() { } } +// Some shell commands should not be used in the install phase. +func (scc *SimpleCommandChecker) checkInstallCommand(shellcmd string) { + if trace.Tracing { + defer trace.Call0()() + } + + if !matches(scc.mklines.target, `^(?:pre|do|post)-install$`) { + return + } + + line := scc.mkline.Line + switch shellcmd { + case "${INSTALL}", + "${INSTALL_DATA}", "${INSTALL_DATA_DIR}", + "${INSTALL_LIB}", "${INSTALL_LIB_DIR}", + "${INSTALL_MAN}", "${INSTALL_MAN_DIR}", + "${INSTALL_PROGRAM}", "${INSTALL_PROGRAM_DIR}", + "${INSTALL_SCRIPT}", + "${LIBTOOL}", + "${LN}", + "${PAX}": + return + + case "sed", "${SED}", + "tr", "${TR}": + // TODO: Pkglint should not complain when sed and tr are used to transform filenames. + line.Warnf("The shell command %q should not be used in the install phase.", shellcmd) + line.Explain( + "In the install phase, the only thing that should be done is to", + "install the prepared files to their final location.", + "The file's contents should not be changed anymore.") + + case "cp", "${CP}": + line.Warnf("${CP} should not be used to install files.") + line.Explain( + "The ${CP} command is highly platform dependent and cannot overwrite read-only files.", + "Please use ${PAX} instead.", + "", + "For example, instead of:", + "\t${CP} -R ${WRKSRC}/* ${PREFIX}/foodir", + "use:", + "\tcd ${WRKSRC} && ${PAX} -wr * ${PREFIX}/foodir") + } +} + func (scc *SimpleCommandChecker) handleForbiddenCommand() bool { if trace.Tracing { defer trace.Call0()() @@ -95,7 +142,7 @@ func (scc *SimpleCommandChecker) handleTool() bool { command := scc.strcmd.Name - tool, usable := G.Tool(scc.MkLines, command, scc.time) + tool, usable := G.Tool(scc.mklines, command, scc.time) if tool != nil && !usable { scc.mkline.Warnf("The %q tool is used but not added to USE_TOOLS.", command) @@ -121,14 +168,14 @@ func (scc *SimpleCommandChecker) handleCommandVariable() bool { varname := varuse.varname - if vartype := G.Pkgsrc.VariableType(scc.MkLines, varname); vartype != nil && vartype.basicType.name == "ShellCommand" { + if vartype := G.Pkgsrc.VariableType(scc.mklines, varname); vartype != nil && vartype.basicType.name == "ShellCommand" { scc.checkInstallCommand(shellword) return true } // When the package author has explicitly defined a command // variable, assume it to be valid. - if scc.MkLines.allVars.IsDefinedSimilar(varname) { + if scc.mklines.allVars.IsDefinedSimilar(varname) { return true } @@ -205,45 +252,64 @@ func (scc *SimpleCommandChecker) checkAutoMkdirs() { return } + containsIgnoredVar := func(arg string) bool { + for _, token := range scc.mkline.Tokenize(arg, false) { + if token.Varuse != nil && matches(token.Varuse.varname, `^[_.]*[a-z]`) { + return true + } + } + return false + } + for _, arg := range scc.strcmd.Args { - // TODO: Replace regex with proper VarUse. - if !contains(arg, "$$") && !matches(arg, `\$\{[_.]*[a-z]`) { - if m, dirname := match1(arg, `^(?:\$\{DESTDIR\})?\$\{PREFIX(?:|:Q)\}/(.*)`); m { - autoMkdirs := false - if G.Pkg != nil { - // FIXME: Add test for absolute path. - plistLine := G.Pkg.Plist.Dirs[NewRelPathString(dirname)] - if plistLine != nil && !containsVarRef(plistLine.Text) { - autoMkdirs = true - } - } + if contains(arg, "$$") || containsIgnoredVar(arg) { + continue + } - if autoMkdirs { - scc.Notef("You can use AUTO_MKDIRS=yes or \"INSTALLATION_DIRS+= %s\" instead of %q.", dirname, cmdname) - scc.Explain( - "Many packages include a list of all needed directories in their", - "PLIST file.", - "In such a case, you can just set AUTO_MKDIRS=yes and be done.", - "The pkgsrc infrastructure will then create all directories in advance.", - "", - "To create directories that are not mentioned in the PLIST file,", - "it is easier to just list them in INSTALLATION_DIRS than to execute the", - "commands explicitly.", - "That way, you don't have to think about which", - "of the many INSTALL_*_DIR variables is appropriate, since", - "INSTALLATION_DIRS takes care of that.") - } else { - scc.Notef("You can use \"INSTALLATION_DIRS+= %s\" instead of %q.", dirname, cmdname) - scc.Explain( - "To create directories during installation, it is easier to just", - "list them in INSTALLATION_DIRS than to execute the commands", - "explicitly.", - "That way, you don't have to think about which", - "of the many INSTALL_*_DIR variables is appropriate,", - "since INSTALLATION_DIRS takes care of that.") - } + m, dirname := match1(arg, `^(?:\$\{DESTDIR\})?\$\{PREFIX(?:|:Q)\}/+([^/]\S*)$`) + if !m { + continue + } + + prefixRel := NewRelPathString(dirname).Clean() + if prefixRel == "." { + continue + } + + autoMkdirs := false + if G.Pkg != nil && prefixRel != "." { + plistLine := G.Pkg.Plist.Dirs[prefixRel] + if plistLine != nil && !containsVarRef(plistLine.Text) { + autoMkdirs = true } } + + if autoMkdirs { + scc.Notef("You can use AUTO_MKDIRS=yes or \"INSTALLATION_DIRS+= %s\" instead of %q.", + prefixRel.String(), cmdname) + scc.Explain( + "Many packages include a list of all needed directories in their", + "PLIST file.", + "In such a case, you can just set AUTO_MKDIRS=yes and be done.", + "The pkgsrc infrastructure will then create all directories in advance.", + "", + "To create directories that are not mentioned in the PLIST file,", + "it is easier to just list them in INSTALLATION_DIRS than to execute the", + "commands explicitly.", + "That way, you don't have to think about which", + "of the many INSTALL_*_DIR variables is appropriate, since", + "INSTALLATION_DIRS takes care of that.") + } else { + scc.Notef("You can use \"INSTALLATION_DIRS+= %s\" instead of %q.", + prefixRel.String(), cmdname) + scc.Explain( + "To create directories during installation, it is easier to just", + "list them in INSTALLATION_DIRS than to execute the commands", + "explicitly.", + "That way, you don't have to think about which", + "of the many INSTALL_*_DIR variables is appropriate,", + "since INSTALLATION_DIRS takes care of that.") + } } } @@ -283,7 +349,7 @@ func (scc *SimpleCommandChecker) checkPaxPe() { if (scc.strcmd.Name == "${PAX}" || scc.strcmd.Name == "pax") && scc.strcmd.HasOption("-pe") { scc.Warnf("Please use the -pp option to pax(1) instead of -pe.") - scc.mkline.Explain( + scc.Explain( "The -pe option tells pax to preserve the ownership of the files.", "", "When extracting distfiles as root user, this means that whatever numeric uid was", @@ -301,7 +367,7 @@ func (scc *SimpleCommandChecker) checkEchoN() { } if scc.strcmd.Name == "${ECHO}" && scc.strcmd.HasOption("-n") { - scc.mkline.Warnf("Please use ${ECHO_N} instead of \"echo -n\".") + scc.Warnf("Please use ${ECHO_N} instead of \"echo -n\".") } } @@ -671,7 +737,7 @@ func (ck *ShellLineChecker) CheckShellCommand(shellcmd string, pSetE *bool, time walker := NewMkShWalker() walker.Callback.SimpleCommand = func(command *MkShSimpleCommand) { - scc := NewSimpleCommandChecker(ck, command, time) + scc := NewSimpleCommandChecker(command, time, ck.mkline, ck.MkLines) scc.Check() // TODO: Implement getopt parsing for StrCommand. if scc.strcmd.Name == "set" && scc.strcmd.AnyArgMatches(`^-.*e`) { @@ -953,51 +1019,6 @@ func (ck *ShellLineChecker) checkVaruseToken(atoms *[]*ShAtom, quoting ShQuoting return true } -// Some shell commands should not be used in the install phase. -func (ck *ShellLineChecker) checkInstallCommand(shellcmd string) { - if trace.Tracing { - defer trace.Call0()() - } - - if !matches(ck.MkLines.target, `^(?:pre|do|post)-install$`) { - return - } - - line := ck.mkline.Line - switch shellcmd { - case "${INSTALL}", - "${INSTALL_DATA}", "${INSTALL_DATA_DIR}", - "${INSTALL_LIB}", "${INSTALL_LIB_DIR}", - "${INSTALL_MAN}", "${INSTALL_MAN_DIR}", - "${INSTALL_PROGRAM}", "${INSTALL_PROGRAM_DIR}", - "${INSTALL_SCRIPT}", - "${LIBTOOL}", - "${LN}", - "${PAX}": - return - - case "sed", "${SED}", - "tr", "${TR}": - // TODO: Pkglint should not complain when sed and tr are used to transform filenames. - line.Warnf("The shell command %q should not be used in the install phase.", shellcmd) - line.Explain( - "In the install phase, the only thing that should be done is to", - "install the prepared files to their final location.", - "The file's contents should not be changed anymore.") - - case "cp", "${CP}": - line.Warnf("${CP} should not be used to install files.") - line.Explain( - "The ${CP} command is highly platform dependent and cannot overwrite read-only files.", - "Please use ${PAX} instead.", - "", - "For example, instead of:", - "\t${CP} -R ${WRKSRC}/* ${PREFIX}/foodir", - "use:", - "\tcd ${WRKSRC} && ${PAX} -wr * ${PREFIX}/foodir") - } -} - func (ck *ShellLineChecker) checkMultiLineComment() { mkline := ck.mkline if !mkline.IsMultiline() || !contains(mkline.Text, "#") { diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go index 6fbbbb76e96..48ff0f4897b 100644 --- a/pkgtools/pkglint/files/shell_test.go +++ b/pkgtools/pkglint/files/shell_test.go @@ -44,6 +44,74 @@ func (s *Suite) Test_SimpleCommandChecker_checkCommandStart__unknown_default(c * "WARN: Makefile:8: UNKNOWN_TOOL is used but not defined.") } +func (s *Suite) Test_SimpleCommandChecker_checkInstallCommand(c *check.C) { + t := s.Init(c) + + lines := func(lines ...string) []string { return lines } + + test := func(lines []string, diagnostics ...string) { + mklines := t.NewMkLines("filename.mk", + mapStr(lines, func(s string) string { return "\t" + s })...) + mklines.target = "do-install" + + mklines.ForEach(func(mkline *MkLine) { + program, err := parseShellProgram(nil, mkline.ShellCommand()) + assertNil(err, "") + + walker := NewMkShWalker() + walker.Callback.SimpleCommand = func(command *MkShSimpleCommand) { + scc := NewSimpleCommandChecker(command, RunTime, mkline, mklines) + scc.checkInstallCommand(command.Name.MkText) + } + walker.Walk(program) + }) + + t.CheckOutput(diagnostics) + } + + test( + lines( + "sed", + "${SED}"), + "WARN: filename.mk:1: The shell command \"sed\" "+ + "should not be used in the install phase.", + "WARN: filename.mk:2: The shell command \"${SED}\" "+ + "should not be used in the install phase.") + + test( + lines( + "tr", + "${TR}"), + "WARN: filename.mk:1: The shell command \"tr\" "+ + "should not be used in the install phase.", + "WARN: filename.mk:2: The shell command \"${TR}\" "+ + "should not be used in the install phase.") + + test( + lines( + "cp", + "${CP}"), + "WARN: filename.mk:1: ${CP} should not be used to install files.", + "WARN: filename.mk:2: ${CP} should not be used to install files.") + + test( + lines( + "${INSTALL}", + "${INSTALL_DATA}", + "${INSTALL_DATA_DIR}", + "${INSTALL_LIB}", + "${INSTALL_LIB_DIR}", + "${INSTALL_MAN}", + "${INSTALL_MAN_DIR}", + "${INSTALL_PROGRAM}", + "${INSTALL_PROGRAM_DIR}", + "${INSTALL_SCRIPT}", + "${LIBTOOL}", + "${LN}", + "${PAX}"), + nil...) +} + func (s *Suite) Test_SimpleCommandChecker_handleForbiddenCommand(c *check.C) { t := s.Init(c) @@ -155,10 +223,14 @@ func (s *Suite) Test_SimpleCommandChecker_handleCommandVariable__from_package(c func (s *Suite) Test_SimpleCommandChecker_handleShellBuiltin(c *check.C) { t := s.Init(c) + mklines := t.NewMkLines("filename.mk", + "\t:") + mkline := mklines.mklines[0] + test := func(command string, isBuiltin bool) { token := NewShToken(command, NewShAtom(shtText, command, shqPlain)) simpleCommand := &MkShSimpleCommand{Name: token} - scc := NewSimpleCommandChecker(nil, simpleCommand, RunTime) + scc := NewSimpleCommandChecker(simpleCommand, RunTime, mkline, mklines) t.CheckEquals(scc.handleShellBuiltin(), isBuiltin) } @@ -352,6 +424,83 @@ func (s *Suite) Test_SimpleCommandChecker_checkAutoMkdirs__conditional_PLIST(c * "instead of \"${INSTALL_DATA_DIR}\".") } +func (s *Suite) Test_SimpleCommandChecker_checkAutoMkdirs__strange_paths(c *check.C) { + t := s.Init(c) + + test := func(path string, diagnostics ...string) { + mklines := t.NewMkLines("filename.mk", + "\t${INSTALL_DATA_DIR} "+path) + mklines.ForEach(func(mkline *MkLine) { + program, err := parseShellProgram(nil, mkline.ShellCommand()) + assertNil(err, "") + + walker := NewMkShWalker() + walker.Callback.SimpleCommand = func(command *MkShSimpleCommand) { + scc := NewSimpleCommandChecker(command, RunTime, mkline, mklines) + scc.checkAutoMkdirs() + } + walker.Walk(program) + }) + t.CheckOutput(diagnostics) + } + + t.Chdir("category/package") + + test("${PREFIX}", + nil...) + + test("${PREFIX}/", + nil...) + + test("${PREFIX}//", + nil...) + + test("${PREFIX}/.", + nil...) + + test("${PREFIX}//non-canonical", + "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= non-canonical\" "+ + "instead of \"${INSTALL_DATA_DIR}\".") + + test("${PREFIX}/non-canonical/////", + "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= non-canonical\" "+ + "instead of \"${INSTALL_DATA_DIR}\".") + + test("${PREFIX}/${VAR}", + "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= ${VAR}\" "+ + "instead of \"${INSTALL_DATA_DIR}\".") + + test("${PREFIX}/${VAR.param}", + "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= ${VAR.param}\" "+ + "instead of \"${INSTALL_DATA_DIR}\".") + + test("${PREFIX}/${.CURDIR}", + "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= ${.CURDIR}\" "+ + "instead of \"${INSTALL_DATA_DIR}\".") + + // Internal variables are ok. + test("${PREFIX}/${_INTERNAL}", + "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= ${_INTERNAL}\" "+ + "instead of \"${INSTALL_DATA_DIR}\".") + + // Ignore variables from a :@ modifier. + test("${PREFIX}/${.f.}", + nil...) + + // Ignore variables from a .for loop. + test("${PREFIX}/${f}", + nil...) + + // Ignore variables from a .for loop. + test("${PREFIX}/${_f_}", + nil...) + + // Ignore paths containing shell variables as it is hard to + // predict their values using static analysis. + test("${PREFIX}/$$f", + nil...) +} + // This test ensures that the command line options to INSTALL_*_DIR are properly // parsed and do not lead to "can only handle one directory at a time" warnings. func (s *Suite) Test_SimpleCommandChecker_checkInstallMulti(c *check.C) { @@ -1681,71 +1830,6 @@ func (s *Suite) Test_ShellLineChecker_variableNeedsQuoting__integration(c *check "WARN: filename.mk:3: Unquoted shell variable \"target\".") } -func (s *Suite) Test_ShellLineChecker_checkInstallCommand(c *check.C) { - t := s.Init(c) - - test := func(lines ...string) { - var cmds []string - i := 0 - for i < len(lines) && lines[i] != "" { - cmds = append(cmds, "\t"+lines[i]) - i++ - } - diagnostics := lines[i+1:] - - mklines := t.NewMkLines("filename.mk", cmds...) - mklines.target = "do-install" - - mklines.ForEach(func(mkline *MkLine) { - ck := NewShellLineChecker(mklines, mkline) - ck.checkInstallCommand(mkline.ShellCommand()) - }) - - t.CheckOutput(diagnostics) - } - - test( - "sed", - "${SED}", - "", - "WARN: filename.mk:1: The shell command \"sed\" "+ - "should not be used in the install phase.", - "WARN: filename.mk:2: The shell command \"${SED}\" "+ - "should not be used in the install phase.") - - test( - "tr", - "${TR}", - "", - "WARN: filename.mk:1: The shell command \"tr\" "+ - "should not be used in the install phase.", - "WARN: filename.mk:2: The shell command \"${TR}\" "+ - "should not be used in the install phase.") - - test( - "cp", - "${CP}", - "", - "WARN: filename.mk:1: ${CP} should not be used to install files.", - "WARN: filename.mk:2: ${CP} should not be used to install files.") - - test( - "${INSTALL}", - "${INSTALL_DATA}", - "${INSTALL_DATA_DIR}", - "${INSTALL_LIB}", - "${INSTALL_LIB_DIR}", - "${INSTALL_MAN}", - "${INSTALL_MAN_DIR}", - "${INSTALL_PROGRAM}", - "${INSTALL_PROGRAM_DIR}", - "${INSTALL_SCRIPT}", - "${LIBTOOL}", - "${LN}", - "${PAX}", - "") -} - func (s *Suite) Test_ShellLineChecker_checkMultiLineComment(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go index 39ae8dcc902..61b586bf202 100644 --- a/pkgtools/pkglint/files/util.go +++ b/pkgtools/pkglint/files/util.go @@ -88,6 +88,22 @@ func mapStr(slice []string, fn func(s string) string) []string { return result } +func invalidCharacters(s string, valid *textproc.ByteSet) string { + var unis strings.Builder + + for _, r := range s { + if r == rune(byte(r)) && valid.Contains(byte(r)) { + continue + } + _, _ = fmt.Fprintf(&unis, " %U", r) + } + + if unis.Len() == 0 { + return "" + } + return unis.String()[1:] +} + // intern returns an independent copy of the given string. // // It should be called when only a small substring of a large string @@ -367,13 +383,19 @@ func detab(s string) string { return detabbed.String() } -// alignWith extends str with as many tabs as needed to reach +// alignWith extends str with as many tabs and spaces as needed to reach // the same screen width as the other string. func alignWith(str, other string) string { - alignBefore := (tabWidth(other) + 7) & -8 - alignAfter := tabWidth(str) & -8 - tabsNeeded := imax((alignBefore-alignAfter)/8, 1) - return str + strings.Repeat("\t", tabsNeeded) + strWidth := tabWidth(str) + otherWidth := tabWidth(other) + if otherWidth <= strWidth { + return str + "\t" + } + if strWidth&-8 != otherWidth&-8 { + strWidth &= -8 + } + alignment := indent(otherWidth - strWidth) + return str + alignment } func indent(width int) string { @@ -462,8 +484,6 @@ func containsVarRef(s string) bool { return false } -func hasAlnumPrefix(s string) bool { return s != "" && textproc.AlnumU.Contains(s[0]) } - // Once remembers with which arguments its FirstTime method has been called // and only returns true on each first call. type Once struct { diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go index eb2e1b4f460..4530a99aa88 100644 --- a/pkgtools/pkglint/files/util_test.go +++ b/pkgtools/pkglint/files/util_test.go @@ -227,9 +227,9 @@ func (s *Suite) Test_alignWith(c *check.C) { // At least one tab is _always_ added. test("", "", "\t") - test("VAR=", "1234567", "VAR=\t") + test("VAR=", "1234567", "VAR= ") test("VAR=", "12345678", "VAR=\t") - test("VAR=", "123456789", "VAR=\t\t") + test("VAR=", "123456789", "VAR=\t ") // At least one tab is added in any case, // even if the other string is shorter. @@ -456,14 +456,6 @@ func (s *Suite) Test_containsVarRef(c *check.C) { test("$$VAR", false) // An escaped dollar character. } -func (s *Suite) Test_hasAlnumPrefix(c *check.C) { - t := s.Init(c) - - t.CheckEquals(hasAlnumPrefix(""), false) - t.CheckEquals(hasAlnumPrefix("A"), true) - t.CheckEquals(hasAlnumPrefix(","), false) -} - func (s *Suite) Test_Once(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go index 16d92e57376..ef269e59b38 100644 --- a/pkgtools/pkglint/files/vartypecheck.go +++ b/pkgtools/pkglint/files/vartypecheck.go @@ -1327,8 +1327,22 @@ func (cv *VartypeCheck) URL() { } func (cv *VartypeCheck) UserGroupName() { - if cv.Value == cv.ValueNoVar && !matches(cv.Value, `^[0-9_a-z][0-9_a-z-]*[0-9_a-z]$`) { - cv.Warnf("Invalid user or group name %q.", cv.Value) + value := cv.Value + if value != cv.ValueNoVar { + return + } + invalid := invalidCharacters(value, textproc.NewByteSet("---0-9_a-z")) + if invalid != "" { + cv.Warnf("User or group name %q contains invalid characters: %s", + value, invalid) + return + } + + if hasPrefix(value, "-") { + cv.Errorf("User or group name %q must not start with a hyphen.", value) + } + if hasSuffix(value, "-") { + cv.Errorf("User or group name %q must not end with a hyphen.", value) } } @@ -1473,7 +1487,7 @@ func (cv *VartypeCheck) Yes() { "but using \".if defined(VARNAME)\" alone.") default: - if !matches(cv.Value, `^(?:YES|yes)(?:[\t ]+#.*)?$`) { + if cv.Value != "YES" && cv.Value != "yes" { cv.Warnf("%s should be set to YES or yes.", cv.Varname) cv.Explain( "This variable means \"yes\" if it is defined, and \"no\" if it is undefined.", @@ -1511,7 +1525,7 @@ func (cv *VartypeCheck) YesNo() { "both forms are actually used.", "As long as this is the case, when checking the variable value,", "both must be accepted.") - } else if !matches(cv.Value, `^(?:YES|yes|NO|no)(?:[\t ]+#.*)?$`) { + } else if !matches(cv.Value, `^(?:YES|yes|NO|no)$`) { cv.Warnf("%s should be set to YES, yes, NO, or no.", cv.Varname) } } diff --git a/pkgtools/pkglint/files/vartypecheck_test.go b/pkgtools/pkglint/files/vartypecheck_test.go index 20d93b1649a..c34b7379f82 100644 --- a/pkgtools/pkglint/files/vartypecheck_test.go +++ b/pkgtools/pkglint/files/vartypecheck_test.go @@ -1875,11 +1875,20 @@ func (s *Suite) Test_VartypeCheck_UserGroupName(c *check.C) { "typical_username", "user123", "domain\\user", - "${OTHER_VAR}") + "${OTHER_VAR}", + "r", + "-rf", + "rf-") vt.Output( - "WARN: filename.mk:1: Invalid user or group name \"user with spaces\".", - "WARN: filename.mk:4: Invalid user or group name \"domain\\\\user\".") + "WARN: filename.mk:1: User or group name \"user with spaces\" "+ + "contains invalid characters: U+0020 U+0020", + "WARN: filename.mk:4: User or group name \"domain\\\\user\" "+ + "contains invalid characters: U+005C", + "ERROR: filename.mk:7: User or group name \"-rf\" "+ + "must not start with a hyphen.", + "ERROR: filename.mk:8: User or group name \"rf-\" "+ + "must not end with a hyphen.") } func (s *Suite) Test_VartypeCheck_VariableName(c *check.C) { @@ -2053,6 +2062,14 @@ func (s *Suite) Test_VartypeCheck_Yes(c *check.C) { "WARN: filename.mk:11: BUILD_USES_MSGFMT should only be used in a \".if defined(...)\" condition.", "WARN: filename.mk:12: BUILD_USES_MSGFMT should only be used in a \".if defined(...)\" condition.", "WARN: filename.mk:13: BUILD_USES_MSGFMT should only be used in a \".if defined(...)\" condition.") + + vt.Op(opAssign) + vt.Values( + // This was accidentally accepted until 2019-12-09. + "yes \\# this is not a comment") + + vt.Output( + "WARN: filename.mk:21: BUILD_USES_MSGFMT should be set to YES or yes.") } func (s *Suite) Test_VartypeCheck_YesNo(c *check.C) { @@ -2063,11 +2080,15 @@ func (s *Suite) Test_VartypeCheck_YesNo(c *check.C) { "yes", "no", "ja", - "${YESVAR}") + "${YESVAR}", + "yes # comment", + "no # comment", + "Yes indeed") vt.Output( "WARN: filename.mk:3: PKG_DEVELOPER should be set to YES, yes, NO, or no.", - "WARN: filename.mk:4: PKG_DEVELOPER should be set to YES, yes, NO, or no.") + "WARN: filename.mk:4: PKG_DEVELOPER should be set to YES, yes, NO, or no.", + "WARN: filename.mk:7: PKG_DEVELOPER should be set to YES, yes, NO, or no.") vt.Op(opUseMatch) vt.Values( @@ -2086,6 +2107,14 @@ func (s *Suite) Test_VartypeCheck_YesNo(c *check.C) { "\"[yY][eE][sS]\" or \"[nN][oO]\", not \"[Yy]es\".", "WARN: filename.mk:15: PKG_DEVELOPER should be matched against "+ "\"[yY][eE][sS]\" or \"[nN][oO]\", not \"[Nn]o\".") + + vt.Op(opAssign) + vt.Values( + // This was accidentally accepted until 2019-12-09. + "yes \\# this is not a comment") + + vt.Output( + "WARN: filename.mk:21: PKG_DEVELOPER should be set to YES, yes, NO, or no.") } func (s *Suite) Test_VartypeCheck_YesNoIndirectly(c *check.C) { |