fpm icon indicating copy to clipboard operation
fpm copied to clipboard

Compiler flags profiles

Open kubajj opened this issue 4 years ago • 19 comments

This pull request contains all my contribution implemented during my GSoC 2021 Handling compiler arguments project.

I have implemented the following features:

  • parsing of profiles table in toml syntax
  • profiles representation in package
  • scope of flags (project-wide profiles, package-wide profiles, files scope flags)
  • organise compiled code into multiple build directories based on the flags of each source file
  • handle compiler profiles hierarchy (user-specified > parent user-specified > built-in profiles)

Changes:

  • Move the default compiler flags to profiles.f90 to the function get_default_profiles, which returns an array of profile_config_ts
  • Change 'gfortran' in fpm.90/build_model to DEFAULT_COMPILER introduced in profiles.f90, for consistency
  • Add proj_dir argument to get_package_data and new_package for file scope profiles
  • Extend package_t and package_config_t definitions with profile_config_t
  • Create all include_dirs at the beginning of build_package subroutine
  • Remove build_name from fpm_command_line and simplify check_build_vals subroutine
  • Modify fpm_compiler/get_module_flags not to include any directories, but just handle module flags
  • Add a conditional checking for allocation of fields in fpm_model - Should fix issues with show_model
  • Replace model%output_directory with function get_output_directory

New features:

  • profile_config_t and profiles toml table parser in src/fpm/manifest/profiles.f90
  • find_profile subroutine which finds a profile with given parameters in an array of profile_config_t
  • info_profile function which returns representation of a profile as a string
  • Add flags to targets in fpm_targets/build_target_list
  • Tests for profiles in test_manifest.f90
  • Example packages tested by ci/run_tests.sh

My blog on Fortran-lang discourse link to Fortran-lang discourse

kubajj avatar Jun 18 '21 15:06 kubajj

Sebastian @awvwgk - would you mind reviewing the changes in src/fpm/dependency.f90?

LKedward avatar Aug 10 '21 07:08 LKedward

I prefer to have the changes for the dependency tree in a different PR, this makes them easier to review and we will also get them merged much faster.

awvwgk avatar Aug 10 '21 09:08 awvwgk

I prefer to have the changes for the dependency tree in a different PR, this makes them easier to review and we will also get them merged much faster.

I opened a new PR #539. Hope this helps.

kubajj avatar Aug 10 '21 11:08 kubajj

I checked the manifest validation a bit and found a few interesting edge cases:

Too deep nested tables are not flagged as invalid as long as they contain valid keywords.

name = "example"
[profiles.gfortran.gfortran.gfortran.gfortran]
[profiles.gfortran.ifort.flang.lfortran]
[profiles.linux.windows.macos.freebsd]

Schema check on profile level does not catch invalid keywords:

name = "example"
[profiles.ifort]
not-supported = true

awvwgk avatar Aug 11 '21 09:08 awvwgk

I checked the manifest validation a bit and found a few interesting edge cases:

Too deep nested tables are not flagged as invalid as long as they contain valid keywords.

name = "example"
[profiles.gfortran.gfortran.gfortran.gfortran]
[profiles.gfortran.ifort.flang.lfortran]
[profiles.linux.windows.macos.freebsd]

Schema check on profile level does not catch invalid keywords:

name = "example"
[profiles.ifort]
not-supported = true

This is one of the reasons why I decided to keep the loop in get_flags, however, when we discussed it with Brad and Laurence, we decided to focus on functionality rather than covering edge cases. Right now, it does not fail, it just ignores them.

kubajj avatar Aug 11 '21 10:08 kubajj

... we decided to focus on functionality rather than covering edge cases. Right now, it does not fail, it just ignores them.

If you've got the time this week to look into addressing these cases, that would be great. It may need to be done before merging anyway. What is the main difficulty in identifying these cases?

LKedward avatar Aug 11 '21 10:08 LKedward

... we decided to focus on functionality rather than covering edge cases. Right now, it does not fail, it just ignores them.

If you've got the time this week to look into addressing these cases, that would be great. It may need to be done before merging anyway. What is the main difficulty in identifying these cases?

If I remember correctly, there were some issues with the difference between string_t and character arrays, but I'll look into it.

kubajj avatar Aug 11 '21 11:08 kubajj

... we decided to focus on functionality rather than covering edge cases. Right now, it does not fail, it just ignores them.

If you've got the time this week to look into addressing these cases, that would be great. It may need to be done before merging anyway. What is the main difficulty in identifying these cases?

If I remember correctly, there were some issues with the difference between string_t and character arrays, but I'll look into it.

I take this back, it was not difficult at all.

kubajj avatar Aug 12 '21 09:08 kubajj

I'll look into the conflicts soon-ish and than we can sort out how we will proceed to get this important patch merged smoothly.

awvwgk avatar Aug 25 '21 19:08 awvwgk

I'll look into the conflicts soon-ish and than we can sort out how we will proceed to get this important patch merged smoothly.

I can look into it tomorrow, but I need to set up fpm on my laptop first.

kubajj avatar Aug 25 '21 20:08 kubajj

@kubajj or @awvwgk , have either of you had time to look at and/or resolve the merge conflicts? I'm getting more and more anxious to start using (and see how others use) this feature. Is there something tricky about resolving them? I could potentially find some time in the next week or two to help out if needed.

everythingfunctional avatar Sep 15 '21 14:09 everythingfunctional

@kubajj or @awvwgk , have either of you had time to look at and/or resolve the merge conflicts? I'm getting more and more anxious to start using (and see how others use) this feature. Is there something tricky about resolving them? I could potentially find some time in the next week or two to help out if needed.

I am sorry for replying so late. I am currently traveling to UK to start the semester on Monday and I had limited connection. I looked into the merge conflicts, but did not manage to fix them. There are parts of the build_model subroutine which were redesigned, so I approached Sebastian who suggested me to implement issue #422 first.

kubajj avatar Sep 17 '21 20:09 kubajj

There is still an open issue with the dependency tree, I believe that it is a bad idea to introduce the parent nodes in the tree structure to fix that we are working with the flattened dependency tree, instead we should define the connectivity in with the child nodes and just walk the graph to apply the profiles. This is something I promised to look into, but haven't found the time yet to create a patch with a proper implementation.

Of course the dependency tree issue is only one part of this patch, the different build directories and the profiles for the package manifest are fine. Therefore, my suggestion was to use the build directory implementation from this patch for fixing #422 and get the first part of this project into the fpm main branch. This way we can make the changes introduced here more modular without affecting all parts of fpm at once.

awvwgk avatar Sep 17 '21 20:09 awvwgk

I started updating the build directory change from this patch in #575. Please have a look if this is sufficient to implement the compiler profiles in the target generation.

awvwgk avatar Sep 20 '21 21:09 awvwgk