unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
* [bug#53235] [PATCH] gnu: Add brogue-ce.
@ 2022-01-13 19:02 Aurélien Coussat
  2022-01-14 12:48 ` Maxime Devos
  0 siblings, 1 reply; 2+ messages in thread
From: Aurélien Coussat @ 2022-01-13 19:02 UTC (permalink / raw)
  To: 53235

[-- Attachment #1: Type: text/plain, Size: 314 bytes --]

Hello Guix,

Before anything else, sorry if I made obvious mistakes: this is my first package ever, and my very first Guix contribution.

This patch adds BrogueCE to games.scm. BrogueCE is the community edition of Brogue, a minimalist roguelike game released under the AGPL-3.0 license.

Thank you very much!

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-Add-brogue-ce.patch --]
[-- Type: text/x-diff; name="0001-gnu-Add-brogue-ce.patch", Size: 5776 bytes --]

From cc67e0e0d482ab9c88cbfd8d509329ff98ec5670 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Aur=C3=A9lien=20Coussat?= <acoussat@mail.fr>
Date: Thu, 13 Jan 2022 17:33:55 +0100
Subject: [PATCH] gnu: Add brogue-ce.

---
 gnu/packages/games.scm | 92 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/gnu/packages/games.scm b/gnu/packages/games.scm
index bfd566aac0..497d8e6002 100644
--- a/gnu/packages/games.scm
+++ b/gnu/packages/games.scm
@@ -68,6 +68,7 @@
 ;;; Copyright © 2021 Brendan Tildesley <mail@brendan.scot>
 ;;; Copyright © 2021 Christopher Baines <mail@cbaines.net>
 ;;; Copyright © 2021 Foo Chuan Wei <chuanwei.foo@hotmail.com>
+;;; Copyright © 2022 Aurélien Coussat <acoussat@mail.fr>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -12588,3 +12589,94 @@ (define-public fheroes2
 Magic II (aka HOMM2) game engine.  It requires assets and game resources to
 play; it will look for them at @file{~/.local/share/fheroes2} folder.")
     (license license:gpl2)))
+
+(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")
+                    (commit (string-append "v" version))))
+              (file-name (git-file-name name version))
+              (sha256
+               (base32
+                "0hgqr6gf0sxi9fv6ahd4rh3dgysbxm2i9yx998mdmw6my7h2756p"))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:tests? #f
+       #: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") "")))))
+                  (add-before 'build 'setenv-cc
+                    (lambda _ (setenv "CC" "gcc")))
+                  (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\"")))))
+                  (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~@
+                              exec ~a/~a $*"
+                                      (which "bash")
+                                      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"
+                         #:exec ,name
+                         #:categories '("RolePlaying" "Game")
+                         #:keywords
+                         '("adventure" "singleplayer")
+                         #:comment
+                         '((#f "Brave the Dungeons of Doom!")))
+                        #t))))))
+    (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")
+    (description "Brogue is a single-player strategy game set in the 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)))
-- 
2.34.0


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* [bug#53235] [PATCH] gnu: Add brogue-ce.
  2022-01-13 19:02 [bug#53235] [PATCH] gnu: Add brogue-ce Aurélien Coussat
@ 2022-01-14 12:48 ` Maxime Devos
  0 siblings, 0 replies; 2+ messages in thread
From: Maxime Devos @ 2022-01-14 12:48 UTC (permalink / raw)
  To: Aurélien Coussat, 53235

[-- Attachment #1: Type: text/plain, Size: 10936 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2022-01-14 12:50 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-13 19:02 [bug#53235] [PATCH] gnu: Add brogue-ce Aurélien Coussat
2022-01-14 12:48 ` Maxime Devos

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).