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

Start at adjusting dependencies to match what windeployqt suggests #537

Merged
merged 8 commits into from Jun 3, 2022

Conversation

macumber
Copy link
Collaborator

@macumber macumber commented May 12, 2022

Fixes #358

@macumber
Copy link
Collaborator Author

windeployqt is also suggesting:

  • iconengines (dir)
  • imageformats (dir)
  • networkinformation (dir)
  • position (dir)
  • tls (dir)
  • Qt6SerialPort.dll

Those all seem plausible (except maybe Qt6SerialPort) but I am not sure if it is too heavy. Total extra size is 2.67 MB so maybe best to just add them.

@macumber
Copy link
Collaborator Author

I'll also want to check this on Mac, I think Qt6::DBus might need to be in QT_LIBS

@macumber
Copy link
Collaborator Author

Updated SHA to include hotfix for NREL/OpenStudio#4584

@macumber macumber marked this pull request as ready for review May 15, 2022 03:09
@macumber macumber requested a review from jmarrec May 15, 2022 03:09
FindOpenStudioSDK.cmake Outdated Show resolved Hide resolved
@macumber macumber marked this pull request as draft May 15, 2022 04:05
@macumber macumber marked this pull request as ready for review May 15, 2022 16:54
@macumber
Copy link
Collaborator Author

@jmarrec I think this is good to go if it looks ok to you

@macumber macumber added this to the OpenStudio Application 1.4.0 milestone May 15, 2022
Copy link
Collaborator

@jmarrec jmarrec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add_custom_command(TARGET ${target_name}
POST_BUILD
COMMAND ${QT_WINDEPLOY_QT} --dir $<TARGET_FILE_DIR:${target_name}> $<TARGET_FILE:${target_name}> )

What you're doing is aligning Mac/Linux onto what windeployqt (that we already use) is doing right?

Comment on lines +180 to +183
#add_custom_command(TARGET ${target_name}
# POST_BUILD
# COMMAND ${QT_MACDEPLOY_QT} $<TARGET_BUNDLE_DIR:${target_name}> -verbose=2
#)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use macdeployqt, and linuxdeployqt?

Copy link
Collaborator Author

@macumber macumber May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually I think so. When I ran macdeployqt (and kept all the other remaining install steps) the app in the resulting package crashed, I don't know why.

@@ -521,7 +521,7 @@ message(STATUS "QT_INSTALL_DIR=${QT_INSTALL_DIR}")

find_package(Qt6 ${QT_VERSION} COMPONENTS CoreTools GuiTools WidgetsTools QmlTools WebEngineCoreTools REQUIRED PATHS ${QT_INSTALL_DIR} NO_DEFAULT_PATH)

find_package(Qt6 ${QT_VERSION} COMPONENTS Core Core5Compat Gui Widgets Sql Network Xml Concurrent PrintSupport Quick QuickWidgets Qml WebChannel Positioning WebEngineCore WebEngineWidgets REQUIRED PATHS ${QT_INSTALL_DIR} NO_DEFAULT_PATH)
find_package(Qt6 ${QT_VERSION} COMPONENTS Core Core5Compat Gui Widgets Sql Svg Network Xml Concurrent PrintSupport Quick QuickWidgets Qml WebChannel Positioning WebEngineCore WebEngineWidgets REQUIRED PATHS ${QT_INSTALL_DIR} NO_DEFAULT_PATH)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We aren't using QSvg nor do we have any .svg files in the repo. Why would we need it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure, Qt might be using SVG internally, I think some of our web assests use SVG. The app does load that dll when it is installed so I figured it was good to just include it:

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked out the source code for windeployqt, what it does is to find some DLLs you do use, then it reads what this DLL depends on and includes them as well. (findDependentQtLibraries -> readExecutable -> readPeExecutable)

Copy link
Collaborator

@jmarrec jmarrec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we ready to merge this?

Comment on lines +365 to +372
list(APPEND QT_DIRS iconengines)
list(APPEND QT_DIRS imageformats)
list(APPEND QT_DIRS networkinformation)
list(APPEND QT_DIRS position)
list(APPEND QT_DIRS tls)
foreach(DIR ${QT_DIRS})
install(DIRECTORY $<TARGET_FILE_DIR:${target_name}>/${DIR} DESTINATION bin/ COMPONENT OpenStudioApp)
endforeach()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those needed? are they needed on other platforms?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see your comment above, "Those all seem plausible (except maybe Qt6SerialPort) but I am not sure if it is too heavy. Total extra size is 2.67 MB so maybe best to just add them."

@jmarrec jmarrec merged commit 76958cc into develop Jun 3, 2022
@jmarrec jmarrec deleted the 358-install-opengl branch June 3, 2022 09:03
@github-actions github-actions bot locked and limited conversation to collaborators Jun 3, 2022
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.

Cannot run geometry editor on systems without OpenGL 2.0
2 participants