Skip to content

CMake CHECK_CXX_COMPILER_FLAG logic invalid #28

@matthewfeickert

Description

@matthewfeickert

In PR #23 CMake logic checks were added to try to catch invalid CXXFLAGS on different platforms. I would argue these should be reverted as:

  • The CMake code

qcdloop/CMakeLists.txt

Lines 34 to 39 in b778fa9

foreach(fl ${CXX_FLAGS_TO_CHECK})
CHECK_CXX_COMPILER_FLAG(${fl} COMPILER_SUPPORTS_${fl})
if(COMPILER_SUPPORTS_${fl})
set(CXX_FLAGS_PASSED "${CXX_FLAGS_PASSED} ${fl}" )
endif()
endforeach()

is wrong if there are any spaces in the flags in ${CXX_FLAGS_TO_CHECK}, which is valid and possible if you set CXXFLAGS and override these hardcoded options.

  • The approach of removing invalid flags instead of failing is not desirable. A user should know when compilation options are invalid and should correct these from the environment or by passing explicit CMake options. (c.f. ENH: Add Linux ppc64le builds conda-forge/qcdloop-feedstock#1)

  • It contains logic errors like

set(CXX_FLAGS_DEBUG_TO_CHECK "${CMAKE_CXX_FLAGS_DEBUG} -fsanitize=address ${CMAKE_CXX_FLAGS_TO_CHECK}")

(CMAKE_CXX_FLAGS_TO_CHECK hasn't been set yet, should be CXX_FLAGS_TO_CHECK).

Trying to guard users from incorrect compilation options ends up causing more harm than good, and it would be better to default to checking for the existence of environment variables like CXXFLAGS (as CMake already does) and in the absence of them set defaults based off of the compiler type.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions