Bug 576685

Package don't seems very complex, nice start. 

You have been reading this I guess:

 http://fedoraproject.org/wiki/PackageMaintainers/Join

Please provide a scratch build of the package.

Then this might be of interest:

 http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

Hi, thanks for your comment! Yes, I have read that page.

I made a scratch build using Koji. You can find the output here:

http://koji.fedoraproject.org/koji/taskinfo?taskID=2074106

Regards,
German.

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.

P.S.: I'm also willing to sponsor you. Please read 
http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group
for more info.

Hi Christoph:

I'm very sorry for the delay in responding, I've been very busy at work but I'm back again to work in Fedora rpms. Please find my updated files here:

Spec URL: http://sites.google.com/site/gracca/pekwm.spec
SRPM URL: http://sites.google.com/site/gracca/pekwm-0.1.12-1.src.rpm

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

That's great! Now I'm going to try to answer your comments.

> Some comments:

> FIX - not latest stable version: Please update the package to 0.1.12

OK, done (updated to version 0.1.12).

> - BuildRequrires: During configure you see some checks that are not fulfilled:
...
> This means you need add libICE-devel, libXpm-devel and libjpeg-devel

OK, done (added to BuildRequires).

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

OK, done (removed).

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

OK, done (changed to XSession type).
 
> - Add INSTALL='install -p' to the make install ... line to preserve timestamps

OK, done (added "install -p").

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

OK, done (downloaded with timestamping turned on in wget).

> - %{_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!)

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

OK, done (tagged as config file).

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

OK, done (both added).

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

OK, I've included a patch to take care of this. However, I need that you read carefully the menu list for possible errors or different ideas/suggestions; some apps I removed and some I replaced. A summary here:

vi: removed (vi is a symlink to vim)
MPlayer: substitued gmplayer by gnome-mplayer
Ogle: substitued by Totem
XCalc: substitued by gcalctool
XMan: removed
GGv: removed
Gkrellm2: removed
ROX Filer: removed
Mozilla: removed
Mozilla Firefox: removed
Pine: substitued by Alpine
Mozilla Thunderbird: removed
Gaim: substitued by Pidgin
StarOffice: removed

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

Once again, thanks very much!
Germán.

(In reply to comment #4)
> P.S.: I'm also willing to sponsor you. Please read 
> http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group
> for more info.    

What a nice comment! I would like to be sponsored!

I've forgotten to include rpmlint outputs. Here they are.

[german@skytux rpmbuild]$ rpmlint SPECS/pekwm.spec 
SPECS/pekwm.spec: W: patch-not-applied Patch0: %{name}-menu.patch
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

[german@skytux rpmbuild]$ rpmlint SRPMS/pekwm-0.1.12-1.src.rpm 
pekwm.src: W: spelling-error %description -l en_US aewm -> Newman, anew, aerie
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: patch-not-applied Patch0: %{name}-menu.patch
1 packages and 0 specfiles checked; 0 errors, 8 warnings.

[german@skytux rpmbuild]$ rpmlint RPMS/i686/pekwm-0.1.12-1.i686.rpm 
pekwm.i686: W: spelling-error %description -l en_US aewm -> Newman, anew, aerie
pekwm.i686: W: spelling-error %description -l en_US pwm -> wpm, pm, pom
pekwm.i686: W: spelling-error %description -l en_US fluxbox -> flux box, flux-box, fluxing
pekwm.i686: W: spelling-error %description -l en_US xinerama -> Cinerama, mineral, cameraman
pekwm.i686: W: spelling-error %description -l en_US keygrabber -> key grabber, key-grabber, grabber
pekwm.i686: W: spelling-error %description -l en_US keychains -> key chains, key-chains, enchains
pekwm.i686: W: unstripped-binary-or-object /usr/bin/pekwm
pekwm.i686: E: executable-marked-as-config-file /etc/pekwm/start
1 packages and 0 specfiles checked; 1 errors, 7 warnings.

Cheers,
Germán.

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.

(In reply to comment #7)

Hi Christoph, thanks once again for your careful review. Please find the updated files here:

Spec URL: http://sites.google.com/site/gracca/pekwm.spec
SRPM URL: http://sites.google.com/site/gracca/pekwm-0.1.12-2.fc12.src.rpm

In response to your comments:

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

OK, patch applied correctly and with backup option.
 
> 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)

OK, fixed. I used %{?dist} tag.

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

Now I see your point. I've included the whole contrib folder to the %doc section. Before doing this, I removed exec permissions to perl scripts. But I'm not sure if the way I did this is the correct one.
 
> 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.

I've read it yet, but I didn't realize of adding you to my pre-reviews. Here are the bugs I've participated:

566598 Review Request: cover-thumbnailer - Display music cover and more in nautilus

576023 Review Request: libwebcam - user-space configuration of the uvcvideo driver

577975 Review Request: kde-plasma-daisy - A versatile application launcher

578269 Review Request: xgospel - An X11 client for Internet Go Server

581216 Review Request: texworks - A simple IDE for authoring TeX documents

581509 Review Request: yacas - easy to use, general purpose Computer Algebra System

584111 Review Request: cmatrix - Simulate the display from "The Matrix"


Hope this package be approved soon!

Cheers,
Germán

Sorry it took so long.

I have no problems sponsoring you, you have done well here and in the other reviews. However there are still some problems with the package:

drwxr-xr-x  /usr/share/doc/pekwm-0.1.12/contrib
-rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/Makefile
-rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/Makefile.am
-rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/Makefile.in
drwxr-xr-x  /usr/share/doc/pekwm-0.1.12/contrib/lobo
-rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/Makefile
-rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/Makefile.am
-rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/Makefile.in
-rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/README
-rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/check.png
-rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/pekwm_autoprop.pl
-rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/pekwm_menu_config.pl
-rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/pekwm_menu_config.pl.vars
-rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/uncheck.png

As you can see the Makefiles end up in the package. Your options:
1. Remove them in %install
2. Install the the files individually and not the whole contrib folder. Then you could also nuke the "lobo" subfolder which is useless as long as it is the only one.

(In reply to comment #9)

Hello Christoph. Please find the updated files here:

Spec URL: http://sites.google.com/site/gracca/pekwm.spec
SRPM URL: http://sites.google.com/site/gracca/pekwm-0.1.12-3.fc12.src.rpm

> Sorry it took so long.

No problem :-)

> I have no problems sponsoring you, you have done well here and in the other
> reviews. However there are still some problems with the package:

Nice to hear this! Thanks!

> drwxr-xr-x  /usr/share/doc/pekwm-0.1.12/contrib
> -rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/Makefile
> -rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/Makefile.am
> -rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/Makefile.in
> drwxr-xr-x  /usr/share/doc/pekwm-0.1.12/contrib/lobo
> -rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/Makefile
> -rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/Makefile.am
> -rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/Makefile.in
> -rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/README
> -rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/check.png
> -rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/pekwm_autoprop.pl
> -rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/pekwm_menu_config.pl
> -rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/pekwm_menu_config.pl.vars
> -rw-r--r--  /usr/share/doc/pekwm-0.1.12/contrib/lobo/uncheck.png
> 
> As you can see the Makefiles end up in the package. Your options:
> 1. Remove them in %install
> 2. Install the the files individually and not the whole contrib folder. Then
> you could also nuke the "lobo" subfolder which is useless as long as it is the
> only one.    

I did the following here. I removed the makefiles from the contrib folder:

find contrib/Makefile* -type f | xargs rm -rf || true
find contrib/lobo/Makefile* -type f | xargs rm -rf || true

and then I rearranged the contents of the contrib folder:

mv contrib/lobo/* contrib/
rm -rf contrib/lobo

Is this made in the right way? Please tell me if I'm doing something wrong or something that is not allowed to.

Here are the outputs of rpmlint:

$ rpmlint SPECS/pekwm.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint SRPMS/pekwm-0.1.12-3.fc12.src.rpm 
pekwm.src: W: spelling-error %description -l en_US aewm -> Newman, anew, aerie
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
1 packages and 0 specfiles checked; 0 errors, 7 warnings.

$ rpmlint RPMS/i686/pekwm-0.1.12-3.fc12.i686.rpm 
pekwm.i686: W: spelling-error %description -l en_US aewm -> Newman, anew, aerie
pekwm.i686: W: spelling-error %description -l en_US pwm -> wpm, pm, pom
pekwm.i686: W: spelling-error %description -l en_US fluxbox -> flux box, flux-box, fluxing
pekwm.i686: W: spelling-error %description -l en_US xinerama -> Cinerama, mineral, cameraman
pekwm.i686: W: spelling-error %description -l en_US keygrabber -> key grabber, key-grabber, grabber
pekwm.i686: W: spelling-error %description -l en_US keychains -> key chains, key-chains, enchains
pekwm.i686: W: unstripped-binary-or-object /usr/bin/pekwm
pekwm.i686: E: executable-marked-as-config-file /etc/pekwm/start
1 packages and 0 specfiles checked; 1 errors, 7 warnings.

Cheers,
German.

(In reply to comment #10)

> Is this made in the right way? Please tell me if I'm doing something wrong or
> something that is not allowed to.

This is what I would have done: At the end of %install, I would add

# Install docs (needed for contrib folder)
mkdir -p %{buildroot}%{_docdir}/%{name}-%{version}/contrib
install -p AUTHORS ChangeLog ChangeLog.aewm++ ChangeLog.until-0.1.6 LICENSE NEWS README \
  %{buildroot}%{_docdir}/%{name}-%{version}
install -p contrib/lobo/README contrib/lobo/check.png contrib/lobo/pekwm_autoprop.pl \
  contrib/lobo/pekwm_menu_config.pl contrib/lobo/pekwm_menu_config.pl.vars \
  contrib/lobo/uncheck.png %{buildroot}%{_docdir}/%{name}-%{version}/contrib

and in %files

%doc %{_docdir}/%{name}-%{version}

But your way is much simpler and works fine too. Nice idea actually. ;)

Congratulations, the package is APPROVED. Good work!

Please follow the instructions from https://fedoraproject.org/wiki/PackageMaintainers/Join#Add_Package_to_CVS_and_Set_Owner and apply for the 'packager' group in the accounts system, so I can sponsor you. If you have problems, don't hesitate to contact me.

(In reply to comment #10)

> pekwm.i686: W: unstripped-binary-or-object /usr/bin/pekwm

Oops, where is this coming from now? I didn't see it in the previous packages and I don't see it in -3 when rebuilding it locally on x86_64. I will look into that tomorrow, please do not import the package yet. Of course you can to the other steps already.

(In reply to comment #11)

> This is what I would have done: At the end of %install, I would add
> 
> # Install docs (needed for contrib folder)
> mkdir -p %{buildroot}%{_docdir}/%{name}-%{version}/contrib
> install -p AUTHORS ChangeLog ChangeLog.aewm++ ChangeLog.until-0.1.6 LICENSE
> NEWS README \
>   %{buildroot}%{_docdir}/%{name}-%{version}
> install -p contrib/lobo/README contrib/lobo/check.png
> contrib/lobo/pekwm_autoprop.pl \
>   contrib/lobo/pekwm_menu_config.pl contrib/lobo/pekwm_menu_config.pl.vars \
>   contrib/lobo/uncheck.png %{buildroot}%{_docdir}/%{name}-%{version}/contrib
> 
> and in %files
> 
> %doc %{_docdir}/%{name}-%{version}
> 
> But your way is much simpler and works fine too. Nice idea actually. ;)

I also thought something similar before doing it this way.

> Congratulations, the package is APPROVED. Good work!

Thank you very much!

(In reply to comment #12)
 
> > pekwm.i686: W: unstripped-binary-or-object /usr/bin/pekwm
> 
> Oops, where is this coming from now? I didn't see it in the previous packages
> and I don't see it in -3 when rebuilding it locally on x86_64. I will look into
> that tomorrow, please do not import the package yet. Of course you can to the
> other steps already.    

Well...I tried a koji build from scratch and it failed! After a long time trying to understand what happened, I realized I was missing the package libSM-devel in BuildRequires. So here are the updated files:

Spec URL: http://sites.google.com/site/gracca/pekwm.spec
SRPM URL: http://sites.google.com/site/gracca/pekwm-0.1.12-4.fc12.src.rpm

After that, the koji build from scratch succeeded:

http://koji.fedoraproject.org/koji/taskinfo?taskID=2178959

and running rpmlint on the packages generated by koji didn't show the 'unstripped-binary-or-object' warning. For example:

$ rpmlint pekwm-0.1.12-4.fc12.x86_64.rpm 
pekwm.x86_64: W: spelling-error %description -l en_US aewm -> Newman, anew, aerie
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
pekwm.x86_64: E: executable-marked-as-config-file /etc/pekwm/start
1 packages and 0 specfiles checked; 1 errors, 6 warnings.

I don't know why this happens. I'll be waiting for your answer.

Cheers,
German.

There must be something wrong with your local builds or your build environment. If I rebuild your package locally I don't get unstripped binaries and koji builds do't have this problem ether. Are you sure you have all necessary packages installed? Do you have the fedora-packager group and the redhat-rpm-config package installed.

As the package is not affected, you can continue with the CVS admin procedure:
http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure

(In reply to comment #15)
> There must be something wrong with your local builds or your build environment.
> If I rebuild your package locally I don't get unstripped binaries and koji
> builds do't have this problem ether. Are you sure you have all necessary
> packages installed? Do you have the fedora-packager group and the
> redhat-rpm-config package installed.
> 
> As the package is not affected, you can continue with the CVS admin procedure:
> http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure    

Yes, I have the fedora-packager group and the package redhat-rpm-config installed in my systems i686 and x86_64, both Fedora 12, and I *always* get unstripped binaries when compiling rpm packages...(???)

I've already applied for the packager group in the FAS. Now I'm going to read about the CVS admin procedure.

Cheers,
German.

New Package CVS Request
=======================
Package Name: pekwm
Short Description: A small and flexible window manager
Owners: skytux
Branches: F-12 F-13
InitialCC:

Christoph: Can you set fedora-review flag to + here?

it seems this package is already in Fedora, but was orphaned. You may want to look at the package for any ideas.

Sorry, my bad. I should have set the flags and I should have noticed that this is an orphaned package.

German, there are indeed some interesting features in the old package, most notably pekwm-xdg-menu.py. Take a look at the old files at
http://cvs.fedoraproject.org/viewvc/rpms/pekwm/devel/
and then lets discuss what is useful and what not.

(In reply to comment #19)
> Sorry, my bad. I should have set the flags and I should have noticed that this
> is an orphaned package.

So this means I don't have to import the package yet?

> German, there are indeed some interesting features in the old package, most
> notably pekwm-xdg-menu.py. Take a look at the old files at
> http://cvs.fedoraproject.org/viewvc/rpms/pekwm/devel/
> and then lets discuss what is useful and what not.    

Ok, I'm going to take a look at the files there.

Cheers,
German.

We should look at the differences to the previous package and discuss what is interesting for us. This process should happen in public for transparency, so bugzilla is a good place. Once we are both fine with the package, you can import it.

(In reply to comment #21)
> We should look at the differences to the previous package and discuss what is
> interesting for us. This process should happen in public for transparency, so
> bugzilla is a good place. Once we are both fine with the package, you can
> import it.    

OK, sure! Let's see:

* The patches for configure and gcc43 are not needed in this version.

* There is a patch for the menu file. The previous packager added an entry, called 'Fedora Menu', for a dynamic menu using a python script called pekwm-xdg-menu.py. I think this dynamic entry could be interesting. I tested it and the only thing I obtain is this:

$ python pekwm-xdg-menu.py
Dynamic {
	 Submenu =  "Applications" {
		 Submenu =  "Accessories" {
			Entry = "AllTray" { Actions = "Exec alltray &" }  
			Entry = "Application Finder" { Actions = "Exec xfce4-appfinder &" }  
			Entry = "Archive Manager" { Actions = "Exec file-roller &" }  
			Entry = "Avant Window Navigator" { Actions = "Exec avant-window-navigator &" }  
			Entry = "CWallpaper" { Actions = "Exec cwallpaper &" }  
			Entry = "CWallpaper Config" { Actions = "Exec cwall-launch &" }  
Traceback (most recent call last):
  File "pekwm-xdg-menu.py", line 74, in <module>
    main(sys.argv[1:])
  File "pekwm-xdg-menu.py", line 70, in main
    parseMenu(menu)
  File "pekwm-xdg-menu.py", line 41, in parseMenu
    parseMenu(entry,depth)
  File "pekwm-xdg-menu.py", line 43, in parseMenu
    checkWm(entry)
  File "pekwm-xdg-menu.py", line 31, in checkWm
    if wm in entry.DesktopEntry.getNotShowIn():
NameError: global name 'wm' is not defined

So, what is the finality of this dynamic menu? There are only 5 or 6 menu entries, but I can't get the other entries because of the error. I don't know python language...it's hard to understand the code, so any help is welcome!

Cheers,
Germán.

CVS Done

pekwm-0.1.12-4.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/pekwm-0.1.12-4.fc13

pekwm-0.1.12-4.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/pekwm-0.1.12-4.fc12

pekwm-0.1.12-4.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update pekwm'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/pekwm-0.1.12-4.fc13

pekwm-0.1.12-4.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update pekwm'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/pekwm-0.1.12-4.fc12

pekwm-0.1.12-4.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

pekwm-0.1.12-4.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Package Change Request
======================
Package Name: pekwm
New Branches: el6
Owners: splinux skytux

I'm authorizing the user splinux to co-maintain this epel branch for pekwm.

Git done (by process-git-requests).