From 3a019a635a3276e5c6d1a5a4d9650fd603fe635b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20M=C3=A1rquez=20P=C3=A9rez=20Mu=C3=B1=C3=ADz=20D?= =?UTF-8?q?=C3=ADaz=20P=C3=BAras=20Thaureaux?= Date: Thu, 23 Jun 2022 16:02:44 +0000 Subject: [PATCH] Add StringAttribute for bp2building Instead of a StringListAttribute of length 1, introduce a more appropriately reduced StringAttribute so that e.g. `select`s work properly Test: new cases in TestGenerateBazelTargetModules Change-Id: I0ae0e4a51e39f85caf55b0d00459222ede6de79c --- bazel/properties.go | 140 ++++++++++++++++++++++++++++++ bp2build/build_conversion_test.go | 48 +++++++++- bp2build/bzl_conversion_test.go | 6 +- bp2build/configurability.go | 30 +++++++ bp2build/testing.go | 39 +++++---- 5 files changed, 246 insertions(+), 17 deletions(-) diff --git a/bazel/properties.go b/bazel/properties.go index aba97c647..bffd97bc5 100644 --- a/bazel/properties.go +++ b/bazel/properties.go @@ -960,6 +960,146 @@ func PartitionLabelListAttribute(ctx OtherModuleContext, lla *LabelListAttribute return ret } +// StringAttribute corresponds to the string Bazel attribute type with +// support for additional metadata, like configurations. +type StringAttribute struct { + // The base value of the string attribute. + Value *string + + // The configured attribute label list Values. Optional + // a map of independent configurability axes + ConfigurableValues configurableStrings +} + +type configurableStrings map[ConfigurationAxis]stringSelectValues + +func (cs configurableStrings) setValueForAxis(axis ConfigurationAxis, config string, str *string) { + if cs[axis] == nil { + cs[axis] = make(stringSelectValues) + } + var v = "" + if str != nil { + v = *str + } + cs[axis][config] = v +} + +type stringSelectValues map[string]string + +// HasConfigurableValues returns true if the attribute contains axis-specific string values. +func (sa StringAttribute) HasConfigurableValues() bool { + for _, selectValues := range sa.ConfigurableValues { + if len(selectValues) > 0 { + return true + } + } + return false +} + +// SetSelectValue set a value for a bazel select for the given axis, config and value. +func (sa *StringAttribute) SetSelectValue(axis ConfigurationAxis, config string, str *string) { + axis.validateConfig(config) + switch axis.configurationType { + case noConfig: + sa.Value = str + case arch, os, osArch, productVariables: + if sa.ConfigurableValues == nil { + sa.ConfigurableValues = make(configurableStrings) + } + sa.ConfigurableValues.setValueForAxis(axis, config, str) + default: + panic(fmt.Errorf("Unrecognized ConfigurationAxis %s", axis)) + } +} + +// SelectValue gets a value for a bazel select for the given axis and config. +func (sa *StringAttribute) SelectValue(axis ConfigurationAxis, config string) *string { + axis.validateConfig(config) + switch axis.configurationType { + case noConfig: + return sa.Value + case arch, os, osArch, productVariables: + if v, ok := sa.ConfigurableValues[axis][config]; ok { + return &v + } else { + return nil + } + default: + panic(fmt.Errorf("Unrecognized ConfigurationAxis %s", axis)) + } +} + +// SortedConfigurationAxes returns all the used ConfigurationAxis in sorted order. +func (sa *StringAttribute) SortedConfigurationAxes() []ConfigurationAxis { + keys := make([]ConfigurationAxis, 0, len(sa.ConfigurableValues)) + for k := range sa.ConfigurableValues { + keys = append(keys, k) + } + + sort.Slice(keys, func(i, j int) bool { return keys[i].less(keys[j]) }) + return keys +} + +// Collapse reduces the configurable axes of the string attribute to a single axis. +// This is necessary for final writing to bp2build, as a configurable string +// attribute can only be comprised by a single select. +func (sa *StringAttribute) Collapse() error { + axisTypes := sa.axisTypes() + _, containsOs := axisTypes[os] + _, containsArch := axisTypes[arch] + _, containsOsArch := axisTypes[osArch] + _, containsProductVariables := axisTypes[productVariables] + if containsProductVariables { + if containsOs || containsArch || containsOsArch { + return fmt.Errorf("boolean attribute could not be collapsed as it has two or more unrelated axes") + } + } + if (containsOs && containsArch) || (containsOsArch && (containsOs || containsArch)) { + // If a bool attribute has both os and arch configuration axes, the only + // way to successfully union their values is to increase the granularity + // of the configuration criteria to os_arch. + for osType, supportedArchs := range osToArchMap { + for _, supportedArch := range supportedArchs { + osArch := osArchString(osType, supportedArch) + if archOsVal := sa.SelectValue(OsArchConfigurationAxis, osArch); archOsVal != nil { + // Do nothing, as the arch_os is explicitly defined already. + } else { + archVal := sa.SelectValue(ArchConfigurationAxis, supportedArch) + osVal := sa.SelectValue(OsConfigurationAxis, osType) + if osVal != nil && archVal != nil { + // In this case, arch takes precedence. (This fits legacy Soong behavior, as arch mutator + // runs after os mutator. + sa.SetSelectValue(OsArchConfigurationAxis, osArch, archVal) + } else if osVal != nil && archVal == nil { + sa.SetSelectValue(OsArchConfigurationAxis, osArch, osVal) + } else if osVal == nil && archVal != nil { + sa.SetSelectValue(OsArchConfigurationAxis, osArch, archVal) + } + } + } + } + // All os_arch values are now set. Clear os and arch axes. + delete(sa.ConfigurableValues, ArchConfigurationAxis) + delete(sa.ConfigurableValues, OsConfigurationAxis) + // Verify post-condition; this should never fail, provided no additional + // axes are introduced. + if len(sa.ConfigurableValues) > 1 { + panic(fmt.Errorf("error in collapsing attribute: %#v", sa)) + } + } + return nil +} + +func (sa *StringAttribute) axisTypes() map[configurationType]bool { + types := map[configurationType]bool{} + for k := range sa.ConfigurableValues { + if strs := sa.ConfigurableValues[k]; len(strs) > 0 { + types[k.configurationType] = true + } + } + return types +} + // StringListAttribute corresponds to the string_list Bazel attribute type with // support for additional metadata, like configurations. type StringListAttribute struct { diff --git a/bp2build/build_conversion_test.go b/bp2build/build_conversion_test.go index c5644ed2d..1ac7518a8 100644 --- a/bp2build/build_conversion_test.go +++ b/bp2build/build_conversion_test.go @@ -230,6 +230,52 @@ func TestGenerateSoongModuleTargets(t *testing.T) { func TestGenerateBazelTargetModules(t *testing.T) { testCases := []Bp2buildTestCase{ + { + Description: "string prop empty", + Blueprint: `custom { + name: "foo", + string_literal_prop: "", + bazel_module: { bp2build_available: true }, +}`, + ExpectedBazelTargets: []string{ + makeBazelTarget("custom", "foo", AttrNameToString{ + "string_literal_prop": `""`, + }), + }, + }, + { + Description: `string prop "PROP"`, + Blueprint: `custom { + name: "foo", + string_literal_prop: "PROP", + bazel_module: { bp2build_available: true }, +}`, + ExpectedBazelTargets: []string{ + makeBazelTarget("custom", "foo", AttrNameToString{ + "string_literal_prop": `"PROP"`, + }), + }, + }, + { + Description: `string prop arch variant`, + Blueprint: `custom { + name: "foo", + arch: { + arm: { string_literal_prop: "ARM" }, + arm64: { string_literal_prop: "ARM64" }, + }, + bazel_module: { bp2build_available: true }, +}`, + ExpectedBazelTargets: []string{ + makeBazelTarget("custom", "foo", AttrNameToString{ + "string_literal_prop": `select({ + "//build/bazel/platforms/arch:arm": "ARM", + "//build/bazel/platforms/arch:arm64": "ARM64", + "//conditions:default": None, + })`, + }), + }, + }, { Description: "string ptr props", Blueprint: `custom { @@ -244,7 +290,7 @@ func TestGenerateBazelTargetModules(t *testing.T) { }, }, { - Description: "string props", + Description: "string list props", Blueprint: `custom { name: "foo", string_list_prop: ["a", "b"], diff --git a/bp2build/bzl_conversion_test.go b/bp2build/bzl_conversion_test.go index 6cb9509ed..28d2c759b 100644 --- a/bp2build/bzl_conversion_test.go +++ b/bp2build/bzl_conversion_test.go @@ -15,11 +15,12 @@ package bp2build import ( - "android/soong/android" "io/ioutil" "os" "strings" "testing" + + "android/soong/android" ) func setUp() { @@ -103,6 +104,7 @@ custom = rule( "one_to_many_prop": attr.bool(), "other_embedded_prop": attr.string(), "string_list_prop": attr.string_list(), + "string_literal_prop": attr.string(), "string_prop": attr.string(), "string_ptr_prop": attr.string(), }, @@ -132,6 +134,7 @@ custom_defaults = rule( "one_to_many_prop": attr.bool(), "other_embedded_prop": attr.string(), "string_list_prop": attr.string_list(), + "string_literal_prop": attr.string(), "string_prop": attr.string(), "string_ptr_prop": attr.string(), }, @@ -161,6 +164,7 @@ custom_test_ = rule( "one_to_many_prop": attr.bool(), "other_embedded_prop": attr.string(), "string_list_prop": attr.string_list(), + "string_literal_prop": attr.string(), "string_prop": attr.string(), "string_ptr_prop": attr.string(), # test_prop start diff --git a/bp2build/configurability.go b/bp2build/configurability.go index d37a52394..9398d127c 100644 --- a/bp2build/configurability.go +++ b/bp2build/configurability.go @@ -13,6 +13,30 @@ import ( type selects map[string]reflect.Value +func getStringValue(str bazel.StringAttribute) (reflect.Value, []selects) { + value := reflect.ValueOf(str.Value) + + if !str.HasConfigurableValues() { + return value, []selects{} + } + + ret := selects{} + for _, axis := range str.SortedConfigurationAxes() { + configToStrs := str.ConfigurableValues[axis] + for config, strs := range configToStrs { + selectKey := axis.SelectKey(config) + ret[selectKey] = reflect.ValueOf(strs) + } + } + // if there is a select, use the base value as the conditions default value + if len(ret) > 0 { + ret[bazel.ConditionsDefaultSelectKey] = value + value = reflect.Zero(value.Type()) + } + + return value, []selects{ret} +} + func getStringListValues(list bazel.StringListAttribute) (reflect.Value, []selects) { value := reflect.ValueOf(list.Value) if !list.HasConfigurableValues() { @@ -137,6 +161,12 @@ func prettyPrintAttribute(v bazel.Attribute, indent int) (string, error) { // If true, print the default attribute value, even if the attribute is zero. shouldPrintDefault := false switch list := v.(type) { + case bazel.StringAttribute: + if err := list.Collapse(); err != nil { + return "", err + } + value, configurableAttrs = getStringValue(list) + defaultSelectValue = &bazelNone case bazel.StringListAttribute: value, configurableAttrs = getStringListValues(list) defaultSelectValue = &emptyBazelList diff --git a/bp2build/testing.go b/bp2build/testing.go index 3ee5096da..0f321de54 100644 --- a/bp2build/testing.go +++ b/bp2build/testing.go @@ -173,11 +173,12 @@ type customProps struct { Bool_prop bool Bool_ptr_prop *bool // Ensure that properties tagged `blueprint:mutated` are omitted - Int_prop int `blueprint:"mutated"` - Int64_ptr_prop *int64 - String_prop string - String_ptr_prop *string - String_list_prop []string + Int_prop int `blueprint:"mutated"` + Int64_ptr_prop *int64 + String_prop string + String_literal_prop *string `android:"arch_variant"` + String_ptr_prop *string + String_list_prop []string Nested_props nestedProps Nested_props_ptr *nestedProps @@ -305,23 +306,29 @@ type OtherEmbeddedAttr struct { type customBazelModuleAttributes struct { EmbeddedAttr *OtherEmbeddedAttr - String_ptr_prop *string - String_list_prop []string - Arch_paths bazel.LabelListAttribute + String_literal_prop bazel.StringAttribute + String_ptr_prop *string + String_list_prop []string + Arch_paths bazel.LabelListAttribute } func (m *customModule) ConvertWithBp2build(ctx android.TopDownMutatorContext) { - paths := bazel.LabelListAttribute{} - if p := m.props.One_to_many_prop; p != nil && *p { customBp2buildOneToMany(ctx, m) return } + paths := bazel.LabelListAttribute{} + strAttr := bazel.StringAttribute{} for axis, configToProps := range m.GetArchVariantProperties(ctx, &customProps{}) { for config, props := range configToProps { - if archProps, ok := props.(*customProps); ok && archProps.Arch_paths != nil { - paths.SetSelectValue(axis, config, android.BazelLabelForModuleSrcExcludes(ctx, archProps.Arch_paths, archProps.Arch_paths_exclude)) + if custProps, ok := props.(*customProps); ok { + if custProps.Arch_paths != nil { + paths.SetSelectValue(axis, config, android.BazelLabelForModuleSrcExcludes(ctx, custProps.Arch_paths, custProps.Arch_paths_exclude)) + } + if custProps.String_literal_prop != nil { + strAttr.SetSelectValue(axis, config, custProps.String_literal_prop) + } } } } @@ -329,10 +336,12 @@ func (m *customModule) ConvertWithBp2build(ctx android.TopDownMutatorContext) { paths.ResolveExcludes() attrs := &customBazelModuleAttributes{ - String_ptr_prop: m.props.String_ptr_prop, - String_list_prop: m.props.String_list_prop, - Arch_paths: paths, + String_literal_prop: strAttr, + String_ptr_prop: m.props.String_ptr_prop, + String_list_prop: m.props.String_list_prop, + Arch_paths: paths, } + attrs.Embedded_attr = m.props.Embedded_prop if m.props.OtherEmbeddedProps != nil { attrs.OtherEmbeddedAttr = &OtherEmbeddedAttr{Other_embedded_attr: m.props.OtherEmbeddedProps.Other_embedded_prop}