llvm_tools: Unify patch json representation
At present, I made a mistake where patch_utils expected
certain keys to exist. However, the original patch_manager.py
didn't. This was further broken by the fact that patch_sync.py
would avoid serializing the 'platforms' field if empty, causing
a back and forth between patch_utils.py and patch_sync.
Update the unittests to match and verify we're doing the correct
thing.
BUG=None
TEST=./patch_utils_unittest.py
TEST=./patch_manager_unittest.py
Change-Id: Ib02c9d552848831f395b006de9a28ea4292b82f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3704542
Tested-by: Jordan Abrahams-Whitehead <ajordanr@google.com>
Reviewed-by: George Burgess <gbiv@chromium.org>
Commit-Queue: Jordan Abrahams-Whitehead <ajordanr@google.com>
diff --git a/llvm_tools/patch_manager.py b/llvm_tools/patch_manager.py
index 51a7476..b499d52 100755
--- a/llvm_tools/patch_manager.py
+++ b/llvm_tools/patch_manager.py
@@ -512,6 +512,8 @@
for pe in patch_entries:
test_result = pe.test_apply(llvm_src_dir)
if not test_result:
+ if pe.version_range is None:
+ pe.version_range = {}
pe.version_range['until'] = svn_version
modified_entries.append(pe)
else:
diff --git a/llvm_tools/patch_utils.py b/llvm_tools/patch_utils.py
index 6fd75c3..cdf9f21 100644
--- a/llvm_tools/patch_utils.py
+++ b/llvm_tools/patch_utils.py
@@ -145,10 +145,10 @@
"""Object mapping of an entry of PATCHES.json."""
workdir: Path
"""Storage location for the patches."""
- metadata: Dict[str, Any]
- platforms: List[str]
+ metadata: Optional[Dict[str, Any]]
+ platforms: Optional[List[str]]
rel_patch_path: str
- version_range: Dict[str, Optional[int]]
+ version_range: Optional[Dict[str, Optional[int]]]
_parsed_hunks = None
def __post_init__(self):
@@ -159,33 +159,29 @@
def from_dict(cls, workdir: Path, data: Dict[str, Any]):
"""Instatiate from a dictionary.
- Dictionary must have at least the following keys:
+ Dictionary must have at least the following key:
{
- 'metadata': {
- 'title': '<title>'
- },
- 'platforms': ['<platform>'],
'rel_patch_path': '<relative patch path to workdir>',
- 'version_range': {
- 'from': <int>,
- 'until': <int>,
- },
}
Returns:
A new PatchEntry.
"""
- return cls(workdir, data['metadata'], data['platforms'],
- data['rel_patch_path'], data['version_range'])
+ return cls(workdir, data.get('metadata'), data.get('platforms'),
+ data['rel_patch_path'], data.get('version_range'))
def to_dict(self) -> Dict[str, Any]:
- return {
+ out = {
'metadata': self.metadata,
- 'platforms': self.platforms,
'rel_patch_path': self.rel_patch_path,
'version_range': self.version_range,
}
+ if self.platforms:
+ # To match patch_sync, only serialized when
+ # non-empty and non-null.
+ out['platforms'] = sorted(self.platforms)
+ return out
def parsed_hunks(self) -> Dict[str, List[Hunk]]:
# Minor caching here because IO is slow.
@@ -200,6 +196,8 @@
def can_patch_version(self, svn_version: int) -> bool:
"""Is this patch meant to apply to `svn_version`?"""
# Sometimes the key is there, but it's set to None.
+ if not self.version_range:
+ return True
from_v = self.version_range.get('from') or 0
until_v = self.version_range.get('until')
if until_v is None:
@@ -208,6 +206,8 @@
def is_old(self, svn_version: int) -> bool:
"""Is this patch old compared to `svn_version`?"""
+ if not self.version_range:
+ return False
until_v = self.version_range.get('until')
# Sometimes the key is there, but it's set to None.
if until_v is None:
@@ -245,7 +245,9 @@
return self.apply(root_dir, ['--dry-run'])
def title(self) -> str:
- return self.metadata['title']
+ if not self.metadata:
+ return ''
+ return self.metadata.get('title', '')
def json_to_patch_entries(workdir: Path, json_fd: IO[str]) -> List[PatchEntry]:
diff --git a/llvm_tools/patch_utils_unittest.py b/llvm_tools/patch_utils_unittest.py
index bef5ae5..3a6409b 100755
--- a/llvm_tools/patch_utils_unittest.py
+++ b/llvm_tools/patch_utils_unittest.py
@@ -5,6 +5,7 @@
"""Unit tests for the patch_utils.py file."""
+import io
from pathlib import Path
import tempfile
import unittest
@@ -84,6 +85,33 @@
self.assertTrue(e5.can_patch_version(5))
self.assertFalse(e5.can_patch_version(9))
+ def test_can_parse_from_json(self):
+ """Test that patches be loaded from json."""
+ json = """
+[
+ {
+ "metadata": {},
+ "platforms": [],
+ "rel_patch_path": "cherry/nowhere.patch",
+ "version_range": {}
+ },
+ {
+ "metadata": {},
+ "rel_patch_path": "cherry/somewhere.patch",
+ "version_range": {}
+ },
+ {
+ "rel_patch_path": "where.patch",
+ "version_range": null
+ },
+ {
+ "rel_patch_path": "cherry/anywhere.patch"
+ }
+]
+ """
+ result = pu.json_to_patch_entries(Path(), io.StringIO(json))
+ self.assertEqual(len(result), 4)
+
def test_parsed_hunks(self):
"""Test that we can parse patch file hunks."""
m = mock.mock_open(read_data=_EXAMPLE_PATCH)
@@ -137,7 +165,7 @@
'metadata': {
'title': 'hello world',
},
- 'platforms': [],
+ 'platforms': ['a'],
'rel_patch_path': 'x/y/z',
'version_range': {
'from': 4,
