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

Hooks: Include OpenGL fallback DLLs for PyQt5. Fixes #3458. #3568

Merged
merged 1 commit into from Jun 15, 2018

Conversation

Projects
None yet
4 participants
@Siecje
Contributor

Siecje commented Jun 11, 2018

No description provided.

@Siecje Siecje force-pushed the Siecje:3458 branch 3 times, most recently from 9350182 to f060837 Jun 11, 2018

@Siecje Siecje force-pushed the Siecje:3458 branch 3 times, most recently from 4f74da1 to ec12915 Jun 12, 2018

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 13, 2018

Nice! Would you do some refactoring? Define a function like qt_include_all_or_none in hook-pyqt5.py that would include all the provided files of all exist, or include none if not all exist. Then apply this to the various groups -- ICU files, software renderer, ANGLE renderer.

@Siecje Siecje force-pushed the Siecje:3458 branch 3 times, most recently from bdba381 to 4e41947 Jun 13, 2018

@Siecje

This comment has been minimized.

Contributor

Siecje commented Jun 14, 2018

@bjones1 Done.

@htgoebel

This comment has been minimized.

Member

htgoebel commented Jun 14, 2018

bjones1 is in charge of the code, so just a remark on the commit message:

  • "Fallback" shall for lower-case"
  • "Fixes issue #3458" shall be just "Fixes #3458" and be on the third line (2nd line is empty)

Thanks

@htgoebel htgoebel requested review from htgoebel and removed request for htgoebel Jun 14, 2018

@Siecje Siecje force-pushed the Siecje:3458 branch from 4e41947 to c6731d5 Jun 14, 2018

for dll in globs_to_include:
dll_path = os.path.join(pyqt5_library_info.location['BinariesPath'],
dll)
dll_file_paths = glob.glob(dll_path)[:1]

This comment has been minimized.

@bjones1

bjones1 Jun 14, 2018

Member

Would you add a comment here, explaining the [:1]? "Allow exactly one match" or something like that?

to_include.append((dll_file_path, dst_dll_path))
if len(to_include) == len(globs_to_include):
return to_include
return []

This comment has been minimized.

@bjones1

bjones1 Jun 14, 2018

Member

Would you put this in an else clause for clarify?

This comment has been minimized.

@Siecje

Siecje Jun 14, 2018

Contributor

I prefer no else.

Here is a lightning talk by Larry Hastings on the matter. https://youtu.be/JVVMMULwR4s?t=4m57s

This comment has been minimized.

@bjones1

bjones1 Jun 14, 2018

Member

That's fine. I disagree with the presenter (Beazley is a great programmer -- I'd prefer to follow his coding convention instead), but it's not a big deal.

binaries = []
angle_files = ['libEGL.dll', 'libGLESv2.dll', 'd3dcompiler_??.dll']

This comment has been minimized.

@bjones1

bjones1 Jun 14, 2018

Member

Very nice. I like your use of named variables (angle_files) to clarify the code's operation.

This comment has been minimized.

@bjones1

bjones1 Jun 14, 2018

Member

Would you also add a comment, explaining the need for include_all_or_none? That is, that Qt will generate exceptions/crashes if all aren't present, but if none are present it will try other methods of rendering?

This comment has been minimized.

@Siecje

Siecje Jun 14, 2018

Contributor

As far as I know the exception/crash is from PyInstaller checking that all of the dependencies for each files included from binaries.

def _getImports_pe(pth):

This comment has been minimized.

@bjones1

bjones1 Jun 14, 2018

Member

??? I mean, the include_all_or_none function isn't needed for all the wildcarded files -- PyInstaller won't complain about not finding them. I think this avoid Qt failures when some but not all files are present, causing an error. Am I totally confused? If not, why do this (except for the opengl32sw.dll case, where there's no wildcard present).

This comment has been minimized.

@Siecje

Siecje Jun 14, 2018

Contributor

We want PyInstaller to include libEGL.dll, libGLESv2.dll, and d3dcompiler_47.dll.

Since d3dcompiler_47.dll is not available in CI, it will fail.

This is a middle ground, that will work if all the files are included and won't cause a failure during CI.

I think in general it is a good idea to fail when you are packaging something without its dependencies. But we can't have CI fail, so this is required until the PyQt5 Windows wheels include d3dcompiler_4?.dll.

This comment has been minimized.

@BoboTiG

BoboTiG Aug 16, 2018

Contributor

As you said, d3dcompiler_??.dll is not require to have a working PyQt5 application. As this DLL is not shipped by PyQt5, the hook will never include any DLLs. Thus applications will not work.
Do you think it will be a good idea to remove that DLL from the hook?

This comment has been minimized.

@bjones1

bjones1 Aug 16, 2018

Member

I don't understand your logic. d3dcompiler_??.dll is only included if the other two DLLs are present, guaranteeing a functioning PyQt5. Wheels in the future may include this; self-compiled PyQt5 installs might; perhaps some Linux packages may, so this supports it if present.

This comment has been minimized.

@BoboTiG

BoboTiG Aug 16, 2018

Contributor

The logic here is that d3dcompiler_??.dll is not required, only the 2 others. A PyQt5 application is working with only libEGL.dll and libGLESv2.dll.

As Siecje pointed, the d3dcompiler_??.dll is not a dependence of any of the other DLLs. Also, it is not shipped with PyQt5. PyQt5 does ship libEGL.dll and libGLESv2.dll.

My point is the hook will only include DLLs when the 3 are found. But as d3dcompiler_??.dll is not found, no DLL are included at all. Could it be possible to include 2 or 3 DLLs, instead of 3 or none?

I think this is working because my application is not doing any use of Qt3D (based on the DLL name), which is the case of most GUI applications. If an application uses Qt3D, then errors logged into the console are good clues for anyone willing to debug.

Or am I missing something?

This comment has been minimized.

@bjones1

bjones1 Aug 16, 2018

Member

Quoting the Qt docs, "If dynamic OpenGL is used, you additionally need to include the libraries required for ANGLE and software rendering. For ANGLE, both libEGL.dll and libGLESv2.dll from Qt's lib directory are required as well as the HLSL compiler from DirectX. The HLSL compiler library, d3dcompiler_XX.dll, where XX is the version number that ANGLE (libGLESv2) was linked against."

To me, this means all three are required if any are required. From your comments, I assume that you have a non-openGL PyQt5 app that works with libEGL.dll and libGLESv2.dll, but doesn't complain if d3dcompiler_??.dll is missing, correct? Does it still work if libEGL.dll and libGLESv2.dll are missing?

On a side note, current PyQt5 wheels do include d3dcompiler_47.dll, so this code will include all three. I'm not sure what version of PyQt5 this started with, but it's fairly recent.

This comment has been minimized.

@BoboTiG

BoboTiG Aug 16, 2018

Contributor

You are right, if libGLE* DLLS are missing, it does not work.
I use PyQt 5.10.2, and the third DLL is missing. Perhaps I just need to include them myself, I did not see the last one is now shipped with PyQt 5.11+.

This is OK like that, thanks for the clarification.

# Include ICU files, if they exist.
# See the "Deployment approach" section in ``PyInstaller/utils/hooks/qt.py``.
icu_files = ['icudt??.dll', 'icuin??.dll', 'icuuc??.dll']
binaries += include_all_or_none(icu_files)

This comment has been minimized.

@bjones1

bjones1 Jun 14, 2018

Member

Thanks for protecting this with your include_all_or_none -- again, that's very helpful.

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 14, 2018

Thanks for the updates! Much better! I have a few more nitpicks, then this looks good.

@@ -20,35 +20,36 @@
if os.path.basename(x[0]) == 'qt.conf']
def include_all_or_none(globs_to_include):
def find_all_or_none(globs_to_include, num_files):

This comment has been minimized.

@bjones1

bjones1 Jun 14, 2018

Member

Nice. I like the switch to using num_files instead of the 1-to-1 rule.

@Siecje Siecje changed the title from Hooks: Include OpenGL Fallback DLLs for PyQt5. Fixes issue #3458. to Hooks: Include OpenGL fallback DLLs for PyQt5. Fixes #3458. Jun 14, 2018

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 15, 2018

OK, looks good. Would you either squash these two commits, or re-title the most recent commit (hooks: refactor based on pull request feedback or something like that)?

@Siecje Siecje force-pushed the Siecje:3458 branch from ca46238 to ecdf8b3 Jun 15, 2018

@Siecje

This comment has been minimized.

Contributor

Siecje commented Jun 15, 2018

I changed the comment in the function and I have squashed the commits.

@htgoebel htgoebel self-requested a review Jun 15, 2018

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 15, 2018

@Siecje, thanks for your work on this! The one Appveyor failure is a testing problem, not anything to do with your contribution.

@bjones1 bjones1 merged commit 1ae6a34 into pyinstaller:develop Jun 15, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment