[bug#60153,v2,1/3] gnu: python-pygame: Update to 2.1.2.

* gnu/packages/game-development.scm (python-pygame): Update to 2.1.2.
[arguments]<#:phases>: Add  ‘fix-build-config’ and ‘fix-sdl2-headers’.
[inputs]: Remove “sdl”, “sdl-image”, “sdl-mixer”, “sdl-ttf”, and “sdl-gfx”.
Add “sdl2”, “sdl2-image”, “sdl2-mixer”, “sdl2-ttf”, and “sdl2-gfx”.

---
 gnu/packages/game-development.scm | 95 ++++++++++---------------------
 1 file changed, 31 insertions(+), 64 deletions(-)

On 12/18/22 23:51, Adam Faiz wrote:
> +         (add-after 'unpack 'fix-build-config
> +           (lambda _
> +             (substitute* "buildconfig/config_unix.py"
> +               (("origincdirs = \\[.*\\]")
> +                "origincdirs = os.environ['C_INCLUDE_PATH'].split(\":\")")
> +               (("ORIGLIBDIRS") "LIBRARY_PATH")
> +               (("incdirs = \\[\\]") "incdirs = origincdirs")
> +               (("libdirs = \\[\\]") "libdirs = origlibdirs"))))
> +         (add-after 'unpack 'fix-sdl2-headers
> +           (lambda _
> +             (substitute* "buildconfig/config_unix.py"
> +               (("SDL_ttf.h") "SDL2/SDL_ttf.h")
> +               (("SDL_image.h") "SDL2/SDL_image.h")
> +               (("SDL_mixer.h") "SDL2/SDL_mixer.h"))
> +             (substitute* "src_c/imageext.c"
> +               (("SDL_image.h") "SDL2/SDL_image.h"))
> +             (substitute* "src_c/font.h"
> +               (("SDL_ttf.h") "SDL2/SDL_ttf.h"))
> +             (substitute* "src_c/mixer.h"
> +               (("SDL_mixer.h") "SDL2/SDL_mixer.h"))
> +             (substitute* "src_c/_sdl2/mixer.c"
> +               (("SDL_mixer.h") "SDL2/SDL_mixer.h")))))))
I don't see why these should go in phases, when I think going in a 
snippet would be better. From "Snippets and Phases" in the Guix manual:

> The source derived
> from an origin should produce a source that can be used to build the
> package on any system that the upstream package supports (i.e., act as
> the corresponding source).  In particular, origin snippets must not
> embed store items in the sources; such patching should rather be done
> using build phases.
The 'fix-build-config is what I consider to be a serious bugfix, it 
doesn't matter that it was originally fixed as part of guix packaging.

The 'fix-sld2-headers phase might stay in a build phase, if most systems 
that pygame supports doesn't keep these headers where guix would locate 
them. But I would consider that a bug in other systems for not storing 
the headers in the file path upstream places them.

The 'fix-sdl2-headers doesn't embed store items, so there's no other 
justification other than the reason given above.

On 12/24/22 07:20, Adam Faiz wrote:
> +               (("incdirs = \\[\\]") "incdirs = origincdirs")
> +               (("libdirs = \\[\\]") "libdirs = origlibdirs")))) 
I didn't do this kind of thing, because I didn't want to hardcode these. 
Most systems that pygame supports want to stay with the defaults.

>                    (substitute* "buildconfig/config_unix.py"
>                      (("localbase.d") "d"
>            #~(modify-phases %standard-phases
>              ;; Pass the dependencies to the configure script
>              ;; through environment variables.
>              (add-before 'build 'set-library-paths
>                (lambda _
>                  (setenv "LOCALBASE" " "))))))
I added this so that Guix as well as other systems can choose to pass 
the dependencies at the precise locations, otherwise it uses the 
defaults and doesn't affect those that use the defaults.

I hope this clarifies my understanding of this issue.
I like that Guix can provide patched and sometimes enhanced package 
sources while Guix-specific things stay in the build phases.

On 12/24/22 07:43, Adam Faiz wrote:
>>                    (substitute* "buildconfig/config_unix.py"
>>                      (("localbase.d") "d"
>>            #~(modify-phases %standard-phases
>>              ;; Pass the dependencies to the configure script
>>              ;; through environment variables.
>>              (add-before 'build 'set-library-paths
>>                (lambda _
>>                  (setenv "LOCALBASE" " "))))))
> I added this so that Guix as well as other systems can choose to pass 
> the dependencies at the precise locations, otherwise it uses the 
> defaults and doesn't affect those that use the defaults.
This part is actually flawed, what was done as part of 'fix-build-config 
is better. My mistake.

Am Samstag, dem 24.12.2022 um 07:20 +0800 schrieb Adam Faiz:
> On 12/18/22 23:51, Adam Faiz wrote:
> > +         (add-after 'unpack 'fix-build-config
> > +           (lambda _
> > +             (substitute* "buildconfig/config_unix.py"
> > +               (("origincdirs = \\[.*\\]")
> > +                "origincdirs =
> > os.environ['C_INCLUDE_PATH'].split(\":\")")
> > +               (("ORIGLIBDIRS") "LIBRARY_PATH")
> > +               (("incdirs = \\[\\]") "incdirs = origincdirs")
> > +               (("libdirs = \\[\\]") "libdirs = origlibdirs"))))
> > +         (add-after 'unpack 'fix-sdl2-headers
> > +           (lambda _
> > +             (substitute* "buildconfig/config_unix.py"
> > +               (("SDL_ttf.h") "SDL2/SDL_ttf.h")
> > +               (("SDL_image.h") "SDL2/SDL_image.h")
> > +               (("SDL_mixer.h") "SDL2/SDL_mixer.h"))
> > +             (substitute* "src_c/imageext.c"
> > +               (("SDL_image.h") "SDL2/SDL_image.h"))
> > +             (substitute* "src_c/font.h"
> > +               (("SDL_ttf.h") "SDL2/SDL_ttf.h"))
> > +             (substitute* "src_c/mixer.h"
> > +               (("SDL_mixer.h") "SDL2/SDL_mixer.h"))
> > +             (substitute* "src_c/_sdl2/mixer.c"
> > +               (("SDL_mixer.h") "SDL2/SDL_mixer.h")))))))
> I don't see why these should go in phases, when I think going in a 
> snippet would be better. From "Snippets and Phases" in the Guix
> manual:
> 
> > The source derived from an origin should produce a source that can
> > be used to build the package on any system that the upstream
> > package supports (i.e., act as the corresponding source).  In
> > particular, origin snippets must not embed store items in the
> > sources; such patching should rather be done using build phases.
> The 'fix-build-config is what I consider to be a serious bugfix, it 
> doesn't matter that it was originally fixed as part of guix
> packaging.
That part of the manual is sadly in need of a serious overhaul (see [1]
for a more extended discussion).  In any case, patching the source so
that it finds certain headers or libraries is typically a #:phases
thing, not a snippet one.  The move to #:phases reflects that.  

Cheers

[1] https://issues.guix.gnu.org/57598

Am Samstag, dem 24.12.2022 um 07:43 +0800 schrieb Adam Faiz:
> On 12/24/22 07:20, Adam Faiz wrote:
> > +               (("incdirs = \\[\\]") "incdirs = origincdirs")
> > +               (("libdirs = \\[\\]") "libdirs = origlibdirs")))) 
> I didn't do this kind of thing, because I didn't want to hardcode
> these. 
> Most systems that pygame supports want to stay with the defaults.
Here, "using the defaults" equates to having both origincdirs and
origlibdirs being empty, i.e. expanded from non-existing or empty
environment variables, right?  In that case, the build config will
still add the default /usr stuff – it just won't start out empty.

I feel as though we could do without this patch by supplying
PYGAME_EXTRA_BASE as a configure flag.  Wouldn't that be better than
patching?

Cheers

On 12/24/22 13:51, Liliana Marie Prikler wrote:
> Am Samstag, dem 24.12.2022 um 07:43 +0800 schrieb Adam Faiz:
>> On 12/24/22 07:20, Adam Faiz wrote:
>>> +               (("incdirs = \\[\\]") "incdirs = origincdirs")
>>> +               (("libdirs = \\[\\]") "libdirs = origlibdirs"))))
>> I didn't do this kind of thing, because I didn't want to hardcode
>> these.
>> Most systems that pygame supports want to stay with the defaults.
> Here, "using the defaults" equates to having both origincdirs and
> origlibdirs being empty, i.e. expanded from non-existing or empty
> environment variables, right?  In that case, the build config will
> still add the default /usr stuff – it just won't start out empty.
Yes, I hadn't considered that when sending the patch.

> I feel as though we could do without this patch by supplying
> PYGAME_EXTRA_BASE as a configure flag.  Wouldn't that be better than
> patching?
Indeed, it's better than patching and should be done if it works for 
you. I feel it's suboptimal though because the include path doesn't 
deserve to be called "extra".
> Cheers