feat: Add support for opam-switch-mode (>= 1.6)

[erikmd]

Overall description

This PR adds support for opam-switch-mode (initially written by @hendriktews for proof-general then as a separate ELPA package, further extended by @monnier and myself).

This minor mode is similar to tuareg's feature M-x tuareg-opam-update-env but it brings several additional features:

• (since version ≥ 1.4) it displays the currently chosen switch in the mode-line;
• it adds an "OPSW" menu-bar (also working in TTY) and an "OPSW" mode-bar menu, allowing to select a global opam switch or a local opam switch;
• it triggers the functions registered in opam-switch-mode's hook (typically, tuareg-kill-ocaml (and likewise for merlin-stop-server));
• and finally it stores the initial environment (before the mode is enabled), providing a "reset" feature to backtrack to the initially chosen opam switch.

See the following screenshot of opam-switch-mode version 1.3 for a glimpse of the minor mode.

Remarks

• This PR does NOT add opam-switch-mode as a dependency of tuareg: it is an optional, accompanying package (only useful for ocaml developers that rely on opam), and it is mostly language-agnostic (apart from using the opam tool !-)
• The opam-switch-mode package is available in MELPA, MELPA Stable, NonGNU-devel, NonGNU.
• It requires the following setting: (setq tuareg-opam-insinuate nil), but fortunately, this is the default value of the option. Edit: this constraint is now relaxed.
• The only Package-Requires: of opam-switch-mode is (emacs "25.1"), which is compatible with Tuareg's one.

Details of the commit

• Add option tuareg-kill-ocaml-on-opam-switch (default: t) and related function (tuareg--kill-ocaml-on-opam-switch) that kills the ocaml toplevel just before we change the opam switch using the OPSW menu-bar or the OPSW mode-bar from opam-switch-mode.

[erikmd]

Hi @monnier !, as said earlier in the other PR, I'll wait for ocaml/merlin#1654 to be finalized before applying changes here.

[erikmd]

Dear @monnier, I believe this PR is ready! To sum up:

• I "cherry-picked" the improvements you had suggested in the similar Merlin PR.
• I pushed a release of opam-switch-mode (1.6) that makes it possible to use tuareg and opam-switch-mode with both (setq tuareg-opam-insinuate nil) and (setq tuareg-opam-insinuate t).

[erikmd]

Copy of the review @monnier just emailed me:

diff --git a/tuareg-opam.el b/tuareg-opam.el index 658e9f3f0c..b3bd0395b2 100644 --- a/tuareg-opam.el +++ b/tuareg-opam.el @@ -348,7 +348,8 @@ issue an \"opam switch\" in a shell. If this variable is set to t, Tuareg will try to use opam to set the right environment for compile',run-ocaml' and merlin-mode' based on the current opam switch at the time the command is run (provided opam is -found). You may also usetuareg-opam-update-env' to set the +found). You may also use tuareg-opam-update-env', or the menus +from the ELPA packageopam-switch-mode', to set the environment for another compiler from within emacs (without changing the opam switch). Beware that setting it to t causes problems if you compile under tramp."

Est-ce que le "(without changing the opam switch)" est encore applicable ici? En tout cas, ça sonne bizarre, ça semble dire qu'opam-switch-mode ne change pas l'Opam switch.

Faudrait-il rephraser genre "without affecting the Opam switches used outside of this Emacs session"?

@@ -382,7 +383,11 @@ error message as a string)."

;;;###autoload (defun tuareg-opam-update-env (switch)

• "Update the environment to follow current OPAM switch configuration."
• "Update the environment to follow current OPAM switch configuration.

• +You may also be interested in the ELPA package opam-switch-mode' that +provides a similar feature, along with a menu-bar and a mode-bar menu +\"OPSW\"'; see https://github.com/ProofGeneral/opam-switch-mode." (interactive (let* ((compl (tuareg-opam-installed-compilers)) (current (tuareg-opam-current-compiler))

This is arguably the more interesting part of the interaction. I see that tuareg-opam-installed-compilers uses

opam switch list -i -s
opam switch list -s

whereas opam-switch--get-switches uses just opam switch with a FIXME suggesting that Tuareg's code is preferable. Should we try and improve opam-switch-modes code to be "at least as good" as Tuareg's?

If the users call tuareg-opam-update-env while also using opam-switch-mode, doesn't opam-switch-mode get confused (e.g. AFAICT it doesn't update the switch that's displayed in the mode line).

Also, I wonder if we should offer to hijack Tuareg's C-c C-w keybinding or even directly make tuareg-opam-update-env delegate its job to opam-switch-set-switch (under appropriate conditions, of course).

+(defcustom tuareg-kill-ocaml-on-opam-switch t

• "If t, kill the OCaml toplevel before the opam switch changes. +If the user changes the opam switch using opam-switch-set-switch' +or an\"OPSW\"' menu from `opam-switch-mode', this option asks to +kill the OCaml toplevel process, so that the next eval command +starts a new process, typically with a different OCaml version +from a different opam switch.

I think this description is too close to the code. How 'bout:

  "If t, kill Tuareg subprocesses when the Opam switch changes.
If the user changes the opam switch using `opam-switch-mode',
this option asks to kill the subprocesses that are using
the old switch.

[ BTW, opam-switch-mode could almost do it on its own, in general, by going through process-list and killing those processes that were launched with another switch. For that we'd probably need an advice on make-process to record the current switch when a process is launched. And I guess some packages may not like their process to be killed without warning "from the outside". ]

I notice that tuareg-opam-update-env did not kill existing subprocesses. Do you know why there is this difference between what tuareg-opam-update-env does and what opam-switch-set-witch does after your patch?

+opam-switch-mode' 1.6+ is compatible withtuareg-mode' whatever +is the value of tuareg-opam-insinuate' (albeit the default value +nil is recommended as it omits the\"opam exec --\"' wrapper)."

Doesn't seem relevant. Sounds like listing one of the bugs we could have had: there would be many more candidates if we decided to go down that route, no?

[erikmd]

And my reply:

On Tuesday, July 18, 2023 15:47 CEST, Stefan Monnier wrote:

diff --git a/tuareg-opam.el b/tuareg-opam.el index 658e9f3f0c..b3bd0395b2 100644 --- a/tuareg-opam.el +++ b/tuareg-opam.el @@ -348,7 +348,8 @@ issue an \"opam switch\" in a shell. If this variable is set to t, Tuareg will try to use opam to set the right environment for compile',run-ocaml' and merlin-mode' based on the current opam switch at the time the command is run (provided opam is -found). You may also usetuareg-opam-update-env' to set the +found). You may also use tuareg-opam-update-env', or the menus +from the ELPA packageopam-switch-mode', to set the environment for another compiler from within emacs (without changing the opam switch). Beware that setting it to t causes problems if you compile under tramp."

Est-ce que le "(without changing the opam switch)" est encore applicable ici? En tout cas, ça sonne bizarre, ça semble dire qu'opam-switch-mode ne change pas l'Opam switch.

Non, this only means it change the environment vars inside emacs, so if we just type "opam switch" in a shell outside emacs, it is unchanged, so there is no unwanted side-effect.

Faudrait-il rephraser genre "without affecting the Opam switches used outside of this Emacs session"?

Yes that looks better, I will change the PR in this way.

@@ -382,7 +383,11 @@ error message as a string)."

;;;###autoload (defun tuareg-opam-update-env (switch)

• "Update the environment to follow current OPAM switch configuration."
• "Update the environment to follow current OPAM switch configuration.

• +You may also be interested in the ELPA package opam-switch-mode' that +provides a similar feature, along with a menu-bar and a mode-bar menu +\"OPSW\"'; see https://github.com/ProofGeneral/opam-switch-mode." (interactive (let* ((compl (tuareg-opam-installed-compilers)) (current (tuareg-opam-current-compiler))

This is arguably the more interesting part of the interaction. I see that tuareg-opam-installed-compilers uses

opam switch list -i -s
opam switch list -s

whereas opam-switch--get-switches uses just opam switch with a FIXME suggesting that Tuareg's code is preferable.

You mention this FIXME? ;; FIXME: Use "opam switch -s" ? Then, I did add this FIXME last week since it is indeed a short-term improvement I plan to do, reusing directly a sexp from opam.

Should we try and improve opam-switch-modes code to be "at least as good" as Tuareg's?

Yes exactly, FYI I've just opened issue https://github.com/ProofGeneral/opam-switch-mode/issues/15 but it seems it's not blocking for the integration of tuareg/opam-switch-mode (?)

If the users call tuareg-opam-update-env while also using opam-switch-mode, doesn't opam-switch-mode get confused (e.g. AFAICT it doesn't update the switch that's displayed in the mode line).

Yes, this is because you recently made me improve the implementation of (opam-switch-mode-lighter) 🙂 if we do (setq opam-switch--mode-lighter nil), then the lighter is OK.

I see only two solutions: → either revert the memoization commit https://github.com/ProofGeneral/opam-switch-mode/commit/d3a75365702ae49eaad9324bbb90f0234a4df451 → or ensure that opam-switch-set-switch is always called

Also, I wonder if we should offer to hijack Tuareg's C-c C-w keybinding or even directly make tuareg-opam-update-env delegate its job to opam-switch-set-switch (under appropriate conditions, of course).

A job delegation to opam-switch-set-switch looks a good idea, indeed (see also below)

+(defcustom tuareg-kill-ocaml-on-opam-switch t

• "If t, kill the OCaml toplevel before the opam switch changes. +If the user changes the opam switch using opam-switch-set-switch' +or an\"OPSW\"' menu from `opam-switch-mode', this option asks to +kill the OCaml toplevel process, so that the next eval command +starts a new process, typically with a different OCaml version +from a different opam switch.

I think this description is too close to the code. How 'bout:

  "If t, kill Tuareg subprocesses when the Opam switch changes.
If the user changes the opam switch using `opam-switch-mode',
this option asks to kill the subprocesses that are using
the old switch.

Good point, your phrasing is more to the point! I will update the PR.

[ BTW, opam-switch-mode could almost do it on its own, in general, by going through process-list and killing those processes that were launched with another switch. For that we'd probably need an advice on make-process to record the current switch when a process is launched.

It could, but for now I only see 3 packages involving opam-based subprocesses: ProofGeneral, tuareg, merlin. Maybe this is a bit too abrupt as a behavior, especially as you mentioned:

And I guess some packages may not like their process to be killed without warning "from the outside". ]

I notice that tuareg-opam-update-env did not kill existing subprocesses. Do you know why there is this difference between what tuareg-opam-update-env does and what opam-switch-set-witch does after your patch?

Thanks for your testing! The difference between vanilla tuareg and opam-switch-mode is summarized in this comment: https://github.com/ocaml/tuareg/pull/307#issue-1799840345 ↓

This minor mode is similar to tuareg's feature M-x tuareg-opam-update-env but it brings several additional features:

• (since version ≥ 1.4) it displays the currently chosen switch in the mode-line;
• it adds an "OPSW" menu-bar (also working in TTY) and an "OPSW" mode-bar menu, allowing to select a global opam switch or a local opam switch;
• it triggers the functions registered in opam-switch-mode's hook (typically, tuareg-kill-ocaml (and https://github.com/ocaml/merlin/pull/1654 for merlin-stop-server));
• and finally it stores the initial environment (before the mode is enabled), providing a "reset" feature to backtrack to the initially chosen opam switch.

I think we can see see tuareg-opam-update-env as the legacy implementation of the feature, and in particular, it does not provide this subprocesses-exit currently. On the one hand, I wouldn't want to suggest we remove/deprecate tuareg's function as I believe it is useful if opam-switch-mode keeps an optional minor mode, but on the other hand, to overcome the potential surprise of users that already know tuareg-opam-update-env and take your remark into account, I believe the best trade-off would be to add anif in tuareg and ensure that tuareg-opam-update-env calls opam-switch-set-switch if the opam-switch-mode is found&enabled.

+opam-switch-mode' 1.6+ is compatible withtuareg-mode' whatever +is the value of tuareg-opam-insinuate' (albeit the default value +nil is recommended as it omits the\"opam exec --\"' wrapper)."

Doesn't seem relevant. Sounds like listing one of the bugs we could have had: there would be many more candidates if we decided to go down that route, no?

No, it is not a bug report. This is intended to highlight the fact that opam-switch-mode has no constraint w.r.t. tuareg-opam-insinuate: it is fully compatible with tuareg. I believe it is the mention of version "1.6+" that looked weird to you. I will remove it, let me know if that's OK with you.

[monnier]

You mention this FIXME? ;; FIXME: Use "opam switch -s" ?

Yes.

Then, I did add this FIXME last week since it is indeed a short-term improvement I plan to do, reusing directly a sexp from opam.

tuareg-opam.el also uses the -i flag, but I can't find what this flag does in the Opam doc (neither could I find the doc for -s, tho).

Yes exactly, FYI I've just opened issue https://github.com/ProofGeneral/opam-switch-mode/issues/15 but it seems it's not blocking for the integration of tuareg/opam-switch-mode (?)

Agreed.

If the users call tuareg-opam-update-env while also using opam-switch-mode, doesn't opam-switch-mode get confused (e.g. AFAICT it doesn't update the switch that's displayed in the mode line). Yes, this is because you recently made me improve the implementation of (opam-switch-mode-lighter) 🙂

Indeed, that's the one I noticed, but looking at the code, I see that it will also "fail" to run things like opam-switch-change-opam-switch-hook.

if we do (setq opam-switch--mode-lighter nil), then the lighter is OK.

Adding such a setq in tuareg-opam.el would be ugly, but it would also be a harmless quickfix, so we could tolerate it. I'd rather delegate to opam-switch-set-switch, tho.

I think we can see see `tuareg-opam-update-env' as the legacy implementation of the feature, and in particular, it does not provide this subprocesses-exit currently.

My question was trying to find the reason why the old code did not kill, so if I read between the lines you're suggesting that there was no deep reason, it was just a missing feature?

On the one hand, I wouldn't want to suggest we remove/deprecate tuareg's function as I believe it is useful if opam-switch-mode keeps an optional minor mode, but on the other hand, to overcome the potential surprise of users that already know tuareg-opam-update-env and take your remark into account, I believe the best trade-off would be to add anif in tuareg and ensure that tuareg-opam-update-env calls opam-switch-set-switch if the opam-switch-mode is found&enabled.

Yes, I suspect we should deprecate tuareg-opam-update-env. I.e. mark it as obsolete, and make it delegate to opam-switch-set-switch when "found&enabled". I think it could be a bit cumbersome to delegate the interactive spec, but delegating the actual body of the function should be easy.

+opam-switch-mode' 1.6+ is compatible withtuareg-mode' whatever +is the value of tuareg-opam-insinuate (albeit the default value +nil is recommended as it omits the \"opam exec --\"' wrapper)." Doesn't seem relevant. Sounds like listing one of the bugs we could have had: there would be many more candidates if we decided to go down that route, no? No, it is not a bug report. This is intended to highlight the fact thatopam-switch-modehas no constraint w.r.t.tuareg-opam-insinuate`: it is fully compatible with tuareg.

But I see no reason why the reader would presume that there would be such an incompatibility. I think this is only present because an earlier version of the patch did have such a limitation, so you're just documenting the absence of a bug that noone other than you has seen anyway. We rarely document bugs in docstrings, so it seems definitely odd to document their absence :-)

[ BTW, the code for tuareg-opam-insinuate is rather crude. I suspect/hope it's not used very much. ]

    Stefan

[erikmd]

Dear @monnier, Ideas on how to fix this bytecompile error?

In toplevel form:
tuareg-opam.el:403:18:Error: reference to free variable ‘opam-switch-set-switch’

[erikmd]

@monnier To reply your other questions:

tuareg-opam.el also uses the -i flag, but I can't find what this flag does in the Opam doc

IINM, the -i flag was just the opam 1.x phrasing (opam version now deprecated since a long time)

My question was trying to find the reason why the old code did not kill, so if I read between the lines you're suggesting that there was no deep reason, it was just a missing feature?

Yes (just a missing feature :)

I think it could be a bit cumbersome to delegate the interactive spec, but delegating the actual body of the function should be easy.

done: to do a delegation for the interactive spec as well, I added the code below.

 (defun tuareg--shell-command-to-string (command)
@@ -385,23 +385,30 @@ error message as a string)."
 (defun tuareg-opam-update-env (switch)
   "Update the environment to follow current OPAM switch configuration.

-You may also be interested in the ELPA package `opam-switch-mode' that
-provides a similar feature, along with a menu-bar and a mode-bar menu
-`\"OPSW\"'; see https://github.com/ProofGeneral/opam-switch-mode."
+Delegate the task to `opam-switch-set-switch' if the minor mode
+`opam-switch-mode' (https://github.com/ProofGeneral/opam-switch-mode)
+is installed. This ELPA package also provides a menu-bar and a
+mode-bar menu `\"OPSW\"'."
   (interactive
    (let* ((compl (tuareg-opam-installed-compilers))
           (current (tuareg-opam-current-compiler))
           (default (if current current "current"))
           (prompt (format "opam switch (default: %s): " default)))
-     (list (completing-read prompt compl))))
-  (let* ((switch (if (string= switch "") nil switch))
-         (env (tuareg-opam-config-env switch)))
-    (if env…
+     (if (featurep 'opam-switch-mode)
+         (list "")
+       (list (completing-read prompt compl)))))
+  (if (featurep 'opam-switch-mode)
+      (if (called-interactively-p 'any)
+          (call-interactively #'opam-switch-set-switch)
+        (funcall opam-switch-set-switch switch))
+    (let* ((switch (if (string= switch "") nil switch))
+           (env (tuareg-opam-config-env switch)))
+      (if env… ; same code afterwards

Unfortunately, this triggers an error currently: https://github.com/ocaml/tuareg/pull/307#issuecomment-1640476345

In toplevel form:
tuareg-opam.el:403:18:Error: reference to free variable ‘opam-switch-set-switch’

Ideas?

But I see no reason why the reader would presume that there would be such an incompatibility.

I don't think so, the tuareg-opam-insinuate option intends to "better locate the ocaml binary if opam is installed", so in terms of scope, it cleary ovelaps opam-switch-mode's feature.

[monnier]

@@ -385,23 +385,30 @@ error message as a string)." (defun tuareg-opam-update-env (switch) "Update the environment to follow current OPAM switch configuration.

-You may also be interested in the ELPA package opam-switch-mode' that -provides a similar feature, along with a menu-bar and a mode-bar menu -\"OPSW\"'; see https://github.com/ProofGeneral/opam-switch-mode." +Delegate the task to opam-switch-set-switch' if the minor mode +opam-switch-mode' (https://github.com/ProofGeneral/opam-switch-mode) +is installed. This ELPA package also provides a menu-bar and a +mode-bar menu `\"OPSW\"'." (interactive (let* ((compl (tuareg-opam-installed-compilers)) (current (tuareg-opam-current-compiler)) (default (if current current "current")) (prompt (format "opam switch (default: %s): " default)))

• (list (completing-read prompt compl))))
• (if (featurep 'opam-switch-mode)
• (list "")
• (list (completing-read prompt compl)))))

featurep tests whether the code was loaded, which is usually something Emacs does on-demand behind the scenes rather than something the user is expected to be aware of.

I'd recommend you use either (bound-and-true-p 'opam-switch-mode), i.e. test if the user activated the minor mode, or (fboundp 'opam-switch-set-switch), i.e. test whether opam-switch-mode is available.

Nitpick1: (list "") can be optimized to '("") :-) Nitpick2: "" is a value which could also be returned by the other branch of the if, so I'd personally tend to return some dummy value like please-use-opam-switch-mode-interactively instead.

• (if (featurep 'opam-switch-mode)

Same as above, of course

• (if (called-interactively-p)

called-interactively-p sucks, but we can test the special dummy value proposed above instead.

• (call-interactively #'opam-switch-set-switch)
• (funcall opam-switch-set-switch switch))

There's a #' missing after the funcall or (better) the funcall is extraneous. Note that those #' will cause compiler warnings when opam-switch-mode is not available. We can silence them by testing (fboundp 'opam-switch-set-switch) or with the use of declare-function.

    Stefan

[erikmd]

I'd recommend you use either (bound-and-true-p 'opam-switch-mode), i.e. test if the user activated the minor mode,

This is not a good option, as it is possible to call the interactive function in another buffer where opam-switch-mode is not enabled.

or (fboundp 'opam-switch-set-switch), i.e. test whether opam-switch-mode is available.

Looks better, thanks!

[erikmd]

Just force-pushed. Let's wait for the CI… Thanks @monnier for your input! 👍

[erikmd]

Good news 🙃, the CI went fine! thanks to your suggestion regarding (fboundp 'opam-switch-set-switch)

[erikmd]

Wait a minute, was the '("please-use-opam-switch-mode-directly") string really necessary?

[erikmd]

Wait a minute, was the '("please-use-opam-switch-mode-directly") string really necessary?

(Hopefully this does not cause any unwanted side-effect.)

[erikmd]

OK, after more testing in all cases, looks very fine! 😅 FYI the final version of the code uses '("Please use opam-switch-mode interactively"):

  (interactive
   (let* ((compl (tuareg-opam-installed-compilers))
          (current (tuareg-opam-current-compiler))
          (default (if current current "current"))
          (prompt (format "opam switch (default: %s): " default)))
     (if (fboundp 'opam-switch-set-switch)
         '("Please use opam-switch-mode interactively")
       (list (completing-read prompt compl)))))
  (if (fboundp 'opam-switch-set-switch)
      (if (called-interactively-p 'any)
          (call-interactively #'opam-switch-set-switch)
        (opam-switch-set-switch switch))
    (let* ; etc.

[monnier]

Wait a minute, was the '("please-use-opam-switch-mode-directly") string really necessary?

No, and it was definitely not meant to be a string (tho unlikely it's still hypothetically imaginable to have an Opam switch named "Please use opam-switch-mode interactively").

I pushed it to master with this change. Crossing fingers,

    Stefan

[erikmd]

Thanks @monnier! so I close the PR.

Regarding the CI of master, the only issue is the following one:

In toplevel form:
tuareg.el:1496:34: Error: ‘tuareg-opam-update-env’ is an obsolete function (as of 2023-07); use ‘opam-switch-set-switch’ instead.

[erikmd]

tuareg.el:1496:34: Error: ‘tuareg-opam-update-env’ is an obsolete function (as of 2023-07); use ‘opam-switch-set-switch’ instead.

regarding this error, two suggestions @monnier:

1. either you remove the line 1496 (define-key map "\C-c\C-w" #'tuareg-opam-update-env), and I add the key-binding in opam-switch-mode;
2. or you keep the key-binding in tuareg, and you remove the obsolete clause, which makes sense also by considering that opam-switch-mode stays an "optional dependency".

→1. is "cleaner" maybe, but small preference for 2. as it is simpler.

Let me know what your prefer.

[erikmd]

Also, an issue for solution 1. is that opam-switch-mode is useful for ProofGeneral as well, but C-c C-w is already bound in PG: C-c C-w runs the command pg-response-clear-displays.

[monnier]

Regarding the CI of master, the only issue is the following one:

In toplevel form:
tuareg.el:1496:34: Error: ‘tuareg-opam-update-env’ is an obsolete function
(as of 2023-07); use ‘opam-switch-set-switch’ instead.

Where did you find that info? All I could see was "Error" :-( In any case it seems to be fixed now, thanks.

[erikmd]

Where did you find that info?

I browsed https://github.com/ocaml/tuareg/commits/master then opened the full log of the CI run; the direct link being available when clicking on the tick (:heavy_check_mark: or :x:) near the SHA1 of the commit, cf. the following two screenshots:

[erikmd]

In any case it seems to be fixed now

IINM, there is no error anymore because of using quoting rather than sharp-quoting after your last commit:

    (define-key map "\C-c\C-w" (if (fboundp 'opam-switch-set-switch)
                                   #'opam-switch-set-switch
                                 'tuareg-opam-update-env))

• To fix this, I just opened PR #308

[monnier]

IINM, there is no error anymore because of using quoting rather than sharp-quoting after your [last commit]

Yes, that's why I used a non-sharp quote. The alternative is to use with-suppressed-warnings which doesn't seem worth the trouble.

    Stefan