Share path resolution code for Windows

The path lookup is symmetric on all platforms and more code can be
shared. The only difference is that the command and argv0 must use
backslashes on Windows. The windows code can reuse the same path lookup
code, just with the modification that the paths sent from a Linux client
use forward slashes.

pkg/runner/local_runner.go

@@ -3,7 +3,10 @@ package runner
 import (
 	"context"
 	"errors"
+	"os"
 	"os/exec"
+	"path/filepath"
+	"strings"
 
 	"github.com/buildbarn/bb-remote-execution/pkg/proto/runner"
 	"github.com/buildbarn/bb-storage/pkg/filesystem"
@@ -201,3 +204,63 @@ func (r *localRunner) CheckReadiness(ctx context.Context, request *runner.CheckR
 
 	return &emptypb.Empty{}, nil
 }
+
+// getExecutablePath returns the path of an executable within a given
+// search path that is part of the PATH environment variable.
+func getExecutablePath(baseDirectory *path.Builder, searchPathStr, argv0 string) (string, error) {
+	searchPath, scopeWalker := baseDirectory.Join(path.VoidScopeWalker)
+	if err := path.Resolve(path.NewLocalParser(searchPathStr), scopeWalker); err != nil {
+		return "", err
+	}
+
+	executablePath, scopeWalker := searchPath.Join(path.VoidScopeWalker)
+	if err := path.Resolve(path.NewLocalParser(argv0), scopeWalker); err != nil {
+		return "", err
+	}
+	return path.GetLocalString(executablePath)
+}
+
+// lookupExecutable returns the path of an executable, taking the PATH
+// environment variable into account.
+// This operates on platform native paths, or unix-style slash paths.
+// The latter are customarily sent by Bazel in multiplatform builds.
+func lookupExecutable(workingDirectory *path.Builder, pathVariable, argv0 string) (string, error) {
+	if strings.ContainsRune(argv0, os.PathSeparator) {
+		// No PATH processing needs to be performed.
+		return argv0, nil
+	}
+
+	// Executable path does not contain any slashes. Perform PATH
+	// lookups.
+	//
+	// We cannot use exec.LookPath() directly, as that function
+	// disregards the working directory of the action. It also uses
+	// the PATH environment variable of the current process, as
+	// opposed to respecting the value that is provided as part of
+	// the action. Do call into this function to validate the
+	// existence of the executable.
+	for _, searchPathStr := range filepath.SplitList(pathVariable) {
+		executablePathAbs, err := getExecutablePath(workingDirectory, searchPathStr, argv0)
+		if err != nil {
+			return "", util.StatusWrapf(err, "Failed to resolve executable %#v in search path %#v", argv0, searchPathStr)
+		}
+		if _, err := exec.LookPath(executablePathAbs); err == nil {
+			// Regular compiled executables will receive the
+			// argv[0] that we provide, but scripts starting
+			// with '#!' will receive the literal executable
+			// path.
+			//
+			// Most shells seem to guarantee that if argv[0]
+			// is relative, the executable path is relative
+			// as well. Prevent these scripts from breaking
+			// by recomputing the executable path once more,
+			// but relative.
+			executablePathRel, err := getExecutablePath(&path.EmptyBuilder, searchPathStr, argv0)
+			if err != nil {
+				return "", util.StatusWrapf(err, "Failed to resolve executable %#v in search path %#v", argv0, searchPathStr)
+			}
+			return executablePathRel, nil
+		}
+	}
+	return "", status.Errorf(codes.InvalidArgument, "Cannot find executable %#v in search paths %#v", argv0, pathVariable)
+}

pkg/runner/local_runner_unix.go

@@ -7,7 +7,6 @@ import (
 	"context"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"strings"
 	"syscall"
 
@@ -16,69 +15,9 @@ import (
 	"github.com/buildbarn/bb-storage/pkg/util"
 
 	"golang.org/x/sys/unix"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/durationpb"
 )
 
-// getExecutablePath returns the path of an executable within a given
-// search path that is part of the PATH environment variable.
-func getExecutablePath(baseDirectory *path.Builder, searchPathStr, argv0 string) (string, error) {
-	searchPath, scopeWalker := baseDirectory.Join(path.VoidScopeWalker)
-	if err := path.Resolve(path.NewLocalParser(searchPathStr), scopeWalker); err != nil {
-		return "", err
-	}
-
-	executablePath, scopeWalker := searchPath.Join(path.VoidScopeWalker)
-	if err := path.Resolve(path.NewLocalParser(argv0), scopeWalker); err != nil {
-		return "", err
-	}
-	return path.GetLocalString(executablePath)
-}
-
-// lookupExecutable returns the path of an executable, taking the PATH
-// environment variable into account.
-func lookupExecutable(workingDirectory *path.Builder, pathVariable, argv0 string) (string, error) {
-	if strings.ContainsRune(argv0, os.PathSeparator) {
-		// No PATH processing needs to be performed.
-		return argv0, nil
-	}
-
-	// Executable path does not contain any slashes. Perform PATH
-	// lookups.
-	//
-	// We cannot use exec.LookPath() directly, as that function
-	// disregards the working directory of the action. It also uses
-	// the PATH environment variable of the current process, as
-	// opposed to respecting the value that is provided as part of
-	// the action. Do call into this function to validate the
-	// existence of the executable.
-	for _, searchPathStr := range filepath.SplitList(pathVariable) {
-		executablePathAbs, err := getExecutablePath(workingDirectory, searchPathStr, argv0)
-		if err != nil {
-			return "", util.StatusWrapf(err, "Failed to resolve executable %#v in search path %#v", argv0, searchPathStr)
-		}
-		if _, err := exec.LookPath(executablePathAbs); err == nil {
-			// Regular compiled executables will receive the
-			// argv[0] that we provide, but scripts starting
-			// with '#!' will receive the literal executable
-			// path.
-			//
-			// Most shells seem to guarantee that if argv[0]
-			// is relative, the executable path is relative
-			// as well. Prevent these scripts from breaking
-			// by recomputing the executable path once more,
-			// but relative.
-			executablePathRel, err := getExecutablePath(&path.EmptyBuilder, searchPathStr, argv0)
-			if err != nil {
-				return "", util.StatusWrapf(err, "Failed to resolve executable %#v in search path %#v", argv0, searchPathStr)
-			}
-			return executablePathRel, nil
-		}
-	}
-	return "", status.Errorf(codes.InvalidArgument, "Cannot find executable %#v in search paths %#v", argv0, pathVariable)
-}
-
 // NewPlainCommandCreator returns a CommandCreator for cases where we don't
 // need to chroot into the input root directory.
 func NewPlainCommandCreator(sysProcAttr *syscall.SysProcAttr) CommandCreator {

pkg/runner/local_runner_windows.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"syscall"
 
 	"github.com/buildbarn/bb-remote-execution/pkg/proto/resourceusage"
@@ -25,6 +26,14 @@ func NewPlainCommandCreator(sysProcAttr *syscall.SysProcAttr) CommandCreator {
 	return func(ctx context.Context, arguments []string, inputRootDirectory *path.Builder, workingDirectoryParser path.Parser, pathVariable string) (*exec.Cmd, error) {
 		// TODO: This may not work correctly if the action sets
 		// the PATH environment variable explicitly.
+		//
+		// exec.CommandContext() has some smartness to call
+		// exec.LookPath() under the hood, which we don't want.
+		// Call it with a placeholder path, followed by setting
+		// cmd.Path and cmd.Args manually. This ensures that our
+		// own values remain respected.
+		argv0 := arguments[0]
+		arguments[0] = "/nonexistent-place-back-later"
 		cmd := exec.CommandContext(ctx, arguments[0], arguments[1:]...)
 		cmd.SysProcAttr = sysProcAttr
 
@@ -39,6 +48,19 @@ func NewPlainCommandCreator(sysProcAttr *syscall.SysProcAttr) CommandCreator {
 			return nil, util.StatusWrap(err, "Failed to create local representation of working directory")
 		}
 		cmd.Dir = workingDirectoryStr
+
+		executablePath, err := lookupExecutable(workingDirectory, pathVariable, argv0)
+		if err != nil {
+			return nil, err
+		}
+
+		// Must use backslashes to execute relative paths
+		executablePath = filepath.FromSlash(executablePath)
+		arguments[0] = executablePath
+
+		cmd.Args = arguments
+		cmd.Path = executablePath
+		cmd.SysProcAttr = sysProcAttr
 		return cmd, nil
 	}
 }
@@ -54,6 +76,7 @@ var temporaryDirectoryEnvironmentVariablePrefixes = [...]string{"TMP=", "TEMP="}
 var invalidArgumentErrs = [...]error{exec.ErrNotFound, os.ErrPermission, os.ErrNotExist, windows.ERROR_BAD_EXE_FORMAT}
 
 func getPOSIXResourceUsage(cmd *exec.Cmd) *resourceusage.POSIXResourceUsage {
+	// TODO: These do not work
 	processState := cmd.ProcessState
 	return &resourceusage.POSIXResourceUsage{
 		UserTime:   durationpb.New(processState.SystemTime()),