Avoid building the compiler in the sandbox #56
Conversation
Speed up the workflow by using the same compiler as in the user's switch. This uses the same mechanism as for the "system" switch: the system switch doesn't contain the compiler and the usual PATH lookup will fallback to the compiler installed on the system. This approach doesn't manipulate the PATH variable but directly add symlinks pointing to the compiler tools into the sandbox switch. The "sys-ocaml-version" variable must be temporarily set, as Opam will check that the version of the "system" compiler matches.
Helps when testing manually.
Co-authored-by: Paul-Elliot <[email protected]>
|
It would be nice to have a test, as @panglesd suggested. I'm working on it. |
|
The CI is catching a serious problem, I'm working on it. We cannot simply symlink the "parent" compiler into the sandbox This script assumes that the compiler is not placed into the same directory, otherwise it calls itself in an infinite loop that exhaust the resources. The fix is to change the symlinking strategy: place the symlinks into a temporary directory and add it to the |
We cannot simply symlink the "parent" compiler into the sandbox "bin"
folder because `ocamlfind` generates a script like this:
BINDIR=$(dirname "$(command -v ocamlc)")
"$BINDIR/ocaml" -I "$OCAML_TOPLEVEL_PATH" "$@"
This script assumes that the compiler is not placed into the same
directory, otherwise it calls itself in an infinite loop.
The fix is to change the symlinking strategy: place the symlinks into a
temporary directory and add it to the "PATH" just before calling
commands inside the sandbox.
The "opam_opts" mechanism is perfect for that.
Some important tests were missing. Also, remove a bit of duplication.
It is no longer needed and it now fails.
|
This is ready for review. |
| Opam.Config.Var.get { opam_opts with switch = Some sw } "prefix" | ||
| let* prefix = Opam.Config.Var.get sandbox_opts "prefix" >>| Fpath.v in | ||
| let* () = | ||
| if List.exists (( = ) sw) all_sw then Ok () |
There was a problem hiding this comment.
I'm curious here. Iiuc, this line makes happen the following: if the user happens to have a switch called opam-tools-4.14.0, then ocaml-platform won't notice that. Instead, it will install the platform tools in that switch (which might not be the currently selected switch) and will then remove that switch. Is that right?
If so, that's probably not the kind of situation you had in mind when writing this line. So I'm wondering: what did you have in mind? Did you have in mind the situation that at some time in the past when the user ran ocaml-platform, the sandbox switch didn't get removed correctly and hence there's still an old sandbox switch around that can be re-used?
There was a problem hiding this comment.
I think when the condition of this line is true, ocaml-platform should probably fail. Do you agree?
We could also consider providing an ocaml-platform clean-up functionality or similar to do a "manual" clean-up in case the automatic ocaml-platform clean-up went wrong in the past (and here we could then point to that clean-up command). What do you think?
There was a problem hiding this comment.
This line was here already, I planned on fixing it later.
Ideally, I don't want any of failing or having a clean-up command. My plan is to:
- Make the sandbox switch name non deterministic. This removes the check and the eventual error.
- Hide the sandbox switch from Opam and place it into a temporary directory. The cleanup is done by the system if ours went wrong. Bonus: The user can't see the switch either.
I don't know if point 2 is possible at all. I would like to avoid using a separate opam root at first but that also a solution to try.
There was a problem hiding this comment.
Sounds good to me!
Until we get to implementing that, I'd fail in the situation here instead of going along and risking removing a user's switch. But if you prefer, we can do that in a separate PR (as you've pointed out: you've just moved this piece of code in this PR, not introduced it).
There was a problem hiding this comment.
It will be a separate PR :)
|
Let's merge! |
Re-open of https://github.com/tarides/ocaml-platform/pull/52. I had to remove and re-create my Github fork after the main repository has been made public.