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

Compiler and flags #220

Merged

Conversation

everythingfunctional
Copy link
Member

This slightly modifies the command line options of the bootstrap version to be more in line with the Fortran version, and adds options to the bootstrap version to allow specifying the compiler and flags.

Note: There is a change to the name of the build directory. It now includes hashes that are unique to compiler version and flags used. This is in some ways a breaking change, because even our CI scripts were reliant on the name(s) we were using, even though we never promised this would be consistent or guaranteed.

I view this partly as just an example of how this could be done that our users can play with, not necessarily exactly how it should be implemented in the Fortran version.

@everythingfunctional
Copy link
Member Author

The main implications for the way the logic is currently implemented is that, if you specify anything other than the exact string "gfortran" as the compiler (which is the default value), then no default flags are provided. Also, if you specify any compiler flags, then only those specified are actually used (aside from ones like -c, -o, etc that are included as required for the build to work). I'm open to suggestions about if there is a better way to do this, but my feeling is that whatever is specified on the command line should override any other settings. I believe that's how most tools usually work.

@everythingfunctional
Copy link
Member Author

Actually, hold on merging this one for a bit. The -- args part isn't working right yet.

- this allows the '--' to signify that all remaining arguments are to 
the test/executable
@everythingfunctional
Copy link
Member Author

Ok, fpm test -- args --to test now works properly

@urbanjost
Copy link
Contributor

A vexing issue. Fortran and C compilers in many cases are primarily a "text file to machine instructions" converter, but usually require special flags for profiling, loading auxiliary libraries, special flags best for use with debuggers, ... but usually have little or no support for packaging and IDE types of interfaces (or fpm would not be needed). So ideally there needs to be a way to specify compiler commands right in the TOML file so a package can use things like X11 libraries or readline or any other libraries installed on a machine. And yet there will always be a special case where you want direct control of compiler options. Should we just supply the file names in dependency order for that case? Should we just allow you to specify a "compiler" name and assume that iit s a script you have set up to do the compile that just needs the filenames? Compiler switches are more standarized than at any time in the past but even the assumed parameters like -c and -o and -I and -J may not do quite what you want when automatically built.

So far in the h-fpm version the packager can create a custom build with a toml.mk file and the use of a few environment variables but there is nothing on the user side that is equivalent on the command line. And even with the commands put into the TOML files it is not clear if it would work to have a base compile command and supplimental keyword/option pairs like "debugger" and "profile" or whether it is just best to allow "complete" commands to be specified with different names. So I think we need a way to do this through the TOML file so special options can be packaged; I think some things might best be handled by just supplying the filenames in the project with a more complete "fpm build -list" output ;
but can still think of scenarios where a compiler option is needed. Not sure that should not be done by specifying a build script name like the h-fpm toml file allows with toml.mk though. But cannot think of any "perfect" solution so it seems like, as you said, this is a good idea to add and try.

So far I haven't seen anything better in any package manager I looked at that would apply to Fortran (or C/C++). My gut feeling is that we need to have something that is "packageable" too and this is more like a back door to let you specify on-the-fly options more than a way to "make a distributable package" but I have been wanting something like this almost every time I try to make an FPM package myself; at least during development. Just getting it pulled down and built now. So here I am just - but I think it sounds like something to try.trying to capture my first thoughts about alternatives before I get too involved in the nuts and bolts of how this works just to further the discussion

@urbanjost
Copy link
Contributor

maybe something else to think about would be a --dryrun that would just echo all the execute_system_command commands instead of doing them to let you create a script for a custom build or a switch to write a "fpm.mk" file are supplemental approachs?

@urbanjost
Copy link
Contributor

I think --release and --flag should be mutually exclusive after trying it a bit. -- and --flag -value have worked, which are both tricky to implement. Is it intended that a --flag only take one option? Should I be able to say --flag '-g -O3 -p ....'?

"--release --compiler NAME" makes sense but --release presumes certain options and using --flag seems to set options to just those specified so I was not sure what " --release --flag OPTION" would do;

fpm build --release --compiler f95 --flag '-g' --flag '-O3'

Copy link
Member

@LKedward LKedward left a comment

Choose a reason for hiding this comment

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

Thanks @everythingfunctional, this looks great! I will definitely be using this functionality straight away for different compiler versions! I agree with @urbanjost that being able to specify flags in the manifest is desirable (#191 (comment)) but this PR is a good step forward.

I agree that we should move away from directly addressing output executables. Despite this I still have a very minor aesthetic complaint which is it would be preferable for the hash values to be output in hex instead of decimal.

I notice that if the compiler has key-value flags where the value is the next positional argument, then you have to specify --flag key --flag value which is odd, but this doesn't affect any gfortran options I don't think. I would prefer being able to specify all compiler arguments within a single space delimited quoted string, but this may not be feasible.


It looks like we can't quite support different compiler vendors just yet - I tried with ifort but you have to add a flag to specify module location, which then changes the module location due to the flags hash etc.

I think that compiler support for major compilers will have to be built-in to fpm in order to support various flags such as module location etc, but this is for future work.

@everythingfunctional
Copy link
Member Author

@urbanjost , thanks for your comments. As illustrated by #112, #191 the "packaging" of compiler options is rather complicated, and this effectively just sidesteps all of that to provide something people can play with for now.

Yes, --dry-run would be a useful option. I can probably get to it within the next few weeks. Not sure exactly how easy it will be yet.

Also, I agree that --release and --flag should be mutually exclusive, I just haven't added in the error checking logic yet. In fact, anything other than --compiler gfortran should invalidate --release also, to make the internal logic more apparent to the user.

@everythingfunctional
Copy link
Member Author

everythingfunctional commented Oct 29, 2020

@LKedward , I agree with you that hash values should probably be output in hex. I'll see if I can figure out to do that shortly.

That's disappointing about ifort. I was really hoping the -I and -J flags were the "standard" way of specifying module locations. It very much complicates the build logic. As far as specifying all the flags at once, something like this should work --flag '-Wall -Wextra ...', since multiple flags are just stuck together with strings and stuck as-is into the command.

@awvwgk
Copy link
Member

awvwgk commented Oct 29, 2020

To make the module output directory automatic fpm should be able to detect the compiler it is using. There seem to be at least five six different commands for modules (according to meson: https://github.com/mesonbuild/meson/blob/master/mesonbuild/compilers/fortran.py), with -module being the most common.

  • Intel, PGI, Pathscale, Flang: -module
  • G95: -fmod
  • Gnu: -J
  • Sun: -moddir=
  • NAG: -mdir
  • Lahey: -M

@everythingfunctional
Copy link
Member Author

@awvwgk , eesh. I was hoping we wouldn't have to detect which compiler. It's probably better that we do, as it will allow us to always provide a default set of flags.

Also, those are the flags for "where to put the produced *.mod" files. Do you know the equivalent flags for "where else to look for *.mod" files?

@awvwgk
Copy link
Member

awvwgk commented Oct 29, 2020

The include directory command is of course not consistent between compiler. Fortunately, most compilers use -I with the exception of the Sun Fortran compiler, which is using -M, and the Lahey compiler, which uses --mod instead, as far as I can see.

@everythingfunctional
Copy link
Member Author

Are -c and -o universal?

@awvwgk
Copy link
Member

awvwgk commented Oct 29, 2020

Looks like they are. I have access to PGI/NVIDIA, Intel, NAG and Lahey, so I could run some tests or provide some example outputs for --version if required. Fortunately, we don't have to learn everything from scratch, we should take the knowledge embedded in CMake and/or meson for the compiler handling.

@everythingfunctional
Copy link
Member Author

Great. Definitely useful to try and leverage the CMake and meson examples. Hopefully that info isn't too difficult to tease out of their code base or documentation.

@awvwgk
Copy link
Member

awvwgk commented Oct 29, 2020

I linked the Fortran compilers meson knows about above, they are all well contained in one file and the Python code is readable enough. For CMake I'm not volunteering to search for this information.

@LKedward
Copy link
Member

As far as specifying all the flags at once, something like this should work --flag '-Wall -Wextra ...', since multiple flags are just stuck together with strings and stuck as-is into the command.

Hmm, okay it looks like fpm is passing the flag string to the shell command including quotes:

$ fpm build --flag '-Wall -Wextra'
Command line: gfortran -c -Jbuild/gfortran_70f5c1d2b5b6190e_459e958fab3e1c6b/fhash '-Wall -Wextra' -o build/gfortran_70f5c1d2b5b6190e_459e958fab3e1c6b/fhash/src_fhash_fnv.f90.o src/fhash_fnv.f90
Exit code: 1
Stderr:
gfortran: error: unrecognized command-line option ‘-Wall -Wextra’

...

@awvwgk
Copy link
Member

awvwgk commented Oct 31, 2020

@everythingfunctional I created #223 to track the available compilers and their different flavours of command line flags.

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

I personally don't like the option to specify the compile flags via command line, it quickly becomes cumbersome to carry into every subcommand and more importantly, in case I forget to specify it, a complete rebuild of the entire source tree will be triggered.

Wouldn't it be easier to specify the flags in the build table in fpm.toml instead?

@everythingfunctional
Copy link
Member Author

I'm not particularly enamored with it either, since you're correct that it gets quite cumbersome, but I don't know of a much better solution. I don't think we want to specify them in the fpm.toml file until we've developed some compiler agnostic way of doing it, and know exactly how they interact between packages.

@ianabc
Copy link

ianabc commented Nov 7, 2020

This is exciting to see, passing multiple --flag options covers my use-case for now (coarrays).

@awvwgk
Copy link
Member

awvwgk commented Nov 9, 2020

After looking around a bit I found cargo-rustc, I like the idea of cargo to separate a build and a compile mode, maybe we should copy this approach and implement a separate command (fpm compile?), which allows this for fpm, but leave the build command deterministic by only using arguments from the package manifest (see #112).

@everythingfunctional
Copy link
Member Author

@awvwgk , that's an interesting idea. However, we would need additional options to run and test to allow specifying that they run the version(s) built with such a command.

@everythingfunctional
Copy link
Member Author

I've added the error check suggested regarding the mutually exclusive options, and done some refactoring to make it easier to add support for more compilers. I'll give everybody a few days to take another look, but I think with some support and approval already, this is ready to merge in.

@awvwgk
Copy link
Member

awvwgk commented Nov 11, 2020

However, we would need additional options to run and test to allow specifying that they run the version(s) built with such a command.

cargo-rustc seems to implement run and test mode inside the rustc subcommand again to keep everything contained.

Another option might be to generate a lock file with the used compile flags which is also used for run and test if present, this requires to set the flags only once with build. Once the flags are changed in build the lock file will be updated as well.

The file could be something human-readable like TOML:

fortran-compiler = "gfortran"
fortran-compile-args = [
  "-O3",
# ...
]

Maybe the lock file should be placed in the build directory? One should not be able to check it in into the version control system, I think.

@everythingfunctional
Copy link
Member Author

I think having the compile (or some other name) subcommand reimplement build, test and run allowing flags would be the most self consistent and easiest to explain. I.e.

fpm [compile [--flag FLAG]] build|run|test ...

I don't like the idea of a lock file, as it introduces implicit, global state and makes it more complicated to explain. I.e.

...
fpm run
...
fpm run

Without knowing what the ...'s are, you don't know if the two invocations of fpm run will do the same thing.

Either way I think it's something that should be explored in a future PR.

@everythingfunctional
Copy link
Member Author

Without any other significant comments that I think need addressed here, and some requests from some people funding this effort to make it available, I'm going to go ahead and merge this now.

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.

None yet

5 participants