-
Notifications
You must be signed in to change notification settings - Fork 456
Include more compiler constraints for dev tools #12834
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
base: main
Are you sure you want to change the base?
Conversation
bin/lock_dev_tool.ml
Outdated
|
|
||
| let extra_compiler_names = [ Package_name.of_string "ocaml-base-compiler" | ||
| ; Package_name.of_string "ocaml-variants" | ||
| ; Package_name.of_string "ocaml-compiler" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would make sense to share the hardcoded list of compiler names with Pkg_toolchain which (for similar reasons) has to hardcode the same list.
I would say: yes. It seems sensible to me that the dev-tools would try to pick up the right constraint for the compiler. The reason they don't at the moment seems more like an oversight than an conscious decision. |
|
Instead of relying on constraints to resolve to the same compiler version, how about just fixing it using a pin? |
|
@rgrinberg The issue is that for some dev-tools (most notably the LSP server) one needs to build them with the same version as the compiler that is used in the project. Thus we can:
At the moment we do the latter, but the only constraint we add is on |
Why is that? |
Because it's not explicitly required by the solver currently to match it, so it ends up picking up the base compiler. For instance, if your project uses |
This is exactly why I'm saying that we shouldn't rely on constraints. Running the solver for the dev tools should pin the version of the compiler so that we'll get exactly the version that we need. |
Could you please clarify what you mean by pinning here? Do you mean adding it as a pin here: Line 62 in 79509aa
|
|
Yes, that's what I mean. It is a way of directing the solver to use a particular package that does not rely on version constraints. |
|
But doesn't that require additional configuration (that the user must be aware of to do)? With the dev-tool automatically adding constraints on the ocaml compiler packages that we know about it would just work out of the box. And we already do that in some cases anyway. Should we stop doing that and ask people to instead pin the compiler? |
|
Sorry, I should probably just explain the problem clearly first. Pop quiz: What happens when a user writes this in their workspace file: And we try to solve for dev tools? Answer: there are no constraints that are powerful enough to make sure that the dev tools use this version of OCaml. Constraints on version numbers are simply not powerful enough in such cases. It's OK that we've chosen an approach that doesn't support the compiler being pinned, but ideally, I would prefer an approach that supports both pinned and unpinned compilers. Pinning on the other hand is powerful enough to support fixing the version in both cases. I'm not sure if our pinning API is powerful enough to express what we want, but fundamentally, pinning is the thing to use when we want to limit the solver to only see the "right" version of a particular package. For now, feel free to proceed with the constraints approach if this alternative proves to be much more difficult. |
|
Thanks for elaborating the problem, I see now what you mean. We can make the lock dir stanza for dev-tools also implicitly add pins on e.g. a case like this: |
|
Yes, thanks for explaining @rgrinberg — I see your point. I tend to agree with @Leonidas-from-XIV that in generic cases it could be a constraint, and that for cases where the user has pinned a compiler, we need to pin the compiler. I can look into the pinning API to see if this is possible. However, this makes me think: should packages that lie in the intersection between user-pinned packages and the dev tool’s dependencies (which the user is trying to install) also be pinned? |
|
There are a few things happening here:
|
Signed-off-by: Sudha Parimala <sudharg247@gmail.com>
Signed-off-by: Sudha Parimala <sudharg247@gmail.com>
Signed-off-by: Sudha Parimala <sudharg247@gmail.com>
cd46c2f to
44a0df7
Compare
|
I've added a cram test; this should be ready for review. I'll open an issue about the pinning part. |
Leonidas-from-XIV
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is shaping up nicely but needs a bit more work. Also to make the code more understandable without reading this issue first.
| versions. Creates the directory if it doesn't already exist. *) | ||
| val base_dir : unit -> Path.Outside_build_dir.t | ||
|
|
||
| val compiler_package_names : Dune_lang.Package_name.t list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A documentation comment would be nice here. But tbh, I am not sure this is the right place for this to live. Maybe it can be in src/dune_pkg from which both the toolchains as well as the dev-tools can use it?
| > (version 5.2.0) | ||
| > EOF | ||
|
|
||
| $ dune tools exec ocamllsp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you ask narration here that we expect ocamllsp to pick up ocaml-variants?
| > (allow_empty)) | ||
| > EOF | ||
|
|
||
| $ make_lockdir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nicer if both the project and the dev tool used the solver. That way one can also show that the result matches.
| > | ||
| > (package | ||
| > (name foo) | ||
| > (allow_empty)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a dependency on any compiler. If one were to run dune pkg lock it wouldn't pick up any compiler. That's probably not what we want.
| module Stanzas = Stanzas | ||
| module Lock_dir = Lock_dir | ||
| module Pkg_dev_tool = Pkg_dev_tool | ||
| module Pkg_toolchain = Pkg_toolchain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Pkg_toolchain.compiler_package_names could go somewhere else this doesn't need to be exposed.
| "Add a dependency on %S to one of the packages in dune-project and then \ | ||
| run" | ||
| (Package_name.to_string pkg_name) | ||
| ; User_message.command "dune pkg lock" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would drop the recommendation to run dune pkg lock here. We probably don't want to suggest people that use autolocking to suddenly opt out of it.
| let* more_compiler_packages = extra_compiler_constraints () in | ||
| let+ constraint_ = locked_ocaml_compiler_constraint () in | ||
| [ constraint_ ] | ||
| constraint_ :: more_compiler_packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have two mechanisms to add constraints? I would think that adding the constraints via extra_compiler_constraints should be sufficient. If it is not - why?
| @@ -0,0 +1,36 @@ | |||
| Test the special compiler version is picked up by ocamllsp. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be easier to call them ocaml variants, since that's what they're called upstream (instead of introducing a new term "special").
| "ocaml" package used to compile the project in the default build | ||
| context. | ||
| TODO: This only makes sure to match the default compiler packages (defined in | ||
| Pkg_toolchain). This won't work for custom compilers added as pins. Ideally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to make sense to me. It is not defined in Pkg_toolchain, it is defined here?
|
|
||
| let locked_ocaml_compiler_version () = | ||
| let open Memo.O in | ||
| let* packages = load_packages () in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make that let+ and avoid the Memo.return below, but YMMV :)
This adds as dependencies more compiler constraints to dev tools if it needs to use the same compiler.
I was discussing with @Alizter last week that when you try to install
ocamllspin a project built with OxCaml, it doesn't automatically pickup the OxCaml version ofocaml-lsp-serverand that one has to explicitly specify this indune-workspace. Digging further, I found that the packageocamlis only considered as a compiler package, whereas OxCaml and other compiler branches exist asocaml-variants. To this end, it seemed like it would make sense to add them as compiler dependencies.This PR is not in the best shape, but I'm opening it early to ask if this is something we should be doing while locking dev tools. If this is worth adding, I'll work on refactoring this a bit, add a cram test, and make it ready for review.
I've run some basic tests with this branch and it does pickup the right dev tools on OxCaml projects.