Bug 188574 - Review Request: rss-glx -- Really Slick Screensavers
Review Request: rss-glx -- Really Slick Screensavers
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
http://rss-glx.sourceforge.net/
:
: 90133 (view as bug list)
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-04-11 08:33 EDT by Nils Philippsen
Modified: 2007-11-30 17:11 EST (History)
11 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-06-02 06:37:31 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Some fixes for rss-glx.spec (2.73 KB, patch)
2006-05-29 11:56 EDT, Mamoru TASAKA
no flags Details | Diff

  None (edit)
Description Nils Philippsen 2006-04-11 08:33:03 EDT
SRPM: http://tiptoe.de/dav/rss-glx-0.8.1-0.2.src.rpm
SPEC: http://tiptoe.de/dav/rss-glx.spec

rss-glx is a collection of cool graphics hacks that can be used in conjunction
with xscreensaver, gnome-screensaver or KDE.

NB: The package does NOT contain the original source. I've included a modified
tarball which doesn't include the matrixview hack as that one includes images
from the movie itself and I find it highly likely that we don't have permission
to include these.

rss-glx is a collection of cool graphics hacks that can be used in conjunction
with xscreensaver, gnome-screensaver or KDE.

NB: The package does NOT contain the original source. I've included a modified
tarball which doesn't include the matrixview hack as that one includes images
from the movie itself and I find it highly likely that we don't have permission
to include these.

NB^2: I've opened a new BZ entry for this since the old one (bug #90133) was
opened against a different Product/Component and was in state ASSIGNED. It seems
to me that people thought someone already had commited to do a review due to that.
Comment 1 Nils Philippsen 2006-04-11 08:36:53 EDT
*** Bug 90133 has been marked as a duplicate of this bug. ***
Comment 2 Jason Tibbitts 2006-05-20 01:26:31 EDT
I figured this could use a little attention, so here are some comments:

Could you provide a script to generate your patched source tarball from upstream's?

Might it be possible to include the matrixview hack but replace the images with,
I don't know, the Fedora logo and pictures of Seth or something?  Or even
nothing; it only uses the compiled-in images if the user doesn't specify a
directory containing images.

The package builds in mock (development, x86_64) but rpmlint finds quite a bit
to complain about.  I'll group the complaints by type:

W: rss-glx no-version-in-last-changelog
W: rss-glx-debuginfo no-version-in-last-changelog
W: rss-glx-gnome-screensaver no-version-in-last-changelog
W: rss-glx-kde no-version-in-last-changelog
W: rss-glx-xscreensaver no-version-in-last-changelog

Many of your changelog entries don't include version information.

E: rss-glx obsolete-not-provided rss_glx

If you obsolete something, you must also provide it.

E: rss-glx zero-length /usr/share/doc/rss-glx-0.8.1/NEWS
E: rss-glx zero-length /usr/share/doc/rss-glx-0.8.1/AUTHORS

No point in packaging these.

W: rss-glx-debuginfo dangling-relative-symlink
/usr/src/debug/rss-glx_0.8.1.p/oglc_src/driver.cpp ../src/driver.cpp
W: rss-glx-debuginfo dangling-relative-symlink
/usr/src/debug/rss-glx_0.8.1.p/other_src/driver.c ../src/driver.cpp
W: rss-glx-debuginfo dangling-relative-symlink
/usr/src/debug/rss-glx_0.8.1.p/reallyslick/cpp_src/driver.cpp ../../src/driver.cpp
W: rss-glx-debuginfo dangling-relative-symlink
/usr/src/debug/rss-glx_0.8.1.p/reallyslick/c_src/driver.c ../../src/driver.cpp

These all seem to be bogus.

W: rss-glx-gnome-screensaver no-documentation
W: rss-glx-kde no-documentation
W: rss-glx-xscreensaver no-documentation

Definitely bogus.

A large number of warnings like this:
W: rss-glx-gnome-screensaver dangling-symlink
/usr/libexec/gnome-screensaver/rss-glx-cyclone /usr/bin/cyclone

rpmlint is smart enough to ignore symlinks to files in required packages when
those symlinks are in -devel packages, but not in this case.  These can all be
ignored.

W: rss-glx-gnome-screensaver non-standard-dir-in-usr libexec

I was recently informed on IRC that /usr/libexec is not discouraged in Fedora,
so I don't understand this warning at all.

I'll do a full review tomorrow.
Comment 3 Nils Philippsen 2006-05-22 05:26:46 EDT
(In reply to comment #2)
> I figured this could use a little attention, so here are some comments:
> 
> Could you provide a script to generate your patched source tarball from
upstream's?

I've included the script, necessary patch file and a README.fedora to describe
these as documentation. I've uploaded these also here:

http://tiptoe.de/dav/README.fedora
http://tiptoe.de/dav/rss-glx-rm-matrixview.sh
http://tiptoe.de/dav/rss-glx-0.8.1-0.8.1.p.diff

The new, script-generated tarball is also at:

http://tiptoe.de/dav/rss-glx_0.8.1.p.tar.bz2

> 
> Might it be possible to include the matrixview hack but replace the images with,
> I don't know, the Fedora logo and pictures of Seth or something?  Or even
> nothing; it only uses the compiled-in images if the user doesn't specify a
> directory containing images.

The format in which these default images are kept is quite obscure... I might
try it at a later point (or hey, I'll accept patches ;-) but for starters we can
do without that hack I think.

> The package builds in mock (development, x86_64) but rpmlint finds quite a bit
> to complain about.  I'll group the complaints by type:
> 
> W: rss-glx no-version-in-last-changelog
> W: rss-glx-debuginfo no-version-in-last-changelog
> W: rss-glx-gnome-screensaver no-version-in-last-changelog
> W: rss-glx-kde no-version-in-last-changelog
> W: rss-glx-xscreensaver no-version-in-last-changelog
> 
> Many of your changelog entries don't include version information.

I'va added version info for the latest and next-to-latest entry, I'm not sure
about the older ones so I'll leave them blank.

> 
> E: rss-glx obsolete-not-provided rss_glx
> 
> If you obsolete something, you must also provide it.

I don't obsolete it anymore as it was only for an old version I never really
distributed.

> E: rss-glx zero-length /usr/share/doc/rss-glx-0.8.1/NEWS
> E: rss-glx zero-length /usr/share/doc/rss-glx-0.8.1/AUTHORS
> 
> No point in packaging these.

Removed.

> 
> W: rss-glx-debuginfo dangling-relative-symlink
> /usr/src/debug/rss-glx_0.8.1.p/oglc_src/driver.cpp ../src/driver.cpp
> W: rss-glx-debuginfo dangling-relative-symlink
> /usr/src/debug/rss-glx_0.8.1.p/other_src/driver.c ../src/driver.cpp
> W: rss-glx-debuginfo dangling-relative-symlink
> /usr/src/debug/rss-glx_0.8.1.p/reallyslick/cpp_src/driver.cpp ../../src/driver.cpp
> W: rss-glx-debuginfo dangling-relative-symlink
> /usr/src/debug/rss-glx_0.8.1.p/reallyslick/c_src/driver.c ../../src/driver.cpp
> 
> These all seem to be bogus.
> 
> W: rss-glx-gnome-screensaver no-documentation
> W: rss-glx-kde no-documentation
> W: rss-glx-xscreensaver no-documentation
> 
> Definitely bogus.
> 
> A large number of warnings like this:
> W: rss-glx-gnome-screensaver dangling-symlink
> /usr/libexec/gnome-screensaver/rss-glx-cyclone /usr/bin/cyclone
> 
> rpmlint is smart enough to ignore symlinks to files in required packages when
> those symlinks are in -devel packages, but not in this case.  These can all be
> ignored.
> 
> W: rss-glx-gnome-screensaver non-standard-dir-in-usr libexec
> 
> I was recently informed on IRC that /usr/libexec is not discouraged in Fedora,
> so I don't understand this warning at all.
> 
> I'll do a full review tomorrow.

Thanks.

I've uploaded the new spec file as well, new SRPM is uploading at the moment,
both will be at:

http://tiptoe.de/dav/rss-glx.spec
http://tiptoe.de/dav/rss-glx-0.8.1-0.3.src.rpm
Comment 4 Nils Philippsen 2006-05-22 05:31:09 EDT
Hmm, that generated tarball didn't build here. Stay tuned, I'll probably have a
bug in the generating script or in the patch.
Comment 5 Nils Philippsen 2006-05-22 05:42:31 EDT
The diff file didn't contain changes to one Makefile.am (ouch). Fixed versions
of the diff file, the generated tarball and SRPM are uploading right now.
Comment 6 Jason Tibbitts 2006-05-23 00:39:22 EDT
Thanks; your method for generating the fixed tarball is quite nice.

You might consider being a bit more descriptive in %description.  Perhaps
something like:

A port of the Really Slick Screensavers to GLX.  Provides several visually
impressive and graphically intensive screensavers.

Note that this package contains only the display hacks themselves; you will need
to install the appropriate subpackage for your desktop environment in order to
use them as screensavers.

(Or whatever; I'm making this up on the spot.  The point is that people won't
understand what is meant by "contains only the hacks themselves".)

The permissions on rss-glx-rm-matrixview.sh are 0775, which is a bit odd (and
rpmlint complains about it).  Executable documentation is generally frowned upon
and rpmlint also complains about it (because your documentation pulls in an
additional /bin/bash dependency).  I would recommend just installing it 0644 and
leave it at that.

Other rpmlint warnings are bogus as previously addressed.

Is there any reason to package rss-glx_install.pl?  This pulls in an odd
perl(strict) dependency but not a plain perl dependency, which looks a bit odd.
 (I know perl provides perl(strict), but perl probably shouldn't be needed at all.)

You use $RPM_BUILD_ROOT in some places and %buildroot in others.  The packaging
guidelines require one or the other to be used consistently.

Review:
* package meets naming and packaging guidelines.
X specfile is properly named and is cleanly written and uses macros consistently.
* dist tag is present.
* license field matches the actual license.
* license is open source-compatible.  License text included in package.
O source files don't match upsteam due to removal of unacceptable content.
* latest version is being packaged.
* BuildRequires are proper.
* package builds in mock (development, x86_64).
X rpmlint complains about the executable rm-matrixview script.
? final provides are fine; requires are a bit odd:
   rss-glx = 0.8.1-0.3.fc6
  -
   /bin/bash
   /usr/bin/env
   libGL.so.1()(64bit)
   libGLU.so.1()(64bit)
   libICE.so.6()(64bit)
   libSM.so.6()(64bit)
   libX11.so.6()(64bit)
   libalut.so.0()(64bit)
   libbz2.so.1()(64bit)
   libc.so.6()(64bit)
   libc.so.6(GLIBC_2.2.5)(64bit)
   libgcc_s.so.1()(64bit)
   libgcc_s.so.1(GCC_3.0)(64bit)
   libm.so.6()(64bit)
   libm.so.6(GLIBC_2.2.5)(64bit)
   libopenal.so.0()(64bit)
   libstdc++.so.6()(64bit)
   libstdc++.so.6(CXXABI_1.3)(64bit)
   libstdc++.so.6(GLIBCXX_3.4)(64bit)
?  perl(strict)
* no shared libraries are present.
* package is not relocatable.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
X file permissions are appropriate (ok except for mode 770 rss-glx-rm-matrixview.sh
* %clean is present
* %check is not present; no test suite upstream.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no libtool .la droppings.
O not a GUI app.  (Well, sort of; special desktop files for each environment are
included, but the hacks aren't indended to be run directly.)
Comment 7 Nils Philippsen 2006-05-23 10:46:56 EDT
(In reply to comment #6)
 
> You might consider being a bit more descriptive in %description.  Perhaps
> something like:
> 
> A port of the Really Slick Screensavers to GLX.  Provides several visually
> impressive and graphically intensive screensavers.
> 
> Note that this package contains only the display hacks themselves; you will need
> to install the appropriate subpackage for your desktop environment in order to
> use them as screensavers.
> 
> (Or whatever; I'm making this up on the spot.  The point is that people won't
> understand what is meant by "contains only the hacks themselves".)

Thanks, I've updated the description blocks accordingly.

> The permissions on rss-glx-rm-matrixview.sh are 0775, which is a bit odd (and
> rpmlint complains about it).  Executable documentation is generally frowned upon
> and rpmlint also complains about it (because your documentation pulls in an
> additional /bin/bash dependency).  I would recommend just installing it 0644 and
> leave it at that.

Done.

> Is there any reason to package rss-glx_install.pl?  This pulls in an odd
> perl(strict) dependency but not a plain perl dependency, which looks a bit odd.
>  (I know perl provides perl(strict), but perl probably shouldn't be needed at
all.)

Not necessary and removed.

> You use $RPM_BUILD_ROOT in some places and %buildroot in others.  The packaging
> guidelines require one or the other to be used consistently.

Fixed.

The new files are at the usual locations with the new SRPM at:

http://tiptoe.de/dav/rss-glx-0.8.1-0.4.src.rpm
Comment 8 Jason Tibbitts 2006-05-23 11:28:09 EDT
Things are looking good now.  One new rpmlint warning (a line in %description is
too long).

However, I went to test this on a couple of i386 FC5 machines under KDE (so
rss-glx and rss-glx-kde are installed); the hacks display when run directly
(they open a small window in the background) but as KDE screensavers there are
some issues:

They show up at the bottom of the list, not filed under "OpenGL Screen Savers".
 Maybe they should have their own place in the hierarchy?  ("Really Slick Screen
Savers"?)  I think you do this by using X-KDE-Category= in the desktop file. 
It's probably also worth using X-KDE-Type=OpenGL as well.  But it looks like
you're just using the desktop files supplied by upstream here; I'm not sure if
it's worth it to hack them up.

The desktop files include "Actions=InWindow;Root" but then go on to define a
Setup action as well.

The setup page for each screensaver doesn't work at all unless you also install
rss-glx-screensaver.  Should this be a dependency of rss-glx-kde, or is
something else wrong?

The screensavers don't actually do anything; the "Test" button causes the
desktop to pause (system monitor and clock stop updating), but nothing is
actually displayed.  This happens on my home machine with binary Nvidia driver
and a machine here at work with a Radeon R200 (stock X driver).  Any ideas?

So at this point the form of the package is fine; you just need to fix that
overlong line in %description.  Unfortunately there's still some debugging to be
done.
Comment 9 Nils Philippsen 2006-05-24 12:06:53 EDT
(In reply to comment #8)
> Things are looking good now.  One new rpmlint warning (a line in %description is
> too long).

Fixed.

> However, I went to test this on a couple of i386 FC5 machines under KDE (so
> rss-glx and rss-glx-kde are installed); the hacks display when run directly
> (they open a small window in the background) but as KDE screensavers there are
> some issues:
> 
> They show up at the bottom of the list, not filed under "OpenGL Screen Savers".
>  Maybe they should have their own place in the hierarchy?  ("Really Slick Screen
> Savers"?)  I think you do this by using X-KDE-Category= in the desktop file. 
> It's probably also worth using X-KDE-Type=OpenGL as well.  But it looks like
> you're just using the desktop files supplied by upstream here; I'm not sure if
> it's worth it to hack them up.
> 
> The desktop files include "Actions=InWindow;Root" but then go on to define a
> Setup action as well.

Let others decide whether it was worth it, it was just one line of awk per
desktop file ;-).

> The setup page for each screensaver doesn't work at all unless you also install
> rss-glx-screensaver.  Should this be a dependency of rss-glx-kde, or is
> something else wrong?

Seems KDE (specifixally its kxs* tools) depends on the XSS files being in place
for all XSS-like hacks. I've added a dependency.

> The screensavers don't actually do anything; the "Test" button causes the
> desktop to pause (system monitor and clock stop updating), but nothing is
> actually displayed.  This happens on my home machine with binary Nvidia driver
> and a machine here at work with a Radeon R200 (stock X driver).  Any ideas?

Not ATM. I'll try it at a colleague's desktop (he uses KDE) to see whether the
above fixes change anything in that regard.

I've put the new package at (note the added ".p" in the version to indicate that
we're working with modified sources):

http://tiptoe.de/dav/rss-glx-0.8.1.p-0.5.src.rpm
Comment 10 Nils Philippsen 2006-05-24 12:22:22 EDT
Hmm, it didn't work on his KDE desktop either. Should I disable the -kde
subpackage then until upstream fixes it or someone provides a patch as I don't
really have a clue about the KDE screensaver system?
Comment 11 Jason Tibbitts 2006-05-25 00:25:21 EDT
I did some work and found something useful:

KDE does this to start the biof (for example) screensaver when you click the
"Test" button:

kxsrun biof -- --root

(this comes from the biof.desktop file).

strace'ing this gives (among a big pile of stuff):

8989  access("/usr/libexec/xscreensaver/biof", F_OK) = -1 ENOENT (No such file
or directory)

But it was installed in /usr/bin/biof.  Symlinking it to where kxsrun wants to
see it results in a working screensaver.

So I guess it's reasonable to ask why these are in /usr/bin instead of
/usr/libexec/xscreensaver.  They aren't really useful when called directly, are
they?

I also noticed that xscreensaver-demo doesn't show any of the new hacks, even
after I make the symlink.  (I have the rss-glx-xscreensaver package installed.)
Comment 12 Dennis Gilmore 2006-05-25 00:47:09 EDT
running strings 
/usr/bin/kxsrun  

shows up 
/usr/libexec/xscreensaver 

If i had to guess there is a hardcoded check there  so your two options are 
move the binaries or symlink them in the kde package.  while not useful by 
themselves  you can call them  and get a preview in a window
Comment 13 Jason Tibbitts 2006-05-25 11:30:41 EDT
I note that neither xscreensaver-extras nor xscreensaver-gl-extras installs any
of the hacks in /usr/bin.  Perhaps would be a good idea for this package to
follow that example, not only due to the KDE issue but to maintain consistency.
Comment 14 Nils Philippsen 2006-05-26 05:05:40 EDT
Agreed, at least symlinking those files in the xscreensavers subpackage sounds
feasible, which might also help with xscreensaver-demo. The placement of them in
/usr/bin is upstream matter and I'll leave them there.
Comment 15 Nils Philippsen 2006-05-26 07:20:24 EDT
The new SRPM is at:

http://tiptoe.de/dav/rss-glx-0.8.1.p-0.5.src.rpm

Changes:
- Contains the symlinks for XScreenSaver/kxs*
- owns non-standard directories
Comment 16 Nils Philippsen 2006-05-26 07:22:09 EDT
Wrong URL (cut and paste error), real one is:

http://tiptoe.de/dav/rss-glx-0.8.1.p-0.6.src.rpm
Comment 17 Jason Tibbitts 2006-05-27 00:48:01 EDT
OK, it builds fine and all of the hacks work under KDE.  They still don't show
up in xscreensaver-demo, but I believe that in order for that to work this
package would have to patch /usr/share/X11/app-defaults/XScreenSaver to add the
new hacks to *programs, which is probably a bad idea.

I do still think these shouldn't live in /usr/bin, but as you say that's a
choice made by upstream and you can indeed get useful behavior by running the
hacks manually.  (I have flying donuts in my root menu at the moment.)

APPROVED
Comment 18 Mamoru TASAKA 2006-05-29 09:21:32 EDT
Debuginfo rpm contains broken symlinks.

./rss-glx_0.8.1.p/oglc_src/driver.cpp
./rss-glx_0.8.1.p/other_src/driver.c
./rss-glx_0.8.1.p/reallyslick/c_src/driver.c
./rss-glx_0.8.1.p/reallyslick/cpp_src/driver.cpp

All of these are broken.
Comment 19 Nils Philippsen 2006-05-29 11:32:54 EDT
(In reply to comment #18)
> Debuginfo rpm contains broken symlinks.
> 
> ./rss-glx_0.8.1.p/oglc_src/driver.cpp
> ./rss-glx_0.8.1.p/other_src/driver.c
> ./rss-glx_0.8.1.p/reallyslick/c_src/driver.c
> ./rss-glx_0.8.1.p/reallyslick/cpp_src/driver.cpp
> 
> All of these are broken.

This is a bug in rpmbuild debuginfo creation, see bug #189928.

Comment 20 Mamoru TASAKA 2006-05-29 11:56:15 EDT
Created attachment 130194 [details]
Some fixes for rss-glx.spec

I am very glad that rss-glx is packaged in fedora-extras, Thanks!!

Now:
A. I agree with comment 11 by Jason Tibbitts that binary file should be located
on %{_libexecdir}/xscreensaver.
B. As I have commented on comment 18, debuginfo includes some broken symlinks.
C. Compilation options does not follow ${RPM_OPT_FLAGS}.

Please consider my patch for rss-glx.spec .
Comment 21 Mamoru TASAKA 2006-05-29 12:07:10 EDT
Hello, Nils.

For symlink problem, I received your comment #19  by mail AFTER I posted #20.
Sorry for not recognizing your comment.
Comment 22 Nils Philippsen 2006-06-02 06:37:31 EDT
In 0.8.1.p-2 I've put in your code to work around RPM bug #189928 as well as a
patch to configure.in which actually honours the CFLAGS/CXXFLAGS passed on by
%configure.

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