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

kit-ty-kate · 2024-09-24T21:02:50Z

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.

rjbou

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

src/client/opamAuxCommands.ml

tests/reftests/lock.test

rjbou · 2024-10-22T17:28:54Z

src/client/opamAuxCommands.ml

@@ -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 ||


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

src/client/opamAuxCommands.ml

master_changes.md

rjbou · 2024-10-22T18:24:15Z

master_changes.md

@@ -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]


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.

why is it not only about local dir?

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.

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

…l opam file changes while being pinned

…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.

rjbou · 2024-10-24T17:00:40Z

src/client/opamAuxCommands.ml

+             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


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.

rjbou · 2024-10-24T17:01:44Z

tests/reftests/pin.test

+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)


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.

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:

opam/tests/reftests/autopin.test

Line 231 in f3466d9

### opam pin

opam/tests/reftests/autopin.test

Line 259 in f3466d9

### opam pin -n ./foo

rjbou · 2024-10-24T17:11:49Z

tests/reftests/pin.test

+### ::: 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).


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.

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.

rjbou · 2024-10-24T17:13:03Z

src/client/opamAuxCommands.ml

@@ -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


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

kit-ty-kate added KIND: BUG AREA: ENGINE labels Sep 24, 2024

kit-ty-kate added this to the 2.4.0~alpha1 milestone Sep 24, 2024

kit-ty-kate mentioned this pull request Sep 24, 2024

opam install . --deps-only --working-dir incorrectly reports "Nothing to do" #5567

Open

kit-ty-kate requested a review from rjbou September 24, 2024 21:08

kit-ty-kate force-pushed the fix-opam-install-local-pinned branch from ce39103 to e8fc621 Compare September 24, 2024 21:12

kit-ty-kate mentioned this pull request Oct 17, 2024

opam install --deps-only --locked has nothing to do or installs wrong version #6248

Open

rjbou requested changes Oct 22, 2024

View reviewed changes

kit-ty-kate force-pushed the fix-opam-install-local-pinned branch from e8fc621 to baafe9e Compare October 22, 2024 18:41

reftest: add a test showing the behaviour of opam install when a loca…

9130a7a

…l opam file changes while being pinned

kit-ty-kate force-pushed the fix-opam-install-local-pinned branch from baafe9e to 53418ca Compare October 22, 2024 19:25

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

b060e1d

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

kit-ty-kate requested a review from rjbou October 22, 2024 21:19

Fix "opam install --deps/--show <local_dir>" not updating pinned pack…

5e531b5

…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 force-pushed the fix-opam-install-local-pinned branch from a95f45f to 5e531b5 Compare October 22, 2024 21:56

kit-ty-kate mentioned this pull request Oct 22, 2024

Fix #6248 #6256

Draft

rjbou reviewed Oct 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kit-ty-kate commented Sep 24, 2024

rjbou left a comment

rjbou Oct 22, 2024 •

edited

Loading

rjbou Oct 22, 2024 •

edited

Loading

kit-ty-kate Oct 22, 2024

rjbou Oct 22, 2024 •

edited

Loading

kit-ty-kate Oct 22, 2024

rjbou Oct 24, 2024

rjbou Oct 24, 2024

kit-ty-kate Oct 24, 2024

rjbou Oct 24, 2024

kit-ty-kate Oct 24, 2024

rjbou Oct 24, 2024

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

Are you sure you want to change the base?

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

Conversation

kit-ty-kate commented Sep 24, 2024

rjbou left a comment

Choose a reason for hiding this comment

rjbou Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

rjbou Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjbou Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjbou Oct 22, 2024 •

edited

Loading

rjbou Oct 22, 2024 •

edited

Loading

rjbou Oct 22, 2024 •

edited

Loading