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

Populate pin-depends only with the packages that needs to be pinned in the switch #400

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
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
4 changes: 4 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@

### Changed

- Trim the list of packages in the `pin-depends` field of the lock file to
only keep the ones that needs to be installed in the switch
(#400, mirage/mirage#1476, @samoht reported by @dinosaure)

### Deprecated

### Fixed
Expand Down
37 changes: 36 additions & 1 deletion cli/lock.ml
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,24 @@ let root_pin_depends local_opam_files =
local_opam_files []
|> D.Pin_depends.sort_uniq

let get_local_pins source_config =
match (source_config : D.Source_opam_config.t) with
| { repositories = Some _; _ } ->
(* ignore local pins with using x-opam-monorepo-repositories *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small typo:

Suggested change
(* ignore local pins with using x-opam-monorepo-repositories *)
(* ignore local pins when using x-opam-monorepo-repositories *)

[]
| _ ->
OpamGlobalState.with_ `Lock_none @@ fun global_state ->
OpamSwitchState.with_ `Lock_none global_state @@ fun switch_state ->
OpamPinned.packages switch_state
|> OpamPackage.Set.to_seq
|> Seq.filter_map (fun pkg ->
match OpamSwitchState.url switch_state pkg with
| None ->
(* ignore version-pinned packages *)
None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would make more sense to make this into a named function is_pinned then it would be clearer that checking for an empty URL means a pinned package (or actually, maybe the OPAM libraries should expose this in some more obviously correct way instead of us trying to interpret what OPAM does).

| Some url -> Some (pkg, OpamFile.URL.url url))
|> List.of_seq

let pull_pin_depends ~global_state pin_depends =
let open Result.O in
if Base.List.is_empty pin_depends then Ok OpamPackage.Name.Map.empty
Expand Down Expand Up @@ -570,11 +588,28 @@ let run (`Root root) (`Recurse_opam recurse) (`Build_only build_only)
~require_cross_compile ~preferred_versions ~ocaml_version
~local_opam_files:opam_files ~target_packages
in
let dependency_table =
let aux { D.Opam.Dependency_entry.package_summary; vendored } =
(package_summary.package, vendored)
in
dependency_entries |> List.to_seq |> Seq.map aux |> OpamPackage.Map.of_seq
in
Common.Logs.app (fun l -> l "Calculating exact pins for each of them.");
let* duniverse = compute_duniverse ~dependency_entries >>= resolve_ref in
let target_depexts = target_depexts opam_files target_packages in
let* pin_depends = root_pin_depends opam_files in
let local_pins = get_local_pins source_config in
let pin_depends =
List.filter
~f:(fun (p, _) ->
match OpamPackage.Map.find_opt p dependency_table with
| None -> assert false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case find can also be used, if that is not a case that we expect to happen.

| Some vendored -> not vendored)
pin_depends
@ local_pins
in
let lockfile =
D.Lockfile.create ~source_config ~root_packages:target_packages
D.Lockfile.create ~source_config ~root_packages:target_packages ~pin_depends
~dependency_entries ~root_depexts:target_depexts ~duniverse ()
in
let cli_args = raw_cli_args () in
Expand Down
11 changes: 2 additions & 9 deletions lib/lockfile.ml
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,6 @@ end
module Pin_depends = struct
type t = (OpamPackage.t * OpamUrl.t) list

let from_duniverse l =
let open Duniverse.Repo in
List.concat_map l ~f:(fun { provided_packages; url; _ } ->
let url = Url.to_opam_url url in
List.map provided_packages ~f:(fun p -> (p, url)))

let sort t =
List.sort ~cmp:(fun (pkg, _) (pkg', _) -> OpamPackage.compare pkg pkg') t
end
Expand Down Expand Up @@ -293,11 +287,10 @@ type t = {

let depexts t = t.depexts

let create ~source_config ~root_packages ~dependency_entries ~root_depexts
~duniverse () =
let create ~source_config ~root_packages ~pin_depends ~dependency_entries
~root_depexts ~duniverse () =
let version = Version.current in
let depends = Depends.from_dependency_entries dependency_entries in
let pin_depends = Pin_depends.from_duniverse duniverse in
let duniverse_dirs = Duniverse_dirs.from_duniverse duniverse in
let depexts = Depexts.all ~root_depexts ~dependency_entries in
{
Expand Down
1 change: 1 addition & 0 deletions lib/lockfile.mli
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ end
val create :
source_config:Source_opam_config.t ->
root_packages:OpamPackage.Name.Set.t ->
pin_depends:(OpamPackage.t * OpamUrl.t) list ->
dependency_entries:Opam.Dependency_entry.t list ->
root_depexts:(OpamSysPkg.Set.t * OpamTypes.filter) list list ->
duniverse:Duniverse.t ->
Expand Down
1 change: 1 addition & 0 deletions test/bin/pin-depends.t/duniverse/b/b.opam
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
opam-version: "2.0"
12 changes: 12 additions & 0 deletions test/bin/pin-depends.t/pin-depends.opam
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
opam-version: "2.0"
depends: [
"dune"
"b"
"c"
]
pin-depends: ["b.dev" "./duniverse/b"]
x-opam-monorepo-opam-provided: ["b"]
x-opam-monorepo-opam-repositories: [
"file://$OPAM_MONOREPO_CWD/minimal-repo"
"file://$OPAM_MONOREPO_CWD/repo"
]
12 changes: 12 additions & 0 deletions test/bin/pin-depends.t/repo/packages/b/b.1/opam
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
opam-version: "2.0"
depends: [
"dune"
]
build: [ "dune" "build" ]
dev-repo: "git+https://github.com/b/b"
url {
src: "https://b.com/b.tbz"
checksum: [
"sha256=0000000000000000000000000000000000000000000000000000000000000000"
]
}
12 changes: 12 additions & 0 deletions test/bin/pin-depends.t/repo/packages/c/c.1/opam
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
opam-version: "2.0"
depends: [
"dune"
]
build: [ "dune" "build" ]
dev-repo: "git+https://github.com/c/c"
url {
src: "https://c.com/c.tbz"
checksum: [
"sha256=0000000000000000000000000000000000000000000000000000000000000001"
]
}
1 change: 1 addition & 0 deletions test/bin/pin-depends.t/repo/repo
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
opam-version: "2.0"
66 changes: 66 additions & 0 deletions test/bin/pin-depends.t/run.t
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
We have a simple project with a single package defined at the root.
It has a `x-opam-monorepo-opam-repositories` field set to use a local
opam-repository for locking and a pinned package.

$ cat pin-depends.opam
opam-version: "2.0"
depends: [
"dune"
"b"
"c"
]
pin-depends: ["b.dev" "./duniverse/b"]
x-opam-monorepo-opam-provided: ["b"]
x-opam-monorepo-opam-repositories: [
"file://$OPAM_MONOREPO_CWD/minimal-repo"
"file://$OPAM_MONOREPO_CWD/repo"
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think printing it here makes sense. If you want to have the contents of the pin-depends.opam in the run.t I would suggest just creating it with cat > pin-depends.opam <<EOF, otherwise its just the same info duplicated.


We provided a minimal opam-repository but locking should be successful.

$ gen-minimal-repo
$ opam-monorepo lock
==> Using 1 locally scanned package as the target.
==> Found 10 opam dependencies for the target package.
==> Querying opam database for their metadata and Dune compatibility.
==> Calculating exact pins for each of them.
==> Wrote lockfile with 1 entries to $TESTCASE_ROOT/pin-depends.opam.locked. You can now run opam monorepo pull to fetch their sources.

The lockfile should contain the base packages, dune and our 2 dependencies
`b` and `c` which should be pulled in the duniverse

$ cat pin-depends.opam.locked | sed 's|file://.*/pin-depends.t/|file://$LOCAL_PATH/|'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make this print just the relevant subsets of the lockfile? Otherwise the test is full of incidental content of the lockfile that will need to keep getting updated.

Something like opam show --just-file --raw -fpin-depends ./pin-depends.opam.locked.

opam-version: "2.0"
synopsis: "opam-monorepo generated lockfile"
maintainer: "opam-monorepo"
depends: [
"b" {= "dev"}
"base-bigarray" {= "base"}
"base-threads" {= "base"}
"base-unix" {= "base"}
"c" {= "1" & ?vendor}
"dune" {= "2.9.1"}
"ocaml" {= "4.13.1"}
"ocaml-base-compiler" {= "4.13.1"}
"ocaml-config" {= "2"}
"ocaml-options-vanilla" {= "1"}
]
pin-depends: [
"b.dev"
"file://$LOCAL_PATH/duniverse/b"
]
x-opam-monorepo-duniverse-dirs: [
[
"https://c.com/c.tbz"
"c"
[
"sha256=0000000000000000000000000000000000000000000000000000000000000001"
]
]
]
x-opam-monorepo-opam-provided: ["b"]
x-opam-monorepo-opam-repositories: [
"file://$OPAM_MONOREPO_CWD/minimal-repo" "file://$OPAM_MONOREPO_CWD/repo"
]
x-opam-monorepo-root-packages: ["pin-depends"]
x-opam-monorepo-version: "0.3"
4 changes: 0 additions & 4 deletions test/bin/simple-lock.t/run.t
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,6 @@ The lockfile should contain the base packages, dune and our 2 dependencies
"ocaml-config" {= "2"}
"ocaml-options-vanilla" {= "1"}
]
pin-depends: [
["b.1" "https://b.com/b.tbz"]
["c.1" "https://c.com/c.tbz"]
]
x-opam-monorepo-duniverse-dirs: [
[
"https://b.com/b.tbz"
Expand Down