diff options
| author | rillig <rillig@pkgsrc.org> | 2018-10-09 19:12:13 +0000 |
|---|---|---|
| committer | rillig <rillig@pkgsrc.org> | 2018-10-09 19:12:13 +0000 |
| commit | d89baa507057514c8df4b92d3a8a9b80c36e6b6d (patch) | |
| tree | 70b7b78a7c3aa5b91381fc2bca5e8049d6190b56 /pkgtools | |
| parent | af862a3b4d2479c8b8ef211e17b75f35efc741ec (diff) | |
| download | pkgsrc-d89baa507057514c8df4b92d3a8a9b80c36e6b6d.tar.gz | |
pkgtools/pkglint: update to 5.6.4
Changes since 5.6.3:
* Allow += for COMMENT
* Sync variable type definitions with reality
* Fix check for "used but not defined" variables. This check had been
broken since pkgtools/pkglint/files/pkglint.pl r1.776 from 2008-10-18
(3cd071958e63), which missed its 10-year anniversary by just 9 days.
After fixing this check, pkglint produces about 800 new warnings
spread all over pkgsrc, most of which are real typos.
* Detect used variables also in .if and .elif conditions. This is
closely related to the above fix and reduces the number of "defined
but not used" variables, while at the same time producing new warnings
because these variables are used at load time, where some of these
variables are not yet defined.
* Detect variables for which pkglint doesn't know the exact data type
by scanning all files under mk/ at startup. Currently there are about
470 of these variables. No "used but not defined" warnings are issued
for these variables anymore.
* To speed up pkglint when checking the whole pkgsrc tree at once, the
most often needed files are cached to reduce IO load. The checks for
USE_TOOLS are optimized now since they were a major bottleneck.
Together with other performance improvements this makes pkglint about
50% faster when checking the whole pkgsrc tree including pkgsrc-wip.
Diffstat (limited to 'pkgtools')
44 files changed, 1494 insertions, 780 deletions
diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index f2703f86ec0..c0ae9c30ff4 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.549 2018/10/03 22:27:53 rillig Exp $ +# $NetBSD: Makefile,v 1.550 2018/10/09 19:12:13 rillig Exp $ -PKGNAME= pkglint-5.6.3 +PKGNAME= pkglint-5.6.4 DISTFILES= # none CATEGORIES= pkgtools diff --git a/pkgtools/pkglint/files/alternatives_test.go b/pkgtools/pkglint/files/alternatives_test.go index a7e47ddb594..32b8d7aed79 100644 --- a/pkgtools/pkglint/files/alternatives_test.go +++ b/pkgtools/pkglint/files/alternatives_test.go @@ -21,11 +21,7 @@ func (s *Suite) Test_CheckfileAlternatives__PLIST(c *check.C) { G.CheckDirent(".") - // TODO: Remove redundant diagnostics. t.CheckOutputLines( - "NOTE: ALTERNATIVES:1: @PREFIX@/ can be omitted from the file name.", - "NOTE: ALTERNATIVES:2: @PREFIX@/ can be omitted from the file name.", - "ERROR: ALTERNATIVES:5: Invalid ALTERNATIVES line \"invalid\".", "ERROR: ALTERNATIVES:1: Alternative implementation \"@PREFIX@/sbin/sendmail.postfix@POSTFIXVER@\" must appear in the PLIST as \"sbin/sendmail.postfix${POSTFIXVER}\".", "NOTE: ALTERNATIVES:1: @PREFIX@/ can be omitted from the file name.", "NOTE: ALTERNATIVES:2: @PREFIX@/ can be omitted from the file name.", diff --git a/pkgtools/pkglint/files/autofix.go b/pkgtools/pkglint/files/autofix.go index beb636c1b56..32352682a94 100644 --- a/pkgtools/pkglint/files/autofix.go +++ b/pkgtools/pkglint/files/autofix.go @@ -128,7 +128,8 @@ func (fix *Autofix) Realign(mkline MkLine, newWidth int) { { // Interpreting the continuation marker as variable value // is cheating, but works well. - m, _, _, _, _, valueAlign, value, _, _ := MatchVarassign(mkline.raw[0].orignl) + text := strings.TrimSuffix(mkline.raw[0].orignl, "\n") + m, _, _, _, _, valueAlign, value, _, _ := MatchVarassign(text) if m && value != "\\" { oldWidth = tabWidth(valueAlign) } diff --git a/pkgtools/pkglint/files/buildlink3_test.go b/pkgtools/pkglint/files/buildlink3_test.go index 3b4fe3df1bb..c87cd151dee 100644 --- a/pkgtools/pkglint/files/buildlink3_test.go +++ b/pkgtools/pkglint/files/buildlink3_test.go @@ -343,6 +343,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk__indentation(c *check.C) { // No warning about the indentation of the .include lines. t.CheckOutputLines( + "WARN: ~/buildlink3.mk:3: VAAPI_AVAILABLE is used but not defined.", "ERROR: ~/buildlink3.mk:11: \"multimedia/libva\" does not exist.", "ERROR: ~/buildlink3.mk:11: There is no package in \"multimedia/libva\".", "ERROR: ~/buildlink3.mk:13: \"x11/libX11/buildlink3.mk\" does not exist.", diff --git a/pkgtools/pkglint/files/check_test.go b/pkgtools/pkglint/files/check_test.go index d8e8eef7366..bca5370f9a8 100644 --- a/pkgtools/pkglint/files/check_test.go +++ b/pkgtools/pkglint/files/check_test.go @@ -161,17 +161,8 @@ func (t *Tester) SetupOption(name, description string) { G.Pkgsrc.PkgOptions[name] = description } -func (t *Tester) SetupTool(name, varname string) *Tool { - tools := G.Pkgsrc.Tools - return tools.Define(name, varname, dummyMkLine) -} - -// SetupToolUsable registers a tool and immediately makes it usable, -// as if the tool were predefined globally in pkgsrc. -func (t *Tester) SetupToolUsable(name, varname string) *Tool { - tool := t.SetupTool(name, varname) - tool.SetValidity(AtRunTime, G.Pkgsrc.Tools.TraceName) - return tool +func (t *Tester) SetupTool(name, varname string, validity Validity) *Tool { + return G.Pkgsrc.Tools.defTool(name, varname, false, validity) } // SetupFileLines creates a temporary file and writes the given lines to it. @@ -247,8 +238,9 @@ func (t *Tester) SetupCategory(name string) { } } -// SetupPackage sets up all files for a package so that it does not produce -// any warnings. +// SetupPackage sets up all files for a package (including the pkgsrc +// infrastructure) so that it does not produce any warnings. After calling +// this method, individual files can be overwritten as necessary. // // The given makefileLines start in line 20. Except if they are variable // definitions for already existing variables, then they replace that line. diff --git a/pkgtools/pkglint/files/files.go b/pkgtools/pkglint/files/files.go index 7c8dc228e0f..f71b803b45b 100644 --- a/pkgtools/pkglint/files/files.go +++ b/pkgtools/pkglint/files/files.go @@ -48,7 +48,9 @@ func Load(fileName string, options LoadOptions) []Line { } result := convertToLogicalLines(fileName, rawText, options&Makefile != 0) - G.fileCache.Put(fileName, options, result) + if hasSuffix(fileName, ".mk") { + G.fileCache.Put(fileName, options, result) + } return result } diff --git a/pkgtools/pkglint/files/getopt/getopt.go b/pkgtools/pkglint/files/getopt/getopt.go index e0865ee54d3..e7e1cd8d796 100644 --- a/pkgtools/pkglint/files/getopt/getopt.go +++ b/pkgtools/pkglint/files/getopt/getopt.go @@ -18,6 +18,14 @@ func NewOptions() *Options { return new(Options) } +// AddFlagGroup adds an option that takes multiple flag values. +// +// Example: +// var extra bool +// +// opts := NewOptions() +// warnings := opts.AddFlagGroup('W', "warnings", "warning,...", "Enable the given warnings") +// warnings.AddFlagVar("extra", &extra, false, "Print extra warnings") func (o *Options) AddFlagGroup(shortName rune, longName, argDescription, description string) *FlagGroup { grp := new(FlagGroup) opt := &option{shortName, longName, argDescription, description, grp} @@ -37,6 +45,8 @@ func (o *Options) AddStrList(shortName rune, longName string, plist *[]string, d o.options = append(o.options, opt) } +// Parse extracts the command line options from the given arguments. +// args[0] is the program name, as in os.Args. func (o *Options) Parse(args []string) (remainingArgs []string, err error) { var skip int for i := 1; i < len(args) && err == nil; i++ { @@ -108,20 +118,24 @@ func (o *Options) handleLongOption(args []string, i int, opt *option, argval *st } return 0, nil case *[]string: - if argval != nil { + switch { + case argval != nil: *data = append(*data, *argval) return 0, nil - } else if i+1 < len(args) { + case i+1 < len(args): *data = append(*data, args[i+1]) return 1, nil - } else { + default: return 0, optErr("option requires an argument: --" + opt.longName) } case *FlagGroup: - if argval == nil { - return 1, data.parse("--"+opt.longName+"=", args[i+1]) - } else { + switch { + case argval != nil: return 0, data.parse("--"+opt.longName+"=", *argval) + case i+1 < len(args): + return 1, data.parse("--"+opt.longName+"=", args[i+1]) + default: + return 0, optErr("option requires an argument: --" + opt.longName) } } panic("getopt: unknown option type") @@ -139,23 +153,25 @@ optchar: case *[]string: argarg := optchars[ai+utf8.RuneLen(optchar):] - if argarg != "" { + switch { + case argarg != "": *data = append(*data, argarg) return 0, nil - } else if i+1 < len(args) { + case i+1 < len(args): *data = append(*data, args[i+1]) return 1, nil - } else { + default: return 0, optErr("option requires an argument: -" + string([]rune{optchar})) } case *FlagGroup: argarg := optchars[ai+utf8.RuneLen(optchar):] - if argarg != "" { + switch { + case argarg != "": return 0, data.parse(string([]rune{'-', optchar}), argarg) - } else if i+1 < len(args) { + case i+1 < len(args): return 1, data.parse(string([]rune{'-', optchar}), args[i+1]) - } else { + default: return 0, optErr("option requires an argument: -" + string([]rune{optchar})) } } diff --git a/pkgtools/pkglint/files/getopt/getopt_test.go b/pkgtools/pkglint/files/getopt/getopt_test.go index 091a41368a5..f6b07fd7dd9 100644 --- a/pkgtools/pkglint/files/getopt/getopt_test.go +++ b/pkgtools/pkglint/files/getopt/getopt_test.go @@ -2,6 +2,7 @@ package getopt import ( "gopkg.in/check.v1" + "strings" "testing" ) @@ -177,3 +178,100 @@ func (s *Suite) Test_Options_Parse__long_flags(c *check.C) { c.Check(iflag, check.Equals, true) c.Check(err, check.ErrorMatches, `^progname: invalid argument for option --jflag$`) } + +func (s *Suite) Test_Options_handleLongOption__flag_group_without_argument(c *check.C) { + var extra bool + + opts := NewOptions() + group := opts.AddFlagGroup('W', "warnings", "warning,...", "Print selected warnings") + group.AddFlagVar("extra", &extra, false, "Print extra warnings") + + args, err := opts.Parse([]string{"progname", "--warnings"}) + + c.Check(args, check.IsNil) + c.Check(err.Error(), check.Equals, "progname: option requires an argument: --warnings") + c.Check(extra, check.Equals, false) +} + +func (s *Suite) Test_Options_handleLongOption__flag_group_separate_argument(c *check.C) { + var extra bool + + opts := NewOptions() + group := opts.AddFlagGroup('W', "warnings", "warning,...", "Print selected warnings") + group.AddFlagVar("extra", &extra, false, "Print extra warnings") + + args, err := opts.Parse([]string{"progname", "--warnings", "extra,unknown"}) + + c.Check(args, check.IsNil) + c.Check(err.Error(), check.Equals, "progname: unknown option: --warnings=unknown") + c.Check(extra, check.Equals, true) +} + +func (s *Suite) Test_Options_handleLongOption__flag_group_negated(c *check.C) { + var extra bool + + opts := NewOptions() + group := opts.AddFlagGroup('W', "warnings", "warning,...", "Print selected warnings") + group.AddFlagVar("extra", &extra, true, "Print extra warnings") + + args, err := opts.Parse([]string{"progname", "--warnings", "all,no-extra"}) + + c.Check(args, check.IsNil) + c.Check(err, check.IsNil) + c.Check(extra, check.Equals, false) +} + +func (s *Suite) Test_Options_handleLongOption__internal_error(c *check.C) { + var extra bool + + opts := NewOptions() + group := opts.AddFlagGroup('W', "warnings", "warning,...", "Print selected warnings") + group.AddFlagVar("extra", &extra, false, "Print extra warnings") + opts.options[0].data = "unexpected value" + + c.Check( + func() { _, _ = opts.Parse([]string{"progname", "--warnings"}) }, + check.Panics, + "getopt: unknown option type") +} + +func (s *Suite) Test_Options_parseShortOptions__flag_group_separate_argument(c *check.C) { + var extra bool + + opts := NewOptions() + group := opts.AddFlagGroup('W', "warnings", "warning,...", "Print selected warnings") + group.AddFlagVar("extra", &extra, false, "Print extra warnings") + + args, err := opts.Parse([]string{"progname", "-W", "extra,unknown"}) + + c.Check(args, check.IsNil) + c.Check(err.Error(), check.Equals, "progname: unknown option: -Wunknown") + c.Check(extra, check.Equals, true) +} + +func (s *Suite) Test_Options_Help(c *check.C) { + var verbose, basic, extra bool + + opts := NewOptions() + opts.AddFlagVar('v', "verbose", &verbose, false, "Print a detailed log") + group := opts.AddFlagGroup('W', "warnings", "warning,...", "Print selected warnings") + group.AddFlagVar("basic", &basic, true, "Print basic warnings") + group.AddFlagVar("extra", &extra, false, "Print extra warnings") + + var out strings.Builder + opts.Help(&out, "progname [options] args") + + c.Check(out.String(), check.Equals, ""+ + "usage: progname [options] args\n"+ + "\n"+ + " -v, --verbose Print a detailed log\n"+ + " -W, --warnings=warning,... Print selected warnings\n"+ + "\n"+ + " Flags for -W, --warnings:\n"+ + " all all of the following\n"+ + " none none of the following\n"+ + " basic Print basic warnings (enabled)\n"+ + " extra Print extra warnings (disabled)\n"+ + "\n"+ + " (Prefix a flag with \"no-\" to disable it.)\n") +} diff --git a/pkgtools/pkglint/files/licenses/licenses.go b/pkgtools/pkglint/files/licenses/licenses.go index b81d2b156f7..317660292ba 100644 --- a/pkgtools/pkglint/files/licenses/licenses.go +++ b/pkgtools/pkglint/files/licenses/licenses.go @@ -20,7 +20,7 @@ type Condition struct { func Parse(licenses string, res *regex.Registry) *Condition { lexer := &licenseLexer{repl: textproc.NewPrefixReplacer(licenses, res)} result := liyyNewParser().Parse(lexer) - if result == 0 { + if result == 0 && lexer.repl.EOF() { return lexer.result } return nil @@ -64,7 +64,7 @@ type licenseLexer struct { func (lexer *licenseLexer) Lex(llval *liyySymType) int { repl := lexer.repl - repl.AdvanceHspace() + repl.SkipHspace() switch { case repl.EOF(): return 0 diff --git a/pkgtools/pkglint/files/licenses/licenses_test.go b/pkgtools/pkglint/files/licenses/licenses_test.go index 8e9b08455af..b3681a52e0a 100644 --- a/pkgtools/pkglint/files/licenses/licenses_test.go +++ b/pkgtools/pkglint/files/licenses/licenses_test.go @@ -51,6 +51,8 @@ func (s *Suite) Test_Parse(c *check.C) { c.Check(Parse("a AND b OR c AND d", &res).String(), check.Equals, "a MIXED b MIXED c MIXED d") c.Check(Parse("AND artistic", &res), check.IsNil) + + c.Check(Parse("invalid/character", &res), check.IsNil) } func (s *Suite) Test_Condition_String(c *check.C) { @@ -81,6 +83,32 @@ func (s *Suite) Test_Condition_String(c *check.C) { c.Check(mixed.String(), check.Equals, "a MIXED b MIXED c") } +func (s *Suite) Test_Walk(c *check.C) { + res := regex.NewRegistry() + + condition := Parse("(a OR b) AND (c AND d)", &res) + + var out []string + condition.Walk(func(condition *Condition) { + switch { + case condition.Name != "": + out = append(out, condition.Name) + case condition.Paren != nil: + out = append(out, "()") + case condition.And && condition.Or: + out = append(out, "MIXED") + case condition.And: + out = append(out, "AND") + case condition.Or: + out = append(out, "OR") + default: + panic("unexpected") + } + }) + + c.Check(out, check.DeepEquals, []string{"a", "b", "OR", "()", "c", "d", "AND", "()", "AND"}) +} + func NewName(name string) *Condition { return &Condition{Name: name} } diff --git a/pkgtools/pkglint/files/mkline.go b/pkgtools/pkglint/files/mkline.go index 37355504306..254c3f6b288 100644 --- a/pkgtools/pkglint/files/mkline.go +++ b/pkgtools/pkglint/files/mkline.go @@ -19,7 +19,8 @@ type MkLineImpl struct { Line data interface{} // One of the following mkLine* types } -type mkLineAssign struct { +type mkLineAssign = *mkLineAssignImpl +type mkLineAssignImpl struct { commented bool // Whether the whole variable assignment is commented out varname string // e.g. "HOMEPAGE", "SUBST_SED.perl" varcanon string // e.g. "HOMEPAGE", "SUBST_SED.*" @@ -34,14 +35,17 @@ type mkLineShell struct { } type mkLineComment struct{} type mkLineEmpty struct{} -type mkLineDirective struct { +type mkLineDirective = *mkLineDirectiveImpl +type mkLineDirectiveImpl struct { indent string directive string args string comment string elseLine MkLine // (filled in later) + cond MkCond // (filled in later, as needed) } -type mkLineInclude struct { +type mkLineInclude = *mkLineIncludeImpl +type mkLineIncludeImpl struct { mustExist bool sys bool indent string @@ -56,7 +60,7 @@ type mkLineDependency struct { func NewMkLine(line Line) *MkLineImpl { text := line.Text - if hasPrefix(text, " ") { + if hasPrefix(text, " ") && line.Basename != "bsd.buildlink3.mk" { line.Warnf("Makefile lines should not start with space characters.") Explain( "If you want this line to contain a shell program, use a tab", @@ -89,7 +93,7 @@ func NewMkLine(line Line) *MkLineImpl { value = strings.Replace(value, "\\#", "#", -1) varparam := varnameParam(varname) - return &MkLineImpl{line, mkLineAssign{ + return &MkLineImpl{line, &mkLineAssignImpl{ commented, varname, varnameCanon(varname), @@ -105,7 +109,7 @@ func NewMkLine(line Line) *MkLineImpl { return &MkLineImpl{line, mkLineShell{shellcmd}} } - trimmedText := strings.TrimSpace(text) + trimmedText := trimHspace(text) if strings.HasPrefix(trimmedText, "#") { return &MkLineImpl{line, mkLineComment{}} } @@ -115,15 +119,15 @@ func NewMkLine(line Line) *MkLineImpl { } if m, indent, directive, args, comment := matchMkDirective(text); m { - return &MkLineImpl{line, mkLineDirective{indent, directive, args, comment, nil}} + return &MkLineImpl{line, &mkLineDirectiveImpl{indent, directive, args, comment, nil, nil}} } if m, indent, directive, includefile := MatchMkInclude(text); m { - return &MkLineImpl{line, mkLineInclude{directive == "include", false, indent, includefile, ""}} + return &MkLineImpl{line, &mkLineIncludeImpl{directive == "include", false, indent, includefile, ""}} } if m, indent, directive, includefile := match3(text, `^\.(\s*)(s?include)\s+<([^>]+)>\s*(?:#.*)?$`); m { - return &MkLineImpl{line, mkLineInclude{directive == "include", true, indent, includefile, ""}} + return &MkLineImpl{line, &mkLineIncludeImpl{directive == "include", true, indent, includefile, ""}} } if m, targets, whitespace, sources := match3(text, `^([^\s:]+(?:\s*[^\s:]+)*)(\s*):\s*([^#]*?)(?:\s*#.*)?$`); m { @@ -247,6 +251,19 @@ func (mkline *MkLineImpl) Directive() string { return mkline.data.(mkLineDirecti // Args returns the arguments from an .if, .ifdef, .ifndef, .elif, .for, .undef. func (mkline *MkLineImpl) Args() string { return mkline.data.(mkLineDirective).args } +// Cond applies to an .if or .elif line and returns the parsed condition. +// +// If a parse error occurs, it is silently swallowed, returning a +// best-effort part of the condition, or even nil. +func (mkline *MkLineImpl) Cond() MkCond { + cond := mkline.data.(mkLineDirective).cond + if cond == nil { + cond = NewMkParser(mkline.Line, mkline.Args(), false).MkCond() + mkline.data.(mkLineDirective).cond = cond + } + return cond +} + // DirectiveComment is the trailing end-of-line comment, typically at a deeply nested .endif or .endfor. func (mkline *MkLineImpl) DirectiveComment() string { return mkline.data.(mkLineDirective).comment } func (mkline *MkLineImpl) HasElseBranch() bool { return mkline.data.(mkLineDirective).elseLine != nil } @@ -262,9 +279,9 @@ func (mkline *MkLineImpl) IncludeFile() string { return mkline.data.(mkLineInclu func (mkline *MkLineImpl) Targets() string { return mkline.data.(mkLineDependency).targets } func (mkline *MkLineImpl) Sources() string { return mkline.data.(mkLineDependency).sources } -// ConditionalVars is a space-separated list of those variable names -// on which the inclusion depends. It is initialized later, -// step by step, when parsing other lines +// ConditionalVars applies to .include lines and is a space-separated +// list of those variable names on which the inclusion depends. +// It is initialized later, step by step, when parsing other lines. func (mkline *MkLineImpl) ConditionalVars() string { return mkline.data.(mkLineInclude).conditionalVars } func (mkline *MkLineImpl) SetConditionalVars(varnames string) { include := mkline.data.(mkLineInclude) @@ -306,7 +323,7 @@ func (mkline *MkLineImpl) ValueSplit(value string, separator string) []string { if token.Varuse == nil && contains(token.Text, separator) { var subs []string if separator == "" { - subs = splitOnSpace(token.Text) + subs = fields(token.Text) } else { subs = strings.Split(token.Text, separator) } @@ -324,15 +341,13 @@ func (mkline *MkLineImpl) ValueTokens() []*MkToken { } func (mkline *MkLineImpl) WithoutMakeVariables(value string) string { - valueNovar := value - for { - // TODO: properly parse nested variables - replaced := replaceFirst(valueNovar, `\$\{[^{}]*\}`, "") - if replaced == valueNovar { - return valueNovar + valueNovar := "" + for _, token := range NewMkParser(nil, value, false).MkTokens() { + if token.Varuse == nil { + valueNovar += token.Text } - valueNovar = replaced } + return valueNovar } func (mkline *MkLineImpl) ResolveVarsInRelativePath(relativePath string, adjustDepth bool) string { @@ -386,21 +401,8 @@ func (mkline *MkLineImpl) ResolveVarsInRelativePath(relativePath string, adjustD func (ind *Indentation) RememberUsedVariables(cond MkCond) { NewMkCondWalker().Walk(cond, &MkCondCallback{ - Defined: func(varname string) { - ind.AddVar(varname) - }, - Empty: func(varuse *MkVarUse) { + VarUse: func(varuse *MkVarUse) { ind.AddVar(varuse.varname) - }, - CompareVarNum: func(varuse *MkVarUse, op string, num string) { - ind.AddVar(varuse.varname) - }, - CompareVarStr: func(varuse *MkVarUse, op string, str string) { - ind.AddVar(varuse.varname) - }, - CompareVarVar: func(left *MkVarUse, op string, right *MkVarUse) { - ind.AddVar(left.varname) - ind.AddVar(right.varname) }}) } @@ -426,15 +428,20 @@ func matchMkDirective(text string) (m bool, indent, directive, args, comment str indentEnd := i directiveStart := i - for i < n && 'a' <= text[i] && text[i] <= 'z' { + for i < n && ('a' <= text[i] && text[i] <= 'z' || text[i] == '-') { i++ } directiveEnd := i directive = text[directiveStart:directiveEnd] switch directive { - case "if", "ifdef", "ifndef", "else", "elif", "endif", "for", "endfor", "undef": + case "if", "else", "elif", "endif", + "ifdef", "ifndef", + "for", "endfor", "undef", + "error", "warning", "info", + "export", "export-env", "unexport", "unexport-env": break default: + // Intentionally not supported are: ifmake ifnmake elifdef elifndef elifmake elifnmake. return } @@ -483,7 +490,7 @@ func (mkline *MkLineImpl) VariableNeedsQuoting(varname string, vartype *Vartype, defer trace.Call(varname, vartype, vuc, trace.Result(&needsQuoting))() } - if vartype == nil || vuc.vartype == nil { + if vartype == nil || vuc.vartype == nil || vartype.basicType == BtUnknown { return nqDontKnow } @@ -581,67 +588,63 @@ func (mkline *MkLineImpl) VariableNeedsQuoting(varname string, vartype *Vartype, return nqDontKnow } -// TODO: merge with determineUsedVariables -func (mkline *MkLineImpl) ExtractUsedVariables(text string) []string { - re := G.res.Compile(`^(?:[^\$]+|\$[\$*<>?@]|\$\{([.0-9A-Z_a-z]+)(?::(?:[^\${}]|\$[^{])+)?\})`) - rest := text - var result []string - for { - m := re.FindStringSubmatchIndex(rest) - if m == nil { - break - } - varname := rest[negToZero(m[2]):negToZero(m[3])] - rest = rest[:m[0]] + rest[m[1]:] - if varname != "" { - result = append(result, varname) - } - } +func (mkline *MkLineImpl) DetermineUsedVariables() []string { + var varnames []string - if trace.Tracing && rest != "" { - trace.Step1("extractUsedVariables: rest=%q", rest) + add := func(varname string) { + varnames = append(varnames, varname) } - return result -} -func (mkline *MkLineImpl) DetermineUsedVariables() (varnames []string) { - rest := mkline.Text + var searchIn func(text string) - if strings.HasPrefix(rest, "#") { - return + searchInVarUse := func(varuse *MkVarUse) { + varname := varuse.varname + if !varuse.IsExpression() { + add(varname) + } + searchIn(varname) + for _, mod := range varuse.modifiers { + searchIn(mod) + } } - for { - p1 := strings.Index(rest, "${") - p2 := strings.Index(rest, "$(") - p3 := strings.Index(rest, "defined(") - p4 := strings.Index(rest, "empty(") - if p1 == -1 && p2 == -1 && p3 == -1 && p4 == -1 { + searchIn = func(text string) { + if !contains(text, "$") { return } - min := -1 - if min == -1 || (p1 != -1 && p1 < min) { - min = p1 - } - if min == -1 || (p2 != -1 && p2 < min) { - min = p2 - } - if min == -1 || (p3 != -1 && p3 < min) { - min = p3 - } - if min == -1 || (p4 != -1 && p4 < min) { - min = p4 - } - rest = rest[min:] - m := G.res.Compile(`(?:\$\{|\$\(|defined\(|empty\()([*+\-.0-9A-Z_a-z]+)[:})]`).FindStringSubmatchIndex(rest) - if m == nil { - return + for _, token := range NewMkParser(nil, text, false).MkTokens() { + if token.Varuse != nil { + searchInVarUse(token.Varuse) + } } - varname := rest[m[2]:m[3]] - varnames = append(varnames, varname) - rest = rest[:m[0]] + rest[m[1]:] } + + switch { + + case mkline.IsVarassign(): + searchIn(mkline.Value()) + + case mkline.IsDirective() && mkline.Directive() == "for": + searchIn(mkline.Args()) + + case mkline.IsDirective() && mkline.Cond() != nil: + NewMkCondWalker().Walk( + mkline.Cond(), + &MkCondCallback{VarUse: searchInVarUse}) + + case mkline.IsShellCommand(): + searchIn(mkline.ShellCommand()) + + case mkline.IsDependency(): + searchIn(mkline.Targets()) + searchIn(mkline.Sources()) + + case mkline.IsInclude(): + searchIn(mkline.IncludeFile()) + } + + return varnames } type MkOperator uint8 @@ -775,7 +778,7 @@ func (ind *Indentation) String() string { s += " (" + strings.Join(level.conditionalVars, " ") + ")" } } - return "[" + strings.TrimSpace(s) + "]" + return "[" + trimHspace(s) + "]" } type indentationLevel struct { @@ -897,12 +900,9 @@ func (ind *Indentation) TrackBefore(mkline MkLine) { trace.Stepf("Indentation before line %s: %s", mkline.Linenos(), ind) } - directive := mkline.Directive() - args := mkline.Args() - - switch directive { + switch mkline.Directive() { case "for", "if", "ifdef", "ifndef": - ind.Push(mkline, ind.top().depth, args) + ind.Push(mkline, ind.top().depth, mkline.Args()) } } @@ -918,32 +918,26 @@ func (ind *Indentation) TrackAfter(mkline MkLine) { case "if": // For multiple-inclusion guards, the indentation stays at the same level. guard := false - if hasPrefix(args, "!defined") && hasSuffix(args, "_MK)") { - if hasPrefix(args, "!defined(") && hasSuffix(args, ")") { - varname := args[9 : len(args)-1] - if varname != "" && isalnum(varname) { - ind.AddVar(varname) - guard = true - } + if hasPrefix(args, "!defined(") && hasSuffix(args, "_MK)") { + varname := args[9 : len(args)-1] + if varname != "" && isalnum(varname) { + ind.AddVar(varname) + guard = true } } if !guard { ind.top().depth += 2 } - // Note: adding the used variables for arbitrary conditions - // happens in MkLineChecker.checkDirectiveCond for performance reasons. + if cond := mkline.Cond(); cond != nil { + ind.RememberUsedVariables(cond) - if contains(args, "exists") { - cond := NewMkParser(mkline.Line, args, false).MkCond() - if cond != nil { - NewMkCondWalker().Walk(cond, &MkCondCallback{ - Call: func(name string, arg string) { - if name == "exists" { - ind.AddCheckedFile(arg) - } - }}) - } + NewMkCondWalker().Walk(cond, &MkCondCallback{ + Call: func(name string, arg string) { + if name == "exists" { + ind.AddCheckedFile(arg) + } + }}) } case "for", "ifdef", "ifndef": @@ -986,13 +980,26 @@ func MatchVarassign(text string) (m, commented bool, varname, spaceAfterVarname, for ; i < n; i++ { b := text[i] switch { + + // As of go1.11.1 (October 2018), the Go compiler doesn't emit good + // code for these kinds of comparisons. + // See https://github.com/golang/go/issues/17372. case 'A' <= b && b <= 'Z', 'a' <= b && b <= 'z', b == '_', '0' <= b && b <= '9', - '$' <= b && b <= '.' && (b == '$' || b == '*' || b == '+' || b == '-' || b == '.'), - b == '[', - b == '{', b == '}': + '*' <= b && b <= '.' && (b == '*' || b == '+' || b == '-' || b == '.'), + b == '[': // For the tool of the same name, e.g. "TOOLS_PATH.[". + continue + + case b == '$': + parser := NewMkParser(nil, text[i:], false) + varuse := parser.VarUse() + if varuse == nil { + return + } + varuseLen := len(text[i:]) - len(parser.Rest()) + i += varuseLen - 1 continue } break @@ -1055,7 +1062,7 @@ func MatchVarassign(text string) (m, commented bool, varname, spaceAfterVarname, spaceAfterVarname = text[varnameEnd:opStart] op = text[opStart:opEnd] valueAlign = text[0:valueStart] - value = strings.TrimSpace(string(valuebuf[:j])) + value = trimHspace(string(valuebuf[:j])) spaceAfterValue = text[valueEnd:commentStart] comment = text[commentStart:commentEnd] return @@ -1069,12 +1076,12 @@ func MatchMkInclude(text string) (m bool, indentation, directive, filename strin } if repl.AdvanceStr("include") || repl.AdvanceStr("sinclude") { directive = repl.Str() - repl.AdvanceHspace() + repl.SkipHspace() if repl.AdvanceByte('"') { if repl.AdvanceBytesFunc(func(c byte) bool { return c != '"' }) { filename = repl.Str() if repl.AdvanceByte('"') { - repl.AdvanceHspace() + repl.SkipHspace() if repl.EOF() || repl.PeekByte() == '#' { m = true return diff --git a/pkgtools/pkglint/files/mkline_test.go b/pkgtools/pkglint/files/mkline_test.go index d38bfb0f99f..7fe16f95809 100644 --- a/pkgtools/pkglint/files/mkline_test.go +++ b/pkgtools/pkglint/files/mkline_test.go @@ -218,6 +218,18 @@ func (s *Suite) Test_NewMkLine__autofix_space_after_varname(c *check.C) { "pkgbase := pkglint") } +func (s *Suite) Test_MkLine_Cond(c *check.C) { + t := s.Init(c) + + mkline := t.NewMkLine("Makefile", 2, ".if ${VAR} == Value") + + cond := mkline.Cond() + + c.Check(cond.CompareVarStr.Var.varname, equals, "VAR") + c.Check(cond.CompareVarStr.Str, equals, "Value") + c.Check(mkline.Cond(), equals, cond) +} + // Guessing the variable type works for both plain and parameterized variable names. func (s *Suite) Test_Pkgsrc_VariableType__varparam(c *check.C) { t := s.Init(c) @@ -277,11 +289,48 @@ func (s *Suite) Test_NewMkLine__leading_space(c *check.C) { t := s.Init(c) _ = t.NewMkLine("rubyversion.mk", 427, " _RUBYVER=\t2.15") + _ = t.NewMkLine("bsd.buildlink3.mk", 132, " ok:=yes") + // In mk/buildlink3/bsd.buildlink3.mk, the leading space is really helpful, + // therefore no warnings for that file. t.CheckOutputLines( "WARN: rubyversion.mk:427: Makefile lines should not start with space characters.") } +// Exotic test cases from the pkgsrc infrastructure. +// Hopefully, pkgsrc packages don't need such complicated code. +func (s *Suite) Test_NewMkLine__infrastructure(c *check.C) { + t := s.Init(c) + + mklines := t.NewMkLines("infra.mk", + MkRcsID, + " USE_BUILTIN.${_pkg_:S/^-//}:=no", + ".error \"Something went wrong\"", + ".export WRKDIR", + ".export", + ".unexport-env WRKDIR", + "", + ".ifmake target1", // Luckily, this is not used in the wild. + ".elifnmake target2", // Neither is this. + ".endif") + + c.Check(mklines.mklines[1].Varcanon(), equals, "USE_BUILTIN.*") + c.Check(mklines.mklines[2].Directive(), equals, "error") + c.Check(mklines.mklines[3].Directive(), equals, "export") + + t.CheckOutputLines( + "WARN: infra.mk:2: Makefile lines should not start with space characters.", + "ERROR: infra.mk:8: Unknown Makefile line format: \".ifmake target1\".", + "ERROR: infra.mk:9: Unknown Makefile line format: \".elifnmake target2\".") + + mklines.Check() + + t.CheckOutputLines( + "WARN: infra.mk:2: USE_BUILTIN.${_pkg_:S/^-//} is defined but not used.", + "ERROR: infra.mk:5: \".export\" requires arguments.", + "ERROR: infra.mk:10: Unmatched .endif.") +} + func (s *Suite) Test_MkLines_Check__extra(c *check.C) { t := s.Init(c) @@ -387,8 +436,8 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__command_in_command(c *check.C) t.SetupCommandLine("-Wall") t.SetupVartypes() - t.SetupToolUsable("find", "FIND") - t.SetupToolUsable("sort", "SORT") + t.SetupTool("find", "FIND", AtRunTime) + t.SetupTool("sort", "SORT", AtRunTime) G.Pkg = NewPackage(t.File("category/pkgbase")) G.Mk = t.NewMkLines("Makefile", MkRcsID, @@ -425,8 +474,8 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__command_as_command_argument(c t := s.Init(c) t.SetupCommandLine("-Wall") - t.SetupToolUsable("perl", "PERL5") - t.SetupToolUsable("bash", "BASH") + t.SetupTool("perl", "PERL5", AtRunTime) + t.SetupTool("bash", "BASH", AtRunTime) t.SetupVartypes() mklines := t.NewMkLines("Makefile", MkRcsID, @@ -465,8 +514,8 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__command_in_subshell(c *check.C t.SetupCommandLine("-Wall") t.SetupVartypes() - t.SetupToolUsable("awk", "AWK") - t.SetupToolUsable("echo", "ECHO") + t.SetupTool("awk", "AWK", AtRunTime) + t.SetupTool("echo", "ECHO", AtRunTime) G.Mk = t.NewMkLines("xpi.mk", MkRcsID, "\t id=$$(${AWK} '{print}' < ${WRKSRC}/idfile) && echo \"$$id\"", @@ -546,8 +595,8 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__tool_in_quotes_in_subshell_in_ t := s.Init(c) t.SetupCommandLine("-Wall") - t.SetupToolUsable("echo", "ECHO") - t.SetupToolUsable("sh", "SH") + t.SetupTool("echo", "ECHO", AtRunTime) + t.SetupTool("sh", "SH", AtRunTime) t.SetupVartypes() G.Mk = t.NewMkLines("x11/labltk/Makefile", MkRcsID, @@ -615,7 +664,9 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__list_in_list(c *check.C) { G.Mk.Check() - t.CheckOutputEmpty() // Don't warn about missing :Q modifiers. + // Don't warn about missing :Q modifiers. + t.CheckOutputLines( + "WARN: x11/eterm/Makefile:2: PIXMAP_FILES is used but not defined.") } func (s *Suite) Test_MkLine_VariableNeedsQuoting__PKGNAME_and_URL_list_in_URL_list(c *check.C) { @@ -638,7 +689,7 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__tool_in_CONFIGURE_ENV(c *check t.SetupCommandLine("-Wall") t.SetupVartypes() - t.SetupToolUsable("tar", "TAR") + t.SetupTool("tar", "TAR", AtRunTime) mklines := t.NewMkLines("Makefile", MkRcsID, "", @@ -659,7 +710,7 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__backticks(c *check.C) { t.SetupCommandLine("-Wall") t.SetupVartypes() - t.SetupToolUsable("cat", "CAT") + t.SetupTool("cat", "CAT", AtRunTime) mklines := t.NewMkLines("Makefile", MkRcsID, "", @@ -734,7 +785,7 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__tool_in_shell_command(c *check t.SetupCommandLine("-Wall,no-space") t.SetupVartypes() - t.SetupToolUsable("bash", "BASH") + t.SetupTool("bash", "BASH", AtRunTime) mklines := t.SetupFileMkLines("Makefile", MkRcsID, @@ -767,7 +818,10 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__uncovered_cases(c *check.C) { t.CheckOutputLines( "WARN: ~/Makefile:4: The variable LINKER_RPATH_FLAG may not be set by any package.", "WARN: ~/Makefile:4: Please use ${LINKER_RPATH_FLAG:S/-rpath/& /:Q} instead of ${LINKER_RPATH_FLAG:S/-rpath/& /}.", - "WARN: ~/Makefile:4: LINKER_RPATH_FLAG should not be evaluated at load time.") + "WARN: ~/Makefile:4: LINKER_RPATH_FLAG should not be evaluated at load time.", + "WARN: ~/Makefile:6: The variable PATH may not be set by any package.", + "WARN: ~/Makefile:6: PREFIX should not be evaluated at load time.", + "WARN: ~/Makefile:6: PATH should not be evaluated at load time.") } func (s *Suite) Test_ShellLine_CheckWord__PKGMANDIR(c *check.C) { @@ -800,8 +854,9 @@ func (s *Suite) Test_MkLines_Check__VERSION_as_wordpart_in_MASTER_SITES(c *check mklines.Check() t.CheckOutputLines( - "WARN: geography/viking/Makefile:2: " + - "The list variable MASTER_SITE_SOURCEFORGE should not be embedded in a word.") + "WARN: geography/viking/Makefile:2: "+ + "The list variable MASTER_SITE_SOURCEFORGE should not be embedded in a word.", + "WARN: geography/viking/Makefile:2: VERSION is used but not defined.") } func (s *Suite) Test_MkLines_Check__shell_command_as_wordpart_in_ENV_list(c *check.C) { @@ -825,6 +880,7 @@ func (s *Suite) Test_MkLine__shell_varuse_in_backt_dquot(c *check.C) { t.SetupCommandLine("-Wall") t.SetupVartypes() + t.SetupTool("grep", "GREP", AtRunTime) mklines := t.NewMkLines("x11/motif/Makefile", MkRcsID, "post-patch:", @@ -833,8 +889,7 @@ func (s *Suite) Test_MkLine__shell_varuse_in_backt_dquot(c *check.C) { mklines.Check() // Just ensure that there are no parse errors. - t.CheckOutputLines( - "WARN: x11/motif/Makefile:3: Unknown shell command \"${GREP}\".") + t.CheckOutputEmpty() } // See PR 46570, Ctrl+F "3. In lang/perl5". @@ -1050,25 +1105,57 @@ func (s *Suite) Test_Indentation_RememberUsedVariables(c *check.C) { mkline := t.NewMkLine("Makefile", 123, ".if ${PKGREVISION} > 0") ind := NewIndentation() - cond := NewMkParser(mkline.Line, mkline.Args(), false) - ind.RememberUsedVariables(cond.MkCond()) + ind.RememberUsedVariables(mkline.Cond()) t.CheckOutputEmpty() c.Check(ind.Varnames(), equals, "PKGREVISION") } -func (s *Suite) Test_MkLine_ExtractUsedVariables(c *check.C) { +func (s *Suite) Test_MkLine_DetermineUsedVariables(c *check.C) { t := s.Init(c) - mkline := t.NewMkLine("Makefile", 123, "") - - nestedVarnames := mkline.ExtractUsedVariables("${VAR.${param}}") - - // ExtractUsedVariables is very old code, should be more advanced. - c.Check(nestedVarnames, check.IsNil) - - plainVarnames := mkline.ExtractUsedVariables("${VAR}and${VAR2}") + mklines := t.NewMkLines("Makefile", + MkRcsID, + "VAR=\t${VALUE} # ${varassign.comment}", + ".if ${OPSYS:M${endianness}} == ${Hello:L} # ${if.comment}", + ".for var in one ${two} three # ${for.comment}", + "# ${empty.comment}", + "${TARGETS}: ${SOURCES} # ${dependency.comment}", + ".include \"${OTHER_FILE}\"", + "", + "\t"+ + "${VAR.${param}}"+ + "${VAR}and${VAR2}"+ + "${VAR:M${pattern}}"+ + "$(ROUND_PARENTHESES)"+ + "$$shellvar"+ + "$<$@$x") + + var varnames []string + for _, mkline := range mklines.mklines { + varnames = append(varnames, mkline.DetermineUsedVariables()...) + } - c.Check(plainVarnames, deepEquals, []string{"VAR", "VAR2"}) + c.Check(varnames, deepEquals, []string{ + "VALUE", + "OPSYS", + "endianness", + // "Hello" is not a variable name, the :L modifier makes it an expression. + "two", + "TARGETS", + "SOURCES", + "OTHER_FILE", + + "VAR.${param}", + "param", + "VAR", + "VAR2", + "VAR", + "pattern", + "ROUND_PARENTHESES", + // Shell variables are ignored here. + "<", + "@", + "x"}) } diff --git a/pkgtools/pkglint/files/mklinechecker.go b/pkgtools/pkglint/files/mklinechecker.go index b9c7981baf7..650e6b84556 100644 --- a/pkgtools/pkglint/files/mklinechecker.go +++ b/pkgtools/pkglint/files/mklinechecker.go @@ -79,7 +79,7 @@ func (ck MkLineChecker) checkInclude() { "Makefile.common.") case IsPrefs(includefile): - if path.Base(mkline.Filename) == "buildlink3.mk" && includefile == "../../mk/bsd.prefs.mk" { + if mkline.Basename == "buildlink3.mk" && includefile == "../../mk/bsd.prefs.mk" { mkline.Notef("For efficiency reasons, please include bsd.fast.prefs.mk instead of bsd.prefs.mk.") } @@ -112,7 +112,11 @@ func (ck MkLineChecker) checkDirective(forVars map[string]bool, ind *Indentation needsArgument := false switch directive { - case "if", "ifdef", "ifndef", "elif", "for", "undef": + case + "if", "ifdef", "ifndef", "elif", + "for", "undef", + "error", "warning", "info", + "export", "export-env", "unexport", "unexport-env": needsArgument = true } @@ -136,8 +140,8 @@ func (ck MkLineChecker) checkDirective(forVars map[string]bool, ind *Indentation } else if directive == "for" { ck.checkDirectiveFor(forVars, ind) - } else if directive == "undef" && args != "" { - for _, varname := range splitOnSpace(args) { + } else if directive == "undef" { + for _, varname := range fields(args) { if forVars[varname] { mkline.Notef("Using \".undef\" after a \".for\" loop is unnecessary.") } @@ -170,7 +174,7 @@ func (ck MkLineChecker) checkDirectiveFor(forVars map[string]bool, indentation * args := mkline.Args() if m, vars, values := match2(args, `^(\S+(?:\s*\S+)*?)\s+in\s+(.*)$`); m { - for _, forvar := range splitOnSpace(vars) { + for _, forvar := range fields(vars) { indentation.AddVar(forvar) if !G.Infrastructure && hasPrefix(forvar, "_") { mkline.Warnf("Variable names starting with an underscore (%s) are reserved for internal pkgsrc use.", forvar) @@ -189,7 +193,7 @@ func (ck MkLineChecker) checkDirectiveFor(forVars map[string]bool, indentation * // Check if any of the value's types is not guessed. guessed := true - for _, value := range splitOnSpace(values) { + for _, value := range fields(values) { if m, vname := match1(value, `^\$\{(.*)\}`); m { vartype := G.Pkgsrc.VariableType(vname) if vartype != nil && !vartype.guessed { @@ -200,7 +204,7 @@ func (ck MkLineChecker) checkDirectiveFor(forVars map[string]bool, indentation * forLoopType := &Vartype{lkSpace, BtUnknown, []ACLEntry{{"*", aclpAllRead}}, guessed} forLoopContext := &VarUseContext{forLoopType, vucTimeParse, vucQuotFor, false} - for _, forLoopVar := range mkline.ExtractUsedVariables(values) { + for _, forLoopVar := range mkline.DetermineUsedVariables() { ck.CheckVaruse(&MkVarUse{forLoopVar, nil}, forLoopContext) } } @@ -222,8 +226,8 @@ func (ck MkLineChecker) checkDirectiveIndentation(expectedDepth int) { func (ck MkLineChecker) checkDependencyRule(allowedTargets map[string]bool) { mkline := ck.MkLine - targets := splitOnSpace(mkline.Targets()) - sources := splitOnSpace(mkline.Sources()) + targets := fields(mkline.Targets()) + sources := fields(mkline.Sources()) for _, source := range sources { if source == ".PHONY" { @@ -283,7 +287,7 @@ func (ck MkLineChecker) checkVarassignPermissions() { return } - perms := vartype.EffectivePermissions(mkline.Filename) + perms := vartype.EffectivePermissions(mkline.Basename) // E.g. USE_TOOLS:= ${USE_TOOLS:Nunwanted-tool} if op == opAssignEval && perms&aclpAppend != 0 { @@ -351,14 +355,15 @@ func (ck MkLineChecker) CheckVaruse(varuse *MkVarUse, vuc *VarUseContext) { case !G.opts.WarnExtra: case vartype != nil && !vartype.guessed: // Well-known variables are probably defined by the infrastructure. - case varIsUsed(varname): + case varIsDefinedSimilar(varname): case containsVarRef(varname): + case G.Mk != nil && !G.Mk.FirstTime("used but not defined: "+varname): default: mkline.Warnf("%s is used but not defined.", varname) } if hasPrefix(varuse.Mod(), ":=") && vartype != nil && !vartype.IsConsideredList() { - mkline.Warnf("The :from=to modifier should only be used with lists.") + mkline.Warnf("The :from=to modifier should only be used with lists, not with %s.", varuse.varname) Explain( "Instead of:", "\tMASTER_SITES=\t${HOMEPAGE:=repository/}", @@ -423,37 +428,37 @@ func (ck MkLineChecker) checkVarusePermissions(varname string, vartype *Vartype, } mkline := ck.MkLine - perms := vartype.EffectivePermissions(mkline.Filename) + perms := vartype.EffectivePermissions(mkline.Basename) - isLoadTime := false // Will the variable be used at load time? + warnLoadTime := false // Will the variable be used at load time? // Might the variable be used indirectly at load time, for example // by assigning it to another variable which then gets evaluated? - isIndirect := false + warnIndirect := false switch { case vuc.vartype != nil && vuc.vartype.guessed: - // Don't warn about unknown variables. + // Don't warn about unknown variables. case vuc.time == vucTimeParse && !perms.Contains(aclpUseLoadtime): - isLoadTime = true + warnLoadTime = true case vuc.vartype != nil && vuc.vartype.Union().Contains(aclpUseLoadtime) && !perms.Contains(aclpUseLoadtime): - isLoadTime = true - isIndirect = true + warnLoadTime = true + warnIndirect = true } - if isLoadTime { + if warnLoadTime { if tool := G.ToolByVarname(varname, LoadTime); tool != nil { ck.checkVaruseToolLoadTime(varname, tool) } else { - ck.checkVaruseLoadTime(varname, isIndirect) + ck.warnVaruseLoadTime(varname, warnIndirect) } } if !perms.Contains(aclpUseLoadtime) && !perms.Contains(aclpUse) { needed := aclpUse - if isLoadTime { + if warnLoadTime { needed = aclpUseLoadtime } alternativeFiles := vartype.AllowedFiles(needed) @@ -482,7 +487,7 @@ func (ck MkLineChecker) checkVaruseToolLoadTime(varname string, tool *Tool) { return } - if path.Base(ck.MkLine.Filename) == "Makefile" { + if ck.MkLine.Basename == "Makefile" { pkgsrcTool := G.Pkgsrc.Tools.ByName(tool.Name) if pkgsrcTool != nil && pkgsrcTool.Validity == Nowhere { // The tool must have been added too late to USE_TOOLS, @@ -506,7 +511,7 @@ func (ck MkLineChecker) checkVaruseToolLoadTime(varname string, tool *Tool) { "only be used at run time, except in the package Makefile itself.") } -func (ck MkLineChecker) checkVaruseLoadTime(varname string, isIndirect bool) { +func (ck MkLineChecker) warnVaruseLoadTime(varname string, isIndirect bool) { mkline := ck.MkLine if !isIndirect { @@ -646,7 +651,7 @@ func (ck MkLineChecker) checkVarassignPythonVersions(varname, value string) { } mkline := ck.MkLine - strversions := splitOnSpace(value) + strversions := fields(value) intversions := make([]int, len(strversions)) for i, strversion := range strversions { iver, err := strconv.Atoi(strversion) @@ -692,11 +697,13 @@ func (ck MkLineChecker) checkVarassign() { trace.Step1("%s might be unused unless it is an argument to a procedure file.", varname) } - } else if !varIsUsed(varname) { + } else if !varIsUsedSimilar(varname) { if vartypes := G.Pkgsrc.vartypes; vartypes[varname] != nil || vartypes[varcanon] != nil { // Ok } else if deprecated := G.Pkgsrc.Deprecated; deprecated[varname] != "" || deprecated[varcanon] != "" { // Ok + } else if G.Mk != nil && !G.Mk.FirstTime("defined but not used: "+varname) { + // Skip } else { mkline.Warnf("%s is defined but not used.", varname) } @@ -870,7 +877,7 @@ func (ck MkLineChecker) CheckVartype(varname string, op MkOperator, value, comme if op == opAssignAppend { if vartype != nil && !vartype.MayBeAppendedTo() { - mkline.Warnf("The \"+=\" operator should only be used with lists.") + mkline.Warnf("The \"+=\" operator should only be used with lists, not with %s.", varname) } } @@ -892,7 +899,7 @@ func (ck MkLineChecker) CheckVartype(varname string, op MkOperator, value, comme break case vartype.kindOfList == lkSpace: - for _, word := range splitOnSpace(value) { + for _, word := range fields(value) { ck.CheckVartypePrimitive(varname, vartype.basicType, op, word, comment, vartype.guessed) } @@ -971,7 +978,7 @@ func (ck MkLineChecker) checkDirectiveCond() { defer trace.Call1(mkline.Args())() } - p := NewMkParser(mkline.Line, mkline.Args(), false) + p := NewMkParser(mkline.Line, mkline.Args(), false) // XXX: Why false? cond := p.MkCond() if !p.EOF() { mkline.Warnf("Invalid condition, unrecognized part: %q.", p.Rest()) @@ -1028,13 +1035,16 @@ func (ck MkLineChecker) checkDirectiveCond() { } } + checkVarUse := func(varuse *MkVarUse) { + var vartype *Vartype // TODO: Insert a better type guess here. + vuc := &VarUseContext{vartype, vucTimeParse, vucQuotPlain, false} + ck.CheckVaruse(varuse, vuc) + } + NewMkCondWalker().Walk(cond, &MkCondCallback{ Empty: checkEmpty, - CompareVarStr: checkCompareVarStr}) - - if G.Mk != nil { - G.Mk.indentation.RememberUsedVariables(cond) - } + CompareVarStr: checkCompareVarStr, + VarUse: checkVarUse}) } func (ck MkLineChecker) checkCompareVarStr(varname, op, value string) { diff --git a/pkgtools/pkglint/files/mklinechecker_test.go b/pkgtools/pkglint/files/mklinechecker_test.go index 21976d57c83..376f64d9f3c 100644 --- a/pkgtools/pkglint/files/mklinechecker_test.go +++ b/pkgtools/pkglint/files/mklinechecker_test.go @@ -176,13 +176,19 @@ func (s *Suite) Test_MkLineChecker_CheckVartype__skip(c *check.C) { func (s *Suite) Test_MkLineChecker_CheckVartype__append_to_non_list(c *check.C) { t := s.Init(c) + t.SetupCommandLine("-Wall") t.SetupVartypes() - mkline := t.NewMkLine("fname", 1, "DISTNAME+=suffix") + mklines := t.NewMkLines("fname.mk", + MkRcsID, + "DISTNAME+=\tsuffix", + "COMMENT=\tComment for", + "COMMENT+=\tthe package") - MkLineChecker{mkline}.Check() + mklines.Check() t.CheckOutputLines( - "WARN: fname:1: The \"+=\" operator should only be used with lists.") + "WARN: fname.mk:2: The variable DISTNAME may not be appended to (only set, given a default value) in this file.", + "WARN: fname.mk:2: The \"+=\" operator should only be used with lists, not with DISTNAME.") } // Pkglint once interpreted all lists as consisting of shell tokens, @@ -347,11 +353,17 @@ func (s *Suite) Test_MkLineChecker_checkVarusePermissions__load_time(c *check.C) t.SetupVartypes() mklines := t.NewMkLines("options.mk", MkRcsID, - "WRKSRC:=${.CURDIR}") + "WRKSRC:=${.CURDIR}", + ".if ${PKG_SYSCONFDIR.gdm} != \"etc\"", + ".endif") mklines.Check() - // Don't warn that ".CURDIR should not be evaluated at load time." + // Evaluating PKG_SYSCONFDIR.* at load time is probably ok, though + // pkglint cannot prove anything here. + // + // Evaluating .CURDIR at load time is ok since it is defined from + // the beginning. t.CheckOutputLines( "NOTE: options.mk:2: This variable value should be aligned to column 17.") } @@ -555,8 +567,10 @@ func (s *Suite) Test_MkLineChecker_CheckVaruseShellword__mstar(c *check.C) { mklines.Check() - // FIXME: There should be some notes and warnings; prevented by the PERL5 case in VariableNeedsQuoting. - t.CheckOutputEmpty() + // FIXME: There should be some notes and warnings about missing :M*; + // these are currently prevented by the PERL5 case in VariableNeedsQuoting. + t.CheckOutputLines( + "WARN: ~/options.mk:4: ADA_FLAGS is used but not defined.") } func (s *Suite) Test_MkLineChecker_CheckVaruseShellword__mstar_not_needed(c *check.C) { @@ -611,7 +625,7 @@ func (s *Suite) Test_MkLineChecker_CheckVaruse__eq_nonlist(c *check.C) { mklines.Check() t.CheckOutputLines( - "WARN: ~/options.mk:2: The :from=to modifier should only be used with lists.") + "WARN: ~/options.mk:2: The :from=to modifier should only be used with lists, not with WRKDIR.") } func (s *Suite) Test_MkLineChecker_CheckVaruse__for(c *check.C) { @@ -631,6 +645,27 @@ func (s *Suite) Test_MkLineChecker_CheckVaruse__for(c *check.C) { t.CheckOutputEmpty() } +func (s *Suite) Test_MkLineChecker_CheckVaruse__defined_in_infrastructure(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wall") + t.SetupPkgsrc() + t.SetupVartypes() + t.CreateFileLines("mk/deeply/nested/infra.mk", + MkRcsID, + "INFRA_VAR?=\tvalue") + G.Pkgsrc.LoadInfrastructure() + mklines := t.SetupFileMkLines("category/package/module.mk", + MkRcsID, + "do-fetch:", + "\t: ${INFRA_VAR} ${UNDEFINED}") + + mklines.Check() + + t.CheckOutputLines( + "WARN: ~/category/package/module.mk:3: UNDEFINED is used but not defined.") +} + func (s *Suite) Test_MkLineChecker_CheckVaruse__build_defs(c *check.C) { t := s.Init(c) @@ -668,8 +703,9 @@ func (s *Suite) Test_MkLineChecker_checkVarassignSpecific(c *check.C) { "_TOOLS_VARNAME.sed= SED", "DIST_SUBDIR= ${PKGNAME}", "WRKSRC= ${PKGNAME}", - "SITES_distfile.tar.gz= ${MASTER_SITES_GITHUB:=user/}", - // TODO: The first of the below assignments should be flagged as redundant by RedundantScope. + "SITES_distfile.tar.gz= ${MASTER_SITE_GITHUB:=user/}", + // TODO: The first of the below assignments should be flagged as redundant by RedundantScope; + // that check is currently only implemented for package Makefiles, not for other files. "PYTHON_VERSIONS_ACCEPTED= -13", "PYTHON_VERSIONS_ACCEPTED= 27 36") diff --git a/pkgtools/pkglint/files/mklines.go b/pkgtools/pkglint/files/mklines.go index ff328db4849..3ef4409e735 100644 --- a/pkgtools/pkglint/files/mklines.go +++ b/pkgtools/pkglint/files/mklines.go @@ -2,7 +2,6 @@ package main import ( "netbsd.org/pkglint/trace" - "path" "strings" ) @@ -17,7 +16,7 @@ type MkLines struct { plistVarAdded map[string]MkLine // Identifiers that are added to PLIST_VARS. plistVarSet map[string]MkLine // Identifiers for which PLIST.${id} is defined. plistVarSkip bool // True if any of the PLIST_VARS identifiers refers to a variable. - Tools Tools // Tools defined in file scope. + Tools *Tools // Tools defined in file scope. indentation *Indentation // Indentation depth of preprocessing directives; only available during MkLines.ForEach. Once } @@ -34,7 +33,7 @@ func NewMkLines(lines []Line) *MkLines { } tools := NewTools(traceName) - tools.AddAll(G.Pkgsrc.Tools) + tools.Fallback(G.Pkgsrc.Tools) return &MkLines{ mklines, @@ -66,15 +65,6 @@ func (mklines *MkLines) Check() { G.Mk = mklines defer func() { G.Mk = nil }() - allowedTargets := make(map[string]bool) - prefixes := [...]string{"pre", "do", "post"} - actions := [...]string{"fetch", "extract", "patch", "tools", "wrapper", "configure", "build", "test", "install", "package", "clean"} - for _, prefix := range prefixes { - for _, action := range actions { - allowedTargets[prefix+"-"+action] = true - } - } - // In the first pass, all additions to BUILD_DEFS and USE_TOOLS // are collected to make the order of the definitions irrelevant. mklines.DetermineUsedVariables() @@ -83,12 +73,28 @@ func (mklines *MkLines) Check() { mklines.collectElse() // In the second pass, the actual checks are done. + mklines.checkAll() + + SaveAutofixChanges(mklines.lines) +} + +func (mklines *MkLines) checkAll() { + allowedTargets := func() map[string]bool { + targets := make(map[string]bool) + prefixes := [...]string{"pre", "do", "post"} + actions := [...]string{"fetch", "extract", "patch", "tools", "wrapper", "configure", "build", "test", "install", "package", "clean"} + for _, prefix := range prefixes { + for _, action := range actions { + targets[prefix+"-"+action] = true + } + } + return targets + }() CheckLineRcsid(mklines.lines[0], `#\s+`, "# ") - substcontext := NewSubstContext() + substContext := NewSubstContext() var varalign VaralignBlock - lastMkline := mklines.mklines[len(mklines.mklines)-1] isHacksMk := mklines.lines[0].Basename == "hacks.mk" lineAction := func(mkline MkLine) bool { @@ -99,16 +105,16 @@ func (mklines *MkLines) Check() { ck := MkLineChecker{mkline} ck.Check() varalign.Check(mkline) - mklines.Tools.ParseToolLine(mkline) + mklines.Tools.ParseToolLine(mkline, false, false) switch { case mkline.IsEmpty(): - substcontext.Finish(mkline) + substContext.Finish(mkline) case mkline.IsVarassign(): mklines.target = "" mkline.Tokenize(mkline.Value(), true) // Just for the side-effect of the warnings. - substcontext.Varassign(mkline) + substContext.Varassign(mkline) switch mkline.Varcanon() { case "PLIST_VARS": @@ -134,7 +140,7 @@ func (mklines *MkLines) Check() { case mkline.IsDirective(): ck.checkDirective(mklines.forVars, mklines.indentation) - substcontext.Directive(mkline) + substContext.Directive(mkline) case mkline.IsDependency(): ck.checkDependencyRule(allowedTargets) @@ -160,16 +166,15 @@ func (mklines *MkLines) Check() { } mklines.ForEachEnd(lineAction, atEnd) - substcontext.Finish(lastMkline) + substContext.Finish(NewMkLine(NewLineEOF(mklines.lines[0].Filename))) varalign.Finish() ChecklinesTrailingEmptyLines(mklines.lines) - - SaveAutofixChanges(mklines.lines) } // ForEach calls the action for each line, until the action returns false. -// It keeps track of the indentation and all conditional variables. +// It keeps track of the indentation (see MkLines.indentation) +// and all conditional variables (see Indentation.IsConditional). func (mklines *MkLines) ForEach(action func(mkline MkLine)) { mklines.ForEachEnd( func(mkline MkLine) bool { action(mkline); return true }, @@ -201,7 +206,7 @@ func (mklines *MkLines) DetermineDefinedVariables() { } for _, mkline := range mklines.mklines { - mklines.Tools.ParseToolLine(mkline) + mklines.Tools.ParseToolLine(mkline, false, true) if !mkline.IsVarassign() { continue @@ -211,14 +216,24 @@ func (mklines *MkLines) DetermineDefinedVariables() { varcanon := mkline.Varcanon() switch varcanon { - case "BUILD_DEFS", "PKG_GROUPS_VARS", "PKG_USERS_VARS": - for _, varname := range splitOnSpace(mkline.Value()) { + case + "BUILD_DEFS", + "PKG_GROUPS_VARS", + "PKG_USERS_VARS": + for _, varname := range fields(mkline.Value()) { mklines.buildDefs[varname] = true if trace.Tracing { trace.Step1("%q is added to BUILD_DEFS.", varname) } } + case + "BUILTIN_FIND_FILES_VAR", + "BUILTIN_FIND_HEADERS_VAR": + for _, varname := range fields(mkline.Value()) { + mklines.vars.Define(varname, mkline) + } + case "PLIST_VARS": ids := mkline.ValueSplit(resolveVariableRefs(mkline.Value()), "") for _, id := range ids { @@ -234,7 +249,7 @@ func (mklines *MkLines) DetermineDefinedVariables() { } case "SUBST_VARS.*": - for _, svar := range splitOnSpace(mkline.Value()) { + for _, svar := range fields(mkline.Value()) { mklines.UseVar(mkline, varnameCanon(svar)) if trace.Tracing { trace.Step1("varuse %s", svar) @@ -242,7 +257,7 @@ func (mklines *MkLines) DetermineDefinedVariables() { } case "OPSYSVARS": - for _, osvar := range splitOnSpace(mkline.Value()) { + for _, osvar := range fields(mkline.Value()) { mklines.UseVar(mkline, osvar+".*") defineVar(mkline, osvar) } @@ -282,11 +297,11 @@ func (mklines *MkLines) collectElse() { func (mklines *MkLines) DetermineUsedVariables() { for _, mkline := range mklines.mklines { - varnames := mkline.DetermineUsedVariables() - for _, varname := range varnames { + for _, varname := range mkline.DetermineUsedVariables() { mklines.UseVar(mkline, varname) } } + mklines.determineDocumentedVariables() } @@ -312,7 +327,7 @@ func (mklines *MkLines) determineDocumentedVariables() { text := mkline.Text switch { case hasPrefix(text, "#"): - words := splitOnSpace(text) + words := fields(text) if len(words) <= 1 { break } @@ -346,7 +361,7 @@ func (mklines *MkLines) determineDocumentedVariables() { func (mklines *MkLines) CheckRedundantVariables() { scope := NewRedundantScope() isRelevant := func(old, new MkLine) bool { - if path.Base(old.Filename) != "Makefile" && path.Base(new.Filename) == "Makefile" { + if old.Basename != "Makefile" && new.Basename == "Makefile" { return false } if new.Op() == opAssignEval { @@ -447,7 +462,8 @@ func (va *VaralignBlock) Check(mkline MkLine) { if mkline.IsMultiline() { // Interpreting the continuation marker as variable value // is cheating, but works well. - m, _, _, _, _, _, value, _, _ := MatchVarassign(mkline.raw[0].orignl) + text := strings.TrimSuffix(mkline.raw[0].orignl, "\n") + m, _, _, _, _, _, value, _, _ := MatchVarassign(text) continuation = m && value == "\\" } diff --git a/pkgtools/pkglint/files/mklines_test.go b/pkgtools/pkglint/files/mklines_test.go index dd880ae8728..c037bc99ff7 100644 --- a/pkgtools/pkglint/files/mklines_test.go +++ b/pkgtools/pkglint/files/mklines_test.go @@ -86,9 +86,10 @@ func (s *Suite) Test_MkLines__for_loop_multiple_variables(c *check.C) { t := s.Init(c) t.SetupCommandLine("-Wall") - t.SetupToolUsable("echo", "ECHO") - t.SetupToolUsable("find", "FIND") - t.SetupToolUsable("pax", "PAX") + t.SetupVartypes() + t.SetupTool("echo", "ECHO", AtRunTime) + t.SetupTool("find", "FIND", AtRunTime) + t.SetupTool("pax", "PAX", AtRunTime) mklines := t.NewMkLines("Makefile", // From audio/squeezeboxserver MkRcsID, "", @@ -102,6 +103,7 @@ func (s *Suite) Test_MkLines__for_loop_multiple_variables(c *check.C) { t.CheckOutputLines( "WARN: Makefile:3: Variable names starting with an underscore (_list_) are reserved for internal pkgsrc use.", "WARN: Makefile:3: Variable names starting with an underscore (_dir_) are reserved for internal pkgsrc use.", + "WARN: Makefile:3: SBS_COPY is used but not defined.", "WARN: Makefile:4: The exitcode of \"${FIND}\" at the left of the | operator is ignored.") } @@ -129,6 +131,7 @@ func (s *Suite) Test_MkLines__varuse_sh_modifier(c *check.C) { t.SetupCommandLine("-Wall") t.SetupVartypes() + t.SetupTool("sed", "SED", AfterPrefsMk) mklines := t.NewMkLines("lang/qore/module.mk", MkRcsID, "qore-version=\tqore --short-version | ${SED} -e s/-.*//", @@ -209,6 +212,7 @@ func (s *Suite) Test_MkLines__indirect_variables(c *check.C) { t := s.Init(c) t.SetupCommandLine("-Wall") + t.SetupTool("echo", "ECHO", AfterPrefsMk) mklines := t.NewMkLines("net/uucp/Makefile", MkRcsID, "", @@ -220,14 +224,18 @@ func (s *Suite) Test_MkLines__indirect_variables(c *check.C) { mklines.Check() // No warning about UUCP_${var} being used but not defined. - t.CheckOutputLines( - "WARN: net/uucp/Makefile:5: Unknown shell command \"${ECHO}\".") + // Normally, parameterized variables use a dot instead of an + // underscore as separator. This is one of the other cases, + // and pkglint just doesn't warn about dynamic variable names + // like UUCP_${var} or SITES_${distfile}. + t.CheckOutputEmpty() } func (s *Suite) Test_MkLines_Check__list_variable_as_part_of_word(c *check.C) { t := s.Init(c) t.SetupCommandLine("-Wall") + t.SetupVartypes() mklines := t.NewMkLines("converters/chef/Makefile", MkRcsID, "\tcd ${WRKSRC} && tr '\\r' '\\n' < ${DISTDIR}/${DIST_SUBDIR}/${DISTFILES} > chef.l") @@ -346,9 +354,6 @@ func (s *Suite) Test_MkLines_DetermineDefinedVariables(c *check.C) { // The SUV variable is used implicitly by the SUBST framework, therefore no warning. // The OSV.NetBSD variable is used implicitly via the OSV variable, therefore no warning. t.CheckOutputLines( - // FIXME: For most lists, using the := operator to exclude an item is ok. - "WARN: determine-defined-variables.mk:4: USE_TOOLS should not be evaluated at load time.", - "WARN: determine-defined-variables.mk:4: USE_TOOLS may not be used in any file; it is a write-only variable.", // FIXME: the below warning is wrong; it's ok to have SUBST blocks in all files, maybe except buildlink3.mk. "WARN: determine-defined-variables.mk:12: The variable SUBST_VARS.subst may not be set (only given a default value, appended to) in this file; it would be ok in Makefile, Makefile.common, options.mk.", // FIXME: the below warning is wrong; variables mentioned in SUBST_VARS should be allowed in that block. @@ -356,6 +361,31 @@ func (s *Suite) Test_MkLines_DetermineDefinedVariables(c *check.C) { "WARN: determine-defined-variables.mk:16: Unknown shell command \"unknown-command\".") } +func (s *Suite) Test_MkLines_DetermineDefinedVariables__BUILTIN_FIND_FILES_VAR(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wall,no-space") + t.SetupPackage("category/package") + t.CreateFileLines("mk/buildlink3/bsd.builtin.mk", + MkRcsID) + mklines := t.SetupFileMkLines("category/package/builtin.mk", + MkRcsID, + "", + "BUILTIN_FIND_FILES_VAR:= H_XFT2", + "BUILTIN_FIND_FILES.H_XFT2= ${X11BASE}/include/X11/Xft/Xft.h", + "", + ".include \"../../mk/buildlink3/bsd.builtin.mk\"", + "", + ".if ${H_XFT2:N__nonexistent__} && ${H_UNDEF:N__nonexistent__}", + ".endif") + G.Pkgsrc.LoadInfrastructure() + + mklines.Check() + + t.CheckOutputLines( + "WARN: ~/category/package/builtin.mk:8: H_UNDEF is used but not defined.") +} + func (s *Suite) Test_MkLines_DetermineUsedVariables__simple(c *check.C) { t := s.Init(c) @@ -382,8 +412,8 @@ func (s *Suite) Test_MkLines_DetermineUsedVariables__nested(c *check.C) { c.Check(len(mklines.vars.used), equals, 3) c.Check(mklines.vars.FirstUse("inner"), equals, mkline) - c.Check(mklines.vars.FirstUse("outer."), equals, mkline) // XXX: why the . at the end? c.Check(mklines.vars.FirstUse("outer.*"), equals, mkline) + c.Check(mklines.vars.FirstUse("outer.${inner}"), equals, mkline) } func (s *Suite) Test_MkLines__private_tool_undefined(c *check.C) { @@ -424,6 +454,7 @@ func (s *Suite) Test_MkLines_Check__indentation(c *check.C) { t := s.Init(c) t.SetupCommandLine("-Wall") + t.SetupVartypes() mklines := t.NewMkLines("options.mk", MkRcsID, ". if !defined(GUARD_MK)", @@ -445,14 +476,19 @@ func (s *Suite) Test_MkLines_Check__indentation(c *check.C) { t.CheckOutputLines( "NOTE: options.mk:2: This directive should be indented by 0 spaces.", + "WARN: options.mk:2: GUARD_MK is used but not defined.", "NOTE: options.mk:3: This directive should be indented by 0 spaces.", "NOTE: options.mk:4: This directive should be indented by 2 spaces.", + "WARN: options.mk:4: FILES is used but not defined.", "NOTE: options.mk:5: This directive should be indented by 4 spaces.", + "WARN: options.mk:5: GUARD2_MK is used but not defined.", "NOTE: options.mk:6: This directive should be indented by 4 spaces.", "NOTE: options.mk:7: This directive should be indented by 4 spaces.", "NOTE: options.mk:8: This directive should be indented by 2 spaces.", "NOTE: options.mk:9: This directive should be indented by 2 spaces.", + "WARN: options.mk:9: COND1 is used but not defined.", "NOTE: options.mk:10: This directive should be indented by 2 spaces.", + "WARN: options.mk:10: COND2 is used but not defined.", "NOTE: options.mk:11: This directive should be indented by 2 spaces.", "ERROR: options.mk:11: \".else\" does not take arguments. If you meant \"else if\", use \".elif\".", "NOTE: options.mk:12: This directive should be indented by 2 spaces.", @@ -466,17 +502,18 @@ func (s *Suite) Test_MkLines_Check__endif_comment(c *check.C) { t := s.Init(c) t.SetupCommandLine("-Wall") + t.SetupVartypes() mklines := t.NewMkLines("opsys.mk", MkRcsID, "", ".for i in 1 2 3 4 5", ". if ${OPSYS} == NetBSD", - ". if ${ARCH} == x86_64", + ". if ${MACHINE_ARCH} == x86_64", ". if ${OS_VERSION:M8.*}", - ". endif # ARCH", // Wrong, should be OS_VERSION. - ". endif # OS_VERSION", // Wrong, should be ARCH. - ". endif # OPSYS", // Correct. - ".endfor # j", // Wrong, should be i. + ". endif # MACHINE_ARCH", // Wrong, should be OS_VERSION. + ". endif # OS_VERSION", // Wrong, should be MACHINE_ARCH. + ". endif # OPSYS", // Correct. + ".endfor # j", // Wrong, should be i. "", ".if ${PKG_OPTIONS:Moption}", ".endif # option", @@ -492,9 +529,10 @@ func (s *Suite) Test_MkLines_Check__endif_comment(c *check.C) { mklines.Check() t.CheckOutputLines( - "WARN: opsys.mk:7: Comment \"ARCH\" does not match condition \"${OS_VERSION:M8.*}\".", - "WARN: opsys.mk:8: Comment \"OS_VERSION\" does not match condition \"${ARCH} == x86_64\".", + "WARN: opsys.mk:7: Comment \"MACHINE_ARCH\" does not match condition \"${OS_VERSION:M8.*}\".", + "WARN: opsys.mk:8: Comment \"OS_VERSION\" does not match condition \"${MACHINE_ARCH} == x86_64\".", "WARN: opsys.mk:10: Comment \"j\" does not match loop \"i in 1 2 3 4 5\".", + "WARN: opsys.mk:12: Unknown option \"option\".", "WARN: opsys.mk:20: Comment \"NetBSD\" does not match condition \"${OPSYS} == FreeBSD\".") } @@ -502,12 +540,13 @@ func (s *Suite) Test_MkLines_Check__unbalanced_directives(c *check.C) { t := s.Init(c) t.SetupCommandLine("-Wall") + t.SetupVartypes() mklines := t.NewMkLines("opsys.mk", MkRcsID, "", ".for i in 1 2 3 4 5", ". if ${OPSYS} == NetBSD", - ". if ${ARCH} == x86_64", + ". if ${MACHINE_ARCH} == x86_64", ". if ${OS_VERSION:M8.*}") mklines.Check() @@ -516,6 +555,24 @@ func (s *Suite) Test_MkLines_Check__unbalanced_directives(c *check.C) { "ERROR: opsys.mk:6: Directive indentation is not 0, but 8.") } +func (s *Suite) Test_MkLines_Check__incomplete_subst_at_end(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wall") + t.SetupVartypes() + mklines := t.NewMkLines("subst.mk", + MkRcsID, + "", + "SUBST_CLASSES+=\tclass") + + mklines.Check() + + t.CheckOutputLines( + "WARN: subst.mk:EOF: Incomplete SUBST block: SUBST_STAGE.class missing.", + "WARN: subst.mk:EOF: Incomplete SUBST block: SUBST_FILES.class missing.", + "WARN: subst.mk:EOF: Incomplete SUBST block: SUBST_SED.class, SUBST_VARS.class or SUBST_FILTER_CMD.class missing.") +} + // Demonstrates how to define your own make(1) targets for creating // files in the current directory. The pkgsrc-wip category Makefile // does this, while all other categories don't need any custom code. @@ -524,7 +581,7 @@ func (s *Suite) Test_MkLines__wip_category_Makefile(c *check.C) { t.SetupCommandLine("-Wall", "--explain") t.SetupVartypes() - t.SetupToolUsable("rm", "RM") + t.SetupTool("rm", "RM", AtRunTime) t.CreateFileLines("mk/misc/category.mk") mklines := t.SetupFileMkLines("wip/Makefile", MkRcsID, @@ -566,7 +623,7 @@ func (s *Suite) Test_MkLines_determineDocumentedVariables(c *check.C) { t.SetupCommandLine("-Wall") t.SetupVartypes() - t.SetupToolUsable("rm", "RM") + t.SetupTool("rm", "RM", AtRunTime) mklines := t.NewMkLines("Makefile", MkRcsID, "#", @@ -896,3 +953,39 @@ func (s *Suite) Test_MkLines_Check__hacks_mk(c *check.C) { // No warning about including bsd.prefs.mk before using the ?= operator. t.CheckOutputEmpty() } + +func (s *Suite) Test_MkLines_ForEach__conditional_variables(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wall,no-space") + t.SetupVartypes() + mklines := t.NewMkLines("conditional.mk", + MkRcsID, + "", + ".if defined(PKG_DEVELOPER)", + "DEVELOPER=\tyes", + ".endif", + "", + ".if ${USE_TOOLS:Mgettext}", + "USES_GETTEXT=\tyes", + ".endif") + + seenDeveloper := false + seenUsesGettext := false + + mklines.ForEach(func(mkline MkLine) { + if mkline.IsVarassign() { + switch mkline.Varname() { + case "DEVELOPER": + c.Check(mklines.indentation.IsConditional(), equals, true) + seenDeveloper = true + case "USES_GETTEXT": + c.Check(mklines.indentation.IsConditional(), equals, true) + seenUsesGettext = true + } + } + }) + + c.Check(seenDeveloper, equals, true) + c.Check(seenUsesGettext, equals, true) +} diff --git a/pkgtools/pkglint/files/mkparser.go b/pkgtools/pkglint/files/mkparser.go index cac6856c630..37e04c1984b 100644 --- a/pkgtools/pkglint/files/mkparser.go +++ b/pkgtools/pkglint/files/mkparser.go @@ -217,7 +217,8 @@ func (p *MkParser) MkCond() MkCond { ands := []MkCond{and} for { mark := p.repl.Mark() - if !p.repl.AdvanceRegexp(`^\s*\|\|\s*`) { + p.repl.SkipHspace() + if !p.repl.AdvanceStr("||") { break } next := p.mkCondAnd() @@ -265,7 +266,7 @@ func (p *MkParser) mkCondAtom() MkCond { repl := p.repl mark := repl.Mark() - repl.SkipSpace() + repl.SkipHspace() switch { case repl.AdvanceStr("!"): cond := p.mkCondAtom() @@ -275,25 +276,25 @@ func (p *MkParser) mkCondAtom() MkCond { case repl.AdvanceStr("("): cond := p.MkCond() if cond != nil { - repl.SkipSpace() + repl.SkipHspace() if repl.AdvanceStr(")") { return cond } } - case repl.AdvanceRegexp(`^defined\s*\(`): + case repl.HasPrefix("defined") && repl.AdvanceRegexp(`^defined\s*\(`): if varname := p.Varname(); varname != "" { if repl.AdvanceStr(")") { return &mkCond{Defined: varname} } } - case repl.AdvanceRegexp(`^empty\s*\(`): + case repl.HasPrefix("empty") && repl.AdvanceRegexp(`^empty\s*\(`): if varname := p.Varname(); varname != "" { modifiers := p.VarUseModifiers(varname, ")") if repl.AdvanceStr(")") { return &mkCond{Empty: &MkVarUse{varname, modifiers}} } } - case repl.AdvanceRegexp(`^(commands|exists|make|target)\s*\(`): + case uint(repl.PeekByte()-'a') <= 'z'-'a' && repl.AdvanceRegexp(`^(commands|exists|make|target)\s*\(`): funcname := repl.Group(1) argMark := repl.Mark() for p.VarUse() != nil || repl.AdvanceRegexp(`^[^$)]+`) { @@ -402,6 +403,7 @@ type MkCondCallback struct { CompareVarStr func(varuse *MkVarUse, op string, str string) CompareVarVar func(left *MkVarUse, op string, right *MkVarUse) Call func(name string, arg string) + VarUse func(varuse *MkVarUse) } type MkCondWalker struct{} @@ -425,25 +427,42 @@ func (w *MkCondWalker) Walk(cond MkCond, callback *MkCondCallback) { if callback.Defined != nil { callback.Defined(cond.Defined) } + if callback.VarUse != nil { + callback.VarUse(&MkVarUse{cond.Defined, nil}) + } case cond.Empty != nil: if callback.Empty != nil { callback.Empty(cond.Empty) } + if callback.VarUse != nil { + callback.VarUse(cond.Empty) + } case cond.CompareVarVar != nil: if callback.CompareVarVar != nil { cvv := cond.CompareVarVar callback.CompareVarVar(cvv.Left, cvv.Op, cvv.Right) } + if callback.VarUse != nil { + cvv := cond.CompareVarVar + callback.VarUse(cvv.Left) + callback.VarUse(cvv.Right) + } case cond.CompareVarStr != nil: if callback.CompareVarStr != nil { cvs := cond.CompareVarStr callback.CompareVarStr(cvs.Var, cvs.Op, cvs.Str) } + if callback.VarUse != nil { + callback.VarUse(cond.CompareVarStr.Var) + } case cond.CompareVarNum != nil: if callback.CompareVarNum != nil { cvn := cond.CompareVarNum callback.CompareVarNum(cvn.Var, cvn.Op, cvn.Num) } + if callback.VarUse != nil { + callback.VarUse(cond.CompareVarNum.Var) + } case cond.Call != nil: if callback.Call != nil { call := cond.Call diff --git a/pkgtools/pkglint/files/mkparser_test.go b/pkgtools/pkglint/files/mkparser_test.go index a917a82dfce..21bf6d1da86 100644 --- a/pkgtools/pkglint/files/mkparser_test.go +++ b/pkgtools/pkglint/files/mkparser_test.go @@ -1,7 +1,9 @@ package main import ( + "fmt" "gopkg.in/check.v1" + "strings" ) func (s *Suite) Test_MkParser_MkTokens(c *check.C) { @@ -241,7 +243,7 @@ func (s *Suite) Test_MkParser_MkCond(c *check.C) { " || defined(PKG_OPTIONS:Msamplerate)") checkRest("${LEFT} &&", &mkCond{Not: &mkCond{Empty: varuse("LEFT")}}, - " &&") + "&&") checkRest("\"unfinished string literal", nil, "\"unfinished string literal") @@ -270,3 +272,61 @@ func (s *Suite) Test_MkParser__varuse_parentheses_autofix(c *check.C) { MkRcsID, "COMMENT=${P1} ${P2}) ${P3:Q} ${BRACES}") } + +func (s *Suite) Test_MkCondWalker_Walk(c *check.C) { + t := s.Init(c) + + mkline := t.NewMkLine("Makefile", 4, ""+ + ".if ${VAR:Mmatch} == ${OTHER} || "+ + "${STR} == Str || "+ + "${NUM} == 3 && "+ + "defined(VAR) && "+ + "!exists(file.mk) && "+ + "(((${NONEMPTY})))") + var events []string + + varuseStr := func(varuse *MkVarUse) string { + return strings.Join(append([]string{varuse.varname}, varuse.modifiers...), ":") + } + + addEvent := func(name string, args ...string) { + events = append(events, fmt.Sprintf("%14s %s", name, strings.Join(args, ", "))) + } + + NewMkCondWalker().Walk(mkline.Cond(), &MkCondCallback{ + Defined: func(varname string) { + addEvent("defined", varname) + }, + Empty: func(varuse *MkVarUse) { + addEvent("empty", varuseStr(varuse)) + }, + CompareVarNum: func(varuse *MkVarUse, op string, num string) { + addEvent("compareVarNum", varuseStr(varuse), num) + }, + CompareVarStr: func(varuse *MkVarUse, op string, str string) { + addEvent("compareVarStr", varuseStr(varuse), str) + }, + CompareVarVar: func(left *MkVarUse, op string, right *MkVarUse) { + addEvent("compareVarVar", varuseStr(left), varuseStr(right)) + }, + Call: func(name string, arg string) { + addEvent("call", name, arg) + }, + VarUse: func(varuse *MkVarUse) { + addEvent("varUse", varuseStr(varuse)) + }}) + + c.Check(events, deepEquals, []string{ + " compareVarVar VAR:Mmatch, OTHER", + " varUse VAR:Mmatch", + " varUse OTHER", + " compareVarStr STR, Str", + " varUse STR", + " compareVarNum NUM, 3", + " varUse NUM", + " defined VAR", + " varUse VAR", + " call exists, file.mk", + " empty NONEMPTY", + " varUse NONEMPTY"}) +} diff --git a/pkgtools/pkglint/files/options.go b/pkgtools/pkglint/files/options.go index 779f3d303b6..1c32edc49b7 100755 --- a/pkgtools/pkglint/files/options.go +++ b/pkgtools/pkglint/files/options.go @@ -37,7 +37,7 @@ loop: case mkline.IsVarassign(): switch mkline.Varcanon() { case "PKG_SUPPORTED_OPTIONS", "PKG_OPTIONS_GROUP.*", "PKG_OPTIONS_SET.*": - for _, option := range splitOnSpace(mkline.Value()) { + for _, option := range fields(mkline.Value()) { if !containsVarRef(option) { declaredOptions[option] = mkline optionsInDeclarationOrder = append(optionsInDeclarationOrder, option) @@ -68,7 +68,7 @@ loop: for ; !exp.EOF(); exp.Advance() { mkline := exp.CurrentMkLine() if mkline.IsDirective() && (mkline.Directive() == "if" || mkline.Directive() == "elif") { - cond := NewMkParser(mkline.Line, mkline.Args(), false).MkCond() + cond := mkline.Cond() if cond == nil { continue } diff --git a/pkgtools/pkglint/files/options_test.go b/pkgtools/pkglint/files/options_test.go index 9828f652708..6f831613d05 100755 --- a/pkgtools/pkglint/files/options_test.go +++ b/pkgtools/pkglint/files/options_test.go @@ -54,6 +54,7 @@ func (s *Suite) Test_ChecklinesOptionsMk(c *check.C) { ChecklinesOptionsMk(mklines) t.CheckOutputLines( + "WARN: ~/category/package/options.mk:6: l is used but not defined.", "WARN: ~/category/package/options.mk:18: Unknown option \"undeclared\".", "NOTE: ~/category/package/options.mk:21: The positive branch of the .if/.else should be the one where the option is set.", "WARN: ~/category/package/options.mk:6: Option \"mc-charset\" should be handled below in an .if block.", diff --git a/pkgtools/pkglint/files/package.go b/pkgtools/pkglint/files/package.go index 297fe3a60b7..e496311a5de 100644 --- a/pkgtools/pkglint/files/package.go +++ b/pkgtools/pkglint/files/package.go @@ -169,12 +169,14 @@ func (pkglint *Pkglint) checkdirPackage(dir string) { if pkg.DistinfoFile != pkg.vars.fallback["DISTINFO_FILE"] { files = append(files, pkg.File(pkg.DistinfoFile)) } + haveDistinfo := false havePatches := false // Determine the used variables and PLIST directories before checking any of the Makefile fragments. for _, fname := range files { - if (hasPrefix(path.Base(fname), "Makefile.") || hasSuffix(fname, ".mk")) && + basename := path.Base(fname) + if (hasPrefix(basename, "Makefile.") || hasSuffix(fname, ".mk")) && !matches(fname, `patch-`) && !contains(fname, pkg.Pkgdir+"/") && !contains(fname, pkg.Filesdir+"/") { @@ -182,7 +184,7 @@ func (pkglint *Pkglint) checkdirPackage(dir string) { mklines.DetermineUsedVariables() } } - if hasPrefix(path.Base(fname), "PLIST") { + if hasPrefix(basename, "PLIST") { pkg.loadPlistDirs(fname) } } @@ -194,9 +196,10 @@ func (pkglint *Pkglint) checkdirPackage(dir string) { } continue } - if fname == pkg.File("Makefile") { + + if path.Base(fname) == "Makefile" { if st, err := os.Lstat(fname); err == nil { - pkglint.checkExecutable(st) + pkglint.checkExecutable(fname, st) } if G.opts.CheckMakefile { pkg.checkfilePackageMakefile(fname, lines) @@ -217,14 +220,6 @@ func (pkglint *Pkglint) checkdirPackage(dir string) { NewLineWhole(pkg.File(pkg.DistinfoFile)).Warnf("File not found. Please run \"%s makepatchsum\".", confMake) } } - - if G.opts.CheckAlternatives { - for _, fname := range files { - if path.Base(fname) == "ALTERNATIVES" { - CheckfileAlternatives(fname, pkg.PlistFiles) - } - } - } } func (pkg *Package) loadPackageMakefile() *MkLines { @@ -246,6 +241,11 @@ func (pkg *Package) loadPackageMakefile() *MkLines { } } + if pkg.vars.Defined("USE_CMAKE") { + mainLines.Tools.defTool("cmake", "", false, AtRunTime) + mainLines.Tools.defTool("cpack", "", false, AtRunTime) + } + allLines.DetermineUsedVariables() allLines.CheckRedundantVariables() @@ -255,11 +255,11 @@ func (pkg *Package) loadPackageMakefile() *MkLines { pkg.Patchdir, _ = pkg.vars.Value("PATCHDIR") // See lang/php/ext.mk - if varIsDefined("PHPEXT_MK") { - if !varIsDefined("USE_PHP_EXT_PATCHES") { + if varIsDefinedSimilar("PHPEXT_MK") { + if !varIsDefinedSimilar("USE_PHP_EXT_PATCHES") { pkg.Patchdir = "patches" } - if varIsDefined("PECL_VERSION") { + if varIsDefinedSimilar("PECL_VERSION") { pkg.DistinfoFile = "distinfo" } else { pkg.IgnoreMissingPatches = true @@ -315,7 +315,7 @@ func (pkg *Package) readMakefile(fname string, mainLines *MkLines, allLines *MkL } if includeFile != "" { - if path.Base(fname) != "buildlink3.mk" { + if mkline.Basename != "buildlink3.mk" { if m, bl3File := match1(includeFile, `^\.\./\.\./(.*)/buildlink3\.mk$`); m { pkg.bl3[bl3File] = mkline.Line if trace.Tracing { @@ -333,7 +333,7 @@ func (pkg *Package) readMakefile(fname string, mainLines *MkLines, allLines *MkL mkline.ExplainRelativeDirs() } - if path.Base(fname) == "Makefile" && !hasPrefix(incDir, "../../mk/") && incBase != "buildlink3.mk" && incBase != "builtin.mk" && incBase != "options.mk" { + if mkline.Basename == "Makefile" && !hasPrefix(incDir, "../../mk/") && incBase != "buildlink3.mk" && incBase != "builtin.mk" && incBase != "options.mk" { if trace.Tracing { trace.Step1("Including %q sets seenMakefileCommon.", includeFile) } diff --git a/pkgtools/pkglint/files/package_test.go b/pkgtools/pkglint/files/package_test.go index 0477e347fe2..715ae80f6da 100644 --- a/pkgtools/pkglint/files/package_test.go +++ b/pkgtools/pkglint/files/package_test.go @@ -395,7 +395,7 @@ func (s *Suite) Test_Package_loadPackageMakefile__dump(c *check.C) { t.CheckOutputLines( "Whole Makefile (with all included files) follows:", - "~/category/package/Makefile:1: # $NetBSD: package_test.go,v 1.30 2018/10/03 22:27:53 rillig Exp $", + "~/category/package/Makefile:1: "+MkRcsID, "~/category/package/Makefile:2: ", "~/category/package/Makefile:3: CATEGORIES=category", "~/category/package/Makefile:4: ", @@ -489,7 +489,7 @@ func (s *Suite) Test_Package__varuse_at_load_time(c *check.C) { t := s.Init(c) t.SetupPkgsrc() - t.SetupToolUsable("printf", "") + t.SetupTool("printf", "", AtRunTime) t.CreateFileLines("licenses/2-clause-bsd", "# dummy") t.CreateFileLines("misc/Makefile") @@ -1054,7 +1054,7 @@ func (s *Suite) Test_Pkglint_checkdirPackage__filename_with_variable(c *check.C) // Pkglint cannot currently resolve the location of DISTINFO_FILE completely // because the variable \"rv\" comes from a .for loop. // - // TODO: resolve variables in simple .for loops like the above. + // TODO: iterate over variables in simple .for loops like the above. G.CheckDirent(pkg) t.CheckOutputEmpty() diff --git a/pkgtools/pkglint/files/patches_test.go b/pkgtools/pkglint/files/patches_test.go index 6f6ded8e407..7c62d0c7c0c 100644 --- a/pkgtools/pkglint/files/patches_test.go +++ b/pkgtools/pkglint/files/patches_test.go @@ -686,14 +686,14 @@ func (s *Suite) Test_PatchChecker_checktextRcsid(c *check.C) { " $"+"Id$", "-old line", "+new line", - " $Author: rillig $") + " $"+"Author: authorship $") ChecklinesPatch(lines) t.CheckOutputLines( - "WARN: ~/patch-aa:7: Found RCS tag \"$Id: patches_test.go,v 1.21 2018/10/03 22:27:53 rillig Exp $\". Please remove it.", - "WARN: ~/patch-aa:8: Found RCS tag \"$Id: patches_test.go,v 1.21 2018/10/03 22:27:53 rillig Exp $\". Please remove it by reducing the number of context lines using pkgdiff or \"diff -U[210]\".", - "WARN: ~/patch-aa:11: Found RCS tag \"$Author: rillig $\". Please remove it by reducing the number of context lines using pkgdiff or \"diff -U[210]\".") + "WARN: ~/patch-aa:7: Found RCS tag \"$"+"Id$\". Please remove it.", + "WARN: ~/patch-aa:8: Found RCS tag \"$"+"Id$\". Please remove it by reducing the number of context lines using pkgdiff or \"diff -U[210]\".", + "WARN: ~/patch-aa:11: Found RCS tag \"$"+"Author$\". Please remove it by reducing the number of context lines using pkgdiff or \"diff -U[210]\".") } func (s *Suite) Test_FileType_String(c *check.C) { diff --git a/pkgtools/pkglint/files/pkglint.go b/pkgtools/pkglint/files/pkglint.go index 4cfe04a7526..f04c647da01 100644 --- a/pkgtools/pkglint/files/pkglint.go +++ b/pkgtools/pkglint/files/pkglint.go @@ -59,7 +59,7 @@ type Pkglint struct { func NewPkglint() Pkglint { return Pkglint{ res: regex.NewRegistry(), - fileCache: NewFileCache(100)} + fileCache: NewFileCache(200)} } type CmdOpts struct { @@ -546,7 +546,7 @@ func (pkglint *Pkglint) Checkfile(fname string) { return } - pkglint.checkExecutable(st) + pkglint.checkExecutable(fname, st) pkglint.checkMode(fname, st.Mode()) } @@ -575,7 +575,11 @@ func (pkglint *Pkglint) checkMode(fname string, mode os.FileMode) { case basename == "ALTERNATIVES": if pkglint.opts.CheckAlternatives { - CheckfileAlternatives(fname, nil) + var plistFiles map[string]bool + if G.Pkg != nil { + plistFiles = G.Pkg.PlistFiles + } + CheckfileAlternatives(fname, plistFiles) } case basename == "buildlink3.mk": @@ -665,9 +669,20 @@ func (pkglint *Pkglint) checkMode(fname string, mode os.FileMode) { } } -func (pkglint *Pkglint) checkExecutable(st os.FileInfo) { - fname := st.Name() - if st.Mode().IsRegular() && st.Mode().Perm()&0111 != 0 && !isCommitted(fname) { +func (pkglint *Pkglint) checkExecutable(fname string, st os.FileInfo) { + switch { + case !st.Mode().IsRegular(): + // Directories and other entries may be executable. + + case st.Mode().Perm()&0111 == 0: + // Good. + + case isCommitted(fname): + // Too late to be fixed by the package developer, since + // CVS remembers the executable bit in the repo file. + // At this point, it can only be reset by the CVS admins. + + default: line := NewLine(fname, 0, "", nil) fix := line.Autofix() fix.Warnf("Should not be executable.") @@ -709,33 +724,13 @@ func (pkglint *Pkglint) Tool(command string, time ToolTime) (tool *Tool, usable varname = toolVarname } - if G.Mk != nil { - tools := G.Mk.Tools - if t := tools.ByName(command); t != nil { - if tools.Usable(t, time) { - return t, true - } - tool = t - } - - if t := tools.ByVarname(varname); t != nil { - if tools.Usable(t, time) { - return t, true - } - if tool == nil { - tool = t - } - } - } + tools := pkglint.tools() - tools := G.Pkgsrc.Tools if t := tools.ByName(command); t != nil { if tools.Usable(t, time) { return t, true } - if tool == nil { - tool = t - } + tool = t } if t := tools.ByVarname(varname); t != nil { @@ -746,7 +741,6 @@ func (pkglint *Pkglint) Tool(command string, time ToolTime) (tool *Tool, usable tool = t } } - return } @@ -756,27 +750,13 @@ func (pkglint *Pkglint) Tool(command string, time ToolTime) (tool *Tool, usable // current package. It is not guaranteed to be usable; that must be // checked by the calling code. func (pkglint *Pkglint) ToolByVarname(varname string, time ToolTime) *Tool { + return pkglint.tools().ByVarname(varname) +} - var tool *Tool +func (pkglint *Pkglint) tools() *Tools { if G.Mk != nil { - tools := G.Mk.Tools - if t := tools.ByVarname(varname); t != nil { - if tools.Usable(t, time) { - return t - } - tool = t - } - } - - tools := G.Pkgsrc.Tools - if t := tools.ByVarname(varname); t != nil { - if tools.Usable(t, time) { - return t - } - if tool == nil { - tool = t - } + return G.Mk.Tools + } else { + return G.Pkgsrc.Tools } - - return tool } diff --git a/pkgtools/pkglint/files/pkglint_test.go b/pkgtools/pkglint/files/pkglint_test.go index 6424673a25b..99136c06d17 100644 --- a/pkgtools/pkglint/files/pkglint_test.go +++ b/pkgtools/pkglint/files/pkglint_test.go @@ -2,6 +2,7 @@ package main import ( "io/ioutil" + "path" "strings" "time" @@ -259,7 +260,7 @@ func (s *Suite) Test_Pkglint__coverage(c *check.C) { cmdline := os.Getenv("PKGLINT_TESTCMDLINE") if cmdline != "" { G.logOut, G.logErr, trace.Out = NewSeparatorWriter(os.Stdout), NewSeparatorWriter(os.Stderr), os.Stdout - G.Main(append([]string{"pkglint"}, splitOnSpace(cmdline)...)...) + G.Main(append([]string{"pkglint"}, fields(cmdline)...)...) } } @@ -578,9 +579,9 @@ func (s *Suite) Test_Pkglint_Tool__lookup_by_name_fallback(c *check.C) { loadTimeTool, loadTimeUsable := G.Tool("tool", LoadTime) runTimeTool, runTimeUsable := G.Tool("tool", RunTime) - c.Check(loadTimeTool, equals, global) + c.Check(*loadTimeTool, equals, *global) c.Check(loadTimeUsable, equals, false) - c.Check(runTimeTool, equals, global) + c.Check(*runTimeTool, equals, *global) c.Check(runTimeUsable, equals, false) } @@ -607,16 +608,14 @@ func (s *Suite) Test_Pkglint_Tool__lookup_by_varname_fallback(c *check.C) { t := s.Init(c) G.Mk = t.NewMkLines("Makefile", MkRcsID) - global := G.Pkgsrc.Tools.Define("tool", "TOOL", dummyMkLine) - - global.Validity = Nowhere + G.Pkgsrc.Tools.defTool("tool", "TOOL", false, Nowhere) loadTimeTool, loadTimeUsable := G.Tool("${TOOL}", LoadTime) runTimeTool, runTimeUsable := G.Tool("${TOOL}", RunTime) - c.Check(loadTimeTool, equals, global) + c.Check(loadTimeTool.String(), equals, "tool:TOOL::Nowhere") c.Check(loadTimeUsable, equals, false) - c.Check(runTimeTool, equals, global) + c.Check(runTimeTool.String(), equals, "tool:TOOL::Nowhere") c.Check(runTimeUsable, equals, false) } @@ -624,16 +623,14 @@ func (s *Suite) Test_Pkglint_Tool__lookup_by_varname_fallback_runtime(c *check.C t := s.Init(c) G.Mk = t.NewMkLines("Makefile", MkRcsID) - global := G.Pkgsrc.Tools.Define("tool", "TOOL", dummyMkLine) - - global.Validity = AtRunTime + G.Pkgsrc.Tools.defTool("tool", "TOOL", false, AtRunTime) loadTimeTool, loadTimeUsable := G.Tool("${TOOL}", LoadTime) runTimeTool, runTimeUsable := G.Tool("${TOOL}", RunTime) - c.Check(loadTimeTool, equals, global) + c.Check(loadTimeTool.String(), equals, "tool:TOOL::AtRunTime") c.Check(loadTimeUsable, equals, false) - c.Check(runTimeTool, equals, global) + c.Check(runTimeTool.String(), equals, "tool:TOOL::AtRunTime") c.Check(runTimeUsable, equals, true) } @@ -651,16 +648,14 @@ func (s *Suite) Test_Pkglint_ToolByVarname__prefer_mk_over_pkgsrc(c *check.C) { c.Check(G.ToolByVarname("TOOL", RunTime), equals, local) } -func (s *Suite) Test_Pkglint_ToolByVarname__fallback(c *check.C) { +func (s *Suite) Test_Pkglint_ToolByVarname(c *check.C) { t := s.Init(c) G.Mk = t.NewMkLines("Makefile", MkRcsID) - global := G.Pkgsrc.Tools.Define("tool", "TOOL", dummyMkLine) + G.Pkgsrc.Tools.defTool("tool", "TOOL", false, AtRunTime) - global.Validity = AtRunTime - - c.Check(G.ToolByVarname("TOOL", LoadTime), equals, global) - c.Check(G.ToolByVarname("TOOL", RunTime), equals, global) + c.Check(G.ToolByVarname("TOOL", LoadTime).String(), equals, "tool:TOOL::AtRunTime") + c.Check(G.ToolByVarname("TOOL", RunTime).String(), equals, "tool:TOOL::AtRunTime") } func (s *Suite) Test_CheckfileExtra(c *check.C) { @@ -775,13 +770,13 @@ func (s *Suite) Test_Pkglint_Checkfile__readme_and_todo(c *check.C) { "", "SHA1 (patch-README) = b9101ebf0bca8ce243ed6433b65555fa6a5ecd52") - // Copy category/package to wip/package. + // Copy category/package/** to wip/package. for _, basename := range []string{"files/README", "patches/patch-README", "Makefile", "PLIST", "README", "TODO", "distinfo"} { src := "category/package/" + basename dst := "wip/package/" + basename text, err := ioutil.ReadFile(t.File(src)) c.Check(err, check.IsNil) - t.CreateFileLines(dst, strings.TrimSpace(string(text))) + t.CreateFileLines(dst, strings.TrimSuffix(string(text), "\n")) } t.SetupPkgsrc() @@ -887,18 +882,21 @@ func (s *Suite) Test_CheckfileMk__enoent(c *check.C) { func (s *Suite) Test_Pkglint_checkExecutable(c *check.C) { t := s.Init(c) - G.checkExecutable(ExecutableFileInfo{t.File("fname.mk")}) + fileName := t.File("file.mk") + fileInfo := ExecutableFileInfo{path.Base(fileName)} + + G.checkExecutable(fileName, fileInfo) t.CheckOutputLines( - "WARN: ~/fname.mk: Should not be executable.") + "WARN: ~/file.mk: Should not be executable.") t.SetupCommandLine("--autofix") - G.checkExecutable(ExecutableFileInfo{t.File("fname.mk")}) + G.checkExecutable(fileName, fileInfo) // FIXME: The error message "Cannot clear executable bits" is swallowed. t.CheckOutputLines( - "AUTOFIX: ~/fname.mk: Clearing executable bits") + "AUTOFIX: ~/file.mk: Clearing executable bits") } func (s *Suite) Test_main(c *check.C) { diff --git a/pkgtools/pkglint/files/pkgsrc.go b/pkgtools/pkglint/files/pkgsrc.go index 00b8a50188b..3d29c5af6a9 100644 --- a/pkgtools/pkglint/files/pkgsrc.go +++ b/pkgtools/pkglint/files/pkgsrc.go @@ -4,6 +4,8 @@ import ( "io/ioutil" "netbsd.org/pkglint/regex" "netbsd.org/pkglint/trace" + "os" + "path/filepath" "sort" "strconv" "strings" @@ -21,7 +23,7 @@ type Pkgsrc struct { // within the bsd.pkg.mk file. buildDefs map[string]bool - Tools Tools + Tools *Tools MasterSiteURLToVar map[string]string // "https://github.com/" => "MASTER_SITE_GITHUB" MasterSiteVarToURL map[string]string // "MASTER_SITE_GITHUB" => "https://github.com/" @@ -62,13 +64,30 @@ func NewPkgsrc(dir string) *Pkgsrc { // Some user-defined variables do not influence the binary // package at all and therefore do not have to be added to // BUILD_DEFS; therefore they are marked as "already added". - src.AddBuildDefs("DISTDIR", "FETCH_CMD", "FETCH_OUTPUT_ARGS") + src.AddBuildDefs( + "DISTDIR", + "FETCH_CMD", + "FETCH_OUTPUT_ARGS", + "FETCH_USING", + "PKGSRC_RUN_TEST") + + // The following variables are used so often that not every + // package should need to add it to BUILD_DEFS manually. + src.AddBuildDefs( + "PKGSRC_COMPILER", + "PKGSRC_USE_SSP", + "UNPRIVILEGED", + "USE_CROSS_COMPILE") + + // The following variables are so obscure that they are + // probably not used in practice. + src.AddBuildDefs( + "MANINSTALL") // The following variables are added to _BUILD_DEFS by the pkgsrc // infrastructure and thus don't need to be added by the package again. // To regenerate the below list: // grep -hr '^_BUILD_DEFS+=' mk/ | tr ' \t' '\n\n' | sed -e 's,.*=,,' -e '/^_/d' -e '/^$/d' -e 's,.*,"&"\,,' | sort -u - src.AddBuildDefs("PKG_HACKS") src.AddBuildDefs( "ABI", "BUILTIN_PKGS", @@ -129,6 +148,7 @@ func (src *Pkgsrc) LoadInfrastructure() { src.loadUserDefinedVars() src.loadTools() src.initDeprecatedVars() + src.loadUntypedVars() } // Latest returns the latest package matching the given pattern. @@ -221,45 +241,38 @@ func (src *Pkgsrc) loadTools() { } } - // TODO: parse bsd.prefs.mk instead of hardcoding this. - toolDefs := []struct { - Name string - Varname string + // TODO: parse bsd.prefs.mk and bsd.pkg.mk instead of hardcoding this. + toolDefs := [...]struct { + Name string + Varname string + Validity Validity }{ - {"echo", "ECHO"}, - {"echo -n", "ECHO_N"}, - {"false", "FALSE"}, - {"test", "TEST"}, - {"true", "TRUE"}} + {"echo", "ECHO", AfterPrefsMk}, + {"echo -n", "ECHO_N", AfterPrefsMk}, + {"false", "FALSE", AtRunTime}, // from bsd.pkg.mk + {"test", "TEST", AfterPrefsMk}, + {"true", "TRUE", AfterPrefsMk}} for _, toolDef := range toolDefs { - tool := tools.Define(toolDef.Name, toolDef.Varname, dummyMkLine) - tool.MustUseVarForm = true - if toolDef.Name != "false" { - tool.SetValidity(AfterPrefsMk, tools.TraceName) - } + tools.defTool(toolDef.Name, toolDef.Varname, true, toolDef.Validity) } for _, basename := range toolFiles { mklines := G.Pkgsrc.LoadMk("mk/tools/"+basename, MustSucceed|NotEmpty) - for _, mkline := range mklines.mklines { - tools.ParseToolLineCreate(mkline, true) - } + mklines.ForEach(func(mkline MkLine) { + tools.ParseToolLine(mkline, true, !mklines.indentation.IsConditional()) + }) } for _, relativeName := range [...]string{"mk/bsd.prefs.mk", "mk/bsd.pkg.mk"} { mklines := G.Pkgsrc.LoadMk(relativeName, MustSucceed|NotEmpty) - for _, mkline := range mklines.mklines { + mklines.ForEach(func(mkline MkLine) { if mkline.IsVarassign() { - switch mkline.Varname() { + varname := mkline.Varname() + switch varname { case "USE_TOOLS": - // Since this line is in the pkgsrc infrastructure, each tool mentioned - // in USE_TOOLS is trusted to be also defined somewhere in the actual - // list of available tools. - // - // This assumption does not work for processing USE_TOOLS in packages, though. - tools.ParseToolLineCreate(mkline, true) + tools.ParseToolLine(mkline, true, !mklines.indentation.IsConditional()) case "_BUILD_DEFS": for _, bdvar := range mkline.ValueSplit(mkline.Value(), "") { @@ -267,7 +280,7 @@ func (src *Pkgsrc) loadTools() { } } } - } + }) } if trace.Tracing { @@ -275,6 +288,56 @@ func (src *Pkgsrc) loadTools() { } } +// loadUntypedVars scans all pkgsrc infrastructure files in mk/ +// to find variable definitions that are not yet covered in +// Pkgsrc.InitVartypes. +// +// Even if pkglint cannot guess the type of each variable, +// at least prevent the "used but not defined" warnings. +func (src *Pkgsrc) loadUntypedVars() { + + // Setting guessed to false prevents the vartype.guessed case in MkLineChecker.CheckVaruse. + unknownType := &Vartype{lkNone, BtUnknown, []ACLEntry{{"*", aclpAll}}, false} + + handleLine := func(mkline MkLine) { + if mkline.IsVarassign() { + varcanon := mkline.Varcanon() + + switch { + case + src.vartypes[varcanon] != nil, // Already defined + src.Tools.ByVarname(varcanon) != nil, // Already known as a tool + hasPrefix(varcanon, "_"), // Skip internal variables + contains(varcanon, "$"), // Indirect or parameterized + hasSuffix(varcanon, "_MK"): // Multiple-inclusion guard + + default: + if trace.Tracing { + trace.Stepf("Untyped variable %q in %s", varcanon, mkline) + } + src.vartypes[varcanon] = unknownType + } + } + } + + handleMkFile := func(path string) { + mklines := LoadMk(path, 0) + if mklines != nil { + mklines.ForEach(handleLine) + } + } + + handleFile := func(pathName string, info os.FileInfo, err error) error { + baseName := info.Name() + if hasSuffix(baseName, ".mk") || baseName == "mk.conf" { + handleMkFile(filepath.ToSlash(pathName)) + } + return nil + } + + _ = filepath.Walk(src.File("mk"), handleFile) +} + func (src *Pkgsrc) parseSuggestedUpdates(lines []Line) []SuggestedUpdate { var updates []SuggestedUpdate state := 0 @@ -633,7 +696,7 @@ func (src *Pkgsrc) loadMasterSites() { if mkline.IsVarassign() { varname := mkline.Varname() if hasPrefix(varname, "MASTER_SITE_") && varname != "MASTER_SITE_BACKUP" { - for _, url := range splitOnSpace(mkline.Value()) { + for _, url := range fields(mkline.Value()) { if matches(url, `^(?:http://|https://|ftp://)`) { if nameToURL[varname] == "" { nameToURL[varname] = url @@ -641,6 +704,8 @@ func (src *Pkgsrc) loadMasterSites() { urlToName[url] = varname } } + // TODO: register variable type, to avoid redundant + // definitions in vardefs.go. } } } diff --git a/pkgtools/pkglint/files/pkgsrc_test.go b/pkgtools/pkglint/files/pkgsrc_test.go index f9ad0035ee8..8baf7900d71 100644 --- a/pkgtools/pkglint/files/pkgsrc_test.go +++ b/pkgtools/pkglint/files/pkgsrc_test.go @@ -74,7 +74,9 @@ func (s *Suite) Test_Pkgsrc_loadTools(c *check.C) { t.CreateFileLines("mk/tools/flex.mk", "# empty") t.CreateFileLines("mk/tools/gettext.mk", - "USE_TOOLS+=msgfmt", + ".if ${USE_TOOLS:Mgettext}", // This conditional prevents msgfmt from + "USE_TOOLS+=msgfmt", // being added to the default USE_TOOLS. + ".endif", "TOOLS_CREATE+=msgfmt") t.CreateFileLines("mk/tools/strip.mk", ".if defined(_INSTALL_UNSTRIPPED) || !defined(TOOLS_PLATFORM.strip)", @@ -102,20 +104,20 @@ func (s *Suite) Test_Pkgsrc_loadTools(c *check.C) { t.CheckOutputLines( "TRACE: + (*Tools).Trace(\"Pkgsrc\")", - "TRACE: 1 tool &{Name:bzcat Varname: MustUseVarForm:false Validity:Nowhere}", - "TRACE: 1 tool &{Name:bzip2 Varname: MustUseVarForm:false Validity:Nowhere}", - "TRACE: 1 tool &{Name:chown Varname:CHOWN MustUseVarForm:false Validity:Nowhere}", - "TRACE: 1 tool &{Name:echo Varname:ECHO MustUseVarForm:true Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:echo -n Varname:ECHO_N MustUseVarForm:true Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:false Varname:FALSE MustUseVarForm:true Validity:Nowhere}", - "TRACE: 1 tool &{Name:gawk Varname:AWK MustUseVarForm:false Validity:Nowhere}", - "TRACE: 1 tool &{Name:m4 Varname: MustUseVarForm:false Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:msgfmt Varname: MustUseVarForm:false Validity:Nowhere}", - "TRACE: 1 tool &{Name:mv Varname:MV MustUseVarForm:false Validity:AtRunTime}", - "TRACE: 1 tool &{Name:pwd Varname:PWD MustUseVarForm:false Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:strip Varname: MustUseVarForm:false Validity:Nowhere}", - "TRACE: 1 tool &{Name:test Varname:TEST MustUseVarForm:true Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:true Varname:TRUE MustUseVarForm:true Validity:AfterPrefsMk}", + "TRACE: 1 tool bzcat:::Nowhere", + "TRACE: 1 tool bzip2:::Nowhere", + "TRACE: 1 tool chown:CHOWN::Nowhere", + "TRACE: 1 tool echo:ECHO:var:AfterPrefsMk", + "TRACE: 1 tool echo -n:ECHO_N:var:AfterPrefsMk", + "TRACE: 1 tool false:FALSE:var:AtRunTime", + "TRACE: 1 tool gawk:AWK::Nowhere", + "TRACE: 1 tool m4:::AfterPrefsMk", + "TRACE: 1 tool msgfmt:::Nowhere", + "TRACE: 1 tool mv:MV::AtRunTime", + "TRACE: 1 tool pwd:PWD::AfterPrefsMk", + "TRACE: 1 tool strip:::Nowhere", + "TRACE: 1 tool test:TEST:var:AfterPrefsMk", + "TRACE: 1 tool true:TRUE:var:AfterPrefsMk", "TRACE: - (*Tools).Trace(\"Pkgsrc\")") } @@ -124,7 +126,7 @@ func (s *Suite) Test_Pkgsrc_loadTools__BUILD_DEFS(c *check.C) { t := s.Init(c) t.SetupCommandLine("-Wall") - t.SetupToolUsable("echo", "ECHO") + t.SetupTool("echo", "ECHO", AtRunTime) pkg := t.SetupPackage("category/package", "pre-configure:", "\t@${ECHO} ${PKG_SYSCONFDIR} ${VARBASE}") diff --git a/pkgtools/pkglint/files/plist.go b/pkgtools/pkglint/files/plist.go index fe2417119fb..1238d3a5e4b 100644 --- a/pkgtools/pkglint/files/plist.go +++ b/pkgtools/pkglint/files/plist.go @@ -53,8 +53,8 @@ func (ck *PlistChecker) Check(plainLines []Line) { plines := ck.NewLines(plainLines) ck.collectFilesAndDirs(plines) - if fname := plines[0].Filename; path.Base(fname) == "PLIST.common_end" { - commonLines := Load(strings.TrimSuffix(fname, "_end"), NotEmpty) + if plines[0].Basename == "PLIST.common_end" { + commonLines := Load(strings.TrimSuffix(plines[0].Filename, "_end"), NotEmpty) if commonLines != nil { ck.collectFilesAndDirs(ck.NewLines(commonLines)) } @@ -429,7 +429,7 @@ func (pline *PlistLine) CheckDirective(cmd, arg string) { "command in the PLIST") case "imake-man": - args := splitOnSpace(arg) + args := fields(arg) switch { case len(args) != 3: pline.Warnf("Invalid number of arguments for imake-man.") diff --git a/pkgtools/pkglint/files/shell.go b/pkgtools/pkglint/files/shell.go index 11dddfb10e6..c2d3c5cee1a 100644 --- a/pkgtools/pkglint/files/shell.go +++ b/pkgtools/pkglint/files/shell.go @@ -6,7 +6,6 @@ import ( "netbsd.org/pkglint/textproc" "netbsd.org/pkglint/trace" "path" - "strings" ) const ( @@ -165,7 +164,7 @@ outer: } } - if strings.TrimSpace(parser.Rest()) != "" { + if trimHspace(parser.Rest()) != "" { line.Warnf("Pkglint parse error in ShellLine.CheckWord at %q (quoting=%s), rest: %s", token, quoting, parser.Rest()) } } @@ -283,7 +282,7 @@ func (shline *ShellLine) CheckShellCommandLine(shelltext string) { "to understand, since all the complexity of using sed and mv is", "hidden behind the scenes.", "", - "Run \"bmake help topic=subst\" for more information.") + "Run \""+confMake+" help topic=subst\" for more information.") if contains(shelltext, "#") { Explain( "When migrating to the SUBST framework, pay attention to \"#\"", @@ -304,7 +303,7 @@ func (shline *ShellLine) CheckShellCommandLine(shelltext string) { } repl := G.NewPrefixReplacer(shelltext) - repl.AdvanceRegexp(`^\s+`) + repl.SkipHspace() if repl.AdvanceRegexp(`^[-@]+`) { shline.checkHiddenAndSuppress(repl.Group(0), repl.Rest()) } @@ -452,6 +451,8 @@ func (scc *SimpleCommandChecker) checkCommandStart() { } shellword := scc.strcmd.Name + scc.shline.checkInstallCommand(shellword) + switch { case shellword == "${RUN}" || shellword == "": case scc.handleForbiddenCommand(): @@ -487,11 +488,10 @@ func (scc *SimpleCommandChecker) handleTool() bool { scc.shline.mkline.Warnf("The %q tool is used but not added to USE_TOOLS.", command) } - if tool != nil && !containsVarRef(command) && tool.MustUseVarForm { + if tool != nil && tool.MustUseVarForm && !containsVarRef(command) { scc.shline.mkline.Warnf("Please use \"${%s}\" instead of %q.", tool.Varname, command) } - scc.shline.checkCommandUse(command) return tool != nil } @@ -527,12 +527,12 @@ func (scc *SimpleCommandChecker) handleCommandVariable() bool { if tool.Validity == Nowhere { scc.shline.mkline.Warnf("The %q tool is used but not added to USE_TOOLS.", tool.Name) } - scc.shline.checkCommandUse(shellword) + scc.shline.checkInstallCommand(shellword) return true } if vartype := G.Pkgsrc.VariableType(varname); vartype != nil && vartype.basicType.name == "ShellCommand" { - scc.shline.checkCommandUse(shellword) + scc.shline.checkInstallCommand(shellword) return true } @@ -917,7 +917,7 @@ func (spc *ShellProgramChecker) checkSetE(list *MkShList, index int, andor *MkSh } // Some shell commands should not be used in the install phase. -func (shline *ShellLine) checkCommandUse(shellcmd string) { +func (shline *ShellLine) checkInstallCommand(shellcmd string) { if trace.Tracing { defer trace.Call()() } diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go index ed1926eea9c..9b9ece7f5ce 100644 --- a/pkgtools/pkglint/files/shell_test.go +++ b/pkgtools/pkglint/files/shell_test.go @@ -137,10 +137,10 @@ func (s *Suite) Test_ShellLine_CheckShellCommandLine(c *check.C) { t.SetupCommandLine("-Wall") t.SetupVartypes() - t.SetupToolUsable("awk", "AWK") - t.SetupToolUsable("cp", "CP") - t.SetupToolUsable("mkdir", "MKDIR") // This is actually "mkdir -p". - t.SetupToolUsable("unzip", "UNZIP_CMD") + t.SetupTool("awk", "AWK", AtRunTime) + t.SetupTool("cp", "CP", AtRunTime) + t.SetupTool("mkdir", "MKDIR", AtRunTime) // This is actually "mkdir -p". + t.SetupTool("unzip", "UNZIP_CMD", AtRunTime) checkShellCommandLine := func(shellCommand string) { G.Mk = t.NewMkLines("fname", @@ -164,7 +164,7 @@ func (s *Suite) Test_ShellLine_CheckShellCommandLine(c *check.C) { "WARN: fname:1: Unknown shell command \"echo\".", "WARN: fname:1: Unknown shell command \"echo\".") - t.SetupToolUsable("echo", "") + t.SetupTool("echo", "", AtRunTime) t.SetupVartypes() checkShellCommandLine("echo ${PKGNAME:Q}") // vucQuotPlain @@ -323,7 +323,7 @@ func (s *Suite) Test_ShellLine_CheckShellCommandLine__nofix(c *check.C) { t.SetupCommandLine("-Wall") t.SetupVartypes() - t.SetupToolUsable("echo", "") + t.SetupTool("echo", "", AtRunTime) G.Mk = t.NewMkLines("Makefile", "\techo ${PKGNAME:Q}") shline := NewShellLine(G.Mk.mklines[0]) @@ -339,7 +339,7 @@ func (s *Suite) Test_ShellLine_CheckShellCommandLine__show_autofix(c *check.C) { t.SetupCommandLine("-Wall", "--show-autofix") t.SetupVartypes() - t.SetupToolUsable("echo", "") + t.SetupTool("echo", "", AtRunTime) G.Mk = t.NewMkLines("Makefile", "\techo ${PKGNAME:Q}") shline := NewShellLine(G.Mk.mklines[0]) @@ -356,11 +356,11 @@ func (s *Suite) Test_ShellProgramChecker_checkPipeExitcode(c *check.C) { t.SetupCommandLine("-Wall") t.SetupVartypes() - t.SetupToolUsable("cat", "") - t.SetupToolUsable("echo", "") - t.SetupToolUsable("printf", "") - t.SetupToolUsable("sed", "") - t.SetupToolUsable("right-side", "") + t.SetupTool("cat", "", AtRunTime) + t.SetupTool("echo", "", AtRunTime) + t.SetupTool("printf", "", AtRunTime) + t.SetupTool("sed", "", AtRunTime) + t.SetupTool("right-side", "", AtRunTime) G.Mk = t.NewMkLines("Makefile", "\t echo | right-side", "\t sed s,s,s, | right-side", @@ -394,7 +394,7 @@ func (s *Suite) Test_ShellLine_CheckShellCommandLine__autofix(c *check.C) { t.SetupCommandLine("-Wall", "--autofix") t.SetupVartypes() - t.SetupToolUsable("echo", "") + t.SetupTool("echo", "", AtRunTime) G.Mk = t.NewMkLines("Makefile", "\techo ${PKGNAME:Q}") shline := NewShellLine(G.Mk.mklines[0]) @@ -438,7 +438,7 @@ func (s *Suite) Test_ShellLine_CheckShellCommandLine__dollar_without_variable(c t := s.Init(c) t.SetupVartypes() - t.SetupToolUsable("pax", "") + t.SetupTool("pax", "", AtRunTime) G.Mk = t.NewMkLines("fname", "# dummy") shline := NewShellLine(G.Mk.mklines[0]) @@ -614,7 +614,7 @@ func (s *Suite) Test_ShellLine_variableNeedsQuoting(c *check.C) { t.SetupCommandLine("-Wall") t.SetupVartypes() - t.SetupToolUsable("cp", "") + t.SetupTool("cp", "", AtRunTime) mklines := t.NewMkLines("fname.mk", MkRcsID, "", @@ -636,7 +636,7 @@ func (s *Suite) Test_ShellLine_CheckShellCommandLine__echo(c *check.C) { t := s.Init(c) t.SetupCommandLine("-Wall") - echo := t.SetupToolUsable("echo", "ECHO") + echo := t.SetupTool("echo", "ECHO", AtRunTime) echo.MustUseVarForm = true G.Mk = t.NewMkLines("fname", "# dummy") @@ -679,7 +679,7 @@ func (s *Suite) Test_ShellLine_CheckShellCommandLine__shell_variables(c *check.C "WARN: Makefile:3: Please use the RCD_SCRIPTS mechanism to install rc.d scripts automatically to ${RCD_SCRIPTS_EXAMPLEDIR}.") } -func (s *Suite) Test_ShellLine_checkCommandUse(c *check.C) { +func (s *Suite) Test_ShellLine_checkInstallCommand(c *check.C) { t := s.Init(c) G.Mk = t.NewMkLines("fname", @@ -688,12 +688,12 @@ func (s *Suite) Test_ShellLine_checkCommandUse(c *check.C) { shline := t.NewShellLine("fname", 1, "\tdummy") - shline.checkCommandUse("sed") + shline.checkInstallCommand("sed") t.CheckOutputLines( "WARN: fname:1: The shell command \"sed\" should not be used in the install phase.") - shline.checkCommandUse("cp") + shline.checkInstallCommand("cp") t.CheckOutputLines( "WARN: fname:1: ${CP} should not be used to install files.") @@ -950,8 +950,8 @@ func (s *Suite) Test_SimpleCommandChecker_handleForbiddenCommand(c *check.C) { func (s *Suite) Test_SimpleCommandChecker_handleCommandVariable(c *check.C) { t := s.Init(c) - t.SetupToolUsable("perl", "PERL5") - t.SetupTool("perl6", "PERL6") + t.SetupTool("perl", "PERL5", AtRunTime) + t.SetupTool("perl6", "PERL6", Nowhere) mklines := t.NewMkLines("Makefile", MkRcsID, "", @@ -960,13 +960,10 @@ func (s *Suite) Test_SimpleCommandChecker_handleCommandVariable(c *check.C) { mklines.Check() - // FIXME: Warn about using _PERL5_VARS because it starts with an underscore. - // FIXME: Omit the redundant PERL5_VARS_CMD warning in line 4. // FIXME: In PERL5:Q and PERL6:Q, the :Q is wrong. t.CheckOutputLines( "WARN: Makefile:3: PERL5_VARS_CMD is defined but not used.", - "WARN: Makefile:4: The \"perl6\" tool is used but not added to USE_TOOLS.", - "WARN: Makefile:4: PERL5_VARS_CMD is defined but not used.") + "WARN: Makefile:4: The \"perl6\" tool is used but not added to USE_TOOLS.") } // The package Makefile and other .mk files in a package directory @@ -1088,9 +1085,9 @@ func (s *Suite) Test_ShellProgramChecker_checkSetE__simple_commands(c *check.C) t := s.Init(c) t.SetupCommandLine("-Wall") - t.SetupToolUsable("echo", "") - t.SetupToolUsable("rm", "") - t.SetupToolUsable("touch", "") + t.SetupTool("echo", "", AtRunTime) + t.SetupTool("rm", "", AtRunTime) + t.SetupTool("touch", "", AtRunTime) mklines := t.NewMkLines("Makefile", MkRcsID, "pre-configure:", @@ -1108,8 +1105,8 @@ func (s *Suite) Test_ShellProgramChecker_checkSetE__compound_commands(c *check.C t := s.Init(c) t.SetupCommandLine("-Wall") - t.SetupToolUsable("echo", "") - t.SetupToolUsable("touch", "") + t.SetupTool("echo", "", AtRunTime) + t.SetupTool("touch", "", AtRunTime) mklines := t.NewMkLines("Makefile", MkRcsID, "pre-configure:", @@ -1127,12 +1124,12 @@ func (s *Suite) Test_ShellProgramChecker_canFail(c *check.C) { t.SetupCommandLine("-Wall") t.SetupVartypes() - t.SetupToolUsable("echo", "") - t.SetupToolUsable("grep", "GREP") - t.SetupToolUsable("sed", "") - t.SetupToolUsable("touch", "") - t.SetupToolUsable("tr", "tr") - t.SetupToolUsable("true", "TRUE") + t.SetupTool("echo", "", AtRunTime) + t.SetupTool("grep", "GREP", AtRunTime) + t.SetupTool("sed", "", AtRunTime) + t.SetupTool("touch", "", AtRunTime) + t.SetupTool("tr", "tr", AtRunTime) + t.SetupTool("true", "TRUE", AtRunTime) mklines := t.NewMkLines("Makefile", MkRcsID, "pre-configure:", diff --git a/pkgtools/pkglint/files/shtokenizer_test.go b/pkgtools/pkglint/files/shtokenizer_test.go index aee08e1b88a..e2be6c69934 100644 --- a/pkgtools/pkglint/files/shtokenizer_test.go +++ b/pkgtools/pkglint/files/shtokenizer_test.go @@ -507,7 +507,7 @@ func (s *Suite) Test_ShTokenizer__examples_from_fuzzing(c *check.C) { "\t"+"\"`'`y", // Covers shAtomDquotBackt: return nil - // FIXME: Pkglint must parse unescpaed dollar in the same way, everywhere. + // FIXME: Pkglint must parse unescaped dollar in the same way, everywhere. "\t"+"\"`$|", // Covers shAtomDquotBacktDquot: return nil diff --git a/pkgtools/pkglint/files/substcontext.go b/pkgtools/pkglint/files/substcontext.go index c55252c5a3d..678c281f473 100644 --- a/pkgtools/pkglint/files/substcontext.go +++ b/pkgtools/pkglint/files/substcontext.go @@ -54,7 +54,7 @@ func (ctx *SubstContext) Varassign(mkline MkLine) { op := mkline.Op() value := mkline.Value() if varcanon == "SUBST_CLASSES" || varcanon == "SUBST_CLASSES.*" { - classes := splitOnSpace(value) + classes := fields(value) if len(classes) > 1 { mkline.Warnf("Please add only one class at a time to SUBST_CLASSES.") } diff --git a/pkgtools/pkglint/files/textproc/prefixreplacer.go b/pkgtools/pkglint/files/textproc/prefixreplacer.go index 41ec82a6d64..1d0720cf1c9 100644 --- a/pkgtools/pkglint/files/textproc/prefixreplacer.go +++ b/pkgtools/pkglint/files/textproc/prefixreplacer.go @@ -80,11 +80,7 @@ func (pr *PrefixReplacer) AdvanceBytesFunc(fn func(c byte) bool) bool { // AdvanceHspace advances over as many spaces and tabs as possible. func (pr *PrefixReplacer) AdvanceHspace() bool { - i := 0 - rest := pr.rest - for i < len(rest) && (rest[i] == ' ' || rest[i] == '\t') { - i++ - } + i := initialHspace(pr.rest) if i != 0 { pr.s = pr.rest[:i] pr.rest = pr.rest[i:] @@ -134,8 +130,8 @@ func (pr *PrefixReplacer) Skip(n int) { pr.rest = pr.rest[n:] } -func (pr *PrefixReplacer) SkipSpace() { - pr.rest = strings.TrimLeft(pr.rest, " \t") +func (pr *PrefixReplacer) SkipHspace() { + pr.rest = pr.rest[initialHspace(pr.rest):] } // Since returns the substring between the mark and the current position. @@ -156,3 +152,11 @@ func (pr *PrefixReplacer) HasPrefix(str string) bool { func (pr *PrefixReplacer) HasPrefixRegexp(re regex.Pattern) bool { return pr.res.Matches(pr.rest, re) } + +func initialHspace(s string) int { + i := 0 + for i < len(s) && (s[i] == ' ' || s[i] == '\t') { + i++ + } + return i +} diff --git a/pkgtools/pkglint/files/tools.go b/pkgtools/pkglint/files/tools.go index 1f92dfe279a..e8c20ddc089 100644 --- a/pkgtools/pkglint/files/tools.go +++ b/pkgtools/pkglint/files/tools.go @@ -1,8 +1,8 @@ package main import ( + "fmt" "netbsd.org/pkglint/trace" - "path" "sort" "strings" ) @@ -12,6 +12,10 @@ import ( // pkgsrc. // // See `mk/tools/`. +// +// TODO: MustUseVarForm does not really depend on the tool but only depends +// on where the tool is used (load time, run time). This had already been +// modeled wrong in pkglint 4, more than 10 years ago. type Tool struct { Name string // e.g. "sed", "gzip" Varname string // e.g. "SED", "GZIP_CMD" @@ -19,11 +23,9 @@ type Tool struct { Validity Validity } -func (tool *Tool) SetValidity(validity Validity, traceName string) { - if trace.Tracing && validity != tool.Validity { - trace.Stepf("%s: Setting validity of %q to %s", traceName, tool.Name, validity) - } - tool.Validity = validity +func (tool *Tool) String() string { + return fmt.Sprintf("%s:%s:%s:%s", + tool.Name, tool.Varname, ifelseStr(tool.MustUseVarForm, "var", ""), tool.Validity) } // UsableAtLoadTime means that the tool may be used by its variable @@ -82,19 +84,22 @@ type Tools struct { TraceName string // Only for the trace log byName map[string]*Tool // "sed" => tool byVarname map[string]*Tool // "GREP_CMD" => tool - SeenPrefs bool // Determines the effect of adding the tool to USE_TOOLS + fallback *Tools + SeenPrefs bool // Determines the effect of adding the tool to USE_TOOLS } -func NewTools(traceName string) Tools { - return Tools{ +func NewTools(traceName string) *Tools { + return &Tools{ traceName, make(map[string]*Tool), make(map[string]*Tool), + nil, false} } // Define registers the tool by its name and the corresponding -// variable name (if nonempty). +// variable name (if nonempty). Depending on the given mkline, +// it may be added to USE_TOOLS automatically. // // After this tool is added to USE_TOOLS, it may be used by this name // (e.g. "awk") or by its variable (e.g. ${AWK}). @@ -103,33 +108,28 @@ func (tr *Tools) Define(name, varname string, mkline MkLine) *Tool { trace.Stepf("Tools.Define for %s: %q %q in %s", tr.TraceName, name, varname, mkline) } - tool := tr.def(name, varname, mkline) - if varname != "" { - tool.Varname = varname + if !tr.IsValidToolName(name) { + mkline.Errorf("Invalid tool name %q.", name) } - return tool + + validity := tr.validity(mkline.Basename, false) + return tr.defTool(name, varname, false, validity) } -func (tr *Tools) def(name, varname string, mkline MkLine) *Tool { - if mkline != nil && !tr.IsValidToolName(name) { - mkline.Errorf("Invalid tool name %q.", name) - } +func (tr *Tools) defTool(name, varname string, mustUseVarForm bool, validity Validity) *Tool { + fresh := &Tool{name, varname, mustUseVarForm, validity} - validity := Nowhere - if mkline != nil { - if IsPrefs(mkline.Filename) { - validity = AfterPrefsMk - } else if path.Base(mkline.Filename) == "bsd.pkg.mk" { - validity = AtRunTime - } + tool := tr.byName[name] + if tool == nil { + tool = fresh + tr.byName[name] = tool + } else { + tr.merge(tool, fresh) } - tool := &Tool{name, varname, false, validity} - if name != "" { - if existing := tr.byName[name]; existing != nil { - tool = existing - } else { - tr.byName[name] = tool + if tr.fallback != nil { + if fallback := tr.fallback.ByName(name); fallback != nil { + tr.merge(tool, fallback) } } @@ -142,6 +142,18 @@ func (tr *Tools) def(name, varname string, mkline MkLine) *Tool { return tool } +func (tr *Tools) merge(target, source *Tool) { + if target.Varname == "" && source.Varname != "" { + target.Varname = source.Varname + } + if !target.MustUseVarForm && source.MustUseVarForm { + target.MustUseVarForm = true + } + if source.Validity > target.Validity { + target.Validity = source.Validity + } +} + func (tr *Tools) Trace() { if trace.Tracing { defer trace.Call1(tr.TraceName)() @@ -158,17 +170,23 @@ func (tr *Tools) Trace() { for _, toolname := range keys { trace.Stepf("tool %+v", tr.byName[toolname]) } + + if tr.fallback != nil { + tr.fallback.Trace() + } } // ParseToolLine updates the tool definitions according to the given // line from a Makefile. -func (tr *Tools) ParseToolLine(mkline MkLine) { - tr.ParseToolLineCreate(mkline, false) -} - -// ParseToolLineCreate updates the tool definitions according to the given -// line from a Makefile, registering the tools if necessary. -func (tr *Tools) ParseToolLineCreate(mkline MkLine, createIfAbsent bool) { +// +// If fromInfrastructure is true, the tool is defined even when it is only +// added to USE_TOOLS (which normally doesn't define anything). This way, +// pkglint also finds those tools whose definitions are too difficult to +// parse from the code. +// +// 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) { switch { case mkline.IsVarassign(): @@ -194,13 +212,13 @@ func (tr *Tools) ParseToolLineCreate(mkline MkLine, createIfAbsent bool) { case "_TOOLS.*": if !containsVarRef(varparam) { tr.Define(varparam, "", mkline) - for _, tool := range splitOnSpace(value) { + for _, tool := range fields(value) { tr.Define(tool, "", mkline) } } case "USE_TOOLS": - tr.parseUseTools(mkline, createIfAbsent) + tr.parseUseTools(mkline, fromInfrastructure, addToUseTools) } case mkline.IsInclude(): @@ -214,61 +232,68 @@ func (tr *Tools) ParseToolLineCreate(mkline MkLine, createIfAbsent bool) { // It determines the validity of the tool, i.e. in which places it may be used. // // If createIfAbsent is true and the tools is unknown, it is registered. -func (tr *Tools) parseUseTools(mkline MkLine, createIfAbsent bool) { +// This can be done only in the pkgsrc infrastructure files, where the +// actual definition is assumed to be in some other file. In packages +// though, this assumption cannot be made and pkglint needs to be strict. +func (tr *Tools) parseUseTools(mkline MkLine, createIfAbsent bool, addToUseTools bool) { value := mkline.Value() if containsVarRef(value) { return } - deps := splitOnSpace(value) + deps := fields(value) // See mk/tools/autoconf.mk:/^\.if !defined/ if matches(value, `\bautoconf213\b`) { - for _, name := range [...]string{"autoconf-2.13", "autoheader-2.13", "autoreconf-2.13", "autoscan-2.13", "autoupdate-2.13", "ifnames-2.13"} { - if createIfAbsent { - tr.Define(name, "", mkline) - } - deps = append(deps, name) - } + deps = append(deps, "autoconf-2.13", "autoheader-2.13", "autoreconf-2.13", "autoscan-2.13", "autoupdate-2.13", "ifnames-2.13") } if matches(value, `\bautoconf\b`) { - for _, name := range [...]string{"autoheader", "autom4te", "autoreconf", "autoscan", "autoupdate", "ifnames"} { - if createIfAbsent { - tr.Define(name, "", mkline) - } - deps = append(deps, name) - } + deps = append(deps, "autoheader", "autom4te", "autoreconf", "autoscan", "autoupdate", "ifnames") } + validity := tr.validity(mkline.Basename, addToUseTools) for _, dep := range deps { name := strings.Split(dep, ":")[0] - tool := tr.ByName(name) - if tool == nil && createIfAbsent { - tr.Define(name, "", mkline) - } - if tool != nil { - validity := tr.validity(mkline.Filename) - if validity > tool.Validity { - tool.SetValidity(validity, tr.TraceName) - } + if createIfAbsent || tr.ByName(name) != nil { + tr.defTool(name, "", false, validity) } } } -func (tr *Tools) validity(fileName string) Validity { - basename := path.Base(fileName) - if basename == "Makefile" && tr.SeenPrefs { - return AtRunTime - } - if basename == "bsd.prefs.mk" || basename == "Makefile" { +func (tr *Tools) validity(basename string, useTools bool) Validity { + switch { + case IsPrefs(basename): // IsPrefs is not 100% accurate here, but good enough return AfterPrefsMk + case basename == "Makefile" && !tr.SeenPrefs: + return AfterPrefsMk + case useTools, basename == "bsd.pkg.mk": + return AtRunTime + default: + return Nowhere } - return AtRunTime } -func (tr *Tools) ByVarname(varname string) (tool *Tool) { return tr.byVarname[varname] } +func (tr *Tools) ByVarname(varname string) *Tool { + tool := tr.byVarname[varname] + if tool == nil && tr.fallback != nil { + fallback := tr.fallback.ByVarname(varname) + if fallback != nil { + return tr.defTool(fallback.Name, fallback.Varname, fallback.MustUseVarForm, fallback.Validity) + } + } + return tool +} -func (tr *Tools) ByName(name string) (tool *Tool) { return tr.byName[name] } +func (tr *Tools) ByName(name string) *Tool { + tool := tr.byName[name] + if tool == nil && tr.fallback != nil { + fallback := tr.fallback.ByName(name) + if fallback != nil { + return tr.defTool(fallback.Name, fallback.Varname, fallback.MustUseVarForm, fallback.Validity) + } + } + return tool +} func (tr *Tools) Usable(tool *Tool, time ToolTime) bool { if time == LoadTime { @@ -278,40 +303,9 @@ func (tr *Tools) Usable(tool *Tool, time ToolTime) bool { } } -func (tr *Tools) AddAll(other Tools) { - if trace.Tracing && len(other.byName) != 0 { - defer trace.Call(other.TraceName+" to "+tr.TraceName, len(other.byName))() - } - - // Same as the code below, just a little faster. - if !trace.Tracing { - for _, otherTool := range other.byName { - tool := tr.def(otherTool.Name, otherTool.Varname, nil) - tool.MustUseVarForm = tool.MustUseVarForm || otherTool.MustUseVarForm - if otherTool.Validity > tool.Validity { - tool.SetValidity(otherTool.Validity, tr.TraceName) - } - } - return - } - - var names []string - for name := range other.byName { - names = append(names, name) - } - sort.Strings(names) - - for _, name := range names { - otherTool := other.byName[name] - if trace.Tracing { - trace.Stepf("Tools.AddAll %+v", *otherTool) - } - tool := tr.def(otherTool.Name, otherTool.Varname, nil) - tool.MustUseVarForm = tool.MustUseVarForm || otherTool.MustUseVarForm - if otherTool.Validity > tool.Validity { - tool.SetValidity(otherTool.Validity, tr.TraceName) - } - } +func (tr *Tools) Fallback(other *Tools) { + G.Assertf(tr.fallback == nil, "Tools.Fallback must only be called once.") + tr.fallback = other } func (tr *Tools) IsValidToolName(name string) bool { diff --git a/pkgtools/pkglint/files/tools_test.go b/pkgtools/pkglint/files/tools_test.go index 1e5168c2f73..b0016045a45 100644 --- a/pkgtools/pkglint/files/tools_test.go +++ b/pkgtools/pkglint/files/tools_test.go @@ -2,10 +2,37 @@ package main import "gopkg.in/check.v1" +func (s *Suite) Test_Tool_UsableAtLoadTime(c *check.C) { + + nowhere := Tool{"nowhere", "", false, Nowhere} + c.Check(nowhere.UsableAtLoadTime(false), equals, false) + c.Check(nowhere.UsableAtLoadTime(true), equals, false) + + load := Tool{"load", "", false, AfterPrefsMk} + c.Check(load.UsableAtLoadTime(false), equals, false) + c.Check(load.UsableAtLoadTime(true), equals, true) + + run := Tool{"run", "", false, AtRunTime} + 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} + c.Check(nowhere.UsableAtRunTime(), equals, false) + + load := Tool{"load", "", false, AfterPrefsMk} + c.Check(load.UsableAtRunTime(), equals, true) + + run := Tool{"run", "", false, AtRunTime} + c.Check(run.UsableAtRunTime(), equals, true) +} + func (s *Suite) Test_Tools_ParseToolLine(c *check.C) { t := s.Init(c) - t.SetupToolUsable("tool1", "") + t.SetupTool("tool1", "", Nowhere) t.SetupVartypes() t.CreateFileLines("Makefile", MkRcsID, @@ -18,7 +45,7 @@ func (s *Suite) Test_Tools_ParseToolLine(c *check.C) { t.CheckOutputEmpty() } -func (s *Suite) Test_Tools_def__invalid_tool_name(c *check.C) { +func (s *Suite) Test_Tools_Define__invalid_tool_name(c *check.C) { t := s.Init(c) reg := NewTools("") @@ -65,7 +92,8 @@ func (s *Suite) Test_Tools__USE_TOOLS_predefined_sed(c *check.C) { t.CheckOutputLines( "WARN: ~/module.mk:5: Unknown shell command \"${AWK}\".", - "0 errors and 1 warning found.", + "WARN: ~/module.mk:5: AWK is used but not defined.", + "0 errors and 2 warnings found.", "(Run \"pkglint -e\" to show explanations.)") } @@ -89,9 +117,9 @@ func (s *Suite) Test_Tools__load_from_infrastructure(c *check.C) { tools := NewTools("") - tools.ParseToolLine(t.NewMkLine("create.mk", 2, "TOOLS_CREATE+= load")) - tools.ParseToolLine(t.NewMkLine("create.mk", 3, "TOOLS_CREATE+= run")) - tools.ParseToolLine(t.NewMkLine("create.mk", 4, "TOOLS_CREATE+= nowhere")) + 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) // The references to the tools are stable, // the lookup methods always return the same objects. @@ -106,9 +134,9 @@ func (s *Suite) Test_Tools__load_from_infrastructure(c *check.C) { c.Check(nowhere, deepEquals, &Tool{"nowhere", "", false, Nowhere}) // The name RUN_CMD avoids conflicts with RUN. - tools.ParseToolLine(t.NewMkLine("varnames.mk", 2, "_TOOLS_VARNAME.load= LOAD")) - tools.ParseToolLine(t.NewMkLine("varnames.mk", 3, "_TOOLS_VARNAME.run= RUN_CMD")) - tools.ParseToolLine(t.NewMkLine("varnames.mk", 4, "_TOOLS_VARNAME.nowhere= NOWHERE")) + 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) // At this point the tools can be found by their variable names, too. // They still may not be used. @@ -128,7 +156,7 @@ func (s *Suite) Test_Tools__load_from_infrastructure(c *check.C) { c.Check(nowhere.UsableAtLoadTime(true), equals, false) c.Check(nowhere.UsableAtRunTime(), equals, false) - tools.ParseToolLine(t.NewMkLine("bsd.prefs.mk", 2, "USE_TOOLS+= load")) + tools.ParseToolLine(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. @@ -138,7 +166,7 @@ func (s *Suite) Test_Tools__load_from_infrastructure(c *check.C) { c.Check(load.UsableAtLoadTime(true), equals, true) c.Check(load.UsableAtRunTime(), equals, true) - tools.ParseToolLine(t.NewMkLine("bsd.pkg.mk", 2, "USE_TOOLS+= run")) + tools.ParseToolLine(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). @@ -174,7 +202,7 @@ func (s *Suite) Test_Tools__package_Makefile(c *check.C) { G.Pkgsrc.LoadInfrastructure() tools := NewTools("") - tools.AddAll(G.Pkgsrc.Tools) + tools.Fallback(G.Pkgsrc.Tools) load := tools.ByName("load") run := tools.ByName("run") @@ -190,7 +218,7 @@ 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 though of as being true from the beginning. - tools.ParseToolLine(t.NewMkLine("Makefile", 2, "USE_TOOLS+= pkg-before-prefs")) + tools.ParseToolLine(t.NewMkLine("Makefile", 2, "USE_TOOLS+= pkg-before-prefs"), false, true) c.Check(before.Validity, equals, AfterPrefsMk) c.Check(before.UsableAtLoadTime(false), equals, false) @@ -199,11 +227,11 @@ func (s *Suite) Test_Tools__package_Makefile(c *check.C) { c.Check(tools.SeenPrefs, equals, false) - tools.ParseToolLine(t.NewMkLine("Makefile", 3, ".include \"../../mk/bsd.prefs.mk\"")) + tools.ParseToolLine(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")) + tools.ParseToolLine(t.NewMkLine("Makefile", 4, "USE_TOOLS+= pkg-after-prefs"), false, true) c.Check(after.Validity, equals, AtRunTime) c.Check(after.UsableAtLoadTime(false), equals, false) @@ -327,19 +355,19 @@ func (s *Suite) Test_Tools__tools_having_the_same_variable_name(c *check.C) { t.CheckOutputLines( "TRACE: + (*Tools).Trace(\"Pkgsrc\")", - "TRACE: 1 tool &{Name:awk Varname:AWK MustUseVarForm:false Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:echo Varname:ECHO MustUseVarForm:true Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:echo -n Varname:ECHO_N MustUseVarForm:true Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:false Varname:FALSE MustUseVarForm:true Validity:Nowhere}", - "TRACE: 1 tool &{Name:gawk Varname:AWK MustUseVarForm:false Validity:Nowhere}", - "TRACE: 1 tool &{Name:gsed Varname:SED MustUseVarForm:false Validity:Nowhere}", - "TRACE: 1 tool &{Name:sed Varname:SED MustUseVarForm:false Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:test Varname:TEST MustUseVarForm:true Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:true Varname:TRUE MustUseVarForm:true Validity:AfterPrefsMk}", + "TRACE: 1 tool awk:AWK::AfterPrefsMk", + "TRACE: 1 tool echo:ECHO:var:AfterPrefsMk", + "TRACE: 1 tool echo -n:ECHO_N:var:AfterPrefsMk", + "TRACE: 1 tool false:FALSE:var:AtRunTime", + "TRACE: 1 tool gawk:AWK::Nowhere", + "TRACE: 1 tool gsed:SED::Nowhere", + "TRACE: 1 tool sed:SED::AfterPrefsMk", + "TRACE: 1 tool test:TEST:var:AfterPrefsMk", + "TRACE: 1 tool true:TRUE:var:AfterPrefsMk", "TRACE: - (*Tools).Trace(\"Pkgsrc\")") tools := NewTools("module.mk") - tools.AddAll(G.Pkgsrc.Tools) + tools.Fallback(G.Pkgsrc.Tools) t.EnableTracingToLog() tools.Trace() @@ -347,15 +375,17 @@ func (s *Suite) Test_Tools__tools_having_the_same_variable_name(c *check.C) { t.CheckOutputLines( "TRACE: + (*Tools).Trace(\"module.mk\")", - "TRACE: 1 tool &{Name:awk Varname:AWK MustUseVarForm:false Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:echo Varname:ECHO MustUseVarForm:true Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:echo -n Varname:ECHO_N MustUseVarForm:true Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:false Varname:FALSE MustUseVarForm:true Validity:Nowhere}", - "TRACE: 1 tool &{Name:gawk Varname:AWK MustUseVarForm:false Validity:Nowhere}", - "TRACE: 1 tool &{Name:gsed Varname:SED MustUseVarForm:false Validity:Nowhere}", - "TRACE: 1 tool &{Name:sed Varname:SED MustUseVarForm:false Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:test Varname:TEST MustUseVarForm:true Validity:AfterPrefsMk}", - "TRACE: 1 tool &{Name:true Varname:TRUE MustUseVarForm:true Validity:AfterPrefsMk}", + "TRACE: 1 + (*Tools).Trace(\"Pkgsrc\")", + "TRACE: 1 2 tool awk:AWK::AfterPrefsMk", + "TRACE: 1 2 tool echo:ECHO:var:AfterPrefsMk", + "TRACE: 1 2 tool echo -n:ECHO_N:var:AfterPrefsMk", + "TRACE: 1 2 tool false:FALSE:var:AtRunTime", + "TRACE: 1 2 tool gawk:AWK::Nowhere", + "TRACE: 1 2 tool gsed:SED::Nowhere", + "TRACE: 1 2 tool sed:SED::AfterPrefsMk", + "TRACE: 1 2 tool test:TEST:var:AfterPrefsMk", + "TRACE: 1 2 tool true:TRUE:var:AfterPrefsMk", + "TRACE: 1 - (*Tools).Trace(\"Pkgsrc\")", "TRACE: - (*Tools).Trace(\"module.mk\")") } @@ -387,52 +417,102 @@ func (s *Suite) Test_Tools__var(c *check.C) { t.CheckOutputEmpty() } -// Demonstrates how the Tools type handles tool that share the same +// Demonstrates how the Tools type handles tools that share the same // variable name. Of these tools, the GNU variant is preferred. // +// In this realistic variant, the non-GNU tool is defined in bsd.prefs.mk +// and the GNU tool is only defined but not made available. +// // See also Pkglint.Tool. -func (s *Suite) Test_Tools_AddAll__tools_having_the_same_variable_name(c *check.C) { +func (s *Suite) Test_Tools_Fallback__tools_having_the_same_variable_name_realistic(c *check.C) { nonGnu := NewTools("non-gnu") - nonGnu.Define("sed", "SED", dummyMkLine).SetValidity(AfterPrefsMk, "") + nonGnu.defTool("sed", "SED", false, AfterPrefsMk) gnu := NewTools("gnu") - gnu.Define("gsed", "SED", dummyMkLine) + gnu.defTool("gsed", "SED", false, Nowhere) local1 := NewTools("local") - local1.AddAll(nonGnu) - local1.AddAll(gnu) + local1.defTool("sed", "SED", false, AfterPrefsMk) + local1.Fallback(gnu) c.Check(local1.ByName("sed").Validity, equals, AfterPrefsMk) c.Check(local1.ByName("gsed").Validity, equals, Nowhere) local1.Trace() local2 := NewTools("local") - local2.AddAll(gnu) - local2.AddAll(nonGnu) + local2.defTool("gsed", "SED", false, Nowhere) + local2.Fallback(nonGnu) c.Check(local2.ByName("sed").Validity, equals, AfterPrefsMk) c.Check(local2.ByName("gsed").Validity, equals, Nowhere) local2.Trace() - nonGnu.ByName("sed").Validity = Nowhere - gnu.ByName("gsed").Validity = AfterPrefsMk + // No matter in which order the tool definitions are encountered, + // the non-GNU version is always chosen since the GNU version is + // not available at all. + c.Check(local1.ByVarname("SED").String(), equals, "sed:SED::AfterPrefsMk") + c.Check(local2.ByVarname("SED").String(), equals, "sed:SED::AfterPrefsMk") +} + +// Demonstrates how the Tools type handles tools that share the same +// variable name. Of these tools, the GNU variant is preferred. +// +// In this unrealistic variant, the GNU tool is defined in bsd.prefs.mk +// and the non-GNU tool is only defined but not made available. +// +// See also Pkglint.Tool. +func (s *Suite) Test_Tools_Fallback__tools_having_the_same_variable_name_unrealistic(c *check.C) { + nonGnu := NewTools("non-gnu") + nonGnu.defTool("sed", "SED", false, Nowhere) + + gnu := NewTools("gnu") + gnu.defTool("gsed", "SED", false, AfterPrefsMk) + + local1 := NewTools("local") + local1.defTool("sed", "SED", false, Nowhere) + local1.Fallback(gnu) - local3 := NewTools("local") - local3.AddAll(nonGnu) - local3.AddAll(gnu) + c.Check(local1.ByName("sed").Validity, equals, Nowhere) + c.Check(local1.ByName("gsed").Validity, equals, AfterPrefsMk) + local1.Trace() - c.Check(local3.ByName("sed").Validity, equals, Nowhere) - c.Check(local3.ByName("gsed").Validity, equals, AfterPrefsMk) - local3.Trace() + local2 := NewTools("local") + local2.defTool("gsed", "SED", false, AfterPrefsMk) + local2.Fallback(nonGnu) - local4 := NewTools("local") - local4.AddAll(gnu) - local4.AddAll(nonGnu) + c.Check(local2.ByName("sed").Validity, equals, Nowhere) + c.Check(local2.ByName("gsed").Validity, equals, AfterPrefsMk) + local2.Trace() - c.Check(local4.ByName("sed").Validity, equals, Nowhere) - c.Check(local4.ByName("gsed").Validity, equals, AfterPrefsMk) - local4.Trace() + // 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, deepEquals, local2) - c.Check(local4, deepEquals, local4) +// The cmake tool is included conditionally. The condition is so simple that +// pkglint could parse it but it depends on the particular package. +// This is something that pkglint cannot do right now, since the global tools +// are loaded once for all packages. +// +// Therefore there is a workaround for USE_CMAKE. +// +// See mk/tools/cmake.mk. +func (s *Suite) Test_Tools__cmake(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wall") + t.SetupPackage("category/package", + "USE_CMAKE=\tyes", + "", + "do-test:", + "\tcd ${WRKSRC} && cmake") + t.CreateFileLines("mk/tools/defaults.mk", + ".if defined(USE_CMAKE)", + "USE_TOOLS+=\tcmake cpack", + ".endif") + G.Pkgsrc.LoadInfrastructure() + + G.CheckDirent(t.File("category/package")) + + t.CheckOutputEmpty() } diff --git a/pkgtools/pkglint/files/trace/tracing_test.go b/pkgtools/pkglint/files/trace/tracing_test.go index f63b31e0fa8..7a274d81cb5 100755 --- a/pkgtools/pkglint/files/trace/tracing_test.go +++ b/pkgtools/pkglint/files/trace/tracing_test.go @@ -65,6 +65,52 @@ func (s *Suite) Test_Call__argumentsAndResultWrong(c *check.C) { "TRACE: - netbsd.org/pkglint/trace.argumentsAndResultWrong(\"arg0\", 1234, \"\")\n") } +func (s *Suite) Test__fixed_argument_variants(c *check.C) { + + output := s.captureTracingOutput(func() { + defer Call0()() + defer Call1("x")() + defer Call2("x", "y")() + Step1("step %s", "a") + Step2("step %s, %s", "a", "b") + }) + + c.Check(output, check.Equals, ""+ + "TRACE: + netbsd.org/pkglint/trace.(*Suite).Test__fixed_argument_variants.func1()\n"+ + "TRACE: 1 + netbsd.org/pkglint/trace.(*Suite).Test__fixed_argument_variants.func1(\"x\")\n"+ + "TRACE: 1 2 + netbsd.org/pkglint/trace.(*Suite).Test__fixed_argument_variants.func1(\"x\", \"y\")\n"+ + "TRACE: 1 2 3 step a\n"+ + "TRACE: 1 2 3 step a, b\n"+ + "TRACE: 1 2 - netbsd.org/pkglint/trace.(*Suite).Test__fixed_argument_variants.func1(\"x\", \"y\")\n"+ + "TRACE: 1 - netbsd.org/pkglint/trace.(*Suite).Test__fixed_argument_variants.func1(\"x\")\n"+ + "TRACE: - netbsd.org/pkglint/trace.(*Suite).Test__fixed_argument_variants.func1()\n") +} + +func (s *Suite) Test__stringer_arg(c *check.C) { + + output := s.captureTracingOutput(func() { + defer Call(str{}, &str{})() + }) + + c.Check(output, check.Equals, ""+ + "TRACE: + netbsd.org/pkglint/trace.(*Suite).Test__stringer_arg.func1(It's a string, It's a string)\n"+ + "TRACE: - netbsd.org/pkglint/trace.(*Suite).Test__stringer_arg.func1(It's a string, It's a string)\n") +} + +func (s *Suite) Test_traceCall__panic(c *check.C) { + c.Check( + func() { traceCall() }, + check.Panics, + "Internal pkglint error: calls to trace.Call must only occur in tracing mode") +} + +func (s *Suite) Test_Result__panic(c *check.C) { + c.Check( + func() { Result("invalid argument") }, + check.Panics, + "Result must be called with a pointer to the result, not \"invalid argument\".") +} + func (s *Suite) captureTracingOutput(action func()) string { out := bytes.Buffer{} Out = &out @@ -76,3 +122,9 @@ func (s *Suite) captureTracingOutput(action func()) string { Out = nil return out.String() } + +type str struct{} + +func (str) String() string { + return "It's a string" +} diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go index ff156bdc061..f4c8c58243f 100644 --- a/pkgtools/pkglint/files/util.go +++ b/pkgtools/pkglint/files/util.go @@ -13,7 +13,6 @@ import ( "strconv" "strings" "time" - "unicode" ) type YesNoUnknown uint8 @@ -38,6 +37,9 @@ func hasPrefix(s, prefix string) bool { func hasSuffix(s, suffix string) bool { return strings.HasSuffix(s, suffix) } +func fields(s string) []string { + return strings.Fields(s) +} func matches(s string, re regex.Pattern) bool { return G.res.Matches(s, re) } @@ -56,10 +58,6 @@ func match4(s string, re regex.Pattern) (matched bool, m1, m2, m3, m4 string) { func match5(s string, re regex.Pattern) (matched bool, m1, m2, m3, m4, m5 string) { return G.res.Match5(s, re) } -func replaceFirst(s string, re regex.Pattern, repl string) string { - m, replaced := G.res.ReplaceFirst(s, re, repl) - return ifelseStr(m != nil, replaced, s) -} func replaceAll(s string, re regex.Pattern, repl string) string { return G.res.Compile(re).ReplaceAllString(s, repl) } @@ -67,6 +65,22 @@ func replaceAllFunc(s string, re regex.Pattern, repl func(string) string) string return G.res.Compile(re).ReplaceAllStringFunc(s, repl) } +// trimHspace returns str, with leading and trailing space (U+0020) +// and tab (U+0009) removed. +// +// It is simpler and faster than strings.TrimSpace. +func trimHspace(str string) string { + start := 0 + end := len(str) + for start < end && (str[start] == ' ' || str[start] == '\t') { + start++ + } + for start < end && (str[end-1] == ' ' || str[end-1] == '\t') { + end-- + } + return str[start:end] +} + func ifelseStr(cond bool, a, b string) string { if cond { return a @@ -261,47 +275,20 @@ func defineVar(mkline MkLine, varname string) { } } -// varIsDefined tests whether the variable (or its canonicalized form) +// varIsDefinedSimilar tests whether the variable (or its canonicalized form) // is defined in the current package or in the current file. -// TODO: rename to similar -func varIsDefined(varname string) bool { - return G.Mk != nil && G.Mk.vars.DefinedSimilar(varname) || +func varIsDefinedSimilar(varname string) bool { + return G.Mk != nil && (G.Mk.vars.DefinedSimilar(varname) || G.Mk.forVars[varname]) || G.Pkg != nil && G.Pkg.vars.DefinedSimilar(varname) } -// varIsUsed tests whether the variable (or its canonicalized form) +// varIsUsedSimilar tests whether the variable (or its canonicalized form) // is used in the current package or in the current file. -// TODO: rename to similar -func varIsUsed(varname string) bool { +func varIsUsedSimilar(varname string) bool { return G.Mk != nil && G.Mk.vars.UsedSimilar(varname) || G.Pkg != nil && G.Pkg.vars.UsedSimilar(varname) } -func splitOnSpace(s string) []string { - i := 0 - n := len(s) - - for i < n && unicode.IsSpace(rune(s[i])) { - i++ - } - - var parts []string - for i < n { - start := i - for i < n && !unicode.IsSpace(rune(s[i])) { - i++ - } - if start != i { - parts = append(parts, s[start:i]) - } - for i < n && unicode.IsSpace(rune(s[i])) { - i++ - } - } - - return parts -} - func fileExists(fname string) bool { st, err := os.Stat(fname) return err == nil && st.Mode().IsRegular() @@ -312,14 +299,6 @@ func dirExists(fname string) bool { return err == nil && st.Mode().IsDir() } -// Useful in combination with regex.Find*Index -func negToZero(i int) int { - if i >= 0 { - return i - } - return 0 -} - func toInt(s string, def int) int { if n, err := strconv.Atoi(s); err == nil { return n @@ -729,13 +708,16 @@ func (s *RedundantScope) Handle(mkline MkLine) { } } +// IsPrefs returns whether the given file, when included, loads the user +// preferences. func IsPrefs(fileName string) bool { switch path.Base(fileName) { - case "bsd.prefs.mk", - "bsd.fast.prefs.mk", - "bsd.builtin.mk", // mk/buildlink3/bsd.builtin.mk - "pkgconfig-builtin.mk", - "bsd.options.mk": + case // See https://github.com/golang/go/issues/28057 + "bsd.prefs.mk", // in mk/ + "bsd.fast.prefs.mk", // in mk/ + "bsd.builtin.mk", // in mk/buildlink3/ + "pkgconfig-builtin.mk", // in mk/buildlink3/ + "bsd.options.mk": // in mk/ return true } return false @@ -760,10 +742,10 @@ type FileCache struct { } type fileCacheEntry struct { - count int - fileName string - options LoadOptions - lines []Line + count int + key string + options LoadOptions + lines []Line } func NewFileCache(size int) *FileCache { @@ -780,32 +762,52 @@ func (c *FileCache) Put(fileName string, options LoadOptions, lines []Line) { entry := c.mapping[key] if entry == nil { if len(c.table) == cap(c.table) { - sort.Slice(c.table, func(i, j int) bool { return c.table[j].count < c.table[i].count }) - minCount := c.table[len(c.table)-1].count - newLen := len(c.table) - for newLen > 0 && c.table[newLen-1].count == minCount { - delete(c.mapping, c.key(c.table[newLen-1].fileName)) - newLen-- - } - c.table = c.table[0:newLen] - - // To avoid files from getting stuck in the cache. - for _, e := range c.table { - e.count /= 2 - } + c.removeOldEntries() } - entry = &fileCacheEntry{0, fileName, options, lines} + entry = new(fileCacheEntry) c.table = append(c.table, entry) c.mapping[key] = entry } - entry.count = 0 - entry.fileName = fileName + entry.count = 1 + entry.key = key entry.options = options entry.lines = lines } +func (c *FileCache) removeOldEntries() { + printStats := func() bool { return false }() + + sort.Slice(c.table, func(i, j int) bool { return c.table[j].count < c.table[i].count }) + + if printStats { + for _, e := range c.table { + G.logOut.Printf("FileCache %q with count %d.\n", e.key, e.count) + } + } + + minCount := c.table[len(c.table)-1].count + newLen := len(c.table) + for newLen > 0 && c.table[newLen-1].count == minCount { + e := c.table[newLen-1] + if printStats { + G.logOut.Printf("FileCache.Evict %q with count %d.\n", e.key, e.count) + } + delete(c.mapping, e.key) + newLen-- + } + c.table = c.table[0:newLen] + + // To avoid files getting stuck in the cache. + for _, e := range c.table { + if printStats { + G.logOut.Printf("FileCache.Halve %q with count %d.\n", e.key, e.count) + } + e.count /= 2 + } +} + func (c *FileCache) Get(fileName string, options LoadOptions) []Line { key := c.key(fileName) entry, found := c.mapping[key] diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go index fb1fd51c4a3..50626ed9847 100644 --- a/pkgtools/pkglint/files/util_test.go +++ b/pkgtools/pkglint/files/util_test.go @@ -220,13 +220,6 @@ func emptyToNil(slice []string) []string { return slice } -func (s *Suite) Test_splitOnSpace(c *check.C) { - c.Check(splitOnSpace("aaaaa"), deepEquals, []string{"aaaaa"}) - c.Check(splitOnSpace(" a b\tc\n"), deepEquals, []string{"a", "b", "c"}) - c.Check(splitOnSpace(" "), check.IsNil) - c.Check(splitOnSpace(""), check.IsNil) -} - func (s *Suite) Test_isLocallyModified(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/vardefs.go b/pkgtools/pkglint/files/vardefs.go index 4fea03798f6..9a91f26c8a7 100644 --- a/pkgtools/pkglint/files/vardefs.go +++ b/pkgtools/pkglint/files/vardefs.go @@ -41,6 +41,14 @@ func (src *Pkgsrc) InitVartypes() { "Makefile.*, *.mk: default, set, use") } + // pkgload is the same as pkg, except that the variable may be accessed at load time. + pkgload := func(varname string, kindOfList KindOfList, checker *BasicType) { + acl(varname, kindOfList, checker, ""+ + "Makefile: set, use, use-loadtime; "+ + "buildlink3.mk, builtin.mk:; "+ + "Makefile.*, *.mk: default, set, use, use-loadtime") + } + // A package-defined list may be appended to in all Makefiles except buildlink3.mk and builtin.mk. // Simple assignment (instead of appending) is only allowed in Makefile and Makefile.common. pkglist := func(varname string, kindOfList KindOfList, checker *BasicType) { @@ -57,9 +65,17 @@ func (src *Pkgsrc) InitVartypes() { sys := func(varname string, kindOfList KindOfList, checker *BasicType) { acl(varname, kindOfList, checker, "buildlink3.mk:; *: use") } + usr := func(varname string, kindOfList KindOfList, checker *BasicType) { acl(varname, kindOfList, checker, "buildlink3.mk:; *: use-loadtime, use") } + + // sysload defines a system-provided variable that may already be used + // at load time. + sysload := func(varname string, kindOfList KindOfList, checker *BasicType) { + acl(varname, kindOfList, checker, "*: use-loadtime, use") + } + bl3list := func(varname string, kindOfList KindOfList, checker *BasicType) { acl(varname, kindOfList, checker, "buildlink3.mk, builtin.mk: append") } @@ -74,7 +90,7 @@ func (src *Pkgsrc) InitVartypes() { if mklines != nil { for _, mkline := range mklines.mklines { if mkline.IsDirective() && mkline.Directive() == "for" { - words := splitOnSpace(mkline.Args()) + words := fields(mkline.Args()) if len(words) > 2 && words[0] == "_version_" { for _, word := range words[2:] { languages[word] = true @@ -83,7 +99,7 @@ func (src *Pkgsrc) InitVartypes() { } } } - for _, language := range splitOnSpace("ada c c99 c++ c++11 fortran fortran77 java objc obj-c++") { + for _, language := range [...]string{"ada", "c", "c99", "c++", "c++11", "fortran", "fortran77", "java", "objc", "obj-c++"} { languages[language] = true } @@ -186,6 +202,7 @@ func (src *Pkgsrc) InitVartypes() { usr("PKGSRC_USE_FORTIFY", lkNone, BtYesNo) usr("PKGSRC_USE_RELRO", lkNone, BtYesNo) usr("PKGSRC_USE_SSP", lkNone, enum("no yes strong all")) + usr("PREFER.*", lkNone, enum("pkgsrc native")) usr("PREFER_PKGSRC", lkShell, BtIdentifier) usr("PREFER_NATIVE", lkShell, BtIdentifier) usr("PREFER_NATIVE_PTHREADS", lkNone, BtYesNo) @@ -193,7 +210,7 @@ func (src *Pkgsrc) InitVartypes() { usr("LOCALBASE", lkNone, BtPathname) usr("CROSSBASE", lkNone, BtPathname) usr("VARBASE", lkNone, BtPathname) - usr("X11_TYPE", lkNone, enum("modular native")) + acl("X11_TYPE", lkNone, enum("modular native"), "*: use-loadtime, use") usr("X11BASE", lkNone, BtPathname) usr("MOTIFBASE", lkNone, BtPathname) usr("PKGINFODIR", lkNone, BtPathname) @@ -469,6 +486,7 @@ func (src *Pkgsrc) InitVartypes() { acl("BDB_DEFAULT", lkNone, enum("db1 db2 db3 db4 db5 db6"), "") sys("BDB_LIBS", lkShell, BtLdFlag) sys("BDB_TYPE", lkNone, enum("db1 db2 db3 db4 db5 db6")) + sys("BIGENDIANPLATFORMS", lkSpace, BtMachinePlatformPattern) sys("BINGRP", lkNone, BtUserGroupName) sys("BINMODE", lkNone, BtFileMode) sys("BINOWN", lkNone, BtUserGroupName) @@ -476,11 +494,11 @@ func (src *Pkgsrc) InitVartypes() { pkg("BOOTSTRAP_PKG", lkNone, BtYesNo) acl("BROKEN", lkNone, BtMessage, "") pkg("BROKEN_GETTEXT_DETECTION", lkNone, BtYesNo) - pkglist("BROKEN_EXCEPT_ON_PLATFORM", lkShell, BtMachinePlatformPattern) + pkglist("BROKEN_EXCEPT_ON_PLATFORM", lkSpace, BtMachinePlatformPattern) pkglist("BROKEN_ON_PLATFORM", lkSpace, BtMachinePlatformPattern) sys("BSD_MAKE_ENV", lkShell, BtShellWord) - acl("BUILDLINK_ABI_DEPENDS.*", lkSpace, BtDependency, "builtin.mk: append, use-loadtime; *: append") - acl("BUILDLINK_API_DEPENDS.*", lkSpace, BtDependency, "builtin.mk: append, use-loadtime; *: append") + acl("BUILDLINK_ABI_DEPENDS.*", lkSpace, BtDependency, "buildlink3.mk, builtin.mk: append, use-loadtime; *: append") + acl("BUILDLINK_API_DEPENDS.*", lkSpace, BtDependency, "buildlink3.mk, builtin.mk: append, use-loadtime; *: append") acl("BUILDLINK_AUTO_DIRS.*", lkNone, BtYesNo, "buildlink3.mk: append") acl("BUILDLINK_CONTENTS_FILTER", lkNone, BtShellCommand, "") sys("BUILDLINK_CFLAGS", lkShell, BtCFlag) @@ -510,6 +528,7 @@ func (src *Pkgsrc) InitVartypes() { acl("BUILDLINK_TRANSFORM", lkShell, BtWrapperTransform, "*: append") acl("BUILDLINK_TRANSFORM.*", lkShell, BtWrapperTransform, "*: append") acl("BUILDLINK_TREE", lkShell, BtIdentifier, "buildlink3.mk: append") + acl("BUILDLINK_X11_DIR", lkNone, BtPathname, "*: use") acl("BUILD_DEFS", lkShell, BtVariableName, "Makefile, Makefile.common, options.mk: append") pkglist("BUILD_DEFS_EFFECTS", lkShell, BtVariableName) acl("BUILD_DEPENDS", lkSpace, BtDependencyWithPath, "Makefile, Makefile.common, *.mk: append") @@ -533,12 +552,12 @@ func (src *Pkgsrc) InitVartypes() { sys("BUILTIN_X11_TYPE", lkNone, BtUnknown) sys("BUILTIN_X11_VERSION", lkNone, BtUnknown) acl("CATEGORIES", lkShell, BtCategory, "Makefile: set, append; Makefile.common: set, default, append") - sys("CC_VERSION", lkNone, BtMessage) - sys("CC", lkNone, BtShellCommand) + sysload("CC_VERSION", lkNone, BtMessage) + sysload("CC", lkNone, BtShellCommand) pkglist("CFLAGS", lkShell, BtCFlag) // may also be changed by the user pkglist("CFLAGS.*", lkShell, BtCFlag) // may also be changed by the user acl("CHECK_BUILTIN", lkNone, BtYesNo, "builtin.mk: default; Makefile: set") - acl("CHECK_BUILTIN.*", lkNone, BtYesNo, "Makefile, options.mk, buildlink3.mk: set; builtin.mk: default; *: use-loadtime") + acl("CHECK_BUILTIN.*", lkNone, BtYesNo, "Makefile, options.mk, buildlink3.mk: set; builtin.mk: default, use-loadtime; *: use-loadtime") acl("CHECK_FILES_SKIP", lkShell, BtBasicRegularExpression, "Makefile, Makefile.common: append") pkg("CHECK_FILES_SUPPORTED", lkNone, BtYesNo) usr("CHECK_HEADERS", lkNone, BtYesNo) @@ -559,7 +578,14 @@ func (src *Pkgsrc) InitVartypes() { pkg("CMAKE_ARG_PATH", lkNone, BtPathname) pkglist("CMAKE_ARGS", lkShell, BtShellWord) pkglist("CMAKE_ARGS.*", lkShell, BtShellWord) + pkglist("CMAKE_DEPENDENCIES_REWRITE", lkShell, BtPathmask) // Relative to WRKSRC + pkglist("CMAKE_MODULE_PATH_OVERRIDE", lkShell, BtPathmask) // Relative to WRKSRC + pkg("CMAKE_PKGSRC_BUILD_FLAGS", lkNone, BtYesNo) + pkglist("CMAKE_PREFIX_PATH", lkShell, BtPathmask) + pkglist("CMAKE_USE_GNU_INSTALL_DIRS", lkNone, BtYesNo) + pkg("CMAKE_INSTALL_PREFIX", lkNone, BtPathname) // The default is ${PREFIX}. acl("COMMENT", lkNone, BtComment, "Makefile, Makefile.common: set, append") + sys("COMPILE.*", lkNone, BtShellCommand) acl("COMPILER_RPATH_FLAG", lkNone, enum("-Wl,-rpath"), "*: use") pkglist("CONFIGURE_ARGS", lkShell, BtShellWord) pkglist("CONFIGURE_ARGS.*", lkShell, BtShellWord) @@ -586,6 +612,7 @@ func (src *Pkgsrc) InitVartypes() { pkglist("CXXFLAGS", lkShell, BtCFlag) pkglist("CXXFLAGS.*", lkShell, BtCFlag) pkglist("CWRAPPERS_APPEND.*", lkShell, BtShellWord) + sys("DEFAULT_DISTFILES", lkShell, BtFetchURL) // From mk/fetch/bsd.fetch-vars.mk. acl("DEINSTALL_FILE", lkNone, BtPathname, "Makefile: set") acl("DEINSTALL_SRC", lkShell, BtPathname, "Makefile: set; Makefile.common: default, set") acl("DEINSTALL_TEMPLATES", lkShell, BtPathname, "Makefile: set, append; Makefile.common: set, default, append") @@ -650,8 +677,8 @@ func (src *Pkgsrc) InitVartypes() { sys("EMUL_OPSYS", lkNone, enum("darwin freebsd hpux irix linux osf1 solaris sunos none")) pkg("EMUL_PKG_FMT", lkNone, enum("plain rpm")) usr("EMUL_PLATFORM", lkNone, BtEmulPlatform) - pkg("EMUL_PLATFORMS", lkShell, BtEmulPlatform) - usr("EMUL_PREFER", lkShell, BtEmulPlatform) + pkg("EMUL_PLATFORMS", lkSpace, BtEmulPlatform) + usr("EMUL_PREFER", lkSpace, BtEmulPlatform) pkg("EMUL_REQD", lkSpace, BtDependency) usr("EMUL_TYPE.*", lkNone, enum("native builtin suse suse-9.1 suse-9.x suse-10.0 suse-10.x")) sys("ERROR_CAT", lkNone, BtShellCommand) @@ -688,6 +715,8 @@ func (src *Pkgsrc) InitVartypes() { acl("FONTS_DIRS.*", lkShell, BtPathname, "Makefile: set, append, use; Makefile.common: append, use") sys("GAMEDATAMODE", lkNone, BtFileMode) sys("GAMES_GROUP", lkNone, BtUserGroupName) + sys("GAMEDATA_PERMS", lkShell, BtPerms) + sys("GAMEDIR_PERMS", lkShell, BtPerms) sys("GAMEMODE", lkNone, BtFileMode) sys("GAMES_USER", lkNone, BtUserGroupName) pkglist("GCC_REQD", lkShell, BtVersion) @@ -756,6 +785,7 @@ func (src *Pkgsrc) InitVartypes() { sys("LD", lkNone, BtShellCommand) pkglist("LDFLAGS", lkShell, BtLdFlag) pkglist("LDFLAGS.*", lkShell, BtLdFlag) + sysload("LIBABISUFFIX", lkNone, BtIdentifier) // Can also be empty. sys("LIBGRP", lkNone, BtUserGroupName) sys("LIBMODE", lkNone, BtFileMode) sys("LIBOWN", lkNone, BtUserGroupName) @@ -768,16 +798,19 @@ func (src *Pkgsrc) InitVartypes() { acl("LICENCE", lkNone, BtLicense, "Makefile, Makefile.common, options.mk: set, append") acl("LICENSE", lkNone, BtLicense, "Makefile, Makefile.common, options.mk: set, append") pkg("LICENSE_FILE", lkNone, BtPathname) + sys("LINK.*", lkNone, BtShellCommand) sys("LINKER_RPATH_FLAG", lkNone, BtShellWord) + sys("LITTLEENDIANPLATFORMS", lkSpace, BtMachinePlatformPattern) sys("LOWER_OPSYS", lkNone, BtIdentifier) sys("LOWER_VENDOR", lkNone, BtIdentifier) + sys("LP64PLATFORMS", lkSpace, BtMachinePlatformPattern) acl("LTCONFIG_OVERRIDE", lkShell, BtPathmask, "Makefile: set, append; Makefile.common: append") - sys("MACHINE_ARCH", lkNone, enumMachineArch) - sys("MACHINE_GNU_ARCH", lkNone, enumMachineGnuArch) - sys("MACHINE_GNU_PLATFORM", lkNone, BtMachineGnuPlatform) - sys("MACHINE_PLATFORM", lkNone, BtMachinePlatform) + sysload("MACHINE_ARCH", lkNone, enumMachineArch) + sysload("MACHINE_GNU_ARCH", lkNone, enumMachineGnuArch) + sysload("MACHINE_GNU_PLATFORM", lkNone, BtMachineGnuPlatform) + sysload("MACHINE_PLATFORM", lkNone, BtMachinePlatform) acl("MAINTAINER", lkNone, BtMailAddress, "Makefile: set; Makefile.common: default") - sys("MAKE", lkNone, BtShellCommand) + sysload("MAKE", lkNone, BtShellCommand) pkglist("MAKEFLAGS", lkShell, BtShellWord) acl("MAKEVARS", lkShell, BtVariableName, "buildlink3.mk, builtin.mk, hacks.mk: append") pkglist("MAKE_DIRS", lkShell, BtPathname) @@ -798,6 +831,7 @@ func (src *Pkgsrc) InitVartypes() { pkglist("MASTER_SITES", lkShell, BtFetchURL) sys("MASTER_SITE_APACHE", lkShell, BtFetchURL) sys("MASTER_SITE_BACKUP", lkShell, BtFetchURL) + sys("MASTER_SITE_CRATESIO", lkShell, BtFetchURL) sys("MASTER_SITE_CYGWIN", lkShell, BtFetchURL) sys("MASTER_SITE_DEBIAN", lkShell, BtFetchURL) sys("MASTER_SITE_FREEBSD", lkShell, BtFetchURL) @@ -816,9 +850,12 @@ func (src *Pkgsrc) InitVartypes() { sys("MASTER_SITE_MOZILLA_ESR", lkShell, BtFetchURL) sys("MASTER_SITE_MYSQL", lkShell, BtFetchURL) sys("MASTER_SITE_NETLIB", lkShell, BtFetchURL) + sys("MASTER_SITE_OPENBSD", lkShell, BtFetchURL) sys("MASTER_SITE_OPENOFFICE", lkShell, BtFetchURL) sys("MASTER_SITE_OSDN", lkShell, BtFetchURL) sys("MASTER_SITE_PERL_CPAN", lkShell, BtFetchURL) + sys("MASTER_SITE_PGSQL", lkShell, BtFetchURL) + sys("MASTER_SITE_PYPI", lkShell, BtFetchURL) sys("MASTER_SITE_R_CRAN", lkShell, BtFetchURL) sys("MASTER_SITE_RUBYGEMS", lkShell, BtFetchURL) sys("MASTER_SITE_SOURCEFORGE", lkShell, BtFetchURL) @@ -827,12 +864,14 @@ func (src *Pkgsrc) InitVartypes() { sys("MASTER_SITE_TEX_CTAN", lkShell, BtFetchURL) sys("MASTER_SITE_XCONTRIB", lkShell, BtFetchURL) sys("MASTER_SITE_XEMACS", lkShell, BtFetchURL) + sys("MASTER_SITE_XORG", lkShell, BtFetchURL) pkglist("MESSAGE_SRC", lkShell, BtPathname) acl("MESSAGE_SUBST", lkShell, BtShellWord, "Makefile, Makefile.common, options.mk: append") pkg("META_PACKAGE", lkNone, BtYes) sys("MISSING_FEATURES", lkShell, BtIdentifier) acl("MYSQL_VERSIONS_ACCEPTED", lkShell, mysqlVersions, "Makefile: set") usr("MYSQL_VERSION_DEFAULT", lkNone, BtVersion) + sys("NATIVE_CC", lkNone, BtShellCommand) // See mk/platform/tools.NetBSD.mk (and some others). sys("NM", lkNone, BtShellCommand) sys("NONBINMODE", lkNone, BtFileMode) pkg("NOT_FOR_COMPILER", lkShell, compilers) @@ -852,13 +891,15 @@ func (src *Pkgsrc) InitVartypes() { acl("NO_PKGTOOLS_REQD_CHECK", lkNone, BtYes, "Makefile: set") acl("NO_SRC_ON_CDROM", lkNone, BtRestricted, "Makefile, Makefile.common: set") acl("NO_SRC_ON_FTP", lkNone, BtRestricted, "Makefile, Makefile.common: set") + sysload("OBJECT_FMT", lkNone, enum("COFF ECOFF ELF SOM XCOFF Mach-O PE a.out")) pkglist("ONLY_FOR_COMPILER", lkShell, compilers) pkglist("ONLY_FOR_PLATFORM", lkSpace, BtMachinePlatformPattern) pkg("ONLY_FOR_UNPRIVILEGED", lkNone, BtYesNo) - sys("OPSYS", lkNone, BtIdentifier) + sysload("OPSYS", lkNone, BtIdentifier) acl("OPSYSVARS", lkShell, BtVariableName, "Makefile, Makefile.common: append") acl("OSVERSION_SPECIFIC", lkNone, BtYes, "Makefile, Makefile.common: set") - sys("OS_VERSION", lkNone, BtVersion) + sysload("OS_VERSION", lkNone, BtVersion) + sysload("OSX_VERSION", lkNone, BtVersion) // See mk/platform/Darwin.mk. pkg("OVERRIDE_DIRDEPTH*", lkNone, BtInteger) pkg("OVERRIDE_GNU_CONFIG_SCRIPTS", lkNone, BtYes) acl("OWNER", lkNone, BtMailAddress, "Makefile: set; Makefile.common: default") @@ -874,9 +915,27 @@ func (src *Pkgsrc) InitVartypes() { acl("PATCH_DIST_STRIP*", lkNone, BtShellWord, "buildlink3.mk, builtin.mk:; Makefile, Makefile.common, *.mk: set") acl("PATCH_SITES", lkShell, BtFetchURL, "Makefile, Makefile.common, options.mk: set") acl("PATCH_STRIP", lkNone, BtShellWord, "") + sys("PATH", lkNone, BtPathlist) // From the PATH environment variable. + sys("PAXCTL", lkNone, BtShellCommand) // See mk/pax.mk. acl("PERL5_PACKLIST", lkShell, BtPerl5Packlist, "Makefile: set; options.mk: set, append") acl("PERL5_PACKLIST_DIR", lkNone, BtPathname, "") pkg("PERL5_REQD", lkShell, BtVersion) + sys("PERL5_INSTALLARCHLIB", lkNone, BtPathname) // See lang/perl5/vars.mk + sys("PERL5_INSTALLSCRIPT", lkNone, BtPathname) + sys("PERL5_INSTALLVENDORBIN", lkNone, BtPathname) + sys("PERL5_INSTALLVENDORSCRIPT", lkNone, BtPathname) + sys("PERL5_INSTALLVENDORARCH", lkNone, BtPathname) + sys("PERL5_INSTALLVENDORLIB", lkNone, BtPathname) + sys("PERL5_INSTALLVENDORMAN1DIR", lkNone, BtPathname) + sys("PERL5_INSTALLVENDORMAN3DIR", lkNone, BtPathname) + sys("PERL5_SUB_INSTALLARCHLIB", lkNone, BtPrefixPathname) // See lang/perl5/vars.mk + sys("PERL5_SUB_INSTALLSCRIPT", lkNone, BtPrefixPathname) + sys("PERL5_SUB_INSTALLVENDORBIN", lkNone, BtPrefixPathname) + sys("PERL5_SUB_INSTALLVENDORSCRIPT", lkNone, BtPrefixPathname) + sys("PERL5_SUB_INSTALLVENDORARCH", lkNone, BtPrefixPathname) + sys("PERL5_SUB_INSTALLVENDORLIB", lkNone, BtPrefixPathname) + sys("PERL5_SUB_INSTALLVENDORMAN1DIR", lkNone, BtPrefixPathname) + sys("PERL5_SUB_INSTALLVENDORMAN3DIR", lkNone, BtPrefixPathname) pkg("PERL5_USE_PACKLIST", lkNone, BtYesNo) sys("PGSQL_PREFIX", lkNone, BtPathname) acl("PGSQL_VERSIONS_ACCEPTED", lkShell, pgsqlVersions, "") @@ -895,18 +954,21 @@ func (src *Pkgsrc) InitVartypes() { sys("PKGLOCALEDIR", lkNone, BtPathname) pkg("PKGNAME", lkNone, BtPkgName) sys("PKGNAME_NOREV", lkNone, BtPkgName) - sys("PKGPATH", lkNone, BtPathname) + sysload("PKGPATH", lkNone, BtPathname) acl("PKGREPOSITORY", lkNone, BtUnknown, "") acl("PKGREVISION", lkNone, BtPkgRevision, "Makefile: set") sys("PKGSRCDIR", lkNone, BtPathname) acl("PKGSRCTOP", lkNone, BtYes, "Makefile: set") + sys("PKGSRC_SETENV", lkNone, BtShellCommand) acl("PKGTOOLS_ENV", lkShell, BtShellWord, "") sys("PKGVERSION", lkNone, BtVersion) + sys("PKGVERSION_NOREV", lkNone, BtVersion) // Without the nb* part. sys("PKGWILDCARD", lkNone, BtFilemask) sys("PKG_ADMIN", lkNone, BtShellCommand) sys("PKG_APACHE", lkNone, enum("apache24")) pkg("PKG_APACHE_ACCEPTED", lkShell, enum("apache24")) usr("PKG_APACHE_DEFAULT", lkNone, enum("apache24")) + sysload("PKG_BUILD_OPTIONS.*", lkShell, BtOption) usr("PKG_CONFIG", lkNone, BtYes) // ^^ No, this is not the popular command from GNOME, but the setting // whether the pkgsrc user wants configuration files automatically @@ -918,6 +980,7 @@ func (src *Pkgsrc) InitVartypes() { sys("PKG_DELETE", lkNone, BtShellCommand) acl("PKG_DESTDIR_SUPPORT", lkShell, enum("destdir user-destdir"), "Makefile, Makefile.common: set") pkglist("PKG_FAIL_REASON", lkShell, BtShellWord) + sysload("PKG_FORMAT", lkNone, BtIdentifier) acl("PKG_GECOS.*", lkNone, BtMessage, "Makefile: set") acl("PKG_GID.*", lkNone, BtInteger, "Makefile: set") acl("PKG_GROUPS", lkShell, BtShellWord, "Makefile: set, append") @@ -950,7 +1013,10 @@ func (src *Pkgsrc) InitVartypes() { acl("PKG_SUGGESTED_OPTIONS", lkShell, BtOption, "Makefile, Makefile.common, options.mk: set, append") acl("PKG_SUGGESTED_OPTIONS.*", lkShell, BtOption, "Makefile, Makefile.common, options.mk: set, append") acl("PKG_SUPPORTED_OPTIONS", lkShell, BtOption, "Makefile: set, append; Makefile.common: set; options.mk: set, append, use") - pkg("PKG_SYSCONFDIR*", lkNone, BtPathname) + acl("PKG_SYSCONFDIR*", lkNone, BtPathname, ""+ + "Makefile: set, use, use-loadtime; "+ + "buildlink3.mk, builtin.mk: use-loadtime; "+ + "Makefile.*, *.mk: default, set, use, use-loadtime") pkglist("PKG_SYSCONFDIR_PERMS", lkShell, BtPerms) sys("PKG_SYSCONFBASEDIR", lkNone, BtPathname) pkg("PKG_SYSCONFSUBDIR", lkNone, BtPathname) @@ -959,7 +1025,7 @@ func (src *Pkgsrc) InitVartypes() { acl("PKG_USERS", lkShell, BtShellWord, "Makefile: set, append") pkglist("PKG_USERS_VARS", lkShell, BtVariableName) acl("PKG_USE_KERBEROS", lkNone, BtYes, "Makefile, Makefile.common: set") - pkg("PLIST.*", lkNone, BtYes) + pkgload("PLIST.*", lkNone, BtYes) pkglist("PLIST_VARS", lkShell, BtIdentifier) pkglist("PLIST_SRC", lkShell, BtRelativePkgPath) pkglist("PLIST_SUBST", lkShell, BtShellWord) @@ -974,7 +1040,7 @@ func (src *Pkgsrc) InitVartypes() { sys("PTHREAD_LDFLAGS", lkShell, BtLdFlag) sys("PTHREAD_LIBS", lkShell, BtLdFlag) acl("PTHREAD_OPTS", lkShell, enum("native optional require"), "Makefile: set, append; Makefile.common, buildlink3.mk: append") - sys("PTHREAD_TYPE", lkNone, BtIdentifier) // Or "native" or "none". + sysload("PTHREAD_TYPE", lkNone, BtIdentifier) // Or "native" or "none". pkg("PY_PATCHPLIST", lkNone, BtYes) acl("PYPKGPREFIX", lkNone, enum("py27 py34 py35 py36"), "pyversion.mk: set; *: use-loadtime, use") pkg("PYTHON_FOR_BUILD_ONLY", lkNone, BtYes) @@ -988,6 +1054,8 @@ func (src *Pkgsrc) InitVartypes() { pkglist("RCD_SCRIPTS", lkShell, BtFilename) acl("RCD_SCRIPT_SRC.*", lkNone, BtPathname, "Makefile: set") acl("RCD_SCRIPT_WRK.*", lkNone, BtPathname, "Makefile: set") + usr("REAL_ROOT_USER", lkNone, BtUserGroupName) + usr("REAL_ROOT_GROUP", lkNone, BtUserGroupName) acl("REPLACE.*", lkNone, BtUnknown, "Makefile: set") pkglist("REPLACE_AWK", lkShell, BtPathmask) pkglist("REPLACE_BASH", lkShell, BtPathmask) @@ -1013,7 +1081,8 @@ func (src *Pkgsrc) InitVartypes() { sys("RUN", lkNone, BtShellCommand) sys("RUN_LDCONFIG", lkNone, BtYesNo) acl("SCRIPTS_ENV", lkShell, BtShellWord, "Makefile, Makefile.common: append") - usr("SETUID_ROOT_PERMS", lkShell, BtShellWord) + usr("SETGID_GAMES_PERMS", lkShell, BtPerms) + usr("SETUID_ROOT_PERMS", lkShell, BtPerms) pkg("SET_LIBDIR", lkNone, BtYes) sys("SHAREGRP", lkNone, BtUserGroupName) sys("SHAREMODE", lkNone, BtFileMode) @@ -1022,6 +1091,7 @@ func (src *Pkgsrc) InitVartypes() { acl("SHLIB_HANDLING", lkNone, enum("YES NO no"), "") acl("SHLIBTOOL", lkNone, BtShellCommand, "Makefile: use") acl("SHLIBTOOL_OVERRIDE", lkShell, BtPathmask, "Makefile: set, append; Makefile.common: append") + sysload("SHLIB_TYPE", lkNone, enum("COFF ECOFF ELF SOM XCOFF Mach-O PE PEwin a.out aixlib dylib none")) acl("SITES.*", lkShell, BtFetchURL, "Makefile, Makefile.common, options.mk: set, append, use") usr("SMF_PREFIS", lkNone, BtPathname) pkg("SMF_SRCDIR", lkNone, BtPathname) @@ -1058,7 +1128,7 @@ func (src *Pkgsrc) InitVartypes() { sys("TOOLS_GNU_MISSING", lkShell, BtTool) sys("TOOLS_NOOP", lkShell, BtTool) sys("TOOLS_PATH.*", lkNone, BtPathname) - sys("TOOLS_PLATFORM.*", lkNone, BtShellCommand) + sysload("TOOLS_PLATFORM.*", lkNone, BtShellCommand) sys("TOUCH_FLAGS", lkShell, BtShellWord) pkglist("UAC_REQD_EXECS", lkShell, BtPrefixPathname) acl("UNLIMIT_RESOURCES", lkShell, enum("cputime datasize memorysize stacksize"), "Makefile: set, append; Makefile.common: append") @@ -1067,8 +1137,9 @@ func (src *Pkgsrc) InitVartypes() { pkglist("UNWRAP_FILES", lkShell, BtPathmask) usr("UPDATE_TARGET", lkShell, BtIdentifier) pkg("USERGROUP_PHASE", lkNone, enum("configure build pre-install")) + usr("USER_ADDITIONAL_PKGS", lkShell, BtPkgPath) pkg("USE_BSD_MAKEFILE", lkNone, BtYes) - acl("USE_BUILTIN.*", lkNone, BtYesNoIndirectly, "builtin.mk: set") + acl("USE_BUILTIN.*", lkNone, BtYesNoIndirectly, "buildlink3.mk: use-loadtime; builtin.mk: set, use, use-loadtime; options.mk: use-loadtime") pkg("USE_CMAKE", lkNone, BtYes) usr("USE_DESTDIR", lkNone, BtYes) pkglist("USE_FEATURES", lkShell, BtIdentifier) @@ -1088,17 +1159,19 @@ func (src *Pkgsrc) InitVartypes() { pkg("USE_PKGINSTALL", lkNone, BtYes) pkg("USE_PKGLOCALEDIR", lkNone, BtYesNo) usr("USE_PKGSRC_GCC", lkNone, BtYes) - acl("USE_TOOLS", lkShell, BtTool, "*: append") - acl("USE_TOOLS.*", lkShell, BtTool, "*: append") + acl("USE_TOOLS", lkShell, BtTool, "*: append, use-loadtime") + acl("USE_TOOLS.*", lkShell, BtTool, "*: append, use-loadtime") pkg("USE_X11", lkNone, BtYes) sys("WARNINGS", lkShell, BtShellWord) sys("WARNING_MSG", lkNone, BtShellCommand) sys("WARNING_CAT", lkNone, BtShellCommand) + sysload("WRAPPER_DIR", lkNone, BtPathname) acl("WRAPPER_REORDER_CMDS", lkShell, BtWrapperReorder, "Makefile, Makefile.common, buildlink3.mk: append") pkg("WRAPPER_SHELL", lkNone, BtShellCommand) acl("WRAPPER_TRANSFORM_CMDS", lkShell, BtWrapperTransform, "Makefile, Makefile.common, buildlink3.mk: append") sys("WRKDIR", lkNone, BtPathname) pkg("WRKSRC", lkNone, BtWrkdirSubdirectory) + pkglist("X11_LDFLAGS", lkShell, BtLdFlag) sys("X11_PKGSRCDIR.*", lkNone, BtPathname) usr("XAW_TYPE", lkNone, enum("3d neXtaw standard xpm")) acl("XMKMF_FLAGS", lkShell, BtShellWord, "") @@ -1115,7 +1188,7 @@ func (src *Pkgsrc) InitVartypes() { func enum(values string) *BasicType { valueMap := make(map[string]bool) - for _, value := range splitOnSpace(values) { + for _, value := range fields(values) { valueMap[value] = true } name := "enum: " + values + " " // See IsEnum diff --git a/pkgtools/pkglint/files/vardefs_test.go b/pkgtools/pkglint/files/vardefs_test.go index 23a0aa3d1e2..0eb2ccd0efb 100644 --- a/pkgtools/pkglint/files/vardefs_test.go +++ b/pkgtools/pkglint/files/vardefs_test.go @@ -61,3 +61,17 @@ func (s *Suite) Test_parseACLEntries(c *check.C) { func() { parseACLEntries("VARNAME", "*.mk: use; buildlink3.mk: append") }, "FATAL: Ineffective ACL glob \"buildlink3.mk\" for \"VARNAME\".") } + +func (s *Suite) Test_Pkgsrc_InitVartypes__LP64PLATFORMS(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wall") + pkg := t.SetupPackage("category/package", + "BROKEN_ON_PLATFORM=\t${LP64PLATFORMS}") + + G.CheckDirent(pkg) + + // No warning about a missing :Q operator. + // All PLATFORM variables must be either lkNone or lkSpace. + t.CheckOutputEmpty() +} diff --git a/pkgtools/pkglint/files/vartype.go b/pkgtools/pkglint/files/vartype.go index ff93c5142ce..e21aeb93ce3 100644 --- a/pkgtools/pkglint/files/vartype.go +++ b/pkgtools/pkglint/files/vartype.go @@ -70,9 +70,9 @@ func (perms ACLPermissions) HumanString() string { return strings.TrimRight(result, ", ") } -func (vt *Vartype) EffectivePermissions(fname string) ACLPermissions { +func (vt *Vartype) EffectivePermissions(basename string) ACLPermissions { for _, aclEntry := range vt.aclEntries { - if m, _ := path.Match(aclEntry.glob, path.Base(fname)); m { + if m, _ := path.Match(aclEntry.glob, basename); m { return aclEntry.permissions } } @@ -118,7 +118,7 @@ func (vt *Vartype) IsConsideredList() bool { } func (vt *Vartype) MayBeAppendedTo() bool { - return vt.kindOfList != lkNone || vt.IsConsideredList() + return vt.kindOfList != lkNone || vt.IsConsideredList() || vt.basicType == BtComment } func (vt *Vartype) String() string { diff --git a/pkgtools/pkglint/files/vartype_test.go b/pkgtools/pkglint/files/vartype_test.go index e39e68a9c9c..3142ba187f8 100644 --- a/pkgtools/pkglint/files/vartype_test.go +++ b/pkgtools/pkglint/files/vartype_test.go @@ -9,17 +9,16 @@ func (s *Suite) Test_Vartype_EffectivePermissions(c *check.C) { t.SetupVartypes() - if t := G.Pkgsrc.vartypes["PREFIX"]; c.Check(t, check.NotNil) { - c.Check(t.basicType.name, equals, "Pathname") - c.Check(t.aclEntries, check.DeepEquals, []ACLEntry{{glob: "*", permissions: aclpUse}}) - c.Check(t.EffectivePermissions("Makefile"), equals, aclpUse) + if typ := G.Pkgsrc.vartypes["PREFIX"]; c.Check(typ, check.NotNil) { + c.Check(typ.basicType.name, equals, "Pathname") + c.Check(typ.aclEntries, check.DeepEquals, []ACLEntry{{glob: "*", permissions: aclpUse}}) + c.Check(typ.EffectivePermissions("Makefile"), equals, aclpUse) } - if t := G.Pkgsrc.vartypes["EXTRACT_OPTS"]; c.Check(t, check.NotNil) { - c.Check(t.basicType.name, equals, "ShellWord") - c.Check(t.EffectivePermissions("Makefile"), equals, aclpAppend|aclpSet) - c.Check(t.EffectivePermissions("../Makefile"), equals, aclpAppend|aclpSet) - c.Check(t.EffectivePermissions("options.mk"), equals, aclpUnknown) + if typ := G.Pkgsrc.vartypes["EXTRACT_OPTS"]; c.Check(typ, check.NotNil) { + c.Check(typ.basicType.name, equals, "ShellWord") + c.Check(typ.EffectivePermissions("Makefile"), equals, aclpAppend|aclpSet) + c.Check(typ.EffectivePermissions("options.mk"), equals, aclpUnknown) } } diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go index 74a5315d823..aaa6a0edb45 100644 --- a/pkgtools/pkglint/files/vartypecheck.go +++ b/pkgtools/pkglint/files/vartypecheck.go @@ -24,12 +24,10 @@ type VartypeCheck struct { // fields except the value. This is typically used when checking parts // of composite types. func NewVartypeCheckValue(vc *VartypeCheck, value string) *VartypeCheck { - valueNoVar := vc.MkLine.WithoutMakeVariables(value) - - copy := *vc - copy.Value = value - copy.ValueNoVar = valueNoVar - return © + newVc := *vc + newVc.Value = value + newVc.ValueNoVar = vc.MkLine.WithoutMakeVariables(value) + return &newVc } const ( @@ -152,7 +150,7 @@ func (cv *VartypeCheck) CFlag() { } } -// The single-line description of the package. +// Comment checks for the single-line description of the package. func (cv *VartypeCheck) Comment() { line, value := cv.Line, cv.Value @@ -179,7 +177,7 @@ func (cv *VartypeCheck) Comment() { "provide additional information instead.") } } - if matches(value, `^[a-z]`) { + if matches(value, `^[a-z]`) && cv.Op == opAssign { line.Warnf("COMMENT should start with a capital letter.") } if hasSuffix(value, ".") { @@ -763,7 +761,7 @@ func (cv *VartypeCheck) PkgRevision() { if !matches(cv.Value, `^[1-9]\d*$`) { cv.Line.Warnf("%s must be a positive integer number.", cv.Varname) } - if path.Base(cv.Line.Filename) != "Makefile" { + if cv.Line.Basename != "Makefile" { cv.Line.Errorf("%s only makes sense directly in the package Makefile.", cv.Varname) Explain( "Usually, different packages using the same Makefile.common have", diff --git a/pkgtools/pkglint/files/vartypecheck_test.go b/pkgtools/pkglint/files/vartypecheck_test.go index 7f3db87b21f..d9e62e447fa 100644 --- a/pkgtools/pkglint/files/vartypecheck_test.go +++ b/pkgtools/pkglint/files/vartypecheck_test.go @@ -907,9 +907,9 @@ func (s *Suite) Test_VartypeCheck_Tool(c *check.C) { t := s.Init(c) vt := NewVartypeCheckTester(t, (*VartypeCheck).Tool) - t.SetupToolUsable("tool1", "") - t.SetupToolUsable("tool2", "") - t.SetupToolUsable("tool3", "") + t.SetupTool("tool1", "", AtRunTime) + t.SetupTool("tool2", "", AtRunTime) + t.SetupTool("tool3", "", AtRunTime) vt.Varname("USE_TOOLS") vt.Op(opAssignAppend) |