Bug 539387 (InsightToolkit)

Summary: Review Request: InsightToolkit - Medical imaging processing library
Product: [Fedora] Fedora Reporter: Mario Ceresa <mrceresa>
Component: Package ReviewAssignee: Peter Lemenkov <lemenkov>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: adam, arnaud_gelas, bloch, dvratil, fedora-package-review, hobbes1069, lemenkov, michel, mrceresa, notting, panemade, sanjay.ankur, sigmund.augdal, taylor, volker27
Target Milestone: ---Flags: lemenkov: fedora‑review+
limburgher: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-05-02 00:31:37 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On: 566725, 567086    
Bug Blocks: 673841, 691774, 721112, 570224, 720121, 726201    
Attachments:
Description Flags
Updated spec file for version 3.20 with fixes for lib/lib64 issue none

Description Mario Ceresa 2009-11-19 18:56:10 EST
Spec URL: http://www.4shared.com/file/155554417/4e7f103a/InsightToolkit.html
SRPM URL: http://www.4shared.com/file/155554414/d7764180/InsightToolkit-3160-1fc12src.html

Description: 
ITK is an open-source software toolkit for performing registration and 
segmentation. Segmentation is the process of identifying and classifying data
found in a digitally sampled representation. Typically the sampled
representation is an image acquired from such medical instrumentation as CT or
MRI scanners. Registration is the task of aligning or developing 
correspondences between data. For example, in the medical environment, a CT
scan may be aligned with a MRI scan in order to combine the information
contained in both.

I've been using the library for a while and I thought I might contribute this back to the Fedora community.

I need a sponsor because this is my first package!

These are rpmlint's outputs:
$ rpmlint InsightToolkit.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint InsightToolkit-3.16.0-1.fc12.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint InsightToolkit-3.16.0-1.fc12.i686.rpm
InsightToolkit.i686: W: incoherent-version-in-changelog InsightToolkit-3.16.0-1.fc12 ['3.16.0-1.fc12', '3.16.0-1']
InsightToolkit.i686: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

$ rpmlint InsightToolkit-devel-3.16.0-1.fc12.i686.rpm
InsightToolkit-devel.i686: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Builds ok with:
mock -r fedora-12-i386 rebuild InsightToolkit-3.16.0-1.fc12.src.rpm
Comment 1 Mario Ceresa 2009-11-24 09:04:39 EST
Hello! I hope not to bother: I'd only like to say that I submitted the second ITK related package at:

https://bugzilla.redhat.com/show_bug.cgi?id=540885

and that developers and users from upstream are waiting too for the packages to be reviewed:

http://www.itk.org/pipermail/insight-users/2009-November/034011.html
http://www.itk.org/pipermail/insight-users/2009-November/034053.html

Is there anything I could do to contact someone who might want to review the packages?

Thanks and regards,

Mario
Comment 2 Mario Ceresa 2009-12-01 05:32:38 EST
Following Mamoru comments on bug 540885, I looked at fedora people for an alternative way to store the specs and srpms, but it seems I'd need to be already sponsored in order to get the space. 

A friend of mine offered to host them for a while at:

ftp://85.54.198.199/FEDORA/InsightToolkit-3.16.0-1.fc12.src.rpm
ftp://85.54.198.199/FEDORA/InsightToolkit.spec

Hope this could ease the review process :)

Mario
Comment 3 Peter Lemenkov 2009-12-01 06:15:18 EST
Removing FE-NEEDSPONSOR - I just sponsored Mario
Comment 4 Peter Lemenkov 2009-12-01 06:16:28 EST
I'll review it.
Comment 5 Peter Lemenkov 2009-12-01 09:53:09 EST
Notes:

* I'm not sure about name. Perhaps 'itk' would be better name than InsightToolkit?
* No need to re-define %{name} and %{version}. Just put correct values to proper fields (Name: and Version:) at the top of the spec.
* Source0 url should be corrected. See Source1 for example.
* %{_libdir}/%{name}/*.cmake should be placed in devel rather than in main package, I believe. Also, I'm not sure this is a correct place to put CMake files in. 
* Unowned directory - %{_libdir}/%{name} . Just list it as %dir in main package's %files section.
* Source1 should be added as %doc in devel-subpackage
* In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
* Files within 'Copyrght' foler must be packages as %doc.
* Consider, also, packaging of 'Docmentation' and 'Examples' folders.

Please, comment my notes, and I'll continue.

BTW, since I sponsored you, you can store files at fedorapeople.org
Comment 6 Peter Lemenkov 2009-12-01 10:00:44 EST
* I just found, that ITK contains numerous bundled libraries, many of them are duplication Fedora's system ones - see 'Utilities' directory. This should be fixed (and necessary BuildRequires should be added).

* Also I'm anxious about the contents of 'Code/Patented' folder.
Comment 7 Mario Ceresa 2009-12-02 11:07:32 EST
Hello Peter!
thanks for your comments. 

* I'm not sure about name. Perhaps 'itk' would be better name than
InsightToolkit?

Yes, but package itk already exists and is an object oriented extensions to Tk. Maybe I should use libitk?

* I just found, that ITK contains numerous bundled libraries, many of them are
duplication Fedora's system ones - see 'Utilities' directory. This should be
fixed (and necessary BuildRequires should be added).

You're right. Shame on me! I'll fix it.

* Also I'm anxious about the contents of 'Code/Patented' folder. 

The Patented code is optional for ITK and is actually disabled by default. However, I'll add the -DITK_USE_PATENTED:BOOL=OFF cmake flag to the specs to be sure that is never included in the packages. Do you want the folder to be erased in the %prep or %build part?

I'm a bit busy and can't make all the requested changes right now, but I'll try to address all of your notes and make a new package later this week. 

BTW I activated my brand new fedora people account (how cool! :) ) and republished there the srpms and specs. 

http://mrceresa.fedorapeople.org/InsightToolkit.spec
http://mrceresa.fedorapeople.org/InsightToolkit-3.16.0-1.fc12.src.rpm

Beware that I simply copied there the old ones! I'll post a message when the new version is available.

Cheers,

Mario
Comment 8 Mario Ceresa 2009-12-06 10:24:45 EST
Hello Peter, 
Here we are. I have some questions, sorry if they are stupid, but I'm not very familiar with spec files:

* No need to re-define %{name} and %{version}. Just put correct values to
proper fields (Name: and Version:) at the top of the spec.

Fixed

* Source0 url should be corrected. See Source1 for example

I'm not quite sure if I get this. Do you want me to remove %name and %version from Source0 and write directly http://voxel.dl.sourceforge.net/sourceforge/itk/InsightToolkit-3.16.0.tar.gz ?

* %{_libdir}/%{name}/*.cmake should be placed in devel rather than in main
package, I believe. Also, I'm not sure this is a correct place to put CMake
files in.

Moved to the devel section. Neither I am sure about their correct position. Maybe they should be put in cmake config dir?

* Unowned directory - %{_libdir}/%{name} . Just list it as %dir in main
package's %files section.

Fixed

* Source1 should be added as %doc in devel-subpackage

First copied to builddir in %prep and then added as %doc. Not sure if this is the correct way

* In the vast majority of cases, devel packages must require the base package
using a fully versioned dependency: Requires: %{name} = %{version}-%{release}

Fixed

* Files within 'Copyrght' foler must be packages as %doc.

Fixed

* Consider, also, packaging of 'Docmentation' and 'Examples' folders.

Added doc and examples sub-packages

* I just found, that ITK contains numerous bundled libraries, many of them are
duplication Fedora's system ones - see 'Utilities' directory. This should be
fixed (and necessary BuildRequires should be added).

Fixed adding -DUSE_SYSTEM_* to cmake flags. Anyway I didn't find any package for niether GDCM nor VXL.
Should I package them as well or can I leave the itk versions for now?

* Also I'm anxious about the contents of 'Code/Patented' folder.  

Fixed.

After asking to the developers, we agreed to remove both Patented and Review:

http://www.itk.org/pipermail/insight-users/2009-December/034323.html

$ rpmlint SPECS/InsightToolkit.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings

$ rpmlint SRPMS/InsightToolkit-3.16.0-2.fc12.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint RPMS/i686/InsightToolkit-devel-3.16.0-2.fc12.i686.rpm
InsightToolkit-devel.i686: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

$ rpmlint RPMS/i686/InsightToolkit-doc-3.16.0-2.fc12.i686.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

I have a lot of warnings for:
$ rpmlint RPMS/i686/InsightToolkit-examples-3.16.0-2.fc12.i686.rpm
Mostly W: devel-file-in-non-devel-package and W: spurious-executable-perm 
But actually the C++ examples are devel files so I'm not sure if I should package here or in devel
Also some error in the form of E: script-without-shebang but I thought that might wait until we decide if to package them into the example or the devel package.

$ mock -r fedora-12-i386 rebuild InsightToolkit-3.16.0-2.fc12.src.rpm
INFO: Done(SRPMS/InsightToolkit-3.16.0-2.fc12.src.rpm) Config(fedora-12-i386) 15 minutes 19 seconds

but:
$ koji build --scratch dist-f12 InsightToolkit-3.16.0-2.fc12.src.rpm

exited with errors, but I don't understand why:
https://koji.fedoraproject.org/koji/taskinfo?taskID=1852520

The new spec file and srpms are available in my brand new fedorapeople space! (Thanks Peter for that, it really works like a charm!)

http://mrceresa.fedorapeople.org/InsightToolkit.spec
http://mrceresa.fedorapeople.org/InsightToolkit-3.16.0-2.fc12.src.rpm

I'm looking forward to hearing from you soon,

Mario
Comment 9 Peter Lemenkov 2009-12-06 11:07:04 EST
Great! I'll review your new srpm tomorrow.
Comment 10 Peter Lemenkov 2009-12-27 13:05:49 EST
Ok, finally found some time to continue reviewing this.
Comment 11 Peter Lemenkov 2009-12-27 13:27:23 EST
(In reply to comment #8)
 
> * Source0 url should be corrected. See Source1 for example
> 
> I'm not quite sure if I get this. Do you want me to remove %name and %version
> from Source0 and write directly
> http://voxel.dl.sourceforge.net/sourceforge/itk/InsightToolkit-3.16.0.tar.gz ?

For files, stored at SF, you should use something like this:

Source0: http://downloads.sourceforge.net/itk/%{name}-%{version}.tar.gz

Note, that this URL doesn't have pre-defined mirror site (like yours one have). 

> * %{_libdir}/%{name}/*.cmake should be placed in devel rather than in main
> package, I believe. Also, I'm not sure this is a correct place to put CMake
> files in.
> 
> Moved to the devel section. Neither I am sure about their correct position.
> Maybe they should be put in cmake config dir?

I suppose so.

> * Source1 should be added as %doc in devel-subpackage
> 
> First copied to builddir in %prep and then added as %doc. Not sure if this is
> the correct way

Not so correct. You should simply copy %{SOURCE1} instead of using full path:

- cp %{_builddir}/../SOURCES/ItkSoftwareGuide-2.4.0.pdf  .
+ cp %{SOURCE1} .

> * I just found, that ITK contains numerous bundled libraries, many of them are
> duplication Fedora's system ones - see 'Utilities' directory. This should be
> fixed (and necessary BuildRequires should be added).
> 
> Fixed adding -DUSE_SYSTEM_* to cmake flags. Anyway I didn't find any package
> for niether GDCM nor VXL.

To be really sure, you need to remove these directories with duplicated system libraries, at a %prep stage. And properly patch the rest of the code, if something will go wrong.

> Should I package them as well or can I leave the itk versions for now?

Ideally, yes. But it's not *MUST*, it's a *SHOULD* rule. So we may simply use them as-is for now.

I'm looking at at your new spec right now, so more notes to come.
Comment 12 Peter Lemenkov 2009-12-27 14:40:36 EST
Ok, here are new issues:

* Unowned directory %{_datadir}/%{name}. I suggest you either to add it to the main sub-package, or to put examples into docdir.

* doc sub-package mustn't own %{_docdir}/%{name}-%{version} - it's already owned by main sub-package.
Comment 13 Per Inge Mathisen 2011-03-16 09:26:24 EDT
Created attachment 485731 [details]
Updated spec file for version 3.20 with fixes for lib/lib64 issue

I took the latest spec file posted in this ticket and updated it to work against version 3.20, and worked around a showstopper issue related to it putting 64bit libraries in /usr/lib for install but expecting them in /usr/lib64 for packaging. The workaround might be ugly but at least I got an RPM out of it.
Comment 14 Arnaud Gelas 2011-03-16 13:26:43 EDT
When you set cmake variable, you set twice BUILD_EXAMPLES once to ON, once to OFF.
I guess you should either set it to ON or OFF.

[..]
       -DBUILD_EXAMPLES:BOOL=ON \
       -DCMAKE_BUILD_TYPE:STRING="Release"\
       -DCMAKE_VERBOSE_MAKEFILE=OFF\
       -DBUILD_EXAMPLES=OFF\
       -DBUILD_TESTING=OFF\
       -DITK_USE_REVIEW:BOOL=OFF \
       -DITK_USE_PATENTED:BOOL=OFF \
       -DITK_USE_SYSTEM_TIFF=ON \
       -DITK_USE_SYSTEM_PNG=ON \
       -DITK_USE_SYSTEM_ZLIB=ON \
       -DITK_USE_SYSTEM_LIBXML2=ON \
       -DUSE_FFTWD=ON  .
[...]
Comment 15 Mario Ceresa 2011-03-20 07:19:55 EDT
Hello Pet, hello Arnaud: thanks for your help with itk!

The newest spec and srpms are always on: http://mrceresa.fedorapeople.org/ and they include the last efforts in packaging such a big library as itk.

You can build and use those packages, but be warned that they do not met the official fedora packaging guidelines yet, mainly because there are still some bundled  libraries.

Unfortunately for the same reason, this revision is blocked until we package all the remaining dependent libraries that we identified so far:

- dcmtk - https://bugzilla.redhat.com/show_bug.cgi?id=573910
- vxl - https://bugzilla.redhat.com/show_bug.cgi?id=567086

We've already packaged: gdcm, CharLS, CableSwig and rply

If you like medical software and are interested in helping with itk, vxl, dcmtk or with some of the ones listed here (https://fedoraproject.org/wiki/Medical_Imaging) you are *more then* welcome! :)

Thanks and best regards,

Mario
Comment 16 Mario Ceresa 2011-05-28 15:30:35 EDT
Here it goes the new version:

http://mrceresa.fedorapeople.org/InsightToolkit-3.20.0-5.fc15.src.rpm
http://mrceresa.fedorapeople.org/InsightToolkit.spec

This fixes a problem with gcc 4.6 in f15

Mario
Comment 17 Mario Ceresa 2011-05-31 08:05:54 EDT
New version where I tested build & install with last vxl rpm

http://mrceresa.fedorapeople.org/InsightToolkit-3.20.0-5.fc15.src.rpm
http://mrceresa.fedorapeople.org/InsightToolkit.spec
Comment 18 Ankur Sinha (FranciscoD) 2011-07-09 02:09:46 EDT
Hey Peter, 

I'd like to take over this review as well, since I'm working only on fedora-medical related packages. 

I've mistakenly set the review flag to myself in haste. You had asked me to take over the vxl review too recently, but I really should've asked you first. Sorry :/

If you'd like to continue the review, please reset it to yourself. 

I haven't reassigned it to myself. I'll do that once I have your permission. 

Thanks,
Ankur
Comment 19 Peter Lemenkov 2011-07-09 02:18:02 EDT
(In reply to comment #18)

> I'd like to take over this review as well, since I'm working only on
> fedora-medical related packages. 

Good! Just go on with that :)

> I've mistakenly set the review flag to myself in haste. You had asked me to
> take over the vxl review too recently, but I really should've asked you first.

No, it's ok - proceed further. I reaaly don't have much time to spend on all these packages now.
Comment 20 Richard Shaw 2011-08-15 09:26:30 EDT
Ankur,

I was re-reading this review request and noticed it appears that you took over responsibility for the review request so I assume you now need a reviewer?

Let me know and I'll take it.

Richard
Comment 21 Ankur Sinha (FranciscoD) 2011-08-15 13:52:17 EDT
Richard,

Mario is the submitter, and I'm the reviewer here ;)

This one is also waiting for vxl.

Thanks,
Ankur
Comment 22 Richard Shaw 2011-08-15 14:06:08 EDT
You might want to get him to take it then, it's still showing as assigned to you.

Richard
Comment 23 Volker Fröhlich 2011-12-13 14:42:46 EST
Any progress here? Can I help out somehow? I'm interested to see this package in Fedora.
Comment 24 Mario Ceresa 2011-12-14 03:44:17 EST
Hello Volker: thanks for your interest. Would you like to help with packaging of VXL? This is one of the main dependency of ITK and honestly I have no spare time to devote on it in this period...

You can always find the last srpms on:

http://mrceresa.fedorapeople.org/

Best,

Mario
Comment 25 Mario Ceresa 2012-02-01 06:28:48 EST
Hi everybody! please see this post on the current status of ITK dependencies:

https://fedorahosted.org/pipermail/medical-sig/2012-February/000240.html

Best,

Mario
Comment 26 Michel Alexandre Salim 2012-05-31 11:33:15 EDT
Hi all,

I've just started using Ginkgo CADx, which depends on ITK, so I'll look around when possible and try and help push this forward.

Given the protracted issues, perhaps we should do what the OCaml and Erlang packagers were discussing on fedora-devel, and set up a Git repository for tracking specs and patches? It'd probably help get everyone get up to speed if they can see how issues are fixed.

-- 
Michel
Comment 27 Volker Fröhlich 2012-05-31 17:11:37 EDT
That could be useful. I'm still interested in having this package in Fedora and did some minor work on VXL, which I still didn't publish.
Comment 28 Peter Lemenkov 2012-10-08 08:36:21 EDT
Ok, It seems that we need to restart reviewing it. Mario?
Comment 29 Mario Ceresa 2012-10-09 06:35:37 EDT
The same here as for vxl, the repository is at:
http://repos.fedorapeople.org/repos/mrceresa/cil/

but for now I'm fixing VXL on f17-f18

I'll also have a git repository here:
https://github.com/mrceresa/ITK/tree/v3.20.1_fc15

where we can collaborate on the patches.
Comment 30 Mario Ceresa 2012-11-24 08:24:54 EST
(In reply to comment #26)
> Hi all,
> 
> I've just started using Ginkgo CADx, which depends on ITK, so I'll look
> around when possible and try and help push this forward.
> 
> Given the protracted issues, perhaps we should do what the OCaml and Erlang
> packagers were discussing on fedora-devel, and set up a Git repository for
> tracking specs and patches? It'd probably help get everyone get up to speed
> if they can see how issues are fixed.
> 
> -- 
> Michel

With vxl approved (did I say YHUHUHUHU! ?) now it's time to work again on it.
ITK is now at version 4.2 - 4.3 so I'll update the source accordingly.

As Michel suggested here there is the git repo for medical image related packages.

https://github.com/mrceresa/fedora_medical
Comment 31 Mario Ceresa 2012-11-24 10:26:05 EST
Uploading new packages for 4.2.1 (both src and binaries) to:

http://repos.fedorapeople.org/repos/mrceresa/cil/fedora-17/
Comment 32 Mario Ceresa 2012-11-24 10:29:57 EST
Uploading new packages for 4.2.1 (both src and binaries) to:

http://repos.fedorapeople.org/repos/mrceresa/cil/fedora-17/
Comment 33 Peter Lemenkov 2012-12-04 06:10:21 EST
FTBFS in Rawhide:

* http://koji.fedoraproject.org/koji/taskinfo?taskID=4754775
Comment 34 Mario Ceresa 2012-12-04 16:02:40 EST
Built a new version with a vxl patch. There is still an error (http://kojipkgs.fedoraproject.org//work/tasks/6820/4756820/root.log):

DEBUG util.py:257:  Error: Package: vtk-5.10.1-1.fc19.i686 (build)
DEBUG util.py:257:             Requires: hdf5 = 1.8.9
DEBUG util.py:257:             Installing: hdf5-1.8.10-1.fc19.i686 (build)
DEBUG util.py:257:                 hdf5 = 1.8.10-1.fc19
DEBUG util.py:257:   You could try using --skip-broken to work around the problem
DEBUG util.py:257:   You could try running: rpm -Va --nofiles --nodigest

Not sure what it is that is broken.

Builds locally in mock with f17. Testing with koji for f17-f18
Comment 35 Richard Shaw 2012-12-04 16:44:34 EST
Wasn't there a hdf5 update recently? Perhaps this is fallout from it working through the system? Looks like vtk needs to be rebuilt for the new hdf5.
Comment 36 Mario Ceresa 2012-12-06 11:32:45 EST
Indeed! It works now:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4763503
Comment 37 Peter Lemenkov 2012-12-10 13:30:55 EST
*** Bug 885825 has been marked as a duplicate of this bug. ***
Comment 38 Mario Ceresa 2013-01-11 16:01:53 EST
Okay, new year has started so let's try to nail this down...

I'll use fedora-review to spot the problems that we still needs to solve. For now just let's repost the last links:
SPEC: https://raw.github.com/mrceresa/fedora_medical/master/ITK/InsightToolkit.spec
SRPMS: http://mrceresa.fedorapeople.org/InsightToolkit-4.2.1-4.fc17.src.rpm
Comment 39 Daniel Vrátil 2013-01-13 18:48:54 EST
Hi,

a few suggestions from fedora-review:

 - remove gcc-c++ from BR (part of minimum build environment)
 - tabs-vs-spaces on several places

and several suggestions from me:

 - improve %description of subpackages. Using "%{summary}." (note the period) 
   is usually OK
 - if you use DITK_USE_SYSTEM_TIFF=OFF, remove libtiff-devel from BR (and
   explain why you can't use system libtiff). But better try to find out if you
   can enable ITK_USE_SYSTEM_TIFF and use Fedora's libtiff
 - consider cleaning up the commented lines, especially in %files (they make
   the spec harder to read)
 - you don't need to use %dir to create a folder before installing files
   into it. AFAIK %dir is supposed to be used to create an empty folder that the
   program might expect to exist during runtime (and would fail if they would
   not exist)
 - you should install LICENSE, README.txt, NOTICE and COPYRIGHT in the
   regular package (using %doc macro) and install only the README.html and the
   PDF in the -doc subpackage
 - don't list README.html and the PDF in %doc of -devel subpackage if you
   ship them in the -doc subpackage. Either remove the %doc macro from -devel 
   or drop the -doc subpackage
 - consider installing the PDF to the same folder as LICENSE and README (this 
   applies only if you decide to keep the -doc subpackage)
 - the package should own %{_libdir}/%{name} and %{_datadir}/%{name} folders
Comment 40 Mario Ceresa 2013-01-14 04:50:08 EST
Hello Dan,
Thanks for your comments! 

I'll try and implement them when I came back home from work. 

Mario
Comment 41 Mario Ceresa 2013-01-25 19:51:12 EST
New spec and sources (uploading, should take half an hour or so...):

SPEC: https://raw.github.com/mrceresa/fedora_medical/master/ITK/InsightToolkit.spec
SRPMS: http://mrceresa.fedorapeople.org/InsightToolkit-4.3.1-1.fc18.src.rpm
Comment 42 Mario Ceresa 2013-01-26 10:02:22 EST
Rawhide was in a bad shape so I rebuilt for f18:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4904450
Comment 43 Volker Fröhlich 2013-01-27 06:28:35 EST
A few comments:

Please move the %package and %description blocks to the top and away from their %files blocks. "Name:" is commonly put before "Summary" as well.

Use the name macro in Source0.

The vendor tag is not used anymore.

Patch names should be %{name}...
Please include links to the corresponding bug reports or the source of the patches.

Are you going for EPEL 5? That'd require vxl there. If not, remove clean section, buildroot definition and the rm -rf in the install section.

Remove the version constraint for cmake. Every target, including EPEL 5, has a new enough version.

You should not need to BR libjpeg-turbo-devel. libjpeg-devel should be enough.

Preserve the timestamp when installing SOURCE1. Couldn't you just install it with %doc right away?

I'm not sure what "Patented" is, but non-free content should be removed from the tarball in the first place.

Use the ?_isa macro, see http://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

At least the doc sub-package should be noarch. If you go for EPEL 5, this has to be conditional.
Comment 44 Mario Ceresa 2013-02-12 15:43:45 EST
Hello,
here there is the new version (uploading):

SPEC: https://raw.github.com/mrceresa/fedora_medical/master/ITK/InsightToolkit.spec
SRPMS: http://mrceresa.fedorapeople.org/InsightToolkit-4.3.1-2.fc18.src.rpm

@Volker: how do I "install SOURCE1 with %doc right away"?

I don't see "Patented" anymore in this version so I dropped the line. Indeed upstream deeply re-organized the sources since v3.20

As for the patches, I'm already in contact with upstream to see if we can work out a solution to manually change all VXL related calls (http://www.itk.org/pipermail/insight-users/2013-January/047026.html)
Comment 45 Volker Fröhlich 2013-03-17 04:52:52 EDT
I think it's fine the way you did it, i. e. copying it in in the prep section. I'm not sure what I meant there.
Comment 46 sigmund.augdal 2013-04-08 05:09:11 EDT
Hi,

I've tried using the above referenced vxl and insightToolkit packages, and while they do compile and install properly, it is still not possible to use them to compile some applications using InsightToolkit since there are still several vnl_math_* references in template functions that does not get built at package compile time.
Comment 47 sigmund.augdal 2013-04-08 05:10:51 EDT
particularly these files are affected for me: itkOtsuThresholdCalculator.hxx and itkConnectedComponentImageFilter.hxx
Comment 48 Mario Ceresa 2013-04-09 03:40:35 EDT
Hello Sigmund! 

Upstream released some compatibility patches for vxl that I incorporated early this month. I have a new release of ITK which builds fine against them and I'll upload shortly, but you'll need to use f19-rawhide to get the new vxl. 

Mario
Comment 49 sigmund.augdal 2013-04-09 03:49:50 EDT
I found the above mentioned patches in the fedora_medical repo linked to above, however the spec file there does not actually apply them. I managed to patch it to do so and then everything works. Thanks for your effort
Comment 50 Mario Ceresa 2013-04-16 06:29:16 EDT
And here we have a new release:

SPEC: https://raw.github.com/mrceresa/fedora_medical/master/ITK/InsightToolkit.spec
SRPM: http://mrceresa.fedorapeople.org/InsightToolkit-4.3.1-5.fc18.src.rpm

This release:

* Fixes problems with vxl compatibility
* Builds all testing and passes them all on f18. On rawhide there are a three which are failing (over more than 2000) and I'm investigating why
* Builds all documentation and includes in a separate package
* Fixes a long standing problem with SYSTEM_TIFF

Please remember that, until new vxl hits f18 stable you'll have to install from updates-testing of f19/rawhide to get the compatibility patches.

TODO for next releases:
* Fix failing tests in rawhide
* Enable examples and create a new subpackage
* Build documentation
* Enable python wrapping

Best,

Mario
Comment 51 Mario Ceresa 2013-04-19 07:26:55 EDT
SPEC: https://raw.github.com/mrceresa/fedora_medical/master/ITK/InsightToolkit.spec
SRPM: http://mrceresa.fedorapeople.org/InsightToolkit-4.3.1-6.fc18.src.rpm

* Fixes more problems vxl compatibility problems

Koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5273906

I really don't understand why I have those exception in tests. In my machine I cannot reproduce them and I don't know how to debug...
Comment 52 Mario Ceresa 2013-04-22 11:27:45 EDT
SPEC: https://raw.github.com/mrceresa/fedora_medical/master/ITK/InsightToolkit.spec
SRPM: http://mrceresa.fedorapeople.org/InsightToolkit-4.3.1-8.fc18.src.rpm

* Build all examples
* Made tests only informative for now while we debug this with upstream
* Made an initial test with python bindings but I hit bug http://www.gccxml.org/Bug/view.php?id=13372 Disabled until fixed.
Comment 53 Mario Ceresa 2013-04-23 05:56:43 EDT
Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5288701
Comment 54 Peter Lemenkov 2013-04-23 08:39:59 EDT
Ok, I really think we should approve it in the current state. We can spend a lot of time polishing it further, but the truth is that this package is already in a pretty good shape but still not available in Fedora (thus blocking other packages from inclusion). So here is my 


REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

+ rpmlint is not silent but all its messages may be safely omitted:

work ~: rpmlint Desktop/InsightToolkit-* | grep -v devel-file-in-non-devel-package
InsightToolkit.src: W: spelling-error %description -l en_US templated -> templates, template, template d
InsightToolkit.src: W: spelling-error %description -l en_US templating -> contemplating, template, tempting
InsightToolkit.src:85: W: macro-in-comment %package
InsightToolkit.src:90: W: macro-in-comment %description
InsightToolkit.src:91: W: macro-in-comment %{summary}
InsightToolkit.x86_64: W: spelling-error %description -l en_US templated -> templates, template, template d
InsightToolkit.x86_64: W: spelling-error %description -l en_US templating -> contemplating, template, tempting
InsightToolkit-devel.x86_64: W: only-non-binary-in-usr-lib
InsightToolkit-devel.x86_64: W: no-documentation
InsightToolkit-examples.x86_64: W: no-documentation
InsightToolkit-examples.x86_64: W: no-manual-page-for-binary itkTestDriver
6 packages and 0 specfiles checked; 0 errors, 304 warnings.
work ~: 

I removed a lot of warnings triggered by example C++ files.

+ The package is named according to the  Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.

- The License field in the package spec file MUST match the actual license (ASL 2.0). Please fix it.

+ The file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package, match the upstream source, as provided in the spec URL.

work ~/Desktop: sha256sum InsightToolkit-4.3.1.tar.gz*
85b8f2a43400f8913198df09b3df2670a015911ed912960f86e808a4ac368cbd  InsightToolkit-4.3.1.tar.gz
85b8f2a43400f8913198df09b3df2670a015911ed912960f86e808a4ac368cbd  InsightToolkit-4.3.1.tar.gz.1
work ~/Desktop: 

+ The package successfully compiles and builds into binary rpms on at least one primary architecture. See koji link above.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
+ The package calls ldconfig in %post and %postun. 
+ The package does bundle copies of system libraries in the directory Modules/ThirdParty but it doesn't use them durign build stage if systemd-wide copies are available (see -DITK_USE_SYSTEM_SOMETHING switches in the spec-file).
0 The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
0 The package DOESN'T have a %clean section, so it won't build cleanly on systems with old rpm (EL-4 and EL-5). Beware.
+ The package consistently uses macros.
+ The package contains code, or permissible content.
+ Large documentation files stired in a *-doc subpackage.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
+ Header files are stored in a -devel package (except example files which are stired in *-examples).
0 No static libraries.
0 No pkgconfig(.pc) files.
+ The library file(s) that end in .so (without suffix) is(are) stored in a -devel package.
+ The -devel package requires the base package using a fully versioned dependency: Requires: %{name}%{?_isa} = %{version}-%{release}
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other packages.
+ All filenames in rpm packages are valid UTF-8.


OK, the only issue I see so far is wrong License tag. Please correct it before uploading package. I don't see any other blocking issues so this package is


APPROVED.
Comment 55 Mario Ceresa 2013-04-23 10:20:43 EDT
New Package SCM Request
=======================
Package Name: InsightToolkit
Short Description: Medical imaging processing library
Owners: mrceresa
Branches: f18 f19 
InitialCC: peter
Comment 56 Jon Ciesla 2013-04-23 10:26:04 EDT
Git done (by process-git-requests).
Comment 57 Fedora Update System 2013-04-23 17:40:31 EDT
InsightToolkit-4.3.1-9.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/InsightToolkit-4.3.1-9.fc19
Comment 58 Fedora Update System 2013-04-23 17:41:52 EDT
InsightToolkit-4.3.1-9.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/InsightToolkit-4.3.1-9.fc18
Comment 59 Fedora Update System 2013-04-24 12:42:02 EDT
InsightToolkit-4.3.1-9.fc19 has been pushed to the Fedora 19 testing repository.
Comment 60 Fedora Update System 2013-04-27 13:19:56 EDT
InsightToolkit-4.3.1-10.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/InsightToolkit-4.3.1-10.fc19
Comment 61 Fedora Update System 2013-04-27 13:23:41 EDT
InsightToolkit-4.3.1-10.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/InsightToolkit-4.3.1-10.fc18
Comment 62 Fedora Update System 2013-05-02 00:31:43 EDT
InsightToolkit-4.3.1-10.fc19 has been pushed to the Fedora 19 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 63 Fedora Update System 2013-05-05 23:52:04 EDT
InsightToolkit-4.3.1-10.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.