From 94e2603ab705d9f2ef23b140ebb30cac488e984a Mon Sep 17 00:00:00 2001 From: Alix Date: Tue, 16 Aug 2022 20:37:33 +0000 Subject: [PATCH 1/2] refactoring build_conversion.go to remove panic Returning errors throughout instead of calling panic() Errors will be more useful for testing Test: bp2build tests Change-Id: I3b03f0a30e7a80878e91c7f0e2df5a94d9d6b780 --- bp2build/build_conversion.go | 93 +++++++++++++++++++++++------------- bp2build/testing.go | 3 ++ 2 files changed, 64 insertions(+), 32 deletions(-) diff --git a/bp2build/build_conversion.go b/bp2build/build_conversion.go index cf465335b..ca8185e57 100644 --- a/bp2build/build_conversion.go +++ b/bp2build/build_conversion.go @@ -289,7 +289,9 @@ func GenerateBazelTargets(ctx *CodegenContext, generateFilegroups bool) (convers return } } - targets = generateBazelTargets(bpCtx, aModule) + var targetErrs []error + targets, targetErrs = generateBazelTargets(bpCtx, aModule) + errs = append(errs, targetErrs...) for _, t := range targets { // A module can potentially generate more than 1 Bazel // target, each of a different rule class. @@ -306,7 +308,10 @@ func GenerateBazelTargets(ctx *CodegenContext, generateFilegroups bool) (convers // be mapped cleanly to a bazel label. return } - t := generateSoongModuleTarget(bpCtx, m) + t, err := generateSoongModuleTarget(bpCtx, m) + if err != nil { + errs = append(errs, err) + } targets = append(targets, t) default: errs = append(errs, fmt.Errorf("Unknown code-generation mode: %s", ctx.Mode())) @@ -347,12 +352,18 @@ func GenerateBazelTargets(ctx *CodegenContext, generateFilegroups bool) (convers }, errs } -func generateBazelTargets(ctx bpToBuildContext, m android.Module) []BazelTarget { +func generateBazelTargets(ctx bpToBuildContext, m android.Module) ([]BazelTarget, []error) { var targets []BazelTarget + var errs []error for _, m := range m.Bp2buildTargets() { - targets = append(targets, generateBazelTarget(ctx, m)) + target, err := generateBazelTarget(ctx, m) + if err != nil { + errs = append(errs, err) + return targets, errs + } + targets = append(targets, target) } - return targets + return targets, errs } type bp2buildModule interface { @@ -363,13 +374,16 @@ type bp2buildModule interface { BazelAttributes() []interface{} } -func generateBazelTarget(ctx bpToBuildContext, m bp2buildModule) BazelTarget { +func generateBazelTarget(ctx bpToBuildContext, m bp2buildModule) (BazelTarget, error) { ruleClass := m.BazelRuleClass() bzlLoadLocation := m.BazelRuleLoadLocation() // extract the bazel attributes from the module. attrs := m.BazelAttributes() - props := extractModuleProperties(attrs, true) + props, err := extractModuleProperties(attrs, true) + if err != nil { + return BazelTarget{}, err + } // name is handled in a special manner delete(props.Attrs, "name") @@ -389,13 +403,13 @@ func generateBazelTarget(ctx bpToBuildContext, m bp2buildModule) BazelTarget { targetName, attributes, ), - } + }, nil } // Convert a module and its deps and props into a Bazel macro/rule // representation in the BUILD file. -func generateSoongModuleTarget(ctx bpToBuildContext, m blueprint.Module) BazelTarget { - props := getBuildProperties(ctx, m) +func generateSoongModuleTarget(ctx bpToBuildContext, m blueprint.Module) (BazelTarget, error) { + props, err := getBuildProperties(ctx, m) // TODO(b/163018919): DirectDeps can have duplicate (module, variant) // items, if the modules are added using different DependencyTag. Figure @@ -429,21 +443,21 @@ func generateSoongModuleTarget(ctx bpToBuildContext, m blueprint.Module) BazelTa ctx.ModuleSubDir(m), depLabelList, attributes), - } + }, err } -func getBuildProperties(ctx bpToBuildContext, m blueprint.Module) BazelAttributes { +func getBuildProperties(ctx bpToBuildContext, m blueprint.Module) (BazelAttributes, error) { // TODO: this omits properties for blueprint modules (blueprint_go_binary, // bootstrap_go_binary, bootstrap_go_package), which will have to be handled separately. if aModule, ok := m.(android.Module); ok { return extractModuleProperties(aModule.GetProperties(), false) } - return BazelAttributes{} + return BazelAttributes{}, nil } // Generically extract module properties and types into a map, keyed by the module property name. -func extractModuleProperties(props []interface{}, checkForDuplicateProperties bool) BazelAttributes { +func extractModuleProperties(props []interface{}, checkForDuplicateProperties bool) (BazelAttributes, error) { ret := map[string]string{} // Iterate over this android.Module's property structs. @@ -456,24 +470,29 @@ func extractModuleProperties(props []interface{}, checkForDuplicateProperties bo // manipulate internal props, if needed. if isStructPtr(propertiesValue.Type()) { structValue := propertiesValue.Elem() - for k, v := range extractStructProperties(structValue, 0) { + ok, err := extractStructProperties(structValue, 0) + if err != nil { + return BazelAttributes{}, err + } + for k, v := range ok { if existing, exists := ret[k]; checkForDuplicateProperties && exists { - panic(fmt.Errorf( + return BazelAttributes{}, fmt.Errorf( "%s (%v) is present in properties whereas it should be consolidated into a commonAttributes", - k, existing)) + k, existing) } ret[k] = v } } else { - panic(fmt.Errorf( - "properties must be a pointer to a struct, got %T", - propertiesValue.Interface())) + return BazelAttributes{}, + fmt.Errorf( + "properties must be a pointer to a struct, got %T", + propertiesValue.Interface()) } } return BazelAttributes{ Attrs: ret, - } + }, nil } func isStructPtr(t reflect.Type) bool { @@ -531,7 +550,12 @@ func prettyPrint(propertyValue reflect.Value, indent int, emitZeroValues bool) ( } // Sort and print the struct props by the key. - structProps := extractStructProperties(propertyValue, indent) + structProps, err := extractStructProperties(propertyValue, indent) + + if err != nil { + return "", err + } + if len(structProps) == 0 { return "", nil } @@ -550,11 +574,13 @@ func prettyPrint(propertyValue reflect.Value, indent int, emitZeroValues bool) ( // which each property value correctly pretty-printed and indented at the right nest level, // since property structs can be nested. In Starlark, nested structs are represented as nested // dicts: https://docs.bazel.build/skylark/lib/dict.html -func extractStructProperties(structValue reflect.Value, indent int) map[string]string { +func extractStructProperties(structValue reflect.Value, indent int) (map[string]string, error) { if structValue.Kind() != reflect.Struct { - panic(fmt.Errorf("Expected a reflect.Struct type, but got %s", structValue.Kind())) + return map[string]string{}, fmt.Errorf("Expected a reflect.Struct type, but got %s", structValue.Kind()) } + var err error + ret := map[string]string{} structType := structValue.Type() for i := 0; i < structValue.NumField(); i++ { @@ -575,7 +601,10 @@ func extractStructProperties(structValue reflect.Value, indent int) map[string]s fieldValue = fieldValue.Elem() } if fieldValue.Type().Kind() == reflect.Struct { - propsToMerge := extractStructProperties(fieldValue, indent) + propsToMerge, err := extractStructProperties(fieldValue, indent) + if err != nil { + return map[string]string{}, err + } for prop, value := range propsToMerge { ret[prop] = value } @@ -584,20 +613,20 @@ func extractStructProperties(structValue reflect.Value, indent int) map[string]s } propertyName := proptools.PropertyNameForField(field.Name) - prettyPrintedValue, err := prettyPrint(fieldValue, indent+1, false) + var prettyPrintedValue string + prettyPrintedValue, err = prettyPrint(fieldValue, indent+1, false) if err != nil { - panic( - fmt.Errorf( - "Error while parsing property: %q. %s", - propertyName, - err)) + return map[string]string{}, fmt.Errorf( + "Error while parsing property: %q. %s", + propertyName, + err) } if prettyPrintedValue != "" { ret[propertyName] = prettyPrintedValue } } - return ret + return ret, nil } func isZero(value reflect.Value) bool { diff --git a/bp2build/testing.go b/bp2build/testing.go index a0d512f1d..ac1268cb5 100644 --- a/bp2build/testing.go +++ b/bp2build/testing.go @@ -384,6 +384,9 @@ func customBp2buildOneToMany(ctx android.TopDownMutatorContext, m *customModule) func generateBazelTargetsForDir(codegenCtx *CodegenContext, dir string) (BazelTargets, []error) { // TODO: Set generateFilegroups to true and/or remove the generateFilegroups argument completely res, err := GenerateBazelTargets(codegenCtx, false) + if err != nil { + return BazelTargets{}, err + } return res.buildFileToTargets[dir], err } From bbfd53832655bbae39c57aed62e821fc23a4bad7 Mon Sep 17 00:00:00 2001 From: Alix Date: Thu, 9 Jun 2022 18:52:05 +0000 Subject: [PATCH 2/2] product_variables srcs in prebuilt_etc bp2build conversion handles product_variable srcs bug: 228353067 Test: prebuilt_etc_conversion_test.go Change-Id: I82d3a384ee14d4e981d502dd9eb824c87d5ae2c7 --- android/variable.go | 33 ++++++--- bazel/configurability.go | 5 +- bazel/properties.go | 14 +++- bazel/properties_test.go | 10 +-- bp2build/prebuilt_etc_conversion_test.go | 85 +++++++++++++++++++++++- etc/prebuilt_etc.go | 17 +++++ 6 files changed, 144 insertions(+), 20 deletions(-) diff --git a/android/variable.go b/android/variable.go index 7a080fef7..b15605170 100644 --- a/android/variable.go +++ b/android/variable.go @@ -592,6 +592,9 @@ type ProductConfigProperty struct { // "acme__board__soc_a", "acme__board__soc_b", and // "acme__board__conditions_default" FullConfig string + + // keeps track of whether this product variable is nested under an arch variant + OuterAxis bazel.ConfigurationAxis } func (p *ProductConfigProperty) AlwaysEmit() bool { @@ -600,11 +603,11 @@ func (p *ProductConfigProperty) AlwaysEmit() bool { func (p *ProductConfigProperty) ConfigurationAxis() bazel.ConfigurationAxis { if p.Namespace == "" { - return bazel.ProductVariableConfigurationAxis(p.FullConfig) + return bazel.ProductVariableConfigurationAxis(p.FullConfig, p.OuterAxis) } else { // Soong config variables can be uniquely identified by the namespace // (e.g. acme, android) and the product variable name (e.g. board, size) - return bazel.ProductVariableConfigurationAxis(p.Namespace + "__" + p.Name) + return bazel.ProductVariableConfigurationAxis(p.Namespace+"__"+p.Name, bazel.NoConfigAxis) } } @@ -663,9 +666,11 @@ func ProductVariableProperties(ctx BazelConversionPathContext) ProductConfigProp moduleBase.variableProperties, "", "", - &productConfigProperties) + &productConfigProperties, + bazel.ConfigurationAxis{}, + ) - for _, configToProps := range moduleBase.GetArchVariantProperties(ctx, moduleBase.variableProperties) { + for axis, configToProps := range moduleBase.GetArchVariantProperties(ctx, moduleBase.variableProperties) { for config, props := range configToProps { // GetArchVariantProperties is creating an instance of the requested type // and productVariablesValues expects an interface, so no need to cast @@ -674,7 +679,8 @@ func ProductVariableProperties(ctx BazelConversionPathContext) ProductConfigProp props, "", config, - &productConfigProperties) + &productConfigProperties, + axis) } } } @@ -687,7 +693,8 @@ func ProductVariableProperties(ctx BazelConversionPathContext) ProductConfigProp namespacedVariableProp, namespace, "", - &productConfigProperties) + &productConfigProperties, + bazel.NoConfigAxis) } } } @@ -803,6 +810,7 @@ func (props *ProductConfigProperties) zeroValuesForNamespacedVariables() { p.Name, p.FullConfig, zeroValue, + bazel.NoConfigAxis, ) } } @@ -810,7 +818,7 @@ func (props *ProductConfigProperties) zeroValuesForNamespacedVariables() { } func (p *ProductConfigProperties) AddProductConfigProperty( - propertyName, namespace, productVariableName, config string, property interface{}) { + propertyName, namespace, productVariableName, config string, property interface{}, outerAxis bazel.ConfigurationAxis) { if (*p)[propertyName] == nil { (*p)[propertyName] = make(map[ProductConfigProperty]interface{}) } @@ -819,6 +827,7 @@ func (p *ProductConfigProperties) AddProductConfigProperty( Namespace: namespace, // e.g. acme, android Name: productVariableName, // e.g. size, feature1, feature2, FEATURE3, board FullConfig: config, // e.g. size, feature1-x86, size__conditions_default + OuterAxis: outerAxis, } if existing, ok := (*p)[propertyName][productConfigProp]; ok && namespace != "" { @@ -869,7 +878,7 @@ func maybeExtractConfigVarProp(v reflect.Value) (reflect.Value, bool) { return v, true } -func (productConfigProperties *ProductConfigProperties) AddProductConfigProperties(namespace, suffix string, variableValues reflect.Value) { +func (productConfigProperties *ProductConfigProperties) AddProductConfigProperties(namespace, suffix string, variableValues reflect.Value, outerAxis bazel.ConfigurationAxis) { // variableValues can either be a product_variables or // soong_config_variables struct. // @@ -974,7 +983,8 @@ func (productConfigProperties *ProductConfigProperties) AddProductConfigProperti namespace, // e.g. acme, android productVariableName, // e.g. size, feature1, FEATURE2, board config, - field.Field(k).Interface(), // e.g. ["-DDEFAULT"], ["foo", "bar"] + field.Field(k).Interface(), // e.g. ["-DDEFAULT"], ["foo", "bar"], + outerAxis, ) } } else if property.Kind() != reflect.Interface { @@ -988,6 +998,7 @@ func (productConfigProperties *ProductConfigProperties) AddProductConfigProperti productVariableName, config, property.Interface(), + outerAxis, ) } } @@ -998,14 +1009,14 @@ func (productConfigProperties *ProductConfigProperties) AddProductConfigProperti // product_variables and soong_config_variables to structs that can be generated // as select statements. func productVariableValues( - fieldName string, variableProps interface{}, namespace, suffix string, productConfigProperties *ProductConfigProperties) { + fieldName string, variableProps interface{}, namespace, suffix string, productConfigProperties *ProductConfigProperties, outerAxis bazel.ConfigurationAxis) { if suffix != "" { suffix = "-" + suffix } // variableValues represent the product_variables or soong_config_variables struct. variableValues := reflect.ValueOf(variableProps).Elem().FieldByName(fieldName) - productConfigProperties.AddProductConfigProperties(namespace, suffix, variableValues) + productConfigProperties.AddProductConfigProperties(namespace, suffix, variableValues, outerAxis) } func VariableMutator(mctx BottomUpMutatorContext) { diff --git a/bazel/configurability.go b/bazel/configurability.go index e1cdd4acf..7ff202bf2 100644 --- a/bazel/configurability.go +++ b/bazel/configurability.go @@ -295,10 +295,11 @@ var ( ) // ProductVariableConfigurationAxis returns an axis for the given product variable -func ProductVariableConfigurationAxis(variable string) ConfigurationAxis { +func ProductVariableConfigurationAxis(variable string, outerAxis ConfigurationAxis) ConfigurationAxis { return ConfigurationAxis{ configurationType: productVariables, subType: variable, + outerAxisType: outerAxis.configurationType, } } @@ -309,6 +310,8 @@ type ConfigurationAxis struct { // some configuration types (e.g. productVariables) have multiple independent axes, subType helps // distinguish between them without needing to list all 17 product variables. subType string + // used to keep track of which product variables are arch variant + outerAxisType configurationType } func (ca *ConfigurationAxis) less(other ConfigurationAxis) bool { diff --git a/bazel/properties.go b/bazel/properties.go index d82fa6417..11f62477d 100644 --- a/bazel/properties.go +++ b/bazel/properties.go @@ -309,7 +309,19 @@ func (la *LabelAttribute) Collapse() error { _, containsProductVariables := axisTypes[productVariables] if containsProductVariables { if containsOs || containsArch || containsOsArch { - return fmt.Errorf("label attribute could not be collapsed as it has two or more unrelated axes") + if containsArch { + allProductVariablesAreArchVariant := true + for k := range la.ConfigurableValues { + if k.configurationType == productVariables && k.outerAxisType != arch { + allProductVariablesAreArchVariant = false + } + } + if !allProductVariablesAreArchVariant { + return fmt.Errorf("label attribute could not be collapsed as it has two or more unrelated axes") + } + } else { + return fmt.Errorf("label attribute could not be collapsed as it has two or more unrelated axes") + } } } if (containsOs && containsArch) || (containsOsArch && (containsOs || containsArch)) { diff --git a/bazel/properties_test.go b/bazel/properties_test.go index dc4d5b0f5..8729381b5 100644 --- a/bazel/properties_test.go +++ b/bazel/properties_test.go @@ -247,13 +247,13 @@ func TestResolveExcludes(t *testing.T) { OsArchConfigurationAxis: labelListSelectValues{ "linux_x86": makeLabelList([]string{"linux_x86_include"}, []string{}), }, - ProductVariableConfigurationAxis("product_with_defaults"): labelListSelectValues{ + ProductVariableConfigurationAxis("product_with_defaults", NoConfigAxis): labelListSelectValues{ "a": makeLabelList([]string{}, []string{"not_in_value"}), "b": makeLabelList([]string{"b_val"}, []string{}), "c": makeLabelList([]string{"c_val"}, []string{}), ConditionsDefaultConfigKey: makeLabelList([]string{"c_val", "default", "default2"}, []string{}), }, - ProductVariableConfigurationAxis("product_only_with_excludes"): labelListSelectValues{ + ProductVariableConfigurationAxis("product_only_with_excludes", NoConfigAxis): labelListSelectValues{ "a": makeLabelList([]string{}, []string{"not_in_value"}), }, }, @@ -281,7 +281,7 @@ func TestResolveExcludes(t *testing.T) { "linux_x86": makeLabels("linux_x86_include"), ConditionsDefaultConfigKey: nilLabels, }, - ProductVariableConfigurationAxis("product_with_defaults"): { + ProductVariableConfigurationAxis("product_with_defaults", NoConfigAxis): { "a": nilLabels, "b": makeLabels("b_val"), "c": makeLabels("c_val"), @@ -674,7 +674,7 @@ func TestDeduplicateAxesFromBase(t *testing.T) { OsArchConfigurationAxis: stringListSelectValues{ "linux_x86": {"linux_x86_include"}, }, - ProductVariableConfigurationAxis("a"): stringListSelectValues{ + ProductVariableConfigurationAxis("a", NoConfigAxis): stringListSelectValues{ "a": []string{"not_in_value"}, }, }, @@ -699,7 +699,7 @@ func TestDeduplicateAxesFromBase(t *testing.T) { "linux": []string{"linux_include"}, }, OsArchConfigurationAxis: stringListSelectValues{}, - ProductVariableConfigurationAxis("a"): stringListSelectValues{ + ProductVariableConfigurationAxis("a", NoConfigAxis): stringListSelectValues{ "a": []string{"not_in_value"}, }, } diff --git a/bp2build/prebuilt_etc_conversion_test.go b/bp2build/prebuilt_etc_conversion_test.go index b1e70dc5f..aa0a5b728 100644 --- a/bp2build/prebuilt_etc_conversion_test.go +++ b/bp2build/prebuilt_etc_conversion_test.go @@ -15,10 +15,11 @@ package bp2build import ( + "fmt" + "testing" + "android/soong/android" "android/soong/etc" - - "testing" ) func runPrebuiltEtcTestCase(t *testing.T, tc Bp2buildTestCase) { @@ -128,6 +129,32 @@ prebuilt_etc { "dir": `"etc/tz"`, })}}) } +func TestPrebuiltEtcProductVariables(t *testing.T) { + runPrebuiltEtcTestCase(t, Bp2buildTestCase{ + Description: "prebuilt etc - product variables", + Filesystem: map[string]string{}, + Blueprint: ` +prebuilt_etc { + name: "apex_tz_version", + src: "version/tz_version", + filename: "tz_version", + product_variables: { + native_coverage: { + src: "src1", + }, + }, +} +`, + ExpectedBazelTargets: []string{ + MakeBazelTarget("prebuilt_file", "apex_tz_version", AttrNameToString{ + "filename": `"tz_version"`, + "src": `select({ + "//build/bazel/product_variables:native_coverage": "src1", + "//conditions:default": "version/tz_version", + })`, + "dir": `"etc"`, + })}}) +} func runPrebuiltUsrShareTestCase(t *testing.T, tc Bp2buildTestCase) { t.Helper() @@ -265,3 +292,57 @@ prebuilt_etc { "dir": `"etc"`, })}}) } + +func TestPrebuiltEtcProductVariableArchSrcs(t *testing.T) { + runPrebuiltEtcTestCase(t, Bp2buildTestCase{ + Description: "prebuilt etc- SRcs from arch variant product variables", + Filesystem: map[string]string{}, + Blueprint: ` +prebuilt_etc { + name: "foo", + filename: "fooFilename", + arch: { + arm: { + src: "armSrc", + product_variables: { + native_coverage: { + src: "nativeCoverageArmSrc", + }, + }, + }, + }, +}`, + ExpectedBazelTargets: []string{ + MakeBazelTarget("prebuilt_file", "foo", AttrNameToString{ + "filename": `"fooFilename"`, + "dir": `"etc"`, + "src": `select({ + "//build/bazel/platforms/arch:arm": "armSrc", + "//build/bazel/product_variables:native_coverage-arm": "nativeCoverageArmSrc", + "//conditions:default": None, + })`, + })}}) +} + +func TestPrebuiltEtcProductVariableError(t *testing.T) { + runPrebuiltEtcTestCase(t, Bp2buildTestCase{ + Description: "", + Filesystem: map[string]string{}, + Blueprint: ` +prebuilt_etc { + name: "foo", + filename: "fooFilename", + arch: { + arm: { + src: "armSrc", + }, + }, + product_variables: { + native_coverage: { + src: "nativeCoverageArmSrc", + }, + }, +}`, + ExpectedErr: fmt.Errorf("label attribute could not be collapsed"), + }) +} diff --git a/etc/prebuilt_etc.go b/etc/prebuilt_etc.go index b2361ce84..7520f5875 100644 --- a/etc/prebuilt_etc.go +++ b/etc/prebuilt_etc.go @@ -31,6 +31,7 @@ import ( "encoding/json" "fmt" "path/filepath" + "reflect" "strings" "github.com/google/blueprint/proptools" @@ -692,6 +693,22 @@ func (module *PrebuiltEtc) ConvertWithBp2build(ctx android.TopDownMutatorContext src.SetSelectValue(axis, config, label) } } + + for propName, productConfigProps := range android.ProductVariableProperties(ctx) { + for configProp, propVal := range productConfigProps { + if propName == "Src" { + props, ok := propVal.(*string) + if !ok { + ctx.PropertyErrorf(" Expected Property to have type string, but was %s\n", reflect.TypeOf(propVal).String()) + continue + } + if props != nil { + label := android.BazelLabelForModuleSrcSingle(ctx, *props) + src.SetSelectValue(configProp.ConfigurationAxis(), configProp.SelectKey(), label) + } + } + } + } } var filename string