From c70d42cc33dd7f0035b869ef7c510fcd7c598d05 Mon Sep 17 00:00:00 2001 From: Jade Guiton Date: Thu, 19 Dec 2024 21:06:31 +0100 Subject: [PATCH] Add `crosslink tidylist` command (#642) * Add `crosslink tidy` command * Applied suggestions and added test * Fixed linting and license check * Enable schedule validation in test * Renamed "tidy" to "tidylist" --------- Co-authored-by: Pablo Baeyens --- .chloggen/crosslink-tidy-v1.yaml | 16 ++ crosslink/README.md | 10 + crosslink/cmd/root.go | 32 ++- crosslink/internal/common.go | 23 ++ crosslink/internal/config.go | 2 + crosslink/internal/graph.go | 24 +- .../mock_test_data/testTidyListAcyclic/gomod | 3 + .../testTidyListAcyclic/testA/gomod | 5 + .../testTidyListAcyclic/testB/gomod | 5 + .../testTidyListAcyclic/testC/gomod | 3 + .../allow-circular-overpermissive.txt | 5 + .../testTidyListCyclic/allow-circular.txt | 4 + .../mock_test_data/testTidyListCyclic/gomod | 3 + .../testTidyListCyclic/testA/gomod | 5 + .../testTidyListCyclic/testB/gomod | 6 + .../testTidyListCyclic/testC/gomod | 3 + crosslink/internal/tidylist.go | 219 ++++++++++++++++++ crosslink/internal/tidylist_test.go | 96 ++++++++ 18 files changed, 436 insertions(+), 28 deletions(-) create mode 100644 .chloggen/crosslink-tidy-v1.yaml create mode 100644 crosslink/internal/mock_test_data/testTidyListAcyclic/gomod create mode 100644 crosslink/internal/mock_test_data/testTidyListAcyclic/testA/gomod create mode 100644 crosslink/internal/mock_test_data/testTidyListAcyclic/testB/gomod create mode 100644 crosslink/internal/mock_test_data/testTidyListAcyclic/testC/gomod create mode 100644 crosslink/internal/mock_test_data/testTidyListCyclic/allow-circular-overpermissive.txt create mode 100644 crosslink/internal/mock_test_data/testTidyListCyclic/allow-circular.txt create mode 100644 crosslink/internal/mock_test_data/testTidyListCyclic/gomod create mode 100644 crosslink/internal/mock_test_data/testTidyListCyclic/testA/gomod create mode 100644 crosslink/internal/mock_test_data/testTidyListCyclic/testB/gomod create mode 100644 crosslink/internal/mock_test_data/testTidyListCyclic/testC/gomod create mode 100644 crosslink/internal/tidylist.go create mode 100644 crosslink/internal/tidylist_test.go diff --git a/.chloggen/crosslink-tidy-v1.yaml b/.chloggen/crosslink-tidy-v1.yaml new file mode 100644 index 00000000..9e866bd1 --- /dev/null +++ b/.chloggen/crosslink-tidy-v1.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: 'enhancement' + +# The name of the component, or a single word describing the area of concern, (e.g. crosslink) +component: crosslink + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Adds a 'tidy' subcommand to generate 'go mod tidy' schedules + +# One or more tracking issues related to the change +issues: [642] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: diff --git a/crosslink/README.md b/crosslink/README.md index 91a1ced0..88a52782 100644 --- a/crosslink/README.md +++ b/crosslink/README.md @@ -162,3 +162,13 @@ for out-dated intra-repository Go modules. Go version applied when new `go.work` file is created (default "1.22"). crosslink work --go=1.23 + +### tidylist + +The 'tidylist' command generates a sequence of intra-repository modules, such that +running 'go mod tidy' on each module in this sequence guarantees that changes +in the 'go.mod' of one module will be propagated to all of its dependent +modules. This ensures that no modules are left in a broken 'updates to go.mod +needed' state at the end. For an acyclic dependency graph, this corresponds to +topological order. If modules are found to have circular dependencies, they will +be checked against a provided allowlist. diff --git a/crosslink/cmd/root.go b/crosslink/cmd/root.go index df4d4a59..4f67251c 100644 --- a/crosslink/cmd/root.go +++ b/crosslink/cmd/root.go @@ -29,12 +29,13 @@ import ( ) type commandConfig struct { - runConfig cl.RunConfig - excludeFlags []string - skipFlags []string - rootCommand cobra.Command - pruneCommand cobra.Command - workCommand cobra.Command + runConfig cl.RunConfig + excludeFlags []string + skipFlags []string + rootCommand cobra.Command + pruneCommand cobra.Command + workCommand cobra.Command + tidyListCommand cobra.Command } func newCommandConfig() *commandConfig { @@ -118,6 +119,23 @@ func newCommandConfig() *commandConfig { } c.rootCommand.AddCommand(&c.workCommand) + c.tidyListCommand = cobra.Command{ + Use: "tidylist output_path", + Short: "Generate a schedule of modules to tidy that guarantees full propagation of changes", + Long: "The 'tidylist' command generates a sequence of intra-repository modules, such that\n" + + "running 'go mod tidy' on each module in this sequence guarantees that changes\n" + + "in the 'go.mod' of one module will be propagated to all of its dependent\n" + + "modules. This ensures that no modules are left in a broken 'updates to go.mod\n" + + "needed' state at the end. For an acyclic dependency graph, this corresponds to\n" + + "topological order. If modules are found to have circular dependencies, they will\n" + + "be checked against a provided allowlist.", + Args: cobra.ExactArgs(1), + RunE: func(_ *cobra.Command, args []string) error { + return cl.TidyList(c.runConfig, args[0]) + }, + } + c.rootCommand.AddCommand(&c.tidyListCommand) + return c } @@ -146,6 +164,8 @@ func init() { comCfg.pruneCommand.Flags().StringSliceVar(&comCfg.excludeFlags, "exclude", []string{}, "list of comma separated go modules that crosslink will ignore in operations."+ "multiple calls of --exclude can be made") comCfg.workCommand.Flags().StringVar(&comCfg.runConfig.GoVersion, "go", "1.22", "Go version applied when new go.work file is created") + comCfg.tidyListCommand.Flags().StringVar(&comCfg.runConfig.AllowCircular, "allow-circular", "", "path to list of go modules that are allowed to have circular dependencies") + comCfg.tidyListCommand.Flags().BoolVar(&comCfg.runConfig.Validate, "validate", false, "enables brute force validation of the tidy schedule") } // transform array slice into map diff --git a/crosslink/internal/common.go b/crosslink/internal/common.go index 050dbd54..46b0f2ce 100644 --- a/crosslink/internal/common.go +++ b/crosslink/internal/common.go @@ -76,3 +76,26 @@ func forGoModules(logger *zap.Logger, rootPath string, fn func(path string) erro return fn(path) }) } + +func forGoModFiles(rc RunConfig, fn func(modPath string, modName string, modFile *modfile.File) error) error { + return forGoModules(rc.Logger, rc.RootPath, func(path string) error { + if _, ok := rc.SkippedPaths[path]; ok { + rc.Logger.Debug("skipping", zap.String("path", path)) + return nil + } + fullPath := filepath.Join(rc.RootPath, path) + modFile, err := os.ReadFile(filepath.Clean(fullPath)) + if err != nil { + return fmt.Errorf("failed to read file: %w", err) + } + + modContents, err := modfile.Parse(fullPath, modFile, nil) + if err != nil { + rc.Logger.Error("Modfile could not be parsed", + zap.Error(err), + zap.String("file_path", path)) + } + + return fn(path, modfile.ModulePath(modFile), modContents) + }) +} diff --git a/crosslink/internal/config.go b/crosslink/internal/config.go index cf689381..6c983c1b 100644 --- a/crosslink/internal/config.go +++ b/crosslink/internal/config.go @@ -41,6 +41,8 @@ type RunConfig struct { Overwrite bool Prune bool GoVersion string + AllowCircular string + Validate bool Logger *zap.Logger } diff --git a/crosslink/internal/graph.go b/crosslink/internal/graph.go index adb279b9..1def0c04 100644 --- a/crosslink/internal/graph.go +++ b/crosslink/internal/graph.go @@ -16,11 +16,8 @@ package crosslink import ( "fmt" - "os" - "path/filepath" "strings" - "go.uber.org/zap" "golang.org/x/mod/modfile" ) @@ -30,25 +27,8 @@ import ( func buildDepedencyGraph(rc RunConfig, rootModulePath string) (map[string]*moduleInfo, error) { moduleMap := make(map[string]*moduleInfo) - err := forGoModules(rc.Logger, rc.RootPath, func(path string) error { - if _, ok := rc.SkippedPaths[path]; ok { - rc.Logger.Debug("skipping", zap.String("path", path)) - return nil - } - fullPath := filepath.Join(rc.RootPath, path) - modFile, err := os.ReadFile(filepath.Clean(fullPath)) - if err != nil { - return fmt.Errorf("failed to read file: %w", err) - } - - modContents, err := modfile.Parse(fullPath, modFile, nil) - if err != nil { - rc.Logger.Error("Modfile could not be parsed", - zap.Error(err), - zap.String("file_path", path)) - } - - moduleMap[modfile.ModulePath(modFile)] = newModuleInfo(*modContents) + err := forGoModFiles(rc, func(_ string, modPath string, modContents *modfile.File) error { + moduleMap[modPath] = newModuleInfo(*modContents) return nil }) if err != nil { diff --git a/crosslink/internal/mock_test_data/testTidyListAcyclic/gomod b/crosslink/internal/mock_test_data/testTidyListAcyclic/gomod new file mode 100644 index 00000000..3b5442d6 --- /dev/null +++ b/crosslink/internal/mock_test_data/testTidyListAcyclic/gomod @@ -0,0 +1,3 @@ +module go.opentelemetry.io/build-tools/crosslink/testroot + +go 1.20 diff --git a/crosslink/internal/mock_test_data/testTidyListAcyclic/testA/gomod b/crosslink/internal/mock_test_data/testTidyListAcyclic/testA/gomod new file mode 100644 index 00000000..27fff3f4 --- /dev/null +++ b/crosslink/internal/mock_test_data/testTidyListAcyclic/testA/gomod @@ -0,0 +1,5 @@ +module go.opentelemetry.io/build-tools/crosslink/testroot/testA + +go 1.20 + +require go.opentelemetry.io/build-tools/crosslink/testroot/testB v1.0.0 diff --git a/crosslink/internal/mock_test_data/testTidyListAcyclic/testB/gomod b/crosslink/internal/mock_test_data/testTidyListAcyclic/testB/gomod new file mode 100644 index 00000000..cf24b434 --- /dev/null +++ b/crosslink/internal/mock_test_data/testTidyListAcyclic/testB/gomod @@ -0,0 +1,5 @@ +module go.opentelemetry.io/build-tools/crosslink/testroot/testB + +go 1.20 + +require go.opentelemetry.io/build-tools/crosslink/testroot/testC v1.0.0 diff --git a/crosslink/internal/mock_test_data/testTidyListAcyclic/testC/gomod b/crosslink/internal/mock_test_data/testTidyListAcyclic/testC/gomod new file mode 100644 index 00000000..93ca41cd --- /dev/null +++ b/crosslink/internal/mock_test_data/testTidyListAcyclic/testC/gomod @@ -0,0 +1,3 @@ +module go.opentelemetry.io/build-tools/crosslink/testroot/testC + +go 1.20 diff --git a/crosslink/internal/mock_test_data/testTidyListCyclic/allow-circular-overpermissive.txt b/crosslink/internal/mock_test_data/testTidyListCyclic/allow-circular-overpermissive.txt new file mode 100644 index 00000000..f33fd817 --- /dev/null +++ b/crosslink/internal/mock_test_data/testTidyListCyclic/allow-circular-overpermissive.txt @@ -0,0 +1,5 @@ +# This file lists the modules that are expected to have circular dependencies. +# If it does not match the actual list, `crosslink tidylist` will return an error. +testA +testB +testC diff --git a/crosslink/internal/mock_test_data/testTidyListCyclic/allow-circular.txt b/crosslink/internal/mock_test_data/testTidyListCyclic/allow-circular.txt new file mode 100644 index 00000000..5382a087 --- /dev/null +++ b/crosslink/internal/mock_test_data/testTidyListCyclic/allow-circular.txt @@ -0,0 +1,4 @@ +# This file lists the modules that are expected to have circular dependencies. +# If it does not match the actual list, `crosslink tidylist` will return an error. +testA +testB diff --git a/crosslink/internal/mock_test_data/testTidyListCyclic/gomod b/crosslink/internal/mock_test_data/testTidyListCyclic/gomod new file mode 100644 index 00000000..3b5442d6 --- /dev/null +++ b/crosslink/internal/mock_test_data/testTidyListCyclic/gomod @@ -0,0 +1,3 @@ +module go.opentelemetry.io/build-tools/crosslink/testroot + +go 1.20 diff --git a/crosslink/internal/mock_test_data/testTidyListCyclic/testA/gomod b/crosslink/internal/mock_test_data/testTidyListCyclic/testA/gomod new file mode 100644 index 00000000..27fff3f4 --- /dev/null +++ b/crosslink/internal/mock_test_data/testTidyListCyclic/testA/gomod @@ -0,0 +1,5 @@ +module go.opentelemetry.io/build-tools/crosslink/testroot/testA + +go 1.20 + +require go.opentelemetry.io/build-tools/crosslink/testroot/testB v1.0.0 diff --git a/crosslink/internal/mock_test_data/testTidyListCyclic/testB/gomod b/crosslink/internal/mock_test_data/testTidyListCyclic/testB/gomod new file mode 100644 index 00000000..491dd283 --- /dev/null +++ b/crosslink/internal/mock_test_data/testTidyListCyclic/testB/gomod @@ -0,0 +1,6 @@ +module go.opentelemetry.io/build-tools/crosslink/testroot/testB + +go 1.20 + +require go.opentelemetry.io/build-tools/crosslink/testroot/testA v1.0.0 +require go.opentelemetry.io/build-tools/crosslink/testroot/testC v1.0.0 diff --git a/crosslink/internal/mock_test_data/testTidyListCyclic/testC/gomod b/crosslink/internal/mock_test_data/testTidyListCyclic/testC/gomod new file mode 100644 index 00000000..93ca41cd --- /dev/null +++ b/crosslink/internal/mock_test_data/testTidyListCyclic/testC/gomod @@ -0,0 +1,3 @@ +module go.opentelemetry.io/build-tools/crosslink/testroot/testC + +go 1.20 diff --git a/crosslink/internal/tidylist.go b/crosslink/internal/tidylist.go new file mode 100644 index 00000000..e0b9ffbb --- /dev/null +++ b/crosslink/internal/tidylist.go @@ -0,0 +1,219 @@ +// Copyright The OpenTelemetry Authors +// +// 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 crosslink + +import ( + "bufio" + "fmt" + "os" + "path" + "slices" + "strings" + + "go.uber.org/zap" + "golang.org/x/mod/modfile" +) + +type graphNode struct { + file *modfile.File + path string + name string + deps []string + index int + sccRoot int + onStack bool +} + +func TidyList(rc RunConfig, outputPath string) error { + rc.Logger.Debug("crosslink run config", zap.Any("run_config", rc)) + + rootModule, err := identifyRootModule(rc.RootPath) + if err != nil { + return fmt.Errorf("failed to identify root module: %w", err) + } + + // Read circular dependency allowlist + + var allowCircular []string + if rc.AllowCircular != "" { + var file *os.File + file, err = os.Open(rc.AllowCircular) + if err != nil { + return fmt.Errorf("failed to open cicular dependency allowlist: %w", err) + } + defer file.Close() + scanner := bufio.NewScanner(file) + for scanner.Scan() { + line := scanner.Text() + if line != "" && !strings.HasPrefix(line, "#") { + allowCircular = append(allowCircular, line) + } + } + if err = scanner.Err(); err != nil { + return fmt.Errorf("failed to read cicular dependency allowlist: %w", err) + } + } + + // Read intra-repository dependency graph + + graph := make(map[string]*graphNode) + var modsAlpha []string + err = forGoModFiles(rc, func(filePath string, name string, file *modfile.File) error { + if !strings.HasPrefix(name, rootModule) { + rc.Logger.Debug("ignoring module outside root module namespace", zap.String("mod_name", name)) + return nil + } + if !strings.HasSuffix(filePath, "go.mod") { + return fmt.Errorf("logic error: 'forGoModFiles' should iterate over 'go.mod' files") + } + modsAlpha = append(modsAlpha, name) + modPath := path.Dir(filePath) + graph[name] = &graphNode{ + file: file, + path: modPath, + name: name, + deps: nil, + index: -1, + sccRoot: -1, + onStack: false, + } + return nil + }) + if err != nil { + return fmt.Errorf("failed during file walk: %w", err) + } + + for _, node := range graph { + for _, req := range node.file.Require { + if _, ok := graph[req.Mod.Path]; ok { + node.deps = append(node.deps, req.Mod.Path) + } + } + } + + rc.Logger.Debug("read module graph", zap.Int("mod_cnt", len(graph))) + + // Compute tidying schedule + // We use Tarjan's algorithm to identify the topological order of strongly- + // connected components of the graph, then apply a naive solution to each. + + var modsTopo []string + nextIdx := 0 + var circular []string + var stack []*graphNode + + var visit func(mod *graphNode) + visit = func(mod *graphNode) { + mod.index = nextIdx + mod.sccRoot = nextIdx + nextIdx++ + stack = append(stack, mod) + mod.onStack = true + + for _, mod2Name := range mod.deps { + mod2 := graph[mod2Name] + if mod2.index == -1 { + visit(mod2) + } else if !mod2.onStack { + continue + } + mod.sccRoot = min(mod.sccRoot, mod2.sccRoot) + } + + if mod.index == mod.sccRoot { + var scc []string + for { + mod2 := stack[len(stack)-1] + stack = stack[:len(stack)-1] + mod2.onStack = false + scc = append(scc, mod2.path) + if mod2 == mod { + break + } + } + if len(scc) > 1 { // circular dependencies + rc.Logger.Debug("found SCC in module graph", zap.Any("scc", scc)) + circular = append(circular, scc...) + } + + // Apply a naive solution for each SCC + // (quadratic in the number of modules, but optimal for 1 or 2) + for i := 0; i < len(scc)-1; i++ { + modsTopo = append(modsTopo, scc...) + } + modsTopo = append(modsTopo, scc[0]) + } + } + for _, modName := range modsAlpha { + mod := graph[modName] + if mod.index == -1 { + visit(mod) + } + } + + rc.Logger.Debug("computed tidy schedule", zap.Int("schedule_len", len(modsTopo))) + + circularMismatch := false + for _, mod := range circular { + if !slices.Contains(allowCircular, mod) { + fmt.Printf("module has circular dependencies but is not allowlisted: %s\n", mod) + circularMismatch = true + } + } + for _, mod := range allowCircular { + if !slices.Contains(circular, mod) { + fmt.Printf("module is allowlisted but has no circular dependencies: %s\n", mod) + circularMismatch = true + } + } + if circularMismatch { + return fmt.Errorf("list of circular dependencies does not match allowlist") + } + + // Writing out schedule + err = os.WriteFile(outputPath, []byte(strings.Join(modsTopo, "\n")), 0600) + if err != nil { + return fmt.Errorf("failed to write tidy schedule file: %w", err) + } + + if rc.Validate { + // Check validity of solution + // (iterate over possible paths and check they are all subsequences of modsTopo) + + var queue [][]string + for _, modName := range modsAlpha { + queue = append(queue, []string{modName}) + } + for len(queue) > 0 { + path := queue[0] + queue = queue[1:] + i := 0 + for _, modName := range path { + i = slices.Index(path[i:], modName) + if i == -1 { + return fmt.Errorf("tidy schedule is invalid; changes may not be propagated along path: %v", path) + } + } + for _, dep := range graph[path[0]].deps { + if !slices.Contains(path, dep) { + path2 := slices.Clone(path) + queue = append(queue, slices.Insert(path2, 0, dep)) + } + } + } + } + + return nil +} diff --git a/crosslink/internal/tidylist_test.go b/crosslink/internal/tidylist_test.go new file mode 100644 index 00000000..8fe20128 --- /dev/null +++ b/crosslink/internal/tidylist_test.go @@ -0,0 +1,96 @@ +// Copyright The OpenTelemetry Authors +// +// 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 crosslink + +import ( + "os" + "path" + "strings" + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/zap" +) + +func TestTidy(t *testing.T) { + defaultConfig := DefaultRunConfig() + defaultConfig.Logger, _ = zap.NewDevelopment() + defaultConfig.Verbose = true + defaultConfig.Validate = true + + tests := []struct { + name string + mock string + config func(*RunConfig) + expErr string + expSched []string + }{ + { // A -> B -> C should give CBA + name: "testTidyListAcyclic", + mock: "testTidyListAcyclic", + config: func(*RunConfig) {}, + expSched: []string{".", "testC", "testB", "testA"}, + }, + { // A <=> B -> C without allowlist should error + name: "testTidyListNotAllowlisted", + mock: "testTidyListCyclic", + config: func(*RunConfig) {}, + expErr: "list of circular dependencies does not match allowlist", + }, + { // A <=> B -> C with an over-permissive allowlist should error + name: "testTidyListOverpermissive", + mock: "testTidyListCyclic", + config: func(config *RunConfig) { + config.AllowCircular = path.Join(config.RootPath, "allow-circular-overpermissive.txt") + }, + expErr: "list of circular dependencies does not match allowlist", + }, + { // A <=> B -> C should give CBAB + name: "testTidyListCyclic", + mock: "testTidyListCyclic", + config: func(config *RunConfig) { + config.AllowCircular = path.Join(config.RootPath, "allow-circular.txt") + }, + expSched: []string{".", "testC", "testB", "testA", "testB"}, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + testDir, err := createTempTestDir(test.mock) + require.NoError(t, err, "error creating temporary directory") + t.Cleanup(func() { os.RemoveAll(testDir) }) + err = renameGoMod(testDir) + require.NoError(t, err, "error renaming gomod files") + outputPath := path.Join(testDir, "schedule.txt") + + config := defaultConfig + config.RootPath = testDir + test.config(&config) + + err = TidyList(config, outputPath) + + if test.expErr != "" { + require.ErrorContains(t, err, test.expErr, "expected error in TidyList") + return + } + require.NoError(t, err, "unexpected error in TidyList") + + outputBytes, err := os.ReadFile(outputPath) // #nosec G304 -- Path comes from os.MkdirTemp + require.NoError(t, err, "error reading output file") + schedule := strings.Split(string(outputBytes), "\n") + require.Equal(t, test.expSched, schedule, "generated schedule differs from expectation") + }) + } +}