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.

Sign up for GitHub

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
Draft

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 •
edited
Loading

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:

ITKElastix/CMakeLists.txt

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

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


# To ease enablement with Python packaging
if(DEFINED ENV{ELASTIX_USE_OPENCL})
Expand Down Expand Up @@ -96,7 +96,6 @@ endif()
if(NOT ITK_SOURCE_DIR)
find_package(ITK REQUIRED)
list(APPEND CMAKE_MODULE_PATH ${ITK_CMAKE_DIR})
include(ITKModuleExternal)
else()
set(ITK_DIR ${CMAKE_BINARY_DIR})
itk_module_impl()
Expand Down