cmake files working as they should

[Ryder17z]

I just wanted to throw in a "Thanks" here because usually when I encounter cmake files in a project, they throw so many errors that it's not even worth checking the logs.

[ArthurSonzogni]

Thanks for this message! Always happy to get positive feedback.

[mavam]

In general, agreeing that CMake setup is neat. Now the "but" :-)

We struggle with one thing: setting global options. That's an anti-pattern in modern CMake. Typically things a target-based now. Case in point:

  # Play nicely if we are being consumed by another project
  # and use their CMAKE_CXX_STANDARD. Otherwise, fall back to 17
  # C++17 is used. We require fold expression at least.
  if (DEFINED CMAKE_CXX_STANDARD)
    set(CXX_STANDARD ${CMAKE_CXX_STANDARD})
  else()
    set(CXX_STANDARD 17)
  endif()

👆 This requires us to use FTXUI like this:

set(CMAKE_CXX_STANDARD 20)
add_subdirectory(aux/ftxui)
unset(CMAKE_CXX_STANDARD)

Better: use target_compile_features and specify the standard there. Here's an example how we do it:

# Require standard C++20.
target_compile_features(libvast_internal INTERFACE cxx_std_20)

Then we could just write:

add_subdirectory(aux/ftxui)

and be on the happy path.

[ArthurSonzogni]

@mavam This changed recently 3w ago:
#620
+CC @lefticus FYI

Previously, this was only set "locally":

set_target_properties(${library} PROPERTIES
    CXX_STANDARD 17
    CXX_EXTENSIONS OFF
  )

While I agree global option is a legacy anti-pattern from older cmake versions, this is needed here to avoid mixing two different C++ version in between the dependent and the dependency.

Would you have suggestions to reconcile @mavam & @lefticus goals? We want to avoid compiling FTXUI with C++17 and the dependent with C++20.

[dominiklohmann]

Generally, you should not set CMAKE_CXX_STANDARD at all, not even on a per-target basis. Instead, you should use target_compile_features, which allows for setting the minimum a target needs.

[mavam]

👆 Like Dominik said, @ArthurSonzogni I think you can get the same desired outcome using that function.

[ArthurSonzogni]

See this cmake discussion: https://gitlab.kitware.com/cmake/cmake/-/issues/19136
And @lefticus replis too:
#620 (comment)

target_compile_features only specifies a requirement on the library and all of its dependencies. It flows only one way. It means you can still have ftxui on C++17, and the dependent on C++20.

Doing so cause runtime issues that are extremely difficult to Debug.

We would need the dependency to flow in the other direction too: make ftxui to use an upper standards specified by the dependency.

While I agree in general the requirements must be specified by target. There are cases like this where this should really be a global flag, similar to Debug/Release flag.

No sure how we can improve over the current status-co

[mavam]

target_compile_features only specifies a requirement on the library and all of its dependencies. It flows only one way. It means you can still have ftxui on C++17, and the dependent on C++20.

I'd have to defer to @dominiklohmann (our CMake ninja) for how this works, but I would hope that CMake does a multi-pass analysis that takes the maximum of all minima to select the correct -std flag for all TUs in a compatible fashion.

No sure how we can improve over the current status-co

I think the status quo is "not bad" in that it's possible to workaround these (really just minor) things.