Bug 714328 (xmedcon)

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.

The obvious fix is to have libtpcimgio and libtpcmisc spit out proper PIC enabled shared libraries. I'm working on patches for you.

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.

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

Perhaps you only built for i386? It is more tolerant of PIC issues than x86_64.

Ah!

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

Thanks.
Ankur

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.

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

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

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

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

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

Nice work. Approved.

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

Git done (by process-git-requests).


Please take ownership of review BZs.  Thanks!

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

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

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

Hello, 

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

Thank you for the review spot :)

Ankur

xmedcon-0.10.7-4.fc14 has been pushed to the Fedora 14 stable repository.

xmedcon-0.10.7-4.fc15 has been pushed to the Fedora 15 stable repository.

xmedcon-0.10.7-4.fc16 has been pushed to the Fedora 16 stable repository.