From e90c30c5e2b6923fc137c0c300046752b3e6df98 Mon Sep 17 00:00:00 2001 From: Kyle Benesch <4b796c65+github@gmail.com> Date: Thu, 16 Jan 2025 07:03:11 -0800 Subject: [PATCH] Smarter detection of binary files during delocate-merge (#236) * Smarter detection of binary files during delocate-merge Updates merging to use `lipo` to detect binary files and removes the previous less reliable behavior. This handles the common case of binary files having no suffix. Reusing old tests instead of trying to keep the old behavior. Fuse wheels tests requires lipo * Only suppress known lipo errors and raise other errors lipo_fuse calls _run which messes up the output strings that needed to be checked. --------- Co-authored-by: Matthew Brett --- Changelog.md | 4 ++ delocate/fuse.py | 97 +++++++++++++++++++++++----------- delocate/tests/test_scripts.py | 2 +- delocate/tools.py | 12 +++-- 4 files changed, 80 insertions(+), 35 deletions(-) diff --git a/Changelog.md b/Changelog.md index d68d7f28..1e56fe72 100644 --- a/Changelog.md +++ b/Changelog.md @@ -14,6 +14,8 @@ rules on making a good Changelog. - `patch_wheel` function raises `FileNotFoundError` instead of `ValueError` on missing patch files. +- `delocate.fuse.fuse_trees` now auto-detects binary files instead of testing + filename suffixes. ### Deprecated @@ -29,6 +31,8 @@ rules on making a good Changelog. - Now checks all architectures instead of an arbitrary default. This was causing inconsistent behavior across MacOS versions. [#230](https://github.com/matthew-brett/delocate/pull/230) +- `delocate-merge` now supports libraries with missing or unusual extensions. + [#228](https://github.com/matthew-brett/delocate/issues/228) ### Removed diff --git a/delocate/fuse.py b/delocate/fuse.py index 7cfecd43..1bcd837e 100644 --- a/delocate/fuse.py +++ b/delocate/fuse.py @@ -15,11 +15,13 @@ from __future__ import annotations import os +import re import shutil +import subprocess import tempfile +import warnings +from collections.abc import Container from os import PathLike -from os.path import exists, relpath, splitext -from os.path import join as pjoin from pathlib import Path from packaging.utils import parse_wheel_filename @@ -29,8 +31,8 @@ chmod_perms, cmp_contents, dir2zip, - lipo_fuse, open_rw, + replace_signature, zip2dir, ) from .wheeltools import rewrite_record @@ -82,18 +84,25 @@ def _retag_wheel(to_wheel: Path, from_wheel: Path, to_tree: Path) -> str: return retag_name +_RE_LIPO_UNKNOWN_FILE_STDERR = re.compile( + r"^fatal error: (?P.+): " + r"can't figure out the architecture type of: (?P.+)\n$" +) + + def fuse_trees( - to_tree: str | PathLike, - from_tree: str | PathLike, - lib_exts=(".so", ".dylib", ".a"), -): + to_tree: str | PathLike[str], + from_tree: str | PathLike[str], + lib_exts: Container[str] | None = None, +) -> None: """Fuse path `from_tree` into path `to_tree`. - For each file in `from_tree` - check for library file extension (in - `lib_exts` - if present, check if there is a file with matching relative - path in `to_tree`, if so, use :func:`delocate.tools.lipo_fuse` to fuse the - two libraries together and write into `to_tree`. If any of these - conditions are not met, just copy the file from `from_tree` to `to_tree`. + Any files in `from_tree` which are not in `to_tree` will be copied over to + `to_tree`. + + Files existing in both `from_tree` and `to_tree` will be parsed. + Binary files on the same path in both directories will be merged using + :func:`delocate.tools.lipo_fuse`. Parameters ---------- @@ -102,32 +111,60 @@ def fuse_trees( from_tree : str or Path-like path of tree to fuse from (update from) lib_exts : sequence, optional - filename extensions for libraries + This parameter is deprecated and should be ignored. + + .. versionchanged:: Unreleased + Binary files are auto-detected instead of using `lib_exts` to test file + suffixes. """ + if lib_exts: + warnings.warn( + "`lib_exts` parameter ignored, will be removed in future.", + FutureWarning, + stacklevel=2, + ) for from_dirpath, dirnames, filenames in os.walk(Path(from_tree)): - to_dirpath = pjoin(to_tree, relpath(from_dirpath, from_tree)) + to_dirpath = Path(to_tree, Path(from_dirpath).relative_to(from_tree)) # Copy any missing directories in to_path - for dirname in tuple(dirnames): - to_path = pjoin(to_dirpath, dirname) - if not exists(to_path): - from_path = pjoin(from_dirpath, dirname) + for dirname in dirnames.copy(): + to_path = Path(to_dirpath, dirname) + if not to_path.exists(): + from_path = Path(from_dirpath, dirname) shutil.copytree(from_path, to_path) # If copying, don't further analyze this directory dirnames.remove(dirname) - for fname in filenames: - root, ext = splitext(fname) - from_path = pjoin(from_dirpath, fname) - to_path = pjoin(to_dirpath, fname) - if not exists(to_path): + for filename in filenames: + file = Path(filename) + from_path = Path(from_dirpath, file) + to_path = Path(to_dirpath, file) + if not to_path.exists(): _copyfile(from_path, to_path) - elif cmp_contents(from_path, to_path): - pass - elif ext in lib_exts: - # existing lib that needs fuse - lipo_fuse(from_path, to_path, to_path) - else: - # existing not-lib file not identical to source + continue + if cmp_contents(from_path, to_path): + continue + try: + # Try to fuse this file using lipo + subprocess.run( + [ + "lipo", + "-create", + from_path, + to_path, + "-output", + to_path, + ], + check=True, + text=True, + capture_output=True, + ) + except subprocess.CalledProcessError as exc: + if not _RE_LIPO_UNKNOWN_FILE_STDERR.match(exc.stderr): + # Unexpected error on library file + raise RuntimeError(exc.stderr) from None + # Existing non-library file not identical to source _copyfile(from_path, to_path) + else: + replace_signature(to_path, "-") def fuse_wheels( diff --git a/delocate/tests/test_scripts.py b/delocate/tests/test_scripts.py index 6062e65e..2ea3b2a7 100644 --- a/delocate/tests/test_scripts.py +++ b/delocate/tests/test_scripts.py @@ -397,7 +397,7 @@ def _fix_break_fix(arch: str) -> None: @pytest.mark.xfail( # type: ignore[misc] - sys.platform == "win32", reason="Can't run scripts." + sys.platform != "darwin", reason="requires lipo" ) def test_fuse_wheels(script_runner: ScriptRunner) -> None: # Some tests for wheel fusing diff --git a/delocate/tools.py b/delocate/tools.py index 943bcc2e..afad217f 100644 --- a/delocate/tools.py +++ b/delocate/tools.py @@ -1183,18 +1183,22 @@ def get_archs(libname: str) -> frozenset[str]: raise ValueError(f"Unexpected output: '{stdout}' for {libname}") +@deprecated("Call lipo directly") def lipo_fuse( - in_fname1: str, in_fname2: str, out_fname: str, ad_hoc_sign: bool = True + in_fname1: str | PathLike[str], + in_fname2: str | PathLike[str], + out_fname: str | PathLike[str], + ad_hoc_sign: bool = True, ) -> str: """Use lipo to merge libs `filename1`, `filename2`, store in `out_fname`. Parameters ---------- - in_fname1 : str + in_fname1 : str or PathLike filename of library - in_fname2 : str + in_fname2 : str or PathLike filename of library - out_fname : str + out_fname : str or PathLike filename to which to write new fused library ad_hoc_sign : {True, False}, optional If True, sign file with ad-hoc signature