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-validation-layers: 1.1.82.0 -> 1.1.85.0 #51672

Merged
merged 1 commit into from Dec 16, 2018

Conversation

@acowley
Copy link
Contributor

acowley commented Dec 7, 2018

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@acowley

This comment has been minimized.

Copy link
Contributor Author

acowley commented Dec 8, 2018

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.

Copy link
Contributor

Ralith left a comment

Why not go straight to version 1.1.92.0?

pkgs/development/tools/vulkan-validation-layers/default.nix Outdated
@@ -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

This comment has been minimized.

Copy link
@Ralith

Ralith Dec 8, 2018

Contributor

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.

This comment has been minimized.

Copy link
@acowley

acowley Dec 8, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@acowley

acowley Dec 8, 2018

Author Contributor

It works perfectly, thanks again.

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

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

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

This comment has been minimized.

Copy link
@Ralith

Ralith Dec 8, 2018

Contributor

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

This comment has been minimized.

Copy link
@acowley

acowley Dec 8, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@Ralith

Ralith Dec 8, 2018

Contributor

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

This comment has been minimized.

Copy link
@acowley

acowley Dec 8, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@Ralith

Ralith Dec 8, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor Author

acowley commented Dec 8, 2018

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

This comment has been minimized.

Copy link
Contributor

Ralith commented Dec 8, 2018

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 acowley force-pushed the acowley:vulkan-validation branch to 6492af6 Dec 8, 2018
@acowley

This comment has been minimized.

Copy link
Contributor Author

acowley commented Dec 8, 2018

Squashed

@Ralith
Ralith approved these changes Dec 8, 2018
rev = "ff684ffc6a35d2a58f0f63108877d0064ea33feb";
sha256 = "0ypjx61ksr6vda2iy3kxhyjia5qxf0x4qa4jij0giw9x5rsnga6g";
rev = "d5b2e1255f706ce1f88812217e9a554f299848af";
sha256 = "18530mpa030ckjhn78z9vbfd35l5jgh3mg22ra6k8gw8zx4wjhsl";
};
};

in

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

This comment has been minimized.

Copy link
@alyssais

alyssais Dec 10, 2018

Member
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 comment has been minimized.

Copy link
@acowley

acowley Dec 10, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@acowley

acowley Dec 10, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@alyssais

alyssais Dec 10, 2018

Member

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.

This comment has been minimized.

Copy link
@alyssais

alyssais Dec 10, 2018

Member

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.

This comment has been minimized.

Copy link
@Ralith

Ralith Dec 10, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor Author

acowley commented Dec 15, 2018

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.

@acowley acowley closed this Dec 15, 2018
@Ralith

This comment has been minimized.

Copy link
Contributor

Ralith commented Dec 15, 2018

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

This comment has been minimized.

Copy link
Member

alyssais commented Dec 15, 2018

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

This comment has been minimized.

Copy link
Contributor Author

acowley commented Dec 16, 2018

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.

@acowley acowley reopened this Dec 16, 2018
@Ralith

This comment has been minimized.

Copy link
Contributor

Ralith commented Dec 16, 2018

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

This comment has been minimized.

Copy link
Member

alyssais commented Dec 16, 2018

@GrahamcOfBorg build vulkan-validation-layers

@alyssais

This comment has been minimized.

Copy link
Member

alyssais commented Dec 16, 2018

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.

@alyssais alyssais merged commit 24e6074 into NixOS:master Dec 16, 2018
12 checks passed
12 checks passed
nix-build -A vulkan-validation-layers --argstr system x86_64-darwin Build Results
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
nix-build -A vulkan-validation-layers --argstr system aarch64-linux Build Results
Details
nix-build -A vulkan-validation-layers --argstr system x86_64-linux Build Results
Details
@acowley acowley deleted the acowley:vulkan-validation branch Jan 27, 2019
@Ralith Ralith mentioned this pull request Feb 19, 2019
3 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.