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

Add upper bound on ocaml 4.08 to result <1.5 #24868

Closed
wants to merge 1 commit into from
Closed

Conversation

mseri
Copy link
Member

@mseri mseri commented Nov 29, 2023

This will make the constraints no longer necessary.
See #24263

This will make the constraints no longer necessary.
See ocaml#24263

Signed-off-by: Marcello Seri <[email protected]>
@hannesm
Copy link
Member

hannesm commented Nov 29, 2023

I do appreciate this change, though I think that @dra27 should jump in (from #19880 (comment)): "it's annoying having to add a conflict on a package which isn't (necessarily) related to you, but it is the "correct" thing here (lock files really do matter)." If I understand this correctly, @dra27 has insight into active usages of lockfiles that, if this PR is merged, will break all the users thereof.

@dra27
Copy link
Member

dra27 commented Dec 4, 2023

Nothing's changed technically since 2021 as far as I can see. The issue is that anyone with a lockfile which locks OCaml 4.08-4.13 and any version of result prior to 1.5 will be unable to apply that lockfile after this upperbounds change. The policy has been in the past that we avoid making that of change. I'm not sure what technical value there is in re-hashing a two-year-old technical discussion - the policy can of course be altered.

For this PR, I do have a couple of comments: we long-since lost the installability check. Ignoring hypothetical lockfiles, the change concretely means alcotest.0.8.0, alcotest.0.8.1, alcotest.0.8.3, alcotest.0.8.4, alcotest.0.8.5, cmdliner.1.0.0, cmdliner.1.0.1, cmdliner.1.0.2, containers.2.6, containers.2.6.1, containers.2.7, datakit-client.0.12.0, datakit-client.0.12.2, datakit-client.0.12.3, datakit-client.1.0.0, depyt.0.2.0, irmin.1.3.2, irmin.1.4.0, logs.0.6.2, mirage-block-lwt.1.0.0, mirage-block-lwt.1.1.0, ocaml-version.2.3.0, promise.1.0.2, tls.0.9.2, tls.0.9.3, tls.0.10.1, tsdl.0.9.1, tsdl.0.9.2, tsdl.0.9.3, tsdl.0.9.4, tsdl.0.9.5, tsdl.0.9.6, tsdl-image.0.2.0, x509.0.6.2, zed.2.0, zed.2.0.1, zed.2.0.2, zed.2.0.3, zed.2.0.4 and zed.2.0.5 no longer install on 4.08+.

As I see it, the core of this very real problem stems from the lower-bounds checker. I realise altering opam-repo-ci is not a favourite subject, but can the lower-bounds check not be configured either indirectly by using avoid-version on the older results packages or even directly by changing ranking of the result package specs passed internally to the 0install solver? The lower-bounds check is meant to help maintenance of the repo, if it's not then the lower-bounds check ought to be tweaked, rather than necessarily the repo?

Separately, are we already discouraging/rejecting package submissions which directly depend on "result" but also stipulate 4.08 as a minimum? I haven't checked, but it looks as though we have the same error going on as has pervaded with the bytes package. The result library is designed to provide compatibility for a codebase to build with OCaml 4.02 and earlier. If a package doesn't need to support 4.02 or earlier, it has no reason to use the result library and should update its code to remove the dependency completely.

@mseri
Copy link
Member Author

mseri commented Dec 4, 2023

The point on the uninstallable packages is fair, although I would be curious to see if they are actually installable right now. So I am fine in closing this PR just for that.

Unfortunately using avoid-version requires a bound for opam >= 2.1, so this would also be breaking in some sense. But we can look into some other solution. The convenience of the lower bound check is not just the CI but also ensuring as much as we can that when people change their switch or downgrade something they can do it safely without breaking their switches.

Note that this PR is not discouraging to use result if somebody needs it: result 1.5 is compatible with all ocaml versions. I would be in favor of discouraging the use of older result packages, though: these are shadowing the result module and making all its functions unavailable, leading to nearly everything nowadays conflicting with it. Having an old result package in the lockfile is actually preventing any practical use of the result module, so it should probably be encouraged to update it to require result 1.5 instead, but that is not my decision to make for them.

@mseri mseri closed this Dec 4, 2023
@hannesm
Copy link
Member

hannesm commented Dec 5, 2023

I'm pretty disappointed by the discussion, I'm not motivated to contribute anything to this repository, being it that there are foggy "policies" that nobody cares to write down, and the argument being that there may be something that may break - instead of seeing the burden you impose on package authors.

So long and thanks for all the fish, opam-repository.

@dra27
Copy link
Member

dra27 commented Dec 5, 2023

The point on the uninstallable packages is fair, although I would be curious to see if they are actually installable right now. So I am fine in closing this PR just for that.

They are indeed all installable (I verified both their installability and, as it was much quicker to, that the change definitely changed the installability before commenting) - that was how I discovered #24891.

Unfortunately using avoid-version requires a bound for opam >= 2.1, so this would also be breaking in some sense. But we can look into some other solution. The convenience of the lower bound check is not just the CI but also ensuring as much as we can that when people change their switch or downgrade something they can do it safely without breaking their switches.

Adding avoid-version doesn't require a bound on opam - opam 2.0 ignores flags it doesn't understand. That's why we still have the ocaml-beta-repository dance for opam 2.0 with the compiler's beta versions, but opam 2.0 doesn't matter for the lowerbounds check (or in general - I don't think mccs in opam 2.0 is able to do the lowerbounds solves anyway, at least in the lifetime of the universe 🙂). Might be worth an experiment, given that (I expect) it's a lot easier to try than editing the solver directly.

Note that this PR is not discouraging to use result if somebody needs it: result 1.5 is compatible with all ocaml versions. I would be in favor of discouraging the use of older result packages, though: these are shadowing the result module and making all its functions unavailable, leading to nearly everything nowadays conflicting with it. Having an old result package in the lockfile is actually preventing any practical use of the result module, so it should probably be encouraged to update it to require result 1.5 instead, but that is not my decision to make for them.

Oh, indeed - those (old) packages which are constrained to result < 1.5 have done that for a reason, though (1.5 is a breaking change for anything that assumed only Result.result was defined by the library). The main issue is that no package which requires 4.03+ can possibly need to use the result library. We end up with these shims hanging around in codebases seemingly forever!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants