pkgsrc-Changes archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
CVS commit: pkgsrc/pkgtools/pkglint
Module Name: pkgsrc
Committed By: rillig
Date: Mon Dec 9 20:38:16 UTC 2019
Modified Files:
pkgsrc/pkgtools/pkglint: Makefile
pkgsrc/pkgtools/pkglint/files: logging.go logging_test.go mkline.go
mkline_test.go mklineparser.go mklineparser_test.go mklines_test.go
mkvarusechecker.go mkvarusechecker_test.go package_test.go
path_test.go pkglint.1 pkglint_test.go plist.go shell.go
shell_test.go util.go util_test.go vartypecheck.go
vartypecheck_test.go
Log Message:
pkgtools/pkglint: update to 19.3.16
Changes since 19.3.15:
When a package-settable variable gets a default value using the ?=
operator, pkglint no longer suggests to include bsd.prefs.mk, since that
doesn't make sense. Including bsd.prefs.mk only defines user-settable
and system-provided variables.
User and group names may be a single character only. While not widely
used, it's syntactically valid and there's no reason to prevent this.
In variable assignments, when pkglint removes unnecessary whitespace
between the variable name and the operator, it keeps the indentation of
the variable value the same as before. Previously, the indentation had
been changed, which required another run of pkglint --autofix.
PREFIX can only be used as a replacement for LOCALBASE after the whole
package Makefile has been loaded. This is because PREFIX is defined
very late, by bsd.pkg.mk. Therefore, don't suggest to replace LOCALBASE
with PREFIX in .if conditions.
When pkglint suggests to replace INSTALL_DATA_DIR commands with setting
INSTALLATION_DIRS instead, paths with a trailing slash are correctly
looked up in the PLIST. This suggests to use AUTO_MKDIRS more often.
To generate a diff of this commit:
cvs rdiff -u -r1.615 -r1.616 pkgsrc/pkgtools/pkglint/Makefile
cvs rdiff -u -r1.34 -r1.35 pkgsrc/pkgtools/pkglint/files/logging.go
cvs rdiff -u -r1.24 -r1.25 pkgsrc/pkgtools/pkglint/files/logging_test.go
cvs rdiff -u -r1.69 -r1.70 pkgsrc/pkgtools/pkglint/files/mkline.go
cvs rdiff -u -r1.75 -r1.76 pkgsrc/pkgtools/pkglint/files/mkline_test.go
cvs rdiff -u -r1.7 -r1.8 pkgsrc/pkgtools/pkglint/files/mklineparser.go \
pkgsrc/pkgtools/pkglint/files/mklineparser_test.go
cvs rdiff -u -r1.55 -r1.56 pkgsrc/pkgtools/pkglint/files/mklines_test.go \
pkgsrc/pkgtools/pkglint/files/pkglint_test.go
cvs rdiff -u -r1.2 -r1.3 pkgsrc/pkgtools/pkglint/files/mkvarusechecker.go \
pkgsrc/pkgtools/pkglint/files/mkvarusechecker_test.go
cvs rdiff -u -r1.63 -r1.64 pkgsrc/pkgtools/pkglint/files/package_test.go
cvs rdiff -u -r1.6 -r1.7 pkgsrc/pkgtools/pkglint/files/path_test.go
cvs rdiff -u -r1.61 -r1.62 pkgsrc/pkgtools/pkglint/files/pkglint.1 \
pkgsrc/pkgtools/pkglint/files/shell_test.go
cvs rdiff -u -r1.48 -r1.49 pkgsrc/pkgtools/pkglint/files/plist.go
cvs rdiff -u -r1.53 -r1.54 pkgsrc/pkgtools/pkglint/files/shell.go
cvs rdiff -u -r1.64 -r1.65 pkgsrc/pkgtools/pkglint/files/util.go \
pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go
cvs rdiff -u -r1.41 -r1.42 pkgsrc/pkgtools/pkglint/files/util_test.go
cvs rdiff -u -r1.71 -r1.72 pkgsrc/pkgtools/pkglint/files/vartypecheck.go
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: pkgsrc/pkgtools/pkglint/Makefile
diff -u pkgsrc/pkgtools/pkglint/Makefile:1.615 pkgsrc/pkgtools/pkglint/Makefile:1.616
--- pkgsrc/pkgtools/pkglint/Makefile:1.615 Sun Dec 8 22:03:37 2019
+++ pkgsrc/pkgtools/pkglint/Makefile Mon Dec 9 20:38:15 2019
@@ -1,6 +1,6 @@
-# $NetBSD: Makefile,v 1.615 2019/12/08 22:03:37 rillig Exp $
+# $NetBSD: Makefile,v 1.616 2019/12/09 20:38:15 rillig Exp $
-PKGNAME= pkglint-19.3.15
+PKGNAME= pkglint-19.3.16
CATEGORIES= pkgtools
DISTNAME= tools
MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/}
Index: pkgsrc/pkgtools/pkglint/files/logging.go
diff -u pkgsrc/pkgtools/pkglint/files/logging.go:1.34 pkgsrc/pkgtools/pkglint/files/logging.go:1.35
--- pkgsrc/pkgtools/pkglint/files/logging.go:1.34 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/logging.go Mon Dec 9 20:38:16 2019
@@ -257,10 +257,14 @@ func (l *Logger) Logf(level *LogLevel, f
if G.Testing {
if level != Error {
- assertf(!contains(format, "must"), "Must must only appear in errors: %s", format)
+ assertf(
+ !contains(format, "must"),
+ "The word \"must\" must only appear in errors: %s", format)
}
if level != Warn && level != Note {
- assertf(!contains(format, "should"), "Should must only appear in warnings: %s", format)
+ assertf(
+ !contains(format, "should"),
+ "The word \"should\" must only appear in warnings: %s", format)
}
}
Index: pkgsrc/pkgtools/pkglint/files/logging_test.go
diff -u pkgsrc/pkgtools/pkglint/files/logging_test.go:1.24 pkgsrc/pkgtools/pkglint/files/logging_test.go:1.25
--- pkgsrc/pkgtools/pkglint/files/logging_test.go:1.24 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/logging_test.go Mon Dec 9 20:38:16 2019
@@ -805,6 +805,23 @@ func (s *Suite) Test_Logger_Logf__panic(
"Pkglint internal error: Diagnostic format \"No period\" must end in a period.")
}
+func (s *Suite) Test_Logger_Logf__wording(c *check.C) {
+ t := s.Init(c)
+
+ t.ExpectPanic(
+ func() { G.Logger.Logf(Error, "filename", "13", "This should.", "This should.") },
+ "Pkglint internal error: The word \"should\" must only appear in warnings: This should.")
+
+ t.ExpectPanic(
+ func() { G.Logger.Logf(Warn, "filename", "13", "This must.", "This must.") },
+ "Pkglint internal error: The word \"must\" must only appear in errors: This must.")
+
+ G.Logger.Logf(Note, "filename", "13", "This should.", "This should.")
+
+ t.CheckOutputLines(
+ "NOTE: filename:13: This should.")
+}
+
func (s *Suite) Test_Logger_TechErrorf__gcc_format(c *check.C) {
t := s.Init(c)
Index: pkgsrc/pkgtools/pkglint/files/mkline.go
diff -u pkgsrc/pkgtools/pkglint/files/mkline.go:1.69 pkgsrc/pkgtools/pkglint/files/mkline.go:1.70
--- pkgsrc/pkgtools/pkglint/files/mkline.go:1.69 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mkline.go Mon Dec 9 20:38:16 2019
@@ -1277,29 +1277,46 @@ var (
)
func MatchMkInclude(text string) (m bool, indentation, directive string, filename RelPath) {
- lexer := textproc.NewLexer(text)
- if lexer.SkipByte('.') {
- indentation = lexer.NextHspace()
- directive = lexer.NextString("include")
- if directive == "" {
- directive = lexer.NextString("sinclude")
- }
- if directive != "" {
- lexer.NextHspace()
- if lexer.SkipByte('"') {
- // Note: strictly speaking, the full MkVarUse would have to be parsed
- // here. But since these usually don't contain double quotes, it has
- // worked fine up to now.
- filename = NewRelPathString(lexer.NextBytesFunc(func(c byte) bool { return c != '"' }))
- if !filename.IsEmpty() && lexer.SkipByte('"') {
- lexer.NextHspace()
- if lexer.EOF() {
- m = true
- return
- }
- }
- }
- }
+ tokens, rest := NewMkLexer(text, nil).MkTokens()
+ if rest != "" {
+ return false, "", "", ""
}
- return false, "", "", ""
+
+ lexer := NewMkTokensLexer(tokens)
+ if !lexer.SkipByte('.') {
+ return false, "", "", ""
+ }
+
+ indentation = lexer.NextHspace()
+
+ directive = lexer.NextString("include")
+ if directive == "" {
+ directive = lexer.NextString("sinclude")
+ }
+ if directive == "" {
+ return false, "", "", ""
+ }
+
+ lexer.SkipHspace()
+ if !lexer.SkipByte('"') {
+ return false, "", "", ""
+ }
+
+ mark := lexer.Mark()
+ for lexer.NextBytesFunc(func(c byte) bool { return c != '"' && c != '$' }) != "" ||
+ lexer.NextVarUse() != nil {
+ }
+ enclosed := NewPath(lexer.Since(mark))
+
+ if enclosed.IsEmpty() || enclosed.IsAbs() || !lexer.SkipByte('"') {
+ return false, "", "", ""
+ }
+ lexer.SkipHspace()
+ if !lexer.EOF() {
+ return false, "", "", ""
+ }
+
+ filename = NewRelPath(enclosed)
+ m = true
+ return
}
Index: pkgsrc/pkgtools/pkglint/files/mkline_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mkline_test.go:1.75 pkgsrc/pkgtools/pkglint/files/mkline_test.go:1.76
--- pkgsrc/pkgtools/pkglint/files/mkline_test.go:1.75 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mkline_test.go Mon Dec 9 20:38:16 2019
@@ -1547,13 +1547,6 @@ func (s *Suite) Test_MatchMkInclude(c *c
}
}
- testFail("")
- testFail(".")
- testFail(".include")
- testFail(".include \"")
- testFail(".include \"other.mk")
- testFail(".include \"other.mk\" \"")
-
test(".include \"other.mk\"",
"", "include", "other.mk", "")
@@ -1566,5 +1559,25 @@ func (s *Suite) Test_MatchMkInclude(c *c
test(".include \"other.mk\"\t# comment",
"", "include", "other.mk", "# comment")
- t.CheckOutputEmpty()
+ test(". include \"${DIR}/file.mk\"",
+ " ", "include", "${DIR}/file.mk", "")
+
+ test(". include \"${DIR:S,\",',g}/file.mk\"",
+ " ", "include", "${DIR:S,\",',g}/file.mk", "")
+
+ test(".include \"${}\"",
+ "", "include", "${}", "")
+
+ testFail("")
+ testFail(".")
+ testFail(".include")
+ testFail(".include \"")
+ testFail(".include \"\"")
+ testFail(".include \"$\"")
+ testFail(".include \"$$\"")
+ testFail(".include \"other.mk")
+ testFail(".include \"other.mk\" \"")
+ testFail(".include \"/absolute\"")
+ testFail(".include \"/absolute\"rest")
+ testFail(".include \"/absolute\" rest")
}
Index: pkgsrc/pkgtools/pkglint/files/mklineparser.go
diff -u pkgsrc/pkgtools/pkglint/files/mklineparser.go:1.7 pkgsrc/pkgtools/pkglint/files/mklineparser.go:1.8
--- pkgsrc/pkgtools/pkglint/files/mklineparser.go:1.7 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mklineparser.go Mon Dec 9 20:38:16 2019
@@ -72,33 +72,8 @@ func (p MkLineParser) parseVarassign(lin
return nil
}
- if a.spaceAfterVarname != "" {
- varname := a.varname
- op := a.op
- switch {
- case hasSuffix(varname, "+") && (op == opAssign || op == opAssignAppend):
- break
- case matches(varname, `^[a-z]`) && op == opAssignEval:
- break
- default:
- fix := line.Autofix()
- fix.Notef("Unnecessary space after variable name %q.", varname)
- fix.Replace(varname+a.spaceAfterVarname+op.String(), varname+op.String())
- fix.Apply()
- // FIXME: Preserve the alignment of the variable value.
- }
- }
-
- if splitResult.hasComment && a.value != "" && splitResult.spaceBeforeComment == "" {
- line.Warnf("The # character starts a Makefile comment.")
- line.Explain(
- "In a variable assignment, an unescaped # starts a comment that",
- "continues until the end of the line.",
- "To escape the #, write \\#.",
- "",
- "If this # character intentionally starts a comment,",
- "it should be preceded by a space in order to make it more visible.")
- }
+ p.fixSpaceAfterVarname(line, a)
+ p.checkUnintendedComment(&splitResult, a, line)
return &MkLine{line, splitResult, a}
}
@@ -181,6 +156,45 @@ func (p MkLineParser) MatchVarassign(lin
}
}
+func (p MkLineParser) fixSpaceAfterVarname(line *Line, a *mkLineAssign) {
+ if !(a.spaceAfterVarname != "") {
+ return
+ }
+
+ varname := a.varname
+ op := a.op
+ switch {
+ case hasSuffix(varname, "+") && (op == opAssign || op == opAssignAppend):
+ break
+ case matches(varname, `^[a-z]`) && op == opAssignEval:
+ break
+
+ default:
+ before := a.valueAlign
+ after := alignWith(varname+op.String(), before)
+
+ fix := line.Autofix()
+ fix.Notef("Unnecessary space after variable name %q.", varname)
+ fix.Replace(before, after)
+ fix.Apply()
+ }
+}
+
+func (p MkLineParser) checkUnintendedComment(splitResult *mkLineSplitResult, a *mkLineAssign, line *Line) {
+ if !(splitResult.hasComment && a.value != "" && splitResult.spaceBeforeComment == "") {
+ return
+ }
+
+ line.Warnf("The # character starts a Makefile comment.")
+ line.Explain(
+ "In a variable assignment, an unescaped # starts a comment that",
+ "continues until the end of the line.",
+ "To escape the #, write \\#.",
+ "",
+ "If this # character intentionally starts a comment,",
+ "it should be preceded by a space in order to make it more visible.")
+}
+
func (p MkLineParser) parseShellcmd(line *Line, splitResult mkLineSplitResult) *MkLine {
return &MkLine{line, splitResult, mkLineShell{line.Text[1:]}}
}
Index: pkgsrc/pkgtools/pkglint/files/mklineparser_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mklineparser_test.go:1.7 pkgsrc/pkgtools/pkglint/files/mklineparser_test.go:1.8
--- pkgsrc/pkgtools/pkglint/files/mklineparser_test.go:1.7 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mklineparser_test.go Mon Dec 9 20:38:16 2019
@@ -146,58 +146,6 @@ func (s *Suite) Test_MkLineParser_parseV
"WARN: rubyversion.mk:427: Makefile lines should not start with space characters.")
}
-func (s *Suite) Test_MkLineParser_parseVarassign__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_MkLineParser_parseVarassign__autofix_space_after_varname(c *check.C) {
- t := s.Init(c)
-
- filename := t.CreateFileLines("Makefile",
- MkCvsID,
- "VARNAME +=\t${VARNAME}",
- "VARNAME+ =\t${VARNAME+}",
- "VARNAME+ +=\t${VARNAME+}",
- "VARNAME+ ?=\t${VARNAME}",
- "pkgbase := pkglint")
-
- CheckFileMk(filename)
-
- t.CheckOutputLines(
- "NOTE: ~/Makefile:2: Unnecessary space after variable name \"VARNAME\".",
-
- // The assignment operators other than = and += cannot lead to ambiguities.
- "NOTE: ~/Makefile:5: Unnecessary space after variable name \"VARNAME+\".",
-
- "WARN: ~/Makefile:5: "+
- "Please include \"../../mk/bsd.prefs.mk\" before using \"?=\".")
-
- t.SetUpCommandLine("-Wall", "--autofix")
-
- CheckFileMk(filename)
-
- t.CheckOutputLines(
- "AUTOFIX: ~/Makefile:2: Replacing \"VARNAME +=\" with \"VARNAME+=\".",
- "AUTOFIX: ~/Makefile:5: Replacing \"VARNAME+ ?=\" with \"VARNAME+?=\".")
- t.CheckFileLines("Makefile",
- MkCvsID+"",
- "VARNAME+=\t${VARNAME}",
- "VARNAME+ =\t${VARNAME+}",
- "VARNAME+ +=\t${VARNAME+}",
- "VARNAME+?=\t${VARNAME}",
- "pkgbase := pkglint")
-}
-
func (s *Suite) Test_MkLineParser_parseVarassign__append(c *check.C) {
t := s.Init(c)
@@ -458,6 +406,88 @@ func (s *Suite) Test_MkLineParser_MatchV
"")
}
+func (s *Suite) Test_MkLineParser_fixSpaceAfterVarname__show_autofix(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_MkLineParser_fixSpaceAfterVarname__autofix(c *check.C) {
+ t := s.Init(c)
+
+ filename := t.CreateFileLines("Makefile",
+ MkCvsID,
+ "VARNAME +=\t${VARNAME}",
+ "VARNAME+ =\t${VARNAME+}",
+ "VARNAME+ +=\t${VARNAME+}",
+ "VARNAME+ ?=\t${VARNAME}",
+ "pkgbase := pkglint")
+
+ CheckFileMk(filename)
+
+ t.CheckOutputLines(
+ "NOTE: ~/Makefile:2: Unnecessary space after variable name \"VARNAME\".",
+
+ // The assignment operators other than = and += cannot lead to ambiguities.
+ "NOTE: ~/Makefile:5: Unnecessary space after variable name \"VARNAME+\".",
+
+ "WARN: ~/Makefile:5: "+
+ "Please include \"../../mk/bsd.prefs.mk\" before using \"?=\".")
+
+ t.SetUpCommandLine("-Wall", "--autofix")
+
+ CheckFileMk(filename)
+
+ t.CheckOutputLines(
+ "AUTOFIX: ~/Makefile:2: Replacing \"VARNAME +=\\t\" with \"VARNAME+=\\t\".",
+ "AUTOFIX: ~/Makefile:5: Replacing \"VARNAME+ ?=\\t\" with \"VARNAME+?=\\t\".")
+ t.CheckFileLines("Makefile",
+ MkCvsID+"",
+ "VARNAME+=\t${VARNAME}",
+ "VARNAME+ =\t${VARNAME+}",
+ "VARNAME+ +=\t${VARNAME+}",
+ "VARNAME+?=\t${VARNAME}",
+ "pkgbase := pkglint")
+}
+
+func (s *Suite) Test_MkLineParser_fixSpaceAfterVarname__preserve_alignment(c *check.C) {
+ t := s.Init(c)
+
+ t.SetUpCommandLine("--show-autofix")
+
+ test := func(before, after string, diagnostics ...string) {
+
+ doTest := func(autofix bool) {
+ mkline := t.NewMkLine("filename.mk", 123, before)
+ t.CheckEquals(mkline.Text, condStr(autofix, after, before))
+ }
+
+ t.ExpectDiagnosticsAutofix(doTest, diagnostics...)
+ }
+
+ test(
+ "V += ${VARNAME}",
+ "V+=\t\t${VARNAME}",
+
+ "NOTE: filename.mk:123: Unnecessary space after variable name \"V\".",
+ "AUTOFIX: filename.mk:123: Replacing \"V += \" with \"V+=\\t\\t\".")
+
+ test(
+ "V += ${VARNAME}",
+ "V+=\t ${VARNAME}",
+
+ "NOTE: filename.mk:123: Unnecessary space after variable name \"V\".",
+ "AUTOFIX: filename.mk:123: Replacing \"V += \" with \"V+=\\t \".")
+}
+
func (s *Suite) Test_MkLineParser_parseShellcmd(c *check.C) {
t := s.Init(c)
Index: pkgsrc/pkgtools/pkglint/files/mklines_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mklines_test.go:1.55 pkgsrc/pkgtools/pkglint/files/mklines_test.go:1.56
--- pkgsrc/pkgtools/pkglint/files/mklines_test.go:1.55 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mklines_test.go Mon Dec 9 20:38:16 2019
@@ -743,6 +743,28 @@ func (s *Suite) Test_MkLines_ForEach__co
t.CheckEquals(seenUsesGettext, true)
}
+func (s *Suite) Test_MkLines_ForEachEnd(c *check.C) {
+ t := s.Init(c)
+
+ t.SetUpVartypes()
+
+ mklines := t.NewMkLines("filename.mk",
+ MkCvsID)
+
+ // Calls to MkLines.ForEach cannot nest since they modify fields
+ // in the MkLines type. As of December 2019 there is no separation
+ // between:
+ // - The MkLines as a collection of data
+ // - An iterator over the MkLines
+ // - The MkLinesChecker
+ t.ExpectAssert(func() {
+ mklines.ForEach(func(mkline *MkLine) {
+ mklines.ForEach(func(mkline *MkLine) {
+ })
+ })
+ })
+}
+
func (s *Suite) Test_MkLines_collectElse(c *check.C) {
t := s.Init(c)
Index: pkgsrc/pkgtools/pkglint/files/pkglint_test.go
diff -u pkgsrc/pkgtools/pkglint/files/pkglint_test.go:1.55 pkgsrc/pkgtools/pkglint/files/pkglint_test.go:1.56
--- pkgsrc/pkgtools/pkglint/files/pkglint_test.go:1.55 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/pkglint_test.go Mon Dec 9 20:38:16 2019
@@ -275,12 +275,18 @@ func (s *Suite) Test_Pkglint_Main__autof
// install https://github.com/rillig/gobco and adjust the following code:
//
// env \
-// PKGLINT_TESTDIR=C:/Users/rillig/git/pkgsrc \
-// PKGLINT_TESTCMDLINE="-r -Wall -Call -e" \
-// gobco -test.covermode=count \
-// -test.coverprofile=pkglint-pkgsrc.pprof \
-// -timeout=3600s -check.f '^Test_Pkglint__realistic' \
-// > pkglint-pkgsrc.out
+// PKGLINT_TESTDIR="C:/Users/rillig/git/pkgsrc" \
+// PKGLINT_TESTCMDLINE="-r -Wall -Call -p -s -e" \
+// gobco \
+// -test=-test.covermode=count
+// -test=-test.coverprofile="C:/Users/rillig/go/src/netbsd.org/pkglint/stats-go.txt"
+// -test=-timeout=3600s \
+// -test=-check.f="^Test_Pkglint_Main__realistic" \
+// -stats="stats-gobco.json" \
+// > out
+//
+// Note that the path to -test.coverprofile must be absolute, since gobco
+// runs "go test" in a temporary directory.
//
// See https://github.com/rillig/gobco for the tool to measure the branch coverage.
func (s *Suite) Test_Pkglint_Main__realistic(c *check.C) {
Index: pkgsrc/pkgtools/pkglint/files/mkvarusechecker.go
diff -u pkgsrc/pkgtools/pkglint/files/mkvarusechecker.go:1.2 pkgsrc/pkgtools/pkglint/files/mkvarusechecker.go:1.3
--- pkgsrc/pkgtools/pkglint/files/mkvarusechecker.go:1.2 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mkvarusechecker.go Mon Dec 9 20:38:16 2019
@@ -25,7 +25,7 @@ func (ck *MkVarUseChecker) Check(vuc *Va
ck.checkUndefined()
ck.checkPermissions(vuc)
- ck.checkVarname()
+ ck.checkVarname(vuc.time)
ck.checkModifiers()
ck.checkQuoting(vuc)
@@ -130,7 +130,7 @@ func (ck *MkVarUseChecker) checkModifier
fix.Apply()
}
-func (ck *MkVarUseChecker) checkVarname() {
+func (ck *MkVarUseChecker) checkVarname(time VucTime) {
varname := ck.use.varname
if varname == "@" {
ck.MkLine.Warnf("Please use %q instead of %q.", "${.TARGET}", "$@")
@@ -139,7 +139,7 @@ func (ck *MkVarUseChecker) checkVarname(
"of the same name.")
}
- if varname == "LOCALBASE" && !G.Infrastructure {
+ if varname == "LOCALBASE" && !G.Infrastructure && time == VucRunTime {
fix := ck.MkLine.Autofix()
fix.Warnf("Please use PREFIX instead of LOCALBASE.")
fix.ReplaceRegex(`\$\{LOCALBASE\b`, "${PREFIX", 1)
@@ -380,12 +380,10 @@ func (ck *MkVarUseChecker) checkUseAtLoa
return
}
- if ck.vartype.IsPackageSettable() &&
- basename != "Makefile" && basename != "options.mk" {
-
- // For package-settable variables, the explanation doesn't
- // make sense since it talks about completely different
- // types of variables.
+ if ck.vartype.IsPackageSettable() {
+ // For package-settable variables, the explanation below
+ // doesn't make sense since including bsd.prefs.mk won't
+ // define any package-settable variables.
return
}
Index: pkgsrc/pkgtools/pkglint/files/mkvarusechecker_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mkvarusechecker_test.go:1.2 pkgsrc/pkgtools/pkglint/files/mkvarusechecker_test.go:1.3
--- pkgsrc/pkgtools/pkglint/files/mkvarusechecker_test.go:1.2 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mkvarusechecker_test.go Mon Dec 9 20:38:16 2019
@@ -355,7 +355,24 @@ func (s *Suite) Test_MkVarUseChecker_che
}
func (s *Suite) Test_MkVarUseChecker_checkVarname(c *check.C) {
- // FIXME
+ t := s.Init(c)
+
+ mklines := t.NewMkLines("filename.mk",
+ "\t: $@",
+ ".if ${LOCALBASE} == /usr/pkg", // Use at load time
+ "\t: ${LOCALBASE}", // Use at run time
+ ".endif")
+
+ mklines.ForEach(func(mkline *MkLine) {
+ mkline.ForEachUsed(func(varUse *MkVarUse, time VucTime) {
+ ck := NewMkVarUseChecker(varUse, mklines, mkline)
+ ck.checkVarname(time)
+ })
+ })
+
+ t.CheckOutputLines(
+ "WARN: filename.mk:1: Please use \"${.TARGET}\" instead of \"$@\".",
+ "WARN: filename.mk:3: Please use PREFIX instead of LOCALBASE.")
}
func (s *Suite) Test_MkVarUseChecker_checkPermissions(c *check.C) {
@@ -430,33 +447,6 @@ func (s *Suite) Test_MkVarUseChecker_che
"\ttech-pkg%NetBSD.org@localhost mailing list.", "")
}
-func (s *Suite) Test_MkVarUseChecker_checkPermissions__load_time(c *check.C) {
- t := s.Init(c)
-
- t.SetUpPkgsrc()
- t.Chdir("category/package")
- t.FinishSetUp()
- mklines := t.NewMkLines("options.mk",
- MkCvsID,
- "",
- "# don't include bsd.prefs.mk here",
- "",
- "WRKSRC:=\t${.CURDIR}",
- ".if ${PKG_SYSCONFDIR.gdm} != \"etc\"",
- ".endif")
-
- mklines.Check()
-
- // The PKG_SYSCONFDIR.* depend on the directory layout that is
- // specified in mk.conf, therefore bsd.prefs.mk must be included first.
- //
- // Evaluating .CURDIR at load time is definitely ok since it is defined
- // internally by bmake to be AlwaysInScope.
- t.CheckOutputLines(
- "WARN: options.mk:6: To use PKG_SYSCONFDIR.gdm at load time, " +
- ".include \"../../mk/bsd.prefs.mk\" first.")
-}
-
func (s *Suite) Test_MkVarUseChecker_checkPermissions__load_time_in_condition(c *check.C) {
t := s.Init(c)
@@ -860,6 +850,52 @@ func (s *Suite) Test_MkVarUseChecker_che
".include \"../../mk/bsd.prefs.mk\" first.")
}
+func (s *Suite) Test_MkVarUseChecker_checkUseAtLoadTime__PKG_SYSCONFDIR(c *check.C) {
+ t := s.Init(c)
+
+ t.SetUpVartypes()
+ t.Chdir("category/package")
+ mklines := t.NewMkLines("options.mk",
+ MkCvsID,
+ ".if ${PKG_SYSCONFDIR.gdm} != \"etc\"",
+ ".endif")
+
+ mklines.Check()
+
+ // The PKG_SYSCONFDIR.* directories typically start with ${PREFIX}.
+ // Since PREFIX is not defined until bsd.pkg.mk, including bsd.prefs.mk
+ // wouldn't help, therefore pkglint doesn't suggest it.
+ t.CheckOutputEmpty()
+}
+
+func (s *Suite) Test_MkVarUseChecker_checkUseAtLoadTime__package_settable(c *check.C) {
+ t := s.Init(c)
+
+ btAnything := &BasicType{"Anything", func(cv *VartypeCheck) {}}
+ t.SetUpType("PKG", btAnything, PackageSettable)
+ t.Chdir("category/package")
+
+ test := func(filename CurrPath, diagnostics ...string) {
+ mklines := t.NewMkLines(filename,
+ MkCvsID,
+ ".if ${PKG} != \"etc\"",
+ ".endif")
+
+ mklines.Check()
+
+ t.CheckOutput(diagnostics)
+ }
+
+ test("Makefile",
+ nil...)
+
+ test("options.mk",
+ nil...)
+
+ test("other.mk",
+ nil...)
+}
+
func (s *Suite) Test_MkVarUseChecker_warnToolLoadTime(c *check.C) {
t := s.Init(c)
Index: pkgsrc/pkgtools/pkglint/files/package_test.go
diff -u pkgsrc/pkgtools/pkglint/files/package_test.go:1.63 pkgsrc/pkgtools/pkglint/files/package_test.go:1.64
--- pkgsrc/pkgtools/pkglint/files/package_test.go:1.63 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/package_test.go Mon Dec 9 20:38:16 2019
@@ -1471,13 +1471,25 @@ func (s *Suite) Test_Package_checkfilePa
t.CheckOutputEmpty()
}
+// When the lines of the package Makefile are checked, it is necessary
+// to know whether bsd.prefs.mk has already been included.
+//
+// When the files are loaded recursively, Package.seenPrefs is set to
+// true as soon as some file includes bsd.prefs.mk. After that, when
+// loading reaches the main package Makefile again, Package.prefsLine
+// is set to the line that had just been included.
+//
+// In this test case, the preferences are loaded indirectly by line 22,
+// which includes common.mk, and that in turn includes bsd.prefs.mk.
func (s *Suite) Test_Package_checkfilePackageMakefile__prefs_indirect(c *check.C) {
t := s.Init(c)
- t.SetUpOption("option", "An example option")
+ // FIXME: remove t.SetUpOption("option", "An example option")
t.SetUpPackage("category/package",
- ".include \"common.mk\"",
- ".if ${OPSYS} == NetBSD",
+ ".if ${OPSYS} == NetBSD", // 20: OPSYS is not yet defined here.
+ ".endif", // 21
+ ".include \"common.mk\"", // 22: OPSYS gets defined.
+ ".if ${OPSYS} == NetBSD", // 23: Now OPSYS is defined.
".endif")
t.CreateFileLines("category/package/common.mk",
MkCvsID,
@@ -1491,16 +1503,19 @@ func (s *Suite) Test_Package_checkfilePa
files, mklines, allLines := G.Pkg.load()
t.CheckEquals(G.Pkg.seenPrefs, false)
- t.CheckEquals(G.Pkg.prefsLine, mklines.mklines[19])
+ t.CheckEquals(G.Pkg.prefsLine, mklines.mklines[21])
G.Pkg.check(files, mklines, allLines)
t.CheckEquals(G.Pkg.seenPrefs, true)
- t.CheckEquals(G.Pkg.prefsLine, mklines.mklines[19])
+ t.CheckEquals(G.Pkg.prefsLine, mklines.mklines[21])
// Since bsd.prefs.mk is included indirectly by common.mk,
- // OPSYS may be used at load time in line 21.
- t.CheckOutputEmpty()
+ // OPSYS may be used at load time in line 23, but not in line 20.
+ t.CheckOutputLines(
+ "WARN: ~/category/package/Makefile:20: " +
+ "To use OPSYS at load time, " +
+ ".include \"../../mk/bsd.prefs.mk\" first.")
}
func (s *Suite) Test_Package_checkfilePackageMakefile__redundancy_in_infra(c *check.C) {
Index: pkgsrc/pkgtools/pkglint/files/path_test.go
diff -u pkgsrc/pkgtools/pkglint/files/path_test.go:1.6 pkgsrc/pkgtools/pkglint/files/path_test.go:1.7
--- pkgsrc/pkgtools/pkglint/files/path_test.go:1.6 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/path_test.go Mon Dec 9 20:38:16 2019
@@ -525,8 +525,10 @@ func (s *Suite) Test_Path_Rel(c *check.C
test("a/../b", "c/../d", "../d")
test(".", "dir/file", "dir/file")
- test(".", "dir/subdir/", "dir/subdir") // FIXME: missing /. at the end
- test(".", "dir/subdir/.", "dir/subdir") // FIXME: missing /. at the end
+ // XXX: maybe the /. is missing at the end
+ test(".", "dir/subdir/", "dir/subdir")
+ // XXX: maybe the /. is missing at the end
+ test(".", "dir/subdir/.", "dir/subdir")
}
func (s *Suite) Test_NewCurrPath(c *check.C) {
Index: pkgsrc/pkgtools/pkglint/files/pkglint.1
diff -u pkgsrc/pkgtools/pkglint/files/pkglint.1:1.61 pkgsrc/pkgtools/pkglint/files/pkglint.1:1.62
--- pkgsrc/pkgtools/pkglint/files/pkglint.1:1.61 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/pkglint.1 Mon Dec 9 20:38:16 2019
@@ -1,4 +1,4 @@
-.\" $NetBSD: pkglint.1,v 1.61 2019/12/08 22:03:38 rillig Exp $
+.\" $NetBSD: pkglint.1,v 1.62 2019/12/09 20:38:16 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@localhost>.
Index: pkgsrc/pkgtools/pkglint/files/shell_test.go
diff -u pkgsrc/pkgtools/pkglint/files/shell_test.go:1.61 pkgsrc/pkgtools/pkglint/files/shell_test.go:1.62
--- pkgsrc/pkgtools/pkglint/files/shell_test.go:1.61 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/shell_test.go Mon Dec 9 20:38:16 2019
@@ -44,6 +44,74 @@ func (s *Suite) Test_SimpleCommandChecke
"WARN: Makefile:8: UNKNOWN_TOOL is used but not defined.")
}
+func (s *Suite) Test_SimpleCommandChecker_checkInstallCommand(c *check.C) {
+ t := s.Init(c)
+
+ lines := func(lines ...string) []string { return lines }
+
+ test := func(lines []string, diagnostics ...string) {
+ mklines := t.NewMkLines("filename.mk",
+ mapStr(lines, func(s string) string { return "\t" + s })...)
+ mklines.target = "do-install"
+
+ mklines.ForEach(func(mkline *MkLine) {
+ program, err := parseShellProgram(nil, mkline.ShellCommand())
+ assertNil(err, "")
+
+ walker := NewMkShWalker()
+ walker.Callback.SimpleCommand = func(command *MkShSimpleCommand) {
+ scc := NewSimpleCommandChecker(command, RunTime, mkline, mklines)
+ scc.checkInstallCommand(command.Name.MkText)
+ }
+ walker.Walk(program)
+ })
+
+ t.CheckOutput(diagnostics)
+ }
+
+ test(
+ lines(
+ "sed",
+ "${SED}"),
+ "WARN: filename.mk:1: The shell command \"sed\" "+
+ "should not be used in the install phase.",
+ "WARN: filename.mk:2: The shell command \"${SED}\" "+
+ "should not be used in the install phase.")
+
+ test(
+ lines(
+ "tr",
+ "${TR}"),
+ "WARN: filename.mk:1: The shell command \"tr\" "+
+ "should not be used in the install phase.",
+ "WARN: filename.mk:2: The shell command \"${TR}\" "+
+ "should not be used in the install phase.")
+
+ test(
+ lines(
+ "cp",
+ "${CP}"),
+ "WARN: filename.mk:1: ${CP} should not be used to install files.",
+ "WARN: filename.mk:2: ${CP} should not be used to install files.")
+
+ test(
+ lines(
+ "${INSTALL}",
+ "${INSTALL_DATA}",
+ "${INSTALL_DATA_DIR}",
+ "${INSTALL_LIB}",
+ "${INSTALL_LIB_DIR}",
+ "${INSTALL_MAN}",
+ "${INSTALL_MAN_DIR}",
+ "${INSTALL_PROGRAM}",
+ "${INSTALL_PROGRAM_DIR}",
+ "${INSTALL_SCRIPT}",
+ "${LIBTOOL}",
+ "${LN}",
+ "${PAX}"),
+ nil...)
+}
+
func (s *Suite) Test_SimpleCommandChecker_handleForbiddenCommand(c *check.C) {
t := s.Init(c)
@@ -155,10 +223,14 @@ func (s *Suite) Test_SimpleCommandChecke
func (s *Suite) Test_SimpleCommandChecker_handleShellBuiltin(c *check.C) {
t := s.Init(c)
+ mklines := t.NewMkLines("filename.mk",
+ "\t:")
+ mkline := mklines.mklines[0]
+
test := func(command string, isBuiltin bool) {
token := NewShToken(command, NewShAtom(shtText, command, shqPlain))
simpleCommand := &MkShSimpleCommand{Name: token}
- scc := NewSimpleCommandChecker(nil, simpleCommand, RunTime)
+ scc := NewSimpleCommandChecker(simpleCommand, RunTime, mkline, mklines)
t.CheckEquals(scc.handleShellBuiltin(), isBuiltin)
}
@@ -352,6 +424,83 @@ func (s *Suite) Test_SimpleCommandChecke
"instead of \"${INSTALL_DATA_DIR}\".")
}
+func (s *Suite) Test_SimpleCommandChecker_checkAutoMkdirs__strange_paths(c *check.C) {
+ t := s.Init(c)
+
+ test := func(path string, diagnostics ...string) {
+ mklines := t.NewMkLines("filename.mk",
+ "\t${INSTALL_DATA_DIR} "+path)
+ mklines.ForEach(func(mkline *MkLine) {
+ program, err := parseShellProgram(nil, mkline.ShellCommand())
+ assertNil(err, "")
+
+ walker := NewMkShWalker()
+ walker.Callback.SimpleCommand = func(command *MkShSimpleCommand) {
+ scc := NewSimpleCommandChecker(command, RunTime, mkline, mklines)
+ scc.checkAutoMkdirs()
+ }
+ walker.Walk(program)
+ })
+ t.CheckOutput(diagnostics)
+ }
+
+ t.Chdir("category/package")
+
+ test("${PREFIX}",
+ nil...)
+
+ test("${PREFIX}/",
+ nil...)
+
+ test("${PREFIX}//",
+ nil...)
+
+ test("${PREFIX}/.",
+ nil...)
+
+ test("${PREFIX}//non-canonical",
+ "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= non-canonical\" "+
+ "instead of \"${INSTALL_DATA_DIR}\".")
+
+ test("${PREFIX}/non-canonical/////",
+ "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= non-canonical\" "+
+ "instead of \"${INSTALL_DATA_DIR}\".")
+
+ test("${PREFIX}/${VAR}",
+ "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= ${VAR}\" "+
+ "instead of \"${INSTALL_DATA_DIR}\".")
+
+ test("${PREFIX}/${VAR.param}",
+ "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= ${VAR.param}\" "+
+ "instead of \"${INSTALL_DATA_DIR}\".")
+
+ test("${PREFIX}/${.CURDIR}",
+ "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= ${.CURDIR}\" "+
+ "instead of \"${INSTALL_DATA_DIR}\".")
+
+ // Internal variables are ok.
+ test("${PREFIX}/${_INTERNAL}",
+ "NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= ${_INTERNAL}\" "+
+ "instead of \"${INSTALL_DATA_DIR}\".")
+
+ // Ignore variables from a :@ modifier.
+ test("${PREFIX}/${.f.}",
+ nil...)
+
+ // Ignore variables from a .for loop.
+ test("${PREFIX}/${f}",
+ nil...)
+
+ // Ignore variables from a .for loop.
+ test("${PREFIX}/${_f_}",
+ nil...)
+
+ // Ignore paths containing shell variables as it is hard to
+ // predict their values using static analysis.
+ test("${PREFIX}/$$f",
+ nil...)
+}
+
// 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) {
@@ -1681,71 +1830,6 @@ func (s *Suite) Test_ShellLineChecker_va
"WARN: filename.mk:3: Unquoted shell variable \"target\".")
}
-func (s *Suite) Test_ShellLineChecker_checkInstallCommand(c *check.C) {
- t := s.Init(c)
-
- test := func(lines ...string) {
- var cmds []string
- i := 0
- for i < len(lines) && lines[i] != "" {
- cmds = append(cmds, "\t"+lines[i])
- i++
- }
- diagnostics := lines[i+1:]
-
- mklines := t.NewMkLines("filename.mk", cmds...)
- mklines.target = "do-install"
-
- mklines.ForEach(func(mkline *MkLine) {
- ck := NewShellLineChecker(mklines, mkline)
- ck.checkInstallCommand(mkline.ShellCommand())
- })
-
- t.CheckOutput(diagnostics)
- }
-
- test(
- "sed",
- "${SED}",
- "",
- "WARN: filename.mk:1: The shell command \"sed\" "+
- "should not be used in the install phase.",
- "WARN: filename.mk:2: The shell command \"${SED}\" "+
- "should not be used in the install phase.")
-
- test(
- "tr",
- "${TR}",
- "",
- "WARN: filename.mk:1: The shell command \"tr\" "+
- "should not be used in the install phase.",
- "WARN: filename.mk:2: The shell command \"${TR}\" "+
- "should not be used in the install phase.")
-
- test(
- "cp",
- "${CP}",
- "",
- "WARN: filename.mk:1: ${CP} should not be used to install files.",
- "WARN: filename.mk:2: ${CP} should not be used to install files.")
-
- test(
- "${INSTALL}",
- "${INSTALL_DATA}",
- "${INSTALL_DATA_DIR}",
- "${INSTALL_LIB}",
- "${INSTALL_LIB_DIR}",
- "${INSTALL_MAN}",
- "${INSTALL_MAN_DIR}",
- "${INSTALL_PROGRAM}",
- "${INSTALL_PROGRAM_DIR}",
- "${INSTALL_SCRIPT}",
- "${LIBTOOL}",
- "${LN}",
- "${PAX}",
- "")
-}
-
func (s *Suite) Test_ShellLineChecker_checkMultiLineComment(c *check.C) {
t := s.Init(c)
Index: pkgsrc/pkgtools/pkglint/files/plist.go
diff -u pkgsrc/pkgtools/pkglint/files/plist.go:1.48 pkgsrc/pkgtools/pkglint/files/plist.go:1.49
--- pkgsrc/pkgtools/pkglint/files/plist.go:1.48 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/plist.go Mon Dec 9 20:38:16 2019
@@ -578,8 +578,7 @@ func NewPlistLineSorter(plines []*PlistL
func (s *plistLineSorter) Sort() {
if line := s.unsortable; line != nil {
- if G.Logger.IsAutofix() {
- // FIXME: Missing trace.Enabled
+ if G.Logger.IsAutofix() && trace.Tracing {
trace.Stepf("%s: This line prevents pkglint from sorting the PLIST automatically.", line)
}
return
Index: pkgsrc/pkgtools/pkglint/files/shell.go
diff -u pkgsrc/pkgtools/pkglint/files/shell.go:1.53 pkgsrc/pkgtools/pkglint/files/shell.go:1.54
--- pkgsrc/pkgtools/pkglint/files/shell.go:1.53 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/shell.go Mon Dec 9 20:38:16 2019
@@ -9,15 +9,17 @@ import (
// Parsing and checking shell commands embedded in Makefiles
type SimpleCommandChecker struct {
- *ShellLineChecker
cmd *MkShSimpleCommand
strcmd *StrCommand
time ToolTime
+
+ mkline *MkLine
+ mklines *MkLines
}
-func NewSimpleCommandChecker(ck *ShellLineChecker, cmd *MkShSimpleCommand, time ToolTime) *SimpleCommandChecker {
+func NewSimpleCommandChecker(cmd *MkShSimpleCommand, time ToolTime, mkline *MkLine, mklines *MkLines) *SimpleCommandChecker {
strcmd := NewStrCommand(cmd)
- return &SimpleCommandChecker{ck, cmd, strcmd, time}
+ return &SimpleCommandChecker{cmd, strcmd, time, mkline, mklines}
}
@@ -58,7 +60,7 @@ func (scc *SimpleCommandChecker) checkCo
case matches(shellword, `\$\{(PKGSRCDIR|PREFIX)(:Q)?\}`):
break
default:
- if G.Opts.WarnExtra && !scc.MkLines.indentation.DependsOn("OPSYS") {
+ if G.Opts.WarnExtra && !scc.mklines.indentation.DependsOn("OPSYS") {
scc.mkline.Warnf("Unknown shell command %q.", shellword)
scc.mkline.Explain(
"To make the package portable to all platforms that pkgsrc supports,",
@@ -69,6 +71,51 @@ func (scc *SimpleCommandChecker) checkCo
}
}
+// Some shell commands should not be used in the install phase.
+func (scc *SimpleCommandChecker) checkInstallCommand(shellcmd string) {
+ if trace.Tracing {
+ defer trace.Call0()()
+ }
+
+ if !matches(scc.mklines.target, `^(?:pre|do|post)-install$`) {
+ return
+ }
+
+ line := scc.mkline.Line
+ switch shellcmd {
+ case "${INSTALL}",
+ "${INSTALL_DATA}", "${INSTALL_DATA_DIR}",
+ "${INSTALL_LIB}", "${INSTALL_LIB_DIR}",
+ "${INSTALL_MAN}", "${INSTALL_MAN_DIR}",
+ "${INSTALL_PROGRAM}", "${INSTALL_PROGRAM_DIR}",
+ "${INSTALL_SCRIPT}",
+ "${LIBTOOL}",
+ "${LN}",
+ "${PAX}":
+ return
+
+ case "sed", "${SED}",
+ "tr", "${TR}":
+ // TODO: Pkglint should not complain when sed and tr are used to transform filenames.
+ line.Warnf("The shell command %q should not be used in the install phase.", shellcmd)
+ line.Explain(
+ "In the install phase, the only thing that should be done is to",
+ "install the prepared files to their final location.",
+ "The file's contents should not be changed anymore.")
+
+ case "cp", "${CP}":
+ line.Warnf("${CP} should not be used to install files.")
+ line.Explain(
+ "The ${CP} command is highly platform dependent and cannot overwrite read-only files.",
+ "Please use ${PAX} instead.",
+ "",
+ "For example, instead of:",
+ "\t${CP} -R ${WRKSRC}/* ${PREFIX}/foodir",
+ "use:",
+ "\tcd ${WRKSRC} && ${PAX} -wr * ${PREFIX}/foodir")
+ }
+}
+
func (scc *SimpleCommandChecker) handleForbiddenCommand() bool {
if trace.Tracing {
defer trace.Call0()()
@@ -95,7 +142,7 @@ func (scc *SimpleCommandChecker) handleT
command := scc.strcmd.Name
- tool, usable := G.Tool(scc.MkLines, command, scc.time)
+ tool, usable := G.Tool(scc.mklines, command, scc.time)
if tool != nil && !usable {
scc.mkline.Warnf("The %q tool is used but not added to USE_TOOLS.", command)
@@ -121,14 +168,14 @@ func (scc *SimpleCommandChecker) handleC
varname := varuse.varname
- if vartype := G.Pkgsrc.VariableType(scc.MkLines, varname); vartype != nil && vartype.basicType.name == "ShellCommand" {
+ if vartype := G.Pkgsrc.VariableType(scc.mklines, varname); vartype != nil && vartype.basicType.name == "ShellCommand" {
scc.checkInstallCommand(shellword)
return true
}
// When the package author has explicitly defined a command
// variable, assume it to be valid.
- if scc.MkLines.allVars.IsDefinedSimilar(varname) {
+ if scc.mklines.allVars.IsDefinedSimilar(varname) {
return true
}
@@ -205,45 +252,64 @@ func (scc *SimpleCommandChecker) checkAu
return
}
+ containsIgnoredVar := func(arg string) bool {
+ for _, token := range scc.mkline.Tokenize(arg, false) {
+ if token.Varuse != nil && matches(token.Varuse.varname, `^[_.]*[a-z]`) {
+ return true
+ }
+ }
+ return false
+ }
+
for _, arg := range scc.strcmd.Args {
- // TODO: Replace regex with proper VarUse.
- if !contains(arg, "$$") && !matches(arg, `\$\{[_.]*[a-z]`) {
- if m, dirname := match1(arg, `^(?:\$\{DESTDIR\})?\$\{PREFIX(?:|:Q)\}/(.*)`); m {
- autoMkdirs := false
- if G.Pkg != nil {
- // FIXME: Add test for absolute path.
- plistLine := G.Pkg.Plist.Dirs[NewRelPathString(dirname)]
- if plistLine != nil && !containsVarRef(plistLine.Text) {
- autoMkdirs = true
- }
- }
+ if contains(arg, "$$") || containsIgnoredVar(arg) {
+ continue
+ }
- if autoMkdirs {
- scc.Notef("You can use AUTO_MKDIRS=yes or \"INSTALLATION_DIRS+= %s\" instead of %q.", dirname, cmdname)
- scc.Explain(
- "Many packages include a list of all needed directories in their",
- "PLIST file.",
- "In such a case, you can just set AUTO_MKDIRS=yes and be done.",
- "The pkgsrc infrastructure will then create all directories in advance.",
- "",
- "To create directories that are not mentioned in the PLIST file,",
- "it is easier to just list them in INSTALLATION_DIRS than to execute the",
- "commands explicitly.",
- "That way, you don't have to think about which",
- "of the many INSTALL_*_DIR variables is appropriate, since",
- "INSTALLATION_DIRS takes care of that.")
- } else {
- scc.Notef("You can use \"INSTALLATION_DIRS+= %s\" instead of %q.", dirname, cmdname)
- scc.Explain(
- "To create directories during installation, it is easier to just",
- "list them in INSTALLATION_DIRS than to execute the commands",
- "explicitly.",
- "That way, you don't have to think about which",
- "of the many INSTALL_*_DIR variables is appropriate,",
- "since INSTALLATION_DIRS takes care of that.")
- }
+ m, dirname := match1(arg, `^(?:\$\{DESTDIR\})?\$\{PREFIX(?:|:Q)\}/+([^/]\S*)$`)
+ if !m {
+ continue
+ }
+
+ prefixRel := NewRelPathString(dirname).Clean()
+ if prefixRel == "." {
+ continue
+ }
+
+ autoMkdirs := false
+ if G.Pkg != nil && prefixRel != "." {
+ plistLine := G.Pkg.Plist.Dirs[prefixRel]
+ if plistLine != nil && !containsVarRef(plistLine.Text) {
+ autoMkdirs = true
}
}
+
+ if autoMkdirs {
+ scc.Notef("You can use AUTO_MKDIRS=yes or \"INSTALLATION_DIRS+= %s\" instead of %q.",
+ prefixRel.String(), cmdname)
+ scc.Explain(
+ "Many packages include a list of all needed directories in their",
+ "PLIST file.",
+ "In such a case, you can just set AUTO_MKDIRS=yes and be done.",
+ "The pkgsrc infrastructure will then create all directories in advance.",
+ "",
+ "To create directories that are not mentioned in the PLIST file,",
+ "it is easier to just list them in INSTALLATION_DIRS than to execute the",
+ "commands explicitly.",
+ "That way, you don't have to think about which",
+ "of the many INSTALL_*_DIR variables is appropriate, since",
+ "INSTALLATION_DIRS takes care of that.")
+ } else {
+ scc.Notef("You can use \"INSTALLATION_DIRS+= %s\" instead of %q.",
+ prefixRel.String(), cmdname)
+ scc.Explain(
+ "To create directories during installation, it is easier to just",
+ "list them in INSTALLATION_DIRS than to execute the commands",
+ "explicitly.",
+ "That way, you don't have to think about which",
+ "of the many INSTALL_*_DIR variables is appropriate,",
+ "since INSTALLATION_DIRS takes care of that.")
+ }
}
}
@@ -283,7 +349,7 @@ func (scc *SimpleCommandChecker) checkPa
if (scc.strcmd.Name == "${PAX}" || scc.strcmd.Name == "pax") && scc.strcmd.HasOption("-pe") {
scc.Warnf("Please use the -pp option to pax(1) instead of -pe.")
- scc.mkline.Explain(
+ scc.Explain(
"The -pe option tells pax to preserve the ownership of the files.",
"",
"When extracting distfiles as root user, this means that whatever numeric uid was",
@@ -301,7 +367,7 @@ func (scc *SimpleCommandChecker) checkEc
}
if scc.strcmd.Name == "${ECHO}" && scc.strcmd.HasOption("-n") {
- scc.mkline.Warnf("Please use ${ECHO_N} instead of \"echo -n\".")
+ scc.Warnf("Please use ${ECHO_N} instead of \"echo -n\".")
}
}
@@ -671,7 +737,7 @@ func (ck *ShellLineChecker) CheckShellCo
walker := NewMkShWalker()
walker.Callback.SimpleCommand = func(command *MkShSimpleCommand) {
- scc := NewSimpleCommandChecker(ck, command, time)
+ scc := NewSimpleCommandChecker(command, time, ck.mkline, ck.MkLines)
scc.Check()
// TODO: Implement getopt parsing for StrCommand.
if scc.strcmd.Name == "set" && scc.strcmd.AnyArgMatches(`^-.*e`) {
@@ -953,51 +1019,6 @@ func (ck *ShellLineChecker) checkVaruseT
return true
}
-// Some shell commands should not be used in the install phase.
-func (ck *ShellLineChecker) checkInstallCommand(shellcmd string) {
- if trace.Tracing {
- defer trace.Call0()()
- }
-
- if !matches(ck.MkLines.target, `^(?:pre|do|post)-install$`) {
- return
- }
-
- line := ck.mkline.Line
- switch shellcmd {
- case "${INSTALL}",
- "${INSTALL_DATA}", "${INSTALL_DATA_DIR}",
- "${INSTALL_LIB}", "${INSTALL_LIB_DIR}",
- "${INSTALL_MAN}", "${INSTALL_MAN_DIR}",
- "${INSTALL_PROGRAM}", "${INSTALL_PROGRAM_DIR}",
- "${INSTALL_SCRIPT}",
- "${LIBTOOL}",
- "${LN}",
- "${PAX}":
- return
-
- case "sed", "${SED}",
- "tr", "${TR}":
- // TODO: Pkglint should not complain when sed and tr are used to transform filenames.
- line.Warnf("The shell command %q should not be used in the install phase.", shellcmd)
- line.Explain(
- "In the install phase, the only thing that should be done is to",
- "install the prepared files to their final location.",
- "The file's contents should not be changed anymore.")
-
- case "cp", "${CP}":
- line.Warnf("${CP} should not be used to install files.")
- line.Explain(
- "The ${CP} command is highly platform dependent and cannot overwrite read-only files.",
- "Please use ${PAX} instead.",
- "",
- "For example, instead of:",
- "\t${CP} -R ${WRKSRC}/* ${PREFIX}/foodir",
- "use:",
- "\tcd ${WRKSRC} && ${PAX} -wr * ${PREFIX}/foodir")
- }
-}
-
func (ck *ShellLineChecker) checkMultiLineComment() {
mkline := ck.mkline
if !mkline.IsMultiline() || !contains(mkline.Text, "#") {
Index: pkgsrc/pkgtools/pkglint/files/util.go
diff -u pkgsrc/pkgtools/pkglint/files/util.go:1.64 pkgsrc/pkgtools/pkglint/files/util.go:1.65
--- pkgsrc/pkgtools/pkglint/files/util.go:1.64 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/util.go Mon Dec 9 20:38:16 2019
@@ -88,6 +88,22 @@ func mapStr(slice []string, fn func(s st
return result
}
+func invalidCharacters(s string, valid *textproc.ByteSet) string {
+ var unis strings.Builder
+
+ for _, r := range s {
+ if r == rune(byte(r)) && valid.Contains(byte(r)) {
+ continue
+ }
+ _, _ = fmt.Fprintf(&unis, " %U", r)
+ }
+
+ if unis.Len() == 0 {
+ return ""
+ }
+ return unis.String()[1:]
+}
+
// intern returns an independent copy of the given string.
//
// It should be called when only a small substring of a large string
@@ -367,13 +383,19 @@ func detab(s string) string {
return detabbed.String()
}
-// alignWith extends str with as many tabs as needed to reach
+// alignWith extends str with as many tabs and spaces as needed to reach
// the same screen width as the other string.
func alignWith(str, other string) string {
- alignBefore := (tabWidth(other) + 7) & -8
- alignAfter := tabWidth(str) & -8
- tabsNeeded := imax((alignBefore-alignAfter)/8, 1)
- return str + strings.Repeat("\t", tabsNeeded)
+ strWidth := tabWidth(str)
+ otherWidth := tabWidth(other)
+ if otherWidth <= strWidth {
+ return str + "\t"
+ }
+ if strWidth&-8 != otherWidth&-8 {
+ strWidth &= -8
+ }
+ alignment := indent(otherWidth - strWidth)
+ return str + alignment
}
func indent(width int) string {
@@ -462,8 +484,6 @@ func containsVarRef(s string) bool {
return false
}
-func hasAlnumPrefix(s string) bool { return s != "" && textproc.AlnumU.Contains(s[0]) }
-
// Once remembers with which arguments its FirstTime method has been called
// and only returns true on each first call.
type Once struct {
Index: pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go
diff -u pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go:1.64 pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go:1.65
--- pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go:1.64 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go Mon Dec 9 20:38:16 2019
@@ -1875,11 +1875,20 @@ func (s *Suite) Test_VartypeCheck_UserGr
"typical_username",
"user123",
"domain\\user",
- "${OTHER_VAR}")
+ "${OTHER_VAR}",
+ "r",
+ "-rf",
+ "rf-")
vt.Output(
- "WARN: filename.mk:1: Invalid user or group name \"user with spaces\".",
- "WARN: filename.mk:4: Invalid user or group name \"domain\\\\user\".")
+ "WARN: filename.mk:1: User or group name \"user with spaces\" "+
+ "contains invalid characters: U+0020 U+0020",
+ "WARN: filename.mk:4: User or group name \"domain\\\\user\" "+
+ "contains invalid characters: U+005C",
+ "ERROR: filename.mk:7: User or group name \"-rf\" "+
+ "must not start with a hyphen.",
+ "ERROR: filename.mk:8: User or group name \"rf-\" "+
+ "must not end with a hyphen.")
}
func (s *Suite) Test_VartypeCheck_VariableName(c *check.C) {
@@ -2053,6 +2062,14 @@ func (s *Suite) Test_VartypeCheck_Yes(c
"WARN: filename.mk:11: BUILD_USES_MSGFMT should only be used in a \".if defined(...)\" condition.",
"WARN: filename.mk:12: BUILD_USES_MSGFMT should only be used in a \".if defined(...)\" condition.",
"WARN: filename.mk:13: BUILD_USES_MSGFMT should only be used in a \".if defined(...)\" condition.")
+
+ vt.Op(opAssign)
+ vt.Values(
+ // This was accidentally accepted until 2019-12-09.
+ "yes \\# this is not a comment")
+
+ vt.Output(
+ "WARN: filename.mk:21: BUILD_USES_MSGFMT should be set to YES or yes.")
}
func (s *Suite) Test_VartypeCheck_YesNo(c *check.C) {
@@ -2063,11 +2080,15 @@ func (s *Suite) Test_VartypeCheck_YesNo(
"yes",
"no",
"ja",
- "${YESVAR}")
+ "${YESVAR}",
+ "yes # comment",
+ "no # comment",
+ "Yes indeed")
vt.Output(
"WARN: filename.mk:3: PKG_DEVELOPER should be set to YES, yes, NO, or no.",
- "WARN: filename.mk:4: PKG_DEVELOPER should be set to YES, yes, NO, or no.")
+ "WARN: filename.mk:4: PKG_DEVELOPER should be set to YES, yes, NO, or no.",
+ "WARN: filename.mk:7: PKG_DEVELOPER should be set to YES, yes, NO, or no.")
vt.Op(opUseMatch)
vt.Values(
@@ -2086,6 +2107,14 @@ func (s *Suite) Test_VartypeCheck_YesNo(
"\"[yY][eE][sS]\" or \"[nN][oO]\", not \"[Yy]es\".",
"WARN: filename.mk:15: PKG_DEVELOPER should be matched against "+
"\"[yY][eE][sS]\" or \"[nN][oO]\", not \"[Nn]o\".")
+
+ vt.Op(opAssign)
+ vt.Values(
+ // This was accidentally accepted until 2019-12-09.
+ "yes \\# this is not a comment")
+
+ vt.Output(
+ "WARN: filename.mk:21: PKG_DEVELOPER should be set to YES, yes, NO, or no.")
}
func (s *Suite) Test_VartypeCheck_YesNoIndirectly(c *check.C) {
Index: pkgsrc/pkgtools/pkglint/files/util_test.go
diff -u pkgsrc/pkgtools/pkglint/files/util_test.go:1.41 pkgsrc/pkgtools/pkglint/files/util_test.go:1.42
--- pkgsrc/pkgtools/pkglint/files/util_test.go:1.41 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/util_test.go Mon Dec 9 20:38:16 2019
@@ -227,9 +227,9 @@ func (s *Suite) Test_alignWith(c *check.
// At least one tab is _always_ added.
test("", "", "\t")
- test("VAR=", "1234567", "VAR=\t")
+ test("VAR=", "1234567", "VAR= ")
test("VAR=", "12345678", "VAR=\t")
- test("VAR=", "123456789", "VAR=\t\t")
+ test("VAR=", "123456789", "VAR=\t ")
// At least one tab is added in any case,
// even if the other string is shorter.
@@ -456,14 +456,6 @@ func (s *Suite) Test_containsVarRef(c *c
test("$$VAR", false) // An escaped dollar character.
}
-func (s *Suite) Test_hasAlnumPrefix(c *check.C) {
- t := s.Init(c)
-
- t.CheckEquals(hasAlnumPrefix(""), false)
- t.CheckEquals(hasAlnumPrefix("A"), true)
- t.CheckEquals(hasAlnumPrefix(","), false)
-}
-
func (s *Suite) Test_Once(c *check.C) {
t := s.Init(c)
Index: pkgsrc/pkgtools/pkglint/files/vartypecheck.go
diff -u pkgsrc/pkgtools/pkglint/files/vartypecheck.go:1.71 pkgsrc/pkgtools/pkglint/files/vartypecheck.go:1.72
--- pkgsrc/pkgtools/pkglint/files/vartypecheck.go:1.71 Sun Dec 8 22:03:38 2019
+++ pkgsrc/pkgtools/pkglint/files/vartypecheck.go Mon Dec 9 20:38:16 2019
@@ -1327,8 +1327,22 @@ func (cv *VartypeCheck) URL() {
}
func (cv *VartypeCheck) UserGroupName() {
- if cv.Value == cv.ValueNoVar && !matches(cv.Value, `^[0-9_a-z][0-9_a-z-]*[0-9_a-z]$`) {
- cv.Warnf("Invalid user or group name %q.", cv.Value)
+ value := cv.Value
+ if value != cv.ValueNoVar {
+ return
+ }
+ invalid := invalidCharacters(value, textproc.NewByteSet("---0-9_a-z"))
+ if invalid != "" {
+ cv.Warnf("User or group name %q contains invalid characters: %s",
+ value, invalid)
+ return
+ }
+
+ if hasPrefix(value, "-") {
+ cv.Errorf("User or group name %q must not start with a hyphen.", value)
+ }
+ if hasSuffix(value, "-") {
+ cv.Errorf("User or group name %q must not end with a hyphen.", value)
}
}
@@ -1473,7 +1487,7 @@ func (cv *VartypeCheck) Yes() {
"but using \".if defined(VARNAME)\" alone.")
default:
- if !matches(cv.Value, `^(?:YES|yes)(?:[\t ]+#.*)?$`) {
+ if cv.Value != "YES" && cv.Value != "yes" {
cv.Warnf("%s should be set to YES or yes.", cv.Varname)
cv.Explain(
"This variable means \"yes\" if it is defined, and \"no\" if it is undefined.",
@@ -1511,7 +1525,7 @@ func (cv *VartypeCheck) YesNo() {
"both forms are actually used.",
"As long as this is the case, when checking the variable value,",
"both must be accepted.")
- } else if !matches(cv.Value, `^(?:YES|yes|NO|no)(?:[\t ]+#.*)?$`) {
+ } else if !matches(cv.Value, `^(?:YES|yes|NO|no)$`) {
cv.Warnf("%s should be set to YES, yes, NO, or no.", cv.Varname)
}
}
Home |
Main Index |
Thread Index |
Old Index