Skip to content

Comments

protobuf: build with c++14#255844

Closed
happysalada wants to merge 1 commit intoNixOS:masterfrom
happysalada:protobuf
Closed

protobuf: build with c++14#255844
happysalada wants to merge 1 commit intoNixOS:masterfrom
happysalada:protobuf

Conversation

@happysalada
Copy link
Contributor

@happysalada happysalada commented Sep 18, 2023

Description of changes

the readme explicitely states that protobuf should be built with c++14.
I was wondering if this was a discussion that was made before.
Is there a reason not to ?
should we use c++17 ? I'm not sure if it will have some impacts downstream ?
(I know that if this is merged, it will have to be merged on staging, but I'm opening this PR to discuss the idea).
@acairncross is the one who noticed it.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 1001-2500 This PR causes many rebuilds on Darwin and should most likely target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. labels Sep 18, 2023
Copy link
Contributor

@tobim tobim left a comment

Choose a reason for hiding this comment

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

This is not a good idea because you will also have to build abseil-cpp with c++14, and since that is a propagated input you will also have to build all reverse dependencies that make use of abseil in c++14 mode. Many of those don't even support c++14 any more.

The readme simply makes a poor statement there, it should really say c++14 or later, c++ goes to great lengths to preserve backwards compatibility after all.

@acairncross
Copy link
Contributor

@tobim To clarify, my comment was why (or how, even) we were compiling protobuf without specifying any C++ version at all, making me assume we're using a pretty old C++ version yet somehow able to compile protobuf.

However, I've just looked through the docs of g++ and it turns out gnu++14 is the default C++ version, so in fact we are compiling with C++ 14 after all.

@tobim
Copy link
Contributor

tobim commented Sep 18, 2023

However, I've just looked through the docs of g++ and it turns out gnu++14 is the default C++ version, so in fact we are compiling with C++ 14 after all.

It is actually c++17, to which it was bumped with GCC11: https://www.gnu.org/software/gcc/gcc-11/changes.html.

You can check your own compiler with

❯ g++ --version
g++ (GCC) 12.3.0
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

❯ g++ -dM -E -x c++  /dev/null | grep -F __cplusplus
#define __cplusplus 201703L

@tobim
Copy link
Contributor

tobim commented Sep 18, 2023

Note that the situation is different in when stdenv.cc is clang, because our default clang version is very old (llvm11). Setting c++14 in that situation should be fine, because it is not a downgrade from C++17.

@acairncross
Copy link
Contributor

It is actually c++17, to which it was bumped with GCC11

Ah I see, that clears up quite a few things for me, thanks! (I had been looking at the man pages in a nix-shell which wasn't for the right version of g++.)

@happysalada
Copy link
Contributor Author

@tobim I've made the change you suggested, on darwin stdenv.cc defaults to the old clang, so protobuf would build with c++14 by default only on that platform.
let me know if that works for you and I'll make a PR to target staging.

@ofborg ofborg bot added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. and removed 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. labels Sep 18, 2023
@tobim
Copy link
Contributor

tobim commented Sep 18, 2023

I have checked the clang release notes again and found out that all versions from 6 to 15 default to C++14. Sorry for misleading you.

I just opened #255979 to allow a somewhat simpler approach to this kind of problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 1001-2500 This PR causes many rebuilds on Darwin and should most likely target the staging branches. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants