pkgsrc-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[pkgsrc/trunk]: pkgsrc/pkgtools/pkglint pkgtools/pkglint: update to 19.3.3



details:   https://anonhg.NetBSD.org/pkgsrc/rev/29507b490a2d
branches:  trunk
changeset: 403382:29507b490a2d
user:      rillig <rillig%pkgsrc.org@localhost>
date:      Sat Oct 26 11:43:36 2019 +0000

description:
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.

diffstat:

 pkgtools/pkglint/Makefile                    |   4 +-
 pkgtools/pkglint/files/check_test.go         |  10 +++-
 pkgtools/pkglint/files/mkline.go             |   2 +
 pkgtools/pkglint/files/mklinechecker.go      |  25 +---------
 pkgtools/pkglint/files/mklinechecker_test.go |  38 +++++++++++---
 pkgtools/pkglint/files/mklineparser.go       |   3 +-
 pkgtools/pkglint/files/mklineparser_test.go  |   1 +
 pkgtools/pkglint/files/mklines.go            |  20 +++++++
 pkgtools/pkglint/files/mklines_test.go       |  71 ++++++++++++++++++++++++++++
 pkgtools/pkglint/files/package_test.go       |  12 ++--
 pkgtools/pkglint/files/shell_test.go         |   2 +-
 pkgtools/pkglint/files/util.go               |   8 +++
 pkgtools/pkglint/files/vardefs_test.go       |   4 +-
 13 files changed, 155 insertions(+), 45 deletions(-)

diffs (truncated from 365 to 300 lines):

diff -r f5ad1e1e4661 -r 29507b490a2d pkgtools/pkglint/Makefile
--- a/pkgtools/pkglint/Makefile Sat Oct 26 11:32:27 2019 +0000
+++ b/pkgtools/pkglint/Makefile Sat Oct 26 11:43:36 2019 +0000
@@ -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 -r f5ad1e1e4661 -r 29507b490a2d pkgtools/pkglint/files/check_test.go
--- a/pkgtools/pkglint/files/check_test.go      Sat Oct 26 11:32:27 2019 +0000
+++ b/pkgtools/pkglint/files/check_test.go      Sat Oct 26 11:43:36 2019 +0000
@@ -403,8 +403,14 @@
                "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 -r f5ad1e1e4661 -r 29507b490a2d pkgtools/pkglint/files/mkline.go
--- a/pkgtools/pkglint/files/mkline.go  Sat Oct 26 11:32:27 2019 +0000
+++ b/pkgtools/pkglint/files/mkline.go  Sat Oct 26 11:43:36 2019 +0000
@@ -76,6 +76,8 @@
 
 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 -r f5ad1e1e4661 -r 29507b490a2d pkgtools/pkglint/files/mklinechecker.go
--- a/pkgtools/pkglint/files/mklinechecker.go   Sat Oct 26 11:32:27 2019 +0000
+++ b/pkgtools/pkglint/files/mklinechecker.go   Sat Oct 26 11:43:36 2019 +0000
@@ -131,8 +131,7 @@
                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 @@
                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 @@
                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 -r f5ad1e1e4661 -r 29507b490a2d pkgtools/pkglint/files/mklinechecker_test.go
--- a/pkgtools/pkglint/files/mklinechecker_test.go      Sat Oct 26 11:32:27 2019 +0000
+++ b/pkgtools/pkglint/files/mklinechecker_test.go      Sat Oct 26 11:43:36 2019 +0000
@@ -150,15 +150,15 @@
        //  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 @@
                        "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 -r f5ad1e1e4661 -r 29507b490a2d pkgtools/pkglint/files/mklineparser.go
--- a/pkgtools/pkglint/files/mklineparser.go    Sat Oct 26 11:32:27 2019 +0000
+++ b/pkgtools/pkglint/files/mklineparser.go    Sat Oct 26 11:43:36 2019 +0000
@@ -355,7 +355,7 @@
                }
        }
 
-       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 @@
        tokens             []*MkToken
        spaceBeforeComment string
        hasComment         bool
+       hasRationale       bool // filled in later, by MkLines.collectRationale
        comment            string
 }
diff -r f5ad1e1e4661 -r 29507b490a2d pkgtools/pkglint/files/mklineparser_test.go
--- a/pkgtools/pkglint/files/mklineparser_test.go       Sat Oct 26 11:32:27 2019 +0000
+++ b/pkgtools/pkglint/files/mklineparser_test.go       Sat Oct 26 11:43:36 2019 +0000
@@ -1007,6 +1007,7 @@
                                        "EGDIR/apparmor.d ${EGDIR/dbus-1/system.d ${EGDIR/pam.d")),
                        "",
                        false,
+                       false,
                        "",
                },
 
diff -r f5ad1e1e4661 -r 29507b490a2d pkgtools/pkglint/files/mklines.go
--- a/pkgtools/pkglint/files/mklines.go Sat Oct 26 11:32:27 2019 +0000
+++ b/pkgtools/pkglint/files/mklines.go Sat Oct 26 11:43:36 2019 +0000
@@ -86,6 +86,7 @@
 
        // 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 @@
        // 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 -r f5ad1e1e4661 -r 29507b490a2d pkgtools/pkglint/files/mklines_test.go
--- a/pkgtools/pkglint/files/mklines_test.go    Sat Oct 26 11:32:27 2019 +0000
+++ b/pkgtools/pkglint/files/mklines_test.go    Sat Oct 26 11:43:36 2019 +0000
@@ -501,6 +501,77 @@
        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 -r f5ad1e1e4661 -r 29507b490a2d pkgtools/pkglint/files/package_test.go



Home | Main Index | Thread Index | Old Index