Bug 684938 - Review Request: wmdrawer - Retractable button bar launcher dockapp
Summary: Review Request: wmdrawer - Retractable button bar launcher dockapp
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Richard Shaw
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-03-14 20:34 UTC by Mario Blättermann
Modified: 2011-08-22 14:48 UTC (History)
3 users (show)

Fixed In Version: wmdrawer-0.10.5-11.fc16
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-08-09 01:30:09 UTC
Type: ---
hobbes1069: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Mario Blättermann 2011-03-14 20:34:51 UTC
Spec URL: http://dl.dropbox.com/u/19373040/Fedora/SPECS/wmdrawer.spec
SRPM URL: http://dl.dropbox.com/u/19373040/Fedora/wmdrawer-0.10.5-8.fc14.src.rpm
Description: wmDrawer is a dock application (dockapp) which provides a drawer
(retractable button bar) to launch applications.

Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2911935

Comment 1 Mario Blättermann 2011-03-25 19:15:54 UTC
Due to continuing problems with the startup script, I've dropped it now and added a hint to the description.

The initial problem was: The dockapp doesn't start if no local config file was found. The script runs a test if a config file exists, and if not, then the example config file shipped with the package will be copied to the home folder. Actually I assume that dockapp users should be somewhat familiar with the command line, because that's the only way to see the error message. We have no *.desktop file, that's why the user has to launch wmdrawer from the command line anyway. 

Spec URL: http://dl.dropbox.com/u/19373040/Fedora/SPECS/wmdrawer.spec
SRPM URL:
http://dl.dropbox.com/u/19373040/Fedora/wmdrawer-0.10.5-9.fc14.src.rpm

Comment 2 Mario Blättermann 2011-07-03 17:21:40 UTC
Files have moved to a new location:

Spec URL: http://mariobl.fedorapeople.org/Review/SPECS/wmdrawer.spec
SRPM URL: http://mariobl.fedorapeople.org/Review/SRPMS/wmdrawer-0.10.5-9.fc15.src.rpm

Comment 3 Richard Shaw 2011-07-25 19:28:42 UTC
Ok, first things first! :)

Looking at the spec file you're copying INSTALL with the %doc's. I've noticed several packages do this but you don't need the install directions since we're building a binary package, also, the guidelines say to leave it out.

I built the package under mock for F14 x86_64 and I get the following rpmlint output:
"""
$ rpmlint *.rpm
wmdrawer.src: W: spelling-error Summary(en_US) dockapp -> dock app, dock-app, dockage
wmdrawer.src: I: enchant-dictionary-not-found de
wmdrawer.src: W: spelling-error %description -l en_US dockapp -> dock app, dock-app, dockage
wmdrawer.src: W: spelling-error %description -l en_US usr -> use, us, user
wmdrawer.src: W: file-size-mismatch wmdrawer-0.10.5.tar.gz = 49109, http://people.easter-eggs.org/~valos/wmdrawer/wmdrawer-0.10.5.tar.gz = 49479
wmdrawer.x86_64: W: spelling-error Summary(en_US) dockapp -> dock app, dock-app, dockage
wmdrawer.x86_64: W: spelling-error %description -l en_US dockapp -> dock app, dock-app, dockage
wmdrawer.x86_64: W: file-not-utf8 /usr/share/doc/wmdrawer-0.10.5/ChangeLog
wmdrawer.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/wmdrawer-0.10.5/wmdrawer-it.sgml
wmdrawer.x86_64: W: file-not-utf8 /usr/share/doc/wmdrawer-0.10.5/wmdrawer-it.sgml
wmdrawer.x86_64: W: file-not-utf8 /usr/share/doc/wmdrawer-0.10.5/AUTHORS
wmdrawer.x86_64: E: incorrect-fsf-address /usr/share/doc/wmdrawer-0.10.5/COPYING
wmdrawer-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/wmdrawer-0.10.5/graphics.c
wmdrawer-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/wmdrawer-0.10.5/config.c
wmdrawer-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/wmdrawer-0.10.5/utils.c
wmdrawer-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/wmdrawer-0.10.5/images.c
wmdrawer-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/wmdrawer-0.10.5/types_defs.h
wmdrawer-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/wmdrawer-0.10.5/wmdrawer.c
3 packages and 0 specfiles checked; 7 errors, 10 warnings.
"""

1. Problem: The file-size-mismatch. Did you alter the source archive? Or maybe upstream tweaked something without creating a new version?

2. All the wrong-file-end-of-line-encoding and file-not-utf8 errors should probably be fixed with "dos2linux -k <file>" after "make install". 

3. The incorrect-fsf-address is nothing you can fix but should be reported upstream.

Comment 4 Richard Shaw 2011-07-25 19:41:51 UTC
After looking into things a bit more I noticed a few other issues. 

4. There are no files in the wmdrawer-debuginfo package. The reason is that the Makefile is using "strip" to remove the debugging symbols. There's no flag to pass to stop that behavior so I fixed it with sed as follows:

# Prevent the Makefile from stripping the binaries
# Otherwise there are no debugging symbols
sed -i '/strip/d' Makefile

5. The makefile is not honoring your "OPTS" argument to make. I tried changing it to CFLAGS but that broke compiling because it replaced CFLAGS in the makefile instead of adding to it. I fixed that as follows:

# Patch the Makefile so that CFLAGS are added not replaced.
sed -i 's/CFLAGS =/override CFLAGS +=/g' Makefile

Then updated your make command to:

make -L CFLAGS='%{optflags}' %{?_smp_mflags}

6. Not really a problem but you could (if you wanted to) replace the last two arguments to %doc with:

doc/*.smgl

That's it as far as I know. I'll probably run it through the packaging guidelines tomorrow.

Here's a link to the updated spec if you want to use it:
http://hobbes1069.fedorapeople.org/wmdrawer.spec

Richard

Comment 5 Mario Blättermann 2011-07-25 20:13:01 UTC
Thanks for your hints and the updated spec file. But please wait with a full review. I found some more problems (runtime font requirements, use of gdk-pixbuf-2 instead of gdk-pixbuf). Stay tuned, I will fix it tomorrow.

Comment 6 Richard Shaw 2011-07-25 20:40:59 UTC
It should be a simple fix. I think I remember something in the makefile about gdk-pixbuf. I think it can use 3 different versions and you just uncomment the appropriate one. 

That being the case I think we should give up on the sed's and just make a patch to the makefile to take care of all 3 problems.

Comment 7 Mario Blättermann 2011-07-26 19:02:13 UTC
OK, I've patched the Makefile to concatenate the sed commands and the use of gdk-pixbuf2. Now it works fine.

Koji scratch build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=3232188

(In reply to comment #3)
> 
> 1. Problem: The file-size-mismatch. Did you alter the source archive? Or maybe
> upstream tweaked something without creating a new version?
Nothing done with the sources,really. I don't understand it... I've downloaded the sources again and used them for the new package. That's all I can do for now.
> 
> 2. All the wrong-file-end-of-line-encoding and file-not-utf8 errors should
> probably be fixed with "dos2linux -k <file>" after "make install". 
> 
Not dos2linux, but dos2unix is available from the yum repos. But dos2unix seems to need an input declaration. Is there a more simple way to convert the files to utf-8 with correct line breaks?

> 3. The incorrect-fsf-address is nothing you can fix but should be reported
> upstream.
It is a common problem in older software, such as a lot of WindowMaker dockapps. Upstream is dead in most cases. Well, I could fix it by patching the source files and COPYING, but I think it isn't worth the effort.

(In reply to comment #4)
[...]

> 6. Not really a problem but you could (if you wanted to) replace the last two
> arguments to %doc with:
> 
> doc/*.smgl
> 
What's the reason for that? I don't know about *.smgl files... Normally, the sgml files should be processed with docbook2man to get a manpage. Actually, I can drop the English version, because there is already a real manpage. The Italian version seems to be incomplete, given the file size. In my opinion, the sgml files should be dropped completely.

New files:

Spec URL: http://mariobl.fedorapeople.org/Review/SPECS/wmdrawer.spec
SRPM URL:
http://mariobl.fedorapeople.org/Review/SRPMS/wmdrawer-0.10.5-10.fc15.src.rpm

Comment 8 Richard Shaw 2011-07-26 19:17:39 UTC
(In reply to comment #7)
> OK, I've patched the Makefile to concatenate the sed commands and the use of
> gdk-pixbuf2. Now it works fine.
> 
> Koji scratch build:
> 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=3232188
> 
> (In reply to comment #3)
> > 
> > 1. Problem: The file-size-mismatch. Did you alter the source archive? Or maybe
> > upstream tweaked something without creating a new version?
> Nothing done with the sources,really. I don't understand it... I've downloaded
> the sources again and used them for the new package. That's all I can do for
> now.

As long as the md5sum's match it should be good, not sure why it was "off"...


> > 2. All the wrong-file-end-of-line-encoding and file-not-utf8 errors should
> > probably be fixed with "dos2linux -k <file>" after "make install". 
> > 
> Not dos2linux, but dos2unix is available from the yum repos. But dos2unix seems
> to need an input declaration. Is there a more simple way to convert the files
> to utf-8 with correct line breaks?

Sorry, you'll have to excuse my typo's, of course I meant dos2unix. I don't think it requires anything but it doesn't always get it "right" either. I've used it on a few occasions with just the "dos2unix -k <file>". If it makes rpmlint happy then we're good, if not, there's always iconv which does require more arguments and as far as I know doesn't do the conversion "in place". 

 
> > 3. The incorrect-fsf-address is nothing you can fix but should be reported
> > upstream.
> It is a common problem in older software, such as a lot of WindowMaker
> dockapps. Upstream is dead in most cases. Well, I could fix it by patching the
> source files and COPYING, but I think it isn't worth the effort.
> 
> (In reply to comment #4)
> [...]
> 
> > 6. Not really a problem but you could (if you wanted to) replace the last two

Ok, just thought I'd check.


> > arguments to %doc with:
> > 
> > doc/*.smgl
> > 
> What's the reason for that? I don't know about *.smgl files... Normally, the
> sgml files should be processed with docbook2man to get a manpage. Actually, I
> can drop the English version, because there is already a real manpage. The
> Italian version seems to be incomplete, given the file size. In my opinion, the
> sgml files should be dropped completely.

You'll have to excuse my typo again, I meant "*.sgml". All I was trying to point out is that you don't need to list both files separately on the %doc line since globs are allowed. Dropping them is also fine.

Richard

Comment 9 Richard Shaw 2011-07-26 19:31:03 UTC
Ok, two things.

1. You'r dos2unix path was pointing to the buildroot but they are not there until the %doc macro copies them, I changed it to this:

dos2unix -k AUTHORS     \
            ChangeLog   \
            doc/%{name}-it.sgml

2. New problem, rpmlint shows mixed spaces and tabs. I ran the spec file through "expand" to convert it all to spaces.

Now onto the guidelines!

Richard

Comment 10 Richard Shaw 2011-07-26 19:40:13 UTC
Assuming the above are fixed...

+: OK
-: must be fixed
=: should be fixed (at your discretion)
?: Question or clairification needed
N: not applicable

MUST:
[+] rpmlint output: shown in comment: 
[+] follows package naming guidelines
[+] spec file base name matches package name
[+] package meets the packaging guidelines
[+] package uses a Fedora approved license: GPLv2+
[+] license field matches the actual license.
[+] license file is included in %doc: COPYING
[+] spec file is in American English
[+] spec file is legible
[+] sources match upstream: md5sums match (608bf2f75fdd084f8e63764c31a2a9a5)
[+] package builds on at least one primary arch: Tested F14
[N] appropriate use of ExcludeArch
[+] all build requirements in BuildRequires
[N] spec file handles locales properly
[N] ldconfig in %post and %postun
[+] no bundled copies of system libraries
[N] no relocatable packages
[+] package owns all directories that it creates
[+] no files listed twice in %files
[+] proper permissions on files
[+] consistent use of macros
[+] code or permissible content
[N] large documentation in -doc
[+] no runtime dependencies in %doc
[N] header files in -devel
[N] static libraries in -static
[N] .so in -devel
[N] -devel requires main package
[+] package contains no libtool archives
[N] package contains a desktop file, uses desktop-file-install/validate
[+] package does not own files/dirs owned by other packages
[+] all filenames in UTF-8

SHOULD:
[N] query upstream for license text
[N] description and summary contains available translations
[+] package builds in mock
[+] package builds on all supported arches
[?] package functions as described: Did not test.
[+] sane scriptlets
[N] subpackages require the main package
[N] placement of pkgconfig files
[N] file dependencies versus package dependencies
[+] package contains man pages for binaries/scripts

*** APPROVED ***

Comment 11 Mario Blättermann 2011-07-27 19:36:30 UTC
Many thanks for your review and especially your help. I will import the following package once the Git repo has been established:

http://mariobl.fedorapeople.org/Review/SRPMS/wmdrawer-0.10.5-11.fc15.src.rpm

The remaining problems are fixed.

Comment 12 Mario Blättermann 2011-07-27 19:39:44 UTC
New Package SCM Request
=======================
Package Name: wmdrawer
Short Description: Retractable button bar launcher dockapp
Owners: mariobl
Branches: f15 f16

Comment 13 Gwyn Ciesla 2011-07-27 19:41:40 UTC
Git done (by process-git-requests).

Comment 14 Fedora Update System 2011-07-27 20:14:26 UTC
wmdrawer-0.10.5-10.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/wmdrawer-0.10.5-10.fc15

Comment 15 Fedora Update System 2011-07-27 20:37:01 UTC
wmdrawer-0.10.5-11.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/wmdrawer-0.10.5-11.fc16

Comment 16 Fedora Update System 2011-07-27 20:37:09 UTC
wmdrawer-0.10.5-11.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/wmdrawer-0.10.5-11.fc15

Comment 17 Fedora Update System 2011-07-31 03:57:55 UTC
wmdrawer-0.10.5-11.fc15 has been pushed to the Fedora 15 testing repository.

Comment 18 Fedora Update System 2011-08-09 01:30:02 UTC
wmdrawer-0.10.5-11.fc15 has been pushed to the Fedora 15 stable repository.

Comment 19 Fedora Update System 2011-08-22 14:48:16 UTC
wmdrawer-0.10.5-11.fc16 has been pushed to the Fedora 16 stable repository.


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