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 handling of OpenMP. #77

Merged
merged 3 commits into from
Jun 25, 2022
Merged

Fix handling of OpenMP. #77

merged 3 commits into from
Jun 25, 2022

Conversation

susilehtola
Copy link
Collaborator

Fixes #76 .

Status

  • Ready for merge

@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2022

Codecov Report

Merging #77 (1610c3d) into main (54f56b5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #77   +/-   ##
=======================================
  Coverage   68.23%   68.23%           
=======================================
  Files         330      330           
  Lines       71696    71696           
=======================================
  Hits        48925    48925           
  Misses      22771    22771           
Impacted Files Coverage Δ
src/input/wrtkey.F90 64.13% <ø> (ø)
src/run_mopac.F90 78.04% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54f56b5...1610c3d. Read the comment docs.

@godotalgorithm
Copy link
Collaborator

I'm a little bit hesitant to accept this, as I removed the use of find_package(OpenMP) in #74 because find_package(BLAS) and find_package(OpenMP) were finding different OpenMP libraries, and I was inadvertently linking the two libraries, which was causing weird problems. However, you are only using the find_package system to set OpenMP compiler flags here and not additionally linking an OpenMP library, which might be the correct middle ground. For normal usage of OpenMP, you would only normally specify the compiler flag anyways. The additional linking of libiomp5 is a specific nuance of the Intel MKL library that I don't fully understand.

@godotalgorithm
Copy link
Collaborator

godotalgorithm commented Jun 25, 2022

This is just some nitpicking. I'm trying to be consistent about using target-based CMake settings rather than global settings whenever possible. Also, the case of xxx_FOUND is supposed to match the name of the package or library that you are looking for. However, OPENMP_FOUND instead of OpenMP_FOUND is such a common typo that it was explicitly added to the find_package(OpenMP) script as a redundant status variable.

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.

MOPAC fails to link
3 participants