This service will be undergoing maintenance at 00:00 UTC, 2016-09-28. It is expected to last about 1 hours
Bug 239428 - Review Request: slim - Simple Login Manager
Review Request: slim - Simple Login Manager
Status: CLOSED UPSTREAM
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jochen Schmitt
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-08 08:20 EDT by Anders F Björklund
Modified: 2007-11-30 17:12 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-10-14 14:25:54 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
jochen: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Anders F Björklund 2007-05-08 08:20:35 EDT
Spec URL: http://www.algonet.se/~afb/xfce/slim.spec
SRPM URL: http://www.algonet.se/~afb/xfce/slim-1.2.6-1.fc7.src.rpm
Description:
SLiM (Simple Login Manager) is a graphical login manager for X11.
It aims to be simple, fast and independent from the various
desktop environments.
Screenshot: http://www.algonet.se/~afb/xfce/slim-fedora-login.jpg

This is my first Fedora Extras package, sponsor needed.
Comment 1 Jochen Schmitt 2007-05-08 11:44:21 EDT
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)




Comment 2 Anders F Björklund 2007-05-08 11:50:57 EDT
(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...
Comment 3 Jochen Schmitt 2007-05-08 12:25:09 EDT
It may be nice to use a direct link in the Source0 stanza.
Comment 4 Anders F Björklund 2007-05-08 12:28:39 EDT
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
Comment 5 manuel wolfshant 2007-05-08 15:47:43 EDT
A1: each time you modify the spec, the release should be incremented and the
links to src.rpm and spec reposted here.
Comment 6 Anders F Björklund 2007-05-09 16:51:15 EDT
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
Comment 7 Anders F Björklund 2007-05-13 18:26:23 EDT
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".
Comment 8 Jochen Schmitt 2007-05-14 11:15:41 EDT
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.
Comment 9 Anders F Björklund 2007-05-14 11:25:01 EDT
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
Comment 10 Anders F Björklund 2007-05-14 11:29:48 EDT
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 ?
Comment 11 Patrice Dumas 2007-05-14 16:48:09 EDT
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.
Comment 12 Anders F Björklund 2007-05-21 09:49:21 EDT
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...
Comment 13 Jochen Schmitt 2007-05-21 14:15:24 EDT
No, We diskussed about the Fedora specific customization of the package.
Comment 14 Anders F Björklund 2007-05-21 14:19:35 EDT
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.
Comment 15 Anders F Björklund 2007-05-21 14:23:58 EDT
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)
Comment 16 Patrice Dumas 2007-05-21 15:23:23 EDT
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.
Comment 17 Anders F Björklund 2007-05-22 04:32:14 EDT
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
Comment 18 Jochen Schmitt 2007-05-22 10:29:59 EDT
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 ***


Comment 19 Jochen Schmitt 2007-05-22 10:39:17 EDT
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 ***
Comment 20 Jochen Schmitt 2007-05-22 10:56:33 EDT
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
Comment 21 Anders F Björklund 2007-05-22 11:38:06 EDT
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
Comment 22 Jochen Schmitt 2007-05-23 10:58:26 EDT
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
Comment 23 Jochen Schmitt 2007-06-11 14:21:09 EDT
Please put a CVS-Admin request and close this bug after you have built this
package in koji.
Comment 24 Patrice Dumas 2007-06-17 03:05:01 EDT
(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
Comment 25 Patrice Dumas 2007-06-17 03:23:29 EDT
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
Comment 26 Anders F Björklund 2007-06-17 04:16:46 EDT
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...
Comment 27 Patrice Dumas 2007-06-17 05:11:25 EDT
(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.
Comment 28 Anders F Björklund 2007-06-17 05:25:54 EDT
(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 ?
Comment 29 Patrice Dumas 2007-06-17 06:32:30 EDT
(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.
Comment 30 Patrice Dumas 2007-06-21 14:23:21 EDT
You can apply to sponsorship.
Comment 31 Jochen Schmitt 2007-06-21 14:38:36 EDT
I don't see any CVS-Admin request, so I want to ask, was is the issue.

I want to have a closed bug.
Comment 32 Patrice Dumas 2007-06-21 16:11:25 EDT
You should be sponsored now. Removing blocker.
Comment 33 Jochen Schmitt 2007-07-02 12:16:37 EDT
Ping Andres,

Please put a CVS Admin request on this bug.
Comment 34 Anders F Björklund 2007-07-02 13:45:58 EDT
New Package CVS Request
=======================
Package Name: slim
Short Description: Simple Login Manager
Owners: afb@users.sourceforge.net
Branches: F-7
InitialCC: 
Comment 35 Jochen Schmitt 2007-07-16 11:01:25 EDT
It will be nice, if you can import your package into the cvs and initiate the
build process.
Comment 36 Anders F Björklund 2007-07-17 06:05:26 EDT
Seems to have built now, once I finally completed the setup process.
Thanks to everybody that helped, and hope it's now working somewhat...
Comment 37 Jochen Schmitt 2007-07-26 12:00:58 EDT
Question: Did you start the update process in bodthi?
Comment 38 Nikolay Vladimirov 2007-08-02 07:08:09 EDT
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? 
Comment 39 Nikolay Vladimirov 2007-08-04 14:34:13 EDT
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)


Comment 40 Nikolay Vladimirov 2007-08-04 19:25:49 EDT
slim-update_slim_wmlist requires perl-File-DesktopEntry
Comment 41 Gilboa Davara 2007-08-06 08:38:34 EDT
Are you planning on making it available in EPEL?

- Gilboa
Comment 42 Anders F Björklund 2007-08-06 12:37:38 EDT
(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
Comment 43 Anders F Björklund 2007-08-06 12:39:34 EDT
(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)
Comment 44 Anders F Björklund 2007-08-06 17:06:28 EDT
(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)
Comment 45 Jochen Schmitt 2007-08-14 14:40:43 EDT
Your package is pending on bodhi, You have to push it to update-texting explicitly.
Comment 46 Anders F Björklund 2007-08-14 14:58:28 EDT
Ah, thanks. Should build 1.2.6-6 and 1.3.0-1 (rawhide) as well..
Comment 47 Patrice Dumas 2007-10-04 04:10:46 EDT
This bug should certainly be closed. It would be nice to have
consolekit support, though. Is it planned upstream?

Note You need to log in before you can comment on or make changes to this bug.