Bug 239428

God:
+ Naming seems ok.
+ License seems ok.
+ Package contains verbatin copy of the license.


Bad:
- Rpmlint complaints on source rpm.
W: slim strange-permission slim-update_slim_wmlist 0775
- Rpmlint complaints binary rpm
W: slim dangling-relative-symlink /usr/share/slim/themes/default/background.jpg
../../../backgrounds/images/default.jpg
W: slim no-version-in-last-changelog
W: slim conffile-without-noreplace-flag /etc/slim.conf
- Tar ball doesn't maches with upstream tar ball.
1bf891f046014a03236c21ce6cbe455b  ../SOURCES/slim-1.2.6.tar.gz  (RPM)
7a98d588cb6778baefcaa368f3dfa2a4  slim-1.2.6.tar.gz   (Upstream)
- Mockbuild fails with errors:
freetype2/config -I/usr/include/libpng12 -I/usr/include -DPACKAGE=\"slim\"
-DVERSION=\"1.2.6\" -DPKGDATADIR=\"/usr/share/slim\" -DSYSCONFDIR=\"/etc\"
-DHAVE_SHADOW -c main.cpp -o main.o
/usr/bin/g++ -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -I.
-I/usr/X11R6/include -I/usr/include/freetype2 -I/usr/include/freetype2/config
-I/usr/include/libpng12 -I/usr/include -DPACKAGE=\"slim\" -DVERSION=\"1.2.6\"
-DPKGDATADIR=\"/usr/share/slim\" -DSYSCONFDIR=\"/etc\" -DHAVE_SHADOW -c
image.cpp -o image.o
In file included from panel.h:28,
                 from app.h:23,
                 from main.cpp:12:
input.h:17:25: error: X11/Xft/Xft.h: No such file or directory
image.cpp: In member function 'bool Image::Read(const char*)':
image.cpp:71: warning: ignoring return value of 'size_t fread(void*, size_t,
size_t, FILE*)', declared with attribute warn_unused_result
panel.h:57: error: 'XftDraw' has not been declared
panel.h:57: error: 'XftColor' has not been declared
panel.h:57: error: 'XftFont' has not been declared
panel.h:58: error: 'XftChar8' has not been declared
panel.h:59: error: 'XftColor' has not been declared
panel.h:71: error: ISO C++ forbids declaration of 'XftFont' with no type
panel.h:71: error: expected ';' before '*' token
panel.h:72: error: 'XftColor' does not name a type
panel.h:73: error: 'XftColor' does not name a type
panel.h:74: error: 'XftColor' does not name a type
panel.h:75: error: 'XftColor' does not name a type
panel.h:76: error: ISO C++ forbids declaration of 'XftFont' with no type
panel.h:76: error: expected ';' before '*' token
panel.h:77: error: 'XftColor' does not name a type
panel.h:78: error: ISO C++ forbids declaration of 'XftFont' with no type
panel.h:78: error: expected ';' before '*' token
panel.h:79: error: ISO C++ forbids declaration of 'XftFont' with no type
panel.h:79: error: expected ';' before '*' token
panel.h:80: error: 'XftColor' does not name a type
panel.h:81: error: 'XftColor' does not name a type
panel.h:82: error: ISO C++ forbids declaration of 'XftFont' with no type
panel.h:82: error: expected ';' before '*' token
panel.h:83: error: 'XftColor' does not name a type
panel.h:84: error: 'XftColor' does not name a type
make: *** [main.o] Error 1
make: *** Waiting for unfinished jobs....
error: Bad exit status from /var/tmp/rpm-tmp.95810 (%build)

(In reply to comment #1)
> - Tar ball doesn't maches with upstream tar ball.
> 1bf891f046014a03236c21ce6cbe455b  ../SOURCES/slim-1.2.6.tar.gz  (RPM)
> 7a98d588cb6778baefcaa368f3dfa2a4  slim-1.2.6.tar.gz   (Upstream)

Upstream URL is a redirect HTML page, will see if I can get a direct link...

It may be nice to use a direct link in the Source0 stanza.

Fixed the Source0 URL and added libXft-devel, installing "mock" to test...

Q1: Do I bump the release to 2 now, or will it still be at 1 until done ?

Q2: Regarding the dangling symlink to the default background image,
do we want it to use the default image as provided by SLiM instead ?
(until a proper new Fedora theme can be made, using the GDM images)
The default image looks like http://slim.berlios.de/images/slim01.jpg

A1: each time you modify the spec, the release should be incremented and the
links to src.rpm and spec reposted here.

Wasn't able to get "mock" running (not enough disk space, I think)
But uploaded the other changes that I made yesterday, as release 2:

Spec URL: http://www.algonet.se/~afb/xfce/slim.spec
SRPM URL: http://www.algonet.se/~afb/xfce/slim-1.2.6-2.fc7.src.rpm

By the way it seems that PAM support is not enabled yet in SLiM,
but might as well include the pam files and other infrastructure ?

http://developer.berlios.de/patch/?func=detailpatch&patch_id=1979&group_id=2663

Spec URL: http://www.algonet.se/~afb/xfce/slim.spec
SRPM URL: http://www.algonet.se/~afb/xfce/slim-1.2.6-3.fc7.src.rpm

Changed the background image from Fedora default back to SLiM default,
added some more build dependencies, added a README and fixed "console".

Why did you changed the background image back to the default?

I think, if this should be a package for Fedora, it should use the Fedora
background image.

1) to avoid the "dangling symlink" in rpmlint
2) to provide proper Fedora themes later on

I "borrowed" the setup from the wdm package where it uses the default desktop
background for the login page as well. But the login themes for GDM/KDM does not
use the desktop background, instead they use a different image of the same theme. 

Besides, Xfce does not use a Fedora icon but the Xfce icon for the menu. And
IceWM does not use the Fedora icon or theme either, it uses a plain IceWM icon.
So I wasn't sure whether it should be a) a straight packaging or b) themed for
Fedora

http://www.algonet.se/~afb/xfce/wdm-fedora-login.jpg
http://www.algonet.se/~afb/xfce/gdm-fedora-login.jpg
http://www.algonet.se/~afb/xfce/kdm-fedora-login.jpg

Easy enough to change the theme back with a release 4,
I made some typos in the README file anyway. It would
be nice to know whether it worked with mock, though ?

You should have a look at
Bug 228110

I personally think that it is better to use a background from
the fedora artwork which nicely changes between releases, but
I don't think it is very important.

Is this bug waiting for new features like PAM or
ConsoleKit to be added to SLiM upstream properly ?

Or is it just waiting on default panel theming,
because that's pretty trivial to change back...

No, We diskussed about the Fedora specific customization of the package.

I haven't had the opportunity to do new Fedora themes for Slim (based on the gdm
originals), so either we go with the desktop background or the upstream original.

I didn't get mock installed, so if you could test it again it'd be appreciated.

Q: Should we remove the PAM files, until upstream releases pam support properly
? (currently it's only available as an experimental patch and a subversion branch)

If the pam support is what will be upstream later I think there is no
problem in shipping it, even though it is experimental. It is more
or less mandatory for integration in fedora. Moreover, I expect the 
slim users not to be average users. If the overall design may not
be included, then I think it is better not to ship it and wait
a bit.

Spec URL: http://www.algonet.se/~afb/xfce/slim.spec
SRPM URL: http://www.algonet.se/~afb/xfce/slim-1.2.6-4.fc7.src.rpm

- changed background back to the Fedora default desktop again
- left the PAM files in (unused), should they be useful later

Screenshot: http://www.algonet.se/~afb/xfce/slim-fedora-login.jpg

Except for "missing features" like themes and buttons and PAM
and ConsoleKit, I don't think there are any more RPM problems.
Going back to "GDM Lite" for use with Xfce myself, I think...
Like http://www.algonet.se/~afb/xfce/gdmlite-fedora-login.png

Good:
+ Naming seems ok.
+ Rpmlint quite on source rpm.
+ Local build works fine.
+ License seems ok.
+ Mock build works fine.
+ Local install 
+ Package contains verbatin copy of the license
+ Rpmlint quite on installed package.


Bad:
- Rpmlint complaints on binary package:
rpmlint slim-1.2.6-4.x86_64.rpm
W: slim dangling-relative-symlink /usr/share/slim/themes/default/background.jpg
../../../backgrounds/images/default.jpg

*** May be continue on next comment ***

Continuation of comment #16:

God:
+ Local install and uninstall works fine.

Suggested Enhancedments:

* After Login, the background image should stay active like on kdm.
* On KDE I have got an error message about an none starting sound system.

*** APPROVED ***

Unfortunately I have found a important error in your package:

Please make sure, that the directory %{_datadir}/slim is owned by your package.

Best Regards:

Jochen Schmitt

Spec URL: http://www.algonet.se/~afb/xfce/slim.spec
SRPM URL: http://www.algonet.se/~afb/xfce/slim-1.2.6-5.fc7.src.rpm

Comment #20: now fixed, in release 5
Comment #18: unfortunate side effect

Look nice.

As a hint, if a entry in the file stanza ends with a slash, the directory will
be owned to the package and all files and subdirectories will be recursivly
included in the package.

Best Regards:

Jochen Schmitt

Please put a CVS-Admin request and close this bug after you have built this
package in koji.

(In reply to comment #22)

> As a hint, if a entry in the file stanza ends with a slash, the directory will
> be owned to the package and all files and subdirectories will be recursivly
> included in the package.

It is not exactly that, the slash is not needed in order to
have the directory and  all files and subdirectories be
recursively included in the package. However it is, in my
opinion, better to add a slash when a directory is owned,
to show visually in the spec file that it is indeed a directory.

To have a directory owned, without owning the files are 
subdirectories, one has to use
%dir /some/dir

It seems to me that using xinit config files when loggued through
slim is wrong. The current practice seems to me to be that xinit
is not used when login through a display manager, when login 
through a display manager something like Xsession is used, that
looks into .xsession and not .xinitrc. Moreover it seems to me
that the 'generic' Xsession that is used both for kdm and gdm
(and wdm, but I am biased here :-) should be used, instead of the
one from xdm. This should amounts to adding
Requires:   xorg-x11-xinit

and modify /etc/slim.conf such that the login_cmd is along:
login_cmd           exec /bin/bash -login /etc/X11/xinit/Xsession %session

To reuse the previously used window manager you could do something 
similar with what I did with wdm, by using a wrapper around
/etc/X11/xinit/Xsession, /etc/wdm/Xsession in wdm that stores the
previously chosen window manager in $HOME/.wm_style and reuse it
when calling /etc/X11/xinit/Xsession.


2 other comments:
* with pam support missing, the device special files are not
  owned by the user loggued in, because pam_console.so is not
  used. I guess there may be other issues, but this one is a
  very problematic one.
* even if this was solved, the user won't have the right to
  mount automountable device or do anything else that goes
  through hal, because of a lack of consolekit handling. 
  However this is also the case for wdm and xdm.

Because of the pam issue it isn't that clear to me that this
package should go in a stable release, and if it should it should
deserve a comment somewhere, an obvious place being the README.fedora

In this case it was simply a matter of a missing "parent directory", in that it
only owned the themes directory and not the otherwise empry %{_datadir}/slim...

It's clear that there are both features missing (such as PAM) and ugly hacks
used (such my own for xinitrc) in this package, but once has to start somewhere.


But maybe wait with the package until upstream has their pam support integrated
? (at the moment I haven't got a Fedora Account or the build system setup either)

Using GDM meanwhile: http://www.algonet.se/~afb/xfce/gdmlite-fedora-login.png
If anyone else wants to take the package and fix the issues, go right ahead...

(In reply to comment #26)
> In this case it was simply a matter of a missing "parent directory", in that it
> only owned the themes directory and not the otherwise empry %{_datadir}/slim...

I don't really understand this comment. slim rightly owns the 
directories...

> It's clear that there are both features missing (such as PAM) and ugly hacks
> used (such my own for xinitrc) in this package, but once has to start somewhere.
 
Sure.

What do you think about my proposal about using Xsession instead 
of xinitrc?
 
> But maybe wait with the package until upstream has their pam support integrated

My opinion is that it is right for slim to be in fedora devel
(http://fedoraproject.org/wiki/Releases/Rawhide),
but for stable releases (Fedora 7, Fedora 6) it seems to me that
we should wait for pam support. As I also said you could also
go for stable release branches, but in that case a comment
in README.fedora should state that after login the user won't
have access to most hardware. Given what flows on the fedora-devel
list it seems clear to me that many fedora contributors would find
that having a display manager without pam support in fedora stable 
releases is bad.

> ? (at the moment I haven't got a Fedora Account or the build system setup either)

Do you mean that you aren't sponsored?
Did you have a look at:
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

> Using GDM meanwhile: http://www.algonet.se/~afb/xfce/gdmlite-fedora-login.png
> If anyone else wants to take the package and fix the issues, go right ahead...

Clearly this is an upstream task.

(In reply to comment #27)

> > In this case it was simply a matter of a missing "parent directory", in that it
> > only owned the themes directory and not the otherwise empry %{_datadir}/slim...
> 
> I don't really understand this comment. slim rightly owns the 
> directories...

What I meant was that it was a simple packaging ommision, and nothing regarding
how directories or slashes are treated by RPM. Added the parent, all's well.

> What do you think about my proposal about using Xsession instead 
> of xinitrc?

Sounds good. (haven't tried it yet)

> My opinion is that it is right for slim to be in fedora devel
> (http://fedoraproject.org/wiki/Releases/Rawhide),
> but for stable releases (Fedora 7, Fedora 6) it seems to me that
> we should wait for pam support. As I also said you could also
> go for stable release branches, but in that case a comment
> in README.fedora should state that after login the user won't
> have access to most hardware. Given what flows on the fedora-devel
> list it seems clear to me that many fedora contributors would find
> that having a display manager without pam support in fedora stable 
> releases is bad.

Rawhide it is then.

> > ? (at the moment I haven't got a Fedora Account or the build system setup
either)
> 
> Do you mean that you aren't sponsored?
> Did you have a look at:
> http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

I think so ? Package is reviewed/approved (Comment #19), so that would probably
be next ?

(In reply to comment #28)

> Rawhide it is then.

If the pam support doesn't make its way before fedora 8 release, you'll
have to make sure that it isn't tagged as a fedora 8 package, since
the devel (rawhide) packages become automatically part of the next fedora
release, unless something is done to avoid it.

> I think so ? Package is reviewed/approved (Comment #19), so that would probably
> be next ?

Looks like Jochen is not a sponsor (correct me, Jochen if I am wrong), 
but I am. You seem to understand the guidelines enough, I researched a 
bit on the net, on redhat bugzilla about you, you have a rather good 
record, so there is no issue about competence. The main issue regarding
sponsoring (in my opinion) is to avoid sponsoring people that lose rapidly
interest in fedora and leave the project with orphaned packages behind.
I think that it shouldn't be the case with you, so I am ready to sponsor 
you.

I'd also appreciate being co-maintainer for this package if you are
ok, since it shares some similarities with wdm.

In the meantime I added the FE-NEEDSPONSOR blocker.

You can apply to sponsorship.

I don't see any CVS-Admin request, so I want to ask, was is the issue.

I want to have a closed bug.

You should be sponsored now. Removing blocker.

Ping Andres,

Please put a CVS Admin request on this bug.

New Package CVS Request
=======================
Package Name: slim
Short Description: Simple Login Manager
Owners: afb.net
Branches: F-7
InitialCC: 

It will be nice, if you can import your package into the cvs and initiate the
build process.

Seems to have built now, once I finally completed the setup process.
Thanks to everybody that helped, and hope it's now working somewhat...

Question: Did you start the update process in bodthi?

There is 1.3.0 release:

>1.3.0 - 2006.07.14
>    * Added PAM support by Martin Parm

So the slim is ready for the stable fedora releases? 

I made something like a FedoraFlyingHigh theme for SLiM.

The theme: http://turki.fedorapeople.org/slim.theme
The panel: http://turki.fedorapeople.org/panel.png
In action: http://turki.fedorapeople.org/slim.png

I used gdm's artwork to put together the panel. Any comments and suggestions are
welcome. 
(NOTE: this is my first slim theme ever)

slim-update_slim_wmlist requires perl-File-DesktopEntry

Are you planning on making it available in EPEL?

- Gilboa

(In reply to comment #38)
> There is 1.3.0 release:
> 
> >1.3.0 - 2006.07.14
> >    * Added PAM support by Martin Parm
> 
> So the slim is ready for the stable fedora releases? 

I'll try to roll a 1.3.0 version as soon as I get back in track after summer

(In reply to comment #40)
> slim-update_slim_wmlist requires perl-File-DesktopEntry

That's OK, cause it already has perl(File::DesktopEntry) ?

So I don't see any need to add the package explicitly.
(BTW it is being used to parse the available wm list)

(In reply to comment #37)
> Question: Did you start the update process in bodthi?

I have now... (after doing my homework on CVS/Koji/Bodhi)

Your package is pending on bodhi, You have to push it to update-texting explicitly.

Ah, thanks. Should build 1.2.6-6 and 1.3.0-1 (rawhide) as well..

This bug should certainly be closed. It would be nice to have
consolekit support, though. Is it planned upstream?