Skip to content

[ensmallen] Fix usage #38515

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

Thomas1664
Copy link
Contributor

@Thomas1664 Thomas1664 commented May 1, 2024

  • 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.

Comment on lines -9 to -10
"blas",
"lapack",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dependencies of armadillo, not direct dependencies

@WangWeiLin-MV WangWeiLin-MV added the category:port-bug The issue is with a library, which is something the port should already support label May 6, 2024
Copy link
Contributor

@WangWeiLin-MV WangWeiLin-MV left a comment

Choose a reason for hiding this comment

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

The port usage tests pass with the following triplets:

  • x64-windows

@WangWeiLin-MV WangWeiLin-MV added the info:reviewed Pull Request changes follow basic guidelines label May 7, 2024
@dg0yt
Copy link
Contributor

dg0yt commented May 7, 2024

@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 WangWeiLin-MV removed the info:reviewed Pull Request changes follow basic guidelines label May 7, 2024
@WangWeiLin-MV
Copy link
Contributor

... 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
Copy link
Contributor

dg0yt commented May 7, 2024

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
Copy link
Contributor

WangWeiLin-MV commented May 7, 2024

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.

@WangWeiLin-MV WangWeiLin-MV marked this pull request as draft May 11, 2024 10:02
@BillyONeal
Copy link
Member

I pushed the changes that I believe @dg0yt was suggesting. @Thomas1664 does this make any sense? If not feel free to just chop off my commit and the merge.

@BillyONeal BillyONeal added the info:reviewed Pull Request changes follow basic guidelines label Aug 8, 2025
@dg0yt
Copy link
Contributor

dg0yt commented Aug 8, 2025

With CMake in vpckg being recent enough, wouldn't the best way using find_package(OpenMP) in the build (patch)?
This package interacts with other packages, and the original code might be intented to use the result of find_package(OpenMP) from a parent project (vendoring ensmallen).

@BillyONeal
Copy link
Member

With CMake in vpckg being recent enough, wouldn't the best way using find_package(OpenMP) in the build (patch)? This package interacts with other packages, and the original code might be intented to use the result of find_package(OpenMP) from a parent project (vendoring ensmallen).

I wasn't sure, but that makes sense to me. Stand by...

@BillyONeal BillyONeal marked this pull request as ready for review August 8, 2025 22:55
@BillyONeal
Copy link
Member

Hmmm.... what the port was doing worked on macOS and find_package(OpenPM does not. But looking at this again... did this ever actually turn on OpenMP at all?

The original tried:

set_property(TARGET OpenMP::OpenMP_CXX
        PROPERTY INTERFACE_COMPILE_OPTIONS ${OpenMP_CXX_FLAGS})

but as far as I can tell ${OpenMP_CXX_FLAGS} will be empty string?

@Thomas1664
Copy link
Contributor Author

I can confirm that in commit 0ba9b4e, CMake wasn't even looking for OpenMP. In Bill's latest commit, it searches for it but fails:

 Run Build Command(s): /opt/homebrew/bin/ninja -v cmTC_5dced
        [1/2] /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++   -fPIC -Xclang -fopenmp -v -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.5.sdk -MD -MT CMakeFiles/cmTC_5dced.dir/OpenMPTryFlag.cpp.o -MF CMakeFiles/cmTC_5dced.dir/OpenMPTryFlag.cpp.o.d -o CMakeFiles/cmTC_5dced.dir/OpenMPTryFlag.cpp.o -c /Users/.../dev/vcpkg/buildtrees/ensmallen/arm64-osx-rel/CMakeFiles/CMakeScratch/TryCompile-GQw9sE/OpenMPTryFlag.cpp
        FAILED: CMakeFiles/cmTC_5dced.dir/OpenMPTryFlag.cpp.o 
        /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++   -fPIC -Xclang -fopenmp -v -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.5.sdk -MD -MT CMakeFiles/cmTC_5dced.dir/OpenMPTryFlag.cpp.o -MF CMakeFiles/cmTC_5dced.dir/OpenMPTryFlag.cpp.o.d -o CMakeFiles/cmTC_5dced.dir/OpenMPTryFlag.cpp.o -c /Users/.../dev/vcpkg/buildtrees/ensmallen/arm64-osx-rel/CMakeFiles/CMakeScratch/TryCompile-GQw9sE/OpenMPTryFlag.cpp
        Apple clang version 17.0.0 (clang-1700.0.13.5)
        Target: arm64-apple-darwin24.5.0
        Thread model: posix
        InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
        ignoring nonexistent directory "/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1"
         "/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang" -cc1 -triple arm64-apple-macosx15.0.0 -Wundef-prefix=TARGET_OS_ -Wdeprecated-objc-isa-usage -Werror=deprecated-objc-isa-usage -Werror=implicit-function-declaration -emit-obj -disable-free -clear-ast-before-backend -disable-llvm-verifier -discard-value-names -main-file-name OpenMPTryFlag.cpp -mrelocation-model pic -pic-level 2 -mframe-pointer=non-leaf -fno-strict-return -ffp-contract=on -fno-rounding-math -funwind-tables=1 -fobjc-msgsend-selector-stubs -target-sdk-version=15.5 -fvisibility-inlines-hidden-static-local-var -fdefine-target-os-macros -fno-assume-unique-vtables -fno-modulemap-allow-subdirectory-search -target-cpu apple-m1 -target-feature +zcm -target-feature +zcz -target-feature +v8.5a -target-feature +aes -target-feature +altnzcv -target-feature +ccdp -target-feature +complxnum -target-feature +crc -target-feature +dotprod -target-feature +fp-armv8 -target-feature +fp16fml -target-feature +fptoint -target-feature +fullfp16 -target-feature +jsconv -target-feature +lse -target-feature +neon -target-feature +pauth -target-feature +perfmon -target-feature +predres -target-feature +ras -target-feature +rcpc -target-feature +rdm -target-feature +sb -target-feature +sha2 -target-feature +sha3 -target-feature +specrestrict -target-feature +ssbs -target-abi darwinpcs -debugger-tuning=lldb -fdebug-compilation-dir=/Users/.../dev/vcpkg/buildtrees/ensmallen/arm64-osx-rel/CMakeFiles/CMakeScratch/TryCompile-GQw9sE -target-linker-version 1167.5 -v -fcoverage-compilation-dir=/Users/.../dev/vcpkg/buildtrees/ensmallen/arm64-osx-rel/CMakeFiles/CMakeScratch/TryCompile-GQw9sE -resource-dir /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/17 -dependency-file CMakeFiles/cmTC_5dced.dir/OpenMPTryFlag.cpp.o.d -skip-unused-modulemap-deps -MT CMakeFiles/cmTC_5dced.dir/OpenMPTryFlag.cpp.o -sys-header-deps -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.5.sdk -internal-isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.5.sdk/usr/include/c++/v1 -internal-isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.5.sdk/usr/local/include -internal-isystem /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/17/include -internal-externc-isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.5.sdk/usr/include -internal-externc-isystem /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include -Wno-reorder-init-list -Wno-implicit-int-float-conversion -Wno-c99-designator -Wno-final-dtor-non-final-class -Wno-extra-semi-stmt -Wno-misleading-indentation -Wno-quoted-include-in-framework-header -Wno-implicit-fallthrough -Wno-enum-enum-conversion -Wno-enum-float-conversion -Wno-elaborated-enum-base -Wno-reserved-identifier -Wno-gnu-folding-constant -fdeprecated-macro -ferror-limit 19 -stack-protector 1 -fstack-check -mdarwin-stkchk-strong-link -fblocks -fencode-extended-block-signature -fregister-global-dtors-with-atexit -fgnuc-version=4.2.1 -fno-cxx-modules -fskip-odr-check-in-gmf -fcxx-exceptions -fexceptions -fmax-type-align=16 -fcommon -fopenmp -clang-vendor-feature=+disableNonDependentMemberExprInCurrentInstantiation -fno-odr-hash-protocols -clang-vendor-feature=+enableAggressiveVLAFolding -clang-vendor-feature=+revert09abecef7bbf -clang-vendor-feature=+thisNoAlignAttr -clang-vendor-feature=+thisNoNullAttr -clang-vendor-feature=+disableAtImportPrivateFrameworkInImplementationError -D__GCC_HAVE_DWARF2_CFI_ASM=1 -o CMakeFiles/cmTC_5dced.dir/OpenMPTryFlag.cpp.o -x c++ /Users/.../dev/vcpkg/buildtrees/ensmallen/arm64-osx-rel/CMakeFiles/CMakeScratch/TryCompile-GQw9sE/OpenMPTryFlag.cpp
        clang -cc1 version 17.0.0 (clang-1700.0.13.5) default target arm64-apple-darwin24.5.0
        ignoring nonexistent directory "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.5.sdk/usr/local/include"
        ignoring nonexistent directory "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.5.sdk/System/Library/SubFrameworks"
        ignoring nonexistent directory "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.5.sdk/Library/Frameworks"
        #include "..." search starts here:
        #include <...> search starts here:
         /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.5.sdk/usr/include/c++/v1
         /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/17/include
         /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.5.sdk/usr/include
         /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include
         /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.5.sdk/System/Library/Frameworks (framework directory)
        End of search list.
        /Users/.../dev/vcpkg/buildtrees/ensmallen/arm64-osx-rel/CMakeFiles/CMakeScratch/TryCompile-GQw9sE/OpenMPTryFlag.cpp:2:10: fatal error: 'omp.h' file not found
            2 | #include <omp.h>
              |          ^~~~~~~
        1 error generated.
        ninja: build stopped: subcommand failed.
        
      exitCode: 1

@dg0yt
Copy link
Contributor

dg0yt commented Aug 10, 2025

This is how we deal with openmp on osx:

suitesparse-graphblas[openmp](osx) = feature-fails # No openmp on default osx toolchain

i.e. allow it to be used with other toolchains.

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:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants