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

Fixup constraints #2760

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Leonidas-from-XIV
Copy link
Contributor

While updating an older OPAM switch I realized that the versions that are installed are too old to successfully compile, so I had to add a few constraints on versions and add dependencies that were not declared before.

Older versions like 0.1.1 exit fail with this error:

```
File "tool/ood-gen/lib/success_story.ml", line 13, characters 12-79:
13 | [@@deriving of_yaml, stable_record ~version:t ~add:[ slug; body_md; body_html ]]
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Ppxlib.Deriving: 'of_yaml' is not a supported type deriving generator
```
Previous versions fail with

```
File "tool/ood-gen/lib/academic_institution.ml", line 41, characters 53-79:
41 |       str ^ "T00:00:00+00:00" |> Ptime.of_rfc3339 |> Ptime.rfc3339_string_error
                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: Unbound value Ptime.rfc3339_string_error
```
The old version 0.10.6 fails with

```
File "src/ocamlorg_web/lib/ocamlorg_web.ml", line 12, characters 35-69:
12 |   Mirage_crypto_rng_lwt.initialize (module Mirage_crypto_rng.Fortuna);
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: This expression is packed module, but the expected type is unit
```

So this adds the dependency to the build files as well as to the OPAM
files.
@sabine
Copy link
Collaborator

sabine commented Dec 10, 2024

Since we're managing dependency versions via a pin on opam-repository, can we close this now? @cuihtlauac

@cuihtlauac
Copy link
Collaborator

I'm tempted to merge this one. I find it nice to have a .opam file that allows building without a switch.

@sabine
Copy link
Collaborator

sabine commented Dec 10, 2024

nice to have a .opam file that allows building without a switch

this is not enabled by this patch, since it's all just lower bounds and the upper bound is provided by the opam-repository pin

@cuihtlauac
Copy link
Collaborator

@Leonidas-from-XIV told me he successfully built without creating a local switch and without a repo pin, only adding this PR

@sabine
Copy link
Collaborator

sabine commented Dec 10, 2024

I think this is trying to solve a problem that shouldn't exist in the first place, since everyone should be developing against the set of dependencies we're deploying with - in order to avoid wasting time debugging inconsistencies or bugs with dependencies in different people's switches

@Leonidas-from-XIV
Copy link
Contributor Author

Leonidas-from-XIV commented Dec 10, 2024

I think this is trying to solve a problem that shouldn't exist in the first place, since everyone should be developing against the set of dependencies we're deploying with

I still think that the dependencies are wrongly described in that case. If the versions are fixed then the dependencies should either be removed from the dune-project or fixed with (= version) constraints, but the state of having dependencies that can't build the project is not good.

Futhermore, pinning the opam-repository only fixes the upper bound but this PR fixes the lower bound, so the problem I ran into is still possible even when using a pin.

@cuihtlauac
Copy link
Collaborator

I’ve been discussing this matter with @mtelvers and @shonfeder. Thanks to them, I’m now convinced we should move away from the repo pin and pin the docker image instead. That will enable caching opam switch building stage, which should speed up the ocaml-ci builds.

When building in Docker, either with pinned repo or pinned image, we don’t need any versionning information, what’s under the pin's cap does the job.

What’s needed is a way to have the same switch as in Docker when building outside Docker, for day-to-day development. Maybe, a GitHub action could derive an Opam switch dump when there's a pin change and create a PR out of that. Something automatic would be handy. Maintaining consistency on this is a weak spot in our set up.

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.

3 participants