Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Enable use of C++20 for compatibility with poppler 24.05 #961

Merged
merged 1 commit into from
May 14, 2024

Conversation

Vekhir
Copy link
Contributor

@Vekhir Vekhir commented May 13, 2024

Poppler uses std::string::starts_with since poppler/poppler@fbb645. This commit (fbb645) is included in poppler 24.05. This change also affects the public headers of GooString, which means that all projects depending on poppler need to be aware of std::string::starts_with, even if they don't use it themselves.

std::string::starts_with is new with C++ 20 (see above), which means that OpenBoard must also be compiled with C++ 20. Otherwise, this error occurs:

/usr/include/poppler/goo/GooString.h:241:24: error: ‘starts_with’ has not been declared in ‘std::string’
  241 |     using std::string::starts_with;
      |                        ^~~~~~~~~~~

Possible solutions

  1. Make CMAKE_CXX_STANDARD a cache variable. This makes it easier to override for the Arch package (and in the future), while additional support burden is low. Does make the build system more complex.
  2. Change CMAKE_CXX_STANDARD depending on version of poppler. I haven't found a way to cleanly achieve this. Might mean that devs have to test with different versions of poppler.
  3. Simply bump the CMAKE_CXX_STANDARD to 20 from 17. While C++20 is considered experimental within GCC, GCC 11 is probably fine. Still means that some distributions would have to be dropped.

Solution presented in this PR

Solution 3 is the most straightforward fix, so this is chosen here.
Edit: Changed to solution 1 since dropping support for older distros like Ubuntu 20.04 is undesirable. It's also more flexible considering that this situation already occurred with the transition to C++17.

Conclusion

OpenBoard will eventually have to move on to C++ 20. Support for that is mostly complete in GCC, but still considered experimental. Poppler already requires it to build starting with 24.05, which afaics is only packaged with Arch Linux at this stage. Naturally, this will change in the future.

Fixes #958

Suggestions and feedback are always welcome!
-- Vekhir

@letsfindaway
Copy link
Collaborator

Accepting this PR would break builds for several other distro versions:

  • Ubuntu 20.04 is still a maintained version
  • The default gcc version for openSUSE Leap 15 is 7.5.0, even if newer versions are also available. It would however mean that additional effort is needed to use a newer gcc version for builds for Leap 15.5

If your proposed alternative solution 1 (cache variable) is feasible, then I would very much prefer this, as it does not affect or break other builds.

@Vekhir
Copy link
Contributor Author

Vekhir commented May 13, 2024

I thought as much, though I didn't expect Leap to still default to 7.5. But doesn't matter.

As for solution 1 - which I think is the next best solution - looks probably something like this:

set(CMAKE_CXX_STANDARD 17 CACHE STRING "C++ standard to use - defaults to C++17")

@Vekhir Vekhir changed the title fix: Use C++20 for compatibility with poppler 24.05 fix: Enable use of C++20 for compatibility with poppler 24.05 May 13, 2024
poppler 24.05 exposes std::string::starts_with in its headers
which requires C++20. Requiring C++20 means dropping support
for still maintained distributions. As such, the C++ standard
defaults to the current C++17, but can be overridden where
necessary.

Emit a status message showing the chosen C++ standard for debug
purposes.
@Vekhir
Copy link
Contributor Author

Vekhir commented May 13, 2024

@letsfindaway I've pushed the update. Tested with C++17, C++20, and default (C++17).

@letsfindaway
Copy link
Collaborator

I would like to clarify the problem #958 (comment) before merging.

@Vekhir
Copy link
Contributor Author

Vekhir commented May 14, 2024

With #962 merged, I believe this PR can proceed.

@letsfindaway Any thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants