Bug 226402

I'm behind a windows machine ATM, so no full review, but a few items to fix and 
a few questions to get started:

SHOULDFIX items:
* replace "--x-includes=/usr/include --x-libraries=/usr/%{_lib}"
  with "--x-includes=%{_includedir} --x-libraries=%{_libdir}
* BuildRequires: nasm should be: %ifarch %{ix86}, I doubt ppc owners will
  be amused when they try to rebuild SDL from srpm for some reason and then
  need to install nasm.

questions:
* Why this? : "export tagname=CC"
* Why add -O3 is there any bench mark proof this is benificial?
* Since you now pass "--x-includes=/usr/include --x-libraries=/usr/%{_lib}",
  to work around configure's X-detection, do you still need:
  BuildRequires imake and libXt-devel?
* Does devel really Requires libXt-devel?

I must say all in all a pretty good specfile, I've seen much worse (both in FE 
as in FC).

An important question when moving on with this is what todo with current bz 
tickets against SDL. Quite a few of them seem legitimate and not all that hard 
to fix. I don't know however if open bz tickets should be concidered blockers 
for the review. I see that someone has made one of them block this ticket, but 
that can be removed. AFAIK there are no rules for this, we could ask the 
mailinglist but that usually leads to much ado about nothing. In my opninion we 
should try to fix as many BZ's against SDL as possible during this review, but 
not let them block the review, agreed?

Which brings me to the next subject one of the main reasons why I've decided to 
review SDL and not just any package is because I'm very active in packaging 
games and gaming related libraries (allegro (ask jnovy), CLanLib 0.6 and 0.8, 
plib) and as an experiment in co-maintainer ship between (former) FE and FC 
maintainers I would like to become a co-maintainer of SDL.

Judging from the current open BZ tickets against SDL, of which most seem easy 
to fix, currently other work has higher priorities then SDL, and thus you could 
use a hand. I don't know howto shape this co-maintainership for now I'll try to 
take a look at some of the open BZ tickets and write fixes for those, notice 
btw that bug 217389 already contains fix I've reviewed the fix and it looks 
good to me.

Unfortunately I currently don't have internet access at home so I'll only be 
able to communicate about this mon, wed, thu and fri.

Answers for questions:
- tagname was needed for old libtool
- O3 was needed for old gcc
- libXt-devel was requires, beacuse there was a bug in autofoo*, where it did not 
  find X11 at all with missing libXt, same for imake
- x-includes and x-libraries is not needed anymore after fixing X11 library path 
  problem in configure

Fixed in rawhide in package SDL-1.2.11-2.

Thomas,

Thanks for fixing most open SDL bugs and for fixing/answering most of my
questions. As stated however, my initial list of questions wasn't a full review
yet, so bug isn't resolved yet, reopening.

And then now the full review:

MUST:
=====
0 rpmlint output is:
W: SDL-devel summary-ended-with-dot Files needed to develop Simple DirectMedia
Layer applications.
W: SDL rpm-buildroot-usage %prep rm -rf %{buildroot}
W: SDL macro-in-%changelog build
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License ok
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel x86_64
* BR: ok
* No locales
* ldconfig run for shared libraries
* Not relocatable
0 Package owns / or requires all dirs
* No duplicate files & Permissions
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* -devel package as needed
* no .desktop file required

Should Fix
==========
* remove this line: "CFLAGS="$RPM_OPT_FLAGS" CXXFLAGS="$RPM_OPT_FLAGS" \",
  this is Superfluous, as %configure already does this itself
* remove "README-SDL.txt COPYING CREDITS BUGS" from %files devel's %doc,
  they are already included in the main %doc, which is required by -devel
* bug 234823 bug 208212

Must Fix
========
* From rpmlint:
  * Drop the . at the end of the -devel package summary
  * Drop "rm -rf %{buildroot}" from %prep (already done at the begin of
    %install)
  * change %build in %changelog to %%build
* Drop the static library from -devel
* Add "Requires: pkgcconfig" to -devel subpackage for proper ownership of
  the /usr/%{_lib}/pkgconfig directory
* Add "Requires: automake" to -devel subpackage for proper ownership of
  the /usr/share/aclocal directory

As said before all in all a pretty good package! and once the above fixes have
been applied its "perfect"

Why is dropping the static library a must?

bug 208212 has already been fixed in package SDL-1.2.11-1.

(In reply to comment #4)
> Why is dropping the static library a must?
> 
The line between Must and Should is thin, anything is debatable. In general
there are little good reasons to keep a static library around. Do you want to
keep it around and if so, why?

> bug 208212 has already been fixed in package SDL-1.2.11-1.

Okay, then maybe it should be closed?

Why compile support for esound?

I even wonder why we need arts support after 2002 or 2003 (I can't remember
exactly when alsa introduces support for software mixing).

ping?

Yes, this bug can be closed.

BTW: I am glad, that we still have esd and arts support in SDL. Esd support is
currently needed because of problems with pulseaudio.

(In reply to comment #8)
> Yes, this bug can be closed.
> 

No it can't I just checked SDL.spec in devel CVS and the following items from
the review still hold true:


Must Fix
========
* From rpmlint:
  * Drop the . at the end of the -devel package summary
  * Drop "rm -rf %{buildroot}" from %prep (already done at the begin of
    %install)
  * change %build in %changelog to %%build
* Drop the static library from -devel
* Add "Requires: pkgcconfig" to -devel subpackage for proper ownership of
  the /usr/%{_lib}/pkgconfig directory
* Add "Requires: automake" to -devel subpackage for proper ownership of
  the /usr/share/aclocal directory

Should Fix
==========
* remove this line: "CFLAGS="$RPM_OPT_FLAGS" CXXFLAGS="$RPM_OPT_FLAGS" \",
  this is Superfluous, as %configure already does this itself
* remove "README-SDL.txt COPYING CREDITS BUGS" from %files devel's %doc,
  they are already included in the main %doc, which is required by -devel


Or basicly, you didn't fix a _single_ thing from my review.

Oups, I am sorry - I misread your comment from above.

Fixed version will be in rawhide in a few minutes.

Okay,

I just checked SDL.spec in CVS again, much better now, thanks! Closing.

I see that you've also updated to upstream 1.2.13, good! And that by doing that
you've fixed bug 310841 and bug 426475. And for good measure you've also fixed
bug 426579, good work!

Shouldn't those be closed then too? Shall I or will you?

These are bugs for F-8. The F-8 update package will make it into testing.

(In reply to comment #12)
> These are bugs for F-8. The F-8 update package will make it into testing.

Ah right, sorry.

Hans, it seems that you closed this but fedora-review is still set to '?'.  I'm
guessing you intended to set it to '+'; I'll do that now.