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

Proposal: Use C++20 #7203

Merged
merged 2 commits into from
Feb 1, 2023
Merged

Proposal: Use C++20 #7203

merged 2 commits into from
Feb 1, 2023

Conversation

graham33
Copy link
Contributor

Would there be an objection to using C++20 for the Nix codebase? We are working on a future patch (related to S3) that might benefit from C++ 20, so thought it was worth raising a PR to upgrade to see if it's feasible. I'm not sure what the constraints are for compatibility in various environments.

@graham33 graham33 marked this pull request as ready for review October 22, 2022 14:17
Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

We're already using some C++20 features in practice (designated initializers is the first that comes to my mind but I think there's a couple other things). So if something requires C++20, I don't see any objection towards fully making the switch.

Deferring to @edolstra or the rest of the maintainers though for a final call

@edolstra
Copy link
Member

The compilers we're currently using don't support all of C++20. So we can only use some features like designated initializers.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-38/22805/1

@thufschmitt
Copy link
Member

@edolstra which compilers are you refeering to? The darwin builds used to have a very old clang, but it is now using clang 11 Afaik (which technically doesn't support the whole of c++20, but close-enough). Is there anything else blocking us there?

@edolstra
Copy link
Member

I was mostly thinking about gcc, but it looks like the support for C++20 in gcc/libstdc++ 11 is pretty good now:

https://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html
https://gcc.gnu.org/projects/cxx-status.html#cxx20

@graham33
Copy link
Contributor Author

graham33 commented Nov 1, 2022

Yes I think gcc 11's support for C++20 is pretty good. See also https://en.cppreference.com/w/cpp/compiler_support

@edolstra edolstra merged commit 14b0b9e into NixOS:master Feb 1, 2023
@edolstra
Copy link
Member

edolstra commented Feb 1, 2023

Thanks, I just needed this for std::span on the lazy-trees branch!

@roberth
Copy link
Member

roberth commented Feb 1, 2023

Looks like CI doesn't run on a an up to date merge commit, as this merge has broken master.

Bors could help us prevent this.

@edolstra
Copy link
Member

edolstra commented Feb 1, 2023

Weird, I did test a version of this rebased on master.

@roberth
Copy link
Member

roberth commented Feb 1, 2023

Maybe it only happens on clang.
The build failure came from the darwin job (but the linux one was cancelled because of that fwiw).

@edolstra
Copy link
Member

edolstra commented Feb 1, 2023

It also fails on aarch64-linux, though with a different error:

g++: error: unrecognized command line option '-std=c++20'; did you mean '-std=c++2a'?

Looks like we're using an older version of gcc (9.3.0) on aarch64-linux... 😖 According to NixOS/nixpkgs/00a0eeb2cb9f63b42dbc15d4a15b5d2a403852f5 this is because of "unresolved issues".

@roberth
Copy link
Member

roberth commented Feb 1, 2023

Not compatible with boehmgc, it seems. NixOS/nixpkgs#174282 (comment) I misread.

That's rather non-optional. Maybe we're not quite ready for C++20 then. Might c++2a be a viable solution, despite being insufficiently descriptive? Otherwise we'll have to revert this. That may actually be the best option until the boehmgc compatibility is resolved.

@roberth
Copy link
Member

roberth commented Feb 1, 2023

There's a tracking issue

@edolstra
Copy link
Member

edolstra commented Feb 1, 2023

Or we use gcc11Stdenv on aarch64-linux.

It would be rather sad to revert this because aarch64-linux is stuck indefinitely on an old version of gcc. That cannot be a maintainable situation for nixpkgs either. It kind of defeats the purpose of a standard environment if stdenv on some architectures has a different version of gcc...

@roberth
Copy link
Member

roberth commented Feb 1, 2023

@roberth
Copy link
Member

roberth commented Feb 1, 2023

Or we use gcc11Stdenv on aarch64-linux.

I wonder what bugs we'd be getting ourselves into, and isn't this risky to combine with non-gcc-11 packages?

purpose of a standard environment

I don't think that's the reason for the name, but I agree.

@roberth
Copy link
Member

roberth commented Feb 1, 2023

I'm afraid that this upgrade will send us down a very counterproductive rabbit hole. Is there a reasonable alternative to std::span you could use?

@edolstra edolstra mentioned this pull request Feb 1, 2023
7 tasks
@edolstra
Copy link
Member

edolstra commented Feb 1, 2023

Of course, but being stuck on an outdated version of gcc is not a viable long-term situation.

@tpwrules
Copy link
Contributor

tpwrules commented Feb 2, 2023

This might be a more relevant tracking/goal issue: NixOS/nixpkgs#208412

TL;DR: We have a solution to upgrade GCC on aarch64-linux for now (upgrade the bootstrap tarball) and intend to have it in by NixOS 23.05. But it doesn't fix the underlying problems and is likely to cause the version to get stuck again. We are putting in significant effort on more comprehensive solutions so we can fix the issue for good but the issues are quite non-trivial.

My vote would be to revert this until a solution is merged into nixpkgs. I would worry about mixing the GCC version of such a fundamental package, and there is the possibility of rare and hard to detect bugs being induced with GCC 11 (which is why it hasn't been upgraded yet).

@vcunat
Copy link
Member

vcunat commented Feb 2, 2023

The mixing sounds problematic just for packages that link against nix libs. They will probably be forced to use the same compiler version (because C++ ABI).

cole-h added a commit to cole-h/nix that referenced this pull request Feb 8, 2023
This reverts commit 14b0b9e, reversing
changes made to 0079d29.
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.

7 participants