Cygwin fixes #26021

corngood · 2017-05-23T13:40:34Z

Motivation for this change

This was enough to get git building on cygwin.

The changes should all be limited to stdenv.isCygwin.

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
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/)
Fits CONTRIBUTING.md.

mention-bot · 2017-05-23T13:40:37Z

@corngood, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zimbatm, @jagajaga and @durko to be potential reviewers.

corngood · 2017-05-23T13:49:53Z

pkgs/build-support/setup-hooks/win-dll-link.sh

@@ -35,7 +35,7 @@ _linkDLLs() {
        local dllPath2
        for dllPath2 in "$dllPath" "$(dirname $(readlink "$dllPath" || echo "$dllPath"))"/*.dll; do
            if [ -e ./"$(basename "$dllPath2")" ]; then continue; fi
-            ln -sr "$dllPath2" .
+            CYGWIN+=\ winsymlinks:nativestrict ln -sr "$dllPath2" .


This forces cygwin to use real symlinks, which are needed for the windows library loader. Unfortunately it depends on having elevated permissions in some cases.

corngood · 2017-05-23T13:54:41Z

lib/systems/parse.nix

@@ -71,7 +71,7 @@ rec {
  kernels = with execFormats; with kernelFamilies; setTypesAssert "kernel"
    (x: isExecFormat x.execFormat && all isKernelFamily (attrValues x.families))
  {
-    cygwin  = { execFormat = pe;      families = { inherit /*unix*/ windows-nt; }; };
+    cygwin  = { execFormat = pe;      families = { inherit unix windows-nt; }; };


Could also add cygwin support per-package, but it seems to be that cygwin is as unixy as the others here.

corngood · 2017-05-23T13:55:31Z

pkgs/top-level/perl-packages.nix

    name = "LocaleGettext-1.05";
    buildInputs = stdenv.lib.optional (stdenv.isFreeBSD || stdenv.isDarwin || stdenv.isCygwin) pkgs.gettext;
    NIX_CFLAGS_LINK = stdenv.lib.optional (stdenv.isFreeBSD || stdenv.isDarwin || stdenv.isCygwin) "-lintl";
    src = fetchurl {
      url = mirror://cpan/authors/id/P/PV/PVANDRY/gettext-1.05.tar.gz;
      sha256 = "15262a00vx714szpx8p2z52wxkz46xp7acl72znwjydyq4ypydi7";
    };
-  };
+  } // stdenv.lib.optionalAttrs stdenv.isCygwin {
+    LANG="C";


Tests fail without this

I think it is safe to set this on all platforms.

corngood · 2017-05-23T13:56:00Z

pkgs/stdenv/cygwin/rebase-x86_64.sh

        return
    fi
-    find $out -name "*.dll" | while read DLL; do
+    find "$prefix" -name "*.dll" -type f | while read DLL; do


Stop it from rebasing linked libs that have already been rebased.

zimbatm

Looking good!

I don't think that cygwin is really supported right now so we might as well merge that in quickly.

Do you have anything specific you want to tackle before merging into master?

corngood · 2017-05-25T22:02:31Z

@zimbatm I just wanted to make sure it good a solid review, but I'm not aware of any problems, so I removed WIP.

Unfortunately, there's an non trivial conflict now (the cygwin kernel was removed), so I'll have to resolve that and test with latest master.

zimbatm · 2017-05-26T15:06:38Z

Thanks, ping me when you fixed the rebase conflicts

corngood · 2017-06-09T14:08:28Z

@Ericson2314 This patch currently adds back the cygwin kernel you recently removed. The intention was to allow cygwin to inherit 'unix' packages. Perhaps 'unix' doesn't really make sense as a kernel family... What do you think?

Ericson2314 · 2017-06-09T14:37:41Z

@corngood Thanks for the @-mention and good work! So I originally had a cygwin kernel too, but then, following LLVM's example took it out. For now, perhaps you can modify the definition of isUnix instead?

Also, I've been meaning to deprecate stdenv.is*, and use {build,host,target}Platform.is* instead, as the latter are more precise. If you could preempt my deprecation by using that, it would be much appreciated.

Ericson2314 · 2017-06-09T14:38:58Z

pkgs/development/libraries/gdbm/default.nix

@@ -1,4 +1,4 @@
-{ stdenv, fetchurl }:
+{ stdenv, lib, fetchurl }:


For just one use of lib, it's more idiomatic to just do stdenv.lib.

There was one other use of stdenv.lib, so I shortened it.

Ericson2314 · 2017-06-09T14:41:07Z

pkgs/development/libraries/libxslt/default.nix

@@ -41,6 +41,8 @@ stdenv.mkDerivation rec {
    moveToOutput lib/python2.7 "$py"
  '';

+  makeFlags = optionalString stdenv.isCygwin "LDFLAGS=-no-undefined";


Could this be passed to configure instead? Not sure which is better, but somehow curious.

Ericson2314 · 2017-06-09T14:42:05Z

pkgs/development/libraries/openssl/default.nix

@@ -22,7 +22,6 @@ let
      ++ [ ./nix-ssl-cert-file.patch ]
      ++ optional (versionOlder version "1.1.0")
          (if stdenv.isDarwin then ./use-etc-ssl-certs-darwin.patch else ./use-etc-ssl-certs.patch)
-      ++ optional stdenv.isCygwin ./1.0.1-cygwin64.patch


Did you delete this patch file?

corngood · 2017-06-09T14:56:15Z

@Ericson2314 Rewriting isUnix and removing the unix kernel family seems like a good idea. I'm thinking it's best to define Unix as a whitelist of:

    Darwin       = { kernel = kernels.darwin; };
    Linux        = { kernel = kernels.linux; };
    SunOS        = { kernel = kernels.solaris; };
    FreeBSD      = { kernel = kernels.freebsd; };
    Hurd         = { kernel = kernels.hurd; };
    NetBSD       = { kernel = kernels.netbsd; };
    OpenBSD      = { kernel = kernels.openbsd; };
    Cygwin       = { kernel = kernels.cygwin; abi = abis.cygnus; };

Ericson2314 · 2017-06-09T16:44:16Z

@corngood yeah feel free to remove the whole notion of "kernel familes" if you like. matchAttrs doesn't really provide much notion of "or"---you can either just define isUnix as a function predicate, or maybe make a matchAnyAttrs that uses list for or?

corngood · 2017-06-12T17:39:40Z

@Ericson2314 f851fba is what I came up with for isUnix. If I made all the patterns lists, then matchAnyAttrs might make sense in attrsets.nix, but I left it local for now.

I didn't completely remove kernel families, because there is at least a conceivable use for them, but I can do that if you'd prefer.

Ericson2314 · 2017-06-12T17:51:17Z

lib/systems/inspect.nix

    BSD          = { kernel = { families = { inherit (kernelFamilies) bsd; }; }; };
+    Unix         = [ BSD Linux SunOS Hurd Cygwin ];


Could you either remove the inner rec and do with patterns; here, or remove the existing patterns. below?

I removed the redundant patterns., aside from the string indexed ones. I'm not aware of a better way to deal with them. I don't really have strong feelings on this vs. with patterns;, but this is slightly more concise.

I think it was the string ones that led me to avoid the inner rec to begin with, but that was probably an overkill move on my part :).

Ericson2314 · 2017-06-12T17:54:32Z

@corngood good decisions on both counts. I just left you one tiny nit---hardly worth mentioning. Other than that, there is the notes on stdenv.is vs hostPlatform.is* on #26021 (comment) . I suppose it's fine to just to deal with that latter, but it would be useful if anyone wants to cross compile for cygwin.

corngood · 2017-06-12T20:08:53Z

@Ericson2314 Is 974df1f what you had in mind? I'm pretty sure those are all correct as hostPlatform.

I can squash them into the original changes if you prefer.

Ericson2314 · 2017-06-12T20:15:07Z

@corngood a quick skim found one that should actually be buildPlatform.isCygwin but the rest I think are good. Thanks for making these changes!

No preference on redoing the history or not.

corngood · 2017-06-13T11:36:28Z

@Ericson2314 good catch. I made that one change and squashed each package.

Looks like CI is failing due to timeout / log limit?

Ericson2314 · 2017-06-15T19:03:31Z

CI is failing because travis sucks and the hydra channels are broken for days, sniff. I suppose I could just throw my hands in the error and merge this like other PRs, but I was waiting in hopes the channels would eventually be fixed and then it could be properly tested.

It's really awful that

We allow master to be broken
We allow merging PRs unrelated to fixing it once it is

the rest were removed in 1dc6f15

System predicate patterns can now be specified as a list of OR'd attribute sets.

It's always on, and you get a warning if you specify it

corngood · 2017-06-26T16:52:47Z

@Ericson2314 rebased on staging.

Building stdenv (especially gcc) is a can of worms. I'll keep plugging away at it in my spare time, but there's no point in holding this up.

Ericson2314 · 2017-06-26T17:02:42Z

@corngood Fair enough. So is this PR just progress then? Or does git still build?

corngood · 2017-06-26T17:33:52Z

@Ericson2314 git doesn't currently build due to breakage in ruby, via docbook.

corngood · 2017-06-26T18:51:47Z

@Ericson2314 actually looks like gitMinimal does build, so it's sufficient for fetchgit, which is what I was originally looking for.

Ericson2314 · 2017-06-26T19:04:30Z

Sounds great; I'm going to merge! Sorry this took so long, @corngood, but excellent job!

vcunat · 2017-07-03T09:22:44Z

@Ericson2314, @corngood: this made all darwin packages evaluate as broken, stopping Hydra builds. Any ideas? I bisected the error to 4ac1901, but I don't understand the changes enough (yet).

Ericson2314 · 2017-07-03T11:41:39Z

The Unix list should include Darwin, but doesn't. Sorry I didn't catch this the first time around.

corngood · 2017-07-03T12:36:32Z

Shit, sorry about that. I prepared a PR to fix isUnix: #27089. I'm just in the process of reproducing it.

vcunat · 2017-07-03T13:30:19Z

Thanks for the quick fixup. Job counts on Hydra seem OK now.

corngood commented May 23, 2017

View reviewed changes

zimbatm approved these changes May 25, 2017

View reviewed changes

corngood changed the title ~~WIP: Cygwin fixes~~ Cygwin fixes May 25, 2017

corngood force-pushed the cygwin-wip branch from 04aa930 to 3f6f090 Compare June 9, 2017 14:02

Ericson2314 reviewed Jun 9, 2017

View reviewed changes

corngood force-pushed the cygwin-wip branch 2 times, most recently from 715adca to f851fba Compare June 12, 2017 17:32

Ericson2314 reviewed Jun 12, 2017

View reviewed changes

corngood force-pushed the cygwin-wip branch 3 times, most recently from e89ad85 to 24a3b34 Compare June 12, 2017 19:36

corngood force-pushed the cygwin-wip branch from 974df1f to b7a25eb Compare June 13, 2017 11:34

corngood added 16 commits June 26, 2017 09:26

ncurses: fix cygwin build

33eec8e

help2man: help wrapper find libintl on cygwin

18125e6

bash: fix cygwin build

af16517

perl-LocaleGettext: fix test failure on cygwin by setting $LANG

f3c039f

procps-ng: allow cygwin

a08024b

gpm: allow cygwin

84a4ee5

libiconv: remove old cygwin hacks

260afef

gettext: fix cygwin build

b928404

openssl: fix cygwin build

c580ab4

zlib: fix cygwin build

cbc0db0

stdenv: remove leftover 'system' reference

f81117e

the rest were removed in 1dc6f15

gdbm: fix cygwin build

f4a2351

pkgconfig: fix cygwin build

bf517c1

cygwin: fix doubleFromSystem for cygwin

be75c5d

stdenv: remove unix kernel family

4ac1901

System predicate patterns can now be specified as a list of OR'd attribute sets.

stdenv: disable -fPIC on cygwin

63a1a2a

It's always on, and you get a warning if you specify it

corngood force-pushed the cygwin-wip branch from a06426d to 63a1a2a Compare June 26, 2017 16:48

Ericson2314 merged commit b686fd1 into NixOS:staging Jun 26, 2017

corngood mentioned this pull request Jul 3, 2017

lib: Include darwin in isUnix #27089

Merged

8 tasks

Ericson2314 added the 6.topic: windows label Dec 29, 2017

Ericson2314 mentioned this pull request Feb 21, 2018

[RFC 0023] Support musl libc NixOS/rfcs#23

Merged

		@@ -1,4 +1,4 @@
		{ stdenv, fetchurl }:
		{ stdenv, lib, fetchurl }:

		BSD = { kernel = { families = { inherit (kernelFamilies) bsd; }; }; };
		Unix = [ BSD Linux SunOS Hurd Cygwin ];

Cygwin fixes #26021

Cygwin fixes #26021

Conversation

corngood commented May 23, 2017

Motivation for this change

Things done

mention-bot commented May 23, 2017

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

zimbatm left a comment

Choose a reason for hiding this comment

corngood commented May 25, 2017

zimbatm commented May 26, 2017

corngood commented Jun 9, 2017

Ericson2314 commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corngood commented Jun 9, 2017

Ericson2314 commented Jun 9, 2017

corngood commented Jun 12, 2017 • edited Loading

Ericson2314 Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Jun 12, 2017

corngood commented Jun 12, 2017

Ericson2314 commented Jun 12, 2017 • edited Loading

corngood commented Jun 13, 2017 • edited Loading

Ericson2314 commented Jun 15, 2017

corngood commented Jun 26, 2017

Ericson2314 commented Jun 26, 2017

corngood commented Jun 26, 2017

corngood commented Jun 26, 2017

Ericson2314 commented Jun 26, 2017

vcunat commented Jul 3, 2017

Ericson2314 commented Jul 3, 2017 • edited Loading

corngood commented Jul 3, 2017

vcunat commented Jul 3, 2017

Ericson2314 Jun 9, 2017 •

edited

Loading

corngood commented Jun 12, 2017 •

edited

Loading

Ericson2314 Jun 12, 2017 •

edited

Loading

Ericson2314 commented Jun 12, 2017 •

edited

Loading

corngood commented Jun 13, 2017 •

edited

Loading

Ericson2314 commented Jul 3, 2017 •

edited

Loading