Bug 207853

{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

You may like to use 
sed -i -e 's|\t| |g' tclabc.spec to remove that rpmlint warning

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.

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

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?

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.

This is the correct new srpm:
http://math.ifi.unizh.ch/fedora/5/i386/SRPMS.gemi/tclabc-1.0.7-2.src.rpm

I will review this later.

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.

(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.

(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).

Here is the update with fixes:
http://math.ifi.unizh.ch/fedora/5/i386/SRPMS.gemi/tclabc-1.0.7-3.src.rpm

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.

(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.