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
wivrn: init at 0.12 #293058
wivrn: init at 0.12 #293058
Conversation
362c731
to
310f891
Compare
64a6b34
to
affee1a
Compare
5788f2b
to
9deebbc
Compare
I did manage to get it to work so it appears that the module is functional. I will open the PR for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution! Some feedback below:
f2c0de5
to
877579f
Compare
Updated with requested changes |
404ff9d
to
ba0dc70
Compare
Oops, didn't get notifications for comments on those 2 open requests. Will work on those tomorrow. |
hi, I think this should only be done after this v0.11 is merged, but here's v0.12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, it looks very good on the surface. Some sharp edges in the review.
|
||
openFirewall = mkEnableOption "the default ports in the firewall for the WiVRn server"; | ||
|
||
defaultRuntime = mkEnableOption '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Non-blocking] How do we know we are the only default runtime? Currently there's only vanilla monado and WiVRn so this isn't pressing yet, but we'll want to make that error nicer at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we be able to detect if there is a conflict (Both modules being enabled as the default runtime) for Monado and WiVRn? As far as I know, the only conflict would be in the active_runtime.json as WiVRn lists its ipc separately from Monado.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the behavior should already be correct if they both try to set environment.etc.…
to different values, but it's not great once there are more runtimes with NixOS modules. Non-blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could have some kind of hardware.openxr
interface where one can configure the default runtime and potentially other openxr-loader options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works, but out of scope for this PR unless @PassiveLemon disagrees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could have some kind of
hardware.openxr
interface where one can configure the default runtime and potentially other openxr-loader options
I have a home-manager module that already does some of this here, but I could try to port it to a NixOS module if we think its necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Source looks good. I'll test it with hardware and approve later (^:
pkgs/by-name/wi/wivrn/package.nix
Outdated
# The library path to the OpenXR runtime requires a relative path from the config file to the binary in the nix store | ||
# The config file is usually located at ~/.config/openxr/1/ but the wivrn module puts it at /etc/xdg/openxr/1/ | ||
# The CMakeList has relative directory paths that cause malformation of the path. https://github.com/Meumeu/WiVRn/issues/47 | ||
# What it is: ../../..//nix/store/... | ||
# What it should be: ../../../../nix/store/... | ||
# Details about the required path here (Section 3): https://monado.freedesktop.org/valve-index-setup.html | ||
patchPhase = '' | ||
substituteInPlace ./server/CMakeLists.txt \ | ||
--replace "../../../" "../../../.." | ||
''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really require a relative path? I wonder if you could just replace ../../../
with am empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested it without relative paths and it does appear to work but I'm not sure if that is intended functionality. I've simply just gone by what Monado/OpenComposite says to do. It would make it a lot easier if a relative path is not required though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not quite sure here. The existing Nix packages for Monado generate a manifest with an absolute path.
According to the OpenXR Loader spec, both absolute and relative paths are supported. I think an absolute path is definitely preferable here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's real nice then haha
I think all that's left to figure out is where to generate the active_runtime.json
so it can be found by Steam
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should attach a link to #293058 (comment) in the comment as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PRESSURE_VESSEL_FILESYSTEMS_RW=/etc/xdg/openxr
in the game's launch options should work. Nothing we can do about that here though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That wouldn't work because PRESSURE_VESSEL_FILESYSTEMS_RW=$XDG_RUNTIME_DIR/wivrn_comp_ipc
has to be defined to use WiVRn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, maybe ask Valve to fix that missing default passthrough then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, that variables should theoretically be able to add a second path by separating it with a colon like so: PRESSURE_VESSEL_FILESYSTEMS_RW="$XDG_RUNTIME_DIR/wivrn_comp_ipc:/etc/xdg/openxr/1/active_runtime.json" %command%
but I cannot get that to work. I'll try to open an issue upstream and see if I can get something done about it there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need wivrn and monado to implement find_package support... |
I see things are in flux again so I'll hold off on the hw testing a bit longer (^: |
Didn't know changing the name of the branch would just close the PR but I guess that makes sense. Anyways, I reopened the same PR here: #299830 |
Description of changes
Adds WiVRn
#278126 (comment)
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.