diff options
Diffstat (limited to 'pkgtools')
22 files changed, 480 insertions, 142 deletions
diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index b09a3c9283e..965a4cf21a8 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.632 2020/02/17 20:22:21 rillig Exp $ +# $NetBSD: Makefile,v 1.633 2020/03/07 23:35:35 rillig Exp $ -PKGNAME= pkglint-19.4.9 +PKGNAME= pkglint-19.4.10 CATEGORIES= pkgtools DISTNAME= tools MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/} diff --git a/pkgtools/pkglint/files/homepage_test.go b/pkgtools/pkglint/files/homepage_test.go index bdb9e3cd063..49a6d90e491 100644 --- a/pkgtools/pkglint/files/homepage_test.go +++ b/pkgtools/pkglint/files/homepage_test.go @@ -64,8 +64,8 @@ func (s *Suite) Test_HomepageChecker_checkBasedOnMasterSites(c *check.C) { vt.Output( "WARN: filename.mk:11: HOMEPAGE should not be defined in terms of MASTER_SITEs.") - delete(pkg.vars.firstDef, "MASTER_SITES") - delete(pkg.vars.lastDef, "MASTER_SITES") + pkg.vars.v("MASTER_SITES").firstDef = nil + pkg.vars.v("MASTER_SITES").lastDef = nil pkg.vars.Define("MASTER_SITES", t.NewMkLine(pkg.File("Makefile"), 5, "MASTER_SITES=\thttps://cdn.NetBSD.org/pub/pkgsrc/distfiles/")) @@ -76,8 +76,8 @@ func (s *Suite) Test_HomepageChecker_checkBasedOnMasterSites(c *check.C) { "WARN: filename.mk:21: HOMEPAGE should not be defined in terms of MASTER_SITEs. " + "Use https://cdn.NetBSD.org/pub/pkgsrc/distfiles/ directly.") - delete(pkg.vars.firstDef, "MASTER_SITES") - delete(pkg.vars.lastDef, "MASTER_SITES") + pkg.vars.v("MASTER_SITES").firstDef = nil + pkg.vars.v("MASTER_SITES").lastDef = nil pkg.vars.Define("MASTER_SITES", t.NewMkLine(pkg.File("Makefile"), 5, "MASTER_SITES=\t${MASTER_SITE_GITHUB}")) @@ -89,8 +89,8 @@ func (s *Suite) Test_HomepageChecker_checkBasedOnMasterSites(c *check.C) { vt.Output( "WARN: filename.mk:31: HOMEPAGE should not be defined in terms of MASTER_SITEs.") - delete(pkg.vars.firstDef, "MASTER_SITES") - delete(pkg.vars.lastDef, "MASTER_SITES") + pkg.vars.v("MASTER_SITES").firstDef = nil + pkg.vars.v("MASTER_SITES").lastDef = nil pkg.vars.Define("MASTER_SITES", t.NewMkLine(pkg.File("Makefile"), 5, "MASTER_SITES=\t# none")) @@ -393,7 +393,7 @@ func (s *Suite) Test_HomepageChecker_checkReachable(c *check.C) { "https://no-such-name.example.org/", // The "unknown network error" is for compatibility with Go < 1.13. `^WARN: filename\.mk:1: Homepage "https://no-such-name.example.org/" `+ - `cannot be checked: (name not found|unknown network error:.*)$`) + `cannot be checked: (name not found|timeout|unknown network error:.*)$`) // Syntactically invalid URLs are silently skipped since VartypeCheck.URL // already warns about them. diff --git a/pkgtools/pkglint/files/logging.go b/pkgtools/pkglint/files/logging.go index eeb194263a0..e0f3db1b792 100644 --- a/pkgtools/pkglint/files/logging.go +++ b/pkgtools/pkglint/files/logging.go @@ -368,7 +368,7 @@ func (l *Logger) ShowSummary(args []string) { } l.out.Write(sprintf("%s found.\n", - joinSkipEmptyCambridge("and", + joinCambridge("and", num(l.errors, "error", "errors"), num(l.warnings, "warning", "warnings"), num(l.notes, "note", "notes")))) diff --git a/pkgtools/pkglint/files/mkcondchecker.go b/pkgtools/pkglint/files/mkcondchecker.go index 3bba635f716..ebe0915d5dd 100644 --- a/pkgtools/pkglint/files/mkcondchecker.go +++ b/pkgtools/pkglint/files/mkcondchecker.go @@ -29,6 +29,7 @@ func (ck *MkCondChecker) Check() { checkVarUse := func(varuse *MkVarUse) { var vartype *Vartype // TODO: Insert a better type guess here. + // See Test_MkVarUseChecker_checkAssignable__shell_command_in_exists. vuc := VarUseContext{vartype, VucLoadTime, VucQuotPlain, false} NewMkVarUseChecker(varuse, ck.MkLines, mkline).Check(&vuc) } diff --git a/pkgtools/pkglint/files/mkline_test.go b/pkgtools/pkglint/files/mkline_test.go index 312b24f71d6..2114dcfcdca 100644 --- a/pkgtools/pkglint/files/mkline_test.go +++ b/pkgtools/pkglint/files/mkline_test.go @@ -1034,6 +1034,10 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__shellword_part(c *check.C) { } // Tools, when used in a shell command, must not be quoted. +// Shell commands may have command line arguments, pathnames must not. +// The original intention of having both CONFIG_SHELL and CONFIG_SHELL_FLAGS +// was to separate the command from its arguments. +// It doesn't hurt though if the command includes some of the arguments as well. func (s *Suite) Test_MkLine_VariableNeedsQuoting__tool_in_shell_command(c *check.C) { t := s.Init(c) @@ -1043,11 +1047,14 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__tool_in_shell_command(c *check mklines := t.SetUpFileMkLines("Makefile", MkCvsID, "", - "CONFIG_SHELL=\t${BASH}") + "CONFIG_SHELL=\t${BASH}", + "DIST_SUBDIR=\t${BASH}") mklines.Check() - t.CheckOutputEmpty() + t.CheckOutputLines( + "WARN: ~/Makefile:4: Incompatible types: " + + "BASH (type \"ShellCommand\") cannot be assigned to type \"Pathname\".") } // This test provides code coverage for the "switch vuc.quoting" in the case diff --git a/pkgtools/pkglint/files/mklines.go b/pkgtools/pkglint/files/mklines.go index 378b98b23e7..80e7679f950 100644 --- a/pkgtools/pkglint/files/mklines.go +++ b/pkgtools/pkglint/files/mklines.go @@ -194,9 +194,9 @@ func (mklines *MkLines) collectDocumentedVariables() { // The commentLines include the the line containing the variable name, // leaving 2 of these 3 lines for the actual documentation. if commentLines >= 3 && relevant { - forEachStringMkLine(scope.used, func(varname string, mkline *MkLine) { - mklines.allVars.Define(varname, mkline) - mklines.allVars.Use(varname, mkline, VucRunTime) + scope.forEach(func(varname string, data *scopeVar) { + mklines.allVars.Define(varname, data.used) + mklines.allVars.Use(varname, data.used, VucRunTime) }) } diff --git a/pkgtools/pkglint/files/mklines_test.go b/pkgtools/pkglint/files/mklines_test.go index acdf1ec616a..cfc0d4040e1 100644 --- a/pkgtools/pkglint/files/mklines_test.go +++ b/pkgtools/pkglint/files/mklines_test.go @@ -494,7 +494,8 @@ func (s *Suite) Test_MkLines_collectUsedVariables__simple(c *check.C) { mklines.collectUsedVariables() - t.CheckDeepEquals(mklines.allVars.used, map[string]*MkLine{"VAR": mkline}) + t.Check(mklines.allVars.vs, check.HasLen, 1) + t.CheckEquals(mklines.allVars.v("VAR").used, mkline) t.CheckEquals(mklines.allVars.FirstUse("VAR"), mkline) } @@ -513,7 +514,7 @@ func (s *Suite) Test_MkLines_collectUsedVariables__nested(c *check.C) { mklines.collectUsedVariables() - t.CheckEquals(len(mklines.allVars.used), 5) + t.CheckEquals(len(mklines.allVars.vs), 5) t.CheckEquals(mklines.allVars.FirstUse("lparam"), assignMkline) t.CheckEquals(mklines.allVars.FirstUse("rparam"), assignMkline) t.CheckEquals(mklines.allVars.FirstUse("inner"), shellMkline) @@ -570,9 +571,11 @@ func (s *Suite) Test_MkLines_collectDocumentedVariables(c *check.C) { mklines.collectDocumentedVariables() var varnames []string - for varname, mkline := range mklines.allVars.used { - varnames = append(varnames, sprintf("%s (line %s)", varname, mkline.Linenos())) - } + mklines.allVars.forEach(func(varname string, data *scopeVar) { + if data.used != nil { + varnames = append(varnames, sprintf("%s (line %s)", varname, data.used.Linenos())) + } + }) sort.Strings(varnames) expected := []string{ @@ -694,7 +697,7 @@ func (s *Suite) Test_MkLines_collectVariables__find_files_and_headers(c *check.C mklines.Check() t.CheckDeepEquals( - keys(mklines.allVars.firstDef), + keys(mklines.allVars.vs), []string{ "BUILTIN_FIND_FILES_VAR", "BUILTIN_FIND_HEADERS_VAR", diff --git a/pkgtools/pkglint/files/mkvarusechecker.go b/pkgtools/pkglint/files/mkvarusechecker.go index 8538c6003a8..01964291922 100644 --- a/pkgtools/pkglint/files/mkvarusechecker.go +++ b/pkgtools/pkglint/files/mkvarusechecker.go @@ -27,8 +27,10 @@ func (ck *MkVarUseChecker) Check(vuc *VarUseContext) { ck.checkVarname(vuc.time) ck.checkModifiers() + ck.checkAssignable(vuc) ck.checkQuoting(vuc) + ck.checkToolsPlatform() ck.checkBuildDefs() ck.checkDeprecated() @@ -445,6 +447,35 @@ func (ck *MkVarUseChecker) warnToolLoadTime(varname string, tool *Tool) { "except in the package Makefile itself.") } +func (ck *MkVarUseChecker) checkAssignable(vuc *VarUseContext) { + leftType := vuc.vartype + if leftType == nil || leftType.basicType != BtPathname { + return + } + rightType := G.Pkgsrc.VariableType(ck.MkLines, ck.use.varname) + if rightType == nil || rightType.basicType != BtShellCommand { + return + } + + mkline := ck.MkLine + if mkline.Varcanon() == "PKG_SHELL.*" { + switch ck.use.varname { + case "SH", "BASH", "TOOLS_PLATFORM.sh": + return + } + } + + mkline.Warnf( + "Incompatible types: %s (type %q) cannot be assigned to type %q.", + ck.use.varname, rightType.basicType.name, leftType.basicType.name) + mkline.Explain( + "Shell commands often start with a pathname.", + "They could also start with a list of environment variable", + "definitions, since that is accepted by the shell.", + "They can also contain addition command line arguments", + "that are not filenames at all.") +} + // checkVarUseWords checks whether a variable use of the form ${VAR} // or ${VAR:modifiers} is allowed in a certain context. func (ck *MkVarUseChecker) checkQuoting(vuc *VarUseContext) { @@ -638,6 +669,40 @@ func (ck *MkVarUseChecker) warnRedundantModifierQ(mod string) { fix.Apply() } +func (ck *MkVarUseChecker) checkToolsPlatform() { + if ck.MkLine.IsDirective() { + return + } + + varname := ck.use.varname + if varnameCanon(varname) != "TOOLS_PLATFORM.*" { + return + } + + indentation := ck.MkLines.indentation + switch { + case indentation.DependsOn("OPSYS"), + indentation.DependsOn("MACHINE_PLATFORM"), + indentation.DependsOn(varname): + // TODO: Only return if the conditional is on the correct OPSYS. + return + } + + toolName := varnameParam(varname) + tool := G.Pkgsrc.Tools.ByName(toolName) + if tool == nil { + return + } + + if len(tool.undefinedOn) > 0 { + ck.MkLine.Warnf("%s is undefined on %s.", + varname, joinCambridge("and", tool.undefinedOn...)) + } else if len(tool.conditionalOn) > 0 { + ck.MkLine.Warnf("%s may be undefined on %s.", + varname, joinCambridge("and", tool.conditionalOn...)) + } +} + func (ck *MkVarUseChecker) checkBuildDefs() { varname := ck.use.varname diff --git a/pkgtools/pkglint/files/mkvarusechecker_test.go b/pkgtools/pkglint/files/mkvarusechecker_test.go index 85483bd5b12..c2638fd11f0 100644 --- a/pkgtools/pkglint/files/mkvarusechecker_test.go +++ b/pkgtools/pkglint/files/mkvarusechecker_test.go @@ -957,6 +957,77 @@ func (s *Suite) Test_MkVarUseChecker_warnToolLoadTime__local_tool(c *check.C) { "WARN: ~/category/package/Makefile:7: The tool ${MK_TOOL} cannot be used at load time.") } +func (s *Suite) Test_MkVarUseChecker_checkAssignable(c *check.C) { + t := s.Init(c) + + t.SetUpVartypes() + mklines := t.NewMkLines("filename.mk", + "BUILTIN_FIND_FILES_VAR:=\tBIN_FILE", + "BUILTIN_FIND_FILES.BIN_FILE=\t${TOOLS_PLATFORM.file} /bin/file /usr/bin/file") + + mklines.ForEach(func(mkline *MkLine) { + ck := NewMkAssignChecker(mkline, mklines) + ck.checkVarassignRight() + }) + + t.CheckOutputLines( + "WARN: filename.mk:2: Incompatible types: " + + "TOOLS_PLATFORM.file (type \"ShellCommand\") " + + "cannot be assigned to type \"Pathname\".") +} + +// NetBSD's chsh program only allows a simple pathname for the shell, without +// any command line arguments. This makes sense since the shell is started +// using execve, not system (which would require shell-like argument parsing). +// +// Under the assumption that TOOLS_PLATFORM.sh does not contain any command +// line arguments, it's ok in that special case. This covers most of the +// real-life situations where this type mismatch (Pathname := ShellCommand) +// occurs. +func (s *Suite) Test_MkVarUseChecker_checkAssignable__shell_command_to_pathname(c *check.C) { + t := s.Init(c) + + t.SetUpVartypes() + t.SetUpTool("sh", "SH", AtRunTime) + t.SetUpTool("bash", "BASH", AtRunTime) + mklines := t.NewMkLines("filename.mk", + "PKG_SHELL.user=\t${TOOLS_PLATFORM.sh}", + "PKG_SHELL.user=\t${SH}", + "PKG_SHELL.user=\t${BASH}") + + mklines.ForEach(func(mkline *MkLine) { + ck := NewMkAssignChecker(mkline, mklines) + ck.checkVarassignRight() + }) + + t.CheckOutputLines( + "WARN: filename.mk:1: Please use ${TOOLS_PLATFORM.sh:Q} " + + "instead of ${TOOLS_PLATFORM.sh}.") +} + +func (s *Suite) Test_MkVarUseChecker_checkAssignable__shell_command_in_exists(c *check.C) { + t := s.Init(c) + + t.SetUpTool("sh", "SH", AfterPrefsMk) + t.SetUpTool("bash", "BASH", AfterPrefsMk) + t.SetUpPkgsrc() + t.Chdir(".") + t.FinishSetUp() + mklines := t.NewMkLines("filename.mk", + MkCvsID, + ".include \"mk/bsd.prefs.mk\"", + ".if exists(${TOOLS_PLATFORM.sh})", + ".elif exists(${SH})", + ".elif exists(${BASH})", + ".endif") + + mklines.Check() + + // TODO: Call MkVarUseChecker.checkAssignable with a VarUseContext of type + // BtPathname here. + t.CheckOutputEmpty() +} + func (s *Suite) Test_MkVarUseChecker_checkQuoting(c *check.C) { t := s.Init(c) @@ -1153,6 +1224,65 @@ func (s *Suite) Test_MkVarUseChecker_fixQuotingModifiers(c *check.C) { "AUTOFIX: ~/filename.mk:5: Replacing \"${CFLAGS:N*:Q}\" with \"${CFLAGS:N*:M*:Q}\".") } +func (s *Suite) Test_MkVarUseChecker_checkToolsPlatform(c *check.C) { + t := s.Init(c) + + t.SetUpPkgsrc() + t.SetUpTool("available", "", AfterPrefsMk) + t.SetUpTool("cond1", "", AfterPrefsMk) + t.SetUpTool("cond2", "", AfterPrefsMk) + t.SetUpTool("undefined", "", AfterPrefsMk) + t.CreateFileLines("mk/tools/tools.NetBSD.mk", + "TOOLS_PLATFORM.available?=\t/bin/available", + "TOOLS_PLATFORM.cond1?=\t/usr/cond1", + "TOOLS_PLATFORM.cond2?=\t/usr/cond2", + "TOOLS_PLATFORM.undefined?=\t/usr/undefined") + t.CreateFileLines("mk/tools/tools.SunOS.mk", + "TOOLS_PLATFORM.available?=\t/bin/available", + "", + ".if exists(/usr/gnu/bin/cond1)", + "TOOLS_PLATFORM.cond1?=\t/usr/gnu/bin/cond1", + ".endif", + "", + ".if exists(/usr/gnu/bin/cond2)", + "TOOLS_PLATFORM.cond2?=\t/usr/gnu/bin/cond2", + ".else", + "TOOLS_PLATFORM.cond2?=\t/usr/sfw/bin/cond2", + ".endif", + "", + "# No definition for undefined.") + t.Chdir(".") + t.FinishSetUp() + mklines := t.NewMkLines("filename.mk", + MkCvsID, + "", + ".include \"mk/bsd.prefs.mk\"", + "", + ".if ${OPSYS} == SunOS", + "post-build:", + "\t${TOOLS_PLATFORM.available}", + "\t${TOOLS_PLATFORM.cond1}", + "\t${TOOLS_PLATFORM.cond2}", + "\t${TOOLS_PLATFORM.undefined}", + ".endif", + "", + "do-build:", + "\t${TOOLS_PLATFORM.available}", + "\t${TOOLS_PLATFORM.cond1}", + "\t${TOOLS_PLATFORM.cond2}", + "\t${TOOLS_PLATFORM.undefined}", + "", + ".if defined(TOOLS_PLATFORM.undefined)", + "\t${TOOLS_PLATFORM.undefined}", + ".endif") + + mklines.Check() + + t.CheckOutputLines( + "WARN: filename.mk:15: TOOLS_PLATFORM.cond1 may be undefined on SunOS.", + "WARN: filename.mk:17: TOOLS_PLATFORM.undefined is undefined on SunOS.") +} + func (s *Suite) Test_MkVarUseChecker_checkBuildDefs(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/package.go b/pkgtools/pkglint/files/package.go index 370adf6345c..5014cffd7e8 100644 --- a/pkgtools/pkglint/files/package.go +++ b/pkgtools/pkglint/files/package.go @@ -135,7 +135,7 @@ func (pkg *Package) load() ([]CurrPath, *MkLines, *MkLines) { files = append(files, pkg.File(pkg.Pkgdir).ReadPaths()...) } files = append(files, pkg.File(pkg.Patchdir).ReadPaths()...) - if pkg.DistinfoFile != NewPackagePathString(pkg.vars.fallback["DISTINFO_FILE"]) { + if pkg.DistinfoFile != NewPackagePathString(pkg.vars.v("DISTINFO_FILE").fallback) { files = append(files, pkg.File(pkg.DistinfoFile)) } diff --git a/pkgtools/pkglint/files/pkglint.go b/pkgtools/pkglint/files/pkglint.go index a205945c84f..579e50498f2 100644 --- a/pkgtools/pkglint/files/pkglint.go +++ b/pkgtools/pkglint/files/pkglint.go @@ -384,13 +384,13 @@ func resolveVariableRefs(text string, mklines *MkLines, pkg *Package) string { if mklines != nil { // TODO: At load time, use mklines.loadVars instead. - if value, ok := mklines.allVars.LastValueFound(varname); ok { + if value, found, indeterminate := mklines.allVars.LastValueFound(varname); found && !indeterminate { return value } } if pkg != nil { - if value, ok := pkg.vars.LastValueFound(varname); ok { + if value, found, indeterminate := pkg.vars.LastValueFound(varname); found && !indeterminate { return value } } diff --git a/pkgtools/pkglint/files/pkgsrc.go b/pkgtools/pkglint/files/pkgsrc.go index 300d7d755ba..e176fe7f190 100644 --- a/pkgtools/pkglint/files/pkgsrc.go +++ b/pkgtools/pkglint/files/pkgsrc.go @@ -465,11 +465,84 @@ func (src *Pkgsrc) loadTools() { }) } + src.loadToolsPlatform() + if trace.Tracing { tools.Trace() } } +func (src *Pkgsrc) loadToolsPlatform() { + var systems []string + scopes := make(map[string]*RedundantScope) + for _, mkFile := range src.File("mk/tools").ReadPaths() { + m, opsys := match1(mkFile.Base(), `^tools\.(.+)\.mk$`) + if !m { + continue + } + systems = append(systems, opsys) + + mklines := LoadMk(mkFile, nil, MustSucceed) + scope := NewRedundantScope() + // Suppress any warnings, just compute the variable state. + scope.IsRelevant = func(*MkLine) bool { return false } + scope.Check(mklines) + scopes[opsys] = scope + } + + // 0 = undefined, 1 = conditional, 2 = definitely assigned + type status int + statusByNameAndOpsys := make(map[string]map[string]status) + + for opsys, scope := range scopes { + for varname, varinfo := range scope.vars { + if varnameCanon(varname) == "TOOLS_PLATFORM.*" { + var s status + if varinfo.vari.IsConditional() { + if len(varinfo.vari.WriteLocations()) == 1 { + s = 1 + } else { + // TODO: Don't just count the number of assignments, + // check whether they definitely assign the variable. + // See substScope. + s = 2 + } + } else if varinfo.vari.IsConstant() { + s = 2 + } else { + continue + } + + name := varnameParam(varname) + if statusByNameAndOpsys[name] == nil { + statusByNameAndOpsys[name] = make(map[string]status) + } + statusByNameAndOpsys[name][opsys] = s + } + } + } + + for name, tool := range src.Tools.byName { + undefined := make(map[string]bool) + conditional := make(map[string]bool) + for _, opsys := range systems { + undefined[opsys] = true + conditional[opsys] = true + } + for opsys, status := range statusByNameAndOpsys[name] { + switch status { + case 1: + delete(undefined, opsys) + case 2: + delete(undefined, opsys) + delete(conditional, opsys) + } + } + tool.undefinedOn = keysSorted(undefined) + tool.conditionalOn = keysSorted(conditional) + } +} + func (src *Pkgsrc) addBuildDefs(varnames ...string) { for _, varname := range varnames { src.buildDefs[varname] = true @@ -680,11 +753,16 @@ func (src *Pkgsrc) loadUntypedVars() { mklines := LoadMk(path, nil, MustSucceed) mklines.collectVariables() mklines.collectUsedVariables() - def := func(varname string, mkline *MkLine) { - define(varnameCanon(varname), mkline) - } - forEachStringMkLine(mklines.allVars.firstDef, def) - forEachStringMkLine(mklines.allVars.used, def) + mklines.allVars.forEach(func(varname string, data *scopeVar) { + if data.firstDef != nil { + define(varnameCanon(varname), data.firstDef) + } + }) + mklines.allVars.forEach(func(varname string, data *scopeVar) { + if data.used != nil { + define(varnameCanon(varname), data.used) + } + }) } handleFile := func(pathName string, info os.FileInfo, err error) error { diff --git a/pkgtools/pkglint/files/shell.go b/pkgtools/pkglint/files/shell.go index 41b305f2d8e..99c9f4449a0 100644 --- a/pkgtools/pkglint/files/shell.go +++ b/pkgtools/pkglint/files/shell.go @@ -168,7 +168,8 @@ func (scc *SimpleCommandChecker) handleCommandVariable() bool { varname := varuse.varname - if vartype := G.Pkgsrc.VariableType(scc.mklines, varname); vartype != nil && vartype.basicType.name == "ShellCommand" { + vartype := G.Pkgsrc.VariableType(scc.mklines, varname) + if vartype != nil && (vartype.basicType == BtShellCommand || vartype.basicType == BtPathname) { scc.checkInstallCommand(shellword) return true } diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go index 720505b1fbd..1664ed71b9c 100644 --- a/pkgtools/pkglint/files/shell_test.go +++ b/pkgtools/pkglint/files/shell_test.go @@ -45,6 +45,20 @@ func (s *Suite) Test_SimpleCommandChecker_checkCommandStart__unknown_default(c * "WARN: Makefile:8: UNKNOWN_TOOL is used but not defined.") } +// Despite its name, the TOOLS_PATH.* name the whole shell command, +// not just the path of its executable. +func (s *Suite) Test_SimpleCommandChecker_checkCommandStart__TOOLS_PATH(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package", + "CONFIG_SHELL=\t${TOOLS_PATH.bash}") + t.Chdir("category/package") + t.FinishSetUp() + G.checkdirPackage(".") + + t.CheckOutputEmpty() +} + func (s *Suite) Test_SimpleCommandChecker_checkInstallCommand(c *check.C) { t := s.Init(c) @@ -841,6 +855,7 @@ func (s *Suite) Test_ShellLineChecker_CheckShellCommandLine(c *check.C) { t := s.Init(c) t.SetUpVartypes() + t.SetUpTool("[", "TEST", AtRunTime) t.SetUpTool("awk", "AWK", AtRunTime) t.SetUpTool("cp", "CP", AtRunTime) t.SetUpTool("echo", "", AtRunTime) @@ -927,16 +942,13 @@ func (s *Suite) Test_ShellLineChecker_CheckShellCommandLine(c *check.C) { "WARN: filename.mk:1: The exitcode of \"${UNZIP_CMD}\" at the left of the | operator is ignored.") // From x11/wxGTK28/Makefile - // TODO: Why is TOOLS_PATH.msgfmt not recognized? - // At least, the warning should be more specific, mentioning USE_TOOLS. test(""+ "set -e; cd ${WRKSRC}/locale; "+ "for lang in *.po; do "+ " [ \"$${lang}\" = \"wxstd.po\" ] && continue; "+ " ${TOOLS_PATH.msgfmt} -c -o \"$${lang%.po}.mo\" \"$${lang}\"; "+ "done", - "WARN: filename.mk:1: Unknown shell command \"[\".", - "WARN: filename.mk:1: Unknown shell command \"${TOOLS_PATH.msgfmt}\".") + nil...) test("@cp from to", "WARN: filename.mk:1: The shell command \"cp\" should not be hidden.") diff --git a/pkgtools/pkglint/files/tools.go b/pkgtools/pkglint/files/tools.go index 7036716e47a..5ec09453f09 100644 --- a/pkgtools/pkglint/files/tools.go +++ b/pkgtools/pkglint/files/tools.go @@ -33,6 +33,15 @@ type Tool struct { MustUseVarForm bool Validity Validity Aliases []string + + // The operating systems on which the tool is defined conditionally, + // usually by enclosing the tool definition in an ".if exists". + // See mk/tools/tools.*.mk. + conditionalOn []string + + // The operating systems on which the tool is not defined at all. + // See mk/tools/tools.*.mk. + undefinedOn []string } func (tool *Tool) String() string { @@ -152,7 +161,7 @@ func (tr *Tools) Define(name, varname string, mkline *MkLine) *Tool { func (tr *Tools) def(name, varname string, mustUseVarForm bool, validity Validity, aliases []string) *Tool { assert(tr.IsValidToolName(name)) - fresh := Tool{name, varname, mustUseVarForm, validity, aliases} + fresh := Tool{name, varname, mustUseVarForm, validity, aliases, nil, nil} tool := tr.byName[name] if tool == nil { diff --git a/pkgtools/pkglint/files/tools_test.go b/pkgtools/pkglint/files/tools_test.go index 8383c0d5d28..6c454462596 100644 --- a/pkgtools/pkglint/files/tools_test.go +++ b/pkgtools/pkglint/files/tools_test.go @@ -5,15 +5,15 @@ import "gopkg.in/check.v1" func (s *Suite) Test_Tool_UsableAtLoadTime(c *check.C) { t := s.Init(c) - nowhere := Tool{"nowhere", "", false, Nowhere, nil} + nowhere := Tool{"nowhere", "", false, Nowhere, nil, nil, nil} t.CheckEquals(nowhere.UsableAtLoadTime(false), false) t.CheckEquals(nowhere.UsableAtLoadTime(true), false) - load := Tool{"load", "", false, AfterPrefsMk, nil} + load := Tool{"load", "", false, AfterPrefsMk, nil, nil, nil} t.CheckEquals(load.UsableAtLoadTime(false), false) t.CheckEquals(load.UsableAtLoadTime(true), true) - run := Tool{"run", "", false, AtRunTime, nil} + run := Tool{"run", "", false, AtRunTime, nil, nil, nil} t.CheckEquals(run.UsableAtLoadTime(false), false) t.CheckEquals(run.UsableAtLoadTime(true), false) } @@ -52,13 +52,13 @@ func (s *Suite) Test_Tool_UsableAtLoadTime__pkgconfig_builtin_mk(c *check.C) { func (s *Suite) Test_Tool_UsableAtRunTime(c *check.C) { t := s.Init(c) - nowhere := Tool{"nowhere", "", false, Nowhere, nil} + nowhere := Tool{"nowhere", "", false, Nowhere, nil, nil, nil} t.CheckEquals(nowhere.UsableAtRunTime(), false) - load := Tool{"load", "", false, AfterPrefsMk, nil} + load := Tool{"load", "", false, AfterPrefsMk, nil, nil, nil} t.CheckEquals(load.UsableAtRunTime(), true) - run := Tool{"run", "", false, AtRunTime, nil} + run := Tool{"run", "", false, AtRunTime, nil, nil, nil} t.CheckEquals(run.UsableAtRunTime(), true) } diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go index f76f358f69c..e12ad7964e9 100644 --- a/pkgtools/pkglint/files/util.go +++ b/pkgtools/pkglint/files/util.go @@ -210,12 +210,16 @@ func condInt(cond bool, trueValue, falseValue int) int { } func keysJoined(m map[string]bool) string { + return strings.Join(keysSorted(m), " ") +} + +func keysSorted(m map[string]bool) []string { var keys []string for key := range m { keys = append(keys, key) } sort.Strings(keys) - return strings.Join(keys, " ") + return keys } func copyStringMkLine(m map[string]*MkLine) map[string]*MkLine { @@ -617,22 +621,30 @@ func (o *Once) check(key uint64) bool { // // See also RedundantScope. type Scope struct { - firstDef map[string]*MkLine // TODO: Can this be removed? - lastDef map[string]*MkLine - value map[string]string - used map[string]*MkLine - usedAtLoadTime map[string]bool - fallback map[string]string + vs map[string]*scopeVar +} + +type scopeVar struct { + firstDef *MkLine + lastDef *MkLine + value string + used *MkLine + fallback string + usedAtLoadTime bool + indeterminate bool } func NewScope() Scope { - return Scope{ - make(map[string]*MkLine), - make(map[string]*MkLine), - make(map[string]string), - make(map[string]*MkLine), - make(map[string]bool), - make(map[string]string)} + return Scope{make(map[string]*scopeVar)} +} + +func (s *Scope) v(varname string) *scopeVar { + if v, found := s.vs[varname]; found { + return v + } + var sv scopeVar + s.vs[varname] = &sv + return &sv } // Define marks the variable and its canonicalized form as defined. @@ -645,8 +657,9 @@ func (s *Scope) Define(varname string, mkline *MkLine) { } func (s *Scope) def(name string, mkline *MkLine) { - if s.firstDef[name] == nil { - s.firstDef[name] = mkline + v := s.v(name) + if v.firstDef == nil { + v.firstDef = mkline if trace.Tracing { trace.Step2("Defining %q for the first time in %s", name, mkline.String()) } @@ -654,7 +667,7 @@ func (s *Scope) def(name string, mkline *MkLine) { trace.Step2("Defining %q in %s", name, mkline.String()) } - s.lastDef[name] = mkline + v.lastDef = mkline // In most cases the defining lines are indeed variable assignments. // Exceptions are comments from documentation sections, which still mark @@ -669,35 +682,37 @@ func (s *Scope) def(name string, mkline *MkLine) { value := mkline.Value() if trace.Tracing { trace.Stepf("Scope.Define.append %s: %s = %q + %q", - mkline.String(), name, s.value[name], value) + mkline.String(), name, v.value, value) } - s.value[name] += " " + value + v.value += " " + value case opAssignDefault: - if _, set := s.value[name]; !set { - s.value[name] = mkline.Value() + if v.value == "" && !v.indeterminate { + v.value = mkline.Value() } case opAssignShell: - delete(s.value, name) + v.value = "" + v.indeterminate = true default: - s.value[name] = mkline.Value() + v.value = mkline.Value() } } func (s *Scope) Fallback(varname string, value string) { - s.fallback[varname] = value + s.v(varname).fallback = value } // Use marks the variable and its canonicalized form as used. func (s *Scope) Use(varname string, line *MkLine, time VucTime) { use := func(name string) { - if s.used[name] == nil { - s.used[name] = line + v := s.v(name) + if v.used == nil { + v.used = line if trace.Tracing { trace.Step2("Using %q in %s", name, line.String()) } } if time == VucLoadTime { - s.usedAtLoadTime[name] = true + v.usedAtLoadTime = true } } @@ -710,7 +725,7 @@ func (s *Scope) Use(varname string, line *MkLine, time VucTime) { // - mentioned in a commented variable assignment, // - mentioned in a documentation comment. func (s *Scope) Mentioned(varname string) *MkLine { - return s.firstDef[varname] + return s.v(varname).firstDef } // IsDefined tests whether the variable is defined. @@ -719,7 +734,7 @@ func (s *Scope) Mentioned(varname string) *MkLine { // Even if IsDefined returns true, FirstDefinition doesn't necessarily return true // since the latter ignores the default definitions from vardefs.go, keyword dummyVardefMkline. func (s *Scope) IsDefined(varname string) bool { - mkline := s.firstDef[varname] + mkline := s.v(varname).firstDef return mkline != nil && mkline.IsVarassign() } @@ -745,21 +760,21 @@ func (s *Scope) IsDefinedSimilar(varname string) bool { // IsUsed tests whether the variable is used. // It does NOT test the canonicalized variable name. func (s *Scope) IsUsed(varname string) bool { - return s.used[varname] != nil + return s.v(varname).used != nil } // IsUsedSimilar tests whether the variable or its canonicalized form is used. func (s *Scope) IsUsedSimilar(varname string) bool { - if s.used[varname] != nil { + if s.v(varname).used != nil { return true } - return s.used[varnameCanon(varname)] != nil + return s.v(varnameCanon(varname)).used != nil } // IsUsedAtLoadTime returns true if the variable is used at load time // somewhere. func (s *Scope) IsUsedAtLoadTime(varname string) bool { - return s.usedAtLoadTime[varname] + return s.v(varname).usedAtLoadTime } // FirstDefinition returns the line in which the variable has been defined first. @@ -771,7 +786,7 @@ func (s *Scope) IsUsedAtLoadTime(varname string) bool { // round: the including file sets a value first, and the included file then // assigns a default value using ?=. func (s *Scope) FirstDefinition(varname string) *MkLine { - mkline := s.firstDef[varname] + mkline := s.v(varname).firstDef if mkline != nil && mkline.IsVarassign() { lastLine := s.LastDefinition(varname) if trace.Tracing && lastLine != mkline { @@ -792,7 +807,7 @@ func (s *Scope) FirstDefinition(varname string) *MkLine { // round: the including file sets a value first, and the included file then // assigns a default value using ?=. func (s *Scope) LastDefinition(varname string) *MkLine { - mkline := s.lastDef[varname] + mkline := s.v(varname).lastDef if mkline != nil && mkline.IsVarassign() { return mkline } @@ -804,10 +819,10 @@ func (s *Scope) LastDefinition(varname string) *MkLine { // mk/defaults/mk.conf for documentation. func (s *Scope) Commented(varname string) *MkLine { var mklines []*MkLine - if first := s.firstDef[varname]; first != nil { + if first := s.v(varname).firstDef; first != nil { mklines = append(mklines, first) } - if last := s.lastDef[varname]; last != nil { + if last := s.v(varname).lastDef; last != nil { mklines = append(mklines, last) } @@ -827,7 +842,7 @@ func (s *Scope) Commented(varname string) *MkLine { } func (s *Scope) FirstUse(varname string) *MkLine { - return s.used[varname] + return s.v(varname).used } // LastValue returns the value from the last variable definition. @@ -837,28 +852,50 @@ func (s *Scope) FirstUse(varname string) *MkLine { // was not found, or that the variable value cannot be determined // reliably. To distinguish these cases, call LastValueFound instead. func (s *Scope) LastValue(varname string) string { - value, _ := s.LastValueFound(varname) + value, _, _ := s.LastValueFound(varname) return value } -func (s *Scope) LastValueFound(varname string) (value string, found bool) { - value, found = s.value[varname] - if !found { - value, found = s.fallback[varname] +func (s *Scope) LastValueFound(varname string) (value string, found bool, indeterminate bool) { + v := s.vs[varname] + if v == nil { + return } - return + + value = v.value + found = v.firstDef != nil && v.firstDef.IsVarassign() + indeterminate = v.indeterminate + if found { + return + } + + return v.fallback, v.fallback != "", v.indeterminate } func (s *Scope) DefineAll(other Scope) { var varnames []string - for varname := range other.firstDef { + for varname := range other.vs { varnames = append(varnames, varname) } sort.Strings(varnames) for _, varname := range varnames { - s.Define(varname, other.firstDef[varname]) - s.Define(varname, other.lastDef[varname]) + v := other.vs[varname] + if v.firstDef != nil { + s.Define(varname, v.firstDef) + s.Define(varname, v.lastDef) + } + } +} + +func (s *Scope) forEach(action func(varname string, data *scopeVar)) { + var keys []string + for key := range s.vs { + keys = append(keys, key) + } + sort.Strings(keys) + for _, key := range keys { + action(key, s.vs[key]) } } @@ -1183,32 +1220,32 @@ func joinSkipEmpty(sep string, elements ...string) string { return strings.Join(nonempty, sep) } -func joinSkipEmptyCambridge(conn string, elements ...string) string { - var nonempty []string +// joinCambridge returns "first, second conn third". +// It is used when each element is a single word. +// Empty elements are ignored completely. +func joinCambridge(conn string, elements ...string) string { + parts := make([]string, 0, 2+2*len(elements)) for _, element := range elements { if element != "" { - nonempty = append(nonempty, element) + parts = append(parts, ", ", element) } } - var sb strings.Builder - for i, element := range nonempty { - if i > 0 { - if i == len(nonempty)-1 { - sb.WriteRune(' ') - sb.WriteString(conn) - sb.WriteRune(' ') - } else { - sb.WriteString(", ") - } - } - sb.WriteString(element) + if len(parts) == 0 { + return "" + } + if len(parts) < 4 { + return parts[1] } - return sb.String() + parts = append(parts[1:len(parts)-2], " ", conn, " ", parts[len(parts)-1]) + return strings.Join(parts, "") } -func joinSkipEmptyOxford(conn string, elements ...string) string { +// joinOxford returns "first, second, conn third". +// It is used when each element may consist of multiple words. +// Empty elements are ignored completely. +func joinOxford(conn string, elements ...string) string { var nonempty []string for _, element := range elements { if element != "" { diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go index e25a2685fde..b8f9358c027 100644 --- a/pkgtools/pkglint/files/util_test.go +++ b/pkgtools/pkglint/files/util_test.go @@ -543,9 +543,10 @@ func (s *Suite) Test_Scope__commented_varassign(c *check.C) { t.CheckEquals(scope.Mentioned("VAR"), mkline) t.CheckEquals(scope.Commented("VAR"), mkline) - value, found := scope.LastValueFound("VAR") + value, found, indeterminate := scope.LastValueFound("VAR") t.CheckEquals(value, "") t.CheckEquals(found, false) + t.CheckEquals(indeterminate, false) } func (s *Suite) Test_Scope_Define(c *check.C) { @@ -553,39 +554,40 @@ func (s *Suite) Test_Scope_Define(c *check.C) { scope := NewScope() - test := func(line string, ok bool, value string) { + test := func(line string, found, indeterminate bool, value string) { mkline := t.NewMkLine("file.mk", 123, line) scope.Define("BUILD_DIRS", mkline) - actualValue, actualFound := scope.LastValueFound("BUILD_DIRS") + actualValue, actualFound, actualIndeterminate := scope.LastValueFound("BUILD_DIRS") - t.CheckEquals(actualValue, value) - t.CheckEquals(actualFound, ok) - t.CheckEquals(scope.value["BUILD_DIRS"], value) + t.CheckDeepEquals( + []interface{}{actualFound, actualIndeterminate, actualValue}, + []interface{}{found, indeterminate, value}) + t.CheckEquals(scope.vs["BUILD_DIRS"].value, value) } test("BUILD_DIRS?=\tdefault", - true, "default") + true, false, "default") test( "BUILD_DIRS=\tone two three", - true, "one two three") + true, false, "one two three") test( "BUILD_DIRS+=\tfour", - true, "one two three four") + true, false, "one two three four") // Later default assignments do not have an effect. test("BUILD_DIRS?=\tdefault", - true, "one two three four") + true, false, "one two three four") test("BUILD_DIRS!=\techo dynamic", - false, "") + true, true, "") - // FIXME: This is not correct. The shell assignment sets the variable, - // after which all further default assignments are ignored. + // The shell assignment above sets the variable to an indeterminate + // value, after which all further default assignments are ignored. test("BUILD_DIRS?=\tdefault after shell assignment", - true, "default after shell assignment") + true, true, "") } func (s *Suite) Test_Scope_Mentioned(c *check.C) { @@ -727,9 +729,7 @@ func (s *Suite) Test_Scope_DefineAll(c *check.C) { dst := NewScope() dst.DefineAll(src) - c.Check(dst.firstDef, check.HasLen, 0) - c.Check(dst.lastDef, check.HasLen, 0) - c.Check(dst.used, check.HasLen, 0) + c.Check(dst.vs, check.HasLen, 0) src.Define("VAR", t.NewMkLine("file.mk", 1, "VAR=value")) dst.DefineAll(src) @@ -1020,23 +1020,23 @@ func (s *Suite) Test_joinSkipEmpty(c *check.C) { "one, two, three") } -func (s *Suite) Test_joinSkipEmptyCambridge(c *check.C) { +func (s *Suite) Test_joinCambridge(c *check.C) { t := s.Init(c) t.CheckDeepEquals( - joinSkipEmptyCambridge("and", "", "one", "", "", "two", "", "three"), + joinCambridge("and", "", "one", "", "", "two", "", "three"), "one, two and three") t.CheckDeepEquals( - joinSkipEmptyCambridge("and", "", "one", "", ""), + joinCambridge("and", "", "one", "", ""), "one") } -func (s *Suite) Test_joinSkipEmptyOxford(c *check.C) { +func (s *Suite) Test_joinOxford(c *check.C) { t := s.Init(c) t.CheckDeepEquals( - joinSkipEmptyOxford("and", "", "one", "", "", "two", "", "three"), + joinOxford("and", "", "one", "", "", "two", "", "three"), "one, two, and three") } diff --git a/pkgtools/pkglint/files/varalignblock.go b/pkgtools/pkglint/files/varalignblock.go index 34f443e14fc..c891072be28 100644 --- a/pkgtools/pkglint/files/varalignblock.go +++ b/pkgtools/pkglint/files/varalignblock.go @@ -562,6 +562,7 @@ func (info *varalignLine) alignValueSingle(newWidth int) { } fix.ReplaceAt(info.rawIndex, info.spaceBeforeValueIndex(), oldSpace, newSpace) fix.Apply() + info.spaceBeforeValue = newSpace } func (info *varalignLine) alignValueInitial(newWidth int) { @@ -855,10 +856,7 @@ func (p *varalignParts) isCanonicalFollow() bool { } func (p *varalignParts) isTooLongFor(valueColumn int) bool { - // FIXME: As of 2020-01-27, this method is called from - // Test_VaralignBlock__right_margin with valueColumn == 0, - // which doesn't make sense. It should at least be 8. - column := tabWidthAppend(valueColumn, p.value) + column := tabWidthAppend(imax(valueColumn, 8), p.value) if p.isContinuation() { column += 2 } diff --git a/pkgtools/pkglint/files/varalignblock_test.go b/pkgtools/pkglint/files/varalignblock_test.go index adaff884274..d4d15e6d6cf 100644 --- a/pkgtools/pkglint/files/varalignblock_test.go +++ b/pkgtools/pkglint/files/varalignblock_test.go @@ -2073,7 +2073,7 @@ func (s *Suite) Test_VaralignBlock__right_margin(c *check.C) { "\t............................................................70 \t\t\\", "\t........................................................66") vt.Diagnostics( - "NOTE: Makefile:2: The continuation backslash should be in column 73, not 81.", + "NOTE: Makefile:2: The continuation backslash should be preceded by a single space.", "NOTE: Makefile:3: The continuation backslash should be in column 73, not 81.") vt.Autofixes( "AUTOFIX: Makefile:2: Replacing \"\\t\" with \" \".", @@ -3585,11 +3585,7 @@ func (s *Suite) Test_varalignLine_alignValueSingle(c *check.C) { t.CheckEqualsf( mkline.RawText(0), after, "Line.raw.text, autofix=%v", autofix) - - // As of 2019-12-11, the info fields are not updated - // accordingly, but they should. - // FIXME: update info accordingly - t.CheckEqualsf(info.String(), before, + t.CheckEqualsf(info.String(), after, "info.String, autofix=%v", autofix) } diff --git a/pkgtools/pkglint/files/vardefs.go b/pkgtools/pkglint/files/vardefs.go index f0a696227b9..445fde5039f 100644 --- a/pkgtools/pkglint/files/vardefs.go +++ b/pkgtools/pkglint/files/vardefs.go @@ -421,7 +421,7 @@ func (reg *VarTypeRegistry) enumFrom( G.Logger.TechFatalf( mklines.lines.Filename, "Must contain at least 1 variable definition for %s.", - joinSkipEmptyCambridge("or", varcanons...)) + joinCambridge("or", varcanons...)) } if trace.Tracing { @@ -1067,7 +1067,8 @@ func (reg *VarTypeRegistry) Init(src *Pkgsrc) { reg.pkg("CONFIGURE_SCRIPT", BtPathname) reg.pkglist("CONFIG_GUESS_OVERRIDE", BtPathPattern) reg.pkglist("CONFIG_STATUS_OVERRIDE", BtPathPattern) - reg.pkg("CONFIG_SHELL", BtPathname) + reg.pkg("CONFIG_SHELL", BtShellCommand) + reg.cmdline("CONFIG_SHELL_FLAGS", BtShellWord, List) reg.pkglist("CONFIG_SUB_OVERRIDE", BtPathPattern) reg.pkglist("CONFLICTS", BtDependencyPattern) reg.pkgappend("CONF_FILES", BtConfFiles) diff --git a/pkgtools/pkglint/files/vartype.go b/pkgtools/pkglint/files/vartype.go index 51fda5eef32..deab6ea8bb3 100644 --- a/pkgtools/pkglint/files/vartype.go +++ b/pkgtools/pkglint/files/vartype.go @@ -182,7 +182,7 @@ func (perms ACLPermissions) String() string { } func (perms ACLPermissions) HumanString() string { - return joinSkipEmptyOxford("or", + return joinOxford("or", condStr(perms.Contains(aclpSet), "set", ""), condStr(perms.Contains(aclpSetDefault), "given a default value", ""), condStr(perms.Contains(aclpAppend), "appended to", ""), @@ -265,12 +265,12 @@ func (vt *Vartype) AlternativeFiles(perms ACLPermissions) string { neg = merge(neg) } - positive := joinSkipEmptyCambridge("or", pos...) + positive := joinCambridge("or", pos...) if positive == "" { return "" } - negative := joinSkipEmptyCambridge("or", neg...) + negative := joinCambridge("or", neg...) if negative == "" { return positive } |