From d721e870bcc3caf2219adaa4716a3193d3c479d0 Mon Sep 17 00:00:00 2001 From: Jiyong Park Date: Mon, 22 Jun 2020 17:30:57 +0900 Subject: [PATCH 1/7] Support optional prop assignments This CL adds a number of changes to make the assignment of system properties to be less confusing. 1. Added `a ?= b` syntax, which is called optional prop assignments. The prop `a` gets the value `b` only when there is no non-optional prop assignment for `a` such as `a = c`. This is useful for props that provide some reasonable default values as fallback. 2. With the introduction of the optional prop assignment syntax, duplicated non-optional assignments is prohibited; e.g., the follwing now triggers a build-time error: a = b a = c , but the following doesn't: a ?= b a = c Note that the textual order between the optional and non-optional assignments doesn't matter. The non-optional assignment eclipses the optional assignment even when the former appears 'before' the latter. a = c a ?= b In the above, `a` gets the value `c` When there are multiple optional assignments without a non-optional assignments as shown below, the last one wins: a ?= b a ?= c `a` becomes `c`. Specifically, the former assignment is commented out and the latter is converted to a non-optional assignment. 3. post_process_props.py is modified so that when a prop assignment is deleted, changed, or added, the changes are recorded as comments. This is to aid debugging. Previously, it was often difficult to find out why a certain sysprop assignment is missing or is added. 4. post_process_prop.py now has a unittest Bug: 117892318 Bug: 158735147 Test: atest --host post_process_prop_unittest Exempt-From-Owner-Approval: cherry-pick from master Merged-In: I9c073a21c8257987cf2378012cadaeeeb698a4fb (cherry picked from commit 7aeb8de74e08eb2d305686aa8eff45353973e7d7) Change-Id: I9c073a21c8257987cf2378012cadaeeeb698a4fb --- core/sysprop.mk | 7 +- tools/Android.bp | 19 +++ tools/post_process_props.py | 138 ++++++++++++---- tools/post_process_props_unittest.xml | 6 + tools/test_post_process_props.py | 230 ++++++++++++++++++++++++++ 5 files changed, 369 insertions(+), 31 deletions(-) create mode 100644 tools/post_process_props_unittest.xml create mode 100644 tools/test_post_process_props.py diff --git a/core/sysprop.mk b/core/sysprop.mk index 0cee81c6c..3806a9691 100644 --- a/core/sysprop.mk +++ b/core/sysprop.mk @@ -71,10 +71,11 @@ endef define build-properties ALL_DEFAULT_INSTALLED_MODULES += $(2) -# TODO(b/117892318): eliminate the call to uniq-pairs-by-first-component when -# it is guaranteed that there is no dup. +$(eval # Properties can be assigned using `prop ?= value` or `prop = value` syntax.) +$(eval # Eliminate spaces around the ?= and = separators.) $(foreach name,$(strip $(4)),\ - $(eval _resolved_$(name) := $$(call collapse-pairs, $$(call uniq-pairs-by-first-component,$$($(name)),=)))\ + $(eval _temp := $$(call collapse-pairs,$$($(name)),?=))\ + $(eval _resolved_$(name) := $$(call collapse-pairs,$$(_temp),=))\ ) $(2): $(POST_PROCESS_PROPS) $(INTERNAL_BUILD_ID_MAKEFILE) $(API_FINGERPRINT) $(3) diff --git a/tools/Android.bp b/tools/Android.bp index 159890ce4..149d06df4 100644 --- a/tools/Android.bp +++ b/tools/Android.bp @@ -37,3 +37,22 @@ python_binary_host { }, }, } + +python_test_host { + name: "post_process_props_unittest", + main: "test_post_process_props.py", + srcs: [ + "post_process_props.py", + "test_post_process_props.py", + ], + version: { + py2: { + enabled: false, + }, + py3: { + enabled: true, + }, + }, + test_config: "post_process_props_unittest.xml", + test_suites: ["general-tests"], +} diff --git a/tools/post_process_props.py b/tools/post_process_props.py index 4fa15bcd6..397526f05 100755 --- a/tools/post_process_props.py +++ b/tools/post_process_props.py @@ -16,8 +16,8 @@ import sys -# Usage: post_process_props.py file.prop [blacklist_key, ...] -# Blacklisted keys are removed from the property file, if present +# Usage: post_process_props.py file.prop [disallowed_key, ...] +# Disallowed keys are removed from the property file, if present # See PROP_VALUE_MAX in system_properties.h. # The constant in system_properties.h includes the terminating NUL, @@ -29,8 +29,8 @@ PROP_VALUE_MAX = 91 def mangle_build_prop(prop_list): # If ro.debuggable is 1, then enable adb on USB by default # (this is for userdebug builds) - if prop_list.get("ro.debuggable") == "1": - val = prop_list.get("persist.sys.usb.config") + if prop_list.get_value("ro.debuggable") == "1": + val = prop_list.get_value("persist.sys.usb.config") if "adb" not in val: if val == "": val = "adb" @@ -40,52 +40,121 @@ def mangle_build_prop(prop_list): # UsbDeviceManager expects a value here. If it doesn't get it, it will # default to "adb". That might not the right policy there, but it's better # to be explicit. - if not prop_list.get("persist.sys.usb.config"): + if not prop_list.get_value("persist.sys.usb.config"): prop_list.put("persist.sys.usb.config", "none"); def validate(prop_list): """Validate the properties. + If the value of a sysprop exceeds the max limit (91), it's an error, unless + the sysprop is a read-only one. + + Checks if there is no optional prop assignments. + Returns: True if nothing is wrong. """ check_pass = True - for p in prop_list.get_all(): + for p in prop_list.get_all_props(): if len(p.value) > PROP_VALUE_MAX and not p.name.startswith("ro."): check_pass = False sys.stderr.write("error: %s cannot exceed %d bytes: " % (p.name, PROP_VALUE_MAX)) sys.stderr.write("%s (%d)\n" % (p.value, len(p.value))) + + if p.is_optional(): + check_pass = False + sys.stderr.write("error: found unresolved optional prop assignment:\n") + sys.stderr.write(str(p) + "\n") + return check_pass +def override_optional_props(prop_list): + """Override a?=b with a=c, if the latter exists + + Overriding is done by deleting a?=b + When there are a?=b and a?=c, then only the last one survives + When there are a=b and a=c, then it's an error. + + Returns: + True if the override was successful + """ + success = True + for name in prop_list.get_all_names(): + props = prop_list.get_props(name) + optional_props = [p for p in props if p.is_optional()] + overriding_props = [p for p in props if not p.is_optional()] + if len(overriding_props) > 1: + # 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 + success = False + sys.stderr.write("error: found duplicate sysprop assignments:\n") + for p in overriding_props: + sys.stderr.write("%s\n" % str(p)) + elif len(overriding_props) == 1: + for p in optional_props: + p.delete("overridden by %s" % str(overriding_props[0])) + else: + if len(optional_props) > 1: + for p in optional_props[:-1]: + p.delete("overridden by %s" % str(optional_props[-1])) + # Make the last optional one as non-optional + optional_props[-1].optional = False + + return success + class Prop: - def __init__(self, name, value, comment=None): + def __init__(self, name, value, optional=False, comment=None): self.name = name.strip() self.value = value.strip() - self.comment = comment + if comment != None: + self.comments = [comment] + else: + self.comments = [] + self.optional = optional @staticmethod def from_line(line): line = line.rstrip('\n') if line.startswith("#"): - return Prop("", "", line) + return Prop("", "", comment=line) + elif "?=" in line: + name, value = line.split("?=", 1) + return Prop(name, value, optional=True) elif "=" in line: name, value = line.split("=", 1) - return Prop(name, value) + return Prop(name, value, optional=False) else: # don't fail on invalid line # TODO(jiyong) make this a hard error - return Prop("", "", line) + return Prop("", "", comment=line) def is_comment(self): - return self.comment != None + return bool(self.comments and not self.name) + + def is_optional(self): + return (not self.is_comment()) and self.optional + + def make_as_comment(self): + # Prepend "#" to the last line which is the prop assignment + if not self.is_comment(): + assignment = str(self).rsplit("\n", 1)[-1] + self.comments.append("#" + assignment) + self.name = "" + self.value = "" + + def delete(self, reason): + self.comments.append("# Removed by post_process_props.py because " + reason) + self.make_as_comment() def __str__(self): - if self.is_comment(): - return self.comment - else: - return self.name + "=" + self.value + assignment = [] + if not self.is_comment(): + operator = "?=" if self.is_optional() else "=" + assignment.append(self.name + operator + self.value) + return "\n".join(self.comments + assignment) class PropList: @@ -94,25 +163,35 @@ class PropList: self.props = [Prop.from_line(l) for l in f.readlines() if l.strip() != ""] - def get_all(self): + def get_all_props(self): return [p for p in self.props if not p.is_comment()] - def get(self, name): + def get_all_names(self): + return set([p.name for p in self.get_all_props()]) + + def get_props(self, name): + return [p for p in self.get_all_props() if p.name == name] + + def get_value(self, name): + # Caution: only the value of the first sysprop having the name is returned. return next((p.value for p in self.props if p.name == name), "") def put(self, name, value): - index = next((i for i,p in enumerate(self.props) if p.name == name), -1) + # Note: when there is an optional prop for the name, its value isn't changed. + # Instead a new non-optional prop is appended, which will override the + # optional prop. Otherwise, the new value might be overridden by an existing + # non-optional prop of the same name. + index = next((i for i,p in enumerate(self.props) + if p.name == name and not p.is_optional()), -1) if index == -1: - self.props.append(Prop(name, value)) + self.props.append(Prop(name, value, + comment="# Auto-added by post_process_props.py")) else: + self.props[index].comments.append( + "# Value overridden by post_process_props.py. Original value: %s" % + self.props[index].value) self.props[index].value = value - def delete(self, name): - index = next((i for i,p in enumerate(self.props) if p.name == name), -1) - if index != -1: - new_comment = "# removed by post_process_props.py\n#" + str(self.props[index]) - self.props[index] = Prop.from_line(new_comment) - def write(self, filename): with open(filename, 'w+') as f: for p in self.props: @@ -127,12 +206,15 @@ def main(argv): props = PropList(filename) mangle_build_prop(props) + if not override_optional_props(props): + sys.exit(1) if not validate(props): sys.exit(1) - # Drop any blacklisted keys + # Drop any disallowed keys for key in argv[2:]: - props.delete(key) + for p in props.get_props(key): + p.delete("%s is a disallowed key" % key) props.write(filename) diff --git a/tools/post_process_props_unittest.xml b/tools/post_process_props_unittest.xml new file mode 100644 index 000000000..4a6ecc2c6 --- /dev/null +++ b/tools/post_process_props_unittest.xml @@ -0,0 +1,6 @@ + + + + diff --git a/tools/test_post_process_props.py b/tools/test_post_process_props.py new file mode 100644 index 000000000..8a9c3edc6 --- /dev/null +++ b/tools/test_post_process_props.py @@ -0,0 +1,230 @@ +#!/usr/bin/env python3 +# +# Copyright (C) 2020 The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import contextlib +import io +import unittest + +from unittest.mock import * +from post_process_props import * + +class PropTestCase(unittest.TestCase): + def test_createFromLine(self): + p = Prop.from_line("# this is comment") + self.assertTrue(p.is_comment()) + self.assertEqual("", p.name) + self.assertEqual("", p.value) + self.assertFalse(p.is_optional()) + self.assertEqual("# this is comment", str(p)) + + for line in ["a=b", "a = b", "a= b", "a =b", " a=b "]: + p = Prop.from_line(line) + self.assertFalse(p.is_comment()) + self.assertEqual("a", p.name) + self.assertEqual("b", p.value) + self.assertFalse(p.is_optional()) + self.assertEqual("a=b", str(p)) + + for line in ["a?=b", "a ?= b", "a?= b", "a ?=b", " a?=b "]: + p = Prop.from_line(line) + self.assertFalse(p.is_comment()) + self.assertEqual("a", p.name) + self.assertEqual("b", p.value) + self.assertTrue(p.is_optional()) + self.assertEqual("a?=b", str(p)) + + def test_makeAsComment(self): + p = Prop.from_line("a=b") + p.comments.append("# a comment") + self.assertFalse(p.is_comment()) + + p.make_as_comment() + self.assertTrue(p.is_comment()) + self.assertTrue("# a comment\n#a=b", str(p)) + +class PropListTestcase(unittest.TestCase): + def setUp(self): + content = """ + # comment + foo=true + bar=false + qux?=1 + # another comment + foo?=false + """ + self.patcher = patch("post_process_props.open", mock_open(read_data=content)) + self.mock_open = self.patcher.start() + self.props = PropList("file") + + def tearDown(self): + self.patcher.stop() + self.props = None + + def test_readFromFile(self): + self.assertEqual(4, len(self.props.get_all_props())) + expected = [ + ("foo", "true", False), + ("bar", "false", False), + ("qux", "1", True), + ("foo", "false", True) + ] + for i,p in enumerate(self.props.get_all_props()): + self.assertEqual(expected[i][0], p.name) + self.assertEqual(expected[i][1], p.value) + self.assertEqual(expected[i][2], p.is_optional()) + self.assertFalse(p.is_comment()) + + self.assertEqual(set(["foo", "bar", "qux"]), self.props.get_all_names()) + + self.assertEqual("true", self.props.get_value("foo")) + self.assertEqual("false", self.props.get_value("bar")) + self.assertEqual("1", self.props.get_value("qux")) + + # there are two assignments for 'foo' + self.assertEqual(2, len(self.props.get_props("foo"))) + + def test_putNewProp(self): + self.props.put("new", "30") + + self.assertEqual(5, len(self.props.get_all_props())) + last_prop = self.props.get_all_props()[-1] + self.assertEqual("new", last_prop.name) + self.assertEqual("30", last_prop.value) + self.assertFalse(last_prop.is_optional()) + + def test_putExistingNonOptionalProp(self): + self.props.put("foo", "NewValue") + + self.assertEqual(4, len(self.props.get_all_props())) + foo_prop = self.props.get_props("foo")[0] + self.assertEqual("foo", foo_prop.name) + self.assertEqual("NewValue", foo_prop.value) + self.assertFalse(foo_prop.is_optional()) + self.assertEqual("# Value overridden by post_process_props.py. " + + "Original value: true\nfoo=NewValue", str(foo_prop)) + + def test_putExistingOptionalProp(self): + self.props.put("qux", "2") + + self.assertEqual(5, len(self.props.get_all_props())) + last_prop = self.props.get_all_props()[-1] + self.assertEqual("qux", last_prop.name) + self.assertEqual("2", last_prop.value) + self.assertFalse(last_prop.is_optional()) + self.assertEqual("# Auto-added by post_process_props.py\nqux=2", + str(last_prop)) + + def test_deleteNonOptionalProp(self): + props_to_delete = self.props.get_props("foo")[0] + props_to_delete.delete(reason="testing") + + self.assertEqual(3, len(self.props.get_all_props())) + self.assertEqual("# Removed by post_process_props.py because testing\n" + + "#foo=true", str(props_to_delete)) + + def test_deleteOptionalProp(self): + props_to_delete = self.props.get_props("qux")[0] + props_to_delete.delete(reason="testing") + + self.assertEqual(3, len(self.props.get_all_props())) + self.assertEqual("# Removed by post_process_props.py because testing\n" + + "#qux?=1", str(props_to_delete)) + + def test_overridingNonOptional(self): + props_to_be_overridden = self.props.get_props("foo")[1] + self.assertTrue("true", props_to_be_overridden.value) + + self.assertTrue(override_optional_props(self.props)) + + # size reduced to 3 because foo?=false was overridden by foo=true + self.assertEqual(3, len(self.props.get_all_props())) + + self.assertEqual(1, len(self.props.get_props("foo"))) + self.assertEqual("true", self.props.get_props("foo")[0].value) + + self.assertEqual("# Removed by post_process_props.py because " + + "overridden by foo=true\n#foo?=false", + str(props_to_be_overridden)) + + def test_overridingOptional(self): + content = """ + # comment + qux?=2 + foo=true + bar=false + qux?=1 + # another comment + foo?=false + """ + with patch('post_process_props.open', mock_open(read_data=content)) as m: + props = PropList("hello") + + props_to_be_overridden = props.get_props("qux")[0] + self.assertEqual("2", props_to_be_overridden.value) + + self.assertTrue(override_optional_props(props)) + + self.assertEqual(1, len(props.get_props("qux"))) + self.assertEqual("1", props.get_props("qux")[0].value) + # the only left optional assignment becomes non-optional + self.assertFalse(props.get_props("qux")[0].is_optional()) + + self.assertEqual("# Removed by post_process_props.py because " + + "overridden by qux?=1\n#qux?=2", + str(props_to_be_overridden)) + + def test_overridingDuplicated(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") + + # fails due to duplicated foo=true and foo=false + self.assertFalse(override_optional_props(props)) + + self.assertEqual("error: found duplicate sysprop assignments:\n" + + "foo=true\nfoo=false\n", stderr_redirect.getvalue()) + + def test_overridingDuplicatedWithSameValue(self): + content = """ + # comment + foo=true + bar=false + qux?=1 + foo=true + # 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=true, but that's allowed + # since they have the same value + self.assertTrue(override_optional_props(props)) + +if __name__ == '__main__': + unittest.main(verbosity=2) From 8b266f16a90d072dc47415b9332782a0a86292a4 Mon Sep 17 00:00:00 2001 From: Jiyong Park Date: Mon, 22 Jun 2020 19:42:59 +0900 Subject: [PATCH 2/7] Don't inherit tablet-dalvik-heap for GSI and emulator GSI and emulator should not be specialized for tablet. This is also to avoid the expected sysprop conflict after I9c073a21c8257987cf2378012cadaeeeb698a4fb gets in. With the change, duplicate assignments of a sysprop is prohibited. We currently have the duplication due to the following hierarchy chain: aosp_arm64.mk -> emulator_vendor.mk -> goldfish/vendor.mk -> phone-xhdpi-2048-dalvik-heap.mk -> generic_arm64/device.mk -> tablet-dalvik-heap.mk Many of the dalvik.vm.* properties are duplicated between phone-*- dalvik-heap.mk and tablet-dalvik-heap.mk files. Bug: 117892318 Bug: 158735147 Test: atest --host post_process_prop_unittest Exempt-From-Owner-Approval: cherry-pick from master Merged-In: I4d1e2f819fe688a4a85e58387b6af58d603399d3 (cherry picked from commit 9f2f6dd9c9a48b526742a3ca8e21328ac3355b9f) Change-Id: I4d1e2f819fe688a4a85e58387b6af58d603399d3 --- target/board/emulator_arm64/device.mk | 4 ---- target/board/generic_arm64/device.mk | 4 ---- 2 files changed, 8 deletions(-) diff --git a/target/board/emulator_arm64/device.mk b/target/board/emulator_arm64/device.mk index 57675d02d..73dc2f4c4 100644 --- a/target/board/emulator_arm64/device.mk +++ b/target/board/emulator_arm64/device.mk @@ -26,7 +26,3 @@ endif PRODUCT_COPY_FILES += \ $(LOCAL_KERNEL):kernel - -# Adjust the Dalvik heap to be appropriate for a tablet. -$(call inherit-product-if-exists, frameworks/base/build/tablet-dalvik-heap.mk) -$(call inherit-product-if-exists, frameworks/native/build/tablet-dalvik-heap.mk) diff --git a/target/board/generic_arm64/device.mk b/target/board/generic_arm64/device.mk index 3b7cd44ac..b34004fd7 100644 --- a/target/board/generic_arm64/device.mk +++ b/target/board/generic_arm64/device.mk @@ -18,7 +18,3 @@ PRODUCT_COPY_FILES += \ device/google/cuttlefish_kernel/5.4-arm64/kernel-5.4:kernel-5.4 \ device/google/cuttlefish_kernel/5.4-arm64/kernel-5.4-gz:kernel-5.4-gz \ device/google/cuttlefish_kernel/5.4-arm64/kernel-5.4-lz4:kernel-5.4-lz4 - -# Adjust the Dalvik heap to be appropriate for a tablet. -$(call inherit-product-if-exists, frameworks/base/build/tablet-dalvik-heap.mk) -$(call inherit-product-if-exists, frameworks/native/build/tablet-dalvik-heap.mk) From 8d521ec2c8cf9bf6707c3fbf8ca6fc96868d2630 Mon Sep 17 00:00:00 2001 From: Jiyong Park Date: Mon, 22 Jun 2020 19:54:44 +0900 Subject: [PATCH 3/7] ro.zygote in base_system.mk is optional The setting of ro.zygote in base_system.mk is optional, which means the value can be overriden by other (probably more specific) mk files. Bug: 117892318 Bug: 158735147 Test: atest --host post_process_prop_unittest Exempt-From-Owner-Approval: cherry-pick from master Merged-In: Ia7a67c0a04fad343d6591417f40dd4b9ddadc5e4 (cherry picked from commit b1261aac333e9980842af1120a4af747130e9f03) Change-Id: Ia7a67c0a04fad343d6591417f40dd4b9ddadc5e4 --- target/product/base_system.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/product/base_system.mk b/target/product/base_system.mk index bec550bc2..f6770fb69 100644 --- a/target/product/base_system.mk +++ b/target/product/base_system.mk @@ -348,7 +348,7 @@ PRODUCT_BOOT_JARS += android.test.base endif PRODUCT_COPY_FILES += system/core/rootdir/init.zygote32.rc:system/etc/init/hw/init.zygote32.rc -PRODUCT_SYSTEM_PROPERTIES += ro.zygote=zygote32 +PRODUCT_SYSTEM_PROPERTIES += ro.zygote?=zygote32 PRODUCT_SYSTEM_PROPERTIES += debug.atrace.tags.enableflags=0 From 19746f4686bd7ecb82e1c37ff37fcd195c2972a7 Mon Sep 17 00:00:00 2001 From: Jiyong Park Date: Mon, 22 Jun 2020 20:15:55 +0900 Subject: [PATCH 4/7] Some properties are set as optional Some properties that are designed to provide a safe default value are explicitly set as optional using the 'a ?= b' syntax. Bug: 117892318 Bug: 158735147 Test: m Exempt-From-Owner-Approval: cherry-pick from master Merged-In: Ie6a50ab7e0bcb210e282bc18e8c1daf412903f90 (cherry picked from commit dfb3937ce48009676f2ef62278393c934079c60b) Change-Id: Ie6a50ab7e0bcb210e282bc18e8c1daf412903f90 --- target/product/aosp_product.mk | 6 +++--- target/product/full_base.mk | 4 ++-- target/product/full_base_telephony.mk | 4 ++-- target/product/handheld_system.mk | 6 +++--- target/product/media_system.mk | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/target/product/aosp_product.mk b/target/product/aosp_product.mk index f22c3a31f..a3da1c931 100644 --- a/target/product/aosp_product.mk +++ b/target/product/aosp_product.mk @@ -23,9 +23,9 @@ $(call inherit-product-if-exists, frameworks/base/data/sounds/AllAudio.mk) # Additional settings used in all AOSP builds PRODUCT_PRODUCT_PROPERTIES += \ - ro.config.ringtone=Ring_Synth_04.ogg \ - ro.config.notification_sound=pixiedust.ogg \ - ro.com.android.dataroaming=true \ + ro.config.ringtone?=Ring_Synth_04.ogg \ + ro.config.notification_sound?=pixiedust.ogg \ + ro.com.android.dataroaming?=true \ # More AOSP packages PRODUCT_PACKAGES += \ diff --git a/target/product/full_base.mk b/target/product/full_base.mk index dfb22047e..64f61ffbc 100644 --- a/target/product/full_base.mk +++ b/target/product/full_base.mk @@ -43,8 +43,8 @@ PRODUCT_PACKAGES += \ # Additional settings used in all AOSP builds PRODUCT_VENDOR_PROPERTIES := \ - ro.config.ringtone=Ring_Synth_04.ogg \ - ro.config.notification_sound=pixiedust.ogg + ro.config.ringtone?=Ring_Synth_04.ogg \ + ro.config.notification_sound?=pixiedust.ogg # Put en_US first in the list, so make it default. PRODUCT_LOCALES := en_US diff --git a/target/product/full_base_telephony.mk b/target/product/full_base_telephony.mk index 5e18c058c..d8a54cd7f 100644 --- a/target/product/full_base_telephony.mk +++ b/target/product/full_base_telephony.mk @@ -20,8 +20,8 @@ # entirely appropriate to inherit from for on-device configurations. PRODUCT_VENDOR_PROPERTIES := \ - keyguard.no_require_sim=true \ - ro.com.android.dataroaming=true + keyguard.no_require_sim?=true \ + ro.com.android.dataroaming?=true PRODUCT_COPY_FILES := \ device/sample/etc/apns-full-conf.xml:system/etc/apns-conf.xml \ diff --git a/target/product/handheld_system.mk b/target/product/handheld_system.mk index 22c817e54..e2c91b666 100644 --- a/target/product/handheld_system.mk +++ b/target/product/handheld_system.mk @@ -84,6 +84,6 @@ PRODUCT_COPY_FILES += \ frameworks/av/media/libeffects/data/audio_effects.conf:system/etc/audio_effects.conf PRODUCT_VENDOR_PROPERTIES += \ - ro.carrier=unknown \ - ro.config.notification_sound=OnTheHunt.ogg \ - ro.config.alarm_alert=Alarm_Classic.ogg + ro.carrier?=unknown \ + ro.config.notification_sound?=OnTheHunt.ogg \ + ro.config.alarm_alert?=Alarm_Classic.ogg diff --git a/target/product/media_system.mk b/target/product/media_system.mk index a3fafb342..7a2dd73e7 100644 --- a/target/product/media_system.mk +++ b/target/product/media_system.mk @@ -74,7 +74,7 @@ PRODUCT_COPY_FILES += $(call add-to-product-copy-files-if-exists,\ # On userdebug builds, collect more tombstones by default. ifneq (,$(filter userdebug eng,$(TARGET_BUILD_VARIANT))) PRODUCT_VENDOR_PROPERTIES += \ - tombstoned.max_tombstone_count=50 + tombstoned.max_tombstone_count?=50 endif PRODUCT_VENDOR_PROPERTIES += \ From 85471ed82e5ff8e84b2e8fb838d683b4e40f5d34 Mon Sep 17 00:00:00 2001 From: Jiyong Park Date: Tue, 23 Jun 2020 16:46:38 +0900 Subject: [PATCH 5/7] pm.dexopt.* props in runtime_libart.mk becomes optional The mk file is designed to provide safe default values which can be overridden by target-specific mk files. Previously it was difficult to correctly configure the mk files because the final prop value that is baked in the system/build.prop is highly dependent (and sensitive as well) to the mk file inheritance order which is very difficult (and non-intuitive) to follow. I9c073a21c8257987cf2378012cadaeeeb698a4fb is an attempt to make it much easier and intuitive. Specifically, the new `a ?= b` syntax makes the assignment optional, which means it is used only when there is no non-optional assignment for the same prop regardless of the relative ordering among them. In addition, the change prohibits having multiple non-optional prop assignments for the same prop name. pm.dex.* prop in runtime_libart.mk are now set using the `a ?= b` syntax to explicitly mark that they provide default values. Bug: 117892318 Bug: 158735147 Test: m Exempt-From-Owner-Approval: cherry-pick from master Merged-In: I044486d313d699607cd54222ae34d9eae24762b9 (cherry picked from commit bca4ea477a7a7e854491e80b38fab9ebf621ce07) Change-Id: I044486d313d699607cd54222ae34d9eae24762b9 --- target/product/runtime_libart.mk | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/target/product/runtime_libart.mk b/target/product/runtime_libart.mk index 78a4af09c..b96601ddd 100644 --- a/target/product/runtime_libart.mk +++ b/target/product/runtime_libart.mk @@ -57,23 +57,23 @@ PRODUCT_SYSTEM_PROPERTIES += \ # On eng builds, make "boot" reasons only extract for faster turnaround. ifeq (eng,$(TARGET_BUILD_VARIANT)) PRODUCT_SYSTEM_PROPERTIES += \ - pm.dexopt.first-boot=extract \ - pm.dexopt.boot=extract + pm.dexopt.first-boot?=extract \ + pm.dexopt.boot?=extract else PRODUCT_SYSTEM_PROPERTIES += \ - pm.dexopt.first-boot=quicken \ - pm.dexopt.boot=verify + pm.dexopt.first-boot?=quicken \ + pm.dexopt.boot?=verify endif # The install filter is speed-profile in order to enable the use of # profiles from the dex metadata files. Note that if a profile is not provided # or if it is empty speed-profile is equivalent to (quicken + empty app image). PRODUCT_SYSTEM_PROPERTIES += \ - pm.dexopt.install=speed-profile \ - pm.dexopt.bg-dexopt=speed-profile \ - pm.dexopt.ab-ota=speed-profile \ - pm.dexopt.inactive=verify \ - pm.dexopt.shared=speed + pm.dexopt.install?=speed-profile \ + pm.dexopt.bg-dexopt?=speed-profile \ + pm.dexopt.ab-ota?=speed-profile \ + pm.dexopt.inactive?=verify \ + pm.dexopt.shared?=speed # Pass file with the list of updatable boot class path packages to dex2oat. PRODUCT_SYSTEM_PROPERTIES += \ From 0b4fccb66dc33e6cc28c0e638ee834fbf8f53c50 Mon Sep 17 00:00:00 2001 From: Jiyong Park Date: Fri, 26 Jun 2020 17:38:00 +0900 Subject: [PATCH 6/7] 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 453ea6c7e..61e6bb66c 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 ae1614f24..1fbd9898d 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 3806a9691..aa4fe9188 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 397526f05..78a23fb72 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 8a9c3edc6..44fe957d7 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) From 24d9cad56383fdc6c2c61526c44d2ffd726b8e7b Mon Sep 17 00:00:00 2001 From: Jiyong Park Date: Tue, 30 Jun 2020 11:41:23 +0900 Subject: [PATCH 7/7] Handle the case when non-optional props have the same value foo=true foo=true foo?=false Consider the above case: Then the duplication of foo is allowed because they have the same value (true). However, there was a bug that the optional assirgnment foo?=false is left unmodified. This fixes the bug by commenting such optional assignments. Exempt-From-Owner-Approval: fixes a broken build Bug: 117892318 Bug: 158735147 Test: atest test_post_process_props Test: m out/target/product/vsoc_x86/vendor/build.prop for cf_x86_auto Exempt-From-Owner-Approval: cherry-pick from master Merged-In: Iba9b61d9779d93e86d9bead2286f945f8d51ab1d (cherry picked from commit 9a32636759822bfb04071c9e28b101e9714ce305) Change-Id: Iba9b61d9779d93e86d9bead2286f945f8d51ab1d --- tools/post_process_props.py | 2 ++ tools/test_post_process_props.py | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/tools/post_process_props.py b/tools/post_process_props.py index 78a23fb72..d8c9cb157 100755 --- a/tools/post_process_props.py +++ b/tools/post_process_props.py @@ -88,6 +88,8 @@ def override_optional_props(prop_list, allow_dup=False): if len(overriding_props) > 1: # duplicated props are allowed when the all have the same value if all(overriding_props[0].value == p.value for p in overriding_props): + for p in optional_props: + p.delete("overridden by %s" % str(overriding_props[0])) continue # or if dup is explicitly allowed for compat reason if allow_dup: diff --git a/tools/test_post_process_props.py b/tools/test_post_process_props.py index 44fe957d7..12d52e566 100644 --- a/tools/test_post_process_props.py +++ b/tools/test_post_process_props.py @@ -221,11 +221,17 @@ class PropListTestcase(unittest.TestCase): stderr_redirect = io.StringIO() with contextlib.redirect_stderr(stderr_redirect): props = PropList("hello") + optional_prop = props.get_props("foo")[2] # the last foo?=false one # we have duplicated foo=true and foo=true, but that's allowed # since they have the same value self.assertTrue(override_optional_props(props)) + # foo?=false should be commented out + self.assertEqual("# Removed by post_process_props.py because " + + "overridden by foo=true\n#foo?=false", + str(optional_prop)) + def test_allowDuplicates(self): content = """ # comment