xtensor 0.27 requires C++ 20

[KrisThielemans]

From the xtensor README:

Up to version 0.26.0, xtensor requires a C++ compiler supporting C++14. xtensor 0.26.x requires a C++ compiler supporting C++17. xtensor 0.27.x requires a C++ compiler supporting C++20.

Seems we need to cope with this similarly to #262, although I think it's really a xtensor issue

[KrisThielemans]

@naegelejd not sure if you can release a new yardl version now, but potentially you fix this one as well. I'd rather have a release soon though, as we can easily work-around this one.

[naegelejd]

Happy to look into this, but we have not run into any issues with xtensor 0.27.1. We build and test Yardl and MRD with both C++17 and C++20. If you have an error I can reproduce I will investigate.

[KrisThielemans]

Interesting. This was reported by @NikEfth in ETSInitiative/PETSIRD#181 (comment). That PR's CI is working fine with C++-20. I've now reverted it to C++17 and get https://github.com/ETSInitiative/PETSIRD/actions/runs/23446821536/job/68214200730?pr=181#step:9:53

/usr/share/miniconda/envs/yardl/include/xtensor/utils/xutils.hpp:590:5: error: 'concept' does not name a type; did you mean 'const'?
  590 |     concept has_data_interface_concept = has_data_interface<E>::value;

which doesn't surprise me as the xtensor doc says they need C++20 from 0.27.

[KrisThielemans]

hmmm. I think xtensor does this for us:
https://github.com/xtensor-stack/xtensor/blob/18f6524829d8ac6399374f9ecbd21b959f75424d/CMakeLists.txt#L209
So, now think this is because @NikEfth tried to eliminate explicit xtensor dependencies in https://github.com/NikEfth/PETSIRD/blob/f5cc9edc50c1b4767c7b7af77111df576b0c82fd/cpp/helpers/CMakeLists.txt#L27-L36

I think you can ignore this for a bit therefore. Hopefully, i can just close this.

[naegelejd]

Hah I just debugged this and found the same thing. xtensor 0.27.1 INTERFACE_COMPILE_FEATURES sets cxx_std_20, so I have never seen the error you posted.

This also means that our C++17 tests are not actually testing against C++17 ever since we bumped xtensor to 0.27.1.

I don't have a real solution yet... We could

• Add a new test configuration for (C++17, xtensor<0.27) separate from the current (C++20, xtensor>=0.27)
• Bump the minimum Yardl C++ version to 20
• Remove xtensor entirely, it's not a hard dependency but it's convenient for a NumPy-like array API

[KrisThielemans]

We could

• Add a new test configuration for (C++17, xtensor<0.27) separate from the current (C++20, xtensor>=0.27)

could be a good idea

• Bump the minimum Yardl C++ version to 20

Not sure why you'd do that. Yardl doesn't need it, xtensor takes care of it. everyone who uses CMake properly will be fine. Possibly, you could add a warning in the doc somewhere

• Remove xtensor entirely, it's not a hard dependency but it's convenient for a NumPy-like array API

That'd break some stuff on our side, so that's a bit more of a long term plan I hope :-) Could be worth doing (at least optionally), as xtensor does seem to be moving fast and relying on a code in "alpha" state seems dangerous.

[KrisThielemans]

From my perspective, this can be closed as it's working fine, unless you want to close it by adding a new test case.

[naegelejd]

I definitely can't close this until we add a new test configuration for C++17. But in the meantime, if you're using CMake properly you shouldn't have any issues, correct.