Bug 701079 (wmSun)

Just a quick comment: $RPM_BUILD_ROOT and %{buildroot} are mixed.

Thanks for the hint.

New files:
Spec URL: http://mariobl.fedorapeople.org/Review/SPECS/wmSun.spec
SRPM URL:
http://mariobl.fedorapeople.org/Review/SRPMS/wmSun-1.03-2.fc15.src.rpm

I'll take this one :)

Review:

+ OK
- NA
? Issue

+ Package meets naming and packaging guidelines
+ Spec file matches base package name.
+ Spec has consistant macro usage.
+ Meets Packaging Guidelines.
+ License
+ License field in spec matches
+ License file included in package
+ Spec in American English
+ Spec is legible.
+ Sources match upstream md5sum:
[ankur@ankur rpmbuild]$ md5sum wmSun-1.03.tar.gz SOURCES/wmSun-1.03.tar.gz 
34a2ea132f1a04e18be2d93abce40722  wmSun-1.03.tar.gz
34a2ea132f1a04e18be2d93abce40722  SOURCES/wmSun-1.03.tar.gz


- Package needs ExcludeArch
+ BuildRequires correct
- Spec handles locales/find_lang
- Package is relocatable and has a reason to be.
+ Package has %defattr and permissions on files is good.
+ Package has a correct %clean section.
+ Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
+ Package is code or permissible content.
- Doc subpackage needed/used.
+ Packages %doc files don't affect runtime.

- Headers/static libs in -devel subpackage.
- Spec has needed ldconfig in post and postun
- .pc files in -devel subpackage/requires pkgconfig
- .so files in -devel subpackage.
- -devel package Requires: %{name} = %{version}-%{release}
- .la files are removed.

? Package is a GUI app and has a .desktop file
^^^

It is a GUI, isn't it? I haven't seen a desktop file in the source. If you can, please create one and add it as an extra source. Do also send this upstream. (Only if it is a GUI of course.)

+ Package compiles and builds on at least one arch.
+ Package has no duplicate files in %files.
+ Package doesn't own any directories other packages own.
+ Package owns all the directories it creates.
? No rpmlint output.
[ankur@ankur SRPMS]$ rpmlint ../SPECS/wmSun.spec wmSun-1.03-2.fc15.src.rpm /var/lib/mock/fedora-rawhide-i386/result/*.rpm
../SPECS/wmSun.spec:51: W: macro-in-%changelog %{buildroot}
wmSun.src: W: spelling-error Summary(en_US) dockapp -> dock app, dock-app, paddock
wmSun.src: W: spelling-error %description -l en_US dockapp -> dock app, dock-app, paddock
wmSun.src:51: W: macro-in-%changelog %{buildroot}
wmSun.i686: W: spelling-error Summary(en_US) dockapp -> dock app, dock-app, paddock
wmSun.i686: W: spelling-error %description -l en_US dockapp -> dock app, dock-app, paddock
wmSun.i686: E: incorrect-fsf-address /usr/share/doc/wmSun-1.03/COPYING
wmSun.src: W: spelling-error Summary(en_US) dockapp -> dock app, dock-app, paddock
wmSun.src: W: spelling-error %description -l en_US dockapp -> dock app, dock-app, paddock
wmSun.src:51: W: macro-in-%changelog %{buildroot}
wmSun-debuginfo.i686: E: empty-debuginfo-package
4 packages and 1 specfiles checked; 2 errors, 9 warnings.

^^^ 

-> I see you're using the -s (strip) option while installing. Please do not do that.
http://fedoraproject.org/wiki/Packaging:Debuginfo#Useless_or_incomplete_debuginfo_packages_due_to_packaging_issues

-> The macro in changelog is ignorable. 

+ final provides and requires are sane:
[ankur@ankur result]$ review-req-check 
== wmSun-1.03-2.fc16.i686.rpm ==
Provides:
wmSun = 1.03-2.fc16
wmSun(x86-32) = 1.03-2.fc16

Requires:
libX11.so.6  
libXext.so.6  
libXpm.so.4  
libc.so.6  
libc.so.6(GLIBC_2.0)  
libc.so.6(GLIBC_2.1)  
libm.so.6  
libm.so.6(GLIBC_2.0)  
rtld(GNU_HASH)  

== wmSun-1.03-2.fc16.src.rpm ==
Provides:

Requires:
libXext-devel  
libXpm-devel  

== wmSun-debuginfo-1.03-2.fc16.i686.rpm ==
Provides:
wmSun-debuginfo = 1.03-2.fc16
wmSun-debuginfo(x86-32) = 1.03-2.fc16

Requires:


SHOULD Items:

+ Should build in mock.
+ Should build on all supported archs
- Should function as described.
+ Should have sane scriptlets.
- Should have subpackages require base package with fully versioned depend.
+ Should have dist tag
+ Should package latest version
- check for outstanding bugs on package. (For core merge reviews)

Issues:

1. buildroot tag isn't required any more
2. the clean section isn't required any more
3. Please report the incorrect FSF address upstream.
4. Please remove the -s flag from install. It should then generate debuginfo. 

Small, easily correctable issues :)

Thanks,
Ankur.

(In reply to comment #4)

> ? Package is a GUI app and has a .desktop file
> ^^^
> 
> It is a GUI, isn't it? I haven't seen a desktop file in the source. If you can,
> please create one and add it as an extra source. Do also send this upstream.
> (Only if it is a GUI of course.)

The package has a GUI, but a *.desktop file is not needed for Windowmaker dockapps, because users don't start them from a menu. Usually, dockapps will be launched from a startup script. No one of the current dockapp packages in Fedora has a desktop file. Look here what "Spot" said:

https://bugzilla.redhat.com/show_bug.cgi?id=478744#c4

> 3. Please report the incorrect FSF address upstream.

Impossible, unfortunately. The development is down for a long time. In such cases, I don't have to do anything, because there is no chance to get it ever fixed upstream:

https://bugzilla.redhat.com/show_bug.cgi?id=690919#c5


Updated package:

Spec URL: http://mariobl.fedorapeople.org/Review/SPECS/wmSun.spec
SRPM URL:
http://mariobl.fedorapeople.org/Review/SRPMS/wmSun-1.03-3.fc15.src.rpm

(In reply to comment #5)
> (In reply to comment #4)
> 
> > ? Package is a GUI app and has a .desktop file
> > ^^^
> > 
> > It is a GUI, isn't it? I haven't seen a desktop file in the source. If you can,
> > please create one and add it as an extra source. Do also send this upstream.
> > (Only if it is a GUI of course.)
> 
> The package has a GUI, but a *.desktop file is not needed for Windowmaker
> dockapps, because users don't start them from a menu. Usually, dockapps will be
> launched from a startup script. No one of the current dockapp packages in
> Fedora has a desktop file. Look here what "Spot" said:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=478744#c4
> 
> > 3. Please report the incorrect FSF address upstream.
> 
> Impossible, unfortunately. The development is down for a long time. In such
> cases, I don't have to do anything, because there is no chance to get it ever
> fixed upstream:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=690919#c5
> 
> 
> Updated package:
> 
> Spec URL: http://mariobl.fedorapeople.org/Review/SPECS/wmSun.spec
> SRPM URL:
> http://mariobl.fedorapeople.org/Review/SRPMS/wmSun-1.03-3.fc15.src.rpm

Hello!

[ankur@ankur SRPMS]$ rpmlint ../SPECS/wmSun.spec wmSun-1.03-3.fc15.src.rpm /var/lib/mock/fedora-rawhide-i386/result/*.rpm
../SPECS/wmSun.spec:51: W: macro-in-%changelog %{buildroot}
wmSun.src: W: spelling-error Summary(en_US) dockapp -> dock app, dock-app, paddock
wmSun.src: W: spelling-error %description -l en_US dockapp -> dock app, dock-app, paddock
wmSun.src:51: W: macro-in-%changelog %{buildroot}
wmSun.i686: W: spelling-error Summary(en_US) dockapp -> dock app, dock-app, paddock
wmSun.i686: W: spelling-error %description -l en_US dockapp -> dock app, dock-app, paddock
wmSun.i686: E: incorrect-fsf-address /usr/share/doc/wmSun-1.03/COPYING
wmSun.src: W: spelling-error Summary(en_US) dockapp -> dock app, dock-app, paddock
wmSun.src: W: spelling-error %description -l en_US dockapp -> dock app, dock-app, paddock
wmSun.src:51: W: macro-in-%changelog %{buildroot}
wmSun-debuginfo.i686: E: debuginfo-without-sources
4 packages and 1 specfiles checked; 2 errors, 9 warnings.


- You can maybe change the spelling (dockapp -> dock app).

- You can also get rid of the rm -rf $RPM_BUILD_ROOT in the install section. It is not needed any more. 

- Debuginfo is still empty. This is because the fedora optflags are not being honoured in the build:

DEBUG: + make 'CFLAGS=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -Wl,-z,relro -m32 -march=i6
86 -mtune=atom -fasynchronous-unwind-tables' -j4
DEBUG: gcc  -DLinux -c wmSun.c -o wmSun.o -I/usr/X11R6/include/X11
DEBUG: gcc  -DLinux -c SunRise.c -o SunRise.o -I/usr/X11R6/include/X11
DEBUG: gcc  -DLinux -c ../wmgeneral/wmgeneral.c -o ../wmgeneral/wmgeneral.o -I/usr/X11R6/include/X11
DEBUG: gcc   -lm -o wmSun wmSun.o SunRise.o ../wmgeneral/wmgeneral.o -I/usr/X11R6/include/X11 -L/usr/X11R6/lib -lXpm -lX11 -lXext


Even though CFLAGS has been defined in the makefile, they aren't used in the compile command. Change the make command as so:

make COPTS='%{optflags}' %{?_smp_mflags}

This fixes it. 

Thanks,
Ankur

(In reply to comment #6)
> - You can maybe change the spelling (dockapp -> dock app).
> 
It is the usual spelling for Windowmaker related applets:
http://en.wikipedia.org/wiki/Dockapp

> - You can also get rid of the rm -rf $RPM_BUILD_ROOT in the install section. It
> is not needed any more. 
>
Done.
> - Debuginfo is still empty. This is because the fedora optflags are not being
> honoured in the build:
...
> make COPTS='%{optflags}' %{?_smp_mflags}
> 
Done.

Updated package:

Spec URL: http://mariobl.fedorapeople.org/Review/SPECS/wmSun.spec
SRPM URL:
http://mariobl.fedorapeople.org/Review/SRPMS/wmSun-1.03-4.fc15.src.rpm

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

Package looks good. Please do inform upstream about the incorrect FSF address when you can. 

Final rpmlint output:

[ankur@ankur SRPMS]$ rpmlint /var/lib/mock/fedora-rawhide-i386/result/*.rpm
wmSun.i686: W: spelling-error Summary(en_US) dockapp -> dock app, dock-app, paddock
wmSun.i686: W: spelling-error %description -l en_US dockapp -> dock app, dock-app, paddock
wmSun.i686: E: incorrect-fsf-address /usr/share/doc/wmSun-1.03/COPYING
wmSun.src: W: spelling-error Summary(en_US) dockapp -> dock app, dock-app, paddock
wmSun.src: W: spelling-error %description -l en_US dockapp -> dock app, dock-app, paddock
wmSun.src:54: W: macro-in-%changelog %{buildroot}
wmSun-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/wmSun-1.03/wmSun/wmSun.c
3 packages and 0 specfiles checked; 2 errors, 5 warnings.


Ignorable. 

This package is XXX APPROVED XXX :D

Many thanks for your review!

(In reply to comment #8)
> Package looks good. Please do inform upstream about the incorrect FSF address
> when you can. 

As I already wrote, upstream is dead. But there is a software pool for Windowmaker dockapps at http://www.dockapps.org. I will leave a comment there.

New Package SCM Request
=======================
Package Name: wmSun
Short Description: Rise/Set time of Sun in a dockapp
Owners: mariobl
Branches: f15

Git done (by process-git-requests).

Ankur, in the future, please assign reviews you are working on to yourself. 
Thank you!

(In reply to comment #11)
> Git done (by process-git-requests).
> 
> Ankur, in the future, please assign reviews you are working on to yourself. 
> Thank you!

OH!! I somehow forgot. Won't happen again.

wmSun-1.03-4.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/wmSun-1.03-4.fc15

wmSun-1.03-4.fc15 has been pushed to the Fedora 15 testing repository.

wmSun-1.03-4.fc15 has been pushed to the Fedora 15 stable repository.