-
-
Notifications
You must be signed in to change notification settings - Fork 380
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
Fix install step for SYSTEM/ITK/VTK, fix corner case in cmake config. #867
Conversation
Would you like me to test this? I have noticed that with commits after yesterday (554b696) I get the following error during The error is not at the very end, but a bit further up, look for this bit:
I'm just letting you know since this will make it more difficult for me to test your change, because I have to rebase it on an older commit. |
This error shouldn't be there with the pull I merged yesterday. |
@ntustison :( but sadly it is. As you can see in the build log pasted above, the failure is with d073c62 (current HEAD). |
@ntustison he is building against a system ITK that is behind master branch |
@gdevenyi If that's the only way forward, I can bump ITK to 5.1_beta or newer, the Gentoo package manager supports custom version definition at the commit level (so we could have |
@ntustison I ran more tests and the problem is not introduced by 64ef514 but by one of the two newer commits. |
@TheChymera I don't understand the distinction. Yesterday's pull is the same as the current head, is it not? Either way, the issue that you're running into is that an ITK developer changed the class |
@ntustison no problem. Can you tell me what ITK commit I would be needing? (though to clarify, I think you linked to a different pull than you intended: that is 64ef514 , the current head is d073c62 ) |
Ah, thanks for the clarification. Yes, I made a mistake. The pull actually contains two commits (because I was lazy, although they both concern N3/N4). I linked to the first commit of the pull while the current head contains both. |
The ITK tag is here |
ok, brb after I get a sufficiently new ITK building. Don't worry, I won't chase after the HEAD forever, I'm just looking for an ANTs commit where CMake works in my use case. So as soon as I can get that, I'll just pin the corresponding ITK commit and that's probably it until your next versioned ANTs release. |
I had to revert that commit anyway. Sorry for the waste of time. |
@ntustison ah well :D |
Passing here, should fix @TheChymera's make install with system libs issue. |
@gdevenyi ok, so I got the ITK issue out of the way, and I tested this patch, still, I get the same error:
|
2dd8f11
to
0ab2cbd
Compare
I had to write down an old-fashioned truth table to sort out the logic. Should be correct this time... |
0ab2cbd
to
98c454c
Compare
And bitten by CMAKE again. What a terrible default to silently allow for undefined variables. |
@gdevenyi I am getting the same error again:
maybe I am using the wrong commit for the patch? What I see you updated this PR to is 98c454c, but that seems to be 2 days old already, and probably not the newest version you were referring to. |
Is this a complete rebuild from a clean directory? I have tested the version you're referring to, and it works as expected. The date is such because it is an amended commit. |
@gdevenyi yes, sadly this is the case. I even wiped everything and tried again. And as you can see in the snippet above the respective files are also correctly edited :-/ any other info I could share? I can archive and upload the entire build directory tree if you would like. |
Okay, I've just tested again and it still works so...
Please |
Cmake:
Directory tree: tree.txt |
And the build log please? |
Okay, we have a corner case. So
So, you can fix your build by adding -DUSE_VTK=ON, but I need to add some kind of error condition on the cmake so that you can't get into this state. |
@gdevenyi ok, will do. Just so that I now whether to hard-code of parameterize this: can ANTs be built and is it of much use with |
VTK is optional and enables a number of surface enabled tools. |
You're missing itkvtkglue in your itk build ANTs/SuperBuild/External_ITKv5.cmake Line 140 in f78b2d4
|
98c454c
to
b2c828f
Compare
Yup, further adjustment to the logic here, tested locally found your bug. |
Ok, this worked, thank you :) |
Ready for merge. |
Addresses #857 (comment)