rocm-cmake icon indicating copy to clipboard operation
rocm-cmake copied to clipboard

[googletest] Possible False Positive of ROCMChecks WARNING

Open junliume opened this issue 2 years ago • 4 comments

[Observations] with googletest v1.14.0 (latest version as of 10/31/2023)

FetchContent_Declare(
    googletest
    GIT_REPOSITORY https://github.com/google/googletest.git
    GIT_TAG f8d7d77c06936315286eb55f8de22cd23c188571
)

We observe multiple ROCMChecks WARNING warnings like

*******************************************************************************
*------------------------------- ROCMChecks WARNING --------------------------*
  Options and properties should be set on a cmake target where possible. The
  variable 'CMAKE_C_FLAGS' may be set by the cmake toolchain, either by
  calling 'cmake -DCMAKE_C_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_C_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:52 (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:38 (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)


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

[Likely Root Cause]:

https://github.com/google/googletest/blob/b10fad38c4026a29ea6561ab15fc4818170d1c10/googletest/cmake/internal_utils.cmake#L25

It is likely that rocm-cmake grep the keywords from

    foreach (flag_var
             CMAKE_C_FLAGS CMAKE_C_FLAGS_DEBUG CMAKE_C_FLAGS_RELEASE
             CMAKE_C_FLAGS_MINSIZEREL CMAKE_C_FLAGS_RELWITHDEBINFO
             CMAKE_CXX_FLAGS CMAKE_CXX_FLAGS_DEBUG CMAKE_CXX_FLAGS_RELEASE
             CMAKE_CXX_FLAGS_MINSIZEREL CMAKE_CXX_FLAGS_RELWITHDEBINFO)

such as CMAKE_CXX_FLAGS and CMAKE_C_FLAGS

Should this be a false positive? How can we disable or suppress warning like this one?

junliume avatar Oct 31 '23 19:10 junliume

Dont build dependencies directly using FetchContent. This prevents the project from being built without an internet connection. It prevents distro maintainers from using their own prebuilt dependencies to build the component as well.

FetchContent can be used to build dependencies but only from a superproject. That way users can just build the project directly without any FetchContent. However, FetchContent is a very poor man's package manager and suffers from many issues such as downloading and building everytime. Its better to use a package manager to handle dependencies instead.

Due to issues with using FetchContent in our projects I am considering adding some kind of warning in ROCMChecks for FetchContent.

pfultz2 avatar Nov 01 '23 15:11 pfultz2

@pfultz2 which way we should build googletest can indeed be discussed, but this ticket itself is pointing to an issue with ROCMChecks WARNING:

  • This warning cannot be disabled "safely" or re-directed, and repeated warnings cannot be consolidated to avoid clogging screen output
  • ROCMCHECKS_WARN_TOOLCHAIN_VAR can disable it but also disabled other checks?
  • The case above is most likely a false positive:
    # This replacement code is taken from sample in the CMake Wiki at
    # https://gitlab.kitware.com/cmake/community/wikis/FAQ#dynamic-replace.

junliume avatar Nov 07 '23 23:11 junliume

This warning cannot be disabled "safely" or re-directed, and repeated warnings cannot be consolidated to avoid clogging screen output

The warning shouldn't be ignored. And this case its even more problematic because a third-party library is changing the compilation flags for MIOpen outside of your control.

Furthermore, in the future these warnings will become errors.

The case above is most likely a false positive:

Its not a false positive. Following the wiki page, it suggests using a different approach than whats written in googletest.

pfultz2 avatar Nov 08 '23 18:11 pfultz2

I also see this warning from rocThrust:

CMake Warning at /opt/rocm/share/rocmcmakebuildtools/cmake/ROCMChecks.cmake:46 (message):
  'CMAKE_CXX_FLAGS' is set at
  /__w/traccc/traccc/build/_deps/rocthrust-src/CMakeLists.txt:<line#> shown
  below:
Call Stack (most recent call first):
  build/_deps/rocthrust-src/CMakeLists.txt:9223372036854775807 (rocm_check_toolchain_var)
  build/_deps/rocthrust-src/CMakeLists.txt:105 (set)

when built with FetchContent. Setting ROCMCHECKS_WARN_TOOLCHAIN_VAR does not seem to disable it.

StewMH avatar May 16 '24 15:05 StewMH