analyze_bcpf: Explain why a package is split/single
Previously, the script would determine whether a package was split,
single or could be used as a prefix but did not explain why. This
change provides additional information to explain why they are split.
Bug: 202154151
Test: m analyze_bcpf && analyze_bcpf --bcpf art-bootclasspath-fragment
m analyze_bcpf && analyze_bcpf --bcpf art-bootclasspath-fragment --fix
atest --host analyze_bcpf_test
Change-Id: I3e2d5c0b54b5cc028013ce5ea979ebd9b9bf2c0d
diff --git a/scripts/hiddenapi/analyze_bcpf.py b/scripts/hiddenapi/analyze_bcpf.py
index 9e8807e..595343b 100644
--- a/scripts/hiddenapi/analyze_bcpf.py
+++ b/scripts/hiddenapi/analyze_bcpf.py
@@ -319,12 +319,48 @@
@dataclasses.dataclass()
+class PackagePropertyReason:
+ """Provides the reasons why a package was added to a specific property.
+
+ A split package is one that contains classes from the bootclasspath_fragment
+ and other bootclasspath modules. So, for a split package this contains the
+ corresponding lists of classes.
+
+ A single package is one that contains classes sub-packages from the
+ For a split package this contains a list of classes in that package that are
+ provided by the bootclasspath_fragment and a list of classes
+ """
+
+ # The list of classes/sub-packages that is provided by the
+ # bootclasspath_fragment.
+ bcpf: typing.List[str]
+
+ # The list of classes/sub-packages that is provided by other modules on the
+ # bootclasspath.
+ other: typing.List[str]
+
+
+@dataclasses.dataclass()
class Result:
"""Encapsulates the result of the analysis."""
# The diffs in the flags.
diffs: typing.Optional[FlagDiffs] = None
+ # A map from package name to the reason why it belongs in the
+ # split_packages property.
+ split_packages: typing.Dict[str, PackagePropertyReason] = dataclasses.field(
+ default_factory=dict)
+
+ # A map from package name to the reason why it belongs in the
+ # single_packages property.
+ single_packages: typing.Dict[str,
+ PackagePropertyReason] = dataclasses.field(
+ default_factory=dict)
+
+ # The list of packages to add to the package_prefixes property.
+ package_prefixes: typing.List[str] = dataclasses.field(default_factory=list)
+
# The bootclasspath_fragment hidden API properties changes.
property_changes: typing.List[HiddenApiPropertyChange] = dataclasses.field(
default_factory=list)
@@ -1053,13 +1089,16 @@
""").strip("\n")
def analyze_hiddenapi_package_properties(self, result):
- split_packages, single_packages, package_prefixes = \
- self.compute_hiddenapi_package_properties()
+ self.compute_hiddenapi_package_properties(result)
+
+ def indent_lines(lines):
+ return "\n".join([f" {cls}" for cls in lines])
# 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.
+ split_packages = result.split_packages.keys()
result.property_changes.append(
HiddenApiPropertyChange(
property_name="split_packages",
@@ -1090,6 +1129,19 @@
details.
""")
+ for package in split_packages:
+ reason = result.split_packages[package]
+ self.report(f"""
+ Package {package} is split because while this bootclasspath_fragment
+ provides the following classes:
+{indent_lines(reason.bcpf)}
+
+ Other module(s) on the bootclasspath provides the following classes in
+ that package:
+{indent_lines(reason.other)}
+""")
+
+ single_packages = result.single_packages.keys()
if single_packages:
result.property_changes.append(
HiddenApiPropertyChange(
@@ -1105,6 +1157,30 @@
action=PropertyChangeAction.REPLACE,
))
+ self.report_dedent(f"""
+ bootclasspath_fragment {self.bcpf} contains classes from
+ packages that has sub-packages which contain classes provided by
+ other bootclasspath modules. Those packages are called single
+ packages. Single packages should be avoided where possible but
+ are often unavoidable when modularizing existing code.
+
+ Because some sub-packages contains classes from other
+ bootclasspath modules it is not possible to use the package as a
+ package prefix as that treats the package and all its
+ sub-packages as being provided by this module.
+ """)
+ for package in single_packages:
+ reason = result.single_packages[package]
+ self.report(f"""
+ Package {package} is not a package prefix because while this
+ bootclasspath_fragment provides the following sub-packages:
+{indent_lines(reason.bcpf)}
+
+ Other module(s) on the bootclasspath provide the following sub-packages:
+{indent_lines(reason.other)}
+""")
+
+ package_prefixes = result.package_prefixes
if package_prefixes:
result.property_changes.append(
HiddenApiPropertyChange(
@@ -1154,7 +1230,7 @@
by another module will end with .../*.
""")
- def compute_hiddenapi_package_properties(self):
+ def compute_hiddenapi_package_properties(self, result):
trie = signature_trie()
# Populate the trie with the classes that are provided by the
# bootclasspath_fragment tagging them to make it clear where they
@@ -1163,6 +1239,7 @@
for class_name in sorted_classes:
trie.add(class_name + _FAKE_MEMBER, ClassProvider.BCPF)
+ # Now the same for monolithic classes.
monolithic_classes = set()
abs_flags_file = os.path.join(self.top_dir, _FLAGS_FILE)
with open(abs_flags_file, "r", encoding="utf8") as f:
@@ -1177,15 +1254,54 @@
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
+ self.recurse_hiddenapi_packages_trie(trie, result)
- def recurse_hiddenapi_packages_trie(self, node, split_packages,
- single_packages, package_prefixes):
+ @staticmethod
+ def selector_to_java_reference(node):
+ return node.selector.replace("/", ".")
+
+ @staticmethod
+ def determine_reason_for_single_package(node):
+ bcpf_packages = []
+ other_packages = []
+
+ def recurse(n):
+ if n.type != "package":
+ return
+
+ providers = n.get_matching_rows("*")
+ package_ref = BcpfAnalyzer.selector_to_java_reference(n)
+ if ClassProvider.BCPF in providers:
+ bcpf_packages.append(package_ref)
+ else:
+ other_packages.append(package_ref)
+
+ children = n.child_nodes()
+ if children:
+ for child in children:
+ recurse(child)
+
+ recurse(node)
+ return PackagePropertyReason(bcpf=bcpf_packages, other=other_packages)
+
+ @staticmethod
+ def determine_reason_for_split_package(node):
+ bcpf_classes = []
+ other_classes = []
+ for child in node.child_nodes():
+ if child.type != "class":
+ continue
+
+ providers = child.values(lambda _: True)
+ class_ref = BcpfAnalyzer.selector_to_java_reference(child)
+ if ClassProvider.BCPF in providers:
+ bcpf_classes.append(class_ref)
+ else:
+ other_classes.append(class_ref)
+
+ return PackagePropertyReason(bcpf=bcpf_classes, other=other_classes)
+
+ def recurse_hiddenapi_packages_trie(self, node, result):
nodes = node.child_nodes()
if nodes:
for child in nodes:
@@ -1193,7 +1309,7 @@
if child.type != "package":
continue
- package = child.selector.replace("/", ".")
+ package = self.selector_to_java_reference(child)
providers = set(child.get_matching_rows("**"))
if not providers:
@@ -1204,7 +1320,7 @@
# 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)
+ result.package_prefixes.append(package)
# There is no point traversing into the sub packages.
continue
elif providers == {ClassProvider.OTHER}:
@@ -1229,8 +1345,17 @@
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)
+ logging.debug(
+ "Package '%s.*' is not split but does have "
+ "sub-packages from other modules", package)
+
+ # Partition the sub-packages into those that are provided by
+ # this bootclasspath_fragment and those provided by other
+ # modules. They can be used to explain the reason for the
+ # single package to developers.
+ reason = self.determine_reason_for_single_package(child)
+ result.single_packages[package] = reason
+
elif providers == {ClassProvider.OTHER}:
# The package contains no classes provided by the
# bootclasspath_fragment. Child nodes make contain such
@@ -1241,11 +1366,15 @@
# 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)
+ # Partition the classes in this split package into those
+ # that come from this bootclasspath_fragment and those that
+ # come from other modules. That can be used to explain the
+ # reason for the split package to developers.
+ reason = self.determine_reason_for_split_package(child)
+ result.split_packages[package] = reason
+
+ self.recurse_hiddenapi_packages_trie(child, result)
def newline_stripping_iter(iterator):
diff --git a/scripts/hiddenapi/analyze_bcpf_test.py b/scripts/hiddenapi/analyze_bcpf_test.py
index 650dd54..a32ffd0 100644
--- a/scripts/hiddenapi/analyze_bcpf_test.py
+++ b/scripts/hiddenapi/analyze_bcpf_test.py
@@ -377,6 +377,7 @@
La/b/c/D;->m()V
La/b/c/E;->m()V
La/b/c/d/E;->m()V
+La/b/c/d/e/F;->m()V
Lb/c/D;->m()V
Lb/c/E;->m()V
Lb/c/d/E;->m()V
@@ -385,11 +386,21 @@
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)
+ result = ab.Result()
+ analyzer.compute_hiddenapi_package_properties(result)
+ self.assertEqual(["a.b"], list(result.split_packages.keys()))
+
+ reason = result.split_packages["a.b"]
+ self.assertEqual(["a.b.C"], reason.bcpf)
+ self.assertEqual(["a.b.D", "a.b.E"], reason.other)
+
+ self.assertEqual(["a.b.c"], list(result.single_packages.keys()))
+
+ reason = result.single_packages["a.b.c"]
+ self.assertEqual(["a.b.c"], reason.bcpf)
+ self.assertEqual(["a.b.c.d", "a.b.c.d.e"], reason.other)
+
+ self.assertEqual(["b"], result.package_prefixes)
class TestHiddenApiPropertyChange(unittest.TestCase):