Browse Source

devutils/validate_patches.py: Add --dry-run to verbose error

Fixes #957
Eloston 4 years ago
parent
commit
7ccefb672e

+ 1 - 0
.cirrus.yml

@@ -16,6 +16,7 @@ code_check_task:
     devutils_script:
         - python3 -m yapf --style '.style.yapf' -e '*/third_party/*' -rpd devutils
         - ./devutils/run_devutils_pylint.py --hide-fixme
+        - ./devutils/run_devutils_tests.sh
 
 validate_config_task:
     validate_config_script: ./devutils/validate_config.py

+ 1 - 1
.cirrus_Dockerfile

@@ -2,4 +2,4 @@
 
 FROM python:3.7-slim
 
-RUN apt update && apt install -y xz-utils
+RUN apt update && apt install -y xz-utils patch

+ 4 - 1
devutils/pytest.ini

@@ -4,4 +4,7 @@ testpaths = tests
 #	error
 #	ignore::DeprecationWarning
 #addopts = --cov-report term-missing --hypothesis-show-statistics -p no:warnings
-addopts = --cov=. --cov-config=.coveragerc --cov-report term-missing -p no:warnings
+# Live logging
+#log_cli=true
+#log_level=DEBUG
+addopts = -capture=all --cov=. --cov-config=.coveragerc --cov-report term-missing -p no:warnings

+ 1 - 2
devutils/tests/test_check_patch_files.py

@@ -1,9 +1,8 @@
 # -*- coding: UTF-8 -*-
 
-# Copyright (c) 2019 The ungoogled-chromium Authors. All rights reserved.
+# Copyright (c) 2020 The ungoogled-chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
-
 """Test check_patch_files.py"""
 
 import tempfile

+ 72 - 0
devutils/tests/test_validate_patches.py

@@ -0,0 +1,72 @@
+# -*- coding: UTF-8 -*-
+
+# Copyright (c) 2020 The ungoogled-chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+"""Test validate_patches.py"""
+
+import logging
+import tempfile
+import sys
+from pathlib import Path
+
+sys.path.insert(0, str(Path(__file__).resolve().parent.parent.parent / 'utils'))
+from _common import LOGGER_NAME
+sys.path.pop(0)
+
+from .. import validate_patches
+
+
+def test_test_patches(caplog):
+    """Test _dry_check_patched_file"""
+
+    #pylint: disable=protected-access
+    caplog.set_level(logging.DEBUG, logger=LOGGER_NAME)
+    #set_logging_level(logging.DEBUG)
+
+    orig_file_content = """bye world"""
+    series_iter = ['test.patch']
+
+    def _run_test_patches(patch_content):
+        with tempfile.TemporaryDirectory() as tmpdirname:
+            Path(tmpdirname, 'foobar.txt').write_text(orig_file_content)
+            Path(tmpdirname, 'test.patch').write_text(patch_content)
+            _, patch_cache = validate_patches._load_all_patches(series_iter, Path(tmpdirname))
+            required_files = validate_patches._get_required_files(patch_cache)
+            files_under_test = validate_patches._retrieve_local_files(required_files,
+                                                                      Path(tmpdirname))
+            return validate_patches._test_patches(series_iter, patch_cache, files_under_test)
+
+    # Check valid modification
+    patch_content = """--- a/foobar.txt
++++ b/foobar.txt
+@@ -1 +1 @@
+-bye world
++hello world
+"""
+    assert not _run_test_patches(patch_content)
+
+    # Check invalid modification
+    patch_content = """--- a/foobar.txt
++++ b/foobar.txt
+@@ -1 +1 @@
+-hello world
++olleh world
+"""
+    assert _run_test_patches(patch_content)
+
+    # Check correct removal
+    patch_content = """--- a/foobar.txt
++++ /dev/null
+@@ -1 +0,0 @@
+-bye world
+"""
+    assert not _run_test_patches(patch_content)
+
+    # Check incorrect removal
+    patch_content = """--- a/foobar.txt
++++ /dev/null
+@@ -1 +0,0 @@
+-this line does not exist in foobar
+"""
+    assert _run_test_patches(patch_content)

+ 34 - 1
devutils/validate_patches.py

@@ -1,7 +1,7 @@
 #!/usr/bin/env python3
 # -*- coding: utf-8 -*-
 
-# Copyright (c) 2019 The ungoogled-chromium Authors. All rights reserved.
+# Copyright (c) 2020 The ungoogled-chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 """
@@ -15,7 +15,9 @@ import ast
 import base64
 import email.utils
 import json
+import logging
 import sys
+import tempfile
 from pathlib import Path
 
 sys.path.insert(0, str(Path(__file__).resolve().parent / 'third_party'))
@@ -26,6 +28,7 @@ sys.path.pop(0)
 sys.path.insert(0, str(Path(__file__).resolve().parent.parent / 'utils'))
 from domain_substitution import TREE_ENCODINGS
 from _common import ENCODING, get_logger, get_chromium_version, parse_series, add_common_params
+from patches import dry_run_check
 sys.path.pop(0)
 
 try:
@@ -524,12 +527,30 @@ def _apply_file_unidiff(patched_file, files_under_test):
         assert patched_file[0].target_start == 1
         files_under_test[patched_file_path] = [x.value for x in patched_file[0]]
     elif patched_file.is_removed_file:
+        # Remove lines to see if file to be removed matches patch
+        _modify_file_lines(patched_file, files_under_test[patched_file_path])
         files_under_test[patched_file_path] = None
     else: # Patching an existing file
         assert patched_file.is_modified_file
         _modify_file_lines(patched_file, files_under_test[patched_file_path])
 
 
+def _dry_check_patched_file(patched_file, orig_file_content):
+    """Run "patch --dry-check" on a unidiff.PatchedFile for diagnostics"""
+    with tempfile.TemporaryDirectory() as tmpdirname:
+        tmp_dir = Path(tmpdirname)
+        # Write file to patch
+        patched_file_path = tmp_dir / patched_file.path
+        patched_file_path.parent.mkdir(parents=True, exist_ok=True)
+        patched_file_path.write_text(orig_file_content)
+        # Write patch
+        patch_path = tmp_dir / 'broken_file.patch'
+        patch_path.write_text(str(patched_file))
+        # Dry run
+        _, dry_stdout, _ = dry_run_check(patch_path, tmp_dir)
+        return dry_stdout
+
+
 def _test_patches(series_iter, patch_cache, files_under_test):
     """
     Tests the patches specified in the iterable series_iter
@@ -538,12 +559,24 @@ def _test_patches(series_iter, patch_cache, files_under_test):
     """
     for patch_path_str in series_iter:
         for patched_file in patch_cache[patch_path_str]:
+            orig_file_content = None
+            if get_logger().isEnabledFor(logging.DEBUG):
+                orig_file_content = files_under_test.get(Path(patched_file.path))
+                if orig_file_content:
+                    orig_file_content = ' '.join(orig_file_content)
             try:
                 _apply_file_unidiff(patched_file, files_under_test)
             except _PatchValidationError as exc:
                 get_logger().warning('Patch failed validation: %s', patch_path_str)
                 get_logger().debug('Specifically, file "%s" failed validation: %s',
                                    patched_file.path, exc)
+                if get_logger().isEnabledFor(logging.DEBUG):
+                    # _PatchValidationError cannot be thrown when a file is added
+                    assert patched_file.is_modified_file or patched_file.is_removed_file
+                    assert orig_file_content is not None
+                    get_logger().debug(
+                        'Output of "patch --dry-run" for this patch on this file:\n%s',
+                        _dry_check_patched_file(patched_file, orig_file_content))
                 return True
             except: #pylint: disable=bare-except
                 get_logger().warning('Patch failed validation: %s', patch_path_str)

+ 17 - 12
utils/_common.py

@@ -1,6 +1,6 @@
 # -*- coding: UTF-8 -*-
 
-# Copyright (c) 2019 The ungoogled-chromium Authors. All rights reserved.
+# Copyright (c) 2020 The ungoogled-chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 """Common code and constants"""
@@ -16,6 +16,8 @@ ENCODING = 'UTF-8' # For config files and patches
 
 SEVENZIP_USE_REGISTRY = '_use_registry'
 
+LOGGER_NAME = 'ungoogled'
+
 # Public classes
 
 
@@ -39,9 +41,18 @@ class SetLogLevel(argparse.Action): #pylint: disable=too-few-public-methods
 
     def __call__(self, parser, namespace, value, option_string=None):
         if option_string in ('--verbose', '-v'):
-            value = 'DEBUG'
+            value = logging.DEBUG
         elif option_string in ('--quiet', '-q'):
-            value = 'ERROR'
+            value = logging.ERROR
+        else:
+            levels = {
+                'FATAL': logging.FATAL,
+                'ERROR': logging.ERROR,
+                'WARNING': logging.WARNING,
+                'INFO': logging.INFO,
+                'DEBUG': logging.DEBUG
+            }
+            value = levels[value]
         set_logging_level(value)
 
 
@@ -51,7 +62,7 @@ class SetLogLevel(argparse.Action): #pylint: disable=too-few-public-methods
 def get_logger(initial_level=logging.INFO):
     """Gets the named logger"""
 
-    logger = logging.getLogger('ungoogled')
+    logger = logging.getLogger(LOGGER_NAME)
 
     if logger.level == logging.NOTSET:
         logger.setLevel(initial_level)
@@ -71,14 +82,8 @@ def get_logger(initial_level=logging.INFO):
 def set_logging_level(logging_level):
     """Sets logging level of logger and all its handlers"""
 
-    levels = {
-        'FATAL': logging.FATAL,
-        'ERROR': logging.ERROR,
-        'WARNING': logging.WARNING,
-        'INFO': logging.INFO,
-        'DEBUG': logging.DEBUG
-    }
-    logging_level = levels.get(logging_level, 'INFO')
+    if not logging_level:
+        logging_level = logging.INFO
 
     logger = get_logger()
     logger.setLevel(logging_level)

+ 84 - 8
utils/patches.py

@@ -1,12 +1,13 @@
 #!/usr/bin/env python3
 # -*- coding: UTF-8 -*-
 
-# Copyright (c) 2019 The ungoogled-chromium Authors. All rights reserved.
+# Copyright (c) 2020 The ungoogled-chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 """Applies unified diff patches"""
 
 import argparse
+import os
 import shutil
 import subprocess
 from pathlib import Path
@@ -14,6 +15,86 @@ from pathlib import Path
 from _common import get_logger, parse_series, add_common_params
 
 
+def _find_patch_from_env():
+    patch_bin_path = None
+    patch_bin_env = os.environ.get('PATCH_BIN')
+    if patch_bin_env:
+        patch_bin_path = Path(patch_bin_env)
+        if patch_bin_path.exists():
+            get_logger().debug('Found PATCH_BIN with path "%s"', patch_bin_path)
+        else:
+            patch_which = shutil.which(patch_bin_env)
+            if patch_which:
+                get_logger().debug('Found PATCH_BIN for command with path "%s"', patch_which)
+                patch_bin_path = Path(patch_which)
+    else:
+        get_logger().debug('PATCH_BIN env variable is not set')
+    return patch_bin_path
+
+
+def _find_patch_from_which():
+    patch_which = shutil.which('patch')
+    if not patch_which:
+        get_logger().debug('Did not find "patch" in PATH environment variable')
+        return None
+    return Path(patch_which)
+
+
+def find_and_check_patch(patch_bin_path=None):
+    """
+    Find and/or check the patch binary is working. It finds a path to patch in this order:
+
+    1. Use patch_bin_path if it is not None
+    2. See if "PATCH_BIN" environment variable is set
+    3. Do "which patch" to find GNU patch
+
+    Then it does some sanity checks to see if the patch command is valid.
+
+    Returns the path to the patch binary found.
+    """
+    if patch_bin_path is None:
+        patch_bin_path = _find_patch_from_env()
+    if patch_bin_path is None:
+        patch_bin_path = _find_patch_from_which()
+    if not patch_bin_path:
+        raise ValueError('Could not find patch from PATCH_BIN env var or "which patch"')
+
+    if not patch_bin_path.exists():
+        raise ValueError('Could not find the patch binary: {}'.format(patch_bin_path))
+
+    # Ensure patch actually runs
+    cmd = [str(patch_bin_path), '--version']
+    result = subprocess.run(cmd, capture_output=True, text=True)
+    if result.returncode:
+        get_logger().error('"%s" returned non-zero exit code', ' '.join(cmd))
+        get_logger().error('stdout:\n%s', result.stdout)
+        get_logger().error('stderr:\n%s', result.stderr)
+        raise RuntimeError('Got non-zero exit code running "%s"'.format(' '.join(cmd)))
+
+    return patch_bin_path
+
+
+def dry_run_check(patch_path, tree_path, patch_bin_path=None):
+    """
+    Run patch --dry-run on a patch
+
+    tree_path is the pathlib.Path of the source tree to patch
+    patch_path is a pathlib.Path to check
+    reverse is whether the patches should be reversed
+    patch_bin_path is the pathlib.Path of the patch binary, or None to find it automatically
+        See find_and_check_patch() for logic to find "patch"
+
+    Returns the status code, stdout, and stderr of patch --dry-run
+    """
+    cmd = [
+        str(find_and_check_patch(patch_bin_path)), '-p1', '--ignore-whitespace', '-i',
+        str(patch_path), '-d',
+        str(tree_path), '--no-backup-if-mismatch', '--dry-run'
+    ]
+    result = subprocess.run(cmd, capture_output=True, text=True)
+    return result.returncode, result.stdout, result.stderr
+
+
 def apply_patches(patch_path_iter, tree_path, reverse=False, patch_bin_path=None):
     """
     Applies or reverses a list of patches
@@ -22,17 +103,12 @@ def apply_patches(patch_path_iter, tree_path, reverse=False, patch_bin_path=None
     patch_path_iter is a list or tuple of pathlib.Path to patch files to apply
     reverse is whether the patches should be reversed
     patch_bin_path is the pathlib.Path of the patch binary, or None to find it automatically
-        On Windows, this will look for the binary in third_party/git/usr/bin/patch.exe
-        On other platforms, this will search the PATH environment variable for "patch"
+        See find_and_check_patch() for logic to find "patch"
 
     Raises ValueError if the patch binary could not be found.
     """
     patch_paths = list(patch_path_iter)
-    if patch_bin_path is None:
-        windows_patch_bin_path = (tree_path / 'third_party' / 'git' / 'usr' / 'bin' / 'patch.exe')
-        patch_bin_path = Path(shutil.which('patch') or windows_patch_bin_path)
-        if not patch_bin_path.exists():
-            raise ValueError('Could not find the patch binary')
+    patch_bin_path = find_and_check_patch(patch_bin_path=patch_bin_path)
     if reverse:
         patch_paths.reverse()
 

+ 40 - 0
utils/tests/test_patches.py

@@ -0,0 +1,40 @@
+# -*- coding: UTF-8 -*-
+
+# Copyright (c) 2020 The ungoogled-chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+from pathlib import Path
+import os
+import shutil
+
+import pytest
+
+from .. import patches
+
+
+def test_find_and_check_patch():
+    patches.find_and_check_patch()
+
+    with pytest.raises(ValueError):
+        patches.find_and_check_patch(patch_bin_path=Path('/this/should/not/exist'))
+
+    with pytest.raises(RuntimeError):
+        # Use comamnd "false" to return non-zero exit code
+        patches.find_and_check_patch(patch_bin_path=Path('/bin/false'))
+
+
+def test_patch_from_which():
+    # We assume GNU patch is already installed to PATH
+    assert patches._find_patch_from_which()
+
+
+def test_patch_from_env():
+    os.environ['PATCH_BIN'] = 'patch'
+    assert patches._find_patch_from_env() == Path(shutil.which('patch'))
+
+    os.environ['PATCH_BIN'] = shutil.which('patch')
+    assert patches._find_patch_from_env() == Path(shutil.which('patch'))
+
+    del os.environ['PATCH_BIN']
+    assert patches._find_patch_from_env() is None