create a custom variable for process-connection-type

bufordrat commented 4 months ago

Summary

The proposal in this issue is to make process-connection-type customizable.

Background

In commit cd5229e, in order to address #83, process-connection-type was changed from nil to t. This allowed Tuareg-Interactive mode to display the output of ctypes functions when they wrote to stdout. However, a (presumably unintended) consequence of this change was that it became impossible to run utop under Tuareg-Interactive mode, which previously worked smoothly. When process-connection-type is set to t, utop becomes unusable when run by tuareg-run-ocaml.

For more information, please see the OCaml Discuss thread that uncovered this issue: https://discuss.ocaml.org/t/comint-version-of-utop-in-emacs/14466

Proposal

A forthcoming PR will make the value of process-connnection-type configurable via Emacs' customize facility. The default value will be t, which means that this change will have no discernible effect for most Tuareg users. But the Emacs user will be able to set process-connection-type to nil if they would like to use tuareg-run-ocaml with utop. Having this ability will be incredibly valuable, especially to those of us at the UChicago Library who run utop under Tuareg.

monnier commented 4 months ago

When process-connection-type is set to t, utop becomes unusable when run by tuareg-run-ocaml. For more information, please see the OCaml Discuss thread that uncovered this issue: https://discuss.ocaml.org/t/comint-version-of-utop-in-emacs/14466

Apparently utop by default checks whether it's connected to a PTY to decide whether to use the "dumb" UI or the fancy UI that relies on ANSI escape sequences.

I don't like the idea of changing process-connection-type for that. I see two other solutions:

Change Utop so it (also) pays attention to $TERM. Tuareg runs the subprocess with a $TERM set to "dumb" which should instruct the subprocess to not use ANSI escapes.
If this is not an option, then pass utop an option that requests the "dumb" UI.

bufordrat commented 4 months ago

These are good points, and I would definitely be interested in working on a PR to the utop project that recommends one of these changes. However, I wonder whether you would be willing to consider the code in the PR associated with this issue as a short-term solution, for the following reasons:

prior to commit cd5229e, process-connection-type was in fact hardcoded to nil with no problems that I know of, other than the inability to display the output of ctypes functions
there are no major downsides (at least none that I can think of) to giving Tuareg users the ability to customize how the Tuareg interactive process creates a REPL
doing so immediately fixes a broken dev environment, which seems like a high payoff

In other words, this customization solution seems very low-cost, and will help Tuareg users waiting for an upstream utop fix. If future Tuareg users of future REPLs have a similar problem, they will also avoid having a similar wait-time till solution.

monnier commented 4 months ago

These are good points, and I would definitely be interested in working on a PR to the utop project that recommends one of these changes. However, I wonder whether you would be willing to consider the code in the PR associated with this issue as a short-term solution, for the following reasons:

I think rather add a specific workaround in tuareg.el and ask users to

(setq tuareg-process-connection-type nil)

in their init file, I'd rather tell them they don't need to upgrade Tuareg and can use

;; Workaround problem where UTop assumes it's running in an
;; ANSI-compatible terminal (issue #314 of Tuareg).
(advice-add 'make-comint :around #'my-utop-workaround)
(defun my-utop-workaround (orig-fun name &rest args)
  (if (not (equal name "OCaml"))
      (apply orig-fun name args)
    (let ((process-connection-type nil))
      (apply orig-fun name args))))

bufordrat commented 4 months ago

Wowie zowie, I just tried pasting that exact code into my init file and it worked like a charm outta the box. Ok, this solution works for me; thanks! Will get going on a PR to utop.

ocaml / tuareg