Merge pull request #3000 from buildkite/fix-homedir

Prefer $HOME on all platforms

clicommand/agent_start.go

@@ -30,7 +30,7 @@ import (
 	"github.com/buildkite/agent/v3/internal/experiments"
 	"github.com/buildkite/agent/v3/internal/job/hook"
 	"github.com/buildkite/agent/v3/internal/job/shell"
-	"github.com/buildkite/agent/v3/internal/utils"
+	"github.com/buildkite/agent/v3/internal/osutil"
 	"github.com/buildkite/agent/v3/logger"
 	"github.com/buildkite/agent/v3/metrics"
 	"github.com/buildkite/agent/v3/process"
@@ -1133,7 +1133,7 @@ var AgentStartCommand = cli.Command{
 
 		// confirm the BuildPath is exists. The bootstrap is going to write to it when a job executes,
 		// so we may as well check that'll work now and fail early if it's a problem
-		if !utils.FileExists(agentConf.BuildPath) {
+		if !osutil.FileExists(agentConf.BuildPath) {
 			l.Info("Build Path doesn't exist, creating it (%s)", agentConf.BuildPath)
 			// Actual file permissions will be reduced by umask, and won't be 0777 unless the user has manually changed the umask to 000
 			if err := os.MkdirAll(agentConf.BuildPath, 0o777); err != nil {
@@ -1383,7 +1383,7 @@ func agentLifecycleHook(hookName string, log logger.Logger, cfg AgentStartConfig
 }
 
 func defaultSocketsPath() string {
-	home, err := os.UserHomeDir()
+	home, err := osutil.UserHomeDir()
 	if err != nil {
 		return filepath.Join(os.TempDir(), "buildkite-sockets")
 	}

cliconfig/file.go

@@ -7,7 +7,7 @@ import (
 	"os"
 	"strings"
 
-	"github.com/buildkite/agent/v3/internal/utils"
+	"github.com/buildkite/agent/v3/internal/osutil"
 )
 
 type File struct {
@@ -60,7 +60,7 @@ func (f *File) Load() error {
 }
 
 func (f File) AbsolutePath() (string, error) {
-	return utils.NormalizeFilePath(f.Path)
+	return osutil.NormalizeFilePath(f.Path)
 }
 
 func (f File) Exists() bool {

cliconfig/loader.go

@@ -12,7 +12,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/buildkite/agent/v3/internal/utils"
+	"github.com/buildkite/agent/v3/internal/osutil"
 	"github.com/buildkite/agent/v3/logger"
 	"github.com/oleiade/reflections"
 	"github.com/urfave/cli"
@@ -372,7 +372,7 @@ func (l Loader) normalizeField(fieldName string, normalization string) error {
 
 		// Normalize the field to be a filepath
 		if valueAsString, ok := value.(string); ok {
-			normalizedPath, err := utils.NormalizeFilePath(valueAsString)
+			normalizedPath, err := osutil.NormalizeFilePath(valueAsString)
 			if err != nil {
 				return err
 			}
@@ -392,7 +392,7 @@ func (l Loader) normalizeField(fieldName string, normalization string) error {
 
 		// Normalize the field to be a command
 		if valueAsString, ok := value.(string); ok {
-			normalizedCommandPath, err := utils.NormalizeCommand(valueAsString)
+			normalizedCommandPath, err := osutil.NormalizeCommand(valueAsString)
 			if err != nil {
 				return err
 			}

internal/job/checkout.go

@@ -11,7 +11,7 @@ import (
 
 	"github.com/buildkite/agent/v3/internal/experiments"
 	"github.com/buildkite/agent/v3/internal/job/shell"
-	"github.com/buildkite/agent/v3/internal/utils"
+	"github.com/buildkite/agent/v3/internal/osutil"
 	"github.com/buildkite/agent/v3/tracetools"
 	"github.com/buildkite/roko"
 )
@@ -80,7 +80,7 @@ func (e *Executor) removeCheckoutDir() error {
 func (e *Executor) createCheckoutDir() error {
 	checkoutPath, _ := e.shell.Env.Get("BUILDKITE_BUILD_CHECKOUT_PATH")
 
-	if !utils.FileExists(checkoutPath) {
+	if !osutil.FileExists(checkoutPath) {
 		e.shell.Commentf("Creating \"%s\"", checkoutPath)
 		// Actual file permissions will be reduced by umask, and won't be 0777 unless the user has manually changed the umask to 000
 		if err := os.MkdirAll(checkoutPath, 0777); err != nil {
@@ -258,7 +258,7 @@ func (e *Executor) CheckoutPhase(ctx context.Context) error {
 }
 
 func hasGitSubmodules(sh *shell.Shell) bool {
-	return utils.FileExists(filepath.Join(sh.Getwd(), ".gitmodules"))
+	return osutil.FileExists(filepath.Join(sh.Getwd(), ".gitmodules"))
 }
 
 func hasGitCommit(ctx context.Context, sh *shell.Shell, gitDir string, commit string) bool {
@@ -283,7 +283,7 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string) (stri
 	isMainRepository := repository == e.Repository
 
 	// Create the mirrors path if it doesn't exist
-	if baseDir := filepath.Dir(mirrorDir); !utils.FileExists(baseDir) {
+	if baseDir := filepath.Dir(mirrorDir); !osutil.FileExists(baseDir) {
 		e.shell.Commentf("Creating \"%s\"", baseDir)
 		// Actual file permissions will be reduced by umask, and won't be 0777 unless the user has manually changed the umask to 000
 		if err := os.MkdirAll(baseDir, 0777); err != nil {
@@ -309,7 +309,7 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string) (stri
 	defer mirrorCloneLock.Unlock()
 
 	// If we don't have a mirror, we need to clone it
-	if !utils.FileExists(mirrorDir) {
+	if !osutil.FileExists(mirrorDir) {
 		e.shell.Commentf("Cloning a mirror of the repository to %q", mirrorDir)
 		flags := "--mirror " + e.GitCloneMirrorFlags
 		if err := gitClone(ctx, e.shell, flags, repository, mirrorDir); err != nil {
@@ -455,7 +455,7 @@ func (e *Executor) getOrUpdateMirrorDir(ctx context.Context, repository string)
 		e.shell.Commentf("Skipping update and using existing mirror for repository %s at %s.", repository, mirrorDir)
 
 		// Check if specified mirrorDir exists, otherwise the clone will fail.
-		if !utils.FileExists(mirrorDir) {
+		if !osutil.FileExists(mirrorDir) {
 			// Fall back to a clean clone, rather than failing the clone and therefore the build
 			e.shell.Commentf("No existing mirror found for repository %s at %s.", repository, mirrorDir)
 			mirrorDir = ""
@@ -507,7 +507,7 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error {
 
 	// Does the git directory exist?
 	existingGitDir := filepath.Join(e.shell.Getwd(), ".git")
-	if utils.FileExists(existingGitDir) {
+	if osutil.FileExists(existingGitDir) {
 		// Update the origin of the repository so we can gracefully handle
 		// repository renames
 		if _, err := e.updateRemoteURL(ctx, "", e.Repository); err != nil {
@@ -647,7 +647,7 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error {
 
 				// Tests use a local temp path for the repository, real repositories don't. Handle both.
 				var repositoryPath string
-				if !utils.FileExists(repository) {
+				if !osutil.FileExists(repository) {
 					repositoryPath = filepath.Join(e.ExecutorConfig.GitMirrorsPath, dirForRepository(repository))
 				} else {
 					repositoryPath = repository

internal/job/executor.go

@@ -26,11 +26,11 @@ import (
 	"github.com/buildkite/agent/v3/internal/file"
 	"github.com/buildkite/agent/v3/internal/job/hook"
 	"github.com/buildkite/agent/v3/internal/job/shell"
+	"github.com/buildkite/agent/v3/internal/osutil"
 	"github.com/buildkite/agent/v3/internal/redact"
 	"github.com/buildkite/agent/v3/internal/replacer"
 	"github.com/buildkite/agent/v3/internal/shellscript"
 	"github.com/buildkite/agent/v3/internal/tempfile"
-	"github.com/buildkite/agent/v3/internal/utils"
 	"github.com/buildkite/agent/v3/kubernetes"
 	"github.com/buildkite/agent/v3/process"
 	"github.com/buildkite/agent/v3/tracetools"
@@ -338,7 +338,7 @@ func (e *Executor) executeHook(ctx context.Context, hookCfg HookConfig) error {
 	}
 	hookName += " " + hookCfg.Name
 
-	if !utils.FileExists(hookCfg.Path) {
+	if !osutil.FileExists(hookCfg.Path) {
 		if e.Debug {
 			e.shell.Commentf("Skipping %s hook, no script at \"%s\"", hookName, hookCfg.Path)
 		}
@@ -420,7 +420,7 @@ func logOpenedHookInfo(l shell.Logger, debug bool, hookName, path string) {
 		} else {
 			l.Errorf("The %s hook failed to run the %s process has the hook file open", hookName, procPath)
 		}
-	case utils.FileExists("/dev/fd"):
+	case osutil.FileExists("/dev/fd"):
 		isOpened, err := file.IsOpened(l, debug, path)
 		if err == nil {
 			if isOpened {
@@ -715,7 +715,7 @@ func dirForRepository(repository string) string {
 
 // Given a repository, it will add the host to the set of SSH known_hosts on the machine
 func addRepositoryHostToSSHKnownHosts(ctx context.Context, sh *shell.Shell, repository string) {
-	if utils.FileExists(repository) {
+	if osutil.FileExists(repository) {
 		return
 	}
 
@@ -965,7 +965,7 @@ func (e *Executor) defaultCommandPhase(ctx context.Context) error {
 
 	scriptFileName := strings.Replace(e.Command, "\n", "", -1)
 	pathToCommand, err := filepath.Abs(filepath.Join(e.shell.Getwd(), scriptFileName))
-	commandIsScript := err == nil && utils.FileExists(pathToCommand)
+	commandIsScript := err == nil && osutil.FileExists(pathToCommand)
 	span.AddAttributes(map[string]string{"hook.command": pathToCommand})
 
 	// If the command isn't a script, then it's something we need
@@ -1039,7 +1039,7 @@ func (e *Executor) defaultCommandPhase(ctx context.Context) error {
 		// shouldn't be vulnerable to this!
 		if e.ExecutorConfig.CommandEval {
 			// Make script executable
-			if err = utils.ChmodExecutable(pathToCommand); err != nil {
+			if err = osutil.ChmodExecutable(pathToCommand); err != nil {
 				e.shell.Warningf("Error marking script %q as executable: %v", pathToCommand, err)
 				return err
 			}

internal/job/hook/hook.go

@@ -10,7 +10,7 @@ import (
 	"runtime"
 
 	"github.com/buildkite/agent/v3/internal/job/shell"
-	"github.com/buildkite/agent/v3/internal/utils"
+	"github.com/buildkite/agent/v3/internal/osutil"
 )
 
 // Find returns the absolute path to the best matching hook file in a path, or
@@ -23,7 +23,7 @@ func Find(hookDir string, name string) (string, error) {
 		}
 	}
 	// otherwise chech for th default shell script
-	if p := filepath.Join(hookDir, name); utils.FileExists(p) {
+	if p := filepath.Join(hookDir, name); osutil.FileExists(p) {
 		return p, nil
 	}
 	// Don't wrap os.ErrNotExist without checking callers handle it.

internal/job/knownhosts.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/buildkite/agent/v3/internal/job/shell"
+	"github.com/buildkite/agent/v3/internal/osutil"
 	"golang.org/x/crypto/ssh/knownhosts"
 )
 
@@ -19,7 +20,7 @@ type knownHosts struct {
 }
 
 func findKnownHosts(sh *shell.Shell) (*knownHosts, error) {
-	userHomePath, err := os.UserHomeDir()
+	userHomePath, err := osutil.UserHomeDir()
 	if err != nil {
 		return nil, fmt.Errorf("Could not find the current users home directory (%s)", err)
 	}

internal/job/plugin.go

@@ -13,7 +13,7 @@ import (
 
 	"github.com/buildkite/agent/v3/agent/plugin"
 	"github.com/buildkite/agent/v3/internal/job/hook"
-	"github.com/buildkite/agent/v3/internal/utils"
+	"github.com/buildkite/agent/v3/internal/osutil"
 	"github.com/buildkite/roko"
 )
 
@@ -162,7 +162,7 @@ func (e *Executor) VendoredPluginPhase(ctx context.Context) error {
 			return fmt.Errorf("Failed to resolve vendored plugin path for plugin %s: %w", p.Name(), err)
 		}
 
-		if !utils.FileExists(pluginLocation) {
+		if !osutil.FileExists(pluginLocation) {
 			return fmt.Errorf("Vendored plugin path %s doesn't exist", p.Location)
 		}
 
@@ -324,7 +324,7 @@ func (e *Executor) checkoutPlugin(ctx context.Context, p *plugin.Plugin) (*plugi
 	// that means a potentially slow and unnecessary clone on every build step.  Sigh.  I think the
 	// tradeoff is favourable for just blowing away an existing clone if we want least-hassle
 	// guarantee that the user will get the latest version of their plugin branch/tag/whatever.
-	if e.ExecutorConfig.PluginsAlwaysCloneFresh && utils.FileExists(pluginDirectory) {
+	if e.ExecutorConfig.PluginsAlwaysCloneFresh && osutil.FileExists(pluginDirectory) {
 		e.shell.Commentf("BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH is true; removing previous checkout of plugin %s", p.Label())
 		err = os.RemoveAll(pluginDirectory)
 		if err != nil {
@@ -333,7 +333,7 @@ func (e *Executor) checkoutPlugin(ctx context.Context, p *plugin.Plugin) (*plugi
 		}
 	}
 
-	if utils.FileExists(pluginGitDirectory) {
+	if osutil.FileExists(pluginGitDirectory) {
 		// It'd be nice to show the current commit of the plugin, so
 		// let's figure that out.
 		headCommit, err := gitRevParseInWorkingDirectory(ctx, e.shell, pluginDirectory, "--short=7", "HEAD")

internal/osutil/doc.go

@@ -0,0 +1,2 @@
+// Package osutil provides some OS-level helper functions.
+package osutil

internal/utils/file.go → internal/osutil/file.go

@@ -1,4 +1,4 @@
-package utils
+package osutil
 
 import (
 	"fmt"

internal/osutil/homedir.go

@@ -0,0 +1,12 @@
+package osutil
+
+import "os"
+
+// UserHomeDir is similar to os.UserHomeDir, but prefers $HOME when available
+// over other options (such as USERPROFILE on Windows).
+func UserHomeDir() (string, error) {
+	if h := os.Getenv("HOME"); h != "" {
+		return h, nil
+	}
+	return os.UserHomeDir()
+}

internal/osutil/homedir_test.go

@@ -0,0 +1,50 @@
+package osutil
+
+import (
+	"os"
+	"runtime"
+	"testing"
+)
+
+func TestUserHomeDir(t *testing.T) {
+	// not parallel because it messes with env vars
+	origHome := os.Getenv("HOME")
+	origUserProfile := os.Getenv("USERPROFILE")
+	t.Cleanup(func() {
+		os.Setenv("HOME", origHome)
+		os.Setenv("USERPROFILE", origUserProfile)
+	})
+
+	type testCase struct {
+		home, userProfile, want string
+	}
+
+	tests := []testCase{
+		{
+			// Prefer $HOME on all platforms
+			home:        "home",
+			userProfile: "userProfile",
+			want:        "home",
+		},
+	}
+	if runtime.GOOS == "windows" {
+		// Windows can use %USERPROFILE% as a treat when $HOME is unavailable
+		tests = append(tests, testCase{
+			home:        "",
+			userProfile: "userProfile",
+			want:        "userProfile",
+		})
+	}
+
+	for _, test := range tests {
+		os.Setenv("HOME", test.home)
+		os.Setenv("USERPROFILE", test.userProfile)
+		got, err := UserHomeDir()
+		if err != nil {
+			t.Errorf("HOME=%q USERPROFILE=%q UserHomeDir() error = %v", test.home, test.userProfile, err)
+		}
+		if got != test.want {
+			t.Errorf("HOME=%q USERPROFILE=%q UserHomeDir() = %q, want %q", test.home, test.userProfile, got, test.want)
+		}
+	}
+}

internal/utils/path.go → internal/osutil/path.go

@@ -1,4 +1,4 @@
-package utils
+package osutil
 
 import (
 	"errors"

internal/utils/path_test.go → internal/osutil/path_test.go

@@ -1,4 +1,4 @@
-package utils
+package osutil
 
 import (
 	"os"

internal/utils/path_windows_test.go → internal/osutil/path_windows_test.go

@@ -1,7 +1,7 @@
 //go:build windows
 // +build windows
 
-package utils
+package osutil
 
 import (
 	"os"