GNU bug report logs - #55893
[PATCH] gnu: python-xyz: Add python-pysdl2.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 55893 in the body.
You can then email your comments to 55893 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jean Pierre De Jesus DIAZ <me <at> jeandudey.tech>
To: "guix-patches <at> gnu.org" <guix-patches <at> gnu.org>
Subject: [PATCH] gnu: python-xyz: Add python-pysdl2.
Date: Fri, 10 Jun 2022 18:09:04 +0000

[Message part 1 (text/plain, inline)]

Adds the PySDL2 package published on PyPi.

The package definition sets a fixed path for SDL2 libraries instead of relying on `LD_LIBRARY_PATH`.

[0001-gnu-python-xyz-Add-python-pysdl2.patch (text/x-patch, attachment)]

Message #8 received at 55893 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Jean Pierre De Jesus DIAZ <me <at> jeandudey.tech>, 55893 <at> debbugs.gnu.org
Subject: Re: [bug#55893] [PATCH] gnu: python-xyz: Add python-pysdl2.
Date: Sat, 11 Jun 2022 00:13:59 +0200

[Message part 1 (text/plain, inline)]

Jean Pierre De Jesus DIAZ via Guix-patches via schreef op vr 10-06-2022
om 18:09 [+0000]:
> +    (native-inputs
> +      (list sdl2 sdl2-image sdl2-gfx sdl2-mixer sdl2-ttf))

These need to be in 'inputs', not native-inputs -- their shared
libraries will actually be executed when python-pysdl2 executed, which
can only work if they are compiled for the same architecture as python-
pysdl2 is compiled for (that's what 'inputs' means; for 'native-
inputs', it would be compiled for the architecture on which python-
pysdl2 is compiled, not the architecture it is compiled for). 

> +    (synopsis "Python ctypes wrapper around SDL2")

ctypes sounds like an implementation detail not relevant to users of
python-pysdl2, maybe: ‘Python bindings around SDL2’?

> +                ; Disable pysdl2-dll. Not needed.

Nitpick: the convention is two ;;, not a single ;.

> +                 (string-append "DLL(\"SDL2\", [\"SDL2\", \"SDL2
2.0\","
> +                                "\"SDL2-2.0.0\"], "
> +                                "\""

Thee strings above can be combined.

> +                                (dirname
> +                                  (search-input-file inputs
> +                                                     "/lib/libSDL2.so"))

Indentations seems a bit wonky -- if this is to not make the line too long,
maybe try putting a line break between the 'string-append' and the "DLL(..."?

> +                                "\""
> +                                ")")))

These strings too.

> +    (arguments
> +      `(#:tests? #f ; Requires /dev/dri, OpenGL module, etc.
> +        #:phases
> +        (modify-phases %standard-phases

Recommended style (considered more readable):

  (list #:tests? #f ; etcetera
        #:phases
        #~(modify-phases [etcetera]))

(Many other packages don't do it like that yet, it has only
be discovered recently -- I would point you at IRC logs but
I'm currently offline.)

Also, don't put the package definition simply at the end, that
leads to merge conflicts.  Instead, try keep packages
alphabetical ... which is difficult here, because it has
historically neglected alphebetical ordening, but maybe right
after python-py would be a good fit?

Otherwise, the package definition LGTM from a distance, though
I only looked at the definition, I didn't check the source code
(for simplifying the substitute*-ions or checking for malware)
or build it.

Greetings,
Maxime.

[signature.asc (application/pgp-signature, inline)]

Message #11 received at 55893 <at> debbugs.gnu.org (full text, mbox):

From: Jean Pierre De Jesus DIAZ <me <at> jeandudey.tech>
To: "55893 <at> debbugs.gnu.org" <55893 <at> debbugs.gnu.org>
Subject: [PATCH] gnu: python-xyz: Add python-pysdl2.
Date: Tue, 14 Jun 2022 23:13:23 +0000

[Message part 1 (text/plain, inline)]

Applied the suggestions to the code and relocated the definition of the package on the `python-py` packages section.

Thanks for the review.

> ------- Original Message -------
> On Saturday, June 11th, 2022 at 12:13 AM, Maxime Devos maximedevos <at> telenet.be wrote:
>
>
>
> > Jean Pierre De Jesus DIAZ via Guix-patches via schreef op vr 10-06-2022
> > om 18:09 [+0000]:
> >
> > > + (native-inputs
> > > + (list sdl2 sdl2-image sdl2-gfx sdl2-mixer sdl2-ttf))
> >
> > These need to be in 'inputs', not native-inputs -- their shared
> > libraries will actually be executed when python-pysdl2 executed, which
> > can only work if they are compiled for the same architecture as python-
> > pysdl2 is compiled for (that's what 'inputs' means; for 'native-
> > inputs', it would be compiled for the architecture on which python-
> > pysdl2 is compiled, not the architecture it is compiled for).
> >
> > > + (synopsis "Python ctypes wrapper around SDL2")
> >
> > ctypes sounds like an implementation detail not relevant to users of
> > python-pysdl2, maybe: ‘Python bindings around SDL2’?
> >
> > > + ; Disable pysdl2-dll. Not needed.
> >
> > Nitpick: the convention is two ;;, not a single ;.
> >
> > > + (string-append "DLL(\"SDL2\", [\"SDL2\", \"SDL2
> >
> > 2.0\","
> >
> > > + "\"SDL2-2.0.0\"], "
> > > + "\""
> >
> > Thee strings above can be combined.
> >
> > > + (dirname
> > > + (search-input-file inputs
> > > + "/lib/libSDL2.so"))
> >
> > Indentations seems a bit wonky -- if this is to not make the line too long,
> > maybe try putting a line break between the 'string-append' and the "DLL(..."?
> >
> > > + "\""
> > > + ")")))
> >
> > These strings too.
> >
> > > + (arguments
> > > + `(#:tests? #f ; Requires /dev/dri, OpenGL module, etc.
> > > + #:phases
> > > + (modify-phases %standard-phases
> >
> > Recommended style (considered more readable):
> >
> > (list #:tests? #f ; etcetera
> > #:phases
> > #~(modify-phases [etcetera]))
> >
> > (Many other packages don't do it like that yet, it has only
> > be discovered recently -- I would point you at IRC logs but
> > I'm currently offline.)
> >
> > Also, don't put the package definition simply at the end, that
> > leads to merge conflicts. Instead, try keep packages
> > alphabetical ... which is difficult here, because it has
> > historically neglected alphebetical ordening, but maybe right
> > after python-py would be a good fit?
> >
> > Otherwise, the package definition LGTM from a distance, though
> > I only looked at the definition, I didn't check the source code
> > (for simplifying the substitute*-ions or checking for malware)
> > or build it.
> >
> > Greetings,
> > Maxime.

[0001-gnu-python-xyz-Add-python-pysdl2.patch (text/x-patch, attachment)]

Message #16 received at 55893-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Jean Pierre De Jesus DIAZ <me <at> jeandudey.tech>
Cc: "55893 <at> debbugs.gnu.org" <55893-done <at> debbugs.gnu.org>
Subject: Re: bug#55893: [PATCH] gnu: python-xyz: Add python-pysdl2.
Date: Fri, 17 Jun 2022 15:35:51 +0200

Hi Jean-Pierre,

Jean Pierre De Jesus DIAZ <me <at> jeandudey.tech> skribis:

> From 542a138fa26eec2c25cb7743be612a2d7f4a502d Mon Sep 17 00:00:00 2001
> Message-Id: <542a138fa26eec2c25cb7743be612a2d7f4a502d.1655242487.git.me <at> jeandudey.tech>
> From: Jean-Pierre De Jesus DIAZ <me <at> jeandudey.tech>
> Date: Fri, 10 Jun 2022 19:57:31 +0200
> Subject: [PATCH] gnu: python-xyz: Add python-pysdl2.
>
> * gnu/packages/python-xyz.scm (python-pysdl2): Add package.

Applied, thank you, and thanks Maxime for reviewing!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.