[bug#39263,2/2] gnu: godot: Unbundle some dependencies.

* gnu/packages/game-development.scm (godot)[inputs]: Add bullet, pcre2, zstd.
[arguments](configure-flags): Use system libraries for the above.
[source](snippet): Remove bundled copies.
---
 gnu/packages/game-development.scm | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Timotej Lazar <timotej.lazar@araneo.si> writes:

> * gnu/packages/game-development.scm (godot)[inputs]: Add bullet, pcre2, zstd.
> [arguments](configure-flags): Use system libraries for the above.
> [source](snippet): Remove bundled copies.
> ---

Generally, this is good :)

>  gnu/packages/game-development.scm | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/gnu/packages/game-development.scm b/gnu/packages/game-development.scm
> index 79e3f6dc6c..9ded71375f 100644
> --- a/gnu/packages/game-development.scm
> +++ b/gnu/packages/game-development.scm
> @@ -78,6 +78,7 @@
>    #:use-module (gnu packages multiprecision)
>    #:use-module (gnu packages music)
>    #:use-module (gnu packages ncurses)
> +  #:use-module (gnu packages pcre)
>    #:use-module (gnu packages pkg-config)
>    #:use-module (gnu packages pulseaudio)
>    #:use-module (gnu packages python)
> @@ -1521,7 +1522,8 @@ games.")
>                    ;; of these may be modified; see "thirdparty/README.md".
>                    (with-directory-excursion "thirdparty"
>                      (for-each delete-file-recursively
> -                              '("freetype"
> +                              '("bullet"
> +                                "freetype"
>                                  "libogg"
>                                  "libpng"
>                                  "libtheora"
> @@ -1529,7 +1531,9 @@ games.")
>                                  "libvpx"
>                                  "libwebp"
>                                  "opus"
> -                                "zlib"))
> +                                "pcre2"
> +                                "zlib"
> +                                "zstd"))
>                      #t)))))

One thought I had here is that it would be more rigorous to have a list
of directories that are kept, and anything not on the list is
deleted. That way it's harder for new thirdparty dependencies to sneak
in. Not something that necessarily needs changing now though.

>      (build-system scons-build-system)
>      (arguments
> @@ -1541,6 +1545,7 @@ games.")
>                                 '())
>                             ;; Avoid using many of the bundled libs.
>                             ;; Note: These options can be found in the SConstruct file.
> +                           "builtin_bullet=no"
>                             "builtin_freetype=no"
>                             "builtin_glew=no"
>                             "builtin_libmpdec=no"
> @@ -1551,7 +1556,9 @@ games.")
>                             "builtin_libvpx=no"
>                             "builtin_libwebp=no"
>                             "builtin_opus=no"
> -                           "builtin_zlib=no")
> +                           "builtin_pcre2=no"
> +                           "builtin_zlib=no"
> +                           "builtin_zstd=no")
>         #:tests? #f ; There are no tests
>         #:phases
>         (modify-phases %standard-phases
> @@ -1598,6 +1605,7 @@ games.")
>                 #t))))))
>      (native-inputs `(("pkg-config" ,pkg-config)))
>      (inputs `(("alsa-lib" ,alsa-lib)
> +              ("bullet" ,bullet)
>                ("freetype" ,freetype)
>                ("glew" ,glew)
>                ("glu" ,glu)
> @@ -1612,7 +1620,9 @@ games.")
>                ("libxrandr" ,libxrandr)
>                ("mesa" ,mesa)
>                ("opusfile" ,opusfile)
> -              ("pulseaudio" ,pulseaudio)))
> +              ("pcre2" ,pcre2)
> +              ("pulseaudio" ,pulseaudio)
> +              ("zstd" ,zstd "lib")))
>      (home-page "https://godotengine.org/")
>      (synopsis "Advanced 2D and 3D game engine")
>      (description

Thanks for the feedback! I am sending updated patches after this reply.

Christopher Baines <mail@cbaines.net> [2020-01-25 09:16:08+0000]:
> I did have a look if the package builds with the mbedtls-apache
> package, rather than using the included source code, and it looks to.
> Although I'm aware that [1] says there are modifications.

The two Godot patches for mbedtls don’t seem to be relevant to Guix, so
I replaced the bundled copy with the mbedtls-apache package. I don’t
have a use case to test this, but the minimal example from the
HTTPRequest tutorial seems to work OK with an HTTPS URI.

Christopher Baines <mail@cbaines.net> [2020-01-25 09:18:33+0000]:
> One thought I had here is that it would be more rigorous to have a list
> of directories that are kept, and anything not on the list is deleted.
> That way it's harder for new thirdparty dependencies to sneak in.

Makes sense. As you suggest, I flipped the logic for removing thirdparty
files: whitelist preserved files and remove everything else. The snippet
can only preserve direct children of the thirdparty/ directory, which
keeps it simple but perhaps not flexible enough in the long run.

Do we generally prefer whitelisting bundled files? Most packages I have
seen (and written) do the opposite and list the files to remove. Maybe
we could add a guideline somewhere? Or point me to the one I missed. :)

Timotej Lazar <timotej.lazar@araneo.si> writes:

> Thanks for the feedback! I am sending updated patches after this reply.
>
> Christopher Baines <mail@cbaines.net> [2020-01-25 09:16:08+0000]:
>> I did have a look if the package builds with the mbedtls-apache
>> package, rather than using the included source code, and it looks to.
>> Although I'm aware that [1] says there are modifications.
>
> The two Godot patches for mbedtls don’t seem to be relevant to Guix, so
> I replaced the bundled copy with the mbedtls-apache package. I don’t
> have a use case to test this, but the minimal example from the
> HTTPRequest tutorial seems to work OK with an HTTPS URI.

Wonderful :)

> Christopher Baines <mail@cbaines.net> [2020-01-25 09:18:33+0000]:
>> One thought I had here is that it would be more rigorous to have a list
>> of directories that are kept, and anything not on the list is deleted.
>> That way it's harder for new thirdparty dependencies to sneak in.
>
> Makes sense. As you suggest, I flipped the logic for removing thirdparty
> files: whitelist preserved files and remove everything else. The snippet
> can only preserve direct children of the thirdparty/ directory, which
> keeps it simple but perhaps not flexible enough in the long run.

Great, this looks really useful.

> Do we generally prefer whitelisting bundled files? Most packages I have
> seen (and written) do the opposite and list the files to remove. Maybe
> we could add a guideline somewhere? Or point me to the one I missed. :)

I don't know if it's written down somewhere, all I can say is it
occurred to me when looking at the package definition.

I've pushed the 3 latest patches you sent to master, so they're included
in 18f8e935e85a99d5c284c0a6b719351a402ada21.

Thanks,

Chris