Relax ocaml compiler warning to allow ocaml-variants #290
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Leonidas-from-XIV
added
no changelog
NathanReb
reviewed
Apr 20, 2022
There was a problem hiding this comment.
I wonder if this shouldn't be made simpler and just compare ocaml versions. There are few things that can go wrong here:
ocaml-systemandocaml-base-compilershould likely be treated as equal here otherwise one can get a warning when their switch is perfectly fit to build the project.ocaml-variantscan have slightly different versions such as4.14.0+trunkwhich could be treated as equal in some circumstances- It might make sense in certain situation to use an ocaml-variant even if the lockfile says
ocaml-base-compiler, for instance if you want to try flambda.
With all this in mind I think just making sure ocaml's version match would be enough.
lib/config.ml
Outdated
| @@ -30,7 +30,9 @@ let base_packages = | |||
| |> List.map ~f:OpamPackage.Name.of_string | |||
| |> OpamPackage.Name.Set.of_list | |||
|
|
|||
| let compiler_package_name = OpamPackage.Name.of_string "ocaml-base-compiler" | |||
| let compiler_package_names = | |||
| [ "ocaml-base-compiler"; "ocaml-variants" ] | |||
There was a problem hiding this comment.
ocaml-system is also a possible compiler package. It shouldn't appear in the lockfile but if we want something lockfile specific it's probably best to move this list to another module to prevent it from being re-used for other purposes.
|
You might be right, it would be quite annoying to have to match OCaml versions across packages with all kinds of special cases. I've changed it so it only uses I'll remove the patch and its revert before merging. |
Leonidas-from-XIV
force-pushed
the
ocaml-variants-warning
branch
from
67ab42b
to
8f0c9a4
Compare
NathanReb
added a commit
to NathanReb/opam-repository
that referenced
this pull request
Apr 20, 2022
CHANGES: ### Added - Add opam extensions `x-opam-monorepo-opam-repositories` and `x-opam-monorepo-global-opam-vars` to make `lock` fully reproducible. (tarides/opam-monorepo#250, tarides/opam-monorepo#253, @NathanReb) - Show an error message when the solver can't find any version that satisfies the requested version constraint in the user's OPAM file (tarides/opam-monorepo#215, tarides/opam-monorepo#248, tarides/opam-monorepo#290 @Leonidas-from-XIV) - Allow packages to be marked as being provided by Opam and not to be pulled by `opam-monorepo`. To control this a new optional Opam file field, `x-opam-monorepo-opam-provided` is introduced. Its value is a list of package names that are to be excluded from being pulled (tarides/opam-monorepo#234, @Leonidas-from-XIV) - Show an error message when the OCaml version of the lock file does not match the OCaml version of the switch (tarides/opam-monorepo#267, tarides/opam-monorepo#268, @Leonidas-from-XIV) - Generate a `duniverse/README.md` file to explain the basics of `opam-monorepo` in the vendored directory (tarides/opam-monorepo#272, tarides/opam-monorepo#274, @Leonidas-from-XIV) - Add a `--prefer-cross-compile` flag for the solver to select cross-compiling versions of packages when available. This is determined through the presence of the `"cross-compile"` tag in the opam metadata. ### Changed - Bump lockfile version to 0.3 (tarides/opam-monorepo#285, @NathanReb) - Mark packages to be pulled by opam-monorepo with the `vendor` variable so using OPAM with `opam install --deps-only --locked .` will not install packages that will be installed with `opam-monorepo pull` (tarides/opam-monorepo#237, @Leonidas-from-XIV) ### Fixed - Fix a bug where a package which had a single version that built with dune and got selected by the solver would be reported has having no version building with dune. (tarides/opam-monorepo#245, @Leonidas-from-XIV) - Fix the solver so it does not select beta versions of the compiler unless forced to by version constraints or `--ocaml-version`. (tarides/opam-monorepo#269, @NathanReb) ### Removed - Drop support for lockfile versions 0.2 and lower (tarides/opam-monorepo#285, @NathanReb)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The warning introduced in #268 had the downside that it only worked when users would use
ocaml-base-compiler. However, when using variants the compiler is notocaml-base-compilerbutocaml-variants, as such the check needs to be relaxed to accept either of these options.Sidenote: Our
List.memfromStdextdoes not support passing in a customequalfunction, maybe a good idea to add?Reported by @patricoferris