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

Jump to bottom

wine: add MinGW-w64 support #103358

Merged
merged 1 commit into from Nov 14, 2020
Merged

wine: add MinGW-w64 support #103358

merged 1 commit into from Nov 14, 2020

Conversation

kira-bruneau
Copy link
Contributor

@kira-bruneau kira-bruneau commented Nov 10, 2020 •
edited

Motivation for this change

Makes it easier to compile wine with MinGW-64 support (See #103102)

Things done

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)

  • Built on platform(s)

    • NixOS
    • macOS
    • other Linux distributions

  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)

  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"

  • Tested execution of all binary files (usually in ./result/bin/)

  • Determined the impact on package closure size (by running nix path-info -S before and after)

    Currently, the closure size is dramatically increased by this change:

    • wine32 (minimal): 829.3M -> 2.6G
    • wine64 (minimal): 912.5M -> 2.6G
    • wineWow (minimal): 1.5G -> 4.9G

    It looks like development header files from mingw-64-gcc are being referenced in all DLLS under lib/wine. For example, these paths are referenced in the wine32 build:

    /nix/store/4pif79nqpcigcrq3934p0jaijq24dy3v-i686-w64-mingw32-stage-final-gcc-debug-9.3.0/i686-w64-mingw32/sys-include
/nix/store/lwlaj95fph6gbvvcnnzg1pws9vqa1np7-mingw-w64-6.0.0-i686-w64-mingw32-dev/include
/nix/store/xrswl3dhfjb3zi4dri7ijwzp2ql4zjkv-mingw-w64-6.0.0-i686-w64-mingw32-headers/include
/nix/store/xrswl3dhfjb3zi4dri7ijwzp2ql4zjkv-mingw-w64-6.0.0-i686-w64-mingw32-headers/include/psdk_inc

    Once binutils 2.34 is merged to master we can re-enable stripping to avoid this closure size increase.

  • Ensured that relevant documentation is up to date

  • Fits CONTRIBUTING.md.

@avnik @bendlas @7c6f434c @cawilliamson

@ofborg ofborg bot requested review from avnik and bendlas November 10, 2020 23:08
avnik
avnik reviewed Nov 11, 2020
View reviewed changes
Copy link
Contributor

@avnik avnik left a comment

Choose a reason for hiding this comment

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

Also would be nice, if build with mingwW64Support=true will fail, if can't find suitable compilers etc.

Regarding closure sizes/dependencies, I suspect it because debug info not stripped, and idk how can be stripped from cross-compiled dlls.

pkgs/misc/emulators/wine/packages.nix Outdated Show resolved Hide resolved
@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Nov 12, 2020 •
edited

@avnik @7c6f434c It looks like the fixup phase actually removes the reference to the debug headers in PE files when stripping is enabled, but it was disabled:

https://github.com/NixOS/nixpkgs/blob/c3d3ba47094dedfa111d3284e856f08830eb9c65/pkgs/misc/emulators/wine/base.nix#L94-L99

With stripping & MinGW-64 support enabled, a full wineWow build closure size gets reduced from the original 3.9GB down to 3.0GB. The problem is that strip resets the DOS stub, but wine stores information in the DOS stub to identify builtin & placeholder DLLs.

lib/wine/kernelbase.dll:
dontStrip = true vs dontStrip = false
image

We could try to workaround this, but this is a bug with strip and should probably be fixed upstream.

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Nov 12, 2020 •
edited

@avnik @7c6f434c Oh wait, it looks like this bug was already fixed in bminor/binutils-gdb@70cf683 and is available starting in binutils 2.34. Should we wait until at least binutils 2.34 is submitted to nixpkgs to complete this PR?

@avnik
Copy link
Contributor

avnik commented Nov 12, 2020

I wouldn't consider it as a blocker, until this option off by default.

Regarding variable naming -- I bet you can rename it to just "mingw" or "mingwCross" without any clashing with something.

@kira-bruneau kira-bruneau changed the title [WIP] wine: add MinGW-w64 support wine: add MinGW-w64 support Nov 12, 2020
@kira-bruneau
Copy link
Contributor Author

@avnik Ok sounds good. I've renamed all instances of mingwW64 -> mingw, but I wasn't quite sure how to approach asserting the GCC packages for mingwSupport since they're passed through an array. We don't currently assert any other packages based on the supportFlags, so do you think the PR is fine as it is?

@ofborg ofborg bot requested a review from avnik November 12, 2020 22:56
@7c6f434c 7c6f434c merged commit f344274 into NixOS:master Nov 14, 2020
@kira-bruneau kira-bruneau mentioned this pull request Nov 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@7c6f434c 7c6f434c Awaiting requested review from 7c6f434c

@bendlas bendlas Awaiting requested review from bendlas

@avnik avnik Awaiting requested review from avnik

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kira-bruneau @avnik @7c6f434c