pkgsrc-Changes archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
CVS commit: pkgsrc/pkgtools/pkglint
Module Name: pkgsrc
Committed By: rillig
Date: Mon May 6 20:27:17 UTC 2019
Modified Files:
pkgsrc/pkgtools/pkglint: Makefile
pkgsrc/pkgtools/pkglint/files: buildlink3.go buildlink3_test.go
check_test.go distinfo.go distinfo_test.go licenses.go
mklinechecker.go mklinechecker_test.go mklines_test.go package.go
package_test.go pkglint.go pkglint_test.go pkgsrc.go
redundantscope_test.go shell.go shell_test.go toplevel.go util.go
vardefs.go
Log Message:
pkgtools/pkglint: update to 5.7.9
Changes since 5.7.8:
* Buildlink3.mk files are checked for typos in the identifier that is
used for BUILDLINK_TREE, to detect copy-and-paste mistakes.
* Having a rationale is recommended for some variables, especially those
that make a package fail to build or crash at runtime. This check is
only active when -Wextra is given, since it is still actively debated
whether such a check is actually useful.
* Files called Makefile.php can easily be mistaken to be PHP files.
Therefore the recommended naming convention is to have auxiliary files
called *.mk. There are already many more files called *.mk than those
being called Makefile.*.
* The check for unquoted sed substitution commands has been made more
detailed, but since it is completely disabled, there's nothing to see
for now.
* The definitions for MASTER_SITE_* are loaded directly from the pkgsrc
infrastructure instead of hard-coding them in pkglint.
To generate a diff of this commit:
cvs rdiff -u -r1.578 -r1.579 pkgsrc/pkgtools/pkglint/Makefile
cvs rdiff -u -r1.21 -r1.22 pkgsrc/pkgtools/pkglint/files/buildlink3.go
cvs rdiff -u -r1.30 -r1.31 pkgsrc/pkgtools/pkglint/files/buildlink3_test.go
cvs rdiff -u -r1.40 -r1.41 pkgsrc/pkgtools/pkglint/files/check_test.go \
pkgsrc/pkgtools/pkglint/files/pkglint_test.go
cvs rdiff -u -r1.32 -r1.33 pkgsrc/pkgtools/pkglint/files/distinfo.go
cvs rdiff -u -r1.29 -r1.30 pkgsrc/pkgtools/pkglint/files/distinfo_test.go
cvs rdiff -u -r1.23 -r1.24 pkgsrc/pkgtools/pkglint/files/licenses.go
cvs rdiff -u -r1.37 -r1.38 pkgsrc/pkgtools/pkglint/files/mklinechecker.go
cvs rdiff -u -r1.33 -r1.34 \
pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go
cvs rdiff -u -r1.42 -r1.43 pkgsrc/pkgtools/pkglint/files/mklines_test.go
cvs rdiff -u -r1.52 -r1.53 pkgsrc/pkgtools/pkglint/files/package.go
cvs rdiff -u -r1.45 -r1.46 pkgsrc/pkgtools/pkglint/files/package_test.go \
pkgsrc/pkgtools/pkglint/files/shell_test.go
cvs rdiff -u -r1.53 -r1.54 pkgsrc/pkgtools/pkglint/files/pkglint.go
cvs rdiff -u -r1.25 -r1.26 pkgsrc/pkgtools/pkglint/files/pkgsrc.go
cvs rdiff -u -r1.3 -r1.4 pkgsrc/pkgtools/pkglint/files/redundantscope_test.go
cvs rdiff -u -r1.39 -r1.40 pkgsrc/pkgtools/pkglint/files/shell.go
cvs rdiff -u -r1.17 -r1.18 pkgsrc/pkgtools/pkglint/files/toplevel.go
cvs rdiff -u -r1.43 -r1.44 pkgsrc/pkgtools/pkglint/files/util.go
cvs rdiff -u -r1.63 -r1.64 pkgsrc/pkgtools/pkglint/files/vardefs.go
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: pkgsrc/pkgtools/pkglint/Makefile
diff -u pkgsrc/pkgtools/pkglint/Makefile:1.578 pkgsrc/pkgtools/pkglint/Makefile:1.579
--- pkgsrc/pkgtools/pkglint/Makefile:1.578 Sun Apr 28 18:13:53 2019
+++ pkgsrc/pkgtools/pkglint/Makefile Mon May 6 20:27:17 2019
@@ -1,6 +1,6 @@
-# $NetBSD: Makefile,v 1.578 2019/04/28 18:13:53 rillig Exp $
+# $NetBSD: Makefile,v 1.579 2019/05/06 20:27:17 rillig Exp $
-PKGNAME= pkglint-5.7.8
+PKGNAME= pkglint-5.7.9
CATEGORIES= pkgtools
DISTNAME= tools
MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/}
Index: pkgsrc/pkgtools/pkglint/files/buildlink3.go
diff -u pkgsrc/pkgtools/pkglint/files/buildlink3.go:1.21 pkgsrc/pkgtools/pkglint/files/buildlink3.go:1.22
--- pkgsrc/pkgtools/pkglint/files/buildlink3.go:1.21 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/buildlink3.go Mon May 6 20:27:17 2019
@@ -2,6 +2,7 @@ package pkglint
import (
"netbsd.org/pkglint/pkgver"
+ "path"
"strings"
)
@@ -84,12 +85,36 @@ func (ck *Buildlink3Checker) checkFirstP
if containsVarRef(pkgbase) {
ck.checkVaruseInPkgbase(pkgbase, pkgbaseLine)
}
+
+ ck.checkUniquePkgbase(pkgbase, pkgbaseLine)
+
mlex.SkipEmptyOrNote()
ck.pkgbase = pkgbase
ck.pkgbaseLine = pkgbaseLine
return true
}
+func (ck *Buildlink3Checker) checkUniquePkgbase(pkgbase string, mkline MkLine) {
+ prev := G.InterPackage.Bl3(pkgbase, &mkline.Location)
+ if prev == nil {
+ return
+ }
+
+ base, name := trimCommon(pkgbase, path.Base(path.Dir(mkline.Filename)))
+ if base == "" && matches(name, `^(\d*|-cvs|-fossil|-git|-hg|-svn|-devel|-snapshot)$`) {
+ return
+ }
+
+ mkline.Errorf("Duplicate package identifier %q already appeared in %s.",
+ pkgbase, mkline.RefToLocation(*prev))
+ mkline.Explain(
+ "Each buildlink3.mk file must have a unique identifier.",
+ "These identifiers are used for multiple-inclusion guards,",
+ "and using the same identifier for different packages",
+ "(often by copy-and-paste) may change the dependencies",
+ "of a package in subtle and unexpected ways.")
+}
+
// checkSecondParagraph checks the multiple inclusion protection and
// introduces the uppercase package identifier.
func (ck *Buildlink3Checker) checkSecondParagraph(mlex *MkLinesLexer) bool {
Index: pkgsrc/pkgtools/pkglint/files/buildlink3_test.go
diff -u pkgsrc/pkgtools/pkglint/files/buildlink3_test.go:1.30 pkgsrc/pkgtools/pkglint/files/buildlink3_test.go:1.31
--- pkgsrc/pkgtools/pkglint/files/buildlink3_test.go:1.30 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/buildlink3_test.go Mon May 6 20:27:17 2019
@@ -520,6 +520,59 @@ func (s *Suite) Test_CheckLinesBuildlink
"(also in other variables in this file).")
}
+func (s *Suite) Test_Buildlink3Checker_checkUniquePkgbase(c *check.C) {
+ t := s.Init(c)
+
+ G.InterPackage.Enable()
+
+ test := func(pkgbase, pkgpath string, diagnostics ...string) {
+ mkline := t.NewMkLine(t.File(pkgpath+"/buildlink3.mk"), 123, "")
+
+ (*Buildlink3Checker).checkUniquePkgbase(nil, pkgbase, mkline)
+
+ t.CheckOutput(diagnostics)
+ }
+
+ // From now on, the pkgbase "php" may only be used for "php\d+".
+ test("php", "lang/php56",
+ nil...)
+
+ // No warning since "php" is a valid buildlink3 basename for "php56".
+ test("php", "lang/php72",
+ nil...)
+
+ // But this is a clear typo.
+ test("php", "security/pgp",
+ "ERROR: ~/security/pgp/buildlink3.mk:123: "+
+ "Duplicate package identifier \"php\" already appeared "+
+ "in ../../lang/php56/buildlink3.mk:123.")
+
+ // This combination is not allowed because the names "php" and "php-pcre"
+ // differ too much. The only allowed inexact match is that the pkgname
+ // has one more number than the pkgbase, no matter at which position.
+ test("php", "textproc/php-pcre",
+ "ERROR: ~/textproc/php-pcre/buildlink3.mk:123: "+
+ "Duplicate package identifier \"php\" already appeared "+
+ "in ../../lang/php56/buildlink3.mk:123.")
+
+ test("ruby-module", "net/ruby24-module",
+ nil...)
+
+ test("ruby-module", "net/ruby26-module",
+ nil...)
+
+ test("ruby-module", "net/ruby26-module12",
+ "ERROR: ~/net/ruby26-module12/buildlink3.mk:123: "+
+ "Duplicate package identifier \"ruby-module\" already appeared "+
+ "in ../../net/ruby24-module/buildlink3.mk:123.")
+
+ test("package", "devel/package",
+ nil...)
+
+ test("package", "wip/package",
+ nil...)
+}
+
func (s *Suite) Test_Buildlink3Checker_checkMainPart__if_else_endif(c *check.C) {
t := s.Init(c)
Index: pkgsrc/pkgtools/pkglint/files/check_test.go
diff -u pkgsrc/pkgtools/pkglint/files/check_test.go:1.40 pkgsrc/pkgtools/pkglint/files/check_test.go:1.41
--- pkgsrc/pkgtools/pkglint/files/check_test.go:1.40 Sat Apr 27 19:33:57 2019
+++ pkgsrc/pkgtools/pkglint/files/check_test.go Mon May 6 20:27:17 2019
@@ -177,6 +177,13 @@ func (t *Tester) SetUpVartypes() {
}
func (t *Tester) SetUpMasterSite(varname string, urls ...string) {
+ if !G.Pkgsrc.vartypes.DefinedExact(varname) {
+ G.Pkgsrc.vartypes.DefineParse(varname, BtFetchURL,
+ List|SystemProvided,
+ "buildlink3.mk: none",
+ "*: use")
+ }
+
for _, url := range urls {
G.Pkgsrc.registerMasterSite(varname, url)
}
@@ -272,6 +279,8 @@ func (t *Tester) SetUpPkgsrc() {
// The various MASTER_SITE_* variables for use in the
// MASTER_SITES are defined in this file.
//
+ // To define a MASTER_SITE for a pkglint test, call t.SetUpMasterSite.
+ //
// See Pkgsrc.loadMasterSites.
t.CreateFileLines("mk/fetch/sites.mk",
MkRcsID)
@@ -497,6 +506,14 @@ func (t *Tester) File(relativeFileName s
return path.Clean(t.tmpdir + "/" + relativeFileName)
}
+// Copy copies a file inside the temporary directory.
+func (t *Tester) Copy(relativeSrc, relativeDst string) {
+ data, err := ioutil.ReadFile(t.File(relativeSrc))
+ G.AssertNil(err, "Copy.Read")
+ err = ioutil.WriteFile(t.File(relativeDst), data, 0777)
+ G.AssertNil(err, "Copy.Write")
+}
+
// Chdir changes the current working directory to the given subdirectory
// of the temporary directory, creating it if necessary.
//
@@ -552,7 +569,7 @@ func (t *Tester) Remove(relativeFileName
// "VAR= env")))
//
// mklines := get("including.mk")
-// module := get("module.mk")
+// module := get("subdir/module.mk")
//
// The filenames passed to the include function are all relative to the
// same location, but that location is irrelevant in practice. The generated
Index: pkgsrc/pkgtools/pkglint/files/pkglint_test.go
diff -u pkgsrc/pkgtools/pkglint/files/pkglint_test.go:1.40 pkgsrc/pkgtools/pkglint/files/pkglint_test.go:1.41
--- pkgsrc/pkgtools/pkglint/files/pkglint_test.go:1.40 Sat Apr 27 19:33:57 2019
+++ pkgsrc/pkgtools/pkglint/files/pkglint_test.go Mon May 6 20:27:17 2019
@@ -1187,3 +1187,24 @@ func (s *Suite) Test_Main(c *check.C) {
"Looks fine.")
// outProfiling is not checked because it contains timing information.
}
+
+func (s *Suite) Test_InterPackage_Bl3__same_identifier(c *check.C) {
+ t := s.Init(c)
+
+ t.SetUpPackage("category/package1",
+ "PKGNAME=\t${DISTNAME:@v@${v}@}") // Make the package name non-obvious.
+ t.SetUpPackage("category/package2",
+ "PKGNAME=\t${DISTNAME:@v@${v}@}") // Make the package name non-obvious.
+ t.CreateFileDummyBuildlink3("category/package1/buildlink3.mk")
+ t.Copy("category/package1/buildlink3.mk", "category/package2/buildlink3.mk")
+ t.Chdir(".")
+ t.FinishSetUp()
+
+ G.InterPackage.Enable()
+ G.Check("category/package1")
+ G.Check("category/package2")
+
+ t.CheckOutputLines(
+ "ERROR: category/package2/buildlink3.mk:3: Duplicate package identifier " +
+ "\"package1\" already appeared in ../../category/package1/buildlink3.mk:3.")
+}
Index: pkgsrc/pkgtools/pkglint/files/distinfo.go
diff -u pkgsrc/pkgtools/pkglint/files/distinfo.go:1.32 pkgsrc/pkgtools/pkglint/files/distinfo.go:1.33
--- pkgsrc/pkgtools/pkglint/files/distinfo.go:1.32 Tue Apr 23 21:20:49 2019
+++ pkgsrc/pkgtools/pkglint/files/distinfo.go Mon May 6 20:27:17 2019
@@ -323,8 +323,8 @@ func (ck *distinfoLinesChecker) checkUnr
// Inter-package check for differing distfile checksums.
func (ck *distinfoLinesChecker) checkGlobalDistfileMismatch(info distinfoHash) {
- hashes := G.Hashes
- if hashes == nil {
+
+ if !G.InterPackage.Enabled() {
return
}
@@ -348,23 +348,20 @@ func (ck *distinfoLinesChecker) checkGlo
return
}
- key := alg + ":" + filename
- otherHash := hashes[key]
-
// See https://github.com/golang/go/issues/29802
hashBytes := make([]byte, hex.DecodedLen(len(hash)))
_, err := hex.Decode(hashBytes, []byte(hash))
if err != nil {
line.Errorf("The %s hash for %s contains a non-hex character.", alg, filename)
+ return
}
+ otherHash := G.InterPackage.Hash(alg, filename, hashBytes, &line.Location)
if otherHash != nil {
if !bytes.Equal(otherHash.hash, hashBytes) {
line.Errorf("The %s hash for %s is %s, which conflicts with %s in %s.",
alg, filename, hash, hex.EncodeToString(otherHash.hash), line.RefToLocation(otherHash.location))
}
- } else {
- hashes[key] = &Hash{hashBytes, line.Location}
}
}
Index: pkgsrc/pkgtools/pkglint/files/distinfo_test.go
diff -u pkgsrc/pkgtools/pkglint/files/distinfo_test.go:1.29 pkgsrc/pkgtools/pkglint/files/distinfo_test.go:1.30
--- pkgsrc/pkgtools/pkglint/files/distinfo_test.go:1.29 Tue Apr 23 21:20:49 2019
+++ pkgsrc/pkgtools/pkglint/files/distinfo_test.go Mon May 6 20:27:17 2019
@@ -204,8 +204,8 @@ func (s *Suite) Test_distinfoLinesChecke
"(Run \"pkglint -e\" to show explanations.)")
// Ensure that hex.DecodeString does not waste memory here.
- t.Check(len(G.Hashes["SHA512:distfile-1.0.tar.gz"].hash), equals, 8)
- t.Check(cap(G.Hashes["SHA512:distfile-1.0.tar.gz"].hash), equals, 8)
+ t.Check(len(G.InterPackage.hashes["SHA512:distfile-1.0.tar.gz"].hash), equals, 8)
+ t.Check(cap(G.InterPackage.hashes["SHA512:distfile-1.0.tar.gz"].hash), equals, 8)
}
func (s *Suite) Test_distinfoLinesChecker_checkAlgorithms__missing_patch_with_distfile_checksums(c *check.C) {
Index: pkgsrc/pkgtools/pkglint/files/licenses.go
diff -u pkgsrc/pkgtools/pkglint/files/licenses.go:1.23 pkgsrc/pkgtools/pkglint/files/licenses.go:1.24
--- pkgsrc/pkgtools/pkglint/files/licenses.go:1.23 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/licenses.go Mon May 6 20:27:17 2019
@@ -32,9 +32,7 @@ func (lc *LicenseChecker) checkName(lice
}
if licenseFile == "" {
licenseFile = G.Pkgsrc.File("licenses/" + license)
- if G.UsedLicenses != nil {
- G.UsedLicenses[intern(license)] = true
- }
+ G.InterPackage.UseLicense(license)
}
if !fileExists(licenseFile) {
Index: pkgsrc/pkgtools/pkglint/files/mklinechecker.go
diff -u pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.37 pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.38
--- pkgsrc/pkgtools/pkglint/files/mklinechecker.go:1.37 Sun Apr 28 18:13:53 2019
+++ pkgsrc/pkgtools/pkglint/files/mklinechecker.go Mon May 6 20:27:17 2019
@@ -403,6 +403,9 @@ func (ck MkLineChecker) explainPermissio
}
func (ck MkLineChecker) checkVarassignLeftRationale() {
+ if !G.Opts.WarnExtra {
+ return
+ }
isRationale := func(mkline MkLine) bool {
if mkline.IsVarassign() || mkline.IsCommentedVarassign() {
Index: pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go:1.33 pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go:1.34
--- pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go:1.33 Sun Apr 28 18:13:53 2019
+++ pkgsrc/pkgtools/pkglint/files/mklinechecker_test.go Mon May 6 20:27:17 2019
@@ -766,6 +766,13 @@ func (s *Suite) Test_MkLineChecker_check
"WARN: filename.mk:2: Setting variable ONLY_FOR_PLATFORM should have a rationale.",
"WARN: filename.mk:3: Setting variable NOT_FOR_PLATFORM should have a rationale.",
"WARN: filename.mk:11: Setting variable ONLY_FOR_PLATFORM should have a rationale.")
+
+ // This check is only enabled when -Wextra is given.
+ t.SetUpCommandLine()
+
+ mklines.Check()
+
+ t.CheckOutputEmpty()
}
func (s *Suite) Test_MkLineChecker_checkVarassignOpShell(c *check.C) {
@@ -2150,6 +2157,7 @@ func (s *Suite) Test_MkLineChecker_check
t := s.Init(c)
t.SetUpPkgsrc()
+ t.SetUpMasterSite("MASTER_SITE_GITHUB", "https://download.github.com/";)
t.SetUpCommandLine("-Wall,no-space")
mklines := t.SetUpFileMkLines("module.mk",
MkRcsID,
Index: pkgsrc/pkgtools/pkglint/files/mklines_test.go
diff -u pkgsrc/pkgtools/pkglint/files/mklines_test.go:1.42 pkgsrc/pkgtools/pkglint/files/mklines_test.go:1.43
--- pkgsrc/pkgtools/pkglint/files/mklines_test.go:1.42 Sat Apr 27 19:33:57 2019
+++ pkgsrc/pkgtools/pkglint/files/mklines_test.go Mon May 6 20:27:17 2019
@@ -903,6 +903,7 @@ func (s *Suite) Test_MkLines_Check__VERS
t := s.Init(c)
t.SetUpVartypes()
+ t.SetUpMasterSite("MASTER_SITE_SOURCEFORGE", "https://download.sf.net/";)
mklines := t.NewMkLines("geography/viking/Makefile",
MkRcsID,
"MASTER_SITES=\t${MASTER_SITE_SOURCEFORGE:=viking/}${VERSION}/")
Index: pkgsrc/pkgtools/pkglint/files/package.go
diff -u pkgsrc/pkgtools/pkglint/files/package.go:1.52 pkgsrc/pkgtools/pkglint/files/package.go:1.53
--- pkgsrc/pkgtools/pkglint/files/package.go:1.52 Sat Apr 27 19:33:57 2019
+++ pkgsrc/pkgtools/pkglint/files/package.go Mon May 6 20:27:17 2019
@@ -218,11 +218,11 @@ func (pkg *Package) load() ([]string, Mk
return files, mklines, allLines
}
-func (pkg *Package) check(files []string, mklines, allLines MkLines) {
+func (pkg *Package) check(filenames []string, mklines, allLines MkLines) {
haveDistinfo := false
havePatches := false
- for _, filename := range files {
+ for _, filename := range filenames {
if containsVarRef(filename) {
if trace.Tracing {
trace.Stepf("Skipping file %q because the name contains an unresolved variable.", filename)
@@ -1013,6 +1013,28 @@ func (pkg *Package) CheckVarorder(mkline
seeGuide("Package components, Makefile", "components.Makefile"))
}
+func (pkg *Package) checkFileMakefileExt(filename string) {
+ base := path.Base(filename)
+ if !hasPrefix(base, "Makefile.") || base == "Makefile.common" {
+ return
+ }
+ ext := strings.TrimPrefix(base, "Makefile.")
+
+ line := NewLineWhole(filename)
+ line.Notef("Consider renaming %q to %q.", base, ext+".mk")
+ line.Explain(
+ "The main definition of a pkgsrc package should be in the Makefile.",
+ "Common definitions for a few very closely related packages can be",
+ "placed in a Makefile.common, these may cover various topics.",
+ "",
+ "All other definitions should be grouped by topics and implemented",
+ "in separate files named *.mk after their topics. Typical examples",
+ "are extension.mk, module.mk, version.mk.",
+ "",
+ "These topic files should be documented properly so that their",
+ sprintf("content can be queried using %q.", makeHelp("help")))
+}
+
// checkLocallyModified checks files that are about to be committed.
// Depending on whether the package has a MAINTAINER or an OWNER,
// the wording differs.
Index: pkgsrc/pkgtools/pkglint/files/package_test.go
diff -u pkgsrc/pkgtools/pkglint/files/package_test.go:1.45 pkgsrc/pkgtools/pkglint/files/package_test.go:1.46
--- pkgsrc/pkgtools/pkglint/files/package_test.go:1.45 Sat Apr 27 19:33:57 2019
+++ pkgsrc/pkgtools/pkglint/files/package_test.go Mon May 6 20:27:17 2019
@@ -1445,3 +1445,32 @@ func (s *Suite) Test_Package_readMakefil
"~/category/package/Makefile:22: ",
"~/category/package/Makefile:23: .include \"../../mk/bsd.pkg.mk\"")
}
+
+// As of April 2019, there are only a few files in the whole pkgsrc tree
+// that are called Makefile.*, except Makefile.common, which occurs more
+// often.
+//
+// Using the file extension for variants of that Makefile is confusing,
+// therefore they should be renamed to *.mk.
+func (s *Suite) Test_Package__Makefile_files(c *check.C) {
+ t := s.Init(c)
+
+ t.SetUpPackage("category/package")
+ t.CreateFileLines("category/package/Makefile.common",
+ MkRcsID)
+ t.CreateFileLines("category/package/Makefile.orig",
+ MkRcsID)
+ t.CreateFileLines("category/package/Makefile.php",
+ MkRcsID)
+ t.CreateFileLines("category/package/ext.mk",
+ MkRcsID)
+ t.FinishSetUp()
+
+ G.Check(t.File("category/package"))
+
+ // No warning for the Makefile.orig since the package is not
+ // being imported at the moment; see Pkglint.checkReg.
+ t.CheckOutputLines(
+ "NOTE: ~/category/package/Makefile.php: " +
+ "Consider renaming \"Makefile.php\" to \"php.mk\".")
+}
Index: pkgsrc/pkgtools/pkglint/files/shell_test.go
diff -u pkgsrc/pkgtools/pkglint/files/shell_test.go:1.45 pkgsrc/pkgtools/pkglint/files/shell_test.go:1.46
--- pkgsrc/pkgtools/pkglint/files/shell_test.go:1.45 Sat Apr 27 19:33:57 2019
+++ pkgsrc/pkgtools/pkglint/files/shell_test.go Mon May 6 20:27:17 2019
@@ -409,7 +409,8 @@ func (s *Suite) Test_ShellLineChecker_Ch
ck.CheckShellCommandLine("pax -rwpp -s /.*~$$//g . ${DESTDIR}${PREFIX}")
- t.CheckOutputEmpty()
+ t.CheckOutputLines(
+ "WARN: filename.mk:1: Substitution commands like \"/.*~$$//g\" should always be quoted.")
}
func (s *Suite) Test_ShellLineChecker_CheckWord(c *check.C) {
@@ -1188,30 +1189,39 @@ func (s *Suite) Test_ShellProgramChecker
func (s *Suite) Test_SimpleCommandChecker_checkRegexReplace(c *check.C) {
t := s.Init(c)
- t.SetUpTool("pax", "PAX", AtRunTime)
- t.SetUpTool("sed", "SED", AtRunTime)
- mklines := t.NewMkLines("Makefile",
- MkRcsID,
- "pre-configure:",
- "\t${PAX} -s s,.*,, src dst",
- "\tpax -s s,.*,, src dst",
- "\t${SED} -e s,.*,, src dst",
- "\tsed -e s,.*,, src dst",
- "\tpax -s s,\\.orig,, src dst",
- "\tsed -e s,a,b,g src dst")
+ test := func(cmd string, diagnostics ...string) {
+ t.SetUpTool("pax", "PAX", AtRunTime)
+ t.SetUpTool("sed", "SED", AtRunTime)
+ mklines := t.NewMkLines("Makefile",
+ MkRcsID,
+ "pre-configure:",
+ "\t"+cmd)
- mklines.Check()
+ mklines.Check()
+
+ t.CheckOutput(diagnostics)
+ }
+
+ test("${PAX} -s s,.*,, src dst",
+ "WARN: Makefile:3: Substitution commands like \"s,.*,,\" should always be quoted.")
+
+ test("pax -s s,.*,, src dst",
+ "WARN: Makefile:3: Substitution commands like \"s,.*,,\" should always be quoted.")
+
+ test("${SED} -e s,.*,, src dst",
+ "WARN: Makefile:3: Substitution commands like \"s,.*,,\" should always be quoted.")
+
+ test("sed -e s,.*,, src dst",
+ "WARN: Makefile:3: Substitution commands like \"s,.*,,\" should always be quoted.")
+
+ test("pax -s s,\\.orig,, src dst",
+ nil...)
+
+ test("sed -e s,a,b,g src dst",
+ nil...)
- // FIXME: warn for "pax -s".
- // FIXME: warn for "sed -e".
- // TODO: don't warn for "pax .orig".
- // TODO: don't warn for "s,a,b,g".
// TODO: Merge the code with BtSedCommands.
// TODO: Finally, remove the G.Testing from the main code.
- t.CheckOutputLines(
- "WARN: Makefile:3: Substitution commands like \"s,.*,,\" should always be quoted.",
- "WARN: Makefile:5: Substitution commands like \"s,.*,,\" should always be quoted.")
-
}
func (s *Suite) Test_ShellProgramChecker_checkSetE__simple_commands(c *check.C) {
Index: pkgsrc/pkgtools/pkglint/files/pkglint.go
diff -u pkgsrc/pkgtools/pkglint/files/pkglint.go:1.53 pkgsrc/pkgtools/pkglint/files/pkglint.go:1.54
--- pkgsrc/pkgtools/pkglint/files/pkglint.go:1.53 Sat Apr 27 19:33:57 2019
+++ pkgsrc/pkgtools/pkglint/files/pkglint.go Mon May 6 20:27:17 2019
@@ -45,8 +45,7 @@ type Pkglint struct {
// There is no other use for it.
cwd string
- Hashes map[string]*Hash // Maps "alg:filename" => hash (inter-package check).
- UsedLicenses map[string]bool // Maps "license name" => true (inter-package check).
+ InterPackage InterPackage
}
func NewPkglint() Pkglint {
@@ -60,6 +59,58 @@ func NewPkglint() Pkglint {
interner: NewStringInterner()}
}
+type InterPackage struct {
+ hashes map[string]*Hash // Maps "alg:filename" => hash (inter-package check).
+ usedLicenses map[string]struct{} // Maps "license name" => true (inter-package check).
+ bl3Names map[string]Location // Maps buildlink3 identifiers to their first occurrence.
+}
+
+func (ip *InterPackage) Enable() {
+ *ip = InterPackage{
+ make(map[string]*Hash),
+ make(map[string]struct{}),
+ make(map[string]Location)}
+}
+
+func (ip *InterPackage) Enabled() bool { return ip.hashes != nil }
+
+func (ip *InterPackage) Hash(alg, filename string, hashBytes []byte, loc *Location) *Hash {
+ key := alg + ":" + filename
+ if otherHash := ip.hashes[key]; otherHash != nil {
+ return otherHash
+ }
+
+ ip.hashes[key] = &Hash{hashBytes, *loc}
+ return nil
+}
+
+func (ip *InterPackage) UseLicense(name string) {
+ if ip.usedLicenses != nil {
+ ip.usedLicenses[intern(name)] = struct{}{}
+ }
+}
+
+func (ip *InterPackage) LicenseUsed(name string) bool {
+ _, used := ip.usedLicenses[name]
+ return used
+}
+
+// Bl3 remembers that the given buildlink3 name is used at the given location.
+// Since these names must be unique, there should be no other location where
+// the same name is used.
+func (ip *InterPackage) Bl3(name string, loc *Location) *Location {
+ if ip.bl3Names == nil {
+ return nil
+ }
+
+ if prev, found := ip.bl3Names[name]; found {
+ return &prev
+ }
+
+ ip.bl3Names[name] = *loc
+ return nil
+}
+
type CmdOpts struct {
CheckExtra,
CheckGlobal bool
@@ -575,6 +626,10 @@ func CheckFileMk(filename string) {
return
}
+ if G.Pkg != nil {
+ G.Pkg.checkFileMakefileExt(filename)
+ }
+
mklines.Check()
mklines.SaveAutofixChanges()
}
Index: pkgsrc/pkgtools/pkglint/files/pkgsrc.go
diff -u pkgsrc/pkgtools/pkglint/files/pkgsrc.go:1.25 pkgsrc/pkgtools/pkglint/files/pkgsrc.go:1.26
--- pkgsrc/pkgtools/pkglint/files/pkgsrc.go:1.25 Sat Apr 27 19:33:57 2019
+++ pkgsrc/pkgtools/pkglint/files/pkgsrc.go Mon May 6 20:27:17 2019
@@ -13,6 +13,10 @@ import (
// Pkgsrc describes a pkgsrc installation.
// In each pkglint run, only a single pkgsrc installation is ever loaded.
// It just doesn't make sense to check multiple pkgsrc installations at once.
+//
+// This type only contains data that is loaded once and then stays constant.
+// Everything else (distfile hashes, package names) is recorded in the Pkglint
+// type instead.
type Pkgsrc struct {
// The top directory (PKGSRCDIR), either absolute or relative to
// the current working directory.
@@ -249,15 +253,14 @@ func (src *Pkgsrc) ListVersions(category
}
func (src *Pkgsrc) checkToplevelUnusedLicenses() {
- usedLicenses := G.UsedLicenses
- if usedLicenses == nil {
+ if !G.InterPackage.Enabled() {
return
}
licensesDir := src.File("licenses")
for _, licenseFile := range src.ReadDir("licenses") {
licenseName := licenseFile.Name()
- if !usedLicenses[licenseName] {
+ if !G.InterPackage.LicenseUsed(licenseName) {
licensePath := licensesDir + "/" + licenseName
if fileExists(licensePath) {
NewLineWhole(licensePath).Warnf("This license seems to be unused.")
@@ -387,6 +390,7 @@ func (src *Pkgsrc) loadUntypedVars() {
}
handleFile := func(pathName string, info os.FileInfo, err error) error {
+ G.AssertNil(err, "handleFile %q", pathName)
baseName := info.Name()
if hasSuffix(baseName, ".mk") || baseName == "mk.conf" {
handleMkFile(filepath.ToSlash(pathName))
Index: pkgsrc/pkgtools/pkglint/files/redundantscope_test.go
diff -u pkgsrc/pkgtools/pkglint/files/redundantscope_test.go:1.3 pkgsrc/pkgtools/pkglint/files/redundantscope_test.go:1.4
--- pkgsrc/pkgtools/pkglint/files/redundantscope_test.go:1.3 Sat Apr 20 17:43:24 2019
+++ pkgsrc/pkgtools/pkglint/files/redundantscope_test.go Mon May 6 20:27:17 2019
@@ -730,10 +730,118 @@ func (s *Suite) Test_RedundantScope__var
"NOTE: filename.mk:4: Definition of VAR is redundant because of line 2.")
}
+func (s *Suite) Test_RedundantScope__incomplete_then_default(c *check.C) {
+ t := s.Init(c)
+
+ include, get := t.SetUpHierarchy()
+
+ include("including.mk",
+ ".if ${OPSYS} == NetBSD",
+ "VAR=\tNetBSD",
+ ".elif ${OPSYS} == FreeBSD",
+ "VAR=\tFreeBSD",
+ ".endif",
+ "",
+ "VAR?=\tdefault")
+
+ mklines := get("including.mk")
+
+ NewRedundantScope().Check(mklines)
+
+ t.CheckOutputEmpty()
+}
+
+func (s *Suite) Test_RedundantScope__complete_then_default(c *check.C) {
+ t := s.Init(c)
+
+ include, get := t.SetUpHierarchy()
+
+ include("including.mk",
+ ".if ${OPSYS} == NetBSD",
+ "VAR=\tNetBSD",
+ ".else",
+ "VAR=\tFreeBSD",
+ ".endif",
+ "",
+ "VAR?=\tdefault")
+
+ mklines := get("including.mk")
+
+ NewRedundantScope().Check(mklines)
+
+ // TODO: Pkglint could know that the ?= is redundant because VAR is
+ // definitely assigned.
+ t.CheckOutputEmpty()
+}
+
+func (s *Suite) Test_RedundantScope__conditional_then_override(c *check.C) {
+ t := s.Init(c)
+
+ include, get := t.SetUpHierarchy()
+
+ include("including.mk",
+ ".if ${OPSYS} == NetBSD",
+ "VAR=\tNetBSD",
+ ".else",
+ "VAR=\tFreeBSD",
+ ".endif",
+ "",
+ "VAR=\tdefault")
+
+ mklines := get("including.mk")
+
+ NewRedundantScope().Check(mklines)
+
+ // TODO: Pkglint could know that no matter which branch is taken,
+ // the variable will be overwritten in the last line.
+ t.CheckOutputEmpty()
+}
+
+func (s *Suite) Test_RedundantScope__set_then_conditional(c *check.C) {
+ t := s.Init(c)
+
+ include, get := t.SetUpHierarchy()
+
+ include("including.mk",
+ "VAR=\tdefault",
+ "",
+ ".if ${OPSYS} == NetBSD",
+ "VAR=\tNetBSD",
+ ".else",
+ "VAR=\tFreeBSD",
+ ".endif")
+
+ mklines := get("including.mk")
+
+ NewRedundantScope().Check(mklines)
+
+ // TODO: Pkglint could know that no matter which branch is taken,
+ // one of the branches will overwrite the assignment from line 1.
+ t.CheckOutputEmpty()
+}
+
+func (s *Suite) Test_RedundantScope__branch_with_set_then_set(c *check.C) {
+ t := s.Init(c)
+
+ include, get := t.SetUpHierarchy()
+
+ include("including.mk",
+ ".if ${OPSYS} == NetBSD",
+ "VAR=\tfirst",
+ "VAR=\tsecond",
+ ".endif")
+
+ mklines := get("including.mk")
+
+ NewRedundantScope().Check(mklines)
+
+ // TODO: Pkglint could know that the second assignment overwrites the
+ // first assignment since they are in the same basic block.
+ t.CheckOutputEmpty()
+}
+
// FIXME: Continue the systematic redundancy tests.
//
-// Tests where the variables are defined conditionally using .if, .else, .endif.
-//
// Tests where the variables are defined in a .for loop that might not be
// evaluated at all.
//
Index: pkgsrc/pkgtools/pkglint/files/shell.go
diff -u pkgsrc/pkgtools/pkglint/files/shell.go:1.39 pkgsrc/pkgtools/pkglint/files/shell.go:1.40
--- pkgsrc/pkgtools/pkglint/files/shell.go:1.39 Tue Apr 23 21:20:49 2019
+++ pkgsrc/pkgtools/pkglint/files/shell.go Mon May 6 20:27:17 2019
@@ -189,10 +189,30 @@ func (ck *ShellLineChecker) checkVaruseT
// This is ok as long as these variables don't have embedded [$\\"'`].
case quoting == shqDquot && varuse.IsQ():
- ck.mkline.Warnf("The :Q modifier should not be used inside double quotes.")
- ck.mkline.Explain(
+ ck.Warnf("The :Q modifier should not be used inside double quotes.")
+ ck.Explain(
+ "The :Q modifier is intended for embedding a string into a shell program.",
+ "It escapes all characters that have a special meaning in shell programs.",
+ "It only works correctly when it appears outside of \"double\" or 'single'",
+ "quotes or `backticks`.",
+ "",
+ "When it is used inside double quotes or backticks, the resulting string may",
+ "contain more backslashes than intended.",
+ "",
+ "When it is used inside single quotes and the string contains a single quote",
+ "itself, it produces syntax errors in the shell.",
+ "",
"To fix this warning, either remove the :Q or the double quotes.",
- "In most cases, it is more appropriate to remove the double quotes.")
+ "In most cases, it is more appropriate to remove the double quotes.",
+ "",
+ "A special case is for empty strings.",
+ "If the empty string should be preserved as an empty string,",
+ "the correct form is ${VAR:Q}'' with either leading or trailing single or double quotes.",
+ "If the empty string should just be skipped,",
+ "a simple ${VAR:Q} without any surrounding quotes is correct.")
+
+ // TODO: What about single quotes?
+ // TODO: What about backticks?
}
if ck.checkVarUse {
@@ -623,17 +643,42 @@ func (scc *SimpleCommandChecker) checkRe
defer trace.Call0()()
}
- cmdname := scc.strcmd.Name
- isSubst := false
- for _, arg := range scc.strcmd.Args {
- if G.Testing && isSubst && !matches(arg, `"^[\"\'].*[\"\']$`) {
- scc.Warnf("Substitution commands like %q should always be quoted.", arg)
- scc.Explain(
- "Usually these substitution commands contain characters like '*' or",
- "other shell metacharacters that might lead to lookup of matching",
- "filenames and then expand to more than one word.")
+ if !G.Testing {
+ return
+ }
+
+ checkArg := func(arg string) {
+ if matches(arg, `"^[\"\'].*[\"\']$`) {
+ return
}
- isSubst = cmdname == "${PAX}" && arg == "-s" || cmdname == "${SED}" && arg == "-e"
+
+ // Substitution commands that consist only of safe characters cannot
+ // have any side effects, therefore they don't need to be quoted.
+ if matches(arg, `^([\w,.]|\\[.])+$`) {
+ return
+ }
+
+ scc.Warnf("Substitution commands like %q should always be quoted.", arg)
+ scc.Explain(
+ "Usually these substitution commands contain characters like '*' or",
+ "other shell metacharacters that might lead to lookup of matching",
+ "filenames and then expand to more than one word.")
+ }
+
+ checkArgAfter := func(opt string) {
+ args := scc.strcmd.Args
+ for i, arg := range args {
+ if i > 0 && args[i-1] == opt {
+ checkArg(arg)
+ }
+ }
+ }
+
+ switch scc.strcmd.Name {
+ case "${PAX}", "pax":
+ checkArgAfter("-s")
+ case "${SED}", "sed":
+ checkArgAfter("-e")
}
}
Index: pkgsrc/pkgtools/pkglint/files/toplevel.go
diff -u pkgsrc/pkgtools/pkglint/files/toplevel.go:1.17 pkgsrc/pkgtools/pkglint/files/toplevel.go:1.18
--- pkgsrc/pkgtools/pkglint/files/toplevel.go:1.17 Thu Feb 21 22:49:03 2019
+++ pkgsrc/pkgtools/pkglint/files/toplevel.go Mon May 6 20:27:17 2019
@@ -29,8 +29,7 @@ func CheckdirToplevel(dir string) {
if G.Opts.Recursive {
if G.Opts.CheckGlobal {
- G.UsedLicenses = make(map[string]bool)
- G.Hashes = make(map[string]*Hash)
+ G.InterPackage.Enable()
}
G.Todo = append(append([]string(nil), ctx.subdirs...), G.Todo...)
}
Index: pkgsrc/pkgtools/pkglint/files/util.go
diff -u pkgsrc/pkgtools/pkglint/files/util.go:1.43 pkgsrc/pkgtools/pkglint/files/util.go:1.44
--- pkgsrc/pkgtools/pkglint/files/util.go:1.43 Sat Apr 27 19:33:57 2019
+++ pkgsrc/pkgtools/pkglint/files/util.go Mon May 6 20:27:17 2019
@@ -91,6 +91,22 @@ func trimHspace(str string) string {
return str[start:end]
}
+func trimCommon(a, b string) (string, string) {
+ // trim common prefix
+ for len(a) > 0 && len(b) > 0 && a[0] == b[0] {
+ a = a[1:]
+ b = b[1:]
+ }
+
+ // trim common suffix
+ for len(a) > 0 && len(b) > 0 && a[len(a)-1] == b[len(b)-1] {
+ a = a[:len(a)-1]
+ b = b[:len(b)-1]
+ }
+
+ return a, b
+}
+
func isHspace(ch byte) bool {
return ch == ' ' || ch == '\t'
}
Index: pkgsrc/pkgtools/pkglint/files/vardefs.go
diff -u pkgsrc/pkgtools/pkglint/files/vardefs.go:1.63 pkgsrc/pkgtools/pkglint/files/vardefs.go:1.64
--- pkgsrc/pkgtools/pkglint/files/vardefs.go:1.63 Thu May 2 08:36:10 2019
+++ pkgsrc/pkgtools/pkglint/files/vardefs.go Mon May 6 20:27:17 2019
@@ -1220,7 +1220,7 @@ func (reg *VarTypeRegistry) Init(src *Pk
pkg("MAKE_FILE", BtPathname)
pkglist("MAKE_FLAGS", BtShellWord)
pkglist("MAKE_FLAGS.*", BtShellWord)
- pkg("MAKE_JOBS_SAFE", BtYesNo)
+ pkgrat("MAKE_JOBS_SAFE", BtYesNo)
pkg("MAKE_PROGRAM", BtShellCommand)
pkg("MANCOMPRESSED", BtYesNo)
pkg("MANCOMPRESSED_IF_MANZ", BtYes)
@@ -1228,44 +1228,20 @@ func (reg *VarTypeRegistry) Init(src *Pk
sys("MANMODE", BtFileMode)
sys("MANOWN", BtUserGroupName)
pkglist("MASTER_SITES", BtFetchURL)
- // TODO: Extract the MASTER_SITE_* definitions from mk/fetch/sites.mk
- // instead of listing them here.
- syslist("MASTER_SITE_APACHE", BtFetchURL)
- syslist("MASTER_SITE_BACKUP", BtFetchURL)
- syslist("MASTER_SITE_CRATESIO", BtFetchURL)
- syslist("MASTER_SITE_CYGWIN", BtFetchURL)
- syslist("MASTER_SITE_DEBIAN", BtFetchURL)
- syslist("MASTER_SITE_FREEBSD", BtFetchURL)
- syslist("MASTER_SITE_FREEBSD_LOCAL", BtFetchURL)
- syslist("MASTER_SITE_GENTOO", BtFetchURL)
- syslist("MASTER_SITE_GITHUB", BtFetchURL)
- syslist("MASTER_SITE_GNOME", BtFetchURL)
- syslist("MASTER_SITE_GNU", BtFetchURL)
- syslist("MASTER_SITE_GNUSTEP", BtFetchURL)
- syslist("MASTER_SITE_IFARCHIVE", BtFetchURL)
- syslist("MASTER_SITE_HASKELL_HACKAGE", BtFetchURL)
- syslist("MASTER_SITE_KDE", BtFetchURL)
- syslist("MASTER_SITE_LOCAL", BtFetchURL)
- syslist("MASTER_SITE_MOZILLA", BtFetchURL)
- syslist("MASTER_SITE_MOZILLA_ALL", BtFetchURL)
- syslist("MASTER_SITE_MOZILLA_ESR", BtFetchURL)
- syslist("MASTER_SITE_MYSQL", BtFetchURL)
- syslist("MASTER_SITE_NETLIB", BtFetchURL)
- syslist("MASTER_SITE_OPENBSD", BtFetchURL)
- syslist("MASTER_SITE_OPENOFFICE", BtFetchURL)
- syslist("MASTER_SITE_OSDN", BtFetchURL)
- syslist("MASTER_SITE_PERL_CPAN", BtFetchURL)
- syslist("MASTER_SITE_PGSQL", BtFetchURL)
- syslist("MASTER_SITE_PYPI", BtFetchURL)
- syslist("MASTER_SITE_R_CRAN", BtFetchURL)
- syslist("MASTER_SITE_RUBYGEMS", BtFetchURL)
- syslist("MASTER_SITE_SOURCEFORGE", BtFetchURL)
- syslist("MASTER_SITE_SUNSITE", BtFetchURL)
- syslist("MASTER_SITE_SUSE", BtFetchURL)
- syslist("MASTER_SITE_TEX_CTAN", BtFetchURL)
- syslist("MASTER_SITE_XCONTRIB", BtFetchURL)
- syslist("MASTER_SITE_XEMACS", BtFetchURL)
- syslist("MASTER_SITE_XORG", BtFetchURL)
+
+ for _, filename := range []string{"mk/fetch/sites.mk", "mk/fetch/fetch.mk"} {
+ sitesMk := LoadMk(src.File(filename), NotEmpty)
+ if sitesMk != nil {
+ sitesMk.ForEach(func(mkline MkLine) {
+ if mkline.IsVarassign() && hasPrefix(mkline.Varname(), "MASTER_SITE_") {
+ syslist(mkline.Varname(), BtFetchURL)
+ }
+ })
+ } else {
+ // During tests, use t.SetUpMasterSite instead to declare these variables.
+ }
+ }
+
pkglist("MESSAGE_SRC", BtPathname)
pkglist("MESSAGE_SUBST", BtShellWord)
pkg("META_PACKAGE", BtYes)
Home |
Main Index |
Thread Index |
Old Index