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

master_changes.md

@@ -28,6 +28,8 @@ users)
 
 ## Install
   * Do not keep the build directory of the pinned packages [#6436 @kit-ty-kate]
+  * [BUG] Fix `opam install <local_dir>` not updating and storing pinned packages' metadata [#6209 @kit-ty-kate - fix #5567]
+  * [BUG] Fix `opam install --deps-only/--show-action <local_dir>` not updating (without storing) pinned packages' metadata [#6209 @kit-ty-kate - fix #5567]
 
 ## Build (package)
   * Patches are now applied using the `patch` OCaml library instead of GNU Patch [#5892 @kit-ty-kate - fix #6019 #6052]
@@ -201,6 +203,9 @@ users)
   * Add a test for packages with subpath in a repository [#6439 @rjbou]
   * Add tests for `--keep-build-dir` and `OPAMKEEPBUILDDIR` [#6436 @rjbou @kit-ty-kate]
   * Add admin filter subcommand test [#6166 @rjbou]
+  * Add a test showing the behaviour of opam install when a local opam file changes while being pinned [#6209 @kit-ty-kate]
+  * Add pin test to show stored overlay opam files [#6209 @rjbou]
+  * Add show test to highlight precedence of opam file selection and check that if an opam file is given it is always this one that is taken [#6209 @rjbou]
 
 ### Engine
 
@@ -247,6 +252,7 @@ users)
   * `OpamArg.InvalidCLI`: export exception [#6150 @rjbou]
   * `OpamArg`: export `require_checksums` and `no_checksums`, that are shared with `build_options` [#5563 @rjbou]
   * `OpamArg.hash_kinds`: was added [#5960 @kit-ty-kate]
+  * `OpamAuxCommands.{simulate_autopin,autopin ~simulate:true}`: now updates the `reinstall` field of the returned `switch_state` if necessary [#6209 @kit-ty-kate]
   * `OpamRepositoryCommand.switch_repos`: expose the function [#5014 @kit-ty-kate]
   * `OpamLockCommand.lock_opam`: add `~keep_local` argument to add local pins to pin-depends (and not resolve them) [#6411 @rjbou]
   * `OpamLockCommand.lock_opam`: make the `?only_direct` argument non-optional [#6411 @kit-ty-kate]

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
@@ -293,27 +293,39 @@ let autopin_aux st ?quiet ?(for_view=false) ?recurse ?subpath ?locked
           (* check of the target to avoid repin of pin to update with `opam
              install .` and loose edited opams *)
           let pinned_pkg = OpamPinned.package st nf.pin_name in
-          OpamSwitchState.primary_url st pinned_pkg = Some nf.pin.pin_url
-          &&
-          (match OpamSwitchState.opam_opt st pinned_pkg with
-           | Some opam ->
-             (match locked, OpamFile.OPAM.locked opam with
-              | Some ext , Some ext' -> String.equal ext ext'
-              | None, None -> true
-              | _ -> 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
-             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
-           | _, _ -> false)
+          match OpamSwitchState.opam_opt st pinned_pkg with
+          | Some opam0 ->
+            (match OpamFile.OPAM.read_opt nf.pin.pin_file with
+             | Some opam ->
+               (* Keep synchronised with [OpamFile.OPAM.pp_raw_fields] added
+                  fields. *)
+               let opam = OpamFile.OPAM.with_locked_opt nf.pin.pin_locked opam in
+               let opam =
+                 match OpamFile.OPAM.name_opt opam with
+                 | None -> OpamFile.OPAM.with_name nf.pin_name opam
+                 | Some _ -> 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 =
+                 match OpamFile.OPAM.url opam with
+                 | None ->
+                   OpamFile.OPAM.with_url
+                     (OpamFile.URL.create ?subpath:nf.pin.pin_subpath
+                        nf.pin.pin_url)
+                     opam
+                 | Some _ -> opam
+               in
+               OpamStd.Option.equal String.equal
+                 locked (OpamFile.OPAM.locked opam)
+               && OpamFile.OPAM.effectively_equal opam0 opam
+             | None -> false)
+          | None -> false
         with Not_found -> false)
       to_pin
   in
@@ -389,6 +401,29 @@ 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
@@ -408,7 +443,7 @@ let simulate_pinned_atoms pins atoms =
 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/format/opamFile.ml

@@ -3370,6 +3370,8 @@ module OPAMSyntax = struct
         OpamStd.Option.Op.(t.url >>= URL.subpath) = None)
       ~errmsg:"The url.subpath field is not allowed in files with \
                `opam-version` <= 2.0" -|
+    (* Fields added here are also added in [OpamAuxCommands.autopin_aux] to
+       check pin status. Keep synchronised. *)
     handle_subpath_2_0 -|
     handle_locked -|
     handle_env_paths