Bug 618354 - Review Request: cwallpaper - A front end for fbsetbg, Esetroot, feh, and other wallpaper changers
Summary: Review Request: cwallpaper - A front end for fbsetbg, Esetroot, feh, and othe...
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Martin Gieseking
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2010-07-26 18:32 UTC by Germán Racca
Modified: 2011-12-23 07:19 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2011-12-22 15:00:19 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Germán Racca 2010-07-26 18:32:52 UTC
Spec: http://skytux.fedorapeople.org/packages/cwallpaper.spec

SRPM: http://skytux.fedorapeople.org/packages/cwallpaper-0.3.2-2.fc13.src.rpm

Description:
CWallpaper is a graphical wallpaper changing program designed to be used with
environments such as Fluxbox and Blackbox which lack their own wallpaper
changing programs. It is designed to be compatible with nearly every desktop
environment and root wallpaper changer possible.

Current Features:

 * Compatible with fbsetbg, bsetbg, esetroot, feh.
 * Also has KDE Desktop, Nautilus, and Rox support through shell scripts.
 * MS Windows support, using bsetroot from the Blackbox for Windows project.
 * Thumbnail preview of the wallpaper.
 * Loading different config files from command line.
 * Long wallpaper lists.
 * A GTK and FLTK version.
________________________________________________________________________________

Koji scratch builds:

F-12: http://koji.fedoraproject.org/koji/taskinfo?taskID=2351737
F-13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2351789
Rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=2351874
________________________________________________________________________________

$ rpmlint {SPECS,SRPMS,RPMS/i686}/cwallpaper*
SPECS/cwallpaper.spec: W: no-cleaning-of-buildroot %install
cwallpaper.src: W: spelling-error Summary(en_US) fbsetbg -> setback, subset, besetting
cwallpaper.src: W: spelling-error Summary(en_US) feh -> eh, fee, fen
cwallpaper.src: W: spelling-error %description -l en_US fbsetbg -> setback, subset, besetting
cwallpaper.src: W: spelling-error %description -l en_US bsetbg -> subset, setback, basset
cwallpaper.src: W: spelling-error %description -l en_US esetroot -> beetroot, esotropia, restroom
cwallpaper.src: W: spelling-error %description -l en_US feh -> eh, fee, fen
cwallpaper.src: W: spelling-error %description -l en_US bsetroot -> beetroot, betroths, betroth
cwallpaper.src: W: spelling-error %description -l en_US config -> con fig, con-fig, configure
cwallpaper.src: W: no-cleaning-of-buildroot %install
cwallpaper.i686: W: spelling-error Summary(en_US) fbsetbg -> setback, subset, besetting
cwallpaper.i686: W: spelling-error Summary(en_US) feh -> eh, fee, fen
cwallpaper.i686: W: spelling-error %description -l en_US fbsetbg -> setback, subset, besetting
cwallpaper.i686: W: spelling-error %description -l en_US bsetbg -> subset, setback, basset
cwallpaper.i686: W: spelling-error %description -l en_US esetroot -> beetroot, esotropia, restroom
cwallpaper.i686: W: spelling-error %description -l en_US feh -> eh, fee, fen
cwallpaper.i686: W: spelling-error %description -l en_US bsetroot -> beetroot, betroths, betroth
cwallpaper.i686: W: spelling-error %description -l en_US config -> con fig, con-fig, configure
cwallpaper.i686: W: unstripped-binary-or-object /usr/bin/cwallpaper
cwallpaper.i686: W: no-manual-page-for-binary cwallpaper
2 packages and 1 specfiles checked; 0 errors, 20 warnings.

Comment 1 Ralph Lange 2010-09-08 21:47:20 UTC
Hi Germán,

I had a look at your package, which looks almost fine to me.

General question: You are packaging the 0.3.2 GTK version of cwallpaper, not
the newer versions (0.3.3 -> 0.4.2) that are based on fltk, which is also part
of Fedora. What is your reason for this?

Looking at the spec file:
 * Your summary could even be more concise and skip the "A" at the beginning.
 * Add "rm -rf %{buildroot}" to the beginning of the %install section 
   to get rid of rpmlint's no-cleaning-of-buildroot warning.

The build generates ~12 warnings, none of which seems critical.
Upstream might still be interested in fixing those.

Here's an informal review - I'm still working on my reviewing skills...

$ rpmlint cwallpaper.spec /var/lib/mock/fedora-rawhide-i386/result/cwallpaper*
cwallpaper.spec: W: no-cleaning-of-buildroot %install
cwallpaper.i686: W: spelling-error Summary(en_US) fbsetbg -> setback, subset, besetting
cwallpaper.i686: W: spelling-error Summary(en_US) feh -> eh, fee, fen
cwallpaper.i686: W: spelling-error %description -l en_US fbsetbg -> setback, subset, besetting
cwallpaper.i686: W: spelling-error %description -l en_US bsetbg -> subset, setback, basset
cwallpaper.i686: W: spelling-error %description -l en_US esetroot -> beetroot, esotropia, restroom
cwallpaper.i686: W: spelling-error %description -l en_US feh -> eh, fee, fen
cwallpaper.i686: W: spelling-error %description -l en_US bsetroot -> beetroot, betroths, betroth
cwallpaper.i686: W: spelling-error %description -l en_US config -> con fig, con-fig, configure
cwallpaper.i686: W: no-manual-page-for-binary cwallpaper
cwallpaper.src: W: spelling-error Summary(en_US) fbsetbg -> setback, subset, besetting
cwallpaper.src: W: spelling-error Summary(en_US) feh -> eh, fee, fen
cwallpaper.src: W: spelling-error %description -l en_US fbsetbg -> setback, subset, besetting
cwallpaper.src: W: spelling-error %description -l en_US bsetbg -> subset, setback, basset
cwallpaper.src: W: spelling-error %description -l en_US esetroot -> beetroot, esotropia, restroom
cwallpaper.src: W: spelling-error %description -l en_US feh -> eh, fee, fen
cwallpaper.src: W: spelling-error %description -l en_US bsetroot -> beetroot, betroths, betroth
cwallpaper.src: W: spelling-error %description -l en_US config -> con fig, con-fig, configure
cwallpaper.src: W: no-cleaning-of-buildroot %install
3 packages and 1 specfiles checked; 0 errors, 19 warnings.

A manual page is not available, the no-cleaning-of-buildroot warning can be fixed, all others
seem to be false positives.
I don't see the unstripped-binary-or-object warning that you saw, and the binary is stripped.

---------------------------------
key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
[+] MUST: The License field in the package spec file must match the actual license.
[+] MUST: The file containing the text of the license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum cwallpaper-0.3.2.tar.gz*
    85ca1399e8960097cbb6580dceb47163  cwallpaper-0.3.2.tar.gz
    85ca1399e8960097cbb6580dceb47163  cwallpaper-0.3.2.tar.gz.1

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[.] MUST: If the package does not successfully compile, build or work ...
[+] MUST: All build dependencies must be listed in BuildRequires.
[.] MUST: The spec file MUST handle locales properly.
[.] MUST: Packages storing shared library files must call ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates.
[+] MUST: A Fedora package must not list a file more than once in %files.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[.] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[.] MUST: .so files must go in a -devel package.
[.] MUST: devel packages must require the base package.
[+] MUST: Packages must NOT contain any .la libtool archives.
[+] MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed.
[+] MUST: Packages must not own files or directories already owned by other packages.
[-] MUST: All filenames in rpm packages must be valid UTF-8.

[.] SHOULD: If the source package does not include license text(s) ...
[.] SHOULD: description and summary translations for supported Non-English languages.
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The package should compile and build into binary rpms on all supported architectures.
[X] SHOULD: The reviewer should test that the package functions as described.
    Trying to run it in mock, I get:
        mock-chroot> cwallpaper 
        Attempting to use the config file: /builddir/.config/cwallpaper/config.
        Config file was not found, using internal defaults (Hope you have fbsetbg :D ).
        Gtk-Message: Failed to load module "pk-gtk-module": libpk-gtk-module.so: cannot open shared object file: No such file or directory
        Gtk-Message: Failed to load module "canberra-gtk-module": libcanberra-gtk-module.so: cannot open shared object file: No such file or directory

        (cwallpaper:3249): Pango-WARNING **: failed to choose a font, expect ugly output. engine-type='PangoRenderFc', script='latin'
        Loading

        (cwallpaper:3249): Pango-WARNING **: failed to choose a font, expect ugly output. engine-type='PangoRenderFc', script='common'

        (cwallpaper:3249): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed
        Quiting peaceablly
    And a window with no fonts displayed.
    I am not into GTK enough to decide if this is not supposed to work in mock, or something that needs to be fixed.

[.] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin ...
[X] SHOULD: your package should contain man pages for binaries/scripts.
    Would be nice to have a manpage - but even the Debian packaged version doesn't have one.

Comment 2 Martin Gieseking 2010-09-09 17:29:19 UTC
I agree with Ralph's comments. Germán, if you plan to maintain this package for EPEL too, you must clean the buildroot at the beginning of %install. Otherwise, it's not necessary and you can also remove the BuildRoot tag.


(In reply to comment #1)
> [X] SHOULD: The reviewer should test that the package functions as described.
>     And a window with no fonts displayed.
>     I am not into GTK enough to decide if this is not supposed to work in mock,
>     or something that needs to be fixed.

That's OK. mock doesn't install xorg packages and fonts by default. Thus, GUI applications usually don't work in a mock chroot.


Two additional remarks:

- drop the the following lines from the %description as they don't really
  relate to the binary package to be built:
  * MS Windows support, using bsetroot from the Blackbox for Windows project.
  * A GTK and FLTK version.

- I suggest to add a desktop icon to the package. You can download one at
  http://manualinux.my-place.us/cwallpaper.html
  (You should ask the author whether it's free and may be used in Fedora.)

Comment 3 Germán Racca 2010-09-09 22:53:18 UTC
Many thanks to Ralph and Martin for the review and suggestions!

Ralph:

* I'm packaging the last version for GTK+, which is 0.3.2, to be consistent with Gnome desktop, and because I like GTK more than FLTK :). Also this version works perfectly well (at least for me).

* Well, I've added the cleaning of buildroot because in the future someone else could take this package and maintain it for EPEL (but not me, because I'm not interested in, at least by now).

* I've changed the Summary as you suggested.

* I've also mailed upstream suggesting to add a man page.

Martin:

* The cleaning of buildroot and the buildroot tag are preserved because as I said above someone else could take this package for EPEL in the future.

* I've modified the %description as you suggested.

* I can't connect to this site: http://manualinux.my-place.us/cwallpaper.html. I'm going to continue to try later.

Thanks again folks!

Please find updated files here:

SPEC: http://skytux.fedorapeople.org/packages/cwallpaper.spec

SRPM: http://skytux.fedorapeople.org/packages/cwallpaper-0.3.2-3.fc13.src.rpm
________________________________________________________________________________

rpmlint output:

Checking RPM package (cwallpaper-0.3.2-3.fc13.i686.rpm)
--------------------
cwallpaper.i686: W: spelling-error Summary(en_US) fbsetbg -> setback, subset, besetting
cwallpaper.i686: W: spelling-error Summary(en_US) feh -> eh, fee, fen
cwallpaper.i686: W: spelling-error %description -l en_US fbsetbg -> setback, subset, besetting
cwallpaper.i686: W: spelling-error %description -l en_US bsetbg -> subset, setback, basset
cwallpaper.i686: W: spelling-error %description -l en_US esetroot -> beetroot, esotropia, restroom
cwallpaper.i686: W: spelling-error %description -l en_US feh -> eh, fee, fen
cwallpaper.i686: W: spelling-error %description -l en_US config -> con fig, con-fig, configure
cwallpaper.i686: W: no-manual-page-for-binary cwallpaper
1 packages and 0 specfiles checked; 0 errors, 8 warnings.

Checking DEBUG package (cwallpaper-debuginfo-0.3.2-3.fc13.i686.rpm)
----------------------
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Checking SPEC file (cwallpaper.spec)
------------------
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

Checking SRPM package (cwallpaper-0.3.2-3.fc13.src.rpm)
---------------------
cwallpaper.src: W: spelling-error Summary(en_US) fbsetbg -> setback, subset, besetting
cwallpaper.src: W: spelling-error Summary(en_US) feh -> eh, fee, fen
cwallpaper.src: W: spelling-error %description -l en_US fbsetbg -> setback, subset, besetting
cwallpaper.src: W: spelling-error %description -l en_US bsetbg -> subset, setback, basset
cwallpaper.src: W: spelling-error %description -l en_US esetroot -> beetroot, esotropia, restroom
cwallpaper.src: W: spelling-error %description -l en_US feh -> eh, fee, fen
cwallpaper.src: W: spelling-error %description -l en_US config -> con fig, con-fig, configure
1 packages and 0 specfiles checked; 0 errors, 7 warnings.
________________________________________________________________________________

Koji builds from scratch:

F12: http://koji.fedoraproject.org/koji/taskinfo?taskID=2458445
F13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2458454
F14: http://koji.fedoraproject.org/koji/taskinfo?taskID=2458463
F15: http://koji.fedoraproject.org/koji/taskinfo?taskID=2458480

Comment 4 Martin Gieseking 2010-11-23 15:45:28 UTC
Hi Germán,

your package looks fine now. It would be great if you could add an desktop icon for cwallpaper. Do you still have problems to access the icon at http://manualinux.my-place.us/iconos/cwallpaper-48.png ?

Comment 5 Martin Gieseking 2011-05-16 15:17:31 UTC
Germán, what's the status of this package? Would you still like to maintain it?

Comment 6 Martin Gieseking 2011-12-07 12:59:06 UTC
Any progress here? Germán, if you're no longer interested in this package, please close the ticket (I will close it if there's no response within a week).

Comment 8 Germán Racca 2011-12-23 00:49:59 UTC
I'm sorry Martin, I have been busy and I didn't see your comments before...but now I'm late because the bug is closed...

Comment 9 Martin Gieseking 2011-12-23 07:19:43 UTC
Not a problem at all, Germán. If you're still interested in the package, feel free to open a new review request, and I'll have a look.


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