From 9607a9f248c2aaf207c5175417004e159cb9176f Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Wed, 20 Jun 2018 11:16:37 -0700 Subject: [PATCH] Allow multiple dependencies on the same module Allow adding multiple dependencies on the same module. Adds a flag to Context.walkDeps to determine whether dependencies should be visited multiple times, and clarifies the behavior of VisitDepsDepthFirst (continues to visit each module once, even if there are multiple dependencies on it) and VisitDirectDeps (may visit a module multiple times). Test: TestWalkDepsDuplicates, TestVisit Change-Id: I58afbe90490aca102d242d63e185386e1fe55d73 --- context.go | 14 +++----- context_test.go | 95 +++++++++++++++++++++++++++++++++++++++++++++++-- module_ctx.go | 32 ++++++++++++++--- visit_test.go | 18 ++++++++-- 4 files changed, 139 insertions(+), 20 deletions(-) diff --git a/context.go b/context.go index f96306b..11ee6f0 100644 --- a/context.go +++ b/context.go @@ -1401,12 +1401,6 @@ func (c *Context) addDependency(module *moduleInfo, tag DependencyTag, depName s } if m := c.findMatchingVariant(module, possibleDeps); m != nil { - for _, dep := range module.directDeps { - if m == dep.module { - // TODO(ccross): what if adding a dependency with a different tag? - return nil - } - } module.directDeps = append(module.directDeps, depInfo{m, tag}) atomic.AddUint32(&c.depsModified, 1) return nil @@ -2386,7 +2380,7 @@ func (c *Context) processLocalBuildActions(out, in *localBuildActions, return nil } -func (c *Context) walkDeps(topModule *moduleInfo, +func (c *Context) walkDeps(topModule *moduleInfo, allowDuplicates bool, visitDown func(depInfo, *moduleInfo) bool, visitUp func(depInfo, *moduleInfo)) { visited := make(map[*moduleInfo]bool) @@ -2402,7 +2396,7 @@ func (c *Context) walkDeps(topModule *moduleInfo, var walk func(module *moduleInfo) walk = func(module *moduleInfo) { for _, dep := range module.directDeps { - if !visited[dep.module] { + if allowDuplicates || !visited[dep.module] { visited[dep.module] = true visiting = dep.module recurse := true @@ -2884,7 +2878,7 @@ func (c *Context) VisitDepsDepthFirst(module Module, visit func(Module)) { } }() - c.walkDeps(topModule, nil, func(dep depInfo, parent *moduleInfo) { + c.walkDeps(topModule, false, nil, func(dep depInfo, parent *moduleInfo) { visiting = dep.module visit(dep.module.logicModule) }) @@ -2902,7 +2896,7 @@ func (c *Context) VisitDepsDepthFirstIf(module Module, pred func(Module) bool, v } }() - c.walkDeps(topModule, nil, func(dep depInfo, parent *moduleInfo) { + c.walkDeps(topModule, false, nil, func(dep depInfo, parent *moduleInfo) { if pred(dep.module.logicModule) { visiting = dep.module visit(dep.module.logicModule) diff --git a/context_test.go b/context_test.go index 635f73e..c08b0b7 100644 --- a/context_test.go +++ b/context_test.go @@ -189,7 +189,7 @@ func TestWalkDeps(t *testing.T) { var outputDown string var outputUp string topModule := ctx.modulesFromName("A", nil)[0] - ctx.walkDeps(topModule, + ctx.walkDeps(topModule, false, func(dep depInfo, parent *moduleInfo) bool { if dep.module.logicModule.(Walker).Walk() { outputDown += ctx.ModuleName(dep.module.logicModule) @@ -203,10 +203,99 @@ func TestWalkDeps(t *testing.T) { } }) if outputDown != "CFG" { - t.Fatalf("unexpected walkDeps behaviour: %s\ndown should be: CFG", outputDown) + t.Errorf("unexpected walkDeps behaviour: %s\ndown should be: CFG", outputDown) } if outputUp != "GFC" { - t.Fatalf("unexpected walkDeps behaviour: %s\nup should be: GFC", outputUp) + t.Errorf("unexpected walkDeps behaviour: %s\nup should be: GFC", outputUp) + } +} + +// |---B===D - represents a non-walkable edge +// A = represents a walkable edge +// |===C===E===\ +// | | A should not be visited because it's the root node. +// |===F===G B, D should not be walked. +// \===/ G should be visited multiple times +func TestWalkDepsDuplicates(t *testing.T) { + ctx := NewContext() + ctx.MockFileSystem(map[string][]byte{ + "Blueprints": []byte(` + foo_module { + name: "A", + deps: ["B", "C"], + } + + bar_module { + name: "B", + deps: ["D"], + } + + foo_module { + name: "C", + deps: ["E", "F"], + } + + foo_module { + name: "D", + } + + foo_module { + name: "E", + deps: ["G"], + } + + foo_module { + name: "F", + deps: ["G", "G"], + } + + foo_module { + name: "G", + } + `), + }) + + ctx.RegisterModuleType("foo_module", newFooModule) + ctx.RegisterModuleType("bar_module", newBarModule) + _, errs := ctx.ParseBlueprintsFiles("Blueprints") + if len(errs) > 0 { + t.Errorf("unexpected parse errors:") + for _, err := range errs { + t.Errorf(" %s", err) + } + t.FailNow() + } + + _, errs = ctx.ResolveDependencies(nil) + if len(errs) > 0 { + t.Errorf("unexpected dep errors:") + for _, err := range errs { + t.Errorf(" %s", err) + } + t.FailNow() + } + + var outputDown string + var outputUp string + topModule := ctx.modulesFromName("A", nil)[0] + ctx.walkDeps(topModule, true, + func(dep depInfo, parent *moduleInfo) bool { + if dep.module.logicModule.(Walker).Walk() { + outputDown += ctx.ModuleName(dep.module.logicModule) + return true + } + return false + }, + func(dep depInfo, parent *moduleInfo) { + if dep.module.logicModule.(Walker).Walk() { + outputUp += ctx.ModuleName(dep.module.logicModule) + } + }) + if outputDown != "CEGFGG" { + t.Errorf("unexpected walkDeps behaviour: %s\ndown should be: CEGFGG", outputDown) + } + if outputUp != "GEGGFC" { + t.Errorf("unexpected walkDeps behaviour: %s\nup should be: GEGGFC", outputUp) } } diff --git a/module_ctx.go b/module_ctx.go index fe6c12b..200fd9a 100644 --- a/module_ctx.go +++ b/module_ctx.go @@ -304,6 +304,9 @@ func (m *baseModuleContext) OtherModuleErrorf(logicModule Module, format string, }) } +// OtherModuleDependencyTag returns the dependency tag used to depend on a module, or nil if there is no dependency +// on the module. When called inside a Visit* method with current module being visited, and there are multiple +// dependencies on the module being visited, it returns the dependency tag used for the current dependency. func (m *baseModuleContext) OtherModuleDependencyTag(logicModule Module) DependencyTag { // fast path for calling OtherModuleDependencyTag from inside VisitDirectDeps if logicModule == m.visitingDep.module.logicModule { @@ -319,8 +322,9 @@ func (m *baseModuleContext) OtherModuleDependencyTag(logicModule Module) Depende return nil } -// GetDirectDep returns the Module and DependencyTag for the direct dependency with the specified -// name, or nil if none exists. +// GetDirectDep returns the Module and DependencyTag for the direct dependency with the specified +// name, or nil if none exists. If there are multiple dependencies on the same module it returns +// the first DependencyTag. func (m *baseModuleContext) GetDirectDep(name string) (Module, DependencyTag) { for _, dep := range m.module.directDeps { if dep.module.Name() == name { @@ -347,6 +351,8 @@ func (m *baseModuleContext) GetDirectDepWithTag(name string, tag DependencyTag) return nil } +// VisitDirectDeps calls visit for each direct dependency. If there are multiple direct dependencies on the same module +// visit will be called multiple times on that module and OtherModuleDependencyTag will return a different tag for each. func (m *baseModuleContext) VisitDirectDeps(visit func(Module)) { defer func() { if r := recover(); r != nil { @@ -366,6 +372,9 @@ func (m *baseModuleContext) VisitDirectDeps(visit func(Module)) { m.visitingDep = depInfo{} } +// VisitDirectDepsIf calls pred for each direct dependency, and if pred returns true calls visit. If there are multiple +// direct dependencies on the same module pred and visit will be called multiple times on that module and +// OtherModuleDependencyTag will return a different tag for each. func (m *baseModuleContext) VisitDirectDepsIf(pred func(Module) bool, visit func(Module)) { defer func() { if r := recover(); r != nil { @@ -387,6 +396,10 @@ func (m *baseModuleContext) VisitDirectDepsIf(pred func(Module) bool, visit func m.visitingDep = depInfo{} } +// VisitDepsDepthFirst calls visit for each transitive dependency, traversing the dependency tree in depth first order. +// visit will only be called once for any given module, even if there are multiple paths through the dependency tree +// to the module or multiple direct dependencies with different tags. OtherModuleDependencyTag will return the tag for +// the first path found to the module. func (m *baseModuleContext) VisitDepsDepthFirst(visit func(Module)) { defer func() { if r := recover(); r != nil { @@ -395,7 +408,7 @@ func (m *baseModuleContext) VisitDepsDepthFirst(visit func(Module)) { } }() - m.context.walkDeps(m.module, nil, func(dep depInfo, parent *moduleInfo) { + m.context.walkDeps(m.module, false, nil, func(dep depInfo, parent *moduleInfo) { m.visitingParent = parent m.visitingDep = dep visit(dep.module.logicModule) @@ -405,6 +418,11 @@ func (m *baseModuleContext) VisitDepsDepthFirst(visit func(Module)) { m.visitingDep = depInfo{} } +// VisitDepsDepthFirst calls pred for each transitive dependency, and if pred returns true calls visit, traversing the +// dependency tree in depth first order. visit will only be called once for any given module, even if there are +// multiple paths through the dependency tree to the module or multiple direct dependencies with different tags. +// OtherModuleDependencyTag will return the tag for the first path found to the module. The return value of pred does +// not affect which branches of the tree are traversed. func (m *baseModuleContext) VisitDepsDepthFirstIf(pred func(Module) bool, visit func(Module)) { @@ -415,7 +433,7 @@ func (m *baseModuleContext) VisitDepsDepthFirstIf(pred func(Module) bool, } }() - m.context.walkDeps(m.module, nil, func(dep depInfo, parent *moduleInfo) { + m.context.walkDeps(m.module, false, nil, func(dep depInfo, parent *moduleInfo) { if pred(dep.module.logicModule) { m.visitingParent = parent m.visitingDep = dep @@ -427,8 +445,12 @@ func (m *baseModuleContext) VisitDepsDepthFirstIf(pred func(Module) bool, m.visitingDep = depInfo{} } +// WalkDeps calls visit for each transitive dependency, traversing the dependency tree in top down order. visit may be +// called multiple times for the same module if there are multiple paths through the dependency tree to the module or +// multiple direct dependencies with different tags. OtherModuleDependencyTag will return the tag for the currently +// visited path to the module. If visit returns false WalkDeps will not continue recursing down the current path. func (m *baseModuleContext) WalkDeps(visit func(Module, Module) bool) { - m.context.walkDeps(m.module, func(dep depInfo, parent *moduleInfo) bool { + m.context.walkDeps(m.module, true, func(dep depInfo, parent *moduleInfo) bool { m.visitingParent = parent m.visitingDep = dep return visit(dep.module.logicModule, parent.logicModule) diff --git a/visit_test.go b/visit_test.go index 3aa0f1b..873e72c 100644 --- a/visit_test.go +++ b/visit_test.go @@ -83,6 +83,9 @@ func visitMutator(ctx TopDownMutatorContext) { // D // | // E +// / \ +// \ / +// F func setupVisitTest(t *testing.T) *Context { ctx := NewContext() ctx.RegisterModuleType("visit_module", newVisitModule) @@ -113,6 +116,11 @@ func setupVisitTest(t *testing.T) *Context { visit_module { name: "E", + visit: ["F", "F"], + } + + visit_module { + name: "F", } `), }) @@ -142,10 +150,16 @@ func TestVisit(t *testing.T) { ctx := setupVisitTest(t) topModule := ctx.modulesFromName("A", nil)[0].logicModule.(*visitModule) - assertString(t, topModule.properties.VisitDepsDepthFirst, "EDCB") - assertString(t, topModule.properties.VisitDepsDepthFirstIf, "EDC") + assertString(t, topModule.properties.VisitDepsDepthFirst, "FEDCB") + assertString(t, topModule.properties.VisitDepsDepthFirstIf, "FEDC") assertString(t, topModule.properties.VisitDirectDeps, "B") assertString(t, topModule.properties.VisitDirectDepsIf, "") + + eModule := ctx.modulesFromName("E", nil)[0].logicModule.(*visitModule) + assertString(t, eModule.properties.VisitDepsDepthFirst, "F") + assertString(t, eModule.properties.VisitDepsDepthFirstIf, "F") + assertString(t, eModule.properties.VisitDirectDeps, "FF") + assertString(t, eModule.properties.VisitDirectDepsIf, "FF") } func assertString(t *testing.T, got, expected string) {