Skip to content

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

Merged
merged 4 commits into from
Feb 7, 2020
Merged

Conversation

umar456
Copy link
Member

@umar456 umar456 commented Feb 6, 2020

Fixed several issues when building on Windows.

  • For certain generators the CMAKE_GENERATOR_PLATFORM was not set and caused an error due to changes in Fix build configs of mutli-config cmake generators #2736
  • Release builds do not generate PDB files but the install script failed because it couldn't find them
  • MKL sets the LIB environment variable instead of the LIBRARY_PATH environment variable on linux
  • Fixed a ArrayFireConfig.cmake issue caused by multiple configuration files in the build/install folder. This fixes cmake build issue #2623

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"
Copy link
Member

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}>>
Copy link
Member

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")
Copy link
Member

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.

@9prady9 9prady9 merged commit a054fff into arrayfire:master Feb 7, 2020
@9prady9 9prady9 deleted the cmake_fixes branch February 7, 2020 07:21
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.

cmake build issue
2 participants