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: Sun Dec 8 22:03:38 UTC 2019
Modified Files:
pkgsrc/pkgtools/pkglint: Makefile
pkgsrc/pkgtools/pkglint/files: autofix_test.go buildlink3.go
category.go category_test.go check_test.go logging.go
logging_test.go mkassignchecker.go mkassignchecker_test.go
mkcondchecker.go mkcondchecker_test.go mklexer.go mklexer_test.go
mkline.go mklinechecker.go mklinechecker_test.go
mklineparser_test.go mklines.go mklines_test.go mkparser_test.go
mktypes_test.go mkvarusechecker.go mkvarusechecker_test.go
options.go options_test.go package.go package_test.go
patches_test.go path.go path_test.go pkglint.1 pkglint.go
pkglint_test.go pkgsrc.go plist.go plist_test.go redundantscope.go
shell.go shell_test.go shtokenizer_test.go substcontext.go util.go
util_test.go vardefs.go vargroups.go vartype.go vartypecheck.go
vartypecheck_test.go
Log Message:
pkgtools/pkglint: update pkglint to 19.3.15
Changes since 19.3.14:
Invalid lines in PLIST files are now reported as errors instead of
warnings. If pkglint doesn't know about it, it must be an error.
In PLIST files, all paths are validated to be canonical. That is, no
dotdot components, no absolute paths, no extra slashes, no intermediate
dot components.
Fewer notes for unexpanded variable expressions in DESCR files. Before,
the text $@ was reported as possible Makefile variable even though it
was just a Perl expression.
README files are allowed again in pkgsrc package directories. There was
no convincing argument why these should be forbidden.
A few diagnostics have been changed from NOTE to WARNING or from WARNING
to ERROR, to match their wording.
When pkglint suggests to replace :M with ==, the wording is now "can be
made" instead of "should".
To generate a diff of this commit:
cvs rdiff -u -r1.614 -r1.615 pkgsrc/pkgtools/pkglint/Makefile
cvs rdiff -u -r1.34 -r1.35 pkgsrc/pkgtools/pkglint/files/autofix_test.go
cvs rdiff -u -r1.28 -r1.29 pkgsrc/pkgtools/pkglint/files/buildlink3.go \
pkgsrc/pkgtools/pkglint/files/category.go \
pkgsrc/pkgtools/pkglint/files/category_test.go
cvs rdiff -u -r1.58 -r1.59 pkgsrc/pkgtools/pkglint/files/check_test.go
cvs rdiff -u -r1.33 -r1.34 pkgsrc/pkgtools/pkglint/files/logging.go \
pkgsrc/pkgtools/pkglint/files/patches_test.go
cvs rdiff -u -r1.23 -r1.24 pkgsrc/pkgtools/pkglint/files/logging_test.go
cvs rdiff -u -r1.1 -r1.2 pkgsrc/pkgtools/pkglint/files/mkassignchecker.go \
pkgsrc/pkgtools/pkglint/files/mkassignchecker_test.go \
pkgsrc/pkgtools/pkglint/files/mkcondchecker.go \
pkgsrc/pkgtools/pkglint/files/mkcondchecker_test.go \
pkgsrc/pkgtools/pkglint/files/mkvarusechecker.go \
pkgsrc/pkgtools/pkglint/files/mkvarusechecker_test.go
cvs rdiff -u -r1.4 -r1.5 pkgsrc/pkgtools/pkglint/files/mklexer.go
cvs rdiff -u -r1.3 -r1.4 pkgsrc/pkgtools/pkglint/files/mklexer_test.go
cvs rdiff -u -r1.68 -r1.69 pkgsrc/pkgtools/pkglint/files/mkline.go
cvs rdiff -u -r1.57 -r1.58 pkgsrc/pkgtools/pkglint/files/mklinechecker.go
cvs rdiff -u -r1.52 -r1.53 \
pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go \
pkgsrc/pkgtools/pkglint/files/shell.go
cvs rdiff -u -r1.6 -r1.7 pkgsrc/pkgtools/pkglint/files/mklineparser_test.go
cvs rdiff -u -r1.63 -r1.64 pkgsrc/pkgtools/pkglint/files/mklines.go \
pkgsrc/pkgtools/pkglint/files/util.go \
pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go
cvs rdiff -u -r1.54 -r1.55 pkgsrc/pkgtools/pkglint/files/mklines_test.go \
pkgsrc/pkgtools/pkglint/files/pkglint_test.go
cvs rdiff -u -r1.37 -r1.38 pkgsrc/pkgtools/pkglint/files/mkparser_test.go
cvs rdiff -u -r1.19 -r1.20 pkgsrc/pkgtools/pkglint/files/mktypes_test.go \
pkgsrc/pkgtools/pkglint/files/options.go
cvs rdiff -u -r1.21 -r1.22 pkgsrc/pkgtools/pkglint/files/options_test.go
cvs rdiff -u -r1.73 -r1.74 pkgsrc/pkgtools/pkglint/files/package.go
cvs rdiff -u -r1.62 -r1.63 pkgsrc/pkgtools/pkglint/files/package_test.go
cvs rdiff -u -r1.5 -r1.6 pkgsrc/pkgtools/pkglint/files/path.go \
pkgsrc/pkgtools/pkglint/files/path_test.go \
pkgsrc/pkgtools/pkglint/files/vargroups.go
cvs rdiff -u -r1.60 -r1.61 pkgsrc/pkgtools/pkglint/files/pkglint.1 \
pkgsrc/pkgtools/pkglint/files/shell_test.go
cvs rdiff -u -r1.69 -r1.70 pkgsrc/pkgtools/pkglint/files/pkglint.go
cvs rdiff -u -r1.46 -r1.47 pkgsrc/pkgtools/pkglint/files/pkgsrc.go
cvs rdiff -u -r1.47 -r1.48 pkgsrc/pkgtools/pkglint/files/plist.go
cvs rdiff -u -r1.41 -r1.42 pkgsrc/pkgtools/pkglint/files/plist_test.go \
pkgsrc/pkgtools/pkglint/files/vartype.go
cvs rdiff -u -r1.10 -r1.11 pkgsrc/pkgtools/pkglint/files/redundantscope.go
cvs rdiff -u -r1.22 -r1.23 pkgsrc/pkgtools/pkglint/files/shtokenizer_test.go
cvs rdiff -u -r1.31 -r1.32 pkgsrc/pkgtools/pkglint/files/substcontext.go
cvs rdiff -u -r1.40 -r1.41 pkgsrc/pkgtools/pkglint/files/util_test.go
cvs rdiff -u -r1.81 -r1.82 pkgsrc/pkgtools/pkglint/files/vardefs.go
cvs rdiff -u -r1.70 -r1.71 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.614 pkgsrc/pkgtools/pkglint/Makefile:1.615
--- pkgsrc/pkgtools/pkglint/Makefile:1.614 Sun Dec 8 00:06:37 2019
+++ pkgsrc/pkgtools/pkglint/Makefile Sun Dec 8 22:03:37 2019
@@ -1,6 +1,6 @@
-# $NetBSD: Makefile,v 1.614 2019/12/08 00:06:37 rillig Exp $
+# $NetBSD: Makefile,v 1.615 2019/12/08 22:03:37 rillig Exp $
-PKGNAME= pkglint-19.3.14
+PKGNAME= pkglint-19.3.15
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.34 pkgsrc/pkgtools/pkglint/files/autofix_test.go:1.35
--- pkgsrc/pkgtools/pkglint/files/autofix_test.go:1.34 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/autofix_test.go Sun Dec 8 22:03:38 2019
@@ -164,7 +164,8 @@ func (s *Suite) Test_Autofix__lonely_sou
t.CheckOutputLines(
">\tPRE_XORGPROTO_LIST_MISSING =\tapplewmproto",
- "NOTE: x11/xorgproto/builtin.mk:5: Unnecessary space after variable name \"PRE_XORGPROTO_LIST_MISSING\".")
+ "NOTE: x11/xorg-cf-files/../../x11/xorgproto/builtin.mk:5: "+
+ "Unnecessary space after variable name \"PRE_XORGPROTO_LIST_MISSING\".")
}
// Up to 2018-11-26, pkglint in some cases logged only the source without
@@ -557,7 +558,7 @@ func (s *Suite) Test_Autofix_ReplaceAt(c
lines := func(lines ...string) []string { return lines }
test := func(texts []string, rawIndex int, column int, from, to string, diagnostics ...string) {
- mainPart := func() {
+ mainPart := func(autofix bool) {
mklines := t.NewMkLines("filename.mk", texts...)
assert(len(mklines.mklines) == 1)
mkline := mklines.mklines[0]
Index: pkgsrc/pkgtools/pkglint/files/buildlink3.go
diff -u pkgsrc/pkgtools/pkglint/files/buildlink3.go:1.28 pkgsrc/pkgtools/pkglint/files/buildlink3.go:1.29
--- pkgsrc/pkgtools/pkglint/files/buildlink3.go:1.28 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/buildlink3.go Sun Dec 8 22:03:38 2019
@@ -99,7 +99,7 @@ func (ck *Buildlink3Checker) checkUnique
return
}
- dirname := G.Pkgsrc.ToRel(mkline.Filename.DirNoClean()).Base()
+ dirname := G.Pkgsrc.Rel(mkline.Filename.DirNoClean()).Base()
base, name := trimCommon(pkgbase, dirname)
if base == "" && matches(name, `^(\d*|-cvs|-fossil|-git|-hg|-svn|-devel|-snapshot)$`) {
return
Index: pkgsrc/pkgtools/pkglint/files/category.go
diff -u pkgsrc/pkgtools/pkglint/files/category.go:1.28 pkgsrc/pkgtools/pkglint/files/category.go:1.29
--- pkgsrc/pkgtools/pkglint/files/category.go:1.28 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/category.go Sun Dec 8 22:03:38 2019
@@ -148,7 +148,7 @@ func CheckdirCategory(dir CurrPath) {
mlex.SkipEmptyOrNote()
mlex.SkipContainsOrWarn(".include \"../mk/misc/category.mk\"")
if !mlex.EOF() {
- mlex.CurrentLine().Errorf("The file should end here.")
+ mlex.CurrentLine().Errorf("The file must end here.")
}
}
Index: pkgsrc/pkgtools/pkglint/files/category_test.go
diff -u pkgsrc/pkgtools/pkglint/files/category_test.go:1.28 pkgsrc/pkgtools/pkglint/files/category_test.go:1.29
--- pkgsrc/pkgtools/pkglint/files/category_test.go:1.28 Mon Dec 2 23:32:09 2019
+++ pkgsrc/pkgtools/pkglint/files/category_test.go Sun Dec 8 22:03:38 2019
@@ -33,7 +33,7 @@ func (s *Suite) Test_CheckdirCategory__t
"ERROR: ~/archivers/Makefile:3: \"aaaaa\" exists in the Makefile but not in the file system.",
"NOTE: ~/archivers/Makefile:3: Empty line expected after this line.",
"WARN: ~/archivers/Makefile:4: This line should contain the following text: .include \"../mk/misc/category.mk\"",
- "ERROR: ~/archivers/Makefile:4: The file should end here.")
+ "ERROR: ~/archivers/Makefile:4: The file must end here.")
}
func (s *Suite) Test_CheckdirCategory__invalid_comment(c *check.C) {
@@ -305,7 +305,7 @@ func (s *Suite) Test_CheckdirCategory__c
"ERROR: ~/category/Makefile:3: \"package\" exists in the file system but not in the Makefile.",
"NOTE: ~/category/Makefile:2: Empty line expected after this line.",
"WARN: ~/category/Makefile:3: This line should contain the following text: .include \"../mk/misc/category.mk\"",
- "ERROR: ~/category/Makefile:3: The file should end here.")
+ "ERROR: ~/category/Makefile:3: The file must end here.")
}
func (s *Suite) Test_CheckdirCategory__unexpected_EOF_while_reading_SUBDIR(c *check.C) {
Index: pkgsrc/pkgtools/pkglint/files/check_test.go
diff -u pkgsrc/pkgtools/pkglint/files/check_test.go:1.58 pkgsrc/pkgtools/pkglint/files/check_test.go:1.59
--- pkgsrc/pkgtools/pkglint/files/check_test.go:1.58 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/check_test.go Sun Dec 8 22:03:38 2019
@@ -102,8 +102,8 @@ func (s *Suite) TearDownTest(c *check.C)
// Ensures that all test names follow a common naming scheme:
//
// Test_${Type}_${Method}__${description_using_underscores}
-func (s *Suite) Test__qa(c *check.C) {
- ck := intqa.NewQAChecker(c.Errorf)
+func Test__qa(t *testing.T) {
+ ck := intqa.NewQAChecker(t.Errorf)
ck.Configure("autofix.go", "*", "*", -intqa.EMissingTest) // TODO
ck.Configure("buildlink3.go", "*", "*", -intqa.EMissingTest) // TODO
@@ -131,7 +131,6 @@ func (s *Suite) Test__qa(c *check.C) {
ck.Configure("patches.go", "*", "*", -intqa.EMissingTest) // TODO
ck.Configure("pkglint.go", "*", "*", -intqa.EMissingTest) // TODO
ck.Configure("pkgsrc.go", "*", "*", -intqa.EMissingTest) // TODO
- ck.Configure("plist.go", "*", "*", -intqa.EMissingTest) // TODO
ck.Configure("redundantscope.go", "*", "*", -intqa.EMissingTest) // TODO
ck.Configure("shell.go", "*", "*", -intqa.EMissingTest) // TODO
ck.Configure("shtokenizer.go", "*", "*", -intqa.EMissingTest) // TODO
@@ -144,7 +143,6 @@ func (s *Suite) Test__qa(c *check.C) {
ck.Configure("vardefs.go", "*", "*", -intqa.EMissingTest) // TODO
ck.Configure("vargroups.go", "*", "*", -intqa.EMissingTest) // TODO
ck.Configure("vartype.go", "*", "*", -intqa.EMissingTest) // TODO
- ck.Configure("vartypecheck.go", "*", "*", -intqa.EMissingTest) // TODO
// For now, don't require tests for all the test code.
// Having good coverage for the main code is more important.
@@ -262,6 +260,27 @@ func (t *Tester) SetUpTool(name, varname
return G.Pkgsrc.Tools.def(name, varname, false, validity, nil)
}
+// SetUpType defines a variable to have a certain type and access permissions,
+// like in the type definitions in vardefs.go.
+//
+// Example:
+// SetUpType("PKGPATH", BtPkgpath, DefinedIfInScope|NonemptyIfDefined,
+// "Makefile, *.mk: default, set, append, use, use-loadtime")
+func (t *Tester) SetUpType(varname string, basicType *BasicType,
+ options vartypeOptions, aclEntries ...string) {
+
+ if len(aclEntries) == 0 {
+ aclEntries = []string{"Makefile, *.mk: default, set, append, use, use-loadtime"}
+ }
+
+ G.Pkgsrc.vartypes.acl(varname, basicType, options, aclEntries...)
+
+ // Make sure that registering the type succeeds.
+ // This is necessary for BtUnknown and guessed types.
+ vartype := G.Pkgsrc.VariableType(nil, varname)
+ t.c.Assert(vartype.basicType, check.Equals, basicType)
+}
+
// SetUpFileLines creates a temporary file and writes the given lines to it.
// The file is then read in, without interpreting line continuations.
//
@@ -399,7 +418,7 @@ func (t *Tester) SetUpPkgsrc() {
// SetUpCategory makes the given category valid by creating a dummy Makefile.
// After that, it can be mentioned in the CATEGORIES variable of a package.
func (t *Tester) SetUpCategory(name RelPath) {
- assert(G.Pkgsrc.ToRel(t.File(name)).Count() == 1)
+ assert(G.Pkgsrc.Rel(t.File(name)).Count() == 1)
makefile := name.JoinNoClean("Makefile")
if !t.File(makefile).IsFile() {
@@ -535,7 +554,7 @@ func (t *Tester) CreateFileLines(filenam
// temporary directory.
func (t *Tester) CreateFileDummyPatch(filename RelPath) {
// Patch files only make sense in category/package/patches directories.
- assert(G.Pkgsrc.ToRel(t.File(filename)).Count() == 4)
+ assert(G.Pkgsrc.Rel(t.File(filename)).Count() == 4)
t.CreateFileLines(filename,
CvsID,
@@ -551,7 +570,7 @@ func (t *Tester) CreateFileDummyPatch(fi
func (t *Tester) CreateFileBuildlink3(filename RelPath, customLines ...string) {
// Buildlink3.mk files only make sense in category/package directories.
- assert(G.Pkgsrc.ToRel(t.File(filename)).Count() == 3)
+ assert(G.Pkgsrc.Rel(t.File(filename)).Count() == 3)
dir := filename.DirClean()
lower := dir.Base()
@@ -924,12 +943,12 @@ func (t *Tester) ExpectAssert(action fun
// ExpectDiagnosticsAutofix first runs the given action with -Wall, and
// then another time with -Wall --autofix.
-func (t *Tester) ExpectDiagnosticsAutofix(action func(), diagnostics ...string) {
+func (t *Tester) ExpectDiagnosticsAutofix(action func(autofix bool), diagnostics ...string) {
t.SetUpCommandLine("-Wall")
- action()
+ action(false)
t.SetUpCommandLine("-Wall", "--autofix")
- action()
+ action(true)
t.CheckOutput(diagnostics)
}
Index: pkgsrc/pkgtools/pkglint/files/logging.go
diff -u pkgsrc/pkgtools/pkglint/files/logging.go:1.33 pkgsrc/pkgtools/pkglint/files/logging.go:1.34
--- pkgsrc/pkgtools/pkglint/files/logging.go:1.33 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/logging.go Sun Dec 8 22:03:38 2019
@@ -255,6 +255,15 @@ func (l *Logger) Logf(level *LogLevel, f
return
}
+ if G.Testing {
+ if level != Error {
+ assertf(!contains(format, "must"), "Must must only appear in errors: %s", format)
+ }
+ if level != Warn && level != Note {
+ assertf(!contains(format, "should"), "Should must only appear in warnings: %s", format)
+ }
+ }
+
if G.Testing && format != AutofixFormat {
if textproc.Alpha.Contains(format[0]) {
assertf(
Index: pkgsrc/pkgtools/pkglint/files/patches_test.go
diff -u pkgsrc/pkgtools/pkglint/files/patches_test.go:1.33 pkgsrc/pkgtools/pkglint/files/patches_test.go:1.34
--- pkgsrc/pkgtools/pkglint/files/patches_test.go:1.33 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/patches_test.go Sun Dec 8 22:03:38 2019
@@ -590,7 +590,7 @@ func (s *Suite) Test_PatchChecker_Check_
CheckLinesPatch(lines)
- // FIXME: Patches must not apply to absolute paths.
+ // XXX: Patches must not apply to absolute paths.
// The only allowed exception is /dev/null.
// ^(---|\+\+\+) /(?!dev/null)
t.CheckOutputEmpty()
Index: pkgsrc/pkgtools/pkglint/files/logging_test.go
diff -u pkgsrc/pkgtools/pkglint/files/logging_test.go:1.23 pkgsrc/pkgtools/pkglint/files/logging_test.go:1.24
--- pkgsrc/pkgtools/pkglint/files/logging_test.go:1.23 Wed Nov 27 22:10:07 2019
+++ pkgsrc/pkgtools/pkglint/files/logging_test.go Sun Dec 8 22:03:38 2019
@@ -172,11 +172,11 @@ func (s *Suite) Test_Logger_Diag__duplic
logger := Logger{out: NewSeparatorWriter(&sw)}
line := t.NewLine("filename", 3, "Text")
- logger.Diag(line, Error, "Blue should be %s.", "orange")
- logger.Diag(line, Error, "Blue should be %s.", "orange")
+ logger.Diag(line, Error, "Blue must be %s.", "orange")
+ logger.Diag(line, Error, "Blue must be %s.", "orange")
t.CheckEquals(sw.String(), ""+
- "ERROR: filename:3: Blue should be orange.\n")
+ "ERROR: filename:3: Blue must be orange.\n")
}
// Explanations are associated with their diagnostics. Therefore, when one
@@ -189,22 +189,22 @@ func (s *Suite) Test_Logger_Diag__explan
logger.Opts.Explain = true
line := t.NewLine("filename", 3, "Text")
- logger.Diag(line, Error, "Blue should be %s.", "orange")
+ logger.Diag(line, Error, "Blue must be %s.", "orange")
logger.Explain(
"The colors have changed.")
- logger.Diag(line, Error, "Blue should be %s.", "orange")
+ logger.Diag(line, Error, "Blue must be %s.", "orange")
logger.Explain(
"The colors have changed.")
// Even when the text of the explanation is not the same, it is still
// suppressed since it belongs to the diagnostic.
- logger.Diag(line, Error, "Blue should be %s.", "orange")
+ logger.Diag(line, Error, "Blue must be %s.", "orange")
logger.Explain(
"The colors have further changed.")
t.CheckEquals(sw.String(), ""+
- "ERROR: filename:3: Blue should be orange.\n"+
+ "ERROR: filename:3: Blue must be orange.\n"+
"\n"+
"\tThe colors have changed.\n"+
"\n")
@@ -571,10 +571,10 @@ func (s *Suite) Test_Logger_Logf(c *chec
var sw strings.Builder
logger := Logger{out: NewSeparatorWriter(&sw)}
- logger.Logf(Error, "filename", "3", "Blue should be %s.", "Blue should be orange.")
+ logger.Logf(Error, "filename", "3", "Blue must be %s.", "Blue must be orange.")
t.CheckEquals(sw.String(), ""+
- "ERROR: filename:3: Blue should be orange.\n")
+ "ERROR: filename:3: Blue must be orange.\n")
}
// Logf doesn't filter duplicates, but Diag does.
@@ -584,12 +584,12 @@ func (s *Suite) Test_Logger_Logf__duplic
var sw strings.Builder
logger := Logger{out: NewSeparatorWriter(&sw)}
- logger.Logf(Error, "filename", "3", "Blue should be %s.", "Blue should be orange.")
- logger.Logf(Error, "filename", "3", "Blue should be %s.", "Blue should be orange.")
+ logger.Logf(Error, "filename", "3", "Blue must be %s.", "Blue must be orange.")
+ logger.Logf(Error, "filename", "3", "Blue must be %s.", "Blue must be orange.")
t.CheckEquals(sw.String(), ""+
- "ERROR: filename:3: Blue should be orange.\n"+
- "ERROR: filename:3: Blue should be orange.\n")
+ "ERROR: filename:3: Blue must be orange.\n"+
+ "ERROR: filename:3: Blue must be orange.\n")
}
// Ensure that suppressing a diagnostic doesn't influence later calls to Logf.
Index: pkgsrc/pkgtools/pkglint/files/mkassignchecker.go
diff -u pkgsrc/pkgtools/pkglint/files/mkassignchecker.go:1.1 pkgsrc/pkgtools/pkglint/files/mkassignchecker.go:1.2
--- pkgsrc/pkgtools/pkglint/files/mkassignchecker.go:1.1 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mkassignchecker.go Sun Dec 8 22:03:38 2019
@@ -56,7 +56,7 @@ func (ck *MkAssignChecker) checkVarassig
return
}
- if ck.MkLines.vars.IsUsedSimilar(varname) {
+ if ck.MkLines.allVars.IsUsedSimilar(varname) {
return
}
@@ -384,7 +384,7 @@ func (ck *MkAssignChecker) checkVarassig
categories := mkline.ValueFields(mkline.Value())
actual := categories[0]
- expected := G.Pkgsrc.ToRel(mkline.Filename).DirNoClean().DirNoClean().Base()
+ expected := G.Pkgsrc.Rel(mkline.Filename).DirNoClean().DirNoClean().Base()
if expected == "wip" || actual == expected {
return
Index: pkgsrc/pkgtools/pkglint/files/mkassignchecker_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mkassignchecker_test.go:1.1 pkgsrc/pkgtools/pkglint/files/mkassignchecker_test.go:1.2
--- pkgsrc/pkgtools/pkglint/files/mkassignchecker_test.go:1.1 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mkassignchecker_test.go Sun Dec 8 22:03:38 2019
@@ -89,7 +89,7 @@ func (s *Suite) Test_MkAssignChecker_che
MkCvsID,
"_VARNAME=\tvalue")
// Only to prevent "defined but not used".
- mklines.vars.Use("_VARNAME", mklines.mklines[1], VucRunTime)
+ mklines.allVars.Use("_VARNAME", mklines.mklines[1], VucRunTime)
mklines.Check()
@@ -380,7 +380,7 @@ func (s *Suite) Test_MkAssignChecker_che
func (s *Suite) Test_MkAssignChecker_checkVarassignLeftUserSettable__vartype_nil(c *check.C) {
t := s.Init(c)
- t.CreateFileLines("category/package/vars.mk",
+ t.CreateFileLines("category/package/allVars.mk",
MkCvsID,
"#",
"# User-settable variables:",
@@ -651,7 +651,7 @@ func (s *Suite) Test_MkAssignChecker_che
"",
"MUST_BE_EARLY!=\techo 123 # must be evaluated early",
"",
- "show-package-vars: .PHONY",
+ "show-package-allVars: .PHONY",
"\techo OS_NAME=${OS_NAME:Q}",
"\techo MUST_BE_EARLY=${MUST_BE_EARLY:Q}")
t.FinishSetUp()
Index: pkgsrc/pkgtools/pkglint/files/mkcondchecker.go
diff -u pkgsrc/pkgtools/pkglint/files/mkcondchecker.go:1.1 pkgsrc/pkgtools/pkglint/files/mkcondchecker.go:1.2
--- pkgsrc/pkgtools/pkglint/files/mkcondchecker.go:1.1 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mkcondchecker.go Sun Dec 8 22:03:38 2019
@@ -14,7 +14,7 @@ func NewMkCondChecker(mkLine *MkLine, mk
return &MkCondChecker{MkLine: mkLine, MkLines: mkLines}
}
-func (ck *MkCondChecker) checkDirectiveCond() {
+func (ck *MkCondChecker) Check() {
mkline := ck.MkLine
if trace.Tracing {
defer trace.Call1(mkline.Args())()
@@ -39,26 +39,26 @@ func (ck *MkCondChecker) checkDirectiveC
checkNotEmpty := func(not *MkCond) {
empty := not.Empty
if empty != nil {
- ck.checkDirectiveCondEmpty(empty, true, true)
+ ck.checkEmpty(empty, true, true)
done[empty] = true
}
if not.Term != nil && not.Term.Var != nil {
varUse := not.Term.Var
- ck.checkDirectiveCondEmpty(varUse, false, false)
+ ck.checkEmpty(varUse, false, false)
done[varUse] = true
}
}
checkEmpty := func(empty *MkVarUse) {
if !done[empty] {
- ck.checkDirectiveCondEmpty(empty, true, false)
+ ck.checkEmpty(empty, true, false)
}
}
checkVar := func(varUse *MkVarUse) {
if !done[varUse] {
- ck.checkDirectiveCondEmpty(varUse, false, true)
+ ck.checkEmpty(varUse, false, true)
}
}
@@ -66,19 +66,19 @@ func (ck *MkCondChecker) checkDirectiveC
Not: checkNotEmpty,
Empty: checkEmpty,
Var: checkVar,
- Compare: ck.checkDirectiveCondCompare,
+ Compare: ck.checkCompare,
VarUse: checkVarUse})
}
-// checkDirectiveCondEmpty checks a condition of the form empty(VAR),
+// checkEmpty checks a condition of the form empty(VAR),
// empty(VAR:Mpattern) or ${VAR:Mpattern} in an .if directive.
-func (ck *MkCondChecker) checkDirectiveCondEmpty(varuse *MkVarUse, fromEmpty bool, neg bool) {
- ck.checkDirectiveCondEmptyExpr(varuse)
- ck.checkDirectiveCondEmptyType(varuse)
- ck.simplifyCondition(varuse, fromEmpty, neg)
+func (ck *MkCondChecker) checkEmpty(varuse *MkVarUse, fromEmpty bool, neg bool) {
+ ck.checkEmptyExpr(varuse)
+ ck.checkEmptyType(varuse)
+ ck.simplify(varuse, fromEmpty, neg)
}
-func (ck *MkCondChecker) checkDirectiveCondEmptyExpr(varuse *MkVarUse) {
+func (ck *MkCondChecker) checkEmptyExpr(varuse *MkVarUse) {
if !matches(varuse.varname, `^\$.*:[MN]`) {
return
}
@@ -97,7 +97,7 @@ func (ck *MkCondChecker) checkDirectiveC
"\t${VARNAME:Mpattern}")
}
-func (ck *MkCondChecker) checkDirectiveCondEmptyType(varuse *MkVarUse) {
+func (ck *MkCondChecker) checkEmptyType(varuse *MkVarUse) {
for _, modifier := range varuse.modifiers {
ok, _, pattern, _ := modifier.MatchMatch()
if ok {
@@ -123,14 +123,14 @@ var mkCondStringLiteralUnquoted = textpr
// that are interpreted literally in the :M and :N modifiers.
var mkCondModifierPatternLiteral = textproc.NewByteSet("+---./0-9<=>@A-Z_a-z")
-// simplifyCondition replaces an unnecessarily complex condition with
+// simplify 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...}.
//
// * neg is true for the form !empty(VAR...), and false for empty(VAR...).
// It also applies to the ${VAR} form.
-func (ck *MkCondChecker) simplifyCondition(varuse *MkVarUse, fromEmpty bool, neg bool) {
+func (ck *MkCondChecker) simplify(varuse *MkVarUse, fromEmpty bool, neg bool) {
varname := varuse.varname
mods := varuse.modifiers
modifiers := mods
@@ -147,7 +147,8 @@ func (ck *MkCondChecker) simplifyConditi
return true
}
- if ck.MkLines.vars.IsDefined(varname) {
+ // TODO: Use ck.MkLines.loadVars instead.
+ if ck.MkLines.allVars.IsDefined(varname) {
return true
}
@@ -218,7 +219,8 @@ func (ck *MkCondChecker) simplifyConditi
}
fix := ck.MkLine.Autofix()
- fix.Notef("%s should be compared using \"%s\" instead of matching against %q.",
+ fix.Notef("%s can be compared using the simpler \"%s\" "+
+ "instead of matching against %q.",
varname, to, ":"+modifier.Text)
fix.Explain(
"This variable has a single value, not a list of values.",
@@ -232,19 +234,24 @@ func (ck *MkCondChecker) simplifyConditi
fix.Apply()
}
-func (ck *MkCondChecker) checkDirectiveCondCompare(left *MkCondTerm, op string, right *MkCondTerm) {
+func (ck *MkCondChecker) checkCompare(left *MkCondTerm, op string, right *MkCondTerm) {
switch {
case left.Var != nil && right.Var == nil && right.Num == "":
- ck.checkDirectiveCondCompareVarStr(left.Var, op, right.Str)
+ ck.checkCompareVarStr(left.Var, op, right.Str)
}
}
-func (ck *MkCondChecker) checkDirectiveCondCompareVarStr(varuse *MkVarUse, op string, str string) {
+func (ck *MkCondChecker) checkCompareVarStr(varuse *MkVarUse, op string, str string) {
varname := varuse.varname
varmods := varuse.modifiers
switch len(varmods) {
case 0:
- ck.checkCompareVarStr(varname, op, str)
+ mkLineChecker := NewMkLineChecker(ck.MkLines, ck.MkLine)
+ mkLineChecker.checkVartype(varname, opUseCompare, str, "")
+
+ if varname == "PKGSRC_COMPILER" {
+ ck.checkCompareVarStrCompiler(op, str)
+ }
case 1:
if m, _, pattern, _ := varmods[0].MatchMatch(); m {
@@ -272,15 +279,6 @@ func (ck *MkCondChecker) checkDirectiveC
}
}
-func (ck *MkCondChecker) checkCompareVarStr(varname, op, value string) {
- mkLineChecker := NewMkLineChecker(ck.MkLines, ck.MkLine)
- mkLineChecker.checkVartype(varname, opUseCompare, value, "")
-
- if varname == "PKGSRC_COMPILER" {
- ck.checkCompareVarStrCompiler(op, value)
- }
-}
-
func (ck *MkCondChecker) checkCompareVarStrCompiler(op string, value string) {
if !matches(value, `^\w+$`) {
return
Index: pkgsrc/pkgtools/pkglint/files/mkcondchecker_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mkcondchecker_test.go:1.1 pkgsrc/pkgtools/pkglint/files/mkcondchecker_test.go:1.2
--- pkgsrc/pkgtools/pkglint/files/mkcondchecker_test.go:1.1 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mkcondchecker_test.go Sun Dec 8 22:03:38 2019
@@ -14,7 +14,7 @@ func (s *Suite) Test_NewMkCondChecker(c
t.CheckEquals(ck.MkLines, mklines)
}
-func (s *Suite) Test_MkCondChecker_checkDirectiveCond(c *check.C) {
+func (s *Suite) Test_MkCondChecker_Check(c *check.C) {
t := s.Init(c)
t.SetUpPkgsrc()
@@ -65,8 +65,8 @@ func (s *Suite) Test_MkCondChecker_check
// PKG_LIBTOOL is only available after including bsd.pkg.mk,
// therefore the :U and the subsequent warning.
test(".if ${PKG_LIBTOOL:U:Mlibtool}",
- "NOTE: filename.mk:4: PKG_LIBTOOL "+
- "should be compared using \"${PKG_LIBTOOL:U} == libtool\" "+
+ "NOTE: filename.mk:4: PKG_LIBTOOL can be "+
+ "compared using the simpler \"${PKG_LIBTOOL:U} == libtool\" "+
"instead of matching against \":Mlibtool\".",
"WARN: filename.mk:4: PKG_LIBTOOL should not be used at load time in any file.")
@@ -84,8 +84,8 @@ func (s *Suite) Test_MkCondChecker_check
"m68000 m68k m88k mips mips64 mips64eb mips64el mipseb mipsel mipsn32 mlrisc ns32k pc532 pmax "+
"powerpc powerpc64 rs6000 s390 sh3eb sh3el sparc sparc64 vax x86_64 "+
"} for MACHINE_ARCH.",
- "NOTE: filename.mk:4: MACHINE_ARCH "+
- "should be compared using \"${MACHINE_ARCH} == x86\" "+
+ "NOTE: filename.mk:4: MACHINE_ARCH can be "+
+ "compared using the simpler \"${MACHINE_ARCH} == x86\" "+
"instead of matching against \":Mx86\".")
// Doesn't occur in practice since it is surprising that the ! applies
@@ -127,7 +127,7 @@ func (s *Suite) Test_MkCondChecker_check
"WARN: filename.mk:4: MASTER_SITES should not be used at load time in any file.")
}
-func (s *Suite) Test_MkCondChecker_checkDirectiveCond__tracing(c *check.C) {
+func (s *Suite) Test_MkCondChecker_Check__tracing(c *check.C) {
t := s.Init(c)
t.EnableTracingToLog()
@@ -143,7 +143,7 @@ func (s *Suite) Test_MkCondChecker_check
"WARN: filename.mk:2: VAR is used but not defined.")
}
-func (s *Suite) Test_MkCondChecker_checkDirectiveCond__comparison_with_shell_command(c *check.C) {
+func (s *Suite) Test_MkCondChecker_Check__comparison_with_shell_command(c *check.C) {
t := s.Init(c)
t.SetUpPkgsrc()
@@ -167,7 +167,7 @@ func (s *Suite) Test_MkCondChecker_check
// The :N modifier filters unwanted values. After this filter, any variable value
// may be compared with the empty string, regardless of the variable type.
// Effectively, the :N modifier changes the type from T to Option(T).
-func (s *Suite) Test_MkCondChecker_checkDirectiveCond__compare_pattern_with_empty(c *check.C) {
+func (s *Suite) Test_MkCondChecker_Check__compare_pattern_with_empty(c *check.C) {
t := s.Init(c)
t.SetUpPkgsrc()
@@ -196,7 +196,7 @@ func (s *Suite) Test_MkCondChecker_check
"WARN: filename.mk:8: The pathname \"*\" contains the invalid character \"*\".")
}
-func (s *Suite) Test_MkCondChecker_checkDirectiveCond__comparing_PKGSRC_COMPILER_with_eqeq(c *check.C) {
+func (s *Suite) Test_MkCondChecker_Check__comparing_PKGSRC_COMPILER_with_eqeq(c *check.C) {
t := s.Init(c)
t.SetUpPkgsrc()
@@ -218,7 +218,7 @@ func (s *Suite) Test_MkCondChecker_check
"ERROR: Makefile:6: Use ${PKGSRC_COMPILER:Ngcc} instead of the != operator.")
}
-func (s *Suite) Test_MkCondChecker_checkDirectiveCondEmpty(c *check.C) {
+func (s *Suite) Test_MkCondChecker_checkEmpty(c *check.C) {
t := s.Init(c)
t.SetUpPackage("category/package")
@@ -236,7 +236,7 @@ func (s *Suite) Test_MkCondChecker_check
".endif")
t.ExpectDiagnosticsAutofix(
- mklines.Check,
+ func(autofix bool) { mklines.Check() },
diagnostics...)
afterMklines := LoadMk(t.File("filename.mk"), MustSucceed)
@@ -249,8 +249,8 @@ func (s *Suite) Test_MkCondChecker_check
"WARN: filename.mk:3: The pattern \"Unknown\" cannot match any of "+
"{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS } for OPSYS.",
- "NOTE: filename.mk:3: OPSYS should be "+
- "compared using \"${OPSYS:U} == Unknown\" "+
+ "NOTE: filename.mk:3: OPSYS can be "+
+ "compared using the simpler \"${OPSYS:U} == Unknown\" "+
"instead of matching against \":MUnknown\".",
// TODO: Turn the bsd.prefs.mk warning into an error,
// once pkglint is confident enough to get this check right.
@@ -271,7 +271,7 @@ func (s *Suite) Test_MkCondChecker_check
".include \"../../mk/bsd.prefs.mk\" first.")
}
-func (s *Suite) Test_MkCondChecker_checkDirectiveCondEmptyExpr(c *check.C) {
+func (s *Suite) Test_MkCondChecker_checkEmptyExpr(c *check.C) {
t := s.Init(c)
test := func(use *MkVarUse, diagnostics ...string) {
@@ -279,7 +279,7 @@ func (s *Suite) Test_MkCondChecker_check
"# dummy")
ck := NewMkCondChecker(mklines.mklines[0], mklines)
- ck.checkDirectiveCondEmptyExpr(use)
+ ck.checkEmptyExpr(use)
t.CheckOutput(diagnostics)
}
@@ -305,7 +305,7 @@ func (s *Suite) Test_MkCondChecker_check
"name as parameter, not a variable expression.")
}
-func (s *Suite) Test_MkCondChecker_checkDirectiveCondEmptyType(c *check.C) {
+func (s *Suite) Test_MkCondChecker_checkEmptyType(c *check.C) {
t := s.Init(c)
t.SetUpPackage("category/package")
@@ -322,7 +322,7 @@ func (s *Suite) Test_MkCondChecker_check
mklines.ForEach(func(mkline *MkLine) {
ck := NewMkCondChecker(mkline, mklines)
mkline.ForEachUsed(func(varUse *MkVarUse, time VucTime) {
- ck.checkDirectiveCondEmptyType(varUse)
+ ck.checkEmptyType(varUse)
})
})
@@ -364,226 +364,366 @@ func (s *Suite) Test_MkCondChecker_check
nil...)
}
-func (s *Suite) Test_MkCondChecker_simplifyCondition(c *check.C) {
+func (s *Suite) Test_MkCondChecker_simplify(c *check.C) {
t := s.Init(c)
- t.SetUpPackage("category/package")
+ t.CreateFileLines("mk/bsd.prefs.mk")
t.Chdir("category/package")
- t.FinishSetUp()
+
+ // The Anything type suppresses the warnings from type checking.
+ // BtUnknown would not work, see Pkgsrc.VariableType.
+ btAnything := &BasicType{"Anything", func(cv *VartypeCheck) {}}
+
+ // For simplifying the expressions, it is necessary to know whether
+ // a variable can be undefined. Undefined variables need the
+ // :U modifier, otherwise bmake will complain about "malformed
+ // conditions", which is not entirely precise since the expression
+ // is syntactically valid, it's just the evaluation that fails.
+ //
+ // Some variables such as MACHINE_ARCH are in scope from the very
+ // beginning.
+ //
+ // Some variables such as PKGPATH are in scope after bsd.prefs.mk
+ // has been included.
+ //
+ // Some variables such as PREFIX (as of December 2019) are only in
+ // scope after bsd.pkg.mk has been included. These cannot be used
+ // in .if conditions at all.
+ //
+ // Even when they are in scope, some variables such as PKGREVISION
+ // or MAKE_JOBS may be undefined.
+
+ t.SetUpType("IN_SCOPE_DEFINED", btAnything, AlwaysInScope|DefinedIfInScope,
+ "*.mk: use, use-loadtime")
+ t.SetUpType("IN_SCOPE", btAnything, AlwaysInScope,
+ "*.mk: use, use-loadtime")
+ t.SetUpType("PREFS_DEFINED", btAnything, DefinedIfInScope,
+ "*.mk: use, use-loadtime")
+ t.SetUpType("PREFS", btAnything, NoVartypeOptions,
+ "*.mk: use, use-loadtime")
+ t.SetUpType("LATER_DEFINED", btAnything, DefinedIfInScope,
+ "*.mk: use")
+ t.SetUpType("LATER", btAnything, NoVartypeOptions,
+ "*.mk: use")
+ // UNDEFINED is also used in the following tests, but is obviously
+ // not defined here.
// prefs: whether to include bsd.prefs.mk before
// before: the directive before the condition is simplified
// after: the directive after the condition is simplified
- test := func(prefs bool, before, after string, diagnostics ...string) {
+ doTest := func(prefs bool, before, after string, diagnostics ...string) {
+ if !matches(before, `IN_SCOPE|PREFS|LATER|UNDEFINED`) {
+ c.Errorf("Condition %q must include one of the above variable names.", before)
+ }
mklines := t.SetUpFileMkLines("filename.mk",
MkCvsID,
condStr(prefs, ".include \"../../mk/bsd.prefs.mk\"", ""),
before,
".endif")
- // The high-level call MkLines.Check is used here to
- // get MkLines.Tools.SeenPrefs correct, which decides
- // whether the :U modifier is necessary.
- //
- // TODO: Replace MkLines.Check this with a more specific method.
+ action := func(autofix bool) {
+ mklines.ForEach(func(mkline *MkLine) {
+ // Sets mklines.Tools.SeenPrefs, which decides whether the :U modifier
+ // is necessary; see MkLines.checkLine.
+ mklines.Tools.ParseToolLine(mklines, mkline, false, false)
+
+ if mkline.IsDirective() && mkline.Directive() != "endif" {
+ // TODO: Replace Check with a more
+ // specific method that does not do the type checks.
+ NewMkCondChecker(mkline, mklines).Check()
+ }
+ })
- t.ExpectDiagnosticsAutofix(
- mklines.Check,
- diagnostics...)
+ if autofix {
+ afterMklines := LoadMk(t.File("filename.mk"), MustSucceed)
+ t.CheckEquals(afterMklines.mklines[2].Text, after)
+ }
+ }
- // TODO: Move this assertion above the assertion about the diagnostics.
- afterMklines := LoadMk(t.File("filename.mk"), MustSucceed)
- t.CheckEquals(afterMklines.mklines[2].Text, after)
+ t.ExpectDiagnosticsAutofix(action, diagnostics...)
+ }
+
+ testBeforePrefs := func(before, after string, diagnostics ...string) {
+ doTest(false, before, after, diagnostics...)
}
testAfterPrefs := func(before, after string, diagnostics ...string) {
- test(true, before, after, diagnostics...)
+ doTest(true, before, after, diagnostics...)
+ }
+ testBeforeAndAfterPrefs := func(before, after string, diagnostics ...string) {
+ doTest(true, before, after, diagnostics...)
}
- test(
- false,
- ".if ${PKGPATH:Mpattern}",
- ".if ${PKGPATH:U} == pattern",
+ testBeforeAndAfterPrefs(
+ ".if ${IN_SCOPE_DEFINED:Mpattern}",
+ ".if ${IN_SCOPE_DEFINED} == pattern",
+
+ "NOTE: filename.mk:3: IN_SCOPE_DEFINED can be "+
+ "compared using the simpler \"${IN_SCOPE_DEFINED} == pattern\" "+
+ "instead of matching against \":Mpattern\".",
+ "AUTOFIX: filename.mk:3: Replacing \"${IN_SCOPE_DEFINED:Mpattern}\" "+
+ "with \"${IN_SCOPE_DEFINED} == pattern\".")
+
+ testBeforeAndAfterPrefs(
+ ".if ${IN_SCOPE:Mpattern}",
+ ".if ${IN_SCOPE:U} == pattern",
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH:U} == pattern\" "+
+ "NOTE: filename.mk:3: IN_SCOPE can be "+
+ "compared using the simpler \"${IN_SCOPE:U} == pattern\" "+
"instead of matching against \":Mpattern\".",
- "WARN: filename.mk:3: To use PKGPATH at load time, "+
+ "AUTOFIX: filename.mk:3: Replacing \"${IN_SCOPE:Mpattern}\" "+
+ "with \"${IN_SCOPE:U} == pattern\".")
+
+ // Even though PREFS_DEFINED is declared as DefinedIfInScope,
+ // it is not in scope yet. Therefore it needs the :U modifier.
+ // The warning that this variable is not yet in scope comes from
+ // a different part of pkglint.
+ testBeforePrefs(
+ ".if ${PREFS_DEFINED:Mpattern}",
+ ".if ${PREFS_DEFINED:U} == pattern",
+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED:U} == pattern\" "+
+ "instead of matching against \":Mpattern\".",
+ "WARN: filename.mk:3: To use PREFS_DEFINED at load time, "+
".include \"../../mk/bsd.prefs.mk\" first.",
- "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Mpattern}\" "+
- "with \"${PKGPATH:U} == pattern\".")
+ "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Mpattern}\" "+
+ "with \"${PREFS_DEFINED:U} == pattern\".")
testAfterPrefs(
- ".if ${PKGPATH:Mpattern}",
- ".if ${PKGPATH} == pattern",
+ ".if ${PREFS_DEFINED:Mpattern}",
+ ".if ${PREFS_DEFINED} == pattern",
+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} == pattern\" "+
+ "instead of matching against \":Mpattern\".",
+ "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Mpattern}\" "+
+ "with \"${PREFS_DEFINED} == pattern\".")
+
+ testBeforePrefs(
+ ".if ${PREFS:Mpattern}",
+ ".if ${PREFS:U} == pattern",
+
+ "NOTE: filename.mk:3: PREFS can be "+
+ "compared using the simpler \"${PREFS:U} == pattern\" "+
+ "instead of matching against \":Mpattern\".",
+ "WARN: filename.mk:3: To use PREFS at load time, "+
+ ".include \"../../mk/bsd.prefs.mk\" first.",
+ "AUTOFIX: filename.mk:3: Replacing \"${PREFS:Mpattern}\" "+
+ "with \"${PREFS:U} == pattern\".")
+
+ // Preferences that may be undefined always need the :U modifier,
+ // even when they are in scope.
+ testAfterPrefs(
+ ".if ${PREFS:Mpattern}",
+ ".if ${PREFS:U} == pattern",
+
+ "NOTE: filename.mk:3: PREFS can be "+
+ "compared using the simpler \"${PREFS:U} == pattern\" "+
+ "instead of matching against \":Mpattern\".",
+ "AUTOFIX: filename.mk:3: Replacing \"${PREFS:Mpattern}\" "+
+ "with \"${PREFS:U} == pattern\".")
+
+ // Variables that are defined later always need the :U modifier.
+ // It is probably a mistake to use them in conditions at all.
+ testBeforeAndAfterPrefs(
+ ".if ${LATER_DEFINED:Mpattern}",
+ ".if ${LATER_DEFINED:U} == pattern",
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH} == pattern\" "+
+ "NOTE: filename.mk:3: LATER_DEFINED can be "+
+ "compared using the simpler \"${LATER_DEFINED:U} == pattern\" "+
"instead of matching against \":Mpattern\".",
- "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Mpattern}\" "+
- "with \"${PKGPATH} == pattern\".")
+ "WARN: filename.mk:3: "+
+ "LATER_DEFINED should not be used at load time in any file.",
+ "AUTOFIX: filename.mk:3: Replacing \"${LATER_DEFINED:Mpattern}\" "+
+ "with \"${LATER_DEFINED:U} == pattern\".")
+
+ // Variables that are defined later always need the :U modifier.
+ // It is probably a mistake to use them in conditions at all.
+ testBeforeAndAfterPrefs(
+ ".if ${LATER:Mpattern}",
+ ".if ${LATER:U} == pattern",
+
+ "NOTE: filename.mk:3: LATER can be "+
+ "compared using the simpler \"${LATER:U} == pattern\" "+
+ "instead of matching against \":Mpattern\".",
+ "WARN: filename.mk:3: "+
+ "LATER should not be used at load time in any file.",
+ "AUTOFIX: filename.mk:3: Replacing \"${LATER:Mpattern}\" "+
+ "with \"${LATER:U} == pattern\".")
+
+ testBeforeAndAfterPrefs(
+ ".if ${UNDEFINED:Mpattern}",
+ ".if ${UNDEFINED:Mpattern}",
+
+ "WARN: filename.mk:3: UNDEFINED is used but not defined.")
// When the pattern contains placeholders, it cannot be converted to == or !=.
testAfterPrefs(
- ".if ${PKGPATH:Mpa*n}",
- ".if ${PKGPATH:Mpa*n}",
+ ".if ${PREFS_DEFINED:Mpa*n}",
+ ".if ${PREFS_DEFINED:Mpa*n}",
nil...)
// When deciding whether to replace the expression, only the
// last modifier is inspected. All the others are copied.
testAfterPrefs(
- ".if ${PKGPATH:tl:Mpattern}",
- ".if ${PKGPATH:tl} == pattern",
+ ".if ${PREFS_DEFINED:tl:Mpattern}",
+ ".if ${PREFS_DEFINED:tl} == pattern",
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH:tl} == pattern\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED:tl} == pattern\" "+
"instead of matching against \":Mpattern\".",
- "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:tl:Mpattern}\" "+
- "with \"${PKGPATH:tl} == pattern\".")
+ "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:tl:Mpattern}\" "+
+ "with \"${PREFS_DEFINED:tl} == pattern\".")
// Negated pattern matches are supported as well,
// as long as the variable is guaranteed to be nonempty.
+ // TODO: Actually implement this.
+ // As of December 2019, IsNonemptyIfDefined is not used anywhere.
testAfterPrefs(
- ".if ${PKGPATH:Ncategory/package}",
- ".if ${PKGPATH} != category/package",
+ ".if ${PREFS_DEFINED:Npattern}",
+ ".if ${PREFS_DEFINED} != pattern",
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH} != category/package\" "+
- "instead of matching against \":Ncategory/package\".",
- "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Ncategory/package}\" "+
- "with \"${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.
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} != pattern\" "+
+ "instead of matching against \":Npattern\".",
+ "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Npattern}\" "+
+ "with \"${PREFS_DEFINED} != pattern\".")
+
+ // ${PREFS_DEFINED:None:Ntwo} is a short variant of
+ // ${PREFS_DEFINED} != "one" && ${PREFS_DEFINED} != "two".
+ // Applying the transformation would make the condition longer
+ // than before, therefore nothing can be simplified here,
+ // even though all patterns are exact matches.
testAfterPrefs(
- ".if ${PKGPATH:None:Ntwo}",
- ".if ${PKGPATH:None:Ntwo}",
+ ".if ${PREFS_DEFINED:None:Ntwo}",
+ ".if ${PREFS_DEFINED:None:Ntwo}",
nil...)
// Note: this combination doesn't make sense since the patterns
// "one" and "two" don't overlap.
+ // Nevertheless it is possible and valid to simplify the condition.
testAfterPrefs(
- ".if ${PKGPATH:Mone:Mtwo}",
- ".if ${PKGPATH:Mone} == two",
+ ".if ${PREFS_DEFINED:Mone:Mtwo}",
+ ".if ${PREFS_DEFINED:Mone} == two",
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH:Mone} == two\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED:Mone} == two\" "+
"instead of matching against \":Mtwo\".",
- "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Mone:Mtwo}\" "+
- "with \"${PKGPATH:Mone} == two\".")
+ "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Mone:Mtwo}\" "+
+ "with \"${PREFS_DEFINED:Mone} == two\".")
+ // There is no ! before the empty, which is easy to miss.
+ // Because of this missing negation, the comparison operator is !=.
testAfterPrefs(
- ".if !empty(PKGPATH:Mpattern)",
- ".if ${PKGPATH} == pattern",
+ ".if empty(PREFS_DEFINED:Mpattern)",
+ ".if ${PREFS_DEFINED} != pattern",
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH} == pattern\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} != pattern\" "+
"instead of matching against \":Mpattern\".",
- "AUTOFIX: filename.mk:3: Replacing \"!empty(PKGPATH:Mpattern)\" "+
- "with \"${PKGPATH} == pattern\".")
+ "AUTOFIX: filename.mk:3: Replacing \"empty(PREFS_DEFINED:Mpattern)\" "+
+ "with \"${PREFS_DEFINED} != pattern\".")
testAfterPrefs(
- ".if empty(PKGPATH:Mpattern)",
- ".if ${PKGPATH} != pattern",
-
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH} != pattern\" "+
- "instead of matching against \":Mpattern\".",
- "AUTOFIX: filename.mk:3: Replacing \"empty(PKGPATH:Mpattern)\" "+
- "with \"${PKGPATH} != pattern\".")
-
- testAfterPrefs(
- ".if !!empty(PKGPATH:Mpattern)",
+ ".if !!empty(PREFS_DEFINED:Mpattern)",
// TODO: The ! and == could be combined into a !=.
// Luckily the !! pattern doesn't occur in practice.
- ".if !${PKGPATH} == pattern",
+ ".if !${PREFS_DEFINED} == pattern",
// TODO: When taking all the ! into account, this is actually a
// test for emptiness, therefore the diagnostics should suggest
// the != operator instead of ==.
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH} == pattern\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} == pattern\" "+
"instead of matching against \":Mpattern\".",
- "AUTOFIX: filename.mk:3: Replacing \"!empty(PKGPATH:Mpattern)\" "+
- "with \"${PKGPATH} == pattern\".")
+ "AUTOFIX: filename.mk:3: Replacing \"!empty(PREFS_DEFINED:Mpattern)\" "+
+ "with \"${PREFS_DEFINED} == pattern\".")
- testAfterPrefs(".if empty(PKGPATH:Mpattern) || 0",
- ".if ${PKGPATH} != pattern || 0",
+ // Simplifying the condition also works in complex expressions.
+ testAfterPrefs(".if empty(PREFS_DEFINED:Mpattern) || 0",
+ ".if ${PREFS_DEFINED} != pattern || 0",
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH} != pattern\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} != pattern\" "+
"instead of matching against \":Mpattern\".",
- "AUTOFIX: filename.mk:3: Replacing \"empty(PKGPATH:Mpattern)\" "+
- "with \"${PKGPATH} != pattern\".")
+ "AUTOFIX: filename.mk:3: Replacing \"empty(PREFS_DEFINED:Mpattern)\" "+
+ "with \"${PREFS_DEFINED} != pattern\".")
// No note in this case since there is no implicit !empty around the varUse.
+ // There is no obvious way of writing this expression in a simpler form.
testAfterPrefs(
- ".if ${PKGPATH:Mpattern} != ${OTHER}",
- ".if ${PKGPATH:Mpattern} != ${OTHER}",
+ ".if ${PREFS_DEFINED:Mpattern} != ${OTHER}",
+ ".if ${PREFS_DEFINED:Mpattern} != ${OTHER}",
"WARN: filename.mk:3: OTHER is used but not defined.")
+ // The condition is also simplified if it doesn't use the !empty
+ // form but the implicit conversion to boolean.
testAfterPrefs(
- ".if ${PKGPATH:Mpattern}",
- ".if ${PKGPATH} == pattern",
+ ".if ${PREFS_DEFINED:Mpattern}",
+ ".if ${PREFS_DEFINED} == pattern",
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH} == pattern\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} == pattern\" "+
"instead of matching against \":Mpattern\".",
- "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Mpattern}\" "+
- "with \"${PKGPATH} == pattern\".")
+ "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Mpattern}\" "+
+ "with \"${PREFS_DEFINED} == pattern\".")
+ // A single negation outside the implicit conversion is taken into
+ // account when simplifying the condition.
testAfterPrefs(
- ".if !${PKGPATH:Mpattern}",
- ".if ${PKGPATH} != pattern",
+ ".if !${PREFS_DEFINED:Mpattern}",
+ ".if ${PREFS_DEFINED} != pattern",
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH} != pattern\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} != pattern\" "+
"instead of matching against \":Mpattern\".",
- "AUTOFIX: filename.mk:3: Replacing \"!${PKGPATH:Mpattern}\" "+
- "with \"${PKGPATH} != pattern\".")
+ "AUTOFIX: filename.mk:3: Replacing \"!${PREFS_DEFINED:Mpattern}\" "+
+ "with \"${PREFS_DEFINED} != pattern\".")
// TODO: Merge the double negation into the comparison operator.
testAfterPrefs(
- ".if !!${PKGPATH:Mpattern}",
- ".if !${PKGPATH} != pattern",
+ ".if !!${PREFS_DEFINED:Mpattern}",
+ ".if !${PREFS_DEFINED} != pattern",
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH} != pattern\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} != pattern\" "+
"instead of matching against \":Mpattern\".",
- "AUTOFIX: filename.mk:3: Replacing \"!${PKGPATH:Mpattern}\" "+
- "with \"${PKGPATH} != pattern\".")
+ "AUTOFIX: filename.mk:3: Replacing \"!${PREFS_DEFINED:Mpattern}\" "+
+ "with \"${PREFS_DEFINED} != pattern\".")
// This pattern with spaces doesn't make sense at all in the :M
// modifier since it can never match.
// Or can it, if the PKGPATH contains quotes?
- // How exactly does bmake apply the matching here, are both values unquoted?
- testAfterPrefs(
- ".if ${PKGPATH:Mpattern with spaces}",
- ".if ${PKGPATH:Mpattern with spaces}",
+ // TODO: How exactly does bmake apply the matching here,
+ // are both values unquoted first? Probably not, but who knows.
+ testBeforeAndAfterPrefs(
+ ".if ${IN_SCOPE_DEFINED:Mpattern with spaces}",
+ ".if ${IN_SCOPE_DEFINED:Mpattern with spaces}",
- "WARN: filename.mk:3: The pathname pattern \"pattern with spaces\" "+
- "contains the invalid characters \" \".")
+ nil...)
// TODO: ".if ${PKGPATH} == \"pattern with spaces\"")
- testAfterPrefs(
- ".if ${PKGPATH:M'pattern with spaces'}",
- ".if ${PKGPATH:M'pattern with spaces'}",
+ testBeforeAndAfterPrefs(
+ ".if ${IN_SCOPE_DEFINED:M'pattern with spaces'}",
+ ".if ${IN_SCOPE_DEFINED:M'pattern with spaces'}",
- "WARN: filename.mk:3: The pathname pattern \"'pattern with spaces'\" "+
- "contains the invalid characters \"' '\".")
+ nil...)
// TODO: ".if ${PKGPATH} == 'pattern with spaces'")
- testAfterPrefs(
- ".if ${PKGPATH:M&&}",
- ".if ${PKGPATH:M&&}",
+ testBeforeAndAfterPrefs(
+ ".if ${IN_SCOPE_DEFINED:M&&}",
+ ".if ${IN_SCOPE_DEFINED:M&&}",
- "WARN: filename.mk:3: The pathname pattern \"&&\" "+
- "contains the invalid characters \"&&\".")
+ nil...)
// TODO: ".if ${PKGPATH} == '&&'")
+ // The :N modifier involves another negation and is therefore more
+ // difficult to understand. That's even more reason to use the
+ // well-known == and != comparison operators instead.
+ //
// If PKGPATH is "", the condition is false.
// If PKGPATH is "negative-pattern", the condition is false.
// In all other cases, the condition is true.
@@ -596,222 +736,156 @@ func (s *Suite) Test_MkCondChecker_simpl
// 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.
+ //
+ // TODO: This is where NonemptyIfDefined comes into play.
testAfterPrefs(
- ".if ${PKGPATH:Nnegative-pattern}",
- ".if ${PKGPATH} != negative-pattern",
+ ".if ${PREFS_DEFINED:Nnegative-pattern}",
+ ".if ${PREFS_DEFINED} != negative-pattern",
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH} != negative-pattern\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} != negative-pattern\" "+
"instead of matching against \":Nnegative-pattern\".",
- "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Nnegative-pattern}\" "+
- "with \"${PKGPATH} != negative-pattern\".")
+ "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Nnegative-pattern}\" "+
+ "with \"${PREFS_DEFINED} != negative-pattern\".")
- // Since UNKNOWN is not a well-known system-provided variable that is
+ // Since UNDEFINED is not a well-known variable that is
// guaranteed to be non-empty (see the previous example), it is not
// transformed at all.
- test(
- false,
- ".if ${UNKNOWN:Nnegative-pattern}",
- ".if ${UNKNOWN:Nnegative-pattern}",
+ testBeforePrefs(
+ ".if ${UNDEFINED:Nnegative-pattern}",
+ ".if ${UNDEFINED:Nnegative-pattern}",
- "WARN: filename.mk:3: UNKNOWN is used but not defined.")
+ "WARN: filename.mk:3: UNDEFINED is used but not defined.")
- test(
- true,
- ".if ${UNKNOWN:Nnegative-pattern}",
- ".if ${UNKNOWN:Nnegative-pattern}",
+ testAfterPrefs(
+ ".if ${UNDEFINED:Nnegative-pattern}",
+ ".if ${UNDEFINED:Nnegative-pattern}",
- "WARN: filename.mk:3: UNKNOWN is used but not defined.")
+ "WARN: filename.mk:3: UNDEFINED is used but not defined.")
+ // A complex condition may contain several simple conditions
+ // that are each simplified independently, in the same go.
testAfterPrefs(
- ".if ${PKGPATH:Mpath1} || ${PKGPATH:Mpath2}",
- ".if ${PKGPATH} == path1 || ${PKGPATH} == path2",
+ ".if ${PREFS_DEFINED:Mpath1} || ${PREFS_DEFINED:Mpath2}",
+ ".if ${PREFS_DEFINED} == path1 || ${PREFS_DEFINED} == path2",
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH} == path1\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} == path1\" "+
"instead of matching against \":Mpath1\".",
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH} == path2\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} == path2\" "+
"instead of matching against \":Mpath2\".",
- "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Mpath1}\" "+
- "with \"${PKGPATH} == path1\".",
- "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Mpath2}\" "+
- "with \"${PKGPATH} == path2\".")
-
+ "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Mpath1}\" "+
+ "with \"${PREFS_DEFINED} == path1\".",
+ "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Mpath2}\" "+
+ "with \"${PREFS_DEFINED} == path2\".")
+
+ // Removing redundant parentheses may be useful one day but is
+ // not urgent.
+ // Simplifying the inner expression keeps all parentheses as-is.
testAfterPrefs(
- ".if (((((${PKGPATH:Mpath})))))",
- ".if (((((${PKGPATH} == path)))))",
+ ".if (((((${PREFS_DEFINED:Mpath})))))",
+ ".if (((((${PREFS_DEFINED} == path)))))",
- "NOTE: filename.mk:3: PKGPATH "+
- "should be compared using \"${PKGPATH} == path\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} == path\" "+
"instead of matching against \":Mpath\".",
- "AUTOFIX: filename.mk:3: Replacing \"${PKGPATH:Mpath}\" "+
- "with \"${PKGPATH} == path\".")
-
- // MACHINE_ARCH is built-in into bmake and is always available.
- // Therefore it doesn't matter whether bsd.prefs.mk is included or not.
- test(
- false,
- ".if ${MACHINE_ARCH:Mx86_64}",
- ".if ${MACHINE_ARCH} == x86_64",
-
- "NOTE: filename.mk:3: MACHINE_ARCH "+
- "should be compared using \"${MACHINE_ARCH} == x86_64\" "+
- "instead of matching against \":Mx86_64\".",
- "AUTOFIX: filename.mk:3: Replacing \"${MACHINE_ARCH:Mx86_64}\" "+
- "with \"${MACHINE_ARCH} == x86_64\".")
-
- // MACHINE_ARCH is built-in into bmake and is always available.
- // Therefore it doesn't matter whether bsd.prefs.mk is included or not.
- test(
- true,
- ".if ${MACHINE_ARCH:Mx86_64}",
- ".if ${MACHINE_ARCH} == x86_64",
-
- "NOTE: filename.mk:3: MACHINE_ARCH "+
- "should be compared using \"${MACHINE_ARCH} == x86_64\" "+
- "instead of matching against \":Mx86_64\".",
- "AUTOFIX: filename.mk:3: Replacing \"${MACHINE_ARCH:Mx86_64}\" "+
- "with \"${MACHINE_ARCH} == x86_64\".")
-
- test(
- false,
- ".if !empty(OPSYS:MUnknown)",
- ".if ${OPSYS:U} == Unknown",
-
- // FIXME: This warning is not the job of simplifyCondition.
- // Therefore don't test it here.
- "WARN: filename.mk:3: The pattern \"Unknown\" cannot match any of "+
- "{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS } for OPSYS.",
- "NOTE: filename.mk:3: OPSYS should be "+
- "compared using \"${OPSYS:U} == Unknown\" "+
- "instead of matching against \":MUnknown\".",
- "WARN: filename.mk:3: To use OPSYS at load time, "+
- ".include \"../../mk/bsd.prefs.mk\" first.",
- "AUTOFIX: filename.mk:3: Replacing \"!empty(OPSYS:MUnknown)\" "+
- "with \"${OPSYS:U} == Unknown\".")
+ "AUTOFIX: filename.mk:3: Replacing \"${PREFS_DEFINED:Mpath}\" "+
+ "with \"${PREFS_DEFINED} == path\".")
+ // Several modifiers like :S and :C may change the variable value.
+ // Whether the condition can be simplified or not only depends on the
+ // last modifier in the chain.
testAfterPrefs(
- ".if !empty(OPSYS:S,NetBSD,ok,:Mok)",
- ".if ${OPSYS:S,NetBSD,ok,} == ok",
+ ".if !empty(PREFS_DEFINED:S,NetBSD,ok,:Mok)",
+ ".if ${PREFS_DEFINED:S,NetBSD,ok,} == ok",
- "NOTE: filename.mk:3: OPSYS should be "+
- "compared using \"${OPSYS:S,NetBSD,ok,} == ok\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED:S,NetBSD,ok,} == ok\" "+
"instead of matching against \":Mok\".",
- "AUTOFIX: filename.mk:3: Replacing \"!empty(OPSYS:S,NetBSD,ok,:Mok)\" "+
- "with \"${OPSYS:S,NetBSD,ok,} == ok\".")
+ "AUTOFIX: filename.mk:3: Replacing \"!empty(PREFS_DEFINED:S,NetBSD,ok,:Mok)\" "+
+ "with \"${PREFS_DEFINED:S,NetBSD,ok,} == ok\".")
testAfterPrefs(
- ".if empty(OPSYS:tl:Msunos)",
- ".if ${OPSYS:tl} != sunos",
+ ".if empty(PREFS_DEFINED:tl:Msunos)",
+ ".if ${PREFS_DEFINED:tl} != sunos",
- "NOTE: filename.mk:3: OPSYS should be "+
- "compared using \"${OPSYS:tl} != sunos\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED:tl} != sunos\" "+
"instead of matching against \":Msunos\".",
- "AUTOFIX: filename.mk:3: Replacing \"empty(OPSYS:tl:Msunos)\" "+
- "with \"${OPSYS:tl} != sunos\".")
+ "AUTOFIX: filename.mk:3: Replacing \"empty(PREFS_DEFINED:tl:Msunos)\" "+
+ "with \"${PREFS_DEFINED:tl} != sunos\".")
+ // The condition can only be simplified if the :M or :N modifier
+ // appears at the end of the chain.
testAfterPrefs(
- ".if !empty(OPSYS:O:MUnknown:S,a,b,)",
- ".if !empty(OPSYS:O:MUnknown:S,a,b,)",
+ ".if !empty(PREFS_DEFINED:O:MUnknown:S,a,b,)",
+ ".if !empty(PREFS_DEFINED:O:MUnknown:S,a,b,)",
- "WARN: filename.mk:3: The pattern \"Unknown\" cannot match any of "+
- "{ Cygwin DragonFly FreeBSD Linux NetBSD SunOS } for OPSYS.")
+ nil...)
- // The dot is just an ordinary character.
- // It's only special when used in number literals.
+ // The dot is just an ordinary character in a pattern.
+ // In comparisons, an unquoted 1.2 is interpreted as a number though.
testAfterPrefs(
- ".if !empty(PKGPATH:Mcategory/package1.2)",
- ".if ${PKGPATH} == category/package1.2",
+ ".if !empty(PREFS_DEFINED:Mpackage1.2)",
+ ".if ${PREFS_DEFINED} == package1.2",
- "NOTE: filename.mk:3: PKGPATH should be "+
- "compared using \"${PKGPATH} == category/package1.2\" "+
- "instead of matching against \":Mcategory/package1.2\".",
- "AUTOFIX: filename.mk:3: Replacing \"!empty(PKGPATH:Mcategory/package1.2)\" "+
- "with \"${PKGPATH} == category/package1.2\".")
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} == package1.2\" "+
+ "instead of matching against \":Mpackage1.2\".",
+ "AUTOFIX: filename.mk:3: Replacing \"!empty(PREFS_DEFINED:Mpackage1.2)\" "+
+ "with \"${PREFS_DEFINED} == package1.2\".")
// Numbers must be enclosed in quotes, otherwise they are compared
- // as numbers, not as strings. The :M and :N modifiers always compare
- // strings.
+ // as numbers, not as strings.
+ // The :M and :N modifiers always compare strings.
testAfterPrefs(
- ".if empty(ABI:U:M64)",
- ".if ${ABI:U} != \"64\"",
+ ".if empty(PREFS:U:M64)",
+ ".if ${PREFS:U} != \"64\"",
- "NOTE: filename.mk:3: ABI should be compared using \"${ABI:U} != \"64\"\" "+
+ "NOTE: filename.mk:3: PREFS can be "+
+ "compared using the simpler \"${PREFS:U} != \"64\"\" "+
"instead of matching against \":M64\".",
- "AUTOFIX: filename.mk:3: Replacing \"empty(ABI:U:M64)\" "+
- "with \"${ABI:U} != \\\"64\\\"\".")
+ "AUTOFIX: filename.mk:3: Replacing \"empty(PREFS:U:M64)\" "+
+ "with \"${PREFS:U} != \\\"64\\\"\".")
// Fractional numbers must also be enclosed in quotes.
testAfterPrefs(
- ".if empty(PKGVERSION_NOREV:U:M19.12)",
- ".if ${PKGVERSION_NOREV:U} != \"19.12\"",
+ ".if empty(PREFS:U:M19.12)",
+ ".if ${PREFS:U} != \"19.12\"",
- "NOTE: filename.mk:3: PKGVERSION_NOREV should be "+
- "compared using \"${PKGVERSION_NOREV:U} != \"19.12\"\" "+
+ "NOTE: filename.mk:3: PREFS can be "+
+ "compared using the simpler \"${PREFS:U} != \"19.12\"\" "+
"instead of matching against \":M19.12\".",
- "WARN: filename.mk:3: PKGVERSION_NOREV should not be used at load time in any file.",
- "AUTOFIX: filename.mk:3: Replacing \"empty(PKGVERSION_NOREV:U:M19.12)\" "+
- "with \"${PKGVERSION_NOREV:U} != \\\"19.12\\\"\".")
+ "AUTOFIX: filename.mk:3: Replacing \"empty(PREFS:U:M19.12)\" "+
+ "with \"${PREFS:U} != \\\"19.12\\\"\".")
testAfterPrefs(
- ".if !empty(PKG_INFO:Mpkg_info)",
- ".if ${PKG_INFO} == pkg_info",
-
- "NOTE: filename.mk:3: PKG_INFO should be "+
- "compared using \"${PKG_INFO} == pkg_info\" "+
- "instead of matching against \":Mpkg_info\".",
- "AUTOFIX: filename.mk:3: "+
- "Replacing \"!empty(PKG_INFO:Mpkg_info)\" "+
- "with \"${PKG_INFO} == pkg_info\".")
-
- t.CheckEquals(
- G.Pkgsrc.VariableType(nil, "PKG_LIBTOOL").
- Union().Contains(aclpUseLoadtime),
- false)
- testAfterPrefs(
- ".if !empty(PKG_LIBTOOL:Npattern)",
- ".if !empty(PKG_LIBTOOL:Npattern)",
+ ".if !empty(LATER:Npattern)",
+ ".if !empty(LATER:Npattern)",
// No diagnostics about the :N modifier yet,
- // see MkLineChecker.simplifyCondition.replace.
- "WARN: filename.mk:3: PKG_LIBTOOL should not be used "+
+ // see MkCondChecker.simplify.replace.
+ "WARN: filename.mk:3: LATER should not be used "+
"at load time in any file.")
// TODO: Add a note that the :U is unnecessary, and explain why.
testAfterPrefs(
- ".if ${PKGPATH:U:Mcategory/package}",
- ".if ${PKGPATH:U} == category/package",
-
- "NOTE: filename.mk:3: PKGPATH should be "+
- "compared using \"${PKGPATH:U} == category/package\" "+
- "instead of matching against \":Mcategory/package\".",
- "AUTOFIX: filename.mk:3: "+
- "Replacing \"${PKGPATH:U:Mcategory/package}\" "+
- "with \"${PKGPATH:U} == category/package\".")
+ ".if ${PREFS_DEFINED:U:Mpattern}",
+ ".if ${PREFS_DEFINED:U} == pattern",
- testAfterPrefs(
- ".if ${UNKNOWN:Mpattern}",
- ".if ${UNKNOWN:Mpattern}",
-
- "WARN: filename.mk:3: UNKNOWN is used but not defined.")
-
- // MAKE is AlwaysInScope and DefinedIfInScope and NonemptyIfDefined.
- testAfterPrefs(
- ".if ${MAKE:Mpattern}",
- ".if ${MAKE} == pattern",
-
- "NOTE: filename.mk:3: MAKE should be "+
- "compared using \"${MAKE} == pattern\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED:U} == pattern\" "+
"instead of matching against \":Mpattern\".",
"AUTOFIX: filename.mk:3: "+
- "Replacing \"${MAKE:Mpattern}\" "+
- "with \"${MAKE} == pattern\".")
+ "Replacing \"${PREFS_DEFINED:U:Mpattern}\" "+
+ "with \"${PREFS_DEFINED:U} == pattern\".")
- // VarUse without any modifiers is skipped.
- testAfterPrefs(
- ".if ${MAKE}",
- ".if ${MAKE}",
+ // Conditions without any modifiers cannot be simplified
+ // and are therefore skipped.
+ testBeforeAndAfterPrefs(
+ ".if ${IN_SCOPE_DEFINED}",
+ ".if ${IN_SCOPE_DEFINED}",
nil...)
@@ -821,53 +895,55 @@ func (s *Suite) Test_MkCondChecker_simpl
// replaced automatically, see mkCondLiteralChars.
// TODO: Add tests for all characters that are special in string literals or patterns.
// TODO: Then, extend the set of characters for which the expressions are simplified.
- testAfterPrefs(
- ".if ${FETCH_CMD:M&&}",
- ".if ${FETCH_CMD:M&&}",
+ testBeforeAndAfterPrefs(
+ ".if ${PREFS_DEFINED:M&&}",
+ ".if ${PREFS_DEFINED:M&&}",
+
+ nil...)
+
+ testBeforeAndAfterPrefs(
+ ".if ${PREFS:M&&}",
+ ".if ${PREFS:M&&}",
+ // TODO: Warn that the :U is missing.
nil...)
- // The + is contained in mkCondStringLiteralUnquoted.
- // The + is contained in mkCondModifierPatternLiteral.
+ // The + is contained in both mkCondStringLiteralUnquoted and
+ // mkCondModifierPatternLiteral, therefore it is copied verbatim.
testAfterPrefs(
- ".if ${PKGPATH:Mcategory/gtk+}",
- ".if ${PKGPATH} == category/gtk+",
+ ".if ${PREFS_DEFINED:Mcategory/gtk+}",
+ ".if ${PREFS_DEFINED} == category/gtk+",
- "NOTE: filename.mk:3: PKGPATH should be "+
- "compared using \"${PKGPATH} == category/gtk+\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} == category/gtk+\" "+
"instead of matching against \":Mcategory/gtk+\".",
"AUTOFIX: filename.mk:3: "+
- "Replacing \"${PKGPATH:Mcategory/gtk+}\" "+
- "with \"${PKGPATH} == category/gtk+\".")
+ "Replacing \"${PREFS_DEFINED:Mcategory/gtk+}\" "+
+ "with \"${PREFS_DEFINED} == category/gtk+\".")
// The characters <=> may be used unescaped in :M and :N patterns
// but not in .if conditions. There they must be enclosed in quotes.
- testAfterPrefs(
- ".if ${PKGPATH:M<=>}",
- ".if ${PKGPATH} == \"<=>\"",
+ testBeforeAndAfterPrefs(
+ ".if ${PREFS_DEFINED:M<=>}",
+ ".if ${PREFS_DEFINED} == \"<=>\"",
- "WARN: filename.mk:3: The pathname pattern \"<=>\" "+
- "contains the invalid characters \"<=>\".",
- "NOTE: filename.mk:3: PKGPATH should be "+
- "compared using \"${PKGPATH} == \"<=>\"\" "+
+ "NOTE: filename.mk:3: PREFS_DEFINED can be "+
+ "compared using the simpler \"${PREFS_DEFINED} == \"<=>\"\" "+
"instead of matching against \":M<=>\".",
"AUTOFIX: filename.mk:3: "+
- "Replacing \"${PKGPATH:M<=>}\" "+
- "with \"${PKGPATH} == \\\"<=>\\\"\".")
+ "Replacing \"${PREFS_DEFINED:M<=>}\" "+
+ "with \"${PREFS_DEFINED} == \\\"<=>\\\"\".")
// If pkglint replaces this particular pattern, the resulting string
// literal must be escaped properly.
- testAfterPrefs(
- ".if ${PKGPATH:M\"}",
- ".if ${PKGPATH:M\"}",
+ testBeforeAndAfterPrefs(
+ ".if ${IN_SCOPE_DEFINED:M\"}",
+ ".if ${IN_SCOPE_DEFINED:M\"}",
- // TODO: Find a better variable than PKGPATH,
- // to get rid of this unrelated warning.
- "WARN: filename.mk:3: The pathname pattern \"\\\"\" "+
- "contains the invalid character \"\\\"\".")
+ nil...)
}
-func (s *Suite) Test_MkCondChecker_simplifyCondition__defined_in_same_file(c *check.C) {
+func (s *Suite) Test_MkCondChecker_simplify__defined_in_same_file(c *check.C) {
t := s.Init(c)
t.SetUpPackage("category/package")
@@ -894,7 +970,7 @@ func (s *Suite) Test_MkCondChecker_simpl
// TODO: Replace MkLines.Check this with a more specific method.
t.ExpectDiagnosticsAutofix(
- mklines.Check,
+ func(autofix bool) { mklines.Check() },
diagnostics...)
// TODO: Move this assertion above the assertion about the diagnostics.
@@ -924,8 +1000,8 @@ func (s *Suite) Test_MkCondChecker_simpl
".if ${OK_DIR:Mpattern}",
".if ${OK_DIR} == pattern",
- "NOTE: filename.mk:4: OK_DIR should be "+
- "compared using \"${OK_DIR} == pattern\" "+
+ "NOTE: filename.mk:4: OK_DIR can be "+
+ "compared using the simpler \"${OK_DIR} == pattern\" "+
"instead of matching against \":Mpattern\".",
"AUTOFIX: filename.mk:4: "+
"Replacing \"${OK_DIR:Mpattern}\" "+
@@ -939,15 +1015,15 @@ func (s *Suite) Test_MkCondChecker_simpl
// FIXME: Warn that LATER_DIR is used before it is defined.
// FIXME: Add :U modifier since LATER_DIR is not yet defined.
- "NOTE: filename.mk:4: LATER_DIR should be "+
- "compared using \"${LATER_DIR} == pattern\" "+
+ "NOTE: filename.mk:4: LATER_DIR can be "+
+ "compared using the simpler \"${LATER_DIR} == pattern\" "+
"instead of matching against \":Mpattern\".",
"AUTOFIX: filename.mk:4: "+
"Replacing \"${LATER_DIR:Mpattern}\" "+
"with \"${LATER_DIR} == pattern\".")
}
-func (s *Suite) Test_MkCondChecker_checkDirectiveCondCompare(c *check.C) {
+func (s *Suite) Test_MkCondChecker_checkCompare(c *check.C) {
t := s.Init(c)
t.SetUpVartypes()
@@ -956,7 +1032,7 @@ func (s *Suite) Test_MkCondChecker_check
mklines := t.NewMkLines("filename.mk",
cond)
mklines.ForEach(func(mkline *MkLine) {
- NewMkCondChecker(mkline, mklines).checkDirectiveCond()
+ NewMkCondChecker(mkline, mklines).Check()
})
t.CheckOutput(output)
}
@@ -995,7 +1071,7 @@ func (s *Suite) Test_MkCondChecker_check
"WARN: filename.mk:1: Invalid condition, unrecognized part: \"empty{VAR}\".")
}
-func (s *Suite) Test_MkCondChecker_checkDirectiveCondCompareVarStr__no_tracing(c *check.C) {
+func (s *Suite) Test_MkCondChecker_checkCompareVarStr__no_tracing(c *check.C) {
t := s.Init(c)
b := NewMkTokenBuilder()
@@ -1007,22 +1083,11 @@ func (s *Suite) Test_MkCondChecker_check
ck := NewMkCondChecker(mklines.mklines[0], mklines)
varUse := b.VarUse("DISTFILES", "Mpattern", "O", "u")
// TODO: mklines.ForEach
- ck.checkDirectiveCondCompareVarStr(varUse, "==", "distfile-1.0.tar.gz")
+ ck.checkCompareVarStr(varUse, "==", "distfile-1.0.tar.gz")
t.CheckOutputEmpty()
}
-func (s *Suite) Test_MkCondChecker_checkCompareVarStr(c *check.C) {
- t := s.Init(c)
-
- test := func() {
- // FIXME
- t.CheckEquals(true, true)
- }
-
- test()
-}
-
func (s *Suite) Test_MkCondChecker_checkCompareVarStrCompiler(c *check.C) {
t := s.Init(c)
Index: pkgsrc/pkgtools/pkglint/files/mkvarusechecker.go
diff -u pkgsrc/pkgtools/pkglint/files/mkvarusechecker.go:1.1 pkgsrc/pkgtools/pkglint/files/mkvarusechecker.go:1.2
--- pkgsrc/pkgtools/pkglint/files/mkvarusechecker.go:1.1 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mkvarusechecker.go Sun Dec 8 22:03:38 2019
@@ -45,9 +45,10 @@ func (ck *MkVarUseChecker) checkUndefine
case !G.Opts.WarnExtra,
// Well-known variables are probably defined by the infrastructure.
vartype != nil && !vartype.IsGuessed(),
- ck.MkLines.vars.IsDefinedSimilar(varname),
+ // TODO: At load time, check ck.MkLines.loadVars instead of allVars.
+ ck.MkLines.allVars.IsDefinedSimilar(varname),
ck.MkLines.forVars[varname],
- ck.MkLines.vars.Mentioned(varname) != nil,
+ ck.MkLines.allVars.Mentioned(varname) != nil,
G.Pkg != nil && G.Pkg.vars.IsDefinedSimilar(varname),
containsVarRef(varname),
G.Pkgsrc.vartypes.IsDefinedCanon(varname),
@@ -61,9 +62,7 @@ func (ck *MkVarUseChecker) checkUndefine
}
func (ck *MkVarUseChecker) checkModifiers() {
- varuse := ck.use
- mods := varuse.modifiers
- if len(mods) == 0 {
+ if len(ck.use.modifiers) == 0 {
return
}
Index: pkgsrc/pkgtools/pkglint/files/mkvarusechecker_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mkvarusechecker_test.go:1.1 pkgsrc/pkgtools/pkglint/files/mkvarusechecker_test.go:1.2
--- pkgsrc/pkgtools/pkglint/files/mkvarusechecker_test.go:1.1 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mkvarusechecker_test.go Sun Dec 8 22:03:38 2019
@@ -275,7 +275,25 @@ func (s *Suite) Test_MkVarUseChecker_che
}
func (s *Suite) Test_MkVarUseChecker_checkModifiers(c *check.C) {
- // FIXME
+ t := s.Init(c)
+
+ test := func(text string, diagnostics ...string) {
+ mklines := t.NewMkLines("filename.mk",
+ text)
+ mklines.ForEach(func(mkline *MkLine) {
+ mkline.ForEachUsed(func(varUse *MkVarUse, time VucTime) {
+ ck := NewMkVarUseChecker(varUse, mklines, mkline)
+ ck.checkModifiers()
+ })
+ })
+ t.CheckOutput(diagnostics)
+ }
+
+ test("\t${VAR}",
+ nil...)
+
+ test("\t${VAR:from=to:Q}",
+ "WARN: filename.mk:1: The text \":Q\" looks like a modifier but isn't.")
}
func (s *Suite) Test_MkVarUseChecker_checkModifiersSuffix(c *check.C) {
@@ -1075,7 +1093,7 @@ func (s *Suite) Test_MkVarUseChecker_fix
t.SetUpVartypes()
- test := func() {
+ test := func(autofix bool) {
mklines := t.SetUpFileMkLines("filename.mk",
MkCvsID,
"",
Index: pkgsrc/pkgtools/pkglint/files/mklexer.go
diff -u pkgsrc/pkgtools/pkglint/files/mklexer.go:1.4 pkgsrc/pkgtools/pkglint/files/mklexer.go:1.5
--- pkgsrc/pkgtools/pkglint/files/mklexer.go:1.4 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mklexer.go Sun Dec 8 22:03:38 2019
@@ -73,7 +73,7 @@ func (p *MkLexer) VarUse() *MkVarUse {
// This is an escaped dollar character and not a variable use.
return nil
- case '@', '<', ' ':
+ case '>', '!', '<', '%', '?', '*', '@', ' ':
// These variable names are known to exist.
//
// Many others are also possible but not used in practice.
@@ -81,6 +81,13 @@ func (p *MkLexer) VarUse() *MkVarUse {
// the $ must not be interpreted as a variable name,
// even when it looks like $/ could refer to the "/" variable.
//
+ // Example:
+ // ${:U }= space
+ // ${:U"}= quot
+ //
+ // all:
+ // @echo ${ } $ d, ${"} $"ed # space spaced, quote quoted
+ //
// TODO: Find out whether $" is a variable use when it appears in the :M modifier.
p.lexer.Skip(2)
return NewMkVarUse(rest[1:2])
Index: pkgsrc/pkgtools/pkglint/files/mklexer_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mklexer_test.go:1.3 pkgsrc/pkgtools/pkglint/files/mklexer_test.go:1.4
--- pkgsrc/pkgtools/pkglint/files/mklexer_test.go:1.3 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mklexer_test.go Sun Dec 8 22:03:38 2019
@@ -404,7 +404,7 @@ func (s *Suite) Test_MkLexer_VarUse(c *c
func (s *Suite) Test_MkLexer_varUseBrace__autofix_parentheses(c *check.C) {
t := s.Init(c)
- test := func() {
+ test := func(autofix bool) {
mklines := t.SetUpFileMkLines("Makefile",
MkCvsID,
"COMMENT=\t$(P1) $(P2)) $(P3:Q) ${BRACES} $(A.$(B.$(C))) $(A:M\\#)",
@@ -1248,7 +1248,7 @@ func (s *Suite) Test_MkLexer_Explain(c *
func (s *Suite) Test_MkLexer_Autofix(c *check.C) {
t := s.Init(c)
- test := func() {
+ test := func(autofix bool) {
mklines := t.SetUpFileMkLines("filename.mk",
"# before")
lex := NewMkLexer("", mklines.lines.Lines[0])
Index: pkgsrc/pkgtools/pkglint/files/mkline.go
diff -u pkgsrc/pkgtools/pkglint/files/mkline.go:1.68 pkgsrc/pkgtools/pkglint/files/mkline.go:1.69
--- pkgsrc/pkgtools/pkglint/files/mkline.go:1.68 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mkline.go Sun Dec 8 22:03:38 2019
@@ -1239,7 +1239,7 @@ func (ind *Indentation) TrackAfter(mklin
cond.Walk(&MkCondCallback{
Call: func(name string, arg string) {
if name == "exists" && !NewPath(arg).IsAbs() {
- rel := G.Pkgsrc.ToRel(mkline.File(NewRelPathString(arg)))
+ rel := G.Pkgsrc.Rel(mkline.File(NewRelPathString(arg)))
ind.AddCheckedFile(rel)
}
}})
Index: pkgsrc/pkgtools/pkglint/files/mklinechecker.go
diff -u pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.57 pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.58
--- pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.57 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mklinechecker.go Sun Dec 8 22:03:38 2019
@@ -310,7 +310,7 @@ func (ck MkLineChecker) CheckRelativePat
abs := mkline.Filename.DirNoClean().JoinNoClean(resolvedPath)
if !abs.Exists() {
- pkgsrcPath := G.Pkgsrc.ToRel(ck.MkLine.File(resolvedPath))
+ pkgsrcPath := G.Pkgsrc.Rel(ck.MkLine.File(resolvedPath))
if mustExist && !ck.MkLines.indentation.HasExists(pkgsrcPath) {
mkline.Errorf("Relative path %q does not exist.", resolvedPath)
}
@@ -327,7 +327,7 @@ func (ck MkLineChecker) CheckRelativePat
case matches(resolvedPath.String(), `^\.\./\.\./[^./][^/]*/[^/]`):
// From a package to another package.
- case resolvedPath.HasPrefixPath("../mk") && G.Pkgsrc.ToRel(mkline.Filename).Count() == 2:
+ case resolvedPath.HasPrefixPath("../mk") && G.Pkgsrc.Rel(mkline.Filename).Count() == 2:
// For category Makefiles.
// TODO: Or from a pkgsrc wip package to wip/mk.
@@ -406,7 +406,7 @@ func (ck MkLineChecker) checkDirective(f
case directive == "if" || directive == "elif":
mkCondChecker := NewMkCondChecker(mkline, ck.MkLines)
- mkCondChecker.checkDirectiveCond()
+ mkCondChecker.Check()
case directive == "ifdef" || directive == "ifndef":
mkline.Warnf("The \".%s\" directive is deprecated. Please use \".if %sdefined(%s)\" instead.",
Index: pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go:1.52 pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go:1.53
--- pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go:1.52 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go Sun Dec 8 22:03:38 2019
@@ -209,7 +209,7 @@ func (s *Suite) Test_MkLineChecker_check
MkCvsID,
"DISTNAME=\tgcc-${GCC_VERSION}")
- mklines.vars.Define("GCC_VERSION", mklines.mklines[1])
+ mklines.allVars.Define("GCC_VERSION", mklines.mklines[1])
mklines.Check()
t.CheckOutputEmpty()
Index: pkgsrc/pkgtools/pkglint/files/shell.go
diff -u pkgsrc/pkgtools/pkglint/files/shell.go:1.52 pkgsrc/pkgtools/pkglint/files/shell.go:1.53
--- pkgsrc/pkgtools/pkglint/files/shell.go:1.52 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/shell.go Sun Dec 8 22:03:38 2019
@@ -128,7 +128,7 @@ func (scc *SimpleCommandChecker) handleC
// When the package author has explicitly defined a command
// variable, assume it to be valid.
- if scc.MkLines.vars.IsDefinedSimilar(varname) {
+ if scc.MkLines.allVars.IsDefinedSimilar(varname) {
return true
}
@@ -531,7 +531,7 @@ func (ck *ShellLineChecker) checkPipeExi
var shellCommandsType = NewVartype(BtShellCommands, NoVartypeOptions, NewACLEntry("*", aclpAllRuntime))
-// FIXME: Why is this called shell_Word_Vuc and not shell_Commands_Vuc?
+// XXX: Why is this called shell_Word_Vuc and not shell_Commands_Vuc?
var shellWordVuc = &VarUseContext{shellCommandsType, VucUnknownTime, VucQuotPlain, false}
func NewShellLineChecker(mklines *MkLines, mkline *MkLine) *ShellLineChecker {
@@ -657,7 +657,7 @@ func (ck *ShellLineChecker) CheckShellCo
line := ck.mkline.Line
program, err := parseShellProgram(line, shellcmd)
- // FIXME: This code is duplicated in checkWordQuoting.
+ // XXX: This code is duplicated in checkWordQuoting.
if err != nil && contains(shellcmd, "$$(") { // Hack until the shell parser can handle subshells.
line.Warnf("Invoking subshells via $(...) is not portable enough.")
return
Index: pkgsrc/pkgtools/pkglint/files/mklineparser_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mklineparser_test.go:1.6 pkgsrc/pkgtools/pkglint/files/mklineparser_test.go:1.7
--- pkgsrc/pkgtools/pkglint/files/mklineparser_test.go:1.6 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mklineparser_test.go Sun Dec 8 22:03:38 2019
@@ -913,7 +913,7 @@ func (s *Suite) Test_MkLineParser_split(
comment: " comment after spaces",
})
- // FIXME: This theoretical edge case is interpreted differently
+ // XXX: This theoretical edge case is interpreted differently
// between bmake and pkglint. Pkglint treats the # as a comment,
// while bmake interprets it as a regular character.
test("\\[#",
Index: pkgsrc/pkgtools/pkglint/files/mklines.go
diff -u pkgsrc/pkgtools/pkglint/files/mklines.go:1.63 pkgsrc/pkgtools/pkglint/files/mklines.go:1.64
--- pkgsrc/pkgtools/pkglint/files/mklines.go:1.63 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mklines.go Sun Dec 8 22:03:38 2019
@@ -7,7 +7,7 @@ type MkLines struct {
mklines []*MkLine
lines *Lines
target string // Current make(1) target; only available during checkAll
- vars Scope //
+ allVars Scope // The variables after loading the complete file
buildDefs map[string]bool // Variables that are registered in BUILD_DEFS, to ensure that all user-defined variables are added to it.
plistVarAdded map[string]*MkLine // Identifiers that are added to PLIST_VARS.
plistVarSet map[string]*MkLine // Identifiers for which PLIST.${id} is defined.
@@ -124,7 +124,7 @@ func (mklines *MkLines) collectUsedVaria
// UseVar remembers that the given variable is used in the given line.
// This controls the "defined but not used" warning.
func (mklines *MkLines) UseVar(mkline *MkLine, varname string, time VucTime) {
- mklines.vars.Use(varname, mkline, time)
+ mklines.allVars.Use(varname, mkline, time)
if G.Pkg != nil {
G.Pkg.vars.Use(varname, mkline, time)
}
@@ -148,8 +148,8 @@ func (mklines *MkLines) collectDocumente
// leaving 2 of these 3 lines for the actual documentation.
if commentLines >= 3 && relevant {
for varname, mkline := range scope.used {
- mklines.vars.Define(varname, mkline)
- mklines.vars.Use(varname, mkline, VucRunTime)
+ mklines.allVars.Define(varname, mkline)
+ mklines.allVars.Use(varname, mkline, VucRunTime)
}
}
@@ -231,7 +231,7 @@ func (mklines *MkLines) collectVariables
"BUILTIN_FIND_FILES_VAR",
"BUILTIN_FIND_HEADERS_VAR":
for _, varname := range mkline.Fields() {
- mklines.vars.Define(varname, mkline)
+ mklines.allVars.Define(varname, mkline)
}
case "PLIST_VARS":
@@ -284,6 +284,9 @@ func (mklines *MkLines) ForEachEnd(actio
// Multiple of these line checkers could be run in parallel, so that
// the diagnostics appear in the correct order, from top to bottom.
+ // ForEachEnd must not be called within itself.
+ assert(mklines.indentation == nil)
+
mklines.indentation = NewIndentation()
mklines.Tools.SeenPrefs = false
@@ -307,7 +310,7 @@ func (mklines *MkLines) ForEachEnd(actio
// defineVar marks a variable as defined in both the current package and the current file.
func (mklines *MkLines) defineVar(pkg *Package, mkline *MkLine, varname string) {
- mklines.vars.Define(varname, mkline)
+ mklines.allVars.Define(varname, mkline)
if pkg != nil {
pkg.vars.Define(varname, mkline)
}
Index: pkgsrc/pkgtools/pkglint/files/util.go
diff -u pkgsrc/pkgtools/pkglint/files/util.go:1.63 pkgsrc/pkgtools/pkglint/files/util.go:1.64
--- pkgsrc/pkgtools/pkglint/files/util.go:1.63 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/util.go Sun Dec 8 22:03:38 2019
@@ -449,7 +449,17 @@ func mkopSubst(s string, left bool, from
}
func containsVarRef(s string) bool {
- return contains(s, "${") || contains(s, "$(")
+ if !contains(s, "$") {
+ return false
+ }
+ lex := NewMkLexer(s, nil)
+ tokens, _ := lex.MkTokens()
+ for _, token := range tokens {
+ if token.Varuse != nil {
+ return true
+ }
+ }
+ return false
}
func hasAlnumPrefix(s string) bool { return s != "" && textproc.AlnumU.Contains(s[0]) }
Index: pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go
diff -u pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go:1.63 pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go:1.64
--- pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go:1.63 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/vartypecheck_test.go Sun Dec 8 22:03:38 2019
@@ -4,6 +4,126 @@ import (
"gopkg.in/check.v1"
)
+func (s *Suite) Test_VartypeCheck_Errorf(c *check.C) {
+ t := s.Init(c)
+
+ mkline := t.NewMkLine("filename.mk", 123, "")
+ cv := VartypeCheck{MkLine: mkline}
+
+ cv.Errorf("Error %q.", "message")
+
+ t.CheckOutputLines(
+ "ERROR: filename.mk:123: Error \"message\".")
+}
+
+func (s *Suite) Test_VartypeCheck_Warnf(c *check.C) {
+ t := s.Init(c)
+
+ mkline := t.NewMkLine("filename.mk", 123, "")
+ cv := VartypeCheck{MkLine: mkline}
+
+ cv.Warnf("Warning %q.", "message")
+
+ t.CheckOutputLines(
+ "WARN: filename.mk:123: Warning \"message\".")
+}
+
+func (s *Suite) Test_VartypeCheck_Notef(c *check.C) {
+ t := s.Init(c)
+
+ mkline := t.NewMkLine("filename.mk", 123, "")
+ cv := VartypeCheck{MkLine: mkline}
+
+ cv.Notef("Note %q.", "message")
+
+ t.CheckOutputLines(
+ "NOTE: filename.mk:123: Note \"message\".")
+}
+
+func (s *Suite) Test_VartypeCheck_Explain(c *check.C) {
+ t := s.Init(c)
+
+ t.SetUpCommandLine("--explain")
+ mkline := t.NewMkLine("filename.mk", 123, "")
+ cv := VartypeCheck{MkLine: mkline}
+
+ cv.Notef("Note %q.", "message")
+ cv.Explain("Explanation.")
+
+ t.CheckOutputLines(
+ "NOTE: filename.mk:123: Note \"message\".",
+ "",
+ "\tExplanation.",
+ "")
+}
+
+func (s *Suite) Test_VartypeCheck_Autofix(c *check.C) {
+ t := s.Init(c)
+
+ mkline := t.NewMkLine("filename.mk", 123, "")
+ cv := VartypeCheck{MkLine: mkline}
+
+ t.CheckEquals(cv.Autofix(), mkline.Autofix())
+}
+
+func (s *Suite) Test_VartypeCheck_WithValue(c *check.C) {
+ t := s.Init(c)
+
+ cv := VartypeCheck{
+ Varname: "OLD",
+ Value: "oldValue${VAR}",
+ ValueNoVar: "oldValue",
+ }
+
+ copied := cv.WithValue("newValue${NEW_VAR}")
+
+ t.CheckEquals(copied.Varname, "OLD")
+ t.CheckEquals(copied.Value, "newValue${NEW_VAR}")
+ t.CheckEquals(copied.ValueNoVar, "newValue")
+ t.CheckEquals(cv.Value, "oldValue${VAR}")
+ t.CheckEquals(cv.ValueNoVar, "oldValue")
+}
+
+func (s *Suite) Test_VartypeCheck_WithVarnameValue(c *check.C) {
+ t := s.Init(c)
+
+ cv := VartypeCheck{
+ Varname: "OLD",
+ Value: "oldValue${VAR}",
+ ValueNoVar: "oldValue",
+ }
+
+ copied := cv.WithVarnameValue("NEW", "newValue${NEW_VAR}")
+
+ t.CheckEquals(copied.Varname, "NEW")
+ t.CheckEquals(copied.Value, "newValue${NEW_VAR}")
+ t.CheckEquals(copied.ValueNoVar, "newValue")
+ t.CheckEquals(cv.Value, "oldValue${VAR}")
+ t.CheckEquals(cv.ValueNoVar, "oldValue")
+}
+
+func (s *Suite) Test_VartypeCheck_WithVarnameValueMatch(c *check.C) {
+ t := s.Init(c)
+
+ cv := VartypeCheck{
+ Varname: "OLD",
+ Op: opAssign,
+ Value: "oldValue${VAR}",
+ ValueNoVar: "oldValue",
+ }
+
+ copied := cv.WithVarnameValueMatch("NEW", "newValue${NEW_VAR}")
+
+ t.CheckEquals(copied.Varname, "NEW")
+ t.CheckEquals(copied.Op, opUseMatch)
+ t.CheckEquals(copied.Value, "newValue${NEW_VAR}")
+ t.CheckEquals(copied.ValueNoVar, "newValue")
+ t.CheckEquals(cv.Varname, "OLD")
+ t.CheckEquals(cv.Op, opAssign)
+ t.CheckEquals(cv.Value, "oldValue${VAR}")
+ t.CheckEquals(cv.ValueNoVar, "oldValue")
+}
+
func (s *Suite) Test_VartypeCheck_AwkCommand(c *check.C) {
t := s.Init(c)
vt := NewVartypeCheckTester(t, BtAwkCommand)
@@ -516,8 +636,8 @@ func (s *Suite) Test_VartypeCheck_Enum__
mklines.Check()
t.CheckOutputLines(
- "NOTE: module.mk:5: MACHINE_ARCH "+
- "should be compared using \"${MACHINE_ARCH} == i386\" "+
+ "NOTE: module.mk:5: MACHINE_ARCH can be "+
+ "compared using the simpler \"${MACHINE_ARCH} == i386\" "+
"instead of matching against \":Mi386\".",
"",
"\tThis variable has a single value, not a list of values. Therefore it",
@@ -626,15 +746,17 @@ func (s *Suite) Test_VartypeCheck_FetchU
vt.Values(
"https://example.org/pub";,
- "https://example.org/$@";, // Doesn't make sense at all.
+ "https://example.org/$@";,
"https://example.org/?f=";,
"https://example.org/download:";,
- "https://example.org/download?";)
+ "https://example.org/download?";,
+ "https://example.org/$$";)
vt.Output(
"WARN: filename.mk:71: The fetch URL \"https://example.org/pub\"; should end with a slash.",
- "WARN: filename.mk:72: \"https://example.org/$@\"; is not a valid URL.",
- "WARN: filename.mk:75: The fetch URL \"https://example.org/download?\"; should end with a slash.")
+ "WARN: filename.mk:75: The fetch URL \"https://example.org/download?\"; should end with a slash.",
+ "WARN: filename.mk:76: \"https://example.org/$$\"; is not a valid URL.",
+ "WARN: filename.mk:76: The fetch URL \"https://example.org/$$\"; should end with a slash.")
// The transport protocol doesn't matter for matching the MASTER_SITEs.
// See url2pkg.py, function adjust_site_from_sites_mk.
@@ -659,6 +781,15 @@ func (s *Suite) Test_VartypeCheck_FetchU
"instead of \"-ftp://ftp.gnu.org/pub/gnu/bash/bash-5.0.tar.gz\".";,
"WARN: filename.mk:86: Please use ${MASTER_SITE_GNU:S,^,-,:=bash/bash-5.0.tar.gz} "+
"instead of \"-https://ftp.gnu.org/pub/gnu/bash/bash-5.0.tar.gz\".";)
+
+ // The ${.TARGET} variable doesn't make sense at all in a URL.
+ // Other variables might, and there could be checks for them.
+ // As of December 2019 these are skipped completely,
+ // see containsVarRef in VartypeCheck.URL.
+ vt.Values(
+ "https://example.org/$@";)
+
+ vt.OutputEmpty()
}
func (s *Suite) Test_VartypeCheck_FetchURL__without_package(c *check.C) {
@@ -992,6 +1123,60 @@ func (s *Suite) Test_VartypeCheck_Machin
"WARN: filename.mk:6: \"x86_64-pc\" is not a valid platform pattern.")
}
+func (s *Suite) Test_VartypeCheck_MachinePlatform(c *check.C) {
+ vt := NewVartypeCheckTester(s.Init(c), BtMachinePlatform)
+
+ // There is no need to test the assignment operators since the
+ // only variable of this type is read-only.
+
+ vt.Varname("MACHINE_PLATFORM")
+ vt.Op(opUseMatch)
+ vt.Values(
+ "linux-i386",
+ "nextbsd-5.0-8087",
+ "netbsd-7.0-l*",
+ "NetBSD-1.6.2-i386",
+ "FreeBSD*",
+ "FreeBSD-*",
+ "${LINUX}",
+ "NetBSD-[0-1]*-*")
+
+ vt.Output(
+ "WARN: filename.mk:1: \"linux-i386\" is not a valid platform pattern.",
+ "WARN: filename.mk:2: The pattern \"nextbsd\" cannot match any of "+
+ "{ AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD HPUX Haiku "+
+ "IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare "+
+ "} for the operating system part of MACHINE_PLATFORM.",
+ "WARN: filename.mk:2: The pattern \"8087\" cannot match any of "+
+ "{ aarch64 aarch64eb alpha amd64 arc arm arm26 arm32 "+
+ "cobalt coldfire convex dreamcast "+
+ "earm earmeb earmhf earmhfeb earmv4 earmv4eb "+
+ "earmv5 earmv5eb earmv6 earmv6eb earmv6hf "+
+ "earmv6hfeb earmv7 earmv7eb earmv7hf earmv7hfeb evbarm hpcmips hpcsh hppa hppa64 "+
+ "i386 i586 i686 ia64 m68000 m68k m88k "+
+ "mips mips64 mips64eb mips64el mipseb mipsel mipsn32 "+
+ "mlrisc ns32k pc532 pmax powerpc powerpc64 "+
+ "rs6000 s390 sh3eb sh3el sparc sparc64 vax x86_64 "+
+ "} for the hardware architecture part of MACHINE_PLATFORM.",
+ "WARN: filename.mk:3: The pattern \"netbsd\" cannot match any of "+
+ "{ AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD HPUX Haiku "+
+ "IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare "+
+ "} for the operating system part of MACHINE_PLATFORM.",
+ "WARN: filename.mk:3: The pattern \"l*\" cannot match any of "+
+ "{ aarch64 aarch64eb alpha amd64 arc arm arm26 arm32 "+
+ "cobalt coldfire convex dreamcast "+
+ "earm earmeb earmhf earmhfeb earmv4 earmv4eb "+
+ "earmv5 earmv5eb earmv6 earmv6eb earmv6hf "+
+ "earmv6hfeb earmv7 earmv7eb earmv7hf earmv7hfeb evbarm hpcmips hpcsh hppa hppa64 "+
+ "i386 i586 i686 ia64 m68000 m68k m88k "+
+ "mips mips64 mips64eb mips64el mipseb mipsel mipsn32 "+
+ "mlrisc ns32k pc532 pmax powerpc powerpc64 "+
+ "rs6000 s390 sh3eb sh3el sparc sparc64 vax x86_64 "+
+ "} for the hardware architecture part of MACHINE_PLATFORM.",
+ "WARN: filename.mk:5: \"FreeBSD*\" is not a valid platform pattern.",
+ "WARN: filename.mk:8: Please use \"[0-1].*\" instead of \"[0-1]*\" as the version pattern.")
+}
+
func (s *Suite) Test_VartypeCheck_MachinePlatformPattern(c *check.C) {
vt := NewVartypeCheckTester(s.Init(c), BtMachinePlatformPattern)
@@ -1271,7 +1456,7 @@ func (s *Suite) Test_VartypeCheck_Pkgrev
"3a")
vt.Output(
- "WARN: filename.mk:1: PKGREVISION must be a positive integer number.",
+ "ERROR: filename.mk:1: PKGREVISION must be a positive integer number.",
"ERROR: filename.mk:1: PKGREVISION only makes sense directly in the package Makefile.")
vt.File("Makefile")
@@ -1354,6 +1539,31 @@ func (s *Suite) Test_VartypeCheck_RPkgVe
vt.OutputEmpty()
}
+func (s *Suite) Test_VartypeCheck_RelativePkgDir(c *check.C) {
+ t := s.Init(c)
+ vt := NewVartypeCheckTester(t, BtRelativePkgDir)
+
+ t.CreateFileLines("category/other-package/Makefile")
+ t.Chdir("category/package")
+
+ vt.Varname("PKGDIR")
+ vt.Values(
+ "category/other-package",
+ "../../category/other-package",
+ "${OTHER_VAR}",
+ "invalid",
+ "../../invalid/relative",
+ "/absolute")
+
+ vt.Output(
+ "ERROR: filename.mk:1: Relative path \"category/other-package/Makefile\" does not exist.",
+ "WARN: filename.mk:1: \"category/other-package\" is not a valid relative package directory.",
+ "ERROR: filename.mk:4: Relative path \"invalid/Makefile\" does not exist.",
+ "WARN: filename.mk:4: \"invalid\" is not a valid relative package directory.",
+ "ERROR: filename.mk:5: Relative path \"../../invalid/relative/Makefile\" does not exist.",
+ "ERROR: filename.mk:6: The path \"/absolute\" must be relative.")
+}
+
func (s *Suite) Test_VartypeCheck_RelativePkgPath(c *check.C) {
t := s.Init(c)
vt := NewVartypeCheckTester(t, BtRelativePkgPath)
@@ -1367,12 +1577,14 @@ func (s *Suite) Test_VartypeCheck_Relati
"../../category/other-package",
"${OTHER_VAR}",
"invalid",
- "../../invalid/relative")
+ "../../invalid/relative",
+ "/absolute")
vt.Output(
"ERROR: filename.mk:1: Relative path \"category/other-package\" does not exist.",
"ERROR: filename.mk:4: Relative path \"invalid\" does not exist.",
- "ERROR: filename.mk:5: Relative path \"../../invalid/relative\" does not exist.")
+ "ERROR: filename.mk:5: Relative path \"../../invalid/relative\" does not exist.",
+ "ERROR: filename.mk:6: The path \"/absolute\" must be relative.")
}
func (s *Suite) Test_VartypeCheck_Restricted(c *check.C) {
@@ -1405,7 +1617,7 @@ func (s *Suite) Test_VartypeCheck_SedCom
vt.Output(
"NOTE: filename.mk:1: Please always use \"-e\" in sed commands, even if there is only one substitution.",
- "NOTE: filename.mk:2: Each sed command should appear in an assignment of its own.",
+ "WARN: filename.mk:2: Each sed command should appear in an assignment of its own.",
"WARN: filename.mk:3: The # character starts a Makefile comment.",
"ERROR: filename.mk:3: Invalid shell words \"\\\"s,\" in sed commands.",
"WARN: filename.mk:8: Unknown sed command \"1d\".",
@@ -1590,6 +1802,23 @@ func (s *Suite) Test_VartypeCheck_Tool(c
vt.OutputEmpty()
}
+func (s *Suite) Test_VartypeCheck_Unknown(c *check.C) {
+ t := s.Init(c)
+ vt := NewVartypeCheckTester(t, BtUnknown)
+
+ vt.Varname("BDB185_DEFAULT")
+ vt.Values(
+ "# empty",
+ "Something",
+ "'quotes are ok'",
+ "!\"#$%&/()*+,-./0-9:;<=>?@A-Z[\\]^_a-z{|}~")
+
+ // This warning is produced as a side effect of parsing the lines.
+ // It is not specific to the BtUnknown type.
+ vt.Output(
+ "WARN: filename.mk:4: The # character starts a Makefile comment.")
+}
+
func (s *Suite) Test_VartypeCheck_URL(c *check.C) {
t := s.Init(c)
vt := NewVartypeCheckTester(t, BtURL)
@@ -1749,6 +1978,30 @@ func (s *Suite) Test_VartypeCheck_Wrappe
"WARN: filename.mk:8: Unknown wrapper transform command \"unknown\".")
}
+func (s *Suite) Test_VartypeCheck_WrkdirSubdirectory(c *check.C) {
+ vt := NewVartypeCheckTester(s.Init(c), BtWrkdirSubdirectory)
+
+ vt.Varname("WRKSRC")
+ vt.Op(opAssign)
+ vt.Values(
+ "${WRKDIR}",
+ "${WRKDIR}/",
+ "${WRKDIR}/.",
+ "${WRKDIR}/subdir",
+ ".",
+ "${DISTNAME}",
+ "${PKGNAME_NOREV}",
+ "two words",
+ "../other",
+ "${WRKSRC}", // Recursive definition.
+ "${PKGDIR}/files")
+
+ // XXX: Many more consistency checks are possible here.
+ vt.Output(
+ "WARN: filename.mk:8: The pathname \"two words\" " +
+ "contains the invalid character \" \".")
+}
+
func (s *Suite) Test_VartypeCheck_WrksrcSubdirectory(c *check.C) {
vt := NewVartypeCheckTester(s.Init(c), BtWrksrcSubdirectory)
Index: pkgsrc/pkgtools/pkglint/files/mklines_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mklines_test.go:1.54 pkgsrc/pkgtools/pkglint/files/mklines_test.go:1.55
--- pkgsrc/pkgtools/pkglint/files/mklines_test.go:1.54 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mklines_test.go Sun Dec 8 22:03:38 2019
@@ -496,8 +496,8 @@ func (s *Suite) Test_MkLines_collectUsed
mklines.collectUsedVariables()
- t.CheckDeepEquals(mklines.vars.used, map[string]*MkLine{"VAR": mkline})
- t.CheckEquals(mklines.vars.FirstUse("VAR"), mkline)
+ t.CheckDeepEquals(mklines.allVars.used, map[string]*MkLine{"VAR": mkline})
+ t.CheckEquals(mklines.allVars.FirstUse("VAR"), mkline)
}
func (s *Suite) Test_MkLines_collectUsedVariables__nested(c *check.C) {
@@ -515,12 +515,12 @@ func (s *Suite) Test_MkLines_collectUsed
mklines.collectUsedVariables()
- t.CheckEquals(len(mklines.vars.used), 5)
- t.CheckEquals(mklines.vars.FirstUse("lparam"), assignMkline)
- t.CheckEquals(mklines.vars.FirstUse("rparam"), assignMkline)
- t.CheckEquals(mklines.vars.FirstUse("inner"), shellMkline)
- t.CheckEquals(mklines.vars.FirstUse("outer.*"), shellMkline)
- t.CheckEquals(mklines.vars.FirstUse("outer.${inner}"), shellMkline)
+ t.CheckEquals(len(mklines.allVars.used), 5)
+ t.CheckEquals(mklines.allVars.FirstUse("lparam"), assignMkline)
+ t.CheckEquals(mklines.allVars.FirstUse("rparam"), assignMkline)
+ t.CheckEquals(mklines.allVars.FirstUse("inner"), shellMkline)
+ t.CheckEquals(mklines.allVars.FirstUse("outer.*"), shellMkline)
+ t.CheckEquals(mklines.allVars.FirstUse("outer.${inner}"), shellMkline)
}
func (s *Suite) Test_MkLines_collectDocumentedVariables(c *check.C) {
@@ -572,7 +572,7 @@ func (s *Suite) Test_MkLines_collectDocu
mklines.collectDocumentedVariables()
var varnames []string
- for varname, mkline := range mklines.vars.used {
+ for varname, mkline := range mklines.allVars.used {
varnames = append(varnames, sprintf("%s (line %s)", varname, mkline.Linenos()))
}
sort.Strings(varnames)
@@ -696,7 +696,7 @@ func (s *Suite) Test_MkLines_collectVari
mklines.Check()
t.CheckDeepEquals(
- keys(mklines.vars.firstDef),
+ keys(mklines.allVars.firstDef),
[]string{
"BUILTIN_FIND_FILES_VAR",
"BUILTIN_FIND_HEADERS_VAR",
Index: pkgsrc/pkgtools/pkglint/files/pkglint_test.go
diff -u pkgsrc/pkgtools/pkglint/files/pkglint_test.go:1.54 pkgsrc/pkgtools/pkglint/files/pkglint_test.go:1.55
--- pkgsrc/pkgtools/pkglint/files/pkglint_test.go:1.54 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/pkglint_test.go Sun Dec 8 22:03:38 2019
@@ -237,13 +237,12 @@ func (s *Suite) Test_Pkglint_Main__compl
"WARN: ~/sysutils/checkperms/Makefile:3: "+
"This package should be updated to 1.13 (supports more file formats; see ../../doc/TODO:5).",
"ERROR: ~/sysutils/checkperms/Makefile:4: Invalid category \"tools\".",
- "ERROR: ~/sysutils/checkperms/README: Packages in main pkgsrc must not have a README file.",
"ERROR: ~/sysutils/checkperms/TODO: Packages in main pkgsrc must not have a TODO file.",
"ERROR: ~/sysutils/checkperms/distinfo:7: SHA1 hash of patches/patch-checkperms.c differs "+
"(distinfo has asdfasdf, patch file has e775969de639ec703866c0336c4c8e0fdd96309c).",
"WARN: ~/sysutils/checkperms/patches/patch-checkperms.c:12: Premature end of patch hunk "+
"(expected 1 lines to be deleted and 0 lines to be added).",
- "4 errors, 2 warnings and 1 note found.",
+ "3 errors, 2 warnings and 1 note found.",
t.Shquote("(Run \"pkglint -e -Wall -Call %s\" to show explanations.)", "sysutils/checkperms"),
t.Shquote("(Run \"pkglint -fs -Wall -Call %s\" to show what can be fixed automatically.)", "sysutils/checkperms"),
t.Shquote("(Run \"pkglint -F -Wall -Call %s\" to automatically fix some issues.)", "sysutils/checkperms"))
@@ -754,10 +753,46 @@ func (s *Suite) Test_CheckLinesDescr(c *
// in devel/go-properties/DESCR.
t.CheckOutputLines(
"WARN: DESCR:1: Line too long (should be no more than 80 characters).",
- "NOTE: DESCR:11: Variables are not expanded in the DESCR file.",
+ "NOTE: DESCR:11: Variables like \"${PREFIX}\" are not expanded in the DESCR file.",
"WARN: DESCR:25: File too long (should be no more than 24 lines).")
}
+// The package author may think that variables like ${PREFIX}
+// are expanded in DESCR files too, but that doesn't happen.
+func (s *Suite) Test_CheckLinesDescr__variables(c *check.C) {
+ t := s.Init(c)
+
+ t.SetUpVartypes()
+
+ test := func(text string, diagnostics ...string) {
+ lines := t.NewLines("DESCR",
+ text)
+
+ CheckLinesDescr(lines)
+
+ t.CheckOutput(diagnostics)
+ }
+
+ test("${PREFIX}",
+
+ "NOTE: DESCR:1: Variables like \"${PREFIX}\" "+
+ "are not expanded in the DESCR file.")
+
+ // Variables in parentheses are unusual in pkgsrc.
+ // Therefore they are not worth being mentioned.
+ test("$(PREFIX)", nil...)
+
+ // Variables that are not well-known in pkgsrc are not warned
+ // about since these are probably legitimate examples, as seen
+ // in devel/go-properties/DESCR.
+ test("${UNDEFINED}", nil...)
+
+ test("$<", nil...)
+
+ // This one occurs in a few Perl packages.
+ test("$@", nil...)
+}
+
func (s *Suite) Test_CheckLinesMessage__one_line_of_text(c *check.C) {
t := s.Init(c)
@@ -990,18 +1025,15 @@ func (s *Suite) Test_Pkglint_checkReg__r
t.Main("category/package", "wip/package")
t.CheckOutputLines(
- "ERROR: category/package/README: Packages in main pkgsrc must not have a README file.",
"ERROR: category/package/TODO: Packages in main pkgsrc must not have a TODO file.",
- "2 errors found.")
+ "1 error found.")
t.Main("--import", "category/package", "wip/package")
t.CheckOutputLines(
- "ERROR: category/package/README: Packages in main pkgsrc must not have a README file.",
"ERROR: category/package/TODO: Packages in main pkgsrc must not have a TODO file.",
- "ERROR: wip/package/README: Must be cleaned up before committing the package.",
"ERROR: wip/package/TODO: Must be cleaned up before committing the package.",
- "4 errors found.")
+ "2 errors found.")
}
func (s *Suite) Test_Pkglint_checkReg__unknown_file_in_patches(c *check.C) {
Index: pkgsrc/pkgtools/pkglint/files/mkparser_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mkparser_test.go:1.37 pkgsrc/pkgtools/pkglint/files/mkparser_test.go:1.38
--- pkgsrc/pkgtools/pkglint/files/mkparser_test.go:1.37 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mkparser_test.go Sun Dec 8 22:03:38 2019
@@ -12,7 +12,7 @@ func (s *Suite) Test_MkParser_MkCond(c *
testRest := func(input string, expectedTree *MkCond, expectedRest string) {
// As of July 2019 p.MkCond does not emit warnings;
- // this is left to MkLineChecker.checkDirectiveCond.
+ // this is left to MkCondChecker.Check.
line := t.NewLine("filename.mk", 1, ".if "+input)
p := NewMkParser(line, input)
actualTree := p.MkCond()
Index: pkgsrc/pkgtools/pkglint/files/mktypes_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mktypes_test.go:1.19 pkgsrc/pkgtools/pkglint/files/mktypes_test.go:1.20
--- pkgsrc/pkgtools/pkglint/files/mktypes_test.go:1.19 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/mktypes_test.go Sun Dec 8 22:03:38 2019
@@ -192,7 +192,9 @@ func (s *Suite) Test_MkVarUseModifier_Ch
test("E", false)
test("H", false)
- // FIXME: The :M and :N modifiers obviously change the number of words.
+ // The :M and :N modifiers may reduce the number of words in a
+ // variable, but they don't change the interpretation from a list
+ // to a non-list.
test("Mpattern", false)
test("Npattern", false)
Index: pkgsrc/pkgtools/pkglint/files/options.go
diff -u pkgsrc/pkgtools/pkglint/files/options.go:1.19 pkgsrc/pkgtools/pkglint/files/options.go:1.20
--- pkgsrc/pkgtools/pkglint/files/options.go:1.19 Sat Nov 2 16:37:48 2019
+++ pkgsrc/pkgtools/pkglint/files/options.go Sun Dec 8 22:03:38 2019
@@ -184,7 +184,8 @@ func (ck *OptionsLinesChecker) handleLow
Var: recordVarUse})
if cond.Empty != nil && cond.Empty.varname == "PKG_OPTIONS" && mkline.HasElseBranch() {
- mkline.Notef("The positive branch of the .if/.else should be the one where the option is set.")
+ mkline.Warnf("The positive branch of the .if/.else " +
+ "should be the one where the option is set.")
mkline.Explain(
"For consistency among packages, the upper branch of this",
".if/.else statement should always handle the case where the",
Index: pkgsrc/pkgtools/pkglint/files/options_test.go
diff -u pkgsrc/pkgtools/pkglint/files/options_test.go:1.21 pkgsrc/pkgtools/pkglint/files/options_test.go:1.22
--- pkgsrc/pkgtools/pkglint/files/options_test.go:1.21 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/options_test.go Sun Dec 8 22:03:38 2019
@@ -305,7 +305,7 @@ func (s *Suite) Test_CheckLinesOptionsMk
t.CheckOutputLines(
"WARN: ~/category/package/options.mk:6: l is used but not defined.",
"WARN: ~/category/package/options.mk:18: Unknown option \"undeclared\".",
- "NOTE: ~/category/package/options.mk:21: "+
+ "WARN: ~/category/package/options.mk:21: "+
"The positive branch of the .if/.else should be the one where the option is set.",
// TODO: The diagnostics should appear in the correct order.
"WARN: ~/category/package/options.mk:6: "+
Index: pkgsrc/pkgtools/pkglint/files/package.go
diff -u pkgsrc/pkgtools/pkglint/files/package.go:1.73 pkgsrc/pkgtools/pkglint/files/package.go:1.74
--- pkgsrc/pkgtools/pkglint/files/package.go:1.73 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/package.go Sun Dec 8 22:03:38 2019
@@ -78,7 +78,7 @@ type Package struct {
}
func NewPackage(dir CurrPath) *Package {
- pkgpath := G.Pkgsrc.ToRel(dir)
+ pkgpath := G.Pkgsrc.Rel(dir)
// Package directory must be two subdirectories below the pkgsrc root.
// As of November 2019, it is technically possible to create packages
@@ -143,8 +143,7 @@ func (pkg *Package) load() ([]CurrPath,
if !hasPrefix(basename, "Makefile.") && !filename.HasSuffixText(".mk") {
return false
}
- // FIXME: consider DirNoClean
- if filename.DirClean().Base() == "patches" {
+ if filename.DirNoClean().Base() == "patches" {
return false
}
if pkg.Pkgdir == "." {
@@ -247,15 +246,14 @@ func (pkg *Package) parse(mklines *MkLin
func(mkline *MkLine) {})
if includingFileForUsedCheck != "" {
- mklines.CheckUsedBy(G.Pkgsrc.ToRel(includingFileForUsedCheck))
+ mklines.CheckUsedBy(G.Pkgsrc.Rel(includingFileForUsedCheck))
}
// For every included buildlink3.mk, include the corresponding builtin.mk
// automatically since the pkgsrc infrastructure does the same.
filename := mklines.lines.Filename
if filename.Base() == "buildlink3.mk" {
- // FIXME: consider DirNoClean
- builtin := filename.DirClean().JoinNoClean("builtin.mk").CleanPath()
+ builtin := filename.DirNoClean().JoinNoClean("builtin.mk").CleanPath()
builtinRel := G.Pkgsrc.Relpath(pkg.dir, builtin)
if pkg.included.FirstTime(builtinRel.String()) && builtin.IsFile() {
builtinMkLines := LoadMk(builtin, MustSucceed|LogErrors)
@@ -276,7 +274,7 @@ func (pkg *Package) parseLine(mklines *M
includedMkLines, skip := pkg.loadIncluded(mkline, includingFile)
if includedMkLines == nil {
- pkgsrcPath := G.Pkgsrc.ToRel(mkline.File(includedFile))
+ pkgsrcPath := G.Pkgsrc.Rel(mkline.File(includedFile))
if skip || mklines.indentation.HasExists(pkgsrcPath) {
return true // See https://github.com/rillig/pkglint/issues/1
}
@@ -372,7 +370,7 @@ func (pkg *Package) loadIncluded(mkline
return nil, false
}
- mkline.Notef("The path to the included file should be %q.",
+ mkline.Warnf("The path to the included file should be %q.",
mkline.Rel(fullIncludedFallback))
mkline.Explain(
"The .include directive first searches the file relative to the including file.",
@@ -389,7 +387,7 @@ func (pkg *Package) loadIncluded(mkline
// their actual values.
func (pkg *Package) resolveIncludedFile(mkline *MkLine, includingFilename CurrPath) RelPath {
- // FIXME: resolveVariableRefs uses G.Pkg implicitly. It should be made explicit.
+ // XXX: resolveVariableRefs uses G.Pkg implicitly. It should be made explicit.
// TODO: Try to combine resolveVariableRefs and ResolveVarsInRelativePath.
resolved := mkline.ResolveVarsInRelativePath(mkline.IncludedFile())
includedText := resolveVariableRefs(nil /* XXX: or maybe some mklines? */, resolved.String())
@@ -515,7 +513,7 @@ func (pkg *Package) check(filenames []Cu
// since all those files come from calls to dirglob.
break
- case filename.HasBase("Makefile") && G.Pkgsrc.ToRel(filename).Count() == 3:
+ case filename.HasBase("Makefile") && G.Pkgsrc.Rel(filename).Count() == 3:
G.checkExecutable(filename, st.Mode())
pkg.checkfilePackageMakefile(filename, mklines, allLines)
@@ -593,10 +591,13 @@ func (pkg *Package) checkfilePackageMake
pkg.redundant = NewRedundantScope()
pkg.redundant.IsRelevant = func(mkline *MkLine) bool {
- if !G.Infrastructure && !G.Opts.CheckGlobal {
- return !G.Pkgsrc.IsInfra(mkline.Filename)
- }
- return true
+ // As of December 2019, the RedundantScope is only used for
+ // checking a whole package. Therefore, G.Infrastructure can
+ // never be true and there is no point testing it.
+ //
+ // If the RedundantScope is applied also to individual files,
+ // it would have to be added here.
+ return G.Opts.CheckGlobal || !G.Pkgsrc.IsInfra(mkline.Filename)
}
pkg.redundant.Check(allLines) // Updates the variables in the scope
pkg.checkCategories()
@@ -941,7 +942,7 @@ func (pkg *Package) checkCategories() {
return
}
- // FIXME: Decide what exactly this map means.
+ // XXX: Decide what exactly this map means.
// Is it "this category has been seen somewhere",
// or is it "this category has definitely been added"?
seen := map[string]*MkLine{}
@@ -949,7 +950,7 @@ func (pkg *Package) checkCategories() {
switch mkline.Op() {
case opAssignDefault:
for _, category := range mkline.ValueFields(mkline.Value()) {
- // FIXME: This looks wrong. It should probably be replaced by
+ // XXX: This looks wrong. It should probably be replaced by
// an "if len(seen) == 0" outside the for loop.
if seen[category] == nil {
seen[category] = mkline
@@ -1267,7 +1268,7 @@ func (pkg *Package) checkDirent(dirent C
switch {
case mode.IsRegular():
- G.checkReg(dirent, basename, G.Pkgsrc.ToRel(dirent).Count())
+ G.checkReg(dirent, basename, G.Pkgsrc.Rel(dirent).Count())
case hasPrefix(basename, "work"):
if G.Opts.Import {
@@ -1279,8 +1280,7 @@ func (pkg *Package) checkDirent(dirent C
switch {
case basename == "files",
basename == "patches",
- // FIXME: consider DirNoClean
- dirent.DirClean().Base() == "files",
+ dirent.DirNoClean().Base() == "files",
isEmptyDir(dirent):
break
Index: pkgsrc/pkgtools/pkglint/files/package_test.go
diff -u pkgsrc/pkgtools/pkglint/files/package_test.go:1.62 pkgsrc/pkgtools/pkglint/files/package_test.go:1.63
--- pkgsrc/pkgtools/pkglint/files/package_test.go:1.62 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/package_test.go Sun Dec 8 22:03:38 2019
@@ -316,7 +316,7 @@ func (s *Suite) Test_Package__case_insen
G.Check(t.File("category/package"))
- // FIXME: On a case-sensitive filesystem, p5-net-dns would not be found.
+ // TODO: On a case-sensitive filesystem, p5-net-dns would not be found.
t.CheckOutputEmpty()
}
@@ -1005,7 +1005,7 @@ func (s *Suite) Test_Package_parse__fall
G.Check(t.File("category/package"))
t.CheckOutputLines(
- "NOTE: ~/mk/pthread.buildlink3.mk:2: " +
+ "WARN: ~/mk/pthread.buildlink3.mk:2: " +
"The path to the included file should be \"pthread.builtin.mk\".")
}
@@ -1503,6 +1503,38 @@ func (s *Suite) Test_Package_checkfilePa
t.CheckOutputEmpty()
}
+func (s *Suite) Test_Package_checkfilePackageMakefile__redundancy_in_infra(c *check.C) {
+ t := s.Init(c)
+
+ t.SetUpPackage("category/package",
+ ".include \"../../mk/redundant.mk\"",
+ ".include \"redundant.mk\"")
+ t.CreateFileLines("mk/redundant.mk",
+ MkCvsID,
+ "INFRA_REDUNDANT:=\t# empty",
+ "INFRA_REDUNDANT=\t# empty")
+ t.CreateFileLines("category/package/redundant.mk",
+ MkCvsID,
+ "PKG_REDUNDANT:=\t# empty",
+ "PKG_REDUNDANT=\t# empty")
+ t.Chdir(".")
+ t.FinishSetUp()
+
+ G.checkdirPackage("category/package")
+
+ t.CheckOutputLines(
+ "NOTE: category/package/redundant.mk:3: "+
+ "Definition of PKG_REDUNDANT is redundant because of line 2.",
+ "WARN: category/package/redundant.mk:2: "+
+ "PKG_REDUNDANT is defined but not used.")
+
+ G.Check("mk/redundant.mk")
+
+ // The redundancy check is only performed when a whole package
+ // is checked. Therefore nothing is reported here.
+ t.CheckOutputEmpty()
+}
+
// When a package defines PLIST_SRC, it may or may not use the
// PLIST file from the package directory. Therefore the check
// is skipped completely.
Index: pkgsrc/pkgtools/pkglint/files/path.go
diff -u pkgsrc/pkgtools/pkglint/files/path.go:1.5 pkgsrc/pkgtools/pkglint/files/path.go:1.6
--- pkgsrc/pkgtools/pkglint/files/path.go:1.5 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/path.go Sun Dec 8 22:03:38 2019
@@ -408,8 +408,7 @@ func NewPackagePath(p RelPath) PackagePa
}
func NewPackagePathString(p string) PackagePath {
- _ = NewRelPathString(p)
- return PackagePath(p)
+ return PackagePath(NewRelPathString(p))
}
func (p PackagePath) AsPath() Path { return Path(p) }
@@ -434,8 +433,7 @@ func NewRelPath(p Path) RelPath {
}
func NewRelPathString(p string) RelPath {
- assert(!NewPath(p).IsAbs())
- return RelPath(p)
+ return NewRelPath(NewPath(p))
}
func (p RelPath) AsPath() Path { return NewPath(string(p)) }
Index: pkgsrc/pkgtools/pkglint/files/path_test.go
diff -u pkgsrc/pkgtools/pkglint/files/path_test.go:1.5 pkgsrc/pkgtools/pkglint/files/path_test.go:1.6
--- pkgsrc/pkgtools/pkglint/files/path_test.go:1.5 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/path_test.go Sun Dec 8 22:03:38 2019
@@ -87,6 +87,8 @@ func (s *Suite) Test_Path_DirNoClean(c *
test("filename", ".")
test("dir/filename", "dir")
test("dir/filename\\with\\backslash", "dir")
+ test("dir/./file", "dir")
+ test("./file", ".")
}
func (s *Suite) Test_Path_Base(c *check.C) {
@@ -427,6 +429,7 @@ func (s *Suite) Test_Path_CleanDot(c *ch
test("", "")
test(".", ".")
test("./././", ".")
+ test("dir/", "dir/") // TODO: Or maybe "dir/."?
test("a/bb///../c", "a/bb/../c")
test("./filename", "filename")
test("/absolute", "/absolute")
Index: pkgsrc/pkgtools/pkglint/files/vargroups.go
diff -u pkgsrc/pkgtools/pkglint/files/vargroups.go:1.5 pkgsrc/pkgtools/pkglint/files/vargroups.go:1.6
--- pkgsrc/pkgtools/pkglint/files/vargroups.go:1.5 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/vargroups.go Sun Dec 8 22:03:38 2019
@@ -39,7 +39,7 @@ func NewVargroupsChecker(mklines *MkLine
func (ck *VargroupsChecker) init() {
mklines := ck.mklines
- scope := mklines.vars
+ scope := mklines.allVars
if !scope.IsDefined("_VARGROUPS") {
ck.skip = true
return
Index: pkgsrc/pkgtools/pkglint/files/pkglint.1
diff -u pkgsrc/pkgtools/pkglint/files/pkglint.1:1.60 pkgsrc/pkgtools/pkglint/files/pkglint.1:1.61
--- pkgsrc/pkgtools/pkglint/files/pkglint.1:1.60 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/pkglint.1 Sun Dec 8 22:03:38 2019
@@ -1,4 +1,4 @@
-.\" $NetBSD: pkglint.1,v 1.60 2019/12/08 00:06:38 rillig Exp $
+.\" $NetBSD: pkglint.1,v 1.61 2019/12/08 22:03:38 rillig Exp $
.\" From FreeBSD: portlint.1,v 1.8 1997/11/25 14:53:14 itojun Exp
.\"
.\" Copyright (c) 1997 by Jun-ichiro Itoh <itojun%itojun.org@localhost>.
Index: pkgsrc/pkgtools/pkglint/files/shell_test.go
diff -u pkgsrc/pkgtools/pkglint/files/shell_test.go:1.60 pkgsrc/pkgtools/pkglint/files/shell_test.go:1.61
--- pkgsrc/pkgtools/pkglint/files/shell_test.go:1.60 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/shell_test.go Sun Dec 8 22:03:38 2019
@@ -131,7 +131,7 @@ func (s *Suite) Test_SimpleCommandChecke
// On the contrary, when pkglint checks a single .mk file, these
// commands are not guaranteed to be defined, not even when the
// .mk file includes the file defining the command.
-// FIXME: This paragraph sounds wrong. All commands from included files should be valid.
+// TODO: This paragraph sounds wrong. All commands from included files should be valid.
//
// The PYTHON_BIN variable below must not be called *_CMD, or another code path is taken.
func (s *Suite) Test_SimpleCommandChecker_handleCommandVariable__from_package(c *check.C) {
@@ -1222,9 +1222,9 @@ func (s *Suite) Test_ShellLineChecker_Ch
mklines.Check()
- // FIXME: Fix the parse errors (nested subshells).
- // FIXME: Fix the duplicate diagnostic in line 6.
- // FIXME: "(" is not a shell command, it's an operator.
+ // XXX: Fix the parse errors (nested subshells).
+ // XXX: Fix the duplicate diagnostic in line 6.
+ // TODO: "(" is not a shell command, it's an operator.
t.CheckOutputLines(
"WARN: Makefile:4: The shell command \"(\" should not be hidden.",
"WARN: Makefile:5: Internal pkglint error in ShTokenizer.ShAtom at \"$$(echo 1024))\" (quoting=S).",
@@ -1350,7 +1350,7 @@ func (s *Suite) Test_ShellLineChecker_Ch
ck.CheckWord(ck.mkline.ShellCommand(), false, RunTime)
- // FIXME: Make consumes the dollar silently.
+ // XXX: Make consumes the dollar silently.
// This could be worth another pkglint warning.
t.CheckOutputEmpty()
}
Index: pkgsrc/pkgtools/pkglint/files/pkglint.go
diff -u pkgsrc/pkgtools/pkglint/files/pkglint.go:1.69 pkgsrc/pkgtools/pkglint/files/pkglint.go:1.70
--- pkgsrc/pkgtools/pkglint/files/pkglint.go:1.69 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/pkglint.go Sun Dec 8 22:03:38 2019
@@ -201,8 +201,7 @@ func (pkglint *Pkglint) setUpProfiling()
func (pkglint *Pkglint) prepareMainLoop() {
firstDir := pkglint.Todo.Front()
if firstDir.IsFile() {
- // FIXME: consider DirNoClean
- firstDir = firstDir.DirClean()
+ firstDir = firstDir.DirNoClean()
}
relTopdir := findPkgsrcTopdir(firstDir)
@@ -318,7 +317,7 @@ func (pkglint *Pkglint) checkMode(dirent
}
basename := dirent.Base()
- pkgsrcRel := pkglint.Pkgsrc.ToRel(dirent)
+ pkgsrcRel := pkglint.Pkgsrc.Rel(dirent)
pkglint.Wip = pkgsrcRel.HasPrefixPath("wip")
pkglint.Infrastructure = pkgsrcRel.HasPrefixPath("mk")
@@ -393,7 +392,8 @@ func resolveVariableRefs(mklines *MkLine
}
}
if mklines != nil {
- if value, ok := mklines.vars.LastValueFound(varname); ok {
+ // TODO: At load time, use mklines.loadVars instead.
+ if value, ok := mklines.allVars.LastValueFound(varname); ok {
return value
}
}
@@ -430,20 +430,26 @@ func CheckLinesDescr(lines *Lines) {
defer trace.Call(lines.Filename)()
}
+ checkVarRefs := func(line *Line) {
+ tokens, _ := NewMkLexer(line.Text, nil).MkTokens()
+ for _, token := range tokens {
+ switch {
+ case token.Varuse == nil,
+ !hasPrefix(token.Text, "${"),
+ G.Pkgsrc.VariableType(nil, token.Varuse.varname) == nil:
+ default:
+ line.Notef("Variables like %q are not expanded in the DESCR file.",
+ token.Text)
+ }
+ }
+ }
+
for _, line := range lines.Lines {
ck := LineChecker{line}
ck.CheckLength(80)
ck.CheckTrailingWhitespace()
ck.CheckValidCharacters()
-
- if containsVarRef(line.Text) {
- tokens, _ := NewMkLexer(line.Text, nil).MkTokens()
- for _, token := range tokens {
- if token.Varuse != nil && G.Pkgsrc.VariableType(nil, token.Varuse.varname) != nil {
- line.Notef("Variables are not expanded in the DESCR file.")
- }
- }
- }
+ checkVarRefs(line)
}
CheckLinesTrailingEmptyLines(lines)
@@ -542,8 +548,7 @@ func CheckFileMk(filename CurrPath) {
func (pkglint *Pkglint) checkReg(filename CurrPath, basename string, depth int) {
if depth == 3 && !pkglint.Wip {
- // FIXME: There's no good reason for prohibiting a README file.
- if contains(basename, "README") || contains(basename, "TODO") {
+ if contains(basename, "TODO") {
NewLineWhole(filename).Errorf("Packages in main pkgsrc must not have a %s file.", basename)
// TODO: Add a convincing explanation.
return
@@ -554,7 +559,6 @@ func (pkglint *Pkglint) checkReg(filenam
case hasSuffix(basename, "~"),
hasSuffix(basename, ".orig"),
hasSuffix(basename, ".rej"),
- contains(basename, "README") && depth == 3,
contains(basename, "TODO") && depth == 3:
if pkglint.Opts.Import {
NewLineWhole(filename).Errorf("Must be cleaned up before committing the package.")
@@ -599,20 +603,16 @@ func (pkglint *Pkglint) checkReg(filenam
CheckLinesPatch(lines)
}
- // FIXME: consider DirNoClean
- case filename.DirClean().Base() == "patches" && matches(filename.Base(), `^manual[^/]*$`):
+ case filename.DirNoClean().Base() == "patches" && matches(filename.Base(), `^manual[^/]*$`):
if trace.Tracing {
trace.Stepf("Unchecked file %q.", filename)
}
- // FIXME: consider DirNoClean
- case filename.DirClean().Base() == "patches":
+ case filename.DirNoClean().Base() == "patches":
NewLineWhole(filename).Warnf("Patch files should be named \"patch-\", followed by letters, '-', '_', '.', and digits only.")
case (hasPrefix(basename, "Makefile") || hasSuffix(basename, ".mk")) &&
- // FIXME: consider DirNoClean
- // FIXME: G.Pkgsrc.Rel(filename) instead of filename
- !filename.DirClean().ContainsPath("files"):
+ !G.Pkgsrc.Rel(filename).AsPath().ContainsPath("files"):
CheckFileMk(filename)
case hasPrefix(basename, "PLIST"):
@@ -620,16 +620,18 @@ func (pkglint *Pkglint) checkReg(filenam
CheckLinesPlist(G.Pkg, lines)
}
+ case contains(basename, "README"):
+ break
+
case hasPrefix(basename, "CHANGES-"):
// This only checks the file but doesn't register the changes globally.
_ = pkglint.Pkgsrc.loadDocChangesFromFile(filename)
- // FIXME: consider DirNoClean
- case filename.DirClean().Base() == "files":
+ case filename.DirNoClean().Base() == "files":
// Skip files directly in the files/ directory, but not those further down.
case basename == "spec":
- if !pkglint.Pkgsrc.ToRel(filename).HasPrefixPath("regress") {
+ if !pkglint.Pkgsrc.Rel(filename).HasPrefixPath("regress") {
NewLineWhole(filename).Warnf("Only packages in regress/ may have spec files.")
}
@@ -731,7 +733,6 @@ func (pkglint *Pkglint) tools(mklines *M
}
func (pkglint *Pkglint) loadCvsEntries(filename CurrPath) map[string]CvsEntry {
- // FIXME: consider DirNoClean
dir := filename.DirClean()
if dir == pkglint.cvsEntriesDir {
return pkglint.cvsEntries
Index: pkgsrc/pkgtools/pkglint/files/pkgsrc.go
diff -u pkgsrc/pkgtools/pkglint/files/pkgsrc.go:1.46 pkgsrc/pkgtools/pkglint/files/pkgsrc.go:1.47
--- pkgsrc/pkgtools/pkglint/files/pkgsrc.go:1.46 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/pkgsrc.go Sun Dec 8 22:03:38 2019
@@ -150,8 +150,8 @@ func (src *Pkgsrc) loadDocChanges() {
var filenames []RelPath
for _, file := range files {
filename := file.Name()
- if matches(filename, `^CHANGES-20\d\d$`) && filename >= "CHANGES-2011" { // TODO: Why 2011?
- filenames = append(filenames, NewRelPathString(filename)) // FIXME: low-level API
+ if matches(filename, `^CHANGES-20\d\d$`) && filename >= "CHANGES-2011" { // XXX: Why 2011?
+ filenames = append(filenames, NewRelPathString(filename)) // XXX: low-level API
}
}
@@ -676,10 +676,10 @@ func (src *Pkgsrc) loadUntypedVars() {
mklines := LoadMk(path, MustSucceed)
mklines.collectVariables()
mklines.collectUsedVariables()
- for varname, mkline := range mklines.vars.firstDef {
+ for varname, mkline := range mklines.allVars.firstDef {
define(varnameCanon(varname), mkline)
}
- for varname, mkline := range mklines.vars.used {
+ for varname, mkline := range mklines.allVars.used {
define(varnameCanon(varname), mkline)
}
}
@@ -688,7 +688,7 @@ func (src *Pkgsrc) loadUntypedVars() {
assertNil(err, "handleFile %q", pathName)
baseName := info.Name()
if info.Mode().IsRegular() && (hasSuffix(baseName, ".mk") || baseName == "mk.conf") {
- handleMkFile(NewCurrPathSlash(pathName)) // FIXME: This is too deep to handle os-specific paths
+ handleMkFile(NewCurrPathSlash(pathName)) // XXX: This is too deep to handle os-specific paths
}
return nil
}
@@ -1117,29 +1117,28 @@ func (src *Pkgsrc) File(relativeName Pkg
return src.topdir.JoinNoClean(cleaned).CleanDot()
}
-// ToRel returns the path of `filename`, relative to the pkgsrc top directory.
+// Rel returns the path of `filename`, relative to the pkgsrc top directory.
//
// Example:
-// NewPkgsrc("/usr/pkgsrc").ToRel("/usr/pkgsrc/distfiles") => "distfiles"
-// FIXME: Rename to Rel.
-func (src *Pkgsrc) ToRel(filename CurrPath) PkgsrcPath {
+// NewPkgsrc("/usr/pkgsrc").Rel("/usr/pkgsrc/distfiles") => "distfiles"
+func (src *Pkgsrc) Rel(filename CurrPath) PkgsrcPath {
return NewPkgsrcPath(src.Relpath(src.topdir, filename).AsPath())
}
// IsInfra returns whether the given filename is part of the pkgsrc
// infrastructure.
func (src *Pkgsrc) IsInfra(filename CurrPath) bool {
- rel := src.ToRel(filename)
+ rel := src.Rel(filename)
return rel.HasPrefixPath("mk") || rel.HasPrefixPath("wip/mk")
}
func (src *Pkgsrc) IsInfraMain(filename CurrPath) bool {
- rel := src.ToRel(filename)
+ rel := src.Rel(filename)
return rel.HasPrefixPath("mk")
}
func (src *Pkgsrc) IsWip(filename CurrPath) bool {
- rel := src.ToRel(filename)
+ rel := src.Rel(filename)
return rel.HasPrefixPath("wip")
}
Index: pkgsrc/pkgtools/pkglint/files/plist.go
diff -u pkgsrc/pkgtools/pkglint/files/plist.go:1.47 pkgsrc/pkgtools/pkglint/files/plist.go:1.48
--- pkgsrc/pkgtools/pkglint/files/plist.go:1.47 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/plist.go Sun Dec 8 22:03:38 2019
@@ -2,7 +2,6 @@ package pkglint
import (
"netbsd.org/pkglint/textproc"
- "path"
"sort"
"strings"
)
@@ -29,13 +28,7 @@ func CheckLinesPlist(pkg *Package, lines
return
}
- ck := PlistChecker{
- pkg,
- make(map[RelPath]*PlistLine),
- make(map[RelPath]*PlistLine),
- "",
- Once{},
- false}
+ ck := NewPlistChecker(pkg)
ck.Check(lines)
}
@@ -43,19 +36,29 @@ type PlistChecker struct {
pkg *Package
allFiles map[RelPath]*PlistLine
allDirs map[RelPath]*PlistLine
- lastFname string
+ lastFname RelPath
once Once
nonAsciiAllowed bool
}
+func NewPlistChecker(pkg *Package) *PlistChecker {
+ return &PlistChecker{
+ pkg,
+ make(map[RelPath]*PlistLine),
+ make(map[RelPath]*PlistLine),
+ "",
+ Once{},
+ false}
+}
+
func (ck *PlistChecker) Load(lines *Lines) []*PlistLine {
- plines := ck.NewLines(lines)
+ plines := ck.newLines(lines)
ck.collectFilesAndDirs(plines)
if lines.BaseName == "PLIST.common_end" {
commonLines := Load(lines.Filename.TrimSuffix("_end"), NotEmpty)
if commonLines != nil {
- ck.collectFilesAndDirs(ck.NewLines(commonLines))
+ ck.collectFilesAndDirs(ck.newLines(commonLines))
}
}
@@ -78,7 +81,7 @@ func (ck *PlistChecker) Check(plainLines
}
}
-func (ck *PlistChecker) NewLines(lines *Lines) []*PlistLine {
+func (*PlistChecker) newLines(lines *Lines) []*PlistLine {
plines := make([]*PlistLine, lines.Len())
for i, line := range lines.Lines {
var conditions []string
@@ -103,33 +106,41 @@ var plistLineStart = textproc.NewByteSet
func (ck *PlistChecker) collectFilesAndDirs(plines []*PlistLine) {
for _, pline := range plines {
- if text := pline.text; len(text) > 0 {
- first := text[0]
- switch {
- case plistLineStart.Contains(first):
- // FIXME: Add test for absolute path.
- path := NewRelPathString(text)
- if prev := ck.allFiles[path]; prev == nil || stringSliceLess(pline.conditions, prev.conditions) {
- ck.allFiles[path] = pline
- }
- // FIXME: consider DirNoClean
- // FIXME: consider DirNoClean
- for dir := path.DirClean(); dir != "."; dir = dir.DirClean() {
- ck.allDirs[dir] = pline
- }
- case first == '@':
- if m, dirname := match1(text, `^@exec \$\{MKDIR\} %D/(.*)$`); m {
- // FIXME: consider DirNoClean
- // FIXME: Add test for absolute path.
- for dir := NewRelPathString(dirname); dir != "."; dir = dir.DirClean() {
- ck.allDirs[dir] = pline
- }
- }
- }
+ text := pline.text
+ switch {
+ case text == "":
+ break
+ case plistLineStart.Contains(text[0]):
+ ck.collectPath(NewRelPathString(text), pline)
+ case text[0] == '@':
+ ck.collectDirective(pline)
}
}
}
+func (ck *PlistChecker) collectPath(rel RelPath, pline *PlistLine) {
+
+ // TODO: What about paths containing variables?
+ // Are they intended to be collected as well?
+
+ if prev := ck.allFiles[rel]; prev == nil || stringSliceLess(pline.conditions, prev.conditions) {
+ ck.allFiles[rel] = pline
+ }
+ for dir := rel.DirNoClean(); dir != "."; dir = dir.DirNoClean() {
+ ck.allDirs[dir] = pline
+ }
+}
+
+func (ck *PlistChecker) collectDirective(pline *PlistLine) {
+ m, dirname := match1(pline.text, `^@exec \$\{MKDIR\} %D/(.*)$`)
+ if !m || NewPath(dirname).IsAbs() {
+ return
+ }
+ for dir := NewRelPathString(dirname); dir != "."; dir = dir.DirNoClean() {
+ ck.allDirs[dir] = pline
+ }
+}
+
func (ck *PlistChecker) checkLine(pline *PlistLine) {
text := pline.text
@@ -139,32 +150,30 @@ func (ck *PlistChecker) checkLine(pline
fix.Delete()
fix.Apply()
- } else if textproc.AlnumU.Contains(text[0]) || text[0] == '$' {
- ck.checkPath(pline)
+ } else if plistLineStart.Contains(text[0]) {
+ ck.checkPath(pline, pline.Path())
} else if m, cmd, arg := match2(text, `^@([a-z-]+)[\t ]*(.*)`); m {
pline.CheckDirective(cmd, arg)
- ck.nonAsciiAllowed = pline.firstLine > 1
+ if cmd == "comment" && pline.firstLine > 1 {
+ ck.nonAsciiAllowed = true
+ }
} else {
- pline.Warnf("Invalid line type: %s", pline.Line.Text)
+ pline.Errorf("Invalid line type: %s", pline.Line.Text)
}
}
-func (ck *PlistChecker) checkPath(pline *PlistLine) {
- text := pline.text
- dirSlash, basename := path.Split(text)
- dirname := strings.TrimSuffix(dirSlash, "/")
-
+func (ck *PlistChecker) checkPath(pline *PlistLine, rel RelPath) {
ck.checkPathNonAscii(pline)
ck.checkSorted(pline)
ck.checkDuplicate(pline)
- if contains(basename, "${IMAKE_MANNEWSUFFIX}") {
+ if contains(rel.Base(), "${IMAKE_MANNEWSUFFIX}") {
pline.warnImakeMannewsuffix()
}
- if hasPrefix(text, "${PKGMANDIR}/") {
+ if rel.HasPrefixPath("${PKGMANDIR}") {
fix := pline.Autofix()
fix.Notef("PLIST files should use \"man/\" instead of \"${PKGMANDIR}\".")
fix.Explain(
@@ -177,44 +186,53 @@ func (ck *PlistChecker) checkPath(pline
pline.text = strings.Replace(pline.text, "${PKGMANDIR}/", "man/", 1)
}
- topdir := strings.SplitN(text, "/", 2)[0]
+ topdir := rel.Parts()[0]
switch topdir {
case "bin":
- ck.checkPathBin(pline, dirname, basename)
+ ck.checkPathBin(pline, rel)
case "doc":
pline.Errorf("Documentation must be installed under share/doc, not doc.")
case "etc":
- ck.checkPathEtc(pline, dirname, basename)
+ ck.checkPathEtc(pline)
case "info":
- ck.checkPathInfo(pline, dirname, basename)
+ ck.checkPathInfo(pline)
case "lib":
- ck.checkPathLib(pline, dirname, basename)
+ ck.checkPathLib(pline, rel)
case "man":
ck.checkPathMan(pline)
case "share":
ck.checkPathShare(pline)
}
- if contains(text, "${PKGLOCALEDIR}") && ck.pkg != nil && !ck.pkg.vars.IsDefined("USE_PKGLOCALEDIR") {
+ ck.checkPathMisc(rel, pline)
+}
+
+func (ck *PlistChecker) checkPathMisc(rel RelPath, pline *PlistLine) {
+ if rel.ContainsText("${PKGLOCALEDIR}") && ck.pkg != nil && !ck.pkg.vars.IsDefined("USE_PKGLOCALEDIR") {
pline.Warnf("PLIST contains ${PKGLOCALEDIR}, but USE_PKGLOCALEDIR is not set in the package Makefile.")
}
- if contains(text, "/CVS/") {
+ if rel.ContainsPath("CVS") {
pline.Warnf("CVS files should not be in the PLIST.")
}
- if hasSuffix(text, ".orig") {
+ if rel.HasSuffixText(".orig") {
pline.Warnf(".orig files should not be in the PLIST.")
}
- if hasSuffix(text, "/perllocal.pod") {
+ if rel.HasBase("perllocal.pod") {
pline.Warnf("The perllocal.pod file should not be in the PLIST.")
pline.Explain(
"This file is handled automatically by the INSTALL/DEINSTALL scripts",
"since its contents depends on more than one package.")
}
- if contains(text, ".egg-info/") {
+ if rel.ContainsText(".egg-info/") {
pline.Warnf("Include \"../../lang/python/egg.mk\" instead of listing .egg-info files directly.")
}
+ if rel.ContainsPath("..") {
+ pline.Errorf("Paths in PLIST files must not contain \"..\".")
+ } else if canonical := rel.Clean(); canonical != rel {
+ pline.Errorf("Paths in PLIST files must be canonical (%s).", canonical)
+ }
}
func (ck *PlistChecker) checkPathNonAscii(pline *PlistLine) {
@@ -246,38 +264,38 @@ func (ck *PlistChecker) checkPathNonAsci
}
func (ck *PlistChecker) checkSorted(pline *PlistLine) {
- if text := pline.text; hasAlnumPrefix(text) && !containsVarRef(text) {
- if ck.lastFname != "" {
- if ck.lastFname > text && !G.Logger.Opts.Autofix {
- pline.Warnf("%q should be sorted before %q.", text, ck.lastFname)
- pline.Explain(
- "The files in the PLIST should be sorted alphabetically.",
- "This allows human readers to quickly see whether a file is included or not.")
- }
- }
- ck.lastFname = text
+ if !pline.HasPlainPath() {
+ return
+ }
+
+ rel := pline.Path()
+ if ck.lastFname != "" && ck.lastFname > rel && !G.Logger.Opts.Autofix {
+ pline.Warnf("%q should be sorted before %q.", rel.String(), ck.lastFname.String())
+ pline.Explain(
+ "The files in the PLIST should be sorted alphabetically.",
+ "This allows human readers to quickly see whether a file is included or not.")
}
+ ck.lastFname = rel
}
func (ck *PlistChecker) checkDuplicate(pline *PlistLine) {
- text := pline.text
- if !hasAlnumPrefix(text) || containsVarRef(text) {
+ if !pline.HasPlainPath() {
return
}
- prev := ck.allFiles[NewRelPathString(text)]
+ prev := ck.allFiles[pline.Path()]
if prev == pline || len(prev.conditions) > 0 {
return
}
fix := pline.Autofix()
- fix.Errorf("Duplicate filename %q, already appeared in %s.", text, pline.RelLine(prev.Line))
+ fix.Errorf("Duplicate filename %q, already appeared in %s.", pline.text, pline.RelLine(prev.Line))
fix.Delete()
fix.Apply()
}
-func (ck *PlistChecker) checkPathBin(pline *PlistLine, dirname, basename string) {
- if contains(dirname, "/") {
+func (ck *PlistChecker) checkPathBin(pline *PlistLine, rel RelPath) {
+ if rel.Count() > 2 {
pline.Warnf("The bin/ directory should not have subdirectories.")
pline.Explain(
"The programs in bin/ are collected there to be executable by the",
@@ -288,7 +306,7 @@ func (ck *PlistChecker) checkPathBin(pli
}
}
-func (ck *PlistChecker) checkPathEtc(pline *PlistLine, dirname, basename string) {
+func (ck *PlistChecker) checkPathEtc(pline *PlistLine) {
if hasPrefix(pline.text, "etc/rc.d/") {
pline.Errorf("RCD_SCRIPTS must not be registered in the PLIST.")
pline.Explain(
@@ -301,7 +319,7 @@ func (ck *PlistChecker) checkPathEtc(pli
"Please use the CONF_FILES framework, which is described in mk/pkginstall/bsd.pkginstall.mk.")
}
-func (ck *PlistChecker) checkPathInfo(pline *PlistLine, dirname, basename string) {
+func (ck *PlistChecker) checkPathInfo(pline *PlistLine) {
if pline.text == "info/dir" {
pline.Errorf("\"info/dir\" must not be listed. Use install-info to add/remove an entry.")
return
@@ -312,20 +330,23 @@ func (ck *PlistChecker) checkPathInfo(pl
}
}
-func (ck *PlistChecker) checkPathLib(pline *PlistLine, dirname, basename string) {
+func (ck *PlistChecker) checkPathLib(pline *PlistLine, rel RelPath) {
switch {
- case hasPrefix(pline.text, "lib/locale/"):
+ case rel.HasPrefixPath("lib/locale"):
pline.Errorf("\"lib/locale\" must not be listed. Use ${PKGLOCALEDIR}/locale and set USE_PKGLOCALEDIR instead.")
return
}
+ basename := rel.Base()
if contains(basename, ".a") || contains(basename, ".so") {
- if m, noext := match1(pline.text, `^(.*)(?:\.a|\.so[0-9.]*)$`); m {
- // FIXME: Add test for absolute path.
- if laLine := ck.allFiles[NewRelPathString(noext+".la")]; laLine != nil {
- pline.Warnf("Redundant library found. The libtool library is in %s.", pline.RelLine(laLine.Line))
+ la := replaceAll(pline.text, `(\.a|\.so[0-9.]*)$`, ".la")
+ if la != pline.text {
+ laLine := ck.allFiles[NewRelPathString(la)]
+ if laLine != nil {
+ pline.Warnf("Redundant library found. The libtool library is in %s.",
+ pline.RelLine(laLine.Line))
}
}
}
@@ -445,6 +466,15 @@ type PlistLine struct {
text string // Line.Text without any conditions of the form ${PLIST.cond}
}
+func (pline *PlistLine) Path() RelPath { return NewRelPathString(pline.text) }
+
+func (pline *PlistLine) HasPlainPath() bool {
+ text := pline.text
+ return text != "" &&
+ plistLineStart.Contains(text[0]) &&
+ !containsVarRef(text)
+}
+
func (pline *PlistLine) CheckTrailingWhitespace() {
if hasSuffix(pline.text, " ") || hasSuffix(pline.text, "\t") {
pline.Errorf("Pkgsrc does not support filenames ending in whitespace.")
@@ -549,6 +579,7 @@ func NewPlistLineSorter(plines []*PlistL
func (s *plistLineSorter) Sort() {
if line := s.unsortable; line != nil {
if G.Logger.IsAutofix() {
+ // FIXME: Missing trace.Enabled
trace.Stepf("%s: This line prevents pkglint from sorting the PLIST automatically.", line)
}
return
Index: pkgsrc/pkgtools/pkglint/files/plist_test.go
diff -u pkgsrc/pkgtools/pkglint/files/plist_test.go:1.41 pkgsrc/pkgtools/pkglint/files/plist_test.go:1.42
--- pkgsrc/pkgtools/pkglint/files/plist_test.go:1.41 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/plist_test.go Sun Dec 8 22:03:38 2019
@@ -24,7 +24,8 @@ func (s *Suite) Test_CheckLinesPlist(c *
"share/icons/hicolor/icon1.png",
"share/icons/hicolor/icon2.png", // No additional error for hicolor-icon-theme.
"share/tzinfo",
- "share/tzinfo")
+ "share/tzinfo",
+ "/absolute")
CheckLinesPlist(G.Pkg, lines)
@@ -45,7 +46,8 @@ func (s *Suite) Test_CheckLinesPlist(c *
"WARN: PLIST:14: Packages that install icon theme files should set ICON_THEMES.",
"ERROR: PLIST:15: Packages that install hicolor icons "+
"must include \"../../graphics/hicolor-icon-theme/buildlink3.mk\" in the Makefile.",
- "ERROR: PLIST:18: Duplicate filename \"share/tzinfo\", already appeared in line 17.")
+ "ERROR: PLIST:18: Duplicate filename \"share/tzinfo\", already appeared in line 17.",
+ "ERROR: PLIST:19: Invalid line type: /absolute")
}
func (s *Suite) Test_CheckLinesPlist__single_file_no_comment(c *check.C) {
@@ -336,11 +338,11 @@ func (s *Suite) Test_PlistChecker__inval
CheckLinesPlist(nil, lines)
t.CheckOutputLines(
- "WARN: ~/PLIST:2: Invalid line type: ---invalid",
- "WARN: ~/PLIST:3: Invalid line type: +++invalid",
- "WARN: ~/PLIST:4: Invalid line type: <<<<<<<< merge conflict",
- "WARN: ~/PLIST:5: Invalid line type: ======== merge conflict",
- "WARN: ~/PLIST:6: Invalid line type: >>>>>>>> merge conflict")
+ "ERROR: ~/PLIST:2: Invalid line type: ---invalid",
+ "ERROR: ~/PLIST:3: Invalid line type: +++invalid",
+ "ERROR: ~/PLIST:4: Invalid line type: <<<<<<<< merge conflict",
+ "ERROR: ~/PLIST:5: Invalid line type: ======== merge conflict",
+ "ERROR: ~/PLIST:6: Invalid line type: >>>>>>>> merge conflict")
}
func (s *Suite) Test_PlistChecker__doc(c *check.C) {
@@ -400,6 +402,153 @@ func (s *Suite) Test_PlistChecker__PKGLO
t.CheckOutputEmpty()
}
+func (s *Suite) Test_NewPlistChecker(c *check.C) {
+ t := s.Init(c)
+
+ pkg := NewPackage(t.File("category/package"))
+
+ ck := NewPlistChecker(pkg)
+
+ t.CheckEquals(ck.pkg, pkg)
+ t.Check(ck.allDirs, check.NotNil)
+ t.Check(ck.allFiles, check.NotNil)
+}
+
+func (s *Suite) Test_PlistChecker_Load__common_end(c *check.C) {
+ t := s.Init(c)
+
+ t.Chdir(".")
+ t.CreateFileLines("PLIST",
+ PlistCvsID,
+ "bin/plist")
+ t.CreateFileLines("PLIST.common",
+ PlistCvsID,
+ "bin/plist_common")
+ t.CreateFileLines("PLIST.common_end",
+ PlistCvsID,
+ "bin/plist_common_end")
+
+ ck := NewPlistChecker(nil)
+
+ plistLines := ck.Load(Load(t.File("PLIST.common_end"), MustSucceed))
+
+ // The corresponding PLIST.common is loaded if possible.
+ // Its lines are not appended to plistLines since they
+ // are checked separately.
+ t.Check(plistLines, check.HasLen, 2)
+
+ // But the files and directories from PLIST.common are registered,
+ // to check for duplicates and to make these lists available to
+ // the package being checked, for cross-validation.
+ t.Check(ck.allFiles["bin/plist"], check.IsNil)
+ t.CheckEquals(
+ ck.allFiles["bin/plist_common"].String(),
+ "PLIST.common:2: bin/plist_common")
+ t.CheckEquals(
+ ck.allFiles["bin/plist_common_end"].String(),
+ "PLIST.common_end:2: bin/plist_common_end")
+}
+
+func (s *Suite) Test_PlistChecker_Check(c *check.C) {
+ t := s.Init(c)
+
+ lines := t.NewLines("PLIST",
+ "bin/subdir/program")
+ ck := NewPlistChecker(nil)
+
+ ck.Check(lines)
+
+ t.CheckOutputLines(
+ "WARN: PLIST:1: The bin/ directory should not have subdirectories.")
+}
+
+func (s *Suite) Test_PlistChecker_newLines(c *check.C) {
+ t := s.Init(c)
+
+ lines := t.NewLines("PLIST",
+ "bin/program",
+ "${PLIST.cond}bin/conditional",
+ "${PLIST.abs}${PLIST.abs2}/bin/conditional-absolute",
+ "${PLIST.mod:Q}invalid")
+
+ plistLines := (*PlistChecker)(nil).newLines(lines)
+
+ // The invalid condition in line 4 is silently skipped when the
+ // lines are parsed. The actual check happens later.
+
+ t.Check(plistLines, check.HasLen, 4)
+ t.CheckEquals(plistLines[0].text, "bin/program")
+ t.CheckEquals(plistLines[1].text, "bin/conditional")
+ t.CheckEquals(plistLines[2].text, "/bin/conditional-absolute")
+ t.CheckEquals(plistLines[3].text, "${PLIST.mod:Q}invalid")
+
+ t.Check(plistLines[0].conditions, check.HasLen, 0)
+ t.CheckDeepEquals(plistLines[1].conditions, []string{"PLIST.cond"})
+ t.CheckDeepEquals(plistLines[2].conditions, []string{"PLIST.abs", "PLIST.abs2"})
+ t.Check(plistLines[3].conditions, check.HasLen, 0)
+}
+
+func (s *Suite) Test_PlistChecker_collectFilesAndDirs(c *check.C) {
+ t := s.Init(c)
+
+ lines := t.NewLines("PLIST",
+ PlistCvsID,
+ "bin/program",
+ "man/man1/program.1",
+ "/absolute",
+ "${PLIST.cond}/absolute",
+ "@exec ${MKDIR} %D//absolute")
+ ck := NewPlistChecker(nil)
+ plistLines := ck.newLines(lines)
+
+ ck.collectFilesAndDirs(plistLines)
+
+ t.CheckDeepEquals(keys(ck.allDirs),
+ []string{"bin", "man", "man/man1"})
+ t.CheckDeepEquals(keys(ck.allFiles),
+ []string{"bin/program", "man/man1/program.1"})
+}
+
+func (s *Suite) Test_PlistChecker_collectPath(c *check.C) {
+ t := s.Init(c)
+
+ line := t.NewLine("PLIST", 1, "a/b/c/program")
+ ck := NewPlistChecker(nil)
+
+ ck.collectPath("a/b/c/program", &PlistLine{line, nil, line.Text})
+
+ t.CheckDeepEquals(keys(ck.allDirs),
+ []string{"a", "a/b", "a/b/c"})
+ t.CheckDeepEquals(keys(ck.allFiles),
+ []string{"a/b/c/program"})
+}
+
+func (s *Suite) Test_PlistChecker_collectDirective(c *check.C) {
+ t := s.Init(c)
+
+ test := func(directive string, dirs ...string) {
+ line := t.NewLine("PLIST", 1, directive)
+ ck := NewPlistChecker(nil)
+
+ ck.collectDirective(&PlistLine{line, nil, line.Text})
+
+ t.CheckDeepEquals(keys(ck.allDirs), dirs)
+ t.Check(keys(ck.allFiles), check.HasLen, 0)
+ }
+
+ test("@exec ${MKDIR} %D/a/b/c",
+ "a", "a/b", "a/b/c")
+
+ test("@exec echo hello",
+ nil...)
+
+ test("@exec ${MKDIR} %D//absolute",
+ nil...)
+
+ test("@exec ${MKDIR} %D/a/../../../breakout",
+ "a", "a/..", "a/../..", "a/../../..", "a/../../../breakout")
+}
+
func (s *Suite) Test_PlistChecker_checkLine(c *check.C) {
t := s.Init(c)
@@ -430,7 +579,7 @@ func (s *Suite) Test_PlistChecker_checkL
"WARN: PLIST:4: \"bin/arm-linux-only\" should be sorted before \"bin/conditional-program\".",
"WARN: PLIST:10: PLISTs should not contain empty lines.",
"WARN: PLIST:11: PLISTs should not contain empty lines.",
- "WARN: PLIST:14: Invalid line type: <<<<<<<<< merge conflict")
+ "ERROR: PLIST:14: Invalid line type: <<<<<<<<< merge conflict")
}
func (s *Suite) Test_PlistChecker_checkPath__PKGMANDIR(c *check.C) {
@@ -446,7 +595,7 @@ func (s *Suite) Test_PlistChecker_checkP
"NOTE: PLIST:2: PLIST files should use \"man/\" instead of \"${PKGMANDIR}\".")
}
-func (s *Suite) Test_PlistChecker_checkPath__python_egg(c *check.C) {
+func (s *Suite) Test_PlistChecker_checkPathMisc__python_egg(c *check.C) {
t := s.Init(c)
lines := t.NewLines("PLIST",
@@ -459,21 +608,38 @@ func (s *Suite) Test_PlistChecker_checkP
"WARN: PLIST:2: Include \"../../lang/python/egg.mk\" instead of listing .egg-info files directly.")
}
-func (s *Suite) Test_PlistChecker_checkPath__unwanted_entries(c *check.C) {
+func (s *Suite) Test_PlistChecker_checkPathMisc__unwanted_entries(c *check.C) {
t := s.Init(c)
lines := t.SetUpFileLines("PLIST",
PlistCvsID,
"share/perllocal.pod",
"share/pkgbase/CVS/Entries",
- "share/pkgbase/Makefile.orig")
+ "share/pkgbase/Makefile.orig",
+ "../breakout",
+ "t/../../breakout",
+ "t/../../breakout/${VAR}",
+ "t/./non-canonical",
+ "t///non-canonical",
+ "t///non-canonical/${VAR}",
+ "t///non-canonical${VAR}",
+ "t/non-canonical/",
+ "t/ok/${VAR}")
CheckLinesPlist(nil, lines)
t.CheckOutputLines(
"WARN: ~/PLIST:2: The perllocal.pod file should not be in the PLIST.",
"WARN: ~/PLIST:3: CVS files should not be in the PLIST.",
- "WARN: ~/PLIST:4: .orig files should not be in the PLIST.")
+ "WARN: ~/PLIST:4: .orig files should not be in the PLIST.",
+ "ERROR: ~/PLIST:5: Invalid line type: ../breakout",
+ "ERROR: ~/PLIST:6: Paths in PLIST files must not contain \"..\".",
+ "ERROR: ~/PLIST:7: Paths in PLIST files must not contain \"..\".",
+ "ERROR: ~/PLIST:8: Paths in PLIST files must be canonical (t/non-canonical).",
+ "ERROR: ~/PLIST:9: Paths in PLIST files must be canonical (t/non-canonical).",
+ "ERROR: ~/PLIST:10: Paths in PLIST files must be canonical (t/non-canonical/${VAR}).",
+ "ERROR: ~/PLIST:11: Paths in PLIST files must be canonical (t/non-canonical${VAR}).",
+ "ERROR: ~/PLIST:12: Paths in PLIST files must be canonical (t/non-canonical).")
}
func (s *Suite) Test_PlistChecker_checkPathNonAscii(c *check.C) {
@@ -508,6 +674,11 @@ func (s *Suite) Test_PlistChecker_checkP
"sbin/iconv",
"sbin/\U0001F603", // Smiling face with open mouth
+
+ // Directives other than comments do not allow non-ASCII.
+ "unicode/00FC/reset",
+ "@exec true",
+ "unicode/00FC/\u00FC", // u-umlaut
)
CheckLinesPlist(nil, lines)
@@ -526,7 +697,65 @@ func (s *Suite) Test_PlistChecker_checkP
"\tcontain non-ASCII filenames.",
"",
"WARN: PLIST:5: Non-ASCII filename \"dir2/<U+0633><U+0644><U+0627><U+0645>\".",
- "WARN: PLIST:11: Non-ASCII filename \"sbin/<U+1F603>\".")
+ "WARN: PLIST:11: Non-ASCII filename \"sbin/<U+1F603>\".",
+ "WARN: PLIST:14: Non-ASCII filename \"unicode/00FC/<U+00FC>\".")
+}
+
+func (s *Suite) Test_PlistChecker_checkSorted(c *check.C) {
+ t := s.Init(c)
+
+ lines := t.NewLines("PLIST",
+ PlistCvsID,
+ "bin/program2",
+ "bin/program1")
+
+ CheckLinesPlist(nil, lines)
+
+ t.CheckOutputLines(
+ "WARN: PLIST:3: \"bin/program1\" should be " +
+ "sorted before \"bin/program2\".")
+}
+
+func (s *Suite) Test_PlistChecker_checkDuplicate(c *check.C) {
+ t := s.Init(c)
+
+ lines := t.NewLines("PLIST",
+ PlistCvsID,
+ "bin/program",
+ "bin/program")
+
+ CheckLinesPlist(nil, lines)
+
+ t.CheckOutputLines(
+ "ERROR: PLIST:3: Duplicate filename \"bin/program\", " +
+ "already appeared in line 2.")
+}
+
+func (s *Suite) Test_PlistChecker_checkPathBin(c *check.C) {
+ t := s.Init(c)
+
+ lines := t.NewLines("PLIST",
+ PlistCvsID,
+ "bin",
+ "bin/subdir/program")
+
+ CheckLinesPlist(nil, lines)
+
+ t.CheckOutputLines(
+ "WARN: PLIST:3: The bin/ directory should not have subdirectories.")
+}
+
+func (s *Suite) Test_PlistChecker_checkPathEtc(c *check.C) {
+ t := s.Init(c)
+
+ lines := t.NewLines("PLIST",
+ PlistCvsID,
+ "etc/config")
+
+ CheckLinesPlist(nil, lines)
+
+ t.CheckOutputLines(
+ "ERROR: PLIST:2: Configuration files must not be registered in the PLIST.")
}
func (s *Suite) Test_PlistChecker_checkPathInfo(c *check.C) {
@@ -775,6 +1004,38 @@ func (s *Suite) Test_PlistChecker_checkP
t.CheckOutputEmpty()
}
+func (s *Suite) Test_PlistLine_Path(c *check.C) {
+ t := s.Init(c)
+
+ t.CheckEquals(
+ (&PlistLine{text: "relative"}).Path(),
+ NewRelPathString("relative"))
+
+ t.ExpectAssert(
+ func() { (&PlistLine{text: "/absolute"}).Path() })
+}
+
+func (s *Suite) Test_PlistLine_HasPlainPath(c *check.C) {
+ t := s.Init(c)
+
+ test := func(text string, hasPlainPath bool) {
+ t.CheckEquals((&PlistLine{text: text}).HasPlainPath(), hasPlainPath)
+ }
+
+ test("abc", true)
+ test("9plan", true)
+ test("bin/program", true)
+
+ test("", false)
+ test("@", false)
+ test(":", false)
+ test("/absolute", false)
+ test("-rf", false)
+ test("\\", false)
+ test("bin/$<", false)
+ test("bin/${VAR}", false)
+}
+
func (s *Suite) Test_PlistLine_CheckTrailingWhitespace(c *check.C) {
t := s.Init(c)
@@ -880,7 +1141,7 @@ func (s *Suite) Test_plistLineSorter_Sor
"lib/after.la",
"@exec echo \"after lib/after.la\"")
ck := PlistChecker{nil, nil, nil, "", Once{}, false}
- plines := ck.NewLines(lines)
+ plines := ck.newLines(lines)
sorter1 := NewPlistLineSorter(plines)
t.CheckEquals(sorter1.unsortable, lines.Lines[5])
@@ -888,7 +1149,7 @@ func (s *Suite) Test_plistLineSorter_Sor
cleanedLines := append(append(lines.Lines[0:5], lines.Lines[6:8]...), lines.Lines[9:]...) // Remove ${UNKNOWN} and @exec
sorter2 := NewPlistLineSorter((&PlistChecker{nil, nil, nil, "", Once{}, false}).
- NewLines(NewLines(lines.Filename, cleanedLines)))
+ newLines(NewLines(lines.Filename, cleanedLines)))
c.Check(sorter2.unsortable, check.IsNil)
Index: pkgsrc/pkgtools/pkglint/files/vartype.go
diff -u pkgsrc/pkgtools/pkglint/files/vartype.go:1.41 pkgsrc/pkgtools/pkglint/files/vartype.go:1.42
--- pkgsrc/pkgtools/pkglint/files/vartype.go:1.41 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/vartype.go Sun Dec 8 22:03:38 2019
@@ -2,6 +2,7 @@ package pkglint
import (
"path"
+ "sort"
"strings"
)
@@ -45,7 +46,7 @@ const (
// This variable is provided by either the pkgsrc infrastructure in
// mk/*, or by <sys.mk>, which is included at the very beginning.
//
- // FIXME: Clearly distinguish between:
+ // TODO: Clearly distinguish between:
// * sys.mk
// * bsd.prefs.mk
// * bsd.pkg.mk
@@ -199,7 +200,7 @@ func (vt *Vartype) NeedsRationale() bool
func (vt *Vartype) IsOnePerLine() bool { return vt.options&OnePerLine != 0 }
func (vt *Vartype) IsAlwaysInScope() bool { return vt.options&AlwaysInScope != 0 }
func (vt *Vartype) IsDefinedIfInScope() bool { return vt.options&DefinedIfInScope != 0 }
-func (vt *Vartype) IsNonemptyIfInScope() bool { return vt.options&NonemptyIfDefined != 0 }
+func (vt *Vartype) IsNonemptyIfDefined() bool { return vt.options&NonemptyIfDefined != 0 }
func (vt *Vartype) EffectivePermissions(basename string) ACLPermissions {
for _, aclEntry := range vt.aclEntries {
@@ -464,6 +465,13 @@ var (
BtYesNo = &BasicType{"YesNo", (*VartypeCheck).YesNo}
BtYesNoIndirectly = &BasicType{"YesNoIndirectly", (*VartypeCheck).YesNoIndirectly}
+ BtMachineOpsys = enumFromValues(machineOpsysValues)
+ BtMachineArch = enumFromValues(machineArchValues)
+ BtMachineGnuArch = enumFromValues(machineGnuArchValues)
+ BtEmulOpsys = enumFromValues(emulOpsysValues)
+ BtEmulArch = enumFromValues(machineArchValues) // Just a wild guess.
+ BtMachineGnuPlatformOpsys = BtEmulOpsys
+
btCond = &BasicType{".if condition", nil /* never called */}
btForLoop = &BasicType{".for loop", nil /* never called */}
)
@@ -477,3 +485,51 @@ func init() {
BtShellCommands.checker = (*VartypeCheck).ShellCommands
BtShellWord.checker = (*VartypeCheck).ShellWord
}
+
+// TODO: Move these values to VarTypeRegistry.Init and read them from the
+// pkgsrc infrastructure files, as far as possible.
+const (
+ machineOpsysValues = "" + // See mk/platform
+ "AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD " +
+ "HPUX Haiku IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare"
+
+ // See mk/emulator/emulator-vars.mk.
+ emulOpsysValues = "" +
+ "bitrig bsdos cygwin darwin dragonfly freebsd " +
+ "haiku hpux interix irix linux mirbsd netbsd openbsd osf1 solaris sunos"
+
+ // Hardware architectures having the same name in bsd.own.mk and the GNU world.
+ // These are best-effort guesses, since they depend on the operating system.
+ archValues = "" +
+ "aarch64 alpha amd64 arc arm cobalt convex dreamcast i386 " +
+ "hpcmips hpcsh hppa hppa64 ia64 " +
+ "m68k m88k mips mips64 mips64el mipseb mipsel mipsn32 mlrisc " +
+ "ns32k pc532 pmax powerpc powerpc64 rs6000 s390 sparc sparc64 vax x86_64"
+
+ // See mk/bsd.prefs.mk:/^GNU_ARCH\./
+ machineArchValues = "" +
+ archValues + " " +
+ "aarch64eb amd64 arm26 arm32 coldfire earm earmeb earmhf earmhfeb earmv4 earmv4eb earmv5 " +
+ "earmv5eb earmv6 earmv6eb earmv6hf earmv6hfeb earmv7 earmv7eb earmv7hf earmv7hfeb evbarm " +
+ "i386 i586 i686 m68000 mips mips64eb sh3eb sh3el"
+
+ // See mk/bsd.prefs.mk:/^GNU_ARCH\./
+ machineGnuArchValues = "" +
+ archValues + " " +
+ "aarch64_be arm armeb armv4 armv4eb armv6 armv6eb armv7 armv7eb " +
+ "i486 m5407 m68010 mips64 mipsel sh shle x86_64"
+)
+
+func enumFromValues(spaceSeparated string) *BasicType {
+ values := strings.Fields(spaceSeparated)
+ sort.Strings(values)
+ seen := make(map[string]bool)
+ var unique []string
+ for _, value := range values {
+ if !seen[value] {
+ seen[value] = true
+ unique = append(unique, value)
+ }
+ }
+ return enum(strings.Join(unique, " "))
+}
Index: pkgsrc/pkgtools/pkglint/files/redundantscope.go
diff -u pkgsrc/pkgtools/pkglint/files/redundantscope.go:1.10 pkgsrc/pkgtools/pkglint/files/redundantscope.go:1.11
--- pkgsrc/pkgtools/pkglint/files/redundantscope.go:1.10 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/redundantscope.go Sun Dec 8 22:03:38 2019
@@ -97,7 +97,7 @@ func (s *RedundantScope) handleVarassign
effOp = opAssign
}
- // FIXME: Skip the whole redundancy check if the value is not known to be constant.
+ // TODO: Skip the whole redundancy check if the value is not known to be constant.
if effOp == opAssign && info.vari.Value() == value {
effOp = opAssignDefault
}
Index: pkgsrc/pkgtools/pkglint/files/shtokenizer_test.go
diff -u pkgsrc/pkgtools/pkglint/files/shtokenizer_test.go:1.22 pkgsrc/pkgtools/pkglint/files/shtokenizer_test.go:1.23
--- pkgsrc/pkgtools/pkglint/files/shtokenizer_test.go:1.22 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/shtokenizer_test.go Sun Dec 8 22:03:38 2019
@@ -41,7 +41,7 @@ func (s *Suite) Test_ShTokenizer__exampl
"WARN: filename.mk:2: Pkglint ShellLine.CheckShellCommand: splitIntoShellTokens couldn't parse \"\\\"`'`y\"")
// Covers shAtomDquotBackt: return nil
- // FIXME: Pkglint must parse unescaped dollar in the same way, everywhere.
+ // XXX: Pkglint should parse unescaped dollar in the same way, everywhere.
test(
"\"`$|",
"WARN: filename.mk:2: Internal pkglint error in ShTokenizer.ShAtom at \"$|\" (quoting=db).",
@@ -49,7 +49,7 @@ func (s *Suite) Test_ShTokenizer__exampl
"WARN: filename.mk:2: Internal pkglint error in MkLine.Tokenize at \"$|\".")
// Covers shAtomDquotBacktDquot: return nil
- // FIXME: Pkglint must support unlimited nesting.
+ // XXX: Pkglint should support unlimited nesting.
test(
"\"`\"`",
"WARN: filename.mk:2: Internal pkglint error in ShTokenizer.ShAtom at \"`\" (quoting=dbd).",
Index: pkgsrc/pkgtools/pkglint/files/substcontext.go
diff -u pkgsrc/pkgtools/pkglint/files/substcontext.go:1.31 pkgsrc/pkgtools/pkglint/files/substcontext.go:1.32
--- pkgsrc/pkgtools/pkglint/files/substcontext.go:1.31 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/substcontext.go Sun Dec 8 22:03:38 2019
@@ -142,7 +142,7 @@ func (ctx *SubstContext) Varassign(mklin
fix.Replace("pre-patch", "post-extract")
fix.Replace("post-patch", "pre-configure")
fix.Apply()
- // FIXME: Add test that has "SUBST_STAGE.id=pre-patch # or rather post-patch?"
+ // XXX: Add test that has "SUBST_STAGE.id=pre-patch # or rather post-patch?"
}
if G.Pkg != nil && (value == "pre-configure" || value == "post-configure") {
Index: pkgsrc/pkgtools/pkglint/files/util_test.go
diff -u pkgsrc/pkgtools/pkglint/files/util_test.go:1.40 pkgsrc/pkgtools/pkglint/files/util_test.go:1.41
--- pkgsrc/pkgtools/pkglint/files/util_test.go:1.40 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/util_test.go Sun Dec 8 22:03:38 2019
@@ -436,24 +436,24 @@ func (s *Suite) Test_containsVarRef(c *c
test("$", false) // A syntax error.
// See the bmake manual page.
- test("$>", false) // FIXME: true; .ALLSRC
- test("$!", false) // FIXME: true; .ARCHIVE
- test("$<", false) // FIXME: true; .IMPSRC
- test("$%", false) // FIXME: true; .MEMBER
- test("$?", false) // FIXME: true; .OODATE
- test("$*", false) // FIXME: true; .PREFIX
- test("$@", false) // FIXME: true; .TARGET
+ test("$>", true) // .ALLSRC
+ test("$!", true) // .ARCHIVE
+ test("$<", true) // .IMPSRC
+ test("$%", true) // .MEMBER
+ test("$?", true) // .OODATE
+ test("$*", true) // .PREFIX
+ test("$@", true) // .TARGET
- test("$V", false) // FIXME: true
- test("$v", false) // FIXME: true
+ test("$V", true)
+ test("$v", true)
test("${Var}", true)
test("${VAR.${param}}", true)
test("$(VAR)", true)
- test("$$", false) // An escaped dollar character.
- test("$$(VAR)", true) // FIXME: false; An escaped dollar character; probably a subshell.
- test("$${VAR}", true) // FIXME: false; An escaped dollar character; probably a shell variable.
- test("$$VAR", false) // An escaped dollar character.
+ test("$$", false) // An escaped dollar character.
+ test("$$(VAR)", false) // An escaped dollar character; probably a subshell.
+ test("$${VAR}", false) // An escaped dollar character; probably a shell variable.
+ test("$$VAR", false) // An escaped dollar character.
}
func (s *Suite) Test_hasAlnumPrefix(c *check.C) {
@@ -645,7 +645,8 @@ func (s *Suite) Test_Scope_LastValue(c *
mklines.Check()
- t.CheckEquals(mklines.vars.LastValue("VAR"), "third (conditional)")
+ // TODO: At load time, use loadVars instead of allVars.
+ t.CheckEquals(mklines.allVars.LastValue("VAR"), "third (conditional)")
t.CheckOutputLines(
"WARN: file.mk:2: VAR is defined but not used.")
Index: pkgsrc/pkgtools/pkglint/files/vardefs.go
diff -u pkgsrc/pkgtools/pkglint/files/vardefs.go:1.81 pkgsrc/pkgtools/pkglint/files/vardefs.go:1.82
--- pkgsrc/pkgtools/pkglint/files/vardefs.go:1.81 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/vardefs.go Sun Dec 8 22:03:38 2019
@@ -1282,13 +1282,13 @@ func (reg *VarTypeRegistry) Init(src *Pk
reg.pkglist("LTCONFIG_OVERRIDE", BtPathPattern)
// See devel/bmake/files/main.c:/Var_Set."MACHINE_ARCH"/.
- reg.sysload("MACHINE_ARCH", enumMachineArch, AlwaysInScope|DefinedIfInScope|NonemptyIfDefined)
+ reg.sysload("MACHINE_ARCH", BtMachineArch, AlwaysInScope|DefinedIfInScope|NonemptyIfDefined)
// From mk/endian.mk, determined by a shell program that compiles
// a C program. That's just too much for pkglint to analyze.
reg.sysload("MACHINE_ENDIAN", enum("big little unknown"), DefinedIfInScope|NonemptyIfDefined)
- reg.sysload("MACHINE_GNU_ARCH", enumMachineGnuArch, DefinedIfInScope|NonemptyIfDefined)
+ reg.sysload("MACHINE_GNU_ARCH", BtMachineGnuArch, DefinedIfInScope|NonemptyIfDefined)
reg.sysload("MACHINE_GNU_PLATFORM", BtMachineGnuPlatform, DefinedIfInScope|NonemptyIfDefined)
reg.sysload("MACHINE_PLATFORM", BtMachinePlatform, DefinedIfInScope|NonemptyIfDefined)
reg.pkg("MAINTAINER", BtMailAddress)
Index: pkgsrc/pkgtools/pkglint/files/vartypecheck.go
diff -u pkgsrc/pkgtools/pkglint/files/vartypecheck.go:1.70 pkgsrc/pkgtools/pkglint/files/vartypecheck.go:1.71
--- pkgsrc/pkgtools/pkglint/files/vartypecheck.go:1.70 Sun Dec 8 00:06:38 2019
+++ pkgsrc/pkgtools/pkglint/files/vartypecheck.go Sun Dec 8 22:03:38 2019
@@ -4,7 +4,6 @@ import (
"netbsd.org/pkglint/regex"
"netbsd.org/pkglint/textproc"
"path"
- "sort"
"strings"
)
@@ -81,7 +80,7 @@ func (cv *VartypeCheck) WithVarnameValue
// and the value.
//
// This is typically used when checking parts of composite types,
-// especially patterns.
+// such as the patterns from ONLY_FOR_PLATFORM.
func (cv *VartypeCheck) WithVarnameValueMatch(varname, value string) *VartypeCheck {
newVc := *cv
newVc.Varname = varname
@@ -91,61 +90,6 @@ func (cv *VartypeCheck) WithVarnameValue
return &newVc
}
-const (
- machineOpsysValues = "" + // See mk/platform
- "AIX BSDOS Bitrig Cygwin Darwin DragonFly FreeBSD FreeMiNT GNUkFreeBSD " +
- "HPUX Haiku IRIX Interix Linux Minix MirBSD NetBSD OSF1 OpenBSD QNX SCO_SV SunOS UnixWare"
-
- // See mk/emulator/emulator-vars.mk.
- emulOpsysValues = "" +
- "bitrig bsdos cygwin darwin dragonfly freebsd " +
- "haiku hpux interix irix linux mirbsd netbsd openbsd osf1 solaris sunos"
-
- // Hardware architectures having the same name in bsd.own.mk and the GNU world.
- // These are best-effort guesses, since they depend on the operating system.
- archValues = "" +
- "aarch64 alpha amd64 arc arm cobalt convex dreamcast i386 " +
- "hpcmips hpcsh hppa hppa64 ia64 " +
- "m68k m88k mips mips64 mips64el mipseb mipsel mipsn32 mlrisc " +
- "ns32k pc532 pmax powerpc powerpc64 rs6000 s390 sparc sparc64 vax x86_64"
-
- // See mk/bsd.prefs.mk:/^GNU_ARCH\./
- machineArchValues = "" +
- archValues + " " +
- "aarch64eb amd64 arm26 arm32 coldfire earm earmeb earmhf earmhfeb earmv4 earmv4eb earmv5 " +
- "earmv5eb earmv6 earmv6eb earmv6hf earmv6hfeb earmv7 earmv7eb earmv7hf earmv7hfeb evbarm " +
- "i386 i586 i686 m68000 mips mips64eb sh3eb sh3el"
-
- // See mk/bsd.prefs.mk:/^GNU_ARCH\./
- machineGnuArchValues = "" +
- archValues + " " +
- "aarch64_be arm armeb armv4 armv4eb armv6 armv6eb armv7 armv7eb " +
- "i486 m5407 m68010 mips64 mipsel sh shle x86_64"
-)
-
-func enumFromValues(spaceSeparated string) *BasicType {
- values := strings.Fields(spaceSeparated)
- sort.Strings(values)
- seen := make(map[string]bool)
- var unique []string
- for _, value := range values {
- if !seen[value] {
- seen[value] = true
- unique = append(unique, value)
- }
- }
- return enum(strings.Join(unique, " "))
-}
-
-var (
- enumMachineOpsys = enumFromValues(machineOpsysValues)
- enumMachineArch = enumFromValues(machineArchValues)
- enumMachineGnuArch = enumFromValues(machineGnuArchValues)
- enumEmulOpsys = enumFromValues(emulOpsysValues)
- enumEmulArch = enumFromValues(machineArchValues) // Just a wild guess.
- enumMachineGnuPlatformOpsys = enumEmulOpsys
-)
-
func (cv *VartypeCheck) AwkCommand() {
if trace.Tracing {
trace.Step1("Unchecked AWK command: %q", cv.Value)
@@ -497,10 +441,10 @@ func (cv *VartypeCheck) EmulPlatform() {
const rePair = `^(` + rePart + `)-(` + rePart + `)$`
if m, opsysPattern, archPattern := match2(cv.Value, rePair); m {
opsysCv := cv.WithVarnameValue("the operating system part of "+cv.Varname, opsysPattern)
- enumEmulOpsys.checker(opsysCv)
+ BtEmulOpsys.checker(opsysCv)
archCv := cv.WithVarnameValue("the hardware architecture part of "+cv.Varname, archPattern)
- enumEmulArch.checker(archCv)
+ BtEmulArch.checker(archCv)
} else {
cv.Warnf("%q is not a valid emulation platform.", cv.Value)
cv.Explain(
@@ -806,14 +750,14 @@ func (cv *VartypeCheck) MachineGnuPlatfo
archCv := cv.WithVarnameValueMatch(
"the hardware architecture part of "+cv.Varname,
archPattern)
- enumMachineGnuArch.checker(archCv)
+ BtMachineGnuArch.checker(archCv)
_ = vendorPattern
opsysCv := cv.WithVarnameValueMatch(
"the operating system part of "+cv.Varname,
opsysPattern)
- enumMachineGnuPlatformOpsys.checker(opsysCv)
+ BtMachineGnuPlatformOpsys.checker(opsysCv)
} else {
cv.Warnf("%q is not a valid platform pattern.", cv.Value)
@@ -848,13 +792,13 @@ func (cv *VartypeCheck) MachinePlatformP
if m, opsysPattern, versionPattern, archPattern := match3(pattern, reTriple); m {
opsysCv := cv.WithVarnameValueMatch("the operating system part of "+cv.Varname, opsysPattern)
- enumMachineOpsys.checker(opsysCv)
+ BtMachineOpsys.checker(opsysCv)
versionCv := cv.WithVarnameValueMatch("the version part of "+cv.Varname, versionPattern)
versionCv.Version()
archCv := cv.WithVarnameValueMatch("the hardware architecture part of "+cv.Varname, archPattern)
- enumMachineArch.checker(archCv)
+ BtMachineArch.checker(archCv)
} else {
cv.Warnf("%q is not a valid platform pattern.", cv.Value)
@@ -1099,7 +1043,7 @@ func (cv *VartypeCheck) Pkgpath() {
func (cv *VartypeCheck) Pkgrevision() {
if !matches(cv.Value, `^[1-9]\d*$`) {
- cv.Warnf("%s must be a positive integer number.", cv.Varname)
+ cv.Errorf("%s must be a positive integer number.", cv.Varname)
}
if cv.MkLine.Basename != "Makefile" {
cv.Errorf("%s only makes sense directly in the package Makefile.", cv.Varname)
@@ -1253,7 +1197,7 @@ func (cv *VartypeCheck) SedCommands() {
i++
ncommands++
if ncommands > 1 {
- cv.Notef("Each sed command should appear in an assignment of its own.")
+ cv.Warnf("Each sed command should appear in an assignment of its own.")
cv.Explain(
"For example, instead of",
" SUBST_SED.foo+= -e s,command1,, -e s,command2,,",
Home |
Main Index |
Thread Index |
Old Index