Added nando's vkeybd icon.  Minor cleanups.

Spec URL: http://people.redhat.com/green/FE/FC5/vkeybd.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/vkeybd-0.1.17-4.src.rpm

Not a Review but some Chnages you need in SPEC :-
1) Use of dist tag in Release tag
2) Consider using make %{?_smp_mflags} in %build
3) Include License file

(In reply to comment #2)
> Not a Review but some Chnages you need in SPEC :-

Thanks for the comments. New bits here..

Spec URL: http://people.redhat.com/green/FE/FC5/vkeybd.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/vkeybd-0.1.17-5.src.rpm

Some source files do not have an Copyright / GPL header:

fskip.c
malloc.c
itypes.h
util.h

About the license file:
MUST: If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package must be included in %doc.

Please ask upstream to add the license and add copyright headers to the
mentioned files.

I noticed something else:

A BR is: tcl-devel >= 8.4 and in %build you use TCL_VERSION=8.4
It seems to me that because of this the spec file will not work for tcl-devel
version 8.6 so you may better use tcl-devel = 8.4 imho.

Sorry for writing so many comments, I forgot to mention that rpmlint shows an error:

E: vkeybd non-executable-script /usr/share/vkeybd/vkeybd.tcl 0444

(In reply to comment #4)
> Please ask upstream to add the license and add copyright headers to the
> mentioned files.

I've asked upstream. 

(In reply to comment #4)
> Please ask upstream to add the license and add copyright headers to the
> mentioned files.

This was the response:
"I don't mind to add but am too lazy to release a newer version for
such small pieces.  The license issue is written in README."

So, should I just remove the COPYING file I added and proceed?

just remove your COPYING file and proceed. Hopefully there will be a new 
release sometime with the file added.

Here are some other items that need to be taken care of:
- move the .desktop file to a separate file
- I think the invocation of "update-desktop-database %{_datadir}/applications" 
is not needed. According to 
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?highlight=%28update-desktop-database%29#head-de6770dd9867fcd085a73a4700f6bcd0d10294ef 
it is only need when the .desktop file has an MimeType entry
- then you do not need 
"Requires(post):   desktop-file-utils" and
"Requires(postun): desktop-file-utils"
- I am not sure, but $RPM_BUILD_ROOT/%{_datadir}/icons/vkeybd.png seems not 
the be the correct location for the icon since on my system all other icons 
are in subdirectories of icons, e.g. hicolor/<size>
- alsa-lib-devel, jack-audio-connection-kit-devel and e2fsprogs-devel can be 
removed from BuildRequires since they are already required by lash-devel
- tcl-devel can be removed from BuildRequires since it is already required by 
tk-devel if the versioned dependency can be done with tk-devel instead of 
tcl-devel

and please do not forget the issues in comment 5 and 6

I've fixed everything mentioned so far and more....

New bits here:
Spec URL: http://people.redhat.com/green/FE/FC5/vkeybd.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/vkeybd-0.1.17-6.src.rpm

Thanks.

The specfile is not up to date.

The requires for gtk should be removed, see:
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda

(In reply to comment #11)
> The requires for gtk should be removed, see:

D'oh!  Thanks for pointing that out.

New bits here:
Spec URL: http://people.redhat.com/green/FE/FC5/vkeybd.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/vkeybd-0.1.17-7.src.rpm

It is too late for me to do a complete review now but there are still some minor
issues:

- please do not use a full path in the .desktop file in the Exec line because
this make the path to the binary hardcoded.

- there are some warnings in build.log in mock: sffile.c:122: warning: ignoring
return value of 'fread', declared with attribute warn_unused_result - see them
all in the attachment.

- ChangeLog is not packaged

- did you ask upstream to include your icon / desktop files? There is already an
desktop file in the upstream tarball, so maybe upstream will include your
improved desktop file.

- the lash patch does not patch the README properly(LADCCA is still mentioned):

-  --ladcca bool
+  --lash bool
        Specify the support of LADCCA.  Give yes or no as the

- the manpage does not mention the --lash option (the upstream version not the
--ladcca option)

- have you submitted lash patch to upstream? (Just out of curiosity, what are
the advantages of lash against ladcca?

- some files have strange permissions, but I don't know whether or not this
needs to be fixed:
$ rpm -vql vkeybd | grep -- -r--r--r
-r--r--r--    1 root    root             2278 Sep 19 23:41
/usr/share/man/man1/vkeybd.1.gz
-r--r--r--    1 root    root             5765 Sep 19 23:41
/usr/share/vkeybd/vkeybd.list
-r--r--r--    1 root    root              282 Sep 19 23:41
/usr/share/vkeybd/vkeybdmap
-r--r--r--    1 root    root              590 Sep 19 23:41
/usr/share/vkeybd/vkeybdmap-german

- changing %{_datadir}/vkeybd to %{_datadir}/vkeybd/ in %files makes it more
obvious that an directory is meant

Just noticed something else:

You can remove these lines from your spec
  --add-category Application                    
  --add-category AudioVideo 
and add to the .desktop file:

Categories=Application;AudioVideo;
or
Categories=Application;AudioVideo;Audio;Midi;Music;
This makes the .desktop file even more useful for inclusion in upstream.

Created attachment 136691 [details]
mock build.log from revision 6 for development

The 0444 permission on some files do not need to be changed.

(In reply to comment #13) 
> - please do not use a full path in the .desktop file in the Exec line because
> this make the path to the binary hardcoded.

Fixed.

> - there are some warnings in build.log in mock: sffile.c:122: warning: ignoring
> return value of 'fread', declared with attribute warn_unused_result - see them
> all in the attachment.

I'll let upstream know.

> - ChangeLog is not packaged

Fixed.

> - did you ask upstream to include your icon / desktop files? There is already an
> desktop file in the upstream tarball, so maybe upstream will include your
> improved desktop file.

I'll do that.
 
> - the lash patch does not patch the README properly(LADCCA is still mentioned):
> 
> -  --ladcca bool
> +  --lash bool
>         Specify the support of LADCCA.  Give yes or no as the

Fixed.

> - the manpage does not mention the --lash option (the upstream version not the
> --ladcca option)

I'll report upstream.

> - have you submitted lash patch to upstream? (Just out of curiosity, what are
> the advantages of lash against ladcca?

LADCCA is dead.  LASH is the new LADCCA.

> - some files have strange permissions, but I don't know whether or not this
> needs to be fixed:
> $ rpm -vql vkeybd | grep -- -r--r--r
> -r--r--r--    1 root    root             2278 Sep 19 23:41
> /usr/share/man/man1/vkeybd.1.gz

Fixed.

> -r--r--r--    1 root    root             5765 Sep 19 23:41
> /usr/share/vkeybd/vkeybd.list
> -r--r--r--    1 root    root              282 Sep 19 23:41
> /usr/share/vkeybd/vkeybdmap
> -r--r--r--    1 root    root              590 Sep 19 23:41
> /usr/share/vkeybd/vkeybdmap-german

I didn't change these.

> - changing %{_datadir}/vkeybd to %{_datadir}/vkeybd/ in %files makes it more
> obvious that an directory is meant

Done.

I also updated the .desktop file as per comment #14.

Updated bits here:

Spec URL: http://people.redhat.com/green/FE/FC5/vkeybd.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/vkeybd-0.1.17-8.src.rpm

- I just noticed that it is not the latest version, 0.1.17a is available at
http://www.alsa-project.org/~iwai/vkeybd-0.1.17a.tar.bz2 with french keyboard
layout in vkeybdmap-french
- Requires: jack-audio-connection-kit is not needed because lash already depends
on it.

Builds fine in mock and rpmlint does not complain, naming, licenses, files and
scriptlets are ok.
md5sum: 3be348329a5aac3cdc21f89458d9369a matches.

So remove Requires: jack-audio-connection-kit and update to the latest version
and the package is approved.

(In reply to comment #18)
> So remove Requires: jack-audio-connection-kit and update to the latest version
> and the package is approved.

Done and done.  Thanks!