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

7zz: cross-compile for mingw #217472

Merged
merged 2 commits into from
Feb 21, 2023
Merged

7zz: cross-compile for mingw #217472

merged 2 commits into from
Feb 21, 2023

Conversation

AtnNn
Copy link
Contributor

@AtnNn AtnNn commented Feb 21, 2023

Description of changes

Fixes all build errors in pkgsCross.mingwW64._7zz on x86_64 Linux.

$ nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
$ wine result/bin/7zz.exe
7-Zip (z) 22.01 (x64) : Copyright (c) 1999-2022 Igor Pavlov : 2022-07-15
Usage: 7zz <command> [<switches>...] <archive_name> [<file_names>...] [@listfile]
[...]
Things done
  • Built on platform(s)
    • x86_64-linux
    • 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/)
  • 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.

pkgs/tools/archivers/7zz/default.nix Outdated Show resolved Hide resolved
@AtnNn AtnNn requested review from flokli and removed request for peterhoeg, anna328p and 06kellyjac February 21, 2023 10:36
@flokli
Copy link
Contributor

flokli commented Feb 21, 2023

Successfully compiled nix-build -A pkgsCross.mingwW64._7zz and pkgsCross.mingw32._7zz.

I tried running the resulting result/bin/7zz.exe, with wine/wine64, but they complained about mcfgthread-12.dll. I'd assume that library is available on a real windows, so I didn't test any further :-D

@flokli flokli merged commit 81e45cc into NixOS:master Feb 21, 2023
@@ -109,7 +121,7 @@ stdenv.mkDerivation rec {
# the unRAR compression code is disabled by default
lib.optionals enableUnfree [ unfree ];
maintainers = with maintainers; [ anna328p peterhoeg jk ];
platforms = platforms.unix;
platforms = platforms.unix ++ platforms.windows;
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for working on this.

One question though. Is platforms.windows correct here?

nix-repl> lib.platforms.windows
[ "i686-cygwin" "x86_64-cygwin" "x86_64-windows" "i686-windows" ]

I don't know much about windows builds but it seems like Cygwin is a bit different to MinGW
Is "x86_64-cygwin" actually under isMinGW?

isCygwin       = { kernel = kernels.windows; abi = abis.cygnus; };
isMinGW        = { kernel = kernels.windows; abi = abis.gnu; };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cygwin and MinGW are indeed very different. I don't think it matters here because platforms.unix already includes Cygwin.

Copy link
Member

Choose a reason for hiding this comment

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

so it should have stayed platforms.unix and not added platforms.windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The two mingw doubles were needed, which platforms.windows provides.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your question makes sense. Perhaps there should be a platforms.mingw, using that would have made the intention of the change clearer.

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