Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compilation failure with DISABLE_GADGETRON=ON due to syn_utilities #622

Open
KrisThielemans opened this issue Apr 16, 2020 · 5 comments · May be fixed by #1194
Open

compilation failure with DISABLE_GADGETRON=ON due to syn_utilities #622

KrisThielemans opened this issue Apr 16, 2020 · 5 comments · May be fixed by #1194
Assignees
Milestone

Comments

@KrisThielemans
Copy link
Member

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

#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
    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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

Step 1:

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
Copy link
Member

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
Copy link
Member Author

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.

KrisThielemans added a commit to KrisThielemans/SIRF that referenced this issue May 11, 2023
The synergistic library/executables/tests have now been
adapted for DISABLE_Gadgetron etc.

The work-around in common/utilities.* has therefore been removed.

a job with DISABLE_Gadgetron has been added to GHA

Fixes SyneRBI#622
@KrisThielemans KrisThielemans linked a pull request May 11, 2023 that will close this issue
KrisThielemans added a commit to KrisThielemans/SIRF that referenced this issue May 24, 2023
The synergistic library/executables/tests have now been
adapted for DISABLE_Gadgetron etc.

The work-around in common/utilities.* has therefore been removed.

a job with DISABLE_Gadgetron has been added to GHA

Fixes SyneRBI#622
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants