[Bug 576685] Review Request: pekwm - A small and flexible window manager

Fri Apr 9 17:06:58 UTC 2010

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=576685

Christoph Wickert <cwickert at fedoraproject.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |cwickert at fedoraproject.org
         AssignedTo|nobody at fedoraproject.org    |cwickert at fedoraproject.org
               Flag|                            |fedora-review?

--- Comment #3 from Christoph Wickert <cwickert at fedoraproject.org> 2010-04-09 13:06:52 EDT ---
Hi Germán,

I have been packaging this today too before I found this review. Our specs are
looking nearly the same which is a good sign.

Some comments:

OK - MUST: 
rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/pekwm-*
pekwm.src: W: spelling-error %description -l en_US aewm -> Newman, anew, Aesop
pekwm.src: W: spelling-error %description -l en_US pwm -> wpm, pm, pom
pekwm.src: W: spelling-error %description -l en_US fluxbox -> flux box,
flux-box, fluxing
pekwm.src: W: spelling-error %description -l en_US autoproperties -> auto
properties, auto-properties, properties
pekwm.src: W: spelling-error %description -l en_US xinerama -> Cinerama,
mineral, cameraman
pekwm.src: W: spelling-error %description -l en_US keygrabber -> key grabber,
key-grabber, grabber
pekwm.src: W: spelling-error %description -l en_US keychains -> key chains,
key-chains, enchains
pekwm.src: W: no-buildroot-tag
pekwm.x86_64: W: spelling-error %description -l en_US aewm -> Newman, anew,
Aesop
pekwm.x86_64: W: spelling-error %description -l en_US pwm -> wpm, pm, pom
pekwm.x86_64: W: spelling-error %description -l en_US fluxbox -> flux box,
flux-box, fluxing
pekwm.x86_64: W: spelling-error %description -l en_US xinerama -> Cinerama,
mineral, cameraman
pekwm.x86_64: W: spelling-error %description -l en_US keygrabber -> key
grabber, key-grabber, grabber
pekwm.x86_64: W: spelling-error %description -l en_US keychains -> key chains,
key-chains, enchains
3 packages and 0 specfiles checked; 0 errors, 14 warnings.

The spelling errors can be ignored. The BuildRoot tag is not strictly required
any longer, but I would add it so the spec is more general and will build on
more systems such as EPEL. 

OK - MUST: named according to the Package Naming Guidelines
OK - MUST: spec file name matches the base package %{name}
OK - MUST: package meets the Packaging Guidelines
OK - MUST: Fedora approved license and meets the Licensing Guidelines: GPLv2+
OK - MUST: license field in spec file matches the actual license
OK - MUST: license file included in %doc
OK - MUST: spec is in American English
OK - MUST: spec is legible
OK - MUST: sources match the upstream source by MD5
79df6d01c48e6eb1907dcd3a8246410c
OK - MUST: successfully compiles and builds into binary rpms on x86_64
N/A - MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch.
FIX - MUST: not all build dependencies are listed in BuildRequires: Some
optional ones are missing, see below
N/A - MUST: handles locales properly with %find_lang
N/A - MUST: Every binary RPM package (or subpackage) which stores shared
library files (not just symlinks) in any of the dynamic linker's default paths,
must call ldconfig in %post and %postun.
OK - MUST: Package does not bundle copies of system libraries.
N/A - MUST: If the package is designed to be relocatable, the packager must
state this fact in the request for review
FIX - MUST: owns all directories that it creates: %{_datadir}/%{name}/ is not
owned
OK - MUST: no duplicate files in the %files listing
OK - MUST: Permissions on files are set properly, includes %defattr(...)
OK - MUST: package has a %clean section, which contains rm -rf %{buildroot}
OK - MUST: consistently uses macros
OK - MUST: package contains code, or permissable content
N/A - MUST: Large documentation files should go in a -doc subpackage
OK - MUST: Files included as %doc do not affect the runtime of the application
N/A - MUST: Header files must be in a -devel package
N/A - MUST: Static libraries must be in a -static package
N/A - MUST: Packages containing pkgconfig(.pc) files must 'Requires:
pkgconfig'.
N/A - MUST: If a package contains library files with a suffix, then library
files that end in .so must go in a -devel package.
N/A - MUST: devel packages must require the base package using a fully
versioned dependency
OK - MUST: The package does not contain any .la libtool archives.
N/A - MUST: Packages containing GUI applications must include a %{name}.desktop
file, and that file must be properly installed with desktop-file-install in the
%install section.
OK - MUST: package does not own files or directories already owned by other
packages.
OK - MUST: at the beginning of %install, the package runs rm -rf %{buildroot}.
OK - MUST: all filenames valid UTF-8

SHOULD Items:
OK - SHOULD: Source package includes license text(s) as a separate file.
N/A - SHOULD: The description and summary sections in the package spec file
should contain translations for supported Non-English languages, if available.
OK - SHOULD: builds in mock.
OK - SHOULD: compiles and builds into binary rpms on all supported
architectures.
OK - SHOULD: functions as described.
N/A - SHOULD: Scriptlets are used, those scriptlets must be sane.
N/A - SHOULD: Usually, subpackages other than devel should require the base
package using a fully versioned dependency.
N/A - SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg
N/A - SHOULD: no file dependencies outside of /etc, /bin, /sbin, /usr/bin, or
/usr/sbin

Other items:
FIX - not latest stable version: Please update the package to 0.1.12
OK - SourceURL valid
OK - Compiler flags ok
OK - Debuginfo complete

Issues:
- BuildRequrires: During configure you see some checks that are not fulfilled:

checking for IceConnectionNumber in -lICE... no
...
checking wheter to build support XPM images... yes
checking for XpmReadFileToPixmap in -lXpm... no
checking wheter to build support for JPEG images... yes
checking for jpeg_read_header in -ljpeg... no

This results in:
FEATURES:  XShape Xinerama Xft image-png Xrandr menus harbour

while it should probably be:
FEATURES:  XShape Xinerama Xft image-xpm image-jpeg image-png Xrandr menus
harbour

This means you need add libICE-devel, libXpm-devel and libjpeg-devel

- I would prefer having the desktop file as s separate Source1, but this is up
to you

- touch %{buildroot}%{_datadir}/xsessions/%{name}.desktop is not needed

- xsession file should have Type=XSession instead of Type=Application

- Add INSTALL='install -p' to the make install ... line to preserve timestamps,
see https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

- The timestamp of the source tarball should also match upstream, see above
link for how to achieve this.

- %{_datadir}/%{name}/ is not owned, only the files inside. Thus an empty
folder will remain after uninstalling the package. Just drop the * at the end
of that line.

- %{_sysconfdir}/%{name}/start should also be tagged as %config(noreplace). The
warning of rpmlint can be ignored.

- ChangeLog.until-0.1.6 should be included in %doc, maybe also ChangeLog.aewm++

- I think the applications menu should be patched to not include apps that are
not in Fedora, e.g mozilla-firefox is called firefox. At least Pine and
StarOffice should not be in the menu since they are not free software. Use
Alpine instead of Pine. There is a lot of room for improvement, add what you
think makes sense.

- How about including the stuff from the contrib folder in doc? If you include
a script in doc, make sure it is not executable.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.