[Insight-developers] SimpleITK CMake best practices review

Fri Jun 3 12:11:52 EDT 2011

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> 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
>  # 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, ./sitkGenerateSimpleITKConfig.cmake,
> ./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" )
>  Testing/Unit/CMakeLists.txt:    file ( GLOB LUA_TEMPLATE_FILES
> "*Template*.lua.in" )
>  Testing/Unit/CMakeLists.txt:    file ( GLOB PYTHON_TEMPLATE_FILES
> "*Template*py.in" )
>  Testing/Unit/CMakeLists.txt:    file ( GLOB TCL_TEMPLATE_FILES
> "*Template*.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" )
>  Testing/Unit/CMakeLists.txt:    file ( GLOB JAVA_TEMPLATE_FILES
> "*Template*.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.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20110603/9c5fbc7f/attachment.htm>