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

export fpm model to TOML and JSON #879

Merged
merged 88 commits into from
Feb 25, 2024

Conversation

perazz
Copy link
Contributor

@perazz perazz commented Apr 13, 2023

As introduced in #877, we want fpm to pass its internal information to third-party software in a standardized way.

Command-line interface

  • fpm build --dump FILENAME (dumps whole model. default: fpm_model.toml)
  • fpm update --dump FILENAME (dumps updated dependency tree. default: fpm_dependencies.toml)
  • fpm export --manifest fpm.json (dumps whole manifest without building anything)
  • fpm export --dependencies deps.toml (dumps dependency tree)
  • fpm export --model model.json (dumps whole internal model)
  • FILENAME extension determines format: use JSON if .json, TOML otherwise

A full fpm_model.toml file of the fpm v0.8.1 project is here.

Relevant discussion in this weekly meeting, starting at 40:25 mins.

Key implementation points:

  • fpm_model_t and package_t are fully serialized/deserialized from TOML/JSON streams
  • serialization/deserialization achieves the exact same representation of the variable in memory including allocation status, i.e.:
    • unallocated string -> no key
    • allocated empty string -> key = []
    • allocated non-empty string -> key = "value"
    • size of allocated arrays does not necessarily correspond to the number of useful entries they contain
  • No checks are made to the contents besides syntactic correctness
    • e.g. if fpm_model_t contains Windows paths and I load it on a Unix machine, they are not updated
  • we can later extend serializable_t to further fpm classes, restart and other features possible

@henilp105 henilp105 force-pushed the serialize_fpm_model branch 2 times, most recently from 3ebbcf4 to ba27a3b Compare December 7, 2023 14:39
@gnikit
Copy link
Member

gnikit commented Dec 21, 2023

I will have a look at this in a couple of hours. Thanks for your work @perazz

@perazz
Copy link
Contributor Author

perazz commented Dec 21, 2023

The failing tests are fixed by maintenance PR #976

@gnikit
Copy link
Member

gnikit commented Dec 21, 2023

The failing tests are fixed by maintenance PR #976

Thanks for the heads up. I will have a look at all the open prs and leave reviews. Sorry it took so long.

@perazz
Copy link
Contributor Author

perazz commented Jan 7, 2024

@gnikit @arteevraina @henilp105 please advise whether it'd be beneficial to include this feature in the upcoming release.

@gnikit
Copy link
Member

gnikit commented Jan 8, 2024

@perazz I would leave it for the next release. I would also like to understand a bit better the use of this. The TOML manifest can be converted to JSON/YAML with one of the Python/JS libs, I am not sure if fpm should be the one providing these fime format model conversions. I think what fpm really needs is the serialisation of the build process, much like Cmake's compile_commands.json

@perazz
Copy link
Contributor Author

perazz commented Jan 8, 2024

That could be an interesting extension @gnikit, however, this feature was designed such that the internal model representation could be exported and parsed by an external tool: for example the registry (cc @arteevraina @henilp105) should be able to check that all files necessary to the build are present, that no malicious changes occurred during upload of the package, etc., without actually performing any builds.

@henilp105
Copy link
Member

@perazz I think we should add it to the next release as we still haven't implemented the upload of fpm model during package upload , we have completed the package verification on the registry side currently supporting linux, windows and mac builds.

@gnikit
Copy link
Member

gnikit commented Jan 15, 2024

should be able to check that all files necessary to the build are present, that no malicious changes occurred during upload of the package, etc., without actually performing any builds.

I see, that makes sense, thanks for the explanation. Then, I am not sure if my review should carry any substantial weight in this PR since I have been only tangentially involved in the registry efforts. The only thing that I would say is that because this PR adds almost 5k lines of code in fpm maybe it should be reviewed with a very conservative Fortran eye to try and keep potential bugs and unexpected behaviour to a minimum.

@perazz
Copy link
Contributor Author

perazz commented Jan 16, 2024

@gnikit Here are a few hints that should make the review easier (to whom it may concern).

  • All settings classes used to have manually-written info (print to screen for debug) and new (load from toml) routines. Now, this has been standardized by making them extensions of a serializable_t class (This is boring Fortran 2003 stuff).

fpm/src/fpm/toml.f90

Lines 33 to 57 in a1e71e9

!> An abstract interface for any fpm class that should be fully serializable to/from TOML/JSON
type, abstract, public :: serializable_t
contains
!> Dump to TOML table, unit, file
procedure(to_toml), deferred, private :: dump_to_toml
procedure, non_overridable, private :: dump_to_file
procedure, non_overridable, private :: dump_to_unit
generic :: dump => dump_to_toml, dump_to_file, dump_to_unit
!> Load from TOML table, unit, file
procedure(from_toml), deferred, private :: load_from_toml
procedure, non_overridable, private :: load_from_file
procedure, non_overridable, private :: load_from_unit
generic :: load => load_from_toml, load_from_file, load_from_unit
!> Serializable entities need a way to check that they're equal
procedure(is_equal), deferred, private :: serializable_is_same
generic :: operator(==) => serializable_is_same
!> Test load/write roundtrip
procedure, non_overridable :: test_serialization
end type serializable_t

  • This forces (deferred routines) all settings classes to have procedures to read/write from toml-f structures, otherwise the code will not compile. For example:

!> Serialization interface
procedure :: serializable_is_same => fortran_is_same
procedure :: dump_to_toml
procedure :: load_from_toml

  • Testing is fully automated because it's done at the serializable_t level (test_serialization procedure). Any change (for example, adding one more logical flag ) would break the roundtrip Fortran -> TOML -> Fortran test if not added to the serialization routine, hence force the developer to fix it.

fpm/src/fpm/toml.f90

Lines 128 to 169 in a1e71e9

!> Test serialization of a serializable object
subroutine test_serialization(self, message, error)
class(serializable_t), intent(inout) :: self
character(len=*), intent(in) :: message
type(error_t), allocatable, intent(out) :: error
integer :: iunit, ii
class(serializable_t), allocatable :: copy
character(len=4), parameter :: formats(2) = ['TOML','JSON']
all_formats: do ii = 1, 2
open(newunit=iunit,form='formatted',action='readwrite',status='scratch')
!> Dump to scratch file
call self%dump(iunit, error, json=ii==2)
if (allocated(error)) then
error%message = formats(ii)//': '//error%message
return
endif
!> Load from scratch file
rewind(iunit)
allocate(copy,mold=self)
call copy%load(iunit,error, json=ii==2)
if (allocated(error)) then
error%message = formats(ii)//': '//error%message
return
endif
close(iunit)
!> Check same
if (.not.(self==copy)) then
call fatal_error(error,'serializable object failed '//formats(ii)//&
' write/reread test: '//trim(message))
return
end if
deallocate(copy)
end do all_formats
end subroutine test_serialization

  • Tests for all settings classes have been deployed, and they all pass, for example:

! Test package serialization
call package%test_serialization('test_valid_manifest',error)
if (allocated(error)) return

It will be super easy to add new settings classes this way.
(PS: there is no way to split this PR in smaller chunks: either it all works or it doesn't)

@certik
Copy link
Member

certik commented Feb 9, 2024

This PR is ready. Just resolving merge conflicts.

The CI failure is due to #994 and is unrelated.

@certik
Copy link
Member

certik commented Feb 9, 2024

@perazz let's schedule a video call with @henilp105 and @arteevraina and me and review this PR on the call, so that we can merge it. It will go quick that way.

@perazz
Copy link
Contributor Author

perazz commented Feb 10, 2024

I have updated the PR so it accounts for the changes in package_t that had taken place on main.
However, the CI for ifx still fails due to the following issue in fpm_settings.f90:

/tmp/ifx1633921843HhfFCb/ifxrJzAnL.i90: error #6405: The same named entity from different modules and/or program units cannot be referenced.   [TOML_TABLE]

This is clearly a compiler bug, as toml_table is only used once and there is no name duplication in the module. I believe this recent discussion at the Intel forum confirms the hypothesis. Unfortunately that also says that the bug is not fixed yet

@perazz
Copy link
Contributor Author

perazz commented Feb 11, 2024

ifx issue workaround (#994):

  • fpm now built with gfortran-10 (instead of with ifx)
  • only the test cases, that ifx handles without crashing, are built with ifx+intelmpi.

@henilp105
Copy link
Member

Thank you everyone, let's wait a few days to see if there are further comments, and if not, I will merge due to the requirements for the registry.

Copy link
Member

@henilp105 henilp105 left a comment

Choose a reason for hiding this comment

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

Thanks everyone, merging!

@henilp105 henilp105 merged commit 9a2849d into fortran-lang:main Feb 25, 2024
23 checks passed
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.

7 participants