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@xxxxxxxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |cwickert@xxxxxxxxxxxxxxxxx AssignedTo|nobody@xxxxxxxxxxxxxxxxx |cwickert@xxxxxxxxxxxxxxxxx Flag| |fedora-review? --- Comment #3 from Christoph Wickert <cwickert@xxxxxxxxxxxxxxxxx> 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review