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 5.7.8



details:   https://anonhg.NetBSD.org/pkgsrc/rev/5331bc788ab8
branches:  trunk
changeset: 333012:5331bc788ab8
user:      rillig <rillig%pkgsrc.org@localhost>
date:      Sun Apr 28 18:13:53 2019 +0000

description:
pkgtools/pkglint: update to 5.7.8

Changes since 5.7.7:

Warn about definitions of NOT_FOR_* and ONLY_FOR_* which are missing a
rationale. When maintaining such packages it helps a lot to know why the
package cannot be built on a particular platform or with a particular
compiler or Python version.

diffstat:

 pkgtools/pkglint/Makefile                    |   4 +-
 pkgtools/pkglint/files/logging.go            |   5 +-
 pkgtools/pkglint/files/mklinechecker.go      |  58 ++++++++++++++++++++
 pkgtools/pkglint/files/mklinechecker_test.go |  42 ++++++++++++-
 pkgtools/pkglint/files/pkgsrc_test.go        |  11 +++-
 pkgtools/pkglint/files/vardefs.go            |  81 +++++++++++++++++++--------
 pkgtools/pkglint/files/vartype.go            |   6 ++
 7 files changed, 170 insertions(+), 37 deletions(-)

diffs (truncated from 424 to 300 lines):

diff -r 8f00cd842725 -r 5331bc788ab8 pkgtools/pkglint/Makefile
--- a/pkgtools/pkglint/Makefile Sun Apr 28 17:11:53 2019 +0000
+++ b/pkgtools/pkglint/Makefile Sun Apr 28 18:13:53 2019 +0000
@@ -1,6 +1,6 @@
-# $NetBSD: Makefile,v 1.577 2019/04/27 19:33:56 rillig Exp $
+# $NetBSD: Makefile,v 1.578 2019/04/28 18:13:53 rillig Exp $
 
-PKGNAME=       pkglint-5.7.7
+PKGNAME=       pkglint-5.7.8
 CATEGORIES=    pkgtools
 DISTNAME=      tools
 MASTER_SITES=  ${MASTER_SITE_GITHUB:=golang/}
diff -r 8f00cd842725 -r 5331bc788ab8 pkgtools/pkglint/files/logging.go
--- a/pkgtools/pkglint/files/logging.go Sun Apr 28 17:11:53 2019 +0000
+++ b/pkgtools/pkglint/files/logging.go Sun Apr 28 18:13:53 2019 +0000
@@ -236,9 +236,8 @@
 
 // Errorf logs a technical error on the error output.
 //
-// location must be either an empty string or a slash-separated filename,
-// such as the one in Location.Filename. It may be followed by the usual
-// ":123" for line numbers.
+// location must be a slash-separated filename, such as the one in
+// Location.Filename. It may be followed by the usual ":123" for line numbers.
 //
 // For diagnostics, use Logf instead.
 func (l *Logger) Errorf(location string, format string, args ...interface{}) {
diff -r 8f00cd842725 -r 5331bc788ab8 pkgtools/pkglint/files/mklinechecker.go
--- a/pkgtools/pkglint/files/mklinechecker.go   Sun Apr 28 17:11:53 2019 +0000
+++ b/pkgtools/pkglint/files/mklinechecker.go   Sun Apr 28 18:13:53 2019 +0000
@@ -402,6 +402,63 @@
        ck.MkLine.Explain(expl...)
 }
 
+func (ck MkLineChecker) checkVarassignLeftRationale() {
+
+       isRationale := func(mkline MkLine) bool {
+               if mkline.IsVarassign() || mkline.IsCommentedVarassign() {
+                       return mkline.VarassignComment() != ""
+               }
+               return mkline.IsComment() && !hasPrefix(mkline.Text, "# $")
+       }
+
+       needsRationale := func(mkline MkLine) bool {
+               if !mkline.IsVarassign() && !mkline.IsCommentedVarassign() {
+                       return false
+               }
+               vartype := G.Pkgsrc.VariableType(ck.MkLines, mkline.Varname())
+               return vartype != nil && vartype.NeedsRationale()
+       }
+
+       mkline := ck.MkLine
+       if !needsRationale(mkline) {
+               return
+       }
+
+       if mkline.VarassignComment() != "" {
+               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,",
+               "the reasons for this restriction should be documented.",
+               "Otherwise it becomes too difficult to check whether these restrictions still apply",
+               "when the package is updated by someone else later.",
+               "",
+               "To add the rationale, put it in a comment at the end of this line,",
+               "or in a separate comment in the line above.",
+               "The rationale should try to answer these questions:",
+               "",
+               "* which specific aspects of the package are affected?",
+               "* if it's a dependency, is the dependency too old or too new?",
+               "* in which situations does a crash occur, if any?",
+               "* has it been reported upstream?")
+}
+
 // CheckVaruse checks a single use of a variable in a specific context.
 func (ck MkLineChecker) CheckVaruse(varuse *MkVarUse, vuc *VarUseContext) {
        mkline := ck.MkLine
@@ -932,6 +989,7 @@
        if !ck.checkVarassignLeftUserSettable() {
                ck.checkVarassignLeftPermissions()
        }
+       ck.checkVarassignLeftRationale()
 
        ck.checkTextVarUse(
                ck.MkLine.Varname(),
diff -r 8f00cd842725 -r 5331bc788ab8 pkgtools/pkglint/files/mklinechecker_test.go
--- a/pkgtools/pkglint/files/mklinechecker_test.go      Sun Apr 28 17:11:53 2019 +0000
+++ b/pkgtools/pkglint/files/mklinechecker_test.go      Sun Apr 28 18:13:53 2019 +0000
@@ -736,6 +736,38 @@
        t.CheckOutputEmpty()
 }
 
+func (s *Suite) Test_MkLineChecker_checkVarassignLeftRationale(c *check.C) {
+       t := s.Init(c)
+
+       t.SetUpVartypes()
+       mklines := t.NewMkLines("filename.mk",
+               MkRcsID,
+               "ONLY_FOR_PLATFORM=\t*-*-*", // The CVS Id above is not a rationale.
+               "NOT_FOR_PLATFORM=\t*-*-*",  // Neither does this line have a rationale.
+               "",
+               "ONLY_FOR_PLATFORM+=\t*-*-* # rationale",
+               "",
+               "# rationale in the line above",
+               "ONLY_FOR_PLATFORM+=\t*-*-*",
+               "",
+               "#VAR=\tvalue",               // This comment is not a rationale.
+               "ONLY_FOR_PLATFORM+=\t*-*-*", // Needs a rationale.
+               "",
+               "# rationale",
+               "BROKEN_ON_PLATFORM+=\t*-*-*",
+               "BROKEN_ON_PLATFORM+=\t*-*-*", // The rationale applies to this line, too.
+               "",
+               "PKGNAME=\tpackage-1.0", // Does not need a rationale.
+               "UNKNOWN=\t${UNKNOWN}")  // Unknown type, does not need a rationale.
+
+       mklines.Check()
+
+       t.CheckOutputLines(
+               "WARN: filename.mk:2: Setting variable ONLY_FOR_PLATFORM should have a rationale.",
+               "WARN: filename.mk:3: Setting variable NOT_FOR_PLATFORM should have a rationale.",
+               "WARN: filename.mk:11: Setting variable ONLY_FOR_PLATFORM should have a rationale.")
+}
+
 func (s *Suite) Test_MkLineChecker_checkVarassignOpShell(c *check.C) {
        t := s.Init(c)
 
@@ -1225,11 +1257,11 @@
        t.SetUpVartypes()
        mklines := t.NewMkLines("Makefile",
                MkRcsID,
-               "PYTHON_VERSIONS_ACCEPTED=\t36 __future__",
-               "PYTHON_VERSIONS_ACCEPTED=\t36 -13",
-               "PYTHON_VERSIONS_ACCEPTED=\t36 ${PKGVERSION_NOREV}",
-               "PYTHON_VERSIONS_ACCEPTED=\t36 37",
-               "PYTHON_VERSIONS_ACCEPTED=\t37 36 27 25")
+               "PYTHON_VERSIONS_ACCEPTED=\t36 __future__ # rationale",
+               "PYTHON_VERSIONS_ACCEPTED=\t36 -13 # rationale",
+               "PYTHON_VERSIONS_ACCEPTED=\t36 ${PKGVERSION_NOREV} # rationale",
+               "PYTHON_VERSIONS_ACCEPTED=\t36 37 # rationale",
+               "PYTHON_VERSIONS_ACCEPTED=\t37 36 27 25 # rationale")
 
        // TODO: All but the last of the above assignments should be flagged as
        //  redundant by RedundantScope; as of March 2019, that check is only
diff -r 8f00cd842725 -r 5331bc788ab8 pkgtools/pkglint/files/pkgsrc_test.go
--- a/pkgtools/pkglint/files/pkgsrc_test.go     Sun Apr 28 17:11:53 2019 +0000
+++ b/pkgtools/pkglint/files/pkgsrc_test.go     Sun Apr 28 18:13:53 2019 +0000
@@ -199,6 +199,12 @@
        t.CreateFileLines("mk/bsd.pkg.mk",
                MkRcsID,
                "_BUILD_DEFS+=\tPKG_SYSCONFBASEDIR PKG_SYSCONFDIR")
+       t.CreateFileLines("mk/defaults/mk.conf",
+               MkRcsID,
+               "",
+               "VARBASE=\t\t/var/pkg",
+               "PKG_SYSCONFBASEDIR=\t/usr/pkg/etc",
+               "PKG_SYSCONFDIR=\t/usr/pkg/etc")
        t.FinishSetUp()
 
        G.Check(pkg)
@@ -206,8 +212,9 @@
        c.Check(G.Pkgsrc.IsBuildDef("PKG_SYSCONFDIR"), equals, true)
        c.Check(G.Pkgsrc.IsBuildDef("VARBASE"), equals, false)
 
-       // FIXME: There should be a warning for VARBASE, but G.Pkgsrc.UserDefinedVars
-       //  does not contain anything at mklinechecker.go:/UserDefinedVars/.
+       t.CheckOutputLines(
+               "WARN: ~/category/package/Makefile:21: " +
+                       "The user-defined variable VARBASE is used but not added to BUILD_DEFS.")
 }
 
 func (s *Suite) Test_Pkgsrc_loadDocChanges__not_found(c *check.C) {
diff -r 8f00cd842725 -r 5331bc788ab8 pkgtools/pkglint/files/vardefs.go
--- a/pkgtools/pkglint/files/vardefs.go Sun Apr 28 17:11:53 2019 +0000
+++ b/pkgtools/pkglint/files/vardefs.go Sun Apr 28 18:13:53 2019 +0000
@@ -124,6 +124,14 @@
                        "Makefile, Makefile.*, *.mk: default, set, use")
        }
 
+       // Like pkg, but always needs a rationale.
+       pkgrat := func(varname string, basicType *BasicType) {
+               acl(varname, basicType,
+                       PackageSettable|NeedsRationale,
+                       "buildlink3.mk, builtin.mk: none",
+                       "Makefile, Makefile.*, *.mk: default, set, use")
+       }
+
        // pkgload is the same as pkg, except that the variable may be accessed at load time.
        pkgload := func(varname string, basicType *BasicType) {
                acl(varname, basicType,
@@ -145,6 +153,14 @@
                        "Makefile, Makefile.*, *.mk: default, set, append, use")
        }
 
+       // Like pkglist, but always needs a rationale.
+       pkglistrat := func(varname string, basicType *BasicType) {
+               acllist(varname, basicType,
+                       List|PackageSettable|NeedsRationale,
+                       "buildlink3.mk, builtin.mk: none",
+                       "Makefile, Makefile.*, *.mk: default, set, append, use")
+       }
+
        // pkgappend declares a variable that may use the += operator,
        // even though it is not a list where each item can be interpreted
        // on its own.
@@ -168,6 +184,14 @@
                        "Makefile, Makefile.*, *.mk: default, set, append, use")
        }
 
+       // Like pkgappend, but always needs a rationale.
+       pkgappendrat := func(varname string, basicType *BasicType) {
+               acl(varname, basicType,
+                       PackageSettable|NeedsRationale,
+                       "buildlink3.mk, builtin.mk: none",
+                       "Makefile, Makefile.*, *.mk: default, set, append, use")
+       }
+
        // Some package-defined variables may be modified in buildlink3.mk files.
        // These variables are typically related to compiling and linking files
        // from C and related languages.
@@ -184,6 +208,13 @@
                        "Makefile, Makefile.*, *.mk: default, set, append, use")
        }
 
+       // Like pkglistbl3, but always needs a rationale.
+       pkglistbl3rat := func(varname string, basicType *BasicType) {
+               acl(varname, basicType,
+                       List|PackageSettable|NeedsRationale,
+                       "Makefile, Makefile.*, *.mk: default, set, append, use")
+       }
+
        // sys declares a user-defined or system-defined variable that must not
        // be modified by packages.
        //
@@ -776,10 +807,10 @@
        pkglist("BOOTSTRAP_DEPENDS", BtDependencyWithPath)
        pkg("BOOTSTRAP_PKG", BtYesNo)
        // BROKEN should better be a list of messages instead of a simple string.
-       pkgappend("BROKEN", BtMessage)
+       pkgappendrat("BROKEN", BtMessage)
        pkg("BROKEN_GETTEXT_DETECTION", BtYesNo)
-       pkglist("BROKEN_EXCEPT_ON_PLATFORM", BtMachinePlatformPattern)
-       pkglist("BROKEN_ON_PLATFORM", BtMachinePlatformPattern)
+       pkglistrat("BROKEN_EXCEPT_ON_PLATFORM", BtMachinePlatformPattern)
+       pkglistrat("BROKEN_ON_PLATFORM", BtMachinePlatformPattern)
        syslist("BSD_MAKE_ENV", BtShellWord)
        // TODO: Align the permissions of the various BUILDLINK_*.* variables with each other.
        acllist("BUILDLINK_ABI_DEPENDS.*", BtDependency,
@@ -1026,7 +1057,7 @@
        pkglist("EMACS_VERSIONS_ACCEPTED", emacsVersions)
        sys("EMACS_VERSION_MAJOR", BtInteger)
        sys("EMACS_VERSION_MINOR", BtInteger)
-       pkglist("EMACS_VERSION_REQD", emacsVersions)
+       pkglistrat("EMACS_VERSION_REQD", emacsVersions)
        sys("EMULDIR", BtPathname)
        sys("EMULSUBDIR", BtPathname)
        sys("OPSYS_EMULDIR", BtPathname)
@@ -1070,17 +1101,17 @@
        pkglist("FILES_SUBST", BtShellWord)
        syslist("FILES_SUBST_SED", BtShellWord)
        pkglist("FIX_RPATH", BtVariableName)
-       pkglist("FLEX_REQD", BtVersion)
+       pkglistrat("FLEX_REQD", BtVersion)
        pkglist("FONTS_DIRS.*", BtPathname)
        syslist("GAMEDATA_PERMS", BtPerms)
        syslist("GAMEDIR_PERMS", BtPerms)
-       pkglistbl3("GCC_REQD", BtGccReqd)
+       pkglistbl3rat("GCC_REQD", BtGccReqd)
        pkgappend("GENERATE_PLIST", BtShellCommands)
        pkg("GITHUB_PROJECT", BtIdentifier)
        pkg("GITHUB_TAG", BtIdentifier)
        pkg("GITHUB_RELEASE", BtFileName)
        pkg("GITHUB_TYPE", enum("tag release"))
-       pkg("GMAKE_REQD", BtVersion)
+       pkgrat("GMAKE_REQD", BtVersion)
        // Some packages need to set GNU_ARCH.i386 to either i486 or i586.
        pkg("GNU_ARCH.*", BtIdentifier)
        // GNU_CONFIGURE needs to be tested in some buildlink3.mk files,
@@ -1104,7 +1135,7 @@
                PackageSettable,
                "*: set, use-loadtime")
        sys("IMAKE", BtShellCommand)
-       pkglistbl3("INCOMPAT_CURSES", BtMachinePlatformPattern)



Home | Main Index | Thread Index | Old Index