diff --git a/clicommand/bootstrap.go b/clicommand/bootstrap.go index 7d7de3d0b5..e1c32ff30a 100644 --- a/clicommand/bootstrap.go +++ b/clicommand/bootstrap.go @@ -60,6 +60,7 @@ type BootstrapConfig struct { PullRequest string `cli:"pullrequest"` PullRequestUsingMergeRefspec bool `cli:"pull-request-using-merge-refspec"` GitSubmodules bool `cli:"git-submodules"` + GitLFSEnabled bool `cli:"git-lfs-enabled"` SSHKeyscan bool `cli:"ssh-keyscan"` AgentName string `cli:"agent" validate:"required"` Queue string `cli:"queue"` @@ -311,6 +312,11 @@ var BootstrapCommand = cli.Command{ Usage: "Enable git submodules (default: true)", EnvVar: "BUILDKITE_GIT_SUBMODULES", }, + cli.BoolFlag{ + Name: "git-lfs-enabled", + Usage: "Enable Git LFS object download during checkout (default: false)", + EnvVar: "BUILDKITE_GIT_LFS_ENABLED", + }, cli.BoolTFlag{ Name: "pty", Usage: "Run jobs within a pseudo terminal (default: true)", @@ -461,6 +467,7 @@ var BootstrapCommand = cli.Command{ GitCloneFlags: cfg.GitCloneFlags, GitCloneMirrorFlags: cfg.GitCloneMirrorFlags, GitFetchFlags: cfg.GitFetchFlags, + GitLFSEnabled: cfg.GitLFSEnabled, GitSparseCheckoutPaths: cfg.GitSparseCheckoutPaths, GitSSHKey: cfg.GitSSHKey, GitMirrorsLockTimeout: cfg.GitMirrorsLockTimeout, diff --git a/internal/job/checkout.go b/internal/job/checkout.go index 13f5efb54c..97f23d3fcc 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -262,6 +262,24 @@ func (e *Executor) checkout(ctx context.Context) error { break } + // Fail fast before any git work if git-lfs is required but missing. + // This operation only handles default checkout behavior, so it's possible for a custom checkout hook to require git-lfs but not have this check. That's a bit unfortunate, but we can add it to custom hooks later if needed. + // + // We probe via `git lfs version` rather than looking up `git-lfs` on + // PATH directly: git resolves subcommands via GIT_EXEC_PATH before + // falling back to PATH, so on platforms where git-lfs is bundled + // alongside git (notably Git for Windows) the binary is reachable to + // `git lfs ...` even when a PATH lookup would miss it. This matches + // the resolution path used by the actual LFS commands later. + if e.GitLFSEnabled { + // Leave stderr visible: when this probe fails it is almost always + // a misconfigured agent environment, and git's specific message + // (e.g. "'lfs' is not a git command") is the fastest diagnostic. + if _, err := e.shell.Command("git", "lfs", "version").RunAndCaptureStdout(ctx, shell.ShowStderr(true)); err != nil { + return fmt.Errorf("BUILDKITE_GIT_LFS_ENABLED=true but `git lfs version` failed; git-lfs may not be installed or not resolvable by git: %w", err) + } + } + maxAttempts := e.CheckoutAttempts if maxAttempts <= 0 { maxAttempts = 6 @@ -964,6 +982,15 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) { return fmt.Errorf("cleaning git repository: %w", err) } + // Install LFS filter before fetch so the filter is registered before any + // network operation, following the conventional git-lfs setup order. + if e.GitLFSEnabled { + e.shell.Commentf("Installing Git LFS filter") + if err := e.shell.Command("git", "lfs", "install", "--local").Run(ctx); err != nil { + return fmt.Errorf("installing git lfs filter: %w", err) + } + } + if err := e.fetchSource(ctx); err != nil { return err } @@ -1081,6 +1108,23 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) { } } + // When sparse-checkout is active, scope LFS to the same paths so we don't + // pull objects outside the sparse set (SUP-6529). If sparse fell back to a + // full checkout (e.g. git < 2.26), fetch unscoped so files outside the + // requested paths still get their LFS content. + if e.GitLFSEnabled { + lfsArgs := gitLFSFetchCheckoutArgs{ + Shell: e.shell, + Retry: true, + } + if sparseCheckoutActive { + lfsArgs.Include = cleanGitSparseCheckoutPaths(e.GitSparseCheckoutPaths) + } + if err := gitLFSFetchCheckout(ctx, lfsArgs); err != nil { + return err + } + } + // Git clean after checkout. We need to do this because submodules could have // changed in between the last checkout and this one. A double clean is the only // good solution to this problem that we've found diff --git a/internal/job/checkout_test.go b/internal/job/checkout_test.go index dca2a7019f..ca64b52b95 100644 --- a/internal/job/checkout_test.go +++ b/internal/job/checkout_test.go @@ -1,9 +1,12 @@ package job import ( + "fmt" "os" + "os/exec" "path/filepath" "runtime" + "strings" "testing" "time" @@ -442,3 +445,229 @@ func TestDefaultCheckoutPhase_DelayedRefCreation(t *testing.T) { t.Fatalf("tt.executor.defaultCheckoutPhase(ctx) error = %v, want nil", err) } } + +func TestGitLFSBinaryMissing(t *testing.T) { + // Not parallel: the test manipulates PATH via t.Setenv, which modifies + // process-global state. + + if runtime.GOOS == "windows" { + // Git for Windows bundles git-lfs.exe inside GIT_EXEC_PATH, which + // `git lfs ...` resolves before PATH. We can't reliably simulate + // "git-lfs unavailable to git" by restricting PATH on Windows. + t.Skip("Git for Windows bundles git-lfs.exe inside GIT_EXEC_PATH; precheck can't be broken via PATH") + } + + ctx := t.Context() + + // Provide a PATH where `git` is reachable but `git-lfs` is not, so the + // precheck's `git lfs version` exits non-zero with `'lfs' is not a git + // command`. This is the failure the precheck is meant to catch. + t.Setenv("PATH", gitOnlyBinDir(t)) + // Also clear GIT_EXEC_PATH so an inherited value (e.g. from Apple Git or + // a distro git package that bundles git-lfs in libexec/git-core) can't + // satisfy `git lfs ...` behind our back. + t.Setenv("GIT_EXEC_PATH", "") + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v, want nil", err) + } + + executor := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + Repository: "https://github.com/buildkite/agent.git", + GitLFSEnabled: true, + }, + } + + err = executor.checkout(ctx) + if err == nil { + t.Fatalf("executor.checkout(ctx) error = nil, want error containing %q", "git lfs version") + } + if !strings.Contains(err.Error(), "git lfs version") { + t.Errorf("executor.checkout(ctx) error = %q, want it to contain %q", err.Error(), "git lfs version") + } +} + +// gitOnlyBinDir returns a temp dir containing git (via a symlink on Unix or +// a .bat wrapper on Windows) but no git-lfs, so `git lfs ...` will fail with +// "'lfs' is not a git command" while plain git commands still work. +func gitOnlyBinDir(t *testing.T) string { + t.Helper() + gitBin, err := exec.LookPath("git") + if err != nil { + t.Fatalf("exec.LookPath(\"git\") error = %v", err) + } + binDir := t.TempDir() + if runtime.GOOS == "windows" { + // Use a .bat wrapper to avoid copying the multi-MB binary and to + // sidestep the symlink-privilege requirement on Windows. + wrapper := fmt.Sprintf("@echo off\r\n\"%s\" %%*\r\n", gitBin) + if err := os.WriteFile(filepath.Join(binDir, "git.bat"), []byte(wrapper), 0o755); err != nil { + t.Fatalf("os.WriteFile() error = %v", err) + } + } else { + if err := os.Symlink(gitBin, filepath.Join(binDir, "git")); err != nil { + t.Fatalf("os.Symlink() error = %v", err) + } + } + return binDir +} + +func TestDefaultCheckoutPhase_GitLFS(t *testing.T) { + // Not parallel: subtests manipulate PATH via t.Setenv, which modifies + // process-global state. + ctx := t.Context() + + t.Setenv("GIT_AUTHOR_NAME", "Buildkite Agent") + t.Setenv("GIT_AUTHOR_EMAIL", "agent@example.com") + t.Setenv("GIT_COMMITTER_NAME", "Buildkite Agent") + t.Setenv("GIT_COMMITTER_EMAIL", "agent@example.com") + + // fakeLFSBinDir returns a temp dir that has git (via gitOnlyBinDir) plus a + // fake git-lfs whose behaviour is defined by the provided scripts. + // unixScript is a #!/bin/sh script; winBatch is a .bat file body. + fakeLFSBinDir := func(t *testing.T, unixScript, winBatch string) string { + t.Helper() + binDir := gitOnlyBinDir(t) + if runtime.GOOS == "windows" { + if err := os.WriteFile(filepath.Join(binDir, "git-lfs.bat"), []byte(winBatch), 0o755); err != nil { + t.Fatalf("os.WriteFile() error = %v", err) + } + } else { + if err := os.WriteFile(filepath.Join(binDir, "git-lfs"), []byte(unixScript), 0o755); err != nil { + t.Fatalf("os.WriteFile() error = %v", err) + } + } + return binDir + } + + tests := []struct { + name string + lfsEnabled bool + setupPath func(t *testing.T) + wantErr string + }{ + { + name: "LFS disabled", + lfsEnabled: false, + }, + { + name: "LFS enabled binary present", + lfsEnabled: true, + setupPath: func(t *testing.T) { + if _, err := exec.LookPath("git-lfs"); err != nil { + t.Skip("git-lfs not installed") + } + }, + }, + { + name: "LFS enabled git lfs command fails", + lfsEnabled: true, + setupPath: func(t *testing.T) { + // Git for Windows ships its own git-lfs.exe inside + // GIT_EXEC_PATH, which git resolves before falling back to + // PATH. We can't fool git's subcommand lookup with a PATH + // override the way we can fool Go's exec.LookPath. + if runtime.GOOS == "windows" { + t.Skip("Not runnable on Windows: git for Windows uses bundled git-lfs.exe regardless of PATH") + } + t.Setenv("PATH", fakeLFSBinDir(t, + "#!/bin/sh\nexit 1\n", + "@echo off\r\nexit /b 1\r\n", + )) + }, + wantErr: "installing git lfs filter", + }, + { + name: "LFS enabled git lfs fetch fails", + lfsEnabled: true, + setupPath: func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Not runnable on Windows: git for Windows uses bundled git-lfs.exe regardless of PATH") + } + t.Setenv("PATH", fakeLFSBinDir(t, + "#!/bin/sh\ncase \"$1\" in\n install) exit 0 ;;\n *) exit 1 ;;\nesac\n", + "@echo off\r\nif \"%1\"==\"install\" exit /b 0\r\nexit /b 1\r\n", + )) + }, + wantErr: "git lfs fetch", + }, + } + + s := githttptest.NewServer() + t.Cleanup(s.Close) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Set up the remote repository BEFORE restricting PATH so that + // githttptest's git operations use the real git binary. + projectName := "test-" + strings.ReplaceAll(strings.ToLower(tt.name), " ", "-") + if err := s.CreateRepository(projectName); err != nil { + t.Fatalf("s.CreateRepository(%q) error = %v", projectName, err) + } + out, err := s.InitRepository(projectName) + if err != nil { + t.Fatalf("s.InitRepository(%q) error = %v, output: %s", projectName, err, out) + } + + // Restrict PATH after the repo is initialised. + if tt.setupPath != nil { + tt.setupPath(t) + } + + sh, err := shell.New() + if err != nil { + t.Fatalf("shell.New() error = %v", err) + } + + // Use os.MkdirTemp + best-effort cleanup rather than t.TempDir(): + // on Windows, git's child processes (credential helpers, git-lfs + // filter-process) can hold file handles open past their parent's + // exit, and t.TempDir()'s strict cleanup fails the test. + checkoutDir, err := os.MkdirTemp("", "checkout-path-") + if err != nil { + t.Fatalf("os.MkdirTemp() error = %v", err) + } + t.Cleanup(func() { + os.RemoveAll(checkoutDir) //nolint:errcheck // Best-effort cleanup. + }) + buildDir, err := os.MkdirTemp("", "build-path-") + if err != nil { + t.Fatalf("os.MkdirTemp() error = %v", err) + } + t.Cleanup(func() { + os.RemoveAll(buildDir) //nolint:errcheck // Best-effort cleanup. + }) + sh.Env.Set("BUILDKITE_BUILD_CHECKOUT_PATH", checkoutDir) + + executor := &Executor{ + shell: sh, + ExecutorConfig: ExecutorConfig{ + Commit: "HEAD", + Branch: "main", + GitCleanFlags: "-f -d -x", + BuildPath: buildDir, + Repository: s.RepoURL(projectName), + GitLFSEnabled: tt.lfsEnabled, + }, + } + + err = executor.defaultCheckoutPhase(ctx) + if tt.wantErr == "" { + if err != nil { + t.Errorf("defaultCheckoutPhase() error = %v, want nil", err) + } + return + } + if err == nil { + t.Errorf("defaultCheckoutPhase() error = nil, want error containing %q", tt.wantErr) + return + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("defaultCheckoutPhase() error = %q, want it to contain %q", err.Error(), tt.wantErr) + } + }) + } +} diff --git a/internal/job/config.go b/internal/job/config.go index 3370abaf4b..9f3b8d0778 100644 --- a/internal/job/config.go +++ b/internal/job/config.go @@ -53,6 +53,9 @@ type ExecutorConfig struct { // Should git submodules be checked out GitSubmodules bool `env:"BUILDKITE_GIT_SUBMODULES"` + // Whether to enable Git LFS operations during checkout + GitLFSEnabled bool `env:"BUILDKITE_GIT_LFS_ENABLED"` + // If the commit was part of a pull request, this will container the PR number PullRequest string diff --git a/internal/job/executor.go b/internal/job/executor.go index 0b88cd58bc..629b8239a9 100644 --- a/internal/job/executor.go +++ b/internal/job/executor.go @@ -916,6 +916,10 @@ func (e *Executor) setUp(ctx context.Context) (retErr error) { // Disable any interactive Git/SSH prompting e.shell.Env.Set("GIT_TERMINAL_PROMPT", "0") + // Force-set GIT_LFS_SKIP_SMUDGE=1 before any git operations so LFS + // objects are not downloaded automatically during checkout. + e.shell.Env.Set("GIT_LFS_SKIP_SMUDGE", "1") + // Fetch and set secrets before environment hook execution if e.Secrets != "" { if err := e.fetchAndSetSecrets(ctx); err != nil { diff --git a/internal/job/git.go b/internal/job/git.go index aef370940a..343385179e 100644 --- a/internal/job/git.go +++ b/internal/job/git.go @@ -33,6 +33,9 @@ const ( gitErrorClean gitErrorCleanSubmodules gitErrorRepack + // LFS fetch or checkout failure; distinct from gitErrorFetch because the + // gitFetch retry-clean/bad-object recovery paths don't apply to LFS. + gitErrorLFS ) const ( @@ -133,6 +136,73 @@ func gitCleanSubmodules(ctx context.Context, sh *shell.Shell, gitCleanFlags stri return nil } +type gitLFSFetchCheckoutArgs struct { + Shell *shell.Shell + Retry bool // Whether to retry the fetch+checkout on failure + // Include scopes LFS to these paths: passed as --include= to + // `git lfs fetch` and as positional pathspecs to `git lfs checkout`. + // Empty means fetch/checkout all LFS objects. + Include []string +} + +// gitLFSFetchCheckout fetches LFS objects for the current HEAD then materialises +// them. Fetch and checkout failures are wrapped with distinct messages so that a +// caller can tell which step failed from the error string alone. +// +// When args.Retry is true, the fetch+checkout pair is retried with exponential +// backoff to ride out transient network errors talking to the LFS server. +// Unlike gitFetch, we don't smelt for specific error strings: git-lfs uses +// different exit codes and error vocabulary than git itself, so we retry +// indiscriminately on any failure and rely on the retry budget to bound the +// damage from a genuinely permanent error. +// +// On exhaustion, the error is wrapped as a *gitError with WasRetried=true so +// that the outer checkout retrier in defaultCheckoutPhase's caller does not +// loop on top of this one — without that signal, a permanent LFS failure +// could be attempted 6 × 5 = 30 times instead of 5. +func gitLFSFetchCheckout(ctx context.Context, args gitLFSFetchCheckoutArgs) error { + retrier := roko.NewRetrier( + roko.WithStrategy(roko.Constant(0)), + roko.WithMaxAttempts(1), + ) + + if args.Retry { + retrier = roko.NewRetrier( + roko.WithStrategy(roko.ExponentialSubsecond(1*time.Second)), + roko.WithMaxAttempts(5), // 5 attempts will take ~16s + roko.WithJitter(), + ) + } + + fetchCmd := []string{"lfs", "fetch"} + checkoutCmd := []string{"lfs", "checkout"} + if len(args.Include) > 0 { + fetchCmd = append(fetchCmd, "--include="+strings.Join(args.Include, ",")) + checkoutCmd = append(checkoutCmd, args.Include...) + } + + err := retrier.DoWithContext(ctx, func(retrier *roko.Retrier) error { + if err := args.Shell.Command("git", fetchCmd...).Run(ctx); err != nil { + if args.Retry { + args.Shell.Commentf("%s", retrier) + } + return fmt.Errorf("git lfs fetch: %w", err) + } + if err := args.Shell.Command("git", checkoutCmd...).Run(ctx); err != nil { + if args.Retry { + args.Shell.Commentf("%s", retrier) + } + return fmt.Errorf("git lfs checkout: %w", err) + } + return nil + }) + + if err != nil && args.Retry { + return &gitError{error: err, Type: gitErrorLFS, WasRetried: args.Retry} + } + return err +} + func gitRepack(ctx context.Context, sh *shell.Shell, args ...string) error { commandArgs := []string{"repack"} commandArgs = append(commandArgs, args...) diff --git a/internal/job/git_test.go b/internal/job/git_test.go index de96125e88..8a6df06e28 100644 --- a/internal/job/git_test.go +++ b/internal/job/git_test.go @@ -310,3 +310,60 @@ func TestGitFetch(t *testing.T) { t.Errorf("executed commands diff (-got +want):\n%s", diff) } } + +func TestGitLFSFetchCheckout(t *testing.T) { + t.Parallel() + ctx := t.Context() + + var gotLog [][]string + sh := shell.NewTestShell(t, shell.WithDryRun(true), shell.WithCommandLog(&gotLog)) + + absoluteGit, err := sh.AbsolutePath("git") + if err != nil { + t.Fatalf("sh.AbsolutePath(git) = %v", err) + } + + if err := gitLFSFetchCheckout(ctx, gitLFSFetchCheckoutArgs{ + Shell: sh, + Retry: true, + }); err != nil { + t.Fatalf("gitLFSFetchCheckout(ctx, ...) = %v", err) + } + + wantLog := [][]string{ + {absoluteGit, "lfs", "fetch"}, + {absoluteGit, "lfs", "checkout"}, + } + if diff := cmp.Diff(gotLog, wantLog); diff != "" { + t.Errorf("executed commands diff (-got +want):\n%s", diff) + } +} + +func TestGitLFSFetchCheckoutWithInclude(t *testing.T) { + t.Parallel() + ctx := t.Context() + + var gotLog [][]string + sh := shell.NewTestShell(t, shell.WithDryRun(true), shell.WithCommandLog(&gotLog)) + + absoluteGit, err := sh.AbsolutePath("git") + if err != nil { + t.Fatalf("sh.AbsolutePath(git) = %v", err) + } + + if err := gitLFSFetchCheckout(ctx, gitLFSFetchCheckoutArgs{ + Shell: sh, + Retry: true, + Include: []string{"src/", "docs/"}, + }); err != nil { + t.Fatalf("gitLFSFetchCheckout(ctx, ...) = %v", err) + } + + wantLog := [][]string{ + {absoluteGit, "lfs", "fetch", "--include=src/,docs/"}, + {absoluteGit, "lfs", "checkout", "src/", "docs/"}, + } + if diff := cmp.Diff(gotLog, wantLog); diff != "" { + t.Errorf("executed commands diff (-got +want):\n%s", diff) + } +}