analyze_bcpf: Add --fix option am: 26f19919ea am: 37a95341d9

Original change: https://android-review.googlesource.com/c/platform/build/soong/+/2047416

Change-Id: I23ae9e0208e5297f8b9297039e79499b4796c31c
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

android/bazel.go

@@ -509,6 +509,9 @@ var (
 
 		"brotli-fuzzer-corpus", // b/202015218: outputs are in location incompatible with bazel genrule handling.
 
+		// python modules
+		"analyze_bcpf", // depends on bpmodify a blueprint_go_binary.
+
 		// b/203369847: multiple genrules in the same package creating the same file
 		// //development/sdk/...
 		"platform_tools_properties",

scripts/hiddenapi/Android.bp

@@ -22,6 +22,8 @@ python_binary_host {
     name: "analyze_bcpf",
     main: "analyze_bcpf.py",
     srcs: ["analyze_bcpf.py"],
+    // Make sure that the bpmodify tool is built.
+    data: [":bpmodify"],
     libs: [
         "signature_trie",
     ],
@@ -43,6 +45,8 @@ python_test_host {
         "analyze_bcpf.py",
         "analyze_bcpf_test.py",
     ],
+    // Make sure that the bpmodify tool is built.
+    data: [":bpmodify"],
     libs: [
         "signature_trie",
     ],

scripts/hiddenapi/analyze_bcpf.py

@@ -98,6 +98,33 @@ class ModuleInfo:
         return paths[0]
 
 
+def extract_indent(line):
+    return re.match(r"([ \t]*)", line).group(1)
+
+
+_SPECIAL_PLACEHOLDER: str = "SPECIAL_PLACEHOLDER"
+
+
+@dataclasses.dataclass
+class BpModifyRunner:
+
+    bpmodify_path: str
+
+    def add_values_to_property(self, property_name, values, module_name,
+                               bp_file):
+        cmd = [
+            self.bpmodify_path, "-a", values, "-property", property_name, "-m",
+            module_name, "-w", bp_file, bp_file
+        ]
+
+        logging.debug(" ".join(cmd))
+        subprocess.run(
+            cmd,
+            stderr=subprocess.STDOUT,
+            stdout=log_stream_for_subprocess(),
+            check=True)
+
+
 @dataclasses.dataclass
 class FileChange:
     path: str
@@ -129,6 +156,95 @@ class HiddenApiPropertyChange:
         snippet += "],\n"
         return snippet
 
+    def fix_bp_file(self, bcpf_bp_file, bcpf, bpmodify_runner: BpModifyRunner):
+        # Add an additional placeholder value to identify the modification that
+        # bpmodify makes.
+        bpmodify_values = [_SPECIAL_PLACEHOLDER]
+        bpmodify_values.extend(self.values)
+
+        packages = ",".join(bpmodify_values)
+        bpmodify_runner.add_values_to_property(
+            f"hidden_api.{self.property_name}", packages, bcpf, bcpf_bp_file)
+
+        with open(bcpf_bp_file, "r", encoding="utf8") as tio:
+            lines = tio.readlines()
+            lines = [line.rstrip("\n") for line in lines]
+
+        if self.fixup_bpmodify_changes(bcpf_bp_file, lines):
+            with open(bcpf_bp_file, "w", encoding="utf8") as tio:
+                for line in lines:
+                    print(line, file=tio)
+
+    def fixup_bpmodify_changes(self, bcpf_bp_file, lines):
+        # Find the line containing the placeholder that has been inserted.
+        place_holder_index = -1
+        for i, line in enumerate(lines):
+            if _SPECIAL_PLACEHOLDER in line:
+                place_holder_index = i
+                break
+        if place_holder_index == -1:
+            logging.debug("Could not find %s in %s", _SPECIAL_PLACEHOLDER,
+                          bcpf_bp_file)
+            return False
+
+        # Remove the place holder. Do this before inserting the comment as that
+        # would change the location of the place holder in the list.
+        place_holder_line = lines[place_holder_index]
+        if place_holder_line.endswith("],"):
+            place_holder_line = place_holder_line.replace(
+                f'"{_SPECIAL_PLACEHOLDER}"', "")
+            lines[place_holder_index] = place_holder_line
+        else:
+            del lines[place_holder_index]
+
+        # Scan forward to the end of the property block to remove a blank line
+        # that bpmodify inserts.
+        end_property_array_index = -1
+        for i in range(place_holder_index, len(lines)):
+            line = lines[i]
+            if line.endswith("],"):
+                end_property_array_index = i
+                break
+        if end_property_array_index == -1:
+            logging.debug("Could not find end of property array in %s",
+                          bcpf_bp_file)
+            return False
+
+        # If bdmodify inserted a blank line afterwards then remove it.
+        if (not lines[end_property_array_index + 1] and
+                lines[end_property_array_index + 2].endswith("},")):
+            del lines[end_property_array_index + 1]
+
+        # Scan back to find the preceding property line.
+        property_line_index = -1
+        for i in range(place_holder_index, 0, -1):
+            line = lines[i]
+            if line.lstrip().startswith(f"{self.property_name}: ["):
+                property_line_index = i
+                break
+        if property_line_index == -1:
+            logging.debug("Could not find property line in %s", bcpf_bp_file)
+            return False
+
+        # 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
+                not re.match("([ \t]+)// ", line_preceding_property)):
+            # Extract the indent from the property line and use it to format the
+            # comment.
+            indent = extract_indent(lines[property_line_index])
+            comment_lines = format_comment_as_lines(self.property_comment,
+                                                    indent)
+
+            # If the line before the comment is not blank then insert an extra
+            # blank line at the beginning of the comment.
+            if line_preceding_property:
+                comment_lines.insert(0, "")
+
+            # Insert the comment before the property.
+            lines[property_line_index:property_line_index] = comment_lines
+        return True
+
 
 @dataclasses.dataclass()
 class Result:
@@ -148,6 +264,9 @@ class Result:
 
 @dataclasses.dataclass()
 class BcpfAnalyzer:
+    # Path to this tool.
+    tool_path: str
+
     # Directory pointed to by ANDROID_BUILD_OUT
     top_dir: str
 
@@ -168,6 +287,10 @@ class BcpfAnalyzer:
     # informational purposes.
     sdk: str
 
+    # If true then this will attempt to automatically fix any issues that are
+    # found.
+    fix: bool = False
+
     # All the signatures, loaded from all-flags.csv, initialized by
     # load_all_flags().
     _signatures: typing.Set[str] = dataclasses.field(default_factory=set)
@@ -354,20 +477,34 @@ Cleaning potentially stale files.
         self.build_monolithic_flags(result)
 
         # If there were any changes that need to be made to the Android.bp
-        # file then report them.
+        # file then either apply or report them.
         if result.property_changes:
             bcpf_dir = self.module_info.module_path(self.bcpf)
             bcpf_bp_file = os.path.join(self.top_dir, bcpf_dir, "Android.bp")
-            hiddenapi_snippet = ""
+            if self.fix:
-            for property_change in result.property_changes:
+                tool_dir = os.path.dirname(self.tool_path)
-                hiddenapi_snippet += property_change.snippet("        ")
+                bpmodify_path = os.path.join(tool_dir, "bpmodify")
+                bpmodify_runner = BpModifyRunner(bpmodify_path)
+                for property_change in result.property_changes:
+                    property_change.fix_bp_file(bcpf_bp_file, self.bcpf,
+                                                bpmodify_runner)
 
-            # Remove leading and trailing blank lines.
+                result.file_changes.append(
-            hiddenapi_snippet = hiddenapi_snippet.strip("\n")
+                    self.new_file_change(
+                        bcpf_bp_file,
+                        f"Updated hidden_api properties of '{self.bcpf}'"))
 
-            result.file_changes.append(
+            else:
-                self.new_file_change(
+                hiddenapi_snippet = ""
-                    bcpf_bp_file, f"""
+                for property_change in result.property_changes:
+                    hiddenapi_snippet += property_change.snippet("        ")
+
+                # Remove leading and trailing blank lines.
+                hiddenapi_snippet = hiddenapi_snippet.strip("\n")
+
+                result.file_changes.append(
+                    self.new_file_change(
+                        bcpf_bp_file, f"""
 Add the following snippet into the {self.bcpf} bootclasspath_fragment module
 in the {bcpf_dir}/Android.bp file. If the hidden_api block already exists then
 merge these properties into it.
@@ -378,8 +515,15 @@ merge these properties into it.
 """))
 
         if result.file_changes:
-            self.report("""
+            if self.fix:
-The following modifications need to be made:""")
+                file_change_message = """
+The following files were modified by this script:"""
+            else:
+                file_change_message = """
+The following modifications need to be made:"""
+
+            self.report(f"""
+{file_change_message}""")
             result.file_changes.sort()
             for file_change in result.file_changes:
                 self.report(f"""
@@ -387,6 +531,12 @@ The following modifications need to be made:""")
         {file_change.description}
 """.lstrip("\n"))
 
+            if not self.fix:
+                self.report("""
+Run the command again with the --fix option to automatically make the above
+changes.
+""".lstrip())
+
     def new_file_change(self, file, description):
         return FileChange(
             path=os.path.relpath(file, self.top_dir), description=description)
@@ -665,6 +815,81 @@ Checking custom hidden API flags....
                     values=[rel_bcpf_flags_file],
                 ))
 
+    def fix_hidden_api_flag_files(self, result, property_name, flags_file,
+                                  rel_bcpf_flags_file, bcpf_flags_file):
+        # Read the file in frameworks/base/boot/hiddenapi/<file> copy any
+        # flags that relate to the bootclasspath_fragment into a local
+        # file in the hiddenapi subdirectory.
+        tmp_flags_file = flags_file + ".tmp"
+
+        # Make sure the directory containing the bootclasspath_fragment specific
+        # hidden api flags exists.
+        os.makedirs(os.path.dirname(bcpf_flags_file), exist_ok=True)
+
+        bcpf_flags_file_exists = os.path.exists(bcpf_flags_file)
+
+        matched_signatures = set()
+        # Open the flags file to read the flags from.
+        with open(flags_file, "r", encoding="utf8") as f:
+            # Open a temporary file to write the flags (minus any removed
+            # flags).
+            with open(tmp_flags_file, "w", encoding="utf8") as t:
+                # Open the bootclasspath_fragment file for append just in
+                # case it already exists.
+                with open(bcpf_flags_file, "a", encoding="utf8") as b:
+                    for line in iter(f.readline, ""):
+                        signature = line.rstrip()
+                        if signature in self.signatures:
+                            # The signature is provided by the
+                            # bootclasspath_fragment so write it to the new
+                            # bootclasspath_fragment specific file.
+                            print(line, file=b, end="")
+                            matched_signatures.add(signature)
+                        else:
+                            # The signature is NOT provided by the
+                            # bootclasspath_fragment. Copy it to the new
+                            # monolithic file.
+                            print(line, file=t, end="")
+
+        # If the bootclasspath_fragment specific flags file is not empty
+        # then it contains flags. That could either be new flags just moved
+        # from frameworks/base or previous contents of the file. In either
+        # case the file must not be removed.
+        if matched_signatures:
+            # There are custom flags related to the bootclasspath_fragment
+            # so replace the frameworks/base/boot/hiddenapi file with the
+            # file that does not contain those flags.
+            shutil.move(tmp_flags_file, flags_file)
+
+            result.file_changes.append(
+                self.new_file_change(flags_file,
+                                     f"Removed '{self.bcpf}' specific entries"))
+
+            result.property_changes.append(
+                HiddenApiPropertyChange(
+                    property_name=property_name,
+                    values=[rel_bcpf_flags_file],
+                ))
+
+            # Make sure that the files are sorted.
+            self.run_command([
+                "tools/platform-compat/hiddenapi/sort_api.sh",
+                bcpf_flags_file,
+            ])
+
+            if bcpf_flags_file_exists:
+                desc = f"Added '{self.bcpf}' specific entries"
+            else:
+                desc = f"Created with '{self.bcpf}' specific entries"
+            result.file_changes.append(
+                self.new_file_change(bcpf_flags_file, desc))
+        else:
+            # There are no custom flags related to the
+            # bootclasspath_fragment so clean up the working files.
+            os.remove(tmp_flags_file)
+            if not bcpf_flags_file_exists:
+                os.remove(bcpf_flags_file)
+
     def check_frameworks_base_boot_hidden_api_files(self, result):
         hiddenapi_dir = os.path.join(self.top_dir,
                                      "frameworks/base/boot/hiddenapi")
@@ -695,10 +920,14 @@ Checking custom hidden API flags....
             bcpf_flags_file = os.path.join(self.top_dir, bcpf_dir,
                                            rel_bcpf_flags_file)
 
-            self.report_hidden_api_flag_file_changes(result, property_name,
+            if self.fix:
-                                                     flags_file,
+                self.fix_hidden_api_flag_files(result, property_name,
-                                                     rel_bcpf_flags_file,
+                                               flags_file, rel_bcpf_flags_file,
-                                                     bcpf_flags_file)
+                                               bcpf_flags_file)
+            else:
+                self.report_hidden_api_flag_file_changes(
+                    result, property_name, flags_file, rel_bcpf_flags_file,
+                    bcpf_flags_file)
 
 
 def newline_stripping_iter(iterator):
@@ -750,6 +979,11 @@ def main(argv):
         "allow this script to give more useful messages and it may be"
         "required in future.",
         default="SPECIFY-SDK-OPTION")
+    args_parser.add_argument(
+        "--fix",
+        help="Attempt to fix any issues found automatically.",
+        action="store_true",
+        default=False)
     args = args_parser.parse_args(argv[1:])
     top_dir = os.environ["ANDROID_BUILD_TOP"] + "/"
     out_dir = os.environ.get("OUT_DIR", os.path.join(top_dir, "out"))
@@ -778,12 +1012,14 @@ def main(argv):
         print(f"Writing log to {abs_log_file}")
         try:
             analyzer = BcpfAnalyzer(
+                tool_path=argv[0],
                 top_dir=top_dir,
                 out_dir=out_dir,
                 product_out_dir=product_out_dir,
                 bcpf=args.bcpf,
                 apex=args.apex,
                 sdk=args.sdk,
+                fix=args.fix,
             )
             analyzer.analyze()
         finally:

scripts/hiddenapi/analyze_bcpf_test.py

@@ -20,6 +20,8 @@ import tempfile
 import unittest
 import unittest.mock
 
+import sys
+
 import analyze_bcpf as ab
 
 _FRAMEWORK_HIDDENAPI = "frameworks/base/boot/hiddenapi"
@@ -73,7 +75,8 @@ class TestAnalyzeBcpf(unittest.TestCase):
                                  fs=None,
                                  bcpf="bcpf",
                                  apex="apex",
-                                 sdk="sdk"):
+                                 sdk="sdk",
+                                 fix=False):
         if fs:
             self.populate_fs(fs)
 
@@ -86,12 +89,14 @@ class TestAnalyzeBcpf(unittest.TestCase):
         module_info = ab.ModuleInfo(modules)
 
         analyzer = ab.BcpfAnalyzer(
+            tool_path=os.path.join(out_dir, "bin"),
             top_dir=top_dir,
             out_dir=out_dir,
             product_out_dir=product_out_dir,
             bcpf=bcpf,
             apex=apex,
             sdk=sdk,
+            fix=fix,
             module_info=module_info,
         )
         analyzer.load_all_flags()
@@ -114,7 +119,7 @@ that traverses multiple lines.
    that should not be reformatted.
 """, reformatted)
 
-    def test_build_flags(self):
+    def do_test_build_flags(self, fix):
         lines = """
 ERROR: Hidden API flags are inconsistent:
 < out/soong/.intermediates/bcpf-dir/bcpf-dir/filtered-flags.csv
@@ -169,7 +174,7 @@ Lacme/test/Other;->getThing()Z,blocked
 """,
         }
 
-        analyzer = self.create_analyzer_for_test(fs)
+        analyzer = self.create_analyzer_for_test(fs, fix=fix)
 
         # Override the build_file_read_output() method to just return a fake
         # build operation.
@@ -214,6 +219,11 @@ Lacme/test/Other;->getThing()Z,blocked
             result.property_changes,
             msg="property changes")
 
+        return result
+
+    def test_build_flags_report(self):
+        result = self.do_test_build_flags(fix=False)
+
         expected_file_changes = [
             ab.FileChange(
                 path="bcpf-dir/hiddenapi/"
@@ -268,6 +278,86 @@ Lacme/test/Other;->getThing()Z,blocked
         self.assertEqual(
             expected_file_changes, result.file_changes, msg="file_changes")
 
+    def test_build_flags_fix(self):
+        result = self.do_test_build_flags(fix=True)
+
+        expected_file_changes = [
+            ab.FileChange(
+                path="bcpf-dir/hiddenapi/"
+                "hiddenapi-max-target-o-low-priority.txt",
+                description="Created with 'bcpf' specific entries"),
+            ab.FileChange(
+                path="bcpf-dir/hiddenapi/hiddenapi-max-target-p.txt",
+                description="Added 'bcpf' specific entries"),
+            ab.FileChange(
+                path="bcpf-dir/hiddenapi/hiddenapi-max-target-q.txt",
+                description="Created with 'bcpf' specific entries"),
+            ab.FileChange(
+                path="bcpf-dir/hiddenapi/"
+                "hiddenapi-max-target-r-low-priority.txt",
+                description="Created with 'bcpf' specific entries"),
+            ab.FileChange(
+                path=_MAX_TARGET_O,
+                description="Removed 'bcpf' specific entries"),
+            ab.FileChange(
+                path=_MAX_TARGET_P,
+                description="Removed 'bcpf' specific entries"),
+            ab.FileChange(
+                path=_MAX_TARGET_Q,
+                description="Removed 'bcpf' specific entries"),
+            ab.FileChange(
+                path=_MAX_TARGET_R,
+                description="Removed 'bcpf' specific entries")
+        ]
+
+        result.file_changes.sort()
+        self.assertEqual(
+            expected_file_changes, result.file_changes, msg="file_changes")
+
+        expected_file_contents = {
+            "bcpf-dir/hiddenapi/hiddenapi-max-target-o-low-priority.txt":
+                """
+Lacme/test/Class;-><init>()V
+""",
+            "bcpf-dir/hiddenapi/hiddenapi-max-target-p.txt":
+                """
+Lacme/old/Class;->getWidget()Lacme/test/Widget;
+Lacme/test/Other;->getThing()Z
+""",
+            "bcpf-dir/hiddenapi/hiddenapi-max-target-q.txt":
+                """
+Lacme/test/Widget;-><init()V
+""",
+            "bcpf-dir/hiddenapi/hiddenapi-max-target-r-low-priority.txt":
+                """
+Lacme/test/Gadget;->NAME:Ljava/lang/String;
+""",
+            _MAX_TARGET_O:
+                """
+Lacme/items/Magnet;->size:I
+""",
+            _MAX_TARGET_P:
+                """
+Lacme/items/Rocket;->size:I
+""",
+            _MAX_TARGET_Q:
+                """
+Lacme/items/Rock;->size:I
+""",
+            _MAX_TARGET_R:
+                """
+Lacme/items/Lever;->size:I
+""",
+        }
+        for file_change in result.file_changes:
+            path = file_change.path
+            expected_contents = expected_file_contents[path].lstrip()
+            abs_path = os.path.join(self.test_dir, path)
+            with open(abs_path, "r", encoding="utf8") as tio:
+                contents = tio.read()
+                self.assertEqual(
+                    expected_contents, contents, msg=f"{path} contents")
+
 
 class TestHiddenApiPropertyChange(unittest.TestCase):
 
@@ -279,6 +369,20 @@ class TestHiddenApiPropertyChange(unittest.TestCase):
         # Remove the directory after the test
         shutil.rmtree(self.test_dir)
 
+    def check_change_fix(self, change, bpmodify_output, expected):
+        file = os.path.join(self.test_dir, "Android.bp")
+
+        with open(file, "w", encoding="utf8") as tio:
+            tio.write(bpmodify_output.strip("\n"))
+
+        bpmodify_runner = ab.BpModifyRunner(
+            os.path.join(os.path.dirname(sys.argv[0]), "bpmodify"))
+        change.fix_bp_file(file, "bcpf", bpmodify_runner)
+
+        with open(file, "r", encoding="utf8") as tio:
+            contents = tio.read()
+            self.assertEqual(expected.lstrip("\n"), contents)
+
     def check_change_snippet(self, change, expected):
         snippet = change.snippet("        ")
         self.assertEqual(expected, snippet)
@@ -296,6 +400,33 @@ class TestHiddenApiPropertyChange(unittest.TestCase):
         ],
 """)
 
+        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: [
+            "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: [
+            "android.provider",
+        ],
+    },
+}
+""")
+
     def test_change_property_with_value_and_comment(self):
         change = ab.HiddenApiPropertyChange(
             property_name="split_packages",
@@ -313,17 +444,45 @@ class TestHiddenApiPropertyChange(unittest.TestCase):
         ],
 """)
 
-    def test_change_without_value_or_empty_comment(self):
+        self.check_change_fix(
-        change = ab.HiddenApiPropertyChange(
-            property_name="split_packages",
-            values=[],
-            property_comment="Single line comment.",
-        )
-
-        self.check_change_snippet(
             change, """
-        // Single line comment.
+bootclasspath_fragment {
-        split_packages: [],
+    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: [
+            "android.provider",
+        ],
+
+        single_packages: [
+            "android.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: [
+            "android.provider",
+        ],
+
+        single_packages: [
+            "android.system",
+        ],
+
+    },
+}
 """)