Complete format upgrade mechanism at repo & switch level#6417
Complete format upgrade mechanism at repo & switch level#6417rjbou merged 5 commits intoocaml:masterfrom
Conversation
|
To test, there is the revert last commit that adds switch & repo changes in older versions, it is possible to launch opam root version test with theses changes and check the behaviour. |
src/state/opamFormatUpgrade.ml
Outdated
| let as_necessary_repo lock_kind gt = | ||
| let updates = [ | ||
| ] in | ||
| as_necessary_repo_switch_t |
There was a problem hiding this comment.
| let as_necessary_repo lock_kind gt = | |
| let updates = [ | |
| ] in | |
| as_necessary_repo_switch_t | |
| let as_necessary_repo lock_kind gt = | |
| if gt.global_state_to_upgrade.gtc_repo then None else | |
| let updates = [ | |
| ] in | |
| as_necessary_repo_switch_t |
Implem details: I'm not if its better, there is pro & cons:
- as is: factorise the "no change" case like that we ensur same beahviour between repo & switch
- suggestion: no need to define all these arguments if nothing is done, which is 99% of cases.
There was a problem hiding this comment.
i think i prefer your suggestion indeed
kit-ty-kate
left a comment
There was a problem hiding this comment.
Looks mighty fine, thanks!
However thinking more generally about #6393, i'm wondering why upgrades are not something done at a lower level (in OpamFile). For example some parts of the code currently uses OpamStateConfig.Repos.safe_read directly – rightly or wrongly – e.g.
opam/src/client/opamCommands.ml
Line 2549 in fa93c54
If upgrades were done directly on the lowest-level read function (e.g.
OpamFile.Repos_config.read) it could avoid some issues in the future if we were to use the lower level functions directly without the upgrade functions, and it might make all this easier to use for us.
In any case this question can be discussed later and the PR in itself is just fine to merge tomorrow. If we want to change the level upgrades are done, we can do it in a separate PR later.
src/state/opamFormatUpgrade.ml
Outdated
| * If it is a light upgrade, returns as second element if the repo or switch | ||
| need an light upgrade with `gtc_*` values. | ||
| * If it is an hard upgrade, performs repo & switch upgrade in upgrade | ||
| * [Should not happen] If it is an hard upgrade, performs repo & switch upgrade in upgrade |
There was a problem hiding this comment.
it looks like you meant to remove the previous line
master_changes.md
Outdated
| * Speedup the detection of available system packages with pacman and brew [#6324 @kit-ty-kate] | ||
|
|
||
| ## Format upgrade | ||
| * Add a note about to enforce no more upgrading last hard upgrade version (2.0.0~beta5), as far as possible. [#6416 @rjbou] |
There was a problem hiding this comment.
i don't think this should be in the master changelog. This is a note for ourselves in the code, nothing that users sees
There was a problem hiding this comment.
Should we notify users that we are aiming to no more have non compatible roots (hard upgrade) ?
src/state/opamFormatUpgrade.ml
Outdated
| let as_necessary_repo lock_kind gt = | ||
| let updates = [ | ||
| ] in | ||
| as_necessary_repo_switch_t |
There was a problem hiding this comment.
i think i prefer your suggestion indeed
dra27
left a comment
There was a problem hiding this comment.
Looks good - a small question in terms of use as to whether we can move the "upgrade" part a bit later in the two uses, and I've distilled a more full (and hopefully correct!!) explanation of Hard/Light upgrades along with why we hope not to have to do hard upgrades ever again.
src/state/opamFormatUpgrade.ml
Outdated
| let config_f = OpamPath.config gt.root in | ||
| (* If we don't have a written opam root version in a config file, | ||
| we are unable to determine if there is an upgrade to do. This can happen in | ||
| case there is only on the fly upgrades from 2.0. Should we fail ? *) |
There was a problem hiding this comment.
Can't we just assume that the absence of a root version means that it's 2.0?
There was a problem hiding this comment.
From
Line 1391 in 34f65ad
7581bc6 to
645a394
Compare
645a394 to
068142b
Compare
… are needed from repo/switch driven upgrades
…from repo or switch change
068142b to
3586aa6
Compare
Current mechanism permit to load on-the-fly if no write lock is required global config files. These files are written if at global, repo or switch level a write is requested.
We want to change repo state format, so we need to update that mechanism to act also on repo & switch config files.
In this PR, we still have the old system : hard upgrade that block, light upgrades that don't block (confirmation message), and on-the-fly upgrade on global config. Plus, is added a mechanism to retrieve repo & switch config on-the-fly if lock read or none is requested, and if write lock is required a global upgrade with writes is launched (global lock required).
I've put it in its own commit the comment about no more upgrading hard upgrade version for visibility (and in changes)