From 9fbf88b06fcdb2decda76aa6184c828e41d5c6cc Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Mon, 20 Jan 2025 16:18:35 -0800 Subject: [PATCH] Presubmit error for bad delimiters This change introduces a new error for license fields that use any of the following `["/", ";", " and ", " or "]`. I chose to include the offending character/s in the error message because I find it easier to parse error messages that tell me exactly which character is the bad one. Similarly I've included conditions in the reason to handle the plural cases correctly, generating either: `License contains a bad delimiter character ...`, or `License contains bad delimiter characters ...` I realise this means that any downstream rules looking to detect this error will need to check for a common subset, e.g 'bad delimiter character', however I think it's worth it for the improved user experience of receiving the error. I've also anticipated that most of these errors will be due to situations where multiple licenses are offered, and included additional text explaining that only the most permissive of the choices should be included. This will affect 9 dependencies and they need to choose between multiple licenses anyway so it's okay to generate an error and have partybug file bugs. Bug: http://b/374850412 Change-Id: I6eb53a8a3bd541a1801dff133884b719dcdfe04d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6181848 Reviewed-by: Jiewei Qian Reviewed-by: Rachael Newitt Commit-Queue: Jordan Brown --- metadata/fields/custom/license.py | 12 ++++++++++++ metadata/tests/fields_test.py | 11 +++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/metadata/fields/custom/license.py b/metadata/fields/custom/license.py index f70b2d21d..5661e92af 100644 --- a/metadata/fields/custom/license.py +++ b/metadata/fields/custom/license.py @@ -11,6 +11,9 @@ from typing import List, Tuple, Optional _THIS_DIR = os.path.abspath(os.path.dirname(__file__)) # The repo's root directory. _ROOT_DIR = os.path.abspath(os.path.join(_THIS_DIR, "..", "..", "..")) +# Bad delimiter characters. +BAD_DELIMITERS = ["/", ";", " and ", " or "] +BAD_DELIMITERS_REGEX = re.compile("|".join(re.escape(delimiter) for delimiter in BAD_DELIMITERS)) # Add the repo's root directory for clearer imports. sys.path.insert(0, _ROOT_DIR) @@ -92,6 +95,15 @@ class LicenseField(field_types.SingleLineTextField): if util.is_empty(license): return vr.ValidationError( reason=f"{self._name} has an empty value.") + if BAD_DELIMITERS_REGEX.search(license): + return vr.ValidationError( + reason=f"{self._name} contains a bad license separator. " + "Separate licenses by commas only.", + # Try and preemptively address the root cause of this behaviour, + # which is having multiple choices for a license. + additional=[f"When given a choice of licenses, chose the most " + "permissive one, do not list all options."] + ) if not allowed: not_allowlisted.append(license) diff --git a/metadata/tests/fields_test.py b/metadata/tests/fields_test.py index 78a8ff323..30f449309 100644 --- a/metadata/tests/fields_test.py +++ b/metadata/tests/fields_test.py @@ -123,10 +123,17 @@ class FieldValidationTest(unittest.TestCase): "APSL-2.0, MIT", "APSL-2.0 ,MIT", ], - error_values=["", "\n", ",", "Apache 2.0 ,"], + error_values=[ + "", + "\n", + ",", + "Apache 2.0 ,", + "Custom / MIT", + "Apache-2.0 and MIT", + "Apache-2.0; MIT; BSD-2-Clause", + ], warning_values=[ "Custom license", - "Custom / MIT", "Custom, MIT", ], )