GPUOpen-LibrariesAndSDKs / HIPRT

Other
89 stars 5 forks source link

next-release. Use CMAKE_PROJECT_DIR or CMAKE_CURRENT_SOURCE_DIR instead of CMAKE_SOURCE_DIR to be able to use HIPRT as a submodule? #13

Open TomClabault opened 2 months ago

TomClabault commented 2 months ago

Maybe this is already planned but just in case:

Suppose I have this project structure:

MyProject/
├── CMakeLists.txt
└── HIPRT/
    ├── CMakeLists.txt
    ├── contrib
    ├── hiprt
    ├── scripts
    ├── test
    ├── tools
    └── ...

If I want to use HIPRT in one of my project as a subdirectory, I would use add_subdirectory(HIPRT). However, the CMakeLists.txt of HIPRT currently uses ${CMAKE_SOURCE_DIR } to refer to the HIPRT/ directory:

https://github.com/GPUOpen-LibrariesAndSDKs/HIPRT/blob/d14726ed77e26fe690ca8add17328d8f8cd2737f/CMakeLists.txt#L244

In the case where HIPRT's CMakeLists.txt is called from add_subdirectory(HIPRT), ${CMAKE_SOURCE_DIR } refers to the MyProject/ directory instead of HIPRT/ as expected.

A solution would be to use either ${CMAKE_CURRENT_SOURCE_DIR } or ${CMAKE_PROJECT_DIR}

RichardGe commented 2 months ago

Hi @TomClabault thanks for this catch. I prepared a new cmake file that should manage your usecase correctly.

here is a preview of it, in case you want to test it: CMakeLists.txt Note that the binaries will still be inside : ${CMAKE_CURRENT_SOURCE_DIR}/dist/bin

will be included in our next release.

TomClabault commented 2 months ago

I tested it on Windows with the VS2022 generator.

I could configure my project using HIPRT as a submodule with the new CMake but:

Here the CMakeLists.txt I used for my testings along with the very simple main.txt for the testing the linking with the library (that's a .cpp file but Github doesn't want attached CPP files).

TomClabault commented 2 months ago

After further testing, it seems that the reason why HIPRT isn't automatically built alongside the project that adds it as a dependency with add_subdirectory() is because it is linked with target_link_libraries.

I added these two lines to the HIPRT CMakeLists.txt:

add_library(hiprtLib INTERFACE)
target_link_libraries(hiprtLib INTERFACE ${HIPRT_NAME})

below add_library(${HIPRT_NAME} SHARED) and I linked my TestProject (from the CMakeLists in my previous comment) against hiprtLib:

target_link_libraries(TestHIPRTExec hiprtLib)

Now HIPRT compiles and generate a .lib file under dist/bin/.

So maybe in the end what's needed is just a "public" name to link against outside of HIPRT's CMakeLists.txt.

RichardGe commented 1 month ago

Thanks Tom. I need to review your changes carefully as we have several projects depending on HIPRT and I need to make sure I don't break anything with those cmake changes.