[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

--- Comment #2 from Ankur Sinha <sanjay.ankur at gmail.com> 2011-06-30 14:10:47 EDT ---
(In reply to comment #1)
> 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:

Thanks Richard. I don't need a sponsor. I'm a packager already :)

> 
> # 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 doubt it. I'll get rid of the devel package. 

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

Okay. 

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

I went through this:

http://fedoraproject.org/wiki/PackagingDrafts/AutoConf

It says it isn't suggested to run autoconf during the build. What I've done is
listed as one of the solutions :)

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

Hrm, I just copied that from the find lang section in the guidelines. No harm
leaving it there, just for info. 
.
> 
> Also, since GConf2 is a BR: I don't think the BR(pre,post,preun)'s are needed
> or even do anything. 

Again, copied from the guidelines. Letting it be. 

http://fedoraproject.org/wiki/Packaging:ScriptletSnippets

> 
> 
> rm -rvf dcmtk
> 
> This line should have a comment above it. I assume you're removing a bundled
> library? 

Yes. Added a comment. 

> 
> 
> touch ./NEWS

configure fails in the absence of 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.


http://ankursinha.fedorapeople.org/aeskulap/aeskulap.spec

http://ankursinha.fedorapeople.org/aeskulap/aeskulap-0.2.2-0.2beta1.fc15.src.rpm

Thanks!
Ankur

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