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
Add support for CMAKE_*_OUTPUT_DIRECTORY
variables for Slint build artefacts
#1979
Conversation
This includes support for placing build artefacts into output directories using standard cmake variables.
We can now use the standard CMake variables to make sure that a copy of the DLL lands next to the executables for Windows.
d54f435
to
3a564b7
Compare
Now that the binaries are placed in their respective CMAKE_RUNTIME_OUTPUT_DIRECTORY, that's config suffixed, let's use the ctest signature that accepts a cmake target and thus automatically figures out the correct location of the executable.
Tell ctest what configuration we want to test
f556dd3
to
d02a425
Compare
@@ -148,7 +148,7 @@ jobs: | |||
buildWithCMakeArgs: '--config Debug' | |||
- name: ctest | |||
working-directory: ${{ runner.workspace }}/cppbuild | |||
run: ctest --verbose | |||
run: ctest --verbose -C Debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an intended change as part of the OUTPUT_DIRECTORY addition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/bin/debug) | ||
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/bin/release) | ||
endif() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to go above the include(CTest)
to get applied to all the testing code or is it fine here? CMake sometimes is a bit tricky with when variables take effect :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine here AFAIK. CTest doesn't care about the output directory variables. All it cares about are the commands to run and how to locate them (in our case via the target).
@@ -382,7 +377,7 @@ if(BUILD_TESTING) | |||
target_compile_definitions(test_${NAME} PRIVATE | |||
SOURCE_DIR=\"${CMAKE_CURRENT_SOURCE_DIR}/\" | |||
) | |||
add_test(test_${NAME} ${CMAKE_CURRENT_BINARY_DIR}/test_${NAME}) | |||
add_test(NAME test_${NAME} COMMAND test_${NAME}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it save to assume that will be found on the system? That "${CMAKE_CURRENT_BINARY_DIR}/" added an absolute path and made sure this will be found... I assume the tests end up in "${CMAKE_BINARY_DIR}/bin" now? Is that in PATH?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is safe that it will be found on the system, because we're passing a CMake target here, and this variant of add_test
supports that, so it'll fetch the location actually from the cmake target, not assume that 'test_${NAME}' is in PATH
or so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right!
Upgrade to a newer version of Corrosion that implements this and remove our workarounds.
Closes #1025