Merge pull request ocaml#9373 from Leonidas-from-XIV/remove-opam-repo…

…sitory-url-cli-arg feat(pkg): Remove `opam-repository-url` option
Leonidas-from-XIV · Dec 6, 2023 · 21f64f1 · 21f64f1
2 parents ac4970f + 2c638d0
commit 21f64f1
Show file tree

Hide file tree

Showing 20 changed files with 241 additions and 186 deletions.
diff --git a/bin/pkg/lock.ml b/bin/pkg/lock.ml
@@ -35,8 +35,6 @@ let check_for_dup_lock_dir_paths ts =
 
 let solve
   per_context
-  ~opam_repository_path
-  ~opam_repository_url
   ~update_opam_repositories
   ~solver_env_from_current_system
   ~experimental_translate_opam_filters
@@ -64,14 +62,7 @@ let solve
          let solver_env =
            solver_env ~solver_env_from_context ~solver_env_from_current_system
          in
-         let* repos =
-           get_repos
-             repos
-             ~opam_repository_path
-             ~opam_repository_url
-             ~repositories
-             ~update_opam_repositories
-         in
+         let* repos = get_repos repos ~repositories ~update_opam_repositories in
          let overlay =
            Console.Status_line.add_overlay (Constant (Pp.text "Solving for Build Plan"))
          in
@@ -130,8 +121,6 @@ let lock
   ~all_contexts
   ~dont_poll_system_solver_variables
   ~version_preference
-  ~opam_repository_path
-  ~opam_repository_url
   ~update_opam_repositories
   ~experimental_translate_opam_filters
   =
@@ -151,17 +140,13 @@ let lock
   in
   solve
     per_context
-    ~opam_repository_path
-    ~opam_repository_url
     ~update_opam_repositories
     ~solver_env_from_current_system
     ~experimental_translate_opam_filters
 ;;
 
 let term =
   let+ builder = Common.Builder.term
-  and+ opam_repository_path = Opam_repository_path.term
-  and+ opam_repository_url = Opam_repository_url.term
   and+ context_name =
     context_term
       ~doc:
@@ -213,8 +198,6 @@ let term =
       ~all_contexts
       ~dont_poll_system_solver_variables
       ~version_preference
-      ~opam_repository_path
-      ~opam_repository_url
       ~update_opam_repositories:(not skip_update)
       ~experimental_translate_opam_filters)
 ;;

diff --git a/bin/pkg/outdated.ml b/bin/pkg/outdated.ml
@@ -3,14 +3,7 @@ open Pkg_common
 module Lock_dir = Dune_pkg.Lock_dir
 module Opam_repo = Dune_pkg.Opam_repo
 
-let find_outdated_packages
-  ~context_name_arg
-  ~all_contexts_arg
-  ~opam_repository_path
-  ~opam_repository_url
-  ~transitive
-  ()
-  =
+let find_outdated_packages ~context_name_arg ~all_contexts_arg ~transitive () =
   let open Fiber.O in
   let+ pps, not_founds =
     Per_context.choose ~context_name_arg ~all_contexts_arg ~version_preference_arg:None
@@ -27,13 +20,7 @@ let find_outdated_packages
               }
             ->
             (* updating makes sense when checking for outdated packages *)
-            let* repos =
-              get_repos
-                repos
-                ~opam_repository_path
-                ~opam_repository_url
-                ~repositories
-                ~update_opam_repositories:true
+            let* repos = get_repos repos ~repositories ~update_opam_repositories:true
             and+ local_packages = find_local_packages in
             let lock_dir = Lock_dir.read_disk lock_dir_path in
             let+ results =
@@ -89,8 +76,6 @@ let term =
       value
       & flag
       & info [ "all-contexts" ] ~doc:"Check for outdated packages in all contexts")
-  and+ opam_repository_path = Opam_repository_path.term
-  and+ opam_repository_url = Opam_repository_url.term
   and+ transitive =
     Arg.(
       value
@@ -102,12 +87,7 @@ let term =
   let builder = Common.Builder.forbid_builds builder in
   let common, config = Common.init builder in
   Scheduler.go ~common ~config
-  @@ find_outdated_packages
-       ~context_name_arg
-       ~all_contexts_arg
-       ~opam_repository_path
-       ~opam_repository_url
-       ~transitive
+  @@ find_outdated_packages ~context_name_arg ~all_contexts_arg ~transitive
 ;;
 
 let info =

diff --git a/bin/pkg/pkg_common.ml b/bin/pkg/pkg_common.ml
@@ -143,49 +143,27 @@ let location_of_opam_url url =
       ]
 ;;
 
-let get_repos
-  repos
-  ~opam_repository_path
-  ~opam_repository_url
-  ~repositories
-  ~update_opam_repositories
-  =
+let get_repos repos ~repositories ~update_opam_repositories =
   let module Repository_id = Dune_pkg.Repository_id in
   let module Opam_repo = Dune_pkg.Opam_repo in
-  let open Fiber.O in
-  match opam_repository_path, opam_repository_url with
-  | Some _, Some _ ->
-    (* in theory you can set both, but how to prioritize them? *)
-    User_error.raise [ Pp.text "Can't specify both path and URL to an opam-repository" ]
-  | Some path, None ->
-    let repo_id = Repository_id.of_path path in
-    Fiber.return @@ [ Opam_repo.of_opam_repo_dir_path ~source:None ~repo_id path ]
-  | None, Some (url : OpamUrl.t) ->
-    let+ opam_repo =
-      Opam_repo.of_git_repo
-        ~repo_id:None
-        ~update:update_opam_repositories
-        ~source:url.path
-    in
-    [ opam_repo ]
-  | None, None ->
-    repositories
-    |> Fiber.parallel_map ~f:(fun name ->
-      match Dune_pkg.Pkg_workspace.Repository.Name.Map.find repos name with
-      | None ->
-        (* TODO: have loc for this failure? *)
-        User_error.raise
-          [ Pp.textf "Repository '%s' is not a known repository"
-            @@ Dune_pkg.Pkg_workspace.Repository.Name.to_string name
-          ]
-      | Some repo ->
-        let opam_url = Dune_pkg.Pkg_workspace.Repository.opam_url repo in
-        (match location_of_opam_url opam_url with
-         | `Git source ->
-           Opam_repo.of_git_repo ~repo_id:None ~update:update_opam_repositories ~source
-         | `Path path ->
-           let repo_id = Repository_id.of_path path in
-           Fiber.return @@ Opam_repo.of_opam_repo_dir_path ~source:None ~repo_id path))
+  let module Repository = Dune_pkg.Pkg_workspace.Repository in
+  repositories
+  |> Fiber.parallel_map ~f:(fun name ->
+    match Repository.Name.Map.find repos name with
+    | None ->
+      (* TODO: have loc for this failure? *)
+      User_error.raise
+        [ Pp.textf "Repository '%s' is not a known repository"
+          @@ Repository.Name.to_string name
+        ]
+    | Some repo ->
+      let opam_url = Dune_pkg.Pkg_workspace.Repository.opam_url repo in
+      (match location_of_opam_url opam_url with
+       | `Git source ->
+         Opam_repo.of_git_repo ~repo_id:None ~update:update_opam_repositories ~source
+       | `Path path ->
+         let repo_id = Repository_id.of_path path in
+         Fiber.return @@ Opam_repo.of_opam_repo_dir_path ~source:None ~repo_id path))
 ;;
 
 let find_local_packages =
@@ -197,52 +175,6 @@ let find_local_packages =
   Dune_project.packages project |> Package.Name.Map.map ~f:Package.to_local_package
 ;;
 
-module Opam_repository_path = struct
-  let term =
-    let dune_path =
-      let parser s =
-        s
-        |> Path.External.of_filename_relative_to_initial_cwd
-        |> Path.external_
-        |> Result.ok
-      in
-      let printer pf p = Pp.to_fmt pf (Path.pp p) in
-      Arg.conv (parser, printer)
-    in
-    Arg.(
-      value
-      & opt (some dune_path) None
-      & info
-          [ "opam-repository-path" ]
-          ~docv:"PATH"
-          ~doc:
-            "Path to a local opam repository. This should be a directory containing a \
-             valid opam repository such as the one at \
-             https://github.com/ocaml/opam-repository.")
-  ;;
-end
-
-module Opam_repository_url = struct
-  let term =
-    let parser s =
-      match OpamUrl.parse_opt s with
-      | Some url -> Ok url
-      | None -> Error (`Msg "URL can't be parsed")
-    in
-    let printer pf u = Pp.to_fmt pf (Pp.text (OpamUrl.to_string u)) in
-    let opam_url = Arg.conv (parser, printer) in
-    Arg.(
-      value
-      & opt (some opam_url) None
-      & info
-          [ "opam-repository-url" ]
-          ~docv:"URL"
-          ~doc:
-            "URL of opam repository to download. Can be either a git repository or a \
-             link to the tarball of a repository.")
-  ;;
-end
-
 let pp_packages packages =
   let module Package_version = Dune_pkg.Package_version in
   Pp.enumerate

diff --git a/bin/pkg/pkg_common.mli b/bin/pkg/pkg_common.mli
@@ -38,22 +38,12 @@ end
 
 val get_repos
   :  Dune_pkg.Pkg_workspace.Repository.t Dune_pkg.Pkg_workspace.Repository.Name.Map.t
-  -> opam_repository_path:Path.t option
-  -> opam_repository_url:OpamUrl.t option
   -> repositories:Dune_pkg.Pkg_workspace.Repository.Name.t list
   -> update_opam_repositories:bool
   -> Dune_pkg.Opam_repo.t list Fiber.t
 
 val find_local_packages : Dune_pkg.Local_package.t Package_name.Map.t Fiber.t
 
-module Opam_repository_path : sig
-  val term : Path.t option Term.t
-end
-
-module Opam_repository_url : sig
-  val term : OpamUrl.t option Term.t
-end
-
 (** [pp_packages lock_dir] returns a list of pretty-printed packages
     occuring in [lock_dir]. *)
 val pp_packages : Dune_pkg.Lock_dir.Pkg.t list -> 'a Pp.t
diff --git a/test/blackbox-tests/test-cases/pkg/additional-constraints.t b/test/blackbox-tests/test-cases/pkg/additional-constraints.t
@@ -5,7 +5,11 @@ It's possible to include additional packages or constraints in workspace files:
   $ cat >dune-workspace <<EOF
   > (lang dune 3.11)
   > (lock_dir
-  >  (constraints doesnotexist foo (bar (= 1.0.0))))
+  >  (constraints doesnotexist foo (bar (= 1.0.0)))
+  >  (repositories mock))
+  > (repository
+  >  (name mock)
+  >  (source "file://$(pwd)/mock-opam-repository"))
   > EOF
 
   $ mkrepo

diff --git a/test/blackbox-tests/test-cases/pkg/describe-pkg-lock.t b/test/blackbox-tests/test-cases/pkg/describe-pkg-lock.t
@@ -20,6 +20,14 @@ First we setup a repo.
   >  (default
   >   (name "foo")
   >   (lock_dir foo.lock)))
+  > (lock_dir
+  >  (repositories mock))
+  > (lock_dir
+  >  (path foo.lock)
+  >  (repositories mock))
+  > (repository
+  >  (name mock)
+  >  (source "file://$(pwd)/mock-opam-repository"))
   > EOF
 
 Here is the output of solving for multiple contexts:

diff --git a/test/blackbox-tests/test-cases/pkg/detect-duplicate-lock-dir-paths.t b/test/blackbox-tests/test-cases/pkg/detect-duplicate-lock-dir-paths.t
@@ -12,21 +12,26 @@ Define several build contexts that all use the default lockdir
   > (context
   >  (default
   >   (name custom-context-with-default-lock-dir)))
+  > (lock_dir
+  >  (repositories mock))
+  > (repository
+  >  (name mock)
+  >  (source "file://$(pwd)/mock-opam-repository"))
   > EOF
 
 Check that we can still generate lockdirs for individual contexts:
-  $ dune pkg lock --opam-repository-path=mock-opam-repository
+  $ dune pkg lock
   Solution for dune.lock:
   (no dependencies to lock)
-  $ dune pkg lock --opam-repository-path=mock-opam-repository --context=default
+  $ dune pkg lock --context=default
   Solution for dune.lock:
   (no dependencies to lock)
-  $ dune pkg lock --opam-repository-path=mock-opam-repository --context=custom-context-with-default-lock-dir
+  $ dune pkg lock --context=custom-context-with-default-lock-dir
   Solution for dune.lock:
   (no dependencies to lock)
 
 It's an error to use --all-contexts when there are multiple contexts with the same lockdir:
-  $ dune pkg lock --opam-repository-path=mock-opam-repository --all-contexts
+  $ dune pkg lock --all-contexts
   File "dune-workspace", line 5, characters 1-56:
   5 |  (default
   6 |   (name custom-context-with-default-lock-dir)))
@@ -48,18 +53,24 @@ Define several build contexts that all use the same custom lockdir:
   >  (default
   >   (name a)
   >   (lock_dir foo.lock)))
+  > (lock_dir
+  >  (path foo.lock)
+  >  (repositories mock))
+  > (repository
+  >  (name mock)
+  >  (source "file://$(pwd)/mock-opam-repository"))
   > EOF
 
 Check that we can still generate lockdirs for individual contexts:
-  $ dune pkg lock --opam-repository-path=mock-opam-repository --context=a
+  $ dune pkg lock --context=a
   Solution for foo.lock:
   (no dependencies to lock)
-  $ dune pkg lock --opam-repository-path=mock-opam-repository --context=b
+  $ dune pkg lock --context=b
   Solution for foo.lock:
   (no dependencies to lock)
 
 It's an error to use --all-contexts when there are multiple contexts with the same lockdir:
-  $ dune pkg lock --opam-repository-path=mock-opam-repository --all-contexts
+  $ dune pkg lock --all-contexts
   File "dune-workspace", line 7, characters 1-43:
   7 |  (default
   8 |   (name a)

diff --git a/test/blackbox-tests/test-cases/pkg/env-conditional-dependencies.t b/test/blackbox-tests/test-cases/pkg/env-conditional-dependencies.t
@@ -31,10 +31,12 @@ Create a workspace config that defines separate build contexts for macos and lin
   > (lang dune 3.8)
   > (lock_dir
   >  (path dune.linux.lock)
+  >  (repositories mock)
   >  (solver_env
   >   (os linux)))
   > (lock_dir
   >  (path dune.macos.lock)
+  >  (repositories mock)
   >  (solver_env
   >   (os macos)))
   > (context
@@ -45,10 +47,13 @@ Create a workspace config that defines separate build contexts for macos and lin
   >  (default
   >   (name macos)
   >   (lock_dir dune.macos.lock)))
+  > (repository
+  >  (name mock)
+  >  (source "file://$(pwd)/mock-opam-repository"))
   > EOF
 
 Now the os-specific dependencies are included on their respective systems.
-  $ dune pkg lock --dont-poll-system-solver-variables --opam-repository-path=mock-opam-repository --all-contexts
+  $ dune pkg lock --dont-poll-system-solver-variables --all-contexts
   Solution for dune.macos.lock:
   - foo.0.0.1
   - foo-macos.0.0.1