Bug 189889 - Review Request: vkeybd - Virtual MIDI Keyboard
Review Request: vkeybd - Virtual MIDI Keyboard
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Till Maas
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-04-25 11:28 EDT by Anthony Green
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-21 13:14:28 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
mock build.log from revision 6 for development (12.00 KB, text/plain)
2006-09-19 19:57 EDT, Till Maas
no flags Details

  None (edit)
Description Anthony Green 2006-04-25 11:28:18 EDT
Spec URL: http://people.redhat.com/green/FE/FC5/vkeybd.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/vkeybd-0.1.17-3.src.rpm
Description: 
This is a virtual keyboard for AWE, MIDI and ALSA drivers.
It's a simple fake of a MIDI keyboard on X-windows system.
Enjoy a music with your mouse and "computer" keyboard :-)

Adapted from old ccmra spec file.
Comment 1 Anthony Green 2006-05-18 22:42:57 EDT
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
Comment 2 Parag AN(पराग) 2006-06-01 09:31:08 EDT
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
Comment 3 Anthony Green 2006-06-01 11:08:39 EDT
(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
Comment 4 Till Maas 2006-08-23 12:10:33 EDT
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.
Comment 5 Till Maas 2006-08-23 12:15:25 EDT
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.
Comment 6 Till Maas 2006-08-23 12:17:40 EDT
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
Comment 7 Anthony Green 2006-09-17 11:41:07 EDT
(In reply to comment #4)
> Please ask upstream to add the license and add copyright headers to the
> mentioned files.

I've asked upstream. 
Comment 8 Anthony Green 2006-09-19 08:02:12 EDT
(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?
Comment 9 Till Maas 2006-09-19 10:16:22 EDT
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
Comment 10 Anthony Green 2006-09-19 15:22:46 EDT
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.
Comment 11 Till Maas 2006-09-19 18:20:59 EDT
The specfile is not up to date.

The requires for gtk should be removed, see:
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda
Comment 12 Anthony Green 2006-09-19 18:27:40 EDT
(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

Comment 13 Till Maas 2006-09-19 19:47:27 EDT
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
Comment 14 Till Maas 2006-09-19 19:56:05 EDT
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.
Comment 15 Till Maas 2006-09-19 19:57:39 EDT
Created attachment 136691 [details]
mock build.log from revision 6 for development
Comment 16 Till Maas 2006-09-20 04:42:25 EDT
The 0444 permission on some files do not need to be changed.
Comment 17 Anthony Green 2006-09-25 13:40:40 EDT
(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
Comment 18 Till Maas 2006-10-17 06:48:19 EDT
- 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.
Comment 19 Anthony Green 2006-10-21 13:14:28 EDT
(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!

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