From 01812020c1e4c00e6d73a93bac84f1cabc540ce1 Mon Sep 17 00:00:00 2001 From: Jingwen Chen Date: Fri, 19 Nov 2021 14:29:43 +0000 Subject: [PATCH] Add support for writing all Soong config variables into @soong_injection. Also remove the need to use bp2build_available on soong_config_module_types as we want to convert every single of them into the tree for a complete soong_injection soong_config_variables.bzl file. The variables are split into their bool, value and string types respectively, as they all need to be handled differently on the Bazel product_platform side, as well as for generating constraint values and settings. For example, value variables need to integrate with TemplateVariableInfo, and string variables need to include the string value itself into the select key/constraint value. Sample soong_config_variables.bzl file: https://gist.github.com/jin/cef700bfb20c8656a931306dd71d47e1 Test: CI Bug: 198556411 Change-Id: I8665dd1269a507edb37de62407ed3641564bea5c --- android/bazel.go | 20 ++- android/config.go | 7 +- android/soong_config_modules.go | 6 +- android/soongconfig/modules.go | 95 ++++++++++++++- android/soongconfig/modules_test.go | 114 ++++++++++++++++++ bazel/properties.go | 17 --- bp2build/Android.bp | 1 + bp2build/conversion.go | 2 + bp2build/conversion_test.go | 7 +- ...oong_config_module_type_conversion_test.go | 14 --- 10 files changed, 239 insertions(+), 44 deletions(-) diff --git a/android/bazel.go b/android/bazel.go index f4686a09f..9efc033de 100644 --- a/android/bazel.go +++ b/android/bazel.go @@ -15,7 +15,6 @@ package android import ( - "android/soong/bazel" "fmt" "io/ioutil" "path/filepath" @@ -25,11 +24,28 @@ import ( "github.com/google/blueprint/proptools" ) +type bazelModuleProperties struct { + // The label of the Bazel target replacing this Soong module. When run in conversion mode, this + // will import the handcrafted build target into the autogenerated file. Note: this may result in + // a conflict due to duplicate targets if bp2build_available is also set. + Label *string + + // If true, bp2build will generate the converted Bazel target for this module. Note: this may + // cause a conflict due to the duplicate targets if label is also set. + // + // This is a bool pointer to support tristates: true, false, not set. + // + // To opt-in a module, set bazel_module: { bp2build_available: true } + // To opt-out a module, set bazel_module: { bp2build_available: false } + // To defer the default setting for the directory, do not set the value. + Bp2build_available *bool +} + // Properties contains common module properties for Bazel migration purposes. type properties struct { // In USE_BAZEL_ANALYSIS=1 mode, this represents the Bazel target replacing // this Soong module. - Bazel_module bazel.BazelModuleProperties + Bazel_module bazelModuleProperties } // namespacedVariableProperties is a map from a string representing a Soong diff --git a/android/config.go b/android/config.go index 6f055650b..5ee28e735 100644 --- a/android/config.go +++ b/android/config.go @@ -155,9 +155,10 @@ type config struct { fs pathtools.FileSystem mockBpList string - runningAsBp2Build bool - bp2buildPackageConfig Bp2BuildConfig - bp2buildModuleTypeConfig map[string]bool + runningAsBp2Build bool + bp2buildPackageConfig Bp2BuildConfig + bp2buildModuleTypeConfig map[string]bool + Bp2buildSoongConfigDefinitions soongconfig.Bp2BuildSoongConfigDefinitions // If testAllowNonExistentPaths is true then PathForSource and PathForModuleSrc won't error // in tests when a path doesn't exist. diff --git a/android/soong_config_modules.go b/android/soong_config_modules.go index dcf2223ef..91bbce68e 100644 --- a/android/soong_config_modules.go +++ b/android/soong_config_modules.go @@ -380,6 +380,9 @@ func loadSoongConfigModuleTypeDefinition(ctx LoadHookContext, from string) map[s } mtDef, errs := soongconfig.Parse(r, from) + if ctx.Config().runningAsBp2Build { + ctx.Config().Bp2buildSoongConfigDefinitions.AddVars(*mtDef) + } if len(errs) > 0 { reportErrors(ctx, from, errs...) @@ -415,14 +418,13 @@ func configModuleFactory(factory blueprint.ModuleFactory, moduleType *soongconfi if !conditionalFactoryProps.IsValid() { return factory } - useBp2buildHook := bp2build && proptools.BoolDefault(moduleType.Bp2buildAvailable, false) return func() (blueprint.Module, []interface{}) { module, props := factory() conditionalProps := proptools.CloneEmptyProperties(conditionalFactoryProps) props = append(props, conditionalProps.Interface()) - if useBp2buildHook { + if bp2build { // The loadhook is different for bp2build, since we don't want to set a specific // set of property values based on a vendor var -- we want __all of them__ to // generate select statements, so we put the entire soong_config_variables diff --git a/android/soongconfig/modules.go b/android/soongconfig/modules.go index 1af89ba5d..3a93f478d 100644 --- a/android/soongconfig/modules.go +++ b/android/soongconfig/modules.go @@ -15,7 +15,6 @@ package soongconfig import ( - "android/soong/bazel" "fmt" "io" "reflect" @@ -121,8 +120,6 @@ type ModuleTypeProperties struct { // the list of properties that this module type will extend. Properties []string - - Bazel_module bazel.BazelModuleProperties } func processModuleTypeDef(v *SoongConfigDefinition, def *parser.Module) (errs []error) { @@ -233,6 +230,96 @@ type SoongConfigDefinition struct { variables map[string]soongConfigVariable } +// Bp2BuildSoongConfigDefinition keeps a global record of all soong config +// string vars, bool vars and value vars created by every +// soong_config_module_type in this build. +type Bp2BuildSoongConfigDefinitions struct { + StringVars map[string]map[string]bool + BoolVars map[string]bool + ValueVars map[string]bool +} + +// SoongConfigVariablesForBp2build extracts information from a +// SoongConfigDefinition that bp2build needs to generate constraint settings and +// values for, in order to migrate soong_config_module_type usages to Bazel. +func (defs *Bp2BuildSoongConfigDefinitions) AddVars(mtDef SoongConfigDefinition) { + if defs.StringVars == nil { + defs.StringVars = make(map[string]map[string]bool) + } + if defs.BoolVars == nil { + defs.BoolVars = make(map[string]bool) + } + if defs.ValueVars == nil { + defs.ValueVars = make(map[string]bool) + } + for _, moduleType := range mtDef.ModuleTypes { + for _, v := range moduleType.Variables { + key := strings.Join([]string{moduleType.ConfigNamespace, v.variableProperty()}, "__") + if strVar, ok := v.(*stringVariable); ok { + if _, ok := defs.StringVars[key]; !ok { + defs.StringVars[key] = make(map[string]bool, 0) + } + for _, value := range strVar.values { + defs.StringVars[key][value] = true + } + } else if _, ok := v.(*boolVariable); ok { + defs.BoolVars[key] = true + } else if _, ok := v.(*valueVariable); ok { + defs.ValueVars[key] = true + } else { + panic(fmt.Errorf("Unsupported variable type: %+v", v)) + } + } + } +} + +// This is a copy of the one available in soong/android/util.go, but depending +// on the android package causes a cyclic dependency. A refactoring here is to +// extract common utils out from android/utils.go for other packages like this. +func sortedStringKeys(m interface{}) []string { + v := reflect.ValueOf(m) + if v.Kind() != reflect.Map { + panic(fmt.Sprintf("%#v is not a map", m)) + } + keys := v.MapKeys() + s := make([]string, 0, len(keys)) + for _, key := range keys { + s = append(s, key.String()) + } + sort.Strings(s) + return s +} + +// String emits the Soong config variable definitions as Starlark dictionaries. +func (defs Bp2BuildSoongConfigDefinitions) String() string { + ret := "" + ret += "soong_config_bool_variables = {\n" + for _, boolVar := range sortedStringKeys(defs.BoolVars) { + ret += fmt.Sprintf(" \"%s\": True,\n", boolVar) + } + ret += "}\n" + ret += "\n" + + ret += "soong_config_value_variables = {\n" + for _, valueVar := range sortedStringKeys(defs.ValueVars) { + ret += fmt.Sprintf(" \"%s\": True,\n", valueVar) + } + ret += "}\n" + ret += "\n" + + ret += "soong_config_string_variables = {\n" + for _, stringVar := range sortedStringKeys(defs.StringVars) { + ret += fmt.Sprintf(" \"%s\": [\n", stringVar) + for _, choice := range sortedStringKeys(defs.StringVars[stringVar]) { + ret += fmt.Sprintf(" \"%s\",\n", choice) + } + ret += fmt.Sprintf(" ],\n") + } + ret += "}" + + return ret +} + // CreateProperties returns a reflect.Value of a newly constructed type that contains the desired // property layout for the Soong config variables, with each possible value an interface{} that // contains a nil pointer to another newly constructed type that contains the affectable properties. @@ -436,7 +523,6 @@ type ModuleType struct { affectableProperties []string variableNames []string - Bp2buildAvailable *bool } func newModuleType(props *ModuleTypeProperties) (*ModuleType, []error) { @@ -445,7 +531,6 @@ func newModuleType(props *ModuleTypeProperties) (*ModuleType, []error) { ConfigNamespace: props.Config_namespace, BaseModuleType: props.Module_type, variableNames: props.Variables, - Bp2buildAvailable: props.Bazel_module.Bp2build_available, } for _, name := range props.Bool_variables { diff --git a/android/soongconfig/modules_test.go b/android/soongconfig/modules_test.go index 48cdfe776..b14f8b43b 100644 --- a/android/soongconfig/modules_test.go +++ b/android/soongconfig/modules_test.go @@ -364,3 +364,117 @@ func Test_PropertiesToApply(t *testing.T) { } } } + +func Test_Bp2BuildSoongConfigDefinitions(t *testing.T) { + testCases := []struct { + defs Bp2BuildSoongConfigDefinitions + expected string + }{ + { + defs: Bp2BuildSoongConfigDefinitions{}, + expected: `soong_config_bool_variables = { +} + +soong_config_value_variables = { +} + +soong_config_string_variables = { +}`}, { + defs: Bp2BuildSoongConfigDefinitions{ + BoolVars: map[string]bool{ + "bool_var": true, + }, + }, + expected: `soong_config_bool_variables = { + "bool_var": True, +} + +soong_config_value_variables = { +} + +soong_config_string_variables = { +}`}, { + defs: Bp2BuildSoongConfigDefinitions{ + ValueVars: map[string]bool{ + "value_var": true, + }, + }, + expected: `soong_config_bool_variables = { +} + +soong_config_value_variables = { + "value_var": True, +} + +soong_config_string_variables = { +}`}, { + defs: Bp2BuildSoongConfigDefinitions{ + StringVars: map[string]map[string]bool{ + "string_var": map[string]bool{ + "choice1": true, + "choice2": true, + "choice3": true, + }, + }, + }, + expected: `soong_config_bool_variables = { +} + +soong_config_value_variables = { +} + +soong_config_string_variables = { + "string_var": [ + "choice1", + "choice2", + "choice3", + ], +}`}, { + defs: Bp2BuildSoongConfigDefinitions{ + BoolVars: map[string]bool{ + "bool_var_one": true, + }, + ValueVars: map[string]bool{ + "value_var_one": true, + "value_var_two": true, + }, + StringVars: map[string]map[string]bool{ + "string_var_one": map[string]bool{ + "choice1": true, + "choice2": true, + "choice3": true, + }, + "string_var_two": map[string]bool{ + "foo": true, + "bar": true, + }, + }, + }, + expected: `soong_config_bool_variables = { + "bool_var_one": True, +} + +soong_config_value_variables = { + "value_var_one": True, + "value_var_two": True, +} + +soong_config_string_variables = { + "string_var_one": [ + "choice1", + "choice2", + "choice3", + ], + "string_var_two": [ + "bar", + "foo", + ], +}`}, + } + for _, test := range testCases { + actual := test.defs.String() + if actual != test.expected { + t.Errorf("Expected:\n%s\nbut got:\n%s", test.expected, actual) + } + } +} diff --git a/bazel/properties.go b/bazel/properties.go index 76e73c588..b370bbfb2 100644 --- a/bazel/properties.go +++ b/bazel/properties.go @@ -24,23 +24,6 @@ import ( "github.com/google/blueprint" ) -type BazelModuleProperties struct { - // The label of the Bazel target replacing this Soong module. When run in conversion mode, this - // will import the handcrafted build target into the autogenerated file. Note: this may result in - // a conflict due to duplicate targets if bp2build_available is also set. - Label *string - - // If true, bp2build will generate the converted Bazel target for this module. Note: this may - // cause a conflict due to the duplicate targets if label is also set. - // - // This is a bool pointer to support tristates: true, false, not set. - // - // To opt-in a module, set bazel_module: { bp2build_available: true } - // To opt-out a module, set bazel_module: { bp2build_available: false } - // To defer the default setting for the directory, do not set the value. - Bp2build_available *bool -} - // BazelTargetModuleProperties contain properties and metadata used for // Blueprint to BUILD file conversion. type BazelTargetModuleProperties struct { diff --git a/bp2build/Android.bp b/bp2build/Android.bp index 1250e92d0..9b66354b1 100644 --- a/bp2build/Android.bp +++ b/bp2build/Android.bp @@ -18,6 +18,7 @@ bootstrap_go_package { ], deps: [ "soong-android", + "soong-android-soongconfig", "soong-apex", "soong-bazel", "soong-cc", diff --git a/bp2build/conversion.go b/bp2build/conversion.go index 944cb83a3..81a4b2624 100644 --- a/bp2build/conversion.go +++ b/bp2build/conversion.go @@ -25,6 +25,8 @@ func CreateSoongInjectionFiles(cfg android.Config, metrics CodegenMetrics) []Baz files = append(files, newFile("metrics", "converted_modules.txt", strings.Join(metrics.convertedModules, "\n"))) + files = append(files, newFile("product_config", "soong_config_variables.bzl", cfg.Bp2buildSoongConfigDefinitions.String())) + return files } diff --git a/bp2build/conversion_test.go b/bp2build/conversion_test.go index d09238a2b..3e6d9e614 100644 --- a/bp2build/conversion_test.go +++ b/bp2build/conversion_test.go @@ -82,7 +82,8 @@ func TestCreateBazelFiles_QueryView_AddsTopLevelFiles(t *testing.T) { } func TestCreateBazelFiles_Bp2Build_CreatesDefaultFiles(t *testing.T) { - files := CreateSoongInjectionFiles(android.Config{}, CodegenMetrics{}) + testConfig := android.TestConfig("", make(map[string]string), "", make(map[string][]byte)) + files := CreateSoongInjectionFiles(testConfig, CodegenMetrics{}) expectedFilePaths := []bazelFilepath{ { @@ -97,6 +98,10 @@ func TestCreateBazelFiles_Bp2Build_CreatesDefaultFiles(t *testing.T) { dir: "metrics", basename: "converted_modules.txt", }, + { + dir: "product_config", + basename: "soong_config_variables.bzl", + }, } if len(files) != len(expectedFilePaths) { diff --git a/bp2build/soong_config_module_type_conversion_test.go b/bp2build/soong_config_module_type_conversion_test.go index 523f75031..d21db0459 100644 --- a/bp2build/soong_config_module_type_conversion_test.go +++ b/bp2build/soong_config_module_type_conversion_test.go @@ -44,7 +44,6 @@ soong_config_module_type { config_namespace: "acme", bool_variables: ["feature1"], properties: ["cflags"], - bazel_module: { bp2build_available: true }, } custom_cc_library_static { @@ -85,7 +84,6 @@ soong_config_module_type { config_namespace: "acme", bool_variables: ["feature1"], properties: ["cflags"], - bazel_module: { bp2build_available: true }, } ` bp := ` @@ -140,7 +138,6 @@ soong_config_module_type { config_namespace: "acme", variables: ["board"], properties: ["cflags"], - bazel_module: { bp2build_available: true }, } custom_cc_library_static { @@ -201,7 +198,6 @@ soong_config_module_type { config_namespace: "acme", variables: ["feature1", "feature2", "board"], properties: ["cflags"], - bazel_module: { bp2build_available: true }, } custom_cc_library_static { @@ -271,7 +267,6 @@ soong_config_module_type { config_namespace: "acme", variables: ["board"], properties: ["cflags", "static_libs"], - bazel_module: { bp2build_available: true }, } custom_cc_library_static { @@ -335,7 +330,6 @@ soong_config_module_type { config_namespace: "vendor_foo", bool_variables: ["feature"], properties: ["cflags", "cppflags"], - bazel_module: { bp2build_available: true }, } vendor_foo_cc_defaults { @@ -400,7 +394,6 @@ soong_config_module_type { config_namespace: "acme", bool_variables: ["feature"], properties: ["cflags"], - bazel_module: { bp2build_available: true }, } soong_config_module_type { @@ -409,7 +402,6 @@ soong_config_module_type { config_namespace: "acme", bool_variables: ["feature"], properties: ["cflags", "asflags"], - bazel_module: { bp2build_available: true }, } foo_cc_defaults { @@ -506,7 +498,6 @@ soong_config_module_type { config_namespace: "vendor_foo", bool_variables: ["feature"], properties: ["cflags"], - bazel_module: { bp2build_available: true }, } soong_config_module_type { @@ -515,7 +506,6 @@ soong_config_module_type { config_namespace: "vendor_bar", bool_variables: ["feature"], properties: ["cflags"], - bazel_module: { bp2build_available: true }, } soong_config_module_type { @@ -524,7 +514,6 @@ soong_config_module_type { config_namespace: "vendor_qux", bool_variables: ["feature"], properties: ["cflags"], - bazel_module: { bp2build_available: true }, } vendor_foo_cc_defaults { @@ -611,7 +600,6 @@ soong_config_module_type { "shared_libs", "static_libs", ], - bazel_module: { bp2build_available: true }, } library_linking_strategy_cc_defaults { @@ -711,7 +699,6 @@ soong_config_module_type { "shared_libs", "static_libs", ], - bazel_module: { bp2build_available: true }, } library_linking_strategy_cc_defaults { @@ -796,7 +783,6 @@ soong_config_module_type { "shared_libs", "static_libs", ], - bazel_module: { bp2build_available: true }, } alphabet_cc_defaults {