Bug 210087

There are some BR (BuildRequires) that may be missing:

libpng-devel libXinerama-devel libjpeg-devel 

Did you check the link flags in mock?

Why don't you enable pcre?

in files, you should have
%dir %{_datadir}/%{name}
otherwise the things within that dir are listed twice.

There is a dot missing in description.

Optionally you could shorten the summary by removing 'based on the 
aewm++ window manager'.

Are the
           --x-includes=%{_includedir} \
           --x-libraries=%{_libdir} \
really needed?

Could you expand a bit on            --sysconfdir=%{_datadir}
That seems a bit strange...

(In reply to comment #1)
> There are some BR (BuildRequires) that may be missing:
> 
> libpng-devel libXinerama-devel libjpeg-devel 
> 
> Did you check the link flags in mock?
> 
No, how do I check that? What I did was went to the src dir and did a grep for
includes. The one for xinerama came from X11/extensions/Xinerama.h so I did an
rpmquery -f on /usr/include/X11/extensions/Xinerama.h and found it was provided
by xorg-x11-proto-devel-7.0-6 
I guess I just over looked the jpeg and png header files.

> Why don't you enable pcre?
No reason, I guess I should build with all the options I guess?
 
> in files, you should have
> %dir %{_datadir}/%{name}
> otherwise the things within that dir are listed twice.

Ah ok, I wondered why that was happening

> Are the
>            --x-includes=%{_includedir} \
>            --x-libraries=%{_libdir} \
> really needed?

No I guess not, but thats how the fluxbox package is done so I followed suit.
 
> Could you expand a bit on            --sysconfdir=%{_datadir}
> That seems a bit strange...

Yes, before doing this it would put all these files into /etc/pekwm and they
really had no place there. rpmlint was complaining about it, and they seem to be
better placed in /usr/share/pekwm to me. 

(In reply to comment #2)

> No, how do I check that? What I did was went to the src dir and did a grep for
> includes. The one for xinerama came from X11/extensions/Xinerama.h so I did an
> rpmquery -f on /usr/include/X11/extensions/Xinerama.h and found it was provided
> by xorg-x11-proto-devel-7.0-6 
> I guess I just over looked the jpeg and png header files.

There is also the .so linked that are needed:
# rpm -qf /usr/lib/libXinerama.so
libXinerama-devel-1.0.1-2.1

> > Why don't you enable pcre?
> No reason, I guess I should build with all the options I guess?

Indeed. 

> No I guess not, but thats how the fluxbox package is done so I followed suit.

You can leave it if you prefer.

> > Could you expand a bit on            --sysconfdir=%{_datadir}
> > That seems a bit strange...
> 
> Yes, before doing this it would put all these files into /etc/pekwm and they
> really had no place there. rpmlint was complaining about it, and they seem to be
> better placed in /usr/share/pekwm to me. 

I've read a bit of doc, and it seems like the config files
are copied to the home as soon as possible. So it is not that 
important where the defaults are. The scripts would better be 
in /usr/share/pekwm or even in /usr/bin, but the 
config files may be in %_sysconfdir, in case the local sysadm wants to 
tailor the default config. In fluxbox it is in /usr/share, but I 
am not convinced it is right.

The theme is installed in /usr/share allready.

You can easily overwrite what is done by make in menu.in or 
config.in, by doing the sed which sets the path yourself. 

So my proposal is, keep sysconfdir as is, but move the 
scrips to %_bindir, and use sed yourself to regenerate menu from
data/menu.in.

A wild guess is that to put the scripts for example in %_bindir
you could just redefine scriptsdir on the make install command line.

Otherwise scripts use pkill from procps, and also there seems
to be a need for xprop, which is a virtual provides of 
xorg-x11-utils.

The default menu brings in a lot of dependencies. I am not convinced
what should be done there. Have a menu with wrong entries? Trim
it down and add requires? 

Would you feel like modifying fluxbox-xdg-menu, remove the footer
and header, change parseMenu to generate pekwm format, 
and remove the Submenus from Submenu = "Editors" to 
Submenu = "Development", and replace them by an entry like:
Entry = "" { Actions = "Dynamic %{bindir}/pekwm-xdg-menu" }?
This seems to be the best thing to do, currently it doesn't
really integrate with fedora.

(In reply to comment #3)
> 
> So my proposal is, keep sysconfdir as is, but move the 
> scrips to %_bindir, and use sed yourself to regenerate menu from
> data/menu.in.
When you say this do you mean to leave it like I have it now, or remove what I
have now and let this stuff be places in %{_sysconfdir} 

> 
> A wild guess is that to put the scripts for example in %_bindir
> you could just redefine scriptsdir on the make install command line.

This sounds fine to me, Ill see what I can do to get it like that.

 
> Otherwise scripts use pkill from procps, and also there seems
> to be a need for xprop, which is a virtual provides of 
> xorg-x11-utils.

Does there need to be a requires for this or are they pretty much a standard
tool to have on board?

 
> The default menu brings in a lot of dependencies. I am not convinced
> what should be done there. Have a menu with wrong entries? Trim
> it down and add requires? 
> 
> Would you feel like modifying fluxbox-xdg-menu, remove the footer
> and header, change parseMenu to generate pekwm format, 
> and remove the Submenus from Submenu = "Editors" to 
> Submenu = "Development", and replace them by an entry like:
> Entry = "" { Actions = "Dynamic %{bindir}/pekwm-xdg-menu" }?
> This seems to be the best thing to do, currently it doesn't
> really integrate with fedora.

Solved this. :)

(In reply to comment #4)
> (In reply to comment #3)
> > 

> When you say this do you mean to leave it like I have it now, or remove what I
> have now and let this stuff be places in %{_sysconfdir} 

My proposal would be let this stuff be placed in %{_sysconfdir}.
and set scriptsdir=%{_bindir}.
And adjust paths.
 
> > Otherwise scripts use pkill from procps, and also there seems
> > to be a need for xprop, which is a virtual provides of 
> > xorg-x11-utils.
> 
> Does there need to be a requires for this or are they pretty much a standard
> tool to have on board?

They are standard, but I think we shouldn't assume anything,
especially for lightweight window managers, so requires should be 
the right ones.

> Solved this. :)

And that's great :) 

OK, I got another srpm ready today

http://errr.fluxbox-wiki.org/fedora_stuff/pekwm/3/pekwm-0.1.5-3.fc5.src.rpm
http://errr.fluxbox-wiki.org/fedora_stuff/pekwm/3/pekwm.spec

I get one error from rpmlint about the start file in /etc/pekwm being 755
What should I do on that??

The rpmlint error is ignorable. This executable in etc is 
something we want.

I still think that the summary would be better without
'based on the aewm++ window manager', but it is not a blocker.

The dot missing at the end of the %description is a blocker
(although one easy to fix).

Not a blocker, but I would have requires xprop and not xorg-x11-utils.
 
What about the procps Requires?

* rpmlint gives
E: pekwm executable-marked-as-config-file /etc/pekwm/start
which is ignorable.
* free software. License not included, should be asked upstream, 
  especially since in files it is referred to that file.
* name right
* follow guidelines
* spec legible
* build on x86
* buildrequires mostly right, missing pcre-devel
* sane provides:
Provides: config(pekwm) = 0.1.5-3
* files right except for an unowned directory
* source match upstream:
fe3e0d77250d27963991994f83ccb4ea  pekwm-0.1.5.tar.bz2

MUSTFIX:
unowned directory, must add
%dir %{_datadir}/%{name}/themes/

dot at end of %description

BR pcre-devel

SHOULDFIX:
handle properly UTF-8. I won't make it a bloker, but having a
graphical app which don't accept UTF8 accents in menu is not
right in fedora.

(In reply to comment #7)
> The rpmlint error is ignorable. This executable in etc is 
> something we want.
OK
> I still think that the summary would be better without
> 'based on the aewm++ window manager', but it is not a blocker.
Done

> Not a blocker, but I would have requires xprop and not xorg-x11-utils.
Done
  
> What about the procps Requires?
What do you mean here? 

> MUSTFIX:
> unowned directory, must add
> %dir %{_datadir}/%{name}/themes/
Done 

> dot at end of %description
Done 

> BR pcre-devel
Added
 
> SHOULDFIX:
> handle properly UTF-8. I won't make it a bloker, but having a
> graphical app which don't accept UTF8 accents in menu is not
> right in fedora.

I will ask upstream about plans for this.

(In reply to comment #8)
> (In reply to comment #7)

> > What about the procps Requires?
> What do you mean here? 

procps is required because of pkill used in some script.

Currently it is required by initscripts, but this may change,
and with initng I don't know if initscript will be required.

OK All done. 

http://errr.fluxbox-wiki.org/fedora_stuff/pekwm/4/pekwm.spec
http://errr.fluxbox-wiki.org/fedora_stuff/pekwm/4/pekwm-0.1.5-4.src.rpm

All the blocking item have been fixed, so this is

APPROVED

Patrice, I got a message from upstream, how would you feel about me adding this
to the package:

http://adresh.com/pekwm/dev_docs/html/faq/answers.html#FAQ-1546

I think it would be right to do what is said in the FAQ entry 
to have unicode support. 8 bit locales use should be very rare
in fedora.


It would have been even better to switch depending on the 
locale, but it is something that should be done upstream. Maybe 
worth asking upstream, too?

You can add it after importing to cvs.