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

SIRF build fails on NiftyMoMo #784

Closed
evgueni-ovtchinnikov opened this issue Jul 22, 2020 · 12 comments · Fixed by #859
Closed

SIRF build fails on NiftyMoMo #784

evgueni-ovtchinnikov opened this issue Jul 22, 2020 · 12 comments · Fixed by #859
Assignees

Comments

@evgueni-ovtchinnikov
Copy link
Contributor

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

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

rijobro commented Jul 22, 2020

Yup, sorry about this. See #783.

@evgueni-ovtchinnikov
Copy link
Contributor Author

thanks!

@paskino
Copy link
Contributor

paskino commented Jan 21, 2021

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

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

rijobro commented Jan 21, 2021

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

paskino commented Feb 2, 2021

What about the SuperBuild to remove the directory if present?

@rijobro
Copy link
Contributor

rijobro commented Feb 2, 2021

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

KrisThielemans commented Feb 2, 2021

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

paskino commented Feb 2, 2021

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

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

paskino commented Feb 2, 2021

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.

paskino added a commit to paskino/SIRF that referenced this issue Feb 2, 2021
KrisThielemans added a commit that referenced this issue Feb 2, 2021
remove BEFORE in target_include_directories.

Fixes #784
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.

4 participants