glava: init at v1.4.5 #42103

eadwu · 2018-06-16T15:43:09Z

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/)
Fits CONTRIBUTING.md.

infinisil

While it would be nice to have this in nixpkgs, it throws an error when I actually run it:

Using backend: 'glx'
Failed to load GLX functions (libGL and libGLX do not exist!)

infinisil · 2018-07-03T13:59:17Z

pkgs/applications/misc/glava/default.nix

+    fixupPhase = ''
+      mkdir -p $out/bin
+
+      ln -sT $out/usr/bin/glava $out/bin/glava


Binaries shouldn't be in $out/usr/bin in any case, so you should move it to $out/bin/glava instead and remove the /usr folder

infinisil · 2018-07-03T14:08:53Z

pkgs/applications/misc/glava/default.nix

+    ];
+
+    patchPhase = ''
+      cp -rt glad --no-preserve=all ${glad}/*


It's more conventional to have the target at the end: cp -r --no-preserve=all ${glad}/* glad

infinisil · 2018-07-03T14:11:06Z

pkgs/applications/misc/glava/default.nix

+    buildInputs = [
+      libX11
+      libXext
+      python3


python should be in nativeBuildInputs (it's only used during the build afaik)

eadwu · 2018-07-04T00:55:40Z

The way I get libGL.so into the mix is through hardware.opengl.extraPackages. Don't really know how to apply that in the build process.

  hardware = {
    opengl = {
      enable = true;
      extraPackages = with pkgs; [
        libGL
      ];
    };
  };

eadwu · 2018-07-04T01:12:05Z

Seems to work if I just copy the libs from libGL through cp -r ${libGL}/lib $out/lib. Don't know whether or not I should just copy the required libs from the package.

infinisil

Ah, I figured it out, it apparently uses dlopen to find the library files: https://github.com/wacossusca34/glava/blob/ebade264ea1855afe88421ccc3ec22fc835696dc/glx_wcb.c#L191-L192

To make this work, you need to add patchelf --set-rpath "${makeLibraryPath [ libGL ]}" in postFixup. Pretty sure at least. Or you could patch the source directly.

(Having applied the bad fix for now), the next error I get is when running glava --copy-config, because it expects the default config to be in /etc/xdg/glava, but it's really in /nix/store/<hash>-glava/etc/xdg/glava. You can use substituteInPlace to patch this: https://github.com/wacossusca34/glava/blob/ebade264ea1855afe88421ccc3ec22fc835696dc/glava.c#L57

infinisil · 2018-07-04T01:06:22Z

pkgs/applications/misc/glava/default.nix

+
+    fixupPhase = ''
+      mv $out/usr/bin $out/bin
+      rm -rf $out/usr


Nitpick: rmdir $out/usr, since this dir is empty here

infinisil · 2018-07-04T01:26:58Z

pkgs/applications/misc/glava/default.nix

+    ] ++ optional enableGlfw glfw;
+
+    nativeBuildInputs = [
+      pkgconfig


Builds fine without pkgconfig.

infinisil · 2018-07-04T01:27:49Z

pkgs/applications/misc/glava/default.nix

+    fixupPhase = ''
+      cp -r ${libGL}/lib $out/lib
+      mv $out/usr/bin $out/bin
+      rm -rf $out/usr


Nitpick: Since this directory will be empty, you can use rmdir $out/usr, also makes sure that we don't accidentally remove something in case it ever installs anything else in /usr.

infinisil · 2018-07-04T01:29:18Z

pkgs/applications/misc/glava/default.nix

@@ -0,0 +1,85 @@
+{ stdenv, fetchurl, fetchFromGitHub, pkgconfig
+, libGL, libX11, libXext, python3, libXrandr, libXrender, libpulseaudio, libXcomposite
+, enableGlfw ? false, glfw }:


I don't know much about OpenGL, but why is this disabled by default?

It doesn't seem to be a required dependency so I just left it out by default.
GLFW 3.1+ (optional, disable with DISABLE_GLFW=1)

eadwu · 2018-07-04T02:33:35Z

glava --copy-config now works though the resulting directory has symlinks for its directories to /nix/store/*glava-VERSION/etc/xdg/glava

glava/bars
glava/circle
glava/graph
glava/radial
glava/util
glava/wave

infinisil

Awesome, it runs well :D

Yeah the --copy-config is a problem. I think the easiest solution would be to wrap the binary with a small shell script that catches the --copy-config flag and does its own copying of the config (with no symlinking), without passing the flag onto the actual glava executable. Feel free to create such a wrapper, but I'm fine without one.

Edit: Let me know whether you wanna try to make the wrapper or not, I'll wait with merging.

eadwu · 2018-07-07T17:09:49Z

Actually that just looks like the default behavior so I'll just leave it like that.
https://github.com/wacossusca34/glava/blob/dd5586a76e5cde938558ed8a6364b24111f4b1a8/glava.c#L68

infinisil · 2018-07-07T17:35:08Z

The problem is that the symlinks will be to /nix/store/hash-glava/... which will be garbage collected eventually, breaking the config.

eadwu · 2018-07-07T18:44:43Z

Currently I have the following, though I can't seem to expand $out

  writeScript "glava" ''
    #!${stdenv.shell}
    case "$1" in
      --copy-config)
        cp -r $out/etc/xdg/glava ~/.config/glava
        ;;
      *)
        $out/usr/bin/glava $@
    esac
  ''

eadwu · 2018-07-07T19:14:50Z

Ended up just replacing $out in fixupPhase

infinisil · 2018-07-07T20:05:51Z

There are a couple things to correct again, I hope you don't mind the constant back-and-forth! Here's a patch:

diff --git a/pkgs/applications/misc/glava/default.nix b/pkgs/applications/misc/glava/default.nix
index 64bc20fc11a..d115d1ae1d2 100644
--- a/pkgs/applications/misc/glava/default.nix
+++ b/pkgs/applications/misc/glava/default.nix
@@ -30,10 +30,14 @@ let
     #!${stdenv.shell}
     case "$1" in
       --copy-config)
-        cp -r --no-preserve=all $out/etc/xdg/glava ~/.config/glava
+        # The binary would symlink it, which won't work in Nix because the
+        # garbage collector will eventually remove the original files after
+        # updates
+        echo "Nix wrapper: Copying glava config to ~/.config/glava"
+        cp -r --no-preserve=all @out@/etc/xdg/glava ~/.config/glava
         ;;
       *)
-        $out/usr/bin/glava $@
+        exec @out@/bin/.glava-unwrapped "$@"
     esac
   '';
 in
@@ -77,11 +81,14 @@ in
 
     fixupPhase = ''
       mkdir -p $out/bin
-      patchelf \
-        --set-rpath "$(patchelf --print-rpath $out/usr/bin/glava):${makeLibraryPath [ libGL ]}" \
-        $out/usr/bin/glava
-      cp ${wrapperScript} $out/bin/glava
-      substituteInPlace $out/bin/glava --replace "\$out" "$out"
+      mv $out/usr/bin/glava $out/bin/.glava-unwrapped
+      rm -rf $out/usr
+
+      patchelf $out/bin/.glava-unwrapped \
+        --set-rpath "$(patchelf --print-rpath $out/bin/.glava-unwrapped):${makeLibraryPath [ libGL ]}" \
+
+      substitute ${wrapperScript} $out/bin/glava --subst-var out
+      chmod +x $out/bin/glava
     '';
 
     meta = with stdenv.lib; {

Things to note:

Using @out@ in the wrapper, substituting with --subst-var out which automatically replaces it with the value of $out
Added comments and echo to the wrapper
Using exec and "$@" instead of $@ to correctly handle arguments with spaces
Removing $out/usr again, had to use rm -rf after all heh

eadwu · 2018-07-07T20:19:17Z

It's fine, applied patch

infinisil

Thanks for the PR!

GrahamcOfBorg added 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Jun 16, 2018

eadwu force-pushed the init/glava branch 3 times, most recently from ff0307a to 5540be9 Compare June 20, 2018 00:28

infinisil requested changes Jul 3, 2018

View reviewed changes

eadwu force-pushed the init/glava branch from 5540be9 to f273d0c Compare July 4, 2018 00:52

eadwu force-pushed the init/glava branch from f273d0c to 9b57132 Compare July 4, 2018 01:10

infinisil requested changes Jul 4, 2018

View reviewed changes

eadwu force-pushed the init/glava branch from 9b57132 to 91db01e Compare July 4, 2018 02:29

infinisil approved these changes Jul 4, 2018

View reviewed changes

eadwu force-pushed the init/glava branch from 91db01e to 76ec809 Compare July 7, 2018 19:14

glava: init at v1.4.5

2ec495e

eadwu force-pushed the init/glava branch from 76ec809 to 2ec495e Compare July 7, 2018 20:19

infinisil approved these changes Jul 7, 2018

View reviewed changes

infinisil merged commit 129f94f into NixOS:master Jul 7, 2018

eadwu deleted the init/glava branch November 17, 2020 23:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

glava: init at v1.4.5 #42103

glava: init at v1.4.5 #42103

eadwu commented Jun 16, 2018

infinisil left a comment

infinisil Jul 3, 2018

infinisil Jul 3, 2018

infinisil Jul 3, 2018

eadwu commented Jul 4, 2018

eadwu commented Jul 4, 2018

infinisil left a comment •

edited

infinisil Jul 4, 2018

infinisil Jul 4, 2018

infinisil Jul 4, 2018

infinisil Jul 4, 2018

eadwu Jul 4, 2018

eadwu commented Jul 4, 2018

infinisil left a comment •

edited

eadwu commented Jul 7, 2018

infinisil commented Jul 7, 2018 •

edited

eadwu commented Jul 7, 2018

eadwu commented Jul 7, 2018

infinisil commented Jul 7, 2018

eadwu commented Jul 7, 2018

infinisil left a comment

glava: init at v1.4.5 #42103

glava: init at v1.4.5 #42103

Conversation

eadwu commented Jun 16, 2018

Motivation for this change

Things done

infinisil 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

eadwu commented Jul 4, 2018

eadwu commented Jul 4, 2018

infinisil left a comment • 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

Choose a reason for hiding this comment

eadwu commented Jul 4, 2018

infinisil left a comment • edited

Choose a reason for hiding this comment

eadwu commented Jul 7, 2018

infinisil commented Jul 7, 2018 • edited

eadwu commented Jul 7, 2018

eadwu commented Jul 7, 2018

infinisil commented Jul 7, 2018

eadwu commented Jul 7, 2018

infinisil left a comment

Choose a reason for hiding this comment

infinisil left a comment •

edited

infinisil left a comment •

edited

infinisil commented Jul 7, 2018 •

edited