Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

COMP: VtkGlue module-Provide support for VTK new cmake targets #731

Merged
merged 1 commit into from
Apr 15, 2019

Conversation

schmidje
Copy link
Contributor

Since VTK 8.2, the CMake building system is now using more intensively the CMake
targets system as proposed in "modern cmake". As a result, some CMake macros
used in the old VTK have been removed and the vtkGlue module of ITK does not
compile anymore.

The provided fix should support both new and previous VTK versions.

PR Checklist

Refer to the ITK Software Guide for
further development details if necessary.

Since VTK 8.2, the CMake building system is now using more intensively the CMake
targets system as proposed in "modern cmake". As a result, some CMake macros
used in the old VTK have been removed and the vtkGlue module of ITK does not
compile anymore.

The provided fix should support both new and previous VTK versions.
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The definition of macro(vtk_module_config ns) seems to be repeated 3 times. Is this intentional? Why are the second and third definition needed?

@schmidje
Copy link
Contributor Author

the macro is repeated in the sections corresponding to ITKVtkGlue_EXPORT_CODE_BUILD and ITKVtkGlue_EXPORT_CODE_INSTALL that are respectively used at the generation stage to produce files necessary in the future to link against ITK in the build or install directories. For instance ITKVtkGlue_EXPORT_CODE_BUILD will produce the file <build_dir>\ITK\Modules\Bridge\VtkGlue\CMakeFiles\ITKVtkGlue.cmake.

If you look closely the macro code is written within a section enclosed by "".

Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

@dzenanz dzenanz merged commit c79da79 into InsightSoftwareConsortium:master Apr 15, 2019
@schmidje
Copy link
Contributor Author

Closing the PR, thanks for the review. Hope the fix will work for others.

BR

@mathstuf
Copy link
Contributor

I'll note that this approach is basically trying to keep the old API up-to-date rather than properly migrating to the new one. Since ITK aims to support both, maybe this is OK. I'll note that one thing missing here is vtk_module_autoinit calls which means that object factories are not guaranteed to be properly initialized.

@schmidje
Copy link
Contributor Author

I agree with you, ITK should eventually properly migrate to the new CMake modules architecture, modern cmake offers so much advantages. However users should be better educated at linking with only the modules they really need, many of us, included myself, were too used to include the ITK_USE_FILE and link with all the ITK libraries.

Regarding the vtk_module_autoinit I am not sure the ITKVtkGlue CMakeLists.txt is the right place to put it as autoinit is used from my understanding at runtime. So I use it e.g. in the CMakeLists.txt of an exec using VTK and ITK.

@dzenanz
Copy link
Member

dzenanz commented Apr 16, 2019

Rendering part of ITKVtkGlue still does not work for me. This is a typical failure.

@mathstuf
Copy link
Contributor

That failure means that autoinit macros were not generated, so vtkRenderer never had an override register itself. It's needed at every place that VTK libraries are linked. Brad and I discussed how to do it with usage requirements and generator expressions, but there's no way with CMake today and the logic CMake would need to provide is non-trivial.

@dzenanz
Copy link
Member

dzenanz commented Apr 16, 2019

I created an issue to track this.

@schmidje
Copy link
Contributor Author

schmidje commented Apr 16, 2019

Indeed with the new VTK I had to add something like:

find_package(VTK COMPONENTS 
  ...)

# with vtk > 8.2 we must use the following
# otherwise it will fail at *runtime* for the rendering
vtk_module_autoinit(
  TARGETS my-target
MODULES ${VTK_LIBRARIES})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants