diff --git a/internal/gps/cmd.go b/internal/gps/cmd.go deleted file mode 100644 index 6ab6032f11..0000000000 --- a/internal/gps/cmd.go +++ /dev/null @@ -1,205 +0,0 @@ -// Copyright 2017 The Go 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 gps - -import ( - "bytes" - "context" - "fmt" - "os/exec" - "sync" - "sync/atomic" - "time" - - "github.com/Masterminds/vcs" - "github.com/pkg/errors" -) - -// monitoredCmd wraps a cmd and will keep monitoring the process until it -// finishes, the provided context is canceled, or a certain amount of time has -// passed and the command showed no signs of activity. -type monitoredCmd struct { - cmd *exec.Cmd - timeout time.Duration - stdout *activityBuffer - stderr *activityBuffer -} - -// noProgressError indicates that the monitored process was terminated due to -// exceeding exceeding the progress timeout. -type noProgressError struct { - timeout time.Duration -} - -// killCmdError indicates that an error occurred while sending a kill signal to -// the monitored process. -type killCmdError struct { - err error -} - -func newMonitoredCmd(cmd *exec.Cmd, timeout time.Duration) *monitoredCmd { - stdout, stderr := newActivityBuffer(), newActivityBuffer() - cmd.Stdout, cmd.Stderr = stdout, stderr - return &monitoredCmd{ - cmd: cmd, - timeout: timeout, - stdout: stdout, - stderr: stderr, - } -} - -// run will wait for the command to finish and return the error, if any. If the -// command does not show any progress, as indicated by writing to stdout or -// stderr, for more than the specified timeout, the process will be killed. -func (c *monitoredCmd) run(ctx context.Context) error { - // Check for cancellation before even starting - if ctx.Err() != nil { - return ctx.Err() - } - - err := c.cmd.Start() - if err != nil { - return err - } - - ticker := time.NewTicker(c.timeout) - defer ticker.Stop() - - // Atomic marker to track proc exit state. Guards against bad channel - // select receive order, where a tick or context cancellation could come - // in at the same time as process completion, but one of the former are - // picked first; in such a case, cmd.Process could(?) be nil by the time we - // call signal methods on it. - var isDone int32 - done := make(chan error, 1) - - go func() { - // Wait() can only be called once, so this must act as the completion - // indicator for both normal *and* signal-induced termination. - done <- c.cmd.Wait() - atomic.CompareAndSwapInt32(&isDone, 0, 1) - }() - - var killerr error -selloop: - for { - select { - case err := <-done: - return err - case <-ticker.C: - if !atomic.CompareAndSwapInt32(&isDone, 1, 1) && c.hasTimedOut() { - if err := killProcess(c.cmd, &isDone); err != nil { - killerr = &killCmdError{err} - } else { - killerr = &noProgressError{c.timeout} - } - break selloop - } - case <-ctx.Done(): - if !atomic.CompareAndSwapInt32(&isDone, 1, 1) { - if err := killProcess(c.cmd, &isDone); err != nil { - killerr = &killCmdError{err} - } else { - killerr = ctx.Err() - } - break selloop - } - } - } - - // This is only reachable on the signal-induced termination path, so block - // until a message comes through the channel indicating that the command has - // exited. - // - // TODO(sdboyer) if the signaling process errored (resulting in a - // killCmdError stored in killerr), is it possible that this receive could - // block forever on some kind of hung process? - <-done - return killerr -} - -func (c *monitoredCmd) hasTimedOut() bool { - t := time.Now().Add(-c.timeout) - return c.stderr.lastActivity().Before(t) && - c.stdout.lastActivity().Before(t) -} - -func (c *monitoredCmd) combinedOutput(ctx context.Context) ([]byte, error) { - c.cmd.Stderr = c.stdout - err := c.run(ctx) - return c.stdout.Bytes(), errors.Wrapf(err, "command failed: %v", c.cmd.Args) -} - -// activityBuffer is a buffer that keeps track of the last time a Write -// operation was performed on it. -type activityBuffer struct { - sync.Mutex - buf *bytes.Buffer - lastActivityStamp time.Time -} - -func newActivityBuffer() *activityBuffer { - return &activityBuffer{ - buf: bytes.NewBuffer(nil), - } -} - -func (b *activityBuffer) Write(p []byte) (int, error) { - b.Lock() - defer b.Unlock() - - b.lastActivityStamp = time.Now() - - return b.buf.Write(p) -} - -func (b *activityBuffer) String() string { - b.Lock() - defer b.Unlock() - - return b.buf.String() -} - -func (b *activityBuffer) Bytes() []byte { - b.Lock() - defer b.Unlock() - - return b.buf.Bytes() -} - -func (b *activityBuffer) lastActivity() time.Time { - b.Lock() - defer b.Unlock() - - return b.lastActivityStamp -} - -func (e noProgressError) Error() string { - return fmt.Sprintf("command killed after %s of no activity", e.timeout) -} - -func (e killCmdError) Error() string { - return fmt.Sprintf("error killing command: %s", e.err) -} - -func runFromCwd(ctx context.Context, timeout time.Duration, cmd string, args ...string) ([]byte, error) { - c := newMonitoredCmd(exec.Command(cmd, args...), timeout) - return c.combinedOutput(ctx) -} - -func runFromRepoDir(ctx context.Context, repo vcs.Repo, timeout time.Duration, cmd string, args ...string) ([]byte, error) { - c := newMonitoredCmd(repo.CmdFromDir(cmd, args...), timeout) - return c.combinedOutput(ctx) -} - -const ( - // expensiveCmdTimeout is meant to be used in a command that is expensive - // in terms of computation and we know it will take long or one that uses - // the network, such as clones, updates, .... - expensiveCmdTimeout = 2 * time.Minute - // defaultCmdTimeout is just an umbrella value for all other commands that - // should not take much. - defaultCmdTimeout = 30 * time.Second -) diff --git a/internal/gps/cmd_test.go b/internal/gps/cmd_test.go deleted file mode 100644 index 6c25f5def3..0000000000 --- a/internal/gps/cmd_test.go +++ /dev/null @@ -1,124 +0,0 @@ -// Copyright 2017 The Go 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 gps - -import ( - "context" - "fmt" - "os" - "os/exec" - "strings" - "testing" - "time" -) - -func mkTestCmd(iterations int) *monitoredCmd { - return newMonitoredCmd( - exec.Command("./echosleep", "-n", fmt.Sprint(iterations)), - 490*time.Millisecond, - ) -} - -func TestMonitoredCmd(t *testing.T) { - // Sleeps and compile make this a bit slow - if testing.Short() { - t.Skip("skipping test with sleeps on short") - } - - err := exec.Command("go", "build", "./_testdata/cmd/echosleep/echosleep.go").Run() - if err != nil { - t.Errorf("Unable to build echosleep binary: %s", err) - } - defer os.Remove("./echosleep") - - tests := []struct { - name string - iterations int - output string - err bool - timeout bool - }{ - {"success", 2, "foo\nfoo\n", false, false}, - {"timeout", 5, "foo\nfoo\nfoo\nfoo\n", true, true}, - } - - for _, want := range tests { - t.Run(want.name, func(t *testing.T) { - cmd := mkTestCmd(want.iterations) - - err := cmd.run(context.Background()) - if !want.err && err != nil { - t.Errorf("Eexpected command not to fail, got error: %s", err) - } else if want.err && err == nil { - t.Error("expected command to fail") - } - - got := cmd.stdout.String() - if want.output != got { - t.Errorf("unexpected output:\n\t(GOT):\n%s\n\t(WNT):\n%s", got, want.output) - } - - if want.timeout { - _, ok := err.(*noProgressError) - if !ok { - t.Errorf("Expected a timeout error, but got: %s", err) - } - } - }) - } - - t.Run("cancel", func(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - sync, errchan := make(chan struct{}), make(chan error) - cmd := mkTestCmd(2) - go func() { - close(sync) - errchan <- cmd.run(ctx) - }() - - // Make sure goroutine is at least started before we cancel the context. - <-sync - // Give it a bit to get the process started. - <-time.After(5 * time.Millisecond) - cancel() - - err := <-errchan - if err != context.Canceled { - t.Errorf("expected a canceled error, got %s", err) - } - }) -} - -func TestCombinedOutput(t *testing.T) { - // Compile makes this a bit slow - if testing.Short() { - t.Skip("skipping test with compilation on short") - } - - err := exec.Command("go", "build", "./_testdata/cmd/stdout_stderr/stdout_stderr.go").Run() - if err != nil { - t.Errorf("Unable to build stdout_stderr binary: %s", err) - } - defer os.Remove("./stdout_stderr") - - cmd := newMonitoredCmd( - exec.Command("./stdout_stderr"), - 490*time.Millisecond, - ) - - out, err := cmd.combinedOutput(context.Background()) - if err != nil { - t.Errorf("Unexpected error: %s", err) - } - - output := string(out) - if !strings.Contains(output, "stdout") { - t.Errorf("Expecting to receive output from stdout") - } - - if !strings.Contains(output, "stderr") { - t.Errorf("Expecting to receive output from stderr") - } -} diff --git a/internal/gps/cmd_unix.go b/internal/gps/cmd_unix.go index 9c58b300ef..d5df8193b8 100644 --- a/internal/gps/cmd_unix.go +++ b/internal/gps/cmd_unix.go @@ -7,50 +7,83 @@ package gps import ( + "bytes" + "context" "os" "os/exec" - "sync/atomic" "time" + + "github.com/pkg/errors" ) -// killProcess manages the termination of subprocesses in a way that tries to be -// gentle (via os.Interrupt), but resorts to Kill if needed. -// -// -// TODO(sdboyer) should the return differentiate between whether gentle methods -// succeeded vs. falling back to a hard kill? -func killProcess(cmd *exec.Cmd, isDone *int32) error { - if err := cmd.Process.Signal(os.Interrupt); err != nil { - // If an error comes back from attempting to signal, proceed immediately - // to hard kill. - return cmd.Process.Kill() +type cmd struct { + // ctx is provided by the caller; SIGINT is sent when it is cancelled. + ctx context.Context + // cancel is called when the graceful shutdown timeout expires. + cancel context.CancelFunc + cmd *exec.Cmd +} + +func commandContext(ctx context.Context, name string, arg ...string) cmd { + // Grab the caller's context and pass a derived one to CommandContext. + c := cmd{ctx: ctx} + ctx, cancel := context.WithCancel(ctx) + c.cmd = exec.CommandContext(ctx, name, arg...) + c.cancel = cancel + return c +} + +func (c cmd) Args() []string { + return c.cmd.Args +} + +func (c cmd) SetDir(dir string) { + c.cmd.Dir = dir +} + +// CombinedOutput is like (*os/exec.Cmd).CombinedOutput except that it +// terminates subprocesses gently (via os.Interrupt), but resorts to Kill if +// the subprocess fails to exit after 1 minute. +func (c cmd) CombinedOutput() ([]byte, error) { + // Adapted from (*os/exec.Cmd).CombinedOutput + if c.cmd.Stdout != nil { + return nil, errors.New("exec: Stdout already set") + } + if c.cmd.Stderr != nil { + return nil, errors.New("exec: Stderr already set") + } + var b bytes.Buffer + c.cmd.Stdout = &b + c.cmd.Stderr = &b + if err := c.cmd.Start(); err != nil { + return nil, err } - // If the process doesn't exit immediately, check every 50ms, up to 3s, - // after which send a hard kill. - // - // Cannot rely on cmd.ProcessState.Exited() here, as that is not set - // correctly when the process exits due to a signal. See - // https://github.com/golang/go/issues/19798 . Also cannot rely on it - // because cmd.ProcessState will be nil before the process exits, and - // checking if nil create a data race. - if !atomic.CompareAndSwapInt32(isDone, 1, 1) { - to := time.NewTimer(3 * time.Second) - tick := time.NewTicker(50 * time.Millisecond) - - defer to.Stop() - defer tick.Stop() - - // Loop until the ProcessState shows up, indicating the proc has exited, - // or the timer expires and - for !atomic.CompareAndSwapInt32(isDone, 1, 1) { - select { - case <-to.C: - return cmd.Process.Kill() - case <-tick.C: + var t *time.Timer + defer func() { + if t != nil { + t.Stop() + } + }() + // Adapted from (*os/exec.Cmd).Start + waitDone := make(chan struct{}) + defer close(waitDone) + go func() { + select { + case <-c.ctx.Done(): + if err := c.cmd.Process.Signal(os.Interrupt); err != nil { + // If an error comes back from attempting to signal, proceed + // immediately to hard kill. + c.cancel() + } else { + t = time.AfterFunc(time.Minute, c.cancel) } + case <-waitDone: } - } + }() - return nil + if err := c.cmd.Wait(); err != nil { + return nil, err + } + return b.Bytes(), nil } diff --git a/internal/gps/cmd_windows.go b/internal/gps/cmd_windows.go index 7069758a60..943925e7b5 100644 --- a/internal/gps/cmd_windows.go +++ b/internal/gps/cmd_windows.go @@ -2,13 +2,25 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// +build windows - package gps -import "os/exec" +import ( + "context" + "os/exec" +) + +type cmd struct { + *exec.Cmd +} + +func commandContext(ctx context.Context, name string, arg ...string) cmd { + return cmd{Cmd: exec.CommandContext(ctx, name, arg...)} +} + +func (c cmd) Args() []string { + return c.Cmd.Args +} -func killProcess(cmd *exec.Cmd, isDone *int32) error { - // TODO it'd be great if this could be more sophisticated... - return cmd.Process.Kill() +func (c cmd) SetDir(dir string) { + c.Cmd.Dir = dir } diff --git a/internal/gps/vcs_repo.go b/internal/gps/vcs_repo.go index 3c17c2ea4e..03f82cf2a2 100644 --- a/internal/gps/vcs_repo.go +++ b/internal/gps/vcs_repo.go @@ -14,6 +14,7 @@ import ( "time" "github.com/Masterminds/vcs" + "github.com/pkg/errors" ) type ctxRepo interface { @@ -84,27 +85,54 @@ func newVcsLocalErrorOr(msg string, err error, out string) error { } func (r *gitRepo) get(ctx context.Context) error { - out, err := runFromCwd(ctx, expensiveCmdTimeout, "git", "clone", "--recursive", "-v", "--progress", r.Remote(), r.LocalPath()) - if err != nil { - return newVcsRemoteErrorOr("unable to get repository", err, string(out)) + cmd := commandContext( + ctx, + "git", + "clone", + "--recursive", + "-v", + "--progress", + r.Remote(), + r.LocalPath(), + ) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsRemoteErrorOr( + "unable to get repository", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) } return nil } func (r *gitRepo) fetch(ctx context.Context) error { - // Perform a fetch to make sure everything is up to date. - out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "git", "fetch", "--tags", "--prune", r.RemoteLocation) - if err != nil { - return newVcsRemoteErrorOr("unable to update repository", err, string(out)) + cmd := commandContext( + ctx, + "git", + "fetch", + "--tags", + "--prune", + r.RemoteLocation, + ) + cmd.SetDir(r.LocalPath()) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsRemoteErrorOr( + "unable to update repository", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out)) } return nil } func (r *gitRepo) updateVersion(ctx context.Context, v string) error { - out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "git", "checkout", v) - if err != nil { - return newVcsLocalErrorOr("Unable to update checked out version", err, string(out)) + cmd := commandContext(ctx, "git", "checkout", v) + cmd.SetDir(r.LocalPath()) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsLocalErrorOr( + "unable to update checked out version", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out)) } return r.defendAgainstSubmodules(ctx) @@ -114,22 +142,58 @@ func (r *gitRepo) updateVersion(ctx context.Context, v string) error { // submodules. Or nested submodules. What a great idea, submodules. func (r *gitRepo) defendAgainstSubmodules(ctx context.Context) error { // First, update them to whatever they should be, if there should happen to be any. - out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "git", "submodule", "update", "--init", "--recursive") - if err != nil { - return newVcsLocalErrorOr("unexpected error while defensively updating submodules", err, string(out)) + { + cmd := commandContext( + ctx, + "git", + "submodule", + "update", + "--init", + "--recursive", + ) + cmd.SetDir(r.LocalPath()) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsLocalErrorOr( + "unexpected error while defensively updating submodules", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) + } } // Now, do a special extra-aggressive clean in case changing versions caused // one or more submodules to go away. - out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "git", "clean", "-x", "-d", "-f", "-f") - if err != nil { - return newVcsLocalErrorOr("unexpected error while defensively cleaning up after possible derelict submodule directories", err, string(out)) + { + cmd := commandContext(ctx, "git", "clean", "-x", "-d", "-f", "-f") + cmd.SetDir(r.LocalPath()) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsLocalErrorOr( + "unexpected error while defensively cleaning up after possible derelict submodule directories", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) + } } // Then, repeat just in case there are any nested submodules that went away. - out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "git", "submodule", "foreach", "--recursive", "git", "clean", "-x", "-d", "-f", "-f") - if err != nil { - return newVcsLocalErrorOr("unexpected error while defensively cleaning up after possible derelict nested submodule directories", err, string(out)) + { + cmd := commandContext( + ctx, + "git", + "submodule", + "foreach", + "--recursive", + "git", + "clean", "-x", "-d", "-f", "-f", + ) + cmd.SetDir(r.LocalPath()) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsLocalErrorOr( + "unexpected error while defensively cleaning up after possible derelict nested submodule directories", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) + } } return nil @@ -148,26 +212,40 @@ func (r *bzrRepo) get(ctx context.Context) error { } } - out, err := runFromCwd(ctx, expensiveCmdTimeout, "bzr", "branch", r.Remote(), r.LocalPath()) - if err != nil { - return newVcsRemoteErrorOr("unable to get repository", err, string(out)) + cmd := commandContext(ctx, "bzr", "branch", r.Remote(), r.LocalPath()) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsRemoteErrorOr( + "unable to get repository", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) } return nil } func (r *bzrRepo) fetch(ctx context.Context) error { - out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "bzr", "pull") - if err != nil { - return newVcsRemoteErrorOr("unable to update repository", err, string(out)) + cmd := commandContext(ctx, "bzr", "pull") + cmd.SetDir(r.LocalPath()) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsRemoteErrorOr( + "unable to update repository", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) } return nil } func (r *bzrRepo) updateVersion(ctx context.Context, version string) error { - out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "bzr", "update", "-r", version) - if err != nil { - return newVcsLocalErrorOr("unable to update checked out version", err, string(out)) + cmd := commandContext(ctx, "bzr", "update", "-r", version) + cmd.SetDir(r.LocalPath()) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsLocalErrorOr( + "unable to update checked out version", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) } return nil } @@ -177,26 +255,40 @@ type hgRepo struct { } func (r *hgRepo) get(ctx context.Context) error { - out, err := runFromCwd(ctx, expensiveCmdTimeout, "hg", "clone", r.Remote(), r.LocalPath()) - if err != nil { - return newVcsRemoteErrorOr("unable to get repository", err, string(out)) + cmd := commandContext(ctx, "hg", "clone", r.Remote(), r.LocalPath()) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsRemoteErrorOr( + "unable to get repository", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) } return nil } func (r *hgRepo) fetch(ctx context.Context) error { - out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "hg", "pull") - if err != nil { - return newVcsRemoteErrorOr("unable to fetch latest changes", err, string(out)) + cmd := commandContext(ctx, "hg", "pull") + cmd.SetDir(r.LocalPath()) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsRemoteErrorOr( + "unable to fetch latest changes", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) } return nil } func (r *hgRepo) updateVersion(ctx context.Context, version string) error { - out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "hg", "update", version) - if err != nil { - return newVcsRemoteErrorOr("unable to update checked out version", err, string(out)) + cmd := commandContext(ctx, "hg", "update", version) + cmd.SetDir(r.LocalPath()) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsRemoteErrorOr( + "unable to update checked out version", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) } return nil @@ -214,27 +306,41 @@ func (r *svnRepo) get(ctx context.Context) error { remote = "file:///" + remote } - out, err := runFromCwd(ctx, expensiveCmdTimeout, "svn", "checkout", remote, r.LocalPath()) - if err != nil { - return newVcsRemoteErrorOr("unable to get repository", err, string(out)) + cmd := commandContext(ctx, "svn", "checkout", remote, r.LocalPath()) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsRemoteErrorOr( + "unable to get repository", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) } return nil } func (r *svnRepo) fetch(ctx context.Context) error { - out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "svn", "update") - if err != nil { - return newVcsRemoteErrorOr("unable to update repository", err, string(out)) + cmd := commandContext(ctx, "svn", "update") + cmd.SetDir(r.LocalPath()) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsRemoteErrorOr( + "unable to update repository", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) } - return err + return nil } func (r *svnRepo) updateVersion(ctx context.Context, version string) error { - out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "svn", "update", "-r", version) - if err != nil { - return newVcsRemoteErrorOr("unable to update checked out version", err, string(out)) + cmd := commandContext(ctx, "svn", "update", "-r", version) + cmd.SetDir(r.LocalPath()) + if out, err := cmd.CombinedOutput(); err != nil { + return newVcsRemoteErrorOr( + "unable to update checked out version", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) } return nil @@ -254,14 +360,18 @@ func (r *svnRepo) CommitInfo(id string) (*vcs.CommitInfo, error) { Commit commit `xml:"entry>commit"` } - out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "svn", "info", "-r", id, "--xml") + cmd := commandContext(ctx, "svn", "info", "-r", id, "--xml") + cmd.SetDir(r.LocalPath()) + out, err := cmd.CombinedOutput() if err != nil { - return nil, newVcsLocalErrorOr("unable to retrieve commit information", err, string(out)) + return nil, newVcsLocalErrorOr("unable to retrieve commit information", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) } infos := new(info) - err = xml.Unmarshal(out, &infos) - if err != nil { + if err := xml.Unmarshal(out, &infos); err != nil { return nil, newVcsLocalErrorOr("unable to retrieve commit information", err, string(out)) } @@ -271,9 +381,14 @@ func (r *svnRepo) CommitInfo(id string) (*vcs.CommitInfo, error) { } } - out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "svn", "log", "-r", id, "--xml") + cmd := commandContext(ctx, "svn", "log", "-r", id, "--xml") + cmd.SetDir(r.LocalPath()) + out, err := cmd.CombinedOutput() if err != nil { - return nil, newVcsRemoteErrorOr("unable to retrieve commit information", err, string(out)) + return nil, newVcsRemoteErrorOr("unable to retrieve commit information", + errors.Wrapf(err, "command failed: %v", cmd.Args()), + string(out), + ) } type logentry struct { @@ -288,8 +403,7 @@ func (r *svnRepo) CommitInfo(id string) (*vcs.CommitInfo, error) { } logs := new(log) - err = xml.Unmarshal(out, &logs) - if err != nil { + if err := xml.Unmarshal(out, &logs); err != nil { return nil, newVcsLocalErrorOr("unable to retrieve commit information", err, string(out)) } diff --git a/internal/gps/vcs_source.go b/internal/gps/vcs_source.go index 36f86c4dce..d2984d7c5b 100644 --- a/internal/gps/vcs_source.go +++ b/internal/gps/vcs_source.go @@ -12,7 +12,6 @@ import ( "os/exec" "path/filepath" "strings" - "time" "github.com/Masterminds/semver" "github.com/golang/dep/internal/fs" @@ -142,9 +141,12 @@ func (s *gitSource) exportRevisionTo(ctx context.Context, rev Revision, to strin // could have an err here...but it's hard to imagine how? defer fs.RenameWithFallback(bak, idx) - out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "git", "read-tree", rev.String()) - if err != nil { - return errors.Wrap(err, string(out)) + { + cmd := exec.CommandContext(ctx, "git", "read-tree", rev.String()) + cmd.Dir = r.LocalPath() + if out, err := cmd.CombinedOutput(); err != nil { + return errors.Wrap(err, string(out)) + } } // Ensure we have exactly one trailing slash @@ -158,9 +160,12 @@ func (s *gitSource) exportRevisionTo(ctx context.Context, rev Revision, to strin // though we have a bunch of housekeeping to do to set up, then tear // down, the sparse checkout controls, as well as restore the original // index and HEAD. - out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "git", "checkout-index", "-a", "--prefix="+to) - if err != nil { - return errors.Wrap(err, string(out)) + { + cmd := exec.CommandContext(ctx, "git", "checkout-index", "-a", "--prefix="+to) + cmd.Dir = r.LocalPath() + if out, err := cmd.CombinedOutput(); err != nil { + return errors.Wrap(err, string(out)) + } } return nil @@ -169,12 +174,10 @@ func (s *gitSource) exportRevisionTo(ctx context.Context, rev Revision, to strin func (s *gitSource) listVersions(ctx context.Context) (vlist []PairedVersion, err error) { r := s.repo - var out []byte - c := newMonitoredCmd(exec.Command("git", "ls-remote", r.Remote()), 30*time.Second) + cmd := exec.CommandContext(ctx, "git", "ls-remote", r.Remote()) // Ensure no prompting for PWs - c.cmd.Env = mergeEnvLists([]string{"GIT_ASKPASS=", "GIT_TERMINAL_PROMPT=0"}, os.Environ()) - out, err = c.combinedOutput(ctx) - + cmd.Env = append([]string{"GIT_ASKPASS=", "GIT_TERMINAL_PROMPT=0"}, os.Environ()...) + out, err := cmd.CombinedOutput() if err != nil { return nil, err } @@ -393,15 +396,18 @@ func (s *bzrSource) listVersions(ctx context.Context) ([]PairedVersion, error) { } // Now, list all the tags - out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "bzr", "tags", "--show-ids", "-v") + tagsCmd := exec.CommandContext(ctx, "bzr", "tags", "--show-ids", "-v") + tagsCmd.Dir = r.LocalPath() + out, err := tagsCmd.CombinedOutput() if err != nil { return nil, errors.Wrap(err, string(out)) } all := bytes.Split(bytes.TrimSpace(out), []byte("\n")) - var branchrev []byte - branchrev, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "bzr", "version-info", "--custom", "--template={revision_id}", "--revision=branch:.") + viCmd := exec.CommandContext(ctx, "bzr", "version-info", "--custom", "--template={revision_id}", "--revision=branch:.") + viCmd.Dir = r.LocalPath() + branchrev, err := viCmd.CombinedOutput() if err != nil { return nil, errors.Wrap(err, string(branchrev)) } @@ -472,7 +478,9 @@ func (s *hgSource) listVersions(ctx context.Context) ([]PairedVersion, error) { } // Now, list all the tags - out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "hg", "tags", "--debug", "--verbose") + tagsCmd := exec.CommandContext(ctx, "hg", "tags", "--debug", "--verbose") + tagsCmd.Dir = r.LocalPath() + out, err := tagsCmd.CombinedOutput() if err != nil { return nil, errors.Wrap(err, string(out)) } @@ -506,7 +514,9 @@ func (s *hgSource) listVersions(ctx context.Context) ([]PairedVersion, error) { // bookmarks next, because the presence of the magic @ bookmark has to // determine how we handle the branches var magicAt bool - out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "hg", "bookmarks", "--debug") + bookmarksCmd := exec.CommandContext(ctx, "hg", "bookmarks", "--debug") + bookmarksCmd.Dir = r.LocalPath() + out, err = bookmarksCmd.CombinedOutput() if err != nil { // better nothing than partial and misleading return nil, errors.Wrap(err, string(out)) @@ -539,7 +549,9 @@ func (s *hgSource) listVersions(ctx context.Context) ([]PairedVersion, error) { } } - out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "hg", "branches", "-c", "--debug") + cmd := exec.CommandContext(ctx, "hg", "branches", "-c", "--debug") + cmd.Dir = r.LocalPath() + out, err = cmd.CombinedOutput() if err != nil { // better nothing than partial and misleading return nil, errors.Wrap(err, string(out)) @@ -569,19 +581,3 @@ func (s *hgSource) listVersions(ctx context.Context) ([]PairedVersion, error) { return vlist, nil } - -// This func copied from Masterminds/vcs so we can exec our own commands -func mergeEnvLists(in, out []string) []string { -NextVar: - for _, inkv := range in { - k := strings.SplitAfterN(inkv, "=", 2)[0] - for i, outkv := range out { - if strings.HasPrefix(outkv, k) { - out[i] = inkv - continue NextVar - } - } - out = append(out, inkv) - } - return out -}