vulkan-validation-layers: 1.1.82.0 -> 1.1.85.0 #51672

acowley · 2018-12-07T18:24:42Z

This requires knock-on upgrades for glslang and spirv-tools.

I have also made the validation layers easier to use:

library files identified by layer definitions now use absolute paths
a VK_LAYER_PATH environment variable is set

Previously, one would have to modify LD_LIBRARY_PATH or install the
derivation in a known location for vulkan-loader to find relevant
files. These changes make using validation layers in a nix-shell work automatically.

Motivation for this change

Things done

acowley · 2018-12-08T03:58:45Z

I had thought @Ralith would be tagged automatically, but in any case they are the listed maintainer for these derivations so I hope they can take a look at these changes.

Ralith

Why not go straight to version 1.1.92.0?

Ralith · 2018-12-08T04:28:31Z

pkgs/development/tools/vulkan-validation-layers/default.nix

@@ -18,6 +19,17 @@ stdenv.mkDerivation rec {

  cmakeFlags = [ "-DGLSLANG_INSTALL_DIR=${glslang}" ];

+  # Help vulkan-loader find the validation layers
+  setupHook = writeText "setup-hook" ''
+    export VK_LAYER_PATH=@out@/share/vulkan/explicit_layer.d


A more graceful approach here would be to append @out@/share to XDG_DATA_DIRS, therefore not clobbering the visibility of any other layers in the environment.

That is a great suggestion! I'll check to see if it works.

It works perfectly, thanks again.

Ralith · 2018-12-08T04:29:31Z

pkgs/development/tools/vulkan-validation-layers/default.nix

@@ -18,6 +19,17 @@ stdenv.mkDerivation rec {

  cmakeFlags = [ "-DGLSLANG_INSTALL_DIR=${glslang}" ];

+  # Help vulkan-loader find the validation layers
+  setupHook = writeText "setup-hook" ''


Should this really be a setupHook? Layers are runtime dependencies, not build-time ones. There's also a question of host vs. target compatibility.

The setupHook affects the environment in a nix-shell, so if I am building and running a program under development in a nix-shell, this lines things up to work.

That's what shellHook is for. Making changes only intended for shells in the setup hook might have unintended side effects.

I found that not to work as I want. Specifically, if you add vulkan-validation-layers to your buildInputs, then enter a nix-shell to work on your program, the environment variable is not updated by shellHook. If you want to work on vulkan-validation-layers itself, by some mechanism like nix-shell '<nixpkgs>' -A vulkan-validation-layers, then, yes, it does get set (though then the use of @out@ is no longer correct).

As a user, I certainly want the layers found when I take vulkan-validation-layers as a dependency, but perhaps I am thinking too narrowly and there is some setup where you only want the layers when working directly on the vulkan-validation-layers derivation.

Ah, right. Well, I remain concerned about how idiomatic this is, but I concede that your approach is eminently useful. Might as well leave it in until someone complains; I suspect validation layers in build environments other than shell environments are a rarity.

acowley · 2018-12-08T04:35:38Z

Regarding the version, I just wanted to bring it in line with the other vulkan pieces in nixpkgs to avoid bundling too many changes into one PR. I thought a version bump on all the pieces could follow this one.

Ralith · 2018-12-08T05:38:39Z

Best squash before merging.

This requires knock-on upgrades for glslang and spirv-tools. I have also made the validation layers easier to use: - library files identified by layer definitions now use absolute paths - the layer definition path is prepended to XDG_DATA_DIRS Previously, one would have to modify LD_LIBRARY_PATH or install the derivation in a known location for vulkan-loader to find relevant files. These changes make using validation layers in a nix-shell work automatically. Use XDG_DATA_DIRS environment variable rather than VK_LAYER_PATH

acowley · 2018-12-08T05:56:26Z

Squashed

alyssais · 2018-12-10T15:33:43Z

pkgs/development/tools/spirv-tools/default.nix

-    rev = "ff684ffc6a35d2a58f0f63108877d0064ea33feb";
-    sha256 = "0ypjx61ksr6vda2iy3kxhyjia5qxf0x4qa4jij0giw9x5rsnga6g";
+    rev = "d5b2e1255f706ce1f88812217e9a554f299848af";
+    sha256 = "18530mpa030ckjhn78z9vbfd35l5jgh3mg22ra6k8gw8zx4wjhsl";
  };
 };

 in

 stdenv.mkDerivation rec {
  name = "spirv-tools-${version}";


Suggested change

name = "spirv-tools-${version}";

name = "spirv-tools-${version}-unstable";

(same goes for glslang)

If a package is not a release but a commit from a repository, then the version part of the name must be the date of that (fetched) commit. The date must be in "YYYY-MM-DD" format. Also append "unstable" to the name - e.g., "pkgname-unstable-2014-09-23".

https://nixos.org/nixpkgs/manual/#sec-package-naming

This change is fine with me, but I'd like to point out that the release status of these packages is quite strange: we are using a released version of vulkan-validation-layers, and that package's source identifies a specific revision of glslang to be good. Likewise, glslang identifies a specific revision of spirv-tools.

This puts us in a troubling spot, because if we rely on releases of the upstream packages (e.g. spirv-tools) rather than revisions identified by the downstream packages (e.g. vulkan-validation-layers), things break (I experienced this myself in preparing this little patch). But if we use all the "known good" revisions starting with a downstream package (like vulkan-validation-layers) and follow the nixpkgs naming schemes, we will have a bunch of unstable versions of things despite these being the "known good" versions required by the downstream packages.

Can we at least get @Ralith to chime in, since they are the maintainer? I am fine with appending unstable, but have a very slight preference to not do so because it has an inaccurate connotation. On the other hand, a good consequence of appending unstable to these "known good" versions is that it would make room for the released versions if somebody needed them, though this does seem ripe for confusion.

Corollary: we may want to add the unstable suffix to a bunch of these packages that flow from the known_good.json files. Should we do that in this PR or split it out into a separate task?

Oh, I see. In that case, I don't think unstable is right here after all – I didn't realise the strange way the versioning worked.

Should we do that in this PR or split it out into a separate task?

If you want to do this, I think you might as well do it here. It's a small change.

I don't have a strong opinion here; vulkan tools/validation layers selecting a specific glslang certainly seems like a declaration that it's stable enough for their purposes, but e.g. shaderc uses entirely different commits, and there doesn't seem to be any pretense of a conventional stability guarantee.

acowley · 2018-12-15T20:06:24Z

I'm not sure how to advance this as it was intended primarily as a usability improvement since I had trouble making the existing packaging work. There is a reasonable argument that this constellation of packages -- vulkan-loader, vulkan-headers, glslang, spirv-tools, spirv-headers, and probably others -- should be reconsidered given the way upstream cuts releases. Namely, upstream effectively treats everything as a submodule, but doesn't use the native git mechanisms for doing so.

It might make sense, then, for nixpkgs to move dependencies into the packages that require them (either as let bindings or separate .nix files in, e.g., the vulkan-validation-layers directory) rather than placing particular revisions into the nixpkgs name space. We would probably want to take care that things like vulkan-headers and vulkan-loader not exist at multiple distinct revisions across nixpkgs, but we can hope that the relevant known_good.json won't demand that.

I'm not going to make a sweeping change as I don't know if there is a broader willingness to do so, and everything other than the validation layers was already working fine. But it seems that this PR is stuck on the derivation naming objection until someone does the necessary refactoring.

I'll try to find a home for the usability changes on NUR or somewhere so anyone else wanting to use the validation layers can have an easier time.

Ralith · 2018-12-15T20:18:01Z

I don't think this (or other version/usability updates) should be blocked on orthogonal organizational questions. I think this should be reopened and merged as-is.

alyssais · 2018-12-15T23:36:07Z

Agreed. I was suggesting a simple cosmetic change and it ended up being way more of a can of worms than a realise. @acowley if you want to reopen I'm happy to merge.

acowley · 2018-12-16T00:32:36Z

Okay, great! I think you’re actually right that this is a lurking problem. I wonder if we should nudge upstream to depend on tagged releases.

Ralith · 2018-12-16T03:33:54Z

I think the right way to go forward in the future is to switch the exposed packages of glslang and spirv-tools to use the release tags they seem to have finally started making (even shaderc has one now!), then use overrides or local definitions as needed in vulkan-* and shaderc. Might be best to pursue that in a separate PR, though.

alyssais · 2018-12-16T11:07:03Z

@GrahamcOfBorg build vulkan-validation-layers

alyssais · 2018-12-16T11:10:21Z

Okay, great! I think you’re actually right that this is a lurking problem. I wonder if we should nudge upstream to depend on tagged releases.

I think the right way to go forward in the future is to switch the exposed packages of glslang and spirv-tools to use the release tags they seem to have finally started making (even shaderc has one now!), then use overrides or local definitions as needed in vulkan-* and shaderc.

Vith of these sound like very good ideas, and I strongly encourage both of you to follow up on them. We shouldn’t have packages pinned to a specific git commit in nixpkgs just because that’s what one dependent package wants. For now, I don’t think that updates should be blocked on that, but it really should be fixed sooner rather than later — if other things start to depend on those packages, them being at arbitrary commits could start to cause problems.

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Dec 7, 2018

Ralith suggested changes Dec 8, 2018

View reviewed changes

acowley force-pushed the vulkan-validation branch from 0fbb424 to 6492af6 Compare December 8, 2018 05:52

Ralith approved these changes Dec 8, 2018

View reviewed changes

alyssais reviewed Dec 10, 2018

View reviewed changes

acowley closed this Dec 15, 2018

acowley reopened this Dec 16, 2018

alyssais merged commit 24e6074 into NixOS:master Dec 16, 2018

acowley deleted the vulkan-validation branch January 27, 2019 19:10

Ralith mentioned this pull request Feb 19, 2019

Update Vulkan packages #55959

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vulkan-validation-layers: 1.1.82.0 -> 1.1.85.0 #51672

vulkan-validation-layers: 1.1.82.0 -> 1.1.85.0 #51672

acowley commented Dec 7, 2018

acowley commented Dec 8, 2018

Ralith left a comment

Ralith Dec 8, 2018

acowley Dec 8, 2018

acowley Dec 8, 2018

Ralith Dec 8, 2018

acowley Dec 8, 2018

Ralith Dec 8, 2018

acowley Dec 8, 2018

Ralith Dec 8, 2018 •

edited

acowley commented Dec 8, 2018

Ralith commented Dec 8, 2018

acowley commented Dec 8, 2018

alyssais Dec 10, 2018 •

edited

acowley Dec 10, 2018 •

edited

acowley Dec 10, 2018

alyssais Dec 10, 2018

alyssais Dec 10, 2018

Ralith Dec 10, 2018

acowley commented Dec 15, 2018

Ralith commented Dec 15, 2018

alyssais commented Dec 15, 2018

acowley commented Dec 16, 2018

Ralith commented Dec 16, 2018

alyssais commented Dec 16, 2018

alyssais commented Dec 16, 2018

	name = "spirv-tools-${version}";
	name = "spirv-tools-${version}-unstable";

vulkan-validation-layers: 1.1.82.0 -> 1.1.85.0 #51672

vulkan-validation-layers: 1.1.82.0 -> 1.1.85.0 #51672

Conversation

acowley commented Dec 7, 2018

Motivation for this change

Things done

acowley commented Dec 8, 2018

Ralith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith Dec 8, 2018 • edited

Choose a reason for hiding this comment

acowley commented Dec 8, 2018

Ralith commented Dec 8, 2018

acowley commented Dec 8, 2018

alyssais Dec 10, 2018 • edited

Choose a reason for hiding this comment

acowley Dec 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acowley commented Dec 15, 2018

Ralith commented Dec 15, 2018

alyssais commented Dec 15, 2018

acowley commented Dec 16, 2018

Ralith commented Dec 16, 2018

alyssais commented Dec 16, 2018

alyssais commented Dec 16, 2018

Ralith Dec 8, 2018 •

edited

alyssais Dec 10, 2018 •

edited

acowley Dec 10, 2018 •

edited