Subject: Re: [bug#77272] [PATCH 2/2] gnu: dolphin-emu: Update to 2503.

From: Ian Eure <[email-masked]>
Date: Sat, 29 Mar 2025 20:31:17 -0700

Hi msglm,

msglm <[email-masked]> writes:

> * gnu/packages/emulators.scm (dolphin-emu): Update to 2503, Fix 
> netplay
> * gnu/packages/game-development.scm (sfml-3): New Package
> * gnu/packages/game-development.scm (miniaudio): New Package
> * gnu/packages/networking.scm (enet): Update to 2.30.9

All these changes need to be separate commits, which are sent as a 
patch series.  See `(guix)Sending a Patch Series' in the Guix 
manual.

The commit messages also need to follow `(standards)Change Logs'.

> Change-Id: I5242f46e457db6552663c03c19dc0f227efb80cc
> ---
>  gnu/packages/emulators.scm        | 50 ++++++++++++++++--------
>  gnu/packages/game-development.scm | 65 
>  +++++++++++++++++++++++++++++++
>  gnu/packages/networking.scm       |  4 +-
>  gnu/packages/sdl.scm              |  4 +-
>  4 files changed, 103 insertions(+), 20 deletions(-)
>
> diff --git a/gnu/packages/emulators.scm 
> b/gnu/packages/emulators.scm
> index e71c2803a3..83ae89c172 100644
> --- a/gnu/packages/emulators.scm
> +++ b/gnu/packages/emulators.scm
> @@ -278,20 +278,19 @@ (define-public desmume
>  ;; Following commits and revision numbers of beta versions 
>  listed at
>  ;; https://dolphin-emu.org/download/.
>  (define-public dolphin-emu
> -  (let ((commit "f9deb68aee962564b1495ff04c54c015e58d086f")
> -        (revision "13669"))

The `package' form needs to be dedented now that the enclosing 
`let' is gone.

>        (source
>         (origin
>           (method git-fetch)
>           (uri (git-reference
>                 (url "https://github.com/dolphin-emu/dolphin")
> -               (commit commit)))
> +               (recursive? #t)

Is it reasonably possible to do a non-recursive clone?  It looks 
like that’s vendoring dependencies, which Guix strongly prefers to 
avoid.  I also notice that many of them are deleted in the loop in 
the next hunk.

> @@ -300,14 +299,19 @@ (define-public dolphin-emu
>               (for-each (lambda (dir)
>                           (delete-file-recursively
>                            (string-append "Externals/" dir)))
> -                       '("LZO" "OpenAL" "Qt" "SFML" "bzip2"
> +                       '(
> +                         ;"LZO" "OpenAL" "Qt" "SFML" "bzip2"
> +
> +                         "LZO" "OpenAL" "Qt" "bzip2" ;;TODO: 
> Ensure SFML is removed and update the package to make it happen
> +

Please remove commented out code and address TODOs.

> -                         "ffmpeg" "fmt" "gettext"
> +                         ;"ffmpeg" "fmt" "gettext"
> +                         "fmt" "gettext"

These too.

> -                         "hidapi" "libpng" "libusb" "mbedtls"
> -                         "miniupnpc" "minizip" "MoltenVK" 
> "pugixml"
> +                         ;"hidapi" "libpng" "libusb" "mbedtls"
> +                         "hidapi" "libusb" "mbedtls"
> +                         ;"miniupnpc" "minizip" "MoltenVK" 
> "pugixml"
> +                         "miniupnpc" "MoltenVK" "pugixml"
>                           "soundtouch"
> -                         "xxhash" "zlib" "zstd"))
> +                         ;"xxhash" "zlib" "zstd"
> +                         "xxhash" "zstd"

And these.

> +                         ))

Parens should be nested on the closing line, not left dangling.

> @@ -339,12 +347,21 @@ (define-public dolphin-emu
>                 ((".*add_subdirectory.*Externals/enet.*") "")
>                 ((".*add_subdirectory.*Externals/soundtouch.*") 
>                 "")
>                 ((".*add_subdirectory.*Externals/xxhash.*") 
>                 ""))))
> -         (patches (search-patches "dolphin-emu-data.patch"))))

Please include a patch deleting this file if it’s no longer 
needed.

> +         ))

Please nest parens.

>        (build-system cmake-build-system)
>        (arguments
>         (list
>          #:phases
>          #~(modify-phases %standard-phases
> +            (add-before 'configure 'set-scm-desc-str
> +                        (lambda _
> +                          ;;For netplay to work, the 
> SCM_REV_STR must match the
> +                          ;;commit hash of whatever version 
> we're on.
> +                          ;;THIS MUST BE UPDATED EVERY UPDATE 
> SO THAT NETPLAY
> +                          ;;CONTINUES TO FUNCTION!

There should be a space between the comment characters text, ex 
";; For netplay..."

> +                          (substitute* 
> "Source/Core/Common/scmrev.h.in" (("#define SCM_REV_STR 
> \"\\$\\{DOLPHIN_WC_REVISION\\}\"") "#define SCM_REV_STR 
> \"9763c0a1e2b9db0c3861d25bc2f5a0ace6a15ee3\""))

It looks like this is populated from the $DOLPHIN_WC_REVISION 
environment variable, can you set that instead of patching the .in 
template?  That would be much cleaner.

You might want to let-bind this at the top of the package 
definition, so it’s easier to see.

This line is too long and needs to be wrapped.

> +                          ))
> +

Please nest closing parens.

> diff --git a/gnu/packages/game-development.scm 
> b/gnu/packages/game-development.scm
> index 91369089b9..bb1ae5bc2f 100644
> --- a/gnu/packages/game-development.scm
> +++ b/gnu/packages/game-development.scm
> @@ -1115,6 +1115,71 @@ (define-public sfml
>  of five modules: system, window, graphics, audio and network.")
>      (license license:zlib)))
>  
> +(define-public sfml-3 ;;For dolphin-emu

Please remove the comment.

If possible, it’d be prefereable to upgrade the existing SFML 
package.  Is 3.0.0 backwards compatible with 2.5.x, or would that 
break that other things that use it?

> +               (package
> +                 (inherit sfml)
> +                 (name "sfml-3")
> +                 (version "3.0.0")
> +                 (inputs (modify-inputs (package-inputs sfml)
> +                                        (prepend libxcursor 
> libxi miniaudio)))
> +                 (source (origin
> +                           (method git-fetch)
> +                           (uri (git-reference
> +                                  (url 
> "https://github.com/SFML/SFML")
> +                                  (recursive? #t)
> +                                  (commit version)))
> +                           (file-name (git-file-name name 
> version))
> +                           (sha256
> +                             (base32
> + 
> "0y37cmpp490gcibajigxsbjc6icggqv40jrhzj2kwygpc0ppzb3v"))
> +                           (modules '((guix build utils)))
> +                           ;(snippet
> +                           ;  '(begin
> +                           ;     ;; Ensure system libraries are 
> used.
> +                           ;     (delete-file-recursively 
> "extlibs")
> +                           ;     #t))

Guix packages should basically never have commented code, please 
delete it.

Does this mean that this version of SFML is building with vendored 
libraries?  Guix strongly discourages vendored libraries, so, it 
shouldn’t.

> +;; For sfml-3

Please remove the comment

> +(define-public miniaudio
> +               (package

Indentation here is wrong, it should be two characters deeper than 
the "(define-public" text.

> +                 (name "miniaudio")
> +                 (version "0.11.22")
> +                 (source (origin
> +                           (method git-fetch)
> +                           (uri (git-reference
> +                                  (url 
> "https://github.com/mackron/miniaudio")
> +                                  (commit version)))
> +                           (file-name (git-file-name name 
> version))
> +                           (sha256
> +                             (base32
> + 
> "1pjaiq71x24n9983vkhjxrsbraa24053h727db22b1rb2xyfrzm3"))))
> +                 (build-system cmake-build-system)
> +                 (arguments
> +                   (list
> +                     #:tests? #f
> +                     #:phases
> +                     #~(modify-phases %standard-phases
> +                                      (replace 'install
> +                                               (lambda _
> +                                                 (let ((out 
> #$output))
> +                                                   ;; Ensure 
> the output directory exists
> +                                                   (mkdir-p 
> (string-append out "/lib"))
> +                                                   ;; Copy the 
> static libraries to the output directory
> +                                                   (for-each 
> (lambda (file)
> + 
> (copy-file file (string-append out "/lib/" file))) ; Ensure the 
> destination is a file
> + 
> '("libminiaudio.a"
> + 
> "libminiaudio_channel_combiner_node.a"
> + 
> "libminiaudio_channel_separator_node.a"
> + 
> "libminiaudio_ltrim_node.a"
> + 
> "libminiaudio_reverb_node.a"
> + 
> "libminiaudio_vocoder_node.a"))))))))

This feels odd.  Does the upstream not have a way to install the 
libraries?  Does it only build static libs, and not .so files?

> diff --git a/gnu/packages/networking.scm 
> b/gnu/packages/networking.scm
> index 65d44e975a..ff3e032a2b 100644
> --- a/gnu/packages/networking.scm
> +++ b/gnu/packages/networking.scm
> @@ -2627,14 +2627,14 @@ (define-public proxychains-ng
>  (define-public enet
>    (package
>      (name "enet")
> -    (version "1.3.17")
> +    (version "1.3.18")
>      (source
>       (origin
>         (method url-fetch)
>         (uri (string-append "http://enet.bespin.org/download/"
>                             "enet-" version ".tar.gz"))
>         (sha256
> -        (base32 
> "1p6f9mby86af6cs7pv6h48032ip9g32c05cb7d9mimam8lchz3x3"))))
> +        (base32 
> "0djxz2j8248bsvbrs42vr39fhxlrqr3lqbhzs7yb92ync19hr2ia"))))
>      (build-system gnu-build-system)
>      (native-inputs
>       (list pkg-config))
> diff --git a/gnu/packages/sdl.scm b/gnu/packages/sdl.scm
> index 80de707819..3a736107c8 100644
> --- a/gnu/packages/sdl.scm
> +++ b/gnu/packages/sdl.scm
> @@ -72,7 +72,7 @@ (define-module (gnu packages sdl)
>  (define-public sdl2
>    (package
>      (name "sdl2")
> -    (version "2.30.8")
> +    (version "2.30.9")
>      (source (origin
>                (method url-fetch)
>                (uri
> @@ -80,7 +80,7 @@ (define-public sdl2
>                                version ".tar.gz"))
>                (sha256
>                 (base32
> - 
> "0n006l1zds2av8a9p6m6l0mj7jwb3jbr6mq7j0nxg6vblxg2j31q"))))
> + 
> "197bdcfnnsd4k7q91y518kari0p3rcqbdfq40zsn79w73kvp9d94"))))
>      (build-system gnu-build-system)
>      (arguments
>       (list

Both these look great.

Please:

- Address comments, or give a compelling rationale for why the 
  suggestion won’t work / isn’t desirable.
- Run `guix lint' on your changes and make sure all issues that 
  raises are fixed.  `guix style' may help here.
- Send a v2 patch series.

Thanks,

  -- Ian