Fix or nix Android CI builds

[d-frey]

Currently, the Andriod builds are broken due to using std::filesystem::path. On some non-Android builds this was fixed by adding -llibstdc++fs, on some builds it just worked. Android, however, seems to be too old in some cases and I wouldn't know how to add the above option (in case it is required and helps). Also, the version we are using now seem to be a bit outdated.

We therefore need to either fix (and possibly modernize the build) or I will have to remove them (and most likely Android is then broken).

Any help is appreciated, @Bjoe ;)

[wravery]

Relevant discussion in an NDK issue: android/ndk#609

It looks like support was added fairly recently in NDK r22.

[ColinH]

Do you have experience with the NDK? We would greatly appreciate a PR that would fix things again.

[Bjoe]

@d-frey Sorry for the late response... something bad happened to my relatives in August... anyways

I'm surprise why the the android toolchain is using libstdc++ ... it is not anymore updated by the NDK. PEGTL should use libc++ instead.
Give me some day, I will create a PR to fix it.

[Bjoe]

@ColinH @d-frey As @wravery mentioned. std::filesystem is supported at NDK r22. At the moment NDK r21.3.6528147 is released. So r22 is a future release.

See also NDK issue: android/ndk/issues/1272…
See NDK roadmap: https://android.googlesource.com/platform/ndk/+/master/docs/Roadmap.md#c_file-system-api

NDK r21 is not yet fully support C++17 features.

android/ndk/issues/1265 is also still open.

I'm not sure if -llibstdc++fs will help, because the NDK is using libc++ not the libstdc++ ....

[d-frey]

@Bjoe We appreciate your help with our little project, but of course people are more important than machines, so please take all the time you need to help your family - the PEGTL can wait. I am looking forward to see you again in person in the C++ user group once the pandemic is hopefully dealt with in spring/summer next year. For now, I'll disable the CI jobs for Android. Once r22 becomes available, we can try again to create a new CI job for it. Thanks again for your support!

[Bjoe]

@d-frey Thank you for condolence ... it's three months ago so we are getting back to normal...

I found some more information about the NDK r22 release. So there exists a NDK "22.0.6917172-beta1" version.
There are also a changlog for this version.

I used this version locally to build PEGTL for android and it compiles ! :-)

I try now to add this beta version in travis .. see https://travis-ci.org/github/Bjoe/PEGTL/builds/744958133

It will help you to test if PEGTL is compiling for android. It won't help your "customer" as NDK r22 is not yet released.

To introduce -llibstdc++fs in the NDK, I do not recommend this.
If you really will have "now" a solution for your "customer", then I purpose to use boost::filesystem. This works 100% on android. I already use boost::filesystem on android. But this means, your project is depended on a third party lib, not sure if you like it.

If recommended following: Use the NDK r22 "Beta" in travis, so you know that PEGTL will compile on android (I'm already on it). Add a hint for the android users that your project can compile/use on android when NDK r22 is released (or if they like use the beta NDK r22). I think it will not take a long time if NDK r22 is released.

[d-frey]

I agree that we should use the new r22 instead of wacky work-arounds like -lstdc++fs and I think it is reasonable to just point out to users that this is the minimum version that is supported for Android with the PEGTL 3.x branch. People on older Android systems will have to use the 2.x branch. I'm looking forward for the PR, thanks again for helping out with this!

[Bjoe]

@d-frey I created a proposal. See PR #233

[ColinH]

@Bjoe Thanks a lot, greatly appreciated!

[Frank-Buss]

I have the same problem when using cmake on Linux Debian, it doesn't work with GCC 8.3, compiling stops with an error complaining about undefined reference to std::filesystem::__cxx11::path::_M_split_cmpts()'. Some examples needs it as well, e.g. the lua example compiles only like this: g++ -lstdc++fs -std=c++17 -I ../../../include lua53_parse.cpp -lstdc++fs`

[wravery]

The dependency on std::filesystem also requires a newer toolchain on many Linux distributions. I know that gcc-7 in Ubuntu 16.04 is too old to support it because I had to update several CI pipelines to use a newer Ubuntu agent in my project. I suspect you need at least gcc-9 (hopefully not a whole distro upgrade) to get it, or you need to link with -lstdc++fs on gcc-8 (but that didn't work for Android).

[wravery]

@Bjoe, @d-frey, @ColinH, how does adding a boost::filesystem fallback sound, like @Bjoe suggested as a "now" solution? It seems like there are quite a few potential users of PEGTL who are still using older versions of gcc which didn't have support for std::filesystem yet.

If that sounds good I can put together a PR which switches namespaces/libraries based on a CMake config setting. That's the approach I used in the past for https://github.com/microsoft/cppgraphqlgen, and it worked pretty well. It brings in a Boost dependency, but only for users who opt in because they have an older compiler.

For some reason that's not one of the language features you can query in CMake, so I just used if(!MSVC) before, but if any of you know a better way to test this in the CMake configuration let me know.

[wravery]

Answered my own question, I've never used it before but it looks like you can use try_compile to see if a test file builds with the current toolchain. If it can't build a source file with #include <filesystem> then it could emit a warning/error that the user must set the config option or upgrade their toolchain. If it doesn't link, it could retry with -lstdc++fs and automatically add that to the public target_link_options for PEGTL if that succeeds.

[d-frey]

I'm unsure about this. We require a feature from C++17 and it's almost 2021. It's not like we are requiring bleeding-edge stuff, but I don't want to support old stuff forever. THB, I always wonder why platforms like Android don't do a better job in supporting newer standards. Such a big platform has a scale effect on how much time and effort it can safe for all their users.

For the PEGTL, currently we have the 2.x branch without std::filesystem (and which has been actively maintained) and the new 3.x branch (aka the main branch) with std::filesystem.

Would it be OK to point Android users to the 2.x branch for the time being and only support r22 and newer for the 3.x main branch? I understand that the situation on Android is unfortunate, but I'd think that this will (hopefully) solve itself over time as newer Android versions do support std::filesystem (as Jörg already tried out with r22). Or do Android users (which I'm not one of) feel that this is too much of a drawback and r22 will take too long to become the standard?

[d-frey]

I have the same problem when using cmake on Linux Debian, it doesn't work with GCC 8.3, compiling stops with an error complaining about undefined reference to std::filesystem::__cxx11::path::_M_split_cmpts()'. Some examples needs it as well, e.g. the lua example compiles only like this: g++ -lstdc++fs -std=c++17 -I ../../../include lua53_parse.cpp -lstdc++fs`

Is there a simple way to automatically detect GCC 8 and add the required flag in CMakeLists.txt? A PR would be appreciated.

[d-frey]

BTW: If anyone has experience with GitHub Actions and would like to create a PR to move our existing CI tests on TravisCI and AppVeyor to GitHub Actions (and provide the same features/coverage), I would be more than happy to accept that PR 😄

[wravery]

I'm unsure about this. We require a feature from C++17 and it's almost 2021. It's not like we are requiring bleeding-edge stuff, but I don't want to support old stuff forever. THB, I always wonder why platforms like Android don't do a better job in supporting newer standards. Such a big platform has a scale effect on how much time and effort it can safe for all their users.

I totally agree, I don't know why std::filesystem was left out of so many toolchains for so long, particularly when they say they have C++17 support. But there are still a lot of those toolchains in use, besides Android this affects several n-1 LTS Linux distributions and macOS 10.15 as well.

Or do Android users (which I'm not one of) feel that this is too much of a drawback and r22 will take too long to become the standard?

For selfish reasons I'd also like to enable them for the pegtl vcpkg port since vcpkg maintains support for a wider range of toolchains. So even if each of these targets has a newer toolchain which works (including the pre-release r22 NDK for Android), that still cuts off support for those targets downstream in the cppgraphqlgen port.

Is there a simple way to automatically detect GCC 8 and add the required flag in CMakeLists.txt? A PR would be appreciated.

Not as simple as I'd like, but I think so. I would use a try_compile test in CMake to see if it links without that library and then automatically add it if that is what's needed to build the test. That at least unblocks the n-1 LTS Linux distros which are on gcc-8, but it wouldn't fully unblock the vcpkg port I'm working on since those still target gcc-7 (and macOS 10.15 apparently).

[ColinH]

So at the lowest level there would be a macro/define that switches from the default std::filesystem to boost::filesystem which can be manually added to the Makefile by the user, and some CMake magic to autodetect when this is necessary for those that use CMake? That doesn't sound too bad.

[d-frey]

The real question is: Is this enough to make the new PEGTL work with GCC 7? There might be other issues, so you might want to try it first and see wether it makes sense to spend more time on a PR. Also, if possible,the PR should then include a new CI job to make sure that it works and we don't accidentally break it in the future.

[Frank-Buss]

Maybe this helps:
https://github.com/quick-lint/quick-lint-js/blob/fefda8a49d19b02d3838ad25fcb2c4cf06490111/cmake/QuickLintJSCompiler.cmake#L40
I can compile the quicklint project on my Debian system without problems, looks like it does some tests to make it portable for many platforms and compilers. I don't have experience with cmake, so I can't help with this, but shouldn't be difficult to use the parts from the quicklint project for fixing this problem.

[d-frey]

Closing as #233 has been merged. Remaining work can still be discussed or separate PRs can be created, but the main issue is now solved. Thanks again @Bjoe :)