Bug 714328 (xmedcon) - Review Request: xmedcon - A medical image conversion utility and library
Summary: Review Request: xmedcon - A medical image conversion utility and library
Keywords:
Status: CLOSED ERRATA
Alias: xmedcon
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: nifticlib 714326 libtpcimgio
Blocks: amide
TreeView+ depends on / blocked
 
Reported: 2011-06-18 04:42 UTC by Ankur Sinha (FranciscoD)
Modified: 2011-09-30 19:36 UTC (History)
3 users (show)

Fixed In Version: xmedcon-0.10.7-4.fc16
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-08-10 17:49:02 UTC
Type: ---
tcallawa: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Ankur Sinha (FranciscoD) 2011-06-18 04:42:47 UTC
Spec URL: http://ankursinha.fedorapeople.org/xmedcon/xmedcon.spec
SRPM URL: http://ankursinha.fedorapeople.org/xmedcon/xmedcon-0.10.7-1.fc16.src.rpm

Description: 
This project stands for Medical Image Conversion and is released under the
GNU's (L)GPL license. It bundles the C sourcecode, a library, a flexible
command-line utility and a graphical front-end based on the amazing Gtk+
toolkit.

Its main purpose is image conversion while preserving valuable medical
study information. The currently supported formats are: Acr/Nema 2.0,
Analyze (SPM), Concorde/uPET, DICOM 3.0, CTI ECAT 6/7, InterFile 3.3
and PNG or Gif87a/89a towards desktop applications.

============================================================================

[ankur@ankur SRPMS]$ rpmlint /var/lib/mock/fedora-rawhide-i386/result/*.rpm ../SPECS/xmedcon.spec
xmedcon.i686: W: spelling-error %description -l en_US Acr -> Ac, Ar, Apr
xmedcon.i686: W: spelling-error %description -l en_US uPET -> u Pet, PET, u PET
xmedcon.i686: W: invalid-license BSD with attribution
xmedcon.i686: E: shlib-with-non-pic-code /usr/lib/libmdc.so.2.0.1
^^^^^

I need to look into this :/
Any clues will be helpful. 


xmedcon.i686: W: shared-lib-calls-exit /usr/lib/libmdc.so.2.0.1 exit@GLIBC_2.0
xmedcon.i686: W: devel-file-in-non-devel-package /usr/bin/xmedcon-config
xmedcon.i686: E: incorrect-fsf-address /usr/share/doc/xmedcon-0.10.7/COPYING
xmedcon.i686: E: incorrect-fsf-address /usr/share/doc/xmedcon-0.10.7/COPYING.LIB
xmedcon.src: W: spelling-error %description -l en_US Acr -> Ac, Ar, Apr
xmedcon.src: W: spelling-error %description -l en_US uPET -> u Pet, PET, u PET
xmedcon.src: W: invalid-license BSD with attribution
xmedcon-debuginfo.i686: W: invalid-license BSD with attribution
xmedcon-devel.i686: W: spelling-error %description -l en_US libmdc -> libido
xmedcon-devel.i686: W: invalid-license BSD with attribution
xmedcon-devel.i686: E: incorrect-fsf-address /usr/share/doc/xmedcon-devel-0.10.7/COPYING.LIB
xmedcon-devel.i686: E: incorrect-fsf-address /usr/share/doc/xmedcon-devel-0.10.7/COPYING
4 packages and 1 specfiles checked; 5 errors, 11 warnings.

Comment 1 Tom "spot" Callaway 2011-08-08 17:53:13 UTC
I'll take a pass at this. One obvious fix:

Instead of doing:

%ifarch x86_64
    sed -i \
           -e  "s|tpc_prefix/lib|tpc_prefix/lib64|" \
           -e  "s|nifti_prefix/lib|nifti_prefix/lib64|" configure
%endif

Just do:

sed -i \
       -e  "s|tpc_prefix/lib|tpc_prefix/%{_lib}|" \
       -e  "s|nifti_prefix/lib|nifti_prefix/%{_lib}|" configure

Also, I could not get this code to build at all, because of errors like this:

/usr/bin/ld: /usr/lib64/libtpcimgio.a(ecat7w.o): relocation R_X86_64_32 against `.rodata.str1.1' can not be used when making a shared object; recompile with -fPIC
/usr/lib64/libtpcimgio.a: could not read symbols: Bad value

Sure enough, the libtpcimgio and libtpcmisc packages only spits out non-PIC static libs. Which is fine if you never need to make a shared object from them later, but we definitely do in xmedcon.

Comment 2 Tom "spot" Callaway 2011-08-08 17:54:11 UTC
The obvious fix is to have libtpcimgio and libtpcmisc spit out proper PIC enabled shared libraries. I'm working on patches for you.

Comment 3 Tom "spot" Callaway 2011-08-08 18:28:52 UTC
I'm building updates for libtpcimgio and libtpcmisc now.

Here are some other items to resolve:

Here's the correct license tag:
License:        LGPLv2+ and Copyright only and MIT and BSD and libtiff

You will also need to remove rpath:

https://fedoraproject.org/wiki/Packaging/Guidelines#Removing_Rpath

I tested the libtool method and it worked fine.

Comment 4 Ankur Sinha (FranciscoD) 2011-08-08 18:39:47 UTC
Hi spot, 

That was *really really quick*. I thought I'd go work on generating shared
objects but I just got notification emails for your git pushes. Thank you! 

btw, I just rebuilt xmedcon in mock, and somehow xmedcon built just fine... I guess this is why I didn't think of generating the shared objects for libtpc* earlier. Not sure why it failed for you.. 

Regards,
Ankur

Comment 5 Tom "spot" Callaway 2011-08-08 18:42:22 UTC
Perhaps you only built for i386? It is more tolerant of PIC issues than x86_64.

Comment 6 Ankur Sinha (FranciscoD) 2011-08-08 18:44:23 UTC
Ah!

Aye, my default mock configuration is only for rawhide-i386. I'll do a scratch build and confirm this time. 

Thanks.
Ankur

Comment 7 Tom "spot" Callaway 2011-08-08 19:24:26 UTC
Okay, I've made updates for the fixed libtpcmisc and libtpcimgio. I had to delete your updates to work around a bodhi issue where it is possible that they would get tagged in the mash, not the newer ones (normally, my updates would just obsolete yours, but they hadn't been pushed to testing yet). I had it close out the respective Review Request bugs, so you probably already got bug spammed about it. :)

Show me a fixed spec file for xmedcon and I'll finish this review.

Comment 8 Ankur Sinha (FranciscoD) 2011-08-08 19:38:06 UTC
Hello,

I did indeed get the email(s). I'm just waiting on the new packages to hit the repos. I still run into the static error with libtpc* here because mock still finds the older package. As soon as I can build them correctly on x86_64, I shall submit a new spec. :)

Thanks!
Ankur

Comment 9 Ankur Sinha (FranciscoD) 2011-08-09 13:48:02 UTC
Hello,

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

http://ankursinha.fedorapeople.org/xmedcon/xmedcon-0.10.7-3.fc15.src.rpm

* Tue Aug 09 2011 Ankur Sinha <ankursinha AT fedoraproject DOT org> - 0.10.7-3
- Fix license tag
- remove rpath
- https://bugzilla.redhat.com/show_bug.cgi?id=714328#c3
- Fix sed


Thanks!
Ankur

Comment 10 Ankur Sinha (FranciscoD) 2011-08-09 13:51:13 UTC
http://koji.fedoraproject.org/koji/taskinfo?taskID=3261863

Comment 11 Tom "spot" Callaway 2011-08-09 15:34:33 UTC
== Review ==

Here are the must fix items:

* The xmedcon-config binary belongs in the -devel subpackage.
* The -devel package must require the main subpackage with %{?_isa}:
  Requires: %{name}%{?_isa} = %{version}-%{release}
  https://fedoraproject.org/wiki/Packaging/Guidelines#Requires
  This prevents mismatch in multilib scenarios.
* There must be a desktop file (and an icon) for xmedcon.
  https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files
  You might ask upstream for an icon, or use one of the generic icons for an 
  image tool.

Here are the optional fixes:

* You're using %defattr(-,root,root,-) in %files sections, but this is now the 
  default in all active Fedora branches. Consider removing it, although, this 
  is not a blocker.

== Details ==

- rpmlint checks return:
xmedcon.src: W: spelling-error %description -l en_US Acr -> Ac, Ar, Apr
xmedcon.src: W: spelling-error %description -l en_US uPET -> u Pet, PET, u PET
xmedcon.x86_64: W: spelling-error %description -l en_US Acr -> Ac, Ar, Apr
xmedcon.x86_64: W: spelling-error %description -l en_US uPET -> u Pet, PET, u PET
xmedcon-devel.x86_64: W: spelling-error %description -l en_US libmdc -> libido

All spelling errors safe to ignore.

xmedcon.x86_64: W: shared-lib-calls-exit /usr/lib64/libmdc.so.2.0.1 exit@GLIBC_2.2.5

Safe to ignore.

xmedcon.x86_64: W: devel-file-in-non-devel-package /usr/bin/xmedcon-config

The xmedcon-config binary belongs in the -devel subpackage, this is a must-fix.

xmedcon.x86_64: E: incorrect-fsf-address /usr/share/doc/xmedcon-0.10.7/COPYING.LIB
xmedcon.x86_64: E: incorrect-fsf-address /usr/share/doc/xmedcon-0.10.7/COPYING
xmedcon-devel.x86_64: E: incorrect-fsf-address /usr/share/doc/xmedcon-devel-0.10.7/COPYING.LIB
xmedcon-devel.x86_64: E: incorrect-fsf-address /usr/share/doc/xmedcon-devel-0.10.7/COPYING

Please inform the xmedcon upstream that they are using an outdated copy of the FSF license texts with the old FSF address, and ask them to please update this in their next release.

- package meets naming guidelines
- package meets packaging guidelines
- license (LGPLv2+ and Copyright only and MIT and BSD and libtiff) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream:
bc76d1edbe8e65bbea8afeca8a1d44a7d5e286a1befb5d42a743d1bfc6fe5016
- package compiles on devel (koji scratch OK)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- desktop file missing
- devel package ok (except for misplaced xmedcon-config binary)
- no .la files
- post/postun ldconfig ok
- devel requires base package n-v-r, but is missing %{_isa}

Comment 12 Ankur Sinha (FranciscoD) 2011-08-09 18:56:59 UTC
Hi spot,

Updated spec/srpm with issues corrected are here:

http://ankursinha.fedorapeople.org/xmedcon/xmedcon.spec
http://ankursinha.fedorapeople.org/xmedcon/xmedcon-0.10.7-4.fc15.src.rpm


* Tue Aug 09 2011 Ankur Sinha <ankursinha AT fedoraproject DOT org> - 0.10.7-4
- Move xmedcon-config to -devel
- Correct requires for -devel
- Add icon, scriptlets
- Add desktop file, scriptlets
- Add a xmedconrc.linux file in docs
- Remove defattr

Thank you,
Ankur

Comment 13 Tom "spot" Callaway 2011-08-10 14:47:28 UTC
Nice work. Approved.

Comment 14 Ankur Sinha (FranciscoD) 2011-08-10 14:57:30 UTC
New Package SCM Request
=======================
Package Name: xmedcon
Short Description: A medical image conversion utility and library
Owners: ankursinha
Branches: f14 f15 f16 
InitialCC: susmit mrceresa

Comment 15 Gwyn Ciesla 2011-08-10 15:13:20 UTC
Git done (by process-git-requests).


Please take ownership of review BZs.  Thanks!

Comment 16 Fedora Update System 2011-08-10 17:27:51 UTC
xmedcon-0.10.7-4.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/xmedcon-0.10.7-4.fc15

Comment 17 Fedora Update System 2011-08-10 17:27:59 UTC
xmedcon-0.10.7-4.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/xmedcon-0.10.7-4.fc14

Comment 18 Fedora Update System 2011-08-10 17:28:07 UTC
xmedcon-0.10.7-4.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/xmedcon-0.10.7-4.fc16

Comment 19 Ankur Sinha (FranciscoD) 2011-08-10 17:49:02 UTC
Hello, 

Build for f14,15,16 and rawhide. Closing.

Thank you for the review spot :)

Ankur

Comment 20 Fedora Update System 2011-08-31 01:30:54 UTC
xmedcon-0.10.7-4.fc14 has been pushed to the Fedora 14 stable repository.

Comment 21 Fedora Update System 2011-08-31 01:32:00 UTC
xmedcon-0.10.7-4.fc15 has been pushed to the Fedora 15 stable repository.

Comment 22 Fedora Update System 2011-09-30 19:36:40 UTC
xmedcon-0.10.7-4.fc16 has been pushed to the Fedora 16 stable repository.


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