From 2d85ce65421e387dae1641197acc891263f8106d Mon Sep 17 00:00:00 2001 From: JC Date: Tue, 16 Jul 2024 18:23:38 +0100 Subject: [PATCH] Remove replace directive labels when not needed (#117) Previously we didn't remove labels when a replace directive was removed. This would mean replace directive labels would likely grow where they should be managed automatically by puku. This PR removes any replace directive labels on targets synced which no longer have replace directives in the go.mod. --- edit/edit.go | 34 +++++++ sync/integration/syncmod/sync_mod_test.go | 22 ++++- .../test_repo/third_party/go/BUILD_FILE | 6 ++ sync/sync.go | 89 ++++++++++++++----- 4 files changed, 124 insertions(+), 27 deletions(-) diff --git a/edit/edit.go b/edit/edit.go index f6d50d0..53e66a9 100644 --- a/edit/edit.go +++ b/edit/edit.go @@ -179,3 +179,37 @@ func AddLabel(rule *build.Rule, label string) error { rule.SetAttr("labels", ruleLabelsList) return nil } + +func RemoveLabel(rule *build.Rule, label string) error { + // Fetch the labels attribute, if it exists + ruleLabels := rule.Attr("labels") + if ruleLabels == nil { + return nil + } + // Check it's a list of expressions + ruleLabelsList, ok := ruleLabels.(*build.ListExpr) + if !ok { + return errors.New("rule already has a `labels` attribute, and it is not a list") + } + // Build a new list of labels which doesn't include the specified one. + newLabels := &build.ListExpr{} + for _, labelExpr := range ruleLabelsList.List { + // Copy any non-string labels + labelStringExpr, ok := labelExpr.(*build.StringExpr) + if !ok { + newLabels.List = append(newLabels.List, labelExpr) + continue + } + // Copy any string labels which do not match + if labelStringExpr.Value != label { + newLabels.List = append(newLabels.List, labelExpr) + } + } + // If labels is empty, remove the attribute + if len(newLabels.List) == 0 { + rule.DelAttr("labels") + } else { + rule.SetAttr("labels", newLabels) + } + return nil +} diff --git a/sync/integration/syncmod/sync_mod_test.go b/sync/integration/syncmod/sync_mod_test.go index 23a5287..f8e9495 100644 --- a/sync/integration/syncmod/sync_mod_test.go +++ b/sync/integration/syncmod/sync_mod_test.go @@ -42,7 +42,7 @@ func TestModSync(t *testing.T) { // Read version info from the go.mod file expectedVers := readModFileVersions() - // We expect to generate the following for the replace in the go.mod: + // We expect to generate the following for the replace directives in the go.mod: // go_mod_download( // name = "github.com_peterebden_buildtools_dl", // module = "github.com/peterebden/buildtools", @@ -52,6 +52,13 @@ func TestModSync(t *testing.T) { // go_repo( // download = ":github.com_peterebden_buildtools_dl", // module = "github.com/bazelbuild/buildtools", + // labels = ["go_replace_directive"], + // ) + // + // go_repo( + // module = "github.com/stretchr/testify", + // version = "v1.3.0", + // labels = ["go_replace_directive"], // ) for _, repoRule := range thirdPartyBuildFile.Rules("go_repo") { @@ -68,10 +75,16 @@ func TestModSync(t *testing.T) { return } - // Check that testify is labelled for a replace directive + // Check that testify is the only other one labelled for a replace directive + labels := listLabels(repoRule) if repoRule.AttrString("module") == "github.com/stretchr/testify" { - labels := listLabels(repoRule) assert.Contains(t, labels, "go_replace_directive") + } else { + assert.NotContains(t, labels, "go_replace_directive") + // Ensure there aren't empty labels list attributes + if len(labels) == 0 { + assert.Nil(t, repoRule.Attr("labels")) + } } // All rules start off at v0.0.1 and should be updated to their version listed in the go.mod @@ -90,6 +103,9 @@ func TestModSync(t *testing.T) { func listLabels(rule *build.Rule) []string { labelsExpr := rule.Attr("labels") + if labelsExpr == nil { + return []string{} + } labels := make([]string, 0, len(labelsExpr.(*build.ListExpr).List)) for _, labelExpr := range labelsExpr.(*build.ListExpr).List { labels = append(labels, labelExpr.(*build.StringExpr).Value) diff --git a/sync/integration/syncmod/test_repo/third_party/go/BUILD_FILE b/sync/integration/syncmod/test_repo/third_party/go/BUILD_FILE index 839db2f..49fe43e 100644 --- a/sync/integration/syncmod/test_repo/third_party/go/BUILD_FILE +++ b/sync/integration/syncmod/test_repo/third_party/go/BUILD_FILE @@ -6,16 +6,22 @@ go_repo( ) go_repo( + labels = [ + "example_label", + "go_replace_directive", + ], module = "github.com/davecgh/go-spew", version = "v0.0.1", ) go_repo( + labels = ["example_label"], module = "github.com/pmezard/go-difflib", version = "v0.0.1", ) go_repo( + labels = ["go_replace_directive"], module = "github.com/stretchr/objx", version = "v0.0.1", ) diff --git a/sync/sync.go b/sync/sync.go index 159135b..68c5516 100644 --- a/sync/sync.go +++ b/sync/sync.go @@ -103,13 +103,32 @@ func (s *syncer) syncModFile(conf *config.Config, file *build.File, existingRule return err } + // Remove "go_replace_directive" label from any rules which lack a replace directive + for modPath, rule := range existingRules { + // Find any matching replace directive + var matchingReplace *modfile.Replace + for _, replace := range f.Replace { + if replace.Old.Path == modPath { + matchingReplace = replace + } + } + + // Remove the replace label if not needed + if matchingReplace == nil { + err := edit.RemoveLabel(rule, ReplaceLabel) + if err != nil { + log.Warningf("Failed to remove replace label from %v: %v", modPath, err) + } + } + } + + // Check all modules listed in go.mod for _, req := range f.Require { - reqVersion := req.Mod.Version + // Find any matching replace directive var matchingReplace *modfile.Replace - for _, r := range f.Replace { - if r.Old.Path == req.Mod.Path { - matchingReplace = r - reqVersion = r.New.Version + for _, replace := range f.Replace { + if replace.Old.Path == req.Mod.Path { + matchingReplace = replace } } @@ -121,36 +140,58 @@ func (s *syncer) syncModFile(conf *config.Config, file *build.File, existingRule // delete the old go_repo rule and regenerate it with a go_mod_download and a go_repo. edit.RemoveTarget(file, rule) } else { - // Make sure the version is up-to-date - rule.SetAttr("version", edit.NewStringExpr(reqVersion)) - // Add label for the replace directive - if matchingReplace != nil { - err := edit.AddLabel(rule, ReplaceLabel) - if err != nil { - log.Warningf("Failed to add replace label to %v: %v", req.Mod.Path, err) - } - } + s.syncExistingRule(rule, req, matchingReplace) // No other changes needed continue } } - ls, err := s.licences.Get(req.Mod.Path, req.Mod.Version) - if err != nil { - return fmt.Errorf("failed to get licences for %v: %v", req.Mod.Path, err) + // Add a new rule to the build file if one does not exist + if err = s.addNewRule(file, req, matchingReplace); err != nil { + return fmt.Errorf("failed to add new rule %v: %v", req.Mod.Path, err) } + } + + return nil +} - // If no replace directive, or replace directive is just replacing the version, add a simple rule - if matchingReplace == nil || matchingReplace.New.Path == req.Mod.Path { - file.Stmt = append(file.Stmt, edit.NewGoRepoRule(req.Mod.Path, reqVersion, "", ls, []string{ReplaceLabel})) - continue +func (s *syncer) syncExistingRule(rule *build.Rule, requireDirective *modfile.Require, replaceDirective *modfile.Replace) { + reqVersion := requireDirective.Mod.Version + // Add label for the replace directive + if replaceDirective != nil { + err := edit.AddLabel(rule, ReplaceLabel) + if err != nil { + log.Warningf("Failed to add replace label to %v: %v", requireDirective.Mod.Path, err) } + // Update the requested version + reqVersion = replaceDirective.New.Version + } + // Make sure the version is up-to-date + rule.SetAttr("version", edit.NewStringExpr(reqVersion)) +} + +func (s *syncer) addNewRule(file *build.File, requireDirective *modfile.Require, replaceDirective *modfile.Replace) error { + // List licences + ls, err := s.licences.Get(requireDirective.Mod.Path, requireDirective.Mod.Version) + if err != nil { + return fmt.Errorf("failed to get licences for %v: %v", requireDirective.Mod.Path, err) + } + + // If no replace directive, add a simple rule + if replaceDirective == nil { + file.Stmt = append(file.Stmt, edit.NewGoRepoRule(requireDirective.Mod.Path, requireDirective.Mod.Version, "", ls, []string{})) + return nil + } - dl, dlName := edit.NewModDownloadRule(matchingReplace.New.Path, matchingReplace.New.Version, ls) - file.Stmt = append(file.Stmt, dl) - file.Stmt = append(file.Stmt, edit.NewGoRepoRule(req.Mod.Path, "", dlName, nil, []string{ReplaceLabel})) + // If replace directive is just replacing the version, add a simple rule + if replaceDirective.New.Path == requireDirective.Mod.Path { + file.Stmt = append(file.Stmt, edit.NewGoRepoRule(requireDirective.Mod.Path, replaceDirective.New.Version, "", ls, []string{ReplaceLabel})) + return nil } + dl, dlName := edit.NewModDownloadRule(replaceDirective.New.Path, replaceDirective.New.Version, ls) + file.Stmt = append(file.Stmt, dl) + file.Stmt = append(file.Stmt, edit.NewGoRepoRule(requireDirective.Mod.Path, "", dlName, nil, []string{ReplaceLabel})) return nil }