SyneRBI / SIRF

Main repository for the CCP SynerBI software
http://www.ccpsynerbi.ac.uk
Other
58 stars 29 forks source link

SIRF build fails on NiftyMoMo #784

Closed evgueni-ovtchinnikov closed 3 years ago

evgueni-ovtchinnikov commented 4 years ago

Building SIRF master on VM fails with

/home/sirfuser/devel/buildVM/sources/SIRF/src/Registration/NiftyMoMo/BSplineTransformation.cpp:1340:39: error: prototype for ‘NiftyMoMo::BSplineTransformation::PrecisionType* NiftyMoMo::BSplineTransformation::GetDVFGradientWRTTransformationParameters(nifti_image*)’ does not match any in class ‘NiftyMoMo::BSplineTransformation’
 BSplineTransformation::PrecisionType* BSplineTransformation::GetDVFGradientWRTTransformationParameters( nifti_image* denseDVFIn )
                                       ^~~~~~~~~~~~~~~~~~~~~
In file included from /home/sirfuser/devel/buildVM/sources/SIRF/src/Registration/NiftyMoMo/BSplineTransformation.cpp:21:0:
/home/sirfuser/devel/install/include/sirf/NiftyMoMo/BSplineTransformation.h:90:49: error: candidate is: virtual NiftyMoMo::BSplineTransformation::PrecisionType* NiftyMoMo::BSplineTransformation::GetDVFGradientWRTTransformationParameters(nifti_image*, nifti_image*)
   virtual BSplineTransformation::PrecisionType* GetDVFGradientWRTTransformationParameters( nifti_image* denseDVFIn, nifti_image* sourceImage );
                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
KrisThielemans commented 4 years ago

This was flagged up by @rijobro as a potential problem, and indeed it's there 😢

note that it got the include file from /home/sirfuser/devel/install/include/sirf/NiftyMoMo/BSplineTransformation.h. Solution is therefore to rm -rf /home/sirfuser/devel/install/include/sirf/NiftyMoMo

I guess we will have to put this in SyneRBI/UPDATE_VM.sh

rijobro commented 4 years ago

Yup, sorry about this. See https://github.com/SyneRBI/SIRF/issues/783.

evgueni-ovtchinnikov commented 4 years ago

thanks!

paskino commented 3 years ago

I stumbled upon this problem building SIRF master after I built v2.2.0. Maybe it's something for SIRF's CMake to remove that directory rather than UPDATE.sh?

Actually, we could delete all sirf/include directory?

KrisThielemans commented 3 years ago

I can't see us doing this in CMake (it'd be rather weird to have a build wipe some of your install), but of course the UPDATE.sh script could easily do that.

rijobro commented 3 years ago

Sorry that this has caused so many problems. Might be easier to address the problem this way as it would be tidier and more future proof.

paskino commented 3 years ago

What about the SuperBuild to remove the directory if present?

rijobro commented 3 years ago

As @KrisThielemans , could go in the UPDATE.sh script, but shouldn't really go in the build/install instructions. Wouldn't be expected for either of those steps to be deleting folders in the install location.

KrisThielemans commented 3 years ago

I find deleting things in an install directory a bit scary. You don't know what the user has done or wants to do.

I suspect that there are few people left who haven't deleted this themselves, or wouldn't be using the VM UPDATE.sh. Possibly we could create a "pre-compile" step that warns about it and stops the compilation, but I can see this eating up your time.

paskino commented 3 years ago

This problem prevents SIRF to build in the SuperBuild throwing a very strange error. I could put the fix in UPDATE.sh in the VM, but if you don't use the VM you will hit this problem.

I think this is mostly a SuperBuild bug and should be fixed there with a pre compile step.

KrisThielemans commented 3 years ago

It is not a SuperBuild bug. It is a SIRF bug that it looks for include files in the wrong place (you should never go and find things in the install directory first, before looking in the source directory). The problem is that it's hard to track down who puts the install path before the SIRF source, and then to fix that. See #783 for info.

As far as I understand, you will only hit this problem if you have built an old version of SIRF before, and are trying to install in the same location. This is something that we don't recommend on our wiki.

If it happens as well with a new version, then this problem becomes much more serious

paskino commented 3 years ago

I believe the problem is this line.

${NIFTYREG_INCLUDE_DIRS} expands to install/include in my machine.

The solution is just to remove BEFORE at that line, apparently.