`executils` refactoring / fix "debug" command startup (#921)

* Added executils.Process helper

This helper should avoid common pitfalls arising mainly after the adoption
of the 'avrdude-stdin-hack'. For example see https://github.com/arduino/arduino-cli/pull/904
It also streamlines some of the most common operations (like process
Kill or Signal).

* Removed no more used executils.Command

* Run 'board list' test anyway

Even if no board are attached to the host system, it's always another
smoke-test that runs automatically.

Having this check enabled, for example, would have prevented:
https://github.com/arduino/arduino-cli/pull/904

arduino/cores/packagemanager/install_uninstall.go

@@ -54,13 +54,11 @@ func (pm *PackageManager) RunPostInstallScript(platformRelease *cores.PlatformRe
 	}
 	postInstall := platformRelease.InstallDir.Join(postInstallFilename)
 	if postInstall.Exist() && postInstall.IsNotDir() {
-		cmd, err := executils.Command(postInstall.String())
+		cmd, err := executils.NewProcessFromPath(postInstall)
 		if err != nil {
 			return err
 		}
-		cmd.Dir = platformRelease.InstallDir.String()
-		cmd.Stdout = nil
-		cmd.Stderr = nil
+		cmd.SetDirFromPath(platformRelease.InstallDir)
 		if err := cmd.Run(); err != nil {
 			return err
 		}

commands/bundled_tools_serial_discovery.go

@@ -131,13 +131,12 @@ func ListBoards(pm *packagemanager.PackageManager) ([]*BoardPort, error) {
 	}
 
 	// build the command to be executed
-	cmd, err := executils.Command(t.InstallDir.Join("serial-discovery").String())
+	cmd, err := executils.NewProcessFromPath(t.InstallDir.Join("serial-discovery"))
 	if err != nil {
 		return nil, errors.Wrap(err, "creating discovery process")
 	}
 
 	// attach in/out pipes to the process
-	cmd.Stdin = nil
 	in, err := cmd.StdinPipe()
 	if err != nil {
 		return nil, fmt.Errorf("creating stdin pipe for discovery: %s", err)
@@ -187,7 +186,7 @@ func ListBoards(pm *packagemanager.PackageManager) ([]*BoardPort, error) {
 	out.Close()
 	// kill the process if it takes too long to quit
 	time.AfterFunc(time.Second, func() {
-		cmd.Process.Kill()
+		cmd.Kill()
 	})
 	cmd.Wait()
 

commands/debug/debug.go

@@ -64,7 +64,7 @@ func Debug(ctx context.Context, req *dbg.DebugConfigReq, inStream io.Reader, out
 	}
 	entry.Debug("Executing debugger")
 
-	cmd, err := executils.Command(commandLine...)
+	cmd, err := executils.NewProcess(commandLine...)
 	if err != nil {
 		return nil, errors.Wrap(err, "Cannot execute debug tool")
 	}
@@ -77,8 +77,8 @@ func Debug(ctx context.Context, req *dbg.DebugConfigReq, inStream io.Reader, out
 	defer in.Close()
 
 	// Merge tool StdOut and StdErr to stream them in the io.Writer passed stream
-	cmd.Stdout = out
-	cmd.Stderr = out
+	cmd.RedirectStdoutTo(out)
+	cmd.RedirectStderrTo(out)
 
 	// Start the debug command
 	if err := cmd.Start(); err != nil {
@@ -91,7 +91,7 @@ func Debug(ctx context.Context, req *dbg.DebugConfigReq, inStream io.Reader, out
 				if sig, ok := <-interrupt; !ok {
 					break
 				} else {
-					cmd.Process.Signal(sig)
+					cmd.Signal(sig)
 				}
 			}
 		}()
@@ -103,7 +103,7 @@ func Debug(ctx context.Context, req *dbg.DebugConfigReq, inStream io.Reader, out
 		// In any case, try process termination after a second to avoid leaving
 		// zombie process.
 		time.Sleep(time.Second)
-		cmd.Process.Kill()
+		cmd.Kill()
 	}()
 
 	// Wait for process to finish

commands/upload/upload.go

@@ -373,13 +373,13 @@ func runTool(recipeID string, props *properties.Map, outStream, errStream io.Wri
 	if verbose {
 		outStream.Write([]byte(fmt.Sprintln(cmdLine)))
 	}
-	cmd, err := executils.Command(cmdArgs...)
+	cmd, err := executils.NewProcess(cmdArgs...)
 	if err != nil {
 		return fmt.Errorf("cannot execute upload tool: %s", err)
 	}
 
-	cmd.Stdout = outStream
-	cmd.Stderr = errStream
+	cmd.RedirectStdoutTo(outStream)
+	cmd.RedirectStderrTo(errStream)
 
 	if err := cmd.Start(); err != nil {
 		return fmt.Errorf("cannot execute upload tool: %s", err)

executils/executils.go

@@ -17,7 +17,6 @@ package executils
 
 import (
 	"bytes"
-	"fmt"
 	"io"
 	"os/exec"
 )
@@ -71,20 +70,3 @@ func call(stack []*exec.Cmd, pipes []*io.PipeWriter) (err error) {
 func TellCommandNotToSpawnShell(cmd *exec.Cmd) {
 	tellCommandNotToSpawnShell(cmd)
 }
-
-// Command creates a command with the provided command line arguments.
-// The first argument is the path to the executable, the remainder are the
-// arguments to the command.
-func Command(args ...string) (*exec.Cmd, error) {
-	if args == nil || len(args) == 0 {
-		return nil, fmt.Errorf("no executable specified")
-	}
-	cmd := exec.Command(args[0], args[1:]...)
-	TellCommandNotToSpawnShell(cmd)
-
-	// This is required because some tools detects if the program is running
-	// from terminal by looking at the stdin/out bindings.
-	// https://github.com/arduino/arduino-cli/issues/844
-	cmd.Stdin = NullReader
-	return cmd, nil
-}

executils/process.go

+// This file is part of arduino-cli.
+//
+// Copyright 2020 ARDUINO SA (http://www.arduino.cc/)
+//
+// This software is released under the GNU General Public License version 3,
+// which covers the main part of arduino-cli.
+// The terms of this license can be found at:
+// https://www.gnu.org/licenses/gpl-3.0.en.html
+//
+// You can be released from the requirements of the above licenses by purchasing
+// a commercial license. Buying such a license is mandatory if you want to
+// modify or otherwise use the software for commercial activities involving the
+// Arduino software without disclosing the source code of your own applications.
+// To purchase a commercial license, send an email to license@arduino.cc.
+
+package executils
+
+import (
+	"io"
+	"os"
+	"os/exec"
+
+	"github.com/arduino/go-paths-helper"
+	"github.com/pkg/errors"
+)
+
+// Process is representation of an external process run
+type Process struct {
+	cmd *exec.Cmd
+}
+
+// NewProcess creates a command with the provided command line arguments.
+// The first argument is the path to the executable, the remainder are the
+// arguments to the command.
+func NewProcess(args ...string) (*Process, error) {
+	if args == nil || len(args) == 0 {
+		return nil, errors.New("no executable specified")
+	}
+	p := &Process{
+		cmd: exec.Command(args[0], args[1:]...),
+	}
+	TellCommandNotToSpawnShell(p.cmd)
+
+	// This is required because some tools detects if the program is running
+	// from terminal by looking at the stdin/out bindings.
+	// https://github.com/arduino/arduino-cli/issues/844
+	p.cmd.Stdin = NullReader
+	return p, nil
+}
+
+// NewProcessFromPath creates a command from the provided executable path and
+// command line arguments.
+func NewProcessFromPath(executable *paths.Path, args ...string) (*Process, error) {
+	processArgs := []string{executable.String()}
+	processArgs = append(processArgs, args...)
+	return NewProcess(processArgs...)
+}
+
+// RedirectStdoutTo will redirect the process' stdout to the specified
+// writer. Any previous redirection will be overwritten.
+func (p *Process) RedirectStdoutTo(out io.Writer) {
+	p.cmd.Stdout = out
+}
+
+// RedirectStderrTo will redirect the process' stdout to the specified
+// writer. Any previous redirection will be overwritten.
+func (p *Process) RedirectStderrTo(out io.Writer) {
+	p.cmd.Stderr = out
+}
+
+// StdinPipe returns a pipe that will be connected to the command's standard
+// input when the command starts. The pipe will be closed automatically after
+// Wait sees the command exit. A caller need only call Close to force the pipe
+// to close sooner. For example, if the command being run will not exit until
+// standard input is closed, the caller must close the pipe.
+func (p *Process) StdinPipe() (io.WriteCloser, error) {
+	if p.cmd.Stdin == NullReader {
+		p.cmd.Stdin = nil
+	}
+	return p.cmd.StdinPipe()
+}
+
+// StdoutPipe returns a pipe that will be connected to the command's standard
+// output when the command starts.
+func (p *Process) StdoutPipe() (io.ReadCloser, error) {
+	return p.cmd.StdoutPipe()
+}
+
+// StderrPipe returns a pipe that will be connected to the command's standard
+// error when the command starts.
+func (p *Process) StderrPipe() (io.ReadCloser, error) {
+	return p.cmd.StderrPipe()
+}
+
+// Start will start the underliyng process.
+func (p *Process) Start() error {
+	return p.cmd.Start()
+}
+
+// Wait waits for the command to exit and waits for any copying to stdin or copying
+// from stdout or stderr to complete.
+func (p *Process) Wait() error {
+	// TODO: make some helpers to retrieve exit codes out of *ExitError.
+	return p.cmd.Wait()
+}
+
+// Signal sends a signal to the Process. Sending Interrupt on Windows is not implemented.
+func (p *Process) Signal(sig os.Signal) error {
+	return p.cmd.Process.Signal(sig)
+}
+
+// Kill causes the Process to exit immediately. Kill does not wait until the Process has
+// actually exited. This only kills the Process itself, not any other processes it may
+// have started.
+func (p *Process) Kill() error {
+	return p.cmd.Process.Kill()
+}
+
+// SetDir sets the working directory of the command. If Dir is the empty string, Run
+// runs the command in the calling process's current directory.
+func (p *Process) SetDir(dir string) {
+	p.cmd.Dir = dir
+}
+
+// SetDirFromPath sets the working directory of the command. If path is nil, Run
+// runs the command in the calling process's current directory.
+func (p *Process) SetDirFromPath(path *paths.Path) {
+	if path == nil {
+		p.cmd.Dir = ""
+	} else {
+		p.cmd.Dir = path.String()
+	}
+}
+
+// Run starts the specified command and waits for it to complete.
+func (p *Process) Run() error {
+	return p.cmd.Run()
+}

test/test_board.py

@@ -372,7 +372,6 @@ gold_board = """
 """  # noqa: E501
 
 
-@pytest.mark.skipif(running_on_ci(), reason="VMs have no serial ports")
 def test_board_list(run_command):
     result = run_command("core update-index")
     assert result.ok