Fix missing genrule srcs and tools with ALLOW_MISSING_DEPENDENCIES=true

Set the location label for missing srcs and tools to avoid
nonsensical errors when parsing the command.

Test: genrule_test.go
Test: paths_test.go
Test: unbundled branch with missing framework-res module needed by robolectric genrule
Change-Id: I9c1f1cd82a80f048c0e903b8e93910b1ae34b0b1

android/paths.go

@@ -217,13 +217,41 @@ func ExistentPathsForSources(ctx PathContext, paths []string) Paths {
 	return ret
 }
 
-// PathsForModuleSrc returns Paths rooted from the module's local source
+// PathsForModuleSrc returns Paths rooted from the module's local source directory.  It expands globs and references
-// directory
+// to SourceFileProducer modules using the ":name" syntax.  Properties passed as the paths argument must have been
+// annotated with struct tag `android:"path"` so that dependencies on SourceFileProducer modules will have already
+// been handled by the path_properties mutator.  If ctx.Config().AllowMissingDependencies() is true, then any missing
+// SourceFileProducer dependencies will cause the module to be marked as having missing dependencies.
 func PathsForModuleSrc(ctx ModuleContext, paths []string) Paths {
 	return PathsForModuleSrcExcludes(ctx, paths, nil)
 }
 
+// PathsForModuleSrcExcludes returns Paths rooted from the module's local source directory, excluding paths listed in
+// the excludes arguments.  It expands globs and references to SourceFileProducer modules in both paths and excludes
+// using the ":name" syntax.  Properties passed as the paths or excludes argument must have been annotated with struct
+// tag `android:"path"` so that dependencies on SourceFileProducer modules will have already been handled by the
+// path_properties mutator.  If ctx.Config().AllowMissingDependencies() is true, then any missing SourceFileProducer
+// dependencies will cause the module to be marked as having missing dependencies.
 func PathsForModuleSrcExcludes(ctx ModuleContext, paths, excludes []string) Paths {
+	ret, missingDeps := PathsAndMissingDepsForModuleSrcExcludes(ctx, paths, excludes)
+	if ctx.Config().AllowMissingDependencies() {
+		ctx.AddMissingDependencies(missingDeps)
+	} else {
+		for _, m := range missingDeps {
+			ctx.ModuleErrorf(`missing dependency on %q, is the property annotated with android:"path"?`, m)
+		}
+	}
+	return ret
+}
+
+// PathsAndMissingDepsForModuleSrcExcludes returns Paths rooted from the module's local source directory, excluding
+// paths listed in the excludes arguments, and a list of missing dependencies.  It expands globs and references to
+// SourceFileProducer modules in both paths and excludes using the ":name" syntax.  Properties passed as the paths or
+// excludes argument must have been annotated with struct tag `android:"path"` so that dependencies on
+// SourceFileProducer modules will have already been handled by the path_properties mutator.  If
+// ctx.Config().AllowMissingDependencies() is true, then any missing SourceFileProducer dependencies will be returned,
+// and they will NOT cause the module to be marked as having missing dependencies.
+func PathsAndMissingDepsForModuleSrcExcludes(ctx ModuleContext, paths, excludes []string) (Paths, []string) {
 	prefix := pathForModuleSrc(ctx).String()
 
 	var expandedExcludes []string
@@ -231,15 +259,13 @@ func PathsForModuleSrcExcludes(ctx ModuleContext, paths, excludes []string) Path
 		expandedExcludes = make([]string, 0, len(excludes))
 	}
 
+	var missingExcludeDeps []string
+
 	for _, e := range excludes {
 		if m := SrcIsModule(e); m != "" {
 			module := ctx.GetDirectDepWithTag(m, SourceDepTag)
 			if module == nil {
-				if ctx.Config().AllowMissingDependencies() {
+				missingExcludeDeps = append(missingExcludeDeps, m)
-					ctx.AddMissingDependencies([]string{m})
-				} else {
-					ctx.ModuleErrorf(`missing dependency on %q, is the property annotated with android:"path"?`, m)
-				}
 				continue
 			}
 			if srcProducer, ok := module.(SourceFileProducer); ok {
@@ -253,24 +279,23 @@ func PathsForModuleSrcExcludes(ctx ModuleContext, paths, excludes []string) Path
 	}
 
 	if paths == nil {
-		return nil
+		return nil, missingExcludeDeps
 	}
 
+	var missingDeps []string
+
 	expandedSrcFiles := make(Paths, 0, len(paths))
 	for _, s := range paths {
 		srcFiles, err := expandOneSrcPath(ctx, s, expandedExcludes)
 		if depErr, ok := err.(missingDependencyError); ok {
-			if ctx.Config().AllowMissingDependencies() {
+			missingDeps = append(missingDeps, depErr.missingDeps...)
-				ctx.AddMissingDependencies(depErr.missingDeps)
-			} else {
-				ctx.ModuleErrorf(`%s, is the property annotated with android:"path"?`, depErr.Error())
-			}
 		} else if err != nil {
 			reportPathError(ctx, err)
 		}
 		expandedSrcFiles = append(expandedSrcFiles, srcFiles...)
 	}
-	return expandedSrcFiles
+
+	return expandedSrcFiles, append(missingDeps, missingExcludeDeps...)
 }
 
 type missingDependencyError struct {

android/paths_test.go

@@ -714,6 +714,8 @@ type pathForModuleSrcTestModule struct {
 		Exclude_srcs []string `android:"path"`
 
 		Src *string `android:"path"`
+
+		Module_handles_missing_deps bool
 	}
 
 	src string
@@ -733,7 +735,12 @@ func pathForModuleSrcTestModuleFactory() Module {
 }
 
 func (p *pathForModuleSrcTestModule) GenerateAndroidBuildActions(ctx ModuleContext) {
-	srcs := PathsForModuleSrcExcludes(ctx, p.props.Srcs, p.props.Exclude_srcs)
+	var srcs Paths
+	if p.props.Module_handles_missing_deps {
+		srcs, p.missingDeps = PathsAndMissingDepsForModuleSrcExcludes(ctx, p.props.Srcs, p.props.Exclude_srcs)
+	} else {
+		srcs = PathsForModuleSrcExcludes(ctx, p.props.Srcs, p.props.Exclude_srcs)
+	}
 	p.srcs = srcs.Strings()
 
 	for _, src := range srcs {
@@ -748,7 +755,9 @@ func (p *pathForModuleSrcTestModule) GenerateAndroidBuildActions(ctx ModuleConte
 		}
 	}
 
-	p.missingDeps = ctx.GetMissingDependencies()
+	if !p.props.Module_handles_missing_deps {
+		p.missingDeps = ctx.GetMissingDependencies()
+	}
 }
 
 type pathForModuleSrcTestCase struct {
@@ -957,6 +966,13 @@ func TestPathsForModuleSrc_AllowMissingDependencies(t *testing.T) {
 			exclude_srcs: [":b"],
 			src: ":c",
 		}
+
+		test {
+			name: "bar",
+			srcs: [":d"],
+			exclude_srcs: [":e"],
+			module_handles_missing_deps: true,
+		}
 	`
 
 	mockFS := map[string][]byte{
@@ -974,17 +990,26 @@ func TestPathsForModuleSrc_AllowMissingDependencies(t *testing.T) {
 	foo := ctx.ModuleForTests("foo", "").Module().(*pathForModuleSrcTestModule)
 
 	if g, w := foo.missingDeps, []string{"a", "b", "c"}; !reflect.DeepEqual(g, w) {
-		t.Errorf("want missing deps %q, got %q", w, g)
+		t.Errorf("want foo missing deps %q, got %q", w, g)
 	}
 
 	if g, w := foo.srcs, []string{}; !reflect.DeepEqual(g, w) {
-		t.Errorf("want srcs %q, got %q", w, g)
+		t.Errorf("want foo srcs %q, got %q", w, g)
 	}
 
 	if g, w := foo.src, ""; g != w {
-		t.Errorf("want src %q, got %q", w, g)
+		t.Errorf("want foo src %q, got %q", w, g)
 	}
 
+	bar := ctx.ModuleForTests("bar", "").Module().(*pathForModuleSrcTestModule)
+
+	if g, w := bar.missingDeps, []string{"d", "e"}; !reflect.DeepEqual(g, w) {
+		t.Errorf("want bar missing deps %q, got %q", w, g)
+	}
+
+	if g, w := bar.srcs, []string{}; !reflect.DeepEqual(g, w) {
+		t.Errorf("want bar srcs %q, got %q", w, g)
+	}
 }
 
 func ExampleOutputPath_ReplaceExtension() {

genrule/genrule.go

@@ -189,6 +189,8 @@ func (g *Module) GenerateAndroidBuildActions(ctx android.ModuleContext) {
 	}
 
 	if len(g.properties.Tools) > 0 {
+		seenTools := make(map[string]bool)
+
 		ctx.VisitDirectDepsBlueprint(func(module blueprint.Module) {
 			switch tag := ctx.OtherModuleDependencyTag(module).(type) {
 			case hostToolDependencyTag:
@@ -220,11 +222,25 @@ func (g *Module) GenerateAndroidBuildActions(ctx android.ModuleContext) {
 				if path.Valid() {
 					g.deps = append(g.deps, path.Path())
 					addLocationLabel(tag.label, []string{path.Path().String()})
+					seenTools[tag.label] = true
 				} else {
 					ctx.ModuleErrorf("host tool %q missing output file", tool)
 				}
 			}
 		})
+
+		// If AllowMissingDependencies is enabled, the build will not have stopped when
+		// AddFarVariationDependencies was called on a missing tool, which will result in nonsensical
+		// "cmd: unknown location label ..." errors later.  Add a dummy file to the local label.  The
+		// command that uses this dummy file will never be executed because the rule will be replaced with
+		// an android.Error rule reporting the missing dependencies.
+		if ctx.Config().AllowMissingDependencies() {
+			for _, tool := range g.properties.Tools {
+				if !seenTools[tool] {
+					addLocationLabel(tool, []string{"***missing tool " + tool + "***"})
+				}
+			}
+		}
 	}
 
 	if ctx.Failed() {
@@ -239,9 +255,24 @@ func (g *Module) GenerateAndroidBuildActions(ctx android.ModuleContext) {
 
 	var srcFiles android.Paths
 	for _, in := range g.properties.Srcs {
-		paths := android.PathsForModuleSrcExcludes(ctx, []string{in}, g.properties.Exclude_srcs)
+		paths, missingDeps := android.PathsAndMissingDepsForModuleSrcExcludes(ctx, []string{in}, g.properties.Exclude_srcs)
-		srcFiles = append(srcFiles, paths...)
+		if len(missingDeps) > 0 {
-		addLocationLabel(in, paths.Strings())
+			if !ctx.Config().AllowMissingDependencies() {
+				panic(fmt.Errorf("should never get here, the missing dependencies %q should have been reported in DepsMutator",
+					missingDeps))
+			}
+
+			// If AllowMissingDependencies is enabled, the build will not have stopped when
+			// the dependency was added on a missing SourceFileProducer module, which will result in nonsensical
+			// "cmd: label ":..." has no files" errors later.  Add a dummy file to the local label.  The
+			// command that uses this dummy file will never be executed because the rule will be replaced with
+			// an android.Error rule reporting the missing dependencies.
+			ctx.AddMissingDependencies(missingDeps)
+			addLocationLabel(in, []string{"***missing srcs " + in + "***"})
+		} else {
+			srcFiles = append(srcFiles, paths...)
+			addLocationLabel(in, paths.Strings())
+		}
 	}
 
 	task := g.taskGenerator(ctx, String(g.properties.Cmd), srcFiles)

genrule/genrule_test.go

@@ -17,11 +17,13 @@ package genrule
 import (
 	"io/ioutil"
 	"os"
+	"reflect"
 	"strings"
 	"testing"
 
 	"android/soong/android"
-	"reflect"
+
+	"github.com/google/blueprint/proptools"
 )
 
 var buildDir string
@@ -123,6 +125,8 @@ func TestGenruleCmd(t *testing.T) {
 		name string
 		prop string
 
+		allowMissingDependencies bool
+
 		err    string
 		expect string
 	}{
@@ -425,6 +429,30 @@ func TestGenruleCmd(t *testing.T) {
 			`,
 			err: "must have at least one output file",
 		},
+		{
+			name: "srcs allow missing dependencies",
+			prop: `
+				srcs: [":missing"],
+				out: ["out"],
+				cmd: "cat $(location :missing) > $(out)",
+			`,
+
+			allowMissingDependencies: true,
+
+			expect: "cat ***missing srcs :missing*** > __SBOX_OUT_FILES__",
+		},
+		{
+			name: "tool allow missing dependencies",
+			prop: `
+				tools: [":missing"],
+				out: ["out"],
+				cmd: "$(location :missing) > $(out)",
+			`,
+
+			allowMissingDependencies: true,
+
+			expect: "***missing tool :missing*** > __SBOX_OUT_FILES__",
+		},
 	}
 
 	for _, test := range testcases {
@@ -435,7 +463,10 @@ func TestGenruleCmd(t *testing.T) {
 			bp += test.prop
 			bp += "}\n"
 
+			config.TestProductVariables.Allow_missing_dependencies = proptools.BoolPtr(test.allowMissingDependencies)
+
 			ctx := testContext(config, bp, nil)
+			ctx.SetAllowMissingDependencies(test.allowMissingDependencies)
 
 			_, errs := ctx.ParseFileList(".", []string{"Android.bp"})
 			if errs == nil {
@@ -460,8 +491,8 @@ func TestGenruleCmd(t *testing.T) {
 			}
 
 			gen := ctx.ModuleForTests("gen", "").Module().(*Module)
-			if gen.rawCommand != "'"+test.expect+"'" {
+			if g, w := gen.rawCommand, "'"+test.expect+"'"; w != g {
-				t.Errorf("want %q, got %q", test.expect, gen.rawCommand)
+				t.Errorf("want %q, got %q", w, g)
 			}
 		})
 	}