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

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

Merged
merged 1 commit into from Feb 28, 2019
Merged

Conversation

@jules2689
Copy link
Contributor

jules2689 commented Feb 25, 2019

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

This comment has been minimized.

Copy link
Contributor Author

jules2689 commented Feb 25, 2019

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

This comment has been minimized.

Copy link
Contributor

orivej commented Feb 25, 2019

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

This comment has been minimized.

Copy link
Contributor Author

jules2689 commented Feb 25, 2019

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?

#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 jules2689 force-pushed the jules2689:patch-1 branch from b2de6aa to 7ed30d2 Feb 25, 2019
@jules2689

This comment has been minimized.

Copy link
Contributor Author

jules2689 commented Feb 25, 2019

@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

This comment has been minimized.

Copy link
Member

veprbl commented Feb 26, 2019

This is probably just another case of #54606

@orivej

This comment has been minimized.

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…

@orivej
orivej approved these changes Feb 26, 2019
@jules2689 jules2689 marked this pull request as ready for review Feb 26, 2019
@jules2689

This comment has been minimized.

Copy link
Contributor Author

jules2689 commented Feb 26, 2019

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

This comment has been minimized.

Copy link
Contributor Author

jules2689 commented Feb 26, 2019

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
10 checks passed
10 checks passed
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@orivej

This comment has been minimized.

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 jules2689:patch-1 branch Dec 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.