Skip to content

Remove dealing with descr and url files #6253

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

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

Remove dealing with descr and url files #6253

Show file tree
Hide file tree
Changes from 1 commit
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"))
Comment thread
hannesm marked this conversation as resolved.
Outdated
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

@rjbou rjbou Jan 23, 2025

Copy link
Copy Markdown
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")));
Comment thread
hannesm marked this conversation as resolved.
Outdated
];
OpamConsole.status_line "Updated %s" (OpamFile.to_string opam_file))
)
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

@rjbou rjbou Jan 23, 2025

Copy link
Copy Markdown
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.

@kit-ty-kate kit-ty-kate Jan 23, 2025

Copy link
Copy Markdown
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

@rjbou rjbou Jan 24, 2025

Copy link
Copy Markdown
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?

@kit-ty-kate kit-ty-kate Jan 24, 2025

Copy link
Copy Markdown
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