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 C++ memory game tutorial not starting out of the box on Windows #2034

Merged
merged 1 commit into from Jan 9, 2023

Conversation

tronical
Copy link
Member

@tronical tronical commented Jan 6, 2023

After commit 3e5aa21 the Slint DLLs aren't placed in the bin/ directory by default anymore. Since the tutorial builds Slint as an external sub-project, it is entirely isolated and the CMAKE_*_OUTPUT_DIRECTORY variables do not propagate. On macOS and Linux, the program still runs due to rpath. On Windows, the instructions said to put bin into %PATH%, but that doesn't work anymore, the dll is now in _deps/slint-build.

Instead of adjusting %PATH%, this change adjusts the documentation to recommend the use of a custom command on Windows using $<TARGET_RUNTIME_DLLS:tgt> to copy the DLL across. This is guarded with WIN32 due to https://gitlab.kitware.com/cmake/cmake/-/issues/23543 , where the proposed solution requires CMake 3.26 (not released yet).

tronical added a commit to slint-ui/slint-cpp-template that referenced this pull request Jan 6, 2023
@tronical
Copy link
Member Author

tronical commented Jan 6, 2023

Same change over in the C++ template repo: slint-ui/slint-cpp-template#7

CHANGELOG.md Outdated
@@ -10,6 +10,7 @@ All notable changes to this project are documented in this file.
- On an `Image`, the default value of `source-clip-width` and `source-clip-height` is now set to
the size of the image minus the `source-clip-{x,y}`. The source clip size is now used to compute
the default aspect ratio of the image.
- The C++ build now requires CMake >= 3.21.
Copy link
Member

Choose a reason for hiding this comment

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

Does it really? The new features seems to be used only in examples and docs.

People upgrading Slint don't need to care or can still choose not to do the copy. Also it's only on windows.
For this reason, i think we should keep the cmake_minimum_required to 3.19 as more linux distribution may then be supported.

I think in the tutorial docs / readme it's fine to say that the minimum version is 3.21

Copy link
Member Author

Choose a reason for hiding this comment

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

In my opinion we should not diverge between documentation and the technical requirement. Anyway, I've pushed a new version. Does that match what you have in mind?

@tronical tronical force-pushed the simon/cmake-windows-dll-copy branch from 852f9d7 to ddcc65d Compare January 6, 2023 11:59
CHANGELOG.md Outdated
@@ -10,6 +10,8 @@ All notable changes to this project are documented in this file.
- On an `Image`, the default value of `source-clip-width` and `source-clip-height` is now set to
the size of the image minus the `source-clip-{x,y}`. The source clip size is now used to compute
the default aspect ratio of the image.
- The documented minimum CMake version is now 3.21. The library can still be built with 3.19, but
the examples in the documentation require 3.21 on Windows.
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 think it is worth adding in the changelog.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, done.

```

so that Windows to can find the Slint run-time library. Then you can run it with
If you are stepping through this tutorial on a Windows machine, you can run it with
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the command for linux/mac also work on windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

The command on linux/mac is ./memory_game, which does not work on Windows. You'll get something about '.' not being recognised as a command. On the other hand, on Linux and macOS the leading ./ is required since the current directory is not in the PATH.

@@ -120,6 +120,9 @@ FetchContent_MakeAvailable(Slint)
add_executable(my_application main.cpp)
target_link_libraries(my_application PRIVATE Slint::Slint)
slint_target_sources(my_application my_application_ui.slint)
if (WIN32)
add_custom_command(TARGET my_application POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_RUNTIME_DLLS:my_application> $<TARGET_FILE_DIR:my_application> COMMAND_EXPAND_LISTS)
Copy link
Member

Choose a reason for hiding this comment

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

maybe this deserve a small command explaining what this does. I'm afraid this would be black magic for most people otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, done. I'll also add this to the C++ template change then.

tronical added a commit to slint-ui/slint-cpp-template that referenced this pull request Jan 6, 2023
tronical added a commit to slint-ui/slint-cpp-template that referenced this pull request Jan 6, 2023
After commit 3e5aa21 the Slint DLLs
aren't placed in the `bin/` directory by default anymore. Since the
tutorial builds Slint as an external sub-project, it is entirely
isolated and the CMAKE_*_OUTPUT_DIRECTORY variables do not propagate. On
macOS and Linux, the program still runs due to rpath. On Windows, the
instructions said to put `bin` into `%PATH%`, but that doesn't work
anymore, the dll is now in `_deps/slint-build`.

Instead of adjusting `%PATH%`, this change adjusts the documentation to
recommend the use of a custom command on Windows using
$<TARGET_RUNTIME_DLLS:tgt> to copy the DLL across. This is guarded with
WIN32 due to https://gitlab.kitware.com/cmake/cmake/-/issues/23543 ,
where the proposed solution requires CMake 3.26 (not released yet).
@tronical tronical force-pushed the simon/cmake-windows-dll-copy branch from 608af68 to 93e28f3 Compare January 9, 2023 12:29
@tronical tronical merged commit 3c65c61 into master Jan 9, 2023
@ogoffart ogoffart deleted the simon/cmake-windows-dll-copy branch April 19, 2023 09:04
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

2 participants