SyneRBI / SIRF-SuperBuild

SIRF CMake SuperBuild
http://www.ccpsynerbi.ac.uk
Apache License 2.0
15 stars 17 forks source link

Older version of STIR added to include path #414

Open ALEXJAZZ008008 opened 4 years ago

ALEXJAZZ008008 commented 4 years ago

If I select for STIR to be built with ITK I get the following error when trying to use stir_math with ITK:

FactoryRegistry: key ITK not found in current registry
2 possible values are:
Interfile
None

Using value corresponding to key "None"

WARNING: KeyParser warning: unrecognized keyword: itk output file format parameters

WARNING: KeyParser warning: unrecognized keyword: default extension

WARNING: KeyParser warning: unrecognized keyword: end itk output file format parameters

Error parsing output file format from /home/alex/Documents/jrmomo/interfile_to_pet/data/par/stir_math_ITK_output_file_format.par

With this par file:

output file format parameters:=
; sample .par file that can be used with stir_math to specify the
; output file format
;
; See the stir::OutputFileFormat hierarchy for possible values.
; here we use ITK (so this will only work if you linked STIR with ITK)
;
; Example usage to convert an image (in any format that STIR can read) to 
; a file format that ITK can write:
;
; convert input.hv to MetaIO
;    stir_math --output-format stir_math_ITK_output_file_format.par output.mhd input.hv
; use the default extension to determine the file format (here set too Nifti)
;    stir_math --output-format stir_math_ITK_output_file_format.par output input.hv

output file format type := ITK
ITK Output File Format Parameters:=
  ; use Nifti as output (could also use .mhdr, .nhdr etc)
  default extension:=.nii
End ITK Output File Format Parameters:=
END:=

There are no build errors.

KrisThielemans commented 4 years ago

it didn't find ITK when building. Please give more info. Also have a look at ITK_DIR in builds/STIR/build/CMakeCache.txt

ALEXJAZZ008008 commented 4 years ago

ITK_DIR does not exist in builds/STIR/build/CMakeCache.txt although DISABLE_ITK does and is set to ON which would explain my problem. In the SuperBuild there is no option to set DISABLE_ITK.

While looking I also found:

//No help, variable specified on the command line.
DISABLE_HDF5:BOOL=ON

//disable use of HDF5 libraries
DISABLE_HDF5_SUPPORT:BOOL=OFF

Which either seems wrong or redundant.

rijobro commented 4 years ago

From SuperBuild USE_ITK=ON.

DISABLE_HDF5_SUPPORT:BOOL is a variable that no longer exists. if you delete it, it won't/shouldn't come back.

ALEXJAZZ008008 commented 4 years ago

I have USE_ITK on, I am building ITK I am certain of that, I'm not using system ITK (even though I have it) because usually I have fewer problems with the SuperBuild if I allow it to do everything itself. Regardless DISABLE_ITK is on.

I cloned into a new directory for this yesterday so there must be remnants of DISABLE_HDF5_SUPPORT somewhere.

rijobro commented 4 years ago

I have USE_ITK on, I am building ITK I am certain of that, I'm not using system ITK (even though I have it) because usually I have fewer problems with the SuperBuild if I allow it to do everything itself. Regardless DISABLE_ITK is on.

Shouldn't happen because of code snippet below, but I can't say more without seeing logs. Is this in a clean build location? If STIR had DISABLE_ITK as a cached variable, for example, the SB wouldn't overwrite it.

https://github.com/SyneRBI/SIRF-SuperBuild/blob/aff6f779622307956741ecbd064ab24aa47bcac9/SuperBuild/External_STIR.cmake#L116-L117

I cloned into a new directory for this yesterday so there must be remnants of DISABLE_HDF5_SUPPORT somewhere.

With master branch of SB? Are you running with an install script? Is it possible that you're setting it there?

KrisThielemans commented 4 years ago

Talking to @ALEXJAZZ008008 this was likely a case of running the SB with STIR_DISABLE_ITK=OFF first and then rerunning with ON. It might therefore be a case where the STIR cache settings are not updated (I wouldn't know why, or how that changed since recently).

By the way, there's https://github.com/SyneRBI/SIRF-SuperBuild/issues/61 which tells you that the CMake ExternalProject stuff doesn't always update the CMake-cache of the dependent projects the way that you hope it does. I don't know if this is related or not.

rijobro commented 4 years ago

https://github.com/SyneRBI/SIRF-SuperBuild/blob/aff6f779622307956741ecbd064ab24aa47bcac9/SuperBuild/External_STIR.cmake#L114-L118

looks like DISABLE_ITK gets set to ON, but doesn't get switched back off.

@ALEXJAZZ008008 can you try adding this below the block:

  if (USE_ITK)
    set(STIR_CMAKE_ARGS ${STIR_CMAKE_ARGS} -DDISABLE_ITK:BOOL=OFF)
  endif()
rijobro commented 4 years ago

Better:

 if (NOT USE_ITK) 
   set(STIR_CMAKE_ARGS ${STIR_CMAKE_ARGS} -DDISABLE_ITK:BOOL=ON) 
 else()
   set(STIR_CMAKE_ARGS ${STIR_CMAKE_ARGS} -DDISABLE_ITK:BOOL=OFF)
   if (USE_SYSTEM_ITK)   
      set(STIR_CMAKE_ARGS ${STIR_CMAKE_ARGS} -DITK_DIR:PATH=${ITK_DIR}) 
 endif() 
ALEXJAZZ008008 commented 4 years ago

The TOF branch of STIR tagged at 3.1 and a version of SIRF which will build with this can be built using ALEXJAZZ008008/SIRF-SuperBuild_super_build as:

./SIRF-SuperBuild_super_build.sh 15 -c -2

However the master branch of SIRF cannot be built at the moment, see the issue over there.

KrisThielemans commented 4 years ago

Here's the link to your script https://github.com/ALEXJAZZ008008/SIRF-SuperBuild_super_build/blob/master/SIRF-SuperBuild_super_build.sh

i don't see anything special/relevant in the script aside from that it wipes any previous builds, but feel free to correct me.

KrisThielemans commented 4 years ago

We looked at this, and the issue was that CMake included an include path where an older version of STIR was installed, even though STIR_DIRS was ok. We didn't manage to find out which find_package statement was the culprit.

This is likely caused by to the somewhat wide search settings used by find_package, see https://cmake.org/cmake/help/latest/command/find_package.html#search-procedure. In particular, we suspected step 5 "Search the standard system environment variables." @ALEXJAZZ008008 had the old version of SIRF in his PATH and LD_LIBRARY_PATH by sourcing its env_ccppetmr.sh.

It might be possible to narrow the search, but that could also be undesirable.

@ALEXJAZZ008008 could you confirm if you solved this by deleting the old SIRF install path, or by not calling env_ccppetmr.sh?