pkg: workaround to avoid unstable compilers #10668
Conversation
gridbugs
force-pushed
the
avoid-unstable-ocaml-package
branch
from
bbc13a8
to
65c7cf1
Compare
gridbugs
force-pushed
the
avoid-unstable-ocaml-package
branch
from
65c7cf1
to
14619cd
Compare
src/dune_pkg/opam_solver.ml
Outdated
| Option.equal | ||
| OpamPackage.Name.equal | ||
| opam_file.name | ||
| (Some (OpamPackage.Name.of_string "ocaml")) |
There was a problem hiding this comment.
Don't we have a conflict class for OCaml packages? Would be good to leave a comment why we can't use it here.
There was a problem hiding this comment.
I assume you're referring to the ocaml-core-compiler conflict class. The ocaml package isn't part of that class. It seems to only contain compiler implementations such as ocaml-base-compiler and ocaml-variants. How were you imagining to use this class to help here? Just as a way to avoid hard-coding the "ocaml" package name?
There was a problem hiding this comment.
Even if using the conflict class, we then would just hardcode one name (ocaml-core-compiler) for another (ocaml).
There was a problem hiding this comment.
It's not about hard coding one particular name over the other. It's about making sure our code stays consistent with recognizing the compiler. I recall that I hard coded the name of the compiler package in another feature and it turned out wrong. Another reason for doing this in a principled way is that dune should support compiler forks just as well as it supports the original compiler.
src/dune_pkg/opam_solver.ml
Outdated
| - the version of the given package is later than the latest | ||
| version of the ocaml-base-compiler package lacking the | ||
| avoid-version flag *) | ||
| let opam_file_is_ocaml_newer_than_latest_base_compiler_if_prefer_newest |
There was a problem hiding this comment.
I still find this code hard to read tbh. I think the conversion to booleans to define and order is what's making it hard to read. Will try to get rid of it.
There was a problem hiding this comment.
To clarify, @rgrinberg are you planning to do this work or are you asking me to take a look at it. Is there any harm in merging it as is and making some readability changes in a later PR? The build system team is planning to do some more developer interviews soon and this change is necessary for that work so it would help us if it could be merged soon.
There was a problem hiding this comment.
I will try to simplify the code a bit
src/dune_pkg/opam_solver.ml
Outdated
| (match opam_file_is_ocaml_package opam_file with | ||
| | false -> false | ||
| | true -> | ||
| (match Option.both opam_file.version t.latest_released_base_compiler_version with |
There was a problem hiding this comment.
When can the versions be absent here?
There was a problem hiding this comment.
For the lhs, only if the package being compared is named "ocaml" but lacks a version, and for the rhs, only if the opam repo has no package named "ocaml-base-compiler". Neither of these cases should happen in practice but I'm trying to minimize the assumptions dune makes about the opam repo.
Though I notice that elsewhere in this file we call OpamFile.OPAM.version which raises if its argument doesn't have a version, so there's little point checking for the version field here.
Fixes ocaml#10592. Solves an issue where the solver would choose unstable releases of the compiler. When the latest version of the "ocaml" package can only be satisfied by unstable versions of "ocaml-variants", dune would still include this unstable compiler in package solutions. This leads to users being given unstable versions of the compiler by default. This is only a problem because dune doesn't yet fully implement the avoid-version flag, and compiler packages are released under the assumption that the solver respects this flag. The workaround introduced in this change is to determine the latest stable version of "ocaml-base-compiler" (the latest version lacking the avoid-version flag), and have the solver prefer versions of the "ocaml" package that are no later than this version. Signed-off-by: Stephen Sherratt <[email protected]>
rgrinberg
force-pushed
the
avoid-unstable-ocaml-package
branch
from
14619cd
to
7109404
Compare
|
After thinking about, I've implemented a different workaround for this problem. It's imperfect, but at least it's simple to explain: we just skip avoid-versions packages and just treat them as unavailable. The previous workaround was a little too complicated to think about. As a rule of thumb, if something is difficult to explain to other devs on the dune team, it's going to be even more painful to the wider user base. For now, let's keep it simpler and re-evaluate once we get more feedback. If we end up needing packages with avoid-version, we can just introduce a special solver mode that allows for them (perhaps asking the users for a package white list?) |
|
I kept a slightly cleaned up version of the original workaround in the commit history. Just in case we want to revisit things. |
rgrinberg
force-pushed
the
avoid-unstable-ocaml-package
branch
from
7109404
to
9e10bd5
Compare
otherlibs/stdune/src/tuple.ml
Outdated
| @@ -20,4 +20,13 @@ module T3 = struct | |||
| let to_dyn = Dyn.triple | |||
| let hash f g h (a, b, c) = Poly.hash (f a, g b, h c) | |||
| let equal f g h (a1, b1, c1) (a2, b2, c2) = f a1 a2 && g b1 b2 && h c1 c2 | |||
|
|
|||
There was a problem hiding this comment.
This function is no longer used.
|
Looks good to me |
Signed-off-by: Rudi Grinberg <[email protected]>
rgrinberg
force-pushed
the
avoid-unstable-ocaml-package
branch
from
9e10bd5
to
e396683
Compare
Fixes #10592.
Solves an issue where the solver would choose unstable releases of the compiler. When the latest version of the "ocaml" package can only be satisfied by unstable versions of "ocaml-variants", dune would still include this unstable compiler in package solutions. This leads to users being given unstable versions of the compiler by default. This is only a problem because dune doesn't yet fully implement the avoid-version flag, and compiler packages are released under the assumption that the solver respects this flag.
The workaround introduced in this change is to determine the latest stable version of "ocaml-base-compiler" (the latest version lacking the avoid-version flag), and have the solver prefer versions of the "ocaml" package that are no later than this version.