[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 #3 from Richard Shaw <hobbes1069 at gmail.com> 2011-06-30 14:44:13 EDT ---
(In reply to comment #2)
> (In reply to comment #1)
> > # 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 :)

Hmmm... Of course it's a draft and there seems to be people on both sides of
the issue. Since that's the case it's your call.


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

Works for me.


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

Yup, my bad. I was somehow reading the Requires(pre... as BuildRequires(pre...
so please ignore!


> > touch ./NEWS
> 
> configure fails in the absence of NEWS. :/

Interesting... Never mind then.


Ok, so I assume that the schema stuff is right out of the guidelines? If so
that should be fine. Do the spec and SRPM links reflect your changes? If so
I'll give'em a whirl.

One of the reasons I chose to review this package was because I work in the
medical device industry and I at one time wrote a VERY quick and dirty python
image viewer using ImageMagick so we could review DICOM images from a digital
x-ray machine. I even had basic stuff like image rotation and color adjustment
built into it. Really ImageMagick was doing all the work, I just used Python to
wrap a GUI around it.

Richard

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