Skip to content

Offer to remove autopin pkgs on install failure - #6923

Open
NathanReb wants to merge 2 commits into
ocaml:masterfrom
NathanReb:fix-install-dot-half-pinned
Open

Offer to remove autopin pkgs on install failure#6923
NathanReb wants to merge 2 commits into
ocaml:masterfrom
NathanReb:fix-install-dot-half-pinned

Conversation

@NathanReb

@NathanReb NathanReb commented May 5, 2026

Copy link
Copy Markdown
Collaborator

This PR adds a prompt when an opam install command fails to install packages or is aborted, offering them to remove packages that were automatically pinned as part of the process.

This mostly affects installation of local packages, i.e. when opam install is passed a file or directory as this triggers an automatic pin of the given opam file or any opam file found within the directory, alongside their pin-depends.

It allows users to quickly clean up if they realize that was not their intent or if they are unable to proceed with the install.

Queued on

@NathanReb

Copy link
Copy Markdown
Collaborator Author

I'm opening this as draft because I have some questions regarding what should be done in those cases:

  1. when --check is passed and everything is installed already. I'd assume if this is the case, then nothing has been pinned but I might be wrong about that.
  2. when --check is passed and some packages must be installed.
  3. when dry-run or show
  4. when simulate (i.e. --check, --deps-only or --depext-only

1 and 2 are kind of a specific case of 4. If my understanding of 4 is correct, no pin is actually written to disk in that case and therefore there's no clean up left to do.

I also need to correctly update the tests so that the installation aborted test makes sure everything is correctly cleaned up, e.g. by adding pin-depends into the mix. I'll of course have to test the scenarios described above as well.

Comment thread src/client/opamCommands.ml Outdated
@NathanReb
NathanReb force-pushed the fix-install-dot-half-pinned branch 2 times, most recently from b95d7a1 to cffe345 Compare May 6, 2026 08:53
@NathanReb

Copy link
Copy Markdown
Collaborator Author

I updated the code so it unpins only when the user aborts the install and does so without asking for confirmation. The test was updated accordingly and I added a pin-depend in the mix to make sure it was also unpinned correctly.

@NathanReb
NathanReb force-pushed the fix-install-dot-half-pinned branch 2 times, most recently from f0a291c to cbe191e Compare May 6, 2026 10:17

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

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.

Could you split the addition of the new test from the fix? Doing it this way makes it very easy to understand what the fix changed by simply reading the diff

Comment thread tests/reftests/autopin.test
@NathanReb
NathanReb force-pushed the fix-install-dot-half-pinned branch from cbe191e to 4d559e9 Compare May 6, 2026 13:31
@rjbou rjbou added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label May 11, 2026
NathanReb added 2 commits May 18, 2026 10:40
Signed-off-by: Nathan Rebours <nathan.rebours@ocamlpro.com>
Signed-off-by: Nathan Rebours <nathan.rebours@ocamlpro.com>
@NathanReb
NathanReb force-pushed the fix-install-dot-half-pinned branch from 4d559e9 to efd0a99 Compare May 18, 2026 08:41
@NathanReb
NathanReb marked this pull request as ready for review May 20, 2026 13:33
@kit-ty-kate kit-ty-kate added this to the 2.6.0~alpha1 milestone Jun 27, 2026
@kit-ty-kate kit-ty-kate removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Jun 27, 2026
@rjbou
rjbou requested review from kit-ty-kate and rjbou July 3, 2026 14:10
@kit-ty-kate

Copy link
Copy Markdown
Member

While the PR looks reasonable overall i'd like a bit more time than we have to review it and think it over, so i'm bumping this to the 2.7 milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

opam install <local> pins packages even if the user doesn't proceed on the installation

3 participants