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

erikmd · 2023-07-11T22:24:15Z

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 · 2023-07-12T19:50:34Z

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

* 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. * See: https://github.com/ProofGeneral/opam-switch-mode, a package available in MELPA, MELPA Stable, NonGNU-devel, NonGNU that just requires (emacs "25.1"). * This feature typically expects `tuareg-opam-insinuate` to be nil.

erikmd · 2023-07-13T22:50:14Z

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 · 2023-07-18T14:33:44Z

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 use tuareg-opam-update-env' to set the
+found). You may also use tuareg-opam-update-env', or the menus +from the ELPA package opam-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 with tuareg-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 · 2023-07-18T14:42:06Z

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 use tuareg-opam-update-env' to set the
+found). You may also use tuareg-opam-update-env', or the menus +from the ELPA package opam-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 ProofGeneral/opam-switch-mode#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
ProofGeneral/opam-switch-mode@d3a7536
→ 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:
#307 (comment) ↓

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 feat(emacs): Add support for opam-switch-mode (>= 1.3) merlin#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 with tuareg-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 · 2023-07-18T15:30:21Z

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 ProofGeneral/opam-switch-mode#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 an`if` 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 with `tuareg-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.

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 · 2023-07-18T15:40:29Z

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 · 2023-07-18T15:48:44Z

@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:
#307 (comment)

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 · 2023-07-18T15:51:03Z

@@ -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 · 2023-07-18T15:52:54Z

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 · 2023-07-18T15:57:18Z

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

erikmd · 2023-07-18T16:04:42Z

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

erikmd · 2023-07-18T16:06:10Z

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

erikmd · 2023-07-18T16:06:45Z

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

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

(tuareg-opam-update-env): call #'opam-switch-set-switch if (featurep 'opam-switch-mode)

erikmd · 2023-07-18T16:16:45Z

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 · 2023-07-18T19:26:41Z

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 · 2023-07-18T19:57:58Z

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 · 2023-07-18T20:07:08Z

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:

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;
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 · 2023-07-18T20:11:30Z

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 · 2023-07-18T21:01:31Z

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 · 2023-07-18T21:14:58Z

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 · 2023-07-18T21:26:50Z

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 fix: tuareg: use #'; don't mark (tuareg-opam-update-env) as obsolete & docs: Update README.md #308

monnier · 2023-07-18T22:27:00Z

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

This was referenced Jul 11, 2023

tureg+merlin support ProofGeneral/opam-switch-mode#9

Closed

feat(emacs): Add support for opam-switch-mode (>= 1.3) ocaml/merlin#1654

Merged

erikmd force-pushed the opam-switch-mode branch from 6c8aa11 to 50e91db Compare July 13, 2023 22:46

erikmd changed the title ~~feat: Add opam-switch-mode support~~ feat: Add support for opam-switch-mode (>= 1.6) Jul 13, 2023

erikmd force-pushed the opam-switch-mode branch from 326c003 to a2f1ea4 Compare July 18, 2023 15:32

erikmd force-pushed the opam-switch-mode branch from a2f1ea4 to 3d66928 Compare July 18, 2023 15:57

Apply review suggestions by @monnier

ddae9ef

(tuareg-opam-update-env): call #'opam-switch-set-switch if (featurep 'opam-switch-mode)

erikmd force-pushed the opam-switch-mode branch from 3d66928 to ddae9ef Compare July 18, 2023 16:15

erikmd closed this Jul 18, 2023

erikmd mentioned this pull request Jul 18, 2023

fix: tuareg: use #'; don't mark (tuareg-opam-update-env) as obsolete & docs: Update README.md #308

Closed

erikmd deleted the opam-switch-mode branch July 18, 2023 22:31

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

erikmd commented Jul 11, 2023 •

edited

Loading

erikmd commented Jul 12, 2023 •

edited

Loading

erikmd commented Jul 13, 2023 •

edited

Loading

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023 •

edited

Loading

monnier commented Jul 18, 2023 via email

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

monnier commented Jul 18, 2023 via email

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

monnier commented Jul 18, 2023 via email

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

monnier commented Jul 18, 2023 via email

erikmd commented Jul 18, 2023 •

edited

Loading

erikmd commented Jul 18, 2023 •

edited

Loading

monnier commented Jul 18, 2023 via email

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

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

Conversation

erikmd commented Jul 11, 2023 • edited Loading

Overall description

Remarks

Details of the commit

erikmd commented Jul 12, 2023 • edited Loading

erikmd commented Jul 13, 2023 • edited Loading

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023 • edited Loading

monnier commented Jul 18, 2023 via email

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

monnier commented Jul 18, 2023 via email

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

monnier commented Jul 18, 2023 via email

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

erikmd commented Jul 18, 2023

monnier commented Jul 18, 2023 via email

erikmd commented Jul 18, 2023 • edited Loading

erikmd commented Jul 18, 2023 • edited Loading

monnier commented Jul 18, 2023 via email

erikmd commented Jul 11, 2023 •

edited

Loading

erikmd commented Jul 12, 2023 •

edited

Loading

erikmd commented Jul 13, 2023 •

edited

Loading

erikmd commented Jul 18, 2023 •

edited

Loading

erikmd commented Jul 18, 2023 •

edited

Loading

erikmd commented Jul 18, 2023 •

edited

Loading