-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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 ExprTk cmake patch #29801
[VTK] VTK ExprTk cmake patch #29801
Conversation
@ LilyWangLL / @JonLiu1993 / @FrankXie05 when you have a moment can you please review and merge this PR. |
@JavierMatosD Looks like "microsoft.vcpkg.pr (x86_windows) " needs to be rerun, I don't have perms to rerun that part, can you please trigger it.
|
@JonLiu1993 / @ LilyWangLL / @FrankXie05 if there's nothing else can this be merged as the following PR depends on this being merged first: #29665 |
-# find_package(ExprTk 2.71828) | ||
-# to require version 2.71828 or newer of ExprTk. | ||
+# find_package(ExprTk 2.7) | ||
+# to require version 2.7 or newer of ExprTk. | ||
# |
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 find_package call needs to be patched out to remove the version constraint entirely.
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 a comment in the CMake.
+ file(STRINGS "${ExprTk_INCLUDE_DIR}/exprtk.hpp" _exprtk_version_header REGEX "\"[0-9.]+\"") | ||
+ set(ExprTk_VERSION) | ||
+ foreach (_exprtk_version_line IN LISTS _exprtk_version_header) | ||
+ if ("${ExprTk_VERSION}" STREQUAL "") | ||
+ string(REGEX MATCH "version = \"(2\.7[0-9.]+)\".*$" _exprtk_version_match "${_exprtk_version_line}") | ||
+ set(ExprTk_VERSION "${CMAKE_MATCH_1}") | ||
+ else () | ||
+ string(REGEX MATCH "\"([0-9.]+)\".*$" _exprtk_version_match "${_exprtk_version_line}") | ||
+ set(ExprTk_VERSION "${ExprTk_VERSION}${CMAKE_MATCH_1}") | ||
+ endif () | ||
+ if (_exprtk_version_match MATCHES "\;") | ||
+ break() | ||
+ endif () | ||
+ endforeach () | ||
+ if (NOT ExprTk_VERSION) | ||
+ # fallback: version in exprtk.hpp has always started with 2.7 | ||
+ set(ExprTk_VERSION "2.7") | ||
+ 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.
Is there a reason we need to know the version? I believe this should be patched out to remove any version constraints.
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.
@JavierMatosD I'm not sure about the answer to that as I am not familiar with the VTK build setup. For more details there is a discussion here: https://gitlab.kitware.com/vtk/vtk/-/merge_requests/9981
My objective is to get this PR merged to unblock the following PR: #29665 that has been around for some time now. As there are several projects I know of that use ExprTk via vcpkg and do not require/depend on VTK that are waiting on the version bump.
@JavierMatosD in short I'm happy for you to improve this particular cmake as a separate PR. Even better yet if you have any ideas on how to remove the dependency on this PR https://gitlab.kitware.com/vtk/vtk/-/merge_requests/9981 requiring VTK to build as that would make life much easier for all invovled.
@LilyWangLL / @JonLiu1993 / @FrankXie05 / @MonicaLiu0311 / @BillyONeal If there is nothing else that needs doing can we please merge this PR.
This patch is intended to unblock the following PR: #29665
and is based on this VTK MR: https://gitlab.kitware.com/vtk/vtk/-/merge_requests/9981