[vtk-developers] Notes on VTK modularization

[Marcus D. Hanwell]

On Sun, Mar 25, 2012 at 8:14 PM, Pat Marion <pat.marion at kitware.com> wrote:
> I spent some time playing with VTK modular today.  It's looking great, and
> it wasn't too difficult to update VES for the latest master, awesome work
> guys!  Here are my notes...
>
Great.
>
> - doc string for the cmake variable BUILD_SHARED_LIBS still says "Build
> Verdict with shared libraries", maybe it's time to fix this bug now
>
I guess that is the last option call winning on the doc string. I will
remove the line from verdict.

> - while we're redoing the cmake code... in my opinion, there is too much
> cmake variable clutter.  I think more variables should be marked as
> advanced.
>
I will take a look at this, it would be good to keep them sensible.

> - can we add a way to override the module on/off defaults?  I'd like to have
> a way to force all modules to default to OFF during the first cmake run.
> This is useful for superbuilds where I want everything off, with a specific
> set enabled.  Something like:  -DVTK_MODULE_DEFAULT_STATE=OFF
> -DModule_vtkFiltersGeoemtry=ON

This is temporary. I will see about fixing this up tomorrow. The plan
is all off by default, with groups that are use visible to turn on
related modules to make it easy for those that don't want to click
through all of the variables.
>
> - If I do a clean configure, then turn off some modules and reconfigure,
> then I get new Module_* cmake cache variables, why not add them all to the
> cache on the first pass?

I will look into it.
>
> - could we have a nicer pretty print of of the modules, and have this come
> at the very end of the cmake output?

There is some code Alex was working on for KDE I was hoping to look
into. Agreed, we should try and do better here.
>
> - in the cmake output, maybe say "3rd party library" instead of the TPL
> acronym as it currently is- "Processing TPL: ZLIB"

This is debug code that I have removed.
>
> - are the modularization macros/functions reusable by other vtk based
> projects (like paraview)? or even for non vtk projects?

ParaView is planning to re-use, we may need to change them a little to
ensure this works.
>
> - I tried to use vtk from install dir:  error: vtkCommonCoreExport.h: No
> such file or directory

That is an easy fix, I will get on that. Haven't gotten back to
testing against installed VTK.
>
> - We need a build target that compiles just the utilities like
> vtkWrapPython, vtkHashSource, etc.  This is useful for cross compiling.
>
> -"Modularization", this means different things to different people. To be
> sure, there are lots of things we want
> to get out of modularization.  One thing that I am interested in is static
> builds with minimal binary size.  This can be an important
> consideration when building for mobile devices and supercomputers
> (surprisingly similar constraints!)
> Even after employing all the tricks to achieve minimal size, there is
> sometimes a big difference between what the linker says is required, and
> what I think should be required.
> For example, the nm tool shows that vtkAlgorithm.cxx.o requires vtkGraph and
> vtkTable, and the linker agrees.  But I say we could drop that weight.
> vtkAlgorithm calls vtkTable::SafeDownCast() simply to print an error message
> if something is a table. It depends on vtkGraph because some code
> in vtkAlgorithm::GetInputAbstractArrayToProcess() requires special handling
> of vtkGraph to access arrays.  Perhaps the special handling
> could be refactored into a virtual method of vtkDataObject.  Anyway, this is
> just an example, food for thought.  There are plenty of other
> causes of bloat.  It would be neat if CDash could track bloat over time!

I think this is outside the scope of this work, but I couldn't agree
more and I would like to see us being much more careful about
introducing dependencies like this.
>
> - static python wasn't working, here some notes:
>
> (static python means that everything is linked into vtkpython, or paraview,
> or whatever the final binary is.  When you import vtkFilteringCorePython,
> instead of doing a dlopen on vtkFilteringCorePython.so, python recognizes
> this module as a built-in.  This is fairly easy to do in practice, a cmake
> macro configures a header file to be included by the exe)
>
> 1) vtkmodular includes hacks vtkPythonAppInit.cxx that should be reverted.
> I'm not sure if we still need the includes
> for the kit instantiators, but we do need #include <vtkpythonmodules.h> and
> CMakeLoadAllPythonModules();

We will not be building the instantiators by default, I will look into
that code. Is there a dashboard for this anywhere? I will see if I can
get a build working as I have never used VTK Python in this way
before.
>
> 2) vtkpython is built before the vtk kits are processed, this breaks for the
> static python case.
>  The vtkpython exe is added by Wrapping/Python/CMakeLists.txt, this accesses
> a cmake global property PY_STATIC_MODULES_LIST,
>  but this global property is not set until the vtk kits are wrapped.  The
> global property is set by
>  python_add_module, called from the function vtk_add_python_wrapping in
> vtkWrapping.py.

We are fixing the build order, so Wrapping will be added for the
wrapper generators, then all modules processed, and then the Tcl,
Python and Java directories will be added after that. We have noticed
several blocks of code requiring this order.
>
> 3) I was confused by all the dead cmake code in KitCommonBlock.cmake and
> KitCommonPythonWrapBlock.cmake, can this be removed?

Yes - I will be marking them as ignored. None of them are used.
>
> 4) It would be great to somehow move the PYTHON_ADD_MODULE and
> PYTHON_WRITE_MODULES_HEADER macros out of
>  FindPythonLibs.cmake, and remove FindPythonLibs.cmake from VTK.  If/when
> this happens, I have an edit
> I'd like to make to the PYTHON_ADD_MODULE macro:  the option
> PYTHON_MODULE_${_NAME}_BUILD_SHARED should
> default to the value of BUILD_SHARED_LIBS, not TARGET_SUPPORTS_SHARED_LIBS.
>
I was planning to remove FindPythonLibs, as CMake has a current copy.
I am not sure about removing stuff from there though, I will check on
backwards compatibility requirements but I think CMake is pretty
strict. We could always add another, and forward the original to the
new.
>
> VES notes:
>
> -I put a minor patch in ThirdParty/freetype/vtkfreetype/CMakeLists.txt to
> compile vtkfreetype for iOS.
>  The patch sets VTKFREETYPE_MAC_FONT_SUPPORT to 0 even though if(APPLE)
> evaluates to TRUE, because
>  mac font support doesn't work on iOS.  I still need to determine the best
> way to test for iOS from cmake.
>
> -I patch vtkFreeTypeTools.cxx to remove a somewhat trivial dependency on
> ftgl, then I remove the vtkftgl dependency from Rendering/Text/module.cmake
>  so that I can build vtkRenderingText without enabling vtkftgl. But
> vtkRenderingText has other classes that use ftgl in some way, so I just
> comment
>  those out because they are not used by VES.  So I'm still in search of a
> better solution here.  Perhaps it's possible to completely remove ftgl from
> vtk?
>
> - I have to comment out vtkFLUENTReader.cxx, the apple compiler chokes on
> this file when using the -Os flag and targeting iOS.  Compiler is
> i686-apple-darwin10-llvm-g++-4.2 (GCC) 4.2.1.
>
> - VES uses vtkOutlineFilter, which currently lives in Filters/Core but it is
> commented out because it depends
>   on Filters/Sources/vtkOutlineSource.cxx.  I moved vtkOutlineFilter to
> Filters/Sources so it can compile,
>   but this may not be good placement.
>
> - I pushed these hacks required for building VES to the kiwi-fixes branch on
> git://gitorious.org/~patmarion/kitware/patmarion-vtkmodular.git
>
Thanks for the extensive notes Pat. We will work on getting them integrated.

Marcus