diff options
author | rillig <rillig@pkgsrc.org> | 2020-03-15 11:31:24 +0000 |
---|---|---|
committer | rillig <rillig@pkgsrc.org> | 2020-03-15 11:31:24 +0000 |
commit | 7a142dc72b4b3277065ddf26dfbf13da18b3a0f3 (patch) | |
tree | da1a84a7e89f87c3e1b69989a0bf82f43269030a /pkgtools | |
parent | 39884536caff54508398e7f276e66e882bfa9142 (diff) | |
download | pkgsrc-7a142dc72b4b3277065ddf26dfbf13da18b3a0f3.tar.gz |
pkgtools/pkglint: update to 19.4.11
Changes since 19.4.10:
The use of PKG_OPTIONS and PKG_BUILD_OPTIONS in buildlink3.mk and other
files is checked for common mistakes.
Checking the indentation of a continuation line no longer crashes in edge
cases.
Diffstat (limited to 'pkgtools')
-rw-r--r-- | pkgtools/pkglint/Makefile | 4 | ||||
-rw-r--r-- | pkgtools/pkglint/files/buildlink3.go | 34 | ||||
-rw-r--r-- | pkgtools/pkglint/files/buildlink3_test.go | 54 | ||||
-rw-r--r-- | pkgtools/pkglint/files/check_test.go | 26 | ||||
-rw-r--r-- | pkgtools/pkglint/files/mkcondchecker.go | 15 | ||||
-rw-r--r-- | pkgtools/pkglint/files/mkcondchecker_test.go | 52 | ||||
-rw-r--r-- | pkgtools/pkglint/files/mkvarusechecker.go | 24 | ||||
-rw-r--r-- | pkgtools/pkglint/files/mkvarusechecker_test.go | 39 | ||||
-rwxr-xr-x | pkgtools/pkglint/files/options.go | 18 | ||||
-rwxr-xr-x | pkgtools/pkglint/files/options_test.go | 24 | ||||
-rw-r--r-- | pkgtools/pkglint/files/package.go | 38 | ||||
-rw-r--r-- | pkgtools/pkglint/files/package_test.go | 40 | ||||
-rw-r--r-- | pkgtools/pkglint/files/pkglint.go | 12 | ||||
-rw-r--r-- | pkgtools/pkglint/files/util_test.go | 6 | ||||
-rw-r--r-- | pkgtools/pkglint/files/varalignblock.go | 5 | ||||
-rw-r--r-- | pkgtools/pkglint/files/varalignblock_test.go | 24 |
16 files changed, 357 insertions, 58 deletions
diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index 965a4cf21a8..a35746cd92f 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.633 2020/03/07 23:35:35 rillig Exp $ +# $NetBSD: Makefile,v 1.634 2020/03/15 11:31:24 rillig Exp $ -PKGNAME= pkglint-19.4.10 +PKGNAME= pkglint-19.4.11 CATEGORIES= pkgtools DISTNAME= tools MASTER_SITES= ${MASTER_SITE_GITHUB:=golang/} diff --git a/pkgtools/pkglint/files/buildlink3.go b/pkgtools/pkglint/files/buildlink3.go index 124dddcb35d..910313da1c0 100644 --- a/pkgtools/pkglint/files/buildlink3.go +++ b/pkgtools/pkglint/files/buildlink3.go @@ -13,8 +13,8 @@ type Buildlink3Checker struct { abi, api *DependencyPattern } -func CheckLinesBuildlink3Mk(mklines *MkLines) { - (&Buildlink3Checker{mklines: mklines}).Check() +func NewBuildlink3Checker(mklines *MkLines) *Buildlink3Checker { + return &Buildlink3Checker{mklines: mklines} } func (ck *Buildlink3Checker) Check() { @@ -182,6 +182,10 @@ func (ck *Buildlink3Checker) checkMainPart(mlex *MkLinesLexer) bool { case mkline.IsDirective() && mkline.Directive() == "endif": indentLevel-- } + + mkline.ForEachUsed(func(varUse *MkVarUse, time VucTime) { + ck.checkVarUse(varUse, mkline) + }) } if indentLevel > 0 { @@ -195,6 +199,32 @@ func (ck *Buildlink3Checker) checkMainPart(mlex *MkLinesLexer) bool { return true } +func (ck *Buildlink3Checker) checkVarUse(varUse *MkVarUse, mkline *MkLine) { + varname := varUse.varname + if varname == "PKG_OPTIONS" { + mkline.Errorf("PKG_OPTIONS is not available in buildlink3.mk files.") + mkline.Explain( + "The buildlink3.mk file of a package is only ever included", + "by other packages, never by the package itself.", + "Therefore it does not make sense to use the variable PKG_OPTIONS", + "in this place since it contains the package options of a random", + "package that happens to include this file.", + "", + "To access the options of this package, see mk/pkg-build-options.mk.") + } + + if varnameBase(varname) == "PKG_BUILD_OPTIONS" { + param := varnameParam(varname) + if param != "" && param != ck.pkgbase { + mkline.Warnf("Wrong PKG_BUILD_OPTIONS, expected %q instead of %q.", + ck.pkgbase, param) + mkline.Explain( + "The variable parameter for PKG_BUILD_OPTIONS must correspond", + "to the value of \"pkgbase\" above.") + } + } +} + func (ck *Buildlink3Checker) checkVarassign(mkline *MkLine, pkgbase string) { varname, value := mkline.Varname(), mkline.Value() doCheck := false diff --git a/pkgtools/pkglint/files/buildlink3_test.go b/pkgtools/pkglint/files/buildlink3_test.go index 76103062383..9f2599b73e2 100644 --- a/pkgtools/pkglint/files/buildlink3_test.go +++ b/pkgtools/pkglint/files/buildlink3_test.go @@ -2,6 +2,10 @@ package pkglint import "gopkg.in/check.v1" +func CheckLinesBuildlink3Mk(mklines *MkLines) { + NewBuildlink3Checker(mklines).Check() +} + // This test ensures that CheckLinesBuildlink3Mk really checks for // buildlink3.mk files that are included by the buildlink3.mk file // but not by the package. @@ -473,6 +477,25 @@ func (s *Suite) Test_CheckLinesBuildlink3Mk__invalid_dependency_patterns(c *chec "WARN: buildlink3.mk:10: Invalid dependency pattern \"hs-X11!=1.6.1.2nb2\".") } +func (s *Suite) Test_CheckLinesBuildlink3Mk__PKG_OPTIONS(c *check.C) { + t := s.Init(c) + + t.SetUpOption("option", "") + t.SetUpPkgsrc() + t.SetUpPackage("category/package") + t.CreateFileBuildlink3("category/package/buildlink3.mk", + ".include \"../../mk/bsd.fast.prefs.mk\"", + ".if ${PKG_OPTIONS:Moption}", + ".endif") + t.FinishSetUp() + + G.Check(t.File("category/package/buildlink3.mk")) + + t.CheckOutputLines( + "ERROR: ~/category/package/buildlink3.mk:13: " + + "PKG_OPTIONS is not available in buildlink3.mk files.") +} + // Just for branch coverage. func (s *Suite) Test_Buildlink3Checker_Check__no_tracing(c *check.C) { t := s.Init(c) @@ -661,6 +684,37 @@ func (s *Suite) Test_Buildlink3Checker_checkMainPart__comment_at_end_of_file(c * "WARN: ~/category/package/buildlink3.mk:14: The file should end here.") } +func (s *Suite) Test_Buildlink3Checker_checkVarUse__PKG_BUILD_OPTIONS(c *check.C) { + t := s.Init(c) + + t.SetUpOption("option", "") + t.SetUpPkgsrc() + t.CreateFileLines("mk/pkg-build-options.mk") + t.SetUpPackage("category/package") + t.CreateFileBuildlink3("category/package/buildlink3.mk", + "pkgbase := unrelated", + ".include \"../../mk/pkg-build-options.mk\"", + "", + ".if ${PKG_BUILD_OPTIONS:Moption}", // missing variable parameter + ".endif", + "", + ".if ${PKG_BUILD_OPTIONS.package:Moption}", // wrong variable parameter + ".endif", + "", + ".if ${PKG_BUILD_OPTIONS.unrelated:Moption}", // corresponds to pkgbase above + ".endif") + t.FinishSetUp() + + G.Check(t.File("category/package/buildlink3.mk")) + + // TODO: Warn about PKG_BUILD_OPTIONS.package since that variable is not defined. + t.CheckOutputLines( + "WARN: ~/category/package/buildlink3.mk:15: "+ + "PKG_BUILD_OPTIONS is used but not defined.", + "WARN: ~/category/package/buildlink3.mk:21: "+ + "Wrong PKG_BUILD_OPTIONS, expected \"package\" instead of \"unrelated\".") +} + // As of October 2018, pkglint parses package dependencies a little // different than the pkg_* tools. // In all but two cases this works, this is one of the exceptions. diff --git a/pkgtools/pkglint/files/check_test.go b/pkgtools/pkglint/files/check_test.go index be89c8e5bbe..6d66e754a51 100644 --- a/pkgtools/pkglint/files/check_test.go +++ b/pkgtools/pkglint/files/check_test.go @@ -581,15 +581,19 @@ func (t *Tester) CreateFileDummyPatch(filename RelPath) { } func (t *Tester) CreateFileBuildlink3(filename RelPath, customLines ...string) { + lower := filename.Dir().Base() + t.CreateFileBuildlink3Id(filename, lower, customLines...) +} + +func (t *Tester) CreateFileBuildlink3Id(filename RelPath, id string, customLines ...string) { // Buildlink3.mk files only make sense in category/package directories. assert(G.Pkgsrc.Rel(t.File(filename)).Count() == 3) dir := filename.Dir().Clean() - lower := dir.Base() // see pkgtools/createbuildlink/files/createbuildlink, "package specific variables" - upper := strings.Replace(strings.ToUpper(lower), "-", "_", -1) + upperID := strings.Replace(strings.ToUpper(id), "-", "_", -1) - width := tabWidthSlice("BUILDLINK_API_DEPENDS.", lower, "+=\t") + width := tabWidthSlice("BUILDLINK_API_DEPENDS.", id, "+=\t") aligned := func(format string, args ...interface{}) string { msg := sprintf(format, args...) @@ -603,21 +607,21 @@ func (t *Tester) CreateFileBuildlink3(filename RelPath, customLines ...string) { lines = append(lines, MkCvsID, "", - sprintf("BUILDLINK_TREE+=\t%s", lower), + sprintf("BUILDLINK_TREE+=\t%s", id), "", - sprintf(".if !defined(%s_BUILDLINK3_MK)", upper), - sprintf("%s_BUILDLINK3_MK:=", upper), + sprintf(".if !defined(%s_BUILDLINK3_MK)", upperID), + sprintf("%s_BUILDLINK3_MK:=", upperID), "", - aligned("BUILDLINK_API_DEPENDS.%s+=", lower)+sprintf("%s>=0", lower), - aligned("BUILDLINK_PKGSRCDIR.%s?=", lower)+sprintf("../../%s", dir), - aligned("BUILDLINK_DEPMETHOD.%s?=", lower)+"build", + aligned("BUILDLINK_API_DEPENDS.%s+=", id)+sprintf("%s>=0", id), + aligned("BUILDLINK_PKGSRCDIR.%s?=", id)+sprintf("../../%s", dir), + aligned("BUILDLINK_DEPMETHOD.%s?=", id)+"build", "") lines = append(lines, customLines...) lines = append(lines, "", - sprintf(".endif # %s_BUILDLINK3_MK", upper), + sprintf(".endif # %s_BUILDLINK3_MK", upperID), "", - sprintf("BUILDLINK_TREE+=\t-%s", lower)) + sprintf("BUILDLINK_TREE+=\t-%s", id)) t.CreateFileLines(filename, lines...) } diff --git a/pkgtools/pkglint/files/mkcondchecker.go b/pkgtools/pkglint/files/mkcondchecker.go index ebe0915d5dd..7617b04fa26 100644 --- a/pkgtools/pkglint/files/mkcondchecker.go +++ b/pkgtools/pkglint/files/mkcondchecker.go @@ -82,12 +82,21 @@ func (ck *MkCondChecker) checkNotEmpty(not *MkCond) { // This applies especially to the ${VAR:Mpattern} form. // // See MkCondChecker.simplify. - if !G.Experimental { + if !hasPrefix(not.Empty.varname, "PKG_BUILD_OPTIONS.") { return } - ck.MkLine.Notef("!empty(%s%s) can be replaced with the simpler %s.", - not.Empty.varname, not.Empty.Mod(), not.Empty.String()) + fix := ck.MkLine.Autofix() + from := sprintf("!empty(%s%s)", not.Empty.varname, not.Empty.Mod()) + to := not.Empty.String() + fix.Notef("%s can be replaced with %s.", from, to) + fix.Explain( + "Besides being simpler to read, the expression will also fail", + "quickly with a \"Malformed conditional\" error from bmake", + "if it should ever be undefined at this point.", + "This catches typos and other programming mistakes.") + fix.Replace(from, to) + fix.Apply() } // checkEmpty checks a condition of the form empty(VAR), diff --git a/pkgtools/pkglint/files/mkcondchecker_test.go b/pkgtools/pkglint/files/mkcondchecker_test.go index ce4c7fce9d0..b6ec7a31437 100644 --- a/pkgtools/pkglint/files/mkcondchecker_test.go +++ b/pkgtools/pkglint/files/mkcondchecker_test.go @@ -219,7 +219,7 @@ func (s *Suite) Test_MkCondChecker_Check__comparing_PKGSRC_COMPILER_with_eqeq(c func (s *Suite) Test_MkCondChecker_checkNotEmpty(c *check.C) { t := s.Init(c) - G.Experimental = true + t.SetUpVartypes() test := func(cond string, diagnostics ...string) { mklines := t.NewMkLines("filename.mk", @@ -233,8 +233,17 @@ func (s *Suite) Test_MkCondChecker_checkNotEmpty(c *check.C) { } test("!empty(VAR)", - // TODO: Add a :U modifier if VAR might be undefined. - "NOTE: filename.mk:1: !empty(VAR) can be replaced with the simpler ${VAR}.") + + // Only a few variables are suggested to use the simpler form, + // because of the side-effect when the variable is undefined. + // VAR is not one of these variables. + nil...) + + test( + "!empty(PKG_BUILD_OPTIONS.package:Moption)", + + "NOTE: filename.mk:1: !empty(PKG_BUILD_OPTIONS.package:Moption) "+ + "can be replaced with ${PKG_BUILD_OPTIONS.package:Moption}.") } func (s *Suite) Test_MkCondChecker_checkEmpty(c *check.C) { @@ -395,13 +404,14 @@ func (s *Suite) Test_MkCondChecker_simplify(c *check.C) { t.Chdir("category/package") // The Anything type suppresses the warnings from type checking. - // BtUnknown would not work, see Pkgsrc.VariableType. + // BtUnknown would not work here, see Pkgsrc.VariableType. btAnything := &BasicType{"Anything", func(cv *VartypeCheck) {}} // For simplifying the expressions, it is necessary to know whether // a variable can be undefined. Undefined variables need the - // :U modifier, otherwise bmake will complain about "malformed - // conditions", which is not entirely precise since the expression + // :U modifier or must be enclosed in double quotes, otherwise + // bmake will complain about a "Malformed conditional". That error + // message is not entirely precise since the expression // is syntactically valid, it's just the evaluation that fails. // // Some variables such as MACHINE_ARCH are in scope from the very @@ -474,6 +484,7 @@ func (s *Suite) Test_MkCondChecker_simplify(c *check.C) { doTest(true, before, after, diagnostics...) } testBeforeAndAfterPrefs := func(before, after string, diagnostics ...string) { + doTest(false, before, after, diagnostics...) doTest(true, before, after, diagnostics...) } @@ -919,13 +930,24 @@ func (s *Suite) Test_MkCondChecker_simplify(c *check.C) { // replaced automatically, see mkCondLiteralChars. // TODO: Add tests for all characters that are special in string literals or patterns. // TODO: Then, extend the set of characters for which the expressions are simplified. - testBeforeAndAfterPrefs( + testBeforePrefs( + ".if ${PREFS_DEFINED:M&&}", + ".if ${PREFS_DEFINED:M&&}", + + "WARN: filename.mk:3: To use PREFS_DEFINED at load time, .include \"../../mk/bsd.prefs.mk\" first.") + testAfterPrefs( ".if ${PREFS_DEFINED:M&&}", ".if ${PREFS_DEFINED:M&&}", nil...) - testBeforeAndAfterPrefs( + testBeforePrefs( + ".if ${PREFS:M&&}", + ".if ${PREFS:M&&}", + + // TODO: Warn that the :U is missing. + "WARN: filename.mk:3: To use PREFS at load time, .include \"../../mk/bsd.prefs.mk\" first.") + testAfterPrefs( ".if ${PREFS:M&&}", ".if ${PREFS:M&&}", @@ -947,7 +969,19 @@ func (s *Suite) Test_MkCondChecker_simplify(c *check.C) { // The characters <=> may be used unescaped in :M and :N patterns // but not in .if conditions. There they must be enclosed in quotes. - testBeforeAndAfterPrefs( + testBeforePrefs( + ".if ${PREFS_DEFINED:M<=>}", + ".if ${PREFS_DEFINED:U} == \"<=>\"", + + "NOTE: filename.mk:3: PREFS_DEFINED can be "+ + "compared using the simpler \"${PREFS_DEFINED:U} == \"<=>\"\" "+ + "instead of matching against \":M<=>\".", + "WARN: filename.mk:3: To use PREFS_DEFINED at load time, "+ + ".include \"../../mk/bsd.prefs.mk\" first.", + "AUTOFIX: filename.mk:3: "+ + "Replacing \"${PREFS_DEFINED:M<=>}\" "+ + "with \"${PREFS_DEFINED:U} == \\\"<=>\\\"\".") + testAfterPrefs( ".if ${PREFS_DEFINED:M<=>}", ".if ${PREFS_DEFINED} == \"<=>\"", diff --git a/pkgtools/pkglint/files/mkvarusechecker.go b/pkgtools/pkglint/files/mkvarusechecker.go index 01964291922..d76376b53c6 100644 --- a/pkgtools/pkglint/files/mkvarusechecker.go +++ b/pkgtools/pkglint/files/mkvarusechecker.go @@ -33,6 +33,7 @@ func (ck *MkVarUseChecker) Check(vuc *VarUseContext) { ck.checkToolsPlatform() ck.checkBuildDefs() ck.checkDeprecated() + ck.checkPkgBuildOptions() NewMkLineChecker(ck.MkLines, ck.MkLine). checkTextVarUse(ck.use.varname, ck.vartype, vuc.time) @@ -738,3 +739,26 @@ func (ck *MkVarUseChecker) checkDeprecated() { ck.MkLine.Warnf("Use of %q is deprecated. %s", varname, instead) } + +func (ck *MkVarUseChecker) checkPkgBuildOptions() { + pkg := ck.MkLines.pkg + if pkg == nil { + return + } + varname := ck.use.varname + if !hasPrefix(varname, "PKG_BUILD_OPTIONS.") { + return + } + param := varnameParam(varname) + if pkg.seenPkgbase.Seen(param) { + return + } + + ck.MkLine.Warnf("The PKG_BUILD_OPTIONS for %q are not available to this package.", + param) + ck.MkLine.Explain( + "The variable parameter for PKG_BUILD_OPTIONS must correspond", + "to the value of \"pkgbase\" above.", + "", + "For more information, see mk/pkg-build-options.mk.") +} diff --git a/pkgtools/pkglint/files/mkvarusechecker_test.go b/pkgtools/pkglint/files/mkvarusechecker_test.go index c2638fd11f0..46f3458d294 100644 --- a/pkgtools/pkglint/files/mkvarusechecker_test.go +++ b/pkgtools/pkglint/files/mkvarusechecker_test.go @@ -1325,3 +1325,42 @@ func (s *Suite) Test_MkVarUseChecker_checkDeprecated(c *check.C) { "WARN: filename.mk:3: Use of \"USE_CROSSBASE\" is deprecated. "+ "Has been removed.") } + +// This test demonstrates some typos that an inexperienced pkgsrc developer +// might make. This scenario is not intended to be realistic. +func (s *Suite) Test_MkVarUseChecker_checkPkgBuildOptions(c *check.C) { + t := s.Init(c) + + t.SetUpOption("option", "") + t.SetUpPackage("category/package", + ".include \"../../category/lib/buildlink3.mk\"") + t.SetUpPackage("category/lib") + t.CreateFileLines("mk/pkg-build-options.mk") + t.CreateFileBuildlink3("category/package/buildlink3.mk", + ".include \"../../mk/bsd.fast.prefs.mk\"", + "", + ".if ${PKG_BUILD_OPTIONS.lib:Moption}", // Too early + ".endif", + "", + ".if ${PKG_BUILD_OPTIONS.unrelated:Moption}", + ".include \"../../category/lib/buildlink3.mk\"", + ".endif", + "", + ".if ${PKG_BUILD_OPTIONS.lib:Moption}", // Only defined conditionally + ".endif") + t.CreateFileBuildlink3("category/lib/buildlink3.mk", + "pkgbase := lib", + ".include \"../../mk/pkg-build-options.mk\"") + t.Chdir("category/package") + t.FinishSetUp() + + G.Check(".") + + t.CheckOutputLines( + "WARN: Makefile:20: \"../../category/lib/buildlink3.mk\" is included unconditionally here "+ + "and conditionally in buildlink3.mk:18 (depending on PKG_BUILD_OPTIONS.unrelated).", + "WARN: buildlink3.mk:17: The PKG_BUILD_OPTIONS for \"unrelated\" are not available to this package.", + "WARN: buildlink3.mk:14: Wrong PKG_BUILD_OPTIONS, expected \"package\" instead of \"lib\".", + "WARN: buildlink3.mk:17: Wrong PKG_BUILD_OPTIONS, expected \"package\" instead of \"unrelated\".", + "WARN: buildlink3.mk:21: Wrong PKG_BUILD_OPTIONS, expected \"package\" instead of \"lib\".") +} diff --git a/pkgtools/pkglint/files/options.go b/pkgtools/pkglint/files/options.go index e08af3c9d9c..e9a84c8d978 100755 --- a/pkgtools/pkglint/files/options.go +++ b/pkgtools/pkglint/files/options.go @@ -1,8 +1,9 @@ package pkglint -func CheckLinesOptionsMk(mklines *MkLines) { +func CheckLinesOptionsMk(mklines *MkLines, buildlinkID string) { ck := OptionsLinesChecker{ mklines, + buildlinkID, false, make(map[string]*MkLine), false, @@ -16,7 +17,8 @@ func CheckLinesOptionsMk(mklines *MkLines) { // // See mk/bsd.options.mk for a detailed description. type OptionsLinesChecker struct { - mklines *MkLines + mklines *MkLines + buildlinkID string declaredArbitrary bool declaredOptions map[string]*MkLine @@ -49,6 +51,18 @@ func (ck *OptionsLinesChecker) collect() { if !seenInclude { if !seenPkgOptionsVar && mkline.IsVarassign() && mkline.Varname() == "PKG_OPTIONS_VAR" { seenPkgOptionsVar = true + buildlinkID := ck.buildlinkID + optionsID := varnameParam(mkline.Value()) + if buildlinkID != "" && optionsID != "" && buildlinkID != optionsID { + mkline.Warnf("The buildlink3 identifier %q should be the same as the options identifier %q.", + buildlinkID, optionsID) + mkline.Explain( + "Having different identifiers refer to the same package", + "is confusing for the pkgsrc user.", + "The pkgsrc infrastructure doesn't care though", + "if the identifiers in PKG_OPTIONS.* and PKG_BUILD_OPTIONS.*", + "are the same or not.") + } } seenInclude = mkline.IsInclude() && mkline.IncludedFile() == "../../mk/bsd.options.mk" } diff --git a/pkgtools/pkglint/files/options_test.go b/pkgtools/pkglint/files/options_test.go index bd3fd304b46..aae9e26ab61 100755 --- a/pkgtools/pkglint/files/options_test.go +++ b/pkgtools/pkglint/files/options_test.go @@ -88,7 +88,7 @@ func (s *Suite) Test_CheckLinesOptionsMk__literal(c *check.C) { t.Chdir("category/package") t.FinishSetUp() - CheckLinesOptionsMk(mklines) + CheckLinesOptionsMk(mklines, "") t.CheckOutputLines( "WARN: ~/category/package/options.mk:4: "+ @@ -124,7 +124,7 @@ func (s *Suite) Test_CheckLinesOptionsMk__literal_in_for_loop(c *check.C) { t.Chdir("category/package") t.FinishSetUp() - CheckLinesOptionsMk(mklines) + CheckLinesOptionsMk(mklines, "") t.CheckOutputLines( "WARN: ~/category/package/options.mk:5: "+ @@ -158,7 +158,7 @@ func (s *Suite) Test_CheckLinesOptionsMk__prefs(c *check.C) { t.Chdir("category/package") t.FinishSetUp() - CheckLinesOptionsMk(mklines) + CheckLinesOptionsMk(mklines, "") t.CheckOutputEmpty() } @@ -184,7 +184,7 @@ func (s *Suite) Test_CheckLinesOptionsMk__variable_order(c *check.C) { t.Chdir("category/package") t.FinishSetUp() - CheckLinesOptionsMk(mklines) + CheckLinesOptionsMk(mklines, "") t.CheckOutputLines( "WARN: ~/category/package/options.mk:3: " + @@ -201,7 +201,7 @@ func (s *Suite) Test_CheckLinesOptionsMk__empty(c *check.C) { t.Chdir("category/package") t.FinishSetUp() - CheckLinesOptionsMk(mklines) + CheckLinesOptionsMk(mklines, "") t.CheckOutputLines( "ERROR: ~/category/package/options.mk: "+ @@ -240,7 +240,7 @@ func (s *Suite) Test_CheckLinesOptionsMk__conditionals(c *check.C) { ".endif") t.FinishSetUp() - CheckLinesOptionsMk(mklines) + CheckLinesOptionsMk(mklines, "") t.CheckOutputLines( // This warning comes from VarTypeCheck.PkgOption @@ -300,7 +300,7 @@ func (s *Suite) Test_CheckLinesOptionsMk(c *check.C) { ".elif !empty(PKG_OPTIONS:Msqlite)", ".endif") - CheckLinesOptionsMk(mklines) + CheckLinesOptionsMk(mklines, "") t.CheckOutputLines( "WARN: ~/category/package/options.mk:6: l is used but not defined.", @@ -335,7 +335,7 @@ func (s *Suite) Test_CheckLinesOptionsMk__unexpected_line(c *check.C) { "pre-configure:", "\techo \"In the pre-configure stage.\"") - CheckLinesOptionsMk(mklines) + CheckLinesOptionsMk(mklines, "") t.CheckOutputLines( "WARN: ~/category/package/options.mk:6: "+ @@ -375,7 +375,7 @@ func (s *Suite) Test_CheckLinesOptionsMk__malformed_condition(c *check.C) { ".if ${OPSYS} == 'Darwin'", ".endif") - CheckLinesOptionsMk(mklines) + CheckLinesOptionsMk(mklines, "") t.CheckOutputLines( "WARN: category/package/options.mk:15: Invalid condition, unrecognized part: \"${OPSYS} == 'Darwin'\".") @@ -618,7 +618,7 @@ func (s *Suite) Test_CheckLinesOptionsMk__indirect_supported_options_parentheses ". endif", ".endfor") - CheckLinesOptionsMk(mklines) + CheckLinesOptionsMk(mklines, "") t.CheckOutputLines( "WARN: ~/category/package/options.mk:5: "+ @@ -645,7 +645,7 @@ func (s *Suite) Test_CheckLinesOptionsMk__handled_but_not_supported(c *check.C) ".if ${PKG_OPTIONS:Moption}", ".endif") - CheckLinesOptionsMk(mklines) + CheckLinesOptionsMk(mklines, "") t.CheckOutputLines( "WARN: ~/category/package/options.mk:8: " + @@ -670,7 +670,7 @@ func (s *Suite) Test_CheckLinesOptionsMk__supported_but_not_checked(c *check.C) ".if ${PKG_OPTIONS:Mopt${:Uion}}", ".endif") - CheckLinesOptionsMk(mklines) + CheckLinesOptionsMk(mklines, "") // Pkglint does not expand the ${:Uion}, therefore it doesn't know that // the option is indeed handled. Because of this uncertainty, pkglint diff --git a/pkgtools/pkglint/files/package.go b/pkgtools/pkglint/files/package.go index 5014cffd7e8..fec7a7340d4 100644 --- a/pkgtools/pkglint/files/package.go +++ b/pkgtools/pkglint/files/package.go @@ -16,17 +16,21 @@ const rePkgname = `^([\w\-.+]+)-(\d[.0-9A-Z_a-z]*)$` // This is necessary because variables in Makefiles may be used before they are defined, // and such dependencies often span multiple files that are included indirectly. type Package struct { - dir CurrPath // The directory of the package, for resolving files - Pkgpath PkgsrcPath // e.g. "category/pkgdir" - Pkgdir PackagePath // PKGDIR from the package Makefile - Filesdir PackagePath // FILESDIR from the package Makefile - Patchdir PackagePath // PATCHDIR from the package Makefile - DistinfoFile PackagePath // DISTINFO_FILE from the package Makefile - EffectivePkgname string // PKGNAME or DISTNAME from the package Makefile, including nb13, can be empty - EffectivePkgbase string // EffectivePkgname without the version - EffectivePkgversion string // The version part of the effective PKGNAME, excluding nb13 - EffectivePkgnameLine *MkLine // The origin of the three Effective* values - Plist PlistContent // Files and directories mentioned in the PLIST files + dir CurrPath // The directory of the package, for resolving files + Pkgpath PkgsrcPath // e.g. "category/pkgdir" + + EffectivePkgname string // PKGNAME or DISTNAME from the package Makefile, including nb13, can be empty + EffectivePkgbase string // EffectivePkgname without the version + EffectivePkgversion string // The version part of the effective PKGNAME, excluding nb13 + EffectivePkgnameLine *MkLine // The origin of the three Effective* values + buildlinkID string + optionsID string + + Pkgdir PackagePath // PKGDIR from the package Makefile + Filesdir PackagePath // FILESDIR from the package Makefile + Patchdir PackagePath // PATCHDIR from the package Makefile + DistinfoFile PackagePath // DISTINFO_FILE from the package Makefile + Plist PlistContent // Files and directories mentioned in the PLIST files vars Scope redundant *RedundantScope @@ -55,6 +59,9 @@ type Package struct { // TODO: Be more precise about the purpose of this field. seenInclude bool + // The identifiers for which PKG_BUILD_OPTIONS is defined. + seenPkgbase Once + // During both load() and check(), tells whether bsd.prefs.mk has // already been loaded directly or indirectly. // @@ -160,6 +167,11 @@ func (pkg *Package) load() ([]CurrPath, *MkLines, *MkLines) { if isRelevantMk(filename, basename) { fragmentMklines := LoadMk(filename, pkg, MustSucceed) pkg.collectConditionalIncludes(fragmentMklines) + fragmentMklines.ForEach(func(mkline *MkLine) { + if mkline.IsVarassign() && mkline.Varname() == "pkgbase" { + pkg.seenPkgbase.FirstTime(mkline.Value()) + } + }) } if hasPrefix(basename, "PLIST") { pkg.loadPlistDirs(filename) @@ -311,6 +323,10 @@ func (pkg *Package) parseLine(mklines *MkLines, mkline *MkLine, allLines *MkLine } pkg.vars.Define(varname, mkline) } + + if varname == "pkgbase" { + pkg.seenPkgbase.FirstTime(mkline.Value()) + } } return true } diff --git a/pkgtools/pkglint/files/package_test.go b/pkgtools/pkglint/files/package_test.go index 1b61b598290..0bc06a815d3 100644 --- a/pkgtools/pkglint/files/package_test.go +++ b/pkgtools/pkglint/files/package_test.go @@ -320,6 +320,46 @@ func (s *Suite) Test_Package__case_insensitive(c *check.C) { t.CheckOutputEmpty() } +// This package has several identifiers that all differ: +// - it lives in the directory "package" +// - the package name is "pkgname" +// - it downloads "distname-1.0.tar.gz" +// (in some places the distname is used as the package name) +// - in options.mk its name is "optid" +// - in buildlink3.mk its name is "bl3id" +// All these identifiers should ideally be the same. +// For historic reasons, the package directory and the package name +// may differ. +func (s *Suite) Test_Package__different_package_identifiers(c *check.C) { + t := s.Init(c) + + t.SetUpPackage("category/package", + "DISTNAME=\tdistname-1.0", + "PKGNAME=\tpkgname-1.0") + t.CreateFileLines("mk/bsd.options.mk") + t.CreateFileLines("category/package/options.mk", + MkCvsID, + "", + "PKG_OPTIONS_VAR=\tPKG_OPTIONS.optid", + "PKG_SUPPORTED_OPTIONS=\t# none", + "", + ".include \"../../mk/bsd.options.mk\"", + "", + "# Nothing to do here") + t.CreateFileBuildlink3Id("category/package/buildlink3.mk", "bl3id") + t.Chdir("category/package") + t.FinishSetUp() + + G.checkdirPackage(".") + + t.CheckOutputLines( + "ERROR: buildlink3.mk:3: Package name mismatch "+ + "between \"bl3id\" in this file and \"pkgname\" from Makefile:4.", + "WARN: options.mk:3: The buildlink3 identifier \"bl3id\" "+ + "should be the same as the options identifier \"optid\".") + +} + func (s *Suite) Test_NewPackage(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/pkglint.go b/pkgtools/pkglint/files/pkglint.go index 579e50498f2..858d1a9bb07 100644 --- a/pkgtools/pkglint/files/pkglint.go +++ b/pkgtools/pkglint/files/pkglint.go @@ -571,7 +571,11 @@ func (p *Pkglint) checkReg(filename CurrPath, basename string, depth int, pkg *P case basename == "buildlink3.mk": if mklines := LoadMk(filename, pkg, NotEmpty|LogErrors); mklines != nil { - CheckLinesBuildlink3Mk(mklines) + ck := NewBuildlink3Checker(mklines) + ck.Check() + if pkg != nil { + pkg.buildlinkID = ck.pkgbase + } } case hasPrefix(basename, "DESCR"): @@ -594,7 +598,11 @@ func (p *Pkglint) checkReg(filename CurrPath, basename string, depth int, pkg *P case basename == "options.mk": if mklines := LoadMk(filename, pkg, NotEmpty|LogErrors); mklines != nil { - CheckLinesOptionsMk(mklines) + buildlinkID := "" + if pkg != nil { + buildlinkID = pkg.buildlinkID + } + CheckLinesOptionsMk(mklines, buildlinkID) } case matches(basename, `^patch-[-\w.~+]*\w$`): diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go index b8f9358c027..5070d495b60 100644 --- a/pkgtools/pkglint/files/util_test.go +++ b/pkgtools/pkglint/files/util_test.go @@ -304,6 +304,12 @@ func (s *Suite) Test_alignWith(c *check.C) { test("1234567890=", "V=", "1234567890=") } +func (s *Suite) Test_alignmentToWidths(c *check.C) { + t := s.Init(c) + + t.CheckEquals(alignmentToWidths(8, 72), "\t\t\t\t\t\t\t\t") +} + func (s *Suite) Test_indent(c *check.C) { t := s.Init(c) diff --git a/pkgtools/pkglint/files/varalignblock.go b/pkgtools/pkglint/files/varalignblock.go index c891072be28..68e77b9a674 100644 --- a/pkgtools/pkglint/files/varalignblock.go +++ b/pkgtools/pkglint/files/varalignblock.go @@ -656,9 +656,6 @@ func (info *varalignLine) alignContinuation(valueColumn, rightMarginColumn int) fix.Notef("The continuation backslash should be preceded by a single space.") } else { newSpace = alignmentToWidths(info.uptoValueWidth(), rightMarginColumn) - if newSpace == "" { - newSpace = " " - } fix.Notef( "The continuation backslash should be in column %d, not %d.", rightMarginColumn+1, column+1) @@ -824,7 +821,7 @@ func (p *varalignParts) uptoValueWidth() int { if p.value != "" { return p.spaceAfterValueColumn() } else { - return p.varnameOpColumn() + return p.spaceBeforeValueColumn() } } diff --git a/pkgtools/pkglint/files/varalignblock_test.go b/pkgtools/pkglint/files/varalignblock_test.go index d4d15e6d6cf..32576372361 100644 --- a/pkgtools/pkglint/files/varalignblock_test.go +++ b/pkgtools/pkglint/files/varalignblock_test.go @@ -2063,6 +2063,30 @@ func (s *Suite) Test_VaralignBlock__var_tab_value63_space_cont_tab8_value71_spac vt.Run() } +func (s *Suite) Test_VaralignBlock__var8_tabs80_cont_tab_value_tabs72_cont_tab_value_tabs72_cont_tab_value9(c *check.C) { + vt := NewVaralignTester(s, c) + vt.Input( + "VAR...7=\t\t\t\t\t\t\t\t\t\\", + "\t.\t\t\t\t\t\t\t\t\\", + "\t.\t\t\t\t\t\t\t\t\\", + "\t.") + vt.InputDetab( + "VAR...7= \\", + " . \\", + " . \\", + " .") + vt.Diagnostics( + "NOTE: Makefile:1: The continuation backslash should be in column 73, not 81.") + vt.Autofixes( + "AUTOFIX: Makefile:1: Replacing \"\\t\\t\\t\\t\\t\\t\\t\\t\\t\" with \"\\t\\t\\t\\t\\t\\t\\t\\t\".") + vt.Fixed( + "VAR...7= \\", + " . \\", + " . \\", + " .") + vt.Run() +} + // Up to 2020-01-27, pkglint removed all spaces before the backslash, // which was against the rule of having at least one space. func (s *Suite) Test_VaralignBlock__right_margin(c *check.C) { |