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

@@ -2619,8 +2619,8 @@ func (c *Context) walkDeps(topModule *moduleInfo, allowDuplicates bool,
 				}
 				if recurse && !visited[dep.module] {
 					walk(dep.module)
-				}
 					visited[dep.module] = true
+				}
 				if visitUp != nil {
 					visitUp(dep, module)
 				}

context_test.go

@@ -31,10 +31,39 @@ 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
+		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
 }
@@ -63,6 +96,7 @@ type barModule struct {
 	SimpleName
 	properties struct {
 		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,
+	outputDown, outputUp := walkDependencyGraph(ctx, 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)
-		})
 	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,
+	outputDown, outputUp := walkDependencyGraph(ctx, 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)
-		})
 	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)