-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[VTK/vtk-m] Update VTK to 9.0 and add vtk-m #11148
Conversation
Hi @Neumann-A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some weird things I saw; I'm not sure if these are an issue, but I'd like to at least understand the reasoning.
Co-authored-by: nicole mazzuca <mazzucan@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, looks good to me! Thanks @Neumann-A :)
Could some of these patches be sent to our issue tracker? Some look weird or unnecessary (or have more proper fixes if there are things wrong) to me. Thanks. |
lets just say more general fixes. Most findModule.patches are with the background knowledge that we are within VCPKG and have certain things with 100% certainty available. I also already opened up Issue on VTKs Gitlab which I thought relevant: If you have questions about any of the applied patches feel free to ask. |
+ add_library(LibHaru::LibHaru UNKNOWN IMPORTED) | ||
+ set_target_properties(LibHaru::LibHaru PROPERTIES | ||
+ INTERFACE_INCLUDE_DIRECTORIES "${LibHaru_INCLUDE_DIR}") | ||
+ if(LZMA_LIBRARY_RELEASE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy pasta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes copy paste. Need to correct that in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mathstuf: And now I know why I patched it:
-- Found LibHaru: optimized;D:/para/installed/x64-windows/lib/libhpdf.lib;debug;D:/para/installed/x64-windows/debug/lib/libhpdfd.lib (found suitable version "2.4.0-dev", minimum required is "2.4.0")
CMake Error at CMake/vtkDetectLibraryType.cmake:31 (message):
Unparsed arguments for vtk_detect_library_type:
D:/para/installed/x64-windows/lib/libhpdf.lib;debug;D:/para/installed/x64-windows/debug/lib/libhpdfd.lib
Call Stack (most recent call first):
CMake/FindLibHaru.cmake:48 (vtk_detect_library_type)
D:/para/scripts/buildsystems/vcpkg.cmake:329 (_find_package)
CMake/vtkModule.cmake:4134 (find_package)
CMake/vtkModule.cmake:4690 (vtk_module_find_package)
CMake/vtkModule.cmake:4564 (vtk_module_third_party_external)
ThirdParty/libharu/CMakeLists.txt:1 (vtk_module_third_party)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's bad interaction with select_library_configurations
. Hrm. I'll make a fix on VTK's side. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ IMPORTED_IMPLIB_DEBUG "${LZMA_LIBRARY_DEBUG}") | ||
+ endif() | ||
+ | ||
+ # Guard against possible stupidity of VTK reading only LOCATION without configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VTK definitely shouldn't be doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is why "possible" is there. But it is not only VTK it also includes all possible consumers of VTK. I did a quick search and could not find anything regarding this within VTK but I have seen real strange things in CMakeLists.txt of ports. Like getting the IMPORTED_LOCATION
from a target. Extracting the filename. Removing any PREFIX
and SUFFIX
from it and then using the result in target_link_libraries
..... When I wrote the patch I probably just encountered that problem in another port and thought about guarding against that behavior proactively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, those shenanigans should be nuked from orbit on sight :) . However, the wording makes it seem like VTK's at fault here which is why I noticed it. Might I suggest stupidity of various projects
? Issues when those are found are nice (feel free to Cc me if you need help in discussions with upstreams).
@@ -0,0 +1,269 @@ | |||
# Distributed under the OSI-approved BSD 3-Clause License. See accompanying |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can backport CMake's FindPostgreSQL into VTK; it looks like that's what this is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just a copy of FindPostgreSQL from CMake. I thought you wold be updating that automatically with the VTK release. Apparently not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only pull down modules from CMake I know we need to backport for specific use cases.
set(_vtk_python_genex_include_directories | ||
"$<TARGET_PROPERTY:${_vtk_python_target_name},INCLUDE_DIRECTORIES>") | ||
+ set(_vtk_python_genex_interface_include_directories | ||
+ "$<TARGET_PROPERTY:${_vtk_python_target_name},INTERFACE_INCLUDE_DIRECTORIES>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually required to run ParaView
https://gitlab.kitware.com/paraview/paraview/-/issues/19515
this patch is actually for 8.9 the code as since then slightly changed in this location.
Updated code will be available in
#9960 as soon as I am able to run ParaView.exe locally without it crashing due to missing wrapping.
|
||
-list(INSERT 0 CMAKE_MODULE_PATH | ||
- "${VTK_SOURCE_DIR}/ThirdParty/vtkm/vtkvtkm/vtk-m/CMake") | ||
+find_package(VTKm CONFIG REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be vtk_module_find_package
so that VTK finds VTKm for consumers if necessary.
new file mode 100644 | ||
index 000000000..c0fe00c0e | ||
--- /dev/null | ||
+++ b/CMake/FindVTKm.cmake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems redundant given the CONFIG
passed to the find_package(VTKm)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the problems I had was to understand when VTK requires a FindModule.cmake
for an external dependency and when not. I think I got an error without it telling me that there is no module for it and I just made one minimal on up. Probably because I did not use vtk_module_find_package
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just let find_package
do its precedence. CMake 3.17's --debug-find
should help figure out why it isn't being found (or at least where CMake is looking). I guess VTKm's config file was not found out-of-the-box without this? All vtk_module_find_package
does is let the module system know it was looked for, does sanity checks for a few things, and puts the find_package
into the vtk-config
code path.
STRING(REGEX REPLACE "/W[0-4]" "/W4" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}") | ||
else() | ||
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4") | ||
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4 /bigobj") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think VTKm is working on a fix for this. @robertmaynard
- find_package(VTKm | ||
- PATHS "${CMAKE_CURRENT_LIST_DIR}/vtkm" | ||
- NO_DEFAULT_PATH) | ||
+ find_package(VTKm CONFIG REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole block can probably go away if vtk_module_find_package
is used.
@Neumann-A Thanks. I've done a pass over the code here. |
closes #11135
most changes are extracted from #9960
should close the following:
closes #9211
closes #9200
closes #9213
closes #9023
might close #9322
vtk-m did not test any features.
vtk build with
vtk[atlmfc,core,paraview,python,qt,vtkm]:x64-windows