[GR-23618] Improve packages patching: support separate ginstall/pip patches and versioning of the pip patches.

PullRequest: graalpython/976

Author: steve-s

docs/contributor/IMPLEMENTATION_DETAILS.md

@@ -144,3 +144,58 @@ as needed, we use the same mechanism to also pass the currently handled
 exception. Unlike CPython we do not use a stack of currently handled exceptions,
 instead we utilize the call stack of Java by always passing the current exception
 and holding on to the last (if any) in a local variable.
+
+## Patching of Packages
+
+Some PyPI packages contain code that is not compatible with GraalPython.
+To overcome this limitation and support such packages, GraalPython contains
+patches for some popular packages. The patches are applied to packages
+installed via GraalPython specific utility `ginstall` and also to packages
+installed via `pip`. This is achieved by patching `pip` code during the loading
+of the `pip` module in `pip_hook.py`.
+
+The patches are regular POSIX `patch` command compatible diffs located
+in `lib-graalpython/patches`. The directory structure is following:
+
+```
+patches
+├── atomicwrites
+│   └── sdist
+│       ├── atomicwrites.patch
+│       └── atomicwrites-5.3.1.patch
+└── pytest
+    └── whl
+        ├── pytest.dir
+        ├── pytest.patch
+        ├── pytest-5.dir
+        ├── pytest-5.patch
+        ├── pytest-5.2.dir
+        ├── pytest-5.2.patch
+        ├── pytest-5.2.3.dir
+        └── pytest-5.2.3.patch
+```
+
+The directory names are names of the Python packages.
+
+Patches in the `sdist` subdirectory apply to the source distribution tarballs,
+i.e., such patches may patch, for example, the `setup.py` file.
+
+Patches in the `whl` subdirectory apply to both the wheels binary distributions
+and to appropriate subdirectory of the source distributions. For example, in `pytest`
+the Python source files that are deployed into the top-level in the wheels binary
+archive are located in the `src` subdirectory inside the source distribution. To reuse
+one patch for both binary and source distributions, we need to also record the
+subdirectory where the sources live in the source distribution - path to that directory
+(relative to the root of the source distribution) is inside the `*.dir` files.
+
+All the `*.patch` and `*.dir` files may contain version string. The search for
+an appropriate `*.patch` file for a package `P` of version `X.Y.Z` happens in
+this order:
+
+* `P-X.Y.Z.patch`
+* `P-X.Y.patch`
+* `P-X.patch`
+* `P.patch`
+
+The same applies to `*.dir` files, and the search is independent of the search
+for the `*.patch` file, i.e., multiple versions of `*.patch` may share one `*.dir`.

graalpython/lib-graalpython/modules/ginstall.py

@@ -36,7 +36,7 @@
 # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 # SOFTWARE.
-
+import re
 from pathlib import Path
 import argparse
 import json
@@ -46,8 +46,8 @@
 import subprocess
 import tempfile
 import importlib
-import sys
 
+import sys
 
 WARNING = '\033[93m'
 FAIL = '\033[91m'
@@ -365,7 +365,7 @@ def xit(msg, status=-1):
     exit(-1)
 
 
-def _install_from_url(url, package, extra_opts=[], add_cflags="", ignore_errors=False, env={}):
+def _install_from_url(url, package, extra_opts=[], add_cflags="", ignore_errors=False, env={}, version=None):
     name = url[url.rfind("/")+1:]
     tempdir = tempfile.mkdtemp()
 
@@ -407,11 +407,25 @@ def _install_from_url(url, package, extra_opts=[], add_cflags="", ignore_errors=
     else:
         xit("Unknown file type: %s" % name)
 
-    patch_file_path = get_patch(package)
+    file_realpath = os.path.dirname(os.path.realpath(__file__))
+    patches_dir = os.path.join(Path(file_realpath).parent, 'patches', package)
+    # empty match group to have the same groups range as in pip_hook
+    # unlike with pip, the version number may not be available at all
+    versions = re.search("()(\\d+)?(.\\d+)?(.\\d+)?", "" if version is None else version)
+
+    patch_file_path = first_existing(package, versions, os.path.join(patches_dir, "sdist"), ".patch")
     if patch_file_path:
         system("patch -d %s/%s/ -p1 < %s" %
                (tempdir, bare_name, patch_file_path))
 
+    whl_patches_dir = os.path.join(patches_dir, "whl")
+    patch_file_path = first_existing(package, versions, whl_patches_dir, ".patch")
+    subdir = read_first_existing(package, versions, whl_patches_dir, ".dir")
+    subdir = "" if subdir is None else "/" + subdir
+    if patch_file_path:
+        system("patch -d %s/%s%s -p1 < %s" %
+               (tempdir, bare_name, subdir, patch_file_path))
+
     if "--prefix" not in extra_opts and site.ENABLE_USER_SITE:
         user_arg = "--user"
     else:
@@ -420,19 +434,41 @@ def _install_from_url(url, package, extra_opts=[], add_cflags="", ignore_errors=
     if status != 0 and not ignore_errors:
         xit("An error occurred trying to run `setup.py install %s %s'" % (user_arg, " ".join(extra_opts)))
 
-def get_patch(package):
-    dir_path = os.path.dirname(os.path.realpath(__file__))
-    parent_folder = Path(dir_path).parent
-    patch_file_path = os.path.join(
-        parent_folder, 'patches', "%s.patch" % package
-    )
-    if os.path.exists(patch_file_path):
-        return patch_file_path
+# NOTE: Following 3 functions are duplicated in pip_hook.py:
+# creates a search list of a versioned file:
+# {name}-X.Y.Z.{suffix}, {name}-X.Y.{suffix}, {name}-X.{suffix}, {name}.{suffix}
+# 'versions' is a result of re.search
+def list_versioned(pkg_name, versions, dir, suffix):
+    acc = ""
+    res = []
+    for i in range(2,5):
+        v = versions.group(i)
+        if v is not None:
+            acc = acc + v
+            res.append(acc)
+    res.reverse()
+    res = [os.path.join(dir, pkg_name + "-" + ver + suffix) for ver in res]
+    res.append(os.path.join(dir, pkg_name + suffix))
+    return res
+
+def first_existing(pkg_name, versions, dir, suffix):
+    for filename in list_versioned(pkg_name, versions, dir, suffix):
+        if os.path.exists(filename):
+            return filename
+
+def read_first_existing(pkg_name, versions, dir, suffix):
+    filename = first_existing(pkg_name, versions, dir, suffix)
+    if filename:
+        with open(filename, "r") as f:
+            return f.read()
+
+# end of code duplicated in pip_hook.py
 
 def install_from_pypi(package, extra_opts=[], add_cflags="", ignore_errors=True, env={}):
     package_pattern = os.environ.get("GINSTALL_PACKAGE_PATTERN", "https://pypi.org/pypi/%s/json")
     package_version_pattern = os.environ.get("GINSTALL_PACKAGE_VERSION_PATTERN", "https://pypi.org/pypi/%s/%s/json")
 
+    version = None
     if "==" in package:
         package, _, version = package.rpartition("==")
         url = package_version_pattern % (package, version)
@@ -456,7 +492,8 @@ def install_from_pypi(package, extra_opts=[], add_cflags="", ignore_errors=True,
                     break
 
     if url:
-        _install_from_url(url, package=package, extra_opts=extra_opts, add_cflags=add_cflags, ignore_errors=ignore_errors, env=env)
+        _install_from_url(url, package=package, extra_opts=extra_opts, add_cflags=add_cflags,
+                          ignore_errors=ignore_errors, env=env, version=version)
     else:
         xit("Package not found: '%s'" % package)
 

graalpython/lib-graalpython/patches/atomicwrites.patch renamed to graalpython/lib-graalpython/patches/atomicwrites/sdist/atomicwrites.patch

@@ -0,0 +1 @@
+src

graalpython/lib-graalpython/patches/attrs.patch renamed to graalpython/lib-graalpython/patches/attrs/sdist/attrs.patch

@@ -1,9 +1,8 @@
-
-diff --git a/src/_pytest/_code/code.py b/src/_pytest/_code/code.py
-index aa4dcff..029e659 100644
---- a/src/_pytest/_code/code.py
-+++ b/src/_pytest/_code/code.py
-@@ -211,15 +211,17 @@ class TracebackEntry:
+diff --git a/_pytest/_code/code.py b/_pytest/_code/code.py
+index 7d72234e7..1f0ca7f7e 100644
+--- a/_pytest/_code/code.py
++++ b/_pytest/_code/code.py
+@@ -222,15 +222,17 @@ class TracebackEntry:
              if key is not None:
                  astnode = astcache.get(key, None)
          start = self.getfirstlinesource()
@@ -30,11 +29,24 @@ index aa4dcff..029e659 100644
          return source[start:end]
 
      source = property(getsource)
-diff --git a/src/_pytest/python.py b/src/_pytest/python.py
-index 66d8530..8bb2ab6 100644
---- a/src/_pytest/python.py
-+++ b/src/_pytest/python.py
-@@ -494,8 +494,10 @@ class Module(nodes.File, PyCollector):
+diff --git a/_pytest/assertion/__init__.py b/_pytest/assertion/__init__.py
+index 126929b6a..8ece37d23 100644
+--- a/_pytest/assertion/__init__.py
++++ b/_pytest/assertion/__init__.py
+@@ -15,7 +15,7 @@ def pytest_addoption(parser):
+         action="store",
+         dest="assertmode",
+         choices=("rewrite", "plain"),
+-        default="rewrite",
++        default="plain",
+         metavar="MODE",
+         help="""Control assertion debugging tools.  'plain'
+                             performs no assertion debugging.  'rewrite'
+diff --git a/_pytest/python.py b/_pytest/python.py
+index 913a93bc0..0b6d75dd8 100644
+--- a/_pytest/python.py
++++ b/_pytest/python.py
+@@ -497,8 +497,10 @@ class Module(nodes.File, PyCollector):
      def _importtestmodule(self):
          # we assume we are only called once per module
          importmode = self.config.getoption("--import-mode")
@@ -45,7 +57,7 @@ index 66d8530..8bb2ab6 100644
          except SyntaxError:
              raise self.CollectError(
                  _pytest._code.ExceptionInfo.from_current().getrepr(style="short")
-@@ -538,6 +540,10 @@ class Module(nodes.File, PyCollector):
+@@ -541,6 +543,10 @@ class Module(nodes.File, PyCollector):
                  "or @pytest.mark.skipif decorators instead, and to skip a "
                  "module use `pytestmark = pytest.mark.{skip,skipif}."
              )
@@ -56,4 +68,3 @@ index 66d8530..8bb2ab6 100644
          self.config.pluginmanager.consider_module(mod)
          return mod
 
-