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.

Sign up for GitHub

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

Jump to bottom

[vtk-m] Fix feature support #31750

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

[vtk-m] Fix feature support #31750

wants to merge 4 commits into from
+24 −4

Conversation

JonLiu1993
Copy link
Member

@JonLiu1993 JonLiu1993 commented Jun 1, 2023 •
edited
Loading

Fixes #13014

Fix error:

CMake Error at C:/Program Files/CMake/share/cmake-3.18/Modules/FindPackageHandleStandardArgs.cmake:165 (message):
  Could NOT find OpenMP_CXX: Found unsuitable version "2.0", but required is
  at least "4.0" (found -openmp)

Remove the version constraint of find_package, and add support like feature omp only supports linux.

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

Feature double,tbb tested successfully in the following triplet:

  • x64-windows
  • x64-windows-static

Feature mpi tested successfully in the following triplet:

  • x64-windows
  • x64-windows-static
  • x64-linux

Feature cuda and omp failed as expected
@JonLiu1993 JonLiu1993 added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Jun 1, 2023
dg0yt
dg0yt reviewed Jun 5, 2023
View reviewed changes
ports/vtk-m/fix-build-error.patch
Comment on lines +9 to +10
- find_package(OpenMP 4.0 REQUIRED COMPONENTS CXX QUIET)
+ find_package(OpenMP REQUIRED COMPONENTS CXX QUIET)
Copy link
Contributor

Choose a reason for hiding this comment

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

Upstream requires OpenMP 4.0. Did you sufficiently verify that OpenMP 2.0 is sufficient?
Copy link
Member Author

Choose a reason for hiding this comment

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

@dg0yt, I submitted an issue upstream to ask whether the minimum openmp version required by vtk-m is 4.0, and whether version 2.0 is ok.
https://gitlab.kitware.com/vtk/vtk-m/-/issues/784
Copy link
Contributor

Choose a reason for hiding this comment

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

So upstream says 4.0 means 4.0. This patch is obsolete.
Copy link
Member Author

Choose a reason for hiding this comment

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

Upstream told me to turn off VTKm_ENABLE_OPENMP ,Turning on VTKm_ENABLE_TBB means that openmp is no longer used?
Copy link
Contributor

@dg0yt dg0yt Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You can't use what isn't supported. If the compiler (MSVC) doesn't support the requirements (OpenMP 4.0) of this feature (VTKm_ENABLE_OPENMP), it means that this vcpkg port feature (openmp) is not supported for the compiler's vcpkg triplets/platforms. The only thing which needs to be done is to put this information into the manifest.
dg0yt
dg0yt reviewed Jun 26, 2023
View reviewed changes
ports/vtk-m/fix-build-error.patch
Comment on lines +9 to +10
- find_package(OpenMP 4.0 REQUIRED COMPONENTS CXX QUIET)
+ find_package(OpenMP REQUIRED COMPONENTS CXX QUIET)
Copy link
Contributor

Choose a reason for hiding this comment

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

So upstream says 4.0 means 4.0. This patch is obsolete.
ports/vtk-m/vcpkg.json
@@ -33,7 +33,8 @@
]
},
"omp": {
"description": "Use the OpenMP device adapter."
"description": "Use the OpenMP device adapter.",
"supports": "linux"
Copy link
Contributor

Choose a reason for hiding this comment

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

"supports": "!windows | mingw"
(However, it really depends on the compiler.)
Copy link
Contributor

Choose a reason for hiding this comment

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

"supports": "!msvc" would be required but vcpkg has no clue about the used compiler. Clang-cl can probably build with omp without a problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team.
4 participants
@JonLiu1993 @dg0yt @Neumann-A @Cheney-W