Fix broken fs_config_generator tests

These tests were failing at TOT. Also add a
python_test_host target so the tests will be
run on CI in the future.

Bug: 203436762
Test: m libc, prebuilts/build-tools/linux-x86/bin/py2-cmd build/make/tools/fs_config/test_fs_config_generator.py
Change-Id: I6dda841023a2f5e76d59360d08626fc1a8842ffe

4 files changed, 120 insertions, 47 deletions

diff --git a/tools/fs_config/Android.bp b/tools/fs_config/Android.bp
index 8891a0a6ec..d57893c18c 100644
--- a/tools/fs_config/Android.bp
+++ b/tools/fs_config/Android.bp
@@ -40,14 +40,44 @@ cc_binary_host {
     cflags: ["-Werror"],
 }
 
+python_binary_host {
+    name: "fs_config_generator",
+    srcs: ["fs_config_generator.py"],
+    version: {
+        py2: {
+            enabled: true,
+        },
+        py3: {
+            enabled: false,
+        },
+    },
+}
+
+python_test_host {
+    name: "test_fs_config_generator",
+    main: "test_fs_config_generator.py",
+    srcs: [
+        "test_fs_config_generator.py",
+        "fs_config_generator.py",
+    ],
+    version: {
+        py2: {
+            enabled: true,
+        },
+        py3: {
+            enabled: false,
+        },
+    },
+}
+
 target_fs_config_gen_filegroup {
     name: "target_fs_config_gen",
 }
 
 genrule {
     name: "oemaids_header_gen",
-    tool_files: ["fs_config_generator.py"],
-    cmd: "$(location fs_config_generator.py) oemaid --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
+    tools: ["fs_config_generator"],
+    cmd: "$(location fs_config_generator) oemaid --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
     srcs: [
         ":target_fs_config_gen",
         ":android_filesystem_config_header",
@@ -67,8 +97,8 @@ cc_library_headers {
 // TARGET_FS_CONFIG_GEN files.
 genrule {
     name: "passwd_gen_system",
-    tool_files: ["fs_config_generator.py"],
-    cmd: "$(location fs_config_generator.py) passwd --partition=system --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
+    tools: ["fs_config_generator"],
+    cmd: "$(location fs_config_generator) passwd --partition=system --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
     srcs: [
         ":target_fs_config_gen",
         ":android_filesystem_config_header",
@@ -84,8 +114,8 @@ prebuilt_etc {
 
 genrule {
     name: "passwd_gen_vendor",
-    tool_files: ["fs_config_generator.py"],
-    cmd: "$(location fs_config_generator.py) passwd --partition=vendor --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
+    tools: ["fs_config_generator"],
+    cmd: "$(location fs_config_generator) passwd --partition=vendor --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
     srcs: [
         ":target_fs_config_gen",
         ":android_filesystem_config_header",
@@ -102,8 +132,8 @@ prebuilt_etc {
 
 genrule {
     name: "passwd_gen_odm",
-    tool_files: ["fs_config_generator.py"],
-    cmd: "$(location fs_config_generator.py) passwd --partition=odm --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
+    tools: ["fs_config_generator"],
+    cmd: "$(location fs_config_generator) passwd --partition=odm --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
     srcs: [
         ":target_fs_config_gen",
         ":android_filesystem_config_header",
@@ -120,8 +150,8 @@ prebuilt_etc {
 
 genrule {
     name: "passwd_gen_product",
-    tool_files: ["fs_config_generator.py"],
-    cmd: "$(location fs_config_generator.py) passwd --partition=product --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
+    tools: ["fs_config_generator"],
+    cmd: "$(location fs_config_generator) passwd --partition=product --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
     srcs: [
         ":target_fs_config_gen",
         ":android_filesystem_config_header",
@@ -138,8 +168,8 @@ prebuilt_etc {
 
 genrule {
     name: "passwd_gen_system_ext",
-    tool_files: ["fs_config_generator.py"],
-    cmd: "$(location fs_config_generator.py) passwd --partition=system_ext --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
+    tools: ["fs_config_generator"],
+    cmd: "$(location fs_config_generator) passwd --partition=system_ext --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
     srcs: [
         ":target_fs_config_gen",
         ":android_filesystem_config_header",
@@ -159,8 +189,8 @@ prebuilt_etc {
 // TARGET_FS_CONFIG_GEN files.
 genrule {
     name: "group_gen_system",
-    tool_files: ["fs_config_generator.py"],
-    cmd: "$(location fs_config_generator.py) group --partition=system --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
+    tools: ["fs_config_generator"],
+    cmd: "$(location fs_config_generator) group --partition=system --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
     srcs: [
         ":target_fs_config_gen",
         ":android_filesystem_config_header",
@@ -176,8 +206,8 @@ prebuilt_etc {
 
 genrule {
     name: "group_gen_vendor",
-    tool_files: ["fs_config_generator.py"],
-    cmd: "$(location fs_config_generator.py) group --partition=vendor --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
+    tools: ["fs_config_generator"],
+    cmd: "$(location fs_config_generator) group --partition=vendor --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
     srcs: [
         ":target_fs_config_gen",
         ":android_filesystem_config_header",
@@ -194,8 +224,8 @@ prebuilt_etc {
 
 genrule {
     name: "group_gen_odm",
-    tool_files: ["fs_config_generator.py"],
-    cmd: "$(location fs_config_generator.py) group --partition=odm --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
+    tools: ["fs_config_generator"],
+    cmd: "$(location fs_config_generator) group --partition=odm --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
     srcs: [
         ":target_fs_config_gen",
         ":android_filesystem_config_header",
@@ -212,8 +242,8 @@ prebuilt_etc {
 
 genrule {
     name: "group_gen_product",
-    tool_files: ["fs_config_generator.py"],
-    cmd: "$(location fs_config_generator.py) group --partition=product --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
+    tools: ["fs_config_generator"],
+    cmd: "$(location fs_config_generator) group --partition=product --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
     srcs: [
         ":target_fs_config_gen",
         ":android_filesystem_config_header",
@@ -230,8 +260,8 @@ prebuilt_etc {
 
 genrule {
     name: "group_gen_system_ext",
-    tool_files: ["fs_config_generator.py"],
-    cmd: "$(location fs_config_generator.py) group --partition=system_ext --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
+    tools: ["fs_config_generator"],
+    cmd: "$(location fs_config_generator) group --partition=system_ext --aid-header=$(location :android_filesystem_config_header) $(locations :target_fs_config_gen) >$(out)",
     srcs: [
         ":target_fs_config_gen",
         ":android_filesystem_config_header",
diff --git a/tools/fs_config/README.md b/tools/fs_config/README.md
index bad5e10410..62d6d1e4f4 100644
--- a/tools/fs_config/README.md
+++ b/tools/fs_config/README.md
@@ -69,13 +69,13 @@ generated header file.
 
 From within the `fs_config` directory, unit tests can be executed like so:
 
-    $ python -m unittest test_fs_config_generator.Tests
-    .............
+    $ python test_fs_config_generator.py
+    ................
     ----------------------------------------------------------------------
-    Ran 13 tests in 0.004s
-
+    Ran 16 tests in 0.004s
     OK
 
+
 One could also use nose if they would like:
 
     $ nose2
diff --git a/tools/fs_config/fs_config_generator.py b/tools/fs_config/fs_config_generator.py
index 098fde664f..cb1616ad2d 100755
--- a/tools/fs_config/fs_config_generator.py
+++ b/tools/fs_config/fs_config_generator.py
@@ -179,6 +179,10 @@ class AID(object):
             and self.normalized_value == other.normalized_value \
             and self.login_shell == other.login_shell
 
+    def __repr__(self):
+        return "AID { identifier = %s, value = %s, normalized_value = %s, login_shell = %s }" % (
+            self.identifier, self.value, self.normalized_value, self.login_shell)
+
     @staticmethod
     def is_friendly(name):
         """Determines if an AID is a freindly name or C define.
@@ -312,7 +316,7 @@ class AIDHeaderParser(object):
     ]
     _AID_DEFINE = re.compile(r'\s*#define\s+%s.*' % AID.PREFIX)
     _RESERVED_RANGE = re.compile(
-        r'#define AID_(.+)_RESERVED_\d*_*(START|END)\s+(\d+)')
+        r'#define AID_(.+)_RESERVED_(?:(\d+)_)?(START|END)\s+(\d+)')
 
     # AID lines cannot end with _START or _END, ie AID_FOO is OK
     # but AID_FOO_START is skiped. Note that AID_FOOSTART is NOT skipped.
@@ -345,6 +349,7 @@ class AIDHeaderParser(object):
             aid_file (file): The open AID header file to parse.
         """
 
+        ranges_by_name = {}
         for lineno, line in enumerate(aid_file):
 
             def error_message(msg):
@@ -355,20 +360,24 @@ class AIDHeaderParser(object):
 
             range_match = self._RESERVED_RANGE.match(line)
             if range_match:
-                partition = range_match.group(1).lower()
-                value = int(range_match.group(3), 0)
+                partition, name, start, value = range_match.groups()
+                partition = partition.lower()
+                if name is None:
+                    name = "unnamed"
+                start = start == "START"
+                value = int(value, 0)
 
                 if partition == 'oem':
                     partition = 'vendor'
 
-                if partition in self._ranges:
-                    if isinstance(self._ranges[partition][-1], int):
-                        self._ranges[partition][-1] = (
-                            self._ranges[partition][-1], value)
-                    else:
-                        self._ranges[partition].append(value)
-                else:
-                    self._ranges[partition] = [value]
+                if partition not in ranges_by_name:
+                    ranges_by_name[partition] = {}
+                if name not in ranges_by_name[partition]:
+                    ranges_by_name[partition][name] = [None, None]
+                if ranges_by_name[partition][name][0 if start else 1] is not None:
+                    sys.exit(error_message("{} of range {} of partition {} was already defined".format(
+                        "Start" if start else "End", name, partition)))
+                ranges_by_name[partition][name][0 if start else 1] = value
 
             if AIDHeaderParser._AID_DEFINE.match(line):
                 chunks = line.split()
@@ -390,6 +399,21 @@ class AIDHeaderParser(object):
                         error_message('{} for "{}"'.format(
                             exception, identifier)))
 
+        for partition in ranges_by_name:
+            for name in ranges_by_name[partition]:
+                start = ranges_by_name[partition][name][0]
+                end = ranges_by_name[partition][name][1]
+                if start is None:
+                    sys.exit("Range '%s' for partition '%s' had undefined start" % (name, partition))
+                if end is None:
+                    sys.exit("Range '%s' for partition '%s' had undefined end" % (name, partition))
+                if start > end:
+                    sys.exit("Range '%s' for partition '%s' had start after end. Start: %d, end: %d" % (name, partition, start, end))
+
+                if partition not in self._ranges:
+                    self._ranges[partition] = []
+                self._ranges[partition].append((start, end))
+
     def _handle_aid(self, identifier, value):
         """Handle an AID C #define.
 
diff --git a/tools/fs_config/test_fs_config_generator.py b/tools/fs_config/test_fs_config_generator.py
index b7f173eb92..76ed8f433c 100755
--- a/tools/fs_config/test_fs_config_generator.py
+++ b/tools/fs_config/test_fs_config_generator.py
@@ -78,11 +78,11 @@ class Tests(unittest.TestCase):
             temp_file.flush()
 
             parser = AIDHeaderParser(temp_file.name)
-            oem_ranges = parser.oem_ranges
+            ranges = parser.ranges
             aids = parser.aids
 
-            self.assertTrue((2900, 2999) in oem_ranges)
-            self.assertFalse((5000, 6000) in oem_ranges)
+            self.assertTrue((2900, 2999) in ranges["vendor"])
+            self.assertFalse((5000, 6000) in ranges["vendor"])
 
             for aid in aids:
                 self.assertTrue(aid.normalized_value in ['1000', '1001'])
@@ -105,11 +105,11 @@ class Tests(unittest.TestCase):
             temp_file.flush()
 
             parser = AIDHeaderParser(temp_file.name)
-            oem_ranges = parser.oem_ranges
+            ranges = parser.ranges
             aids = parser.aids
 
-            self.assertTrue((2900, 2999) in oem_ranges)
-            self.assertFalse((5000, 6000) in oem_ranges)
+            self.assertTrue((2900, 2999) in ranges["vendor"])
+            self.assertFalse((5000, 6000) in ranges["vendor"])
 
             for aid in aids:
                 self.assertTrue(aid.normalized_value in ['1000', '1001'])
@@ -168,6 +168,22 @@ class Tests(unittest.TestCase):
             with self.assertRaises(SystemExit):
                 AIDHeaderParser(temp_file.name)
 
+    def test_aid_header_parser_bad_oem_range_duplicated(self):
+        """Test AID Header Parser bad oem range (no start) input file"""
+
+        with tempfile.NamedTemporaryFile() as temp_file:
+            temp_file.write(
+                textwrap.dedent("""
+                #define AID_OEM_RESERVED_START 2000
+                #define AID_OEM_RESERVED_END 2900
+                #define AID_OEM_RESERVED_START 3000
+                #define AID_OEM_RESERVED_END 3900
+            """))
+            temp_file.flush()
+
+            with self.assertRaises(SystemExit):
+                AIDHeaderParser(temp_file.name)
+
     def test_aid_header_parser_bad_oem_range_mismatch_start_end(self):
         """Test AID Header Parser bad oem range mismatched input file"""
 
@@ -262,7 +278,7 @@ class Tests(unittest.TestCase):
             """))
             temp_file.flush()
 
-            parser = FSConfigFileParser([temp_file.name], [(5000, 5999)])
+            parser = FSConfigFileParser([temp_file.name], {"oem1": [(5000, 5999)]})
             files = parser.files
             dirs = parser.dirs
             aids = parser.aids
@@ -284,7 +300,7 @@ class Tests(unittest.TestCase):
                              FSConfig('0777', 'AID_FOO', 'AID_SYSTEM', '0',
                                       '/vendor/path/dir/', temp_file.name))
 
-            self.assertEqual(aid, AID('AID_OEM1', '0x1389', temp_file.name, '/vendor/bin/sh'))
+            self.assertEqual(aid, AID('AID_OEM1', '0x1389', temp_file.name, '/bin/sh'))
 
     def test_fs_config_file_parser_bad(self):
         """Test FSConfig Parser bad input file"""
@@ -298,7 +314,7 @@ class Tests(unittest.TestCase):
             temp_file.flush()
 
             with self.assertRaises(SystemExit):
-                FSConfigFileParser([temp_file.name], [(5000, 5999)])
+                FSConfigFileParser([temp_file.name], {})
 
     def test_fs_config_file_parser_bad_aid_range(self):
         """Test FSConfig Parser bad aid range value input file"""
@@ -312,4 +328,7 @@ class Tests(unittest.TestCase):
             temp_file.flush()
 
             with self.assertRaises(SystemExit):
-                FSConfigFileParser([temp_file.name], [(5000, 5999)])
+                FSConfigFileParser([temp_file.name], {"oem1": [(5000, 5999)]})
+
+if __name__ == "__main__":
+    unittest.main()