diff options
| author | rillig <rillig@pkgsrc.org> | 2020-01-04 19:53:13 +0000 |
|---|---|---|
| committer | rillig <rillig@pkgsrc.org> | 2020-01-04 19:53:13 +0000 |
| commit | dd58dfdd40fd5d6acfa623c6d5fac2ebf6ad6e61 (patch) | |
| tree | c812521a0125dd325b18bb8851c5ea71c369eff5 /pkgtools | |
| parent | f652d6e75417a495716ca3404de3ee4205f6fa62 (diff) | |
| download | pkgsrc-dd58dfdd40fd5d6acfa623c6d5fac2ebf6ad6e61.tar.gz | |
pkgtools/pkglint: update to 19.4.1
Changes since 19.4.0:
The notes for inserting an empty line have been changed from "insert
after this line" to "insert before this line" to make the line numbers
in the diagnostics contiguous. There had been several places where the
diagnostics went from line 1 to line 2 and then back to line 1, which
was confusing.
The lines in ALTERNATIVES files are checked for trailing whitespace.
This is only for consistency with the other checks. In the whole pkgsrc
tree all ALTERNATIVES files are already fine.
The diagnostics for comments in .endif/.endfor lines that don't
correspond to their .if/.elif/.for counterparts now includes the exact
line number of the corresponding condition, to make the warning easier
to fix.
The diagnostics for wrong variable value alignment now mention the
current column in addition to the desired column, to make it easier to
see by how much and in which direction the indentation should be fixed.
Variables that are used in conditions before they are actually defined
need the :U modifier.
Diffstat (limited to 'pkgtools')
69 files changed, 1349 insertions, 1012 deletions
diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index b72868caecd..f746b3d112a 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.621 2019/12/30 16:27:13 rillig Exp $ +# $NetBSD: Makefile,v 1.622 2020/01/04 19:53:13 rillig Exp $ -PKGNAME= pkglint-19.4.0 +PKGNAME= pkglint-19.4.1 CATEGORIES= pkgtools DISTNAME= tools MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/} diff --git a/pkgtools/pkglint/PLIST b/pkgtools/pkglint/PLIST index 955dcdfeb75..0acd59783d3 100644 --- a/pkgtools/pkglint/PLIST +++ b/pkgtools/pkglint/PLIST @@ -1,4 +1,4 @@ -@comment $NetBSD: PLIST,v 1.22 2019/12/15 01:29:06 rillig Exp $ +@comment $NetBSD: PLIST,v 1.23 2020/01/04 19:53:13 rillig Exp $ bin/pkglint gopkg/pkg/${GO_PLATFORM}/netbsd.org/pkglint.a gopkg/pkg/${GO_PLATFORM}/netbsd.org/pkglint/getopt.a @@ -29,7 +29,6 @@ gopkg/src/netbsd.org/pkglint/getopt/getopt.go gopkg/src/netbsd.org/pkglint/getopt/getopt_test.go gopkg/src/netbsd.org/pkglint/histogram/histogram.go gopkg/src/netbsd.org/pkglint/histogram/histogram_test.go -gopkg/src/netbsd.org/pkglint/intqa/ideas.go gopkg/src/netbsd.org/pkglint/intqa/qa.go gopkg/src/netbsd.org/pkglint/intqa/qa_test.go gopkg/src/netbsd.org/pkglint/licenses.go diff --git a/pkgtools/pkglint/files/alternatives.go b/pkgtools/pkglint/files/alternatives.go index 3ac2df943f6..4d3d8526a11 100644 --- a/pkgtools/pkglint/files/alternatives.go +++ b/pkgtools/pkglint/files/alternatives.go @@ -5,14 +5,14 @@ import ( "strings" ) -func CheckFileAlternatives(filename CurrPath) { +func CheckFileAlternatives(filename CurrPath, pkg *Package) { lines := Load(filename, NotEmpty|LogErrors) if lines == nil { return } var ck AlternativesChecker - ck.Check(lines, G.Pkg) + ck.Check(lines, pkg) } type AlternativesChecker struct{} @@ -24,15 +24,14 @@ func (ck *AlternativesChecker) Check(lines *Lines, pkg *Package) { } for _, line := range lines.Lines { - ck.checkLine(line, plistFiles) + ck.checkLine(line, plistFiles, pkg) } } // checkLine checks a single line for the following format: // wrapper alternative [optional arguments] -func (ck *AlternativesChecker) checkLine(line *Line, plistFiles map[RelPath]*PlistLine) { - // TODO: Add $ to the regex, just for confidence - m, wrapper, space, alternative := match3(line.Text, `^([^\t ]+)([ \t]+)([^\t ]+)`) +func (ck *AlternativesChecker) checkLine(line *Line, plistFiles map[RelPath]*PlistLine, pkg *Package) { + m, wrapper, space, alternative := match3(line.Text, `^([^\t ]+)([ \t]+)([^\t ]+).*$`) if !m { line.Errorf("Invalid line %q.", line.Text) line.Explain( @@ -44,10 +43,12 @@ func (ck *AlternativesChecker) checkLine(line *Line, plistFiles map[RelPath]*Pli ck.checkWrapperPlist(line, NewRelPathString(wrapper), plistFiles) } if plistFiles != nil { - ck.checkAlternativePlist(line, alternative, plistFiles) + ck.checkAlternativePlist(line, alternative, plistFiles, pkg) } ck.checkAlternativeAbs(alternative, line, space) + + LineChecker{line}.CheckTrailingWhitespace() } func (ck *AlternativesChecker) checkWrapperAbs(line *Line, wrapper Path) bool { @@ -85,7 +86,7 @@ func (ck *AlternativesChecker) checkAlternativeAbs(alternative string, line *Lin } func (ck *AlternativesChecker) checkAlternativePlist(line *Line, alternative string, - plistFiles map[RelPath]*PlistLine) { + plistFiles map[RelPath]*PlistLine, pkg *Package) { relImplementation := strings.Replace(alternative, "@PREFIX@/", "", 1) plistName := replaceAll(relImplementation, `@(\w+)@`, "${$1}") @@ -96,7 +97,7 @@ func (ck *AlternativesChecker) checkAlternativePlist(line *Line, alternative str } rel := NewRelPathString(plistName) - if plistFiles[rel] != nil || G.Pkg.vars.IsDefined("ALTERNATIVES_SRC") { + if plistFiles[rel] != nil || pkg.vars.IsDefined("ALTERNATIVES_SRC") { return } if plistFiles[rel.Replace("${PKGMANDIR}", "man")] != nil { diff --git a/pkgtools/pkglint/files/alternatives_test.go b/pkgtools/pkglint/files/alternatives_test.go index b5883982a47..eeda4d183de 100644 --- a/pkgtools/pkglint/files/alternatives_test.go +++ b/pkgtools/pkglint/files/alternatives_test.go @@ -8,9 +8,9 @@ func (s *Suite) Test_CheckFileAlternatives__empty(c *check.C) { t.Chdir("category/package") t.CreateFileLines("ALTERNATIVES") - G.Pkg = NewPackage(".") + pkg := NewPackage(".") - CheckFileAlternatives("ALTERNATIVES") + CheckFileAlternatives("ALTERNATIVES", pkg) t.CheckOutputLines( "ERROR: ALTERNATIVES: Must not be empty.") @@ -92,7 +92,7 @@ func (s *Suite) Test_AlternativesChecker_checkLine(c *check.C) { "bin/no-args @PREFIX@/bin/echo", "bin/with-args @PREFIX@/bin/echo hello,", "bin/with-quoted-args @PREFIX@/bin/echo \"hello, world\" \\ cowboy", - "bin/trailing @PREFIX@/bin/echo spaces ", // TODO: warn about this + "bin/trailing @PREFIX@/bin/echo spaces ", "/abs-echo @PREFIX@/bin/echo") t.CreateFileLines("PLIST", PlistCvsID, @@ -102,7 +102,8 @@ func (s *Suite) Test_AlternativesChecker_checkLine(c *check.C) { G.Check(".") t.CheckOutputLines( - "ERROR: ALTERNATIVES:5: Alternative wrapper \"/abs-echo\" " + + "NOTE: ALTERNATIVES:4: Trailing whitespace.", + "ERROR: ALTERNATIVES:5: Alternative wrapper \"/abs-echo\" "+ "must be relative to PREFIX.") } @@ -113,7 +114,7 @@ func (s *Suite) Test_AlternativesChecker_checkWrapperAbs(c *check.C) { "relative @PREFIX@/bin/echo", "/absolute @PREFIX@/bin/echo") - CheckFileAlternatives(t.File("ALTERNATIVES")) + CheckFileAlternatives(t.File("ALTERNATIVES"), nil) t.CheckOutputLines( "ERROR: ~/ALTERNATIVES:2: Alternative wrapper \"/absolute\" " + diff --git a/pkgtools/pkglint/files/autofix.go b/pkgtools/pkglint/files/autofix.go index 78b050d03bc..249b616d72c 100644 --- a/pkgtools/pkglint/files/autofix.go +++ b/pkgtools/pkglint/files/autofix.go @@ -142,13 +142,6 @@ func (fix *Autofix) ReplaceAt(rawIndex int, textIndex int, from string, to strin assert(from != to) fix.assertRealLine() - // XXX: This should only affect the diagnostics, but not the modifications - // to the text of the affected line, since that text will be used in - // further checks. - if fix.skip() { - return - } - rawLine := fix.line.raw[rawIndex] assert(textIndex < len(rawLine.textnl)) assert(hasPrefix(rawLine.textnl[textIndex:], from)) @@ -166,6 +159,9 @@ func (fix *Autofix) ReplaceAt(rawIndex int, textIndex int, from string, to strin // This probably requires a generic notification mechanism. _, fix.line.Text = replaceOnce(fix.line.Text, from, to) + if fix.skip() { + return + } fix.Describef(rawLine.Lineno, "Replacing %q with %q.", from, to) } diff --git a/pkgtools/pkglint/files/autofix_test.go b/pkgtools/pkglint/files/autofix_test.go index 303060a63b9..bf86e136d2e 100644 --- a/pkgtools/pkglint/files/autofix_test.go +++ b/pkgtools/pkglint/files/autofix_test.go @@ -341,6 +341,32 @@ func (s *Suite) Test_Autofix__noop_replace(c *check.C) { "WARN: Makefile:14: The word ABC should not be used.") } +// Contrary to Line.Autofix(), the NewAutofix constructor does not check +// whether the previous autofix is already finished, since it cannot know. +func (s *Suite) Test_NewAutofix(c *check.C) { + t := s.Init(c) + + line := t.NewLine("filename.mk", 123, "") + + fix := NewAutofix(line) + fix2 := NewAutofix(line) + + t.Check(fix2, check.Not(check.Equals), fix) +} + +func (s *Suite) Test_Autofix_Errorf(c *check.C) { + t := s.Init(c) + + line := t.NewLine("DESCR", 1, "Description of the package") + + fix := line.Autofix() + fix.Errorf("Error.") + fix.Apply() + + t.CheckOutputLines( + "ERROR: DESCR:1: Error.") +} + func (s *Suite) Test_Autofix_Warnf__duplicate(c *check.C) { t := s.Init(c) @@ -351,6 +377,19 @@ func (s *Suite) Test_Autofix_Warnf__duplicate(c *check.C) { t.ExpectAssert(func() { fix.Warnf("Warning 2.") }) } +func (s *Suite) Test_Autofix_Notef(c *check.C) { + t := s.Init(c) + + line := t.NewLine("DESCR", 1, "Description of the package") + + fix := line.Autofix() + fix.Notef("Note.") + fix.Apply() + + t.CheckOutputLines( + "NOTE: DESCR:1: Note.") +} + func (s *Suite) Test_Autofix_Explain__without_explain_option(c *check.C) { t := s.Init(c) @@ -465,6 +504,23 @@ func (s *Suite) Test_Autofix_Explain__silent_with_diagnostic(c *check.C) { t.CheckEquals(fix.RawText(), "before\nText\nafter\n") } +func (s *Suite) Test_Autofix_Replace(c *check.C) { + t := s.Init(c) + + doTest := func(bool) { + line := t.NewLine("filename.mk", 123, "text") + fix := line.Autofix() + fix.Warnf("Warning.") + fix.Replace("text", "replacement") + fix.Apply() + } + + t.ExpectDiagnosticsAutofix( + doTest, + "WARN: filename.mk:123: Warning.", + "AUTOFIX: filename.mk:123: Replacing \"text\" with \"replacement\".") +} + func (s *Suite) Test_Autofix_ReplaceAfter__autofix_in_continuation_line(c *check.C) { t := s.Init(c) @@ -708,6 +764,24 @@ func (s *Suite) Test_Autofix_InsertBefore(c *check.C) { ">\toriginal") } +func (s *Suite) Test_Autofix_InsertAfter(c *check.C) { + t := s.Init(c) + + doTest := func(bool) { + line := t.NewLine("DESCR", 1, "Description of the package") + + fix := line.Autofix() + fix.Errorf("Error.") + fix.InsertAfter("after") + fix.Apply() + } + + t.ExpectDiagnosticsAutofix( + doTest, + "ERROR: DESCR:1: Error.", + "AUTOFIX: DESCR:1: Inserting a line \"after\" after this line.") +} + func (s *Suite) Test_Autofix_Delete(c *check.C) { t := s.Init(c) @@ -821,6 +895,29 @@ func (s *Suite) Test_Autofix_Custom__in_memory(c *check.C) { t.CheckEquals(lines.Lines[2].Text, "LINE3") } +func (s *Suite) Test_Autofix_Describef(c *check.C) { + t := s.Init(c) + + doTest := func(bool) { + line := t.NewLine("DESCR", 1, "Description of the package") + + fix := line.Autofix() + fix.Errorf("Error.") + fix.Custom(func(showAutofix, autofix bool) { + fix.Describef(123, "Masking.") + raw := line.raw[0] + raw.textnl = replaceAll(raw.Text(), `\p{L}`, "*") + "\n" + }) + fix.Apply() + t.CheckEquals(line.raw[0].Text(), "*********** ** *** *******") + } + + t.ExpectDiagnosticsAutofix( + doTest, + "ERROR: DESCR:123: Error.", + "AUTOFIX: DESCR:123: Masking.") +} + // With the default command line options, this warning is printed. // With the --show-autofix option this warning is NOT printed since it // cannot be fixed automatically. @@ -1051,30 +1148,6 @@ func (s *Suite) Test_Autofix_Apply__source_without_explain(c *check.C) { // After fixing part of a line, the whole line needs to be parsed again. // // As of May 2019, this is not done yet. -func (s *Suite) Test_Autofix_Apply__text_after_replacing_string(c *check.C) { - t := s.Init(c) - - t.SetUpCommandLine("-Wall", "--autofix") - mkline := t.NewMkLine("filename.mk", 123, "VAR=\tvalue") - - fix := mkline.Autofix() - fix.Notef("Just a demo.") - fix.Replace("value", "new value") - fix.Apply() - - t.CheckOutputLines( - "AUTOFIX: filename.mk:123: Replacing \"value\" with \"new value\".") - - t.CheckEquals(mkline.raw[0].textnl, "VAR=\tnew value\n") - t.CheckEquals(mkline.raw[0].orignl, "VAR=\tvalue\n") - t.CheckEquals(mkline.Text, "VAR=\tnew value") - // TODO: should be updated as well. - t.CheckEquals(mkline.Value(), "value") -} - -// After fixing part of a line, the whole line needs to be parsed again. -// -// As of May 2019, this is not done yet. func (s *Suite) Test_Autofix_Apply__text_after_replacing(c *check.C) { t := s.Init(c) @@ -1202,7 +1275,7 @@ func (s *Suite) Test_Autofix_skip(c *check.C) { "VAR=\t111 222 333 444 555 \\", "666") t.CheckEquals(fix.RawText(), ""+ - "VAR=\t111 222 333 444 555 \\\n"+ + "NEW=\t111 222 333 444 555 \\\n"+ "666\n") } diff --git a/pkgtools/pkglint/files/buildlink3.go b/pkgtools/pkglint/files/buildlink3.go index a59df037849..58d4a21c8c1 100644 --- a/pkgtools/pkglint/files/buildlink3.go +++ b/pkgtools/pkglint/files/buildlink3.go @@ -62,8 +62,9 @@ func (ck *Buildlink3Checker) Check() { llex.CurrentLine().Warnf("The file should end here.") } - if G.Pkg != nil { - G.Pkg.checkLinesBuildlink3Inclusion(mklines) + pkg := ck.mklines.pkg + if pkg != nil { + pkg.checkLinesBuildlink3Inclusion(mklines) } mklines.SaveAutofixChanges() @@ -81,8 +82,8 @@ func (ck *Buildlink3Checker) checkFirstParagraph(mlex *MkLinesLexer) bool { pkgbase := m[1] pkgbaseLine := mlex.PreviousMkLine() - if containsVarRef(pkgbase) { - ck.checkVaruseInPkgbase(pkgbase, pkgbaseLine) + if containsVarUse(pkgbase) { + ck.checkVaruseInPkgbase(pkgbaseLine) } ck.checkUniquePkgbase(pkgbase, pkgbaseLine) @@ -132,7 +133,7 @@ func (ck *Buildlink3Checker) checkSecondParagraph(mlex *MkLinesLexer) bool { // See pkgtools/createbuildlink/files/createbuildlink, keyword PKGUPPER ucPkgbase := strings.ToUpper(strings.Replace(pkgbase, "-", "_", -1)) - if ucPkgbase != pkgupper && !containsVarRef(pkgbase) { + if ucPkgbase != pkgupper && !containsVarUse(pkgbase) { pkgupperLine.Errorf("Package name mismatch between multiple-inclusion guard %q (expected %q) and package name %q (from %s).", pkgupper, ucPkgbase, pkgbase, pkgupperLine.RelMkLine(ck.pkgbaseLine)) } @@ -142,11 +143,12 @@ func (ck *Buildlink3Checker) checkSecondParagraph(mlex *MkLinesLexer) bool { } func (ck *Buildlink3Checker) checkPkgbaseMismatch(bl3base string) { - if G.Pkg == nil { + pkg := ck.mklines.pkg + if pkg == nil { return } - mkbase := G.Pkg.EffectivePkgbase + mkbase := pkg.EffectivePkgbase if mkbase == "" || mkbase == bl3base || strings.TrimPrefix(mkbase, "lib") == bl3base { return } @@ -156,7 +158,7 @@ func (ck *Buildlink3Checker) checkPkgbaseMismatch(bl3base string) { } ck.pkgbaseLine.Errorf("Package name mismatch between %q in this file and %q from %s.", - bl3base, mkbase, ck.pkgbaseLine.RelMkLine(G.Pkg.EffectivePkgnameLine)) + bl3base, mkbase, ck.pkgbaseLine.RelMkLine(pkg.EffectivePkgnameLine)) } // Third paragraph: Package information. @@ -172,7 +174,7 @@ func (ck *Buildlink3Checker) checkMainPart(mlex *MkLinesLexer) bool { switch { case mkline.IsVarassign(): - ck.checkVarassign(mlex, mkline, pkgbase) + ck.checkVarassign(mkline, pkgbase) case mkline.IsDirective() && mkline.Directive() == "if": indentLevel++ @@ -193,7 +195,7 @@ func (ck *Buildlink3Checker) checkMainPart(mlex *MkLinesLexer) bool { return true } -func (ck *Buildlink3Checker) checkVarassign(mlex *MkLinesLexer, mkline *MkLine, pkgbase string) { +func (ck *Buildlink3Checker) checkVarassign(mkline *MkLine, pkgbase string) { varname, value := mkline.Varname(), mkline.Value() doCheck := false @@ -223,8 +225,8 @@ func (ck *Buildlink3Checker) checkVarassign(mlex *MkLinesLexer, mkline *MkLine, } if doCheck { - if ck.abi != nil && ck.abi.Lower != "" && !containsVarRef(ck.abi.Lower) { - if ck.api != nil && ck.api.Lower != "" && !containsVarRef(ck.api.Lower) { + if ck.abi != nil && ck.abi.Lower != "" && !containsVarUse(ck.abi.Lower) { + if ck.api != nil && ck.api.Lower != "" && !containsVarUse(ck.api.Lower) { if pkgver.Compare(ck.abi.Lower, ck.api.Lower) < 0 { ck.abiLine.Warnf("ABI version %q should be at least API version %q (see %s).", ck.abi.Lower, ck.api.Lower, ck.abiLine.RelMkLine(ck.apiLine)) @@ -240,7 +242,7 @@ func (ck *Buildlink3Checker) checkVarassign(mlex *MkLinesLexer, mkline *MkLine, } } -func (ck *Buildlink3Checker) checkVaruseInPkgbase(pkgbase string, pkgbaseLine *MkLine) { +func (ck *Buildlink3Checker) checkVaruseInPkgbase(pkgbaseLine *MkLine) { tokens, _ := pkgbaseLine.ValueTokens() for _, token := range tokens { if token.Varuse == nil { diff --git a/pkgtools/pkglint/files/buildlink3_test.go b/pkgtools/pkglint/files/buildlink3_test.go index a6c409288d1..220953d17bf 100644 --- a/pkgtools/pkglint/files/buildlink3_test.go +++ b/pkgtools/pkglint/files/buildlink3_test.go @@ -600,10 +600,9 @@ func (s *Suite) Test_Buildlink3Checker_checkMainPart__if_else_endif(c *check.C) t.CheckOutputEmpty() } -// Since the buildlink3 checker does not use MkLines.ForEach, it has to keep -// track of the nesting depth of .if directives. -// -// TODO: Use MkLines.ForEach. +// The buildlink3 checker does not use MkLines.ForEach since that would make +// the code more difficult to understand. Without MkLines.ForEach, it has to +// keep track of the nesting depth of .if directives itself. func (s *Suite) Test_Buildlink3Checker_checkMainPart__nested_if(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/category.go b/pkgtools/pkglint/files/category.go index 54336461f7e..2fceb4be3be 100644 --- a/pkgtools/pkglint/files/category.go +++ b/pkgtools/pkglint/files/category.go @@ -7,7 +7,7 @@ func CheckdirCategory(dir CurrPath) { defer trace.Call(dir)() } - mklines := LoadMk(dir.JoinNoClean("Makefile"), NotEmpty|LogErrors) // TODO: Remove the "./" here already + mklines := LoadMk(dir.JoinNoClean("Makefile").CleanDot(), nil, NotEmpty|LogErrors) if mklines == nil { return } @@ -46,30 +46,37 @@ func CheckdirCategory(dir CurrPath) { fSubdirs := getSubdirs(dir) var mSubdirs []subdir - seen := make(map[string]*MkLine) + seen := make(map[RelPath]*MkLine) for !mlex.EOF() { mkline := mlex.CurrentMkLine() if mkline.IsVarassignMaybeCommented() && mkline.Varname() == "SUBDIR" { mlex.Skip() - name := mkline.Value() // TODO: Maybe NewPath here already + value := NewPath(mkline.Value()) + if value.IsAbs() { + mkline.Errorf("%q must be a relative path.", value.String()) + continue + } + sub := NewRelPath(value) + if mkline.IsCommentedVarassign() && !mkline.HasComment() { - mkline.Warnf("%q commented out without giving a reason.", name) + mkline.Warnf("%q commented out without giving a reason.", sub) } - if prev := seen[name]; prev != nil { - mkline.Errorf("%q must only appear once, already seen in %s.", name, mkline.RelMkLine(prev)) + if prev := seen[sub]; prev != nil { + mkline.Errorf("%q must only appear once, already seen in %s.", + sub, mkline.RelMkLine(prev)) } - seen[name] = mkline + seen[sub] = mkline if len(mSubdirs) > 0 { - if prev := mSubdirs[len(mSubdirs)-1].name; name < prev.String() { - mkline.Warnf("%q should come before %q.", name, prev) + if prev := mSubdirs[len(mSubdirs)-1].name; sub < prev { + mkline.Warnf("%q should come before %q.", sub, prev) } } - mSubdirs = append(mSubdirs, subdir{NewRelPathString(name), mkline}) + mSubdirs = append(mSubdirs, subdir{sub, mkline}) } else { if !mkline.IsEmpty() { diff --git a/pkgtools/pkglint/files/category_test.go b/pkgtools/pkglint/files/category_test.go index 262ab2358a9..adaece865e3 100644 --- a/pkgtools/pkglint/files/category_test.go +++ b/pkgtools/pkglint/files/category_test.go @@ -20,18 +20,18 @@ func (s *Suite) Test_CheckdirCategory__totally_broken(c *check.C) { "ERROR: ~/archivers/Makefile:1: Expected \"# $"+"NetBSD$\".", "WARN: ~/archivers/Makefile:4: Line contains invalid characters (U+2019).", "WARN: ~/archivers/Makefile:4: SUBDIR- is defined but not used.", - "NOTE: ~/archivers/Makefile:2: This variable value should be aligned to column 17.", - "NOTE: ~/archivers/Makefile:3: This variable value should be aligned with tabs, not spaces, to column 17.", - "NOTE: ~/archivers/Makefile:4: This variable value should be aligned to column 17.", + "NOTE: ~/archivers/Makefile:2: This variable value should be aligned to column 17 instead of 9.", + "NOTE: ~/archivers/Makefile:3: This variable value should be aligned with tabs, not spaces, to column 17 instead of 10.", + "NOTE: ~/archivers/Makefile:4: This variable value should be aligned to column 17 instead of 9.", "ERROR: ~/archivers/Makefile:6: Relative path \"../mk/category.mk\" does not exist.", - "NOTE: ~/archivers/Makefile:1: Empty line expected after this line.", + "NOTE: ~/archivers/Makefile:2: Empty line expected before this line.", "ERROR: ~/archivers/Makefile:2: COMMENT= line expected.", - "NOTE: ~/archivers/Makefile:1: Empty line expected after this line.", // XXX: Duplicate. + "NOTE: ~/archivers/Makefile:2: Empty line expected before this line.", "WARN: ~/archivers/Makefile:3: \"aaaaa\" should come before \"pkg1\".", "ERROR: ~/archivers/Makefile:4: SUBDIR+= line or empty line expected.", "ERROR: ~/archivers/Makefile:2: \"pkg1\" exists in the Makefile but not in the file system.", "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.", + "NOTE: ~/archivers/Makefile:4: Empty line expected before this line.", "WARN: ~/archivers/Makefile:4: This line should contain the following text: .include \"../mk/misc/category.mk\"", "ERROR: ~/archivers/Makefile:4: The file must end here.") } @@ -270,7 +270,8 @@ func (s *Suite) Test_CheckdirCategory__indentation(c *check.C) { CheckdirCategory(t.File("category")) t.CheckOutputLines( - "NOTE: ~/category/Makefile:5: This variable value should be aligned with tabs, not spaces, to column 17.") + "NOTE: ~/category/Makefile:5: This variable value should be aligned " + + "with tabs, not spaces, to column 17 instead of 29.") } func (s *Suite) Test_CheckdirCategory__comment_at_the_top(c *check.C) { @@ -300,10 +301,10 @@ func (s *Suite) Test_CheckdirCategory__comment_at_the_top(c *check.C) { // is rather exotic anyway. t.CheckOutputLines( "ERROR: ~/category/Makefile:3: COMMENT= line expected.", - "NOTE: ~/category/Makefile:2: Empty line expected after this line.", + "NOTE: ~/category/Makefile:3: Empty line expected before this line.", "ERROR: ~/category/Makefile:3: SUBDIR+= line or empty line expected.", "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.", + "NOTE: ~/category/Makefile:3: Empty line expected before this line.", "WARN: ~/category/Makefile:3: This line should contain the following text: .include \"../mk/misc/category.mk\"", "ERROR: ~/category/Makefile:3: The file must end here.") } @@ -368,3 +369,50 @@ func (s *Suite) Test_CheckdirCategory__case_mismatch(c *check.C) { "ERROR: ~/category/Makefile:5: \"p5-net-dns\" "+ "exists in the Makefile but not in the file system.") } + +func (s *Suite) Test_CheckdirCategory__dot(c *check.C) { + t := s.Init(c) + + t.SetUpPkgsrc() + t.CreateFileLines("mk/misc/category.mk") + t.CreateFileLines("category/Makefile", + MkCvsID, + "", + "COMMENT=\tCategory comment", + "", + "SUBDIR+=\tpackage", + "", + ".include \"../mk/misc/category.mk\"") + t.Chdir("category") + t.FinishSetUp() + + G.Check(".") + + t.CheckOutputLines( + "ERROR: Makefile:5: \"package\" exists in the Makefile " + + "but not in the file system.") +} + +func (s *Suite) Test_CheckdirCategory__absolute(c *check.C) { + t := s.Init(c) + + t.SetUpPkgsrc() + t.CreateFileLines("mk/misc/category.mk") + t.CreateFileLines("category/Makefile", + MkCvsID, + "", + "COMMENT=\tCategory comment", + "", + "SUBDIR+=\t/other", + "", + ".include \"../mk/misc/category.mk\"") + t.Chdir("category") + t.FinishSetUp() + + G.Check(".") + + t.CheckOutputLines( + "WARN: Makefile:5: The filename \"/other\" "+ + "contains the invalid character \"/\".", + "ERROR: Makefile:5: \"/other\" must be a relative path.") +} diff --git a/pkgtools/pkglint/files/check_test.go b/pkgtools/pkglint/files/check_test.go index e96c133ce18..a9b75f48681 100644 --- a/pkgtools/pkglint/files/check_test.go +++ b/pkgtools/pkglint/files/check_test.go @@ -104,7 +104,6 @@ func (s *Suite) TearDownTest(c *check.C) { 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 ck.Configure("distinfo.go", "*", "*", -intqa.EMissingTest) // TODO ck.Configure("files.go", "*", "*", -intqa.EMissingTest) // TODO @@ -300,8 +299,12 @@ func (t *Tester) SetUpFileLines(filename RelPath, lines ...string) *Lines { // // If the filename is irrelevant for the particular test, take filename.mk. func (t *Tester) SetUpFileMkLines(filename RelPath, lines ...string) *MkLines { + return t.SetUpFileMkLinesPkg(filename, nil, lines...) +} + +func (t *Tester) SetUpFileMkLinesPkg(filename RelPath, pkg *Package, lines ...string) *MkLines { abs := t.CreateFileLines(filename, lines...) - return LoadMk(abs, MustSucceed) + return LoadMk(abs, pkg, MustSucceed) } // LoadMkInclude loads the given Makefile fragment and all the files it includes, @@ -313,11 +316,11 @@ func (t *Tester) LoadMkInclude(filename RelPath) *MkLines { // TODO: Include files with multiple-inclusion guard only once. // TODO: Include files without multiple-inclusion guard as often as needed. - // TODO: Set an upper limit, to prevent denial of service. var load func(filename CurrPath) load = func(filename CurrPath) { - for _, mkline := range NewMkLines(Load(filename, MustSucceed)).mklines { + mklines := NewMkLines(Load(filename, MustSucceed), nil) + for _, mkline := range mklines.mklines { lines = append(lines, mkline.Line) if mkline.IsInclude() { @@ -330,7 +333,7 @@ func (t *Tester) LoadMkInclude(filename RelPath) *MkLines { // This assumes that the test files do not contain parse errors. // Otherwise the diagnostics would appear twice. - return NewMkLines(NewLines(t.File(filename), lines)) + return NewMkLines(NewLines(t.File(filename), lines), nil) } // SetUpPkgsrc sets up a minimal but complete pkgsrc installation in the @@ -747,7 +750,7 @@ func (t *Tester) SetUpHierarchy() ( } } - mklines := NewMkLines(NewLines(NewCurrPath(relFilename.AsPath()), lines)) + mklines := NewMkLines(NewLines(NewCurrPath(relFilename.AsPath()), lines), nil) assertf(files[filename] == nil, "MkLines with name %q already exists.", filename) files[filename] = mklines return mklines @@ -1053,6 +1056,10 @@ func (t *Tester) NewLinesAt(filename CurrPath, firstLine int, texts ...string) * // // If the filename is irrelevant for the particular test, take filename.mk. func (t *Tester) NewMkLines(filename CurrPath, lines ...string) *MkLines { + return t.NewMkLinesPkg(filename, nil, lines...) +} + +func (t *Tester) NewMkLinesPkg(filename CurrPath, pkg *Package, lines ...string) *MkLines { basename := filename.Base() assertf( hasSuffix(basename, ".mk") || basename == "Makefile" || hasPrefix(basename, "Makefile."), @@ -1063,7 +1070,7 @@ func (t *Tester) NewMkLines(filename CurrPath, lines ...string) *MkLines { rawText.WriteString(line) rawText.WriteString("\n") } - return NewMkLines(convertToLogicalLines(filename, rawText.String(), true)) + return NewMkLines(convertToLogicalLines(filename, rawText.String(), true), pkg) } // Returns and consumes the output from both stdout and stderr. diff --git a/pkgtools/pkglint/files/distinfo.go b/pkgtools/pkglint/files/distinfo.go index 1f6ba51780c..fcb5793cef8 100644 --- a/pkgtools/pkglint/files/distinfo.go +++ b/pkgtools/pkglint/files/distinfo.go @@ -51,7 +51,7 @@ func (ck *distinfoLinesChecker) parse() { lines := ck.lines llex := NewLinesLexer(lines) - if lines.CheckCvsID(0, ``, "") { + if !llex.EOF() && lines.CheckCvsID(0, ``, "") { llex.Skip() } llex.SkipEmptyOrNote() diff --git a/pkgtools/pkglint/files/distinfo_test.go b/pkgtools/pkglint/files/distinfo_test.go index d83a641782c..70faa874974 100644 --- a/pkgtools/pkglint/files/distinfo_test.go +++ b/pkgtools/pkglint/files/distinfo_test.go @@ -21,9 +21,9 @@ func (s *Suite) Test_CheckLinesDistinfo__parse_errors(c *check.C) { "SHA1 (patch-ab) = 6b98dd609f85a9eb9c4c1e4e7055a6aaa62b7cc7", "Another invalid line", "SHA1 (patch-nonexistent) = 1234") - G.Pkg = NewPackage(".") + pkg := NewPackage(".") - CheckLinesDistinfo(G.Pkg, lines) + CheckLinesDistinfo(pkg, lines) t.CheckOutputLines( "ERROR: distinfo:1: Expected \"$"+"NetBSD$\".", @@ -36,7 +36,7 @@ func (s *Suite) Test_CheckLinesDistinfo__parse_errors(c *check.C) { "WARN: distinfo:9: Patch file \"patch-nonexistent\" does not exist in directory \"patches\".") } -func (s *Suite) Test_distinfoLinesChecker_parse__empty(c *check.C) { +func (s *Suite) Test_distinfoLinesChecker_parse__trailing_empty_line(c *check.C) { t := s.Init(c) lines := t.SetUpFileLines("distinfo", @@ -49,6 +49,46 @@ func (s *Suite) Test_distinfoLinesChecker_parse__empty(c *check.C) { "NOTE: ~/distinfo:2: Trailing empty lines.") } +func (s *Suite) Test_distinfoLinesChecker_parse__empty_file(c *check.C) { + t := s.Init(c) + + lines := t.SetUpFileLines("distinfo", + CvsID) + + CheckLinesDistinfo(nil, lines) + + t.CheckOutputLines( + "NOTE: ~/distinfo:1: Empty line expected after this line.") +} + +func (s *Suite) Test_distinfoLinesChecker_parse__commented_first_line(c *check.C) { + t := s.Init(c) + + // This mismatch can happen for inexperienced pkgsrc users. + // It's not easy to keep all these different file types apart. + lines := t.SetUpFileLines("distinfo", + PlistCvsID) + + CheckLinesDistinfo(nil, lines) + + t.CheckOutputLines( + "ERROR: ~/distinfo:1: Expected \""+CvsID+"\".", + "NOTE: ~/distinfo:1: Empty line expected before this line.", + "ERROR: ~/distinfo:1: Invalid line: "+PlistCvsID) +} + +func (s *Suite) Test_distinfoLinesChecker_parse__completely_empty_file(c *check.C) { + t := s.Init(c) + + lines := t.SetUpFileLines("distinfo", + nil...) + + CheckLinesDistinfo(nil, lines) + + t.CheckOutputLines( + "NOTE: ~/distinfo:EOF: Empty line expected before this line.") +} + // When the distinfo file and the patches are placed in the same package, // their diagnostics use short relative paths. func (s *Suite) Test_distinfoLinesChecker_check__distinfo_and_patches_in_separate_directory(c *check.C) { @@ -96,9 +136,9 @@ func (s *Suite) Test_distinfoLinesChecker_check__manual_patches(c *check.C) { // the PATCHDIR is not known and therefore no diagnostics are logged. t.CheckOutputEmpty() - G.Pkg = NewPackage(".") + pkg := NewPackage(".") - CheckLinesDistinfo(G.Pkg, lines) + CheckLinesDistinfo(pkg, lines) // When a distinfo file is checked in the context of a package, // the PATCHDIR is known, therefore the check is active. @@ -169,9 +209,9 @@ func (s *Suite) Test_distinfoLinesChecker_checkAlgorithms__nonexistent_distfile_ "", "MD5 (patch-5.3.tar.gz) = 12345678901234567890123456789012", "SHA1 (patch-5.3.tar.gz) = 1234567890123456789012345678901234567890") - G.Pkg = NewPackage(".") + pkg := NewPackage(".") - CheckLinesDistinfo(G.Pkg, lines) + CheckLinesDistinfo(pkg, lines) // Even though the filename starts with "patch-" and therefore looks like // a patch, it is a normal distfile because it has other hash algorithms @@ -760,10 +800,10 @@ func (s *Suite) Test_distinfoLinesChecker_checkPatchSha1__relative_path_in_disti func (s *Suite) Test_distinfoLinesChecker_checkPatchSha1(c *check.C) { t := s.Init(c) - G.Pkg = NewPackage(t.File("category/package")) + pkg := NewPackage(t.File("category/package")) distinfoLine := t.NewLine(t.File("category/package/distinfo"), 5, "") - checker := distinfoLinesChecker{G.Pkg, nil, "", false, nil, nil} + checker := distinfoLinesChecker{pkg, nil, "", false, nil, nil} checker.checkPatchSha1(distinfoLine, "patch-nonexistent", "distinfo-sha1") t.CheckOutputLines( diff --git a/pkgtools/pkglint/files/files.go b/pkgtools/pkglint/files/files.go index d1a84df1859..a806f977f83 100644 --- a/pkgtools/pkglint/files/files.go +++ b/pkgtools/pkglint/files/files.go @@ -14,12 +14,12 @@ const ( LogErrors // ) -func LoadMk(filename CurrPath, options LoadOptions) *MkLines { +func LoadMk(filename CurrPath, pkg *Package, options LoadOptions) *MkLines { lines := Load(filename, options|Makefile) if lines == nil { return nil } - return NewMkLines(lines) + return NewMkLines(lines, pkg) } func Load(filename CurrPath, options LoadOptions) *Lines { @@ -76,7 +76,7 @@ func convertToLogicalLines(filename CurrPath, rawText string, joinBackslashLines } } else { for _, rawLine := range rawLines { - text := strings.TrimSuffix(rawLine.textnl, "\n") + text := rawLine.Text() logline := NewLine(filename, rawLine.Lineno, text, rawLine) loglines = append(loglines, logline) } @@ -92,9 +92,9 @@ func convertToLogicalLines(filename CurrPath, rawText string, joinBackslashLines func nextLogicalLine(filename CurrPath, rawLines []*RawLine, index int) (*Line, int) { { // Handle the common case efficiently rawLine := rawLines[index] - textnl := rawLine.textnl - if hasSuffix(textnl, "\n") && !hasSuffix(textnl, "\\\n") { - return NewLine(filename, rawLine.Lineno, textnl[:len(textnl)-1], rawLines[index]), index + 1 + text := rawLine.Text() + if !hasSuffix(text, "\\") { + return NewLine(filename, rawLine.Lineno, text, rawLines[index]), index + 1 } } @@ -105,7 +105,7 @@ func nextLogicalLine(filename CurrPath, rawLines []*RawLine, index int) (*Line, trim := "" for i, rawLine := range interestingRawLines { - indent, rawText, outdent, cont := matchContinuationLine(rawLine.textnl) + indent, rawText, outdent, cont := matchContinuationLine(rawLine.Text()) if text.Len() == 0 { text.WriteString(indent) @@ -130,36 +130,32 @@ func nextLogicalLine(filename CurrPath, rawLines []*RawLine, index int) (*Line, return NewLineMulti(filename, firstlineno, lastlineno, text.String(), lineRawLines), index + 1 } -func matchContinuationLine(textnl string) (leadingWhitespace, text, trailingWhitespace, cont string) { - j := len(textnl) +func matchContinuationLine(text string) (leadingWhitespace, result, trailingWhitespace, cont string) { + end := len(text) - if textnl[j-1] == '\n' { + j := end + for j > 0 && text[j-1] == '\\' { j-- } - - backslashes := 0 - for j > 0 && textnl[j-1] == '\\' { - j-- - backslashes++ - } - cont = textnl[j : j+backslashes%2] - j += backslashes / 2 + backslashes := (end - j) % 2 + j = end - backslashes + cont = text[j:end] trailingEnd := j - for j > 0 && isHspace(textnl[j-1]) { + for j > 0 && isHspace(text[j-1]) { j-- } trailingStart := j - trailingWhitespace = textnl[trailingStart:trailingEnd] + trailingWhitespace = text[trailingStart:trailingEnd] i := 0 leadingStart := i - for i < j && isHspace(textnl[i]) { + for i < j && isHspace(text[i]) { i++ } leadingEnd := i - leadingWhitespace = textnl[leadingStart:leadingEnd] + leadingWhitespace = text[leadingStart:leadingEnd] - text = textnl[leadingEnd:trailingStart] + result = text[leadingEnd:trailingStart] return } diff --git a/pkgtools/pkglint/files/files_test.go b/pkgtools/pkglint/files/files_test.go index f9432256905..6dcdc7d4d80 100644 --- a/pkgtools/pkglint/files/files_test.go +++ b/pkgtools/pkglint/files/files_test.go @@ -166,17 +166,17 @@ func (s *Suite) Test_convertToLogicalLines__comments(c *check.C) { "", "# Multiline comment", "# Another escaped comment that goes on and on", - "# This is NOT an escaped comment due to the double backslashes \\", + "# This is NOT an escaped comment due to the double backslashes \\\\", "VAR=\tThis is not a comment", "", "# This is a comment", - "#\\", - "\tThis is no comment", - "#\\ This is a comment", "#\\\\", "\tThis is no comment", "#\\\\ This is a comment", - "#\\\\\\", + "#\\\\\\\\", + "\tThis is no comment", + "#\\\\\\\\ This is a comment", + "#\\\\\\\\\\\\", "\tThis is no comment"}) t.CheckOutputEmpty() @@ -244,14 +244,14 @@ func (s *Suite) Test_nextLogicalLine__commented_multi(c *check.C) { func (s *Suite) Test_matchContinuationLine(c *check.C) { t := s.Init(c) - leadingWhitespace, text, trailingWhitespace, continuation := matchContinuationLine("\n") + leadingWhitespace, text, trailingWhitespace, continuation := matchContinuationLine("") t.CheckEquals(leadingWhitespace, "") t.CheckEquals(text, "") t.CheckEquals(trailingWhitespace, "") t.CheckEquals(continuation, "") - leadingWhitespace, text, trailingWhitespace, continuation = matchContinuationLine("\tword \\\n") + leadingWhitespace, text, trailingWhitespace, continuation = matchContinuationLine("\tword \\") t.CheckEquals(leadingWhitespace, "\t") t.CheckEquals(text, "word") diff --git a/pkgtools/pkglint/files/licenses.go b/pkgtools/pkglint/files/licenses.go index ad1b277a0cc..43656bf533a 100644 --- a/pkgtools/pkglint/files/licenses.go +++ b/pkgtools/pkglint/files/licenses.go @@ -8,7 +8,7 @@ type LicenseChecker struct { } func (lc *LicenseChecker) Check(value string, op MkOperator) { - expanded := resolveVariableRefs(lc.MkLines, value) // For ${PERL5_LICENSE} + expanded := resolveVariableRefs(value, lc.MkLines, nil) // For ${PERL5_LICENSE} cond := licenses.Parse(condStr(op == opAssignAppend, "append-placeholder ", "") + expanded) if cond == nil { @@ -25,10 +25,11 @@ func (lc *LicenseChecker) Check(value string, op MkOperator) { func (lc *LicenseChecker) checkName(license string) { licenseFile := NewCurrPath("") - if G.Pkg != nil { - if mkline := G.Pkg.vars.FirstDefinition("LICENSE_FILE"); mkline != nil { - rel := mkline.ResolveVarsInRelativePath(NewRelPathString(mkline.Value())) - licenseFile = G.Pkg.File(NewPackagePath(rel)) + pkg := lc.MkLines.pkg + if pkg != nil { + if mkline := pkg.vars.FirstDefinition("LICENSE_FILE"); mkline != nil { + rel := mkline.ResolveVarsInRelativePath(NewRelPathString(mkline.Value()), lc.MkLines.pkg) + licenseFile = pkg.File(NewPackagePath(rel)) } } if licenseFile.IsEmpty() { diff --git a/pkgtools/pkglint/files/line.go b/pkgtools/pkglint/files/line.go index 651e0e417ff..af4a4c49677 100644 --- a/pkgtools/pkglint/files/line.go +++ b/pkgtools/pkglint/files/line.go @@ -37,13 +37,14 @@ type RawLine struct { func (rline *RawLine) String() string { return sprintf("%d:%s", rline.Lineno, rline.textnl) } -func (rline *RawLine) text() string { - // TODO: use this method everywhere - // TODO: add orig() - // TODO: export these two functions +func (rline *RawLine) Text() string { return strings.TrimSuffix(rline.textnl, "\n") } +func (rline *RawLine) Orig() string { + return strings.TrimSuffix(rline.orignl, "\n") +} + type Location struct { Filename CurrPath firstLine int32 // zero means the whole file, -1 means EOF @@ -184,7 +185,6 @@ func (line *Line) String() string { // // fix.Replace("from", "to") // fix.ReplaceAfter("prefix", "from", "to") -// fix.ReplaceRegex(`[\t ]+`, "space", -1) // fix.InsertBefore("new line") // fix.InsertAfter("new line") // fix.Delete() diff --git a/pkgtools/pkglint/files/linechecker.go b/pkgtools/pkglint/files/linechecker.go index d96c221f9c2..b384044f141 100644 --- a/pkgtools/pkglint/files/linechecker.go +++ b/pkgtools/pkglint/files/linechecker.go @@ -42,9 +42,9 @@ func (ck LineChecker) CheckTrailingWhitespace() { // be Markdown files in pkgsrc, this code has to be adjusted. rawIndex := len(ck.line.raw) - 1 - text := ck.line.raw[rawIndex].text() - trimmed := rtrimHspace(text) - if len(trimmed) == len(text) { + text := ck.line.raw[rawIndex].Text() + trimmedLen := len(rtrimHspace(text)) + if trimmedLen == len(text) { return } @@ -52,6 +52,6 @@ func (ck LineChecker) CheckTrailingWhitespace() { fix.Notef("Trailing whitespace.") fix.Explain( "This whitespace is irrelevant and can be removed.") - fix.ReplaceAt(rawIndex, len(trimmed), text[len(trimmed):], "") + fix.ReplaceAt(rawIndex, trimmedLen, text[trimmedLen:], "") fix.Apply() } diff --git a/pkgtools/pkglint/files/lineslexer.go b/pkgtools/pkglint/files/lineslexer.go index 1e7b5c1264d..fcf4d69d007 100644 --- a/pkgtools/pkglint/files/lineslexer.go +++ b/pkgtools/pkglint/files/lineslexer.go @@ -92,10 +92,12 @@ func (llex *LinesLexer) SkipEmptyOrNote() bool { return true } - if llex.index == 0 { + if llex.index < llex.lines.Len() || llex.lines.Len() == 0 { fix := llex.CurrentLine().Autofix() fix.Notef("Empty line expected before this line.") - fix.InsertBefore("") + if !llex.EOF() { + fix.InsertBefore("") + } fix.Apply() } else { fix := llex.PreviousLine().Autofix() diff --git a/pkgtools/pkglint/files/logging.go b/pkgtools/pkglint/files/logging.go index 022798a1cad..a7de151c433 100644 --- a/pkgtools/pkglint/files/logging.go +++ b/pkgtools/pkglint/files/logging.go @@ -110,7 +110,7 @@ func (l *Logger) Diag(line *Line, level *LogLevel, format string, args ...interf case int, string, error: default: // All paths in diagnostics must be relative to the line. - // To achieve that, call line.File(currPath). + // To achieve that, call line.Rel(currPath). _ = arg.(RelPath) } } diff --git a/pkgtools/pkglint/files/mkassignchecker.go b/pkgtools/pkglint/files/mkassignchecker.go index 15f4bb224e2..5e18f8b07c2 100644 --- a/pkgtools/pkglint/files/mkassignchecker.go +++ b/pkgtools/pkglint/files/mkassignchecker.go @@ -60,7 +60,8 @@ func (ck *MkAssignChecker) checkVarassignLeftNotUsed() { return } - if G.Pkg != nil && G.Pkg.vars.IsUsedSimilar(varname) { + pkg := ck.MkLines.pkg + if pkg != nil && pkg.vars.IsUsedSimilar(varname) { return } @@ -323,7 +324,7 @@ func (ck *MkAssignChecker) checkVarassignOpShell() { // Authors of builtin.mk files usually know what they're doing. return - case G.Pkg == nil || G.Pkg.vars.IsUsedAtLoadTime(mkline.Varname()): + case ck.MkLines.pkg == nil || ck.MkLines.pkg.vars.IsUsedAtLoadTime(mkline.Varname()): return } @@ -477,11 +478,12 @@ func (ck *MkAssignChecker) checkVarassignDecreasingVersions() { func (ck *MkAssignChecker) checkVarassignMiscRedundantInstallationDirs() { mkline := ck.MkLine varname := mkline.Varname() + pkg := ck.MkLines.pkg switch { - case G.Pkg == nil, + case pkg == nil, varname != "INSTALLATION_DIRS", - !matches(G.Pkg.vars.LastValue("AUTO_MKDIRS"), `^[Yy][Ee][Ss]$`): + !matches(pkg.vars.LastValue("AUTO_MKDIRS"), `^[Yy][Ee][Ss]$`): return } @@ -491,7 +493,7 @@ func (ck *MkAssignChecker) checkVarassignMiscRedundantInstallationDirs() { } rel := NewRelPathString(dir) - if G.Pkg.Plist.Dirs[rel] != nil { + if pkg.Plist.Dirs[rel] != nil { mkline.Notef("The directory %q is redundant in %s.", rel, varname) mkline.Explain( "This package defines AUTO_MKDIR, and the directory is contained in the PLIST.", @@ -523,7 +525,7 @@ func (ck *MkAssignChecker) checkVarassignRightVaruse() { if vartype != nil && vartype.IsShell() { ck.checkVarassignVaruseShell(vartype, time) - } else { // XXX: This else looks as if it should be omitted. + } else { mkLineChecker := NewMkLineChecker(ck.MkLines, ck.MkLine) mkLineChecker.checkTextVarUse(ck.MkLine.Value(), vartype, time) } diff --git a/pkgtools/pkglint/files/mkassignchecker_test.go b/pkgtools/pkglint/files/mkassignchecker_test.go index b85d6ef9b21..6f4c80ca0ed 100644 --- a/pkgtools/pkglint/files/mkassignchecker_test.go +++ b/pkgtools/pkglint/files/mkassignchecker_test.go @@ -37,9 +37,9 @@ func (s *Suite) Test_MkAssignChecker_checkVarassign(c *check.C) { func (s *Suite) Test_MkAssignChecker_checkVarassign__URL_with_shell_special_characters(c *check.C) { t := s.Init(c) - G.Pkg = NewPackage(t.File("graphics/gimp-fix-ca")) + pkg := NewPackage(t.File("graphics/gimp-fix-ca")) t.SetUpVartypes() - mklines := t.NewMkLines("filename.mk", + mklines := t.NewMkLinesPkg("filename.mk", pkg, MkCvsID, "MASTER_SITES=\thttp://registry.gimp.org/file/fix-ca.c?action=download&id=9884&file=") @@ -871,32 +871,60 @@ func (s *Suite) Test_MkAssignChecker_checkVarassignMisc(c *check.C) { t.SetUpPkgsrc() t.SetUpMasterSite("MASTER_SITE_GITHUB", "https://download.github.com/") + t.FinishSetUp() - mklines := t.SetUpFileMkLines("module.mk", - MkCvsID, + test := func(text string, diagnostics ...string) { + mklines := t.NewMkLines("filename.mk", + MkCvsID, + text) + + mklines.Check() + + t.CheckOutput(diagnostics) + } + + test( "EGDIR=\t\t\t${PREFIX}/etc/rc.d", + "WARN: filename.mk:2: Please use the RCD_SCRIPTS mechanism "+ + "to install rc.d scripts automatically "+ + "to ${RCD_SCRIPTS_EXAMPLEDIR}.") + + // Since RPMIGNOREPATH effectively excludes the path, it is ok to + // mention etc/rc.d there. + test( "RPMIGNOREPATH+=\t\t${PREFIX}/etc/rc.d", + nil...) + + test( "_TOOLS_VARNAME.sed=\tSED", + "WARN: filename.mk:2: Variable names starting with an underscore "+ + "(_TOOLS_VARNAME.sed) are reserved for internal pkgsrc use.", + "WARN: filename.mk:2: _TOOLS_VARNAME.sed is defined but not used.") + + test( "DIST_SUBDIR=\t\t${PKGNAME}", + "WARN: filename.mk:2: PKGNAME should not be used in DIST_SUBDIR "+ + "as it includes the PKGREVISION. Please use PKGNAME_NOREV instead.") + + test( "WRKSRC=\t\t\t${PKGNAME}", + "WARN: filename.mk:2: PKGNAME should not be used in WRKSRC "+ + "as it includes the PKGREVISION. Please use PKGNAME_NOREV instead.") + + test( "SITES_distfile.tar.gz=\t${MASTER_SITE_GITHUB:=user/}", - "MASTER_SITES=\t\thttps://cdn.example.org/${PKGNAME}/", - "MASTER_SITES=\t\thttps://cdn.example.org/distname-${PKGVERSION}/") - t.FinishSetUp() + "WARN: filename.mk:2: SITES_distfile.tar.gz is defined but not used.", + "WARN: filename.mk:2: SITES_* is deprecated. Please use SITES.* instead.") - mklines.Check() + test( + "MASTER_SITES=\t\thttps://cdn.example.org/${PKGNAME}/", + "WARN: filename.mk:2: PKGNAME should not be used in MASTER_SITES "+ + "as it includes the PKGREVISION. Please use PKGNAME_NOREV instead.") - // TODO: Split this test into several, one for each topic. - t.CheckOutputLines( - "WARN: ~/module.mk:2: Please use the RCD_SCRIPTS mechanism to install rc.d scripts automatically to ${RCD_SCRIPTS_EXAMPLEDIR}.", - "WARN: ~/module.mk:4: Variable names starting with an underscore (_TOOLS_VARNAME.sed) are reserved for internal pkgsrc use.", - "WARN: ~/module.mk:4: _TOOLS_VARNAME.sed is defined but not used.", - "WARN: ~/module.mk:5: PKGNAME should not be used in DIST_SUBDIR as it includes the PKGREVISION. Please use PKGNAME_NOREV instead.", - "WARN: ~/module.mk:6: PKGNAME should not be used in WRKSRC as it includes the PKGREVISION. Please use PKGNAME_NOREV instead.", - "WARN: ~/module.mk:7: SITES_distfile.tar.gz is defined but not used.", - "WARN: ~/module.mk:7: SITES_* is deprecated. Please use SITES.* instead.", - "WARN: ~/module.mk:8: PKGNAME should not be used in MASTER_SITES as it includes the PKGREVISION. Please use PKGNAME_NOREV instead.", - "WARN: ~/module.mk:9: PKGVERSION should not be used in MASTER_SITES as it includes the PKGREVISION. Please use PKGVERSION_NOREV instead.") + test( + "MASTER_SITES=\t\thttps://cdn.example.org/distname-${PKGVERSION}/", + "WARN: filename.mk:2: PKGVERSION should not be used in MASTER_SITES "+ + "as it includes the PKGREVISION. Please use PKGVERSION_NOREV instead.") } func (s *Suite) Test_MkAssignChecker_checkVarassignMisc__multiple_inclusion_guards(c *check.C) { diff --git a/pkgtools/pkglint/files/mkcondchecker.go b/pkgtools/pkglint/files/mkcondchecker.go index 0f3de4cb53a..57629eb1257 100644 --- a/pkgtools/pkglint/files/mkcondchecker.go +++ b/pkgtools/pkglint/files/mkcondchecker.go @@ -147,8 +147,9 @@ func (ck *MkCondChecker) simplify(varuse *MkVarUse, fromEmpty bool, neg bool) { return true } - // TODO: Use ck.MkLines.loadVars instead. - if ck.MkLines.allVars.IsDefined(varname) { + // For run time expressions, such as ${${VAR} == value:?yes:no}, + // the scope would need to be changed to ck.MkLines.allVars. + if ck.MkLines.checkAllData.vars.IsDefined(varname) { return true } diff --git a/pkgtools/pkglint/files/mkcondchecker_test.go b/pkgtools/pkglint/files/mkcondchecker_test.go index 062897874f5..d2116c854da 100644 --- a/pkgtools/pkglint/files/mkcondchecker_test.go +++ b/pkgtools/pkglint/files/mkcondchecker_test.go @@ -188,8 +188,6 @@ func (s *Suite) Test_MkCondChecker_Check__compare_pattern_with_empty(c *check.C) mklines.Check() - // TODO: There should be a warning about "<>" containing invalid - // characters for a path. See VartypeCheck.Pathname t.CheckOutputLines( "WARN: filename.mk:8: The pathname pattern \"<>\" contains the invalid characters \"<>\".", "WARN: filename.mk:8: The pathname \"*\" contains the invalid character \"*\".") @@ -224,21 +222,26 @@ func (s *Suite) Test_MkCondChecker_checkEmpty(c *check.C) { t.Chdir("category/package") t.FinishSetUp() - // before: the directive before the condition is simplified - // after: the directive after the condition is simplified - // diagnostics: the usual ones - test := func(before, after string, diagnostics ...string) { + doTest := func(before string) { mklines := t.SetUpFileMkLines("filename.mk", MkCvsID, "", before, ".endif") + mklines.Check() + } + + // before: the directive before the condition is simplified + // after: the directive after the condition is simplified + // diagnostics: the usual ones + test := func(before, after string, diagnostics ...string) { + t.ExpectDiagnosticsAutofix( - func(autofix bool) { mklines.Check() }, + func(bool) { doTest(before) }, diagnostics...) - afterMklines := LoadMk(t.File("filename.mk"), MustSucceed) + afterMklines := LoadMk(t.File("filename.mk"), nil, MustSucceed) t.CheckEquals(afterMklines.mklines[2].Text, after) } @@ -434,7 +437,7 @@ func (s *Suite) Test_MkCondChecker_simplify(c *check.C) { }) if autofix { - afterMklines := LoadMk(t.File("filename.mk"), MustSucceed) + afterMklines := LoadMk(t.File("filename.mk"), nil, MustSucceed) t.CheckEquals(afterMklines.mklines[2].Text, after) } } @@ -949,9 +952,7 @@ func (s *Suite) Test_MkCondChecker_simplify__defined_in_same_file(c *check.C) { t.Chdir("category/package") t.FinishSetUp() - // before: the directive before the condition is simplified - // after: the directive after the condition is simplified - test := func(before, after string, diagnostics ...string) { + doTest := func(before string) { mklines := t.SetUpFileMkLines("filename.mk", MkCvsID, "OK=\t\tok", @@ -965,15 +966,19 @@ func (s *Suite) Test_MkCondChecker_simplify__defined_in_same_file(c *check.C) { // 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. + mklines.Check() + } + + // before: the directive before the condition is simplified + // after: the directive after the condition is simplified + test := func(before, after string, diagnostics ...string) { t.ExpectDiagnosticsAutofix( - func(autofix bool) { mklines.Check() }, + func(bool) { doTest(before) }, diagnostics...) // TODO: Move this assertion above the assertion about the diagnostics. - afterMklines := LoadMk(t.File("filename.mk"), MustSucceed) + afterMklines := LoadMk(t.File("filename.mk"), nil, MustSucceed) t.CheckEquals(afterMklines.mklines[3].Text, after) } @@ -1010,16 +1015,15 @@ func (s *Suite) Test_MkCondChecker_simplify__defined_in_same_file(c *check.C) { // therefore at the time of the .if statement, it is still empty. test( ".if ${LATER_DIR:Mpattern}", - ".if ${LATER_DIR} == pattern", + ".if ${LATER_DIR:U} == pattern", - // FIXME: Warn that LATER_DIR is used before it is defined. - // FIXME: Add :U modifier since LATER_DIR is not yet defined. + // TODO: Warn that LATER_DIR is used before it is defined. "NOTE: filename.mk:4: LATER_DIR can be "+ - "compared using the simpler \"${LATER_DIR} == pattern\" "+ + "compared using the simpler \"${LATER_DIR:U} == pattern\" "+ "instead of matching against \":Mpattern\".", "AUTOFIX: filename.mk:4: "+ "Replacing \"${LATER_DIR:Mpattern}\" "+ - "with \"${LATER_DIR} == pattern\".") + "with \"${LATER_DIR:U} == pattern\".") } func (s *Suite) Test_MkCondChecker_checkCompare(c *check.C) { @@ -1094,7 +1098,7 @@ func (s *Suite) Test_MkCondChecker_checkCompareVarStrCompiler(c *check.C) { t.Chdir("category/package") t.FinishSetUp() - test := func(cond string, diagnostics ...string) { + doTest := func(cond string) { mklines := t.SetUpFileMkLines("filename.mk", MkCvsID, "", @@ -1103,12 +1107,13 @@ func (s *Suite) Test_MkCondChecker_checkCompareVarStrCompiler(c *check.C) { ".if "+cond, ".endif") - t.SetUpCommandLine("-Wall") - mklines.Check() - t.SetUpCommandLine("-Wall", "--autofix") mklines.Check() + } - t.CheckOutput(diagnostics) + test := func(cond string, diagnostics ...string) { + t.ExpectDiagnosticsAutofix( + func(bool) { doTest(cond) }, + diagnostics...) } test( diff --git a/pkgtools/pkglint/files/mklexer.go b/pkgtools/pkglint/files/mklexer.go index 7f87c948eec..efb40b73dd6 100644 --- a/pkgtools/pkglint/files/mklexer.go +++ b/pkgtools/pkglint/files/mklexer.go @@ -56,7 +56,7 @@ func (p *MkLexer) MkToken() *MkToken { return &MkToken{Text: lexer.Since(mark), Varuse: varuse} } - for lexer.NextBytesFunc(func(b byte) bool { return b != '$' }) != "" || lexer.SkipString("$$") { + for lexer.SkipBytesFunc(func(b byte) bool { return b != '$' }) || lexer.SkipString("$$") { } text := lexer.Since(mark) if text != "" { @@ -468,7 +468,7 @@ func (p *MkLexer) varUseModifierSubst(closing byte) (ok bool, regex bool, from s case len(lexer.Rest()) >= 2 && lexer.PeekByte() == '\\' && separator != '\\': _ = lexer.Skip(2) - case lexer.NextBytesFunc(isOther) != "": + case lexer.SkipBytesFunc(isOther): break default: @@ -496,7 +496,7 @@ func (p *MkLexer) varUseModifierSubst(closing byte) (ok bool, regex bool, from s } optionsStart := lexer.Mark() - lexer.NextBytesFunc(func(b byte) bool { return b == '1' || b == 'g' || b == 'W' }) + lexer.SkipBytesFunc(func(b byte) bool { return b == '1' || b == 'g' || b == 'W' }) options = lexer.Since(optionsStart) ok = true diff --git a/pkgtools/pkglint/files/mkline.go b/pkgtools/pkglint/files/mkline.go index 591bce4c8db..419d24440c0 100644 --- a/pkgtools/pkglint/files/mkline.go +++ b/pkgtools/pkglint/files/mkline.go @@ -227,7 +227,7 @@ func (mkline *MkLine) FirstLineContainsValue() bool { assert(mkline.IsMultiline()) // Parsing the continuation marker as variable value is cheating but works well. - text := strings.TrimSuffix(mkline.raw[0].orignl, "\n") + text := mkline.raw[0].Orig() parser := NewMkLineParser() splitResult := parser.split(nil, text, true) _, a := parser.MatchVarassign(mkline.Line, text, &splitResult) @@ -492,6 +492,12 @@ func (mkline *MkLine) ValueFields(value string) []string { return fields } +func (mkline *MkLine) ValueFieldsLiteral() []string { + return filterStr( + mkline.ValueFields(mkline.Value()), + func(s string) bool { return !containsVarUse(s) }) +} + func (mkline *MkLine) ValueTokens() ([]*MkToken, string) { value := mkline.Value() if value == "" { @@ -556,14 +562,14 @@ func (*MkLine) WithoutMakeVariables(value string) string { return valueNovar.String() } -func (mkline *MkLine) ResolveVarsInRelativePath(relativePath RelPath) RelPath { - if !containsVarRef(relativePath.String()) { +func (mkline *MkLine) ResolveVarsInRelativePath(relativePath RelPath, pkg *Package) RelPath { + if !containsVarUse(relativePath.String()) { return relativePath.CleanPath() } var basedir CurrPath - if G.Pkg != nil { - basedir = G.Pkg.File(".") + if pkg != nil { + basedir = pkg.File(".") } else { basedir = mkline.Filename.DirNoClean() } @@ -615,12 +621,12 @@ func (mkline *MkLine) ResolveVarsInRelativePath(relativePath RelPath) RelPath { replaceLatest("${PYPACKAGE}", "lang", `^python[0-9]+$`, "$0") replaceLatest("${SUSE_DIR_PREFIX}", "emulators", `^(suse[0-9]+)_base$`, "$1") - if G.Pkg != nil { + if pkg != nil { // XXX: Even if these variables are defined indirectly, // pkglint should be able to resolve them properly. // There is already G.Pkg.Value, maybe that can be used here. - tmp = tmp.Replace("${FILESDIR}", G.Pkg.Filesdir.String()) - tmp = tmp.Replace("${PKGDIR}", G.Pkg.Pkgdir.String()) + tmp = tmp.Replace("${FILESDIR}", pkg.Filesdir.String()) + tmp = tmp.Replace("${PKGDIR}", pkg.Pkgdir.String()) } tmp = tmp.CleanPath() @@ -1049,6 +1055,7 @@ type indentationLevel struct { mkline *MkLine // The line in which the indentation started; the .if/.for depth int // Number of space characters; always a multiple of 2 args string // The arguments from the .if or .for, or the latest .elif + argsLine *MkLine // conditionalVars []string // Variables on which the current path depends // Files whose existence has been checked in an if branch that is @@ -1093,7 +1100,8 @@ func (ind *Indentation) Pop() { func (ind *Indentation) Push(mkline *MkLine, indent int, args string, guard bool) { assert(mkline.IsDirective()) - ind.levels = append(ind.levels, indentationLevel{mkline, indent, args, nil, nil, guard}) + ind.levels = append(ind.levels, + indentationLevel{mkline, indent, args, mkline, nil, nil, guard}) } // AddVar remembers that the current indentation depends on the given variable, @@ -1154,8 +1162,8 @@ func (ind *Indentation) Varnames() []string { } // Args returns the arguments of the innermost .if, .elif or .for. -func (ind *Indentation) Args() string { - return ind.top().args +func (ind *Indentation) Args() (string, *MkLine) { + return ind.top().args, ind.top().argsLine } func (ind *Indentation) AddCheckedFile(filename PkgsrcPath) { @@ -1216,6 +1224,7 @@ func (ind *Indentation) TrackAfter(mkline *MkLine) { // Handled here instead of TrackBefore to allow the action to access the previous condition. if !ind.IsEmpty() { ind.top().args = args + ind.top().argsLine = mkline } case "else": @@ -1305,7 +1314,7 @@ func MatchMkInclude(text string) (m bool, indentation, directive string, filenam } mark := lexer.Mark() - for lexer.NextBytesFunc(func(c byte) bool { return c != '"' && c != '$' }) != "" || + for lexer.SkipBytesFunc(func(c byte) bool { return c != '"' && c != '$' }) || lexer.NextVarUse() != nil { } enclosed := NewPath(lexer.Since(mark)) diff --git a/pkgtools/pkglint/files/mkline_test.go b/pkgtools/pkglint/files/mkline_test.go index 839ba2d0972..1b1bacb9b78 100644 --- a/pkgtools/pkglint/files/mkline_test.go +++ b/pkgtools/pkglint/files/mkline_test.go @@ -329,7 +329,8 @@ func (s *Suite) Test_MkLine_ValueFields__compared_to_splitIntoShellTokens(c *che words = mkline.ValueFields("a b \"c c c\" d;;d;; \"e\"''`` 'rest") t.CheckDeepEquals(words, []string{"a", "b", "\"c c c\"", "d;;d;;", "\"e\"''``"}) - // TODO: c.Check(rest, equals, "'rest") + // The rest "'rest" is silently discarded. + // Most probably, the shell will complain about it when it is executed. } func (s *Suite) Test_MkLine_ValueTokens(c *check.C) { @@ -532,9 +533,10 @@ func (s *Suite) Test_MkLine_ResolveVarsInRelativePath(c *check.C) { mklines := t.SetUpFileMkLines("Makefile", MkCvsID) mkline := mklines.mklines[0] + var pkg *Package = nil test := func(before RelPath, after RelPath) { - t.CheckEquals(mkline.ResolveVarsInRelativePath(before), after) + t.CheckEquals(mkline.ResolveVarsInRelativePath(before, pkg), after) } test("", ".") @@ -547,7 +549,7 @@ func (s *Suite) Test_MkLine_ResolveVarsInRelativePath(c *check.C) { test("${FILESDIR}", "${FILESDIR}") test("${PKGDIR}", "${PKGDIR}") - G.Pkg = NewPackage(t.File("category/package")) + pkg = NewPackage(t.File("category/package")) test("${FILESDIR}", "files") test("${PKGDIR}", ".") @@ -598,7 +600,7 @@ func (s *Suite) Test_MkLine_ResolveVarsInRelativePath__assertion(c *check.C) { mkline := t.NewMkLine("a/b/c/d/e/f/g.mk", 123, "") t.ExpectPanic( - func() { mkline.ResolveVarsInRelativePath("${PKGSRCDIR}") }, + func() { mkline.ResolveVarsInRelativePath("${PKGSRCDIR}", nil) }, "Pkglint internal error: "+ "Relative path \"../../../../../..\" for \"a/b/c/d/e/f\" is too deep "+ "below the pkgsrc root \".\".") @@ -696,8 +698,8 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__command_in_command(c *check.C) t.SetUpVartypes() t.SetUpTool("find", "FIND", AtRunTime) t.SetUpTool("sort", "SORT", AtRunTime) - G.Pkg = NewPackage(t.File("category/pkgbase")) - mklines := t.NewMkLines("Makefile", + pkg := NewPackage(t.File("category/pkgbase")) + mklines := t.NewMkLinesPkg("Makefile", pkg, MkCvsID, "GENERATE_PLIST= cd ${DESTDIR}${PREFIX}; ${FIND} * \\( -type f -or -type l \\) | ${SORT};") diff --git a/pkgtools/pkglint/files/mklinechecker.go b/pkgtools/pkglint/files/mklinechecker.go index ebd94951093..6fbcf02ee8f 100644 --- a/pkgtools/pkglint/files/mklinechecker.go +++ b/pkgtools/pkglint/files/mklinechecker.go @@ -43,7 +43,7 @@ func (ck MkLineChecker) checkEmptyContinuation() { } line := ck.MkLine.Line - if line.raw[len(line.raw)-1].orignl == "\n" { + if line.raw[len(line.raw)-1].Orig() == "" { lastLine := NewLine(line.Filename, int(line.lastLine), "", line.raw[len(line.raw)-1]) lastLine.Warnf("This line looks empty but continues the previous line.") lastLine.Explain( @@ -190,7 +190,7 @@ func (ck MkLineChecker) checkShellCommand() { shellCommand := mkline.ShellCommand() if hasPrefix(mkline.Text, "\t\t") { - lexer := textproc.NewLexer(mkline.raw[0].textnl) + lexer := textproc.NewLexer(mkline.raw[0].Text()) tabs := lexer.NextBytesFunc(func(b byte) bool { return b == '\t' }) fix := mkline.Autofix() @@ -202,7 +202,7 @@ func (ck MkLineChecker) checkShellCommand() { "or to use more horizontal space than necessary.") for i, raw := range mkline.Line.raw { - if hasPrefix(raw.textnl, tabs) { + if hasPrefix(raw.Text(), tabs) { fix.ReplaceAt(i, 0, tabs, "\t") } } @@ -285,7 +285,7 @@ func (ck MkLineChecker) checkDirectiveIndentation(expectedDepth int) { if expected := strings.Repeat(" ", expectedDepth); indent != expected { fix := mkline.Line.Autofix() fix.Notef("This directive should be indented by %d spaces.", expectedDepth) - if hasPrefix(mkline.Line.raw[0].text(), "."+indent) { + if hasPrefix(mkline.Line.raw[0].Text(), "."+indent) { fix.ReplaceAt(0, 0, "."+indent, "."+expected) } fix.Apply() @@ -304,8 +304,8 @@ func (ck MkLineChecker) CheckRelativePath(relativePath RelPath, mustExist bool) mkline.Errorf("A main pkgsrc package must not depend on a pkgsrc-wip package.") } - resolvedPath := mkline.ResolveVarsInRelativePath(relativePath) - if containsVarRef(resolvedPath.String()) { + resolvedPath := mkline.ResolveVarsInRelativePath(relativePath, ck.MkLines.pkg) + if containsVarUse(resolvedPath.String()) { return } @@ -360,9 +360,9 @@ func (ck MkLineChecker) CheckRelativePkgdir(pkgdir RelPath) { mkline := ck.MkLine ck.CheckRelativePath(pkgdir.JoinNoClean("Makefile"), true) - pkgdir = mkline.ResolveVarsInRelativePath(pkgdir) + pkgdir = mkline.ResolveVarsInRelativePath(pkgdir, ck.MkLines.pkg) - if !matches(pkgdir.String(), `^\.\./\.\./([^./][^/]*/[^./][^/]*)$`) && !containsVarRef(pkgdir.String()) { + if !matches(pkgdir.String(), `^\.\./\.\./([^./][^/]*/[^./][^/]*)$`) && !containsVarUse(pkgdir.String()) { mkline.Warnf("%q is not a valid relative package directory.", pkgdir) mkline.Explain( "A relative pathname always starts with \"../../\", followed", @@ -440,14 +440,16 @@ func (ck MkLineChecker) checkDirectiveEnd(ind *Indentation) { } if directive == "endif" { - if args := ind.Args(); !contains(args, comment) { - mkline.Warnf("Comment %q does not match condition %q.", comment, args) + if args, argsLine := ind.Args(); !contains(args, comment) { + mkline.Warnf("Comment %q does not match condition %q in %s.", + comment, args, mkline.RelMkLine(argsLine)) } } if directive == "endfor" { - if args := ind.Args(); !contains(args, comment) { - mkline.Warnf("Comment %q does not match loop %q.", comment, args) + if args, argsLine := ind.Args(); !contains(args, comment) { + mkline.Warnf("Comment %q does not match loop %q in %s.", + comment, args, mkline.RelMkLine(argsLine)) } } } @@ -474,12 +476,10 @@ func (ck MkLineChecker) checkDirectiveFor(forVars map[string]bool, indentation * forVars[forvar] = true } - // XXX: The type BtUnknown is very unspecific here. For known variables - // or constant values this could probably be improved. - // - // The guessed flag could also be determined more correctly. As of November 2018, - // running pkglint over the whole pkgsrc tree did not produce any different result - // whether guessed was true or false. + // The guessed flag could be determined more correctly. + // As of January 2020, running pkglint over the whole pkgsrc + // tree did not produce any different result whether guessed + // was true or false. forLoopType := NewVartype(btForLoop, List, NewACLEntry("*", aclpAllRead)) forLoopContext := VarUseContext{forLoopType, VucLoadTime, VucQuotPlain, false} mkline.ForEachUsed(func(varUse *MkVarUse, time VucTime) { diff --git a/pkgtools/pkglint/files/mklinechecker_test.go b/pkgtools/pkglint/files/mklinechecker_test.go index ae9d7f645f6..c4ea112c4f2 100644 --- a/pkgtools/pkglint/files/mklinechecker_test.go +++ b/pkgtools/pkglint/files/mklinechecker_test.go @@ -53,12 +53,6 @@ func (s *Suite) Test_MkLineChecker_Check__buildlink3_include_prefs(c *check.C) { ".include \"../../mk/bsd.prefs.mk\"", ".include \"../../mk/bsd.fast.prefs.mk\"") - // If the buildlink3.mk file doesn't actually exist, resolving the - // relative path fails since that depends on the actual file system, - // not on syntactical paths; see os.Stat in CheckRelativePath. - // - // TODO: Refactor Relpath to be independent of a filesystem. - mklines.Check() t.CheckOutputLines( @@ -501,7 +495,7 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveIndentation__autofix(c *check.C ".endif", ".endfor", ".endif") - mklines := NewMkLines(lines) + mklines := NewMkLines(lines, nil) mklines.Check() @@ -838,8 +832,8 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveEnd__ending_comments(c *check.C "", ".if ${OPSYS} == NetBSD", ".elif ${OPSYS} == FreeBSD", - ".endif # NetBSD", // Wrong, should be FreeBSD from the .elif. - "", + ".endif # NetBSD", // Wrong, should be OPSYS, which applies to all branches. + "", // Or FreeBSD since that is the branch being closed right now. ".for ii in 1 2", ". for jj in 1 2", ". endfor # ii", // Note: a simple "i" would not generate a warning because it is found in the word "in". @@ -849,13 +843,12 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveEnd__ending_comments(c *check.C mklines.Check() t.CheckOutputLines( - // TODO: mention the line number of the corresponding condition. - "WARN: opsys.mk:9: Comment \"MACHINE_ARCH\" does not match condition \"${OS_VERSION:M8.*}\".", - "WARN: opsys.mk:10: Comment \"OS_VERSION\" does not match condition \"${MACHINE_ARCH} == x86_64\".", - "WARN: opsys.mk:12: Comment \"j\" does not match loop \"i in 1 2 3 4 5\".", + "WARN: opsys.mk:9: Comment \"MACHINE_ARCH\" does not match condition \"${OS_VERSION:M8.*}\" in line 8.", + "WARN: opsys.mk:10: Comment \"OS_VERSION\" does not match condition \"${MACHINE_ARCH} == x86_64\" in line 7.", + "WARN: opsys.mk:12: Comment \"j\" does not match loop \"i in 1 2 3 4 5\" in line 5.", "WARN: opsys.mk:14: Unknown option \"option\".", - "WARN: opsys.mk:22: Comment \"NetBSD\" does not match condition \"${OPSYS} == FreeBSD\".", - "WARN: opsys.mk:26: Comment \"ii\" does not match loop \"jj in 1 2\".") + "WARN: opsys.mk:22: Comment \"NetBSD\" does not match condition \"${OPSYS} == FreeBSD\" in line 21.", + "WARN: opsys.mk:26: Comment \"ii\" does not match loop \"jj in 1 2\" in line 25.") } // After removing the dummy indentation in commit d5a926af, diff --git a/pkgtools/pkglint/files/mklineparser.go b/pkgtools/pkglint/files/mklineparser.go index 549796ebad0..1d8ef7cad1d 100644 --- a/pkgtools/pkglint/files/mklineparser.go +++ b/pkgtools/pkglint/files/mklineparser.go @@ -16,7 +16,6 @@ func NewMkLineParser() MkLineParser { return MkLineParser{} } func (p MkLineParser) Parse(line *Line) *MkLine { text := line.Text - // XXX: This check should be moved somewhere else. NewMkLine should only be concerned with parsing. if hasPrefix(text, " ") && line.Basename != "bsd.buildlink3.mk" { line.Warnf("Makefile lines should not start with space characters.") line.Explain( @@ -135,7 +134,7 @@ func (p MkLineParser) MatchVarassign(line *Line, text string, splitResult *mkLin value := trimHspace(lexer.Rest()) parsedValueAlign := condStr(commented, "#", "") + lexer.Since(mainStart) - valueAlign := p.getRawValueAlign(line.raw[0].orignl, parsedValueAlign) + valueAlign := p.getRawValueAlign(line.raw[0].Orig(), parsedValueAlign) if value == "" { valueAlign += splitResult.spaceBeforeComment splitResult.spaceBeforeComment = "" diff --git a/pkgtools/pkglint/files/mklineparser_test.go b/pkgtools/pkglint/files/mklineparser_test.go index b459941ad27..2e2a4a160ce 100644 --- a/pkgtools/pkglint/files/mklineparser_test.go +++ b/pkgtools/pkglint/files/mklineparser_test.go @@ -38,7 +38,7 @@ func (s *Suite) Test_MkLineParser_Parse__infrastructure(c *check.C) { "WARN: infra.mk:2: USE_BUILTIN.${_pkg_:S/^-//} is defined but not used.", "WARN: infra.mk:2: _pkg_ is used but not defined.", "ERROR: infra.mk:5: \".export\" requires arguments.", - "NOTE: infra.mk:2: This variable value should be aligned to column 41.", + "NOTE: infra.mk:2: This variable value should be aligned to column 41 instead of 39.", "ERROR: infra.mk:10: Unmatched .endif.") } @@ -431,7 +431,7 @@ func (s *Suite) Test_MkLineParser_fixSpaceAfterVarname__autofix(c *check.C) { "VARNAME+ ?=\t${VARNAME}", "pkgbase := pkglint") - CheckFileMk(filename) + CheckFileMk(filename, nil) t.CheckOutputLines( "NOTE: ~/Makefile:2: Unnecessary space after variable name \"VARNAME\".", @@ -444,7 +444,7 @@ func (s *Suite) Test_MkLineParser_fixSpaceAfterVarname__autofix(c *check.C) { t.SetUpCommandLine("-Wall", "--autofix") - CheckFileMk(filename) + CheckFileMk(filename, nil) t.CheckOutputLines( "AUTOFIX: ~/Makefile:2: Replacing \"VARNAME +=\\t\" with \"VARNAME+=\\t\".", diff --git a/pkgtools/pkglint/files/mklines.go b/pkgtools/pkglint/files/mklines.go index 882fdf91982..fae2f2165f4 100644 --- a/pkgtools/pkglint/files/mklines.go +++ b/pkgtools/pkglint/files/mklines.go @@ -4,9 +4,10 @@ import "strings" // MkLines contains data for the Makefile (or *.mk) that is currently checked. type MkLines struct { - mklines []*MkLine - lines *Lines - target string // Current make(1) target; only available during checkAll + mklines []*MkLine + lines *Lines + pkg *Package + 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. @@ -14,15 +15,28 @@ type MkLines struct { plistVarSkip bool // True if any of the PLIST_VARS identifiers refers to a variable. Tools *Tools // Tools defined in file scope. indentation *Indentation // Indentation depth of preprocessing directives; only available during MkLines.ForEach. - forVars map[string]bool // The variables currently used in .for loops; only available during MkLines.checkAll. once Once - postLine func(mkline *MkLine) // Custom action that is run after checking each line // TODO: Consider extracting plistVarAdded, plistVarSet, plistVarSkip into an own type. // TODO: Describe where each of the above fields is valid. + + checkAllData mklinesCheckAll } -func NewMkLines(lines *Lines) *MkLines { +type mklinesCheckAll struct { + // Current make(1) target + target string + + vars Scope + + // The variables currently used in .for loops + forVars map[string]bool + + // Custom action that is run after checking each line + postLine func(mkline *MkLine) +} + +func NewMkLines(lines *Lines, pkg *Package) *MkLines { mklines := make([]*MkLine, lines.Len()) for i, line := range lines.Lines { mklines[i] = NewMkLineParser().Parse(line) @@ -34,7 +48,7 @@ func NewMkLines(lines *Lines) *MkLines { return &MkLines{ mklines, lines, - "", + pkg, NewScope(), make(map[string]bool), make(map[string]*MkLine), @@ -42,9 +56,12 @@ func NewMkLines(lines *Lines) *MkLines { false, tools, nil, - make(map[string]bool), Once{}, - nil} + mklinesCheckAll{ + target: "", + vars: NewScope(), + forVars: make(map[string]bool), + postLine: nil}} } // TODO: Consider defining an interface MkLinesChecker (different name, though, since this one confuses even me) @@ -81,9 +98,8 @@ func (mklines *MkLines) Check() { mklines.collectUsedVariables() mklines.collectVariables() mklines.collectPlistVars() - mklines.collectElse() - if G.Pkg != nil { - G.Pkg.collectConditionalIncludes(mklines) + if mklines.pkg != nil { + mklines.pkg.collectConditionalIncludes(mklines) } // In the second pass, the actual checks are done. @@ -138,8 +154,8 @@ func (mklines *MkLines) collectUsedVariables() { // This controls the "defined but not used" warning. func (mklines *MkLines) UseVar(mkline *MkLine, varname string, time VucTime) { mklines.allVars.Use(varname, mkline, time) - if G.Pkg != nil { - G.Pkg.vars.Use(varname, mkline, time) + if mklines.pkg != nil { + mklines.pkg.vars.Use(varname, mkline, time) } } @@ -160,10 +176,10 @@ func (mklines *MkLines) collectDocumentedVariables() { // The commentLines include the the line containing the variable name, // leaving 2 of these 3 lines for the actual documentation. if commentLines >= 3 && relevant { - for varname, mkline := range scope.used { + forEachStringMkLine(scope.used, func(varname string, mkline *MkLine) { mklines.allVars.Define(varname, mkline) mklines.allVars.Use(varname, mkline, VucRunTime) - } + }) } scope = NewScope() @@ -219,63 +235,67 @@ func (mklines *MkLines) collectVariables() { } mklines.ForEach(func(mkline *MkLine) { - mklines.Tools.ParseToolLine(mklines, mkline, false, true) + mklines.collectVariable(mkline) + }) +} - if !mkline.IsVarassignMaybeCommented() { - return - } +func (mklines *MkLines) collectVariable(mkline *MkLine) { + mklines.Tools.ParseToolLine(mklines, mkline, false, true) - mklines.defineVar(G.Pkg, mkline, mkline.Varname()) - - varcanon := mkline.Varcanon() - switch varcanon { - case - "BUILD_DEFS", - "PKG_GROUPS_VARS", // see mk/misc/unprivileged.mk - "PKG_USERS_VARS": // see mk/misc/unprivileged.mk - for _, varname := range mkline.Fields() { - mklines.buildDefs[varname] = true - if trace.Tracing { - trace.Step1("%q is added to BUILD_DEFS.", varname) - } - } + if !mkline.IsVarassignMaybeCommented() { + return + } - case - "BUILTIN_FIND_FILES_VAR", - "BUILTIN_FIND_HEADERS_VAR": - for _, varname := range mkline.Fields() { - mklines.allVars.Define(varname, mkline) + mklines.defineVar(mkline, mkline.Varname()) + + varcanon := mkline.Varcanon() + switch varcanon { + case + "BUILD_DEFS", + "PKG_GROUPS_VARS", // see mk/misc/unprivileged.mk + "PKG_USERS_VARS": // see mk/misc/unprivileged.mk + for _, varname := range mkline.Fields() { + mklines.buildDefs[varname] = true + if trace.Tracing { + trace.Step1("%q is added to BUILD_DEFS.", varname) } + } - case "PLIST_VARS": - for _, id := range mkline.ValueFields(resolveVariableRefs(mklines, mkline.Value())) { - if trace.Tracing { - trace.Step1("PLIST.%s is added to PLIST_VARS.", id) - } + case + "BUILTIN_FIND_FILES_VAR", + "BUILTIN_FIND_HEADERS_VAR": + for _, varname := range mkline.Fields() { + mklines.allVars.Define(varname, mkline) + } - if containsVarRef(id) { - mklines.UseVar(mkline, "PLIST.*", mkline.Op().Time()) - mklines.plistVarSkip = true - } else { - mklines.UseVar(mkline, "PLIST."+id, mkline.Op().Time()) - } + case "PLIST_VARS": + for _, id := range mkline.ValueFields(resolveVariableRefs(mkline.Value(), mklines, nil)) { + if trace.Tracing { + trace.Step1("PLIST.%s is added to PLIST_VARS.", id) } - case "SUBST_VARS.*": - for _, substVar := range mkline.Fields() { - mklines.UseVar(mkline, varnameCanon(substVar), mkline.Op().Time()) - if trace.Tracing { - trace.Step1("varuse %s", substVar) - } + if containsVarUse(id) { + mklines.UseVar(mkline, "PLIST.*", mkline.Op().Time()) + mklines.plistVarSkip = true + } else { + mklines.UseVar(mkline, "PLIST."+id, mkline.Op().Time()) } + } - case "OPSYSVARS": - for _, opsysVar := range mkline.Fields() { - mklines.UseVar(mkline, opsysVar+".*", mkline.Op().Time()) - mklines.defineVar(G.Pkg, mkline, opsysVar) + case "SUBST_VARS.*": + for _, substVar := range mkline.Fields() { + mklines.UseVar(mkline, varnameCanon(substVar), mkline.Op().Time()) + if trace.Tracing { + trace.Step1("varuse %s", substVar) } } - }) + + case "OPSYSVARS": + for _, opsysVar := range mkline.Fields() { + mklines.UseVar(mkline, opsysVar+".*", mkline.Op().Time()) + mklines.defineVar(mkline, opsysVar) + } + } } // ForEach calls the action for each line, until the action returns false. @@ -322,10 +342,10 @@ 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) { +func (mklines *MkLines) defineVar(mkline *MkLine, varname string) { mklines.allVars.Define(varname, mkline) - if pkg != nil { - pkg.vars.Define(varname, mkline) + if mklines.pkg != nil { + mklines.pkg.vars.Define(varname, mkline) } } @@ -335,8 +355,8 @@ func (mklines *MkLines) collectPlistVars() { if mkline.IsVarassign() { switch mkline.Varcanon() { case "PLIST_VARS": - for _, id := range mkline.ValueFields(resolveVariableRefs(mklines, mkline.Value())) { - if containsVarRef(id) { + for _, id := range mkline.ValueFields(resolveVariableRefs(mkline.Value(), mklines, nil)) { + if containsVarUse(id) { mklines.plistVarSkip = true } else { mklines.plistVarAdded[id] = mkline @@ -344,7 +364,7 @@ func (mklines *MkLines) collectPlistVars() { } case "PLIST.*": id := mkline.Varparam() - if containsVarRef(id) { + if containsVarUse(id) { mklines.plistVarSkip = true } else { mklines.plistVarSet[id] = mkline @@ -354,13 +374,11 @@ func (mklines *MkLines) collectPlistVars() { } } -func (mklines *MkLines) collectElse() { - // Make a dry-run over the lines, which sets data.elseLine (in mkline.go) as a side-effect. - mklines.ForEach(func(mkline *MkLine) {}) - // TODO: Check whether this ForEach is redundant because it is already run somewhere else. -} - func (mklines *MkLines) checkAll() { + // checkAll must only be called once, even during tests, since it + // doesn't clean up all its effects on mklines. + assert(mklines.once.FirstTime("checkAll")) + allowedTargets := map[string]bool{ "pre-fetch": true, "do-fetch": true, "post-fetch": true, "pre-extract": true, "do-extract": true, "post-extract": true, @@ -376,7 +394,7 @@ func (mklines *MkLines) checkAll() { mklines.lines.CheckCvsID(0, `#[\t ]+`, "# ") - substContext := NewSubstContext() + substContext := NewSubstContext(mklines.pkg) var varalign VaralignBlock vargroupsChecker := NewVargroupsChecker(mklines) isHacksMk := mklines.lines.BaseName == "hacks.mk" @@ -397,7 +415,7 @@ func (mklines *MkLines) checkAll() { // This check is not done by ForEach because ForEach only // manages the iteration, not the actual checks. mklines.indentation.CheckFinish(mklines.lines.Filename) - vargroupsChecker.Finish(mkline) + vargroupsChecker.Finish() }) substContext.Finish(mklines.EOFLine()) @@ -424,37 +442,39 @@ func (mklines *MkLines) checkLine( switch { case mkline.IsVarassign(): - mklines.target = "" + mklines.checkAllData.target = "" mkline.Tokenize(mkline.Value(), true) // Just for the side-effect of the warnings. mklines.checkVarassignPlist(mkline) + varname := mkline.Varname() + mklines.checkAllData.vars.Define(varname, mkline) case mkline.IsInclude(): - mklines.target = "" - if G.Pkg != nil { - G.Pkg.checkIncludeConditionally(mkline, mklines.indentation) + mklines.checkAllData.target = "" + if mklines.pkg != nil { + mklines.pkg.checkIncludeConditionally(mkline, mklines.indentation) } case mkline.IsDirective(): - ck.checkDirective(mklines.forVars, mklines.indentation) + ck.checkDirective(mklines.checkAllData.forVars, mklines.indentation) case mkline.IsDependency(): ck.checkDependencyRule(allowedTargets) - mklines.target = mkline.Targets() + mklines.checkAllData.target = mkline.Targets() case mkline.IsShellCommand(): mkline.Tokenize(mkline.ShellCommand(), true) // Just for the side-effect of the warnings. } - if mklines.postLine != nil { - mklines.postLine(mkline) + if mklines.checkAllData.postLine != nil { + mklines.checkAllData.postLine(mkline) } } func (mklines *MkLines) checkVarassignPlist(mkline *MkLine) { switch mkline.Varcanon() { case "PLIST_VARS": - for _, id := range mkline.ValueFields(resolveVariableRefs(mklines, mkline.Value())) { + for _, id := range mkline.ValueFields(resolveVariableRefs(mkline.Value(), mklines, nil)) { if !mklines.plistVarSkip && mklines.plistVarSet[id] == nil { mkline.Warnf("%q is added to PLIST_VARS, but PLIST.%s is not defined in this file.", id, id) } @@ -611,7 +631,7 @@ func (mklines *MkLines) ExpandLoopVar(varname string) []string { } // TODO: If needed, add support for multi-variable .for loops. - resolved := resolveVariableRefs(mklines, mkline.Args()) + resolved := resolveVariableRefs(mkline.Args(), mklines, nil) words := mkline.ValueFields(resolved) if len(words) >= 3 && words[0] == varname && words[1] == "in" { return words[2:] diff --git a/pkgtools/pkglint/files/mklines_test.go b/pkgtools/pkglint/files/mklines_test.go index 3e3e56ce0b4..33999bbb759 100644 --- a/pkgtools/pkglint/files/mklines_test.go +++ b/pkgtools/pkglint/files/mklines_test.go @@ -10,8 +10,8 @@ func (s *Suite) Test_MkLines__quoting_LDFLAGS_for_GNU_configure(c *check.C) { t := s.Init(c) t.SetUpVartypes() - G.Pkg = NewPackage(t.File("category/pkgbase")) - mklines := t.NewMkLines("Makefile", + pkg := NewPackage(t.File("category/pkgbase")) + mklines := t.NewMkLinesPkg("Makefile", pkg, // XXX: Is in wrong directory. MkCvsID, "GNU_CONFIGURE=\tyes", "CONFIGURE_ENV+=\tX_LIBS=${X11_LDFLAGS:Q}") @@ -342,7 +342,6 @@ func (s *Suite) Test_MkLines_Check__indentation_include(c *check.C) { mklines.Check() t.CheckOutputLines( - // TODO: Use relative path for missing package. "ERROR: module.mk:5: There is no package in \"../../category/nonexistent\".", "NOTE: module.mk:7: This directive should be indented by 2 spaces.", "NOTE: module.mk:9: This directive should be indented by 2 spaces.") @@ -763,7 +762,7 @@ func (s *Suite) Test_MkLines_ForEachEnd(c *check.C) { }) } -func (s *Suite) Test_MkLines_collectElse(c *check.C) { +func (s *Suite) Test_MkLines_checkAll__collect_else(c *check.C) { t := s.Init(c) t.SetUpVartypes() @@ -782,7 +781,9 @@ func (s *Suite) Test_MkLines_collectElse(c *check.C) { ".elif 0", ".endif") - mklines.collectElse() + // As a side-effect of MkLines.ForEach, + // the HasElseBranch in the lines is updated. + mklines.collectVariables() t.CheckEquals(mklines.mklines[2].HasElseBranch(), false) t.CheckEquals(mklines.mklines[5].HasElseBranch(), true) @@ -1107,8 +1108,8 @@ func (s *Suite) Test_MkLines_checkAll__extra_warnings(c *check.C) { t.SetUpCommandLine("-Wextra") t.SetUpVartypes() - G.Pkg = NewPackage(t.File("category/pkgbase")) - mklines := t.NewMkLines("options.mk", + pkg := NewPackage(t.File("category/pkgbase")) + mklines := t.NewMkLinesPkg("options.mk", pkg, MkCvsID, "", ".for word in ${PKG_FAIL_REASON}", @@ -1130,6 +1131,20 @@ func (s *Suite) Test_MkLines_checkAll__extra_warnings(c *check.C) { "NOTE: options.mk:11: You can use \"../build\" instead of \"${WRKSRC}/../build\".") } +// Between 2019-12-31 and 2020-01-01, pkglint panicked because it didn't +// expect that a package would define PKGDIR to point to itself. +func (s *Suite) Test_MkLines_checkAll__assertion(c *check.C) { + t := s.Init(c) + + pkg := NewPackage(t.SetUpPackage("category/package", + "PKGDIR=\t../../category/package")) + t.FinishSetUp() + + pkg.Check() + + t.CheckOutputEmpty() +} + // At 2018-12-02, pkglint had resolved ${MY_PLIST_VARS} into a single word, // whereas the correct behavior is to resolve it into two words. // It had produced warnings about mismatched PLIST_VARS IDs. diff --git a/pkgtools/pkglint/files/mkparser.go b/pkgtools/pkglint/files/mkparser.go index 5afdae576a6..7eedbc2eca7 100644 --- a/pkgtools/pkglint/files/mkparser.go +++ b/pkgtools/pkglint/files/mkparser.go @@ -203,7 +203,7 @@ loop: switch { case p.mklex.VarUse() != nil, lexer.NextBytesSet(textproc.Alnum) != "", - lexer.NextBytesFunc(func(b byte) bool { return b != '"' && b != '\\' }) != "": + lexer.SkipBytesFunc(func(b byte) bool { return b != '"' && b != '\\' }): rhsText.WriteString(lexer.Since(m)) case lexer.SkipString("\\\""), @@ -250,10 +250,11 @@ func (p *MkParser) mkCondFunc() *MkCond { // TODO: Consider suggesting ${VAR} instead of !empty(VAR) since it is shorter and // avoids unnecessary negation, which makes the expression less confusing. // This applies especially to the ${VAR:Mpattern} form. + // See MkCondChecker.simplify. case "commands", "exists", "make", "target": argMark := lexer.Mark() - for p.mklex.VarUse() != nil || lexer.NextBytesFunc(func(b byte) bool { return b != '$' && b != ')' }) != "" { + for p.mklex.VarUse() != nil || lexer.SkipBytesFunc(func(b byte) bool { return b != '$' && b != ')' }) { } arg := lexer.Since(argMark) if lexer.SkipByte(')') { diff --git a/pkgtools/pkglint/files/mkshwalker.go b/pkgtools/pkglint/files/mkshwalker.go index eb96104ff70..0cb6f513906 100644 --- a/pkgtools/pkglint/files/mkshwalker.go +++ b/pkgtools/pkglint/files/mkshwalker.go @@ -1,10 +1,5 @@ package pkglint -import ( - "reflect" - "strings" -) - type MkShWalker struct { Callback struct { List func(list *MkShList) @@ -62,31 +57,6 @@ func NewMkShWalker() *MkShWalker { return &MkShWalker{} } -// Path returns a representation of the path in the AST that is -// currently visited. -// -// It is used for debugging only. -// -// See Test_MkShWalker_Walk, Callback.SimpleCommand for examples. -func (w *MkShWalker) Path() string { - var path []string - for _, level := range w.Context { - elementType := reflect.TypeOf(level.Element) - typeName := elementType.Elem().Name() - if typeName == "" { - typeName = "[]" + elementType.Elem().Elem().Name() - } - abbreviated := strings.TrimPrefix(typeName, "MkSh") - if level.Index == -1 { - // TODO: This form should also be used if index == 0 and len == 1. - path = append(path, abbreviated) - } else { - path = append(path, sprintf("%s[%d]", abbreviated, level.Index)) - } - } - return strings.Join(path, ".") -} - // Walk calls the given callback for each node of the parsed shell program, // in visiting order from large to small. func (w *MkShWalker) Walk(list *MkShList) { diff --git a/pkgtools/pkglint/files/mkshwalker_test.go b/pkgtools/pkglint/files/mkshwalker_test.go index e68dc84650d..8ceeb939c02 100644 --- a/pkgtools/pkglint/files/mkshwalker_test.go +++ b/pkgtools/pkglint/files/mkshwalker_test.go @@ -1,6 +1,37 @@ package pkglint -import "gopkg.in/check.v1" +import ( + "gopkg.in/check.v1" + "reflect" + "strings" +) + +// Path returns a representation of the path in the AST that is +// currently visited. +// +// It is used for debugging only. +// +// See Test_MkShWalker_Walk, Callback.SimpleCommand for examples. +// +// TODO: Move to test file. +func (w *MkShWalker) Path() string { + var path []string + for _, level := range w.Context { + elementType := reflect.TypeOf(level.Element) + typeName := elementType.Elem().Name() + if typeName == "" { + typeName = "[]" + elementType.Elem().Elem().Name() + } + abbreviated := strings.TrimPrefix(typeName, "MkSh") + if level.Index == -1 { + // TODO: This form should also be used if index == 0 and len == 1. + path = append(path, abbreviated) + } else { + path = append(path, sprintf("%s[%d]", abbreviated, level.Index)) + } + } + return strings.Join(path, ".") +} func (s *Suite) Test_MkShWalker_Walk(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/mkvarusechecker.go b/pkgtools/pkglint/files/mkvarusechecker.go index b99fe286e56..a260e0195d8 100644 --- a/pkgtools/pkglint/files/mkvarusechecker.go +++ b/pkgtools/pkglint/files/mkvarusechecker.go @@ -47,10 +47,10 @@ func (ck *MkVarUseChecker) checkUndefined() { vartype != nil && !vartype.IsGuessed(), // TODO: At load time, check ck.MkLines.loadVars instead of allVars. ck.MkLines.allVars.IsDefinedSimilar(varname), - ck.MkLines.forVars[varname], + ck.MkLines.checkAllData.forVars[varname], ck.MkLines.allVars.Mentioned(varname) != nil, - G.Pkg != nil && G.Pkg.vars.IsDefinedSimilar(varname), - containsVarRef(varname), + ck.MkLines.pkg != nil && ck.MkLines.pkg.vars.IsDefinedSimilar(varname), + containsVarUse(varname), G.Pkgsrc.vartypes.IsDefinedCanon(varname), varname == "": return @@ -200,7 +200,9 @@ func (ck *MkVarUseChecker) checkPermissions(vuc *VarUseContext) { effPerms := vartype.EffectivePermissions(basename) if effPerms.Contains(aclpUseLoadtime) { - ck.checkUseAtLoadTime(vuc.time) + if vuc.time == VucLoadTime { + ck.checkUseAtLoadTime() + } // Since the variable may be used at load time, it probably // may be used at run time as well. If it weren't, that would @@ -285,34 +287,32 @@ func (ck *MkVarUseChecker) warnPermissions( } alternativeFiles := vartype.AlternativeFiles(needed) - loadTimeExplanation := func() []string { - return []string{ - "Many variables, especially lists of something, get their values incrementally.", - "Therefore it is generally unsafe to rely on their", - "value until it is clear that it will never change again.", - "This point is reached when the whole package Makefile is loaded and", - "execution of the shell commands starts; in some cases earlier.", - "", - "Additionally, when using the \":=\" operator, each $$ is replaced", - "with a single $, so variables that have references to shell", - "variables or regular expressions are modified in a subtle way."} - } + loadTimeExplanation := []string{ + "Many variables, especially lists of something, get their values incrementally.", + "Therefore it is generally unsafe to rely on their", + "value until it is clear that it will never change again.", + "This point is reached when the whole package Makefile is loaded and", + "execution of the shell commands starts; in some cases earlier.", + "", + "Additionally, when using the \":=\" operator, each $$ is replaced", + "with a single $, so variables that have references to shell", + "variables or regular expressions are modified in a subtle way."} switch { case alternativeFiles == "" && directly: mkline.Warnf("%s should not be used at load time in any file.", varname) - ck.explainPermissions(varname, vartype, loadTimeExplanation()...) + ck.explainPermissions(varname, vartype, loadTimeExplanation...) case alternativeFiles == "": mkline.Warnf("%s should not be used in any file.", varname) - ck.explainPermissions(varname, vartype, loadTimeExplanation()...) + ck.explainPermissions(varname, vartype, loadTimeExplanation...) case directly: mkline.Warnf( "%s should not be used at load time in this file; "+ "it would be ok in %s.", varname, alternativeFiles) - ck.explainPermissions(varname, vartype, loadTimeExplanation()...) + ck.explainPermissions(varname, vartype, loadTimeExplanation...) default: mkline.Warnf( @@ -364,14 +364,11 @@ func (ck *MkVarUseChecker) explainPermissions(varname string, vartype *Vartype, ck.MkLine.Explain(expl...) } -func (ck *MkVarUseChecker) checkUseAtLoadTime(time VucTime) { - if time != VucLoadTime { - return - } +func (ck *MkVarUseChecker) checkUseAtLoadTime() { if ck.vartype.IsAlwaysInScope() || ck.MkLines.Tools.SeenPrefs { return } - if G.Pkg != nil && G.Pkg.seenPrefs { + if ck.MkLines.pkg != nil && ck.MkLines.pkg.seenPrefs { return } mkline := ck.MkLine @@ -417,8 +414,6 @@ func (ck *MkVarUseChecker) warnToolLoadTime(varname string, tool *Tool) { // to skip the shell and execute the commands via execve, which // means that even echo is not a shell-builtin anymore. - // TODO: Replace "parse time" with "load time" everywhere. - if tool.Validity == AfterPrefsMk { ck.MkLine.Warnf("To use the tool ${%s} at load time, bsd.prefs.mk has to be included before.", varname) return @@ -473,7 +468,7 @@ func (ck *MkVarUseChecker) checkQuoting(vuc *VarUseContext) { // since the GNU configure scripts cannot handle these space characters. // // When doing checks outside a package, the :M* modifier is needed for safety. - needMstar := (G.Pkg == nil || G.Pkg.vars.IsDefined("GNU_CONFIGURE")) && + needMstar := (ck.MkLines.pkg == nil || ck.MkLines.pkg.vars.IsDefined("GNU_CONFIGURE")) && matches(varUse.varname, `^(?:.*_)?(?:CFLAGS|CPPFLAGS|CXXFLAGS|FFLAGS|LDFLAGS|LIBS)$`) mkline := ck.MkLine @@ -502,11 +497,11 @@ func (ck *MkVarUseChecker) checkQuotingQM(mod string, needMstar bool, vuc *VarUs if correctMod == mod+":Q" && vuc.IsWordPart && !vartype.IsShell() { isSingleWordConstant := func() bool { - if G.Pkg == nil { + if ck.MkLines.pkg == nil { return false } - varinfo := G.Pkg.redundant.vars[varname] + varinfo := ck.MkLines.pkg.redundant.vars[varname] if varinfo == nil || !varinfo.vari.IsConstant() { return false } diff --git a/pkgtools/pkglint/files/mkvarusechecker_test.go b/pkgtools/pkglint/files/mkvarusechecker_test.go index 58656aaed2c..43c0e063ff9 100644 --- a/pkgtools/pkglint/files/mkvarusechecker_test.go +++ b/pkgtools/pkglint/files/mkvarusechecker_test.go @@ -120,7 +120,7 @@ func (s *Suite) Test_MkVarUseChecker_Check__build_defs(c *check.C) { t := s.Init(c) // XXX: This paragraph should not be necessary since VARBASE and X11_TYPE - // are also defined in vardefs.go. + // are also defined in vardefs.go. t.SetUpPkgsrc() t.CreateFileLines("mk/defaults/mk.conf", "VARBASE?= /usr/pkg/var") @@ -1085,7 +1085,8 @@ func (s *Suite) Test_MkVarUseChecker_checkQuoting__list_variable_with_single_con G.Check(pkg) // TODO: Don't warn here since BUILD_DIRS, although being a list - // variable, contains only a single value. + // variable, contains only a single value, + // no matter which of the branches is taken. t.CheckOutputLines( "WARN: ~/category/package/Makefile:26: " + "The list variable BUILD_DIRS should not be embedded in a word.") diff --git a/pkgtools/pkglint/files/options.go b/pkgtools/pkglint/files/options.go index 49044342988..e08af3c9d9c 100755 --- a/pkgtools/pkglint/files/options.go +++ b/pkgtools/pkglint/files/options.go @@ -78,7 +78,7 @@ func (ck *OptionsLinesChecker) collect() { func (ck *OptionsLinesChecker) handleUpperLine(mkline *MkLine, seenPkgOptionsVar bool) { declare := func(option string) { - if containsVarRef(option) { + if containsVarUse(option) { ck.declaredArbitrary = true } else { ck.declaredOptions[option] = mkline @@ -106,7 +106,7 @@ func (ck *OptionsLinesChecker) handleUpperLine(mkline *MkLine, seenPkgOptionsVar declare(option) } if len(forVars) == 0 { - for _, option := range mkline.ValueFields(resolveVariableRefs(ck.mklines, option)) { + for _, option := range mkline.ValueFields(resolveVariableRefs(option, ck.mklines, nil)) { declare(option) } } @@ -138,7 +138,7 @@ func (ck *OptionsLinesChecker) handleLowerLine(mkline *MkLine) { func (ck *OptionsLinesChecker) handleLowerCondition(mkline *MkLine, cond *MkCond) { recordOption := func(option string) { - if containsVarRef(option) { + if containsVarUse(option) { ck.handledArbitrary = true return } diff --git a/pkgtools/pkglint/files/package.go b/pkgtools/pkglint/files/package.go index 864be372de1..78b39526f0b 100644 --- a/pkgtools/pkglint/files/package.go +++ b/pkgtools/pkglint/files/package.go @@ -131,7 +131,7 @@ func (pkg *Package) load() ([]CurrPath, *MkLines, *MkLines) { } files := pkg.File(".").ReadPaths() - if pkg.Pkgdir != "." { + if pkg.Pkgdir != "." && pkg.Rel(pkg.File(pkg.Pkgdir)) != "." { files = append(files, pkg.File(pkg.Pkgdir).ReadPaths()...) } files = append(files, pkg.File(pkg.Patchdir).ReadPaths()...) @@ -158,7 +158,7 @@ func (pkg *Package) load() ([]CurrPath, *MkLines, *MkLines) { for _, filename := range files { basename := filename.Base() if isRelevantMk(filename, basename) { - fragmentMklines := LoadMk(filename, MustSucceed) + fragmentMklines := LoadMk(filename, pkg, MustSucceed) pkg.collectConditionalIncludes(fragmentMklines) } if hasPrefix(basename, "PLIST") { @@ -176,12 +176,12 @@ func (pkg *Package) loadPackageMakefile() (*MkLines, *MkLines) { defer trace.Call(filename)() } - mainLines := LoadMk(filename, NotEmpty|LogErrors) + mainLines := LoadMk(filename, pkg, NotEmpty|LogErrors) if mainLines == nil { return nil, nil } - allLines := NewMkLines(NewLines("", nil)) + allLines := NewMkLines(NewLines("", nil), pkg) if !pkg.parse(mainLines, allLines, "", true) { return nil, nil } @@ -256,7 +256,7 @@ func (pkg *Package) parse(mklines *MkLines, allLines *MkLines, includingFileForU 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) + builtinMkLines := LoadMk(builtin, pkg, MustSucceed|LogErrors) pkg.parse(builtinMkLines, allLines, "", false) } } @@ -342,7 +342,7 @@ func (pkg *Package) loadIncluded(mkline *MkLine, includingFile CurrPath) (includ if trace.Tracing { trace.Stepf("Including %q.", fullIncluded) } - includedMklines = LoadMk(fullIncluded, 0) + includedMklines = LoadMk(fullIncluded, pkg, 0) if includedMklines != nil { return includedMklines, false } @@ -365,7 +365,7 @@ func (pkg *Package) loadIncluded(mkline *MkLine, includingFile CurrPath) (includ dirname = pkgBasedir fullIncludedFallback := dirname.JoinNoClean(includedFile) - includedMklines = LoadMk(fullIncludedFallback, 0) + includedMklines = LoadMk(fullIncludedFallback, pkg, 0) if includedMklines == nil { return nil, false } @@ -387,12 +387,11 @@ func (pkg *Package) loadIncluded(mkline *MkLine, includingFile CurrPath) (includ // their actual values. func (pkg *Package) resolveIncludedFile(mkline *MkLine, includingFilename CurrPath) RelPath { - // 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()) + resolved := mkline.ResolveVarsInRelativePath(mkline.IncludedFile(), pkg) + includedText := resolveVariableRefs(resolved.String(), nil, pkg) includedFile := NewRelPathString(includedText) - if containsVarRef(includedText) { + if containsVarUse(includedText) { if trace.Tracing && !includingFilename.ContainsPath("mk") { trace.Stepf("%s:%s: Skipping unresolvable include file %q.", mkline.Filename, mkline.Linenos(), includedFile) @@ -403,8 +402,8 @@ func (pkg *Package) resolveIncludedFile(mkline *MkLine, includingFilename CurrPa if mkline.Basename != "buildlink3.mk" { if includedFile.HasSuffixPath("buildlink3.mk") { curr := mkline.File(includedFile) - if G.Pkg != nil && !curr.IsFile() { - curr = G.Pkg.File(PackagePath(includedFile)) + if !curr.IsFile() { + curr = pkg.File(PackagePath(includedFile)) } packagePath := pkg.Rel(curr) pkg.bl3[packagePath] = mkline @@ -496,7 +495,7 @@ func (pkg *Package) check(filenames []CurrPath, mklines, allLines *MkLines) { havePatches := false for _, filename := range filenames { - if containsVarRef(filename.String()) { + if containsVarUse(filename.String()) { if trace.Tracing { trace.Stepf("Skipping file %q because the name contains an unresolved variable.", filename) } @@ -513,7 +512,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.Rel(filename).Count() == 3: + case filename.HasBase("Makefile") && pkg.Rel(filename) == "Makefile": G.checkExecutable(filename, st.Mode()) pkg.checkfilePackageMakefile(filename, mklines, allLines) @@ -561,7 +560,7 @@ func (pkg *Package) checkfilePackageMakefile(filename CurrPath, mklines *MkLines } } else { distinfoFile := pkg.File(pkg.DistinfoFile) - if !containsVarRef(distinfoFile.String()) && !distinfoFile.IsFile() { + if !containsVarUse(distinfoFile.String()) && !distinfoFile.IsFile() { line := NewLineWhole(distinfoFile) line.Warnf("A package that downloads files should have a distinfo file.") line.Explain( @@ -624,7 +623,7 @@ func (pkg *Package) checkfilePackageMakefile(filename CurrPath, mklines *MkLines // TODO: Maybe later collect the conditional includes from allLines // instead of mklines. This will lead to about 6000 new warnings // though. - // pkg.collectConditionalIncludes(allLines) + // pkg.collectConditionalIncludes(allLines) allLines.collectVariables() // To get the tool definitions mklines.Tools = allLines.Tools // TODO: also copy the other collected data @@ -634,7 +633,7 @@ func (pkg *Package) checkfilePackageMakefile(filename CurrPath, mklines *MkLines // set Tools.SeenPrefs, but it should. // // See Test_Package_checkfilePackageMakefile__options_mk. - mklines.postLine = func(mkline *MkLine) { + mklines.checkAllData.postLine = func(mkline *MkLine) { if mkline == pkg.prefsLine { pkg.seenPrefs = true } @@ -950,8 +949,6 @@ func (pkg *Package) checkCategories() { switch mkline.Op() { case opAssignDefault: for _, category := range mkline.ValueFields(mkline.Value()) { - // 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 } @@ -1086,7 +1083,7 @@ func (pkg *Package) determineEffectivePkgVars() { } } - if pkgname == "" && distnameLine != nil && !containsVarRef(distname) && !matches(distname, rePkgname) { + if pkgname == "" && distnameLine != nil && !containsVarUse(distname) && !matches(distname, rePkgname) { distnameLine.Warnf("As DISTNAME is not a valid package name, please define the PKGNAME explicitly.") } @@ -1094,7 +1091,7 @@ func (pkg *Package) determineEffectivePkgVars() { distname = "" } - if effname != "" && !containsVarRef(effname) { + if effname != "" && !containsVarUse(effname) { if m, m1, m2 := match2(effname, rePkgname); m { pkg.EffectivePkgname = effname + pkg.nbPart() pkg.EffectivePkgnameLine = pkgnameLine @@ -1103,7 +1100,7 @@ func (pkg *Package) determineEffectivePkgVars() { } } - if pkg.EffectivePkgnameLine == nil && distname != "" && !containsVarRef(distname) { + if pkg.EffectivePkgnameLine == nil && distname != "" && !containsVarUse(distname) { if m, m1, m2 := match2(distname, rePkgname); m { pkg.EffectivePkgname = distname + pkg.nbPart() pkg.EffectivePkgnameLine = distnameLine @@ -1268,7 +1265,7 @@ func (pkg *Package) checkDirent(dirent CurrPath, mode os.FileMode) { switch { case mode.IsRegular(): - G.checkReg(dirent, basename, G.Pkgsrc.Rel(dirent).Count()) + G.checkReg(dirent, basename, G.Pkgsrc.Rel(dirent).Count(), pkg) case hasPrefix(basename, "work"): if G.Opts.Import { @@ -1361,7 +1358,8 @@ func (pkg *Package) checkFreeze(filename CurrPath) { "See https://www.NetBSD.org/developers/pkgsrc/ for the exact rules.") } -func (pkg *Package) checkFileMakefileExt(filename CurrPath) { +// TODO: Move to MkLinesChecker. +func (*Package) checkFileMakefileExt(filename CurrPath) { base := filename.Base() if !hasPrefix(base, "Makefile.") || base == "Makefile.common" { return @@ -1484,6 +1482,11 @@ func (pkg *Package) checkIncludeConditionally(mkline *MkLine, indentation *Inden // already done with *_MK variables. } +func (pkg *Package) matchesLicenseFile(basename string) bool { + licenseFile := NewPath(pkg.vars.LastValue("LICENSE_FILE")) + return basename == licenseFile.Base() +} + func (pkg *Package) AutofixDistinfo(oldSha1, newSha1 string) { distinfoFilename := pkg.File(pkg.DistinfoFile) if lines := Load(distinfoFilename, NotEmpty|LogErrors); lines != nil { @@ -1502,7 +1505,7 @@ func (pkg *Package) AutofixDistinfo(oldSha1, newSha1 string) { // Variables that are known in the package are resolved, e.g. ${PKGDIR}. func (pkg *Package) File(relativeFileName PackagePath) CurrPath { joined := pkg.dir.JoinNoClean(NewRelPath(relativeFileName.AsPath())) - resolved := resolveVariableRefs(nil /* XXX: or maybe some mklines? */, joined.String()) + resolved := resolveVariableRefs(joined.String(), nil, pkg) return NewCurrPathString(resolved).CleanPath() } diff --git a/pkgtools/pkglint/files/package_test.go b/pkgtools/pkglint/files/package_test.go index e7817320e30..c56d67f63a8 100644 --- a/pkgtools/pkglint/files/package_test.go +++ b/pkgtools/pkglint/files/package_test.go @@ -524,9 +524,9 @@ func (s *Suite) Test_Package_loadPackageMakefile(c *check.C) { "PKGNAME=pkgname-1.67", "DISTNAME=distfile_1_67", ".include \"../../category/package/Makefile\"") - G.Pkg = NewPackage(t.File("category/package")) + pkg := NewPackage(t.File("category/package")) - G.Pkg.loadPackageMakefile() + pkg.loadPackageMakefile() // Including a package Makefile directly is an error (in the last line), // but that is checked later. @@ -673,9 +673,9 @@ func (s *Suite) Test_Package_parse__simple(c *check.C) { t.Chdir("category/package") t.FinishSetUp() - G.Pkg = NewPackage(".") - G.Pkg.included.Trace = true - G.Pkg.load() + pkg := NewPackage(".") + pkg.included.Trace = true + pkg.load() t.CheckOutputLines( "FirstTime: suppress-varorder.mk") @@ -689,9 +689,9 @@ func (s *Suite) Test_Package_parse__nonexistent_Makefile(c *check.C) { t.Remove("Makefile") t.FinishSetUp() - G.Pkg = NewPackage(".") - G.Pkg.included.Trace = true - G.Pkg.load() + pkg := NewPackage(".") + pkg.included.Trace = true + pkg.load() t.CheckOutputLines( "ERROR: Makefile: Cannot be read.") @@ -707,9 +707,9 @@ func (s *Suite) Test_Package_parse__include_in_same_directory(c *check.C) { MkCvsID) t.FinishSetUp() - G.Pkg = NewPackage(".") - G.Pkg.included.Trace = true - G.Pkg.load() + pkg := NewPackage(".") + pkg.included.Trace = true + pkg.load() t.CheckOutputLines( "FirstTime: suppress-varorder.mk", @@ -724,9 +724,9 @@ func (s *Suite) Test_Package_parse__nonexistent_include(c *check.C) { t.Chdir("category/package") t.FinishSetUp() - G.Pkg = NewPackage(".") - G.Pkg.included.Trace = true - G.Pkg.load() + pkg := NewPackage(".") + pkg.included.Trace = true + pkg.load() t.CheckOutputLines( "FirstTime: suppress-varorder.mk", @@ -748,9 +748,9 @@ func (s *Suite) Test_Package_parse__include_twice(c *check.C) { MkCvsID) t.FinishSetUp() - G.Pkg = NewPackage(".") - G.Pkg.included.Trace = true - G.Pkg.load() + pkg := NewPackage(".") + pkg.included.Trace = true + pkg.load() t.CheckOutputLines( "FirstTime: suppress-varorder.mk", @@ -767,9 +767,9 @@ func (s *Suite) Test_Package_parse__include_in_other_directory(c *check.C) { MkCvsID) t.FinishSetUp() - G.Pkg = NewPackage(".") - G.Pkg.included.Trace = true - G.Pkg.load() + pkg := NewPackage(".") + pkg.included.Trace = true + pkg.load() t.CheckOutputLines( "FirstTime: suppress-varorder.mk", @@ -791,9 +791,9 @@ func (s *Suite) Test_Package_parse__includes_in_other_directory(c *check.C) { MkCvsID) t.FinishSetUp() - G.Pkg = NewPackage(".") - G.Pkg.included.Trace = true - G.Pkg.load() + pkg := NewPackage(".") + pkg.included.Trace = true + pkg.load() t.CheckOutputLines( "FirstTime: suppress-varorder.mk", @@ -812,9 +812,9 @@ func (s *Suite) Test_Package_parse__nonexistent_in_other_directory(c *check.C) { ".include \"version.mk\"") t.FinishSetUp() - G.Pkg = NewPackage(".") - G.Pkg.included.Trace = true - G.Pkg.load() + pkg := NewPackage(".") + pkg.included.Trace = true + pkg.load() t.CheckOutputLines( "FirstTime: suppress-varorder.mk", @@ -1496,18 +1496,16 @@ func (s *Suite) Test_Package_checkfilePackageMakefile__prefs_indirect(c *check.C ".include \"../../mk/bsd.prefs.mk\"") t.FinishSetUp() pkg := NewPackage(t.File("category/package")) - G.Pkg = pkg - defer func() { G.Pkg = nil }() - files, mklines, allLines := G.Pkg.load() + files, mklines, allLines := pkg.load() - t.CheckEquals(G.Pkg.seenPrefs, false) - t.CheckEquals(G.Pkg.prefsLine, mklines.mklines[21]) + t.CheckEquals(pkg.seenPrefs, false) + t.CheckEquals(pkg.prefsLine, mklines.mklines[21]) - G.Pkg.check(files, mklines, allLines) + pkg.check(files, mklines, allLines) - t.CheckEquals(G.Pkg.seenPrefs, true) - t.CheckEquals(G.Pkg.prefsLine, mklines.mklines[21]) + t.CheckEquals(pkg.seenPrefs, true) + t.CheckEquals(pkg.prefsLine, mklines.mklines[21]) // Since bsd.prefs.mk is included indirectly by common.mk, // OPSYS may be used at load time in line 23, but not in line 20. @@ -2623,18 +2621,18 @@ func (s *Suite) Test_Package_checkPossibleDowngrade(c *check.C) { G.Pkgsrc.loadDocChanges() t.Chdir("category/pkgbase") - G.Pkg = NewPackage(".") - G.Pkg.EffectivePkgname = "package-1.0nb15" - G.Pkg.EffectivePkgnameLine = t.NewMkLine("Makefile", 5, "PKGNAME=dummy") + pkg := NewPackage(".") + pkg.EffectivePkgname = "package-1.0nb15" + pkg.EffectivePkgnameLine = t.NewMkLine("Makefile", 5, "PKGNAME=dummy") - G.Pkg.checkPossibleDowngrade() + pkg.checkPossibleDowngrade() t.CheckOutputLines( "WARN: Makefile:5: The package is being downgraded from 1.8 (see ../../doc/CHANGES-2018:1) to 1.0nb15.") G.Pkgsrc.LastChange["category/pkgbase"].target = "1.0nb22" - G.Pkg.checkPossibleDowngrade() + pkg.checkPossibleDowngrade() t.CheckOutputEmpty() } @@ -3001,14 +2999,14 @@ func (s *Suite) Test_Package_checkLinesBuildlink3Inclusion__file_but_not_package t.CreateFileLines("category/dependency/buildlink3.mk") t.CreateFileLines("category/dependency/module.mk") - G.Pkg = NewPackage(t.File("category/package")) + pkg := NewPackage(t.File("category/package")) mklines := t.NewMkLines("category/package/buildlink3.mk", MkCvsID, "", ".include \"../../category/dependency/buildlink3.mk\"", ".include \"../../category/dependency/module.mk\"") - G.Pkg.checkLinesBuildlink3Inclusion(mklines) + pkg.checkLinesBuildlink3Inclusion(mklines) t.CheckOutputLines( "WARN: category/package/buildlink3.mk:3: " + @@ -3038,14 +3036,14 @@ func (s *Suite) Test_Package_checkLinesBuildlink3Inclusion__package_but_not_file t := s.Init(c) t.CreateFileLines("category/dependency/buildlink3.mk") - G.Pkg = NewPackage(t.File("category/package")) - G.Pkg.bl3["../../category/dependency/buildlink3.mk"] = + pkg := NewPackage(t.File("category/package")) + pkg.bl3["../../category/dependency/buildlink3.mk"] = t.NewMkLine("../../category/dependency/buildlink3.mk", 1, "") mklines := t.NewMkLines("category/package/buildlink3.mk", MkCvsID) t.EnableTracingToLog() - G.Pkg.checkLinesBuildlink3Inclusion(mklines) + pkg.checkLinesBuildlink3Inclusion(mklines) // This is only traced but not logged as a regular warning since // several packages have build dependencies that are not needed @@ -3074,7 +3072,6 @@ func (s *Suite) Test_Package_checkLinesBuildlink3Inclusion__mk_dotdot_dotdot(c * t.Chdir(".") t.FinishSetUp() pkg := NewPackage("x11/ocaml-graphics") - G.Pkg = pkg files, mklines, allLines := pkg.load() pkg.check(files, mklines, allLines) @@ -3101,7 +3098,6 @@ func (s *Suite) Test_Package_checkLinesBuildlink3Inclusion__mk_dotdot(c *check.C t.Chdir(".") t.FinishSetUp() pkg := NewPackage("x11/ocaml-graphics") - G.Pkg = pkg files, mklines, allLines := pkg.load() pkg.check(files, mklines, allLines) @@ -3499,10 +3495,10 @@ func (s *Suite) Test_Package_AutofixDistinfo__missing_file(c *check.C) { t := s.Init(c) t.SetUpPkgsrc() - G.Pkg = NewPackage(t.File("category/package")) + pkg := NewPackage(t.File("category/package")) t.FinishSetUp() - G.Pkg.AutofixDistinfo("old", "new") + pkg.AutofixDistinfo("old", "new") t.CheckOutputLines( "ERROR: ~/category/package/distinfo: Cannot be read.") diff --git a/pkgtools/pkglint/files/patches.go b/pkgtools/pkglint/files/patches.go index 7d871486f4c..c0c8d0b1885 100644 --- a/pkgtools/pkglint/files/patches.go +++ b/pkgtools/pkglint/files/patches.go @@ -4,8 +4,8 @@ package pkglint import "strings" -func CheckLinesPatch(lines *Lines) { - (&PatchChecker{lines, NewLinesLexer(lines), false, false}).Check() +func CheckLinesPatch(lines *Lines, pkg *Package) { + (&PatchChecker{lines, NewLinesLexer(lines), false, false}).Check(pkg) } type PatchChecker struct { @@ -21,7 +21,7 @@ const ( rePatchUniHunk = `^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@(.*)$` ) -func (ck *PatchChecker) Check() { +func (ck *PatchChecker) Check(pkg *Package) { if ck.lines.CheckCvsID(0, ``, "") { ck.llex.Skip() } @@ -84,10 +84,10 @@ func (ck *PatchChecker) Check() { CheckLinesTrailingEmptyLines(ck.lines) sha1Before := computePatchSha1Hex(ck.lines) - if SaveAutofixChanges(ck.lines) && G.Pkg != nil { + if SaveAutofixChanges(ck.lines) && pkg != nil { linesAfter := Load(ck.lines.Filename, 0) sha1After := computePatchSha1Hex(linesAfter) - G.Pkg.AutofixDistinfo(sha1Before, sha1After) + pkg.AutofixDistinfo(sha1Before, sha1After) } } diff --git a/pkgtools/pkglint/files/patches_test.go b/pkgtools/pkglint/files/patches_test.go index fe3ed787122..2f930035695 100644 --- a/pkgtools/pkglint/files/patches_test.go +++ b/pkgtools/pkglint/files/patches_test.go @@ -23,7 +23,7 @@ func (s *Suite) Test_CheckLinesPatch__with_comment(c *check.C) { "+new line", " context after") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputEmpty() } @@ -51,12 +51,12 @@ func (s *Suite) Test_CheckLinesPatch__without_empty_line__autofix(c *check.C) { "SHA1 (some patch) = 49abd735b7e706ea9ed6671628bb54e91f7f5ffb") t.SetUpCommandLine("-Wall", "--autofix") - G.Pkg = NewPackage(".") + pkg := NewPackage(".") - CheckLinesPatch(patchLines) + CheckLinesPatch(patchLines, pkg) t.CheckOutputLines( - "AUTOFIX: patch-WithoutEmptyLines:1: Inserting a line \"\" after this line.", + "AUTOFIX: patch-WithoutEmptyLines:2: Inserting a line \"\" before this line.", "AUTOFIX: patch-WithoutEmptyLines:3: Inserting a line \"\" before this line.", "AUTOFIX: distinfo:3: Replacing \"49abd735b7e706ea9ed6671628bb54e91f7f5ffb\" "+ "with \"4938fc8c0b483dc2e33e741b0da883d199e78164\".") @@ -90,7 +90,7 @@ func (s *Suite) Test_CheckLinesPatch__no_comment_and_no_empty_lines(c *check.C) "-old line", "+new line") - CheckLinesPatch(patchLines) + CheckLinesPatch(patchLines, nil) // These duplicate notes are actually correct. There should be an // empty line above the documentation and one below it. Since there @@ -98,7 +98,7 @@ func (s *Suite) Test_CheckLinesPatch__no_comment_and_no_empty_lines(c *check.C) // the same. Outside of the testing environment, this duplicate // diagnostic is suppressed; see LogVerbose. t.CheckOutputLines( - "NOTE: ~/patch-WithoutEmptyLines:1: Empty line expected after this line.", + "NOTE: ~/patch-WithoutEmptyLines:2: Empty line expected before this line.", "ERROR: ~/patch-WithoutEmptyLines:2: Each patch must be documented.", "NOTE: ~/patch-WithoutEmptyLines:2: Empty line expected.") } @@ -117,7 +117,7 @@ func (s *Suite) Test_CheckLinesPatch__without_comment(c *check.C) { "+old line", " context after") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "ERROR: patch-WithoutComment:3: Each patch must be documented.") @@ -142,7 +142,7 @@ func (s *Suite) Test_CheckLinesPatch__error_code(c *check.C) { "+old line", " context after") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputEmpty() } @@ -164,7 +164,7 @@ func (s *Suite) Test_CheckLinesPatch__wrong_header_order(c *check.C) { "+old line", " context after") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "WARN: patch-WrongOrder:7: Unified diff headers should be first ---, then +++.") @@ -181,7 +181,7 @@ func (s *Suite) Test_CheckLinesPatch__context_diff(c *check.C) { "*** history.c.orig", "--- history.c") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "ERROR: patch-ctx:4: Each patch must be documented.", @@ -197,7 +197,7 @@ func (s *Suite) Test_CheckLinesPatch__no_patch(c *check.C) { "-- oldfile", "++ newfile") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "ERROR: patch-aa: Contains no patch.") @@ -224,7 +224,7 @@ func (s *Suite) Test_CheckLinesPatch__two_patched_files(c *check.C) { "-old", "+new") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "WARN: patch-aa: Contains patches for 2 files, should be only one.") @@ -250,7 +250,7 @@ func (s *Suite) Test_CheckLinesPatch__two_patched_files_for_CVE(c *check.C) { "-old", "+new") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputEmpty() } @@ -268,7 +268,7 @@ func (s *Suite) Test_CheckLinesPatch__documentation_that_looks_like_patch_lines( "", "*** oldOrNewFile") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "ERROR: patch-aa: Contains no patch.") @@ -285,7 +285,7 @@ func (s *Suite) Test_CheckLinesPatch__only_unified_header_but_no_content(c *chec "--- file.orig", "+++ file") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "ERROR: patch-unified:EOF: No patch hunks for \"file\".") @@ -302,7 +302,7 @@ func (s *Suite) Test_CheckLinesPatch__only_context_header_but_no_content(c *chec "*** file.orig", "--- file") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) // Context diffs are deprecated, therefore it is not worth // adding extra code for checking them thoroughly. @@ -327,7 +327,7 @@ func (s *Suite) Test_CheckLinesPatch__no_newline_with_text_following(c *check.C) "\\ No newline at end of file", "last line (a comment)") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "WARN: patch-aa:12: Empty line or end of file expected.") @@ -349,7 +349,7 @@ func (s *Suite) Test_CheckLinesPatch__no_newline(c *check.C) { "+new", "\\ No newline at end of file") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputEmpty() } @@ -374,7 +374,7 @@ func (s *Suite) Test_CheckLinesPatch__empty_lines_left_out_at_eof(c *check.C) { " 5", " 6") // Line 7 was empty, therefore omitted - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputEmpty() } @@ -397,7 +397,7 @@ func (s *Suite) Test_CheckLinesPatch__context_lines_with_tab_instead_of_space(c "+new", "\tcontext") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputEmpty() } @@ -411,7 +411,7 @@ func (s *Suite) Test_CheckLinesPatch__autofix_empty_patch(c *check.C) { lines := t.NewLines("patch-aa", CvsID) - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputEmpty() } @@ -426,7 +426,7 @@ func (s *Suite) Test_CheckLinesPatch__autofix_long_empty_patch(c *check.C) { CvsID, "") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputEmpty() } @@ -446,7 +446,7 @@ func (s *Suite) Test_CheckLinesPatch__crlf_autofix(c *check.C) { "-old line", "+new line") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) // To relieve the pkgsrc package maintainers from this boring work, // the pkgsrc infrastructure could fix these issues before actually @@ -469,7 +469,7 @@ func (s *Suite) Test_CheckLinesPatch__autogenerated(c *check.C) { "-old line", "+: Avoid regenerating within pkgsrc") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "ERROR: ~/patch-aa:9: This code must not be included in patches.") @@ -490,7 +490,7 @@ func (s *Suite) Test_CheckLinesPatch__empty_context_lines_in_hunk(c *check.C) { "-old line", "+new line") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) // The first context line should start with a single space character, // but that would mean trailing whitespace, so it may be left out. @@ -516,7 +516,7 @@ func (s *Suite) Test_CheckLinesPatch__invalid_line_in_hunk(c *check.C) { "<<<<<<<<", "+new line") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "ERROR: ~/patch-aa:10: Invalid line in unified patch hunk: <<<<<<<<") @@ -530,7 +530,7 @@ func (s *Suite) Test_PatchChecker_Check__missing_CVS_Id(c *check.C) { "", "Documentation") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( sprintf("ERROR: ~/patch-aa:1: Expected %q.", CvsID), @@ -551,7 +551,7 @@ func (s *Suite) Test_PatchChecker_Check__add_file(c *check.C) { "@@ -0,0 +1,1 @@", "+ added line") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputEmpty() } @@ -569,7 +569,7 @@ func (s *Suite) Test_PatchChecker_Check__delete_file(c *check.C) { "@@ -1,1 +0,0 @@", "- deleted line") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputEmpty() } @@ -588,7 +588,7 @@ func (s *Suite) Test_PatchChecker_Check__absolute_path(c *check.C) { "- deleted line", "+ added line") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) // XXX: Patches must not apply to absolute paths. // The only allowed exception is /dev/null. @@ -613,7 +613,7 @@ func (s *Suite) Test_PatchChecker_checkUnifiedDiff__lines_at_end(c *check.C) { "This line is not part of the patch. Since it is separated from", "the patch by an empty line, there is no reason for a warning.") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputEmpty() } @@ -636,7 +636,7 @@ func (s *Suite) Test_PatchChecker_checkBeginDiff__multiple_patches_without_docum "- old", "+ new") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) // The "must be documented" error message is only given before the first // patch since that's the only place where the documentation is expected. @@ -661,7 +661,7 @@ func (s *Suite) Test_PatchChecker_checkConfigure__no_GNU(c *check.C) { "-old line", "+: Avoid regenerating within pkgsrc") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) // No warning since configure.sh is probably not a GNU-style // configure file. @@ -682,7 +682,7 @@ func (s *Suite) Test_PatchChecker_checkConfigure__GNU(c *check.C) { "-old line", "+: Avoid regenerating within pkgsrc") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "ERROR: ~/patch-aa:9: This code must not be included in patches.") @@ -705,7 +705,7 @@ func (s *Suite) Test_PatchChecker_checkConfigure__configure_in(c *check.C) { "-old line", "+: Avoid regenerating within pkgsrc") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "ERROR: ~/patch-aa:9: This code must not be included in patches.") @@ -728,7 +728,7 @@ func (s *Suite) Test_PatchChecker_checkConfigure__configure_ac(c *check.C) { "-old line", "+: Avoid regenerating within pkgsrc") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "ERROR: ~/patch-aa:9: This code must not be included in patches.") @@ -750,7 +750,7 @@ func (s *Suite) Test_PatchChecker_checktextCvsID(c *check.C) { "+new line $varname", " $"+"Author: authorship $") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "WARN: ~/patch-aa:7: Found CVS tag \"$"+"Id$\". Please remove it.", @@ -777,7 +777,7 @@ func (s *Suite) Test_PatchChecker_isEmptyLine(c *check.C) { "-old", "+new") - CheckLinesPatch(lines) + CheckLinesPatch(lines, nil) t.CheckOutputLines( "ERROR: patch-aa:8: Each patch must be documented.") diff --git a/pkgtools/pkglint/files/path_test.go b/pkgtools/pkglint/files/path_test.go index 4cdc95e3ae2..ddcfed4d84c 100644 --- a/pkgtools/pkglint/files/path_test.go +++ b/pkgtools/pkglint/files/path_test.go @@ -396,7 +396,7 @@ func (s *Suite) Test_Path_JoinClean(c *check.C) { func (s *Suite) Test_Path_JoinNoClean(c *check.C) { t := s.Init(c) - test := func(p, rel RelPath, result RelPath) { + test := func(p Path, rel RelPath, result Path) { t.CheckEquals(p.JoinNoClean(rel), result) } @@ -404,6 +404,7 @@ func (s *Suite) Test_Path_JoinNoClean(c *check.C) { test("dir", "///file", "dir////file") test("dir/./../dir/", "///file", "dir/./../dir/////file") test("dir", "..", "dir/..") + test(".", "sub", "./sub") } func (s *Suite) Test_Path_Clean(c *check.C) { diff --git a/pkgtools/pkglint/files/pkglint.go b/pkgtools/pkglint/files/pkglint.go index 3c9998243d6..b8bef21d1df 100644 --- a/pkgtools/pkglint/files/pkglint.go +++ b/pkgtools/pkglint/files/pkglint.go @@ -9,7 +9,6 @@ import ( tracePkg "netbsd.org/pkglint/trace" "os" "os/user" - "path" "runtime" "runtime/debug" "runtime/pprof" @@ -21,9 +20,8 @@ const confVersion = "@VERSION@" // Pkglint is a container for all global variables of this Go package. type Pkglint struct { - Opts CmdOpts // Command line options. - Pkgsrc Pkgsrc // Global data, mostly extracted from mk/*. - Pkg *Package // The package that is currently checked, or nil. + Opts CmdOpts // Command line options. + Pkgsrc Pkgsrc // Global data, mostly extracted from mk/*. Todo CurrPathQueue // The files or directories that still need to be checked. @@ -204,7 +202,7 @@ func (pkglint *Pkglint) prepareMainLoop() { firstDir = firstDir.DirNoClean() } - relTopdir := findPkgsrcTopdir(firstDir) + relTopdir := pkglint.findPkgsrcTopdir(firstDir) if relTopdir.IsEmpty() { // If the first argument to pkglint is not inside a pkgsrc tree, // pkglint doesn't know where to load the infrastructure files from, @@ -321,7 +319,7 @@ func (pkglint *Pkglint) checkMode(dirent CurrPath, mode os.FileMode) { pkglint.Wip = pkgsrcRel.HasPrefixPath("wip") pkglint.Infrastructure = pkgsrcRel.HasPrefixPath("mk") - pkgsrcdir := findPkgsrcTopdir(dir) + pkgsrcdir := pkglint.findPkgsrcTopdir(dir) if pkgsrcdir.IsEmpty() { G.Logger.TechErrorf("", "Cannot determine the pkgsrc root directory for %q.", @@ -331,7 +329,7 @@ func (pkglint *Pkglint) checkMode(dirent CurrPath, mode os.FileMode) { if isReg { pkglint.checkExecutable(dirent, mode) - pkglint.checkReg(dirent, basename, pkgsrcRel.Count()) + pkglint.checkReg(dirent, basename, pkgsrcRel.Count(), nil) return } @@ -358,13 +356,12 @@ func (pkglint *Pkglint) checkdirPackage(dir CurrPath) { defer trace.Call(dir)() } - pkglint.Pkg = NewPackage(dir) - defer func() { pkglint.Pkg = nil }() - pkglint.Pkg.Check() + pkg := NewPackage(dir) + pkg.Check() } // Returns the pkgsrc top-level directory, relative to the given directory. -func findPkgsrcTopdir(dirname CurrPath) RelPath { +func (*Pkglint) findPkgsrcTopdir(dirname CurrPath) RelPath { for _, dir := range [...]RelPath{".", "..", "../..", "../../.."} { if dirname.JoinNoClean(dir).JoinNoClean("mk/bsd.pkg.mk").IsFile() { return dir @@ -373,30 +370,36 @@ func findPkgsrcTopdir(dirname CurrPath) RelPath { return "" } -func resolveVariableRefs(mklines *MkLines, text string) (resolved string) { +func resolveVariableRefs(text string, mklines *MkLines, pkg *Package) string { // TODO: How does this fit into the Scope type, which is newer than this function? - if !containsVarRef(text) { + if !containsVarUse(text) { return text } + if pkg == nil { + pkg = mklines.pkg + } + visited := make(map[string]bool) // To prevent endless loops - replacer := func(m string) string { + replace := func(m string) string { varname := m[2 : len(m)-1] if !visited[varname] { visited[varname] = true - if G.Pkg != nil { - if value, ok := G.Pkg.vars.LastValueFound(varname); ok { - return value - } - } + if mklines != nil { // TODO: At load time, use mklines.loadVars instead. if value, ok := mklines.allVars.LastValueFound(varname); ok { return value } } + + if pkg != nil { + if value, ok := pkg.vars.LastValueFound(varname); ok { + return value + } + } } return "${" + varname + "}" } @@ -404,7 +407,7 @@ func resolveVariableRefs(mklines *MkLines, text string) (resolved string) { str := text for { // TODO: Replace regular expression with full parser. - replaced := replaceAllFunc(str, `\$\{([\w.]+)\}`, replacer) + replaced := replaceAllFunc(str, `\$\{([\w.]+)\}`, replace) if replaced == str { if trace.Tracing && str != text { trace.Stepf("resolveVariableRefs %q => %q", text, replaced) @@ -466,7 +469,7 @@ func CheckLinesDescr(lines *Lines) { SaveAutofixChanges(lines) } -func CheckLinesMessage(lines *Lines) { +func CheckLinesMessage(lines *Lines, pkg *Package) { if trace.Tracing { defer trace.Call(lines.Filename)() } @@ -476,7 +479,7 @@ func CheckLinesMessage(lines *Lines) { // // If the need arises, some of the checks may be activated again, but // that requires more sophisticated code. - if G.Pkg != nil && G.Pkg.vars.IsDefined("MESSAGE_SRC") { + if pkg != nil && pkg.vars.IsDefined("MESSAGE_SRC") { return } @@ -524,18 +527,18 @@ func CheckLinesMessage(lines *Lines) { SaveAutofixChanges(lines) } -func CheckFileMk(filename CurrPath) { +func CheckFileMk(filename CurrPath, pkg *Package) { if trace.Tracing { defer trace.Call(filename)() } - mklines := LoadMk(filename, NotEmpty|LogErrors) + mklines := LoadMk(filename, pkg, NotEmpty|LogErrors) if mklines == nil { return } - if G.Pkg != nil { - G.Pkg.checkFileMakefileExt(filename) + if pkg != nil { + pkg.checkFileMakefileExt(filename) } mklines.Check() @@ -545,7 +548,7 @@ func CheckFileMk(filename CurrPath) { // checkReg checks the given regular file. // depth is 3 for files in the package directory, and 4 or more for files // deeper in the directory hierarchy, such as in files/ or patches/. -func (pkglint *Pkglint) checkReg(filename CurrPath, basename string, depth int) { +func (pkglint *Pkglint) checkReg(filename CurrPath, basename string, depth int, pkg *Package) { if depth == 3 && !pkglint.Wip { if contains(basename, "TODO") { @@ -568,10 +571,10 @@ func (pkglint *Pkglint) checkReg(filename CurrPath, basename string, depth int) switch { case basename == "ALTERNATIVES": - CheckFileAlternatives(filename) + CheckFileAlternatives(filename, pkg) case basename == "buildlink3.mk": - if mklines := LoadMk(filename, NotEmpty|LogErrors); mklines != nil { + if mklines := LoadMk(filename, pkg, NotEmpty|LogErrors); mklines != nil { CheckLinesBuildlink3Mk(mklines) } @@ -582,7 +585,7 @@ func (pkglint *Pkglint) checkReg(filename CurrPath, basename string, depth int) case basename == "distinfo": if lines := Load(filename, NotEmpty|LogErrors); lines != nil { - CheckLinesDistinfo(G.Pkg, lines) + CheckLinesDistinfo(pkg, lines) } case basename == "DEINSTALL" || basename == "INSTALL": @@ -590,17 +593,17 @@ func (pkglint *Pkglint) checkReg(filename CurrPath, basename string, depth int) case hasPrefix(basename, "MESSAGE"): if lines := Load(filename, NotEmpty|LogErrors); lines != nil { - CheckLinesMessage(lines) + CheckLinesMessage(lines, pkg) } case basename == "options.mk": - if mklines := LoadMk(filename, NotEmpty|LogErrors); mklines != nil { + if mklines := LoadMk(filename, pkg, NotEmpty|LogErrors); mklines != nil { CheckLinesOptionsMk(mklines) } case matches(basename, `^patch-[-\w.~+]*\w$`): if lines := Load(filename, NotEmpty|LogErrors); lines != nil { - CheckLinesPatch(lines) + CheckLinesPatch(lines, pkg) } case filename.DirNoClean().Base() == "patches" && matches(filename.Base(), `^manual[^/]*$`): @@ -613,11 +616,11 @@ func (pkglint *Pkglint) checkReg(filename CurrPath, basename string, depth int) case (hasPrefix(basename, "Makefile") || hasSuffix(basename, ".mk")) && !G.Pkgsrc.Rel(filename).AsPath().ContainsPath("files"): - CheckFileMk(filename) + CheckFileMk(filename, pkg) case hasPrefix(basename, "PLIST"): if lines := Load(filename, NotEmpty|LogErrors); lines != nil { - CheckLinesPlist(G.Pkg, lines) + CheckLinesPlist(pkg, lines) } case contains(basename, "README"): @@ -635,7 +638,7 @@ func (pkglint *Pkglint) checkReg(filename CurrPath, basename string, depth int) NewLineWhole(filename).Warnf("Only packages in regress/ may have spec files.") } - case pkglint.matchesLicenseFile(basename): + case pkg != nil && pkg.matchesLicenseFile(basename): break default: @@ -643,15 +646,6 @@ func (pkglint *Pkglint) checkReg(filename CurrPath, basename string, depth int) } } -func (pkglint *Pkglint) matchesLicenseFile(basename string) bool { - if pkglint.Pkg == nil { - return false - } - - licenseFile := pkglint.Pkg.vars.LastValue("LICENSE_FILE") - return basename == path.Base(licenseFile) -} - func (pkglint *Pkglint) checkExecutable(filename CurrPath, mode os.FileMode) { if mode.Perm()&0111 == 0 { // Not executable at all. diff --git a/pkgtools/pkglint/files/pkglint_test.go b/pkgtools/pkglint/files/pkglint_test.go index 8239505f8ed..bd9971177a6 100644 --- a/pkgtools/pkglint/files/pkglint_test.go +++ b/pkgtools/pkglint/files/pkglint_test.go @@ -678,12 +678,12 @@ func (s *Suite) Test_resolveVariableRefs__circular_reference(c *check.C) { t := s.Init(c) mkline := t.NewMkLine("filename.mk", 1, "VAR=\t1:${VAR}+ 2:${VAR}") - G.Pkg = NewPackage(t.File("category/pkgbase")) - G.Pkg.vars.Define("VAR", mkline) + pkg := NewPackage(t.File("category/pkgbase")) + pkg.vars.Define("VAR", mkline) // TODO: It may be better to define MkLines.Resolve and Package.Resolve, // to clearly state the scope of the involved variables. - resolved := resolveVariableRefs(nil, "the a:${VAR} b:${VAR}") + resolved := resolveVariableRefs("the a:${VAR} b:${VAR}", nil, pkg) // TODO: The ${VAR} after "b:" should also be expanded since there // is no recursion. @@ -696,19 +696,34 @@ func (s *Suite) Test_resolveVariableRefs__multilevel(c *check.C) { mkline1 := t.NewMkLine("filename.mk", 10, "FIRST=\t${SECOND}") mkline2 := t.NewMkLine("filename.mk", 11, "SECOND=\t${THIRD}") mkline3 := t.NewMkLine("filename.mk", 12, "THIRD=\tgot it") - G.Pkg = NewPackage(t.File("category/pkgbase")) - G.Pkg.vars.Define("FIRST", mkline1) - G.Pkg.vars.Define("SECOND", mkline2) - G.Pkg.vars.Define("THIRD", mkline3) + pkg := NewPackage(t.File("category/pkgbase")) + pkg.vars.Define("FIRST", mkline1) + pkg.vars.Define("SECOND", mkline2) + pkg.vars.Define("THIRD", mkline3) // TODO: Add a similar test in which some of the variables are defined // conditionally or with differing values, just to see what pkglint does // in such a case. - resolved := resolveVariableRefs(nil, "you ${FIRST}") + resolved := resolveVariableRefs("you ${FIRST}", nil, pkg) t.CheckEquals(resolved, "you got it") } +func (s *Suite) Test_resolveVariableRefs__scope_precedence(c *check.C) { + t := s.Init(c) + + mklines := t.NewMkLines("filename.mk", + MkCvsID, + "ORIGIN=\tfilename.mk") + mklines.collectVariables() + pkg := NewPackage(t.File("category/package")) + pkg.vars.Define("ORIGIN", t.NewMkLine("other.mk", 123, "ORIGIN=\tpackage")) + + resolved := resolveVariableRefs("From ${ORIGIN}", mklines, pkg) + + t.CheckEquals(resolved, "From filename.mk") +} + // Usually, a dot in a variable name means a parameterized form. // In this case, it is part of a version number. Resolving these // variables from the scope works nevertheless. @@ -716,10 +731,10 @@ func (s *Suite) Test_resolveVariableRefs__special_chars(c *check.C) { t := s.Init(c) mkline := t.NewMkLine("filename.mk", 10, "_=x11") - G.Pkg = NewPackage(t.File("category/pkg")) - G.Pkg.vars.Define("GST_PLUGINS0.10_TYPE", mkline) + pkg := NewPackage(t.File("category/pkg")) + pkg.vars.Define("GST_PLUGINS0.10_TYPE", mkline) - resolved := resolveVariableRefs(nil, "gst-plugins0.10-${GST_PLUGINS0.10_TYPE}/distinfo") + resolved := resolveVariableRefs("gst-plugins0.10-${GST_PLUGINS0.10_TYPE}/distinfo", nil, pkg) t.CheckEquals(resolved, "gst-plugins0.10-x11/distinfo") } @@ -805,7 +820,7 @@ func (s *Suite) Test_CheckLinesMessage__one_line_of_text(c *check.C) { lines := t.NewLines("MESSAGE", "one line") - CheckLinesMessage(lines) + CheckLinesMessage(lines, nil) t.CheckOutputLines( "WARN: MESSAGE:1: File too short.") @@ -817,7 +832,7 @@ func (s *Suite) Test_CheckLinesMessage__one_hline(c *check.C) { lines := t.NewLines("MESSAGE", strings.Repeat("=", 75)) - CheckLinesMessage(lines) + CheckLinesMessage(lines, nil) t.CheckOutputLines( "WARN: MESSAGE:1: File too short.") @@ -833,7 +848,7 @@ func (s *Suite) Test_CheckLinesMessage__malformed(c *check.C) { "4", "5") - CheckLinesMessage(lines) + CheckLinesMessage(lines, nil) t.CheckOutputLines( "WARN: MESSAGE:1: Expected a line of exactly 75 \"=\" characters.", @@ -852,7 +867,7 @@ func (s *Suite) Test_CheckLinesMessage__autofix(c *check.C) { "4", "5") - CheckLinesMessage(lines) + CheckLinesMessage(lines, nil) t.CheckOutputLines( "AUTOFIX: ~/MESSAGE:1: Inserting a line \"=============================="+ @@ -894,7 +909,7 @@ func (s *Suite) Test_CheckLinesMessage__common(c *check.C) { func (s *Suite) Test_CheckFileMk__enoent(c *check.C) { t := s.Init(c) - CheckFileMk(t.File("filename.mk")) + CheckFileMk(t.File("filename.mk"), nil) t.CheckOutputLines( "ERROR: ~/filename.mk: Cannot be read.") @@ -923,12 +938,12 @@ func (s *Suite) Test_Pkglint_checkReg__file_not_found(c *check.C) { t.Chdir(".") - G.checkReg("buildlink3.mk", "buildlink3.mk", 3) - G.checkReg("DESCR", "DESCR", 3) - G.checkReg("distinfo", "distinfo", 3) - G.checkReg("MESSAGE", "MESSAGE", 3) - G.checkReg("patches/patch-aa", "patch-aa", 3) - G.checkReg("PLIST", "PLIST", 3) + G.checkReg("buildlink3.mk", "buildlink3.mk", 3, nil) + G.checkReg("DESCR", "DESCR", 3, nil) + G.checkReg("distinfo", "distinfo", 3, nil) + G.checkReg("MESSAGE", "MESSAGE", 3, nil) + G.checkReg("patches/patch-aa", "patch-aa", 3, nil) + G.checkReg("PLIST", "PLIST", 3, nil) t.CheckOutputLines( "ERROR: buildlink3.mk: Cannot be read.", @@ -946,7 +961,7 @@ func (s *Suite) Test_Pkglint_checkReg__no_tracing(c *check.C) { t.Chdir(".") t.DisableTracing() - G.checkReg("patches/manual-aa", "manual-aa", 4) + G.checkReg("patches/manual-aa", "manual-aa", 4, nil) t.CheckOutputEmpty() } @@ -1009,16 +1024,17 @@ func (s *Suite) Test_Pkglint_checkReg__readme_and_todo(c *check.C) { "SHA1 (patch-README) = ebbf34b0641bcb508f17d5a27f2bf2a536d810ac") // Copy category/package/** to wip/package. + // TODO: Extract into Tester.CopyAll. err := filepath.Walk( t.File("category/package").String(), func(pathname string, info os.FileInfo, err error) error { if info.Mode().IsRegular() { src := filepath.ToSlash(pathname) dst := strings.Replace(src, "category/package", "wip/package", 1) - text, e := ioutil.ReadFile(src) + data, e := ioutil.ReadFile(src) c.Check(e, check.IsNil) _ = os.MkdirAll(path.Dir(dst), 0700) - e = ioutil.WriteFile(dst, []byte(text), 0600) + e = ioutil.WriteFile(dst, data, 0600) c.Check(e, check.IsNil) } return err @@ -1047,7 +1063,7 @@ func (s *Suite) Test_Pkglint_checkReg__unknown_file_in_patches(c *check.C) { t.CreateFileDummyPatch("category/Makefile/patches/index") - G.checkReg(t.File("category/Makefile/patches/index"), "index", 4) + G.checkReg(t.File("category/Makefile/patches/index"), "index", 4, nil) t.CheckOutputLines( "WARN: ~/category/Makefile/patches/index: " + @@ -1060,7 +1076,7 @@ func (s *Suite) Test_Pkglint_checkReg__patch_for_Makefile_fragment(c *check.C) { t.CreateFileDummyPatch("category/package/patches/patch-compiler.mk") t.Chdir("category/package") - G.checkReg(t.File("patches/patch-compiler.mk"), "patch-compiler.mk", 4) + G.checkReg(t.File("patches/patch-compiler.mk"), "patch-compiler.mk", 4, nil) t.CheckOutputEmpty() } @@ -1070,7 +1086,7 @@ func (s *Suite) Test_Pkglint_checkReg__file_in_files(c *check.C) { t.CreateFileLines("category/package/files/index") - G.checkReg(t.File("category/package/files/index"), "index", 4) + G.checkReg(t.File("category/package/files/index"), "index", 4, nil) // These files are ignored since they could contain anything. t.CheckOutputEmpty() @@ -1082,8 +1098,8 @@ func (s *Suite) Test_Pkglint_checkReg__spec(c *check.C) { t.CreateFileLines("category/package/spec") t.CreateFileLines("regress/package/spec") - G.checkReg(t.File("category/package/spec"), "spec", 3) - G.checkReg(t.File("regress/package/spec"), "spec", 3) + G.checkReg(t.File("category/package/spec"), "spec", 3, nil) + G.checkReg(t.File("regress/package/spec"), "spec", 3, nil) t.CheckOutputLines( "WARN: ~/category/package/spec: Only packages in regress/ may have spec files.") diff --git a/pkgtools/pkglint/files/pkgsrc.go b/pkgtools/pkglint/files/pkgsrc.go index ee4c80e6ba0..c46c27c6acd 100644 --- a/pkgtools/pkglint/files/pkgsrc.go +++ b/pkgtools/pkglint/files/pkgsrc.go @@ -94,23 +94,30 @@ func (src *Pkgsrc) loadMasterSites() { mklines := src.LoadMk("mk/fetch/sites.mk", MustSucceed|NotEmpty) for _, mkline := range mklines.mklines { - if mkline.IsVarassign() { - varname := mkline.Varname() - // TODO: Give a plausible reason for the MASTER_SITE_BACKUP exception. - if hasPrefix(varname, "MASTER_SITE_") && varname != "MASTER_SITE_BACKUP" { - for _, url := range mkline.ValueFields(mkline.Value()) { - if matches(url, `^(?:http://|https://|ftp://)`) { - src.registerMasterSite(varname, url) - } - } + if !mkline.IsVarassign() { + continue + } + varname := mkline.Varname() - // TODO: register variable type, to avoid redundant definitions in vardefs.go. + // MASTER_SITE_BACKUP is only used internally and should + // not appear in package definitions since it is not the + // primary, official source for getting the files. + if varname == "MASTER_SITE_BACKUP" { + continue + } + if !hasPrefix(varname, "MASTER_SITE_") { + continue + } + + for _, url := range mkline.ValueFields(mkline.Value()) { + if matches(url, `^(?:http://|https://|ftp://)`) { + src.registerMasterSite(varname, url) } } } // Explicitly allowed, although not defined in mk/fetch/sites.mk. - // TODO: Document where this definition comes from and why it is good. + // It is defined in mk/fetch/fetch.mk instead. src.registerMasterSite("MASTER_SITE_LOCAL", "ftp://ftp.NetBSD.org/pub/pkgsrc/distfiles/LOCAL_PORTS/") if trace.Tracing { @@ -150,8 +157,9 @@ func (src *Pkgsrc) loadDocChanges() { var filenames []RelPath for _, file := range files { filename := file.Name() - if matches(filename, `^CHANGES-20\d\d$`) && filename >= "CHANGES-2011" { // XXX: Why 2011? - filenames = append(filenames, NewRelPathString(filename)) // XXX: low-level API + // Files before 2011 are too far in the past to be still relevant today. + if matches(filename, `^CHANGES-20\d\d$`) && filename >= "CHANGES-2011" { + filenames = append(filenames, NewRelPathString(filename)) } } @@ -407,7 +415,7 @@ func (src *Pkgsrc) loadTools() { toolFiles := []RelPath{"defaults.mk"} { toc := src.File("mk/tools/bsd.tools.mk") - mklines := LoadMk(toc, MustSucceed|NotEmpty) + mklines := LoadMk(toc, nil, MustSucceed|NotEmpty) for _, mkline := range mklines.mklines { if mkline.IsInclude() { includedFile := mkline.IncludedFile() @@ -669,22 +677,21 @@ func (src *Pkgsrc) loadUntypedVars() { } handleMkFile := func(path CurrPath) { - mklines := LoadMk(path, MustSucceed) + mklines := LoadMk(path, nil, MustSucceed) mklines.collectVariables() mklines.collectUsedVariables() - for varname, mkline := range mklines.allVars.firstDef { - define(varnameCanon(varname), mkline) - } - for varname, mkline := range mklines.allVars.used { + def := func(varname string, mkline *MkLine) { define(varnameCanon(varname), mkline) } + forEachStringMkLine(mklines.allVars.firstDef, def) + forEachStringMkLine(mklines.allVars.used, def) } handleFile := func(pathName string, info os.FileInfo, err error) error { assertNil(err, "handleFile %q", pathName) baseName := info.Name() if info.Mode().IsRegular() && (hasSuffix(baseName, ".mk") || baseName == "mk.conf") { - handleMkFile(NewCurrPathSlash(pathName)) // XXX: This is too deep to handle os-specific paths + handleMkFile(NewCurrPathSlash(pathName)) } return nil } @@ -796,7 +803,7 @@ func (src *Pkgsrc) ListVersions(category PkgsrcPath, re regex.Pattern, repl stri assert(hasSuffix(string(re), "$")) } - // TODO: Maybe convert cache key to a struct, to save allocations. + // XXX: Maybe convert cache key to a struct, to save allocations. cacheKey := category.String() + "/" + string(re) + " => " + repl if latest, found := src.listVersions[cacheKey]; found { return latest @@ -1027,7 +1034,7 @@ func (src *Pkgsrc) LoadMkExisting(filename PkgsrcPath) *MkLines { // LoadMk loads the Makefile relative to the pkgsrc top directory. func (src *Pkgsrc) LoadMk(filename PkgsrcPath, options LoadOptions) *MkLines { - return LoadMk(src.File(filename), options) + return LoadMk(src.File(filename), nil, options) } // Load loads the file relative to the pkgsrc top directory. diff --git a/pkgtools/pkglint/files/pkgsrc_test.go b/pkgtools/pkglint/files/pkgsrc_test.go index 98179603ea6..944d86cace2 100644 --- a/pkgtools/pkglint/files/pkgsrc_test.go +++ b/pkgtools/pkglint/files/pkgsrc_test.go @@ -163,21 +163,29 @@ func (s *Suite) Test_Pkgsrc_loadDocChangesFromFile(c *check.C) { changes := G.Pkgsrc.loadDocChangesFromFile(t.File("doc/CHANGES-2018")) - c.Assert(changes, check.HasLen, 7) // TODO: refactor to CheckDeepEquals - t.CheckEquals(*changes[0], Change{changes[0].Location, - Added, "category/package", "1.0", "author1", "2015-01-01"}) - t.CheckEquals(*changes[1], Change{changes[1].Location, - Updated, "category/package", "1.5", "author2", "2018-01-02"}) - t.CheckEquals(*changes[2], Change{changes[2].Location, - Renamed, "category/package", "category/pkg", "author3", "2018-01-03"}) - t.CheckEquals(*changes[3], Change{changes[3].Location, - Moved, "category/package", "other/package", "author4", "2018-01-04"}) - t.CheckEquals(*changes[4], Change{changes[4].Location, - Removed, "category/package", "", "author5", "2018-01-09"}) - t.CheckEquals(*changes[5], Change{changes[5].Location, - Removed, "category/package", "category/package2", "author6", "2018-01-06"}) - t.CheckEquals(*changes[6], Change{changes[6].Location, - Downgraded, "category/package", "1.2", "author7", "2018-01-07"}) + t.CheckDeepEquals( + changes, []*Change{ + {changes[0].Location, + Added, "category/package", "1.0", + "author1", "2015-01-01"}, + {changes[1].Location, + Updated, "category/package", "1.5", + "author2", "2018-01-02"}, + {changes[2].Location, + Renamed, "category/package", "category/pkg", + "author3", "2018-01-03"}, + {changes[3].Location, + Moved, "category/package", "other/package", + "author4", "2018-01-04"}, + {changes[4].Location, + Removed, "category/package", "", + "author5", "2018-01-09"}, + {changes[5].Location, + Removed, "category/package", "category/package2", + "author6", "2018-01-06"}, + {changes[6].Location, + Downgraded, "category/package", "1.2", + "author7", "2018-01-07"}}) t.CheckOutputLines( "WARN: ~/doc/CHANGES-2018:1: Year \"2015\" for category/package does not match the filename CHANGES-2018.", diff --git a/pkgtools/pkglint/files/plist.go b/pkgtools/pkglint/files/plist.go index bd4a26cf9af..c1283146b0d 100644 --- a/pkgtools/pkglint/files/plist.go +++ b/pkgtools/pkglint/files/plist.go @@ -239,7 +239,7 @@ func (ck *PlistChecker) checkPathNonAscii(pline *PlistLine) { text := pline.text lex := textproc.NewLexer(text) - lex.NextBytesFunc(func(b byte) bool { return b >= ' ' && b <= '~' }) + lex.SkipBytesFunc(func(b byte) bool { return b >= ' ' && b <= '~' }) ascii := lex.EOF() switch { @@ -472,7 +472,7 @@ func (pline *PlistLine) HasPlainPath() bool { text := pline.text return text != "" && plistLineStart.Contains(text[0]) && - !containsVarRef(text) + !containsVarUse(text) } func (pline *PlistLine) CheckTrailingWhitespace() { diff --git a/pkgtools/pkglint/files/plist_test.go b/pkgtools/pkglint/files/plist_test.go index 58b1fdfc205..cf89b001ce0 100644 --- a/pkgtools/pkglint/files/plist_test.go +++ b/pkgtools/pkglint/files/plist_test.go @@ -5,7 +5,7 @@ import "gopkg.in/check.v1" func (s *Suite) Test_CheckLinesPlist(c *check.C) { t := s.Init(c) - G.Pkg = NewPackage(t.File("category/pkgbase")) + pkg := NewPackage(t.File("category/pkgbase")) lines := t.NewLines("PLIST", "bin/i386/6c", "bin/program", @@ -27,7 +27,7 @@ func (s *Suite) Test_CheckLinesPlist(c *check.C) { "share/tzinfo", "/absolute") - CheckLinesPlist(G.Pkg, lines) + CheckLinesPlist(pkg, lines) t.CheckOutputLines( "ERROR: PLIST:1: Expected \"@comment $"+"NetBSD$\".", @@ -67,13 +67,13 @@ func (s *Suite) Test_CheckLinesPlist__single_file_no_comment(c *check.C) { func (s *Suite) Test_CheckLinesPlist__multiple_libtool_libraries(c *check.C) { t := s.Init(c) - G.Pkg = NewPackage(t.File("category/pkgbase")) + pkg := NewPackage(t.File("category/pkgbase")) lines := t.NewLines("PLIST", PlistCvsID, "lib/libc.la", "lib/libm.la") - CheckLinesPlist(G.Pkg, lines) + CheckLinesPlist(pkg, lines) t.CheckOutputLines( "WARN: PLIST:2: Packages that install libtool libraries should define USE_LIBTOOL.") @@ -121,12 +121,12 @@ func (s *Suite) Test_CheckLinesPlist__common_end_without_common(c *check.C) { func (s *Suite) Test_CheckLinesPlist__condition(c *check.C) { t := s.Init(c) - G.Pkg = NewPackage(t.File("category/pkgbase")) + pkg := NewPackage(t.File("category/pkgbase")) lines := t.NewLines("PLIST", PlistCvsID, "${PLIST.bincmds}bin/subdir/command") - CheckLinesPlist(G.Pkg, lines) + CheckLinesPlist(pkg, lines) t.CheckOutputLines( "WARN: PLIST:2: The bin/ directory should not have subdirectories.") @@ -813,10 +813,10 @@ func (s *Suite) Test_PlistChecker_checkPathLib(c *check.C) { "lib/liberty-1.0.la", "lib/locale/de_DE/liberty.mo", "lib/package/liberty-1.0.so") - G.Pkg = NewPackage(t.File("category/package")) - G.Pkg.EffectivePkgbase = "package" + pkg := NewPackage(t.File("category/package")) + pkg.EffectivePkgbase = "package" - CheckLinesPlist(G.Pkg, lines) + CheckLinesPlist(pkg, lines) t.CheckOutputLines( "ERROR: ~/PLIST:2: Only the libiconv package may install lib/charset.alias.", @@ -877,7 +877,7 @@ func (s *Suite) Test_PlistChecker_checkPathMan(c *check.C) { func (s *Suite) Test_PlistChecker_checkPathMan__gz(c *check.C) { t := s.Init(c) - G.Pkg = NewPackage(t.File("category/package")) + pkg := NewPackage(t.File("category/package")) t.Chdir("category/package") doTest := func(bool) { @@ -885,7 +885,7 @@ func (s *Suite) Test_PlistChecker_checkPathMan__gz(c *check.C) { PlistCvsID, "man/man3/strerror.3.gz") - CheckLinesPlist(G.Pkg, lines) + CheckLinesPlist(pkg, lines) } t.ExpectDiagnosticsAutofix( @@ -905,10 +905,10 @@ func (s *Suite) Test_PlistChecker_checkPathShare(c *check.C) { "share/icons/open_24x24.svg", "share/info/program.1.info", "share/man/man1/program.1") - G.Pkg = NewPackage(t.File("category/package")) - G.Pkg.EffectivePkgbase = "package" + pkg := NewPackage(t.File("category/package")) + pkg.EffectivePkgbase = "package" - CheckLinesPlist(G.Pkg, lines) + CheckLinesPlist(pkg, lines) t.CheckOutputLines( "WARN: ~/PLIST:2: Use of \"share/doc/html\" is deprecated. Use \"share/doc/${PKGBASE}\" instead.", diff --git a/pkgtools/pkglint/files/shell.go b/pkgtools/pkglint/files/shell.go index f9f401e172a..72c5f7794bd 100644 --- a/pkgtools/pkglint/files/shell.go +++ b/pkgtools/pkglint/files/shell.go @@ -77,7 +77,7 @@ func (scc *SimpleCommandChecker) checkInstallCommand(shellcmd string) { defer trace.Call0()() } - if !matches(scc.mklines.target, `^(?:pre|do|post)-install$`) { + if !matches(scc.mklines.checkAllData.target, `^(?:pre|do|post)-install$`) { return } @@ -148,7 +148,7 @@ func (scc *SimpleCommandChecker) handleTool() bool { scc.mkline.Warnf("The %q tool is used but not added to USE_TOOLS.", command) } - if tool != nil && tool.MustUseVarForm && !containsVarRef(command) { + if tool != nil && tool.MustUseVarForm && !containsVarUse(command) { scc.mkline.Warnf("Please use \"${%s}\" instead of %q.", tool.Varname, command) } @@ -179,7 +179,7 @@ func (scc *SimpleCommandChecker) handleCommandVariable() bool { return true } - return G.Pkg != nil && G.Pkg.vars.IsDefinedSimilar(varname) + return scc.mklines.pkg != nil && scc.mklines.pkg.vars.IsDefinedSimilar(varname) } func (scc *SimpleCommandChecker) handleShellBuiltin() bool { @@ -277,9 +277,9 @@ func (scc *SimpleCommandChecker) checkAutoMkdirs() { } autoMkdirs := false - if G.Pkg != nil { - plistLine := G.Pkg.Plist.Dirs[prefixRel] - if plistLine != nil && !containsVarRef(plistLine.Text) { + if scc.mklines.pkg != nil { + plistLine := scc.mklines.pkg.Plist.Dirs[prefixRel] + if plistLine != nil && !containsVarUse(plistLine.Text) { autoMkdirs = true } } @@ -630,7 +630,6 @@ func (ck *ShellLineChecker) CheckShellCommandLine(shelltext string) { "to understand, since all the complexity of using sed and mv is", "hidden behind the scenes.", "", - // TODO: Provide a copy-and-paste example. sprintf("Run %q for more information.", bmakeHelp("subst"))) if contains(shelltext, "#") { line.Explain( @@ -673,7 +672,8 @@ func (ck *ShellLineChecker) checkHiddenAndSuppress(hiddenAndSuppress, rest strin case !contains(hiddenAndSuppress, "@"): // Nothing is hidden at all. - case hasPrefix(ck.MkLines.target, "show-") || hasSuffix(ck.MkLines.target, "-message"): + case hasPrefix(ck.MkLines.checkAllData.target, "show-"), + hasSuffix(ck.MkLines.checkAllData.target, "-message"): // In these targets, all commands may be hidden. case hasPrefix(rest, "#"): @@ -899,7 +899,7 @@ func (ck *ShellLineChecker) unescapeBackticks(atoms *[]*ShAtom, quoting ShQuotin } // XXX: The regular expression is a bit cheated but is good enough until - // pkglint has a real parser for all shell constructs. + // pkglint has a real parser for all shell constructs. if atom.Quoting == shqDquotBackt && matches(atom.MkText, `(^|[^\\])"`) { line.Warnf("Double quotes inside backticks inside double quotes are error prone.") line.Explain( @@ -1006,9 +1006,6 @@ func (ck *ShellLineChecker) checkVaruseToken(atoms *[]*ShAtom, quoting ShQuoting "the correct form is ${VAR:Q}'' with either leading or trailing single or double quotes.", "If the empty string should just be skipped,", "a simple ${VAR:Q} without any surrounding quotes is correct.") - - // TODO: What about single quotes? - // TODO: What about backticks? } if ck.checkVarUse { @@ -1026,8 +1023,7 @@ func (ck *ShellLineChecker) checkMultiLineComment() { } for _, line := range mkline.raw[:len(mkline.raw)-1] { - text := strings.TrimSuffix(line.textnl, "\\\n") - + text := strings.TrimSuffix(line.Text(), "\\") tokens, rest := splitIntoShellTokens(nil, text) if rest != "" { return @@ -1043,8 +1039,7 @@ func (ck *ShellLineChecker) checkMultiLineComment() { } func (ck *ShellLineChecker) warnMultiLineComment(raw *RawLine) { - text := strings.TrimSuffix(raw.textnl, "\n") - line := NewLine(ck.mkline.Filename, raw.Lineno, text, raw) + line := NewLine(ck.mkline.Filename, raw.Lineno, raw.Text(), raw) line.Warnf("The shell comment does not stop at the end of this line.") line.Explain( @@ -1084,7 +1079,7 @@ func (ck *ShellLineChecker) Explain(explanation ...string) { // Example: "word1 word2;;;" => "word1", "word2", ";;", ";" // // TODO: Document what this function should be used for. -func splitIntoShellTokens(line *Line, text string) (tokens []string, rest string) { +func splitIntoShellTokens(line Autofixer, text string) (tokens []string, rest string) { if trace.Tracing { defer trace.Call(line, text)() } diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go index 8c3ec773afb..720505b1fbd 100644 --- a/pkgtools/pkglint/files/shell_test.go +++ b/pkgtools/pkglint/files/shell_test.go @@ -11,9 +11,10 @@ import ( func (s *Suite) Test_SimpleCommandChecker_checkCommandStart__unknown_default(c *check.C) { t := s.Init(c) + var pkg *Package test := func(commandLineArg string, diagnostics ...string) { t.SetUpCommandLine(commandLineArg) - mklines := t.NewMkLines("Makefile", + mklines := t.NewMkLinesPkg("Makefile", pkg, MkCvsID, "", "MY_TOOL.i386=\t${PREFIX}/bin/tool-i386", @@ -29,7 +30,7 @@ func (s *Suite) Test_SimpleCommandChecker_checkCommandStart__unknown_default(c * } t.SetUpPackage("category/package") - G.Pkg = NewPackage(t.File("category/package")) + pkg = NewPackage(t.File("category/package")) t.Chdir("category/package") t.FinishSetUp() @@ -52,7 +53,7 @@ func (s *Suite) Test_SimpleCommandChecker_checkInstallCommand(c *check.C) { 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.checkAllData.target = "do-install" mklines.ForEach(func(mkline *MkLine) { program, err := parseShellProgram(nil, mkline.ShellCommand()) @@ -160,10 +161,10 @@ func (s *Suite) Test_SimpleCommandChecker_handleCommandVariable__parameterized(c t := s.Init(c) t.SetUpPackage("category/package") - G.Pkg = NewPackage(t.File("category/package")) + pkg := NewPackage(t.File("category/package")) t.FinishSetUp() - mklines := t.NewMkLines("Makefile", + mklines := t.NewMkLinesPkg("Makefile", pkg, MkCvsID, "", "MY_TOOL.i386=\t${PREFIX}/bin/tool-i386", @@ -184,10 +185,10 @@ func (s *Suite) Test_SimpleCommandChecker_handleCommandVariable__followed_by_lit t := s.Init(c) t.SetUpPackage("category/package") - G.Pkg = NewPackage(t.File("category/package")) + pkg := NewPackage(t.File("category/package")) t.FinishSetUp() - mklines := t.NewMkLines("Makefile", + mklines := t.NewMkLinesPkg("Makefile", pkg, MkCvsID, "", "QTDIR=\t${PREFIX}", @@ -322,8 +323,10 @@ func (s *Suite) Test_SimpleCommandChecker_checkAutoMkdirs(c *check.C) { t.SetUpTool("mkdir", "MKDIR", AtRunTime) // This is actually "mkdir -p". t.SetUpTool("unzip", "UNZIP_CMD", AtRunTime) + var pkg *Package + test := func(shellCommand string, diagnostics ...string) { - mklines := t.NewMkLines("filename.mk", + mklines := t.NewMkLinesPkg("filename.mk", pkg, "\t"+shellCommand) ck := NewShellLineChecker(mklines, mklines.mklines[0]) @@ -343,8 +346,8 @@ func (s *Suite) Test_SimpleCommandChecker_checkAutoMkdirs(c *check.C) { "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= first\" instead of \"${INSTALL} -d\".", "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= second\" instead of \"${INSTALL} -d\".") - G.Pkg = NewPackage(t.File("category/pkgbase")) - G.Pkg.Plist.Dirs["share/pkgbase"] = &PlistLine{ + pkg = NewPackage(t.File("category/pkgbase")) + pkg.Plist.Dirs["share/pkgbase"] = &PlistLine{ t.NewLine("PLIST", 123, "share/pkgbase/file"), nil, "share/pkgbase/file"} @@ -844,8 +847,9 @@ func (s *Suite) Test_ShellLineChecker_CheckShellCommandLine(c *check.C) { t.SetUpTool("mkdir", "MKDIR", AtRunTime) // This is actually "mkdir -p". t.SetUpTool("unzip", "UNZIP_CMD", AtRunTime) + var pkg *Package test := func(shellCommand string, diagnostics ...string) { - mklines := t.NewMkLines("filename.mk", + mklines := t.NewMkLinesPkg("filename.mk", pkg, "\t"+shellCommand) ck := NewShellLineChecker(mklines, mklines.mklines[0]) @@ -943,8 +947,8 @@ func (s *Suite) Test_ShellLineChecker_CheckShellCommandLine(c *check.C) { test("-${MKDIR} deeply/nested/subdir", "WARN: filename.mk:1: Using a leading \"-\" to suppress errors is deprecated.") - G.Pkg = NewPackage(t.File("category/pkgbase")) - G.Pkg.Plist.Dirs["share/pkgbase"] = &PlistLine{ + pkg = NewPackage(t.File("category/pkgbase")) + pkg.Plist.Dirs["share/pkgbase"] = &PlistLine{ t.NewLine("PLIST", 123, "share/pkgbase/file"), nil, "share/pkgbase/file"} @@ -961,7 +965,7 @@ func (s *Suite) Test_ShellLineChecker_CheckShellCommandLine(c *check.C) { test("${RUN} ${INSTALL_DATA_DIR} ${PREFIX}/share/other", "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= share/other\" instead of \"${INSTALL_DATA_DIR}\".") - G.Pkg = nil + pkg = nil // See PR 46570, item "1. It does not" // No warning about missing error checking ("set -e"). @@ -1434,6 +1438,7 @@ func (s *Suite) Test_ShellLineChecker_CheckWord(c *check.C) { "WARN: filename.mk:1: id is used but not defined.") // TODO: Since $@ refers to ${.TARGET} and not sh.argv, there is no point in checking for quotes. + // The corresponding code in ShellLineChecker.CheckWord should be removed. // TODO: Having the same tests for $$@ would be much more interesting. // The unquoted $@ takes a different code path in pkglint than the quoted $@. @@ -1536,8 +1541,8 @@ func (s *Suite) Test_ShellLineChecker_CheckWord__PKGMANDIR(c *check.C) { t.CheckOutputLines( "WARN: chat/ircII/Makefile:2: Please use ${PKGMANDIR} instead of \"man\".", - "NOTE: chat/ircII/Makefile:2: This variable value should be aligned to column 25.", - "NOTE: chat/ircII/Makefile:3: This variable value should be aligned to column 25.") + "NOTE: chat/ircII/Makefile:2: This variable value should be aligned to column 25 instead of 17.", + "NOTE: chat/ircII/Makefile:3: This variable value should be aligned to column 25 instead of 17.") } func (s *Suite) Test_ShellLineChecker_CheckWord__empty(c *check.C) { diff --git a/pkgtools/pkglint/files/shtokenizer.go b/pkgtools/pkglint/files/shtokenizer.go index c631394898f..00243b7ca7e 100644 --- a/pkgtools/pkglint/files/shtokenizer.go +++ b/pkgtools/pkglint/files/shtokenizer.go @@ -66,8 +66,7 @@ func (p *ShTokenizer) ShAtom(quoting ShQuoting) *ShAtom { p.parser.Warnf("Unclosed shell variable starting at %q.", shorten(lexer.Rest(), 20)) } else { p.parser.Warnf("Internal pkglint error in ShTokenizer.ShAtom at %q (quoting=%s).", - // TODO: shorten(lexer.Rest(), 20) - lexer.Rest(), quoting.String()) + shorten(lexer.Rest(), 20), quoting.String()) } } return atom @@ -386,7 +385,7 @@ func (p *ShTokenizer) shOperator(q ShQuoting) *ShAtom { case lexer.SkipString("||"), lexer.SkipString("&&"), lexer.SkipString(";;"), - lexer.NextBytesFunc(func(b byte) bool { return b == '\n' }) != "", + lexer.SkipBytesFunc(func(b byte) bool { return b == '\n' }), lexer.SkipByte(';'), lexer.SkipByte('('), lexer.SkipByte(')'), diff --git a/pkgtools/pkglint/files/shtokenizer_test.go b/pkgtools/pkglint/files/shtokenizer_test.go index 7e00f9ac59b..92730815645 100644 --- a/pkgtools/pkglint/files/shtokenizer_test.go +++ b/pkgtools/pkglint/files/shtokenizer_test.go @@ -512,9 +512,10 @@ func (s *Suite) Test_ShTokenizer_ShAtom(c *check.C) { subsh(operator("`")), operator(")")) - // Subshell with unbalanced parentheses, taken from src/build.sh, - // around line 160. Many shells (and pkglint) fail this test, - // therefore just don't write code like this. + // Subshell with unbalanced parentheses. Many shells (and pkglint) + // fail this test, therefore please don't write code like this. + // + // See NetBSD/src/build.sh, around line 160. test("var=$$(case x in x) still-subshell;; esac);", text("var="), subsh(subshell), subsh(text("case")), subsh(space), subsh(text("x")), subsh(space), @@ -533,6 +534,17 @@ func (s *Suite) Test_ShTokenizer_ShAtom(c *check.C) { t.CheckOutputLines( "WARN: filename.mk:1: Internal pkglint error " + "in ShTokenizer.ShAtom at \"\\\\${VAR}`\" (quoting=b).") + + // The remaining tokens are shortened in the warning. + testRest("`echo \\${VAR.123456789012345678901234567890}`", + atoms( + backt(text("`")), + backt(text("echo")), + backt(space)), + "\\${VAR.123456789012345678901234567890}`") + t.CheckOutputLines( + "WARN: filename.mk:1: Internal pkglint error in ShTokenizer.ShAtom " + + "at \"\\\\${VAR.1234567890123...\" (quoting=b).") } func (s *Suite) Test_ShTokenizer_ShAtom__quoting(c *check.C) { diff --git a/pkgtools/pkglint/files/substcontext.go b/pkgtools/pkglint/files/substcontext.go index 9dd24875997..e445255ec4f 100644 --- a/pkgtools/pkglint/files/substcontext.go +++ b/pkgtools/pkglint/files/substcontext.go @@ -7,15 +7,17 @@ import "netbsd.org/pkglint/textproc" // // See mk/subst.mk. type SubstContext struct { + // points to a block somewhere in scopes. active *substBlock scopes []*substScope once Once + pkg *Package } -func NewSubstContext() *SubstContext { - return &SubstContext{scopes: []*substScope{newSubstScope()}} +func NewSubstContext(pkg *Package) *SubstContext { + return &SubstContext{nil, []*substScope{newSubstScope()}, Once{}, pkg} } func (ctx *SubstContext) Process(mkline *MkLine) { @@ -73,11 +75,11 @@ func (ctx *SubstContext) varassign(mkline *MkLine) { } block := ctx.block() - block.varassign(mkline) + block.varassign(mkline, ctx.pkg) } func (ctx *SubstContext) varassignClasses(mkline *MkLine) { - ids := mkline.ValueFields(mkline.WithoutMakeVariables(mkline.Value())) + ids := mkline.ValueFieldsLiteral() if len(ids) == 0 { return } @@ -396,10 +398,10 @@ func newSubstBlock(id string) *substBlock { return &substBlock{id: id, conds: []*substCond{newSubstCond()}} } -func (b *substBlock) varassign(mkline *MkLine) { +func (b *substBlock) varassign(mkline *MkLine, pkg *Package) { switch mkline.Varcanon() { case "SUBST_STAGE.*": - b.varassignStage(mkline) + b.varassignStage(mkline, pkg) case "SUBST_MESSAGE.*": b.varassignMessages(mkline) case "SUBST_FILES.*": @@ -413,7 +415,7 @@ func (b *substBlock) varassign(mkline *MkLine) { } } -func (b *substBlock) varassignStage(mkline *MkLine) { +func (b *substBlock) varassignStage(mkline *MkLine, pkg *Package) { if b.isConditional() { mkline.Warnf("%s should not be defined conditionally.", mkline.Varname()) } @@ -437,8 +439,8 @@ func (b *substBlock) varassignStage(mkline *MkLine) { fix.Apply() } - if G.Pkg != nil && (value == "pre-configure" || value == "post-configure") { - if noConfigureLine := G.Pkg.vars.FirstDefinition("NO_CONFIGURE"); noConfigureLine != nil { + if pkg != nil && (value == "pre-configure" || value == "post-configure") { + if noConfigureLine := pkg.vars.FirstDefinition("NO_CONFIGURE"); noConfigureLine != nil { mkline.Warnf("SUBST_STAGE %s has no effect when NO_CONFIGURE is set (in %s).", value, mkline.RelMkLine(noConfigureLine)) mkline.Explain( diff --git a/pkgtools/pkglint/files/substcontext_test.go b/pkgtools/pkglint/files/substcontext_test.go index f7c322d5e87..cff4ea0e565 100644 --- a/pkgtools/pkglint/files/substcontext_test.go +++ b/pkgtools/pkglint/files/substcontext_test.go @@ -6,7 +6,7 @@ func (t *Tester) NewSubstAutofixTest(lines ...string) func(bool) { return func(autofix bool) { mklines := t.NewMkLines("filename.mk", lines...) mklines.collectRationale() - ctx := NewSubstContext() + ctx := NewSubstContext(nil) mklines.ForEach(ctx.Process) ctx.Finish(mklines.EOFLine()) @@ -20,7 +20,7 @@ func (t *Tester) RunSubst(lines ...string) { mklines := t.NewMkLines("filename.mk", lines...) mklines.collectRationale() - ctx := NewSubstContext() + ctx := NewSubstContext(nil) mklines.ForEach(ctx.Process) ctx.Finish(mklines.EOFLine()) @@ -206,7 +206,7 @@ func (s *Suite) Test_SubstContext_varassign__late_addition_to_unknown_class(c *c mklines := t.NewMkLines("filename.mk", "SUBST_VARS.id=\tOPSYS", "") - ctx := NewSubstContext() + ctx := NewSubstContext(nil) mklines.collectRationale() mklines.ForEach(ctx.Process) @@ -257,7 +257,7 @@ func (s *Suite) Test_SubstContext_varassign__interleaved(c *check.C) { func (s *Suite) Test_SubstContext_varassignClasses__OPSYSVARS(c *check.C) { t := s.Init(c) - ctx := NewSubstContext() + ctx := NewSubstContext(nil) // SUBST_CLASSES is added to OPSYSVARS in mk/bsd.pkg.mk. ctx.varassign(t.NewMkLine("filename.mk", 11, "SUBST_CLASSES.SunOS+=prefix")) @@ -395,7 +395,7 @@ func (s *Suite) Test_SubstContext_varassignOutsideBlock__rationale(c *check.C) { "# Rationale that is completely irrelevant.", "SUBST_SED.libs+=\t-e sahara", "") - ctx := NewSubstContext() + ctx := NewSubstContext(nil) mklines.collectRationale() mklines.ForEach(ctx.Process) @@ -772,7 +772,7 @@ func (s *Suite) Test_SubstContext_leave__nested_conditionals(c *check.C) { func (s *Suite) Test_SubstContext_activeId__SUBST_CLASSES_in_separate_paragraph(c *check.C) { t := s.Init(c) - ctx := NewSubstContext() + ctx := NewSubstContext(nil) checkNoActiveId := func() { t.CheckEquals(ctx.isActive(), false) @@ -829,7 +829,7 @@ func (s *Suite) Test_SubstContext_activeId__SUBST_CLASSES_in_separate_paragraph( func (s *Suite) Test_substScope__conditionals(c *check.C) { t := s.Init(c) - ctx := NewSubstContext() + ctx := NewSubstContext(nil) line := func(text string) { mkline := t.NewMkLine("filename.mk", 123, text) @@ -1682,7 +1682,7 @@ func (s *Suite) Test_substBlock_extractVarname(c *check.C) { func (s *Suite) Test_substBlock_isComplete__incomplete(c *check.C) { t := s.Init(c) - ctx := NewSubstContext() + ctx := NewSubstContext(nil) ctx.varassign(t.NewMkLine("filename.mk", 10, "PKGNAME=pkgname-1.0")) @@ -1712,7 +1712,7 @@ func (s *Suite) Test_substBlock_isComplete__incomplete(c *check.C) { func (s *Suite) Test_substBlock_isComplete__complete(c *check.C) { t := s.Init(c) - ctx := NewSubstContext() + ctx := NewSubstContext(nil) ctx.varassign(t.NewMkLine("filename.mk", 10, "PKGNAME=pkgname-1.0")) ctx.varassign(t.NewMkLine("filename.mk", 11, "SUBST_CLASSES+=p")) diff --git a/pkgtools/pkglint/files/textproc/lexer.go b/pkgtools/pkglint/files/textproc/lexer.go index 2c0b83fec9d..158d11bf234 100644 --- a/pkgtools/pkglint/files/textproc/lexer.go +++ b/pkgtools/pkglint/files/textproc/lexer.go @@ -138,10 +138,14 @@ func (l *Lexer) NextByte() byte { return b } +// SkipBytesFunc skips over the longest prefix consisting +// solely of bytes for which fn returns true. +func (l *Lexer) SkipBytesFunc(fn func(b byte) bool) bool { + return l.NextBytesFunc(fn) != "" +} + // NextBytesFunc chops off the longest prefix (possibly empty) consisting // solely of bytes for which fn returns true. -// -// TODO: SkipBytesFunc func (l *Lexer) NextBytesFunc(fn func(b byte) bool) string { i := 0 rest := l.rest diff --git a/pkgtools/pkglint/files/textproc/lexer_test.go b/pkgtools/pkglint/files/textproc/lexer_test.go index 220eff39ad8..0b4cafa17b0 100644 --- a/pkgtools/pkglint/files/textproc/lexer_test.go +++ b/pkgtools/pkglint/files/textproc/lexer_test.go @@ -160,6 +160,16 @@ func (s *Suite) Test_Lexer_NextByte(c *check.C) { c.Check(lexer.NextByte, check.PanicMatches, "^runtime error: index out of range.*") } +func (s *Suite) Test_Lexer_SkipBytesFunc(c *check.C) { + lexer := NewLexer("an alphanumerical string") + + c.Check(lexer.SkipBytesFunc(func(b byte) bool { return 'A' <= b && b <= 'Z' }), equals, false) + c.Check(lexer.SkipBytesFunc(func(b byte) bool { return !unicode.IsSpace(rune(b)) }), equals, true) + c.Check(lexer.SkipHspace(), equals, true) + c.Check(lexer.SkipBytesFunc(func(b byte) bool { return 'a' <= b && b <= 'z' }), equals, true) + c.Check(lexer.SkipBytesFunc(func(b byte) bool { return true }), equals, true) +} + func (s *Suite) Test_Lexer_NextBytesFunc(c *check.C) { lexer := NewLexer("an alphanumerical string") diff --git a/pkgtools/pkglint/files/tools.go b/pkgtools/pkglint/files/tools.go index 24c759b50da..7036716e47a 100644 --- a/pkgtools/pkglint/files/tools.go +++ b/pkgtools/pkglint/files/tools.go @@ -241,17 +241,17 @@ func (tr *Tools) ParseToolLine(mklines *MkLines, mkline *MkLine, fromInfrastruct } case "_TOOLS_VARNAME.*": - if !containsVarRef(varparam) { + if !containsVarUse(varparam) { tr.Define(varparam, value, mkline) } case "TOOLS_PATH.*", "_TOOLS_DEPMETHOD.*": - if !containsVarRef(varparam) { + if !containsVarUse(varparam) { tr.Define(varparam, "", mkline) } case "TOOLS_ALIASES.*": - if containsVarRef(varparam) { + if containsVarUse(varparam) { break } @@ -273,7 +273,7 @@ func (tr *Tools) ParseToolLine(mklines *MkLines, mkline *MkLine, fromInfrastruct } case "_TOOLS.*": - if !containsVarRef(varparam) { + if !containsVarUse(varparam) { tr.Define(varparam, "", mkline) for _, tool := range mkline.ValueFields(value) { tr.Define(tool, "", mkline) @@ -305,7 +305,7 @@ func (tr *Tools) addAlias(tool *Tool, alias string) { // though, this assumption cannot be made and pkglint needs to be strict. func (tr *Tools) parseUseTools(mkline *MkLine, createIfAbsent bool, addToUseTools bool) { value := mkline.Value() - if containsVarRef(value) { + if containsVarUse(value) { return } diff --git a/pkgtools/pkglint/files/toplevel.go b/pkgtools/pkglint/files/toplevel.go index 286d755f11c..4bf9a0bc43d 100644 --- a/pkgtools/pkglint/files/toplevel.go +++ b/pkgtools/pkglint/files/toplevel.go @@ -14,7 +14,7 @@ func CheckdirToplevel(dir CurrPath) { ctx := Toplevel{dir, "", nil} filename := dir.JoinNoClean("Makefile") - mklines := LoadMk(filename, NotEmpty|LogErrors) + mklines := LoadMk(filename, nil, NotEmpty|LogErrors) if mklines == nil { return } @@ -44,7 +44,7 @@ func (ctx *Toplevel) checkSubdir(mkline *MkLine) { } } - if containsVarRef(subdir.String()) || !ctx.dir.JoinNoClean(subdir).JoinNoClean("Makefile").IsFile() { + if containsVarUse(subdir.String()) || !ctx.dir.JoinNoClean(subdir).JoinNoClean("Makefile").IsFile() { return } diff --git a/pkgtools/pkglint/files/toplevel_test.go b/pkgtools/pkglint/files/toplevel_test.go index 01b70cf4753..33278d30ad3 100644 --- a/pkgtools/pkglint/files/toplevel_test.go +++ b/pkgtools/pkglint/files/toplevel_test.go @@ -31,7 +31,8 @@ func (s *Suite) Test_CheckdirToplevel(c *check.C) { // This warning is at the very end because mklines.Check() is called at the end. // Ideally it would be at the same place as the other warning from Makefile:3. - "NOTE: ~/Makefile:3: This variable value should be aligned with tabs, not spaces, to column 17.") + "NOTE: ~/Makefile:3: This variable value should be aligned "+ + "with tabs, not spaces, to column 17 instead of 10.") } func (s *Suite) Test_CheckdirToplevel__recursive(c *check.C) { @@ -106,7 +107,7 @@ func (s *Suite) Test_CheckdirToplevel__indentation(c *check.C) { t.Main("-Wall", ".") t.CheckOutputLines( - "NOTE: ~/Makefile:4: This variable value should be aligned to column 17.", + "NOTE: ~/Makefile:4: This variable value should be aligned to column 17 instead of 25.", "Looks fine.", t.Shquote("(Run \"pkglint -e -Wall %s\" to show explanations.)", "."), t.Shquote("(Run \"pkglint -fs -Wall %s\" to show what can be fixed automatically.)", "."), diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go index 31df2c79431..7242dc53cc1 100644 --- a/pkgtools/pkglint/files/util.go +++ b/pkgtools/pkglint/files/util.go @@ -93,6 +93,16 @@ func mapStr(slice []string, fn func(s string) string) []string { return result } +func filterStr(slice []string, fn func(s string) bool) []string { + result := make([]string, 0, len(slice)) + for _, str := range slice { + if fn(str) { + result = append(result, str) + } + } + return result +} + func invalidCharacters(s string, valid *textproc.ByteSet) string { var unis strings.Builder @@ -489,7 +499,7 @@ func toInt(s string, def int) int { return def } -func containsVarRef(s string) bool { +func containsVarUse(s string) bool { if !contains(s, "$") { return false } @@ -609,40 +619,47 @@ func NewScope() Scope { // Define marks the variable and its canonicalized form as defined. func (s *Scope) Define(varname string, mkline *MkLine) { - def := func(name string) { - if s.firstDef[name] == nil { - s.firstDef[name] = mkline - if trace.Tracing { - trace.Step2("Defining %q for the first time in %s", name, mkline.String()) - } - } else if trace.Tracing { - trace.Step2("Defining %q in %s", name, mkline.String()) + s.def(varname, mkline) + varcanon := varnameCanon(varname) + if varcanon != varname { + s.def(varcanon, mkline) + } +} + +func (s *Scope) def(name string, mkline *MkLine) { + if s.firstDef[name] == nil { + s.firstDef[name] = mkline + if trace.Tracing { + trace.Step2("Defining %q for the first time in %s", name, mkline.String()) } + } else if trace.Tracing { + trace.Step2("Defining %q in %s", name, mkline.String()) + } - s.lastDef[name] = mkline + s.lastDef[name] = mkline - // In most cases the defining lines are indeed variable assignments. - // Exceptions are comments from documentation sections, which still mark - // it as defined so that it doesn't produce the "used but not defined" warning; - // see MkLines.collectDocumentedVariables. - if mkline.IsVarassign() { - switch mkline.Op() { - case opAssignAppend: - s.value[name] += " " + mkline.Value() - case opAssignDefault: - // No change to the value. - case opAssignShell: - delete(s.value, name) - default: - s.value[name] = mkline.Value() - } - } + // In most cases the defining lines are indeed variable assignments. + // Exceptions are comments from documentation sections, which still mark + // it as defined so that it doesn't produce the "used but not defined" warning; + // see MkLines.collectDocumentedVariables. + if !mkline.IsVarassign() { + return } - def(varname) - varcanon := varnameCanon(varname) - if varcanon != varname { - def(varcanon) + switch mkline.Op() { + case opAssignAppend: + value := mkline.Value() + if trace.Tracing { + trace.Stepf("Scope.Define.append %s: %s = %q + %q", + &mkline.Location, name, s.value[name], value) + } + s.value[name] += " " + value + case opAssignDefault: + // No change to the value. + case opAssignShell: + delete(s.value, name) + default: + s.value[name] = mkline.Value() } } diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go index 4b4ab3bdeb9..0a30ea7c7e6 100644 --- a/pkgtools/pkglint/files/util_test.go +++ b/pkgtools/pkglint/files/util_test.go @@ -422,12 +422,11 @@ func emptyToNil(slice []string) []string { return slice } -func (s *Suite) Test_containsVarRef(c *check.C) { +func (s *Suite) Test_containsVarUse(c *check.C) { t := s.Init(c) test := func(str string, containsVar bool) { - // TODO: rename to containsVarUse - t.CheckEquals(containsVarRef(str), containsVar) + t.CheckEquals(containsVarUse(str), containsVar) } test("", false) diff --git a/pkgtools/pkglint/files/varalignblock.go b/pkgtools/pkglint/files/varalignblock.go index 1958e4b72d0..c5fe8969879 100644 --- a/pkgtools/pkglint/files/varalignblock.go +++ b/pkgtools/pkglint/files/varalignblock.go @@ -185,7 +185,7 @@ func (va *VaralignBlock) processVarassign(mkline *MkLine) { var infos []*varalignLine for i, raw := range mkline.raw { - parts := NewVaralignSplitter().split(strings.TrimSuffix(raw.textnl, "\n"), i == 0) + parts := NewVaralignSplitter().split(raw.Text(), i == 0) info := varalignLine{mkline, i, false, parts} infos = append(infos, &info) } @@ -266,12 +266,14 @@ func (va *VaralignBlock) optimalWidth() int { return minVarnameOpWidth&-8 + 8 } +// varnameOpWidths calculates the required width of varnameOp, +// without the trailing whitespace, as well as the outlier. func (va *VaralignBlock) varnameOpWidths() (int, int) { var widths bag - for _, mkinfo := range va.mkinfos { + for i, mkinfo := range va.mkinfos { if !mkinfo.isMultiEmpty() { info := mkinfo.infos[0] - widths.add(info.fixer, info.spaceBeforeValueColumn()) + widths.add(i, info.spaceBeforeValueColumn()) } } if widths.len() == 0 { @@ -281,17 +283,16 @@ func (va *VaralignBlock) varnameOpWidths() (int, int) { widths.sortDesc() longest := widths.opt(0) - longestLine := widths.key(0).(*MkLine) secondLongest := widths.opt(1) haveOutlier := secondLongest != 0 && secondLongest/8+1 < longest/8 && - !longestLine.IsMultiline() // TODO: may be too imprecise + !va.mkinfos[widths.key(0).(int)].infos[0].isContinuation() - // Minimum required width of varnameOp, without the trailing whitespace. - minVarnameOpWidth := condInt(haveOutlier, secondLongest, longest) - outlier := condInt(haveOutlier, longest, 0) - return minVarnameOpWidth, outlier + if haveOutlier { + return secondLongest, longest + } + return longest, 0 } func (va *VaralignBlock) spaceWidths(outlier int) (min, max int) { @@ -541,13 +542,20 @@ func (info *varalignLine) alignValueSingle(newWidth int) { fix := info.fixer.Autofix() if newSpace == " " { - fix.Notef("This outlier variable value should be aligned with a single space.") + fix.Notef( + "This outlier variable value should be aligned " + + "with a single space.") info.explainWrongColumn(fix) } else if hasSpace && column != oldColumn { - fix.Notef("This variable value should be aligned with tabs, not spaces, to column %d.", column+1) + fix.Notef( + "This variable value should be aligned "+ + "with tabs, not spaces, to column %d instead of %d.", + column+1, oldColumn+1) info.explainWrongColumn(fix) } else if column != oldColumn { - fix.Notef("This variable value should be aligned to column %d.", column+1) + fix.Notef( + "This variable value should be aligned to column %d instead of %d.", + column+1, oldColumn+1) info.explainWrongColumn(fix) } else { fix.Notef("Variable values should be aligned with tabs, not spaces.") @@ -583,9 +591,15 @@ func (info *varalignLine) alignValue(width int) { fix := info.fixer.Autofix() if width != oldWidth && contains(oldSpace, " ") { - fix.Notef("This variable value should be aligned with tabs, not spaces, to column %d.", width+1) + fix.Notef( + "This variable value should be aligned "+ + "with tabs, not spaces, to column %d instead of %d.", + width+1, oldWidth+1) } else if width != oldWidth { - fix.Notef("This variable value should be aligned to column %d.", width+1) // TODO: to column %d instead of %d. + fix.Notef( + "This variable value should be aligned "+ + "to column %d instead of %d.", + width+1, oldWidth+1) } else { fix.Notef("Variable values should be aligned with tabs, not spaces.") } diff --git a/pkgtools/pkglint/files/varalignblock_test.go b/pkgtools/pkglint/files/varalignblock_test.go index 90b17f739e9..b679cb34a81 100644 --- a/pkgtools/pkglint/files/varalignblock_test.go +++ b/pkgtools/pkglint/files/varalignblock_test.go @@ -181,7 +181,7 @@ func (vt *VaralignTester) checkTestName() { } for i, input := range mkline.raw { - parts := NewVaralignSplitter().split(strings.TrimSuffix(input.orignl, "\n"), i == 0) + parts := NewVaralignSplitter().split(input.Orig(), i == 0) width = 0 if parts.leadingComment != "" { describe(parts.leadingComment, "lead") @@ -228,7 +228,7 @@ func (s *Suite) Test_VaralignBlock__var_none_value(c *check.C) { vt.Internals( "20 20") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 25.") + "NOTE: Makefile:1: This variable value should be aligned to column 25 instead of 21.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\" with \"\\t\".") vt.Fixed( @@ -247,7 +247,7 @@ func (s *Suite) Test_VaralignBlock__var_space_value(c *check.C) { vt.Internals( "20 21") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25 instead of 22.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".") vt.Fixed( @@ -265,7 +265,7 @@ func (s *Suite) Test_VaralignBlock__var_spaces7_value(c *check.C) { vt.Internals( "04 07") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 9.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 9 instead of 8.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".") vt.Fixed( @@ -284,8 +284,8 @@ func (s *Suite) Test_VaralignBlock__var4_space5_value_var4_spaces6_value(c *chec "04 05", "04 06") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 9.", - "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 9.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 9 instead of 6.", + "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 9 instead of 7.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".", "AUTOFIX: Makefile:2: Replacing \" \" with \"\\t\".") @@ -329,7 +329,7 @@ func (s *Suite) Test_VaralignBlock__var_spaces_value(c *check.C) { vt.Internals( "20 23") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25 instead of 24.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".") vt.Fixed( @@ -359,7 +359,7 @@ func (s *Suite) Test_VaralignBlock__var_tsts_value(c *check.C) { vt.Internals( "20 33") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25 instead of 34.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t \\t \" with \"\\t\".") vt.Fixed( @@ -582,7 +582,7 @@ func (s *Suite) Test_VaralignBlock__var20_none_value_space_cont_indent20_value(c "20 20 26", " 20") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 25.", + "NOTE: Makefile:1: This variable value should be aligned to column 25 instead of 21.", "NOTE: Makefile:2: This continuation line should be indented with \"\\t\\t\\t\".") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\" with \"\\t\".", @@ -602,7 +602,7 @@ func (s *Suite) Test_VaralignBlock__var20_space_value_space_cont_spaces21_value( "20 21 27", " 21") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25.", + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25 instead of 22.", "NOTE: Makefile:2: This continuation line should be indented with \"\\t\\t\\t\".") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".", @@ -638,7 +638,7 @@ func (s *Suite) Test_VaralignBlock__var_spaces_value_space_cont_spaces_value(c * "20 23 29", " 23") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25.", + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25 instead of 24.", "NOTE: Makefile:2: This continuation line should be indented with \"\\t\\t\\t\".") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".", @@ -868,10 +868,10 @@ func (s *Suite) Test_VaralignBlock__var6_space_value_var7_space_value_var8_space "08 09", "09 10") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 17.", - "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 17.", - "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 17.", - "NOTE: Makefile:4: This variable value should be aligned with tabs, not spaces, to column 17.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 17 instead of 8.", + "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 17 instead of 9.", + "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 17 instead of 10.", + "NOTE: Makefile:4: This variable value should be aligned with tabs, not spaces, to column 17 instead of 11.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\\t\".", "AUTOFIX: Makefile:2: Replacing \" \" with \"\\t\\t\".", @@ -919,8 +919,8 @@ func (s *Suite) Test_VaralignBlock__var_tab8_value_var_space15_value(c *check.C) "06 08", "14 15") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 17.", - "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 17.") + "NOTE: Makefile:1: This variable value should be aligned to column 17 instead of 9.", + "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 17 instead of 16.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t\" with \"\\t\\t\".", "AUTOFIX: Makefile:2: Replacing \" \" with \"\\t\".") @@ -941,8 +941,8 @@ func (s *Suite) Test_VaralignBlock__var_tab8_value_var14_tabs40_value(c *check.C "06 08", "14 40") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 17.", - "NOTE: Makefile:2: This variable value should be aligned to column 17.") + "NOTE: Makefile:1: This variable value should be aligned to column 17 instead of 9.", + "NOTE: Makefile:2: This variable value should be aligned to column 17 instead of 41.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t\" with \"\\t\\t\".", "AUTOFIX: Makefile:2: Replacing \"\\t\\t\\t\\t\" with \"\\t\".") @@ -982,7 +982,7 @@ func (s *Suite) Test_VaralignBlock__var17_none_value(c *check.C) { vt.Internals( "17 17") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 25.") + "NOTE: Makefile:1: This variable value should be aligned to column 25 instead of 18.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\" with \"\\t\".") vt.Fixed( @@ -1023,7 +1023,7 @@ func (s *Suite) Test_VaralignBlock__var_tab8_value_var4_space_cont_tab_value(c * "04 05 05", " 08") vt.Diagnostics( - "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 9.") + "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 9 instead of 6.") vt.Autofixes( "AUTOFIX: Makefile:2: Replacing \" \" with \"\\t\".") vt.Fixed( @@ -1066,7 +1066,7 @@ func (s *Suite) Test_VaralignBlock__var14_tab_value_var4_tab_cont_tab_value(c *c "04 08 08", " 08") vt.Diagnostics( - "NOTE: Makefile:2: This variable value should be aligned to column 17.") + "NOTE: Makefile:2: This variable value should be aligned to column 17 instead of 9.") vt.Autofixes( "AUTOFIX: Makefile:2: Replacing \"\\t\" with \"\\t\\t\".") vt.Fixed( @@ -1142,10 +1142,10 @@ func (s *Suite) Test_VaralignBlock__var_tab16_value_var_space_cont_tabs24_value_ "11 32", "11 12") vt.Diagnostics( - "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 17.", + "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 17 instead of 13.", "NOTE: Makefile:3: This continuation line should be indented with \"\\t\\t\".", - "NOTE: Makefile:4: This variable value should be aligned to column 17.", - "NOTE: Makefile:5: This variable value should be aligned with tabs, not spaces, to column 17.") + "NOTE: Makefile:4: This variable value should be aligned to column 17 instead of 33.", + "NOTE: Makefile:5: This variable value should be aligned with tabs, not spaces, to column 17 instead of 13.") vt.Autofixes( "AUTOFIX: Makefile:2: Replacing \" \" with \"\\t\".", "AUTOFIX: Makefile:3: Replacing \"\\t\\t\\t\" with \"\\t\\t\".", @@ -1188,7 +1188,7 @@ func (s *Suite) Test_VaralignBlock__var_tabs16_value_var_tab24_value_space_cont_ "18 24 44", " 24") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 25.") + "NOTE: Makefile:1: This variable value should be aligned to column 25 instead of 17.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t\\t\" with \"\\t\\t\\t\".") vt.Fixed( @@ -1215,7 +1215,7 @@ func (s *Suite) Test_VaralignBlock__var_tab8_value_var_tab16_value(c *check.C) { "03 08", "15 16") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 17.") + "NOTE: Makefile:1: This variable value should be aligned to column 17 instead of 9.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t\" with \"\\t\\t\".") vt.Fixed( @@ -1282,7 +1282,7 @@ func (s *Suite) Test_VaralignBlock__escaped_varname(c *check.C) { "15 16", // 15, since the number sign is still escaped when computing the indentation "21 24") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 25.") + "NOTE: Makefile:1: This variable value should be aligned to column 25 instead of 17.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t\" with \"\\t\\t\".") vt.Fixed( @@ -1310,7 +1310,7 @@ func (s *Suite) Test_VaralignBlock__var_tab8_value_var_tab16_value_var28_space_c " 24 52", " 24") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 17.", + "NOTE: Makefile:1: This variable value should be aligned to column 17 instead of 9.", "NOTE: Makefile:4: This continuation line should be indented with \"\\t\\t\".", "NOTE: Makefile:5: This continuation line should be indented with \"\\t\\t\".") vt.Autofixes( @@ -1345,7 +1345,7 @@ func (s *Suite) Test_VaralignBlock__var_tab8_value_var_tab16_value_var28_space_c " 08 36", " 08") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 17.") + "NOTE: Makefile:1: This variable value should be aligned to column 17 instead of 9.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t\" with \"\\t\\t\".") vt.Fixed( @@ -1375,14 +1375,19 @@ func (s *Suite) Test_VaralignBlock__var_tab8_value_var_tab16_value_var28_tab_val "DISTFILES=\tdistfile-1.0.0.tar.gz", "SITES.distfile-1.0.0.tar.gz=\t${MASTER_SITES_SOURCEFORGE} \\", "\t\t\t\t${MASTER_SITES_GITHUB}") + vt.InputDetab( + "WRKSRC= ${WRKDIR}", + "DISTFILES= distfile-1.0.0.tar.gz", + "SITES.distfile-1.0.0.tar.gz= ${MASTER_SITES_SOURCEFORGE} \\", + " ${MASTER_SITES_GITHUB}") vt.Internals( "07 08", "10 16", "28 32 60", " 32") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 33.", - "NOTE: Makefile:2: This variable value should be aligned to column 33.") + "NOTE: Makefile:1: This variable value should be aligned to column 33 instead of 9.", + "NOTE: Makefile:2: This variable value should be aligned to column 33 instead of 17.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t\" with \"\\t\\t\\t\\t\".", "AUTOFIX: Makefile:2: Replacing \"\\t\" with \"\\t\\t\\t\".") @@ -1416,8 +1421,8 @@ func (s *Suite) Test_VaralignBlock__var_tab8_value_var_tab16_value_var13_space_c " 36 46", " 34") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 17.", - "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 17.", + "NOTE: Makefile:1: This variable value should be aligned to column 17 instead of 9.", + "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 17 instead of 15.", "NOTE: Makefile:4: This continuation line should be indented with \"\\t\\t\".", "NOTE: Makefile:5: This continuation line should be indented with \"\\t\\t \".", "NOTE: Makefile:6: This continuation line should be indented with \"\\t\\t\".") @@ -1456,8 +1461,8 @@ func (s *Suite) Test_VaralignBlock__continuation_mixed_indentation_in_first_line " 36 46", " 34") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 17.", - "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 17.", + "NOTE: Makefile:1: This variable value should be aligned to column 17 instead of 9.", + "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 17 instead of 35.", "NOTE: Makefile:4: This continuation line should be indented with \"\\t\\t \".", "NOTE: Makefile:5: This continuation line should be indented with \"\\t\\t\".") vt.Autofixes( @@ -1547,10 +1552,10 @@ func (s *Suite) Test_VaralignBlock__var_spaces24_value_space_cont_spaces24_value "14 16", "07 24") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 17.", + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 17 instead of 25.", "NOTE: Makefile:2: This continuation line should be indented with \"\\t\\t\".", "NOTE: Makefile:4: Variable values should be aligned with tabs, not spaces.", - "NOTE: Makefile:6: This variable value should be aligned with tabs, not spaces, to column 17.") + "NOTE: Makefile:6: This variable value should be aligned with tabs, not spaces, to column 17 instead of 25.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".", "AUTOFIX: Makefile:2: Replacing \" \" with \"\\t\\t\".", @@ -1580,9 +1585,9 @@ func (s *Suite) Test_VaralignBlock__var_tab16_value_var16_space_value_var14_tab_ "16 17", "14 16") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 25.", - "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 25.", - "NOTE: Makefile:3: This variable value should be aligned to column 25.") + "NOTE: Makefile:1: This variable value should be aligned to column 25 instead of 17.", + "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 25 instead of 18.", + "NOTE: Makefile:3: This variable value should be aligned to column 25 instead of 17.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t\" with \"\\t\\t\".", "AUTOFIX: Makefile:2: Replacing \" \" with \"\\t\".", @@ -1609,9 +1614,9 @@ func (s *Suite) Test_VaralignBlock__var16_space_value_var16_space_value_var16_sp "16 17", "16 17") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25.", - "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 25.", - "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 25.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25 instead of 18.", + "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 25 instead of 18.", + "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 25 instead of 18.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".", "AUTOFIX: Makefile:2: Replacing \" \" with \"\\t\".", @@ -1657,7 +1662,7 @@ func (s *Suite) Test_VaralignBlock__var2_space_value_var2_tab_value(c *check.C) "02 03", "02 08") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 9.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 9 instead of 4.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".") vt.Fixed( @@ -1698,8 +1703,8 @@ func (s *Suite) Test_VaralignBlock__var8_space_value_var2_tab_value(c *check.C) "08 09", "02 08") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 17.", - "NOTE: Makefile:2: This variable value should be aligned to column 17.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 17 instead of 10.", + "NOTE: Makefile:2: This variable value should be aligned to column 17 instead of 9.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".", "AUTOFIX: Makefile:2: Replacing \"\\t\" with \"\\t\\t\".") @@ -1722,7 +1727,7 @@ func (s *Suite) Test_VaralignBlock__var15_space_value_var2_tab_value(c *check.C) "02 08") vt.Diagnostics( "NOTE: Makefile:1: Variable values should be aligned with tabs, not spaces.", - "NOTE: Makefile:2: This variable value should be aligned to column 17.") + "NOTE: Makefile:2: This variable value should be aligned to column 17 instead of 9.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".", "AUTOFIX: Makefile:2: Replacing \"\\t\" with \"\\t\\t\".") @@ -1762,7 +1767,7 @@ func (s *Suite) Test_VaralignBlock__var2_space_value_var9_tab_value(c *check.C) "02 03", "09 16") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 17.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 17 instead of 4.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\\t\".") vt.Fixed( @@ -1781,8 +1786,8 @@ func (s *Suite) Test_VaralignBlock__var22_space_value_var9_tab_value(c *check.C) "22 23", "09 16") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25.", - "NOTE: Makefile:2: This variable value should be aligned to column 25.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25 instead of 24.", + "NOTE: Makefile:2: This variable value should be aligned to column 25 instead of 17.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".", "AUTOFIX: Makefile:2: Replacing \"\\t\" with \"\\t\\t\".") @@ -1802,7 +1807,7 @@ func (s *Suite) Test_VaralignBlock__var23_space_value_var9_tab_value(c *check.C) "09 16") vt.Diagnostics( "NOTE: Makefile:1: Variable values should be aligned with tabs, not spaces.", - "NOTE: Makefile:2: This variable value should be aligned to column 25.") + "NOTE: Makefile:2: This variable value should be aligned to column 25 instead of 17.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".", "AUTOFIX: Makefile:2: Replacing \"\\t\" with \"\\t\\t\".") @@ -1839,8 +1844,8 @@ func (s *Suite) Test_VaralignBlock__var_tabs24_value_var_tabs40_value(c *check.C "08 24", "08 40") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 17.", - "NOTE: Makefile:2: This variable value should be aligned to column 17.") + "NOTE: Makefile:1: This variable value should be aligned to column 17 instead of 25.", + "NOTE: Makefile:2: This variable value should be aligned to column 17 instead of 41.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t\\t\" with \"\\t\".", "AUTOFIX: Makefile:2: Replacing \"\\t\\t\\t\\t\" with \"\\t\".") @@ -1877,7 +1882,7 @@ func (s *Suite) Test_VaralignBlock__var6_space_value_var10_tab_value(c *check.C) "06 07", "10 16") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 17.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 17 instead of 8.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\\t\".") vt.Fixed( @@ -1994,7 +1999,7 @@ func (s *Suite) Test_VaralignBlock__var_tabs24_value_var_tab24_value_var_space_c " 08 23", " 08") vt.Diagnostics( - "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 25.") + "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 25 instead of 16.") vt.Autofixes( "AUTOFIX: Makefile:3: Replacing \" \" with \"\\t\\t\".") vt.Fixed( @@ -2016,7 +2021,7 @@ func (s *Suite) Test_VaralignBlock__lead_var_tab8_value_lead_var_tab16_value(c * "05 08", "11 16") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 17.") + "NOTE: Makefile:1: This variable value should be aligned to column 17 instead of 9.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t\" with \"\\t\\t\".") vt.Fixed( @@ -2106,7 +2111,7 @@ func (s *Suite) Test_VaralignBlock__continuation_line_last_empty(c *check.C) { " 00", "09 16") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 17.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 17 instead of 12.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".") vt.Fixed( @@ -2143,10 +2148,10 @@ func (s *Suite) Test_VaralignBlock__realign_commented_single_lines(c *check.C) { " 01", "06 08") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 17.", - "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 17.", + "NOTE: Makefile:1: This variable value should be aligned to column 17 instead of 9.", + "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 17 instead of 16.", "NOTE: Makefile:6: This continuation line should be indented with \"\\t\\t\".", - "NOTE: Makefile:7: This variable value should be aligned to column 17.") + "NOTE: Makefile:7: This variable value should be aligned to column 17 instead of 9.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \"\\t\" with \"\\t\\t\".", "AUTOFIX: Makefile:3: Replacing \" \" with \"\\t\".", @@ -2327,7 +2332,7 @@ func (s *Suite) Test_VaralignBlock__shift_already_long_line_to_the__right(c *che "11 16 71", " 16") vt.Diagnostics( - "NOTE: Makefile:2: This variable value should be aligned to column 25.", + "NOTE: Makefile:2: This variable value should be aligned to column 25 instead of 17.", "NOTE: Makefile:3: This continuation line should be indented with \"\\t\\t\\t\".") vt.Autofixes( "AUTOFIX: Makefile:2: Replacing \"\\t\" with \"\\t\\t\".", @@ -2440,7 +2445,7 @@ func (s *Suite) Test_VaralignBlock__command_with_arguments(c *check.C) { " 08 18", " 08") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25.", + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 25 instead of 22.", "NOTE: Makefile:2: This continuation line should be indented with \"\\t\\t\\t\".", "NOTE: Makefile:3: This continuation line should be indented with \"\\t\\t\\t\".", "NOTE: Makefile:4: This continuation line should be indented with \"\\t\\t\\t\".") @@ -2901,7 +2906,7 @@ func (s *Suite) Test_VaralignBlock_Process__var_spaces7_value(c *check.C) { vt.Input( "VAR= value") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 9.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 9 instead of 8.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".") vt.Fixed( @@ -2927,7 +2932,7 @@ func (s *Suite) Test_VaralignBlock_Process__var_spaces9_value(c *check.C) { vt.Input( "VAR= value") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 9.") + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 9 instead of 10.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".") vt.Fixed( @@ -2998,8 +3003,8 @@ func (s *Suite) Test_VaralignBlock_Process__reduce_indentation(c *check.C) { t.CheckOutputLines( "NOTE: file.mk:1: Variable values should be aligned with tabs, not spaces.", - "NOTE: file.mk:2: This variable value should be aligned with tabs, not spaces, to column 9.", - "NOTE: file.mk:3: This variable value should be aligned to column 9.") + "NOTE: file.mk:2: This variable value should be aligned with tabs, not spaces, to column 9 instead of 17.", + "NOTE: file.mk:3: This variable value should be aligned to column 9 instead of 33.") } // For every variable assignment, there is at least one space or tab between the variable @@ -3013,10 +3018,14 @@ func (s *Suite) Test_VaralignBlock_Process__longest_line_no_space(c *check.C) { "SUBST_FILES.123456= *.pl", "SUBST_FILTER_CMD.123456=cat") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 33.", - "NOTE: Makefile:2: This variable value should be aligned with tabs, not spaces, to column 33.", - "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 33.", - "NOTE: Makefile:4: This variable value should be aligned to column 33.") + "NOTE: Makefile:1: This variable value should be aligned "+ + "with tabs, not spaces, to column 33 instead of 17.", + "NOTE: Makefile:2: This variable value should be aligned "+ + "with tabs, not spaces, to column 33 instead of 21.", + "NOTE: Makefile:3: This variable value should be aligned "+ + "with tabs, not spaces, to column 33 instead of 21.", + "NOTE: Makefile:4: This variable value should be aligned "+ + "to column 33 instead of 25.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\\t\\t\".", "AUTOFIX: Makefile:2: Replacing \" \" with \"\\t\\t\".", @@ -3046,10 +3055,10 @@ func (s *Suite) Test_VaralignBlock_Process__only_spaces(c *check.C) { varalign.Finish() t.CheckOutputLines( - "NOTE: file.mk:1: This variable value should be aligned with tabs, not spaces, to column 33.", - "NOTE: file.mk:2: This variable value should be aligned with tabs, not spaces, to column 33.", - "NOTE: file.mk:3: This variable value should be aligned with tabs, not spaces, to column 33.", - "NOTE: file.mk:4: This variable value should be aligned with tabs, not spaces, to column 33.") + "NOTE: file.mk:1: This variable value should be aligned with tabs, not spaces, to column 33 instead of 17.", + "NOTE: file.mk:2: This variable value should be aligned with tabs, not spaces, to column 33 instead of 23.", + "NOTE: file.mk:3: This variable value should be aligned with tabs, not spaces, to column 33 instead of 23.", + "NOTE: file.mk:4: This variable value should be aligned with tabs, not spaces, to column 33 instead of 28.") } func (s *Suite) Test_VaralignBlock_processVarassign__comment_with_continuation(c *check.C) { @@ -3058,7 +3067,7 @@ func (s *Suite) Test_VaralignBlock_processVarassign__comment_with_continuation(c "VAR.param= # comment \\", "#\tthe comment continues") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 17.", + "NOTE: Makefile:1: This variable value should be aligned with tabs, not spaces, to column 17 instead of 12.", "NOTE: Makefile:2: This continuation line should be indented with \"\\t\\t\".") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".", @@ -3534,12 +3543,12 @@ func (s *Suite) Test_varalignLine_alignValueSingle(c *check.C) { info.alignValueSingle(column) t.CheckEqualsf( - mkline.raw[0].text(), after, + mkline.raw[0].Text(), after, "Line.raw.text, autofix=%v", autofix) // As of 2019-12-11, the info fields are not updated // accordingly, but they should. - // TODO: update info accordingly + // FIXME: update info accordingly t.CheckEqualsf(info.String(), before, "info.String, autofix=%v", autofix) } @@ -3562,7 +3571,7 @@ func (s *Suite) Test_varalignLine_alignValueSingle(c *check.C) { "VAR=\t\tvalue", "NOTE: filename.mk:123: This variable value "+ - "should be aligned to column 17.", + "should be aligned to column 17 instead of 9.", "AUTOFIX: filename.mk:123: Replacing \"\\t\" with \"\\t\\t\".") // Aligned to the wrong column, using a mixture of tabs and spaces. @@ -3572,7 +3581,7 @@ func (s *Suite) Test_varalignLine_alignValueSingle(c *check.C) { "VAR=\t\tvalue", "NOTE: filename.mk:123: This variable value "+ - "should be aligned with tabs, not spaces, to column 17.", + "should be aligned with tabs, not spaces, to column 17 instead of 13.", "AUTOFIX: filename.mk:123: Replacing \"\\t \" with \"\\t\\t\".") // Correct column, but using spaces for indentation. @@ -3593,7 +3602,7 @@ func (s *Suite) Test_varalignLine_alignValueSingle(c *check.C) { "VAR=\tvalue", "NOTE: filename.mk:123: "+ - "This variable value should be aligned to column 9.", + "This variable value should be aligned to column 9 instead of 41.", "AUTOFIX: filename.mk:123: Replacing \"\\t\\t\\t\\t\\t\" with \"\\t\".") // An outlier should use a single space, to be as far to the @@ -3648,7 +3657,7 @@ func (s *Suite) Test_varalignLine_alignValueInitial__empty_spaces(c *check.C) { "04 05") vt.Diagnostics( "NOTE: Makefile:1: Variable values should be aligned with tabs, not spaces.", - "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 9.") + "NOTE: Makefile:3: This variable value should be aligned with tabs, not spaces, to column 9 instead of 6.") vt.Autofixes( "AUTOFIX: Makefile:1: Replacing \" \" with \"\\t\".", "AUTOFIX: Makefile:3: Replacing \" \" with \"\\t\".") @@ -3692,14 +3701,14 @@ func (s *Suite) Test_varalignLine_alignValueInitial(c *check.C) { assert(len(mklines.mklines) == 1) mkline := mklines.mklines[0] - text := mkline.raw[0].text() + text := mkline.raw[0].Text() parts := NewVaralignSplitter().split(text, true) info := &varalignLine{mkline, 0, false, parts} info.alignValueInitial(column) t.CheckEqualsf( - mkline.raw[0].text(), after, + mkline.raw[0].Text(), after, "Line.raw.text, autofix=%v", autofix) t.CheckEqualsf(info.String(), after, @@ -3722,7 +3731,7 @@ func (s *Suite) Test_varalignLine_alignValueInitial(c *check.C) { 16, "VAR=\t\tvalue \\", - "NOTE: filename.mk:1: This variable value should be aligned to column 17.", + "NOTE: filename.mk:1: This variable value should be aligned to column 17 instead of 9.", "AUTOFIX: filename.mk:1: Replacing \"\\t\" with \"\\t\\t\".") // The column is already correct, @@ -3742,7 +3751,7 @@ func (s *Suite) Test_varalignLine_alignValueInitial(c *check.C) { 16, "VAR=\t\tvalue \\", - "NOTE: filename.mk:1: This variable value should be aligned with tabs, not spaces, to column 17.", + "NOTE: filename.mk:1: This variable value should be aligned with tabs, not spaces, to column 17 instead of 13.", "AUTOFIX: filename.mk:1: Replacing \" \\t \" with \"\\t\\t\".") } @@ -3824,7 +3833,7 @@ func (s *Suite) Test_varalignLine_alignValueMultiFollow(c *check.C) { info.alignValueMultiFollow(width) - t.CheckEquals(raw.text(), after) + t.CheckEquals(raw.Text(), after) } t.ExpectDiagnosticsAutofix( @@ -3985,8 +3994,8 @@ func (s *Suite) Test_varalignLine_alignValueMultiFollow__unindent_long_lines(c * " 64 67", " 64") vt.Diagnostics( - "NOTE: Makefile:1: This variable value should be aligned to column 17.", - "NOTE: Makefile:2: This variable value should be aligned to column 17.", + "NOTE: Makefile:1: This variable value should be aligned to column 17 instead of 9.", + "NOTE: Makefile:2: This variable value should be aligned to column 17 instead of 41.", "NOTE: Makefile:3: This continuation line should be indented with \"\\t\\t\\t\\t\\t\\t\".", "NOTE: Makefile:4: This continuation line should be indented with \"\\t\\t\\t\\t\\t\\t\".", "NOTE: Makefile:5: This continuation line should be indented with \"\\t\\t\\t\\t\\t\\t\".", @@ -4078,14 +4087,14 @@ func (s *Suite) Test_varalignLine_alignValue(c *check.C) { assert(len(mklines.mklines) == 1) mkline := mklines.mklines[0] - text := mkline.raw[0].text() + text := mkline.raw[0].Text() parts := NewVaralignSplitter().split(text, true) info := &varalignLine{mkline, 0, false, parts} info.alignValue(column) t.CheckEqualsf( - mkline.raw[0].text(), after, + mkline.raw[0].Text(), after, "Line.raw.text, autofix=%v", autofix) t.CheckEqualsf(info.String(), after, @@ -4105,7 +4114,7 @@ func (s *Suite) Test_varalignLine_alignValue(c *check.C) { assert(len(mklines.mklines) == 1) mkline := mklines.mklines[0] - text := mkline.raw[0].text() + text := mkline.raw[0].Text() parts := NewVaralignSplitter().split(text, true) info := &varalignLine{mkline, 0, false, parts} @@ -4113,7 +4122,7 @@ func (s *Suite) Test_varalignLine_alignValue(c *check.C) { func() { info.alignValue(column) }) t.CheckEqualsf( - mkline.raw[0].text(), before, + mkline.raw[0].Text(), before, "Line.raw.text, autofix=%v", autofix) t.CheckEqualsf(info.String(), before, @@ -4133,7 +4142,7 @@ func (s *Suite) Test_varalignLine_alignValue(c *check.C) { 16, "VAR=\t\tvalue \\", - "NOTE: filename.mk:1: This variable value should be aligned to column 17.", + "NOTE: filename.mk:1: This variable value should be aligned to column 17 instead of 9.", "AUTOFIX: filename.mk:1: Replacing \"\\t\" with \"\\t\\t\".") // The column is already correct, @@ -4153,7 +4162,7 @@ func (s *Suite) Test_varalignLine_alignValue(c *check.C) { 16, "VAR=\t\tvalue \\", - "NOTE: filename.mk:1: This variable value should be aligned with tabs, not spaces, to column 17.", + "NOTE: filename.mk:1: This variable value should be aligned with tabs, not spaces, to column 17 instead of 13.", "AUTOFIX: filename.mk:1: Replacing \" \\t \" with \"\\t\\t\".") } @@ -4192,14 +4201,14 @@ func (s *Suite) Test_varalignLine_alignContinuation(c *check.C) { assert(len(mklines.mklines) == 1) mkline := mklines.mklines[0] - text := mkline.raw[rawIndex].text() + text := mkline.raw[rawIndex].Text() parts := NewVaralignSplitter().split(text, rawIndex == 0) info := &varalignLine{mkline, rawIndex, false, parts} info.alignContinuation(valueColumn, rightMarginColumn) t.CheckEqualsf( - mkline.raw[rawIndex].text(), after, + mkline.raw[rawIndex].Text(), after, "Line.raw.text, autofix=%v", autofix) t.CheckEqualsf(info.String(), after, diff --git a/pkgtools/pkglint/files/vardefs.go b/pkgtools/pkglint/files/vardefs.go index 6a20877dd0f..08b00ba0c52 100644 --- a/pkgtools/pkglint/files/vardefs.go +++ b/pkgtools/pkglint/files/vardefs.go @@ -18,7 +18,7 @@ import ( // in pkgsrc, and some of them have very specific tasks, like buildlink3.mk, // builtin.mk and options.mk. // -// TODO: There are separate permission rules for files from the pkgsrc +// TODO: There should be separate permission rules for files from the pkgsrc // infrastructure since the infrastructure basically provides the API, and // the packages use the API. // @@ -77,9 +77,9 @@ func (reg *VarTypeRegistry) Define(varname string, basicType *BasicType, options // to prevent typos. To use arbitrary filenames, prefix them with // "special:". // -// TODO: To be implemented: when prefixed with "infra:", the entry only -// applies to files within the pkgsrc infrastructure. Without this prefix, -// the pattern only applies to files outside the pkgsrc infrastructure. +// TODO: When prefixed with "infra:", the entry should only +// apply to files within the pkgsrc infrastructure. Without this prefix, +// the pattern should only apply to files outside the pkgsrc infrastructure. func (reg *VarTypeRegistry) DefineParse(varname string, basicType *BasicType, options vartypeOptions, aclEntries ...string) { parsedEntries := reg.parseACLEntries(varname, aclEntries...) reg.Define(varname, basicType, options, parsedEntries...) @@ -1500,7 +1500,7 @@ func (reg *VarTypeRegistry) Init(src *Pkgsrc) { // In rare cases (audio/speex), ${MACHINE_ARCH} is used for selecting a group, // but not for defining it. // - // TODO: force the pkgsrc packages to only define options in the + // TODO: Consider forcing the pkgsrc packages to only define options in the // options.mk file. Most packages already do this, but some still // define them in the Makefile or Makefile.common. reg.sysloadlist("PKG_OPTIONS", BtOption, DefinedIfInScope|NonemptyIfDefined) diff --git a/pkgtools/pkglint/files/vargroups.go b/pkgtools/pkglint/files/vargroups.go index d385df6764f..b28479d395c 100644 --- a/pkgtools/pkglint/files/vargroups.go +++ b/pkgtools/pkglint/files/vargroups.go @@ -15,7 +15,7 @@ import ( // See mk/misc/show.mk, keyword _VARGROUPS. type VargroupsChecker struct { mklines *MkLines - skip bool + group string registered map[string]*MkLine userVars map[string]*MkLine @@ -27,21 +27,25 @@ type VargroupsChecker struct { sortedVars map[string]*MkLine listedVars map[string]*MkLine + userPrivate string + pkgPrivate string + sysPrivate string + defPrivate string + usePrivate string + undefinedVars map[string]*MkLine unusedVars map[string]*MkLine } func NewVargroupsChecker(mklines *MkLines) *VargroupsChecker { ck := VargroupsChecker{mklines: mklines} - ck.init() + ck.group = trimHspace(mklines.allVars.LastValue("_VARGROUPS")) + ck.collect() return &ck } -func (ck *VargroupsChecker) init() { - mklines := ck.mklines - scope := mklines.allVars - if !scope.IsDefined("_VARGROUPS") { - ck.skip = true +func (ck *VargroupsChecker) collect() { + if ck.group == "" { return } @@ -54,94 +58,87 @@ func (ck *VargroupsChecker) init() { ck.ignVars = make(map[string]*MkLine) ck.sortedVars = make(map[string]*MkLine) ck.listedVars = make(map[string]*MkLine) - userPrivate := "" - pkgPrivate := "" - sysPrivate := "" - defPrivate := "" - usePrivate := "" - - group := "" - - checkGroupName := func(mkline *MkLine) { - varname := mkline.Varname() - if varnameParam(varname) != group { - mkline.Warnf("Expected %s.%s, but found %q.", - varnameBase(varname), group, varnameParam(varname)) + + ck.mklines.ForEach(func(mkline *MkLine) { + if mkline.IsVarassign() { + ck.collectVarassign(mkline) } + }) + + ck.undefinedVars = copyStringMkLine(ck.defVars) + ck.unusedVars = copyStringMkLine(ck.useVars) +} + +func (ck *VargroupsChecker) collectVarassign(mkline *MkLine) { + switch varnameCanon(mkline.Varname()) { + case "_USER_VARS.*": + ck.appendTo(ck.userVars, mkline, true, &ck.userPrivate) + case "_PKG_VARS.*": + ck.appendTo(ck.pkgVars, mkline, true, &ck.pkgPrivate) + case "_SYS_VARS.*": + ck.appendTo(ck.sysVars, mkline, true, &ck.sysPrivate) + case "_DEF_VARS.*": + ck.appendTo(ck.defVars, mkline, false, &ck.defPrivate) + case "_USE_VARS.*": + ck.appendTo(ck.useVars, mkline, false, &ck.usePrivate) + case "_IGN_VARS.*": + ck.appendTo(ck.ignVars, mkline, false, nil) + case "_SORTED_VARS.*": + ck.appendToStyle(ck.sortedVars, mkline) + case "_LISTED_VARS.*": + ck.appendToStyle(ck.listedVars, mkline) } +} - appendTo := func(vars map[string]*MkLine, mkline *MkLine, publicGroup bool, firstPrivate *string) { - checkGroupName(mkline) - - for _, varname := range mkline.ValueFields(mkline.Value()) { - if containsVarRef(varname) { - continue - } - - private := hasPrefix(varname, "_") - if publicGroup && private { - mkline.Warnf("%s should list only variables that start with a letter, not %q.", - mkline.Varname(), varname) - } - - if firstPrivate != nil { - if *firstPrivate != "" && !private { - mkline.Warnf("The public variable %s should be listed before the private variable %s.", - varname, *firstPrivate) - } - if private && *firstPrivate == "" { - *firstPrivate = varname - } - } - - if ck.registered[varname] != nil { - mkline.Warnf("Duplicate variable name %s, already appeared in %s.", - varname, mkline.RelMkLine(ck.registered[varname])) - } else { - ck.registered[varname] = mkline - } - - vars[varname] = mkline - } +func (ck *VargroupsChecker) appendToStyle(vars map[string]*MkLine, mkline *MkLine) { + ck.checkGroupName(mkline) + + for _, varname := range mkline.ValueFieldsLiteral() { + vars[varname] = mkline } +} - appendToStyle := func(vars map[string]*MkLine, mkline *MkLine) { - checkGroupName(mkline) +func (ck *VargroupsChecker) appendTo(vars map[string]*MkLine, mkline *MkLine, publicGroup bool, firstPrivate *string) { + ck.checkGroupName(mkline) - for _, varname := range mkline.ValueFields(mkline.Value()) { - if !containsVarRef(varname) { - vars[varname] = mkline - } - } + for _, varname := range mkline.ValueFieldsLiteral() { + ck.appendToVar(varname, mkline, publicGroup, vars, firstPrivate) } +} - mklines.ForEach(func(mkline *MkLine) { - if mkline.IsVarassign() { - switch varnameCanon(mkline.Varname()) { - case "_VARGROUPS": - group = mkline.Value() - case "_USER_VARS.*": - appendTo(ck.userVars, mkline, true, &userPrivate) - case "_PKG_VARS.*": - appendTo(ck.pkgVars, mkline, true, &pkgPrivate) - case "_SYS_VARS.*": - appendTo(ck.sysVars, mkline, true, &sysPrivate) - case "_DEF_VARS.*": - appendTo(ck.defVars, mkline, false, &defPrivate) - case "_USE_VARS.*": - appendTo(ck.useVars, mkline, false, &usePrivate) - case "_IGN_VARS.*": - appendTo(ck.ignVars, mkline, false, nil) - case "_SORTED_VARS.*": - appendToStyle(ck.sortedVars, mkline) - case "_LISTED_VARS.*": - appendToStyle(ck.listedVars, mkline) - } +func (ck *VargroupsChecker) appendToVar(varname string, mkline *MkLine, publicGroup bool, vars map[string]*MkLine, firstPrivate *string) { + private := hasPrefix(varname, "_") + if publicGroup && private { + mkline.Warnf("%s should list only variables that start with a letter, not %q.", + mkline.Varname(), varname) + } + + if firstPrivate != nil { + if *firstPrivate != "" && !private { + mkline.Warnf("The public variable %s should be listed before the private variable %s.", + varname, *firstPrivate) } - }) + if private && *firstPrivate == "" { + *firstPrivate = varname + } + } - ck.undefinedVars = copyStringMkLine(ck.defVars) - ck.unusedVars = copyStringMkLine(ck.useVars) + if ck.registered[varname] != nil { + mkline.Warnf("Duplicate variable name %s, already appeared in %s.", + varname, mkline.RelMkLine(ck.registered[varname])) + } else { + ck.registered[varname] = mkline + } + + vars[varname] = mkline +} + +func (ck *VargroupsChecker) checkGroupName(mkline *MkLine) { + varname := mkline.Varname() + if varnameParam(varname) != ck.group { + mkline.Warnf("Expected %s.%s, but found %q.", + varnameBase(varname), ck.group, varnameParam(varname)) + } } // CheckVargroups checks that each variable that is used or defined @@ -151,7 +148,7 @@ func (ck *VargroupsChecker) init() { // This check is intended mainly for infrastructure files and similar // support files, such as lang/*/module.mk. func (ck *VargroupsChecker) Check(mkline *MkLine) { - if ck.skip { + if ck.group == "" { return } @@ -194,7 +191,7 @@ func (ck *VargroupsChecker) checkUseVar(mkline *MkLine, varUse *MkVarUse) { func (ck *VargroupsChecker) ignore(varname string) bool { switch { - case containsVarRef(varname), + case containsVarUse(varname), hasSuffix(varname, "_MK"), ck.registered[varname] != nil, G.Pkgsrc.Tools.ExistsVar(varname), @@ -244,8 +241,8 @@ func (ck *VargroupsChecker) isVargroups(varname string) bool { return false } -func (ck *VargroupsChecker) Finish(mkline *MkLine) { - if ck.skip { +func (ck *VargroupsChecker) Finish() { + if ck.group == "" { return } diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go index fdb232a7ae9..648fa89fb86 100644 --- a/pkgtools/pkglint/files/vartypecheck.go +++ b/pkgtools/pkglint/files/vartypecheck.go @@ -46,7 +46,6 @@ func (cv *VartypeCheck) Explain(explanation ...string) { cv.MkLine.E // // fix.Replace("from", "to") // fix.ReplaceAfter("prefix", "from", "to") -// fix.ReplaceRegex(`[\t ]+`, "space", -1) // fix.InsertBefore("new line") // fix.InsertAfter("new line") // fix.Delete() @@ -192,8 +191,6 @@ func (cv *VartypeCheck) Category() { } // CFlag is a single option to the C/C++ compiler. -// -// XXX: How can flags like "-D NAME" be handled? func (cv *VartypeCheck) CFlag() { if cv.Op == opUseMatch { return @@ -230,9 +227,10 @@ func (cv *VartypeCheck) Comment() { cv.Warnf("COMMENT should not begin with %q.", first) } - if G.Pkg != nil && G.Pkg.EffectivePkgbase != "" { - pkgbase := G.Pkg.EffectivePkgbase - if hasPrefix(strings.ToLower(value), strings.ToLower(pkgbase+" ")) { + pkg := cv.MkLines.pkg + if pkg != nil && pkg.EffectivePkgbase != "" { + prefix := strings.ToLower(pkg.EffectivePkgbase + " ") + if hasPrefix(strings.ToLower(value), prefix) { cv.Warnf("COMMENT should not start with the package name.") cv.Explain( "The COMMENT is usually displayed together with the package name.", @@ -395,7 +393,7 @@ func (cv *VartypeCheck) DependencyWithPath() { cv.MkLine.ExplainRelativeDirs() } - if !containsVarRef(relpath.String()) { + if !containsVarUse(relpath.String()) { MkLineChecker{cv.MkLines, cv.MkLine}.CheckRelativePkgdir(relpath) } @@ -529,7 +527,7 @@ func (cv *VartypeCheck) FetchURL() { } if G.Pkgsrc.MasterSiteVarToURL[name] == "" { - if G.Pkg == nil || !G.Pkg.vars.IsDefined(name) { + if cv.MkLines.pkg == nil || !cv.MkLines.pkg.vars.IsDefined(name) { cv.Errorf("The site %s does not exist.", name) } } @@ -590,8 +588,6 @@ func (cv *VartypeCheck) Filename() { func (cv *VartypeCheck) FilePattern() { - // TODO: Decide whether to call this a "mask" or a "pattern", and use only that word everywhere. - invalid := replaceAll(cv.ValueNoVar, `[%*+,\-.0-9?@A-Z\[\]_a-z~]`, "") if invalid == "" { return @@ -641,10 +637,10 @@ func (cv *VartypeCheck) Homepage() { } baseURL := G.Pkgsrc.MasterSiteVarToURL[sitename] - if sitename == "MASTER_SITES" && G.Pkg != nil { - mkline := G.Pkg.vars.FirstDefinition("MASTER_SITES") + if sitename == "MASTER_SITES" && cv.MkLines.pkg != nil { + mkline := cv.MkLines.pkg.vars.FirstDefinition("MASTER_SITES") if mkline != nil { - if !containsVarRef(mkline.Value()) { + if !containsVarUse(mkline.Value()) { masterSites := cv.MkLine.ValueFields(mkline.Value()) if len(masterSites) > 0 { baseURL = masterSites[0] @@ -1305,7 +1301,7 @@ func (cv *VartypeCheck) URL() { if value == "" && hasPrefix(cv.MkComment, "#") { // Ok - } else if containsVarRef(value) { + } else if containsVarUse(value) { // No further checks } else if m, _, host, _, _ := match4(value, `^(https?|ftp|gopher)://([-0-9A-Za-z.]+)(?::(\d+))?/([-%&+,./0-9:;=?@A-Z_a-z~]|#)*$`); m { diff --git a/pkgtools/pkglint/files/vartypecheck_test.go b/pkgtools/pkglint/files/vartypecheck_test.go index 057229614b0..08966963648 100644 --- a/pkgtools/pkglint/files/vartypecheck_test.go +++ b/pkgtools/pkglint/files/vartypecheck_test.go @@ -306,10 +306,11 @@ func (s *Suite) Test_VartypeCheck_CFlag(c *check.C) { func (s *Suite) Test_VartypeCheck_Comment(c *check.C) { t := s.Init(c) - vt := NewVartypeCheckTester(t, BtComment) - G.Pkg = NewPackage(t.File("category/converter")) - G.Pkg.EffectivePkgbase = "converter" + pkg := NewPackage(t.File("category/converter")) + pkg.EffectivePkgbase = "converter" + vt := NewVartypeCheckTester(t, BtComment) + vt.Package(pkg) vt.Varname("COMMENT") vt.Values( @@ -473,7 +474,6 @@ func (s *Suite) Test_VartypeCheck_Dependency(c *check.C) { func (s *Suite) Test_VartypeCheck_DependencyWithPath(c *check.C) { t := s.Init(c) - vt := NewVartypeCheckTester(t, BtDependencyWithPath) t.CreateFileLines("category/package/Makefile") t.CreateFileLines("category/package/files/dummy") @@ -482,11 +482,13 @@ func (s *Suite) Test_VartypeCheck_DependencyWithPath(c *check.C) { t.CreateFileLines("devel/gmake/Makefile") t.CreateFileLines("devel/py-module/Makefile") t.CreateFileLines("x11/alacarte/Makefile") - G.Pkg = NewPackage(t.File("category/package")) + pkg := NewPackage(t.File("category/package")) + vt := NewVartypeCheckTester(t, BtDependencyWithPath) + vt.Package(pkg) vt.Varname("DEPENDS") vt.Op(opAssignAppend) - vt.File(G.Pkg.File("filename.mk")) + vt.File(pkg.File("filename.mk")) vt.Values( "Perl", "perl5>=5.22:../perl5", @@ -663,13 +665,14 @@ func (s *Suite) Test_VartypeCheck_FetchURL(c *check.C) { "MASTER_SITE_OWN=\thttps://example.org/") t.FinishSetUp() - vt := NewVartypeCheckTester(t, BtFetchURL) - t.SetUpMasterSite("MASTER_SITE_GNU", "http://ftp.gnu.org/pub/gnu/") t.SetUpMasterSite("MASTER_SITE_GITHUB", "https://github.com/") - G.Pkg = NewPackage(t.File("category/own-master-site")) - G.Pkg.load() + pkg := NewPackage(t.File("category/own-master-site")) + pkg.load() + + vt := NewVartypeCheckTester(t, BtFetchURL) + vt.Package(pkg) vt.Varname("MASTER_SITES") vt.Values( @@ -785,7 +788,7 @@ func (s *Suite) Test_VartypeCheck_FetchURL(c *check.C) { // 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. + // see containsVarUse in VartypeCheck.URL. vt.Values( "https://example.org/$@") @@ -918,7 +921,8 @@ func (s *Suite) Test_VartypeCheck_Homepage(c *check.C) { vt.Output( "WARN: filename.mk:3: HOMEPAGE should not be defined in terms of MASTER_SITEs.") - G.Pkg = NewPackage(t.File("category/package")) + pkg := NewPackage(t.File("category/package")) + vt.Package(pkg) vt.Values( "${MASTER_SITES}") @@ -929,9 +933,9 @@ func (s *Suite) Test_VartypeCheck_Homepage(c *check.C) { vt.Output( "WARN: filename.mk:11: HOMEPAGE should not be defined in terms of MASTER_SITEs.") - delete(G.Pkg.vars.firstDef, "MASTER_SITES") - delete(G.Pkg.vars.lastDef, "MASTER_SITES") - G.Pkg.vars.Define("MASTER_SITES", t.NewMkLine(G.Pkg.File("Makefile"), 5, + delete(pkg.vars.firstDef, "MASTER_SITES") + delete(pkg.vars.lastDef, "MASTER_SITES") + pkg.vars.Define("MASTER_SITES", t.NewMkLine(pkg.File("Makefile"), 5, "MASTER_SITES=\thttps://cdn.NetBSD.org/pub/pkgsrc/distfiles/")) vt.Values( @@ -941,9 +945,9 @@ func (s *Suite) Test_VartypeCheck_Homepage(c *check.C) { "WARN: filename.mk:21: HOMEPAGE should not be defined in terms of MASTER_SITEs. " + "Use https://cdn.NetBSD.org/pub/pkgsrc/distfiles/ directly.") - delete(G.Pkg.vars.firstDef, "MASTER_SITES") - delete(G.Pkg.vars.lastDef, "MASTER_SITES") - G.Pkg.vars.Define("MASTER_SITES", t.NewMkLine(G.Pkg.File("Makefile"), 5, + delete(pkg.vars.firstDef, "MASTER_SITES") + delete(pkg.vars.lastDef, "MASTER_SITES") + pkg.vars.Define("MASTER_SITES", t.NewMkLine(pkg.File("Makefile"), 5, "MASTER_SITES=\t${MASTER_SITE_GITHUB}")) vt.Values( @@ -954,9 +958,9 @@ func (s *Suite) Test_VartypeCheck_Homepage(c *check.C) { vt.Output( "WARN: filename.mk:31: HOMEPAGE should not be defined in terms of MASTER_SITEs.") - delete(G.Pkg.vars.firstDef, "MASTER_SITES") - delete(G.Pkg.vars.lastDef, "MASTER_SITES") - G.Pkg.vars.Define("MASTER_SITES", t.NewMkLine(G.Pkg.File("Makefile"), 5, + delete(pkg.vars.firstDef, "MASTER_SITES") + delete(pkg.vars.lastDef, "MASTER_SITES") + pkg.vars.Define("MASTER_SITES", t.NewMkLine(pkg.File("Makefile"), 5, "MASTER_SITES=\t# none")) vt.Values( @@ -1096,9 +1100,9 @@ func (s *Suite) Test_VartypeCheck_License(c *check.C) { t.CreateFileLines("licenses/mit", "...") t.FinishSetUp() - G.Pkg = NewPackage(t.File("category/package")) + pkg := NewPackage(t.File("category/package")) - mklines := t.SetUpFileMkLines("perl5.mk", + mklines := t.SetUpFileMkLinesPkg("perl5.mk", pkg, MkCvsID, "PERL5_LICENSE= gnu-gpl-v2 OR artistic") // Also registers the PERL5_LICENSE variable in the package. @@ -1106,6 +1110,7 @@ func (s *Suite) Test_VartypeCheck_License(c *check.C) { vt := NewVartypeCheckTester(t, BtLicense) + vt.Package(pkg) vt.Varname("LICENSE") vt.Values( "gnu-gpl-v2", @@ -2184,6 +2189,7 @@ type VartypeCheckTester struct { lineno int varname string op MkOperator + pkg *Package } // NewVartypeCheckTester starts the test with a filename of "filename", at line 1, @@ -2196,9 +2202,11 @@ func NewVartypeCheckTester(t *Tester, basicType *BasicType) *VartypeCheckTester t.SetUpVartypes() } - return &VartypeCheckTester{t, basicType, "filename.mk", 1, "", opAssign} + return &VartypeCheckTester{t, basicType, "filename.mk", 1, "", opAssign, nil} } +func (vt *VartypeCheckTester) Package(pkg *Package) { vt.pkg = pkg } + func (vt *VartypeCheckTester) Varname(varname string) { vartype := G.Pkgsrc.VariableType(nil, varname) assertNotNil(vartype) @@ -2277,7 +2285,7 @@ func (vt *VartypeCheckTester) Values(values ...string) { text := toText(value) line := vt.tester.NewLine(vt.filename, vt.lineno, text) - mklines := NewMkLines(NewLines(vt.filename, []*Line{line})) + mklines := NewMkLines(NewLines(vt.filename, []*Line{line}), vt.pkg) vt.lineno++ mklines.ForEach(func(mkline *MkLine) { test(mklines, mkline, value) }) |