From dd97fd25200f76e1f7f6b89726700684c0ce31bc Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Mon, 28 Feb 2022 19:22:12 +0000 Subject: [PATCH] analyze_bcpf: Compute hidden_api package properties Analyzes the signatures of the class members contained within the bootclasspath_fragment to construct the split_packages, single_packages and package_prefixes hidden_api properties that are necessary to remove the internal implementation details from the hidden API flags. Bug: 202154151 Test: m analyze_bcpf && analyze_bcpf --bcpf art-bootclasspath-fragment m analyze_bcpf && analyze_bcpf --bcpf art-bootclasspath-fragment --fix m analyze_bcpf && analyze_bcpf --bcpf com.android.mediaprovider-bootclasspath-fragment --fix atest --host analyze_bcpf_test Change-Id: I4a8e5a8bfee2a44775e714b9226cd4a7382e0123 --- scripts/hiddenapi/analyze_bcpf.py | 312 ++++++++++++++++++++++++- scripts/hiddenapi/analyze_bcpf_test.py | 160 +++++++++++++ 2 files changed, 469 insertions(+), 3 deletions(-) diff --git a/scripts/hiddenapi/analyze_bcpf.py b/scripts/hiddenapi/analyze_bcpf.py index b5700fb8a..1ad8d07e9 100644 --- a/scripts/hiddenapi/analyze_bcpf.py +++ b/scripts/hiddenapi/analyze_bcpf.py @@ -16,6 +16,7 @@ """Analyze bootclasspath_fragment usage.""" import argparse import dataclasses +import enum import json import logging import os @@ -25,8 +26,12 @@ import subprocess import tempfile import textwrap import typing +from enum import Enum + import sys +from signature_trie import signature_trie + _STUB_FLAGS_FILE = "out/soong/hiddenapi/hiddenapi-stub-flags.txt" _FLAGS_FILE = "out/soong/hiddenapi/hiddenapi-flags.csv" @@ -135,6 +140,16 @@ class FileChange: return self.path < other.path +class PropertyChangeAction(Enum): + """Allowable actions that are supported by HiddenApiPropertyChange.""" + + # New values are appended to any existing values. + APPEND = 1 + + # New values replace any existing values. + REPLACE = 2 + + @dataclasses.dataclass class HiddenApiPropertyChange: @@ -144,6 +159,9 @@ class HiddenApiPropertyChange: property_comment: str = "" + # The action that indicates how this change is applied. + action: PropertyChangeAction = PropertyChangeAction.APPEND + def snippet(self, indent): snippet = "\n" snippet += format_comment_as_text(self.property_comment, indent) @@ -160,7 +178,29 @@ class HiddenApiPropertyChange: # Add an additional placeholder value to identify the modification that # bpmodify makes. bpmodify_values = [_SPECIAL_PLACEHOLDER] - bpmodify_values.extend(self.values) + + if self.action == PropertyChangeAction.APPEND: + # If adding the values to the existing values then pass the new + # values to bpmodify. + bpmodify_values.extend(self.values) + elif self.action == PropertyChangeAction.REPLACE: + # If replacing the existing values then it is not possible to use + # bpmodify for that directly. It could be used twice to remove the + # existing property and then add a new one but that does not remove + # any related comments and loses the position of the existing + # property as the new property is always added to the end of the + # containing block. + # + # So, instead of passing the new values to bpmodify this this just + # adds an extra placeholder to force bpmodify to format the list + # across multiple lines to ensure a consistent structure for the + # code that removes all the existing values and adds the new ones. + # + # This placeholder has to be different to the other placeholder as + # bpmodify dedups values. + bpmodify_values.append(_SPECIAL_PLACEHOLDER + "_REPLACE") + else: + raise ValueError(f"unknown action {self.action}") packages = ",".join(bpmodify_values) bpmodify_runner.add_values_to_property( @@ -176,6 +216,22 @@ class HiddenApiPropertyChange: print(line, file=tio) def fixup_bpmodify_changes(self, bcpf_bp_file, lines): + """Fixup the output of bpmodify. + + The bpmodify tool does not support all the capabilities that this needs + so it is used to do what it can, including marking the place in the + Android.bp file where it makes its changes and then this gets passed a + list of lines from that file which it then modifies to complete the + change. + + This analyzes the list of lines to find the indices of the significant + lines and then applies some changes. As those changes can insert and + delete lines (changing the indices of following lines) the changes are + generally done in reverse order starting from the end and working + towards the beginning. That ensures that the changes do not invalidate + the indices of following lines. + """ + # Find the line containing the placeholder that has been inserted. place_holder_index = -1 for i, line in enumerate(lines): @@ -226,6 +282,22 @@ class HiddenApiPropertyChange: logging.debug("Could not find property line in %s", bcpf_bp_file) return False + # If this change is replacing the existing values then they need to be + # removed and replaced with the new values. That will change the lines + # after the property but it is necessary to do here as the following + # code operates on earlier lines. + if self.action == PropertyChangeAction.REPLACE: + # This removes the existing values and replaces them with the new + # values. + indent = extract_indent(lines[property_line_index + 1]) + insert = [f'{indent}"{x}",' for x in self.values] + lines[property_line_index + 1:end_property_array_index] = insert + if not self.values: + # If the property has no values then merge the ], onto the + # same line as the property name. + del lines[property_line_index + 1] + lines[property_line_index] = lines[property_line_index] + "]," + # Only insert a comment if the property does not already have a comment. line_preceding_property = lines[(property_line_index - 1)] if (self.property_comment and @@ -262,6 +334,21 @@ class Result: default_factory=list) +class ClassProvider(enum.Enum): + """The source of a class found during the hidden API processing""" + BCPF = "bcpf" + OTHER = "other" + + +# A fake member to use when using the signature trie to compute the package +# properties from hidden API flags. This is needed because while that +# computation only cares about classes the trie expects a class to be an +# interior node but without a member it makes the class a leaf node. That causes +# problems when analyzing inner classes as the outer class is a leaf node for +# its own entry but is used as an interior node for inner classes. +_FAKE_MEMBER = ";->fake()V" + + @dataclasses.dataclass() class BcpfAnalyzer: # Path to this tool. @@ -388,7 +475,7 @@ Making sure that {module_info_file} is up to date. return os.path.join(self.out_dir, "soong/.intermediates", module_path, module_name) - def find_bootclasspath_fragment_output_file(self, basename): + def find_bootclasspath_fragment_output_file(self, basename, required=True): # Find the output file of the bootclasspath_fragment with the specified # base name. found_file = "" @@ -398,7 +485,7 @@ Making sure that {module_info_file} is up to date. if f == basename: found_file = os.path.join(dirpath, f) break - if not found_file: + if not found_file and required: raise Exception(f"Could not find {basename} in {bcpf_out_dir}") return found_file @@ -475,6 +562,8 @@ Cleaning potentially stale files. result = Result() self.build_monolithic_flags(result) + self.analyze_hiddenapi_package_properties(result) + self.explain_how_to_check_signature_patterns() # If there were any changes that need to be made to the Android.bp # file then either apply or report them. @@ -929,6 +1018,223 @@ Checking custom hidden API flags.... result, property_name, flags_file, rel_bcpf_flags_file, bcpf_flags_file) + @staticmethod + def split_package_comment(split_packages): + if split_packages: + return textwrap.dedent(""" + The following packages contain classes from other modules on the + bootclasspath. That means that the hidden API flags for this + module has to explicitly list every single class this module + provides in that package to differentiate them from the classes + provided by other modules. That can include private classes that + are not part of the API. + """).strip("\n") + + return "This module does not contain any split packages." + + @staticmethod + def package_prefixes_comment(): + return textwrap.dedent(""" + The following packages and all their subpackages currently only + contain classes from this bootclasspath_fragment. Listing a package + here won't prevent other bootclasspath modules from adding classes + in any of those packages but it will prevent them from adding those + classes into an API surface, e.g. public, system, etc.. Doing so + will result in a build failure due to inconsistent flags. + """).strip("\n") + + def analyze_hiddenapi_package_properties(self, result): + split_packages, single_packages, package_prefixes = \ + self.compute_hiddenapi_package_properties() + + # TODO(b/202154151): Find those classes in split packages that are not + # part of an API, i.e. are an internal implementation class, and so + # can, and should, be safely moved out of the split packages. + + result.property_changes.append( + HiddenApiPropertyChange( + property_name="split_packages", + values=split_packages, + property_comment=self.split_package_comment(split_packages), + action=PropertyChangeAction.REPLACE, + )) + + if split_packages: + self.report(f""" +bootclasspath_fragment {self.bcpf} contains classes in packages that also +contain classes provided by other sources, those packages are called split +packages. Split packages should be avoided where possible but are often +unavoidable when modularizing existing code. + +The hidden api processing needs to know which packages are split (and conversely +which are not) so that it can optimize the hidden API flags to remove +unnecessary implementation details. +""") + + self.report(""" +By default (for backwards compatibility) the bootclasspath_fragment assumes that +all packages are split unless one of the package_prefixes or split_packages +properties are specified. While that is safe it is not optimal and can lead to +unnecessary implementation details leaking into the hidden API flags. Adding an +empty split_packages property allows the flags to be optimized and remove any +unnecessary implementation details. +""") + + if single_packages: + result.property_changes.append( + HiddenApiPropertyChange( + property_name="single_packages", + values=single_packages, + property_comment=textwrap.dedent(""" + The following packages currently only contain classes from + this bootclasspath_fragment but some of their sub-packages + contain classes from other bootclasspath modules. Packages + should only be listed here when necessary for legacy + purposes, new packages should match a package prefix. + """), + action=PropertyChangeAction.REPLACE, + )) + + if package_prefixes: + result.property_changes.append( + HiddenApiPropertyChange( + property_name="package_prefixes", + values=package_prefixes, + property_comment=self.package_prefixes_comment(), + action=PropertyChangeAction.REPLACE, + )) + + def explain_how_to_check_signature_patterns(self): + signature_patterns_files = self.find_bootclasspath_fragment_output_file( + "signature-patterns.csv", required=False) + if signature_patterns_files: + signature_patterns_files = signature_patterns_files.removeprefix( + self.top_dir) + + self.report(f""" +The purpose of the hiddenapi split_packages and package_prefixes properties is +to allow the removal of implementation details from the hidden API flags to +reduce the coupling between sdk snapshots and the APEX runtime. It cannot +eliminate that coupling completely though. Doing so may require changes to the +code. + +This tool provides support for managing those properties but it cannot decide +whether the set of package prefixes suggested is appropriate that needs the +input of the developer. + +Please run the following command: + m {signature_patterns_files} + +And then check the '{signature_patterns_files}' for any mention of +implementation classes and packages (i.e. those classes/packages that do not +contain any part of an API surface, including the hidden API). If they are +found then the code should ideally be moved to a package unique to this module +that is contained within a package that is part of an API surface. + +The format of the file is a list of patterns: + +* Patterns for split packages will list every class in that package. + +* Patterns for package prefixes will end with .../**. + +* Patterns for packages which are not split but cannot use a package prefix +because there are sub-packages which are provided by another module will end +with .../*. +""") + + def compute_hiddenapi_package_properties(self): + trie = signature_trie() + # Populate the trie with the classes that are provided by the + # bootclasspath_fragment tagging them to make it clear where they + # are from. + sorted_classes = sorted(self.classes) + for class_name in sorted_classes: + trie.add(class_name + _FAKE_MEMBER, ClassProvider.BCPF) + + monolithic_classes = set() + abs_flags_file = os.path.join(self.top_dir, _FLAGS_FILE) + with open(abs_flags_file, "r", encoding="utf8") as f: + for line in iter(f.readline, ""): + signature = self.line_to_signature(line) + class_name = self.signature_to_class(signature) + if (class_name not in monolithic_classes and + class_name not in self.classes): + trie.add( + class_name + _FAKE_MEMBER, + ClassProvider.OTHER, + only_if_matches=True) + monolithic_classes.add(class_name) + + split_packages = [] + single_packages = [] + package_prefixes = [] + self.recurse_hiddenapi_packages_trie(trie, split_packages, + single_packages, package_prefixes) + return split_packages, single_packages, package_prefixes + + def recurse_hiddenapi_packages_trie(self, node, split_packages, + single_packages, package_prefixes): + nodes = node.child_nodes() + if nodes: + for child in nodes: + # Ignore any non-package nodes. + if child.type != "package": + continue + + package = child.selector.replace("/", ".") + + providers = set(child.get_matching_rows("**")) + if not providers: + # The package and all its sub packages contain no + # classes. This should never happen. + pass + elif providers == {ClassProvider.BCPF}: + # The package and all its sub packages only contain + # classes provided by the bootclasspath_fragment. + logging.debug("Package '%s.**' is not split", package) + package_prefixes.append(package) + # There is no point traversing into the sub packages. + continue + elif providers == {ClassProvider.OTHER}: + # The package and all its sub packages contain no + # classes provided by the bootclasspath_fragment. + # There is no point traversing into the sub packages. + logging.debug("Package '%s.**' contains no classes from %s", + package, self.bcpf) + continue + elif ClassProvider.BCPF in providers: + # The package and all its sub packages contain classes + # provided by the bootclasspath_fragment and other + # sources. + logging.debug( + "Package '%s.**' contains classes from " + "%s and other sources", package, self.bcpf) + + providers = set(child.get_matching_rows("*")) + if not providers: + # The package contains no classes. + logging.debug("Package: %s contains no classes", package) + elif providers == {ClassProvider.BCPF}: + # The package only contains classes provided by the + # bootclasspath_fragment. + logging.debug("Package '%s.*' is not split", package) + single_packages.append(package) + elif providers == {ClassProvider.OTHER}: + # The package contains no classes provided by the + # bootclasspath_fragment. Child nodes make contain such + # classes. + logging.debug("Package '%s.*' contains no classes from %s", + package, self.bcpf) + elif ClassProvider.BCPF in providers: + # The package contains classes provided by both the + # bootclasspath_fragment and some other source. + logging.debug("Package '%s.*' is split", package) + split_packages.append(package) + + self.recurse_hiddenapi_packages_trie(child, split_packages, + single_packages, + package_prefixes) + def newline_stripping_iter(iterator): """Return an iterator over the iterator that strips trailing white space.""" diff --git a/scripts/hiddenapi/analyze_bcpf_test.py b/scripts/hiddenapi/analyze_bcpf_test.py index 2259b428c..650dd5494 100644 --- a/scripts/hiddenapi/analyze_bcpf_test.py +++ b/scripts/hiddenapi/analyze_bcpf_test.py @@ -358,6 +358,39 @@ Lacme/items/Lever;->size:I self.assertEqual( expected_contents, contents, msg=f"{path} contents") + def test_compute_hiddenapi_package_properties(self): + fs = { + "out/soong/.intermediates/bcpf-dir/bcpf/all-flags.csv": + """ +La/b/C;->m()V +La/b/c/D;->m()V +La/b/c/E;->m()V +Lb/c/D;->m()V +Lb/c/E;->m()V +Lb/c/d/E;->m()V +""", + "out/soong/hiddenapi/hiddenapi-flags.csv": + """ +La/b/C;->m()V +La/b/D;->m()V +La/b/E;->m()V +La/b/c/D;->m()V +La/b/c/E;->m()V +La/b/c/d/E;->m()V +Lb/c/D;->m()V +Lb/c/E;->m()V +Lb/c/d/E;->m()V +""" + } + analyzer = self.create_analyzer_for_test(fs) + analyzer.load_all_flags() + + split_packages, single_packages, package_prefixes = \ + analyzer.compute_hiddenapi_package_properties() + self.assertEqual(["a.b"], split_packages) + self.assertEqual(["a.b.c"], single_packages) + self.assertEqual(["b"], package_prefixes) + class TestHiddenApiPropertyChange(unittest.TestCase): @@ -485,6 +518,133 @@ bootclasspath_fragment { } """) + def test_set_property_with_value_and_comment(self): + change = ab.HiddenApiPropertyChange( + property_name="split_packages", + values=["another.provider", "other.system"], + property_comment=_MULTI_LINE_COMMENT, + action=ab.PropertyChangeAction.REPLACE, + ) + + self.check_change_snippet( + change, """ + // Lorem ipsum dolor sit amet, consectetur adipiscing elit. Ut arcu + // justo, bibendum eu malesuada vel, fringilla in odio. Etiam gravida + // ultricies sem tincidunt luctus. + split_packages: [ + "another.provider", + "other.system", + ], +""") + + self.check_change_fix( + change, """ +bootclasspath_fragment { + name: "bcpf", + + // modified by the Soong or platform compat team. + hidden_api: { + max_target_o_low_priority: ["hiddenapi/hiddenapi-max-target-o-low-priority.txt"], + split_packages: [ + "another.provider", + "other.system", + ], + }, +} +""", """ +bootclasspath_fragment { + name: "bcpf", + + // modified by the Soong or platform compat team. + hidden_api: { + max_target_o_low_priority: ["hiddenapi/hiddenapi-max-target-o-low-priority.txt"], + + // Lorem ipsum dolor sit amet, consectetur adipiscing elit. Ut arcu + // justo, bibendum eu malesuada vel, fringilla in odio. Etiam gravida + // ultricies sem tincidunt luctus. + split_packages: [ + "another.provider", + "other.system", + ], + }, +} +""") + + def test_set_property_with_no_value_or_comment(self): + change = ab.HiddenApiPropertyChange( + property_name="split_packages", + values=[], + action=ab.PropertyChangeAction.REPLACE, + ) + + self.check_change_snippet(change, """ + split_packages: [], +""") + + self.check_change_fix( + change, """ +bootclasspath_fragment { + name: "bcpf", + + // modified by the Soong or platform compat team. + hidden_api: { + max_target_o_low_priority: ["hiddenapi/hiddenapi-max-target-o-low-priority.txt"], + split_packages: [ + "another.provider", + "other.system", + ], + package_prefixes: ["android.provider"], + }, +} +""", """ +bootclasspath_fragment { + name: "bcpf", + + // modified by the Soong or platform compat team. + hidden_api: { + max_target_o_low_priority: ["hiddenapi/hiddenapi-max-target-o-low-priority.txt"], + split_packages: [], + package_prefixes: ["android.provider"], + }, +} +""") + + def test_set_empty_property_with_no_value_or_comment(self): + change = ab.HiddenApiPropertyChange( + property_name="split_packages", + values=[], + action=ab.PropertyChangeAction.REPLACE, + ) + + self.check_change_snippet(change, """ + split_packages: [], +""") + + self.check_change_fix( + change, """ +bootclasspath_fragment { + name: "bcpf", + + // modified by the Soong or platform compat team. + hidden_api: { + max_target_o_low_priority: ["hiddenapi/hiddenapi-max-target-o-low-priority.txt"], + split_packages: [], + package_prefixes: ["android.provider"], + }, +} +""", """ +bootclasspath_fragment { + name: "bcpf", + + // modified by the Soong or platform compat team. + hidden_api: { + max_target_o_low_priority: ["hiddenapi/hiddenapi-max-target-o-low-priority.txt"], + split_packages: [], + package_prefixes: ["android.provider"], + }, +} +""") + if __name__ == "__main__": unittest.main(verbosity=3)