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

tree-wide: enable LTO on Darwin #328586

Merged
merged 2 commits into from
Jul 20, 2024
Merged

Conversation

reckenrode
Copy link
Contributor

Description of changes

This PR is the second follow-up to #307880. It enables LTO in packages flagged by #19098 as well as others I could identify as having it disabled on Darwin.

I tested building Python on both aarch64- and x86_64-darwin. I also ran a nixpkgs-review on aarch64-darwin with the Python LTO disabled (to keep the number of changed packages manageable). Most packages built. Those that failed did not appear to be LTO-related. I have a number of PRs I will be submitting for some of those broken packages.

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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Override the configure check not to do a universal build. Using the system libmpdec is required to enable LTO on Darwin.
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Jul 20, 2024
@paparodeo
Copy link
Contributor

enable lto for nuitka
diff --git a/pkgs/development/python-modules/nuitka/darwin-lto.patch b/pkgs/development/python-modules/nuitka/darwin-lto.patch
deleted file mode 100644
index bd3a65905498..000000000000
--- a/pkgs/development/python-modules/nuitka/darwin-lto.patch
+++ /dev/null
@@ -1,15 +0,0 @@
-diff --git a/nuitka/build/SconsCompilerSettings.py b/nuitka/build/SconsCompilerSettings.py
-index 319b72c4e..89d40f2a2 100644
---- a/nuitka/build/SconsCompilerSettings.py
-+++ b/nuitka/build/SconsCompilerSettings.py
-@@ -173,8 +173,8 @@ def _enableLtoSettings(
-         lto_mode = False
-         reason = "known to be not supported (CondaCC)"
-     elif isMacOS() and env.gcc_mode and env.clang_mode:
--        lto_mode = True
--        reason = "known to be supported (macOS clang)"
-+        lto_mode = False
-+        reason = "known to not be supported (macOS nix clang)"
-     elif env.mingw_mode and env.clang_mode:
-         lto_mode = False
-         reason = "known to not be supported (new MinGW64 Clang)"
diff --git a/pkgs/development/python-modules/nuitka/default.nix b/pkgs/development/python-modules/nuitka/default.nix
index fa0842267031..cce4467e73e5 100644
--- a/pkgs/development/python-modules/nuitka/default.nix
+++ b/pkgs/development/python-modules/nuitka/default.nix
@@ -23,9 +23,6 @@ buildPythonPackage rec {
     hash = "sha256-nKdCMgA92v9VsSgfktXDbSh3DyKsGlcTjpn0Y7u4rxU=";
   };
 
-  # default lto off for darwin
-  patches = [ ./darwin-lto.patch ];
-
   build-system = [
     setuptools
     wheel

@reckenrode
Copy link
Contributor Author

enable lto for nuitka

Thanks. I added nuitka to the tree-wide commit.

Copy link
Contributor

@paparodeo paparodeo left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -10,7 +10,7 @@
# requiring to build a special variant for that software. Example: 'haproxy'
, variant ? "all"
, extraConfigureFlags ? []
, enableLto ? !(stdenv.isDarwin || stdenv.hostPlatform.isStatic || stdenv.cc.isClang)
, enableLto ? !(stdenv.hostPlatform.isStatic || stdenv.cc.isClang)
Copy link
Contributor

@paparodeo paparodeo Jul 20, 2024

Choose a reason for hiding this comment

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

still not enabled -- wonder why it is gated on clang.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t know, which is why I left that part of the condition in place. If the clang part is resolved, then Darwin should just work.

@reckenrode reckenrode merged commit 59dba7c into NixOS:staging Jul 20, 2024
8 of 9 checks passed
@reckenrode reckenrode deleted the darwin-lto branch July 20, 2024 07:30
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/darwin-updates-news/42249/16

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

Successfully merging this pull request may close these issues.

3 participants