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