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 flags profiles #498

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

kubajj
Copy link
Contributor

@kubajj kubajj commented Jun 18, 2021

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

@awvwgk awvwgk self-requested a review June 18, 2021 15:44
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 had a quick look over the patch and already a couple of questions/comments.

manifest-reference.md Outdated Show resolved Hide resolved
Comment on lines +221 to +223
if (size(key_list).ge.1) then
do ikey=1,size(key_list)
key_name = key_list(ikey)%key
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have to iterate over a list of keys here? All required key names in a profile are known at compile time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is supposed to be ready for further error handling. We had a discussion about whether an error should occur if the table contains other key names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I think that this is the easiest way how to deal with the error handling of the fields as they are. They do not need to be defined, but if they are, they need to be just key-value pairs.

Copy link
Member

Choose a reason for hiding this comment

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

This still needs to be addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still think that this is the easiest solution, if you want to suggest anything else, please do.

src/fpm/manifest/profiles.f90 Outdated Show resolved Hide resolved
src/fpm/manifest/profiles.f90 Outdated Show resolved Hide resolved
src/fpm/manifest/profiles.f90 Outdated Show resolved Hide resolved
src/fpm/manifest/profiles.f90 Outdated Show resolved Hide resolved
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.

Fantastic work Jakub @kubajj! You've achieved a lot so far! I've left a few comments on some things that stand out to me at this stage. We can discuss these at our upcoming meeting, or at an additional one before you head off.

src/fpm_targets.f90 Outdated Show resolved Hide resolved
src/fpm_targets.f90 Outdated Show resolved Hide resolved
src/fpm_model.f90 Outdated Show resolved Hide resolved
src/fpm_targets.f90 Outdated Show resolved Hide resolved
manifest-reference.md Outdated Show resolved Hide resolved
manifest-reference.md Outdated Show resolved Hide resolved
src/fpm/dependency.f90 Outdated Show resolved Hide resolved
src/fpm/dependency.f90 Outdated Show resolved Hide resolved
awvwgk
awvwgk previously requested changes Jul 31, 2021
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.

The profile priority check is fragile, instead of checking the fpm output, it should fail to compile or create a runtime error if the profiles are not used correctly to produce a reliable regression test.

ci/run_tests.sh Outdated Show resolved Hide resolved
example_packages/profiles_priorities/d1/fpm.toml Outdated Show resolved Hide resolved
@awvwgk
Copy link
Member

awvwgk commented Aug 10, 2021

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.

@kubajj kubajj changed the title Draft - Compiler flags profiles Compiler flags profiles Aug 10, 2021
@kubajj
Copy link
Contributor Author

kubajj commented Aug 10, 2021

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.

src/fpm/manifest/profiles.f90 Outdated Show resolved Hide resolved
src/fpm/manifest/profiles.f90 Outdated Show resolved Hide resolved
@awvwgk
Copy link
Member

awvwgk commented Aug 11, 2021

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

@kubajj
Copy link
Contributor Author

kubajj commented Aug 11, 2021

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.

@LKedward
Copy link
Member

... 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?

@kubajj
Copy link
Contributor Author

kubajj commented Aug 11, 2021

... 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
Copy link
Contributor Author

kubajj commented Aug 12, 2021

... 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.

@awvwgk
Copy link
Member

awvwgk commented Aug 25, 2021

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.

@kubajj
Copy link
Contributor Author

kubajj commented Aug 25, 2021

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.

@everythingfunctional
Copy link
Member

@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.

@kubajj
Copy link
Contributor Author

kubajj commented Sep 17, 2021

@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.

@awvwgk
Copy link
Member

awvwgk commented Sep 17, 2021

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
Copy link
Member

awvwgk commented Sep 20, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
specification Issue regarding fpm manifest and model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants