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 --- Comment #6 from Balint Cristian <rezso@xxxxxxxx> 2009-01-04 07:24:11 EDT --- (In reply to comment #5) > 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 - Temporary macro to allow /var/www path. Since this package deal a lot with /var/www path, and will in the future if this piece of software will expand. RPM really miss many of some handfull macros, this is not the first in my opinion. > 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 - Yes true. However if file is not ISO-8859-1 and try force convert to UTF-8 than rpmlint will yale, so its safe for now. ~99% is always ISO-8859-2. - I am upset all time when upstream not follow UTF-8 so this jhack is usefull, especialy rpmlint will allways signal if something. > 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. - Hmm. Never encounter such issue. However this combination never failed on any of my packages untill now. > > 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 - Will take care in next spin %{version}+1 > > 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 - Will proceed in next spin %{version}+1 However these 'sanity' bits was suggested by Patrice Dumas and Mamoru Tasaka, my very first reviewers. I never investigated deeply how can be improved, i just always do it on any of my packages. > 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