Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

delocate-merge does not merge Qt Framework binaries #228

Open
MAKOMO opened this issue Sep 7, 2024 · 5 comments · May be fixed by #236
Open

delocate-merge does not merge Qt Framework binaries #228

MAKOMO opened this issue Sep 7, 2024 · 5 comments · May be fixed by #236
Assignees
@HexDecimal
Labels
bug

Comments

@MAKOMO
Copy link

MAKOMO commented Sep 7, 2024 •
edited by HexDecimal
Loading

Describe the bug
While delocate-merge succeeds to merge PyQt6-Qt6 arm and x86 wheels the resulting wheel contains Qt Frameworks that are not universal2.

To Reproduce

# delocate-merge PyQt6_Qt6-6.7.2-py3-none-macosx_10_14_x86_64.whl PyQt6_Qt6-6.7.2-py3-none-macosx_11_0_arm64.whl -w .
# unzip PyQt6_Qt6-6.7.2-py3-none-macosx_11_0_universal2.whl.zip
# cd PyQt6/Qt6/lib/QtCore.framework/Versions/A
# file QtCore
QtCore: Mach-O 64-bit dynamically linked shared library arm64

Expected behavior
A full universal2 wheel.

Wheels used
If a wheel is involved then consider attaching the original wheel (before being delocated) or linking to the repository where the original wheel can be created.

https://files.pythonhosted.org/packages/7e/9d/517b12a42b0692c909ed348545114dae7d0b4014ef9075e18f6bf48834a1/PyQt6_Qt6-6.7.2-py3-none-macosx_11_0_arm64.whl

https://files.pythonhosted.org/packages/10/38/ba0313442c5e4327d52e6c48d2bb4b39099bf1d191bd872edfd8bb1392ef/PyQt6_Qt6-6.7.2-py3-none-macosx_10_14_x86_64.whl

Platform (please complete the following information):

  • OS version: macOS 14.6.1
  • Delocate version: 0.12.0

Additional context

Only files with the extensions

lib_exts=(".so", ".dylib", ".a")

are fused by fuse_trees but this Qt Framework binary has no extension at all.

Patching fuse_trees as follows works for those Qt packages, but is not be general enough to catch other cases.

def fuse_trees(to_tree, from_tree, lib_exts=(".so", ".dylib", ".a")):
...
        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):
                _copyfile(from_path, to_path)
            elif cmp_contents(from_path, to_path):
                pass
            elif ext in lib_exts or fname.startswith('Qt'):
                # existing lib that needs fuse
                lipo_fuse(from_path, to_path, to_path)
...
@MAKOMO MAKOMO added the bug label Sep 7, 2024
@HexDecimal
Copy link
Collaborator

The lib_exts parameter isn't even used by the caller. This could be replaced with a more reliable filter than the file suffix.

@HexDecimal
Copy link
Collaborator

When I check to see what the main analysis function uses, _is_macho_file shows up again. Could probably use that here.

@justvanrossum
Copy link

I can confirm that using _is_macho_file() fixes the problem.

@HexDecimal HexDecimal self-assigned this Oct 15, 2024
@Pantsworth
Copy link

Just wanted to +1 this and say I also ran into this issue when attempting to create a universal2 wheel for ruff from its x86_64 and arm64 wheels.

ruff copies a file named "ruff" to the bin directory when installed, and the current filter on file extensions doesn't catch it to create a fat binary. When the universal2 wheel is generated, I end up with an x86_64-only version of ruff.

I can confirm that using _is_macho_file() in delocate.fuse.fuse_trees fixes this for ruff as well --

Ex.

def fuse_trees(to_tree, from_tree, lib_exts=(".so", ".dylib", ".a")):
...
        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):
                _copyfile(from_path, to_path)
            elif cmp_contents(from_path, to_path):
                pass
            elif ext in lib_exts or _is_macho_file(from_path):   # added _is_macho_file() check
                # existing lib that needs fuse
                lipo_fuse(from_path, to_path, to_path)
...

Let me know if I can provide any other info :)

@HexDecimal HexDecimal linked a pull request Jan 3, 2025 that will close this issue
Open
5 tasks
@HexDecimal
Copy link
Collaborator

Testing shows _is_macho_file having false negatives with some test files (liba.dylib). I have PR #236 which seems to work but might need review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@HexDecimal HexDecimal

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Smarter detection of binary files during delocate-merge HexDecimal/delocate
4 participants
@HexDecimal @justvanrossum @Pantsworth @MAKOMO