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 219653
Summary: | Review Request: solfege - music education software | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Sindre Pedersen Bjørdal <sindrepb> |
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: | gemi, opensource, peter |
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-12-26 08:59:04 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
Sindre Pedersen Bjørdal
2006-12-14 17:07:50 UTC
Changes: - Use install to install wrapper script - Improvements to wrapper script Updated: Spec URL: http://folk.ntnu.no/sindrb/packages/solfege.spec SRPM URL: http://folk.ntnu.no/sindrb/packages/solfege-3.6.4-2.src.rpm Not sure I should be doing the review, as you're implementing some of my ideas already, but I'll clear up a few simple points. rpmlint produces output for the RPMs. SRPM: W: solfege strange-permission solfege.sh.in 0755 W: solfege mixed-use-of-spaces-and-tabs (spaces: line 6, tab: line 4) For the first, `chmod -x olfege.sh.in` -- the script doesn't need to be executable in the SRPM. The second isn't particularly critical, just a matter of presentation. Pretty easy to fix so you don't have any warnings, though. the main RPM: W: solfege unstripped-binary-or-object /usr/lib/solfege/_solfege_c_midi.so E: solfege non-executable-script /usr/share/solfege/mpd/testsetup.py 0644 E: solfege non-executable-script /usr/share/solfege/src/testsetup.py 0644 No idea what's going on there. debuginfo RPM: E: solfege-debuginfo empty-debuginfo-package I'm a bit confused by this; it looks like the binaries may have gotten stripped by the `make install` or something. Just a comment: W: solfege unstripped-binary-or-object /usr/lib/solfege/_solfege_c_midi.so E: solfege non-executable-script /usr/share/solfege/mpd/testsetup.py 0644 E: solfege non-executable-script /usr/share/solfege/src/testsetup.py 0644 E: solfege-debuginfo empty-debuginfo-package Perhaps all permission issues. * *.py python script should have the permission 0755 * python module (_solfege_c_midi.so) should have the permission 0644. This is also related to debuginfo issue. Binaries are not stripped unless they have executable permission flags. (In reply to comment #3) > * python module (_solfege_c_midi.so) should have the permission 0644. Oops, I wanted to say this should be 0755, not 0644. Tried to fix the permissions. There are still problems, solutions welcome. SRPM: W: solfege strange-permission solfege.sh.in 0755 W: solfege mixed-use-of-spaces-and-tabs (spaces: line 6, tab: line 4) RPM: W: solfege unstripped-binary-or-object /usr/lib/debug/usr/lib/solfege/_solfege_c_midi.so.debug E: solfege shared-lib-without-dependency-information /usr/lib/debug/usr/lib/solfege/_solfege_c_midi.so.debug Updated: Spec URL: http://folk.ntnu.no/sindrb/packages/solfege.spec SRPM URL: http://folk.ntnu.no/sindrb/packages/solfege-3.6.4-3.src.rpm Again just a comment: W: solfege strange-permission solfege.sh.in 0755 - permission should be 0644 W: solfege mixed-use-of-spaces-and-tabs (spaces: line 6, tab: line 4) - Indentation in spec file should be spaces _or_ tabs and using _both_ is not considered good for cosmetic annoyance. W: solfege unstripped-binary-or-object /usr/lib/debug/usr/lib/solfege/_solfege_c_midi.so.debug E: solfege shared-lib-without-dependency-information /usr/lib/debug/usr/lib/solfege/_solfege_c_midi.so.debug - This is because normal binary rpm contain debug information binary. You should not write: ----------------------------------------- %files ....... %{_libdir}/* ------------------------------------------ because this includes %{_libdir}/debug, this is wrong. Got rid of all the rpmlint errors and warnings. updated: Spec URL: http://folk.ntnu.no/sindrb/packages/solfege.spec SRPM URL: http://folk.ntnu.no/sindrb/packages/solfege-3.6.4-4.src.rpm Better use spaces instead of tabs, the formatting is messy. Well, I will review this.... Well, this time I only checked for packaging issue and not checked this package for its funtion. A. From http://fedoraproject.org/wiki/Packaging/Guidelines : * Filesystem Layout - Usually when a wrapper script for the original binary is needed, the original binary should be where user's path are not passed, which is usually %{_libexecdir}. * Use rpmlint - rpmlint (for FC-devel) is not silent, for which please see below * Requires - Well, python related dependencies are not checked automatically by rpmbuild process and then should be checked manually. -- For example, /usr/share/solfege/src/utils.py contails: ----------------------------------------- import gtk ----------------------------------------- This means that this package (solfege) should need "Requires: pygtk2". -- And another example is that /usr/share/solfege/soundcard/midifilesynth.py includes the line: ----------------------------------------- import win32api ----------------------------------------- However, what package provides win32api python module? (or can this file be ignored??) * BuildRequires - libxslt-devel Please check whether this should be whether "libxslt-devel" or "libxslt", since it seems only /usr/bin/xsltproc is used and this package is included in libxslt, not -devel pacakge. * Desktop files -------------------------------------------- Categories=GNOME;Application;AudioVideo;Audio;Education;X-Fedora; -------------------------------------------- Categories "Application" "X-Fedora" is deprecated from desktop-file-utils >= 0.11 and these should be removed (this is also applied for FC5/6). For FC-devel, these causes rpmlint warnings/errors. * Documentation - /usr/share/doc/solfege-3.6.4/{AUTHORS,README} contain non-UTF8 character(s). Please change the encodings of these files to UTF-8. * Timestamps - This package (solfege) contains many image files and for these files timestamps should be kept to show correctly when these files are created. For these purpose: 1. ----------------------------------------------- export INSTALL="%{__install} -c -p" ----------------------------------------------- is needed before ----------------------------------------------- %configure --enable-docbook-stylesheet=%stylesheet ----------------------------------------------- line to pass INSTALL environment to Makefile.in 2. ------------------------------------------------ %{__sed} -i.stamp -e 's|shutil\.copy|shutil.copy2|' tools/pcopy.py ------------------------------------------------ is needed at %prep stage to keep timestamps on image files. B. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines : = this is okay. Changes: - Move original binary to %{_libexecdir} - Remove X-Fedora Category from meny entry - Add pygtk2 Requires - Replace libxlst-devel BuildRequires with libxlst - Keep timestamps for image files - Convert AUTHORS and README from iso8859 to UTF-8 Not sure if I've used the prefered method for converting from iso8859-15 to UTF-8. Please correct me if I've messed this up. Updated: SPEC: http://folk.ntnu.no/sindrb/packages/solfege.spec SRPM: http://folk.ntnu.no/sindrb/packages/solfege-3.6.4-5.src.rpm Well, * Use rpmlint - For src.rpm: ----------------------------------------------------- W: solfege macro-in-%changelog _libexecdir ----------------------------------------------------- rpmbuild tries to expand the macro in %changelog and this should be avoilded. In %changelog, when you want to use percent, please use %%, i.e. ----------------------------------------------------- - Move original binary to %%{_libexecdir} ----------------------------------------------------- * Desktop files ----------------------------------------------------- Categories=GNOME;Application;AudioVideo;Audio;Education; ----------------------------------------------------- - Well, category "Application" is also deprecated and this should be removed. You can use: ----------------------------------------------------- desktop-file-install --vendor fedora --delete-original \ --dir $RPM_BUILD_ROOT%{_datadir}/applications \ --remove-category Application \ $RPM_BUILD_ROOT%{_datadir}/applications/%{name}.desktop ----------------------------------------------------- * Documentation ----------------------------------------------------- iconv -o $RPM_BUILD_DIR/%{name}-%{version}/AUTHORS -f ISO-8859-15 -t UTF-8 $RPM_BUILD_DIR/%{name}-%{version}/AUTHORS iconv -o $RPM_BUILD_DIR/%{name}-%{version}/README -f ISO-8859-15 -t UTF-8 $RPM_BUILD_DIR/%{name}-%{version}/README ----------------------------------------------------- - Well, actually these remove AUTHORS and README. In fact rpmlint complaints as following. ----------------------------------------------------- E: solfege zero-length /usr/share/doc/solfege-3.6.4/AUTHORS E: solfege zero-length /usr/share/doc/solfege-3.6.4/README ----------------------------------------------------- And, $RPM_BUILD_DIR/%{name}-%{version} is not needed because the working directory at this stage is there. Usually: ----------------------------------------------------- for f in AUTHORS README ; do iconv -f ISO-8859-15 -t UTF-8 $f > ${f}.tmp && \ %{__mv} -f ${f}.tmp ${f} || \ %{__rm} -f ${f}.tmp done ----------------------------------------------------- Changes: - Fix charset conversion - Remove Application category from desktop file - Fix changelog Updated: SPEC: http://folk.ntnu.no/sindrb/packages/solfege.spec SRPM: http://folk.ntnu.no/sindrb/packages/solfege-3.6.4-6.src.rpm Okay. -------------------------------------------------- This package (solfege) is APPROVED by me. |