x/lib/cmdline: Fix bugs regarding LookPath subcommands.
One bug caused duplicate help entries. The symptoms of the bug
were that we'd see duplicate help output in both the short and
long descriptions, if either of the following conditions
occurred:
1) A command appeared as both a built-in Child as well as a
binary found via LookPath.
2) The user's PATH contained multiple directories that each
contained the same binary.
The fix is to dedup the subcommands.
Another bug was that if the LookPath subcommand exited with a
non-zero exit code, we wouldn't propagate that back to the
calling binary.
Change-Id: I843a8692bcb816b489ddfd7983ed8a496289445c
MultiPart: 1/2
diff --git a/cmdline/cmdline.go b/cmdline/cmdline.go
index 61778dc..da393e1 100644
--- a/cmdline/cmdline.go
+++ b/cmdline/cmdline.go
@@ -49,6 +49,7 @@
"os/exec"
"reflect"
"strings"
+ "syscall"
"v.io/x/lib/envvar"
_ "v.io/x/lib/metadata" // for the -metadata flag
@@ -398,6 +399,16 @@
return cp
}
+// subNames returns the sub names of c which should be ignored when using look
+// path to find external binaries.
+func (c *Command) subNames() map[string]bool {
+ m := map[string]bool{"help": true}
+ for _, child := range c.Children {
+ m[child.Name] = true
+ }
+ return m
+}
+
// ErrExitCode may be returned by Runner.Run to cause the program to exit with a
// specific error code.
type ErrExitCode int
@@ -442,7 +453,14 @@
cmd.Stderr = env.Stderr
cmd.Env = envvar.MapToSlice(env.Vars)
cmd.Env = append(cmd.Env, "CMDLINE_PREFIX="+b.cmdPath)
- return cmd.Run()
+ err := cmd.Run()
+ // Make sure we return the exit code from the binary, if it exited.
+ if exitError, ok := err.(*exec.ExitError); ok {
+ if status, ok := exitError.Sys().(syscall.WaitStatus); ok {
+ return ErrExitCode(status.ExitStatus())
+ }
+ }
+ return err
}
// lookPath returns a boolean that indicates whether executable <name>
@@ -465,9 +483,10 @@
return false
}
-// lookPathAll returns a list of all executables found in the given
-// directories whose name starts with "<name>-".
-func lookPathAll(name string, dirs []string) (result []string) {
+// lookPathAll returns a deduped list of all executables found in the given
+// directories whose name starts with "<name>-", and where the name doesn't
+// match the given seen set. The seen set may be mutated by this function.
+func lookPathAll(name string, dirs []string, seen map[string]bool) (result []string) {
for _, dir := range dirs {
fileInfos, err := ioutil.ReadDir(dir)
if err != nil {
@@ -477,9 +496,15 @@
if m := fileInfo.Mode(); !m.IsRegular() || (m&os.FileMode(0111)) == 0 {
continue
}
- if strings.HasPrefix(fileInfo.Name(), name+"-") {
- result = append(result, fileInfo.Name())
+ if !strings.HasPrefix(fileInfo.Name(), name+"-") {
+ continue
}
+ subname := fileInfo.Name()[len(name+"-"):]
+ if seen[subname] {
+ continue
+ }
+ seen[subname] = true
+ result = append(result, fileInfo.Name())
}
}
return
diff --git a/cmdline/cmdline_test.go b/cmdline/cmdline_test.go
index a119512..2b72aa7 100644
--- a/cmdline/cmdline_test.go
+++ b/cmdline/cmdline_test.go
@@ -2445,15 +2445,16 @@
}
defer os.RemoveAll(tmpDir)
- // Add the temporary directory to PATH.
+ // Add the temporary directory to PATH. We add it twice to ensure dups are
+ // filtered in the resulting output.
oldPath := os.Getenv("PATH")
defer os.Setenv("PATH", oldPath)
tokens := strings.Split(oldPath, string(os.PathListSeparator))
- tokens = append([]string{tmpDir}, tokens...)
+ tokens = append([]string{tmpDir, tmpDir}, tokens...)
os.Setenv("PATH", strings.Join(tokens, string(os.PathListSeparator)))
// Build the binary subcommands.
- for _, subCmd := range []string{"flat", "foreign", "nested"} {
+ for _, subCmd := range []string{"flat", "foreign", "nested", "repeated", "exitcode"} {
cmd := exec.Command("go", "build", "-o", filepath.Join(tmpDir, "unlikely-"+subCmd), filepath.Join(".", "testdata", subCmd+".go"))
if out, err := cmd.CombinedOutput(); err != nil {
t.Fatalf("%v, %v", string(out), err)
@@ -2473,6 +2474,12 @@
Short: "Short description of command foo",
Long: "Long description of command foo.",
},
+ &Command{
+ Runner: RunnerFunc(runHello),
+ Name: "repeated",
+ Short: "Repeated appears as both a child and as a binary",
+ Long: "Long description of command repeated.",
+ },
},
}
@@ -2489,7 +2496,9 @@
The unlikely commands are:
foo Short description of command foo
+ repeated Repeated appears as both a child and as a binary
help Display help for commands or topics
+ exitcode Short description of command exitcode
flat Short description of command flat
foreign No description available
nested Short description of command nested
@@ -2514,7 +2523,9 @@
The unlikely commands are:
foo Short description of command foo
+ repeated Repeated appears as both a child and as a binary
help Display help for commands or topics
+ exitcode Short description of command exitcode
flat Short description of command flat
foreign No description available
nested Short description of command nested
@@ -2539,7 +2550,9 @@
The unlikely commands are:
foo Short description of command foo
+ repeated Repeated appears as both a child and as a binary
help Display help for commands or topics
+ exitcode Short description of command exitcode
flat Short description of command flat
foreign No description available
nested Short description of command nested
@@ -2558,12 +2571,30 @@
Usage:
unlikely foo
================================================================================
+Unlikely repeated - Repeated appears as both a child and as a binary
+
+Long description of command repeated.
+
+Usage:
+ unlikely repeated
+================================================================================
+Unlikely exitcode - Short description of command exitcode
+
+Long description of command exitcode.
+
+Usage:
+ unlikely exitcode [args]
+
+[args] are ignored
+================================================================================
Unlikely flat - Short description of command flat
Long description of command flat.
Usage:
- unlikely flat
+ unlikely flat [args]
+
+[args] are ignored
================================================================================
Unlikely foreign - No description available
================================================================================
@@ -2622,7 +2653,9 @@
The unlikely commands are:
foo Short description of command foo
+ repeated Repeated appears as both a child and as a binary
help Display help for commands or topics
+ exitcode Short description of command exitcode
flat Short description of command flat
foreign No description available
nested Short description of command nested
@@ -2640,12 +2673,30 @@
Usage:
unlikely foo
+Unlikely repeated - Repeated appears as both a child and as a binary
+
+Long description of command repeated.
+
+Usage:
+ unlikely repeated
+
+Unlikely exitcode - Short description of command exitcode
+
+Long description of command exitcode.
+
+Usage:
+ unlikely exitcode [args]
+
+[args] are ignored
+
Unlikely flat - Short description of command flat
Long description of command flat.
Usage:
- unlikely flat
+ unlikely flat [args]
+
+[args] are ignored
Unlikely foreign - No description available
@@ -2692,9 +2743,33 @@
the CMDLINE_WIDTH environment variable.
`,
},
+ {
+ Args: []string{"flat", "help", "..."},
+ Vars: map[string]string{
+ "PATH": strings.Join(tokens, string(os.PathListSeparator)),
+ },
+ Err: errUsageStr,
+ Stderr: `ERROR: unlikely flat: unsupported help invocation
+
+Long description of command flat.
+
+Usage:
+ unlikely flat [args]
+
+[args] are ignored
+
+The global flags are:
+ -metadata=<just specify -metadata to activate>
+ Displays metadata for the program and exits.
+`,
+ },
+ {
+ Args: []string{"exitcode"},
+ Vars: map[string]string{
+ "PATH": strings.Join(tokens, string(os.PathListSeparator)),
+ },
+ Err: "exit code 42",
+ },
}
runTestCases(t, cmd, tests)
}
-
-// TODO(toddw): Add a test for the case when "help ..." is passed to a
-// childless subcommand.
diff --git a/cmdline/help.go b/cmdline/help.go
index 4eafa16..ab1f5bd 100644
--- a/cmdline/help.go
+++ b/cmdline/help.go
@@ -215,7 +215,7 @@
usageAll(w, append(path, child), config, false)
}
if cmd.LookPath {
- subCmds := lookPathAll(cmd.Name, config.env.pathDirs())
+ subCmds := lookPathAll(cmd.Name, config.env.pathDirs(), cmd.subNames())
for _, subCmd := range subCmds {
runner := binaryRunner{subCmd, cmdPath}
var buffer bytes.Buffer
@@ -282,7 +282,7 @@
}
var subCmds []string
if cmd.LookPath {
- subCmds = lookPathAll(cmd.Name, config.env.pathDirs())
+ subCmds = lookPathAll(cmd.Name, config.env.pathDirs(), cmd.subNames())
}
if !firstCall {
lineBreak(w, config.style)
diff --git a/cmdline/testdata/exitcode.go b/cmdline/testdata/exitcode.go
new file mode 100644
index 0000000..24303e9
--- /dev/null
+++ b/cmdline/testdata/exitcode.go
@@ -0,0 +1,27 @@
+// Copyright 2015 The Vanadium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import (
+ ".."
+)
+
+// cmdExitCode represents the exitcode command.
+var cmdExitCode = &cmdline.Command{
+ Runner: cmdline.RunnerFunc(runExitCode),
+ Name: "exitcode",
+ Short: "Short description of command exitcode",
+ Long: "Long description of command exitcode.",
+ ArgsName: "[args]",
+ ArgsLong: "[args] are ignored",
+}
+
+func runExitCode(env *cmdline.Env, _ []string) error {
+ return cmdline.ErrExitCode(42)
+}
+
+func main() {
+ cmdline.Main(cmdExitCode)
+}
diff --git a/cmdline/testdata/flat.go b/cmdline/testdata/flat.go
index aa06085..7aa0c08 100644
--- a/cmdline/testdata/flat.go
+++ b/cmdline/testdata/flat.go
@@ -10,10 +10,12 @@
// cmdFlat represents the flat command.
var cmdFlat = &cmdline.Command{
- Runner: cmdline.RunnerFunc(runFlat),
- Name: "flat",
- Short: "Short description of command flat",
- Long: "Long description of command flat.",
+ Runner: cmdline.RunnerFunc(runFlat),
+ Name: "flat",
+ Short: "Short description of command flat",
+ Long: "Long description of command flat.",
+ ArgsName: "[args]",
+ ArgsLong: "[args] are ignored",
}
func runFlat(env *cmdline.Env, _ []string) error {
diff --git a/cmdline/testdata/repeated.go b/cmdline/testdata/repeated.go
new file mode 100644
index 0000000..ad6775b
--- /dev/null
+++ b/cmdline/testdata/repeated.go
@@ -0,0 +1,25 @@
+// Copyright 2015 The Vanadium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import (
+ ".."
+)
+
+// cmdRepeated represents the repeated command.
+var cmdRepeated = &cmdline.Command{
+ Runner: cmdline.RunnerFunc(runRepeated),
+ Name: "repeated",
+ Short: "REPEATED SHOULD NEVER APPEAR",
+ Long: "REPEATED SHOULD NEVER APPEAR.",
+}
+
+func runRepeated(env *cmdline.Env, _ []string) error {
+ return nil
+}
+
+func main() {
+ cmdline.Main(cmdRepeated)
+}
