From c4fb13338001987a669e4353d68343dded38482a Mon Sep 17 00:00:00 2001 From: Chris Parsons Date: Tue, 18 May 2021 12:31:25 -0400 Subject: [PATCH] Support middleman actions in mixed builds This allows support of generated hdrs / srcs in mixed builds. Test: Manually verified that libc_bionic_ndk passes compilation (failed previously due to missing generated heaer) Test: bp2build and mixed_libc CI scripts Test: New aquery test Change-Id: I88e359a4bd9eba383c207d5cf812272725ff0a3d --- android/bazel.go | 2 +- bazel/aquery.go | 165 ++++++++++++++++++++++++++----------------- bazel/aquery_test.go | 87 +++++++++++++++++++++++ 3 files changed, 189 insertions(+), 65 deletions(-) diff --git a/android/bazel.go b/android/bazel.go index 4f8392d1f..f3e920a16 100644 --- a/android/bazel.go +++ b/android/bazel.go @@ -223,7 +223,7 @@ var ( // Per-module denylist to opt modules out of mixed builds. Such modules will // still be generated via bp2build. mixedBuildsDisabledList = []string{ - "libc_bionic_ndk", // cparsons@, http://b/183213331, Handle generated headers in mixed builds. + "libc_bionic_ndk", // cparsons@ cc_library_static, depends on //bionic/libc:libsystemproperties "libc_common", // cparsons@ cc_library_static, depends on //bionic/libc:libc_nopthread "libc_common_static", // cparsons@ cc_library_static, depends on //bionic/libc:libc_common "libc_common_shared", // cparsons@ cc_library_static, depends on //bionic/libc:libc_common diff --git a/bazel/aquery.go b/bazel/aquery.go index 555f1dcb4..bda3a1ae6 100644 --- a/bazel/aquery.go +++ b/bazel/aquery.go @@ -81,23 +81,30 @@ type BuildStatement struct { Mnemonic string } -// AqueryBuildStatements returns an array of BuildStatements which should be registered (and output -// to a ninja file) to correspond one-to-one with the given action graph json proto (from a bazel -// aquery invocation). -func AqueryBuildStatements(aqueryJsonProto []byte) ([]BuildStatement, error) { - buildStatements := []BuildStatement{} - - var aqueryResult actionGraphContainer - err := json.Unmarshal(aqueryJsonProto, &aqueryResult) - - if err != nil { - return nil, err - } +// 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 { + // Maps middleman artifact Id to input artifact depset ID. + // Middleman artifacts are treated as "substitute" artifacts for mixed builds. For example, + // if we find a middleman action which has outputs [foo, bar], and output [baz_middleman], then, + // for each other action which has input [baz_middleman], we add [foo, bar] to the inputs for + // that action instead. + middlemanIdToDepsetIds map[int][]int + // Maps depset Id to depset struct. + depsetIdToDepset map[int]depSetOfFiles + // depsetIdToArtifactIdsCache is a memoization of depset flattening, because flattening + // may be an expensive operation. + depsetIdToArtifactIdsCache map[int][]int + // Maps artifact Id to fully expanded path. + artifactIdToPath map[int]string +} +func newAqueryHandler(aqueryResult actionGraphContainer) (*aqueryArtifactHandler, error) { pathFragments := map[int]pathFragment{} for _, pathFragment := range aqueryResult.PathFragments { pathFragments[pathFragment.Id] = pathFragment } + artifactIdToPath := map[int]string{} for _, artifact := range aqueryResult.Artifacts { artifactPath, err := expandPathFragment(artifact.PathFragmentId, pathFragments) @@ -112,22 +119,87 @@ func AqueryBuildStatements(aqueryJsonProto []byte) ([]BuildStatement, error) { depsetIdToDepset[depset.Id] = depset } - // depsetIdToArtifactIdsCache is a memoization of depset flattening, because flattening - // may be an expensive operation. - depsetIdToArtifactIdsCache := map[int][]int{} - // Do a pass through all actions to identify which artifacts are middleman artifacts. - // These will be omitted from the inputs of other actions. - // TODO(b/180945500): Handle middleman actions; without proper handling, depending on generated - // headers may cause build failures. - middlemanArtifactIds := map[int]bool{} + middlemanIdToDepsetIds := map[int][]int{} for _, actionEntry := range aqueryResult.Actions { if actionEntry.Mnemonic == "Middleman" { for _, outputId := range actionEntry.OutputIds { - middlemanArtifactIds[outputId] = true + middlemanIdToDepsetIds[outputId] = actionEntry.InputDepSetIds } } } + return &aqueryArtifactHandler{ + middlemanIdToDepsetIds: middlemanIdToDepsetIds, + depsetIdToDepset: depsetIdToDepset, + depsetIdToArtifactIdsCache: map[int][]int{}, + artifactIdToPath: artifactIdToPath, + }, nil +} + +func (a *aqueryArtifactHandler) getInputPaths(depsetIds []int) ([]string, error) { + inputPaths := []string{} + + for _, inputDepSetId := range depsetIds { + inputArtifacts, err := a.artifactIdsFromDepsetId(inputDepSetId) + if err != nil { + return nil, err + } + for _, inputId := range inputArtifacts { + if middlemanInputDepsetIds, isMiddlemanArtifact := a.middlemanIdToDepsetIds[inputId]; isMiddlemanArtifact { + // Add all inputs from middleman actions which created middleman artifacts which are + // in the inputs for this action. + swappedInputPaths, err := a.getInputPaths(middlemanInputDepsetIds) + if err != nil { + return nil, err + } + inputPaths = append(inputPaths, swappedInputPaths...) + } else { + inputPath, exists := a.artifactIdToPath[inputId] + if !exists { + return nil, fmt.Errorf("undefined input artifactId %d", inputId) + } + inputPaths = append(inputPaths, inputPath) + } + } + } + return inputPaths, nil +} + +func (a *aqueryArtifactHandler) artifactIdsFromDepsetId(depsetId int) ([]int, error) { + if result, exists := a.depsetIdToArtifactIdsCache[depsetId]; exists { + return result, nil + } + if depset, exists := a.depsetIdToDepset[depsetId]; exists { + result := depset.DirectArtifactIds + for _, childId := range depset.TransitiveDepSetIds { + childArtifactIds, err := a.artifactIdsFromDepsetId(childId) + if err != nil { + return nil, err + } + result = append(result, childArtifactIds...) + } + a.depsetIdToArtifactIdsCache[depsetId] = result + return result, nil + } else { + return nil, fmt.Errorf("undefined input depsetId %d", depsetId) + } +} + +// AqueryBuildStatements returns an array of BuildStatements which should be registered (and output +// to a ninja file) to correspond one-to-one with the given action graph json proto (from a bazel +// aquery invocation). +func AqueryBuildStatements(aqueryJsonProto []byte) ([]BuildStatement, error) { + buildStatements := []BuildStatement{} + + var aqueryResult actionGraphContainer + err := json.Unmarshal(aqueryJsonProto, &aqueryResult) + if err != nil { + return nil, err + } + aqueryHandler, err := newAqueryHandler(aqueryResult) + if err != nil { + return nil, err + } for _, actionEntry := range aqueryResult.Actions { if shouldSkipAction(actionEntry) { @@ -136,7 +208,7 @@ func AqueryBuildStatements(aqueryJsonProto []byte) ([]BuildStatement, error) { outputPaths := []string{} var depfile *string for _, outputId := range actionEntry.OutputIds { - outputPath, exists := artifactIdToPath[outputId] + outputPath, exists := aqueryHandler.artifactIdToPath[outputId] if !exists { return nil, fmt.Errorf("undefined outputId %d", outputId) } @@ -151,25 +223,11 @@ func AqueryBuildStatements(aqueryJsonProto []byte) ([]BuildStatement, error) { outputPaths = append(outputPaths, outputPath) } } - inputPaths := []string{} - for _, inputDepSetId := range actionEntry.InputDepSetIds { - inputArtifacts, err := - artifactIdsFromDepsetId(depsetIdToDepset, depsetIdToArtifactIdsCache, inputDepSetId) - if err != nil { - return nil, err - } - for _, inputId := range inputArtifacts { - if _, isMiddlemanArtifact := middlemanArtifactIds[inputId]; isMiddlemanArtifact { - // Omit middleman artifacts. - continue - } - inputPath, exists := artifactIdToPath[inputId] - if !exists { - return nil, fmt.Errorf("undefined input artifactId %d", inputId) - } - inputPaths = append(inputPaths, inputPath) - } + inputPaths, err := aqueryHandler.getInputPaths(actionEntry.InputDepSetIds) + if err != nil { + return nil, err } + buildStatement := BuildStatement{ Command: strings.Join(proptools.ShellEscapeList(actionEntry.Arguments), " "), Depfile: depfile, @@ -192,8 +250,9 @@ func shouldSkipAction(a action) bool { if a.Mnemonic == "Symlink" || a.Mnemonic == "SourceSymlinkManifest" || a.Mnemonic == "SymlinkTree" { return true } - // TODO(b/180945500): Handle middleman actions; without proper handling, depending on generated - // headers may cause build failures. + // Middleman actions are not handled like other actions; they are handled separately as a + // preparatory step so that their inputs may be relayed to actions depending on middleman + // artifacts. if a.Mnemonic == "Middleman" { return true } @@ -209,28 +268,6 @@ func shouldSkipAction(a action) bool { return false } -func artifactIdsFromDepsetId(depsetIdToDepset map[int]depSetOfFiles, - depsetIdToArtifactIdsCache map[int][]int, depsetId int) ([]int, error) { - if result, exists := depsetIdToArtifactIdsCache[depsetId]; exists { - return result, nil - } - if depset, exists := depsetIdToDepset[depsetId]; exists { - result := depset.DirectArtifactIds - for _, childId := range depset.TransitiveDepSetIds { - childArtifactIds, err := - artifactIdsFromDepsetId(depsetIdToDepset, depsetIdToArtifactIdsCache, childId) - if err != nil { - return nil, err - } - result = append(result, childArtifactIds...) - } - depsetIdToArtifactIdsCache[depsetId] = result - return result, nil - } else { - return nil, fmt.Errorf("undefined input depsetId %d", depsetId) - } -} - func expandPathFragment(id int, pathFragmentsMap map[int]pathFragment) (string, error) { labels := []string{} currId := id diff --git a/bazel/aquery_test.go b/bazel/aquery_test.go index fa8810f0d..7b40dcdda 100644 --- a/bazel/aquery_test.go +++ b/bazel/aquery_test.go @@ -718,6 +718,93 @@ func TestTransitiveInputDepsets(t *testing.T) { assertBuildStatements(t, expectedBuildStatements, actualbuildStatements) } +func TestMiddlemenAction(t *testing.T) { + const inputString = ` +{ + "artifacts": [{ + "id": 1, + "pathFragmentId": 1 + }, { + "id": 2, + "pathFragmentId": 2 + }, { + "id": 3, + "pathFragmentId": 3 + }, { + "id": 4, + "pathFragmentId": 4 + }, { + "id": 5, + "pathFragmentId": 5 + }, { + "id": 6, + "pathFragmentId": 6 + }], + "pathFragments": [{ + "id": 1, + "label": "middleinput_one" + }, { + "id": 2, + "label": "middleinput_two" + }, { + "id": 3, + "label": "middleman_artifact" + }, { + "id": 4, + "label": "maininput_one" + }, { + "id": 5, + "label": "maininput_two" + }, { + "id": 6, + "label": "output" + }], + "depSetOfFiles": [{ + "id": 1, + "directArtifactIds": [1, 2] + }, { + "id": 2, + "directArtifactIds": [3, 4, 5] + }], + "actions": [{ + "targetId": 1, + "actionKey": "x", + "mnemonic": "Middleman", + "arguments": ["touch", "foo"], + "inputDepSetIds": [1], + "outputIds": [3], + "primaryOutputId": 3 + }, { + "targetId": 2, + "actionKey": "y", + "mnemonic": "Main action", + "arguments": ["touch", "foo"], + "inputDepSetIds": [2], + "outputIds": [6], + "primaryOutputId": 6 + }] +}` + + actual, err := AqueryBuildStatements([]byte(inputString)) + if err != nil { + t.Errorf("Unexpected error %q", err) + } + if expected := 1; len(actual) != expected { + t.Fatalf("Expected %d build statements, got %d", expected, len(actual)) + } + + bs := actual[0] + expectedInputs := []string{"middleinput_one", "middleinput_two", "maininput_one", "maininput_two"} + if !reflect.DeepEqual(bs.InputPaths, expectedInputs) { + t.Errorf("Expected main action inputs %q, but got %q", expectedInputs, bs.InputPaths) + } + + expectedOutputs := []string{"output"} + if !reflect.DeepEqual(bs.OutputPaths, expectedOutputs) { + t.Errorf("Expected main action outputs %q, but got %q", expectedOutputs, bs.OutputPaths) + } +} + func assertError(t *testing.T, err error, expected string) { if err == nil { t.Errorf("expected error '%s', but got no error", expected)