Skip to content

Conversation

@joshheinrichs-shopify
Copy link
Contributor

Currently percona-server_8_0 fails to compile on Darwin for a few reasons. First it hits an error due to adding multiple copies of OpenSSL[1] which Homebrew has fixed and attempted to upstream[2][3]. After addressing that, it fails to compile due to the removal of std::char_traits<uchar> in LLVM 19. This has already been patched in mysql80[4], but I couldn't find a precedent for grabbing patches from another package, so I opted to adapt the patch instead. The Percona-specific changes are minimal and could be extracted into a separate patch if that's desirable.

[1] https://perconadev.atlassian.net/jira/software/c/projects/PS/issues/PS-9641
[2] percona/percona-server#5537
[3] Homebrew/homebrew-core#204799
[4] #374591

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Apr 24, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Apr 24, 2025
@yzx9
Copy link
Contributor

yzx9 commented Apr 28, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 401322


x86_64-linux

✅ 2 packages built:
  • percona-server_8_0
  • percona-server_8_0.static

aarch64-darwin

✅ 2 packages built:
  • percona-server_8_0
  • percona-server_8_0.static

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why the patch includes a license. How about using fetchpatch instead?

Copy link
Contributor Author

@joshheinrichs-shopify joshheinrichs-shopify May 9, 2025

Choose a reason for hiding this comment

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

sorry it took me a while to get back to this!

this patch is adapted from a patch to mysql80 in nixpkgs[1] which a change in mysql 9.2 backported to 8.0[2][3], hence the license. there's an extra bit in there to apply the changes to range_optimizer/index_range_scan_plan.cc. i could split this up so i instead reference the patch out of mysql80 and then only add the range_optimizer/index_range_scan_plan.cc bit as a separate patch to percona. i just couldn't find any other examples of packages using patches from other packages and it felt better to keep things together since it's one logical change. no strong preference on my end though.

[1] https://github.com/NixOS/nixpkgs/blob/33949e56e5d67374ca143da77bcc8ab994c4a2e6/pkgs/servers/sql/mysql/libcpp-fixes.patch
[2] #374591 (comment)
[3] mysql/mysql-server@3a51d7f

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think we have to split up the patch into the smallest possible parts just by principle, this can also affect readability. So I think your approach is feasible.
Another reason for splitting the patch into the base patch you adapted, and the changes you made yourself on top, could be making it easier to pull in future changes from mysql as well. But instead you can just optionally reference the base patch by URI in the comment on top of the patch that currently only holds the license.

One minor license nitpick is that, when distributing modified cersions of GPL source code, you need to clearly state that you've modified the source (2.a), adding your own copyright notice there.

Currently percona-server_8_0 fails to compile on Darwin for a few
reasons. First it hits an error due to adding multiple copies of
OpenSSL[1] which Homebrew has fixed and attempted to upstream[2][3].
After addressing that, it fails to compile due to the removal of
std::char_traits<uchar> in LLVM 19. This has already been patched in
mysql80[4], but I couldn't find a precedent for grabbing patches from
another package, so I opted to adapt the patch instead. The
Percona-specific changes are minimal and could be extracted into a
separate patch if that's desirable.

[1] https://perconadev.atlassian.net/jira/software/c/projects/PS/issues/PS-9641
[2] percona/percona-server#5537
[3] Homebrew/homebrew-core#204799
[4] NixOS#374591
@joshheinrichs-shopify joshheinrichs-shopify force-pushed the percona-server-8_0-darwin branch from 8faa4e3 to 321d3db Compare May 9, 2025 12:37
@yzx9 yzx9 added the 0.kind: ZHF Fixes Fixes during the Zero Hydra Failures (ZHF) campaign label May 9, 2025
@vcunat vcunat merged commit 73e0a98 into NixOS:master May 29, 2025
28 checks passed
@vcunat vcunat added the backport release-25.05 Backport PR automatically label May 29, 2025
@nixpkgs-ci
Copy link
Contributor

nixpkgs-ci bot commented May 29, 2025

Successfully created backport PR for release-25.05:

@nixpkgs-ci
Copy link
Contributor

nixpkgs-ci bot commented May 29, 2025

Git push to origin failed for release-25.05 with exitcode 1

@github-actions github-actions bot added the 8.has: port to stable This PR already has a backport to the stable release. label May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0.kind: ZHF Fixes Fixes during the Zero Hydra Failures (ZHF) campaign 6.topic: darwin Running or building packages on Darwin 8.has: port to stable This PR already has a backport to the stable release. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. backport release-25.05 Backport PR automatically

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants