From f47e142ffdbfb8e6d39744e5a5f20127467d4d44 Mon Sep 17 00:00:00 2001 From: MarkDacek Date: Wed, 19 Apr 2023 16:47:36 +0000 Subject: [PATCH] Refactor MixedBuildsEnabled and add --ensure-allowlist-integrity. Currently, there is little verification around allowlisted modules actually being mixed-built. This flag would allow us to verify that a module allowlisted is mixed-built for at least one variant. Bug: 278910100 Test: m nothing --bazel-mode-staging --ensure-allowlist-integrity Test: m nothing --bazel-mode-staging --ensure-allowlist-integrity --bazel-force-enabled-modules=com.google.android.neuralnetworks (This fails, as expected) Test: build/soong/test/mixed_mode_test.sh Change-Id: Icd5976f4f44f1a8caca1e5247d986642f7995f97 --- android/bazel.go | 50 +++++++++++++++++++++++++++++++-------- android/bazel_handler.go | 6 ++--- android/config.go | 11 +++++++++ android/module.go | 2 +- cc/cc_test.go | 2 +- cmd/soong_build/main.go | 50 +++++++++++++++++++++++++++++++++++++-- tests/mixed_mode_test.sh | 2 +- ui/build/config.go | 51 +++++++++++++++++++++++----------------- ui/build/soong.go | 3 +++ 9 files changed, 136 insertions(+), 41 deletions(-) diff --git a/android/bazel.go b/android/bazel.go index 164688396..58d9d87db 100644 --- a/android/bazel.go +++ b/android/bazel.go @@ -32,6 +32,22 @@ const ( Bp2BuildTopLevel = "." ) +type MixedBuildEnabledStatus int + +const ( + // This module can be mixed_built. + MixedBuildEnabled = iota + + // There is a technical incompatibility preventing this module from being + // bazel-analyzed. Note: the module might also be incompatible. + TechnicalIncompatibility + + // This module cannot be mixed_built due to some incompatibility with it + // that is not a platform incompatibility. Example: the module-type is not + // enabled, or is not bp2build-converted. + ModuleIncompatibility +) + // FileGroupAsLibrary describes a filegroup module that is converted to some library // such as aidl_library or proto_library. type FileGroupAsLibrary interface { @@ -346,24 +362,31 @@ func GetBp2BuildAllowList() Bp2BuildConversionAllowlist { }).(Bp2BuildConversionAllowlist) } -// MixedBuildsEnabled returns true if a module is ready to be replaced by a -// converted or handcrafted Bazel target. As a side effect, calling this -// method will also log whether this module is mixed build enabled for -// metrics reporting. -func MixedBuildsEnabled(ctx BaseModuleContext) bool { +// MixedBuildsEnabled returns a MixedBuildEnabledStatus regarding whether +// a module is ready to be replaced by a converted or handcrafted Bazel target. +// As a side effect, calling this method will also log whether this module is +// mixed build enabled for metrics reporting. +func MixedBuildsEnabled(ctx BaseModuleContext) MixedBuildEnabledStatus { module := ctx.Module() apexInfo := ctx.Provider(ApexInfoProvider).(ApexInfo) withinApex := !apexInfo.IsForPlatform() + + platformIncompatible := isPlatformIncompatible(ctx.Os(), ctx.Arch().ArchType) + if platformIncompatible { + ctx.Config().LogMixedBuild(ctx, false) + return TechnicalIncompatibility + } + mixedBuildEnabled := ctx.Config().IsMixedBuildsEnabled() && - ctx.Os() != Windows && // Windows toolchains are not currently supported. - ctx.Os() != LinuxBionic && // Linux Bionic toolchains are not currently supported. - ctx.Os() != LinuxMusl && // Linux musl toolchains are not currently supported (b/259266326). - ctx.Arch().ArchType != Riscv64 && // TODO(b/262192655) Riscv64 toolchains are not currently supported. module.Enabled() && convertedToBazel(ctx, module) && ctx.Config().BazelContext.IsModuleNameAllowed(module.Name(), withinApex) ctx.Config().LogMixedBuild(ctx, mixedBuildEnabled) - return mixedBuildEnabled + + if mixedBuildEnabled { + return MixedBuildEnabled + } + return ModuleIncompatibility } // ConvertedToBazel returns whether this module has been converted (with bp2build or manually) to Bazel. @@ -388,6 +411,13 @@ type bazelOtherModuleContext interface { OtherModuleDir(m blueprint.Module) string } +func isPlatformIncompatible(osType OsType, arch ArchType) bool { + return osType == Windows || // Windows toolchains are not currently supported. + osType == LinuxBionic || // Linux Bionic toolchains are not currently supported. + osType == LinuxMusl || // Linux musl toolchains are not currently supported (b/259266326). + arch == Riscv64 // TODO(b/262192655) Riscv64 toolchains are not currently supported. +} + func (b *BazelModuleBase) shouldConvertWithBp2build(ctx bazelOtherModuleContext, module blueprint.Module) bool { if !b.bazelProps().Bazel_module.CanConvertToBazel { return false diff --git a/android/bazel_handler.go b/android/bazel_handler.go index 9c273d9a3..debc7a2fd 100644 --- a/android/bazel_handler.go +++ b/android/bazel_handler.go @@ -86,12 +86,10 @@ func RegisterMixedBuildsMutator(ctx RegistrationContext) { func mixedBuildsPrepareMutator(ctx BottomUpMutatorContext) { if m := ctx.Module(); m.Enabled() { if mixedBuildMod, ok := m.(MixedBuildBuildable); ok { - queueMixedBuild := mixedBuildMod.IsMixedBuildSupported(ctx) && MixedBuildsEnabled(ctx) + mixedBuildEnabled := MixedBuildsEnabled(ctx) + queueMixedBuild := mixedBuildMod.IsMixedBuildSupported(ctx) && mixedBuildEnabled == MixedBuildEnabled if queueMixedBuild { mixedBuildMod.QueueBazelCall(ctx) - } else if _, ok := ctx.Config().bazelForceEnabledModules[m.Name()]; ok { - // TODO(b/273910287) - remove this once --ensure_allowlist_integrity is added - ctx.ModuleErrorf("Attempted to force enable an unready module: %s. Did you forget to Bp2BuildDefaultTrue its directory?\n", m.Name()) } } } diff --git a/android/config.go b/android/config.go index 032172d6e..7141e54c3 100644 --- a/android/config.go +++ b/android/config.go @@ -102,6 +102,8 @@ type CmdArgs struct { UseBazelProxy bool BuildFromTextStub bool + + EnsureAllowlistIntegrity bool } // Build modes that soong_build can run as. @@ -278,6 +280,11 @@ type config struct { // If buildFromTextStub is true then the Java API stubs are // built from the signature text files, not the source Java files. buildFromTextStub bool + + // If ensureAllowlistIntegrity is true, then the presence of any allowlisted + // modules that aren't mixed-built for at least one variant will cause a build + // failure + ensureAllowlistIntegrity bool } type deviceConfig struct { @@ -1904,6 +1911,10 @@ func (c *config) UseHostMusl() bool { return Bool(c.productVariables.HostMusl) } +func (c *config) GetMixedBuildsEnabledModules() map[string]struct{} { + return c.mixedBuildEnabledModules +} + func (c *config) LogMixedBuild(ctx BaseModuleContext, useBazel bool) { moduleName := ctx.Module().Name() c.mixedBuildsLock.Lock() diff --git a/android/module.go b/android/module.go index ba474530d..c8670c31a 100644 --- a/android/module.go +++ b/android/module.go @@ -2438,7 +2438,7 @@ func (m *ModuleBase) GenerateBuildActions(blueprintCtx blueprint.ModuleContext) func (m *ModuleBase) isHandledByBazel(ctx ModuleContext) (MixedBuildBuildable, bool) { if mixedBuildMod, ok := m.module.(MixedBuildBuildable); ok { - if mixedBuildMod.IsMixedBuildSupported(ctx) && MixedBuildsEnabled(ctx) { + if mixedBuildMod.IsMixedBuildSupported(ctx) && (MixedBuildsEnabled(ctx) == MixedBuildEnabled) { return mixedBuildMod, true } } diff --git a/cc/cc_test.go b/cc/cc_test.go index 787669c75..fde6bffc2 100644 --- a/cc/cc_test.go +++ b/cc/cc_test.go @@ -59,7 +59,7 @@ func registerTestMutators(ctx android.RegistrationContext) { func mixedBuildsPrepareMutator(ctx android.BottomUpMutatorContext) { if m := ctx.Module(); m.Enabled() { if mixedBuildMod, ok := m.(android.MixedBuildBuildable); ok { - if mixedBuildMod.IsMixedBuildSupported(ctx) && android.MixedBuildsEnabled(ctx) { + if mixedBuildMod.IsMixedBuildSupported(ctx) && android.MixedBuildsEnabled(ctx) == android.MixedBuildEnabled { mixedBuildMod.QueueBazelCall(ctx) } } diff --git a/cmd/soong_build/main.go b/cmd/soong_build/main.go index 79a5ce498..53e0e55c4 100644 --- a/cmd/soong_build/main.go +++ b/cmd/soong_build/main.go @@ -85,7 +85,7 @@ func init() { flag.BoolVar(&cmdlineArgs.BazelModeDev, "bazel-mode-dev", false, "use bazel for analysis of a large number of modules (less stable)") flag.BoolVar(&cmdlineArgs.UseBazelProxy, "use-bazel-proxy", false, "communicate with bazel using unix socket proxy instead of spawning subprocesses") flag.BoolVar(&cmdlineArgs.BuildFromTextStub, "build-from-text-stub", false, "build Java stubs from API text files instead of source files") - + flag.BoolVar(&cmdlineArgs.EnsureAllowlistIntegrity, "ensure-allowlist-integrity", false, "verify that allowlisted modules are mixed-built") // Flags that probably shouldn't be flags of soong_build, but we haven't found // the time to remove them yet flag.BoolVar(&cmdlineArgs.RunGoTests, "t", false, "build and run go tests during bootstrap") @@ -288,6 +288,46 @@ func writeMetrics(configuration android.Config, eventHandler *metrics.EventHandl maybeQuit(err, "error writing soong_build metrics %s", metricsFile) } +// Errors out if any modules expected to be mixed_built were not, unless +// there is a platform incompatibility. +func checkForAllowlistIntegrityError(configuration android.Config, isStagingMode bool) error { + modules := findModulesNotMixedBuiltForAnyVariant(configuration, isStagingMode) + if len(modules) == 0 { + return nil + } + + return fmt.Errorf("Error: expected the following modules to be mixed_built: %s", modules) +} + +// Returns the list of modules that should have been mixed_built (per the +// allowlists and cmdline flags) but were not. +func findModulesNotMixedBuiltForAnyVariant(configuration android.Config, isStagingMode bool) []string { + retval := []string{} + forceEnabledModules := configuration.BazelModulesForceEnabledByFlag() + + mixedBuildsEnabled := configuration.GetMixedBuildsEnabledModules() + for _, module := range allowlists.ProdMixedBuildsEnabledList { + if _, ok := mixedBuildsEnabled[module]; !ok && module != "" { + retval = append(retval, module) + } + } + + if isStagingMode { + for _, module := range allowlists.StagingMixedBuildsEnabledList { + if _, ok := mixedBuildsEnabled[module]; !ok && module != "" { + retval = append(retval, module) + } + } + } + + for module, _ := range forceEnabledModules { + if _, ok := mixedBuildsEnabled[module]; !ok && module != "" { + retval = append(retval, module) + } + } + return retval +} + func writeJsonModuleGraphAndActions(ctx *android.Context, cmdArgs android.CmdArgs) { graphFile, graphErr := os.Create(shared.JoinPath(topDir, cmdArgs.ModuleGraphFile)) maybeQuit(graphErr, "graph err") @@ -433,8 +473,14 @@ func main() { writeMetrics(configuration, ctx.EventHandler, metricsDir) default: ctx.Register() - if configuration.IsMixedBuildsEnabled() { + isMixedBuildsEnabled := configuration.IsMixedBuildsEnabled() + if isMixedBuildsEnabled { finalOutputFile = runMixedModeBuild(ctx, extraNinjaDeps) + if cmdlineArgs.EnsureAllowlistIntegrity { + if err := checkForAllowlistIntegrityError(configuration, cmdlineArgs.BazelModeStaging); err != nil { + maybeQuit(err, "") + } + } } else { finalOutputFile = runSoongOnlyBuild(ctx, extraNinjaDeps) } diff --git a/tests/mixed_mode_test.sh b/tests/mixed_mode_test.sh index a1a792de3..ca63fdfe4 100755 --- a/tests/mixed_mode_test.sh +++ b/tests/mixed_mode_test.sh @@ -88,7 +88,7 @@ EOF fail "Bazel actions not found for force-enabled module" fi - unused=`run_soong --bazel-force-enabled-modules=unenabled-touch-file nothing >/dev/null` + unused=`run_soong --bazel-force-enabled-modules=unenabled-touch-file --ensure-allowlist-integrity nothing >/dev/null` if [[ $? -ne 1 ]]; then fail "Expected failure due to force-enabling an unenabled module " diff --git a/ui/build/config.go b/ui/build/config.go index bf4aec9b9..81ea69e86 100644 --- a/ui/build/config.go +++ b/ui/build/config.go @@ -67,28 +67,29 @@ type configImpl struct { logsPrefix string // From the arguments - parallel int - keepGoing int - verbose bool - checkbuild bool - dist bool - jsonModuleGraph bool - apiBp2build bool // Generate BUILD files for Soong modules that contribute APIs - bp2build bool - queryview bool - reportMkMetrics bool // Collect and report mk2bp migration progress metrics. - soongDocs bool - multitreeBuild bool // This is a multitree build. - skipConfig bool - skipKati bool - skipKatiNinja bool - skipSoong bool - skipNinja bool - skipSoongTests bool - searchApiDir bool // Scan the Android.bp files generated in out/api_surfaces - skipMetricsUpload bool - buildStartedTime int64 // For metrics-upload-only - manually specify a build-started time - buildFromTextStub bool + parallel int + keepGoing int + verbose bool + checkbuild bool + dist bool + jsonModuleGraph bool + apiBp2build bool // Generate BUILD files for Soong modules that contribute APIs + bp2build bool + queryview bool + reportMkMetrics bool // Collect and report mk2bp migration progress metrics. + soongDocs bool + multitreeBuild bool // This is a multitree build. + skipConfig bool + skipKati bool + skipKatiNinja bool + skipSoong bool + skipNinja bool + skipSoongTests bool + searchApiDir bool // Scan the Android.bp files generated in out/api_surfaces + skipMetricsUpload bool + buildStartedTime int64 // For metrics-upload-only - manually specify a build-started time + buildFromTextStub bool + ensureAllowlistIntegrity bool // For CI builds - make sure modules are mixed-built // From the product config katiArgs []string @@ -883,6 +884,8 @@ func (c *configImpl) parseArgs(ctx Context, args []string) { } else { ctx.Fatalf("Error parsing build-time-started-unix-millis", err) } + } else if arg == "--ensure-allowlist-integrity" { + c.ensureAllowlistIntegrity = true } else if len(arg) > 0 && arg[0] == '-' { parseArgNum := func(def int) int { if len(arg) > 2 { @@ -1710,6 +1713,10 @@ func (c *configImpl) SkipMetricsUpload() bool { return c.skipMetricsUpload } +func (c *configImpl) EnsureAllowlistIntegrity() bool { + return c.ensureAllowlistIntegrity +} + // Returns a Time object if one was passed via a command-line flag. // Otherwise returns the passed default. func (c *configImpl) BuildStartedTimeOrDefault(defaultTime time.Time) time.Time { diff --git a/ui/build/soong.go b/ui/build/soong.go index 9287731f1..563199bee 100644 --- a/ui/build/soong.go +++ b/ui/build/soong.go @@ -288,6 +288,9 @@ func bootstrapBlueprint(ctx Context, config Config) { if config.buildFromTextStub { mainSoongBuildExtraArgs = append(mainSoongBuildExtraArgs, "--build-from-text-stub") } + if config.ensureAllowlistIntegrity { + mainSoongBuildExtraArgs = append(mainSoongBuildExtraArgs, "--ensure-allowlist-integrity") + } queryviewDir := filepath.Join(config.SoongOutDir(), "queryview") // The BUILD files will be generated in out/soong/.api_bp2build (no symlinks to src files)