[Bug 451153] Review Request: mapbender - Geospatial portal for OGC OWS architectures

[bugzilla at redhat.com]

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 at rdsor.ro>  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.