Skip to content

Conversation

leonfoks
Copy link

I've updated the CMAKE build procedure to a similar setup that I have in Coretran.

Parsed out compiler flag options into different files for easier reading
Added DEBUG and RELEASE mode with DEBUG default.
Moved test dir to /bin
Moved .mod files to /include
Moved built libraries (.o .a .dylib etc) to /lib
Added OpenMP finder (we might not need this thought)
Added new directories to gitignore.

CMake is a pain for people who are not used to it.
So I print extra information to the screen explaining the options chosen,
the flags being used etc. and how to change these options.

Added more information when using CMAKE to help users when compiling.

TODO: Add a compile flags file for PGI
TODO: Add a compile flags file for CRAY? I can do this.
TODO: Add information about how to change CMAKE_MAXIMUM_RANK - Easy

Added DEBUG and RELEASE mode with DEBUG default.
Moved test dir to /bin, .mod files to /include, and .o .a .dylib to /lib
Added OpenMP finder
Added more information when using CMAKE to help users when compiling.
@leonfoks
Copy link
Author

leonfoks commented Mar 13, 2020

Obviously, this is all subject to change. We can discuss paths for mod files etc.

I've added extra flags for a DEBUG build which might be causing a build failure. But it's the best way to develop the lib from the start with harsh warnings IMHO

@jvdp1
Copy link
Member

jvdp1 commented Mar 14, 2020

CMake is a pain for people who are not used to it.
So I print extra information to the screen explaining the options chosen,
the flags being used etc. and how to change these options.

Added more information when using CMAKE to help users when compiling.

I can't jugde the changes, but this one is useful. Thanks!

Copy link
Member

@scivision scivision left a comment

Choose a reason for hiding this comment

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

Just a few modern CMake comments.

if(NOT CMAKE_MODULE_PATH)
set(CMAKE_MODULE_PATH)
endif()
list(APPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake")
Copy link
Member

Choose a reason for hiding this comment

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

I think line 10 should move to line 8 as

set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake")


# Setup the Fortran cmake environment
# Sets up the flags for compiling shared libraries or static libraries, and returns the libType parameter.
include("${CMAKE_MODULE_PATH}/FortranEnvironment.cmake")
Copy link
Member

Choose a reason for hiding this comment

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

no need for " quotes here


enable_language(Fortran)

option (BUILD_SHARED_LIBS "Shared or static libraries" ON)
Copy link
Member

@scivision scivision May 24, 2020

Choose a reason for hiding this comment

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

Default shared libs don't work in general for Fortran compilers like Intel on Windows

# ================================
find_package(OpenMP)
if(OPENMP_FOUND)
set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} ${OpenMP_Fortran_FLAGS}")
Copy link
Member

Choose a reason for hiding this comment

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

these two lines should be like

string(APPEND CMAKE_Fortran_FLAGS " ${OpenMP_Fortran_FLAGS}")

# Set the output directories for compiled libraries and module files.
# Paths are relative to the build folder where you call cmake
# ================================
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/../lib) # Static library location
Copy link
Member

Choose a reason for hiding this comment

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

To make it clear these lines are creating side effects (creating artifacts outside of the build directory) would it be possible to do like:

set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${PROJECT_SOURCE_DIR}/lib)

Copy link
Member

Choose a reason for hiding this comment

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

Very good point. I think, it is generally not a nice idea to create anything outside of the build folder (not even in the source folder), unless the user explicitly asks for it. So, why not something like

set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/_install/lib)

and the user can override it, in case it should be put somewhere else.

# ================================
if(BUILD_SHARED_LIBS)
# Add any shared library related stuff here
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -shared -fpic")
Copy link
Member

Choose a reason for hiding this comment

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

I think it's preferable to use CMake POSITION_INDEPENDENT_CODE

# Add any shared library related stuff here
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -shared -fpic")

if(LINUX OR APPLE)
Copy link
Member

Choose a reason for hiding this comment

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

if(UNIX)

set(CMAKE_FIND_LIBRARY_SUFFIXES ".a")
find_library(LIB_QUADMATH quadmath)

if ("${LIB_QUADMATH}" STREQUAL "LIB_QUADMATH-NOTFOUND")
Copy link
Member

Choose a reason for hiding this comment

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

if(NOT LIB_QUADMATH)

endif()

set(CMAKE_Fortran_FLAGS_RELEASE "${CMAKE_Fortran_FLAGS_RELEASE} -O3 -funroll-all-loops -finline-functions")
set(CMAKE_Fortran_FLAGS_DEBUG "${CMAKE_Fortran_FLAGS_DEBUG} -O0 -fbacktrace -fbounds-check -Waliasing -Wampersand -Wconversion -Wsurprising -Wc-binding-type -Wintrinsics-std -Wtabs -Wintrinsic-shadow -Wline-truncation -Wtarget-lifetime -Wreal-q-constant")
Copy link
Member

Choose a reason for hiding this comment

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

string(APPEND CMAKE_Fortran_FLAGS_DEBUG " ....")

message(STATUS "Getting ifort flags")

if(WIN32)
set(CMAKE_Fortran_FLAGS_RELEASE "${CMAKE_Fortran_FLAGS_RELEASE} -nologo -fpp -O3 -heap-arrays1024 -QaxCORE-AVX2,CORE-AVX-I,AVX,SSE4.2,SSSE3 -Qipo -fp:fast=2 -Qdiag-disable:remark -Qmkl")
Copy link
Member

Choose a reason for hiding this comment

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

Normally CMake handles these flags fine specifically for Intel compiler, including on Windows. What about older CPUs? I typically use for Intel compiler -march=native or /arch:native instead of all these flags.

Note: For Gfortran, -march=native does not work in general, -mtune=native should be used instead.

@certik
Copy link
Member

certik commented May 24, 2020

The CI fails on Windows, so that has to be fixed.

It adds a lot more complexity to the CMake, hundreds of new lines to maintain. I feel we should just let cmake do its thing, and keep the number of lines that we have to write and maintain to a minimum.

And ultimately, we are hoping to switch to the Fortran Package Manager for stdlib once it is ready, and fpm will be able to automatically generate the CMake files.

fortran-lang/fpm#69

@milancurcic
Copy link
Member

@leonfoks How would you like to proceed with this PR? Can you make it work on Windows? There are still quite a few unresolved suggestions.

Also, can you reduce the added code to a minimum, as per @certik's suggestion?

I wish I could provide more feedback here, but CMake is really not in my wheelhouse.

@leonfoks
Copy link
Author

leonfoks commented Jul 6, 2020

@milancurcic yes i can get this working on windows i think. sorry for the delay, Paternity leave!

@milancurcic
Copy link
Member

@leonfoks Great to hear and congrats (I have a 4-month old next to me as I type this)! No rush.

@awvwgk awvwgk added the waiting for OP This patch requires action from the OP label Apr 17, 2021
@awvwgk
Copy link
Member

awvwgk commented May 31, 2021

@leonfoks, what is the current status of this patch? The CMake files got a couple of modifications from my work of making it reusable in subprojects, which are currently conflicting with this patch. To move this pull request forward it would require a rebase against the current default branch first.

@milancurcic
Copy link
Member

I will close this for now, @leonfoks please re-open when you're ready to continue working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for OP This patch requires action from the OP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants