-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
[itk] update to 5.4.0 #39757
[itk] update to 5.4.0 #39757
Conversation
Module_ITKCudaCommon never exists
And is off by default
Should I add commit from #39778 ? and close the other PR ? |
#39778 can duplicate to this PR. |
Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review". |
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.
The rest looks good to me. Please mark ready for review when you've responded.
diff --git a/Modules/Core/GPUCommon/CMakeLists.txt b/Modules/Core/GPUCommon/CMakeLists.txt | ||
index da2d66b63..6fb476680 100644 | ||
--- a/Modules/Core/GPUCommon/CMakeLists.txt | ||
+++ b/Modules/Core/GPUCommon/CMakeLists.txt | ||
@@ -24,9 +24,13 @@ if(ITK_USE_GPU AND APPLE AND NOT ITK_COMPILER_HAS_BLOCKS) | ||
endif() | ||
|
||
if(ITK_USE_GPU) | ||
+ message(STATUS "OPENCL_LIBRARIES:${OPENCL_LIBRARIES}") | ||
+ if(TARGET OpenCL::OpenCL) | ||
+ message(STATUS "OPENCL_LIBRARIES:${OpenCL_LIBRARIES}") | ||
+ endif() | ||
set(ITKGPUCommon_LIBRARIES ITKGPUCommon) | ||
- set(ITKGPUCommon_SYSTEM_INCLUDE_DIRS ${OPENCL_INCLUDE_DIRS}) | ||
- set(ITKGPUCommon_SYSTEM_LIBRARY_DIRS ${OPENCL_LIBRARIES}) | ||
+ set(ITKGPUCommon_SYSTEM_INCLUDE_DIRS ${OpenCL_INCLUDE_DIRS}) | ||
+ set(ITKGPUCommon_SYSTEM_LIBRARY_DIRS ${OpenCL_LIBRARIES}) | ||
endif() | ||
|
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.
It looks like you removed the meaningful content in this patch. The rest of the changes looks like its just removing a space between the if and the parenthesis. Is this patch still needed?
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.
I removed this part because ITK replaced OPENCL_INCLUDE_DIRS
by OpenCL_INCLUDE_DIRS
in Modules/Core/GPUCommon/CMakeLists.txt
. But it's the only place where they did it.
The change about the space is only in the context of the patch. ITK removed them in the source code. But it's still needed to replace OPENCL_INCLUDE_DIRS
by OpenCL_INCLUDE_DIRS
.
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.
If you read the patch (instead of the diff of the patch), the highlight syntax is really more readable.
@@ -4,7 +4,7 @@ index 16c611fd3..13978724c 100644 | |||
+++ b/Modules/ThirdParty/DoubleConversion/CMakeLists.txt | |||
@@ -9,7 +9,16 @@ mark_as_advanced(ITK_USE_SYSTEM_DOUBLECONVERSION) | |||
if(ITK_USE_SYSTEM_DOUBLECONVERSION) | |||
find_package(double-conversion REQUIRED) | |||
find_package(double-conversion 3.1.6 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.
We don't typically add versions to the find_package calls. Is there a specific reason for 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.
A diff in a patch is unfriendly to read. I didn't add 3.1.6. It's ITK. This line is part of the context of the patch. It's not one of the modified lines.
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.
Next time, I will add more explanation about modification of patches. For example, I removed the patch about C++17 because they already did it. |
Thank you @bansan85 and apologies for the confusion! |
Fixes #34663
./vcpkg x-add-version --all
and committing the result.