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.

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

Fix msys2 dll paths in GdkPixbuf loaders.cache gen #7842

Merged

Conversation

coolcatco888
Copy link
Contributor

@coolcatco888 coolcatco888 commented Aug 10, 2023

When compiling a program that depends on GTK and GdkPixbuf loaders in msys2 Windows, the loaders.cache generates dlls with an absolute path that is local to the machine it was generated on.

When the resulting compiled program is run on another Windows machine, the local paths of the dlls do not exist and the program crashes.

Example loader.cache snippet

"C:/tools/msys64/mingw64/bin/../lib/gdk-pixbuf-2.0/2.10.0/loaders/libpixbufloader-png.dll"
"png" 5 "gdk-pixbuf" "PNG" "LGPL"
"image/png" ""
"png" ""
"\211PNG\r\n\032\n" "" 100

The correct output should be

"lib\\gdk-pixbuf\\loaders\\libpixbufloader-png.dll"
"png" 5 "gdk-pixbuf" "PNG" "LGPL"
"image/png" ""
"png" ""
"\211PNG\r\n\032\n" "" 100

The root cause of the bug lies the libdir variable.
libdir = C:/tools/msys64/mingw64/bin

The GdkPixbuf hook generates a loaders.cache file with absolute paths and uses prefix matching to replace the dll paths with relative paths that the program can find. However, this will match none of the prefixes and therefore, the absolute path will not be changed to the relative path in the loaders.cache.

In the code example below, we see that it will try to match the following prefixes:
prefix = "C:/tools/msys64/mingw64/bin/gdk-pixbuf-2.0/2.10.0
win_prefix "\\lib\\gdk-pixbuf-2.0\\2.10.0

This will not work given the line from cachedata
"C:/tools/msys64/mingw64/bin/../lib/gdk-pixbuf-2.0/2.10.0/loaders/libpixbufloader-png.dll"

    output_lines = []
    prefix = '"' + os.path.join(libdir, 'gdk-pixbuf-2.0', '2.10.0')
    plen = len(prefix)

    win_prefix = '"' + '\\\\'.join(['lib', 'gdk-pixbuf-2.0', '2.10.0'])
    win_plen = len(win_prefix)

    # For each line in the updated loader cache...
    for line in cachedata.splitlines():
        if line.startswith('#'):
            continue
        if line.startswith(prefix):
            line = '"@executable_path/' + LOADER_CACHE_DEST_PATH + line[plen:]
        elif line.startswith(win_prefix):
            line = '"' + LOADER_CACHE_DEST_PATH.replace('/', '\\\\') + line[win_plen:]
        output_lines.append(line)

This fix addresses this by accounting for the dll prefix for msys2.

My solution will simplify the line path to be:
"C:/tools/msys64/mingw64/lib/gdk-pixbuf-2.0/2.10.0/loaders/libpixbufloader-png.dll"

which will match my new prefix:
msys2_prefix = "C:/tools/msys64/mingw64/lib/gdk-pixbuf-2.0/2.10.0

And convert the line to be:
line = "lib\\gdk-pixbuf\\loaders\\libpixbufloader-png.dll"

fixes #7838

@rokm
Copy link
Member

rokm commented Aug 11, 2023

Hmm, I think long-term, all that prefix matching should be replaced with a bit more advanced loaders.config parser that identifies the lines that contain path to library (seems to be first line in a block, where blocks are separated by empty lines), and processes them using path-utility functions. I.e., strips the quote character, takes the base name, prepends the collection path (anchored at @executable_path for non-Windows), and adds the quotes. Then it won't matter what the original path was.

But for the time being, the proposed fix should do the trick. Please add a news fragment for the changelog, then we should be ready to merge.

@coolcatco888
Copy link
Contributor Author

I added the news fragment. I also noticed that the previous test run failed this test:

tests (3.12-dev, ubuntu-20.04)

Would I still be able to merge?

@coolcatco888
Copy link
Contributor Author

Hi @rokm,

I have noticed that this test fails: tests (3.12-dev, ubuntu-20.04). I don't think the failure is related to my change since I saw a couple of pull requests get merged without this test passing.

Would you be able to merge my change? Do not have permissions to merge this pull request without all checks passing.

@bwoodsend bwoodsend merged commit 55b0d33 into pyinstaller:develop Aug 12, 2023
bwoodsend pushed a commit to bwoodsend/pyinstaller that referenced this pull request Aug 25, 2023
bwoodsend pushed a commit to bwoodsend/pyinstaller that referenced this pull request Aug 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gtk libpixbufloader-png.dll load path is not relative - exe will crash if run on another machine
3 participants