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

Fix "opam install <local_dir>" not updating pinned packages' metadata #6209

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kit-ty-kate
Copy link
Member

Fixes #5567

I have no idea why for_view was introduced in kit-ty-kate@9b2a67f but i've tested opam show and i'm not seeing any breaking nor does the testsuite.

The bypass here was causing local packages to be put in the already_pinned set. However this set is only used to update the <switch>/.opam-switch/sources/<pkg> directory, not the <...>/overlay directory as the assumption with this set is that the package description hasn't change.
Removing that bypass makes the function much more consistent.

The reinstall bit is a bit easier to understand: when using --deps-only or --show-action, the codepath goes through simulate_local_pinning which didn't take into account that some packages being pinned are already pinned. In that case the reinstall field must be updated to ensure the rest of the code knows that package should be reinstalled.

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 reviewed commit per commit, it's better to look at comments per commit.
I'm not sure about the third commit (reinstall packages), it raises a good question. we want to update the opam file internally, but do we want to upgrade the package itself? Usually, when users use --deps-only they don't want to launch an install of given package (no need, or don't want to double compile it). When the package is already installed, there is the "we want to have a consistent state" that leads us to want to upgrade the package. From CLI perspective, it is more consistent to not upgrade the package.
Maybe a note or warning to highlight the situation package updated internally, if you really want to reinstall it, launch opam install pkg is better than a full reinstall triggered by --deps-only.

tests/reftests/pin.test Outdated Show resolved Hide resolved
src/client/opamAuxCommands.ml Show resolved Hide resolved
tests/reftests/lock.test Outdated Show resolved Hide resolved
@@ -303,10 +303,7 @@ let autopin_aux st ?quiet ?(for_view=false) ?recurse ?subpath ?locked
| _ -> false)
| None -> false)
&&
(* For `opam show`, we need to check does the opam file changed to
perform a simulated pin if so *)
(not for_view ||
Copy link
Collaborator

@rjbou rjbou Oct 22, 2024

Choose a reason for hiding this comment

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

This change will trigger a OpamPinCommad.source_pin call that retrieves the whole surce, while what we want here is to update the opam file only.

tests/reftests/pin.test Show resolved Hide resolved
tests/reftests/pin.test Outdated Show resolved Hide resolved
tests/reftests/pin.test Outdated Show resolved Hide resolved
src/client/opamAuxCommands.ml Outdated Show resolved Hide resolved
master_changes.md Outdated Show resolved Hide resolved
@@ -25,6 +25,7 @@ users)

## Install
* Fix `opam install <local_dir>` not updating pinned packages' metadata [#6209 @kit-ty-kate - fix #5567]
* Fix `opam install --deps-only/--show-action <local_dir>` not updating pinned packages' metadata [#6209 @kit-ty-kate - fix #5567]
Copy link
Collaborator

@rjbou rjbou Oct 22, 2024

Choose a reason for hiding this comment

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

Also, the PR name should change to something more general, like fix opam install when targeting a local directory sometimes not updating pinned packages' metadata, it is no more only about local dir as a target.

Copy link
Member Author

Choose a reason for hiding this comment

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

why is it not only about local dir?

Copy link
Collaborator

@rjbou rjbou Oct 22, 2024

Choose a reason for hiding this comment

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

My mistake, I meant that it is not only the command opam install <local-dir> but also combined with option --deps-only and --show-action that is fixed here. I updated the proposal.

Copy link
Member Author

Choose a reason for hiding this comment

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

what about "Fix opam install on a local directory not updating pinned packages' metadata" ?

@kit-ty-kate kit-ty-kate force-pushed the fix-opam-install-local-pinned branch 2 times, most recently from 3a911c1 to a95f45f Compare October 22, 2024 21:18
…ages' metadata

The reinstall field from the switch state wasn't updated according to
the implicit pin and thus opam didn't think there was anything to do.
@kit-ty-kate kit-ty-kate mentioned this pull request Oct 22, 2024
let opam =
(* This is required to avoid the case where locked opam files were
stored with the added `available: opam-version >= "2.1"` *)
OpamFile.OPAM.with_available (OpamFile.OPAM.available opam0) opam
Copy link
Collaborator

Choose a reason for hiding this comment

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

It worth adding a comment where these fields are added for the internal stored opam, in order to update this piece of code when it is changed.
I don't know if we can have a test to follow that, keep the consistency between these fields added, and the ones checked here.

depends: "dependency" {= "1"}
### ::: behaviour when the package is not pinned
### opam install ./pin-change
pin-change is now pinned to git+file://${BASEDIR}/pin-change#master (version dev)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't want the repin note, it is not repinned to another url (no source fetching as source_pin does), only the opam file updated/changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

i general i would say yes, however this is not the role of this PR. This behaviour is the one that opam currently has and this PR is not changing it.
See for example these two lines where a foo-core and foo-format were repinned and the "is now pinned to" message still appears:

### ::: behaviour when the package is not pinned
### opam install ./pin-change
pin-change is now pinned to git+file://${BASEDIR}/pin-change#master (version dev)
[NOTE] Ignoring uncommitted changes in ${BASEDIR}/pin-change (`--working-dir' not specified or specified with no argument).
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we have a more fine grained handling of pin update (opam file vs source update), we can even remove that note if only the uncommitted opam file is updated, as now we have the information.

Copy link
Member Author

Choose a reason for hiding this comment

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

same as with https://github.com/ocaml/opam/pull/6209/files#r1815454356 i believe. I think those two improvements can be their own tickets to be fixed later.

@@ -303,16 +303,31 @@ let autopin_aux st ?quiet ?(for_view=false) ?recurse ?subpath ?locked
| _ -> false)
| None -> false)
&&
(* For `opam show`, we need to check does the opam file changed to
Copy link
Collaborator

Choose a reason for hiding this comment

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

It worth adding a test for opam show that highlights that there is no change in the behaviour

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 . --deps-only --working-dir incorrectly reports "Nothing to do"
2 participants