Subject: Re: bug#26267: [PATCH] gnu: hyperrogue: Update to 9.4c.

From: Marius Bakke <[email-masked]>
Date: Mon, 27 Mar 2017 23:29:10 +0200

Kei Kebreau <[email-masked]> writes:

> * gnu/packages/games.scm (hyperrogue): Update to 9.4c.
> [source]: Remove snippet.
> [arguments]: Remove #:make-flags; adjust configure and install phases; add
> install-data phase.
> [inputs]: Add hyperrogue-data.

Thanks!

[...]

> +         (add-after 'install 'install-data
> +           (lambda* (#:key inputs outputs #:allow-other-keys)
> +             (let* ((data (assoc-ref inputs "hyperrogue-data"))
> +                    (out (assoc-ref outputs "out"))
> +                    (share-dir (string-append out "/share/hyperrogue")))
> +               (system* "tar" "xvf" data "-C" ".")
> +               (with-directory-excursion "hyperrogue94c-win"

You can access the "version" variable here by "unquoting" it (with
comma). Another approach is skipping the "top folder" of the tarball
with "--strip-components=1". Maybe the "sounds" subdirectory can be
extracted directly to "out/share/sounds"? See e.g.
"font-adobe-source-code-pro" for some tar trickery.

> +                 (for-each
> +                  (lambda (file)
> +                    (install-file file (string-append share-dir "/sounds")))
> +                  (append (find-files "." "\\.ogg$")
> +                          (find-files "sounds" ".*$")))
> +                 (for-each
> +                  (lambda (file)
> +                    (install-file file share-dir))
> +                  '("papermodeldata.txt" "papermodeldata-help.txt"))))
>               #t)))))
>      (inputs
>       `(("font-dejavu" ,font-dejavu)
> @@ -3227,7 +3238,23 @@ throwing people around in pseudo-randomly generated buildings.")
>         ("sdl-union" ,(sdl-union (list sdl
>                                        sdl-gfx
>                                        sdl-mixer
> -                                      sdl-ttf)))))
> +                                      sdl-ttf)))
> +       ("hyperrogue-data"
> +        ,(origin
> +           (method url-fetch)
> +           (uri
> +            (string-append
> +             "http://www.roguetemple.com/z/hyper/" name
> +             (string-join (string-split version #\.) "")
> +             "-win.zip"))
> +           (sha256
> +            (base32
> +             "1cyyrsnrixygg3zyz97hpsm6jzwbhydiwk3kl0lm7qjnw2nzkhhh"))

It would be good to have a comment near the source stating "remember to
update hyperrogue-data as well". Otherwise, if the enterprising Guix
contributor already have the old tarball in their store, they may not
notice the impending version/hash mismatch.

Also, this can be a "native-input" since it's downloaded, extracted and
thrown away, and only carry platform-independent data (for us, anyway).

> +           (modules '((guix build utils)))
> +           (snippet
> +            '(begin
> +               ;; Remove .exe and .dll files.
> +               (for-each delete-file (find-files "." "\\.(exe|dll)$"))))))))

...for the same reason, I don't think this is necessary, especially if
we can avoid extracting these files to begin with. It will just use up
*more* space in the store, since it creates a derivation in addition to
the tarball. :-)

The rest LGTM, thanks!

>      (home-page "http://www.roguetemple.com/z/hyper/")
>      (synopsis "Non-euclidean graphical rogue-like game")
>      (description
> -- 
> 2.12.0