Bug 712624 (aeskulap) - Review Request: aeskulap - A full open source replacement for commercially available DICOM viewer
Summary: Review Request: aeskulap - A full open source replacement for commercially av...
Keywords:
Status: CLOSED ERRATA
Alias: aeskulap
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Richard Shaw
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: fedora-medical
TreeView+ depends on / blocked
 
Reported: 2011-06-11 20:57 UTC by Ankur Sinha (FranciscoD)
Modified: 2011-07-22 19:34 UTC (History)
3 users (show)

Fixed In Version: aeskulap-0.2.2-0.5beta1.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-07-17 14:23:08 UTC
Type: ---
hobbes1069: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Ankur Sinha (FranciscoD) 2011-06-11 20:57:37 UTC
Spec URL: http://ankursinha.fedorapeople.org/aeskulap/aeskulap.spec
SRPM URL: http://ankursinha.fedorapeople.org/aeskulap/aeskulap-0.2.2-0.1beta1.fc15.src.rpm

Description: 
Aeskulap is able to load a series of special images stored in the
DICOM format for review. Additionally Aeskulap is able to query
and fetch DICOM images from archive nodes (also called PACS) over
the network.

The goal of this project is to create a full open source replacement
for commercially available DICOM viewers.

Aeskulap is based on gtkmm, glademm and gconfmm and designed to run
under Linux. Ports of these packages are available for different
platforms. It should be quite easy to port Aeskulap to any platform
were these packages are available.

Comment 1 Richard Shaw 2011-06-27 18:59:26 UTC
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.

Comment 2 Ankur Sinha (FranciscoD) 2011-06-30 18:10:47 UTC
(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

Comment 3 Richard Shaw 2011-06-30 18:44:13 UTC
(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

Comment 4 Ankur Sinha (FranciscoD) 2011-06-30 18:56:09 UTC
Yep. The links reflect the changes :)

Thanks,
Ankur

Comment 5 Richard Shaw 2011-06-30 20:34:34 UTC
Ok, I've started working through the guidelines. Here's the rpmlint output from all packages generated by rpmbuild:


$ rpmlint aeskulap-*.rpm
aeskulap.src: W: spelling-error %description -l en_US gtkmm
aeskulap.src: W: spelling-error %description -l en_US glademm -> glade mm, glade-mm, glade
aeskulap.src: W: spelling-error %description -l en_US gconfmm -> confirm, conformal, conformism
aeskulap.src: W: patch-not-applied Patch7: %{name}-configure.patch
aeskulap.x86_64: W: spelling-error %description -l en_US gtkmm
aeskulap.x86_64: W: spelling-error %description -l en_US glademm -> glade mm, glade-mm, glade
aeskulap.x86_64: W: spelling-error %description -l en_US gconfmm -> confirm, conformal, conformism

None of the spelling-error's are real


aeskulap.x86_64: W: non-conffile-in-etc /etc/gconf/schemas/aeskulap.schemas

Looking at my system this seems to be the right place, perhaps rpmlint should be updated..


aeskulap.x86_64: E: zero-length /usr/share/doc/aeskulap-0.2.2/NEWS

We knew this one would be a problem, I think it's safe to ignore.


aeskulap.x86_64: W: no-manual-page-for-binary aeskulap

I assume this is a GUI only program in which case a man page would not be expected?


aeskulap.x86_64: W: dangerous-command-in-%pre rm
aeskulap.x86_64: W: dangerous-command-in-%post rm

This obviously are part of the %gconf... macros which we're not responsible for so they can be ignored.


3 packages and 0 specfiles checked; 1 errors, 11 warnings.

Comment 6 Richard Shaw 2011-06-30 21:15:21 UTC
Are you going to be building for Fedora 14 as well? I went ahead and tried doing a mock build for F14 x86_64 and got this error:

libtool: Version mismatch error.  This is libtool 2.4, but the
libtool: definition of this LT_INIT comes from libtool 2.2.10.
libtool: You should recreate aclocal.m4 with macros from libtool 2.4
libtool: and run autoconf again.

Perhaps this is why some people prefer to re-run autoconf instead of using a patch?

Richard

Comment 7 Ankur Sinha (FranciscoD) 2011-07-03 12:00:59 UTC
(In reply to comment #6)
> Are you going to be building for Fedora 14 as well? I went ahead and tried
> doing a mock build for F14 x86_64 and got this error:
> 
> libtool: Version mismatch error.  This is libtool 2.4, but the
> libtool: definition of this LT_INIT comes from libtool 2.2.10.
> libtool: You should recreate aclocal.m4 with macros from libtool 2.4
> libtool: and run autoconf again.
> 
> Perhaps this is why some people prefer to re-run autoconf instead of using a
> patch?
> 
> Richard

hrm! This is disappointing. I'll look into it. I was running into the same error on F15 with the original package and the patch had fixed it. I think I'll mail the -devel list and ask what the correct thing to do here would be. 

Thanks Richard,
I'll submit a new srpm ASAP.

Ankur

Comment 8 Richard Shaw 2011-07-03 21:10:38 UTC
Well it doesn't look like it's hit the online mailing archives yet but there is a discussion about calling autoconf from the spec file and the end result is that two people suggested running "autoconf -i -f" whether it needs it or not. I'm assuming the -f is for "force" which should ignore any issues due to autoconf version. YMMV...

Richard

Comment 9 Richard Shaw 2011-07-03 21:11:17 UTC
Oops... that was autoreconf -i -f, not autoconf...

Comment 10 Ankur Sinha (FranciscoD) 2011-07-04 14:53:13 UTC
rebuilt :)

Should build on F14 now.(I've actually tested it ;))


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

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

Thanks!
Ankur

Comment 11 Richard Shaw 2011-07-05 14:34:16 UTC
Ok, here's the formal review. Please ask questions as I still haven't done a lot of these and it's possible I got something wrong :)

+: OK
-: must be fixed
=: should be fixed (at your discretion)
?: Question or clairification needed
N: not applicable

MUST:
[+] rpmlint output: shown in comment: No major issues.
[+] follows package naming guidelines
[+] spec file base name matches package name
[=] package meets the packaging guidelines
[+] package uses a Fedora approved license: LGPLv2+
[-] license field matches the actual license: After more formal review the most authorative information I could find in the source was COPYING and COPYING.LIB. I think the spec should be updated to LGPLv2+ per Fedora Licensing requirements: http://fedoraproject.org/wiki/Licensing.
[N] license file is included in %doc
[+] spec file is in American English
[+] spec file is legible
[+] sources match upstream: md5sum "33a0f8659909426c67bebc10bd61b1d0" for both.
[+] package builds on at least one primary arch: Tested F14 x86_64 and F15 i686
[N] appropriate use of ExcludeArch
[+] all build requirements in BuildRequires
[+] spec file handles locales properly
[N] ldconfig in %post and %postun
[+] no bundled copies of system libraries
[N] no relocatable packages
[+] package owns all directories that it creates
[+] no files listed twice in %files
[+] proper permissions on files
[+] consistent use of macros
[+] code or permissible content
[N] large documentation in -doc
[+] no runtime dependencies in %doc
[N] header files in -devel
[N] static libraries in -static
[N] .so in -devel
[N] -devel requires main package
[+] package contains no libtool archives
[+] package contains a desktop file, uses desktop-file-install/validate
[+] package does not own files/dirs owned by other packages
[+] all filenames in UTF-8

SHOULD:
[=] query upstream for license text
[N] description and summary contains available translations
[+] package builds in mock
[+] package builds on all supported arches
[+] package functions as described: Ran binary and help was displayed
[+] sane scriptlets
[N] subpackages require the main package
[N] placement of pkgconfig files
[N] file dependencies versus package dependencies
[N] package contains man pages for binaries/scripts

Comment 12 Ankur Sinha (FranciscoD) 2011-07-09 05:14:20 UTC
Hi Richard,

I've updated the spec/srpm:

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

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

Thanks! :)
Ankur

Comment 13 Ankur Sinha (FranciscoD) 2011-07-09 05:15:31 UTC
Oh, and could you please set the review flag to "?" as per the review guidelines?

Thanks,
Ankur

Comment 14 Richard Shaw 2011-07-09 12:46:28 UTC
(In reply to comment #13)
> Oh, and could you please set the review flag to "?" as per the review
> guidelines?

Woops! Got it.

Comment 15 Richard Shaw 2011-07-09 13:06:12 UTC
Ok, I think I've found the last problem before I can approve.

Apparently the macros in the gconf scriptlets assume a .schemas extention so when you use:
"""
%pre
%gconf_schema_prepare %{name}.schemas

%post
%gconf_schema_upgrade %{name}.schemas
touch --no-create %{_datadir}/icons/hicolor &>/dev/null || :

%preun
%gconf_schema_remove %{name}.schemas
"""

It actually looks for aeskulap.schemas.schemas (see yum install output):
"""
Running Transaction Test
Transaction Test Succeeded
Running Transaction
  Installing     : aeskulap-0.2.2-0.4beta1.fc14.x86_64                      1/1 
I/O warning : failed to load external entity "/etc/gconf/schemas/aeskulap.schemas.schemas"
Failed to open `/etc/gconf/schemas/aeskulap.schemas.schemas': No such file or directory

Installed:
  aeskulap.x86_64 0:0.2.2-0.4beta1.fc14                                         

Complete!
"""

I think if you take those extensions off we should be good. I checked the GConf guidelines[1] again and it is subtle but the example it showed was for "foobar.schemas" then you just use:

%gconf_schema_prepare foobar
%gconf_schema_obsolete foo

Thanks,
Richard

[1] http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#GConf

Comment 16 Ankur Sinha (FranciscoD) 2011-07-09 15:18:12 UTC
Okay. I've fixed that. A scratch build is in process. I'll install and check it too. 

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

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

Thanks :)
Ankur

Comment 17 Ankur Sinha (FranciscoD) 2011-07-09 15:54:24 UTC
http://koji.fedoraproject.org/koji/taskinfo?taskID=3188339

Works!!

Comment 18 Richard Shaw 2011-07-09 22:14:31 UTC
I also did a mock build for F14 x86_64 and confirmed the problem is fix. 

At this time your package is APPROVED.

Comment 19 Ankur Sinha (FranciscoD) 2011-07-10 03:31:11 UTC
Thank you for reviewing this one Richard!! :D

Comment 20 Ankur Sinha (FranciscoD) 2011-07-10 03:33:49 UTC
New Package SCM Request
=======================
Package Name: aeskulap
Short Description: A full open source replacement for commercially available DICOM viewer
Owners: ankursinha
Branches: f14 f15
InitialCC: susmit mrceresa

Comment 21 Gwyn Ciesla 2011-07-10 04:09:01 UTC
Git done (by process-git-requests).

Comment 22 Fedora Update System 2011-07-10 05:30:33 UTC
aeskulap-0.2.2-0.5beta1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/aeskulap-0.2.2-0.5beta1.fc15

Comment 23 Fedora Update System 2011-07-10 05:31:48 UTC
aeskulap-0.2.2-0.5beta1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/aeskulap-0.2.2-0.5beta1.fc14

Comment 24 Fedora Update System 2011-07-12 05:06:10 UTC
aeskulap-0.2.2-0.5beta1.fc14 has been pushed to the Fedora 14 testing repository.

Comment 25 Ankur Sinha (FranciscoD) 2011-07-17 14:23:08 UTC
Built and pushed to repos for testing. Closing.

Comment 26 Fedora Update System 2011-07-22 19:29:48 UTC
aeskulap-0.2.2-0.5beta1.fc15 has been pushed to the Fedora 15 stable repository.

Comment 27 Fedora Update System 2011-07-22 19:34:54 UTC
aeskulap-0.2.2-0.5beta1.fc14 has been pushed to the Fedora 14 stable repository.


Note You need to log in before you can comment on or make changes to this bug.