Change the delimiter for license validation

Change the delimiter for license field from allowing complex cases using "and", "or", and "/" to only allowing a single comma separated list of licenses that are in use.

When given a choice of licenses OWNERS should choose the most appropriate and list this one. In nearly all cases this should be 'whichever is the least restrictive'.

Corresponding change in documentation: https://crrev.com/c/6068628

Change-Id: Ic30dfacb9ba586137b9493cec878b636107a55f4
Bug: 311097536
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6055313
Reviewed-by: Jordan Brown <rop@google.com>
Commit-Queue: Rachael Newitt <renewitt@google.com>
Auto-Submit: Jordan Brown <rop@google.com>
Reviewed-by: Rachael Newitt <renewitt@google.com>

metadata/fields/custom/license.py

@@ -20,13 +20,6 @@ import metadata.fields.util as util
 import metadata.validation_result as vr
 from metadata.fields.custom.license_allowlist import ALLOWED_SPDX_LICENSES
 
-_PATTERN_VERBOSE_DELIMITER = re.compile(r" and | or | / ")
-
-# Split on the canonical delimiter, or any of the non-canonical delimiters.
-_PATTERN_SPLIT_LICENSE = re.compile("{}|{}".format(
-    _PATTERN_VERBOSE_DELIMITER.pattern,
-    field_types.MetadataField.VALUE_DELIMITER))
-
 
 def process_license_value(value: str,
                           atomic_delimiter: str) -> List[Tuple[str, bool]]:
@@ -50,18 +43,12 @@ def process_license_value(value: str,
         return [(value, True)]
 
     breakdown = []
-    if re.search(_PATTERN_VERBOSE_DELIMITER, value):
+    # Split using the standard value delimiter. This results in
-        # Split using the verbose delimiters.
+    # atomic values; there is no further splitting possible.
-        for component in re.split(_PATTERN_VERBOSE_DELIMITER, value):
+    for atomic_value in value.split(atomic_delimiter):
-            breakdown.extend(
+        atomic_value = atomic_value.strip()
-                process_license_value(component.strip(), atomic_delimiter))
+        breakdown.append(
-    else:
+            (atomic_value, is_license_allowlisted(atomic_value)))
-        # Split using the standard value delimiter. This results in
-        # atomic values; there is no further splitting possible.
-        for atomic_value in value.split(atomic_delimiter):
-            atomic_value = atomic_value.strip()
-            breakdown.append(
-                (atomic_value, is_license_allowlisted(atomic_value)))
 
     return breakdown
 
@@ -74,30 +61,30 @@ def is_license_allowlisted(value: str) -> bool:
 
 
 class LicenseField(field_types.SingleLineTextField):
-    """Custom field for the package's license type(s).
+  """Custom field for the package's license type(s).
 
     e.g. Apache 2.0, MIT, BSD, Public Domain.
     """
-    def __init__(self):
+  def __init__(self):
-        super().__init__(name="License")
+    super().__init__(name="License")
 
-    def validate(self, value: str) -> Optional[vr.ValidationResult]:
+  def validate(self, value: str) -> Optional[vr.ValidationResult]:
-        """Checks the given value consists of recognized license types.
+    """Checks the given value consists of recognized license types.
 
         Note: this field supports multiple values.
         """
-        not_allowlisted = []
+    not_allowlisted = []
-        licenses = process_license_value(value,
+    licenses = process_license_value(value,
                                          atomic_delimiter=self.VALUE_DELIMITER)
-        for license, allowed in licenses:
+    for license, allowed in licenses:
-            if util.is_empty(license):
+      if util.is_empty(license):
-                return vr.ValidationError(
+        return vr.ValidationError(
                     reason=f"{self._name} has an empty value.")
-            if not allowed:
+      if not allowed:
-                not_allowlisted.append(license)
+        not_allowlisted.append(license)
 
-        if not_allowlisted:
+    if not_allowlisted:
-            return vr.ValidationWarning(
+      return vr.ValidationWarning(
                 reason=f"{self._name} has a license not in the allowlist."
                 " (see https://source.chromium.org/chromium/chromium/tools/depot_tools/+/main:metadata/fields/custom/license_allowlist.py).",
                 additional=[
@@ -105,12 +92,12 @@ class LicenseField(field_types.SingleLineTextField):
                     f"{util.quoted(not_allowlisted)}.",
                 ])
 
-        return None
+    return None
 
-    def narrow_type(self, value: str) -> Optional[List[str]]:
+  def narrow_type(self, value: str) -> Optional[List[str]]:
-        if not value:
+    if not value:
-            # Empty License field is equivalent to "not declared".
+      # Empty License field is equivalent to "not declared".
-            return None
+      return None
 
-        parts = _PATTERN_SPLIT_LICENSE.split(value)
+    parts = value.split(self.VALUE_DELIMITER)
-        return list(filter(bool, map(lambda str: str.strip(), parts)))
+    return list(filter(bool, map(lambda str: str.strip(), parts)))

metadata/tests/data/README.chromium.test.multi-invalid

@@ -4,7 +4,7 @@ URL: https://www.example.com/metadata,
      https://www.example.com/parser
 Version: 1.0.12
 Date: 2020-12-03
-License: Apache-2.0 and MIT
+License: Apache-2.0 and MIT and not_an_spdx
 License File: LICENSE
 Security Critical: yes
 Shipped: yes

metadata/tests/data/README.chromium.test.multi-valid

@@ -4,7 +4,7 @@ URL: https://www.example.com/metadata,
      https://www.example.com/parser
 Version: 1.0.12
 Date: 2020-12-03
-License: Apache-2.0 and MIT
+License: Apache-2.0, MIT
 License File: LICENSE
 Security Critical: yes
 Shipped: yes

metadata/tests/fields_test.py

@@ -115,17 +115,19 @@ class FieldValidationTest(unittest.TestCase):
         self._run_field_validation(
             field=known_fields.LICENSE,
             valid_values=[
-                "Apache-2.0 / MIT",
+                "Apache-2.0 , MIT",
                 "Apache-2.0",
                 "BSD-2-Clause",
                 "BSD-2-Clause-FreeBSD",
                 "MIT",
-                "APSL-2.0 and MIT",
+                "APSL-2.0, MIT",
+                "APSL-2.0 ,MIT",
             ],
-            error_values=["", "\n", ",", "Apache 2.0 / MIT / "],
+            error_values=["", "\n", ",", "Apache 2.0 ,"],
             warning_values=[
                 "Custom license",
                 "Custom / MIT",
+                "Custom, MIT",
             ],
         )
 

metadata/tests/parse_test.py

@@ -77,7 +77,7 @@ class ParseTest(unittest.TestCase):
                  "     https://www.example.com/parser"),
                 ("Version", "1.0.12"),
                 ("Date", "2020-12-03"),
-                ("License", "Apache-2.0 and MIT"),
+                ('License', 'Apache-2.0 and MIT and not_an_spdx'),
                 ("License File", "LICENSE"),
                 ("Security Critical", "yes"),
                 ("Shipped", "yes"),

metadata/tests/type_narrowing_test.py

@@ -127,7 +127,7 @@ class FieldValidationTest(unittest.TestCase):
         expect = self._test_on_field(fields.LICENSE)
         expect("", None, "treat empty string as None")
         expect("LICENSE-1", ["LICENSE-1"], "return as a list")
-        expect("LGPL v2 and BSD", ["LGPL v2", "BSD"], "return as a list")
+        expect("LGPL v2, BSD", ["LGPL v2", "BSD"], "return as a list")
 
     def test_license_file(self):
         # TODO(b/321154076): Consider excluding files that doesn't exist on

metadata/tests/validate_test.py

@@ -56,7 +56,7 @@ class ValidateContentTest(unittest.TestCase):
             source_file_dir=_SOURCE_FILE_DIR,
             repo_root_dir=_THIS_DIR,
         )
-        self.assertEqual(len(results), 9)
+        self.assertEqual(len(results), 11)
         error_count = 0
         warning_count = 0
         for result in results:
@@ -64,8 +64,8 @@ class ValidateContentTest(unittest.TestCase):
                 error_count += 1
             else:
                 warning_count += 1
-        self.assertEqual(error_count, 7)
+        self.assertEqual(error_count, 8)
-        self.assertEqual(warning_count, 2)
+        self.assertEqual(warning_count, 3)
 
 
 class ValidateFileTest(unittest.TestCase):
@@ -94,7 +94,7 @@ class ValidateFileTest(unittest.TestCase):
             filepath=_INVALID_METADATA_FILEPATH,
             repo_root_dir=_THIS_DIR,
         )
-        self.assertEqual(len(results), 9)
+        self.assertEqual(len(results), 11)
         error_count = 0
         warning_count = 0
         for result in results:
@@ -102,8 +102,8 @@ class ValidateFileTest(unittest.TestCase):
                 error_count += 1
             else:
                 warning_count += 1
-        self.assertEqual(error_count, 7)
+        self.assertEqual(error_count, 8)
-        self.assertEqual(warning_count, 2)
+        self.assertEqual(warning_count, 3)
 
 
 class CheckFileTest(unittest.TestCase):
@@ -141,7 +141,7 @@ class CheckFileTest(unittest.TestCase):
         # self.assertEqual(len(errors), 7)
         # self.assertEqual(len(warnings), 2)
         self.assertEqual(len(errors), 0)
-        self.assertEqual(len(warnings), 9)
+        self.assertEqual(len(warnings), 11)
 
 
 class ValidationResultTest(unittest.TestCase):