-
Notifications
You must be signed in to change notification settings - Fork 108
Add --compiler switch #255
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
Conversation
added called *--fc* that sets the Fortran compiler name. + The default compiler name to use is taken from the value of the environment variable FPM_FC. + If not set, the environment variable FC is used. + If it is not set the name _gfortran_ is used. + The value specified on the command line overrides any default. FC is apparently a commonly used environment variable for the compiler, is short, and allows for specifying compilers for other languages like C (ie. `-cc gcc`) That is part of the reasons to use `--fc` instead of `--compiler`. Since there is a chance the currently set value of FC may be used by other applications being used, and to allow for a temporary change of the default the fpm-specific variable FPM_FC is supported in addition to FC and has higher precedence. However, since the Haskell version has a similar switch called **--compiler** that name is an alias for **-fc**. A skeleton was started for standard debug and release builds that allows for compilers other than **gfortran**. I now have access to **ifort** and **nvfortran** and it works with at least simple cases for those compilers. Looking for someone with access to other compilers to help flesh that out. The list of executables to run with the `run` and `test` subcommands can now be prefixed with **--target**, as with the Haskell version of ffpm(1). So default usage is unchanged from the previous version. By simply setting FC or FPM_FC you can use it like the previous version with other compilers for default debug and release builds. How does that sound? wget http://www.urbanjost.altervista.org/REMOVE/ffpm.f90 gfortran ffpm.f90 -o $HOME/.local/bin/ffpm ``` # get a test package or use your own if [ ! -d M_CLI2 ] then git clone https://github.com/urbanjost/M_CLI2 fi cd M_CLI2 # default build using ifort ffpm build --fc ifort export FPM_FC=ifort ffpm run ffpm test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR and sorry for joining the discussion late.
Overall, I recommend using only one name for env variables and compiler switches, rather than having aliases that do the same thing and implicitly used env variables.
I agree with @awvwgk's earlier argument that FC
is commonly used in other tools and would not be surprising to the user here. It's short and familiar. At the same time, I also see value for fpm to use only env variables prefixed with FPM_
, so that the user always knows for sure what is used and not used. Prefixing all fpm env variables with FPM_
also allows for easy listing of currently set fpm-related variables by doing env | grep FPM_
, and a peace of mind that nothing else is being used. These things are easy to forget.
Ditto for the flag. Pick --fc
or --compiler
, but not both. There's no need to initially introduce both and bloat the UI. The more we have to explain the fpm CLI, the harder it will be to use, which is against fpm's vision. It's harder to remove things later than to add them later if needed. I like --fc
better.
I think the precedence order of settings (flag > env variable > default) is good.
Remove white-space changes from compiler branch for PR
After trying it for seveal days while trying to load in as many packages as I could find FC being used, I find it is used heavily, but inconsitently. CMake uses it in an interesting way where the name can be followed by compiler options that are used to set the options for a build, which might be an approach to adapt for custom builds not specified via a config file/TOML file setting. Make of course uses it, and is probably where the "standard" use of FC orginated. Several environment modules such as one for nvfortran use it with a full pathname to the executable. This version handles this by using the basename of the first word of FC; but since the usage is common but varies in meaning significantly I think FPM_FC is needed. If not, as in this implementation, as an override of FC then the cleanest single-option solution seems to be FPM_FC and --fc or FPM_COMPILER and --compiler. Possibly a short name for --compiler would also be desireable. Since the name --fc implies some association with the FC environment variable that is not strictly true in the first scenario, I think it comes down to the second choice. Another factor in favor of that is that --compiler is more compatible with h-fpm. So, although not my personal first choice. I think that it will be --compiler COMPILER_NAME where the default compiler name is taken from the environment variable FPM_COMPILER if set, else 'gfortran' is used. I still find the use of an environment variable for establishing a default useful so I do not propose dropping that. Any dissents or alternatives? |
Having thought some more and read your message, I agree and now prefer |
CMake also supports reading compile flags from
In case of conda-build the
Using the basename of the compiler found in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @urbanjost, this looks good. I agree with @milancurcic to not introduce CLI aliases and instead just pick one. I prefer --compiler
because it is more explicit than --fc
and I don't consider it a common requirement that environment variables must match command line switches.
With that said, one thing that we should keep in mind is how to specify a c
compiler. (Currently we simply pass c
code to gfortran
and ignore it's complaints - this will change). We can implicitly infer the c
compiler based on our internal table of supported Fortran compilers, however we should also be able to specify the c
compiler separately as well.
Maybe:
FPM_F_COMPILER
and--f-compiler
FPM_C_COMPILER
and--c-compiler
in the future?
In the long run, compiler-specific treatment should be separated out of the fpm
module and possibly abstracted behind an abstract type; however for the time-being the current implementation is fine.
I agree with @milancurcic that they should be |
fpm/src/fpm.f90
Outdated
! Open64 ? ? -module -I -mp discontinued | ||
! Unisys ? ? ? ? ? discontinued | ||
|
||
select case(settings%build_name//'_'//settings%compiler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move most of the compiler logic to a separate module (src/fpm/compiler.f90
maybe), this would keep the toplevel fpm module clean from this kind of logic and we have a dedicated place to expand on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was thinking same thing; was thinking of defining a compiler type type instead of select; but as you mentioned just moving it to a seperate module simplifies that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the file fpm_compiler.f90 was added with a routine to set default compiler flags that encapsulates that function for
further expansion.
The only change to the user interface is the addtion of the --compiler COMPILER_NAME switch and the use of the environment variable FPM_COMPILER to select the default compiler. The full value of COMPILER_NAME is used as the compiler command (allowing for full pathnames) but the basename of that pathname must match one of the pre-defined compiler names and that name is used to create the build directory name. In general (for now) the names are essentially
limited to those in fpm_compiler.f90
No mechanism for the user to set custom flags is included.
Encoding the possible full pathname that might be in FPM_COMPILER in a way similiar to what h-fpm does for compiler flags could be a way to allow for a more generic mechanism for creating compiler-specific directories.
No use is made of the FC and FFLAGS variables or for allowing user-specific compiler options.
For now, it is assumed COMPILER is a Fortran Compiler. There is the question of how best to allow for a list of encoders for a possible list of supported languages.
In fpm_compiler.f90 if user-defined type is created that defines a compilers options it would be simpler to be able to print a table of the types to be able to show users the acceptable names and their flags but currently there is no way to query what compilers are supported. Either way if anyone has suggested flag values for the various compilers included let me know.
The verbose switch is available for any command, although I didn't add much logic to use it. One question is whether the external commands should only echo if the --verbose switch is on; or conversely if there should be a --quiet switch (I see several other packages have both a -v|-V an a -q switch to allow for quiet, normal, and verbose).
experiences
As is, I find it very useful to build using multiple compilers. I find I tend to use the variable and aliases fpm-i, fpm-n, and fpm-g for three different compilers myself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@urbanjost since you mentioned that you are testing several compilers with aliases. Maybe you know about response files supported by some command line tools, I opened an issue at M_CLI2 regarding this urbanjost/M_CLI2#5. This could allow to store compiler flags in files rather than shell aliases.
PS: Working on a test of the |
I think the latest push resolves the immediate concerns and sets up for a later reconfig of the compiler options in an OOP structure as a possible later change, but segregates this all into fpm_compiler.f90 for now. So does anyone see any remaining issues? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @urbanjost, this works nicely and implementation looks fine. Only left a few comments regarding changes to the help texts.
I agree with you that in the long run a derived type to encapsulate compilers would be beneficial, but this is good for now.
Co-authored-by: Laurence Kedward <[email protected]>
Co-authored-by: Laurence Kedward <[email protected]>
Co-authored-by: Laurence Kedward <[email protected]>
Co-authored-by: Laurence Kedward <[email protected]>
Co-authored-by: Laurence Kedward <[email protected]>
Co-authored-by: Laurence Kedward <[email protected]>
Co-authored-by: Laurence Kedward <[email protected]>
Co-authored-by: Laurence Kedward <[email protected]>
Co-authored-by: Laurence Kedward <[email protected]>
Co-authored-by: Laurence Kedward <[email protected]>
Co-authored-by: Laurence Kedward <[email protected]>
run
, test
, and build
Fortran fpm(1) command has a new switchThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks @urbanjost; this looks good 👍
Since ithe conditional compilation PR was closed I wondered if you saw that note on the code still checking whether to build if the executable exists for run and test? Now that conditional compilation is implemented do you want that removed?
I added a note to the PR or issue (do not remember) but I think it was actually closed so not sure if you saw that or not.
|
fpm/src/fpm_compiler.f90
Outdated
fflags = ' ' | ||
case('debug_nagfor') | ||
module_path_switch='-mdir ' | ||
fflags = ' ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking at adding nag support to the bootstrap version, and was going to use -O4
for release, and -g -Call
for debug. On a related note, we've got -fPIC
for gfortran, should that be included for all compilers that support it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking for any suggestions for what the flags should be and still want to reimplement the loader and --flags options in some form. Was waiting for this PR as do not want to built up too many diffs as the --install switch changes some of the same files. That seems like a good change. Many of these are still blank. The -fPIC was in the original but I personally think that is a good suggestion to add it where available. I will add a matching change for nagfor
. The functionality is there in this PR (I have been using it with three compilers). I would like to add short names like -c||-C or even -e (for encoder) or -F for Fortran for --compiler and -V for verbose but this PR is already a little top heavy. Sounds good. Was hoping for a lively discussion on what options to add for some of the compilers I do not have access to, but have not seen much. Hopefully you will spur a discussion on -fPIC and maybe some other options. Right now without fpm.toml supporting option selection and more arguably not having support for a custom build script and CLI options for compiling and loading getting good defaults is especially important. Some is personal taste (I dislike the default allowing C-like escape characters, for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is the following, but I have no access to test it.
module_path_switch='-mdir '
fflags = ' &
& -O4&
& -coarray=single&
& -PIC&
'
case('debug_nagfor')
module_path_switch='-mdir '
fflags = '&
& -g&
& -C=all&
& -O0&
& -gline&
& -coarray=single&
& -PIC&
'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAG will only output modules in the selected directory, but not read from there, I had to apply a quick hack to do a test with nagfor
:
diff --git a/fpm/src/fpm_compiler.f90 b/fpm/src/fpm_compiler.f90
index 540b48f..7bf5b15 100644
--- a/fpm/src/fpm_compiler.f90
+++ b/fpm/src/fpm_compiler.f90
@@ -228,6 +228,9 @@ character(len=:),allocatable :: module_path_switch
! so that values that do not require a space such as -moddir= will work
model%fortran_compile_flags = fflags//' '//&
& module_path_switch//join_path(model%output_directory,model%package_name)
+ if (compiler == "nagfor") &
+ & model%fortran_compile_flags = model%fortran_compile_flags//' '//&
+ & "-I "//join_path(model%output_directory,model%package_name)
end subroutine add_compile_flag_defaults
I know TOML-Fortran and M_CLI2 are compiling with NAG, therefore I gave compiling fpm itself a try, of course it does not work, fixing the NAG specific issues is something for another PR however.
More importantly, it might be worth to have all compilers add an include flag for their module output directory as well, in case of gfortran
it won't do any harm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was hoping for a lively discussion on what options to add for some of the compilers I do not have access to, but have not seen much.
I would not worry about having every compiler setting populated for this PR. This PR sets up the framework; additional compiler support can be added by those with access to such compilers in separate PRs. I'm happy for this PR to go forward as is.
@awvwgk's suggestion for the explicit include flag is a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. So based on that I made a slight change that incorporates that where fflags is the optional flags that a user might override and mandatory is the flags you have to add for ffpm` to work. It adds some redundancy but solves the problem with nagfor and allows more easily for compilers that do not use -I for locating modules although from the list that seems to be very few anymore.
@awvwgk and @everythingfunctional do these look like reasonable values for nagfor? I did some cursory testing with gfortran, ifort, and nvfortran and it does not look like it introduced any problems; but just guessing at the fflags
values based on the NAG webpage document. It would be nice if you can actually try them but as @LKedward said getting optimal values can wait. I would change them if they actually fail!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works in principle with nagfor
, I successfully compiled M_CLI2
with your branch (both debug and release). The main issue with NAG is that a standard violation is always considered a fatal error and is outright rejected by the compiler. So most packages won't compile with nagfor
anyway because they are not considered standard compliant. It doesn't help that NAG has a particular strict interpretation of the Fortran standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@urbanjost , I have not had any luck getting either version of fpm to run on the system I have access to with nag installed. But the flags you've suggested look good to me based on the documentation. As @awvwgk suggested, I think adding the module output directory to the include flags is the simplest way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great work on this PR. This is ready to go as far as I am concerned.
PS: having a -I module_build_directory would make #252 just require writing the TOML data and maybe a build date or even a git commit number in fortran format and sticking it in there with some name like fpm_build.inc. So that paves the way for that if/when it goes forward. |
Wondering about these additional NAG switches. Proposing
if there is time. Several imply the loosen up the strictness of the NAG compiler. Untried, but they sound useful. Running some of the code thru their |
Requested changes have been applied
Many thanks @urbanjost and thanks everyone for reviewing. With three approvals I will now merge. Regarding the additionally proposed flags for NAG, I suggest leaving them out for now since they do not match any flags requested of the other compilers. ( |
I would suggest not adding flags for NAG that reduce strictness. I know that much of NAG's draw (at least for me and some others I know) is that it catches so many more potential issues by default. Adding support for a strict compiler and then removing that strictness by default seems like conflicting intent. |
@everythingfunctional I would suggest to move the discussion about the NAG compiler support to a separate issue to make it more discoverable, now that this PR is merged. |
added new switch called --compiler that sets the Fortran compiler name.
environment variable FPM_COMPILER
--target
The list of executables to run with the
run
andtest
subcommandscan now be prefixed with --target, as with the Haskell version of
fpm(1).
SUMMARY
So default usage is unchanged from the previous version. By simply setting
FPM_COMPILER you can use it like the previous version with other compilers for
default debug and release builds.