Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error when multiple dev-repos collide on dir #377

Merged
merged 2 commits into from
Feb 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@

### Fixed

- Error in case where multiple packages with different dev-repo fields would be
placed in the same duniverse directory (#377, @gridbugs)

### Removed

### Security
Expand Down
1 change: 1 addition & 0 deletions lib/dev_repo.ml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ type t = string
let compare = String.compare
let from_string s = s
let to_string t = t
let pp = Fmt.string

let rec repeat_while_some x ~f =
match f x with None -> x | Some x -> repeat_while_some x ~f
Expand Down
1 change: 1 addition & 0 deletions lib/dev_repo.mli
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ type t

val from_string : string -> t
val to_string : t -> string
val pp : t Fmt.t

val repo_name : t -> (string, Rresult.R.msg) result
(** Computes a name for the repo by applying the following method:
Expand Down
53 changes: 49 additions & 4 deletions lib/duniverse.ml
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,54 @@ let dev_repo_map_from_packages packages =
| Some pkgs -> Some (pkg :: pkgs)
| None -> Some [ pkg ]))

(* converts a map from dev-repos to lists of packages to a list of repos after
checking for errors *)
let dev_repo_package_map_to_repos dev_repo_package_map =
let open Result.O in
let dev_repo_to_repo_result_map =
Dev_repo.Map.mapi dev_repo_package_map ~f:(fun dev_repo pkgs ->
Repo.from_packages ~dev_repo pkgs)
in
(* Handle any errors relating to individual repos but maintain the
association between dev-repo and repo for further error checking. *)
let* repo_by_dev_repo =
Dev_repo.Map.bindings dev_repo_to_repo_result_map
|> List.map ~f:(fun (dev_repo, repo_result) ->
Result.map (fun repo -> (dev_repo, repo)) repo_result)
|> Base.Result.all
in
(* Detect the case where multiple different dev-repos are associated with the
same duniverse directory. *)
let* () =
let dev_repos_by_dir =
List.fold_left repo_by_dev_repo ~init:String.Map.empty
~f:(fun acc (dev_repo, (repo : _ Repo.t)) ->
String.Map.update acc repo.dir ~f:(function
| None -> Some [ dev_repo ]
| Some dev_repos -> Some (dev_repo :: dev_repos)))
|> String.Map.bindings
in
match
List.find_opt dev_repos_by_dir ~f:(fun (_, dev_repos) ->
List.length dev_repos > 1)
with
| None -> Ok ()
| Some (dir, dev_repos) ->
let dir_path = Fpath.(Config.vendor_dir / dir) in
let message_first_line =
Fmt.str "Multiple dev-repos would be vendored into the directory: %a"
Fpath.pp dir_path
in
let dev_repos_pp =
Fmt.list
~sep:Fmt.(const char '\n')
(fun ppf dev_repo -> Fmt.pf ppf "- %a" Dev_repo.pp dev_repo)
in
Rresult.R.error_msgf "%s\nDev-repos:\n%a" message_first_line
dev_repos_pp dev_repos
in
Ok (List.map ~f:snd repo_by_dev_repo)

let from_dependency_entries ~get_default_branch dependencies =
let open Result.O in
let summaries =
Expand All @@ -247,10 +295,7 @@ let from_dependency_entries ~get_default_branch dependencies =
let* pkg_opts = Base.Result.all results in
let pkgs = Base.List.filter_opt pkg_opts in
let dev_repo_map = dev_repo_map_from_packages pkgs in
Dev_repo.Map.fold dev_repo_map ~init:[]
~f:(fun ~key:dev_repo ~data:pkgs acc ->
Repo.from_packages ~dev_repo pkgs :: acc)
|> Base.Result.all
dev_repo_package_map_to_repos dev_repo_map

let resolve ~resolve_ref t =
Parallel.map ~f:(Repo.resolve ~resolve_ref) t |> Base.Result.all
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
opam-version: "2.0"
dev-repo: "git+https://github.com/bar/project"
depends: [
"dune"
]
build: [ "dune" "build" ]
url {
src: "https://test.com/bar.tbz"
checksum: [
"sha256=0000000000000000000000000000000000000000000000000000000000000000"
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
opam-version: "2.0"
dev-repo: "git+https://github.com/foo/project.git#branch"
depends: [
"dune"
]
build: [ "dune" "build" ]
url {
src: "https://test.com/foo-branch.tbz"
checksum: [
"sha256=0000000000000000000000000000000000000000000000000000000000000000"
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
opam-version: "2.0"
dev-repo: "git+https://github.com/foo/project"
depends: [
"dune"
]
build: [ "dune" "build" ]
url {
src: "https://test.com/foo.tbz"
checksum: [
"sha256=0000000000000000000000000000000000000000000000000000000000000000"
]
}
16 changes: 16 additions & 0 deletions test/bin/error-on-directory-conflict.t/run.t
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
We have a project which depends on multiple packages with different dev-repos
but which happen to resolve to the same directory under duniverse. This tests
that such a situation results in an explicit error.

$ gen-minimal-repo
$ opam-monorepo lock
==> Using 1 locally scanned package as the target.
==> Found 11 opam dependencies for the target package.
==> Querying opam database for their metadata and Dune compatibility.
==> Calculating exact pins for each of them.
opam-monorepo: [ERROR] Multiple dev-repos would be vendored into the directory: duniverse/project
Dev-repos:
- git+https://github.com/foo/project.git#branch
- git+https://github.com/foo/project
- git+https://github.com/bar/project
[1]
10 changes: 10 additions & 0 deletions test/bin/error-on-directory-conflict.t/x.opam
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
opam-version: "2.0"
depends: [
"foo"
"foo-branch"
"bar"
]
x-opam-monorepo-opam-repositories: [
"file://$OPAM_MONOREPO_CWD/minimal-repo"
"file://$OPAM_MONOREPO_CWD/repo"
]