Remove dealing with descr and url files - #6253
Conversation
OpamAdminRepoUpgrade: deal locally with descr/url files OpamAdminCommand: adapt checks whether url is present, and whether the opam file version is good
kit-ty-kate
left a comment
kit-ty-kate
left a comment
There was a problem hiding this comment.
I have a couple of suggestions for the code first.
I'll do a proper review later.
Co-authored-by: Kate <kit-ty-kate@outlook.com>
d304842 to
fc6454e
Compare
6d2e879 to
abe2fc4
Compare
abe2fc4 to
adb134e
Compare
|
there's quite some tests that fail now... I'm not sure whether this is related (likely it is) -- but also not sure whether updating the tests is preferred, or whether some bits of the code should be re-instantiated? |
| (** Return the description file for a given package: | ||
| {i $repo/packages/XXX/$NAME.VERSION/descr} *) | ||
| val descr: dirname -> string option -> package -> OpamFile.Descr.t OpamFile.t | ||
|
|
||
| (** urls {i $repo/package/XXX/$NAME.$VERSION/url} *) | ||
| val url: dirname -> string option -> package -> OpamFile.URL.t OpamFile.t | ||
|
|
There was a problem hiding this comment.
we still need them for upgrade functions. We can mark them as deprecated for use in external libraries, but opam lib need to keep for backward compatibility ftm.
There was a problem hiding this comment.
what if we simply stopped supporting opam 1.x instead? 6 and a half years seems like a reasonable time for people to upgrade from a deprecated format
There was a problem hiding this comment.
In that case, we should remove all 1.2 upgrade code, not only a part.
6 years to upgrade is a reasonable time indeed, but if the code do not require maintenance, why do we need to remove it?
There was a problem hiding this comment.
it does not require maintenance per say but it holds us back from doing this type of change for example, which we could also call as some kind of maintenance burden
| List.iter OpamFilename.remove | ||
| OpamPath.Switch.Overlay.([ | ||
| OpamFile.filename opam_file; | ||
| OpamFile.filename (url root st.switch name); | ||
| OpamFile.filename (descr root st.switch name); | ||
| ]); |
There was a problem hiding this comment.
Note to myself:
- check that it is handled in switch upgrade function
| let (url_file: OpamFile.URL.t OpamFile.t) = | ||
| OpamFile.make (dir // "url") | ||
| in | ||
| let (descr_file: OpamFile.Descr.t OpamFile.t) = | ||
| OpamFile.make (dir // "descr") | ||
| in |
There was a problem hiding this comment.
descr & url reading & handling should be moved to OpamFormatUpgrade.opam_file or OpamFormatUpgrade.opam_file_from_1_2_to_2_0
| (** URL overlay: {i | ||
| $meta/overlay/$name.$version/url} *) | ||
| val url: t -> switch -> name -> OpamFile.URL.t OpamFile.t | ||
|
|
||
| (** Descr orverlay *) | ||
| val descr: t -> switch -> name -> OpamFile.Descr.t OpamFile.t | ||
|
|
There was a problem hiding this comment.
we still need them for upgrade functions. We can mark them as deprecated for use in external libraries, but opam lib need to keep for backward compatibility ftm.
|
|
||
| let internal = "descr" | ||
| let format_version = OpamVersion.of_string "0" | ||
| module Descr = struct |
There was a problem hiding this comment.
The module can be moved in the bottom of the section; It also need a comment that it is deprecated and used only for compat.
| (** Package descriptions: [$opam/descr/] *) | ||
| module Descr: sig | ||
|
|
||
| include IO_FILE |
There was a problem hiding this comment.
IO_FILE functions are still used, why remove the module inclusion?
| (OpamRepositoryPath.url repo_root prefix nv)); | ||
| true) | ||
| else has_error | ||
| check_opam_file_version_url has_error repo_root prefix nv opam |
There was a problem hiding this comment.
Does this command supposed to work only on 2.0 repos ? if yes, there is no need to handle url files. It is sufficient to have a check at the beginning and fail with a hint to opam admin upgrade.
|
ref #6827 |
kit-ty-kate
added
the
PR: QUEUED
3 participants
They have been deprecated since quite some time. I suspect this PR could go further, and remove the OpamFile.DescrIO module - but I failed to understand what to replace it with.
Please let me know whether this direction is the right one. I'm especially unsure whether there need to be more guards checking that url / descr do not exist.
Fixes #5086
Queued on #6827