diff options
author | rillig <rillig@pkgsrc.org> | 2015-12-05 22:42:01 +0000 |
---|---|---|
committer | rillig <rillig@pkgsrc.org> | 2015-12-05 22:42:01 +0000 |
commit | e3a506130fe63a79216666d59ca8a74a5c4ac0ad (patch) | |
tree | f23a947e33731d07a59197c8bf084aa01b1f5fc6 /pkgtools/pkglint/files | |
parent | c5c4967e2a201b9cc2a8799d51d80ce4f7bc9730 (diff) | |
download | pkgsrc-e3a506130fe63a79216666d59ca8a74a5c4ac0ad.tar.gz |
Code cleanup
Makefile lines are now distinguished from ordinary lines.
Running "pkglint -r -Wall -Call" on the whole pkgsrc tree produces
the same result as before, except for the reporting of internal
pkglint errors, where pkglint doesn't know how to parse certain shell
commands. Therefore no version change.
Diffstat (limited to 'pkgtools/pkglint/files')
-rw-r--r-- | pkgtools/pkglint/files/makefiles.go | 8 | ||||
-rw-r--r-- | pkgtools/pkglint/files/shell.go | 222 | ||||
-rw-r--r-- | pkgtools/pkglint/files/substcontext.go | 11 | ||||
-rw-r--r-- | pkgtools/pkglint/files/substcontext_test.go | 43 | ||||
-rw-r--r-- | pkgtools/pkglint/files/util.go | 17 |
5 files changed, 157 insertions, 144 deletions
diff --git a/pkgtools/pkglint/files/makefiles.go b/pkgtools/pkglint/files/makefiles.go index 74d6e4b1924..0b2c318974f 100644 --- a/pkgtools/pkglint/files/makefiles.go +++ b/pkgtools/pkglint/files/makefiles.go @@ -323,16 +323,16 @@ func ChecklinesMk(lines []*Line) { checklineTrailingWhitespace(line) if line.extra["is_empty"] != nil { - substcontext.Finish(line) + substcontext.Finish(NewMkLine(line)) } else if line.extra["is_comment"] != nil { // No further checks. - } else if m, varname, op, value, _ := matchVarassign(text); m { + } else if line.extra["is_varassign"] != nil { ml := NewMkLine(line) ml.checkVaralign() ml.checkVarassign() - substcontext.Varassign(line, varname, op, value) + substcontext.Varassign(NewMkLine(line)) } else if hasPrefix(text, "\t") { shellcmd := text[1:] @@ -516,7 +516,7 @@ func ChecklinesMk(lines []*Line) { _ = G.opts.DebugMisc && line.debugf("Unknown line format") } } - substcontext.Finish(lines[len(lines)-1]) + substcontext.Finish(NewMkLine(lines[len(lines)-1])) checklinesTrailingEmptyLines(lines) diff --git a/pkgtools/pkglint/files/shell.go b/pkgtools/pkglint/files/shell.go index 9bd79eca233..e8d83fc6ce3 100644 --- a/pkgtools/pkglint/files/shell.go +++ b/pkgtools/pkglint/files/shell.go @@ -5,7 +5,6 @@ package main import ( "path" "strings" - "unicode" ) const ( @@ -81,6 +80,16 @@ func NewMkShellLine(line *Line) *MkShellLine { return &MkShellLine{line} } +type ShellwordState string + +const ( + swstPlain ShellwordState = "plain" + swstSquot ShellwordState = "squot" + swstDquot ShellwordState = "dquot" + swstDquotBackt ShellwordState = "dquot+backt" + swstBackt ShellwordState = "backt" +) + func (msline *MkShellLine) checkShellword(shellword string, checkQuoting bool) { defer tracecall("MkShellLine.checklineMkShellword", shellword, checkQuoting)() @@ -104,78 +113,27 @@ func (msline *MkShellLine) checkShellword(shellword string, checkQuoting bool) { line.warnf("Please use the RCD_SCRIPTS mechanism to install rc.d scripts automatically to ${RCD_SCRIPTS_EXAMPLEDIR}.") } - type ShellwordState string - const ( - swstPlain ShellwordState = "plain" - swstSquot ShellwordState = "squot" - swstDquot ShellwordState = "dquot" - swstDquotBackt ShellwordState = "dquot+backt" - swstBackt ShellwordState = "backt" - ) - - rest := shellword + repl := NewPrefixReplacer(shellword) state := swstPlain outer: - for rest != "" { - _ = G.opts.DebugShell && line.debugf("shell state %s: %q", state, rest) + for repl.rest != "" { + _ = G.opts.DebugShell && line.debugf("shell state %s: %q", state, repl.rest) - var m []string switch { // When parsing inside backticks, it is more // reasonable to check the whole shell command // recursively, instead of splitting off the first // make(1) variable. case state == swstBackt || state == swstDquotBackt: - // Scan for the end of the backticks, checking - // for single backslashes and removing one level - // of backslashes. Backslashes are only removed - // before a dollar, a backslash or a backtick. - // - // References: - // * http://www.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_03 - stripped := "" - for rest != "" { - switch { - case replacePrefix(&rest, &m, "^`"): - if state == swstBackt { - state = swstPlain - } else { - state = swstDquot - } - goto endOfBackticks - - case replacePrefix(&rest, &m, "^\\\\([\\\\`$])"): - stripped += m[1] - - case replacePrefix(&rest, &m, `^(\\)`): - line.warnf("Backslashes should be doubled inside backticks.") - stripped += m[1] - - case state == swstDquotBackt && replacePrefix(&rest, &m, `^"`): - line.warnf("Double quotes inside backticks inside double quotes are error prone.") - line.explain( - "According to the SUSv3, they produce undefined results.", - "", - "See the paragraph starting \"Within the backquoted ...\" in", - "http://www.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html") - - case replacePrefix(&rest, &m, "^([^\\\\`]+)"): - stripped += m[1] - - default: - line.errorf("Internal pkglint error: checklineMkShellword shellword=%q rest=%q", shellword, rest) - } - } - line.errorf("Unfinished backquotes: rest=%q", rest) - - endOfBackticks: - msline.checkShelltext(stripped) + var backtCommand string + backtCommand, state = msline.unescapeBackticks(shellword, repl, state) + msline.checkShelltext(backtCommand) // Make(1) variables have the same syntax, no matter in which state we are currently. - case replacePrefix(&rest, &m, `^\$\{(`+reVarnameDirect+`|@)(:[^\{]+)?\}`), - replacePrefix(&rest, &m, `^\$\((`+reVarnameDirect+`|@])(:[^\)]+)?\)`), - replacePrefix(&rest, &m, `^\$([\w@])()`): - varname, mod := m[1], m[2] + case repl.startsWith(`^\$\{(` + reVarnameDirect + `|@)(:[^\{]+)?\}`), + repl.startsWith(`^\$\((` + reVarnameDirect + `|@])(:[^\)]+)?\)`), + repl.startsWith(`^\$([\w@])()`): + varname, mod := repl.m[1], repl.m[2] if varname == "@" { line.warnf("Please use \"${.TARGET}\" instead of \"$@\".") @@ -218,16 +176,16 @@ outer: // The syntax of the variable modifiers can get quite // hairy. In lack of motivation, we just skip anything // complicated, hoping that at least the braces are balanced. - case replacePrefix(&rest, &m, `^\$\{`): + case repl.startsWith(`^\$\{`): braces := 1 skip: - for rest != "" && braces > 0 { + for repl.rest != "" && braces > 0 { switch { - case replacePrefix(&rest, &m, `^\}`): + case repl.startsWith(`^\}`): braces-- - case replacePrefix(&rest, &m, `^\{`): + case repl.startsWith(`^\{`): braces++ - case replacePrefix(&rest, &m, `^[^{}]+`): + case repl.startsWith(`^[^{}]+`): // skip default: break skip @@ -236,18 +194,18 @@ outer: case state == swstPlain: switch { - case replacePrefix(&rest, &m, `^[!#\%&\(\)*+,\-.\/0-9:;<=>?@A-Z\[\]^_a-z{|}~]+`), - replacePrefix(&rest, &m, `^\\(?:[ !"#'\(\)*;?\\^{|}]|\$\$)`): - case replacePrefix(&rest, &m, `^'`): + case repl.startsWith(`^[!#\%&\(\)*+,\-.\/0-9:;<=>?@A-Z\[\]^_a-z{|}~]+`), + repl.startsWith(`^\\(?:[ !"#'\(\)*;?\\^{|}]|\$\$)`): + case repl.startsWith(`^'`): state = swstSquot - case replacePrefix(&rest, &m, `^"`): + case repl.startsWith(`^"`): state = swstDquot - case replacePrefix(&rest, &m, "^`"): + case repl.startsWith("^`"): state = swstBackt - case replacePrefix(&rest, &m, `^\$\$([0-9A-Z_a-z]+|\#)`), - replacePrefix(&rest, &m, `^\$\$\{([0-9A-Z_a-z]+|\#)\}`), - replacePrefix(&rest, &m, `^\$\$(\$)\$`): - shvarname := m[1] + case repl.startsWith(`^\$\$([0-9A-Z_a-z]+|\#)`), + repl.startsWith(`^\$\$\{([0-9A-Z_a-z]+|\#)\}`), + repl.startsWith(`^\$\$(\$)\$`): + shvarname := repl.m[1] if G.opts.WarnQuoting && checkQuoting && msline.variableNeedsQuoting(shvarname) { line.warnf("Unquoted shell variable %q.", shvarname) line.explain( @@ -264,19 +222,19 @@ outer: "\tcp \"$fname\" /tmp", "\t# copies one file, as intended") } - case replacePrefix(&rest, &m, `^\$@`): + case repl.startsWith(`^\$@`): line.warnf("Please use %q instead of %q.", "${.TARGET}", "$@") line.explain( "It is more readable and prevents confusion with the shell variable of", "the same name.") - case replacePrefix(&rest, &m, `^\$\$@`): + case repl.startsWith(`^\$\$@`): line.warnf("The $@ shell variable should only be used in double quotes.") - case replacePrefix(&rest, &m, `^\$\$\?`): + case repl.startsWith(`^\$\$\?`): line.warnf("The $? shell variable is often not available in \"set -e\" mode.") - case replacePrefix(&rest, &m, `^\$\$\(`): + case repl.startsWith(`^\$\$\(`): line.warnf("Invoking subshells via $(...) is not portable enough.") line.explain( "The Solaris /bin/sh does not know this way to execute a command in a", @@ -288,11 +246,11 @@ outer: case state == swstSquot: switch { - case replacePrefix(&rest, &m, `^'`): + case repl.startsWith(`^'`): state = swstPlain - case replacePrefix(&rest, &m, `^[^\$\']+`): + case repl.startsWith(`^[^\$\']+`): // just skip - case replacePrefix(&rest, &m, `^\$\$`): + case repl.startsWith(`^\$\$`): // just skip default: break outer @@ -300,22 +258,22 @@ outer: case state == swstDquot: switch { - case replacePrefix(&rest, &m, `^"`): + case repl.startsWith(`^"`): state = swstPlain - case replacePrefix(&rest, &m, "^`"): + case repl.startsWith("^`"): state = swstDquotBackt - case replacePrefix(&rest, &m, "^[^$\"\\\\`]+"): + case repl.startsWith("^[^$\"\\\\`]+"): // just skip - case replacePrefix(&rest, &m, "^\\\\(?:[\\\\\"`]|\\$\\$)"): + case repl.startsWith("^\\\\(?:[\\\\\"`]|\\$\\$)"): // just skip - case replacePrefix(&rest, &m, `^\$\$\{([0-9A-Za-z_]+)\}`), - replacePrefix(&rest, &m, `^\$\$([0-9A-Z_a-z]+|[!#?@]|\$\$)`): - shvarname := m[1] + case repl.startsWith(`^\$\$\{([0-9A-Za-z_]+)\}`), + repl.startsWith(`^\$\$([0-9A-Z_a-z]+|[!#?@]|\$\$)`): + shvarname := repl.m[1] _ = G.opts.DebugShell && line.debugf("checklineMkShellword: found double-quoted variable %q.", shvarname) - case replacePrefix(&rest, &m, `^\$\$`): + case repl.startsWith(`^\$\$`): line.warnf("Unquoted $ or strange shell variable found.") - case replacePrefix(&rest, &m, `^\\(.)`): - char := m[1] + case repl.startsWith(`^\\(.)`): + char := repl.m[1] line.warnf("Please use \"%s\" instead of \"%s\".", "\\\\"+char, "\\"+char) line.explain( "Although the current code may work, it is not good style to rely on", @@ -328,11 +286,54 @@ outer: } } - if strings.TrimSpace(rest) != "" { - line.errorf("Internal pkglint error: checklineMkShellword state=%s, rest=%q, shellword=%q", state, rest, shellword) + if strings.TrimSpace(repl.rest) != "" { + line.errorf("Internal pkglint error: checklineMkShellword state=%s, rest=%q, shellword=%q", state, repl.rest, shellword) } } +// Scan for the end of the backticks, checking for single backslashes +// and removing one level of backslashes. Backslashes are only removed +// before a dollar, a backslash or a backtick. +// +// See http://www.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_03 +func (msline *MkShellLine) unescapeBackticks(shellword string, repl *PrefixReplacer, state ShellwordState) (unescaped string, newState ShellwordState) { + line := msline.line + for repl.rest != "" { + switch { + case repl.startsWith("^`"): + if state == swstBackt { + state = swstPlain + } else { + state = swstDquot + } + return unescaped, state + + case repl.startsWith("^\\\\([\\\\`$])"): + unescaped += repl.m[1] + + case repl.startsWith(`^(\\)`): + line.warnf("Backslashes should be doubled inside backticks.") + unescaped += repl.m[1] + + case state == swstDquotBackt && repl.startsWith(`^"`): + line.warnf("Double quotes inside backticks inside double quotes are error prone.") + line.explain( + "According to the SUSv3, they produce undefined results.", + "", + "See the paragraph starting \"Within the backquoted ...\" in", + "http://www.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html") + + case repl.startsWith("^([^\\\\`]+)"): + unescaped += repl.m[1] + + default: + line.errorf("Internal pkglint error: checklineMkShellword shellword=%q rest=%q", shellword, repl.rest) + } + } + line.errorf("Unfinished backquotes: rest=%q", repl.rest) + return unescaped, state +} + func (msline *MkShellLine) variableNeedsQuoting(shvarname string) bool { switch shvarname { case "#", "?": @@ -372,18 +373,16 @@ func (msline *MkShellLine) checkShelltext(shelltext string) { line.notef("You don't need to use \"-\" before %q.", cmd) } - rest := shelltext - setE := false - var m []string - if replacePrefix(&rest, &m, `^\s*([-@]*)(\$\{_PKG_SILENT\}\$\{_PKG_DEBUG\}|\$\{RUN\}|)`) { - hidden, macro := m[1], m[2] - msline.checkLineStart(hidden, macro, rest, &setE) + repl := NewPrefixReplacer(shelltext) + if repl.startsWith(`^\s*([-@]*)(\$\{_PKG_SILENT\}\$\{_PKG_DEBUG\}|\$\{RUN\}|)`) { + hidden, macro := repl.m[1], repl.m[2] + msline.checkLineStart(hidden, macro, repl.rest, &setE) } state := scstStart - for replacePrefix(&rest, &m, reShellword) { - shellword := m[1] + for repl.startsWith(reShellword) { + shellword := repl.m[1] _ = G.opts.DebugShell && line.debugf("checklineMkShelltext state=%v shellword=%q", state, shellword) @@ -416,8 +415,9 @@ func (msline *MkShellLine) checkShelltext(shelltext string) { state = nextState(line, state, shellword) } - if !matches(rest, `^\s*$`) { - line.errorf("Internal pkglint error: checklineMkShelltext state=%s rest=%q shellword=%q", state, rest, shelltext) + repl.startsWith(`^\s+`) + if repl.rest != "" { + line.errorf("Internal pkglint error: checklineMkShelltext state=%s rest=%q shellword=%q", state, repl.rest, shelltext) } } @@ -862,12 +862,10 @@ func nextState(line *Line, state scState, shellword string) scState { func splitIntoShellwords(line *Line, text string) ([]string, string) { var words []string - rest := text - var m []string - for replacePrefix(&rest, &m, reShellword) { - words = append(words, m[1]) + repl := NewPrefixReplacer(text) + for repl.startsWith(reShellword) { + words = append(words, repl.m[1]) } - - rest = strings.TrimLeftFunc(rest, unicode.IsSpace) - return words, rest + repl.startsWith(`^\s+`) + return words, repl.rest } diff --git a/pkgtools/pkglint/files/substcontext.go b/pkgtools/pkglint/files/substcontext.go index 654eae85adc..f29e5326db3 100644 --- a/pkgtools/pkglint/files/substcontext.go +++ b/pkgtools/pkglint/files/substcontext.go @@ -12,11 +12,15 @@ type SubstContext struct { filterCmd string } -func (ctx *SubstContext) Varassign(line *Line, varname, op, value string) { +func (ctx *SubstContext) Varassign(mkline *MkLine) { if !G.opts.WarnExtra { return } + line:=mkline.line + varname := line.extra["varname"].(string) + op := line.extra["op"].(string) + value := line.extra["value"].(string) if varname == "SUBST_CLASSES" { classes := splitOnSpace(value) if len(classes) > 1 { @@ -46,7 +50,7 @@ func (ctx *SubstContext) Varassign(line *Line, varname, op, value string) { if ctx.IsComplete() { // XXX: This code sometimes produces weird warnings. See // meta-pkgs/xorg/Makefile.common 1.41 for an example. - ctx.Finish(line) + ctx.Finish(mkline) // The following assignment prevents an additional warning, // but from a technically viewpoint, it is incorrect. @@ -82,7 +86,8 @@ func (ctx *SubstContext) IsComplete() bool { (len(ctx.sed) != 0 || len(ctx.vars) != 0 || ctx.filterCmd != "") } -func (ctx *SubstContext) Finish(line *Line) { +func (ctx *SubstContext) Finish(mkline *MkLine) { + line:=mkline.line if ctx.id == "" || !G.opts.WarnExtra { return } diff --git a/pkgtools/pkglint/files/substcontext_test.go b/pkgtools/pkglint/files/substcontext_test.go index 1ea900d0792..03da1ecfc4e 100644 --- a/pkgtools/pkglint/files/substcontext_test.go +++ b/pkgtools/pkglint/files/substcontext_test.go @@ -6,62 +6,63 @@ import ( func (s *Suite) TestSubstContext_Incomplete(c *check.C) { G.opts.WarnExtra = true - line := NewLine("Makefile", "1", "dummy", nil) ctx := new(SubstContext) - ctx.Varassign(line, "PKGNAME", "=", "pkgname-1.0") + ctx.Varassign(newSubstLine("10", "PKGNAME=pkgname-1.0")) c.Check(ctx.id, equals, "") - ctx.Varassign(line, "SUBST_CLASSES", "+=", "interp") + ctx.Varassign(newSubstLine("11", "SUBST_CLASSES+=interp")) c.Check(ctx.id, equals, "interp") - ctx.Varassign(line, "SUBST_FILES.interp", "=", "Makefile") + ctx.Varassign(newSubstLine("12", "SUBST_FILES.interp=Makefile")) c.Check(ctx.IsComplete(), equals, false) - ctx.Varassign(line, "SUBST_SED.interp", "=", "s,@PREFIX@,${PREFIX},g") + ctx.Varassign(newSubstLine("13", "SUBST_SED.interp=s,@PREFIX@,${PREFIX},g")) c.Check(ctx.IsComplete(), equals, false) - ctx.Finish(line) + ctx.Finish(newSubstLine("14", "")) - c.Check(s.Output(), equals, "WARN: Makefile:1: Incomplete SUBST block: SUBST_STAGE.interp missing.\n") + c.Check(s.Output(), equals, "WARN: Makefile:14: Incomplete SUBST block: SUBST_STAGE.interp missing.\n") } func (s *Suite) TestSubstContext_Complete(c *check.C) { G.opts.WarnExtra = true - line := NewLine("Makefile", "1", "dummy", nil) ctx := new(SubstContext) - ctx.Varassign(line, "PKGNAME", "=", "pkgname-1.0") - ctx.Varassign(line, "SUBST_CLASSES", "+=", "p") - ctx.Varassign(line, "SUBST_FILES.p", "=", "Makefile") - ctx.Varassign(line, "SUBST_SED.p", "=", "s,@PREFIX@,${PREFIX},g") + ctx.Varassign(newSubstLine("10", "PKGNAME=pkgname-1.0")) + ctx.Varassign(newSubstLine("11", "SUBST_CLASSES+=p")) + ctx.Varassign(newSubstLine("12", "SUBST_FILES.p=Makefile")) + ctx.Varassign(newSubstLine("13", "SUBST_SED.p=s,@PREFIX@,${PREFIX},g")) c.Check(ctx.IsComplete(), equals, false) - ctx.Varassign(line, "SUBST_STAGE.p", "=", "post-configure") + ctx.Varassign(newSubstLine("14", "SUBST_STAGE.p=post-configure")) c.Check(ctx.IsComplete(), equals, true) - ctx.Finish(line) + ctx.Finish(newSubstLine("15", "")) c.Check(s.Output(), equals, "") } func (s *Suite) TestSubstContext_NoClass(c *check.C) { s.UseCommandLine(c, "-Wextra") - line := NewLine("Makefile", "1", "dummy", nil) ctx := new(SubstContext) - ctx.Varassign(line, "UNRELATED", "=", "anything") - ctx.Varassign(line, "SUBST_FILES.repl", "+=", "Makefile.in") - ctx.Varassign(line, "SUBST_SED.repl", "+=", "-e s,from,to,g") - ctx.Finish(line) + ctx.Varassign(newSubstLine("10", "UNRELATED=anything")) + ctx.Varassign(newSubstLine("11", "SUBST_FILES.repl+=Makefile.in")) + ctx.Varassign(newSubstLine("12", "SUBST_SED.repl+=-e s,from,to,g")) + ctx.Finish(newSubstLine("13","")) c.Check(s.Output(), equals, ""+ - "WARN: Makefile:1: SUBST_CLASSES should come before the definition of \"SUBST_FILES.repl\".\n"+ - "WARN: Makefile:1: Incomplete SUBST block: SUBST_STAGE.repl missing.\n") + "WARN: Makefile:11: SUBST_CLASSES should come before the definition of \"SUBST_FILES.repl\".\n"+ + "WARN: Makefile:13: Incomplete SUBST block: SUBST_STAGE.repl missing.\n") +} + +func newSubstLine(lineno, text string) *MkLine { + return NewMkLine(NewLine("Makefile", lineno, text, nil)) } diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go index e2b12e22e4e..637a79c2dc1 100644 --- a/pkgtools/pkglint/files/util.go +++ b/pkgtools/pkglint/files/util.go @@ -258,10 +258,19 @@ func replaceFirst(s, re, replacement string) ([]string, string) { return nil, s } -func replacePrefix(ps *string, pm *[]string, re string) bool { - if m := regcomp(re).FindStringSubmatch(*ps); m != nil { - *ps = (*ps)[len(m[0]):] - *pm = m +type PrefixReplacer struct { + rest string + m []string +} + +func NewPrefixReplacer(s string) *PrefixReplacer { + return &PrefixReplacer{s, nil} +} + +func (pr *PrefixReplacer) startsWith(re string) bool { + if m := regcomp(re).FindStringSubmatch(pr.rest); m != nil { + pr.rest = pr.rest[len(m[0]):] + pr.m = m return true } return false |