[Cmake] allow disabling the c++11 constraint in check_support

[tdavidcl]

Implement the fix proposed in #320

[tdavidcl]

gotta love github CI

Fetched 43.0 MB in 3s (16.9 MB/s)
E: Failed to fetch http://ports.ubuntu.com/ubuntu-ports/pool/universe/g/gcc-11/gcc-11-base_11.5.0-1ubuntu1%7e24.04_arm64.deb  404  Not Found [IP: 91.189.91.102 80]
E: Failed to fetch http://ports.ubuntu.com/ubuntu-ports/pool/universe/g/gcc-11/libasan6_11.5.0-1ubuntu1%7e24.04_arm64.deb  404  Not Found [IP: 91.189.91.102 80]
E: Failed to fetch http://ports.ubuntu.com/ubuntu-ports/pool/universe/g/gcc-11/libtsan0_11.5.0-1ubuntu1%7e24.04_arm64.deb  404  Not Found [IP: 91.189.91.102 80]
E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?

😅

[jeremy-rifkin]

All good! Github ci has been a bit flakey lately

[jeremy-rifkin]

Lgtm thanks!

[tdavidcl]

I just noticed that there is no apt update in the CI.
I had the same issue in https://github.com/Shamrock-code/Shamrock CI (the project i'm integrating cpptrace in).
After some time whenever a package is updated the old link spuriously returns 404 if you are unlucky...
The only solution I found was to always call apt update before apt install to ensure the latest link which tends to be reliable.

[jeremy-rifkin]

One thought before merging this: CPPTRACE_INHERIT_HOST_PROPERTIES is a pretty broad name and it’s only used for the standard version. Might it be better to just have a CPPTRACE_MINIMUM_STANDARD version that defaults to C++11?

The reason for setting the standard in check_support is just to get the minimum version set to at least C++11 for any old compilers that don’t default to C++11. For supporting adaptivecpp / sycl we’d just want to set it to at least C++17 it sounds like.

Another approach could be to check if the standard is already set and if it’s set to >=11 not overwrite it.

Thoughts?

[tdavidcl]

The reason for setting the standard in check_support is just to get the minimum version set to at least C++11 for any old compilers that don’t default to C++11. For supporting adaptivecpp / sycl we’d just want to set it to at least C++17 it sounds like.

That could work.

set(CMAKE_CXX_STANDARD ${CPPTRACE_MINIMUM_STANDARD})
set(CMAKE_CXX_STANDARD_REQUIRED ON)

Another approach could be to check if the standard is already set and if it’s set to >=11 not overwrite it.

Like this ?

if(NOT DEFINED CMAKE_CXX_STANDARD)
    set(CMAKE_CXX_STANDARD 11)
endif()

set(CMAKE_CXX_STANDARD_REQUIRED ON)

That would work, i just don't know if there are side effect from doing this as this won't be a NFC anymore.

My original reasoning was that cpptrace may want to inherit the CMAKE_CXX_STANDARD set by the host project hence the name CPPTRACE_INHERIT_HOST_PROPERTIES. If we want to ensure c++>=11 for cpptrace we could do this ?

  # Inherit existing CMAKE_CXX_STANDARD automatically
  if(NOT CPPTRACE_INHERIT_HOST_PROPERTIES)
    if(NOT DEFINED CMAKE_CXX_STANDARD)
      set(CMAKE_CXX_STANDARD 11)
      set(CMAKE_CXX_STANDARD_REQUIRED ON)
    endif()
  endif()

What would you prefer ?

[jeremy-rifkin]

Thanks for the reply! I think making this a no-functional change is a good goal. I'd say either the set(CMAKE_CXX_STANDARD ${CPPTRACE_MINIMUM_STANDARD}) approach or what you proposed in the PR sound best to me. How would you feel about changing the name to CPPTRACE_INHERIT_HOST_STANDARD?

[tdavidcl]

I went with the renaming to CPPTRACE_INHERIT_HOST_STANDARD as setting CPPTRACE_MINIMUM_STANDARD to something else than 11 feels weird since cpptrace is c++11.

[jeremy-rifkin]

Looks good to me, thanks so much!