Populate pin-depends only with the packages that needs to be pinned in the switch
#400
Conversation
samoht
changed the title
pin-depends only with the packages that needs to be pinned in the switch
|
This is ready to review. This is a breaking change:
Note that I am still a bit confused with the expected workflow to use when using |
There was a problem hiding this comment.
Yeah, I think this makes sense.
I am not entirely sure what the original reason was for writing all the package in the pin-depends field - maybe indeed it was about being able to reproduce the monorepo with just opam. That decision precedes my involvement in opam-monorepo.
| 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 *) |
There was a problem hiding this comment.
Small typo:
| (* ignore local pins with using x-opam-monorepo-repositories *) | |
| (* ignore local pins when using x-opam-monorepo-repositories *) |
| match OpamSwitchState.url switch_state pkg with | ||
| | None -> | ||
| (* ignore version-pinned packages *) | ||
| None |
There was a problem hiding this comment.
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).
| x-opam-monorepo-opam-repositories: [ | ||
| "file://$OPAM_MONOREPO_CWD/minimal-repo" | ||
| "file://$OPAM_MONOREPO_CWD/repo" | ||
| ] |
There was a problem hiding this comment.
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.
| 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/|' |
There was a problem hiding this comment.
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.
| List.filter | ||
| ~f:(fun (p, _) -> | ||
| match OpamPackage.Map.find_opt p dependency_table with | ||
| | None -> assert false |
There was a problem hiding this comment.
In this case find can also be used, if that is not a case that we expect to happen.
This is a tentative fix for mirage/mirage#1476
I guess we could want to add the switch pins to that pin-depends field for reproducibility.