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
Summary: | Review Request: tclabc - A Tcl interface and a Tk GUI to the ABC notation | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Gérard Milmeister <gemi> |
Component: | Package Review | Assignee: | Mamoru TASAKA <mtasaka> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | mtasaka, panemade |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-09-28 20:15:37 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Gérard Milmeister
2006-09-24 20:21:37 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 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. |