From e33363598325fd35e302fccce6927d0a29086630 Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Thu, 2 Jan 2020 19:10:38 -0800 Subject: [PATCH] Add option to limit environment variables given to ninja Ninja does not track changes in environment variables, so we get potentially incorrect builds when environment variables change during incremental builds if some action was using one of them. Add a variable to limit exposure of these variables to ninja, and thus, all actions run by ninja. Kati and Soong can still read environment variables, they explicitly track which ones they read so that we can re-run them appropriately. This list is just the beginning, there's no good way to detect which environment variables are currently being used and to pass them through. So this initial change won't have a behavioral change, and we'll flip the switch and see what fails or who complains, flipping it off and on and adding to the list until we can make this always happen. Also adds a board-specific `BUILD_BROKEN_NINJA_USES_ENV_VARS := ...` list so that we can temporarily allow board-specific variables until they're fixed. Test: check out/soong.log Test: ALLOW_NINJA_ENV=false m nothing; check out/soong.log Test: set BUILD_BROKEN_NINJA_USES_ENV_VARS := OLDPWD ALLOW_NINJA_ENV=false m nothing; check out/soong.log Change-Id: I08e4834ce12100a577ef7d6a9a21b9e9d345cb93 --- ui/build/config.go | 13 +++++++-- ui/build/dumpvars.go | 4 +++ ui/build/ninja.go | 66 ++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 79 insertions(+), 4 deletions(-) diff --git a/ui/build/config.go b/ui/build/config.go index fae569f09..c0841713a 100644 --- a/ui/build/config.go +++ b/ui/build/config.go @@ -55,8 +55,9 @@ type configImpl struct { pdkBuild bool - brokenDupRules bool - brokenUsesNetwork bool + brokenDupRules bool + brokenUsesNetwork bool + brokenNinjaEnvVars []string pathReplaced bool } @@ -907,6 +908,14 @@ func (c *configImpl) BuildBrokenUsesNetwork() bool { return c.brokenUsesNetwork } +func (c *configImpl) SetBuildBrokenNinjaUsesEnvVars(val []string) { + c.brokenNinjaEnvVars = val +} + +func (c *configImpl) BuildBrokenNinjaUsesEnvVars() []string { + return c.brokenNinjaEnvVars +} + func (c *configImpl) SetTargetDeviceDir(dir string) { c.targetDeviceDir = dir } diff --git a/ui/build/dumpvars.go b/ui/build/dumpvars.go index 4270bb195..c3da38bcd 100644 --- a/ui/build/dumpvars.go +++ b/ui/build/dumpvars.go @@ -216,6 +216,9 @@ func runMakeProductConfig(ctx Context, config Config) { // Whether to enable the network during the build "BUILD_BROKEN_USES_NETWORK", + // Extra environment variables to be exported to ninja + "BUILD_BROKEN_NINJA_USES_ENV_VARS", + // Not used, but useful to be in the soong.log "BOARD_VNDK_VERSION", @@ -284,4 +287,5 @@ func runMakeProductConfig(ctx Context, config Config) { config.SetPdkBuild(make_vars["TARGET_BUILD_PDK"] == "true") config.SetBuildBrokenDupRules(make_vars["BUILD_BROKEN_DUP_RULES"] == "true") config.SetBuildBrokenUsesNetwork(make_vars["BUILD_BROKEN_USES_NETWORK"] == "true") + config.SetBuildBrokenNinjaUsesEnvVars(strings.Fields(make_vars["BUILD_BROKEN_NINJA_USES_ENV_VARS"])) } diff --git a/ui/build/ninja.go b/ui/build/ninja.go index d5baafe75..5357d44cc 100644 --- a/ui/build/ninja.go +++ b/ui/build/ninja.go @@ -18,6 +18,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "strconv" "strings" "time" @@ -65,8 +66,6 @@ func runNinja(ctx Context, config Config) { cmd.Environment.AppendFromKati(config.KatiEnvFile()) } - cmd.Environment.Set("DIST_DIR", config.DistDir()) - // Allow both NINJA_ARGS and NINJA_EXTRA_ARGS, since both have been // used in the past to specify extra ninja arguments. if extra, ok := cmd.Environment.Get("NINJA_ARGS"); ok { @@ -85,6 +84,69 @@ func runNinja(ctx Context, config Config) { ninjaHeartbeatDuration = overrideDuration } } + + // Filter the environment, as ninja does not rebuild files when environment variables change. + // + // Anything listed here must not change the output of rules/actions when the value changes, + // otherwise incremental builds may be unsafe. Vars explicitly set to stable values + // elsewhere in soong_ui are fine. + // + // For the majority of cases, either Soong or the makefiles should be replicating any + // necessary environment variables in the command line of each action that needs it. + if cmd.Environment.IsFalse("ALLOW_NINJA_ENV") { + cmd.Environment.Allow(append([]string{ + "ASAN_SYMBOLIZER_PATH", + "HOME", + "JAVA_HOME", + "LANG", + "LC_MESSAGES", + "OUT_DIR", + "PATH", + "PWD", + "PYTHONDONTWRITEBYTECODE", + "TMPDIR", + "USER", + + // TODO: remove these carefully + "TARGET_BUILD_APPS", + "TARGET_BUILD_VARIANT", + "TARGET_PRODUCT", + + // Goma -- gomacc may not need all of these + "GOMA_DIR", + "GOMA_DISABLED", + "GOMA_FAIL_FAST", + "GOMA_FALLBACK", + "GOMA_GCE_SERVICE_ACCOUNT", + "GOMA_TMP_DIR", + "GOMA_USE_LOCAL", + + // RBE client + "FLAG_exec_root", + "FLAG_exec_strategy", + "FLAG_invocation_id", + "FLAG_log_dir", + "FLAG_platform", + "FLAG_server_address", + + // ccache settings + "CCACHE_COMPILERCHECK", + "CCACHE_SLOPPINESS", + "CCACHE_BASEDIR", + "CCACHE_CPP2", + }, config.BuildBrokenNinjaUsesEnvVars()...)...) + } + + cmd.Environment.Set("DIST_DIR", config.DistDir()) + cmd.Environment.Set("SHELL", "/bin/bash") + + ctx.Verboseln("Ninja environment: ") + envVars := cmd.Environment.Environ() + sort.Strings(envVars) + for _, envVar := range envVars { + ctx.Verbosef(" %s", envVar) + } + // Poll the ninja log for updates; if it isn't updated enough, then we want to show some diagnostics done := make(chan struct{}) defer close(done)