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.

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

[googletest][rocm-cmake] multiple ROCMChecks WARNING messages #2494

Open
junliume opened this issue Oct 31, 2023 · 5 comments
Open

[googletest][rocm-cmake] multiple ROCMChecks WARNING messages #2494

junliume opened this issue Oct 31, 2023 · 5 comments

Comments

@junliume
Copy link
Collaborator

Issues may be observed when users are building MIOpen

*******************************************************************************
*------------------------------- ROCMChecks WARNING --------------------------*
  Options and properties should be set on a cmake target where possible. The
  variable 'CMAKE_CXX_FLAGS' may be set by the cmake toolchain, either by
  calling 'cmake -DCMAKE_CXX_FLAGS=""'
  or set in a toolchain file and added with
  'cmake -DCMAKE_TOOLCHAIN_FILE=<toolchain-file>'. ROCMChecks now calling:
CMake Warning at /opt/rocm/share/rocm/cmake/ROCMChecks.cmake:46 (message):
  'CMAKE_CXX_FLAGS' is set at
  /home/junliu/MIOpen/build/_deps/googletest-src/googletest/CMakeLists.txt:<line#>
  shown below:
Call Stack (most recent call first):
  build/_deps/googletest-src/googletest/cmake/internal_utils.cmake:9223372036854775807 (rocm_check_toolchain_var)
  build/_deps/googletest-src/googletest/cmake/internal_utils.cmake:42 (string)
  build/_deps/googletest-src/googletest/cmake/internal_utils.cmake:72 (fix_default_compiler_settings_)
  build/_deps/googletest-src/googletest/CMakeLists.txt:83 (config_compiler_and_linker)


*-----------------------------------------------------------------------------*
*******************************************************************************


*******************************************************************************
*------------------------------- ROCMChecks WARNING --------------------------*
  Options and properties should be set on a cmake target where possible. The
  variable 'CMAKE_CXX_FLAGS' may be set by the cmake toolchain, either by
  calling 'cmake -DCMAKE_CXX_FLAGS=""'
  or set in a toolchain file and added with
  'cmake -DCMAKE_TOOLCHAIN_FILE=<toolchain-file>'. ROCMChecks now calling:
CMake Warning at /opt/rocm/share/rocm/cmake/ROCMChecks.cmake:46 (message):
  'CMAKE_CXX_FLAGS' is set at
  /home/junliu/MIOpen/build/_deps/googletest-src/googletest/CMakeLists.txt:<line#>
  shown below:
Call Stack (most recent call first):
  build/_deps/googletest-src/googletest/cmake/internal_utils.cmake:9223372036854775807 (rocm_check_toolchain_var)
  build/_deps/googletest-src/googletest/cmake/internal_utils.cmake:47 (string)
  build/_deps/googletest-src/googletest/cmake/internal_utils.cmake:72 (fix_default_compiler_settings_)
  build/_deps/googletest-src/googletest/CMakeLists.txt:83 (config_compiler_and_linker)


*-----------------------------------------------------------------------------*
*******************************************************************************

[Actual Issue Tracked Here]:
ROCm/rocm-cmake#154

@junliume
Copy link
Collaborator Author

Credit to @lawruble13

You can totally suppress the warning by setting the CMake cache variable ROCM_WARN_TOOLCHAIN_VAR to OFF prior to including rocm-cmake, but note that this also suppresses the warning for your own CMake as well (i.e. suppresses any true positives). At present, there is no way to temporarily/partially disable the toolchain variable warnings

@pfultz2
Copy link
Contributor

pfultz2 commented Nov 1, 2023

Ideally, we shouldn't be building googletest in our cmake. It should be listed under requirements.txt(just add google/googletest@v1.14.0) so rbuild/cget can build it once in the dockerfile instead of downloading and rebuilding this everytime.

This also avoids other issues in the cmake such as FORCE overwriting BUILD_SHARED_LIBS to Off, which can make a different ordering of includes in cmake always build miopen as static.

My original review feedback for this was ignored, but is still relevant:

https://github.com/ROCmSoftwarePlatform/MIOpen/pull/1611/files#r912193677

@junliume
Copy link
Collaborator Author

junliume commented Nov 2, 2023

@pfultz2 thanks! The original comment was not ignored, while we thought this is rather standard and officially recommended way of including gooletest, e.g. also in Triton: https://github.com/ROCmSoftwarePlatform/triton/blob/9517d4c2566cd2763d038b95b79713a04a36eac6/unittest/googletest.cmake#L10
I agree that it is not ideal that we rebuild this everytime.

Maybe ROCm should also have included googletest as a dependency to avoid too many different versions among different components.

@junliume
Copy link
Collaborator Author

junliume commented Nov 2, 2023

@xinlipn could you follow up with the above comment, and next time discuss how the way googletest should be installed?

@apwojcik
Copy link
Collaborator

apwojcik commented Nov 14, 2023

According to CMake Dependency Guide the primary methods of bringing dependencies into the build are the find_package() command and the FetchContent module. The ROCm-CMake ROCMCheck is too restrictive on third-party projects. However, that can resolved by using the SYSTEM attribute for FetchContent_Declare().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants