cygwin: add as a cross-compilation target, and get bash.exe to compile #354137

puffnfresh · 2024-11-07T04:29:21Z

Things done

Add a 👍 reaction to pull requests you find important.

lib/systems/examples.nix

pkgs/os-specific/windows/cygwin/headers.nix

RossComputerGuy · 2024-11-07T04:34:12Z

pkgs/os-specific/windows/cygwin/headers.nix

+  meta = {
+    platforms = lib.platforms.windows;
+  };


Could be folded to just meta.platforms but it might be good to fill in the rest of the typical metadata.

This had been marked as resolved but doesn't seem to have actually been?

pkgs/by-name/co/cocom-tool-set/package.nix

RossComputerGuy · 2024-11-07T04:35:49Z

pkgs/development/libraries/readline/8.2.nix

@@ -3,7 +3,7 @@
 , updateAutotoolsGnuConfigScriptsHook
 , ncurses, termcap
 , curses-library ?
-    if stdenv.hostPlatform.isWindows
+    if stdenv.hostPlatform.isMinGW


What's the exact reason for changing from checking for windows in general to mingw? Would isWindows || isMinGW work better?

isWindows includes Cygwin. Under Cygwin we want to use ncurses, not termcap.

RossComputerGuy · 2024-11-07T04:35:54Z

pkgs/development/libraries/readline/8.2.nix

  patches = lib.optionals (curses-library.pname == "ncurses") [
    ./link-against-ncurses.patch
  ] ++ [
    ./no-arch_only-8.2.patch
  ]
  ++ upstreamPatches
-  ++ lib.optionals stdenv.hostPlatform.isWindows [
+  ++ lib.optionals stdenv.hostPlatform.isMinGW [


What's the exact reason for changing from checking for windows in general to mingw? Would isWindows || isMinGW work better?

We don't want to apply these patches under Cygwin. Cywin provides a POSIX API under Windows, so we want to use the normal POSIX calls which readline would usually make.

I guess this could be something like (stdenv.hostPlatform.isWindows && !stdenv.hostPlatform.isUnix) if we wanted to mark Cygwin as the technically‐correct Windows + Unix and confuse everyone for the rest of time?

(Because after all there could theoretically be e.g. an MSVC platform too…)

@emilazy yeah really good point, MSVC is on its way. I actually like the suggestion of isWindows && !isUnix.

Just be prepared for the fact that there’s probably a lot of things in the tree that assume the two are disjoint. But it’d be great to deconfuse that along the way.

And hey, it’ll prepare us for when midipix releases in 2038…

RossComputerGuy

Please squash specific changes amongst your commits. You can make several commits and use git rebase -i to do that. Also, please format your nix files so CI can pass.

Artturin · 2024-11-07T06:35:55Z

Please squash specific changes amongst your commits. You can make several commits and use git rebase -i to do that. Also, please format your nix files so CI can pass.

Check out git-absorb for this @puffnfresh
You can git reset --soft HEAD and then run it to absorb the changes you already committed

Artturin · 2024-11-07T06:43:14Z

pkgs/development/libraries/ncurses/default.nix

@@ -99,6 +99,8 @@ stdenv.mkDerivation (finalAttrs: {
    CFLAGS=-D_XOPEN_SOURCE_EXTENDED
  '';

+  hardeningDisable = lib.optionals stdenv.hostPlatform.isCygwin [ "fortify" ];


What's the error? Include it as a ~1 line verbatim comment

If this needs to be set for every package, why not globally disable fortify for cygwin?

@alyssais I wanted to, but I'm not sure where the best place to do that would be. Should it be an additional condition in the default value of defaultHardeningFlags in pkgs/build-support/bintools-wrapper?

This will also have the added benefit of making it not a mass rebuild, I think

I've moved this to bintools-wrapper and added a few things to avoid the mass rebuild.

The commits that set it individually for packages need to be updated then.

alyssais

This needs to be more than two commits. One logical change at a time, that gets it a bit further.

alyssais · 2024-11-07T12:16:32Z

pkgs/development/libraries/ncurses/default.nix

@@ -99,6 +99,8 @@ stdenv.mkDerivation (finalAttrs: {
    CFLAGS=-D_XOPEN_SOURCE_EXTENDED
  '';

+  hardeningDisable = lib.optionals stdenv.hostPlatform.isCygwin [ "fortify" ];


If this needs to be set for every package, why not globally disable fortify for cygwin?

puffnfresh · 2024-11-08T01:35:50Z

This needs to be more than two commits. One logical change at a time, that gets it a bit further.

I've split this into 10 commits now. Also resolved conflicts.

Ericson2314 · 2024-11-08T15:59:11Z

lib/systems/parse.nix

-        then { cpu = elemAt l 0;                      kernel = "windows";  abi = "cygnus";   }
+        then mkSkeletonFromList [ (elemAt l 0) "pc" "cygwin" ]


This change is purely a matter of style, not behavior, right?

(I kinda wonder if we can just drop this case, as the older cygwin stuff is bit-rotted for years at this point.)

I'm in favour of getting rid of all the older Cygwin support, since it just doesn't work anymore (I tried).

Yeah it's well over a decade old

I had a go at removing the hack altogether: puffnfresh@16e9e8b

It feels a little bit wrong to add Cygwin as a "kernel" but it does seem to work. Let me know what you think.

Ericson2314 · 2024-11-08T16:02:44Z

pkgs/os-specific/windows/cygwin/after-autogen.patch

Could we instead patch configure.ac and regen with autoreconfHook?

Maybe. This was copied from Microsoft's Windows-on-ARM-Experiments repository. Not sure why it was done that way.

oh probably because they are lazy af lol

Why copied and not fetched? If you used fetchpatch you could even use extraPrefix to make it so you can just put it in patches.

The size of various things don't line up when enabling fortification. This might be able to be fixed by looking at the Cywin builds, because I think they don't generally disable fortify.

Ericson2314 · 2024-11-18T16:31:43Z

Oh is this ofborg-eval-darwin failure spurious?

emilazy · 2024-11-18T16:46:02Z

error: cannot connect to socket at '/nix/var/nix/gc-socket/socket': No such file or directory, looks like it needs a retry.

@ofborg eval

Ericson2314 · 2024-11-20T06:22:43Z

OK this is evaluating now!

nixos-discourse · 2024-11-28T03:52:21Z

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

https://discourse.nixos.org/t/dropping-cygwin-support/3425/19

alyssais · 2024-12-19T09:32:22Z

pkgs/development/compilers/gcc/common/configure-flags.nix

@@ -242,6 +242,10 @@ let
      #   cc1: error: fp software completion requires '-mtrap-precision=i' [-Werror]
      "--disable-werror"
    ]
+    ++ lib.optionals targetPlatform.isCygwin [
+      # doesn't cross-compile under Cygwin


Do you mean "for Cygwin" (when running under some other platform)? Otherwise the check is wrong.

alyssais · 2024-12-19T09:32:54Z

pkgs/os-specific/windows/cygwin/headers.nix

+  };
+
+  patches = [
+    (fetchpatch {


This is a static file, so there's no need for patch normalization and fetchurl can be used.

alyssais · 2024-12-19T09:34:07Z

pkgs/os-specific/windows/cygwin/headers.nix

+  meta = {
+    platforms = lib.platforms.windows;
+  };


This had been marked as resolved but doesn't seem to have actually been?

alyssais · 2024-12-19T09:35:23Z

pkgs/by-name/co/cocom-tool-set/package.nix

+
+  nativeBuildInputs = [ bison ];
+
+  patches = [ ./remove-lto.patch ];


This could do with some explanation, ideally in the patch file.

alyssais · 2024-12-19T09:36:38Z

pkgs/by-name/co/cocom-tool-set/package.nix

+    url = "https://git.code.sf.net/p/cocom/git";
+    sha256 = "sha256-utLafkznMC4LrZgF6vKehtIGMwNMwLP9M9Nwu/RyWio=";
+    rev = "64ee80224aa13f9944d439f3f90862ca76158705";
+  };


Why not the release tarball?

alyssais · 2024-12-19T09:39:49Z

pkgs/os-specific/windows/mingw-w64/default.nix

+    ln -fs w32api/libpsapi.a .
+    ln -fs w32api/libuserenv.a .
+    ln -fs w32api/libnetapi32.a .
+    ln -fs w32api/libdbghelp.a .


Would w32api/*.a be wrong?

Changing directory without changing back at the end also makes me worry about fixupPhase not working.

alyssais · 2024-12-19T09:41:14Z

pkgs/os-specific/windows/cygwin/default.nix

+    ];
+  };
+in
+# TODO: Is there something like nix-support which would achieve this better?


Could you just link the appropriate paths in postInstall in the main derivation?

alyssais · 2024-12-19T09:42:09Z

pkgs/development/libraries/ncurses/default.nix

@@ -99,6 +99,8 @@ stdenv.mkDerivation (finalAttrs: {
    CFLAGS=-D_XOPEN_SOURCE_EXTENDED
  '';

+  hardeningDisable = lib.optionals stdenv.hostPlatform.isCygwin [ "fortify" ];


The commits that set it individually for packages need to be updated then.

alyssais · 2024-12-19T09:42:58Z

pkgs/build-support/cc-wrapper/default.nix

@@ -474,6 +474,7 @@ stdenvNoCC.mkDerivation {
        echo addToSearchPath "LINK_DLL_FOLDERS" "${cc_solib}/lib" > $out
        echo addToSearchPath "LINK_DLL_FOLDERS" "${cc_solib}/lib64" >> $out
        echo addToSearchPath "LINK_DLL_FOLDERS" "${cc_solib}/lib32" >> $out
+        echo addToSearchPath "LINK_DLL_FOLDERS" "${libc_bin}/bin" > $out


It's a bit surprising that it's in bin — is that definitely right?

alyssais · 2024-12-19T09:43:47Z

pkgs/shells/bash/5.nix

@@ -107,6 +107,8 @@ stdenv.mkDerivation rec {

  enableParallelBuilding = true;

+  makeFlags = [ ];


Please don't make the code more complicated just to avoid a mass rebuild. Mass rebuilds are fine. The bash changes can do to staging separately from the rest of this if you want.

github-actions bot added the 6.topic: lib The Nixpkgs function library label Nov 7, 2024

nix-owners bot requested review from alyssais, Artturin, emilazy, Ericson2314, philiptaron, reckenrode and RossComputerGuy November 7, 2024 04:30

RossComputerGuy reviewed Nov 7, 2024

View reviewed changes

lib/systems/examples.nix Outdated Show resolved Hide resolved

RossComputerGuy reviewed Nov 7, 2024

View reviewed changes

RossComputerGuy requested changes Nov 7, 2024

View reviewed changes

Artturin reviewed Nov 7, 2024

View reviewed changes

puffnfresh force-pushed the cygwin/target branch 3 times, most recently from 9a9998b to 05826a1 Compare November 7, 2024 11:55

alyssais requested changes Nov 7, 2024

View reviewed changes

puffnfresh requested a review from RossComputerGuy November 7, 2024 13:54

puffnfresh added 4 commits November 8, 2024 01:06

cygwin: init as a target toolchain

8c9c583

cygwin: add pre-libc headers

12c6e62

cygwin: remove old w32api, it's in mingw-w64

e670bc6

cocom-tool-set: init at 0.996

b6262e0

puffnfresh added 4 commits November 8, 2024 01:26

readline: fix compilation under Cygwin

8692585

ncurses: disable fortify under Cygwin

8b47da2

bash: fix compilation under Cygwin

74bc044

cygwin: include libcygwin1.dll in DLL search path

3495b2e

puffnfresh force-pushed the cygwin/target branch from 05826a1 to 3495b2e Compare November 8, 2024 01:35

github-actions bot added the 6.topic: windows Running, or buiding, packages on Windows label Nov 8, 2024

puffnfresh mentioned this pull request Nov 8, 2024

Functional tests that work on both Windows and Unix NixOS/nix#10823

Open

Ericson2314 mentioned this pull request Nov 8, 2024

Add Nix msys2/MINGW-packages#22459

Open

5 tasks

Ericson2314 reviewed Nov 8, 2024

View reviewed changes

puffnfresh added 3 commits November 9, 2024 09:20

bintools-wrapper: don't set fortify on Cygwin

c0b60f3

The size of various things don't line up when enabling fortification. This might be able to be fixed by looking at the Cywin builds, because I think they don't generally disable fortify.

bash: set empty makeFlags to avoid mass rebuild

20d2d5b

mingw-w64: allow Cygwin as a crt

573c420

puffnfresh requested a review from alyssais November 30, 2024 10:03

alyssais requested changes Dec 19, 2024

View reviewed changes

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 31, 2024

		then { cpu = elemAt l 0; kernel = "windows"; abi = "cygnus"; }
		then mkSkeletonFromList [ (elemAt l 0) "pc" "cygwin" ]


		nativeBuildInputs = [ bison ];

		patches = [ ./remove-lto.patch ];

		@@ -107,6 +107,8 @@ stdenv.mkDerivation rec {

		enableParallelBuilding = true;

		makeFlags = [ ];

cygwin: add as a cross-compilation target, and get bash.exe to compile #354137

Are you sure you want to change the base?

cygwin: add as a cross-compilation target, and get bash.exe to compile #354137

Conversation

puffnfresh commented Nov 7, 2024

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilazy Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilazy Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

RossComputerGuy left a comment

Choose a reason for hiding this comment

Artturin commented Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssais left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puffnfresh commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Nov 18, 2024

emilazy commented Nov 18, 2024

Ericson2314 commented Nov 20, 2024

nixos-discourse commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilazy Nov 7, 2024 •

edited

Loading

emilazy Nov 7, 2024 •

edited

Loading

Artturin commented Nov 7, 2024 •

edited

Loading