[ECM] use a macro to add compiler flags conditionally
Needs ReviewPublic

Authored by rjvbb on Nov 15 2018, 12:08 PM.

Details

Reviewers
kfunk
Group Reviewers
Build System
Summary

It is crucial not to add unsupported compiler options because they can cause CMake's check_<lang>_compiler_flags macro to fail, causing problems like https://bugs.kde.org/show_bug.cgi?id=401018.
To prevent this, run an actual test when using clang on Mac.

This is almost unavoidable because determining compiler flag availability from compiler ID + version can be tricky to near impossible on Mac. Apple's llvm version numbers that are ahead of stock llvm versions and not easy to match. In addition, not all cmake versions distinguish between stock clang and "AppleClang" and those that do need policy CMP0025 set to NEW. That policy cannot be set after the project statement in the toplevel cmake file (= not in an ECM module).

This patch introduces a set of macros to simplify writing reliable conditional code for adding compiler flags. If will do the usual compiler ID + version checking to determine if a flag should be supported; when using Clang on Mac it will in addition use check_<lang>_compiler_flag(). This should be safe because Apple's llvm versions have always been ahead of stock versions; false negatives should thus never occur.
The C++ version of the new macro is used where possible.

BUG: 401018

Test Plan

See https://bugs.kde.org/show_bug.cgi?id=401018 ; without this patch the project fails to build using Xcode compiler versions because the generated kcddb_export.h header does not match the visibility settings.

The modified code works as expected on Linux and on Mac with stock Clang (from MacPorts, for instance).

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 7840
Build 7858: arc lint + arc unit
rjvbb created this revision.Nov 15 2018, 12:08 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 15 2018, 12:08 PM
Restricted Application added a subscriber: kde-buildsystem. · View Herald Transcript
rjvbb requested review of this revision.Nov 15 2018, 12:08 PM
apol added a subscriber: apol.Nov 15 2018, 1:03 PM
apol added inline comments.
kde-modules/KDECompilerSettings.cmake
195

Why can't we just use check_cxx_compiler_flag?

203

Why have a macro that only works on Apple?

rjvbb added inline comments.Nov 15 2018, 1:39 PM
kde-modules/KDECompilerSettings.cmake
195

We could of course. I worked under the assumption that is not done currently because checking only ID+version is faster. CMake's check_cxx_compiler_flag is more expensive because it invokes the compiler.

And it can fail, for instance if the user adds an option for which the compiler emits diagnostics output. That will be true for the "clang on Mac" situation too, but why generalise brittleness if you don't have to?

203

? This is a macro that encapsulates a Mac-specific peculiarity but otherwise should work everywhere. Did I miss something?!

NB: ${_RESULT} is not going to be set if COMPILER_OK isn't set first, meaning the macro should always "return false" when a flag isn't supported, even if check_cxx_compiler_flag isn't used.

rjvbb updated this revision to Diff 45567.Nov 16 2018, 9:46 AM

A simpler version, setting CMAKE_<LANG>_FLAGS directly (also fixes a persistence error in my previous implementation).

I've thought some more about "why not just use check_<lang>_compiler_flag everywhere". I think that the risk with using a built-in function is that future developers or maintainers may think to be smart and replace them with built-in ID+version checking. Because we know since what version which compiler supports a given flag, right? Providing a dedicated function should reduce that risk, if not only because of the comments it comes with.

Ideally there should probably also be a version that accepts multiple flags; would that be possible with cmake's language?

rjvbb retitled this revision from [ECM] use a macro to test compiler flag support to [ECM] use a macro to add compiler flags conditionally.Nov 16 2018, 9:47 AM
rjvbb set the repository for this revision to R240 Extra CMake Modules.
rjvbb edited the test plan for this revision. (Show Details)
kfunk requested changes to this revision.Nov 16 2018, 11:37 AM
kfunk added a subscriber: kfunk.

I don't like the hiding of the if-branches as argument to macro. We shouldn't to this as it makes the code harder to understand.

The general issue with the existing code here is: Statements like CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND NOT CMAKE_CXX_COMPILER_VERSION VERSION_LESS 3.5 do not make sense. If we check the compiler version, then we need to differentiate between Clang and AppleClang (cf. something like https://github.com/Microsoft/LightGBM/blob/master/CMakeLists.txt#L20).

Thus these places need to be turned into:

...
if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS "3.8")
  ...
endif()
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS "8.1.0")
  ...
endif()

If you're unsure in which version of AppleClang a certain feature was introduced then:

...
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS "8.1.0")
  add_compile_flag_if_supported(...)
endif()

Please check out the functions provided in: kdevelop.git:cmake/modules/KDevelopMacrosInternal.cmake

#   add_compile_flag_if_supported(<flag> [CXX_ONLY])
#   add_target_compile_flag_if_supported(<target> <INTERFACE|PUBLIC|PRIVATE> <flag>)

They are more clean than your implementation, I think it would make sense to actually add them to KDECompilerSettings.cmake and prefix them with kde_.

kde-modules/KDECompilerSettings.cmake
202

This "let's abuse ${ARGN} as code to be evaluated later" is pretty ugly. The previous version (with the if-statements at the caller side) is way more clean and understandable.

203

I don't really understand why this branch is needed. The macro name name suggests it does the compile check on all compilers. Again very misleading.

This revision now requires changes to proceed.Nov 16 2018, 11:37 AM
Thus these places need to be turned into:

  ...
  if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS "3.8")
  elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS "8.1.0")

As you know this doesn't work reliably: whether it works or not depends on cmake version and on CMP0025 - and thus requires all projects to set that policy before calling project. I don't think that's an option, nor is expecting Mac users to patch all toplevel CMake files of projects they want to work with (or patch CMake so it sets the policy which is basically what I do on my end). I also vaguely recall that cmake used the dotless AppleClang version in earlier versions (= 810 instead of 8.1.0).

Besides, all those repeated separate ifs and elses do not make the code easier to read or parse. Isn't it the whole purpose of having macros to reduce code duplication and to hide complexity in a single location so that the intended behaviour becomes easier to follow?

This makes Aleix's suggestion just to use compiler<lang>_check_flag much more appealing, despite the cost and the fact it's so easy to break.

Statements like CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND NOT CMAKE_CXX_COMPILER_VERSION VERSION_LESS 3.5 do not make sense.

That's a bit too strong. They make sense everywhere except on APPLE (and even there they can be reliable).

If you're unsure in which version of AppleClang a certain feature was introduced then:

This is the gist of what I'm doing, except that I do not want to rely on Apple clang being reported as AppleClang (= CMP0025 set to NEW).

This "let's abuse ${ARGN} as code to be evaluated later" is pretty ugly.

It comes from a cmake ML post by a cmake dev (Brad Kind IIRC) so I guess it's sanctioned ugliness (as so much in cmake syntax). The boolean operators are only available in the IF functions so there aren't much options if you don't want to use CmakeParseArguments. I'm already looking at doing just that for a version that will look like kde_add_supported_compiler_flags(FLAGS <flags> SUPPORTED_IF <conditions>). The names (macro and keywords) are open for discussion of course, and so is the addition of additional options.

I'll look at kdevelop's versions too. I have no real objection to favour my own code over them if they work reliably. But now that you mention kdevelop: I've long been forced to use stock compilers to build KDevelop (on Mac). I always put that off to my AppleClang being too old but with this new knowledge I suspect it's simply because something goes wrong in the compiler detection.

I don't really understand why this branch is needed. The macro name name suggests it does the compile check on all compilers. Again very misleading.

Erm, no?! The macro name suggests it runs a check, which is true. It doesn't say which check...
(FWIW, cmake_<lang>_compiler_check_flag is equally misleading: it doesn't check ${flag} but "${CMAKE_CXX_FLAGS} ${flag}")

rjvbb added a comment.Nov 16 2018, 2:48 PM

Something side-ways related: I went down this hole because cmake's generate_export_header failed because of an unsupported flag that was added.
Regardless of how we implement things here, shouldn't there be something like ecm_generate_export_header which empties CMAKE_CXX_FLAGS temporarily because calling CMake's version and then restores the variable? There's no feedback at all in this function, the generated export header just contains dummy EXPORT macros, leaving the user to wonder why the linker fails. Or should the visibility flags also be set conditionally, after setting all other compiler options?

  1. add_compile_flag_if_supported(<flag> [CXX_ONLY])

So that is basically just a wrapper around compile_check_cxx_compiler_flag with a bit of icing (consistent cache variables, which my approach also has, and a (cripped!) C++-only option).
Issues I see:

1- these will fail if someone adds an unsupported flag for which the compiler emits a warning, potentially breaking a build that would otherwise succeed. You could argue "just don't add unsupported options then". That may still be acceptable for individual projects, but I think it's not a good idea nonetheless. Build systems (and thus the ECM) should be as fool-proof as possible, and using ID+version checks can help here.
2- (citing): "The macro name name suggests it does the compile check on all compilers."
3- this assumes that the C++ compiler accepts all options accepted by all compilers (without emitting warnings), which is not true in practice

Here's a counter proposal that should be clear in its intention and implementation (clear though a bit long and complex because covering all {b,c}ases):

include(CMakeParseArguments)

# ECM_ADD_CXX_COMPILER_FLAGS_IF_SUPPORTED(FLAGS <flag|flags>
#     [SUPPORTED_IF <conditional expression>]
# )
# add <flag> or <flags> to CMAKE_CXX_FLAGS is the compiler supports them.
# Support is determined by the SUPPORTED_IF expression if provided or by
# querying the compiler directly otherwise. The compiler is also queried
# when using a Clang C++ compiler on APPLE.
# examples:
# Check 
# ecm_add_cxx_compiler_flags_if_supported(FLAGS -a -b -c SUPPORTED_IF CMAKE_C_COMPILER_ID STREQUAL "GNU" OR CMAKE_C_COMPILER_ID MATCHES "Clang")
# ecm_add_cxx_compiler_flags_if_supported(FLAGS -d -e -f)

function (ECM_ADD_CXX_COMPILER_FLAGS_IF_SUPPORTED)
    set(_OPTIONS_ARGS)
    set(_ONE_VALUE_ARGS)
    set(_MULTI_VALUE_ARGS FLAGS SUPPORTED_IF)

    cmake_parse_arguments(EASCXXFLAGS "${_OPTIONS_ARGS}" "${_ONE_VALUE_ARGS}" "${_MULTI_VALUE_ARGS}" ${ARGN} )
    if(NOT EASCXXFLAGS_FLAGS)
        message( FATAL_ERROR "ecm_add_cxx_compiler_flags_if_supported: 'FLAGS' is a required argument." )
    endif()
    # if the user provided supported_if conditions, evaluate them now:
    if (NOT EASCXXFLAGS_SUPPORTED_IF OR (${EASCXXFLAGS_SUPPORTED_IF}))
        # without conditions, or with Clang on APPLE we'll need to ask the compiler directly.
        # (Clang on APPLE needs verification because of Apple's llvm versions which cannot be
        # (matched easily to stock llvm versions.
        if(NOT EASCXXFLAGS_SUPPORTED_IF OR (APPLE AND CMAKE_CXX_COMPILER_ID MATCHES "Clang"))
            # one flag at a time:
            foreach(flag IN ITEMS ${EASCXXFLAGS_FLAGS})
                # use a standardised and informative cached test variable
                set(HASFLAG "${CMAKE_CXX_COMPILER_ID}++_ACCEPTS${flag}")
                check_cxx_compiler_flag(${flag} ${HASFLAG})
                if ({${HASFLAG}})
                    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${flag}" PARENT_SCOPE)
                endif()
            endforeach()
        else()
            # all flags can be appended at once
            string(REPLACE ";" " " FLAGS "${EASCXXFLAGS_FLAGS}")
            set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${FLAGS}" PARENT_SCOPE)
        endif()
    endif()
endfunction()

Using a function here to keep temp. variables local. And using the ECM "namespace" because there's nothing KDE specific in this.

rjvbb updated this revision to Diff 45786.Nov 19 2018, 9:50 AM

This implements and uses my idea of an ecm_add_<lang>_compiler_flags_if_supported function set for C and C++. It uses compiler ID+version conditions to determine if flag(s) are supported when those conditions are known and reliable - otherwise and only then does it resort to querying the compiler.

I do not particularly care whether or not the KDE*CompilerSettings modules use the SUPPORTED_IF feature but I think it should be there as a courtesy to devs who have reasons to avoid querying the compiler (and for compilers where CMake's querying algorithm doesn't work).

rjvbb set the repository for this revision to R240 Extra CMake Modules.Nov 19 2018, 9:51 AM
dfaure added a subscriber: dfaure.Jan 27 2019, 6:59 PM

This makes sense to me. Just the name "SUPPORTED_IF" is strange, when reading that, one thinks "well, if we know the compiler flag is supported, why are we testing that it is?". I think this should be something like TRY_IF.
Then it's clearer that no harm will occur if we set a too low compiler version after TRY_IF, it's just an optimization to avoid e.g. testing all gcc flags on MSVC and vice-versa.

This makes sense to me. Just the name "SUPPORTED_IF" is strange, when reading that, one thinks "well, if we know the compiler flag is supported, why are we testing that it is?".

I agree it looks strange, but apparently the combination with the macro name isn't as unambiguous as I hoped.

The idea is: add the given option(s) but only If they are Supported by the compiler ... and they are Supported If the given expression is true.

I think this should be something like TRY_IF.

That reads nicer, but isn't exact either because a priori there will be no more trying if the expression is true...

I'll change the macro name to ECM_ADD_<LANG>_FLAGS_CONDITIONALLY and the parameter name to CONDITION, let's see how that works out.

Then it's clearer that no harm will occur if we set a too low compiler version after TRY_IF, it's just an optimization to avoid e.g. testing all gcc flags on MSVC and vice-versa.

Uhm? That was not the intention. Currently if a TRY_IF condition is present then it is the only criterium used, UNLESS the compiler is Clang/AppleClang on APPLE. In that latter case the compiler is queried; this is also the case when no conditional expression is specified.

I can of course inverse that logic, and make the condition a filter for when to query the compiler. At first sight I'd say that the conditional expressions won't need adapting but I have a feeling it might not be as simple.

rjvbb updated this revision to Diff 50399.Jan 27 2019, 10:14 PM

Renamed macro and parameter names as announced in my last comment.

rjvbb set the repository for this revision to R240 Extra CMake Modules.Jan 27 2019, 10:15 PM

Ah. You meant an "OR", I thought it was an "AND". (as in our known selection of compilers OR/AND appleclang supports it)
But things are certainly not clearer now with the name CONDITION, which doesn't imply either one.

Aside from the AppleClang issue, I've had cases where a compiler flag was added in an unreleased version of the compiler, so it makes sense to me to have a way to have a TRY_IF condition.
At the same time, I guess we can have SUPPORTED_IF for the cases where we know it's supported and we want to save time by not even checking it.
Then one could say "TRY_IF apple and clang" for the flags that we want to double-check on appleclang, instead of the function having this black magic hardcoded inside.
And this way we don't need to check flags that work on all versions of clang like -pedantic.

In summary, I suggest having both SUPPORTED_IF and TRY_IF, and moving AppleClang magic out of the function.

rjvbb added a comment.Jan 28 2019, 9:38 AM

Usually if you have a conditional behaviour the associated condition specifies when to trigger it, no?
You're right that the names don't suggest exactly how the condition is being evaluated (with extra checks or not), but that was also a bit the idea.
Don't bother the user with such details, just provide a macro that will add the flag(s) if they are supported, with an optional conditional expression that can make things faster.

Not that this optimisation makes a huge difference in my experience, so the already suggested simplification of just querying the compiler for everything isn't completely off the table either AFAIAC.

In summary, I suggest having both SUPPORTED_IF and TRY_IF, and moving AppleClang magic out of the function.

So:

SUPPORTED_IF : add the flag(s) if the expression is true?
TRY_IF: query the compiler if the expression is true? How would that intersect with the SUPPORTED_IF test?

What about the macro name itself in that case?

What bothers me here is that it puts the responsability to invoke magic or not back in the hands of the users, most of whom cannot test (directly) on the platform where this is needed. That could well open the door to new issues, either when a new problematic flag is added, or if someone thinks that some cleanup is in order and safe.

Counter proposal: add a SUPPORTED_BY or SUPPORTING_COMPILERS parameter that take a list of compiler IDs for flags like -pedantic that are supported by all versions and can thus be added with only a simple compiler check. The question then is if an error should be raised if both parameters are specified?

rjvbb updated this revision to Diff 50451.Jan 28 2019, 9:24 PM

This follows David's suggestion, but using QUERY_IF instead of the suggested TRY_IF to make it clear that this parameter controls the querying of the compiler.
I haven't yet tested the new logic exhaustively but the as far as I can tell the macro behaves as intended as used in the two compiler settings modules.

I've simplified the QUERY_IF conditions in those modules to just "if APPLE". Querying systematically on APPLE seems reasonable because the likelihood that a non-clang compiler is used for building is very small and when it happens we'd probably be dealing with a non-GNU compiler.

rjvbb set the repository for this revision to R240 Extra CMake Modules.Jan 28 2019, 9:25 PM

SUPPORTED_IF : add the flag(s) if the expression is true?

Yes.

TRY_IF: query the compiler if the expression is true?

Yes.

How would that intersect with the SUPPORTED_IF test?

In addition.

AFAICS this is what you implemented, so I like it. I just find SUPPORTED_IF (as an intro to the condition) more readable than IF_SUPPORTED.

The macro name seems fine to me.
In plain English one would say "Add this flag if it's supported. It's supported for sure if condition A is true. Also try it if condition B is true, since it might work then, but we can't be sure".
In that sentence, one can read "if supported" for the macro name, "SUPPORTED IF" for the first condition, and "TRY IF" for the second condition.

In that sentence, one can read "if supported" for the macro name, ...

That was my idea too, and the reason the macro ends in "_if_supported".

PS: don't forget the unit test for the new module.

cgiboudeaux added inline comments.Jan 30 2019, 9:48 AM
modules/ECMAddCompilerFlag.cmake
2–26

Shall be moved after the module documentation

28–30

Shall be moved after the license block

31–59

Sphinx cannot parse this doc.

Also don't forget to create ECMAddCompilerFlag.rst in docs/modules/

59

Missing "Since 5.xx"

62

Unneeded extra space

67

Unneeded extra space

69

Unneeded extra spaces

97

Unneeded extra space

102

Unneeded extra space

104

Unneeded extra space

In that sentence, one can read "if supported" for the macro name, ...

That was my idea too, and the reason the macro ends in "_if_supported".

Right, but I was saying all this because I think IF_SUPPORTED (the keyword in the arguments) should be SUPPORTED_IF.

Right, but I was saying all this because I think IF_SUPPORTED (the keyword in the arguments) should be SUPPORTED_IF.

Doh?! I thought that was the case, maybe I just restored the wrong way after the CONDITION experiment.

Christophe: can you please be more specific why sphinx fails? Also, is there an existing unittest I can clone and adapt?

rjvbb marked 9 inline comments as done.Jan 31 2019, 10:30 AM
rjvbb added inline comments.
modules/ECMAddCompilerFlag.cmake
2–26

I'm guessing you meant "Please move [...]" and not that this would somehow happen automagically(?)

rjvbb updated this revision to Diff 50589.Jan 31 2019, 10:34 AM
rjvbb marked an inline comment as done.

Updated as requested.

rjvbb set the repository for this revision to R240 Extra CMake Modules.Jan 31 2019, 10:34 AM

Christophe: can you please be more specific why sphinx fails? Also, is there an existing unittest I can clone and adapt?

There are tests for other ECM modules in the tests subdir.

modules/ECMAddCompilerFlag.cmake
2–26

Indeed.

There are tests for other ECM modules in the **tests** subdir.

That's not the expected answer, so let me rephrase: which existing test can I clone and adapt (which is about the only thing I know how to do in this domain)?

I'm going to need a hand here if not only because the only kind of testing that makes sense to me is checking manually with a selection of the compilers you happen to have installed. Anything automatic I can think of would not be able to do much more than taking the resulting CMAKE_??_FLAGS variable and check if the compiler indeed accepts all the arguments. That's basically just repeating the same tests already performed in the macro you're supposed to be testing and thus as much (if not more) a test of the input logic (the conditional expression(s)) as of the macro.

There are tests for other ECM modules in the **tests** subdir.

That's not the expected answer, so let me rephrase: which existing test can I clone and adapt (which is about the only thing I know how to do in this domain)?

And the answer is the same, there are examples in the tests/ folder.

I'm going to need a hand here if not only because the only kind of testing that makes sense to me is checking manually with a selection of the compilers you happen to have installed. Anything automatic I can think of would not be able to do much more than taking the resulting CMAKE_??_FLAGS variable and check if the compiler indeed accepts all the arguments. That's basically just repeating the same tests already performed in the macro you're supposed to be testing and thus as much (if not more) a test of the input logic (the conditional expression(s)) as of the macro.

The macro has different parameters.

Things you can test:

  • Compiler flags accepted by all compilers (-Wall, -Wextra...)
  • flags only accepted by a given compiler. You can then check that it's not added by mistake to unsupported platforms

About the code itself:
The two functions are clones, this can be avoided by only having one ECM_ADD_COMPILER_FLAG function and a LANGUAGE argument.
This has 2 benefits:

  • The module name matches the function name
  • it's shorter than ecm_add_cxx_compiler_flag_if_supported and easier to remember.
modules/ECMAddCompilerFlag.cmake
95

EASCXXFLAGS_SUPPORTED_IF

99

EASCXXFLAGS_SUPPORTED_IF

130

EASCFLAGS_SUPPORTED_IF

134

EASCFLAGS_SUPPORTED_IF

cgiboudeaux added inline comments.Jan 31 2019, 3:12 PM
modules/ECMAddCompilerFlag.cmake
109

if(HASFLAG)

you're testing the result, not the string

cgiboudeaux added inline comments.Jan 31 2019, 3:18 PM
modules/ECMAddCompilerFlag.cmake
109

Forget that. The syntax is confusing, please remove this HASFLAG

rjvbb added a comment.Jan 31 2019, 6:30 PM

Forget that. The syntax is confusing, please remove this HASFLAG

That I'm not going to do. The goal is to both to have useful feedback like below, and to avoid caching issues that would cause on the first query to be performed (if you were to use a single result variable):

-- Performing Test Clang++_ACCEPTS-Wvla
``

I decided against the idea of using a single macro name with a language parameter very early during development. It adds a parameter to each call or extra code to the macro to implement a default value, it can lead to errors that can remain undetected, and it deviates from the usual CMake syntax.

Forget that. The syntax is confusing, please remove this HASFLAG

That I'm not going to do. The goal is to both to have useful feedback like below, and to avoid caching issues that would cause on the first query to be performed (if you were to use a single result variable):

-- Performing Test Clang++_ACCEPTS-Wvla
``

So according to you, this line is useful ? from my point of view, it's needless and just looks like a syntax error.

rjvbb added a comment.Jan 31 2019, 7:15 PM
So according to you, this line is useful ? from my point of view, it's needless and just looks like a syntax error.

Yes, it shows which compiler is being queried and for what. The line is going to be there anyway because CMake's Check*CompilerFlag macros don't have a QUIET option.

We can discuss the exact dynamic variable name but since Cmake will not query the compiler twice for the same return variable the template should contain both the compiler ID and the flag being tested in order to cover all possible uses.
I've considered trying to reset the cached result variable but we probably don't want to lose the benefits of that caching.

rjvbb updated this revision to Diff 50812.Feb 3 2019, 10:39 PM

Now tested more exhaustively and with unittest.

rjvbb set the repository for this revision to R240 Extra CMake Modules.Feb 3 2019, 10:40 PM