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

Jump to bottom

Use C++ 11 for MySQL 5.7 to fix build in mac #56384

Merged
merged 1 commit into from Feb 28, 2019
Merged

Use C++ 11 for MySQL 5.7 to fix build in mac #56384

merged 1 commit into from Feb 28, 2019

Conversation

jules2689
Copy link
Contributor

@jules2689 jules2689 commented Feb 25, 2019 •
edited

Motivation for this change

#49174 shows us the error that we were experiencing.
The last successful build was https://hydra.nixos.org/build/81938220, with the first failing here: https://hydra.nixos.org/build/82083071

Description

#49174 shows us the error that we were experiencing.
The last successful build was https://hydra.nixos.org/build/81938220, with the first failing here: https://hydra.nixos.org/build/82083071

The difference between these 2 builds seems to be Protobuf 3.4 being updated to Protobuf 3.6.

This commit makes MySQL 5.7 use c++ 11 to fix these issues as Protobuf 3.6 uses newer C++ features

Fixes #49174

Things done
  • Not entirely sure how to run all of this.
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
    -> Can't do this one as it didn't work before. It's just a revert anyways, so it is the same as it once was.
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@jules2689
Copy link
Contributor Author

I'd like to run https://github.com/NixOS/nixpkgs/blob/master/nixos/tests/mysql.nix, but I'm not sure how. Same with the sandbox

@orivej
Copy link
Contributor

orivej commented Feb 25, 2019 •
edited

Please use -std=c++11 compilation flag instead; maybe by adding CXXFLAGS = "-std=c++11"; to the nix file.

The tests can be run with nix-build nixos/tests/mysql.nix, but only on Linux.

@jules2689
Copy link
Contributor Author

Please use -std=c++11 compilation flag instead; maybe by adding CXXFLAGS = "-std=c++11"; to the nix file.

Can you elaborate a bit on how this solves the issue? I can see that c++ 11 is used (as opposed to which version?)

Protobuf 3.6 is incompatible, I assume, because it uses a different C++ compiler?

@jules2689
Use c++ 11 for MySQL 5.7 to fix build in mac
7ed30d2 
#49174 shows us the error that we were experiencing.
The last successful build was https://hydra.nixos.org/build/81938220, with the first failing here: https://hydra.nixos.org/build/82083071

The difference between these 2 builds seems to be Protobuf 3.4 being updated to Protobuf 3.6.
Protobuf fails because the newer version uses newer C++ features.

This commit makes MySQL 5.7 use C++ 11 to fix these issues.
@jules2689
Copy link
Contributor Author

@orivej updated! Thanks for the pointer 🙏

@jules2689 jules2689 changed the title Use Protobuf 3.4 for MySQL 5.7 to fix build in mac Use C++ 11 for MySQL 5.7 to fix build in mac Feb 25, 2019
@veprbl
Copy link
Member

veprbl commented Feb 26, 2019

This is probably just another case of #54606

@orivej
Copy link
Contributor

orivej commented Feb 26, 2019

I can see that c++ 11 is used (as opposed to which version?)

Clang 6 has updated the default c++ from 98 to 14, but on Darwin we still use Clang 5. (Is there a github issue about updating the default LLVM on Darwin? Related: #49402, #39986.)

Can you elaborate a bit on how this solves the issue?

Protobuf 3.6 has dropped support for c++ older than 11: now all packages that use protobuf need -std=c++11 or newer.

@veprbl Maybe mysql does not declare that it needs C++ 11 because by itself it does not…

@jules2689 jules2689 marked this pull request as ready for review February 26, 2019 12:43
@jules2689
Copy link
Contributor Author

The tests can be run with nix-build nixos/tests/mysql.nix, but only on Linux.

I'll look into that today, but I don't have easy access to linux. Any reason why this won't work on mac?

@jules2689
Copy link
Contributor Author

Maybe mysql does not declare that it needs C++ 11 because by itself it does not…

Can I add that to this pr? Should I?

Apologise for the many questions, quite new to nix and trying to wrap my head around it to migrate our company's developers to nix from homebrew et al.

@orivej-nixos orivej-nixos merged commit 1aa1360 into NixOS:master Feb 28, 2019
@orivej
Copy link
Contributor

orivej commented Feb 28, 2019

Any reason why this won't work on mac?

NixOS tests build thin NixOS (Linux) images and run them in qemu, sharing host /nix/store with the guest. AFAIK there is no fundamental reason why this or equivalent can't be supported on macOS, but it is not easy. For one thing, if I'm not mistaken, nix-build nixos/tests/….nix does not establish cross compilation from the host OS to Linux if the host OS is not Linux.

Can I add that to this pr? Should I?

I think this PR was ready as it was. I left it for a while for someone more active to merge it.

@jules2689 jules2689 deleted the patch-1 branch December 15, 2019 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orivej orivej orivej approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mysql-5.7.23 on Mac OS 10.14
6 participants
@jules2689 @orivej @veprbl @GrahamcOfBorg @orivej-nixos @Janik-Haag