From 0b4fccb66dc33e6cc28c0e638ee834fbf8f53c50 Mon Sep 17 00:00:00 2001 From: Jiyong Park Date: Fri, 26 Jun 2020 17:38:00 +0900 Subject: [PATCH] BUILD_BROKEN_DUP_SYSPROP as escape hatch for the new sysprop restriction As the final step for the refactoring of sysprop configuration, this change adds BUILD_BROKEN_DUP_SYSPROP which is the escape hatch for the new restriction. When it is turned on, the new syntax `a ?= b` collapses to the old syntax `a = b`, duplicated assignments are allowed, and the dups are resolved following the legacy rule of preferring the first. This change also summarizes all the user-facing changes to the Change.md file. Lastly, post_process_prop.py is refactored to accept new argument '--allow-dup' which when turned on allowes duplicated sysprops. Bug: 117892318 Bug: 158735147 Test: atest --host post_process_prop_unittest Exempt-From-Owner-Approval: cherry-pick from master Merged-In: I7bdfffd47d50aad66a78e28a30c3dad7ebac080c (cherry picked from commit b302cdf6a417b9c8eaedee713187735a74707d6a) Change-Id: I7bdfffd47d50aad66a78e28a30c3dad7ebac080c --- Changes.md | 87 ++++++++++++++++++++++++++++++++ core/board_config.mk | 1 + core/sysprop.mk | 13 ++++- tools/post_process_props.py | 29 ++++++++--- tools/test_post_process_props.py | 19 +++++++ 5 files changed, 141 insertions(+), 8 deletions(-) diff --git a/Changes.md b/Changes.md index 453ea6c7e0..61e6bb66c5 100644 --- a/Changes.md +++ b/Changes.md @@ -1,5 +1,92 @@ # Build System Changes for Android.mk Writers +## Changes in system properties settings + +### Product variables + +System properties for each of the partition is supposed to be set via following +product config variables. + +For system partititon, + +* `PRODUCT_SYSTEM_PROPERITES` +* `PRODUCT_SYSTEM_DEFAULT_PROPERTIES` is highly discouraged. Will be deprecated. + +For vendor partition, + +* `PRODUCT_VENDOR_PROPERTIES` +* `PRODUCT_PROPERTY_OVERRIDES` is highly discouraged. Will be deprecated. +* `PRODUCT_DEFAULT_PROPERTY_OVERRIDES` is also discouraged. Will be deprecated. + +For odm partition, + +* `PRODUCT_ODM_PROPERTIES` + +For system_ext partition, + +* `PRODUCT_SYSTEM_EXT_PROPERTIES` + +For product partition, + +* `PRODUCT_PRODUCT_PROPERTIES` + +### Duplication is not allowed within a partition + +For each partition, having multiple sysprop assignments for the same name is +prohibited. For example, the following will now trigger an error: + +`PRODUCT_VENDOR_PROPERTIES += foo=true foo=false` + +Having duplication across partitions are still allowed. So, the following is +not an error: + +`PRODUCT_VENDOR_PROPERTIES += foo=true` +`PRODUCT_SYSTEM_PROPERTIES += foo=false` + +In that case, the final value is determined at runtime. The precedence is + +* product +* odm +* vendor +* system_ext +* system + +So, `foo` becomes `true` because vendor has higher priority than system. + +To temporarily turn the build-time restriction off, use + +`BUILD_BROKEN_DUP_SYSPROP := true` + +### Optional assignments + +System properties can now be set as optional using the new syntax: + +`name ?= value` + +Then the system property named `name` gets the value `value` only when there +is no other non-optional assignments having the same name. For example, the +following is allowed and `foo` gets `true` + +`PRODUCT_VENDOR_PROPERTIES += foo=true foo?=false` + +Note that the order between the optional and the non-optional assignments +doesn't matter. The following gives the same result as above. + +`PRODUCT_VENDOR_PROPERTIES += foo?=false foo=true` + +Optional assignments can be duplicated and in that case their order matters. +Specifically, the last one eclipses others. + +`PRODUCT_VENDOR_PROPERTIES += foo?=apple foo?=banana foo?=mango` + +With above, `foo` becomes `mango` since its the last one. + +Note that this behavior is different from the previous behavior of preferring +the first one. To go back to the original behavior for compatability reason, +use: + +`BUILD_BROKEN_DUP_SYSPROP := true` + ## ELF prebuilts in PRODUCT_COPY_FILES ELF prebuilts in PRODUCT_COPY_FILES that are installed into these paths are an diff --git a/core/board_config.mk b/core/board_config.mk index ae1614f24f..1fbd9898df 100644 --- a/core/board_config.mk +++ b/core/board_config.mk @@ -93,6 +93,7 @@ _build_broken_var_list := \ BUILD_BROKEN_TREBLE_SYSPROP_NEVERALLOW \ BUILD_BROKEN_USES_NETWORK \ BUILD_BROKEN_VINTF_PRODUCT_COPY_FILES \ + BUILD_BROKEN_DUP_SYSPROP \ _build_broken_var_list += \ $(foreach m,$(AVAILABLE_BUILD_MODULE_TYPES) \ diff --git a/core/sysprop.mk b/core/sysprop.mk index 3806a9691a..aa4fe91889 100644 --- a/core/sysprop.mk +++ b/core/sysprop.mk @@ -78,6 +78,17 @@ $(foreach name,$(strip $(4)),\ $(eval _resolved_$(name) := $$(call collapse-pairs,$$(_temp),=))\ ) +$(eval # Implement the legacy behavior when BUILD_BROKEN_DUP_SYSPROP is on.) +$(eval # Optional assignments are all converted to normal assignments and) +$(eval # when their duplicates the first one wins) +$(if $(filter true,$(BUILD_BROKEN_DUP_SYSPROP)),\ + $(foreach name,$(strip $(4)),\ + $(eval _temp := $$(subst ?=,=,$$(_resolved_$(name))))\ + $(eval _resolved_$(name) := $$(call uniq-pairs-by-first-component,$$(_resolved_$(name)),=))\ + )\ + $(eval _option := --allow-dup)\ +) + $(2): $(POST_PROCESS_PROPS) $(INTERNAL_BUILD_ID_MAKEFILE) $(API_FINGERPRINT) $(3) $(hide) echo Building $$@ $(hide) mkdir -p $$(dir $$@) @@ -100,7 +111,7 @@ $(2): $(POST_PROCESS_PROPS) $(INTERNAL_BUILD_ID_MAKEFILE) $(API_FINGERPRINT) $(3 echo "$$(line)" >> $$@;\ )\ ) - $(hide) $(POST_PROCESS_PROPS) $$@ $(5) + $(hide) $(POST_PROCESS_PROPS) $$(_option) $$@ $(5) $(hide) echo "# end of file" >> $$@ endef diff --git a/tools/post_process_props.py b/tools/post_process_props.py index 397526f05e..78a23fb72d 100755 --- a/tools/post_process_props.py +++ b/tools/post_process_props.py @@ -14,6 +14,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import argparse import sys # Usage: post_process_props.py file.prop [disallowed_key, ...] @@ -69,7 +70,7 @@ def validate(prop_list): return check_pass -def override_optional_props(prop_list): +def override_optional_props(prop_list, allow_dup=False): """Override a?=b with a=c, if the latter exists Overriding is done by deleting a?=b @@ -88,6 +89,15 @@ def override_optional_props(prop_list): # duplicated props are allowed when the all have the same value if all(overriding_props[0].value == p.value for p in overriding_props): continue + # or if dup is explicitly allowed for compat reason + if allow_dup: + # this could left one or more optional props unresolved. + # Convert them into non-optional because init doesn't understand ?= + # syntax + for p in optional_props: + p.optional = False + continue + success = False sys.stderr.write("error: found duplicate sysprop assignments:\n") for p in overriding_props: @@ -198,25 +208,30 @@ class PropList: f.write(str(p) + "\n") def main(argv): - filename = argv[1] + parser = argparse.ArgumentParser(description="Post-process build.prop file") + parser.add_argument("--allow-dup", dest="allow_dup", action="store_true", + default=False) + parser.add_argument("filename") + parser.add_argument("disallowed_keys", metavar="KEY", type=str, nargs="*") + args = parser.parse_args() - if not filename.endswith("/build.prop"): + if not args.filename.endswith("/build.prop"): sys.stderr.write("bad command line: " + str(argv) + "\n") sys.exit(1) - props = PropList(filename) + props = PropList(args.filename) mangle_build_prop(props) - if not override_optional_props(props): + if not override_optional_props(props, args.allow_dup): sys.exit(1) if not validate(props): sys.exit(1) # Drop any disallowed keys - for key in argv[2:]: + for key in args.disallowed_keys: for p in props.get_props(key): p.delete("%s is a disallowed key" % key) - props.write(filename) + props.write(args.filename) if __name__ == "__main__": main(sys.argv) diff --git a/tools/test_post_process_props.py b/tools/test_post_process_props.py index 8a9c3edc61..44fe957d74 100644 --- a/tools/test_post_process_props.py +++ b/tools/test_post_process_props.py @@ -226,5 +226,24 @@ class PropListTestcase(unittest.TestCase): # since they have the same value self.assertTrue(override_optional_props(props)) + def test_allowDuplicates(self): + content = """ + # comment + foo=true + bar=false + qux?=1 + foo=false + # another comment + foo?=false + """ + with patch("post_process_props.open", mock_open(read_data=content)) as m: + stderr_redirect = io.StringIO() + with contextlib.redirect_stderr(stderr_redirect): + props = PropList("hello") + + # we have duplicated foo=true and foo=false, but that's allowed + # because it's explicitly allowed + self.assertTrue(override_optional_props(props, allow_dup=True)) + if __name__ == '__main__': unittest.main(verbosity=2)