Address reviewer comments from the previous commits

Fixes: 170637441
Test: rbcrun build/make/tests/run.rbc
Change-Id: Ifc16fe02d96bc3a4c5562b74da8c1e7b393dc000

core/envsetup.rbc

@@ -1,4 +1,3 @@
-
 # Copyright 2021 Google LLC
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -13,14 +12,14 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-load(":build_id.rbc|init", _build_id_init="init")
+load(":build_id.rbc|init", _build_id_init = "init")
 
 def _all_versions():
     """Returns all known versions."""
-    versions = ["OPR1", "OPD1", "OPD2","OPM1", "OPM2", "PPR1", "PPD1", "PPD2", "PPM1",  "PPM2", "QPR1" ]
+    versions = ["OPR1", "OPD1", "OPD2", "OPM1", "OPM2", "PPR1", "PPD1", "PPD2", "PPM1", "PPM2", "QPR1"]
     for v in ("Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z"):
         for e in ("P1A", "P1B", "P2A", "P2B", "D1A", "D1B", "D2A", "D2B", "Q1A", "Q1B", "Q2A", "Q2B", "Q3A", "Q3B"):
-            versions.append(v+e)
+            versions.append(v + e)
     return versions
 
 def _allowed_versions(all_versions, min_version, max_version, default_version):
@@ -36,7 +35,7 @@ def _allowed_versions(all_versions, min_version, max_version, default_version):
         fail("%s should come before %s in the version list" % (min_version, max_version))
     if def_i < min_i or def_i > max_i:
         fail("%s should come between % and %s" % (default_version, min_version, max_version))
-    return all_versions[min_i:max_i+1]
+    return all_versions[min_i:max_i + 1]
 
 # This function is a manual conversion of the version_defaults.mk
 def _versions_default(g, all_versions):
@@ -70,21 +69,22 @@ def _versions_default(g, all_versions):
     g.setdefault("PLATFORM_VERSION_CODENAME", g["TARGET_PLATFORM_VERSION"])
     # TODO(asmundak): set PLATFORM_VERSION_ALL_CODENAMES
 
-    g.setdefault("PLATFORM_VERSION",
-        g["PLATFORM_VERSION_LAST_STABLE"] if g["PLATFORM_VERSION_CODENAME"] == "REL" else g["PLATFORM_VERSION_CODENAME"])
     g.setdefault("PLATFORM_SDK_VERSION", 30)
-    if g["PLATFORM_VERSION_CODENAME"] == "REL":
+    version_codename = g["PLATFORM_VERSION_CODENAME"]
+    if version_codename == "REL":
+        g.setdefault("PLATFORM_VERSION", g["PLATFORM_VERSION_LAST_STABLE"])
         g["PLATFORM_PREVIEW_SDK_VERSION"] = 0
+        g.setdefault("DEFAULT_APP_TARGET_SDK", g["PLATFORM_SDK_VERSION"])
+        g.setdefault("PLATFORM_VNDK_VERSION", g["PLATFORM_SDK_VERSION"])
     else:
+        g.setdefault("PLATFORM_VERSION", version_codename)
         g.setdefault("PLATFORM_PREVIEW_SDK_VERSION", 1)
+        g.setdefault("DEFAULT_APP_TARGET_SDK", version_codename)
+        g.setdefault("PLATFORM_VNDK_VERSION", version_codename)
 
-    g.setdefault("DEFAULT_APP_TARGET_SDK",
-        g["PLATFORM_SDK_VERSION"] if g["PLATFORM_VERSION_CODENAME"] == "REL" else g["PLATFORM_VERSION_CODENAME"])
-    g.setdefault("PLATFORM_VNDK_VERSION",
-        g["PLATFORM_SDK_VERSION"] if g["PLATFORM_VERSION_CODENAME"] == "REL" else g["PLATFORM_VERSION_CODENAME"])
     g.setdefault("PLATFORM_SYSTEMSDK_MIN_VERSION", 28)
     versions = [str(i) for i in range(g["PLATFORM_SYSTEMSDK_MIN_VERSION"], g["PLATFORM_SDK_VERSION"] + 1)]
-    versions.append(g["PLATFORM_VERSION_CODENAME"])
+    versions.append(version_codename)
     g["PLATFORM_SYSTEMSDK_VERSIONS"] = sorted(versions)
 
     #  Used to indicate the security patch that has been applied to the device.
@@ -117,6 +117,13 @@ def _versions_default(g, all_versions):
     g.setdefault("PLATFORM_MIN_SUPPORTED_TARGET_SDK_VERSION", 23)
 
 def init(g):
+    """Initializes globals.
+
+    The code is the Starlark counterpart of the contents of the
+    envsetup.mk file.
+    Args:
+        g: globals dictionary
+    """
     all_versions = _all_versions()
     _versions_default(g, all_versions)
     for v in all_versions:
@@ -168,7 +175,8 @@ def init(g):
     g["HOST_CROSS_2ND_ARCH_MODULE_SUFFIX"] = "_64"
     g["TARGET_2ND_ARCH_VAR_PREFIX"] = "2ND_"
 
-    # TODO(asmundak): combo-related stuff
+    # TODO(asmundak): envsetup.mk lines 216-226:
+    # convert combo-related stuff from combo/select.mk
 
     # on windows, the tools have .exe at the end, and we depend on the
     # host config stuff being done first
@@ -177,23 +185,23 @@ def init(g):
 
     # the host build defaults to release, and it must be release or debug
     g.setdefault("HOST_BUILD_TYPE", "release")
-    if g["HOST_BUILD_TYPE"] != "release" and g["HOST_BUILD_TYPE"] != "debug":
+    if g["HOST_BUILD_TYPE"] not in ["release", "debug"]:
         fail("HOST_BUILD_TYPE must be either release or debug, not '%s'" % g["HOST_BUILD_TYPE"])
 
-    # TODO(asmundak): a lot more, but not needed for the product configuration
+    # TODO(asmundak): there is more stuff in envsetup.mk lines 249-292, but
+    # it does not seem to affect product configuration. Revisit this.
 
     g["ART_APEX_JARS"] = [
         "com.android.art:core-oj",
         "com.android.art:core-libart",
         "com.android.art:okhttp",
         "com.android.art:bouncycastle",
-        "com.android.art:apache-xml"
+        "com.android.art:apache-xml",
     ]
 
     if g.get("TARGET_BUILD_TYPE", "") != "debug":
         g["TARGET_BUILD_TYPE"] = "release"
 
-
 v_default = "SP1A"
 v_min = "SP1A"
 v_max = "SP1A"

core/product_config.rbc

@@ -1,4 +1,3 @@
-
 # Copyright 2021 Google LLC
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -13,7 +12,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-load("//build/make/core:envsetup.rbc", _envsetup_init="init")
+load("//build/make/core:envsetup.rbc", _envsetup_init = "init")
+
 """Runtime functions."""
 
 def _global_init():
@@ -33,10 +33,13 @@ def _global_init():
 
     # Variables that should be defined.
     mandatory_vars = [
-        "PLATFORM_VERSION_CODENAME", "PLATFORM_VERSION",
+        "PLATFORM_VERSION_CODENAME",
+        "PLATFORM_VERSION",
         "PRODUCT_SOONG_NAMESPACES",
         # TODO(asmundak): do we need TARGET_ARCH? AOSP does not reference it
-        "TARGET_BUILD_TYPE", "TARGET_BUILD_VARIANT", "TARGET_PRODUCT",
+        "TARGET_BUILD_TYPE",
+        "TARGET_BUILD_VARIANT",
+        "TARGET_PRODUCT",
     ]
     for bv in mandatory_vars:
         if not bv in globals:
@@ -56,29 +59,26 @@ def __print_attr(attr, value):
             print(attr, "=", repr(value))
         elif _options.format == "make":
             print(attr, ":=", " ".join(value))
-    else:
+    elif _options.format == "pretty":
-        if _options.format == "pretty":
         print(attr, "=", repr(value))
     elif _options.format == "make":
         print(attr, ":=", value)
     else:
         fail("bad output format", _options.format)
 
-
 def _printvars(globals, cfg):
     """Prints known configuration variables."""
     for attr, val in sorted(cfg.items()):
         __print_attr(attr, val)
     if _options.print_globals:
-        for attr, val in sorted(globals.items()):
         print()
+        for attr, val in sorted(globals.items()):
             if attr not in _globals_base:
                 __print_attr(attr, val)
 
-
+def __printvars_rearrange_list(value_list):
-def __printvars_rearrange_list(l):
     """Rearrange value list: return only distinct elements, maybe sorted."""
-    seen = { item: 0 for item in l}
+    seen = {item: 0 for item in value_list}
     return sorted(seen.keys()) if _options.rearrange == "sort" else seen.keys()
 
 def _product_configuration(top_pcm_name, top_pcm):
@@ -98,43 +98,48 @@ def _product_configuration(top_pcm_name, top_pcm):
     globals = dict(**_globals_base)
 
     config_postfix = []  # Configs in postfix order
+
     # Each PCM is represented by a quadruple of function, config, children names
     # and readyness (that is, the configurations from inherited PCMs have been
     # substituted).
-    configs = { top_pcm_name: (top_pcm, None, [], False)}  # All known PCMs
+    configs = {top_pcm_name: (top_pcm, None, [], False)}  # All known PCMs
 
     stash = []  # Configs to push once their descendants are done
 
-    # Stack maintaining PCMs to be processed. An item in the stack
+    # Stack containing PCMs to be processed. An item in the stack
     # is a pair of PCMs name and its height in the product inheritance tree.
-    pcm_stack = []
+    pcm_stack = [(top_pcm_name, 0)]
-    pcm_stack.append((top_pcm_name, 0))
     pcm_count = 0
+
     # Run it until pcm_stack is exhausted, but no more than N times
     for n in range(1000):
         if not pcm_stack:
             break
         (name, height) = pcm_stack.pop()
         pcm, cfg, c, _ = configs[name]
-        # each PCM is executed once
+
+        # cfg is set only after PCM has been called, leverage this
+        # to prevent calling the same PCM twice
         if cfg != None:
             continue
+
         # Push ancestors until we reach this node's height
         config_postfix.extend([stash.pop() for i in range(len(stash) - height)])
 
         # Run this one, obtaining its configuration and child PCMs.
         if _options.trace_modules:
-            print("%s:" % n[0])
+            print("%d:" % n)
 
-        # The handle passed to the PCM consists of config and inheritance state.dict of inherited modules
+        # Run PCM.
-        # and a list containing the current default value of a list variable.
         handle = __h_new()
         pcm(globals, handle)
+
+        # Now we know everything about this PCM, record it in 'configs'.
         children = __h_inherited_modules(handle)
         if _options.trace_modules:
             print("   ", "    ".join(children.keys()))
         configs[name] = (pcm, __h_cfg(handle), children.keys(), False)
-        pcm_count = pcm_count+1
+        pcm_count = pcm_count + 1
 
         if len(children) == 0:
             # Leaf PCM goes straight to the config_postfix
@@ -143,17 +148,18 @@ def _product_configuration(top_pcm_name, top_pcm):
 
         # Stash this PCM, process children in the sorted order
         stash.append(name)
-        for child_name in sorted(children, reverse=True):
+        for child_name in sorted(children, reverse = True):
             if child_name not in configs:
                 configs[child_name] = (children[child_name], None, [], False)
             pcm_stack.append((child_name, len(stash)))
+    if pcm_stack:
+        fail("Inheritance processing took too many iterations")
 
     # Flush the stash
     config_postfix.extend([stash.pop() for i in range(len(stash))])
     if len(config_postfix) != pcm_count:
         fail("Ran %d modules but postfix tree has only %d entries" % (pcm_count, len(config_postfix)))
 
-
     if _options.trace_modules:
         print("\n---Postfix---")
         for x in config_postfix:
@@ -162,55 +168,56 @@ def _product_configuration(top_pcm_name, top_pcm):
     # Traverse the tree from the bottom, evaluating inherited values
     for pcm_name in config_postfix:
         pcm, cfg, children_names, ready = configs[pcm_name]
+
         # Should run
         if cfg == None:
             fail("%s: has not been run" % pcm_name)
+
         # Ready once
         if ready:
             continue
+
         # Children should be ready
         for child_name in children_names:
             if not configs[child_name][3]:
                 fail("%s: child is not ready" % child_name)
 
-        # if _options.trace_modules:
-        #     print(">%s: %s" % (pcm_name, cfg))
-
         _substitute_inherited(configs, pcm_name, cfg)
         _percolate_inherited(configs, pcm_name, cfg, children_names)
         configs[pcm_name] = pcm, cfg, children_names, True
-        # if _options.trace_modules:
-        #     print("<%s: %s" % (pcm_name, cfg))
 
     return globals, configs[top_pcm_name][1]
 
-
 def _substitute_inherited(configs, pcm_name, cfg):
-    """Substitutes inherited values in all the configuration settings."""
+    """Substitutes inherited values in all the attributes.
-    for attr, val in cfg.items():
-        trace_it = attr in _options.trace_variables
-        if trace_it:
-            old_val = val
 
+    When a value of an attribute is a list, some of its items may be
+    references to a value of a same attribute in an inherited product,
+    e.g., for a given module PRODUCT_PACKAGES can be
+      ["foo", (submodule), "bar"]
+    and for 'submodule' PRODUCT_PACKAGES may be ["baz"]
+    (we use a tuple to distinguish submodule references).
+    After the substitution the value of PRODUCT_PACKAGES for the module
+    will become ["foo", "baz", "bar"]
+    """
+    for attr, val in cfg.items():
         # TODO(asmundak): should we handle single vars?
         if type(val) != "list":
             continue
 
-        if trace_it:
+        if attr not in _options.trace_variables:
+            cfg[attr] = _value_expand(configs, attr, val)
+        else:
+            old_val = val
             new_val = _value_expand(configs, attr, val)
             if new_val != old_val:
                 print("%s(i): %s=%s (was %s)" % (pcm_name, attr, new_val, old_val))
             cfg[attr] = new_val
-            continue
-
-        cfg[attr] = _value_expand(configs, attr, val)
-
-
 
 def _value_expand(configs, attr, values_list):
     """Expands references to inherited values in a given list."""
     result = []
-    expanded={}
+    expanded = {}
     for item in values_list:
         # Inherited values are 1-tuples
         if type(item) != "tuple":
@@ -227,7 +234,6 @@ def _value_expand(configs, attr, values_list):
 
     return result
 
-
 def _percolate_inherited(configs, cfg_name, cfg, children_names):
     """Percolates the settings that are present only in children."""
     percolated_attrs = {}
@@ -251,19 +257,16 @@ def _percolate_inherited(configs, cfg_name, cfg, children_names):
         if attr in percolated_attrs:
             print("%s: %s^=%s" % (cfg_name, attr, cfg[attr]))
 
-
 def __move_items(to_list, from_cfg, attr):
-    l = from_cfg.get(attr, [])
+    value = from_cfg.get(attr, [])
-    if l:
+    if value:
-        to_list.extend(l)
+        to_list.extend(value)
         from_cfg[attr] = []
 
-
 def _indirect(pcm_name):
     """Returns configuration item for the inherited module."""
     return (pcm_name,)
 
-
 def _addprefix(prefix, string_or_list):
     """Adds prefix and returns a list.
 
@@ -276,8 +279,7 @@ def _addprefix(prefix, string_or_list):
         string_or_list
 
     """
-    return [ prefix + x for x in __words(string_or_list)]
+    return [prefix + x for x in __words(string_or_list)]
-
 
 def _addsuffix(suffix, string_or_list):
     """Adds suffix and returns a list.
@@ -290,67 +292,82 @@ def _addsuffix(suffix, string_or_list):
       suffix
       string_or_list
     """
-    return [ x + suffix for x in __words(string_or_list)]
+    return [x + suffix for x in __words(string_or_list)]
-
 
 def __words(string_or_list):
     if type(string_or_list) == "list":
         return string_or_list
     return string_or_list.split()
 
-
+# Handle manipulation functions.
+# A handle passed to a PCM consists of:
+#   product attributes dict ("cfg")
+#   inherited modules dict (maps module name to PCM)
+#   default value list (initially empty, modified by inheriting)
 def __h_new():
     """Constructs a handle which is passed to PCM."""
     return (dict(), dict(), list())
 
 def __h_inherited_modules(handle):
+    """Returns PCM's inherited modules dict."""
     return handle[1]
 
-
 def __h_cfg(handle):
+    """Returns PCM's product configuration attributes dict.
+
+    This function is also exported as rblf.cfg, and every PCM
+    calls it at the beginning.
+    """
     return handle[0]
 
-
 def _setdefault(handle, attr):
-    """Sets given attribute's value if it has not been set."""
+    """If attribute has not been set, assigns default value to it.
+
+    This function is exported as rblf.setdefault().
+    Only list attributes are initialized this way. The default
+    value is kept in the PCM's handle. Calling inherit() updates it.
+    """
     cfg = handle[0]
     if cfg.get(attr) == None:
         cfg[attr] = list(handle[2])
     return cfg[attr]
 
 def _inherit(handle, pcm_name, pcm):
-    """Records inheritance."""
+    """Records inheritance.
+
+    This function is exported as rblf.inherit, PCM calls it when
+    a module is inherited.
+    """
     cfg, inherited, default_lv = handle
-    inherited[pcm_name]=pcm
+    inherited[pcm_name] = pcm
     default_lv.append(_indirect(pcm_name))
+
     # Add inherited module reference to all configuration values
     for attr, val in cfg.items():
         if type(val) == "list":
             val.append(_indirect(pcm_name))
 
-
 def _copy_if_exists(path_pair):
     """If from file exists, returns [from:to] pair."""
-    l = path_pair.split(":", 2)
+    value = path_pair.split(":", 2)
+
     # Check that l[0] exists
-    return [":".join(l)] if rblf_file_exists(l[0]) else []
+    return [":".join(value)] if rblf_file_exists(value[0]) else []
 
 def _enforce_product_packages_exist(pkg_string_or_list):
     """Makes including non-existent modules in PRODUCT_PACKAGES an error."""
+
     #TODO(asmundak)
     pass
 
-
 def _file_wildcard_exists(file_pattern):
     """Return True if there are files matching given bash pattern."""
     return len(rblf_wildcard(file_pattern)) > 0
 
-
 def _find_and_copy(pattern, from_dir, to_dir):
     """Return a copy list for the files matching the pattern."""
     return ["%s/%s:%s/%s" % (from_dir, f, to_dir, f) for f in rblf_wildcard(pattern, from_dir)]
 
-
 def _filter_out(pattern, text):
     """Return all the words from `text' that do not match any word in `pattern'.
 
@@ -367,7 +384,6 @@ def _filter_out(pattern, text):
             res.append(w)
     return res
 
-
 def _filter(pattern, text):
     """Return all the words in `text` that match `pattern`.
 
@@ -383,49 +399,39 @@ def _filter(pattern, text):
             res.append(w)
     return res
 
-
 def __mk2regex(words):
     """Returns regular expression equivalent to Make pattern."""
 
     # TODO(asmundak): this will mishandle '\%'
     return "^(" + "|".join([w.replace("%", ".*", 1) for w in words]) + ")"
 
-
 def _regex_match(regex, w):
     return rblf_regex(regex, w)
 
-
 def _require_artifacts_in_path(paths, allowed_paths):
     """TODO."""
-    #print("require_artifacts_in_path(", __words(paths), ",", __words(allowed_paths), ")")
     pass
 
-
 def _require_artifacts_in_path_relaxed(paths, allowed_paths):
     """TODO."""
     pass
 
-
 def _expand_wildcard(pattern):
     """Expands shell wildcard pattern."""
     return rblf_wildcard(pattern)
 
-
+def _mkerror(file, message = ""):
-def _mkerror(file, message=""):
     """Prints error and stops."""
     fail("%s: %s. Stop" % (file, message))
 
-
+def _mkwarning(file, message = ""):
-def _mkwarning(file, message=""):
     """Prints warning."""
     print("%s: warning: %s" % (file, message))
 
-
+def _mkinfo(file, message = ""):
-def _mkinfo(file, message=""):
     """Prints info."""
     print(message)
 
-
 def __get_options():
     """Returns struct containing runtime global settings."""
     settings = dict(
@@ -455,29 +461,29 @@ def __get_options():
 
 # Settings used during debugging.
 _options = __get_options()
-rblf = struct(addprefix=_addprefix,
+rblf = struct(
-              addsuffix=_addsuffix,
+    addprefix = _addprefix,
-              copy_if_exists=_copy_if_exists,
+    addsuffix = _addsuffix,
-              cfg=__h_cfg,
+    copy_if_exists = _copy_if_exists,
-              enforce_product_packages_exist=_enforce_product_packages_exist,
+    cfg = __h_cfg,
-              expand_wildcard=_expand_wildcard,
+    enforce_product_packages_exist = _enforce_product_packages_exist,
-              file_exists=rblf_file_exists,
+    expand_wildcard = _expand_wildcard,
-              file_wildcard_exists=_file_wildcard_exists,
+    file_exists = rblf_file_exists,
-              filter=_filter,
+    file_wildcard_exists = _file_wildcard_exists,
-              filter_out=_filter_out,
+    filter = _filter,
-              find_and_copy=_find_and_copy,
+    filter_out = _filter_out,
-              global_init=_global_init,
+    find_and_copy = _find_and_copy,
-              inherit=_inherit,
+    global_init = _global_init,
-              indirect=_indirect,
+    inherit = _inherit,
-              mkinfo=_mkinfo,
+    indirect = _indirect,
-              mkerror=_mkerror,
+    mkinfo = _mkinfo,
-              mkwarning=_mkwarning,
+    mkerror = _mkerror,
-              printvars=_printvars,
+    mkwarning = _mkwarning,
-              product_configuration=_product_configuration,
+    printvars = _printvars,
-              require_artifacts_in_path=_require_artifacts_in_path,
+    product_configuration = _product_configuration,
-              require_artifacts_in_path_relaxed=_require_artifacts_in_path_relaxed,
+    require_artifacts_in_path = _require_artifacts_in_path,
-              setdefault=_setdefault,
+    require_artifacts_in_path_relaxed = _require_artifacts_in_path_relaxed,
-              shell=rblf_shell,
+    setdefault = _setdefault,
-              warning=_mkwarning,
+    shell = rblf_shell,
-              )
+    warning = _mkwarning,
-
+)