[Insight-developers] SimpleITK CMake best practices review
David Cole
david.cole at kitware.com
Fri Jun 3 12:59:00 EDT 2011
I will prepare a git topic branch containing changes to address this email.
I'll also have another email reviewing the remainder of the CMake
files coming this afternoon.
Please comment on that email, too, especially if you object to
anything. I'll also prepare commits on the same topic branch
addressing those things, too.
Thanks for the careful reading, and comments,
David
On Fri, Jun 3, 2011 at 12:53 PM, Johnson, Hans J <hans-johnson at uiowa.edu> wrote:
> 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>
> Date: Fri, 3 Jun 2011 12:30:41 -0400
> To: Bradley Lowekamp <blowekamp at mail.nih.gov>
> Cc: David Cole <david.cole at kitware.com>, ITK <insight-developers at itk.org>,
> Luis Ibanez <luis.ibanez at kitware.com>, Daniel Blezek
> <Blezek.Daniel at mayo.edu>, Hans Johnson <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> 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.
>
> 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
>
>
> --
> 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.
> ________________________________
More information about the Insight-developers
mailing list