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

[Bradley Lowekamp]

Dave,

I think Dan did much of the work in these directory, but I will still comment as I can. Your review seems right on overall.


On Jun 3, 2011, at 5:41 PM, David Cole wrote:

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

Makes sense.

> 
> 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}/" )

OK

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

target_link_libraries sounds better to me.

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

I think that JAVA is the only compile language we have bindings for, so that may be the reason.

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

What about the dependencies of this macro? When the test file is modified does CMake have to be re-run? is there a way around that?

> 
> # ./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)
> 

I would remove the GLOB.

> 
> # ./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"  )

This was me. That seem like it should have been very obvious to do that.

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

I think on mac and linux, it's very easy to build all but the C# and R languages. All of those are available on the mac with out special installations.

We hope we can automate much of the building and packaging of the wraps. We should definitely prioritize them. Python, JAVA, and Tcl I think are a must for the beta.

> 
> 
> # ./Wrapping/CSharpModules/FindCSharp.cmake
> 
> just do it and get rid of the "TO DO" comments...

Presumably some one would need both compilers and be familiar with C#. I don't think we have found the person yet.

This file I think was taken from VTK? Perhaps Luis know more about it.

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