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

Inconsistent solution when incompatible packages from the same repository are selected #145

Closed
emillon opened this issue Dec 15, 2020 · 8 comments
Labels
bug Something isn't working requires dune change Would need additional dune features to implement upstream Fix has to be implemented upstream

Comments

@emillon
Copy link
Collaborator

emillon commented Dec 15, 2020

Hi,

When requesting different versions from packages that come from the same repository, some invalid solutions can be selected. For example requesting dune 2.7.1 and dune-build-info 2.6.2 can create the following lockfile fragment:

  [
    "dune-build-info.2.6.2"
    "https://github.com/ocaml/dune/releases/download/2.7.1/dune-2.7.1.tbz"
  ]
@NathanReb
Copy link
Contributor

This is intended behaviour atm. The only clean alternative is to reject such solutions which in some cases mean there won't be any solutions.

One thing I considered was to re-run the solver by adding constraints so that all multi-opam repo packages share the same version but I'm not even sure we can express that constraint and there would still be cases where there is no such solution (e.g. one of the packages hasn't been released together with the others).

The ideal would be to have per package source tree pruning in dune, meaning we could pull packages from multi-opam repos separately which would completely rule out the need to find a compatible version for all those packages.

In the meantime we have to select a version for the tool to remain usable so we pick the highest one.

I agree this is not satisfying but unfortunately I don't think there is a simple solution on our end. In particular note that this is very fragile as if one was to version packages differently in the same repo, the version selected might not be the most recent one according to the repo's VCS history.

I'll open an issue on dune to see if pruning would be an option we're willing to consider.

@emillon
Copy link
Collaborator Author

emillon commented Dec 15, 2020

I think it would be OK to just fail in that case, or at maybe just emit a warning.

@NathanReb
Copy link
Contributor

I emphasize that failing is definitely not an option here unfortunately as it would fail pretty much all the time. From experience, those are very common.

From a user perspective, the only solution would be to require a fixed version for all such packages which is not satisfying at all and again, not always valid!

@NathanReb
Copy link
Contributor

A warning would be good indeed, the log already exist but it's info level atm, meaning it only shows up with -v!

@emillon
Copy link
Collaborator Author

emillon commented Dec 15, 2020

Raising the level seems good enough in the short term.

In the medium term I'd be curious to see if adding the extra constraints can make it work. These constraints reflect the reality (in the opam-monorepo model, the same version is used so there are extra constraints compared to the opam model).

@NathanReb
Copy link
Contributor

NathanReb commented Dec 15, 2020

In the medium term I'd be curious to see if adding the extra constraints can make it work. These constraints reflect the reality (in the opam-monorepo model, the same version is used so there are extra constraints compared to the opam model).

Indeed they reflect the reality in the cases where the selected version exists for all packages in the repo but that's not always true.

I'm writing this for future reference but what I'm describing here is the following situation:
I maintain a repo with packages a, b, and c. I do an initial 0.1.0 release for all of them. Later on I release a 0.1.1 patch for a bug that impacted a and b so opam knows about the following packages:

  • a.0.1.0
  • a.0.1.1
  • b.0.1.0
  • b.0.1.1
  • c.0.1.0

Someone using opam-monorepo will want the bug fix in, so they want a.0.1.1 and b.0.1.1 but since there's no corresponding c version, they can't express valid constraint as you suggest. Failing in that case means they simply can't use opam-monorepo without pinning c. It works but it's a lot of trouble, especially since they potentially have to do it for several repos and only to achieve the exact same result as today which is, pull in the 0.1.1 tarball which contains a valid c package.

@reynir
Copy link
Contributor

reynir commented Nov 18, 2022

I think it is very problematic that you can't trust opam-monorepo to respect the version constraints and especially the pins you specify. Even more so that the user is not informed of this unless they increase the log level -- something you can't easily achieve when calling opam-monorepo from mirage.

Raising the level seems good enough in the short term.
I made a PR to raise the log level to WARN.

I'm writing this for future reference but what I'm describing here is the following situation: I maintain a repo with packages a, b, and c. I do an initial 0.1.0 release for all of them. Later on I release a 0.1.1 patch for a bug that impacted a and b so opam knows about the following packages:

* `a.0.1.0`

* `a.0.1.1`

* `b.0.1.0`

* `b.0.1.1`

* `c.0.1.0`

Someone using opam-monorepo will want the bug fix in, so they want a.0.1.1 and b.0.1.1 but since there's no corresponding c version, they can't express valid constraint as you suggest. Failing in that case means they simply can't use opam-monorepo without pinning c. It works but it's a lot of trouble, especially since they potentially have to do it for several repos and only to achieve the exact same result as today which is, pull in the 0.1.1 tarball which contains a valid c package.

I am curious how many packages this applies to. Could this be better solved with an overlay repository that adds c.0.1.0-1 or similar?

@reynir reynir mentioned this issue Nov 18, 2022
2 tasks
@Leonidas-from-XIV Leonidas-from-XIV added the requires dune change Would need additional dune features to implement label Dec 23, 2022
@tmattio tmattio added bug Something isn't working upstream Fix has to be implemented upstream labels Jan 10, 2023
@tmattio
Copy link
Collaborator

tmattio commented Jan 10, 2023

Closing as a duplicate of #331. This will be worked around by #367

@tmattio tmattio closed this as completed Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working requires dune change Would need additional dune features to implement upstream Fix has to be implemented upstream
Projects
None yet
Development

No branches or pull requests

5 participants