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

C++23BuildFixes #121

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Conversation

ecbadeaux
Copy link
Contributor

@ecbadeaux ecbadeaux commented Jan 8, 2025

Intro:

This is a fix that ensures your project can successfully build with C++23 should you choose to upgrade. While exploring the project, I noticed that switching to a newer C++ version caused build failures. This pull request addresses those issues, allowing you to safely bump your CMAKE_CXX_STANDARD to 23 if desired.

Technical Changes:

Here is a screenshot of the build errors I was encountering.

C++23BuildErrors

You can easily reproduce them in the docker container with the following changes I have in my branch BuildFailuresC++23.

After inspecting the build errors, I was able to see that you were attempting to overload the stream insertion operators for the classes Tag, Id, and Measurement. In each of these classes, you were defining the implementation for the stream operator, which inherently would call the templated fmt::format specialization for each class. The issue is that the definition for each stream insertion would attempt to call the format specialization before it had been defined. In order to get around this error, you need to define the template before defining the stream insertion because of their dependencies.

I also removed the unneeded friend qualifier for the Id stream operator.

After making the changes above, I was able to get the project to build with C++23 🎉.

I've created a branch here that has an additional dummy unit test to ensure C++23 features are now working. This C++23 feature is only available in gcc-14, but, nevertheless, the project will now build with certain C++23 features if supported by the compiler version.

I did not bump the CMAKE_CXX_STANDARD, but if you would like me to, let me know 😄

@copperlight
Copy link
Collaborator

copperlight commented Jan 8, 2025

This is actually really helpful! I had been interested in bumping from C++17 to C++20 or C++23, but I kept running into build issues, which are related to how we were using fmt templates.

Internally, we still need to build on some old OSes - bionic and centos7. If we can get gcc-14 installed after-market on those platforms, then we can bump the CMAKE_CXX_STANDARD. Since we specify -static-libstdc++, we are protected from changing libstdc++ versions in the binaries when we run on these older OSes.

Taking a look this morning!

@ecbadeaux
Copy link
Contributor Author

ecbadeaux commented Jan 8, 2025

Glad I could help!

This pull request is safe to go ahead and merge if you want. We could circle back to upgrading the CXX version at your discretion.

I can look into making a script to get gcc14 working for those systems if you want. Just share the output for 'etc/os-release' on those systems

@copperlight copperlight merged commit 8986259 into Netflix-Skunkworks:main Jan 9, 2025
1 check passed
@ecbadeaux ecbadeaux deleted the C++23BuildFixes branch January 11, 2025 00:56
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.

2 participants