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: Sat Jul 28 18:31:23 UTC 2018
Modified Files:
pkgsrc/pkgtools/pkglint: Makefile
pkgsrc/pkgtools/pkglint/files: check_test.go mkline.go mkline_test.go
mklinechecker.go mklines.go mklines_test.go mkparser.go
mkparser_test.go package.go package_test.go patches.go
patches_test.go pkgsrc.go shell_test.go util.go vartypecheck.go
vartypecheck_test.go
Log Message:
pkgtools/pkglint: update to 5.5.15
Changes since 5.5.14:
* Check that the comments in .endif and .endfor lines match the
corresponding conditions.
* Check for redundant variables (e.g. MASTER_SITES for R packages).
* Check for accidentally overwritten variables.
* Miscellaneous code cleanup and refactoring.
To generate a diff of this commit:
cvs rdiff -u -r1.542 -r1.543 pkgsrc/pkgtools/pkglint/Makefile
cvs rdiff -u -r1.20 -r1.21 pkgsrc/pkgtools/pkglint/files/check_test.go
cvs rdiff -u -r1.32 -r1.33 pkgsrc/pkgtools/pkglint/files/mkline.go
cvs rdiff -u -r1.35 -r1.36 pkgsrc/pkgtools/pkglint/files/mkline_test.go
cvs rdiff -u -r1.14 -r1.15 pkgsrc/pkgtools/pkglint/files/mklinechecker.go
cvs rdiff -u -r1.26 -r1.27 pkgsrc/pkgtools/pkglint/files/mklines.go \
pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go
cvs rdiff -u -r1.21 -r1.22 pkgsrc/pkgtools/pkglint/files/mklines_test.go \
pkgsrc/pkgtools/pkglint/files/patches.go
cvs rdiff -u -r1.12 -r1.13 pkgsrc/pkgtools/pkglint/files/mkparser.go
cvs rdiff -u -r1.10 -r1.11 pkgsrc/pkgtools/pkglint/files/mkparser_test.go
cvs rdiff -u -r1.31 -r1.32 pkgsrc/pkgtools/pkglint/files/package.go
cvs rdiff -u -r1.23 -r1.24 pkgsrc/pkgtools/pkglint/files/package_test.go \
pkgsrc/pkgtools/pkglint/files/util.go
cvs rdiff -u -r1.16 -r1.17 pkgsrc/pkgtools/pkglint/files/patches_test.go
cvs rdiff -u -r1.4 -r1.5 pkgsrc/pkgtools/pkglint/files/pkgsrc.go
cvs rdiff -u -r1.25 -r1.26 pkgsrc/pkgtools/pkglint/files/shell_test.go
cvs rdiff -u -r1.34 -r1.35 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.542 pkgsrc/pkgtools/pkglint/Makefile:1.543
--- pkgsrc/pkgtools/pkglint/Makefile:1.542 Thu Jul 19 06:38:15 2018
+++ pkgsrc/pkgtools/pkglint/Makefile Sat Jul 28 18:31:23 2018
@@ -1,6 +1,6 @@
-# $NetBSD: Makefile,v 1.542 2018/07/19 06:38:15 rillig Exp $
+# $NetBSD: Makefile,v 1.543 2018/07/28 18:31:23 rillig Exp $
-PKGNAME= pkglint-5.5.14
+PKGNAME= pkglint-5.5.15
DISTFILES= # none
CATEGORIES= pkgtools
Index: pkgsrc/pkgtools/pkglint/files/check_test.go
diff -u pkgsrc/pkgtools/pkglint/files/check_test.go:1.20 pkgsrc/pkgtools/pkglint/files/check_test.go:1.21
--- pkgsrc/pkgtools/pkglint/files/check_test.go:1.20 Thu Jul 19 06:38:15 2018
+++ pkgsrc/pkgtools/pkglint/files/check_test.go Sat Jul 28 18:31:23 2018
@@ -3,6 +3,7 @@ package main
import (
"bytes"
"fmt"
+ "io"
"io/ioutil"
"os"
"path"
@@ -296,11 +297,12 @@ func (t *Tester) CheckOutputLines(expect
t.c().Check(emptyToNil(actualLines), deepEquals, emptyToNil(expectedLines))
}
-// EnableTracing redirects all logging output to stdout instead of
-// the buffer. This is useful when stepping through the code, especially
+// EnableTracing redirects all logging output (which is normally captured
+// in an in-memory buffer) additionally to stdout.
+// This is useful when stepping through the code, especially
// in combination with SetupCommandLine("--debug").
func (t *Tester) EnableTracing() {
- G.logOut = NewSeparatorWriter(os.Stdout)
+ G.logOut = NewSeparatorWriter(io.MultiWriter(os.Stdout, &t.stdout))
trace.Out = os.Stdout
trace.Tracing = true
}
Index: pkgsrc/pkgtools/pkglint/files/mkline.go
diff -u pkgsrc/pkgtools/pkglint/files/mkline.go:1.32 pkgsrc/pkgtools/pkglint/files/mkline.go:1.33
--- pkgsrc/pkgtools/pkglint/files/mkline.go:1.32 Thu Jul 12 16:23:36 2018
+++ pkgsrc/pkgtools/pkglint/files/mkline.go Sat Jul 28 18:31:23 2018
@@ -38,6 +38,7 @@ type mkLineConditional struct {
indent string
directive string
args string
+ comment string
}
type mkLineInclude struct {
mustExist bool
@@ -112,8 +113,8 @@ func NewMkLine(line Line) *MkLineImpl {
return &MkLineImpl{line, mkLineEmpty{}}
}
- if m, indent, directive, args := matchMkCond(text); m {
- return &MkLineImpl{line, mkLineConditional{indent, directive, args}}
+ if m, indent, directive, args, comment := matchMkCond(text); m {
+ return &MkLineImpl{line, mkLineConditional{indent, directive, args, comment}}
}
if m, indent, directive, includefile := MatchMkInclude(text); m {
@@ -217,7 +218,8 @@ func (mkline *MkLineImpl) Varparam() str
// Op applies to variable assignments and returns the assignment operator.
func (mkline *MkLineImpl) Op() MkOperator { return mkline.data.(mkLineAssign).op }
-// For a variable assignment, the text up to and including the assignment operator, e.g. VARNAME+=\t
+// ValueAlign applies to variable assignments and returns all the text
+// before the variable value, e.g. "VARNAME+=\t".
func (mkline *MkLineImpl) ValueAlign() string { return mkline.data.(mkLineAssign).valueAlign }
func (mkline *MkLineImpl) Value() string { return mkline.data.(mkLineAssign).value }
@@ -235,14 +237,25 @@ func (mkline *MkLineImpl) Indent() strin
return mkline.data.(mkLineInclude).indent
}
}
-func (mkline *MkLineImpl) Directive() string { return mkline.data.(mkLineConditional).directive }
-func (mkline *MkLineImpl) Args() string { return mkline.data.(mkLineConditional).args }
+
+// Directive returns one of "if", "ifdef", "ifndef", "else", "elif", "endif", "for", "endfor", "undef".
+//
+// See matchMkCond.
+func (mkline *MkLineImpl) Directive() string { return mkline.data.(mkLineConditional).directive }
+func (mkline *MkLineImpl) Args() string { return mkline.data.(mkLineConditional).args }
+
+// CondComment is the trailing end-of-line comment, typically at a deeply nested .endif or .endfor.
+func (mkline *MkLineImpl) CondComment() string { return mkline.data.(mkLineConditional).comment }
+
func (mkline *MkLineImpl) MustExist() bool { return mkline.data.(mkLineInclude).mustExist }
func (mkline *MkLineImpl) IncludeFile() string { return mkline.data.(mkLineInclude).includeFile }
-func (mkline *MkLineImpl) Targets() string { return mkline.data.(mkLineDependency).targets }
-func (mkline *MkLineImpl) Sources() string { return mkline.data.(mkLineDependency).sources }
-// Initialized step by step, when parsing other lines
+func (mkline *MkLineImpl) Targets() string { return mkline.data.(mkLineDependency).targets }
+func (mkline *MkLineImpl) Sources() string { return mkline.data.(mkLineDependency).sources }
+
+// ConditionVars is a space-separated list of those variable names
+// on which the inclusion depends. It is initialized later,
+// step by step, when parsing other lines
func (mkline *MkLineImpl) ConditionVars() string { return mkline.data.(mkLineInclude).conditionVars }
func (mkline *MkLineImpl) SetConditionVars(varnames string) {
include := mkline.data.(mkLineInclude)
@@ -340,7 +353,7 @@ func (mkline *MkLineImpl) ExplainRelativ
"main pkgsrc repository.")
}
-func matchMkCond(text string) (m bool, indent, directive, args string) {
+func matchMkCond(text string) (m bool, indent, directive, args, comment string) {
i, n := 0, len(text)
if i < n && text[i] == '.' {
i++
@@ -375,6 +388,13 @@ func matchMkCond(text string) (m bool, i
for i < n && (text[i] != '#' || text[i-1] == '\\') {
i++
}
+ commentStart := i
+ if commentStart < n {
+ commentStart++
+ for commentStart < n && (text[commentStart] == ' ' || text[commentStart] == '\t') {
+ commentStart++
+ }
+ }
for i > argsStart && (text[i-1] == ' ' || text[i-1] == '\t') {
i--
}
@@ -383,6 +403,7 @@ func matchMkCond(text string) (m bool, i
m = true
indent = text[indentStart:indentEnd]
args = strings.Replace(text[argsStart:argsEnd], "\\#", "#", -1)
+ comment = text[commentStart:]
return
}
@@ -728,57 +749,66 @@ func (vuc *VarUseContext) String() strin
// An excepting are multiple-inclusion guards, they don't increase the
// indentation.
type Indentation struct {
- depth []int // Number of space characters; always a multiple of 2
- conditionVars [][]string // Variables on which the current path depends
+ levels []indentationLevel
+}
+
+func NewIndentation() *Indentation {
+ ind := &Indentation{}
+ ind.Push(0, "") // Dummy
+ return ind
+}
+
+type indentationLevel struct {
+ depth int // Number of space characters; always a multiple of 2
+ condition string // The corresponding condition from the .if or .elif
+ conditionVars []string // Variables on which the current path depends
// Files whose existence has been checked in a related path.
// The check counts for both the "if" and the "else" branch,
// but that sloppiness will be discovered by functional tests.
- checkedFiles [][]string
+ checkedFiles []string
}
func (ind *Indentation) Len() int {
- return len(ind.depth)
+ return len(ind.levels)
+}
+
+func (ind *Indentation) top() *indentationLevel {
+ return &ind.levels[ind.Len()-1]
}
+// Depth returns the number of space characters by which the directive
+// should be indented.
func (ind *Indentation) Depth(directive string) int {
switch directive {
- case "elif", "else", "endfor", "endif":
- return ind.depth[imax(0, len(ind.depth)-2)]
+ case "if", "elif", "else", "endfor", "endif":
+ return ind.levels[imax(0, ind.Len()-2)].depth
}
- return ind.depth[len(ind.depth)-1]
+ return ind.top().depth
}
func (ind *Indentation) Pop() {
- newlen := ind.Len() - 1
- ind.depth = ind.depth[:newlen]
- ind.conditionVars = ind.conditionVars[:newlen]
- ind.checkedFiles = ind.checkedFiles[:newlen]
+ ind.levels = ind.levels[:ind.Len()-1]
}
-func (ind *Indentation) Push(indent int) {
- ind.depth = append(ind.depth, indent)
- ind.conditionVars = append(ind.conditionVars, nil)
- ind.checkedFiles = append(ind.checkedFiles, nil)
+func (ind *Indentation) Push(indent int, condition string) {
+ ind.levels = append(ind.levels, indentationLevel{indent, condition, nil, nil})
}
func (ind *Indentation) AddVar(varname string) {
- level := ind.Len() - 1
- if hasSuffix(varname, "_MK") {
- return
- }
- for _, existingVarname := range ind.conditionVars[level] {
+ vars := &ind.top().conditionVars
+ for _, existingVarname := range *vars {
if varname == existingVarname {
return
}
}
- ind.conditionVars[level] = append(ind.conditionVars[level], varname)
+ *vars = append(*vars, varname)
}
func (ind *Indentation) DependsOn(varname string) bool {
- for _, levelVarnames := range ind.conditionVars {
- for _, levelVarname := range levelVarnames {
+ for _, level := range ind.levels {
+ for _, levelVarname := range level.conditionVars {
if varname == levelVarname {
return true
}
@@ -788,34 +818,46 @@ func (ind *Indentation) DependsOn(varnam
}
func (ind *Indentation) IsConditional() bool {
- for _, vars := range ind.conditionVars {
- if len(vars) > 0 {
- return true
+ for _, level := range ind.levels {
+ for _, varname := range level.conditionVars {
+ if !hasSuffix(varname, "_MK") {
+ return true
+ }
}
}
return false
}
+// Varnames returns the list of all variables that are mentioned in any
+// condition or loop surrounding the current line.
+// Variables named *_MK are excluded since they are usually not interesting.
func (ind *Indentation) Varnames() string {
sep := ""
varnames := ""
- for _, levelVarnames := range ind.conditionVars {
- for _, levelVarname := range levelVarnames {
- varnames += sep + levelVarname
- sep = ", "
+ for _, level := range ind.levels {
+ for _, levelVarname := range level.conditionVars {
+ if !hasSuffix(levelVarname, "_MK") {
+ varnames += sep + levelVarname
+ sep = ", "
+ }
}
}
return varnames
}
+// Condition returns the condition for the innermost .if, .elif or .for.
+func (ind *Indentation) Condition() string {
+ return ind.top().condition
+}
+
func (ind *Indentation) AddCheckedFile(filename string) {
- level := ind.Len() - 1
- ind.checkedFiles[level] = append(ind.checkedFiles[level], filename)
+ top := ind.top()
+ top.checkedFiles = append(top.checkedFiles, filename)
}
func (ind *Indentation) IsCheckedFile(filename string) bool {
- for _, levelFilenames := range ind.checkedFiles {
- for _, levelFilename := range levelFilenames {
+ for _, level := range ind.levels {
+ for _, levelFilename := range level.checkedFiles {
if filename == levelFilename {
return true
}
@@ -824,6 +866,68 @@ func (ind *Indentation) IsCheckedFile(fi
return false
}
+func (ind *Indentation) TrackBefore(mkline MkLine) {
+ if !mkline.IsCond() {
+ return
+ }
+ if trace.Tracing {
+ trace.Stepf("Indentation before line %s: %+v", mkline.Linenos(), ind.levels)
+ }
+
+ directive := mkline.Directive()
+ args := mkline.Args()
+
+ switch directive {
+ case "for", "if", "ifdef", "ifndef":
+ ind.Push(ind.top().depth, args)
+ }
+}
+
+func (ind *Indentation) TrackAfter(mkline MkLine) {
+ if !mkline.IsCond() {
+ return
+ }
+
+ directive := mkline.Directive()
+ args := mkline.Args()
+
+ switch directive {
+ case "if":
+ // For multiple-inclusion guards, the indentation stays at the same level.
+ if m, varname := match1(args, `^!defined\(([\w]+_MK)\)$`); m {
+ ind.AddVar(varname)
+ } else {
+ ind.top().depth += 2
+ }
+
+ // Note: adding the used variables for arbitrary conditions
+ // happens in MkLineChecker.CheckCond for performance reasons.
+
+ if contains(args, "exists") {
+ cond := NewMkParser(mkline.Line, args, false).MkCond()
+ cond.Visit("exists", func(node *Tree) {
+ ind.AddCheckedFile(node.args[0].(string))
+ })
+ }
+
+ case "for", "ifdef", "ifndef":
+ ind.top().depth += 2
+
+ case "elif":
+ // Handled here instead of TrackAfter to allow the action to access the previous condition.
+ ind.top().condition = args
+
+ case "endfor", "endif":
+ if ind.Len() > 1 { // Can only be false in unbalanced files.
+ ind.Pop()
+ }
+ }
+
+ if trace.Tracing {
+ trace.Stepf("Indentation after line %s: %+v", mkline.Linenos(), ind.levels)
+ }
+}
+
func MatchVarassign(text string) (m, commented bool, varname, spaceAfterVarname, op, valueAlign, value, spaceAfterValue, comment string) {
i, n := 0, len(text)
Index: pkgsrc/pkgtools/pkglint/files/mkline_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mkline_test.go:1.35 pkgsrc/pkgtools/pkglint/files/mkline_test.go:1.36
--- pkgsrc/pkgtools/pkglint/files/mkline_test.go:1.35 Thu Jul 12 16:23:36 2018
+++ pkgsrc/pkgtools/pkglint/files/mkline_test.go Sat Jul 28 18:31:23 2018
@@ -150,6 +150,7 @@ func (s *Suite) Test_NewMkLine(c *check.
c.Check(ln[4].Indent(), equals, " ")
c.Check(ln[4].Directive(), equals, "if")
c.Check(ln[4].Args(), equals, "!empty(PKGNAME:M*-*) && ${RUBY_RAILS_SUPPORTED:[#]} == 1")
+ c.Check(ln[4].CondComment(), equals, "cond comment")
c.Check(ln[5].IsInclude(), equals, true)
c.Check(ln[5].Indent(), equals, " ")
@@ -285,6 +286,7 @@ func (s *Suite) Test_MkLines_Check__extr
"COMMENT=\t# defined",
".endfor",
"GAMES_USER?=pkggames",
+ "GAMES_GROUP?=pkggames",
"PLIST_SUBST+= CONDITIONAL=${CONDITIONAL}",
"CONDITIONAL=\"@comment\"",
"BUILD_DIRS=\t${WRKSRC}/../build")
@@ -295,8 +297,8 @@ func (s *Suite) Test_MkLines_Check__extr
"WARN: options.mk:3: The values for PYTHON_VERSIONS_ACCEPTED should be in decreasing order.",
"NOTE: options.mk:5: Please use \"# empty\", \"# none\" or \"yes\" instead of \"# defined\".",
"WARN: options.mk:7: Please include \"../../mk/bsd.prefs.mk\" before using \"?=\".",
- "WARN: options.mk:10: Building the package should take place entirely inside ${WRKSRC}, not \"${WRKSRC}/..\".",
- "NOTE: options.mk:10: You can use \"../build\" instead of \"${WRKSRC}/../build\".")
+ "WARN: options.mk:11: Building the package should take place entirely inside ${WRKSRC}, not \"${WRKSRC}/..\".",
+ "NOTE: options.mk:11: You can use \"../build\" instead of \"${WRKSRC}/../build\".")
}
func (s *Suite) Test_MkLine_variableNeedsQuoting__unknown_rhs(c *check.C) {
@@ -846,16 +848,14 @@ func (s *Suite) Test_MatchVarassign(c *c
}
func (s *Suite) Test_Indentation(c *check.C) {
- ind := &Indentation{}
-
- ind.Push(0)
+ ind := NewIndentation()
c.Check(ind.Depth("if"), equals, 0)
c.Check(ind.DependsOn("VARNAME"), equals, false)
- ind.Push(2)
+ ind.Push(2, "")
- c.Check(ind.Depth("if"), equals, 2)
+ c.Check(ind.Depth("if"), equals, 0) // Because "if" is handled in MkLines.TrackBefore.
c.Check(ind.Depth("endfor"), equals, 0)
ind.AddVar("LEVEL1.VAR1")
@@ -868,7 +868,7 @@ func (s *Suite) Test_Indentation(c *chec
c.Check(ind.DependsOn("LEVEL1.VAR1"), equals, true)
c.Check(ind.DependsOn("OTHER_VAR"), equals, false)
- ind.Push(2)
+ ind.Push(2, "")
ind.AddVar("LEVEL2.VAR")
Index: pkgsrc/pkgtools/pkglint/files/mklinechecker.go
diff -u pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.14 pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.15
--- pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.14 Thu Jul 19 06:38:15 2018
+++ pkgsrc/pkgtools/pkglint/files/mklinechecker.go Sat Jul 28 18:31:23 2018
@@ -110,31 +110,43 @@ func (ck MkLineChecker) checkCond(forVar
directive := mkline.Directive()
args := mkline.Args()
+ comment := mkline.CondComment()
expectedDepth := indentation.Depth(directive)
ck.checkDirectiveIndentation(expectedDepth)
- if directive == "if" && matches(args, `^!defined\([\w]+_MK\)$`) {
- indentation.Push(indentation.Depth(directive))
- } else if matches(directive, `^(?:if|ifdef|ifndef|for)$`) {
- indentation.Push(indentation.Depth(directive) + 2)
- } else if directive == "endfor" || directive == "endif" {
- if indentation.Len() > 1 {
- indentation.Pop()
- } else {
+ if directive == "endfor" || directive == "endif" {
+ if directive == "endif" && comment != "" {
+ if condition := indentation.Condition(); !contains(condition, comment) {
+ mkline.Warnf("Comment %q does not match condition %q.", comment, condition)
+ }
+ }
+
+ if directive == "endfor" && comment != "" {
+ if condition := indentation.Condition(); !contains(condition, comment) {
+ mkline.Warnf("Comment %q does not match loop %q.", comment, condition)
+ }
+ }
+
+ if indentation.Len() <= 1 {
mkline.Errorf("Unmatched .%s.", directive)
}
}
- needsArgument := matches(directive, `^(?:if|ifdef|ifndef|elif|for|undef)$`)
- if needsArgument != (args != "") {
- if needsArgument {
- mkline.Errorf("\".%s\" requires arguments.", directive)
+ needsArgument := false
+ switch directive {
+ case "if", "ifdef", "ifndef", "elif", "for", "undef":
+ needsArgument = true
+ }
+
+ if needsArgument && args == "" {
+ mkline.Errorf("\".%s\" requires arguments.", directive)
+
+ } else if !needsArgument && args != "" {
+ if directive == "else" {
+ mkline.Errorf("\".%s\" does not take arguments. If you meant \"else if\", use \".elif\".", directive)
} else {
mkline.Errorf("\".%s\" does not take arguments.", directive)
- if directive == "else" {
- mkline.Notef("If you meant \"else if\", use \".elif\".")
- }
}
} else if directive == "if" || directive == "elif" {
@@ -147,6 +159,7 @@ func (ck MkLineChecker) checkCond(forVar
} else if directive == "for" {
if m, vars, values := match2(args, `^(\S+(?:\s*\S+)*?)\s+in\s+(.*)$`); m {
for _, forvar := range splitOnSpace(vars) {
+ indentation.AddVar(forvar)
if !G.Infrastructure && hasPrefix(forvar, "_") {
mkline.Warnf("Variable names starting with an underscore (%s) are reserved for internal pkgsrc use.", forvar)
}
@@ -850,6 +863,10 @@ func (ck MkLineChecker) checkVarassignBs
return
}
+ if G.Mk != nil && !G.Mk.FirstTime("include-bsd.prefs.mk") {
+ return
+ }
+
mkline.Warnf("Please include \"../../mk/bsd.prefs.mk\" before using \"?=\".")
Explain(
"The ?= operator is used to provide a default value to a variable.",
Index: pkgsrc/pkgtools/pkglint/files/mklines.go
diff -u pkgsrc/pkgtools/pkglint/files/mklines.go:1.26 pkgsrc/pkgtools/pkglint/files/mklines.go:1.27
--- pkgsrc/pkgtools/pkglint/files/mklines.go:1.26 Thu Jul 19 06:38:15 2018
+++ pkgsrc/pkgtools/pkglint/files/mklines.go Sat Jul 28 18:31:23 2018
@@ -18,7 +18,7 @@ type MkLines struct {
tools map[string]bool // Set of tools that are declared to be used.
toolRegistry ToolRegistry // Tools defined in file scope.
SeenBsdPrefsMk bool
- indentation Indentation // Indentation depth of preprocessing directives
+ indentation *Indentation // Indentation depth of preprocessing directives; only available during MkLines.ForEach.
Once
// XXX: Why both tools and toolRegistry?
}
@@ -46,7 +46,7 @@ func NewMkLines(lines []Line) *MkLines {
tools,
NewToolRegistry(),
false,
- Indentation{},
+ nil,
Once{}}
}
@@ -92,9 +92,9 @@ func (mklines *MkLines) Check() {
substcontext := NewSubstContext()
var varalign VaralignBlock
- indentation := &mklines.indentation
- indentation.Push(0)
- for _, mkline := range mklines.mklines {
+ lastMkline := mklines.mklines[len(mklines.mklines)-1]
+
+ lineAction := func(mkline MkLine) bool {
ck := MkLineChecker{mkline}
ck.Check()
varalign.Check(mkline)
@@ -115,11 +115,11 @@ func (mklines *MkLines) Check() {
mklines.setSeenBsdPrefsMk()
}
if G.Pkg != nil {
- G.Pkg.CheckInclude(mkline, indentation)
+ G.Pkg.CheckInclude(mkline, mklines.indentation)
}
case mkline.IsCond():
- ck.checkCond(mklines.forVars, indentation)
+ ck.checkCond(mklines.forVars, mklines.indentation)
substcontext.Conditional(mkline)
case mkline.IsDependency():
@@ -129,18 +129,42 @@ func (mklines *MkLines) Check() {
case mkline.IsShellCommand():
mkline.Tokenize(mkline.ShellCommand())
}
+
+ return true
}
- lastMkline := mklines.mklines[len(mklines.mklines)-1]
+
+ atEnd := func(mkline MkLine) {
+ ind := mklines.indentation
+ if ind.Len() != 1 && ind.Depth("") != 0 {
+ mkline.Errorf("Directive indentation is not 0, but %d.", ind.Depth(""))
+ }
+ }
+
+ mklines.ForEach(lineAction, atEnd)
+
substcontext.Finish(lastMkline)
varalign.Finish()
ChecklinesTrailingEmptyLines(mklines.lines)
- if indentation.Len() != 1 && indentation.Depth("") != 0 {
- lastMkline.Errorf("Directive indentation is not 0, but %d.", indentation.Depth(""))
+ SaveAutofixChanges(mklines.lines)
+}
+
+// ForEach calls the action for each line, until the action returns false.
+// It keeps track of the indentation and all conditionals.
+func (mklines *MkLines) ForEach(action func(mkline MkLine) bool, atEnd func(mkline MkLine)) {
+ mklines.indentation = NewIndentation()
+
+ for _, mkline := range mklines.mklines {
+ mklines.indentation.TrackBefore(mkline)
+ if !action(mkline) {
+ break
+ }
+ mklines.indentation.TrackAfter(mkline)
}
- SaveAutofixChanges(mklines.lines)
+ atEnd(mklines.mklines[len(mklines.mklines)-1])
+ mklines.indentation = nil
}
func (mklines *MkLines) DetermineDefinedVariables() {
@@ -276,6 +300,41 @@ func (mklines *MkLines) setSeenBsdPrefsM
}
}
+func (mklines *MkLines) CheckRedundantVariables() {
+ scope := NewRedundantScope()
+ isRelevant := func(old, new MkLine) bool {
+ if path.Base(old.Filename) != "Makefile" && path.Base(new.Filename) == "Makefile" {
+ return false
+ }
+ if new.Op() == opAssignEval {
+ return false
+ }
+ return true
+ }
+ scope.OnIgnore = func(old, new MkLine) {
+ if isRelevant(old, new) {
+ old.Notef("Definition of %s is redundant because of %s.", new.Varname(), new.ReferenceFrom(old.Line))
+ }
+ }
+ scope.OnOverwrite = func(old, new MkLine) {
+ if isRelevant(old, new) {
+ old.Warnf("Variable %s is overwritten in %s.", new.Varname(), new.ReferenceFrom(old.Line))
+ Explain(
+ "The variable definition in this line does not have an effect since",
+ "it is overwritten elsewhere. This typically happens because of a",
+ "typo (writing = instead of +=) or because the line that overwrites",
+ "is in another file that is used by several packages.")
+ }
+ }
+
+ mklines.ForEach(
+ func(mkline MkLine) bool {
+ scope.Handle(mkline)
+ return true
+ },
+ func(mkline MkLine) {})
+}
+
// VaralignBlock checks that all variable assignments from a paragraph
// use the same indentation depth for their values.
// It also checks that the indentation uses tabs instead of spaces.
Index: pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go
diff -u pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go:1.26 pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go:1.27
--- pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go:1.26 Thu Jul 12 16:23:36 2018
+++ pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go Sat Jul 28 18:31:23 2018
@@ -570,11 +570,14 @@ func (s *Suite) Test_VartypeCheck_Tool(c
runVartypeChecks(t, "USE_TOOLS", opAssignAppend, (*VartypeCheck).Tool,
"tool3:run",
- "tool2:unknown")
+ "tool2:unknown",
+ "${t}",
+ "mal:formed:tool")
t.CheckOutputLines(
- "ERROR: fname:2: Unknown tool dependency \"unknown\". " +
- "Use one of \"bootstrap\", \"build\", \"pkgsrc\" or \"run\" or \"test\".")
+ "ERROR: fname:2: Unknown tool dependency \"unknown\". "+
+ "Use one of \"bootstrap\", \"build\", \"pkgsrc\", \"run\" or \"test\".",
+ "ERROR: fname:4: Malformed tool dependency: \"mal:formed:tool\".")
runVartypeChecks(t, "USE_TOOLS.NetBSD", opAssignAppend, (*VartypeCheck).Tool,
"tool3:run",
@@ -582,7 +585,7 @@ func (s *Suite) Test_VartypeCheck_Tool(c
t.CheckOutputLines(
"ERROR: fname:2: Unknown tool dependency \"unknown\". " +
- "Use one of \"bootstrap\", \"build\", \"pkgsrc\" or \"run\" or \"test\".")
+ "Use one of \"bootstrap\", \"build\", \"pkgsrc\", \"run\" or \"test\".")
}
func (s *Suite) Test_VartypeCheck_VariableName(c *check.C) {
Index: pkgsrc/pkgtools/pkglint/files/mklines_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mklines_test.go:1.21 pkgsrc/pkgtools/pkglint/files/mklines_test.go:1.22
--- pkgsrc/pkgtools/pkglint/files/mklines_test.go:1.21 Thu Jul 19 06:38:15 2018
+++ pkgsrc/pkgtools/pkglint/files/mklines_test.go Sat Jul 28 18:31:23 2018
@@ -409,8 +409,7 @@ func (s *Suite) Test_MkLines_Check_inden
"NOTE: options.mk:9: This directive should be indented by 2 spaces.",
"NOTE: options.mk:10: This directive should be indented by 2 spaces.",
"NOTE: options.mk:11: This directive should be indented by 2 spaces.",
- "ERROR: options.mk:11: \".else\" does not take arguments.",
- "NOTE: options.mk:11: If you meant \"else if\", use \".elif\".",
+ "ERROR: options.mk:11: \".else\" does not take arguments. If you meant \"else if\", use \".elif\".",
"NOTE: options.mk:12: This directive should be indented by 2 spaces.",
"NOTE: options.mk:13: This directive should be indented by 0 spaces.",
"NOTE: options.mk:14: This directive should be indented by 0 spaces.",
@@ -418,6 +417,42 @@ func (s *Suite) Test_MkLines_Check_inden
"ERROR: options.mk:15: Unmatched .endif.")
}
+func (s *Suite) Test_MkLines_Check__endif_comment(c *check.C) {
+ t := s.Init(c)
+
+ t.SetupCommandLine("-Wall")
+ mklines := t.NewMkLines("opsys.mk",
+ MkRcsID,
+ "",
+ ".for i in 1 2 3 4 5",
+ ". if ${OPSYS} == NetBSD",
+ ". if ${ARCH} == x86_64",
+ ". if ${OS_VERSION:M8.*}",
+ ". endif # ARCH", // Wrong, should be OS_VERSION.
+ ". endif # OS_VERSION", // Wrong, should be ARCH.
+ ". endif # OPSYS", // Correct.
+ ".endfor # j", // Wrong, should be i.
+ "",
+ ".if ${PKG_OPTIONS:Moption}",
+ ".endif # option",
+ "",
+ ".if ${PKG_OPTIONS:Moption}",
+ ".endif # opti", // This typo gets unnoticed since "opti" is a substring of the condition.
+ "",
+ ".if ${OPSYS} == NetBSD",
+ ".elif ${OPSYS} == FreeBSD",
+ ".endif # NetBSD") // Wrong, should be FreeBSD from the .elif.
+
+ // See MkLineChecker.checkCond
+ mklines.Check()
+
+ t.CheckOutputLines(""+
+ "WARN: opsys.mk:7: Comment \"ARCH\" does not match condition \"${OS_VERSION:M8.*}\".",
+ "WARN: opsys.mk:8: Comment \"OS_VERSION\" does not match condition \"${ARCH} == x86_64\".",
+ "WARN: opsys.mk:10: Comment \"j\" does not match loop \"i in 1 2 3 4 5\".",
+ "WARN: opsys.mk:20: Comment \"NetBSD\" does not match condition \"${OPSYS} == FreeBSD\".")
+}
+
// Demonstrates how to define your own make(1) targets.
func (s *Suite) Test_MkLines_wip_category_Makefile(c *check.C) {
t := s.Init(c)
@@ -532,3 +567,35 @@ func (s *Suite) Test_MkLines__unknown_op
t.CheckOutputLines(
"WARN: options.mk:4: Unknown option \"unknown\".")
}
+
+func (s *Suite) Test_MkLines_CheckRedundantVariables(c *check.C) {
+ t := s.Init(c)
+ included := t.NewMkLines("module.mk",
+ "VAR=\tvalue ${OTHER}",
+ "VAR?=\tvalue ${OTHER}",
+ "VAR=\tnew value")
+ makefile := t.NewMkLines("Makefile",
+ "VAR=\tthe package may overwrite variables from other files")
+ allLines := append(append([]Line(nil), included.lines...), makefile.lines...)
+ mklines := NewMkLines(allLines)
+
+ // XXX: The warnings from here are not in the same order as the other warnings.
+ // XXX: There may be some warnings for the same file separated by warnings for other files.
+ mklines.CheckRedundantVariables()
+
+ t.CheckOutputLines(
+ "NOTE: module.mk:1: Definition of VAR is redundant because of line 2.",
+ "WARN: module.mk:1: Variable VAR is overwritten in line 3.")
+}
+
+func (s *Suite) Test_MkLines_CheckRedundantVariables__procedure_call(c *check.C) {
+ t := s.Init(c)
+ mklines := t.NewMkLines("mk/pthread.buildlink3.mk",
+ "CHECK_BUILTIN.pthread:=\tyes",
+ ".include \"../../mk/pthread.builtin.mk\"",
+ "CHECK_BUILTIN.pthread:=\tno")
+
+ mklines.CheckRedundantVariables()
+
+ t.CheckOutputEmpty()
+}
Index: pkgsrc/pkgtools/pkglint/files/patches.go
diff -u pkgsrc/pkgtools/pkglint/files/patches.go:1.21 pkgsrc/pkgtools/pkglint/files/patches.go:1.22
--- pkgsrc/pkgtools/pkglint/files/patches.go:1.21 Thu Jul 12 16:23:36 2018
+++ pkgsrc/pkgtools/pkglint/files/patches.go Sat Jul 28 18:31:23 2018
@@ -143,7 +143,7 @@ func (ck *PatchChecker) checkUnifiedDiff
linesToAdd--
ck.checklineAdded(text[1:], patchedFileType)
case hasPrefix(text, "\\"):
- // \ No newline at end of file
+ // \ No newline at end of file (or a translation of that message)
default:
line.Errorf("Invalid line in unified patch hunk")
return
@@ -168,9 +168,10 @@ func (ck *PatchChecker) checkUnifiedDiff
if !ck.isEmptyLine(line.Text) && !matches(line.Text, rePatchUniFileDel) {
line.Warnf("Empty line or end of file expected.")
Explain(
- "This empty line makes the end of the patch clearly visible.",
- "Otherwise the reader would have to count lines to see where",
- "the patch ends.")
+ "This line is not part of the patch anymore, although it may",
+ "look so. To make this situation clear, there should be an",
+ "empty line before this line. If the line doesn't contain",
+ "useful information, it should be removed.")
}
}
}
@@ -185,7 +186,13 @@ func (ck *PatchChecker) checkBeginDiff(l
Explain(
"Pkgsrc tries to have as few patches as possible. Therefore, each",
"patch must document why it is necessary. Typical reasons are",
- "portability or security.",
+ "portability or security. A typical documented patch looks like",
+ "this:",
+ "",
+ "\t$"+"NetBSD$",
+ "",
+ "\tPortability fixes for GCC 4.8 on Linux.",
+ "\tSee https://github.com/org/repo/issues/7";,
"",
"Patches that are related to a security issue should mention the",
"corresponding CVE identifier.",
Index: pkgsrc/pkgtools/pkglint/files/mkparser.go
diff -u pkgsrc/pkgtools/pkglint/files/mkparser.go:1.12 pkgsrc/pkgtools/pkglint/files/mkparser.go:1.13
--- pkgsrc/pkgtools/pkglint/files/mkparser.go:1.12 Sat Jan 13 23:56:14 2018
+++ pkgsrc/pkgtools/pkglint/files/mkparser.go Sat Jul 28 18:31:23 2018
@@ -201,6 +201,8 @@ func (p *MkParser) VarUseModifiers(varna
return modifiers
}
+// MkCond parses a condition like ${OPSYS} == "NetBSD".
+// See devel/bmake/files/cond.c.
func (p *MkParser) MkCond() *Tree {
return p.mkCondOr()
}
Index: pkgsrc/pkgtools/pkglint/files/mkparser_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mkparser_test.go:1.10 pkgsrc/pkgtools/pkglint/files/mkparser_test.go:1.11
--- pkgsrc/pkgtools/pkglint/files/mkparser_test.go:1.10 Sat Mar 24 14:32:49 2018
+++ pkgsrc/pkgtools/pkglint/files/mkparser_test.go Sat Jul 28 18:31:23 2018
@@ -210,6 +210,8 @@ func (s *Suite) Test_MkParser_MkCond(c *
NewTree("compareVarNum", varuse("OS_VERSION"), "==", "5.3"))
check("!empty(${OS_VARIANT:MIllumos})", // Probably not intended
NewTree("not", NewTree("empty", varuse("${OS_VARIANT:MIllumos}"))))
+ check("defined (VARNAME)", // There may be whitespace before the parenthesis; see devel/bmake/files/cond.c:^compare_function.
+ NewTree("defined", "VARNAME"))
// Errors
checkRest("!empty(PKG_OPTIONS:Msndfile) || defined(PKG_OPTIONS:Msamplerate)",
Index: pkgsrc/pkgtools/pkglint/files/package.go
diff -u pkgsrc/pkgtools/pkglint/files/package.go:1.31 pkgsrc/pkgtools/pkglint/files/package.go:1.32
--- pkgsrc/pkgtools/pkglint/files/package.go:1.31 Thu Jul 12 16:23:36 2018
+++ pkgsrc/pkgtools/pkglint/files/package.go Sat Jul 28 18:31:23 2018
@@ -249,6 +249,7 @@ func (pkg *Package) loadPackageMakefile(
}
allLines.DetermineUsedVariables()
+ allLines.CheckRedundantVariables()
pkg.Pkgdir = pkg.expandVariableWithDefault("PKGDIR", ".")
pkg.DistinfoFile = pkg.expandVariableWithDefault("DISTINFO_FILE", "distinfo")
@@ -287,8 +288,8 @@ func (pkg *Package) readMakefile(fname s
isMainMakefile := len(mainLines.mklines) == 0
- for _, mkline := range fileMklines.mklines {
-
+ result := true
+ lineAction := func(mkline MkLine) bool {
if isMainMakefile {
mainLines.mklines = append(mainLines.mklines, mkline)
mainLines.lines = append(mainLines.lines, mkline.Line)
@@ -296,28 +297,6 @@ func (pkg *Package) readMakefile(fname s
allLines.mklines = append(allLines.mklines, mkline)
allLines.lines = append(allLines.lines, mkline.Line)
- if mkline.IsCond() {
- ind := &fileMklines.indentation
- switch mkline.Directive() {
- case "if", "ifdef", "ifndef", "for":
- ind.Push(0) // Dummy indentation, only the checkedFiles are interesting
- case "endfor", "endif":
- if ind.Len() > 1 {
- ind.Pop()
- }
- }
-
- if mkline.Directive() == "if" {
- args := mkline.Args()
- if contains(args, "exists") {
- cond := NewMkParser(mkline.Line, args, false).MkCond()
- cond.Visit("exists", func(node *Tree) {
- ind.AddCheckedFile(node.args[0].(string))
- })
- }
- }
- }
-
var includeFile, incDir, incBase string
if mkline.IsInclude() {
inc := mkline.IncludeFile()
@@ -368,12 +347,13 @@ func (pkg *Package) readMakefile(fname s
if !fileExists(dirname + "/" + includeFile) {
if fileMklines.indentation.IsCheckedFile(includeFile) {
- continue // See https://github.com/rillig/pkglint/issues/1
+ return true // See https://github.com/rillig/pkglint/issues/1
} else if dirname != G.CurrentDir { // Prevent unnecessary syscalls
dirname = G.CurrentDir
if !fileExists(dirname + "/" + includeFile) {
mkline.Errorf("Cannot read %q.", dirname+"/"+includeFile)
+ result = false
return false
}
}
@@ -385,6 +365,7 @@ func (pkg *Package) readMakefile(fname s
absIncluding := ifelseStr(incBase == "Makefile.common" && incDir != "", fname, "")
absIncluded := dirname + "/" + includeFile
if !pkg.readMakefile(absIncluded, mainLines, allLines, absIncluding) {
+ result = false
return false
}
}
@@ -400,13 +381,16 @@ func (pkg *Package) readMakefile(fname s
G.Pkg.vars.Define(varname, mkline)
}
}
+ return true
}
+ atEnd := func(mkline MkLine) {}
+ fileMklines.ForEach(lineAction, atEnd)
if includingFnameForUsedCheck != "" {
fileMklines.checkForUsedComment(G.Pkgsrc.ToRel(includingFnameForUsedCheck))
}
- return true
+ return result
}
func (pkg *Package) checkfilePackageMakefile(fname string, mklines *MkLines) {
Index: pkgsrc/pkgtools/pkglint/files/package_test.go
diff -u pkgsrc/pkgtools/pkglint/files/package_test.go:1.23 pkgsrc/pkgtools/pkglint/files/package_test.go:1.24
--- pkgsrc/pkgtools/pkglint/files/package_test.go:1.23 Thu Jul 12 16:23:36 2018
+++ pkgsrc/pkgtools/pkglint/files/package_test.go Sat Jul 28 18:31:23 2018
@@ -419,7 +419,11 @@ func (s *Suite) Test_Package_loadPackage
// Including a package Makefile directly is an error (in the last line),
// but that is checked later.
- t.CheckOutputEmpty()
+ // A file including itself does not lead to an endless loop while parsing
+ // but may still produce unexpected warnings, such as redundant definitions.
+ t.CheckOutputLines(
+ "WARN: ~/category/package/Makefile:3: Variable PKGNAME is overwritten in Makefile:3.",
+ "WARN: ~/category/package/Makefile:4: Variable DISTNAME is overwritten in Makefile:4.")
}
func (s *Suite) Test_Package_conditionalAndUnconditionalInclude(c *check.C) {
@@ -538,3 +542,42 @@ func (s *Suite) Test_Package_includeOthe
t.CheckOutputLines(
"ERROR: ~/category/package/another.mk: Cannot be read.")
}
+
+// See https://mail-index.netbsd.org/tech-pkg/2018/07/22/msg020092.html
+func (s *Suite) Test_Package__redundant_master_sites(c *check.C) {
+ t := s.Init(c)
+
+ t.SetupVartypes()
+ t.SetupMasterSite("MASTER_SITE_R_CRAN", "http://cran.r-project.org/src/";)
+ t.CreateFileLines("mk/bsd.pkg.mk")
+ t.CreateFileLines("licenses/gnu-gpl-v2",
+ "The licenses for most software are designed to take away ...")
+ t.CreateFileLines("math/R/Makefile.extension",
+ MkRcsID,
+ "",
+ "PKGNAME?=\tR-${R_PKGNAME}-${R_PKGVER}",
+ "MASTER_SITES?=\t${MASTER_SITE_R_CRAN:=contrib/}",
+ "GENERATE_PLIST+=\techo \"bin/r-package\";",
+ "NO_CHECKSUM=\tyes",
+ "LICENSE?=\tgnu-gpl-v2")
+ t.CreateFileLines("math/R-date/Makefile",
+ MkRcsID,
+ "",
+ "R_PKGNAME=\tdate",
+ "R_PKGVER=\t1.2.3",
+ "COMMENT=\tR package for handling date arithmetic",
+ "MASTER_SITES=\t${MASTER_SITE_R_CRAN:=contrib/}", // Redundant; see math/R/Makefile.extension.
+ "",
+ ".include \"../../math/R/Makefile.extension\"",
+ ".include \"../../mk/bsd.pkg.mk\"")
+
+ G.CurrentDir = t.TempFilename("math/R-date")
+ G.CurPkgsrcdir = "../.."
+
+ // See Package.checkfilePackageMakefile
+ // See Scope.uncond
+ G.checkdirPackage(G.CurrentDir)
+
+ t.CheckOutputLines(
+ "NOTE: ~/math/R-date/Makefile:6: Definition of MASTER_SITES is redundant because of ../R/Makefile.extension:4.")
+}
Index: pkgsrc/pkgtools/pkglint/files/util.go
diff -u pkgsrc/pkgtools/pkglint/files/util.go:1.23 pkgsrc/pkgtools/pkglint/files/util.go:1.24
--- pkgsrc/pkgtools/pkglint/files/util.go:1.23 Mon May 14 20:25:48 2018
+++ pkgsrc/pkgtools/pkglint/files/util.go Sat Jul 28 18:31:23 2018
@@ -561,3 +561,83 @@ func naturalLess(str1, str2 string) bool
// it sorts last.
return len1 < len2
}
+
+// RedundantScope checks for redundant variable definitions and accidentally
+// overwriting variables. It tries to be as correct as possible by not flagging
+// anything that is defined conditionally. There may be some edge cases though
+// like defining PKGNAME, then evaluating it using :=, then defining it again.
+// This pattern is so error-prone that it should not appear in pkgsrc at all,
+// thus pkglint doesn't even expect it. (Well, except for the PKGNAME case,
+// but that's deep in the infrastructure and only affects the "nb13" extension.
+type RedundantScope struct {
+ vars map[string]*redundantScopeVarinfo
+ condLevel int
+ OnIgnore func(old, new MkLine)
+ OnOverwrite func(old, new MkLine)
+}
+type redundantScopeVarinfo struct {
+ mkline MkLine
+ value string
+}
+
+func NewRedundantScope() *RedundantScope {
+ return &RedundantScope{vars: make(map[string]*redundantScopeVarinfo)}
+}
+
+func (s *RedundantScope) Handle(mkline MkLine) {
+ switch {
+ case mkline.IsVarassign():
+ varname := mkline.Varname()
+ if s.condLevel != 0 {
+ s.vars[varname] = nil
+ break
+ }
+
+ op := mkline.Op()
+ value := mkline.Value()
+ valueNovar := mkline.WithoutMakeVariables(value)
+ if op == opAssignEval && value == valueNovar {
+ op = opAssign // They are effectively the same in this case.
+ }
+ existing, found := s.vars[varname]
+ if !found {
+ if op == opAssignShell || op == opAssignEval {
+ s.vars[varname] = nil
+ } else {
+ if op == opAssignAppend {
+ value = " " + value
+ }
+ s.vars[varname] = &redundantScopeVarinfo{mkline, value}
+ }
+ } else if existing != nil {
+ switch op {
+ case opAssign:
+ if s.OnOverwrite != nil {
+ s.OnOverwrite(existing.mkline, mkline)
+ }
+ existing.value = value
+ case opAssignAppend:
+ existing.value += " " + value
+ case opAssignDefault:
+ if s.OnIgnore != nil {
+ s.OnIgnore(existing.mkline, mkline)
+ }
+ case opAssignShell:
+ case opAssignEval:
+ s.vars[varname] = nil
+ }
+ }
+
+ case mkline.IsCond():
+ switch mkline.Directive() {
+ case "for", "if", "ifdef", "ifndef":
+ s.condLevel++
+ case "endfor", "endif":
+ s.condLevel--
+ }
+ }
+}
+
+func (s *RedundantScope) IsConditional(varname string) bool {
+ return s.vars[varname] != nil
+}
Index: pkgsrc/pkgtools/pkglint/files/patches_test.go
diff -u pkgsrc/pkgtools/pkglint/files/patches_test.go:1.16 pkgsrc/pkgtools/pkglint/files/patches_test.go:1.17
--- pkgsrc/pkgtools/pkglint/files/patches_test.go:1.16 Mon Feb 19 12:40:38 2018
+++ pkgsrc/pkgtools/pkglint/files/patches_test.go Sat Jul 28 18:31:23 2018
@@ -2,6 +2,7 @@ package main
import "gopkg.in/check.v1"
+// This is how each patch should look like.
func (s *Suite) Test_ChecklinesPatch__with_comment(c *check.C) {
t := s.Init(c)
@@ -17,7 +18,7 @@ func (s *Suite) Test_ChecklinesPatch__wi
"@@ -5,3 +5,3 @@",
" context before",
"-old line",
- "+old line",
+ "+new line",
" context after")
ChecklinesPatch(lines)
@@ -25,6 +26,8 @@ func (s *Suite) Test_ChecklinesPatch__wi
t.CheckOutputEmpty()
}
+// To make the patch comment clearly visible, it should be surrounded by empty lines.
+// The missing empty lines are inserted by pkglint.
func (s *Suite) Test_ChecklinesPatch__without_empty_line__autofix(c *check.C) {
t := s.Init(c)
@@ -36,12 +39,13 @@ func (s *Suite) Test_ChecklinesPatch__wi
"@@ -5,3 +5,3 @@",
" context before",
"-old line",
- "+old line",
+ "+new line",
" context after")
t.SetupFileLines("distinfo",
RcsID,
"",
- "SHA1 (some patch) = 87ffcaaa0b0677ec679fff612b44df1af05f04df") // Taken from breakpoint at AutofixDistinfo
+ // The hash is taken from a breakpoint at the beginning of AutofixDistinfo, oldSha1
+ "SHA1 (some patch) = 49abd735b7e706ea9ed6671628bb54e91f7f5ffb")
t.SetupCommandLine("-Wall", "--autofix")
G.CurrentDir = t.TmpDir()
@@ -52,8 +56,8 @@ func (s *Suite) Test_ChecklinesPatch__wi
t.CheckOutputLines(
"AUTOFIX: ~/patch-WithoutEmptyLines:2: Inserting a line \"\" before this line.",
"AUTOFIX: ~/patch-WithoutEmptyLines:3: Inserting a line \"\" before this line.",
- "AUTOFIX: ~/distinfo:3: Replacing \"87ffcaaa0b0677ec679fff612b44df1af05f04df\" "+
- "with \"a7c35294b3853da0acedf8a972cb266baa9582a3\".")
+ "AUTOFIX: ~/distinfo:3: Replacing \"49abd735b7e706ea9ed6671628bb54e91f7f5ffb\" "+
+ "with \"4938fc8c0b483dc2e33e741b0da883d199e78164\".")
t.CheckFileLines("patch-WithoutEmptyLines",
RcsID,
@@ -65,12 +69,38 @@ func (s *Suite) Test_ChecklinesPatch__wi
"@@ -5,3 +5,3 @@",
" context before",
"-old line",
- "+old line",
+ "+new line",
" context after")
t.CheckFileLines("distinfo",
RcsID,
"",
- "SHA1 (some patch) = a7c35294b3853da0acedf8a972cb266baa9582a3")
+ "SHA1 (some patch) = 4938fc8c0b483dc2e33e741b0da883d199e78164")
+}
+
+func (s *Suite) Test_ChecklinesPatch__no_comment_and_no_empty_lines(c *check.C) {
+ t := s.Init(c)
+
+ patchLines := t.SetupFileLines("patch-WithoutEmptyLines",
+ RcsID,
+ "--- file.orig",
+ "+++ file",
+ "@@ -1,1 +1,1 @@",
+ "-old line",
+ "+new line")
+
+ t.SetupCommandLine("-Wall")
+
+ ChecklinesPatch(patchLines)
+
+ // These duplicate notes are actually correct. There should be an
+ // empty line above the documentation and one below it. Since there
+ // is no documentation yet, the line number for above and below is
+ // the same. Outside of the testing environment, this duplicate
+ // diagnostic is suppressed; see LogVerbose.
+ t.CheckOutputLines(
+ "NOTE: ~/patch-WithoutEmptyLines:2: Empty line expected.",
+ "ERROR: ~/patch-WithoutEmptyLines:2: Each patch must be documented.",
+ "NOTE: ~/patch-WithoutEmptyLines:2: Empty line expected.")
}
func (s *Suite) Test_ChecklinesPatch__without_comment(c *check.C) {
@@ -94,6 +124,8 @@ func (s *Suite) Test_ChecklinesPatch__wi
"ERROR: patch-WithoutComment:3: Each patch must be documented.")
}
+// Autogenerated git "comments" don't count as real comments since they
+// don't convey any intention of a human developer.
func (s *Suite) Test_ChecklinesPatch__git_without_comment(c *check.C) {
t := s.Init(c)
@@ -125,6 +157,9 @@ func (s *Suite) Test_checklineOtherAbsol
t.CheckOutputEmpty()
}
+// The output of BSD Make typically contains "*** Error code".
+// In some really good patches, this output is included in the patch comment,
+// to document why the patch is necessary.
func (s *Suite) Test_ChecklinesPatch__error_code(c *check.C) {
t := s.Init(c)
@@ -157,8 +192,8 @@ func (s *Suite) Test_ChecklinesPatch__wr
"Text",
"Text",
"",
- "+++ file", // Wrong
- "--- file.orig", // Wrong
+ "+++ file", // Wrong order
+ "--- file.orig", // Wrong order
"@@ -5,3 +5,3 @@",
" context before",
"-old line",
@@ -171,6 +206,7 @@ func (s *Suite) Test_ChecklinesPatch__wr
"WARN: patch-WrongOrder:7: Unified diff headers should be first ---, then +++.")
}
+// Context diffs are old and deprecated. Therefore pkglint doesn't check them thoroughly.
func (s *Suite) Test_ChecklinesPatch__context_diff(c *check.C) {
t := s.Init(c)
@@ -228,6 +264,7 @@ func (s *Suite) Test_ChecklinesPatch__tw
"WARN: patch-aa: Contains patches for 2 files, should be only one.")
}
+// The patch headers are only recognized as such if they appear directly below each other.
func (s *Suite) Test_ChecklinesPatch__documentation_that_looks_like_patch_lines(c *check.C) {
t := s.Init(c)
@@ -308,6 +345,11 @@ func (s *Suite) Test_ChecklinesPatch__Ma
"WARN: patch-unified:10: Found absolute pathname: /bin/cp",
"WARN: patch-unified:13: Found absolute pathname: /bin/cp")
+ // With extra warnings turned on, absolute paths in the context lines
+ // are also checked, to detect absolute paths that might be overlooked.
+ // TODO: Maybe this should only be checked if the patch changes
+ // an absolute path to a relative one, because otherwise these
+ // absolute paths may be intentional.
G.opts.WarnExtra = true
ChecklinesPatch(lines)
@@ -363,6 +405,8 @@ func (s *Suite) Test_ChecklinesPatch__no
t.CheckOutputEmpty()
}
+// Some patch files may end before reaching the expected line count (in this case 7 lines).
+// This is ok if only context lines are missing. These context lines are assumed to be empty lines.
func (s *Suite) Test_ChecklinesPatch__empty_lines_left_out_at_eof(c *check.C) {
t := s.Init(c)
@@ -386,7 +430,7 @@ func (s *Suite) Test_ChecklinesPatch__em
t.CheckOutputEmpty()
}
-// In some context lines, the leading space character is missing.
+// In some context lines, the leading space character may be missing.
// Since this is no problem for patch(1), pkglint also doesn't complain.
func (s *Suite) Test_ChecklinesPatch__context_lines_with_tab_instead_of_space(c *check.C) {
t := s.Init(c)
Index: pkgsrc/pkgtools/pkglint/files/pkgsrc.go
diff -u pkgsrc/pkgtools/pkglint/files/pkgsrc.go:1.4 pkgsrc/pkgtools/pkglint/files/pkgsrc.go:1.5
--- pkgsrc/pkgtools/pkglint/files/pkgsrc.go:1.4 Sat May 19 12:58:25 2018
+++ pkgsrc/pkgtools/pkglint/files/pkgsrc.go Sat Jul 28 18:31:23 2018
@@ -208,7 +208,7 @@ func (src *Pkgsrc) loadTools() {
}
}
- } else if m, _, cond, _ := matchMkCond(text); m {
+ } else if m, _, cond, _, _ := matchMkCond(text); m {
switch cond {
case "if", "ifdef", "ifndef", "for":
condDepth++
Index: pkgsrc/pkgtools/pkglint/files/shell_test.go
diff -u pkgsrc/pkgtools/pkglint/files/shell_test.go:1.25 pkgsrc/pkgtools/pkglint/files/shell_test.go:1.26
--- pkgsrc/pkgtools/pkglint/files/shell_test.go:1.25 Thu Jul 12 16:23:36 2018
+++ pkgsrc/pkgtools/pkglint/files/shell_test.go Sat Jul 28 18:31:23 2018
@@ -116,7 +116,12 @@ func (s *Suite) Test_ShellLine_CheckShel
"\t"+shellCommand)
shline := NewShellLine(G.Mk.mklines[0])
- shline.CheckShellCommandLine(shline.mkline.ShellCommand())
+ G.Mk.ForEach(
+ func(mkline MkLine) bool {
+ shline.CheckShellCommandLine(shline.mkline.ShellCommand())
+ return true
+ },
+ func(mkline MkLine) {})
}
checkShellCommandLine("@# Comment")
@@ -262,9 +267,14 @@ func (s *Suite) Test_ShellLine_CheckShel
checkShellCommandLine := func(shellCommand string) {
G.Mk = t.NewMkLines("fname",
"\t"+shellCommand)
- shline := NewShellLine(G.Mk.mklines[0])
- shline.CheckShellCommandLine(shline.mkline.ShellCommand())
+ G.Mk.ForEach(
+ func(mkline MkLine) bool {
+ shline := NewShellLine(mkline)
+ shline.CheckShellCommandLine(mkline.ShellCommand())
+ return true
+ },
+ func(mkline MkLine) {})
}
checkShellCommandLine("${STRIP} executable")
@@ -380,12 +390,22 @@ func (s *Suite) Test_ShellLine_CheckShel
c.Check(tokens, deepEquals, []string{text})
c.Check(rest, equals, "")
- shline.CheckWord(text, false)
+ G.Mk.ForEach(
+ func(mkline MkLine) bool {
+ shline.CheckWord(text, false)
+ return true
+ },
+ func(mkline MkLine) {})
t.CheckOutputLines(
"WARN: fname:1: Unknown shell command \"echo\".")
- shline.CheckShellCommandLine(text)
+ G.Mk.ForEach(
+ func(mkline MkLine) bool {
+ shline.CheckShellCommandLine(text)
+ return true
+ },
+ func(mkline MkLine) {})
// No parse errors
t.CheckOutputLines(
Index: pkgsrc/pkgtools/pkglint/files/vartypecheck.go
diff -u pkgsrc/pkgtools/pkglint/files/vartypecheck.go:1.34 pkgsrc/pkgtools/pkglint/files/vartypecheck.go:1.35
--- pkgsrc/pkgtools/pkglint/files/vartypecheck.go:1.34 Thu Jul 19 06:38:15 2018
+++ pkgsrc/pkgtools/pkglint/files/vartypecheck.go Sat Jul 28 18:31:23 2018
@@ -274,6 +274,14 @@ func (cv *VartypeCheck) Dependency() {
if m, inside := match1(wildcard, `^\[(.*)\]\*$`); m {
if inside != "0-9" {
line.Warnf("Only [0-9]* is allowed in the numeric part of a dependency.")
+ Explain(
+ "The pattern -[0-9] means any version. All other version patterns",
+ "should be expressed using the comparison operators like < or >= or",
+ "even >=2<3.",
+ "",
+ "Patterns like -[0-7] will only match the first digit of the version",
+ "number and will not do the correct thing when the package reaches",
+ "version 10.")
}
} else if m, ver, suffix := match2(wildcard, `^(\d\w*(?:\.\w+)*)(\.\*|\{,nb\*\}|\{,nb\[0-9\]\*\}|\*|)$`); m {
@@ -956,10 +964,12 @@ func (cv *VartypeCheck) Tool() {
switch tooldep {
case "", "bootstrap", "build", "pkgsrc", "run", "test":
default:
- cv.Line.Errorf("Unknown tool dependency %q. Use one of \"bootstrap\", \"build\", \"pkgsrc\" or \"run\" or \"test\".", tooldep)
+ cv.Line.Errorf("Unknown tool dependency %q. Use one of \"bootstrap\", \"build\", \"pkgsrc\", \"run\" or \"test\".", tooldep)
}
- } else if cv.Op != opUseMatch {
- cv.Line.Errorf("Invalid tool syntax: %q.", cv.Value)
+ } else if cv.Op != opUseMatch && cv.Value == cv.ValueNoVar {
+ cv.Line.Errorf("Malformed tool dependency: %q.", cv.Value)
+ Explain(
+ "A tool dependency typically looks like \"sed\" or \"sed:run\".")
}
}
Home |
Main Index |
Thread Index |
Old Index