pkgtools/pkglint: update to 19.3.3

Changes since 19.3.2:

The rationale for variables like BROKEN, GCC_REQD and for direct
inclusion of builtin.mk files may span multiple lines, and it may end
with an empty comment line.

13 files changed, 155 insertions, 45 deletions

diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile
index 5df1338bee5..e6a7c80c24a 100644
--- a/pkgtools/pkglint/Makefile
+++ b/pkgtools/pkglint/Makefile
@@ -1,6 +1,6 @@
-# $NetBSD: Makefile,v 1.602 2019/10/26 09:51:47 rillig Exp $
+# $NetBSD: Makefile,v 1.603 2019/10/26 11:43:36 rillig Exp $
 
-PKGNAME=	pkglint-19.3.2
+PKGNAME=	pkglint-19.3.3
 CATEGORIES=	pkgtools
 DISTNAME=	tools
 MASTER_SITES=	${MASTER_SITE_GITHUB:=golang/}
diff --git a/pkgtools/pkglint/files/check_test.go b/pkgtools/pkglint/files/check_test.go
index d310e867c28..1dabb5237d2 100644
--- a/pkgtools/pkglint/files/check_test.go
+++ b/pkgtools/pkglint/files/check_test.go
@@ -403,8 +403,14 @@ func (t *Tester) SetUpPackage(pkgpath string, makefileLines ...string) string {
 		"LICENSE=\t2-clause-bsd",
 		"",
 		".include \"suppress-varorder.mk\""}
-	for len(mlines) < 19 {
-		mlines = append(mlines, "# empty")
+	if len(mlines) < 19 {
+		mlines = append(mlines, "")
+	}
+	for len(mlines) < 18 {
+		mlines = append(mlines, "# filler")
+	}
+	if len(mlines) < 19 {
+		mlines = append(mlines, "")
 	}
 
 line:
diff --git a/pkgtools/pkglint/files/mkline.go b/pkgtools/pkglint/files/mkline.go
index a951e6989c1..5d85d5f19c0 100644
--- a/pkgtools/pkglint/files/mkline.go
+++ b/pkgtools/pkglint/files/mkline.go
@@ -76,6 +76,8 @@ func (mkline *MkLine) String() string {
 
 func (mkline *MkLine) HasComment() bool { return mkline.splitResult.hasComment }
 
+func (mkline *MkLine) HasRationale() bool { return mkline.splitResult.hasRationale }
+
 // Comment returns the comment after the first unescaped #.
 //
 // A special case are variable assignments. If these are commented out
diff --git a/pkgtools/pkglint/files/mklinechecker.go b/pkgtools/pkglint/files/mklinechecker.go
index 28958f2c4fe..3446a0ea5fd 100644
--- a/pkgtools/pkglint/files/mklinechecker.go
+++ b/pkgtools/pkglint/files/mklinechecker.go
@@ -131,8 +131,7 @@ func (ck MkLineChecker) checkInclude() {
 		mkline.Warnf("Please write \"USE_TOOLS+= intltool\" instead of this line.")
 
 	case hasSuffix(includedFile, "/builtin.mk"):
-		// TODO: mkline.HasRationale
-		if mkline.Basename != "hacks.mk" && !mkline.HasComment() {
+		if mkline.Basename != "hacks.mk" && !mkline.HasRationale() {
 			fix := mkline.Autofix()
 			fix.Errorf("%s must not be included directly. Include \"%s/buildlink3.mk\" instead.", includedFile, path.Dir(includedFile))
 			fix.Replace("builtin.mk", "buildlink3.mk")
@@ -438,12 +437,6 @@ func (ck MkLineChecker) checkVarassignLeftRationale() {
 		return
 	}
 
-	isRationale := func(mkline *MkLine) bool {
-		return mkline.IsComment() &&
-			!hasPrefix(mkline.Text, "# $") &&
-			!mkline.IsCommentedVarassign()
-	}
-
 	needsRationale := func(mkline *MkLine) bool {
 		if !mkline.IsVarassignMaybeCommented() {
 			return false
@@ -457,24 +450,10 @@ func (ck MkLineChecker) checkVarassignLeftRationale() {
 		return
 	}
 
-	if mkline.HasComment() {
+	if mkline.HasRationale() {
 		return
 	}
 
-	// Check whether there is a comment directly above.
-	for i, other := range ck.MkLines.mklines {
-		if other == mkline && i > 0 {
-			aboveIndex := i - 1
-			for aboveIndex > 0 && needsRationale(ck.MkLines.mklines[aboveIndex]) {
-				aboveIndex--
-			}
-
-			if isRationale(ck.MkLines.mklines[aboveIndex]) {
-				return
-			}
-		}
-	}
-
 	mkline.Warnf("Setting variable %s should have a rationale.", mkline.Varname())
 	mkline.Explain(
 		"Since this variable prevents the package from being built in some situations,",
diff --git a/pkgtools/pkglint/files/mklinechecker_test.go b/pkgtools/pkglint/files/mklinechecker_test.go
index 3d78b54a721..d51ac81585d 100644
--- a/pkgtools/pkglint/files/mklinechecker_test.go
+++ b/pkgtools/pkglint/files/mklinechecker_test.go
@@ -150,15 +150,15 @@ func (s *Suite) Test_MkLineChecker_checkVarassignLeftUserSettable(c *check.C) {
 	//  the expected reading order of human readers.
 
 	t.SetUpPackage("category/package",
-		"ASSIGN_DIFF=\tpkg",          // assignment, differs from default value
-		"ASSIGN_DIFF2=\treally # ok", // ok because of the rationale in the comment
-		"ASSIGN_SAME=\tdefault",      // assignment, same value as default
-		"DEFAULT_DIFF?=\tpkg",        // default, differs from default value
-		"DEFAULT_SAME?=\tdefault",    // same value as default
-		"FETCH_USING=\tcurl",         // both user-settable and package-settable
-		"APPEND_DIRS+=\tdir3",        // appending requires a separate diagnostic
-		"COMMENTED_SAME?=\tdefault",  // commented default, same value as default
-		"COMMENTED_DIFF?=\tpkg")      // commented default, differs from default value
+		"ASSIGN_DIFF=\t\tpkg",          // assignment, differs from default value
+		"ASSIGN_DIFF2=\t\treally # ok", // ok because of the rationale in the comment
+		"ASSIGN_SAME=\t\tdefault",      // assignment, same value as default
+		"DEFAULT_DIFF?=\t\tpkg",        // default, differs from default value
+		"DEFAULT_SAME?=\t\tdefault",    // same value as default
+		"FETCH_USING=\t\tcurl",         // both user-settable and package-settable
+		"APPEND_DIRS+=\t\tdir3",        // appending requires a separate diagnostic
+		"COMMENTED_SAME?=\tdefault",    // commented default, same value as default
+		"COMMENTED_DIFF?=\tpkg")        // commented default, differs from default value
 	t.CreateFileLines("mk/defaults/mk.conf",
 		MkCvsID,
 		"ASSIGN_DIFF?=default",
@@ -413,6 +413,26 @@ func (s *Suite) Test_MkLineChecker_checkInclude__builtin_mk(c *check.C) {
 			"Include \"../../category/package/buildlink3.mk\" instead.")
 }
 
+func (s *Suite) Test_MkLineChecker_checkInclude__builtin_mk_rationale(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpPackage("category/package",
+		"# I have good reasons for including this file directly.",
+		".include \"../../category/package/builtin.mk\"",
+		"",
+		".include \"../../category/package/builtin.mk\"")
+	t.CreateFileLines("category/package/builtin.mk",
+		MkCvsID)
+	t.FinishSetUp()
+
+	G.checkdirPackage(t.File("category/package"))
+
+	t.CheckOutputLines(
+		"ERROR: ~/category/package/Makefile:23: " +
+			"../../category/package/builtin.mk must not be included directly. " +
+			"Include \"../../category/package/buildlink3.mk\" instead.")
+}
+
 func (s *Suite) Test_MkLineChecker__permissions_in_hacks_mk(c *check.C) {
 	t := s.Init(c)
 
diff --git a/pkgtools/pkglint/files/mklineparser.go b/pkgtools/pkglint/files/mklineparser.go
index f1db601aca9..1a1d2da49f5 100644
--- a/pkgtools/pkglint/files/mklineparser.go
+++ b/pkgtools/pkglint/files/mklineparser.go
@@ -355,7 +355,7 @@ func (MkLineParser) split(line *Line, text string, trimComment bool) mkLineSplit
 		}
 	}
 
-	return mkLineSplitResult{mainTrimmed, tokens, spaceBeforeComment, hasComment, comment}
+	return mkLineSplitResult{mainTrimmed, tokens, spaceBeforeComment, hasComment, false, comment}
 }
 
 // unescapeComment takes a Makefile line, as written in a file, and splits
@@ -446,5 +446,6 @@ type mkLineSplitResult struct {
 	tokens             []*MkToken
 	spaceBeforeComment string
 	hasComment         bool
+	hasRationale       bool // filled in later, by MkLines.collectRationale
 	comment            string
 }
diff --git a/pkgtools/pkglint/files/mklineparser_test.go b/pkgtools/pkglint/files/mklineparser_test.go
index 731d318416d..ab36d9499ea 100644
--- a/pkgtools/pkglint/files/mklineparser_test.go
+++ b/pkgtools/pkglint/files/mklineparser_test.go
@@ -1007,6 +1007,7 @@ func (s *Suite) Test_MkLineParser_split__unclosed_varuse(c *check.C) {
 					"EGDIR/apparmor.d ${EGDIR/dbus-1/system.d ${EGDIR/pam.d")),
 			"",
 			false,
+			false,
 			"",
 		},
 
diff --git a/pkgtools/pkglint/files/mklines.go b/pkgtools/pkglint/files/mklines.go
index 4741c8860cc..f91d65cec86 100644
--- a/pkgtools/pkglint/files/mklines.go
+++ b/pkgtools/pkglint/files/mklines.go
@@ -86,6 +86,7 @@ func (mklines *MkLines) Check() {
 
 	// In the first pass, all additions to BUILD_DEFS and USE_TOOLS
 	// are collected to make the order of the definitions irrelevant.
+	mklines.collectRationale()
 	mklines.collectUsedVariables()
 	mklines.collectVariables()
 	mklines.collectPlistVars()
@@ -403,6 +404,25 @@ func (mklines *MkLines) collectElse() {
 	// TODO: Check whether this ForEach is redundant because it is already run somewhere else.
 }
 
+func (mklines *MkLines) collectRationale() {
+
+	useful := func(mkline *MkLine) bool {
+		comment := trimHspace(mkline.Comment())
+		return comment != "" && !hasPrefix(comment, "$")
+	}
+
+	realComment := func(mkline *MkLine) bool {
+		return mkline.IsComment() && !mkline.IsCommentedVarassign()
+	}
+
+	rationale := false
+	for _, mkline := range mklines.mklines {
+		rationale = rationale || realComment(mkline) && useful(mkline)
+		mkline.splitResult.hasRationale = rationale || useful(mkline)
+		rationale = rationale && !mkline.IsEmpty()
+	}
+}
+
 func (mklines *MkLines) collectUsedVariables() {
 	for _, mkline := range mklines.mklines {
 		mkline.ForEachUsed(func(varUse *MkVarUse, time VucTime) {
diff --git a/pkgtools/pkglint/files/mklines_test.go b/pkgtools/pkglint/files/mklines_test.go
index 9b04f407f76..a8b01925fe4 100644
--- a/pkgtools/pkglint/files/mklines_test.go
+++ b/pkgtools/pkglint/files/mklines_test.go
@@ -501,6 +501,77 @@ func (s *Suite) Test_MkLines_ExpandLoopVar__malformed_for(c *check.C) {
 	t.Check(values, check.HasLen, 0)
 }
 
+func (s *Suite) Test_MkLines_collectRationale(c *check.C) {
+	t := s.Init(c)
+
+	test := func(specs ...string) {
+		lines := mapStr(specs, func(s string) string { return s[4:] })
+		mklines := t.NewMkLines("filename.mk", lines...)
+		mklines.collectRationale()
+		var actual []string
+		mklines.ForEach(func(mkline *MkLine) {
+			actual = append(actual, condStr(mkline.HasRationale(), "R   ", "-   ")+mkline.Text)
+		})
+		t.CheckDeepEquals(actual, specs)
+	}
+
+	// An uncommented line does not have a rationale.
+	test(
+		"-   VAR=value")
+
+	// The rationale can be given at the end of the line.
+	// This is useful for short explanations or remarks.
+	test(
+		"R   VAR=value # rationale")
+
+	// A rationale can be given above a line. This is useful for
+	// explanations that don't fit into a single line.
+	test(
+		"R   # rationale",
+		"R   VAR=value")
+
+	// A commented variable assignment is just that, commented code.
+	// It does not count as a rationale.
+	test(
+		"-   #VAR=value",
+		"-   VAR=value")
+
+	// An empty line ends the rationale. The empty line does have a
+	// rationale itself, but that is useless since pkglint doesn't
+	// check empty lines for rationales.
+	test(
+		"R   # rationale",
+		"R   ",
+		"-   VAR=value")
+
+	// A rationale covers all lines that follow, until the next empty
+	// line.
+	test(
+		"R   # rationale",
+		"R   NOT_FOR_PLATFORM+=\tLinux-*-*",
+		"R   NOT_FOR_PLATFORM+=\tNetBSD-*-*",
+		"R   NOT_FOR_PLATFORM+=\tCygwin-*-*")
+
+	// Large comment blocks often end with an empty comment line.
+	test(
+		"R   # rationale",
+		"R   #",
+		"R   NOT_FOR_PLATFORM+=\tLinux-*-*",
+		"R   NOT_FOR_PLATFORM+=\tNetBSD-*-*",
+		"R   NOT_FOR_PLATFORM+=\tCygwin-*-*")
+
+	// The CVS Id is not a rationale.
+	test(
+		"-   "+MkCvsID,
+		"-   VAR=\tvalue")
+
+	// A rationale at the end of a line applies only to that line.
+	test(
+		"-   VAR=\tvalue",
+		"R   VAR=\tvalue # rationale",
+		"-   VAR=\tvalue")
+}
+
 func (s *Suite) Test_MkLines_collectVariables(c *check.C) {
 	t := s.Init(c)
 
diff --git a/pkgtools/pkglint/files/package_test.go b/pkgtools/pkglint/files/package_test.go
index 24c34625726..7873af65b1b 100644
--- a/pkgtools/pkglint/files/package_test.go
+++ b/pkgtools/pkglint/files/package_test.go
@@ -2797,12 +2797,12 @@ func (s *Suite) Test_Package_parse__include_infrastructure(c *check.C) {
 		"~/category/package/Makefile:12: ",
 		"~/category/package/Makefile:13: .include \"suppress-varorder.mk\"",
 		"~/category/package/suppress-varorder.mk:1: "+MkCvsID,
-		"~/category/package/Makefile:14: # empty",
-		"~/category/package/Makefile:15: # empty",
-		"~/category/package/Makefile:16: # empty",
-		"~/category/package/Makefile:17: # empty",
-		"~/category/package/Makefile:18: # empty",
-		"~/category/package/Makefile:19: # empty",
+		"~/category/package/Makefile:14: ",
+		"~/category/package/Makefile:15: # filler",
+		"~/category/package/Makefile:16: # filler",
+		"~/category/package/Makefile:17: # filler",
+		"~/category/package/Makefile:18: # filler",
+		"~/category/package/Makefile:19: ",
 		"~/category/package/Makefile:20: .include \"../../mk/dlopen.buildlink3.mk\"",
 		"~/category/package/../../mk/dlopen.buildlink3.mk:1: .include \"dlopen.builtin.mk\"",
 		"~/mk/dlopen.builtin.mk:1: .include \"pthread.builtin.mk\"",
diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go
index b510fdc074c..8a0e0f73f45 100644
--- a/pkgtools/pkglint/files/shell_test.go
+++ b/pkgtools/pkglint/files/shell_test.go
@@ -1474,7 +1474,7 @@ func (s *Suite) Test_SimpleCommandChecker_checkAutoMkdirs__redundant(c *check.C)
 	t := s.Init(c)
 
 	t.SetUpPackage("category/package",
-		"AUTO_MKDIRS=\tyes",
+		"AUTO_MKDIRS=\t\tyes",
 		"INSTALLATION_DIRS+=\tshare/redundant",
 		"INSTALLATION_DIRS+=\tnot/redundant ${EGDIR}")
 	t.CreateFileLines("category/package/PLIST",
diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go
index 4d7a3712630..b2634f7b6fb 100644
--- a/pkgtools/pkglint/files/util.go
+++ b/pkgtools/pkglint/files/util.go
@@ -82,6 +82,14 @@ func containsStr(slice []string, s string) bool {
 	return false
 }
 
+func mapStr(slice []string, fn func(s string) string) []string {
+	result := make([]string, len(slice))
+	for i, str := range slice {
+		result[i] = fn(str)
+	}
+	return result
+}
+
 // intern returns an independent copy of the given string.
 //
 // It should be called when only a small substring of a large string
diff --git a/pkgtools/pkglint/files/vardefs_test.go b/pkgtools/pkglint/files/vardefs_test.go
index 55f62921fc7..e114a4dcd27 100644
--- a/pkgtools/pkglint/files/vardefs_test.go
+++ b/pkgtools/pkglint/files/vardefs_test.go
@@ -186,7 +186,9 @@ func (s *Suite) Test_VarTypeRegistry_Init__LP64PLATFORMS(c *check.C) {
 	G.Check(pkg)
 
 	// No warning about a missing :Q modifier.
-	t.CheckOutputEmpty()
+	t.CheckOutputLines(
+		"WARN: ~/category/package/Makefile:20: " +
+			"Setting variable BROKEN_ON_PLATFORM should have a rationale.")
 }
 
 func (s *Suite) Test_VarTypeRegistry_Init__no_tracing(c *check.C) {