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: Tue Apr 23 21:20:49 UTC 2019
Modified Files:
pkgsrc/pkgtools/pkglint: Makefile
pkgsrc/pkgtools/pkglint/files: autofix_test.go check_test.go
distinfo.go distinfo_test.go files.go files_test.go
mklinechecker.go mklinechecker_test.go mklines.go
mklines_varalign_test.go mkparser.go mkparser_test.go package.go
package_test.go pkglint.go pkglint_test.go pkgsrc.go shell.go
tools.go tools_test.go vardefs.go
Log Message:
pkgtools/pkglint: update to 5.7.6
Changes since 5.7.5:
* The explanation for distfile hashes is only given when the distfiles
actually need to be downloaded. If they are already there, no
explanation is necessary.
* Makefile lines that are commented and have line continuations are
properly parsed. This affects the autofix for variable value
realignment.
* Variable permissions are not checked in hacks.mk since pkgsrc
developers who know about hacks.mk probably know what they are doing.
From hacks.mk files, builtin.mk files may be included directly, for
the same reason.
* Expressions of the form !empty(PKGPATH:Mpattern), when PKGPATH is not
a list variable and pattern has no wildcards, can be written in a
simpler form, and pkglint autofixes this. For example the above
expression is transformed into ${PKGPATH} == pattern. This
transformation reduces the amount of double negations (!empty) in the
code.
* Duplicate warnings about invalid relative ../package have been merged.
* TOOLS_ALIASES are properly resolved. The line USE_TOOLS=ggrep makes
the tools grep, egrep and fgrep known to pkglint, in the same way as
in the pkgsrc infrastructure.
* The diagnostics for missing or unnecessary distinfo files have been
improved to provide some guidance.
* Packages that use MESSAGE_SRC to build the message from multiple files
no longer produce a warning for malformed message files. These files
are simply skipped.
To generate a diff of this commit:
cvs rdiff -u -r1.575 -r1.576 pkgsrc/pkgtools/pkglint/Makefile
cvs rdiff -u -r1.20 -r1.21 pkgsrc/pkgtools/pkglint/files/autofix_test.go
cvs rdiff -u -r1.38 -r1.39 pkgsrc/pkgtools/pkglint/files/check_test.go \
pkgsrc/pkgtools/pkglint/files/pkglint_test.go \
pkgsrc/pkgtools/pkglint/files/shell.go
cvs rdiff -u -r1.31 -r1.32 pkgsrc/pkgtools/pkglint/files/distinfo.go
cvs rdiff -u -r1.28 -r1.29 pkgsrc/pkgtools/pkglint/files/distinfo_test.go
cvs rdiff -u -r1.24 -r1.25 pkgsrc/pkgtools/pkglint/files/files.go \
pkgsrc/pkgtools/pkglint/files/files_test.go
cvs rdiff -u -r1.34 -r1.35 pkgsrc/pkgtools/pkglint/files/mklinechecker.go
cvs rdiff -u -r1.30 -r1.31 \
pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go
cvs rdiff -u -r1.46 -r1.47 pkgsrc/pkgtools/pkglint/files/mklines.go
cvs rdiff -u -r1.10 -r1.11 \
pkgsrc/pkgtools/pkglint/files/mklines_varalign_test.go
cvs rdiff -u -r1.27 -r1.28 pkgsrc/pkgtools/pkglint/files/mkparser.go \
pkgsrc/pkgtools/pkglint/files/mkparser_test.go
cvs rdiff -u -r1.50 -r1.51 pkgsrc/pkgtools/pkglint/files/package.go
cvs rdiff -u -r1.43 -r1.44 pkgsrc/pkgtools/pkglint/files/package_test.go
cvs rdiff -u -r1.51 -r1.52 pkgsrc/pkgtools/pkglint/files/pkglint.go
cvs rdiff -u -r1.23 -r1.24 pkgsrc/pkgtools/pkglint/files/pkgsrc.go
cvs rdiff -u -r1.12 -r1.13 pkgsrc/pkgtools/pkglint/files/tools.go
cvs rdiff -u -r1.14 -r1.15 pkgsrc/pkgtools/pkglint/files/tools_test.go
cvs rdiff -u -r1.59 -r1.60 pkgsrc/pkgtools/pkglint/files/vardefs.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.575 pkgsrc/pkgtools/pkglint/Makefile:1.576
--- pkgsrc/pkgtools/pkglint/Makefile:1.575 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/Makefile Tue Apr 23 21:20:49 2019
@@ -1,6 +1,6 @@
-# $NetBSD: Makefile,v 1.575 2019/04/20 17:43:24 rillig Exp $
+# $NetBSD: Makefile,v 1.576 2019/04/23 21:20:49 rillig Exp $
-PKGNAME= pkglint-5.7.5
+PKGNAME= pkglint-5.7.6
CATEGORIES= pkgtools
DISTNAME= tools
MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/}
Index: pkgsrc/pkgtools/pkglint/files/autofix_test.go
diff -u pkgsrc/pkgtools/pkglint/files/autofix_test.go:1.20 pkgsrc/pkgtools/pkglint/files/autofix_test.go:1.21
--- pkgsrc/pkgtools/pkglint/files/autofix_test.go:1.20 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/autofix_test.go Tue Apr 23 21:20:49 2019
@@ -897,11 +897,24 @@ func (s *Suite) Test_Autofix_Anyway__aut
fix.Anyway()
fix.Apply()
- // The replacement text is not found, therefore the note should not be logged.
- // Because of fix.Anyway, the note should be logged anyway.
- // But because of the --autofix option, the note should not be logged.
- // Even if the --show-autofix option is explicitly given, the note should not be logged.
- // Therefore, in the end nothing is shown in this case.
+ // The text to be replaced is not found. Because nothing is fixed here,
+ // there's no need to log anything.
+ //
+ // But fix.Anyway is set, therefore the diagnostic should be logged even
+ // though it cannot be fixed automatically. This comes handy in situations
+ // where simple cases can be fixed automatically and more complex cases
+ // (often involving special characters that need to be escaped properly)
+ // should nevertheless result in a diagnostics.
+ //
+ // The --autofix option is set, which means that the diagnostics don't
+ // get logged, only the actual fixes do.
+ //
+ // But then there's also the --show-autofix option, which logs the
+ // corresponding diagnostic for each autofix that actually changes
+ // something. But this autofix doesn't change anything, therefore even
+ // the --show-autofix doesn't have an effect.
+ //
+ // Therefore, in the end nothing is logged in this case.
t.CheckOutputEmpty()
}
Index: pkgsrc/pkgtools/pkglint/files/check_test.go
diff -u pkgsrc/pkgtools/pkglint/files/check_test.go:1.38 pkgsrc/pkgtools/pkglint/files/check_test.go:1.39
--- pkgsrc/pkgtools/pkglint/files/check_test.go:1.38 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/check_test.go Tue Apr 23 21:20:49 2019
@@ -187,7 +187,7 @@ func (t *Tester) SetUpOption(name, descr
}
func (t *Tester) SetUpTool(name, varname string, validity Validity) *Tool {
- return G.Pkgsrc.Tools.def(name, varname, false, validity)
+ return G.Pkgsrc.Tools.def(name, varname, false, validity, nil)
}
// SetUpFileLines creates a temporary file and writes the given lines to it.
@@ -646,6 +646,9 @@ func (t *Tester) FinishSetUp() {
}
// Main runs the pkglint main program with the given command line arguments.
+//
+// Arguments that name existing files or directories in the temporary test
+// directory are transformed to their actual paths.
func (t *Tester) Main(args ...string) int {
if t.seenFinish && !t.seenMain {
t.Errorf("Calling t.FinishSetup() before t.Main() is redundant " +
@@ -659,7 +662,17 @@ func (t *Tester) Main(args ...string) in
G.warnings = 0
G.logged = Once{}
- argv := append([]string{"pkglint"}, args...)
+ argv := []string{"pkglint"}
+ for _, arg := range args {
+ fileArg := t.File(arg)
+ _, err := os.Lstat(fileArg)
+ if err == nil {
+ argv = append(argv, fileArg)
+ } else {
+ argv = append(argv, arg)
+ }
+ }
+
return G.Main(argv...)
}
Index: pkgsrc/pkgtools/pkglint/files/pkglint_test.go
diff -u pkgsrc/pkgtools/pkglint/files/pkglint_test.go:1.38 pkgsrc/pkgtools/pkglint/files/pkglint_test.go:1.39
--- pkgsrc/pkgtools/pkglint/files/pkglint_test.go:1.38 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/pkglint_test.go Tue Apr 23 21:20:49 2019
@@ -561,10 +561,21 @@ func (s *Suite) Test_CheckLinesMessage__
func (s *Suite) Test_CheckLinesMessage__common(c *check.C) {
t := s.Init(c)
- // FIXME: If there is a MESSAGE.common, it is combined with MESSAGE.
- // See meta-pkgs/ruby-redmine-plugins for an example.
+ hline := strings.Repeat("=", 75)
+ t.SetUpPackage("category/package",
+ "MESSAGE_SRC=\t../../category/package/MESSAGE.common",
+ "MESSAGE_SRC+=\t${.CURDIR}/MESSAGE")
+ t.CreateFileLines("category/package/MESSAGE.common",
+ hline,
+ RcsID,
+ "common line")
+ t.CreateFileLines("category/package/MESSAGE",
+ hline)
- t.CheckOutputEmpty()
+ t.Main("category/package")
+
+ t.CheckOutputLines(
+ "Looks fine.")
}
// Demonstrates that an ALTERNATIVES file can be tested individually,
@@ -697,7 +708,7 @@ func (s *Suite) Test_Pkglint_Tool__looku
t := s.Init(c)
mklines := t.NewMkLines("Makefile", MkRcsID)
- G.Pkgsrc.Tools.def("tool", "TOOL", false, Nowhere)
+ G.Pkgsrc.Tools.def("tool", "TOOL", false, Nowhere, nil)
loadTimeTool, loadTimeUsable := G.Tool(mklines, "${TOOL}", LoadTime)
runTimeTool, runTimeUsable := G.Tool(mklines, "${TOOL}", RunTime)
@@ -713,7 +724,7 @@ func (s *Suite) Test_Pkglint_Tool__looku
t := s.Init(c)
mklines := t.NewMkLines("Makefile", MkRcsID)
- G.Pkgsrc.Tools.def("tool", "TOOL", false, AtRunTime)
+ G.Pkgsrc.Tools.def("tool", "TOOL", false, AtRunTime, nil)
loadTimeTool, loadTimeUsable := G.Tool(mklines, "${TOOL}", LoadTime)
runTimeTool, runTimeUsable := G.Tool(mklines, "${TOOL}", RunTime)
@@ -742,7 +753,7 @@ func (s *Suite) Test_Pkglint_ToolByVarna
t := s.Init(c)
mklines := t.NewMkLines("Makefile", MkRcsID)
- G.Pkgsrc.Tools.def("tool", "TOOL", false, AtRunTime)
+ G.Pkgsrc.Tools.def("tool", "TOOL", false, AtRunTime, nil)
c.Check(G.ToolByVarname(mklines, "TOOL").String(), equals, "tool:TOOL::AtRunTime")
}
@@ -955,7 +966,7 @@ func (s *Suite) Test_Pkglint_checkdirPac
t.CheckOutputLines(
"WARN: Makefile: Neither PLIST nor PLIST.common exist, and PLIST_SRC is unset.",
- "WARN: distinfo: File not found. Please run \""+confMake+" makesum\" or define NO_CHECKSUM=yes in the package Makefile.",
+ "WARN: distinfo: A package that downloads files should have a distinfo file.",
"ERROR: Makefile: Each package must define its LICENSE.",
"WARN: Makefile: Each package should define a COMMENT.")
}
@@ -1006,13 +1017,9 @@ func (s *Suite) Test_Pkglint_checkdirPac
G.Check(pkg)
- // FIXME: One of the below warnings is redundant.
t.CheckOutputLines(
- "WARN: ~/category/package/distinfo: File not found. "+
- "Please run \""+confMake+" makesum\" "+
- "or define NO_CHECKSUM=yes in the package Makefile.",
- "WARN: ~/category/package/distinfo: File not found. "+
- "Please run \""+confMake+" makepatchsum\".")
+ "WARN: ~/category/package/distinfo: A package that downloads files should have a distinfo file.",
+ "WARN: ~/category/package/distinfo: A package with patches should have a distinfo file.")
}
func (s *Suite) Test_Pkglint_checkdirPackage__meta_package_without_license(c *check.C) {
@@ -1086,9 +1093,7 @@ func (s *Suite) Test_Pkglint_checkdirPac
G.Check(t.File("category/package"))
t.CheckOutputLines(
- "WARN: ~/category/package/nonexistent: File not found. "+
- "Please run \""+bmake("makesum")+"\" "+
- "or define NO_CHECKSUM=yes in the package Makefile.",
+ "WARN: ~/category/package/nonexistent: A package that downloads files should have a distinfo file.",
"ERROR: ~/category/package/Makefile:20: Relative path \"nonexistent\" does not exist.")
}
Index: pkgsrc/pkgtools/pkglint/files/shell.go
diff -u pkgsrc/pkgtools/pkglint/files/shell.go:1.38 pkgsrc/pkgtools/pkglint/files/shell.go:1.39
--- pkgsrc/pkgtools/pkglint/files/shell.go:1.38 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/shell.go Tue Apr 23 21:20:49 2019
@@ -51,8 +51,7 @@ func (ck *ShellLineChecker) CheckWord(to
// Delegate check for shell words consisting of a single variable use
// to the MkLineChecker. Examples for these are ${VAR:Mpattern} or $@.
- p := NewMkParser(nil, token, false)
- if varuse := p.VarUse(); varuse != nil && p.EOF() {
+ if varuse := ToVarUse(token); varuse != nil {
if ck.checkVarUse {
MkLineChecker{ck.MkLines, ck.mkline}.CheckVaruse(varuse, shellWordVuc)
}
@@ -541,8 +540,7 @@ func (scc *SimpleCommandChecker) handleC
}
shellword := scc.strcmd.Name
- parser := NewMkParser(nil, shellword, false)
- if varuse := parser.VarUse(); varuse != nil && parser.EOF() {
+ if varuse := ToVarUse(shellword); varuse != nil {
varname := varuse.varname
if vartype := G.Pkgsrc.VariableType(scc.MkLines, varname); vartype != nil && vartype.basicType.name == "ShellCommand" {
Index: pkgsrc/pkgtools/pkglint/files/distinfo.go
diff -u pkgsrc/pkgtools/pkglint/files/distinfo.go:1.31 pkgsrc/pkgtools/pkglint/files/distinfo.go:1.32
--- pkgsrc/pkgtools/pkglint/files/distinfo.go:1.31 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/distinfo.go Tue Apr 23 21:20:49 2019
@@ -195,31 +195,32 @@ func (ck *distinfoLinesChecker) checkAlg
distdir := G.Pkgsrc.File("distfiles")
- // It's a rare situation that the explanation is generated
- // this far from the corresponding diagnostic.
- // This explanation only makes sense when there are some
- // hashes missing that can be automatically added by pkglint.
- line.Explain(
- "To add the missing lines to the distinfo file, run",
- sprintf("\t%s", bmake("distinfo")),
- "for each variant of the package until all distfiles are downloaded to",
- "${PKGSRCDIR}/distfiles.",
- "",
- "The variants are typically selected by setting EMUL_PLATFORM",
- "or similar variables in the command line.",
- "",
- "After that, run",
- sprintf("%q", "cvs update -C distinfo"),
- "to revert the distinfo file to the previous state, since the above",
- "commands have removed some of the entries.",
- "",
- "After downloading all possible distfiles, run",
- sprintf("%q,", "pkglint --autofix"),
- "which will find the downloaded distfiles and add the missing",
- "hashes to the distinfo file.")
-
distfile := cleanpath(distdir + "/" + info.filename())
if !fileExists(distfile) {
+
+ // It's a rare situation that the explanation is generated
+ // this far from the corresponding diagnostic.
+ // This explanation only makes sense when there are some
+ // hashes missing that can be automatically added by pkglint.
+ line.Explain(
+ "To add the missing lines to the distinfo file, run",
+ sprintf("\t%s", bmake("distinfo")),
+ "for each variant of the package until all distfiles are downloaded to",
+ "${PKGSRCDIR}/distfiles.",
+ "",
+ "The variants are typically selected by setting EMUL_PLATFORM",
+ "or similar variables in the command line.",
+ "",
+ "After that, run",
+ sprintf("%q", "cvs update -C distinfo"),
+ "to revert the distinfo file to the previous state, since the above",
+ "commands have removed some of the entries.",
+ "",
+ "After downloading all possible distfiles, run",
+ sprintf("%q,", "pkglint --autofix"),
+ "which will find the downloaded distfiles and add the missing",
+ "hashes to the distinfo file.")
+
return
}
Index: pkgsrc/pkgtools/pkglint/files/distinfo_test.go
diff -u pkgsrc/pkgtools/pkglint/files/distinfo_test.go:1.28 pkgsrc/pkgtools/pkglint/files/distinfo_test.go:1.29
--- pkgsrc/pkgtools/pkglint/files/distinfo_test.go:1.28 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/distinfo_test.go Tue Apr 23 21:20:49 2019
@@ -501,10 +501,10 @@ func (s *Suite) Test_distinfoLinesChecke
"ERROR: ~/category/package/distinfo:5: Patch patch-nonexistent does not exist.")
}
-// When there is at least one correct hash for a distfile, running
-// pkglint --autofix adds the missing hashes, provided the distfile has been
-// downloaded to pkgsrc/distfiles, which is the standard distfiles location.
-func (s *Suite) Test_distinfoLinesChecker_checkAlgorithmsDistfile__add_missing_hashes(c *check.C) {
+// When there is at least one correct hash for a distfile and the distfile
+// has already been downloaded to pkgsrc/distfiles, which is the standard
+// distfiles location, running pkglint --autofix adds the missing hashes.
+func (s *Suite) Test_distinfoLinesChecker_checkAlgorithmsDistfile__add_missing_hashes_for_existing_distfile(c *check.C) {
t := s.Init(c)
t.SetUpCommandLine("-Wall", "--explain")
@@ -527,23 +527,6 @@ func (s *Suite) Test_distinfoLinesChecke
"ERROR: ~/category/package/distinfo:3: "+
"Expected SHA1, RMD160, SHA512, Size checksums for \"package-1.0.txt\", "+
"got RMD160, Size, CRC32.",
- "",
- "\tTo add the missing lines to the distinfo file, run",
- "\t\t"+confMake+" distinfo",
- "\tfor each variant of the package until all distfiles are downloaded",
- "\tto ${PKGSRCDIR}/distfiles.",
- "",
- "\tThe variants are typically selected by setting EMUL_PLATFORM or",
- "\tsimilar variables in the command line.",
- "",
- "\tAfter that, run \"cvs update -C distinfo\" to revert the distinfo file",
- "\tto the previous state, since the above commands have removed some of",
- "\tthe entries.",
- "",
- "\tAfter downloading all possible distfiles, run \"pkglint --autofix\",",
- "\twhich will find the downloaded distfiles and add the missing hashes",
- "\tto the distinfo file.",
- "",
"ERROR: ~/category/package/distinfo:3: Missing SHA1 hash for package-1.0.txt.",
"ERROR: ~/category/package/distinfo:3: Missing SHA512 hash for package-1.0.txt.")
@@ -583,6 +566,57 @@ func (s *Suite) Test_distinfoLinesChecke
"got SHA1, RMD160, SHA512, Size, CRC32.")
}
+// When some of the hashes for a distfile are missing, pkglint can calculate
+// them. In order to do this, the distfile needs to be downloaded first. This
+// often requires manual work, otherwise it would have been done already.
+//
+// Since the distfile has not been downloaded in this test case, pkglint can
+// only explain how to download the distfile.
+func (s *Suite) Test_distinfoLinesChecker_checkAlgorithmsDistfile__add_missing_hashes_for_nonexistent_distfile(c *check.C) {
+ t := s.Init(c)
+
+ t.SetUpCommandLine("-Wall", "--explain")
+ t.SetUpPackage("category/package")
+ t.CreateFileLines("category/package/distinfo",
+ RcsID,
+ "",
+ "RMD160 (package-1.0.txt) = 1a88147a0344137404c63f3b695366eab869a98a",
+ "Size (package-1.0.txt) = 13 bytes",
+ "CRC32 (package-1.0.txt) = asdf")
+ t.FinishSetUp()
+
+ G.Check(t.File("category/package"))
+
+ t.CheckOutputLines(
+ "ERROR: ~/category/package/distinfo:3: "+
+ "Expected SHA1, RMD160, SHA512, Size checksums for \"package-1.0.txt\", "+
+ "got RMD160, Size, CRC32.",
+ "",
+ "\tTo add the missing lines to the distinfo file, run",
+ "\t\t"+confMake+" distinfo",
+ "\tfor each variant of the package until all distfiles are downloaded",
+ "\tto ${PKGSRCDIR}/distfiles.",
+ "",
+ "\tThe variants are typically selected by setting EMUL_PLATFORM or",
+ "\tsimilar variables in the command line.",
+ "",
+ "\tAfter that, run \"cvs update -C distinfo\" to revert the distinfo file",
+ "\tto the previous state, since the above commands have removed some of",
+ "\tthe entries.",
+ "",
+ "\tAfter downloading all possible distfiles, run \"pkglint --autofix\",",
+ "\twhich will find the downloaded distfiles and add the missing hashes",
+ "\tto the distinfo file.",
+ "")
+
+ t.SetUpCommandLine("-Wall", "--autofix", "--show-autofix", "--source")
+
+ G.Check(t.File("category/package"))
+
+ // Since the distfile does not exist, pkglint cannot fix anything.
+ t.CheckOutputEmpty()
+}
+
func (s *Suite) Test_distinfoLinesChecker_checkAlgorithmsDistfile__wrong_distfile_hash(c *check.C) {
t := s.Init(c)
Index: pkgsrc/pkgtools/pkglint/files/files.go
diff -u pkgsrc/pkgtools/pkglint/files/files.go:1.24 pkgsrc/pkgtools/pkglint/files/files.go:1.25
--- pkgsrc/pkgtools/pkglint/files/files.go:1.24 Thu Feb 21 22:49:03 2019
+++ pkgsrc/pkgtools/pkglint/files/files.go Tue Apr 23 21:20:49 2019
@@ -2,6 +2,7 @@ package pkglint
import (
"io/ioutil"
+ "netbsd.org/pkglint/textproc"
"path"
"strings"
)
@@ -74,6 +75,7 @@ func nextLogicalLine(filename string, ra
firstlineno := rawLines[index].Lineno
var lineRawLines []*RawLine
interestingRawLines := rawLines[index:]
+ trim := ""
for i, rawLine := range interestingRawLines {
indent, rawText, outdent, cont := matchContinuationLine(rawLine.textnl)
@@ -81,12 +83,14 @@ func nextLogicalLine(filename string, ra
if text.Len() == 0 {
text.WriteString(indent)
}
- text.WriteString(rawText)
+ text.WriteString(strings.TrimPrefix(rawText, trim))
+
lineRawLines = append(lineRawLines, rawLine)
if cont != "" && i != len(interestingRawLines)-1 {
text.WriteString(" ")
index++
+ trim = textproc.NewLexer(rawText).NextString("#")
} else {
text.WriteString(outdent)
text.WriteString(cont)
Index: pkgsrc/pkgtools/pkglint/files/files_test.go
diff -u pkgsrc/pkgtools/pkglint/files/files_test.go:1.24 pkgsrc/pkgtools/pkglint/files/files_test.go:1.25
--- pkgsrc/pkgtools/pkglint/files/files_test.go:1.24 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/files_test.go Tue Apr 23 21:20:49 2019
@@ -98,7 +98,7 @@ func (s *Suite) Test_convertToLogicalLin
t.CheckOutputEmpty()
}
-func (s *Suite) Test_convertToLogicalLines__commented_multi(c *check.C) {
+func (s *Suite) Test_nextLogicalLine__commented_multi(c *check.C) {
t := s.Init(c)
mklines := t.SetUpFileMkLines("filename.mk",
@@ -107,11 +107,9 @@ func (s *Suite) Test_convertToLogicalLin
"#\tcontinuation 2")
mkline := mklines.mklines[0]
- // FIXME: It is more pragmatic to strip the leading comments from the
- // continuation lines as well, so that the variable value is "continuation 1 continuation 2".
- // See nextLogicalLine.
- t.Check(mkline.Value(), equals, "")
- t.Check(mkline.VarassignComment(), equals, "#\tcontinuation 1 #\tcontinuation 2")
+ // The leading comments are stripped from the continuation lines as well.
+ t.Check(mkline.Value(), equals, "continuation 1 \tcontinuation 2")
+ t.Check(mkline.VarassignComment(), equals, "")
}
func (s *Suite) Test_convertToLogicalLines__missing_newline_at_eof(c *check.C) {
Index: pkgsrc/pkgtools/pkglint/files/mklinechecker.go
diff -u pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.34 pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.35
--- pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.34 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/mklinechecker.go Tue Apr 23 21:20:49 2019
@@ -114,10 +114,12 @@ func (ck MkLineChecker) checkInclude() {
mkline.Warnf("Please write \"USE_TOOLS+= intltool\" instead of this line.")
case hasSuffix(includedFile, "/builtin.mk"):
- fix := mkline.Autofix()
- fix.Errorf("%s must not be included directly. Include \"%s/buildlink3.mk\" instead.", includedFile, path.Dir(includedFile))
- fix.Replace("builtin.mk", "buildlink3.mk")
- fix.Apply()
+ if mkline.Basename != "hacks.mk" {
+ fix := mkline.Autofix()
+ fix.Errorf("%s must not be included directly. Include \"%s/buildlink3.mk\" instead.", includedFile, path.Dir(includedFile))
+ fix.Replace("builtin.mk", "buildlink3.mk")
+ fix.Apply()
+ }
}
}
@@ -289,7 +291,13 @@ func (ck MkLineChecker) checkDependencyR
//
// See checkVarusePermissions.
func (ck MkLineChecker) checkVarassignLeftPermissions() {
- if !G.Opts.WarnPerm || G.Infrastructure {
+ if !G.Opts.WarnPerm {
+ return
+ }
+ if G.Infrastructure {
+ // As long as vardefs.go doesn't explicitly define permissions for
+ // infrastructure files, skip the check completely. This avoids
+ // many wrong warnings.
return
}
if trace.Tracing {
@@ -297,6 +305,10 @@ func (ck MkLineChecker) checkVarassignLe
}
mkline := ck.MkLine
+ if ck.MkLine.Basename == "hacks.mk" {
+ return
+ }
+
varname := mkline.Varname()
op := mkline.Op()
vartype := G.Pkgsrc.VariableType(ck.MkLines, varname)
@@ -539,6 +551,12 @@ func (ck MkLineChecker) checkVarusePermi
if !G.Opts.WarnPerm {
return
}
+ if G.Infrastructure {
+ // As long as vardefs.go doesn't explicitly define permissions for
+ // infrastructure files, skip the check completely. This avoids
+ // many wrong warnings.
+ return
+ }
if trace.Tracing {
defer trace.Call(varname, vuc)()
}
@@ -555,6 +573,10 @@ func (ck MkLineChecker) checkVarusePermi
}
mkline := ck.MkLine
+ if mkline.Basename == "hacks.mk" {
+ return
+ }
+
effPerms := vartype.EffectivePermissions(mkline.Basename)
if effPerms.Contains(aclpUseLoadtime) {
// Skip any checks, assuming that if a variable may be used at
@@ -1313,53 +1335,52 @@ func (ck MkLineChecker) checkDirectiveCo
return
}
- checkCompareVarStr := func(varuse *MkVarUse, op string, str string) {
- varname := varuse.varname
- varmods := varuse.modifiers
- switch len(varmods) {
- case 0:
- ck.checkCompareVarStr(varname, op, str)
-
- case 1:
- if m, _, pattern := varmods[0].MatchMatch(); m {
- ck.checkVartype(varname, opUseMatch, pattern, "")
-
- // After applying the :M or :N modifier, every expression may end up empty,
- // regardless of its data type. Therefore there's no point in type-checking that case.
- if str != "" {
- ck.checkVartype(varname, opUseCompare, str, "")
- }
- }
-
- default:
- // This case covers ${VAR:Mfilter:O:u} or similar uses in conditions.
- // To check these properly, pkglint first needs to know the most common
- // modifiers and how they interact.
- // As of March 2019, the modifiers are not modeled.
- // The following tracing statement makes it easy to discover these cases,
- // in order to decide whether checking them is worthwhile.
- if trace.Tracing {
- trace.Stepf("checkCompareVarStr ${%s%s} %s %s",
- varuse.varname, varuse.Mod(), op, str)
- }
- }
- }
-
checkVarUse := func(varuse *MkVarUse) {
var vartype *Vartype // TODO: Insert a better type guess here.
vuc := VarUseContext{vartype, vucTimeParse, VucQuotPlain, false}
ck.CheckVaruse(varuse, &vuc)
}
+ // Skip subconditions that have already been handled as part of the !(...).
+ done := make(map[interface{}]bool)
+
+ checkNotEmpty := func(not MkCond) {
+ empty := not.Empty
+ if empty != nil {
+ ck.checkDirectiveCondEmpty(empty, true, true, not == cond.Not)
+ done[empty] = true
+ }
+
+ varUse := not.Var
+ if varUse != nil {
+ ck.checkDirectiveCondEmpty(varUse, false, false, not == cond.Not)
+ done[varUse] = true
+ }
+ }
+
+ checkEmpty := func(empty *MkVarUse) {
+ if !done[empty] {
+ ck.checkDirectiveCondEmpty(empty, true, false, empty == cond.Empty)
+ }
+ }
+
+ checkVar := func(varUse *MkVarUse) {
+ if !done[varUse] {
+ ck.checkDirectiveCondEmpty(varUse, false, true, varUse == cond.Var)
+ }
+ }
+
cond.Walk(&MkCondCallback{
- Empty: ck.checkDirectiveCondEmpty,
- Var: ck.checkDirectiveCondEmpty,
- CompareVarStr: checkCompareVarStr,
+ Not: checkNotEmpty,
+ Empty: checkEmpty,
+ Var: checkVar,
+ CompareVarStr: ck.checkDirectiveCondCompareVarStr,
VarUse: checkVarUse})
}
-// checkDirectiveCondEmpty checks a condition of the form empty(...) in an .if directive.
-func (ck MkLineChecker) checkDirectiveCondEmpty(varuse *MkVarUse) {
+// checkDirectiveCondEmpty checks a condition of the form empty(VAR),
+// empty(VAR:Mpattern) or ${VAR:Mpattern} in an .if directive.
+func (ck MkLineChecker) checkDirectiveCondEmpty(varuse *MkVarUse, fromEmpty bool, notEmpty bool, toplevel bool) {
varname := varuse.varname
if matches(varname, `^\$.*:[MN]`) {
ck.MkLine.Warnf("The empty() function takes a variable name as parameter, not a variable expression.")
@@ -1375,26 +1396,75 @@ func (ck MkLineChecker) checkDirectiveCo
"\t${VARNAME:Mpattern}")
}
+ ck.simplifyCondition(varuse, fromEmpty, notEmpty, toplevel)
+}
+
+// simplifyCondition replaces an unnecessarily complex condition with
+// a simpler condition that's still equivalent.
+//
+// * fromEmpty is true for the form empty(VAR...), and false for ${VAR...}.
+//
+// * notEmpty is true for the form !empty(VAR...), and false for empty(VAR...).
+// It also applies to the ${VAR} form.
+//
+// * toplevel is true for ${VAR...} and false for ${VAR...} && ${VAR2...}.
+func (ck MkLineChecker) simplifyCondition(varuse *MkVarUse, fromEmpty bool, notEmpty bool, toplevel bool) {
+
+ // replace constructs the state before and after the autofix.
+ // The before state is constructed to ensure that only very simple
+ // patterns get replaced automatically.
+ //
+ // Before putting any cases involving special characters into
+ // production, there need to be more tests for the edge cases.
+ replace := func(varname string, m bool, pattern string) (string, string) {
+ op := ifelseStr(notEmpty == m, "==", "!=")
+
+ from := "" +
+ ifelseStr(notEmpty != fromEmpty, "", "!") +
+ ifelseStr(fromEmpty, "empty(", "${") +
+ varname +
+ ifelseStr(m, ":M", ":N") +
+ pattern +
+ ifelseStr(fromEmpty, ")", "}")
+
+ to := "${" + varname + "} " + op + " " + pattern
+
+ // TODO: Check in more cases whether the parentheses are really necessary.
+ // In a !!${VAR} expression, parentheses are necessary.
+ needParen := !toplevel
+ if needParen {
+ to = "(" + to + ")"
+ }
+
+ return from, to
+ }
+
+ varname := varuse.varname
modifiers := varuse.modifiers
+
for _, modifier := range modifiers {
if m, positive, pattern := modifier.MatchMatch(); m && (positive || len(modifiers) == 1) {
ck.checkVartype(varname, opUseMatch, pattern, "")
vartype := G.Pkgsrc.VariableType(ck.MkLines, varname)
if matches(pattern, `^[\w-/]+$`) && vartype != nil && !vartype.List() {
- ck.MkLine.Notef("%s should be compared using %s instead of matching against %q.",
- varname, ifelseStr(positive, "==", "!="), ":"+modifier.Text)
- ck.MkLine.Explain(
+
+ fix := ck.MkLine.Autofix()
+ fix.Notef("%s should be compared using %s instead of matching against %q.",
+ varname, ifelseStr(positive == notEmpty, "==", "!="), ":"+modifier.Text)
+ fix.Explain(
"This variable has a single value, not a list of values.",
"Therefore it feels strange to apply list operators like :M and :N onto it.",
"A more direct approach is to use the == and != operators.",
"",
"An entirely different case is when the pattern contains wildcards like ^, *, $.",
"In such a case, using the :M or :N modifiers is useful and preferred.")
+ fix.Replace(replace(varname, positive, pattern))
+ fix.Anyway()
+ fix.Apply()
}
}
}
-
}
func (ck MkLineChecker) checkCompareVarStr(varname, op, value string) {
@@ -1408,6 +1478,38 @@ func (ck MkLineChecker) checkCompareVarS
}
}
+func (ck MkLineChecker) checkDirectiveCondCompareVarStr(varuse *MkVarUse, op string, str string) {
+ varname := varuse.varname
+ varmods := varuse.modifiers
+ switch len(varmods) {
+ case 0:
+ ck.checkCompareVarStr(varname, op, str)
+
+ case 1:
+ if m, _, pattern := varmods[0].MatchMatch(); m {
+ ck.checkVartype(varname, opUseMatch, pattern, "")
+
+ // After applying the :M or :N modifier, every expression may end up empty,
+ // regardless of its data type. Therefore there's no point in type-checking that case.
+ if str != "" {
+ ck.checkVartype(varname, opUseCompare, str, "")
+ }
+ }
+
+ default:
+ // This case covers ${VAR:Mfilter:O:u} or similar uses in conditions.
+ // To check these properly, pkglint first needs to know the most common
+ // modifiers and how they interact.
+ // As of March 2019, the modifiers are not modeled.
+ // The following tracing statement makes it easy to discover these cases,
+ // in order to decide whether checking them is worthwhile.
+ if trace.Tracing {
+ trace.Stepf("checkCompareVarStr ${%s%s} %s %s",
+ varuse.varname, varuse.Mod(), op, str)
+ }
+ }
+}
+
// CheckRelativePkgdir checks a reference from one pkgsrc package to another.
// These references should always have the form ../../category/package.
//
@@ -1468,17 +1570,25 @@ func (ck MkLineChecker) CheckRelativePat
}
switch {
- case !hasPrefix(relativePath, "../"):
+ case !hasPrefix(resolvedPath, "../"):
break
- case hasPrefix(relativePath, "../../mk/"):
+
+ case hasPrefix(resolvedPath, "../../mk/"):
// From a package to the infrastructure.
- case matches(relativePath, `^\.\./\.\./[^/]+/[^/]`):
+
+ case matches(resolvedPath, `^\.\./\.\./[^./][^/]*/[^/]`):
// From a package to another package.
- case hasPrefix(relativePath, "../mk/") && relpath(path.Dir(mkline.Filename), G.Pkgsrc.File(".")) == "..":
+
+ case hasPrefix(resolvedPath, "../mk/") && relpath(path.Dir(mkline.Filename), G.Pkgsrc.File(".")) == "..":
// For category Makefiles.
// TODO: Or from a pkgsrc wip package to wip/mk.
- default:
- mkline.Warnf("Invalid relative path %q.", relativePath)
- // TODO: Explain this warning.
+
+ case matches(resolvedPath, `^\.\./[^./][^/]*/[^/]`):
+ if G.Wip && contains(resolvedPath, "/mk/") {
+ mkline.Warnf("References to the pkgsrc-wip infrastructure should look like \"../../wip/mk\", not \"../mk\".")
+ } else {
+ mkline.Warnf("References to other packages should look like \"../../category/package\", not \"../package\".")
+ }
+ mkline.ExplainRelativeDirs()
}
}
Index: pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go:1.30 pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go:1.31
--- pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go:1.30 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go Tue Apr 23 21:20:49 2019
@@ -194,6 +194,44 @@ func (s *Suite) Test_MkLineChecker_check
"ERROR: ~/category/package/Makefile:20: Cannot read \"../../other/existing/Makefile\".")
}
+func (s *Suite) Test_MkLineChecker_checkInclude__hacks(c *check.C) {
+ t := s.Init(c)
+
+ t.SetUpPackage("category/package")
+ t.CreateFileLines("category/package/hacks.mk",
+ MkRcsID,
+ ".include \"../../category/package/nonexistent.mk\"",
+ ".include \"../../category/package/builtin.mk\"")
+ t.CreateFileLines("category/package/builtin.mk",
+ MkRcsID)
+ t.FinishSetUp()
+
+ G.checkdirPackage(t.File("category/package"))
+
+ // The purpose of this "nonexistent" diagnostic is only to show that
+ // hacks.mk is indeed parsed and checked.
+ t.CheckOutputLines(
+ "ERROR: ~/category/package/hacks.mk:2: " +
+ "Relative path \"../../category/package/nonexistent.mk\" does not exist.")
+}
+
+func (s *Suite) Test_MkLineChecker__permissions_in_hacks_mk(c *check.C) {
+ t := s.Init(c)
+
+ t.SetUpVartypes()
+
+ mklines := t.NewMkLines("hacks.mk",
+ MkRcsID,
+ "OPSYS=\t${PKGREVISION}")
+
+ mklines.Check()
+
+ // No matter how strange the definition or use of a variable sounds,
+ // in hacks.mk it is allowed. Special problems sometimes need solutions
+ // that violate all standards.
+ t.CheckOutputEmpty()
+}
+
func (s *Suite) Test_MkLineChecker_checkDirective(c *check.C) {
t := s.Init(c)
@@ -1367,31 +1405,166 @@ func (s *Suite) Test_MkLineChecker_check
t := s.Init(c)
t.SetUpVartypes()
- mkline := t.NewMkLine("module.mk", 123, ".if ${PKGPATH} == \"category/package\"")
- ck := MkLineChecker{nil, mkline}
+ t.Chdir(".")
- ck.checkDirectiveCondEmpty(NewMkVarUse("PKGPATH", "Mpattern"))
+ test := func(before string, diagnosticsAndAfter ...string) {
- // When the pattern contains placeholders, it cannot be converted to == or !=.
- ck.checkDirectiveCondEmpty(NewMkVarUse("PKGPATH", "Mpa*n"))
+ mklines := t.SetUpFileMkLines("module.mk",
+ MkRcsID,
+ before,
+ ".endif")
+ mkline := mklines.mklines[1]
+ ck := MkLineChecker{nil, mkline}
+
+ t.SetUpCommandLine("-Wall")
+ ck.checkDirectiveCond()
+
+ t.SetUpCommandLine("-Wall", "--autofix")
+ ck.checkDirectiveCond()
+
+ mklines.SaveAutofixChanges()
+ afterMklines := t.LoadMkInclude("module.mk")
+
+ if len(diagnosticsAndAfter) > 0 {
+ diagLen := len(diagnosticsAndAfter)
+ diagnostics := diagnosticsAndAfter[:diagLen-1]
+ after := diagnosticsAndAfter[diagLen-1]
+
+ t.CheckOutput(diagnostics)
+ t.Check(afterMklines.mklines[1].Text, equals, after)
+ } else {
+ t.CheckOutputEmpty()
+ }
+ }
- ck.checkDirectiveCondEmpty(NewMkVarUse("PKGPATH", "tl", "Mpattern"))
+ test(
+ ".if ${PKGPATH:Mpattern}",
+ "NOTE: module.mk:2: PKGPATH should be compared using == instead of matching against \":Mpattern\".",
+ "AUTOFIX: module.mk:2: Replacing \"${PKGPATH:Mpattern}\" with \"${PKGPATH} == pattern\".",
+ ".if ${PKGPATH} == pattern")
- ck.checkDirectiveCondEmpty(NewMkVarUse("PKGPATH", "Ncategory/package"))
+ // When the pattern contains placeholders, it cannot be converted to == or !=.
+ test(
+ ".if ${PKGPATH:Mpa*n}",
+ nil...)
+
+ // The :tl modifier prevents the autofix.
+ test(
+ ".if ${PKGPATH:tl:Mpattern}",
+ "NOTE: module.mk:2: PKGPATH should be compared using == instead of matching against \":Mpattern\".",
+ ".if ${PKGPATH:tl:Mpattern}")
- // ${PKGPATH:None:Ntwo} is a short variant of ${PKGPATH} != "one" && ${PKGPATH} != "two",
- // therefore no note is logged in this case.
- ck.checkDirectiveCondEmpty(NewMkVarUse("PKGPATH", "None", "Ntwo"))
+ test(
+ ".if ${PKGPATH:Ncategory/package}",
+ "NOTE: module.mk:2: PKGPATH should be compared using != instead of matching against \":Ncategory/package\".",
+ "AUTOFIX: module.mk:2: Replacing \"${PKGPATH:Ncategory/package}\" with \"${PKGPATH} != category/package\".",
+ ".if ${PKGPATH} != category/package")
+
+ // ${PKGPATH:None:Ntwo} is a short variant of ${PKGPATH} != "one" &&
+ // ${PKGPATH} != "two". Applying the transformation would make the
+ // condition longer than before, therefore nothing is done here.
+ test(
+ ".if ${PKGPATH:None:Ntwo}",
+ nil...)
// Note: this combination doesn't make sense since the patterns "one" and "two" don't overlap.
- ck.checkDirectiveCondEmpty(NewMkVarUse("PKGPATH", "Mone", "Mtwo"))
+ test(".if ${PKGPATH:Mone:Mtwo}",
+ "NOTE: module.mk:2: PKGPATH should be compared using == instead of matching against \":Mone\".",
+ "NOTE: module.mk:2: PKGPATH should be compared using == instead of matching against \":Mtwo\".",
+ ".if ${PKGPATH:Mone:Mtwo}")
+
+ test(".if !empty(PKGPATH:Mpattern)",
+ "NOTE: module.mk:2: PKGPATH should be compared using == instead of matching against \":Mpattern\".",
+ "AUTOFIX: module.mk:2: Replacing \"!empty(PKGPATH:Mpattern)\" with \"${PKGPATH} == pattern\".",
+ ".if ${PKGPATH} == pattern")
+
+ test(".if empty(PKGPATH:Mpattern)",
+ "NOTE: module.mk:2: PKGPATH should be compared using != instead of matching against \":Mpattern\".",
+ "AUTOFIX: module.mk:2: Replacing \"empty(PKGPATH:Mpattern)\" with \"${PKGPATH} != pattern\".",
+ ".if ${PKGPATH} != pattern")
+
+ test(".if !!empty(PKGPATH:Mpattern)",
+ // TODO: When taking all the ! into account, this is actually a
+ // test for emptiness, therefore the diagnostics should suggest
+ // the != operator instead of ==.
+ "NOTE: module.mk:2: PKGPATH should be compared using == instead of matching against \":Mpattern\".",
+ "AUTOFIX: module.mk:2: Replacing \"!empty(PKGPATH:Mpattern)\" with \"(${PKGPATH} == pattern)\".",
+ // TODO: This condition could be simplified even more.
+ // Luckily the !! pattern doesn't occur in practice.
+ ".if !(${PKGPATH} == pattern)")
- t.CheckOutputLines(
- "NOTE: module.mk:123: PKGPATH should be compared using == instead of matching against \":Mpattern\".",
- "NOTE: module.mk:123: PKGPATH should be compared using == instead of matching against \":Mpattern\".",
- "NOTE: module.mk:123: PKGPATH should be compared using != instead of matching against \":Ncategory/package\".",
- "NOTE: module.mk:123: PKGPATH should be compared using == instead of matching against \":Mone\".",
- "NOTE: module.mk:123: PKGPATH should be compared using == instead of matching against \":Mtwo\".")
+ // No note in this case since there is no implicit !empty around the varUse.
+ test(".if ${PKGPATH:Mpattern} != ${OTHER}",
+ nil...)
+
+ test(
+ ".if ${PKGPATH:Mpattern}",
+ "NOTE: module.mk:2: PKGPATH should be compared using == instead of matching against \":Mpattern\".",
+ "AUTOFIX: module.mk:2: Replacing \"${PKGPATH:Mpattern}\" with \"${PKGPATH} == pattern\".",
+ ".if ${PKGPATH} == pattern")
+
+ test(
+ ".if !${PKGPATH:Mpattern}",
+ "NOTE: module.mk:2: PKGPATH should be compared using != instead of matching against \":Mpattern\".",
+ "AUTOFIX: module.mk:2: Replacing \"!${PKGPATH:Mpattern}\" with \"${PKGPATH} != pattern\".",
+ ".if ${PKGPATH} != pattern")
+
+ // This pattern with spaces doesn't make sense at all in the :M
+ // modifier since it can never match.
+ test(
+ ".if ${PKGPATH:Mpattern with spaces}",
+ nil...)
+ // TODO: ".if ${PKGPATH} == \"pattern with spaces\"")
+
+ test(
+ ".if ${PKGPATH:M'pattern with spaces'}",
+ nil...)
+ // TODO: ".if ${PKGPATH} == 'pattern with spaces'")
+
+ test(
+ ".if ${PKGPATH:M&&}",
+ nil...)
+ // TODO: ".if ${PKGPATH} == '&&'")
+
+ // If PKGPATH is "", the condition is false.
+ // If PKGPATH is "negative-pattern", the condition is false.
+ // In all other cases, the condition is true.
+ //
+ // Therefore this condition cannot simply be transformed into
+ // ${PKGPATH} != negative-pattern, since that would produce a
+ // different result in the case where PKGPATH is empty.
+ //
+ // For system-provided variables that are guaranteed to be non-empty,
+ // such as OPSYS or PKGPATH, this replacement is valid.
+ // These variables are only guaranteed to be defined after bsd.prefs.mk
+ // has been included, like everywhere else.
+ test(
+ ".if ${PKGPATH:Nnegative-pattern}",
+ "NOTE: module.mk:2: PKGPATH should be compared using != instead of matching against \":Nnegative-pattern\".",
+ "AUTOFIX: module.mk:2: Replacing \"${PKGPATH:Nnegative-pattern}\" with \"${PKGPATH} != negative-pattern\".",
+ ".if ${PKGPATH} != negative-pattern")
+
+ // Since UNKNOWN is not a well-known system-provided variable that is
+ // guaranteed to be non-empty (see the previous example), it is not
+ // transformed at all.
+ test(
+ ".if ${UNKNOWN:Nnegative-pattern}",
+ nil...)
+
+ test(
+ ".if ${PKGPATH:Mpath1} || ${PKGPATH:Mpath2}",
+ "NOTE: module.mk:2: PKGPATH should be compared using == instead of matching against \":Mpath1\".",
+ "NOTE: module.mk:2: PKGPATH should be compared using == instead of matching against \":Mpath2\".",
+ "AUTOFIX: module.mk:2: Replacing \"${PKGPATH:Mpath1}\" with \"(${PKGPATH} == path1)\".",
+ "AUTOFIX: module.mk:2: Replacing \"${PKGPATH:Mpath2}\" with \"(${PKGPATH} == path2)\".",
+ // TODO: remove the redundant parentheses
+ ".if (${PKGPATH} == path1) || (${PKGPATH} == path2)")
+
+ test(
+ ".if (((((${PKGPATH:Mpath})))))",
+ "NOTE: module.mk:2: PKGPATH should be compared using == instead of matching against \":Mpath\".",
+ "AUTOFIX: module.mk:2: Replacing \"${PKGPATH:Mpath}\" with \"${PKGPATH} == path\".",
+ ".if (((((${PKGPATH} == path)))))")
}
func (s *Suite) Test_MkLineChecker_checkDirectiveCond__comparing_PKGSRC_COMPILER_with_eqeq(c *check.C) {
@@ -1731,8 +1904,7 @@ func (s *Suite) Test_MkLineChecker_Check
// There is no warning about DEFAULT_PREFIX being "defined but not used"
// since Pkgsrc.loadUntypedVars calls Pkgsrc.vartypes.DefineType, which
// registers that variable globally.
- t.CheckOutputLines(
- "WARN: ~/mk/infra.mk:2: PREFIX should not be used indirectly at load time (via LOCALBASE).")
+ t.CheckOutputEmpty()
}
func (s *Suite) Test_MkLineChecker_CheckVaruse__user_defined_variable_and_BUILD_DEFS(c *check.C) {
@@ -1973,7 +2145,10 @@ func (s *Suite) Test_MkLineChecker_Check
".include \"module.mk\"",
".include \"../../category/../category/package/module.mk\"", // Oops
".include \"../../mk/bsd.prefs.mk\"",
- ".include \"../package/module.mk\"")
+ ".include \"../package/module.mk\"",
+ // TODO: warn about this as well, since ${.CURDIR} is essentially
+ // equivalent to ".".
+ ".include \"${.CURDIR}/../package/module.mk\"")
t.FinishSetUp()
mklines.Check()
@@ -1982,8 +2157,10 @@ func (s *Suite) Test_MkLineChecker_Check
"ERROR: ~/category/package/module.mk:2: A main pkgsrc package must not depend on a pkgsrc-wip package.",
"ERROR: ~/category/package/module.mk:3: A main pkgsrc package must not depend on a pkgsrc-wip package.",
"WARN: ~/category/package/module.mk:5: LATEST_PYTHON is used but not defined.",
- // TODO: This warning is unspecific, there is also a pkglint warning "should be ../../category/package".
- "WARN: ~/category/package/module.mk:11: Invalid relative path \"../package/module.mk\".")
+ "WARN: ~/category/package/module.mk:11: References to other packages should "+
+ "look like \"../../category/package\", not \"../package\".",
+ "WARN: ~/category/package/module.mk:12: References to other packages should "+
+ "look like \"../../category/package\", not \"../package\".")
}
func (s *Suite) Test_MkLineChecker_CheckRelativePath__absolute_path(c *check.C) {
@@ -2032,8 +2209,7 @@ func (s *Suite) Test_MkLineChecker_Check
G.Check(t.File("wip/package"))
t.CheckOutputLines(
- "WARN: ~/wip/package/Makefile:20: "+
- "References to the pkgsrc-wip infrastructure should look like \"../../wip/mk\", "+
- "not \"../mk\".",
- "WARN: ~/wip/package/Makefile:20: Invalid relative path \"../mk/git-package.mk\".")
+ "WARN: ~/wip/package/Makefile:20: " +
+ "References to the pkgsrc-wip infrastructure should look like \"../../wip/mk\", " +
+ "not \"../mk\".")
}
Index: pkgsrc/pkgtools/pkglint/files/mklines.go
diff -u pkgsrc/pkgtools/pkglint/files/mklines.go:1.46 pkgsrc/pkgtools/pkglint/files/mklines.go:1.47
--- pkgsrc/pkgtools/pkglint/files/mklines.go:1.46 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/mklines.go Tue Apr 23 21:20:49 2019
@@ -129,7 +129,7 @@ func (mklines *MkLinesImpl) checkAll() {
ck.Check()
varalign.Process(mkline)
- mklines.Tools.ParseToolLine(mkline, false, false)
+ mklines.Tools.ParseToolLine(mklines, mkline, false, false)
switch {
case mkline.IsEmpty():
@@ -229,16 +229,44 @@ func (mklines *MkLinesImpl) ForEachEnd(a
mklines.indentation = nil
}
+// ExpandLoopVar searches the surrounding .for loops for the given
+// variable and returns a slice containing all its values, fully
+// expanded.
+//
+// It can only be used during a active ForEach call.
+func (mklines *MkLinesImpl) ExpandLoopVar(varname string) []string {
+
+ // From the inner loop to the outer loop, just in case
+ // that two loops should ever use the same variable.
+ for i := len(mklines.indentation.levels) - 1; i >= 0; i-- {
+ ind := mklines.indentation.levels[i]
+
+ mkline := ind.mkline
+ if mkline == nil || !mkline.IsDirective() || mkline.Directive() != "for" {
+ continue
+ }
+
+ // TODO: If needed, add support for multi-variable .for loops.
+ resolved := resolveVariableRefs(mklines, mkline.Args())
+ words := mkline.ValueFields(resolved)
+ if 1 < len(words) && words[0] == varname && words[1] == "in" {
+ return words[2:]
+ }
+ }
+
+ return nil
+}
+
func (mklines *MkLinesImpl) collectDefinedVariables() {
if trace.Tracing {
defer trace.Call0()()
}
- for _, mkline := range mklines.mklines {
- mklines.Tools.ParseToolLine(mkline, false, true)
+ mklines.ForEach(func(mkline MkLine) {
+ mklines.Tools.ParseToolLine(mklines, mkline, false, true)
if !mkline.IsVarassign() {
- continue
+ return
}
mklines.defineVar(G.Pkg, mkline, mkline.Varname())
@@ -291,7 +319,7 @@ func (mklines *MkLinesImpl) collectDefin
mklines.defineVar(G.Pkg, mkline, opsysVar)
}
}
- }
+ })
}
// defineVar marks a variable as defined in both the current package and the current file.
Index: pkgsrc/pkgtools/pkglint/files/mklines_varalign_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mklines_varalign_test.go:1.10 pkgsrc/pkgtools/pkglint/files/mklines_varalign_test.go:1.11
--- pkgsrc/pkgtools/pkglint/files/mklines_varalign_test.go:1.10 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/mklines_varalign_test.go Tue Apr 23 21:20:49 2019
@@ -934,18 +934,16 @@ func (s *Suite) Test_Varalign__realign_c
"SHORT=\tvalue")
vt.Diagnostics(
"NOTE: ~/Makefile:1: This variable value should be aligned to column 17.",
- "NOTE: ~/Makefile:3--4: This variable value should be aligned with tabs, not spaces, to column 17.",
"NOTE: ~/Makefile:5--6: This line should be aligned with \"\\t\\t\".",
"NOTE: ~/Makefile:7: This variable value should be aligned to column 17.")
vt.Autofixes(
"AUTOFIX: ~/Makefile:1: Replacing \"\\t\" with \"\\t\\t\".",
- "AUTOFIX: ~/Makefile:3: Replacing \" \" with \"\\t\".",
"AUTOFIX: ~/Makefile:6: Replacing indentation \"\" with \"\\t\\t\".",
"AUTOFIX: ~/Makefile:7: Replacing \"\\t\" with \"\\t\\t\".")
vt.Fixed(
"SHORT= value",
"#DISTFILES= distfile-1.0.0.tar.gz",
- "#CONTINUATION= \\",
+ "#CONTINUATION= \\",
"# continued",
"#CONFIGURE_ENV+= \\",
"# TZ=UTC",
Index: pkgsrc/pkgtools/pkglint/files/mkparser.go
diff -u pkgsrc/pkgtools/pkglint/files/mkparser.go:1.27 pkgsrc/pkgtools/pkglint/files/mkparser.go:1.28
--- pkgsrc/pkgtools/pkglint/files/mkparser.go:1.27 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/mkparser.go Tue Apr 23 21:20:49 2019
@@ -744,7 +744,7 @@ func (p *MkParser) Dependency() *Depende
return &dp
}
- if pkgbaseParser := NewMkParser(nil, dp.Pkgbase, false); pkgbaseParser.VarUse() != nil && pkgbaseParser.EOF() {
+ if ToVarUse(dp.Pkgbase) != nil {
return &dp
}
@@ -758,6 +758,18 @@ func (p *MkParser) Dependency() *Depende
return nil
}
+// ToVarUse converts the given string into a MkVarUse, or returns nil
+// if there is a parse error or some trailing text.
+// Parse errors are silently ignored.
+func ToVarUse(str string) *MkVarUse {
+ p := NewMkParser(nil, str, false)
+ varUse := p.VarUse()
+ if varUse == nil || !p.EOF() {
+ return nil
+ }
+ return varUse
+}
+
// MkCond is a condition in a Makefile, such as ${OPSYS} == NetBSD.
//
// The representation is somewhere between syntactic and semantic.
@@ -801,6 +813,7 @@ type MkCondCall struct {
}
type MkCondCallback struct {
+ Not func(cond MkCond)
Defined func(varname string)
Empty func(empty *MkVarUse)
CompareVarNum func(varuse *MkVarUse, op string, num string)
@@ -830,6 +843,9 @@ func (w *MkCondWalker) Walk(cond MkCond,
}
case cond.Not != nil:
+ if callback.Not != nil {
+ callback.Not(cond.Not)
+ }
w.Walk(cond.Not, callback)
case cond.Defined != "":
Index: pkgsrc/pkgtools/pkglint/files/mkparser_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mkparser_test.go:1.27 pkgsrc/pkgtools/pkglint/files/mkparser_test.go:1.28
--- pkgsrc/pkgtools/pkglint/files/mkparser_test.go:1.27 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/mkparser_test.go Tue Apr 23 21:20:49 2019
@@ -587,8 +587,8 @@ func (s *Suite) Test_MkParser_VarUseModi
varUse := NewMkVarUse
test := func(text string, varUse *MkVarUse, diagnostics ...string) {
- mkline := t.NewMkLine("Makefile", 20, "\t"+text)
- p := NewMkParser(mkline.Line, mkline.ShellCommand(), true)
+ line := t.NewLine("Makefile", 20, "\t"+text)
+ p := NewMkParser(line, text, true)
actual := p.VarUse()
@@ -604,13 +604,9 @@ func (s *Suite) Test_MkParser_VarUseModi
test("${VAR:!command!}", varUse("VAR", "!command!"))
test("${VAR:!command}", varUse("VAR"),
- // FIXME: duplicate diagnostic
- "WARN: Makefile:20: Invalid variable modifier \"!command\" for \"VAR\".",
"WARN: Makefile:20: Invalid variable modifier \"!command\" for \"VAR\".")
test("${VAR:command!}", varUse("VAR"),
- // FIXME: duplicate diagnostic
- "WARN: Makefile:20: Invalid variable modifier \"command!\" for \"VAR\".",
"WARN: Makefile:20: Invalid variable modifier \"command!\" for \"VAR\".")
// The :L modifier makes the variable value "echo hello", and the :[1]
Index: pkgsrc/pkgtools/pkglint/files/package.go
diff -u pkgsrc/pkgtools/pkglint/files/package.go:1.50 pkgsrc/pkgtools/pkglint/files/package.go:1.51
--- pkgsrc/pkgtools/pkglint/files/package.go:1.50 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/package.go Tue Apr 23 21:20:49 2019
@@ -258,8 +258,11 @@ func (pkg *Package) check(files []string
if pkg.Pkgdir == "." {
if havePatches && !haveDistinfo {
- // TODO: Add Line.RefTo to make the context clear.
- NewLineWhole(pkg.File(pkg.DistinfoFile)).Warnf("File not found. Please run %q.", bmake("makepatchsum"))
+ line := NewLineWhole(pkg.File(pkg.DistinfoFile))
+ line.Warnf("A package with patches should have a distinfo file.")
+ line.Explain(
+ "To generate a distinfo file for the existing patches, run",
+ sprintf("%q.", bmake("makepatchsum")))
}
}
}
@@ -290,8 +293,8 @@ func (pkg *Package) loadPackageMakefile(
// See mk/tools/cmake.mk
if pkg.vars.Defined("USE_CMAKE") {
- mainLines.Tools.def("cmake", "", false, AtRunTime)
- mainLines.Tools.def("cpack", "", false, AtRunTime)
+ mainLines.Tools.def("cmake", "", false, AtRunTime, nil)
+ mainLines.Tools.def("cpack", "", false, AtRunTime, nil)
}
allLines.collectUsedVariables()
@@ -362,15 +365,6 @@ func (pkg *Package) readMakefile(filenam
return unknown
}
- if matches(includedFile, `^\.\./[^./][^/]*/[^/]+`) {
- if G.Wip && contains(includedFile, "/mk/") {
- mkline.Warnf("References to the pkgsrc-wip infrastructure should look like \"../../wip/mk\", not \"../mk\".")
- } else {
- mkline.Warnf("References to other packages should look like \"../../category/package\", not \"../package\".")
- }
- mkline.ExplainRelativeDirs()
- }
-
pkg.collectUsedBy(mkline, incDir, incBase, includedFile)
if trace.Tracing {
@@ -548,16 +542,23 @@ func (pkg *Package) checkfilePackageMake
NewLineWhole(filename).Warnf("Neither PLIST nor PLIST.common exist, and PLIST_SRC is unset.")
}
- if (vars.Defined("NO_CHECKSUM") ||
- vars.Defined("META_PACKAGE")) && isEmptyDir(pkg.File(pkg.Patchdir)) {
+ if (vars.Defined("NO_CHECKSUM") || vars.Defined("META_PACKAGE")) &&
+ isEmptyDir(pkg.File(pkg.Patchdir)) {
- if distinfoFile := pkg.File(pkg.DistinfoFile); fileExists(distinfoFile) {
- NewLineWhole(distinfoFile).Warnf("This file should not exist if NO_CHECKSUM or META_PACKAGE is set.")
+ distinfoFile := pkg.File(pkg.DistinfoFile)
+ if fileExists(distinfoFile) {
+ NewLineWhole(distinfoFile).Warnf("This file should not exist since NO_CHECKSUM or META_PACKAGE is set.")
}
} else {
- if distinfoFile := pkg.File(pkg.DistinfoFile); !containsVarRef(distinfoFile) && !fileExists(distinfoFile) {
- NewLineWhole(distinfoFile).Warnf(
- "File not found. Please run %q or define NO_CHECKSUM=yes in the package Makefile.", bmake("makesum"))
+ distinfoFile := pkg.File(pkg.DistinfoFile)
+ if !containsVarRef(distinfoFile) && !fileExists(distinfoFile) {
+ line := NewLineWhole(distinfoFile)
+ line.Warnf("A package that downloads files should have a distinfo file.")
+ line.Explain(
+ sprintf("To generate the distinfo file, run %q.", bmake("makesum")),
+ "",
+ "To mark the package as not needing a distinfo file, set",
+ "NO_CHECKSUM=yes in the package Makefile.")
}
}
Index: pkgsrc/pkgtools/pkglint/files/package_test.go
diff -u pkgsrc/pkgtools/pkglint/files/package_test.go:1.43 pkgsrc/pkgtools/pkglint/files/package_test.go:1.44
--- pkgsrc/pkgtools/pkglint/files/package_test.go:1.43 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/package_test.go Tue Apr 23 21:20:49 2019
@@ -945,7 +945,7 @@ func (s *Suite) Test_Package_checkfilePa
t.CheckOutputLines(
"WARN: ~/category/package/distinfo: " +
- "This file should not exist if NO_CHECKSUM or META_PACKAGE is set.")
+ "This file should not exist since NO_CHECKSUM or META_PACKAGE is set.")
}
func (s *Suite) Test_Package_checkfilePackageMakefile__USE_IMAKE_and_USE_X11(c *check.C) {
@@ -1169,12 +1169,10 @@ func (s *Suite) Test_Package_readMakefil
G.Check(pkg)
- // FIXME: One of the below warnings is redundant.
t.CheckOutputLines(
- "WARN: ~/category/package/Makefile:20: "+
- "References to other packages should look "+
- "like \"../../category/package\", not \"../package\".",
- "WARN: ~/category/package/Makefile:20: Invalid relative path \"../package/extra.mk\".")
+ "WARN: ~/category/package/Makefile:20: " +
+ "References to other packages should look " +
+ "like \"../../category/package\", not \"../package\".")
}
// When a buildlink3.mk file is included, the corresponding builtin.mk
Index: pkgsrc/pkgtools/pkglint/files/pkglint.go
diff -u pkgsrc/pkgtools/pkglint/files/pkglint.go:1.51 pkgsrc/pkgtools/pkglint/files/pkglint.go:1.52
--- pkgsrc/pkgtools/pkglint/files/pkglint.go:1.51 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/pkglint.go Tue Apr 23 21:20:49 2019
@@ -512,6 +512,15 @@ func CheckLinesMessage(lines Lines) {
defer trace.Call1(lines.FileName)()
}
+ // For now, skip all checks when the MESSAGE may be built from multiple
+ // files.
+ //
+ // If the need arises, some of the checks may be activated again, but
+ // that requires more sophisticated code.
+ if G.Pkg != nil && G.Pkg.vars.Defined("MESSAGE_SRC") {
+ return
+ }
+
explanation := func() []string {
return []string{
"A MESSAGE file should consist of a header line, having 75 \"=\"",
@@ -731,8 +740,7 @@ func CheckLinesTrailingEmptyLines(lines
// to USE_TOOLS in the current scope (file or package).
func (pkglint *Pkglint) Tool(mklines MkLines, command string, time ToolTime) (tool *Tool, usable bool) {
varname := ""
- p := NewMkParser(nil, command, false)
- if varUse := p.VarUse(); varUse != nil && p.EOF() {
+ if varUse := ToVarUse(command); varUse != nil {
varname = varUse.varname
}
Index: pkgsrc/pkgtools/pkglint/files/pkgsrc.go
diff -u pkgsrc/pkgtools/pkglint/files/pkgsrc.go:1.23 pkgsrc/pkgtools/pkglint/files/pkgsrc.go:1.24
--- pkgsrc/pkgtools/pkglint/files/pkgsrc.go:1.23 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/pkgsrc.go Tue Apr 23 21:20:49 2019
@@ -300,13 +300,13 @@ func (src *Pkgsrc) loadTools() {
{"true", "TRUE", AfterPrefsMk}}
for _, toolDef := range toolDefs {
- tools.def(toolDef.Name, toolDef.Varname, true, toolDef.Validity)
+ tools.def(toolDef.Name, toolDef.Varname, true, toolDef.Validity, nil)
}
for _, basename := range toolFiles {
mklines := src.LoadMk("mk/tools/"+basename, MustSucceed|NotEmpty)
mklines.ForEach(func(mkline MkLine) {
- tools.ParseToolLine(mkline, true, !mklines.indentation.IsConditional())
+ tools.ParseToolLine(mklines, mkline, true, !mklines.indentation.IsConditional())
})
}
@@ -318,7 +318,7 @@ func (src *Pkgsrc) loadTools() {
varname := mkline.Varname()
switch varname {
case "USE_TOOLS":
- tools.ParseToolLine(mkline, true, !mklines.indentation.IsConditional())
+ tools.ParseToolLine(mklines, mkline, true, !mklines.indentation.IsConditional())
case "_BUILD_DEFS":
// TODO: Compare with src.loadDefaultBuildDefs; is it redundant?
Index: pkgsrc/pkgtools/pkglint/files/tools.go
diff -u pkgsrc/pkgtools/pkglint/files/tools.go:1.12 pkgsrc/pkgtools/pkglint/files/tools.go:1.13
--- pkgsrc/pkgtools/pkglint/files/tools.go:1.12 Sun Mar 24 13:58:38 2019
+++ pkgsrc/pkgtools/pkglint/files/tools.go Tue Apr 23 21:20:49 2019
@@ -32,11 +32,19 @@ type Tool struct {
// (${ECHO}, ${TEST}).
MustUseVarForm bool
Validity Validity
+ Aliases []string
}
func (tool *Tool) String() string {
- return sprintf("%s:%s:%s:%s",
- tool.Name, tool.Varname, ifelseStr(tool.MustUseVarForm, "var", ""), tool.Validity)
+ aliases := ""
+ if len(tool.Aliases) > 0 {
+ aliases = ":" + strings.Join(tool.Aliases, ",")
+ }
+
+ varForm := ifelseStr(tool.MustUseVarForm, "var", "")
+
+ return sprintf("%s:%s:%s:%s%s",
+ tool.Name, tool.Varname, varForm, tool.Validity, aliases)
}
// UsableAtLoadTime means that the tool may be used by its variable
@@ -105,6 +113,11 @@ type Tools struct {
// Adding a tool to USE_TOOLS _after_ bsd.prefs.mk has been included, on the other
// hand, only makes the tool available at run time.
SeenPrefs bool
+
+ // For example, "sed" is an alias of "gsed".
+ //
+ // This means when gsed is added to USE_TOOLS, sed is implicitly added as well.
+ AliasOf map[string]string
}
func NewTools() *Tools {
@@ -112,7 +125,8 @@ func NewTools() *Tools {
make(map[string]*Tool),
make(map[string]*Tool),
nil,
- false}
+ false,
+ make(map[string]string)}
}
// Define registers the tool by its name and the corresponding
@@ -131,11 +145,11 @@ func (tr *Tools) Define(name, varname st
}
validity := tr.validity(mkline.Basename, false)
- return tr.def(name, varname, false, validity)
+ return tr.def(name, varname, false, validity, nil)
}
-func (tr *Tools) def(name, varname string, mustUseVarForm bool, validity Validity) *Tool {
- fresh := Tool{name, varname, mustUseVarForm, validity}
+func (tr *Tools) def(name, varname string, mustUseVarForm bool, validity Validity, aliases []string) *Tool {
+ fresh := Tool{name, varname, mustUseVarForm, validity, aliases}
tool := tr.byName[name]
if tool == nil {
@@ -157,6 +171,10 @@ func (tr *Tools) def(name, varname strin
}
}
+ for _, alias := range aliases {
+ tr.AliasOf[alias] = name
+ }
+
return tool
}
@@ -204,7 +222,7 @@ func (tr *Tools) Trace() {
//
// If addToUseTools is true, a USE_TOOLS line makes a tool immediately
// usable. This should only be done if the current line is unconditional.
-func (tr *Tools) ParseToolLine(mkline MkLine, fromInfrastructure bool, addToUseTools bool) {
+func (tr *Tools) ParseToolLine(mklines MkLines, mkline MkLine, fromInfrastructure bool, addToUseTools bool) {
switch {
case mkline.IsVarassign():
@@ -213,8 +231,10 @@ func (tr *Tools) ParseToolLine(mkline Mk
switch mkline.Varcanon() {
case "TOOLS_CREATE":
- if tr.IsValidToolName(value) {
- tr.def(value, "", false, AtRunTime)
+ for _, name := range mkline.ValueFields(value) {
+ if tr.IsValidToolName(name) {
+ tr.def(name, "", false, AtRunTime, nil)
+ }
}
case "_TOOLS_VARNAME.*":
@@ -227,6 +247,24 @@ func (tr *Tools) ParseToolLine(mkline Mk
tr.Define(varparam, "", mkline)
}
+ case "TOOLS_ALIASES.*":
+ tool := tr.def(varparam, "", false, Nowhere, nil)
+
+ for _, alias := range mkline.ValueFields(value) {
+ if tr.IsValidToolName(alias) {
+ tr.addAlias(tool, alias)
+ } else {
+ varUse := ToVarUse(alias)
+ if varUse != nil {
+ for _, subAlias := range mklines.ExpandLoopVar(varUse.varname) {
+ if tr.IsValidToolName(subAlias) {
+ tr.addAlias(tool, subAlias)
+ }
+ }
+ }
+ }
+ }
+
case "_TOOLS.*":
if !containsVarRef(varparam) {
tr.Define(varparam, "", mkline)
@@ -246,6 +284,11 @@ func (tr *Tools) ParseToolLine(mkline Mk
}
}
+func (tr *Tools) addAlias(tool *Tool, alias string) {
+ tool.Aliases = append(tool.Aliases, alias)
+ tr.AliasOf[alias] = tool.Name
+}
+
// parseUseTools interprets a "USE_TOOLS+=" line from a Makefile fragment.
// It determines the validity of the tool, i.e. in which places it may be used.
//
@@ -273,7 +316,7 @@ func (tr *Tools) parseUseTools(mkline Mk
for _, dep := range deps {
name := strings.Split(dep, ":")[0]
if createIfAbsent || tr.ByName(name) != nil {
- tr.def(name, "", false, validity)
+ tr.def(name, "", false, validity, nil)
}
}
}
@@ -296,9 +339,10 @@ func (tr *Tools) ByName(name string) *To
if tool == nil && tr.fallback != nil {
fallback := tr.fallback.ByName(name)
if fallback != nil {
- return tr.def(fallback.Name, fallback.Varname, fallback.MustUseVarForm, fallback.Validity)
+ tool = tr.def(fallback.Name, fallback.Varname, fallback.MustUseVarForm, fallback.Validity, fallback.Aliases)
}
}
+ tr.adjustValidity(tool)
return tool
}
@@ -307,9 +351,10 @@ func (tr *Tools) ByVarname(varname strin
if tool == nil && tr.fallback != nil {
fallback := tr.fallback.ByVarname(varname)
if fallback != nil {
- return tr.def(fallback.Name, fallback.Varname, fallback.MustUseVarForm, fallback.Validity)
+ tool = tr.def(fallback.Name, fallback.Varname, fallback.MustUseVarForm, fallback.Validity, fallback.Aliases)
}
}
+ tr.adjustValidity(tool)
return tool
}
@@ -332,6 +377,22 @@ func (tr *Tools) IsValidToolName(name st
return name == "[" || name == "echo -n" || matches(name, `^[-0-9a-z.]+$`)
}
+func (tr *Tools) adjustValidity(tool *Tool) {
+ if tool == nil {
+ return
+ }
+
+ aliasName := tr.AliasOf[tool.Name]
+ if aliasName == "" {
+ return
+ }
+
+ alias := tr.ByName(tr.AliasOf[tool.Name])
+ if alias.Validity > tool.Validity {
+ tool.Validity = alias.Validity
+ }
+}
+
type Validity uint8
const (
Index: pkgsrc/pkgtools/pkglint/files/tools_test.go
diff -u pkgsrc/pkgtools/pkglint/files/tools_test.go:1.14 pkgsrc/pkgtools/pkglint/files/tools_test.go:1.15
--- pkgsrc/pkgtools/pkglint/files/tools_test.go:1.14 Sat Apr 20 17:43:25 2019
+++ pkgsrc/pkgtools/pkglint/files/tools_test.go Tue Apr 23 21:20:49 2019
@@ -4,28 +4,28 @@ import "gopkg.in/check.v1"
func (s *Suite) Test_Tool_UsableAtLoadTime(c *check.C) {
- nowhere := Tool{"nowhere", "", false, Nowhere}
+ nowhere := Tool{"nowhere", "", false, Nowhere, nil}
c.Check(nowhere.UsableAtLoadTime(false), equals, false)
c.Check(nowhere.UsableAtLoadTime(true), equals, false)
- load := Tool{"load", "", false, AfterPrefsMk}
+ load := Tool{"load", "", false, AfterPrefsMk, nil}
c.Check(load.UsableAtLoadTime(false), equals, false)
c.Check(load.UsableAtLoadTime(true), equals, true)
- run := Tool{"run", "", false, AtRunTime}
+ run := Tool{"run", "", false, AtRunTime, nil}
c.Check(run.UsableAtLoadTime(false), equals, false)
c.Check(run.UsableAtLoadTime(true), equals, false)
}
func (s *Suite) Test_Tool_UsableAtRunTime(c *check.C) {
- nowhere := Tool{"nowhere", "", false, Nowhere}
+ nowhere := Tool{"nowhere", "", false, Nowhere, nil}
c.Check(nowhere.UsableAtRunTime(), equals, false)
- load := Tool{"load", "", false, AfterPrefsMk}
+ load := Tool{"load", "", false, AfterPrefsMk, nil}
c.Check(load.UsableAtRunTime(), equals, true)
- run := Tool{"run", "", false, AtRunTime}
+ run := Tool{"run", "", false, AtRunTime, nil}
c.Check(run.UsableAtRunTime(), equals, true)
}
@@ -130,9 +130,18 @@ func (s *Suite) Test_Tools__load_from_in
tools := NewTools()
- tools.ParseToolLine(t.NewMkLine("create.mk", 2, "TOOLS_CREATE+= load"), true, false)
- tools.ParseToolLine(t.NewMkLine("create.mk", 3, "TOOLS_CREATE+= run"), true, false)
- tools.ParseToolLine(t.NewMkLine("create.mk", 4, "TOOLS_CREATE+= nowhere"), true, false)
+ // Only used for variable lookup, which is irrelevant for this test.
+ dummyMklines := t.NewMkLines("dummy.mk")
+
+ createMklines := t.NewMkLines("create.mk",
+ MkRcsID,
+ "TOOLS_CREATE+= load",
+ "TOOLS_CREATE+= run",
+ "TOOLS_CREATE+= nowhere")
+
+ createMklines.ForEach(func(mkline MkLine) {
+ tools.ParseToolLine(createMklines, mkline, true, false)
+ })
// The references to the tools are stable,
// the lookup methods always return the same objects.
@@ -147,9 +156,15 @@ func (s *Suite) Test_Tools__load_from_in
c.Check(nowhere.String(), equals, "nowhere:::AtRunTime")
// The variable name RUN is reserved by pkgsrc, therefore RUN_CMD.
- tools.ParseToolLine(t.NewMkLine("varnames.mk", 2, "_TOOLS_VARNAME.load= LOAD"), true, false)
- tools.ParseToolLine(t.NewMkLine("varnames.mk", 3, "_TOOLS_VARNAME.run= RUN_CMD"), true, false)
- tools.ParseToolLine(t.NewMkLine("varnames.mk", 4, "_TOOLS_VARNAME.nowhere= NOWHERE"), true, false)
+ varnamesMklines := t.NewMkLines("varnames.mk",
+ MkRcsID,
+ "_TOOLS_VARNAME.load= LOAD",
+ "_TOOLS_VARNAME.run= RUN_CMD",
+ "_TOOLS_VARNAME.nowhere= NOWHERE")
+
+ varnamesMklines.ForEach(func(mkline MkLine) {
+ tools.ParseToolLine(varnamesMklines, mkline, true, false)
+ })
// At this point the tools can be found by their variable names, too.
// They still may not be used.
@@ -163,14 +178,14 @@ func (s *Suite) Test_Tools__load_from_in
c.Check(run.String(), equals, "run:RUN_CMD::AtRunTime")
c.Check(nowhere.String(), equals, "nowhere:NOWHERE::AtRunTime")
- tools.ParseToolLine(t.NewMkLine("bsd.prefs.mk", 2, "USE_TOOLS+= load"), true, true)
+ tools.ParseToolLine(dummyMklines, t.NewMkLine("bsd.prefs.mk", 2, "USE_TOOLS+= load"), true, true)
// Tools that are added to USE_TOOLS in bsd.prefs.mk may be used afterwards.
// By variable name, they may be used both at load time as well as run time.
// By plain name, they may be used only in {pre,do,post}-* targets.
c.Check(load.String(), equals, "load:LOAD::AfterPrefsMk")
- tools.ParseToolLine(t.NewMkLine("bsd.pkg.mk", 2, "USE_TOOLS+= run"), true, true)
+ tools.ParseToolLine(dummyMklines, t.NewMkLine("bsd.pkg.mk", 2, "USE_TOOLS+= run"), true, true)
// Tools that are added to USE_TOOLS in bsd.pkg.mk may be used afterwards at run time.
// The {pre,do,post}-* targets may use both forms (${CAT} and cat).
@@ -219,16 +234,19 @@ func (s *Suite) Test_Tools__package_Make
// All other files must not use the tools at load time.
// For them, seenPrefs can be thought of as being true from the beginning.
- tools.ParseToolLine(t.NewMkLine("Makefile", 2, "USE_TOOLS+= pkg-before-prefs"), false, true)
+ // Only used for variable lookup, which is irrelevant for this test.
+ dummyMklines := t.NewMkLines("dummy.mk")
+
+ tools.ParseToolLine(dummyMklines, t.NewMkLine("Makefile", 2, "USE_TOOLS+= pkg-before-prefs"), false, true)
c.Check(before.Validity, equals, AfterPrefsMk)
c.Check(tools.SeenPrefs, equals, false)
- tools.ParseToolLine(t.NewMkLine("Makefile", 3, ".include \"../../mk/bsd.prefs.mk\""), false, true)
+ tools.ParseToolLine(dummyMklines, t.NewMkLine("Makefile", 3, ".include \"../../mk/bsd.prefs.mk\""), false, true)
c.Check(tools.SeenPrefs, equals, true)
- tools.ParseToolLine(t.NewMkLine("Makefile", 4, "USE_TOOLS+= pkg-after-prefs"), false, true)
+ tools.ParseToolLine(dummyMklines, t.NewMkLine("Makefile", 4, "USE_TOOLS+= pkg-after-prefs"), false, true)
c.Check(after.Validity, equals, AtRunTime)
}
@@ -419,20 +437,20 @@ func (s *Suite) Test_Tools__var(c *check
// See also Pkglint.Tool.
func (s *Suite) Test_Tools_Fallback__tools_having_the_same_variable_name_realistic(c *check.C) {
nonGnu := NewTools()
- nonGnu.def("sed", "SED", false, AfterPrefsMk)
+ nonGnu.def("sed", "SED", false, AfterPrefsMk, nil)
gnu := NewTools()
- gnu.def("gsed", "SED", false, Nowhere)
+ gnu.def("gsed", "SED", false, Nowhere, nil)
local1 := NewTools()
- local1.def("sed", "SED", false, AfterPrefsMk)
+ local1.def("sed", "SED", false, AfterPrefsMk, nil)
local1.Fallback(gnu)
c.Check(local1.ByName("sed").Validity, equals, AfterPrefsMk)
c.Check(local1.ByName("gsed").Validity, equals, Nowhere)
local2 := NewTools()
- local2.def("gsed", "SED", false, Nowhere)
+ local2.def("gsed", "SED", false, Nowhere, nil)
local2.Fallback(nonGnu)
c.Check(local2.ByName("sed").Validity, equals, AfterPrefsMk)
@@ -453,29 +471,88 @@ func (s *Suite) Test_Tools_Fallback__too
//
// See also Pkglint.Tool.
func (s *Suite) Test_Tools_Fallback__tools_having_the_same_variable_name_unrealistic(c *check.C) {
+
+ // This simulates a tool defined in the tools framework but not added
+ // to USE_TOOLS, neither by bsd.prefs.mk nor by bsd.pkg.mk.
nonGnu := NewTools()
- nonGnu.def("sed", "SED", false, Nowhere)
+ nonGnu.def("sed", "SED", false, Nowhere, nil)
+ // This simulates a tool that is added to USE_TOOLS in bsd.prefs.mk.
gnu := NewTools()
- gnu.def("gsed", "SED", false, AfterPrefsMk)
+ gnu.def("gsed", "SED", false, AfterPrefsMk, nil)
+ gnu.ByName("gsed").Aliases = []string{"sed"}
+ // This simulates a package that doesn't mention the sed tool at all.
+ // The call to .def() is therefore unrealistic.
+ // Nevertheless, since the GNU tools define the gsed tool as well,
+ // it is available even though not explicitly mentioned in the package.
local1 := NewTools()
- local1.def("sed", "SED", false, Nowhere)
+ local1.def("sed", "SED", false, Nowhere, nil)
local1.Fallback(gnu)
c.Check(local1.ByName("sed").Validity, equals, Nowhere)
c.Check(local1.ByName("gsed").Validity, equals, AfterPrefsMk)
local2 := NewTools()
- local2.def("gsed", "SED", false, AfterPrefsMk)
+ local2.def("gsed", "SED", false, AfterPrefsMk, []string{"sed"})
local2.Fallback(nonGnu)
- c.Check(local2.ByName("sed").Validity, equals, Nowhere)
+ c.Check(local2.ByName("sed").Validity, equals, AfterPrefsMk)
c.Check(local2.ByName("gsed").Validity, equals, AfterPrefsMk)
- // FIXME: Must both be gsed:SED::AfterPrefsMk
- c.Check(local1.ByVarname("SED").String(), equals, "sed:SED::Nowhere")
- c.Check(local2.ByVarname("SED").String(), equals, "sed:SED::Nowhere")
+ c.Check(local1.ByVarname("SED").String(), equals, "sed:SED::AfterPrefsMk")
+ c.Check(local2.ByVarname("SED").String(), equals, "sed:SED::AfterPrefsMk")
+}
+
+func (s *Suite) Test_Tools__aliases(c *check.C) {
+ t := s.Init(c)
+
+ mklines := t.NewMkLines("mk/tools/replace.mk",
+ MkRcsID,
+ "TOOLS_CREATE+=\tsed",
+ "TOOLS_PATH.sed=\t/bin/sed",
+ "",
+ "TOOLS_CREATE+=\tgsed",
+ "TOOLS_PATH.gsed=\t/bin/gnu-sed",
+ "TOOLS_ALIASES.gsed=\tsed ${tool}")
+
+ infraTools := NewTools()
+ mklines.ForEach(func(mkline MkLine) {
+ infraTools.ParseToolLine(mklines, mkline, false, false)
+ })
+
+ c.Check(infraTools.ByName("sed").String(), equals, "sed:::AtRunTime")
+ c.Check(infraTools.ByName("gsed").String(), equals, "gsed:::AtRunTime:sed")
+
+ pkgTools := NewTools()
+ pkgTools.Fallback(infraTools)
+
+ c.Check(pkgTools.ByName("sed").String(), equals, "sed:::AtRunTime")
+ c.Check(pkgTools.ByName("gsed").String(), equals, "gsed:::AtRunTime:sed")
+
+ mkline := t.NewMkLine("Makefile", 123, "USE_TOOLS+=\tgsed")
+ pkgTools.ParseToolLine(mklines, mkline, false, false)
+
+ // Since sed is an alias of gsed, it gets the same validity.
+ c.Check(pkgTools.ByName("sed").String(), equals, "sed:::AfterPrefsMk")
+ c.Check(pkgTools.ByName("gsed").String(), equals, "gsed:::AfterPrefsMk:sed")
+}
+
+func (s *Suite) Test_Tools__aliases_in_for_loop(c *check.C) {
+ t := s.Init(c)
+
+ mklines := t.NewMkLines("mk/tools/replace.mk",
+ MkRcsID,
+ "_TOOLS_GREP=\tgrep egrep fgrep",
+ "TOOLS_CREATE+=\tgrep egrep fgrep ggrep",
+ ".for t in ${_TOOLS_GREP}",
+ "TOOLS_ALIASES.ggrep+=\t${t}",
+ ".endfor")
+
+ mklines.collectDefinedVariables() // calls ParseToolLine internally
+
+ c.Check(mklines.Tools.ByName("ggrep").Aliases,
+ deepEquals, []string{"grep", "egrep", "fgrep"})
}
// The cmake tool is included conditionally. The condition is so simple that
Index: pkgsrc/pkgtools/pkglint/files/vardefs.go
diff -u pkgsrc/pkgtools/pkglint/files/vardefs.go:1.59 pkgsrc/pkgtools/pkglint/files/vardefs.go:1.60
--- pkgsrc/pkgtools/pkglint/files/vardefs.go:1.59 Sat Apr 20 17:43:25 2019
+++ pkgsrc/pkgtools/pkglint/files/vardefs.go Tue Apr 23 21:20:49 2019
@@ -864,7 +864,7 @@ func (reg *VarTypeRegistry) Init(src *Pk
"Makefile, Makefile.*, *.mk: append")
acl("BUILDLINK_FNAME_TRANSFORM.*", BtSedCommands,
PackageSettable,
- "Makefile, buildlink3.mk, builtin.mk, hacks.mk, options.mk: append")
+ "Makefile, buildlink3.mk, builtin.mk, options.mk: append")
acllist("BUILDLINK_TRANSFORM", BtWrapperTransform,
PackageSettable,
"*: append")
@@ -1397,10 +1397,11 @@ func (reg *VarTypeRegistry) Init(src *Pk
// Since only the hacks.mk can define hacks, appending to it only makes
// sense there.
//
- // TODO: Is it possible to include hacks.mk files from the dependencies?
+ // TODO: Is it possible that a package includes the hacks.mk file from
+ // one of its dependencies?
acllist("PKG_HACKS", BtIdentifier,
PackageSettable,
- "hacks.mk: append")
+ "*: none")
sys("PKG_INFO", BtShellCommand)
sys("PKG_JAVA_HOME", BtPathname)
sys("PKG_JVM", jvms)
@@ -1723,7 +1724,7 @@ func (reg *VarTypeRegistry) parseACLEntr
switch glob {
case "*",
"Makefile", "Makefile.*",
- "buildlink3.mk", "builtin.mk", "options.mk", "hacks.mk", "*.mk":
+ "buildlink3.mk", "builtin.mk", "options.mk", "*.mk":
break
default:
withoutSpecial := strings.TrimPrefix(glob, "special:")
Home |
Main Index |
Thread Index |
Old Index