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

Implement --flag option for Fortran fpm #390

Merged
merged 9 commits into from Mar 22, 2021

Conversation

awvwgk
Copy link
Member

@awvwgk awvwgk commented Mar 14, 2021

Exploratory implementation of --flag and --profile for Fortran fpm.

  • --profile takes either debug or release, defaults to debug but yields against --flag
  • --profile does not check it's argument, but saves this it in the settings,
    this allows the model to potentially reuse it for reading the compilation profile from the package manifest
  • --flag takes one argument which is forwarded to the compile arguments in the model
  • --flag arguments are replacing all compile arguments if no profile is specified
  • --flag arguments are appended if a compilation profile (debug or release) is present
  • compiler identification works now also for gfortran-10 or powerpc64le-conda-linux-gnu-gfortran

Closes #389

@awvwgk awvwgk marked this pull request as ready for review March 17, 2021 21:21
@awvwgk
Copy link
Member Author

awvwgk commented Mar 17, 2021

Fails due to #327

character(*), intent(in) :: path
character(:), allocatable :: canon
!! FIXME: Lot's of ugly hacks following here
function canon_path(path)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not proud of what I've done here, but it seems to get the job done for now.

Choose a reason for hiding this comment

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

I think adding in ugly hacks would make me less nervous if you also added some unit tests. 😉

- default is equal to debug unless --flag is present and overwrites all arguments
- debug is the developement profile, options given by --flag are appended
- release is the production profile, options given by --flag are appended
@LKedward
Copy link
Member

This is looking really great @awvwgk!

I get a failure when passing flags without a profile:

~/git/fpm_packages/slsqp$ fpm build --flag '-O3'                                                                                              
 + gfortran -c ./src/slsqp_kinds.f90-O3 -J build/gfortran_A05A18BF4FD7B592/slsqp -I build/gfortran_A05A18BF4FD7B592/slsqp -o build/gfortran_A05A18BF4FD7B592/slsq
p/src_slsqp_kinds.f90.o                                                                                                                                          
gfortran: error: ./src/slsqp_kinds.f90-O3: No such file or directory                                                                                             
gfortran: fatal error: no input files                                                                                                                            
compilation terminated.
 Command failed
ERROR STOP

It looks like we were previously relying on compile_flags starting with a space, but really we should just explicitly put a space between the the filename and the compile flags in the backend call:

fpm/fpm/src/fpm_backend.f90

Lines 241 to 242 in 8cbbd80

call run(model%fortran_compiler//" -c " // target%source%file_name // target%compile_flags &
// " -o " // target%output_file)

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.

This all looks good to me. Great job, cheers 👍

@LKedward LKedward linked an issue Mar 18, 2021 that may be closed by this pull request
@awvwgk
Copy link
Member Author

awvwgk commented Mar 19, 2021

Let's have a few more eyes look over this PR before merging it.

@everythingfunctional
Copy link
Member

I think is very much a step in the right direction. Thanks for putting it together.

@urbanjost
Copy link
Contributor

================================================================================
SUGGESTED CHANGES:

help text needs to explain how --profile and --flag interact, and --flags 
often appears where --flag should be.  Suggest something like

 --profile PROF  selects the compilation profile for the build.
                 Currently available profiles are 'release' for
                 high optimization and 'debug' for full debug options.
                 If --flag is not specified the 'debug' flags are the
                 default. 
 --flag  FFLAGS  selects compile arguments for the build. These are
                 added to the profile options if --profile is specified,
                 else these options override the defaults.
                 Note object and .mod directory locations are always
                 built in.

Everywhere where --profile and --flag are described.

Instead of duplicating the descriptions, maybe something short in run(1) and
test(1) saying "see 'help build' for a detailed description"(?).

If would be useful if you came back to your package that you could dehash
the directory names or see them from a cache by entering something like

   fpm build --profile --list

or something like that. Remembering all the options one built with and in
what order would be something I would not be good at. Somewhere in the
documentation it has to explain that is how you do a combination of run|test|build
and use the same build subdirectory.

If the build names were uuencoded or used a reversible hash
the directory names could contain the options used to built them and could
be listed.
================================================================================
LONGER TERM:

Since one must repeat options on run(1) and test(1) to exactly match build(1),
which is difficult (That was the impetus for the previously proposed
-ID option PR) I'll repeat this previous proposal for a spec when -ID
seemed insufficient.

First, this was how -ID worked:

   --flag could only be specified on build(1) and --ID also had to be
   specified when --flag was present, which was used to make the build
   subdirectory name and recorded the flags in a file in the directory,
   and then you could say --ID NAME on build(1) and run(1) and test(1)
   subsequently and it would reuse the same options. If --flag was used
   a new build from scratch was started so you could not reuse the name
   with multiple builds and end up with a build with multiple options
   used on different parts (although that might be considered a feature,
   not a flaw!). It was planned to also add an option to show what names
   and matching flags had been used.

-ID was considered insufficient and by itself did not provide for
packaging the options used and the previous PR was dropped. 

I found having to respecify --flag exactly on build|run|test in the
Haskell version (which this PR basically duplicates) hard to use and
generally ended up having to wipe any previous build and then put the
options into a shell variable so I could repeat them, or build a script
for each combination that I had so I could come back to a project and
pick up where I was. I did not like that except it was better than no
option, which is the current state in f-fpm.

So a subsequent proposal which did not get any comment went something
like this:

There is a separate file(s) called .fpmprofile or a section of the
fpm.toml file that is maintained automatically that would be used to
define names for compiler-specific options and generic profile names.

If the fpm.toml file is used it would be copied as-is till the
"[[profile]]" section is encountered to leave comments and other
user-specified data intact, as most users would not want that
automatically maintained.

Using the fpm.toml file is nice and clean and easy to package for use as a
remote dependency and "self-contained" but more complicated to break into
a user-edited section and an automatically maintained section and would
required TOML, whereas separate files could be any format and could even
be searched for to reduce duplication (but then hard to package). That is,
you could have one in your home directory with your favorite options. For
packaging I would suggest against that; but perhaps there could be an
option to load the fpm.toml file with your favorite options (from a
another TOML file) and/or with the fpm "built-ins".

You would use a separate command to build option aliases something like

    fpm option profile --flag '-pg' [--compiler gfortran|$COMPILER] [--description 'turn on profiling for use with gprof']

or as simply as

    fpm option free --flag '-ffree-form -ffree-line-length-none' 

The first time it runs it might populate with default
option aliases built into fpm, that perhaps would have a reserved prefix like
"fpm_" that the user should not change; or alternatively they could stay
build in but there would be some way to list them so you could easily use
them to build a custom profile.

you can then build a profile using option aliases

    fpm profile free profile fpm_debug --name myopts [--compiler gfortran|$COMPILER]

Using aliases makes it so the profiles are more compiler-independent.

You could then use the profile name on build, run, and test, which would
cause a "unique" build directory name created from a hash of the options,
as done in this version.

There would be a special user-definable name called "package" that would
be used when the package is built as a dependency when present.

alias names could also be specified for files, but not profiles. If
present, they would assume to be always required for those files.

So assuming most users would just use --profile debug and --profile
release and that they are built it and that debug was the default 
usage would not change much except users would use --profile release
where they previously used --release.

If they wanted to use something like the -pg flag they would enter
(assuming gfortran compiler):

    fpm alias profile_flags --flag '-pg'
    fpm alias free --flag '-ffree-form'
    fpm profile profile_flags free --name myopts

from that point on they could enter
    fpm build --profile myopts

They could specify the option names for files (and directories or as
global defaults or with name globbing?)  in the manifest file as well.

If you used a profile name and the aliases were not defined you would get a warning
(they might be defined for gfortran but not for ifort, for example).

The profile subcommand could have a --list option, which would show the built-in
names too.

The --flag option would still exist but users would be strongly encouraged to
not require it for packages.
================================================================================
So I would say this can go forward, and I will use it because right now
I think the lack of adding flags is the biggest issue in using fpm more generally,
but I would prefer it be done more like the -ID proposal.

And then I think we all need to work on a spec for a method that can be used
in a packageable manner; the above being my first thoughts on that. Skipping
the -ID mode and going straight to the packageable proposal could be left 
upward-compatible with this PR. 

So I think this can go forward with the help text corrections, with those
caveats.

@awvwgk
Copy link
Member Author

awvwgk commented Mar 20, 2021

@urbanjost Thanks for the feedback. I agree, in the long term we don't want to rely on the --flag option at all. Dehashing is an interesting idea, we could use the cache.toml to store the compiler flags associated with the respective hash which is straight-forward in TOML:

[flag.gcc]
0x2A42023B310FA28D = " -Wall -Wextra -Wimplicit-interface -fPIC -fmax-errors=1 -g -fcheck=bounds -fcheck=array-temps -fbacktrace -fcoarray=single"
0x5F75F7C92365B9B9 = " -O3 -Wimplicit-interface -fPIC -fmax-errors=1 -funroll-loops -fcoarray=single"

The ID solution is interesting, but I don't like the idea of introducing a global state dependency in the build system which might impact reproducible builds.

@awvwgk
Copy link
Member Author

awvwgk commented Mar 21, 2021

Thanks for the feedback. Unless something is blocking this patch I'll go ahead and merge this later today.

@awvwgk awvwgk merged commit 9842deb into fortran-lang:master Mar 22, 2021
@awvwgk awvwgk deleted the fun-with-flags branch March 22, 2021 07:58
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.

Implement --flag option in Fortran fpm problems with building with local paths
4 participants