Fix opam install on a local directory not updating pinned packages' metadata

master_changes.md

@@ -27,6 +27,8 @@ users)
 ## Actions
 
 ## Install
+  * Fix `opam install <local_dir>` not updating and storing pinned packages' metadata [#6209 @kit-ty-kate - fix #5567]
+  * Fix `opam install --deps-only/--show-action <local_dir>` not updating (without storing) pinned packages' metadata [#6209 @kit-ty-kate - fix #5567]
 
 ## Build (package)
 
@@ -180,6 +182,7 @@ users)
   * Add a test showing the behaviour when a pin depend is unpinned [#6380 @rjbou]
   * Add a test to ensure `opam upgrade <pkg>` will not upgrade unrelated things [#6373 @kit-ty-kate]
   * Add a test in init to show ocaml system compiler selection behaviour [#6307 @kit-ty-kate @rjbou]
+  * Add a test showing the behaviour of opam install when a local opam file changes while being pinned [#6209 @kit-ty-kate]
 
 ### Engine
 

src/client/opamAuxCommands.ml

@@ -250,7 +250,7 @@ let resolve_locals ?(quiet=false) ?locked ?recurse ?subpath
              (OpamUrl.to_string nf.pin.pin_url))
           duplicates)
 
-let autopin_aux st ?quiet ?(for_view=false) ?recurse ?subpath ?locked
+let autopin_aux st ?quiet ?recurse ?subpath ?locked
     atom_or_local_list =
   let to_pin, atoms =
     resolve_locals ?quiet ?recurse ?subpath ?locked atom_or_local_list
@@ -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
-             perform a simulated pin if so *)
-          (not for_view ||
-           match
+          (match
              OpamSwitchState.opam_opt st pinned_pkg,
              OpamFile.OPAM.read_opt nf.pin.pin_file
            with
            | Some opam0, Some opam ->
              let opam = OpamFile.OPAM.with_locked_opt nf.pin.pin_locked opam in
-             OpamFile.OPAM.equal opam0 opam
+             let opam = OpamFile.OPAM.with_name nf.pin_name opam in
+             let opam =
+               match OpamFile.OPAM.version_opt opam with
+               | None ->
+                 OpamFile.OPAM.with_version
+                   (OpamPinCommand.default_version st nf.pin_name) opam
+               | Some _ -> opam
+             in
+             let opam =
+               OpamFile.OPAM.with_url
+                 (OpamFile.URL.create ?subpath:nf.pin.pin_subpath nf.pin.pin_url)
+                 opam
+             in
+             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
+             in
+             OpamFile.OPAM.effectively_equal opam0 opam
            | _, _ -> false)
         with Not_found -> false)
       to_pin
@@ -389,14 +404,37 @@ let simulate_local_pinnings ?quiet ?(for_view=false) st to_pin =
            st.switch_global st.switch st.switch_config ~pinned
            ~opams:local_opams)
     );
+    reinstall = lazy (
+      let open OpamPackage.Set.Op in
+      let installed_pinned = st.pinned %% st.installed in
+      OpamPackage.Set.fold (fun pinned_pkg reinstall ->
+          match
+            OpamPackage.Set.find_opt
+              (fun pkg ->
+                 OpamPackage.Name.equal
+                   (OpamPackage.name pinned_pkg)
+                   (OpamPackage.name pkg))
+              local_packages
+          with
+          | None -> reinstall
+          | Some local_pkg ->
+            let old_opam = OpamPackage.Map.find pinned_pkg st.installed_opams in
+            let new_opam = OpamPackage.Map.find local_pkg local_opams in
+            if OpamFile.OPAM.effectively_equal old_opam new_opam then
+              reinstall
+            else
+              OpamPackage.Set.add local_pkg
+                (OpamPackage.Set.remove pinned_pkg reinstall))
+        installed_pinned (Lazy.force st.reinstall)
+    );
     pinned;
   } in
   st, local_packages
 
 let simulate_autopin st ?quiet ?(for_view=false) ?locked ?recurse ?subpath
     atom_or_local_list =
   let atoms, to_pin, obsolete_pins, already_pinned_set =
-    autopin_aux st ?quiet ~for_view ?recurse ?subpath ?locked atom_or_local_list
+    autopin_aux st ?quiet ?recurse ?subpath ?locked atom_or_local_list
   in
   if to_pin = [] then st, atoms else
   let st =

src/client/opamClient.ml

@@ -2215,9 +2215,6 @@ let install_t t ?ask ?(ignore_conflicts=false) ?(depext_only=false)
   in
   let pkg_reinstall =
     if assume_built then OpamPackage.Set.of_list pkg_skip
-    else if deps_only then OpamPackage.Set.empty
-    (* NOTE: As we only install dependency packages, there are no intersections
-       between t.reinstall and pkg_skip *)
     else Lazy.force t.reinstall %% OpamPackage.Set.of_list pkg_skip
   in
   (* Add the packages to the list of package roots and display a

src/client/opamPinCommand.ml

@@ -496,7 +496,8 @@ and source_pin
                       registered in the current switch."
       (OpamPackage.Name.to_string name);
 
-  (match OpamStd.Option.map OpamFile.URL.url cur_urlf, target_url with
+  let cur_urlf_url = OpamStd.Option.map OpamFile.URL.url cur_urlf in
+  (match cur_urlf_url, target_url with
    | Some u, Some target when OpamUrl.(
        u.transport <> target.transport ||
        u.path <> target.path ||
@@ -633,9 +634,10 @@ and source_pin
 
     if not OpamClientConfig.(!r.show) then
       OpamSwitchAction.write_selections st;
-    OpamConsole.msg "%s is now %s\n"
-      (OpamPackage.Name.to_string name)
-      (string_of_pinned opam);
+    if not (OpamStd.Option.equal OpamUrl.equal cur_urlf_url target_url) then
+      OpamConsole.msg "%s is now %s\n"
+        (OpamPackage.Name.to_string name)
+        (string_of_pinned opam);
 
     st
 

tests/reftests/autopin.test

@@ -259,11 +259,9 @@ Done.
 ### opam pin -n ./foo
 This will pin the following packages: foo-core, foo-format-bis, foo-format. Continue? [Y/n] y
 [NOTE] Package foo-core is already pinned to file://${BASEDIR}/foo (version 2.0).
-foo-core is now pinned to file://${BASEDIR}/foo (version 2.0)
 [NOTE] Package foo-format-bis does not exist in opam repositories registered in the current switch.
 foo-format-bis is now pinned to file://${BASEDIR}/foo (version 2.0)
 [NOTE] Package foo-format is already pinned to file://${BASEDIR}/foo (version 2.0).
-foo-format is now pinned to file://${BASEDIR}/foo (version 2.0)
 ### opam install ./foo --deps-only --show
 
 The following system packages will first need to be installed:

tests/reftests/lock.test

@@ -570,7 +570,6 @@ The following actions would be performed:
   - install foo    1          [required by tolock]
   - install tolock 1 (pinned)
 ### opam install ./tolock --locked
-tolock is now pinned to git+file://${BASEDIR}/tolock#master (version 1)
 [NOTE] Ignoring uncommitted changes in ${BASEDIR}/tolock (`--working-dir' not specified or specified with no argument).
 [tolock.1] synchronised (no changes)
 The following actions will be performed:
@@ -630,7 +629,6 @@ The following actions would be performed:
   - install foo    1          [required by tolock]
   - install tolock 1 (pinned)
 ### opam install ./tolock --locked
-tolock is now pinned to git+file://${BASEDIR}/tolock#master (version 1)
 [NOTE] Ignoring uncommitted changes in ${BASEDIR}/tolock (`--working-dir' not specified or specified with no argument).
 [tolock.1] synchronised (no changes)
 The following actions will be performed:
@@ -690,7 +688,6 @@ The following actions would be performed:
   - install foo    1          [required by tolock]
   - install tolock 1 (pinned)
 ### opam install ./tolock --locked
-tolock is now pinned to git+file://${BASEDIR}/tolock#master (version 1)
 [tolock.1] synchronised (no changes)
 The following actions will be performed:
 === install 4 packages

tests/reftests/pin.test

@@ -292,7 +292,6 @@ Fatal error: Invalid character '/' in package version "./nip2"
 # Return code 5 #
 ### : none - virtual
 ### opam pin add no-url 1 --kind none
-no-url is now pinned locally (version 1)
 
 The following actions will be performed:
 === install 1 package
@@ -312,7 +311,6 @@ The following actions will be performed:
 [NOTE] Pinning command successful, but your installed packages may be out of sync.
 # Return code 31 #
 ### opam pin add no-url2 1 --kind none
-no-url2 is now pinned locally (version 1)
 
 The following actions will be performed:
 === install 1 package
@@ -755,7 +753,6 @@ Done.
 ### sh add-pin.sh nip-mixed.dev3
 ### opam pin nip-mixed
 [NOTE] Package nip-mixed is already pinned to file://${BASEDIR}/nip-mixed (version dev2).
-nip-mixed is now pinned to file://${BASEDIR}/nip-mixed (version dev3)
 
 The following actions will be performed:
 === upgrade 1 package
@@ -785,7 +782,6 @@ Done.
 "add nip-mixed.dev4 package"
 ### opam pin nip-mixed
 [NOTE] Package nip-mixed is already pinned to git+file://${BASEDIR}/nip-mixed#master (version dev4).
-nip-mixed is now pinned to git+file://${BASEDIR}/nip-mixed#master (version dev1)
 
 The following actions will be performed:
 === downgrade 1 package
@@ -819,10 +815,7 @@ Done.
 dev1
 ### sh add-pin.sh nip-of.dev2 git
 ### opam install ./nip-of
-[nip-of.dev1] synchronised (git+file://${BASEDIR}/nip-of#master)
-[nip-of] Conflicting update of the metadata from git+file://${BASEDIR}/nip-of#master.
-Override files in ${BASEDIR}/OPAM/opamfiles/.opam-switch/overlay/nip-of (there will be a backup)? [Y/n] y
-User metadata backed up in ${BASEDIR}/OPAM/backup/nip-of.bak
+[nip-of.dev2] synchronised (git+file://${BASEDIR}/nip-of#master)
 The following actions will be performed:
 === upgrade 1 package
   - upgrade nip-of dev1 to dev2 (pinned)
@@ -838,7 +831,6 @@ dev2
 ### git -C ./nip-of commit -qm "add install file"
 ### opam pin ./nip-of
 [NOTE] Package nip-of is already pinned to git+file://${BASEDIR}/nip-of#master (version dev2).
-nip-of is now pinned to git+file://${BASEDIR}/nip-of#master (version dev3)
 
 The following actions will be performed:
 === upgrade 1 package
@@ -870,10 +862,7 @@ Done.
 dev1
 ### sh add-pin.sh nip-fo.dev2 git
 ### opam install ./nip-fo
-[nip-fo.dev1] synchronised (git+file://${BASEDIR}/nip-fo#master)
-[nip-fo] Conflicting update of the metadata from git+file://${BASEDIR}/nip-fo#master.
-Override files in ${BASEDIR}/OPAM/opamfiles/.opam-switch/overlay/nip-fo (there will be a backup)? [Y/n] y
-User metadata backed up in ${BASEDIR}/OPAM/backup/nip-fo.bak
+[nip-fo.dev2] synchronised (git+file://${BASEDIR}/nip-fo#master)
 The following actions will be performed:
 === upgrade 1 package
   - upgrade nip-fo dev1 to dev2 (pinned)
@@ -889,17 +878,18 @@ dev2
 ### git -C ./nip-fo commit -qm "add install file"
 ### opam install ./nip-fo
 [NOTE] Ignoring uncommitted changes in ${BASEDIR}/nip-fo (`--working-dir' not specified or specified with no argument).
-[nip-fo.dev2] synchronised (git+file://${BASEDIR}/nip-fo#master)
+[nip-fo.dev3] synchronised (git+file://${BASEDIR}/nip-fo#master)
 The following actions will be performed:
-=== recompile 1 package
-  - recompile nip-fo dev2 (pinned)
+=== upgrade 1 package
+  - upgrade nip-fo dev2 to dev3 (pinned)
 
 <><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
+-> retrieved nip-fo.dev3  (no changes)
 -> removed   nip-fo.dev2
--> installed nip-fo.dev2
+-> installed nip-fo.dev3
 Done.
 ### opam show nip-fo --field=version
-dev2
+dev3
 ### :::::::::::::::::::::::
 ### :X: --current is in pin-legacy.test
 ### :::::::::::::::::::::::
@@ -1019,7 +1009,6 @@ The following additional pinnings are required by dep1.dev:
 Pin and install them? [Y/n] y
 [dep2.2] synchronised (no changes)
 dep2 is now pinned to file://${BASEDIR}/dep2 (version 2)
-dep1 is now pinned to file://${BASEDIR}/dep1 (version dev)
 
 The following actions will be performed:
 === recompile 1 package
@@ -1035,3 +1024,104 @@ The following actions will be performed:
 -> installed dep2.2
 -> installed dep1.dev
 Done.
+### : Make sure opam install works when the opam file changes and the package is pinned
+### opam switch create --empty local-pin-pinned-packages
+### <pkg:dependency.1>
+opam-version: "2.0"
+### <pkg:dependency.2>
+opam-version: "2.0"
+### <pkg:dependency.3>
+opam-version: "2.0"
+### mkdir pin-change
+### git -C pin-change init -q --initial-branch=master
+### git -C pin-change config core.autocrlf false
+### <pin:pin-change/opam>
+opam-version: "2.0"
+### git -C pin-change add opam
+### git -C pin-change commit -qm first-commit
+### ::: behaviour when the package is not pinned
+### opam install --deps-only --show ./pin-change
+Nothing to do.
+### <pin:pin-change/opam>
+opam-version: "2.0"
+depends: "dependency" {= "1"}
+### opam install --deps-only --show ./pin-change
+The following actions would be performed:
+=== install 1 package
+  - install dependency 1 [required by pin-change]
+### ::: behaviour when the package is pinned
+### <pin:pin-change/opam>
+opam-version: "2.0"
+### opam install ./pin-change
+[NOTE] Package pin-change does not exist in opam repositories registered in the current switch.
+pin-change is now pinned to git+file://${BASEDIR}/pin-change#master (version dev)
+The following actions will be performed:
+=== install 1 package
+  - install pin-change dev (pinned)
+
+<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
+-> retrieved pin-change.dev  (git+file://${BASEDIR}/pin-change#master)
+-> installed pin-change.dev
+Done.
+### <pin:pin-change/opam>
+opam-version: "2.0"
+depends: "dependency" {= "1"}
+### ::: behaviour when the package is not pinned
+### opam install ./pin-change
+[NOTE] Ignoring uncommitted changes in ${BASEDIR}/pin-change (`--working-dir' not specified or specified with no argument).
+[pin-change.dev] synchronised (no changes)
+The following actions will be performed:
+=== recompile 1 package
+  - recompile pin-change dev (pinned)
+=== install 1 package
+  - install   dependency 1            [required by pin-change]
+
+<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
+-> installed dependency.1
+-> retrieved pin-change.dev  (no changes)
+-> removed   pin-change.dev
+-> installed pin-change.dev
+Done.
+### <pin:pin-change/opam>
+opam-version: "2.0"
+depends: "dependency" {= "2"}
+### opam install --deps-only ./pin-change
+[NOTE] Ignoring uncommitted changes in ${BASEDIR}/pin-change (`--working-dir' not specified or specified with no argument).
+[pin-change.dev] synchronised (no changes)
+The following actions will be performed:
+=== recompile 1 package
+  - recompile pin-change dev (pinned) [uses dependency]
+=== upgrade 1 package
+  - upgrade   dependency 1 to 2       [required by pin-change]
+
+<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
+-> removed   dependency.1
+-> installed dependency.2
+-> retrieved pin-change.dev  (no changes)
+-> removed   pin-change.dev
+-> installed pin-change.dev
+Done.
+### opam remove pin-change
+The following actions will be performed:
+=== remove 1 package
+  - remove pin-change dev (pinned)
+
+<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
+-> removed   pin-change.dev
+Done.
+### <pin:pin-change/opam>
+opam-version: "2.0"
+depends: "dependency" {= "3"}
+### opam install --deps-only ./pin-change/opam
+[NOTE] Ignoring uncommitted changes in ${BASEDIR}/pin-change (`--working-dir' not specified or specified with no argument).
+[pin-change.dev] synchronised (no changes)
+The following actions will be performed:
+=== upgrade 1 package
+  - upgrade dependency 2 to 3 [required by pin-change]
+
+<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
+-> removed   dependency.2
+-> installed dependency.3
+Done.
+### opam pin | '\(at [a-f0-9]+\)' -> '(at HASH)'
+pin-change.dev  (uninstalled)  git  git+file://${BASEDIR}/pin-change#master  (at HASH)

tests/reftests/upgrade.test

@@ -308,7 +308,6 @@ This will pin the following packages: quuux, quux. Continue? [Y/n] y
 [NOTE] Package quuux does not exist in opam repositories registered in the current switch.
 quuux is now pinned to file://${BASEDIR}/quux (version dev)
 [NOTE] Package quux is already pinned to file://${BASEDIR}/quux (version dev5).
-quux is now pinned to file://${BASEDIR}/quux (version dev6)
 ### opam upgrade ./quux --installed
 
 <><> Synchronising pinned packages ><><><><><><><><><><><><><><><><><><><><><><>

tests/reftests/working-dir.test

@@ -256,7 +256,11 @@ Done.
 opam-version: "2.0"
 depends: ["qux" {= "1"}]
 ### opam install ./ongoing --working-dir --show-action
-[NOTE] Package ongoing is already installed (current version is dev).
+The following actions would be performed:
+=== downgrade 1 package
+  - downgrade qux     4 to 1       [required by ongoing]
+=== recompile 1 package
+  - recompile ongoing dev (pinned)
 ### opam reinstall ./ongoing --working-dir --show-action
 
 <><> Synchronising pinned packages ><><><><><><><><><><><><><><><><><><><><><><>