Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=451153 Lubomir Rintel <lkundrak@xxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+ --- Comment #5 from Lubomir Rintel <lkundrak@xxxxx> 2009-01-04 07:10:59 EDT --- Just a few minor issues/notes: 1.) The license seems to be GPL 2 or newer, therefore GPLv2+, please correct the License tag 2.) What is %contentdir for? You do: %define contentdir /var/www mkdir -p -m0755 %{buildroot}%{contentdir}/html Yet you do not seem to be putting anything there, nor is it included in the resulting package. 3.) This does not look very nice: set +x for f in `find . -type f` ; do if file $f | grep -q ISO-8859 ; then I'm not sure whether it's safe to rely on libmagic, since as far as I know (I may be wrong as well..), it identifies the charset used by the first few (kilo?) bytes, not reading the whole file. Why not convert all files -- it may be that iconving all files may be even faster than, or at least comparable, with calling file against them. set -x iconv -f ISO-8859-1 -t UTF-8 $f > ${f}.tmp && \ mv -f ${f}.tmp $f I'm not sure about use of && here. I think you should break the build if iconv fails instead of leaving a stale .tmp file. Also, a good habit is to preserve timestamp -- touch -r $f $f.tmp before mv. set +x fi if file $f | grep -q CRLF ; then I'd say you don't have to call file here as well. It is safe to remove all \r-s. set -x sed -i -e 's|\r||g' $f set +x fi done set -x Given none of these are serious enough to warrant a blocker, the package is APPROVED -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review