diff options
| author | rillig <rillig@pkgsrc.org> | 2019-02-21 22:49:03 +0000 |
|---|---|---|
| committer | rillig <rillig@pkgsrc.org> | 2019-02-21 22:49:03 +0000 |
| commit | f4adb2b4e5d0a5606b1f0a5e90b8ae9e454fe8aa (patch) | |
| tree | 8828cb3a388e4d94ecb7e0828f8677be2a47a2a4 /pkgtools | |
| parent | 6833d8f818484d62dce786d56edc39b9b198c395 (diff) | |
| download | pkgsrc-f4adb2b4e5d0a5606b1f0a5e90b8ae9e454fe8aa.tar.gz | |
pkgtools/pkglint: update to 5.7.0
Changes since 5.6.12:
* Many of the -C and -W command line options have been removed since
they are not used in practice. The -Wall and -Call options continue
to work though; these are the only options mentioned in the pkgsrc
guide.
* When a PLIST file contains redundant libtool libraries (.la and the
corresponding .so), there is only a single warning per file.
* Warnings about the package COMMENT are now strictly ordered from left
to right.
* The hashes for all distfiles must now contain the SHA512 hash. This
hash has been added to many distfiles in 2015. It's time now to
enforce it on all other distfiles as well.
* Makefile fragments that are included inside an .elif exists(...)
are not reported as missing.
* The check for redundant variables and accidentally overwritten
variables has been improved. Now the warning occurs at the later
definition. This especially applies to cases where a file is included
and after that, some of its variables are overridden. Variables in
unrelated files are no longer marked as redundant.
* When a package contains multiple definitions of a single variable
(typical for Makefile.common), the later definition overrides the
earlier definition. That way, the location of DISTINFO_FILE and
PATCHDIR is resolved correctly.
Diffstat (limited to 'pkgtools')
53 files changed, 2132 insertions, 667 deletions
diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index bfd0f926ffc..517aaab8b87 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.566 2019/01/26 16:31:33 rillig Exp $ +# $NetBSD: Makefile,v 1.567 2019/02/21 22:49:03 rillig Exp $ -PKGNAME= pkglint-5.6.12 +PKGNAME= pkglint-5.7.0 CATEGORIES= pkgtools DISTNAME= tools MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/} diff --git a/pkgtools/pkglint/files/alternatives_test.go b/pkgtools/pkglint/files/alternatives_test.go index 95a51135c5f..f17fecfe775 100644 --- a/pkgtools/pkglint/files/alternatives_test.go +++ b/pkgtools/pkglint/files/alternatives_test.go @@ -59,3 +59,23 @@ func (s *Suite) Test_CheckFileAlternatives__empty(c *check.C) { t.CheckOutputLines( "ERROR: ALTERNATIVES: Must not be empty.") } + +func (s *Suite) Test_CheckFileAlternatives__ALTERNATIVES_SRC(c *check.C) { + t := s.Init(c) + + // It's a strange situation, having an ALTERNATIVES file defined by + // the package but then referring to another package's file by means + // of ALTERNATIVES_SRC. As of February 2019 I don't remember if I + // really had this case in mind when I initially wrote the code in + // CheckFileAlternatives. + t.SetUpPackage("category/package", + "ALTERNATIVES_SRC=\talts") + t.CreateFileLines("category/package/ALTERNATIVES", + "bin/pgm @PREFIX@/bin/gnu-program", + "bin/pgm @PREFIX@/bin/nb-program") + G.Pkgsrc.LoadInfrastructure() + + G.Check(t.File("category/package")) + + t.CheckOutputEmpty() +} diff --git a/pkgtools/pkglint/files/autofix.go b/pkgtools/pkglint/files/autofix.go index 0db5bafe503..8e579a12fb5 100644 --- a/pkgtools/pkglint/files/autofix.go +++ b/pkgtools/pkglint/files/autofix.go @@ -96,15 +96,13 @@ func (fix *Autofix) ReplaceAfter(prefix, from string, to string) { } for _, rawLine := range fix.line.raw { - if rawLine.Lineno != 0 { - replaced := strings.Replace(rawLine.textnl, prefix+from, prefix+to, 1) - if replaced != rawLine.textnl { - if G.Logger.IsAutofix() { - rawLine.textnl = replaced - } - fix.Describef(rawLine.Lineno, "Replacing %q with %q.", from, to) - return + replaced := strings.Replace(rawLine.textnl, prefix+from, prefix+to, 1) + if replaced != rawLine.textnl { + if G.Logger.IsAutofix() { + rawLine.textnl = replaced } + fix.Describef(rawLine.Lineno, "Replacing %q with %q.", from, to) + return } } } @@ -122,26 +120,24 @@ func (fix *Autofix) ReplaceRegex(from regex.Pattern, toText string, howOften int done := 0 for _, rawLine := range fix.line.raw { - if rawLine.Lineno != 0 { - var froms []string // The strings that have actually changed + var froms []string // The strings that have actually changed - replace := func(fromText string) string { - if howOften >= 0 && done >= howOften { - return fromText - } - froms = append(froms, fromText) - done++ - return toText + replace := func(fromText string) string { + if howOften >= 0 && done >= howOften { + return fromText } + froms = append(froms, fromText) + done++ + return toText + } - replaced := replaceAllFunc(rawLine.textnl, from, replace) - if replaced != rawLine.textnl { - if G.Logger.IsAutofix() { - rawLine.textnl = replaced - } - for _, fromText := range froms { - fix.Describef(rawLine.Lineno, "Replacing %q with %q.", fromText, toText) - } + replaced := replaceAllFunc(rawLine.textnl, from, replace) + if replaced != rawLine.textnl { + if G.Logger.IsAutofix() { + rawLine.textnl = replaced + } + for _, fromText := range froms { + fix.Describef(rawLine.Lineno, "Replacing %q with %q.", fromText, toText) } } } @@ -231,9 +227,10 @@ func (fix *Autofix) Delete() { } // Anyway has the effect of showing the diagnostic even when nothing can -// be fixed automatically. +// be fixed automatically, but only if neither --show-autofix nor +// --autofix mode is given. func (fix *Autofix) Anyway() { - fix.anyway = true + fix.anyway = !G.Logger.IsAutofix() } // Apply does the actual work. @@ -262,21 +259,25 @@ func (fix *Autofix) Apply() { fix.autofixShortTerm = autofixShortTerm{} } - if !G.Logger.Relevant(fix.diagFormat) || !fix.anyway && len(fix.actions) == 0 { + if !(G.Logger.Relevant(fix.diagFormat) && (len(fix.actions) > 0 || fix.anyway)) { reset() return } - logDiagnostic := (G.Logger.Opts.ShowAutofix || !G.Logger.Opts.Autofix) && - fix.diagFormat != SilentAutofixFormat + logDiagnostic := true + switch { + case fix.diagFormat == SilentAutofixFormat: + logDiagnostic = false + case G.Logger.Opts.Autofix && !G.Logger.Opts.ShowAutofix: + logDiagnostic = false + } + logFix := G.Logger.IsAutofix() if logDiagnostic { msg := sprintf(fix.diagFormat, fix.diagArgs...) - if !logFix { - if fix.diagFormat == AutofixFormat || G.Logger.FirstTime(line.Filename, line.Linenos(), msg) { - line.showSource(G.out) - } + if !logFix && G.Logger.FirstTime(line.Filename, line.Linenos(), msg) { + line.showSource(G.out) } G.Logf(fix.level, line.Filename, line.Linenos(), fix.diagFormat, msg) } @@ -299,7 +300,7 @@ func (fix *Autofix) Apply() { G.Explain(fix.explanation...) } if G.Logger.Opts.ShowSource { - if !G.Logger.Opts.Explain || !logDiagnostic || len(fix.explanation) == 0 { + if !(G.Logger.Opts.Explain && logDiagnostic && len(fix.explanation) > 0) { G.out.Separate() } } @@ -327,8 +328,8 @@ func (fix *Autofix) Realign(mkline MkLine, newWidth int) { { // Parsing the continuation marker as variable value is cheating but works well. text := strings.TrimSuffix(mkline.raw[0].orignl, "\n") - m, _, _, _, _, valueAlign, value, _, _ := MatchVarassign(text) - if m && value != "\\" { + _, _, _, _, _, valueAlign, value, _, _ := MatchVarassign(text) + if value != "\\" { oldWidth = tabWidth(valueAlign) } } @@ -336,7 +337,7 @@ func (fix *Autofix) Realign(mkline MkLine, newWidth int) { for _, rawLine := range fix.line.raw[1:] { _, comment, space := match2(rawLine.textnl, `^(#?)([ \t]*)`) width := tabWidth(comment + space) - if (oldWidth == 0 || width < oldWidth) && width >= 8 && rawLine.textnl != "\n" { + if (oldWidth == 0 || width < oldWidth) && width >= 8 { oldWidth = width } if !matches(space, `^\t* {0,7}$`) { diff --git a/pkgtools/pkglint/files/autofix_test.go b/pkgtools/pkglint/files/autofix_test.go index 30069c1fa25..5b7febc46de 100644 --- a/pkgtools/pkglint/files/autofix_test.go +++ b/pkgtools/pkglint/files/autofix_test.go @@ -210,6 +210,35 @@ func (s *Suite) Test_Autofix_ReplaceRegex__show_autofix_and_source(c *check.C) { "+\tYXXXX") } +// When an autofix replaces text, it does not touch those +// lines that have been inserted before since these are +// usually already correct. +func (s *Suite) Test_Autofix_ReplaceAfter__after_inserting_a_line(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--show-autofix") + line := t.NewLine("filename", 5, "initial text") + + fix := line.Autofix() + fix.Notef("Inserting a line.") + fix.InsertBefore("line before") + fix.InsertAfter("line after") + fix.Apply() + + fix.Notef("Replacing text.") + fix.Replace("l", "L") + fix.ReplaceRegex(`i`, "I", 1) + fix.Apply() + + t.CheckOutputLines( + "NOTE: filename:5: Inserting a line.", + "AUTOFIX: filename:5: Inserting a line \"line before\" before this line.", + "AUTOFIX: filename:5: Inserting a line \"line after\" after this line.", + "NOTE: filename:5: Replacing text.", + "AUTOFIX: filename:5: Replacing \"l\" with \"L\".", + "AUTOFIX: filename:5: Replacing \"i\" with \"I\".") +} + func (s *Suite) Test_SaveAutofixChanges(c *check.C) { t := s.Init(c) @@ -627,6 +656,35 @@ func (s *Suite) Test_Autofix__suppress_unfixable_warnings_with_show_autofix(c *c "+\tTODO") } +// 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) { + t := s.Init(c) + + t.SetUpCommandLine("--show-autofix") + + line := t.NewLine("filename", 3, "") + fix := line.Autofix() + + fix.Warnf("This autofix doesn't match.") + fix.Replace("doesn't match", "") + fix.Anyway() + fix.Apply() + + t.CheckOutputEmpty() + + t.SetUpCommandLine() + + 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.") +} + // If an Autofix doesn't do anything, it must not log any diagnostics. func (s *Suite) Test_Autofix__noop_replace(c *check.C) { t := s.Init(c) @@ -809,6 +867,210 @@ func (s *Suite) Test_Autofix_Apply__explanation_followed_by_note(c *check.C) { "NOTE: README.txt:123: A note without fix.") } +func (s *Suite) Test_Autofix_Anyway__autofix_option(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--autofix") + line := t.NewLine("filename", 5, "text") + + 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. + t.CheckOutputEmpty() +} + +func (s *Suite) Test_Autofix_Anyway__autofix_and_show_autofix_options(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--autofix", "--show-autofix") + line := t.NewLine("filename", 5, "text") + + 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. + // Even if the --show-autofix option is explicitly given, the note should not be logged. + // Therefore, in the end nothing is shown in this case. + t.CheckOutputEmpty() +} + +// The --autofix option normally suppresses the diagnostics and just logs +// the actual fixes. Adding the --show-autofix option logs both. +func (s *Suite) Test_Autofix_Apply__autofix_and_show_autofix_options(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--autofix", "--show-autofix") + line := t.NewLine("filename", 5, "text") + + fix := line.Autofix() + fix.Notef("This line is quite short.") + fix.Replace("text", "replacement") + fix.Apply() + + t.CheckOutputLines( + "NOTE: filename:5: This line is quite short.", + "AUTOFIX: filename:5: Replacing \"text\" with \"replacement\".") +} + +// Ensures that without explanations, the separator between the individual +// diagnostics are generated. +func (s *Suite) Test_Autofix_Apply__source_without_explain(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--source", "--explain", "--show-autofix") + line := t.NewLine("filename", 5, "text") + + fix := line.Autofix() + fix.Notef("This line is quite short.") + fix.Replace("text", "replacement") + fix.Apply() + + fix.Warnf("Follow-up warning, separated.") + fix.Replace("replacement", "text again") + fix.Apply() + + t.CheckOutputLines( + "NOTE: filename:5: This line is quite short.", + "AUTOFIX: filename:5: Replacing \"text\" with \"replacement\".", + "-\ttext", + "+\treplacement", + "", + "WARN: filename:5: Follow-up warning, separated.", + "AUTOFIX: filename:5: Replacing \"replacement\" with \"text again\".", + "-\ttext", + "+\ttext again") +} + +func (s *Suite) Test_Autofix_Realign__wrong_line_type(c *check.C) { + t := s.Init(c) + + mklines := t.NewMkLines("file.mk", + MkRcsID, + ".if \\", + "${PKGSRC_RUN_TESTS}") + + mkline := mklines.mklines[1] + fix := mkline.Autofix() + + t.ExpectPanic( + func() { fix.Realign(mkline, 16) }, + "Pkglint internal error: Line must be a variable assignment.") +} + +func (s *Suite) Test_Autofix_Realign__short_continuation_line(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--autofix") + mklines := t.SetUpFileMkLines("file.mk", + MkRcsID, + "BUILD_DIRS= \\", + "\tdir \\", + "") + mkline := mklines.mklines[1] + fix := mkline.Autofix() + fix.Warnf("Line should be realigned.") + + // In this case realigning has no effect since the oldWidth == 8, + // which counts as "sufficiently intentional not to be modified". + fix.Realign(mkline, 16) + + mklines.SaveAutofixChanges() + + t.CheckOutputEmpty() + t.CheckFileLines("file.mk", + MkRcsID, + "BUILD_DIRS= \\", + "\tdir \\", + "") +} + +func (s *Suite) Test_Autofix_Realign__multiline_indented_with_spaces(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--autofix") + mklines := t.SetUpFileMkLines("file.mk", + MkRcsID, + "BUILD_DIRS= \\", + "\t dir1 \\", + "\t\tdir2 \\", + " ") // Trailing whitespace is not fixed by Autofix.Realign. + + mkline := mklines.mklines[1] + + fix := mkline.Autofix() + fix.Warnf("Warning.") + fix.Realign(mkline, 16) + fix.Apply() + + mklines.SaveAutofixChanges() + + t.CheckOutputLines( + "AUTOFIX: ~/file.mk:3: Replacing indentation \"\\t \" with \"\\t\\t\".") + t.CheckFileLines("file.mk", + MkRcsID, + "BUILD_DIRS= \\", + "\t\tdir1 \\", + "\t\tdir2 \\", + " ") +} + +// Just for branch coverage. +func (s *Suite) Test_Autofix_setDiag__no_testing_mode(c *check.C) { + t := s.Init(c) + + line := t.NewLine("file.mk", 123, "text") + + G.Testing = false + + fix := line.Autofix() + fix.Notef("Note.") + fix.Replace("from", "to") + fix.Apply() + + t.CheckOutputEmpty() +} + +func (s *Suite) Test_Autofix_setDiag__bad_call_sequence(c *check.C) { + t := s.Init(c) + + line := t.NewLine("file.mk", 123, "text") + fix := line.Autofix() + fix.Notef("Note.") + + t.ExpectPanic( + func() { fix.Notef("Note 2.") }, + "Pkglint internal error: Autofix can only have a single diagnostic.") + + fix.level = nil // To cover the second assertion. + t.ExpectPanic( + func() { fix.Notef("Note 2.") }, + "Pkglint internal error: Autofix can only have a single diagnostic.") +} + +func (s *Suite) Test_Autofix_assertRealLine(c *check.C) { + t := s.Init(c) + + line := NewLineEOF("filename.mk") + fix := line.Autofix() + fix.Warnf("Warning.") + + t.ExpectPanic( + func() { fix.Replace("from", "to") }, + "Pkglint internal error: Cannot autofix this line since it is not a real line.") +} + func (s *Suite) Test_SaveAutofixChanges__file_removed(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/buildlink3.go b/pkgtools/pkglint/files/buildlink3.go index 0aeae54f32a..ac9559ad2f3 100644 --- a/pkgtools/pkglint/files/buildlink3.go +++ b/pkgtools/pkglint/files/buildlink3.go @@ -205,7 +205,8 @@ func (ck *Buildlink3Checker) checkVarassign(mlex *MkLinesLexer, mkline MkLine, p } func (ck *Buildlink3Checker) checkVaruseInPkgbase(pkgbase string, pkgbaseLine MkLine) { - for _, token := range pkgbaseLine.ValueTokens() { + tokens, _ := pkgbaseLine.ValueTokens() + for _, token := range tokens { if token.Varuse == nil { continue } diff --git a/pkgtools/pkglint/files/buildlink3_test.go b/pkgtools/pkglint/files/buildlink3_test.go index 7ee1248581b..8de6c282ce4 100644 --- a/pkgtools/pkglint/files/buildlink3_test.go +++ b/pkgtools/pkglint/files/buildlink3_test.go @@ -493,6 +493,192 @@ func (s *Suite) Test_CheckLinesBuildlink3Mk__PKGBASE_with_unknown_variable(c *ch "WARN: buildlink3.mk:3: Please replace \"${LICENSE}\" with a simple string (also in other variables in this file).") } +func (s *Suite) Test_Buildlink3Checker_checkMainPart__if_else_endif(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package", + "PKGNAME=\tpackage-1.0") + t.CreateFileDummyBuildlink3("category/package/buildlink3.mk", + ".if ${X11_TYPE} == modular", + ".else", + ".endif") + + G.Check(t.File("category/package")) + + t.CheckOutputEmpty() +} + +func (s *Suite) Test_Buildlink3Checker_checkVarassign__dependencies_with_path(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package", + "PKGNAME=\tpackage-1.0") + t.CreateFileDummyBuildlink3("category/package/buildlink3.mk", + "BUILDLINK_ABI_DEPENDS.package+=\tpackage>=1.0:../../category/package", + "BUILDLINK_API_DEPENDS.package+=\tpackage>=1.5:../../category/package") + + G.Check(t.File("category/package")) + + // Since these dependencies are malformed, they are not processed further. + // Doing that would reveal that the ABI version should be higher than the API version. + t.CheckOutputLines( + "WARN: ~/category/package/buildlink3.mk:12: "+ + "Invalid dependency pattern \"package>=1.0:../../category/package\".", + "WARN: ~/category/package/buildlink3.mk:13: "+ + "Invalid dependency pattern \"package>=1.5:../../category/package\".") +} + +func (s *Suite) Test_Buildlink3Checker_checkVarassign__abi_without_api(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package", + "PKGNAME=\tpackage-1.0") + // t.CreateFileDummyBuildlink3() cannot be used here since it always adds an API line. + t.CreateFileLines("category/package/buildlink3.mk", + MkRcsID, + "", + "BUILDLINK_TREE+=\tpackage", + "", + ".if !defined(PACKAGE_BUILDLINK3_MK)", + "PACKAGE_BUILDLINK3_MK:=", + "", + "BUILDLINK_PKGSRCDIR.package?=\t../../category/package", + "BUILDLINK_DEPMETHOD.package?=\tbuild", + "BUILDLINK_ABI_DEPENDS.package+=\tpackage>=1.0", + "", + ".endif # PACKAGE_BUILDLINK3_MK", + "", + "BUILDLINK_TREE+=\t-package") + + G.Check(t.File("category/package")) + + // Since only ABI is given but not API, the check for ABI >= API cannot be done. + t.CheckOutputLines( + "WARN: ~/category/package/buildlink3.mk:13: Definition of BUILDLINK_API_DEPENDS is missing.") +} + +func (s *Suite) Test_Buildlink3Checker_checkVarassign__abi_and_api_with_variables(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package", + "PKGNAME=\tpackage-1.0") + t.CreateFileDummyBuildlink3("category/package/buildlink3.mk", + "BUILDLINK_ABI_DEPENDS.package+=\tpackage>=${ABI_VERSION}", + "BUILDLINK_API_DEPENDS.package+=\tpackage>=${API_VERSION}", + "", + "ABI_VERSION=\t1.0", + "API_VERSION=\t1.5") + + G.Check(t.File("category/package")) + + // Since the versions contain variable references, pkglint refuses to compare them. + t.CheckOutputEmpty() +} + +func (s *Suite) Test_Buildlink3Checker_checkVarassign__api_with_variable(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package", + "PKGNAME=\tpackage-1.0") + t.CreateFileDummyBuildlink3("category/package/buildlink3.mk", + "BUILDLINK_ABI_DEPENDS.package+=\tpackage>=1.0", + "BUILDLINK_API_DEPENDS.package+=\tpackage>=${API_VERSION}", + "", + "API_VERSION=\t1.5") + + G.Check(t.File("category/package")) + + // Since the versions contain variable references, pkglint refuses to compare them. + t.CheckOutputEmpty() +} + +func (s *Suite) Test_Buildlink3Checker_checkVarassign__abi_and_api_with_pattern(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package", + "PKGNAME=\tpackage-1.0") + t.CreateFileDummyBuildlink3("category/package/buildlink3.mk", + "BUILDLINK_ABI_DEPENDS.package+=\tpackage-1.*", + "BUILDLINK_API_DEPENDS.package+=\tpackage-2.*") + + G.Check(t.File("category/package")) + + // Since the versions do not contain lower bounds (they are package-1.* + // instead of package>=1), pkglint refuses to compare them. + t.CheckOutputEmpty() +} + +func (s *Suite) Test_Buildlink3Checker_checkVarassign__api_with_pattern(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package", + "PKGNAME=\tpackage-1.0") + t.CreateFileDummyBuildlink3("category/package/buildlink3.mk", + "BUILDLINK_ABI_DEPENDS.package+=\tpackage>=1", + "BUILDLINK_API_DEPENDS.package+=\tpackage-1.*") + + G.Check(t.File("category/package")) + + // Since the API version does not contain lower bounds (it is package-1.* + // instead of package>=1), pkglint refuses to compare the versions. + t.CheckOutputEmpty() +} + +func (s *Suite) Test_Buildlink3Checker_checkVarassign__other_variables(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package", + "PKGNAME=\tpackage-1.0") + t.CreateFileDummyBuildlink3("category/package/buildlink3.mk", + "BUILDLINK_TREE+=\tmistake", // Wrong, but doesn't happen in practice. + "", + "LDFLAGS.NetBSD+=\t-ldl", + "", + "BUILDLINK_DEPMETHOD.other+=\tbuild", + "", + "BUILDLINK_API_DEPENDS.other+=\tother>=3") + + G.Check(t.File("category/package")) + + // FIXME: Why is appending to LDFLAGS forbidden? It sounds useful. + t.CheckOutputLines( + "WARN: ~/category/package/buildlink3.mk:14: "+ + "The variable LDFLAGS.NetBSD may not be appended to in this file; "+ + "it would be ok in Makefile, Makefile.common, options.mk or *.mk.", + "WARN: ~/category/package/buildlink3.mk:16: "+ + "Only buildlink variables for \"package\", "+ + "not \"other\" may be set in this file.") +} + +// Just for branch coverage. +func (s *Suite) Test_Buildlink3Checker_Check__no_tracing(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package") + t.CreateFileDummyBuildlink3("category/package/buildlink3.mk") + t.DisableTracing() + + G.Check(t.File("category/package/buildlink3.mk")) + + t.CheckOutputEmpty() +} + +func (s *Suite) Test_Buildlink3Checker_checkSecondParagraph__missing_mkbase(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package", + "DISTNAME=\t# empty", + "PKGNAME=\t# empty, to force mkbase to be empty") + t.CreateFileDummyBuildlink3("category/package/buildlink3.mk") + + G.Check(t.File("category/package")) + + // There is no warning from buildlink3.mk about mismatched package names + // since that is only a follow-up error of being unable to parse the pkgbase. + t.CheckOutputLines( + "WARN: ~/category/package/Makefile:4: \"\" is not a valid package name.") +} + // Since the buildlink3 checker does not use MkLines.ForEach, it has to keep // track of the nesting depth of .if directives. func (s *Suite) Test_Buildlink3Checker_checkMainPart__nested_if(c *check.C) { diff --git a/pkgtools/pkglint/files/category.go b/pkgtools/pkglint/files/category.go index c5f8e7e3521..f97afc2717c 100644 --- a/pkgtools/pkglint/files/category.go +++ b/pkgtools/pkglint/files/category.go @@ -123,7 +123,7 @@ func CheckdirCategory(dir string) { } fRest = fRest[1:] - } else if len(mRest) > 0 && (len(fRest) == 0 || mRest[0].name < fRest[0]) { + } else if len(fRest) == 0 || mRest[0].name < fRest[0] { if !fCheck[mRest[0].name] { fix := mRest[0].line.Autofix() fix.Errorf("%q exists in the Makefile but not in the file system.", mRest[0].name) diff --git a/pkgtools/pkglint/files/category_test.go b/pkgtools/pkglint/files/category_test.go index 834a2b34ff4..d8f22a218a3 100644 --- a/pkgtools/pkglint/files/category_test.go +++ b/pkgtools/pkglint/files/category_test.go @@ -130,6 +130,89 @@ func (s *Suite) Test_CheckdirCategory__subdirs(c *check.C) { "ERROR: ~/category/Makefile:11: \"commented-mk-only\" exists in the Makefile but not in the file system.") } +func (s *Suite) Test_CheckdirCategory__only_in_Makefile(c *check.C) { + t := s.Init(c) + + t.SetUpPkgsrc() + t.SetUpVartypes() + t.CreateFileLines("mk/misc/category.mk") + t.CreateFileLines("category/both/Makefile") + t.CreateFileLines("category/Makefile", + MkRcsID, + "", + "COMMENT=\tCategory comment", + "", + "SUBDIR+=\tabove-only-in-makefile", + "SUBDIR+=\tboth", + "SUBDIR+=\tonly-in-makefile", + "", + ".include \"../mk/misc/category.mk\"") + + CheckdirCategory(t.File("category")) + + t.CheckOutputLines( + "ERROR: ~/category/Makefile:5: \"above-only-in-makefile\" exists in the Makefile "+ + "but not in the file system.", + "ERROR: ~/category/Makefile:7: \"only-in-makefile\" exists in the Makefile "+ + "but not in the file system.") +} + +func (s *Suite) Test_CheckdirCategory__only_in_file_system(c *check.C) { + t := s.Init(c) + + t.SetUpPkgsrc() + t.SetUpVartypes() + t.CreateFileLines("mk/misc/category.mk") + t.CreateFileLines("category/above-only-in-fs/Makefile") + t.CreateFileLines("category/both/Makefile") + t.CreateFileLines("category/only-in-fs/Makefile") + t.CreateFileLines("category/Makefile", + MkRcsID, + "", + "COMMENT=\tCategory comment", + "", + "SUBDIR+=\tboth", + "", + ".include \"../mk/misc/category.mk\"") + + CheckdirCategory(t.File("category")) + + t.CheckOutputLines( + "ERROR: ~/category/Makefile:5: \"above-only-in-fs\" exists in the file system "+ + "but not in the Makefile.", + "ERROR: ~/category/Makefile:6: \"only-in-fs\" exists in the file system "+ + "but not in the Makefile.") +} + +func (s *Suite) Test_CheckdirCategory__recursive(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("-r") + t.SetUpPkgsrc() + t.SetUpVartypes() + t.CreateFileLines("mk/misc/category.mk") + t.CreateFileLines("category/commented/Makefile") + t.CreateFileLines("category/package/Makefile") + t.CreateFileLines("category/Makefile", + MkRcsID, + "", + "COMMENT=\tCategory comment", + "", + "#SUBDIR+=\tcommented\t# reason", + "SUBDIR+=\tpackage", + "", + ".include \"../mk/misc/category.mk\"") + t.Chdir("category") + + CheckdirCategory(".") + + t.CheckOutputEmpty() + t.Check( + G.Todo, + deepEquals, + []string{"./package"}) +} + // Ensures that a directory in the file system can be added at the very // end of the SUBDIR list. This case takes a different code path than // an addition in the middle. @@ -187,6 +270,61 @@ func (s *Suite) Test_CheckdirCategory__indentation(c *check.C) { "NOTE: ~/category/Makefile:5: This variable value should be aligned with tabs, not spaces, to column 17.") } +func (s *Suite) Test_CheckdirCategory__comment_at_the_top(c *check.C) { + t := s.Init(c) + + t.SetUpPkgsrc() + t.SetUpVartypes() + t.CreateFileLines("mk/misc/category.mk") + t.CreateFileLines("category/package/Makefile") + t.CreateFileLines("category/Makefile", + MkRcsID, + "", + "# This category collects all programs that don't fit anywhere else.", + "", + "COMMENT=\tCategory comment", + "", + "SUBDIR+=\tpackage", + "", + ".include \"../mk/misc/category.mk\"") + + CheckdirCategory(t.File("category")) + + // FIXME: Wow. These are quite a few warnings and errors, just because there is + // an additional comment above the COMMENT definition. + t.CheckOutputLines( + "ERROR: ~/category/Makefile:3: COMMENT= line expected.", + "NOTE: ~/category/Makefile:2: Empty line expected after this line.", + "ERROR: ~/category/Makefile:3: SUBDIR+= line or empty line expected.", + "ERROR: ~/category/Makefile:3: \"package\" exists in the file system but not in the Makefile.", + "NOTE: ~/category/Makefile:2: Empty line expected after this line.", + "WARN: ~/category/Makefile:3: This line should contain the following text: .include \"../mk/misc/category.mk\"", + "ERROR: ~/category/Makefile:3: The file should end here.") +} + +func (s *Suite) Test_CheckdirCategory__unexpected_EOF_while_reading_SUBDIR(c *check.C) { + t := s.Init(c) + + t.SetUpPkgsrc() + t.SetUpVartypes() + t.CreateFileLines("mk/misc/category.mk") + t.CreateFileLines("category/package/Makefile") + t.CreateFileLines("category/Makefile", + MkRcsID, + "", + "COMMENT=\tCategory comment", + "", + "SUBDIR+=\tpackage") + + CheckdirCategory(t.File("category")) + + // Doesn't happen in practice since categories are created very seldom. + t.CheckOutputLines( + "NOTE: ~/category/Makefile:5: Empty line expected after this line.", + "WARN: ~/category/Makefile:EOF: This line should contain the following text: "+ + ".include \"../mk/misc/category.mk\"") +} + func (s *Suite) Test_CheckdirCategory__no_Makefile(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/check_test.go b/pkgtools/pkglint/files/check_test.go index 18c9099492a..154ad8e2f92 100644 --- a/pkgtools/pkglint/files/check_test.go +++ b/pkgtools/pkglint/files/check_test.go @@ -301,6 +301,15 @@ func (t *Tester) SetUpPackage(pkgpath string, makefileLines ...string) string { PlistRcsID, "bin/program") + // Because the package Makefile includes this file, the check for the + // correct ordering of variables is skipped. As of February 2019, the + // SetupPackage function does not insert the custom variables in the + // correct position. To prevent the tests from having to mention the + // unrelated warnings about the variable order, that check is suppressed + // here. + t.CreateFileLines(pkgpath+"/suppress-varorder.mk", + MkRcsID) + // This distinfo file contains dummy hashes since pkglint cannot check the // distfiles hashes anyway. It can only check the hashes for the patches. t.CreateFileLines(pkgpath+"/distinfo", @@ -323,7 +332,8 @@ func (t *Tester) SetUpPackage(pkgpath string, makefileLines ...string) string { "HOMEPAGE=\t# none", "COMMENT=\tDummy package", "LICENSE=\t2-clause-bsd", - ""} + "", + ".include \"suppress-varorder.mk\""} for len(mlines) < 19 { mlines = append(mlines, "# empty") } @@ -665,9 +675,6 @@ func (t *Tester) CheckOutputLines(expectedLines ...string) { // // This is useful when stepping through the code, especially // in combination with SetUpCommandLine("--debug"). -// -// In JetBrains GoLand, the tracing output is suppressed after the first -// failed check, see https://youtrack.jetbrains.com/issue/GO-6154. func (t *Tester) EnableTracing() { G.out = NewSeparatorWriter(io.MultiWriter(os.Stdout, &t.stdout)) trace.Out = os.Stdout @@ -677,8 +684,9 @@ func (t *Tester) EnableTracing() { // EnableTracingToLog enables the tracing and writes the tracing output // to the test log that can be examined with Tester.Output. func (t *Tester) EnableTracingToLog() { - t.EnableTracing() + G.out = NewSeparatorWriter(&t.stdout) trace.Out = &t.stdout + trace.Tracing = true } // EnableSilentTracing enables tracing mode but discards any tracing output. diff --git a/pkgtools/pkglint/files/distinfo.go b/pkgtools/pkglint/files/distinfo.go index acf6a92a911..034a849dc9b 100644 --- a/pkgtools/pkglint/files/distinfo.go +++ b/pkgtools/pkglint/files/distinfo.go @@ -124,7 +124,7 @@ func (ck *distinfoLinesChecker) checkAlgorithms(line Line) { "", "If the patches directory looks wrong, pkglint needs to be improved.") - case algorithms != "SHA1, RMD160, Size" && algorithms != "SHA1, RMD160, SHA512, Size": + case algorithms != "SHA1, RMD160, SHA512, Size": line.Errorf("Expected SHA1, RMD160, SHA512, Size checksums for %q, got %s.", filename, algorithms) } } @@ -144,7 +144,7 @@ func (ck *distinfoLinesChecker) checkUnrecordedPatches() { for _, file := range patchFiles { patchName := file.Name() if file.Mode().IsRegular() && !ck.patches[patchName] && hasPrefix(patchName, "patch-") { - ck.distinfoLines.Errorf("patch %q is not recorded. Run %q.", + ck.distinfoLines.Errorf("Patch %q is not recorded. Run %q.", cleanpath(relpath(path.Dir(ck.distinfoLines.FileName), G.Pkg.File(ck.patchdir+"/"+patchName))), bmake("makepatchsum")) } @@ -161,7 +161,7 @@ func (ck *distinfoLinesChecker) checkGlobalDistfileMismatch(line Line, filename, return } - hashes := G.Pkgsrc.Hashes + hashes := G.Hashes if hashes == nil { return } @@ -174,25 +174,23 @@ func (ck *distinfoLinesChecker) checkGlobalDistfileMismatch(line Line, filename, return } - if hashes != nil && !hasPrefix(filename, "patch-") { - key := alg + ":" + filename - otherHash := hashes[key] + key := alg + ":" + filename + otherHash := hashes[key] - // See https://github.com/golang/go/issues/29802 - hashBytes := make([]byte, hex.DecodedLen(len(hash))) - _, err := hex.Decode(hashBytes, []byte(hash)) - if err != nil { - line.Errorf("The %s hash for %s contains a non-hex character.", alg, filename) - } + // See https://github.com/golang/go/issues/29802 + hashBytes := make([]byte, hex.DecodedLen(len(hash))) + _, err := hex.Decode(hashBytes, []byte(hash)) + if err != nil { + line.Errorf("The %s hash for %s contains a non-hex character.", alg, filename) + } - if otherHash != nil { - if !bytes.Equal(otherHash.hash, hashBytes) { - line.Errorf("The %s hash for %s is %s, which conflicts with %s in %s.", - alg, filename, hash, hex.EncodeToString(otherHash.hash), line.RefToLocation(otherHash.location)) - } - } else { - hashes[key] = &Hash{hashBytes, line.Location} + if otherHash != nil { + if !bytes.Equal(otherHash.hash, hashBytes) { + line.Errorf("The %s hash for %s is %s, which conflicts with %s in %s.", + alg, filename, hash, hex.EncodeToString(otherHash.hash), line.RefToLocation(otherHash.location)) } + } else { + hashes[key] = &Hash{hashBytes, line.Location} } } @@ -244,16 +242,3 @@ func computePatchSha1Hex(patchFilename string) (string, error) { } return sprintf("%x", hasher.Sum(nil)), nil } - -func AutofixDistinfo(oldSha1, newSha1 string) { - distinfoFilename := G.Pkg.File(G.Pkg.DistinfoFile) - if lines := Load(distinfoFilename, NotEmpty|LogErrors); lines != nil { - for _, line := range lines.Lines { - fix := line.Autofix() - fix.Warnf(SilentAutofixFormat) - fix.Replace(oldSha1, newSha1) - fix.Apply() - } - SaveAutofixChanges(lines) - } -} diff --git a/pkgtools/pkglint/files/distinfo_test.go b/pkgtools/pkglint/files/distinfo_test.go index bc87240ce8f..d3927af5d85 100644 --- a/pkgtools/pkglint/files/distinfo_test.go +++ b/pkgtools/pkglint/files/distinfo_test.go @@ -34,6 +34,65 @@ func (s *Suite) Test_CheckLinesDistinfo(c *check.C) { "WARN: distinfo:9: Patch file \"patch-nonexistent\" does not exist in directory \"patches\".") } +func (s *Suite) Test_CheckLinesDistinfo__nonexistent_distfile_called_patch(c *check.C) { + t := s.Init(c) + + t.Chdir("category/package") + lines := t.SetUpFileLines("distinfo", + RcsID, + "", + "MD5 (patch-5.3.tar.gz) = 12345678901234567890123456789012", + "SHA1 (patch-5.3.tar.gz) = 1234567890123456789012345678901234567890") + G.Pkg = NewPackage(".") + + CheckLinesDistinfo(lines) + + // Even though the filename starts with "patch-" and therefore looks like + // a patch, it is a normal distfile because it has other hash algorithms + // than exactly SHA1. + t.CheckOutputLines( + "ERROR: distinfo:EOF: Expected SHA1, RMD160, SHA512, Size checksums " + + "for \"patch-5.3.tar.gz\", got MD5, SHA1.") +} + +func (s *Suite) Test_CheckLinesDistinfo__wrong_distfile_algorithms(c *check.C) { + t := s.Init(c) + + t.Chdir("category/package") + lines := t.SetUpFileLines("distinfo", + RcsID, + "", + "MD5 (distfile.tar.gz) = 12345678901234567890123456789012", + "SHA1 (distfile.tar.gz) = 1234567890123456789012345678901234567890") + + CheckLinesDistinfo(lines) + + t.CheckOutputLines( + "ERROR: distinfo:EOF: Expected SHA1, RMD160, SHA512, Size checksums " + + "for \"distfile.tar.gz\", got MD5, SHA1.") +} + +func (s *Suite) Test_CheckLinesDistinfo__wrong_patch_algorithms(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package") + t.Chdir("category/package") + t.CreateFileDummyPatch("patches/patch-aa") + t.SetUpFileLines("distinfo", + RcsID, + "", + "MD5 (patch-aa) = 12345678901234567890123456789012", + "SHA1 (patch-aa) = 1234567890123456789012345678901234567890") + + G.Check(".") + + t.CheckOutputLines( + "ERROR: distinfo:4: SHA1 hash of patches/patch-aa differs "+ + "(distinfo has 1234567890123456789012345678901234567890, "+ + "patch file has ebbf34b0641bcb508f17d5a27f2bf2a536d810ac).", + "ERROR: distinfo:EOF: Expected SHA1 hash for patch-aa, got MD5, SHA1.") +} + // When checking the complete pkgsrc tree, pkglint has all information it needs // to check whether different packages use the same distfile but require // different hashes for it. @@ -94,8 +153,8 @@ func (s *Suite) Test_distinfoLinesChecker_checkGlobalDistfileMismatch(c *check.C "7 errors and 1 warning found.") // Ensure that hex.DecodeString does not waste memory here. - t.Check(len(G.Pkgsrc.Hashes["SHA512:distfile-1.0.tar.gz"].hash), equals, 8) - t.Check(cap(G.Pkgsrc.Hashes["SHA512:distfile-1.0.tar.gz"].hash), equals, 8) + t.Check(len(G.Hashes["SHA512:distfile-1.0.tar.gz"].hash), equals, 8) + t.Check(cap(G.Hashes["SHA512:distfile-1.0.tar.gz"].hash), equals, 8) } func (s *Suite) Test_CheckLinesDistinfo__uncommitted_patch(c *check.C) { @@ -136,8 +195,8 @@ func (s *Suite) Test_CheckLinesDistinfo__unrecorded_patches(c *check.C) { G.checkdirPackage(".") t.CheckOutputLines( - "ERROR: distinfo: patch \"patches/patch-aa\" is not recorded. Run \""+confMake+" makepatchsum\".", - "ERROR: distinfo: patch \"patches/patch-src-Makefile\" is not recorded. Run \""+confMake+" makepatchsum\".") + "ERROR: distinfo: Patch \"patches/patch-aa\" is not recorded. Run \""+confMake+" makepatchsum\".", + "ERROR: distinfo: Patch \"patches/patch-src-Makefile\" is not recorded. Run \""+confMake+" makepatchsum\".") } // The distinfo file and the patches are usually placed in the package @@ -167,7 +226,7 @@ func (s *Suite) Test_CheckLinesDistinfo__relative_path_in_distinfo(c *check.C) { "(distinfo has ..., patch file has ebbf34b0641bcb508f17d5a27f2bf2a536d810ac).", "WARN: ../../other/common/distinfo:4: Patch file \"patch-only-in-distinfo\" "+ "does not exist in directory \"../../devel/patches/patches\".", - "ERROR: ../../other/common/distinfo: patch \"../../devel/patches/patches/patch-only-in-patches\" "+ + "ERROR: ../../other/common/distinfo: Patch \"../../devel/patches/patches/patch-only-in-patches\" "+ "is not recorded. Run \""+confMake+" makepatchsum\".") } @@ -197,7 +256,7 @@ func (s *Suite) Test_CheckLinesDistinfo__distinfo_and_patches_in_separate_direct "(distinfo has ..., patch file has ebbf34b0641bcb508f17d5a27f2bf2a536d810ac).", "WARN: ../../other/common/distinfo:4: Patch file \"patch-only-in-distinfo\" "+ "does not exist in directory \"patches\".", - "ERROR: ../../other/common/distinfo: patch \"patches/patch-only-in-patches\" "+ + "ERROR: ../../other/common/distinfo: Patch \"patches/patch-only-in-patches\" "+ "is not recorded. Run \""+confMake+" makepatchsum\".") } @@ -281,6 +340,26 @@ func (s *Suite) Test_CheckLinesDistinfo__missing_php_patches(c *check.C) { t.CheckOutputEmpty() } +func (s *Suite) Test_distinfoLinesChecker_checkUncommittedPatch(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package") + t.Chdir("category/package") + t.CreateFileDummyPatch("patches/patch-aa") + t.CreateFileLines("CVS/Entries", + "/distinfo/...") + t.CreateFileLines("patches/CVS/Entries", + "/patch-aa/...") + t.SetUpFileLines("distinfo", + RcsID, + "", + "SHA1 (patch-aa) = ebbf34b0641bcb508f17d5a27f2bf2a536d810ac") + + G.checkdirPackage(".") + + t.CheckOutputEmpty() +} + func (s *Suite) Test_distinfoLinesChecker_checkPatchSha1(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/files.go b/pkgtools/pkglint/files/files.go index b24cbfbe10e..a608e3aa49e 100644 --- a/pkgtools/pkglint/files/files.go +++ b/pkgtools/pkglint/files/files.go @@ -102,7 +102,7 @@ func nextLogicalLine(filename string, rawLines []*RawLine, index int) (Line, int func matchContinuationLine(textnl string) (leadingWhitespace, text, trailingWhitespace, cont string) { j := len(textnl) - if j > 0 && textnl[j-1] == '\n' { + if textnl[j-1] == '\n' { j-- } diff --git a/pkgtools/pkglint/files/getopt/getopt.go b/pkgtools/pkglint/files/getopt/getopt.go index 7d8f930cb8e..b101d0a2373 100644 --- a/pkgtools/pkglint/files/getopt/getopt.go +++ b/pkgtools/pkglint/files/getopt/getopt.go @@ -34,18 +34,55 @@ func (o *Options) AddFlagGroup(shortName rune, longName, argsName, description s return grp } +// AddFlagVar adds a boolean flag to the options. +// +// Example: +// var verbose bool +// +// opts := NewOptions() +// opts.AddFlagVar('v', "verbose", &verbose, false, "Enable verbose output") +// +// This option can be used in the following ways: +// -v +// --verbose +// --verbose=on (or yes, 1, true, enabled) +// --verbose=off (or no, 0, false, disabled) func (o *Options) AddFlagVar(shortName rune, longName string, pflag *bool, defval bool, description string) { *pflag = defval opt := option{shortName, longName, "", description, pflag} o.options = append(o.options, &opt) } +// AddStrVar adds a string option to the options. +// +// Example: +// var outputFilename string +// +// opts := NewOptions() +// opts.AddStrVar('o', "output", &outputFilename, "", "Write the output to the given file") +// +// This option can be used in the following ways: +// -o output.txt +// --output output.txt +// --output=output.txt func (o *Options) AddStrVar(shortName rune, longName string, pstr *string, defval string, description string) { *pstr = defval opt := option{shortName, longName, "", description, pstr} o.options = append(o.options, &opt) } +// AddStrList adds a string option to the options that can be used multiple times. +// +// Example: +// var includes []string +// +// opts := NewOptions() +// opts.AddStrList('i', "include", &includes, nil, "Include the files matching the pattern") +// +// This option can be used in the following ways: +// -i "*.txt" -i "*.docx" +// --include "*.txt" --include "*.md" +// --include="*.txt" --include="*.md" func (o *Options) AddStrList(shortName rune, longName string, plist *[]string, description string) { *plist = []string{} opt := option{shortName, longName, "", description, plist} diff --git a/pkgtools/pkglint/files/licenses.go b/pkgtools/pkglint/files/licenses.go index 0bb76201993..205a6fc5d1b 100644 --- a/pkgtools/pkglint/files/licenses.go +++ b/pkgtools/pkglint/files/licenses.go @@ -31,8 +31,8 @@ func (lc *LicenseChecker) checkName(license string) { } if licenseFile == "" { licenseFile = G.Pkgsrc.File("licenses/" + license) - if G.Pkgsrc.UsedLicenses != nil { - G.Pkgsrc.UsedLicenses[intern(license)] = true + if G.UsedLicenses != nil { + G.UsedLicenses[intern(license)] = true } } diff --git a/pkgtools/pkglint/files/line.go b/pkgtools/pkglint/files/line.go index 75ab70b3e63..19e6eb19c95 100644 --- a/pkgtools/pkglint/files/line.go +++ b/pkgtools/pkglint/files/line.go @@ -19,9 +19,16 @@ import ( ) type RawLine struct { - Lineno int // Counting starts at 1; 0 means inserted by Autofix - orignl string // The line as read in from the file, including newline - textnl string // The line as modified by Autofix, including newline + Lineno int // Counting starts at 1 + + // The line as read in from the file, including newline; + // can never be empty. Only in the very last line of each file, + // the trailing newline might be missing. + orignl string + + // The line as modified by Autofix, including newline; + // empty string for deleted lines. + textnl string // XXX: Since only Autofix needs to distinguish between orignl and textnl, // one of these fields should probably be moved there. @@ -145,9 +152,7 @@ func (line *LineImpl) showSource(out *SeparatorWriter) { for _, rawLine := range rawLines { if rawLine.textnl != rawLine.orignl { - if rawLine.orignl != "" { - writeLine("-\t", rawLine.orignl) - } + writeLine("-\t", rawLine.orignl) if rawLine.textnl != "" { writeLine("+\t", rawLine.textnl) } diff --git a/pkgtools/pkglint/files/line_test.go b/pkgtools/pkglint/files/line_test.go index 7093eea884d..d0c74fc1152 100644 --- a/pkgtools/pkglint/files/line_test.go +++ b/pkgtools/pkglint/files/line_test.go @@ -12,6 +12,23 @@ func (s *Suite) Test_RawLine_String(c *check.C) { c.Check(line.raw[0].String(), equals, "123:text\n") } +func (s *Suite) Test_NewLine__assertion(c *check.C) { + t := s.Init(c) + + t.ExpectPanic( + func() { NewLine("filename", 123, "text", nil) }, + "Pkglint internal error: use NewLineMulti for creating a Line with no RawLine attached to it") +} + +func (s *Suite) Test_Line_IsMultiline(c *check.C) { + t := s.Init(c) + + t.Check(t.NewLine("filename", 123, "text").IsMultiline(), equals, false) + t.Check(NewLineEOF("filename").IsMultiline(), equals, false) + + t.Check(NewLineMulti("filename", 123, 125, "text", nil).IsMultiline(), equals, true) +} + // In case of a fatal error, pkglint quits in a controlled manner, // and the trace log shows where the fatal error happened. func (s *Suite) Test_Line_Fatalf__trace(c *check.C) { diff --git a/pkgtools/pkglint/files/linelexer.go b/pkgtools/pkglint/files/linelexer.go index 6cfd49002e0..aa32309da70 100644 --- a/pkgtools/pkglint/files/linelexer.go +++ b/pkgtools/pkglint/files/linelexer.go @@ -1,9 +1,6 @@ package pkglint -import ( - "netbsd.org/pkglint/regex" - "strings" -) +import "netbsd.org/pkglint/regex" // LinesLexer records the state when checking a list of lines from top to bottom. type LinesLexer struct { @@ -66,7 +63,11 @@ func (llex *LinesLexer) SkipPrefix(prefix string) bool { defer trace.Call2(llex.CurrentLine().Text, prefix)() } - return !llex.EOF() && strings.HasPrefix(llex.lines.Lines[llex.index].Text, prefix) && llex.Skip() + if !llex.EOF() && hasPrefix(llex.lines.Lines[llex.index].Text, prefix) { + llex.Skip() + return true + } + return false } func (llex *LinesLexer) SkipString(text string) bool { @@ -74,7 +75,11 @@ func (llex *LinesLexer) SkipString(text string) bool { defer trace.Call2(llex.CurrentLine().Text, text)() } - return !llex.EOF() && llex.lines.Lines[llex.index].Text == text && llex.Skip() + if !llex.EOF() && llex.lines.Lines[llex.index].Text == text { + llex.Skip() + return true + } + return false } func (llex *LinesLexer) SkipEmptyOrNote() bool { @@ -135,5 +140,9 @@ func (mlex *MkLinesLexer) SkipWhile(pred func(mkline MkLine) bool) { } func (mlex *MkLinesLexer) SkipIf(pred func(mkline MkLine) bool) bool { - return !mlex.EOF() && pred(mlex.CurrentMkLine()) && mlex.Skip() + if !mlex.EOF() && pred(mlex.CurrentMkLine()) { + mlex.Skip() + return true + } + return false } diff --git a/pkgtools/pkglint/files/linelexer_test.go b/pkgtools/pkglint/files/linelexer_test.go index 99909db1172..6aa00b91962 100644 --- a/pkgtools/pkglint/files/linelexer_test.go +++ b/pkgtools/pkglint/files/linelexer_test.go @@ -34,3 +34,18 @@ func (s *Suite) Test_LinesLexer_SkipEmptyOrNote__end_of_file(c *check.C) { t.CheckOutputLines( "NOTE: file.txt:2: Empty line expected after this line.") } + +func (s *Suite) Test_LinesLexer_SkipPrefix(c *check.C) { + t := s.Init(c) + + lines := t.NewLines("file.txt", + "line 1", + "line 2") + llex := NewLinesLexer(lines) + + t.Check(llex.SkipPrefix("line 1"), equals, true) + t.Check(llex.SkipPrefix("line 1"), equals, false) + t.Check(llex.SkipPrefix("line 2"), equals, true) + t.Check(llex.SkipPrefix("line 2"), equals, false) + t.Check(llex.SkipPrefix(""), equals, false) +} diff --git a/pkgtools/pkglint/files/lines_test.go b/pkgtools/pkglint/files/lines_test.go index 7e1468bb971..7c2915efad0 100644 --- a/pkgtools/pkglint/files/lines_test.go +++ b/pkgtools/pkglint/files/lines_test.go @@ -60,4 +60,15 @@ func (s *Suite) Test_Lines_CheckRcsID__wip(c *check.C) { "AUTOFIX: ~/wip/package/file3.mk:1: Inserting a line \"# $"+"NetBSD$\" before this line.", "AUTOFIX: ~/wip/package/file4.mk:1: Inserting a line \"# $"+"NetBSD$\" before this line.", "AUTOFIX: ~/wip/package/file5.mk:1: Inserting a line \"# $"+"NetBSD$\" before this line.") + + // In production mode, this error is disabled since it doesn't provide + // enough benefit compared to the work it would produce. + // + // To make it useful, the majority of pkgsrc-wip packages would first + // have to follow this style. + G.Testing = false + + G.Check(t.File("wip/package")) + + t.CheckOutputEmpty() } diff --git a/pkgtools/pkglint/files/logging.go b/pkgtools/pkglint/files/logging.go index 091bf02b0cc..63d1490f214 100644 --- a/pkgtools/pkglint/files/logging.go +++ b/pkgtools/pkglint/files/logging.go @@ -4,6 +4,7 @@ import ( "bytes" "io" "netbsd.org/pkglint/histogram" + "netbsd.org/pkglint/textproc" "path" ) @@ -51,6 +52,7 @@ var ( var dummyLine = NewLineMulti("", 0, 0, "", nil) +// IsAutofix returns whether one of the --show-autofix or --autofix options is active. func (l *Logger) IsAutofix() bool { return l.Opts.Autofix || l.Opts.ShowAutofix } // Relevant decides and remembers whether the given diagnostic is relevant and should be logged. @@ -186,8 +188,17 @@ func (l *Logger) Logf(level *LogLevel, filename, lineno, format, msg string) { return } - if G.Testing && format != AutofixFormat && !hasSuffix(format, ": %s") && !hasSuffix(format, ". %s") { - G.Assertf(hasSuffix(format, "."), "Diagnostic format %q must end in a period.", format) + if G.Testing && format != AutofixFormat { + if textproc.Alpha.Contains(format[0]) { + G.Assertf( + textproc.Upper.Contains(format[0]), + "Diagnostic %q must start with an uppercase letter.", + format) + } + + if !hasSuffix(format, ": %s") && !hasSuffix(format, ". %s") { + G.Assertf(hasSuffix(format, "."), "Diagnostic format %q must end in a period.", format) + } } if filename != "" { diff --git a/pkgtools/pkglint/files/logging_test.go b/pkgtools/pkglint/files/logging_test.go index 455b2c8c85d..6f3fe96cb0c 100644 --- a/pkgtools/pkglint/files/logging_test.go +++ b/pkgtools/pkglint/files/logging_test.go @@ -2,6 +2,7 @@ package pkglint import ( "gopkg.in/check.v1" + "netbsd.org/pkglint/histogram" "strings" ) @@ -52,6 +53,67 @@ func (s *Suite) Test_Logger_Logf__mixed_with_Diag(c *check.C) { "ERROR: filename:3: Logf output 3.\n") } +func (s *Suite) Test_Logger_Logf__production(c *check.C) { + var sw strings.Builder + logger := Logger{out: NewSeparatorWriter(&sw)} + + // In production mode, the checks for the diagnostic messages are + // turned off, for performance reasons. The unit tests provide + // enough coverage. + G.Testing = false + logger.Logf(Error, "filename", "3", "diagnostic", "message") + + c.Check(sw.String(), equals, ""+ + "ERROR: filename:3: message\n") +} + +func (s *Suite) Test_Logger_Logf__profiling(c *check.C) { + t := s.Init(c) + + line := t.NewLine("filename", 123, "text") + + G.Opts.Profiling = true + G.Logger.histo = histogram.New() + line.Warnf("Warning.") + + G.Logger.histo.PrintStats(G.out.out, "loghisto", -1) + + t.CheckOutputLines( + "WARN: filename:123: Warning.", + "loghisto 1 Warning.") +} + +func (s *Suite) Test_Logger_Logf__profiling_autofix(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--show-autofix", "--source", "--explain") + line := t.NewLine("filename", 123, "text") + + G.Opts.Profiling = true + G.Logger.histo = histogram.New() + + fix := line.Autofix() + fix.Notef("Autofix note.") + fix.Explain( + "Autofix explanation.") + fix.Replace("text", "replacement") + fix.Apply() + + // The AUTOFIX line is not counted in the histogram although + // it uses the same code path as the other messages. + G.Logger.histo.PrintStats(G.out.out, "loghisto", -1) + + t.CheckOutputLines( + "NOTE: filename:123: Autofix note.", + "AUTOFIX: filename:123: Replacing \"text\" with \"replacement\".", + "-\ttext", + "+\treplacement", + "", + "\tAutofix explanation.", + "", + "loghisto 1 Autofix note.") +} + // Diag filters duplicate messages, unlike Logf. func (s *Suite) Test_Logger_Diag__duplicates(c *check.C) { t := s.Init(c) @@ -686,14 +748,17 @@ func (s *Suite) Test_Logger_Diag__source_duplicates(c *check.C) { G.Main("pkglint", "--source", "-Wall", t.File("category/package1"), t.File("category/package2")) t.CheckOutputLines( - "ERROR: ~/category/package1/distinfo: patch \"../dependency/patches/patch-aa\" "+ - "is not recorded. Run \""+confMake+" makepatchsum\".", + "ERROR: ~/category/package1/distinfo: "+ + "Patch \"../dependency/patches/patch-aa\" is not recorded. "+ + "Run \""+confMake+" makepatchsum\".", "", ">\t--- old file", - "ERROR: ~/category/dependency/patches/patch-aa:3: Each patch must be documented.", + "ERROR: ~/category/dependency/patches/patch-aa:3: "+ + "Each patch must be documented.", "", - "ERROR: ~/category/package2/distinfo: patch \"../dependency/patches/patch-aa\" "+ - "is not recorded. Run \""+confMake+" makepatchsum\".", + "ERROR: ~/category/package2/distinfo: "+ + "Patch \"../dependency/patches/patch-aa\" is not recorded. "+ + "Run \""+confMake+" makepatchsum\".", "", "3 errors and 0 warnings found.", "(Run \"pkglint -e\" to show explanations.)") diff --git a/pkgtools/pkglint/files/mkline.go b/pkgtools/pkglint/files/mkline.go index 3749e95275d..dd89fd0c8b0 100644 --- a/pkgtools/pkglint/files/mkline.go +++ b/pkgtools/pkglint/files/mkline.go @@ -354,10 +354,18 @@ func (mkline *MkLineImpl) Tokenize(text string, warn bool) []*MkToken { defer trace.Call(mkline, text)() } - p := NewMkParser(mkline.Line, text, true) - tokens := p.MkTokens() - if warn && p.Rest() != "" { - mkline.Warnf("Internal pkglint error in MkLine.Tokenize at %q.", p.Rest()) + var tokens []*MkToken + var rest string + if (mkline.IsVarassign() || mkline.IsCommentedVarassign()) && text == mkline.Value() { + tokens, rest = mkline.ValueTokens() + } else { + p := NewMkParser(mkline.Line, text, true) + tokens = p.MkTokens() + rest = p.Rest() + } + + if warn && rest != "" { + mkline.Warnf("Internal pkglint error in MkLine.Tokenize at %q.", rest) } return tokens } @@ -458,21 +466,21 @@ func (mkline *MkLineImpl) ValueFields(value string) []string { return split } -func (mkline *MkLineImpl) ValueTokens() []*MkToken { +func (mkline *MkLineImpl) ValueTokens() ([]*MkToken, string) { value := mkline.Value() if value == "" { - return nil + return nil, "" } assign := mkline.data.(mkLineAssign) if assign.valueMk != nil || assign.valueMkRest != "" { - return assign.valueMk + return assign.valueMk, assign.valueMkRest } p := NewMkParser(mkline.Line, value, true) assign.valueMk = p.MkTokens() assign.valueMkRest = p.Rest() - return assign.valueMk + return assign.valueMk, assign.valueMkRest } // Fields applies to variable assignments and .for loops. @@ -528,22 +536,23 @@ func (mkline *MkLineImpl) ResolveVarsInRelativePath(relativePath string) string } else { basedir = path.Dir(mkline.Filename) } - pkgsrcdir := relpath(basedir, G.Pkgsrc.File(".")) - - if G.Testing { - // Relative pkgsrc paths usually only contain two or three levels. - // A possible reason for reaching this assertion is: - // Tests that access the file system must create their lines - // using t.SetUpFileMkLines, not using t.NewMkLines. - G.Assertf(!contains(pkgsrcdir, "../../../../.."), - "Relative path %q for %q is too deep below the pkgsrc root %q.", - pkgsrcdir, basedir, G.Pkgsrc.File(".")) - } tmp := relativePath - tmp = strings.Replace(tmp, "${PKGSRCDIR}", pkgsrcdir, -1) - tmp = strings.Replace(tmp, "${.CURDIR}", ".", -1) - tmp = strings.Replace(tmp, "${.PARSEDIR}", ".", -1) + if contains(tmp, "PKGSRCDIR") { + pkgsrcdir := relpath(basedir, G.Pkgsrc.File(".")) + + if G.Testing { + // Relative pkgsrc paths usually only contain two or three levels. + // A possible reason for reaching this assertion is a pkglint unit test + // that uses t.NewMkLines instead of the correct t.SetUpFileMkLines. + G.Assertf(!contains(pkgsrcdir, "../../../../.."), + "Relative path %q for %q is too deep below the pkgsrc root %q.", + pkgsrcdir, basedir, G.Pkgsrc.File(".")) + } + tmp = strings.Replace(tmp, "${PKGSRCDIR}", pkgsrcdir, -1) + } + tmp = strings.Replace(tmp, "${.CURDIR}", ".", -1) // TODO: Replace with the "typical" os.Getwd(). + tmp = strings.Replace(tmp, "${.PARSEDIR}", ".", -1) // FIXME replaceLatest := func(varuse, category string, pattern regex.Pattern, replacement string) { if contains(tmp, varuse) { @@ -551,11 +560,16 @@ func (mkline *MkLineImpl) ResolveVarsInRelativePath(relativePath string) string tmp = strings.Replace(tmp, varuse, latest, -1) } } + + // These variables are only used in pkgsrc packages, therefore they + // are replaced with the fixed "../.." regardless of where the text appears. replaceLatest("${LUA_PKGSRCDIR}", "lang", `^lua[0-9]+$`, "../../lang/$0") replaceLatest("${PHPPKGSRCDIR}", "lang", `^php[0-9]+$`, "../../lang/$0") - replaceLatest("${SUSE_DIR_PREFIX}", "emulators", `^(suse[0-9]+)_base$`, "$1") replaceLatest("${PYPKGSRCDIR}", "lang", `^python[0-9]+$`, "../../lang/$0") + replaceLatest("${PYPACKAGE}", "lang", `^python[0-9]+$`, "$0") + replaceLatest("${SUSE_DIR_PREFIX}", "emulators", `^(suse[0-9]+)_base$`, "$1") + if G.Pkg != nil { // XXX: Even if these variables are defined indirectly, // pkglint should be able to resolve them properly. @@ -668,7 +682,7 @@ func (mkline *MkLineImpl) VariableNeedsQuoting(varname string, vartype *Vartype, } return no } - if vartype.kindOfList == lkShell && !vuc.IsWordPart { + if !vuc.IsWordPart { return no } } @@ -690,10 +704,8 @@ func (mkline *MkLineImpl) VariableNeedsQuoting(varname string, vartype *Vartype, // Both of these can be correct, depending on the situation: // 1. echo ${PERL5:Q} // 2. xargs ${PERL5} - if !vuc.IsWordPart && vuc.quoting == VucQuotPlain { - if wantList && haveList { - return unknown - } + if !vuc.IsWordPart && wantList && haveList { + return unknown } // Pkglint assumes that the tool definitions don't include very @@ -737,7 +749,7 @@ func (mkline *MkLineImpl) VariableNeedsQuoting(varname string, vartype *Vartype, // Bad: LDADD+= -l${LIBS} // Good: LDADD+= ${LIBS:S,^,-l,} - if wantList && haveList && vuc.IsWordPart { + if wantList { return yes } @@ -1139,17 +1151,6 @@ func (ind *Indentation) TrackAfter(mkline MkLine) { ind.top().depth += 2 } - if cond != nil { - ind.RememberUsedVariables(cond) - - cond.Walk(&MkCondCallback{ - Call: func(name string, arg string) { - if name == "exists" { - ind.AddCheckedFile(arg) - } - }}) - } - case "for", "ifdef", "ifndef": ind.top().depth += 2 @@ -1169,6 +1170,23 @@ func (ind *Indentation) TrackAfter(mkline MkLine) { } } + switch directive { + case "if", "elif": + cond := mkline.Cond() + if cond == nil { + break + } + + ind.RememberUsedVariables(cond) + + cond.Walk(&MkCondCallback{ + Call: func(name string, arg string) { + if name == "exists" { + ind.AddCheckedFile(arg) + } + }}) + } + if trace.Tracing { trace.Stepf("Indentation after line %s: %s", mkline.Linenos(), ind) } diff --git a/pkgtools/pkglint/files/mkline_test.go b/pkgtools/pkglint/files/mkline_test.go index 70dbaa6ab7c..e7236a35202 100644 --- a/pkgtools/pkglint/files/mkline_test.go +++ b/pkgtools/pkglint/files/mkline_test.go @@ -17,6 +17,20 @@ func (s *Suite) Test_NewMkLine__varassign(c *check.C) { c.Check(mkline.VarassignComment(), equals, "# varassign comment") } +func (s *Suite) Test_NewMkLine__varassign_space_around_operator(c *check.C) { + t := s.Init(c) + + t.SetUpCommandLine("--show-autofix", "--source") + t.NewMkLine("test.mk", 101, + "pkgbase = package") + + t.CheckOutputLines( + "NOTE: test.mk:101: Unnecessary space after variable name \"pkgbase\".", + "AUTOFIX: test.mk:101: Replacing \"pkgbase =\" with \"pkgbase=\".", + "-\tpkgbase = package", + "+\tpkgbase= package") +} + func (s *Suite) Test_NewMkLine__shellcmd(c *check.C) { t := s.Init(c) @@ -453,6 +467,25 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__URL_as_part_of_word_in_list(c t.CheckOutputEmpty() // Don't suggest to use ${HOMEPAGE:Q}. } +func (s *Suite) Test_MkLine_VariableNeedsQuoting__MASTER_SITES_and_HOMEPAGE(c *check.C) { + t := s.Init(c) + + t.SetUpVartypes() + mklines := t.NewMkLines("Makefile", + MkRcsID, + "MASTER_SITES=\t${HOMEPAGE}", + "MASTER_SITES=\t${PATH}", // Some nonsense just for branch coverage. + "HOMEPAGE=\t${MASTER_SITES}", + "HOMEPAGE=\t${BUILD_DIRS}") // Some nonsense just for branch coverage. + + mklines.Check() + + t.CheckOutputLines( + "WARN: Makefile:3: Please use ${PATH:Q} instead of ${PATH}.", + "WARN: Makefile:4: HOMEPAGE should not be defined in terms of MASTER_SITEs.", + "WARN: Makefile:5: Please use ${BUILD_DIRS:Q} instead of ${BUILD_DIRS}.") +} + // Before November 2018, pkglint did not parse $$(subshell) commands very well. // As a side effect, it sometimes issued wrong warnings about the :Q modifier. // @@ -743,6 +776,13 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__uncovered_cases(c *check.C) { "WARN: ~/Makefile:6: The variable PATH may not be set by any package.", "WARN: ~/Makefile:6: PREFIX should not be evaluated at load time.", "WARN: ~/Makefile:6: PATH should not be evaluated at load time.") + + // Just for branch coverage. + trace.Tracing = false + MkLineChecker{mklines.mklines[2]}.Check() + + t.CheckOutputLines( + "WARN: ~/Makefile:3: GO_SRCPATH is defined but not used.") } func (s *Suite) Test_MkLine__shell_varuse_in_backt_dquot(c *check.C) { @@ -832,6 +872,17 @@ func (s *Suite) Test_MkLine_ValueSplit(c *check.C) { "", "${VAR2}two", "words") + +} + +func (s *Suite) Test_MkLine_ValueSplit__invalid_argument(c *check.C) { + t := s.Init(c) + + mkline := t.NewMkLine("filename.mk", 123, "VAR=\tvalue") + + t.ExpectPanic( + func() { mkline.ValueSplit("value", "") }, + "Pkglint internal error: Separator must not be empty; use ValueFields to split on whitespace") } func (s *Suite) Test_MkLine_Fields__varassign(c *check.C) { @@ -934,8 +985,8 @@ func (s *Suite) Test_MkLine_ValueTokens(c *check.C) { testTokens := func(value string, expected ...*MkToken) { mkline := t.NewMkLine("Makefile", 1, "PATH=\t"+value) - split := mkline.ValueTokens() - c.Check(split, deepEquals, expected) + tokens, _ := mkline.ValueTokens() + c.Check(tokens, deepEquals, expected) } testTokens("#empty", @@ -957,14 +1008,46 @@ func (s *Suite) Test_MkLine_ValueTokens__caching(c *check.C) { t := s.Init(c) mkline := t.NewMkLine("Makefile", 1, "PATH=\tvalue ${UNFINISHED") - split := mkline.ValueTokens() + tokens, rest := mkline.ValueTokens() + + c.Check(tokens, deepEquals, []*MkToken{{"value ", nil}}) + c.Check(rest, equals, "${UNFINISHED") + + tokens2, rest2 := mkline.ValueTokens() // This time the slice is taken from the cache. + + // In Go, it's not possible to compare slices for reference equality. + c.Check(tokens2, deepEquals, tokens) + c.Check(rest2, equals, rest) +} + +func (s *Suite) Test_MkLine_ValueTokens__caching_parse_error(c *check.C) { + t := s.Init(c) - c.Check(split, deepEquals, []*MkToken{{"value ", nil}}) + mkline := t.NewMkLine("Makefile", 1, "PATH=\t${UNFINISHED") + tokens, rest := mkline.ValueTokens() - split2 := mkline.ValueTokens() // This time the slice is taken from the cache. + c.Check(tokens, check.IsNil) + c.Check(rest, equals, "${UNFINISHED") + + tokens2, rest2 := mkline.ValueTokens() // This time the slice is taken from the cache. // In Go, it's not possible to compare slices for reference equality. - c.Check(split2, deepEquals, split) + c.Check(tokens2, deepEquals, tokens) + c.Check(rest2, equals, rest) +} + +func (s *Suite) Test_MkLine_ValueTokens__warnings(c *check.C) { + t := s.Init(c) + + mklines := t.NewMkLines("Makefile", + MkRcsID, + "ROUND=\t$(ROUND)") + + mklines.mklines[1].ValueTokens() + mklines.Check() + + t.CheckOutputLines( + "WARN: Makefile:2: Please use curly braces {} instead of round parentheses () for ROUND.") } func (s *Suite) Test_MkLine_ResolveVarsInRelativePath(c *check.C) { @@ -983,6 +1066,7 @@ func (s *Suite) Test_MkLine_ResolveVarsInRelativePath(c *check.C) { } test("", ".") + test("${PKGSRCDIR}", ".") test("${LUA_PKGSRCDIR}", "../../lang/lua53") test("${PHPPKGSRCDIR}", "../../lang/php72") test("${SUSE_DIR_PREFIX}", "suse100") @@ -995,6 +1079,10 @@ func (s *Suite) Test_MkLine_ResolveVarsInRelativePath(c *check.C) { test("${FILESDIR}", "files") test("${PKGDIR}", ".") + + // Just for branch coverage. + G.Testing = false + test("${PKGSRCDIR}", "../..") } func (s *Suite) Test_MkLine_ResolveVarsInRelativePath__directory_depth(c *check.C) { @@ -1128,6 +1216,44 @@ func (s *Suite) Test_Indentation_RememberUsedVariables(c *check.C) { c.Check(ind.Varnames(), deepEquals, []string{"PKGREVISION"}) } +func (s *Suite) Test_Indentation_TrackAfter__checked_files(c *check.C) { + t := s.Init(c) + + mklines := t.NewMkLines("file.mk", + MkRcsID, + "", + ".if make(other.mk)", + ". include \"other.mk\"", + ".endif", + "", + ".if exists(checked.mk)", + ". include \"checked.mk\"", + ".elif exists(other-checked.mk)", + ". include \"other-checked.mk\"", + ".endif") + + mklines.Check() + + t.CheckOutputLines( + "ERROR: file.mk:4: Relative path \"other.mk\" does not exist.") +} + +func (s *Suite) Test_Indentation_TrackAfter__lonely_else(c *check.C) { + t := s.Init(c) + + mklines := t.NewMkLines("file.mk", + MkRcsID, + "", + ".else") + + mklines.Check() + + // Surprisingly, pkglint doesn't report an error about this trivial bug. + // This will be caught by bmake, though. Therefore the only purpose of + // this test is the branch coverage in the "top.mkline != nil" case. + t.CheckOutputEmpty() +} + func (s *Suite) Test_MkLine_DetermineUsedVariables(c *check.C) { t := s.Init(c) @@ -1214,8 +1340,58 @@ func (s *Suite) Test_matchMkDirective(c *check.C) { []interface{}{true, expectedIndent, expectedDirective, expectedArgs, expectedComment}) } - test(".if ${VAR} == value", "", "if", "${VAR} == value", "") - test(".\tendif # comment", "\t", "endif", "", "comment") - test(".if ${VAR} == \"#\"", "", "if", "${VAR} == \"", "\"") - test(".if ${VAR:[#]}", "", "if", "${VAR:[#]}", "") + test(".if ${VAR} == value", + "", "if", "${VAR} == value", "") + + test(".\tendif # comment", + "\t", "endif", "", "comment") + + test(".if ${VAR} == \"#\"", + "", "if", "${VAR} == \"", "\"") + + test(".if ${VAR:[#]}", + "", "if", "${VAR:[#]}", "") + + test(".if ${VAR} == \\", + "", "if", "${VAR} == \\", "") +} + +func (s *Suite) Test_MatchMkInclude(c *check.C) { + t := s.Init(c) + + test := func(input, expectedIndent, expectedDirective, expectedFilename string) { + m, indent, directive, args := MatchMkInclude(input) + c.Check( + []interface{}{m, indent, directive, args}, + deepEquals, + []interface{}{true, expectedIndent, expectedDirective, expectedFilename}) + } + + testFail := func(input string) { + m, _, _, _ := MatchMkInclude(input) + if m { + c.Errorf("Text %q unexpectedly matched.", input) + } + } + + testFail("") + testFail(".") + testFail(".include") + testFail(".include \"") + testFail(".include \"other.mk") + testFail(".include \"other.mk\" \"") + + test(".include \"other.mk\"", + "", "include", "other.mk") + + test(".include \"other.mk\"\t", + "", "include", "other.mk") + + test(".include \"other.mk\"\t#", + "", "include", "other.mk") + + test(".include \"other.mk\"\t# comment", + "", "include", "other.mk") + + t.CheckOutputEmpty() } diff --git a/pkgtools/pkglint/files/mklinechecker.go b/pkgtools/pkglint/files/mklinechecker.go index 9207a5b2be1..46b82d44ad9 100644 --- a/pkgtools/pkglint/files/mklinechecker.go +++ b/pkgtools/pkglint/files/mklinechecker.go @@ -4,6 +4,7 @@ import ( "netbsd.org/pkglint/regex" "os" "path" + "path/filepath" "strconv" "strings" ) @@ -298,9 +299,6 @@ func (ck MkLineChecker) checkVarassignLeftPermissions() { op := mkline.Op() vartype := G.Pkgsrc.VariableType(varname) if vartype == nil { - if trace.Tracing { - trace.Step1("No type definition found for %q.", varname) - } return } @@ -308,7 +306,7 @@ func (ck MkLineChecker) checkVarassignLeftPermissions() { // E.g. USE_TOOLS:= ${USE_TOOLS:Nunwanted-tool} if op == opAssignEval && perms&aclpAppend != 0 { - tokens := mkline.ValueTokens() + tokens, _ := mkline.ValueTokens() if len(tokens) == 1 && tokens[0].Varuse != nil && tokens[0].Varuse.varname == varname { return } @@ -563,15 +561,8 @@ func (ck MkLineChecker) checkVarusePermissions(varname string, vartype *Vartype, if !tool.UsableAtLoadTime(G.Mk.Tools.SeenPrefs) { ck.warnVaruseToolLoadTime(varname, tool) } - } else { - // Might the variable be used indirectly at load time, for example - // by assigning it to another variable which then gets evaluated? - isIndirect := vuc.time != vucTimeParse && // Otherwise it would be directly. - // The context might be used at load time somewhere. - vuc.vartype != nil && vuc.vartype.Union().Contains(aclpUseLoadtime) - - ck.warnVaruseLoadTime(varname, isIndirect) + ck.warnVaruseLoadTime(varname, indirectly) } } @@ -1051,10 +1042,6 @@ func (ck MkLineChecker) checkVartype(varname string, op MkOperator, value, comme defer trace.Call(varname, op, value, comment)() } - if !G.Opts.WarnTypes { - return - } - mkline := ck.MkLine vartype := G.Pkgsrc.VariableType(varname) @@ -1191,6 +1178,7 @@ func (ck MkLineChecker) checkDirectiveCond() { cond.Walk(&MkCondCallback{ Empty: ck.checkDirectiveCondEmpty, + Var: ck.checkDirectiveCondEmpty, CompareVarStr: checkCompareVarStr, VarUse: checkVarUse}) } @@ -1298,11 +1286,11 @@ func (ck MkLineChecker) CheckRelativePath(relativePath string, mustExist bool) { } abs := resolvedPath - if !hasPrefix(abs, "/") { + if !filepath.IsAbs(abs) { abs = path.Dir(mkline.Filename) + "/" + abs } if _, err := os.Stat(abs); err != nil { - if mustExist { + if mustExist && !(G.Mk != nil && G.Mk.indentation.IsCheckedFile(resolvedPath)) { mkline.Errorf("Relative path %q does not exist.", resolvedPath) } return diff --git a/pkgtools/pkglint/files/mklinechecker_test.go b/pkgtools/pkglint/files/mklinechecker_test.go index b6f926a63e1..b14dddb5861 100644 --- a/pkgtools/pkglint/files/mklinechecker_test.go +++ b/pkgtools/pkglint/files/mklinechecker_test.go @@ -34,18 +34,22 @@ func (s *Suite) Test_MkLineChecker_Check__buildlink3_include_prefs(c *check.C) { t.SetUpVartypes() t.CreateFileLines("mk/bsd.prefs.mk") + t.CreateFileLines("mk/bsd.fast.prefs.mk") mklines := t.SetUpFileMkLines("category/package/buildlink3.mk", - ".include \"../../mk/bsd.prefs.mk\"") + MkRcsID, + ".include \"../../mk/bsd.prefs.mk\"", + ".include \"../../mk/bsd.fast.prefs.mk\"") + // If the buildlink3.mk file doesn't actually exist, resolving the // relative path fails since that depends on the actual file system, // not on syntactical paths; see os.Stat in CheckRelativePath. // // TODO: Refactor relpath to be independent of a filesystem. - MkLineChecker{mklines.mklines[0]}.Check() + mklines.Check() t.CheckOutputLines( - "NOTE: ~/category/package/buildlink3.mk:1: For efficiency reasons, " + + "NOTE: ~/category/package/buildlink3.mk:2: For efficiency reasons, " + "please include bsd.fast.prefs.mk instead of bsd.prefs.mk.") } @@ -130,7 +134,7 @@ func (s *Suite) Test_MkLineChecker_checkDirective(c *check.C) { "", ".for var in a b c", ".endfor", - ".undef var") + ".undef var unrelated") mklines.Check() @@ -176,6 +180,67 @@ func (s *Suite) Test_MkLineChecker_checkDirective__for_loop_varname(c *check.C) "ERROR: filename.mk:12: Invalid variable name \"${VAR}\".") } +func (s *Suite) Test_MkLineChecker_checkDirectiveEnd__ending_comments(c *check.C) { + t := s.Init(c) + + t.SetUpVartypes() + mklines := t.NewMkLines("opsys.mk", + MkRcsID, + "", + ".for i in 1 2 3 4 5", + ". if ${OPSYS} == NetBSD", + ". if ${MACHINE_ARCH} == x86_64", + ". if ${OS_VERSION:M8.*}", + ". endif # MACHINE_ARCH", // Wrong, should be OS_VERSION. + ". endif # OS_VERSION", // Wrong, should be MACHINE_ARCH. + ". endif # OPSYS", // Correct. + ".endfor # j", // Wrong, should be i. + "", + ".if ${PKG_OPTIONS:Moption}", + ".endif # option", // Correct. + "", + ".if ${PKG_OPTIONS:Moption}", + ".endif # opti", // This typo goes unnoticed since "opti" is a substring of the condition. + "", + ".if ${OPSYS} == NetBSD", + ".elif ${OPSYS} == FreeBSD", + ".endif # NetBSD", // Wrong, should be FreeBSD from the .elif. + "", + ".for ii in 1 2", + ". for jj in 1 2", + ". endfor # ii", // Note: a simple "i" would not generate a warning because it is found in the word "in". + ".endfor # ii") + + // See MkLineChecker.checkDirective + mklines.Check() + + t.CheckOutputLines( + "WARN: opsys.mk:7: Comment \"MACHINE_ARCH\" does not match condition \"${OS_VERSION:M8.*}\".", + "WARN: opsys.mk:8: Comment \"OS_VERSION\" does not match condition \"${MACHINE_ARCH} == x86_64\".", + "WARN: opsys.mk:10: Comment \"j\" does not match loop \"i in 1 2 3 4 5\".", + "WARN: opsys.mk:12: Unknown option \"option\".", + "WARN: opsys.mk:20: Comment \"NetBSD\" does not match condition \"${OPSYS} == FreeBSD\".", + "WARN: opsys.mk:24: Comment \"ii\" does not match loop \"jj in 1 2\".") +} + +func (s *Suite) Test_MkLineChecker_checkDirectiveFor(c *check.C) { + t := s.Init(c) + + t.SetUpPkgsrc() + t.CreateFileLines("mk/file.mk", + MkRcsID, + ".for i = 1 2 3", // The "=" should rather be "in". + ".endfor", + "", + ".for _i_ in 1 2 3", // Underscores are only allowed in infrastructure files. + ".endfor") + + G.Check(t.File("mk/file.mk")) + + // Pkglint doesn't care about trivial syntax errors, bmake will already catch these. + t.CheckOutputEmpty() +} + func (s *Suite) Test_MkLineChecker_checkDependencyRule(c *check.C) { t := s.Init(c) @@ -200,7 +265,6 @@ func (s *Suite) Test_MkLineChecker_checkDependencyRule(c *check.C) { func (s *Suite) Test_MkLineChecker_checkVartype__simple_type(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Wtypes") t.SetUpVartypes() // Since COMMENT is defined in vardefs.go its type is certain instead of guessed. @@ -229,21 +293,6 @@ func (s *Suite) Test_MkLineChecker_checkVartype(c *check.C) { t.CheckOutputEmpty() } -// The command line option -Wno-types can be used to suppress the type checks. -// Suppressing it is rarely needed and comes from Feb 12 2005 when this feature was introduced. -// Since then the type system has matured and proven effective. -func (s *Suite) Test_MkLineChecker_checkVartype__skip(c *check.C) { - t := s.Init(c) - - t.SetUpCommandLine("-Wno-types") - t.SetUpVartypes() - mkline := t.NewMkLine("filename", 1, "DISTNAME=invalid:::distname") - - MkLineChecker{mkline}.Check() - - t.CheckOutputEmpty() -} - func (s *Suite) Test_MkLineChecker_checkVartype__append_to_non_list(c *check.C) { t := s.Init(c) @@ -279,7 +328,6 @@ func (s *Suite) Test_MkLineChecker_checkVarassign__URL_with_shell_special_charac func (s *Suite) Test_MkLineChecker_checkDirectiveCond(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Wtypes") t.SetUpVartypes() test := func(cond string, output ...string) { @@ -296,20 +344,28 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveCond(c *check.C) { "{ ccache ccc clang distcc f2c gcc hp icc ido "+ "mipspro mipspro-ucode pcc sunpro xlc } for PKGSRC_COMPILER.") - test(".elif ${A} != ${B}") + test(".elif ${A} != ${B}", + "WARN: filename:1: A is used but not defined.", + "WARN: filename:1: B is used but not defined.") test(".if ${HOMEPAGE} == \"mailto:someone@example.org\"", - "WARN: filename:1: \"mailto:someone@example.org\" is not a valid URL.") + "WARN: filename:1: \"mailto:someone@example.org\" is not a valid URL.", + "WARN: filename:1: HOMEPAGE should not be evaluated at load time.", + "WARN: filename:1: HOMEPAGE may not be used in any file; it is a write-only variable.") test(".if !empty(PKGSRC_RUN_TEST:M[Y][eE][sS])", "WARN: filename:1: PKGSRC_RUN_TEST should be matched "+ "against \"[yY][eE][sS]\" or \"[nN][oO]\", not \"[Y][eE][sS]\".") - test(".if !empty(IS_BUILTIN.Xfixes:M[yY][eE][sS])") + test(".if !empty(IS_BUILTIN.Xfixes:M[yY][eE][sS])", + "WARN: filename:1: IS_BUILTIN.Xfixes should not be evaluated at load time.", + "WARN: filename:1: IS_BUILTIN.Xfixes may not be used in this file; it would be ok in builtin.mk.") test(".if !empty(${IS_BUILTIN.Xfixes:M[yY][eE][sS]})", "WARN: filename:1: The empty() function takes a variable name as parameter, "+ - "not a variable expression.") + "not a variable expression.", + "WARN: filename:1: IS_BUILTIN.Xfixes should not be evaluated at load time.", + "WARN: filename:1: IS_BUILTIN.Xfixes may not be used in this file; it would be ok in builtin.mk.") test(".if ${PKGSRC_COMPILER} == \"msvc\"", "WARN: filename:1: \"msvc\" is not valid for PKGSRC_COMPILER. "+ @@ -317,7 +373,9 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveCond(c *check.C) { "WARN: filename:1: Use ${PKGSRC_COMPILER:Mmsvc} instead of the == operator.") test(".if ${PKG_LIBTOOL:Mlibtool}", - "NOTE: filename:1: PKG_LIBTOOL should be compared using == instead of matching against \":Mlibtool\".") + "NOTE: filename:1: PKG_LIBTOOL should be compared using == instead of matching against \":Mlibtool\".", + "WARN: filename:1: PKG_LIBTOOL should not be evaluated at load time.", + "WARN: filename:1: PKG_LIBTOOL may not be used in any file; it is a write-only variable.") test(".if ${MACHINE_PLATFORM:MUnknownOS-*-*} || ${MACHINE_ARCH:Mx86}", "WARN: filename:1: "+ @@ -336,7 +394,8 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveCond(c *check.C) { "NOTE: filename:1: MACHINE_ARCH should be compared using == instead of matching against \":Mx86\".") test(".if ${MASTER_SITES:Mftp://*} == \"ftp://netbsd.org/\"", - nil...) + "WARN: filename:1: MASTER_SITES should not be evaluated at load time.", + "WARN: filename:1: MASTER_SITES may not be used in any file; it is a write-only variable.") // The only interesting line from the below tracing output is the one // containing "checkCompareVarStr". @@ -350,6 +409,10 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveCond(c *check.C) { "TRACE: 1 2 + (*Pkgsrc).VariableType(\"VAR\")", "TRACE: 1 2 3 No type definition found for \"VAR\".", "TRACE: 1 2 - (*Pkgsrc).VariableType(\"VAR\", \"=>\", (*pkglint.Vartype)(nil))", + "WARN: filename:1: VAR is used but not defined.", + "TRACE: 1 2 + MkLineChecker.checkVarusePermissions(\"VAR\", (no-type time:parse quoting:plain wordpart:false))", + "TRACE: 1 2 3 No type definition found for \"VAR\".", + "TRACE: 1 2 - MkLineChecker.checkVarusePermissions(\"VAR\", (no-type time:parse quoting:plain wordpart:false))", "TRACE: 1 2 + (*MkLineImpl).VariableNeedsQuoting(\"VAR\", (*pkglint.Vartype)(nil), (no-type time:parse quoting:plain wordpart:false))", "TRACE: 1 2 - (*MkLineImpl).VariableNeedsQuoting(\"VAR\", (*pkglint.Vartype)(nil), (no-type time:parse quoting:plain wordpart:false), \"=>\", unknown)", "TRACE: 1 - MkLineChecker.CheckVaruse(filename:1, ${VAR:Mpattern1:Mpattern2}, (no-type time:parse quoting:plain wordpart:false))", @@ -375,19 +438,37 @@ func (s *Suite) Test_MkLineChecker_checkVarassign(c *check.C) { func (s *Suite) Test_MkLineChecker_checkVarassignLeftPermissions(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Wall,no-space") t.SetUpVartypes() + t.SetUpTool("awk", "AWK", AtRunTime) mklines := t.NewMkLines("options.mk", MkRcsID, - "PKG_DEVELOPER?= yes", - "BUILD_DEFS?= VARBASE") + "PKG_DEVELOPER?=\tyes", + "BUILD_DEFS?=\tVARBASE", + "USE_TOOLS:=\t${USE_TOOLS:Nunwanted-tool}", + "USE_TOOLS:=\t${MY_TOOLS}", + "USE_TOOLS:=\tawk") mklines.Check() t.CheckOutputLines( "WARN: options.mk:2: The variable PKG_DEVELOPER may not be given a default value by any package.", "WARN: options.mk:2: Please include \"../../mk/bsd.prefs.mk\" before using \"?=\".", - "WARN: options.mk:3: The variable BUILD_DEFS may not be given a default value (only appended to) in this file.") + "WARN: options.mk:3: The variable BUILD_DEFS may not be given a default value (only appended to) in this file.", + "WARN: options.mk:5: The variable USE_TOOLS may not be set (only appended to) in this file.", + "WARN: options.mk:5: MY_TOOLS is used but not defined.", + "WARN: options.mk:6: The variable USE_TOOLS may not be set (only appended to) in this file.") +} + +func (s *Suite) Test_MkLineChecker_checkVarassignLeftPermissions__no_tracing(c *check.C) { + t := s.Init(c) + + t.SetUpVartypes() + t.DisableTracing() // Just to reach branch coverage for unknown permissions. + mklines := t.NewMkLines("options.mk", + MkRcsID, + "COMMENT=\tShort package description") + + mklines.Check() } // Don't check the permissions for infrastructure files since they have their own rules. @@ -581,6 +662,51 @@ func (s *Suite) Test_MkLineChecker_checkVarusePermissions__PKGREVISION(c *check. "WARN: any.mk:2: PKGREVISION may not be used in any file; it is a write-only variable.") } +func (s *Suite) Test_MkLineChecker_checkVarusePermissions__indirectly(c *check.C) { + t := s.Init(c) + + t.SetUpVartypes() + mklines := t.NewMkLines("file.mk", + MkRcsID, + "IGNORE_PKG.package=\t${ONLY_FOR_UNPRIVILEGED}") + + mklines.Check() + + t.CheckOutputLines( + "WARN: file.mk:2: IGNORE_PKG.package should be set to YES or yes.", + "WARN: file.mk:2: ONLY_FOR_UNPRIVILEGED should not be evaluated indirectly at load time.") +} + +func (s *Suite) Test_MkLineChecker_warnVaruseToolLoadTime(c *check.C) { + t := s.Init(c) + + t.SetUpVartypes() + t.SetUpTool("nowhere", "NOWHERE", Nowhere) + t.SetUpTool("after-prefs", "AFTER_PREFS", AfterPrefsMk) + t.SetUpTool("at-runtime", "AT_RUNTIME", AtRunTime) + mklines := t.NewMkLines("Makefile", + MkRcsID, + ".if ${NOWHERE} && ${AFTER_PREFS} && ${AT_RUNTIME} && ${MK_TOOL}", + ".endif", + "", + "TOOLS_CREATE+=\t\tmk-tool", + "_TOOLS_VARNAME.mk-tool=\tMK_TOOL") + + mklines.Check() + + t.CheckOutputLines( + "WARN: Makefile:2: To use the tool ${NOWHERE} at load time, "+ + "it has to be added to USE_TOOLS before including bsd.prefs.mk.", + "WARN: Makefile:2: To use the tool ${AFTER_PREFS} at load time, "+ + "bsd.prefs.mk has to be included before.", + "WARN: Makefile:2: The tool ${AT_RUNTIME} cannot be used at load time.", + "WARN: Makefile:2: To use the tool ${MK_TOOL} at load time, "+ + "bsd.prefs.mk has to be included before.", + "WARN: Makefile:6: Variable names starting with an underscore "+ + "(_TOOLS_VARNAME.mk-tool) are reserved for internal pkgsrc use.", + "WARN: Makefile:6: _TOOLS_VARNAME.mk-tool is defined but not used.") +} + func (s *Suite) Test_MkLineChecker_Check__warn_varuse_LOCALBASE(c *check.C) { t := s.Init(c) @@ -764,6 +890,17 @@ func (s *Suite) Test_MkLineChecker_checkVartype__CFLAGS(c *check.C) { "WARN: Makefile:2: Compiler flag \"%s\\\\\\\"\" should start with a hyphen.") } +func (s *Suite) Test_MkLineChecker_checkDirectiveIndentation(c *check.C) { + t := s.Init(c) + + mkline := t.NewMkLine("filename.mk", 123, ".if 0") + + // Calling this method is only useful in the context of a whole file. + MkLineChecker{mkline}.checkDirectiveIndentation(4) + + t.CheckOutputEmpty() +} + func (s *Suite) Test_MkLineChecker_checkDirectiveIndentation__autofix(c *check.C) { t := s.Init(c) @@ -1021,7 +1158,64 @@ func (s *Suite) Test_MkLineChecker_CheckVaruse__build_defs(c *check.C) { "WARN: ~/options.mk:2: The user-defined variable VARBASE is used but not added to BUILD_DEFS.") } -func (s *Suite) Test_MkLineChecker_CheckVaruse__complicated_range(c *check.C) { +// The LOCALBASE variable may be defined and used in the infrastructure. +// It is always equivalent to PREFIX and only exists for historic reasons. +func (s *Suite) Test_MkLineChecker_CheckVaruse__LOCALBASE_in_infrastructure(c *check.C) { + t := s.Init(c) + + t.SetUpPkgsrc() + t.CreateFileLines("mk/infra.mk", + MkRcsID, + "LOCALBASE?=\t${PREFIX}", + "DEFAULT_PREFIX=\t${LOCALBASE}") + G.Pkgsrc.LoadInfrastructure() + + G.Check(t.File("mk/infra.mk")) + + // No warnings about LOCALBASE being used; in packages LOCALBASE is deprecated. + t.CheckOutputLines( + "WARN: ~/mk/infra.mk:2: PREFIX should not be evaluated indirectly at load time.") +} + +func (s *Suite) Test_MkLineChecker_CheckVaruse__user_defined_variable_and_BUILD_DEFS(c *check.C) { + t := s.Init(c) + + t.SetUpPkgsrc() + t.CreateFileLines("mk/defaults/mk.conf", + "VARBASE?=\t${PREFIX}/var", + "PYTHON_VER?=\t36") + G.Pkgsrc.LoadInfrastructure() + mklines := t.NewMkLines("file.mk", + MkRcsID, + "BUILD_DEFS+=\tPYTHON_VER", + "\t: ${VARBASE}", + "\t: ${VARBASE}", + "\t: ${PYTHON_VER}") + + mklines.Check() + + t.CheckOutputLines( + "WARN: file.mk:3: The user-defined variable VARBASE is used but not added to BUILD_DEFS.") +} + +func (s *Suite) Test_MkLineChecker_checkVaruseModifiersSuffix(c *check.C) { + t := s.Init(c) + + t.SetUpVartypes() + mklines := t.NewMkLines("file.mk", + MkRcsID, + "\t: ${HOMEPAGE:=subdir/:Q}", // wrong + "\t: ${BUILD_DIRS:=subdir/}", // correct + "\t: ${BIN_PROGRAMS:=.exe}") // unknown since BIN_PROGRAMS doesn't have a type + + mklines.Check() + + t.CheckOutputLines( + "WARN: file.mk:2: The :from=to modifier should only be used with lists, not with HOMEPAGE.", + "WARN: file.mk:4: BIN_PROGRAMS is used but not defined.") +} + +func (s *Suite) Test_MkLineChecker_checkVaruseModifiersRange(c *check.C) { t := s.Init(c) t.SetUpCommandLine("--show-autofix", "--source") @@ -1039,6 +1233,24 @@ func (s *Suite) Test_MkLineChecker_CheckVaruse__complicated_range(c *check.C) { "Replacing \":C/^/_asdf_/1:M_asdf_*:S/^_asdf_//\" with \":[1]\".", "-\tCC:=\t${CC:C/^/_asdf_/1:M_asdf_*:S/^_asdf_//}", "+\tCC:=\t${CC:[1]}") + + // Now go through all the "almost" cases, to reach full branch coverage. + mklines := t.NewMkLines("gcc.mk", + MkRcsID, + "\t: ${CC:M1:M2:M3}", + "\t: ${CC:C/^begin//:M2:M3}", // M1 pattern not exactly ^ + "\t: ${CC:C/^/_asdf_/g:M2:M3}", // M1 options != "1" + "\t: ${CC:C/^/....../g:M2:M3}", // M1 replacement doesn't match \w+ + "\t: ${CC:C/^/_asdf_/1:O:M3}", // M2 is not a match modifier + "\t: ${CC:C/^/_asdf_/1:N2:M3}", // M2 is :N instead of :M + "\t: ${CC:C/^/_asdf_/1:M_asdf_:M3}", // M2 pattern is missing the * at the end + "\t: ${CC:C/^/_asdf_/1:Mother:M3}", // M2 pattern differs from the M1 pattern + "\t: ${CC:C/^/_asdf_/1:M_asdf_*:M3}", // M3 ist not a substitution modifier + "\t: ${CC:C/^/_asdf_/1:M_asdf_*:S,from,to,}", // M3 pattern differs from the M1 pattern + "\t: ${CC:C/^/_asdf_/1:M_asdf_*:S,^_asdf_,to,}", // M3 replacement is not empty + "\t: ${CC:C/^/_asdf_/1:M_asdf_*:S,^_asdf_,,g}") // M3 modifier has options + + mklines.Check() } func (s *Suite) Test_MkLineChecker_CheckVaruse__deprecated_PKG_DEBUG(c *check.C) { diff --git a/pkgtools/pkglint/files/mklines.go b/pkgtools/pkglint/files/mklines.go index e77b485b328..5537052f1a5 100644 --- a/pkgtools/pkglint/files/mklines.go +++ b/pkgtools/pkglint/files/mklines.go @@ -405,18 +405,15 @@ func (mklines *MkLinesImpl) CheckRedundantAssignments() { scope := NewRedundantScope() isRelevant := func(old, new MkLine) bool { - if old.Basename != "Makefile" && new.Basename == "Makefile" { - return false - } if new.Op() == opAssignEval { return false } return true } - scope.OnIgnore = func(old, new MkLine) { + scope.OnRedundant = func(old, new MkLine) { if isRelevant(old, new) && old.Value() == new.Value() { - old.Notef("Definition of %s is redundant because of %s.", new.Varname(), old.RefTo(new)) + new.Notef("Definition of %s is redundant because of %s.", old.Varname(), new.RefTo(old)) } } diff --git a/pkgtools/pkglint/files/mklines_test.go b/pkgtools/pkglint/files/mklines_test.go index 7890c651bb2..2e7a33fd019 100644 --- a/pkgtools/pkglint/files/mklines_test.go +++ b/pkgtools/pkglint/files/mklines_test.go @@ -511,43 +511,6 @@ func (s *Suite) Test_MkLines_Check__indentation_include(c *check.C) { "NOTE: ~/module.mk:7: This directive should be indented by 2 spaces.") } -func (s *Suite) Test_MkLines_Check__endif_comment(c *check.C) { - t := s.Init(c) - - t.SetUpVartypes() - mklines := t.NewMkLines("opsys.mk", - MkRcsID, - "", - ".for i in 1 2 3 4 5", - ". if ${OPSYS} == NetBSD", - ". if ${MACHINE_ARCH} == x86_64", - ". if ${OS_VERSION:M8.*}", - ". endif # MACHINE_ARCH", // Wrong, should be OS_VERSION. - ". endif # OS_VERSION", // Wrong, should be MACHINE_ARCH. - ". endif # OPSYS", // Correct. - ".endfor # j", // Wrong, should be i. - "", - ".if ${PKG_OPTIONS:Moption}", - ".endif # option", // Correct. - "", - ".if ${PKG_OPTIONS:Moption}", - ".endif # opti", // This typo goes unnoticed since "opti" is a substring of the condition. - "", - ".if ${OPSYS} == NetBSD", - ".elif ${OPSYS} == FreeBSD", - ".endif # NetBSD") // Wrong, should be FreeBSD from the .elif. - - // See MkLineChecker.checkDirective - mklines.Check() - - t.CheckOutputLines( - "WARN: opsys.mk:7: Comment \"MACHINE_ARCH\" does not match condition \"${OS_VERSION:M8.*}\".", - "WARN: opsys.mk:8: Comment \"OS_VERSION\" does not match condition \"${MACHINE_ARCH} == x86_64\".", - "WARN: opsys.mk:10: Comment \"j\" does not match loop \"i in 1 2 3 4 5\".", - "WARN: opsys.mk:12: Unknown option \"option\".", - "WARN: opsys.mk:20: Comment \"NetBSD\" does not match condition \"${OPSYS} == FreeBSD\".") -} - func (s *Suite) Test_MkLines_Check__unfinished_directives(c *check.C) { t := s.Init(c) @@ -747,25 +710,22 @@ func (s *Suite) Test_MkLines_CheckRedundantAssignments__override_in_mk(c *check. "OVERRIDE=\tprevious value", "REDUNDANT=\tredundant") including := t.NewMkLines("including.mk", + ".include \"included.mk\"", "OVERRIDE=\toverridden value", "REDUNDANT=\tredundant") var allLines []Line + allLines = append(allLines, including.lines.Lines[:1]...) allLines = append(allLines, included.lines.Lines...) - allLines = append(allLines, including.lines.Lines...) + allLines = append(allLines, including.lines.Lines[1:]...) mklines := NewMkLines(NewLines(included.lines.FileName, allLines)) // XXX: The warnings from here are not in the same order as the other warnings. // XXX: There may be some warnings for the same file separated by warnings for other files. mklines.CheckRedundantAssignments() - // No warning for VAR=... in Makefile since it makes sense to have common files - // with default values for variables, overriding some of them in each package. t.CheckOutputLines( - // FIXME: The below warning is wrong because overwriting in a different file is ok. - "WARN: included.mk:1: Variable OVERRIDE is overwritten in including.mk:1.", - // FIXME: It's the other way round: including.mk:2 is redundant because of included.mk:2. - "NOTE: included.mk:2: Definition of REDUNDANT is redundant because of including.mk:2.") + "NOTE: including.mk:3: Definition of REDUNDANT is redundant because of included.mk:2.") } func (s *Suite) Test_MkLines_CheckRedundantAssignments__override_in_Makefile(c *check.C) { @@ -774,13 +734,15 @@ func (s *Suite) Test_MkLines_CheckRedundantAssignments__override_in_Makefile(c * "VAR=\tvalue ${OTHER}", "VAR?=\tvalue ${OTHER}", "VAR=\tnew value") - makefile := t.NewMkLines("Makefile", + including := t.NewMkLines("Makefile", + ".include \"module.mk\"", "VAR=\tthe package may overwrite variables from other files") var allLines []Line + allLines = append(allLines, including.lines.Lines[:1]...) allLines = append(allLines, included.lines.Lines...) - allLines = append(allLines, makefile.lines.Lines...) - mklines := NewMkLines(NewLines(included.lines.FileName, allLines)) + allLines = append(allLines, including.lines.Lines[1:]...) + mklines := NewMkLines(NewLines(including.lines.FileName, allLines)) // XXX: The warnings from here are not in the same order as the other warnings. // XXX: There may be some warnings for the same file separated by warnings for other files. @@ -789,7 +751,7 @@ func (s *Suite) Test_MkLines_CheckRedundantAssignments__override_in_Makefile(c * // No warning for VAR=... in Makefile since it makes sense to have common files // with default values for variables, overriding some of them in each package. t.CheckOutputLines( - "NOTE: module.mk:1: Definition of VAR is redundant because of line 2.", + "NOTE: module.mk:2: Definition of VAR is redundant because of line 1.", "WARN: module.mk:1: Variable VAR is overwritten in line 3.") } @@ -826,7 +788,7 @@ func (s *Suite) Test_MkLines_CheckRedundantAssignments__overwrite_same_value(c * mklines.CheckRedundantAssignments() t.CheckOutputLines( - "NOTE: module.mk:1: Definition of VAR is redundant because of line 2.") + "NOTE: module.mk:2: Definition of VAR is redundant because of line 1.") } func (s *Suite) Test_MkLines_CheckRedundantAssignments__conditional_overwrite(c *check.C) { @@ -869,7 +831,7 @@ func (s *Suite) Test_MkLines_CheckRedundantAssignments__overwrite_same_variable_ t.CheckOutputLines( "WARN: module.mk:1: Variable OTHER is overwritten in line 3.", - "NOTE: module.mk:2: Definition of VAR is redundant because of line 4.") + "NOTE: module.mk:4: Definition of VAR is redundant because of line 2.") } func (s *Suite) Test_MkLines_CheckRedundantAssignments__overwrite_different_value_used_between(c *check.C) { @@ -894,8 +856,7 @@ func (s *Suite) Test_MkLines_CheckRedundantAssignments__overwrite_different_valu t.CheckOutputLines( "WARN: module.mk:1: Variable OTHER is overwritten in line 4.", - // FIXME: It's the other way round: line 6 is redundant because of line 2. - "NOTE: module.mk:2: Definition of VAR is redundant because of line 6.") + "NOTE: module.mk:6: Definition of VAR is redundant because of line 2.") } func (s *Suite) Test_MkLines_CheckRedundantAssignments__procedure_call(c *check.C) { diff --git a/pkgtools/pkglint/files/mkparser.go b/pkgtools/pkglint/files/mkparser.go index 7193b9393c3..b10888013ea 100644 --- a/pkgtools/pkglint/files/mkparser.go +++ b/pkgtools/pkglint/files/mkparser.go @@ -27,8 +27,6 @@ func (p *MkParser) Rest() string { // // The text argument is assumed to be after unescaping the # character, // which means the # is a normal character and does not introduce a Makefile comment. -// -// TODO: Remove the emitWarnings argument in order to separate parsing from checking. func NewMkParser(line Line, text string, emitWarnings bool) *MkParser { G.Assertf((line != nil) == emitWarnings, "line must be given iff emitWarnings is set") return &MkParser{line, textproc.NewLexer(text), emitWarnings} @@ -441,7 +439,7 @@ func (p *MkParser) mkCondAtom() MkCond { m := lexer.NextRegexp(G.res.Compile(`^[\t ]*(<|<=|==|!=|>=|>)[\t ]*`)) if m == nil { - return &mkCond{Not: &mkCond{Empty: lhs}} // See devel/bmake/files/cond.c:/\* For \.if \$/ + return &mkCond{Var: lhs} // See devel/bmake/files/cond.c:/\* For \.if \$/ } op := m[1] @@ -697,6 +695,7 @@ type mkCond struct { Defined string Empty *MkVarUse + Var *MkVarUse CompareVarNum *MkCondCompareVarNum CompareVarStr *MkCondCompareVarStr CompareVarVar *MkCondCompareVarVar @@ -730,6 +729,7 @@ type MkCondCallback struct { CompareVarStr func(varuse *MkVarUse, op string, str string) CompareVarVar func(left *MkVarUse, op string, right *MkVarUse) Call func(name string, arg string) + Var func(varuse *MkVarUse) VarUse func(varuse *MkVarUse) } @@ -764,6 +764,14 @@ func (w *MkCondWalker) Walk(cond MkCond, callback *MkCondCallback) { callback.VarUse(&MkVarUse{cond.Defined, nil}) } + case cond.Var != nil: + if callback.Var != nil { + callback.Var(cond.Var) + } + if callback.VarUse != nil { + callback.VarUse(cond.Var) + } + case cond.Empty != nil: if callback.Empty != nil { callback.Empty(cond.Empty) diff --git a/pkgtools/pkglint/files/mkparser_test.go b/pkgtools/pkglint/files/mkparser_test.go index 8daa6a9dd89..26d9976411e 100644 --- a/pkgtools/pkglint/files/mkparser_test.go +++ b/pkgtools/pkglint/files/mkparser_test.go @@ -375,12 +375,8 @@ func (s *Suite) Test_MkParser_MkCond(c *check.C) { } varuse := NewMkVarUse - // TODO: Add tests for &&, ||, !. - - // TODO: Add test for !empty(VAR:M}). - test("${OPSYS:MNetBSD}", - &mkCond{Not: &mkCond{Empty: varuse("OPSYS", "MNetBSD")}}) + &mkCond{Var: varuse("OPSYS", "MNetBSD")}) test("defined(VARNAME)", &mkCond{Defined: "VARNAME"}) @@ -443,8 +439,8 @@ func (s *Suite) Test_MkParser_MkCond(c *check.C) { test("${MACHINE_ARCH:Mi386} || ${MACHINE_OPSYS:MNetBSD}", &mkCond{Or: []*mkCond{ - {Not: &mkCond{Empty: varuse("MACHINE_ARCH", "Mi386")}}, - {Not: &mkCond{Empty: varuse("MACHINE_OPSYS", "MNetBSD")}}}}) + {Var: varuse("MACHINE_ARCH", "Mi386")}, + {Var: varuse("MACHINE_OPSYS", "MNetBSD")}}}) // Exotic cases @@ -485,7 +481,7 @@ func (s *Suite) Test_MkParser_MkCond(c *check.C) { &mkCond{Defined: "VARNAME"}) test("${\"${PKG_OPTIONS:Moption}\":?--enable-option:--disable-option}", - &mkCond{Not: &mkCond{Empty: varuse("\"${PKG_OPTIONS:Moption}\"", "?--enable-option:--disable-option")}}) + &mkCond{Var: varuse("\"${PKG_OPTIONS:Moption}\"", "?--enable-option:--disable-option")}) // Errors @@ -494,7 +490,7 @@ func (s *Suite) Test_MkParser_MkCond(c *check.C) { "|| defined(PKG_OPTIONS:Msamplerate)") testRest("${LEFT} &&", - &mkCond{Not: &mkCond{Empty: varuse("LEFT")}}, + &mkCond{Var: varuse("LEFT")}, "&&") testRest("\"unfinished string literal", @@ -821,6 +817,9 @@ func (s *Suite) Test_MkCondWalker_Walk(c *check.C) { Call: func(name string, arg string) { addEvent("call", name, arg) }, + Var: func(varuse *MkVarUse) { + addEvent("var", varuseStr(varuse)) + }, VarUse: func(varuse *MkVarUse) { addEvent("varUse", varuseStr(varuse)) }}) @@ -836,6 +835,6 @@ func (s *Suite) Test_MkCondWalker_Walk(c *check.C) { " defined VAR", " varUse VAR", " call exists, file.mk", - " empty NONEMPTY", + " var NONEMPTY", " varUse NONEMPTY"}) } diff --git a/pkgtools/pkglint/files/mktypes.go b/pkgtools/pkglint/files/mktypes.go index 1ded441ae53..2d3354dd4c5 100644 --- a/pkgtools/pkglint/files/mktypes.go +++ b/pkgtools/pkglint/files/mktypes.go @@ -133,9 +133,8 @@ func (vu *MkVarUse) Mod() string { return mod.String() } -// IsExpression returns whether the varname is interpreted as a variable -// name (the usual case) or as an expression (rare, only the modifiers -// "?:" and "L" do this). +// IsExpression returns whether the varname is interpreted as an expression +// instead of a variable name (rare, only the modifiers :? and :L do this). func (vu *MkVarUse) IsExpression() bool { if len(vu.modifiers) == 0 { return false diff --git a/pkgtools/pkglint/files/options.go b/pkgtools/pkglint/files/options.go index b8403f265ca..ab7c31daa6c 100755 --- a/pkgtools/pkglint/files/options.go +++ b/pkgtools/pkglint/files/options.go @@ -73,18 +73,20 @@ loop: continue } - cond.Walk(&MkCondCallback{ - Empty: func(varuse *MkVarUse) { - if varuse.varname == "PKG_OPTIONS" && len(varuse.modifiers) == 1 { - if m, positive, pattern := varuse.modifiers[0].MatchMatch(); m && positive { - option := pattern - if !containsVarRef(option) { - handledOptions[option] = mkline - optionsInDeclarationOrder = append(optionsInDeclarationOrder, option) - } + recordUsedOption := func(varuse *MkVarUse) { + if varuse.varname == "PKG_OPTIONS" && len(varuse.modifiers) == 1 { + if m, positive, pattern := varuse.modifiers[0].MatchMatch(); m && positive { + option := pattern + if !containsVarRef(option) { + handledOptions[option] = mkline + optionsInDeclarationOrder = append(optionsInDeclarationOrder, option) } } - }}) + } + } + cond.Walk(&MkCondCallback{ + Empty: recordUsedOption, + Var: recordUsedOption}) if cond.Empty != nil && mkline.HasElseBranch() { mkline.Notef("The positive branch of the .if/.else should be the one where the option is set.") @@ -101,14 +103,14 @@ loop: declared := declaredOptions[option] handled := handledOptions[option] - if handled == nil { + switch { + case handled == nil: declared.Warnf("Option %q should be handled below in an .if block.", option) G.Explain( "If an option is not processed in this file, it may either be a", "typo, or the option does not have any effect.") - } - if declared == nil { + case declared == nil: handled.Warnf("Option %q is handled but not added to PKG_SUPPORTED_OPTIONS.", option) G.Explain( "This block of code will never be run since PKG_OPTIONS cannot", @@ -117,5 +119,5 @@ loop: } } - SaveAutofixChanges(mklines.lines) + mklines.SaveAutofixChanges() } diff --git a/pkgtools/pkglint/files/options_test.go b/pkgtools/pkglint/files/options_test.go index dbc31d4d06d..1f98bbcecdf 100755 --- a/pkgtools/pkglint/files/options_test.go +++ b/pkgtools/pkglint/files/options_test.go @@ -192,3 +192,44 @@ func (s *Suite) Test_CheckLinesOptionsMk__malformed_condition(c *check.C) { t.CheckOutputLines( "WARN: ~/category/package/options.mk:13: Invalid condition, unrecognized part: \"${OPSYS} == 'Darwin'\".") } + +func (s *Suite) Test_CheckLinesOptionsMk__PLIST_VARS_based_on_PKG_SUPPORTED_OPTIONS(c *check.C) { + t := s.Init(c) + + t.SetUpOption("one", "") + t.SetUpOption("two", "") + t.SetUpOption("three", "") + t.SetUpPackage("category/package") + t.CreateFileLines("mk/bsd.options.mk") + t.SetUpFileMkLines("category/package/options.mk", + MkRcsID, + "", + "PKG_OPTIONS_VAR=\tPKG_OPTIONS.package", + "PKG_SUPPORTED_OPTIONS+=\tone", + "PKG_SUPPORTED_OPTIONS+=\ttwo", + "PKG_SUPPORTED_OPTIONS+=\tthree", + "", + ".include \"../../mk/bsd.options.mk\"", + "", + "PLIST_VARS+=\t${PKG_SUPPORTED_OPTIONS}", + "", + ".if ${PKG_OPTIONS:Mone}", + "PLIST.one=\tyes", + ".endif", + "", + ".if ${PKG_OPTIONS:Mthree}", + "PLIST.three=\tyes", + ".endif") + t.Chdir("category/package") + + G.Check(".") + + // Even though PLIST_VARS is defined indirectly by referencing + // PKG_SUPPORTED_OPTIONS and that variable is defined in several + // lines, pkglint gets all the facts correct and knows that + // only PLIST.two is missing. + t.CheckOutputLines( + "WARN: options.mk:10: "+ + "\"two\" is added to PLIST_VARS, but PLIST.two is not defined in this file.", + "WARN: options.mk:5: Option \"two\" should be handled below in an .if block.") +} diff --git a/pkgtools/pkglint/files/package.go b/pkgtools/pkglint/files/package.go index cb255ac3219..501210ed736 100644 --- a/pkgtools/pkglint/files/package.go +++ b/pkgtools/pkglint/files/package.go @@ -185,10 +185,10 @@ func (pkg *Package) loadPackageMakefile() MkLines { allLines.collectUsedVariables() allLines.CheckRedundantAssignments() - pkg.Pkgdir, _ = pkg.vars.Value("PKGDIR") - pkg.DistinfoFile, _ = pkg.vars.Value("DISTINFO_FILE") - pkg.Filesdir, _ = pkg.vars.Value("FILESDIR") - pkg.Patchdir, _ = pkg.vars.Value("PATCHDIR") + pkg.Pkgdir = pkg.vars.LastValue("PKGDIR") + pkg.DistinfoFile = pkg.vars.LastValue("DISTINFO_FILE") + pkg.Filesdir = pkg.vars.LastValue("FILESDIR") + pkg.Patchdir = pkg.vars.LastValue("PATCHDIR") // See lang/php/ext.mk if varIsDefinedSimilar("PHPEXT_MK") { @@ -438,6 +438,11 @@ func (pkg *Package) checkGnuConfigureUseLanguages() { vars := pkg.vars if gnuLine := vars.FirstDefinition("GNU_CONFIGURE"); gnuLine != nil { + + // FIXME: Instead of using the first definition here, a better approach + // is probably to use all the definitions except those from mk/compiler.mk. + // In real pkgsrc, the last definition is typically from mk/compiler.mk + // and only contains c++. if useLine := vars.FirstDefinition("USE_LANGUAGES"); useLine != nil { if matches(useLine.VarassignComment(), `(?-i)\b(?:c|empty|none)\b`) { @@ -460,7 +465,7 @@ func (pkg *Package) checkGnuConfigureUseLanguages() { // It is only used inside pkgsrc to mark changes that are // independent from the upstream package. func (pkg *Package) nbPart() string { - pkgrevision, _ := pkg.vars.Value("PKGREVISION") + pkgrevision := pkg.vars.LastValue("PKGREVISION") if rev, err := strconv.Atoi(pkgrevision); err == nil { return "nb" + strconv.Itoa(rev) } @@ -591,7 +596,7 @@ func (pkg *Package) CheckVarorder(mklines MkLines) { defer trace.Call0()() } - if !G.Opts.WarnOrder || pkg.seenMakefileCommon { + if pkg.seenMakefileCommon { return } @@ -666,12 +671,10 @@ func (pkg *Package) CheckVarorder(mklines MkLines) { variable("TOOL_DEPENDS", many), variable("DEPENDS", many))} - firstRelevant := -1 - lastRelevant := -1 + relevantLines := (func() []MkLine { + firstRelevant := -1 + lastRelevant := -1 - // TODO: understand and explain this code. - // It is much longer and much more complicated than it should be. - skip := func() bool { relevantVars := make(map[string]bool) for _, section := range sections { for _, variable := range section.vars { @@ -693,7 +696,7 @@ func (pkg *Package) CheckVarorder(mklines MkLines) { trace.Stepf("Skipping varorder because of line %s.", mklines.mklines[firstIrrelevant].Linenos()) } - return true + return nil } lastRelevant = i } else { @@ -713,9 +716,13 @@ func (pkg *Package) CheckVarorder(mklines MkLines) { } if firstRelevant == -1 { - return true + return nil } - interesting := mklines.mklines[firstRelevant : lastRelevant+1] + return mklines.mklines[firstRelevant : lastRelevant+1] + })() + + skip := func() bool { + interesting := relevantLines varcanon := func() string { for len(interesting) > 0 && interesting[0].IsComment() { @@ -760,7 +767,7 @@ func (pkg *Package) CheckVarorder(mklines MkLines) { return len(interesting) == 0 } - if skip() { + if len(relevantLines) == 0 || skip() { return } @@ -768,7 +775,7 @@ func (pkg *Package) CheckVarorder(mklines MkLines) { for _, section := range sections { for _, variable := range section.vars { found := false - for _, mkline := range mklines.mklines[firstRelevant : lastRelevant+1] { + for _, mkline := range relevantLines { if mkline.IsVarassign() || mkline.IsCommentedVarassign() { if mkline.Varcanon() == variable.varname { canonical = append(canonical, mkline.Varname()) @@ -791,7 +798,7 @@ func (pkg *Package) CheckVarorder(mklines MkLines) { // TODO: This leads to very long and complicated warnings. // Those parts that are correct should not be mentioned, // except if they are helpful for locating the mistakes. - mkline := mklines.mklines[firstRelevant] + mkline := relevantLines[0] mkline.Warnf("The canonical order of the variables is %s.", strings.Join(canonical, ", ")) G.Explain( "In simple package Makefiles, some common variables should be", @@ -812,8 +819,8 @@ func (pkg *Package) checkLocallyModified(filename string) { defer trace.Call(filename)() } - owner, _ := pkg.vars.Value("OWNER") - maintainer, _ := pkg.vars.Value("MAINTAINER") + owner := pkg.vars.LastValue("OWNER") + maintainer := pkg.vars.LastValue("MAINTAINER") if maintainer == "pkgsrc-users@NetBSD.org" { maintainer = "" } @@ -898,6 +905,19 @@ func (pkg *Package) loadPlistDirs(plistFilename string) { } } +func (pkg *Package) AutofixDistinfo(oldSha1, newSha1 string) { + distinfoFilename := pkg.File(pkg.DistinfoFile) + if lines := Load(distinfoFilename, NotEmpty|LogErrors); lines != nil { + for _, line := range lines.Lines { + fix := line.Autofix() + fix.Warnf(SilentAutofixFormat) + fix.Replace(oldSha1, newSha1) + fix.Apply() + } + lines.SaveAutofixChanges() + } +} + type PlistContent struct { Dirs map[string]bool Files map[string]bool diff --git a/pkgtools/pkglint/files/package_test.go b/pkgtools/pkglint/files/package_test.go index dd29e089390..6162c4c4c80 100644 --- a/pkgtools/pkglint/files/package_test.go +++ b/pkgtools/pkglint/files/package_test.go @@ -72,7 +72,6 @@ func (s *Suite) Test_Package_pkgnameFromDistname(c *check.C) { func (s *Suite) Test_Package_CheckVarorder(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Worder") pkg := NewPackage(t.File("x11/9term")) pkg.CheckVarorder(t.NewMkLines("Makefile", @@ -106,7 +105,6 @@ func (s *Suite) Test_Package_CheckVarorder(c *check.C) { func (s *Suite) Test_Package_CheckVarorder__comments_do_not_crash(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Worder") pkg := NewPackage(t.File("x11/9term")) pkg.CheckVarorder(t.NewMkLines("Makefile", @@ -129,8 +127,6 @@ func (s *Suite) Test_Package_CheckVarorder__comments_do_not_crash(c *check.C) { func (s *Suite) Test_Package_CheckVarorder__comments_are_ignored(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Worder") - pkg := NewPackage(t.File("x11/9term")) pkg.CheckVarorder(t.NewMkLines("Makefile", @@ -150,8 +146,6 @@ func (s *Suite) Test_Package_CheckVarorder__comments_are_ignored(c *check.C) { func (s *Suite) Test_Package_CheckVarorder__skip_if_there_are_directives(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Worder") - pkg := NewPackage(t.File("category/package")) pkg.CheckVarorder(t.NewMkLines("Makefile", @@ -175,7 +169,6 @@ func (s *Suite) Test_Package_CheckVarorder__skip_if_there_are_directives(c *chec func (s *Suite) Test_Package_CheckVarorder__GITHUB_PROJECT_at_the_top(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Worder") pkg := NewPackage(t.File("x11/9term")) pkg.CheckVarorder(t.NewMkLines("Makefile", @@ -196,7 +189,6 @@ func (s *Suite) Test_Package_CheckVarorder__GITHUB_PROJECT_at_the_top(c *check.C func (s *Suite) Test_Package_CheckVarorder__GITHUB_PROJECT_at_the_bottom(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Worder") pkg := NewPackage(t.File("x11/9term")) pkg.CheckVarorder(t.NewMkLines("Makefile", @@ -217,8 +209,6 @@ func (s *Suite) Test_Package_CheckVarorder__GITHUB_PROJECT_at_the_bottom(c *chec func (s *Suite) Test_Package_CheckVarorder__license(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Worder") - t.CreateFileLines("mk/bsd.pkg.mk", "# dummy") t.CreateFileLines("x11/Makefile", MkRcsID) t.CreateFileLines("x11/9term/PLIST", PlistRcsID, "bin/9term") @@ -226,10 +216,10 @@ func (s *Suite) Test_Package_CheckVarorder__license(c *check.C) { t.CreateFileLines("x11/9term/Makefile", MkRcsID, "", - "DISTNAME=9term-1.0", - "CATEGORIES=x11", + "DISTNAME=\t9term-1.0", + "CATEGORIES=\tx11", "", - "COMMENT=Terminal", + "COMMENT=\tTerminal", "", ".include \"../../mk/bsd.pkg.mk\"") @@ -246,7 +236,6 @@ func (s *Suite) Test_Package_CheckVarorder__license(c *check.C) { func (s *Suite) Test_Package_CheckVarorder__MASTER_SITES(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Worder") pkg := NewPackage(t.File("category/package")) pkg.CheckVarorder(t.NewMkLines("Makefile", @@ -267,7 +256,6 @@ func (s *Suite) Test_Package_CheckVarorder__MASTER_SITES(c *check.C) { func (s *Suite) Test_Package_CheckVarorder__diagnostics(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Worder") t.SetUpVartypes() pkg := NewPackage(t.File("category/package")) @@ -354,7 +342,6 @@ func (s *Suite) Test_Package_determineEffectivePkgVars__precedence(c *check.C) { func (s *Suite) Test_Package_determineEffectivePkgVars__same(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Wall,no-order") pkg := t.SetUpPackage("category/package", "DISTNAME=\tdistname-1.0", "PKGNAME=\tdistname-1.0") @@ -369,7 +356,6 @@ func (s *Suite) Test_Package_determineEffectivePkgVars__same(c *check.C) { func (s *Suite) Test_Package_determineEffectivePkgVars__invalid_DISTNAME(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Wall,no-order") pkg := t.SetUpPackage("category/package", "DISTNAME=\tpkgname-version") @@ -541,10 +527,10 @@ func (s *Suite) Test_Package_loadPackageMakefile(c *check.C) { // A file including itself does not lead to an endless loop while parsing // but may still produce unexpected warnings, such as redundant definitions. t.CheckOutputLines( - "NOTE: ~/category/package/Makefile:3: Definition of PKGNAME is redundant "+ - "because of ../../category/package/Makefile:3.", - "NOTE: ~/category/package/Makefile:4: Definition of DISTNAME is redundant "+ - "because of ../../category/package/Makefile:4.") + "NOTE: ~/category/package/Makefile:3: "+ + "Definition of PKGNAME is redundant because of ../../category/package/Makefile:3.", + "NOTE: ~/category/package/Makefile:4: "+ + "Definition of DISTNAME is redundant because of ../../category/package/Makefile:4.") } func (s *Suite) Test_Package_loadPackageMakefile__PECL_VERSION(c *check.C) { @@ -632,9 +618,8 @@ func (s *Suite) Test_Package__include_after_exists(c *check.C) { G.checkdirPackage(t.File("category/package")) - // FIXME: This error message should not appear at all because of the .if exists before. - t.CheckOutputLines( - "ERROR: ~/category/package/Makefile:21: Relative path \"options.mk\" does not exist.") + // No error message at all because of the .if exists before. + t.CheckOutputEmpty() } // See https://github.com/rillig/pkglint/issues/1 @@ -657,11 +642,8 @@ func (s *Suite) Test_Package_readMakefile__include_other_after_exists(c *check.C func (s *Suite) Test_Package__redundant_master_sites(c *check.C) { t := s.Init(c) - t.SetUpVartypes() + t.SetUpPkgsrc() t.SetUpMasterSite("MASTER_SITE_R_CRAN", "http://cran.r-project.org/src/") - t.CreateFileLines("mk/bsd.pkg.mk") - t.CreateFileLines("licenses/gnu-gpl-v2", - "The licenses for most software are designed to take away ...") t.CreateFileLines("math/R/Makefile.extension", MkRcsID, "", @@ -680,13 +662,20 @@ func (s *Suite) Test_Package__redundant_master_sites(c *check.C) { "", ".include \"../../math/R/Makefile.extension\"", ".include \"../../mk/bsd.pkg.mk\"") + G.Pkgsrc.LoadInfrastructure() // See Package.checkfilePackageMakefile - // See Scope.uncond G.checkdirPackage(t.File("math/R-date")) + // The definition in Makefile:6 is redundant because the same definition + // occurs later in Makefile.extension:4. Usually the later definition gets + // the note. In this case though, it would be wrong to mark the + // definition in Makefile.extension as redundant because that definition is + // probably used by other packages as well. Therefore in this case the roles + // of the two lines are swapped; see RedundantScope.Handle, the ".includes" line. t.CheckOutputLines( - "NOTE: ~/math/R-date/Makefile:6: Definition of MASTER_SITES is redundant because of ../../math/R/Makefile.extension:4.") + "NOTE: ~/math/R-date/Makefile:6: " + + "Definition of MASTER_SITES is redundant because of ../../math/R/Makefile.extension:4.") } func (s *Suite) Test_Package_checkUpdate(c *check.C) { @@ -709,7 +698,7 @@ func (s *Suite) Test_Package_checkUpdate(c *check.C) { "\t"+"o package3-3.0 [security update]") t.Chdir(".") - G.Main("pkglint", "-Wall,no-space,no-order", "category/pkg1", "category/pkg2", "category/pkg3") + G.Main("pkglint", "-Wall,no-space", "category/pkg1", "category/pkg2", "category/pkg3") t.CheckOutputLines( "WARN: category/pkg1/../../doc/TODO:3: Invalid line format \"\".", @@ -928,8 +917,6 @@ func (s *Suite) Test_Package_readMakefile__builtin_mk(c *check.C) { func (s *Suite) Test_Package_checkLocallyModified(c *check.C) { t := s.Init(c) - // no-order since SetUpPackage doesn't place OWNER correctly. - t.SetUpCommandLine("-Wall,no-order") G.Username = "example-user" t.CreateFileLines("category/package/CVS/Entries", "/Makefile//modified//") @@ -988,3 +975,68 @@ func (s *Suite) Test_Package_checkLocallyModified(c *check.C) { t.CheckOutputEmpty() } + +// In practice the distinfo file can always be autofixed since it has +// just been read successfully and the corresponding patch file could +// also be autofixed right before. +func (s *Suite) Test_Package_AutofixDistinfo__missing_file(c *check.C) { + t := s.Init(c) + + t.SetUpPkgsrc() + G.Pkg = NewPackage(t.File("category/package")) + + G.Pkg.AutofixDistinfo("old", "new") + + t.CheckOutputLines( + "ERROR: ~/category/package/distinfo: Cannot be read.") +} + +func (s *Suite) Test_Package__using_common_Makefile_overriding_DISTINFO_FILE(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("security/pinentry") + t.CreateFileLines("security/pinentry/Makefile.common", + MkRcsID, + "DISTINFO_FILE=\t${.CURDIR}/../../security/pinentry/distinfo") + t.SetUpPackage("security/pinentry-fltk", + ".include \"../../security/pinentry/Makefile.common\"", + "DISTINFO_FILE=\t${.CURDIR}/distinfo") + t.CreateFileDummyPatch("security/pinentry-fltk/patches/patch-aa") + t.CreateFileLines("security/pinentry-fltk/distinfo", + RcsID, + "", + "SHA1 (patch-aa) = ebbf34b0641bcb508f17d5a27f2bf2a536d810ac") + G.Pkgsrc.LoadInfrastructure() + + G.Check(t.File("security/pinentry")) + + t.CheckOutputEmpty() + + G.Check(t.File("security/pinentry-fltk")) + + // The DISTINFO_FILE definition from pinentry-fltk overrides + // the one from pinentry since it appears later. + // Therefore the patch is searched for at the right location. + t.CheckOutputEmpty() +} + +func (s *Suite) Test_Package__redundant_variable_in_unrelated_files(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("databases/py-trytond-ldap-authentication", + ".include \"../../devel/py-trytond/Makefile.common\"", + ".include \"../../lang/python/egg.mk\"") + t.CreateFileLines("devel/py-trytond/Makefile.common", + MkRcsID, + "PY_PATCHPLIST=\tyes") + t.CreateFileLines("lang/python/egg.mk", + MkRcsID, + "PY_PATCHPLIST=\tyes") + G.Pkgsrc.LoadInfrastructure() + + G.Check(t.File("databases/py-trytond-ldap-authentication")) + + // Since egg.mk and Makefile.common are unrelated, the definition of + // PY_PATCHPLIST is not redundant in these files. + t.CheckOutputEmpty() +} diff --git a/pkgtools/pkglint/files/patches.go b/pkgtools/pkglint/files/patches.go index 963e3362364..b9a0a529c9c 100644 --- a/pkgtools/pkglint/files/patches.go +++ b/pkgtools/pkglint/files/patches.go @@ -98,7 +98,7 @@ func (ck *PatchChecker) Check() { if SaveAutofixChanges(ck.lines) && G.Pkg != nil && err == nil { sha1After, err := computePatchSha1Hex(ck.lines.FileName) if err == nil { - AutofixDistinfo(sha1Before, sha1After) + G.Pkg.AutofixDistinfo(sha1Before, sha1After) } } } diff --git a/pkgtools/pkglint/files/pkglint.0 b/pkgtools/pkglint/files/pkglint.0 index b13868c078c..a421b7c9ffa 100644 --- a/pkgtools/pkglint/files/pkglint.0 +++ b/pkgtools/pkglint/files/pkglint.0 @@ -1,7 +1,7 @@ -PKGLINT(1) NetBSD General Commands Manual PKGLINT(1) +PKGLINT(1) General Commands Manual PKGLINT(1) NNAAMMEE - ppkkgglliinntt -- static analyzer for pkgsrc packages + ppkkgglliinntt - static analyzer for pkgsrc packages SSYYNNOOPPSSIISS ppkkgglliinntt [--ooppttiioonnss] [_d_i_r _._._.] @@ -10,6 +10,7 @@ DDEESSCCRRIIPPTTIIOONN ppkkgglliinntt attempts to detect features of the named pkgsrc packages that are likely to be bugs, or that are simply deprecated. + OOppttiioonnss --CC{{[[nnoo--]]cchheecckk,,......}} Enable or disable specific checks. For a list of checks, see below. @@ -62,78 +63,39 @@ DDEESSCCRRIIPPTTIIOONN nnoonnee Disable all checks. - [[nnoo--]]AALLTTEERRNNAATTIIVVEESS Check the alternatives file. - - [[nnoo--]]DDEESSCCRR Check the DESCR file. - - [[nnoo--]]IINNSSTTAALLLL Check the INSTALL and DEINSTALL scripts. - - [[nnoo--]]MMaakkeeffiillee Check the package Makefile, including all included - files. - - [[nnoo--]]MMEESSSSAAGGEE Check MESSAGE files. - - [[nnoo--]]PPLLIISSTT Check PLIST files. - - [[nnoo--]]bbll33 Check buildlink3 Makefiles. - - [[nnoo--]]ddiissttiinnffoo Check the distinfo file. - [[nnoo--]]eexxttrraa Check remaining files in the package directory. - [[nnoo--]]mmkk Check Makefile fragments besides buildlink3. - - [[nnoo--]]ppaattcchheess Check the pkgsrc specific patch files. + [[nnoo--]]gglloobbaall Check inter-package consistency for distfile hashes + and used licenses. WWaarrnniinnggss aallll Enable all warnings. nnoonnee Disable all warnings. - [[nnoo--]]aabbssnnaammee Warn if a file contains an absolute pathname. - - [[nnoo--]]ddiirreeccttccmmdd Warn if a system command name is used instead of a - variable (e.g. sed instead of ${SED}). - [[nnoo--]]eexxttrraa Emit some additional warnings that are not enabled by default. - [[nnoo--]]oorrddeerr Warn if Makefile variables are not in the preferred - order. - [[nnoo--]]ppeerrmm Warn if a variable is used or modified outside its specified scope. - [[nnoo--]]pplliisstt--ddeepprr Warn if deprecated pathnames are used in _P_L_I_S_T files. - This warning is disabled by default. - - [[nnoo--]]pplliisstt--ssoorrtt Warn if items of a PLIST file are not sorted alpha- - betically. This warning is disabled by default. - [[nnoo--]]qquuoottiinngg Warn for possibly invalid quoting of make variables in shell programs and shell variables themselves. - [[nnoo--]]ssppaaccee Emit notes for inconsistent use of white-space. + [[nnoo--]]ssppaaccee Emit notes for inconsistent use of whitespace. [[nnoo--]]ssttyyllee Warn for stylistic issues that don't affect the build process. - [[nnoo--]]ttyyppeess Warn for some _M_a_k_e_f_i_l_e variables if their assigned - values do not match their type. - - [[nnoo--]]vvaarroorrddeerr Warn if the variables in a package _M_a_k_e_f_i_l_es are not - ordered in the way it is described the pkgsrc guide. - OOtthheerr aarrgguummeennttss - _d_i_r _._._. The pkgsrc directories to be checked. If omit- - ted, the current directory is checked. + _d_i_r _._._. The pkgsrc directories to be checked. If + omitted, the current directory is checked. FFIILLEESS - pkgsrc/mk/* Files from the pkgsrc infrastructure. + _p_k_g_s_r_c_/_m_k_/_* Files from the pkgsrc infrastructure. EEXXAAMMPPLLEESS - ppkkgglliinntt --CCnnoonnee,,ppaattcchheess .. - Checks the patches of the package in the current directory. + ppkkgglliinntt .. Checks the package in the current directory. ppkkgglliinntt --WWaallll //uussrr//ppkkggssrrcc//ddeevveell Checks the category Makefile and reports any warnings it can @@ -155,4 +117,4 @@ BBUUGGSS If you don't understand the messages, feel free to ask the author or on the <pkgsrc-users@pkgsrc.org> mailing list. -NetBSD 7.0.2 January 14, 2018 NetBSD 7.0.2 +NetBSD 8.0 January 14, 2018 NetBSD 8.0 diff --git a/pkgtools/pkglint/files/pkglint.1 b/pkgtools/pkglint/files/pkglint.1 index b3f2cc858a7..9e07708b7b8 100644 --- a/pkgtools/pkglint/files/pkglint.1 +++ b/pkgtools/pkglint/files/pkglint.1 @@ -1,4 +1,4 @@ -.\" $NetBSD: pkglint.1,v 1.52 2018/01/13 23:56:14 rillig Exp $ +.\" $NetBSD: pkglint.1,v 1.53 2019/02/21 22:49:03 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>. @@ -6,7 +6,7 @@ .\" .\" Roland Illig <roland.illig@gmx.de>, 2004, 2005. .\" Thomas Klausner <wiz@NetBSD.org>, 2012. -.\" Roland Illig <rillig@NetBSD.org>, 2015-2018. +.\" Roland Illig <rillig@NetBSD.org>, 2015-2019. .\" .Dd January 14, 2018 .Dt PKGLINT 1 @@ -88,28 +88,10 @@ For a list of warnings, see below. Enable all checks. .It Cm none Disable all checks. -.It Cm [no-]ALTERNATIVES -Check the alternatives file. -.It Cm [no-]DESCR -Check the DESCR file. -.It Cm [no-]INSTALL -Check the INSTALL and DEINSTALL scripts. -.It Cm [no-]Makefile -Check the package Makefile, including all included files. -.It Cm [no-]MESSAGE -Check MESSAGE files. -.It Cm [no-]PLIST -Check PLIST files. -.It Cm [no-]bl3 -Check buildlink3 Makefiles. -.It Cm [no-]distinfo -Check the distinfo file. .It Cm [no-]extra Check remaining files in the package directory. -.It Cm [no-]mk -Check Makefile fragments besides buildlink3. -.It Cm [no-]patches -Check the pkgsrc specific patch files. +.It Cm [no-]global +Check inter-package consistency for distfile hashes and used licenses. .El .\" ======================================================================= .Ss Warnings @@ -118,41 +100,17 @@ Check the pkgsrc specific patch files. Enable all warnings. .It Cm none Disable all warnings. -.It Cm [no-]absname -Warn if a file contains an absolute pathname. -.It Cm [no-]directcmd -Warn if a system command name is used instead of a variable (e.g. sed -instead of ${SED}). .It Cm [no-]extra Emit some additional warnings that are not enabled by default. -.It Cm [no-]order -Warn if Makefile variables are not in the preferred order. .It Cm [no-]perm Warn if a variable is used or modified outside its specified scope. -.It Cm [no-]plist-depr -Warn if deprecated pathnames are used in -.Pa PLIST -files. -This warning is disabled by default. -.It Cm [no-]plist-sort -Warn if items of a PLIST file are not sorted alphabetically. -This warning is disabled by default. .It Cm [no-]quoting Warn for possibly invalid quoting of make variables in shell programs and shell variables themselves. .It Cm [no-]space -Emit notes for inconsistent use of white-space. +Emit notes for inconsistent use of whitespace. .It Cm [no-]style Warn for stylistic issues that don't affect the build process. -.It Cm [no-]types -Warn for some -.Pa Makefile -variables if their assigned values do not match -their type. -.It Cm [no-]varorder -Warn if the variables in a package -.Pa Makefile Ns -s are not ordered in the way it is described the pkgsrc guide. .El .\" ======================================================================= .Ss Other arguments @@ -168,8 +126,8 @@ Files from the pkgsrc infrastructure. .El .Sh EXAMPLES .Bl -tag -width Fl -.It Ic pkglint \-Cnone,patches \&. -Checks the patches of the package in the current directory. +.It Ic pkglint \&. +Checks the package in the current directory. .It Ic pkglint \-Wall /usr/pkgsrc/devel Checks the category Makefile and reports any warnings it can find. .El diff --git a/pkgtools/pkglint/files/pkglint.go b/pkgtools/pkglint/files/pkglint.go index f15af01f679..e5096bc488c 100644 --- a/pkgtools/pkglint/files/pkglint.go +++ b/pkgtools/pkglint/files/pkglint.go @@ -40,6 +40,9 @@ type Pkglint struct { res regex.Registry fileCache *FileCache interner StringInterner + + Hashes map[string]*Hash // Maps "alg:filename" => hash (inter-package check). + UsedLicenses map[string]bool // Maps "license name" => true (inter-package check). } func NewPkglint() Pkglint { @@ -50,24 +53,8 @@ func NewPkglint() Pkglint { } type CmdOpts struct { - // TODO: Are these Check* options really necessary? - // - // They had been introduced in order to make pkglint more flexible, - // but without any actual need. - - CheckAlternatives, - CheckBuildlink3, - CheckDescr, - CheckDistinfo, CheckExtra, - CheckGlobal, - CheckInstall, - CheckMakefile, - CheckMessage, - CheckMk, - CheckOptions, - CheckPatches, - CheckPlist bool + CheckGlobal bool // TODO: Are these Warn* options really all necessary? // @@ -76,16 +63,11 @@ type CmdOpts struct { // could be contrasted by a future --ignore option, in order to suppress // individual checks. - WarnDirectcmd, WarnExtra, - WarnOrder, WarnPerm, - WarnPlistDepr, - WarnPlistSort, WarnQuoting, WarnSpace, - WarnStyle, - WarnTypes bool + WarnStyle bool Profiling, ShowHelp, @@ -255,30 +237,14 @@ func (pkglint *Pkglint) ParseCommandLine(args []string) int { opts.AddFlagVar('V', "version", &gopts.ShowVersion, false, "show the version number of pkglint") warn := opts.AddFlagGroup('W', "warning", "warning,...", "enable or disable groups of warnings") - check.AddFlagVar("ALTERNATIVES", &gopts.CheckAlternatives, true, "check ALTERNATIVES files") - check.AddFlagVar("bl3", &gopts.CheckBuildlink3, true, "check buildlink3.mk files") - check.AddFlagVar("DESCR", &gopts.CheckDescr, true, "check DESCR file") - check.AddFlagVar("distinfo", &gopts.CheckDistinfo, true, "check distinfo file") check.AddFlagVar("extra", &gopts.CheckExtra, false, "check various additional files") check.AddFlagVar("global", &gopts.CheckGlobal, false, "inter-package checks") - check.AddFlagVar("INSTALL", &gopts.CheckInstall, true, "check INSTALL and DEINSTALL scripts") - check.AddFlagVar("Makefile", &gopts.CheckMakefile, true, "check Makefiles") - check.AddFlagVar("MESSAGE", &gopts.CheckMessage, true, "check MESSAGE file") - check.AddFlagVar("mk", &gopts.CheckMk, true, "check other .mk files") - check.AddFlagVar("options", &gopts.CheckOptions, true, "check options.mk files") - check.AddFlagVar("patches", &gopts.CheckPatches, true, "check patches") - check.AddFlagVar("PLIST", &gopts.CheckPlist, true, "check PLIST files") - - warn.AddFlagVar("directcmd", &gopts.WarnDirectcmd, true, "warn about use of direct command names instead of Make variables") + warn.AddFlagVar("extra", &gopts.WarnExtra, false, "enable some extra warnings") - warn.AddFlagVar("order", &gopts.WarnOrder, true, "warn if Makefile entries are unordered") warn.AddFlagVar("perm", &gopts.WarnPerm, false, "warn about unforeseen variable definition and use") - warn.AddFlagVar("plist-depr", &gopts.WarnPlistDepr, false, "warn about deprecated paths in PLISTs") - warn.AddFlagVar("plist-sort", &gopts.WarnPlistSort, false, "warn about unsorted entries in PLISTs") warn.AddFlagVar("quoting", &gopts.WarnQuoting, false, "warn about quoting issues") warn.AddFlagVar("space", &gopts.WarnSpace, false, "warn about inconsistent use of whitespace") warn.AddFlagVar("style", &gopts.WarnStyle, false, "warn about stylistic issues") - warn.AddFlagVar("types", &gopts.WarnTypes, true, "do some simple type checking in Makefiles") remainingArgs, err := opts.Parse(args) if err != nil { @@ -471,9 +437,7 @@ func (pkglint *Pkglint) checkdirPackage(dir string) { case path.Base(filename) == "Makefile": pkglint.checkExecutable(filename, st.Mode()) - if pkglint.Opts.CheckMakefile { - pkg.checkfilePackageMakefile(filename, mklines) - } + pkg.checkfilePackageMakefile(filename, mklines) default: pkglint.checkDirent(filename, st.Mode()) @@ -487,7 +451,7 @@ func (pkglint *Pkglint) checkdirPackage(dir string) { pkg.checkLocallyModified(filename) } - if pkg.Pkgdir == "." && pkglint.Opts.CheckDistinfo && pkglint.Opts.CheckPatches { + if pkg.Pkgdir == "." { if havePatches && !haveDistinfo { // TODO: Add Line.RefTo to make the context clear. NewLineWhole(pkg.File(pkg.DistinfoFile)).Warnf("File not found. Please run %q.", bmake("makepatchsum")) @@ -543,12 +507,12 @@ func resolveVariableRefs(text string) (resolved string) { if !visited[varname] { visited[varname] = true if G.Pkg != nil { - if value, ok := G.Pkg.vars.Value(varname); ok { + if value, ok := G.Pkg.vars.LastValueFound(varname); ok { return value } } if G.Mk != nil { - if value, ok := G.Mk.vars.Value(varname); ok { + if value, ok := G.Mk.vars.LastValueFound(varname); ok { return value } } @@ -700,55 +664,39 @@ func (pkglint *Pkglint) checkReg(filename, basename string, depth int) { switch { case basename == "ALTERNATIVES": - if pkglint.Opts.CheckAlternatives { - CheckFileAlternatives(filename) - } + CheckFileAlternatives(filename) case basename == "buildlink3.mk": - if pkglint.Opts.CheckBuildlink3 { - if mklines := LoadMk(filename, NotEmpty|LogErrors); mklines != nil { - CheckLinesBuildlink3Mk(mklines) - } + if mklines := LoadMk(filename, NotEmpty|LogErrors); mklines != nil { + CheckLinesBuildlink3Mk(mklines) } case hasPrefix(basename, "DESCR"): - if pkglint.Opts.CheckDescr { - if lines := Load(filename, NotEmpty|LogErrors); lines != nil { - CheckLinesDescr(lines) - } + if lines := Load(filename, NotEmpty|LogErrors); lines != nil { + CheckLinesDescr(lines) } case basename == "distinfo": - if pkglint.Opts.CheckDistinfo { - if lines := Load(filename, NotEmpty|LogErrors); lines != nil { - CheckLinesDistinfo(lines) - } + if lines := Load(filename, NotEmpty|LogErrors); lines != nil { + CheckLinesDistinfo(lines) } case basename == "DEINSTALL" || basename == "INSTALL": - if pkglint.Opts.CheckInstall { - CheckFileOther(filename) - } + CheckFileOther(filename) case hasPrefix(basename, "MESSAGE"): - if pkglint.Opts.CheckMessage { - if lines := Load(filename, NotEmpty|LogErrors); lines != nil { - CheckLinesMessage(lines) - } + if lines := Load(filename, NotEmpty|LogErrors); lines != nil { + CheckLinesMessage(lines) } case basename == "options.mk": - if pkglint.Opts.CheckOptions { - if mklines := LoadMk(filename, NotEmpty|LogErrors); mklines != nil { - CheckLinesOptionsMk(mklines) - } + if mklines := LoadMk(filename, NotEmpty|LogErrors); mklines != nil { + CheckLinesOptionsMk(mklines) } case matches(basename, `^patch-[-\w.~+]*\w$`): - if pkglint.Opts.CheckPatches { - if lines := Load(filename, NotEmpty|LogErrors); lines != nil { - CheckLinesPatch(lines) - } + if lines := Load(filename, NotEmpty|LogErrors); lines != nil { + CheckLinesPatch(lines) } case matches(filename, `(?:^|/)patches/manual[^/]*$`): @@ -760,17 +708,13 @@ func (pkglint *Pkglint) checkReg(filename, basename string, depth int) { NewLineWhole(filename).Warnf("Patch files should be named \"patch-\", followed by letters, '-', '_', '.', and digits only.") case (hasPrefix(basename, "Makefile") || hasSuffix(basename, ".mk")) && - !contains(filename, "files/") && - !contains(filename, "patches/"): - if pkglint.Opts.CheckMk { - CheckFileMk(filename) - } + !(hasPrefix(filename, "files/") || contains(filename, "/files/")) && + !(hasPrefix(filename, "patches/") || contains(filename, "/patches/")): + CheckFileMk(filename) case hasPrefix(basename, "PLIST"): - if pkglint.Opts.CheckPlist { - if lines := Load(filename, NotEmpty|LogErrors); lines != nil { - CheckLinesPlist(lines) - } + if lines := Load(filename, NotEmpty|LogErrors); lines != nil { + CheckLinesPlist(lines) } case hasPrefix(basename, "CHANGES-"): @@ -801,7 +745,7 @@ func (pkglint *Pkglint) matchesLicenseFile(basename string) bool { return false } - licenseFile, _ := pkglint.Pkg.vars.Value("LICENSE_FILE") + licenseFile := pkglint.Pkg.vars.LastValue("LICENSE_FILE") return basename == path.Base(licenseFile) } @@ -857,9 +801,9 @@ func CheckLinesTrailingEmptyLines(lines Lines) { // to USE_TOOLS in the current scope (file or package). func (pkglint *Pkglint) Tool(command string, time ToolTime) (tool *Tool, usable bool) { varname := "" - // TODO: Replace regex with proper VarUse. - if m, toolVarname := match1(command, `^\$\{(\w+)\}$`); m { - varname = toolVarname + p := NewMkParser(nil, command, false) + if varUse := p.VarUse(); varUse != nil && p.EOF() { + varname = varUse.varname } tools := pkglint.tools() diff --git a/pkgtools/pkglint/files/pkglint_test.go b/pkgtools/pkglint/files/pkglint_test.go index ca1a95de72a..ea11ec91aea 100644 --- a/pkgtools/pkglint/files/pkglint_test.go +++ b/pkgtools/pkglint/files/pkglint_test.go @@ -38,35 +38,19 @@ func (s *Suite) Test_Pkglint_Main__help(c *check.C) { " -W, --warning=warning,... enable or disable groups of warnings", "", " Flags for -C, --check:", - " all all of the following", - " none none of the following", - " ALTERNATIVES check ALTERNATIVES files (enabled)", - " bl3 check buildlink3.mk files (enabled)", - " DESCR check DESCR file (enabled)", - " distinfo check distinfo file (enabled)", - " extra check various additional files (disabled)", - " global inter-package checks (disabled)", - " INSTALL check INSTALL and DEINSTALL scripts (enabled)", - " Makefile check Makefiles (enabled)", - " MESSAGE check MESSAGE file (enabled)", - " mk check other .mk files (enabled)", - " options check options.mk files (enabled)", - " patches check patches (enabled)", - " PLIST check PLIST files (enabled)", + " all all of the following", + " none none of the following", + " extra check various additional files (disabled)", + " global inter-package checks (disabled)", "", " Flags for -W, --warning:", - " all all of the following", - " none none of the following", - " directcmd warn about use of direct command names instead of Make variables (enabled)", - " extra enable some extra warnings (disabled)", - " order warn if Makefile entries are unordered (enabled)", - " perm warn about unforeseen variable definition and use (disabled)", - " plist-depr warn about deprecated paths in PLISTs (disabled)", - " plist-sort warn about unsorted entries in PLISTs (disabled)", - " quoting warn about quoting issues (disabled)", - " space warn about inconsistent use of whitespace (disabled)", - " style warn about stylistic issues (disabled)", - " types do some simple type checking in Makefiles (enabled)", + " all all of the following", + " none none of the following", + " extra enable some extra warnings (disabled)", + " perm warn about unforeseen variable definition and use (disabled)", + " quoting warn about quoting issues (disabled)", + " space warn about inconsistent use of whitespace (disabled)", + " style warn about stylistic issues (disabled)", "", " (Prefix a flag with \"no-\" to disable it.)") } diff --git a/pkgtools/pkglint/files/pkgsrc.go b/pkgtools/pkglint/files/pkgsrc.go index 78f7001b021..2af34590b56 100644 --- a/pkgtools/pkglint/files/pkgsrc.go +++ b/pkgtools/pkglint/files/pkgsrc.go @@ -34,18 +34,15 @@ type Pkgsrc struct { LastChange map[string]*Change // listVersions map[string][]string // See ListVersions - // Variables that may be overridden by the pkgsrc user. Used for checking BUILD_DEFS. + // Variables that may be overridden by the pkgsrc user. + // They are typically defined in mk/defaults/mk.conf. + // + // Whenever a package uses such a variable, it must add the variable name + // to BUILD_DEFS. UserDefinedVars Scope Deprecated map[string]string // vartypes map[string]*Vartype // varcanon => type - - // TODO: The Hashes and UsedLicenses are modified after the initialization. - // This contradicts the expectation that all pkgsrc data is constant. - // These two fields should probably be moved to the Pkglint type. - - Hashes map[string]*Hash // Maps "alg:filename" => hash (inter-package check). - UsedLicenses map[string]bool // Maps "license name" => true (inter-package check). } func NewPkgsrc(dir string) *Pkgsrc { @@ -62,9 +59,7 @@ func NewPkgsrc(dir string) *Pkgsrc { make(map[string][]string), NewScope(), make(map[string]string), - make(map[string]*Vartype), - nil, // Only initialized when pkglint is run for a whole pkgsrc installation - nil} + make(map[string]*Vartype)} return &src } @@ -222,7 +217,7 @@ func (src *Pkgsrc) ListVersions(category string, re regex.Pattern, repl string, // written without dots, which leads to ambiguities: // // databases/postgresql: 94 < 95 < 96 < 10 < 11 - // lang/go: go19 < go110 < go111 < go2 + // lang/go: 19 < 110 < 111 < 2 keys := make(map[string]int) for _, name := range names { if m, pkgbase, versionStr := match2(name, `^(\D+)(\d+)$`); m { @@ -240,9 +235,7 @@ func (src *Pkgsrc) ListVersions(category string, re regex.Pattern, repl string, } sort.SliceStable(names, func(i, j int) bool { - // TODO: Check if this Less implementation is really transitive. - // examples: ps ps5 ps10 ps96 pq px - if keyI, keyJ := keys[names[i]], keys[names[j]]; keyI != 0 && keyJ != 0 { + if keyI, keyJ := keys[names[i]], keys[names[j]]; keyI != keyJ { return keyI < keyJ } return naturalLess(names[i], names[j]) @@ -258,7 +251,7 @@ func (src *Pkgsrc) ListVersions(category string, re regex.Pattern, repl string, } func (src *Pkgsrc) checkToplevelUnusedLicenses() { - usedLicenses := src.UsedLicenses + usedLicenses := G.UsedLicenses if usedLicenses == nil { return } @@ -827,6 +820,9 @@ func (src *Pkgsrc) addBuildDefs(varnames ...string) { } } +// IsBuildDef returns whether the given variable is automatically added +// to BUILD_DEFS by the pkgsrc infrastructure. In such a case, the +// package doesn't need to add the variable to BUILD_DEFS itself. func (src *Pkgsrc) IsBuildDef(varname string) bool { return src.buildDefs[varname] } diff --git a/pkgtools/pkglint/files/pkgsrc_test.go b/pkgtools/pkglint/files/pkgsrc_test.go index f1dac1db3b1..2a6d951989b 100644 --- a/pkgtools/pkglint/files/pkgsrc_test.go +++ b/pkgtools/pkglint/files/pkgsrc_test.go @@ -435,6 +435,50 @@ func (s *Suite) Test_Pkgsrc_ListVersions__postgresql(c *check.C) { "postgresql11"}) } +func (s *Suite) Test_Pkgsrc_ListVersions__ensure_transitive(c *check.C) { + names := []string{ + "base", + "base0", + "base000", + "base-client", + "base1", + "base01", + "base-client1", + "base5", + "base10"} + + keys := make(map[string]int) + for _, name := range names { + if m, _, versionStr := match2(name, `^(\D+)(\d+)$`); m { + keys[name] = toInt(versionStr, 0) + } + } + + less := func(a, b string) bool { + if keyI, keyJ := keys[a], keys[b]; keyI != keyJ { + return keyI < keyJ + } + return naturalLess(a, b) + } + + test := func(i int, j int) { + actual := less(names[i], names[j]) + expected := i < j + if actual != expected { + c.Check( + []interface{}{names[i], ifelseStr(actual, "<", "!<"), names[j]}, + check.DeepEquals, + []interface{}{names[i], ifelseStr(expected, "<", "!<"), names[j]}) + } + } + + for i := range names { + for j := range names { + test(i, j) + } + } +} + func (s *Suite) Test_Pkgsrc_ListVersions__numeric_multiple_numbers(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/plist.go b/pkgtools/pkglint/files/plist.go index 002670a75bc..0f5f602dd3c 100644 --- a/pkgtools/pkglint/files/plist.go +++ b/pkgtools/pkglint/files/plist.go @@ -65,13 +65,9 @@ func (ck *PlistChecker) Check(plainLines Lines) { } CheckLinesTrailingEmptyLines(plainLines) - if G.Opts.WarnPlistSort { - sorter := NewPlistLineSorter(plines) - sorter.Sort() - if !sorter.autofixed { - SaveAutofixChanges(plainLines) - } - } else { + sorter := NewPlistLineSorter(plines) + sorter.Sort() + if !sorter.autofixed { SaveAutofixChanges(plainLines) } } @@ -112,8 +108,6 @@ func (ck *PlistChecker) collectFilesAndDirs(plines []*PlistLine) { ck.allDirs[dir] = pline } case first == '@': - // TODO: Check if this directive is still used, - // or if it has been removed during a pkg_install re-implementation. if m, dirname := match1(text, `^@exec \$\{MKDIR\} %D/(.*)$`); m { for dir := dirname; dir != "."; dir = path.Dir(dir) { ck.allDirs[dir] = pline @@ -199,7 +193,7 @@ func (ck *PlistChecker) checkPath(pline *PlistLine) { pline.Warnf(".orig files should not be in the PLIST.") } if hasSuffix(text, "/perllocal.pod") { - pline.Warnf("perllocal.pod files should not be in the PLIST.") + pline.Warnf("The perllocal.pod file should not be in the PLIST.") G.Explain( "This file is handled automatically by the INSTALL/DEINSTALL scripts", "since its contents depends on more than one package.") @@ -210,7 +204,7 @@ func (ck *PlistChecker) checkPath(pline *PlistLine) { } func (ck *PlistChecker) checkSorted(pline *PlistLine) { - if text := pline.text; G.Opts.WarnPlistSort && hasAlnumPrefix(text) && !containsVarRef(text) { + if text := pline.text; hasAlnumPrefix(text) && !containsVarRef(text) { if ck.lastFname != "" { if ck.lastFname > text && !G.Logger.Opts.Autofix { pline.Warnf("%q should be sorted before %q.", text, ck.lastFname) @@ -292,7 +286,7 @@ func (ck *PlistChecker) checkPathLib(pline *PlistLine, dirname, basename string) switch ext := path.Ext(basename); ext { case ".la": - if G.Pkg != nil && !G.Pkg.vars.Defined("USE_LIBTOOL") { + if G.Pkg != nil && !G.Pkg.vars.Defined("USE_LIBTOOL") && ck.once.FirstTime("USE_LIBTOOL") { pline.Warnf("Packages that install libtool libraries should define USE_LIBTOOL.") } } @@ -378,9 +372,7 @@ func (ck *PlistChecker) checkPathShare(pline *PlistLine) { } case hasPrefix(text, "share/doc/html/"): - if G.Opts.WarnPlistDepr { - pline.Warnf("Use of \"share/doc/html\" is deprecated. Use \"share/doc/${PKGBASE}\" instead.") - } + pline.Warnf("Use of \"share/doc/html\" is deprecated. Use \"share/doc/${PKGBASE}\" instead.") case G.Pkg != nil && G.Pkg.EffectivePkgbase != "" && (hasPrefix(text, "share/doc/"+G.Pkg.EffectivePkgbase+"/") || hasPrefix(text, "share/examples/"+G.Pkg.EffectivePkgbase+"/")): @@ -398,7 +390,7 @@ func (ck *PlistChecker) checkPathShare(pline *PlistLine) { func (pline *PlistLine) CheckTrailingWhitespace() { if hasSuffix(pline.text, " ") || hasSuffix(pline.text, "\t") { - pline.Errorf("pkgsrc does not support filenames ending in whitespace.") + pline.Errorf("Pkgsrc does not support filenames ending in whitespace.") G.Explain( "Each character in the PLIST is relevant, even trailing whitespace.") } @@ -420,7 +412,7 @@ func (pline *PlistLine) CheckDirective(cmd, arg string) { case "exec", "unexec": switch { case contains(arg, "ldconfig") && !contains(arg, "/usr/bin/true"): - pline.Errorf("ldconfig must be used with \"||/usr/bin/true\".") + pline.Errorf("The ldconfig command must be used with \"||/usr/bin/true\".") } case "comment": diff --git a/pkgtools/pkglint/files/plist_test.go b/pkgtools/pkglint/files/plist_test.go index 7304fa7348d..469b1aef6c6 100644 --- a/pkgtools/pkglint/files/plist_test.go +++ b/pkgtools/pkglint/files/plist_test.go @@ -48,6 +48,23 @@ func (s *Suite) Test_CheckLinesPlist(c *check.C) { "ERROR: PLIST:18: Duplicate filename \"share/tzinfo\", already appeared in line 17.") } +// When a PLIST contains multiple libtool libraries, USE_LIBTOOL needs only +// be defined once in the package Makefile. Therefore, a single warning is enough. +func (s *Suite) Test_CheckLinesPlist__multiple_libtool_libraries(c *check.C) { + t := s.Init(c) + + G.Pkg = NewPackage(t.File("category/pkgbase")) + lines := t.NewLines("PLIST", + PlistRcsID, + "lib/libc.la", + "lib/libm.la") + + CheckLinesPlist(lines) + + t.CheckOutputLines( + "WARN: PLIST:2: Packages that install libtool libraries should define USE_LIBTOOL.") +} + func (s *Suite) Test_CheckLinesPlist__empty(c *check.C) { t := s.Init(c) @@ -92,7 +109,6 @@ func (s *Suite) Test_CheckLinesPlist__condition(c *check.C) { func (s *Suite) Test_CheckLinesPlist__sorting(c *check.C) { t := s.Init(c) - t.SetUpCommandLine("-Wplist-sort") lines := t.NewLines("PLIST", PlistRcsID, "@comment Do not remove", @@ -471,7 +487,7 @@ func (s *Suite) Test_PlistChecker_checkPath__unwanted_entries(c *check.C) { CheckLinesPlist(lines) t.CheckOutputLines( - "WARN: ~/PLIST:2: perllocal.pod files should not be in the PLIST.", + "WARN: ~/PLIST:2: The perllocal.pod file should not be in the PLIST.", "WARN: ~/PLIST:3: CVS files should not be in the PLIST.", "WARN: ~/PLIST:4: .orig files should not be in the PLIST.") } @@ -560,7 +576,7 @@ func (s *Suite) Test_PlistLine_CheckTrailingWhitespace(c *check.C) { CheckLinesPlist(lines) t.CheckOutputLines( - "ERROR: ~/PLIST:2: pkgsrc does not support filenames ending in whitespace.") + "ERROR: ~/PLIST:2: Pkgsrc does not support filenames ending in whitespace.") } func (s *Suite) Test_PlistLine_CheckDirective(c *check.C) { @@ -580,7 +596,7 @@ func (s *Suite) Test_PlistLine_CheckDirective(c *check.C) { t.CheckOutputLines( "WARN: ~/PLIST:2: Please remove this line. It is no longer necessary.", - "ERROR: ~/PLIST:3: ldconfig must be used with \"||/usr/bin/true\".", + "ERROR: ~/PLIST:3: The ldconfig command must be used with \"||/usr/bin/true\".", "WARN: ~/PLIST:5: @dirrm is obsolete. Please remove this line.", "WARN: ~/PLIST:6: Invalid number of arguments for imake-man, should be 3.", "WARN: ~/PLIST:7: IMAKE_MANNEWSUFFIX is not meant to appear in PLISTs.", diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go index ebd26e2e352..2188e20c919 100644 --- a/pkgtools/pkglint/files/shell_test.go +++ b/pkgtools/pkglint/files/shell_test.go @@ -1148,7 +1148,7 @@ func (s *Suite) Test_SimpleCommandChecker_handleCommandVariable(c *check.C) { // FIXME: In PERL5:Q and PERL6:Q, the :Q is wrong. t.CheckOutputLines( "WARN: Makefile:3: PERL5_VARS_CMD is defined but not used.", - "WARN: Makefile:4: The \"perl6\" tool is used but not added to USE_TOOLS.") + "WARN: Makefile:4: The \"${PERL6:Q}\" tool is used but not added to USE_TOOLS.") } // The package Makefile and other .mk files in a package directory @@ -1189,6 +1189,26 @@ func (s *Suite) Test_SimpleCommandChecker_handleComment(c *check.C) { "WARN: file.mk:3: A shell comment should not contain semicolons.") } +// This test ensures that the command line options to INSTALL_*_DIR are properly +// parsed and do not lead to "can only handle one directory at a time" warnings. +func (s *Suite) Test_SimpleCommandChecker_checkInstallMulti(c *check.C) { + t := s.Init(c) + + t.SetUpVartypes() + mklines := t.NewMkLines("install.mk", + MkRcsID, + "", + "do-install:", + "\t${INSTALL_PROGRAM_DIR} -m 0555 -g ${APACHE_GROUP} -o ${APACHE_USER} \\", + "\t\t${DESTDIR}${PREFIX}/lib/apache-modules") + + mklines.Check() + + t.CheckOutputLines( + "NOTE: install.mk:4--5: You can use \"INSTALLATION_DIRS+= lib/apache-modules\" " + + "instead of \"${INSTALL_PROGRAM_DIR}\".") +} + func (s *Suite) Test_SimpleCommandChecker_checkPaxPe(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/substcontext_test.go b/pkgtools/pkglint/files/substcontext_test.go index 1c603ddc964..f0eca45ee8c 100644 --- a/pkgtools/pkglint/files/substcontext_test.go +++ b/pkgtools/pkglint/files/substcontext_test.go @@ -370,18 +370,19 @@ func (s *Suite) Test_SubstContext_suggestSubstVars(c *check.C) { "SUBST_CLASSES+=\t\ttest", "SUBST_STAGE.test=\tpre-configure", "SUBST_FILES.test=\tfilename", - "SUBST_SED.test+=\t-e s,@SH@,${SH},g", // Can be replaced. - "SUBST_SED.test+=\t-e s,@SH@,${SH:Q},g", // Can be replaced, with or without the :Q modifier. - "SUBST_SED.test+=\t-e s,@SH@,${SH:T},g", // Cannot be replaced because of the :T modifier. - "SUBST_SED.test+=\t-e s,@SH@,${SH},", // Can be replaced, even without the g option. - "SUBST_SED.test+=\t-e 's,@SH@,${SH},'", // Can be replaced, whether in single quotes or not. - "SUBST_SED.test+=\t-e \"s,@SH@,${SH},\"", // Can be replaced, whether in double quotes or not. - "SUBST_SED.test+=\t-e s,'@SH@','${SH}',", // Can be replaced, even when the quoting changes midways. - "SUBST_SED.test+=\ts,'@SH@','${SH}',", // Can be replaced manually, even when the -e is missing. - "SUBST_SED.test+=\t-e s,@SH@,${PKGNAME},", // Cannot be replaced since the variable name differs. - "SUBST_SED.test+=\t-e s,@SH@,'\"'${SH:Q}'\"',g", // Cannot be replaced since the double quotes are added. - "SUBST_SED.test+=\t-e s", // Just to get 100% code coverage. - "SUBST_SED.test+=\t-e s,@SH@,${SH:Q}", // Just to get 100% code coverage. + "SUBST_SED.test+=\t-e s,@SH@,${SH},g", // Can be replaced. + "SUBST_SED.test+=\t-e s,@SH@,${SH:Q},g", // Can be replaced, with or without the :Q modifier. + "SUBST_SED.test+=\t-e s,@SH@,${SH:T},g", // Cannot be replaced because of the :T modifier. + "SUBST_SED.test+=\t-e s,@SH@,${SH},", // Can be replaced, even without the g option. + "SUBST_SED.test+=\t-e 's,@SH@,${SH},'", // Can be replaced, whether in single quotes or not. + "SUBST_SED.test+=\t-e \"s,@SH@,${SH},\"", // Can be replaced, whether in double quotes or not. + "SUBST_SED.test+=\t-e s,'@SH@','${SH}',", // Can be replaced, even when the quoting changes midways. + "SUBST_SED.test+=\ts,'@SH@','${SH}',", // Can be replaced manually, even when the -e is missing. + "SUBST_SED.test+=\t-e s,@SH@,${PKGNAME},", // Cannot be replaced since the variable name differs. + "SUBST_SED.test+=\t-e s,@SH@,'\"'${SH:Q}'\"',g", // Cannot be replaced since the double quotes are added. + "SUBST_SED.test+=\t-e s", // Just to get 100% code coverage. + "SUBST_SED.test+=\t-e s,@SH@,${SH:Q}", // Just to get 100% code coverage. + "SUBST_SED.test+=\t-e s,@SH@,${SH:Q}, # comment", // This is not fixed automatically. "# end") mklines.Check() @@ -405,6 +406,8 @@ func (s *Suite) Test_SubstContext_suggestSubstVars(c *check.C) { "NOTE: subst.mk:13: Please always use \"-e\" in sed commands, "+ "even if there is only one substitution.", "NOTE: subst.mk:13: The substitution command \"s,'@SH@','${SH}',\" "+ + "can be replaced with \"SUBST_VARS.test+= SH\".", + "NOTE: subst.mk:18: The substitution command \"s,@SH@,${SH:Q},\" "+ "can be replaced with \"SUBST_VARS.test+= SH\".") t.SetUpCommandLine("--show-autofix") @@ -435,9 +438,7 @@ func (s *Suite) Test_SubstContext_suggestSubstVars(c *check.C) { "NOTE: subst.mk:12: The substitution command \"s,'@SH@','${SH}',\" "+ "can be replaced with \"SUBST_VARS.test+= SH\".", "AUTOFIX: subst.mk:12: Replacing \"SUBST_SED.test+=\\t-e s,'@SH@','${SH}',\" "+ - "with \"SUBST_VARS.test+=\\tSH\".", - "NOTE: subst.mk:13: The substitution command \"s,'@SH@','${SH}',\" "+ - "can be replaced with \"SUBST_VARS.test+= SH\".") + "with \"SUBST_VARS.test+=\\tSH\".") } // simulateSubstLines only tests some of the inner workings of SubstContext. diff --git a/pkgtools/pkglint/files/textproc/lexer.go b/pkgtools/pkglint/files/textproc/lexer.go index 3881b5d5c05..3d22338be95 100644 --- a/pkgtools/pkglint/files/textproc/lexer.go +++ b/pkgtools/pkglint/files/textproc/lexer.go @@ -264,6 +264,7 @@ func (bs *ByteSet) Contains(b byte) bool { return bs.bits[b] } var ( Alnum = NewByteSet("A-Za-z0-9") // Alphanumerical, without underscore AlnumU = NewByteSet("A-Za-z0-9_") // Alphanumerical, including underscore + Alpha = NewByteSet("A-Za-z") // Alphabetical, without underscore Digit = NewByteSet("0-9") // The digits zero to nine Upper = NewByteSet("A-Z") // The uppercase letters from A to Z Lower = NewByteSet("a-z") // The lowercase letters from a to z diff --git a/pkgtools/pkglint/files/textproc/lexer_test.go b/pkgtools/pkglint/files/textproc/lexer_test.go index 61ddbfa3fca..fd21316f11d 100644 --- a/pkgtools/pkglint/files/textproc/lexer_test.go +++ b/pkgtools/pkglint/files/textproc/lexer_test.go @@ -391,6 +391,18 @@ func (s *Suite) Test__XPrint(c *check.C) { c.Check(set.Contains(0xA0), equals, false) } +func (s *Suite) Test__Alpha(c *check.C) { + set := Alpha + + c.Check(set.Contains(0x00), equals, false) + c.Check(set.Contains('@'), equals, false) + c.Check(set.Contains('A'), equals, true) + c.Check(set.Contains('Z'), equals, true) + c.Check(set.Contains('`'), equals, false) + c.Check(set.Contains('a'), equals, true) + c.Check(set.Contains('z'), equals, true) +} + func (s *Suite) Test__test_names(c *check.C) { ck := intqa.NewTestNameChecker(c) ck.AllowCamelCaseDescriptions( diff --git a/pkgtools/pkglint/files/toplevel.go b/pkgtools/pkglint/files/toplevel.go index 5feaeb87580..0e0c2c47844 100644 --- a/pkgtools/pkglint/files/toplevel.go +++ b/pkgtools/pkglint/files/toplevel.go @@ -29,8 +29,8 @@ func CheckdirToplevel(dir string) { if G.Opts.Recursive { if G.Opts.CheckGlobal { - G.Pkgsrc.UsedLicenses = make(map[string]bool) - G.Pkgsrc.Hashes = make(map[string]*Hash) + G.UsedLicenses = make(map[string]bool) + G.Hashes = make(map[string]*Hash) } G.Todo = append(append([]string(nil), ctx.subdirs...), G.Todo...) } diff --git a/pkgtools/pkglint/files/trace/tracing.go b/pkgtools/pkglint/files/trace/tracing.go index 3674399c29a..0f0556041d0 100644 --- a/pkgtools/pkglint/files/trace/tracing.go +++ b/pkgtools/pkglint/files/trace/tracing.go @@ -37,14 +37,32 @@ func (t *Tracer) Step2(format string, arg0, arg1 string) { t.Stepf(format, arg0, arg1) } +// Call0 is used to trace a no-arguments function call. +// +// Usage: +// if trace.Tracing { +// defer trace.Call0()() +// } func (t *Tracer) Call0() func() { return t.traceCall() } +// Call1 is used to trace a function call with a single string argument. +// +// Usage: +// if trace.Tracing { +// defer trace.Call1(str1)() +// } func (t *Tracer) Call1(arg1 string) func() { return t.traceCall(arg1) } +// Call2 is used to trace a function call with 2 string arguments. +// +// Usage: +// if trace.Tracing { +// defer trace.Call2(str1, str2)() +// } func (t *Tracer) Call2(arg1, arg2 string) func() { return t.traceCall(arg1, arg2) } @@ -96,19 +114,19 @@ func (t *Tracer) traceCall(args ...interface{}) func() { panic("Internal pkglint error: calls to trace.Call must only occur in tracing mode") } - funcname := "?" + functionName := "?" if pc, _, _, ok := runtime.Caller(2); ok { if fn := runtime.FuncForPC(pc); fn != nil { - funcname = strings.TrimPrefix(fn.Name(), "netbsd.org/pkglint.") + functionName = strings.TrimPrefix(fn.Name(), "netbsd.org/pkglint.") } } indent := t.traceIndent() - _, _ = fmt.Fprintf(t.Out, "TRACE: %s+ %s(%s)\n", indent, funcname, argsStr(withoutResults(args))) + _, _ = fmt.Fprintf(t.Out, "TRACE: %s+ %s(%s)\n", indent, functionName, argsStr(withoutResults(args))) t.depth++ return func() { t.depth-- - _, _ = fmt.Fprintf(t.Out, "TRACE: %s- %s(%s)\n", indent, funcname, argsStr(withResults(args))) + _, _ = fmt.Fprintf(t.Out, "TRACE: %s- %s(%s)\n", indent, functionName, argsStr(withResults(args))) } } diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go index d56890cf9d4..2fe219eaa51 100644 --- a/pkgtools/pkglint/files/util.go +++ b/pkgtools/pkglint/files/util.go @@ -347,7 +347,8 @@ func mkopSubst(s string, left bool, from string, right bool, to string, flags st }) } -// relpath returns the relative path from `from` to `to`. +// relpath returns the relative path from the directory "from" +// to the filesystem entry "to". func relpath(from, to string) string { // From "dir" to "dir/subdir/...". @@ -454,30 +455,56 @@ func (o *Once) check(key uint64) bool { // Scope remembers which variables are defined and which are used // in a certain scope, such as a package or a file. type Scope struct { - defined map[string]MkLine + firstDef map[string]MkLine // TODO: Can this be removed? + lastDef map[string]MkLine + value map[string]string used map[string]MkLine fallback map[string]string } func NewScope() Scope { - return Scope{make(map[string]MkLine), make(map[string]MkLine), make(map[string]string)} + return Scope{ + make(map[string]MkLine), + make(map[string]MkLine), + make(map[string]string), + make(map[string]MkLine), + make(map[string]string)} } // Define marks the variable and its canonicalized form as defined. func (s *Scope) Define(varname string, mkline MkLine) { - if s.defined[varname] == nil { - s.defined[varname] = mkline - if trace.Tracing { - trace.Step2("Defining %q in %s", varname, mkline.String()) + def := func(name string) { + if s.firstDef[name] == nil { + s.firstDef[name] = mkline + if trace.Tracing { + trace.Step2("Defining %q for the first time in %s", name, mkline.String()) + } + } else if trace.Tracing { + trace.Step2("Defining %q in %s", name, mkline.String()) + } + + s.lastDef[name] = mkline + + // In most cases the defining lines are indeed variable assignments. + // Exceptions are comments that only document the variable but still mark + // it as defined so that it doesn't produce the "used but not defined" warning. + if mkline.IsVarassign() || mkline.IsCommentedVarassign() { + + switch mkline.Op() { + case opAssign, opAssignEval, opAssignShell: + s.value[name] = mkline.Value() + case opAssignAppend: + s.value[name] += " " + mkline.Value() + case opAssignDefault: + // No change to the value. + } } } + def(varname) varcanon := varnameCanon(varname) - if varcanon != varname && s.defined[varcanon] == nil { - s.defined[varcanon] = mkline - if trace.Tracing { - trace.Step2("Defining %q in %s", varcanon, mkline.String()) - } + if varcanon != varname { + def(varcanon) } } @@ -509,12 +536,12 @@ func (s *Scope) Use(varname string, line MkLine) { // Even if Defined returns true, FirstDefinition doesn't necessarily return true // since the latter ignores the default definitions from vardefs.go, keyword dummyVardefMkline. func (s *Scope) Defined(varname string) bool { - return s.defined[varname] != nil + return s.firstDef[varname] != nil } // DefinedSimilar tests whether the variable or its canonicalized form is defined. func (s *Scope) DefinedSimilar(varname string) bool { - if s.defined[varname] != nil { + if s.firstDef[varname] != nil { if trace.Tracing { trace.Step1("Variable %q is defined", varname) } @@ -522,7 +549,7 @@ func (s *Scope) DefinedSimilar(varname string) bool { } varcanon := varnameCanon(varname) - if s.defined[varcanon] != nil { + if s.firstDef[varcanon] != nil { if trace.Tracing { trace.Step2("Variable %q (similar to %q) is defined", varcanon, varname) } @@ -549,7 +576,25 @@ func (s *Scope) UsedSimilar(varname string) bool { // // Having multiple definitions is typical in the branches of "if" statements. func (s *Scope) FirstDefinition(varname string) MkLine { - mkline := s.defined[varname] + mkline := s.firstDef[varname] + if mkline != nil && mkline.IsVarassign() { + lastLine := s.LastDefinition(varname) + if lastLine != mkline { + mkline.Notef("FirstDefinition differs from LastDefinition in %s.", mkline.RefTo(lastLine)) + } + return mkline + } + return nil // See NewPackage and G.Pkgsrc.UserDefinedVars +} + +// LastDefinition returns the line in which the variable has been defined last. +// +// Having multiple definitions is typical in the branches of "if" statements. +// +// Another typical case involves two files: the included file defines a default +// value, and the including file later overrides that value. +func (s *Scope) LastDefinition(varname string) MkLine { + mkline := s.lastDef[varname] if mkline != nil && mkline.IsVarassign() { return mkline } @@ -560,8 +605,23 @@ func (s *Scope) FirstUse(varname string) MkLine { return s.used[varname] } -func (s *Scope) Value(varname string) (value string, found bool) { - mkline := s.FirstDefinition(varname) +// LastValue returns the value from the last variable definition. +// +// If an empty string is returned this can mean either that the +// variable value is indeed the empty string or that the variable +// was not found. To distinguish these cases, call LastValueFound instead. +func (s *Scope) LastValue(varname string) string { + value, _ := s.LastValueFound(varname) + return value +} + +func (s *Scope) LastValueFound(varname string) (value string, found bool) { + value, found = s.value[varname] + if found { + return + } + + mkline := s.LastDefinition(varname) if mkline != nil { return mkline.Value(), true } @@ -573,13 +633,14 @@ func (s *Scope) Value(varname string) (value string, found bool) { func (s *Scope) DefineAll(other Scope) { var varnames []string - for varname := range other.defined { + for varname := range other.firstDef { varnames = append(varnames, varname) } sort.Strings(varnames) for _, varname := range varnames { - s.Define(varname, other.defined[varname]) + s.Define(varname, other.firstDef[varname]) + s.Define(varname, other.lastDef[varname]) } } @@ -661,22 +722,26 @@ func naturalLess(str1, str2 string) bool { return len1 < len2 } -// RedundantScope checks for redundant variable definitions and accidentally -// overwriting variables. It tries to be as correct as possible by not flagging -// anything that is defined conditionally. There may be some edge cases though -// like defining PKGNAME, then evaluating it using :=, then defining it again. -// This pattern is so error-prone that it should not appear in pkgsrc at all, -// thus pkglint doesn't even expect it. (Well, except for the PKGNAME case, -// but that's deep in the infrastructure and only affects the "nb13" extension.) +// RedundantScope checks for redundant variable definitions and for variables +// that are accidentally overwritten. It tries to be as correct as possible +// by not flagging anything that is defined conditionally. +// +// There may be some edge cases though like defining PKGNAME, then evaluating +// it using :=, then defining it again. This pattern is so error-prone that +// it should not appear in pkgsrc at all, thus pkglint doesn't even expect it. +// (Well, except for the PKGNAME case, but that's deep in the infrastructure +// and only affects the "nb13" extension.) type RedundantScope struct { vars map[string]*redundantScopeVarinfo dirLevel int // The number of enclosing directives (.if, .for). - OnIgnore func(old, new MkLine) + includePath includePath + OnRedundant func(old, new MkLine) OnOverwrite func(old, new MkLine) } type redundantScopeVarinfo struct { - mkline MkLine - value string + mkline MkLine + includePath includePath + value string } func NewRedundantScope() *RedundantScope { @@ -684,10 +749,19 @@ func NewRedundantScope() *RedundantScope { } func (s *RedundantScope) Handle(mkline MkLine) { + if mkline.firstLine == 1 { + s.includePath.push(mkline.Location.Filename) + } else { + s.includePath.popUntil(mkline.Location.Filename) + } + switch { case mkline.IsVarassign(): varname := mkline.Varname() if s.dirLevel != 0 { + // Since the variable is defined or assigned conditionally, + // it becomes too complicated for pkglint to check all possible + // code paths. Therefore ignore the variable from now on. s.vars[varname] = nil break } @@ -707,7 +781,7 @@ func (s *RedundantScope) Handle(mkline MkLine) { if op == opAssignAppend { value = " " + value } - s.vars[varname] = &redundantScopeVarinfo{mkline, value} + s.vars[varname] = &redundantScopeVarinfo{mkline, s.includePath.copy(), value} } } else if existing != nil { @@ -717,12 +791,24 @@ func (s *RedundantScope) Handle(mkline MkLine) { switch op { case opAssign: - s.OnOverwrite(existing.mkline, mkline) + if s.includePath.includes(existing.includePath) { + // This is the usual pattern of including a file and + // then overwriting some of them. Although technically + // this overwrites the previous definition, it is not + // worth a warning since this is used a lot and + // intentionally. + } else { + s.OnOverwrite(existing.mkline, mkline) + } existing.value = value case opAssignAppend: existing.value += " " + value case opAssignDefault: - s.OnIgnore(existing.mkline, mkline) + if existing.includePath.includes(s.includePath) { + s.OnRedundant(mkline, existing.mkline) + } else if s.includePath.includes(existing.includePath) || s.includePath.equals(existing.includePath) { + s.OnRedundant(existing.mkline, mkline) + } case opAssignShell, opAssignEval: s.vars[varname] = nil // Won't be checked further. } @@ -738,6 +824,46 @@ func (s *RedundantScope) Handle(mkline MkLine) { } } +type includePath struct { + files []string +} + +func (p *includePath) push(filename string) { + p.files = append(p.files, filename) +} + +func (p *includePath) popUntil(filename string) { + for p.files[len(p.files)-1] != filename { + p.files = p.files[:len(p.files)-1] + } +} + +func (p *includePath) includes(other includePath) bool { + for i, filename := range p.files { + if i < len(other.files) && other.files[i] == filename { + continue + } + return false + } + return len(p.files) < len(other.files) +} + +func (p *includePath) equals(other includePath) bool { + if len(p.files) != len(other.files) { + return false + } + for i, filename := range p.files { + if other.files[i] != filename { + return false + } + } + return true +} + +func (p *includePath) copy() includePath { + return includePath{append([]string(nil), p.files...)} +} + // IsPrefs returns whether the given file, when included, loads the user // preferences. func IsPrefs(filename string) bool { diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go index 7087793afb4..743a66c95ca 100644 --- a/pkgtools/pkglint/files/util_test.go +++ b/pkgtools/pkglint/files/util_test.go @@ -322,6 +322,24 @@ func (s *Suite) Test_isLocallyModified(c *check.C) { c.Check(isLocallyModified(t.File("unmodified")), equals, false) } +func (s *Suite) Test_Scope_Define(c *check.C) { + t := s.Init(c) + + scope := NewScope() + scope.Define("BUILD_DIRS", t.NewMkLine("file.mk", 121, "BUILD_DIRS=\tone two three")) + + c.Check(scope.LastValue("BUILD_DIRS"), equals, "one two three") + + scope.Define("BUILD_DIRS", t.NewMkLine("file.mk", 123, "BUILD_DIRS+=\tfour")) + + c.Check(scope.LastValue("BUILD_DIRS"), equals, "one two three four") + + // Later default assignments do not have an effect. + scope.Define("BUILD_DIRS", t.NewMkLine("file.mk", 123, "BUILD_DIRS?=\tdefault")) + + c.Check(scope.LastValue("BUILD_DIRS"), equals, "one two three four") +} + func (s *Suite) Test_Scope_Defined(c *check.C) { t := s.Init(c) @@ -360,7 +378,8 @@ func (s *Suite) Test_Scope_DefineAll(c *check.C) { dst := NewScope() dst.DefineAll(src) - c.Check(dst.defined, check.HasLen, 0) + c.Check(dst.firstDef, check.HasLen, 0) + c.Check(dst.lastDef, check.HasLen, 0) c.Check(dst.used, check.HasLen, 0) src.Define("VAR", t.NewMkLine("file.mk", 1, "VAR=value")) @@ -373,7 +392,7 @@ func (s *Suite) Test_Scope_FirstDefinition(c *check.C) { t := s.Init(c) mkline1 := t.NewMkLine("fname.mk", 3, "VAR=\tvalue") - mkline2 := t.NewMkLine("fname.mk", 3, ".if ${VAR::=value}") + mkline2 := t.NewMkLine("fname.mk", 3, ".if ${SNEAKY::=value}") scope := NewScope() scope.Define("VAR", mkline1) @@ -388,6 +407,25 @@ func (s *Suite) Test_Scope_FirstDefinition(c *check.C) { t.Check(scope.FirstDefinition("SNEAKY"), check.IsNil) } +func (s *Suite) Test_Scope_LastValue(c *check.C) { + t := s.Init(c) + + mklines := t.NewMkLines("file.mk", + MkRcsID, + "VAR=\tfirst", + "VAR=\tsecond", + ".if 1", + "VAR=\tthird (conditional)", + ".endif") + + mklines.Check() + + t.Check(mklines.vars.LastValue("VAR"), equals, "third (conditional)") + + t.CheckOutputLines( + "WARN: file.mk:2: VAR is defined but not used.") +} + func (s *Suite) Test_Scope__no_tracing(c *check.C) { t := s.Init(c) @@ -672,3 +710,28 @@ func (s *Suite) Test_StringInterner(c *check.C) { t.Check(si.Intern("Hello, world"), equals, "Hello, world") t.Check(si.Intern("Hello, world"[0:5]), equals, "Hello") } + +func (s *Suite) Test_includePath_includes(c *check.C) { + t := s.Init(c) + + path := func(locations ...string) includePath { + return includePath{locations} + } + + var ( + m = path("Makefile") + mc = path("Makefile", "Makefile.common") + mco = path("Makefile", "Makefile.common", "other.mk") + mo = path("Makefile", "other.mk") + ) + + t.Check(m.includes(m), equals, false) + + t.Check(m.includes(mc), equals, true) + t.Check(m.includes(mco), equals, true) + t.Check(mc.includes(mco), equals, true) + + t.Check(mc.includes(m), equals, false) + t.Check(mc.includes(mo), equals, false) + t.Check(mo.includes(mc), equals, false) +} diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go index c5ff7b71c82..1fe3629e486 100644 --- a/pkgtools/pkglint/files/vartypecheck.go +++ b/pkgtools/pkglint/files/vartypecheck.go @@ -232,14 +232,6 @@ func (cv *VartypeCheck) Comment() { cv.Warnf("COMMENT should not begin with %q.", first) } - if m, isA := match1(value, `\b(is an?)\b`); m { - cv.Warnf("COMMENT should not contain %q.", isA) - G.Explain( - "The words \"package is a\" are redundant.", - "Since every package comment could start with them,", - "it is better to remove this redundancy in all cases.") - } - if G.Pkg != nil && G.Pkg.EffectivePkgbase != "" { pkgbase := G.Pkg.EffectivePkgbase if hasPrefix(strings.ToLower(value), strings.ToLower(pkgbase+" ")) { @@ -255,6 +247,14 @@ func (cv *VartypeCheck) Comment() { cv.Warnf("COMMENT should start with a capital letter.") } + if m, isA := match1(value, `\b(is an?)\b`); m { + cv.Warnf("COMMENT should not contain %q.", isA) + G.Explain( + "The words \"package is a\" are redundant.", + "Since every package comment could start with them,", + "it is better to remove this redundancy in all cases.") + } + if hasSuffix(value, ".") { cv.Warnf("COMMENT should not end with a period.") } diff --git a/pkgtools/pkglint/files/vartypecheck_test.go b/pkgtools/pkglint/files/vartypecheck_test.go index 3069a852274..2c9913dc4e7 100644 --- a/pkgtools/pkglint/files/vartypecheck_test.go +++ b/pkgtools/pkglint/files/vartypecheck_test.go @@ -134,6 +134,7 @@ func (s *Suite) Test_VartypeCheck_Comment(c *check.C) { "Package is an awesome package", "The Big New Package is a great package", "Converter converts between measurement units", + "Converter is a unit converter", "\"Official\" office suite", "'SQL injection fuzzer") @@ -148,7 +149,9 @@ func (s *Suite) Test_VartypeCheck_Comment(c *check.C) { "WARN: filename:7: COMMENT should not contain \"is a\".", "WARN: filename:8: COMMENT should not contain \"is an\".", "WARN: filename:9: COMMENT should not contain \"is a\".", - "WARN: filename:10: COMMENT should not start with the package name.") + "WARN: filename:10: COMMENT should not start with the package name.", + "WARN: filename:11: COMMENT should not start with the package name.", + "WARN: filename:11: COMMENT should not contain \"is a\".") } func (s *Suite) Test_VartypeCheck_ConfFiles(c *check.C) { @@ -516,7 +519,8 @@ func (s *Suite) Test_VartypeCheck_Homepage(c *check.C) { vt.Output( "WARN: filename:4: HOMEPAGE should not be defined in terms of MASTER_SITEs.") - delete(G.Pkg.vars.defined, "MASTER_SITES") + delete(G.Pkg.vars.firstDef, "MASTER_SITES") + delete(G.Pkg.vars.lastDef, "MASTER_SITES") G.Pkg.vars.Define("MASTER_SITES", t.NewMkLine(G.Pkg.File("Makefile"), 5, "MASTER_SITES=\thttps://cdn.NetBSD.org/pub/pkgsrc/distfiles/")) @@ -527,7 +531,8 @@ func (s *Suite) Test_VartypeCheck_Homepage(c *check.C) { "WARN: filename:5: HOMEPAGE should not be defined in terms of MASTER_SITEs. " + "Use https://cdn.NetBSD.org/pub/pkgsrc/distfiles/ directly.") - delete(G.Pkg.vars.defined, "MASTER_SITES") + delete(G.Pkg.vars.firstDef, "MASTER_SITES") + delete(G.Pkg.vars.lastDef, "MASTER_SITES") G.Pkg.vars.Define("MASTER_SITES", t.NewMkLine(G.Pkg.File("Makefile"), 5, "MASTER_SITES=\t${MASTER_SITE_GITHUB}")) |