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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Mar 6 2019, 09:27
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 6 2019, 09:27
Herald added a subscriber: schancel. · View Herald Transcript
Fabien updated this revision to Diff 7613.Mar 6 2019, 09:36

Slightly simplify the first regex in build_test_name_from_flag

jasonbcox accepted this revision.Mar 6 2019, 22:25
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.

deadalnix added inline comments.Mar 7 2019, 17:17
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.

Fabien updated this revision to Diff 7650.Mar 8 2019, 12:04

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

Fabien edited the test plan for this revision. (Show Details)Mar 8 2019, 12:04
Fabien updated this revision to Diff 7651.Mar 8 2019, 12:06

Add newline at end of file

Fabien planned changes to this revision.Mar 8 2019, 14:02
Fabien updated this revision to Diff 7657.Mar 8 2019, 14:34

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.

deadalnix added inline comments.Mar 8 2019, 21:35
cmake/modules/SanitizeHelper.cmake
4 ↗(On Diff #7657)

Take a prefix.

Fabien updated this revision to Diff 7670.Mar 11 2019, 13:06

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 accepted this revision.Mar 12 2019, 20:10
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
deadalnix added inline comments.Mar 12 2019, 20:12
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}")
Fabien updated this revision to Diff 7693.Mar 13 2019, 11:40

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

This revision was automatically updated to reflect the committed changes.