Enable profiles in toml#653
Conversation
|
@kubajj and I are trying to take another run at the compiler profiles, submitting PRs in smaller increments. Could one or two people please perform a review of this one? |
|
Thanks @kubajj and @everythingfunctional - I'll put some time to review this this weekend. |
LKedward
left a comment
LKedward
left a comment
There was a problem hiding this comment.
Thanks Jakub @kubajj! I left a few minor comments, but my main feedback is that it would be good to have some more detailed description comments for some of the more complex subroutines to describe their logic for future developers.
Also, should the changes to the manifest reference be included in this PR?
Other than that this all looks good to me and I'm happy to move forward with the rest 👍
| use fpm_toml, only : toml_table, toml_array, toml_key, toml_stat, get_value, & | ||
| & len | ||
| use fpm_versioning, only : version_t, new_version | ||
| use fpm_filesystem, only: join_path |
src/fpm/manifest/profiles.f90
Outdated
| if (.not.(present(profiles).and.present(profindex))) then | ||
| call fatal_error(error, "Both profiles and profindex have to be present") | ||
| return | ||
| end if |
There was a problem hiding this comment.
Is this an internal error or can it be triggered by the user? In case this is user visible (let's assume it is), how helpful would that message be for fixing the manifest?
There was a problem hiding this comment.
It is an internal error. What should I do with it?
There was a problem hiding this comment.
So if the routine cannot be called with the optional arguments absent, can't we just always require them to be present?
There was a problem hiding this comment.
Oh, the thing is, the function operates in two modes, one is for just getting to know how many profiles there are if I am correct and the other actually writes to the profiles array.
That's where the line 403 and 431 comes in with if (present(profiles_size)).
There was a problem hiding this comment.
I see. There are 3 optional arguments. The procedure should be called with either just profiles_size present, or both profiles and profindex present. Would it make sense to split this into 2 procedures where neither has optional arguments?
There was a problem hiding this comment.
@everythingfunctional @awvwgk In the latest commit, I have split the function into two. One is traverse_oss_for_size the other is just traverse_oss. I tried to remove all unnecessary variables. Let me know what you think.
src/fpm/manifest/profiles.f90
Outdated
| if (.not.(present(profiles).and.present(profindex))) then | ||
| call fatal_error(error, "Both profiles and profindex have to be present") | ||
| return | ||
| end if |
There was a problem hiding this comment.
Same here, is this a user visible and does it provide the required information to help the user fix the manifest?
There was a problem hiding this comment.
Again, this is an internal error.
|
@LKedward about the changes to the manifest being included in the PR. I was thinking that since the profiles are not used yet, then we should not include it so that it does not confuse anybody. |
Of course, good point! |
|
This looks like it could be merged, no? |
|
Was there a reason this didn't get merged, or was it just forgotten about? I managed to resolve the merge conflict without much issue. Any chance we can go ahead and merge it now? |
The original PR #653 was opened before C++ support was added. This PR brings the profiles manifest parsing up to date.
5 participants
Enable compiler profiles to be loaded into the package. Profiles are not used in any way yet. Tests in the
test/fpm_test/test_manifest.f90are making sure the functionality is behaving as expected. This is first part of #498 to be merged as it is difficult to merge such a big PR.This is still a huge PR as the file
src/fpm/manifest/profiles.f90is a completely new file with a lot of functionality in it.