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 b302cdf6a4)
Change-Id: I7bdfffd47d50aad66a78e28a30c3dad7ebac080c

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

@@ -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
 

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)

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)