diff options
author | rillig <rillig@pkgsrc.org> | 2018-02-19 12:40:38 +0000 |
---|---|---|
committer | rillig <rillig@pkgsrc.org> | 2018-02-19 12:40:38 +0000 |
commit | 2dd17c0378e6761f5d9553c853ce408f93ea0f69 (patch) | |
tree | da0a43c8a8b791ac8b72631b051afa4e3d74758f | |
parent | 6a47ebea1cc931e0b54acfcdcbaeb9731bbbe331 (diff) | |
download | pkgsrc-2dd17c0378e6761f5d9553c853ce408f93ea0f69.tar.gz |
pkgtools/pkglint: update to 5.5.5
Changes since 5.5.3:
- Removed check for PERL5_PACKLIST, since it was not fixable by the
package author.
- Completely rewrote the check for ordering variables in simple
package Makefiles. Now it reports the variables in the correct order
instead of just saying "this above that" for a few variables.
- Lots of code cleanup and documentation.
35 files changed, 893 insertions, 470 deletions
diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index ce4dfe4847d..74d30213dd5 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.528 2018/01/28 23:21:16 rillig Exp $ +# $NetBSD: Makefile,v 1.529 2018/02/19 12:40:38 rillig Exp $ -PKGNAME= pkglint-5.5.3 +PKGNAME= pkglint-5.5.5 DISTFILES= # none CATEGORIES= pkgtools diff --git a/pkgtools/pkglint/files/autofix.go b/pkgtools/pkglint/files/autofix.go index 3604ed1ae9e..5a760a1bca3 100644 --- a/pkgtools/pkglint/files/autofix.go +++ b/pkgtools/pkglint/files/autofix.go @@ -160,7 +160,7 @@ func (fix *Autofix) InsertBefore(text string) { fix.Describef(fix.lines[0].Lineno, "Inserting a line %q before this line.", text) } -// InsertBefore appends a line after the current line. +// InsertAfter appends a line after the current line. // The newline is added internally. func (fix *Autofix) InsertAfter(text string) { if fix.skip() { @@ -196,14 +196,14 @@ func (fix *Autofix) Notef(format string, args ...interface{}) { fix.diagArgs = args } -// Notef remembers the warning for logging it later when Apply is called. +// Warnf remembers the warning for logging it later when Apply is called. func (fix *Autofix) Warnf(format string, args ...interface{}) { fix.level = llWarn fix.diagFormat = format fix.diagArgs = args } -// Notef remembers the error for logging it later when Apply is called. +// Errorf remembers the error for logging it later when Apply is called. func (fix *Autofix) Errorf(format string, args ...interface{}) { fix.level = llError fix.diagFormat = format @@ -215,7 +215,8 @@ func (fix *Autofix) Explain(explanation ...string) { fix.explanation = explanation } -// Depending on the pkglint mode, either: +// Apply does the actual work. +// Depending on the pkglint mode, it either: // // * logs the associated message (default) // * logs what would be fixed (--show-autofix) diff --git a/pkgtools/pkglint/files/autofix_test.go b/pkgtools/pkglint/files/autofix_test.go index e4e20628ab4..418b52040b5 100644 --- a/pkgtools/pkglint/files/autofix_test.go +++ b/pkgtools/pkglint/files/autofix_test.go @@ -255,7 +255,7 @@ func (s *Suite) Test_Autofix_show_source_code(c *check.C) { t.SetupCommandLine("--show-autofix", "--source") lines := t.SetupFileLinesContinuation("Makefile", - MkRcsId, + MkRcsID, "before \\", "The old song \\", "after") diff --git a/pkgtools/pkglint/files/buildlink3_test.go b/pkgtools/pkglint/files/buildlink3_test.go index f4f93eaecc9..280ef0005eb 100644 --- a/pkgtools/pkglint/files/buildlink3_test.go +++ b/pkgtools/pkglint/files/buildlink3_test.go @@ -7,7 +7,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk(c *check.C) { G.globalData.InitVartypes() mklines := t.NewMkLines("buildlink3.mk", - MkRcsId, + MkRcsID, "# XXX This file was created automatically using createbuildlink-@PKGVERSION@", "", "BUILDLINK_TREE+= Xbae", @@ -45,7 +45,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk_name_mismatch(c *check.C) { G.Pkg.EffectivePkgbase = "X11" G.Pkg.EffectivePkgnameLine = t.NewMkLine("Makefile", 3, "DISTNAME=\tX11-1.0") mklines := t.NewMkLines("buildlink3.mk", - MkRcsId, + MkRcsID, "", "BUILDLINK_TREE+=\ths-X11", "", @@ -70,7 +70,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk_name_mismatch_multiple_inclusion(c * G.globalData.InitVartypes() mklines := t.NewMkLines("buildlink3.mk", - MkRcsId, + MkRcsID, "", "BUILDLINK_TREE+=\tpkgbase1", "", @@ -93,7 +93,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk_name_mismatch_abi_api(c *check.C) { G.globalData.InitVartypes() mklines := t.NewMkLines("buildlink3.mk", - MkRcsId, + MkRcsID, "", "BUILDLINK_TREE+=\ths-X11", "", @@ -118,7 +118,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk_abi_api_versions(c *check.C) { G.globalData.InitVartypes() mklines := t.NewMkLines("buildlink3.mk", - MkRcsId, + MkRcsID, "", "BUILDLINK_TREE+=\ths-X11", "", @@ -143,7 +143,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk_no_BUILDLINK_TREE_at_beginning(c *ch G.globalData.InitVartypes() mklines := t.NewMkLines("buildlink3.mk", - MkRcsId, + MkRcsID, "", ".if !defined(HS_X11_BUILDLINK3_MK)", "HS_X11_BUILDLINK3_MK:=", @@ -167,7 +167,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk_no_BUILDLINK_TREE_at_end(c *check.C) G.globalData.InitVartypes() mklines := t.NewMkLines("buildlink3.mk", - MkRcsId, + MkRcsID, "", "BUILDLINK_DEPMETHOD.hs-X11?=\tfull", "", @@ -196,7 +196,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk_multiple_inclusion_wrong(c *check.C) G.globalData.InitVartypes() mklines := t.NewMkLines("buildlink3.mk", - MkRcsId, + MkRcsID, "", "BUILDLINK_TREE+=\ths-X11", "", @@ -215,7 +215,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk_missing_endif(c *check.C) { G.globalData.InitVartypes() mklines := t.NewMkLines("buildlink3.mk", - MkRcsId, + MkRcsID, "", "BUILDLINK_TREE+=\tpkgbase1", "", @@ -233,7 +233,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk_unknown_dependency_patterns(c *check G.globalData.InitVartypes() mklines := t.NewMkLines("buildlink3.mk", - MkRcsId, + MkRcsID, "", "BUILDLINK_TREE+= hs-X11", "", @@ -260,7 +260,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk_PKGBASE_with_variable(c *check.C) { G.globalData.InitVartypes() mklines := t.NewMkLines("buildlink3.mk", - MkRcsId, + MkRcsID, "", "BUILDLINK_TREE+=\t${PYPKGPREFIX}-wxWidgets", "", @@ -285,7 +285,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk_PKGBASE_with_unknown_variable(c *che G.globalData.InitVartypes() mklines := t.NewMkLines("buildlink3.mk", - MkRcsId, + MkRcsID, "", "BUILDLINK_TREE+=\t${LICENSE}-wxWidgets", "", @@ -316,7 +316,7 @@ func (s *Suite) Test_ChecklinesBuildlink3Mk_indentation(c *check.C) { t.SetupCommandLine("-Wall") G.globalData.InitVartypes() mklines := t.NewMkLines("buildlink3.mk", - MkRcsId, + MkRcsID, "", ".if ${VAAPI_AVAILABLE} == \"yes\"", "", diff --git a/pkgtools/pkglint/files/category_test.go b/pkgtools/pkglint/files/category_test.go index 19472d5fe45..2f36707cffd 100644 --- a/pkgtools/pkglint/files/category_test.go +++ b/pkgtools/pkglint/files/category_test.go @@ -38,7 +38,7 @@ func (s *Suite) Test_CheckdirCategory_invalid_comment(c *check.C) { G.globalData.InitVartypes() t.SetupFileLines("archivers/Makefile", - MkRcsId, + MkRcsID, "COMMENT=\t\\Make $$$$ fast\"", "", "SUBDIR+=\tpackage", diff --git a/pkgtools/pkglint/files/check_test.go b/pkgtools/pkglint/files/check_test.go index bf1b18d55bd..dee97a87eb2 100644 --- a/pkgtools/pkglint/files/check_test.go +++ b/pkgtools/pkglint/files/check_test.go @@ -18,16 +18,20 @@ import ( var equals = check.Equals var deepEquals = check.DeepEquals -const RcsId = "$" + "NetBSD$" -const MkRcsId = "# $" + "NetBSD$" -const PlistRcsId = "@comment $" + "NetBSD$" +const RcsID = "$" + "NetBSD$" +const MkRcsID = "# $" + "NetBSD$" +const PlistRcsID = "@comment $" + "NetBSD$" type Suite struct { Tester *Tester } // Init initializes the suite with the check.C instance for the actual -// test run. See https://github.com/go-check/check/issues/22 +// test run. +// The returned tester can be used to easily setup the test environment +// and check the results using a high-level API. +// +// See https://github.com/go-check/check/issues/22 func (s *Suite) Init(c *check.C) *Tester { t := s.Tester // Has been initialized by SetUpTest if t.checkC != nil { @@ -276,18 +280,18 @@ func (t *Tester) CheckOutputLines(expectedLines ...string) { t.c().Check(emptyToNil(actualLines), deepEquals, emptyToNil(expectedLines)) } -// BeginDebugToStdout redirects all logging output to stdout instead of +// EnableTracing redirects all logging output to stdout instead of // the buffer. This is useful when stepping through the code, especially // in combination with SetupCommandLine("--debug"). -func (t *Tester) BeginDebugToStdout() { +func (t *Tester) EnableTracing() { G.logOut = NewSeparatorWriter(os.Stdout) trace.Out = os.Stdout trace.Tracing = true } -// EndDebugToStdout logs the output to the buffers again, ready to be +// DisableTracing logs the output to the buffers again, ready to be // checked with CheckOutputLines. -func (t *Tester) EndDebugToStdout() { +func (t *Tester) DisableTracing() { G.logOut = NewSeparatorWriter(&t.stdout) trace.Out = &t.stdout trace.Tracing = false diff --git a/pkgtools/pkglint/files/distinfo_test.go b/pkgtools/pkglint/files/distinfo_test.go index a3f02435eeb..66d6729f464 100644 --- a/pkgtools/pkglint/files/distinfo_test.go +++ b/pkgtools/pkglint/files/distinfo_test.go @@ -34,7 +34,7 @@ func (s *Suite) Test_ChecklinesDistinfo_global_hash_mismatch(c *check.C) { otherLine := t.NewLine("other/distinfo", 7, "dummy") G.Hash = map[string]*Hash{"SHA512:pkgname-1.0.tar.gz": {"Some-512-bit-hash", otherLine}} lines := t.NewLines("distinfo", - RcsId, + RcsID, "", "SHA512 (pkgname-1.0.tar.gz) = 12341234") @@ -49,7 +49,7 @@ func (s *Suite) Test_ChecklinesDistinfo_uncommitted_patch(c *check.C) { t := s.Init(c) t.SetupFileLines("patches/patch-aa", - RcsId, + RcsID, "", "--- oldfile", "+++ newfile", @@ -59,7 +59,7 @@ func (s *Suite) Test_ChecklinesDistinfo_uncommitted_patch(c *check.C) { t.SetupFileLines("CVS/Entries", "/distinfo/...") lines := t.SetupFileLines("distinfo", - RcsId, + RcsID, "", "SHA1 (patch-aa) = 5ad1fb9b3c328fff5caa1a23e8f330e707dd50c0") G.CurrentDir = t.TmpDir() @@ -77,7 +77,7 @@ func (s *Suite) Test_ChecklinesDistinfo_unrecorded_patches(c *check.C) { t.SetupFileLines("patches/patch-aa") t.SetupFileLines("patches/patch-src-Makefile") lines := t.SetupFileLines("distinfo", - RcsId, + RcsID, "", "SHA1 (distfile.tar.gz) = ...", "RMD160 (distfile.tar.gz) = ...", @@ -97,7 +97,7 @@ func (s *Suite) Test_ChecklinesDistinfo_manual_patches(c *check.C) { t.SetupFileLines("patches/manual-libtool.m4") lines := t.SetupFileLines("distinfo", - RcsId, + RcsID, "", "SHA1 (patch-aa) = ...") G.CurrentDir = t.TmpDir() diff --git a/pkgtools/pkglint/files/files.go b/pkgtools/pkglint/files/files.go index 7997a97b803..b51a075cfc3 100644 --- a/pkgtools/pkglint/files/files.go +++ b/pkgtools/pkglint/files/files.go @@ -5,6 +5,10 @@ import ( "strings" ) +// LoadNonemptyLines loads the given file. +// If the file doesn't exist or is empty, an error is logged. +// +// See [LoadExistingLines]. func LoadNonemptyLines(fname string, joinBackslashLines bool) []Line { lines, err := readLines(fname, joinBackslashLines) if err != nil { diff --git a/pkgtools/pkglint/files/globaldata.go b/pkgtools/pkglint/files/globaldata.go index 4646d3700ef..a8eda72105a 100644 --- a/pkgtools/pkglint/files/globaldata.go +++ b/pkgtools/pkglint/files/globaldata.go @@ -20,7 +20,7 @@ type GlobalData struct { suggestedUpdates []SuggestedUpdate // suggestedWipUpdates []SuggestedUpdate // LastChange map[string]*Change // - UserDefinedVars map[string]MkLine // varname => line + UserDefinedVars map[string]MkLine // varname => line; used for checking BUILD_DEFS Deprecated map[string]string // vartypes map[string]*Vartype // varcanon => type latest map[string]string // "lang/php[0-9]*" => "lang/php70" @@ -105,17 +105,17 @@ func (gd *GlobalData) loadDistSites() { fname := gd.Pkgsrcdir + "/mk/fetch/sites.mk" lines := LoadExistingLines(fname, true) - name2url := make(map[string]string) - url2name := make(map[string]string) + nameToUrl := make(map[string]string) + urlToName := make(map[string]string) for _, line := range lines { if m, commented, varname, _, _, _, urls, _, _ := MatchVarassign(line.Text); m { if !commented && hasPrefix(varname, "MASTER_SITE_") && varname != "MASTER_SITE_BACKUP" { for _, url := range splitOnSpace(urls) { if matches(url, `^(?:http://|https://|ftp://)`) { - if name2url[varname] == "" { - name2url[varname] = url + if nameToUrl[varname] == "" { + nameToUrl[varname] = url } - url2name[url] = varname + urlToName[url] = varname } } } @@ -123,13 +123,13 @@ func (gd *GlobalData) loadDistSites() { } // Explicitly allowed, although not defined in mk/fetch/sites.mk. - name2url["MASTER_SITE_LOCAL"] = "ftp://ftp.NetBSD.org/pub/pkgsrc/distfiles/LOCAL_PORTS/" + nameToUrl["MASTER_SITE_LOCAL"] = "ftp://ftp.NetBSD.org/pub/pkgsrc/distfiles/LOCAL_PORTS/" if trace.Tracing { - trace.Stepf("Loaded %d MASTER_SITE_* URLs.", len(url2name)) + trace.Stepf("Loaded %d MASTER_SITE_* URLs.", len(urlToName)) } - gd.MasterSiteURLToVar = url2name - gd.MasterSiteVarToURL = name2url + gd.MasterSiteURLToVar = urlToName + gd.MasterSiteVarToURL = nameToUrl } func (gd *GlobalData) loadPkgOptions() { @@ -180,8 +180,8 @@ func (gd *GlobalData) loadTools() { } } - for _, basename := range [...]string{"bsd.prefs.mk", "bsd.pkg.mk"} { - fname := G.globalData.Pkgsrcdir + "/mk/" + basename + for _, relativeName := range [...]string{"mk/bsd.prefs.mk", "mk/bsd.pkg.mk"} { + fname := G.globalData.Pkgsrcdir + "/" + relativeName condDepth := 0 lines := LoadExistingLines(fname, true) @@ -196,12 +196,12 @@ func (gd *GlobalData) loadTools() { if trace.Tracing { trace.Stepf("[condDepth=%d] %s", condDepth, value) } - if condDepth == 0 || condDepth == 1 && basename == "bsd.prefs.mk" { + if condDepth == 0 || condDepth == 1 && relativeName == "mk/bsd.prefs.mk" { for _, toolname := range splitOnSpace(value) { if !containsVarRef(toolname) { for _, tool := range [...]*Tool{reg.Register(toolname), reg.Register("TOOLS_" + toolname)} { tool.Predefined = true - if basename == "bsd.prefs.mk" { + if relativeName == "mk/bsd.prefs.mk" { tool.UsableAtLoadtime = true } } diff --git a/pkgtools/pkglint/files/globaldata_test.go b/pkgtools/pkglint/files/globaldata_test.go index 427c0bdca41..7111c2a2ca2 100644 --- a/pkgtools/pkglint/files/globaldata_test.go +++ b/pkgtools/pkglint/files/globaldata_test.go @@ -124,7 +124,7 @@ func (s *Suite) Test_GlobalData_loadDistSites(c *check.C) { G.globalData.Pkgsrcdir = t.TmpDir() t.CreateFileLines("mk/fetch/sites.mk", - MkRcsId, + MkRcsID, "", "MASTER_SITE_A+= https://example.org/distfiles/", "MASTER_SITE_B+= https://b.example.org/distfiles/ \\", diff --git a/pkgtools/pkglint/files/mkline.go b/pkgtools/pkglint/files/mkline.go index 7d7a1a619a9..a1724fbea40 100644 --- a/pkgtools/pkglint/files/mkline.go +++ b/pkgtools/pkglint/files/mkline.go @@ -203,14 +203,33 @@ func (mkline *MkLineImpl) IsDependency() bool { return ok } -func (mkline *MkLineImpl) Varname() string { return mkline.data.(mkLineAssign).varname } +// Varname applies to variable assignments and returns the variable name, exactly as given in the Makefile. +func (mkline *MkLineImpl) Varname() string { return mkline.data.(mkLineAssign).varname } + +// Varcanon applies to variable assignments and returns the canonicalized variable name for parameterized variables. +// Examples: +// HOMEPAGE => HOMEPAGE +// SUBST_SED.anything => SUBST_SED.* func (mkline *MkLineImpl) Varcanon() string { return mkline.data.(mkLineAssign).varcanon } + +// Varparam applies to variable assignments and returns the parameter for parameterized variables. +// Examples: +// HOMEPAGE => "" +// SUBST_SED.anything => anything func (mkline *MkLineImpl) Varparam() string { return mkline.data.(mkLineAssign).varparam } -func (mkline *MkLineImpl) Op() MkOperator { return mkline.data.(mkLineAssign).op } + +// Op applies to variable assignments and returns the assignment operator. +func (mkline *MkLineImpl) Op() MkOperator { return mkline.data.(mkLineAssign).op } // For a variable assignment, the text up to and including the assignment operator, e.g. VARNAME+=\t -func (mkline *MkLineImpl) ValueAlign() string { return mkline.data.(mkLineAssign).valueAlign } -func (mkline *MkLineImpl) Value() string { return mkline.data.(mkLineAssign).value } +func (mkline *MkLineImpl) ValueAlign() string { return mkline.data.(mkLineAssign).valueAlign } +func (mkline *MkLineImpl) Value() string { return mkline.data.(mkLineAssign).value } + +// VarassignComment applies to variable assignments and returns the comment. +// Example: +// VAR=value # comment +// In the above line, the comment is "# comment". +// The leading "#" is included so that pkglint can distinguish between no comment at all and an empty comment. func (mkline *MkLineImpl) VarassignComment() string { return mkline.data.(mkLineAssign).comment } func (mkline *MkLineImpl) Shellcmd() string { return mkline.data.(mkLineShell).command } func (mkline *MkLineImpl) Indent() string { @@ -412,13 +431,6 @@ func (mkline *MkLineImpl) VariableNeedsQuoting(varname string, vartype *Vartype, return nqNo } - // Determine whether the context expects a list of shell words or not. - wantList := vuc.vartype.IsConsideredList() - haveList := vartype.IsConsideredList() - if trace.Tracing { - trace.Stepf("wantList=%v, haveList=%v", wantList, haveList) - } - // A shell word may appear as part of a shell word, for example COMPILER_RPATH_FLAG. if vuc.IsWordPart && vuc.quoting == vucQuotPlain { if vartype.kindOfList == lkNone && vartype.basicType == BtShellWord { @@ -426,6 +438,13 @@ func (mkline *MkLineImpl) VariableNeedsQuoting(varname string, vartype *Vartype, } } + // Determine whether the context expects a list of shell words or not. + wantList := vuc.vartype.IsConsideredList() + haveList := vartype.IsConsideredList() + if trace.Tracing { + trace.Stepf("wantList=%v, haveList=%v", wantList, haveList) + } + // Both of these can be correct, depending on the situation: // 1. echo ${PERL5:Q} // 2. xargs ${PERL5} @@ -518,15 +537,15 @@ func (mkline *MkLineImpl) VariableType(varname string) *Vartype { perms |= aclpUseLoadtime } } - return &Vartype{lkNone, BtShellCommand, []AclEntry{{"*", perms}}, false} + return &Vartype{lkNone, BtShellCommand, []ACLEntry{{"*", perms}}, false} } if m, toolvarname := match1(varname, `^TOOLS_(.*)`); m && G.globalData.Tools.byVarname[toolvarname] != nil { - return &Vartype{lkNone, BtPathname, []AclEntry{{"*", aclpUse}}, false} + return &Vartype{lkNone, BtPathname, []ACLEntry{{"*", aclpUse}}, false} } - allowAll := []AclEntry{{"*", aclpAll}} - allowRuntime := []AclEntry{{"*", aclpAllRuntime}} + allowAll := []ACLEntry{{"*", aclpAll}} + allowRuntime := []ACLEntry{{"*", aclpAllRuntime}} // Guess the datatype of the variable based on naming conventions. varbase := varnameBase(varname) @@ -635,7 +654,6 @@ func (mkline *MkLineImpl) DetermineUsedVariables() (varnames []string) { varnames = append(varnames, varname) rest = rest[:m[0]] + rest[m[1]:] } - return } // VarUseContext defines the context in which a variable is defined diff --git a/pkgtools/pkglint/files/mkline_test.go b/pkgtools/pkglint/files/mkline_test.go index 1f70c197e11..8f2ed1d72c2 100644 --- a/pkgtools/pkglint/files/mkline_test.go +++ b/pkgtools/pkglint/files/mkline_test.go @@ -179,7 +179,7 @@ func (s *Suite) Test_NewMkLine__autofix_space_after_varname(c *check.C) { t.SetupCommandLine("-Wspace") fname := t.CreateFileLines("Makefile", - MkRcsId, + MkRcsID, "VARNAME +=\t${VARNAME}", "VARNAME+ =\t${VARNAME+}", "VARNAME+ +=\t${VARNAME+}", @@ -199,7 +199,7 @@ func (s *Suite) Test_NewMkLine__autofix_space_after_varname(c *check.C) { "AUTOFIX: ~/Makefile:2: Replacing \"VARNAME +=\" with \"VARNAME+=\".", "AUTOFIX: ~/Makefile:4: Replacing \"VARNAME+ +=\" with \"VARNAME++=\".") t.CheckFileLines("Makefile", - MkRcsId+"", + MkRcsID+"", "VARNAME+=\t${VARNAME}", "VARNAME+ =\t${VARNAME+}", "VARNAME++=\t${VARNAME+}", @@ -278,7 +278,7 @@ func (s *Suite) Test_MkLines_Check__extra(c *check.C) { G.globalData.InitVartypes() G.Pkg = NewPackage("category/pkgbase") G.Mk = t.NewMkLines("options.mk", - MkRcsId, + MkRcsID, ".for word in ${PKG_FAIL_REASON}", "PYTHON_VERSIONS_ACCEPTED=\t27 35 30", "CONFIGURE_ARGS+=\t--sharedir=${PREFIX}/share/kde", @@ -379,7 +379,7 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__command_in_command(c *check.C) t.SetupTool(&Tool{Name: "sort", Varname: "SORT", Predefined: true}) G.Pkg = NewPackage("category/pkgbase") G.Mk = t.NewMkLines("Makefile", - MkRcsId, + MkRcsID, "GENERATE_PLIST= cd ${DESTDIR}${PREFIX}; ${FIND} * \\( -type f -or -type l \\) | ${SORT};") G.Mk.DetermineDefinedVariables() @@ -395,7 +395,7 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__word_as_part_of_word(c *check. t.SetupCommandLine("-Wall") G.globalData.InitVartypes() G.Mk = t.NewMkLines("Makefile", - MkRcsId, + MkRcsID, "EGDIR=\t${EGDIR}/${MACHINE_GNU_PLATFORM}") MkLineChecker{G.Mk.mklines[1]}.Check() @@ -417,7 +417,7 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__command_as_command_argument(c t.SetupTool(&Tool{Name: "bash", Varname: "BASH", Predefined: true}) G.globalData.InitVartypes() G.Mk = t.NewMkLines("Makefile", - MkRcsId, + MkRcsID, "\t${RUN} cd ${WRKSRC} && ( ${ECHO} ${PERL5:Q} ; ${ECHO} ) | ${BASH} ./install", "\t${RUN} cd ${WRKSRC} && ( ${ECHO} ${PERL5} ; ${ECHO} ) | ${BASH} ./install") @@ -436,7 +436,7 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__URL_as_part_of_word_in_list(c t.SetupCommandLine("-Wall") G.globalData.InitVartypes() G.Mk = t.NewMkLines("Makefile", - MkRcsId, + MkRcsID, "MASTER_SITES=${HOMEPAGE}archive/") MkLineChecker{G.Mk.mklines[1]}.Check() @@ -457,7 +457,7 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__command_in_subshell(c *check.C t.SetupTool(&Tool{Name: "awk", Varname: "AWK", Predefined: true}) t.SetupTool(&Tool{Name: "echo", Varname: "ECHO", Predefined: true}) G.Mk = t.NewMkLines("xpi.mk", - MkRcsId, + MkRcsID, "\t id=$$(${AWK} '{print}' < ${WRKSRC}/idfile) && echo \"$$id\"", "\t id=`${AWK} '{print}' < ${WRKSRC}/idfile` && echo \"$$id\"") @@ -478,7 +478,7 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__LDFLAGS_in_single_quotes(c *ch t.SetupCommandLine("-Wall") G.globalData.InitVartypes() G.Mk = t.NewMkLines("x11/mlterm/Makefile", - MkRcsId, + MkRcsID, "SUBST_SED.link=-e 's|(LIBTOOL_LINK).*(LIBS)|& ${LDFLAGS:M*:Q}|g'", "SUBST_SED.link=-e 's|(LIBTOOL_LINK).*(LIBS)|& '${LDFLAGS:M*:Q}'|g'") @@ -501,7 +501,7 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__package_options(c *check.C) { t.SetupCommandLine("-Wall") G.globalData.InitVartypes() G.Mk = t.NewMkLines("Makefile", - MkRcsId, + MkRcsID, "PKG_SUGGESTED_OPTIONS+=\t${PKG_DEFAULT_OPTIONS:Mcdecimal} ${PKG_OPTIONS.py-trytond:Mcdecimal}") MkLineChecker{G.Mk.mklines[1]}.Check() @@ -516,7 +516,7 @@ func (s *Suite) Test_MkLines_Check__MASTER_SITE_in_HOMEPAGE(c *check.C) { t.SetupMasterSite("MASTER_SITE_GITHUB", "https://github.com/") G.globalData.InitVartypes() G.Mk = t.NewMkLines("devel/catch/Makefile", - MkRcsId, + MkRcsID, "HOMEPAGE=\t${MASTER_SITE_GITHUB:=philsquared/Catch/}", "HOMEPAGE=\t${MASTER_SITE_GITHUB}", "HOMEPAGE=\t${MASTER_SITES}", @@ -539,7 +539,7 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__tool_in_quotes_in_subshell_in_ t.SetupTool(&Tool{Name: "sh", Varname: "SH", Predefined: true}) G.globalData.InitVartypes() G.Mk = t.NewMkLines("x11/labltk/Makefile", - MkRcsId, + MkRcsID, "CONFIGURE_ARGS+=\t-tklibs \"`${SH} -c '${ECHO} $$TK_LD_FLAGS'`\"") MkLineChecker{G.Mk.mklines[1]}.Check() @@ -583,14 +583,14 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__guessed_list_variable_in_quote t.SetupCommandLine("-Wall") G.globalData.InitVartypes() G.Mk = t.NewMkLines("audio/jack-rack/Makefile", - MkRcsId, + MkRcsID, "LADSPA_PLUGIN_PATH?=\t${PREFIX}/lib/ladspa", "CPPFLAGS+=\t\t-DLADSPA_PATH=\"\\\"${LADSPA_PLUGIN_PATH}\\\"\"") G.Mk.Check() t.CheckOutputLines( - "WARN: audio/jack-rack/Makefile:3: The list variable LADSPA_PLUGIN_PATH should not be embedded in a word.") + "WARN: audio/jack-rack/Makefile:3: The variable LADSPA_PLUGIN_PATH should be quoted as part of a shell word.") } func (s *Suite) Test_MkLine_variableNeedsQuoting__list_in_list(c *check.C) { @@ -599,7 +599,7 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__list_in_list(c *check.C) { t.SetupCommandLine("-Wall") G.globalData.InitVartypes() G.Mk = t.NewMkLines("x11/eterm/Makefile", - MkRcsId, + MkRcsID, "DISTFILES=\t${DEFAULT_DISTFILES} ${PIXMAP_FILES}") G.Mk.Check() @@ -614,7 +614,7 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__PKGNAME_and_URL_list_in_URL_li t.SetupMasterSite("MASTER_SITE_GNOME", "http://ftp.gnome.org/") G.globalData.InitVartypes() G.Mk = t.NewMkLines("x11/gtk3/Makefile", - MkRcsId, + MkRcsID, "MASTER_SITES=\tftp://ftp.gtk.org/${PKGNAME}/ ${MASTER_SITE_GNOME:=subdir/}") MkLineChecker{G.Mk.mklines[1]}.checkVarassignVaruse() @@ -630,7 +630,7 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__tool_in_CONFIGURE_ENV(c *check G.globalData.Tools = NewToolRegistry() G.globalData.Tools.RegisterVarname("tar", "TAR") mklines := t.NewMkLines("Makefile", - MkRcsId, + MkRcsID, "", "CONFIGURE_ENV+=\tSYS_TAR_COMMAND_PATH=${TOOLS_TAR:Q}") @@ -644,6 +644,31 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__tool_in_CONFIGURE_ENV(c *check "NOTE: Makefile:3: The :Q operator isn't necessary for ${TOOLS_TAR} here.") } +func (s *Suite) Test_MkLine_variableNeedsQuoting__backticks(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wall") + G.globalData.InitVartypes() + G.globalData.Tools = NewToolRegistry() + G.globalData.Tools.RegisterVarname("cat", "CAT") + mklines := t.NewMkLines("Makefile", + MkRcsID, + "", + "COMPILE_CMD=\tcc `${CAT} ${WRKDIR}/compileflags`", + "COMMENT_CMD=\techo `echo ${COMMENT}`") + + MkLineChecker{mklines.mklines[2]}.checkVarassignVaruse() + MkLineChecker{mklines.mklines[3]}.checkVarassignVaruse() + + // Both CAT and WRKDIR are safe from quoting, therefore no warnings. + // But COMMENT may contain arbitrary characters and therefore must + // only appear completely unquoted. There is no practical way of + // using it inside backticks, and luckily there is no need for it. + t.CheckOutputLines( + "WARN: Makefile:4: COMMENT may not be used in any file; it is a write-only variable.", + "WARN: Makefile:4: The variable COMMENT should be quoted as part of a shell word.") +} + // For some well-known directory variables like WRKDIR, PREFIX, LOCALBASE, // the :Q modifier can be safely removed since pkgsrc will never support // having special characters in these directory names. @@ -655,7 +680,7 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__only_remove_known(c *check.C) G.globalData.InitVartypes() lines := t.SetupFileLinesContinuation("Makefile", - MkRcsId, + MkRcsID, "", "demo: .PHONY", "\t${ECHO} ${WRKSRC:Q}", @@ -667,7 +692,7 @@ func (s *Suite) Test_MkLine_variableNeedsQuoting__only_remove_known(c *check.C) t.CheckOutputLines( "AUTOFIX: ~/Makefile:4: Replacing \"${WRKSRC:Q}\" with \"${WRKSRC}\".") t.CheckFileLines("Makefile", - MkRcsId, + MkRcsID, "", "demo: .PHONY", "\t${ECHO} ${WRKSRC}", @@ -680,7 +705,7 @@ func (s *Suite) Test_MkLine_Pkgmandir(c *check.C) { t.SetupCommandLine("-Wall") G.globalData.InitVartypes() G.Mk = t.NewMkLines("chat/ircII/Makefile", - MkRcsId, + MkRcsID, "CONFIGURE_ARGS+=--mandir=${DESTDIR}${PREFIX}/man", "CONFIGURE_ARGS+=--mandir=${DESTDIR}${PREFIX}/${PKGMANDIR}") @@ -698,7 +723,7 @@ func (s *Suite) Test_MkLines_Check__VERSION_as_wordpart_in_MASTER_SITES(c *check t.SetupCommandLine("-Wall") G.globalData.InitVartypes() mklines := t.NewMkLines("geography/viking/Makefile", - MkRcsId, + MkRcsID, "MASTER_SITES=\t${MASTER_SITE_SOURCEFORGE:=viking/}${VERSION}/") mklines.Check() @@ -714,7 +739,7 @@ func (s *Suite) Test_MkLines_Check__shell_command_as_wordpart_in_ENV_list(c *che t.SetupCommandLine("-Wall") G.globalData.InitVartypes() mklines := t.NewMkLines("x11/lablgtk1/Makefile", - MkRcsId, + MkRcsID, "CONFIGURE_ENV+=\tCC=${CC}") mklines.Check() @@ -730,7 +755,7 @@ func (s *Suite) Test_MkLine_shell_varuse_in_backt_dquot(c *check.C) { t.SetupCommandLine("-Wall") G.globalData.InitVartypes() mklines := t.NewMkLines("x11/motif/Makefile", - MkRcsId, + MkRcsID, "post-patch:", "\tfiles=`${GREP} -l \".fB$${name}.fP(3)\" *.3`") @@ -756,7 +781,7 @@ func (s *Suite) Test_MkLine__comment_in_comment(c *check.C) { G.globalData.InitVartypes() mklines := t.NewMkLines("Makefile", - MkRcsId, + MkRcsID, "COMMENT=\tPKCS#5 v2.0 PBKDF2 Module") mklines.Check() diff --git a/pkgtools/pkglint/files/mklinechecker.go b/pkgtools/pkglint/files/mklinechecker.go index 8ce532f81e4..bd3af967319 100644 --- a/pkgtools/pkglint/files/mklinechecker.go +++ b/pkgtools/pkglint/files/mklinechecker.go @@ -166,7 +166,7 @@ func (ck MkLineChecker) checkCond(forVars map[string]bool, indentation *Indentat } } - forLoopType := &Vartype{lkSpace, BtUnknown, []AclEntry{{"*", aclpAllRead}}, guessed} + forLoopType := &Vartype{lkSpace, BtUnknown, []ACLEntry{{"*", aclpAllRead}}, guessed} forLoopContext := &VarUseContext{forLoopType, vucTimeParse, vucQuotFor, false} for _, forLoopVar := range mkline.ExtractUsedVariables(values) { ck.CheckVaruse(&MkVarUse{forLoopVar, nil}, forLoopContext) @@ -256,7 +256,7 @@ func (ck MkLineChecker) checkVarassignDefPermissions() { } perms := vartype.EffectivePermissions(mkline.Filename) - var needed AclPermissions + var needed ACLPermissions switch op { case opAssign, opAssignShell, opAssignEval: needed = aclpSet @@ -502,6 +502,8 @@ func (ck MkLineChecker) checkVaruseFor(varname string, vartype *Vartype, needsQu } } +// CheckVaruseShellword checks whether a variable use of the form ${VAR} +// or ${VAR:Modifier} is allowed in a certain context. func (ck MkLineChecker) CheckVaruseShellword(varname string, vartype *Vartype, vuc *VarUseContext, mod string, needsQuoting NeedsQuoting) { if trace.Tracing { defer trace.Call(varname, vartype, vuc, mod, needsQuoting)() @@ -527,24 +529,31 @@ func (ck MkLineChecker) CheckVaruseShellword(varname string, vartype *Vartype, v } else if needsQuoting == nqYes { correctMod := strippedMod + ifelseStr(needMstar, ":M*:Q", ":Q") if correctMod == mod+":Q" && vuc.IsWordPart && !vartype.IsShell() { - mkline.Warnf("The list variable %s should not be embedded in a word.", varname) - Explain( - "When a list variable has multiple elements, this expression expands", - "to something unexpected:", - "", - "Example: ${MASTER_SITE_SOURCEFORGE}directory/ expands to", - "", - "\thttps://mirror1.sf.net/ https://mirror2.sf.net/directory/", - "", - "The first URL is missing the directory. To fix this, write", - "\t${MASTER_SITE_SOURCEFORGE:=directory/}.", - "", - "Example: -l${LIBS} expands to", - "", - "\t-llib1 lib2", - "", - "The second library is missing the -l. To fix this, write", - "${LIBS:@lib@-l${lib}@}.") + if vartype.IsConsideredList() { + mkline.Warnf("The list variable %s should not be embedded in a word.", varname) + Explain( + "When a list variable has multiple elements, this expression expands", + "to something unexpected:", + "", + "Example: ${MASTER_SITE_SOURCEFORGE}directory/ expands to", + "", + "\thttps://mirror1.sf.net/ https://mirror2.sf.net/directory/", + "", + "The first URL is missing the directory. To fix this, write", + "\t${MASTER_SITE_SOURCEFORGE:=directory/}.", + "", + "Example: -l${LIBS} expands to", + "", + "\t-llib1 lib2", + "", + "The second library is missing the -l. To fix this, write", + "${LIBS:@lib@-l${lib}@}.") + } else { + mkline.Warnf("The variable %s should be quoted as part of a shell word.", varname) + Explain( + "This variable can contain spaces or other special characters.", + "Therefore it should be quoted by replacing ${VAR} with ${VAR:Q}.") + } } else if mod != correctMod { if vuc.quoting == vucQuotPlain { @@ -783,17 +792,6 @@ func (ck MkLineChecker) checkVarassignSpecific() { mkline.Warnf("Variable names starting with an underscore (%s) are reserved for internal pkgsrc use.", varname) } - if varname == "PERL5_PACKLIST" && G.Pkg != nil { - if m, p5pkgname := match1(G.Pkg.EffectivePkgbase, `^p5-(.*)`); m { - guess := "auto/" + strings.Replace(p5pkgname, "-", "/", -1) + "/.packlist" - - ucvalue, ucguess := strings.ToUpper(value), strings.ToUpper(guess) - if ucvalue != ucguess && ucvalue != "${PERL5_SITEARCH}/"+ucguess { - mkline.Warnf("Unusual value for PERL5_PACKLIST -- %q expected.", guess) - } - } - } - if varname == "PYTHON_VERSIONS_ACCEPTED" { ck.checkVarassignPythonVersions(varname, value) } diff --git a/pkgtools/pkglint/files/mklinechecker_test.go b/pkgtools/pkglint/files/mklinechecker_test.go index 2807eb41655..bb6ffe5ed94 100644 --- a/pkgtools/pkglint/files/mklinechecker_test.go +++ b/pkgtools/pkglint/files/mklinechecker_test.go @@ -136,7 +136,7 @@ func (s *Suite) Test_MkLineChecker_checkVarassign(c *check.C) { G.globalData.InitVartypes() G.Mk = t.NewMkLines("Makefile", - MkRcsId, + MkRcsID, "ac_cv_libpari_libs+=\t-L${BUILDLINK_PREFIX.pari}/lib") // From math/clisp-pari/Makefile, rev. 1.8 MkLineChecker{G.Mk.mklines[1]}.checkVarassign() @@ -164,7 +164,7 @@ func (s *Suite) Test_MkLineChecker_CheckVarusePermissions(c *check.C) { t.SetupCommandLine("-Wall") G.globalData.InitVartypes() mklines := t.NewMkLines("options.mk", - MkRcsId, + MkRcsID, "COMMENT=\t${GAMES_USER}", "COMMENT:=\t${PKGBASE}", "PYPKGPREFIX=${PKGBASE}") @@ -188,7 +188,7 @@ func (s *Suite) Test_MkLineChecker_CheckVarusePermissions__load_time(c *check.C) t.SetupCommandLine("-Wall") G.globalData.InitVartypes() mklines := t.NewMkLines("options.mk", - MkRcsId, + MkRcsID, "WRKSRC:=${.CURDIR}") mklines.Check() @@ -257,7 +257,7 @@ func (s *Suite) Test_MkLineChecker_CheckCond__comparison_with_shell_command(c *c t.SetupCommandLine("-Wall") G.globalData.InitVartypes() G.Mk = t.NewMkLines("security/openssl/Makefile", - MkRcsId, + MkRcsID, ".if ${PKGSRC_COMPILER} == \"gcc\" && ${CC} == \"cc\"", ".endif") @@ -274,7 +274,7 @@ func (s *Suite) Test_MkLine_CheckCond_comparing_PKGSRC_COMPILER_with_eqeq(c *che t.SetupCommandLine("-Wall") G.globalData.InitVartypes() G.Mk = t.NewMkLines("audio/pulseaudio/Makefile", - MkRcsId, + MkRcsID, ".if ${OPSYS} == \"Darwin\" && ${PKGSRC_COMPILER} == \"clang\"", ".endif") @@ -290,7 +290,7 @@ func (s *Suite) Test_MkLineChecker_CheckVartype__CFLAGS_with_backticks(c *check. t.SetupCommandLine("-Wall") G.globalData.InitVartypes() G.Mk = t.NewMkLines("chat/pidgin-icb/Makefile", - MkRcsId, + MkRcsID, "CFLAGS+=\t`pkg-config pidgin --cflags`") mkline := G.Mk.mklines[1] @@ -313,7 +313,7 @@ func (s *Suite) Test_MkLineChecker_CheckVartype_CFLAGS(c *check.C) { G.globalData.InitVartypes() mklines := t.NewMkLines("Makefile", - MkRcsId, + MkRcsID, "CPPFLAGS.SunOS+=\t-DPIPECOMMAND=\\\"/usr/sbin/sendmail -bs %s\\\"") mklines.Check() @@ -331,7 +331,7 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveIndentation_autofix(c *check.C) t.SetupCommandLine("-Wall", "--autofix") G.globalData.InitVartypes() lines := t.SetupFileLinesContinuation("options.mk", - MkRcsId, + MkRcsID, ".if ${PKGNAME} == pkgname", ".if \\", " ${PLATFORM:MNetBSD-4.*}", @@ -346,10 +346,35 @@ func (s *Suite) Test_MkLineChecker_checkDirectiveIndentation_autofix(c *check.C) "AUTOFIX: ~/options.mk:5: Replacing \".\" with \". \".") t.CheckFileLines("options.mk", - MkRcsId, + MkRcsID, ".if ${PKGNAME} == pkgname", ". if \\", " ${PLATFORM:MNetBSD-4.*}", ". endif", ".endif") } + +func (s *Suite) Test_MkLineChecker_CheckVaruseShellword(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Wall") + G.globalData.InitVartypes() + lines := t.SetupFileLinesContinuation("options.mk", + MkRcsID, + "GOPATH=\t${WRKDIR}", + "do-build:", + "\tcd ${WRKSRC} && GOPATH=${GOPATH} PATH=${PATH} :") + mklines := NewMkLines(lines) + + mklines.Check() + + // For WRKSRC and GOPATH, no quoting is necessary since pkgsrc directories by + // definition don't contain special characters. Therefore they don't need the + // :Q, not even when used as part of a shell word. + + // For PATH, the quoting is necessary because it may contain directories outside + // of pkgsrc, and these may contain special characters. + + t.CheckOutputLines( + "WARN: ~/options.mk:4: The variable PATH should be quoted as part of a shell word.") +} diff --git a/pkgtools/pkglint/files/mklines_test.go b/pkgtools/pkglint/files/mklines_test.go index f6da0315141..106ed4ffebb 100644 --- a/pkgtools/pkglint/files/mklines_test.go +++ b/pkgtools/pkglint/files/mklines_test.go @@ -9,7 +9,7 @@ func (s *Suite) Test_MkLines_Check__autofix_conditional_indentation(c *check.C) t.SetupCommandLine("--autofix", "-Wspace") lines := t.SetupFileLines("fname.mk", - MkRcsId, + MkRcsID, ".if defined(A)", ".for a in ${A}", ".if defined(C)", @@ -39,7 +39,7 @@ func (s *Suite) Test_MkLines_Check__unusual_target(c *check.C) { t := s.Init(c) mklines := t.NewMkLines("Makefile", - MkRcsId, + MkRcsID, "", "echo: echo.c", "\tcc -o ${.TARGET} ${.IMPSRC}") @@ -69,7 +69,7 @@ func (s *Suite) Test_MkLines_quoting_LDFLAGS_for_GNU_configure(c *check.C) { G.globalData.InitVartypes() G.Pkg = NewPackage("category/pkgbase") mklines := t.NewMkLines("Makefile", - MkRcsId, + MkRcsID, "GNU_CONFIGURE=\tyes", "CONFIGURE_ENV+=\tX_LIBS=${X11_LDFLAGS:Q}") @@ -88,7 +88,7 @@ func (s *Suite) Test_MkLines__for_loop_multiple_variables(c *check.C) { t.SetupTool(&Tool{Name: "find", Varname: "FIND", Predefined: true}) t.SetupTool(&Tool{Name: "pax", Varname: "PAX", Predefined: true}) mklines := t.NewMkLines("Makefile", // From audio/squeezeboxserver - MkRcsId, + MkRcsID, "", ".for _list_ _dir_ in ${SBS_COPY}", "\tcd ${WRKSRC} && ${FIND} ${${_list_}} -type f ! -name '*.orig' 2>/dev/null "+ @@ -109,7 +109,7 @@ func (s *Suite) Test_MkLines__comparing_YesNo_variable_to_string(c *check.C) { t.SetupCommandLine("-Wall") G.globalData.InitVartypes() mklines := t.NewMkLines("databases/gdbm_compat/builtin.mk", - MkRcsId, + MkRcsID, ".if ${USE_BUILTIN.gdbm} == \"no\"", ".endif", ".if ${USE_BUILTIN.gdbm:tu} == \"no\"", // Can never be true, since "no" is not uppercase. @@ -128,7 +128,7 @@ func (s *Suite) Test_MkLines__varuse_sh_modifier(c *check.C) { t.SetupCommandLine("-Wall") G.globalData.InitVartypes() mklines := t.NewMkLines("lang/qore/module.mk", - MkRcsId, + MkRcsID, "qore-version=\tqore --short-version | ${SED} -e s/-.*//", "PLIST_SUBST+=\tQORE_VERSION=\"${qore-version:sh}\"") @@ -152,7 +152,7 @@ func (s *Suite) Test_MkLines__varuse_parameterized(c *check.C) { t.SetupCommandLine("-Wall") G.globalData.InitVartypes() mklines := t.NewMkLines("converters/wv2/Makefile", - MkRcsId, + MkRcsID, "CONFIGURE_ARGS+=\t\t${CONFIGURE_ARGS.${ICONV_TYPE}-iconv}", "CONFIGURE_ARGS.gnu-iconv=\t--with-libiconv=${BUILDLINK_PREFIX.iconv}") @@ -168,7 +168,7 @@ func (s *Suite) Test_MkLines__loop_modifier(c *check.C) { t.SetupCommandLine("-Wall") G.globalData.InitVartypes() mklines := t.NewMkLines("chat/xchat/Makefile", - MkRcsId, + MkRcsID, "GCONF_SCHEMAS=\tapps_xchat_url_handler.schemas", "post-install:", "\t${GCONF_SCHEMAS:@.s.@"+ @@ -189,7 +189,7 @@ func (s *Suite) Test_MkLines__PKG_SKIP_REASON_depending_on_OPSYS(c *check.C) { G.globalData.InitVartypes() mklines := t.NewMkLines("Makefile", - MkRcsId, + MkRcsID, "PKG_SKIP_REASON+=\t\"Fails everywhere\"", ".if ${OPSYS} == \"Cygwin\"", "PKG_SKIP_REASON+=\t\"Fails on Cygwin\"", @@ -207,7 +207,7 @@ func (s *Suite) Test_MkLines__indirect_variables(c *check.C) { t.SetupCommandLine("-Wall") mklines := t.NewMkLines("net/uucp/Makefile", - MkRcsId, + MkRcsID, "", "post-configure:", ".for var in MAIL_PROGRAM CMDPATH", @@ -226,7 +226,7 @@ func (s *Suite) Test_MkLines_Check__list_variable_as_part_of_word(c *check.C) { t.SetupCommandLine("-Wall") mklines := t.NewMkLines("converters/chef/Makefile", - MkRcsId, + MkRcsID, "\tcd ${WRKSRC} && tr '\\r' '\\n' < ${DISTDIR}/${DIST_SUBDIR}/${DISTFILES} > chef.l") mklines.Check() @@ -242,7 +242,7 @@ func (s *Suite) Test_MkLines_Check__absolute_pathname_depending_on_OPSYS(c *chec t.SetupCommandLine("-Wall") G.globalData.InitVartypes() mklines := t.NewMkLines("games/heretic2-demo/Makefile", - MkRcsId, + MkRcsID, ".if ${OPSYS} == \"DragonFly\"", "TOOLS_PLATFORM.gtar=\t/usr/bin/bsdtar", ".endif", @@ -263,7 +263,7 @@ func (s *Suite) Test_MkLines_checkForUsedComment(c *check.C) { t.SetupCommandLine("--show-autofix") t.NewMkLines("Makefile.common", - MkRcsId, + MkRcsID, "", "# used by sysutils/mc", ).checkForUsedComment("sysutils/mc") @@ -275,20 +275,20 @@ func (s *Suite) Test_MkLines_checkForUsedComment(c *check.C) { t.CheckOutputEmpty() t.NewMkLines("Makefile.common", - MkRcsId, + MkRcsID, ).checkForUsedComment("category/package") t.CheckOutputEmpty() t.NewMkLines("Makefile.common", - MkRcsId, + MkRcsID, "", ).checkForUsedComment("category/package") t.CheckOutputEmpty() t.NewMkLines("Makefile.common", - MkRcsId, + MkRcsID, "", "VARNAME=\tvalue", ).checkForUsedComment("category/package") @@ -298,7 +298,7 @@ func (s *Suite) Test_MkLines_checkForUsedComment(c *check.C) { "AUTOFIX: Makefile.common:2: Inserting a line \"# used by category/package\" before this line.") t.NewMkLines("Makefile.common", - MkRcsId, + MkRcsID, "#", "#", ).checkForUsedComment("category/package") @@ -346,7 +346,7 @@ func (s *Suite) Test_MkLines_PrivateTool_Undefined(c *check.C) { t.SetupCommandLine("-Wall") G.globalData.InitVartypes() mklines := t.NewMkLines("fname", - MkRcsId, + MkRcsID, "", "\tmd5sum filename") @@ -362,7 +362,7 @@ func (s *Suite) Test_MkLines_PrivateTool_Defined(c *check.C) { t.SetupCommandLine("-Wall") G.globalData.InitVartypes() mklines := t.NewMkLines("fname", - MkRcsId, + MkRcsID, "TOOLS_CREATE+=\tmd5sum", "", "\tmd5sum filename") @@ -377,7 +377,7 @@ func (s *Suite) Test_MkLines_Check_indentation(c *check.C) { t.SetupCommandLine("-Wall") mklines := t.NewMkLines("options.mk", - MkRcsId, + MkRcsID, ". if !defined(GUARD_MK)", ". if ${OPSYS} == ${OPSYS}", ". for i in ${FILES}", @@ -423,7 +423,7 @@ func (s *Suite) Test_MkLines_wip_category_Makefile(c *check.C) { G.globalData.InitVartypes() t.SetupTool(&Tool{Name: "rm", Varname: "RM", Predefined: true}) mklines := t.NewMkLines("Makefile", - MkRcsId, + MkRcsID, "", "COMMENT=\tWIP pkgsrc packages", "", diff --git a/pkgtools/pkglint/files/mkparser_test.go b/pkgtools/pkglint/files/mkparser_test.go index 02a2a9b062b..2d4e14ab01b 100644 --- a/pkgtools/pkglint/files/mkparser_test.go +++ b/pkgtools/pkglint/files/mkparser_test.go @@ -223,7 +223,7 @@ func (s *Suite) Test_MkParser__varuse_parentheses_autofix(c *check.C) { t.SetupCommandLine("--autofix") G.globalData.InitVartypes() lines := t.SetupFileLines("Makefile", - MkRcsId, + MkRcsID, "COMMENT=$(P1) $(P2)) $(P3:Q) ${BRACES}") mklines := NewMkLines(lines) @@ -234,6 +234,6 @@ func (s *Suite) Test_MkParser__varuse_parentheses_autofix(c *check.C) { "AUTOFIX: ~/Makefile:2: Replacing \"$(P2)\" with \"${P2}\".", "AUTOFIX: ~/Makefile:2: Replacing \"$(P3:Q)\" with \"${P3:Q}\".") t.CheckFileLines("Makefile", - MkRcsId, + MkRcsID, "COMMENT=${P1} ${P2}) ${P3:Q} ${BRACES}") } diff --git a/pkgtools/pkglint/files/mktypes.go b/pkgtools/pkglint/files/mktypes.go index 9a8fb86aa43..186bb6b3119 100644 --- a/pkgtools/pkglint/files/mktypes.go +++ b/pkgtools/pkglint/files/mktypes.go @@ -1,11 +1,16 @@ package main +// MkToken represents a contiguous string from a Makefile. +// It is either a literal string or a variable use. +// // Example (3 tokens): /usr/share/${PKGNAME}/data type MkToken struct { Text string // Used for both literals and varuses. Varuse *MkVarUse } +// MkVarUse represents a reference to a Make variable, with optional modifiers. +// // Example: ${PKGNAME} // Example: ${PKGNAME:S/from/to/} type MkVarUse struct { diff --git a/pkgtools/pkglint/files/package.go b/pkgtools/pkglint/files/package.go index f2ea58863a7..8d99e4c6167 100644 --- a/pkgtools/pkglint/files/package.go +++ b/pkgtools/pkglint/files/package.go @@ -437,7 +437,7 @@ func (pkg *Package) checkfilePackageMakefile(fname string, mklines *MkLines) { pkg.checkUpdate() mklines.Check() - pkg.ChecklinesPackageMakefileVarorder(mklines) + pkg.CheckVarorder(mklines) SaveAutofixChanges(mklines.lines) } @@ -588,7 +588,11 @@ func (pkg *Package) checkUpdate() { } } -func (pkg *Package) ChecklinesPackageMakefileVarorder(mklines *MkLines) { +// CheckVarorder checks that in simple package Makefiles, +// the most common variables appear in a fixed order. +// The order itself is a little arbitrary but provides +// at least a bit of consistency. +func (pkg *Package) CheckVarorder(mklines *MkLines) { if trace.Tracing { defer trace.Call0()() } @@ -597,195 +601,197 @@ func (pkg *Package) ChecklinesPackageMakefileVarorder(mklines *MkLines) { return } - type OccCount uint8 + type Repetition uint8 const ( - once OccCount = iota - optional + optional Repetition = iota + once many ) - type OccDef struct { - varname string - count OccCount - } - type OccGroup struct { - name string - count OccCount - occ []OccDef - } - - var sections = []OccGroup{ - {"Initial comments", once, - []OccDef{}, - }, - {"Unsorted stuff, part 1", once, - []OccDef{ - {"DISTNAME", optional}, - {"PKGNAME", optional}, - {"PKGREVISION", optional}, - {"CATEGORIES", once}, - {"MASTER_SITES", many}, - {"DIST_SUBDIR", optional}, - {"EXTRACT_SUFX", optional}, - {"DISTFILES", many}, - {"SITES.*", many}, - }, - }, - {"Distribution patches", optional, - []OccDef{ - {"PATCH_SITES", optional}, // or once? - {"PATCH_SITE_SUBDIR", optional}, - {"PATCHFILES", optional}, // or once? - {"PATCH_DIST_ARGS", optional}, - {"PATCH_DIST_STRIP", optional}, - {"PATCH_DIST_CAT", optional}, - }, - }, - {"Unsorted stuff, part 2", once, - []OccDef{ - {"MAINTAINER", optional}, - {"OWNER", optional}, - {"HOMEPAGE", optional}, - {"COMMENT", once}, - {"LICENSE", once}, - }, - }, - {"Legal issues", optional, - []OccDef{ - {"LICENSE_FILE", optional}, - {"RESTRICTED", optional}, - {"NO_BIN_ON_CDROM", optional}, - {"NO_BIN_ON_FTP", optional}, - {"NO_SRC_ON_CDROM", optional}, - {"NO_SRC_ON_FTP", optional}, - }, - }, - {"Technical restrictions", optional, - []OccDef{ - {"BROKEN_EXCEPT_ON_PLATFORM", many}, - {"BROKEN_ON_PLATFORM", many}, - {"NOT_FOR_PLATFORM", many}, - {"ONLY_FOR_PLATFORM", many}, - {"NOT_FOR_COMPILER", many}, - {"ONLY_FOR_COMPILER", many}, - {"NOT_FOR_UNPRIVILEGED", optional}, - {"ONLY_FOR_UNPRIVILEGED", optional}, - }, - }, - {"Dependencies", optional, - []OccDef{ - {"BUILD_DEPENDS", many}, - {"TOOL_DEPENDS", many}, - {"DEPENDS", many}, - }, - }, - } - - lineno := 0 - sectindex := -1 - varindex := 0 - nextSection := true - var vars []OccDef - below := make(map[string]string) - var belowWhat string - - // If the current section is optional but contains non-optional - // fields, the complete section may be skipped as long as there - // has not been a non-optional variable. - maySkipSection := false - - // In each iteration, one of the following becomes true: - // - new lineno > old lineno - // - new sectindex > old sectindex - // - new sectindex == old sectindex && new varindex > old varindex - // - new nextSection == true && old nextSection == false - for lineno < len(mklines.lines) { - mkline := mklines.mklines[lineno] - line := mklines.lines[lineno] - text := line.Text - - if trace.Tracing { - trace.Stepf("[varorder] section %d variable %d vars %v", sectindex, varindex, vars) + type Variable struct { + varname string + repetition Repetition + } + type Section struct { + repetition Repetition + vars []Variable + } + variable := func(name string, repetition Repetition) Variable { return Variable{name, repetition} } + section := func(repetition Repetition, vars ...Variable) Section { return Section{repetition, vars} } + + var sections = []Section{ + section(once, + variable("GITHUB_PROJECT", optional), // either here or below MASTER_SITES + variable("GITHUB_TAG", optional), + variable("DISTNAME", optional), + variable("PKGNAME", optional), + variable("PKGREVISION", optional), + variable("CATEGORIES", once), + variable("MASTER_SITES", many), + variable("GITHUB_PROJECT", optional), // either here or at the very top + variable("GITHUB_TAG", optional), + variable("DIST_SUBDIR", optional), + variable("EXTRACT_SUFX", optional), + variable("DISTFILES", many), + variable("SITES.*", many)), + section(optional, + variable("PATCH_SITES", optional), // or once? + variable("PATCH_SITE_SUBDIR", optional), + variable("PATCHFILES", optional), // or once? + variable("PATCH_DIST_ARGS", optional), + variable("PATCH_DIST_STRIP", optional), + variable("PATCH_DIST_CAT", optional)), + section(once, + variable("MAINTAINER", optional), + variable("OWNER", optional), + variable("HOMEPAGE", optional), + variable("COMMENT", once), + variable("LICENSE", once)), + section(optional, + variable("LICENSE_FILE", optional), + variable("RESTRICTED", optional), + variable("NO_BIN_ON_CDROM", optional), + variable("NO_BIN_ON_FTP", optional), + variable("NO_SRC_ON_CDROM", optional), + variable("NO_SRC_ON_FTP", optional)), + section(optional, + variable("BROKEN_EXCEPT_ON_PLATFORM", many), + variable("BROKEN_ON_PLATFORM", many), + variable("NOT_FOR_PLATFORM", many), + variable("ONLY_FOR_PLATFORM", many), + variable("NOT_FOR_COMPILER", many), + variable("ONLY_FOR_COMPILER", many), + variable("NOT_FOR_UNPRIVILEGED", optional), + variable("ONLY_FOR_UNPRIVILEGED", optional)), + section(optional, + variable("BUILD_DEPENDS", many), + variable("TOOL_DEPENDS", many), + variable("DEPENDS", many)), + } + + firstRelevant := -1 + lastRelevant := -1 + skip := func() bool { + relevantVars := make(map[string]bool) + for _, section := range sections { + for _, variable := range section.vars { + relevantVars[variable.varname] = true + } } - if nextSection { - nextSection = false - sectindex++ - if !(sectindex < len(sections)) { + firstIrrelevant := -1 + for i, mkline := range mklines.mklines { + switch { + case mkline.IsVarassign(), mkline.IsCommentedVarassign(): + varcanon := mkline.Varcanon() + if relevantVars[varcanon] { + if firstRelevant == -1 { + firstRelevant = i + } + if firstIrrelevant != -1 { + if trace.Tracing { + trace.Stepf("Skipping varorder because of line %s.", + mklines.mklines[firstIrrelevant].Linenos()) + } + return true + } + lastRelevant = i + } else { + if firstIrrelevant == -1 { + firstIrrelevant = i + } + } + + case mkline.IsComment(), mkline.IsEmpty(): break + + default: + if firstIrrelevant == -1 { + firstIrrelevant = i + } } - vars = sections[sectindex].occ - maySkipSection = sections[sectindex].count == optional - varindex = 0 } - switch { - case hasPrefix(text, "#"): - lineno++ + interesting := mklines.mklines[firstRelevant : lastRelevant+1] - case mkline.IsVarassign(): - varcanon := mkline.Varcanon() + varcanon := func() string { + for len(interesting) != 0 && interesting[0].IsComment() { + interesting = interesting[1:] + } + if len(interesting) != 0 && (interesting[0].IsVarassign() || interesting[0].IsCommentedVarassign()) { + return interesting[0].Varcanon() + } + return "" + } - if belowText, exists := below[varcanon]; exists { - if belowText != "" { - line.Warnf("%s appears too late. Please put it below %s.", varcanon, belowText) - } else { - line.Warnf("%s appears too late. It should be the very first definition.", varcanon) + for _, section := range sections { + for _, variable := range section.vars { + switch variable.repetition { + case optional: + if varcanon() == variable.varname { + interesting = interesting[1:] + } + case once: + if varcanon() == variable.varname { + interesting = interesting[1:] + } else if section.repetition == once { + if variable.varname != "LICENSE" { + if trace.Tracing { + trace.Stepf("Wrong varorder because %s is missing.", variable.varname) + } + return false + } + } + case many: + for varcanon() == variable.varname { + interesting = interesting[1:] + } } - lineno++ - continue } - for varindex < len(vars) && varcanon != vars[varindex].varname && (vars[varindex].count != once || maySkipSection) { - if vars[varindex].count == once { - maySkipSection = false - } - below[vars[varindex].varname] = belowWhat - varindex++ + for len(interesting) != 0 && (interesting[0].IsEmpty() || interesting[0].IsComment()) { + interesting = interesting[1:] } - switch { - case !(varindex < len(vars)): - if sections[sectindex].count != optional { - line.Warnf("Empty line expected.") - } - nextSection = true + } + + return len(interesting) == 0 + } - case varcanon != vars[varindex].varname: - line.Warnf("Expected %s, but found %s.", vars[varindex].varname, varcanon) - lineno++ + if skip() { + return + } - default: - if vars[varindex].count != many { - below[vars[varindex].varname] = belowWhat - varindex++ - } - lineno++ - } - belowWhat = varcanon - - default: - for varindex < len(vars) { - varname := vars[varindex].varname - if vars[varindex].count == once && !maySkipSection { - if varname != "LICENSE" || pkg.once.FirstTime("LICENSE") { - line.Warnf("The canonical position for the required variable %s is here.", varname) - Explain( - "In simple package Makefiles, some common variables should be", - "arranged in a specific order.", - "", - "See doc/Makefile-example or the pkgsrc guide, section", - "\"Package components\", subsection \"Makefile\" for more information.") + var canonical []string + for _, section := range sections { + for _, variable := range section.vars { + found := false + for _, mkline := range mklines.mklines[firstRelevant : lastRelevant+1] { + if mkline.IsVarassign() || mkline.IsCommentedVarassign() { + if mkline.Varcanon() == variable.varname { + canonical = append(canonical, mkline.Varname()) + found = true } } - below[varname] = belowWhat - varindex++ } - nextSection = true - if text == "" { - belowWhat = "the previous empty line" - lineno++ + if !found && section.repetition == once && variable.repetition == once { + canonical = append(canonical, variable.varname) } } + if len(canonical) != 0 && canonical[len(canonical)-1] != "empty line" { + canonical = append(canonical, "empty line") + } + } + if len(canonical) != 0 && canonical[len(canonical)-1] == "empty line" { + canonical = canonical[:len(canonical)-1] } + + mkline := mklines.mklines[firstRelevant] + mkline.Warnf("The canonical order of the variables is %s.", strings.Join(canonical, ", ")) + Explain( + "In simple package Makefiles, some common variables should be", + "arranged in a specific order.", + "", + "See doc/Makefile-example or the pkgsrc guide, section", + "\"Package components\", subsection \"Makefile\" for more information.") } func (mklines *MkLines) checkForUsedComment(relativeName string) { diff --git a/pkgtools/pkglint/files/package_test.go b/pkgtools/pkglint/files/package_test.go index 7869617f148..20352ad2f04 100644 --- a/pkgtools/pkglint/files/package_test.go +++ b/pkgtools/pkglint/files/package_test.go @@ -21,22 +21,25 @@ func (s *Suite) Test_Package_pkgnameFromDistname(c *check.C) { t.CheckOutputEmpty() } -func (s *Suite) Test_Package_ChecklinesPackageMakefileVarorder(c *check.C) { +func (s *Suite) Test_Package_CheckVarorder(c *check.C) { t := s.Init(c) t.SetupCommandLine("-Worder") pkg := NewPackage("x11/9term") - pkg.ChecklinesPackageMakefileVarorder(t.NewMkLines("Makefile", - MkRcsId, + pkg.CheckVarorder(t.NewMkLines("Makefile", + MkRcsID, "", + "GITHUB_PROJECT=project", "DISTNAME=9term", "CATEGORIES=x11")) - t.CheckOutputEmpty() + t.CheckOutputLines( + "WARN: Makefile:3: The canonical order of the variables is " + + "GITHUB_PROJECT, DISTNAME, CATEGORIES, GITHUB_PROJECT, empty line, COMMENT, LICENSE.") - pkg.ChecklinesPackageMakefileVarorder(t.NewMkLines("Makefile", - MkRcsId, + pkg.CheckVarorder(t.NewMkLines("Makefile", + MkRcsID, "", "DISTNAME=9term", "CATEGORIES=x11", @@ -44,8 +47,96 @@ func (s *Suite) Test_Package_ChecklinesPackageMakefileVarorder(c *check.C) { ".include \"../../mk/bsd.pkg.mk\"")) t.CheckOutputLines( - "WARN: Makefile:6: The canonical position for the required variable COMMENT is here.", - "WARN: Makefile:6: The canonical position for the required variable LICENSE is here.") + "WARN: Makefile:3: The canonical order of the variables is " + + "DISTNAME, CATEGORIES, empty line, COMMENT, LICENSE.") +} + +// Ensure that comments and empty lines do not lead to panics. +func (s *Suite) Test_Package_CheckVarorder__comments_do_not_crash(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Worder") + pkg := NewPackage("x11/9term") + + pkg.CheckVarorder(t.NewMkLines("Makefile", + MkRcsID, + "", + "GITHUB_PROJECT=project", + "", + "# comment", + "", + "DISTNAME=9term", + "# comment", + "CATEGORIES=x11")) + + t.CheckOutputLines( + "WARN: Makefile:3: The canonical order of the variables is " + + "GITHUB_PROJECT, DISTNAME, CATEGORIES, GITHUB_PROJECT, empty line, COMMENT, LICENSE.") +} + +func (s *Suite) Test_Package_CheckVarorder__comments_are_ignored(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Worder") + + pkg := NewPackage("x11/9term") + + pkg.CheckVarorder(t.NewMkLines("Makefile", + MkRcsID, + "", + "DISTNAME=\tdistname-1.0", + "CATEGORIES=\tsysutils", + "", + "MAINTAINER=\tpkgsrc-users@pkgsrc.org", + "# comment", + "COMMENT=\tComment", + "LICENSE=\tgnu-gpl-v2")) + + t.CheckOutputEmpty() +} + +func (s *Suite) Test_Package_CheckVarorder__conditionals_skip(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Worder") + + pkg := NewPackage("x11/9term") + + pkg.CheckVarorder(t.NewMkLines("Makefile", + MkRcsID, + "", + "DISTNAME=\tdistname-1.0", + "CATEGORIES=\tsysutils", + "", + ".if ${DISTNAME:Mdistname-*}", + "MAINTAINER=\tpkgsrc-users@pkgsrc.org", + ".endif", + "LICENSE=\tgnu-gpl-v2")) + + // No warning about the missing COMMENT since the conditional + // skips the whole check. + t.CheckOutputEmpty() +} + +func (s *Suite) Test_Package_CheckVarorder_GitHub(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Worder") + pkg := NewPackage("x11/9term") + + pkg.CheckVarorder(t.NewMkLines("Makefile", + MkRcsID, + "", + "DISTNAME=\t\tautocutsel-0.10.0", + "CATEGORIES=\t\tx11", + "MASTER_SITES=\t\t${MASTER_SITE_GITHUB:=sigmike/}", + "GITHUB_PROJECT=\t\tautocutsel", + "GITHUB_TAG=\t\t${PKGVERSION_NOREV}", + "", + "COMMENT=\tComment", + "LICENSE=\tgnu-gpl-v2")) + + t.CheckOutputEmpty() } func (s *Suite) Test_Package_varorder_license(c *check.C) { @@ -54,11 +145,11 @@ func (s *Suite) Test_Package_varorder_license(c *check.C) { t.SetupCommandLine("-Worder") t.CreateFileLines("mk/bsd.pkg.mk", "# dummy") - t.CreateFileLines("x11/Makefile", MkRcsId) - t.CreateFileLines("x11/9term/PLIST", PlistRcsId, "bin/9term") - t.CreateFileLines("x11/9term/distinfo", RcsId) + t.CreateFileLines("x11/Makefile", MkRcsID) + t.CreateFileLines("x11/9term/PLIST", PlistRcsID, "bin/9term") + t.CreateFileLines("x11/9term/distinfo", RcsID) t.CreateFileLines("x11/9term/Makefile", - MkRcsId, + MkRcsID, "", "DISTNAME=9term-1.0", "CATEGORIES=x11", @@ -79,24 +170,81 @@ func (s *Suite) Test_Package_varorder_license(c *check.C) { } // https://mail-index.netbsd.org/tech-pkg/2017/01/18/msg017698.html -func (s *Suite) Test_Package_ChecklinesPackageMakefileVarorder__MASTER_SITES(c *check.C) { +func (s *Suite) Test_Package_CheckVarorder__MASTER_SITES(c *check.C) { t := s.Init(c) t.SetupCommandLine("-Worder") pkg := NewPackage("category/package") - pkg.ChecklinesPackageMakefileVarorder(t.NewMkLines("Makefile", - MkRcsId, + pkg.CheckVarorder(t.NewMkLines("Makefile", + MkRcsID, "", "PKGNAME=\tpackage-1.0", "CATEGORIES=\tcategory", "MASTER_SITES=\thttp://example.org/", - "MASTER_SITES+=\thttp://mirror.example.org/")) + "MASTER_SITES+=\thttp://mirror.example.org/", + "", + "COMMENT=\tComment", + "LICENSE=\tgnu-gpl-v2")) // No warning that "MASTER_SITES appears too late" t.CheckOutputEmpty() } +// The diagnostics must be helpful. +// In the case of wip/ioping, they were ambiguous and wrong. +func (s *Suite) Test_Package_CheckVarorder__diagnostics(c *check.C) { + t := s.Init(c) + + t.SetupCommandLine("-Worder") + G.globalData.InitVartypes() + pkg := NewPackage("category/package") + + pkg.CheckVarorder(t.NewMkLines("Makefile", + MkRcsID, + "", + "CATEGORIES= net", + "", + "COMMENT= Comment", + "LICENSE= gnu-gpl-v3", + "", + "GITHUB_PROJECT= pkgbase", + "DISTNAME= v1.0", + "PKGNAME= ${GITHUB_PROJECT}-${DISTNAME}", + "MASTER_SITES= ${MASTER_SITE_GITHUB:=project/}", + "DIST_SUBDIR= ${GITHUB_PROJECT}", + "", + "MAINTAINER= maintainer@example.org", + "HOMEPAGE= https://github.com/project/pkgbase/", + "", + ".include \"../../mk/bsd.pkg.mk\"")) + + t.CheckOutputLines( + "WARN: Makefile:3: The canonical order of the variables is " + + "GITHUB_PROJECT, DISTNAME, PKGNAME, CATEGORIES, MASTER_SITES, GITHUB_PROJECT, DIST_SUBDIR, empty line, " + + "MAINTAINER, HOMEPAGE, COMMENT, LICENSE.") + + // After moving the variables according to the warning: + pkg.CheckVarorder(t.NewMkLines("Makefile", + MkRcsID, + "", + "GITHUB_PROJECT= pkgbase", + "DISTNAME= v1.0", + "PKGNAME= ${GITHUB_PROJECT}-${DISTNAME}", + "CATEGORIES= net", + "MASTER_SITES= ${MASTER_SITE_GITHUB:=project/}", + "DIST_SUBDIR= ${GITHUB_PROJECT}", + "", + "MAINTAINER= maintainer@example.org", + "HOMEPAGE= https://github.com/project/pkgbase/", + "COMMENT= Comment", + "LICENSE= gnu-gpl-v3", + "", + ".include \"../../mk/bsd.pkg.mk\"")) + + t.CheckOutputEmpty() +} + func (s *Suite) Test_Package_getNbpart(c *check.C) { t := s.Init(c) @@ -160,7 +308,7 @@ func (s *Suite) Test_checkdirPackage(c *check.C) { t := s.Init(c) t.SetupFileLines("Makefile", - MkRcsId) + MkRcsID) G.CurrentDir = t.TmpDir() checkdirPackage(t.TmpDir()) @@ -176,7 +324,7 @@ func (s *Suite) Test_checkdirPackage__meta_package_without_license(c *check.C) { t := s.Init(c) t.CreateFileLines("Makefile", - MkRcsId, + MkRcsID, "", "META_PACKAGE=\tyes") G.CurrentDir = t.TmpDir() @@ -216,7 +364,7 @@ func (s *Suite) Test_Package__varuse_at_load_time(c *check.C) { "# dummy") t.CreateFileLines("category/pkgbase/Makefile", - MkRcsId, + MkRcsID, "", "COMMENT= Unit test", "LICENSE= bsd-2", @@ -241,7 +389,7 @@ func (s *Suite) Test_Package__varuse_at_load_time(c *check.C) { "", ".include \"../../mk/bsd.pkg.mk\"") t.CreateFileLines("category/pkgbase/distinfo", - RcsId) + RcsID) (&Pkglint{}).Main("pkglint", "-q", "-Wperm", t.TmpDir()+"/category/pkgbase") @@ -256,7 +404,7 @@ func (s *Suite) Test_Package_loadPackageMakefile(c *check.C) { t := s.Init(c) t.SetupFileLines("category/package/Makefile", - MkRcsId, + MkRcsID, "", "PKGNAME=pkgname-1.67", "DISTNAME=distfile_1_67", @@ -278,7 +426,7 @@ func (s *Suite) Test_Package_conditionalAndUnconditionalInclude(c *check.C) { G.globalData.InitVartypes() t.CreateFileLines("category/package/Makefile", - MkRcsId, + MkRcsID, "", "COMMENT\t=Description", "LICENSE\t= gnu-gpl-v2", @@ -288,17 +436,17 @@ func (s *Suite) Test_Package_conditionalAndUnconditionalInclude(c *check.C) { ".endif", ".include \"../../mk/bsd.pkg.mk\"") t.CreateFileLines("category/package/options.mk", - MkRcsId, + MkRcsID, "", ".if !empty(PKG_OPTIONS:Mzlib)", ". include \"../../devel/zlib/buildlink3.mk\"", ".endif", ".include \"../../sysutils/coreutils/buildlink3.mk\"") t.CreateFileLines("category/package/PLIST", - PlistRcsId, + PlistRcsID, "bin/program") t.CreateFileLines("category/package/distinfo", - RcsId) + RcsID) t.CreateFileLines("devel/zlib/buildlink3.mk", "") t.CreateFileLines("licenses/gnu-gpl-v2", "") diff --git a/pkgtools/pkglint/files/patches.go b/pkgtools/pkglint/files/patches.go index 19099827ace..194304ece76 100644 --- a/pkgtools/pkglint/files/patches.go +++ b/pkgtools/pkglint/files/patches.go @@ -125,7 +125,7 @@ func (ck *PatchChecker) checkUnifiedDiff(patchedFile string) { } ck.checktextUniHunkCr() - for linesToDel > 0 || linesToAdd > 0 || hasPrefix(ck.exp.CurrentLine().Text, "\\") { + for !ck.exp.EOF() && (linesToDel > 0 || linesToAdd > 0 || hasPrefix(ck.exp.CurrentLine().Text, "\\")) { line := ck.exp.CurrentLine() ck.exp.Advance() text := line.Text @@ -149,6 +149,16 @@ func (ck *PatchChecker) checkUnifiedDiff(patchedFile string) { return } } + + // When these two counts are equal, they may refer to context + // lines that consist only of whitespace and have therefore + // been lost during transmission. There is no way to detect + // this by looking only at the patch file. + if linesToAdd != linesToDel { + line := ck.exp.PreviousLine() + line.Warnf("Premature end of patch hunk (expected %d lines to be deleted and %d lines to be added)", + linesToDel, linesToAdd) + } } if !hasHunks { ck.exp.CurrentLine().Errorf("No patch hunks for %q.", patchedFile) diff --git a/pkgtools/pkglint/files/patches_test.go b/pkgtools/pkglint/files/patches_test.go index 4da652b66bc..774f7af9720 100644 --- a/pkgtools/pkglint/files/patches_test.go +++ b/pkgtools/pkglint/files/patches_test.go @@ -7,7 +7,7 @@ func (s *Suite) Test_ChecklinesPatch__with_comment(c *check.C) { t.SetupCommandLine("-Wall") lines := t.NewLines("patch-WithComment", - RcsId, + RcsID, "", "Text", "Text", @@ -29,7 +29,7 @@ func (s *Suite) Test_ChecklinesPatch__without_empty_line__autofix(c *check.C) { t := s.Init(c) patchLines := t.SetupFileLines("patch-WithoutEmptyLines", - RcsId, + RcsID, "Text", "--- file.orig", "+++ file", @@ -39,7 +39,7 @@ func (s *Suite) Test_ChecklinesPatch__without_empty_line__autofix(c *check.C) { "+old line", " context after") t.SetupFileLines("distinfo", - RcsId, + RcsID, "", "SHA1 (some patch) = 87ffcaaa0b0677ec679fff612b44df1af05f04df") // Taken from breakpoint at AutofixDistinfo @@ -56,7 +56,7 @@ func (s *Suite) Test_ChecklinesPatch__without_empty_line__autofix(c *check.C) { "with \"a7c35294b3853da0acedf8a972cb266baa9582a3\".") t.CheckFileLines("patch-WithoutEmptyLines", - RcsId, + RcsID, "", "Text", "", @@ -68,7 +68,7 @@ func (s *Suite) Test_ChecklinesPatch__without_empty_line__autofix(c *check.C) { "+old line", " context after") t.CheckFileLines("distinfo", - RcsId, + RcsID, "", "SHA1 (some patch) = a7c35294b3853da0acedf8a972cb266baa9582a3") } @@ -78,7 +78,7 @@ func (s *Suite) Test_ChecklinesPatch__without_comment(c *check.C) { t.SetupCommandLine("-Wall") lines := t.NewLines("patch-WithoutComment", - RcsId, + RcsID, "", "--- file.orig", "+++ file", @@ -99,7 +99,7 @@ func (s *Suite) Test_ChecklinesPatch__git_without_comment(c *check.C) { t.SetupCommandLine("-Wall") lines := t.NewLines("patch-aa", - RcsId, + RcsID, "", "diff --git a/aa b/aa", "index 1234567..1234567 100644", @@ -130,7 +130,7 @@ func (s *Suite) Test_ChecklinesPatch__error_code(c *check.C) { t.SetupCommandLine("-Wall") lines := t.NewLines("patch-ErrorCode", - RcsId, + RcsID, "", "*** Error code 1", // Looks like a context diff, but isn't. "", @@ -152,7 +152,7 @@ func (s *Suite) Test_ChecklinesPatch__wrong_header_order(c *check.C) { t.SetupCommandLine("-Wall") lines := t.NewLines("patch-WrongOrder", - RcsId, + RcsID, "", "Text", "Text", @@ -176,7 +176,7 @@ func (s *Suite) Test_ChecklinesPatch__context_diff(c *check.C) { t.SetupCommandLine("-Wall") lines := t.NewLines("patch-ctx", - RcsId, + RcsID, "", "diff -cr history.c.orig history.c", "*** history.c.orig", @@ -193,7 +193,7 @@ func (s *Suite) Test_ChecklinesPatch__no_patch(c *check.C) { t := s.Init(c) lines := t.NewLines("patch-aa", - RcsId, + RcsID, "", "-- oldfile", "++ newfile") @@ -208,7 +208,7 @@ func (s *Suite) Test_ChecklinesPatch__two_patched_files(c *check.C) { t := s.Init(c) lines := t.NewLines("patch-aa", - RcsId, + RcsID, "", "--- oldfile", "+++ newfile", @@ -232,7 +232,7 @@ func (s *Suite) Test_ChecklinesPatch__documentation_that_looks_like_patch_lines( t := s.Init(c) lines := t.NewLines("patch-aa", - RcsId, + RcsID, "", "--- oldfile", "", @@ -250,7 +250,7 @@ func (s *Suite) Test_ChecklinesPatch__only_unified_header_but_no_content(c *chec t := s.Init(c) lines := t.NewLines("patch-unified", - RcsId, + RcsID, "", "Documentation for the patch", "", @@ -267,7 +267,7 @@ func (s *Suite) Test_ChecklinesPatch__only_context_header_but_no_content(c *chec t := s.Init(c) lines := t.NewLines("patch-context", - RcsId, + RcsID, "", "Documentation for the patch", "", @@ -286,7 +286,7 @@ func (s *Suite) Test_ChecklinesPatch__Makefile_with_absolute_pathnames(c *check. t := s.Init(c) lines := t.NewLines("patch-unified", - RcsId, + RcsID, "", "Documentation for the patch", "", @@ -323,7 +323,7 @@ func (s *Suite) Test_ChecklinesPatch__no_newline_with_text_following(c *check.C) t := s.Init(c) lines := t.NewLines("patch-aa", - RcsId, + RcsID, "", "comment", "", @@ -346,7 +346,7 @@ func (s *Suite) Test_ChecklinesPatch__no_newline(c *check.C) { t := s.Init(c) lines := t.NewLines("patch-aa", - RcsId, + RcsID, "", "comment", "", @@ -367,7 +367,7 @@ func (s *Suite) Test_ChecklinesPatch__empty_lines_left_out_at_eof(c *check.C) { t := s.Init(c) lines := t.NewLines("patch-aa", - RcsId, + RcsID, "", "comment", "", @@ -392,7 +392,7 @@ func (s *Suite) Test_ChecklinesPatch__context_lines_with_tab_instead_of_space(c t := s.Init(c) lines := t.NewLines("patch-aa", - RcsId, + RcsID, "", "comment", "", @@ -415,7 +415,7 @@ func (s *Suite) Test_ChecklinesPatch__autofix_empty_patch(c *check.C) { t.SetupCommandLine("-Wall", "--autofix") lines := t.NewLines("patch-aa", - RcsId) + RcsID) ChecklinesPatch(lines) @@ -428,7 +428,7 @@ func (s *Suite) Test_ChecklinesPatch__autofix_long_empty_patch(c *check.C) { t.SetupCommandLine("-Wall", "--autofix") lines := t.NewLines("patch-aa", - RcsId, + RcsID, "") ChecklinesPatch(lines) diff --git a/pkgtools/pkglint/files/pkglint.go b/pkgtools/pkglint/files/pkglint.go index a20b46a11da..42579e21262 100644 --- a/pkgtools/pkglint/files/pkglint.go +++ b/pkgtools/pkglint/files/pkglint.go @@ -26,6 +26,8 @@ func main() { type Pkglint struct{} +// Main runs the main program with the given arguments. +// args[0] is the program name. func (pkglint *Pkglint) Main(args ...string) (exitcode int) { defer func() { if r := recover(); r != nil { @@ -481,7 +483,7 @@ func Checkfile(fname string) { case hasPrefix(basename, "CHANGES-"): // This only checks the file, but doesn't register the changes globally. - G.globalData.loadDocChangesFromFile(fname) + _ = G.globalData.loadDocChangesFromFile(fname) case matches(fname, `(?:^|/)files/[^/]*$`): // Skip diff --git a/pkgtools/pkglint/files/pkglint_test.go b/pkgtools/pkglint/files/pkglint_test.go index cf4e9216210..02a37e776ac 100644 --- a/pkgtools/pkglint/files/pkglint_test.go +++ b/pkgtools/pkglint/files/pkglint_test.go @@ -42,12 +42,222 @@ func (s *Suite) Test_Pkglint_Main__only(c *check.C) { exitcode := new(Pkglint).ParseCommandLine([]string{"pkglint", "-Wall", "-o", ":Q", "--version"}) - c.Check(exitcode, deepEquals, new(int)) + if c.Check(exitcode, check.NotNil) { + c.Check(*exitcode, equals, 0) + } c.Check(G.opts.LogOnly, deepEquals, []string{":Q"}) t.CheckOutputLines( "@VERSION@") } +func (s *Suite) Test_Pkglint_Main__unknown_option(c *check.C) { + t := s.Init(c) + + exitcode := new(Pkglint).Main("pkglint", "--unknown-option") + + c.Check(exitcode, equals, 1) + t.CheckOutputLines( + "pkglint: unknown option: --unknown-option", + "", + "usage: pkglint [options] dir...", + "", + " -C, --check=check,... enable or disable specific checks", + " -d, --debug log verbose call traces for debugging", + " -e, --explain explain the diagnostics or give further help", + " -f, --show-autofix show what pkglint can fix automatically", + " -F, --autofix try to automatically fix some errors (experimental)", + " -g, --gcc-output-format mimic the gcc output format", + " -h, --help print a detailed usage message", + " -I, --dumpmakefile dump the Makefile after parsing", + " -i, --import prepare the import of a wip package", + " -m, --log-verbose allow the same log message more than once", + " -o, --only only log messages containing the given text", + " -p, --profiling profile the executing program", + " -q, --quiet don't print a summary line when finishing", + " -r, --recursive check subdirectories, too", + " -s, --source show the source lines together with diagnostics", + " -V, --version print the version number of pkglint", + " -W, --warning=warning,... enable or disable groups of warnings", + "", + " Flags for -C, --check:", + " all all of the following", + " none none of the following", + " ALTERNATIVES check ALTERNATIVES files (enabled)", + " bl3 check buildlink3.mk files (enabled)", + " DESCR check DESCR file (enabled)", + " distinfo check distinfo file (enabled)", + " extra check various additional files (disabled)", + " global inter-package checks (disabled)", + " INSTALL check INSTALL and DEINSTALL scripts (enabled)", + " Makefile check Makefiles (enabled)", + " MESSAGE check MESSAGE file (enabled)", + " mk check other .mk files (enabled)", + " patches check patches (enabled)", + " PLIST check PLIST files (enabled)", + "", + " Flags for -W, --warning:", + " all all of the following", + " none none of the following", + " absname warn about use of absolute file names (enabled)", + " directcmd warn about use of direct command names instead of Make variables (enabled)", + " extra enable some extra warnings (disabled)", + " order warn if Makefile entries are unordered (disabled)", + " perm warn about unforeseen variable definition and use (disabled)", + " plist-depr warn about deprecated paths in PLISTs (disabled)", + " plist-sort warn about unsorted entries in PLISTs (disabled)", + " quoting warn about quoting issues (disabled)", + " space warn about inconsistent use of white-space (disabled)", + " style warn about stylistic issues (disabled)", + " types do some simple type checking in Makefiles (enabled)", + "", + " (Prefix a flag with \"no-\" to disable it.)") +} + +// Demonstrates which infrastructure files are necessary to actually run +// pkglint in a realistic scenario. +// For most tests, this setup is too much work, therefore they +// initialize only those parts of the infrastructure they really +// need. +// +// Especially covers Pkglint.PrintSummary and Pkglint.Checkfile. +func (s *Suite) Test_Pkglint_Main__complete_package(c *check.C) { + t := s.Init(c) + + // This file is needed to locate the pkgsrc root directory. + // See findPkgsrcTopdir. + t.CreateFileLines("mk/bsd.pkg.mk", + "# dummy") + + // See GlobalData.loadDocChanges. + // FIXME: pkglint should warn that the latest version in this file + // (1.10) doesn't match the current version in the package (1.11). + t.CreateFileLines("doc/CHANGES-2018", + RcsID, + "", + "Changes to the packages collection and infrastructure in 2018:", + "", + "\tUpdated sysutils/checkperms to 1.10 [rillig 2018-01-05]") + + // See GlobalData.loadSuggestedUpdates. + t.CreateFileLines("doc/TODO", + RcsID, + "", + "Suggested package updates", + "", + "\to checkperms-1.13 [supports more file formats]") + + // The LICENSE in the package Makefile is searched here. + t.CreateFileLines("licenses/bsd-2", + "# dummy") + + // The MASTER_SITES in the package Makefile are searched here. + // See GlobalData.loadDistSites. + t.CreateFileLines("mk/fetch/sites.mk", + MkRcsID, + "", + "MASTER_SITE_GITHUB+=\thttps://github.com/") + + // The options for the PKG_OPTIONS framework must be readable. + // See GlobalData.loadPkgOptions. + t.CreateFileLines("mk/defaults/options.description", + "option Description") + + // The user-defined variables are read in to check for missing + // BUILD_DEFS declarations in the package Makefile. + t.CreateFileLines("mk/defaults/mk.conf", + "# dummy") + + // The tool definitions are read in to check for missing + // USE_TOOLS declarations in the package Makefile. + // They spread over several files from the pkgsrc infrastructure. + t.CreateFileLines("mk/tools/bsd.tools.mk", + ".include \"defaults.mk\"") + t.CreateFileLines("mk/tools/defaults.mk", + "# dummy") + t.CreateFileLines("mk/bsd.prefs.mk", + "# dummy") + + // The existence of this file makes the category "sysutils" valid. + // The category "tools" on the other hand is not valid. + t.CreateFileLines("sysutils/Makefile", + "# dummy") + + // The package Makefile is quite simple, containing just the + // standard variable definitions. The data for checking the variable + // values is partly defined in the pkgsrc infrastructure files + // (as defined in the previous lines), and partly in the pkglint + // code directly. Many details can be found in vartypecheck.go. + t.CreateFileLines("sysutils/checkperms/Makefile", + MkRcsID, + "", + "DISTNAME=\tcheckperms-1.11", + "CATEGORIES=\tsysutils tools", + "MASTER_SITES=\t${MASTER_SITE_GITHUB:=rillig/}", + "", + "MAINTAINER=\tpkgsrc-users@pkgsrc.org", + "HOMEPAGE=\thttps://github.com/rillig/checkperms/", + "COMMENT=\tCheck file permissions", + "LICENSE=\tbsd-2", + "", + ".include \"../../mk/bsd.pkg.mk\"") + + t.CreateFileLines("sysutils/checkperms/MESSAGE", + "===========================================================================", + RcsID, + "", + "After installation, this package has to be configured in a special way.", + "", + "===========================================================================") + + t.CreateFileLines("sysutils/checkperms/PLIST", + PlistRcsID, + "bin/checkperms", + "man/man1/checkperms.1") + + t.CreateFileLines("sysutils/checkperms/README", + "When updating this package, test the pkgsrc bootstrap.") + + t.CreateFileLines("sysutils/checkperms/TODO", + "Make the package work on MS-DOS") + + t.CreateFileLines("sysutils/checkperms/patches/patch-checkperms.c", + RcsID, + "", + "A simple patch demonstrating that pkglint checks for missing", + "removed lines. The hunk headers says that one line is to be", + "removed, but in fact, there is no deletion line below it.", + "", + "--- a/checkperms.c", + "+++ b/checkperms.c", + "@@ -1,1 +1,3 @@", // at line 1, delete 1 line; at line 1, add 3 lines + "+// Header 1", + "+// Header 2", + "+// Header 3") + t.CreateFileLines("sysutils/checkperms/distinfo", + RcsID, + "", + "SHA1 (checkperms-1.12.tar.gz) = 34c084b4d06bcd7a8bba922ff57677e651eeced5", + "RMD160 (checkperms-1.12.tar.gz) = cd95029aa930b6201e9580b3ab7e36dd30b8f925", + "SHA512 (checkperms-1.12.tar.gz) = 43e37b5963c63fdf716acdb470928d7e21a7bdfddd6c85cf626a11acc7f45fa52a53d4bcd83d543150328fe8cec5587987d2d9a7c5f0aaeb02ac1127ab41f8ae", + "Size (checkperms-1.12.tar.gz) = 6621 bytes", + "SHA1 (patch-checkperms.c) = asdfasdf") // Invalid SHA-1 checksum + + new(Pkglint).Main("pkglint", "-Wall", "-Call", t.TempFilename("sysutils/checkperms")) + + t.CheckOutputLines( + "WARN: ~/sysutils/checkperms/Makefile:3: This package should be updated to 1.13 ([supports more file formats]).", + "ERROR: ~/sysutils/checkperms/Makefile:4: Invalid category \"tools\".", + "ERROR: ~/sysutils/checkperms/distinfo:7: SHA1 hash of patches/patch-checkperms.c differs "+ + "(distinfo has asdfasdf, patch file has e775969de639ec703866c0336c4c8e0fdd96309c). "+ + "Run \"@BMAKE@ makepatchsum\".", + "WARN: ~/sysutils/checkperms/patches/patch-checkperms.c:12: Premature end of patch hunk "+ + "(expected 1 lines to be deleted and 0 lines to be added)", + "2 errors and 2 warnings found.", + "(Run \"pkglint -e\" to show explanations.)", + "(Run \"pkglint -fs\" to show what can be fixed automatically.)", + "(Run \"pkglint -F\" to automatically fix some issues.)") +} + // go test -c -covermode count // pkgsrcdir=... // env PKGLINT_TESTCMDLINE="$pkgsrcdir -r" ./pkglint.test -test.coverprofile pkglint.cov @@ -204,14 +414,16 @@ func (s *Suite) Test_ChecklinesMessage__autofix(c *check.C) { ChecklinesMessage(lines) t.CheckOutputLines( - "AUTOFIX: ~/MESSAGE:1: Inserting a line \"===================================="+ - "=======================================\" before this line.", - "AUTOFIX: ~/MESSAGE:1: Inserting a line \"$NetBSD: pkglint_test.go,v 1.14 2018/01/28 23:21:16 rillig Exp $\" before this line.", - "AUTOFIX: ~/MESSAGE:5: Inserting a line \"===================================="+ - "=======================================\" after this line.") + "AUTOFIX: ~/MESSAGE:1: Inserting a line "+ + "\"===========================================================================\" "+ + "before this line.", + "AUTOFIX: ~/MESSAGE:1: Inserting a line \"$"+"NetBSD$\" before this line.", + "AUTOFIX: ~/MESSAGE:5: Inserting a line "+ + "\"===========================================================================\" "+ + "after this line.") t.CheckFileLines("MESSAGE", "===========================================================================", - "$NetBSD: pkglint_test.go,v 1.14 2018/01/28 23:21:16 rillig Exp $", + RcsID, "1", "2", "3", diff --git a/pkgtools/pkglint/files/plist.go b/pkgtools/pkglint/files/plist.go index b961ab17667..feb829b28e0 100644 --- a/pkgtools/pkglint/files/plist.go +++ b/pkgtools/pkglint/files/plist.go @@ -342,17 +342,6 @@ func (ck *PlistChecker) checkpathMan(pline *PlistLine) { func (ck *PlistChecker) checkpathShare(pline *PlistLine) { line, text := pline.line, pline.text switch { - // Disabled due to PR 46570, item "10. It should stop". - case false && hasPrefix(text, "share/applications/") && hasSuffix(text, ".desktop"): - f := "../../sysutils/desktop-file-utils/desktopdb.mk" - if G.opts.WarnExtra && G.Pkg != nil && G.Pkg.included[f] == nil { - line.Warnf("Packages that install a .desktop entry should .include %q.", f) - Explain( - "If *.desktop files contain MimeType keys, the global MIME type", - "registry must be updated by desktop-file-utils. Otherwise, this", - "warning is harmless.") - } - case hasPrefix(text, "share/icons/") && G.Pkg != nil: if hasPrefix(text, "share/icons/hicolor/") && G.Pkg.Pkgpath != "graphics/hicolor-icon-theme" { f := "../../graphics/hicolor-icon-theme/buildlink3.mk" diff --git a/pkgtools/pkglint/files/plist_test.go b/pkgtools/pkglint/files/plist_test.go index 19f5762b954..952da23a98f 100644 --- a/pkgtools/pkglint/files/plist_test.go +++ b/pkgtools/pkglint/files/plist_test.go @@ -54,7 +54,7 @@ func (s *Suite) Test_ChecklinesPlist__empty(c *check.C) { t := s.Init(c) lines := t.NewLines("PLIST", - PlistRcsId) + PlistRcsID) ChecklinesPlist(lines) @@ -66,10 +66,10 @@ func (s *Suite) Test_ChecklinesPlist__commonEnd(c *check.C) { t := s.Init(c) t.SetupFileLines("PLIST.common", - PlistRcsId, + PlistRcsID, "bin/common") lines := t.SetupFileLines("PLIST.common_end", - PlistRcsId, + PlistRcsID, "sbin/common_end") ChecklinesPlist(lines) @@ -83,7 +83,7 @@ func (s *Suite) Test_ChecklinesPlist__conditional(c *check.C) { G.Pkg = NewPackage("category/pkgbase") G.Pkg.plistSubstCond["PLIST.bincmds"] = true lines := t.NewLines("PLIST", - PlistRcsId, + PlistRcsID, "${PLIST.bincmds}bin/subdir/command") ChecklinesPlist(lines) @@ -97,7 +97,7 @@ func (s *Suite) Test_ChecklinesPlist__sorting(c *check.C) { t.SetupCommandLine("-Wplist-sort") lines := t.NewLines("PLIST", - PlistRcsId, + PlistRcsID, "@comment Do not remove", "sbin/i386/6c", "sbin/program", @@ -116,7 +116,7 @@ func (s *Suite) Test_PlistLineSorter_Sort(c *check.C) { t.SetupCommandLine("--autofix") lines := t.SetupFileLines("PLIST", - PlistRcsId, + PlistRcsID, "@comment Do not remove", "A", "b", @@ -150,7 +150,7 @@ func (s *Suite) Test_PlistLineSorter_Sort(c *check.C) { t.CheckOutputLines( "AUTOFIX: ~/PLIST:3: Sorting the whole file.") t.CheckFileLines("PLIST", - PlistRcsId, + PlistRcsID, "@comment Do not remove", // The header ends here "A", "C", @@ -167,30 +167,12 @@ func (s *Suite) Test_PlistLineSorter_Sort(c *check.C) { "@exec echo \"after lib/after.la\"") // The footer starts here } -func (s *Suite) Test_PlistChecker_checkpathShare_Desktop(c *check.C) { - // Disabled due to PR 46570, item "10. It should stop". - return - - t := s.Init(c) - - t.SetupCommandLine("-Wextra") - G.Pkg = NewPackage("category/pkgpath") - - ChecklinesPlist(t.NewLines("PLIST", - PlistRcsId, - "share/applications/pkgbase.desktop")) - - t.CheckOutputLines( - "WARN: PLIST:2: Packages that install a .desktop entry " + - "should .include \"../../sysutils/desktop-file-utils/desktopdb.mk\".") -} - func (s *Suite) Test_PlistChecker_checkpathMan_gz(c *check.C) { t := s.Init(c) G.Pkg = NewPackage("category/pkgbase") lines := t.NewLines("PLIST", - PlistRcsId, + PlistRcsID, "man/man3/strerror.3.gz") ChecklinesPlist(lines) @@ -203,7 +185,7 @@ func (s *Suite) TestPlistChecker_checkpath__PKGMANDIR(c *check.C) { t := s.Init(c) lines := t.NewLines("PLIST", - PlistRcsId, + PlistRcsID, "${PKGMANDIR}/man1/sh.1") ChecklinesPlist(lines) @@ -216,7 +198,7 @@ func (s *Suite) TestPlistChecker_checkpath__python_egg(c *check.C) { t := s.Init(c) lines := t.NewLines("PLIST", - PlistRcsId, + PlistRcsID, "${PYSITELIB}/gdspy-${PKGVERSION}-py${PYVERSSUFFIX}.egg-info/PKG-INFO") ChecklinesPlist(lines) @@ -231,7 +213,7 @@ func (s *Suite) Test_PlistChecker__autofix(c *check.C) { t.SetupCommandLine("-Wall") fname := t.CreateFileLines("PLIST", - PlistRcsId, + PlistRcsID, "lib/libvirt/connection-driver/libvirt_driver_storage.la", "${PLIST.hal}lib/libvirt/connection-driver/libvirt_driver_nodedev.la", "${PLIST.xen}lib/libvirt/connection-driver/libvirt_driver_libxl.la", @@ -271,7 +253,7 @@ func (s *Suite) Test_PlistChecker__autofix(c *check.C) { "AUTOFIX: ~/PLIST:2: Sorting the whole file.") c.Check(len(lines), equals, len(fixedLines)) t.CheckFileLines("PLIST", - PlistRcsId, + PlistRcsID, "${PLIST.xen}lib/libvirt/connection-driver/libvirt_driver_libxl.la", "${PLIST.hal}lib/libvirt/connection-driver/libvirt_driver_nodedev.la", "lib/libvirt/connection-driver/libvirt_driver_storage.la", @@ -302,7 +284,7 @@ func (s *Suite) Test_PlistChecker__remove_same_entries(c *check.C) { t.SetupCommandLine("-Wall") lines := t.SetupFileLines("PLIST", - PlistRcsId, + PlistRcsID, "${PLIST.option1}bin/true", "bin/true", "${PLIST.option1}bin/true", @@ -331,7 +313,7 @@ func (s *Suite) Test_PlistChecker__remove_same_entries(c *check.C) { "AUTOFIX: ~/PLIST:8: Deleting this line.", "AUTOFIX: ~/PLIST:2: Sorting the whole file.") t.CheckFileLines("PLIST", - PlistRcsId, + PlistRcsID, "${PLIST.option2}bin/false", "${PLIST.option3}bin/false", "bin/true") diff --git a/pkgtools/pkglint/files/shell.go b/pkgtools/pkglint/files/shell.go index 90fb2a74d3d..935dbf4a819 100644 --- a/pkgtools/pkglint/files/shell.go +++ b/pkgtools/pkglint/files/shell.go @@ -24,7 +24,7 @@ func NewShellLine(mkline MkLine) *ShellLine { return &ShellLine{mkline} } -var shellcommandsContextType = &Vartype{lkNone, BtShellCommands, []AclEntry{{"*", aclpAllRuntime}}, false} +var shellcommandsContextType = &Vartype{lkNone, BtShellCommands, []ACLEntry{{"*", aclpAllRuntime}}, false} var shellwordVuc = &VarUseContext{shellcommandsContextType, vucTimeUnknown, vucQuotPlain, false} func (shline *ShellLine) CheckWord(token string, checkQuoting bool) { @@ -772,7 +772,7 @@ func (spc *ShellProgramChecker) checkWord(word *ShToken, checkQuoting bool) { spc.shline.CheckWord(word.MkText, checkQuoting) } -func (scc *ShellProgramChecker) checkPipeExitcode(line Line, pipeline *MkShPipeline) { +func (spc *ShellProgramChecker) checkPipeExitcode(line Line, pipeline *MkShPipeline) { if trace.Tracing { defer trace.Call()() } @@ -827,7 +827,7 @@ func (scc *ShellProgramChecker) checkPipeExitcode(line Line, pipeline *MkShPipel } } -func (scc *ShellProgramChecker) checkSetE(list *MkShList, eflag *bool) { +func (spc *ShellProgramChecker) checkSetE(list *MkShList, eflag *bool) { if trace.Tracing { defer trace.Call()() } @@ -835,7 +835,7 @@ func (scc *ShellProgramChecker) checkSetE(list *MkShList, eflag *bool) { // Disabled until the shell parser can recognize "command || exit 1" reliably. if false && G.opts.WarnExtra && !*eflag && "the current token" == ";" { *eflag = true - scc.shline.mkline.Warnf("Please switch to \"set -e\" mode before using a semicolon (the one after %q) to separate commands.", "previous token") + spc.shline.mkline.Warnf("Please switch to \"set -e\" mode before using a semicolon (the one after %q) to separate commands.", "previous token") Explain( "Normally, when a shell command fails (returns non-zero), the", "remaining commands are still executed. For example, the following", diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go index ae10e2ace6f..bd819303e5c 100644 --- a/pkgtools/pkglint/files/shell_test.go +++ b/pkgtools/pkglint/files/shell_test.go @@ -589,7 +589,7 @@ func (s *Suite) Test_ShellLine__shell_comment_with_line_continuation(c *check.C) t := s.Init(c) lines := t.SetupFileLinesContinuation("Makefile", - MkRcsId, + MkRcsID, "pre-install:", "\t"+"# comment\\", "\t"+"echo \"hello\"") @@ -622,7 +622,7 @@ func (s *Suite) Test_ShellLine__variable_outside_quotes(c *check.C) { t.SetupCommandLine("-Wall") G.globalData.InitVartypes() mklines := t.NewMkLines("dummy.mk", - MkRcsId, + MkRcsID, "GZIP=\t${ECHO} $$comment") mklines.Check() diff --git a/pkgtools/pkglint/files/shtypes.go b/pkgtools/pkglint/files/shtypes.go index 084fc6b67ac..f474a9efe99 100644 --- a/pkgtools/pkglint/files/shtypes.go +++ b/pkgtools/pkglint/files/shtypes.go @@ -9,12 +9,12 @@ import ( type ShAtomType uint8 const ( - shtSpace ShAtomType = iota - shtVaruse // ${PREFIX} - shtWord // - shtOperator - shtComment // # ... - shtSubshell // $$( + shtSpace ShAtomType = iota + shtVaruse // ${PREFIX} + shtWord // while, cat, ENV=value + shtOperator // (, ;, | + shtComment // # ... + shtSubshell // $$( ) func (t ShAtomType) String() string { @@ -56,9 +56,9 @@ func (token *ShAtom) String() string { } // Returns nil for plain shell tokens. -func (atom *ShAtom) VarUse() *MkVarUse { - if atom.Type == shtVaruse { - return atom.Data.(*MkVarUse) +func (token *ShAtom) VarUse() *MkVarUse { + if token.Type == shtVaruse { + return token.Data.(*MkVarUse) } return nil } @@ -117,11 +117,3 @@ func NewShToken(mkText string, atoms ...*ShAtom) *ShToken { func (token *ShToken) String() string { return fmt.Sprintf("ShToken(%v)", token.Atoms) } - -func (token *ShToken) IsAssignment() bool { - return matches(token.MkText, `^[A-Za-z_]\w*=`) -} - -func (token *ShToken) IsWord() bool { - return token.Atoms[0].Type.IsWord() -} diff --git a/pkgtools/pkglint/files/textproc/prefixreplacer.go b/pkgtools/pkglint/files/textproc/prefixreplacer.go index 1d1df0ca726..285c5bd7678 100644 --- a/pkgtools/pkglint/files/textproc/prefixreplacer.go +++ b/pkgtools/pkglint/files/textproc/prefixreplacer.go @@ -31,12 +31,12 @@ func (pr *PrefixReplacer) Rest() string { return pr.rest } -// Match returns a matching group from the last matched AdvanceRegexp. +// Group returns a matching group from the last matched AdvanceRegexp. func (pr *PrefixReplacer) Group(index int) string { return pr.m[index] } -// Rest returns the last match from AdvanceStr, AdvanceBytesFunc or AdvanceHspace. +// Str returns the last match from AdvanceStr, AdvanceBytesFunc or AdvanceHspace. func (pr *PrefixReplacer) Str() string { return pr.s } diff --git a/pkgtools/pkglint/files/toplevel.go b/pkgtools/pkglint/files/toplevel.go index 517432931a8..9305eadd865 100644 --- a/pkgtools/pkglint/files/toplevel.go +++ b/pkgtools/pkglint/files/toplevel.go @@ -35,7 +35,7 @@ func CheckdirToplevel() { G.UsedLicenses = make(map[string]bool) G.Hash = make(map[string]*Hash) } - G.Todo = append(G.Todo, ctx.subdirs...) + G.Todo = append(append([]string(nil), ctx.subdirs...), G.Todo...) } } diff --git a/pkgtools/pkglint/files/toplevel_test.go b/pkgtools/pkglint/files/toplevel_test.go index a37c09e81a5..d77eaeeb370 100644 --- a/pkgtools/pkglint/files/toplevel_test.go +++ b/pkgtools/pkglint/files/toplevel_test.go @@ -6,7 +6,7 @@ func (s *Suite) Test_CheckdirToplevel(c *check.C) { t := s.Init(c) t.SetupFileLines("Makefile", - MkRcsId, + MkRcsID, "", "SUBDIR+= x11", "SUBDIR+=\tarchivers", diff --git a/pkgtools/pkglint/files/vardefs.go b/pkgtools/pkglint/files/vardefs.go index 64b81838eed..5a9184b6d9d 100644 --- a/pkgtools/pkglint/files/vardefs.go +++ b/pkgtools/pkglint/files/vardefs.go @@ -585,6 +585,7 @@ func (gd *GlobalData) InitVartypes() { pkg("GNU_CONFIGURE_LIBSUBDIR", lkNone, BtPathname) acl("GNU_CONFIGURE_MANDIR", lkNone, BtPathname, "Makefile, Makefile.common: set") acl("GNU_CONFIGURE_PREFIX", lkNone, BtPathname, "Makefile: set") + pkg("GOPATH", lkNone, BtPathname) acl("HAS_CONFIGURE", lkNone, BtYes, "Makefile, Makefile.common: set") pkglist("HEADER_TEMPLATES", lkShell, BtPathname) pkg("HOMEPAGE", lkNone, BtHomepage) @@ -1015,7 +1016,7 @@ func acl(varname string, kindOfList KindOfList, checker *BasicType, aclentries s m := mustMatch(varname, `^([A-Z_.][A-Z0-9_]*)(|\*|\.\*)$`) varbase, varparam := m[1], m[2] - vtype := &Vartype{kindOfList, checker, parseAclEntries(varname, aclentries), false} + vtype := &Vartype{kindOfList, checker, parseACLEntries(varname, aclentries), false} if G.globalData.vartypes == nil { G.globalData.vartypes = make(map[string]*Vartype) @@ -1028,11 +1029,11 @@ func acl(varname string, kindOfList KindOfList, checker *BasicType, aclentries s } } -func parseAclEntries(varname string, aclentries string) []AclEntry { +func parseACLEntries(varname string, aclentries string) []ACLEntry { if aclentries == "" { return nil } - var result []AclEntry + var result []ACLEntry prevperms := "(first)" for _, arg := range strings.Split(aclentries, "; ") { var globs, perms string @@ -1045,7 +1046,7 @@ func parseAclEntries(varname string, aclentries string) []AclEntry { fmt.Printf("Repeated permissions for %s: %s\n", varname, perms) } prevperms = perms - var permissions AclPermissions + var permissions ACLPermissions for _, perm := range strings.Split(perms, ", ") { switch perm { case "append": @@ -1079,7 +1080,7 @@ func parseAclEntries(varname string, aclentries string) []AclEntry { print(fmt.Sprintf("Ineffective ACL glob %q for varname %q.\n", glob, varname)) } } - result = append(result, AclEntry{glob, permissions}) + result = append(result, ACLEntry{glob, permissions}) } } return result diff --git a/pkgtools/pkglint/files/vartype.go b/pkgtools/pkglint/files/vartype.go index 5b5254bfeeb..df33d3307c2 100644 --- a/pkgtools/pkglint/files/vartype.go +++ b/pkgtools/pkglint/files/vartype.go @@ -10,7 +10,7 @@ import ( type Vartype struct { kindOfList KindOfList basicType *BasicType - aclEntries []AclEntry + aclEntries []ACLEntry guessed bool } @@ -22,31 +22,31 @@ const ( lkShell // List entries are shell words; used in the :M, :S modifiers. ) -type AclEntry struct { +type ACLEntry struct { glob string // Examples: "Makefile", "*.mk" - permissions AclPermissions + permissions ACLPermissions } -type AclPermissions uint8 +type ACLPermissions uint8 const ( - aclpSet AclPermissions = 1 << iota // VAR = value + aclpSet ACLPermissions = 1 << iota // VAR = value aclpSetDefault // VAR ?= value aclpAppend // VAR += value aclpUseLoadtime // OTHER := ${VAR}, OTHER != ${VAR} aclpUse // OTHER = ${VAR} aclpUnknown - aclpAll AclPermissions = aclpAppend | aclpSetDefault | aclpSet | aclpUseLoadtime | aclpUse - aclpAllRuntime AclPermissions = aclpAppend | aclpSetDefault | aclpSet | aclpUse - aclpAllWrite AclPermissions = aclpSet | aclpSetDefault | aclpAppend - aclpAllRead AclPermissions = aclpUseLoadtime | aclpUse + aclpAll = aclpAppend | aclpSetDefault | aclpSet | aclpUseLoadtime | aclpUse + aclpAllRuntime = aclpAppend | aclpSetDefault | aclpSet | aclpUse + aclpAllWrite = aclpSet | aclpSetDefault | aclpAppend + aclpAllRead = aclpUseLoadtime | aclpUse ) -func (perms AclPermissions) Contains(subset AclPermissions) bool { +func (perms ACLPermissions) Contains(subset ACLPermissions) bool { return perms&subset == subset } -func (perms AclPermissions) String() string { +func (perms ACLPermissions) String() string { if perms == 0 { return "none" } @@ -60,7 +60,7 @@ func (perms AclPermissions) String() string { return strings.TrimRight(result, ", ") } -func (perms AclPermissions) HumanString() string { +func (perms ACLPermissions) HumanString() string { result := "" + ifelseStr(perms.Contains(aclpSet), "set, ", "") + ifelseStr(perms.Contains(aclpSetDefault), "given a default value, ", "") + @@ -70,7 +70,7 @@ func (perms AclPermissions) HumanString() string { return strings.TrimRight(result, ", ") } -func (vt *Vartype) EffectivePermissions(fname string) AclPermissions { +func (vt *Vartype) EffectivePermissions(fname string) ACLPermissions { for _, aclEntry := range vt.aclEntries { if m, _ := path.Match(aclEntry.glob, path.Base(fname)); m { return aclEntry.permissions @@ -82,15 +82,15 @@ func (vt *Vartype) EffectivePermissions(fname string) AclPermissions { // Returns the union of all possible permissions. This can be used to // check whether a variable may be defined or used at all, or if it is // read-only. -func (vt *Vartype) Union() AclPermissions { - var permissions AclPermissions +func (vt *Vartype) Union() ACLPermissions { + var permissions ACLPermissions for _, aclEntry := range vt.aclEntries { permissions |= aclEntry.permissions } return permissions } -func (vt *Vartype) AllowedFiles(perms AclPermissions) string { +func (vt *Vartype) AllowedFiles(perms ACLPermissions) string { files := make([]string, 0, len(vt.aclEntries)) for _, aclEntry := range vt.aclEntries { if aclEntry.permissions.Contains(perms) { diff --git a/pkgtools/pkglint/files/vartype_test.go b/pkgtools/pkglint/files/vartype_test.go index c2afd2a58f0..f3996301212 100644 --- a/pkgtools/pkglint/files/vartype_test.go +++ b/pkgtools/pkglint/files/vartype_test.go @@ -9,7 +9,7 @@ func (s *Suite) Test_Vartype_EffectivePermissions(c *check.C) { if t := G.globalData.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.aclEntries, check.DeepEquals, []ACLEntry{{glob: "*", permissions: aclpUse}}) c.Check(t.EffectivePermissions("Makefile"), equals, aclpUse) } @@ -38,7 +38,7 @@ func (s *Suite) Test_AclPermissions_Contains(c *check.C) { } func (s *Suite) Test_AclPermissions_String(c *check.C) { - c.Check(AclPermissions(0).String(), equals, "none") + c.Check(ACLPermissions(0).String(), equals, "none") c.Check(aclpAll.String(), equals, "set, set-default, append, use-loadtime, use") c.Check(aclpUnknown.String(), equals, "unknown") } diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go index 2f0b27d59db..78e684130f4 100644 --- a/pkgtools/pkglint/files/vartypecheck.go +++ b/pkgtools/pkglint/files/vartypecheck.go @@ -674,7 +674,8 @@ func (cv *VartypeCheck) Pathmask() { CheckLineAbsolutePathname(cv.Line, cv.Value) } -// Like Filename, but including slashes +// Like Filename, but including slashes. +// // See http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266 func (cv *VartypeCheck) Pathname() { if cv.Op == opUseMatch { |