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 'socket' is called without checking return values. At , 'realloc' failures are silenced "// fail silently return null". At 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 , 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 . You're missing CC-BY-SA4.0 here, see . I didn't check everything. Greetings, Maxime.