Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove dealing with descr and url files #6253

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Remove dealing with descr and url files #6253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
612ff8c
OpamRepositoryPath.descr/url: remove
hannesm Oct 21, 2024
b43c23e
OpamPath: remove url & descr
hannesm Oct 21, 2024
fc6454e
Apply suggestions from code review
hannesm Oct 21, 2024
adb134e
opam-admin-top: remove descr & url
hannesm Oct 21, 2024
9805cd4
remove the unused parts of OpamFile.Descr
kit-ty-kate Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions src/client/opamAdminCommand.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,25 @@ let packages_with_prefixes repo_root packages =
(List.rev missing_pkgs)));
pkgs_map

let check_opam_file_version_url has_error repo_root prefix nv opam =
let min_opam_version = OpamVersion.of_string "2.0" in
let url_file =
let open OpamFilename.Op in
OpamFile.make
(OpamRepositoryPath.packages repo_root prefix nv // "url")
in
let opam_version = opam.OpamFile.OPAM.opam_version in
if OpamFile.exists url_file then
(OpamConsole.warning "Not updating external URL file at %s"
(OpamFile.to_string url_file);
true)
else if OpamVersion.compare opam_version min_opam_version < 0 then
(OpamConsole.warning "OPAM version must be >= 2.0 at %s"
(OpamFile.to_string (OpamRepositoryPath.opam repo_root prefix nv));
true)
else
has_error

let update_extrafiles_command_doc =
"Add extra-files to an opam repository."
let update_extrafiles_command cli =
Expand Down Expand Up @@ -368,12 +387,7 @@ let update_extrafiles_command cli =
let opam_file = OpamRepositoryPath.opam repo_root prefix nv in
let opam = OpamFile.OPAM.read opam_file in
let has_error =
if OpamFile.exists (OpamRepositoryPath.url repo_root prefix nv) then
(OpamConsole.warning "Not updating external URL file at %s"
(OpamFile.to_string
(OpamRepositoryPath.url repo_root prefix nv));
true)
else has_error
check_opam_file_version_url has_error repo_root prefix nv opam
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

in
let files_dir = OpamRepositoryPath.files repo_root prefix nv in
if OpamFilename.exists_dir files_dir then
Expand Down Expand Up @@ -552,11 +566,7 @@ let add_hashes_command cli =
let opam_file = OpamRepositoryPath.opam repo_root prefix nv in
let opam = OpamFile.OPAM.read opam_file in
let has_error =
if OpamFile.exists (OpamRepositoryPath.url repo_root prefix nv) then
(OpamConsole.warning "Not updating external URL file at %s"
(OpamFile.to_string (OpamRepositoryPath.url repo_root prefix nv));
true)
else has_error
check_opam_file_version_url has_error repo_root prefix nv opam
in
let process_url has_error urlf =
let hashes = OpamFile.URL.checksum urlf in
Expand Down
12 changes: 7 additions & 5 deletions src/client/opamAdminRepoUpgrade.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,9 @@ let do_upgrade repo_root =
let descr_file =
OpamFilename.(opt_file (add_extension (chop_extension comp_file) "descr"))
in
let descr = descr_file >>| fun f -> OpamFile.Descr.read (OpamFile.make f) in
let descr =
descr_file >>| fun f -> OpamFile.Descr.create (OpamFilename.read f)
in
let nv, ocaml_version, variant =
match OpamStd.String.cut_at c '+' with
| None ->
Expand Down Expand Up @@ -428,11 +430,11 @@ let do_upgrade repo_root =
in
if opam <> opam0 then
(OpamFile.OPAM.write_with_preserved_format opam_file opam;
let open OpamFilename.Op in
let path = OpamRepositoryPath.packages repo_root prefix package in
List.iter OpamFilename.remove [
OpamFile.filename
(OpamRepositoryPath.descr repo_root prefix package);
OpamFile.filename
(OpamRepositoryPath.url repo_root prefix package);
OpamFile.filename (OpamFile.make (path // "descr"));
OpamFile.filename (OpamFile.make (path // "url"));
];
OpamConsole.status_line "Updated %s" (OpamFile.to_string opam_file))
)
Expand Down
6 changes: 0 additions & 6 deletions src/client/opamPinCommand.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,12 +259,6 @@ let edit st ?version name =
| Some o when OpamFile.OPAM.equal opam o ->
(OpamConsole.msg "Package metadata unchanged.\n"; st)
| _ ->
(* Remove obsolete auxiliary files, in case *)
OpamFilename.remove
(OpamFile.filename (path OpamPath.Switch.Overlay.url));
OpamFilename.remove
(OpamFile.filename (path OpamPath.Switch.Overlay.descr));

let opam_extra =
OpamStd.Option.default [] @@ OpamFile.OPAM.extra_files opam
in
Expand Down
33 changes: 2 additions & 31 deletions src/format/opamFile.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,7 @@ end
content. Formerly, (<repo>/packages/.../descr,
<repo>/compilers/.../<v>.descr) *)

module DescrIO = struct

let internal = "descr"
let format_version = OpamVersion.of_string "0"
module Descr = struct
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


type t = string * string

Expand All @@ -203,39 +200,13 @@ module DescrIO = struct
| "" -> x ^ "\n"
| y -> String.concat "" [x; "\n\n"; y; "\n"]

let of_channel _ ic =
let x =
try OpamStd.String.strip (input_line ic)
with End_of_file | Sys_error _ -> "" in
let y =
try OpamStd.String.strip (OpamSystem.string_of_channel ic)
with End_of_file | Sys_error _ -> ""
in
x, y

let to_channel _ oc (x,y) =
output_string oc x;
output_char oc '\n';
if y <> "" then
(output_char oc '\n';
output_string oc y;
output_char oc '\n')

let create str =
let head, tail =
match OpamStd.String.cut_at str '\n' with
| None -> str, ""
| Some (h,t) -> h, t in
OpamStd.String.strip head, OpamStd.String.strip tail

let of_string _ = create

let to_string _ = full

end
module Descr = struct
include DescrIO
include MakeIO(DescrIO)
end

(* module Comp_descr = Descr *)
Expand Down Expand Up @@ -3182,7 +3153,7 @@ module OPAMSyntax = struct
Pp.V.os_constraint;
"descr", no_cleanup Pp.ppacc_opt with_descr OpamStd.Option.none
(Pp.V.string_tr -|
Pp.of_pair "descr" Descr.(of_string (), to_string ()));
Pp.of_pair "descr" Descr.(create, full));
"extra-sources", no_cleanup Pp.ppacc_opt
with_extra_sources OpamStd.Option.none
(Pp.V.map_list ~depth:2 @@
Expand Down
7 changes: 3 additions & 4 deletions src/format/opamFile.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,12 +298,11 @@ end
(** Package descriptions: [$opam/descr/] *)
module Descr: sig

include IO_FILE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IO_FILE functions are still used, why remove the module inclusion?

type t

val create: string -> t
val empty : t

(** Create an abstract description file from a string *)
val of_string: t typed_file -> string -> t
val create: string -> t

(** Return the first line *)
val synopsis: t -> string
Expand Down
4 changes: 0 additions & 4 deletions src/format/opamPath.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,10 +270,6 @@ module Switch = struct

let tmp_opam t a n = package t a n /- "opam_"

let url t a n = package t a n /- "url"

let descr t a n = package t a n /- "descr"

let files t a n = package t a n / "files"

end
Expand Down
7 changes: 0 additions & 7 deletions src/format/opamPath.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,13 +362,6 @@ module Switch: sig
$meta/overlay/$name.$version/opam_} *)
val tmp_opam: t -> switch -> name -> OpamFile.OPAM.t OpamFile.t

(** 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

Comment on lines -365 to -371
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(** Files overlay *)
val files: t -> switch -> name -> dirname
end
Expand Down
6 changes: 0 additions & 6 deletions src/repository/opamRepositoryPath.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,6 @@ let packages repo_root prefix nv =
let opam repo_root prefix nv =
packages repo_root prefix nv // "opam" |> OpamFile.make

let descr repo_root prefix nv =
packages repo_root prefix nv // "descr" |> OpamFile.make

let url repo_root prefix nv =
packages repo_root prefix nv // "url" |> OpamFile.make

let files repo_root prefix nv =
packages repo_root prefix nv / "files"

Expand Down
7 changes: 0 additions & 7 deletions src/repository/opamRepositoryPath.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,6 @@ val packages: dirname -> string option -> package -> dirname
{i $repo/packages/XXX/$NAME.$VERSION/opam} *)
val opam: dirname -> string option -> package -> OpamFile.OPAM.t OpamFile.t

(** 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

Comment on lines -44 to -50
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(** files {i $repo/packages/XXX/$NAME.$VERSION/files} *)
val files: dirname -> string option -> package -> dirname

Expand Down
35 changes: 0 additions & 35 deletions src/state/opamFileTools.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1297,44 +1297,9 @@ let add_aux_files ?dir ?(files_subdir_hashes=false) opam =
match dir with
| None -> opam
| Some dir ->
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
Comment on lines -1300 to -1305
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

descr & url reading & handling should be moved to OpamFormatUpgrade.opam_file or OpamFormatUpgrade.opam_file_from_1_2_to_2_0

let files_dir =
OpamFilename.Op.(dir / "files")
in
let opam =
match OpamFile.OPAM.url opam, try_read OpamFile.URL.read_opt url_file with
| None, (Some url, None) -> OpamFile.OPAM.with_url url opam
| Some opam_url, (Some url, errs) ->
if url = opam_url && errs = None then
log "Duplicate definition of url in '%s' and opam file"
(OpamFile.to_string url_file)
else
OpamConsole.warning
"File '%s' ignored (conflicting url already specified in the \
'opam' file)"
(OpamFile.to_string url_file);
opam
| _, (_, Some err) ->
OpamFile.OPAM.with_format_errors (err :: opam.format_errors) opam
| _, (None, None) -> opam
in
let opam =
match OpamFile.OPAM.descr opam,
try_read OpamFile.Descr.read_opt descr_file with
| None, (Some descr, None) -> OpamFile.OPAM.with_descr descr opam
| Some _, (Some _, _) ->
log "Duplicate descr in '%s' and opam file"
(OpamFile.to_string descr_file);
opam
| _, (_, Some err) ->
OpamFile.OPAM.with_format_errors (err :: opam.format_errors) opam
| _, (None, None) -> opam
in
let opam =
let extra_files =
OpamFilename.opt_dir files_dir >>| fun dir ->
Expand Down
6 changes: 5 additions & 1 deletion src/state/opamFormatUpgrade.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,11 @@ let from_1_3_dev2_to_1_3_dev5 ~on_the_fly:_ root conf =
OpamStd.Option.default
(OpamFile.Descr.create
"Switch relying on a system-wide installation of OCaml")
(OpamFile.Descr.read_opt descr_f)
(if OpamFile.exists descr_f then
Some (OpamFile.Descr.create
(OpamSystem.read (OpamFile.to_string descr_f)))
else
None)
in
let comp_opam =
OpamFile.Comp.to_package comp (Some descr)
Expand Down
7 changes: 1 addition & 6 deletions src/state/opamUpdate.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,12 +331,7 @@ let pinned_package st ?version ?(autolock=false) ?(working_dir=false) name =
let save_overlay opam =
OpamFilename.mkdir overlay_dir;
let opam_file = OpamPath.Switch.Overlay.opam root st.switch name in
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);
]);
Comment on lines -334 to -339
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to myself:

  • check that it is handled in switch upgrade function

OpamFilename.remove (OpamFile.filename opam_file);
let files_dir = OpamPath.Switch.Overlay.files root st.switch name in
OpamFilename.rmdir files_dir;
let opam =
Expand Down
23 changes: 6 additions & 17 deletions src/tools/opam_admin_top.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,30 +52,20 @@ let iter_packages_gen ?(quiet=false) f =
(OpamPackage.to_string package);
let opam_file = OpamRepositoryPath.opam repo prefix package in
let opam = OpamFile.OPAM.read opam_file in
let descr_file = OpamRepositoryPath.descr repo prefix package in
let descr = OpamFile.Descr.read_opt descr_file in
let url_file = OpamRepositoryPath.url repo prefix package in
let url = OpamFile.URL.read_opt url_file in
let dot_install_file : OpamFile.Dot_install.t OpamFile.t =
OpamFile.make
(OpamRepositoryPath.files repo prefix package
// (OpamPackage.Name.to_string (OpamPackage.name package) ^ ".install"))
in
let dot_install = OpamFile.Dot_install.read_opt dot_install_file in
let opam2, descr2, url2, dot_install2 =
f package ~prefix ~opam ~descr ~url ~dot_install
let opam2, dot_install2 =
f package ~prefix ~opam ~dot_install
in
let descr2 = of_action descr descr2 in
let url2 = of_action url url2 in
let dot_install2 = of_action dot_install dot_install2 in
let changed = ref false in
let upd () = changed := true; incr changed_files in
if opam <> opam2 then
(upd (); OpamFile.OPAM.write_with_preserved_format opam_file opam2);
if descr <> descr2 then
(upd (); wopt OpamFile.Descr.write descr_file descr2);
if url <> url2 then
(upd (); wopt OpamFile.URL.write url_file url2);
if dot_install <> dot_install2 then
(upd (); wopt OpamFile.Dot_install.write dot_install_file dot_install2);
if !changed then
Expand All @@ -92,16 +82,15 @@ let iter_packages_gen ?(quiet=false) f =
!changed_files !changed_pkgs

let iter_packages ?quiet
?(filter=true_) ?f ?(opam=identity) ?descr ?url ?dot_install
?(filter=true_) ?f ?(opam=identity) ?dot_install
() =
iter_packages_gen ?quiet
(fun p ~prefix ~opam:o ~descr:d ~url:u ~dot_install:i ->
(fun p ~prefix ~opam:o ~dot_install:i ->
if filter p then (
apply f p prefix o;
opam p o, to_action descr p d , to_action url p u,
to_action dot_install p i
opam p o, to_action dot_install p i
) else
o, `Keep, `Keep, `Keep)
o, `Keep)

let regexps_of_patterns patterns =
let contains_dot str =
Expand Down
6 changes: 1 addition & 5 deletions src/tools/opam_admin_top.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,8 @@ val iter_packages_gen:
(OpamPackage.t ->
prefix:string option ->
opam:OPAM.t ->
descr:Descr.t option ->
url:URL.t option ->
dot_install:Dot_install.t option ->
OPAM.t * Descr.t action * URL.t action * Dot_install.t action)
OPAM.t * Dot_install.t action)
-> unit

(** Turn a list of glob patterns into a proper filtering function on
Expand All @@ -44,7 +42,5 @@ val iter_packages:
?filter:(OpamPackage.t -> bool) ->
?f:(OpamPackage.t -> string option -> OPAM.t -> unit) ->
?opam:(OpamPackage.t -> OPAM.t -> OPAM.t) ->
?descr:(OpamPackage.t -> Descr.t -> Descr.t) ->
?url:(OpamPackage.t -> URL.t -> URL.t) ->
?dot_install:(OpamPackage.t -> Dot_install.t -> Dot_install.t) ->
unit -> unit
Loading