diff --git a/internal/gapicgen/execv/gocmd/gocmd.go b/internal/gapicgen/execv/gocmd/gocmd.go new file mode 100644 index 00000000000..a839b401990 --- /dev/null +++ b/internal/gapicgen/execv/gocmd/gocmd.go @@ -0,0 +1,92 @@ +// Copyright 2021 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package gocmd provides helers for invoking Go tooling. +package gocmd + +import ( + "errors" + "fmt" + "log" + "os" + "os/exec" + "strings" + + "cloud.google.com/go/internal/gapicgen/execv" +) + +var ( + // ErrBuildConstraint is returned when the Go command returns this error. + ErrBuildConstraint error = errors.New("build constraints exclude all Go files") +) + +// ModTidy tidies go.mod file in the specified directory +func ModTidy(dir string) error { + log.Printf("[%s] running go mod tidy", dir) + c := execv.Command("go", "mod", "tidy") + c.Dir = dir + c.Env = []string{ + fmt.Sprintf("PATH=%s", os.Getenv("PATH")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. + fmt.Sprintf("HOME=%s", os.Getenv("HOME")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. + } + return c.Run() +} + +// ListModName finds a modules name for a given directory. +func ListModName(dir string) (string, error) { + modC := execv.Command("go", "list", "-m") + modC.Dir = dir + mod, err := modC.Output() + return string(mod), err +} + +// ListModDirName finds the directory in which the module resides. Returns +// ErrBuildConstraint if all files in a module are constrained. +func ListModDirName(dir string) (string, error) { + var out []byte + var err error + c := execv.Command("go", "list", "-f", "'{{.Module.Dir}}'") + c.Dir = dir + if out, err = c.Output(); err != nil { + if ee, ok := err.(*exec.ExitError); ok { + if strings.Contains(string(ee.Stderr), "build constraints exclude all Go files") { + return "", ErrBuildConstraint + } + } + return "", err + } + return strings.Trim(strings.TrimSpace(string(out)), "'"), nil +} + +// Build attempts to build all packages recursively from the given directory. +func Build(dir string) error { + log.Println("building generated code") + c := execv.Command("go", "build", "./...") + c.Dir = dir + return c.Run() +} + +// Vet runs linters on all .go files recursively from the given directory. +func Vet(dir string) error { + log.Println("vetting generated code") + c := execv.Command("goimports", "-w", ".") + c.Dir = dir + if err := c.Run(); err != nil { + return err + } + + c = execv.Command("gofmt", "-s", "-d", "-w", "-l", ".") + c.Dir = dir + return c.Run() +} diff --git a/internal/gapicgen/generator/gapics.go b/internal/gapicgen/generator/gapics.go index 71c308a7bd9..9c6f6c58564 100644 --- a/internal/gapicgen/generator/gapics.go +++ b/internal/gapicgen/generator/gapics.go @@ -24,7 +24,9 @@ import ( "strings" "cloud.google.com/go/internal/gapicgen/execv" + "cloud.google.com/go/internal/gapicgen/execv/gocmd" "cloud.google.com/go/internal/gapicgen/gensnippets" + "cloud.google.com/go/internal/gapicgen/git" "gopkg.in/yaml.v2" ) @@ -37,10 +39,11 @@ type GapicGenerator struct { gapicToGenerate string regenOnly bool onlyGenerateGapic bool + modifiedPkgs []string } // NewGapicGenerator creates a GapicGenerator. -func NewGapicGenerator(c *Config) *GapicGenerator { +func NewGapicGenerator(c *Config, modifiedPkgs []string) *GapicGenerator { return &GapicGenerator{ googleapisDir: c.GoogleapisDir, protoDir: c.ProtoDir, @@ -49,6 +52,7 @@ func NewGapicGenerator(c *Config) *GapicGenerator { gapicToGenerate: c.GapicToGenerate, regenOnly: c.RegenOnly, onlyGenerateGapic: c.OnlyGenerateGapic, + modifiedPkgs: modifiedPkgs, } } @@ -72,6 +76,16 @@ func (g *GapicGenerator) Regen(ctx context.Context) error { return err } + // TODO(codyoss): Remove once https://github.com/googleapis/gapic-generator-go/pull/606 + // is released. + if err := gocmd.Vet(g.googleCloudDir); err != nil { + return err + } + + if err := g.resetUnknownVersion(); err != nil { + return err + } + if g.regenOnly { return nil } @@ -94,11 +108,11 @@ func (g *GapicGenerator) Regen(ctx context.Context) error { return err } - if err := vet(g.googleCloudDir); err != nil { + if err := gocmd.Vet(g.googleCloudDir); err != nil { return err } - if err := build(g.googleCloudDir); err != nil { + if err := gocmd.Build(g.googleCloudDir); err != nil { return err } @@ -124,33 +138,19 @@ func (g *GapicGenerator) regenSnippets(ctx context.Context) error { if err := replaceAllForSnippets(g.googleCloudDir, snippetDir); err != nil { return err } - if err := goModTidy(snippetDir); err != nil { + if err := gocmd.ModTidy(snippetDir); err != nil { return err } return nil } -func goModTidy(dir string) error { - log.Printf("[%s] running go mod tidy", dir) - c := execv.Command("go", "mod", "tidy") - c.Dir = dir - c.Env = []string{ - fmt.Sprintf("PATH=%s", os.Getenv("PATH")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. - fmt.Sprintf("HOME=%s", os.Getenv("HOME")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. - } - return c.Run() -} - func replaceAllForSnippets(googleCloudDir, snippetDir string) error { return execv.ForEachMod(googleCloudDir, func(dir string) error { if dir == snippetDir { return nil } - // Get the module name in this dir. - modC := execv.Command("go", "list", "-m") - modC.Dir = dir - mod, err := modC.Output() + mod, err := gocmd.ListModName(dir) if err != nil { return err } @@ -208,23 +208,60 @@ go mod edit -dropreplace "google.golang.org/genproto" return c.Run() } +// resetUnknownVersion resets doc.go files that have only had their version +// changed to UNKNOWN by the generator. +func (g *GapicGenerator) resetUnknownVersion() error { + files, err := git.FindModifiedFiles(g.googleCloudDir) + if err != nil { + return err + } + + for _, file := range files { + if !strings.HasSuffix(file, "doc.go") { + continue + } + diff, err := git.FileDiff(g.googleCloudDir, file) + if err != nil { + return err + } + // More than one diff, don't reset. + if strings.Count(diff, "@@") != 2 { + log.Println(diff) + continue + } + // Not related to version, don't reset. + if !strings.Contains(diff, "+const versionClient = \"UNKNOWN\"") { + continue + } + + if err := git.ResetFile(g.googleCloudDir, file); err != nil { + return err + } + } + return nil +} + // setVersion updates the versionClient constant in all .go files. It may create // .backup files on certain systems (darwin), and so should be followed by a // clean-up of .backup files. func (g *GapicGenerator) setVersion() error { + dirs, err := g.findModifiedDirs() + if err != nil { + return err + } log.Println("updating client version") - // TODO(deklerk): Migrate this all to Go instead of using bash. - - c := execv.Command("bash", "-c", ` + for _, dir := range dirs { + c := execv.Command("bash", "-c", ` ver=$(date +%Y%m%d) -git ls-files -mo | while read modified; do - dir=${modified%/*.*} - find . -path "*/$dir/doc.go" -exec sed -i.backup -e "s/^const versionClient.*/const versionClient = \"$ver\"/" '{}' +; -done +find . -path "*/doc.go" -exec sed -i.backup -e "s/^const versionClient.*/const versionClient = \"$ver\"/" '{}' +; find . -name '*.backup' -delete `) - c.Dir = g.googleCloudDir - return c.Run() + c.Dir = dir + if err := c.Run(); err != nil { + return err + } + } + return nil } // microgen runs the microgenerator on a single microgen config. @@ -501,3 +538,30 @@ func (g *GapicGenerator) parseAPIShortnames(confs []*microgenConfig, manualEntri } return shortnames, nil } + +func (g *GapicGenerator) findModifiedDirs() ([]string, error) { + log.Println("finding modifiled directories") + files, err := git.FindModifiedAndUntrackedFiles(g.googleCloudDir) + if err != nil { + return nil, err + } + dirs := map[string]bool{} + for _, file := range files { + dir := filepath.Dir(filepath.Join(g.googleCloudDir, file)) + dirs[dir] = true + } + + // Add modified dirs from genproto. Sometimes only a request struct will be + // updated, in these cases we should still make modifications the + // corresponding gapic directories. + for _, pkg := range g.modifiedPkgs { + dir := filepath.Join(g.googleCloudDir, pkg) + dirs[dir] = true + } + + var dirList []string + for dir := range dirs { + dirList = append(dirList, dir) + } + return dirList, nil +} diff --git a/internal/gapicgen/generator/generator.go b/internal/gapicgen/generator/generator.go index 4615cf370b5..1c17e0e43d9 100644 --- a/internal/gapicgen/generator/generator.go +++ b/internal/gapicgen/generator/generator.go @@ -21,11 +21,9 @@ import ( "context" "fmt" "io/ioutil" - "log" "os" "path/filepath" - "cloud.google.com/go/internal/gapicgen/execv" "cloud.google.com/go/internal/gapicgen/git" ) @@ -49,10 +47,6 @@ func Generate(ctx context.Context, conf *Config) ([]*git.ChangeInfo, error) { return nil, fmt.Errorf("error generating genproto (may need to check logs for more errors): %v", err) } } - gapicGenerator := NewGapicGenerator(conf) - if err := gapicGenerator.Regen(ctx); err != nil { - return nil, fmt.Errorf("error generating gapics (may need to check logs for more errors): %v", err) - } var changes []*git.ChangeInfo if !conf.LocalMode { @@ -65,6 +59,17 @@ func Generate(ctx context.Context, conf *Config) ([]*git.ChangeInfo, error) { return nil, err } } + var modifiedPkgs []string + for _, v := range changes { + if v.Package != "" { + modifiedPkgs = append(modifiedPkgs, v.PackageDir) + } + } + + gapicGenerator := NewGapicGenerator(conf, modifiedPkgs) + if err := gapicGenerator.Regen(ctx); err != nil { + return nil, fmt.Errorf("error generating gapics (may need to check logs for more errors): %v", err) + } return changes, nil } @@ -81,7 +86,7 @@ func gatherChanges(googleapisDir, genprotoDir string) ([]*git.ChangeInfo, error) } gapicPkgs := make(map[string]string) for _, v := range microgenGapicConfigs { - gapicPkgs[v.inputDirectoryPath] = git.ParseConventionalCommitPkg(v.importPath) + gapicPkgs[v.inputDirectoryPath] = v.importPath } changes, err := git.ParseChangeInfo(googleapisDir, commits, gapicPkgs) if err != nil { @@ -112,25 +117,3 @@ func recordGoogleapisHash(googleapisDir, genprotoDir string) error { } return nil } - -// build attempts to build all packages recursively from the given directory. -func build(dir string) error { - log.Println("building generated code") - c := execv.Command("go", "build", "./...") - c.Dir = dir - return c.Run() -} - -// vet runs linters on all .go files recursively from the given directory. -func vet(dir string) error { - log.Println("vetting generated code") - c := execv.Command("goimports", "-w", ".") - c.Dir = dir - if err := c.Run(); err != nil { - return err - } - - c = execv.Command("gofmt", "-s", "-d", "-w", "-l", ".") - c.Dir = dir - return c.Run() -} diff --git a/internal/gapicgen/generator/genproto.go b/internal/gapicgen/generator/genproto.go index a66ae34ad93..eaf90d92789 100644 --- a/internal/gapicgen/generator/genproto.go +++ b/internal/gapicgen/generator/genproto.go @@ -26,6 +26,7 @@ import ( "strings" "cloud.google.com/go/internal/gapicgen/execv" + "cloud.google.com/go/internal/gapicgen/execv/gocmd" "cloud.google.com/go/internal/gapicgen/git" "golang.org/x/sync/errgroup" ) @@ -131,11 +132,11 @@ func (g *GenprotoGenerator) Regen(ctx context.Context) error { return err } - if err := vet(g.genprotoDir); err != nil { + if err := gocmd.Vet(g.genprotoDir); err != nil { return err } - if err := build(g.genprotoDir); err != nil { + if err := gocmd.Build(g.genprotoDir); err != nil { return err } diff --git a/internal/gapicgen/git/git.go b/internal/gapicgen/git/git.go index f4118e84969..6014448ea4a 100644 --- a/internal/gapicgen/git/git.go +++ b/internal/gapicgen/git/git.go @@ -20,6 +20,8 @@ import ( "io" "log" "os" + "os/exec" + "path/filepath" "strings" "cloud.google.com/go/internal/gapicgen/execv" @@ -31,6 +33,7 @@ type ChangeInfo struct { Body string Title string Package string + PackageDir string GoogleapisHash string } @@ -107,17 +110,15 @@ func ParseChangeInfo(googleapisDir string, hashes []string, gapicPkgs map[string if err != nil { return nil, err } - var pkg string + var pkg, pkgDir string for _, file := range files { - ss := strings.Split(file, "/") - if len(ss) == 0 { + if file == "" { continue } - // remove filename from path - strings.Join(ss[:len(ss)-1], "/") - tempPkg := gapicPkgs[strings.Join(ss[:len(ss)-1], "/")] - if tempPkg != "" { - pkg = tempPkg + importPath := gapicPkgs[filepath.Dir(file)] + if importPath != "" { + pkg = parseConventionalCommitPkg(importPath) + pkgDir = strings.TrimPrefix(importPath, "cloud.google.com/go/") break } } @@ -126,15 +127,16 @@ func ParseChangeInfo(googleapisDir string, hashes []string, gapicPkgs map[string Title: title, Body: body, Package: pkg, + PackageDir: pkgDir, GoogleapisHash: hash, }) } return changes, nil } -// ParseConventionalCommitPkg parses the package context for conventional commit +// parseConventionalCommitPkg parses the package context for conventional commit // messages. -func ParseConventionalCommitPkg(importPath string) string { +func parseConventionalCommitPkg(importPath string) string { s := strings.TrimPrefix(importPath, "cloud.google.com/go/") ss := strings.Split(s, "/") // remove the version, i.e /apiv1 @@ -205,6 +207,45 @@ func DeepClone(repo, dir string) error { return err } +// FindModifiedFiles locates modified files in the git directory provided. +func FindModifiedFiles(dir string) ([]string, error) { + return findModifiedFiles(dir, "-m") +} + +// FindModifiedAndUntrackedFiles locates modified and untracked files in the git +// directory provided. +func FindModifiedAndUntrackedFiles(dir string) ([]string, error) { + return findModifiedFiles(dir, "-mo") +} + +func findModifiedFiles(dir string, filter string) ([]string, error) { + c := execv.Command("git", "ls-files", filter) + c.Dir = dir + out, err := c.Output() + if err != nil { + return nil, err + } + return strings.Split(string(bytes.TrimSpace(out)), "\n"), nil +} + +// ResetFile reverts all changes made to a file in the provided directory. +func ResetFile(dir, filename string) error { + c := exec.Command("git", "checkout", "HEAD", "--", filename) + c.Dir = dir + return c.Run() +} + +// FileDiff returns the git diff for the specified file. +func FileDiff(dir, filename string) (string, error) { + c := exec.Command("git", "diff", "--unified=0", filename) + c.Dir = dir + out, err := c.Output() + if err != nil { + return "", err + } + return strings.TrimSpace(string(out)), nil +} + // filesChanged returns a list of files changed in a commit for the provdied // hash in the given gitDir. func filesChanged(gitDir, hash string) ([]string, error) { diff --git a/internal/gapicgen/git/git_test.go b/internal/gapicgen/git/git_test.go index d100ff4e4fc..fdf02977b33 100644 --- a/internal/gapicgen/git/git_test.go +++ b/internal/gapicgen/git/git_test.go @@ -28,7 +28,7 @@ func TestParseConventionalCommitPkg(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - if got := ParseConventionalCommitPkg(tc.importPath); got != tc.want { + if got := parseConventionalCommitPkg(tc.importPath); got != tc.want { t.Errorf("parseConventionalCommitPkg(%q) = %q, want %q", tc.importPath, got, tc.want) } }) diff --git a/internal/gapicgen/git/github.go b/internal/gapicgen/git/github.go index 37bf4317ddb..fe89097a4a2 100644 --- a/internal/gapicgen/git/github.go +++ b/internal/gapicgen/git/github.go @@ -15,17 +15,21 @@ package git import ( + "bytes" "context" + "errors" "fmt" "io/ioutil" "log" "os" "os/user" "path" + "path/filepath" "strings" "time" "cloud.google.com/go/internal/gapicgen/execv" + "cloud.google.com/go/internal/gapicgen/execv/gocmd" "github.com/google/go-github/v34/github" "github.com/shurcooL/githubv4" "golang.org/x/oauth2" @@ -65,12 +69,6 @@ genbot to assign reviewers to the google-cloud-go PR. ` ) -// githubReviewers is the list of github usernames that will be assigned to -// review the PRs. -// -// TODO(ndietz): Can we use github teams? -var githubReviewers = []string{"hongalex", "broady", "tritone", "codyoss", "tbpg"} - // PullRequest represents a GitHub pull request. type PullRequest struct { Author string @@ -105,7 +103,7 @@ func NewGithubClient(ctx context.Context, username, name, email, accessToken str return &GithubClient{cV3: github.NewClient(tc), cV4: githubv4.NewClient(tc), Username: username}, nil } -// SetGitCreds sets credentials for gerrit. +// setGitCreds configures credentials for GitHub. func setGitCreds(githubName, githubEmail, githubUsername, accessToken string) error { u, err := user.Current() if err != nil { @@ -220,20 +218,6 @@ git push origin $BRANCH_NAME return 0, err } - // Can't assign the submitter of the PR as a reviewer. - var reviewers []string - for _, r := range githubReviewers { - if r != gc.Username { - reviewers = append(reviewers, r) - } - } - - if _, _, err := gc.cV3.PullRequests.RequestReviewers(ctx, "googleapis", "go-genproto", pr.GetNumber(), github.ReviewersRequest{ - Reviewers: reviewers, - }); err != nil { - return 0, err - } - log.Printf("creating genproto PR... done %s\n", pr.GetHTMLURL()) return pr.GetNumber(), nil @@ -353,12 +337,26 @@ func (gc *GithubClient) MarkPRReadyForReview(ctx context.Context, repo string, n // UpdateGocloudGoMod updates the go.mod to include latest version of genproto // for the given gocloud ref. func (gc *GithubClient) UpdateGocloudGoMod(pr *PullRequest) error { - tmpDir, err := ioutil.TempDir("", "finalize-gerrit-cl") + tmpDir, err := ioutil.TempDir("", "finalize-github-pr") if err != nil { return err } defer os.RemoveAll(tmpDir) + if err := checkoutCode(tmpDir); err != nil { + return err + } + if err := updateDeps(tmpDir); err != nil { + return err + } + if err := addAndPushCode(tmpDir); err != nil { + return err + } + + return nil +} + +func checkoutCode(tmpDir string) error { c := execv.Command("/bin/bash", "-c", ` set -ex @@ -366,19 +364,67 @@ git init git remote add origin https://github.com/googleapis/google-cloud-go git fetch --all git checkout $BRANCH_NAME +`) + c.Env = []string{ + fmt.Sprintf("BRANCH_NAME=%s", gocloudBranchName), + } + c.Dir = tmpDir + return c.Run() +} + +func updateDeps(tmpDir string) error { + // Find directories that had code changes. + out := bytes.NewBuffer(nil) + c := execv.Command("git", "diff", "--name-only", "HEAD", "HEAD~1") + c.Stdout = out + c.Dir = tmpDir + if err := c.Run(); err != nil { + return err + } + files := strings.Split(out.String(), "\n") + dirs := map[string]bool{} + for _, file := range files { + dir := filepath.Dir(file) + dirs[filepath.Join(tmpDir, dir)] = true + } + + // Find which modules had code changes. + updatedModDirs := map[string]bool{} + errWriter := bytes.NewBuffer(nil) + for dir := range dirs { + out.Reset() + errWriter.Reset() + modDir, err := gocmd.ListModDirName(dir) + if err != nil { + if errors.Is(err, gocmd.ErrBuildConstraint) { + continue + } + return err + } + updatedModDirs[modDir] = true + } -# tidyall + // Update required modules. + for modDir := range updatedModDirs { + log.Printf("Updating module dir %q", modDir) + c := execv.Command("/bin/bash", "-c", ` +set -ex + +go get -d google.golang.org/api | true # We don't care that there's no files at root. +go get -d google.golang.org/genproto | true # We don't care that there's no files at root. go mod tidy -for i in $(find . -name go.mod); do - pushd $(dirname $i); - # Update genproto and api to latest for every module (latest version is - # always correct version). tidy will remove the dependencies if they're not - # actually used by the client. - go get -d google.golang.org/api | true # We don't care that there's no files at root. - go get -d google.golang.org/genproto | true # We don't care that there's no files at root. - go mod tidy; - popd; -done +`) + c.Dir = modDir + if err := c.Run(); err != nil { + return err + } + } + return nil +} + +func addAndPushCode(tmpDir string) error { + c := execv.Command("/bin/bash", "-c", ` +set -ex git add -A filesUpdated=$( git status --short | wc -l ) @@ -390,7 +436,7 @@ then fi `) c.Env = []string{ - fmt.Sprintf("BRANCH_NAME=%s", "regen_gocloud"), + fmt.Sprintf("BRANCH_NAME=%s", gocloudBranchName), fmt.Sprintf("PATH=%s", os.Getenv("PATH")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. fmt.Sprintf("HOME=%s", os.Getenv("HOME")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. }