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] Possible False Positive of ROCMChecks WARNING #154

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

[googletest] Possible False Positive of ROCMChecks WARNING #154

junliume opened this issue Oct 31, 2023 · 4 comments
Assignees
Labels

Comments

@junliume
Copy link

[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?

@pfultz2
Copy link
Collaborator

pfultz2 commented Nov 1, 2023

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.

@junliume
Copy link
Author

junliume commented Nov 7, 2023

@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.

@pfultz2
Copy link
Collaborator

pfultz2 commented Nov 8, 2023

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.

@StewMH
Copy link

StewMH commented May 16, 2024

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.

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

No branches or pull requests

5 participants