Complete format upgrade mechanism at repo & switch level #6417
Conversation
…y are needed from repo/switch driven upgrades
…from repo or switch change
This reverts commit 52687f6dd5976343b08f76ea897d54de7039805c.
|
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. |
| 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
| let written_root_version = OpamFile.Config.raw_root_version config_f | ||
| in |
There was a problem hiding this comment.
| let written_root_version = OpamFile.Config.raw_root_version config_f | |
| in | |
| let written_root_version = | |
| OpamFile.Config.raw_root_version config_f | |
| in |
| let written_root_version = | ||
| OpamStd.Option.default v2_0 written_root_version | ||
| in |
There was a problem hiding this comment.
| let written_root_version = | |
| OpamStd.Option.default v2_0 written_root_version | |
| in | |
| let written_root_version = | |
| OpamStd.Option.default OpamFile.Config.default_old_root_version written_root_version | |
| in |
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.
| @@ -1151,11 +1151,13 @@ let from_2_2_beta_to_2_2 ~on_the_fly:_ _ conf = conf, gtc_none | |||
| * 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
| @@ -101,6 +101,7 @@ users) | |||
| * 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
| *) | ||
| val as_necessary_repo_switch_light_upgrade: | ||
| 'a lock -> [`Repo | `Switch] -> 'b global_state -> unit | ||
| (* [as_necessary_repo lock gt] does he upgrades at repo level. [lock] is the |
There was a problem hiding this comment.
| (* [as_necessary_repo lock gt] does he upgrades at repo level. [lock] is the | |
| (* [as_necessary_repo lock gt] does the upgrades at repo level. [lock] is the |
| 'a lock -> [`Repo | `Switch] -> 'b global_state -> unit | ||
| (* [as_necessary_repo lock gt] does he upgrades at repo level. [lock] is the | ||
| required lock for repo state and gt the on-the-fly upgraded global lock. If | ||
| [lock] is non or read, it will perform an on-the-fly upgrade of repos-config |
There was a problem hiding this comment.
| [lock] is non or read, it will perform an on-the-fly upgrade of repos-config | |
| [lock] is none or read, it will perform an on-the-fly upgrade of repos-config |
| 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
| if OpamStd.Option.equal OpamVersion.equal | ||
| written_root_version | ||
| (Some OpamFile.Config.root_version) then None else |
There was a problem hiding this comment.
You can use the newly added `OpamStd.Option.equal_some
| if OpamStd.Option.equal OpamVersion.equal | |
| written_root_version | |
| (Some OpamFile.Config.root_version) then None else | |
| if OpamStd.Option.equal_some OpamVersion.equal | |
| OpamFile.Config.root_version written_root_version then | |
| None | |
| else |
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)