From 32e413a2946d2afdb4b62294b412d9a5df8c9806 Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Mon, 4 Apr 2022 10:40:01 -0700 Subject: [PATCH] Fix single value variable inheritance order List variables needed to be percolated in the order of inherit() calls. children.keys() was in that order, due to starlark dictionaries being iterable in the order of insertion, but the previous cl broke that behavior by sorting them. Instead, only sort the children for single value variables. Fixes: 226206409 Fixes: 228044099 Test: ./out/rbcrun ./build/make/tests/run.rbc and testing aosp_arm64 Change-Id: I5b91514e87b158b615e4d4ec7868fccb0248379b --- core/product_config.rbc | 21 +++++++++++++++------ tests/single_value_inheritance/inherit1.rbc | 2 ++ tests/single_value_inheritance/inherit2.rbc | 2 ++ tests/single_value_inheritance/product.rbc | 2 +- tests/single_value_inheritance/test.rbc | 1 + 5 files changed, 21 insertions(+), 7 deletions(-) diff --git a/core/product_config.rbc b/core/product_config.rbc index 0e7dbf186d..f71fb6e6f6 100644 --- a/core/product_config.rbc +++ b/core/product_config.rbc @@ -168,6 +168,8 @@ def _product_configuration(top_pcm_name, top_pcm, input_variables_init): children = handle.inherited_modules if _options.trace_modules: print("# ", " ".join(children.keys())) + # Starlark dictionaries are guaranteed to iterate through in insertion order, + # so children.keys() will be ordered by the inherit() calls configs[name] = (pcm, handle.cfg, children.keys(), False) pcm_count = pcm_count + 1 @@ -291,12 +293,6 @@ def _percolate_inherited(configs, cfg_name, cfg, children_names): child_cfg = configs[child_name][1] for attr, value in child_cfg.items(): if type(value) != "list": - # Single value variables take the first value available from the leftmost - # branch of the tree. If we also had "or attr in percolated_attrs" in this - # if statement, it would take the value from the rightmost branch. - if cfg.get(attr, "") == "": - cfg[attr] = value - percolated_attrs[attr] = True continue if attr in percolated_attrs: # We already are percolating this one, just add this list @@ -306,6 +302,19 @@ def _percolate_inherited(configs, cfg_name, cfg, children_names): cfg[attr] = [] __move_items(cfg[attr], child_cfg, attr) + # single value variables need to be inherited in alphabetical order, + # not in the order of inherit() calls. + for child_name in sorted(children_names): + child_cfg = configs[child_name][1] + for attr, value in child_cfg.items(): + if type(value) != "list": + # Single value variables take the first value available from the leftmost + # branch of the tree. If we also had "or attr in percolated_attrs" in this + # if statement, it would take the value from the rightmost branch. + if cfg.get(attr, "") == "": + cfg[attr] = value + percolated_attrs[attr] = True + for attr in _options.trace_variables: if attr in percolated_attrs: print("%s: %s^=%s" % (cfg_name, attr, cfg[attr])) diff --git a/tests/single_value_inheritance/inherit1.rbc b/tests/single_value_inheritance/inherit1.rbc index b71ffb342e..0cc98a9547 100644 --- a/tests/single_value_inheritance/inherit1.rbc +++ b/tests/single_value_inheritance/inherit1.rbc @@ -19,3 +19,5 @@ def init(g, handle): cfg["PRODUCT_CHARACTERISTICS"] = "tablet" cfg["PRODUCT_DEFAULT_DEV_CERTIFICATE"] = "vendor/myvendor/certs/devkeys/devkey" + cfg.setdefault("PRODUCT_PACKAGES", []) + cfg["PRODUCT_PACKAGES"] += ["bar"] diff --git a/tests/single_value_inheritance/inherit2.rbc b/tests/single_value_inheritance/inherit2.rbc index be85866ffe..ed5e56974c 100644 --- a/tests/single_value_inheritance/inherit2.rbc +++ b/tests/single_value_inheritance/inherit2.rbc @@ -18,3 +18,5 @@ def init(g, handle): cfg = rblf.cfg(handle) cfg["PRODUCT_CHARACTERISTICS"] = "nosdcard" + cfg.setdefault("PRODUCT_PACKAGES", []) + cfg["PRODUCT_PACKAGES"] += ["foo"] diff --git a/tests/single_value_inheritance/product.rbc b/tests/single_value_inheritance/product.rbc index 332704351f..d090af61e3 100644 --- a/tests/single_value_inheritance/product.rbc +++ b/tests/single_value_inheritance/product.rbc @@ -18,7 +18,7 @@ load(":inherit2.rbc", _inherit2_init = "init") def init(g, handle): cfg = rblf.cfg(handle) - rblf.inherit(handle, "test/inherit1", _inherit1_init) rblf.inherit(handle, "test/inherit2", _inherit2_init) + rblf.inherit(handle, "test/inherit1", _inherit1_init) cfg["PRODUCT_DEFAULT_DEV_CERTIFICATE"] = "" diff --git a/tests/single_value_inheritance/test.rbc b/tests/single_value_inheritance/test.rbc index 07a5e65cd5..dcde7e0db7 100644 --- a/tests/single_value_inheritance/test.rbc +++ b/tests/single_value_inheritance/test.rbc @@ -25,3 +25,4 @@ def test(): (globals, config, globals_base) = rblf.product_configuration("test/device", init, input_variables_init) assert_eq("tablet", config["PRODUCT_CHARACTERISTICS"]) assert_eq("vendor/myvendor/certs/devkeys/devkey", config["PRODUCT_DEFAULT_DEV_CERTIFICATE"]) + assert_eq(["foo", "bar"], config["PRODUCT_PACKAGES"])