[bug#62353] [PATCH] gnu: Add glava.

[Maxim Cournoyer]

Hi Spencer,

Spencer Skylar Chan <schan12@umd.edu> writes:

>  From 17eb638d8ff5d5cb40c9bdf9f24544ef7f23088e Mon Sep 17 00:00:00 2001
> From: Skylar Chan <schan12@umd.edu>
> Date: Mon, 30 Jan 2023 13:39:53 -0500
> Subject: [PATCH] gnu: Add glava.
>
> * gnu/packages/audio.scm (glava): New variable.

Thank you!

I'm having problem applying your patch; git says it's corrupted.  It
seems the longer lines were folded?  I'm using this opportunity to share
some review comments.

> ---
>   gnu/packages/audio.scm | 75 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 75 insertions(+)
>
> diff --git a/gnu/packages/audio.scm b/gnu/packages/audio.scm
> index 6f3fa2a580..a0e73ed55a 100644
> --- a/gnu/packages/audio.scm
> +++ b/gnu/packages/audio.scm
> @@ -5109,6 +5109,81 @@ (define-public cava
>   using ALSA, MPD, PulseAudio, or a FIFO buffer as its input.")
>       (license license:expat)))
>
> +(define-public glava
> +  (package
> +    (name "glava")
> +    (version "1.6.3")
> +    (source
> +     (origin
> +       (method git-fetch)
> +       (uri (git-reference
> +             (url "https://github.com/jarcode-foss/glava";)
> +             (commit (string-append "v" version))))
> +       (file-name (git-file-name name version))
> +       (sha256
> +        (base32
> +         "0kqkjxmpqkmgby05lsf6c6iwm45n33jk5qy6gi3zvjx4q4yzal1i"))))
> +    (build-system gnu-build-system)
> +    (arguments
> +     (list #:tests? #f ;no tests.

nitpick: You can drop the trailing period since inline comments are not
complete sentence (contrary to standalone comments, which should be).

> +           #:make-flags
> +           #~(list
> +              (string-append "DESTDIR=" #$output)
> +              (string-append "PREFIX=" #$output)
> +              (string-append "CC=" #$(cc-for-target)))
> +           #:phases
> +           #~(modify-phases %standard-phases
> +               (add-after 'unpack 'fix-etc-references
> +                 (lambda* (#:key outputs #:allow-other-keys)
> +                   (substitute* (find-files ".*")

The first argument to find-files is a directory, not a regexp; so it
should probably be ".".  It's usually best to use a regexps as a 2nd
argument otherwise for large projects it wastes IO scanning tons of
files and sometimes fails on binary images for example.

> +                     (("/etc/xdg") (string-append #$output 
> "/etc/xdg/glava")))))
> 
> +               (add-after 'unpack 'patch-makefile
> +                 (lambda _
> +                   (substitute* "Makefile"
> +                     (("$(DESTDIR)$(SHADERDIR)")
> +                      "$(SHADERDIR)"))))

This substitution must do anything, as the pattern
"$(DESTDIR)$(SHADERDIR)" is a regexp, and $ matches the end of line
character.  You probably meant to escape regexp special characters like:

"\\$\\(DESTDIR\\)\\$\\(SHADERDIR\\)"

in the pattern.

> +               (delete 'configure)
> +               (add-after 'install 'make-wrapper
> +                 ;; 
> https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/misc/glava/default.nix

Please leave a verbose comment (complete sentence) explaining what is
the issue, and prefer linking to the 'raw' github file in a (see: $URL)
at the end of the explanation.

> +                 (lambda* (#:key inputs outputs #:allow-other-keys)

'inputs' appears unused.

> +                   (let* ((wrapper (string-append #$output "/bin/glava")))
> +                     (with-output-to-file wrapper
> +                       (lambda _
> +                         (display
> +                          (string-append
> +                           "#!/bin/sh\n"
> +                           "case \"$1\" in\n"
> +                           "  --copy-config|-C)\n"
> +                           "# The binary would symlink it, which won't 
> work in Guix because the\n"
> +                           "# garbage collector will eventually remove 
> the original files after\n"
> +                           "# updates.\n"
> +                           "  echo \"Guix wrapper: Copying glava config 
> to ~/.config/glava\"\n"
> +                           "  cp -r --no-preserve=all " #$output 
> "/etc/xdg/glava ~/.config/glava\n"
> +                           "  ;;\n"
> +                           "*)\n"
> +                           "  exec " #$output "/bin/.glava-real \"$@\"\n"
> +                           "esac\n"))))
> +                     (chmod wrapper #o555))))

Hm.  I'm not sure we want to preserve this behavior of messing with
the user's HOME files, but I haven't read the issue to get a grasp of
what is the exact problem it tries to solve.

> +               (add-after 'install 'rename-binary
> +                 (lambda* (#:key outputs #:allow-other-keys)
> +                   (mkdir-p (string-append #$output "/bin"))
> +                   (rename-file (string-append #$output "/usr/bin/glava")
> +                                (string-append #$output 
> "/bin/.glava-real"))
> +                   (delete-file-recursively (string-append #$output 
> "/usr")))))))
> +    (inputs (list pulseaudio
> +                  libx11
> +                  libxext
> +                  libxcomposite
> +                  libxrender))
> +    (native-inputs (list python))
> +    (home-page "https://github.com/jarcode-foss/glava";)
> +    (synopsis "OpenGL audio spectrum visualizer")
> +    (description "GLava is an OpenGL audio spectrum visualizer using 
> PulseAudio or
> +MPD's fifo output.  Its primary use case is for desktop windows or 
> backgrounds.
> +It is compatible with most EMWH compliant window managers.")
> +    (license (list license:gpl3+
> +                   license:expat)))) ;; khrplatform.h

nitpick: Please use single ';' for inline comments :-)

Could you please send a revised version with the above taken care of?

-- 
Thanks,
Maxim