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

fix: Add or remove dune from package sets to allow resolving packages depending on it #11103

Merged
merged 4 commits into from
Nov 6, 2024

Conversation

Leonidas-from-XIV
Copy link
Collaborator

There's multiple ways to go about it:

  1. Add some dune package in the space of valid packages. I was not sure how to get the dune version out of dune_rpc or whether that would even be the right thing, so I set the version to dev.
  2. Remove dune from the dependency formulas. Dependency_formula.remove_package formula (Package_name.Set.singleton Dune_dep.name) is an alternate solution.

Fixes #11096

@Leonidas-from-XIV Leonidas-from-XIV changed the title fix: Add dune to existing packages to allow resolving fix: Add or remove dune from package sets to allow resolving packages depending on it Nov 5, 2024
Copy link
Collaborator

@maiste maiste left a comment

Choose a reason for hiding this comment

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

One question to understand but it LGTM 👍

src/dune_pkg/opam_solver.ml Outdated Show resolved Hide resolved
@Leonidas-from-XIV
Copy link
Collaborator Author

I've changed the PR from doing 1. to 2, as I didn't like how we construct a fake dune.dev (and I'd rather construct a dune.<current-version>); that said removing packages from formulas isn't particularly nice. I'm torn on what the best solution would be.

@rgrinberg
Copy link
Member

We need to do a combination of 1 and 2. We need to pick the correct deps for a formula like ("a" "dune" {> 3}) | ("b" "dune" {< 3}).

So we give the right dune version when evaluating the formula and then filtering out dune from the evaluated list. We do the same for external deps as well.

I pushed something along those lines.

Leonidas-from-XIV and others added 4 commits November 6, 2024 10:20
Signed-off-by: Marek Kubica <marek@tarides.com>
Signed-off-by: Marek Kubica <marek@tarides.com>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@Leonidas-from-XIV Leonidas-from-XIV merged commit 619c098 into ocaml:main Nov 6, 2024
27 of 28 checks passed
@Leonidas-from-XIV Leonidas-from-XIV deleted the resolve-dune-pkg branch November 6, 2024 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dune dependency error with dune pkg lock
3 participants