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

dxvk: refactor based on post-merge feedback and bump to 1.10.1 #163474

Merged
merged 3 commits into from
Mar 27, 2022

Conversation

reckenrode
Copy link
Contributor

Description of changes

I received some post-merge feedback on #161522 raising concerns that Darwin could block updates on Linux. I’ve refactored the derivation to address those concerns without cluttering up the derivation too much or requiring separate upstream sources for Darwin and Linux.

I also moved the DXVK compatibility patch to a passthru attribute on darwin.moltenvk. Per @Gcenx, I do not want to provide a package for a patched MoltenVK because it should never be used except with DXVK. Both derivations have several comments emphasizing this. The patch was moved in case different versions of MoltenVK require their own patches.

@Atemu pinging you to see if this addresses your concerns

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.

- Move the synchronization primitive changes to their own patch, so it
  can be applied conditionally on Darwin. Also, document why this change
  is needed; and
- Refactor how `src` is handled to support allowing Darwin and Linux to
  diverge in case the Darwin patches do not apply to the latest version.
  This should address some post-merger concerns that were raised about
  Darwin’s blocking updates for Linux.
While it’s unlikely, it’s possible that different MoltenVK versions
could require their own compatability patches. Support that by making
the `moltenvk` derivation provide the patch via `passthru`. There is no
package with the patch applied because the patch should never be used by
anything other than DXVK.
@reckenrode reckenrode mentioned this pull request Mar 9, 2022
13 tasks
@reckenrode
Copy link
Contributor Author

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

1 package built:
  • dxvk

@reckenrode
Copy link
Contributor Author

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

1 package built:
  • dxvk

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

@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/791

@reckenrode
Copy link
Contributor Author

Bumped DXVK to 1.10.1 while the PR is open.

@reckenrode reckenrode changed the title dxvk: refactor based on post-merge feedback dxvk: refactor based on post-merge feedback and bump to 1.10.1 Mar 27, 2022
@SuperSandro2000 SuperSandro2000 merged commit 1bf32e4 into NixOS:master Mar 27, 2022
@reckenrode reckenrode deleted the dxvk-refactor 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.

None yet

4 participants