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

INSTALL target #745

Merged
merged 8 commits into from
Dec 18, 2020
Merged

INSTALL target #745

merged 8 commits into from
Dec 18, 2020

Conversation

Hoikas
Copy link
Member

@Hoikas Hoikas commented Dec 15, 2020

This adds an INSTALL target that is supposed to install all of the files needed for the client and tools into an organized structure. It doesn't match the GNU standard of bin, lib, etc. because we are a game, not a system library or command line executable.

This is still considered a WIP because not all DLLs are copied into the installed tree on Windows. We currently run windeployqt on all GUI tools, and I would like to do something similar for the non-Qt DLLs. Vcpkg supports VCPKG_APPLOCAL_DEPS, which copies the DLLs into the binary directory. The installed variant of this is still experimental though and seems to error on my bare-bones install cmake commands. Do we want to try to work with vcpkg's experimental feature or roll that functionality ourselves? I realize, of course, that doing such on unix-like platforms is pointless... the goal I'm working toward here is to create good distributions for Windows.

The latest vcpkg master has a correctly functioning X_VCPKG_APPLOCAL_DEPS_INSTALL, so any DLLs needed by vcpkg are correctly copied into the install prefix. Further, our cmake now runs windeployqt on all GUI tools... because not everyone wants to build Qt5 from source using vcpkg .

This obsoletes the clunky "ScriptsSystem" target - now just make install or cmake --build . --target INSTALL to get that done.

Depends on #740.

@Hoikas Hoikas marked this pull request as draft December 15, 2020 02:20
I think vcpkg may be inserting generator expressions into our junk.
Kewl.
@Hoikas Hoikas marked this pull request as ready for review December 18, 2020 00:02
@Hoikas Hoikas changed the title WIP: INSTALL target INSTALL target Dec 18, 2020
CMakeLists.txt Outdated Show resolved Hide resolved
Scripts/CMakeLists.txt Show resolved Hide resolved
cmake/DeployQt5.cmake Outdated Show resolved Hide resolved
# Copy Qt DLLs to the build directory so the stupid application can be run in Visual Studio
add_custom_command(
TARGET ${_pdqt_TARGET} POST_BUILD
COMMAND ${CMAKE_COMMAND} -E remove_directory "$<TARGET_FILE_DIR:${_pdqt_TARGET}>/qt"
Copy link
Member

Choose a reason for hiding this comment

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

If this gets run more than once (which it does), each target will re-populate the deployment directory. Our Qt apps all use the same Qt libs, so the end result is probably still fine, but this still does more work than necessary.

Perhaps instead we can make a list of executables that need to be considered for windeployqt, and then a single target that runs it on all of them at the same time (windeployqt can take multiple executables on the command line)...

Copy link
Member Author

Choose a reason for hiding this comment

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

I was initially considering doing that by doing cmake's undocumented function overloading... But vcpkg already does that for add_executable(), which means we'd introduce an inifite loop. Maybe we could have a plasma_add_qt_executable that handles the book-keeping for us? Appending to a list in the CMakeLists.txt or having a hardcoded list seems kind of dirty to me.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know why you need to overload anything... You could just switch to the normal install command for the Qt executables, and then run the deployment from the Tools directory's CMakeLists.txt, since that file already has knowledge of the full set of Qt executables to build...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not certain how to obtain the list of those executables aside from hardocding a list of them (yuck) or injecting logic to gain it.

Copy link
Member

Choose a reason for hiding this comment

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

Another option, which still duplicates work but at least doesn't potentially delete needed files, would be to add a target that only cleans the qt staging directory once, and then still have each Qt executable run its own deployment...

Copy link
Member Author

@Hoikas Hoikas Dec 18, 2020

Choose a reason for hiding this comment

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

One thing to note is that I've removed the default specification of CMAKE_RUNTIME_DIRECTORY, so, with Visual Studio, each executable is now building in its own directory and has its own qt staging directory. Previously, they were all going into <builddir>/bin/<config> So, if you're debugging one of these directly from the IDE, we might not want to clear its staging directory--you'd potentially be blasting the dependencies away before a debug session. I'm not sure what the behavior is with single config generators eg makefiles. (That comment about debugging from the IDE is irrelevant because we're copying Qt into a staging directory, not into the binary directory itself. Sigh.)

5. Copy the **resource.dat** file from `<build_dir>\bin` to the *MOUL-OS* folder,
or from http://www.guildofwriters.org/tools/resource.dat if you did not build
your own.
5. Download and copy http://www.guildofwriters.org/tools/resource.dat into
Copy link
Member

@Deledrius Deledrius Dec 18, 2020

Choose a reason for hiding this comment

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

Why remove the instructions for using the locally-built copy?
This new INSTALL feature automatically copies it to the correct place, so this step is entirely optional now.

Copy link
Member Author

Choose a reason for hiding this comment

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

resource.dat will only be generated if you manually pip install -r requirements.txt or otherwise have the correct packages preinstalled. I figure that's a problem for another PR 😄

Copy link
Member

@zrax zrax left a comment

Choose a reason for hiding this comment

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

Nice, I like it!

@Hoikas
Copy link
Member Author

Hoikas commented Dec 18, 2020

💥

@Hoikas Hoikas merged commit ca44ce2 into H-uru:master Dec 18, 2020
@Hoikas Hoikas deleted the install-target branch December 18, 2020 23:13
Hoikas added a commit to Hoikas/Plasma that referenced this pull request Dec 21, 2020
This does a lot of things. On a high level, it changes the resource.dat
file generation such that it properly gets marked as a GENERATED
dependency of plClient. Further, we correctly chain the vector graphics
and scripts as dependencies of resource.dat such that resource.dat will
only be regenerated by modifying these files. H-uru#745 unintentionally broke
this by removing the default definition of
`CMAKE_RUNTIME_OUTPUT_DIRECTORY` -- CI didn't detect it because it it
defines that variable in the cache.

More sneakily, we now see the beginnings of more automated dependency
management in that our CMake can now install the python packages for
building resource.dat. For now, it is OFF by default - it seems rude to
unilaterally install packages in a system Python install. Future work to
enable a submodule based vcpkg toolchain would use this facility
however.
@Hoikas Hoikas mentioned this pull request Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants