-
Notifications
You must be signed in to change notification settings - Fork 23
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
STYLE: Rename CMake ITKElastix project "Elastix" to "ITKElastix" #170
base: main
Are you sure you want to change the base?
STYLE: Rename CMake ITKElastix project "Elastix" to "ITKElastix" #170
Conversation
Aims to avoid confusion between the elastix project and the ITKElastix project. No longer `include(ITKModuleExternal)`
@@ -1,5 +1,5 @@ | |||
cmake_minimum_required(VERSION 3.16.3) | |||
project(Elastix) | |||
project(ITKElastix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't just built-in ITK component supposed to have ITK prefix in their name like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so that's the reason! Thanks! :-) So what specifically can go wrong when the CMake project of ITKElastix is named "ITKElastix"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that primary benefit is reduction of confusion. I don't remember whether a prefix in a remote module would throw a wrench in the ITK's build system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the ITK
prefix is a marker that the module is a remote vs builtin module. The prefix is also used for Doxygen grouping, possibly others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I did encounter CMake errors from include(ITKModuleExternal) further on in the file:
Line 99 in 05cde7d
include(ITKModuleExternal) |
Can you possibly explain what that is for? I mean, is it useful for ITKElastix to do include(ITKModuleExternal)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, but it's name within ITK is Elastix
, not ITKElastix
. Similarly to how ITKMontage
is called Montage
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I renamed project(Elastix) to project(ITKElastix), I got the following errors:
CMake Error at F:/X/Src/I/ITK53RC4/CMake/ITKModuleKWStyleTest.cmake:52 (file):
file failed to open for writing (Permission denied):
/ITKKWStyleFiles.txt
Call Stack (most recent call first):
F:/X/Src/I/ITK53RC4/CMake/ITKModuleMacros.cmake:192 (itk_module_kwstyle_test)
F:/X/Src/I/ITK53RC4/CMake/ITKModuleExternal.cmake:181 (itk_module_impl)
CMakeLists.txt:99 (include)
These came indirectly from calling include(ITKModuleExternal)
, at CMakeLists line 99. Any idea what it was trying to do? Was it trying to run KWStyle on ITKElastix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The project name is expected to be the same as the module name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about PyITKElastix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it also provides a interface for C++, soon WebAssembly, so not just Python :-)
If it needs to be changed, perhaps ITKElastixModule
, or ...
Aims to avoid confusion between the elastix project and the ITKElastix project.
No longer
include(ITKModuleExternal)