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

  * 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

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(-)
  

Hi msglm,

msglm <msglm@techchud.xyz> 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

It looks like this and #77271 were opened from the same patch 
series, and based on the TODOs and commented code in this, I’m 
wondering if this one was sent on accident.

Happy to provide feedback if you’d like, but since it seems like 
it might not be ready yet, I thought I’d ask first.

Thanks,
  -- Ian
  

"msglm" <msglm@techchud.xyz> writes:

> I did intentionally send in two patches, but I was not aware 
> that it
> would bundle them together on the mailing list. I'm still new to 
> using
> email-based patching and apologize for this.

No problem at all, even contributors mess up from time to time. 
Each bug needs to have either a single patch, or a series of 
releated patches.  You sent a series, but:

- The patches in the series weren’t related.
- They each created one bug containing half the series (part 1/2 
  in #72271, 2/2 in #72272).

For #72271, you’ll want to make a v2 patch (`git format-patch -v2 
...') and send that to that bug.

For this patch, there are several things that need addressed. 
I’ll reply to the patch with feedback, and you’ll end up sending a 
v2 patch series to this bug when it’s ready.

> Despite this, the version I packaged is much newer, as the 
> previous
> version was 8 years out of date. Furthermore, this version also
> has fixes net-play compatibility for multiplayer emulation.

Yep, it looks like some good fixes!  Thank you for contributing.

 -- Ian
  

Hi msglm,

msglm <msglm@techchud.xyz> 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