Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 207853 - Review Request: tclabc - A Tcl interface and a Tk GUI to the ABC notation
Summary: Review Request: tclabc - A Tcl interface and a Tk GUI to the ABC notation
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-09-24 20:21 UTC by Gérard Milmeister
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-09-28 20:15:37 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Gérard Milmeister 2006-09-24 20:21:37 UTC
Spec URL: http://math.ifi.unizh.ch/fedora/spec/tclabc.spec
SRPM URL: http://math.ifi.unizh.ch/fedora/5/i386/SRPMS.gemi/tclabc-1.0.7-1.src.rpm
Description:
Tclabc is designed to help on writing music in ABC notation.

Comment 1 Parag AN(पराग) 2006-09-25 05:16:00 UTC
{Not Official Reviewer}
packaging looks ok.
+ Mockbuild is successfull for i386 FC6 
- rpmlint on source rpm is not silent 
W: tclabc mixed-use-of-spaces-and-tabs
The specfile mixes use of spaces and tabs for indentation, which is a
cosmetic annoyance.  Use either spaces or tabs for indentation, not both.
+ rpmlint on binary rpm is silent
- dist tag is NOT present
+ Buildroot is correct
+ source URL is correct
+ BR is correct
+ License used is GPL
+ License file LICENSE is included
+ MD5 sum on tarball is matching upstream tarball
34dbcb0177e11888d23ca7fa2304fb17  tclabc-1.0.7.tar.gz
+ No duplicate files



Comment 2 Parag AN(पराग) 2006-09-25 05:19:32 UTC
You may like to use 
sed -i -e 's|\t| |g' tclabc.spec to remove that rpmlint warning

Comment 3 Mamoru TASAKA 2006-09-27 07:19:23 UTC
Quick look:
(In reply to comment #1)
> + Mockbuild is successfull for i386 FC6 
Really? I cannot rebuild this in mock.

Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.91772
+ umask 022
+ cd /builddir/build/BUILD
+ LANG=C
+ export LANG
+ unset DISPLAY
+ cd /builddir/build/BUILD
+ rm -rf tclabc-1.0.7.fc6
+ /bin/gzip -dc /builddir/build/SOURCES/tclabc-1.0.7.tar.gz
+ tar -xf -
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ cd tclabc-1.0.7.fc6
/var/tmp/rpm-tmp.91772: line 33: cd: tclabc-1.0.7.fc6: No such file or directory
error: Bad exit status from /var/tmp/rpm-tmp.91772 (%prep)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.91772 (%prep)

 %{?dist} should not be added in version but in release.

* Well,
  tk requires tcl, tk-devel requires tcl-devel and tk, so
  BuildRequires tcl tk tcl-devel
  Requires  tcl
  are all unnecessary.

Comment 4 Parag AN(पराग) 2006-09-27 07:26:45 UTC
mtasaka,
    Gérard Milmeister has changed package and SPEC after i did revivew. I have
that SPEC file which did not contained any dist tag. If you see my review i have
clearly mentioned that 
- dist tag is NOT present
Here is diff between those 2 SPECs

--- tclabc_old.spec     2006-09-27 12:41:26.000000000 +0530
+++ tclabc.spec 2006-09-27 12:41:49.000000000 +0530
@@ -1,20 +1,20 @@
 Name:           tclabc
-Version:        1.0.7
+Version:        1.0.7%{?dist}
 Release:        1
 Summary:        A Tcl interface and a Tk GUI to the ABC notation

 Group:          Applications/Multimedia
-License:       GPL
+License:        GPL
 URL:            http://moinejf.free.fr
 Source0:        http://moinejf.free.fr/tclabc-1.0.7.tar.gz
 BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 BuildRequires:  tcl
-BuildRequires: tk
-BuildRequires: tcl-devel
-BuildRequires: tk-devel
+BuildRequires:  tk
+BuildRequires:  tcl-devel
+BuildRequires:  tk-devel
 Requires:       tcl
-Requires:      tk
-Requires:      abcm2ps
+Requires:       tk
+Requires:       abcm2ps


 %description


Comment 5 Parag AN(पराग) 2006-09-27 07:32:27 UTC
mtasaka,
 I also checked timestamps and filesizes of both SRPMS, one i have and the one
that is online both are different. do u want MD5 checksum of that?

Comment 6 Mamoru TASAKA 2006-09-27 08:08:52 UTC
Hello, Parag:

Well, anyway Gérard should upload new spec and srpm (with release
tag incremented) to avoid confusion. You and me can wait for it.

Comment 7 Gérard Milmeister 2006-09-27 21:33:51 UTC
This is the correct new srpm:
http://math.ifi.unizh.ch/fedora/5/i386/SRPMS.gemi/tclabc-1.0.7-2.src.rpm

Comment 8 Mamoru TASAKA 2006-09-28 06:04:21 UTC
I will review this later.

Comment 9 Mamoru TASAKA 2006-09-28 15:00:37 UTC
Well, Reviewing this.

1. From http://fedoraproject.org/wiki/Packaging/Guidelines :

* Requires and BuildRequires
  - The following Requires/BuildRequires are not needed.
    * BuildRequires: tcl <- required by tk
    * BuildRequires: tk <- required by tk-devel
    * BuildRequires: tcl-devel <- required by tk-devel
    * Requires: tcl <- required by tk

  - The results of rebuilding this between by normal rpmbuild
    and by mockbuild differ
    * The comparison of what tclabc-1.0.7-1.fc6 requires
      between the one rebuild by rpmbuild and by mockbuild:

--- 2.txt       2006-09-28 23:34:40.000000000 +0900
+++ 1.txt       2006-09-28 23:34:15.000000000 +0900
@@ -1,7 +1,5 @@
 /bin/sh  
 abcm2ps  
-libasound.so.2  
-libasound.so.2(ALSA_0.9)  
 libc.so.6  
 libc.so.6(GLIBC_2.0)  
 libc.so.6(GLIBC_2.1.3)  

    Build log also differ:
.............
@@ -65,7 +341,7 @@
 checking for dlopen in -ldl... yes
 checking for sound card... yes
 checking for SB AWE 32... yes
-checking for the ALSA library... yes
+checking for the ALSA library... no
 updating cache ./config.cache
 creating ./config.status
 creating Makefile
.............
@@ -147,67 +424,69 @@
 midi.c:2720: warning: ignoring return value of 'write', declared with attribute
warn_unused_result
 gcc -shared tclabc.o abcparse.o change.o midi.o \
                -o tclabc.so \
-               -lasound
+               
 + exit 0

    Check the BuildRequires dependency for alsa library.

* Encoding
  - Some text files are encoded in ISO-8859-1

/usr/lib/tclabc/change.tcl
/usr/lib/tclabc/detail.tcl
/usr/lib/tclabc/help.tcl
/usr/lib/tclabc/lang-de.tcl
/usr/lib/tclabc/lang-fr.tcl
/usr/lib/tclabc/lang-nl.tcl
/usr/lib/tclabc/lang-ptBR.tcl
/usr/lib/tclabc/lang-se.tcl
/usr/lib/tclabc/play.tcl
/usr/lib/tclabc/pref.tcl
/usr/lib/tclabc/select.tcl
/usr/lib/tclabc/tkabc.tcl
/usr/lib/tclabc/tkorgan.tcl
/usr/share/doc/tclabc-1.0.7/README

    * At least the encoding of /usr/share/doc/tclabc-1.0.7/README
      should be changed to UTF-8.
    * Also check if other tcl files can also have their encodings changed
      to UTF-8.

* Timestamps
  - This package includes some html files in source files, so keeping
    timestamps for these html files are recommended.
    Use:

    make install prefix=.......  INSTALL_DATA="install -c -p"

    to keep timestamps.

2. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines :

* The sources used to build the package...
  Use: http://moinejf.free.fr/tclabc-%{version}.tar.gz
  for Source0

3. Other things I have noticed.
* Well, I don't understand how to use this, however, when I try
  "tkabc bachto.mid" (bachto.mid is MIDI file), I get:

[tasaka1@localhost TEMP]$ tkabc bachto.mid 
no output MIDI synth
open: No such file or directory
cannot open MIDI in '/dev/midi00'

  .. is this okay? It seems I cannot hear any sound.

Comment 10 Gérard Milmeister 2006-09-28 15:51:18 UTC
(In reply to comment #9)
> Well, Reviewing this.
> 
> 1. From http://fedoraproject.org/wiki/Packaging/Guidelines :
> 
> * Requires and BuildRequires
>   - The following Requires/BuildRequires are not needed.
>     * BuildRequires: tcl <- required by tk
>     * BuildRequires: tk <- required by tk-devel
>     * BuildRequires: tcl-devel <- required by tk-devel
>     * Requires: tcl <- required by tk
Ok.

>     Check the BuildRequires dependency for alsa library.
Ok.

>     * At least the encoding of /usr/share/doc/tclabc-1.0.7/README
>       should be changed to UTF-8.
>     * Also check if other tcl files can also have their encodings changed
>       to UTF-8.
Ok.

> * Timestamps
>   - This package includes some html files in source files, so keeping
>     timestamps for these html files are recommended.
This I don't understand. Why is it necessary?

> 2. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines :
> 
> * The sources used to build the package...
>   Use: http://moinejf.free.fr/tclabc-%{version}.tar.gz
>   for Source0
I see nothing in the guidelines about this, but ok.

> 3. Other things I have noticed.
> * Well, I don't understand how to use this, however, when I try
>   "tkabc bachto.mid" (bachto.mid is MIDI file), I get:
> 
> [tasaka1@localhost TEMP]$ tkabc bachto.mid 
> no output MIDI synth
> open: No such file or directory
> cannot open MIDI in '/dev/midi00'
The MIDI devices have to be configured in the preferences dialog.
When alsa is used, something like "17:0" has to be used for an
alsa midi devices. However this depends on the soundcard.

Comment 11 Mamoru TASAKA 2006-09-28 16:43:39 UTC
(In reply to comment #10)
> > * Timestamps
> >   - This package includes some html files in source files, so keeping
> >     timestamps for these html files are recommended.
> This I don't understand. Why is it necessary?

Keeping timestamps is preferable because it make it clearer 
* if a packager like you added some modification for the original
  sources or text files
* when these text files or source files are originally written

So this is preferable especially when the package created contains
large number of documentations (e.g. kernel-doc).

Comment 12 Gérard Milmeister 2006-09-28 17:40:47 UTC
Here is the update with fixes:
http://math.ifi.unizh.ch/fedora/5/i386/SRPMS.gemi/tclabc-1.0.7-3.src.rpm

Comment 13 Mamoru TASAKA 2006-09-28 18:12:07 UTC
Well, sometimes iconv fails so I usually use:

iconv -f iso-8859-1 -t utf-8 $f -o $f.iconv && mv $f.iconv $f

Perhaps this is preferable (this is not a blocker).

Other things are okay.

----------------------------------------------------------------------
   This package (tclabc) is APPROVED by me.

Comment 14 Gérard Milmeister 2006-09-28 20:15:37 UTC
(In reply to comment #13)
> Well, sometimes iconv fails so I usually use:
> 
> iconv -f iso-8859-1 -t utf-8 $f -o $f.iconv && mv $f.iconv $f
> 
> Perhaps this is preferable (this is not a blocker).
Ok.

Package imported.
Build on FC-5 and devel.
added entries to owners.list, comps-fe5.xml.in and comps-fe6.xml.in.

Thanks for the review.


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