diff options
| author | rillig <rillig@pkgsrc.org> | 2019-11-27 22:10:06 +0000 |
|---|---|---|
| committer | rillig <rillig@pkgsrc.org> | 2019-11-27 22:10:06 +0000 |
| commit | 321a02ff654ecd298b90dfdd4b817db8188a3f92 (patch) | |
| tree | c86f91eea6d1b67622ef48d133096a749f72b9ff /pkgtools | |
| parent | 24c3e78b3e648d38f03ada372e47fad152e61f7d (diff) | |
| download | pkgsrc-321a02ff654ecd298b90dfdd4b817db8188a3f92.tar.gz | |
pkgtools/pkglint: update to 19.3.11
Changes since 19.3.10:
The check for buildlink3.mk files that are included conditionally in one
place and unconditionally in another place have been refined. Now they
also work in cases that do not involve any variables, such as when the
condition is a mere exists(filename).
References to variables that use parentheses instead of the usual braces
produce a warning now, even if pkglint cannot fix them automatically.
This affects only a few instances where more than one such variable
reference appeared in a single line.
The --log-verbose command line option has been removed since it does not
have any practical use other than improving the performance during
pkglint development itself. Because of that it hadn't even been
mentioned in the manual page.
Warnings for missing license files now report the path to the license
file relative to the line where the warning occurs, like everywhere
else.
Diffstat (limited to 'pkgtools')
60 files changed, 2459 insertions, 1711 deletions
diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index c3f3bce7964..e1135530da9 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.610 2019/11/23 23:35:55 rillig Exp $ +# $NetBSD: Makefile,v 1.611 2019/11/27 22:10:06 rillig Exp $ -PKGNAME= pkglint-19.3.10 +PKGNAME= pkglint-19.3.11 CATEGORIES= pkgtools DISTNAME= tools MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/} diff --git a/pkgtools/pkglint/PLIST b/pkgtools/pkglint/PLIST index c3dc5211938..a3d4e9d2983 100644 --- a/pkgtools/pkglint/PLIST +++ b/pkgtools/pkglint/PLIST @@ -1,4 +1,4 @@ -@comment $NetBSD: PLIST,v 1.17 2019/11/23 23:35:55 rillig Exp $ +@comment $NetBSD: PLIST,v 1.18 2019/11/27 22:10:06 rillig Exp $ bin/pkglint gopkg/pkg/${GO_PLATFORM}/netbsd.org/pkglint.a gopkg/pkg/${GO_PLATFORM}/netbsd.org/pkglint/getopt.a @@ -30,8 +30,8 @@ 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/testnames.go -gopkg/src/netbsd.org/pkglint/intqa/testnames_test.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 gopkg/src/netbsd.org/pkglint/licenses/licenses.go gopkg/src/netbsd.org/pkglint/licenses/licenses.y @@ -49,6 +49,8 @@ gopkg/src/netbsd.org/pkglint/lines.go gopkg/src/netbsd.org/pkglint/lines_test.go gopkg/src/netbsd.org/pkglint/logging.go gopkg/src/netbsd.org/pkglint/logging_test.go +gopkg/src/netbsd.org/pkglint/mklexer.go +gopkg/src/netbsd.org/pkglint/mklexer_test.go gopkg/src/netbsd.org/pkglint/mkline.go gopkg/src/netbsd.org/pkglint/mkline_test.go gopkg/src/netbsd.org/pkglint/mklinechecker.go diff --git a/pkgtools/pkglint/files/autofix.go b/pkgtools/pkglint/files/autofix.go index 0ab132f3e28..542dd82647d 100644 --- a/pkgtools/pkglint/files/autofix.go +++ b/pkgtools/pkglint/files/autofix.go @@ -29,7 +29,6 @@ type autofixShortTerm struct { diagFormat string // Is logged only if it couldn't be fixed automatically diagArgs []interface{} // explanation []string // Is printed together with the diagnostic - anyway bool // Print the diagnostic even if it cannot be autofixed } type autofixAction struct { @@ -297,15 +296,6 @@ func (fix *Autofix) Describef(lineno int, format string, args ...interface{}) { fix.actions = append(fix.actions, autofixAction{sprintf(format, args...), lineno}) } -// Anyway has the effect of showing the diagnostic even when nothing can -// be fixed automatically. -// -// As usual, the diagnostic is only shown if neither --show-autofix nor -// --autofix mode is given. -func (fix *Autofix) Anyway() { - fix.anyway = !G.Logger.IsAutofix() -} - // Apply does the actual work. // Depending on the pkglint mode, it either: // @@ -330,7 +320,7 @@ func (fix *Autofix) Apply() { fix.autofixShortTerm = autofixShortTerm{} } - if !(G.Logger.Relevant(fix.diagFormat) && (len(fix.actions) > 0 || fix.anyway)) { + if !(G.Logger.Relevant(fix.diagFormat) && (len(fix.actions) > 0 || !G.Logger.IsAutofix())) { reset() return } @@ -488,12 +478,12 @@ func SaveAutofixChanges(lines *Lines) (autofixed bool) { } err := tmpName.WriteString(text.String()) if err != nil { - G.Logger.Errorf(tmpName, "Cannot write: %s", err) + G.Logger.TechErrorf(tmpName, "Cannot write: %s", err) continue } err = tmpName.Rename(filename) if err != nil { - G.Logger.Errorf(tmpName, "Cannot overwrite with autofixed content: %s", err) + G.Logger.TechErrorf(tmpName, "Cannot overwrite with autofixed content: %s", err) continue } autofixed = true diff --git a/pkgtools/pkglint/files/autofix_test.go b/pkgtools/pkglint/files/autofix_test.go index cd292c36350..4e76e93d52e 100644 --- a/pkgtools/pkglint/files/autofix_test.go +++ b/pkgtools/pkglint/files/autofix_test.go @@ -140,7 +140,7 @@ func (s *Suite) Test_Autofix__lonely_source(c *check.C) { t := s.Init(c) t.SetUpCommandLine("-Wall", "--source") - G.Logger.Opts.LogVerbose = false // For realistic conditions; otherwise all diagnostics are logged. + G.Logger.verbose = false // For realistic conditions; otherwise all diagnostics are logged. t.SetUpPackage("x11/xorg-cf-files", ".include \"../../x11/xorgproto/buildlink3.mk\"") @@ -173,7 +173,7 @@ func (s *Suite) Test_Autofix__lonely_source_2(c *check.C) { t := s.Init(c) t.SetUpCommandLine("-Wall", "--source", "--explain") - G.Logger.Opts.LogVerbose = false // For realistic conditions; otherwise all diagnostics are logged. + G.Logger.verbose = false // For realistic conditions; otherwise all diagnostics are logged. t.SetUpPackage("print/tex-bibtex8", "MAKE_FLAGS+=\tCFLAGS=${CFLAGS.${PKGSRC_COMPILER}}") @@ -245,7 +245,8 @@ func (s *Suite) Test_Autofix__show_autofix_and_source_continuation_line(c *check // shown even when they cannot be autofixed. // // This is typical when an autofix is provided for simple scenarios, -// but the code actually found is a little more complicated. +// but the code actually found is a little more complicated, like needing +// special escaping for some of the characters or containing linebreaks. func (s *Suite) Test_Autofix__show_unfixable_diagnostics_in_default_mode(c *check.C) { t := s.Init(c) @@ -258,17 +259,11 @@ func (s *Suite) Test_Autofix__show_unfixable_diagnostics_in_default_mode(c *chec lines.Lines[0].Warnf("This warning is shown since the --show-autofix option is not given.") fix := lines.Lines[1].Autofix() - fix.Warnf("This warning cannot be fixed and is therefore not shown.") + fix.Warnf("This warning cannot be fixed, nevertheless it is shown (1).") fix.Replace("XXX", "TODO") fix.Apply() - fix.Warnf("This warning cannot be fixed automatically but should be shown anyway.") - fix.Replace("XXX", "TODO") - fix.Anyway() - fix.Apply() - - // If this warning should ever appear it is probably because fix.anyway is not reset properly. - fix.Warnf("This warning cannot be fixed and is therefore not shown.") + fix.Warnf("This warning cannot be fixed, nevertheless it is shown (2).") fix.Replace("XXX", "TODO") fix.Apply() @@ -279,7 +274,8 @@ func (s *Suite) Test_Autofix__show_unfixable_diagnostics_in_default_mode(c *chec "WARN: Makefile:1: This warning is shown since the --show-autofix option is not given.", "", ">\tline2", - "WARN: Makefile:2: This warning cannot be fixed automatically but should be shown anyway.", + "WARN: Makefile:2: This warning cannot be fixed, nevertheless it is shown (1).", + "WARN: Makefile:2: This warning cannot be fixed, nevertheless it is shown (2).", "", ">\tline3", "WARN: Makefile:3: This warning is also shown.") @@ -321,7 +317,7 @@ func (s *Suite) Test_Autofix__suppress_unfixable_warnings_with_show_autofix(c *c "+\tTODO") } -// If an Autofix doesn't do anything, it must not log any diagnostics. +// If an Autofix doesn't do anything, it nevertheless logs the diagnostics. func (s *Suite) Test_Autofix__noop_replace(c *check.C) { t := s.Init(c) @@ -332,8 +328,14 @@ func (s *Suite) Test_Autofix__noop_replace(c *check.C) { fix.ReplaceRegex(`\b[A-Z]{3,}\b`, "---censored---", -1) fix.Apply() - // No output since there was no all-uppercase word in the text. - t.CheckOutputEmpty() + // This warning is wrong. This test therefore demonstrates that each + // autofix must be properly guarded to only apply when it actually + // does something. + // + // As of November 2019 there is no Rollback method since it was not + // needed yet. + t.CheckOutputLines( + "WARN: Makefile:14: All-uppercase words should not be used at all.") } func (s *Suite) Test_Autofix_Warnf__duplicate(c *check.C) { @@ -550,9 +552,7 @@ func (s *Suite) Test_Autofix_ReplaceAt(c *check.C) { t := s.Init(c) lines := func(lines ...string) []string { return lines } - diagnostics := lines - autofixes := lines - test := func(texts []string, rawIndex int, column int, from, to string, diagnostics []string, autofixes []string) { + test := func(texts []string, rawIndex int, column int, from, to string, diagnostics ...string) { mainPart := func() { mklines := t.NewMkLines("filename.mk", texts...) @@ -562,17 +562,10 @@ func (s *Suite) Test_Autofix_ReplaceAt(c *check.C) { fix := mkline.Autofix() fix.Notef("Should be appended instead of assigned.") fix.ReplaceAt(rawIndex, column, from, to) - fix.Anyway() fix.Apply() } - t.SetUpCommandLine("-Wall") - mainPart() - t.CheckOutput(diagnostics) - - t.SetUpCommandLine("-Wall", "--autofix") - mainPart() - t.CheckOutput(autofixes) + t.ExpectDiagnosticsAutofix(mainPart, diagnostics...) } test( @@ -580,10 +573,9 @@ func (s *Suite) Test_Autofix_ReplaceAt(c *check.C) { "VAR=value1 \\", "\tvalue2"), 0, 3, "=", "+=", - diagnostics( - "NOTE: filename.mk:1: Should be appended instead of assigned."), - autofixes( - "AUTOFIX: filename.mk:1: Replacing \"=\" with \"+=\".")) + + "NOTE: filename.mk:1: Should be appended instead of assigned.", + "AUTOFIX: filename.mk:1: Replacing \"=\" with \"+=\".") // If the text at the precisely given position does not match, // the note is still printed because of the fix.Anyway(), but @@ -593,21 +585,19 @@ func (s *Suite) Test_Autofix_ReplaceAt(c *check.C) { "VAR=value1 \\", "\tvalue2"), 0, 3, "?", "+=", - diagnostics( - "NOTE: filename.mk:1--2: Should be appended instead of assigned."), - autofixes( - nil...)) + + "NOTE: filename.mk:1--2: Should be appended instead of assigned.") // Getting the line number wrong is a strange programming error, and // there does not need to be any code checking for this in the main code. t.ExpectPanicMatches( - func() { test(lines("VAR=value"), 10, 3, "from", "to", nil, nil) }, + func() { test(lines("VAR=value"), 10, 3, "from", "to", nil...) }, `runtime error: index out of range.*`) // It is a programming error to replace a string with itself, since that // would produce confusing diagnostics. t.ExpectAssert( - func() { test(lines("VAR=value"), 0, 4, "value", "value", nil, nil) }) + func() { test(lines("VAR=value"), 0, 4, "value", "value", nil...) }) // Getting the column number wrong may happen when a previous replacement // has made the string shorter than before, therefore no panic in this case. @@ -616,10 +606,8 @@ func (s *Suite) Test_Autofix_ReplaceAt(c *check.C) { "VAR=value1 \\", "\tvalue2"), 0, 20, "?", "+=", - diagnostics( - "NOTE: filename.mk:1--2: Should be appended instead of assigned."), - autofixes( - nil...)) + + "NOTE: filename.mk:1--2: Should be appended instead of assigned.") } func (s *Suite) Test_Autofix_ReplaceRegex__show_autofix(c *check.C) { @@ -860,7 +848,7 @@ func (s *Suite) Test_Autofix_Custom__in_memory(c *check.C) { // 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. -func (s *Suite) Test_Autofix_Anyway__options(c *check.C) { +func (s *Suite) Test_Autofix_Apply__show_autofix_option(c *check.C) { t := s.Init(c) t.SetUpCommandLine("--show-autofix") @@ -870,7 +858,6 @@ func (s *Suite) Test_Autofix_Anyway__options(c *check.C) { fix.Warnf("This autofix doesn't match.") fix.Replace("doesn't match", "") - fix.Anyway() fix.Apply() t.CheckOutputEmpty() @@ -879,14 +866,13 @@ func (s *Suite) Test_Autofix_Anyway__options(c *check.C) { fix.Warnf("This autofix doesn't match.") fix.Replace("doesn't match", "") - fix.Anyway() fix.Apply() t.CheckOutputLines( "WARN: filename:3: This autofix doesn't match.") } -func (s *Suite) Test_Autofix_Anyway__autofix_option(c *check.C) { +func (s *Suite) Test_Autofix_Apply__autofix_option(c *check.C) { t := s.Init(c) t.SetUpCommandLine("--autofix") @@ -895,17 +881,13 @@ func (s *Suite) Test_Autofix_Anyway__autofix_option(c *check.C) { fix := line.Autofix() fix.Notef("This line is quite short.") fix.Replace("not found", "needle") - fix.Anyway() fix.Apply() - // The replacement text is not found, therefore the note should not be logged. - // Because of fix.Anyway, the note should be logged anyway. - // But because of the --autofix option, the note should not be logged. - // Therefore, in the end nothing is shown in this case. + // Because of the --autofix option, the note is not logged. t.CheckOutputEmpty() } -func (s *Suite) Test_Autofix_Anyway__autofix_and_show_autofix_options(c *check.C) { +func (s *Suite) Test_Autofix_Apply__autofix_and_show_autofix_options_no_match(c *check.C) { t := s.Init(c) t.SetUpCommandLine("--autofix", "--show-autofix") @@ -914,27 +896,11 @@ func (s *Suite) Test_Autofix_Anyway__autofix_and_show_autofix_options(c *check.C fix := line.Autofix() fix.Notef("This line is quite short.") fix.Replace("not found", "needle") - fix.Anyway() fix.Apply() - // The text to be replaced is not found. Because nothing is fixed here, - // there's no need to log anything. - // - // But fix.Anyway is set, therefore the diagnostic should be logged even - // though it cannot be fixed automatically. This comes handy in situations - // where simple cases can be fixed automatically and more complex cases - // (often involving special characters that need to be escaped properly) - // should nevertheless result in a diagnostics. - // - // The --autofix option is set, which means that the diagnostics don't - // get logged, only the actual fixes do. - // - // But then there's also the --show-autofix option, which logs the - // corresponding diagnostic for each autofix that actually changes - // something. But this autofix doesn't change anything, therefore even - // the --show-autofix doesn't have an effect. - // - // Therefore, in the end nothing is logged in this case. + // Since at least one of the --show-autofix or --autofix options is given, + // only those autofixes that actually change something are logged. + // This one doesn't find the "not found" text, therefore it is not logged. t.CheckOutputEmpty() } @@ -1045,7 +1011,6 @@ func (s *Suite) Test_Autofix_Apply__anyway_error(c *check.C) { fix := mklines.mklines[1].Autofix() fix.Errorf("From must be To.") fix.Replace("from", "to") - fix.Anyway() fix.Apply() mklines.SaveAutofixChanges() @@ -1064,7 +1029,6 @@ func (s *Suite) Test_Autofix_Apply__source_autofix_no_change(c *check.C) { fix := lines.Lines[0].Autofix() fix.Notef("Word should be replaced, but pkglint is not sure which one.") fix.Replace("word", "replacement") - fix.Anyway() fix.Apply() lines.SaveAutofixChanges() @@ -1169,7 +1133,8 @@ func (s *Suite) Test_Autofix_setDiag__no_testing_mode(c *check.C) { fix.Replace("from", "to") fix.Apply() - t.CheckOutputEmpty() + t.CheckOutputLines( + "NOTE: file.mk:123: Note.") } func (s *Suite) Test_Autofix_setDiag__bad_call_sequence(c *check.C) { diff --git a/pkgtools/pkglint/files/check_test.go b/pkgtools/pkglint/files/check_test.go index 6f000552703..4d717e9bae7 100644 --- a/pkgtools/pkglint/files/check_test.go +++ b/pkgtools/pkglint/files/check_test.go @@ -88,23 +88,10 @@ func (s *Suite) TearDownTest(c *check.C) { assertNil(err, "Cannot chdir back to previous dir: %s", err) if t.seenSetupPkgsrc > 0 && !t.seenFinish && !t.seenMain { - t.Errorf("After t.SetupPkgsrc(), either t.FinishSetUp() or t.Main() must be called.") - } - - if out := t.Output(); out != "" { - var msg strings.Builder - msg.WriteString("\n") - _, _ = fmt.Fprintf(&msg, "Unchecked output in %s; check with:\n", c.TestName()) - msg.WriteString("\n") - msg.WriteString("t.CheckOutputLines(\n") - lines := strings.Split(strings.TrimSpace(out), "\n") - for i, line := range lines { - _, _ = fmt.Fprintf(&msg, "\t%q%s\n", line, condStr(i == len(lines)-1, ")", ",")) - } - _, _ = fmt.Fprintf(&msg, "\n") - _, _ = os.Stderr.WriteString(msg.String()) + t.InternalErrorf("After t.SetupPkgsrc(), either t.FinishSetUp() or t.Main() must be called.") } + t.ReportUncheckedOutput() t.tmpdir = "" t.DisableTracing() @@ -160,7 +147,9 @@ func (t *Tester) SetUpCommandLine(args ...string) { // // It also reveals diagnostics that are logged multiple times per // line and thus can easily get annoying to the pkgsrc developers. - G.Logger.Opts.LogVerbose = true + // + // To avoid running a check multiple times, see Line.once or MkLines.once. + G.Logger.verbose = true t.seenSetUpCommandLine = true } @@ -623,7 +612,10 @@ func (t *Tester) SetUpHierarchy() ( case string: addLine(arg) case *MkLines: - text := sprintf(".include %q", relpath(filename.Dir(), arg.lines.Filename)) + fromDir := G.Pkgsrc.File(filename.Dir()) + to := G.Pkgsrc.File(arg.lines.Filename) + rel := G.Pkgsrc.Relpath(fromDir, to) + text := sprintf(".include %q", rel) addLine(text) lines = append(lines, arg.lines.Lines...) default: @@ -678,14 +670,14 @@ func (s *Suite) Test_Tester_SetUpHierarchy(c *check.C) { func (t *Tester) FinishSetUp() { if t.seenSetupPkgsrc == 0 { - t.Errorf("Unnecessary t.FinishSetUp() since t.SetUpPkgsrc() has not been called.") + t.InternalErrorf("Unnecessary t.FinishSetUp() since t.SetUpPkgsrc() has not been called.") } if !t.seenFinish { t.seenFinish = true G.Pkgsrc.LoadInfrastructure() } else { - t.Errorf("Redundant t.FinishSetup() since it was called multiple times.") + t.InternalErrorf("Redundant t.FinishSetup() since it was called multiple times.") } } @@ -698,11 +690,11 @@ func (t *Tester) FinishSetUp() { // Does not work in combination with SetUpOption. func (t *Tester) Main(args ...string) int { if t.seenFinish && !t.seenMain { - t.Errorf("Calling t.FinishSetup() before t.Main() is redundant " + + t.InternalErrorf("Calling t.FinishSetup() before t.Main() is redundant " + "since t.Main() loads the pkgsrc infrastructure.") } if t.seenSetUpCommandLine { - t.Errorf("Calling t.SetupCommandLine() before t.Main() is redundant " + + t.InternalErrorf("Calling t.SetupCommandLine() before t.Main() is redundant " + "since t.Main() accepts the command line options directly.") } @@ -741,7 +733,10 @@ func (t *Tester) CheckDeepEquals(actual interface{}, expected interface{}) bool return t.c.Check(actual, check.DeepEquals, expected) } -func (t *Tester) Errorf(format string, args ...interface{}) { +// InternalErrorf reports a consistency error in the tests. +func (t *Tester) InternalErrorf(format string, args ...interface{}) { + // It is not possible to panic here since check.v1 would then + // ignore all subsequent tests. _, _ = fmt.Fprintf(os.Stderr, "In %s: %s\n", t.testName, sprintf(format, args...)) } @@ -817,6 +812,18 @@ func (t *Tester) ExpectAssert(action func()) { t.Check(action, check.Panics, "Pkglint internal error") } +// ExpectDiagnosticsAutofix first runs the given action with -Wall, and +// then another time with -Wall --autofix. +func (t *Tester) ExpectDiagnosticsAutofix(action func(), diagnostics ...string) { + t.SetUpCommandLine("-Wall") + action() + + t.SetUpCommandLine("-Wall", "--autofix") + action() + + t.CheckOutput(diagnostics) +} + // NewRawLines creates lines from line numbers and raw text, including newlines. // // Arguments are sequences of either (lineno, orignl) or (lineno, orignl, textnl). @@ -1174,3 +1181,22 @@ func (t *Tester) Shquote(format string, rels ...Path) string { } return sprintf(format, subs...) } + +func (t *Tester) ReportUncheckedOutput() { + out := t.Output() + if out == "" { + return + } + + var msg strings.Builder + msg.WriteString("\n") + _, _ = fmt.Fprintf(&msg, "Unchecked output in %s; check with:\n", t.testName) + msg.WriteString("\n") + msg.WriteString("t.CheckOutputLines(\n") + lines := strings.Split(strings.TrimSpace(out), "\n") + for i, line := range lines { + _, _ = fmt.Fprintf(&msg, "\t%q%s\n", line, condStr(i == len(lines)-1, ")", ",")) + } + _, _ = fmt.Fprintf(&msg, "\n") + _, _ = os.Stderr.WriteString(msg.String()) +} diff --git a/pkgtools/pkglint/files/getopt/getopt_test.go b/pkgtools/pkglint/files/getopt/getopt_test.go index acae20fb367..2f521124739 100644 --- a/pkgtools/pkglint/files/getopt/getopt_test.go +++ b/pkgtools/pkglint/files/getopt/getopt_test.go @@ -409,8 +409,8 @@ func (s *Suite) Test_Options_Help__with_flag_group(c *check.C) { " (Prefix a flag with \"no-\" to disable it.)\n") } -func (s *Suite) Test__test_names(c *check.C) { - ck := intqa.NewTestNameChecker(c.Errorf) +func (s *Suite) Test__qa(c *check.C) { + ck := intqa.NewQAChecker(c.Errorf) ck.Configure("*", "*", "*", -intqa.EMissingTest) ck.Check() } diff --git a/pkgtools/pkglint/files/histogram/histogram_test.go b/pkgtools/pkglint/files/histogram/histogram_test.go index da1d7fa934e..4bab39aa821 100644 --- a/pkgtools/pkglint/files/histogram/histogram_test.go +++ b/pkgtools/pkglint/files/histogram/histogram_test.go @@ -28,8 +28,8 @@ func (s *Suite) Test_Histogram(c *check.C) { "caption 2 two\n") } -func (s *Suite) Test__test_names(c *check.C) { - ck := intqa.NewTestNameChecker(c.Errorf) +func (s *Suite) Test__qa(c *check.C) { + ck := intqa.NewQAChecker(c.Errorf) ck.Configure("*", "*", "*", -intqa.EMissingTest) ck.Check() } diff --git a/pkgtools/pkglint/files/intqa/ideas.go b/pkgtools/pkglint/files/intqa/ideas.go index 2438b88a9d5..270821ac1b1 100644 --- a/pkgtools/pkglint/files/intqa/ideas.go +++ b/pkgtools/pkglint/files/intqa/ideas.go @@ -5,8 +5,5 @@ package intqa // then the current package, then a package-qualified identifier. // As if there were a "_ = XYZ" at the beginning of the function. -// XXX: All methods should be defined in the same file as their receiver type. -// If that is not possible, there should only be a small list of exceptions. - // XXX: If there is a constructor for a type, only that constructor may be used // for constructing objects. All other forms (var x Type; x := &Type{}) should be forbidden. diff --git a/pkgtools/pkglint/files/intqa/testnames.go b/pkgtools/pkglint/files/intqa/qa.go index 3c35c228935..532f710a388 100644 --- a/pkgtools/pkglint/files/intqa/testnames.go +++ b/pkgtools/pkglint/files/intqa/qa.go @@ -38,12 +38,15 @@ const ( // The file of the test method does not correspond to the // file of the testee. EFile + + // All methods of a type must be in the same file as the type definition. + EMethodsSameFile ) -// TestNameChecker ensures that all test names follow a common naming scheme: +// QAChecker ensures that all test names follow a common naming scheme: // Test_${Type}_${Method}__${description_using_underscores} // Each of the variable parts may be omitted. -type TestNameChecker struct { +type QAChecker struct { errorf func(format string, args ...interface{}) filters []filter @@ -56,11 +59,11 @@ type TestNameChecker struct { out io.Writer } -// NewTestNameChecker creates a new checker. +// NewQAChecker creates a new checker. // By default, all errors are enabled; // call Configure to disable them selectively. -func NewTestNameChecker(errorf func(format string, args ...interface{})) *TestNameChecker { - ck := TestNameChecker{errorf: errorf, out: os.Stderr} +func NewQAChecker(errorf func(format string, args ...interface{})) *QAChecker { + ck := QAChecker{errorf: errorf, out: os.Stderr} // For test fixtures from https://gopkg.in/check/v1. ck.Configure("*_test.go", "*", "SetUpTest", -EMissingTest) @@ -81,11 +84,11 @@ func NewTestNameChecker(errorf func(format string, args ...interface{})) *TestNa // Individual errors can be enabled by giving their constant and disabled // by negating them, such as -EMissingTestee. To reset everything, use // either EAll or ENone. -func (ck *TestNameChecker) Configure(filenames, typeNames, funcNames string, errors ...Error) { +func (ck *QAChecker) Configure(filenames, typeNames, funcNames string, errors ...Error) { ck.filters = append(ck.filters, filter{filenames, typeNames, funcNames, errors}) } -func (ck *TestNameChecker) Check() { +func (ck *QAChecker) Check() { ck.load(".") ck.checkTestees() ck.checkTests() @@ -94,7 +97,7 @@ func (ck *TestNameChecker) Check() { } // load loads all type, function and method names from the current package. -func (ck *TestNameChecker) load(dir string) { +func (ck *QAChecker) load(dir string) { fileSet := token.NewFileSet() pkgs, err := parser.ParseDir(fileSet, dir, nil, 0) @@ -117,7 +120,7 @@ func (ck *TestNameChecker) load(dir string) { } // loadDecl adds a single type or function declaration to the known elements. -func (ck *TestNameChecker) loadDecl(decl ast.Decl, filename string) { +func (ck *QAChecker) loadDecl(decl ast.Decl, filename string) { switch decl := decl.(type) { case *ast.GenDecl: @@ -125,26 +128,31 @@ func (ck *TestNameChecker) loadDecl(decl ast.Decl, filename string) { switch spec := spec.(type) { case *ast.TypeSpec: typeName := spec.Name.Name - ck.addCode(code{filename, typeName, "", 0}) + ck.addCode(code{filename, typeName, "", 0}, nil) } } case *ast.FuncDecl: - typeName := "" - if decl.Recv != nil { - typeExpr := decl.Recv.List[0].Type.(ast.Expr) - if star, ok := typeExpr.(*ast.StarExpr); ok { - typeName = star.X.(*ast.Ident).Name - } else { - typeName = typeExpr.(*ast.Ident).Name - } + code := ck.parseFuncDecl(filename, decl) + ck.addCode(code, decl) + } +} + +func (*QAChecker) parseFuncDecl(filename string, decl *ast.FuncDecl) code { + typeName := "" + if decl.Recv != nil { + typeExpr := decl.Recv.List[0].Type.(ast.Expr) + if star, ok := typeExpr.(*ast.StarExpr); ok { + typeExpr = star.X } - funcName := decl.Name.Name - ck.addCode(code{filename, typeName, funcName, 0}) + typeName = typeExpr.(*ast.Ident).Name } + + funcName := decl.Name.Name + return code{filename, typeName, funcName, 0} } -func (ck *TestNameChecker) addCode(code code) { +func (ck *QAChecker) addCode(code code, decl *ast.FuncDecl) { if code.isTestScope() && code.isFunc() && code.Func == "TestMain" { // This is not a test for Main, but a wrapper function of the test. // Therefore it is completely ignored. @@ -160,19 +168,39 @@ func (ck *TestNameChecker) addCode(code code) { return } - if code.isTest() { + if ck.isTest(code, decl) { ck.addTest(code) } else { ck.addTestee(code) } } -func (ck *TestNameChecker) addTestee(code code) { +func (*QAChecker) isTest(code code, decl *ast.FuncDecl) bool { + if !code.isTestScope() || !strings.HasPrefix(code.Func, "Test") { + return false + } + if decl.Type.Params.NumFields() != 1 { + return false + } + + paramType := decl.Type.Params.List[0].Type + if star, ok := paramType.(*ast.StarExpr); ok { + paramType = star.X + } + if sel, ok := paramType.(*ast.SelectorExpr); ok { + paramType = sel.Sel + } + + paramTypeName := paramType.(*ast.Ident).Name + return paramTypeName == "C" || paramTypeName == "T" +} + +func (ck *QAChecker) addTestee(code code) { code.order = ck.nextOrder() ck.testees = append(ck.testees, &testee{code}) } -func (ck *TestNameChecker) addTest(code code) { +func (ck *QAChecker) addTest(code code) { if !strings.HasPrefix(code.Func, "Test_") && code.Func != "Test" && ck.addError( @@ -203,14 +231,14 @@ func (ck *TestNameChecker) addTest(code code) { ck.tests = append(ck.tests, &test{code, testeeName, descr, nil}) } -func (ck *TestNameChecker) nextOrder() int { +func (ck *QAChecker) nextOrder() int { id := ck.order ck.order++ return id } // relate connects the tests to their testees. -func (ck *TestNameChecker) relate() { +func (ck *QAChecker) relate() { testeesByPrefix := make(map[string]*testee) for _, testee := range ck.testees { prefix := join(testee.Type, "_", testee.Func) @@ -222,7 +250,7 @@ func (ck *TestNameChecker) relate() { } } -func (ck *TestNameChecker) checkTests() { +func (ck *QAChecker) checkTests() { for _, test := range ck.tests { ck.checkTestFile(test) ck.checkTestTestee(test) @@ -230,13 +258,13 @@ func (ck *TestNameChecker) checkTests() { } } -func (ck *TestNameChecker) checkTestFile(test *test) { +func (ck *QAChecker) checkTestFile(test *test) { testee := test.testee if testee == nil || testee.file == test.file { return } - correctTestFile := strings.TrimSuffix(testee.file, ".go") + "_test.go" + correctTestFile := testee.testFile() if correctTestFile == test.file { return } @@ -248,7 +276,7 @@ func (ck *TestNameChecker) checkTestFile(test *test) { test.fullName(), testee.fullName(), correctTestFile, test.file) } -func (ck *TestNameChecker) checkTestTestee(test *test) { +func (ck *QAChecker) checkTestTestee(test *test) { testee := test.testee if testee != nil || test.testeeName == "" { return @@ -265,7 +293,7 @@ func (ck *TestNameChecker) checkTestTestee(test *test) { // checkTestDescr ensures that the type or function name of the testee // does not accidentally end up in the description of the test. This could // happen if there is a double underscore instead of a single underscore. -func (ck *TestNameChecker) checkTestDescr(test *test) { +func (ck *QAChecker) checkTestDescr(test *test) { testee := test.testee if testee == nil || testee.isMethod() || !isCamelCase(test.descr) { return @@ -278,32 +306,77 @@ func (ck *TestNameChecker) checkTestDescr(test *test) { test.fullName(), test.descr) } -func (ck *TestNameChecker) checkTestees() { +func (ck *QAChecker) checkTestees() { + ck.checkTesteesTest() + ck.checkTesteesMethodsSameFile() +} + +// checkTesteesTest ensures that each testee has a corresponding unit test. +func (ck *QAChecker) checkTesteesTest() { tested := make(map[*testee]bool) for _, test := range ck.tests { tested[test.testee] = true } for _, testee := range ck.testees { - ck.checkTesteeTest(testee, tested) + if tested[testee] { + continue + } + + testName := "Test_" + join(testee.Type, "_", testee.Func) + ck.addError( + EMissingTest, + testee.code, + "Missing unit test %q for %q.", + testName, testee.fullName()) } } -func (ck *TestNameChecker) checkTesteeTest(testee *testee, tested map[*testee]bool) { - if tested[testee] || testee.isType() { - return +// checkTesteesMethodsSameFile ensures that all methods of a type are +// defined in the same file or in the corresponding test file. +func (ck *QAChecker) checkTesteesMethodsSameFile() { + types := map[string]*testee{} + for _, testee := range ck.testees { + if testee.isType() { + types[testee.Type] = testee + } } - testName := "Test_" + join(testee.Type, "_", testee.Func) - ck.addError( - EMissingTest, - testee.code, - "Missing unit test %q for %q.", - testName, testee.fullName()) + for _, testee := range ck.testees { + if !testee.isMethod() { + continue + } + method := testee + + typ := types[method.Type] + if typ == nil || method.file == typ.file { + continue + } + + if method.isTestScope() == typ.isTestScope() { + ck.addError( + EMethodsSameFile, + testee.code, + "Method %s must be in %s, like its type.", + testee.fullName(), typ.file) + continue + } + + correctFile := typ.testFile() + if method.file == correctFile { + continue + } + + ck.addError( + EMethodsSameFile, + testee.code, + "Method %s must be in %s, corresponding to its type.", + testee.fullName(), correctFile) + } } // isRelevant checks whether the given error is enabled. -func (ck *TestNameChecker) isRelevant(filename, typeName, funcName string, e Error) bool { +func (ck *QAChecker) isRelevant(filename, typeName, funcName string, e Error) bool { mask := ^uint64(0) for _, filter := range ck.filters { if matches(filename, filter.filenames) && @@ -315,7 +388,7 @@ func (ck *TestNameChecker) isRelevant(filename, typeName, funcName string, e Err return mask&ck.errorsMask(0, e) != 0 } -func (ck *TestNameChecker) errorsMask(mask uint64, errors ...Error) uint64 { +func (ck *QAChecker) errorsMask(mask uint64, errors ...Error) uint64 { for _, err := range errors { if err == ENone { mask = 0 @@ -332,7 +405,7 @@ func (ck *TestNameChecker) errorsMask(mask uint64, errors ...Error) uint64 { // checkOrder ensures that the tests appear in the same order as their // counterparts in the main code. -func (ck *TestNameChecker) checkOrder() { +func (ck *QAChecker) checkOrder() { maxOrderByFile := make(map[string]*test) for _, test := range ck.tests { @@ -364,7 +437,7 @@ func (ck *TestNameChecker) checkOrder() { } } -func (ck *TestNameChecker) addError(e Error, c code, format string, args ...interface{}) bool { +func (ck *QAChecker) addError(e Error, c code, format string, args ...interface{}) bool { relevant := ck.isRelevant(c.file, c.Type, c.Func, e) if relevant { ck.errors = append(ck.errors, fmt.Sprintf(format, args...)) @@ -372,7 +445,7 @@ func (ck *TestNameChecker) addError(e Error, c code, format string, args ...inte return relevant } -func (ck *TestNameChecker) print() { +func (ck *QAChecker) print() { for _, msg := range ck.errors { _, _ = fmt.Fprintln(ck.out, msg) } @@ -418,13 +491,17 @@ func (c *code) isFunc() bool { return c.Type == "" } func (c *code) isType() bool { return c.Func == "" } func (c *code) isMethod() bool { return c.Type != "" && c.Func != "" } -func (c *code) isTest() bool { - return c.isTestScope() && strings.HasPrefix(c.Func, "Test") -} func (c *code) isTestScope() bool { return strings.HasSuffix(c.file, "_test.go") } +func (c *code) testFile() string { + if strings.HasSuffix(c.file, "_test.go") { + return c.file + } + return strings.TrimSuffix(c.file, ".go") + "_test.go" +} + func isCamelCase(str string) bool { for i := 0; i+1 < len(str); i++ { if str[i] == '_' { diff --git a/pkgtools/pkglint/files/intqa/testnames_test.go b/pkgtools/pkglint/files/intqa/qa_test.go index 0a53fae70fc..ec83119cf7f 100644 --- a/pkgtools/pkglint/files/intqa/testnames_test.go +++ b/pkgtools/pkglint/files/intqa/qa_test.go @@ -4,6 +4,8 @@ import ( "bytes" "fmt" "go/ast" + "go/parser" + "go/token" "gopkg.in/check.v1" "io/ioutil" "path" @@ -12,7 +14,7 @@ import ( type Suite struct { c *check.C - ck *TestNameChecker + ck *QAChecker summary string } @@ -21,19 +23,21 @@ func Test(t *testing.T) { check.TestingT(t) } -func (s *Suite) Init(c *check.C) *TestNameChecker { +func (s *Suite) Init(c *check.C) *QAChecker { errorf := func(format string, args ...interface{}) { s.summary = fmt.Sprintf(format, args...) } s.c = c - s.ck = NewTestNameChecker(errorf) + s.ck = NewQAChecker(errorf) s.ck.out = ioutil.Discard return s.ck } func (s *Suite) TearDownTest(c *check.C) { s.c = c + // The testees and tests are not validated to be checked + // since that would create too much boilerplate code. s.CheckErrors(nil...) s.CheckSummary("") } @@ -67,7 +71,7 @@ func (s *Suite) CheckSummary(summary string) { s.summary = "" } -func (s *Suite) Test_NewTestNameChecker(c *check.C) { +func (s *Suite) Test_NewQAChecker(c *check.C) { ck := s.Init(c) c.Check(ck.isRelevant("*_test.go", "Suite", "SetUpTest", EAll), check.Equals, true) @@ -77,7 +81,7 @@ func (s *Suite) Test_NewTestNameChecker(c *check.C) { c.Check(ck.isRelevant("*_test.go", "Suite", "TearDownTest", EMissingTest), check.Equals, false) } -func (s *Suite) Test_TestNameChecker_Configure(c *check.C) { +func (s *Suite) Test_QAChecker_Configure(c *check.C) { ck := s.Init(c) ck.Configure("*", "*", "*", ENone) // overwrite initialization from Suite.Init @@ -111,7 +115,7 @@ func (s *Suite) Test_TestNameChecker_Configure(c *check.C) { c.Check(ck.isRelevant("", "", "", EMissingTest), check.Equals, false) } -func (s *Suite) Test_TestNameChecker_Configure__ignore_single_function(c *check.C) { +func (s *Suite) Test_QAChecker_Configure__ignore_single_function(c *check.C) { ck := s.Init(c) ck.Configure("*", "*", "*", EAll) @@ -127,32 +131,35 @@ func (s *Suite) Test_TestNameChecker_Configure__ignore_single_function(c *check. c.Check(ck.isRelevant("file_test.go", "*", "TestMain", EAll), check.Equals, true) } -func (s *Suite) Test_TestNameChecker_Check(c *check.C) { +func (s *Suite) Test_QAChecker_Check(c *check.C) { ck := s.Init(c) + ck.Configure("*", "*", "", -EMissingTest) ck.Configure("*", "Suite", "*", -EMissingTest) ck.Check() s.CheckErrors( - "Missing unit test \"Test_TestNameChecker_addCode\" for \"TestNameChecker.addCode\".", - "Missing unit test \"Test_TestNameChecker_relate\" for \"TestNameChecker.relate\".", - "Missing unit test \"Test_TestNameChecker_isRelevant\" for \"TestNameChecker.isRelevant\".") + "Missing unit test \"Test_QAChecker_addCode\" for \"QAChecker.addCode\".", + "Missing unit test \"Test_QAChecker_relate\" for \"QAChecker.relate\".", + "Missing unit test \"Test_QAChecker_isRelevant\" for \"QAChecker.isRelevant\".") s.CheckSummary("3 errors.") } -func (s *Suite) Test_TestNameChecker_load__filtered_nothing(c *check.C) { +func (s *Suite) Test_QAChecker_load__filtered_nothing(c *check.C) { ck := s.Init(c) ck.Configure("*", "*", "*", ENone) ck.load(".") - c.Check(ck.testees, check.IsNil) - c.Check(ck.tests, check.IsNil) + s.CheckTestees( + nil...) + s.CheckTests( + nil...) } -func (s *Suite) Test_TestNameChecker_load__filtered_only_Value(c *check.C) { +func (s *Suite) Test_QAChecker_load__filtered_only_Value(c *check.C) { ck := s.Init(c) ck.Configure("*", "*", "*", ENone) @@ -160,13 +167,14 @@ func (s *Suite) Test_TestNameChecker_load__filtered_only_Value(c *check.C) { ck.load(".") - c.Check(ck.testees, check.DeepEquals, []*testee{ - {code{"testnames_test.go", "Value", "", 0}}, - {code{"testnames_test.go", "Value", "Method", 1}}}) - c.Check(ck.tests, check.IsNil) + s.CheckTestees( + s.newTestee("qa_test.go", "Value", "", 0), + s.newTestee("qa_test.go", "Value", "Method", 1)) + s.CheckTests( + nil...) } -func (s *Suite) Test_TestNameChecker_load__panic(c *check.C) { +func (s *Suite) Test_QAChecker_load__panic(c *check.C) { ck := s.Init(c) c.Check( @@ -175,34 +183,36 @@ func (s *Suite) Test_TestNameChecker_load__panic(c *check.C) { `^open does-not-exist\b.*`) } -func (s *Suite) Test_TestNameChecker_loadDecl(c *check.C) { +func (s *Suite) Test_QAChecker_loadDecl(c *check.C) { ck := s.Init(c) - typeDecl := func(name string) *ast.GenDecl { - return &ast.GenDecl{Specs: []ast.Spec{&ast.TypeSpec{Name: &ast.Ident{Name: name}}}} - } - funcDecl := func(name string) *ast.FuncDecl { - return &ast.FuncDecl{Name: &ast.Ident{Name: name}} - } - methodDecl := func(typeName, methodName string) *ast.FuncDecl { - return &ast.FuncDecl{ - Name: &ast.Ident{Name: methodName}, - Recv: &ast.FieldList{List: []*ast.Field{{Type: &ast.Ident{Name: typeName}}}}} + load := func(filename, decl string) { + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, filename, "package p; "+decl, 0) + c.Assert(err, check.IsNil) + ck.loadDecl(file.Decls[0], filename) } - ck.loadDecl(typeDecl("TypeName"), "file_test.go") + load("file_test.go", "type TypeName int") s.CheckTestees( s.newTestee("file_test.go", "TypeName", "", 0)) // The freestanding TestMain function is ignored by a hardcoded rule, // independently of the configuration. - ck.loadDecl(funcDecl("TestMain"), "file_test.go") + load("file_test.go", "func TestMain() {}") + + s.CheckTestees( + nil...) + s.CheckTests( + nil...) // The TestMain method on a type is relevant, but violates the naming rule. // Therefore it is ignored. - ck.loadDecl(methodDecl("Suite", "TestMain"), "file_test.go") + load("file_test.go", "func (Suite) TestMain(*check.C) {}") + s.CheckTestees( + nil...) s.CheckTests( nil...) s.CheckErrors( @@ -211,38 +221,101 @@ func (s *Suite) Test_TestNameChecker_loadDecl(c *check.C) { // The above error can be disabled, and then the method is handled // like any other test method. ck.Configure("*", "Suite", "*", -EName) - ck.loadDecl(methodDecl("Suite", "TestMain"), "file_test.go") + load("file_test.go", "func (Suite) TestMain(*check.C) {}") + s.CheckTestees( + nil...) s.CheckTests( s.newTest("file_test.go", "Suite", "TestMain", 1, "Main", "", nil)) // There is no special handling for TestMain method with a description. - ck.loadDecl(methodDecl("Suite", "TestMain__descr"), "file_test.go") + load("file_test.go", "func (Suite) TestMain__descr(*check.C) {}") + s.CheckTestees( + nil...) s.CheckTests( s.newTest("file_test.go", "Suite", "TestMain__descr", 2, "Main", "descr", nil)) + s.CheckErrors( + nil...) +} + +func (s *Suite) Test_QAChecker_parseFuncDecl(c *check.C) { + _ = s.Init(c) + + testFunc := func(filename, decl, typeName, funcName string) { + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, filename, "package p; "+decl, 0) + c.Assert(err, check.IsNil) + fn := file.Decls[0].(*ast.FuncDecl) + actual := (*QAChecker).parseFuncDecl(nil, filename, fn) + c.Check(actual, check.Equals, code{filename, typeName, funcName, 0}) + } + + testFunc("f_test.go", "func (t Type) Test() {}", + "Type", "Test") + testFunc("f_test.go", "func (t Type) Test_Type_Method() {}", + "Type", "Test_Type_Method") + testFunc("f_test.go", "func Test() {}", + "", "Test") + testFunc("f_test.go", "func Test_Type_Method() {}", + "", "Test_Type_Method") +} + +func (s *Suite) Test_QAChecker_isTest(c *check.C) { + _ = s.Init(c) + + test := func(filename, typeName, funcName string, isTest bool) { + code := code{filename, typeName, funcName, 0} + c.Check((*QAChecker).isTest(nil, code, nil), check.Equals, isTest) + } + + testFunc := func(filename, decl string, isTest bool) { + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, filename, "package p; "+decl, 0) + c.Assert(err, check.IsNil) + fn := file.Decls[0].(*ast.FuncDecl) + code := (*QAChecker).parseFuncDecl(nil, filename, fn) + c.Check((*QAChecker).isTest(nil, code, fn), check.Equals, isTest) + } + + test("f.go", "Type", "", false) + test("f.go", "", "Func", false) + test("f.go", "Type", "Method", false) + test("f.go", "Type", "Test", false) + test("f.go", "Type", "Test_Type_Method", false) + test("f.go", "", "Test_Type_Method", false) + + testFunc("f_test.go", "func (t Type) Test(c *check.C) {}", true) + testFunc("f_test.go", "func (t Type) Test_Type_Method(c *check.C) {}", true) + testFunc("f_test.go", "func Test_Type_Method(c *check.C) {}", true) + testFunc("f_test.go", "func Test_Type_Method(c *C) {}", true) + testFunc("f_test.go", "func Test_Type_Method(c C) {}", true) + testFunc("f_test.go", "func Test_Type_Method(t *testing.T) {}", true) + testFunc("f_test.go", "func Test_Type_Method(X) {}", false) + testFunc("f_test.go", "func Test_Type_Method(int) {}", false) } -func (s *Suite) Test_TestNameChecker_addTestee(c *check.C) { +func (s *Suite) Test_QAChecker_addTestee(c *check.C) { ck := s.Init(c) - code := code{"filename.go", "Type", "Method", 0} - ck.addTestee(code) + ck.addTestee(code{"filename.go", "Type", "Method", 0}) - c.Check(ck.testees, check.DeepEquals, []*testee{{code}}) + s.CheckTestees( + s.newTestee("filename.go", "Type", "Method", 0)) } -func (s *Suite) Test_TestNameChecker_addTest(c *check.C) { +func (s *Suite) Test_QAChecker_addTest(c *check.C) { ck := s.Init(c) ck.addTest(code{"filename.go", "Type", "Method", 0}) - c.Check(ck.tests, check.IsNil) + s.CheckTests( + nil...) s.CheckErrors( "Test \"Type.Method\" must start with \"Test_\".") } -func (s *Suite) Test_TestNameChecker_addTest__empty_description(c *check.C) { +func (s *Suite) Test_QAChecker_addTest__empty_description(c *check.C) { ck := s.Init(c) ck.addTest(code{"f_test.go", "Suite", "Test_Method__", 0}) @@ -258,7 +331,7 @@ func (s *Suite) Test_TestNameChecker_addTest__empty_description(c *check.C) { nil...) } -func (s *Suite) Test_TestNameChecker_addTest__suppressed_empty_description(c *check.C) { +func (s *Suite) Test_QAChecker_addTest__suppressed_empty_description(c *check.C) { ck := s.Init(c) ck.Configure("*", "*", "*", -EName) @@ -275,7 +348,7 @@ func (s *Suite) Test_TestNameChecker_addTest__suppressed_empty_description(c *ch "Missing testee \"Method\" for test \"Suite.Test_Method__\".") } -func (s *Suite) Test_TestNameChecker_nextOrder(c *check.C) { +func (s *Suite) Test_QAChecker_nextOrder(c *check.C) { ck := s.Init(c) c.Check(ck.nextOrder(), check.Equals, 0) @@ -283,7 +356,7 @@ func (s *Suite) Test_TestNameChecker_nextOrder(c *check.C) { c.Check(ck.nextOrder(), check.Equals, 2) } -func (s *Suite) Test_TestNameChecker_checkTests(c *check.C) { +func (s *Suite) Test_QAChecker_checkTests(c *check.C) { ck := s.Init(c) ck.tests = append(ck.tests, @@ -297,7 +370,7 @@ func (s *Suite) Test_TestNameChecker_checkTests(c *check.C) { "must be in source_test.go instead of wrong_test.go.") } -func (s *Suite) Test_TestNameChecker_checkTestFile__global(c *check.C) { +func (s *Suite) Test_QAChecker_checkTestFile__global(c *check.C) { ck := s.Init(c) ck.checkTestFile(&test{ @@ -311,7 +384,7 @@ func (s *Suite) Test_TestNameChecker_checkTestFile__global(c *check.C) { "must be in other_test.go instead of demo_test.go.") } -func (s *Suite) Test_TestNameChecker_checkTestTestee__global(c *check.C) { +func (s *Suite) Test_QAChecker_checkTestTestee__global(c *check.C) { ck := s.Init(c) ck.checkTestTestee(&test{ @@ -324,7 +397,7 @@ func (s *Suite) Test_TestNameChecker_checkTestTestee__global(c *check.C) { nil...) } -func (s *Suite) Test_TestNameChecker_checkTestTestee__no_testee(c *check.C) { +func (s *Suite) Test_QAChecker_checkTestTestee__no_testee(c *check.C) { ck := s.Init(c) ck.checkTestTestee(&test{ @@ -337,7 +410,7 @@ func (s *Suite) Test_TestNameChecker_checkTestTestee__no_testee(c *check.C) { "Missing testee \"Missing\" for test \"Suite.Test_Missing\".") } -func (s *Suite) Test_TestNameChecker_checkTestTestee__testee_exists(c *check.C) { +func (s *Suite) Test_QAChecker_checkTestTestee__testee_exists(c *check.C) { ck := s.Init(c) ck.checkTestTestee(&test{ @@ -350,7 +423,7 @@ func (s *Suite) Test_TestNameChecker_checkTestTestee__testee_exists(c *check.C) nil...) } -func (s *Suite) Test_TestNameChecker_checkTestDescr__camel_case(c *check.C) { +func (s *Suite) Test_QAChecker_checkTestDescr__camel_case(c *check.C) { ck := s.Init(c) ck.checkTestDescr(&test{ @@ -364,7 +437,7 @@ func (s *Suite) Test_TestNameChecker_checkTestDescr__camel_case(c *check.C) { "must not use CamelCase in the first word.") } -func (s *Suite) Test_TestNameChecker_checkTestees(c *check.C) { +func (s *Suite) Test_QAChecker_checkTestees(c *check.C) { ck := s.Init(c) ck.testees = []*testee{s.newTestee("s.go", "", "Func", 0)} @@ -376,25 +449,50 @@ func (s *Suite) Test_TestNameChecker_checkTestees(c *check.C) { "Missing unit test \"Test_Func\" for \"Func\".") } -func (s *Suite) Test_TestNameChecker_checkTesteeTest(c *check.C) { +func (s *Suite) Test_QAChecker_checkTesteesTest(c *check.C) { ck := s.Init(c) - ck.checkTesteeTest( - &testee{code{"demo.go", "Type", "", 0}}, - nil) - ck.checkTesteeTest( - &testee{code{"demo.go", "", "Func", 0}}, - nil) - ck.checkTesteeTest( - &testee{code{"demo.go", "Type", "Method", 0}}, - nil) + ck.addTestee(code{"demo.go", "Type", "", 0}) + ck.addTestee(code{"demo.go", "", "Func", 0}) + ck.addTestee(code{"demo.go", "Type", "Method", 0}) + ck.addTestee(code{"demo.go", "OkType", "", 0}) + ck.addTestee(code{"demo.go", "", "OkFunc", 0}) + ck.addTestee(code{"demo.go", "OkType", "Method", 0}) + ck.addTest(code{"demo_test.go", "", "Test_OkType", 0}) + ck.addTest(code{"demo_test.go", "", "Test_OkFunc", 0}) + ck.addTest(code{"demo_test.go", "", "Test_OkType_Method", 0}) + ck.relate() + + ck.checkTesteesTest() s.CheckErrors( + "Missing unit test \"Test_Type\" for \"Type\".", "Missing unit test \"Test_Func\" for \"Func\".", "Missing unit test \"Test_Type_Method\" for \"Type.Method\".") } -func (s *Suite) Test_TestNameChecker_errorsMask(c *check.C) { +func (s *Suite) Test_QAChecker_checkTesteesMethodsSameFile(c *check.C) { + ck := s.Init(c) + + ck.addTestee(code{"main.go", "Main", "", 0}) + ck.addTestee(code{"main.go", "Main", "MethodOk", 1}) + ck.addTestee(code{"other.go", "Main", "MethodWrong", 2}) + ck.addTestee(code{"main_test.go", "Main", "MethodOkTest", 3}) + ck.addTestee(code{"other_test.go", "Main", "MethodWrongTest", 4}) + ck.addTestee(code{"main_test.go", "T", "", 100}) + ck.addTestee(code{"main_test.go", "T", "MethodOk", 101}) + ck.addTestee(code{"other_test.go", "T", "MethodWrong", 102}) + + ck.checkTesteesMethodsSameFile() + + s.CheckErrors( + "Method Main.MethodWrong must be in main.go, like its type.", + "Method Main.MethodWrongTest must be in main_test.go, "+ + "corresponding to its type.", + "Method T.MethodWrong must be in main_test.go, like its type.") +} + +func (s *Suite) Test_QAChecker_errorsMask(c *check.C) { ck := s.Init(c) c.Check(ck.errorsMask(0, EAll), check.Equals, ^uint64(0)) @@ -403,7 +501,7 @@ func (s *Suite) Test_TestNameChecker_errorsMask(c *check.C) { c.Check(ck.errorsMask(2, EMissingTest), check.Equals, uint64(10)) } -func (s *Suite) Test_TestNameChecker_checkOrder(c *check.C) { +func (s *Suite) Test_QAChecker_checkOrder(c *check.C) { ck := s.Init(c) ck.addTestee(code{"f.go", "T", "", 10}) @@ -430,7 +528,7 @@ func (s *Suite) Test_TestNameChecker_checkOrder(c *check.C) { "Test \"S.Test_T_M2__1\" must be ordered before \"S.Test_T_M3\".") } -func (s *Suite) Test_TestNameChecker_addError(c *check.C) { +func (s *Suite) Test_QAChecker_addError(c *check.C) { ck := s.Init(c) ck.Configure("ignored*", "*", "*", -EName) @@ -443,7 +541,7 @@ func (s *Suite) Test_TestNameChecker_addError(c *check.C) { "E2") } -func (s *Suite) Test_TestNameChecker_print__empty(c *check.C) { +func (s *Suite) Test_QAChecker_print__empty(c *check.C) { var out bytes.Buffer ck := s.Init(c) ck.out = &out @@ -453,7 +551,7 @@ func (s *Suite) Test_TestNameChecker_print__empty(c *check.C) { c.Check(out.String(), check.Equals, "") } -func (s *Suite) Test_TestNameChecker_print__1_error(c *check.C) { +func (s *Suite) Test_QAChecker_print__1_error(c *check.C) { var out bytes.Buffer ck := s.Init(c) ck.out = &out @@ -466,7 +564,7 @@ func (s *Suite) Test_TestNameChecker_print__1_error(c *check.C) { s.CheckSummary("1 error.") } -func (s *Suite) Test_TestNameChecker_print__2_errors(c *check.C) { +func (s *Suite) Test_QAChecker_print__2_errors(c *check.C) { var out bytes.Buffer ck := s.Init(c) ck.out = &out @@ -532,25 +630,6 @@ func (s *Suite) Test_code_isMethod(c *check.C) { test("Type", "Method", true) } -func (s *Suite) Test_code_isTest(c *check.C) { - _ = s.Init(c) - - test := func(filename, typeName, funcName string, isTest bool) { - code := code{filename, typeName, funcName, 0} - c.Check(code.isTest(), check.Equals, isTest) - } - - test("f.go", "Type", "", false) - test("f.go", "", "Func", false) - test("f.go", "Type", "Method", false) - test("f.go", "Type", "Test", false) - test("f.go", "Type", "Test_Type_Method", false) - test("f.go", "", "Test_Type_Method", false) - test("f_test.go", "Type", "Test", true) - test("f_test.go", "Type", "Test_Type_Method", true) - test("f_test.go", "", "Test_Type_Method", true) -} - func (s *Suite) Test_code_isTestScope(c *check.C) { _ = s.Init(c) @@ -566,6 +645,21 @@ func (s *Suite) Test_code_isTestScope(c *check.C) { test("file_linux_test.go", true) } +func (s *Suite) Test_code_testFile(c *check.C) { + _ = s.Init(c) + + test := func(filename string, testFile string) { + code := code{filename, "", "", 0} + c.Check(code.testFile(), check.Equals, testFile) + } + + test("f.go", "f_test.go") + test("test.go", "test_test.go") + test("_test.go", "_test.go") + test("file_test.go", "file_test.go") + test("file_linux_test.go", "file_linux_test.go") +} + func (s *Suite) Test_isCamelCase(c *check.C) { _ = s.Init(c) @@ -624,5 +718,5 @@ func (s *Suite) Test_Value_Method(c *check.C) { type Value struct{} // Method has no star on the receiver, -// for code coverage of TestNameChecker.loadDecl. +// for code coverage of QAChecker.loadDecl. func (Value) Method() {} diff --git a/pkgtools/pkglint/files/licenses.go b/pkgtools/pkglint/files/licenses.go index 9b155649017..56f15907921 100644 --- a/pkgtools/pkglint/files/licenses.go +++ b/pkgtools/pkglint/files/licenses.go @@ -36,7 +36,8 @@ func (lc *LicenseChecker) checkName(license string) { } if !licenseFile.IsFile() { - lc.MkLine.Warnf("License file %s does not exist.", cleanpath(licenseFile)) + lc.MkLine.Warnf("License file %s does not exist.", + lc.MkLine.PathToFile(licenseFile)) } switch license { diff --git a/pkgtools/pkglint/files/licenses/licenses_test.go b/pkgtools/pkglint/files/licenses/licenses_test.go index 469e44cf1d0..edf1174e3e1 100644 --- a/pkgtools/pkglint/files/licenses/licenses_test.go +++ b/pkgtools/pkglint/files/licenses/licenses_test.go @@ -131,8 +131,8 @@ func Test(t *testing.T) { check.TestingT(t) } -func (s *Suite) Test__test_names(c *check.C) { - ck := intqa.NewTestNameChecker(c.Errorf) +func (s *Suite) Test__qa(c *check.C) { + ck := intqa.NewQAChecker(c.Errorf) ck.Configure("*", "*", "*", -intqa.EMissingTest) ck.Check() } diff --git a/pkgtools/pkglint/files/licenses_test.go b/pkgtools/pkglint/files/licenses_test.go index 62ffcf46246..f7afdd3d4db 100644 --- a/pkgtools/pkglint/files/licenses_test.go +++ b/pkgtools/pkglint/files/licenses_test.go @@ -11,7 +11,7 @@ func (s *Suite) Test_LicenseChecker_Check(c *check.C) { "The licenses for most software are designed to take away ...") test := func(licenseValue string, diagnostics ...string) { - mklines := t.NewMkLines("Makefile", + mklines := t.SetUpFileMkLines("Makefile", "LICENSE=\t"+licenseValue) mklines.ForEach(func(mkline *MkLine) { @@ -22,25 +22,33 @@ func (s *Suite) Test_LicenseChecker_Check(c *check.C) { } test("gpl-v2", - "WARN: Makefile:1: License file ~/licenses/gpl-v2 does not exist.") + "WARN: ~/Makefile:1: License file licenses/gpl-v2 does not exist.") test("no-profit shareware", - "ERROR: Makefile:1: Parse error for license condition \"no-profit shareware\".") + "ERROR: ~/Makefile:1: Parse error for license condition \"no-profit shareware\".") test("no-profit AND shareware", - "WARN: Makefile:1: License file ~/licenses/no-profit does not exist.", - "ERROR: Makefile:1: License \"no-profit\" must not be used.", - "WARN: Makefile:1: License file ~/licenses/shareware does not exist.", - "ERROR: Makefile:1: License \"shareware\" must not be used.") + "WARN: ~/Makefile:1: License file licenses/no-profit does not exist.", + "ERROR: ~/Makefile:1: License \"no-profit\" must not be used.", + "WARN: ~/Makefile:1: License file licenses/shareware does not exist.", + "ERROR: ~/Makefile:1: License \"shareware\" must not be used.") test("gnu-gpl-v2", nil...) test("gnu-gpl-v2 AND gnu-gpl-v2 OR gnu-gpl-v2", - "ERROR: Makefile:1: AND and OR operators in license conditions can only be combined using parentheses.") + "ERROR: ~/Makefile:1: AND and OR operators in license conditions "+ + "can only be combined using parentheses.") + + test("gnu-gpl-v2 AND (gnu-gpl-v2) OR gnu-gpl-v2", + "ERROR: ~/Makefile:1: AND and OR operators in license conditions "+ + "can only be combined using parentheses.") test("(gnu-gpl-v2 OR gnu-gpl-v2) AND gnu-gpl-v2", nil...) + + test("gnu-gpl-v2 OR (gnu-gpl-v2 AND gnu-gpl-v2)", + nil...) } func (s *Suite) Test_LicenseChecker_checkName__LICENSE_FILE(c *check.C) { diff --git a/pkgtools/pkglint/files/line.go b/pkgtools/pkglint/files/line.go index 03bd76718fd..ba875844463 100644 --- a/pkgtools/pkglint/files/line.go +++ b/pkgtools/pkglint/files/line.go @@ -119,7 +119,7 @@ func (line *Line) RefToLocation(other Location) string { // This is typically used for arguments in diagnostics, which should always be // relative to the line with which the diagnostic is associated. func (line *Line) PathToFile(filePath Path) Path { - return relpath(line.Filename.Dir(), filePath) + return G.Pkgsrc.Relpath(line.Filename.Dir(), filePath) } func (line *Line) IsMultiline() bool { @@ -182,6 +182,10 @@ func (line *Line) String() string { func (line *Line) Autofix() *Autofix { if line.autofix == nil { line.autofix = NewAutofix(line) + } else { + // This assertion fails if an Autofix is reused before + // its Apply method is called. + assert(line.autofix.autofixShortTerm.diagFormat == "") } return line.autofix } diff --git a/pkgtools/pkglint/files/line_test.go b/pkgtools/pkglint/files/line_test.go index f636abf0173..0dcea7d1977 100644 --- a/pkgtools/pkglint/files/line_test.go +++ b/pkgtools/pkglint/files/line_test.go @@ -53,3 +53,21 @@ func (s *Suite) Test_Line_Fatalf__trace(c *check.C) { "TRACE: - (*Suite).Test_Line_Fatalf__trace.func2()", "FATAL: filename:123: Cannot continue because of \"reason 1\" and \"reason 2\".") } + +func (s *Suite) Test_Line_Autofix__reuse_incomplete(c *check.C) { + t := s.Init(c) + + line := t.NewLine("filename.mk", 1, "") + + fix := line.Autofix() + fix.Warnf("Warning.") + // For some reason, the other required calls are left out. + + t.ExpectAssert(func() { _ = line.Autofix() }) + + // Properly finish the standard call sequence for an Autofix. + fix.Apply() + + t.CheckOutputLines( + "WARN: filename.mk:1: Warning.") +} diff --git a/pkgtools/pkglint/files/logging.go b/pkgtools/pkglint/files/logging.go index f6e54ca2fbc..89201974fee 100644 --- a/pkgtools/pkglint/files/logging.go +++ b/pkgtools/pkglint/files/logging.go @@ -18,6 +18,7 @@ type Logger struct { suppressExpl bool prevLine *Line + verbose bool // allow duplicate diagnostics, even in the same line logged Once explained Once histo *histogram.Histogram @@ -34,7 +35,6 @@ type LoggerOpts struct { Autofix, Explain, ShowSource, - LogVerbose, GccOutput, Quiet bool } @@ -52,8 +52,6 @@ var ( AutofixLogLevel = &LogLevel{"AUTOFIX", "autofix"} ) -var dummyLine = NewLineMulti("", 0, 0, "", nil) - // Explain outputs an explanation for the preceding diagnostic // if the --explain option is given. Otherwise it just records // that an explanation is available. @@ -126,7 +124,7 @@ func (l *Logger) Diag(line *Line, level *LogLevel, format string, args ...interf } func (l *Logger) FirstTime(filename Path, linenos, msg string) bool { - if l.Opts.LogVerbose { + if l.verbose { return true } @@ -283,13 +281,13 @@ func (l *Logger) Logf(level *LogLevel, filename Path, lineno, format, msg string } } -// Errorf logs a technical error on the error output. +// TechErrorf logs a technical error on the error output. // // location must be a slash-separated filename, such as the one in // Location.Filename. It may be followed by the usual ":123" for line numbers. // // For diagnostics, use Logf instead. -func (l *Logger) Errorf(location Path, format string, args ...interface{}) { +func (l *Logger) TechErrorf(location Path, format string, args ...interface{}) { msg := sprintf(format, args...) var diag string if l.Opts.GccOutput { @@ -359,6 +357,7 @@ type SeparatorWriter struct { } func NewSeparatorWriter(out io.Writer) *SeparatorWriter { + assertNotNil(out) return &SeparatorWriter{out, 3, bytes.Buffer{}} } diff --git a/pkgtools/pkglint/files/logging_test.go b/pkgtools/pkglint/files/logging_test.go index 5ee4c6372a0..c346f468c1c 100644 --- a/pkgtools/pkglint/files/logging_test.go +++ b/pkgtools/pkglint/files/logging_test.go @@ -248,6 +248,10 @@ func (s *Suite) Test_Logger_Diag__show_source_with_whole_file(c *check.C) { func (s *Suite) Test_Logger_Diag__source_duplicates(c *check.C) { t := s.Init(c) + // Up to pkglint 19.3.10, this variable had been reset during + // command line parsing. In 19.3.11 the command line option has + // been removed, therefore it must be reset manually. + G.Logger.verbose = false t.SetUpPkgsrc() t.CreateFileLines("category/dependency/patches/patch-aa", CvsID, @@ -682,7 +686,7 @@ func (s *Suite) Test_Logger_Logf__duplicate_messages(c *check.C) { t := s.Init(c) t.SetUpCommandLine("--explain") - G.Logger.Opts.LogVerbose = false + G.Logger.verbose = false line := t.NewLine("README.txt", 123, "text") // Is logged because it is the first appearance of this warning. @@ -780,7 +784,7 @@ func (s *Suite) Test_Logger_Logf__duplicate_autofix(c *check.C) { t := s.Init(c) t.SetUpCommandLine("--explain", "--autofix") - G.Logger.Opts.LogVerbose = false // See SetUpTest + G.Logger.verbose = false // See SetUpTest line := t.NewLine("README.txt", 123, "text") fix := line.Autofix() @@ -801,12 +805,12 @@ func (s *Suite) Test_Logger_Logf__panic(c *check.C) { "Pkglint internal error: Diagnostic format \"No period\" must end in a period.") } -func (s *Suite) Test_Logger_Errorf__gcc_format(c *check.C) { +func (s *Suite) Test_Logger_TechErrorf__gcc_format(c *check.C) { t := s.Init(c) t.SetUpCommandLine("--gcc-output-format") - G.Logger.Errorf("filename", "Cannot be opened for %s.", "reading") + G.Logger.TechErrorf("filename", "Cannot be opened for %s.", "reading") t.CheckOutputLines( "filename: error: Cannot be opened for reading.") diff --git a/pkgtools/pkglint/files/mklexer.go b/pkgtools/pkglint/files/mklexer.go new file mode 100644 index 00000000000..92c89a2e3b1 --- /dev/null +++ b/pkgtools/pkglint/files/mklexer.go @@ -0,0 +1,414 @@ +package pkglint + +import ( + "netbsd.org/pkglint/regex" + "netbsd.org/pkglint/textproc" + "strings" +) + +// MkLexer splits a text into a sequence of variable uses +// and plain text. +type MkLexer struct { + lexer *textproc.Lexer + line *Line +} + +func NewMkLexer(text string, line *Line) *MkLexer { + return &MkLexer{textproc.NewLexer(text), line} +} + +// MkTokens splits a text like in the following example: +// Text${VAR:Mmodifier}${VAR2}more text${VAR3} +// into tokens like these: +// Text +// ${VAR:Mmodifier} +// ${VAR2} +// more text +// ${VAR3} +func (p *MkLexer) MkTokens() ([]*MkToken, string) { + lexer := p.lexer + + var tokens []*MkToken + for !lexer.EOF() { + mark := lexer.Mark() + if varuse := p.VarUse(); varuse != nil { + tokens = append(tokens, &MkToken{Text: lexer.Since(mark), Varuse: varuse}) + continue + } + + for lexer.NextBytesFunc(func(b byte) bool { return b != '$' }) != "" || lexer.SkipString("$$") { + } + text := lexer.Since(mark) + if text != "" { + tokens = append(tokens, &MkToken{Text: text}) + continue + } + + break + } + return tokens, lexer.Rest() +} + +func (p *MkLexer) VarUse() *MkVarUse { + rest := p.lexer.Rest() + if len(rest) < 2 || rest[0] != '$' { + return nil + } + + switch rest[1] { + case '{', '(': + return p.varUseBrace(rest[1] == '(') + + case '$': + // This is an escaped dollar character and not a variable use. + return nil + + case '@', '<', ' ': + // These variable names are known to exist. + // + // Many others are also possible but not used in practice. + // In particular, when parsing the :C or :S modifier, + // the $ must not be interpreted as a variable name, + // even when it looks like $/ could refer to the "/" variable. + // + // TODO: Find out whether $" is a variable use when it appears in the :M modifier. + p.lexer.Skip(2) + return &MkVarUse{rest[1:2], nil} + + default: + return p.varUseAlnum() + } +} + +// varUseBrace parses: +// ${VAR} +// ${arbitrary text:L} +// ${variable with invalid chars} +// $(PARENTHESES) +// ${VAR:Mpattern:C,:,colon,g:Q:Q:Q} +func (p *MkLexer) varUseBrace(usingRoundParen bool) *MkVarUse { + lexer := p.lexer + + beforeDollar := lexer.Mark() + lexer.Skip(2) + + closing := byte('}') + if usingRoundParen { + closing = ')' + } + + beforeVarname := lexer.Mark() + varname := p.Varname() + p.varUseText(closing) + varExpr := lexer.Since(beforeVarname) + + modifiers := p.VarUseModifiers(varExpr, closing) + + closed := lexer.SkipByte(closing) + + if p.line != nil { + if !closed { + p.line.Warnf("Missing closing %q for %q.", string(rune(closing)), varExpr) + } + + if usingRoundParen && closed { + parenVaruse := lexer.Since(beforeDollar) + edit := []byte(parenVaruse) + edit[1] = '{' + edit[len(edit)-1] = '}' + bracesVaruse := string(edit) + + fix := p.line.Autofix() + fix.Warnf("Please use curly braces {} instead of round parentheses () for %s.", varExpr) + fix.Replace(parenVaruse, bracesVaruse) + fix.Apply() + } + + if len(varExpr) > len(varname) && !(&MkVarUse{varExpr, modifiers}).IsExpression() { + p.line.Warnf("Invalid part %q after variable name %q.", varExpr[len(varname):], varname) + } + } + + return &MkVarUse{varExpr, modifiers} +} + +func (p *MkLexer) Varname() string { + lexer := p.lexer + + // TODO: duplicated code in MatchVarassign + mark := lexer.Mark() + lexer.SkipByte('.') + for lexer.NextBytesSet(VarbaseBytes) != "" || p.VarUse() != nil { + } + if lexer.SkipByte('.') || hasPrefix(lexer.Since(mark), "SITES_") { + for lexer.NextBytesSet(VarparamBytes) != "" || p.VarUse() != nil { + } + } + return lexer.Since(mark) +} + +// varUseText parses any text up to the next colon or closing mark. +// Nested variable uses are parsed as well. +// +// This is used for the :L and :? modifiers since they accept arbitrary +// text as the "variable name" and effectively interpret it as the variable +// value instead. +func (p *MkLexer) varUseText(closing byte) string { + lexer := p.lexer + start := lexer.Mark() + re := regcomp(regex.Pattern(condStr(closing == '}', `^([^$:}]|\$\$)+`, `^([^$:)]|\$\$)+`))) + for p.VarUse() != nil || lexer.SkipRegexp(re) { + } + return lexer.Since(start) +} + +// VarUseModifiers parses the modifiers of a variable being used, such as :Q, :Mpattern. +// +// See the bmake manual page. +func (p *MkLexer) VarUseModifiers(varname string, closing byte) []MkVarUseModifier { + lexer := p.lexer + + var modifiers []MkVarUseModifier + // The :S and :C modifiers may be chained without using the : as separator. + mayOmitColon := false + + for lexer.SkipByte(':') || mayOmitColon { + modifier := p.varUseModifier(varname, closing) + if modifier != "" { + modifiers = append(modifiers, MkVarUseModifier{modifier}) + } + mayOmitColon = modifier != "" && (modifier[0] == 'S' || modifier[0] == 'C') + } + return modifiers +} + +// varUseModifier parses a single variable modifier such as :Q or :S,from,to,. +// The actual parsing starts after the leading colon. +func (p *MkLexer) varUseModifier(varname string, closing byte) string { + lexer := p.lexer + mark := lexer.Mark() + + switch lexer.PeekByte() { + case 'E', 'H', 'L', 'O', 'Q', 'R', 'T', 's', 't', 'u': + mod := lexer.NextBytesSet(textproc.Alnum) + + switch mod { + case + "E", // Extension, e.g. path/file.suffix => suffix + "H", // Head, e.g. dir/subdir/file.suffix => dir/subdir + "L", // XXX: Shouldn't this be handled specially? + "O", // Order alphabetically + "Ox", // Shuffle + "Q", // Quote shell meta-characters + "R", // Strip the file suffix, e.g. path/file.suffix => file + "T", // Basename, e.g. path/file.suffix => file.suffix + "sh", // Evaluate the variable value as shell command + "tA", // Try to convert to absolute path + "tW", // Causes the value to be treated as a single word + "tl", // To lowercase + "tu", // To uppercase + "tw", // Causes the value to be treated as list of words + "u": // Remove adjacent duplicate words (like uniq(1)) + return mod + } + + if hasPrefix(mod, "ts") { + // See devel/bmake/files/var.c:/case 't' + sep := mod[2:] + p.varUseText(closing) + switch { + case sep == "": + lexer.SkipString(":") + case len(sep) == 1: + break + case matches(sep, `^\\\d+`): + break + default: + if p.line != nil { + p.line.Warnf("Invalid separator %q for :ts modifier of %q.", sep, varname) + p.line.Explain( + "The separator for the :ts modifier must be either a single character", + "or an escape sequence like \\t or \\n or an octal or decimal escape", + "sequence; see the bmake man page for further details.") + } + } + return lexer.Since(mark) + } + + case '=', 'D', 'M', 'N', 'U': + lexer.Skip(1) + re := regcomp(regex.Pattern(condStr(closing == '}', `^([^$:\\}]|\$\$|\\.)+`, `^([^$:\\)]|\$\$|\\.)+`))) + for p.VarUse() != nil || lexer.SkipRegexp(re) { + } + arg := lexer.Since(mark) + return strings.Replace(arg, "\\:", ":", -1) + + case 'C', 'S': + if ok, _, _, _, _ := p.varUseModifierSubst(closing); ok { + return lexer.Since(mark) + } + + case '@': + if p.varUseModifierAt(lexer, varname) { + return lexer.Since(mark) + } + + case '[': + if lexer.SkipRegexp(regcomp(`^\[(?:[-.\d]+|#)\]`)) { + return lexer.Since(mark) + } + + case '?': + lexer.Skip(1) + p.varUseText(closing) + if lexer.SkipByte(':') { + p.varUseText(closing) + return lexer.Since(mark) + } + } + + lexer.Reset(mark) + + re := regcomp(regex.Pattern(condStr(closing == '}', `^([^:$}]|\$\$)+`, `^([^:$)]|\$\$)+`))) + for p.VarUse() != nil || lexer.SkipRegexp(re) { + } + modifier := lexer.Since(mark) + + // ${SOURCES:%.c=%.o} or ${:!uname -a!:[2]} + if contains(modifier, "=") || (hasPrefix(modifier, "!") && hasSuffix(modifier, "!")) { + return modifier + } + + if p.line != nil && modifier != "" { + p.line.Warnf("Invalid variable modifier %q for %q.", modifier, varname) + } + + return "" +} + +// varUseModifierSubst parses a :S,from,to, or a :C,from,to, modifier. +func (p *MkLexer) varUseModifierSubst(closing byte) (ok bool, regex bool, from string, to string, options string) { + lexer := p.lexer + regex = lexer.PeekByte() == 'C' + lexer.Skip(1 /* the initial S or C */) + + sep := lexer.PeekByte() // bmake allows _any_ separator, even letters. + if sep == -1 || byte(sep) == closing { + return + } + + lexer.Skip(1) + separator := byte(sep) + + unescape := func(s string) string { + return strings.Replace(s, "\\"+string(separator), string(separator), -1) + } + + isOther := func(b byte) bool { + return b != separator && b != '$' && b != '\\' + } + + skipOther := func() { + for { + switch { + + case p.VarUse() != nil: + break + + case lexer.SkipString("$$"): + break + + case len(lexer.Rest()) >= 2 && lexer.PeekByte() == '\\' && separator != '\\': + _ = lexer.Skip(2) + + case lexer.NextBytesFunc(isOther) != "": + break + + default: + return + } + } + } + + fromStart := lexer.Mark() + lexer.SkipByte('^') + skipOther() + lexer.SkipByte('$') + from = unescape(lexer.Since(fromStart)) + + if !lexer.SkipByte(separator) { + return + } + + toStart := lexer.Mark() + skipOther() + to = unescape(lexer.Since(toStart)) + + if !lexer.SkipByte(separator) { + return + } + + optionsStart := lexer.Mark() + lexer.NextBytesFunc(func(b byte) bool { return b == '1' || b == 'g' || b == 'W' }) + options = lexer.Since(optionsStart) + + ok = true + return +} + +// varUseModifierAt parses a variable modifier like ":@v@echo ${v};@", +// which expands the variable value in a loop. +func (p *MkLexer) varUseModifierAt(lexer *textproc.Lexer, varname string) bool { + lexer.Skip(1 /* the initial @ */) + + loopVar := lexer.NextBytesSet(AlnumDot) + if loopVar == "" || !lexer.SkipByte('@') { + return false + } + + re := regcomp(`^([^$@\\]|\\.)+`) + for p.VarUse() != nil || lexer.SkipString("$$") || lexer.SkipRegexp(re) { + } + + if !lexer.SkipByte('@') && p.line != nil { + p.line.Warnf("Modifier ${%s:@%s@...@} is missing the final \"@\".", varname, loopVar) + } + + return true +} + +func (p *MkLexer) varUseAlnum() *MkVarUse { + lexer := p.lexer + + apparentVarname := textproc.NewLexer(lexer.Rest()[1:]).NextBytesSet(textproc.AlnumU) + if apparentVarname == "" { + return nil + } + + lexer.Skip(2) + + if p.line != nil { + if len(apparentVarname) > 1 { + p.line.Errorf("$%[1]s is ambiguous. Use ${%[1]s} if you mean a Make variable or $$%[1]s if you mean a shell variable.", + apparentVarname) + p.line.Explain( + "Only the first letter after the dollar is the variable name.", + "Everything following it is normal text, even if it looks like a variable name to human readers.") + } else { + p.line.Warnf("$%[1]s is ambiguous. Use ${%[1]s} if you mean a Make variable or $$%[1]s if you mean a shell variable.", apparentVarname) + p.line.Explain( + "In its current form, this variable is parsed as a Make variable.", + "For human readers though, $x looks more like a shell variable than a Make variable,", + "since Make variables are usually written using braces (BSD-style) or parentheses (GNU-style).") + } + } + + return &MkVarUse{apparentVarname[:1], nil} +} + +func (p *MkLexer) EOF() bool { + return p.lexer.EOF() +} + +func (p *MkLexer) Rest() string { + return p.lexer.Rest() +} diff --git a/pkgtools/pkglint/files/mklexer_test.go b/pkgtools/pkglint/files/mklexer_test.go new file mode 100644 index 00000000000..c11adbc1441 --- /dev/null +++ b/pkgtools/pkglint/files/mklexer_test.go @@ -0,0 +1,750 @@ +package pkglint + +import "gopkg.in/check.v1" + +func (s *Suite) Test_MkLexer_MkTokens(c *check.C) { + t := s.Init(c) + b := NewMkTokenBuilder() + + testRest := func(input string, expectedTokens []*MkToken, expectedRest string) { + line := t.NewLines("Test_MkLexer_MkTokens.mk", input).Lines[0] + p := NewMkLexer(input, line) + actualTokens, rest := p.MkTokens() + t.CheckDeepEquals(actualTokens, expectedTokens) + for i, expectedToken := range expectedTokens { + if i < len(actualTokens) { + t.CheckDeepEquals(*actualTokens[i], *expectedToken) + t.CheckDeepEquals(actualTokens[i].Varuse, expectedToken.Varuse) + } + } + t.CheckEquals(rest, expectedRest) + } + test := func(input string, expectedToken *MkToken) { + testRest(input, b.Tokens(expectedToken), "") + } + literal := b.TextToken + varuse := b.VaruseToken + + // Everything except VarUses is passed through unmodified. + + test("literal", + literal("literal")) + + test("\\/share\\/ { print \"share directory\" }", + literal("\\/share\\/ { print \"share directory\" }")) + + test("find . -name \\*.orig -o -name \\*.pre", + literal("find . -name \\*.orig -o -name \\*.pre")) + + test("-e 's|\\$${EC2_HOME.*}|EC2_HOME}|g'", + literal("-e 's|\\$${EC2_HOME.*}|EC2_HOME}|g'")) + + test("$$var1 $$var2 $$? $$", + literal("$$var1 $$var2 $$? $$")) + + testRest("hello, ${W:L:tl}orld", + b.Tokens( + literal("hello, "), + varuse("W", "L", "tl"), + literal("orld")), + "") + + testRest("ftp://${PKGNAME}/ ${MASTER_SITES:=subdir/}", + b.Tokens( + literal("ftp://"), + varuse("PKGNAME"), + literal("/ "), + varuse("MASTER_SITES", "=subdir/")), + "") + + testRest("${VAR:S,a,b,c,d,e,f}", + b.Tokens(b.VaruseTextToken("${VAR:S,a,b,c,d,e,f}", "VAR", "S,a,b,")), + "") + t.CheckOutputLines( + "WARN: Test_MkLexer_MkTokens.mk:1: Invalid variable modifier \"c,d,e,f\" for \"VAR\".") + + testRest("Text${VAR:Mmodifier}${VAR2}more text${VAR3}", + b.Tokens( + literal("Text"), + varuse("VAR", "Mmodifier"), + varuse("VAR2"), + literal("more text"), + varuse("VAR3")), + "") +} + +func (s *Suite) Test_MkLexer_VarUse(c *check.C) { + t := s.Init(c) + b := NewMkTokenBuilder() + varuse := b.VaruseToken + varuseText := b.VaruseTextToken + + testRest := func(input string, expectedTokens []*MkToken, expectedRest string, diagnostics ...string) { + line := t.NewLines("Test_MkLexer_VarUse.mk", input).Lines[0] + p := NewMkLexer(input, line) + + actualTokens, rest := p.MkTokens() + + t.CheckDeepEquals(actualTokens, expectedTokens) + for i, expectedToken := range expectedTokens { + if i < len(actualTokens) { + t.CheckDeepEquals(*actualTokens[i], *expectedToken) + t.CheckDeepEquals(actualTokens[i].Varuse, expectedToken.Varuse) + } + } + t.CheckEquals(rest, expectedRest) + t.CheckOutput(diagnostics) + } + test := func(input string, expectedToken *MkToken, diagnostics ...string) { + testRest(input, b.Tokens(expectedToken), "", diagnostics...) + } + + t.Use(testRest, test, varuse, varuseText) + + test("${VARIABLE}", + varuse("VARIABLE")) + + test("${VARIABLE.param}", + varuse("VARIABLE.param")) + + test("${VARIABLE.${param}}", + varuse("VARIABLE.${param}")) + + test("${VARIABLE.hicolor-icon-theme}", + varuse("VARIABLE.hicolor-icon-theme")) + + test("${VARIABLE.gtk+extra}", + varuse("VARIABLE.gtk+extra")) + + test("${VARIABLE:S/old/new/}", + varuse("VARIABLE", "S/old/new/")) + + test("${GNUSTEP_LFLAGS:S/-L//g}", + varuse("GNUSTEP_LFLAGS", "S/-L//g")) + + test("${SUSE_VERSION:S/.//}", + varuse("SUSE_VERSION", "S/.//")) + + test("${MASTER_SITE_GNOME:=sources/alacarte/0.13/}", + varuse("MASTER_SITE_GNOME", "=sources/alacarte/0.13/")) + + test("${INCLUDE_DIRS:H:T}", + varuse("INCLUDE_DIRS", "H", "T")) + + test("${A.${B.${C.${D}}}}", + varuse("A.${B.${C.${D}}}")) + + test("${RUBY_VERSION:C/([0-9]+)\\.([0-9]+)\\.([0-9]+)/\\1/}", + varuse("RUBY_VERSION", "C/([0-9]+)\\.([0-9]+)\\.([0-9]+)/\\1/")) + + test("${PERL5_${_var_}:Q}", + varuse("PERL5_${_var_}", "Q")) + + test("${PKGNAME_REQD:C/(^.*-|^)py([0-9][0-9])-.*/\\2/}", + varuse("PKGNAME_REQD", "C/(^.*-|^)py([0-9][0-9])-.*/\\2/")) + + test("${PYLIB:S|/|\\\\/|g}", + varuse("PYLIB", "S|/|\\\\/|g")) + + test("${PKGNAME_REQD:C/ruby([0-9][0-9]+)-.*/\\1/}", + varuse("PKGNAME_REQD", "C/ruby([0-9][0-9]+)-.*/\\1/")) + + test("${RUBY_SHLIBALIAS:S/\\//\\\\\\//}", + varuse("RUBY_SHLIBALIAS", "S/\\//\\\\\\//")) + + test("${RUBY_VER_MAP.${RUBY_VER}:U${RUBY_VER}}", + varuse("RUBY_VER_MAP.${RUBY_VER}", "U${RUBY_VER}")) + + test("${RUBY_VER_MAP.${RUBY_VER}:U18}", + varuse("RUBY_VER_MAP.${RUBY_VER}", "U18")) + + test("${CONFIGURE_ARGS:S/ENABLE_OSS=no/ENABLE_OSS=yes/g}", + varuse("CONFIGURE_ARGS", "S/ENABLE_OSS=no/ENABLE_OSS=yes/g")) + + test("${PLIST_RUBY_DIRS:S,DIR=\"PREFIX/,DIR=\",}", + varuse("PLIST_RUBY_DIRS", "S,DIR=\"PREFIX/,DIR=\",")) + + test("${LDFLAGS:S/-Wl,//g:Q}", + varuse("LDFLAGS", "S/-Wl,//g", "Q")) + + test("${_PERL5_REAL_PACKLIST:S/^/${DESTDIR}/}", + varuse("_PERL5_REAL_PACKLIST", "S/^/${DESTDIR}/")) + + test("${_PYTHON_VERSION:C/^([0-9])/\\1./1}", + varuse("_PYTHON_VERSION", "C/^([0-9])/\\1./1")) + + test("${PKGNAME:S/py${_PYTHON_VERSION}/py${i}/}", + varuse("PKGNAME", "S/py${_PYTHON_VERSION}/py${i}/")) + + test("${PKGNAME:C/-[0-9].*$/-[0-9]*/}", + varuse("PKGNAME", "C/-[0-9].*$/-[0-9]*/")) + + // TODO: Does the $@ refer to ${.TARGET}, or is it just an unmatchable + // regular expression? + test("${PKGNAME:C/$@/target?/}", + varuse("PKGNAME", "C/$@/target?/")) + + test("${PKGNAME:S/py${_PYTHON_VERSION}/py${i}/:C/-[0-9].*$/-[0-9]*/}", + varuse("PKGNAME", "S/py${_PYTHON_VERSION}/py${i}/", "C/-[0-9].*$/-[0-9]*/")) + + test("${_PERL5_VARS:tl:S/^/-V:/}", + varuse("_PERL5_VARS", "tl", "S/^/-V:/")) + + test("${_PERL5_VARS_OUT:M${_var_:tl}=*:S/^${_var_:tl}=${_PERL5_PREFIX:=/}//}", + varuse("_PERL5_VARS_OUT", "M${_var_:tl}=*", "S/^${_var_:tl}=${_PERL5_PREFIX:=/}//")) + + test("${RUBY${RUBY_VER}_PATCHLEVEL}", + varuse("RUBY${RUBY_VER}_PATCHLEVEL")) + + test("${DISTFILES:M*.gem}", + varuse("DISTFILES", "M*.gem")) + + test("${LOCALBASE:S^/^_^}", + varuse("LOCALBASE", "S^/^_^")) + + test("${SOURCES:%.c=%.o}", + varuse("SOURCES", "%.c=%.o")) + + test("${GIT_TEMPLATES:@.t.@ ${EGDIR}/${GIT_TEMPLATEDIR}/${.t.} ${PREFIX}/${GIT_CORE_TEMPLATEDIR}/${.t.} @:M*}", + varuse("GIT_TEMPLATES", "@.t.@ ${EGDIR}/${GIT_TEMPLATEDIR}/${.t.} ${PREFIX}/${GIT_CORE_TEMPLATEDIR}/${.t.} @", "M*")) + + test("${DISTNAME:C:_:-:}", + varuse("DISTNAME", "C:_:-:")) + + test("${CF_FILES:H:O:u:S@^@${PKG_SYSCONFDIR}/@}", + varuse("CF_FILES", "H", "O", "u", "S@^@${PKG_SYSCONFDIR}/@")) + + test("${ALT_GCC_RTS:S%${LOCALBASE}%%:S%/%%}", + varuse("ALT_GCC_RTS", "S%${LOCALBASE}%%", "S%/%%")) + + test("${PREFIX:C;///*;/;g:C;/$;;}", + varuse("PREFIX", "C;///*;/;g", "C;/$;;")) + + test("${GZIP_CMD:[1]:Q}", + varuse("GZIP_CMD", "[1]", "Q")) + + test("${RUBY_RAILS_SUPPORTED:[#]}", + varuse("RUBY_RAILS_SUPPORTED", "[#]")) + + test("${GZIP_CMD:[asdf]:Q}", + varuseText("${GZIP_CMD:[asdf]:Q}", "GZIP_CMD", "Q"), + "WARN: Test_MkLexer_VarUse.mk:1: Invalid variable modifier \"[asdf]\" for \"GZIP_CMD\".") + + test("${DISTNAME:C/-[0-9]+$$//:C/_/-/}", + varuse("DISTNAME", "C/-[0-9]+$$//", "C/_/-/")) + + test("${DISTNAME:slang%=slang2%}", + varuse("DISTNAME", "slang%=slang2%")) + + test("${OSMAP_SUBSTVARS:@v@-e 's,\\@${v}\\@,${${v}},g' @}", + varuse("OSMAP_SUBSTVARS", "@v@-e 's,\\@${v}\\@,${${v}},g' @")) + + test("${BRANDELF:D${BRANDELF} -t Linux ${LINUX_LDCONFIG}:U${TRUE}}", + varuse("BRANDELF", "D${BRANDELF} -t Linux ${LINUX_LDCONFIG}", "U${TRUE}")) + + test("${${_var_}.*}", + varuse("${_var_}.*")) + + test("${OPTIONS:@opt@printf 'Option %s is selected\n' ${opt:Q}';@}", + varuse("OPTIONS", "@opt@printf 'Option %s is selected\n' ${opt:Q}';@")) + + /* weird features */ + test("${${EMACS_VERSION_MAJOR}>22:?@comment :}", + varuse("${EMACS_VERSION_MAJOR}>22", "?@comment :")) + + test("${empty(CFLAGS):?:-cflags ${CFLAGS:Q}}", + varuse("empty(CFLAGS)", "?:-cflags ${CFLAGS:Q}")) + + test("${${PKGSRC_COMPILER}==gcc:?gcc:cc}", + varuse("${PKGSRC_COMPILER}==gcc", "?gcc:cc")) + + test("${${XKBBASE}/xkbcomp:L:Q}", + varuse("${XKBBASE}/xkbcomp", "L", "Q")) + + test("${${PKGBASE} ${PKGVERSION}:L}", + varuse("${PKGBASE} ${PKGVERSION}", "L")) + + // The variable name is optional; the variable with the empty name always + // evaluates to the empty string. Bmake actively prevents this variable from + // ever being defined. Therefore the :U branch is always taken, and this + // in turn is used to implement the variables from the .for loops. + test("${:U}", + varuse("", "U")) + + test("${:Ufixed value}", + varuse("", "Ufixed value")) + + // This complicated expression returns the major.minor.patch version + // of the package given in ${d}. + // + // The :L modifier interprets the variable name not as a variable name + // but takes it as the variable value. Followed by the :sh modifier, + // this combination evaluates to the output of pkg_info. + // + // In this output, all non-digit characters are replaced with spaces so + // that the remaining value is a space-separated list of version parts. + // From these parts, the first 3 are taken and joined using a dot as separator. + test("${${${PKG_INFO} -E ${d} || echo:L:sh}:L:C/[^[0-9]]*/ /g:[1..3]:ts.}", + varuse("${${PKG_INFO} -E ${d} || echo:L:sh}", "L", "C/[^[0-9]]*/ /g", "[1..3]", "ts.")) + + // For :S and :C, the colon can be left out. It's confusing but possible. + test("${VAR:S/-//S/.//}", + varuseText("${VAR:S/-//S/.//}", "VAR", "S/-//", "S/.//")) + + // The :S and :C modifiers accept an arbitrary character as separator. Here it is "a". + test("${VAR:Sahara}", + varuse("VAR", "Sahara")) + + // The separator character can be left out, which means empty. + test("${VAR:ts}", + varuse("VAR", "ts")) + + // The separator character can be a long octal number. + test("${VAR:ts\\000012}", + varuse("VAR", "ts\\000012")) + + // Or even decimal. + test("${VAR:ts\\124}", + varuse("VAR", "ts\\124")) + + // The :ts modifier only takes single-character separators. + test("${VAR:ts---}", + varuse("VAR", "ts---"), + "WARN: Test_MkLexer_VarUse.mk:1: Invalid separator \"---\" for :ts modifier of \"VAR\".") + + test("$<", + varuseText("$<", "<")) // Same as ${.IMPSRC} + + test("$(GNUSTEP_USER_ROOT)", + varuseText("$(GNUSTEP_USER_ROOT)", "GNUSTEP_USER_ROOT"), + "WARN: Test_MkLexer_VarUse.mk:1: Please use curly braces {} instead of round parentheses () for GNUSTEP_USER_ROOT.") + + // Opening brace, closing parenthesis. + // Warnings are only printed for balanced expressions. + test("${VAR)", + varuseText("${VAR)", "VAR)"), + "WARN: Test_MkLexer_VarUse.mk:1: Missing closing \"}\" for \"VAR)\".", + "WARN: Test_MkLexer_VarUse.mk:1: Invalid part \")\" after variable name \"VAR\".") + + // Opening parenthesis, closing brace + // Warnings are only printed for balanced expressions. + test("$(VAR}", + varuseText("$(VAR}", "VAR}"), + "WARN: Test_MkLexer_VarUse.mk:1: Missing closing \")\" for \"VAR}\".", + "WARN: Test_MkLexer_VarUse.mk:1: Invalid part \"}\" after variable name \"VAR\".") + + test("${PLIST_SUBST_VARS:@var@${var}=${${var}:Q}@}", + varuse("PLIST_SUBST_VARS", "@var@${var}=${${var}:Q}@")) + + test("${PLIST_SUBST_VARS:@var@${var}=${${var}:Q}}", + varuseText("${PLIST_SUBST_VARS:@var@${var}=${${var}:Q}}", + "PLIST_SUBST_VARS", "@var@${var}=${${var}:Q}}"), + "WARN: Test_MkLexer_VarUse.mk:1: Modifier ${PLIST_SUBST_VARS:@var@...@} is missing the final \"@\".", + "WARN: Test_MkLexer_VarUse.mk:1: Missing closing \"}\" for \"PLIST_SUBST_VARS\".") + + // The replacement text may include closing braces, which is useful + // for AWK programs. + test("${PLIST_SUBST_VARS:@var@{${var}}@}", + varuseText("${PLIST_SUBST_VARS:@var@{${var}}@}", + "PLIST_SUBST_VARS", "@var@{${var}}@"), + nil...) + + // Unfinished variable use + test("${", + varuseText("${", ""), + "WARN: Test_MkLexer_VarUse.mk:1: Missing closing \"}\" for \"\".") + + // Unfinished nested variable use + test("${${", + varuseText("${${", "${"), + "WARN: Test_MkLexer_VarUse.mk:1: Missing closing \"}\" for \"\".", + "WARN: Test_MkLexer_VarUse.mk:1: Missing closing \"}\" for \"${\".") + + test("${arbitrary :Mpattern:---:Q}", + varuseText("${arbitrary :Mpattern:---:Q}", "arbitrary ", "Mpattern", "Q"), + // TODO: Swap the order of these message + "WARN: Test_MkLexer_VarUse.mk:1: Invalid variable modifier \"---\" for \"arbitrary \".", + "WARN: Test_MkLexer_VarUse.mk:1: Invalid part \" \" after variable name \"arbitrary\".") + + // Variable names containing spaces do not occur in pkgsrc. + // Technically they are possible: + // + // VARNAME= name with spaces + // ${VARNAME}= value + // + // all: + // @echo ${name with spaces:Q}'' + test("${arbitrary text}", + varuse("arbitrary text"), + "WARN: Test_MkLexer_VarUse.mk:1: Invalid part \" text\" after variable name \"arbitrary\".") +} + +func (s *Suite) Test_MkLexer_VarUse__ambiguous(c *check.C) { + t := s.Init(c) + b := NewMkTokenBuilder() + + t.SetUpCommandLine("--explain") + + line := t.NewLine("module.mk", 123, "\t$Varname $X") + p := NewMkLexer(line.Text[1:], line) + + tokens, rest := p.MkTokens() + t.CheckDeepEquals(tokens, b.Tokens( + b.VaruseTextToken("$V", "V"), + b.TextToken("arname "), + b.VaruseTextToken("$X", "X"))) + t.CheckEquals(rest, "") + + t.CheckOutputLines( + "ERROR: module.mk:123: $Varname is ambiguous. Use ${Varname} if you mean a Make variable or $$Varname if you mean a shell variable.", + "", + "\tOnly the first letter after the dollar is the variable name.", + "\tEverything following it is normal text, even if it looks like a", + "\tvariable name to human readers.", + "", + "WARN: module.mk:123: $X is ambiguous. Use ${X} if you mean a Make variable or $$X if you mean a shell variable.", + "", + "\tIn its current form, this variable is parsed as a Make variable. For", + "\thuman readers though, $x looks more like a shell variable than a", + "\tMake variable, since Make variables are usually written using braces", + "\t(BSD-style) or parentheses (GNU-style).", + "") +} + +// Pkglint can replace $(VAR) with ${VAR}. It doesn't look at all components +// of nested variables though because this case is not important enough to +// invest much development time. It occurs so seldom that it is acceptable +// to run pkglint multiple times in such a case. +func (s *Suite) Test_MkLexer_varUseBrace__autofix_parentheses(c *check.C) { + t := s.Init(c) + + test := func() { + mklines := t.SetUpFileMkLines("Makefile", + MkCvsID, + "COMMENT=\t$(P1) $(P2)) $(P3:Q) ${BRACES} $(A.$(B.$(C))) $(A:M\\#)", + "P1=\t\t${COMMENT}", + "P2=\t\t# nothing", + "P3=\t\t# nothing", + "BRACES=\t\t# nothing", + "C=\t\t# nothing", + "A=\t\t# nothing") + + mklines.Check() + } + + t.ExpectDiagnosticsAutofix( + test, + + "WARN: ~/Makefile:2: Please use curly braces {} instead of round parentheses () for P1.", + "WARN: ~/Makefile:2: Please use curly braces {} instead of round parentheses () for P2.", + "WARN: ~/Makefile:2: Please use curly braces {} instead of round parentheses () for P3.", + "WARN: ~/Makefile:2: Please use curly braces {} instead of round parentheses () for C.", + "WARN: ~/Makefile:2: Please use curly braces {} instead of round parentheses () for B.$(C).", + "WARN: ~/Makefile:2: Please use curly braces {} instead of round parentheses () for A.$(B.$(C)).", + "WARN: ~/Makefile:2: Please use curly braces {} instead of round parentheses () for A.", + "AUTOFIX: ~/Makefile:2: Replacing \"$(P1)\" with \"${P1}\".", + "AUTOFIX: ~/Makefile:2: Replacing \"$(P2)\" with \"${P2}\".", + "AUTOFIX: ~/Makefile:2: Replacing \"$(P3:Q)\" with \"${P3:Q}\".", + "AUTOFIX: ~/Makefile:2: Replacing \"$(C)\" with \"${C}\".") +} + +func (s *Suite) Test_MkLexer_Varname(c *check.C) { + t := s.Init(c) + + test := func(text string) { + line := t.NewLine("filename.mk", 1, text) + p := NewMkLexer(text, line) + + varname := p.Varname() + + t.CheckEquals(varname, text) + t.CheckEquals(p.Rest(), "") + } + + testRest := func(text string, expectedVarname string, expectedRest string) { + line := t.NewLine("filename.mk", 1, text) + p := NewMkLexer(text, line) + + varname := p.Varname() + + t.CheckEquals(varname, expectedVarname) + t.CheckEquals(p.Rest(), expectedRest) + } + + test("VARNAME") + test("VARNAME.param") + test("VARNAME.${param}") + test("SITES_${param}") + test("SITES_distfile-1.0.tar.gz") + test("SITES.gtk+-2.0") + test("PKGPATH.category/package") + + testRest("VARNAME/rest", "VARNAME", "/rest") +} + +func (s *Suite) Test_MkLexer_VarUseModifiers(c *check.C) { + t := s.Init(c) + + varUse := NewMkTokenBuilder().VarUse + test := func(text string, varUse *MkVarUse, diagnostics ...string) { + line := t.NewLine("Makefile", 20, "\t"+text) + p := NewMkLexer(text, line) + + actual := p.VarUse() + + t.CheckDeepEquals(actual, varUse) + t.CheckEquals(p.Rest(), "") + t.CheckOutput(diagnostics) + } + + // The !command! modifier is used so seldom that pkglint does not + // check whether the command is actually valid. + // At least not while parsing the modifier since at this point it might + // be still unknown which of the commands can be used and which cannot. + test("${VAR:!command!}", varUse("VAR", "!command!")) + + test("${VAR:!command}", varUse("VAR"), + "WARN: Makefile:20: Invalid variable modifier \"!command\" for \"VAR\".") + + test("${VAR:command!}", varUse("VAR"), + "WARN: Makefile:20: Invalid variable modifier \"command!\" for \"VAR\".") + + // The :L modifier makes the variable value "echo hello", and the :[1] + // modifier extracts the "echo". + test("${echo hello:L:[1]}", varUse("echo hello", "L", "[1]")) + + // bmake ignores the :[3] modifier, and the :L modifier just returns the + // variable name, in this case BUILD_DIRS. + test("${BUILD_DIRS:[3]:L}", varUse("BUILD_DIRS", "[3]", "L")) + + test("${PATH:ts::Q}", varUse("PATH", "ts:", "Q")) +} + +func (s *Suite) Test_MkLexer_varUseModifier__invalid_ts_modifier_with_warning(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("-Wall", "--explain") + line := t.NewLine("filename.mk", 123, "${VAR:tsabc}") + p := NewMkLexer("tsabc}", line) + + modifier := p.varUseModifier("VAR", '}') + + t.CheckEquals(modifier, "tsabc") + t.CheckEquals(p.Rest(), "}") + t.CheckOutputLines( + "WARN: filename.mk:123: Invalid separator \"abc\" for :ts modifier of \"VAR\".", + "", + "\tThe separator for the :ts modifier must be either a single character", + "\tor an escape sequence like \\t or \\n or an octal or decimal escape", + "\tsequence; see the bmake man page for further details.", + "") +} + +func (s *Suite) Test_MkLexer_varUseModifier__invalid_ts_modifier_without_warning(c *check.C) { + t := s.Init(c) + + p := NewMkLexer("tsabc}", nil) + + modifier := p.varUseModifier("VAR", '}') + + t.CheckEquals(modifier, "tsabc") + t.CheckEquals(p.Rest(), "}") +} + +func (s *Suite) Test_MkLexer_varUseModifier__square_bracket(c *check.C) { + t := s.Init(c) + + line := t.NewLine("filename.mk", 123, "\t${VAR:[asdf]}") + p := NewMkLexer("[asdf]", line) + + modifier := p.varUseModifier("VAR", '}') + + t.CheckEquals(modifier, "") + t.CheckEquals(p.Rest(), "") + + t.CheckOutputLines( + "WARN: filename.mk:123: Invalid variable modifier \"[asdf]\" for \"VAR\".") +} + +func (s *Suite) Test_MkLexer_varUseModifier__condition_without_colon(c *check.C) { + t := s.Init(c) + b := NewMkTokenBuilder() + + line := t.NewLine("filename.mk", 123, "${${VAR}:?yes:no}${${VAR}:?yes}") + p := NewMkLexer(line.Text, line) + + varUse1 := p.VarUse() + varUse2 := p.VarUse() + + t.CheckDeepEquals(varUse1, b.VarUse("${VAR}", "?yes:no")) + t.CheckDeepEquals(varUse2, b.VarUse("${VAR}")) + t.CheckEquals(p.Rest(), "") + + t.CheckOutputLines( + "WARN: filename.mk:123: Invalid variable modifier \"?yes\" for \"${VAR}\".") +} + +func (s *Suite) Test_MkLexer_varUseModifier__malformed_in_parentheses(c *check.C) { + t := s.Init(c) + b := NewMkTokenBuilder() + + line := t.NewLine("filename.mk", 123, "$(${VAR}:?yes)") + p := NewMkLexer(line.Text, line) + + varUse := p.VarUse() + + t.CheckDeepEquals(varUse, b.VarUse("${VAR}")) + t.CheckEquals(p.Rest(), "") + + t.CheckOutputLines( + "WARN: filename.mk:123: Invalid variable modifier \"?yes\" for \"${VAR}\".", + "WARN: filename.mk:123: Please use curly braces {} instead of round parentheses () for ${VAR}.") +} + +func (s *Suite) Test_MkLexer_varUseModifier__varuse_in_malformed_modifier(c *check.C) { + t := s.Init(c) + b := NewMkTokenBuilder() + + line := t.NewLine("filename.mk", 123, "${${VAR}:?yes${INNER}}") + p := NewMkLexer(line.Text, line) + + varUse := p.VarUse() + + t.CheckDeepEquals(varUse, b.VarUse("${VAR}")) + t.CheckEquals(p.Rest(), "") + + t.CheckOutputLines( + "WARN: filename.mk:123: Invalid variable modifier \"?yes${INNER}\" for \"${VAR}\".") +} + +func (s *Suite) Test_MkLexer_varUseModifierSubst(c *check.C) { + t := s.Init(c) + + varUse := NewMkTokenBuilder().VarUse + test := func(text string, varUse *MkVarUse, rest string, diagnostics ...string) { + line := t.NewLine("Makefile", 20, "\t"+text) + p := NewMkLexer(text, line) + + actual := p.VarUse() + + t.CheckDeepEquals(actual, varUse) + t.CheckEquals(p.Rest(), rest) + t.CheckOutput(diagnostics) + } + + test("${VAR:S", varUse("VAR"), "", + "WARN: Makefile:20: Invalid variable modifier \"S\" for \"VAR\".", + "WARN: Makefile:20: Missing closing \"}\" for \"VAR\".") + + test("${VAR:S}", varUse("VAR"), "", + "WARN: Makefile:20: Invalid variable modifier \"S\" for \"VAR\".") + + test("${VAR:S,}", varUse("VAR"), "", + "WARN: Makefile:20: Invalid variable modifier \"S,\" for \"VAR\".") + + test("${VAR:S,from,to}", varUse("VAR"), "", + "WARN: Makefile:20: Invalid variable modifier \"S,from,to\" for \"VAR\".") + + test("${VAR:S,from,to,}", varUse("VAR", "S,from,to,"), "") + + test("${VAR:S,^from$,to,}", varUse("VAR", "S,^from$,to,"), "") + + test("${VAR:S,@F@,${F},}", varUse("VAR", "S,@F@,${F},"), "") + + test("${VAR:S,from,to,1}", varUse("VAR", "S,from,to,1"), "") + test("${VAR:S,from,to,g}", varUse("VAR", "S,from,to,g"), "") + test("${VAR:S,from,to,W}", varUse("VAR", "S,from,to,W"), "") + + test("${VAR:S,from,to,1gW}", varUse("VAR", "S,from,to,1gW"), "") + + // Inside the :S or :C modifiers, neither a colon nor the closing + // brace need to be escaped. Otherwise these patterns would become + // too difficult to read and write. + test("${VAR:C/[[:alnum:]]{2}/**/g}", + varUse("VAR", "C/[[:alnum:]]{2}/**/g"), + "") + + // Some pkgsrc users really explore the darkest corners of bmake by using + // the backslash as the separator in the :S modifier. Sure, it works, it + // just looks totally unexpected to the average pkgsrc reader. + // + // Using the backslash as separator means that it cannot be used for anything + // else, not even for escaping other characters. + test("${VAR:S\\.post1\\\\1}", + varUse("VAR", "S\\.post1\\\\1"), + "") +} + +func (s *Suite) Test_MkLexer_varUseModifierAt__missing_at_after_variable_name(c *check.C) { + t := s.Init(c) + b := NewMkTokenBuilder() + + line := t.NewLine("filename.mk", 123, "${VAR:@varname}") + p := NewMkLexer(line.Text, line) + + varUse := p.VarUse() + + t.CheckDeepEquals(varUse, b.VarUse("VAR")) + t.CheckEquals(p.Rest(), "") + t.CheckOutputLines( + "WARN: filename.mk:123: Invalid variable modifier \"@varname\" for \"VAR\".") +} + +func (s *Suite) Test_MkLexer_varUseModifierAt__dollar(c *check.C) { + t := s.Init(c) + b := NewMkTokenBuilder() + + line := t.NewLine("filename.mk", 123, "${VAR:@var@$$var@}") + p := NewMkLexer(line.Text, line) + + varUse := p.VarUse() + + t.CheckDeepEquals(varUse, b.VarUse("VAR", "@var@$$var@")) + t.CheckEquals(p.Rest(), "") + t.CheckOutputEmpty() +} + +func (s *Suite) Test_MkLexer_varUseModifierAt__incomplete_without_warning(c *check.C) { + t := s.Init(c) + b := NewMkTokenBuilder() + + p := NewMkLexer("${VAR:@var@$$var}rest", nil) + + varUse := p.VarUse() + + t.CheckDeepEquals(varUse, b.VarUse("VAR", "@var@$$var}rest")) + t.CheckEquals(p.Rest(), "") + t.CheckOutputEmpty() +} + +func (s *Suite) Test_MkLexer_varUseModifierAt(c *check.C) { + t := s.Init(c) + + varUse := NewMkTokenBuilder().VarUse + test := func(text string, varUse *MkVarUse, rest string, diagnostics ...string) { + line := t.NewLine("Makefile", 20, "\t"+text) + p := NewMkLexer(text, line) + + actual := p.VarUse() + + t.CheckDeepEquals(actual, varUse) + t.CheckEquals(p.Rest(), rest) + t.CheckOutput(diagnostics) + } + + test("${VAR:@", + varUse("VAR"), + "", + "WARN: Makefile:20: Invalid variable modifier \"@\" for \"VAR\".", + "WARN: Makefile:20: Missing closing \"}\" for \"VAR\".") + + test("${VAR:@i@${i}}", varUse("VAR", "@i@${i}}"), "", + "WARN: Makefile:20: Modifier ${VAR:@i@...@} is missing the final \"@\".", + "WARN: Makefile:20: Missing closing \"}\" for \"VAR\".") + + test("${VAR:@i@${i}@}", varUse("VAR", "@i@${i}@"), "") + + test("${PKG_GROUPS:@g@${g:Q}:${PKG_GID.${g}:Q}@:C/:*$//g}", + varUse("PKG_GROUPS", "@g@${g:Q}:${PKG_GID.${g}:Q}@", "C/:*$//g"), + "") +} diff --git a/pkgtools/pkglint/files/mkline.go b/pkgtools/pkglint/files/mkline.go index bad8e5c106a..c5ff8e6cd29 100644 --- a/pkgtools/pkglint/files/mkline.go +++ b/pkgtools/pkglint/files/mkline.go @@ -330,9 +330,8 @@ func (mkline *MkLine) Tokenize(text string, warn bool) []*MkToken { if warn { line = mkline.Line } - p := NewMkParser(line, text) - tokens = p.MkTokens() - rest = p.Rest() + p := NewMkLexer(text, line) + tokens, rest = p.MkTokens() } if warn && rest != "" { @@ -502,9 +501,8 @@ func (mkline *MkLine) ValueTokens() ([]*MkToken, string) { // No error checking here since all this has already been done when the // whole line was parsed in MkLineParser.Parse. - p := NewMkParser(nil, value) - assign.valueMk = p.MkTokens() - assign.valueMkRest = p.Rest() + p := NewMkLexer(value, nil) + assign.valueMk, assign.valueMkRest = p.MkTokens() return assign.valueMk, assign.valueMkRest } @@ -545,7 +543,8 @@ func (mkline *MkLine) Fields() []string { func (*MkLine) WithoutMakeVariables(value string) string { var valueNovar strings.Builder - for _, token := range NewMkParser(nil, value).MkTokens() { + tokens, _ := NewMkLexer(value, nil).MkTokens() + for _, token := range tokens { if token.Varuse == nil { valueNovar.WriteString(token.Text) } @@ -567,7 +566,7 @@ func (mkline *MkLine) ResolveVarsInRelativePath(relativePath Path) Path { tmp := relativePath if tmp.ContainsText("PKGSRCDIR") { - pkgsrcdir := relpath(basedir, G.Pkgsrc.File(".")) + pkgsrcdir := G.Pkgsrc.Relpath(basedir, G.Pkgsrc.File(".")) if G.Testing { // Relative pkgsrc paths usually only contain two or three levels. @@ -789,7 +788,8 @@ func (mkline *MkLine) ForEachUsed(action func(varUse *MkVarUse, time VucTime)) { return } - for _, token := range NewMkParser(nil, text).MkTokens() { + tokens, _ := NewMkLexer(text, nil).MkTokens() + for _, token := range tokens { if token.Varuse != nil { searchInVarUse(token.Varuse, time) } diff --git a/pkgtools/pkglint/files/mkline_test.go b/pkgtools/pkglint/files/mkline_test.go index b913f7ed26b..7f9c2a7a8fd 100644 --- a/pkgtools/pkglint/files/mkline_test.go +++ b/pkgtools/pkglint/files/mkline_test.go @@ -282,7 +282,9 @@ func (s *Suite) Test_MkLine_ValueFields__compared_to_splitIntoShellTokens(c *che url := "http://registry.gimp.org/file/fix-ca.c?action=download&id=9884&file=" mkline := t.NewMkLine("filename.mk", 123, "MASTER_SITES=\t"+url) - words, rest := splitIntoShellTokens(dummyLine, url) // Doesn't really make sense + // Splitting a URL into shell tokens doesn't really make sense, + // but is used here anyway. + words, rest := splitIntoShellTokens(mkline.Line, url) t.CheckDeepEquals(words, []string{ "http://registry.gimp.org/file/fix-ca.c?action=download", diff --git a/pkgtools/pkglint/files/mklinechecker.go b/pkgtools/pkglint/files/mklinechecker.go index 1243658f342..0a6152d7c6f 100644 --- a/pkgtools/pkglint/files/mklinechecker.go +++ b/pkgtools/pkglint/files/mklinechecker.go @@ -387,7 +387,7 @@ func (ck MkLineChecker) checkTextVarUse(text string, vartype *Vartype, time VucT defer trace.Call(vartype, time)() } - tokens := NewMkParser(nil, text).MkTokens() + tokens, _ := NewMkLexer(text, nil).MkTokens() for i, token := range tokens { if token.Varuse != nil { spaceLeft := i-1 < 0 || matches(tokens[i-1].Text, `[\t ]$`) @@ -814,7 +814,6 @@ func (ck MkLineChecker) checkVarUseQuoting(varUse *MkVarUse, vartype *Vartype, v fix.Explain( seeGuide("Echoing a string exactly as-is", "echo-literal")) fix.Replace("${"+varname+mod+"}", "${"+varname+correctMod+"}") - fix.Anyway() fix.Apply() } else { mkline.Warnf("Please use ${%s%s} instead of ${%s%s} and make sure"+ @@ -1103,7 +1102,6 @@ func (ck MkLineChecker) checkVarassignRightCategory() { if len(categories) > 1 && categories[1] == expected { fix.Replace(categories[0]+" "+categories[1], categories[1]+" "+categories[0]) } - fix.Anyway() fix.Apply() } @@ -1341,7 +1339,7 @@ func (ck MkLineChecker) checkInclude() { case includedFile.HasSuffixPath("intltool/buildlink3.mk"): mkline.Warnf("Please write \"USE_TOOLS+= intltool\" instead of this line.") - case includedFile.HasSuffixText("/builtin.mk"): // TODO: maybe HasSuffixPath + case includedFile != "builtin.mk" && includedFile.HasSuffixPath("builtin.mk"): if mkline.Basename != "hacks.mk" && !mkline.HasRationale() { fix := mkline.Autofix() fix.Errorf("%s must not be included directly. Include \"%s/buildlink3.mk\" instead.", includedFile, includedFile.Dir()) @@ -1671,7 +1669,6 @@ func (ck MkLineChecker) simplifyCondition(varuse *MkVarUse, fromEmpty bool, notE "An entirely different case is when the pattern contains wildcards like ^, *, $.", "In such a case, using the :M or :N modifiers is useful and preferred.") fix.Replace(replace(varname, positive, pattern)) - fix.Anyway() fix.Apply() } } @@ -1741,7 +1738,6 @@ func (ck MkLineChecker) checkCompareVarStrCompiler(op string, value string) { "Therefore, comparing it using == or != leads to wrong results in these cases.") fix.Replace("${PKGSRC_COMPILER} "+op+" "+value, "${PKGSRC_COMPILER:"+matchOp+value+"}") fix.Replace("${PKGSRC_COMPILER} "+op+" \""+value+"\"", "${PKGSRC_COMPILER:"+matchOp+value+"}") - fix.Anyway() fix.Apply() } @@ -1807,10 +1803,10 @@ func (ck MkLineChecker) checkDependencyRule(allowedTargets map[string]bool) { } func (ck MkLineChecker) checkDependencyTarget(target string, allowedTargets map[string]bool) { - if target == ".PHONY" || - target == ".ORDER" || - NewMkParser(nil, target).VarUse() != nil || - allowedTargets[target] { + if target == ".PHONY" || target == ".ORDER" || allowedTargets[target] { + return + } + if NewMkLexer(target, nil).VarUse() != nil { return } diff --git a/pkgtools/pkglint/files/mklinechecker_test.go b/pkgtools/pkglint/files/mklinechecker_test.go index 28382422a75..43c123a3727 100644 --- a/pkgtools/pkglint/files/mklinechecker_test.go +++ b/pkgtools/pkglint/files/mklinechecker_test.go @@ -57,7 +57,7 @@ func (s *Suite) Test_MkLineChecker_Check__buildlink3_include_prefs(c *check.C) { // 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. + // TODO: Refactor Relpath to be independent of a filesystem. mklines.Check() @@ -102,7 +102,9 @@ func (s *Suite) Test_MkLineChecker_Check__varuse_modifier_L(c *check.C) { t.CheckOutputLines( "WARN: x11/xkeyboard-config/Makefile:3: "+ "Invalid part \"/xkbcomp\" after variable name \"${XKBBASE}\".", - // TODO: Avoid this duplicate diagnostic. + // TODO: Avoid these duplicate diagnostics. + "WARN: x11/xkeyboard-config/Makefile:3: "+ + "Invalid part \"/xkbcomp\" after variable name \"${XKBBASE}\".", "WARN: x11/xkeyboard-config/Makefile:3: "+ "Invalid part \"/xkbcomp\" after variable name \"${XKBBASE}\".", "WARN: x11/xkeyboard-config/Makefile:3: XKBBASE is used but not defined.") @@ -125,6 +127,7 @@ func (s *Suite) Test_MkLineChecker_checkEmptyContinuation(c *check.C) { mklines.Check() t.CheckOutputLines( + "NOTE: ~/filename.mk:2--3: Trailing whitespace.", "WARN: ~/filename.mk:3: This line looks empty but continues the previous line.") } @@ -2240,6 +2243,22 @@ func (s *Suite) Test_MkLineChecker_checkInclude__builtin_mk(c *check.C) { "Include \"../../category/package/buildlink3.mk\" instead.") } +func (s *Suite) Test_MkLineChecker_checkInclude__buildlink3_mk_includes_builtin_mk(c *check.C) { + t := s.Init(c) + + t.SetUpPkgsrc() + mklines := t.SetUpFileMkLines("category/package/buildlink3.mk", + MkCvsID, + ".include \"builtin.mk\"") + t.CreateFileLines("category/package/builtin.mk", + MkCvsID) + t.FinishSetUp() + + mklines.Check() + + t.CheckOutputEmpty() +} + func (s *Suite) Test_MkLineChecker_checkInclude__builtin_mk_rationale(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/mklineparser.go b/pkgtools/pkglint/files/mklineparser.go index 94c911550f6..92a7b13faa1 100644 --- a/pkgtools/pkglint/files/mklineparser.go +++ b/pkgtools/pkglint/files/mklineparser.go @@ -85,6 +85,7 @@ func (p MkLineParser) parseVarassign(line *Line, text string, splitResult mkLine fix.Notef("Unnecessary space after variable name %q.", varname) fix.Replace(varname+a.spaceAfterVarname+op.String(), varname+op.String()) fix.Apply() + // FIXME: Preserve the alignment of the variable value. } } @@ -299,7 +300,7 @@ func (MkLineParser) split(line *Line, text string, trimComment bool) mkLineSplit mainWithSpaces = text } - parser := NewMkParser(line, mainWithSpaces) + parser := NewMkLexer(mainWithSpaces, line) lexer := parser.lexer parseOther := func() string { diff --git a/pkgtools/pkglint/files/mklines.go b/pkgtools/pkglint/files/mklines.go index ad518ff4cdc..4a33353c879 100644 --- a/pkgtools/pkglint/files/mklines.go +++ b/pkgtools/pkglint/files/mklines.go @@ -167,7 +167,7 @@ func (mklines *MkLines) collectDocumentedVariables() { commentLines++ - parser := NewMkParser(nil, words[1]) + parser := NewMkLexer(words[1], nil) varname := parser.Varname() if len(varname) < 3 { break diff --git a/pkgtools/pkglint/files/mkparser.go b/pkgtools/pkglint/files/mkparser.go index 4305248f094..4d46a7a9b7f 100644 --- a/pkgtools/pkglint/files/mkparser.go +++ b/pkgtools/pkglint/files/mkparser.go @@ -1,7 +1,6 @@ package pkglint import ( - "netbsd.org/pkglint/regex" "netbsd.org/pkglint/textproc" "strings" ) @@ -10,6 +9,7 @@ import ( // things related to Makefiles. type MkParser struct { Line *Line + mklex *MkLexer lexer *textproc.Lexer EmitWarnings bool } @@ -31,387 +31,13 @@ func (p *MkParser) Rest() string { // which means the # is a normal character and does not introduce a Makefile comment. // For VarUse, this distinction is irrelevant. func NewMkParser(line *Line, text string) *MkParser { - return &MkParser{line, textproc.NewLexer(text), line != nil} -} - -// MkTokens splits a text like in the following example: -// Text${VAR:Mmodifier}${VAR2}more text${VAR3} -// into tokens like these: -// Text -// ${VAR:Mmodifier} -// ${VAR2} -// more text -// ${VAR3} -func (p *MkParser) MkTokens() []*MkToken { - lexer := p.lexer - - var tokens []*MkToken - for !p.EOF() { - mark := lexer.Mark() - if varuse := p.VarUse(); varuse != nil { - tokens = append(tokens, &MkToken{Text: lexer.Since(mark), Varuse: varuse}) - continue - } - - for lexer.NextBytesFunc(func(b byte) bool { return b != '$' }) != "" || lexer.SkipString("$$") { - } - text := lexer.Since(mark) - if text != "" { - tokens = append(tokens, &MkToken{Text: text}) - continue - } - - break - } - return tokens -} - -func (p *MkParser) VarUse() *MkVarUse { - rest := p.lexer.Rest() - if len(rest) < 2 || rest[0] != '$' { - return nil - } - - switch rest[1] { - case '{', '(': - return p.varUseBrace(rest[1] == '(') - - case '$': - // This is an escaped dollar character and not a variable use. - return nil - - case '@', '<', ' ': - // These variable names are known to exist. - // - // Many others are also possible but not used in practice. - // In particular, when parsing the :C or :S modifier, - // the $ must not be interpreted as a variable name, - // even when it looks like $/ could refer to the "/" variable. - // - // TODO: Find out whether $" is a variable use when it appears in the :M modifier. - p.lexer.Skip(2) - return &MkVarUse{rest[1:2], nil} - - default: - return p.varUseAlnum() - } -} - -// varUseBrace parses: -// ${VAR} -// ${arbitrary text:L} -// ${variable with invalid chars} -// $(PARENTHESES) -// ${VAR:Mpattern:C,:,colon,g:Q:Q:Q} -func (p *MkParser) varUseBrace(usingRoundParen bool) *MkVarUse { - lexer := p.lexer - - beforeDollar := lexer.Mark() - lexer.Skip(2) - - closing := byte('}') - if usingRoundParen { - closing = ')' - } - - beforeVarname := lexer.Mark() - varname := p.Varname() - p.varUseText(closing) - varExpr := lexer.Since(beforeVarname) - - modifiers := p.VarUseModifiers(varExpr, closing) - - closed := lexer.SkipByte(closing) - - if p.EmitWarnings { - if !closed { - p.Line.Warnf("Missing closing %q for %q.", string(rune(closing)), varExpr) - } - - if usingRoundParen && closed { - parenVaruse := lexer.Since(beforeDollar) - edit := []byte(parenVaruse) - edit[1] = '{' - edit[len(edit)-1] = '}' - bracesVaruse := string(edit) - - fix := p.Line.Autofix() - fix.Warnf("Please use curly braces {} instead of round parentheses () for %s.", varExpr) - fix.Replace(parenVaruse, bracesVaruse) - fix.Apply() - } - - if len(varExpr) > len(varname) && !(&MkVarUse{varExpr, modifiers}).IsExpression() { - p.Line.Warnf("Invalid part %q after variable name %q.", varExpr[len(varname):], varname) - } - } - - return &MkVarUse{varExpr, modifiers} -} - -func (p *MkParser) varUseAlnum() *MkVarUse { - lexer := p.lexer - - apparentVarname := textproc.NewLexer(lexer.Rest()[1:]).NextBytesSet(textproc.AlnumU) - if apparentVarname == "" { - return nil - } - - lexer.Skip(2) - - if p.EmitWarnings { - if len(apparentVarname) > 1 { - p.Line.Errorf("$%[1]s is ambiguous. Use ${%[1]s} if you mean a Make variable or $$%[1]s if you mean a shell variable.", - apparentVarname) - p.Line.Explain( - "Only the first letter after the dollar is the variable name.", - "Everything following it is normal text, even if it looks like a variable name to human readers.") - } else { - p.Line.Warnf("$%[1]s is ambiguous. Use ${%[1]s} if you mean a Make variable or $$%[1]s if you mean a shell variable.", apparentVarname) - p.Line.Explain( - "In its current form, this variable is parsed as a Make variable.", - "For human readers though, $x looks more like a shell variable than a Make variable,", - "since Make variables are usually written using braces (BSD-style) or parentheses (GNU-style).") - } - } - - return &MkVarUse{apparentVarname[:1], nil} -} - -// VarUseModifiers parses the modifiers of a variable being used, such as :Q, :Mpattern. -// -// See the bmake manual page. -func (p *MkParser) VarUseModifiers(varname string, closing byte) []MkVarUseModifier { - lexer := p.lexer - - var modifiers []MkVarUseModifier - // The :S and :C modifiers may be chained without using the : as separator. - mayOmitColon := false - - for lexer.SkipByte(':') || mayOmitColon { - modifier := p.varUseModifier(varname, closing) - if modifier != "" { - modifiers = append(modifiers, MkVarUseModifier{modifier}) - } - mayOmitColon = modifier != "" && (modifier[0] == 'S' || modifier[0] == 'C') - } - return modifiers -} - -// varUseModifier parses a single variable modifier such as :Q or :S,from,to,. -// The actual parsing starts after the leading colon. -func (p *MkParser) varUseModifier(varname string, closing byte) string { - lexer := p.lexer - mark := lexer.Mark() - - switch lexer.PeekByte() { - case 'E', 'H', 'L', 'O', 'Q', 'R', 'T', 's', 't', 'u': - mod := lexer.NextBytesSet(textproc.Alnum) - - switch mod { - case - "E", // Extension, e.g. path/file.suffix => suffix - "H", // Head, e.g. dir/subdir/file.suffix => dir/subdir - "L", // XXX: Shouldn't this be handled specially? - "O", // Order alphabetically - "Ox", // Shuffle - "Q", // Quote shell meta-characters - "R", // Strip the file suffix, e.g. path/file.suffix => file - "T", // Basename, e.g. path/file.suffix => file.suffix - "sh", // Evaluate the variable value as shell command - "tA", // Try to convert to absolute path - "tW", // Causes the value to be treated as a single word - "tl", // To lowercase - "tu", // To uppercase - "tw", // Causes the value to be treated as list of words - "u": // Remove adjacent duplicate words (like uniq(1)) - return mod - } - - if hasPrefix(mod, "ts") { - // See devel/bmake/files/var.c:/case 't' - sep := mod[2:] + p.varUseText(closing) - switch { - case sep == "": - lexer.SkipString(":") - case len(sep) == 1: - break - case matches(sep, `^\\\d+`): - break - default: - if p.EmitWarnings { - p.Line.Warnf("Invalid separator %q for :ts modifier of %q.", sep, varname) - p.Line.Explain( - "The separator for the :ts modifier must be either a single character", - "or an escape sequence like \\t or \\n or an octal or decimal escape", - "sequence; see the bmake man page for further details.") - } - } - return lexer.Since(mark) - } - - case '=', 'D', 'M', 'N', 'U': - lexer.Skip(1) - re := regcomp(regex.Pattern(condStr(closing == '}', `^([^$:\\}]|\$\$|\\.)+`, `^([^$:\\)]|\$\$|\\.)+`))) - for p.VarUse() != nil || lexer.SkipRegexp(re) { - } - arg := lexer.Since(mark) - return strings.Replace(arg, "\\:", ":", -1) - - case 'C', 'S': - if ok, _, _, _, _ := p.varUseModifierSubst(closing); ok { - return lexer.Since(mark) - } - - case '@': - if p.varUseModifierAt(lexer, varname) { - return lexer.Since(mark) - } - - case '[': - if lexer.SkipRegexp(regcomp(`^\[(?:[-.\d]+|#)\]`)) { - return lexer.Since(mark) - } - - case '?': - lexer.Skip(1) - p.varUseText(closing) - if lexer.SkipByte(':') { - p.varUseText(closing) - return lexer.Since(mark) - } - } - - lexer.Reset(mark) - - re := regcomp(regex.Pattern(condStr(closing == '}', `^([^:$}]|\$\$)+`, `^([^:$)]|\$\$)+`))) - for p.VarUse() != nil || lexer.SkipRegexp(re) { - } - modifier := lexer.Since(mark) - - // ${SOURCES:%.c=%.o} or ${:!uname -a!:[2]} - if contains(modifier, "=") || (hasPrefix(modifier, "!") && hasSuffix(modifier, "!")) { - return modifier - } - - if p.EmitWarnings && modifier != "" { - p.Line.Warnf("Invalid variable modifier %q for %q.", modifier, varname) - } - - return "" -} - -// varUseText parses any text up to the next colon or closing mark. -// Nested variable uses are parsed as well. -// -// This is used for the :L and :? modifiers since they accept arbitrary -// text as the "variable name" and effectively interpret it as the variable -// value instead. -func (p *MkParser) varUseText(closing byte) string { - lexer := p.lexer - start := lexer.Mark() - re := regcomp(regex.Pattern(condStr(closing == '}', `^([^$:}]|\$\$)+`, `^([^$:)]|\$\$)+`))) - for p.VarUse() != nil || lexer.SkipRegexp(re) { - } - return lexer.Since(start) -} - -// varUseModifierSubst parses a :S,from,to, or a :C,from,to, modifier. -func (p *MkParser) varUseModifierSubst(closing byte) (ok bool, regex bool, from string, to string, options string) { - lexer := p.lexer - regex = lexer.PeekByte() == 'C' - lexer.Skip(1 /* the initial S or C */) - - sep := lexer.PeekByte() // bmake allows _any_ separator, even letters. - if sep == -1 || byte(sep) == closing { - return - } - - lexer.Skip(1) - separator := byte(sep) - - unescape := func(s string) string { - return strings.Replace(s, "\\"+string(separator), string(separator), -1) - } - - isOther := func(b byte) bool { - return b != separator && b != '$' && b != '\\' - } - - skipOther := func() { - for { - switch { - - case p.VarUse() != nil: - break - - case lexer.SkipString("$$"): - break - - case len(lexer.Rest()) >= 2 && lexer.PeekByte() == '\\' && separator != '\\': - _ = lexer.Skip(2) - - case lexer.NextBytesFunc(isOther) != "": - break - - default: - return - } - } - } - - fromStart := lexer.Mark() - lexer.SkipByte('^') - skipOther() - lexer.SkipByte('$') - from = unescape(lexer.Since(fromStart)) - - if !lexer.SkipByte(separator) { - return - } - - toStart := lexer.Mark() - skipOther() - to = unescape(lexer.Since(toStart)) - - if !lexer.SkipByte(separator) { - return - } - - optionsStart := lexer.Mark() - lexer.NextBytesFunc(func(b byte) bool { return b == '1' || b == 'g' || b == 'W' }) - options = lexer.Since(optionsStart) - - ok = true - return -} - -// varUseModifierAt parses a variable modifier like ":@v@echo ${v};@", -// which expands the variable value in a loop. -func (p *MkParser) varUseModifierAt(lexer *textproc.Lexer, varname string) bool { - lexer.Skip(1 /* the initial @ */) - - loopVar := lexer.NextBytesSet(AlnumDot) - if loopVar == "" || !lexer.SkipByte('@') { - return false - } - - re := regcomp(`^([^$@\\]|\\.)+`) - for p.VarUse() != nil || lexer.SkipString("$$") || lexer.SkipRegexp(re) { - } - - if !lexer.SkipByte('@') && p.EmitWarnings { - p.Line.Warnf("Modifier ${%s:@%s@...@} is missing the final \"@\".", varname, loopVar) - } - - return true + mklex := NewMkLexer(text, line) + return &MkParser{line, mklex, mklex.lexer, line != nil} } // MkCond parses a condition like ${OPSYS} == "NetBSD". // // See devel/bmake/files/cond.c. -// -// FIXME: Move over to MkTokensParser func (p *MkParser) MkCond() *MkCond { and := p.mkCondAnd() if and == nil { @@ -556,7 +182,7 @@ func (p *MkParser) mkCondCompare() *MkCond { func (p *MkParser) mkCondTerm() *MkCondTerm { lexer := p.lexer - if rhs := p.VarUse(); rhs != nil { + if rhs := p.mklex.VarUse(); rhs != nil { return &MkCondTerm{Var: rhs} } @@ -566,7 +192,7 @@ func (p *MkParser) mkCondTerm() *MkCondTerm { mark := lexer.Mark() lexer.Skip(1) - if quotedRHS := p.VarUse(); quotedRHS != nil { + if quotedRHS := p.mklex.VarUse(); quotedRHS != nil { if lexer.SkipByte('"') { return &MkCondTerm{Var: quotedRHS} } @@ -579,7 +205,7 @@ loop: for { m := lexer.Mark() switch { - case p.VarUse() != nil, + case p.mklex.VarUse() != nil, lexer.NextBytesSet(textproc.Alnum) != "", lexer.NextBytesFunc(func(b byte) bool { return b != '"' && b != '\\' }) != "": rhsText.WriteString(lexer.Since(m)) @@ -612,14 +238,14 @@ func (p *MkParser) mkCondFunc() *MkCond { switch funcName { case "defined": - varname := p.Varname() + varname := p.mklex.Varname() if varname != "" && lexer.SkipByte(')') { return &MkCond{Defined: varname} } case "empty": - if varname := p.Varname(); varname != "" { - modifiers := p.VarUseModifiers(varname, ')') + if varname := p.mklex.Varname(); varname != "" { + modifiers := p.mklex.VarUseModifiers(varname, ')') if lexer.SkipByte(')') { return &MkCond{Empty: &MkVarUse{varname, modifiers}} } @@ -631,7 +257,7 @@ func (p *MkParser) mkCondFunc() *MkCond { case "commands", "exists", "make", "target": argMark := lexer.Mark() - for p.VarUse() != nil || lexer.NextBytesFunc(func(b byte) bool { return b != '$' && b != ')' }) != "" { + for p.mklex.VarUse() != nil || lexer.NextBytesFunc(func(b byte) bool { return b != '$' && b != ')' }) != "" { } arg := lexer.Since(argMark) if lexer.SkipByte(')') { @@ -643,21 +269,6 @@ func (p *MkParser) mkCondFunc() *MkCond { return nil } -func (p *MkParser) Varname() string { - lexer := p.lexer - - // TODO: duplicated code in MatchVarassign - mark := lexer.Mark() - lexer.SkipByte('.') - for lexer.NextBytesSet(VarbaseBytes) != "" || p.VarUse() != nil { - } - if lexer.SkipByte('.') || hasPrefix(lexer.Since(mark), "SITES_") { - for lexer.NextBytesSet(VarparamBytes) != "" || p.VarUse() != nil { - } - } - return lexer.Since(mark) -} - func (p *MkParser) Op() (bool, MkOperator) { lexer := p.lexer switch { @@ -681,7 +292,7 @@ func (p *MkParser) PkgbasePattern() string { start := lexer.Mark() for { - if p.VarUse() != nil || + if p.mklex.VarUse() != nil || lexer.SkipRegexp(regcomp(`^[\w.*+,{}]+`)) || lexer.SkipRegexp(regcomp(`^\[[\w-]+\]`)) { continue @@ -716,7 +327,7 @@ func (*MkParser) isPkgbasePart(str string) bool { return true } - varUse := NewMkParser(nil, lexer.Rest()).VarUse() + varUse := NewMkLexer(lexer.Rest(), nil).VarUse() if varUse != nil { return !contains(varUse.varname, "VER") && len(varUse.modifiers) == 0 } @@ -740,7 +351,7 @@ func (p *MkParser) Dependency() *DependencyPattern { parseVersion := func() string { mark := lexer.Mark() - for p.VarUse() != nil { + for p.mklex.VarUse() != nil { } if lexer.Since(mark) != "" { return lexer.Since(mark) @@ -799,7 +410,7 @@ func (p *MkParser) Dependency() *DependencyPattern { if lexer.SkipByte('-') && lexer.Rest() != "" { versionMark := lexer.Mark() - for p.VarUse() != nil || lexer.SkipRegexp(regcomp(`^[\w\[\]*_.\-]+`)) { + for p.mklex.VarUse() != nil || lexer.SkipRegexp(regcomp(`^[\w\[\]*_.\-]+`)) { } if !lexer.SkipString("{,nb*}") { @@ -822,7 +433,7 @@ func (p *MkParser) Dependency() *DependencyPattern { // if there is a parse error or some trailing text. // Parse errors are silently ignored. func ToVarUse(str string) *MkVarUse { - p := NewMkParser(nil, str) + p := NewMkLexer(str, nil) varUse := p.VarUse() if varUse == nil || !p.EOF() { return nil @@ -970,7 +581,7 @@ func (w *MkCondWalker) walkAtom(atom *MkCondTerm, callback *MkCondCallback) { func (w *MkCondWalker) walkStr(str string, callback *MkCondCallback) { if callback.VarUse != nil { - tokens := NewMkParser(nil, str).MkTokens() + tokens, _ := NewMkLexer(str, nil).MkTokens() for _, token := range tokens { if token.Varuse != nil { callback.VarUse(token.Varuse) diff --git a/pkgtools/pkglint/files/mkparser_test.go b/pkgtools/pkglint/files/mkparser_test.go index f103fae406f..e6b3c8ed668 100644 --- a/pkgtools/pkglint/files/mkparser_test.go +++ b/pkgtools/pkglint/files/mkparser_test.go @@ -5,707 +5,6 @@ import ( "strings" ) -func (s *Suite) Test_MkParser_MkTokens(c *check.C) { - t := s.Init(c) - b := NewMkTokenBuilder() - - testRest := func(input string, expectedTokens []*MkToken, expectedRest string) { - line := t.NewLines("Test_MkParser_MkTokens.mk", input).Lines[0] - p := NewMkParser(line, input) - actualTokens := p.MkTokens() - t.CheckDeepEquals(actualTokens, expectedTokens) - for i, expectedToken := range expectedTokens { - if i < len(actualTokens) { - t.CheckDeepEquals(*actualTokens[i], *expectedToken) - t.CheckDeepEquals(actualTokens[i].Varuse, expectedToken.Varuse) - } - } - t.CheckEquals(p.Rest(), expectedRest) - } - test := func(input string, expectedToken *MkToken) { - testRest(input, b.Tokens(expectedToken), "") - } - literal := b.TextToken - varuse := b.VaruseToken - - // Everything except VarUses is passed through unmodified. - - test("literal", - literal("literal")) - - test("\\/share\\/ { print \"share directory\" }", - literal("\\/share\\/ { print \"share directory\" }")) - - test("find . -name \\*.orig -o -name \\*.pre", - literal("find . -name \\*.orig -o -name \\*.pre")) - - test("-e 's|\\$${EC2_HOME.*}|EC2_HOME}|g'", - literal("-e 's|\\$${EC2_HOME.*}|EC2_HOME}|g'")) - - test("$$var1 $$var2 $$? $$", - literal("$$var1 $$var2 $$? $$")) - - testRest("hello, ${W:L:tl}orld", - b.Tokens( - literal("hello, "), - varuse("W", "L", "tl"), - literal("orld")), - "") - - testRest("ftp://${PKGNAME}/ ${MASTER_SITES:=subdir/}", - b.Tokens( - literal("ftp://"), - varuse("PKGNAME"), - literal("/ "), - varuse("MASTER_SITES", "=subdir/")), - "") - - testRest("${VAR:S,a,b,c,d,e,f}", - b.Tokens(b.VaruseTextToken("${VAR:S,a,b,c,d,e,f}", "VAR", "S,a,b,")), - "") - t.CheckOutputLines( - "WARN: Test_MkParser_MkTokens.mk:1: Invalid variable modifier \"c,d,e,f\" for \"VAR\".") - - testRest("Text${VAR:Mmodifier}${VAR2}more text${VAR3}", - b.Tokens( - literal("Text"), - varuse("VAR", "Mmodifier"), - varuse("VAR2"), - literal("more text"), - varuse("VAR3")), - "") -} - -func (s *Suite) Test_MkParser_VarUse(c *check.C) { - t := s.Init(c) - b := NewMkTokenBuilder() - varuse := b.VaruseToken - varuseText := b.VaruseTextToken - - testRest := func(input string, expectedTokens []*MkToken, expectedRest string, diagnostics ...string) { - line := t.NewLines("Test_MkParser_VarUse.mk", input).Lines[0] - p := NewMkParser(line, input) - - actualTokens := p.MkTokens() - - t.CheckDeepEquals(actualTokens, expectedTokens) - for i, expectedToken := range expectedTokens { - if i < len(actualTokens) { - t.CheckDeepEquals(*actualTokens[i], *expectedToken) - t.CheckDeepEquals(actualTokens[i].Varuse, expectedToken.Varuse) - } - } - t.CheckEquals(p.Rest(), expectedRest) - t.CheckOutput(diagnostics) - } - test := func(input string, expectedToken *MkToken, diagnostics ...string) { - testRest(input, b.Tokens(expectedToken), "", diagnostics...) - } - - t.Use(testRest, test, varuse, varuseText) - - test("${VARIABLE}", - varuse("VARIABLE")) - - test("${VARIABLE.param}", - varuse("VARIABLE.param")) - - test("${VARIABLE.${param}}", - varuse("VARIABLE.${param}")) - - test("${VARIABLE.hicolor-icon-theme}", - varuse("VARIABLE.hicolor-icon-theme")) - - test("${VARIABLE.gtk+extra}", - varuse("VARIABLE.gtk+extra")) - - test("${VARIABLE:S/old/new/}", - varuse("VARIABLE", "S/old/new/")) - - test("${GNUSTEP_LFLAGS:S/-L//g}", - varuse("GNUSTEP_LFLAGS", "S/-L//g")) - - test("${SUSE_VERSION:S/.//}", - varuse("SUSE_VERSION", "S/.//")) - - test("${MASTER_SITE_GNOME:=sources/alacarte/0.13/}", - varuse("MASTER_SITE_GNOME", "=sources/alacarte/0.13/")) - - test("${INCLUDE_DIRS:H:T}", - varuse("INCLUDE_DIRS", "H", "T")) - - test("${A.${B.${C.${D}}}}", - varuse("A.${B.${C.${D}}}")) - - test("${RUBY_VERSION:C/([0-9]+)\\.([0-9]+)\\.([0-9]+)/\\1/}", - varuse("RUBY_VERSION", "C/([0-9]+)\\.([0-9]+)\\.([0-9]+)/\\1/")) - - test("${PERL5_${_var_}:Q}", - varuse("PERL5_${_var_}", "Q")) - - test("${PKGNAME_REQD:C/(^.*-|^)py([0-9][0-9])-.*/\\2/}", - varuse("PKGNAME_REQD", "C/(^.*-|^)py([0-9][0-9])-.*/\\2/")) - - test("${PYLIB:S|/|\\\\/|g}", - varuse("PYLIB", "S|/|\\\\/|g")) - - test("${PKGNAME_REQD:C/ruby([0-9][0-9]+)-.*/\\1/}", - varuse("PKGNAME_REQD", "C/ruby([0-9][0-9]+)-.*/\\1/")) - - test("${RUBY_SHLIBALIAS:S/\\//\\\\\\//}", - varuse("RUBY_SHLIBALIAS", "S/\\//\\\\\\//")) - - test("${RUBY_VER_MAP.${RUBY_VER}:U${RUBY_VER}}", - varuse("RUBY_VER_MAP.${RUBY_VER}", "U${RUBY_VER}")) - - test("${RUBY_VER_MAP.${RUBY_VER}:U18}", - varuse("RUBY_VER_MAP.${RUBY_VER}", "U18")) - - test("${CONFIGURE_ARGS:S/ENABLE_OSS=no/ENABLE_OSS=yes/g}", - varuse("CONFIGURE_ARGS", "S/ENABLE_OSS=no/ENABLE_OSS=yes/g")) - - test("${PLIST_RUBY_DIRS:S,DIR=\"PREFIX/,DIR=\",}", - varuse("PLIST_RUBY_DIRS", "S,DIR=\"PREFIX/,DIR=\",")) - - test("${LDFLAGS:S/-Wl,//g:Q}", - varuse("LDFLAGS", "S/-Wl,//g", "Q")) - - test("${_PERL5_REAL_PACKLIST:S/^/${DESTDIR}/}", - varuse("_PERL5_REAL_PACKLIST", "S/^/${DESTDIR}/")) - - test("${_PYTHON_VERSION:C/^([0-9])/\\1./1}", - varuse("_PYTHON_VERSION", "C/^([0-9])/\\1./1")) - - test("${PKGNAME:S/py${_PYTHON_VERSION}/py${i}/}", - varuse("PKGNAME", "S/py${_PYTHON_VERSION}/py${i}/")) - - test("${PKGNAME:C/-[0-9].*$/-[0-9]*/}", - varuse("PKGNAME", "C/-[0-9].*$/-[0-9]*/")) - - // TODO: Does the $@ refer to ${.TARGET}, or is it just an unmatchable - // regular expression? - test("${PKGNAME:C/$@/target?/}", - varuse("PKGNAME", "C/$@/target?/")) - - test("${PKGNAME:S/py${_PYTHON_VERSION}/py${i}/:C/-[0-9].*$/-[0-9]*/}", - varuse("PKGNAME", "S/py${_PYTHON_VERSION}/py${i}/", "C/-[0-9].*$/-[0-9]*/")) - - test("${_PERL5_VARS:tl:S/^/-V:/}", - varuse("_PERL5_VARS", "tl", "S/^/-V:/")) - - test("${_PERL5_VARS_OUT:M${_var_:tl}=*:S/^${_var_:tl}=${_PERL5_PREFIX:=/}//}", - varuse("_PERL5_VARS_OUT", "M${_var_:tl}=*", "S/^${_var_:tl}=${_PERL5_PREFIX:=/}//")) - - test("${RUBY${RUBY_VER}_PATCHLEVEL}", - varuse("RUBY${RUBY_VER}_PATCHLEVEL")) - - test("${DISTFILES:M*.gem}", - varuse("DISTFILES", "M*.gem")) - - test("${LOCALBASE:S^/^_^}", - varuse("LOCALBASE", "S^/^_^")) - - test("${SOURCES:%.c=%.o}", - varuse("SOURCES", "%.c=%.o")) - - test("${GIT_TEMPLATES:@.t.@ ${EGDIR}/${GIT_TEMPLATEDIR}/${.t.} ${PREFIX}/${GIT_CORE_TEMPLATEDIR}/${.t.} @:M*}", - varuse("GIT_TEMPLATES", "@.t.@ ${EGDIR}/${GIT_TEMPLATEDIR}/${.t.} ${PREFIX}/${GIT_CORE_TEMPLATEDIR}/${.t.} @", "M*")) - - test("${DISTNAME:C:_:-:}", - varuse("DISTNAME", "C:_:-:")) - - test("${CF_FILES:H:O:u:S@^@${PKG_SYSCONFDIR}/@}", - varuse("CF_FILES", "H", "O", "u", "S@^@${PKG_SYSCONFDIR}/@")) - - test("${ALT_GCC_RTS:S%${LOCALBASE}%%:S%/%%}", - varuse("ALT_GCC_RTS", "S%${LOCALBASE}%%", "S%/%%")) - - test("${PREFIX:C;///*;/;g:C;/$;;}", - varuse("PREFIX", "C;///*;/;g", "C;/$;;")) - - test("${GZIP_CMD:[1]:Q}", - varuse("GZIP_CMD", "[1]", "Q")) - - test("${RUBY_RAILS_SUPPORTED:[#]}", - varuse("RUBY_RAILS_SUPPORTED", "[#]")) - - test("${GZIP_CMD:[asdf]:Q}", - varuseText("${GZIP_CMD:[asdf]:Q}", "GZIP_CMD", "Q"), - "WARN: Test_MkParser_VarUse.mk:1: Invalid variable modifier \"[asdf]\" for \"GZIP_CMD\".") - - test("${DISTNAME:C/-[0-9]+$$//:C/_/-/}", - varuse("DISTNAME", "C/-[0-9]+$$//", "C/_/-/")) - - test("${DISTNAME:slang%=slang2%}", - varuse("DISTNAME", "slang%=slang2%")) - - test("${OSMAP_SUBSTVARS:@v@-e 's,\\@${v}\\@,${${v}},g' @}", - varuse("OSMAP_SUBSTVARS", "@v@-e 's,\\@${v}\\@,${${v}},g' @")) - - test("${BRANDELF:D${BRANDELF} -t Linux ${LINUX_LDCONFIG}:U${TRUE}}", - varuse("BRANDELF", "D${BRANDELF} -t Linux ${LINUX_LDCONFIG}", "U${TRUE}")) - - test("${${_var_}.*}", - varuse("${_var_}.*")) - - test("${OPTIONS:@opt@printf 'Option %s is selected\n' ${opt:Q}';@}", - varuse("OPTIONS", "@opt@printf 'Option %s is selected\n' ${opt:Q}';@")) - - /* weird features */ - test("${${EMACS_VERSION_MAJOR}>22:?@comment :}", - varuse("${EMACS_VERSION_MAJOR}>22", "?@comment :")) - - test("${empty(CFLAGS):?:-cflags ${CFLAGS:Q}}", - varuse("empty(CFLAGS)", "?:-cflags ${CFLAGS:Q}")) - - test("${${PKGSRC_COMPILER}==gcc:?gcc:cc}", - varuse("${PKGSRC_COMPILER}==gcc", "?gcc:cc")) - - test("${${XKBBASE}/xkbcomp:L:Q}", - varuse("${XKBBASE}/xkbcomp", "L", "Q")) - - test("${${PKGBASE} ${PKGVERSION}:L}", - varuse("${PKGBASE} ${PKGVERSION}", "L")) - - // The variable name is optional; the variable with the empty name always - // evaluates to the empty string. Bmake actively prevents this variable from - // ever being defined. Therefore the :U branch is always taken, and this - // in turn is used to implement the variables from the .for loops. - test("${:U}", - varuse("", "U")) - - test("${:Ufixed value}", - varuse("", "Ufixed value")) - - // This complicated expression returns the major.minor.patch version - // of the package given in ${d}. - // - // The :L modifier interprets the variable name not as a variable name - // but takes it as the variable value. Followed by the :sh modifier, - // this combination evaluates to the output of pkg_info. - // - // In this output, all non-digit characters are replaced with spaces so - // that the remaining value is a space-separated list of version parts. - // From these parts, the first 3 are taken and joined using a dot as separator. - test("${${${PKG_INFO} -E ${d} || echo:L:sh}:L:C/[^[0-9]]*/ /g:[1..3]:ts.}", - varuse("${${PKG_INFO} -E ${d} || echo:L:sh}", "L", "C/[^[0-9]]*/ /g", "[1..3]", "ts.")) - - // For :S and :C, the colon can be left out. It's confusing but possible. - test("${VAR:S/-//S/.//}", - varuseText("${VAR:S/-//S/.//}", "VAR", "S/-//", "S/.//")) - - // The :S and :C modifiers accept an arbitrary character as separator. Here it is "a". - test("${VAR:Sahara}", - varuse("VAR", "Sahara")) - - // The separator character can be left out, which means empty. - test("${VAR:ts}", - varuse("VAR", "ts")) - - // The separator character can be a long octal number. - test("${VAR:ts\\000012}", - varuse("VAR", "ts\\000012")) - - // Or even decimal. - test("${VAR:ts\\124}", - varuse("VAR", "ts\\124")) - - // The :ts modifier only takes single-character separators. - test("${VAR:ts---}", - varuse("VAR", "ts---"), - "WARN: Test_MkParser_VarUse.mk:1: Invalid separator \"---\" for :ts modifier of \"VAR\".") - - test("$<", - varuseText("$<", "<")) // Same as ${.IMPSRC} - - test("$(GNUSTEP_USER_ROOT)", - varuseText("$(GNUSTEP_USER_ROOT)", "GNUSTEP_USER_ROOT"), - "WARN: Test_MkParser_VarUse.mk:1: Please use curly braces {} instead of round parentheses () for GNUSTEP_USER_ROOT.") - - // Opening brace, closing parenthesis. - // Warnings are only printed for balanced expressions. - test("${VAR)", - varuseText("${VAR)", "VAR)"), - "WARN: Test_MkParser_VarUse.mk:1: Missing closing \"}\" for \"VAR)\".", - "WARN: Test_MkParser_VarUse.mk:1: Invalid part \")\" after variable name \"VAR\".") - - // Opening parenthesis, closing brace - // Warnings are only printed for balanced expressions. - test("$(VAR}", - varuseText("$(VAR}", "VAR}"), - "WARN: Test_MkParser_VarUse.mk:1: Missing closing \")\" for \"VAR}\".", - "WARN: Test_MkParser_VarUse.mk:1: Invalid part \"}\" after variable name \"VAR\".") - - test("${PLIST_SUBST_VARS:@var@${var}=${${var}:Q}@}", - varuse("PLIST_SUBST_VARS", "@var@${var}=${${var}:Q}@")) - - test("${PLIST_SUBST_VARS:@var@${var}=${${var}:Q}}", - varuseText("${PLIST_SUBST_VARS:@var@${var}=${${var}:Q}}", - "PLIST_SUBST_VARS", "@var@${var}=${${var}:Q}}"), - "WARN: Test_MkParser_VarUse.mk:1: Modifier ${PLIST_SUBST_VARS:@var@...@} is missing the final \"@\".", - "WARN: Test_MkParser_VarUse.mk:1: Missing closing \"}\" for \"PLIST_SUBST_VARS\".") - - // The replacement text may include closing braces, which is useful - // for AWK programs. - test("${PLIST_SUBST_VARS:@var@{${var}}@}", - varuseText("${PLIST_SUBST_VARS:@var@{${var}}@}", - "PLIST_SUBST_VARS", "@var@{${var}}@"), - nil...) - - // Unfinished variable use - test("${", - varuseText("${", ""), - "WARN: Test_MkParser_VarUse.mk:1: Missing closing \"}\" for \"\".") - - // Unfinished nested variable use - test("${${", - varuseText("${${", "${"), - "WARN: Test_MkParser_VarUse.mk:1: Missing closing \"}\" for \"\".", - "WARN: Test_MkParser_VarUse.mk:1: Missing closing \"}\" for \"${\".") - - test("${arbitrary :Mpattern:---:Q}", - varuseText("${arbitrary :Mpattern:---:Q}", "arbitrary ", "Mpattern", "Q"), - // TODO: Swap the order of these message - "WARN: Test_MkParser_VarUse.mk:1: Invalid variable modifier \"---\" for \"arbitrary \".", - "WARN: Test_MkParser_VarUse.mk:1: Invalid part \" \" after variable name \"arbitrary\".") - - // Variable names containing spaces do not occur in pkgsrc. - // Technically they are possible: - // - // VARNAME= name with spaces - // ${VARNAME}= value - // - // all: - // @echo ${name with spaces:Q}'' - test("${arbitrary text}", - varuse("arbitrary text"), - "WARN: Test_MkParser_VarUse.mk:1: Invalid part \" text\" after variable name \"arbitrary\".") -} - -func (s *Suite) Test_MkParser_VarUse__ambiguous(c *check.C) { - t := s.Init(c) - b := NewMkTokenBuilder() - - t.SetUpCommandLine("--explain") - - line := t.NewLine("module.mk", 123, "\t$Varname $X") - p := NewMkParser(line, line.Text[1:]) - - tokens := p.MkTokens() - t.CheckDeepEquals(tokens, b.Tokens( - b.VaruseTextToken("$V", "V"), - b.TextToken("arname "), - b.VaruseTextToken("$X", "X"))) - - t.CheckOutputLines( - "ERROR: module.mk:123: $Varname is ambiguous. Use ${Varname} if you mean a Make variable or $$Varname if you mean a shell variable.", - "", - "\tOnly the first letter after the dollar is the variable name.", - "\tEverything following it is normal text, even if it looks like a", - "\tvariable name to human readers.", - "", - "WARN: module.mk:123: $X is ambiguous. Use ${X} if you mean a Make variable or $$X if you mean a shell variable.", - "", - "\tIn its current form, this variable is parsed as a Make variable. For", - "\thuman readers though, $x looks more like a shell variable than a", - "\tMake variable, since Make variables are usually written using braces", - "\t(BSD-style) or parentheses (GNU-style).", - "") -} - -// Pkglint can replace $(VAR) with ${VAR}. It doesn't look at all components -// of nested variables though because this case is not important enough to -// invest much development time. It occurs so seldom that it is acceptable -// to run pkglint multiple times in such a case. -func (s *Suite) Test_MkParser_VarUse__parentheses_autofix(c *check.C) { - t := s.Init(c) - - t.SetUpCommandLine("--autofix") - t.SetUpVartypes() - lines := t.SetUpFileLines("Makefile", - MkCvsID, - "COMMENT=$(P1) $(P2)) $(P3:Q) ${BRACES} $(A.$(B.$(C)))") - mklines := NewMkLines(lines) - - mklines.Check() - - t.CheckOutputLines( - "AUTOFIX: ~/Makefile:2: Replacing \"$(P1)\" with \"${P1}\".", - "AUTOFIX: ~/Makefile:2: Replacing \"$(P2)\" with \"${P2}\".", - "AUTOFIX: ~/Makefile:2: Replacing \"$(P3:Q)\" with \"${P3:Q}\".", - "AUTOFIX: ~/Makefile:2: Replacing \"$(C)\" with \"${C}\".") - t.CheckFileLines("Makefile", - MkCvsID, - "COMMENT=${P1} ${P2}) ${P3:Q} ${BRACES} $(A.$(B.${C}))") -} - -func (s *Suite) Test_MkParser_VarUseModifiers(c *check.C) { - t := s.Init(c) - - varUse := NewMkTokenBuilder().VarUse - test := func(text string, varUse *MkVarUse, diagnostics ...string) { - line := t.NewLine("Makefile", 20, "\t"+text) - p := NewMkParser(line, text) - - actual := p.VarUse() - - t.CheckDeepEquals(actual, varUse) - t.CheckEquals(p.Rest(), "") - t.CheckOutput(diagnostics) - } - - // The !command! modifier is used so seldom that pkglint does not - // check whether the command is actually valid. - // At least not while parsing the modifier since at this point it might - // be still unknown which of the commands can be used and which cannot. - test("${VAR:!command!}", varUse("VAR", "!command!")) - - test("${VAR:!command}", varUse("VAR"), - "WARN: Makefile:20: Invalid variable modifier \"!command\" for \"VAR\".") - - test("${VAR:command!}", varUse("VAR"), - "WARN: Makefile:20: Invalid variable modifier \"command!\" for \"VAR\".") - - // The :L modifier makes the variable value "echo hello", and the :[1] - // modifier extracts the "echo". - test("${echo hello:L:[1]}", varUse("echo hello", "L", "[1]")) - - // bmake ignores the :[3] modifier, and the :L modifier just returns the - // variable name, in this case BUILD_DIRS. - test("${BUILD_DIRS:[3]:L}", varUse("BUILD_DIRS", "[3]", "L")) - - test("${PATH:ts::Q}", varUse("PATH", "ts:", "Q")) -} - -func (s *Suite) Test_MkParser_varUseModifier__invalid_ts_modifier_with_warning(c *check.C) { - t := s.Init(c) - - t.SetUpCommandLine("-Wall", "--explain") - line := t.NewLine("filename.mk", 123, "${VAR:tsabc}") - p := NewMkParser(line, "tsabc}") - - modifier := p.varUseModifier("VAR", '}') - - t.CheckEquals(modifier, "tsabc") - t.CheckEquals(p.Rest(), "}") - t.CheckOutputLines( - "WARN: filename.mk:123: Invalid separator \"abc\" for :ts modifier of \"VAR\".", - "", - "\tThe separator for the :ts modifier must be either a single character", - "\tor an escape sequence like \\t or \\n or an octal or decimal escape", - "\tsequence; see the bmake man page for further details.", - "") -} - -func (s *Suite) Test_MkParser_varUseModifier__invalid_ts_modifier_without_warning(c *check.C) { - t := s.Init(c) - - p := NewMkParser(nil, "tsabc}") - - modifier := p.varUseModifier("VAR", '}') - - t.CheckEquals(modifier, "tsabc") - t.CheckEquals(p.Rest(), "}") -} - -func (s *Suite) Test_MkParser_varUseModifier__square_bracket(c *check.C) { - t := s.Init(c) - - line := t.NewLine("filename.mk", 123, "\t${VAR:[asdf]}") - p := NewMkParser(line, "[asdf]") - - modifier := p.varUseModifier("VAR", '}') - - t.CheckEquals(modifier, "") - t.CheckEquals(p.Rest(), "") - - t.CheckOutputLines( - "WARN: filename.mk:123: Invalid variable modifier \"[asdf]\" for \"VAR\".") -} - -func (s *Suite) Test_MkParser_varUseModifier__condition_without_colon(c *check.C) { - t := s.Init(c) - b := NewMkTokenBuilder() - - line := t.NewLine("filename.mk", 123, "${${VAR}:?yes:no}${${VAR}:?yes}") - p := NewMkParser(line, line.Text) - - varUse1 := p.VarUse() - varUse2 := p.VarUse() - - t.CheckDeepEquals(varUse1, b.VarUse("${VAR}", "?yes:no")) - t.CheckDeepEquals(varUse2, b.VarUse("${VAR}")) - t.CheckEquals(p.Rest(), "") - - t.CheckOutputLines( - "WARN: filename.mk:123: Invalid variable modifier \"?yes\" for \"${VAR}\".") -} - -func (s *Suite) Test_MkParser_varUseModifier__malformed_in_parentheses(c *check.C) { - t := s.Init(c) - b := NewMkTokenBuilder() - - line := t.NewLine("filename.mk", 123, "$(${VAR}:?yes)") - p := NewMkParser(line, line.Text) - - varUse := p.VarUse() - - t.CheckDeepEquals(varUse, b.VarUse("${VAR}")) - t.CheckEquals(p.Rest(), "") - - t.CheckOutputLines( - "WARN: filename.mk:123: Invalid variable modifier \"?yes\" for \"${VAR}\".", - "WARN: filename.mk:123: Please use curly braces {} instead of round parentheses () for ${VAR}.") -} - -func (s *Suite) Test_MkParser_varUseModifier__varuse_in_malformed_modifier(c *check.C) { - t := s.Init(c) - b := NewMkTokenBuilder() - - line := t.NewLine("filename.mk", 123, "${${VAR}:?yes${INNER}}") - p := NewMkParser(line, line.Text) - - varUse := p.VarUse() - - t.CheckDeepEquals(varUse, b.VarUse("${VAR}")) - t.CheckEquals(p.Rest(), "") - - t.CheckOutputLines( - "WARN: filename.mk:123: Invalid variable modifier \"?yes${INNER}\" for \"${VAR}\".") -} - -func (s *Suite) Test_MkParser_varUseModifierSubst(c *check.C) { - t := s.Init(c) - - varUse := NewMkTokenBuilder().VarUse - test := func(text string, varUse *MkVarUse, rest string, diagnostics ...string) { - line := t.NewLine("Makefile", 20, "\t"+text) - p := NewMkParser(line, text) - - actual := p.VarUse() - - t.CheckDeepEquals(actual, varUse) - t.CheckEquals(p.Rest(), rest) - t.CheckOutput(diagnostics) - } - - test("${VAR:S", varUse("VAR"), "", - "WARN: Makefile:20: Invalid variable modifier \"S\" for \"VAR\".", - "WARN: Makefile:20: Missing closing \"}\" for \"VAR\".") - - test("${VAR:S}", varUse("VAR"), "", - "WARN: Makefile:20: Invalid variable modifier \"S\" for \"VAR\".") - - test("${VAR:S,}", varUse("VAR"), "", - "WARN: Makefile:20: Invalid variable modifier \"S,\" for \"VAR\".") - - test("${VAR:S,from,to}", varUse("VAR"), "", - "WARN: Makefile:20: Invalid variable modifier \"S,from,to\" for \"VAR\".") - - test("${VAR:S,from,to,}", varUse("VAR", "S,from,to,"), "") - - test("${VAR:S,^from$,to,}", varUse("VAR", "S,^from$,to,"), "") - - test("${VAR:S,@F@,${F},}", varUse("VAR", "S,@F@,${F},"), "") - - test("${VAR:S,from,to,1}", varUse("VAR", "S,from,to,1"), "") - test("${VAR:S,from,to,g}", varUse("VAR", "S,from,to,g"), "") - test("${VAR:S,from,to,W}", varUse("VAR", "S,from,to,W"), "") - - test("${VAR:S,from,to,1gW}", varUse("VAR", "S,from,to,1gW"), "") - - // Inside the :S or :C modifiers, neither a colon nor the closing - // brace need to be escaped. Otherwise these patterns would become - // too difficult to read and write. - test("${VAR:C/[[:alnum:]]{2}/**/g}", - varUse("VAR", "C/[[:alnum:]]{2}/**/g"), - "") - - // Some pkgsrc users really explore the darkest corners of bmake by using - // the backslash as the separator in the :S modifier. Sure, it works, it - // just looks totally unexpected to the average pkgsrc reader. - // - // Using the backslash as separator means that it cannot be used for anything - // else, not even for escaping other characters. - test("${VAR:S\\.post1\\\\1}", - varUse("VAR", "S\\.post1\\\\1"), - "") -} - -func (s *Suite) Test_MkParser_varUseModifierAt__missing_at_after_variable_name(c *check.C) { - t := s.Init(c) - b := NewMkTokenBuilder() - - line := t.NewLine("filename.mk", 123, "${VAR:@varname}") - p := NewMkParser(line, line.Text) - - varUse := p.VarUse() - - t.CheckDeepEquals(varUse, b.VarUse("VAR")) - t.CheckEquals(p.Rest(), "") - t.CheckOutputLines( - "WARN: filename.mk:123: Invalid variable modifier \"@varname\" for \"VAR\".") -} - -func (s *Suite) Test_MkParser_varUseModifierAt__dollar(c *check.C) { - t := s.Init(c) - b := NewMkTokenBuilder() - - line := t.NewLine("filename.mk", 123, "${VAR:@var@$$var@}") - p := NewMkParser(line, line.Text) - - varUse := p.VarUse() - - t.CheckDeepEquals(varUse, b.VarUse("VAR", "@var@$$var@")) - t.CheckEquals(p.Rest(), "") - t.CheckOutputEmpty() -} - -func (s *Suite) Test_MkParser_varUseModifierAt__incomplete_without_warning(c *check.C) { - t := s.Init(c) - b := NewMkTokenBuilder() - - p := NewMkParser(nil, "${VAR:@var@$$var}rest") - - varUse := p.VarUse() - - t.CheckDeepEquals(varUse, b.VarUse("VAR", "@var@$$var}rest")) - t.CheckEquals(p.Rest(), "") - t.CheckOutputEmpty() -} - -func (s *Suite) Test_MkParser_varUseModifierAt(c *check.C) { - t := s.Init(c) - - varUse := NewMkTokenBuilder().VarUse - test := func(text string, varUse *MkVarUse, rest string, diagnostics ...string) { - line := t.NewLine("Makefile", 20, "\t"+text) - p := NewMkParser(line, text) - - actual := p.VarUse() - - t.CheckDeepEquals(actual, varUse) - t.CheckEquals(p.Rest(), rest) - t.CheckOutput(diagnostics) - } - - test("${VAR:@", - varUse("VAR"), - "", - "WARN: Makefile:20: Invalid variable modifier \"@\" for \"VAR\".", - "WARN: Makefile:20: Missing closing \"}\" for \"VAR\".") - - test("${VAR:@i@${i}}", varUse("VAR", "@i@${i}}"), "", - "WARN: Makefile:20: Modifier ${VAR:@i@...@} is missing the final \"@\".", - "WARN: Makefile:20: Missing closing \"}\" for \"VAR\".") - - test("${VAR:@i@${i}@}", varUse("VAR", "@i@${i}@"), "") - - test("${PKG_GROUPS:@g@${g:Q}:${PKG_GID.${g}:Q}@:C/:*$//g}", - varUse("PKG_GROUPS", "@g@${g:Q}:${PKG_GID.${g}:Q}@", "C/:*$//g"), - "") -} - func (s *Suite) Test_MkParser_MkCond(c *check.C) { t := s.Init(c) b := NewMkTokenBuilder() @@ -971,40 +270,6 @@ func (s *Suite) Test_MkParser_mkCondCompare(c *check.C) { t.CheckOutputEmpty() } -func (s *Suite) Test_MkParser_Varname(c *check.C) { - t := s.Init(c) - - test := func(text string) { - line := t.NewLine("filename.mk", 1, text) - p := NewMkParser(line, text) - - varname := p.Varname() - - t.CheckEquals(varname, text) - t.CheckEquals(p.Rest(), "") - } - - testRest := func(text string, expectedVarname string, expectedRest string) { - line := t.NewLine("filename.mk", 1, text) - p := NewMkParser(line, text) - - varname := p.Varname() - - t.CheckEquals(varname, expectedVarname) - t.CheckEquals(p.Rest(), expectedRest) - } - - test("VARNAME") - test("VARNAME.param") - test("VARNAME.${param}") - test("SITES_${param}") - test("SITES_distfile-1.0.tar.gz") - test("SITES.gtk+-2.0") - test("PKGPATH.category/package") - - testRest("VARNAME/rest", "VARNAME", "/rest") -} - func (s *Suite) Test_MkParser_PkgbasePattern(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/mkshparser.go b/pkgtools/pkglint/files/mkshparser.go index 1fd0af412a5..25a11e8d96d 100644 --- a/pkgtools/pkglint/files/mkshparser.go +++ b/pkgtools/pkglint/files/mkshparser.go @@ -238,7 +238,7 @@ func (lex *ShellLexer) Lex(lval *shyySymType) (ttype int) { lex.atCommandStart = false case lex.atCommandStart && matches(token, `^[A-Za-z_]\w*=`): ttype = tkASSIGNMENT_WORD - p := NewShTokenizer(dummyLine, token, false) // Just for converting the string to a ShToken + p := NewShTokenizer(nil, token, false) lval.Word = p.ShToken() case hasPrefix(token, "#"): // This doesn't work for multiline shell programs. @@ -246,7 +246,7 @@ func (lex *ShellLexer) Lex(lval *shyySymType) (ttype int) { return 0 default: ttype = tkWORD - p := NewShTokenizer(dummyLine, token, false) // Just for converting the string to a ShToken + p := NewShTokenizer(nil, token, false) lval.Word = p.ShToken() lex.atCommandStart = false diff --git a/pkgtools/pkglint/files/mkshparser_test.go b/pkgtools/pkglint/files/mkshparser_test.go index 0c828fdb8a4..835747bc63e 100644 --- a/pkgtools/pkglint/files/mkshparser_test.go +++ b/pkgtools/pkglint/files/mkshparser_test.go @@ -65,10 +65,11 @@ type ShSuite struct { var _ = check.Suite(&ShSuite{}) func (s *ShSuite) SetUpTest(*check.C) { - G = NewPkglint(nil, nil) + G = unusablePkglint() } func (s *ShSuite) TearDownTest(*check.C) { + s.t.ReportUncheckedOutput() G = unusablePkglint() } @@ -268,10 +269,21 @@ func (s *ShSuite) Test_parseShellProgram__compound_command(c *check.C) { b.Words("*"), b.List().AddCommand(b.SimpleCommand("echo", "$i")).AddSemicolon()))) + s.t.CheckOutputLines( + "WARN: MkShBuilder.Token.mk:1: $i is ambiguous. Use ${i} if you "+ + "mean a Make variable or $$i if you mean a shell variable.", + "WARN: ShSuite.test.mk:1: $i is ambiguous. Use ${i} if you "+ + "mean a Make variable or $$i if you mean a shell variable.") + s.test("case $i in esac", b.List().AddCommand(b.Case( b.Token("$i")))) + s.t.CheckOutputLines( + "WARN: MkShBuilder.Token.mk:1: $i is ambiguous. Use ${i} if you "+ + "mean a Make variable or $$i if you mean a shell variable.", + "WARN: ShSuite.test.mk:1: $i is ambiguous. Use ${i} if you "+ + "mean a Make variable or $$i if you mean a shell variable.") } func (s *ShSuite) Test_parseShellProgram__subshell(c *check.C) { @@ -312,6 +324,12 @@ func (s *ShSuite) Test_parseShellProgram__for_clause(c *check.C) { b.Words("\"$$@\""), b.List().AddCommand(b.SimpleCommand("echo", "$var")).AddSemicolon()))) + s.t.CheckOutputLines( + "ERROR: MkShBuilder.Token.mk:1: $var is ambiguous. Use ${var} if you "+ + "mean a Make variable or $$var if you mean a shell variable.", + "ERROR: ShSuite.test.mk:1: $var is ambiguous. Use ${var} if you "+ + "mean a Make variable or $$var if you mean a shell variable.") + // Only linebreak is allowed, but not semicolon. s.test("for var \n do echo $var ; done", b.List().AddCommand(b.For( @@ -319,30 +337,60 @@ func (s *ShSuite) Test_parseShellProgram__for_clause(c *check.C) { b.Words("\"$$@\""), b.List().AddCommand(b.SimpleCommand("echo", "$var")).AddSemicolon()))) + s.t.CheckOutputLines( + "ERROR: MkShBuilder.Token.mk:1: $var is ambiguous. Use ${var} if you "+ + "mean a Make variable or $$var if you mean a shell variable.", + "ERROR: ShSuite.test.mk:1: $var is ambiguous. Use ${var} if you "+ + "mean a Make variable or $$var if you mean a shell variable.") + s.test("for var in a b c ; do echo $var ; done", b.List().AddCommand(b.For( "var", b.Words("a", "b", "c"), b.List().AddCommand(b.SimpleCommand("echo", "$var")).AddSemicolon()))) + s.t.CheckOutputLines( + "ERROR: MkShBuilder.Token.mk:1: $var is ambiguous. Use ${var} if you "+ + "mean a Make variable or $$var if you mean a shell variable.", + "ERROR: ShSuite.test.mk:1: $var is ambiguous. Use ${var} if you "+ + "mean a Make variable or $$var if you mean a shell variable.") + s.test("for var \n \n \n in a b c ; do echo $var ; done", b.List().AddCommand(b.For( "var", b.Words("a", "b", "c"), b.List().AddCommand(b.SimpleCommand("echo", "$var")).AddSemicolon()))) + s.t.CheckOutputLines( + "ERROR: MkShBuilder.Token.mk:1: $var is ambiguous. Use ${var} if you "+ + "mean a Make variable or $$var if you mean a shell variable.", + "ERROR: ShSuite.test.mk:1: $var is ambiguous. Use ${var} if you "+ + "mean a Make variable or $$var if you mean a shell variable.") + s.test("for var \n in ; do echo $var ; done", b.List().AddCommand(b.For( "var", nil, b.List().AddCommand(b.SimpleCommand("echo", "$var")).AddSemicolon()))) + s.t.CheckOutputLines( + "ERROR: MkShBuilder.Token.mk:1: $var is ambiguous. Use ${var} if you "+ + "mean a Make variable or $$var if you mean a shell variable.", + "ERROR: ShSuite.test.mk:1: $var is ambiguous. Use ${var} if you "+ + "mean a Make variable or $$var if you mean a shell variable.") + s.test("for var in in esac ; do echo $var ; done", b.List().AddCommand(b.For( "var", b.Words("in", "esac"), b.List().AddCommand(b.SimpleCommand("echo", "$var")).AddSemicolon()))) + s.t.CheckOutputLines( + "ERROR: MkShBuilder.Token.mk:1: $var is ambiguous. Use ${var} if you "+ + "mean a Make variable or $$var if you mean a shell variable.", + "ERROR: ShSuite.test.mk:1: $var is ambiguous. Use ${var} if you "+ + "mean a Make variable or $$var if you mean a shell variable.") + s.test("for var in \n do : ; done", b.List().AddCommand(b.For( "var", @@ -366,6 +414,13 @@ func (s *ShSuite) Test_parseShellProgram__case_clause(c *check.C) { s.test("case $var in esac", b.List().AddCommand(b.Case(b.Token("$var")))) + s.t.CheckOutputLines( + "ERROR: MkShBuilder.Token.mk:1: $var is ambiguous. "+ + "Use ${var} if you mean a Make variable "+ + "or $$var if you mean a shell variable.", + "ERROR: ShSuite.test.mk:1: $var is ambiguous. Use ${var} if you "+ + "mean a Make variable or $$var if you mean a shell variable.") + s.test("case selector in pattern) ;; pattern) esac", b.List().AddCommand(b.Case( b.Token("selector"), @@ -658,7 +713,9 @@ func (s *ShSuite) Test_parseShellProgram__io_here(c *check.C) { func (s *ShSuite) init(c *check.C) *MkShBuilder { s.c = c - s.t = &Tester{c: c, testName: c.TestName()} + tmpdir := NewPath("The ShSuite tests don't need a temporary directory") + s.t = &Tester{c: c, testName: c.TestName(), tmpdir: tmpdir} + G = NewPkglint(&s.t.stdout, &s.t.stderr) return NewMkShBuilder() } @@ -666,7 +723,8 @@ func (s *ShSuite) test(program string, expected *MkShList) { t := s.t // See parseShellProgram - tokens, rest := splitIntoShellTokens(dummyLine, program) + line := t.NewLine("ShSuite.test.mk", 1, "") + tokens, rest := splitIntoShellTokens(line, program) t.CheckEquals(rest, "") lexer := NewShellLexer(tokens, rest) parser := shyyParserImpl{} @@ -691,7 +749,8 @@ func (s *ShSuite) test(program string, expected *MkShList) { func (s *ShSuite) testFail(program string, expectedRemaining ...string) { t := s.t - tokens, rest := splitIntoShellTokens(dummyLine, program) + line := t.NewLine("ShSuite.testFail.mk", 1, "") + tokens, rest := splitIntoShellTokens(line, program) t.CheckEquals(rest, "") lexer := ShellLexer{remaining: tokens, atCommandStart: true} parser := shyyParserImpl{} @@ -704,9 +763,11 @@ func (s *ShSuite) testFail(program string, expectedRemaining ...string) { } func (s *ShSuite) Test_ShellLexer_Lex__redirects(c *check.C) { + _ = s.init(c) t := s.t - tokens, rest := splitIntoShellTokens(dummyLine, "2>&1 <& <>file >>file <<EOF <<-EOF > /dev/stderr") + line := t.NewLine("filename.mk", 1, "") + tokens, rest := splitIntoShellTokens(line, "2>&1 <& <>file >>file <<EOF <<-EOF > /dev/stderr") t.CheckDeepEquals(tokens, []string{"2>&", "1", "<&", "<>", "file", ">>", "file", "<<", "EOF", "<<-", "EOF", ">", "/dev/stderr"}) t.CheckEquals(rest, "") @@ -757,7 +818,8 @@ func (s *ShSuite) Test_ShellLexer_Lex__keywords(c *check.C) { t := s.t testErr := func(program, error, remaining string) { - tokens, rest := splitIntoShellTokens(dummyLine, program) + line := t.NewLine("filename.mk", 1, "") + tokens, rest := splitIntoShellTokens(line, program) t.CheckEquals(rest, "") lexer := ShellLexer{ @@ -944,7 +1006,8 @@ func (b *MkShBuilder) Redirected(cmd *MkShCommand, redirects ...*MkShRedirection } func (b *MkShBuilder) Token(mktext string) *ShToken { - tokenizer := NewShTokenizer(dummyLine, mktext, false) + line := NewLine("MkShBuilder.Token.mk", 1, "", &RawLine{1, "\n", "\n"}) + tokenizer := NewShTokenizer(line, mktext, false) token := tokenizer.ShToken() assertf(tokenizer.parser.EOF(), "Invalid token: %q", tokenizer.parser.Rest()) return token diff --git a/pkgtools/pkglint/files/mkshtypes_test.go b/pkgtools/pkglint/files/mkshtypes_test.go index 3a0b9963ba6..ea4d9916606 100644 --- a/pkgtools/pkglint/files/mkshtypes_test.go +++ b/pkgtools/pkglint/files/mkshtypes_test.go @@ -3,3 +3,14 @@ package pkglint func (list *MkShList) AddSemicolon() *MkShList { return list.AddSeparator(sepSemicolon) } func (list *MkShList) AddBackground() *MkShList { return list.AddSeparator(sepBackground) } func (list *MkShList) AddNewline() *MkShList { return list.AddSeparator(sepNewline) } + +// AddCommand adds a command directly to a list of commands, +// creating all the intermediate nodes for the syntactic representation. +// +// As soon as that representation is replaced with a semantic representation, +// this method should no longer be necessary. +func (list *MkShList) AddCommand(command *MkShCommand) *MkShList { + pipeline := NewMkShPipeline(false, []*MkShCommand{command}) + andOr := NewMkShAndOr(pipeline) + return list.AddAndOr(andOr) +} diff --git a/pkgtools/pkglint/files/mkshwalker_test.go b/pkgtools/pkglint/files/mkshwalker_test.go index 04218f80bd4..e68dc84650d 100644 --- a/pkgtools/pkglint/files/mkshwalker_test.go +++ b/pkgtools/pkglint/files/mkshwalker_test.go @@ -15,7 +15,8 @@ func (s *Suite) Test_MkShWalker_Walk(c *check.C) { } test := func(program string, output ...string) { - list, err := parseShellProgram(dummyLine, program) + line := t.NewLine("filename.mk", 1, "") + list, err := parseShellProgram(line, program) if !c.Check(err, check.IsNil) || !c.Check(list, check.NotNil) { return @@ -244,7 +245,8 @@ func (s *Suite) Test_MkShWalker_Walk__empty_callback(c *check.C) { t := s.Init(c) test := func(program string) { - list, err := parseShellProgram(dummyLine, program) + line := t.NewLine("filename.mk", 1, "") + list, err := parseShellProgram(line, program) assertNil(err, "") walker := NewMkShWalker() @@ -270,7 +272,8 @@ func (s *Suite) Test_MkShWalker_Walk__empty_callback(c *check.C) { func (s *Suite) Test_MkShWalker_Walk__assertion(c *check.C) { t := s.Init(c) - list, err := parseShellProgram(dummyLine, "echo \"hello, world\"") + line := t.NewLine("filename.mk", 1, "") + list, err := parseShellProgram(line, "echo \"hello, world\"") assertNil(err, "") walker := NewMkShWalker() diff --git a/pkgtools/pkglint/files/mktypes.go b/pkgtools/pkglint/files/mktypes.go index db6c452d1df..e4044e2760a 100644 --- a/pkgtools/pkglint/files/mktypes.go +++ b/pkgtools/pkglint/files/mktypes.go @@ -43,7 +43,7 @@ func (m MkVarUseModifier) IsSuffixSubst() bool { } func (m MkVarUseModifier) MatchSubst() (ok bool, regex bool, from string, to string, options string) { - p := NewMkParser(nil, m.Text) + p := NewMkLexer(m.Text, nil) return p.varUseModifierSubst('}') } diff --git a/pkgtools/pkglint/files/mktypes_test.go b/pkgtools/pkglint/files/mktypes_test.go index ad2cc690eda..a7c55e53f74 100644 --- a/pkgtools/pkglint/files/mktypes_test.go +++ b/pkgtools/pkglint/files/mktypes_test.go @@ -39,16 +39,6 @@ func (MkTokenBuilder) VarUse(varname string, modifiers ...string) *MkVarUse { return &MkVarUse{varname, mods} } -// AddCommand adds a command directly to a list of commands, -// creating all the intermediate nodes for the syntactic representation. -// As soon as that representation is replaced with a semantic representation, -// this method should no longer be necessary. -func (list *MkShList) AddCommand(command *MkShCommand) *MkShList { - pipeline := NewMkShPipeline(false, []*MkShCommand{command}) - andOr := NewMkShAndOr(pipeline) - return list.AddAndOr(andOr) -} - func (s *Suite) Test_MkVarUseModifier_MatchSubst(c *check.C) { t := s.Init(c) @@ -220,7 +210,7 @@ func (s *Suite) Test_MkVarUse_Mod(c *check.C) { test := func(varUseText string, mod string) { line := t.NewLine("filename.mk", 123, "") - varUse := NewMkParser(line, varUseText).VarUse() + varUse := NewMkLexer(varUseText, line).VarUse() t.CheckOutputEmpty() t.CheckEquals(varUse.Mod(), mod) } diff --git a/pkgtools/pkglint/files/package.go b/pkgtools/pkglint/files/package.go index ec9e3175b97..f1190076569 100644 --- a/pkgtools/pkglint/files/package.go +++ b/pkgtools/pkglint/files/package.go @@ -230,7 +230,7 @@ func (pkg *Package) parse(mklines *MkLines, allLines *MkLines, includingFileForU filename := mklines.lines.Filename if filename.Base() == "buildlink3.mk" { builtin := cleanpath(filename.Dir().JoinNoClean("builtin.mk")) - builtinRel := relpath(pkg.dir, builtin) + builtinRel := G.Pkgsrc.Relpath(pkg.dir, builtin) if pkg.included.FirstTime(builtinRel.String()) && builtin.IsFile() { builtinMkLines := LoadMk(builtin, MustSucceed|LogErrors) pkg.parse(builtinMkLines, allLines, "") @@ -297,7 +297,7 @@ func (pkg *Package) loadIncluded(mkline *MkLine, includingFile Path) (includedMk dirname, _ := includingFile.Split() dirname = cleanpath(dirname) fullIncluded := joinPath(dirname, includedFile) - relIncludedFile := relpath(pkg.dir, fullIncluded) + relIncludedFile := G.Pkgsrc.Relpath(pkg.dir, fullIncluded) if !pkg.shouldDiveInto(includingFile, includedFile) { return nil, true @@ -371,7 +371,7 @@ func (pkg *Package) resolveIncludedFile(mkline *MkLine, includingFilename Path) } if mkline.Basename != "buildlink3.mk" { - if includedFile.HasSuffixText("/buildlink3.mk") { + if includedFile.HasSuffixPath("buildlink3.mk") { pkg.bl3[includedFile] = mkline if trace.Tracing { trace.Step1("Buildlink3 file in package: %q", includedText) @@ -392,11 +392,9 @@ func (*Package) shouldDiveInto(includingFile, includedFile Path) bool { return false } - // FIXME: includingFile may be "../../mk/../devel/readline/buildlink.mk" and thus contain "mk" - // even though the resolved file is not part of the pkgsrc infrastructure. - if includingFile.ContainsPath("mk") && !G.Pkgsrc.ToRel(includingFile).HasPrefixPath("wip/mk") { - // TODO: try ".buildlink.mk", ".builtin.mk" instead, see wip/clfswm. - return includingFile.HasSuffixText("buildlink3.mk") && includedFile.HasSuffixText("builtin.mk") + if G.Pkgsrc.IsInfraMain(includingFile) { + return includingFile.HasSuffixText(".buildlink3.mk") && + includedFile.HasSuffixText(".builtin.mk") } return true @@ -971,7 +969,7 @@ func (pkg *Package) checkUseLanguagesCompilerMk(mklines *MkLines) { } if mkline.Basename == "compiler.mk" { - if relpath(pkg.dir, mkline.Filename) == "../../mk/compiler.mk" { + if G.Pkgsrc.Relpath(pkg.dir, mkline.Filename) == "../../mk/compiler.mk" { return } } @@ -1075,7 +1073,10 @@ func (pkg *Package) nbPart() string { } func (pkg *Package) pkgnameFromDistname(pkgname, distname string) (string, bool) { - tokens := NewMkParser(nil, pkgname).MkTokens() + tokens, rest := NewMkLexer(pkgname, nil).MkTokens() + if rest != "" { + return "", false + } // TODO: Make this resolving of variable references available to all other variables as well. @@ -1206,9 +1207,7 @@ func (pkg *Package) checkDirent(dirent Path, mode os.FileMode) { switch { case mode.IsRegular(): - pkgsrcRel := G.Pkgsrc.ToRel(dirent) - depth := pkgsrcRel.Count() - 1 // FIXME - G.checkReg(dirent, basename, depth) + G.checkReg(dirent, basename, G.Pkgsrc.ToRel(dirent).Count()) case hasPrefix(basename, "work"): if G.Opts.Import { @@ -1377,6 +1376,13 @@ func (pkg *Package) checkIncludeConditionally(mkline *MkLine, indentation *Inden } } + dependingOn := func(varnames []string) string { + if len(varnames) == 0 { + return "" + } + return sprintf(" (depending on %s)", strings.Join(varnames, ", ")) + } + if indentation.IsConditional() { if other := pkg.unconditionalIncludes[key]; other != nil { if !pkg.Once.FirstTimeSlice("checkIncludeConditionally", mkline.Location.String(), other.Location.String()) { @@ -1384,9 +1390,11 @@ func (pkg *Package) checkIncludeConditionally(mkline *MkLine, indentation *Inden } mkline.Warnf( - "%q is included conditionally here (depending on %s) "+ + "%q is included conditionally here%s "+ "and unconditionally in %s.", - cleanpath(mkline.IncludedFile()), strings.Join(mkline.ConditionalVars(), ", "), mkline.RefTo(other)) + cleanpath(mkline.IncludedFile()), + dependingOn(mkline.ConditionalVars()), + mkline.RefTo(other)) explainPkgOptions(other, mkline) } @@ -1399,8 +1407,10 @@ func (pkg *Package) checkIncludeConditionally(mkline *MkLine, indentation *Inden mkline.Warnf( "%q is included unconditionally here "+ - "and conditionally in %s (depending on %s).", - cleanpath(mkline.IncludedFile()), mkline.RefTo(other), strings.Join(other.ConditionalVars(), ", ")) + "and conditionally in %s%s.", + cleanpath(mkline.IncludedFile()), + mkline.RefTo(other), + dependingOn(other.ConditionalVars())) explainPkgOptions(mkline, other) } @@ -1441,7 +1451,7 @@ func (pkg *Package) File(relativeFileName Path) Path { // Example: // NewPackage("category/package").Rel("other/package") == "../../other/package" func (pkg *Package) Rel(filename Path) Path { - return relpath(pkg.dir, filename) + return G.Pkgsrc.Relpath(pkg.dir, filename) } // Returns whether the given file (relative to the package directory) diff --git a/pkgtools/pkglint/files/package_test.go b/pkgtools/pkglint/files/package_test.go index a24048a445a..3dba41feef4 100644 --- a/pkgtools/pkglint/files/package_test.go +++ b/pkgtools/pkglint/files/package_test.go @@ -116,7 +116,7 @@ func (s *Suite) Test_Package__relative_included_filenames_in_same_directory(c *c // TODO: Since other.mk is referenced via "../../category/package", // it would be nice if this relative path would be reflected in the output // instead of referring just to "other.mk". - // This needs to be fixed somewhere near relpath. + // This needs to be fixed somewhere near Relpath. // // The notes are in reverse order because they are produced when checking // other.mk, and there the relative order is correct (line 2 before line 3). @@ -1063,7 +1063,7 @@ func (s *Suite) Test_Package_resolveIncludedFile__skipping(c *check.C) { func (s *Suite) Test_Package_shouldDiveInto(c *check.C) { t := s.Init(c) - t.Chdir(".") + t.Chdir("category/package") test := func(including, included Path, expected bool) { actual := (*Package)(nil).shouldDiveInto(including, included) @@ -2432,6 +2432,15 @@ func (s *Suite) Test_Package_pkgnameFromDistname(c *check.C) { "WARN: ~/category/package/Makefile:4: Invalid variable modifier \"c,d\" for \"DISTNAME\".") test("${DISTFILE:C,\\..*,,}", "aspell-af-0.50-0", "") + + // Parse error because of missing closing brace, parsing succeeds. + test("${DISTNAME:M", "package-1.0", "", + "WARN: ~/category/package/Makefile:4: "+ + "Missing closing \"}\" for \"DISTNAME\".") + + // Parse error with an unparseable rest. + test("$", "package-1.0", "", + nil...) } func (s *Suite) Test_Package_checkPossibleDowngrade(c *check.C) { @@ -2989,6 +2998,45 @@ func (s *Suite) Test_Package_checkIncludeConditionally__no_explanation(c *check. "(depending on OPSYS) and unconditionally in buildlink3.mk:12.") } +func (s *Suite) Test_Package_checkIncludeConditionally__conditionally_no_variable(c *check.C) { + t := s.Init(c) + + t.SetUpOption("zlib", "") + t.SetUpPackage("category/package", + ".include \"../../devel/zlib/buildlink3.mk\"", + ".if exists(/usr/include)", + ".include \"../../sysutils/coreutils/buildlink3.mk\"", + ".endif") + t.CreateFileLines("mk/bsd.options.mk", "") + t.CreateFileLines("devel/zlib/buildlink3.mk", "") + t.CreateFileLines("sysutils/coreutils/buildlink3.mk", "") + + t.CreateFileLines("category/package/options.mk", + MkCvsID, + "", + "PKG_OPTIONS_VAR=\tPKG_OPTIONS.package", + "PKG_SUPPORTED_OPTIONS=\t# none", + "", + ".include \"../../mk/bsd.options.mk\"", + "", + ".if exists(/usr/include)", + ". include \"../../devel/zlib/buildlink3.mk\"", + ".endif", + ".include \"../../sysutils/coreutils/buildlink3.mk\"") + t.Chdir("category/package") + t.FinishSetUp() + + G.checkdirPackage(".") + + t.CheckOutputLines( + "WARN: Makefile:20: \"../../devel/zlib/buildlink3.mk\" "+ + "is included unconditionally here "+ + "and conditionally in options.mk:9.", + "WARN: Makefile:22: \"../../sysutils/coreutils/buildlink3.mk\" "+ + "is included conditionally here "+ + "and unconditionally in options.mk:11.") +} + func (s *Suite) Test_Package_checkIncludeConditionally__explain_PKG_OPTIONS_in_options_mk(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/path.go b/pkgtools/pkglint/files/path.go index 746c5572fc7..4824cc68d1e 100644 --- a/pkgtools/pkglint/files/path.go +++ b/pkgtools/pkglint/files/path.go @@ -13,6 +13,12 @@ import ( // Some paths may contain placeholders like @VAR@ or ${VAR}. // The base directory of relative paths depends on the context // in which the path is used. +// +// TODO: Consider adding several more specialized types of path: +// TODO: CurrPath, relative to the current working directory +// TODO: PkgsrcPath, relative to the pkgsrc root +// TODO: PackagePath, relative to the package directory +// TODO: RelativePath, relative to some other basedir type Path string func NewPath(name string) Path { return Path(name) } @@ -32,10 +38,33 @@ func (p Path) Split() (dir Path, base string) { return Path(strDir), strBase } +// Parts splits the path into its components. +// Multiple adjacent slashes are treated like a single slash. +// Parts that are single dots are skipped. +// Absolute paths have an empty string as its first part. +// All other parts are nonempty. func (p Path) Parts() []string { - return strings.FieldsFunc(string(p), func(r rune) bool { return r == '/' }) + if p == "" { + return nil + } + + parts := strings.Split(string(p), "/") + j := 0 + for i, part := range parts { + if (i == 0 || part != "") && part != "." { + parts[j] = part + j++ + } + } + parts = parts[:j] + if len(parts) == 0 { + return []string{"."} + } + return parts } +// Count returns the number of meaningful parts of the path. +// See Parts. func (p Path) Count() int { return len(p.Parts()) } func (p Path) HasPrefixText(prefix string) bool { @@ -45,8 +74,26 @@ func (p Path) HasPrefixText(prefix string) bool { // HasPrefixPath returns whether the path starts with the given prefix. // The basic unit of comparison is a path component, not a character. func (p Path) HasPrefixPath(prefix Path) bool { - return hasPrefix(string(p), string(prefix)) && - (len(p) == len(prefix) || p[len(prefix)] == '/') + // Handle the simple case first, without any memory allocations. + if hasPrefix(string(p), string(prefix)) { + return len(p) == len(prefix) || p[len(prefix)] == '/' + } + + if prefix == "." { + return !p.IsAbs() + } + + parts := p.Parts() + prefixParts := prefix.Parts() + if len(prefixParts) > len(parts) { + return false + } + for i, prefixPart := range prefixParts { + if parts[i] != prefixPart { + return false + } + } + return true } // TODO: Check each call whether ContainsPath is more appropriate; add tests @@ -75,7 +122,6 @@ func (p Path) ContainsPathCanonical(sub Path) bool { return cleaned.ContainsPath(sub) } -// TODO: Check each call whether HasSuffixPath is more appropriate; add tests func (p Path) HasSuffixText(suffix string) bool { return hasSuffix(string(p), suffix) } @@ -107,15 +153,35 @@ func (p Path) JoinNoClean(s Path) Path { func (p Path) Clean() Path { return NewPath(path.Clean(string(p))) } +// CleanDot returns the path with single dots removed and double slashes +// collapsed. +func (p Path) CleanDot() Path { + if !p.ContainsText(".") { + return p + } + + var parts []string + for i, part := range p.Parts() { + if !(part == "." || i > 0 && part == "") { // See Parts + parts = append(parts, part) + } + } + if len(parts) == 0 { + return "." + } + return NewPath(strings.Join(parts, "/")) +} + func (p Path) IsAbs() bool { return p.HasPrefixText("/") || filepath.IsAbs(filepath.FromSlash(string(p))) } +// Rel returns the relative path from this path to the other. func (p Path) Rel(other Path) Path { fp := filepath.FromSlash(p.String()) fpOther := filepath.FromSlash(other.String()) rel, err := filepath.Rel(fp, fpOther) - assertNil(err, "relpath from %q to %q", p, other) + assertNil(err, "Relpath from %q to %q", p, other) return NewPath(filepath.ToSlash(rel)) } diff --git a/pkgtools/pkglint/files/path_test.go b/pkgtools/pkglint/files/path_test.go index 51b9aec1232..d766010a35d 100644 --- a/pkgtools/pkglint/files/path_test.go +++ b/pkgtools/pkglint/files/path_test.go @@ -104,11 +104,32 @@ func (s *Suite) Test_Path_Parts(c *check.C) { t.CheckDeepEquals(NewPath(p).Parts(), parts) } - test("", []string{}...) - test("././././", ".", ".", ".", ".") // No trailing "" - test("/root", "root") // No leading "" - test("filename", "filename") - test("dir/filename", "dir", "filename") + // Only the empty path returns an empty slice. + test("", nil...) + + // The standard cases for relative paths. + test("relative", "relative") + test("relative/subdir", "relative", "subdir") + test("relative////subdir", "relative", "subdir") + test("relative/..", "relative", "..") + test("relative/.", "relative") + + // Leading dots are removed when they are followed by something. + test("./relative", "relative") + + // A path consisting of only dots produces a single dot. + test("./././.", ".") + + // Slashes at the end are treated like a single dot. + test("././././", ".") + test(".///////", ".") + + // Absolute paths have an empty first component. + test("/", "") + test("/.", "") + test("/root", "", "root") + + // The backslash is not a path separator. test("dir/filename\\with\\backslash", "dir", "filename\\with\\backslash") } @@ -119,12 +140,36 @@ func (s *Suite) Test_Path_Count(c *check.C) { t.CheckEquals(NewPath(p).Count(), count) } - test("", 0) // FIXME - test("././././", 4) - test("/root", 1) // FIXME + test("././././", 1) + test("/root", 2) test("filename", 1) test("dir/filename", 2) test("dir/filename\\with\\backslash", 2) + + // Only the empty path returns an empty slice. + test("", 0) + + // The standard cases for canonical relative paths. + test("relative", 1) + test("relative/subdir", 2) + test("relative////subdir", 2) + test("relative/..", 2) + test("relative/.", 1) + + // A path consisting of only dots produces a single dot. + test("./././.", 1) + + // Slashes at the end are treated like a single dot. + test("././././", 1) + test(".///////", 1) + + // Absolute paths have an empty first component. + test("/", 1) + test("/.", 1) + test("/root", 2) + + // The backslash is not a path separator. + test("dir/filename\\with\\backslash", 2) } func (s *Suite) Test_Path_HasPrefixText(c *check.C) { @@ -155,9 +200,21 @@ func (s *Suite) Test_Path_HasPrefixPath(c *check.C) { test("", "x", false) test("/root", "/r", false) test("/root", "/root", true) - test("/root", "/root/", false) + + // Even though the textual representation of the prefix is longer than + // the path. The trailing slash marks the path as a directory, and + // there are only a few cases where the difference matters, such as + // in rsync and mkdir. + test("/root", "/root/", true) + test("/root/", "/root", true) + test("/root/", "root", false) test("/root/subdir", "/root", true) + test("filename", ".", true) + test("filename", "./filename", true) + test("filename", "./file", false) + test("filename", "./filename/sub", false) + test("/anything", ".", false) } func (s *Suite) Test_Path_ContainsText(c *check.C) { @@ -343,6 +400,23 @@ func (s *Suite) Test_Path_Clean(c *check.C) { test("a/bb///../c", "a/c") } +func (s *Suite) Test_Path_CleanDot(c *check.C) { + t := s.Init(c) + + test := func(p, result Path) { + t.CheckEquals(p.CleanDot(), result) + } + + test("", "") + test(".", ".") + test("./././", ".") + test("a/bb///../c", "a/bb/../c") + test("./filename", "filename") + test("/absolute", "/absolute") + test("/usr/pkgsrc/wip/package", "/usr/pkgsrc/wip/package") + test("/usr/pkgsrc/wip/package/../mk/git-package.mk", "/usr/pkgsrc/wip/package/../mk/git-package.mk") +} + func (s *Suite) Test_Path_IsAbs(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/pkglint.1 b/pkgtools/pkglint/files/pkglint.1 index 88839eaf136..0c69c966a76 100644 --- a/pkgtools/pkglint/files/pkglint.1 +++ b/pkgtools/pkglint/files/pkglint.1 @@ -1,4 +1,4 @@ -.\" $NetBSD: pkglint.1,v 1.56 2019/06/30 20:56:19 rillig Exp $ +.\" $NetBSD: pkglint.1,v 1.57 2019/11/27 22:10:07 rillig Exp $ .\" From FreeBSD: portlint.1,v 1.8 1997/11/25 14:53:14 itojun Exp .\" .\" Copyright (c) 1997 by Jun-ichiro Itoh <itojun@itojun.org>. @@ -8,7 +8,7 @@ .\" Thomas Klausner <wiz@NetBSD.org>, 2012. .\" Roland Illig <rillig@NetBSD.org>, 2015-2019. .\" -.Dd June 17, 2019 +.Dd November 27, 2019 .Dt PKGLINT 1 .Os .Sh NAME diff --git a/pkgtools/pkglint/files/pkglint.go b/pkgtools/pkglint/files/pkglint.go index 8a4ff984465..e612bb6a8e8 100644 --- a/pkgtools/pkglint/files/pkglint.go +++ b/pkgtools/pkglint/files/pkglint.go @@ -43,7 +43,7 @@ type Pkglint struct { interner StringInterner // cwd is the absolute path to the current working - // directory. It is used for speeding up relpath and abspath. + // directory. It is used for speeding up Relpath and abspath. // There is no other use for it. cwd Path @@ -106,7 +106,7 @@ type pkglintFatal struct{} // G is the abbreviation for "global state"; // this and the tracer are the only global variables in this Go package. var ( - G = NewPkglint(nil, nil) + G = NewPkglint(os.Stdout, os.Stderr) trace tracePkg.Tracer ) @@ -179,7 +179,8 @@ func (pkglint *Pkglint) setUpProfiling() func() { f, err := os.Create("pkglint.pprof") if err != nil { - dummyLine.Fatalf("Cannot create profiling file: %s", err) + pkglint.Logger.TechErrorf("pkglint.pprof", "Cannot create profiling file: %s", err) + panic(pkglintFatal{}) } atExit(func() { assertNil(f.Close(), "") }) @@ -217,11 +218,11 @@ func (pkglint *Pkglint) prepareMainLoop() { // pkglint doesn't know where to load the infrastructure files from, // and these are needed for virtually every single check. // Therefore, the only sensible thing to do is to quit immediately. - dummyLine.Fatalf("%q must be inside a pkgsrc tree.", firstDir) + NewLineWhole(firstDir).Fatalf("Must be inside a pkgsrc tree.") } pkglint.Pkgsrc = NewPkgsrc(joinPath(firstDir, relTopdir)) - pkglint.Wip = pkglint.Pkgsrc.ToRel(firstDir).HasPrefixPath("wip") // Same as in Pkglint.Check. + pkglint.Wip = pkglint.Pkgsrc.IsWip(firstDir) // See Pkglint.checkMode. pkglint.Pkgsrc.LoadInfrastructure() currentUser, err := user.Current() @@ -244,7 +245,6 @@ func (pkglint *Pkglint) ParseCommandLine(args []string) int { opts.AddFlagVar('h', "help", &gopts.ShowHelp, false, "show a detailed usage message") opts.AddFlagVar('I', "dumpmakefile", &gopts.DumpMakefile, false, "dump the Makefile after parsing") opts.AddFlagVar('i', "import", &gopts.Import, false, "prepare the import of a wip package") - opts.AddFlagVar('m', "log-verbose", &lopts.LogVerbose, false, "allow the same diagnostic more than once") opts.AddStrList('o', "only", &gopts.LogOnly, "only log diagnostics containing the given text") opts.AddFlagVar('p', "profiling", &gopts.Profiling, false, "profile the executing program") opts.AddFlagVar('q', "quiet", &lopts.Quiet, false, "don't show a summary line when finishing") @@ -338,9 +338,8 @@ func (pkglint *Pkglint) checkMode(dirent Path, mode os.FileMode) { } if isReg { - depth := pkgsrcRel.Count() - 1 // FIXME pkglint.checkExecutable(dirent, mode) - pkglint.checkReg(dirent, basename, depth) + pkglint.checkReg(dirent, basename, pkgsrcRel.Count()) return } @@ -448,7 +447,8 @@ func CheckLinesDescr(lines *Lines) { ck.CheckValidCharacters() if containsVarRef(line.Text) { - for _, token := range NewMkParser(nil, line.Text).MkTokens() { + tokens, _ := NewMkLexer(line.Text, nil).MkTokens() + for _, token := range tokens { if token.Varuse != nil && G.Pkgsrc.VariableType(nil, token.Varuse.varname) != nil { line.Notef("Variables are not expanded in the DESCR file.") } @@ -546,9 +546,13 @@ func CheckFileMk(filename Path) { mklines.SaveAutofixChanges() } +// 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 Path, basename string, depth int) { - if depth == 2 && !pkglint.Wip { + if depth == 3 && !pkglint.Wip { + // FIXME: There's no good reason for prohibiting a README file. if contains(basename, "README") || contains(basename, "TODO") { NewLineWhole(filename).Errorf("Packages in main pkgsrc must not have a %s file.", basename) // TODO: Add a convincing explanation. @@ -560,8 +564,8 @@ func (pkglint *Pkglint) checkReg(filename Path, basename string, depth int) { case hasSuffix(basename, "~"), hasSuffix(basename, ".orig"), hasSuffix(basename, ".rej"), - contains(basename, "README") && depth == 2, - contains(basename, "TODO") && depth == 2: + contains(basename, "README") && depth == 3, + contains(basename, "TODO") && depth == 3: if pkglint.Opts.Import { NewLineWhole(filename).Errorf("Must be cleaned up before committing the package.") } @@ -605,7 +609,7 @@ func (pkglint *Pkglint) checkReg(filename Path, basename string, depth int) { CheckLinesPatch(lines) } - case matches(filename.String(), `(?:^|/)patches/manual[^/]*$`): + case filename.Dir().Base() == "patches" && matches(filename.Base(), `^manual[^/]*$`): if trace.Tracing { trace.Stepf("Unchecked file %q.", filename) } @@ -677,7 +681,7 @@ func (pkglint *Pkglint) checkExecutable(filename Path, mode os.FileMode) { fix.Describef(0, "Clearing executable bits") if autofix { if err := filename.Chmod(mode &^ 0111); err != nil { - G.Logger.Errorf(cleanpath(filename), "Cannot clear executable bits: %s", err) + G.Logger.TechErrorf(cleanpath(filename), "Cannot clear executable bits: %s", err) } } }) diff --git a/pkgtools/pkglint/files/pkglint_test.go b/pkgtools/pkglint/files/pkglint_test.go index ee2d6a87e48..f24cae3c12f 100644 --- a/pkgtools/pkglint/files/pkglint_test.go +++ b/pkgtools/pkglint/files/pkglint_test.go @@ -58,7 +58,6 @@ func (s *Suite) Test_Pkglint_Main__help(c *check.C) { " -h, --help show a detailed usage message", " -I, --dumpmakefile dump the Makefile after parsing", " -i, --import prepare the import of a wip package", - " -m, --log-verbose allow the same diagnostic more than once", " -o, --only only log diagnostics containing the given text", " -p, --profiling profile the executing program", " -q, --quiet don't show a summary line when finishing", @@ -102,7 +101,7 @@ func (s *Suite) Test_Pkglint_Main__no_args(c *check.C) { // The "." from the error message is the implicit argument added in Pkglint.Main. t.CheckEquals(exitcode, 1) t.CheckOutputLines( - "FATAL: \".\" must be inside a pkgsrc tree.") + "FATAL: .: Must be inside a pkgsrc tree.") } func (s *Suite) Test_Pkglint_Main__unknown_option(c *check.C) { @@ -331,7 +330,19 @@ func (s *Suite) Test_Pkglint_Main__profiling_error(c *check.C) { t.CheckEquals(exitcode, 1) t.CheckOutputMatches( - `FATAL: Cannot create profiling file: open pkglint\.pprof: .*`) + `ERROR: pkglint\.pprof: Cannot create profiling file: open pkglint\.pprof: .*`) +} + +// Branch coverage for Logger.Logf, the level != Fatal case. +func (s *Suite) Test_Pkglint_prepareMainLoop__fatal(c *check.C) { + t := s.Init(c) + + t.Chdir(".") + t.Main("--profiling", t.File("does-not-exist").String()) + + t.CheckOutputLines( + "fileCache: 0 hits, 0 misses", + "FATAL: does-not-exist: Must be inside a pkgsrc tree.") } func (s *Suite) Test_Pkglint_ParseCommandLine__only(c *check.C) { @@ -874,12 +885,12 @@ func (s *Suite) Test_Pkglint_checkReg__file_not_found(c *check.C) { t.Chdir(".") - G.checkReg("buildlink3.mk", "buildlink3.mk", 2) - G.checkReg("DESCR", "DESCR", 2) - G.checkReg("distinfo", "distinfo", 2) - G.checkReg("MESSAGE", "MESSAGE", 2) - G.checkReg("patches/patch-aa", "patch-aa", 2) - G.checkReg("PLIST", "PLIST", 2) + 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) t.CheckOutputLines( "ERROR: buildlink3.mk: Cannot be read.", @@ -897,7 +908,7 @@ func (s *Suite) Test_Pkglint_checkReg__no_tracing(c *check.C) { t.Chdir(".") t.DisableTracing() - G.checkReg("patches/manual-aa", "manual-aa", 2) + G.checkReg("patches/manual-aa", "manual-aa", 4) t.CheckOutputEmpty() } @@ -1001,7 +1012,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", 3) + G.checkReg(t.File("category/Makefile/patches/index"), "index", 4) t.CheckOutputLines( "WARN: ~/category/Makefile/patches/index: " + @@ -1014,7 +1025,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", 3) + G.checkReg(t.File("patches/patch-compiler.mk"), "patch-compiler.mk", 4) t.CheckOutputEmpty() } @@ -1024,7 +1035,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", 3) + G.checkReg(t.File("category/package/files/index"), "index", 4) // These files are ignored since they could contain anything. t.CheckOutputEmpty() @@ -1036,8 +1047,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", 2) - G.checkReg(t.File("regress/package/spec"), "spec", 2) + G.checkReg(t.File("category/package/spec"), "spec", 3) + G.checkReg(t.File("regress/package/spec"), "spec", 3) t.CheckOutputLines( "WARN: ~/category/package/spec: Only packages in regress/ may have spec files.") @@ -1047,7 +1058,7 @@ func (s *Suite) Test_Pkglint_checkExecutable(c *check.C) { t := s.Init(c) filename := t.CreateFileLines("file.mk") - err := os.Chmod(filename.String(), 0555) + err := filename.Chmod(0555) assertNil(err, "") G.checkExecutable(filename, 0555) diff --git a/pkgtools/pkglint/files/pkgsrc.go b/pkgtools/pkglint/files/pkgsrc.go index 3e3dd1e3a5c..2c446ed765f 100644 --- a/pkgtools/pkglint/files/pkgsrc.go +++ b/pkgtools/pkglint/files/pkgsrc.go @@ -813,7 +813,7 @@ func (src *Pkgsrc) ListVersions(category Path, re regex.Pattern, repl string, er } if len(names) == 0 { if errorIfEmpty { - dummyLine.Errorf("Cannot find package versions of %q in %q.", re, src.File(category)) + NewLineWhole(src.File(category)).Errorf("Cannot find package versions of %q.", re) } src.listVersions[cacheKey] = nil return nil @@ -1035,13 +1035,92 @@ func (src *Pkgsrc) Load(filename Path, options LoadOptions) *Lines { return Load(src.File(filename), options) } +// Relpath returns the canonical relative path from the directory "from" +// to the filesystem entry "to". +// +// The relative path is built by going from the "from" directory up to the +// pkgsrc root and from there to the "to" filename. This produces the form +// "../../category/package" that is found in DEPENDS and .include lines. +// +// Both from and to are interpreted relative to the current working directory, +// unless they are absolute paths. +// +// This function should only be used if the relative path from one file to +// another cannot be computed in another way. The preferred way is to take +// the relative filenames directly from the .include or exists() where they +// appear. +// +// TODO: Invent data types for all kinds of relative paths that occur in pkgsrc +// and pkglint. Make sure that these paths cannot be accidentally mixed. +func (src *Pkgsrc) Relpath(from, to Path) (result Path) { + cfrom := from.Clean() + cto := to.Clean() + + if cfrom == cto { + return "." + } + + // Take a shortcut for the common case from "dir" to "dir/subdir/...". + if cto.HasPrefixPath(cfrom) { + rel := cfrom.Rel(cto) + if !rel.HasPrefixPath("..") { + return rel + } + } + + // Take a shortcut for the common case from "category/package" to ".". + // This is the most common variant in a complete pkgsrc scan. + if cto == "." { + fromParts := cfrom.Parts() + if len(fromParts) == 2 && fromParts[0] != ".." && fromParts[1] != ".." { + return "../.." + } + } + + if cfrom == "." && !cto.IsAbs() { + return cto.Clean() + } + + absFrom := abspath(cfrom) + absTopdir := abspath(src.topdir) + absTo := abspath(cto) + + up := absFrom.Rel(absTopdir) + down := absTopdir.Rel(absTo) + + if absFrom.HasPrefixPath(absTo) || absTo.HasPrefixPath(absFrom) { + return absFrom.Rel(absTo) + } + + fromParts := absTopdir.Rel(absFrom).Parts() + toParts := down.Parts() + + if len(fromParts) >= 2 && len(toParts) >= 2 { + if fromParts[0] == toParts[0] && fromParts[1] == toParts[1] { + var relParts []string + for _ = range fromParts[2:] { + relParts = append(relParts, "..") + } + relParts = append(relParts, toParts[2:]...) + return NewPath(strings.Join(relParts, "/")).CleanDot() + } + } + + result = up.JoinNoClean(down).CleanDot() + return +} + // File resolves a filename relative to the pkgsrc top directory. // // Example: // NewPkgsrc("/usr/pkgsrc").File("distfiles") => "/usr/pkgsrc/distfiles" func (src *Pkgsrc) File(relativeName Path) Path { + cleaned := relativeName.Clean() + if cleaned == "." { + return src.topdir.CleanDot() + } // TODO: Package.File resolves variables, Pkgsrc.File doesn't. They should behave the same. - return cleanpath(joinPath(src.topdir, relativeName)) + return src.topdir.JoinNoClean(cleaned).CleanDot() } // ToRel returns the path of `filename`, relative to the pkgsrc top directory. @@ -1049,16 +1128,26 @@ func (src *Pkgsrc) File(relativeName Path) Path { // Example: // NewPkgsrc("/usr/pkgsrc").ToRel("/usr/pkgsrc/distfiles") => "distfiles" func (src *Pkgsrc) ToRel(filename Path) Path { - return relpath(src.topdir, filename) + return src.Relpath(src.topdir, filename) } -// IsInfra returns whether the given filename (relative to the pkglint +// IsInfra returns whether the given filename (relative to the current // working directory) is part of the pkgsrc infrastructure. func (src *Pkgsrc) IsInfra(filename Path) bool { rel := src.ToRel(filename) return rel.HasPrefixPath("mk") || rel.HasPrefixPath("wip/mk") } +func (src *Pkgsrc) IsInfraMain(filename Path) bool { + rel := src.ToRel(filename) + return rel.HasPrefixPath("mk") +} + +func (src *Pkgsrc) IsWip(filename Path) bool { + rel := src.ToRel(filename) + return rel.HasPrefixPath("wip") +} + // Change describes a modification to a single package, from the doc/CHANGES-* files. type Change struct { Location Location diff --git a/pkgtools/pkglint/files/pkgsrc_test.go b/pkgtools/pkglint/files/pkgsrc_test.go index ed1073ff32c..88fb5f434a7 100644 --- a/pkgtools/pkglint/files/pkgsrc_test.go +++ b/pkgtools/pkglint/files/pkgsrc_test.go @@ -1,6 +1,10 @@ package pkglint -import "gopkg.in/check.v1" +import ( + "gopkg.in/check.v1" + "os" + "path/filepath" +) func (s *Suite) Test_Pkgsrc__frozen(c *check.C) { t := s.Init(c) @@ -827,7 +831,7 @@ func (s *Suite) Test_Pkgsrc_Latest__not_found(c *check.C) { t.CheckEquals(latest, "") t.CheckOutputLines( - "ERROR: Cannot find package versions of \"^python[0-9]+$\" in \"~/lang\".") + "ERROR: ~/lang: Cannot find package versions of \"^python[0-9]+$\".") } // In 2017, PostgreSQL changed their versioning scheme to SemVer, @@ -956,7 +960,7 @@ func (s *Suite) Test_Pkgsrc_ListVersions__no_basedir(c *check.C) { c.Check(versions, check.HasLen, 0) t.CheckOutputLines( - "ERROR: Cannot find package versions of \"^python[0-9]+$\" in \"~/lang\".") + "ERROR: ~/lang: Cannot find package versions of \"^python[0-9]+$\".") } func (s *Suite) Test_Pkgsrc_ListVersions__no_subdirs(c *check.C) { @@ -968,7 +972,7 @@ func (s *Suite) Test_Pkgsrc_ListVersions__no_subdirs(c *check.C) { c.Check(versions, check.HasLen, 0) t.CheckOutputLines( - "ERROR: Cannot find package versions of \"^python[0-9]+$\" in \"~/lang\".") + "ERROR: ~/lang: Cannot find package versions of \"^python[0-9]+$\".") } // Ensures that failed lookups are also cached since they can be assumed @@ -980,7 +984,7 @@ func (s *Suite) Test_Pkgsrc_ListVersions__error_is_cached(c *check.C) { c.Check(versions, check.HasLen, 0) t.CheckOutputLines( - "ERROR: Cannot find package versions of \"^python[0-9]+$\" in \"~/lang\".") + "ERROR: ~/lang: Cannot find package versions of \"^python[0-9]+$\".") versions2 := G.Pkgsrc.ListVersions("lang", `^python[0-9]+$`, "../../lang/$0", true) @@ -1143,7 +1147,7 @@ func (s *Suite) Test_Pkgsrc_checkToplevelUnusedLicenses(c *check.C) { t.Main("-r", "-Cglobal", ".") t.CheckOutputLines( - "WARN: ~/category/package2/Makefile:11: License file ~/licenses/missing does not exist.", + "WARN: ~/category/package2/Makefile:11: License file ../../licenses/missing does not exist.", "WARN: ~/licenses/gnu-gpl-v2: This license seems to be unused.", // Added by Tester.SetUpPkgsrc "WARN: ~/licenses/gnu-gpl-v3: This license seems to be unused.", "3 warnings found.") @@ -1169,6 +1173,147 @@ func (s *Suite) Test_Pkgsrc_ReadDir(c *check.C) { t.CheckDeepEquals(names, []string{"aaa-subdir", "file", "subdir"}) } +func (s *Suite) Test_Pkgsrc_Relpath(c *check.C) { + t := s.Init(c) + + t.Chdir(".") + t.CheckEquals(G.Pkgsrc.topdir, t.tmpdir) + + test := func(from, to Path, result Path) { + t.CheckEquals(G.Pkgsrc.Relpath(from, to), result) + } + + // TODO: add tests going from each of (top, cat, pkg, pkgsub) to the others + + test("some/dir", "some/directory", "../../some/directory") + test("some/directory", "some/dir", "../../some/dir") + + test("category/package/.", ".", "../..") + + // This case is handled by one of the shortcuts that avoid file system access. + test( + "./.", + "x11/frameworkintegration/../../meta-pkgs/kde/kf5.mk", + "meta-pkgs/kde/kf5.mk") + + test(".hidden/dir", ".", "../..") + test("dir/.hidden", ".", "../..") + + // This happens when "pkglint -r x11" is run. + G.Pkgsrc.topdir = "x11/.." + + test( + "./.", + "x11/frameworkintegration/../../meta-pkgs/kde/kf5.mk", + "meta-pkgs/kde/kf5.mk") + test( + "x11/..", + "x11/frameworkintegration/../../meta-pkgs/kde/kf5.mk", + "meta-pkgs/kde/kf5.mk") + + volume := NewPathSlash(filepath.VolumeName(t.tmpdir.String())) + G.Pkgsrc.topdir = volume.JoinNoClean("usr/pkgsrc") + + // Taken from Test_MkLineChecker_CheckRelativePath__wip_mk + test( + G.Pkgsrc.File("wip/package"), + G.Pkgsrc.File("wip/package/../mk/git-package.mk"), + "../../wip/mk/git-package.mk") + + // Taken from Test_Package_parse__relative + test( + G.Pkgsrc.File("category/package"), + G.Pkgsrc.File("category/package/../package/extra.mk"), + "extra.mk") + + // Make sure that .. in later positions is resolved correctly as well. + test( + G.Pkgsrc.File("category/package"), + G.Pkgsrc.File("category/package/sub/../../package/extra.mk"), + "extra.mk") + + G.Pkgsrc.topdir = t.tmpdir + + test("some/dir", "some/dir/../..", "../..") + test("some/dir", "some/dir/./././../..", "../..") + test("some/dir", "some/dir/", ".") + + test("some/dir", ".", "../..") + test("some/dir/.", ".", "../..") + + chdir := func(path Path) { + // See Tester.Chdir; a direct Chdir works here since this test + // neither loads lines nor processes them. + assertNil(os.Chdir(path.String()), "Chdir %s", path) + G.cwd = abspath(path) + } + + t.CreateFileLines("testdir/subdir/dummy") + chdir("testdir/subdir") + + test(".", ".", ".") + test("./.", "./dir", "dir") + + test("dir", ".", "..") + test("dir", "dir", ".") + test("dir", "dir/file", "file") + test("dir", "dir/..", "..") + + test(".", "../../other/package", "../../other/package") + + // Even though this path could be shortened to "../package", + // in pkgsrc the convention is to always go from a package + // directory up to the root and then back to the other package + // directory. + test(".", "../../testdir/package", "../../testdir/package") + + chdir("..") + + // When pkglint is run from a category directory to test + // the complete pkgsrc. + test("..", "../other/package", "other/package") + + chdir(t.tmpdir.JoinNoClean("..")) + + test( + "pkgsrc/category/package", + "pkgsrc/category/package/../../other/package", + "../../other/package") + + test( + "pkgsrc/category/package", + "pkgsrc/category/package/../../category/other", + "../../category/other") + + chdir(t.tmpdir.JoinNoClean("testdir").JoinNoClean("subdir")) + + test("..", ".", "subdir") + test("../..", ".", "testdir/subdir") + test("../../", ".", "testdir/subdir") +} + +func (s *Suite) Test_Pkgsrc_File(c *check.C) { + t := s.Init(c) + + G.Pkgsrc.topdir = "$pkgsrcdir" + + test := func(rel, resolved Path) { + t.CheckEquals(G.Pkgsrc.File(rel), resolved) + } + + test(".", "$pkgsrcdir") + test("category", "$pkgsrcdir/category") + + test( + "category/package/../../mk/bsd.prefs.mk", + "$pkgsrcdir/mk/bsd.prefs.mk") + + G.Pkgsrc.topdir = "." + + test(".", ".") + test("filename", "filename") +} + func (s *Suite) Test_Change_Version(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/pkgver/vercmp_test.go b/pkgtools/pkglint/files/pkgver/vercmp_test.go index 2717d69a22b..a1b725a764a 100644 --- a/pkgtools/pkglint/files/pkgver/vercmp_test.go +++ b/pkgtools/pkglint/files/pkgver/vercmp_test.go @@ -93,8 +93,8 @@ func (s *Suite) Test_newVersion(c *check.C) { &version{[]int{3, 0, 5, 0, 4, 5, 22, 1710}, 0}) } -func (s *Suite) Test__test_names(c *check.C) { - ck := intqa.NewTestNameChecker(c.Errorf) +func (s *Suite) Test__qa(c *check.C) { + ck := intqa.NewQAChecker(c.Errorf) ck.Configure("*", "*", "*", -intqa.EMissingTest) ck.Check() } diff --git a/pkgtools/pkglint/files/plist_test.go b/pkgtools/pkglint/files/plist_test.go index 43cc041c3b9..146e55a9fda 100644 --- a/pkgtools/pkglint/files/plist_test.go +++ b/pkgtools/pkglint/files/plist_test.go @@ -707,7 +707,7 @@ func (s *Suite) Test_PlistChecker_checkPathShareIcons__using_gnome_icon_theme(c // This variant is typical for recursive runs of pkglint. G.Check("./graphics/gnome-icon-theme-extras") - // Up to March 2019, a bug in relpath produced different behavior + // Up to March 2019, a bug in Relpath produced different behavior // depending on the leading dot. t.CheckOutputEmpty() } diff --git a/pkgtools/pkglint/files/shell.go b/pkgtools/pkglint/files/shell.go index fd4f1c39f7a..98d3db152ed 100644 --- a/pkgtools/pkglint/files/shell.go +++ b/pkgtools/pkglint/files/shell.go @@ -116,7 +116,7 @@ func (scc *SimpleCommandChecker) handleCommandVariable() bool { } shellword := scc.strcmd.Name - varuse := NewMkParser(nil, shellword).VarUse() + varuse := NewMkLexer(shellword, nil).VarUse() if varuse == nil { return false } diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go index 36dc8a06534..956c9f2d909 100644 --- a/pkgtools/pkglint/files/shell_test.go +++ b/pkgtools/pkglint/files/shell_test.go @@ -838,7 +838,7 @@ func (s *Suite) Test_ShellLineChecker_CheckShellCommandLine__implementation(c *c // foobar="`echo \"foo bar\"`" text := "foobar=\"`echo \\\"foo bar\\\"`\"" - tokens, rest := splitIntoShellTokens(dummyLine, text) + tokens, rest := splitIntoShellTokens(ck.mkline.Line, text) t.CheckDeepEquals(tokens, []string{text}) t.CheckEquals(rest, "") @@ -907,13 +907,17 @@ func (s *Suite) Test_ShellLineChecker_CheckShellCommandLine__shell_variables(c * ck.CheckShellCommandLine(text) t.CheckOutputLines( + // TODO: Avoid these duplicate diagnostics. "WARN: Makefile:3: $f is ambiguous. Use ${f} if you mean a Make variable or $$f if you mean a shell variable.", "WARN: Makefile:3: $f is ambiguous. Use ${f} if you mean a Make variable or $$f if you mean a shell variable.", "WARN: Makefile:3: $f is ambiguous. Use ${f} if you mean a Make variable or $$f if you mean a shell variable.", "WARN: Makefile:3: $f is ambiguous. Use ${f} if you mean a Make variable or $$f if you mean a shell variable.", "NOTE: Makefile:3: Please use the SUBST framework instead of ${SED} and ${MV}.", + "WARN: Makefile:3: $f is ambiguous. Use ${f} if you mean a Make variable or $$f if you mean a shell variable.", + "WARN: Makefile:3: $f is ambiguous. Use ${f} if you mean a Make variable or $$f if you mean a shell variable.", + "WARN: Makefile:3: $f is ambiguous. Use ${f} if you mean a Make variable or $$f if you mean a shell variable.", + "WARN: Makefile:3: $f is ambiguous. Use ${f} if you mean a Make variable or $$f if you mean a shell variable.", "WARN: Makefile:3: f is used but not defined.", - // TODO: Avoid these duplicate diagnostics. "WARN: Makefile:3: $f is ambiguous. Use ${f} if you mean a Make variable or $$f if you mean a shell variable.", "WARN: Makefile:3: $f is ambiguous. Use ${f} if you mean a Make variable or $$f if you mean a shell variable.") @@ -1557,19 +1561,21 @@ func (s *Suite) Test_ShellLineChecker_checkInstallCommand(c *check.C) { func (s *Suite) Test_splitIntoShellTokens__line_continuation(c *check.C) { t := s.Init(c) - words, rest := splitIntoShellTokens(dummyLine, "if true; then \\") + line := t.NewLine("filename.mk", 1, "") + words, rest := splitIntoShellTokens(line, "if true; then \\") t.CheckDeepEquals(words, []string{"if", "true", ";", "then"}) t.CheckEquals(rest, "\\") t.CheckOutputLines( - "WARN: Internal pkglint error in ShTokenizer.ShAtom at \"\\\\\" (quoting=plain).") + "WARN: filename.mk:1: Internal pkglint error in ShTokenizer.ShAtom at \"\\\\\" (quoting=plain).") } func (s *Suite) Test_splitIntoShellTokens__dollar_slash(c *check.C) { t := s.Init(c) - words, rest := splitIntoShellTokens(dummyLine, "pax -s /.*~$$//g") + line := t.NewLine("filename.mk", 1, "") + words, rest := splitIntoShellTokens(line, "pax -s /.*~$$//g") t.CheckDeepEquals(words, []string{"pax", "-s", "/.*~$$//g"}) t.CheckEquals(rest, "") @@ -1578,7 +1584,8 @@ func (s *Suite) Test_splitIntoShellTokens__dollar_slash(c *check.C) { func (s *Suite) Test_splitIntoShellTokens__dollar_subshell(c *check.C) { t := s.Init(c) - words, rest := splitIntoShellTokens(dummyLine, "id=$$(${AWK} '{print}' < ${WRKSRC}/idfile) && echo \"$$id\"") + line := t.NewLine("filename.mk", 1, "") + words, rest := splitIntoShellTokens(line, "id=$$(${AWK} '{print}' < ${WRKSRC}/idfile) && echo \"$$id\"") t.CheckDeepEquals(words, []string{"id=$$(${AWK} '{print}' < ${WRKSRC}/idfile)", "&&", "echo", "\"$$id\""}) t.CheckEquals(rest, "") @@ -1587,7 +1594,8 @@ func (s *Suite) Test_splitIntoShellTokens__dollar_subshell(c *check.C) { func (s *Suite) Test_splitIntoShellTokens__semicolons(c *check.C) { t := s.Init(c) - words, rest := splitIntoShellTokens(dummyLine, "word1 word2;;;") + line := t.NewLine("filename.mk", 1, "") + words, rest := splitIntoShellTokens(line, "word1 word2;;;") t.CheckDeepEquals(words, []string{"word1", "word2", ";;", ";"}) t.CheckEquals(rest, "") @@ -1597,7 +1605,8 @@ func (s *Suite) Test_splitIntoShellTokens__whitespace(c *check.C) { t := s.Init(c) text := "\t${RUN} cd ${WRKSRC}&&(${ECHO} ${PERL5:Q};${ECHO})|${BASH} ./install" - words, rest := splitIntoShellTokens(dummyLine, text) + line := t.NewLine("filename.mk", 1, "") + words, rest := splitIntoShellTokens(line, text) t.CheckDeepEquals(words, []string{ "${RUN}", @@ -1611,7 +1620,8 @@ func (s *Suite) Test_splitIntoShellTokens__finished_dquot(c *check.C) { t := s.Init(c) text := "\"\"" - words, rest := splitIntoShellTokens(dummyLine, text) + line := t.NewLine("filename.mk", 1, "") + words, rest := splitIntoShellTokens(line, text) t.CheckDeepEquals(words, []string{"\"\""}) t.CheckEquals(rest, "") @@ -1621,7 +1631,8 @@ func (s *Suite) Test_splitIntoShellTokens__unfinished_dquot(c *check.C) { t := s.Init(c) text := "\t\"" - words, rest := splitIntoShellTokens(dummyLine, text) + line := t.NewLine("filename.mk", 1, "") + words, rest := splitIntoShellTokens(line, text) c.Check(words, check.IsNil) t.CheckEquals(rest, "\"") @@ -1631,7 +1642,8 @@ func (s *Suite) Test_splitIntoShellTokens__unescaped_dollar_in_dquot(c *check.C) t := s.Init(c) text := "echo \"$$\"" - words, rest := splitIntoShellTokens(dummyLine, text) + line := t.NewLine("filename.mk", 1, "") + words, rest := splitIntoShellTokens(line, text) t.CheckDeepEquals(words, []string{"echo", "\"$$\""}) t.CheckEquals(rest, "") @@ -1643,7 +1655,8 @@ func (s *Suite) Test_splitIntoShellTokens__varuse_with_embedded_space_and_other_ t := s.Init(c) varuseWord := "${GCONF_SCHEMAS:@.s.@${INSTALL_DATA} ${WRKSRC}/src/common/dbus/${.s.} ${DESTDIR}${GCONF_SCHEMAS_DIR}/@}" - words, rest := splitIntoShellTokens(dummyLine, varuseWord) + line := t.NewLine("filename.mk", 1, "") + words, rest := splitIntoShellTokens(line, varuseWord) t.CheckDeepEquals(words, []string{varuseWord}) t.CheckEquals(rest, "") @@ -1655,7 +1668,8 @@ func (s *Suite) Test_splitIntoShellTokens__two_shell_variables(c *check.C) { t := s.Init(c) code := "echo $$i$$j" - words, rest := splitIntoShellTokens(dummyLine, code) + line := t.NewLine("filename.mk", 1, "") + words, rest := splitIntoShellTokens(line, code) t.CheckDeepEquals(words, []string{"echo", "$$i$$j"}) t.CheckEquals(rest, "") @@ -1664,7 +1678,8 @@ func (s *Suite) Test_splitIntoShellTokens__two_shell_variables(c *check.C) { func (s *Suite) Test_splitIntoShellTokens__varuse_with_embedded_space(c *check.C) { t := s.Init(c) - words, rest := splitIntoShellTokens(dummyLine, "${VAR:S/ /_/g}") + line := t.NewLine("filename.mk", 1, "") + words, rest := splitIntoShellTokens(line, "${VAR:S/ /_/g}") t.CheckDeepEquals(words, []string{"${VAR:S/ /_/g}"}) t.CheckEquals(rest, "") @@ -1673,7 +1688,8 @@ func (s *Suite) Test_splitIntoShellTokens__varuse_with_embedded_space(c *check.C func (s *Suite) Test_splitIntoShellTokens__redirect(c *check.C) { t := s.Init(c) - words, rest := splitIntoShellTokens(dummyLine, "echo 1>output 2>>append 3>|clobber 4>&5 6<input >>append") + line := t.NewLine("filename.mk", 1, "") + words, rest := splitIntoShellTokens(line, "echo 1>output 2>>append 3>|clobber 4>&5 6<input >>append") t.CheckDeepEquals(words, []string{ "echo", @@ -1685,7 +1701,7 @@ func (s *Suite) Test_splitIntoShellTokens__redirect(c *check.C) { ">>", "append"}) t.CheckEquals(rest, "") - words, rest = splitIntoShellTokens(dummyLine, "echo 1> output 2>> append 3>| clobber 4>& 5 6< input >> append") + words, rest = splitIntoShellTokens(line, "echo 1> output 2>> append 3>| clobber 4>& 5 6< input >> append") t.CheckDeepEquals(words, []string{ "echo", diff --git a/pkgtools/pkglint/files/shtokenizer.go b/pkgtools/pkglint/files/shtokenizer.go index 531cfccb16c..527a38a9a56 100644 --- a/pkgtools/pkglint/files/shtokenizer.go +++ b/pkgtools/pkglint/files/shtokenizer.go @@ -3,14 +3,17 @@ package pkglint import "netbsd.org/pkglint/textproc" type ShTokenizer struct { - parser *MkParser + parser *MkLexer } func NewShTokenizer(line *Line, text string, emitWarnings bool) *ShTokenizer { // TODO: Switching to NewMkParser is nontrivial since emitWarnings must equal (line != nil). // assert((line != nil) == emitWarnings) - p := MkParser{line, textproc.NewLexer(text), emitWarnings} - return &ShTokenizer{&p} + if line != nil { + emitWarnings = true + } + mklex := NewMkLexer(text, line) + return &ShTokenizer{mklex} } // ShAtom parses a basic building block of a shell program. @@ -65,9 +68,9 @@ func (p *ShTokenizer) ShAtom(quoting ShQuoting) *ShAtom { if atom == nil { lexer.Reset(mark) if hasPrefix(lexer.Rest(), "$${") { - p.parser.Line.Warnf("Unclosed shell variable starting at %q.", shorten(lexer.Rest(), 20)) + p.parser.line.Warnf("Unclosed shell variable starting at %q.", shorten(lexer.Rest(), 20)) } else { - p.parser.Line.Warnf("Internal pkglint error in ShTokenizer.ShAtom at %q (quoting=%s).", lexer.Rest(), quoting) + p.parser.line.Warnf("Internal pkglint error in ShTokenizer.ShAtom at %q (quoting=%s).", lexer.Rest(), quoting) } } return atom diff --git a/pkgtools/pkglint/files/shtokenizer_test.go b/pkgtools/pkglint/files/shtokenizer_test.go index 14f08659591..b713facdadc 100644 --- a/pkgtools/pkglint/files/shtokenizer_test.go +++ b/pkgtools/pkglint/files/shtokenizer_test.go @@ -90,9 +90,13 @@ func (s *Suite) Test_ShTokenizer__fuzzing(c *check.C) { fuzzer.Char("\"'`$();|_#", 10) fuzzer.Range('a', 'z', 5) + // This "real" line is necessary because the autofix + // in MkParser.varUseBrace checks this. + line := t.NewLine("Makefile", 1, "\t:") + defer fuzzer.CheckOk() for i := 0; i < 1000; i++ { - tokenizer := NewShTokenizer(dummyLine, fuzzer.Generate(50), false) + tokenizer := NewShTokenizer(line, fuzzer.Generate(50), false) tokenizer.ShAtoms() t.Output() // Discard the output, only react on panics. } @@ -105,7 +109,8 @@ func (s *Suite) Test_ShTokenizer_ShAtom(c *check.C) { // testRest ensures that the given string is parsed to the expected // atoms, and returns the remaining text. testRest := func(s string, expectedAtoms []*ShAtom, expectedRest string) { - p := NewShTokenizer(dummyLine, s, false) + line := t.NewLine("filename.mk", 1, "") + p := NewShTokenizer(line, s, false) actualAtoms := p.ShAtoms() @@ -512,7 +517,8 @@ func (s *Suite) Test_ShTokenizer_ShAtom__quoting(c *check.C) { t := s.Init(c) test := func(input, expectedOutput string) { - p := NewShTokenizer(dummyLine, input, false) + line := t.NewLine("filename.mk", 1, "") + p := NewShTokenizer(line, input, false) q := shqPlain result := "" for { @@ -601,7 +607,8 @@ func (s *Suite) Test_ShTokenizer_ShToken(c *check.C) { // testRest ensures that the given string is parsed to the expected // tokens, and returns the remaining text. testRest := func(str string, expected ...string) string { - p := NewShTokenizer(dummyLine, str, false) + line := t.NewLine("testRest.mk", 1, "") + p := NewShTokenizer(line, str, false) for _, exp := range expected { t.CheckEquals(p.ShToken().MkText, exp) } @@ -609,7 +616,8 @@ func (s *Suite) Test_ShTokenizer_ShToken(c *check.C) { } test := func(str string, expected ...string) { - p := NewShTokenizer(dummyLine, str, false) + line := t.NewLine("test.mk", 1, "") + p := NewShTokenizer(line, str, false) for _, exp := range expected { t.CheckEquals(p.ShToken().MkText, exp) } @@ -618,7 +626,8 @@ func (s *Suite) Test_ShTokenizer_ShToken(c *check.C) { } testNil := func(str string) { - p := NewShTokenizer(dummyLine, str, false) + line := t.NewLine("testNil.mk", 1, "") + p := NewShTokenizer(line, str, false) c.Check(p.ShToken(), check.IsNil) t.CheckEquals(p.Rest(), "") t.CheckOutputEmpty() diff --git a/pkgtools/pkglint/files/shtypes_test.go b/pkgtools/pkglint/files/shtypes_test.go index 0d6cfbdd953..b1e475d4592 100644 --- a/pkgtools/pkglint/files/shtypes_test.go +++ b/pkgtools/pkglint/files/shtypes_test.go @@ -17,7 +17,8 @@ func (s *Suite) Test_ShAtomType_String(c *check.C) { func (s *Suite) Test_ShAtom_String(c *check.C) { t := s.Init(c) - tokenizer := NewShTokenizer(dummyLine, "${ECHO} \"hello, world\"", false) + line := t.NewLine("filename.mk", 1, "") + tokenizer := NewShTokenizer(line, "${ECHO} \"hello, world\"", false) atoms := tokenizer.ShAtoms() @@ -45,7 +46,8 @@ func (s *Suite) Test_NewShToken__no_atoms(c *check.C) { func (s *Suite) Test_ShToken_String(c *check.C) { t := s.Init(c) - tokenizer := NewShTokenizer(dummyLine, "${ECHO} \"hello, world\"", false) + line := t.NewLine("filename.mk", 1, "") + tokenizer := NewShTokenizer(line, "${ECHO} \"hello, world\"", false) t.CheckEquals(tokenizer.ShToken().String(), "ShToken([varuse(\"ECHO\")])") t.CheckEquals(tokenizer.ShToken().String(), "ShToken([ShAtom(text, \"\\\"\", d) ShAtom(text, \"hello, world\", d) \"\\\"\"])") diff --git a/pkgtools/pkglint/files/substcontext.go b/pkgtools/pkglint/files/substcontext.go index d20a6a3791b..0e5fefe9b3e 100644 --- a/pkgtools/pkglint/files/substcontext.go +++ b/pkgtools/pkglint/files/substcontext.go @@ -142,6 +142,7 @@ func (ctx *SubstContext) Varassign(mkline *MkLine) { fix.Replace("pre-patch", "post-extract") fix.Replace("post-patch", "pre-configure") fix.Apply() + // FIXME: Add test that has "SUBST_STAGE.id=pre-patch # or rather post-patch?" } if G.Pkg != nil && (value == "pre-configure" || value == "post-configure") { @@ -270,7 +271,6 @@ func (ctx *SubstContext) suggestSubstVars(mkline *MkLine) { if !mkline.HasComment() && len(tokens) == 2 && tokens[0] == "-e" { fix.Replace(mkline.Text, alignWith(varop, mkline.ValueAlign())+varname) } - fix.Anyway() fix.Apply() ctx.curr.seenVars = true @@ -280,7 +280,7 @@ func (ctx *SubstContext) suggestSubstVars(mkline *MkLine) { // extractVarname extracts the variable name from a sed command of the form // s,@VARNAME@,${VARNAME}, and some related variants thereof. func (*SubstContext) extractVarname(token string) string { - parser := NewMkParser(nil, token) + parser := NewMkLexer(token, nil) lexer := parser.lexer if !lexer.SkipByte('s') { return "" diff --git a/pkgtools/pkglint/files/testnames_test.go b/pkgtools/pkglint/files/testnames_test.go index 0354d53ab0f..b7906eac9ec 100644 --- a/pkgtools/pkglint/files/testnames_test.go +++ b/pkgtools/pkglint/files/testnames_test.go @@ -8,10 +8,11 @@ import ( // Ensures that all test names follow a common naming scheme: // // Test_${Type}_${Method}__${description_using_underscores} -func (s *Suite) Test__test_names(c *check.C) { - ck := intqa.NewTestNameChecker(c.Errorf) +func (s *Suite) Test__qa(c *check.C) { + ck := intqa.NewQAChecker(c.Errorf) ck.Configure("*", "*", "*", -intqa.EMissingTest) ck.Configure("path.go", "*", "*", +intqa.EMissingTest) ck.Configure("*yacc.go", "*", "*", intqa.ENone) + ck.Configure("*", "*", "", -intqa.EMissingTest) ck.Check() } diff --git a/pkgtools/pkglint/files/textproc/lexer_test.go b/pkgtools/pkglint/files/textproc/lexer_test.go index 039225b0da2..220eff39ad8 100644 --- a/pkgtools/pkglint/files/textproc/lexer_test.go +++ b/pkgtools/pkglint/files/textproc/lexer_test.go @@ -413,8 +413,8 @@ func (s *Suite) Test__Alpha(c *check.C) { c.Check(set.Contains('z'), equals, true) } -func (s *Suite) Test__test_names(c *check.C) { - ck := intqa.NewTestNameChecker(c.Errorf) +func (s *Suite) Test__qa(c *check.C) { + ck := intqa.NewQAChecker(c.Errorf) ck.Configure("*", "*", "*", -intqa.EMissingTest) ck.Check() } diff --git a/pkgtools/pkglint/files/trace/tracing_test.go b/pkgtools/pkglint/files/trace/tracing_test.go index 2a122f39867..9f778744ad6 100755 --- a/pkgtools/pkglint/files/trace/tracing_test.go +++ b/pkgtools/pkglint/files/trace/tracing_test.go @@ -143,8 +143,8 @@ func (str) String() string { return "It's a string" } -func (s *Suite) Test__test_names(c *check.C) { - ck := intqa.NewTestNameChecker(c.Errorf) +func (s *Suite) Test__qa(c *check.C) { + ck := intqa.NewQAChecker(c.Errorf) ck.Configure("*", "*", "*", -intqa.EMissingTest) ck.Check() } diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go index b9648c6580d..a3448055003 100644 --- a/pkgtools/pkglint/files/util.go +++ b/pkgtools/pkglint/files/util.go @@ -239,7 +239,7 @@ func assertf(cond bool, format string, args ...interface{}) { } func isEmptyDir(filename Path) bool { - if filename.HasSuffixText("/CVS") { + if filename.HasSuffixPath("CVS") { return true } @@ -457,6 +457,7 @@ func mkopSubst(s string, left bool, from string, right bool, to string, flags st }) } +// FIXME: Replace with Path.JoinNoClean func joinPath(a, b Path, others ...Path) Path { if len(others) == 0 { return a + "/" + b @@ -468,69 +469,6 @@ func joinPath(a, b Path, others ...Path) Path { return NewPath(strings.Join(parts, "/")) } -// relpath returns the relative path from the directory "from" -// to the filesystem entry "to". -// -// The relative path is built by going from the "from" directory via the -// pkgsrc root to the "to" filename. This produces the form -// "../../category/package" that is found in DEPENDS and .include lines. -// -// Both from and to are interpreted relative to the current working directory, -// unless they are absolute paths. -// -// This function should only be used if the relative path from one file to -// another cannot be computed in another way. The preferred way is to take -// the relative filenames directly from the .include or exists() where they -// appear. -// -// TODO: Invent data types for all kinds of relative paths that occur in pkgsrc -// and pkglint. Make sure that these paths cannot be accidentally mixed. -func relpath(from, to Path) (result Path) { - - if trace.Tracing { - defer trace.Call(from, to, trace.Result(&result))() - } - - cfrom := cleanpath(from) - cto := cleanpath(to) - - if cfrom == cto { - return "." - } - - // Take a shortcut for the common case from "dir" to "dir/subdir/...". - if cto.HasPrefixPath(cfrom) { - return cleanpath(cto[len(cfrom)+1:]) - } - - // Take a shortcut for the common case from "category/package" to ".". - // This is the most common variant in a complete pkgsrc scan. - if cto == "." { - fromParts := cfrom.Parts() - if len(fromParts) == 2 && !hasPrefix(fromParts[0], ".") && !hasPrefix(fromParts[1], ".") { - return "../.." - } - } - - if cfrom == "." && !cto.IsAbs() { - return cto.Clean() - } - - absFrom := abspath(cfrom) - absTopdir := abspath(G.Pkgsrc.topdir) - absTo := abspath(cto) - - toTop := absFrom.Rel(absTopdir) - fromTop := absTopdir.Rel(absTo) - - result = cleanpath(toTop.JoinNoClean(fromTop)) - - if trace.Tracing { - trace.Stepf("relpath from %q to %q = %q", cfrom, cto, result) - } - return -} - func abspath(filename Path) Path { abs := filename if !filename.IsAbs() { diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go index 391583a8e35..8e00d9c50de 100644 --- a/pkgtools/pkglint/files/util_test.go +++ b/pkgtools/pkglint/files/util_test.go @@ -89,6 +89,11 @@ func (s *Suite) Test_isEmptyDir(c *check.C) { t.CheckEquals(isEmptyDir(t.File(".")), true) t.CheckEquals(isEmptyDir(t.File("CVS")), true) + + t.Chdir(".") + + t.CheckEquals(isEmptyDir("."), true) + t.CheckEquals(isEmptyDir("CVS"), true) } func (s *Suite) Test_isEmptyDir__and_getSubdirs(c *check.C) { @@ -332,60 +337,6 @@ func (s *Suite) Test__regex_ReplaceFirst(c *check.C) { t.CheckEquals(rest, "X+c+d") } -func (s *Suite) Test_relpath(c *check.C) { - t := s.Init(c) - - t.Chdir(".") - t.CheckEquals(G.Pkgsrc.topdir, t.tmpdir) - - test := func(from, to Path, result Path) { - t.CheckEquals(relpath(from, to), result) - } - - test("some/dir", "some/directory", "../../some/directory") - test("some/directory", "some/dir", "../../some/dir") - - test("category/package/.", ".", "../..") - - // This case is handled by one of the shortcuts that avoid file system access. - test( - "./.", - "x11/frameworkintegration/../../meta-pkgs/kde/kf5.mk", - "meta-pkgs/kde/kf5.mk") - - test(".hidden/dir", ".", "../..") - test("dir/.hidden", ".", "../..") - - // This happens when "pkglint -r x11" is run. - G.Pkgsrc.topdir = "x11/.." - - test( - "./.", - "x11/frameworkintegration/../../meta-pkgs/kde/kf5.mk", - "meta-pkgs/kde/kf5.mk") - test( - "x11/..", - "x11/frameworkintegration/../../meta-pkgs/kde/kf5.mk", - "meta-pkgs/kde/kf5.mk") -} - -// Relpath is called so often that handling the most common calls -// without file system IO makes sense. -func (s *Suite) Test_relpath__quick(c *check.C) { - t := s.Init(c) - - test := func(from, to Path, result Path) { - t.CheckEquals(relpath(from, to), result) - } - - test("some/dir", "some/dir/../..", "../..") - test("some/dir", "some/dir/./././../..", "../..") - test("some/dir", "some/dir/", ".") - - test("some/dir", ".", "../..") - test("some/dir/.", ".", "../..") -} - func (s *Suite) Test_cleanpath(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/varalignblock.go b/pkgtools/pkglint/files/varalignblock.go index 72ca6fd52bb..affee436726 100644 --- a/pkgtools/pkglint/files/varalignblock.go +++ b/pkgtools/pkglint/files/varalignblock.go @@ -676,7 +676,7 @@ func (VaralignSplitter) parseVarnameOp(parser *MkParser, initial bool) (string, } mark := lexer.Mark() - _ = parser.Varname() + _ = parser.mklex.Varname() lexer.SkipHspace() ok, _ := parser.Op() assert(ok) diff --git a/pkgtools/pkglint/files/vardefs.go b/pkgtools/pkglint/files/vardefs.go index 11344c06a3c..3a41434a7ed 100644 --- a/pkgtools/pkglint/files/vardefs.go +++ b/pkgtools/pkglint/files/vardefs.go @@ -406,6 +406,12 @@ func (reg *VarTypeRegistry) enumFrom(pkgsrc *Pkgsrc, filename Path, defval strin return enum(joined) } + if !G.Testing { + mklines.Whole().Fatalf( + "Must contain at least 1 variable definition for %s.", + joinSkipEmptyCambridge("or", varcanons...)) + } + if trace.Tracing { trace.Stepf("Enum from default value: %s", defval) } @@ -421,8 +427,13 @@ func (reg *VarTypeRegistry) enumFrom(pkgsrc *Pkgsrc, filename Path, defval strin func (reg *VarTypeRegistry) enumFromDirs(pkgsrc *Pkgsrc, category Path, re regex.Pattern, repl string, defval string) *BasicType { versions := pkgsrc.ListVersions(category, re, repl, false) if len(versions) == 0 { + if !G.Testing { + NewLineWhole(category).Fatalf( + "Must contain at least 1 subdirectory matching %q.", re) + } return enum(defval) } + return enum(strings.Join(versions, " ")) } @@ -440,8 +451,13 @@ func (reg *VarTypeRegistry) enumFromFiles(basedir Path, re regex.Pattern, repl s } } if len(relevant) == 0 { + if !G.Testing { + NewLineWhole(basedir).Fatalf( + "Must contain at least 1 file matching %q.", re) + } return enum(defval) } + return enum(strings.Join(relevant, " ")) } diff --git a/pkgtools/pkglint/files/vardefs_test.go b/pkgtools/pkglint/files/vardefs_test.go index 06a342ff46c..6776c7cfde1 100644 --- a/pkgtools/pkglint/files/vardefs_test.go +++ b/pkgtools/pkglint/files/vardefs_test.go @@ -94,10 +94,36 @@ func (s *Suite) Test_VarTypeRegistry_enumFrom__no_tracing(c *check.C) { t.CheckEquals(nonexistentType.AllowedEnums(), "defval") } +func (s *Suite) Test_VarTypeRegistry_enumFrom__no_testing(c *check.C) { + t := s.Init(c) + + G.Testing = false + + t.ExpectFatal( + t.SetUpVartypes, + "FATAL: ~/mk/compiler.mk: Cannot be read.") + + t.CreateFileLines("mk/compiler.mk", + MkCvsID) + + t.ExpectFatal( + t.SetUpVartypes, + "FATAL: ~/mk/compiler.mk: Must contain at least 1 variable "+ + "definition for _COMPILERS or _PSEUDO_COMPILERS.") + + t.CreateFileLines("mk/compiler.mk", + MkCvsID, + "_COMPILERS=\tgcc") + + t.ExpectFatal( + t.SetUpVartypes, + "FATAL: ~/editors/emacs/modules.mk: Cannot be read.") +} + func (s *Suite) Test_VarTypeRegistry_enumFromDirs(c *check.C) { t := s.Init(c) - // To make the test useful, these directories must differ from the + // To make the test observable, these directories must differ from the // PYPKGPREFIX default value in vardefs.go. t.CreateFileLines("lang/python28/Makefile", MkCvsID) t.CreateFileLines("lang/python33/Makefile", MkCvsID) @@ -112,6 +138,20 @@ func (s *Suite) Test_VarTypeRegistry_enumFromDirs(c *check.C) { test("PYPKGPREFIX", "enum: py28 py33 (system-provided)") } +func (s *Suite) Test_VarTypeRegistry_enumFromDirs__no_testing(c *check.C) { + t := s.Init(c) + + G.Testing = false + + t.ExpectFatal( + func() { + G.Pkgsrc.vartypes.enumFromDirs( + &G.Pkgsrc, "category", `^pack.*`, "$0", "default") + }, + "FATAL: category: Must contain at least 1 "+ + "subdirectory matching \"^pack.*\".") +} + func (s *Suite) Test_VarTypeRegistry_enumFromFiles(c *check.C) { t := s.Init(c) @@ -130,6 +170,20 @@ func (s *Suite) Test_VarTypeRegistry_enumFromFiles(c *check.C) { test("OPSYS", "enum: NetBSD SunOS (system-provided)") } +func (s *Suite) Test_VarTypeRegistry_enumFromFiles__no_testing(c *check.C) { + t := s.Init(c) + + G.Testing = false + + t.ExpectFatal( + func() { + G.Pkgsrc.vartypes.enumFromFiles( + "mk/platform", `^(\w+)\.mk$`, "$1", "default") + }, + "FATAL: mk/platform: Must contain at least 1 "+ + "file matching \"^(\\\\w+)\\\\.mk$\".") +} + func (s *Suite) Test_VarTypeRegistry_Init(c *check.C) { t := s.Init(c) @@ -178,7 +232,8 @@ func (s *Suite) Test_VarTypeRegistry_Init__no_testing(c *check.C) { G.Testing = false t.ExpectFatal( t.FinishSetUp, - "FATAL: ~/editors/emacs/modules.mk: Cannot be read.") + "FATAL: ~/mk/compiler.mk: Must contain at least 1 "+ + "variable definition for _COMPILERS or _PSEUDO_COMPILERS.") } func (s *Suite) Test_VarTypeRegistry_Init__MASTER_SITES(c *check.C) { diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go index 0233555cae7..4619f935c6f 100644 --- a/pkgtools/pkglint/files/vartypecheck.go +++ b/pkgtools/pkglint/files/vartypecheck.go @@ -726,8 +726,6 @@ func (cv *VartypeCheck) Homepage() { "Defining MASTER_SITES=${HOMEPAGE} is ok, though.") if baseURL != "" { fix.Replace(wrong, fixedURL) - } else { - fix.Anyway() } fix.Apply() } @@ -1083,9 +1081,10 @@ func (cv *VartypeCheck) Pkgpath() { cv.MkLine.Errorf("A main pkgsrc package must not depend on a pkgsrc-wip package.") } - if !G.Pkgsrc.File(pkgpath.JoinNoClean("Makefile")).IsFile() { + pkgdir := G.Pkgsrc.File(pkgpath) + if !pkgdir.JoinNoClean("Makefile").IsFile() { cv.MkLine.Errorf("There is no package in %q.", - cv.MkLine.PathToFile(G.Pkgsrc.File(pkgpath))) + cv.MkLine.PathToFile(pkgdir)) return } diff --git a/pkgtools/pkglint/files/vartypecheck_test.go b/pkgtools/pkglint/files/vartypecheck_test.go index 64b1da229e8..4b8b54b66ec 100644 --- a/pkgtools/pkglint/files/vartypecheck_test.go +++ b/pkgtools/pkglint/files/vartypecheck_test.go @@ -897,13 +897,14 @@ func (s *Suite) Test_VartypeCheck_LdFlag(c *check.C) { func (s *Suite) Test_VartypeCheck_License(c *check.C) { t := s.Init(c) + t.Chdir(".") t.SetUpPkgsrc() // Adds the gnu-gpl-v2 and 2-clause-bsd licenses t.SetUpPackage("category/package") t.FinishSetUp() G.Pkg = NewPackage(t.File("category/package")) - mklines := t.NewMkLines("perl5.mk", + mklines := t.SetUpFileMkLines("perl5.mk", MkCvsID, "PERL5_LICENSE= gnu-gpl-v2 OR artistic") // Also registers the PERL5_LICENSE variable in the package. @@ -920,7 +921,7 @@ func (s *Suite) Test_VartypeCheck_License(c *check.C) { vt.Output( "ERROR: filename.mk:2: Parse error for license condition \"AND mit\".", - "WARN: filename.mk:3: License file ~/licenses/artistic does not exist.", + "WARN: filename.mk:3: License file licenses/artistic does not exist.", "ERROR: filename.mk:4: Parse error for license condition \"${UNKNOWN_LICENSE}\".") vt.Op(opAssignAppend) @@ -930,7 +931,7 @@ func (s *Suite) Test_VartypeCheck_License(c *check.C) { vt.Output( "ERROR: filename.mk:11: Parse error for appended license condition \"gnu-gpl-v2\".", - "WARN: filename.mk:12: License file ~/licenses/mit does not exist.") + "WARN: filename.mk:12: License file licenses/mit does not exist.") } func (s *Suite) Test_VartypeCheck_MachineGnuPlatform(c *check.C) { |