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.

Sign up for GitHub

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

Make sure vcpkg_test_cmake uses the correct generator #5635

Closed
wants to merge 2 commits into from
Closed

Make sure vcpkg_test_cmake uses the correct generator #5635

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6329f57
Make sure vcpkg_test_cmake uses the correct generator
tarcila Mar 11, 2019
08a3951
Make GENERATOR variable intent clearer
tarcila Mar 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 17 additions & 16 deletions scripts/cmake/vcpkg_test_cmake.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
##
## ## Usage:
## ```cmake
## vcpkg_test_cmake(PACKAGE_NAME <name> [MODULE])
## vcpkg_test_cmake(PACKAGE_NAME <name> [MODULE] [GENERATOR generator])
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if targets could also be tested

## ```
##
## ## Parameters:
Expand All @@ -15,8 +15,12 @@
## ### MODULE
## Indicates that the library expects to be found via built-in CMake targets.
##
## ### GENERATOR
## The cmake generator to use for the test. It not provided, fallback to the one used
## during a previous vcpkg_configure_cmake() call, or use CMake default generator.
##
function(vcpkg_test_cmake)
cmake_parse_arguments(_tc "MODULE" "PACKAGE_NAME" "" ${ARGN})
cmake_parse_arguments(_tc "MODULE" "PACKAGE_NAME;GENERATOR" "" ${ARGN})

if(NOT DEFINED _tc_PACKAGE_NAME)
message(FATAL_ERROR "PACKAGE_NAME must be specified")
Expand All @@ -27,6 +31,14 @@ function(vcpkg_test_cmake)
set(PACKAGE_TYPE CONFIG)
endif()

if(_tc_GENERATOR)
set(GENERATOR ${_tc_GENERATOR})
else()
# _VCPKG_CMAKE_GENERATOR is defined if vcpkg_configure_cmake() has been called
# if not, GENERATOR is undefined and will be handled later on
set(GENERATOR ${_VCPKG_CMAKE_GENERATOR})
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor thing:
${_VCPKG_CMAKE_GENERATOR} might not be set.

Changes are similar to:
4cfca10
and
be85bc5

except that you allow to manually define a generator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that's why the content of GENERATOR is tested later on.
I'll update the comment to make this more explicit.

Copy link
Member

Choose a reason for hiding this comment

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

Quote from a discussion about this with ras0219:

We should always use ninja here (and skip it for 32-bit hosts). There's no reason to use the MSBuild generator, nor for it to be configurable. This is because all generators should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking a look at this PR!

I agree with the latest part of the comment. But I don't get the rational for the very first part: why skip 32bits? and why ninja only?

The very first intent of my commit is, by using the same generator, that we target the same platform/architecture: the breakage on my side is that the package is generated for x64 but is searched for by an x86 generator.
But indeed, reusing the same generator might not be the way to go. Maybe just enforcing the architecture could be sufficient first step (I don't know if ninja generator supports -A flag).

Note that in a general case, it seems to me that find_package logic will be mostly independent of the generator used (when try_compile/link/run are not used at least).

endif()

if(VCPKG_PLATFORM_TOOLSET STREQUAL "v142")
message(STATUS "Skipping CMake integration test due to v142 / CMake interaction issues")
return()
Expand All @@ -36,32 +48,21 @@ function(vcpkg_test_cmake)
file(REMOVE_RECURSE ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-test)
file(MAKE_DIRECTORY ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-test)

#Generate Dummy source
# set(VCPKG_TEST_SOURCE ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-test/CMakeIntegration.cpp)
# file(WRITE ${VCPKG_TEST_SOURCE} "int main() \{\n")
# file(APPEND ${VCPKG_TEST_SOURCE} "return 0;}")
# Generate test source CMakeLists.txt
set(VCPKG_TEST_CMAKELIST ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-test/CMakeLists.txt)
file(WRITE ${VCPKG_TEST_CMAKELIST} "cmake_minimum_required(VERSION 3.10)\n")
file(APPEND ${VCPKG_TEST_CMAKELIST} "set(CMAKE_PREFIX_PATH \"${CURRENT_PACKAGES_DIR};${CURRENT_INSTALLED_DIR}\")\n")
file(APPEND ${VCPKG_TEST_CMAKELIST} "\n")
file(APPEND ${VCPKG_TEST_CMAKELIST} "find_package(${_tc_PACKAGE_NAME} ${PACKAGE_TYPE} REQUIRED)\n")
#To properly test if the package is actually working haveway correctly we have to link all targets of a package to
#a test executable and than actually build it. This will not discover if every symbol exported by the library is available/linked
#but it will doscover if all files which are linked by a target actual exist. Problem is: How to discover all targets?
# file(APPEND ${VCPKG_TEST_CMAKELIST} "add_executable(${_tc_PACKAGE_NAME}_exe ${VCPKG_TEST_SOURCE})\n")
# file(APPEND ${VCPKG_TEST_CMAKELIST} "target_link_libraries(${_tc_PACKAGE_NAME}_exe PRIVATE ${_tc_PACKAGE_NAME})\n")

if(DEFINED _VCPKG_CMAKE_GENERATOR)
set(VCPKG_CMAKE_TEST_GENERATOR "${_VCPKG_CMAKE_GENERATOR}")
else()
set(VCPKG_CMAKE_TEST_GENERATOR Ninja)
if(GENERATOR)
set(GENERATOR_ARGS "-G" "${GENERATOR}")
endif()

# Run cmake config with a generated CMakeLists.txt
set(LOGPREFIX "${CURRENT_BUILDTREES_DIR}/test-cmake-${TARGET_TRIPLET}")
execute_process(
COMMAND ${CMAKE_COMMAND} -G ${VCPKG_CMAKE_TEST_GENERATOR} .
COMMAND ${CMAKE_COMMAND} ${GENERATOR_ARGS} .
OUTPUT_FILE "${LOGPREFIX}-out.log"
ERROR_FILE "${LOGPREFIX}-err.log"
RESULT_VARIABLE error_code
Expand Down