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

Move to c++17 #9913

Merged
merged 2 commits into from
Jan 11, 2021
Merged

Move to c++17 #9913

merged 2 commits into from
Jan 11, 2021

Conversation

omoerbeek
Copy link
Member

Short description

Only compile tested with clang on OpenBSD so far.

Creating a PR so the CI and others can easily use this branch to test.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@omoerbeek
Copy link
Member Author

omoerbeek commented Jan 6, 2021

Old Boost is a hurdle.

@rgacogne
Copy link
Member

rgacogne commented Jan 6, 2021

Do we know which version of Boost fixed C++17 compatibility? Would it make sense to try C++14? I see some errors (in the fuzzers build) related to the removal of auto_ptr in C++17, which is still there in C++14.

@omoerbeek
Copy link
Member Author

Do we know which version of Boost fixed C++17 compatibility? Would it make sense to try C++14? I see some errors (in the fuzzers build) related to the removal of auto_ptr in C++17, which is still there in C++14.

I searched for some explicitl statement about C++17 compatibility, but found none.

I am trying CentOS7 now, which has boost 1.53 but seems to have patched get_pointer.hpp. Lets see how much other stufff I encounter. For Ubuntu 16, (as used by the fuzzer) which has boost 1.57 another workaround might be needed. If it all does not work out, I'll try C++14

@rgacogne
Copy link
Member

rgacogne commented Jan 6, 2021

For Ubuntu 16, (as used by the fuzzer) which has boost 1.57 another workaround might be needed.

OSS-Fuzz intends to move to 20.04 eventually (google/oss-fuzz#3756) but I wouldn't hold my breath.

@omoerbeek
Copy link
Member Author

So Unbuntu 16 as used by the fuzzer is a real problem. I was trying to set up a debug env like the fuzzer uses and found out: while the fuzzer build environment uses clang 12, the newest available clang from ubuntu xenial is 8. And if we use that we hit an issue with the c++ libs not being c++17 level. So getting a debug environment up and running is a hassle. Is there a more direct way to get the same env as the fuzzer uses?

@Habbie
Copy link
Member

Habbie commented Jan 6, 2021

Is there a more direct way to get the same env as the fuzzer uses?

https://github.com/google/oss-fuzz/blob/master/infra/base-images/base-builder/Dockerfile ?

@rgacogne
Copy link
Member

rgacogne commented Jan 6, 2021

@omoerbeek
Copy link
Member Author

omoerbeek commented Jan 6, 2021

Ubuntu xenial is a swamp. It's boost 1.57 is using too much deprecated C++11 stuff that has been removed in C++17. Now going for C++14.

@omoerbeek omoerbeek changed the title Move to c++17 Move to c++14 Jan 6, 2021
@omoerbeek omoerbeek force-pushed the move-to-cxx17 branch 3 times, most recently from e3bdefd to 91c61e8 Compare January 8, 2021 09:14
@omoerbeek
Copy link
Member Author

Back to c++17 now that the fuzzer issue should be fixed

@omoerbeek omoerbeek changed the title Move to c++14 Move to c++17 Jan 8, 2021
@omoerbeek omoerbeek force-pushed the move-to-cxx17 branch 7 times, most recently from 2cb4c80 to 116e860 Compare January 10, 2021 15:01
@omoerbeek omoerbeek marked this pull request as ready for review January 10, 2021 15:52
@omoerbeek
Copy link
Member Author

Both CI and buildbot runs are fine. I needed to add one workaround in the bindbackend code.

Copy link
Member

@Habbie Habbie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small things, looks good otherwise!

builder-support/dockerfiles/Dockerfile.rpmbuild Outdated Show resolved Hide resolved
m4/ax_cxx_compile_stdcxx_14.m4 Outdated Show resolved Hide resolved
modules/bindbackend/bindbackend2.cc Outdated Show resolved Hide resolved
@omoerbeek
Copy link
Member Author

Processed review comments and squashed it.

Co-authored-by: Peter van Dijk <peter.van.dijk@powerdns.com>
@omoerbeek omoerbeek merged commit a56a90c into PowerDNS:master Jan 11, 2021
@omoerbeek omoerbeek deleted the move-to-cxx17 branch January 11, 2021 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants