From 8439b0a01e5ed1ac66d44f5f84c44aa274f466bc Mon Sep 17 00:00:00 2001 From: SpangleLabs Date: Mon, 15 Jul 2024 13:00:42 +0100 Subject: [PATCH 1/8] Adding some example labels in the test data, to check that replace labels are cleaned up --- .../integration/syncmod/test_repo/third_party/go/BUILD_FILE | 6 ++++++ 1 file changed, 6 insertions(+) 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", ) From c7fd813e8754dec303c26faf09e40fc0163fb0e3 Mon Sep 17 00:00:00 2001 From: SpangleLabs Date: Mon, 15 Jul 2024 13:02:26 +0100 Subject: [PATCH 2/8] Updating tests to ensure go_replace_directive labels are removed when not needed --- sync/integration/syncmod/sync_mod_test.go | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) 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) From d010608133905258b36e1282f20eb3e552083bfc Mon Sep 17 00:00:00 2001 From: SpangleLabs Date: Mon, 15 Jul 2024 13:02:49 +0100 Subject: [PATCH 3/8] Remove go_replace_directive labels when they are not needed --- edit/edit.go | 34 ++++++++++++++++++++++++++++++++++ sync/sync.go | 6 ++++++ 2 files changed, 40 insertions(+) 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/sync.go b/sync/sync.go index 159135b..1e6ba90 100644 --- a/sync/sync.go +++ b/sync/sync.go @@ -103,6 +103,12 @@ func (s *syncer) syncModFile(conf *config.Config, file *build.File, existingRule return err } + // Remove "go_replace_directive" label from all existing rules, before re-adding it only where appropriate + for _, rule := range existingRules { + edit.RemoveLabel(rule, ReplaceLabel) + } + + // Check all modules listed in go.mod for _, req := range f.Require { reqVersion := req.Mod.Version var matchingReplace *modfile.Replace From b20d6400c7acead539ad0f3b320f501895cdbb2c Mon Sep 17 00:00:00 2001 From: SpangleLabs Date: Mon, 15 Jul 2024 13:03:24 +0100 Subject: [PATCH 4/8] Don't add go_replace_directive labels to every new rule. If it lacks a replace directive, it does not need a label! --- sync/sync.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/sync/sync.go b/sync/sync.go index 1e6ba90..b5f1012 100644 --- a/sync/sync.go +++ b/sync/sync.go @@ -148,7 +148,11 @@ func (s *syncer) syncModFile(conf *config.Config, file *build.File, existingRule // 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})) + replaceLabels := []string{} + if matchingReplace != nil { + replaceLabels = []string{ReplaceLabel} + } + file.Stmt = append(file.Stmt, edit.NewGoRepoRule(req.Mod.Path, reqVersion, "", ls, replaceLabels)) continue } From e1c8d944d6861db4034593c5a78c71aef7f52bd4 Mon Sep 17 00:00:00 2001 From: SpangleLabs Date: Mon, 15 Jul 2024 13:38:04 +0100 Subject: [PATCH 5/8] Handle potential failure of removing label --- sync/sync.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/sync/sync.go b/sync/sync.go index b5f1012..785010c 100644 --- a/sync/sync.go +++ b/sync/sync.go @@ -104,8 +104,11 @@ func (s *syncer) syncModFile(conf *config.Config, file *build.File, existingRule } // Remove "go_replace_directive" label from all existing rules, before re-adding it only where appropriate - for _, rule := range existingRules { - edit.RemoveLabel(rule, ReplaceLabel) + for modPath, rule := range existingRules { + 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 From e6bc39a5781d967ebb806878cae104685a333fcb Mon Sep 17 00:00:00 2001 From: SpangleLabs Date: Mon, 15 Jul 2024 16:57:38 +0100 Subject: [PATCH 6/8] Refactoring to pull more logic out of the syncModFile() method, into individual rule methods for additional clarity --- sync/sync.go | 74 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 46 insertions(+), 28 deletions(-) diff --git a/sync/sync.go b/sync/sync.go index 785010c..e3c5e76 100644 --- a/sync/sync.go +++ b/sync/sync.go @@ -113,12 +113,12 @@ func (s *syncer) syncModFile(conf *config.Config, file *build.File, existingRule // 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 } } @@ -130,40 +130,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) } + } - // 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 { - replaceLabels := []string{} - if matchingReplace != nil { - replaceLabels = []string{ReplaceLabel} - } - file.Stmt = append(file.Stmt, edit.NewGoRepoRule(req.Mod.Path, reqVersion, "", ls, replaceLabels)) - continue + return nil +} + +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) + } - 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 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 + } + + // 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 } From d7f71075cf5ca488af8e81b99fbdc4a970a8de53 Mon Sep 17 00:00:00 2001 From: SpangleLabs Date: Mon, 15 Jul 2024 17:01:17 +0100 Subject: [PATCH 7/8] Remove empty newline which I liked, but the linter disliked --- sync/sync.go | 1 - 1 file changed, 1 deletion(-) diff --git a/sync/sync.go b/sync/sync.go index e3c5e76..1f4b21e 100644 --- a/sync/sync.go +++ b/sync/sync.go @@ -113,7 +113,6 @@ func (s *syncer) syncModFile(conf *config.Config, file *build.File, existingRule // Check all modules listed in go.mod for _, req := range f.Require { - // Find any matching replace directive var matchingReplace *modfile.Replace for _, replace := range f.Replace { From b166be08a7b9996c16f64472b2c6ea1f3becbeb9 Mon Sep 17 00:00:00 2001 From: SpangleLabs Date: Tue, 16 Jul 2024 18:19:51 +0100 Subject: [PATCH 8/8] Only remove go_replace_directive label, if the rule does not have a matching replace directive --- sync/sync.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/sync/sync.go b/sync/sync.go index 1f4b21e..68c5516 100644 --- a/sync/sync.go +++ b/sync/sync.go @@ -103,11 +103,22 @@ func (s *syncer) syncModFile(conf *config.Config, file *build.File, existingRule return err } - // Remove "go_replace_directive" label from all existing rules, before re-adding it only where appropriate + // Remove "go_replace_directive" label from any rules which lack a replace directive for modPath, rule := range existingRules { - err = edit.RemoveLabel(rule, ReplaceLabel) - if err != nil { - log.Warningf("Failed to remove replace label from %v: %v", modPath, err) + // 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) + } } }