[bug#53235] [PATCH] gnu: Add brogue-ce.

[Maxime Devos]

Hi,

Aurélien Coussat schreef op do 13-01-2022 om 20:02 [+0100]:+
> +(define-public brogue-ce
> +  (package
> +    (name "brogue-ce")
> +    (version "1.10.1")
> +    (source (origin
> +              (method git-fetch)
> +              (uri (git-reference
> +                    (url "https://github.com/tmewett/BrogueCE.git";)

Run "./pre-inst-env guix lint brogue-c", if I'm not mistaken
it will tell you to not include the ".git".

> +                    (commit (string-append "v" version))))
> +              (file-name (git-file-name name version))
> +              (sha256
> +               (base32
> +               
> "0hgqr6gf0sxi9fv6ahd4rh3dgysbxm2i9yx998mdmw6my7h2756p"))))

Looking at upstream's source code, there's some error handling
missing in various places.  For example, at

<https://github.com/tmewett/BrogueCE/blob/819fb0145f8bb50c242a30d2cbf5f88131524e3e/src/platform/web-platform.c#L92>

'socket' is called without checking return values.

At
<https://github.com/tmewett/BrogueCE/blob/819fb0145f8bb50c242a30d2cbf5f88131524e3e/src/platform/platformdependent.c#L492>,
'realloc' failures are silenced "// fail silently <newline> return
null".

At
<https://github.com/tmewett/BrogueCE/blob/819fb0145f8bb50c242a30d2cbf5f88131524e3e/src/platform/platformdependent.c#L470>
the return value of 'malloc' isn't checked.

All these things (and the sprintf) aren't exactly reassuring me.

> +    (build-system gnu-build-system)
> +    (arguments
> +     `(#:tests? #f

Why?  When disabling tests, always document why (such that it is
easy to determine if they should be re-enabled after an update, e.g.)
with a comment.

> +       #:phases (modify-phases %standard-phases
> +                  (delete 'configure)
> +                  (add-before 'build 'prepare-build
> +                    ;; Set correct environment for SDL.
> +                    (lambda* (#:key inputs #:allow-other-keys)
> +                      (setenv "CPATH"
> +                              (string-append (assoc-ref inputs
> "sdl")
> +                                             "/include/SDL2:"
> +                                             (or (getenv "CPATH")
> "")))))

Setting CPATH doesn't work when cross-compiling.
Looking at
<https://github.com/tmewett/BrogueCE/blob/master/Makefile#L26>,
maybe substitute* the $(shell $(SDL_CONFIG) ...) instead?
(with -L[...]/include/SDL2).

> +                  (add-before 'build 'setenv-cc
> +                    (lambda _ (setenv "CC" "gcc")))

Won't work when cross-compiling, use cc-for-target.
If you saw this broken pattern elsewhere, please tell such
that it can be corrected.

> +                  (add-before 'build 'fix-datadir
> +                    ;; Set path to reach the correct asset
> directory.
> +                    (lambda* (#:key outputs #:allow-other-keys)
> +                      (substitute* "src/platform/tiles.c"
> +                        (("(\"%s/assets/[^\"]+\"), dataDirectory"
> all filepath)
> +                         (string-append filepath ", \""
> +                           (assoc-ref outputs "out") "/bin\"")))))

The upstream code does something very broken here, it uses sprintf
without checking the buffer size.  This happens to work here because
the file name in the store is relatively small compared to

#define BROGUE_FILENAME_MAX     (min(1024*4, FILENAME_MAX))

but that's still a very bad habit (search for "sprintf" "buffer
overflow" "CVE").

Can this be addressed (upstream)?

Downstream, we could replace "char filename[...]" with
"const char *filename;",
"sprintf(filename, "%s/assets/tiles.png", dataDirectory);"
with 'filename = "@data directory@"' in a patch (likewise for other
uses of 'sprintf' and 'filename'), and substitute* @data directory@ by
(string-append #$output "/bin")

(assoc-ref outputs ...) is slightly deprecated, nowadays you can do
#$output instead (make sure to put ,#~ before the (modify-phases)).

> +                  (replace 'install
> +                    ;; Upstream provides no install phase.
> +                    (lambda* (#:key outputs #:allow-other-keys)
> +                      (let* ((out (assoc-ref outputs "out"))
> +                             (bin (string-append out "/bin"))
> +                             (executable ,name)
> +                             (real-executable
> +                               (string-append "." executable "-
> real"))
> +                             (userdir (string-append "." ,name))
> +                             (share (string-append out "/share"))
> +                             (apps (string-append share
> "/applications")))
> +                        (copy-recursively "bin" bin)
> +                        ;; Create a "fake" executable that calls the
> actual
> +                        ;; executable from a good location.
> +                        (with-directory-excursion bin
> +                          (rename-file "brogue" real-executable)
> +                          (call-with-output-file executable
> +                            (lambda (p)
> +                              (format p
> +                                      "#!~a~@
> +                              cd $HOME~@
> +                              mkdir -p ~a~@
> +                              cd ~a~@

What's going on here with $HOME and mkdir?  Also, you need
to absolutise 'mkdir' such that it works in pure environments
that don't have 'mkdir' (guix shell --pure brogue-ce -- brogue)

> +                              exec ~a/~a $*"
> +                                      (which "bash")

That's broken when cross-compiling, use
(search-input-file inputs "/bin/bash") and add 'bash-minimal' to
inputs.

> +                                      userdir
> +                                      userdir
> +                                      bin
> +                                      real-executable)))
> +                          (chmod executable #o555))
> +                        ;; Create desktop file.
> +                        (mkdir-p apps)
> +                        (make-desktop-entry-file
> +                         (string-append apps "/" ,name ".desktop")
> +                         #:name "Brogue"

Brogue-CE?

> +                         #:exec ,name
> +                         #:categories '("RolePlaying" "Game")
> +                         #:keywords
> +                         '("adventure" "singleplayer")
> +                         #:comment
> +                         '((#f "Brave the Dungeons of Doom!")))
> +                        #t))))))

Returning #true from phases isn't necessary any.

This capitalisation is rather unusual, I'd add a comment
  ;; upstream capitalises the Dungeons of Doom this way.
Adding a desktop file looks good to me.

> +    (inputs
> +     `(("sdl" ,(sdl-union (list sdl2 sdl2-image)))))
> +    (home-page "https://github.com/tmewett/BrogueCE";)
> +    (synopsis "Community-lead fork of the much-loved minimalist
> roguelike game")

See (guix)Synopses and Descriptions, especially

   Descriptions should take between five and ten lines.  Use full
sentences, and avoid using acronyms without first introducing them.
Please avoid marketing phrases such as “world-leading”,
“industrial-strength”, and “next-generation”, and avoid superlatives
like “the most advanced”—they are not helpful to users looking for a
package and may even sound suspicious.  Instead, try to be factual,
mentioning use cases and features.

I'd avoid the marketing phrases 'Community-led', 'much-loved' and
'minimalist' here.  It's hard to imagine any ‘in-progress’ free
software accepting contributions (whether code, direction, ideas) from
any users not being 'community-led'.  'Much-loved' cannot apply to
new software, even if it's very good.  'Minimalist' is rather vague,
do you mean closure size, minimalism as in the $Anything vs. SystemD
flamewars?  Minimalism as in‘this software barely does anything
useful'?

> +    (description "Brogue is a single-player strategy game set [...]

BrogueCE is a fork of Brogue according to the README, so shouldn't
this be Brogue-CE?  How does this compare to, say, nethack and angband?
This is a rather generic description (though still colourful!) that
would easily apply to nethack or angband as well by substituting a few
words.

> he halls of a
> +mysterious and randomly-generated dungeon.  The objective is simple
> enough --
> +retrieve the fabled Amulet of Yendor from the 26th level -- but the
> dungeon is
> +riddled with danger.  Horrifying creatures and devious, trap-ridden
> terrain
> +await.  Yet it is also riddled with weapons, potions, and artifacts
> of forgotten
> +power.  Survival demands strength and cunning in equal measure as
> you descend,
> +making the most of what the dungeon gives you.  You will make
> sacrifices, narrow
> +escapes, and maybe even some friends along the way -- but will you
> be one of the
> +lucky few to return alive?")
> +    (license license:agpl3)))

It appears to be agpl3+, according to 
<https://github.com/tmewett/BrogueCE/blob/master/src/brogue/Rogue.h#L13>.

You're missing CC-BY-SA4.0 here, see
<https://github.com/tmewett/BrogueCE/blob/master/bin/assets/LICENSE.txt>.

I didn't check everything.

Greetings,
Maxime.

signature.asc
Description: This is a digitally signed message part