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

python311: enable windows cross-compilation, make win-dll-hook link to distant dependancies #227818

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

marius851000
Copy link
Contributor

@marius851000 marius851000 commented Apr 23, 2023

Description of changes

A minimal set of change to enable cross-compilation of the python 3.11 interpreter to windows (pkgsCross.mingwW64.python311), thanks to patch used by fedora.

It also change win-dll-hook to not only link necessary dlls from all the outputs in the binary folder, but also those in distant package, based on a new environment variable for Windows cross-build, LINK_DLL_FOLDERS, that list folders containing DLLs that may be copied to the binary folder.

(for exemple, it will move libgcc_s_seh-1.dll, libwinpthread-1.dll, libpython3.11.dll and mcfgthread-12.dll in the bin folder of python311, the python fixup phase will do the same for compiled python dependancies, that are stored in a different folder)

It also cause a world rebuild due to the change in the gcc build script and the generic CC wrapper.

Note that this is only sufficient for compiling the interpreter itself, but not dependancies. I’ll make another PR improving the makeBinaryWrapper, that should also replace shebang with an executable for Windows before trying to take a deeper look at compiling dependancies.

ps: Only tested with Wine on my machine. I haven’t tested on Windows, and I’m a bit worried in regard to the Nix store. Ideally, everything use a relative path, so I can just copy a folder containing all run-time derivation and move it around.

Things done

I tried to make particularly sure that change that are not behind an isWindows or similar doesn’t have side-effect for other packages, althought I didn’t tried to re-compile everything.

  • Built on platform(s)
    • x86_64-linux (for mingwW64)
    • 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/) (NOTE: only tested executable files. Windows can’t run shebang, but I’ll make another independant patch for a generic solution to this problem involving makeBinaryWrapper)
  • 23.05 Release Notes (or backporting 22.11 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.

superseed #84815

@marius851000
Copy link
Contributor Author

Seems that both failed builds are due to timeout.

@marius851000
Copy link
Contributor Author

Additionnally, this should fix #38451 and partially replace #38632 . specific mention to @Ericson2314 , which seems interested by that.

@marius851000
Copy link
Contributor Author

marius851000 commented Apr 24, 2023

Seems that I messed the part that try to not copy dll already loaded by python into the dynload folder. I’ll fix that later today.

@wegank wegank added the 6.topic: exotic Exotic hardware or software platform label May 6, 2023
@Artturin Artturin added the 6.topic: windows Running, or buiding, packages on Windows label Aug 27, 2023
@@ -52,7 +52,7 @@ stdenv.mkDerivation rec {
xz
xz.bin
];
buildInputs = [ bash ]
buildInputs = lib.optionals (!stdenv.hostPlatform.isWindows) [ bash ]
Copy link
Member

Choose a reason for hiding this comment

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

can be dropped 7bc650e

@Artturin
Copy link
Member

Artturin commented Aug 27, 2023

Non python non hook changes split to #251828

Comment on lines 100 to 105
pythonNamespacesHook = callPackage ({ makePythonHook, buildPackages }:
makePythonHook {
name = "python-namespaces-hook.sh";
substitutions = {
inherit pythonSitePackages findutils;
inherit pythonSitePackages;
findutils = buildPackages.findutils;
Copy link
Member

Choose a reason for hiding this comment

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

should be unneeded since #245475

@Artturin
Copy link
Member

Artturin commented Sep 5, 2023

Rebased and tweaked, now only one commit remains! 😄

@ofborg ofborg bot added the 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild label Sep 5, 2023
@Ericson2314
Copy link
Member

@Artturin Do you want to hack this up to make it a non-mass-rebuild, and then fix up the code (to be like it is now) on staging afterwards?

@Artturin Artturin changed the base branch from staging to master September 6, 2023 21:06
@Artturin Artturin force-pushed the python_win branch 3 times, most recently from 6a4ef75 to 6740ec4 Compare September 6, 2023 21:14
@Artturin
Copy link
Member

Artturin commented Sep 6, 2023

@Artturin Do you want to hack this up to make it a non-mass-rebuild, and then fix up the code (to be like it is now) on staging afterwards?

Should be like that now

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux-stdenv This PR causes stdenv to rebuild 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Sep 6, 2023
Comment on lines +48 to +50
# these dont build for windows
, withGdbm ? !stdenv.hostPlatform.isWindows
, withReadline ? !stdenv.hostPlatform.isWindows
Copy link
Member

@Artturin Artturin Sep 6, 2023

Choose a reason for hiding this comment

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

Copy link
Member

@Artturin Artturin Sep 6, 2023

Choose a reason for hiding this comment

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

for readline

diff --git a/pkgs/development/libraries/readline/8.2.nix b/pkgs/development/libraries/readline/8.2.nix
index 1c53da3cdfa4..eca19897e73e 100644
--- a/pkgs/development/libraries/readline/8.2.nix
+++ b/pkgs/development/libraries/readline/8.2.nix
@@ -1,4 +1,4 @@
-{ fetchurl, stdenv, lib, ncurses
+{ fetchurl, stdenv, lib, ncurses, fetchgit
 }:
 
 stdenv.mkDerivation rec {
@@ -33,6 +33,23 @@ stdenv.mkDerivation rec {
     ]
     ++ upstreamPatches;
 
+  postPatch = if (stdenv.hostPlatform.isMinGW) then (
+    let
+      # https://src.fedoraproject.org/rpms/mingw-gdbm
+      readline-patch = fetchgit {
+        name = "mingw-readline-patches";
+        url = "https://src.fedoraproject.org/rpms/mingw-readline.git";
+        rev = "c15e77591106244a9f1c318226167d5bcea7bf04"; # patches for readline 8.2
+        sha256 = "sha256-QcMJ/nq2T4QlLkw0tAutMohAB0zWa1mRUYuN7D/Kvh0=";
+      };
+    in
+    ''
+      for p in ${readline-patch}/*.patch; do
+        patch -p1 -d . -i $p
+      done
+    ''
+  ) else null;
+
   meta = with lib; {
     description = "Library for interactive line editing";
 
@@ -57,7 +74,7 @@ stdenv.mkDerivation rec {
 
     maintainers = with maintainers; [ dtzWill ];
 
-    platforms = platforms.unix;
+    platforms = platforms.unix ++ platforms.windows;
     branch = "8.2";
   };
 }
readline-x86_64-w64-mingw32> /nix/store/8gfd58vf0arh22p7bc8da0qidrydfi4b-x86_64-w64-mingw32-binutils-2.40/bin/x86_64-w64-mingw32-ld: cannot
find -lncurses: No such file or directory
readline-x86_64-w64-mingw32> /nix/store/8gfd58vf0arh22p7bc8da0qidrydfi4b-x86_64-w64-mingw32-binutils-2.40/bin/x86_64-w64-mingw32-ld: cannot
find -ltermcap: No such file or directory

fedora has mingw-termcap https://src.fedoraproject.org/rpms/mingw-termcap/blob/rawhide/f/mingw-termcap.spec

which has a comment

# Note: Termcap was deprecated and removed from Fedora after F-8.  It
	
# has been replaced by ncurses.  However ncurses cannot be compiled on
	
# Windows so we have to supply termcap.

but pkgsCross.mingwW64.ncurses builds for us and is already in propagatedBuildInputs but does not work.

Copy link
Member

Choose a reason for hiding this comment

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

the gdbm patch is for 1.11 and doesn't apply to our 1.23

Comment on lines +386 to +388
# Both fail when building for windows, normally configure checks this by itself but on other platforms this is set to yes always.
"ac_cv_file__dev_ptmx=${if stdenv.hostPlatform.isWindows then "no" else "yes"}"
"ac_cv_file__dev_ptc=${if stdenv.hostPlatform.isWindows then "no" else "yes"}"
Copy link
Member

Choose a reason for hiding this comment

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

Removed the in-tree patch and did this instead.

@Artturin Artturin force-pushed the python_win branch 2 times, most recently from acac81b to ff131bc Compare September 6, 2023 23:13
mingw-patch = fetchgit {
name = "mingw-python-patches";
url = "https://src.fedoraproject.org/rpms/mingw-python3.git";
rev = "45c45833ab9e5480ad0ae00778a05ebf35812ed4"; # for python 3.11.5 at the time of writing.
Copy link
Member

Choose a reason for hiding this comment

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

I updated the rev to python 3.11.5 even though the patch files haven't changed.

Copy link
Member

@Artturin Artturin left a comment

Choose a reason for hiding this comment

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

Merge after eval is done and 0 rebuilds

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 7, 2023
@Artturin Artturin merged commit 57095e2 into NixOS:master Sep 8, 2023
10 checks passed
@marius851000
Copy link
Contributor Author

Thanks, and sorry for not having fixed that myselr. Kept pushing that back to tomorrow since I'm back from vacation. For now, my mind is rather away to Nixpkgs and into other projects (maybe NixCon's stream will change that, maybe not).

Have a nice day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: exotic Exotic hardware or software platform 6.topic: python 6.topic: windows Running, or buiding, packages on Windows 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants