compilation failure with DISABLE_GADGETRON=ON due to syn_utilities

[KrisThielemans]

syn_utilities.cxx fails to compile in that case as it doesn't know about include gadgetron_data_containers.h etc

SIRF/src/common/syn_utilities.cpp

Lines 32 to 35 in 6854699

	#include "sirf/Gadgetron/gadgetron_data_containers.h"
	#include "sirf/STIR/stir_data_containers.h"
	#include "sirf/Reg/NiftiImageData3D.h"
	#include "sirf/Syn/utilities.h"

I'm a bit confused about all this.

• If it's synergistic, then it should sit in Synergistic.
• In fact, this file include Syn/utilities.h, which defines the class ImageDataWrap but
  • a class declaration should ideally sit in a file with the name of the class.
  • the class implementation and declaration should be in the same folder structure
• seems that we need to have ImageWrap existing all the time (so in common I guess), but use preprocessor defines to exclude lines in syn_utilities.cpp that won't compile if an engine is not available.
• The CMakeListst.txt seems to imply that syn_utilities.ccp should only be compiled when all engines are present, which seems to be the opposite to what was intended
  

  SIRF/src/common/CMakeLists.txt

  Lines 30 to 45 in 6854699

  	if ((NOT DISABLE_STIR) AND (NOT DISABLE_Gadgetron) AND (NOT DISABLE_Registration) AND "${STIR_BUILT_WITH_ITK}")
  	MESSAGE(STATUS "Registration, ISMRMRD and STIR (with ITK) have been built.")
  	target_link_libraries(csirf PUBLIC iutilities csyn)
  	else()
  	MESSAGE(STATUS "Either Registration or ISMRMRD or STIR (with ITK) have not been built.")
  	add_library(Syn syn_utilities.cpp)
  	target_include_directories(Syn PUBLIC
  	"$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>$<INSTALL_INTERFACE:include>"
  	)
  	target_include_directories(Syn PUBLIC
  	"$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>$<INSTALL_INTERFACE:include>"
  	)
  	target_link_libraries(Syn PUBLIC cgadgetron cstir Reg)
  	INSTALL(TARGETS Syn DESTINATION ${CMAKE_INSTALL_PREFIX}/lib)
  	target_link_libraries(csirf PUBLIC iutilities Syn)
  	endif()

[KrisThielemans]

I see that ImageDataWrap is used in common/csirf, but tested in cSyn/tests/test_conv_img.cpp.

Keeping in common does imply that all csirf depends on all engines. On the other hand, cstir depends on csirf. So we have a circular dependency. That is not a great idea and causes problems in STIR when using shared libraries.

Seems therefore to me that ImageDataWrap needs to be in synergistic

[evgueni-ovtchinnikov]

I just did it the simplest way (as usual, not being clever enough to work out complicated solutions:-)). And sorry about that cmake kludge intended to stop Travis from going red on some builds.

I will think about other ways, but there is something I do not quite understand: since the synergistic stuff is supposed to explicitly deal with STIR and Gadgetron (see cSyn/utils.cpp), how can it possibly be compiled with any of the two not present? (And what synergy are we talking about in such case?)

[KrisThielemans]

Step 1:

• create preprocessor variables via sirf/config.h.in (see https://github.com/UCL/STIR/blob/master/src/cmake/STIRConfig.h.in)
• modify syn_utilities.cpp to only use what we have
• modify CMakeLists.txt

Step 2:

• remove syn_utilities.cpp and keep synergistic/cSyn/utilities.cpp, but rename to ImageDataWrap.cpp (and .h),
• create C/library etc in synergistic

[ashgillman]

FYI - this also affects DISABLE_STIR and DISABLE_Registration. I couldn't quite tell from the proposed solution if that would get fixed too, I guess so.

Ash

[KrisThielemans]

This is unfortunate, as some people will want to disable certain components as they have problems with compilers etc (and if they don't need the component, shouldn't have to bother with it).

It's too long ago for me to remember it all, and my writing above isn't quite crystal clear, I don't have a good feeling for how much work step 1 would be. Doesn't sound like a lot, but these things tend to explode.