From 72bab1707e042a067c3481cddb39285a63b7aaf9 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Thu, 2 Apr 2020 10:51:33 +0100 Subject: [PATCH] WalkDeps - only record module visited when it has been recursed into Previously, WalkDeps() would record that a module was visited after the first time it encountered the module irrespective of whether it recursed into or not. This change moves the recording so it happens only after it has been recursed into. Added TestWalkDepsDuplicates_IgnoreFirstPath to test the change. Without the change the test fails because it does not visit E. Test refactoring: * A depsMutator was added instead of relying on blueprintDepsMutator to allow different tags to be used for different dependency types. * Modified barModule and fooModule to support the new depsMutator and add support for another type of dependency that is ignored by the walking code. * Extracted walkDependencyGraph() function to reuse common code. --- context.go | 2 +- context_test.go | 164 ++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 132 insertions(+), 34 deletions(-) diff --git a/context.go b/context.go index 7e4979c..da86837 100644 --- a/context.go +++ b/context.go @@ -2619,8 +2619,8 @@ func (c *Context) walkDeps(topModule *moduleInfo, allowDuplicates bool, } if recurse && !visited[dep.module] { walk(dep.module) + visited[dep.module] = true } - visited[dep.module] = true if visitUp != nil { visitUp(dep, module) } diff --git a/context_test.go b/context_test.go index d4e9bf7..0541c06 100644 --- a/context_test.go +++ b/context_test.go @@ -31,11 +31,40 @@ type Walker interface { Walk() bool } +func walkDependencyGraph(ctx *Context, topModule *moduleInfo, allowDuplicates bool) (string, string) { + var outputDown string + var outputUp string + ctx.walkDeps(topModule, allowDuplicates, + func(dep depInfo, parent *moduleInfo) bool { + outputDown += ctx.ModuleName(dep.module.logicModule) + if tag, ok := dep.tag.(walkerDepsTag); ok { + if !tag.follow { + return false + } + } + if dep.module.logicModule.(Walker).Walk() { + return true + } + + return false + }, + func(dep depInfo, parent *moduleInfo) { + outputUp += ctx.ModuleName(dep.module.logicModule) + }) + return outputDown, outputUp +} + +type depsProvider interface { + Deps() []string + IgnoreDeps() []string +} + type fooModule struct { SimpleName properties struct { - Deps []string - Foo string + Deps []string + Ignored_deps []string + Foo string } } @@ -47,10 +76,14 @@ func newFooModule() (Module, []interface{}) { func (f *fooModule) GenerateBuildActions(ModuleContext) { } -func (f *fooModule) DynamicDependencies(ctx DynamicDependerModuleContext) []string { +func (f *fooModule) Deps() []string { return f.properties.Deps } +func (f *fooModule) IgnoreDeps() []string { + return f.properties.Ignored_deps +} + func (f *fooModule) Foo() string { return f.properties.Foo } @@ -62,8 +95,9 @@ func (f *fooModule) Walk() bool { type barModule struct { SimpleName properties struct { - Deps []string - Bar bool + Deps []string + Ignored_deps []string + Bar bool } } @@ -72,10 +106,14 @@ func newBarModule() (Module, []interface{}) { return m, []interface{}{&m.properties, &m.SimpleName.Properties} } -func (b *barModule) DynamicDependencies(ctx DynamicDependerModuleContext) []string { +func (b *barModule) Deps() []string { return b.properties.Deps } +func (b *barModule) IgnoreDeps() []string { + return b.properties.Ignored_deps +} + func (b *barModule) GenerateBuildActions(ModuleContext) { } @@ -87,6 +125,19 @@ func (b *barModule) Walk() bool { return false } +type walkerDepsTag struct { + BaseDependencyTag + // True if the dependency should be followed, false otherwise. + follow bool +} + +func depsMutator(mctx BottomUpMutatorContext) { + if m, ok := mctx.Module().(depsProvider); ok { + mctx.AddDependency(mctx.Module(), walkerDepsTag{follow: false}, m.IgnoreDeps()...) + mctx.AddDependency(mctx.Module(), walkerDepsTag{follow: true}, m.Deps()...) + } +} + func TestContextParse(t *testing.T) { ctx := NewContext() ctx.RegisterModuleType("foo_module", newFooModule) @@ -168,6 +219,7 @@ func TestWalkDeps(t *testing.T) { ctx.RegisterModuleType("foo_module", newFooModule) ctx.RegisterModuleType("bar_module", newBarModule) + ctx.RegisterBottomUpMutator("deps", depsMutator) _, errs := ctx.ParseBlueprintsFiles("Blueprints", nil) if len(errs) > 0 { t.Errorf("unexpected parse errors:") @@ -186,20 +238,8 @@ func TestWalkDeps(t *testing.T) { t.FailNow() } - var outputDown string - var outputUp string topModule := ctx.moduleGroupFromName("A", nil).modules[0] - ctx.walkDeps(topModule, false, - func(dep depInfo, parent *moduleInfo) bool { - outputDown += ctx.ModuleName(dep.module.logicModule) - if dep.module.logicModule.(Walker).Walk() { - return true - } - return false - }, - func(dep depInfo, parent *moduleInfo) { - outputUp += ctx.ModuleName(dep.module.logicModule) - }) + outputDown, outputUp := walkDependencyGraph(ctx, topModule, false) if outputDown != "BCEFG" { t.Errorf("unexpected walkDeps behaviour: %s\ndown should be: BCEFG", outputDown) } @@ -260,6 +300,7 @@ func TestWalkDepsDuplicates(t *testing.T) { ctx.RegisterModuleType("foo_module", newFooModule) ctx.RegisterModuleType("bar_module", newBarModule) + ctx.RegisterBottomUpMutator("deps", depsMutator) _, errs := ctx.ParseBlueprintsFiles("Blueprints", nil) if len(errs) > 0 { t.Errorf("unexpected parse errors:") @@ -278,20 +319,8 @@ func TestWalkDepsDuplicates(t *testing.T) { t.FailNow() } - var outputDown string - var outputUp string topModule := ctx.moduleGroupFromName("A", nil).modules[0] - ctx.walkDeps(topModule, true, - func(dep depInfo, parent *moduleInfo) bool { - outputDown += ctx.ModuleName(dep.module.logicModule) - if dep.module.logicModule.(Walker).Walk() { - return true - } - return false - }, - func(dep depInfo, parent *moduleInfo) { - outputUp += ctx.ModuleName(dep.module.logicModule) - }) + outputDown, outputUp := walkDependencyGraph(ctx, topModule, true) if outputDown != "BCEGHFGG" { t.Errorf("unexpected walkDeps behaviour: %s\ndown should be: BCEGHFGG", outputDown) } @@ -300,6 +329,75 @@ func TestWalkDepsDuplicates(t *testing.T) { } } +// - represents a non-walkable edge +// A = represents a walkable edge +// |===B-------\ A should not be visited because it's the root node. +// | | B -> D should not be walked. +// |===C===D===E B -> C -> D -> E should be walked +func TestWalkDepsDuplicates_IgnoreFirstPath(t *testing.T) { + ctx := NewContext() + ctx.MockFileSystem(map[string][]byte{ + "Blueprints": []byte(` + foo_module { + name: "A", + deps: ["B"], + } + + foo_module { + name: "B", + deps: ["C"], + ignored_deps: ["D"], + } + + foo_module { + name: "C", + deps: ["D"], + } + + foo_module { + name: "D", + deps: ["E"], + } + + foo_module { + name: "E", + } + `), + }) + + ctx.RegisterModuleType("foo_module", newFooModule) + ctx.RegisterModuleType("bar_module", newBarModule) + ctx.RegisterBottomUpMutator("deps", depsMutator) + _, errs := ctx.ParseBlueprintsFiles("Blueprints", nil) + 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() + } + + topModule := ctx.moduleGroupFromName("A", nil).modules[0] + outputDown, outputUp := walkDependencyGraph(ctx, topModule, true) + expectedDown := "BDCDE" + if outputDown != expectedDown { + t.Errorf("unexpected walkDeps behaviour: %s\ndown should be: %s", outputDown, expectedDown) + } + expectedUp := "DEDCB" + if outputUp != expectedUp { + t.Errorf("unexpected walkDeps behaviour: %s\nup should be: %s", outputUp, expectedUp) + } +} + func TestCreateModule(t *testing.T) { ctx := newContext() ctx.MockFileSystem(map[string][]byte{ @@ -312,7 +410,7 @@ func TestCreateModule(t *testing.T) { }) ctx.RegisterTopDownMutator("create", createTestMutator) - ctx.RegisterBottomUpMutator("deps", blueprintDepsMutator) + ctx.RegisterBottomUpMutator("deps", depsMutator) ctx.RegisterModuleType("foo_module", newFooModule) ctx.RegisterModuleType("bar_module", newBarModule)