[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


Lubomir Rintel <lkundrak at v3.sk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-review?              |fedora-review+




--- Comment #5 from Lubomir Rintel <lkundrak at v3.sk>  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.