Re: [PATCH] gnu: Add nethack.

[ludo@gnu.org (Ludovic Courtès)]

Hi!

Kei Kebreau <kei@openmailbox.org> skribis:

> From b728e078408f17136e8a4c3344b606e8f152b9e4 Mon Sep 17 00:00:00 2001
> From: Kei Kebreau <kei@openmailbox.org>
> Date: Tue, 31 May 2016 17:42:28 -0400
> Subject: [PATCH] gnu: Add nethack.
>
> * gnu/packages/games.scm (nethack): New variable.

You need to mention the new .patch file here (see ‘git log’ for
examples.)

You also need to add the .patch file to gnu/local.mk, and to mention the
change to gnu/local.mk in the commit log.

[...]

> +         (replace 'configure
> +           (lambda* (#:key inputs outputs #:allow-other-keys)
> +             (let ((out (assoc-ref outputs "out")))
> +               (substitute* "sys/unix/hints/linux"
> +                 (("^PREFIX=.*$")
> +                  (string-append "PREFIX=" out "\n"))
> +                 (("/bin/gzip") (which "gzip")))
> +               (substitute* "sys/unix/setup.sh"
> +                 (("/bin/sh") (which "bash"))))
> +             (system* "sh" "sys/unix/setup.sh" "sys/unix/hints/linux")))

Should be: (zero? (system* …)), which returns #t on success (a phase
must return a true value to be considered successful.)

> +         (add-after 'install 'move-state-files
> +           (lambda* (#:key inputs outputs #:allow-other-keys)
> +             (let* ((out (assoc-ref outputs "out")))
> +               (mkdir (string-append out "/games/lib/nethack-state-files"))
> +               (chdir (string-append out "/games/lib/nethackdir"))
> +               (for-each (lambda (file)
> +                           (system* "mv" file
> +                                    (string-append
> +                                     out "/games/lib/nethack-state-files")))

Instead of using the ‘mv’ command (or any other Coreutils command, for
that matter), use the Scheme equivalent.  Here it would be:

  (install-file file directory)

Besides, “/games” is unusual in the file system hierarchy.  Usually,
state files go to the localstatedir, i.e., the var/PACKAGE subdirectory.

Thus, what about putting state files in OUT/var/nethack?

But again, OUT is immutable, so these files cannot be modified, so
they’re not really “state.”

> +                         '("logfile" "perm" "record" "save" "xlogfile")))))

For clarity, have the phase return #t.

> +         (add-after 'move-state-files 'wrap-program
> +           (lambda* (#:key inputs outputs #:allow-other-keys)
> +             (let* ((out (assoc-ref outputs "out"))
> +                    (bin (string-append out "/bin"))
> +                    (nethack-user-dir "~/.nethack"))
> +               (mkdir bin)
> +               (with-directory-excursion bin
> +                 (call-with-output-file "nethack"
> +                   (lambda (port)
> +                     (format port "#!~a/bin/sh -e
> +# Create NetHack directory in user's $HOME if it isn't there
> +if [ ! -d ~a ]; then
> +  mkdir -p ~a
> +  cp -r ~a/* ~a
> +  chmod -R +w ~a
> +fi
> +
> +RUNDIR=$(mktemp -d)
> +
> +cleanup() {
> +  rm -rf $RUNDIR
> +}
> +trap cleanup EXIT
> +
> +cd $RUNDIR
> +for i in ~a/*; do
> +  ln -s $i $(basename $i)
> +done
> +for i in ~a/*; do
> +  ln -s $i $(basename $i)
> +done
> +./nethack~%"

Do we really need this wrapper?  Can’t we instead take it as a patch
from Debian or something?  I’m not a fan of inline Bash code, and not
very confident of scripts that do ‘rm -rf’.  :-)


> +--- nethack-3.6.0.orig/include/config.h	2016-05-27 17:20:03.062318307 -0400
> ++++ nethack-3.6.0/include/config.h	2016-05-31 16:48:04.283642766 -0400

Patches must always start with a line or two indicating what they do and
what their upstream status or origin is.

> +@@ -308,7 +308,6 @@
> + #define INSURANCE /* allow crashed game recovery */
> + 
> + #ifndef MAC
> +-#define CHDIR /* delete if no chdir() available */
> + #endif

Why?

> +-# CC = gcc
> ++CC = gcc
> + #
> + #	For Bull DPX/2 systems at B.O.S. 2.0 or higher use the following:
> + #
> +@@ -104,11 +104,11 @@
> + 
> + # yacc/lex programs to use to generate *_comp.h, *_lex.c, and *_yacc.c.
> + # if, instead of yacc/lex you have bison/flex, comment/uncomment the following.
> +-YACC     = yacc
> +-LEX      = lex
> +-# YACC     = bison -y
> ++# YACC     = yacc
> ++# LEX      = lex
> ++YACC     = bison -y
> + # YACC     = byacc
> +-# LEX      = flex
> ++LEX      = flex

Would it work to, instead, do:

  #:make-flags '("CC=gcc" "LEX=flex" …)

If it does, I think it’s preferable.

Could you send an updated patch?

Thanks for working on this tricky package!  ;-)

Ludo’.