Support back to GCC 7

[multiplemonomials]

I'm really loving this library so far, but it can be a bit troublesome to install because it requires GCC 8.1 or newer, which isn't readily available on all distros/machines. This PR adds support for back to GCC 7.1 and possibly even older versions (don't have a machine readily available to test).

The biggest change was related to std::filesystem. This PR allows CMake to find the old libstdc++ std::experimental::filesystem implementation, and introduces a new generated header (matplot/std_filesystem.h) to automatically include the correct library. std::experimental::filesystem is mostly compatible with std::filesystem, but it's missing the path::lexically_proximate() function and a few others. So, the matplot library builds fine but I had to disable generate_examples for gcc 7.x.

There were also a few other changes, mainly dealing with a constexpr related bug and helping the compiler out with some types it couldn't deduce.

With these changes, the library builds and works fine on GCC 7.1, land it has never gone before! Hope this is useful.

[lacc97]

Why remove this bit?

[lacc97]

Why this change?

[lacc97]

Some of the changes to source/matplot/CMakeLists.txt suggest you've been working on an older version of the file. I've marked a couple of those in comments.

Other than that it looks good.

[multiplemonomials]

Oh yeah that is odd, it definitely isn't intentional.

[multiplemonomials]

Oh dang, I figured out what happens. Turns out I copied a CMake file from a different source folder I cloned a few days ago. Anyway, should be fixed now.

[alandefreitas]

Interesting. That's very useful to make it easier to integrate. These changes to make the library compatible with C++14 or older GCCs are always welcome. At least for a while, to make the library more convenient to build and install. We should do that as much as possible.

A few small questions I had looking at the changes:

• Is this moving back to C++14 or to the portion of C++17 GCC7 supports?
• Have you tested the installers and the cpack packages built with GCC7? CI already tests if the packages are being created correctly but not if they are being imported correctly.
• Can't we test the string_view bug directly in the build script rather than assuming it happens if and only if if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU" AND ${CMAKE_CXX_COMPILER_VERSION} VERSION_LESS 7.3)? I find these implicit conditionals a little dangerous. There are so many environments, so many compilers, and so many patches. The test condition might not apply to all of them. I think it does. But it might not.
• Can't we put all that filesystem logic inside FindFilesystem.cmake? Just setting booleans with the information from STD_FS_INCLUDE and STD_FS_NAMESPACE seem to be enough. There's not much information in std_filesystem.h and it makes the installer more complicated that it has to. For instance, the target FindFilesystem.cmake creates can directly include as a public include directory, so that we can directly use #include in both cases. The STD_FS_NAMESPACE we can use directly as a private compile definition in the few source files that depend on filesystem (only one .cpp source file IIRRC). It seems like this would save just as much trouble without the necessity to reformulate and test the installers and packages all over again.

Besides these small points, the PR looks great. However, I think it's a little dangerous to make this compatibility with C++14 or GCC7 a promise rather than a convenience so that people don't ever have to update GCC yet. Making this a promise would increase maintenance costs and would require us to keep testing the library with other compilers on all other operating systems and reformulate the installers to support older or alternative versions of , and whatever libraries we need from C++17. If this becomes a promise, the cost might be much higher than the benefits of this convenience of not having to update GCC.

[multiplemonomials]

Is this moving back to C++14 or to the portion of C++17 GCC7 supports?

Heck no, I'm not trying to move the code back to C++14. GCC7 supports like 90% of C++17, I'm just trying to patch up the 10% that isn't supported.

Have you tested the installers and the cpack packages built with GCC7? CI already tests if the packages are being created correctly but not if they are being imported correctly.

No I haven't. Is there a procedure written up somewhere to run the packaging process? Is it just make package?

Can't we test the string_view bug directly in the build script

Yeah I can set that up. It's a bit of extra work though.

Can't we put all that filesystem logic inside FindFilesystem.cmake?

I suppose we can, I kinda assumed it was a 3rd party module and didn't want to mess with it. However, we do need to handle the fact that std_filesystem.h needs to be installed along with the rest of the library headers, unless you know you won't ever need to use std::filesystem from a public header. Since it needed to be installed I thought the code made more sense in the buildscript

I think it's a little dangerous to make this compatibility with C++14 or GCC7 a promise rather than a convenience so that people don't ever have to update GCC yet.

I was afraid you'd say that... doesn't bode well for my chances of merging this PR :P. I've worked on projects with a couple strategies for dealing with this. You could specify in the readme that matplotplusplus requires a fully C++17 compliant compiler and provide minimum version of GCC, Clang, etc, potentially with an exception for GCC 7.x. You could also set up a CI build that tests whether the code compiles with the oldest supported versions of common compilers. This is useful to keep other changes from unintentionally breaking compatibility. Alternately, you could announce that there is no support for compilers older than GCC 8 etc, but maybe still accept patches like this if people are providing them. Basically, I think that depending on such a new language standard (using the C++ world definition of "new" at least), adds some extra requirements for users of the library, and it's good to have a policy about which compilers will work and which aren't supported.

[alandefreitas]

Is this moving back to C++14 or to the portion of C++17 GCC7 supports?

Heck no, I'm not trying to move the code back to C++14. GCC7 supports like 90% of C++17, I'm just trying to patch up the 10% that isn't supported.

Great. I just wanted to make sure I understand your intention. No worries.

Have you tested the installers and the cpack packages built with GCC7? CI already tests if the packages are being created correctly but not if they are being imported correctly.

No I haven't. Is there a procedure written up somewhere to run the packaging process? Is it just make package?

Everytime we push or make a pull request, the CI script already generates all the packages and uploads them as artifacts. For now, I usually install them locally in a virtual machine whenever I make a big change in the installer to make sure they are OK. Mostly to make sure the default directories are still ok and that I'm able to find_package(Matplot++).

This is not a perfect solution and this test could be part of the CI script, but there so many possible paths that depend on the OS that it's a little though to automate all that. I can test the new packages here if you need help with that. I have these virtual machines set up locally so that I don't depend on github actions to warn me about every little problem in the code.

But it's one of the reasons why I avoid changing the install / package scripts too much unless I have to. There are so many systems out there. People are always creating issues because the package is now behaving in a different way somewhere.

Can't we test the string_view bug directly in the build script

Yeah I can set that up. It's a bit of extra work though.

You can do that with CheckCXXSourceRuns. It shouldn't take more than 5 lines of code. I can help you with that if you want.

Can't we put all that filesystem logic inside FindFilesystem.cmake?

I suppose we can, I kinda assumed it was a 3rd party module and didn't want to mess with it. However, we do need to handle the fact that std_filesystem.h needs to be installed along with the rest of the library headers, unless you know you won't ever need to use std::filesystem from a public header. Since it needed to be installed I thought the code made more sense in the buildscript

I see. That's OK. There's no problem in adding a few more flags to FindFilesystem.cmake. Actually, FindFilesystem.cmake might be already generating some of these flags you need. We probably just have to use them. But there are no plans to use <filesystem> in a public header in the near future. Even if there were plans to do that, it's possible to adapt Matplot++Config.cmake to deal with that, because the installer already includes FindFilesystem.cmake. In any case, I hope we won't depend too much on this workaround in 1 or 2 years.

I think it's a little dangerous to make this compatibility with C++14 or GCC7 a promise rather than a convenience so that people don't ever have to update GCC yet.

I was afraid you'd say that... doesn't bode well for my chances of merging this PR :P.

Don't worry about that. Your PR is definitely being merged. :D It's a great proposal.

I've worked on projects with a couple strategies for dealing with this. You could specify in the readme that matplotplusplus requires a fully C++17 compliant compiler and provide minimum version of GCC, Clang, etc, potentially with an exception for GCC 7.x.

Exactly. That's what I was thinking. We can conservatively assume people need C++17 (because they do) but silently support the incomplete C++17 that GCC7 supports, in case people don't have access to GCC8 yet. As we are not fully supporting C++14, that seems easier than writing about all these ifs and elses in the documentation. It might confuse people more than help. Many people here use it for scientific computing so they're not used to dealing with that kind of stuff.

You could also set up a CI build that tests whether the code compiles with the oldest supported versions of common compilers.

Sure. We can just update build.yml to stop testing it with GCC8. It will go to GCC7 by default. We just need to update this line

matplotplusplus/.github/workflows/build.yml

Line 52 in 5c9fa68

cmake_extra_args: "-DCMAKE_C_COMPILER=/usr/bin/gcc-8 -DCMAKE_CXX_COMPILER=/usr/bin/g++-8 -DCMAKE_CXX_FLAGS=\"-O2\"",

to cmake_extra_args: "-DCMAKE_CXX_FLAGS=\"-O2\"", or cmake_extra_args: "-DCMAKE_C_COMPILER=/usr/bin/gcc-7 -DCMAKE_CXX_COMPILER=/usr/bin/g++-7 -DCMAKE_CXX_FLAGS=\"-O2\"",.

This is useful to keep other changes from unintentionally breaking compatibility. Alternately, you could announce that there is no support for compilers older than GCC 8 etc, but maybe still accept patches like this if people are providing them.

Exactly.

Basically, I think that depending on such a new language standard (using the C++ world definition of "new" at least), adds some extra requirements for users of the library, and it's good to have a policy about which compilers will work and which aren't supported.

Exactly.

Btw, was there a specific reason to remove the BUILD_WITH_PEDANTIC_WARNINGS option? This is very useful while developing because we can catch errors in our systems before sending the code to CI or testing in a slower virtual machine. Of course we could use pass -Wall -Wextra -pedantic -Werror or /W4 /WX to CMake but passing -DBUILD_WITH_PEDANTIC_WARNINGS=ON is much more convenient. It's an optional flag and it's OFF for users anyway.

[alandefreitas]

This seems to be stale and most changes are already implemented in the current master branch. Let me know if you want me to reopen that.

[multiplemonomials]

Yeah sorry I got a bit busy and didn't have time to come back to this.