[Insight-developers] SimpleITK CMake best practices review, part 2

[David Cole]

Testing, Utilities and Wrapping

Here are my next notes on reviewing the SimpleITK source tree as it
pertains to using CMake best practices.

I will prepare patches for git as a topic branch for most / many of
these points.

Please feel free to make comments, especially if you think I've
mis-interpreted something while reviewing these items.

Thanks,
David Cole


This portion of the review covers the following directories and files:

davidcole at HUT11 ~/Dashboards/My Tests/SimpleITK (next)
$ find . | grep -i cmake
#done ./Testing/CMakeLists.txt
#done ./Testing/Unit/CMakeLists.txt
#done -- apparently unused... -- ./Testing/Unit/GoogleTest/CMakeLists.txt
#done ./Utilities/CMakeLists.txt
#done ./Utilities/Doxygen/Doxygen.cmake
#done ./Utilities/Doxygen/GenerateExamplesDox.cmake
#done ./Utilities/KWStyle/CMakeLists.txt
#done ./Utilities/KWStyle/KWStyle.cmake
#done ./Wrapping/CMakeLists.txt
#done ./Wrapping/CSharpModules/FindCSharp.cmake
# still under review ./Wrapping/CSharpModules/FindDotNetFrameworkSdk.cmake
#done -- trivial / unimplemented -- ./Wrapping/CSharpModules/FindMono.cmake
#done ./Wrapping/CSharpModules/UseCSharp.cmake
#done -- trivial / no contents --
./Wrapping/CSharpModules/UseDotNetFrameworkSdk.cmake
#done -- trivial / unimplemented -- ./Wrapping/CSharpModules/UseMono.cmake
#done ./Wrapping/FindR.cmake
#done ./Wrapping/UseSWIGLocal.cmake


# ./Testing/Unit/CMakeLists.txt:

These file(GLOB operations are done inside the foreach loop, but their results
should be the same on each loop iteration. I'd do them before the foreach loop
so that they are not repeated over and over again. They're not the
quickest operations on the block...
  file ( GLOB CXX_TEMPLATE_FILES "*Template*.cxx.in" )
  # etc. for all the wrapper languages, too

Each wrapper language's add_custom_command for generating these tests with lua
is astonishingly similar to the other wrapper languages. I think a function is
in order here, passing in the necessary different bits as parameters.

Why the PARENT_SCOPE here?
  SET(GTEST_INCLUDE_DIRS "${CMAKE_CURRENT_SOURCE_DIR}/GoogleTest" PARENT_SCOPE)
  SET(GTEST_LIBRARY_DIRS "${CMAKE_CURRENT_BINARY_DIR}" PARENT_SCOPE)

Should not have a trailing "/":
  set(TEST_HARNESS_EXECUTABLE_DIRECTORY
"${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/${CMAKE_CFG_INTDIR}/" )

Why the ESCAPE_QUOTES here?
  configure_file(${CMAKE_CURRENT_SOURCE_DIR}/SimpleITKTestHarnessPaths.h.in

${CMAKE_CURRENT_BINARY_DIR}/SimpleITKTestHarnessPaths.h ESCAPE_QUOTES)

Why use "link_libraries"? (Why not target_link_libraries where necessary?
link_libraries links it to everything added afterwards...
  link_libraries(gtest)

Why is there only a java dependency here? Do the other wrapper languages not
need to be built first for the tests to be generated successfully?
  # Add org.itk.simple.jar dependency if necessary
  if( WRAP_JAVA )
    add_dependencies(WrappedGeneratedTests org_itk_simple_jar)
  endif()

This should include 'gtest' and exclude the ITK_LIBRARIES, since
SimpleITK_LIBRARIES will pull them in automatically:
  target_link_libraries ( SimpleITKUnitTestDriver ${ITK_LIBRARIES}
${SimpleITK_LIBRARIES} )

I like the ADD_GOOGLE_TESTS macro: very cool to parse all the tests from the
actual test source code... The only potential downside I see here is that it
may be easy to introduce duplicate test names in the system since they are
distributed among multiple files. Unless the first component of the name is
always guaranteed to be based on the filename: is that true?


# ./Testing/Unit/GoogleTest/CMakeLists.txt

This file is apparently unused... I presume it's just there because the tree
is an exact copy of the GoogleTest project...?


# ./Utilities/CMakeLists.txt

This line mentions "lua-5.1.4/src/luac.c" although there is no such file...
  list ( REMOVE_ITEM LUA_LIB_SOURCE lua-5.1.4/src/lua.c lua-5.1.4/src/luac.c )

Why GLOB?
  file ( GLOB LUA_SOURCE lua-5.1.4/src/lua.c )
Why not just set?
  set(LUA_SOURCE lua-5.1.4/src/lua.c)


# ./Utilities/Doxygen/GenerateExamplesDox.cmake

Unless there's a real need for the separate lists, these could all be done in
one call to file(GLOB_RECURSE with multiple file glob expressions as
the final args:
file( GLOB_RECURSE EXAMPLES_CXX RELATIVE
"${PROJECT_SOURCE_DIR}/Examples" "*.cxx"  )
file( GLOB_RECURSE EXAMPLES_PYTHON RELATIVE
"${PROJECT_SOURCE_DIR}/Examples" "*.py"  )
file( GLOB_RECURSE EXAMPLES_JAVA RELATIVE
"${PROJECT_SOURCE_DIR}/Examples" "*.java"  )
file( GLOB_RECURSE EXAMPLES_TCL RELATIVE
"${PROJECT_SOURCE_DIR}/Examples" "*.tcl"  )
file( GLOB_RECURSE EXAMPLES_RUBY RELATIVE
"${PROJECT_SOURCE_DIR}/Examples" "*.rb"  )
file( GLOB_RECURSE EXAMPLES_R RELATIVE "${PROJECT_SOURCE_DIR}/Examples" "*.R"  )

mismatch:
  FOREACH( f IN LISTS EXAMPLES_LIST )
  ENDFOREACH( f IN LIST EXAMPLES_LIST )

no new line at EOF


# ./Utilities/KWStyle/CMakeLists.txt

empty / unnecessary?


# ./Utilities/KWStyle/KWStyle.cmake

included directly in top level CMakeLists.txt file
looks good, substantially the same as what's in ITK


# ./Wrapping/CMakeLists.txt

use CMAKE_CURRENT_SOURCE_DIR refs instead; append to existing
CMAKE_MODULE_PATH, too:
  set(CMAKE_MODULE_PATH
"${SimpleITK_SOURCE_DIR}/Wrapping;${PROJECT_SOURCE_DIR}/Wrapping/CSharpModules"
)

Does "-w" mean the same thing on all supported compilers?
  set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/SimpleITKLUA_wrap.cxx
COMPILE_FLAGS "-w")

No need for ITK_LIBRARIES here:
  target_link_libraries ( SimpleITKLua ${SimpleITK_LIBRARIES}
${LUA_LIB} ${ITK_LIBRARIES} )

curses assumed available on UNIX (but not checked for or documented
that I found):
  if ( UNIX )
    target_link_libraries ( SimpleITKLua curses )
  endif()

I am not yet a swig expert, so I am not certain that all the uses of the swig
macros in this file are correct. Nor do I have all of these wrapper language
SDKs and libraries installed, so I can't test them all personally either. It's
quite a number of wrapper languages to support, especially given the intent to
create binary distributions for all of them. Who's going to be creating those
distributions? How frequently? Are all of them well-tested on the dashboard
submissions that we do have?


# ./Wrapping/CSharpModules/FindCSharp.cmake

just do it and get rid of the "TO DO" comments...

Why are we unsetting the cache variables and re-finding every time?
  unset( CSHARP_COMPILER CACHE )
  unset( CSHARP_TYPE CACHE )
  unset( CSHARP_VERSION CACHE )

should be "SEND_ERROR" or "FATAL_ERROR":
  message( ERROR "C# wrapping for Mono is not supported (yet)" )

should use (to set CSHARP_FOUND to TRUE):
  INCLUDE(FindPackageHandleStandardArgs.cmake)
  FIND_PACKAGE_HANDLE_STANDARD_ARGS(CSHARP DEFAULT_MSG CSHARP_TYPE
CSHARP_COMPILER)


# ./Wrapping/CSharpModules/FindDotNetFrameworkSdk.cmake

I need more time to look at this one more carefully...


# ./Wrapping/CSharpModules/UseCSharp.cmake

# TODO: ADD SUPPORT FOR LINK LIBRARIES

works for now on Windows, but won't work (eventually) on Mac/Linux with mono:
  string( REPLACE "/" "\\" sources ${sources} )


# ./Wrapping/FindR.cmake

should use (to set R_FOUND to TRUE):
  INCLUDE(FindPackageHandleStandardArgs.cmake)
  FIND_PACKAGE_HANDLE_STANDARD_ARGS(R DEFAULT_MSG R_COMMAND
R_INCLUDE_DIR R_LIBRARIES RSCRIPT_EXECUTABLE)


#./Wrapping/UseSWIGLocal.cmake

no comments yet...