Conversation
Old `nvToolsExt` is deprecated and the new `nvtx3` is apparently preferred. [Evidence](https://cmake.org/cmake/help/latest/module/FindCUDAToolkit.html#nvtoolsext)
JBludau
left a comment
JBludau
left a comment
There was a problem hiding this comment.
this seems a bit over the place ... would it not be better to update documentation instead of the cmake?
| if(WIN32) | ||
| set(BUILD_SHARED_LIBS OFF) # We need to add __declspec(dllexport/dllimport) for Windows DLLs | ||
| set(BUILD_SHARED_LIBS OFF) | ||
| message(STATUS "We need to add __declspec(dllexport/dllimport) for Windows DLLs, until then windows building is not supported") |
There was a problem hiding this comment.
This is not really a STATUS. Is there a way to add this to the library?
There was a problem hiding this comment.
This is not really a STATUS
I agree, I wanted to keep the things that were being built on windows remaining for pipelines and testing.
Is there a way to add this to the library?
I am intending to do so under another ticket
| if(WIN32) | ||
| set(KokkosTools_ENABLE_SINGLE ON) | ||
| endif() | ||
| option(KokkosTools_ENABLE_SINGLE "Build single library interfacing all profilers and dispatching at runtime" ${WIN32}) |
There was a problem hiding this comment.
I like the old approach better since it is easier to read and reason about
| set(COMMON_HEADERS_PATH ${CMAKE_CURRENT_BINARY_DIR}/common) | ||
| include_directories(${COMMON_HEADERS_PATH}) | ||
|
|
|
|
||
| if(APPLE) | ||
| message(STATUS "Apple OSX target detected.") | ||
| message(STATUS "Apple OSX target detected - skipping Unix-only tools.") |
There was a problem hiding this comment.
it is misleading to print this here ... as this line is not skipping anything
| kp_add_library(kp_nvtx_connector kp_nvtx_connector.cpp) | ||
|
|
||
| target_link_libraries(kp_nvtx_connector CUDA::nvToolsExt) | ||
| target_link_libraries(kp_nvtx_connector CUDA::nvtx3) |
There was a problem hiding this comment.
how is that related to the description of the pull request?
| if(CMAKE_SOURCE_DIR STREQUAL CMAKE_BINARY_DIR) | ||
| message(FATAL_ERROR "FATAL: In-source builds are not allowed. You should create a separate directory for build files.") | ||
| message(FATAL_ERROR "FATAL: In-source builds are not allowed. You should create a separate directory for build files. \ | ||
| This can be done with the invocation cmake -B build -S . to create a build directory called build") |
There was a problem hiding this comment.
I would refrain from details how to do stuff in cmake ...
Will be done in another pull request This reverts commit 0d7b874.
| set(BUILD_SHARED_LIBS "Build shared libraries" ON) | ||
| if(WIN32) | ||
| set(BUILD_SHARED_LIBS OFF) # We need to add __declspec(dllexport/dllimport) for Windows DLLs | ||
| option(BUILD_SHARED_LIBS "Build shared libraries" ON) |
There was a problem hiding this comment.
This is not really an option. The tools will only work as shared libs.
| # We need to add __declspec(dllexport/dllimport) for Windows DLLs | ||
| if(WIN32 AND BUILD_SHARED_LIBS) | ||
| set(BUILD_SHARED_LIBS OFF) | ||
| message(STATUS "Windows building of .dlls is not yet supported") |
There was a problem hiding this comment.
I think we should error out
|
Replaced by pull request #298 I had a think about what the best thing to do is. It would make sense to leave anything that can compile on Windows to remain able to compile so it can be checked on pipelines. However there are no pipelines that compile anything on Windows so that's all moot. I am going to open a new PR that fails early if on windows (and a cmake variable is false so it can be turned off for dev) with a message explaining it's not supported. |
2 participants
I recently built kokkos tools on windows and found that some things could be communicated better, particularly why some features were not made available.
I have added warnings and more verbose messages to the CMake to make this more obvious.