[Bug 712624] Review Request: aeskulap - A full open source replacement for commercially available DICOM viewer

[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=712624

Richard Shaw <hobbes1069 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hobbes1069 at gmail.com

--- Comment #1 from Richard Shaw <hobbes1069 at gmail.com> 2011-06-27 14:59:26 EDT ---
If you don't need a sponsor then I can review your package. Here's a quick and
dirty review of the spec file. I'll add my comments inline with OK stuff
snipped out:

# Note: devel package does not contain any headers. Do I need to add them
# .so files are shared libraries, I need to call /sbin/ldconfig, but where? In
a
# post section for the devel package?
# Schemas handling also needs to be looked at.

I'm not sure if you need a devel package. Is there any reason a program would
want to build against this one? If so, then it probably should contain the
headers, if not, then probably no. 

I don't think you need to call ldconfig since you're putting the libraries in a
named directory and not in the default (i.e. /usr/lib{,64}). 

I'm not qualified to review the schema portion so we'll need some help on that
part.


# applied all the patches from the debian package
# svn export svn://svn.debian.org/svn/debian-med/trunk/packages/aeskulap/trunk/
aeskulap-debian
Patch0:         %{name}-circular-svg.patch
Patch2:         %{name}-DcmElement.patch
Patch3:         %{name}-desktop.patch
Patch4:         %{name}-findAndCopyElement.patch
Patch5:         %{name}-gcc.patch
Patch6:         %{name}-i18n.patch
# This is used to update the configure.in before running autoreconf fo update
the autotoolization. 

fo -> to


# Not used in spec file. It's listed here so it's there with the sources
Patch7:         %{name}-configure.patch
Patch8:         %{name}-oflog.patch
Patch9:         %{name}-patientNames.patch
# applied patch7, ran autreconf -if, and then took a diff to generate this
patch
# The "-if" flag is to update the libtool macros.
Patch10:        %{name}-autotools.patch

I wonder if this is the best way to handle this. I've run into a problem like
this with a package I maintain on RPM Fusion. I ended up calling autoconf from
%build, i.e.:
"""
%build
# Necessary due to patched configure.in
/bin/bash ./autogen.sh 
"""

My way is pretty ugly too. Not sure which way is better.


BuildRequires:   dcmtk-devel
BuildRequires:   gettext intltool libpng-devel libjpeg-turbo-devel
BuildRequires:   libtiff-devel gtkmm24-devel libglademm24-devel 
BuildRequires:   gconfmm26-devel libtool
BuildRequires:   openssl-devel
BuildRequires:   gettext-devel
BuildRequires:   tcp_wrappers-devel
BuildRequires:   desktop-file-utils
BuildRequires:   GConf2
Requires(pre):   GConf2
Requires(post):  GConf2
Requires(preun): GConf2

I couldn't find anything definitive on this so hopefully someone more
knowledgeable will chime in. Since you are already requiring "gettext-devel",
I'm not sure you need "gettext".

Also, since GConf2 is a BR: I don't think the BR(pre,post,preun)'s are needed
or even do anything. 


rm -rvf dcmtk

This line should have a comment above it. I assume you're removing a bundled
library? 


touch ./NEWS

If there's no NEWS file upstream I don't think adding an empty one is
necessary, in fact I believe rpmlint will complain.

# remove .la files. Is this sufficient?
find $RPM_BUILD_ROOT -name "*.la" -exec rm -fv '{}' \;

Yup, that will do it.

-- 
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.