From 598ea49881dac479c4bfd469e8ebe2852d00fc67 Mon Sep 17 00:00:00 2001 From: Jiyong Park Date: Mon, 22 Jun 2020 17:30:57 +0900 Subject: [PATCH] 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)