This PR uses the new ?report_action_result argument we added to
OpamSolution.apply.
I tested the code and it seems to behave as expected. By inspecting
the git logs, I cannot check that the file only gets appended-to, but
I could check that killing and restarting marracheck is able to parse
back the synchronized logs and keep appending to it correctly.
Two potential issues I realized while working on this:
We may later need more instrumentation of OpamSolution.apply, for
concurrency control: if we want to have the information that one
cover-element worker is in the process of performing an action, and
be able to delay other workers that are trying the same action, then
we need to also have a callback before an action starts, not only
after it finishes. We may have to rework the opam-side API, so in
particular I think we should not try to upstream
?report_action_result yet, until we figured this out.
We designed our own datatypes in term of "packages", but opam works
on "actions". For example in the Aborted case, the explanation is
a set of action and not of packages. I changed our own type to reuse
the Opam type when it seemed to make sense.
This also made me realize that we can have the same package show up
several time in the report (both before this PR and after this PR:
I preserved this behavior which makes sense). Indeed, if Fetch or Build are Aborted, then `Install will also be aborted, and all
aborts will be stored independently in the report_item list.
Notice in particular that only one abort reason is stored in the
report-map of old cover elements: this means that our conversion is
lossy, and I am not sure how to deal with it. We could store a set
of reasons for an abort, but I think the better approach is to store
the "first abort" for the package in action-dependency order.
There is still something unpleasant with the way we have to
reverse-engineer the action-dependency order for a package in the
code. (I don't deal properly with abort-reason yet, but I had to
deal with Success/Error recording in a action-dependency-aware way,
as the previous code already was doing.) It feels like we are
missing something; maybe our report items and maps could work with
opam actions as the primitive piece of data, instead of packages?
I am not sure.
(On top of #3.)
This PR uses the new
?report_action_result
argument we added toOpamSolution.apply
.I tested the code and it seems to behave as expected. By inspecting the git logs, I cannot check that the file only gets appended-to, but I could check that killing and restarting marracheck is able to parse back the synchronized logs and keep appending to it correctly.
Two potential issues I realized while working on this:
?report_action_result
yet, until we figured this out.We designed our own datatypes in term of "packages", but opam works on "actions". For example in the
Aborted
case, the explanation is a set of action and not of packages. I changed our own type to reuse the Opam type when it seemed to make sense.This also made me realize that we can have the same package show up several time in the report (both before this PR and after this PR: I preserved this behavior which makes sense). Indeed, if
Fetch or
Build are Aborted, then `Install will also be aborted, and all aborts will be stored independently in the report_item list.Notice in particular that only one abort reason is stored in the report-map of old cover elements: this means that our conversion is lossy, and I am not sure how to deal with it. We could store a set of reasons for an abort, but I think the better approach is to store the "first abort" for the package in action-dependency order.
There is still something unpleasant with the way we have to reverse-engineer the action-dependency order for a package in the code. (I don't deal properly with abort-reason yet, but I had to deal with Success/Error recording in a action-dependency-aware way, as the previous code already was doing.) It feels like we are missing something; maybe our report items and maps could work with opam actions as the primitive piece of data, instead of packages? I am not sure.