Bug 202437 - Review Request: perl-SDL - SDL bindings for the Perl language
Review Request: perl-SDL - SDL bindings for the Perl language
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT 202439
  Show dependency treegraph
 
Reported: 2006-08-14 10:04 EDT by Hans de Goede
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-08-26 01:35:12 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2006-08-14 10:04:31 EDT
Spec URL: http://people.atrpms.net/~hdegoede/perl-SDL.spec
SRPM URL: http://people.atrpms.net/~hdegoede/perl-SDL-1.20.3-8.src.rpm
Description:
SDL (Simple DirectMedia Layer) bindings for the perl language.

---

Notice that this package comes from the repo that must be named, where it used to live because it depends on smpeg which contains patented code, however it can be build without smpeg support too and currently the packages using it don't need the smpeg part.
Comment 1 Kevin Fenzi 2006-08-14 23:00:51 EDT
Per talking with tibbs on IRC I am going to take over the review, as I had just 
started in on one just before he did. 

OK - Package name
OK - Spec file matches base package name.
OK - Meets Packaging Guidelines.
OK - License (LGPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
ab7fb92a1ed0db54a88839e64b9ce2c6  SDL_perl-1.20.3.tar.gz
ab7fb92a1ed0db54a88839e64b9ce2c6  SDL_perl-1.20.3.tar.gz.1
OK - Package compiles and builds on at least one arch.
n/a - Package needs ExcludeArch
OK - BuildRequires correct
n/a - Spec handles locales/find_lang
n/a - Spec has needed ldconfig in post and postun
n/a - Package is relocatable and has a reason to be.
OK - Package owns all the directories it creates.
OK - Package has no duplicate files in %files.
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Spec has consistant macro usage.
OK - Package is code or permissible content.
n/a - -doc subpackage needed/used.
OK - Packages %doc files don't affect runtime.
n/a - Headers/static libs in -devel subpackage.
n/a - .pc files in -devel subpackage.
n/a - .so files in -devel subpackage.
n/a - -devel package Requires: %{name} = %{version}-%{release}
n/a - .la files are removed.
n/a - Package is a GUI app and has a .desktop file
OK - Package doesn't own any directories other packages own.
OK - No rpmlint output.

SHOULD Items:

OK - Should include License or ask upstream to include it.
OK - Should build in mock.

Issues/Questions:

1. There is a SDL_perl 2.1.3 on CPAN:
http://search.cpan.org/~dgoehrig/SDL_Perl-2.1.3/
Is that version usable for the packages that use this version?
Or totally diffrent interface? If that package is imported someday
would it conflict with this one?
Comment 2 Hans de Goede 2006-08-15 02:07:58 EDT
(In reply to comment #1)
> Issues/Questions:
> 
> 1. There is a SDL_perl 2.1.3 on CPAN:
> http://search.cpan.org/~dgoehrig/SDL_Perl-2.1.3/
> Is that version usable for the packages that use this version?
> Or totally diffrent interface? 

I haven't tried myself but have been told by the previous maintainers from the
repo that must not be named that that version is not usable, so I assumed it has
a different interface. Also the frozen-bubble package which this bug block
contains: "Requires:       perl-SDL >= 0:1.19.0, perl-SDL < 0:2.0" and an
identical BR.

However I've just checked a few other distros / rpm-repo's and I've found that
rpmforge have frozen-bubble working with perl-SDL 2.1.2 (with a small patch, so
I guess the interface really is different).


> If that package is imported someday
> would it conflict with this one?
> 

I honestly don't know, but since it seems that frozen-bubble can be made to work
with 2.1.x quite easily I'll guess it would be better to make the jumpt to 2.1.x
now. I'll post a new version soon.
Comment 3 Hans de Goede 2006-08-15 06:31:48 EDT
Ok, I've updated both perl-SDL and frozen-bubble to work with the newer perl-SDL.
New perl-SDL here:
Spec URL: http://people.atrpms.net/~hdegoede/perl-SDL.spec
SRPM URL: http://people.atrpms.net/~hdegoede/perl-SDL-2.1.3-1.src.rpm
Comment 4 Hans de Goede 2006-08-15 06:37:12 EDT
p.s.

I think this should only be build for the devel branch now since it breaks
existing apps, which is BAD todo with release versions. Leaving it in that other
repo for FC-4 and FC-5.

Comment 5 Kevin Fenzi 2006-08-15 22:50:07 EDT
The version from comment #3 fails in mock here with (from build.log):
+ echo 'Patch #0 (sdlperl_2.1.2-1.diff.gz):'
Patch #0 (sdlperl_2.1.2-1.diff.gz):
+ /bin/gzip -d
+ patch -p1 -s
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ patch -p1 -b -z .deb
patching file src/OpenGL.xs
+ patch -p1
patching file src/OpenGL.xs
Hunk #1 succeeded at 912 (offset 2 lines).
Hunk #3 succeeded at 1006 (offset 2 lines).
Hunk #5 succeeded at 1090 (offset 2 lines).
+ echo 'Patch #1 (perl-SDL-no-mixertest.patch):'
Patch #1 (perl-SDL-no-mixertest.patch):
+ patch -p1 -b --suffix .no-mixertest -s
The text leading up to this was:
--------------------------
|--- t/mixerpm.t~       2003-03-21 19:48:58.000000000 +0200
|+++ t/mixerpm.t        2004-07-11 00:12:59.851670873 +0300
--------------------------
File to patch:
Skip this patch? [y]
2 out of 2 hunks ignored
error: Bad exit status from /var/tmp/rpm-tmp.2893 (%prep)

Agreed with comment #4. 
Comment 6 Hans de Goede 2006-08-16 00:43:22 EDT
(In reply to comment #5)
> The version from comment #3 fails in mock here with (from build.log):
> + echo 'Patch #0 (sdlperl_2.1.2-1.diff.gz):'
> Patch #0 (sdlperl_2.1.2-1.diff.gz):
> + /bin/gzip -d
> + patch -p1 -s
> + STATUS=0
> + '[' 0 -ne 0 ']'
> + patch -p1 -b -z .deb
> patching file src/OpenGL.xs
> + patch -p1
> patching file src/OpenGL.xs
> Hunk #1 succeeded at 912 (offset 2 lines).
> Hunk #3 succeeded at 1006 (offset 2 lines).
> Hunk #5 succeeded at 1090 (offset 2 lines).
> + echo 'Patch #1 (perl-SDL-no-mixertest.patch):'
> Patch #1 (perl-SDL-no-mixertest.patch):
> + patch -p1 -b --suffix .no-mixertest -s
> The text leading up to this was:
> --------------------------
> |--- t/mixerpm.t~       2003-03-21 19:48:58.000000000 +0200
> |+++ t/mixerpm.t        2004-07-11 00:12:59.851670873 +0300
> --------------------------
> File to patch:
> Skip this patch? [y]
> 2 out of 2 hunks ignored
> error: Bad exit status from /var/tmp/rpm-tmp.2893 (%prep)
> 
> Agreed with comment #4. 

Erm, a patch failing to apply is not mock specific, it seems that for some
reason you are still using the patch from the 1.20.3 package which was updated
for / in the 2.1.3 package. I just tried to install the provided SRPM on a
different PC then where it was developed and there it builds fine.
Comment 7 Kevin Fenzi 2006-08-16 01:12:56 EDT
ugh. I thought I checked all the files, but I missed that patch, you are right. 
Sorry for the trouble...

Since this is pretty much a totally new package,
I will go ahead and run thru my review checklist on it
again. ;)

OK - Package name
OK - Spec file matches base package name.
OK - Meets Packaging Guidelines.
OK - License (LGPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
6ce26e1b710ce52def4ec22637cd5176  SDL_Perl-2.1.3.tar.gz
6ce26e1b710ce52def4ec22637cd5176  SDL_Perl-2.1.3.tar.gz.1
OK - Package compiles and builds on at least one arch.
n/a - Package needs ExcludeArch
OK - BuildRequires correct
n/a - Spec handles locales/find_lang
n/a - Spec has needed ldconfig in post and postun
n/a - Package is relocatable and has a reason to be.
OK - Package owns all the directories it creates.
OK - Package has no duplicate files in %files.
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Spec has consistant macro usage.
OK - Package is code or permissible content.
n/a - -doc subpackage needed/used.
OK - Packages %doc files don't affect runtime.
n/a - Headers/static libs in -devel subpackage.
n/a - .pc files in -devel subpackage.
n/a - .so files in -devel subpackage.
n/a - -devel package Requires: %{name} = %{version}-%{release}
n/a - .la files are removed.
n/a - Package is a GUI app and has a .desktop file
OK - Package doesn't own any directories other packages own.
See below - No rpmlint output.

SHOULD Items:

OK - Should include License or ask upstream to include it.
OK - Should build in mock.

Issues:

1. rpmlint says:

W: perl-SDL strange-permission filter-depends.sh 0755

I think that can be ignored. It should be 755 I would think.

2. I see the package is providing:
perl(Walker)
and
perl(main)
Is that correct or should those be filtered out as well?
Comment 8 Hans de Goede 2006-08-16 01:17:24 EDT
(In reply to comment #7)
> ugh. I thought I checked all the files, but I missed that patch, you are right. 
> Sorry for the trouble...
>
No problem.
 
> 1. rpmlint says:
> 
> W: perl-SDL strange-permission filter-depends.sh 0755
> 
> I think that can be ignored. It should be 755 I would think.
>
Yes very much so.

> 2. I see the package is providing:
> perl(Walker)
> and
> perl(main)
> Is that correct or should those be filtered out as well?
> 

That indeed looks wrong I'll do a new version filtering out those too.
Comment 9 Hans de Goede 2006-08-16 01:43:50 EDT
Version without the wrong Provides:
Spec URL: http://people.atrpms.net/~hdegoede/perl-SDL.spec
SRPM URL: http://people.atrpms.net/~hdegoede/perl-SDL-2.1.3-2.src.rpm

Comment 10 Kevin Fenzi 2006-08-16 02:23:22 EDT
Excellent. That looks all good to go to me, so this package is APPROVED. 
Don't forget to close this as NEXTRELEASE when you have it checked in and built 
for devel. 
Comment 11 Hans de Goede 2006-08-16 05:58:01 EDT
Thanks I'll import it right away but I'll wait with the building until
frozen-bubble is approved too, so I can build them in quick succession in order
to not brake anybody's systems.
Comment 12 Hans de Goede 2006-08-26 01:35:12 EDT
Build (finally), closing.

Note You need to log in before you can comment on or make changes to this bug.