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.

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 unnecessary copies/move when fetching archives #5018

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

kit-ty-kate
Copy link
Member

@kit-ty-kate kit-ty-kate commented Jan 21, 2022

In #4586 (comment) we can see that to download an archive, opam currently does basically:

+ curl -L -o "${OPAM_SWITCH_PREFIX}/.opam-switch/sources/dune.2.9.1/dune-2.9.1.tbz.part" -- "${ARCHIVE}"
+ mv "${OPAM_SWITCH_PREFIX}/.opam-switch/sources/dune.2.9.1/dune-2.9.1.tbz.part" "${OPAM_SWITCH_PREFIX}/.opam-switch/sources/dune.2.9.1/dune-2.9.1.tbz"
+ mv "${OPAM_SWITCH_PREFIX}/.opam-switch/sources/dune.2.9.1/dune-2.9.1.tbz" "/tmp/opam-14-82d098/dune-2.9.1.tbz"
+ tar xfj /tmp/opam-14-82d098/dune-2.9.1.tbz -C /tmp/opam-14-1e6357
+ cp -PRp /tmp/opam-14-1e6357/dune-2.9.1/* /home/opam/.opam/4.13/.opam-switch/sources/dune.2.9.1
+ cp -PRp "${OPAM_SWITCH_PREFIX}/.opam-switch/sources/dune.2.9.1" "${OPAM_SWITCH_PREFIX}/.opam-switch/build/dune.2.9.1"

The three first commands can be combined into one very easily, giving us:

+ curl -L -o /tmp/opam-14-82d098/dune-2.9.1.tbz.part -- "${ARCHIVE}"
+ mv /tmp/opam-14-82d098/dune-2.9.1.tbz.part /tmp/opam-14-82d098/dune-2.9.1.tbz
+ tar xfj /tmp/opam-14-82d098/dune-2.9.1.tbz -C /tmp/opam-14-1e6357
+ cp -PRp /tmp/opam-14-1e6357/dune-2.9.1/* /home/opam/.opam/4.13/.opam-switch/sources/dune.2.9.1
+ cp -PRp "${OPAM_SWITCH_PREFIX}/.opam-switch/sources/dune.2.9.1" "${OPAM_SWITCH_PREFIX}/.opam-switch/build/dune.2.9.1"

Fixes #5939

@kit-ty-kate kit-ty-kate added the PR: WIP Not for merge at this stage label Jan 22, 2022
@kit-ty-kate kit-ty-kate added PR: WIP Not for merge at this stage and removed PR: WIP Not for merge at this stage labels Jan 24, 2022
@dra27 dra27 removed the PR: WIP Not for merge at this stage label May 17, 2022
@rjbou rjbou added this to the 2.2.0~alpha milestone May 18, 2022
@rjbou
Copy link
Collaborator

rjbou commented May 18, 2022

Reworked it, it was more tedious than at first sight

@rjbou
Copy link
Collaborator

rjbou commented May 23, 2022

thought: It complicates the code while it removes a unneeded mv.

Copy link
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've added a test to highlight the updates.
On my side, the code seems good. But from previous comments it seems that I wrote a part of it, so another air of eyes to look at, to the test too.

@kit-ty-kate
Copy link
Member Author

lgtm now

@kit-ty-kate kit-ty-kate merged commit ab00af7 into ocaml:master Aug 8, 2024
29 checks passed
@kit-ty-kate kit-ty-kate deleted the fast-extract branch August 8, 2024 18:40
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.

pin-depends system tries to remove part of the content
3 participants