Make vtkAddon installable and remove link of Python library#67
Make vtkAddon installable and remove link of Python library#67AlexyPellegrini wants to merge 2 commits intoSlicer:mainfrom
Conversation
dzenanz
left a comment
There was a problem hiding this comment.
No objections after a glance.
lassoan
left a comment
There was a problem hiding this comment.
Thanks for the update. It is great that the amount of custom Python wrapping code will be decreased. There are just a few questions/problems to address.
When I've tried to build it with the VTK build folder of a Slicer build, I got this error:
Build started at 3:57 PM...
1>------ Build started: Project: ZERO_CHECK, Configuration: Release x64 ------
1>1>Checking Build System
2>------ Build started: Project: vtkAddon, Configuration: Release x64 ------
2>Building Custom Rule C:/D/vtkAddon/CMakeLists.txt
2>vtkAddonMathUtilities.cxx
2>C:\D\vtkAddon\vtkAddonMathUtilities.cxx(24,10): error C1083: Cannot open include file: 'vtk_eigen.h': No such file or directory
2>Done building project "vtkAddon.vcxproj" -- FAILED.
3>------ Skipped Build: Project: INSTALL, Configuration: Release x64 ------
3>Project not selected to build for this solution configuration
========== Build: 1 succeeded, 1 failed, 1 up-to-date, 1 skipped ==========
========== Build completed at 3:57 PM and took 02.079 seconds ==========
There are no build errors when using the same settings for building the latest main version of vtkAddon.
Does this new version will still allow building VMTK? See https://github.com/vmtk/vmtk/blob/master/CMake/CMakeLists.txt#L1-L6
If VMTK will no longer be able to rely on vtkAddon scripts for Python wrapping and packaging then we'll need help with updating VMTK.
Instead of adding explanations into commit comments, it would be better to have those in the source code (no need to hunt for explanations in commit comments). For example, there is no comment in the source code for the lib/cmake/vtkAddon path for the set(${PROJECT_NAME}_INSTALL_CMAKE_DIR lib/cmake/vtkAddon) line, but there is a commit comment "Use a saner default for CMake install destination". It is not clear why it is saner. If the previously set value was not good then at least it should be described how the current value was chosen.
This would allow reducing the noise in the commit history and having fewer higher-level commits. Commit comments of these higher-level commits can be much more meaningful: they can describe at a higher level why the set of changes were done. If we fragment changes into tiny independent commits then the overall plot is lost.
6b857bf to
36bedfa
Compare
vtkAddon install tree used to contain absolute paths, miss headers and do not have all needed usage requirements. This refactor most of the install code, it still requires vtkAddon_INSTALL_NO_DEVELOPMENT=OFF to have the dev files to be installed: cmake export files and headers.
36bedfa to
2024498
Compare
I didn't try to compile Slicer or any other project until this morning, I successfully built slicer and all tests passed!
It should not break anything, all install related stuff is unrelated, and the python change is a runtime detail. We will help with the transition in case of issues.
I squashed all work commits, and added the information I think are needed in comment. Let me know if something still isn't clear. |
This PR contains all changes that I needed to make slicer-core package-able without too much effort.
related to #27
@lassoan @jamesobutler @Thibault-Pelletier @finetjul