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

[exprtk] Update ExprTk #29665

Closed
wants to merge 1 commit into from
Closed

[exprtk] Update ExprTk #29665

wants to merge 1 commit into from

Conversation

ArashPartow
Copy link
Contributor

@ArashPartow ArashPartow commented Feb 15, 2023

this PR is waiting for the following PR to be merged: #29955

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 93895b28ea7bc8cda10f156c5d336f3fc070f8b1 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/e-/exprtk.json b/versions/e-/exprtk.json
index 473d7cc..8ca616b 100644
--- a/versions/e-/exprtk.json
+++ b/versions/e-/exprtk.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "664a8f5fac8595c858e17ba9fed7192cca8e0ba7",
+      "version-date": "2023-01-01",
+      "port-version": 2
+    },
     {
       "git-tree": "f643b034afeb28a7e39b6556eb78594aa49181a4",
       "version-date": "2022-01-01",

@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label Feb 15, 2023
ports/exprtk/vcpkg.json Outdated Show resolved Hide resolved
@JonLiu1993
Copy link
Member

Please get failure logs here. Please ping me if you need any help.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 93895b28ea7bc8cda10f156c5d336f3fc070f8b1 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 3086fce..550a2c9 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -2302,7 +2302,7 @@
     },
     "exprtk": {
       "baseline": "2023-01-01",
-      "port-version": 2
+      "port-version": 0
     },
     "ezc3d": {
       "baseline": "1.4.7",
diff --git a/versions/e-/exprtk.json b/versions/e-/exprtk.json
index 473d7cc..8842ca8 100644
--- a/versions/e-/exprtk.json
+++ b/versions/e-/exprtk.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "9a56c38bc29871ea06f03153e11df006bb05d3cc",
+      "version-date": "2023-01-01",
+      "port-version": 0
+    },
     {
       "git-tree": "f643b034afeb28a7e39b6556eb78594aa49181a4",
       "version-date": "2022-01-01",

@ArashPartow
Copy link
Contributor Author

ArashPartow commented Feb 15, 2023

@JonLiu1993 I'm not sure how to handle the following error as it seems to be related to another package and not ExprTk:

##[error]PowerShell wrote one or more lines to the standard error stream.
##[error]REGRESSIONS:
REGRESSION: vtk:x64-linux failed with BUILD_FAILED. If expected, add vtk:x64-linux=fail to /agent/_work/1/s/scripts/azure-pipelines/../ci.baseline.txt.

https://dev.azure.com/vcpkg/public/_build/results?buildId=85406&view=logs&j=f79cfdd7-47a8-597f-8f57-dc3e21a8f2ad&t=da63cf11-1f12-503b-6d64-3b2fb713c172&l=7479

Any ideas on how to proceed?

@JonLiu1993
Copy link
Member

vtk depends on exprtk, and the update of exprtk makes the port that depends on it cannot find the "2.7" version port when looking for exprtk

-- Could NOT find ExprTk: (Required is at least version "2.7") (found D:/installed/x86-windows/include)
CMake Error at CMake/vtkModule.cmake:4579 (message):
  Could not find the ExprTk external dependency.
Call Stack (most recent call first):
  CMake/vtkModule.cmake:5173 (vtk_module_find_package)
  CMake/vtkModule.cmake:5044 (vtk_module_third_party_external)
  ThirdParty/exprtk/CMakeLists.txt:1 (vtk_module_third_party)

@6ziv
Copy link
Contributor

6ziv commented Feb 15, 2023

Do not update version files manually. Instead, run vcpkg x-add-versions

@JonLiu1993 I'm not sure how to handle the following error as it seems to be related to another package and not ExprTk:

##[error]PowerShell wrote one or more lines to the standard error stream.
##[error]REGRESSIONS:
REGRESSION: vtk:x64-linux failed with BUILD_FAILED. If expected, add vtk:x64-linux=fail to /agent/_work/1/s/scripts/azure-pipelines/../ci.baseline.txt.

https://dev.azure.com/vcpkg/public/_build/results?buildId=85406&view=logs&j=f79cfdd7-47a8-597f-8f57-dc3e21a8f2ad&t=da63cf11-1f12-503b-6d64-3b2fb713c172&l=7479

Any ideas on how to proceed?

Seems like a bug in vtk. In "CMake/FindExprTk" there is a hard-coded search for a regex starting with "static const char* ", however exprtk changed that to "static char_cptr version" in a recent commit.
Maybe we can make a fix by removing that search (exprtk version number seems meaningless: it is always 2.71828.)

@ArashPartow
Copy link
Contributor Author

@6ziv - yeah I agree it seems to be a kludge to pull the version... Also VTK seems have its own copy of ExprTk:

https://github.com/Kitware/VTK/tree/7c3269c99905a1511934a9f30ecf8e5e3873c680/ThirdParty/exprtk/vtkexprtk

@spyridon97 would you happen to have any suggestions for this issue? Perhaps VTK could pull ExprTk via vcpkg?

@6ziv
Copy link
Contributor

6ziv commented Feb 15, 2023

@ArashPartow I'd suggest patching the vtk port like this attachment.
FindExprTk.patch

Maybe we can also patch ExprTk (maybe put the original line in comment, so it can still be matched by cmake regex search?), but it seem so strange...

@spyridon97
Copy link

spyridon97 commented Feb 15, 2023

In VTK we have a third party repo of exprtk https://gitlab.kitware.com/third-party/exprtk, in this repo we ahve the master branch which basically the same thing as https://github.com/ArashPartow/exprtk (at some point when we pulled) and the for/vtk branch in which we add some very small changes as you can see here so that it can be understood as an internal vtk module without conflict https://gitlab.kitware.com/third-party/exprtk/-/commits/for/vtk.

@ArashPartow Would you like us to synchronize our master branch with your github branch again and rebase our for/vtk branch based on the updated master branch?

@ArashPartow
Copy link
Contributor Author

ArashPartow commented Feb 15, 2023

@spyridon97 Agreed, I think it would be a good idea to update the VTK's ExprTk repo.
However the main problem seems to be coming from here:

https://github.com/Kitware/VTK/blob/master/CMake/FindExprTk.cmake#L23-L25

file(STRINGS "${ExprTk_INCLUDE_DIR}/exprtk.hpp" _exprtk_version_header
  REGEX "static const char\\* version")
string(REGEX MATCH "static const char\\* version = \"([0-9.]+)\"" _exprtk_version_match "${_exprtk_version_header}")

The whole regex approach to getting the version needs to be rethought, at the very least the regex should perhaps be in the form of:

static .*version \= \"2\.7([0-9.]+)\"

The following PR should resolve the above issue:

https://gitlab.kitware.com/vtk/vtk/-/merge_requests/9967

@ArashPartow ArashPartow closed this by deleting the head repository Feb 21, 2023
This was referenced Feb 21, 2023
@ArashPartow ArashPartow reopened this Feb 24, 2023
@ArashPartow ArashPartow reopened this Feb 25, 2023
@JonLiu1993
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ArashPartow ArashPartow closed this Mar 1, 2023
@ArashPartow ArashPartow reopened this Mar 1, 2023
@ArashPartow ArashPartow mentioned this pull request Mar 1, 2023
5 tasks
@ArashPartow
Copy link
Contributor Author

@Cheney-W when you merge #29955 can you also look into merging this PR.

ports/exprtk/vcpkg.json Show resolved Hide resolved
ports/exprtk/portfile.cmake Show resolved Hide resolved
@JonLiu1993
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JonLiu1993
Copy link
Member

Depends on #29955.

@JonLiu1993 JonLiu1993 added the depends:different-pr This PR or Issue depends on a PR which has been filed label Mar 6, 2023
@ArashPartow
Copy link
Contributor Author

ArashPartow commented Mar 6, 2023

@JonLiu1993 I see, ok can we please have #30058 be merged

@ArashPartow ArashPartow mentioned this pull request Mar 7, 2023
5 tasks
@JonLiu1993
Copy link
Member

@JonLiu1993 I see, ok can we please have #30058 be merged

@ArashPartow, thanks for your contribution, don't worry, if ci passes, we will merge as soon as possible after review.

@JonLiu1993 JonLiu1993 removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Mar 28, 2023
@JonLiu1993
Copy link
Member

Pinging @ArashPartow for response. Is work still being done for this PR?

@PhoebeHui PhoebeHui added the depends:different-pr This PR or Issue depends on a PR which has been filed label Mar 30, 2023
@PhoebeHui
Copy link
Contributor

Depends on #30058.

@JonLiu1993
Copy link
Member

Convert this PR to draft since there is no progress. Please ping us if this PR is ready for review again.

@JonLiu1993 JonLiu1993 marked this pull request as draft May 5, 2023 02:49
@dan-shaw
Copy link
Contributor

Closing this PR since there is no progress for a few months. Feel free to open a new PR if you are still working on it

@dan-shaw dan-shaw closed this Jun 10, 2023
@PhoebeHui
Copy link
Contributor

Hi @ArashPartow, I can't reopened this PR, since the Repo had been deleted, please submit a new PR instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision depends:different-pr This PR or Issue depends on a PR which has been filed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants