wine64Packages.unstable: 8.13 -> 8.14 #251357
Conversation
|
@reckenrode Looks like Darwin patches don't apply on Darwin anymore? (I have never used Darwin platforms) |
|
I’m working on a fix. Upstream changed some of the logic, so it no longer users property notation in some places, but it causes the patch to fail to apply there. Adapting the patch is easy, but I also need to do a post-processing pass to avoid an implicit pointer to integer conversion that is an error with clang 16 (in preparation for #241692). |
|
I don’t have write access to the repo, so I can’t push directly to the PR (unless I’m doing it wrong). The following patch should fix the build issues on Darwin. From 95b96f594b946041c391c45c05b1ca9eb71d17d1 Mon Sep 17 00:00:00 2001
From: Randy Eckenrode <[email protected]>
Date: Sat, 26 Aug 2023 08:52:28 -0400
Subject: [PATCH] wine64Packages.unstable: Darwin compatibility fixes
- Wine 8.12 changed the implementation of `macdrv_get_gpus_from_metal`,
causing the patch used by nixpkgs to break. This patch splits that
patch up to apply cleanly depending on the version;
- Silence an implicit pointer to integer conversion warning due to the
above patch (required by the Clang 16 stdenv bump);
- Add the PCSC framework on Darwin, which is required as of Wine 8.14.
Wine 8.14 changes the implementation of `macdrv_get_gpus_from_metal`,
causing the patch to no longer apply cleanly. Splitting the patch allows
only the parts that are still needed to apply cleanly dependin gon the
Wine version being built.
---
pkgs/applications/emulators/wine/base.nix | 19 +++++++++++++---
.../darwin-metal-compat-patch-pre8.12.txt | 22 +++++++++++++++++++
.../emulators/wine/darwin-metal-compat.patch | 21 +-----------------
3 files changed, 39 insertions(+), 23 deletions(-)
create mode 100644 pkgs/applications/emulators/wine/darwin-metal-compat-patch-pre8.12.txt
diff --git a/pkgs/applications/emulators/wine/base.nix b/pkgs/applications/emulators/wine/base.nix
index bc8051d6de5f..e6b31985d647 100644
--- a/pkgs/applications/emulators/wine/base.nix
+++ b/pkgs/applications/emulators/wine/base.nix
@@ -90,7 +90,7 @@ stdenv.mkDerivation ((lib.optionalAttrs (buildScript != null) {
++ lib.optionals tlsSupport [ pkgs.openssl pkgs.gnutls ]
++ lib.optionals (openglSupport && !stdenv.isDarwin) [ pkgs.libGLU pkgs.libGL pkgs.mesa.osmesa pkgs.libdrm ]
++ lib.optionals stdenv.isDarwin (with pkgs.buildPackages.darwin.apple_sdk.frameworks; [
- CoreServices Foundation ForceFeedback AppKit OpenGL IOKit DiskArbitration Security
+ CoreServices Foundation ForceFeedback AppKit OpenGL IOKit DiskArbitration PCSC Security
ApplicationServices AudioToolbox CoreAudio AudioUnit CoreMIDI OpenCL Cocoa Carbon
])
++ lib.optionals (stdenv.isLinux && !waylandSupport) (with pkgs.xorg; [
@@ -103,14 +103,27 @@ stdenv.mkDerivation ((lib.optionalAttrs (buildScript != null) {
patches = [ ]
++ lib.optionals stdenv.isDarwin [
- # Wine requires `MTLDevice.registryID` for `winemac.drv`, but that property is not available
- # in the 10.12 SDK (current SDK on x86_64-darwin). Work around that by using selector syntax.
+ # Wine uses `MTLDevice.registryID` in `winemac.drv`, but that property is not available in
+ # the 10.12 SDK (current SDK on x86_64-darwin). That can be worked around by using selector
+ # syntax. As of Wine 8.12, the logic has changed and uses selector syntax, but it still
+ # uses property syntax in one place. The first patch is necessary only with older
+ # versions of Wine. The second is needed on all versions of Wine.
+ (lib.optional (lib.versionOlder version "8.12") ./darwin-metal-compat-pre8.12.patch)
./darwin-metal-compat.patch
# Wine requires `qos.h`, which is not included by default on the 10.12 SDK in nixpkgs.
./darwin-qos.patch
]
++ patches';
+ # Because the 10.12 SDK doesn’t define `registryID`, clang assumes the undefined selector returns
+ # `id`, which is a pointer. This causes implicit pointer to integer errors in clang 15+.
+ # The following post-processing step adds a cast to `uint64_t` before the selector invocation to
+ # silence these errors.
+ postPatch = lib.optionalString stdenv.isDarwin ''
+ sed -e 's|\(\[[A-Za-z_][][A-Za-z_0-9]* registryID\]\)|(uint64_t)\1|' \
+ -i dlls/winemac.drv/cocoa_display.m
+ '';
+
configureFlags = prevConfigFlags
++ lib.optionals supportFlags.waylandSupport [ "--with-wayland" ]
++ lib.optionals supportFlags.vulkanSupport [ "--with-vulkan" ]
diff --git a/pkgs/applications/emulators/wine/darwin-metal-compat-patch-pre8.12.txt b/pkgs/applications/emulators/wine/darwin-metal-compat-patch-pre8.12.txt
new file mode 100644
index 000000000000..aaf8a1b89bfd
--- /dev/null
+++ b/pkgs/applications/emulators/wine/darwin-metal-compat-patch-pre8.12.txt
@@ -0,0 +1,22 @@
+diff --git a/dlls/winemac.drv/cocoa_display.m b/dlls/winemac.drv/cocoa_display.m
+--- a/dlls/winemac.drv/cocoa_display.m
++++ b/dlls/winemac.drv/cocoa_display.m
+@@ -289,7 +289,7 @@ static int macdrv_get_gpus_from_metal(struct macdrv_gpu** new_gpus, int* count)
+ * the primary GPU because we need to hide the integrated GPU for an automatic graphic switching pair to avoid apps
+ * using the integrated GPU. This is the behavior of Windows on a Mac. */
+ primary_device = [MTLCreateSystemDefaultDevice() autorelease];
+- if (macdrv_get_gpu_info_from_registry_id(&primary_gpu, primary_device.registryID))
++ if (macdrv_get_gpu_info_from_registry_id(&primary_gpu, [primary_device registryID]))
+ goto done;
+
+ /* Hide the integrated GPU if the system default device is a dedicated GPU */
+@@ -301,7 +301,7 @@ static int macdrv_get_gpus_from_metal(struct macdrv_gpu** new_gpus, int* count)
+
+ for (i = 0; i < devices.count; i++)
+ {
+- if (macdrv_get_gpu_info_from_registry_id(&gpus[gpu_count], devices[i].registryID))
++ if (macdrv_get_gpu_info_from_registry_id(&gpus[gpu_count], [devices[i] registryID]))
+ goto done;
+
+ if (hide_integrated && devices[i].isLowPower)
+
diff --git a/pkgs/applications/emulators/wine/darwin-metal-compat.patch b/pkgs/applications/emulators/wine/darwin-metal-compat.patch
index aeee7533bbd4..181b2a0d1a47 100644
--- a/pkgs/applications/emulators/wine/darwin-metal-compat.patch
+++ b/pkgs/applications/emulators/wine/darwin-metal-compat.patch
@@ -1,31 +1,12 @@
diff --git a/dlls/winemac.drv/cocoa_display.m b/dlls/winemac.drv/cocoa_display.m
-index f64a6c0f6ad..6da0391e3fa 100644
--- a/dlls/winemac.drv/cocoa_display.m
+++ b/dlls/winemac.drv/cocoa_display.m
-@@ -289,7 +289,7 @@ static int macdrv_get_gpus_from_metal(struct macdrv_gpu** new_gpus, int* count)
- * the primary GPU because we need to hide the integrated GPU for an automatic graphic switching pair to avoid apps
- * using the integrated GPU. This is the behavior of Windows on a Mac. */
- primary_device = [MTLCreateSystemDefaultDevice() autorelease];
-- if (macdrv_get_gpu_info_from_registry_id(&primary_gpu, primary_device.registryID))
-+ if (macdrv_get_gpu_info_from_registry_id(&primary_gpu, (uint64_t)[primary_device registryID]))
- goto done;
-
- /* Hide the integrated GPU if the system default device is a dedicated GPU */
-@@ -301,7 +301,7 @@ static int macdrv_get_gpus_from_metal(struct macdrv_gpu** new_gpus, int* count)
-
- for (i = 0; i < devices.count; i++)
- {
-- if (macdrv_get_gpu_info_from_registry_id(&gpus[gpu_count], devices[i].registryID))
-+ if (macdrv_get_gpu_info_from_registry_id(&gpus[gpu_count], (uint64_t)[devices[i] registryID]))
- goto done;
-
- if (hide_integrated && devices[i].isLowPower)
@@ -354,7 +354,7 @@ static int macdrv_get_gpu_info_from_display_id_using_metal(struct macdrv_gpu* gp
device = [CGDirectDisplayCopyCurrentMetalDevice(display_id) autorelease];
if (device && [device respondsToSelector:@selector(registryID)])
- ret = macdrv_get_gpu_info_from_registry_id(gpu, device.registryID);
-+ ret = macdrv_get_gpu_info_from_registry_id(gpu, (uint64_t)[device registryID]);
++ ret = macdrv_get_gpu_info_from_registry_id(gpu, [device registryID]);
done:
[pool release];
--
2.40.1
|
- Wine 8.12 changed the implementation of `macdrv_get_gpus_from_metal`, causing the patch used by nixpkgs to break. This patch splits that patch up to apply cleanly depending on the version; - Silence an implicit pointer to integer conversion warning due to the above patch (required by the Clang 16 stdenv bump); - Add the PCSC framework on Darwin, which is required as of Wine 8.14. Wine 8.14 changes the implementation of `macdrv_get_gpus_from_metal`, causing the patch to no longer apply cleanly. Splitting the patch allows only the parts that are still needed to apply cleanly dependin gon the Wine version being built.
|
I’m running
For the first one, it appears Wine is being built without MinGW, which is the default for stable releases. Both of those failures are for 8.0.2, which is the stable release. Darwin should probably be marked broken when MinGW support is disabled. For the second one, it’s very strange. The build process attempts to strip the Wine DLLs during the For the third, this happened with wow64 builds. It may be Wow64 needs some tweaks to build on Darwin. I can take a look at this, but any fixes are going to be made assuming the clang 16 stdenv from #241692. |
|
Thanks! Oh well, a pity. What you would advise to do now (waiting is a valid advice, too)? (Hm, if we don't even have Wine on Hydra, I am not sure if it was broken before the changes…) |
|
I’m going to run nixpkgs-review on this PR with the clang 16 stdenv, but I’ll probably set it up to run overnight due to the amount of time it takes and my desire to do things with my computer this evening. That will give a better picture of what exactly is broken. My expectation is Wine staging/unstable will build, default Wine stable will remain broken due to the MinGW issue, and Wow64 may or may not be broken.
https://hydra.nixos.org/job/nixpkgs/trunk/wine64Packages.unstable.x86_64-darwin It’s been broken on Darwin since 8.13 and 8.0.2 landed. I should get myself added to the maintainers list, so I get pinged on these updates to test on Darwin. I missed the breakage because I haven’t updated my own configs since when 8.10 was current (due to having used them to test the clang 16 stdenv and not wanting to do even more rebuilds).
At this point, probably just wait. If this PR is merged, it won’t make anything worse for Darwin, so if going ahead will help Linux, the other patches will fix things when Darwin is finally able to build Wine again. Using a newer clang stdenv won’t help because it needs a version of |
|
All other things being equal, I like the idea of having the intended-as-latest version of Wine be indeed the latest we have… Waiting a bit if that waiting helps to fix something is fine, but it sounds like no new breakage and also your patch will be in put it in place in preparation for later-stage fixes. Should I first: a) add you as a maintainer, b) mark something as broken on some subset of platforms? (When the problem is a segfault, I don't even know if it translates between two CPU arches for Darwin…) |
Committing this wouldn’t be a regression. It’s not ideal that Wine is broken on Darwin, but it won’t be making things worse.
If you could do that, that’s be great. I don’t know how many other Darwin Wine users there are in nixpkgs, but I’d appreciate the awareness, so I can keep it working.
I’m building under Rosetta 2, but I don’t know that that matters. Most of the Darwin builders are Rosetta 2 anyway. If you want to mark it broken in the meantime, I can open a PR once the stdenv is merged to unmark it broken (or refine it just to be broken when MinGW is disabled). |
|
OK, added you as a maintainer, skipping marking broken if this is not new but might be solved soon. |
Automatic update generated by nixpkgs-update tools. This update was made based on information from passthru.updateScript.
meta.description for wine64Packages.unstable is: An Open Source implementation of the Windows API on top of X, OpenGL, and Unix
meta.homepage for wine64Packages.unstable is: https://www.winehq.org/
Updates performed
To inspect upstream changes
Impact
Checks done (click to expand)
passthru.tests, if any, passedRebuild report (if merged into master) (click to expand)
Instructions to test this update (click to expand)
Either download from Cachix:
(The Cachix cache is only trusted for this store-path realization.)
For the Cachix download to work, your user must be in the
trusted-userslist or you can usesudosince root is effectively trusted.Or, build yourself:
After you've downloaded or built it, look at the files and if there are any, run the binaries:
Pre-merge build results
We have automatically built all packages that will get rebuilt due to
this change.
This gives evidence on whether the upgrade will break dependent packages.
Note sometimes packages show up as failed to build independent of the
change, simply because they are already broken on the target branch.
nixpkgs-review took longer than 45m and timed out
Maintainer pings
cc @avnik @7c6f434c @bendlas @jmc-figueira for testing.