From 10c5a3cb98205bb1e448507cd1f00a0f199e8671 Mon Sep 17 00:00:00 2001 From: jan Date: Wed, 7 Feb 2024 10:38:25 +0000 Subject: [PATCH 1/9] Resolve dependencies from both host go.mod and module's go.mod Previously we would only look at the host repo's go.mod or the module's go.mod, but not both. This changes makes it so that we always look at both available go.mod files and add logic to prevent reading the `replace` directives in the module's go.mod if we also have the host repo's go.mod configured. --- build_defs/go.build_defs | 2 +- tools/please_go/generate/generate.go | 44 ++++++++++++++++++++-------- tools/please_go/please_go.go | 4 +-- 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs index a47131b..f85373f 100644 --- a/build_defs/go.build_defs +++ b/build_defs/go.build_defs @@ -1219,7 +1219,7 @@ def go_repo(module: str, version:str='', download:str=None, name:str=None, insta "download": [download], } if CONFIG.GO.MOD_FILE: - modFileArg = "--mod_file $SRCS_MOD_FILE" + modFileArg = "--host_mod_file $SRCS_MOD_FILE" srcs["mod_file"] = [CONFIG.GO.MOD_FILE] else: modFileArg = "" diff --git a/tools/please_go/generate/generate.go b/tools/please_go/generate/generate.go index 1f95212..1312c2f 100644 --- a/tools/please_go/generate/generate.go +++ b/tools/please_go/generate/generate.go @@ -2,11 +2,13 @@ package generate import ( "bufio" + "errors" "fmt" "go/build" "io/fs" "log" "os" + "path" "path/filepath" "strings" @@ -22,17 +24,16 @@ type Generate struct { srcRoot string subrepo string buildContext build.Context - modFile string + hostModFile string buildFileNames []string moduleDeps []string - pluginTarget string replace map[string]string knownImportTargets map[string]string // cache these so we don't end up looping over all the modules for every import thirdPartyFolder string install []string } -func New(srcRoot, thirdPartyFolder, modFile, module, version, subrepo string, buildFileNames, moduleDeps, install []string, buildTags []string) *Generate { +func New(srcRoot, thirdPartyFolder, hostModFile, module, version, subrepo string, buildFileNames, moduleDeps, install []string, buildTags []string) *Generate { moduleArg := module if version != "" { moduleArg += "@" + version @@ -46,7 +47,7 @@ func New(srcRoot, thirdPartyFolder, modFile, module, version, subrepo string, bu buildContext: ctxt, buildFileNames: buildFileNames, moduleDeps: moduleDeps, - modFile: modFile, + hostModFile: hostModFile, knownImportTargets: map[string]string{}, thirdPartyFolder: thirdPartyFolder, install: install, @@ -59,9 +60,24 @@ func New(srcRoot, thirdPartyFolder, modFile, module, version, subrepo string, bu // Generate generates a new Please project at the src root. It will walk through the directory tree generating new BUILD // files. This is primarily intended to generate a please subrepo for third party code. func (g *Generate) Generate() error { - if err := g.readGoMod(g.modFile); err != nil { - return fmt.Errorf("failed to read go.mod: %w", err) + readModuleReplacePaths := false + if g.hostModFile != "" { + if err := g.readGoMod(g.hostModFile, true); err != nil { + return fmt.Errorf("failed to read host repo go.mod %q: %w", g.hostModFile, err) + } + } else { + // We only want to read the dep replacement rules if there isn't any host go.mod, otherwise third-party dependencies + // would dictate the host's naming. There could also be conflicts across different third-party modules. + readModuleReplacePaths = true + } + + moduleGoMod := path.Join(g.srcRoot, "go.mod") + if err := g.readGoMod(moduleGoMod, readModuleReplacePaths); err != nil { + if !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("failed to read module go.mod %q: %w", moduleGoMod, err) + } } + if err := g.writeConfig(); err != nil { return fmt.Errorf("failed to write config: %w", err) } @@ -196,10 +212,7 @@ func (g *Generate) writeInstallFilegroup() error { } // readGoMod reads the module dependencies -func (g *Generate) readGoMod(path string) error { - if path == "" { - path = filepath.Join(g.srcRoot, "go.mod") - } +func (g *Generate) readGoMod(path string, readReplacePaths bool) error { data, err := os.ReadFile(path) if err != nil { return err @@ -216,10 +229,15 @@ func (g *Generate) readGoMod(path string) error { g.moduleDeps = append(g.moduleDeps, g.moduleName) - g.replace = make(map[string]string, len(modFile.Replace)) - for _, replace := range modFile.Replace { - g.replace[replace.Old.Path] = replace.New.Path + if readReplacePaths { + if g.replace == nil { + g.replace = make(map[string]string, len(modFile.Replace)) + } + for _, replace := range modFile.Replace { + g.replace[replace.Old.Path] = replace.New.Path + } } + return nil } diff --git a/tools/please_go/please_go.go b/tools/please_go/please_go.go index 2b33eef..a0aa2fd 100644 --- a/tools/please_go/please_go.go +++ b/tools/please_go/please_go.go @@ -96,7 +96,7 @@ var opts = struct { SrcRoot string `short:"r" long:"src_root" description:"The src root of the module to inspect"` ImportPath string `long:"import_path" description:"overrides the module's import path. If not set, the import path from the go.mod will be used.'"` ThirdPartyFolder string `short:"t" long:"third_part_folder" description:"The folder containing the third party subrepos" default:"third_party/go"` - ModFile string `long:"mod_file" description:"Path to the mod file to use to resolve dependencies against"` + HostModFile string `long:"host_mod_file" description:"Path to the host repo mod file to use to resolve dependencies against (dependencies will be resolved against the module as well if it exists)"` Module string `long:"module" description:"The name of the current module"` Version string `long:"version" description:"The version of the current module"` Install []string `long:"install" description:"The packages to add to the :install alias"` @@ -184,7 +184,7 @@ var subCommands = map[string]func() int{ }, "generate": func() int { gen := opts.Generate - g := generate.New(gen.SrcRoot, gen.ThirdPartyFolder, gen.ModFile, gen.Module, gen.Version, gen.Subrepo, []string{"BUILD", "BUILD.plz"}, gen.Args.Requirements, gen.Install, gen.BuildTags) + g := generate.New(gen.SrcRoot, gen.ThirdPartyFolder, gen.HostModFile, gen.Module, gen.Version, gen.Subrepo, []string{"BUILD", "BUILD.plz"}, gen.Args.Requirements, gen.Install, gen.BuildTags) if err := g.Generate(); err != nil { log.Fatalf("failed to generate go rules: %v", err) } From d43fdfe2812b27961b2e8dd569efc390dfbaf03e Mon Sep 17 00:00:00 2001 From: jan Date: Wed, 7 Feb 2024 13:23:17 +0000 Subject: [PATCH 2/9] Add test --- test/go_repo_transitive_deps/BUILD | 7 ++++ .../test_repo/.plzconfig | 14 +++++++ .../test_repo/BUILD_FILE | 7 ++++ .../test_repo/go_repo_transitive_deps_test.go | 16 +++++++ .../test_repo/plugins/BUILD_FILE | 4 ++ .../test_repo/third_party/go/BUILD_FILE | 42 +++++++++++++++++++ .../test_repo/third_party/go/go.mod | 10 +++++ 7 files changed, 100 insertions(+) create mode 100644 test/go_repo_transitive_deps/BUILD create mode 100644 test/go_repo_transitive_deps/test_repo/.plzconfig create mode 100644 test/go_repo_transitive_deps/test_repo/BUILD_FILE create mode 100644 test/go_repo_transitive_deps/test_repo/go_repo_transitive_deps_test.go create mode 100644 test/go_repo_transitive_deps/test_repo/plugins/BUILD_FILE create mode 100644 test/go_repo_transitive_deps/test_repo/third_party/go/BUILD_FILE create mode 100644 test/go_repo_transitive_deps/test_repo/third_party/go/go.mod diff --git a/test/go_repo_transitive_deps/BUILD b/test/go_repo_transitive_deps/BUILD new file mode 100644 index 0000000..4d123bd --- /dev/null +++ b/test/go_repo_transitive_deps/BUILD @@ -0,0 +1,7 @@ +subinclude("///shell//build_defs:shell", "//test/build_defs:e2e") + +please_repo_e2e_test( + name = "go_repo_transitive_deps_test", + plz_command = "plz test", + repo = "test_repo", +) diff --git a/test/go_repo_transitive_deps/test_repo/.plzconfig b/test/go_repo_transitive_deps/test_repo/.plzconfig new file mode 100644 index 0000000..b1a9564 --- /dev/null +++ b/test/go_repo_transitive_deps/test_repo/.plzconfig @@ -0,0 +1,14 @@ +[Parse] +BuildFileName = BUILD_FILE +BuildFileName = BUILD + +[Plugin "go"] +Target = //plugins:go +PleaseGoTool = +GoTool = +FeatureFlags = go_get +Stdlib = //third_party/go:std +Modfile = //third_party/go:modfile + +[FeatureFlags] +ExcludeGoRules = true diff --git a/test/go_repo_transitive_deps/test_repo/BUILD_FILE b/test/go_repo_transitive_deps/test_repo/BUILD_FILE new file mode 100644 index 0000000..6c77cee --- /dev/null +++ b/test/go_repo_transitive_deps/test_repo/BUILD_FILE @@ -0,0 +1,7 @@ +subinclude("///go//build_defs:go") + +go_test( + name = "go_repo_transitive_deps_test", + srcs = ["go_repo_transitive_deps_test.go"], + deps = ["//third_party/go:testify"], +) \ No newline at end of file diff --git a/test/go_repo_transitive_deps/test_repo/go_repo_transitive_deps_test.go b/test/go_repo_transitive_deps/test_repo/go_repo_transitive_deps_test.go new file mode 100644 index 0000000..be07ac3 --- /dev/null +++ b/test/go_repo_transitive_deps/test_repo/go_repo_transitive_deps_test.go @@ -0,0 +1,16 @@ +package test_repo + +// Testify has a dependency on github.com/pmezard/go-difflib which we DON'T +// have in our go.mod file but testify does. +// The test ensures that go_please generate considers both the host and the +// module go.mod and correctly generates the dependencies for the subrepo. + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestAssertImportable(t *testing.T) { + assert.True(t, true) +} diff --git a/test/go_repo_transitive_deps/test_repo/plugins/BUILD_FILE b/test/go_repo_transitive_deps/test_repo/plugins/BUILD_FILE new file mode 100644 index 0000000..f197577 --- /dev/null +++ b/test/go_repo_transitive_deps/test_repo/plugins/BUILD_FILE @@ -0,0 +1,4 @@ +plugin_repo( + name = "go", + revision = "master", +) \ No newline at end of file diff --git a/test/go_repo_transitive_deps/test_repo/third_party/go/BUILD_FILE b/test/go_repo_transitive_deps/test_repo/third_party/go/BUILD_FILE new file mode 100644 index 0000000..c2c9fe7 --- /dev/null +++ b/test/go_repo_transitive_deps/test_repo/third_party/go/BUILD_FILE @@ -0,0 +1,42 @@ +subinclude("///go//build_defs:go") + +filegroup( + name = "modfile", + srcs = ["go.mod"], +) + +go_stdlib( + name = "std", +) + +go_repo( + name = "testify", + licences = ["MIT"], + module = "github.com/stretchr/testify", + install = ["..."], + version = "v1.7.0", +) + +go_repo( + licences = ["MIT"], + module = "github.com/stretchr/objx", + version = "v0.5.0", +) + +go_repo( + licences = ["BSD-3-Clause"], + module = "github.com/pmezard/go-difflib", + version = "v1.0.0", +) + +go_repo( + licences = ["ISC"], + module = "github.com/davecgh/go-spew", + version = "v1.1.1", +) + +go_repo( + licences = ["MIT"], + module = "gopkg.in/yaml.v3", + version = "v3.0.0-20210107192922-496545a6307b", +) diff --git a/test/go_repo_transitive_deps/test_repo/third_party/go/go.mod b/test/go_repo_transitive_deps/test_repo/third_party/go/go.mod new file mode 100644 index 0000000..0d2f3c4 --- /dev/null +++ b/test/go_repo_transitive_deps/test_repo/third_party/go/go.mod @@ -0,0 +1,10 @@ +module gorepotransitivedeps + +go 1.21 + +require ( + // For this test to work this go.mod should only ever list "testify" as a dependency. + // The point of the test is to ensure that the go.mod of "testify" (or any third-party) + // itself is used as part of dependency resolving. + github.com/stretchr/testify v1.8.4 +) From 12abc9023858cd84f790ec971318db4864443c18 Mon Sep 17 00:00:00 2001 From: jan Date: Wed, 7 Feb 2024 13:24:55 +0000 Subject: [PATCH 3/9] fix indentation --- test/go_repo_transitive_deps/test_repo/third_party/go/go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/go_repo_transitive_deps/test_repo/third_party/go/go.mod b/test/go_repo_transitive_deps/test_repo/third_party/go/go.mod index 0d2f3c4..7491f7f 100644 --- a/test/go_repo_transitive_deps/test_repo/third_party/go/go.mod +++ b/test/go_repo_transitive_deps/test_repo/third_party/go/go.mod @@ -6,5 +6,5 @@ require ( // For this test to work this go.mod should only ever list "testify" as a dependency. // The point of the test is to ensure that the go.mod of "testify" (or any third-party) // itself is used as part of dependency resolving. - github.com/stretchr/testify v1.8.4 + github.com/stretchr/testify v1.8.4 ) From 96cc3e790edd352e34b5b2ec614411d97ad94030 Mon Sep 17 00:00:00 2001 From: jan Date: Wed, 7 Feb 2024 16:06:07 +0000 Subject: [PATCH 4/9] Refactor gomod logic into a separate pkg to make testing easier. Add tests. --- tools/please_go/generate/BUILD | 4 +- tools/please_go/generate/generate.go | 56 ++--------- tools/please_go/generate/gomoddeps/BUILD | 22 +++++ .../please_go/generate/gomoddeps/gomoddeps.go | 71 ++++++++++++++ .../generate/gomoddeps/gomoddeps_test.go | 95 +++++++++++++++++++ .../generate/gomoddeps/testdata/BUILD | 10 ++ .../generate/gomoddeps/testdata/host_go_mod | 11 +++ .../gomoddeps/testdata/invalid_go_mod | 7 ++ .../generate/gomoddeps/testdata/module_go_mod | 10 ++ 9 files changed, 235 insertions(+), 51 deletions(-) create mode 100644 tools/please_go/generate/gomoddeps/BUILD create mode 100644 tools/please_go/generate/gomoddeps/gomoddeps.go create mode 100644 tools/please_go/generate/gomoddeps/gomoddeps_test.go create mode 100644 tools/please_go/generate/gomoddeps/testdata/BUILD create mode 100644 tools/please_go/generate/gomoddeps/testdata/host_go_mod create mode 100644 tools/please_go/generate/gomoddeps/testdata/invalid_go_mod create mode 100644 tools/please_go/generate/gomoddeps/testdata/module_go_mod diff --git a/tools/please_go/generate/BUILD b/tools/please_go/generate/BUILD index 03c3100..50f0115 100644 --- a/tools/please_go/generate/BUILD +++ b/tools/please_go/generate/BUILD @@ -12,7 +12,7 @@ go_library( visibility = ["//tools/..."], deps = [ "//third_party/go:buildtools", - "//third_party/go:mod", + "//tools/please_go/generate/gomoddeps", ], ) @@ -23,4 +23,4 @@ go_test( "//third_party/go:testify", ":generate", ], -) \ No newline at end of file +) diff --git a/tools/please_go/generate/generate.go b/tools/please_go/generate/generate.go index 1312c2f..04b0a2a 100644 --- a/tools/please_go/generate/generate.go +++ b/tools/please_go/generate/generate.go @@ -2,7 +2,6 @@ package generate import ( "bufio" - "errors" "fmt" "go/build" "io/fs" @@ -12,10 +11,10 @@ import ( "path/filepath" "strings" - "golang.org/x/mod/modfile" - bazelbuild "github.com/bazelbuild/buildtools/build" bazeledit "github.com/bazelbuild/buildtools/edit" + + "github.com/please-build/go-rules/tools/please_go/generate/gomoddeps" ) type Generate struct { @@ -60,23 +59,12 @@ func New(srcRoot, thirdPartyFolder, hostModFile, module, version, subrepo string // Generate generates a new Please project at the src root. It will walk through the directory tree generating new BUILD // files. This is primarily intended to generate a please subrepo for third party code. func (g *Generate) Generate() error { - readModuleReplacePaths := false - if g.hostModFile != "" { - if err := g.readGoMod(g.hostModFile, true); err != nil { - return fmt.Errorf("failed to read host repo go.mod %q: %w", g.hostModFile, err) - } - } else { - // We only want to read the dep replacement rules if there isn't any host go.mod, otherwise third-party dependencies - // would dictate the host's naming. There could also be conflicts across different third-party modules. - readModuleReplacePaths = true - } - - moduleGoMod := path.Join(g.srcRoot, "go.mod") - if err := g.readGoMod(moduleGoMod, readModuleReplacePaths); err != nil { - if !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("failed to read module go.mod %q: %w", moduleGoMod, err) - } + deps, replacements, err := gomoddeps.GetCombinedDepsAndRequirements(g.hostModFile, path.Join(g.srcRoot, "go.mod")) + if err != nil { + return err } + g.moduleDeps = append(deps, g.moduleName) + g.replace = replacements if err := g.writeConfig(); err != nil { return fmt.Errorf("failed to write config: %w", err) @@ -211,36 +199,6 @@ func (g *Generate) writeInstallFilegroup() error { return saveBuildFile(buildFile) } -// readGoMod reads the module dependencies -func (g *Generate) readGoMod(path string, readReplacePaths bool) error { - data, err := os.ReadFile(path) - if err != nil { - return err - } - modFile, err := modfile.ParseLax(path, data, nil) - if err != nil { - return err - } - - // TODO we could probably validate these are known modules - for _, req := range modFile.Require { - g.moduleDeps = append(g.moduleDeps, req.Mod.Path) - } - - g.moduleDeps = append(g.moduleDeps, g.moduleName) - - if readReplacePaths { - if g.replace == nil { - g.replace = make(map[string]string, len(modFile.Replace)) - } - for _, replace := range modFile.Replace { - g.replace[replace.Old.Path] = replace.New.Path - } - } - - return nil -} - func (g *Generate) writeConfig() error { file, err := os.Create(filepath.Join(g.srcRoot, ".plzconfig")) if err != nil { diff --git a/tools/please_go/generate/gomoddeps/BUILD b/tools/please_go/generate/gomoddeps/BUILD new file mode 100644 index 0000000..ad69479 --- /dev/null +++ b/tools/please_go/generate/gomoddeps/BUILD @@ -0,0 +1,22 @@ +subinclude("///go//build_defs:go") + +go_library( + name = "gomoddeps", + srcs = glob(["*.go"], exclude=["*_test.go"]), + visibility = ["//tools/please_go/generate/..."], + deps = [ + "//third_party/go:mod", + ], +) + +go_test( + name = "gomoddeps_test", + srcs = glob(["*_test.go"]), + data = [ + "//tools/please_go/generate/gomoddeps/testdata:test_go_mod_files", + ], + deps = [ + "//third_party/go:testify", + ":gomoddeps", + ], +) \ No newline at end of file diff --git a/tools/please_go/generate/gomoddeps/gomoddeps.go b/tools/please_go/generate/gomoddeps/gomoddeps.go new file mode 100644 index 0000000..e6dc4fc --- /dev/null +++ b/tools/please_go/generate/gomoddeps/gomoddeps.go @@ -0,0 +1,71 @@ +package gomoddeps + +import ( + "errors" + "fmt" + "io/fs" + "os" + + "golang.org/x/mod/modfile" +) + +// GetCombinedDepsAndRequirements returns dependencies and replacements after inspecting both +// the host and the module go.mod files. +// Module's replacement are only returned if there is no host go.mod file. +func GetCombinedDepsAndRequirements(hostGoModPath, moduleGoModPath string) ([]string, map[string]string, error) { + var err error + + hostDeps := []string{} + hostReplacements := map[string]string{} + if hostGoModPath != "" { + hostDeps, hostReplacements, err = getDepsAndReplacements(hostGoModPath) + if err != nil { + return nil, nil, fmt.Errorf("failed to read host repo go.mod %q: %w", hostGoModPath, err) + } + } + + var moduleDeps []string + var moduleReplacements = map[string]string{} + moduleDeps, moduleReplacements, err = getDepsAndReplacements(moduleGoModPath) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return hostDeps, hostReplacements, nil + } + return nil, nil, fmt.Errorf("failed to read module go.mod %q: %w", moduleGoModPath, err) + } + + var replacements map[string]string + if hostGoModPath == "" { + replacements = moduleReplacements + } else { + replacements = hostReplacements + } + + return append(hostDeps, moduleDeps...), replacements, nil +} + +// getDepsAndReplacements parses the go.mod file and returns all the dependencies +// and replacement directives from it. +func getDepsAndReplacements(goModPath string) ([]string, map[string]string, error) { + data, err := os.ReadFile(goModPath) + if err != nil { + return nil, nil, err + } + modFile, err := modfile.Parse(goModPath, data, nil) + if err != nil { + return nil, nil, err + } + + moduleDeps := make([]string, 0, len(modFile.Require)) + // TODO we could probably validate these are known modules + for _, req := range modFile.Require { + moduleDeps = append(moduleDeps, req.Mod.Path) + } + + replacements := make(map[string]string, len(modFile.Replace)) + for _, replace := range modFile.Replace { + replacements[replace.Old.Path] = replace.New.Path + } + + return moduleDeps, replacements, nil +} diff --git a/tools/please_go/generate/gomoddeps/gomoddeps_test.go b/tools/please_go/generate/gomoddeps/gomoddeps_test.go new file mode 100644 index 0000000..03a6470 --- /dev/null +++ b/tools/please_go/generate/gomoddeps/gomoddeps_test.go @@ -0,0 +1,95 @@ +package gomoddeps + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +var hostGoModPath = "tools/please_go/generate/gomoddeps/testdata/host_go_mod" +var moduleGoModPath = "tools/please_go/generate/gomoddeps/testdata/module_go_mod" +var invalidGoModPath = "tools/please_go/generate/gomoddeps/testdata/invalid_go_mod" + +func TestErrors(t *testing.T) { + t.Run("errors if host go.mod does not exist", func(t *testing.T) { + deps, replacements, err := GetCombinedDepsAndRequirements("/does/not/exist", "/does/not/matter") + assert.Error(t, err) + assert.Nil(t, deps) + assert.Nil(t, replacements) + }) + + t.Run("does not error if module go.mod does not exist", func(t *testing.T) { + deps, replacements, err := GetCombinedDepsAndRequirements(hostGoModPath, "/does/not/exist") + assert.NoError(t, err) + assert.NotNil(t, deps) + assert.NotNil(t, replacements) + }) + + t.Run("errors if host go.mod is invalid", func(t *testing.T) { + deps, replacements, err := GetCombinedDepsAndRequirements(invalidGoModPath, "/does/not/exist") + assert.Error(t, err) + assert.Nil(t, deps) + assert.Nil(t, replacements) + }) + + t.Run("errors if module go.mod is invalid", func(t *testing.T) { + deps, replacements, err := GetCombinedDepsAndRequirements(hostGoModPath, invalidGoModPath) + assert.Error(t, err) + assert.Nil(t, deps) + assert.Nil(t, replacements) + }) +} + +func TestHostOnlyDeps(t *testing.T) { + t.Run("returns all host dependencies", func(t *testing.T) { + deps, _, err := GetCombinedDepsAndRequirements(hostGoModPath, "/does/not/matter") + assert.NoError(t, err) + + assert.Len(t, deps, 3) + assert.Equal(t, []string{"example.com/foo", "example.com/bar", "example.com/bob"}, deps) + }) + + t.Run("returns all host replacements", func(t *testing.T) { + _, replacements, err := GetCombinedDepsAndRequirements(hostGoModPath, "/does/not/matter") + assert.NoError(t, err) + + assert.Len(t, replacements, 1) + assert.Equal(t, map[string]string{"example.com/bob": "example.com/new-bob"}, replacements) + }) +} + +func TestModuleOnlyDeps(t *testing.T) { + t.Run("returns all host dependencies", func(t *testing.T) { + deps, _, err := GetCombinedDepsAndRequirements("", moduleGoModPath) + assert.NoError(t, err) + + assert.Len(t, deps, 2) + assert.Equal(t, []string{"example.com/dob", "example.com/bab"}, deps) + }) + + t.Run("returns all host replacements", func(t *testing.T) { + _, replacements, err := GetCombinedDepsAndRequirements("", moduleGoModPath) + assert.NoError(t, err) + + assert.Len(t, replacements, 1) + assert.Equal(t, map[string]string{"example.com/bab": "example.com/new-bab"}, replacements) + }) +} + +func TestBothDeps(t *testing.T) { + t.Run("returns combined dependencies", func(t *testing.T) { + deps, _, err := GetCombinedDepsAndRequirements(hostGoModPath, moduleGoModPath) + assert.NoError(t, err) + + assert.Len(t, deps, 5) + assert.Equal(t, []string{"example.com/foo", "example.com/bar", "example.com/bob", "example.com/dob", "example.com/bab"}, deps) + }) + + t.Run("returns only host replacements", func(t *testing.T) { + _, replacements, err := GetCombinedDepsAndRequirements(hostGoModPath, moduleGoModPath) + assert.NoError(t, err) + + assert.Len(t, replacements, 1) + assert.Equal(t, map[string]string{"example.com/bob": "example.com/new-bob"}, replacements) + }) +} diff --git a/tools/please_go/generate/gomoddeps/testdata/BUILD b/tools/please_go/generate/gomoddeps/testdata/BUILD new file mode 100644 index 0000000..9b2e08e --- /dev/null +++ b/tools/please_go/generate/gomoddeps/testdata/BUILD @@ -0,0 +1,10 @@ +filegroup( + name = "test_go_mod_files", + srcs = [ + "host_go_mod", + "invalid_go_mod", + "module_go_mod", + ], + test_only = True, + visibility = ["//tools/please_go/generate/..."], +) diff --git a/tools/please_go/generate/gomoddeps/testdata/host_go_mod b/tools/please_go/generate/gomoddeps/testdata/host_go_mod new file mode 100644 index 0000000..5d3b7e3 --- /dev/null +++ b/tools/please_go/generate/gomoddeps/testdata/host_go_mod @@ -0,0 +1,11 @@ +module hostmod + +go 1.21 + +require ( + example.com/foo v0.0.0-1234567890 + example.com/bar v1.1.1 + example.com/bob v1.2.2 +) + +replace example.com/bob => example.com/new-bob v42.0.0 diff --git a/tools/please_go/generate/gomoddeps/testdata/invalid_go_mod b/tools/please_go/generate/gomoddeps/testdata/invalid_go_mod new file mode 100644 index 0000000..79a4974 --- /dev/null +++ b/tools/please_go/generate/gomoddeps/testdata/invalid_go_mod @@ -0,0 +1,7 @@ +module malformed + +go 1.21 + +require # uh oh no ( here ... + example.com/foo v0.0.0-1234567890 +) diff --git a/tools/please_go/generate/gomoddeps/testdata/module_go_mod b/tools/please_go/generate/gomoddeps/testdata/module_go_mod new file mode 100644 index 0000000..811a71a --- /dev/null +++ b/tools/please_go/generate/gomoddeps/testdata/module_go_mod @@ -0,0 +1,10 @@ +module modulemod + +go 1.21 + +require ( + example.com/dob v1.1.1 + example.com/bab v1.2.2 +) + +replace example.com/bab => example.com/new-bab v42.0.0 From 5905326866d178b33ca430e813fca76edc4121e1 Mon Sep 17 00:00:00 2001 From: jan Date: Thu, 8 Feb 2024 09:04:17 +0000 Subject: [PATCH 5/9] Sort out dependencies correctly --- tools/please_go/BUILD | 1 + tools/please_go/generate/gomoddeps/BUILD | 12 ++++++++++-- tools/please_go/generate/gomoddeps/gomoddeps_test.go | 6 +++--- .../generate/gomoddeps/{testdata => test_data}/BUILD | 0 .../gomoddeps/{testdata => test_data}/host_go_mod | 0 .../gomoddeps/{testdata => test_data}/invalid_go_mod | 0 .../gomoddeps/{testdata => test_data}/module_go_mod | 0 7 files changed, 14 insertions(+), 5 deletions(-) rename tools/please_go/generate/gomoddeps/{testdata => test_data}/BUILD (100%) rename tools/please_go/generate/gomoddeps/{testdata => test_data}/host_go_mod (100%) rename tools/please_go/generate/gomoddeps/{testdata => test_data}/invalid_go_mod (100%) rename tools/please_go/generate/gomoddeps/{testdata => test_data}/module_go_mod (100%) diff --git a/tools/please_go/BUILD b/tools/please_go/BUILD index 0013204..6f386ab 100644 --- a/tools/please_go/BUILD +++ b/tools/please_go/BUILD @@ -42,6 +42,7 @@ genrule( "//tools/please_go/embed:srcs", "//tools/please_go/filter:srcs", "//tools/please_go/generate:srcs", + "//tools/please_go/generate/gomoddeps:srcs", "//tools/please_go/goget:srcs", "//tools/please_go/install:srcs", "//tools/please_go/install/exec:srcs", diff --git a/tools/please_go/generate/gomoddeps/BUILD b/tools/please_go/generate/gomoddeps/BUILD index ad69479..4ed194c 100644 --- a/tools/please_go/generate/gomoddeps/BUILD +++ b/tools/please_go/generate/gomoddeps/BUILD @@ -1,8 +1,16 @@ subinclude("///go//build_defs:go") +filegroup( + name = "srcs", + srcs = glob(["*.go"], exclude=["*_test.go"]), + visibility = ["//tools/please_go:bootstrap"], +) + go_library( name = "gomoddeps", - srcs = glob(["*.go"], exclude=["*_test.go"]), + srcs = [ + ":srcs", + ], visibility = ["//tools/please_go/generate/..."], deps = [ "//third_party/go:mod", @@ -13,7 +21,7 @@ go_test( name = "gomoddeps_test", srcs = glob(["*_test.go"]), data = [ - "//tools/please_go/generate/gomoddeps/testdata:test_go_mod_files", + "//tools/please_go/generate/gomoddeps/test_data:test_go_mod_files", ], deps = [ "//third_party/go:testify", diff --git a/tools/please_go/generate/gomoddeps/gomoddeps_test.go b/tools/please_go/generate/gomoddeps/gomoddeps_test.go index 03a6470..f2a00a9 100644 --- a/tools/please_go/generate/gomoddeps/gomoddeps_test.go +++ b/tools/please_go/generate/gomoddeps/gomoddeps_test.go @@ -6,9 +6,9 @@ import ( "github.com/stretchr/testify/assert" ) -var hostGoModPath = "tools/please_go/generate/gomoddeps/testdata/host_go_mod" -var moduleGoModPath = "tools/please_go/generate/gomoddeps/testdata/module_go_mod" -var invalidGoModPath = "tools/please_go/generate/gomoddeps/testdata/invalid_go_mod" +var hostGoModPath = "tools/please_go/generate/gomoddeps/test_data/host_go_mod" +var moduleGoModPath = "tools/please_go/generate/gomoddeps/test_data/module_go_mod" +var invalidGoModPath = "tools/please_go/generate/gomoddeps/test_data/invalid_go_mod" func TestErrors(t *testing.T) { t.Run("errors if host go.mod does not exist", func(t *testing.T) { diff --git a/tools/please_go/generate/gomoddeps/testdata/BUILD b/tools/please_go/generate/gomoddeps/test_data/BUILD similarity index 100% rename from tools/please_go/generate/gomoddeps/testdata/BUILD rename to tools/please_go/generate/gomoddeps/test_data/BUILD diff --git a/tools/please_go/generate/gomoddeps/testdata/host_go_mod b/tools/please_go/generate/gomoddeps/test_data/host_go_mod similarity index 100% rename from tools/please_go/generate/gomoddeps/testdata/host_go_mod rename to tools/please_go/generate/gomoddeps/test_data/host_go_mod diff --git a/tools/please_go/generate/gomoddeps/testdata/invalid_go_mod b/tools/please_go/generate/gomoddeps/test_data/invalid_go_mod similarity index 100% rename from tools/please_go/generate/gomoddeps/testdata/invalid_go_mod rename to tools/please_go/generate/gomoddeps/test_data/invalid_go_mod diff --git a/tools/please_go/generate/gomoddeps/testdata/module_go_mod b/tools/please_go/generate/gomoddeps/test_data/module_go_mod similarity index 100% rename from tools/please_go/generate/gomoddeps/testdata/module_go_mod rename to tools/please_go/generate/gomoddeps/test_data/module_go_mod From 477a9e3a75452a12feb089f673fed28242423f15 Mon Sep 17 00:00:00 2001 From: jan Date: Thu, 8 Feb 2024 16:27:15 +0000 Subject: [PATCH 6/9] Fix endless recurison & use ParseLax for modules If the host go.mod file contains a `replace` directive for a module that we are generating and the directive only changes the version, the dependency resolution went into an infinite recursion. Update the code so that we use strict parsing only for the host repo's go.mod and not for the module's, as that can cause issues. --- tools/please_go/generate/generate.go | 4 ++-- .../please_go/generate/gomoddeps/gomoddeps.go | 18 +++++++++++------ .../generate/gomoddeps/gomoddeps_test.go | 20 +++++++++---------- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/tools/please_go/generate/generate.go b/tools/please_go/generate/generate.go index 04b0a2a..5a0d448 100644 --- a/tools/please_go/generate/generate.go +++ b/tools/please_go/generate/generate.go @@ -59,7 +59,7 @@ func New(srcRoot, thirdPartyFolder, hostModFile, module, version, subrepo string // Generate generates a new Please project at the src root. It will walk through the directory tree generating new BUILD // files. This is primarily intended to generate a please subrepo for third party code. func (g *Generate) Generate() error { - deps, replacements, err := gomoddeps.GetCombinedDepsAndRequirements(g.hostModFile, path.Join(g.srcRoot, "go.mod")) + deps, replacements, err := gomoddeps.GetCombinedDepsAndReplacements(g.hostModFile, path.Join(g.srcRoot, "go.mod")) if err != nil { return err } @@ -440,7 +440,7 @@ func (g *Generate) depTarget(importPath string) string { return target } - if replacement, ok := g.replace[importPath]; ok { + if replacement, ok := g.replace[importPath]; ok && replacement != importPath { target := g.depTarget(replacement) g.knownImportTargets[importPath] = target return target diff --git a/tools/please_go/generate/gomoddeps/gomoddeps.go b/tools/please_go/generate/gomoddeps/gomoddeps.go index e6dc4fc..9148eed 100644 --- a/tools/please_go/generate/gomoddeps/gomoddeps.go +++ b/tools/please_go/generate/gomoddeps/gomoddeps.go @@ -9,16 +9,16 @@ import ( "golang.org/x/mod/modfile" ) -// GetCombinedDepsAndRequirements returns dependencies and replacements after inspecting both +// GetCombinedDepsAndReplacements returns dependencies and replacements after inspecting both // the host and the module go.mod files. // Module's replacement are only returned if there is no host go.mod file. -func GetCombinedDepsAndRequirements(hostGoModPath, moduleGoModPath string) ([]string, map[string]string, error) { +func GetCombinedDepsAndReplacements(hostGoModPath, moduleGoModPath string) ([]string, map[string]string, error) { var err error hostDeps := []string{} hostReplacements := map[string]string{} if hostGoModPath != "" { - hostDeps, hostReplacements, err = getDepsAndReplacements(hostGoModPath) + hostDeps, hostReplacements, err = getDepsAndReplacements(hostGoModPath, false) if err != nil { return nil, nil, fmt.Errorf("failed to read host repo go.mod %q: %w", hostGoModPath, err) } @@ -26,7 +26,7 @@ func GetCombinedDepsAndRequirements(hostGoModPath, moduleGoModPath string) ([]st var moduleDeps []string var moduleReplacements = map[string]string{} - moduleDeps, moduleReplacements, err = getDepsAndReplacements(moduleGoModPath) + moduleDeps, moduleReplacements, err = getDepsAndReplacements(moduleGoModPath, true) if err != nil { if errors.Is(err, fs.ErrNotExist) { return hostDeps, hostReplacements, nil @@ -46,12 +46,18 @@ func GetCombinedDepsAndRequirements(hostGoModPath, moduleGoModPath string) ([]st // getDepsAndReplacements parses the go.mod file and returns all the dependencies // and replacement directives from it. -func getDepsAndReplacements(goModPath string) ([]string, map[string]string, error) { +func getDepsAndReplacements(goModPath string, useLaxParsing bool) ([]string, map[string]string, error) { data, err := os.ReadFile(goModPath) if err != nil { return nil, nil, err } - modFile, err := modfile.Parse(goModPath, data, nil) + + var modFile *modfile.File + if useLaxParsing { + modFile, err = modfile.ParseLax(goModPath, data, nil) + } else { + modFile, err = modfile.Parse(goModPath, data, nil) + } if err != nil { return nil, nil, err } diff --git a/tools/please_go/generate/gomoddeps/gomoddeps_test.go b/tools/please_go/generate/gomoddeps/gomoddeps_test.go index f2a00a9..96707c1 100644 --- a/tools/please_go/generate/gomoddeps/gomoddeps_test.go +++ b/tools/please_go/generate/gomoddeps/gomoddeps_test.go @@ -12,28 +12,28 @@ var invalidGoModPath = "tools/please_go/generate/gomoddeps/test_data/invalid_go_ func TestErrors(t *testing.T) { t.Run("errors if host go.mod does not exist", func(t *testing.T) { - deps, replacements, err := GetCombinedDepsAndRequirements("/does/not/exist", "/does/not/matter") + deps, replacements, err := GetCombinedDepsAndReplacements("/does/not/exist", "/does/not/matter") assert.Error(t, err) assert.Nil(t, deps) assert.Nil(t, replacements) }) t.Run("does not error if module go.mod does not exist", func(t *testing.T) { - deps, replacements, err := GetCombinedDepsAndRequirements(hostGoModPath, "/does/not/exist") + deps, replacements, err := GetCombinedDepsAndReplacements(hostGoModPath, "/does/not/exist") assert.NoError(t, err) assert.NotNil(t, deps) assert.NotNil(t, replacements) }) t.Run("errors if host go.mod is invalid", func(t *testing.T) { - deps, replacements, err := GetCombinedDepsAndRequirements(invalidGoModPath, "/does/not/exist") + deps, replacements, err := GetCombinedDepsAndReplacements(invalidGoModPath, "/does/not/exist") assert.Error(t, err) assert.Nil(t, deps) assert.Nil(t, replacements) }) t.Run("errors if module go.mod is invalid", func(t *testing.T) { - deps, replacements, err := GetCombinedDepsAndRequirements(hostGoModPath, invalidGoModPath) + deps, replacements, err := GetCombinedDepsAndReplacements(hostGoModPath, invalidGoModPath) assert.Error(t, err) assert.Nil(t, deps) assert.Nil(t, replacements) @@ -42,7 +42,7 @@ func TestErrors(t *testing.T) { func TestHostOnlyDeps(t *testing.T) { t.Run("returns all host dependencies", func(t *testing.T) { - deps, _, err := GetCombinedDepsAndRequirements(hostGoModPath, "/does/not/matter") + deps, _, err := GetCombinedDepsAndReplacements(hostGoModPath, "/does/not/matter") assert.NoError(t, err) assert.Len(t, deps, 3) @@ -50,7 +50,7 @@ func TestHostOnlyDeps(t *testing.T) { }) t.Run("returns all host replacements", func(t *testing.T) { - _, replacements, err := GetCombinedDepsAndRequirements(hostGoModPath, "/does/not/matter") + _, replacements, err := GetCombinedDepsAndReplacements(hostGoModPath, "/does/not/matter") assert.NoError(t, err) assert.Len(t, replacements, 1) @@ -60,7 +60,7 @@ func TestHostOnlyDeps(t *testing.T) { func TestModuleOnlyDeps(t *testing.T) { t.Run("returns all host dependencies", func(t *testing.T) { - deps, _, err := GetCombinedDepsAndRequirements("", moduleGoModPath) + deps, _, err := GetCombinedDepsAndReplacements("", moduleGoModPath) assert.NoError(t, err) assert.Len(t, deps, 2) @@ -68,7 +68,7 @@ func TestModuleOnlyDeps(t *testing.T) { }) t.Run("returns all host replacements", func(t *testing.T) { - _, replacements, err := GetCombinedDepsAndRequirements("", moduleGoModPath) + _, replacements, err := GetCombinedDepsAndReplacements("", moduleGoModPath) assert.NoError(t, err) assert.Len(t, replacements, 1) @@ -78,7 +78,7 @@ func TestModuleOnlyDeps(t *testing.T) { func TestBothDeps(t *testing.T) { t.Run("returns combined dependencies", func(t *testing.T) { - deps, _, err := GetCombinedDepsAndRequirements(hostGoModPath, moduleGoModPath) + deps, _, err := GetCombinedDepsAndReplacements(hostGoModPath, moduleGoModPath) assert.NoError(t, err) assert.Len(t, deps, 5) @@ -86,7 +86,7 @@ func TestBothDeps(t *testing.T) { }) t.Run("returns only host replacements", func(t *testing.T) { - _, replacements, err := GetCombinedDepsAndRequirements(hostGoModPath, moduleGoModPath) + _, replacements, err := GetCombinedDepsAndReplacements(hostGoModPath, moduleGoModPath) assert.NoError(t, err) assert.Len(t, replacements, 1) From a2fde40af76f9a2718c76b021829c13d19e839ad Mon Sep 17 00:00:00 2001 From: jan Date: Thu, 8 Feb 2024 16:32:28 +0000 Subject: [PATCH 7/9] Bump version to 2.0.0 --- tools/please_go/ChangeLog | 5 +++++ tools/please_go/VERSION | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/tools/please_go/ChangeLog b/tools/please_go/ChangeLog index b180d13..396e593 100644 --- a/tools/please_go/ChangeLog +++ b/tools/please_go/ChangeLog @@ -1,3 +1,8 @@ +Version 2.0.0 +------------- + * Inspect both the host repo and the module's go.mod when resolving dependencies in the + `generate` command. The breaking change is the renaming of `--mod_file` to `--host_mod_file`. + Version 1.9.2 ------------- * Generate correct paths for subdir imports within a module. diff --git a/tools/please_go/VERSION b/tools/please_go/VERSION index 8fdcf38..227cea2 100644 --- a/tools/please_go/VERSION +++ b/tools/please_go/VERSION @@ -1 +1 @@ -1.9.2 +2.0.0 From 65f8712a4e83ecbe2ca6686ebfd1f05019a40c20 Mon Sep 17 00:00:00 2001 From: jan Date: Thu, 8 Feb 2024 16:49:59 +0000 Subject: [PATCH 8/9] correctly handle module only replacement parsing --- tools/please_go/generate/gomoddeps/gomoddeps.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tools/please_go/generate/gomoddeps/gomoddeps.go b/tools/please_go/generate/gomoddeps/gomoddeps.go index 9148eed..93ad45f 100644 --- a/tools/please_go/generate/gomoddeps/gomoddeps.go +++ b/tools/please_go/generate/gomoddeps/gomoddeps.go @@ -1,3 +1,4 @@ +// Package gomoddeps parses dependencies and replacements from the host and module go.mod files. package gomoddeps import ( @@ -26,7 +27,13 @@ func GetCombinedDepsAndReplacements(hostGoModPath, moduleGoModPath string) ([]st var moduleDeps []string var moduleReplacements = map[string]string{} - moduleDeps, moduleReplacements, err = getDepsAndReplacements(moduleGoModPath, true) + useLaxParsingForModule := true + if hostGoModPath == "" { + // If we're only considering the module then we want to extract the replacement's as well (lax mode + // doesn't parse them). + useLaxParsingForModule = false + } + moduleDeps, moduleReplacements, err = getDepsAndReplacements(moduleGoModPath, useLaxParsingForModule) if err != nil { if errors.Is(err, fs.ErrNotExist) { return hostDeps, hostReplacements, nil From 369ecde1359e5815644ed792e66c1125380ffd06 Mon Sep 17 00:00:00 2001 From: jan Date: Fri, 9 Feb 2024 09:29:28 +0000 Subject: [PATCH 9/9] Remove backwards incompatiable change --- build_defs/go.build_defs | 2 +- tools/please_go/ChangeLog | 4 ++-- tools/please_go/VERSION | 2 +- tools/please_go/please_go.go | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/build_defs/go.build_defs b/build_defs/go.build_defs index f85373f..a47131b 100644 --- a/build_defs/go.build_defs +++ b/build_defs/go.build_defs @@ -1219,7 +1219,7 @@ def go_repo(module: str, version:str='', download:str=None, name:str=None, insta "download": [download], } if CONFIG.GO.MOD_FILE: - modFileArg = "--host_mod_file $SRCS_MOD_FILE" + modFileArg = "--mod_file $SRCS_MOD_FILE" srcs["mod_file"] = [CONFIG.GO.MOD_FILE] else: modFileArg = "" diff --git a/tools/please_go/ChangeLog b/tools/please_go/ChangeLog index 396e593..bcc376d 100644 --- a/tools/please_go/ChangeLog +++ b/tools/please_go/ChangeLog @@ -1,7 +1,7 @@ -Version 2.0.0 +Version 1.10.0 ------------- * Inspect both the host repo and the module's go.mod when resolving dependencies in the - `generate` command. The breaking change is the renaming of `--mod_file` to `--host_mod_file`. + `generate` command. Version 1.9.2 ------------- diff --git a/tools/please_go/VERSION b/tools/please_go/VERSION index 227cea2..81c871d 100644 --- a/tools/please_go/VERSION +++ b/tools/please_go/VERSION @@ -1 +1 @@ -2.0.0 +1.10.0 diff --git a/tools/please_go/please_go.go b/tools/please_go/please_go.go index a0aa2fd..ca8f03c 100644 --- a/tools/please_go/please_go.go +++ b/tools/please_go/please_go.go @@ -96,7 +96,7 @@ var opts = struct { SrcRoot string `short:"r" long:"src_root" description:"The src root of the module to inspect"` ImportPath string `long:"import_path" description:"overrides the module's import path. If not set, the import path from the go.mod will be used.'"` ThirdPartyFolder string `short:"t" long:"third_part_folder" description:"The folder containing the third party subrepos" default:"third_party/go"` - HostModFile string `long:"host_mod_file" description:"Path to the host repo mod file to use to resolve dependencies against (dependencies will be resolved against the module as well if it exists)"` + ModFile string `long:"mod_file" description:"Path to the host repo mod file to use to resolve dependencies against (dependencies will be resolved against the module as well if it exists)"` Module string `long:"module" description:"The name of the current module"` Version string `long:"version" description:"The version of the current module"` Install []string `long:"install" description:"The packages to add to the :install alias"` @@ -184,7 +184,7 @@ var subCommands = map[string]func() int{ }, "generate": func() int { gen := opts.Generate - g := generate.New(gen.SrcRoot, gen.ThirdPartyFolder, gen.HostModFile, gen.Module, gen.Version, gen.Subrepo, []string{"BUILD", "BUILD.plz"}, gen.Args.Requirements, gen.Install, gen.BuildTags) + g := generate.New(gen.SrcRoot, gen.ThirdPartyFolder, gen.ModFile, gen.Module, gen.Version, gen.Subrepo, []string{"BUILD", "BUILD.plz"}, gen.Args.Requirements, gen.Install, gen.BuildTags) if err := g.Generate(); err != nil { log.Fatalf("failed to generate go rules: %v", err) }