Page MenuHomePhabricator

[CMAKE] Fix cached variable that prevents testing for flag support
ClosedPublic

Authored by Fabien on Mar 6 2019, 09:27.

Details

Summary

The CHECK_C_COMPILER_FLAG() and CHECK_CXX_COMPILER_FLAG() functions
from CMake return the result of the test in a variable passed as an
argument.
This variable is cached internally, so that subsequent calls of the
function with the same variable return the result without performing the
test again.

The add_c_compiler_flag() and add_cxx_compiler_flag() functions are
calling CHECK_C_COMPILER_FLAG() and CHECK_CXX_COMPILER_FLAG() with
the same variable FLAG_IS_SUPPORTED, making the test ineffective.

This diffs fixes the issue by generating a new variable name based on
the flag name. This allow for keeping advantage of the caching
mechanisme while having a different variable for each flag, making the
test working as expected.

Test Plan

At the end of src/CMakeLists.txt, add the following line:
add_cxx_compiler_flag(-foo)
Then run:

mkdir buildcmake && cd buildcmake
cmake -GNinja ..
ninja -v

Before the patch:
CMake will output a Performing Test FLAG_IS_SUPPORTED - Success line
and the -foo option will be added on the command lines, leading to a
build failure.

After the patch:
CMake will output a Performing Test have_cxx_foo - Failed line for
the -foo option and it will not be added on the command lines.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cmake_fix_checkcompileflags_cache
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5187
Build 8437: Bitcoin ABC Buildbot (legacy)
Build 8436: arc lint + arc unit

Event Timeline

Slightly simplify the first regex in build_test_name_from_flag

This revision is now accepted and ready to land.Mar 6 2019, 22:25
deadalnix requested changes to this revision.Mar 7 2019, 17:15

This will generate the same names for C and C++.

This revision now requires changes to proceed.Mar 7 2019, 17:15

I'd suggest building a proper sanitation function that escape char properly as this only is susceptible to collisions.

cmake/modules/AddCompilerFlags.cmake
17 ↗(On Diff #7613)

Using a function and/or macro that take the language as argument would go a long way toward reducing code duplication.

Add a variable sanitizer, factorize flag checking, differentiate between c and cxx

Add newline at end of file

Fabien planned changes to this revision.Mar 8 2019, 14:02

Fix conflict of ':' and '=' chars with the cache format

Fabien planned changes to this revision.Mar 8 2019, 15:19

',' char appears to screw the cache as well, need some rework.

cmake/modules/SanitizeHelper.cmake
4 ↗(On Diff #7657)

Take a prefix.

Jump back to characters replacement instead of escaping.
Escaping the chars just won't work as soon as the variable is cached.
Also add a prefix argument to the sanitizing macro.

deadalnix added inline comments.
cmake/modules/SanitizeHelper.cmake
18 ↗(On Diff #7670)

Empty lines are entirely free. I recommend you use some of those.

This revision is now accepted and ready to land.Mar 12 2019, 20:10
cmake/modules/SanitizeHelper.cmake
17 ↗(On Diff #7670)

You can do it all in one op:

string(REGEX REPLACE "([^a-zA-Z0-9/_.+-])" "_" ${SANITIZED_VAR} "${PREFIX}${RAW_VAR}")

Add some empty lines, merge replacement and prefixing in a single operation

This revision was automatically updated to reflect the committed changes.