diff options
author | rillig <rillig@pkgsrc.org> | 2020-01-18 21:56:09 +0000 |
---|---|---|
committer | rillig <rillig@pkgsrc.org> | 2020-01-18 21:56:09 +0000 |
commit | 18cbbb5ad54e44b4f4b0d62c3a801c287d36295d (patch) | |
tree | dc67b8d14b17de5611d734262eef611cbf2edc00 /pkgtools | |
parent | fa6099c821dfad37f55c8eec60b48a1d692a52a7 (diff) | |
download | pkgsrc-18cbbb5ad54e44b4f4b0d62c3a801c287d36295d.tar.gz |
pkgtools/pkglint: update to 19.4.4
Changes since 19.4.3:
Packages that still use http in their HOMEPAGE URL generate warnings that
the URL should use https instead.
Diffstat (limited to 'pkgtools')
22 files changed, 457 insertions, 143 deletions
diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index fd4e0082407..2d377ca9a15 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.625 2020/01/11 15:48:28 rillig Exp $ +# $NetBSD: Makefile,v 1.626 2020/01/18 21:56:09 rillig Exp $ -PKGNAME= pkglint-19.4.3 +PKGNAME= pkglint-19.4.4 CATEGORIES= pkgtools DISTNAME= tools MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/} diff --git a/pkgtools/pkglint/files/buildlink3.go b/pkgtools/pkglint/files/buildlink3.go index 58d4a21c8c1..5210ff15503 100644 --- a/pkgtools/pkglint/files/buildlink3.go +++ b/pkgtools/pkglint/files/buildlink3.go @@ -202,7 +202,7 @@ func (ck *Buildlink3Checker) checkVarassign(mkline *MkLine, pkgbase string) { if varname == "BUILDLINK_ABI_DEPENDS."+pkgbase { ck.abiLine = mkline parser := NewMkParser(nil, value) - if dp := parser.Dependency(); dp != nil && parser.EOF() { + if dp := parser.DependencyPattern(); dp != nil && parser.EOF() { ck.abi = dp } doCheck = true @@ -211,7 +211,7 @@ func (ck *Buildlink3Checker) checkVarassign(mkline *MkLine, pkgbase string) { if varname == "BUILDLINK_API_DEPENDS."+pkgbase { ck.apiLine = mkline parser := NewMkParser(nil, value) - if dp := parser.Dependency(); dp != nil && parser.EOF() { + if dp := parser.DependencyPattern(); dp != nil && parser.EOF() { ck.api = dp } doCheck = true diff --git a/pkgtools/pkglint/files/check_test.go b/pkgtools/pkglint/files/check_test.go index 7c32bf9d636..9c229bbc9f7 100644 --- a/pkgtools/pkglint/files/check_test.go +++ b/pkgtools/pkglint/files/check_test.go @@ -145,6 +145,9 @@ func Test__qa(t *testing.T) { ck.Configure("vargroups.go", "*", "*", -intqa.EMissingTest) // TODO ck.Configure("vartype.go", "*", "*", -intqa.EMissingTest) // TODO + // Don't require tests for helper methods. + ck.Configure("*.go", "VartypeCheck", "[a-z]*", -intqa.EMissingTest) + // For now, don't require tests for all the test code. // Having good coverage for the main code is more important. ck.Configure("*_test.go", "*", "*", -intqa.EMissingTest) diff --git a/pkgtools/pkglint/files/licenses.go b/pkgtools/pkglint/files/licenses.go index 43656bf533a..927e4df83b8 100644 --- a/pkgtools/pkglint/files/licenses.go +++ b/pkgtools/pkglint/files/licenses.go @@ -28,8 +28,10 @@ func (lc *LicenseChecker) checkName(license string) { pkg := lc.MkLines.pkg if pkg != nil { if mkline := pkg.vars.FirstDefinition("LICENSE_FILE"); mkline != nil { - rel := mkline.ResolveVarsInRelativePath(NewRelPathString(mkline.Value()), lc.MkLines.pkg) - licenseFile = pkg.File(NewPackagePath(rel)) + // TODO: Not every path is relative to the package directory. + rel := NewPackagePathString(mkline.Value()) + relResolved := mkline.ResolveVarsInRelativePath(rel, lc.MkLines.pkg) + licenseFile = pkg.File(relResolved) } } if licenseFile.IsEmpty() { diff --git a/pkgtools/pkglint/files/mkassignchecker.go b/pkgtools/pkglint/files/mkassignchecker.go index 5e18f8b07c2..9d23e6fd2e0 100644 --- a/pkgtools/pkglint/files/mkassignchecker.go +++ b/pkgtools/pkglint/files/mkassignchecker.go @@ -284,7 +284,7 @@ func (ck *MkAssignChecker) checkVarassignLeftRationale() { return } - if mkline.HasRationale() { + if mkline.Rationale() != "" { return } diff --git a/pkgtools/pkglint/files/mkcondchecker.go b/pkgtools/pkglint/files/mkcondchecker.go index 57629eb1257..3bba635f716 100644 --- a/pkgtools/pkglint/files/mkcondchecker.go +++ b/pkgtools/pkglint/files/mkcondchecker.go @@ -36,9 +36,10 @@ func (ck *MkCondChecker) Check() { // Skip subconditions that have already been handled as part of the !(...). done := make(map[interface{}]bool) - checkNotEmpty := func(not *MkCond) { + checkNot := func(not *MkCond) { empty := not.Empty if empty != nil { + ck.checkNotEmpty(not) ck.checkEmpty(empty, true, true) done[empty] = true } @@ -48,6 +49,8 @@ func (ck *MkCondChecker) Check() { ck.checkEmpty(varUse, false, false) done[varUse] = true } + + ck.checkNotCompare(not) } checkEmpty := func(empty *MkVarUse) { @@ -63,13 +66,29 @@ func (ck *MkCondChecker) Check() { } cond.Walk(&MkCondCallback{ - Not: checkNotEmpty, + Not: checkNot, Empty: checkEmpty, Var: checkVar, Compare: ck.checkCompare, VarUse: checkVarUse}) } +func (ck *MkCondChecker) checkNotEmpty(not *MkCond) { + // Consider suggesting ${VAR} instead of !empty(VAR) since it is + // shorter and avoids unnecessary negation, which makes the + // expression less confusing. + // + // This applies especially to the ${VAR:Mpattern} form. + // + // See MkCondChecker.simplify. + if !G.Experimental { + return + } + + ck.MkLine.Notef("!empty(%s%s) can be replaced with the simpler %s.", + not.Empty.varname, not.Empty.Mod(), not.Empty.String()) +} + // checkEmpty checks a condition of the form empty(VAR), // empty(VAR:Mpattern) or ${VAR:Mpattern} in an .if directive. func (ck *MkCondChecker) checkEmpty(varuse *MkVarUse, fromEmpty bool, neg bool) { @@ -90,6 +109,7 @@ func (ck *MkCondChecker) checkEmptyExpr(varuse *MkVarUse) { "", "\tempty(VARNAME:Mpattern)", "\t${VARNAME:Mpattern} == \"\"", + "\t!${VARNAME:Mpattern}", "", "Instead of !empty(${VARNAME:Mpattern}), you should write either of the following:", "", @@ -129,7 +149,6 @@ var mkCondModifierPatternLiteral = textproc.NewByteSet("+---./0-9<=>@A-Z_a-z") // * 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) simplify(varuse *MkVarUse, fromEmpty bool, neg bool) { varname := varuse.varname mods := varuse.modifiers @@ -300,3 +319,11 @@ func (ck *MkCondChecker) checkCompareVarStrCompiler(op string, value string) { fix.Replace("${PKGSRC_COMPILER} "+op+" \""+value+"\"", "${PKGSRC_COMPILER:"+matchOp+value+"}") fix.Apply() } + +func (ck *MkCondChecker) checkNotCompare(not *MkCond) { + if not.Compare == nil { + return + } + + ck.MkLine.Warnf("The ! should use parentheses or be merged into the comparison operator.") +} diff --git a/pkgtools/pkglint/files/mkcondchecker_test.go b/pkgtools/pkglint/files/mkcondchecker_test.go index d2116c854da..e4e3e209d46 100644 --- a/pkgtools/pkglint/files/mkcondchecker_test.go +++ b/pkgtools/pkglint/files/mkcondchecker_test.go @@ -90,6 +90,7 @@ func (s *Suite) Test_MkCondChecker_Check(c *check.C) { // Doesn't occur in practice since it is surprising that the ! applies // to the comparison operator, and not to one of its arguments. test(".if !${VAR} == value", + "WARN: filename.mk:4: The ! should use parentheses or be merged into the comparison operator.", "WARN: filename.mk:4: VAR is used but not defined.") // Doesn't occur in practice since this string can never be empty. @@ -215,6 +216,27 @@ func (s *Suite) Test_MkCondChecker_Check__comparing_PKGSRC_COMPILER_with_eqeq(c "ERROR: Makefile:6: Use ${PKGSRC_COMPILER:Ngcc} instead of the != operator.") } +func (s *Suite) Test_MkCondChecker_checkNotEmpty(c *check.C) { + t := s.Init(c) + + G.Experimental = true + + test := func(cond string, diagnostics ...string) { + mklines := t.NewMkLines("filename.mk", + ".if "+cond) + mkline := mklines.mklines[0] + ck := NewMkCondChecker(mkline, mklines) + + ck.checkNotEmpty(mkline.Cond().Not) + + t.CheckOutput(diagnostics) + } + + test("!empty(VAR)", + // FIXME: Add a :U modifier if VAR might be undefined. + "NOTE: filename.mk:1: !empty(VAR) can be replaced with the simpler ${VAR}.") +} + func (s *Suite) Test_MkCondChecker_checkEmpty(c *check.C) { t := s.Init(c) @@ -1150,3 +1172,32 @@ func (s *Suite) Test_MkCondChecker_checkCompareVarStrCompiler(c *check.C) { nil...) } + +func (s *Suite) Test_MkCondChecker_checkNotCompare(c *check.C) { + t := s.Init(c) + + test := func(cond string, diagnostics ...string) { + mklines := t.NewMkLines("filename.mk", + ".if "+cond) + mkline := mklines.mklines[0] + ck := NewMkCondChecker(mkline, mklines) + + ck.checkNotCompare(mkline.Cond().Not) + + t.CheckOutput(diagnostics) + } + + test("!${VAR} == value", + "WARN: filename.mk:1: The ! should use parentheses "+ + "or be merged into the comparison operator.") + + test("!${VAR} != value", + "WARN: filename.mk:1: The ! should use parentheses "+ + "or be merged into the comparison operator.") + + test("!(${VAR} == value)", + nil...) + + test("!${VAR}", + nil...) +} diff --git a/pkgtools/pkglint/files/mkline.go b/pkgtools/pkglint/files/mkline.go index 8d1cf461af9..13d4d5edf66 100644 --- a/pkgtools/pkglint/files/mkline.go +++ b/pkgtools/pkglint/files/mkline.go @@ -74,8 +74,11 @@ func (mkline *MkLine) String() string { func (mkline *MkLine) HasComment() bool { return mkline.splitResult.hasComment } -func (mkline *MkLine) HasRationale() bool { return mkline.splitResult.rationale != "" } - +// Rationale returns the comments that are close enough to this line. +// +// These comments are used to suppress pkglint warnings, +// such as for BROKEN, NOT_FOR_PLATFORMS, MAKE_JOBS_SAFE, +// and HOMEPAGE using http instead of https. func (mkline *MkLine) Rationale() string { return mkline.splitResult.rationale } // Comment returns the comment after the first unescaped #. @@ -564,7 +567,8 @@ func (*MkLine) WithoutMakeVariables(value string) string { return valueNovar.String() } -func (mkline *MkLine) ResolveVarsInRelativePath(relativePath RelPath, pkg *Package) RelPath { +func (mkline *MkLine) ResolveVarsInRelativePath(relativePath PackagePath, pkg *Package) PackagePath { + // TODO: Not every path is relative to the package directory. if !containsVarUse(relativePath.String()) { return relativePath.CleanPath() } diff --git a/pkgtools/pkglint/files/mkline_test.go b/pkgtools/pkglint/files/mkline_test.go index 1b1bacb9b78..312b24f71d6 100644 --- a/pkgtools/pkglint/files/mkline_test.go +++ b/pkgtools/pkglint/files/mkline_test.go @@ -535,7 +535,7 @@ func (s *Suite) Test_MkLine_ResolveVarsInRelativePath(c *check.C) { mkline := mklines.mklines[0] var pkg *Package = nil - test := func(before RelPath, after RelPath) { + test := func(before PackagePath, after PackagePath) { t.CheckEquals(mkline.ResolveVarsInRelativePath(before, pkg), after) } @@ -1164,7 +1164,7 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__uncovered_cases(c *check.C) { "", "GO_SRCPATH=\t\t${HOMEPAGE:S,https://,,}", "LINKER_RPATH_FLAG:=\t${LINKER_RPATH_FLAG:S/-rpath/& /}", - "HOMEPAGE=\t\thttp://godoc.org/${GO_SRCPATH}", + "HOMEPAGE=\t\thttps://godoc.org/${GO_SRCPATH}", "PATH:=\t\t\t${PREFIX}/cross/bin:${PATH}", "NO_SRC_ON_FTP=\t\t${RESTRICTED}") diff --git a/pkgtools/pkglint/files/mklinechecker.go b/pkgtools/pkglint/files/mklinechecker.go index 6fbcf02ee8f..5e728d7e95a 100644 --- a/pkgtools/pkglint/files/mklinechecker.go +++ b/pkgtools/pkglint/files/mklinechecker.go @@ -236,7 +236,8 @@ func (ck MkLineChecker) checkInclude() { if trace.Tracing { trace.Stepf("includingFile=%s includedFile=%s", mkline.Filename, includedFile) } - ck.CheckRelativePath(includedFile, mustExist) + // TODO: Not every path is relative to the package directory. + ck.CheckRelativePath(NewPackagePath(includedFile), includedFile, mustExist) switch { case includedFile.HasBase("Makefile"): @@ -269,7 +270,7 @@ func (ck MkLineChecker) checkInclude() { mkline.Warnf("Please write \"USE_TOOLS+= intltool\" instead of this line.") case includedFile != "builtin.mk" && includedFile.HasSuffixPath("builtin.mk"): - if mkline.Basename != "hacks.mk" && !mkline.HasRationale() { + if mkline.Basename != "hacks.mk" && mkline.Rationale() == "" { fix := mkline.Autofix() fix.Errorf("%q must not be included directly. Include %q instead.", includedFile, includedFile.DirNoClean().JoinNoClean("buildlink3.mk")) @@ -294,26 +295,28 @@ func (ck MkLineChecker) checkDirectiveIndentation(expectedDepth int) { // CheckRelativePath checks a relative path that leads to the directory of another package // or to a subdirectory thereof or a file within there. -func (ck MkLineChecker) CheckRelativePath(relativePath RelPath, mustExist bool) { +func (ck MkLineChecker) CheckRelativePath(pp PackagePath, rel RelPath, mustExist bool) { + // TODO: Not every path is relative to the package directory. if trace.Tracing { - defer trace.Call(relativePath, mustExist)() + defer trace.Call(rel, mustExist)() } mkline := ck.MkLine - if !G.Wip && relativePath.ContainsPath("wip") { + if !G.Wip && rel.ContainsPath("wip") { mkline.Errorf("A main pkgsrc package must not depend on a pkgsrc-wip package.") } - resolvedPath := mkline.ResolveVarsInRelativePath(relativePath, ck.MkLines.pkg) + resolvedPath := mkline.ResolveVarsInRelativePath(pp, ck.MkLines.pkg) if containsVarUse(resolvedPath.String()) { return } - abs := mkline.Filename.DirNoClean().JoinNoClean(resolvedPath) + resolvedRel := resolvedPath.AsRelPath() + abs := mkline.Filename.DirNoClean().JoinNoClean(resolvedRel) if !abs.Exists() { - pkgsrcPath := G.Pkgsrc.Rel(ck.MkLine.File(resolvedPath)) + pkgsrcPath := G.Pkgsrc.Rel(ck.MkLine.File(resolvedRel)) if mustExist && !ck.MkLines.indentation.HasExists(pkgsrcPath) { - mkline.Errorf("Relative path %q does not exist.", resolvedPath) + mkline.Errorf("Relative path %q does not exist.", rel) } return } @@ -353,17 +356,19 @@ func (ck MkLineChecker) CheckRelativePath(relativePath RelPath, mustExist bool) // // When used in .include directives, the relative package directories must be written // with the leading ../.. anyway, so the benefit might not be too big at all. -func (ck MkLineChecker) CheckRelativePkgdir(pkgdir RelPath) { +func (ck MkLineChecker) CheckRelativePkgdir(rel RelPath, pkgdir PackagePath) { + // TODO: Not every path is relative to the package directory. if trace.Tracing { defer trace.Call(pkgdir)() } mkline := ck.MkLine - ck.CheckRelativePath(pkgdir.JoinNoClean("Makefile"), true) + makefile := pkgdir.JoinNoClean("Makefile") + ck.CheckRelativePath(makefile, makefile.AsRelPath(), true) pkgdir = mkline.ResolveVarsInRelativePath(pkgdir, ck.MkLines.pkg) if !matches(pkgdir.String(), `^\.\./\.\./([^./][^/]*/[^./][^/]*)$`) && !containsVarUse(pkgdir.String()) { - mkline.Warnf("%q is not a valid relative package directory.", pkgdir) + mkline.Warnf("%q is not a valid relative package directory.", rel) mkline.Explain( "A relative pathname always starts with \"../../\", followed", "by a category, a slash and a the directory name of the package.", diff --git a/pkgtools/pkglint/files/mklinechecker_test.go b/pkgtools/pkglint/files/mklinechecker_test.go index 9cf6b8babcf..47707cd8aa9 100644 --- a/pkgtools/pkglint/files/mklinechecker_test.go +++ b/pkgtools/pkglint/files/mklinechecker_test.go @@ -703,13 +703,14 @@ func (s *Suite) Test_MkLineChecker_CheckRelativePkgdir(c *check.C) { t.CreateFileLines("other/package/Makefile") - test := func(relativePkgdir RelPath, diagnostics ...string) { + test := func(relativePkgdir PackagePath, diagnostics ...string) { // Must be in the filesystem because of directory references. mklines := t.SetUpFileMkLines("category/package/Makefile", "# dummy") checkRelativePkgdir := func(mkline *MkLine) { - MkLineChecker{mklines, mkline}.CheckRelativePkgdir(relativePkgdir) + ck := MkLineChecker{mklines, mkline} + ck.CheckRelativePkgdir(relativePkgdir.AsRelPath(), relativePkgdir) } mklines.ForEach(checkRelativePkgdir) diff --git a/pkgtools/pkglint/files/mklines_test.go b/pkgtools/pkglint/files/mklines_test.go index 40524c53f9a..a96076652c2 100644 --- a/pkgtools/pkglint/files/mklines_test.go +++ b/pkgtools/pkglint/files/mklines_test.go @@ -423,7 +423,7 @@ func (s *Suite) Test_MkLines_collectRationale(c *check.C) { mklines.collectRationale() var actual []string mklines.ForEach(func(mkline *MkLine) { - actual = append(actual, condStr(mkline.HasRationale(), "R ", "- ")+mkline.Text) + actual = append(actual, condStr(mkline.Rationale() != "", "R ", "- ")+mkline.Text) }) t.CheckDeepEquals(actual, specs) } diff --git a/pkgtools/pkglint/files/mkparser.go b/pkgtools/pkglint/files/mkparser.go index 7eedbc2eca7..0a0049fb15e 100644 --- a/pkgtools/pkglint/files/mkparser.go +++ b/pkgtools/pkglint/files/mkparser.go @@ -113,7 +113,7 @@ func (p *MkParser) mkCondCompare() *MkCond { if cond != nil { lexer.SkipHspace() if lexer.SkipByte(')') { - return cond + return &MkCond{Paren: cond} } } lexer.Reset(mark) @@ -247,11 +247,6 @@ func (p *MkParser) mkCondFunc() *MkCond { } } - // TODO: Consider suggesting ${VAR} instead of !empty(VAR) since it is shorter and - // avoids unnecessary negation, which makes the expression less confusing. - // This applies especially to the ${VAR:Mpattern} form. - // See MkCondChecker.simplify. - case "commands", "exists", "make", "target": argMark := lexer.Mark() for p.mklex.VarUse() != nil || lexer.SkipBytesFunc(func(b byte) bool { return b != '$' && b != ')' }) { @@ -341,8 +336,8 @@ type DependencyPattern struct { Wildcard string // "[0-9]*", "1.5.*", "${PYVER}" } -// Dependency parses a dependency pattern like "pkg>=1<2" or "pkg-[0-9]*". -func (p *MkParser) Dependency() *DependencyPattern { +// DependencyPattern parses a dependency pattern like "pkg>=1<2" or "pkg-[0-9]*". +func (p *MkParser) DependencyPattern() *DependencyPattern { lexer := p.lexer parseVersion := func() string { @@ -454,6 +449,7 @@ type MkCond struct { Term *MkCondTerm Compare *MkCondCompare Call *MkCondCall + Paren *MkCond } type MkCondCompare struct { Left MkCondTerm @@ -483,6 +479,7 @@ type MkCondCallback struct { Empty func(empty *MkVarUse) Compare func(left *MkCondTerm, op string, right *MkCondTerm) Call func(name string, arg string) + Paren func(cond *MkCond) // Var is called for every atomic expression that consists solely // of a variable use, possibly enclosed in double quotes, but without @@ -560,6 +557,12 @@ func (w *MkCondWalker) Walk(cond *MkCond, callback *MkCondCallback) { callback.Call(call.Name, call.Arg) } w.walkStr(cond.Call.Arg, callback) + + case cond.Paren != nil: + if callback.Paren != nil { + callback.Paren(cond.Paren) + } + w.Walk(cond.Paren, callback) } } diff --git a/pkgtools/pkglint/files/mkparser_test.go b/pkgtools/pkglint/files/mkparser_test.go index c6c5154b3b0..08638031081 100644 --- a/pkgtools/pkglint/files/mkparser_test.go +++ b/pkgtools/pkglint/files/mkparser_test.go @@ -8,7 +8,37 @@ import ( func (s *Suite) Test_MkParser_MkCond(c *check.C) { t := s.Init(c) b := NewMkTokenBuilder() - varUse := b.VarUse + + cmp := func(left MkCondTerm, op string, right MkCondTerm) *MkCond { + return &MkCond{Compare: &MkCondCompare{left, op, right}} + } + cvar := func(name string, modifiers ...string) MkCondTerm { + return MkCondTerm{Var: b.VarUse(name, modifiers...)} + } + cstr := func(s string) MkCondTerm { return MkCondTerm{Str: s} } + cnum := func(s string) MkCondTerm { return MkCondTerm{Num: s} } + + termVar := func(varname string, mods ...string) *MkCond { + return &MkCond{Term: &MkCondTerm{Var: b.VarUse(varname, mods...)}} + } + termNum := func(num string) *MkCond { + return &MkCond{Term: &MkCondTerm{Num: num}} + } + termStr := func(str string) *MkCond { + return &MkCond{Term: &MkCondTerm{Str: str}} + } + + or := func(args ...*MkCond) *MkCond { return &MkCond{Or: args} } + and := func(args ...*MkCond) *MkCond { return &MkCond{And: args} } + not := func(cond *MkCond) *MkCond { return &MkCond{Not: cond} } + call := func(name string, arg string) *MkCond { + return &MkCond{Call: &MkCondCall{name, arg}} + } + empty := func(varname string, mods ...string) *MkCond { + return &MkCond{Empty: b.VarUse(varname, mods...)} + } + defined := func(varname string) *MkCond { return &MkCond{Defined: varname} } + paren := func(cond *MkCond) *MkCond { return &MkCond{Paren: cond} } testRest := func(input string, expectedTree *MkCond, expectedRest string) { // As of July 2019 p.MkCond does not emit warnings; @@ -22,158 +52,144 @@ func (s *Suite) Test_MkParser_MkCond(c *check.C) { test := func(input string, expectedTree *MkCond) { testRest(input, expectedTree, "") } - varTerm := func(name string, modifiers ...string) MkCondTerm { - return MkCondTerm{Var: varUse(name, modifiers...)} - } - str := func(s string) MkCondTerm { return MkCondTerm{Str: s} } - num := func(s string) MkCondTerm { return MkCondTerm{Num: s} } - - t.Use(testRest, test, varTerm) test("${OPSYS:MNetBSD}", - &MkCond{Term: &MkCondTerm{Var: varUse("OPSYS", "MNetBSD")}}) + termVar("OPSYS", "MNetBSD")) test("defined(VARNAME)", - &MkCond{Defined: "VARNAME"}) + defined("VARNAME")) test("empty(VARNAME)", - &MkCond{Empty: varUse("VARNAME")}) + empty("VARNAME")) test("!empty(VARNAME)", - &MkCond{Not: &MkCond{Empty: varUse("VARNAME")}}) + not(empty("VARNAME"))) test("!empty(VARNAME:M[yY][eE][sS])", - &MkCond{Not: &MkCond{Empty: varUse("VARNAME", "M[yY][eE][sS]")}}) + not(empty("VARNAME", "M[yY][eE][sS]"))) // Colons are unescaped at this point because they cannot be mistaken for separators anymore. test("!empty(USE_TOOLS:Mautoconf\\:run)", - &MkCond{Not: &MkCond{Empty: varUse("USE_TOOLS", "Mautoconf:run")}}) + not(empty("USE_TOOLS", "Mautoconf:run"))) test("${VARNAME} != \"Value\"", - &MkCond{Compare: &MkCondCompare{varTerm("VARNAME"), "!=", str("Value")}}) + cmp(cvar("VARNAME"), "!=", cstr("Value"))) test("${VARNAME:Mi386} != \"Value\"", - &MkCond{Compare: &MkCondCompare{varTerm("VARNAME", "Mi386"), "!=", str("Value")}}) + cmp(cvar("VARNAME", "Mi386"), "!=", cstr("Value"))) test("${VARNAME} != Value", - &MkCond{Compare: &MkCondCompare{varTerm("VARNAME"), "!=", str("Value")}}) + cmp(cvar("VARNAME"), "!=", cstr("Value"))) test("\"${VARNAME}\" != Value", - &MkCond{Compare: &MkCondCompare{varTerm("VARNAME"), "!=", str("Value")}}) + cmp(cvar("VARNAME"), "!=", cstr("Value"))) test("${pkg} == \"${name}\"", - &MkCond{Compare: &MkCondCompare{varTerm("pkg"), "==", varTerm("name")}}) + cmp(cvar("pkg"), "==", cvar("name"))) test("\"${pkg}\" == \"${name}\"", - &MkCond{Compare: &MkCondCompare{varTerm("pkg"), "==", varTerm("name")}}) + cmp(cvar("pkg"), "==", cvar("name"))) // The right-hand side is not analyzed further to keep the data types simple. test("${ABC} == \"${A}B${C}\"", - &MkCond{Compare: &MkCondCompare{varTerm("ABC"), "==", str("${A}B${C}")}}) + cmp(cvar("ABC"), "==", cstr("${A}B${C}"))) test("${ABC} == \"${A}\\\"${B}\\\\${C}$${shellvar}${D}\"", - &MkCond{Compare: &MkCondCompare{varTerm("ABC"), "==", str("${A}\"${B}\\${C}$${shellvar}${D}")}}) + cmp(cvar("ABC"), "==", cstr("${A}\"${B}\\${C}$${shellvar}${D}"))) test("exists(/etc/hosts)", - &MkCond{Call: &MkCondCall{"exists", "/etc/hosts"}}) + call("exists", "/etc/hosts")) test("exists(${PREFIX}/var)", - &MkCond{Call: &MkCondCall{"exists", "${PREFIX}/var"}}) + call("exists", "${PREFIX}/var")) test("${OPSYS} == \"NetBSD\" || ${OPSYS} == \"OpenBSD\"", - &MkCond{Or: []*MkCond{ - {Compare: &MkCondCompare{varTerm("OPSYS"), "==", str("NetBSD")}}, - {Compare: &MkCondCompare{varTerm("OPSYS"), "==", str("OpenBSD")}}}}) + or( + cmp(cvar("OPSYS"), "==", cstr("NetBSD")), + cmp(cvar("OPSYS"), "==", cstr("OpenBSD")))) test("${OPSYS} == \"NetBSD\" && ${MACHINE_ARCH} == \"i386\"", - &MkCond{And: []*MkCond{ - {Compare: &MkCondCompare{varTerm("OPSYS"), "==", str("NetBSD")}}, - {Compare: &MkCondCompare{varTerm("MACHINE_ARCH"), "==", str("i386")}}}}) + and( + cmp(cvar("OPSYS"), "==", cstr("NetBSD")), + cmp(cvar("MACHINE_ARCH"), "==", cstr("i386")))) test("defined(A) && defined(B) || defined(C) && defined(D)", - &MkCond{Or: []*MkCond{ - {And: []*MkCond{ - {Defined: "A"}, - {Defined: "B"}}}, - {And: []*MkCond{ - {Defined: "C"}, - {Defined: "D"}}}}}) + or( + and(defined("A"), defined("B")), + and(defined("C"), defined("D")))) test("${MACHINE_ARCH:Mi386} || ${MACHINE_OPSYS:MNetBSD}", - &MkCond{Or: []*MkCond{ - {Term: &MkCondTerm{Var: varUse("MACHINE_ARCH", "Mi386")}}, - {Term: &MkCondTerm{Var: varUse("MACHINE_OPSYS", "MNetBSD")}}}}) + or( + termVar("MACHINE_ARCH", "Mi386"), + termVar("MACHINE_OPSYS", "MNetBSD"))) test("${VAR} == \"${VAR}suffix\"", - &MkCond{Compare: &MkCondCompare{varTerm("VAR"), "==", str("${VAR}suffix")}}) + cmp(cvar("VAR"), "==", cstr("${VAR}suffix"))) // Exotic cases // ".if 0" can be used to skip over a block of code. test("0", - &MkCond{Term: &MkCondTerm{Num: "0"}}) + termNum("0")) test("0xCAFEBABE", - &MkCond{Term: &MkCondTerm{Num: "0xCAFEBABE"}}) + termNum("0xCAFEBABE")) test("${VAR} == 0xCAFEBABE", - &MkCond{ - Compare: &MkCondCompare{ - varTerm("VAR"), - "==", - num("0xCAFEBABE")}}) + cmp(cvar("VAR"), "==", cnum("0xCAFEBABE"))) test("! ( defined(A) && empty(VARNAME) )", - &MkCond{Not: &MkCond{ - And: []*MkCond{ - {Defined: "A"}, - {Empty: varUse("VARNAME")}}}}) + not(paren(and(defined("A"), empty("VARNAME"))))) test("${REQD_MAJOR} > ${MAJOR}", - &MkCond{Compare: &MkCondCompare{varTerm("REQD_MAJOR"), ">", varTerm("MAJOR")}}) + cmp(cvar("REQD_MAJOR"), ">", cvar("MAJOR"))) test("${OS_VERSION} >= 6.5", - &MkCond{Compare: &MkCondCompare{varTerm("OS_VERSION"), ">=", num("6.5")}}) + cmp(cvar("OS_VERSION"), ">=", cnum("6.5"))) test("${OS_VERSION} == 5.3", - &MkCond{Compare: &MkCondCompare{varTerm("OS_VERSION"), "==", num("5.3")}}) + cmp(cvar("OS_VERSION"), "==", cnum("5.3"))) test("!empty(${OS_VARIANT:MIllumos})", // Probably not intended - &MkCond{Not: &MkCond{Empty: varUse("${OS_VARIANT:MIllumos}")}}) + not(empty("${OS_VARIANT:MIllumos}"))) - // There may be whitespace before the parenthesis; see devel/bmake/files/cond.c:^compare_function. + // There may be whitespace before the parenthesis. + // See devel/bmake/files/cond.c:/^compare_function/. test("defined (VARNAME)", - &MkCond{Defined: "VARNAME"}) + defined("VARNAME")) test("${\"${PKG_OPTIONS:Moption}\":?--enable-option:--disable-option}", - &MkCond{Term: &MkCondTerm{Var: varUse("\"${PKG_OPTIONS:Moption}\"", "?--enable-option:--disable-option")}}) + termVar("\"${PKG_OPTIONS:Moption}\"", "?--enable-option:--disable-option")) // Contrary to most other programming languages, the == operator binds - // more tightly that the ! operator. + // more tightly than the ! operator. // - // TODO: Since this operator precedence is surprising there should be a warning, - // suggesting to replace "!${VAR} == value" with "${VAR} != value". + // See MkCondChecker.checkNotCompare. test("!${VAR} == value", - &MkCond{Not: &MkCond{Compare: &MkCondCompare{varTerm("VAR"), "==", str("value")}}}) + not(cmp(cvar("VAR"), "==", cstr("value")))) // The left-hand side of the comparison can be a quoted string. test("\"${VAR}suffix\" == value", - &MkCond{Compare: &MkCondCompare{MkCondTerm{Str: "${VAR}suffix"}, "==", MkCondTerm{Str: "value"}}}) + cmp(cstr("${VAR}suffix"), "==", cstr("value"))) test("\"${VAR}str\"", - &MkCond{Term: &MkCondTerm{Str: "${VAR}str"}}) + termStr("${VAR}str")) test("commands(show-var)", - &MkCond{Call: &MkCondCall{"commands", "show-var"}}) + call("commands", "show-var")) test("exists(/usr/bin)", - &MkCond{Call: &MkCondCall{"exists", "/usr/bin"}}) + call("exists", "/usr/bin")) test("make(show-var)", - &MkCond{Call: &MkCondCall{"make", "show-var"}}) + call("make", "show-var")) test("target(do-build)", - &MkCond{Call: &MkCondCall{"target", "do-build"}}) + call("target", "do-build")) + + // TODO: ok "${q}text${q} == ${VAR}" + // TODO: fail "text${q} == ${VAR}" + // TODO: ok "${VAR} == ${q}text${q}" // Errors @@ -202,11 +218,11 @@ func (s *Suite) Test_MkParser_MkCond(c *check.C) { "exists(/unfinished") testRest("!empty(PKG_OPTIONS:Msndfile) || defined(PKG_OPTIONS:Msamplerate)", - &MkCond{Not: &MkCond{Empty: varUse("PKG_OPTIONS", "Msndfile")}}, + not(empty("PKG_OPTIONS", "Msndfile")), "|| defined(PKG_OPTIONS:Msamplerate)") testRest("${LEFT} &&", - &MkCond{Term: &MkCondTerm{Var: varUse("LEFT")}}, + termVar("LEFT"), "&&") testRest("\"unfinished string literal", @@ -243,7 +259,7 @@ func (s *Suite) Test_MkParser_MkCond(c *check.C) { // Too many closing parentheses are a syntax error. testRest("(${VAR}))", - &MkCond{Term: &MkCondTerm{Var: varUse("VAR")}}, + paren(termVar("VAR")), ")") // The left-hand side of the comparison cannot be an unquoted string literal. @@ -356,12 +372,12 @@ func (s *Suite) Test_MkParser_isPkgbasePart(c *check.C) { test("_client", false) // The combination foo-_client looks strange. } -func (s *Suite) Test_MkParser_Dependency(c *check.C) { +func (s *Suite) Test_MkParser_DependencyPattern(c *check.C) { t := s.Init(c) testRest := func(pattern string, expected DependencyPattern, rest string) { parser := NewMkParser(nil, pattern) - dp := parser.Dependency() + dp := parser.DependencyPattern() if c.Check(dp, check.NotNil) { t.CheckEquals(*dp, expected) t.CheckEquals(parser.Rest(), rest) @@ -370,7 +386,7 @@ func (s *Suite) Test_MkParser_Dependency(c *check.C) { testNil := func(pattern string) { parser := NewMkParser(nil, pattern) - dp := parser.Dependency() + dp := parser.DependencyPattern() if c.Check(dp, check.IsNil) { t.CheckEquals(parser.Rest(), pattern) } @@ -488,17 +504,19 @@ func (s *Suite) Test_MkCondWalker_Walk(c *check.C) { events = append(events, sprintf("%14s %s", name, strings.Join(args, ", "))) } - // TODO: Add callbacks for And, Or, Not if needed. - // Especially Not(Empty(VARNAME)) should be an interesting case. + // XXX: Add callbacks for And and Or if needed. mkline.Cond().Walk(&MkCondCallback{ - Defined: func(varname string) { + func(cond *MkCond) { + addEvent("not") + }, + func(varname string) { addEvent("defined", varname) }, - Empty: func(varuse *MkVarUse) { + func(varuse *MkVarUse) { addEvent("empty", varuseStr(varuse)) }, - Compare: func(left *MkCondTerm, op string, right *MkCondTerm) { + func(left *MkCondTerm, op string, right *MkCondTerm) { assert(left.Var != nil) switch { case right.Var != nil: @@ -509,13 +527,16 @@ func (s *Suite) Test_MkCondWalker_Walk(c *check.C) { addEvent("compareVarStr", varuseStr(left.Var), right.Str) } }, - Call: func(name string, arg string) { + func(name string, arg string) { addEvent("call", name, arg) }, - Var: func(varuse *MkVarUse) { + func(cond *MkCond) { + addEvent("paren") + }, + func(varuse *MkVarUse) { addEvent("var", varuseStr(varuse)) }, - VarUse: func(varuse *MkVarUse) { + func(varuse *MkVarUse) { addEvent("varUse", varuseStr(varuse)) }}) @@ -533,9 +554,13 @@ func (s *Suite) Test_MkCondWalker_Walk(c *check.C) { " varUse NUM", " defined VAR", " varUse VAR", + " not ", " call exists, file.mk", " call exists, ${FILE}", " varUse FILE", + " paren ", + " paren ", + " paren ", " var NONEMPTY", " varUse NONEMPTY"}) } diff --git a/pkgtools/pkglint/files/mkvarusechecker_test.go b/pkgtools/pkglint/files/mkvarusechecker_test.go index 43c0e063ff9..85483bd5b12 100644 --- a/pkgtools/pkglint/files/mkvarusechecker_test.go +++ b/pkgtools/pkglint/files/mkvarusechecker_test.go @@ -754,7 +754,7 @@ func (s *Suite) Test_MkVarUseChecker_warnPermissions__not_directly_and_no_altern t.CheckEquals(toolDependsType.AlternativeFiles(aclpUseLoadtime), "") apiDependsType := G.Pkgsrc.VariableType(nil, "BUILDLINK_API_DEPENDS.*") - t.CheckEquals(apiDependsType.String(), "Dependency (list, package-settable)") + t.CheckEquals(apiDependsType.String(), "DependencyPattern (list, package-settable)") t.CheckEquals(apiDependsType.AlternativeFiles(aclpUse), "") t.CheckEquals(apiDependsType.AlternativeFiles(aclpUseLoadtime), "buildlink3.mk or builtin.mk only") diff --git a/pkgtools/pkglint/files/package.go b/pkgtools/pkglint/files/package.go index 91566790f49..5ac4728c89c 100644 --- a/pkgtools/pkglint/files/package.go +++ b/pkgtools/pkglint/files/package.go @@ -395,7 +395,8 @@ func (pkg *Package) loadIncluded(mkline *MkLine, includingFile CurrPath) (includ func (pkg *Package) resolveIncludedFile(mkline *MkLine, includingFilename CurrPath) RelPath { // TODO: Try to combine resolveVariableRefs and ResolveVarsInRelativePath. - resolved := mkline.ResolveVarsInRelativePath(mkline.IncludedFile(), pkg) + // TODO: Not every relative path is relative to the package directory. + resolved := mkline.ResolveVarsInRelativePath(NewPackagePath(mkline.IncludedFile()), pkg) includedText := resolveVariableRefs(resolved.String(), nil, pkg) includedFile := NewRelPathString(includedText) if containsVarUse(includedText) { diff --git a/pkgtools/pkglint/files/path.go b/pkgtools/pkglint/files/path.go index 71289421093..92399a216b5 100644 --- a/pkgtools/pkglint/files/path.go +++ b/pkgtools/pkglint/files/path.go @@ -421,8 +421,28 @@ func (p PackagePath) JoinNoClean(other RelPath) PackagePath { return NewPackagePathString(p.AsPath().JoinNoClean(other).String()) } +func (p PackagePath) CleanPath() PackagePath { + return NewPackagePathString(p.AsPath().CleanPath().String()) +} + func (p PackagePath) IsEmpty() bool { return p.AsPath().IsEmpty() } +func (p PackagePath) HasPrefixPath(sub Path) bool { + return p.AsPath().HasPrefixPath(sub) +} + +func (p PackagePath) ContainsPath(sub Path) bool { + return p.AsPath().ContainsPath(sub) +} + +func (p PackagePath) ContainsText(contained string) bool { + return p.AsPath().ContainsText(contained) +} + +func (p PackagePath) Replace(from, to string) PackagePath { + return NewPackagePathString(strings.Replace(string(p), from, to, -1)) +} + // RelPath is a path that is relative to some base directory that is not // further specified. type RelPath string diff --git a/pkgtools/pkglint/files/path_test.go b/pkgtools/pkglint/files/path_test.go index ddcfed4d84c..7b18a77a147 100644 --- a/pkgtools/pkglint/files/path_test.go +++ b/pkgtools/pkglint/files/path_test.go @@ -1234,6 +1234,16 @@ func (s *Suite) Test_PackagePath_JoinNoClean(c *check.C) { "../../category/package/patches/patch-aa") } +func (s *Suite) Test_PackagePath_CleanPath(c *check.C) { + t := s.Init(c) + + test := func(p PackagePath, cleaned PackagePath) { + t.CheckEquals(p.CleanPath(), cleaned) + } + + test("a/b/../../c/d/../.././e/../f", "a/b/../../e/../f") +} + func (s *Suite) Test_PackagePath_IsEmpty(c *check.C) { t := s.Init(c) @@ -1245,6 +1255,55 @@ func (s *Suite) Test_PackagePath_IsEmpty(c *check.C) { test(".", false) } +func (s *Suite) Test_PackagePath_HasPrefixPath(c *check.C) { + t := s.Init(c) + + test := func(p PackagePath, sub Path, hasPrefix bool) { + t.CheckEquals(p.HasPrefixPath(sub), hasPrefix) + } + + test("/root/subdir", "subdir", false) + test("/root/subdir", "/root", true) + test("/root/subdir", "/r", false) +} + +func (s *Suite) Test_PackagePath_ContainsPath(c *check.C) { + t := s.Init(c) + + test := func(p PackagePath, sub Path, hasPrefix bool) { + t.CheckEquals(p.ContainsPath(sub), hasPrefix) + } + + test("/root/subdir", "subdir", true) + test("/root/subdir", "/root", true) + test("/root/subdir", "/r", false) +} + +func (s *Suite) Test_PackagePath_ContainsText(c *check.C) { + t := s.Init(c) + + test := func(p PackagePath, sub string, hasPrefix bool) { + t.CheckEquals(p.ContainsText(sub), hasPrefix) + } + + test("/root/subdir", "subdir", true) + test("/root/subdir", "/root", true) + test("/root/subdir", "/r", true) + test("/root/subdir", "t//sub", false) +} + +func (s *Suite) Test_PackagePath_Replace(c *check.C) { + t := s.Init(c) + + test := func(p PackagePath, from, to string, result PackagePath) { + t.CheckEquals(p.Replace(from, to), result) + } + + test("dir/file", "dir", "other", "other/file") + test("dir/file", "r", "sk", "disk/file") + test("aaa/file", "a", "sub/", "sub/sub/sub//file") +} + func (s *Suite) Test_NewRelPath(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/vardefs.go b/pkgtools/pkglint/files/vardefs.go index 212bbf96cff..6e2f6952460 100644 --- a/pkgtools/pkglint/files/vardefs.go +++ b/pkgtools/pkglint/files/vardefs.go @@ -886,11 +886,11 @@ func (reg *VarTypeRegistry) Init(src *Pkgsrc) { reg.pkglistrat("BROKEN_ON_PLATFORM", BtMachinePlatformPattern) reg.syslist("BSD_MAKE_ENV", BtShellWord) // TODO: Align the permissions of the various BUILDLINK_*.* variables with each other. - reg.acllist("BUILDLINK_ABI_DEPENDS.*", BtDependency, + reg.acllist("BUILDLINK_ABI_DEPENDS.*", BtDependencyPattern, PackageSettable, "buildlink3.mk, builtin.mk: append, use-loadtime", "*: append") - reg.acllist("BUILDLINK_API_DEPENDS.*", BtDependency, + reg.acllist("BUILDLINK_API_DEPENDS.*", BtDependencyPattern, PackageSettable, "buildlink3.mk, builtin.mk: append, use-loadtime", "*: append") @@ -1066,7 +1066,7 @@ func (reg *VarTypeRegistry) Init(src *Pkgsrc) { reg.pkglist("CONFIG_STATUS_OVERRIDE", BtPathPattern) reg.pkg("CONFIG_SHELL", BtPathname) reg.pkglist("CONFIG_SUB_OVERRIDE", BtPathPattern) - reg.pkglist("CONFLICTS", BtDependency) + reg.pkglist("CONFLICTS", BtDependencyPattern) reg.pkgappend("CONF_FILES", BtConfFiles) reg.pkg("CONF_FILES_MODE", enum("0644 0640 0600 0400")) reg.pkglist("CONF_FILES_PERMS", BtPerms) @@ -1145,7 +1145,7 @@ func (reg *VarTypeRegistry) Init(src *Pkgsrc) { reg.usr("EMUL_PLATFORM", BtEmulPlatform) reg.pkglist("EMUL_PLATFORMS", BtEmulPlatform) reg.usrlist("EMUL_PREFER", BtEmulPlatform) - reg.pkglist("EMUL_REQD", BtDependency) + reg.pkglist("EMUL_REQD", BtDependencyPattern) reg.usr("EMUL_TYPE.*", enum("native builtin suse suse-10.0 suse-12.1 suse-13.1")) reg.sys("ERROR_CAT", BtShellCommand) reg.sys("ERROR_MSG", BtShellCommand) @@ -1673,7 +1673,7 @@ func (reg *VarTypeRegistry) Init(src *Pkgsrc) { reg.pkgbl3("SUBST_STAGE.*", BtStage) reg.pkglistbl3("SUBST_VARS.*", BtVariableName) - reg.pkglist("SUPERSEDES", BtDependency) + reg.pkglist("SUPERSEDES", BtDependencyPattern) reg.pkglist("TEST_DEPENDS", BtDependencyWithPath) reg.pkglist("TEST_DIRS", BtWrksrcSubdirectory) reg.pkglist("TEST_ENV", BtShellWord) diff --git a/pkgtools/pkglint/files/vartype.go b/pkgtools/pkglint/files/vartype.go index e520b34d957..897eb0ba154 100644 --- a/pkgtools/pkglint/files/vartype.go +++ b/pkgtools/pkglint/files/vartype.go @@ -411,7 +411,7 @@ var ( BtCFlag = &BasicType{"CFlag", (*VartypeCheck).CFlag} BtComment = &BasicType{"Comment", (*VartypeCheck).Comment} BtConfFiles = &BasicType{"ConfFiles", (*VartypeCheck).ConfFiles} - BtDependency = &BasicType{"Dependency", (*VartypeCheck).Dependency} + BtDependencyPattern = &BasicType{"DependencyPattern", (*VartypeCheck).DependencyPattern} BtDependencyWithPath = &BasicType{"DependencyWithPath", (*VartypeCheck).DependencyWithPath} BtDistSuffix = &BasicType{"DistSuffix", (*VartypeCheck).DistSuffix} BtEmulPlatform = &BasicType{"EmulPlatform", (*VartypeCheck).EmulPlatform} diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go index 88acccc0a33..e2293c1e336 100644 --- a/pkgtools/pkglint/files/vartypecheck.go +++ b/pkgtools/pkglint/files/vartypecheck.go @@ -289,11 +289,11 @@ func (cv *VartypeCheck) ConfFiles() { } } -func (cv *VartypeCheck) Dependency() { +func (cv *VartypeCheck) DependencyPattern() { value := cv.Value parser := NewMkParser(nil, value) - deppat := parser.Dependency() + deppat := parser.DependencyPattern() rest := parser.Rest() if deppat != nil && deppat.Wildcard == "" && (rest == "{,nb*}" || rest == "{,nb[0-9]*}") { @@ -384,8 +384,9 @@ func (cv *VartypeCheck) DependencyWithPath() { parts := cv.MkLine.ValueSplit(value, ":") if len(parts) == 2 { pattern := parts[0] - relpath := NewRelPathString(parts[1]) - pathParts := relpath.Parts() + packagePath := NewPackagePathString(parts[1]) + relPath := packagePath.AsRelPath() + pathParts := relPath.Parts() pkg := pathParts[len(pathParts)-1] if len(pathParts) >= 2 && pathParts[0] == ".." && pathParts[1] != ".." { @@ -393,8 +394,9 @@ func (cv *VartypeCheck) DependencyWithPath() { cv.MkLine.ExplainRelativeDirs() } - if !containsVarUse(relpath.String()) { - MkLineChecker{cv.MkLines, cv.MkLine}.CheckRelativePkgdir(relpath) + if !containsVarUse(packagePath.String()) { + ck := MkLineChecker{cv.MkLines, cv.MkLine} + ck.CheckRelativePkgdir(relPath, packagePath) } switch pkg { @@ -406,7 +408,7 @@ func (cv *VartypeCheck) DependencyWithPath() { cv.Warnf("Please use USE_TOOLS+=gmake instead of this dependency.") } - cv.WithValue(pattern).Dependency() + cv.WithValue(pattern).DependencyPattern() return } @@ -630,7 +632,11 @@ func (cv *VartypeCheck) GccReqd() { func (cv *VartypeCheck) Homepage() { cv.URL() + cv.homepageBasedOnMasterSites() + cv.homepageHttp() +} +func (cv *VartypeCheck) homepageBasedOnMasterSites() { m, wrong, sitename, subdir := match3(cv.Value, `^(\$\{(MASTER_SITE\w+)(?::=([\w\-/]+))?\})`) if !m { return @@ -670,6 +676,70 @@ func (cv *VartypeCheck) Homepage() { fix.Apply() } +func (cv *VartypeCheck) homepageHttp() { + m, host := match1(cv.Value, `http://([A-Za-z0-9-.]+)`) + if !m { + return + } + + rationale := cv.MkLine.Rationale() + if containsWord(rationale, "http") || containsWord(rationale, "https") { + return + } + + hasAnySuffix := func(s string, suffixes ...string) bool { + for _, suffix := range suffixes { + if hasSuffix(s, suffix) { + dotIndex := len(s) - len(suffix) + if dotIndex == 0 || s[dotIndex-1] == '.' { + return true + } + } + } + return false + } + + // Don't warn about sites that don't support https at all. + if hasAnySuffix(host, + "www.gnustep.org", // 2020-01-18 + "aspell.net", // 2020-01-18 + ) { + return + } + + supportsHttps := hasAnySuffix(host, + "apache.org", + "archive.org", + "ctan.org", + "freedesktop.org", + "github.com", + "github.io", + "gnome.org", + "gnu.org", + "kde.org", + "kldp.net", + "linuxfoundation.org", + "NetBSD.org", + "nongnu.org", + "sf.net", + "sourceforge.net", + "tryton.org", + "tug.org") + + fix := cv.Autofix() + fix.Warnf("HOMEPAGE should use https instead of http.") + if supportsHttps { + fix.Replace("http", "https") + } + fix.Explain( + "To provide secure communication by default,", + "the HOMEPAGE URL should use the https protocol if available.", + "", + "If the HOMEPAGE really does not support https,", + "add a comment near the HOMEPAGE variable stating this clearly.") + fix.Apply() +} + // Identifier checks for valid identifiers in various contexts, limiting the // valid characters to A-Za-z0-9_. func (cv *VartypeCheck) IdentifierDirect() { @@ -1148,17 +1218,21 @@ func (cv *VartypeCheck) RPkgVer() { } } -// RelativePkgDir refers to a package directory, e.g. ../../category/pkgbase. +// RelativePkgDir refers from one package directory to another package +// directory, e.g. ../../category/pkgbase. func (cv *VartypeCheck) RelativePkgDir() { if NewPath(cv.Value).IsAbs() { cv.Errorf("The path %q must be relative.", cv.Value) return } - MkLineChecker{cv.MkLines, cv.MkLine}.CheckRelativePkgdir(NewRelPathString(cv.Value)) + ck := MkLineChecker{cv.MkLines, cv.MkLine} + packagePath := NewPackagePathString(cv.Value) + ck.CheckRelativePkgdir(packagePath.AsRelPath(), packagePath) } -// RelativePkgPath refers to a file or directory, e.g. ../../category/pkgbase, +// RelativePkgPath refers from one package directory to another file +// or directory, e.g. ../../category/pkgbase, // ../../category/pkgbase/Makefile. // // See RelativePkgDir, which requires a directory, not a file. @@ -1168,7 +1242,9 @@ func (cv *VartypeCheck) RelativePkgPath() { return } - MkLineChecker{cv.MkLines, cv.MkLine}.CheckRelativePath(NewRelPathString(cv.Value), true) + packagePath := NewPackagePathString(cv.Value) + ck := MkLineChecker{cv.MkLines, cv.MkLine} + ck.CheckRelativePath(packagePath, packagePath.AsRelPath(), true) } func (cv *VartypeCheck) Restricted() { diff --git a/pkgtools/pkglint/files/vartypecheck_test.go b/pkgtools/pkglint/files/vartypecheck_test.go index 73792175f76..58e01a2eef5 100644 --- a/pkgtools/pkglint/files/vartypecheck_test.go +++ b/pkgtools/pkglint/files/vartypecheck_test.go @@ -362,9 +362,9 @@ func (s *Suite) Test_VartypeCheck_ConfFiles(c *check.C) { "WARN: filename.mk:5: The destination file \"/etc/bootrc\" should start with a variable reference.") } -// See Test_MkParser_Dependency. -func (s *Suite) Test_VartypeCheck_Dependency(c *check.C) { - vt := NewVartypeCheckTester(s.Init(c), BtDependency) +// See Test_MkParser_DependencyPattern. +func (s *Suite) Test_VartypeCheck_DependencyPattern(c *check.C) { + vt := NewVartypeCheckTester(s.Init(c), BtDependencyPattern) vt.Varname("CONFLICTS") vt.Op(opAssignAppend) @@ -919,6 +919,7 @@ func (s *Suite) Test_VartypeCheck_Homepage(c *check.C) { "${MASTER_SITES}") vt.Output( + "WARN: filename.mk:1: HOMEPAGE should use https instead of http.", "WARN: filename.mk:3: HOMEPAGE should not be defined in terms of MASTER_SITEs.") pkg := NewPackage(t.File("category/package")) @@ -972,6 +973,27 @@ func (s *Suite) Test_VartypeCheck_Homepage(c *check.C) { "WARN: filename.mk:41: HOMEPAGE should not be defined in terms of MASTER_SITEs.") } +func (s *Suite) Test_VartypeCheck_Homepage__http(c *check.C) { + t := s.Init(c) + vt := NewVartypeCheckTester(t, BtHomepage) + + vt.Varname("HOMEPAGE") + vt.Values( + "http://www.gnustep.org/", + "http://www.pkgsrc.org/", + "http://project.sourceforge.net/", + "http://sf.net/p/project/", + "http://example.org/ # doesn't support https", + "http://example.org/ # only supports http", + "http://asf.net/") + + vt.Output( + "WARN: filename.mk:2: HOMEPAGE should use https instead of http.", + "WARN: filename.mk:3: HOMEPAGE should use https instead of http.", + "WARN: filename.mk:4: HOMEPAGE should use https instead of http.", + "WARN: filename.mk:7: HOMEPAGE should use https instead of http.") +} + func (s *Suite) Test_VartypeCheck_IdentifierDirect(c *check.C) { t := s.Init(c) vt := NewVartypeCheckTester(t, BtIdentifierDirect) @@ -1651,6 +1673,20 @@ func (s *Suite) Test_VartypeCheck_RelativePkgPath(c *check.C) { "ERROR: filename.mk:4: Relative path \"invalid\" does not exist.", "ERROR: filename.mk:5: Relative path \"../../invalid/relative\" does not exist.", "ERROR: filename.mk:6: The path \"/absolute\" must be relative.") + + vt.File("../../mk/infra.mk") + vt.Values( + "../package", + "../../category/other-package", + "../../missing/package", + "../../category/missing") + + vt.Output( + "ERROR: ../../mk/infra.mk:1: Relative path \"../package\" does not exist.", + // FIXME: This directory _does_ exist. + "ERROR: ../../mk/infra.mk:2: Relative path \"../../category/other-package\" does not exist.", + "ERROR: ../../mk/infra.mk:3: Relative path \"../../missing/package\" does not exist.", + "ERROR: ../../mk/infra.mk:4: Relative path \"../../category/missing\" does not exist.") } func (s *Suite) Test_VartypeCheck_Restricted(c *check.C) { @@ -2322,6 +2358,7 @@ func (vt *VartypeCheckTester) Values(values ...string) { line := vt.tester.NewLine(vt.filename, vt.lineno, text) mklines := NewMkLines(NewLines(vt.filename, []*Line{line}), vt.pkg, nil) + mklines.collectRationale() vt.lineno++ mklines.ForEach(func(mkline *MkLine) { test(mklines, mkline, value) }) |