From 39644c090308a6ce3e9ad798b421f37154056491 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Fri, 21 Aug 2020 18:20:38 -0700 Subject: [PATCH 1/4] Add tests for findVariant Add tests for findVariant behavior that provides the matching behaviors of AddVariationDependencies, AddFarVariationDependencies, etc. Test: Test_findVariant Change-Id: I3494d57179c8b3d62f7d32e5a1b43c9b9672c2df --- context.go | 34 +++------- context_test.go | 166 ++++++++++++++++++++++++++++++++++++++++++++++++ module_ctx.go | 4 +- 3 files changed, 177 insertions(+), 27 deletions(-) diff --git a/context.go b/context.go index a4fea26..482a2b1 100644 --- a/context.go +++ b/context.go @@ -1542,31 +1542,15 @@ func blueprintDepsMutator(ctx BottomUpMutatorContext) { } } -// findMatchingVariant searches the moduleGroup for a module with the same variant as module, -// and returns the matching module, or nil if one is not found. -func (c *Context) findMatchingVariant(module *moduleInfo, possible *moduleGroup, reverse bool) *moduleInfo { +// findExactVariantOrSingle searches the moduleGroup for a module with the same variant as module, +// and returns the matching module, or nil if one is not found. A group with exactly one module +// is always considered matching. +func findExactVariantOrSingle(module *moduleInfo, possible *moduleGroup, reverse bool) *moduleInfo { if len(possible.modules) == 1 { return possible.modules[0] } else { - var variantToMatch variationMap - if !reverse { - // For forward dependency, ignore local variants by matching against - // dependencyVariant which doesn't have the local variants - variantToMatch = module.variant.dependencyVariations - } else { - // For reverse dependency, use all the variants - variantToMatch = module.variant.variations - } - for _, m := range possible.modules { - if m.variant.variations.equal(variantToMatch) { - return m - } - } - for _, m := range possible.aliases { - if m.variant.variations.equal(variantToMatch) { - return m.target - } - } + found, _ := findVariant(module, possible, nil, false, reverse) + return found } return nil @@ -1589,7 +1573,7 @@ func (c *Context) addDependency(module *moduleInfo, tag DependencyTag, depName s return c.discoveredMissingDependencies(module, depName) } - if m := c.findMatchingVariant(module, possibleDeps, false); m != nil { + if m := findExactVariantOrSingle(module, possibleDeps, false); m != nil { module.newDirectDeps = append(module.newDirectDeps, depInfo{m, tag}) atomic.AddUint32(&c.depsModified, 1) return nil @@ -1626,7 +1610,7 @@ func (c *Context) findReverseDependency(module *moduleInfo, destName string) (*m }} } - if m := c.findMatchingVariant(module, possibleDeps, true); m != nil { + if m := findExactVariantOrSingle(module, possibleDeps, true); m != nil { return m, nil } @@ -1644,7 +1628,7 @@ func (c *Context) findReverseDependency(module *moduleInfo, destName string) (*m }} } -func (c *Context) findVariant(module *moduleInfo, possibleDeps *moduleGroup, variations []Variation, far bool, reverse bool) (*moduleInfo, variationMap) { +func findVariant(module *moduleInfo, possibleDeps *moduleGroup, variations []Variation, far bool, reverse bool) (*moduleInfo, variationMap) { // We can't just append variant.Variant to module.dependencyVariant.variantName and // compare the strings because the result won't be in mutator registration order. // Create a new map instead, and then deep compare the maps. diff --git a/context_test.go b/context_test.go index 0541c06..073693c 100644 --- a/context_test.go +++ b/context_test.go @@ -606,3 +606,169 @@ func TestParseFailsForModuleWithoutName(t *testing.T) { t.Errorf("Incorrect errors; expected:\n%s\ngot:\n%s", expectedErrs, errs) } } + +func Test_findVariant(t *testing.T) { + module := &moduleInfo{ + variant: variant{ + name: "normal_local", + variations: variationMap{ + "normal": "normal", + "local": "local", + }, + dependencyVariations: variationMap{ + "normal": "normal", + }, + }, + } + + type alias struct { + variations variationMap + target int + } + + makeDependencyGroup := func(modules []*moduleInfo, aliases []alias) *moduleGroup { + group := &moduleGroup{ + name: "dep", + modules: modules, + } + + for _, alias := range aliases { + group.aliases = append(group.aliases, &moduleAlias{ + variant: variant{ + variations: alias.variations, + }, + target: group.modules[alias.target], + }) + } + + for _, m := range group.modules { + m.group = group + } + return group + } + + tests := []struct { + name string + possibleDeps *moduleGroup + variations []Variation + far bool + reverse bool + want string + }{ + { + name: "AddVariationDependencies(nil)", + // A dependency that matches the non-local variations of the module + possibleDeps: makeDependencyGroup([]*moduleInfo{ + { + variant: variant{ + name: "normal", + variations: variationMap{ + "normal": "normal", + }, + }, + }, + }, nil), + variations: nil, + far: false, + reverse: false, + want: "normal", + }, + { + name: "AddVariationDependencies(nil) to alias", + // A dependency with an alias that matches the non-local variations of the module + possibleDeps: makeDependencyGroup([]*moduleInfo{ + { + variant: variant{ + name: "normal_a", + variations: variationMap{ + "normal": "normal", + "a": "a", + }, + }, + }, + }, []alias{ + { + variations: variationMap{ + "normal": "normal", + }, + target: 0, + }, + }), + variations: nil, + far: false, + reverse: false, + want: "normal_a", + }, + { + name: "AddVariationDependencies(a)", + // A dependency with local variations + possibleDeps: makeDependencyGroup([]*moduleInfo{ + { + variant: variant{ + name: "normal_a", + variations: variationMap{ + "normal": "normal", + "a": "a", + }, + }, + }, + }, nil), + variations: []Variation{{"a", "a"}}, + far: false, + reverse: false, + want: "normal_a", + }, + { + name: "AddFarVariationDependencies(far)", + // A dependency with far variations + possibleDeps: makeDependencyGroup([]*moduleInfo{ + { + variant: variant{ + name: "far", + variations: variationMap{ + "far": "far", + }, + }, + }, + }, nil), + variations: []Variation{{"far", "far"}}, + far: true, + reverse: false, + want: "far", + }, + { + name: "AddFarVariationDependencies(far) to alias", + // A dependency with far variations and aliases + possibleDeps: makeDependencyGroup([]*moduleInfo{ + { + variant: variant{ + name: "far_a", + variations: variationMap{ + "far": "far", + "a": "a", + }, + }, + }, + }, []alias{ + { + variations: variationMap{ + "far": "far", + }, + target: 0, + }, + }), + variations: []Variation{{"far", "far"}}, + far: true, + reverse: false, + want: "far_a", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, _ := findVariant(module, tt.possibleDeps, tt.variations, tt.far, tt.reverse) + if g, w := got.String(), fmt.Sprintf("module %q variant %q", "dep", tt.want); g != w { + t.Errorf("findVariant() got = %v, want %v", g, w) + } + }) + } +} diff --git a/module_ctx.go b/module_ctx.go index c992c0a..34f70f4 100644 --- a/module_ctx.go +++ b/module_ctx.go @@ -510,7 +510,7 @@ func (m *baseModuleContext) OtherModuleDependencyVariantExists(variations []Vari if possibleDeps == nil { return false } - found, _ := m.context.findVariant(m.module, possibleDeps, variations, false, false) + found, _ := findVariant(m.module, possibleDeps, variations, false, false) return found != nil } @@ -519,7 +519,7 @@ func (m *baseModuleContext) OtherModuleReverseDependencyVariantExists(name strin if possibleDeps == nil { return false } - found, _ := m.context.findVariant(m.module, possibleDeps, nil, false, true) + found, _ := findVariant(m.module, possibleDeps, nil, false, true) return found != nil } From 5df74a8e3866129732ee83eed5bfadbe70ffebae Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Mon, 24 Aug 2020 16:18:21 -0700 Subject: [PATCH 2/4] Maintain ordering between variants and aliases AddFarVariationDependencies takes the first matching variant. To maintain sensible behavior on a module with aliases, the ordering of aliases and module variants needs to be maintained so that AddFarVariationDependencies can find an earlier matching alias instead of a more specific variant. Test: go test . Change-Id: I78f4e96edd98159f3a62d94e240e5d652667bec4 --- context.go | 265 +++++++++++++++++++++++------------------ context_test.go | 127 ++++++++++++-------- module_ctx.go | 54 +++++---- module_ctx_test.go | 40 ++----- name_interface.go | 4 +- splice_modules_test.go | 58 ++++----- visit_test.go | 4 +- 7 files changed, 299 insertions(+), 253 deletions(-) diff --git a/context.go b/context.go index 482a2b1..4ca4ef8 100644 --- a/context.go +++ b/context.go @@ -163,16 +163,65 @@ type moduleAlias struct { target *moduleInfo } +func (m *moduleAlias) alias() *moduleAlias { return m } +func (m *moduleAlias) module() *moduleInfo { return nil } +func (m *moduleAlias) moduleOrAliasTarget() *moduleInfo { return m.target } +func (m *moduleAlias) moduleOrAliasVariant() variant { return m.variant } + +func (m *moduleInfo) alias() *moduleAlias { return nil } +func (m *moduleInfo) module() *moduleInfo { return m } +func (m *moduleInfo) moduleOrAliasTarget() *moduleInfo { return m } +func (m *moduleInfo) moduleOrAliasVariant() variant { return m.variant } + +type moduleOrAlias interface { + alias() *moduleAlias + module() *moduleInfo + moduleOrAliasTarget() *moduleInfo + moduleOrAliasVariant() variant +} + +type modulesOrAliases []moduleOrAlias + +func (l modulesOrAliases) firstModule() *moduleInfo { + for _, moduleOrAlias := range l { + if m := moduleOrAlias.module(); m != nil { + return m + } + } + panic(fmt.Errorf("no first module!")) +} + +func (l modulesOrAliases) lastModule() *moduleInfo { + for i := range l { + if m := l[len(l)-1-i].module(); m != nil { + return m + } + } + panic(fmt.Errorf("no last module!")) +} + type moduleGroup struct { name string ninjaName string - modules []*moduleInfo - aliases []*moduleAlias + modules modulesOrAliases namespace Namespace } +func (group *moduleGroup) moduleOrAliasByVariantName(name string) moduleOrAlias { + for _, module := range group.modules { + if module.moduleOrAliasVariant().name == name { + return module + } + } + return nil +} + +func (group *moduleGroup) moduleByVariantName(name string) *moduleInfo { + return group.moduleOrAliasByVariantName(name).module() +} + type moduleInfo struct { // set during Parse typeName string @@ -201,8 +250,7 @@ type moduleInfo struct { waitingCount int // set during each runMutator - splitModules []*moduleInfo - pendingAliases []pendingAlias + splitModules modulesOrAliases // set during PrepareBuildActions actionDefs localBuildActions @@ -1256,14 +1304,14 @@ func newVariant(module *moduleInfo, mutatorName string, variationName string, } func (c *Context) createVariations(origModule *moduleInfo, mutatorName string, - defaultVariationName *string, variationNames []string, local bool) ([]*moduleInfo, []error) { + defaultVariationName *string, variationNames []string, local bool) (modulesOrAliases, []error) { if len(variationNames) == 0 { panic(fmt.Errorf("mutator %q passed zero-length variation list for module %q", mutatorName, origModule.Name())) } - newModules := []*moduleInfo{} + var newModules modulesOrAliases var errs []error @@ -1312,37 +1360,19 @@ func (c *Context) convertDepsToVariation(module *moduleInfo, if dep.module.logicModule == nil { var newDep *moduleInfo for _, m := range dep.module.splitModules { - if m.variant.variations[mutatorName] == variationName { - newDep = m + if m.moduleOrAliasVariant().variations[mutatorName] == variationName { + newDep = m.moduleOrAliasTarget() break } } - if newDep == nil { - // check aliases against variationNam - for _, a := range dep.module.pendingAliases { - if a.fromVariant.variations[mutatorName] == variationName { - newDep = a.target - break - } - } - } if newDep == nil && defaultVariationName != nil { // give it a second chance; match with defaultVariationName for _, m := range dep.module.splitModules { - if m.variant.variations[mutatorName] == *defaultVariationName { - newDep = m + if m.moduleOrAliasVariant().variations[mutatorName] == *defaultVariationName { + newDep = m.moduleOrAliasTarget() break } } - if newDep == nil { - // check aliases against defaultVariationName - for _, a := range dep.module.pendingAliases { - if a.fromVariant.variations[mutatorName] == *defaultVariationName { - newDep = a.target - break - } - } - } } if newDep == nil { errs = append(errs, &BlueprintError{ @@ -1372,14 +1402,14 @@ func (c *Context) prettyPrintVariant(variations variationMap) string { func (c *Context) prettyPrintGroupVariants(group *moduleGroup) string { var variants []string - for _, mod := range group.modules { - variants = append(variants, c.prettyPrintVariant(mod.variant.variations)) + for _, moduleOrAlias := range group.modules { + if mod := moduleOrAlias.module(); mod != nil { + variants = append(variants, c.prettyPrintVariant(mod.variant.variations)) + } else if alias := moduleOrAlias.alias(); alias != nil { + variants = append(variants, c.prettyPrintVariant(alias.variant.variations)+ + "(alias to "+c.prettyPrintVariant(alias.target.variant.variations)+")") + } } - for _, mod := range group.aliases { - variants = append(variants, c.prettyPrintVariant(mod.variant.variations)+ - "(alias to "+c.prettyPrintVariant(mod.target.variant.variations)+")") - } - sort.Strings(variants) return strings.Join(variants, "\n ") } @@ -1458,7 +1488,7 @@ func (c *Context) addModule(module *moduleInfo) []error { group := &moduleGroup{ name: name, - modules: []*moduleInfo{module}, + modules: modulesOrAliases{module}, } module.group = group namespace, errs := c.nameInterface.NewModule( @@ -1546,14 +1576,19 @@ func blueprintDepsMutator(ctx BottomUpMutatorContext) { // and returns the matching module, or nil if one is not found. A group with exactly one module // is always considered matching. func findExactVariantOrSingle(module *moduleInfo, possible *moduleGroup, reverse bool) *moduleInfo { - if len(possible.modules) == 1 { - return possible.modules[0] - } else { - found, _ := findVariant(module, possible, nil, false, reverse) - return found + found, _ := findVariant(module, possible, nil, false, reverse) + if found == nil { + for _, moduleOrAlias := range possible.modules { + if m := moduleOrAlias.module(); m != nil { + if found != nil { + // more than one possible match, give up + return nil + } + found = m + } + } } - - return nil + return found } func (c *Context) addDependency(module *moduleInfo, tag DependencyTag, depName string) []error { @@ -1660,21 +1695,12 @@ func findVariant(module *moduleInfo, possibleDeps *moduleGroup, variations []Var var foundDep *moduleInfo for _, m := range possibleDeps.modules { - if check(m.variant.variations) { - foundDep = m + if check(m.moduleOrAliasVariant().variations) { + foundDep = m.moduleOrAliasTarget() break } } - if foundDep == nil { - for _, m := range possibleDeps.aliases { - if check(m.variant.variations) { - foundDep = m.target - break - } - } - } - return foundDep, newVariant } @@ -1689,7 +1715,7 @@ func (c *Context) addVariationDependency(module *moduleInfo, variations []Variat return c.discoveredMissingDependencies(module, depName) } - foundDep, newVariant := c.findVariant(module, possibleDeps, variations, far, false) + foundDep, newVariant := findVariant(module, possibleDeps, variations, far, false) if foundDep == nil { if c.allowMissingDependencies { @@ -1732,14 +1758,16 @@ func (c *Context) addInterVariantDependency(origModule *moduleInfo, tag Dependen } var fromInfo, toInfo *moduleInfo - for _, m := range origModule.splitModules { - if m.logicModule == from { - fromInfo = m - } - if m.logicModule == to { - toInfo = m - if fromInfo != nil { - panic(fmt.Errorf("%q depends on later version of itself", origModule.Name())) + for _, moduleOrAlias := range origModule.splitModules { + if m := moduleOrAlias.module(); m != nil { + if m.logicModule == from { + fromInfo = m + } + if m.logicModule == to { + toInfo = m + if fromInfo != nil { + panic(fmt.Errorf("%q depends on later version of itself", origModule.Name())) + } } } } @@ -1971,7 +1999,9 @@ func (c *Context) updateDependencies() (errs []error) { if dep == module { break } - deps[dep] = true + if depModule := dep.module(); depModule != nil { + deps[depModule] = true + } } for _, dep := range module.directDeps { @@ -2213,7 +2243,7 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, errsCh := make(chan []error) globalStateCh := make(chan globalStateChange) - newVariationsCh := make(chan []*moduleInfo) + newVariationsCh := make(chan modulesOrAliases) done := make(chan bool) c.depsModified = 0 @@ -2223,8 +2253,6 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, panic("split module found in sorted module list") } - module.pendingAliases = nil - mctx := &mutatorContext{ baseModuleContext: baseModuleContext{ context: c, @@ -2286,8 +2314,10 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, newModules = append(newModules, globalStateChange.newModules...) deps = append(deps, globalStateChange.deps...) case newVariations := <-newVariationsCh: - for _, m := range newVariations { - newModuleInfo[m.logicModule] = m + for _, moduleOrAlias := range newVariations { + if m := moduleOrAlias.module(); m != nil { + newModuleInfo[m.logicModule] = m + } } case <-done: return @@ -2311,31 +2341,27 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, for _, group := range c.moduleGroups { for i := 0; i < len(group.modules); i++ { - module := group.modules[i] + module := group.modules[i].module() + if module == nil { + // Existing alias, skip it + continue + } // Update module group to contain newly split variants if module.splitModules != nil { group.modules, i = spliceModules(group.modules, i, module.splitModules) } - // Create any new aliases. - for _, alias := range module.pendingAliases { - group.aliases = append(group.aliases, &moduleAlias{ - variant: alias.fromVariant, - target: alias.target, - }) - } - // Fix up any remaining dependencies on modules that were split into variants // by replacing them with the first variant for j, dep := range module.directDeps { if dep.module.logicModule == nil { - module.directDeps[j].module = dep.module.splitModules[0] + module.directDeps[j].module = dep.module.splitModules.firstModule() } } if module.createdBy != nil && module.createdBy.logicModule == nil { - module.createdBy = module.createdBy.splitModules[0] + module.createdBy = module.createdBy.splitModules.firstModule() } // Add in any new direct dependencies that were added by the mutator @@ -2344,26 +2370,30 @@ func (c *Context) runMutator(config interface{}, mutator *mutatorInfo, } findAliasTarget := func(variant variant) *moduleInfo { - for _, alias := range group.aliases { - if alias.variant.variations.equal(variant.variations) { - return alias.target + for _, moduleOrAlias := range group.modules { + if alias := moduleOrAlias.alias(); alias != nil { + if alias.variant.variations.equal(variant.variations) { + return alias.target + } } } return nil } // Forward or delete any dangling aliases. - for i := 0; i < len(group.aliases); i++ { - alias := group.aliases[i] - - if alias.target.logicModule == nil { - newTarget := findAliasTarget(alias.target.variant) - if newTarget != nil { - alias.target = newTarget - } else { - // The alias was left dangling, remove it. - group.aliases = append(group.aliases[:i], group.aliases[i+1:]...) - i-- + // Use a manual loop instead of range because len(group.modules) can + // change inside the loop + for i := 0; i < len(group.modules); i++ { + if alias := group.modules[i].alias(); alias != nil { + if alias.target.logicModule == nil { + newTarget := findAliasTarget(alias.target.variant) + if newTarget != nil { + alias.target = newTarget + } else { + // The alias was left dangling, remove it. + group.modules = append(group.modules[:i], group.modules[i+1:]...) + i-- + } } } } @@ -2438,15 +2468,15 @@ func (c *Context) cloneModules() { // Removes modules[i] from the list and inserts newModules... where it was located, returning // the new slice and the index of the last inserted element -func spliceModules(modules []*moduleInfo, i int, newModules []*moduleInfo) ([]*moduleInfo, int) { +func spliceModules(modules modulesOrAliases, i int, newModules modulesOrAliases) (modulesOrAliases, int) { spliceSize := len(newModules) newLen := len(modules) + spliceSize - 1 - var dest []*moduleInfo + var dest modulesOrAliases if cap(modules) >= len(modules)-1+len(newModules) { // We can fit the splice in the existing capacity, do everything in place dest = modules[:newLen] } else { - dest = make([]*moduleInfo, newLen) + dest = make(modulesOrAliases, newLen) copy(dest, modules[:i]) } @@ -2703,14 +2733,8 @@ func (c *Context) moduleMatchingVariant(module *moduleInfo, name string) *module } for _, m := range group.modules { - if module.variant.name == m.variant.name { - return m - } - } - - for _, m := range group.aliases { - if module.variant.name == m.variant.name { - return m.target + if module.variant.variations.equal(m.moduleOrAliasVariant().variations) { + return m.moduleOrAliasTarget() } } @@ -2807,8 +2831,10 @@ func (c *Context) visitAllModules(visit func(Module)) { }() for _, moduleGroup := range c.sortedModuleGroups() { - for _, module = range moduleGroup.modules { - visit(module.logicModule) + for _, moduleOrAlias := range moduleGroup.modules { + if module = moduleOrAlias.module(); module != nil { + visit(module.logicModule) + } } } } @@ -2826,9 +2852,11 @@ func (c *Context) visitAllModulesIf(pred func(Module) bool, }() for _, moduleGroup := range c.sortedModuleGroups() { - for _, module := range moduleGroup.modules { - if pred(module.logicModule) { - visit(module.logicModule) + for _, moduleOrAlias := range moduleGroup.modules { + if module = moduleOrAlias.module(); module != nil { + if pred(module.logicModule) { + visit(module.logicModule) + } } } } @@ -2846,8 +2874,10 @@ func (c *Context) visitAllModuleVariants(module *moduleInfo, } }() - for _, variant = range module.group.modules { - visit(variant.logicModule) + for _, moduleOrAlias := range module.group.modules { + if variant = moduleOrAlias.module(); variant != nil { + visit(variant.logicModule) + } } } @@ -3191,12 +3221,11 @@ func (c *Context) VisitDepsDepthFirstIf(module Module, pred func(Module) bool, v } func (c *Context) PrimaryModule(module Module) Module { - return c.moduleInfo[module].group.modules[0].logicModule + return c.moduleInfo[module].group.modules.firstModule().logicModule } func (c *Context) FinalModule(module Module) Module { - modules := c.moduleInfo[module].group.modules - return modules[len(modules)-1].logicModule + return c.moduleInfo[module].group.modules.lastModule().logicModule } func (c *Context) VisitAllModuleVariants(module Module, @@ -3756,15 +3785,15 @@ func (c *Context) writeLocalBuildActions(nw *ninjaWriter, return nil } -func beforeInModuleList(a, b *moduleInfo, list []*moduleInfo) bool { +func beforeInModuleList(a, b *moduleInfo, list modulesOrAliases) bool { found := false if a == b { return false } for _, l := range list { - if l == a { + if l.module() == a { found = true - } else if l == b { + } else if l.module() == b { return found } } diff --git a/context_test.go b/context_test.go index 073693c..b33c7bf 100644 --- a/context_test.go +++ b/context_test.go @@ -238,7 +238,7 @@ func TestWalkDeps(t *testing.T) { t.FailNow() } - topModule := ctx.moduleGroupFromName("A", nil).modules[0] + topModule := ctx.moduleGroupFromName("A", nil).modules.firstModule() outputDown, outputUp := walkDependencyGraph(ctx, topModule, false) if outputDown != "BCEFG" { t.Errorf("unexpected walkDeps behaviour: %s\ndown should be: BCEFG", outputDown) @@ -319,7 +319,7 @@ func TestWalkDepsDuplicates(t *testing.T) { t.FailNow() } - topModule := ctx.moduleGroupFromName("A", nil).modules[0] + topModule := ctx.moduleGroupFromName("A", nil).modules.firstModule() outputDown, outputUp := walkDependencyGraph(ctx, topModule, true) if outputDown != "BCEGHFGG" { t.Errorf("unexpected walkDeps behaviour: %s\ndown should be: BCEGHFGG", outputDown) @@ -386,7 +386,7 @@ func TestWalkDepsDuplicates_IgnoreFirstPath(t *testing.T) { t.FailNow() } - topModule := ctx.moduleGroupFromName("A", nil).modules[0] + topModule := ctx.moduleGroupFromName("A", nil).modules.firstModule() outputDown, outputUp := walkDependencyGraph(ctx, topModule, true) expectedDown := "BDCDE" if outputDown != expectedDown { @@ -432,10 +432,10 @@ func TestCreateModule(t *testing.T) { t.FailNow() } - a := ctx.moduleGroupFromName("A", nil).modules[0].logicModule.(*fooModule) - b := ctx.moduleGroupFromName("B", nil).modules[0].logicModule.(*barModule) - c := ctx.moduleGroupFromName("C", nil).modules[0].logicModule.(*barModule) - d := ctx.moduleGroupFromName("D", nil).modules[0].logicModule.(*fooModule) + a := ctx.moduleGroupFromName("A", nil).modules.firstModule().logicModule.(*fooModule) + b := ctx.moduleGroupFromName("B", nil).modules.firstModule().logicModule.(*barModule) + c := ctx.moduleGroupFromName("C", nil).modules.firstModule().logicModule.(*barModule) + d := ctx.moduleGroupFromName("D", nil).modules.firstModule().logicModule.(*fooModule) checkDeps := func(m Module, expected string) { var deps []string @@ -622,28 +622,42 @@ func Test_findVariant(t *testing.T) { } type alias struct { - variations variationMap - target int + variant variant + target int } - makeDependencyGroup := func(modules []*moduleInfo, aliases []alias) *moduleGroup { + makeDependencyGroup := func(in ...interface{}) *moduleGroup { group := &moduleGroup{ - name: "dep", - modules: modules, + name: "dep", + } + for _, x := range in { + switch m := x.(type) { + case *moduleInfo: + m.group = group + group.modules = append(group.modules, m) + case alias: + // aliases may need to target modules that haven't been processed + // yet, put an empty alias in for now. + group.modules = append(group.modules, nil) + default: + t.Fatalf("unexpected type %T", x) + } } - for _, alias := range aliases { - group.aliases = append(group.aliases, &moduleAlias{ - variant: variant{ - variations: alias.variations, - }, - target: group.modules[alias.target], - }) + for i, x := range in { + switch m := x.(type) { + case *moduleInfo: + // already added in the first pass + case alias: + group.modules[i] = &moduleAlias{ + variant: m.variant, + target: group.modules[m.target].moduleOrAliasTarget(), + } + default: + t.Fatalf("unexpected type %T", x) + } } - for _, m := range group.modules { - m.group = group - } return group } @@ -658,8 +672,8 @@ func Test_findVariant(t *testing.T) { { name: "AddVariationDependencies(nil)", // A dependency that matches the non-local variations of the module - possibleDeps: makeDependencyGroup([]*moduleInfo{ - { + possibleDeps: makeDependencyGroup( + &moduleInfo{ variant: variant{ name: "normal", variations: variationMap{ @@ -667,7 +681,7 @@ func Test_findVariant(t *testing.T) { }, }, }, - }, nil), + ), variations: nil, far: false, reverse: false, @@ -676,8 +690,17 @@ func Test_findVariant(t *testing.T) { { name: "AddVariationDependencies(nil) to alias", // A dependency with an alias that matches the non-local variations of the module - possibleDeps: makeDependencyGroup([]*moduleInfo{ - { + possibleDeps: makeDependencyGroup( + alias{ + variant: variant{ + name: "normal", + variations: variationMap{ + "normal": "normal", + }, + }, + target: 1, + }, + &moduleInfo{ variant: variant{ name: "normal_a", variations: variationMap{ @@ -686,14 +709,7 @@ func Test_findVariant(t *testing.T) { }, }, }, - }, []alias{ - { - variations: variationMap{ - "normal": "normal", - }, - target: 0, - }, - }), + ), variations: nil, far: false, reverse: false, @@ -702,8 +718,8 @@ func Test_findVariant(t *testing.T) { { name: "AddVariationDependencies(a)", // A dependency with local variations - possibleDeps: makeDependencyGroup([]*moduleInfo{ - { + possibleDeps: makeDependencyGroup( + &moduleInfo{ variant: variant{ name: "normal_a", variations: variationMap{ @@ -712,7 +728,7 @@ func Test_findVariant(t *testing.T) { }, }, }, - }, nil), + ), variations: []Variation{{"a", "a"}}, far: false, reverse: false, @@ -721,8 +737,8 @@ func Test_findVariant(t *testing.T) { { name: "AddFarVariationDependencies(far)", // A dependency with far variations - possibleDeps: makeDependencyGroup([]*moduleInfo{ - { + possibleDeps: makeDependencyGroup( + &moduleInfo{ variant: variant{ name: "far", variations: variationMap{ @@ -730,7 +746,7 @@ func Test_findVariant(t *testing.T) { }, }, }, - }, nil), + ), variations: []Variation{{"far", "far"}}, far: true, reverse: false, @@ -739,8 +755,17 @@ func Test_findVariant(t *testing.T) { { name: "AddFarVariationDependencies(far) to alias", // A dependency with far variations and aliases - possibleDeps: makeDependencyGroup([]*moduleInfo{ - { + possibleDeps: makeDependencyGroup( + alias{ + variant: variant{ + name: "far", + variations: variationMap{ + "far": "far", + }, + }, + target: 2, + }, + &moduleInfo{ variant: variant{ name: "far_a", variations: variationMap{ @@ -749,18 +774,20 @@ func Test_findVariant(t *testing.T) { }, }, }, - }, []alias{ - { - variations: variationMap{ - "far": "far", + &moduleInfo{ + variant: variant{ + name: "far_b", + variations: variationMap{ + "far": "far", + "b": "b", + }, }, - target: 0, }, - }), + ), variations: []Variation{{"far", "far"}}, far: true, reverse: false, - want: "far_a", + want: "far_b", }, } for _, tt := range tests { diff --git a/module_ctx.go b/module_ctx.go index 34f70f4..50fbcf0 100644 --- a/module_ctx.go +++ b/module_ctx.go @@ -643,11 +643,11 @@ func (m *baseModuleContext) WalkDeps(visit func(child, parent Module) bool) { } func (m *baseModuleContext) PrimaryModule() Module { - return m.module.group.modules[0].logicModule + return m.module.group.modules.firstModule().logicModule } func (m *baseModuleContext) FinalModule() Module { - return m.module.group.modules[len(m.module.group.modules)-1].logicModule + return m.module.group.modules.lastModule().logicModule } func (m *baseModuleContext) VisitAllModuleVariants(visit func(Module)) { @@ -722,8 +722,8 @@ type mutatorContext struct { reverseDeps []reverseDep rename []rename replace []replace - newVariations []*moduleInfo // new variants of existing modules - newModules []*moduleInfo // brand new modules + newVariations modulesOrAliases // new variants of existing modules + newModules []*moduleInfo // brand new modules defaultVariation *string } @@ -921,7 +921,7 @@ func (mctx *mutatorContext) createVariations(variationNames []string, local bool } for _, module := range modules { - ret = append(ret, module.logicModule) + ret = append(ret, module.module().logicModule) } if mctx.newVariations != nil { @@ -937,25 +937,30 @@ func (mctx *mutatorContext) createVariations(variationNames []string, local bool } func (mctx *mutatorContext) AliasVariation(variationName string) { - for _, alias := range mctx.module.pendingAliases { - if alias.fromVariant.variations.equal(mctx.module.variant.variations) { - panic(fmt.Errorf("AliasVariation already called")) + for _, moduleOrAlias := range mctx.module.splitModules { + if alias := moduleOrAlias.alias(); alias != nil { + if alias.variant.variations.equal(mctx.module.variant.variations) { + panic(fmt.Errorf("AliasVariation already called")) + } } } for _, variant := range mctx.newVariations { - if variant.variant.variations[mctx.name] == variationName { - mctx.module.pendingAliases = append(mctx.module.pendingAliases, pendingAlias{ - fromVariant: mctx.module.variant, - target: variant, - }) + if variant.moduleOrAliasVariant().variations[mctx.name] == variationName { + alias := &moduleAlias{ + variant: mctx.module.variant, + target: variant.moduleOrAliasTarget(), + } + // Prepend the alias so that AddFarVariationDependencies subset match matches + // the alias before matching the first variation. + mctx.module.splitModules = append(modulesOrAliases{alias}, mctx.module.splitModules...) return } } var foundVariations []string for _, variant := range mctx.newVariations { - foundVariations = append(foundVariations, variant.variant.variations[mctx.name]) + foundVariations = append(foundVariations, variant.moduleOrAliasVariant().variations[mctx.name]) } panic(fmt.Errorf("no %q variation in module variations %q", variationName, foundVariations)) } @@ -963,17 +968,22 @@ func (mctx *mutatorContext) AliasVariation(variationName string) { func (mctx *mutatorContext) CreateAliasVariation(aliasVariationName, targetVariationName string) { newVariant := newVariant(mctx.module, mctx.name, aliasVariationName, false) - for _, alias := range mctx.module.pendingAliases { - if alias.fromVariant.variations.equal(newVariant.variations) { - panic(fmt.Errorf("can't alias %q to %q, already aliased to %q", aliasVariationName, targetVariationName, alias.target.variant.name)) + for _, moduleOrAlias := range mctx.module.splitModules { + if moduleOrAlias.moduleOrAliasVariant().variations.equal(newVariant.variations) { + if alias := moduleOrAlias.alias(); alias != nil { + panic(fmt.Errorf("can't alias %q to %q, already aliased to %q", aliasVariationName, targetVariationName, alias.target.variant.name)) + } else { + panic(fmt.Errorf("can't alias %q to %q, there is already a variant with that name", aliasVariationName, targetVariationName)) + } } } for _, variant := range mctx.newVariations { - if variant.variant.variations[mctx.name] == targetVariationName { - mctx.module.pendingAliases = append(mctx.module.pendingAliases, pendingAlias{ - fromVariant: newVariant, - target: variant, + if variant.moduleOrAliasVariant().variations[mctx.name] == targetVariationName { + // Append the alias here so that it comes after any aliases created by AliasVariation. + mctx.module.splitModules = append(mctx.module.splitModules, &moduleAlias{ + variant: newVariant, + target: variant.moduleOrAliasTarget(), }) return } @@ -981,7 +991,7 @@ func (mctx *mutatorContext) CreateAliasVariation(aliasVariationName, targetVaria var foundVariations []string for _, variant := range mctx.newVariations { - foundVariations = append(foundVariations, variant.variant.variations[mctx.name]) + foundVariations = append(foundVariations, variant.moduleOrAliasVariant().variations[mctx.name]) } panic(fmt.Errorf("no %q variation in module variations %q", targetVariationName, foundVariations)) } diff --git a/module_ctx_test.go b/module_ctx_test.go index 22666dc..62a89c1 100644 --- a/module_ctx_test.go +++ b/module_ctx_test.go @@ -131,12 +131,8 @@ func TestAliasVariation(t *testing.T) { run(ctx) - foo := ctx.moduleGroupFromName("foo", nil).modules[0] - barB := ctx.moduleGroupFromName("bar", nil).modules[1] - - if g, w := barB.variant.name, "b"; g != w { - t.Fatalf("expected bar.modules[1] variant to be %q, got %q", w, g) - } + foo := ctx.moduleGroupFromName("foo", nil).moduleByVariantName("") + barB := ctx.moduleGroupFromName("bar", nil).moduleByVariantName("b") if g, w := foo.forwardDeps, []*moduleInfo{barB}; !reflect.DeepEqual(g, w) { t.Fatalf("expected foo deps to be %q, got %q", w, g) @@ -155,12 +151,8 @@ func TestAliasVariation(t *testing.T) { run(ctx) - foo := ctx.moduleGroupFromName("foo", nil).modules[0] - barBB := ctx.moduleGroupFromName("bar", nil).modules[3] - - if g, w := barBB.variant.name, "b_b"; g != w { - t.Fatalf("expected bar.modules[3] variant to be %q, got %q", w, g) - } + foo := ctx.moduleGroupFromName("foo", nil).moduleByVariantName("") + barBB := ctx.moduleGroupFromName("bar", nil).moduleByVariantName("b_b") if g, w := foo.forwardDeps, []*moduleInfo{barBB}; !reflect.DeepEqual(g, w) { t.Fatalf("expected foo deps to be %q, got %q", w, g) @@ -179,12 +171,8 @@ func TestAliasVariation(t *testing.T) { run(ctx) - foo := ctx.moduleGroupFromName("foo", nil).modules[0] - barAB := ctx.moduleGroupFromName("bar", nil).modules[1] - - if g, w := barAB.variant.name, "a_b"; g != w { - t.Fatalf("expected bar.modules[1] variant to be %q, got %q", w, g) - } + foo := ctx.moduleGroupFromName("foo", nil).moduleByVariantName("") + barAB := ctx.moduleGroupFromName("bar", nil).moduleByVariantName("a_b") if g, w := foo.forwardDeps, []*moduleInfo{barAB}; !reflect.DeepEqual(g, w) { t.Fatalf("expected foo deps to be %q, got %q", w, g) @@ -270,12 +258,8 @@ func TestCreateAliasVariations(t *testing.T) { run(ctx) - foo := ctx.moduleGroupFromName("foo", nil).modules[0] - barB := ctx.moduleGroupFromName("bar", nil).modules[1] - - if g, w := barB.variant.name, "b"; g != w { - t.Fatalf("expected bar.modules[1] variant to be %q, got %q", w, g) - } + foo := ctx.moduleGroupFromName("foo", nil).moduleByVariantName("") + barB := ctx.moduleGroupFromName("bar", nil).moduleByVariantName("b") if g, w := foo.forwardDeps, []*moduleInfo{barB}; !reflect.DeepEqual(g, w) { t.Fatalf("expected foo deps to be %q, got %q", w, g) @@ -294,12 +278,8 @@ func TestCreateAliasVariations(t *testing.T) { run(ctx) - foo := ctx.moduleGroupFromName("foo", nil).modules[0] - barBB := ctx.moduleGroupFromName("bar", nil).modules[3] - - if g, w := barBB.variant.name, "b_b"; g != w { - t.Fatalf("expected bar.modules[3] variant to be %q, got %q", w, g) - } + foo := ctx.moduleGroupFromName("foo", nil).moduleByVariantName("") + barBB := ctx.moduleGroupFromName("bar", nil).moduleByVariantName("b_b") if g, w := foo.forwardDeps, []*moduleInfo{barBB}; !reflect.DeepEqual(g, w) { t.Fatalf("expected foo deps to be %q, got %q", w, g) diff --git a/name_interface.go b/name_interface.go index 1849e9d..5e7e16e 100644 --- a/name_interface.go +++ b/name_interface.go @@ -109,7 +109,7 @@ func (s *SimpleNameInterface) NewModule(ctx NamespaceContext, group ModuleGroup, return nil, []error{ // seven characters at the start of the second line to align with the string "error: " fmt.Errorf("module %q already defined\n"+ - " %s <-- previous definition here", name, group.modules[0].pos), + " %s <-- previous definition here", name, group.modules.firstModule().pos), } } @@ -130,7 +130,7 @@ func (s *SimpleNameInterface) Rename(oldName string, newName string, namespace N // seven characters at the start of the second line to align with the string "error: " fmt.Errorf("renaming module %q to %q conflicts with existing module\n"+ " %s <-- existing module defined here", - oldName, newName, existingGroup.modules[0].pos), + oldName, newName, existingGroup.modules.firstModule().pos), } } diff --git a/splice_modules_test.go b/splice_modules_test.go index 059ff41..473999a 100644 --- a/splice_modules_test.go +++ b/splice_modules_test.go @@ -29,82 +29,82 @@ var ( ) var spliceModulesTestCases = []struct { - in []*moduleInfo + in modulesOrAliases at int - with []*moduleInfo - out []*moduleInfo + with modulesOrAliases + out modulesOrAliases outAt int reallocate bool }{ { // Insert at the beginning - in: []*moduleInfo{testModuleA, testModuleB, testModuleC}, + in: modulesOrAliases{testModuleA, testModuleB, testModuleC}, at: 0, - with: []*moduleInfo{testModuleD, testModuleE}, - out: []*moduleInfo{testModuleD, testModuleE, testModuleB, testModuleC}, + with: modulesOrAliases{testModuleD, testModuleE}, + out: modulesOrAliases{testModuleD, testModuleE, testModuleB, testModuleC}, outAt: 1, reallocate: true, }, { // Insert in the middle - in: []*moduleInfo{testModuleA, testModuleB, testModuleC}, + in: modulesOrAliases{testModuleA, testModuleB, testModuleC}, at: 1, - with: []*moduleInfo{testModuleD, testModuleE}, - out: []*moduleInfo{testModuleA, testModuleD, testModuleE, testModuleC}, + with: modulesOrAliases{testModuleD, testModuleE}, + out: modulesOrAliases{testModuleA, testModuleD, testModuleE, testModuleC}, outAt: 2, reallocate: true, }, { // Insert at the end - in: []*moduleInfo{testModuleA, testModuleB, testModuleC}, + in: modulesOrAliases{testModuleA, testModuleB, testModuleC}, at: 2, - with: []*moduleInfo{testModuleD, testModuleE}, - out: []*moduleInfo{testModuleA, testModuleB, testModuleD, testModuleE}, + with: modulesOrAliases{testModuleD, testModuleE}, + out: modulesOrAliases{testModuleA, testModuleB, testModuleD, testModuleE}, outAt: 3, reallocate: true, }, { // Insert over a single element - in: []*moduleInfo{testModuleA}, + in: modulesOrAliases{testModuleA}, at: 0, - with: []*moduleInfo{testModuleD, testModuleE}, - out: []*moduleInfo{testModuleD, testModuleE}, + with: modulesOrAliases{testModuleD, testModuleE}, + out: modulesOrAliases{testModuleD, testModuleE}, outAt: 1, reallocate: true, }, { // Insert at the beginning without reallocating - in: []*moduleInfo{testModuleA, testModuleB, testModuleC, nil}[0:3], + in: modulesOrAliases{testModuleA, testModuleB, testModuleC, nil}[0:3], at: 0, - with: []*moduleInfo{testModuleD, testModuleE}, - out: []*moduleInfo{testModuleD, testModuleE, testModuleB, testModuleC}, + with: modulesOrAliases{testModuleD, testModuleE}, + out: modulesOrAliases{testModuleD, testModuleE, testModuleB, testModuleC}, outAt: 1, reallocate: false, }, { // Insert in the middle without reallocating - in: []*moduleInfo{testModuleA, testModuleB, testModuleC, nil}[0:3], + in: modulesOrAliases{testModuleA, testModuleB, testModuleC, nil}[0:3], at: 1, - with: []*moduleInfo{testModuleD, testModuleE}, - out: []*moduleInfo{testModuleA, testModuleD, testModuleE, testModuleC}, + with: modulesOrAliases{testModuleD, testModuleE}, + out: modulesOrAliases{testModuleA, testModuleD, testModuleE, testModuleC}, outAt: 2, reallocate: false, }, { // Insert at the end without reallocating - in: []*moduleInfo{testModuleA, testModuleB, testModuleC, nil}[0:3], + in: modulesOrAliases{testModuleA, testModuleB, testModuleC, nil}[0:3], at: 2, - with: []*moduleInfo{testModuleD, testModuleE}, - out: []*moduleInfo{testModuleA, testModuleB, testModuleD, testModuleE}, + with: modulesOrAliases{testModuleD, testModuleE}, + out: modulesOrAliases{testModuleA, testModuleB, testModuleD, testModuleE}, outAt: 3, reallocate: false, }, { // Insert over a single element without reallocating - in: []*moduleInfo{testModuleA, nil}[0:1], + in: modulesOrAliases{testModuleA, nil}[0:1], at: 0, - with: []*moduleInfo{testModuleD, testModuleE}, - out: []*moduleInfo{testModuleD, testModuleE}, + with: modulesOrAliases{testModuleD, testModuleE}, + out: modulesOrAliases{testModuleD, testModuleE}, outAt: 1, reallocate: false, }, @@ -112,7 +112,7 @@ var spliceModulesTestCases = []struct { func TestSpliceModules(t *testing.T) { for _, testCase := range spliceModulesTestCases { - in := make([]*moduleInfo, len(testCase.in), cap(testCase.in)) + in := make(modulesOrAliases, len(testCase.in), cap(testCase.in)) copy(in, testCase.in) origIn := in got, gotAt := spliceModules(in, testCase.at, testCase.with) @@ -139,6 +139,6 @@ func TestSpliceModules(t *testing.T) { } } -func sameArray(a, b []*moduleInfo) bool { +func sameArray(a, b modulesOrAliases) bool { return &a[0:cap(a)][cap(a)-1] == &b[0:cap(b)][cap(b)-1] } diff --git a/visit_test.go b/visit_test.go index efaadba..1c74b93 100644 --- a/visit_test.go +++ b/visit_test.go @@ -149,13 +149,13 @@ func setupVisitTest(t *testing.T) *Context { func TestVisit(t *testing.T) { ctx := setupVisitTest(t) - topModule := ctx.moduleGroupFromName("A", nil).modules[0].logicModule.(*visitModule) + topModule := ctx.moduleGroupFromName("A", nil).modules.firstModule().logicModule.(*visitModule) 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.moduleGroupFromName("E", nil).modules[0].logicModule.(*visitModule) + eModule := ctx.moduleGroupFromName("E", nil).modules.firstModule().logicModule.(*visitModule) assertString(t, eModule.properties.VisitDepsDepthFirst, "F") assertString(t, eModule.properties.VisitDepsDepthFirstIf, "F") assertString(t, eModule.properties.VisitDirectDeps, "FF") From 5dc6759951022a4f943dabe222900ebf8c6335cb Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Mon, 24 Aug 2020 14:46:13 -0700 Subject: [PATCH 3/4] Fix AddFarVariationDependencies subset checks AddFarVariationDependencies claims to take the first variant that matches all the requested variations, but it did nothing of the sort. It took the first variant that either matched or did not contain each of the requested variations. A module with no variations was always matched, and requesting variations that didn't apply to a module still matched (for example, requesting an image variation for a host module that was ignored by the image mutator). Fix AddFarVariationDependencies by making subset actually check for subsets. Test: Test_findVariant Change-Id: I10063fec342db2a1c0685a7db08e4a650d14bd4e --- context.go | 8 ++++---- context_test.go | 43 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/context.go b/context.go index 4ca4ef8..e1bd892 100644 --- a/context.go +++ b/context.go @@ -323,10 +323,10 @@ func (vm variationMap) clone() variationMap { } // Compare this variationMap to another one. Returns true if the every entry in this map -// is either the same in the other map or doesn't exist in the other map. -func (vm variationMap) subset(other variationMap) bool { +// exists and has the same value in the other map. +func (vm variationMap) subsetOf(other variationMap) bool { for k, v1 := range vm { - if v2, ok := other[k]; ok && v1 != v2 { + if v2, ok := other[k]; !ok || v1 != v2 { return false } } @@ -1687,7 +1687,7 @@ func findVariant(module *moduleInfo, possibleDeps *moduleGroup, variations []Var check := func(variant variationMap) bool { if far { - return variant.subset(newVariant) + return newVariant.subsetOf(variant) } else { return variant.equal(newVariant) } diff --git a/context_test.go b/context_test.go index b33c7bf..91089e6 100644 --- a/context_test.go +++ b/context_test.go @@ -738,6 +738,12 @@ func Test_findVariant(t *testing.T) { name: "AddFarVariationDependencies(far)", // A dependency with far variations possibleDeps: makeDependencyGroup( + &moduleInfo{ + variant: variant{ + name: "", + variations: nil, + }, + }, &moduleInfo{ variant: variant{ name: "far", @@ -789,12 +795,45 @@ func Test_findVariant(t *testing.T) { reverse: false, want: "far_b", }, + { + name: "AddFarVariationDependencies(far, b) to missing", + // A dependency with far variations and aliases + possibleDeps: makeDependencyGroup( + alias{ + variant: variant{ + name: "far", + variations: variationMap{ + "far": "far", + }, + }, + target: 1, + }, + &moduleInfo{ + variant: variant{ + name: "far_a", + variations: variationMap{ + "far": "far", + "a": "a", + }, + }, + }, + ), + variations: []Variation{{"far", "far"}, {"a", "b"}}, + far: true, + reverse: false, + want: "nil", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, _ := findVariant(module, tt.possibleDeps, tt.variations, tt.far, tt.reverse) - if g, w := got.String(), fmt.Sprintf("module %q variant %q", "dep", tt.want); g != w { - t.Errorf("findVariant() got = %v, want %v", g, w) + if g, w := got == nil, tt.want == "nil"; g != w { + t.Fatalf("findVariant() got = %v, want %v", got, tt.want) + } + if got != nil { + if g, w := got.String(), fmt.Sprintf("module %q variant %q", "dep", tt.want); g != w { + t.Errorf("findVariant() got = %v, want %v", g, w) + } } }) } From edbdb8c2d31a580ac281e4477dacb786e779c8ae Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Fri, 11 Sep 2020 19:22:27 -0700 Subject: [PATCH 4/4] Relax check in moduleMatchingVariant Pull request #317 made the equality check in moduleMatchingVariant more correct by comparing the variant maps instead of the names. Using only the names effectively ignores any variation with an empty name. Unfortuantely this broke a current user of ReplaceDependenciesIf. Go back to the relaxed check for now. Test: build on a branch with sdk modules Change-Id: I11016c8df7bd91fd022d04fd3033756f54d7fa0b --- context.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/context.go b/context.go index e1bd892..d6bb851 100644 --- a/context.go +++ b/context.go @@ -2733,7 +2733,7 @@ func (c *Context) moduleMatchingVariant(module *moduleInfo, name string) *module } for _, m := range group.modules { - if module.variant.variations.equal(m.moduleOrAliasVariant().variations) { + if module.variant.name == m.moduleOrAliasVariant().name { return m.moduleOrAliasTarget() } }