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

vulkan-tools-lunarg: init at 1.2.141.0 #103957

Merged
merged 3 commits into from Nov 17, 2020
Merged

Conversation

expipiplus1
Copy link
Contributor

@expipiplus1 expipiplus1 commented Nov 16, 2020

Requires some small changes to vulkan-validation-layers on which it
depends.

I chose the name vulkan-tools-lunarg to keep it alphabetically near its
sister packages, and to distinguish it from the other vulkan-tools.

Things done

I've tested that the binaries don't crash on start

The setup hook exposes the layers correctly to the vulkan loader and I've tested a couple of the layers with a vulkan program.

  • 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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@expipiplus1
Copy link
Contributor Author

oh, @Ralith please feel free to add yourself to the maintainers list if you'd like :D

These are dependencies of other layers such as the layers in LunarG Vulkan Tools
@expipiplus1
Copy link
Contributor Author

For the license field, it's mainly asl20, but some more permissively licensed things: https://github.com/LunarG/VulkanTools/blob/master/LICENSE.txt#L1-L3

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

pkgs/tools/graphics/vulkan-tools/default.nix Outdated Show resolved Hide resolved
pkgs/tools/graphics/vulkan-tools-lunarg/default.nix Outdated Show resolved Hide resolved
# The version must match that in vulkan-headers
inherit version;

src = fetchFromGitHub {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
src = fetchFromGitHub {
src = (assert versoin == vulkan-headers.version; fetchFromGitHub {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done!

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.

Good stuff! Kinda confusing naming, but I guess that's upstream's fault. I wouldn't mind having a description field that disambiguates them with a little more detail.

@expipiplus1
Copy link
Contributor Author

@Ralith, Would it be possible for you to write a comment explaining the dontPatchElf and libPath lines? I just copied them from the other layer derivation. It kind of makes sense given the dynamic loading these things do but it would be nice to have that written down somewhere.

Use the github repo here as the homepage as lunarg.com has heaps of
stuff besides these tools.
@expipiplus1
Copy link
Contributor Author

I've made some changes to the metadata of both packages and it should be a little clearer, although given the similarity of both project and their similar naming there's only so much that can be done...

@ofborg ofborg bot requested a review from Ralith November 17, 2020 07:36
@doronbehar doronbehar merged commit 6bd360b into NixOS:master Nov 17, 2020
@expipiplus1
Copy link
Contributor Author

expipiplus1 commented Nov 17, 2020 via email

@Ralith
Copy link
Contributor

Ralith commented Nov 17, 2020

@Ralith, Would it be possible for you to write a comment explaining the dontPatchElf and libPath lines?

I'm not sure, actually. @CajuM wrote those, maybe they can help?

@CajuM
Copy link

CajuM commented Nov 17, 2020

Hello, I added the library path because vulkaninfo opens libvulkan(found in vulkan-loader) using dlopen. It is documented just above -DCMAKE_INSTALL_RPATH. I added dontPatchELF = true because vulkan-loader would be removed from RPATH without it.

@expipiplus1 expipiplus1 deleted the joe-vulkan branch November 18, 2020 04:28
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