matthew-brett / delocate

Find and copy needed dynamic libraries into python wheels
BSD 2-Clause "Simplified" License
266 stars 58 forks source link

Running delocate on an already delocated wheel results in file modifications but no changes to the RECORD. #156

Closed TansyArron closed 2 years ago

TansyArron commented 2 years ago

Describe the bug If delocate has been run more than once on a wheel, installing that wheel will fail with a bad hash for file error.

To Reproduce

  1. grab a wheel:
    curl -L0 'https://files.pythonhosted.org/packages/d8/bf/fcbe42bf8b0abaae4fa7de05bcc5942a8c1e526200150e330de773e249c8/scipy-1.8.0-cp39-cp39-macosx_10_9_x86_64.whl#sha256=de2e80ee1d925984c2504812a310841c241791c5279352be4707cdcd7c255039' --output scipy-1.8.0-cp39-cp39-macosx_10_9_x86_64.wh

    Check the hash of a file against the hash in RECORD:

    
    unzip -p scipy-1.8.0-cp39-cp39-macosx_10_9_x86_64.wh scipy-1.8.0.dist-info/RECORD | grep scipy/linalg/_flapack.cpython-39-darwin.so
    scipy/linalg/_flapack.cpython-39-darwin.so,sha256=S_Y7-a3d54cos5fjrM5pOsfTwpsGA59efnWUmN7wEdo,2060144

unzip -p scipy-1.8.0-cp39-cp39-macosx_10_9_x86_64.wh scipy/linalg/_flapack.cpython-39-darwin.so | openssl dgst -sha256 -binary | base64 S/Y7+a3d54cos5fjrM5pOsfTwpsGA59efnWUmN7wEdo=


The two hashes match. 

Now run delocate:

.pvenvs/fs3/bin/python3 -m pip install delocate .pvenvs/fs3/bin/delocate-wheel -vvv -k -w /tmp/wheel_debug --require-archs=x86_64 --dylibs-only scipy-1.8.0-cp39-cp39-macosx_10_9_x86_64.wh

In the output here you'll see a lot of `Modifying install name in scipy/stats/_mvn.cpython-39-darwin.so from @loader_path/../.dylibs/libgcc_s.1.dylib to @loader_path/../.dylibs/libgcc_s.1.dylib`

Now check the hashes again:

unzip -p /tmp/wheel_debug/scipy-1.8.0-cp39-cp39-macosx_12_0_universal2.macosx_10_9_x86_64.whl scipy-1.8.0.dist-info/RECORD | grep scipy/linalg/_flapack.cpython-39-darwin.so scipy/linalg/_flapack.cpython-39-darwin.so,sha256=ExagtcdwMg8I4ZanBQXef4psNz78SJPGxVmr0d1q9Dg,3957792

unzip -p /tmp/wheel_debug/scipy-1.8.0-cp39-cp39-macosx_12_0_universal2.macosx_10_9_x86_64.whl scipy/linalg/_flapack.cpython-39-darwin.so | openssl dgst -sha256 -binary | base64 EoUfb6ocV4qu/OpE61bC280FiQzrzSvTxOsQ5NjifNw=

They do not match. 

**Expected behavior**
I would expect running delocate on a previously delocated wheel to be a noop.

**Wheels used**
scipy 1.8.0 right off pypi. I also hit the same error with numpy 1.21.6. 

[scipy wheel](https://files.pythonhosted.org/packages/d8/bf/fcbe42bf8b0abaae4fa7de05bcc5942a8c1e526200150e330de773e249c8/scipy-1.8.0-cp39-cp39-macosx_10_9_x86_64.whl#sha256=de2e80ee1d925984c2504812a310841c241791c5279352be4707cdcd7c255039)

**Platform (please complete the following information):**
 - OS version: macOS 11.6.8
 - Delocate version: 0.10.2

**Additional context**
I think a reasonable fix would look something like this: 
```diff --git a/delocate/delocating.py b/delocate/delocating.py
index 76cd2e2..51fa34d 100644
--- a/delocate/delocating.py
+++ b/delocate/delocating.py
@@ -210,13 +210,20 @@ def _update_install_names(
         for requiring, orig_install_name in lib_dict[required].items():
             req_rel = relpath(required, dirname(requiring))
             new_install_name = "@loader_path/" + req_rel
-            logger.info(
-                "Modifying install name in %s from %s to %s",
-                relpath(requiring, root_path),
-                orig_install_name,
-                new_install_name,
-            )
-            set_install_name(requiring, orig_install_name, new_install_name)
+            if orig_install_name == new_install_name:
+                logger.info(
+                    "NOT modifying install name in %s from %s, as the new name would be the same.",
+                    relpath(requiring, root_path),
+                    orig_install_name,
+                )
+            else:
+                logger.info(
+                    "Modifying install name in %s from %s to %s",
+                    relpath(requiring, root_path),
+                    orig_install_name,
+                    new_install_name,
+                )
+                set_install_name(requiring, orig_install_name, new_install_name)

Another option would be:

index 9bf31ef..76cd2e2 100644
--- a/delocate/delocating.py
+++ b/delocate/delocating.py
@@ -670,8 +670,7 @@ def delocate_wheel(
             libraries=libraries_in_lib_path,
             install_id_prefix=DLC_PREFIX + relpath(lib_sdir, wheel_dir),
         )
-        if len(copied_libs):
-            rewrite_record(wheel_dir)
+        rewrite_record(wheel_dir)
         if len(copied_libs) or not in_place:
             dir2zip(wheel_dir, out_wheel)
     return stripped_lib_dict(copied_libs, wheel_dir + os.path.sep)
HexDecimal commented 2 years ago

Looks like an unintended interaction with copied_libs causing the rewrite_record call to be missed. Delocate re-signed the dylibs assuming they were modified but didn't signal to update the record with copied_libs.

I think I'd go with applying both suggestions.