Do depexts status check at opam update time #6489
Conversation
arozovyk
force-pushed
the
depexts_status_on_update_2
branch
from
a9ed214 to
6be401b
Compare
arozovyk
force-pushed
the
depexts_status_on_update_2
branch
4 times, most recently
from
dac6fd8 to
4af5735
Compare
src/state/opamSwitchState.ml
Outdated
| st | ||
| else | ||
| {st with sys_packages = | ||
| lazy ((OpamPackage.Map.union (fun x _ -> x) |
There was a problem hiding this comment.
lazy ((OpamPackage.Map.union (fun x _ -> x)
Why you take the first element, the one that is already in sys_package instead of updating with the new status ?
There was a problem hiding this comment.
If you remember, It did matter at some point which element to take but it was due to a buggy behaviour of the system overall that led to the fact that it had an importance.
Here, if the package is present in sys_package map, means that its status have already been computed (with depexts_status_of_packages by the way) and the status value is the same as from depexts_status_of_packages called here L1286.
There was a problem hiding this comment.
This function is used in two cases : deps only newly created virtual packages and newly pinned packages. It is not an issue for the first case, but for the second case we want to retrieve the last information, no?
If this function is only meant to add new packages, the documentation and the implementation should be updated.
tests/reftests/depexts.test
Outdated
| <><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><> | ||
| -> installed comp.1 | ||
| Done. | ||
| Fatal error: External dependency handling not supported for OS family 'unknown'. |
arozovyk
force-pushed
the
depexts_status_on_update_2
branch
10 times, most recently
from
c715f6a to
48e64e4
Compare
arozovyk
force-pushed
the
depexts_status_on_update_2
branch
from
48e64e4 to
9d59425
Compare
The fix is not good as in the case where there is no depexts possible, the previous version was returning an empty map and not supposing the all packages are available. We may want to add another variant to `available_packages` to handle the no depext case ?
rjbou
force-pushed
the
depexts_status_on_update_2
branch
from
9d59425 to
28b4831
Compare
Draft for #6461