diff options
author | rillig <rillig@pkgsrc.org> | 2015-12-05 08:54:08 +0000 |
---|---|---|
committer | rillig <rillig@pkgsrc.org> | 2015-12-05 08:54:08 +0000 |
commit | 0877f14540c0accf4deb3c2926ecc4f349f2fe27 (patch) | |
tree | 3aaae751b6594fe02578755ccff8cb1406827bb8 /pkgtools | |
parent | 8ad1bbc2c95ff3b9469edde34991ce31bfeb47ef (diff) | |
download | pkgsrc-0877f14540c0accf4deb3c2926ecc4f349f2fe27.tar.gz |
Updated pkglint to 5.2.
Changes since 5.1:
* Fixed distinfo check for unrecorded patch files (thanks, wiz)
* Command line options parser accepts abbreviations (--a instead of --autofix)
* Realistic unit tests using temporary files
* General code cleanup (using gometalinter)
Diffstat (limited to 'pkgtools')
32 files changed, 583 insertions, 348 deletions
diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile index af8628fb171..0f70a880576 100644 --- a/pkgtools/pkglint/Makefile +++ b/pkgtools/pkglint/Makefile @@ -1,6 +1,6 @@ -# $NetBSD: Makefile,v 1.469 2015/12/02 21:46:46 rillig Exp $ +# $NetBSD: Makefile,v 1.470 2015/12/05 08:54:08 rillig Exp $ -PKGNAME= pkglint-5.1 +PKGNAME= pkglint-5.2 DISTFILES= # none CATEGORIES= pkgtools diff --git a/pkgtools/pkglint/files/category.go b/pkgtools/pkglint/files/category.go index a6b5db21fce..06dd0acd10f 100644 --- a/pkgtools/pkglint/files/category.go +++ b/pkgtools/pkglint/files/category.go @@ -4,8 +4,8 @@ import ( "sort" ) -type Subdir struct { - subdir string +type subdir struct { + name string line *Line active bool } @@ -42,33 +42,33 @@ func checkdirCategory() { fSubdirs := getSubdirs(G.currentDir) sort.Sort(sort.StringSlice(fSubdirs)) - var mSubdirs []Subdir + var mSubdirs []subdir prevSubdir := "" for !exp.eof() { line := exp.currentLine() text := line.text - if m, commentFlag, indentation, subdir, comment := match4(text, `^(#?)SUBDIR\+=(\s*)(\S+)\s*(?:#\s*(.*?)\s*|)$`); m { + if m, commentFlag, indentation, name, comment := match4(text, `^(#?)SUBDIR\+=(\s*)(\S+)\s*(?:#\s*(.*?)\s*|)$`); m { commentedOut := commentFlag == "#" if commentedOut && comment == "" { - line.warnf("%q commented out without giving a reason.", subdir) + line.warnf("%q commented out without giving a reason.", name) } if indentation != "\t" { line.warnf("Indentation should be a single tab character.") } - if subdir == prevSubdir { - line.errorf("%q must only appear once.", subdir) - } else if subdir < prevSubdir { - line.warnf("%q should come before %q.", subdir, prevSubdir) + if name == prevSubdir { + line.errorf("%q must only appear once.", name) + } else if name < prevSubdir { + line.warnf("%q should come before %q.", name, prevSubdir) } else { // correctly ordered } - mSubdirs = append(mSubdirs, Subdir{subdir, line, !commentedOut}) - prevSubdir = subdir + mSubdirs = append(mSubdirs, subdir{name, line, !commentedOut}) + prevSubdir = name exp.advance() } else { @@ -88,62 +88,62 @@ func checkdirCategory() { fCheck[fsub] = true } for _, msub := range mSubdirs { - mCheck[msub.subdir] = true + mCheck[msub.name] = true } - f_index, f_atend, f_neednext, f_current := 0, false, true, "" - m_index, m_atend, m_neednext, m_current := 0, false, true, "" + fIndex, fAtend, fNeednext, fCurrent := 0, false, true, "" + mIndex, mAtend, mNeednext, mCurrent := 0, false, true, "" var subdirs []string var line *Line - m_active := false + mActive := false - for !(m_atend && f_atend) { - if !m_atend && m_neednext { - m_neednext = false - if m_index >= len(mSubdirs) { - m_atend = true + for !(mAtend && fAtend) { + if !mAtend && mNeednext { + mNeednext = false + if mIndex >= len(mSubdirs) { + mAtend = true line = exp.currentLine() continue } else { - m_current = mSubdirs[m_index].subdir - line = mSubdirs[m_index].line - m_active = mSubdirs[m_index].active - m_index++ + mCurrent = mSubdirs[mIndex].name + line = mSubdirs[mIndex].line + mActive = mSubdirs[mIndex].active + mIndex++ } } - if !f_atend && f_neednext { - f_neednext = false - if f_index >= len(fSubdirs) { - f_atend = true + if !fAtend && fNeednext { + fNeednext = false + if fIndex >= len(fSubdirs) { + fAtend = true continue } else { - f_current = fSubdirs[f_index] - f_index++ + fCurrent = fSubdirs[fIndex] + fIndex++ } } - if !f_atend && (m_atend || f_current < m_current) { - if !mCheck[f_current] { - line.errorf("%q exists in the file system, but not in the Makefile.", f_current) - line.insertBefore("SUBDIR+=\t" + f_current) + if !fAtend && (mAtend || fCurrent < mCurrent) { + if !mCheck[fCurrent] { + line.errorf("%q exists in the file system, but not in the Makefile.", fCurrent) + line.insertBefore("SUBDIR+=\t" + fCurrent) } - f_neednext = true + fNeednext = true - } else if !m_atend && (f_atend || m_current < f_current) { - if !fCheck[m_current] { - line.errorf("%q exists in the Makefile, but not in the file system.", m_current) + } else if !mAtend && (fAtend || mCurrent < fCurrent) { + if !fCheck[mCurrent] { + line.errorf("%q exists in the Makefile, but not in the file system.", mCurrent) line.delete() } - m_neednext = true + mNeednext = true } else { // f_current == m_current - f_neednext = true - m_neednext = true - if m_active { - subdirs = append(subdirs, G.currentDir+"/"+m_current) + fNeednext = true + mNeednext = true + if mActive { + subdirs = append(subdirs, G.currentDir+"/"+mCurrent) } } } diff --git a/pkgtools/pkglint/files/check_test.go b/pkgtools/pkglint/files/check_test.go index 3f0e53ce0ff..25b10d644a6 100644 --- a/pkgtools/pkglint/files/check_test.go +++ b/pkgtools/pkglint/files/check_test.go @@ -2,6 +2,10 @@ package main import ( "bytes" + "io/ioutil" + "os" + "path" + "path/filepath" "testing" check "gopkg.in/check.v1" @@ -13,6 +17,7 @@ var deepEquals = check.DeepEquals type Suite struct { stdout bytes.Buffer stderr bytes.Buffer + tmpdir string } func (s *Suite) Stdout() string { @@ -49,6 +54,17 @@ func (s *Suite) DummyLine() *Line { return NewLine("fname", "1", "dummy", nil) } +func (s *Suite) CreateTmpFile(c *check.C, fname, content string) { + if s.tmpdir == "" { + s.tmpdir = filepath.ToSlash(c.MkDir()) + } + err := os.MkdirAll(s.tmpdir+"/"+path.Dir(fname), 0777) + c.Check(err, check.IsNil) + + err = ioutil.WriteFile(s.tmpdir+"/"+fname, []byte(content), 0666) + c.Check(err, check.IsNil) +} + func (s *Suite) ExpectFatalError(action func()) { if r := recover(); r != nil { if _, ok := r.(pkglintFatal); ok { @@ -69,6 +85,7 @@ func (s *Suite) TearDownTest(c *check.C) { if out := s.Output(); out != "" { c.Logf("Unchecked output; check with: c.Check(s.Output(), equals, %q)", out) } + s.tmpdir = "" } var _ = check.Suite(new(Suite)) diff --git a/pkgtools/pkglint/files/deprecated.go b/pkgtools/pkglint/files/deprecated.go index feb2c1d70eb..e286b9a1475 100644 --- a/pkgtools/pkglint/files/deprecated.go +++ b/pkgtools/pkglint/files/deprecated.go @@ -137,9 +137,6 @@ func getDeprecatedVars() map[string]string { "_PKG_SILENT": "Use RUN (with more error checking) instead.", "_PKG_DEBUG": "Use RUN (with more error checking) instead.", "LICENCE": "Use LICENSE instead.", - // The following variable is not yet deprecated, as there has been - // a large disagreement on the proper spelling. - //ACCEPTABLE_LICENCES Use ACCEPTABLE_LICENSES instead. // November 2007 //USE_NCURSES Include "../../devel/ncurses/buildlink3.mk" instead. diff --git a/pkgtools/pkglint/files/deprecated_test.go b/pkgtools/pkglint/files/deprecated_test.go new file mode 100644 index 00000000000..37687c31624 --- /dev/null +++ b/pkgtools/pkglint/files/deprecated_test.go @@ -0,0 +1,14 @@ +package main + +import ( + check "gopkg.in/check.v1" +) + +func (s *Suite) TestDeprecated(c *check.C) { + G.globalData.deprecated = getDeprecatedVars() + + line := NewLine("Makefile", "5", "dummy", nil) + checklineMkVarassign(line, "USE_PERL5", "=", "yes", "") + + c.Check(s.Output(), equals, "WARN: Makefile:5: Definition of USE_PERL5 is deprecated. Use USE_TOOLS+=perl or USE_TOOLS+=perl:run instead.\n") +} diff --git a/pkgtools/pkglint/files/distinfo.go b/pkgtools/pkglint/files/distinfo.go index 5d5e561aeca..e76d7e624c0 100644 --- a/pkgtools/pkglint/files/distinfo.go +++ b/pkgtools/pkglint/files/distinfo.go @@ -29,11 +29,11 @@ func checklinesDistinfo(lines []*Line) { } type distinfoLinesChecker struct { - fname string - patchdir string // Relative to G.currentDir - isCommitted bool + distinfoFilename string + patchdir string // Relative to G.currentDir + distinfoIsCommitted bool - patches map[string]bool + patches map[string]bool // "patch-aa" => true previousFilename string isPatch bool algorithms []string @@ -49,28 +49,28 @@ func (ck *distinfoLinesChecker) checkLines(lines []*Line) { if i < 2 { continue } - m, alg, fname, hash := match3(line.text, `^(\w+) \((\w[^)]*)\) = (.*)(?: bytes)?$`) + m, alg, filename, hash := match3(line.text, `^(\w+) \((\w[^)]*)\) = (.*)(?: bytes)?$`) if !m { line.errorf("Invalid line.") continue } - if fname != ck.previousFilename { - ck.onFilenameChange(line, fname) + if filename != ck.previousFilename { + ck.onFilenameChange(line, filename) } ck.algorithms = append(ck.algorithms, alg) - ck.checkGlobalMismatch(line, fname, alg, hash) - ck.checkUncommittedPatch(line, fname, hash) + ck.checkGlobalMismatch(line, filename, alg, hash) + ck.checkUncommittedPatch(line, filename, hash) } - ck.onFilenameChange(NewLine(ck.fname, "EOF", "", nil), "") + ck.onFilenameChange(NewLine(ck.distinfoFilename, "EOF", "", nil), "") } -func (ctx *distinfoLinesChecker) onFilenameChange(line *Line, nextFname string) { - prevFname := ctx.previousFilename +func (ck *distinfoLinesChecker) onFilenameChange(line *Line, nextFname string) { + prevFname := ck.previousFilename if prevFname != "" { - algorithms := strings.Join(ctx.algorithms, ", ") - if ctx.isPatch { + algorithms := strings.Join(ck.algorithms, ", ") + if ck.isPatch { if algorithms != "SHA1" { line.errorf("Expected SHA1 hash for %s, got %s.", prevFname, algorithms) } @@ -81,15 +81,15 @@ func (ctx *distinfoLinesChecker) onFilenameChange(line *Line, nextFname string) } } - ctx.isPatch = matches(nextFname, `^patch-.+$`) && fileExists(G.currentDir+"/"+ctx.patchdir+"/"+nextFname) - ctx.previousFilename = nextFname - ctx.algorithms = nil + ck.isPatch = matches(nextFname, `^patch-.+$`) && fileExists(G.currentDir+"/"+ck.patchdir+"/"+nextFname) + ck.previousFilename = nextFname + ck.algorithms = nil } -func (ctx *distinfoLinesChecker) checkPatchSha1(line *Line, fname, distinfoSha1Hex string) { - patchBytes, err := ioutil.ReadFile(fname) +func (ck *distinfoLinesChecker) checkPatchSha1(line *Line, patchFname, distinfoSha1Hex string) { + patchBytes, err := ioutil.ReadFile(G.currentDir + "/" + patchFname) if err != nil { - line.errorf("%s does not exist.", fname) + line.errorf("%s does not exist.", patchFname) return } @@ -102,18 +102,21 @@ func (ctx *distinfoLinesChecker) checkPatchSha1(line *Line, fname, distinfoSha1H } fileSha1Hex := sprintf("%x", h.Sum(nil)) if distinfoSha1Hex != fileSha1Hex { - line.errorf("%s hash of %s differs (distinfo has %s, patch file has %s). Run \"%s makepatchsum\".", "SHA1", fname, distinfoSha1Hex, fileSha1Hex, confMake) + line.errorf("%s hash of %s differs (distinfo has %s, patch file has %s). Run \"%s makepatchsum\".", "SHA1", patchFname, distinfoSha1Hex, fileSha1Hex, confMake) } } func (ck *distinfoLinesChecker) checkUnrecordedPatches() { files, err := ioutil.ReadDir(G.currentDir + "/" + ck.patchdir) if err != nil { - for _, file := range files { - patch := file.Name() - if !ck.patches[patch] { - errorf(ck.fname, NO_LINES, "patch is not recorded. Run \"%s makepatchsum\".", confMake) - } + _ = G.opts.DebugUnchecked && debugf(ck.distinfoFilename, NO_LINES, "Cannot read patchesDir %q: %s", ck.patchdir, err) + return + } + + for _, file := range files { + patch := file.Name() + if !ck.patches[patch] { + errorf(ck.distinfoFilename, NO_LINES, "patch %q is not recorded. Run \"%s makepatchsum\".", ck.patchdir+"/"+patch, confMake) } } } @@ -134,13 +137,13 @@ func (ck *distinfoLinesChecker) checkGlobalMismatch(line *Line, fname, alg, hash } } -func (ck *distinfoLinesChecker) checkUncommittedPatch(line *Line, fname, sha1Hash string) { +func (ck *distinfoLinesChecker) checkUncommittedPatch(line *Line, patchName, sha1Hash string) { if ck.isPatch { - fname := G.currentDir + "/" + ck.patchdir + "/" + fname - if ck.isCommitted && !isCommitted(fname) { - line.warnf("%s/%s is registered in distinfo but not added to CVS.", ck.patchdir, fname) + patchFname := ck.patchdir + "/" + patchName + if ck.distinfoIsCommitted && !isCommitted(G.currentDir+"/"+patchFname) { + line.warnf("%s is registered in distinfo but not added to CVS.", patchFname) } - ck.checkPatchSha1(line, fname, sha1Hash) - ck.patches[fname] = true + ck.checkPatchSha1(line, patchFname, sha1Hash) + ck.patches[patchName] = true } } diff --git a/pkgtools/pkglint/files/distinfo_test.go b/pkgtools/pkglint/files/distinfo_test.go index 653142c3146..8b55439d188 100644 --- a/pkgtools/pkglint/files/distinfo_test.go +++ b/pkgtools/pkglint/files/distinfo_test.go @@ -2,36 +2,79 @@ package main import ( check "gopkg.in/check.v1" - "io/ioutil" - "os" - "path/filepath" ) func (s *Suite) TestChecklinesDistinfo(c *check.C) { - tmpdir := c.MkDir() - patchesdir := tmpdir + "/patches" - patchAa := patchesdir + "/patch-aa" - patchContents := "" + - "$" + "NetBSD$ line is ignored\n" + - "patch contents\n" - - os.Mkdir(patchesdir, 0777) - if err := ioutil.WriteFile(patchAa, []byte(patchContents), 0666); err != nil { - c.Fatal(err) - } - G.currentDir = filepath.ToSlash(tmpdir) - - lines := s.NewLines("distinfo", + s.CreateTmpFile(c, "patches/patch-aa", ""+ + "$"+"NetBSD$ line is ignored\n"+ + "patch contents\n") + G.currentDir = s.tmpdir + + checklinesDistinfo(s.NewLines("distinfo", "should be the RCS ID", "should be empty", "MD5 (distfile.tar.gz) = 12345678901234567890123456789012", "SHA1 (distfile.tar.gz) = 1234567890123456789012345678901234567890", - "SHA1 (patch-aa) = 6b98dd609f85a9eb9c4c1e4e7055a6aaa62b7cc7") - - checklinesDistinfo(lines) + "SHA1 (patch-aa) = 6b98dd609f85a9eb9c4c1e4e7055a6aaa62b7cc7")) c.Check(s.Output(), equals, ""+ "ERROR: distinfo:1: Expected \"$"+"NetBSD$\".\n"+ "NOTE: distinfo:2: Empty line expected.\n"+ "ERROR: distinfo:5: Expected SHA1, RMD160, SHA512, Size checksums for \"distfile.tar.gz\", got MD5, SHA1.\n") } + +func (s *Suite) TestChecklinesDistinfo_GlobalHashMismatch(c *check.C) { + otherLine := NewLine("other/distinfo", "7", "dummy", nil) + G.ipcDistinfo = make(map[string]*Hash) + G.ipcDistinfo["SHA512:pkgname-1.0.tar.gz"] = &Hash{"asdfasdf", otherLine} + + checklinesDistinfo(s.NewLines("distinfo", + "$"+"NetBSD$", + "", + "SHA512 (pkgname-1.0.tar.gz) = 12341234")) + + c.Check(s.Output(), equals, ""+ + "ERROR: distinfo:3: The hash SHA512 for pkgname-1.0.tar.gz is 12341234, ...\n"+ + "ERROR: other/distinfo:7: ... which differs from asdfasdf.\n"+ + "ERROR: distinfo:EOF: Expected SHA1, RMD160, SHA512, Size checksums for \"pkgname-1.0.tar.gz\", got SHA512.\n") +} + +func (s *Suite) TestChecklinesDistinfo_UncommittedPatch(c *check.C) { + s.CreateTmpFile(c, "patches/patch-aa", ""+ + "$"+"NetBSD$\n"+ + "\n"+ + "--- oldfile\n"+ + "+++ newfile\n"+ + "@@ -1,1 +1,1 @@\n"+ + "-old\n"+ + "+new\n") + s.CreateTmpFile(c, "CVS/Entries", + "/distinfo/...\n") + G.currentDir = s.tmpdir + + checklinesDistinfo(s.NewLines(s.tmpdir+"/distinfo", + "$"+"NetBSD$", + "", + "SHA1 (patch-aa) = 5ad1fb9b3c328fff5caa1a23e8f330e707dd50c0")) + + c.Check(s.Output(), equals, "WARN: "+s.tmpdir+"/distinfo:3: patches/patch-aa is registered in distinfo but not added to CVS.\n") +} + +func (s *Suite) TestChecklinesDistinfo_UnrecordedPatches(c *check.C) { + s.CreateTmpFile(c, "patches/patch-aa", "") + s.CreateTmpFile(c, "patches/patch-src-Makefile", "") + G.currentDir = s.tmpdir + + checklinesDistinfo(s.NewLines(s.tmpdir+"/distinfo", + "$"+"NetBSD$", + "", + "SHA1 (distfile.tar.gz) = ...", + "RMD160 (distfile.tar.gz) = ...", + "SHA512 (distfile.tar.gz) = ...", + "Size (distfile.tar.gz) = 1024 bytes")) + + c.Check(s.Output(), equals, sprintf(""+ + "ERROR: %[1]s: patch \"patches/patch-aa\" is not recorded. Run \"%s makepatchsum\".\n"+ + "ERROR: %[1]s: patch \"patches/patch-src-Makefile\" is not recorded. Run \"%s makepatchsum\".\n", + s.tmpdir+"/distinfo", confMake)) +} diff --git a/pkgtools/pkglint/files/getopt.go b/pkgtools/pkglint/files/getopt.go index a07cd41f09c..6cea5d037c9 100644 --- a/pkgtools/pkglint/files/getopt.go +++ b/pkgtools/pkglint/files/getopt.go @@ -11,54 +11,50 @@ import ( ) type Options struct { - options []*Option + options []*option } func NewOptions() *Options { return new(Options) } -func (self *Options) AddFlagGroup(shortName rune, longName, argDescription, description string) *FlagGroup { +func (o *Options) AddFlagGroup(shortName rune, longName, argDescription, description string) *FlagGroup { grp := new(FlagGroup) - opt := &Option{shortName, longName, argDescription, description, grp} - self.options = append(self.options, opt) + opt := &option{shortName, longName, argDescription, description, grp} + o.options = append(o.options, opt) return grp } -func (self *Options) AddFlagVar(shortName rune, longName string, pflag *bool, defval bool, description string) { +func (o *Options) AddFlagVar(shortName rune, longName string, pflag *bool, defval bool, description string) { *pflag = defval - opt := &Option{shortName, longName, "", description, pflag} - self.options = append(self.options, opt) + opt := &option{shortName, longName, "", description, pflag} + o.options = append(o.options, opt) } -func (self *Options) Parse(args []string) (remainingArgs []string, err error) { - defer func() { - if r := recover(); r != nil { - if rerr, ok := r.(OptErr); ok { - err = OptErr(args[0] + ": " + string(rerr)) - } else { - panic(r) - } - } - }() - - for i := 1; i < len(args); i++ { +func (o *Options) Parse(args []string) (remainingArgs []string, err error) { + var skip int + for i := 1; i < len(args) && err == nil; i++ { arg := args[i] switch { case arg == "--": - return append(remainingArgs, args[i+1:]...), nil + return append(remainingArgs, args[i+1:]...), err case hasPrefix(arg, "--"): - i += self.parseLongOption(args, i, arg[2:]) + skip, err = o.parseLongOption(args, i, arg[2:]) + i += skip case hasPrefix(arg, "-"): - i += self.parseShortOptions(args, i, arg[1:]) + skip, err = o.parseShortOptions(args, i, arg[1:]) + i += skip default: remainingArgs = append(remainingArgs, arg) } } + if err != nil { + err = optErr(args[0] + ": " + err.Error()) + } return } -func (self *Options) parseLongOption(args []string, i int, argRest string) int { +func (o *Options) parseLongOption(args []string, i int, argRest string) (skip int, err error) { parts := strings.SplitN(argRest, "=", 2) argname := parts[0] var argval *string @@ -66,41 +62,58 @@ func (self *Options) parseLongOption(args []string, i int, argRest string) int { argval = &parts[1] } - for _, opt := range self.options { + for _, opt := range o.options { if argname == opt.longName { - switch data := opt.data.(type) { - case *bool: - if argval == nil { - *data = true - } else { - switch *argval { - case "true", "on", "enabled", "1": - *data = true - case "false", "off", "disabled", "0": - *data = false - default: - panic(OptErr("invalid argument for option --" + opt.longName)) - } - } - return 0 - case *FlagGroup: - if argval == nil { - data.parse("--"+opt.longName+"=", args[i+1]) - return 1 - } else { - data.parse("--"+opt.longName+"=", *argval) - return 0 - } + return o.handleLongOption(args, i, opt, argval) + } + } + + var prefixOpt *option + for _, opt := range o.options { + if strings.HasPrefix(opt.longName, argname) { + if prefixOpt == nil { + prefixOpt = opt + } else { + return 0, optErr(fmt.Sprintf("ambiguous option: --%s could mean --%s or --%s", argRest, prefixOpt.longName, opt.longName)) } } } - panic(OptErr("unknown option: --" + argRest)) + if prefixOpt != nil { + return o.handleLongOption(args, i, prefixOpt, argval) + } + return 0, optErr("unknown option: --" + argRest) +} + +func (o *Options) handleLongOption(args []string, i int, opt *option, argval *string) (skip int, err error) { + switch data := opt.data.(type) { + case *bool: + if argval == nil { + *data = true + } else { + switch *argval { + case "true", "on", "enabled", "1": + *data = true + case "false", "off", "disabled", "0": + *data = false + default: + return 0, optErr("invalid argument for option --" + opt.longName) + } + } + return 0, nil + case *FlagGroup: + if argval == nil { + return 1, data.parse("--"+opt.longName+"=", args[i+1]) + } else { + return 0, data.parse("--"+opt.longName+"=", *argval) + } + } + panic("getopt: unknown option type") } -func (self *Options) parseShortOptions(args []string, i int, optchars string) int { +func (o *Options) parseShortOptions(args []string, i int, optchars string) (skip int, err error) { optchar: for ai, optchar := range optchars { - for _, opt := range self.options { + for _, opt := range o.options { if optchar == opt.shortName { switch data := opt.data.(type) { case *bool: @@ -109,28 +122,26 @@ optchar: case *FlagGroup: argarg := optchars[ai+utf8.RuneLen(optchar):] if argarg != "" { - data.parse(sprintf("-%c", optchar), argarg) - return 0 + return 0, data.parse(sprintf("-%c", optchar), argarg) } else { - data.parse(sprintf("-%c", optchar), args[i+1]) - return 1 + return 1, data.parse(sprintf("-%c", optchar), args[i+1]) } } } } - panic(OptErr(sprintf("unknown option: -%c", optchar))) + return 0, optErr(sprintf("unknown option: -%c", optchar)) } - return 0 + return 0, nil } -func (self *Options) Help(out io.Writer, generalUsage string) { +func (o *Options) Help(out io.Writer, generalUsage string) { wr := tabwriter.NewWriter(out, 1, 0, 2, ' ', tabwriter.TabIndent) fmt.Fprintf(wr, "usage: %s\n", generalUsage) fmt.Fprintln(wr) wr.Flush() - for _, opt := range self.options { + for _, opt := range o.options { if opt.argDescription == "" { fmt.Fprintf(wr, " -%c, --%s\t %s\n", opt.shortName, opt.longName, opt.description) @@ -142,7 +153,7 @@ func (self *Options) Help(out io.Writer, generalUsage string) { wr.Flush() hasFlagGroups := false - for _, opt := range self.options { + for _, opt := range o.options { switch flagGroup := opt.data.(type) { case *FlagGroup: hasFlagGroups = true @@ -163,7 +174,7 @@ func (self *Options) Help(out io.Writer, generalUsage string) { } } -type Option struct { +type option struct { shortName rune longName string argDescription string @@ -172,25 +183,25 @@ type Option struct { } type FlagGroup struct { - flags []*GroupFlag + flags []*groupFlag } -func (self *FlagGroup) AddFlagVar(name string, flag *bool, defval bool, help string) { - opt := &GroupFlag{name, flag, help} - self.flags = append(self.flags, opt) +func (fg *FlagGroup) AddFlagVar(name string, flag *bool, defval bool, help string) { + opt := &groupFlag{name, flag, help} + fg.flags = append(fg.flags, opt) *flag = defval } -func (self *FlagGroup) parse(optionPrefix, arg string) { +func (fg *FlagGroup) parse(optionPrefix, arg string) (err error) { argopt: for _, argopt := range strings.Split(arg, ",") { if argopt == "none" || argopt == "all" { - for _, opt := range self.flags { + for _, opt := range fg.flags { *opt.value = argopt == "all" } continue argopt } - for _, opt := range self.flags { + for _, opt := range fg.flags { if argopt == opt.name { *opt.value = true continue argopt @@ -200,18 +211,19 @@ argopt: continue argopt } } - panic(OptErr("unknown option: " + optionPrefix + argopt)) + return optErr("unknown option: " + optionPrefix + argopt) } + return nil } -type GroupFlag struct { +type groupFlag struct { name string value *bool help string } -type OptErr string +type optErr string -func (err OptErr) Error() string { +func (err optErr) Error() string { return string(err) } diff --git a/pkgtools/pkglint/files/getopt_test.go b/pkgtools/pkglint/files/getopt_test.go index ca8e569c834..52e7f7fceeb 100644 --- a/pkgtools/pkglint/files/getopt_test.go +++ b/pkgtools/pkglint/files/getopt_test.go @@ -32,7 +32,7 @@ func (s *Suite) TestGetopt_UnknownLong(c *check.C) { c.Check(err.Error(), equals, "progname: unknown option: --unknown-long") } -func (s *Suite) TestGetopt_UnknownFlag(c *check.C) { +func (s *Suite) TestGetopt_UnknownFlagInGroup(c *check.C) { opts := NewOptions() opts.AddFlagGroup('W', "warnings", "", "") @@ -45,6 +45,32 @@ func (s *Suite) TestGetopt_UnknownFlag(c *check.C) { c.Check(err.Error(), equals, "progname: unknown option: --warnings=no-error") } +func (s *Suite) TestGetopt_AbbreviatedLong(c *check.C) { + opts := NewOptions() + var longFlag, longerFlag bool + opts.AddFlagVar('?', "long", &longFlag, false, "") + opts.AddFlagVar('?', "longer", &longerFlag, false, "") + + _, err := opts.Parse([]string{"progname", "--lo"}) + + c.Check(err.Error(), equals, "progname: ambiguous option: --lo could mean --long or --longer") + + args, err := opts.Parse([]string{"progname", "--long"}) + + c.Assert(err, check.IsNil) + c.Check(args, check.IsNil) + c.Check(longFlag, equals, true) + c.Check(longerFlag, equals, false) + + longFlag = false + args, err = opts.Parse([]string{"progname", "--longe"}) + + c.Assert(err, check.IsNil) + c.Check(args, check.IsNil) + c.Check(longFlag, equals, false) + c.Check(longerFlag, equals, true) +} + func (s *Suite) TestGetopt_MixedArgsAndOptions(c *check.C) { opts := NewOptions() var aflag, bflag bool diff --git a/pkgtools/pkglint/files/globaldata.go b/pkgtools/pkglint/files/globaldata.go index a050bae3fc9..f3557bc5c75 100644 --- a/pkgtools/pkglint/files/globaldata.go +++ b/pkgtools/pkglint/files/globaldata.go @@ -67,8 +67,8 @@ func (gd *GlobalData) Initialize() { gd.deprecated = getDeprecatedVars() } -func (self *GlobalData) loadDistSites() { - fname := self.pkgsrcdir + "/mk/fetch/sites.mk" +func (gd *GlobalData) loadDistSites() { + fname := gd.pkgsrcdir + "/mk/fetch/sites.mk" lines := LoadExistingLines(fname, true) names := make(map[string]bool) @@ -91,25 +91,25 @@ func (self *GlobalData) loadDistSites() { names["MASTER_SITE_LOCAL"] = true _ = G.opts.DebugMisc && debugf(fname, NO_LINES, "Loaded %d MASTER_SITE_* URLs.", len(url2name)) - self.masterSiteUrls = url2name - self.masterSiteVars = names + gd.masterSiteUrls = url2name + gd.masterSiteVars = names } -func (self *GlobalData) loadPkgOptions() { - fname := self.pkgsrcdir + "/mk/defaults/options.description" +func (gd *GlobalData) loadPkgOptions() { + fname := gd.pkgsrcdir + "/mk/defaults/options.description" lines := LoadExistingLines(fname, false) - self.pkgOptions = make(map[string]string) + gd.pkgOptions = make(map[string]string) for _, line := range lines { if m, optname, optdescr := match2(line.text, `^([-0-9a-z_+]+)(?:\s+(.*))?$`); m { - self.pkgOptions[optname] = optdescr + gd.pkgOptions[optname] = optdescr } else { line.fatalf("Unknown line format.") } } } -func (self *GlobalData) loadTools() { +func (gd *GlobalData) loadTools() { toolFiles := []string{"defaults.mk"} { fname := G.globalData.pkgsrcdir + "/mk/tools/bsd.tools.mk" @@ -221,19 +221,19 @@ func (self *GlobalData) loadTools() { systemBuildDefs["GAMEOWN"] = true systemBuildDefs["GAMEGRP"] = true - self.tools = tools - self.vartools = vartools - self.predefinedTools = predefinedTools - self.varnameToToolname = varnameToToolname - self.systemBuildDefs = systemBuildDefs - self.toolvarsVarRequired = map[string]bool{ + gd.tools = tools + gd.vartools = vartools + gd.predefinedTools = predefinedTools + gd.varnameToToolname = varnameToToolname + gd.systemBuildDefs = systemBuildDefs + gd.toolvarsVarRequired = map[string]bool{ "ECHO": true, "ECHO_N": true, "FALSE": true, "TEST": true, "TRUE": true, } - self.toolsVarRequired = map[string]bool{ + gd.toolsVarRequired = map[string]bool{ "echo": true, "false": true, "test": true, @@ -277,14 +277,14 @@ func parselinesSuggestedUpdates(lines []*Line) []SuggestedUpdate { return updates } -func (self *GlobalData) loadSuggestedUpdates() { - self.suggestedUpdates = loadSuggestedUpdates(G.globalData.pkgsrcdir + "/doc/TODO") +func (gd *GlobalData) loadSuggestedUpdates() { + gd.suggestedUpdates = loadSuggestedUpdates(G.globalData.pkgsrcdir + "/doc/TODO") if wipFilename := G.globalData.pkgsrcdir + "/wip/TODO"; fileExists(wipFilename) { - self.suggestedWipUpdates = loadSuggestedUpdates(wipFilename) + gd.suggestedWipUpdates = loadSuggestedUpdates(wipFilename) } } -func (self *GlobalData) loadDocChangesFromFile(fname string) []Change { +func (gd *GlobalData) loadDocChangesFromFile(fname string) []Change { lines := LoadExistingLines(fname, false) var changes []Change @@ -317,15 +317,15 @@ func (self *GlobalData) loadDocChangesFromFile(fname string) []Change { return changes } -func (self *GlobalData) getSuggestedPackageUpdates() []SuggestedUpdate { +func (gd *GlobalData) getSuggestedPackageUpdates() []SuggestedUpdate { if G.isWip { - return self.suggestedWipUpdates + return gd.suggestedWipUpdates } else { - return self.suggestedUpdates + return gd.suggestedUpdates } } -func (self *GlobalData) loadDocChanges() { +func (gd *GlobalData) loadDocChanges() { docdir := G.globalData.pkgsrcdir + "/doc" files, err := ioutil.ReadDir(docdir) if err != nil { @@ -341,24 +341,24 @@ func (self *GlobalData) loadDocChanges() { } sort.Strings(fnames) - self.lastChange = make(map[string]*Change) + gd.lastChange = make(map[string]*Change) for _, fname := range fnames { - changes := self.loadDocChangesFromFile(docdir + "/" + fname) + changes := gd.loadDocChangesFromFile(docdir + "/" + fname) for _, change := range changes { c := change - self.lastChange[change.pkgpath] = &c + gd.lastChange[change.pkgpath] = &c } } } -func (self *GlobalData) loadUserDefinedVars() { +func (gd *GlobalData) loadUserDefinedVars() { lines := LoadExistingLines(G.globalData.pkgsrcdir+"/mk/defaults/mk.conf", true) - self.userDefinedVars = make(map[string]*Line) + gd.userDefinedVars = make(map[string]*Line) for _, line := range lines { parselineMk(line) if m, varname, _, _, _ := matchVarassign(line.text); m { - self.userDefinedVars[varname] = line + gd.userDefinedVars[varname] = line } } } diff --git a/pkgtools/pkglint/files/globaldata_test.go b/pkgtools/pkglint/files/globaldata_test.go index 27c384c3bee..bc8cf695246 100644 --- a/pkgtools/pkglint/files/globaldata_test.go +++ b/pkgtools/pkglint/files/globaldata_test.go @@ -8,7 +8,7 @@ func (s *Suite) TestGlobalDataVartypes(c *check.C) { G.globalData.InitVartypes() c.Check(G.globalData.vartypes["BSD_MAKE_ENV"].checker.name, equals, "ShellWord") - c.Check(G.globalData.vartypes["USE_BUILTIN.*"].checker.name, equals, "YesNo_Indirectly") + c.Check(G.globalData.vartypes["USE_BUILTIN.*"].checker.name, equals, "YesNoIndirectly") } func (s *Suite) TestParselinesSuggestedUpdates(c *check.C) { diff --git a/pkgtools/pkglint/files/line.go b/pkgtools/pkglint/files/line.go index 66515957241..ba256fbdc05 100644 --- a/pkgtools/pkglint/files/line.go +++ b/pkgtools/pkglint/files/line.go @@ -43,41 +43,41 @@ func NewLine(fname, linenos, text string, rawLines []*RawLine) *Line { return &Line{fname, linenos, text, rawLines, false, nil, nil, make(map[string]interface{})} } -func (self *Line) rawLines() []*RawLine { - return append(self.before, append(self.raw, self.after...)...) +func (ln *Line) rawLines() []*RawLine { + return append(ln.before, append(ln.raw, ln.after...)...) } -func (self *Line) printSource(out io.Writer) { +func (ln *Line) printSource(out io.Writer) { if G.opts.PrintSource { io.WriteString(out, "\n") - for _, rawLine := range self.rawLines() { + for _, rawLine := range ln.rawLines() { fmt.Fprintf(out, "> %s", rawLine.textnl) } } } -func (self *Line) fatalf(format string, args ...interface{}) bool { - self.printSource(G.logErr) - return fatalf(self.fname, self.lines, format, args...) +func (ln *Line) fatalf(format string, args ...interface{}) bool { + ln.printSource(G.logErr) + return fatalf(ln.fname, ln.lines, format, args...) } -func (self *Line) errorf(format string, args ...interface{}) bool { - self.printSource(G.logOut) - return errorf(self.fname, self.lines, format, args...) +func (ln *Line) errorf(format string, args ...interface{}) bool { + ln.printSource(G.logOut) + return errorf(ln.fname, ln.lines, format, args...) } -func (self *Line) warnf(format string, args ...interface{}) bool { - self.printSource(G.logOut) - return warnf(self.fname, self.lines, format, args...) +func (ln *Line) warnf(format string, args ...interface{}) bool { + ln.printSource(G.logOut) + return warnf(ln.fname, ln.lines, format, args...) } -func (self *Line) notef(format string, args ...interface{}) bool { - self.printSource(G.logOut) - return notef(self.fname, self.lines, format, args...) +func (ln *Line) notef(format string, args ...interface{}) bool { + ln.printSource(G.logOut) + return notef(ln.fname, ln.lines, format, args...) } -func (self *Line) debugf(format string, args ...interface{}) bool { - self.printSource(G.logOut) - return debugf(self.fname, self.lines, format, args...) +func (ln *Line) debugf(format string, args ...interface{}) bool { + ln.printSource(G.logOut) + return debugf(ln.fname, ln.lines, format, args...) } -func (self *Line) explain(explanation ...string) { +func (ln *Line) explain(explanation ...string) { if G.opts.Explain { complete := strings.Join(explanation, "\n") if G.explanationsGiven[complete] { @@ -97,41 +97,41 @@ func (self *Line) explain(explanation ...string) { G.explanationsAvailable = true } -func (self *Line) String() string { - return self.fname + ":" + self.lines + ": " + self.text +func (ln *Line) String() string { + return ln.fname + ":" + ln.lines + ": " + ln.text } -func (self *Line) insertBefore(line string) { - self.before = append(self.before, &RawLine{0, line + "\n"}) - self.noteAutofix("Autofix: inserting a line %q before this line.", line) +func (ln *Line) insertBefore(line string) { + ln.before = append(ln.before, &RawLine{0, line + "\n"}) + ln.noteAutofix("Autofix: inserting a line %q before this line.", line) } -func (self *Line) insertAfter(line string) { - self.after = append(self.after, &RawLine{0, line + "\n"}) - self.noteAutofix("Autofix: inserting a line %q after this line.", line) +func (ln *Line) insertAfter(line string) { + ln.after = append(ln.after, &RawLine{0, line + "\n"}) + ln.noteAutofix("Autofix: inserting a line %q after this line.", line) } -func (self *Line) delete() { - self.raw = nil - self.changed = true +func (ln *Line) delete() { + ln.raw = nil + ln.changed = true } -func (self *Line) replace(from, to string) { - for _, rawLine := range self.raw { +func (ln *Line) replace(from, to string) { + for _, rawLine := range ln.raw { if rawLine.lineno != 0 { if replaced := strings.Replace(rawLine.textnl, from, to, 1); replaced != rawLine.textnl { rawLine.textnl = replaced - self.noteAutofix("Autofix: replacing %q with %q.", from, to) + ln.noteAutofix("Autofix: replacing %q with %q.", from, to) } } } } -func (self *Line) replaceRegex(from, to string) { - for _, rawLine := range self.raw { +func (ln *Line) replaceRegex(from, to string) { + for _, rawLine := range ln.raw { if rawLine.lineno != 0 { if replaced := regcomp(from).ReplaceAllString(rawLine.textnl, to); replaced != rawLine.textnl { rawLine.textnl = replaced - self.noteAutofix("Autofix: replacing regular expression %q with %q.", from, to) + ln.noteAutofix("Autofix: replacing regular expression %q with %q.", from, to) } } } diff --git a/pkgtools/pkglint/files/main.go b/pkgtools/pkglint/files/main.go index 9df44e6c91f..28005ae658c 100644 --- a/pkgtools/pkglint/files/main.go +++ b/pkgtools/pkglint/files/main.go @@ -42,7 +42,7 @@ func (p *Pkglint) Main(args ...string) (exitcode int) { if G.opts.Profiling { f, err := os.Create("pkglint.pprof") if err != nil { - fatalf(NO_FILE, NO_LINES, "Cannot create profiling file: %s", err) + dummyLine.fatalf("Cannot create profiling file: %s", err) } pprof.StartCPUProfile(f) defer pprof.StopCPUProfile() diff --git a/pkgtools/pkglint/files/makefiles.go b/pkgtools/pkglint/files/makefiles.go index 1327d53523a..49c991fd03d 100644 --- a/pkgtools/pkglint/files/makefiles.go +++ b/pkgtools/pkglint/files/makefiles.go @@ -450,7 +450,7 @@ func ChecklinesMk(lines []*Line) { } } else if directive == "if" || directive == "elif" { - checklineMkCondition(line, args) + checklineMkIf(line, args) } else if directive == "ifdef" || directive == "ifndef" { if matches(args, `\s`) { diff --git a/pkgtools/pkglint/files/mkcond.go b/pkgtools/pkglint/files/mkcond.go index 6e4b28b7e60..4d4b869c196 100644 --- a/pkgtools/pkglint/files/mkcond.go +++ b/pkgtools/pkglint/files/mkcond.go @@ -29,7 +29,7 @@ func parseMkCond(line *Line, cond string) *Tree { return NewTree("unknown", cond) } -func checklineMkCondition(line *Line, condition string) { +func checklineMkIf(line *Line, condition string) { defer tracecall("checklineMkCond", condition)() tree := parseMkCond(line, condition) diff --git a/pkgtools/pkglint/files/mkcond_test.go b/pkgtools/pkglint/files/mkcond_test.go index 7741e7c6fee..1af382a7b26 100644 --- a/pkgtools/pkglint/files/mkcond_test.go +++ b/pkgtools/pkglint/files/mkcond_test.go @@ -25,17 +25,17 @@ func (s *Suite) TestChecklineMkCondition(c *check.C) { G.globalData.InitVartypes() line := NewLine("fname", "1", "", nil) - checklineMkCondition(line, "!empty(PKGSRC_COMPILER:Mmycc)") + checklineMkIf(line, "!empty(PKGSRC_COMPILER:Mmycc)") c.Check(s.Stdout(), equals, "WARN: fname:1: Invalid :M value \"mycc\". "+ "Only { ccache ccc clang distcc f2c gcc hp icc ido gcc mipspro "+ "mipspro-ucode pcc sunpro xlc } are allowed.\n") - checklineMkCondition(line, "${A} != ${B}") + checklineMkIf(line, "${A} != ${B}") c.Check(s.Stdout(), equals, "") // Unknown condition types are silently ignored - checklineMkCondition(line, "${HOMEPAGE} == \"mailto:someone@example.org\"") + checklineMkIf(line, "${HOMEPAGE} == \"mailto:someone@example.org\"") c.Check(s.Output(), equals, "WARN: fname:1: \"mailto:someone@example.org\" is not a valid URL.\n") } diff --git a/pkgtools/pkglint/files/patches.go b/pkgtools/pkglint/files/patches.go index b081ab871fd..86f454256a8 100644 --- a/pkgtools/pkglint/files/patches.go +++ b/pkgtools/pkglint/files/patches.go @@ -111,7 +111,6 @@ func checklineOtherAbsolutePathname(line *Line, text string) { } const ( - rePatchRcsid = `^\$.*\$$` rePatchNonempty = `^(.+)$` rePatchEmpty = `^$` rePatchTextError = `\*\*\* Error code` diff --git a/pkgtools/pkglint/files/pkglint.go b/pkgtools/pkglint/files/pkglint.go index bdee9038c5c..b188e129472 100644 --- a/pkgtools/pkglint/files/pkglint.go +++ b/pkgtools/pkglint/files/pkglint.go @@ -15,7 +15,6 @@ const ( reMkCond = `^\.(\s*)(if|ifdef|ifndef|else|elif|endif|for|endfor|undef)(?:\s+([^\s#][^#]*?))?\s*(?:#.*)?$` reMkInclude = `^\.\s*(s?include)\s+\"([^\"]+)\"\s*(?:#.*)?$` reVarassign = `^ *([-*+A-Z_a-z0-9.${}\[]+?)\s*([!+:?]?=)\s*((?:\\#|[^#])*?)(?:\s*(#.*))?$` - reVarname = `(?:[-*+.0-9A-Z_a-z{}\[]+|\$\{[\w_]+\})+` rePkgname = `^([\w\-.+]+)-(\d(?:\w|\.\d)*)$` rePkgbase = `(?:[+.\w]|-[A-Z_a-z])+` rePkgversion = `\d(?:\w|\.\d)*` diff --git a/pkgtools/pkglint/files/pkglint_test.go b/pkgtools/pkglint/files/pkglint_test.go index 7d48cfef671..84f24727f86 100644 --- a/pkgtools/pkglint/files/pkglint_test.go +++ b/pkgtools/pkglint/files/pkglint_test.go @@ -83,15 +83,6 @@ func (s *Suite) TestResolveVariableRefs_SpecialChars(c *check.C) { c.Check(resolved, equals, "gst-plugins0.10-x11/distinfo") } -func (s *Suite) TestReVarname(c *check.C) { - re := `^(?:` + reVarname + `)$` - - c.Check(matches("", re), equals, false) - c.Check(matches("VARNAME", re), equals, true) - c.Check(matches("0FLAGS", re), equals, true) - c.Check(matches("CFLAGS.${prog}.DEBUG", re), equals, true) -} - func (s *Suite) TestChecklineRcsid(c *check.C) { lines := s.NewLines("fname", "$"+"NetBSD: dummy $", diff --git a/pkgtools/pkglint/files/plist_test.go b/pkgtools/pkglint/files/plist_test.go index 7c2bfee093e..a910a8fd1a9 100644 --- a/pkgtools/pkglint/files/plist_test.go +++ b/pkgtools/pkglint/files/plist_test.go @@ -15,7 +15,7 @@ func (s *Suite) TestChecklinesPlist(c *check.C) { checklinesPlist(lines) c.Check(s.Output(), equals, ""+ - "ERROR: PLIST:1: Expected \"@comment $NetBSD: plist_test.go,v 1.2 2015/12/02 21:46:46 rillig Exp $\".\n"+ + "ERROR: PLIST:1: Expected \"@comment $"+"NetBSD$\".\n"+ "WARN: PLIST:1: The bin/ directory should not have subdirectories.\n"+ "WARN: PLIST:5: Please remove this line. It is no longer necessary.\n") } diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go index 1db763e070c..8b47bdfd9fe 100644 --- a/pkgtools/pkglint/files/shell_test.go +++ b/pkgtools/pkglint/files/shell_test.go @@ -19,10 +19,69 @@ func (s *Suite) TestSplitIntoShellwords_LineContinuation(c *check.C) { } func (s *Suite) TestChecklineMkShelltext(c *check.C) { + s.UseCommandLine(c, "-Wall") G.mkContext = newMkContext() - line := NewLine("fname", "1", "dummy", nil) + msline := NewMkShellLine(NewLine("fname", "1", "dummy", nil)) + + msline.checklineMkShelltext("@# Comment") + + c.Check(s.Output(), equals, "") + + msline.checklineMkShelltext("uname=`uname`; echo $$uname") + + c.Check(s.Output(), equals, ""+ + "WARN: fname:1: Unknown shell command \"uname\".\n"+ + "WARN: fname:1: Please switch to \"set -e\" mode before using a semicolon to separate commands.\n"+ + "WARN: fname:1: Unknown shell command \"echo\".\n"+ + "WARN: fname:1: Unquoted shell variable \"uname\".\n") + + // The following test case goes beyond the limits of the current shell parser. + + // foobar="`echo \"foo bar\"`" + msline.checklineMkShelltext("foobar=\"`echo \\\"foo bar\\\"`\"") + + c.Check(s.Output(), equals, ""+ + "WARN: fname:1: Backslashes should be doubled inside backticks.\n"+ + "WARN: fname:1: Double quotes inside backticks inside double quotes are error prone.\n"+ + "WARN: fname:1: Backslashes should be doubled inside backticks.\n"+ + "WARN: fname:1: Double quotes inside backticks inside double quotes are error prone.\n"+ + "WARN: fname:1: Unknown shell command \"echo\".\n"+ + "ERROR: fname:1: Internal pkglint error: checklineMkShellword state=plain, rest=\"\\\\foo\", shellword=\"\\\\foo\"\n"+ + "ERROR: fname:1: Internal pkglint error: checklineMkShelltext state=continuation rest=\"\\\\\" shellword=\"echo \\\\foo bar\\\\\"\n") + + G.globalData.tools = map[string]bool{"echo": true} + G.globalData.predefinedTools = map[string]bool{"echo": true} + G.mkContext = newMkContext() + G.globalData.InitVartypes() + + msline.checklineMkShelltext("echo ${PKGNAME:Q}") // VUC_SHW_PLAIN + + c.Check(s.Output(), equals, ""+ + "WARN: fname:1: PKGNAME may not be used in this file.\n"+ + "NOTE: fname:1: The :Q operator isn't necessary for ${PKGNAME} here.\n") - NewMkShellLine(line).checklineMkShelltext("@# Comment") + msline.checklineMkShelltext("echo \"${CFLAGS:Q}\"") // VUC_SHW_DQUOT + + c.Check(s.Output(), equals, ""+ + "WARN: fname:1: Please don't use the :Q operator in double quotes.\n"+ + "WARN: fname:1: CFLAGS may not be used in this file.\n"+ + "WARN: fname:1: Please use ${CFLAGS:M*:Q} instead of ${CFLAGS:Q} and make sure the variable appears outside of any quoting characters.\n") + + msline.checklineMkShelltext("echo '${COMMENT:Q}'") // VUC_SHW_SQUOT + + c.Check(s.Output(), equals, "WARN: fname:1: COMMENT may not be used in this file.\n") + + msline.checklineMkShelltext("echo $$@") + + c.Check(s.Output(), equals, "WARN: fname:1: The $@ shell variable should only be used in double quotes.\n") + + msline.checklineMkShelltext("echo \"$$\"") // As seen by make(1); the shell sees: echo $ + + c.Check(s.Output(), equals, "WARN: fname:1: Unquoted $ or strange shell variable found.\n") + + msline.checklineMkShelltext("echo \"\\n\"") // As seen by make(1); the shell sees: echo "\n" + + c.Check(s.Output(), equals, "WARN: fname:1: Please use \"\\\\n\" instead of \"\\n\".\n") } func (s *Suite) TestChecklineMkShellword(c *check.C) { @@ -30,7 +89,6 @@ func (s *Suite) TestChecklineMkShellword(c *check.C) { G.globalData.InitVartypes() line := NewLine("fname", "1", "dummy", nil) - c.Check(matches("${list}", `^`+reVarname+`$`), equals, true) c.Check(matches("${list}", `^`+reVarnameDirect+`$`), equals, false) checklineMkShellword(line, "${${list}}", false) @@ -56,3 +114,35 @@ func (s *Suite) TestShelltextContext_CheckCommandStart(c *check.C) { "WARN: fname:3: The \"echo\" tool is used but not added to USE_TOOLS.\n"+ "WARN: fname:3: Please use \"${ECHO}\" instead of \"echo\".\n") } + +func (s *Suite) TestMkShellLine_checklineMkShelltext(c *check.C) { + + shline := NewMkShellLine(s.DummyLine()) + + shline.checklineMkShelltext("for f in *.pl; do ${SED} s,@PREFIX@,${PREFIX}, < $f > $f.tmp && ${MV} $f.tmp $f; done") + + c.Check(s.Output(), equals, "NOTE: fname:1: Please use the SUBST framework instead of ${SED} and ${MV}.\n") + + shline.checklineMkShelltext("install -c manpage.1 ${PREFIX}/man/man1/manpage.1") + + c.Check(s.Output(), equals, "WARN: fname:1: Please use ${PKGMANDIR} instead of \"man\".\n") + + shline.checklineMkShelltext("cp init-script ${PREFIX}/etc/rc.d/service") + + c.Check(s.Output(), equals, "WARN: fname:1: Please use the RCD_SCRIPTS mechanism to install rc.d scripts automatically to ${RCD_SCRIPTS_EXAMPLEDIR}.\n") +} + +func (s *Suite) TestMkShellLine_checkCommandUse(c *check.C) { + G.mkContext = newMkContext() + G.mkContext.target = "do-install" + + shline := NewMkShellLine(s.DummyLine()) + + shline.checkCommandUse("sed") + + c.Check(s.Output(), equals, "WARN: fname:1: The shell command \"sed\" should not be used in the install phase.\n") + + shline.checkCommandUse("cp") + + c.Check(s.Output(), equals, "WARN: fname:1: ${CP} should not be used to install files.\n") +} diff --git a/pkgtools/pkglint/files/substcontext.go b/pkgtools/pkglint/files/substcontext.go index cdeef42819c..d442ec5ca82 100644 --- a/pkgtools/pkglint/files/substcontext.go +++ b/pkgtools/pkglint/files/substcontext.go @@ -12,7 +12,7 @@ type SubstContext struct { filterCmd string } -func (self *SubstContext) Varassign(line *Line, varname, op, value string) { +func (ctx *SubstContext) Varassign(line *Line, varname, op, value string) { if !G.opts.WarnExtra { return } @@ -22,54 +22,54 @@ func (self *SubstContext) Varassign(line *Line, varname, op, value string) { if len(classes) > 1 { line.warnf("Please add only one class at a time to SUBST_CLASSES.") } - if self.id != "" { + if ctx.id != "" { line.warnf("SUBST_CLASSES should only appear once in a SUBST block.") } - self.id = classes[0] + ctx.id = classes[0] return } m, varbase, varparam := match2(varname, `^(SUBST_(?:STAGE|MESSAGE|FILES|SED|VARS|FILTER_CMD))\.([\-\w_]+)$`) if !m { - if self.id != "" { + if ctx.id != "" { line.warnf("Foreign variable %q in SUBST block.", varname) } return } - if self.id == "" { + if ctx.id == "" { line.warnf("SUBST_CLASSES should come before the definition of %q.", varname) - self.id = varparam + ctx.id = varparam } - if varparam != self.id { - if self.IsComplete() { + if varparam != ctx.id { + if ctx.IsComplete() { // XXX: This code sometimes produces weird warnings. See // meta-pkgs/xorg/Makefile.common 1.41 for an example. - self.Finish(line) + ctx.Finish(line) // The following assignment prevents an additional warning, // but from a technically viewpoint, it is incorrect. - self.id = varparam + ctx.id = varparam } else { - line.warnf("Variable %q does not match SUBST class %q.", varname, self.id) + line.warnf("Variable %q does not match SUBST class %q.", varname, ctx.id) } return } switch varbase { case "SUBST_STAGE": - self.dup(line, &self.stage, varname, value) + ctx.dup(line, &ctx.stage, varname, value) case "SUBST_MESSAGE": - self.dup(line, &self.message, varname, value) + ctx.dup(line, &ctx.message, varname, value) case "SUBST_FILES": - self.duplist(line, &self.files, varname, op, value) + ctx.duplist(line, &ctx.files, varname, op, value) case "SUBST_SED": - self.duplist(line, &self.sed, varname, op, value) + ctx.duplist(line, &ctx.sed, varname, op, value) case "SUBST_FILTER_CMD": - self.dup(line, &self.filterCmd, varname, value) + ctx.dup(line, &ctx.filterCmd, varname, value) case "SUBST_VARS": - self.duplist(line, &self.vars, varname, op, value) + ctx.duplist(line, &ctx.vars, varname, op, value) default: line.warnf("Foreign variable %q in SUBST block.", varname) } @@ -82,27 +82,27 @@ func (ctx *SubstContext) IsComplete() bool { (len(ctx.sed) != 0 || len(ctx.vars) != 0 || ctx.filterCmd != "") } -func (self *SubstContext) Finish(line *Line) { - if self.id == "" || !G.opts.WarnExtra { +func (ctx *SubstContext) Finish(line *Line) { + if ctx.id == "" || !G.opts.WarnExtra { return } - if self.stage == "" { - line.warnf("Incomplete SUBST block: %s missing.", self.varname("SUBST_STAGE")) + if ctx.stage == "" { + line.warnf("Incomplete SUBST block: %s missing.", ctx.varname("SUBST_STAGE")) } - if len(self.files) == 0 { - line.warnf("Incomplete SUBST block: %s missing.", self.varname("SUBST_FILES")) + if len(ctx.files) == 0 { + line.warnf("Incomplete SUBST block: %s missing.", ctx.varname("SUBST_FILES")) } - if len(self.sed) == 0 && len(self.vars) == 0 && self.filterCmd == "" { + if len(ctx.sed) == 0 && len(ctx.vars) == 0 && ctx.filterCmd == "" { line.warnf("Incomplete SUBST block: %s, %s or %s missing.", - self.varname("SUBST_SED"), self.varname("SUBST_VARS"), self.varname("SUBST_FILTER_CMD")) + ctx.varname("SUBST_SED"), ctx.varname("SUBST_VARS"), ctx.varname("SUBST_FILTER_CMD")) } - self.id = "" - self.stage = "" - self.message = "" - self.files = nil - self.sed = nil - self.vars = nil - self.filterCmd = "" + ctx.id = "" + ctx.stage = "" + ctx.message = "" + ctx.files = nil + ctx.sed = nil + ctx.vars = nil + ctx.filterCmd = "" } func (ctx *SubstContext) varname(varbase string) string { diff --git a/pkgtools/pkglint/files/toplevel.go b/pkgtools/pkglint/files/toplevel.go index dd481c7ac53..de34c1c76a3 100644 --- a/pkgtools/pkglint/files/toplevel.go +++ b/pkgtools/pkglint/files/toplevel.go @@ -18,9 +18,6 @@ func checkdirToplevel() { } ParselinesMk(lines) - if 0 < len(lines) { - checklineRcsid(lines[0], `#\s+`, "# ") - } for _, line := range lines { if m, commentedOut, indentation, subdir, comment := match4(line.text, `^(#?)SUBDIR\s*\+=(\s*)(\S+)\s*(?:#\s*(.*?)\s*|)$`); m { @@ -40,7 +37,7 @@ func checkdirToplevel() { func (ctx *Toplevel) checkSubdir(line *Line, commentedOut bool, indentation, subdir, comment string) { if commentedOut && comment == "" { - line.warnf("%s commented out without giving a reason.", subdir) + line.warnf("%q commented out without giving a reason.", subdir) } if indentation != "\t" { diff --git a/pkgtools/pkglint/files/toplevel_test.go b/pkgtools/pkglint/files/toplevel_test.go new file mode 100644 index 00000000000..f232369fcd2 --- /dev/null +++ b/pkgtools/pkglint/files/toplevel_test.go @@ -0,0 +1,32 @@ +package main + +import ( + check "gopkg.in/check.v1" +) + +func (s *Suite) TestCheckdirToplevel(c *check.C) { + s.CreateTmpFile(c, "Makefile", ""+ + "# $"+"NetBSD$\n"+ + "\n"+ + "SUBDIR+= x11\n"+ + "SUBDIR+=\tarchivers\n"+ + "SUBDIR+=\tccc\n"+ + "SUBDIR+=\tccc\n"+ + "#SUBDIR+=\tignoreme\n"+ + "SUBDIR+=\tnonexisting\n"+ // This just doesn’t happen in practice. + "SUBDIR+=\tbbb\n") + s.CreateTmpFile(c, "archivers/Makefile", "") + s.CreateTmpFile(c, "bbb/Makefile", "") + s.CreateTmpFile(c, "ccc/Makefile", "") + s.CreateTmpFile(c, "x11/Makefile", "") + G.globalData.InitVartypes() + G.currentDir = s.tmpdir + + checkdirToplevel() + + c.Check(s.Output(), equals, ""+ + "WARN: "+s.tmpdir+"/Makefile:3: Indentation should be a single tab character.\n"+ + "ERROR: "+s.tmpdir+"/Makefile:6: Each subdir must only appear once.\n"+ + "WARN: "+s.tmpdir+"/Makefile:7: \"ignoreme\" commented out without giving a reason.\n"+ + "WARN: "+s.tmpdir+"/Makefile:9: bbb should come before ccc\n") +} diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go index f608d880b20..bd11f08ca96 100644 --- a/pkgtools/pkglint/files/util.go +++ b/pkgtools/pkglint/files/util.go @@ -21,13 +21,6 @@ var ( hasSuffix = strings.HasSuffix ) -func intMax(a, b int) int { - if a > b { - return a - } - return b -} - func ifelseStr(cond bool, a, b string) string { if cond { return a @@ -175,14 +168,6 @@ func dirExists(fname string) bool { return err == nil && st.Mode().IsDir() } -func stringset(s string) map[string]bool { - result := make(map[string]bool) - for _, m := range splitOnSpace(strings.TrimSpace(s)) { - result[m] = true - } - return result -} - var res = make(map[string]*regexp.Regexp) func regcomp(re string) *regexp.Regexp { @@ -282,13 +267,6 @@ func replacePrefix(ps *string, pm *[]string, re string) bool { return false } -func nilToZero(pi *int) int { - if pi != nil { - return *pi - } - return 0 -} - // Useful in combination with regex.Find*Index func negToZero(i int) int { if i >= 0 { diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go index f279a4c5ea8..ec857f1e0f1 100644 --- a/pkgtools/pkglint/files/util_test.go +++ b/pkgtools/pkglint/files/util_test.go @@ -58,3 +58,23 @@ func (s *Suite) TestCleanpath(c *check.C) { c.Check(cleanpath("cat/pkg.v1/../../cat/pkg.v2/Makefile"), equals, "cat/pkg.v1/../../cat/pkg.v2/Makefile") c.Check(cleanpath("dir/"), equals, "dir") } + +func (s *Suite) TestIsEmptyDirAndGetSubdirs(c *check.C) { + s.CreateTmpFile(c, "CVS/Entries", "dummy\n") + + c.Check(isEmptyDir(s.tmpdir), equals, true) + c.Check(getSubdirs(s.tmpdir), check.DeepEquals, []string(nil)) + + s.CreateTmpFile(c, "somedir/file", "") + + c.Check(isEmptyDir(s.tmpdir), equals, false) + c.Check(getSubdirs(s.tmpdir), check.DeepEquals, []string{"somedir"}) + + if nodir := s.tmpdir + "/nonexistent"; true { + c.Check(isEmptyDir(nodir), equals, true) // Counts as empty. + defer s.ExpectFatalError(func() { + c.Check(s.Output(), equals, "FATAL: "+nodir+": Cannot be read: open "+nodir+": The system cannot find the file specified.\n") + }) + c.Check(getSubdirs(nodir), check.DeepEquals, []string(nil)) + } +} diff --git a/pkgtools/pkglint/files/vardefs.go b/pkgtools/pkglint/files/vardefs.go index e0300ecdd4e..ac2eaa4b538 100644 --- a/pkgtools/pkglint/files/vardefs.go +++ b/pkgtools/pkglint/files/vardefs.go @@ -357,7 +357,7 @@ func (gd *GlobalData) InitVartypes() { acl("INSTALL_TEMPLATES", LK_SHELL, CheckvarPathname, "Makefile:as", "Makefile.common:ads") acl("INSTALL_UNSTRIPPED", LK_NONE, CheckvarYesNo, "Makefile:s", "Makefile.common:s") pkg("INTERACTIVE_STAGE", LK_SHELL, enum("fetch extract configure build install")) - acl("IS_BUILTIN.*", LK_NONE, CheckvarYesNo_Indirectly, "builtin.mk:psu") + acl("IS_BUILTIN.*", LK_NONE, CheckvarYesNoIndirectly, "builtin.mk:psu") sys("JAVA_BINPREFIX", LK_NONE, CheckvarPathname) pkg("JAVA_CLASSPATH", LK_NONE, CheckvarShellWord) pkg("JAVA_HOME", LK_NONE, CheckvarPathname) @@ -658,7 +658,7 @@ func (gd *GlobalData) InitVartypes() { pkglist("UNWRAP_FILES", LK_SHELL, CheckvarPathmask) usr("UPDATE_TARGET", LK_SHELL, CheckvarIdentifier) pkg("USE_BSD_MAKEFILE", LK_NONE, CheckvarYes) - acl("USE_BUILTIN.*", LK_NONE, CheckvarYesNo_Indirectly, "builtin.mk:s") + acl("USE_BUILTIN.*", LK_NONE, CheckvarYesNoIndirectly, "builtin.mk:s") pkg("USE_CMAKE", LK_NONE, CheckvarYes) acl("USE_CROSSBASE", LK_NONE, CheckvarYes, "Makefile:s") pkg("USE_FEATURES", LK_SHELL, CheckvarIdentifier) diff --git a/pkgtools/pkglint/files/vars.go b/pkgtools/pkglint/files/vars.go index cd58ef77ca7..0829e461247 100644 --- a/pkgtools/pkglint/files/vars.go +++ b/pkgtools/pkglint/files/vars.go @@ -17,7 +17,7 @@ func variableNeedsQuoting(line *Line, varname string, vuc *VarUseContext) NeedsQ return NQ_DONT_KNOW } - cond := vartype.checker.IsEnum() + isPlainWord := vartype.checker.IsEnum() switch vartype.checker.name { case "DistSuffix", "FileMode", "Filename", @@ -28,9 +28,9 @@ func variableNeedsQuoting(line *Line, varname string, vuc *VarUseContext) NeedsQ "UserGroupName", "Varname", "Version", "WrkdirSubdirectory": - cond = true + isPlainWord = true } - if cond { + if isPlainWord { if vartype.kindOfList == LK_NONE { return NQ_DOESNT_MATTER } diff --git a/pkgtools/pkglint/files/vars_test.go b/pkgtools/pkglint/files/vars_test.go new file mode 100644 index 00000000000..c98bfbb9366 --- /dev/null +++ b/pkgtools/pkglint/files/vars_test.go @@ -0,0 +1,17 @@ +package main + +import ( + check "gopkg.in/check.v1" +) + +func (s *Suite) TestVariableNeedsQuoting(c *check.C) { + line := s.DummyLine() + G.globalData.InitVartypes() + pkgnameType := G.globalData.vartypes["PKGNAME"] + + // In Makefile: PKGNAME := ${UNKNOWN} + vuc := &VarUseContext{VUC_TIME_LOAD, pkgnameType, VUC_SHW_UNKNOWN, VUC_EXTENT_UNKNOWN} + nq := variableNeedsQuoting(line, "UNKNOWN", vuc) + + c.Check(nq, equals, NQ_DONT_KNOW) +} diff --git a/pkgtools/pkglint/files/vartype.go b/pkgtools/pkglint/files/vartype.go index 3ddb1708f81..1b22c897d8e 100644 --- a/pkgtools/pkglint/files/vartype.go +++ b/pkgtools/pkglint/files/vartype.go @@ -35,8 +35,8 @@ const ( ) // The allowed actions in this file, or "?" if unknown. -func (self *Vartype) effectivePermissions(fname string) string { - for _, aclEntry := range self.aclEntries { +func (vt *Vartype) effectivePermissions(fname string) string { + for _, aclEntry := range vt.aclEntries { if m, _ := path.Match(aclEntry.glob, path.Base(fname)); m { return aclEntry.permissions } @@ -68,9 +68,9 @@ func ReadableVartypePermissions(perms string) string { // 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 (self *Vartype) union() string { +func (vt *Vartype) union() string { var permissions string - for _, aclEntry := range self.aclEntries { + for _, aclEntry := range vt.aclEntries { permissions += aclEntry.permissions } return permissions @@ -79,34 +79,34 @@ func (self *Vartype) union() string { // Returns whether the type is considered a shell list. // This distinction between “real lists” and “considered a list” makes // the implementation of checklineMkVartype easier. -func (self *Vartype) isConsideredList() bool { - switch self.kindOfList { +func (vt *Vartype) isConsideredList() bool { + switch vt.kindOfList { case LK_SHELL: return true case LK_SPACE: return false } - switch self.checker { + switch vt.checker { case CheckvarSedCommands, CheckvarShellCommand: return true } return false } -func (self *Vartype) mayBeAppendedTo() bool { - return self.kindOfList != LK_NONE || - self.checker == CheckvarAwkCommand || - self.checker == CheckvarSedCommands +func (vt *Vartype) mayBeAppendedTo() bool { + return vt.kindOfList != LK_NONE || + vt.checker == CheckvarAwkCommand || + vt.checker == CheckvarSedCommands } -func (self *Vartype) String() string { - switch self.kindOfList { +func (vt *Vartype) String() string { + switch vt.kindOfList { case LK_NONE: - return self.checker.name + return vt.checker.name case LK_SPACE: - return "SpaceList of " + self.checker.name + return "SpaceList of " + vt.checker.name case LK_SHELL: - return "ShellList of " + self.checker.name + return "ShellList of " + vt.checker.name default: panic("Unknown list type") } @@ -181,7 +181,7 @@ var ( CheckvarWrksrcSubdirectory = &VarChecker{"WrksrcSubdirectory", (*VartypeCheck).WrksrcSubdirectory} CheckvarYes = &VarChecker{"Yes", (*VartypeCheck).Yes} CheckvarYesNo = &VarChecker{"YesNo", (*VartypeCheck).YesNo} - CheckvarYesNo_Indirectly = &VarChecker{"YesNo_Indirectly", (*VartypeCheck).YesNo_Indirectly} + CheckvarYesNoIndirectly = &VarChecker{"YesNoIndirectly", (*VartypeCheck).YesNoIndirectly} ) func init() { // Necessary due to circular dependency diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go index 603d130d9fb..3d85d645161 100644 --- a/pkgtools/pkglint/files/vartypecheck.go +++ b/pkgtools/pkglint/files/vartypecheck.go @@ -740,7 +740,7 @@ func (cv *VartypeCheck) YesNo() { // Like YesNo, but the value may be produced by a shell command using the // != operator. -func (cv *VartypeCheck) YesNo_Indirectly() { +func (cv *VartypeCheck) YesNoIndirectly() { if cv.valueNovar != "" && !matches(cv.value, `^(?:YES|yes|NO|no)(?:\s+#.*)?$`) { cv.line.warnf("%s should be set to YES, yes, NO, or no.", cv.varname) } diff --git a/pkgtools/pkglint/files/varusecontext.go b/pkgtools/pkglint/files/varusecontext.go index 0a49d74b9f5..f45fd68db2a 100644 --- a/pkgtools/pkglint/files/varusecontext.go +++ b/pkgtools/pkglint/files/varusecontext.go @@ -55,14 +55,14 @@ const ( VUC_EXT_WORDPART // Example: echo LOCALBASE=${LOCALBASE} ) -func (self *VarUseContext) String() string { +func (vuc *VarUseContext) String() string { typename := "no-type" - if self.vartype != nil { - typename = self.vartype.String() + if vuc.vartype != nil { + typename = vuc.vartype.String() } return sprintf("(%s %s %s %s)", - []string{"unknown", "load-time", "run-time"}[self.time], + []string{"unknown", "load-time", "run-time"}[vuc.time], typename, - []string{"unknown", "plain", "dquot", "squot", "backt", "for"}[self.shellword], - []string{"unknown", "word", "word-part"}[self.extent]) + []string{"unknown", "plain", "dquot", "squot", "backt", "for"}[vuc.shellword], + []string{"unknown", "word", "word-part"}[vuc.extent]) } |