zlib: use default configure script on windows

[greg-hellings]

This avoids the pitfalls of win32/Makefile.gcc (which prevents building
on compilers other than gcc without patching and has non-standard
installation behavior) and fixes cross compilation for ucrtAarch64

Things done

• Built on platform:
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• Tested, as applicable:
  • NixOS tests in nixos/tests.
  • Package tests at passthru.tests.
  • Tests in lib/tests or pkgs/test for functions and "core" functionality.
• Ran nixpkgs-review on this PR. See nixpkgs-review usage.
• Tested basic functionality of all binary files, usually in ./result/bin/.
• Nixpkgs Release Notes
  • Package update: when the change is major or breaking.
• NixOS Release Notes
  • Module addition: when adding a new NixOS module.
  • Module update: when the change is significant.
• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[acid-bong]

Change target branch to staging (and also rebase your branch onto it), mass rebuilds go there

[greg-hellings]

Change target branch to staging (and also rebase your branch onto it), mass rebuilds go there

Done

[nix-owners]

The PR's base branch is set to master, but 259 commits from the staging branch are included. Make sure you know the right base branch for your changes, then:

• If the changes should go to the staging branch, change the base branch to staging
• If the changes should go to the master branch, rebase your PR onto the merge base with the master branch:

  # git rebase --onto $(git merge-base upstream/master HEAD) $(git merge-base upstream/staging HEAD)
  git rebase --onto 6a506984e35eedbcc7381d2353a1f0625a5cc183 b72dfdf5d87e1ad9a8982d9ccc77abffb6e177fb
  git push --force-with-lease

[Ericson2314]

Can this be a symlink instead like before?

[greg-hellings]

I have written about the details of why extensively in other PRs related to Windows cross builds, but the short answer is no. DLL files properly belong under the bin/ folder on Windows, not symlinked there.

[RossSmyth]

I thought there was a setup hook that did this for you, but I guess not.

[greg-hellings]

I thought there was a setup hook that did this for you, but I guess not.

Most packages seem to have it automatically done, but I believe that is because of updates to autotools over the years and setting the destination folders properly nowadays. In the early days of MinGW, the post fixup had to be done to every single package as autotools didn't have any real knowledge of DLL files. However, zlib uses its own homegrown system so as not to rely on autotools, so I think that's why it is not handled by the standard scripts and options. If there is an automatic hook that would handle this properly, then I would be happy to use it, instead.

[RossSmyth]

It happens with my Rust projects, which do not use autotools. Looks like it's called win-dll-link

[Ericson2314]

If it's fine to symlink transitive DLLs I don't get why it's not fine to symlink DLLs from lib.

I would but the static libs / stub libs in $dev/lib, and the DLLs in $out/lib.

[RossSmyth]

Can also change the hook (in a future PR)

[greg-hellings]

DLLs properly belong in the bin/ folder on Windows targets. This is because Windows has an extremely limited search path for loading dynamic libraries, but it will always look in the same directory alongside the executing binary. Therefore, the DLLs do not belong in $out/lib but instead belong in $out/bin. This has been established practice for the MinGW packages due to the limitations in Windows search paths and it is how every other package in pkgsCross.mingwW64 handles it, it's how autotools handles it, it's how CMake handles it. The final target's $out/bin when building a .exe gets the dependency and transitive dependency DLL files symlinked into place because there is no equivalent to setting LD paths for Windows. There simply is no precedent anywhere else, including in other nixpkgs Windows targets, for putting the DLL file in lib/. It doesn't belong there. It is not a linker-related file, it is a binary executable on Windows.

Stub libraries and fully static libraries end up in lib/ because they are used at link time but are not necessary to distribute alongside binary files unless you are intentionally distributing build products to enable a downstream dev environment. They could live in $dev/lib or $out/lib depending on the purpose of the parent package, but that's a moot discussion here.

The only reason this difference is exposed here is because zlib has a custom, hand-rolled build system instead of using something like autotools. This change is just maintaining how the library already works, but enabling it to be built for Windows with LLVM compilers. This move is not introducing anything new or altering existing behavior. Here is the current output of building pkgsCross.mingwW64.zlib without this patch:

We really should put this information up in the packaging guidelines somewhere so it doesn't need to be reiterated on every pull request.

[RossSmyth]

I am very aware how windows search paths work. I am just wondering out loud if we can automate it, because us devs who care about Windows is a much much smaller population than the other major platforms.

[Ericson2314]

#431820

I am opening a separate issue for this point, which is a general disagreement on policy separate from zlib in particular.

[RossSmyth]

fwiw you can probably reduce the # of rebuilds if you gate the patch.

Suggested change

	patches = [
	./mingw-shared.patch
	];
	patches = lib.optional stdenv.hostPlatform.isMingw ./mingw-shared.patch;

[RossSmyth]

Unsure if it really matters.

[greg-hellings]

That has been suggested before on other cross build package fixes and was found to still cause rebuilds. I believe this is because previously it was null and the conditional will return an empty array when false. If there was already a list of patches, gating it like that would prevent rebuilds but when adding a new patches field it doesn't seem to have any beneficial effect, sadly.

[RossSmyth]

If that's the case then you could try it as an optionalAttrs instead, or an if ... then ... else null

[greg-hellings]

Is that preferred? At other times I've been suggested to leave it in place if the patch file is cross-platform friendly, but if you want it conditional then I'm happy to change it here.

[greg-hellings]

Looking through the code this doesn't really improve anything in terms of the rebuilds. The three differences are still going to all result in a change - the unconditional list, the conditional list, or the conditional null will all be read as different inputs for the simple fact that they have a key defined at all, even if it is defined as null.

However, now that the code is in with the conditional, it at least reduces the likelihood of a breakage to other targets resulting from this patch having unwanted side effects.

[rhelmot]

I've been told that conditionally applied patches are generally not desired in nixpkgs since we want to catch the breakage if a patch no longer applies to a new version of the source as soon as possible. Conditional patches are acceptable if the patch's behavior would affect other systems' behavior in undesirable ways.

[greg-hellings]

That is consistent with what I have been told in the past, but I haven't kept pace with the latest in the guidelines for that. Seeing as this commit has been languishing since December when it was first submitted and it keeps getting the the point of consensus and then ignored and not merged, I will do absolutely anything a reviewer says which might finally convince one of them to merge it at this point, though.

[RossSmyth]

I would merge it if I could

[doronbehar]

I'm sorry @greg-hellings that this has been held up so long, and now there are merge conflicts. There are issues with zlib on Windows now which are more urgent, and should be fixed here:

• zlib: disable static output on windows #490607

I think we can look into this patch of yours afterwards. FYI, see also:

• zlib: simplify arguments & explain --shared & --static #490608

[alexfmpe]

This is unblocked now, right?