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 2 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
31 changes: 20 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,24 @@ 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 ( // ) = OpamFilename.Op.( // ) in
OpamFile.(make
(OpamRepositoryPath.packages repo_root prefix nv // "url"))
hannesm marked this conversation as resolved.
Show resolved Hide resolved
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.OpamFile.OPAM.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 +386,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 +565,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
9 changes: 5 additions & 4 deletions src/client/opamAdminRepoUpgrade.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,11 +428,12 @@ let do_upgrade repo_root =
in
if opam <> opam0 then
(OpamFile.OPAM.write_with_preserved_format opam_file opam;
let ( // ) = OpamFilename.Op.( // ) in
List.iter OpamFilename.remove [
OpamFile.filename
(OpamRepositoryPath.descr repo_root prefix package);
OpamFile.filename
(OpamRepositoryPath.url repo_root prefix package);
OpamFile.(filename (make
(OpamRepositoryPath.packages repo_root prefix package // "descr")));
OpamFile.(filename (make
(OpamRepositoryPath.packages repo_root prefix package // "url")));
hannesm marked this conversation as resolved.
Show resolved Hide resolved
];
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
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
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
Loading