Skip to content

new subcommand: opam admin migrate-extrafiles#5960

Merged
rjbou merged 4 commits intoocaml:masterfrom
hannesm:migrate
Feb 24, 2025
Merged

new subcommand: opam admin migrate-extrafiles#5960
rjbou merged 4 commits intoocaml:masterfrom
hannesm:migrate

Conversation

@hannesm
Copy link
Copy Markdown
Member

@hannesm hannesm commented May 16, 2024

This subcommand allows to move all extra-files of an existing opam repository into extra-sources. The files are extracted into a specified local directory, and the opam files are edited in-place.

see #5811 for motivation

@rjbou
Copy link
Copy Markdown
Collaborator

rjbou commented Jun 3, 2024

Thanks for the PR! We'll look at it once opam 2.2 release is done

@hannesm
Copy link
Copy Markdown
Member Author

hannesm commented Jun 11, 2024

For the future reviewer, a slightly modified piece (see hannesm/opam@migrate...hannesm:opam:migrate-extra-files) was used for driving the rewrite for opam-repository (ocaml/opam-repository#25960)

The main differences are:

  • if SHA512 is used as checksum, keep it (no need to replace with SHA256)
  • for easier reviewability, keep existing MD5 checksums (I even wrote another tool, https://github.com/hannesm/opam-check-checksum, to check that) of extra-files, and add the SHA256 to extra-source next to the MD5
  • file naming: after more thoughts: deduplcation is great, but for a future developer, it is great if there are e.g. 25 META files for a single package, they should have reasonable names -- so the migration uses the META.PKG_VERSION (and at the point when it is not able to deduplicate everything, it just duplicates the same file if necessary) -- to support this, a two-stage approach was used, and the "not-to-be-deduplciated-files" are part of the commits shown in the link above.

So, it boils down to what to expect from such an automated migration, and whether there's need to have the result conveniently editable by people (or by scripts). Since some choices are hardcoded (such as the hash algorithm -- the tool has only ever been applied with SHA256 (and there are special code paths for "what if an MD5 is present" and "what to do if a SHA512 is present"), which is unlikely something you'd want to embed into the opam source tree.

About file naming, for deduplication reasons (and please note, there are other ways to achieve deduplication), the layout on the opam-source-archives repository is "patches/pkg-name/file1..N", i.e. not another subdirectory for pkg_version.

Copy link
Copy Markdown
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

I've rebased and made some improvements to the UI of the command but i haven't reviewed the core of the code yet, although it looks fine on first read.

Copy link
Copy Markdown
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

I added a commit with some modification, to review.
Note that there is no requirement on the already present extrafiles, no checksum check done, etc. We may not want to fail in case an extra file is not declared in the opam file, but the checksum mismatch should be notified at least.

\ (package opam)\n\
\ (files%s))\n"
(String.concat " "
(String.concat ""
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.

This should be extracted to its own PR

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.

a PR sounds a little excessive for such an internal change. What about its own commit?

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.

It was not just about self-contained PRs, but it is also adding "noise" in the diff of the PR.

kit-ty-kate and others added 4 commits February 21, 2025 18:11
This subcommand allows to move all extra-files of an existing opam repository
into extra-sources. The files are extracted into a specified local directory,
and the opam files are edited in-place.
Copy link
Copy Markdown
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

Thanks all! A 6 hands PR :D

@rjbou rjbou merged commit 3d83651 into ocaml:master Feb 24, 2025
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants