[pkgsrc/trunk]: pkgsrc/pkgtools/pkglint/files pkgtools/pkglint: hotfix for re...

[rillig <rillig%pkgsrc.org@localhost>]

details:   https://anonhg.NetBSD.org/pkgsrc/rev/17736e37f2e5
branches:  trunk
changeset: 383352:17736e37f2e5
user:      rillig <rillig%pkgsrc.org@localhost>
date:      Sat Jul 28 20:44:45 2018 +0000

description:
pkgtools/pkglint: hotfix for release 5.5.15

* Fixed detection of redundant variable definitions.
* Fixed check for PATH environment variable.

diffstat:

 pkgtools/pkglint/files/mkline.go            |  23 +++++++++++++++++++++++
 pkgtools/pkglint/files/mkline_test.go       |  29 +++++++++++++++++++++++++++++
 pkgtools/pkglint/files/mklines.go           |   2 +-
 pkgtools/pkglint/files/mklines_test.go      |  11 +++++++++++
 pkgtools/pkglint/files/vartypecheck.go      |  11 +++++++----
 pkgtools/pkglint/files/vartypecheck_test.go |   5 +++--
 6 files changed, 74 insertions(+), 7 deletions(-)

diffs (156 lines):

diff -r 5f04264688fe -r 17736e37f2e5 pkgtools/pkglint/files/mkline.go
--- a/pkgtools/pkglint/files/mkline.go  Sat Jul 28 18:31:33 2018 +0000
+++ b/pkgtools/pkglint/files/mkline.go  Sat Jul 28 20:44:45 2018 +0000
@@ -276,10 +276,33 @@
        return tokens
 }
 
+// ValueSplit splits the variable value of an assignment line,
+// taking care of variable references. For example, when the value
+// "/bin:${PATH:S,::,::,}" is split at ":", it results in
+// {"/bin", "${PATH:S,::,::,}"}.
+func (mkline *MkLineImpl) ValueSplit(value string, separator string) []string {
+       tokens := mkline.Tokenize(value)
+       var split []string
+       for _, token := range tokens {
+               if split == nil {
+                       split = []string{""}
+               }
+               if token.Varuse == nil && contains(token.Text, separator) {
+                       subs := strings.Split(token.Text, separator)
+                       split[len(split)-1] += subs[0]
+                       split = append(split, subs[1:]...)
+               } else {
+                       split[len(split)-1] += token.Text
+               }
+       }
+       return split
+}
+
 func (mkline *MkLineImpl) WithoutMakeVariables(value string) string {
        valueNovar := value
        for {
                var m []string
+               // TODO: properly parse nested variables
                m, valueNovar = regex.ReplaceFirst(valueNovar, `\$\{[^{}]*\}`, "")
                if m == nil {
                        return valueNovar
diff -r 5f04264688fe -r 17736e37f2e5 pkgtools/pkglint/files/mkline_test.go
--- a/pkgtools/pkglint/files/mkline_test.go     Sat Jul 28 18:31:33 2018 +0000
+++ b/pkgtools/pkglint/files/mkline_test.go     Sat Jul 28 20:44:45 2018 +0000
@@ -802,6 +802,35 @@
        c.Check(mkline.ConditionVars(), equals, "OPSYS")
 }
 
+func (s *Suite) Test_MkLine_ValueSplit(c *check.C) {
+       t := s.Init(c)
+
+       checkSplit := func(value string, expected ...string) {
+               mkline := t.NewMkLine("Makefile", 1, "PATH=\t"+value)
+               split := mkline.ValueSplit(value, ":")
+               c.Check(split, deepEquals, expected)
+       }
+
+       checkSplit("#empty",
+               []string(nil)...)
+
+       checkSplit("/bin",
+               "/bin")
+
+       checkSplit("/bin:/sbin",
+               "/bin",
+               "/sbin")
+
+       checkSplit("${DESTDIR}/bin:/bin/${SUBDIR}",
+               "${DESTDIR}/bin",
+               "/bin/${SUBDIR}")
+
+       checkSplit("/bin:${DESTDIR}${PREFIX}:${DESTDIR:S,/,\\:,:S,:,:,}/sbin",
+               "/bin",
+               "${DESTDIR}${PREFIX}",
+               "${DESTDIR:S,/,\\:,:S,:,:,}/sbin")
+}
+
 func (s *Suite) Test_MatchVarassign(c *check.C) {
        s.Init(c)
 
diff -r 5f04264688fe -r 17736e37f2e5 pkgtools/pkglint/files/mklines.go
--- a/pkgtools/pkglint/files/mklines.go Sat Jul 28 18:31:33 2018 +0000
+++ b/pkgtools/pkglint/files/mklines.go Sat Jul 28 20:44:45 2018 +0000
@@ -312,7 +312,7 @@
                return true
        }
        scope.OnIgnore = func(old, new MkLine) {
-               if isRelevant(old, new) {
+               if isRelevant(old, new) && old.Value() == new.Value() {
                        old.Notef("Definition of %s is redundant because of %s.", new.Varname(), new.ReferenceFrom(old.Line))
                }
        }
diff -r 5f04264688fe -r 17736e37f2e5 pkgtools/pkglint/files/mklines_test.go
--- a/pkgtools/pkglint/files/mklines_test.go    Sat Jul 28 18:31:33 2018 +0000
+++ b/pkgtools/pkglint/files/mklines_test.go    Sat Jul 28 20:44:45 2018 +0000
@@ -588,6 +588,17 @@
                "WARN: module.mk:1: Variable VAR is overwritten in line 3.")
 }
 
+func (s *Suite) Test_MkLines_CheckRedundantVariables__different_value(c *check.C) {
+       t := s.Init(c)
+       mklines := t.NewMkLines("module.mk",
+               "VAR=\tvalue ${OTHER}",
+               "VAR?=\tdifferent value")
+
+       mklines.CheckRedundantVariables()
+
+       t.CheckOutputEmpty()
+}
+
 func (s *Suite) Test_MkLines_CheckRedundantVariables__procedure_call(c *check.C) {
        t := s.Init(c)
        mklines := t.NewMkLines("mk/pthread.buildlink3.mk",
diff -r 5f04264688fe -r 17736e37f2e5 pkgtools/pkglint/files/vartypecheck.go
--- a/pkgtools/pkglint/files/vartypecheck.go    Sat Jul 28 18:31:33 2018 +0000
+++ b/pkgtools/pkglint/files/vartypecheck.go    Sat Jul 28 20:44:45 2018 +0000
@@ -669,19 +669,22 @@
        line.Errorf("Invalid option name %q. Option names must start with a lowercase letter and be all-lowercase.", value)
 }
 
-// The PATH environment variable
+// Pathlist checks variables like the PATH environment variable.
 func (cv *VartypeCheck) Pathlist() {
+       // Sometimes, variables called PATH contain a single pathname,
+       // especially those with auto-guessed type from MkLineImpl.VariableType.
        if !contains(cv.Value, ":") && cv.Guessed {
                MkLineChecker{cv.MkLine}.CheckVartypePrimitive(cv.Varname, BtPathname, cv.Op, cv.Value, cv.MkComment, cv.Guessed)
                return
        }
 
-       for _, path := range strings.Split(cv.Value, ":") {
-               if contains(path, "${") {
+       for _, path := range cv.MkLine.ValueSplit(cv.Value, ":") {
+               if hasPrefix(path, "${") {
                        continue
                }
 
-               if !matches(path, `^[-0-9A-Za-z._~+%/]*$`) {
+               pathNoVar := cv.MkLine.WithoutMakeVariables(path)
+               if !matches(pathNoVar, `^[-0-9A-Za-z._~+%/]*$`) {
                        cv.Line.Warnf("%q is not a valid pathname.", path)
                }
 
diff -r 5f04264688fe -r 17736e37f2e5 pkgtools/pkglint/files/vartypecheck_test.go
--- a/pkgtools/pkglint/files/vartypecheck_test.go       Sat Jul 28 18:31:33 2018 +0000
+++ b/pkgtools/pkglint/files/vartypecheck_test.go       Sat Jul 28 20:44:45 2018 +0000
@@ -394,10 +394,11 @@
        t := s.Init(c)
 
        runVartypeChecks(t, "PATH", opAssign, (*VartypeCheck).Pathlist,
-               "/usr/bin:/usr/sbin:.:${LOCALBASE}/bin")
+               "/usr/bin:/usr/sbin:.::${LOCALBASE}/bin:${HOMEPAGE:S,https://,,}";)
 
        t.CheckOutputLines(
-               "WARN: fname:1: All components of PATH (in this case \".\") should be absolute paths.")
+               "WARN: fname:1: All components of PATH (in this case \".\") should be absolute paths.",
+               "WARN: fname:1: All components of PATH (in this case \"\") should be absolute paths.")
 }
 
 func (s *Suite) Test_VartypeCheck_Perms(c *check.C) {