From 08bd504b625b18cf8f5028854de5bd1996d9c1d0 Mon Sep 17 00:00:00 2001 From: Sam Delmerico Date: Tue, 21 Feb 2023 12:10:27 -0500 Subject: [PATCH] ignore bp files from PRODUCT_SOURCE_ROOT_DIRS Soong analyzes the entire source tree even though not every lunch target needs to know about every module. For example, OEM sources can be ignored for cuttlefish products. This functionality allows blueprint to ignore a list of undesired directories. Bug: 269457150 Change-Id: Icbbf8f3b66813ad639a7ebd27b1a3ec153cbf269 --- context.go | 113 ++++++++++++++++-- context_test.go | 290 ++++++++++++++++++++++++++++++++++++++++++++++ name_interface.go | 43 ++++++- parser/ast.go | 24 ++++ 4 files changed, 460 insertions(+), 10 deletions(-) diff --git a/context.go b/context.go index 56d4deb..2592289 100644 --- a/context.go +++ b/context.go @@ -142,6 +142,45 @@ type Context struct { // String values that can be used to gate build graph traversal includeTags *IncludeTags + + sourceRootDirs *SourceRootDirs +} + +// A container for String keys. The keys can be used to gate build graph traversal +type SourceRootDirs struct { + dirs []string +} + +func (dirs *SourceRootDirs) Add(names ...string) { + dirs.dirs = append(dirs.dirs, names...) +} + +func (dirs *SourceRootDirs) SourceRootDirAllowed(path string) (bool, string) { + sort.Slice(dirs.dirs, func(i, j int) bool { + return len(dirs.dirs[i]) < len(dirs.dirs[j]) + }) + last := len(dirs.dirs) + for i := range dirs.dirs { + // iterate from longest paths (most specific) + prefix := dirs.dirs[last-i-1] + disallowedPrefix := false + if len(prefix) >= 1 && prefix[0] == '-' { + prefix = prefix[1:] + disallowedPrefix = true + } + if strings.HasPrefix(path, prefix) { + if disallowedPrefix { + return false, prefix + } else { + return true, prefix + } + } + } + return true, "" +} + +func (c *Context) AddSourceRootDirs(dirs ...string) { + c.sourceRootDirs.Add(dirs...) } // A container for String keys. The keys can be used to gate build graph traversal @@ -427,6 +466,7 @@ func newContext() *Context { fs: pathtools.OsFs, finishedMutators: make(map[*mutatorInfo]bool), includeTags: &IncludeTags{}, + sourceRootDirs: &SourceRootDirs{}, outDir: nil, requiredNinjaMajor: 1, requiredNinjaMinor: 7, @@ -968,15 +1008,25 @@ func (c *Context) ParseBlueprintsFiles(rootFile string, return c.ParseFileList(baseDir, pathsToParse, config) } +type shouldVisitFileInfo struct { + shouldVisitFile bool + skippedModules []string + reasonForSkip string + errs []error +} + // Returns a boolean for whether this file should be analyzed // Evaluates to true if the file either // 1. does not contain a blueprint_package_includes // 2. contains a blueprint_package_includes and all requested tags are set // This should be processed before adding any modules to the build graph -func shouldVisitFile(c *Context, file *parser.File) (bool, []error) { +func shouldVisitFile(c *Context, file *parser.File) shouldVisitFileInfo { + skippedModules := []string{} + var blueprintPackageIncludes *PackageIncludes for _, def := range file.Defs { switch def := def.(type) { case *parser.Module: + skippedModules = append(skippedModules, def.Name()) if def.Type != "blueprint_package_includes" { continue } @@ -984,14 +1034,43 @@ func shouldVisitFile(c *Context, file *parser.File) (bool, []error) { if len(errs) > 0 { // This file contains errors in blueprint_package_includes // Visit anyways so that we can report errors on other modules in the file - return true, errs + return shouldVisitFileInfo{ + shouldVisitFile: true, + errs: errs, + } } logicModule, _ := c.cloneLogicModule(module) - pi := logicModule.(*PackageIncludes) - return pi.MatchesIncludeTags(c), []error{} + blueprintPackageIncludes = logicModule.(*PackageIncludes) } } - return true, []error{} + + if blueprintPackageIncludes != nil { + packageMatches := blueprintPackageIncludes.MatchesIncludeTags(c) + if !packageMatches { + return shouldVisitFileInfo{ + shouldVisitFile: false, + skippedModules: skippedModules, + reasonForSkip: fmt.Sprintf( + "module is defined in %q which contains a blueprint_package_includes module with unsatisfied tags", + file.Name, + ), + } + } + } + + shouldVisit, invalidatingPrefix := c.sourceRootDirs.SourceRootDirAllowed(file.Name) + if !shouldVisit { + return shouldVisitFileInfo{ + shouldVisitFile: shouldVisit, + skippedModules: skippedModules, + reasonForSkip: fmt.Sprintf( + "%q is a descendant of %q, and that path prefix was not included in PRODUCT_SOURCE_ROOT_DIRS", + file.Name, + invalidatingPrefix, + ), + } + } + return shouldVisitFileInfo{shouldVisitFile: true} } func (c *Context) ParseFileList(rootDir string, filePaths []string, @@ -1009,9 +1088,15 @@ func (c *Context) ParseFileList(rootDir string, filePaths []string, added chan<- struct{} } + type newSkipInfo struct { + shouldVisitFileInfo + file string + } + moduleCh := make(chan newModuleInfo) errsCh := make(chan []error) doneCh := make(chan struct{}) + skipCh := make(chan newSkipInfo) var numErrs uint32 var numGoroutines int32 @@ -1046,12 +1131,17 @@ func (c *Context) ParseFileList(rootDir string, filePaths []string, } return nil } - shouldVisit, errs := shouldVisitFile(c, file) + shouldVisitInfo := shouldVisitFile(c, file) + errs := shouldVisitInfo.errs if len(errs) > 0 { atomic.AddUint32(&numErrs, uint32(len(errs))) errsCh <- errs } - if !shouldVisit { + if !shouldVisitInfo.shouldVisitFile { + skipCh <- newSkipInfo{ + file: file.Name, + shouldVisitFileInfo: shouldVisitInfo, + } // TODO: Write a file that lists the skipped bp files return } @@ -1108,6 +1198,14 @@ loop: if n == 0 { break loop } + case skipped := <-skipCh: + nctx := newNamespaceContextFromFilename(skipped.file) + for _, name := range skipped.skippedModules { + c.nameInterface.NewSkippedModule(nctx, name, SkippedModuleInfo{ + filename: skipped.file, + reason: skipped.reasonForSkip, + }) + } } } @@ -3491,7 +3589,6 @@ func (c *Context) discoveredMissingDependencies(module *moduleInfo, depName stri func (c *Context) missingDependencyError(module *moduleInfo, depName string) (errs error) { err := c.nameInterface.MissingDependencyError(module.Name(), module.namespace(), depName) - return &BlueprintError{ Err: err, Pos: module.pos, diff --git a/context_test.go b/context_test.go index 2c0a6c0..2ea7801 100644 --- a/context_test.go +++ b/context_test.go @@ -1272,3 +1272,293 @@ func TestDeduplicateOrderOnlyDeps(t *testing.T) { }) } } + +func TestSourceRootDirAllowed(t *testing.T) { + type pathCase struct { + path string + decidingPrefix string + allowed bool + } + testcases := []struct { + desc string + rootDirs []string + pathCases []pathCase + }{ + { + desc: "simple case", + rootDirs: []string{ + "a", + "b/c/d", + "-c", + "-d/c/a", + "c/some_single_file", + }, + pathCases: []pathCase{ + { + path: "a", + decidingPrefix: "a", + allowed: true, + }, + { + path: "a/b/c", + decidingPrefix: "a", + allowed: true, + }, + { + path: "b", + decidingPrefix: "", + allowed: true, + }, + { + path: "b/c/d/a", + decidingPrefix: "b/c/d", + allowed: true, + }, + { + path: "c", + decidingPrefix: "c", + allowed: false, + }, + { + path: "c/a/b", + decidingPrefix: "c", + allowed: false, + }, + { + path: "c/some_single_file", + decidingPrefix: "c/some_single_file", + allowed: true, + }, + { + path: "d/c/a/abc", + decidingPrefix: "d/c/a", + allowed: false, + }, + }, + }, + { + desc: "root directory order matters", + rootDirs: []string{ + "-a", + "a/c/some_allowed_file", + "a/b/d/some_allowed_file", + "a/b", + "a/c", + "-a/b/d", + }, + pathCases: []pathCase{ + { + path: "a", + decidingPrefix: "a", + allowed: false, + }, + { + path: "a/some_disallowed_file", + decidingPrefix: "a", + allowed: false, + }, + { + path: "a/c/some_allowed_file", + decidingPrefix: "a/c/some_allowed_file", + allowed: true, + }, + { + path: "a/b/d/some_allowed_file", + decidingPrefix: "a/b/d/some_allowed_file", + allowed: true, + }, + { + path: "a/b/c", + decidingPrefix: "a/b", + allowed: true, + }, + { + path: "a/b/c/some_allowed_file", + decidingPrefix: "a/b", + allowed: true, + }, + { + path: "a/b/d", + decidingPrefix: "a/b/d", + allowed: false, + }, + }, + }, + } + for _, tc := range testcases { + dirs := SourceRootDirs{} + dirs.Add(tc.rootDirs...) + for _, pc := range tc.pathCases { + t.Run(fmt.Sprintf("%s: %s", tc.desc, pc.path), func(t *testing.T) { + allowed, decidingPrefix := dirs.SourceRootDirAllowed(pc.path) + if allowed != pc.allowed { + if pc.allowed { + t.Errorf("expected path %q to be allowed, but was not; root allowlist: %q", pc.path, tc.rootDirs) + } else { + t.Errorf("path %q was allowed unexpectedly; root allowlist: %q", pc.path, tc.rootDirs) + } + } + if decidingPrefix != pc.decidingPrefix { + t.Errorf("expected decidingPrefix to be %q, but got %q", pc.decidingPrefix, decidingPrefix) + } + }) + } + } +} + +func TestSourceRootDirs(t *testing.T) { + root_foo_bp := ` + foo_module { + name: "foo", + deps: ["foo_dir1", "foo_dir_ignored_special_case"], + } + ` + dir1_foo_bp := ` + foo_module { + name: "foo_dir1", + deps: ["foo_dir_ignored"], + } + ` + dir_ignored_foo_bp := ` + foo_module { + name: "foo_dir_ignored", + } + ` + dir_ignored_special_case_foo_bp := ` + foo_module { + name: "foo_dir_ignored_special_case", + } + ` + mockFs := map[string][]byte{ + "Android.bp": []byte(root_foo_bp), + "dir1/Android.bp": []byte(dir1_foo_bp), + "dir_ignored/Android.bp": []byte(dir_ignored_foo_bp), + "dir_ignored/special_case/Android.bp": []byte(dir_ignored_special_case_foo_bp), + } + fileList := []string{} + for f := range mockFs { + fileList = append(fileList, f) + } + testCases := []struct { + sourceRootDirs []string + expectedModuleDefs []string + unexpectedModuleDefs []string + expectedErrs []string + }{ + { + sourceRootDirs: []string{}, + expectedModuleDefs: []string{ + "foo", + "foo_dir1", + "foo_dir_ignored", + "foo_dir_ignored_special_case", + }, + }, + { + sourceRootDirs: []string{"-", ""}, + unexpectedModuleDefs: []string{ + "foo", + "foo_dir1", + "foo_dir_ignored", + "foo_dir_ignored_special_case", + }, + }, + { + sourceRootDirs: []string{"-"}, + unexpectedModuleDefs: []string{ + "foo", + "foo_dir1", + "foo_dir_ignored", + "foo_dir_ignored_special_case", + }, + }, + { + sourceRootDirs: []string{"dir1"}, + expectedModuleDefs: []string{ + "foo", + "foo_dir1", + "foo_dir_ignored", + "foo_dir_ignored_special_case", + }, + }, + { + sourceRootDirs: []string{"-dir1"}, + expectedModuleDefs: []string{ + "foo", + "foo_dir_ignored", + "foo_dir_ignored_special_case", + }, + unexpectedModuleDefs: []string{ + "foo_dir1", + }, + expectedErrs: []string{ + "Android.bp:2:2: \"foo\" depends on undefined module \"foo_dir1\"", + }, + }, + { + sourceRootDirs: []string{"-", "dir1"}, + expectedModuleDefs: []string{ + "foo_dir1", + }, + unexpectedModuleDefs: []string{ + "foo", + "foo_dir_ignored", + "foo_dir_ignored_special_case", + }, + expectedErrs: []string{ + "dir1/Android.bp:2:2: \"foo_dir1\" depends on undefined module \"foo_dir_ignored\"", + }, + }, + { + sourceRootDirs: []string{"-", "dir1", "dir_ignored/special_case/Android.bp"}, + expectedModuleDefs: []string{ + "foo_dir1", + "foo_dir_ignored_special_case", + }, + unexpectedModuleDefs: []string{ + "foo", + "foo_dir_ignored", + }, + expectedErrs: []string{ + "dir1/Android.bp:2:2: \"foo_dir1\" depends on undefined module \"foo_dir_ignored\"", + }, + }, + } + for _, tc := range testCases { + t.Run(fmt.Sprintf(`source root dirs are %q`, tc.sourceRootDirs), func(t *testing.T) { + ctx := NewContext() + ctx.MockFileSystem(mockFs) + ctx.RegisterModuleType("foo_module", newFooModule) + ctx.RegisterBottomUpMutator("deps", depsMutator) + ctx.AddSourceRootDirs(tc.sourceRootDirs...) + RegisterPackageIncludesModuleType(ctx) + ctx.ParseFileList(".", fileList, nil) + _, actualErrs := ctx.ResolveDependencies(nil) + + stringErrs := []string(nil) + for i := range actualErrs { + stringErrs = append(stringErrs, fmt.Sprint(actualErrs[i])) + } + if !reflect.DeepEqual(tc.expectedErrs, stringErrs) { + t.Errorf("expected to find errors %v; got %v", tc.expectedErrs, stringErrs) + } + for _, modName := range tc.expectedModuleDefs { + allMods := ctx.moduleGroupFromName(modName, nil) + if allMods == nil || len(allMods.modules) != 1 { + mods := modulesOrAliases{} + if allMods != nil { + mods = allMods.modules + } + t.Errorf("expected to find one definition for module %q, but got %v", modName, mods) + } + } + + for _, modName := range tc.unexpectedModuleDefs { + allMods := ctx.moduleGroupFromName(modName, nil) + if allMods != nil { + t.Errorf("expected to find no definitions for module %q, but got %v", modName, allMods.modules) + } + } + }) + } +} diff --git a/name_interface.go b/name_interface.go index 5e7e16e..c675ded 100644 --- a/name_interface.go +++ b/name_interface.go @@ -16,7 +16,9 @@ package blueprint import ( "fmt" + "os" "sort" + "strings" ) // This file exposes the logic of locating a module via a query string, to enable @@ -54,6 +56,9 @@ type NameInterface interface { // Gets called when a new module is created NewModule(ctx NamespaceContext, group ModuleGroup, module Module) (namespace Namespace, err []error) + // Gets called when a module was pruned from the build tree by SourceRootDirs + NewSkippedModule(ctx NamespaceContext, name string, skipInfo SkippedModuleInfo) + // Finds the module with the given name ModuleFromName(moduleName string, namespace Namespace) (group ModuleGroup, found bool) @@ -88,18 +93,29 @@ func newNamespaceContext(moduleInfo *moduleInfo) (ctx NamespaceContext) { return &namespaceContextImpl{moduleInfo.pos.Filename} } +func newNamespaceContextFromFilename(filename string) NamespaceContext { + return &namespaceContextImpl{filename} +} + func (ctx *namespaceContextImpl) ModulePath() string { return ctx.modulePath } +type SkippedModuleInfo struct { + filename string + reason string +} + // a SimpleNameInterface just stores all modules in a map based on name type SimpleNameInterface struct { - modules map[string]ModuleGroup + modules map[string]ModuleGroup + skippedModules map[string][]SkippedModuleInfo } func NewSimpleNameInterface() *SimpleNameInterface { return &SimpleNameInterface{ - modules: make(map[string]ModuleGroup), + modules: make(map[string]ModuleGroup), + skippedModules: make(map[string][]SkippedModuleInfo), } } @@ -118,8 +134,31 @@ func (s *SimpleNameInterface) NewModule(ctx NamespaceContext, group ModuleGroup, return nil, []error{} } +func (s *SimpleNameInterface) NewSkippedModule(ctx NamespaceContext, name string, info SkippedModuleInfo) { + if name == "" { + return + } + s.skippedModules[name] = append(s.skippedModules[name], info) +} + func (s *SimpleNameInterface) ModuleFromName(moduleName string, namespace Namespace) (group ModuleGroup, found bool) { group, found = s.modules[moduleName] + skipInfos, skipped := s.skippedModules[moduleName] + if skipped { + filesFound := make([]string, 0, len(skipInfos)) + reasons := make([]string, 0, len(skipInfos)) + for _, info := range skipInfos { + filesFound = append(filesFound, info.filename) + reasons = append(reasons, info.reason) + } + fmt.Fprintf( + os.Stderr, + "module %q was defined in files(s) [%v], but was skipped for reason(s) [%v]\n", + moduleName, + strings.Join(filesFound, ", "), + strings.Join(reasons, "; "), + ) + } return group, found } diff --git a/parser/ast.go b/parser/ast.go index fee2ec2..ea774e6 100644 --- a/parser/ast.go +++ b/parser/ast.go @@ -60,6 +60,8 @@ type Module struct { Type string TypePos scanner.Position Map + //TODO(delmerico) make this a private field once ag/21588220 lands + Name__internal_only *string } func (m *Module) Copy() *Module { @@ -86,6 +88,28 @@ func (m *Module) definitionTag() {} func (m *Module) Pos() scanner.Position { return m.TypePos } func (m *Module) End() scanner.Position { return m.Map.End() } +func (m *Module) Name() string { + if m.Name__internal_only != nil { + return *m.Name__internal_only + } + for _, prop := range m.Properties { + if prop.Name == "name" { + if stringProp, ok := prop.Value.(*String); ok { + name := stringProp.Value + m.Name__internal_only = &name + } else { + name := prop.Value.String() + m.Name__internal_only = &name + } + } + } + if m.Name__internal_only == nil { + name := "" + m.Name__internal_only = &name + } + return *m.Name__internal_only +} + // A Property is a name: value pair within a Map, which may be a top level Module. type Property struct { Name string