Adjust how TriBITS handles compiler and linker options according to modern CMake best practices

[bartlettroscoe]

CC: @keitat

Part of modernizing TriBITS and CMake build systems based on TriBITS is the handling of compiler and linker options. This story is to probe the issues at hand and some of the driving requirements from Kokkos and Trilinos.

A comment that come from #299 (comment) was "change variables to lists instead of strings". In "Professional CMake: 5th Edition", it says:

If multiple compiler flags need to be set, they need to be specified as a single string, not as a list.

So what are all of the issues here?

Tasks:

• Discuss the issues involved and pin down the expected behavior of modern CMake projects and best practices
  => CMake projects should use target_compile_options( ... <PUBLIC|INTERFACE> ...) sparingly and likely only Kokkos will do this.
• Determine refactoring targets for TriBITS and impacts on Trilinos and Kokkos integration
  => For now, TriBITS will not add any special cache variables for accepting compiler flags that will be propagated to downstream customers and will just stick with CMAKE_<LANG>_FLAGS (see below)

[bartlettroscoe]

CC: @masterleinad, @keitat, @fnrizzi, @crtrott

@bradking, @zackgalbreath,

I know Kokkos made the change to handle compiler flags as a list of arguments due to @jjwilke's work but where did that change in CMake come from and where is that documented?

Note that in "Professional CMake: 5th Edition" it says:

If multiple compiler flags need to be set, they need to be specified as a single string, not as a list.

so when did it change that CMake wants you to handle compiler and linker flags as a list of arguments instead of a single string?

There is no hit of how you are supposed to handle this at:

• https://cmake.org/cmake/help/v3.17/variable/CMAKE_LANG_FLAGS.html#variable:CMAKE_%3CLANG%3E_FLAGS

I can't find handling compiler options as lists anywhere. (On the other hand, handling arguments to add_test() as a list has been the case for a long time.)

[bradking]

Some interfaces take lists of canonical options, others take flags as command-line string fragments. In general, settings with _FLAGS in their name take command-line string fragments, and settings with _OPTIONS in their name take lists. Many settings document which they take.

CMake MR 5915 added documentation that CMAKE_<LANG>_FLAGS is a command-line string fragment. This has always been the case, but now it is documented.

The add_compile_options and target_compile_options commands and associated infrastructure use ;-separated lists of options.

[crtrott]

So @bradking partly the problem is I think we add stuff via add_compile_options which has quotes in the flags we need to pass to the compiler, and something then went wrong at the export for the Trilinos install. But I understand you that lists are the correct way to go for add_compile_options right?

[bartlettroscoe]

@bradking

CMake MR 5915 added documentation that CMAKE_<LANG>_FLAGS is a command-line string fragment. This has always been the case, but now it is documented.

Okay, I see that in 'git-master' at:

• https://cmake.org/cmake/help/git-master/variable/CMAKE_LANG_FLAGS.html#variable:CMAKE_%3CLANG%3E_FLAGS

which says:

This value is a command-line string fragment. Therefore, multiple options should be separated by spaces, and options with spaces should be quoted.

so this will be in CMake 3.21+ documentation. Thanks for pointing that out.

The add_compile_options and target_compile_options commands and associated infrastructure use ;-separated lists of options.

With modern CMake, everything should be target based, right? So you should only use target_compile_option() and then link targets to proporate the correct compiler flags. With that, the exported targets in the <Package>Config.cmake files will have those compiler options associated with them. Then, when those targets are imported with find_package(<Package>) and the targets are linked to with target_link_libraries(MyLib <Package>::all_libs).

Okay but how is CMAKE_<LANG>_FLAGS supposed to interact with all of this? What if a user sets some critical flags with CMAKE_CXX_FLAGS when they configure and install Kokkos that should be pulled into Trilinos (through KokkosConfig.cmake) that is configured and installed and then passed to on to customers of Trilinos (through TrilinosConfigure.cmake), how is this supposed to work?

[bartlettroscoe]

But I understand you that lists are the correct way to go for add_compile_options right?

@bradking

But how does add_compile_options() interact with CMAKE_<LANG>_FLAGS? Does one overwrite the other? Does one come after the other? I don't see that documented anywhere. How is a user (on the CMake command-line) supposed to append/override compiler options in a consistent way with modern CMake projects without editing CMakeLists.txt files?

[bradking]

lists are the correct way to go for add_compile_options right?

Yes. CMake will quote/escape things for you. Same for target_compile_options.

But how does add_compile_options() interact with CMAKE__FLAGS?

Both are used. The CMAKE_<LANG>_FLAGS typically come first, much like a $CC $CFLAGS ... convention.

What if a user sets some critical flags with CMAKE_CXX_FLAGS

If the user adds ABI-changing flags to CMAKE_<LANG>_FLAGS, they are responsible for making the downstream consumers compatible. Don't try to force such flags on consumers in <PackageName>Config.cmake files.

How is a user (on the CMake command-line) supposed to append/override compiler options in a consistent way with modern CMake projects without editing CMakeLists.txt files?

If the project wants to add optional/overridable flags, it should provide its own cache settings that affect how it adds them.

[crtrott]

So we actually discussed this recently. CMAKE_CXX_FLAGS set during Kokkos install are NOT propagated to downstream folks right now (when using Kokkos outside of Trilinos). We considered telling users to set Kokkos_CXX_FLAGS for stuff which should be propagated, and we would attach those to add_compile_options.

[crtrott]

Ah @bradking: what I just wrote there is essentially the recommended way of handling this?

[bradking]

what I just wrote there is essentially the recommended way of handling this?

Yes, though I'd be interested in an example of what flags you might propagate to consumers, e.g. via target_compile_options(MyLib PUBLIC ...).

[crtrott]

we propagate all kinds of stuff. In particular a lot of architecture specific and backend specific flags. Like all the "compile this for GPU" flags, "use these optimization flags", "enable specific options" stuff.
Like --expt-extended-lambda, --arch=sm_70, -rdc, -Xcompiler -qsmp=omp,, -fopenmp=libomp -Wno-openmp-mapping, -march=armv8.2-a+sve -msve-vector-bits=512. You get the idea.

Its all here somewhere: https://github.com/kokkos/kokkos/tree/master/cmake

[bartlettroscoe]

We considered telling users to set Kokkos_CXX_FLAGS for stuff which should be propagated, and we would attach those to add_compile_options.

@crtrott, @bradking, so it would be considered recommended/good practice for a modern CMake project to allow users to pass compiler options on the CMake command-line as:

• CMAKE_<LANG>_FLAGS: Only used to build the local project targets. (These will not get propagated to downstream customers through <Project>Config.cmake.)

• <Project>_<LANG>_OPTIONS: Custom CMake ';' separated list var of compiler options that will be added be added to the base project add_compiler_options() and therefore will be propagated to downstream customers through <Project>Config.cmake.

?

Is that a reasonable convention/standard?

NOTE: If you don't define <Project>_<LANG>_OPTIONS as a ';' separated list, then you have to write specialized CMake code like here to make the translation from a string of compiler options to a ';' separated list of options (but in the opposite direction in this case). I think it would be nice to eliminate this type of tricky code future TriBITS and other CMake code if possible. (Or can we trust separate_arguments() to be super robust for this purpose and can therefore change this to <Project>_<LANG>_FLAGS which would make it easier for existing Trilinos users to transition their configure scripts for Trilinos to propagate compiler options.)

[bradking]

@bartlettroscoe that may work, but I think only experience will tell for sure. Note that add_compile_options only adds directory-level settings that apply to targets in that directory, but do not propagate as usage requirements. Use target_compile_options to add options that propagate.

The separate_arguments UNIX_COMMAND mode is used by the target_compile_options's SHELL: syntax which can be used to group options in ;-separated lists. It should be robust enough to convert <Project>_<LANG>_FLAGS command-line string fragments if that is a better user interface for other reasons.

FWIW, I generally try to avoid forcing flags on consumers, even through INTERFACE_COMPILE_OPTIONS:

• They often need to be guarded by generator expressions like COMPILE_LANG_AND_ID to make sure they apply only to appropriate compilers.
• On compilers for which they do not apply, we cannot rely on those options for important things like ABI anyway.
• Custom flags make poor usage requirements because CMake does not know semantics of how different settings combine. If multiple projects all try to force flags this way, ordering and conflict problems arise.

The last bullet is somewhat mitigated by target-centric APIs because only consuming targets that link to the relevant targets get those requirements. IIUC, Kokkos's purpose is in part to provide these flags.

[bartlettroscoe]

@bradking

Note that add_compile_options() only adds directory-level settings that apply to targets in that directory, but do not propagate as usage requirements

Okay, I think I see how this works. The command add_compile_options() adds (appends?) options to the directory-level property list variable COMPILE_OPTIONS which provides the default value for the target-level property COMPILE_OPTIONS and calls to target_compile_options() with PRIVATE|PUBLIC append more options to the target-level property COMPILE_OPTIONS and these options in COMPILE_OPTIONS are only used to build the target; they are not propagated to downstream targets in the same project (through target_link_libraries()) or exported to external customers through exported/imported targets in generated <Package>Config.cmake files. Is that correct?

So the only way to get compiler options to propagate to downstream targets is to use target_compile_options( ... <INTERFACE|PUBLIC> <options_list>) which sets the target-level property INTERFACE_COMPILE_OPTIONS?

If this is the case, then I guess you have to define <Project>_<LANG>_FLAGS as:

• <Project>_<LANG>_FLAGS: A string of compiler flags that will be separated into a list using separate_arguments() that will be passed to target_link_library( ... PUBLIC ) and will therefore be applied to every target in <Project> and every downstream customer in exported targets in the installed <Project>Config.cmake file.

(You would allow this to be specified at the TriBITS package level as well with <Package>_<LANG>_FLAGS.)

But to apply such options consistently and call target_link_library( ... PUBLIC ) for every target in the project (or link to a dummy target that has the right compiler options set), you need a set of wrapper functions to create targets or people will screw it up if they use raw CMake commands. (But tribits_add_library() and tribits_add_executable() can do that.)

The CMAKE_<LANG>_FLAGS typically come first

The word "typically" worries me here. Is it documented (and is it pinned down with automated tests in the CMake test suite) what order all of these are these compiler flags are listed when a given source file is actually compiled? That is actually very important in some cases (as some flags can override others). The set of compiler flags that I see here are:

• CMAKE_<LANG>_FLAGS
• CMAKE_<LANG>_FLAGS_<BUILDTYPE>
• COMPILE_OPTIONS target property
• INTERFACE_COMPILE_OPTIONS target properties from one or more upstream targets (where each target can have a different set of flags)

So what is the order? I could do some experiments to try to find out but it would be better if the documentation just explicitly spelled this out. And then there is the question of how can the user append arbitrary compiler options to override whatever is set in the internal CMakeLists.txt files in case they need to? For TriBITS, I provided CMAKE_<LANG>_FLAGS_<CMAKE_BUILD_TYPE>_OVERRIDE but it sounds like that may not work now using target properties COMPILE_OPTIONS and INTERFACE_COMPILE_OPTIONS.

FWIW, I generally try to avoid forcing flags on consumers

Note that the current <Project>Config.cmake and <Package>Config.cmake files generated by TriBITS don't actually force compiler flags/options on downstream users. Instead, those files provide variables called <Package>_<LANG>_COMPILER_FLAGS and the downstream customer has to decide to pull in those flags or not. It is just that with the way that Kokkos works, you have to have some of those compiler flags or you don't even get correct code. But I think in this TriBITS/Trilinos build system modernization, we can apply just the critical Kokkos system-specific flags that @crtrott mentions above to the INTERFACE_COMPILER_OPTIONS target properties in Trilinos and then any extra compiler flags set through CMAKE_<LANG>_FLAGS can be left as is.

Actually, the refactoring in #299 will technically break backward compatibility of Trilinos in that targets exported/imported from Trilinos will have the Kokkos system-specific flags set in their INTERFACE_COMPILE_OPTIONS properties and customers will get them by just linking to the targets even without setting set(CMAKE_CXX_FLAGS "${Trilinos_CXX_COMPILER_FLAGS} ${CMAKE_CXX_FLAGS}").

On compilers for which they do not apply

Note that for deeply templated C++ code is usually very dangerous to mix and match compilers and even different compiler versions. The only safe way to build a stack of C++ software that uses the C++ standard library is to use the exact same C++ compiler for everything from the bare metal on up. (We are lucky if we get a large amount of C++ code to build and work when using even a single C++ compiler :-))

Custom flags make poor usage requirements because CMake does not know semantics of how different settings combine.

In general, I agree. A good library (even a C++ library) should be usable without any compiler flags (not even -D<defines> as those should be provided in configured and installed header files). But I think Kokkos is really a special case where were you don't even get correct code in some cases if you don't get the very specialized set of compiler flags that the Kokkos build system gives you. And you will not get performant code if you don't get the right compiler options so in the case of Kokkos, it is really critical that that subset of compiler options get propagated automatically and correctly (and the updated TriBITS and Trilinos just needs to not get in the way of that).

[crtrott]

IIUC, Kokkos's purpose is in part to provide these flags.

Yes. I mean its impossible for normal users to figure out which flags are needed on all these different platforms to get correct code. In some of these cases (like for SYCL) we figured these flags out only with direct input from the compiler developers. It is a pretty iffy situation, made worse by the fact that all these new compilers by default identify as clang even though they might not even accept all normal clang flags.

[bradking]

So the only way to get compiler options to propagate to downstream targets is to use target_compile_options( ... <INTERFACE|PUBLIC> <options_list>) which sets the target-level property INTERFACE_COMPILE_OPTIONS?

Yes.

<Project>_<LANG>_FLAGS: ... passed to target_link_library( ... PUBLIC ) and will therefore be applied to every target

After thinking about that approach more, there may be some problems:

• <Project>_<LANG>_FLAGS is specific to <LANG>, but target_compile_options needs generator expressions to express per-language options. You'll need to add those expressions as part of implementing support.

• Adding the flags to every target individually will cause them all to have the flags in INTERFACE_COMPILE_OPTIONS, which will lead to a lot of redundancy and reliance on CMake's de-duplication behavior. Instead one could have some kind of base interface library target responsible for adding these flags, and link all targets to that.

• The separate_arguments approach will not be able to express grouped options like -option A -option B. Parsing that as -option;A;-option;B will cause de-duplication to produce -option;A;B which will be generated as -option A B. target_compile_options offers a SHELL:... syntax for grouping options. It might be best to offer the ;-list syntax directly in an _OPTIONS setting so that users can choose to use the SHELL:... syntax to group options.

I think Kokkos is really a special case

In that case, why offer a general-purpose <Project>_* setting at all? Kokkos can hard-code target_compile_options calls.

how can the user append arbitrary compiler options to override whatever is set in the internal CMakeLists.txt

CMake has no model for that. The project code has final control. If it wants to offer control to the user, the project code must do that. In particular, the project should offer settings to avoid getting the flags in the first place.

The word "typically" worries me here.

It is true for Ninja and Makefile generators. On generators like Visual Studio, we don't actually control the specific order of the command line. MSBuild does that. We map from the command-line flags to .vcxproj file property settings, which MSBuild converts back into flags, which might be in a different order. However, when mapping to those properties, we do process flags in the semantic order (CMAKE_<LANG>_FLAGS first) so that conflicting settings resolve in the same order the command-line generators would. The result is that the .vcxproj properties are set to the "final" variant of each flag seen, and only that flag will be added by MSBuild.

Is it documented (and is it pinned down with automated tests in the CMake test suite) what order all of these are these compiler flags are listed when a given source file is actually compiled?

I opened CMake MR 6187 to formally document and test the order.

[bartlettroscoe]

CC: @keitat, @crtrott, @masterleinad

I think Kokkos is really a special case

In that case, why offer a general-purpose <Project>_* setting at all? Kokkos can hard-code target_compile_options() calls.

@bradking, that is a good point. With the can of worms that de-duplication and ordering of compiler options opens up as described above, I think it is best that TriBITS and Trilinos (and other TriBITS-base projects) don't even bother provide a general way to set compiler options that get forwarded to downstream customers automatically.

Note that to maintain backward compatibility, the TriBITS-generated <Project>Config.cmake and <Package>Config.cmake files will still need to contain the variables <Project>_<LANG>_COMPILER_FLAGS and <Package>_<LANG>_COMPILER_FLAGS, respectively. But downstream customers can choose to use or ignore those flags as always so there is no harm in leaving those there. (But after #299, the critical Kokkos compiler options will be propagated automatically through INTERFACE_COMPILE_OPTIONS.)

So going forward, the refactoring of TriBITS and Trilinos will just focus on making sure that any INTERFACE_COMPILE_OPTIONS target properties get propagated properly and that is it.

It might be best to offer the ;-list syntax directly in an _OPTIONS setting so that users can choose to use the SHELL:... syntax to group options.

If, in the future, we find the need to support compiler options that get automatically forwarded to downstream customers, then we will use the ;-separated list <Project>_<LANG>_OPTIONS form (so users can use the SHELL:... syntax to be robust with the de-duplication of options). But for now, we will not bother to avoid the headaches associated with all of this.

I opened CMake MR 6187 to formally document and test the order.

Thanks for that!

In conclusion, I don't think there is anything more that needs to be done here in this Issue #375. We will just focus on doing #299 correctly (i.e. propagating everything correctly between targets with exported/imported targets in <Package>Config.cmake files and target_link_libraries() which includes the INTERFACE_COMPILE_OPTIONS target properties if set).

Thanks for the discussion!