Merge pull request ocaml#5047 from rjbou/pin-dpx

Fix auto pinning and depext handling
rjbou · Aug 8, 2022 · bf45934 · bf45934
1 parent 065aaa9
commit bf45934
Show file tree

Hide file tree

Showing 5 changed files with 124 additions and 40 deletions.
diff --git a/CHANGES b/CHANGES
@@ -26,6 +26,9 @@ are not marked). Those prefixed with "(+)" are new command/option (since
   an invalid `switch-config` file [#5027 @rjbou]
 * When a field is defined in switch and global scope, try to determine the
   scope also by checking switch selection [#5027 @rjbou]
+* [BUG] Handle external dependencies when updating switch state pin status (all
+  pins), instead as a post pin action (only when called with `opam pin`
+  [#5047 @rjbou - fix #5046]
 * Stop Zypper from upgrading packages on updates on OpenSUSE
   [#4978 @kit-ty-kate]
 * Clearer error message if a command doesn't exist

diff --git a/src/client/opamClient.ml b/src/client/opamClient.ml
@@ -1479,39 +1479,16 @@ module PIN = struct
   open OpamPinCommand
 
   let post_pin_action st was_pinned names =
-    let pkgs =
-      let newly = st.pinned -- was_pinned in
-      let old =
-        OpamPackage.packages_of_names was_pinned
-          OpamPackage.Name.Set.Op.(
-            OpamPackage.Name.Set.of_list names
-            -- OpamPackage.names_of_packages newly)
-      in
-      newly ++ old
-    in
-    let no_depexts =
-      not (OpamFile.Config.depext st.switch_global.config)
-      || OpamSysPkg.Set.is_empty
-        ((OpamPackage.Set.fold (fun pkg acc ->
-             OpamSysPkg.Set.union acc (OpamSwitchState.depexts st pkg)))
-           pkgs OpamSysPkg.Set.empty)
+    let names =
+      OpamPackage.Set.Op.(st.pinned -- was_pinned)
+      |> OpamPackage.names_of_packages
+      |> (fun s ->
+          List.fold_left
+            (fun s p -> OpamPackage.Name.Set.add p s)
+            s names)
+      |> OpamPackage.Name.Set.elements
     in
     try
-      let st =
-        if no_depexts then st else
-        let st =
-          { st with sys_packages =  lazy (
-                OpamPackage.Map.union (fun _ n -> n)
-                  (Lazy.force st.sys_packages)
-                  (OpamSwitchState.depexts_status_of_packages st pkgs)
-              )}
-        in
-        { st with available_packages = lazy (
-              OpamPackage.Set.filter (fun nv ->
-                  OpamSwitchState.depexts_unavailable st nv = None)
-                (Lazy.force st.available_packages)
-            )}
-      in
       upgrade_t
         ~strict_upgrade:false ~auto_install:true ~ask:true ~terse:true
         ~all:false

diff --git a/src/state/opamSwitchState.ml b/src/state/opamSwitchState.ml
@@ -1172,15 +1172,28 @@ let update_pin nv opam st =
     OpamStd.Option.default nv.version (OpamFile.OPAM.version_opt opam)
   in
   let nv = OpamPackage.create nv.name version in
-  update_package_metadata nv opam @@
-  { st with
-    pinned =
-      OpamPackage.Set.add nv
-        (OpamPackage.filter_name_out st.pinned nv.name);
-    available_packages = lazy (
-      OpamPackage.filter_name_out (Lazy.force st.available_packages) nv.name
-    );
-  }
+  let pinned =
+    OpamPackage.Set.add nv (OpamPackage.filter_name_out st.pinned nv.name)
+  in
+  let available_packages = lazy (
+    OpamPackage.filter_name_out (Lazy.force st.available_packages) nv.name
+  ) in
+  let st =
+    update_package_metadata nv opam { st with pinned; available_packages }
+  in
+  if not (OpamFile.Config.depext st.switch_global.config)
+  || OpamSysPkg.Set.is_empty (depexts st nv)
+  then st else
+  let sys_packages = lazy (
+    OpamPackage.Map.union (fun _ n -> n)
+      (Lazy.force st.sys_packages)
+      (depexts_status_of_packages st (OpamPackage.Set.singleton nv))
+  ) in
+  let available_packages = lazy (
+    OpamPackage.Set.filter (fun nv -> depexts_unavailable st nv = None)
+      (Lazy.force st.available_packages)
+  ) in
+  { st with sys_packages; available_packages }
 
 let do_backup lock st = match lock with
   | `Lock_write ->

diff --git a/tests/reftests/dune.inc b/tests/reftests/dune.inc
@@ -393,6 +393,26 @@
    %{targets}
    (run ./run.exe %{bin:opam} %{dep:pin.test} %{read-lines:testing-env}))))
 
+(alias
+ (name reftest-pin.unix)
+ (enabled_if (= %{os_type} "Unix"))
+ (action
+  (diff pin.unix.test pin.unix.out)))
+
+(alias
+ (name reftest)
+ (enabled_if (= %{os_type} "Unix"))
+ (deps (alias reftest-pin.unix)))
+
+(rule
+ (targets pin.unix.out)
+ (deps root-N0REP0)
+ (enabled_if (= %{os_type} "Unix"))
+ (action
+  (with-stdout-to
+   %{targets}
+   (run ./run.exe %{bin:opam} %{dep:pin.unix.test} %{read-lines:testing-env}))))
+
 (alias
  (name reftest-remove)
  (action

diff --git a/tests/reftests/pin.unix.test b/tests/reftests/pin.unix.test
@@ -0,0 +1,71 @@
+N0REP0
+### : depext update :
+### OPAMYES=1
+### opam option depext-run-installs=false
+Set to 'false' the field depext-run-installs in global configuration
+### <pin:bar/bar.opam>
+opam-version: "2.0"
+depends: "qux"
+depexts: [ "inexistant" ]
+### <add_pin_depends.sh>
+echo "pin-depends: [ \"qux.dev\" \"file://$BASEDIR/qux\" ]" >> bar/bar.opam
+### sh add_pin_depends.sh
+### <pin:qux/qux.opam>
+opam-version: "2.0"
+depexts: [ "another-inexistant" ]
+### opam switch create pinning --empty
+### opam pin ./bar | '(apt-get|brew|port)' -> 'syspkgmanager'
+Package bar does not exist, create as a NEW package? [Y/n] y
+The following additional pinnings are required by bar.~dev:
+  - qux.dev at file://${BASEDIR}/qux
+Pin and install them? [Y/n] y
+Package qux does not exist, create as a NEW package? [Y/n] y
+[qux.dev] synchronised (no changes)
+qux is now pinned to file://${BASEDIR}/qux (version dev)
+bar is now pinned to file://${BASEDIR}/bar (version ~dev)
+
+The following actions will be performed:
+  - install qux dev*
+  - install bar ~dev*
+===== 2 to install =====
+
+The following system packages will first need to be installed:
+    another-inexistant inexistant
+
+<><> Handling external dependencies <><><><><><><><><><><><><><><><><><><><><><>
+This command should get the requirements installed:
+
+    syspkgmanager install another-inexistant inexistant
+
+You can retry with '--assume-depexts' to skip this check, or run 'opam option depext=false' to permanently disable handling of system packages altogether.
+[NOTE] Pinning command successful, but your installed packages may be out of sync.
+# Return code 10 #
+### opam unpin bar qux
+Ok, qux is no longer pinned to file://${BASEDIR}/qux (version dev)
+Ok, bar is no longer pinned to file://${BASEDIR}/bar (version ~dev)
+### opam option 'depext-bypass-=["another-inexistant" "inexistant"]'
+No modification in switch pinning
+### opam install ./bar | '(apt-get|brew|port)' -> 'syspkgmanager'
+Package bar does not exist, create as a NEW package? [Y/n] y
+The following additional pinnings are required by bar.~dev:
+  - qux.dev at file://${BASEDIR}/qux
+Pin and install them? [Y/n] y
+Package qux does not exist, create as a NEW package? [Y/n] y
+[qux.dev] synchronised (no changes)
+qux is now pinned to file://${BASEDIR}/qux (version dev)
+bar is now pinned to file://${BASEDIR}/bar (version ~dev)
+The following actions will be performed:
+  - install qux dev*  [required by bar]
+  - install bar ~dev*
+===== 2 to install =====
+
+The following system packages will first need to be installed:
+    another-inexistant inexistant
+
+<><> Handling external dependencies <><><><><><><><><><><><><><><><><><><><><><>
+This command should get the requirements installed:
+
+    syspkgmanager install another-inexistant inexistant
+
+You can retry with '--assume-depexts' to skip this check, or run 'opam option depext=false' to permanently disable handling of system packages altogether.
+# Return code 10 #