Bug 188574

*** Bug 90133 has been marked as a duplicate of this bug. ***

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.

(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

Hmm, that generated tarball didn't build here. Stay tuned, I'll probably have a
bug in the generating script or in the patch.

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.

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

(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

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.

(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

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?

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

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

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.

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.

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

Wrong URL (cut and paste error), real one is:

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

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

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.

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

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 .

Hello, Nils.

For symlink problem, I received your comment #19  by mail AFTER I posted #20.
Sorry for not recognizing your comment.

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.