Bug 489751 (btanks)

I'll review it.

Notes:

> %patch0 -p1

Add postfix. E.g. you should invoke %patch in the following way:

%patch0 -p1 -b .some_descriptive_postfix

> dos2unix -k *.txt ChangeLog *.url LICENSE EXCEPTION

Consider using sed -i 's|\r||g' instead (just to avoid dos2unix as a BR).

> iconv -f latin1 -t utf-8 EXCEPTION > EXCEPTION.new
touch -r EXCEPTION EXCEPTION.new
mv -f EXCEPTION.new EXCEPTION

This piece of spec (and similar ones) is not fail-safe. You should change it to something like this:

iconv -f latin1 -t utf-8 EXCEPTION > EXCEPTION.new && touch -r EXCEPTION EXCEPTION.new && mv -f EXCEPTION.new EXCEPTION

Also, you should use some bash syntax sugar:

"mv -f EXCEPTION.new EXCEPTION" == "mv -f EXCEPTION{.new,}"
"touch -r EXCEPTION EXCEPTION.new" == "touch -r EXCEPTION{,.new}"

> %files
...
%{_libdir}/*.so

This is not a blocker, actually, but I believe, that dlopened objects should be in a proper subdir in %_libdir (for example %{_libdir}/%{name} ). However, there are many exceptions (unixODBC, tcl/tk among them), who stores their *so-libraries in %_libdir.

Spec URL: http://atorkhov.fedorapeople.org/btanks.spec
SRPM URL: http://atorkhov.fedorapeople.org/btanks-0.8.7686-2.fc10.src.rpm

* Tue Mar 17 2009 Alexey Torkhov <atorkhov> - 0.8.7686-3
- Add patch backups
- Simplify scripts a bit
- Move libs to -libs subpackage to ensure better work in multilib environment
- Cleanier remove-smpeg patch

(In reply to comment #2)
> > %patch0 -p1
> 
> Add postfix. E.g. you should invoke %patch in the following way:
> 
> %patch0 -p1 -b .some_descriptive_postfix

I'm wondering why those backups are needed? I never found it useful.
Anyway, fixed.

> > dos2unix -k *.txt ChangeLog *.url LICENSE EXCEPTION
> 
> Consider using sed -i 's|\r||g' instead (just to avoid dos2unix as a BR).

No, I don't want to do it. Dos2unix is tiny and useful. Otherwise, I had to do touch to save timestamps.

> > iconv -f latin1 -t utf-8 EXCEPTION > EXCEPTION.new
> touch -r EXCEPTION EXCEPTION.new
> mv -f EXCEPTION.new EXCEPTION
> 
> This piece of spec (and similar ones) is not fail-safe. You should change it
> to something like this:
> 
> iconv -f latin1 -t utf-8 EXCEPTION > EXCEPTION.new && touch -r EXCEPTION
> EXCEPTION.new && mv -f EXCEPTION.new EXCEPTION

Those pieces are equivalent, as script are executed under -e shell option and it will exit after first command that fails.

> Also, you should use some bash syntax sugar:
> 
> "mv -f EXCEPTION.new EXCEPTION" == "mv -f EXCEPTION{.new,}"
> "touch -r EXCEPTION EXCEPTION.new" == "touch -r EXCEPTION{,.new}"

Fixed.

> > %files
> ...
> %{_libdir}/*.so
> 
> This is not a blocker, actually, but I believe, that dlopened objects should
> be in a proper subdir in %_libdir (for example %{_libdir}/%{name} ). However,
> there are many exceptions (unixODBC, tcl/tk among them), who stores their
> *so-libraries in %_libdir.  

Actually, that is how it is made now - btanks and bted are linked against libs under %{_libdir} (check ldd), but dlopen'ed libbt_objects plugin is in %{_libdir}/%{name}.

Spec URL: http://atorkhov.fedorapeople.org/btanks.spec
SRPM URL: http://atorkhov.fedorapeople.org/btanks-0.8.7686-4.fc10.src.rpm

* Tue Mar 17 2009 Alexey Torkhov <atorkhov> - 0.8.7686-4
- Split data to subpackage instead of libs

Also, running ldconfig.

Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1246013

Spec URL: http://atorkhov.fedorapeople.org/btanks.spec
SRPM URL: http://atorkhov.fedorapeople.org/btanks-0.8.7686-5.fc10.src.rpm

* Tue Mar 17 2009 Alexey Torkhov <atorkhov> - 0.8.7686-5
- Add license for libraries

Please, fix - use full patch for Source0.

REVIEW:

- rpmlint is not silent:

[petro@host-12-116 Desktop]$ rpmlint btanks-*rpm
btanks.i386: W: no-soname /usr/lib/libclunk.so
btanks.i386: W: shared-lib-calls-exit /usr/lib/libclunk.so exit
btanks.i386: W: no-soname /usr/lib/libsdlx.so
btanks.i386: W: no-soname /usr/lib/libbt.so
btanks.i386: W: shared-lib-calls-exit /usr/lib/libbt.so exit
btanks.i386: W: no-soname /usr/lib/libmrt.so
btanks-data.i386: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 7 warnings.
[petro@host-12-116 Desktop]$ 

+ The package is named according to the Package Naming Guidelines .
+ The spec file name matches the base package %{name}, in the format %{name}.spec
+ The package meets the Packaging Guidelines .
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines .
+ The License field in the package spec file matchуы the actual license. 
+ The text of the license(s) is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package matches the upstream source, as provided in the spec URL.

[petro@host-12-116 Desktop]$ md5sum btanks-0.8.7686.tar.bz2*
f5e4076e8562f4ad54fefeceaa37870d  btanks-0.8.7686.tar.bz2
f5e4076e8562f4ad54fefeceaa37870d  btanks-0.8.7686.tar.bz2.from_srpm
[petro@host-12-116 Desktop]$

+ The package successfully compiles and builds into binary rpms on at least one primary architecture.
+ All build dependencies are listed in BuildRequires.
+ The package calls ldconfig in %post and %postun.
+ A package owns all directories that it creates.
+ Doesn't contain files, listed more than once in the spec file's %files listings.
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ The package consistently uses macros.
+ The package contains code, or permissable content.
+ No large documentation files
+ Everything, the package includes as %doc, does not affect the runtime of the application.
+ No header files
+ No static libraries
+ No pkgconfig(.pc) files
+ The package does not contain any .la libtool archives
+ The package includes a %{name}.desktop file

- The %{name}.desktop file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation.

+ The package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [25]
+ All filenames in rpm packages are valid UTF-8.


Ok, here are my final notes:

* please fix Source0, 
* fix spec to properly install desktop-file
* please, comment the above rpmlint warnings.

Spec URL: http://atorkhov.fedorapeople.org/btanks.spec
SRPM URL: http://atorkhov.fedorapeople.org/btanks-0.8.7686-6.fc10.src.rpm

* Tue Mar 17 2009 Alexey Torkhov <atorkhov> - 0.8.7686-6
- Fixed source url
- Properly installing desktop files

> - rpmlint is not silent:

> btanks.i386: W: no-soname /usr/lib/libclunk.so
> btanks.i386: W: no-soname /usr/lib/libsdlx.so
> btanks.i386: W: no-soname /usr/lib/libbt.so
> btanks.i386: W: no-soname /usr/lib/libmrt.so

Those libraries are unversioned and thus are missing soname.

> btanks.i386: W: shared-lib-calls-exit /usr/lib/libclunk.so exit
> btanks.i386: W: shared-lib-calls-exit /usr/lib/libbt.so exit

Eh... that is how those libraries are written upstream.

> btanks-data.i386: W: no-documentation

No need to duplicate documentation in subpackage.

Spec URL: http://atorkhov.fedorapeople.org/btanks.spec
SRPM URL: http://atorkhov.fedorapeople.org/btanks-0.8.7686-7.fc10.src.rpm

* Tue Mar 17 2009 Alexey Torkhov <atorkhov> - 0.8.7686-7
- Add forgotten desktop-file-utils build requires

Spec URL: http://atorkhov.fedorapeople.org/btanks.spec
SRPM URL: http://atorkhov.fedorapeople.org/btanks-0.8.7686-7.fc10.src.rpm

* Tue Mar 17 2009 Alexey Torkhov <atorkhov> - 0.8.7686-7
- Add forgotten desktop-file-utils build requires
- Fixing bad BuildRoot

Ok, all issues mentioned above, has been resolved (or, at least, commented), so this package is


APPROVED

New Package CVS Request
=======================
Package Name: btanks
Short Description: Funny battle game with tanks
Owners: atorkhov
Branches: F-9 F-10
InitialCC:

cvs done.

btanks-0.8.7686-8.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/btanks-0.8.7686-8.fc9

btanks-0.8.7686-8.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/btanks-0.8.7686-8.fc10

btanks-0.8.7686-8.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

btanks-0.8.7686-8.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.