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