You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The current use of HIPFORT_ prefixed setting variables is a very non-standard way to effect a number of build options. Overwriting various CMAKE_ prefixed settings prevents users from using the commonly accepted ways of specifying, compiler, flags, and install directory. This will be confusing for users that are well versed in CMake. The hard-coded default compiler path is also problematic as this does not respect environment variables like FC, which is a commonly accepted way to specify a Fortran compiler for most build systems.
Suggested changes:
Drop all HIPFORT_ prefixed variables as inputs and use them as internal variables only or
HIPFORT_COMPILER should default to being initialized from CMAKE_Fortran_COMPILER_INIT
HIPFORT_COMPILER_FLAGS should default to being initialized from CMAKE_Fortran_FLAGS_INIT
HIPFORT_BUILD_TYPE should default to being initialized from CMAKE_BUILD_TYPE (it could be made to pick release if empty)
Use set(CMAKE_Fortran_FORMAT FREE) to set source to freeform without relying on users to provide a correct compiler flag
Use CMAKE_Fortran_PREPROCESS to turn on preprocessing in all source files (available in CMake 3.18)
Use VERSION in the project function to specify version (note that having HIPFORT_VERSION as input does not make much sense as the user does not name the versions of the libray, the version should be built into the project by developers)
Please use check_fortran_compiler_flag (since CMake 3.3) for any flags you intend to hardcode. I don't know how ubiquitous the -std flag is, but the current build system only works with compilers that support specifically -std=f2008 or Cray (which means no LLVMFlang)
I am willing to provide patches for some of these changes.
Operating System
No response
GPU
No response
ROCm Component
No response
The text was updated successfully, but these errors were encountered:
I agree on all counts. The library build is a bit weird. This has been on my todo list, but there's a lot of things that are a higher priority. It would be a while before I could work on it.
I am willing to provide patches for some of these changes.
That would certainly speed up the process. I will happily review and merge PRs that make the build process more idiomatic.
Suggestion Description
The current use of
HIPFORT_
prefixed setting variables is a very non-standard way to effect a number of build options. Overwriting variousCMAKE_
prefixed settings prevents users from using the commonly accepted ways of specifying, compiler, flags, and install directory. This will be confusing for users that are well versed in CMake. The hard-coded default compiler path is also problematic as this does not respect environment variables likeFC
, which is a commonly accepted way to specify a Fortran compiler for most build systems.Suggested changes:
HIPFORT_
prefixed variables as inputs and use them as internal variables only orHIPFORT_COMPILER
should default to being initialized fromCMAKE_Fortran_COMPILER_INIT
HIPFORT_COMPILER_FLAGS
should default to being initialized fromCMAKE_Fortran_FLAGS_INIT
HIPFORT_BUILD_TYPE
should default to being initialized fromCMAKE_BUILD_TYPE
(it could be made to pick release if empty)set(CMAKE_Fortran_FORMAT FREE)
to set source to freeform without relying on users to provide a correct compiler flagCMAKE_Fortran_PREPROCESS
to turn on preprocessing in all source files (available in CMake 3.18)VERSION
in theproject
function to specify version (note that havingHIPFORT_VERSION
as input does not make much sense as the user does not name the versions of the libray, the version should be built into the project by developers)check_fortran_compiler_flag
(since CMake 3.3) for any flags you intend to hardcode. I don't know how ubiquitous the-std
flag is, but the current build system only works with compilers that support specifically-std=f2008
or Cray (which means no LLVMFlang)I am willing to provide patches for some of these changes.
Operating System
No response
GPU
No response
ROCm Component
No response
The text was updated successfully, but these errors were encountered: