[ensmallen] Fix usage - Githubissues

microsoft / vcpkg

C++ Library Manager for Windows, Linux, and MacOS

MIT License

21.76k stars 6.05k forks source link

[ensmallen] Fix usage #38515

Open Thomas1664 opened 2 weeks ago

Thomas1664 commented 2 weeks ago

[x] Changes comply with the maintainer guide.
[x] 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.~
[x] Any patches that are no longer applied are deleted from the port's directory.
[x] The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
[x] Only one version is added to each modified port's versions file.

dg0yt commented 1 week ago

@WangWeiLin-MV I am aware of what FindOpenMP provides. The point is that find_dependency(OpenMP) may fail and not create the targets, while the "old method" in CMakeLists.txt will always create the targets. So it may be possible to build the port without OpenMP, but not to use it. This is bad.

WangWeiLin-MV commented 1 week ago

... The point is that find_dependency(OpenMP) may fail and not create the targets, while the "old method" in CMakeLists.txt will always create the targets. So it may be possible to build the port without OpenMP, but not to use it. This is bad.

@dg0yt Thanks for your explanation. The upstream uses an old version of CMake, and it seems that there is no simple way to detect.

@Thomas1664 It might be a safety workaround that disable the option USE_OPENMP.

dg0yt commented 1 week ago

vcpkg use a new version of CMake to build that port. It is not necessary to disable OpenMP. I just ask for using the same pattern during build and usage.

WangWeiLin-MV commented 1 week ago

vcpkg use a new version of CMake to build that port. It is not necessary to disable OpenMP. I just ask for using the same pattern during build and usage.

@dg0yt How about using the new FindOpenMP and skip the original target creation:

# Find OpenMP and link it.
if(USE_OPENMP)
+  find_package(OpenMP REQUIRED)
+  if(0)
-  if(NOT TARGET OpenMP::OpenMP_CXX)

The local test passes, but I haven't test it without omp.