Bug 712624 (aeskulap)

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.

(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

(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

Yep. The links reflect the changes :)

Thanks,
Ankur

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.

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

(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

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

Oops... that was autoreconf -i -f, not autoconf...

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

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

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

Oh, and could you please set the review flag to "?" as per the review guidelines?

Thanks,
Ankur

(In reply to comment #13)
> Oh, and could you please set the review flag to "?" as per the review
> guidelines?

Woops! Got it.

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

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

http://koji.fedoraproject.org/koji/taskinfo?taskID=3188339

Works!!

I also did a mock build for F14 x86_64 and confirmed the problem is fix. 

At this time your package is APPROVED.

Thank you for reviewing this one Richard!! :D

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

Git done (by process-git-requests).

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

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

aeskulap-0.2.2-0.5beta1.fc14 has been pushed to the Fedora 14 testing repository.

Built and pushed to repos for testing. Closing.

aeskulap-0.2.2-0.5beta1.fc15 has been pushed to the Fedora 15 stable repository.

aeskulap-0.2.2-0.5beta1.fc14 has been pushed to the Fedora 14 stable repository.