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

wivrn: init at 0.19 #316975

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

wivrn: init at 0.19 #316975

wants to merge 2 commits into from

Conversation

PassiveLemon
Copy link
Contributor

@PassiveLemon PassiveLemon commented Jun 3, 2024

Description of changes

Adds WiVRn #278126 (comment)

Continuation of #299830 because I goofed.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@PassiveLemon
Copy link
Contributor Author

@ckiee
Copy link
Member

ckiee commented Jun 3, 2024

Welcome back.. It's a right of passage sometimes.

@Scrumplex
Copy link
Member

Both nixpkgs-review and ofborg fail to evaluate. While I can evaluate it when built by itself, FDO GitLab is currently under maintenance, so I can't pull the Monado source.

@Pandapip1
Copy link
Contributor

@GrahamcOfBorg eval

@PassiveLemon
Copy link
Contributor Author

From what Ofborg says, it looks like the monado CMake feature is involved but I don't really know why it fails. It builds on my system and it built in the other PR before

@Scrumplex
Copy link
Member

I think it just can't build the monado derivation currently because of the downtime of FDO GitLab

@Scrumplex
Copy link
Member

Scrumplex commented Jun 4, 2024

I have refactored the derivation a little, utilizing finalAttrs and moving the sources into the derivation itself, so overriding is much easier.

From 49cf6b5a420e347c1edcfc18f7e9787091fc1434 Mon Sep 17 00:00:00 2001
From: Sefa Eyeoglu <[email protected]>
Date: Tue, 4 Jun 2024 23:23:01 +0200
Subject: [PATCH] wivrn: refactor derivation

Signed-off-by: Sefa Eyeoglu <[email protected]>
---
 pkgs/by-name/wi/wivrn/package.nix | 59 ++++++++++++++-----------------
 1 file changed, 26 insertions(+), 33 deletions(-)

diff --git a/pkgs/by-name/wi/wivrn/package.nix b/pkgs/by-name/wi/wivrn/package.nix
index 67ec086ab758..81c798ee8b6b 100644
--- a/pkgs/by-name/wi/wivrn/package.nix
+++ b/pkgs/by-name/wi/wivrn/package.nix
@@ -3,7 +3,7 @@
 , stdenv
 , fetchFromGitHub
 , fetchFromGitLab
-, fetchpatch
+, applyPatches
 , avahi
 , boost
 , cmake
@@ -38,55 +38,36 @@
 , vulkan-tools
 , x264
 }:
-let
-  wivrnVersion = "0.15";
+stdenv.mkDerivation (finalAttrs: {
+  pname = "wivrn";
+  version = "0.15";
 
-  wivrnSrc = fetchFromGitHub {
+  src = fetchFromGitHub {
     owner = "meumeu";
     repo = "wivrn";
-    rev = "v${wivrnVersion}";
+    rev = "v${finalAttrs.version}";
     hash = "sha256-RVRbL9hqy9pMKjvzwaP+9HGEfdpAhmlnnvqZsEGxlCw=";
   };
 
-  monadoVersion = builtins.head (builtins.elemAt (builtins.split
-    "monado\n +GIT_TAG +([A-Za-z0-9]+)"
-    (builtins.readFile (wivrnSrc + "/CMakeLists.txt"))
-  ) 1);
-
-  monado = stdenv.mkDerivation {
-    pname = "monado";
-    version = monadoVersion;
-
+  monadoSrc = applyPatches {
     src = fetchFromGitLab {
       domain = "gitlab.freedesktop.org";
       owner = "monado";
       repo = "monado";
-      rev = monadoVersion;
+      rev = "ffb71af26f8349952f5f820c268ee4774613e200";
       hash = "sha256-+RTHS9ShicuzhiAVAXf38V6k4SVr+Bc2xUjpRWZoB0c=";
     };
 
     patches = [
-      (wivrnSrc + "/patches/monado/0001-c-multi-disable-dropping-of-old-frames.patch")
-      (wivrnSrc + "/patches/monado/0002-ipc-server-Always-listen-to-stdin.patch")
-      (wivrnSrc + "/patches/monado/0003-c-multi-Don-t-log-frame-time-diff.patch")
+      ("${finalAttrs.src}/patches/monado/0001-c-multi-disable-dropping-of-old-frames.patch")
+      ("${finalAttrs.src}/patches/monado/0002-ipc-server-Always-listen-to-stdin.patch")
+      ("${finalAttrs.src}/patches/monado/0003-c-multi-Don-t-log-frame-time-diff.patch")
     ];
 
     postPatch = ''
       substituteInPlace CMakeLists.txt --replace "add_subdirectory(doc)" ""
     '';
-
-    dontBuild = true;
-
-    installPhase = ''
-      cp -r . $out
-    '';
   };
-in
-stdenv.mkDerivation {
-  pname = "wivrn";
-  version = wivrnVersion;
-
-  src = wivrnSrc;
 
   nativeBuildInputs = [
     cmake
@@ -128,6 +109,18 @@ stdenv.mkDerivation {
     cudaPackages.cudatoolkit
   ];
 
+  postUnpack = ''
+    # Let's make sure our monado source revision matches what is used by WiVRn upstream
+    ourMonadoRev="${finalAttrs.monadoSrc.src.rev}"
+    theirMonadoRev=$(grep -A1 "https://gitlab.freedesktop.org/monado/monado" ${finalAttrs.src.name}/CMakeLists.txt | tail -n1 | sed 's/.*GIT_TAG\s*//')
+    if [ ! "$theirMonadoRev" == "$ourMonadoRev" ]; then
+      echo "Our Monado source revision doesn't match CMakeLists.txt." >&2
+      echo "  theirs: $theirMonadoRev" >&2
+      echo "    ours: $ourMonadoRev" >&2
+      return 1
+    fi
+  '';
+
   cmakeFlags = [
     (lib.cmakeBool "WIVRN_USE_VAAPI" true)
     (lib.cmakeBool "WIVRN_USE_X264" true)
@@ -138,7 +131,7 @@ stdenv.mkDerivation {
     (lib.cmakeBool "WIVRN_BUILD_CLIENT" false)
     (lib.cmakeBool "WIVRN_OPENXR_INSTALL_ABSOLUTE_RUNTIME_PATH" true)
     (lib.cmakeBool "FETCHCONTENT_FULLY_DISCONNECTED" true)
-    (lib.cmakeFeature "FETCHCONTENT_SOURCE_DIR_MONADO" "${monado}")
+    (lib.cmakeFeature "FETCHCONTENT_SOURCE_DIR_MONADO" "${finalAttrs.monadoSrc}")
   ];
 
   passthru.updateScript = nix-update-script { };
@@ -146,10 +139,10 @@ stdenv.mkDerivation {
   meta = with lib; {
     description = "An OpenXR streaming application to a standalone headset";
     homepage = "https://github.com/Meumeu/WiVRn/";
-    changelog = "https://github.com/Meumeu/WiVRn/releases/tag/v${wivrnVersion}";
+    changelog = "https://github.com/Meumeu/WiVRn/releases/tag/v${finalAttrs.version}";
     license = licenses.gpl3Only;
     maintainers = with maintainers; [ passivelemon ];
     platforms = platforms.linux;
     mainProgram = "wivrn-server";
   };
-}
+})
-- 
2.44.1

@Pandapip1
Copy link
Contributor

Actually, might it make sense to patch wivrn to use the latest version of monado instead?

@xytovl
Copy link

xytovl commented Jun 5, 2024

Actually, might it make sense to patch wivrn to use the latest version of monado instead?

I do not recommend it, updating monado often requires changes in WiVRn itself, see latest updates on dev branch:
Meumeu/WiVRn@27235d3
Meumeu/WiVRn@eaf6efd
Meumeu/WiVRn@41044fe (which didn't require any change).

You may backport those changes if you wish, but you may not always have a compatible version.

@PassiveLemon
Copy link
Contributor Author

I believe it currently is best to target the official releases rather than backporting changes. I like the idea case of checking if the versions match so if WiVRn is automatically updated but the Monado source isn't, a broken package isn't released.

@PassiveLemon
Copy link
Contributor Author

Also I think it would be of interest to add WiVRn configuration to the module as well. It's in json so there's probably some parts of other modules I can use to get that working. I'll probably look into it tomorrow.

@Pandapip1
Copy link
Contributor

Fair enough. Is it possible to have an update script update something other than the derivation's src?

@PassiveLemon
Copy link
Contributor Author

Config file doesn't appear to be that hard to implement, however it's loaded from the users config dir by default and I don't think there's a flag to specify a config file to load. Not sure how this could be done otherwise. Maybe a suggestion for upstream.

@PassiveLemon
Copy link
Contributor Author

Seems to have done it

@Pandapip1
Copy link
Contributor

Result of nixpkgs-review pr 316975 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
1 package failed to build:
  • wivrn

@PassiveLemon
Copy link
Contributor Author

My nixpkgs-review succeeds, evaluated on the same commit as this PR

@Pandapip1
Copy link
Contributor

Pandapip1 commented Aug 18, 2024

Weird. IIRC, mine failed to fetch sources. Maybe due to fixed-output derivation caching?

EDIT:

error: builder for '/nix/store/h3cjn6yhcjrsy0zys1sc8g57khgrfx7f-source.drv' failed with exit code 1;
       last 8 log lines:
       >
       > trying https://github.com/meumeu/wivrn/archive/v0.18.tar.gz
       >   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
       >                                  Dload  Upload   Total   Spent    Left  Speed
       >   0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
       >   0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
       > curl: (22) The requested URL returned error: 404
       > error: cannot download source from any mirror
       For full logs, run 'nix log /nix/store/h3cjn6yhcjrsy0zys1sc8g57khgrfx7f-source.drv'.

@Scrumplex
Copy link
Member

Result of nixpkgs-review pr 316975 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
1 package built:
  • wivrn

@Pandapip1
Copy link
Contributor

Result of nixpkgs-review pr 316975 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
1 package failed to build:
  • wivrn

@PassiveLemon
Copy link
Contributor Author

interesting

@LuNeder
Copy link

LuNeder commented Aug 18, 2024

I get a 404 here too. Entering the link, I see WiVRn has moved repositories.

From WiVRn’s readme: WiVRn has moved to https://github.com/WiVRn/WiVRn

So yeah, your nix is probably using a cached tar or something (so much for reproducibility…)

Here’s the cute pic on it’s readme bc it’s too cute not to add:
Cute WiVRn logo dinossaur (wearing a vr headset) driving a truck that has a lil smiley face carrying a house and some boxes in the trunk

@PassiveLemon
Copy link
Contributor Author

PassiveLemon commented Aug 18, 2024

Ooooh haha that makes sense, I will update the package

How can I make sure I don't have any caching affecting the results like in this case?

@LuNeder
Copy link

LuNeder commented Aug 18, 2024

Ye

(I’m skeptical about how safe it is to drive a truck while wearing a vr headset tho, I’m glad the moving went fine xD)

@LuNeder
Copy link

LuNeder commented Aug 18, 2024

How can I make sure I don't have any caching affecting the results like in this case?

I assume running the garbage collector could do the job? Unsure tho, never used nixpkgs-review

(I was about to say there might be an obscure nearly-undocumented flag/option for that too as often happens, but from that issue panda just opened I assume that’s not the case)

@SuperSandro2000
Copy link
Member

This can be easily solved by running eg. nix-build -K wivrn.src / nix-build --rebuild wivrn.src

@PassiveLemon
Copy link
Contributor Author

Any update on this? Xytovl said they will make a new release probably in a week or 2 so maybe we can wait until then

@PassiveLemon
Copy link
Contributor Author

PassiveLemon commented Sep 13, 2024

Updated the package postUnpack to fix an issue where "theirMonadoRev" becomes incorrect due to a change in the CMakeLists.txt file.

From:
grep -A1 "https://gitlab.freedesktop.org/monado/monado" ${finalAttrs.src.name}/CMakeLists.txt | tail -n1 | sed 's/.*GIT_TAG\s*//'
To:
grep "GIT_TAG" ${finalAttrs.src.name}/CMakeLists.txt | awk '{print $2}'

@PassiveLemon PassiveLemon changed the title wivrn: init at 0.18 wivrn: init at 0.19 Sep 19, 2024
@PassiveLemon
Copy link
Contributor Author

Would there happen to be an easier way to apply patches that doesn't involve me listing each one?

@Pandapip1
Copy link
Contributor

Would there happen to be an easier way to apply patches that doesn't involve me listing each one?

You could use runCommand and make a script that iterates through wivrn's patches directory and applies them to monado.

@Pandapip1
Copy link
Contributor

Also, just a heads up: every time you update the PR non-trivially, it requires a re-review. This is a pretty big PR, so reviews take a while. Once you're happy with it, it might make sense to just leave it alone, even letting the PR lag a couple versions behind, and only making changes requested by reviews. Once that's done, subsequent PRs to update wivrn will be able to be merged much quicker.

@PassiveLemon
Copy link
Contributor Author

I'm happy with it now, but maybe in the future, if enough patches need to be applied, I might consider refactoring the package. I'm only going to update versions until it gets merged, which hopefully won't take much longer than it already is

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.

10 participants