-
Notifications
You must be signed in to change notification settings - Fork 548
Several CMake fixes on Windows #2743
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
Conversation
In Visual Studio the CMAKE_GENERATOR_PLATFORM is not set if you use a generator that includes the arch flag in its name.
On windows the compilevars.sh script sets the LIB variable to the location of the *.lib files. This is LIBRARY_PATH on Linux
In a multi-config build, CMake will generate multiple files targeting different types of builds. If you targeted one build then switched to another when building ArrayFire, the old ArrayFire CMake config files were still there and parsed. When multiple configs are found CMake will return a list of configs instead of one. This case was not handled by the current ArrayFireConfig file. Fixed in this commit.
@@ -341,7 +342,7 @@ find_package_handle_standard_args(MKL_Shared | |||
MKL_ThreadingLibrary_LINK_LIBRARY) | |||
|
|||
find_package_handle_standard_args(MKL_Static | |||
FAIL_MESSAGE "Source the compilervars.sh or mklvars.sh scripts included with your installation of MKL. Looking in MKLROOT and LIBRARY_PATHS environment variables" | |||
FAIL_MESSAGE "Could NOT find MKL: Source the compilervars.sh or mklvars.sh scripts included with your installation of MKL. This script searches for the libraries in MKLROOT, LIBRARY_PATHS(Linux), and LIB(Windows) environment variables" |
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.
nit: compilevars.<sh/bat> script. It may cause in github issues asking what should we do for windows.
@@ -20,7 +20,8 @@ function(af_split_debug_info _target _destination_dir) | |||
set(SPLIT_TOOL_EXISTS OFF) | |||
if (MSVC) | |||
install(FILES | |||
$<TARGET_PDB_FILE:${_target}> | |||
$<$<CONFIG:Debug>:$<TARGET_PDB_FILE:${_target}>> | |||
$<$<CONFIG:RelWithDebInfo>:$<TARGET_PDB_FILE:${_target}>> |
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.
Interesting, so if there is no file at all for install(FILES ...
it won't error out as I expected.
@@ -37,7 +37,7 @@ index 9446499..786f7db 100644 | |||
set(CLBLAST_PATCH_COMMAND ${GIT} apply ${ArrayFire_BINARY_DIR}/clblast.patch) | |||
endif() | |||
|
|||
if(WIN32 AND NOT CMAKE_GENERATOR MATCHES "Ninja") | |||
if(WIN32 AND CMAKE_GENERATOR_PLATFORM AND NOT CMAKE_GENERATOR MATCHES "Ninja") |
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.
Interestingly, CMAKE_GENERATOR_PLATFORM is available only when user sets it. I didn't realize that earlier, I was under the impression this is auto-populated (when not explicitly provided for relevant generators) with project call/during compiler check.
Starting with CMake 3.14, there is a new variable CMAKE_VS_PLATFORM_NAME_DEFAULT that contains default platform for Visual Studio generators. This variable defaults to Win32 for VS 2017 or below.
Fixed several issues when building on Windows.