feat(internal/command): add command execution package with logging (#2968)

An internal/command package is created to execute external commands and
print them to stderr before execution, in order to provide progress
reports to the user.

Existing call sites from internal/sidekick/external are migrated to use
the new package and the old package is deleted.

For #2966

Author: julieqiu

internal/sidekick/external/external.go renamed to internal/command/command.go

@@ -12,25 +12,21 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// Package external provides helper functions to work with external commands.
-package external
+// Package command provides helpers to execute external commands with logging.
+package command
 
 import (
 	"fmt"
+	"os"
 	"os/exec"
 )
 
-// Exec executes a command and captures any error output.
-func Exec(cmd *exec.Cmd) error {
+// Run executes a program (with arguments) and captures any error output.
+func Run(command string, arg ...string) error {
+	cmd := exec.Command(command, arg...)
+	fmt.Fprintf(os.Stderr, "Running: %s\n", cmd.String())
 	if output, err := cmd.CombinedOutput(); err != nil {
 		return fmt.Errorf("%v: %v\n%s", cmd, err, output)
 	}
 	return nil
 }
-
-// Run executes a program (with arguments) and captures any error output.
-func Run(command string, arg ...string) error {
-	cmd := exec.Command(command, arg...)
-	cmd.Dir = "."
-	return Exec(cmd)
-}

internal/sidekick/external/external_test.go renamed to internal/command/command_test.go

@@ -12,32 +12,25 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package external
+package command
 
 import (
-	"os/exec"
+	"strings"
 	"testing"
 )
 
-func TestSuccess(t *testing.T) {
-	// "go" must be installed, otherwise: how are you running the unit tests?
-	cmd := exec.Command("go", "help")
-	if err := Exec(cmd); err != nil {
+func TestRun(t *testing.T) {
+	if err := Run("go", "version"); err != nil {
 		t.Fatal(err)
 	}
 }
 
-func TestError(t *testing.T) {
-	// Seems unlikely that `go` will gain this subcommand. I will buy you a cold
-	// beverage if I am wrong.
-	cmd := exec.Command("go", "invalid-subcommand-bad-bad-bad")
-	if err := Exec(cmd); err == nil {
-		t.Errorf("expected an error using go invalid-subcommand-bad-bad-bad")
+func TestRunError(t *testing.T) {
+	err := Run("go", "invalid-subcommand-bad-bad-bad")
+	if err == nil {
+		t.Fatal("expected error, got nil")
 	}
-}
-
-func TestRun(t *testing.T) {
-	if err := Run("go", "help"); err != nil {
-		t.Fatal(err)
+	if !strings.Contains(err.Error(), "invalid-subcommand-bad-bad-bad") {
+		t.Errorf("error should mention the invalid subcommand, got: %v", err)
 	}
 }

internal/sidekick/dart/dart.go

@@ -21,8 +21,8 @@ import (
 	"strings"
 	"unicode"
 
+	"github.com/googleapis/librarian/internal/command"
 	"github.com/googleapis/librarian/internal/sidekick/api"
-	"github.com/googleapis/librarian/internal/sidekick/external"
 	"github.com/iancoleman/strcase"
 )
 
@@ -257,7 +257,7 @@ func shouldGenerateMethod(m *api.Method) bool {
 }
 
 func formatDirectory(dir string) error {
-	if err := external.Run("dart", "format", dir); err != nil {
+	if err := command.Run("dart", "format", dir); err != nil {
 		return fmt.Errorf("got an error trying to run `dart format`; perhaps try https://dart.dev/get-dart (%w)", err)
 	}
 	return nil

internal/sidekick/rust_prost/generate.go

@@ -21,9 +21,9 @@ import (
 	"os/exec"
 	"path/filepath"
 
+	"github.com/googleapis/librarian/internal/command"
 	"github.com/googleapis/librarian/internal/sidekick/api"
 	"github.com/googleapis/librarian/internal/sidekick/config"
-	"github.com/googleapis/librarian/internal/sidekick/external"
 	"github.com/googleapis/librarian/internal/sidekick/language"
 )
 
@@ -35,10 +35,10 @@ func Generate(model *api.API, outdir string, cfg *config.Config) error {
 	if cfg.General.SpecificationFormat != "protobuf" {
 		return fmt.Errorf("the `rust+prost` generator only supports `protobuf` as a specification source, outdir=%s", outdir)
 	}
-	if err := external.Run("cargo", "--version"); err != nil {
+	if err := command.Run("cargo", "--version"); err != nil {
 		return fmt.Errorf("got an error trying to run `cargo --version`, the instructions on https://www.rust-lang.org/learn/get-started may solve this problem: %w", err)
 	}
-	if err := external.Run("protoc", "--version"); err != nil {
+	if err := command.Run("protoc", "--version"); err != nil {
 		return fmt.Errorf("got an error trying to run `protoc --version`, the instructions on https://grpc.io/docs/protoc-installation/ may solve this problem: %w", err)
 	}
 
@@ -81,5 +81,9 @@ func buildRS(rootName, tmpDir, outDir string) error {
 	cmd.Dir = tmpDir
 	cmd.Env = append(os.Environ(), fmt.Sprintf("SOURCE_ROOT=%s", absRoot))
 	cmd.Env = append(cmd.Env, fmt.Sprintf("DEST=%s", absOutDir))
-	return external.Exec(cmd)
+	fmt.Fprintf(os.Stderr, "Running: %s\n", cmd.String())
+	if output, err := cmd.CombinedOutput(); err != nil {
+		return fmt.Errorf("%v: %v\n%s", cmd, err, output)
+	}
+	return nil
 }

internal/sidekick/rust_release/bump_versions.go

@@ -19,8 +19,8 @@ import (
 	"log/slog"
 	"slices"
 
+	"github.com/googleapis/librarian/internal/command"
 	"github.com/googleapis/librarian/internal/sidekick/config"
-	"github.com/googleapis/librarian/internal/sidekick/external"
 )
 
 // BumpVersions finds all the crates that need a version bump and performs the
@@ -54,7 +54,7 @@ func BumpVersions(config *config.Release) error {
 	}
 	for _, name := range crates {
 		slog.Info("runnning cargo semver-checks", "crate", name)
-		if err := external.Run(cargoExe(config), "semver-checks", "--all-features", "-p", name); err != nil {
+		if err := command.Run(cargoExe(config), "semver-checks", "--all-features", "-p", name); err != nil {
 			return err
 		}
 	}

internal/sidekick/rust_release/bump_versions_test.go

@@ -23,8 +23,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/googleapis/librarian/internal/command"
 	"github.com/googleapis/librarian/internal/sidekick/config"
-	"github.com/googleapis/librarian/internal/sidekick/external"
 )
 
 const (
@@ -58,7 +58,7 @@ func TestBumpVersionsSuccess(t *testing.T) {
 	if err := os.WriteFile(name, []byte(newLibRsContents), 0644); err != nil {
 		t.Fatal(err)
 	}
-	if err := external.Run("git", "commit", "-m", "feat: changed storage", "."); err != nil {
+	if err := command.Run("git", "commit", "-m", "feat: changed storage", "."); err != nil {
 		t.Fatal(err)
 	}
 	if err := BumpVersions(config); err != nil {
@@ -87,7 +87,7 @@ func TestBumpVersionsNoCargoTools(t *testing.T) {
 	if err := os.WriteFile(name, []byte(newLibRsContents), 0644); err != nil {
 		t.Fatal(err)
 	}
-	if err := external.Run("git", "commit", "-m", "feat: changed storage", "."); err != nil {
+	if err := command.Run("git", "commit", "-m", "feat: changed storage", "."); err != nil {
 		t.Fatal(err)
 	}
 	if err := BumpVersions(config); err != nil {
@@ -116,7 +116,7 @@ func TestBumpVersionsNoSemverChecks(t *testing.T) {
 	if err := os.WriteFile(name, []byte(newLibRsContents), 0644); err != nil {
 		t.Fatal(err)
 	}
-	if err := external.Run("git", "commit", "-m", "feat: changed storage", "."); err != nil {
+	if err := command.Run("git", "commit", "-m", "feat: changed storage", "."); err != nil {
 		t.Fatal(err)
 	}
 	if err := BumpVersions(config); err != nil {
@@ -167,7 +167,7 @@ func TestBumpVersionsManifestError(t *testing.T) {
 	if err := os.WriteFile(name, []byte("invalid-toml-file = {"), 0644); err != nil {
 		t.Fatal(err)
 	}
-	if err := external.Run("git", "commit", "-m", "feat: broke storage manifest file", "."); err != nil {
+	if err := command.Run("git", "commit", "-m", "feat: broke storage manifest file", "."); err != nil {
 		t.Fatal(err)
 	}
 	if err := BumpVersions(config); err == nil {
@@ -179,12 +179,12 @@ func setupForVersionBump(t *testing.T, wantTag string) {
 	remoteDir := t.TempDir()
 	continueInNewGitRepository(t, remoteDir)
 	initRepositoryContents(t)
-	if err := external.Run("git", "tag", wantTag); err != nil {
+	if err := command.Run("git", "tag", wantTag); err != nil {
 		t.Fatal(err)
 	}
 	cloneDir := t.TempDir()
 	t.Chdir(cloneDir)
-	if err := external.Run("git", "clone", remoteDir, "."); err != nil {
+	if err := command.Run("git", "clone", remoteDir, "."); err != nil {
 		t.Fatal(err)
 	}
 	configNewGitRepository(t)
@@ -194,14 +194,14 @@ func setupForPublish(t *testing.T, wantTag string) string {
 	remoteDir := t.TempDir()
 	continueInNewGitRepository(t, remoteDir)
 	initRepositoryContents(t)
-	if err := external.Run("git", "tag", wantTag); err != nil {
+	if err := command.Run("git", "tag", wantTag); err != nil {
 		t.Fatal(err)
 	}
 	name := path.Join("src", "storage", "src", "lib.rs")
 	if err := os.WriteFile(name, []byte(newLibRsContents), 0644); err != nil {
 		t.Fatal(err)
 	}
-	if err := external.Run("git", "commit", "-m", "feat: changed storage", "."); err != nil {
+	if err := command.Run("git", "commit", "-m", "feat: changed storage", "."); err != nil {
 		t.Fatal(err)
 	}
 	return remoteDir
@@ -210,7 +210,7 @@ func setupForPublish(t *testing.T, wantTag string) string {
 func cloneRepository(t *testing.T, remoteDir string) {
 	cloneDir := t.TempDir()
 	t.Chdir(cloneDir)
-	if err := external.Run("git", "clone", remoteDir, "."); err != nil {
+	if err := command.Run("git", "clone", remoteDir, "."); err != nil {
 		t.Fatal(err)
 	}
 	configNewGitRepository(t)
@@ -220,7 +220,7 @@ func continueInNewGitRepository(t *testing.T, tmpDir string) {
 	t.Helper()
 	requireCommand(t, "git")
 	t.Chdir(tmpDir)
-	if err := external.Run("git", "init", "-b", "main"); err != nil {
+	if err := command.Run("git", "init", "-b", "main"); err != nil {
 		t.Fatal(err)
 	}
 	configNewGitRepository(t)
@@ -265,10 +265,10 @@ func requireGitVersion(t *testing.T) {
 }
 
 func configNewGitRepository(t *testing.T) {
-	if err := external.Run("git", "config", "user.email", "[email protected]"); err != nil {
+	if err := command.Run("git", "config", "user.email", "[email protected]"); err != nil {
 		t.Fatal(err)
 	}
-	if err := external.Run("git", "config", "user.name", "Test Account"); err != nil {
+	if err := command.Run("git", "config", "user.name", "Test Account"); err != nil {
 		t.Fatal(err)
 	}
 }
@@ -283,10 +283,10 @@ func initRepositoryContents(t *testing.T) {
 	addCrate(t, path.Join("src", "gax-internal"), "google-cloud-gax-internal")
 	addCrate(t, path.Join("src", "gax-internal", "echo-server"), "echo-server")
 	addGeneratedCrate(t, path.Join("src", "generated", "cloud", "secretmanager", "v1"), "google-cloud-secretmanager-v1")
-	if err := external.Run("git", "add", "."); err != nil {
+	if err := command.Run("git", "add", "."); err != nil {
 		t.Fatal(err)
 	}
-	if err := external.Run("git", "commit", "-m", "initial version"); err != nil {
+	if err := command.Run("git", "commit", "-m", "initial version"); err != nil {
 		t.Fatal(err)
 	}
 }

internal/sidekick/rust_release/changes_test.go

@@ -20,8 +20,8 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/googleapis/librarian/internal/command"
 	"github.com/googleapis/librarian/internal/sidekick/config"
-	"github.com/googleapis/librarian/internal/sidekick/external"
 )
 
 const (
@@ -63,10 +63,10 @@ func TestMatchesDirtyCloneError(t *testing.T) {
 	remoteDir := setupForPublish(t, "v1.0.0")
 	cloneRepository(t, remoteDir)
 	addCrate(t, path.Join("src", "pubsub"), "google-cloud-pubsub")
-	if err := external.Run("git", "add", path.Join("src", "pubsub")); err != nil {
+	if err := command.Run("git", "add", path.Join("src", "pubsub")); err != nil {
 		t.Fatal(err)
 	}
-	if err := external.Run("git", "commit", "-m", "feat: created pubsub", "."); err != nil {
+	if err := command.Run("git", "commit", "-m", "feat: created pubsub", "."); err != nil {
 		t.Fatal(err)
 	}
 
@@ -91,10 +91,10 @@ func TestIsNewFile(t *testing.T) {
 	if err := os.WriteFile(newName, []byte(newLibRsContents), 0644); err != nil {
 		t.Fatal(err)
 	}
-	if err := external.Run("git", "add", "."); err != nil {
+	if err := command.Run("git", "add", "."); err != nil {
 		t.Fatal(err)
 	}
-	if err := external.Run("git", "commit", "-m", "feat: changed storage", "."); err != nil {
+	if err := command.Run("git", "commit", "-m", "feat: changed storage", "."); err != nil {
 		t.Fatal(err)
 	}
 	if isNewFile(&release, wantTag, existingName) {
@@ -136,7 +136,7 @@ func TestFilesChangedSuccess(t *testing.T) {
 	if err := os.WriteFile(name, []byte(newLibRsContents), 0644); err != nil {
 		t.Fatal(err)
 	}
-	if err := external.Run("git", "commit", "-m", "feat: changed storage", "."); err != nil {
+	if err := command.Run("git", "commit", "-m", "feat: changed storage", "."); err != nil {
 		t.Fatal(err)
 	}
 

internal/sidekick/rust_release/preflight.go

@@ -18,19 +18,19 @@ import (
 	"fmt"
 	"log/slog"
 
+	"github.com/googleapis/librarian/internal/command"
 	"github.com/googleapis/librarian/internal/sidekick/config"
-	"github.com/googleapis/librarian/internal/sidekick/external"
 )
 
 // PreFlight() verifies all the necessary tools are installed.
 func PreFlight(config *config.Release) error {
-	if err := external.Run(gitExe(config), "--version"); err != nil {
+	if err := command.Run(gitExe(config), "--version"); err != nil {
 		return err
 	}
-	if err := external.Run(cargoExe(config), "--version"); err != nil {
+	if err := command.Run(cargoExe(config), "--version"); err != nil {
 		return err
 	}
-	if err := external.Run(gitExe(config), "remote", "get-url", config.Remote); err != nil {
+	if err := command.Run(gitExe(config), "remote", "get-url", config.Remote); err != nil {
 		return err
 	}
 	tools, ok := config.Tools["cargo"]
@@ -40,7 +40,7 @@ func PreFlight(config *config.Release) error {
 	for _, tool := range tools {
 		slog.Info("installing cargo tool", "name", tool.Name, "version", tool.Version)
 		spec := fmt.Sprintf("%s@%s", tool.Name, tool.Version)
-		if err := external.Run(cargoExe(config), "install", "--locked", spec); err != nil {
+		if err := command.Run(cargoExe(config), "install", "--locked", spec); err != nil {
 			return err
 		}
 	}

internal/sidekick/rust_release/publish.go

@@ -19,13 +19,13 @@ import (
 	"log/slog"
 	"maps"
 	"os"
-	"os/exec"
+	stdexec "os/exec"
 	"slices"
 	"strings"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/googleapis/librarian/internal/command"
 	"github.com/googleapis/librarian/internal/sidekick/config"
-	"github.com/googleapis/librarian/internal/sidekick/external"
 )
 
 // Publish finds all the crates that should be published, (optionally) runs
@@ -56,7 +56,7 @@ func Publish(config *config.Release, dryRun bool, skipSemverChecks bool) error {
 		}
 	}
 	slog.Info("computing publication plan with: cargo workspaces plan")
-	cmd := exec.Command(cargoExe(config), "workspaces", "plan", "--skip-published")
+	cmd := stdexec.Command(cargoExe(config), "workspaces", "plan", "--skip-published")
 	if config.RootsPem != "" {
 		cmd.Env = append(os.Environ(), fmt.Sprintf("CARGO_HTTP_CAINFO=%s", config.RootsPem))
 	}
@@ -85,7 +85,7 @@ func Publish(config *config.Release, dryRun bool, skipSemverChecks bool) error {
 				continue
 			}
 			slog.Info("runnning cargo semver-checks to detect breaking changes", "crate", name)
-			if err := external.Run(cargoExe(config), "semver-checks", "--all-features", "-p", name); err != nil {
+			if err := command.Run(cargoExe(config), "semver-checks", "--all-features", "-p", name); err != nil {
 				return err
 			}
 		}
@@ -95,7 +95,7 @@ func Publish(config *config.Release, dryRun bool, skipSemverChecks bool) error {
 	if dryRun {
 		args = append(args, "--dry-run")
 	}
-	cmd = exec.Command(cargoExe(config), args...)
+	cmd = stdexec.Command(cargoExe(config), args...)
 	if config.RootsPem != "" {
 		cmd.Env = append(os.Environ(), fmt.Sprintf("CARGO_HTTP_CAINFO=%s", config.RootsPem))
 	}