[autoroll] Fix broken transitive deps
The dependency versions stored in the Child Revisions are keyed by the
Child's name for the dependencies, which may be different from the names
used by the Parent. Therefore, we need to have access to both when we're
updating dependencies of the Parent using Revisions of the Child.
Change-Id: Ie13b35399967a2e397ab3c379679754bbf4b45dd
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/290027
Reviewed-by: Ravi Mistry <rmistry@google.com>
Commit-Queue: Eric Boren <borenet@google.com>
diff --git a/autoroll/go/repo_manager/common/version_file_common/version_file_common.go b/autoroll/go/repo_manager/common/version_file_common/version_file_common.go
index 6cb400a..c8a4a41 100644
--- a/autoroll/go/repo_manager/common/version_file_common/version_file_common.go
+++ b/autoroll/go/repo_manager/common/version_file_common/version_file_common.go
@@ -31,13 +31,37 @@
return nil
}
+// TransitiveDepConfig provides configuration for a single transitive
+// dependency.
+type TransitiveDepConfig struct {
+ Child *VersionFileConfig `json:"child"`
+ Parent *VersionFileConfig `json:"parent"`
+}
+
+// See documentation for util.Validator interface.
+func (c *TransitiveDepConfig) Validate() error {
+ if c.Child == nil {
+ return skerr.Fmt("Child is required")
+ }
+ if err := c.Child.Validate(); err != nil {
+ return skerr.Wrap(err)
+ }
+ if c.Parent == nil {
+ return skerr.Fmt("Parent is required")
+ }
+ if err := c.Parent.Validate(); err != nil {
+ return skerr.Wrap(err)
+ }
+ return nil
+}
+
// DependencyConfig provides configuration for a dependency whose version is
// pinned in a file and which may have transitive dependencies.
type DependencyConfig struct {
// Primary dependency.
VersionFileConfig
// Transitive dependencies.
- TransitiveDeps []*VersionFileConfig
+ TransitiveDeps []*TransitiveDepConfig
}
// See documentation for util.Validator interface.
@@ -161,18 +185,18 @@
if len(primaryDep.TransitiveDeps) > 0 {
for _, dep := range primaryDep.TransitiveDeps {
// Find the new revision.
- newVersion, ok := rev.Dependencies[dep.ID]
+ newVersion, ok := rev.Dependencies[dep.Child.ID]
if !ok {
- return nil, nil, skerr.Fmt("Could not find transitive dependency %q in %#v", dep.ID, rev)
+ return nil, nil, skerr.Fmt("Could not find transitive dependency %q in %#v", dep.Child.ID, rev)
}
// Update.
- oldVersion, err := updateSingleDep(ctx, *dep, newVersion, changes, getFile)
+ oldVersion, err := updateSingleDep(ctx, *dep.Parent, newVersion, changes, getFile)
if err != nil {
return nil, nil, skerr.Wrap(err)
}
// Add the transitive dep to the list.
td = append(td, &TransitiveDepUpdate{
- Dep: dep.ID,
+ Dep: dep.Parent.ID,
RollingFrom: oldVersion,
RollingTo: newVersion,
})
diff --git a/autoroll/go/repo_manager/fuchsia_sdk_repo_manager.go b/autoroll/go/repo_manager/fuchsia_sdk_repo_manager.go
index 61a1eee..966c011 100644
--- a/autoroll/go/repo_manager/fuchsia_sdk_repo_manager.go
+++ b/autoroll/go/repo_manager/fuchsia_sdk_repo_manager.go
@@ -57,12 +57,18 @@
// splitParentChild breaks the FuchsiaSDKRepoManagerConfig into parent and child
// configs.
func (c *FuchsiaSDKRepoManagerConfig) splitParentChild() (parent.GitilesConfig, child.FuchsiaSDKConfig, error) {
- var parentDeps []*version_file_common.VersionFileConfig
+ var transitiveDeps []*version_file_common.TransitiveDepConfig
if c.IncludeMacSDK {
- parentDeps = []*version_file_common.VersionFileConfig{
+ transitiveDeps = []*version_file_common.TransitiveDepConfig{
{
- ID: child.FuchsiaSDKGSLatestPathMac,
- Path: FuchsiaSDKVersionFilePathMac,
+ Child: &version_file_common.VersionFileConfig{
+ ID: child.FuchsiaSDKGSLatestPathMac,
+ Path: FuchsiaSDKVersionFilePathMac,
+ },
+ Parent: &version_file_common.VersionFileConfig{
+ ID: child.FuchsiaSDKGSLatestPathMac,
+ Path: FuchsiaSDKVersionFilePathMac,
+ },
},
}
}
@@ -84,7 +90,7 @@
ID: "FuchsiaSDK",
Path: FuchsiaSDKVersionFilePathLinux,
},
- TransitiveDeps: parentDeps,
+ TransitiveDeps: transitiveDeps,
},
GitilesConfig: gitiles_common.GitilesConfig{
Branch: c.NoCheckoutRepoManagerConfig.CommonRepoManagerConfig.ParentBranch,
diff --git a/autoroll/go/repo_manager/github_deps_repo_manager.go b/autoroll/go/repo_manager/github_deps_repo_manager.go
index e8ab66e..d3ba9fc 100644
--- a/autoroll/go/repo_manager/github_deps_repo_manager.go
+++ b/autoroll/go/repo_manager/github_deps_repo_manager.go
@@ -26,13 +26,10 @@
// workdir + parent repo.
GithubParentPath string `json:"githubParentPath,omitempty"`
- // Optional; transitive dependencies to roll. This is a mapping of
- // dependencies of the child repo which are also dependencies of the
- // parent repo and should be rolled at the same time. Keys are paths
- // to transitive dependencies within the child repo (as specified in
- // DEPS), and values are paths to those dependencies within the parent
- // repo.
- TransitiveDeps map[string]string `json:"transitiveDeps"`
+ // TransitiveDeps is an optional set of dependencies shared by the Parent
+ // and Child which are updated in the Parent to match the versions of the
+ // Child.
+ TransitiveDeps []*version_file_common.TransitiveDepConfig `json:"transitiveDeps"`
}
// Validate the config.
@@ -52,6 +49,13 @@
// TODO(borenet): Update the config format to directly define the parent
// and child. We shouldn't need most of the New.*RepoManager functions.
func (c GithubDEPSRepoManagerConfig) splitParentChild() (parent.DEPSLocalConfig, child.GitCheckoutConfig, error) {
+ var childDeps []*version_file_common.VersionFileConfig
+ if c.TransitiveDeps != nil {
+ childDeps = make([]*version_file_common.VersionFileConfig, 0, len(c.TransitiveDeps))
+ for _, dep := range c.TransitiveDeps {
+ childDeps = append(childDeps, dep.Child)
+ }
+ }
parentCfg := parent.DEPSLocalConfig{
GitCheckoutConfig: parent.GitCheckoutConfig{
BaseConfig: parent.BaseConfig{
@@ -71,6 +75,7 @@
ID: c.ChildRepo,
Path: deps_parser.DepsFileName,
},
+ TransitiveDeps: c.TransitiveDeps,
},
},
CheckoutPath: c.GithubParentPath,
@@ -83,9 +88,10 @@
}
childCfg := child.GitCheckoutConfig{
GitCheckoutConfig: git_common.GitCheckoutConfig{
- Branch: c.ChildBranch,
- RepoURL: c.ChildRepo,
- RevLinkTmpl: c.DepotToolsRepoManagerConfig.CommonRepoManagerConfig.ChildRevLinkTmpl,
+ Branch: c.ChildBranch,
+ RepoURL: c.ChildRepo,
+ RevLinkTmpl: c.DepotToolsRepoManagerConfig.CommonRepoManagerConfig.ChildRevLinkTmpl,
+ Dependencies: childDeps,
},
}
if err := childCfg.Validate(); err != nil {
diff --git a/autoroll/go/repo_manager/github_deps_repo_manager_test.go b/autoroll/go/repo_manager/github_deps_repo_manager_test.go
index 5ff95f8..ab1e5ab 100644
--- a/autoroll/go/repo_manager/github_deps_repo_manager_test.go
+++ b/autoroll/go/repo_manager/github_deps_repo_manager_test.go
@@ -13,6 +13,7 @@
github_api "github.com/google/go-github/v29/github"
"github.com/stretchr/testify/require"
+ "go.skia.org/infra/autoroll/go/repo_manager/common/version_file_common"
"go.skia.org/infra/autoroll/go/repo_manager/parent"
"go.skia.org/infra/go/exec"
git_testutils "go.skia.org/infra/go/git/testutils"
@@ -68,16 +69,15 @@
ctx := context.Background()
// Create child and parent repos.
- grandchild := git_testutils.GitInit(t, ctx)
- f := "somefile.txt"
- grandchildA := grandchild.CommitGen(ctx, f)
- grandchildB := grandchild.CommitGen(ctx, f)
-
child := git_testutils.GitInit(t, ctx)
- child.Add(ctx, "DEPS", fmt.Sprintf(`deps = {
- "child/dep": "%s@%s"
-}`, grandchild.RepoUrl(), grandchildB))
+ child.Add(ctx, "DEPS", `deps = {
+ "child/dep": {
+ "url": "https://grandchild-in-child@def4560000def4560000def4560000def4560000",
+ "condition": "False",
+ },
+}`)
child.Commit(ctx)
+ f := "somefile.txt"
childCommits := make([]string, 0, 10)
for i := 0; i < numChildCommits; i++ {
childCommits = append(childCommits, child.CommitGen(ctx, f))
@@ -86,8 +86,11 @@
parent := git_testutils.GitInit(t, ctx)
parent.Add(ctx, "DEPS", fmt.Sprintf(`deps = {
"%s": "%s@%s",
- "parent/dep": "%s@%s",
-}`, childPath, child.RepoUrl(), childCommits[0], grandchild.RepoUrl(), grandchildA))
+ "parent/dep": {
+ "url": "https://grandchild-in-parent@abc1230000abc1230000abc1230000abc1230000",
+ "condition": "False",
+ },
+}`, childPath, child.RepoUrl(), childCommits[0]))
parent.Commit(ctx)
fork := git_testutils.GitInit(t, ctx)
@@ -215,8 +218,17 @@
unittest.LargeTest(t)
cfg := githubDEPSCfg(t)
- cfg.TransitiveDeps = map[string]string{
- "child/dep": "parent/dep",
+ cfg.TransitiveDeps = []*version_file_common.TransitiveDepConfig{
+ {
+ Child: &version_file_common.VersionFileConfig{
+ ID: "https://grandchild-in-child",
+ Path: "DEPS",
+ },
+ Parent: &version_file_common.VersionFileConfig{
+ ID: "https://grandchild-in-parent",
+ Path: "DEPS",
+ },
+ },
}
ctx, rm, _, _, _, _, _, urlMock, cleanup := setupGithubDEPS(t, cfg)
defer cleanup()
diff --git a/autoroll/go/repo_manager/github_repo_manager.go b/autoroll/go/repo_manager/github_repo_manager.go
index 9fd42f0..6a938d7 100644
--- a/autoroll/go/repo_manager/github_repo_manager.go
+++ b/autoroll/go/repo_manager/github_repo_manager.go
@@ -33,7 +33,7 @@
// TransitiveDeps is an optional mapping of dependency ID (eg. repo URL)
// to the paths within the parent and child repo, respectively, where
// those dependencies are versioned, eg. "DEPS".
- TransitiveDeps []*TransitiveDepConfig `json:"transitiveDeps"`
+ TransitiveDeps []*version_file_common.TransitiveDepConfig `json:"transitiveDeps"`
}
// See documentation for util.Validator interface.
@@ -47,12 +47,10 @@
// TODO(borenet): Update the config format to directly define the parent
// and child. We shouldn't need most of the New.*RepoManager functions.
func (c GithubRepoManagerConfig) splitParentChild() (parent.GitCheckoutGithubFileConfig, child.GitCheckoutGithubConfig, error) {
- var childDeps, parentDeps []*version_file_common.VersionFileConfig
+ var childDeps []*version_file_common.VersionFileConfig
if c.TransitiveDeps != nil {
childDeps = make([]*version_file_common.VersionFileConfig, 0, len(c.TransitiveDeps))
- parentDeps = make([]*version_file_common.VersionFileConfig, 0, len(c.TransitiveDeps))
for _, dep := range c.TransitiveDeps {
- parentDeps = append(parentDeps, dep.Parent)
childDeps = append(childDeps, dep.Child)
}
}
@@ -77,7 +75,7 @@
ID: c.ChildRepoURL,
Path: c.RevisionFile,
},
- TransitiveDeps: parentDeps,
+ TransitiveDeps: c.TransitiveDeps,
},
},
ForkRepoURL: c.ForkRepoURL,
diff --git a/autoroll/go/repo_manager/no_checkout_deps_repo_manager.go b/autoroll/go/repo_manager/no_checkout_deps_repo_manager.go
index 56c31a3..9fb7e67 100644
--- a/autoroll/go/repo_manager/no_checkout_deps_repo_manager.go
+++ b/autoroll/go/repo_manager/no_checkout_deps_repo_manager.go
@@ -21,13 +21,6 @@
getDepRegex = regexp.MustCompile("[a-f0-9]+")
)
-// TransitiveDepConfig provides configuration for a single transitive
-// dependency.
-type TransitiveDepConfig struct {
- Child *version_file_common.VersionFileConfig `json:"child"`
- Parent *version_file_common.VersionFileConfig `json:"parent"`
-}
-
// NoCheckoutDEPSRepoManagerConfig provides configuration for RepoManagers which
// don't use a local checkout.
type NoCheckoutDEPSRepoManagerConfig struct {
@@ -36,10 +29,10 @@
// URL of the child repo.
ChildRepo string `json:"childRepo"` // TODO(borenet): Can we just get this from DEPS?
- // TransitiveDeps is an optional mapping of dependency ID (eg. repo URL)
- // to the paths within the parent and child repo, respectively, where
- // those dependencies are versioned, eg. "DEPS".
- TransitiveDeps []*TransitiveDepConfig `json:"transitiveDeps"`
+ // TransitiveDeps is an optional set of dependencies shared by the Parent
+ // and Child which are updated in the Parent to match the versions of the
+ // Child.
+ TransitiveDeps []*version_file_common.TransitiveDepConfig `json:"transitiveDeps"`
}
// See documentation for util.Validator interface.
@@ -81,12 +74,10 @@
// TODO(borenet): Update the config format to directly define the parent
// and child. We shouldn't need most of the New.*RepoManager functions.
func (c NoCheckoutDEPSRepoManagerConfig) splitParentChild() (parent.GitilesConfig, child.GitilesConfig, error) {
- var childDeps, parentDeps []*version_file_common.VersionFileConfig
+ var childDeps []*version_file_common.VersionFileConfig
if c.TransitiveDeps != nil {
childDeps = make([]*version_file_common.VersionFileConfig, 0, len(c.TransitiveDeps))
- parentDeps = make([]*version_file_common.VersionFileConfig, 0, len(c.TransitiveDeps))
for _, dep := range c.TransitiveDeps {
- parentDeps = append(parentDeps, dep.Parent)
childDeps = append(childDeps, dep.Child)
}
}
@@ -104,7 +95,7 @@
ID: c.ChildRepo,
Path: deps_parser.DepsFileName,
},
- TransitiveDeps: parentDeps,
+ TransitiveDeps: c.TransitiveDeps,
},
GitilesConfig: gitiles_common.GitilesConfig{
Branch: c.NoCheckoutRepoManagerConfig.CommonRepoManagerConfig.ParentBranch,
diff --git a/autoroll/go/repo_manager/no_checkout_deps_repo_manager_test.go b/autoroll/go/repo_manager/no_checkout_deps_repo_manager_test.go
index 291c41a..9f2834b 100644
--- a/autoroll/go/repo_manager/no_checkout_deps_repo_manager_test.go
+++ b/autoroll/go/repo_manager/no_checkout_deps_repo_manager_test.go
@@ -31,7 +31,7 @@
// Create child and parent repos.
child := git_testutils.GitInit(t, context.Background())
child.Add(context.Background(), "DEPS", `deps = {
- "child/dep": "grandchild@def4560000def4560000def4560000def4560000",
+ "child/dep": "https://grandchild-in-child@def4560000def4560000def4560000def4560000",
}`)
child.Commit(context.Background())
f := "somefile.txt"
@@ -47,7 +47,7 @@
parent := git_testutils.GitInit(t, context.Background())
parent.Add(context.Background(), "DEPS", fmt.Sprintf(`deps = {
"%s": "%s@%s",
- "parent/dep": "grandchild@abc1230000abc1230000abc1230000abc1230000",
+ "parent/dep": "https://grandchild-in-parent@abc1230000abc1230000abc1230000abc1230000",
}`, childPath, child.RepoUrl(), childCommits[0]))
parent.Commit(context.Background())
@@ -238,7 +238,7 @@
// Mock the request to modify the DEPS file.
reqBody = []byte(fmt.Sprintf(`deps = {
"%s": "%s@%s",
- "parent/dep": "grandchild@abc1230000abc1230000abc1230000abc1230000",
+ "parent/dep": "https://grandchild-in-parent@abc1230000abc1230000abc1230000abc1230000",
}`, childPath, childRepo.RepoUrl(), tipRev.Id))
urlmock.MockOnce("https://fake-skia-review.googlesource.com/a/changes/123/edit/DEPS", mockhttpclient.MockPutDialogue("", reqBody, []byte("")))
@@ -287,14 +287,14 @@
func TestNoCheckoutDEPSRepoManagerCreateNewRollTransitive(t *testing.T) {
cfg := noCheckoutDEPSCfg(t)
- cfg.TransitiveDeps = []*TransitiveDepConfig{
+ cfg.TransitiveDeps = []*version_file_common.TransitiveDepConfig{
{
Child: &version_file_common.VersionFileConfig{
- ID: "grandchild",
+ ID: "https://grandchild-in-child",
Path: "DEPS",
},
Parent: &version_file_common.VersionFileConfig{
- ID: "grandchild",
+ ID: "https://grandchild-in-parent",
Path: "DEPS",
},
},
@@ -345,7 +345,7 @@
git log %s..%s --date=short --first-parent --format='%%ad %%ae %%s'
%s
Also rolling transitive DEPS:
- grandchild abc1230000ab..def4560000de
+ https://grandchild-in-parent abc1230000ab..def4560000de
Created with:
gclient setdep -r %s@%s
@@ -390,7 +390,7 @@
// Mock the request to modify the DEPS file.
reqBody = []byte(fmt.Sprintf(`deps = {
"%s": "%s@%s",
- "parent/dep": "grandchild@def4560000def4560000def4560000def4560000",
+ "parent/dep": "https://grandchild-in-parent@def4560000def4560000def4560000def4560000",
}`, childPath, childRepo.RepoUrl(), tipRev.Id))
urlmock.MockOnce("https://fake-skia-review.googlesource.com/a/changes/123/edit/DEPS", mockhttpclient.MockPutDialogue("", reqBody, []byte("")))
diff --git a/autoroll/go/repo_manager/parent/git_checkout.go b/autoroll/go/repo_manager/parent/git_checkout.go
index 8b151cc..e3f7f09 100644
--- a/autoroll/go/repo_manager/parent/git_checkout.go
+++ b/autoroll/go/repo_manager/parent/git_checkout.go
@@ -83,7 +83,9 @@
// Create the local checkout.
deps := make([]*version_file_common.VersionFileConfig, 0, len(c.DependencyConfig.TransitiveDeps)+1)
deps = append(deps, &c.DependencyConfig.VersionFileConfig)
- deps = append(deps, c.DependencyConfig.TransitiveDeps...)
+ for _, td := range c.TransitiveDeps {
+ deps = append(deps, td.Parent)
+ }
c.GitCheckoutConfig.Dependencies = deps
checkout, err := git_common.NewCheckout(ctx, c.GitCheckoutConfig, reg, workdir, userName, userEmail, co)
if err != nil {
diff --git a/autoroll/go/repo_manager/parent/gitiles.go b/autoroll/go/repo_manager/parent/gitiles.go
index 267371e..8fb21cc 100644
--- a/autoroll/go/repo_manager/parent/gitiles.go
+++ b/autoroll/go/repo_manager/parent/gitiles.go
@@ -84,7 +84,9 @@
}
deps := make([]*version_file_common.VersionFileConfig, 0, len(c.DependencyConfig.TransitiveDeps)+1)
deps = append(deps, &c.DependencyConfig.VersionFileConfig)
- deps = append(deps, c.DependencyConfig.TransitiveDeps...)
+ for _, td := range c.DependencyConfig.TransitiveDeps {
+ deps = append(deps, td.Parent)
+ }
c.GitilesConfig.Dependencies = deps
gr, err := gitiles_common.NewGitilesRepo(ctx, c.GitilesConfig, reg, client)
if err != nil {
