Merge changes from topic "sysprop"

* changes:
  Handle the case when non-optional props have the same value
  BUILD_BROKEN_DUP_SYSPROP as escape hatch for the new sysprop restriction
  pm.dexopt.* props in runtime_libart.mk becomes optional
  Some properties are set as optional
  ro.zygote in base_system.mk is optional
  Don't inherit tablet-dalvik-heap for GSI and emulator
  Support optional prop assignments

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

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) \

core/sysprop.mk

@@ -71,10 +71,22 @@ endef
 define build-properties
 ALL_DEFAULT_INSTALLED_MODULES += $(2)
 
-# TODO(b/117892318): eliminate the call to uniq-pairs-by-first-component when
+$(eval # Properties can be assigned using `prop ?= value` or `prop = value` syntax.)
-# it is guaranteed that there is no dup.
+$(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),=))\
+)
+
+$(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)
@@ -99,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
 

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)

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)

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.ringtone?=Ring_Synth_04.ogg \
-    ro.config.notification_sound=pixiedust.ogg \
+    ro.config.notification_sound?=pixiedust.ogg \
-    ro.com.android.dataroaming=true \
+    ro.com.android.dataroaming?=true \
 
 # More AOSP packages
 PRODUCT_PACKAGES += \

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
 

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.ringtone?=Ring_Synth_04.ogg \
-    ro.config.notification_sound=pixiedust.ogg
+    ro.config.notification_sound?=pixiedust.ogg
 
 # Put en_US first in the list, so make it default.
 PRODUCT_LOCALES := en_US

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 \
+    keyguard.no_require_sim?=true \
-    ro.com.android.dataroaming=true
+    ro.com.android.dataroaming?=true
 
 PRODUCT_COPY_FILES := \
     device/sample/etc/apns-full-conf.xml:system/etc/apns-conf.xml \

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.carrier?=unknown \
-    ro.config.notification_sound=OnTheHunt.ogg \
+    ro.config.notification_sound?=OnTheHunt.ogg \
-    ro.config.alarm_alert=Alarm_Classic.ogg
+    ro.config.alarm_alert?=Alarm_Classic.ogg

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 += \

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.first-boot?=extract \
-        pm.dexopt.boot=extract
+        pm.dexopt.boot?=extract
 else
     PRODUCT_SYSTEM_PROPERTIES += \
-        pm.dexopt.first-boot=quicken \
+        pm.dexopt.first-boot?=quicken \
-        pm.dexopt.boot=verify
+        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.install?=speed-profile \
-    pm.dexopt.bg-dexopt=speed-profile \
+    pm.dexopt.bg-dexopt?=speed-profile \
-    pm.dexopt.ab-ota=speed-profile \
+    pm.dexopt.ab-ota?=speed-profile \
-    pm.dexopt.inactive=verify \
+    pm.dexopt.inactive?=verify \
-    pm.dexopt.shared=speed
+    pm.dexopt.shared?=speed
 
 # Pass file with the list of updatable boot class path packages to dex2oat.
 PRODUCT_SYSTEM_PROPERTIES += \

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"],
+}

tools/post_process_props.py

@@ -14,10 +14,11 @@
 # 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 [blacklist_key, ...]
+# Usage: post_process_props.py file.prop [disallowed_key, ...]
-# Blacklisted keys are removed from the property file, if present
+# 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 +30,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":
+  if prop_list.get_value("ro.debuggable") == "1":
-    val = prop_list.get("persist.sys.usb.config")
+    val = prop_list.get_value("persist.sys.usb.config")
     if "adb" not in val:
       if val == "":
         val = "adb"
@@ -40,52 +41,132 @@ 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, allow_dup=False):
+  """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):
+        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:
+        # 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:
+        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():
+    assignment = []
-      return self.comment
+    if not self.is_comment():
-    else:
+      operator = "?=" if self.is_optional() else "="
-      return self.name + "=" + self.value
+      assignment.append(self.name + operator + self.value)
+    return "\n".join(self.comments + assignment)
 
 class PropList:
 
@@ -94,47 +175,65 @@ 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:
         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, args.allow_dup):
+    sys.exit(1)
   if not validate(props):
     sys.exit(1)
 
-  # Drop any blacklisted keys
+  # Drop any disallowed keys
-  for key in argv[2:]:
+  for key in args.disallowed_keys:
-    props.delete(key)
+    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)

tools/post_process_props_unittest.xml

@@ -0,0 +1,6 @@
+<configuration description="Config to run post_process_props_unittest">
+    <test class="com.android.tradefed.testtype.python.PythonBinaryHostTest" >
+        <option name="par-file-name" value="post_process_props_unittest" />
+        <option name="test-timeout" value="1m" />
+    </test>
+</configuration>

tools/test_post_process_props.py

@@ -0,0 +1,255 @@
+#!/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")
+        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
+    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)