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

CMakeLists: fixed default build type, added option for static build #35

Merged
merged 2 commits into from
May 15, 2022

Conversation

Honza-R
Copy link
Contributor

@Honza-R Honza-R commented May 9, 2022

Small improvements in CMakeLists

Issue: When CMAKE_BUILD_TYPE was not provided by the user (the variable was empty), cmake defaulted to a debug build. As a result, the default compilation run without any options (and that's what most users will do) yields a binary with lower performance.

Solution: The default value is set depending on ENABLE_COVERAGE which correctly defaults to OFF. Therefore, in no options are set by user, the build type is set to "Release". Additionally, if both options are set, conflict between CMAKE_BUILD_TYPE=Release and ENABLE_COVERAGE=ON is checked and yields an error.

Additional feature: I've added a new option STATIC_BUILD for building a static binaries. The default is OFF, and the script behaves the same as before. The static compilation works well with gfortran/OpenBlas but fails with Intel/MKL.

Status

  • Ready for merge

@codecov-commenter
Copy link

codecov-commenter commented May 15, 2022

Codecov Report

Merging #35 (de67917) into main (47634e0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #35   +/-   ##
=======================================
  Coverage   68.23%   68.23%           
=======================================
  Files         330      330           
  Lines       71589    71589           
=======================================
  Hits        48852    48852           
  Misses      22737    22737           

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 47634e0...de67917. Read the comment docs.

@godotalgorithm godotalgorithm merged commit f35ce9f into openmopac:main May 15, 2022
@godotalgorithm
Copy link
Collaborator

I generally welcome changes to MOPAC's CMake build system to improve user friendliness that don't break any existing use cases.

However, I'm trying to comply with the latest CMake standards of target-specific options rather than global options, so I've made some changes to your PR. I'm using multiple target_link_options commands instead of modifying CMAKE_EXE_LINKER_FLAGS. The CMAKE_FIND_LIBRARY_SUFFIXES change is in a gray area here - it is a global option, but I am not aware of an equivalent target option and it seems to be reset between projects anyways to avoid problems.

This should function exactly like your original PR, and if it doesn't please open an Issue or file another PR as needed.

@Honza-R Honza-R deleted the cmake_script_fix branch December 21, 2022 10:26
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