Merge "prune out empty bazel depsets" am: e136efdcfa

Original change: https://android-review.googlesource.com/c/platform/build/soong/+/2404275

Change-Id: If6f62a507931557dd72c8d263d4a574fb5cfc533
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

android/bazel_handler.go

@@ -1135,10 +1135,11 @@ func (c *bazelSingleton) GenerateBuildActions(ctx SingletonContext) {
 		// the 'bazel' package, which cannot depend on 'android' package where ctx is defined,
 		// because this would cause circular dependency. So, until we move aquery processing
 		// to the 'android' package, we need to handle special cases here.
-		if buildStatement.Mnemonic == "FileWrite" || buildStatement.Mnemonic == "SourceSymlinkManifest" {
+		switch buildStatement.Mnemonic {
+		case "FileWrite", "SourceSymlinkManifest":
 			out := PathForBazelOut(ctx, buildStatement.OutputPaths[0])
 			WriteFileRuleVerbatim(ctx, out, buildStatement.FileContents)
-		} else if buildStatement.Mnemonic == "SymlinkTree" {
+		case "SymlinkTree":
 			// build-runfiles arguments are the manifest file and the target directory
 			// where it creates the symlink tree according to this manifest (and then
 			// writes the MANIFEST file to it).
@@ -1157,7 +1158,7 @@ func (c *bazelSingleton) GenerateBuildActions(ctx SingletonContext) {
 					"outDir": outDir,
 				},
 			})
-		} else {
+		default:
 			panic(fmt.Sprintf("unhandled build statement: %v", buildStatement))
 		}
 	}

bazel/aquery.go

@@ -118,12 +118,11 @@ type BuildStatement struct {
 // A helper type for aquery processing which facilitates retrieval of path IDs from their
 // less readable Bazel structures (depset and path fragment).
 type aqueryArtifactHandler struct {
-	// Switches to true if any depset contains only `bazelToolsDependencySentinel`
-	bazelToolsDependencySentinelNeeded bool
 	// Maps depset id to AqueryDepset, a representation of depset which is
 	// post-processed for middleman artifact handling, unhandled artifact
 	// dropping, content hashing, etc.
 	depsetIdToAqueryDepset map[depsetId]AqueryDepset
+	emptyDepsetIds         map[depsetId]struct{}
 	// Maps content hash to AqueryDepset.
 	depsetHashToAqueryDepset map[string]AqueryDepset
 
@@ -145,9 +144,6 @@ var templateActionOverriddenTokens = map[string]string{
 // The file name of py3wrapper.sh, which is used by py_binary targets.
 const py3wrapperFileName = "/py3wrapper.sh"
 
-// A file to be put into depsets that are otherwise empty
-const bazelToolsDependencySentinel = "BAZEL_TOOLS_DEPENDENCY_SENTINEL"
-
 func indexBy[K comparable, V any](values []V, keyFn func(v V) K) map[K]V {
 	m := map[K]V{}
 	for _, v := range values {
@@ -192,6 +188,7 @@ func newAqueryHandler(aqueryResult actionGraphContainer) (*aqueryArtifactHandler
 		depsetIdToAqueryDepset:         map[depsetId]AqueryDepset{},
 		depsetHashToAqueryDepset:       map[string]AqueryDepset{},
 		depsetHashToArtifactPathsCache: map[string][]string{},
+		emptyDepsetIds:                 make(map[depsetId]struct{}, 0),
 		artifactIdToPath:               artifactIdToPath,
 	}
 
@@ -208,16 +205,16 @@ func newAqueryHandler(aqueryResult actionGraphContainer) (*aqueryArtifactHandler
 
 // Ensures that the handler's depsetIdToAqueryDepset map contains an entry for the given
 // depset.
-func (a *aqueryArtifactHandler) populateDepsetMaps(depset depSetOfFiles, middlemanIdToDepsetIds map[artifactId][]depsetId, depsetIdToDepset map[depsetId]depSetOfFiles) (AqueryDepset, error) {
+func (a *aqueryArtifactHandler) populateDepsetMaps(depset depSetOfFiles, middlemanIdToDepsetIds map[artifactId][]depsetId, depsetIdToDepset map[depsetId]depSetOfFiles) (*AqueryDepset, error) {
 	if aqueryDepset, containsDepset := a.depsetIdToAqueryDepset[depset.Id]; containsDepset {
-		return aqueryDepset, nil
+		return &aqueryDepset, nil
 	}
 	transitiveDepsetIds := depset.TransitiveDepSetIds
 	var directArtifactPaths []string
 	for _, artifactId := range depset.DirectArtifactIds {
 		path, pathExists := a.artifactIdToPath[artifactId]
 		if !pathExists {
-			return AqueryDepset{}, fmt.Errorf("undefined input artifactId %d", artifactId)
+			return nil, fmt.Errorf("undefined input artifactId %d", artifactId)
 		}
 		// Filter out any inputs which are universally dropped, and swap middleman
 		// artifacts with their corresponding depsets.
@@ -226,6 +223,7 @@ func (a *aqueryArtifactHandler) populateDepsetMaps(depset depSetOfFiles, middlem
 			transitiveDepsetIds = append(transitiveDepsetIds, depsetsToUse...)
 		} else if strings.HasSuffix(path, py3wrapperFileName) ||
 			strings.HasPrefix(path, "../bazel_tools") {
+			continue
 			// Drop these artifacts.
 			// See go/python-binary-host-mixed-build for more details.
 			// 1) Drop py3wrapper.sh, just use python binary, the launcher script generated by the
@@ -241,20 +239,23 @@ func (a *aqueryArtifactHandler) populateDepsetMaps(depset depSetOfFiles, middlem
 	for _, childDepsetId := range transitiveDepsetIds {
 		childDepset, exists := depsetIdToDepset[childDepsetId]
 		if !exists {
-			return AqueryDepset{}, fmt.Errorf("undefined input depsetId %d (referenced by depsetId %d)", childDepsetId, depset.Id)
+			if _, empty := a.emptyDepsetIds[childDepsetId]; empty {
+				continue
+			} else {
+				return nil, fmt.Errorf("undefined input depsetId %d (referenced by depsetId %d)", childDepsetId, depset.Id)
 			}
-		childAqueryDepset, err := a.populateDepsetMaps(childDepset, middlemanIdToDepsetIds, depsetIdToDepset)
-		if err != nil {
-			return AqueryDepset{}, err
 		}
+		if childAqueryDepset, err := a.populateDepsetMaps(childDepset, middlemanIdToDepsetIds, depsetIdToDepset); err != nil {
+			return nil, err
+		} else if childAqueryDepset == nil {
+			continue
+		} else {
 			childDepsetHashes = append(childDepsetHashes, childAqueryDepset.ContentHash)
 		}
+	}
 	if len(directArtifactPaths) == 0 && len(childDepsetHashes) == 0 {
-		// We could omit this depset altogether but that requires cleanup on
-		// transitive dependents.
-		// As a simpler alternative, we use this sentinel file as a dependency.
-		directArtifactPaths = append(directArtifactPaths, bazelToolsDependencySentinel)
-		a.bazelToolsDependencySentinelNeeded = true
+		a.emptyDepsetIds[depset.Id] = struct{}{}
+		return nil, nil
 	}
 	aqueryDepset := AqueryDepset{
 		ContentHash:            depsetContentHash(directArtifactPaths, childDepsetHashes),
@@ -263,7 +264,7 @@ func (a *aqueryArtifactHandler) populateDepsetMaps(depset depSetOfFiles, middlem
 	}
 	a.depsetIdToAqueryDepset[depset.Id] = aqueryDepset
 	a.depsetHashToAqueryDepset[aqueryDepset.ContentHash] = aqueryDepset
-	return aqueryDepset, nil
+	return &aqueryDepset, nil
 }
 
 // getInputPaths flattens the depsets of the given IDs and returns all transitive
@@ -392,14 +393,6 @@ func AqueryBuildStatements(aqueryJsonProto []byte) ([]BuildStatement, []AqueryDe
 	}
 
 	var buildStatements []BuildStatement
-	if aqueryHandler.bazelToolsDependencySentinelNeeded {
-		buildStatements = append(buildStatements, BuildStatement{
-			Command:     fmt.Sprintf("touch '%s'", bazelToolsDependencySentinel),
-			OutputPaths: []string{bazelToolsDependencySentinel},
-			Mnemonic:    bazelToolsDependencySentinel,
-		})
-	}
-
 	for _, actionEntry := range aqueryResult.Actions {
 		if shouldSkipAction(actionEntry) {
 			continue
@@ -484,7 +477,9 @@ func (a *aqueryArtifactHandler) depsetContentHashes(inputDepsetIds []depsetId) (
 	var hashes []string
 	for _, depsetId := range inputDepsetIds {
 		if aqueryDepset, exists := a.depsetIdToAqueryDepset[depsetId]; !exists {
-			return nil, fmt.Errorf("undefined input depsetId %d", depsetId)
+			if _, empty := a.emptyDepsetIds[depsetId]; !empty {
+				return nil, fmt.Errorf("undefined (not even empty) input depsetId %d", depsetId)
+			}
 		} else {
 			hashes = append(hashes, aqueryDepset.ContentHash)
 		}

bazel/aquery_test.go

@@ -227,7 +227,7 @@ func TestInvalidInputDepsetIdFromAction(t *testing.T) {
 		return
 	}
 	_, _, err = AqueryBuildStatements(data)
-	assertError(t, err, "undefined input depsetId 2")
+	assertError(t, err, "undefined (not even empty) input depsetId 2")
 }
 
 func TestInvalidInputDepsetIdFromDepset(t *testing.T) {
@@ -584,13 +584,18 @@ func TestBazelOutRemovalFromInputDepsets(t *testing.T) {
    { "id": 60, "label": ".."}
  ]
 }`
+	/* depsets
+	       1111  2222
+	       /  \   |
+	../dep2    ../bazel_tools/dep1
+	*/
 	data, err := JsonToActionGraphContainer(inputString)
 	if err != nil {
 		t.Error(err)
 		return
 	}
 	actualBuildStatements, actualDepsets, _ := AqueryBuildStatements(data)
-	if len(actualDepsets) != 2 {
+	if len(actualDepsets) != 1 {
 		t.Errorf("expected 1 depset but found %#v", actualDepsets)
 		return
 	}
@@ -624,6 +629,82 @@ func TestBazelOutRemovalFromInputDepsets(t *testing.T) {
 	}
 }
 
+func TestBazelOutRemovalFromTransitiveInputDepsets(t *testing.T) {
+	const inputString = `{
+ "artifacts": [
+   { "id": 1, "path_fragment_id": 10 },
+   { "id": 2, "path_fragment_id": 20 },
+   { "id": 3, "path_fragment_id": 30 }],
+ "dep_set_of_files": [{
+   "id": 1111,
+   "transitive_dep_set_ids": [2222]
+ }, {
+   "id": 2222,
+   "direct_artifact_ids": [3]
+ }, {
+   "id": 3333,
+   "direct_artifact_ids": [3]
+ }, {
+   "id": 4444,
+   "transitive_dep_set_ids": [3333]
+ }],
+ "actions": [{
+   "target_id": 100,
+   "action_key": "x",
+   "input_dep_set_ids": [1111, 4444],
+   "mnemonic": "x",
+   "arguments": ["bogus", "command"],
+   "output_ids": [2],
+   "primary_output_id": 1
+ }],
+ "path_fragments": [
+   { "id": 10, "label": "input" },
+   { "id": 20, "label": "output" },
+   { "id": 30, "label": "dep", "parent_id": 50 },
+   { "id": 50, "label": "bazel_tools", "parent_id": 60 },
+   { "id": 60, "label": ".."}
+ ]
+}`
+	/* depsets
+	    1111    4444
+	     ||      ||
+	    2222    3333
+	      |      |
+	../bazel_tools/dep
+	Note: in dep_set_of_files:
+	  1111 appears BEFORE its dependency,2222 while
+	  4444 appears AFTER its dependency 3333
+	and this test shows that that order doesn't affect empty depset pruning
+	*/
+	data, err := JsonToActionGraphContainer(inputString)
+	if err != nil {
+		t.Error(err)
+		return
+	}
+	actualBuildStatements, actualDepsets, _ := AqueryBuildStatements(data)
+	if len(actualDepsets) != 0 {
+		t.Errorf("expected 0 depsets but found %#v", actualDepsets)
+		return
+	}
+
+	expectedBuildStatement := BuildStatement{
+		Command:     "bogus command",
+		OutputPaths: []string{"output"},
+		Mnemonic:    "x",
+	}
+	buildStatementFound := false
+	for _, actualBuildStatement := range actualBuildStatements {
+		if buildStatementEquals(actualBuildStatement, expectedBuildStatement) == "" {
+			buildStatementFound = true
+			break
+		}
+	}
+	if !buildStatementFound {
+		t.Errorf("expected but missing %#v in %#v", expectedBuildStatement, actualBuildStatements)
+		return
+	}
+}
+
 func TestMiddlemenAction(t *testing.T) {
 	const inputString = `
 {