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

vkBasalt: init at 0.3.2.2 #92401

Closed
wants to merge 1 commit into from
Closed

Conversation

kira-bruneau
Copy link
Contributor

Motivation for this change

Closes #84684

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/)
    No binary files, tested through use of Vulkan game
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Rendering The Legend of Zelda: Breath of the Wild through Cemu with ASCII shader:
Screenshot from 2020-07-05 23:46:15

};

# Currently doesn't support 32bit Vulkan games on 64bit systems
# See https://github.com/KhronosGroup/Vulkan-Loader/issues/155
Copy link
Contributor

@jtojnar jtojnar Jul 6, 2020

Choose a reason for hiding this comment

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

This will need to be absolute either way. Maybe open a PR against vkbasalt?

I am not familiar with Vulkan layers but I guess you would do something like add ${pkgsi686Linux.vkbasalt}/share to VK_LAYER_PATH environment variable for 32-bit games..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using an absolute path by default would break multi lib compatibility on other systems, so I don't think the vkBasalt maintainer would accept the change.

It also wouldn't work to use VK_LAYER_PATH since the Vulkan loader does deduplication and will select the first JSON file it finds for the layer name. I suppose we can try appending the architecture to the name to workaround this.

@kira-bruneau kira-bruneau changed the title vkbasalt: init at 0.3.2.1 vkBasalt: init at 0.3.2.1 Jul 6, 2020
@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Jul 6, 2020

@jtonar Do you think the workaround I just added to support 32bit Vulkan applications is an acceptable approach?

@jtojnar
Copy link
Contributor

jtojnar commented Jul 6, 2020

Why link it into a single package instead of using two separate ones (possibly with manifest renaming to avoid conflicts).

@kira-bruneau
Copy link
Contributor Author

@jtojnar It's mostly for convience and constitency with how it's packaged on Arch. Do you think it would be better to have to install both versions separately?

@jtojnar
Copy link
Contributor

jtojnar commented Jul 6, 2020

Yeah, separate packages sound more idiomatic to me. We do not need multilib since Nix supports effortless cross-platform builds.

@kira-bruneau kira-bruneau force-pushed the vkbasalt branch 2 times, most recently from d93eb1c to cd247e4 Compare July 6, 2020 23:10
@kira-bruneau
Copy link
Contributor Author

I had to do some more platform specific renaming so both the 32bit and 64bit variant can be installed together.

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Jul 11, 2020

@jtojnar After coming back to this to update it to 0.3.2.2, I'm not really sold on the idea of using separate packages. It requires more patching just so the two packages can be installed together. I think it would make sense to use separate packages if all the required functionality was provided by just one package, but vkBasalt is designed with the intention that you have both builds on a 64bit system. I'm leaving it with the separate package approach for now, but I have the single package approach up in my NUR repo for comparison.

EDIT: I noticed that you mention that we don't need multilib, do you mean using something like multiStdenv? Since I'm building both packages with platform specific stdenvs, just linking the 32bit layer into the 64bit one.

@kira-bruneau kira-bruneau changed the title vkBasalt: init at 0.3.2.1 vkBasalt: init at 0.3.2.2 Jul 11, 2020
This was referenced Jul 13, 2020
@kira-bruneau
Copy link
Contributor Author

Version 0.3.2.3 now uses $LIB to load the appropriate library when compiled with -Dappend_libdir_vkbasalt=true. This would be a better way to handle both 32bit & 64bit library loading, but it doesn't currently work on NixOS: #101597.

For now, I will just close this PR and reopen a new one once #101597 is fixed.

@TheEvilSkeleton
Copy link

@MetaDark you might be interested in this: DadSchoorse/vkBasalt#123 (comment)

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Nov 18, 2020

@TheEvilSkeleton Thanks, this could be useful if we want to create a NixOS module for system-wide configuration, but right now there's still the problem with selecting the 32bit vs 64bit library when running a 32bit or 64bit executable. This can be hacked around by creating two Vulkan layers for each build like in my original commit, but a proper solution is blocked by #101597.

@Atemu Atemu mentioned this pull request Nov 27, 2020
10 tasks
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.

vkBasalt
3 participants