tarides / ocaml-platform-installer

The best way for developers to write software in OCaml
ISC License
61 stars 8 forks source link

Create the sandbox switch in a temporary directory #67

Closed Julow closed 2 years ago

Julow commented 2 years ago

The sandbox switch is now a local switch into a temporary directory, which can be cleaned up more easily and can't be used concurrently.

It still shows in 'opam switch list' but it now has a weirder name.

A cleanup step is still needed, but if it fails, the switch will eventually be removed by the system and Opam will automatically cleanup its states. We don't remove the reference to the sandbox switch from Opam's state because it's still useful to know about switch that would not be cleaned up automatically by the system.

Julow commented 2 years ago

I think this closes https://github.com/tarides/ocaml-platform-installer/issues/39 We don't try to handle signals (that would be too much) but we make sure that left overs are harmless and eventually cleared. Using a different root is not necessary, it wouldn't solve more.

This leaves enabling the repo. I don't see a solution but I think it's not a big deal if it stays activated. Opam was also shutdown and the user's switch might also be in an inconsistent state and the platform installer might need to be re-run anyway. (TODO: Check that we don't crash if the cache repo was already enabled while we enable it again)

panglesd commented 2 years ago

I don't think it closes #39 (although it "mostly" closes it), as C-c user interruption are quite common in operations that take a long time. Maybe just adding Sys.catch_break true at the beginning of the program, in addition to your PR, would close #39.

Julow commented 2 years ago

I agree this doesn't close #39. Sys.catch_break is not enough, we first need to register more things to at_exit: everything we currently wrap into a Fun.protect.

Let's merge this when the CI returns.

Julow commented 2 years ago

The CI failure is due to Yojson's release. I'll merge now