[Insight-developers] SimpleITK CMake best practices review
Johnson, Hans J
hans-johnson at uiowa.edu
Fri Jun 3 12:53:30 EDT 2011
I'd propose that this work "be done".
David's original e-mail seemed to be asking permission to start making code changes. It seems like consensus has been reached on most of this.
Hans
From: Gabe Hart <gabe.hart at kitware.com<mailto:gabe.hart at kitware.com>>
Date: Fri, 3 Jun 2011 12:30:41 -0400
To: Bradley Lowekamp <blowekamp at mail.nih.gov<mailto:blowekamp at mail.nih.gov>>
Cc: David Cole <david.cole at kitware.com<mailto:david.cole at kitware.com>>, ITK <insight-developers at itk.org<mailto:insight-developers at itk.org>>, Luis Ibanez <luis.ibanez at kitware.com<mailto:luis.ibanez at kitware.com>>, Daniel Blezek <Blezek.Daniel at mayo.edu<mailto:Blezek.Daniel at mayo.edu>>, Hans Johnson <hans-johnson at uiowa.edu<mailto:hans-johnson at uiowa.edu>>
Subject: Re: SimpleITK CMake best practices review
These all look like good comments (both from David and Brad). I've added to the one with my name on it below.
On 06/03/2011 12:11 PM, Bradley Lowekamp wrote:
Hello David,
Thanks for looking through the CMake files. I interleaved what ever comment I had.
On Jun 2, 2011, at 3:52 PM, David Cole wrote:
One more thing that I thought about after sending this, but before going on to the rest of the files...
One thing I found strange was the file list in Common. It includes files directly from Common, then from a subdirectory called Ancillary, and then also from ../Registration. Why the multiple directories if they're all just built into one library?
I have no explanation for "../Registration".
I believe the goal of the Ancillary directory had to do with the desired interface to expose. We only want to expose a minimal set of headers to the users. This directory should contain needed includes that are not part of the ITK interface, but due to C++ templates are required to be included.
If they're all supposed to be built into the Common library, then I think the sources should simply be organized into the Common folder.
Let me know if it would be easier to simply point you to a git topic branch rather than do this by email. (I like the explanatory text I can write in emails, but I realize you guys might want me to just "shut up and do it" -- so let me know...)
The e-mail currently is working better for me, until I return full time to work from paternity leave.
Thanks,
David
On Wed, Jun 1, 2011 at 5:43 PM, David Cole <david.cole at kitware.com<mailto:david.cole at kitware.com>> wrote:
Here are my initial notes on reviewing the SimpleITK source tree as it
pertains to using CMake best practices.
I can prepare a patch for git (if you all agree with the points I'm
making) and push it to 'next' -- let me know if you'd like me to do
that. Also, please advise whether you think it would be a good idea to
copy the ITK Developers list on this or not...
This portion of the review covers the following directories and files:
davidcole at HUT11 ~/Dashboards/My Tests/SimpleITK (next)
$ find . | grep -i cmake
# done ./CMake
# done ./CMake/generate_filter_source.cmake
# done ./CMake/same_uint64_ulong.cxx
# done ./CMake/static_assert.cxx
# done ./CMakeLists.txt
# done ./Code/BasicFilters/CMakeLists.txt
# done ./Code/CMakeLists.txt
# done ./Code/Common/CMakeLists.txt
# done ./Code/IO/CMakeLists.txt
# done ./CTestConfig.cmake
# done ./CTestCustom.cmake.in<http://CTestCustom.cmake.in/>
# done ./Documentation/CMakeLists.txt
# done ./Documentation/Tutorials/CMakeLists.txt
# done ./Documentation/Tutorials/Medium/CMakeLists.txt
# done ./Documentation/Tutorials/Short/CMakeLists.txt
# done ./Examples/CMakeLists.txt
# done ./Examples/Segmentation/CMakeLists.txt
My review of the directories Testing, Utilities and Wrapping is still
to come (tomorrow).
Additionally, I'll reserve review/judgment on these top level files:
./SimpleITKConfig.cmake.in<http://SimpleITKConfig.cmake.in/>, ./sitkGenerateSimpleITKConfig.cmake,
./UseSimpleITK.cmake.in<http://UseSimpleITK.cmake.in/> until after we add install rules and actually
have a build-tree/install-tree dichotomy to talk about.
Here are the detailed notes from my initial pass:
file(GLOB is used in several places:
CMake/generate_filter_source.cmake: file ( GLOB json_config_files
${generated_code_input_path}/json/*.json)
CMakeLists.txt:file( GLOB template_components
Code/BasicFilters/CMakeLists.txt:file ( GLOB
SimpleITKBasicFiltersSource include/*.txx src/*.cxx )
Code/Common/CMakeLists.txt:file ( GLOB SimpleITKCommonSource *.cxx
*.h ../Registration/*.h ../Registration/*.cxx)
Code/Common/CMakeLists.txt:file ( GLOB SimpleITKAncillarySource
Ancillary/*.h Ancillary/*.cxx )
Code/IO/CMakeLists.txt:file ( GLOB SimpleITKIOSource *.cxx *.h )
Testing/Unit/CMakeLists.txt: file ( GLOB CXX_TEMPLATE_FILES
"*Template*.cxx.in<http://cxx.in/>" )
Testing/Unit/CMakeLists.txt: file ( GLOB LUA_TEMPLATE_FILES
"*Template*.lua.in<http://lua.in/>" )
Testing/Unit/CMakeLists.txt: file ( GLOB PYTHON_TEMPLATE_FILES
"*Template*py.in<http://py.in/>" )
Testing/Unit/CMakeLists.txt: file ( GLOB TCL_TEMPLATE_FILES
"*Template*.tcl.in<http://tcl.in/>" )
Testing/Unit/CMakeLists.txt: file ( GLOB R_TEMPLATE_FILES
"*Template*.R.in" )
Testing/Unit/CMakeLists.txt: file ( GLOB RUBY_TEMPLATE_FILES
"*Template*rb.in<http://rb.in/>" )
Testing/Unit/CMakeLists.txt: file ( GLOB JAVA_TEMPLATE_FILES
"*Template*.java.in<http://java.in/>" )
Utilities/CMakeLists.txt:file ( GLOB LUA_LIB_SOURCE lua-5.1.4/src/*.c )
Utilities/CMakeLists.txt:file ( GLOB LUA_SOURCE lua-5.1.4/src/lua.c )
Utilities/Doxygen/GenerateExamplesDox.cmake:file( GLOB_RECURSE
EXAMPLES_CXX RELATIVE "${PROJECT_SOURCE_DIR}/Examples" "*
Utilities/Doxygen/GenerateExamplesDox.cmake:file( GLOB_RECURSE
EXAMPLES_PYTHON RELATIVE "${PROJECT_SOURCE_DIR}/Examples"
Utilities/Doxygen/GenerateExamplesDox.cmake:file( GLOB_RECURSE
EXAMPLES_JAVA RELATIVE "${PROJECT_SOURCE_DIR}/Examples" "
Utilities/Doxygen/GenerateExamplesDox.cmake:file( GLOB_RECURSE
EXAMPLES_TCL RELATIVE "${PROJECT_SOURCE_DIR}/Examples" "*
Utilities/Doxygen/GenerateExamplesDox.cmake:file( GLOB_RECURSE
EXAMPLES_RUBY RELATIVE "${PROJECT_SOURCE_DIR}/Examples" "
Utilities/Doxygen/GenerateExamplesDox.cmake:file( GLOB_RECURSE
EXAMPLES_R RELATIVE "${PROJECT_SOURCE_DIR}/Examples" "*.R
Wrapping/CMakeLists.txt: file ( GLOB SWIG_HEADERS
${SimpleITK_SOURCE_DIR}/Code/*/*.h)
Wrapping/CSharpModules/FindDotNetFrameworkSdk.cmake:file(
GLOB_RECURSE csharp_dotnet_executables "${csharp_dotnet_framew
This is fine as long as all of the developers understand the following caveat:
When you add or remove a file that is included in one of these glob results,
you have to re-run CMake *manually* in order for the change to be reflected
in the build tree. CMake does not automatically re-run when files
appear/disappear in the file system.
I am not a big fan of these GLOBs and they do cause me some building issues. I would hope that we could at least get the Code directory free of them.
In the macro "get_dependent_template_components" the following code:
if("${h_contents}" MATCHES ".*${filename}.*")
set(${out_var_name} ${${out_var_name}} ${component})
endif()
if("${cxx_contents}" MATCHES ".*${filename}.*")
set(${out_var_name} ${${out_var_name}} ${component})
endif()
will add "${component}" to the list twice if both header file and cxx file
match. (I don't know if that's likely to occur, or if that's the intent, or if
it's accounted for already in other code elsewhere, but that's what the code
says.)
Sounds like a reasonable fix.
In the macro "expand_template" these two lines could be consolidated into one:
set ( ${library_name}GeneratedSource
${${library_name}GeneratedSource} "${output_h}" CACHE INTERNAL "" )
set ( ${library_name}GeneratedSource
${${library_name}GeneratedSource} "${output_cxx}" CACHE INTERNAL "" )
Could be:
set ( ${library_name}GeneratedSource ${${library_name}GeneratedSource}
"${output_h}" "${output_cxx}" CACHE INTERNAL "" )
ok
GENERATED_FILTER_LIST is one giant list of unqualified/un-namespaced/
module-unknown names. Are name clashes avoided in some way?
All filters should be includes in ITK. So if ITK doesn't have header conflicts then we should be ok too?
The macro "generate_filter_source" assumes a "Documentation" target exists
already if BUILD_DOXYGEN is on.
if (BUILD_DOXYGEN)
add_dependencies(Documentation ${directory_name}SourceCode)
endif (BUILD_DOXYGEN)
Is there a better way to add the dependency that the Doxygen needs the generated code?
In reviewing the CMakeCache.txt file produced for a SimpleITK build, I notice
that IMAGE_FILTER_LIST appears to contain two full copies of a list of classes.
The typo "C++x0" occurs twice:
CMake/static_assert.cxx: static_assert( true, "this should compile
with C++x0 static_assert keyword support");
Code/Common/sitkMacro.h:// utilize the c++x0 static_assert if available
This is my fault is should be fix. But truthfully do we know which decade the next version of C++ will come out?
1) should appear before the "project" command
2) should use FATAL_ERROR
as in:
cmake_minimum_required(VERSION 2.8 FATAL_ERROR)
project(SimpleITK)
Variable "SimpleITK_INCLUDE_DIR" should be plural since it is a list of dirs
What is SITK_EXPRESS_INSTANTIATEDPIXELS for?
Does turning it on just cut down on build times?
It was used early on, but most developers are now using distcc or ccache, so it could be removed.
This message should point directly to the mentioned FAQ:
message( FATAL_ERROR "SimpleITK requires usage of parts C++
Technical Report 1 (TR1).\n"
"It may be available as an optional download for your compiler.
Please see the FAQ for details.\n" )
Is this testing for the wrong file or is the variable named wrong?
check_include_file_cxx( stdint.h SITK_HAS_STDINT )
Again with the FAQ:
message( FATAL_ERROR "SimpleITK requires usage of C99 stdint.\n"
"It may be available as an optional download for your compiler.\n"
"Please see the FAQ for details and to see if your compiler is
supported.\n" )
Agreed. We should also write the FAQ too. And the Doxygen webpage should be moved from NLM, to Kitware.
Compiler flags settings should be inside a compiler-specific block, not an
OS-specific block. Change this "WIN32" to "MSVC":
if (WIN32)
# /bigobj is required for windows builds because of the size of
# some object files (CastImage for instance)
set ( CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /bigobj" )
set ( CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /bigobj" )
# Avoid some warnings
add_definitions ( -D_SCL_SECURE_NO_WARNINGS )
endif()
OK
Incorrect comment:
option ( BUILD_DOCUMENTS "Build testing" OFF )
Good.
I tried configuring with BUILD_DOCUMENTS ON and got this:
pdflatex compiler was not found. Please pass to advanced mode and
provide its full pathFATAL_ERROR
(looks like the FATAL_ERROR is misplaced in the message call in
Documentation/Tutorials/CMakeLists.txt -- if using FATAL_ERROR, it should be
the first arg to message)
OK
In "Code/Common/CMakeLists.txt" there is this:
target_link_libraries ( SimpleITKCommon ${ITK_LIBRARIES} )
Therefore, these:
target_link_libraries ( SimpleITKBasicFilters SimpleITKCommon ${ITK_LIBRARIES} )
target_link_libraries ( SimpleITKIO SimpleITKCommon ${ITK_LIBRARIES} )
do not need the ${ITK_LIBRARIES} listed. They depend on Common, therefore they
will automatically depend on the things Common depends on.
OK
In "Code/CMakeLists.txt", why is there a comment that "IO" must be last?
It appears that IO only depends on Common, and also that BasicFilters only
depends on Common. Seems like the order of IO and BasicFilters shouldn't
matter as long as they both come after Common. Is there a reason for this
comment?
Git blame says that Gabe added that line. I don't know the reason myself.
Uh oh, I was thinking this was someone else's issue. It's possible that this was part of the original setup and it got labeled with my name when I refactored the directory structure a long time ago. Either way, it's probably vestigial and can probably be removed.
CTestConfig points to ITK's CDash dashboard.
Perhaps SimpleITK should have its own CDash dashboard so that the updates
listed as associated with a build are relevant and viewable on a web-based git
viewer onto the SimpleITK repo. As it stands now, the updates that occur on a
SimpleITK dashboard do not have web views because they're on the ITK dashboard
and the ITK dashboard is set up to link to the ITK repo's web view...
Move the repo, new SimpleITK dashboard, and if we move the Doxygen to Kitware SimpleITK will be all set up there.
Examples/Segmentation/CMakeLists.txt has:
include_directories ( ${SimpleITK_INCLUDE_DIR} )
...Even though it inherits this value from the parent directory. Is this
necessary? Will Examples/Segmentation be built without coming in from the
parent CMakeLists.txt file?
Perhaps this example was written with the idea of being a standalone project?
sitkGenerateSimpleITKConfig.cmake only generates a UseITK.cmake file for the
build tree. There isn't one for the install tree, yet.
We have not tackled installation yet.
Cheers,
David C.
========================================================
Bradley Lowekamp
Lockheed Martin Contractor for
Office of High Performance Computing and Communications
National Library of Medicine
blowekamp at mail.nih.gov<mailto:blowekamp at mail.nih.gov>
--
Gabe Hart
R&D Engineer
Kitware, Inc. - North Carolina Office
http://www.kitware.com
(919) 969-6990 x317
________________________________
Notice: This UI Health Care e-mail (including attachments) is covered by the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521, is confidential and may be legally privileged. If you are not the intended recipient, you are hereby notified that any retention, dissemination, distribution, or copying of this communication is strictly prohibited. Please reply to the sender that you have received the message in error, then delete it. Thank you.
________________________________
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20110603/1569eaf6/attachment.htm>
More information about the Insight-developers
mailing list