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.

Sign up for GitHub

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

Jump to bottom

Pin depends #167

Merged
merged 13 commits into from
Aug 31, 2021
Merged

Pin depends #167

merged 13 commits into from
Aug 31, 2021

Conversation

TheLortex
Copy link
Contributor

@TheLortex TheLortex commented May 27, 2021 •
edited
Loading

This is a rebase of @rizo's PR: #159

Fixes #153

@rizo rizo mentioned this pull request Jun 3, 2021
Closed
emillon
emillon reviewed Jul 26, 2021
View reviewed changes
cli/lock.ml Outdated Show resolved Hide resolved
cli/lock.ml Outdated
local_opam_files []

let pull_pin_depends (pin_depends : (OpamPackage.t * OpamUrl.t) list) =
(* TODO: Group pin-depends with the same url. *)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be taken care of or a ticket created for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I implemented it and removed the comment!

@emillon
Copy link
Collaborator

emillon commented Jul 26, 2021

I just rebased and added a changelog entry.

@NathanReb
Copy link
Contributor

Thanks for the multiple rebase, I'll revive this PR.

There are a couple things that I'd like to improve, in particular the interface for Opam_solve and code sharing for pull and pulling the pin depends.

rizo and others added 9 commits August 23, 2021 14:46
@rizo @NathanReb
Read and pull pin-depends from root packages.
13688df
@rizo @NathanReb
Use pins in dependencies resolution.
cc7f0f7
@rizo @NathanReb
Handle pin packages during solve.
562ddfd 
How it works:
1. The list of pinned packages is *not* excluded from the solution.
2. When creating package summaries for pins use the local opam file.
@rizo @NathanReb
Pass the pinned packages to opam solve.
09541b1 
1. Pass pin packages to `calculate_opam` alongside `local_packages`.
2. The `url.src` field in opam files for pinned packages is updated. This is similar to how opam works on `opam pin ...`.
@emillon @NathanReb
Add changelog entry
94cd2d4
@NathanReb
Improve Opam_solve and merge pin and local opam files there instead o…
efcea97 
…f Lock

The fact they are treated the same way by the solver is an implementation
detail and fits better there than in Lock.

Signed-off-by: Nathan Rebours <[email protected]>
@NathanReb
Only load global state once by calculating pin-depends in `calculate_…
2a02e90 
…opam`

Signed-off-by: Nathan Rebours <[email protected]>
@NathanReb
Improve get_opam_info
a169d32 
It used to go through the pin_depends map twice

Signed-off-by: Nathan Rebours <[email protected]>
@NathanReb
Report an error when a local package is in a pin-depends field
e72c279 
Signed-off-by: Nathan Rebours <[email protected]>
@NathanReb
Copy link
Contributor

I made a few improvements:

  • Clean up the interface of Opam_solve and let it deal with pin_depends rather than merging them into local_opam_files in Lock. calculate now expects a map for local packages and a map for pin-depends, abstracting everything else away from Lock.
  • I moved the pin_depends extraction further down the chain so that we only load opam states once
  • I fixed a small quirk in Opam_solve.get_opam_info so that the pin-depends map is only looked up once instead of twice (find_opt instead of mem + find)
  • I forbid having local packages in a pin-depends field. Before lock would have allowed it but the pin would effectively be ignored since local packages are removed from the solution in the lockfile. There's no way to properly support this as you either want it local and use whatever you have in your repo or use a pin that will be pulled in your duniverse. Having both will lead to a dune error because you have the same package twice in your workspace.
  • I factored opam pulling code for pin-depends and the pull command
  • I added deduplication of the repository's pin-depends (the same pin could appear several times in different opam files)
  • I added rejection of different pins for the same package
  • we group pin-depends per URL, meaning if several packages are pinned to the same URLs, the repo is pulled once instead of once per package
  • I implemented @emillon suggestion to use Bos to generate a temporary directory

I need to test this a bit.
@TheLortex would you mind trying this out for mirage and see if it still works?
@emillon could you give this another round of review?

@TheLortex
Copy link
Contributor Author

Thank you for your work on this ! The mirage CI hasn't failed so that's a good sign.
Now I'll use the tool on my machine and report any issues.

@NathanReb NathanReb merged commit 421d192 into tarides:main Aug 31, 2021
@NathanReb NathanReb mentioned this pull request Sep 13, 2021
Merged
NathanReb added a commit to NathanReb/opam-repository that referenced this pull request Sep 13, 2021
@NathanReb
[new release] opam-monorepo (0.2.4)
173cdc7 
CHANGES:

### Added

- When querying the solver for the local packages, if no explicit version was
  provided, use the value of the version field in the opam files instead of the
  default `zdev` if it is defined (tarides/opam-monorepo#183, @emillon)

- Add a `-l`/`--lockfile` command line option to explicitly set the lockfile
  to use or generate in `pull` or `lock` (tarides/opam-monorepo#163, @NathanReb)

- Honor `pin-depends` field in opam files. When present, these will be used by
  the solver (tarides/opam-monorepo#153, tarides/opam-monorepo#159, tarides/opam-monorepo#167, @rizo, @TheLortex, @NathanReb).

### Fixed

- Improve `lock` performance (about 2x faster) by loading the repository state
  only once (tarides/opam-monorepo#188, tarides/opam-monorepo#192, @emillon)

- Fix a bug where the dune-project parsing in the `pull` command would fail
  if it used CRLF for new lines. (tarides/opam-monorepo#191, @NathanReb)

- Simply warn instead of exiting when the dune-project file can't be parsed
  by `pull` as it only use it to suggest updating the lang version for
  convenience (tarides/opam-monorepo#191, @NathanReb)

- Fix a bug where `pull` and `lock` would expect the lockfile to sit in a
  different place and pull would fail. `pull` now simply looks for a
  `.opam.locked` file and pulls it unless there are multiple matching files in
  the repository's root. (tarides/opam-monorepo#163, @NathanReb)

- Fix failure when a package is pinned to a specific commit. `lock` now skips
  resolution when the ref is actually a commit pointed by a remote branch or
  when it looks like a commit (hexadecimal characters only, at least 7
  characters-long). (tarides/opam-monorepo#195, fixes tarides/opam-monorepo#127, @TheLortex)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NathanReb NathanReb NathanReb left review comments

@emillon emillon emillon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add suport for pin-depends
4 participants
@TheLortex @emillon @NathanReb @rizo