chore: address feedback

Author: meltsufin

internal/librariangen/bazel/parser.go

@@ -65,6 +65,7 @@ func (c *Config) Validate() error {
 }
 
 var javaGapicLibraryRE = regexp.MustCompile(`java_gapic_library\((?s:.)*?\)`)
+
 // Parse reads a BUILD.bazel file from the given directory and extracts the
 // relevant configuration from the java_gapic_library rule.
 func Parse(dir string) (*Config, error) {

internal/librariangen/bazel/parser_test.go

@@ -256,4 +256,4 @@ func TestParse_noBuildFile(t *testing.T) {
 	if err == nil {
 		t.Error("Parse() succeeded; want error")
 	}
-}
+}

internal/librariangen/execv/execv.go

@@ -48,4 +48,4 @@ func Run(ctx context.Context, args []string, workingDir string) error {
 		return fmt.Errorf("librariangen: command failed: %w", err)
 	}
 	return nil
-}
+}

internal/librariangen/execv/execv_test.go

@@ -76,4 +76,4 @@ func TestRun(t *testing.T) {
 			}
 		})
 	}
-}
+}

internal/librariangen/generate/generator.go

@@ -83,7 +83,11 @@ func Generate(ctx context.Context, cfg *Config) error {
 		GRPCDir:  filepath.Join(cfg.OutputDir, "grpc"),
 		ProtoDir: filepath.Join(cfg.OutputDir, "proto"),
 	}
-	defer cleanupIntermediateFiles(outputConfig)
+	defer func() {
+		if err := cleanupIntermediateFiles(outputConfig); err != nil {
+			slog.Error("librariangen: failed to clean up intermediate files", "error", err)
+		}
+	}()
 
 	generateReq, err := readGenerateReq(cfg.LibrarianDir)
 	if err != nil {
@@ -133,7 +137,7 @@ func invokeProtoc(ctx context.Context, cfg *Config, generateReq *message.Library
 		}
 
 		if err := execvRun(ctx, args, cfg.OutputDir); err != nil {
-			return fmt.Errorf("librariangen: protoc failed for api %q in library %q: %w", api.Path, generateReq.ID, err)
+			return fmt.Errorf("librariangen: protoc failed for api %q in library %q: %w, execvRun error: %v", api.Path, generateReq.ID, err, err)
 		}
 	}
 	return nil
@@ -165,7 +169,7 @@ func moveFiles(sourceDir, targetDir string) error {
 		newPath := filepath.Join(targetDir, f.Name())
 		slog.Debug("librariangen: moving file", "from", oldPath, "to", newPath)
 		if err := os.Rename(oldPath, newPath); err != nil {
-			return fmt.Errorf("librariangen: failed to move %s to %s: %w", oldPath, newPath, err)
+			return fmt.Errorf("librariangen: failed to move %s to %s: %w, os.Rename error: %v", oldPath, newPath, err, err)
 		}
 	}
 	return nil
@@ -211,7 +215,7 @@ func restructureOutput(outputDir, libraryID string) error {
 		gapicSrcDir:  gapicDestDir,
 		gapicTestDir: gapicTestDestDir,
 		protoSrcDir:  protoDestDir,
-		grpcSrcDir:  grpcDestDir,
+		grpcSrcDir:   grpcDestDir,
 		samplesDir:   samplesDestDir,
 	}
 	for src, dest := range moves {
@@ -250,20 +254,21 @@ func copyAndMerge(src, dest string) error {
 			}
 		} else {
 			if err := os.Rename(srcPath, destPath); err != nil {
-				return fmt.Errorf("librariangen: failed to move %s to %s: %w", srcPath, destPath, err)
+				return fmt.Errorf("librariangen: failed to move %s to %s: %w, os.Rename error: %v", srcPath, destPath, err, err)
 			}
 		}
 	}
 	return nil
 }
 
-func cleanupIntermediateFiles(outputConfig *protoc.OutputConfig) {
+func cleanupIntermediateFiles(outputConfig *protoc.OutputConfig) error {
 	slog.Debug("librariangen: cleaning up intermediate files")
 	for _, path := range []string{outputConfig.GAPICDir, outputConfig.GRPCDir, outputConfig.ProtoDir} {
 		if err := os.RemoveAll(path); err != nil {
-			slog.Error("librariangen: failed to clean up intermediate file", "path", path, "error", err)
+			return fmt.Errorf("failed to clean up intermediate file at %s: %w", path, err)
 		}
 	}
+	return nil
 }
 
 func unzip(src, dest string) error {

internal/librariangen/generate/generator_test.go

@@ -16,13 +16,10 @@ package generate
 
 import (
 	"archive/zip"
-	"bytes"
 	"context"
 	"errors"
-	"log/slog"
 	"os"
 	"path/filepath"
-	"strings"
 	"testing"
 
 	"cloud.google.com/java/internal/librariangen/protoc"
@@ -123,6 +120,25 @@ func createFakeZip(t *testing.T, path string) {
 	}
 }
 
+func setupFakeProtocOutput(t *testing.T, e *testEnv) {
+	// Simulate protoc creating the zip file.
+	zipPath := filepath.Join(e.outputDir, "gapic", "temp-codegen.srcjar")
+	if err := os.MkdirAll(filepath.Dir(zipPath), 0755); err != nil {
+		t.Fatalf("failed to create directory: %v", err)
+	}
+	createFakeZip(t, zipPath)
+	// Create the directory that is expected by restructureOutput.
+	if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "src", "main", "java"), 0755); err != nil {
+		t.Fatalf("failed to create directory: %v", err)
+	}
+	if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "src", "test", "java"), 0755); err != nil {
+		t.Fatalf("failed to create directory: %v", err)
+	}
+	if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "samples", "snippets"), 0755); err != nil {
+		t.Fatalf("failed to create directory: %v", err)
+	}
+}
+
 func TestGenerate(t *testing.T) {
 	singleAPIRequest := `{"id": "foo", "apis": [{"path": "api/v1"}]}`
 	validBazel := `
@@ -231,22 +247,7 @@ java_gapic_library(
 					t.Errorf("protocRun called with %s; want %s", args[0], want)
 				}
 				if tt.protocErr == nil && tt.name != "unzip fails" {
-					// Simulate protoc creating the zip file.
-					zipPath := filepath.Join(e.outputDir, "gapic", "temp-codegen.srcjar")
-					if err := os.MkdirAll(filepath.Dir(zipPath), 0755); err != nil {
-						t.Fatalf("failed to create directory: %v", err)
-					}
-					createFakeZip(t, zipPath)
-					// Create the directory that is expected by restructureOutput.
-					if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "src", "main", "java"), 0755); err != nil {
-						t.Fatalf("failed to create directory: %v", err)
-					}
-					if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "src", "test", "java"), 0755); err != nil {
-						t.Fatalf("failed to create directory: %v", err)
-					}
-					if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "samples", "snippets"), 0755); err != nil {
-						t.Fatalf("failed to create directory: %v", err)
-					}
+					setupFakeProtocOutput(t, e)
 				}
 				protocRunCount++
 				return tt.protocErr
@@ -382,12 +383,12 @@ func TestCopyAndMerge(t *testing.T) {
 	srcDir := filepath.Join(e.tmpDir, "src")
 	destDir := filepath.Join(e.tmpDir, "dest")
 	sourceFiles := map[string]string{
-		"com/google/foo.java":         "",
-		"com/google/bar/baz.java":     "",
+		"com/google/foo.java":          "",
+		"com/google/bar/baz.java":      "",
 		"com/google/bar/qux/quux.java": "",
 	}
 	destFiles := map[string]string{
-		"com/google/existing.java": "",
+		"com/google/existing.java":    "",
 		"com/google/bar/another.java": "",
 	}
 	for path, content := range sourceFiles {
@@ -562,9 +563,6 @@ func TestMoveFiles(t *testing.T) {
 }
 
 func TestCleanupIntermediateFiles(t *testing.T) {
-	var buf bytes.Buffer
-	slog.SetDefault(slog.New(slog.NewTextHandler(&buf, nil)))
-
 	e := newTestEnv(t)
 	defer e.cleanup(t)
 
@@ -587,9 +585,7 @@ func TestCleanupIntermediateFiles(t *testing.T) {
 		GRPCDir:  filepath.Join(e.outputDir, "grpc"),
 		ProtoDir: protectedDir,
 	}
-	cleanupIntermediateFiles(outputConfig)
-
-	if !strings.Contains(buf.String(), "failed to clean up intermediate file") {
-		t.Errorf("cleanupIntermediateFiles() should log an error on failure, but did not. Log: %s", buf.String())
+	if err := cleanupIntermediateFiles(outputConfig); err == nil {
+		t.Error("cleanupIntermediateFiles() should return an error on failure, but did not")
 	}
 }

internal/librariangen/main_test.go

@@ -63,4 +63,4 @@ func TestParseLogLevel(t *testing.T) {
 			}
 		})
 	}
-}
+}

internal/librariangen/protoc/protoc.go

@@ -106,4 +106,4 @@ func Build(apiServiceDir string, config ConfigProvider, sourceDir string, output
 	args = append(args, protoFiles...)
 
 	return args, nil
-}
+}

internal/librariangen/protoc/protoc_test.go

@@ -47,10 +47,10 @@ func TestBuild(t *testing.T) {
 		t.Fatalf("failed to get absolute path for sourceDir: %v", err)
 	}
 	tests := []struct {
-		name          string
-		apiPath       string
-		config        mockConfigProvider
-		want          []string
+		name    string
+		apiPath string
+		config  mockConfigProvider
+		want    []string
 	}{
 		{
 			name:    "java_grpc_library rule",
@@ -112,7 +112,7 @@ func TestBuild(t *testing.T) {
 			name:    "proto-only",
 			apiPath: "google/cloud/secretmanager/v1beta2",
 			config: mockConfigProvider{
-				hasGAPIC:  false,
+				hasGAPIC: false,
 			},
 			want: []string{
 				"protoc",
@@ -141,4 +141,4 @@ func TestBuild(t *testing.T) {
 			}
 		})
 	}
-}
+}