From 8b9c3cc54b89dc191d8333d137f7533d4ef1daa6 Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Tue, 27 Feb 2018 02:15:32 -0800 Subject: [PATCH] Mark export/unexport as deprecated Make it so that `export`/`unexport` are deprecated during product configuration, but obsolete during Android.mk parsing (and later in the build, since we can't un-obsolete it). Remove some ccache / goma exports, those don't need to be exports, since soong_ui asks about them explicitly. They also only run doing the initial project configuration, so we don't run anything with that environment. Bug: 73959648 Test: m nothing Test: build_test on all downstream branches Change-Id: I55a749f46775660439ae57e881a02c914e83de16 --- Changes.md | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ core/ccache.mk | 8 ++++---- core/config.mk | 7 +++---- core/envsetup.mk | 8 ++++++++ core/goma.mk | 3 --- 5 files changed, 65 insertions(+), 11 deletions(-) diff --git a/Changes.md b/Changes.md index 2583ae269..140ceaa99 100644 --- a/Changes.md +++ b/Changes.md @@ -1,5 +1,55 @@ # Build System Changes for Android.mk Writers +### `export` and `unexport` deprecation {#export_keyword} + +The `export` and `unexport` keywords have been deprecated, and will throw +warnings or errors depending on where they are used. + +Early in the make system, during product configuration and BoardConfig.mk +reading: these will throw a warnings, and will be an error in the future. +Device specific configuration should not be able to affect common core build +steps -- we're looking at triggering build steps to be invalidated if the set +of environment variables they can access changes. If device specific +configuration is allowed to change those, switching devices with the same +output directory could become significantly more expensive than it already can +be. + +Later, during Android.mk files, and later tasks: these will throw errors, since +it is increasingly likely that they are being used incorrectly, attempting to +change the environment for a single build step, and instead setting it for +hundreds of thousands. + +It is not recommended to just move the environment variable setting outside of +the build (in vendorsetup.sh, or some other configuration script or wrapper). +We expect to limit the environment variables that the build respects in the +future, others will be cleared. (There will be methods to get custom variables +into the build, just not to every build step) + +Instead, write the export commands into the rule command lines themselves: + +``` make +$(intermediates)/generated_output.img: + rm -rf $@ + export MY_ENV_A="$(MY_A)"; make ... +``` + +If you want to set many environment variables, and/or use them many times, +write them out to a script and source the script: + +``` make +envsh := $(intermediates)/env.sh +$(envsh): + rm -rf $@ + echo 'export MY_ENV_A="$(MY_A)"' >$@ + echo 'export MY_ENV_B="$(MY_B)"' >>$@ + +$(intermediates)/generated_output.img: PRIVATE_ENV := $(envsh) +$(intermediates)/generated_output.img: $(envsh) a/b/c/package.sh + rm -rf $@ + source $(PRIVATE_ENV); make ... + source $(PRIVATE_ENV); a/b/c/package.sh ... +``` + ## Implicit make rules are deprecated {#implicit_rules} Implicit rules look something like the following: diff --git a/core/ccache.mk b/core/ccache.mk index 893c98564..d10aceb01 100644 --- a/core/ccache.mk +++ b/core/ccache.mk @@ -32,24 +32,24 @@ ifneq ($(CCACHE_EXEC),) ifneq ($(filter-out false,$(USE_CCACHE)),) # The default check uses size and modification time, causing false misses # since the mtime depends when the repo was checked out - export CCACHE_COMPILERCHECK ?= content + CCACHE_COMPILERCHECK ?= content # See man page, optimizations to get more cache hits # implies that __DATE__ and __TIME__ are not critical for functionality. # Ignore include file modification time since it will depend on when # the repo was checked out - export CCACHE_SLOPPINESS := time_macros,include_file_mtime,file_macro + CCACHE_SLOPPINESS := time_macros,include_file_mtime,file_macro # Turn all preprocessor absolute paths into relative paths. # Fixes absolute paths in preprocessed source due to use of -g. # We don't really use system headers much so the rootdir is # fine; ensures these paths are relative for all Android trees # on a workstation. - export CCACHE_BASEDIR := / + CCACHE_BASEDIR := / # Workaround for ccache with clang. # See http://petereisentraut.blogspot.com/2011/09/ccache-and-clang-part-2.html - export CCACHE_CPP2 := true + CCACHE_CPP2 := true ifndef CC_WRAPPER CC_WRAPPER := $(CCACHE_EXEC) diff --git a/core/config.mk b/core/config.mk index 9f8426ce8..d8a00f260 100644 --- a/core/config.mk +++ b/core/config.mk @@ -85,6 +85,9 @@ $(KATI_obsolete_var \ $(KATI_obsolete_var PRODUCT_COMPATIBILITY_MATRIX_LEVEL_OVERRIDE,Set FCM Version in device manifest instead. See $(CHANGES_URL)#PRODUCT_COMPATIBILITY_MATRIX_LEVEL_OVERRIDE) $(KATI_obsolete_var USE_CLANG_PLATFORM_BUILD,Clang is the only supported Android compiler. See $(CHANGES_URL)#USE_CLANG_PLATFORM_BUILD) +# This is marked as obsolete in envsetup.mk after reading the BoardConfig.mk +$(KATI_deprecate_export It is a global setting. See $(CHANGES_URL)#export_keyword) + CHANGES_URL := # Used to force goals to build. Only use for conditionally defined goals. @@ -358,10 +361,6 @@ endif ifeq ($(CALLED_FROM_SETUP),true) include $(BUILD_SYSTEM)/ccache.mk include $(BUILD_SYSTEM)/goma.mk - -export CC_WRAPPER -export CXX_WRAPPER -export JAVAC_WRAPPER endif ifdef TARGET_PREFER_32_BIT diff --git a/core/envsetup.mk b/core/envsetup.mk index 223521976..874ea916c 100644 --- a/core/envsetup.mk +++ b/core/envsetup.mk @@ -288,6 +288,14 @@ $(foreach var,$(vars), \ .KATI_READONLY := $(vars) +CHANGES_URL := https://android.googlesource.com/platform/build/+/master/Changes.md + +# "" is equivalent to true currently. +ifeq ($(BUILD_BROKEN_ANDROIDMK_EXPORTS),false) +$(KATI_obsolete_export It is a global setting. See $(CHANGES_URL)#export_keyword) +endif + +CHANGES_URL := ########################################### # Now we can substitute with the real value of TARGET_COPY_OUT_VENDOR diff --git a/core/goma.mk b/core/goma.mk index 2fb37a752..3787dfd19 100644 --- a/core/goma.mk +++ b/core/goma.mk @@ -14,9 +14,6 @@ # limitations under the License. # -# Used by the compiler wrapper, but should only be set by gomacc -unexport GOMACC_PATH - # Notice: this works only with Google's Goma build infrastructure. ifneq ($(filter-out false,$(USE_GOMA)),) # Goma requires a lot of processes and file descriptors.