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

MoltenVK: init at 1.1.7 #158534

Merged
merged 7 commits into from
Feb 17, 2022
Merged

MoltenVK: init at 1.1.7 #158534

merged 7 commits into from
Feb 17, 2022

Conversation

reckenrode
Copy link
Contributor

Motivation for this change

I’m working on getting DXVK and WINE to build on Darwin, but first I need MoltenVK. This derivation is currently impure. Once x86_64-darwin has updated its SDK, it should build. I’ve taken pains to structure the build phase so it should build with xcbuild.

Since this is a library, I tested two applications. The first was CrossOver using an x86_64-darwin build of MoltenVK running under Rosetta 2 on my MBP. The other was vkQuake. Once this is merged, I’ll submit PRs to update vkQuake and the Vulkan SDK stuff to build on Darwin.

Screenshots (Why doesn’t GitHub support WebP? ☹️)

FF XIV running under CrossOver (x86_64-darwin via Rosetta 2)

ffxiv

vkQuake (aarch64-darwin)

vkquake

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/)
  • 22.05 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@reckenrode
Copy link
Contributor Author

I should add that I originally wrote this derivation to build using xcbuild. It worked, but it only built on aarch64-darwin. I tried to keep it (more or less) compatible with xcbuild even though it’s currently impure, so once x86_64-darwin gets a newer SDK, I can switch it over to building as a pure derivation.

@reckenrode
Copy link
Contributor Author

I pushed a change to follow upstream and use MVK_spirv_cross for the spirv-cross namespace.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/719

@reckenrode
Copy link
Contributor Author

Pushed another change to vendor the DXVK patch. It’s sourced from Gcenx/MoltenVK, which appears to rebase on new releases. I don’t know how GitHub handles commits that aren’t referenced on a branch anymore, and the patch doesn’t seem to change from release to release, so it’s easier to just vendor it to avoid potentially having it go missing.

@reckenrode
Copy link
Contributor Author

Result of nixpkgs-review pr 158534 run on aarch64-darwin 1

@reckenrode
Copy link
Contributor Author

Result of nixpkgs-review pr 158534 run on x86_64-darwin 1

@Gcenx
Copy link

Gcenx commented Feb 9, 2022

I’d recommend not applying the hacks for DXVK by default as this does cause issues for other applications/games that try to call the faked Vulkan extensions.

edit:
Seems the DXVK patch is applied conditionally, guess my message can be ignored.

@reckenrode
Copy link
Contributor Author

Latest push is just running nixpkgs-fmt.

@reckenrode
Copy link
Contributor Author

I’d recommend not applying the hacks for DXVK by default as this does cause issues for other applications/games that try to call the faked Vulkan extensions.

edit: Seems the DXVK patch is applied conditionally, guess my message can be ignored.

I can see an argument that the DXVK patch should be moved to the WINE derivation, which would then use overrideAttrs to apply the patch. I think that risks getting messy (e.g., if the patch gets out of sync), but I’m not set against it if that would be a better approach. Otherwise, you’re right. I don’t want to break apps by advertising extensions that aren’t actually implemented (hence the default to being off).

@Gcenx
Copy link

Gcenx commented Feb 9, 2022

I can see an argument that the DXVK patch should be moved to the WINE derivation, which would then use overrideAttrs to apply the patch. I think that risks getting messy (e.g., if the patch gets out of sync), but I’m not set against it if that would be a better approach. Otherwise, you’re right. I don’t want to break apps by advertising extensions that aren’t actually implemented (hence the default to being off).

I’d forgo the patch in this keeping it inline with upstream and building MoltenVK with the DXVK patch from within the later wine package.

pkgs/os-specific/darwin/MoltenVK/default.nix Outdated Show resolved Hide resolved
pkgs/os-specific/darwin/MoltenVK/default.nix Outdated Show resolved Hide resolved
@@ -148,6 +148,8 @@ impure-cmds // appleSourcePackages // chooseLibs // {

lsusb = callPackage ../os-specific/darwin/lsusb { };

MoltenVK = callPackage ../os-specific/darwin/MoltenVK { };
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
MoltenVK = callPackage ../os-specific/darwin/MoltenVK { };
moltenvk = callPackage ../os-specific/darwin/moltenvk { };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn’t sure whether to follow the style of other frameworks or everything else. I’ve made the change. Thanks for the feedback!

@reckenrode
Copy link
Contributor Author

I pushed changes reflecting your feedback @SuperSandro2000 and @Gcenx. Thanks!

Also pinging @Ralith since this PR now includes several of his packages.

@reckenrode
Copy link
Contributor Author

Result of nixpkgs-review pr 158534 run on aarch64-darwin 1

2 packages built:
  • spirv-tools
  • vulkan-headers

@reckenrode
Copy link
Contributor Author

Result of nixpkgs-review pr 158534 run on x86_64-darwin 1

2 packages built:
  • spirv-tools
  • vulkan-headers

@reckenrode
Copy link
Contributor Author

Result of nixpkgs-review pr 158534 run on aarch64-linux 1

@reckenrode
Copy link
Contributor Author

Result of nixpkgs-review pr 158534 run on x86_64-linux 1

Copy link
Contributor

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

The trivial changes to the existing Vulkan packages LGTM. Did not deeply investigate the moltenvk package itself; would be nice if it could use the standard nixpkgs builds of its dependencies.

@reckenrode
Copy link
Contributor Author

would be nice if it could use the standard nixpkgs builds of its dependencies.

I started to go down that route (meaning bumping nixpkgs deps as necessary), but most of the pinned dependencies don’t correspond to releases or tags. My guess is they’re tracking HEAD, and the commits are whatever is current at the time. It didn’t seem like a good idea to bump them to some random commit. At least I can still use the nixpkgs derivations and override the commits.

@Gcenx
Copy link

Gcenx commented Feb 11, 2022

The Vulkan-Headers and Vulkan-Tools repos have both tagged releases for the new SDK

No, they do not. SDK releases are tagged sdk-.

That’s no longer the case KhronosGroup/Vulkan-Headers@1dace16 v1.3.204

KhronosGroup/Vulkan-Tools@bb32aa1 was a last min bug fix before v1.3.205 so better to just use v1.3.205

@reckenrode
Copy link
Contributor Author

The Vulkan-Headers and Vulkan-Tools repos have both tagged releases for the new SDK

No, they do not. SDK releases are tagged sdk-.

Okay. Maybe I’m not understanding. What’s the difference between e.g., sdk-1.2.298 and v1.2.298?

@Ralith
Copy link
Contributor

Ralith commented Feb 11, 2022

sdk-* are SDK releases; plain version number tags are updates to a spec version, which are more frequent than SDK releases and may not include changes not due to the spec (e.g. bugfixes).

@Ralith
Copy link
Contributor

Ralith commented Feb 12, 2022

It should probably be noted explicitly that SDK releases are of particular interest to us because they're what undergo careful testing and are deemed suitable for wide distribution by Khronos, whereas anything else is basically just an unstable dev build.

@reckenrode
Copy link
Contributor Author

reckenrode commented Feb 12, 2022

Thanks for the explanation. I think I understand now. Once the SDK is released, and the corresponding packages in nixpkgs are updated, it seems reasonable to update MoltenVK to use those instead.

Also, it looks like this is the tracking issue for the new release. The only thing left on the list appears to be the SDK itself.

@reckenrode
Copy link
Contributor Author

I pushed a couple of commits to enable vulkan-loader to build on Darwin, which is a dependency of vkd3d and needed for Wine to support Vulkan on Darwin.

@Ralith Let me know if I should change my approach. I propagated the MoltenVK dependency because seemed like the easiest way to minimize the impact of enabling Darwin support in other packages that support MoltenVK upstream (but have it disabled currently in nixpkgs).

@Gcenx
Copy link

Gcenx commented Feb 16, 2022

I pushed a couple of commits to enable vulkan-loader to build on Darwin, which is a dependency of vkd3d and needed for Wine to support Vulkan on Darwin.

@Ralith Let me know if I should change my approach. I propagated the MoltenVK dependency because seemed like the easiest way to minimize the impact of enabling Darwin support in other packages that support MoltenVK upstream (but have it disabled currently in nixpkgs).

The Vulkan-loader isn’t a build dep for vkd3d on macOS it straight used MoltenVK

vkd3d is mostly useless on macOS anyway, only currently used for WineD3D Vulkan for dx10/11 64Bit not DX12

@reckenrode
Copy link
Contributor Author

reckenrode commented Feb 16, 2022

The Vulkan-loader isn’t a build dep for vkd3d on macOS it straight used MoltenVK

The vkd3d package has vulkan-loader as a dependency in nixpkgs because it also supports building on Linux.

What I’m trying to avoid is having every single Vulkan-using package in nixpkgs need both a dependency on Vulkan-Loader and on MoltenVK to support both Linux and Darwin. Darwin isn’t a tier 1 platform in nixpkgs, and requiring more work to support it means there’s a chance something that could build on Darwin “for free” won’t, and the package will be marked as Linux-only or broken on Darwin. What the propagatedBuildInputs mechanism does is make that dependency available to dependent packages (normally dependencies are not propagated to dependents).

On the other hand, @Ralith is responsible for the Vulkan packages. If he’d prefer it not be done that way, I’m happy to update the patch not to propagate MoltenVK and update everything that needs it to take it as an explicit dependency instead.

@Gcenx FWIW, I opened PR #160256 to update Wine 64 in nixpkgs to work better on Darwin. Not sure how interested you are in that. It’s a bit gnarly due to how Nix goes about building things.

@reckenrode
Copy link
Contributor Author

I pushed an update per the feedback from @Ralith. It removes the propagatedBuildInputs and specifies the SYSCONFDIR as {darwin.moltenvk}/share.

@ofborg ofborg bot requested a review from Ralith February 16, 2022 23:36
Copy link
Contributor

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Vulkan infra changes LGTM.

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
@ofborg ofborg bot requested a review from Ralith February 17, 2022 00:43
@SuperSandro2000 SuperSandro2000 merged commit 59e6ece into NixOS:master Feb 17, 2022
@reckenrode reckenrode deleted the moltenvk branch May 11, 2022 23:06
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.

5 participants