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

STYLE: Rename CMake ITKElastix project "Elastix" to "ITKElastix" #170

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

N-Dekker
Copy link
Collaborator

Aims to avoid confusion between the elastix project and the ITKElastix project.

No longer include(ITKModuleExternal)

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

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?

Copy link
Collaborator Author

@N-Dekker N-Dekker Oct 17, 2022

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"?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Collaborator Author

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:

include(ITKModuleExternal)

Can you possibly explain what that is for? I mean, is it useful for ITKElastix to do include(ITKModuleExternal)?

Copy link
Member

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.

Copy link
Collaborator Author

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?

Copy link
Member

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about PyITKElastix?

Copy link
Member

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 ...

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 this pull request may close these issues.

3 participants