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

[bugzilla at redhat.com]

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

--- Comment #7 from Christoph Wickert <cwickert at fedoraproject.org> 2010-04-24 06:42:16 EDT ---
Let's see what we have:

OK - MUST: all build dependencies are listed in BuildRequires
OK - MUST: owns all directories that it create
OK - MUST: %{name}.desktop file is correct
OK - MUST: %doc is complete
OK - SHOULD: latest stable version
OK - SHOULD: timestamp of Source0 matches upstream
OK - SHOULD: timestamps are preserved during install
FIX - rpmlint:
    - The spelling errors can be ignored
    - executable-marked-as-config-file can be ignored too, see above
    - patch-not-applied should be fixed. You are applying the patch with 
        patch -p0 < %{PATCH0}
      but rpm already has a marco for that called %patch. Use
        %patch0 
      or 
        %patch0 -p0
      or even better
         %patch0 -p0 -b .orig
      I recommend doing backups if you apply several patches. Use the name of
      the patch as backup extension.

FIX - MUST: the package is not named according to the Package Naming
Guidelines. The disttag is missing, see
http://fedoraproject.org/wiki/Packaging:DistTag
(Sorry I didn't catch this before)


(In reply to comment #5)
> > - %{_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.
> 
> OK, done (I've learned a lot whit this comment: Thanks!)

Another way of fixing this would have been do use %dir %{_datadir}/%{name}/ and
then list the filed inside. But if the package owns everything, we just use
%{_datadir}/%{name}/. The / in the end is not strictly necessary, it is just
for humans to understand we are dealing with a dir instead of a file.


> I need that you read
> carefully the menu list for possible errors or different ideas/suggestions;
> some apps I removed and some I replaced. 

Everything looks fine, than you for looking at this so carefully.


> > - How about including the stuff from the contrib folder in doc? If you include
> > a script in doc, make sure it is not executable.
> 
> Here I didn't understand very well. Why contrib applications in doc section?

The contrib directory contains two scripts that are not needed for pekwm to
operate, nevertheless they are part of the source and they are useful. Thus it
might be a good idea to ship them. If people are really low on disk space can
install pekwm with --excludedocs and don't get the scripts.

As the scripts require manual configuration, we cannot install them directly.
People will be disappointed/confused because they don't work out of the box. By
shipping them as doc we make sure that people actually look at them and the
README file before the use them.

When we ship the scripts we don't want to add additional dependencies (e.g. one
script needs zenity). Docs must never be executable because rpm will add a
dependency on sh or whatever interpreter is needed for the scripts (in this
case perl).


Germán, please fix the %patch thing and the disttag and read
http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group
so I can approve this package and sponsor you. If you are doing a pre-review,
please CC me.

-- 
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.