Bug 243147 - Review Request: arm-gp2x-linux-SDL - Cross Compiled SDL Library targeted at arm-gp2x-linux
Review Request: arm-gp2x-linux-SDL - Cross Compiled SDL Library targeted at a...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Kofler
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-07 11:15 EDT by Hans de Goede
Modified: 2010-09-06 19:43 EDT (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-08-07 10:11:12 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2007-06-07 11:15:11 EDT
Spec URL: http://people.atrpms.net/~hdegoede/arm-gp2x-linux-SDL.spec
SRPM URL: http://people.atrpms.net/~hdegoede/arm-gp2x-linux-SDL-1.2.9-2.fc8.src.rpm
Description:
This is a Cross Compiled version of the SDL Library, which can be used to
compile and link binaries for the arm-gp2x-linux platform, instead of for the
native i386 platform.
Comment 1 Kevin Kofler 2007-07-26 15:41:36 EDT
Reviewing this one.
Comment 2 Kevin Kofler 2007-07-28 09:28:50 EDT
This one is also missing the %defattr in the %files section.
Comment 3 Kevin Kofler 2007-08-03 15:38:15 EDT
Can you please submit an updated specfile for this with the missing %defattr 
added and the License tag updated to the new standard (specifying LGPL version) 
so we don't waste time on these during the review?
Comment 4 Hans de Goede 2007-08-05 15:03:26 EDT
New version with these changes:
* Sat Aug  4 2007 Hans de Goede <j.w.r.degoede@hhs.nl> 1.2.9-3  
- Add missing %%defattr to %%files   
- Strip some unneeded file removals / additions from Patch0, dramatically
  reducing it in size
- Update License tag for new Licensing Guidelines compliance
- Do NOT provide and require native .so files <sigh>

here:
Spec URL: http://people.atrpms.net/~hdegoede/arm-gp2x-linux-SDL.spec
SRPM URL: http://people.atrpms.net/~hdegoede/arm-gp2x-linux-SDL-1.2.9-3.fc8.src.rpm
Comment 5 Kevin Kofler 2007-08-06 10:46:32 EDT
Currently going through the review checklist. Any particular reason you aren't 
using %{?_smp_mflags}? The native SDL package is built with it (and has been 
even back when it was at 1.2.9).
Comment 6 Hans de Goede 2007-08-06 10:52:18 EDT
(In reply to comment #5)
> Currently going through the review checklist. Any particular reason you aren't 
> using %{?_smp_mflags}? The native SDL package is built with it (and has been 
> even back when it was at 1.2.9).

No reason, probably a cut and paste error.
Comment 7 Kevin Kofler 2007-08-06 11:11:13 EDT
MUST Items:
+ rpmlint output:
  + SRPM:
    W: arm-gp2x-linux-SDL invalid-license LGPLv2+
    This is the F7 rpmlint being out of date. :-)
    E: arm-gp2x-linux-SDL configure-without-libdir-spec
    This one's OK for a cross library. (There's no 64-bit GP2X ;-).)
  + noarch RPM:
    W: arm-gp2x-linux-SDL 
devel-file-in-non-devel-package /usr/arm-gp2x-linux/include/SDL/SDL_cpuinfo.h
    (and more like this). OK because this is a cross-development package, and 
these are all target development files. It would make no sense to make a 
separate devel vs. runtime part because we aren't going to run ARM GP2X 
binaries on i386/x86_64/ppc/... Fedora anyway.
    E: arm-gp2x-linux-SDL 
library-without-ldconfig-postin /usr/arm-gp2x-linux/lib/libSDL-1.2.so.0.7.2
    E: arm-gp2x-linux-SDL 
library-without-ldconfig-postun /usr/arm-gp2x-linux/lib/libSDL-1.2.so.0.7.2
    OK, as these are target libraries which aren't even in the ldconfig search 
path. The required symlinks are already there anyway.
    W: arm-gp2x-linux-SDL invalid-license LGPLv2+
    Again, the F7 rpmlint being out of date.
    E: arm-gp2x-linux-SDL 
arch-independent-package-contains-binary-or-object /usr/arm-gp2x-linux/lib/libSDLmain.a
    E: arm-gp2x-linux-SDL 
arch-independent-package-contains-binary-or-object /usr/arm-gp2x-linux/lib/libSDL.a
    E: arm-gp2x-linux-SDL 
arch-independent-package-contains-binary-or-object /usr/arm-gp2x-linux/lib/libSDL-1.2.so.0.7.2
    Again, this is OK because those are target files.
    W: arm-gp2x-linux-SDL non-standard-dir-in-usr arm-gp2x-linux
    This one's OK too for a cross-library package.
+ named and versioned according to the Package Naming Guidelines
+ spec file name matches base package name
+ Packaging Guidelines:
  + License LGPLv2+ OK, matches actual license
  + No known patent problems
  + No emulator, no firmware, no binary-only or prebuilt components
  + Complies with the FHS (with the cross-toolchain exception 
for %{_prefix}/%{target})
  + proper changelog, tags, BuildRoot, Requires, BuildRequires, Summary, 
Description
  + no non-UTF-8 characters
  + relevant documentation is included
  + It would make no sense to use RPM_OPT_FLAGS here because this is a target 
package, which is built using a cross GCC which won't understand stuff 
like -fstack-protector, and for which x86 -march and -mtune switches definitely 
don't make sense. Thus the omission of RPM_OPT_FLAGS is correct.
  + no -debuginfo package because this is noarch
  + no host static libraries nor .la files
    (I think we can give the target static libraries a pass. This isn't a 
Fedora target, so trying to apply our static library policies to the target 
wouldn't make much sense.)
  + no duplicated system libraries
  + no rpaths (no host executables or libraries at all, I also ran readelf -d 
on the target shared library to make sure and there's no rpath there either)
  + no configuration files, so %config guideline doesn't apply
  + no init scripts, so init script guideline doesn't apply
  + no GUI programs, so no .desktop file present or needed
  + no timestamp-clobbering file commands
  ! _smp_mflags not used
  + scriptlets are valid
  + not a web application, so web application guideline doesn't apply
  + no conflicts
+ complies with all the legal guidelines
+ COPYING (yes, it's the LGPL, even if it's not named COPYING.LIB) included 
as %doc
+ spec file written in American English
+ spec file is legible
+ source matches upstream:
  MD5SUM: 80919ef556425ff82a8555ff40a579a0
  SHA1SUM: 8140de00e73ccdbdee196fa8fd9952ddb3cc75f1
  (This one looks like a pretty pointless exercise given the 380 KB patch 
applied to it, but... ;-) )
+ builds on at least one arch (F7 i386 live system)
+ no known non-working arches, so no ExcludeArch needed
+ no missing BR
+ no translations, so translation/locale guidelines don't apply
+ no host shared libraries, so no ldconfig calls needed
+ package not relocatable
+ ownership correct (owns package-specific directories, doesn't own directories 
owned by another package)
+ no duplicate files in %files
+ permissions set properly (%defattr present)
+ %clean section present and correct
+ macros used where possible (%configure not used for several reasons, 
including it playing jokes with --target and using host-specific RPM_OPT_FLAGS)
+ no non-code content
+ no large documentation files, so no -doc package needed
+ %doc files not required at runtime
+ no host headers, target headers are OK in this cross-development package
+ no host static libraries, so no -static package needed
+ no .pc files, so no Requires: pkgconfig needed
+ no host shared libraries, so .so symlink guidelines don't apply
+ no -devel package, so the guideline to require the main package in it doesn't 
apply
+ no .la files
+ no GUI programs, so no .desktop file needed
+ buildroot is deleted at the beginning of %install
  (same nitpick about mkdir $RPM_BUILD_ROOT as for arm-gp2x-linux-binutils)
+ all filenames are valid UTF-8

SHOULD Items:
+ license already included upstream
+ no translations for description and summary provided by upstream
* Skipping mock test.
* Skipping the "all architectures" test, I only have i386.
+ package functions as described:
  The following test program:
  #include <stdlib.h>
  #include <SDL.h>
  
  int main(void)
  {
    if (SDL_Init(SDL_INIT_AUDIO|SDL_INIT_VIDEO) < 0) {
      fprintf(stderr, "Unable to init SDL: %s\n", SDL_GetError());
      return 1;
    }
    atexit(SDL_Quit);
    return 0;
  }
  compiles and links fine using:
  arm-gp2x-linux-gcc -Wall -Wextra -Os gp2x-sdl-test.c 
`arm-gp2x-linux-sdl-config --cflags --libs` -o gp2x-sdl-test
+ scriptlets are sane
+ no subpackages other than -devel, so "Usually, subpackages other than devel 
should require the base package using a fully versioned dependency." is 
irrelevant
+ no .pc files, so "placement of .pc files" is irrelevant
+ no file dependencies

MUST FIX:
* please use _smp_mflags as there's no good reason not to
Comment 8 Hans de Goede 2007-08-06 13:53:28 EDT
Many thanks for the review!

(In reply to comment #7)
> MUST FIX:
> * please use _smp_mflags as there's no good reason not to

Fixed, new version here:
Spec URL: http://people.atrpms.net/~hdegoede/arm-gp2x-linux-SDL.spec
SRPM URL: http://people.atrpms.net/~hdegoede/arm-gp2x-linux-SDL-1.2.9-4.fc8.src.rpm

p.s.

I don't want to discourage you / abuse your kindness, but SDL and zlib are just
the top of the mountain, to be able to provide a full SDK on which most gp2x
software can be found, the following are needed to:
tremor (integer vorbis decoder)
libogg
 SDL_mixer
libpng
libjpg
libtiff
 SDL_image

And I might be forgetting some. I haven't submitted these yet as I still need to
clean them up, and I first wanted to get some more experience, but I will be
slowly submitting them in the near future, now that the base is there. Once I've
submitted a few I'll drop you a note, if you don't feel like reviewing these too
thats 100% understandable, no hard feelings.
Comment 9 Kevin Kofler 2007-08-06 14:00:47 EDT
Verified fixed. => APPROVED

For the other libraries needed for a complete SDK: We'll see about these once 
you submit them. We can still hope that the presence of the base packages 
(especially the really hard ones like GCC) will motivate more people to help 
out. :-) Otherwise, I will, it would be sad to see half a SDK in and the other 
half stuck in the review queue forever. It's too bad there aren't more people 
interested in cross-toolchains.
Comment 10 Hans de Goede 2007-08-06 15:38:33 EDT
New Package CVS Request
=======================
Package Name:      arm-gp2x-linux-SDL
Short Description: Cross Compiled SDL Library targeted at arm-gp2x-linux
Owners:            j.w.r.degoede@hhs.nl
Branches:          FC-6 F-7 devel
InitialCC:         <empty>
Comment 11 Tom "spot" Callaway 2007-08-06 17:19:54 EDT
Cvs done.
Comment 12 Hans de Goede 2007-08-07 10:11:12 EDT
imported and build for rawhide (rest is slow due to new F-7 procedures delaying
needed other bits)
Comment 13 Hans de Goede 2010-09-06 04:46:48 EDT
Package Change Request
======================
Package Name: arm-gp2x-linux-SDL
New Branches: el6
Owners: jwrdegoede
Comment 14 Kevin Fenzi 2010-09-06 19:43:53 EDT
Git done (by process-git-requests).

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