From 1d1d611496bdec250a1481587fc4127a09ba7fa8 Mon Sep 17 00:00:00 2001 From: rillig Date: Fri, 25 Jun 2021 14:15:00 +0000 Subject: [PATCH] pkgtools/pkglint: update to 21.2.1 Changes since 21.2.0: Files whose names ends in '~' are ignored by pkglint since they are ignored by CVS as well. Variables with name BUILDLINK_TRANSFORM.* may contain '-Wl,-rpath,' directly in commands of the form 'rm:*', just like their counterpart BUILDLINK_TRANSFORM without a package name in the variable name. Several new tests. --- pkgtools/pkglint/Makefile | 4 +- pkgtools/pkglint/files/check_test.go | 7 - pkgtools/pkglint/files/distinfo_test.go | 95 ++++++++ pkgtools/pkglint/files/files_test.go | 17 ++ pkgtools/pkglint/files/intqa/qa.go | 20 +- pkgtools/pkglint/files/licenses.go | 32 +-- pkgtools/pkglint/files/licenses_test.go | 28 ++- pkgtools/pkglint/files/line_test.go | 228 ++++++++++++++++++- pkgtools/pkglint/files/linechecker_test.go | 14 ++ pkgtools/pkglint/files/lines_test.go | 77 +++++++ pkgtools/pkglint/files/logging_test.go | 174 ++++++++++++++ pkgtools/pkglint/files/makepat/pat_test.go | 1 + pkgtools/pkglint/files/mklinechecker.go | 3 +- pkgtools/pkglint/files/mklinechecker_test.go | 12 +- pkgtools/pkglint/files/mklineparser_test.go | 3 + pkgtools/pkglint/files/package.go | 26 ++- pkgtools/pkglint/files/pkglint_test.go | 24 +- pkgtools/pkglint/files/pkgsrc.go | 2 +- pkgtools/pkglint/files/pkgver/vercmp_test.go | 2 + pkgtools/pkglint/files/util.go | 4 +- pkgtools/pkglint/files/util_test.go | 6 + pkgtools/pkglint/files/vartypecheck.go | 2 +- 22 files changed, 727 insertions(+), 54 deletions(-) diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index 36ce1832bc61..7db6c96df772 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.689 2021/06/17 09:11:55 rillig Exp $ +# $NetBSD: Makefile,v 1.690 2021/06/25 14:15:00 rillig Exp $ -PKGNAME= pkglint-21.2.0 +PKGNAME= pkglint-21.2.1 CATEGORIES= pkgtools DISTNAME= tools MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/} diff --git a/pkgtools/pkglint/files/check_test.go b/pkgtools/pkglint/files/check_test.go index 964287306293..d0e9deb5539e 100644 --- a/pkgtools/pkglint/files/check_test.go +++ b/pkgtools/pkglint/files/check_test.go @@ -104,13 +104,6 @@ func (s *Suite) TearDownTest(c *check.C) { func Test__qa(t *testing.T) { ck := intqa.NewQAChecker(t.Errorf) - ck.Configure("distinfo.go", "*", "*", -intqa.EMissingTest) // TODO - ck.Configure("files.go", "*", "*", -intqa.EMissingTest) // TODO - ck.Configure("licenses.go", "*", "*", -intqa.EMissingTest) // TODO - ck.Configure("line.go", "*", "*", -intqa.EMissingTest) // TODO - ck.Configure("linechecker.go", "*", "*", -intqa.EMissingTest) // TODO - ck.Configure("lines.go", "*", "*", -intqa.EMissingTest) // TODO - ck.Configure("logging.go", "*", "*", -intqa.EMissingTest) // TODO ck.Configure("mkline.go", "*", "*", -intqa.EMissingTest) // TODO ck.Configure("mklineparser.go", "*", "*", -intqa.EMissingTest) // TODO ck.Configure("mklinechecker.go", "*", "*", -intqa.EMissingTest) // TODO diff --git a/pkgtools/pkglint/files/distinfo_test.go b/pkgtools/pkglint/files/distinfo_test.go index 525fb2288fc2..dfe065458ab6 100644 --- a/pkgtools/pkglint/files/distinfo_test.go +++ b/pkgtools/pkglint/files/distinfo_test.go @@ -877,6 +877,63 @@ func (s *Suite) Test_distinfoLinesChecker_checkPatchSha1(c *check.C) { "ERROR: ~/category/package/distinfo:5: Patch patch-nonexistent does not exist.") } +func (s *Suite) Test_distinfoFileInfo_filename(c *check.C) { + t := s.Init(c) + + info := distinfoFileInfo{ + isPatch: no, + hashes: []distinfoHash{{ + line: t.NewLine("irrelevant", 12345, "irrelevant"), + filename: "distinfo", + algorithm: "irrelevant", + hash: "irrelevant", + }}, + } + + t.CheckEquals(info.filename(), RelPath("distinfo")) +} + +func (s *Suite) Test_distinfoFileInfo_line(c *check.C) { + t := s.Init(c) + + line := t.NewLine("irrelevant", 12345, "irrelevant") + info := distinfoFileInfo{ + isPatch: no, + hashes: []distinfoHash{{ + line: line, + filename: "irrelevant", + algorithm: "irrelevant", + hash: "irrelevant", + }}, + } + + t.CheckEquals(info.line(), line) +} + +func (s *Suite) Test_distinfoFileInfo_algorithms(c *check.C) { + t := s.Init(c) + + info := distinfoFileInfo{ + isPatch: no, + hashes: []distinfoHash{ + { + line: t.NewLine("irrelevant", 12345, "irrelevant"), + filename: "irrelevant", + algorithm: "SHA1", + hash: "irrelevant", + }, + { + line: t.NewLine("irrelevant", 12346, "irrelevant"), + filename: "irrelevant", + algorithm: "Size", + hash: "irrelevant", + }, + }, + } + + t.CheckEquals(info.algorithms(), "SHA1, Size") +} + // The check for versioned distfiles only makes sense if the file // has the usual hashes for distfiles. func (s *Suite) Test_distinfoFileInfo_hasDistfileAlgorithms__code_coverage(c *check.C) { @@ -920,3 +977,41 @@ func (s *Suite) Test_distinfoFileInfo_hasDistfileAlgorithms__code_coverage(c *ch "ERROR: distinfo:15: Expected SHA1, RMD160, SHA512, Size checksums for "+ "\"dist-d.tar.gz\", got SHA1, RMD160, SHA512, other.") } + +func (s *Suite) Test_computePatchSha1Hex(c *check.C) { + t := s.Init(c) + + test := func(expectedHash string, lines ...string) { + fileLines := t.NewLines("irrelevant", lines...) + + hash := computePatchSha1Hex(fileLines) + + t.CheckEquals(hash, expectedHash) + } + + // The well-known hash for an empty string. + test( + "da39a3ee5e6b4b0d3255bfef95601890afd80709", + nil..., + ) + + // Any line containing "$" + "NetBSD" is ignored for computing + // the hash since the CVS IDs are irrelevant. This assumes that + // the patch hunks themselves do not contain CVS IDs, and this + // is checked in PatchChecker.checktextCvsID. + test( + "da39a3ee5e6b4b0d3255bfef95601890afd80709", + CvsID, + CvsID+"$", + // This is not a CVS ID since the final '$' is missing, but it is + // close enough. See mk/checksum/distinfo.awk, function patchsum. + "Some text $"+"NetBSD-2020", + ) + + test( + // == sha1Hex("--- old\n+++ new\n") + "bb5a3903635415d367f47c02990e031469ab0346", + "--- old", + "+++ new", + ) +} diff --git a/pkgtools/pkglint/files/files_test.go b/pkgtools/pkglint/files/files_test.go index 6dcdc7d4d80e..7cd406d96934 100644 --- a/pkgtools/pkglint/files/files_test.go +++ b/pkgtools/pkglint/files/files_test.go @@ -4,6 +4,23 @@ import ( "gopkg.in/check.v1" ) +func (s *Suite) Test_LoadMk(c *check.C) { + t := s.Init(c) + + t.Chdir(".") + t.CreateFileLines("filename.mk", + "# line 1 \\", + "# continues in line 2") + + mklines := LoadMk("filename.mk", nil, 0) + + t.CheckEquals(len(mklines.mklines), 1) + // The '#' from line 2 is discarded in nextLogicalLine, to properly + // parse multi-line variable assignments that are commented out. + // See Test_MkLineParser_MatchVarassign, 'multi-line variable'. + t.CheckEquals(mklines.mklines[0].Text, "# line 1 continues in line 2") +} + func (s *Suite) Test_Load(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/intqa/qa.go b/pkgtools/pkglint/files/intqa/qa.go index 766e0b38a4e7..83e058b474d8 100644 --- a/pkgtools/pkglint/files/intqa/qa.go +++ b/pkgtools/pkglint/files/intqa/qa.go @@ -21,25 +21,27 @@ const ( ENone Error = iota + 1 EAll - // A function or method does not have a corresponding test. + // EMissingTest complains if a function or method does not have a + // corresponding test. EMissingTest - // The name of a test function does not correspond to a program - // element to be tested. + // EMissingTestee complains if the name of a test function does not + // correspond to a program element to be tested. EMissingTestee - // The tests are not in the same order as their corresponding - // testees in the main code. + // EOrder complains if the tests are not in the same order as their + // corresponding testees in the main code. EOrder - // The test method does not have a valid name. + // EName complains if the test method does not have a valid name. EName - // The file of the test method does not correspond to the - // file of the testee. + // EFile complains if the file of the test method does not correspond + // to the file of the testee. EFile - // All methods of a type must be in the same file as the type definition. + // EMethodsSameFile complains if a method of a type is in a different + // file than its corresponding type definition. EMethodsSameFile ) diff --git a/pkgtools/pkglint/files/licenses.go b/pkgtools/pkglint/files/licenses.go index dd2a79c65c18..4f014531fcbe 100644 --- a/pkgtools/pkglint/files/licenses.go +++ b/pkgtools/pkglint/files/licenses.go @@ -23,6 +23,22 @@ func (lc *LicenseChecker) Check(value string, op MkOperator) { cond.Walk(lc.checkNode) } +func (lc *LicenseChecker) checkNode(cond *licenses.Condition) { + if name := cond.Name; name != "" && name != "append-placeholder" { + lc.checkName(name) + return + } + + if cond.And && cond.Or { + lc.MkLine.Errorf("AND and OR operators in license conditions can only be combined using parentheses.") + lc.MkLine.Explain( + "Examples for valid license conditions are:", + "", + "\tlicense1 AND license2 AND (license3 OR license4)", + "\t(((license1 OR license2) AND (license3 OR license4)))") + } +} + func (lc *LicenseChecker) checkName(license string) { licenseFile := NewCurrPath("") pkg := lc.MkLines.pkg @@ -54,19 +70,3 @@ func (lc *LicenseChecker) checkName(license string) { seeGuide("Handling licenses", "handling-licenses"))) } } - -func (lc *LicenseChecker) checkNode(cond *licenses.Condition) { - if name := cond.Name; name != "" && name != "append-placeholder" { - lc.checkName(name) - return - } - - if cond.And && cond.Or { - lc.MkLine.Errorf("AND and OR operators in license conditions can only be combined using parentheses.") - lc.MkLine.Explain( - "Examples for valid license conditions are:", - "", - "\tlicense1 AND license2 AND (license3 OR license4)", - "\t(((license1 OR license2) AND (license3 OR license4)))") - } -} diff --git a/pkgtools/pkglint/files/licenses_test.go b/pkgtools/pkglint/files/licenses_test.go index 4fa8f20a7d35..be7263a4ca57 100644 --- a/pkgtools/pkglint/files/licenses_test.go +++ b/pkgtools/pkglint/files/licenses_test.go @@ -15,7 +15,8 @@ func (s *Suite) Test_LicenseChecker_Check(c *check.C) { "LICENSE=\t"+licenseValue) mklines.ForEach(func(mkline *MkLine) { - (&LicenseChecker{mklines, mkline}).Check(mkline.Value(), opAssign) + ck := LicenseChecker{mklines, mkline} + ck.Check(mkline.Value(), mkline.Op()) }) t.CheckOutput(diagnostics) @@ -49,6 +50,31 @@ func (s *Suite) Test_LicenseChecker_Check(c *check.C) { nil...) } +func (s *Suite) Test_LicenseChecker_checkNode(c *check.C) { + t := s.Init(c) + + t.CreateFileLines("licenses/gnu-gpl-v2", + "The licenses for most software are designed to take away ...") + + test := func(assignment string, diagnostics ...string) { + mklines := t.SetUpFileMkLines("Makefile", + assignment) + + mklines.ForEach(func(mkline *MkLine) { + ck := LicenseChecker{mklines, mkline} + ck.Check(mkline.Value(), mkline.Op()) + }) + + t.CheckOutput(diagnostics) + } + + test("LICENSE=\tfirst second", + "ERROR: ~/Makefile:1: Parse error for license condition \"first second\".") + + test("LICENSE+=\tadded", + "ERROR: ~/Makefile:1: Parse error for appended license condition \"added\".") +} + func (s *Suite) Test_LicenseChecker_checkName__LICENSE_FILE(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/line_test.go b/pkgtools/pkglint/files/line_test.go index fb18d6657b06..a3e90c88913f 100644 --- a/pkgtools/pkglint/files/line_test.go +++ b/pkgtools/pkglint/files/line_test.go @@ -2,14 +2,105 @@ package pkglint import ( "gopkg.in/check.v1" + "netbsd.org/pkglint/regex" ) +func (s *Suite) Test_RawLine_Orig(c *check.C) { + t := s.Init(c) + + line := t.NewLine("filename", 123, "text") + + t.CheckEquals(line.raw[0].orignl, "text\n") + t.CheckEquals(line.raw[0].Orig(), "text") +} + +func (s *Suite) Test_NewLocation(c *check.C) { + t := s.Init(c) + + loc := NewLocation("subdir/filename", 123) + + t.CheckEquals(loc.Filename, NewCurrPathString("subdir/filename")) + t.CheckEquals(loc.lineno, 123) +} + +func (s *Suite) Test_Location_Lineno(c *check.C) { + t := s.Init(c) + + loc := NewLocation("subdir/filename", 123) + + t.CheckEquals(loc.Lineno(0), 123) +} + +func (s *Suite) Test_Location_File(c *check.C) { + t := s.Init(c) + + loc := NewLocation("subdir/filename", 123) + + t.CheckEquals(loc.File("other"), NewCurrPathString("subdir/other")) +} + func (s *Suite) Test_NewLine__assertion(c *check.C) { t := s.Init(c) t.ExpectAssert(func() { NewLine("filename", 123, "text", nil) }) } +func (s *Suite) Test_NewLineMulti(c *check.C) { + t := s.Init(c) + + line := NewLineMulti("subdir/filename", 123, "", []*RawLine{nil, nil, nil}) + + t.CheckEquals(line.Filename(), NewCurrPathString("subdir/filename")) + t.CheckEquals(line.Linenos(), "123--125") +} + +func (s *Suite) Test_NewLineEOF(c *check.C) { + t := s.Init(c) + + line := NewLineEOF("subdir/filename") + + t.CheckEquals(line.Filename(), NewCurrPathString("subdir/filename")) + t.CheckEquals(line.Linenos(), "EOF") +} + +func (s *Suite) Test_NewLineWhole(c *check.C) { + t := s.Init(c) + + line := NewLineWhole("subdir/filename") + + t.CheckEquals(line.Filename(), NewCurrPathString("subdir/filename")) + t.CheckEquals(line.Linenos(), "") +} + +func (s *Suite) Test_Line_Filename(c *check.C) { + t := s.Init(c) + + line := t.NewLine("subdir/filename", 123, "") + + t.CheckEquals(line.Filename(), NewCurrPathString("subdir/filename")) +} + +func (s *Suite) Test_Line_File(c *check.C) { + t := s.Init(c) + + line := t.NewLine("subdir/filename", 123, "") + + t.CheckEquals(line.File("subsub/nested"), + NewCurrPathString("subdir/subsub/nested")) +} + +func (s *Suite) Test_Line_Linenos(c *check.C) { + t := s.Init(c) + + single := t.NewLine("filename", 123, "") + whole := NewLineWhole("filename") + multi := NewLineMulti("filename", 123, "text", []*RawLine{nil, nil, nil}) + + t.CheckEquals(single.Linenos(), "123") + t.CheckEquals(whole.Linenos(), "") + t.CheckEquals(multi.Linenos(), "123--125") +} + func (s *Suite) Test_Line_RelLine__assertion(c *check.C) { t := s.Init(c) @@ -28,13 +119,148 @@ func (s *Suite) Test_Line_RelLocation__assertion(c *check.C) { t.ExpectAssert(func() { line.RelLocation(whole.Location) }) } +func (s *Suite) Test_Line_Rel(c *check.C) { + t := s.Init(c) + + t.Chdir(".") + line1 := t.NewLine("subdir/filename", 123, "from") + + rel := line1.Rel("other/filename") + + t.CheckEquals(rel, NewRelPathString("../other/filename")) +} + func (s *Suite) Test_Line_IsMultiline(c *check.C) { t := s.Init(c) t.CheckEquals(t.NewLine("filename", 123, "text").IsMultiline(), false) + t.CheckEquals(NewLineEOF("filename").IsMultiline(), false) - t.CheckEquals(NewLineMulti("filename", 123, "text", []*RawLine{nil, nil, nil}).IsMultiline(), true) + multi := NewLineMulti("filename", 123, "text", []*RawLine{nil, nil, nil}) + t.CheckEquals(multi.IsMultiline(), true) +} + +func (s *Suite) Test_Line_RawText(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--show-autofix") + line := t.NewLine("filename", 123, "text") + + t.CheckEquals(line.RawText(0), "text") + + fix := line.Autofix() + fix.Notef("Replacing.") + fix.Replace("text", "replaced") + fix.Apply() + + t.CheckEquals(line.RawText(0), "replaced") + + t.CheckOutputLines( + "NOTE: filename:123: Replacing.", + "AUTOFIX: filename:123: Replacing \"text\" with \"replaced\".") +} + +func (s *Suite) Test_Line_IsCvsID(c *check.C) { + t := s.Init(c) + + test := func(text string, prefix regex.Pattern, expectedID bool, expectedExpanded bool) { + line := t.NewLine("filename", 123, text) + + id, expanded := line.IsCvsID(prefix) + + t.CheckEquals(id, expectedID) + t.CheckEquals(expanded, expectedExpanded) + } + + test(CvsID, ``, true, false) + + test("$"+"NetBSD: ... $", ``, true, true) +} + +func (s *Suite) Test_Line_Errorf(c *check.C) { + t := s.Init(c) + + line := t.NewLine("filename", 123, "text") + + line.Errorf("For your information.") + + t.CheckOutputLines( + "ERROR: filename:123: For your information.") +} + +func (s *Suite) Test_Line_Warnf(c *check.C) { + t := s.Init(c) + + line := t.NewLine("filename", 123, "text") + + line.Warnf("For your information.") + + t.CheckOutputLines( + "WARN: filename:123: For your information.") +} + +func (s *Suite) Test_Line_Notef(c *check.C) { + t := s.Init(c) + + line := t.NewLine("filename", 123, "text") + + line.Notef("For your information.") + + t.CheckOutputLines( + "NOTE: filename:123: For your information.") +} + +func (s *Suite) Test_Line_Explain(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--explain") + line := t.NewLine("filename", 123, "text") + + line.Notef("For your information.") + line.Explain( + "First line", + "continues.") + + t.CheckOutputLines( + "NOTE: filename:123: For your information.", + "", + "\tFirst line continues.", + "") +} + +func (s *Suite) Test_Line_Explain__without_diagnostic(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--explain") + line := t.NewLine("filename", 123, "text") + + // It is unusual to have an explanation without any preceding + // diagnostic. + line.Explain( + "First line", + "continues.") + + // There is no empty line above the explanation since that is the + // job of the SeparatorWriter, which only steps in if there is a + // diagnostic above the explanation. + t.CheckOutputLines( + "\tFirst line continues.", + "") +} + +func (s *Suite) Test_Line_String(c *check.C) { + t := s.Init(c) + + line := t.NewLine("filename", 123, "text") + t.CheckEquals(line.String(), "filename:123: text") + + // Since Line.String is only used for debugging purposes, trailing + // whitespace is OK. + t.CheckEquals(NewLineEOF("filename").String(), "filename:EOF: ") + + multi := NewLineMulti("filename", 123, "text", []*RawLine{nil, nil, nil}) + t.CheckEquals(multi.String(), "filename:123--125: text") } func (s *Suite) Test_Line_Autofix__reuse_incomplete(c *check.C) { diff --git a/pkgtools/pkglint/files/linechecker_test.go b/pkgtools/pkglint/files/linechecker_test.go index fa14c4e497df..f9c000116224 100644 --- a/pkgtools/pkglint/files/linechecker_test.go +++ b/pkgtools/pkglint/files/linechecker_test.go @@ -17,6 +17,20 @@ func (s *Suite) Test_LineChecker_CheckLength(c *check.C) { "WARN: DESCR:1: Line too long (should be no more than 20 characters).") } +func (s *Suite) Test_LineChecker_CheckValidCharacters(c *check.C) { + t := s.Init(c) + + doTest := func(autofix bool) { + line := t.NewLine("filename", 32, "The letter \u00DC is an umlaut.") + + LineChecker{line}.CheckValidCharacters() + } + + t.ExpectDiagnosticsAutofix( + doTest, + "WARN: filename:32: Line contains invalid characters (U+00DC).") +} + func (s *Suite) Test_LineChecker_CheckTrailingWhitespace(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/lines_test.go b/pkgtools/pkglint/files/lines_test.go index 42baed61c1fe..4264708db36b 100644 --- a/pkgtools/pkglint/files/lines_test.go +++ b/pkgtools/pkglint/files/lines_test.go @@ -2,6 +2,83 @@ package pkglint import "gopkg.in/check.v1" +func (s *Suite) Test_NewLines(c *check.C) { + t := s.Init(c) + + lines := t.NewLines("filename", + "text") + + t.CheckEquals(lines.Filename, NewCurrPathString("filename")) +} + +func (s *Suite) Test_Lines_Len(c *check.C) { + t := s.Init(c) + + lines := t.NewLines("filename", + "one", + "two", + "three") + + t.CheckEquals(lines.Len(), 3) +} + +func (s *Suite) Test_Lines_LastLine(c *check.C) { + t := s.Init(c) + + lines := t.NewLines("filename", + "text") + + whole := lines.LastLine() + + t.CheckEquals(whole.String(), "filename:1: text") +} + +func (s *Suite) Test_Lines_EOFLine(c *check.C) { + t := s.Init(c) + + lines := t.NewLines("filename", + "text") + + whole := lines.EOFLine() + + // The text of the line after the ': ' is empty. + t.CheckEquals(whole.String(), "filename:EOF: ") +} + +func (s *Suite) Test_Lines_Whole(c *check.C) { + t := s.Init(c) + + lines := t.NewLines("filename", + "text") + + whole := lines.Whole() + + // The lineno between the '::' is empty. + // The text of the line after the ': ' is empty as well. + t.CheckEquals(whole.String(), "filename:: ") +} + +func (s *Suite) Test_Lines_SaveAutofixChanges(c *check.C) { + t := s.Init(c) + + doTest := func(autofix bool) { + lines := t.SetUpFileLines("filename", + "before") + + fix := lines.Lines[0].Autofix() + fix.Notef("Replacing.") + fix.Replace("before", "after") + fix.Apply() + + lines.SaveAutofixChanges() + } + + t.ExpectDiagnosticsAutofix( + doTest, + "NOTE: ~/filename:1: Replacing.", + "AUTOFIX: ~/filename:1: Replacing \"before\" with \"after\".") +} + func (s *Suite) Test_Lines_CheckCvsID(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/logging_test.go b/pkgtools/pkglint/files/logging_test.go index 4702902f9544..1a3115864a80 100644 --- a/pkgtools/pkglint/files/logging_test.go +++ b/pkgtools/pkglint/files/logging_test.go @@ -286,6 +286,48 @@ func (s *Suite) Test_Logger_Diag__source_duplicates(c *check.C) { "category/package1", "category/package2")) } +func (s *Suite) Test_Logger_FirstTime__not_verbose(c *check.C) { + t := s.Init(c) + + G.Logger.verbose = false // as in a realistic run + + t.CheckEquals(G.Logger.FirstTime("filename", "123", "Message."), true) + t.CheckEquals(G.Logger.FirstTime("filename", "123", "Message."), false) + t.CheckEquals(G.Logger.FirstTime("filename", "124", "Message."), true) + t.CheckEquals(G.Logger.FirstTime("filename", "124", "Message."), false) + t.CheckEquals(G.Logger.FirstTime("filename", "124", "Message."), false) +} + +func (s *Suite) Test_Logger_Relevant(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine(nil...) + + t.CheckEquals(G.Logger.Relevant("Options should not contain whitespace."), true) + t.CheckEquals(G.Logger.suppressDiag, false) + t.CheckEquals(G.Logger.suppressExpl, false) // XXX: Why not true? + + t.SetUpCommandLine("--only", "whitespace") + + t.CheckEquals(G.Logger.Relevant("Options should not contain whitespace."), true) + t.CheckEquals(G.Logger.suppressDiag, false) + t.CheckEquals(G.Logger.suppressExpl, false) // XXX: Why not true? + + t.CheckEquals(G.Logger.Relevant("Options should not contain space."), false) + t.CheckEquals(G.Logger.suppressDiag, true) + t.CheckEquals(G.Logger.suppressExpl, true) + + t.SetUpCommandLine("--explain") + + t.CheckEquals(G.Logger.Relevant("Options should not contain whitespace."), true) + t.CheckEquals(G.Logger.suppressDiag, false) + t.CheckEquals(G.Logger.suppressExpl, false) + + t.CheckEquals(G.Logger.Relevant("Options should not contain space."), true) + t.CheckEquals(G.Logger.suppressDiag, false) + t.CheckEquals(G.Logger.suppressExpl, false) +} + func (s *Suite) Test_Logger_shallBeLogged(c *check.C) { t := s.Init(c) @@ -642,6 +684,61 @@ func (s *Suite) Test_Logger_writeSource__first_warn_then_autofix(c *check.C) { "+\tThe last line") } +func (s *Suite) Test_Logger_writeDiff(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--show-autofix") + line := t.NewLine("filename", 123, "before") + fix := line.Autofix() + fix.Silent() + fix.Replace("before", "after") + fix.Apply() + + G.Logger.writeDiff(line) + + // The diff lines are indented with a tab so that the indentation + // from the actual lines is properly represented in the output. + // If a space had been used here instead of the tab, the output + // would become garbled. + t.CheckOutputLines( + "AUTOFIX: filename:123: Replacing \"before\" with \"after\".", + "-\tbefore", + "+\tafter") +} + +func (s *Suite) Test_Logger_writeLine(c *check.C) { + t := s.Init(c) + + G.Logger.writeLine("> ", "\u0007\u00FC text") + + t.CheckOutputLines( + "> text") +} + +func (s *Suite) Test_Logger_IsAutofix__default(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("-Wall") + + t.CheckEquals(G.Logger.IsAutofix(), false) +} + +func (s *Suite) Test_Logger_IsAutofix__show_autofix(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("-Wall", "--show-autofix") + + t.CheckEquals(G.Logger.IsAutofix(), true) +} + +func (s *Suite) Test_Logger_IsAutofix__autofix(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("-Wall", "--autofix") + + t.CheckEquals(G.Logger.IsAutofix(), true) +} + // Calling Logf without further preparation just logs the message. // Suppressing duplicate messages or filtering messages happens // in other methods of the Logger, namely Relevant, FirstTime, Diag. @@ -1162,6 +1259,58 @@ func (s *Suite) Test_SeparatorWriter(c *check.C) { t.CheckEquals(sb.String(), "a\nb\n\nc\n") } +func (s *Suite) Test_NewSeparatorWriter(c *check.C) { + t := s.Init(c) + + var sb strings.Builder + wr := NewSeparatorWriter(&sb) + + t.CheckEquals(wr.out, &sb) + t.CheckEquals(wr.state, uint8(3)) + t.CheckEquals(wr.line.Len(), 0) +} + +func (s *Suite) Test_SeparatorWriter_WriteLine(c *check.C) { + t := s.Init(c) + + var sb strings.Builder + wr := NewSeparatorWriter(&sb) + + wr.WriteLine("first") + wr.Separate() + + t.CheckEquals(sb.String(), "first\n") + + wr.WriteLine("second") + + t.CheckEquals(sb.String(), "first\n\nsecond\n") +} + +func (s *Suite) Test_SeparatorWriter_Write(c *check.C) { + t := s.Init(c) + + var sb strings.Builder + wr := NewSeparatorWriter(&sb) + + wr.Write("first") + wr.Write("") + + t.CheckEquals(wr.line.String(), "first") + + wr.Write("\n") + + t.CheckEquals(wr.line.String(), "") + t.CheckEquals(sb.String(), "first\n") + + wr.Separate() + + t.CheckEquals(sb.String(), "first\n") + + wr.WriteLine("second") + + t.CheckEquals(sb.String(), "first\n\nsecond\n") +} + func (s *Suite) Test_SeparatorWriter_Separate(c *check.C) { t := s.Init(c) @@ -1199,6 +1348,31 @@ func (s *Suite) Test_SeparatorWriter_Separate__at_the_beginning(c *check.C) { t.CheckEquals(sb.String(), "a\n") } +func (s *Suite) Test_SeparatorWriter_write(c *check.C) { + t := s.Init(c) + + var sb strings.Builder + wr := NewSeparatorWriter(&sb) + + t.CheckEquals(wr.state, uint8(3)) + + wr.write('a') + + t.CheckEquals(wr.state, uint8(1)) + + wr.write('\n') + + t.CheckEquals(wr.state, uint8(0)) + + wr.Separate() + + t.CheckEquals(wr.state, uint8(2)) + + wr.write('\n') + + t.CheckEquals(wr.state, uint8(3)) +} + func (s *Suite) Test_SeparatorWriter_Flush(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/makepat/pat_test.go b/pkgtools/pkglint/files/makepat/pat_test.go index 543c65105751..3b9ec9ae56b9 100644 --- a/pkgtools/pkglint/files/makepat/pat_test.go +++ b/pkgtools/pkglint/files/makepat/pat_test.go @@ -175,6 +175,7 @@ func Test_Intersect(t *testing.T) { {"N-*", "N-*", "N-*", true, true}, {"N-9.99.*", "N-[1-9].*", "", false, true}, {"N-9.99.*", "N-[1-9][0-9].*", "", false, false}, + {"*.c", "*.h", "", false, false}, } for _, tt := range tests { t.Run(tt.str, func(t *testing.T) { diff --git a/pkgtools/pkglint/files/mklinechecker.go b/pkgtools/pkglint/files/mklinechecker.go index 082a019549f5..c52228a28e38 100644 --- a/pkgtools/pkglint/files/mklinechecker.go +++ b/pkgtools/pkglint/files/mklinechecker.go @@ -112,7 +112,8 @@ func (ck MkLineChecker) checkTextWrksrcDotDot(text string) { func (ck MkLineChecker) checkTextRpath(text string) { mkline := ck.MkLine - if mkline.IsVarassign() && mkline.Varname() == "BUILDLINK_TRANSFORM" && + if mkline.IsVarassign() && + varnameBase(mkline.Varname()) == "BUILDLINK_TRANSFORM" && hasPrefix(mkline.Value(), "rm:") { return diff --git a/pkgtools/pkglint/files/mklinechecker_test.go b/pkgtools/pkglint/files/mklinechecker_test.go index a412a2cd1b5a..478eaa163b47 100644 --- a/pkgtools/pkglint/files/mklinechecker_test.go +++ b/pkgtools/pkglint/files/mklinechecker_test.go @@ -181,14 +181,18 @@ func (s *Suite) Test_MkLineChecker_checkTextRpath(c *check.C) { t.SetUpVartypes() mklines := t.NewMkLines("filename.mk", MkCvsID, - "BUILDLINK_TRANSFORM+=\trm:-Wl,-R/usr/lib", - "BUILDLINK_TRANSFORM+=\trm:-Wl,-rpath,/usr/lib", - "BUILDLINK_TRANSFORM+=\topt:-Wl,-rpath,/usr/lib") + "BUILDLINK_TRANSFORM+=\t\trm:-Wl,-R/usr/lib", + "BUILDLINK_TRANSFORM+=\t\trm:-Wl,-rpath,/usr/lib", + "BUILDLINK_TRANSFORM+=\t\topt:-Wl,-rpath,/usr/lib", + "BUILDLINK_TRANSFORM.pkgbase+=\trm:-Wl,-R/usr/lib", + "BUILDLINK_TRANSFORM.pkgbase+=\trm:-Wl,-rpath,/usr/lib", + "BUILDLINK_TRANSFORM.pkgbase+=\topt:-Wl,-rpath,/usr/lib") mklines.Check() t.CheckOutputLines( - "WARN: filename.mk:4: Please use ${COMPILER_RPATH_FLAG} instead of \"-Wl,-rpath,\".") + "WARN: filename.mk:4: Please use ${COMPILER_RPATH_FLAG} instead of \"-Wl,-rpath,\".", + "WARN: filename.mk:7: Please use ${COMPILER_RPATH_FLAG} instead of \"-Wl,-rpath,\".") } func (s *Suite) Test_MkLineChecker_checkVartype__simple_type(c *check.C) { diff --git a/pkgtools/pkglint/files/mklineparser_test.go b/pkgtools/pkglint/files/mklineparser_test.go index ee113e4ccdcb..8c7b255bc285 100644 --- a/pkgtools/pkglint/files/mklineparser_test.go +++ b/pkgtools/pkglint/files/mklineparser_test.go @@ -402,6 +402,9 @@ func (s *Suite) Test_MkLineParser_MatchVarassign(c *check.C) { "", "") + // A multi-line variable assignment that is commented out. + // The '#' from the continued lines are discarded since they are not + // part of the variable value. testLine( lines( "#VAR=\t\t\t\\", diff --git a/pkgtools/pkglint/files/package.go b/pkgtools/pkglint/files/package.go index 0e922768b361..d273ee7557fc 100644 --- a/pkgtools/pkglint/files/package.go +++ b/pkgtools/pkglint/files/package.go @@ -7,10 +7,6 @@ import ( "strings" ) -// TODO: What about package names that refer to other variables? -// TODO: Allow a hyphen in the middle of a version number. -const rePkgname = `^([\w\-.+]+)-([0-9][.0-9A-Z_a-z]*)$` - // Package is the pkgsrc package that is currently checked. // // Most of the information is loaded first, and after loading the actual checks take place. @@ -1266,7 +1262,7 @@ func (pkg *Package) determineEffectivePkgVars() { pkg.checkPkgnameRedundant(pkgnameLine, pkgname, distname) - if pkgname == "" && distnameLine != nil && !containsVarUse(distname) && !matches(distname, rePkgname) { + if pkgname == "" && distnameLine != nil && !containsVarUse(distname) && !matchesPkgname(distname) { distnameLine.Warnf("As DISTNAME is not a valid package name, please define the PKGNAME explicitly.") } @@ -1275,7 +1271,7 @@ func (pkg *Package) determineEffectivePkgVars() { } if effname != "" && !containsVarUse(effname) { - if m, m1, m2 := match2(effname, rePkgname); m { + if m, m1, m2 := matchPkgname(effname); m { pkg.EffectivePkgname = effname + pkg.nbPart() pkg.EffectivePkgnameLine = pkgnameLine pkg.EffectivePkgbase = m1 @@ -1284,7 +1280,7 @@ func (pkg *Package) determineEffectivePkgVars() { } if pkg.EffectivePkgnameLine == nil && distname != "" && !containsVarUse(distname) { - if m, m1, m2 := match2(distname, rePkgname); m { + if m, m1, m2 := matchPkgname(distname); m { pkg.EffectivePkgname = distname + pkg.nbPart() pkg.EffectivePkgnameLine = distnameLine pkg.EffectivePkgbase = m1 @@ -1371,7 +1367,7 @@ func (pkg *Package) checkPossibleDowngrade() { defer trace.Call0()() } - m, _, pkgversion := match2(pkg.EffectivePkgname, rePkgname) + m, _, pkgversion := matchPkgname(pkg.EffectivePkgname) if !m { return } @@ -1795,3 +1791,17 @@ func NewPlistContent() PlistContent { make(map[RelPath]*PlistLine), make(map[string]bool)} } + +// matchPkgname tests whether the string has the form of a package name that +// does not contain any variable expressions. +func matchPkgname(s string) (m bool, base string, version string) { + // TODO: Allow a hyphen in the middle of a version number. + return match2(s, `^([\w\-.+]+)-([0-9][.0-9A-Z_a-z]*)$`) +} + +// matchPkgname tests whether the string has the form of a package name that +// does not contain any variable expressions. +func matchesPkgname(s string) bool { + m, _, _ := matchPkgname(s) + return m +} diff --git a/pkgtools/pkglint/files/pkglint_test.go b/pkgtools/pkglint/files/pkglint_test.go index f73c397b698f..4983af4b02e0 100644 --- a/pkgtools/pkglint/files/pkglint_test.go +++ b/pkgtools/pkglint/files/pkglint_test.go @@ -515,7 +515,6 @@ func (s *Suite) Test_Pkglint_Check__invalid_files_before_import(c *check.C) { t.CheckOutputLines( "ERROR: ~/category/package/Makefile.orig: Must be cleaned up before committing the package.", "ERROR: ~/category/package/Makefile.rej: Must be cleaned up before committing the package.", - "ERROR: ~/category/package/Makefile~: Must be cleaned up before committing the package.", "ERROR: ~/category/package/work: Must be cleaned up before committing the package.") } @@ -1117,9 +1116,11 @@ func (s *Suite) Test_Pkglint_checkReg__unknown_file_in_patches(c *check.C) { "Patch files should be named \"patch-\", followed by letters, '-', '_', '.', and digits only.") } -func (s *Suite) Test_Pkglint_checkReg__patch_for_Makefile_fragment(c *check.C) { +func (s *Suite) Test_Pkglint_checkReg__patch_for_makefile_fragment(c *check.C) { t := s.Init(c) + // This is a patch file, and even though its name ends with ".mk", it must + // not be interpreted as a makefile fragment. t.CreateFileDummyPatch("category/package/patches/patch-compiler.mk") t.Chdir("category/package") @@ -1197,6 +1198,25 @@ func (s *Suite) Test_Pkglint_checkReg__wip_commit_message(c *check.C) { t.CheckOutputEmpty() } +func (s *Suite) Test_Pkglint_checkReg__file_ignored_by_CVS(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package") + t.Chdir("category/package") + t.CreateFileLines("PLIST", + PlistCvsID, + "bin/program") + t.CreateFileLines("PLIST.~1.7.~", + PlistCvsID, + "bin/program") + t.FinishSetUp() + + G.Check(".") + + // Files ending in "~" should be ignored since CVS ignores them as well. + t.CheckOutputEmpty() +} + func (s *Suite) Test_Pkglint_checkRegCvsSubst(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/pkgsrc.go b/pkgtools/pkglint/files/pkgsrc.go index cbd3bb73e965..e131318fc233 100644 --- a/pkgtools/pkglint/files/pkgsrc.go +++ b/pkgtools/pkglint/files/pkgsrc.go @@ -471,7 +471,7 @@ func (src *Pkgsrc) parseSuggestedUpdates(lines *Lines) []SuggestedUpdate { llex.Skip() if m, pkgname, comment := match2(text, `^\to[\t ]([^\t ]+)(?:[\t ]*(.+))?$`); m { - if m, pkgbase, pkgversion := match2(pkgname, rePkgname); m { + if m, pkgbase, pkgversion := matchPkgname(pkgname); m { if hasPrefix(comment, "[") && hasSuffix(comment, "]") { comment = comment[1 : len(comment)-1] } diff --git a/pkgtools/pkglint/files/pkgver/vercmp_test.go b/pkgtools/pkglint/files/pkgver/vercmp_test.go index a3ecd8066c42..43ac7dfce0df 100644 --- a/pkgtools/pkglint/files/pkgver/vercmp_test.go +++ b/pkgtools/pkglint/files/pkgver/vercmp_test.go @@ -35,6 +35,8 @@ func (s *Suite) Test_Compare(c *check.C) { {"5.0"}, {"5.0nb5"}, {"5.5", "5.005"}, + {"2021.06.17", "2021.6.17"}, + {"2021.12.01", "2021.12.1"}, {"20151110"}, } diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go index 478dc1ff302a..ca24e48f3c38 100644 --- a/pkgtools/pkglint/files/util.go +++ b/pkgtools/pkglint/files/util.go @@ -341,7 +341,9 @@ func isIgnoredFilename(filename string) bool { case "CVS", ".svn", ".git", ".hg", ".idea": return true } - return hasPrefix(filename, ".#") + + // https://www.gnu.org/software/trans-coord/manual/cvs/cvs.html#cvsignore + return hasPrefix(filename, ".#") || hasSuffix(filename, "~") } // Checks whether a file is already committed to the CVS repository. diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go index 812d419ad3b9..53c4ee8e26e3 100644 --- a/pkgtools/pkglint/files/util_test.go +++ b/pkgtools/pkglint/files/util_test.go @@ -214,6 +214,12 @@ func (s *Suite) Test_isIgnoredFilename(c *check.C) { // There is actually an IDEA plugin for pkgsrc. // See https://github.com/rillig/intellij-pkgsrc. test(".idea", true) + + // After editing a file, run 'cvs up -CA'. This creates a backup. + test(".#Makefile.1.689", true) + + // https://www.gnu.org/software/trans-coord/manual/cvs/cvs.html#cvsignore + test("PLIST.~1.7.~", true) } func (s *Suite) Test_isLocallyModified(c *check.C) { diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go index ad6c7d054466..393bf7449199 100644 --- a/pkgtools/pkglint/files/vartypecheck.go +++ b/pkgtools/pkglint/files/vartypecheck.go @@ -1054,7 +1054,7 @@ func (cv *VartypeCheck) Perms() { func (cv *VartypeCheck) Pkgname() { value := cv.Value - if cv.Op != opUseMatch && value == cv.ValueNoVar && !matches(value, rePkgname) { + if cv.Op != opUseMatch && value == cv.ValueNoVar && !matchesPkgname(value) { cv.Warnf("%q is not a valid package name.", value) cv.Explain( "A valid package name has the form packagename-version, where version",