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

Fix omp #39

Merged
merged 3 commits into from
Nov 18, 2022
Merged

Fix omp #39

merged 3 commits into from
Nov 18, 2022

Conversation

Vollstrecker
Copy link
Collaborator

Superseedes #34
Fixed and simplified omp detection for homebrewed libopenmp plus testing that stuff.

@smessmer
Copy link
Contributor

Is the plan to just land this or to also land #34 ? The goal of #34 is to add /opt/homebrew/opt to the path, which this PR doesn't seem to be doing. I would be surprised if this PR worked on Apple Silicon Macs. Were you able to test that?

@Vollstrecker
Copy link
Collaborator Author

Should work now also.

@abdes
Copy link
Owner

abdes commented Nov 18, 2022

This (7fbbe2e) does not work with:

❯ uname -a
Darwin Abdessattars-MacBook-Pro.local 22.1.0 Darwin Kernel Version 22.1.0: Sun Oct  9 20:15:52 PDT 2022; root:xnu-8792.41.9~2/RELEASE_ARM64_T8112 arm64

I installed libomp, and it went under /opt/homebrew/opt.

==> Pouring libomp--15.0.4.arm64_ventura.bottle.tar.gz
==> Caveats
libomp is keg-only, which means it was not symlinked into /opt/homebrew,
because it can override GCC headers and result in broken builds.

For compilers to find libomp you may need to set:
  export LDFLAGS="-L/opt/homebrew/opt/libomp/lib"
  export CPPFLAGS="-I/opt/homebrew/opt/libomp/include"

This is the output of the configure I run:

❯ cmake .. -D CMAKE_BUILD_TYPE=Debug -G Ninja -D CMAKE_MAKE_PROGRAM=ninja -D USE_CCACHE=ON -D CMAKE_VERBOSE_MAKEFILE=ON -D USE_OPENMP=ON
=> Project : cryptopp v8.7.0 (7fbbe2e27b42)
-- Using ccache (/opt/homebrew/bin/ccache) (via wrapper).
-- CPM: adding package Ccache.cmake@1.2.3 (v1.2.3)
-- Using ccache: /opt/homebrew/bin/ccache
-- downloading/updating cryptopp
-- cryptopp directory found, pulling...
From https://github.com/weidai11/cryptopp
 * branch              master     -> FETCH_HEAD
-- Crypto++ auto fetched at: /Users/abdessattar/Projects/cryptopp-cmake/_build/_deps/cryptopp
-- Adding install integration test: int-install-default
-- Adding install integration test: int-install-prefix
=> Module : cryptopp
-- [cryptopp] CMake version 3.25.0
-- [cryptopp] System Darwin
-- [cryptopp] Processor arm64
-- [cryptopp]     CMAKE_OSX_ARCHITECTURES :
-- [cryptopp] CMAKE_HOST_SYSTEM_PROCESSOR : arm64
-- [cryptopp]      CMAKE_SYSTEM_PROCESSOR : arm64
-- [cryptopp] Target architecture detected as: arm64 -> CRYPTOPP_ARMV8
-- Could NOT find OpenMP_C (missing: OpenMP_C_FLAGS OpenMP_C_LIB_NAMES)
-- Could NOT find OpenMP_CXX (missing: OpenMP_CXX_FLAGS OpenMP_CXX_LIB_NAMES)
-- Could NOT find OpenMP (missing: OpenMP_C_FOUND OpenMP_CXX_FOUND)
-- OpenMP: Applying workaround for OSX OpenMP with old cmake that doesn't have FindOpenMP
-- OpenMP: Applying workaround for old CMake that doesn't define FindOpenMP using targets
-- [cryptopp] Generating cmake package config files
-- [cryptopp] Generating pkgconfig files
-- [cryptopp] Platform: ARMv8
-- [cryptopp] Compiler definitions:  CRYPTOPP_DATA_DIR="/Users/abdessattar/Projects/cryptopp-cmake/_build/_deps/cryptopp";CRYPTOPP_ARM_NEON_HEADER=1;CRYPTOPP_ARM_ACLE_HEADER=1;CRYPTOPP_DISABLE_ARM_CRC32=1;CRYPTOPP_DISABLE_ARM_PMULL=1
-- [cryptopp] Compiler options:
-- [cryptopp] Build type: Debug
-- Configuring done
CMake Error: install(EXPORT "cryptopp_Targets" ...) includes target "cryptopp" which requires target "OpenMP_TARGET" that is not in any export set.
-- Generating done
CMake Generate step failed.  Build files cannot be regenerated correctly.

@abdes
Copy link
Owner

abdes commented Nov 18, 2022

Should work now also.

This (f00503d) works.

Please change the commit messages to meet the expectations of commitlint. Start with 'fix: ', have a short one line message, an empty line, then details of the commit changes. I use this to automatically generate the changelog 😄

Run npx commitlint --from 3d8a0a576bccb718cebd4ed97a09feb520e60641 --to f00503db7a5a3f25f274baf364f82f1896d9577a --verbose
[4](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:5)
⧗   input: Fix: Detection of libomp on Silicon Macs
[5](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:6)
✖   subject must not be sentence-case, start-case, pascal-case, upper-case [subject-case]
[6](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:7)
✖   type must be lower-case [type-case]
[7](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:8)
✖   type must be one of [build, chore, ci, docs, feat, fix, perf, refactor, revert, style, test] [type-enum]
[8](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:9)

[9](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:10)
✖   found 3 problems, 0 warnings
[10](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:11)
ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint
[11](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:12)

[12](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:13)
⧗   input: Added openmp homebrew test
[13](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:14)
✖   subject may not be empty [subject-empty]
[14](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:15)
✖   type may not be empty [type-empty]
[15](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:16)

[16](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:17)
✖   found 2 problems, 0 warnings
[17](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:18)
ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint
[18](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:19)

[19](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:20)
⧗   input: Fix detection of homebrewed libomp
[20](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:21)
✖   subject may not be empty [subject-empty]
[21](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:22)
✖   type may not be empty [type-empty]
[22](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:23)

[23](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:24)
✖   found 2 problems, 0 warnings
[24](https://github.com/abdes/cryptopp-cmake/actions/runs/3499225665/jobs/5860544849#step:5:25)
ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint

@abdes
Copy link
Owner

abdes commented Nov 18, 2022

@Vollstrecker This is good to go for a squash and merge with a conventional commit compliant message please 😄. I will leave it to you to do the merge... Thanks for your excellent contribution as usual. Thanks @smessmer for your help.

@Vollstrecker Vollstrecker merged commit f11e96a into master Nov 18, 2022
@Vollstrecker
Copy link
Collaborator Author

Done, I guess the description should be ok also, but this check doesn't seem to run on merge.

@abdes
Copy link
Owner

abdes commented Nov 18, 2022

It's ok.
I recommend you do the simple steps in https://github.com/abdes/cryptopp-cmake/blob/master/CONTRIBUTING.md#conventional-commits to make your life much easier for future contributions. Your commit messages will be locally listed immediately before you push them.

@Vollstrecker Vollstrecker deleted the fix-omp branch April 13, 2023 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants