[BUG] ob-R tests fail on Emacs 27 since ESS commit 536b5b914a7 [9.7-pre (release_9.6.4-318-gcfe5e0.dirty @ /home/yantar92/.emacs.d/straight/build/org/)]

[BUG] ob-R tests fail on Emacs 27 since ESS commit 536b5b914a7 [9.7-pre (release_9.6.4-318-gcfe5e0.dirty @ /home/yantar92/.emacs.d/straight/build/org/)]

[Ihor Radchenko]

Steps to reproduce:

1. cd /path/to/org/git/repo
2. make test EMACS=emacs-27 BTEST_POST="-L /path/to/ESS/lisp -l ob-R -l ess -l ess-r-mode" BTEST_RE="async"

3 unexpected results:
   FAILED  ob-session-async-R-named-output
   FAILED  ob-session-async-R-output-drawer
   FAILED  ob-session-async-R-simple-session-async-output

The tests started failing since 2023/04/04.
https://builds.sr.ht/~bzg/job/968099

I bisected the failure down to the following ESS commit:

536b5b914a75afa37ee05aa8ba0aecd0f9c9eb33
Author:     Lionel Henry <lionel.hry@gmail.com>
AuthorDate: Mon Apr 3 13:01:11 2023 +0200
Commit:     Lionel Henry <lionel.hry@gmail.com>
CommitDate: Mon Apr 3 13:01:11 2023 +0200

Parent:     fef059c3 Wait for process after `ess-r-post-run-hook`
Contained:  master
Follows:    ESSRv1.8 (21)

Move `ess-r` startup commands out of `ess-r-post-run-hook`

They are blocking commands and the user might have added streaming
commands to the hook.

Also document why users should be careful when mixing blocking and
streaming commands.

------

I am not sure what is going on as ob-R does not really use the changed
hooks. The suspects are

(ess-execute-screen-options t)
(ess-set-working-directory default-directory)))

In particular, I think that `ess-execute-screen-options' is doing
something funny because one of the tests fails with

"1:5" outputting

 1 2 3
 4 5

split into two lines, which looks like some text filling problem.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] ob-R tests fail on Emacs 27 since ESS commit 536b5b914a7 [9.7-pre (release_9.6.4-318-gcfe5e0.dirty @ /home/yantar92/.emacs.d/straight/build/org/)]

[Lionel Henry]

Hi Ihor,

Thanks for looking into this. I think you're right it's probably
caused by ESS now setting the width on startup. In that case you could
fix your tests by setting `options(width = )` to some hardcoded value.
You could do it via `ess-r-post-run-hook`.

Best,
Lionel


On 4/17/23, Ihor Radchenko <yantar92@posteo.net> wrote:
>
> Steps to reproduce:
>
> 1. cd /path/to/org/git/repo
> 2. make test EMACS=emacs-27 BTEST_POST="-L /path/to/ESS/lisp -l ob-R -l ess
> -l ess-r-mode" BTEST_RE="async"
>
> 3 unexpected results:
>    FAILED  ob-session-async-R-named-output
>    FAILED  ob-session-async-R-output-drawer
>    FAILED  ob-session-async-R-simple-session-async-output
>
> The tests started failing since 2023/04/04.
> https://builds.sr.ht/~bzg/job/968099
>
> I bisected the failure down to the following ESS commit:
>
> 536b5b914a75afa37ee05aa8ba0aecd0f9c9eb33
> Author:     Lionel Henry <lionel.hry@gmail.com>
> AuthorDate: Mon Apr 3 13:01:11 2023 +0200
> Commit:     Lionel Henry <lionel.hry@gmail.com>
> CommitDate: Mon Apr 3 13:01:11 2023 +0200
>
> Parent:     fef059c3 Wait for process after `ess-r-post-run-hook`
> Contained:  master
> Follows:    ESSRv1.8 (21)
>
> Move `ess-r` startup commands out of `ess-r-post-run-hook`
>
> They are blocking commands and the user might have added streaming
> commands to the hook.
>
> Also document why users should be careful when mixing blocking and
> streaming commands.
>
> ------
>
> I am not sure what is going on as ob-R does not really use the changed
> hooks. The suspects are
>
> (ess-execute-screen-options t)
> (ess-set-working-directory default-directory)))
>
> In particular, I think that `ess-execute-screen-options' is doing
> something funny because one of the tests fails with
>
> "1:5" outputting
>
>  1 2 3
>  4 5
>
> split into two lines, which looks like some text filling problem.
>
> --
> Ihor Radchenko // yantar92,
> Org mode contributor,
> Learn more about Org mode at <https://orgmode.org/>.
> Support Org development at <https://liberapay.com/org-mode>,
> or support my work at <https://liberapay.com/yantar92>
>

Re: [BUG] ob-R tests fail on Emacs 27 since ESS commit 536b5b914a7 [9.7-pre (release_9.6.4-318-gcfe5e0.dirty @ /home/yantar92/.emacs.d/straight/build/org/)]

[Ihor Radchenko]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

Lionel Henry <lionel.hry@gmail.com> writes:

> Thanks for looking into this. I think you're right it's probably
> caused by ESS now setting the width on startup. In that case you could
> fix your tests by setting `options(width = )` to some hardcoded value.
> You could do it via `ess-r-post-run-hook`.

I tried using `ess-r-post-run-hook', but it fails for some reason.
See the attached patch.

With the patch, the tests fail with

Test ob-session-async-R-value-drawer condition:
    (user-error "ESS process not ready. Finish your command before trying again")

In contrast, directly editing `ess-r-initialize' like
  ;; (ess-execute-screen-options t)
  (ess-command (ess-calculate-width 9999))

does not fail and make all the tests pass.

Is there anything I am missing about `ess-r-post-run-hook'?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ob-R-Fix-regression-after-ESS-commit-536b5b914a7.patch --]
[-- Type: text/x-patch, Size: 2621 bytes --]

From 07407519d2e8de0f2925a9d2971b855fd0be86b4 Mon Sep 17 00:00:00 2001
Message-Id: <07407519d2e8de0f2925a9d2971b855fd0be86b4.1681814636.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Tue, 18 Apr 2023 12:42:29 +0200
Subject: [PATCH] ob-R: Fix regression after ESS commit 536b5b914a7

* lisp/ob-R.el (org-babel-R--set-wide-screen-width): New function
setting fill column to large value in ESS interactive shell.
(org-babel-R-initiate-session): Override the default ESS truncation
settings.

Reported-by: Ihor Radchenko <yantar92@posteo.net>
Link: https://orgmode.org/list/87ilduqrem.fsf@localhost
---
 lisp/ob-R.el | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/lisp/ob-R.el b/lisp/ob-R.el
index 7ea137197..9ad878370 100644
--- a/lisp/ob-R.el
+++ b/lisp/ob-R.el
@@ -39,6 +39,10 @@ (declare-function orgtbl-to-tsv "org-table" (table params))
 (declare-function run-ess-r "ext:ess-r-mode" (&optional start-args))
 (declare-function inferior-ess-send-input "ext:ess-inf" ())
 (declare-function ess-make-buffer-current "ext:ess-inf" ())
+(declare-function
+ ess-command "ext:ess-inf"
+ (cmd &optional out-buffer sleep no-prompt-check wait proc proc force-redisplay timeout))
+(declare-function ess-calculate-width "ext:ess-inf" (opt))
 (declare-function ess-eval-buffer "ext:ess-inf" (vis))
 (declare-function ess-wait-for-process "ext:ess-inf"
 		  (&optional proc sec-prompt wait force-redisplay))
@@ -261,7 +265,13 @@ (defun org-babel-R-assign-elisp (name value colnames-p rownames-p)
 	  (t                (format "%s <- %S" name (prin1-to-string value))))))
 
 
+(defun org-babel-R--set-wide-screen-width ()
+  "Set large screen width in current R buffer."
+  (org-require-package 'ess-inf "ESS")
+  (ess-command (ess-calculate-width 9999)))
+
 (defvar ess-ask-for-ess-directory) ; dynamically scoped
+(defvar ess-r-post-run-hook)
 (defun org-babel-R-initiate-session (session params)
   "If there is not a current R process then create one."
   (unless (string= session "none")
@@ -277,7 +287,10 @@ (defun org-babel-R-initiate-session (session params)
 	    ;; Session buffer exists, but with dead process
 	    (set-buffer session))
           (org-require-package 'ess-r-mode "ESS")
-          (set-buffer (run-ess-r))
+          (let ((ess-r-post-run-hook
+                 (cons #'org-babel-R--set-wide-screen-width
+                       ess-r-post-run-hook)))
+            (set-buffer (run-ess-r)))
 	  (let ((R-proc (get-process (or ess-local-process-name
 					 ess-current-process-name))))
 	    (while (process-get R-proc 'callbacks)
-- 
2.40.0


[-- Attachment #3: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] ob-R tests fail on Emacs 27 since ESS commit 536b5b914a7 [9.7-pre (release_9.6.4-318-gcfe5e0.dirty @ /home/yantar92/.emacs.d/straight/build/org/)]

[Lionel Henry]

Can you try with the latest commit please?
https://github.com/emacs-ess/ESS/commit/ed6c12ee3dfb1046fb030200f45e75a65b31e49e

By the way, it looks like you're setting the width in the exported
initialization routine rather than just in tests. For user-facing
features at least you probably want to set the width to something that
will produce readable output.

Best,
Lionel


On 4/18/23, Ihor Radchenko <yantar92@posteo.net> wrote:
> Lionel Henry <lionel.hry@gmail.com> writes:
>
>> Thanks for looking into this. I think you're right it's probably
>> caused by ESS now setting the width on startup. In that case you could
>> fix your tests by setting `options(width = )` to some hardcoded value.
>> You could do it via `ess-r-post-run-hook`.
>
> I tried using `ess-r-post-run-hook', but it fails for some reason.
> See the attached patch.
>
> With the patch, the tests fail with
>
> Test ob-session-async-R-value-drawer condition:
>     (user-error "ESS process not ready. Finish your command before trying
> again")
>
> In contrast, directly editing `ess-r-initialize' like
>   ;; (ess-execute-screen-options t)
>   (ess-command (ess-calculate-width 9999))
>
> does not fail and make all the tests pass.
>
> Is there anything I am missing about `ess-r-post-run-hook'?
>
>

Re: [BUG] ob-R tests fail on Emacs 27 since ESS commit 536b5b914a7 [9.7-pre (release_9.6.4-318-gcfe5e0.dirty @ /home/yantar92/.emacs.d/straight/build/org/)]

[Ihor Radchenko]

Lionel Henry <lionel.hry@gmail.com> writes:

> Can you try with the latest commit please?
> https://github.com/emacs-ess/ESS/commit/ed6c12ee3dfb1046fb030200f45e75a65b31e49e

Works now. Thank you!

> By the way, it looks like you're setting the width in the exported
> initialization routine rather than just in tests. For user-facing
> features at least you probably want to set the width to something that
> will produce readable output.

You are right. For some reason, I thought that the problem also affects
:results value, when we want to convert ESS interactive output into
Elisp data. But it was not the case - only :results output has been
affected.

Since we have no reason to disregard ESS output settings in :results
output, I pushed an alternative fix that only affects tests, as you
suggested.

Do note that the root cause remains on the ESS side - `run-ess-r' in
Emacs 27, when called non-interactively, may set line splitting to
something strange. I am unsure if it is something that should be fixed
on ESS side. I am also unsure if the problem does not happen in other
Emacs versions.

Fixed, on main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=83e446ea8

I also added a test for :results value when the output is long. Just in
case.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=3c449cc43

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] ob-R tests fail on Emacs 27 since ESS commit 536b5b914a7 [9.7-pre (release_9.6.4-318-gcfe5e0.dirty @ /home/yantar92/.emacs.d/straight/build/org/)]

[Lionel Henry]

> Do note that the root cause remains on the ESS side - `run-ess-r' in
> Emacs 27, when called non-interactively, may set line splitting to
> something strange.

I think you're right. Probably we shouldn't be executing screen
options when called non-interactively and instead leave the R default
width (80).

I've opened an issue to track this:
https://github.com/emacs-ess/ESS/issues/1248

Best,
Lionel


On 4/18/23, Ihor Radchenko <yantar92@posteo.net> wrote:
> Lionel Henry <lionel.hry@gmail.com> writes:
>
>> Can you try with the latest commit please?
>> https://github.com/emacs-ess/ESS/commit/ed6c12ee3dfb1046fb030200f45e75a65b31e49e
>
> Works now. Thank you!
>
>> By the way, it looks like you're setting the width in the exported
>> initialization routine rather than just in tests. For user-facing
>> features at least you probably want to set the width to something that
>> will produce readable output.
>
> You are right. For some reason, I thought that the problem also affects
> :results value, when we want to convert ESS interactive output into
> Elisp data. But it was not the case - only :results output has been
> affected.
>
> Since we have no reason to disregard ESS output settings in :results
> output, I pushed an alternative fix that only affects tests, as you
> suggested.
>
> Do note that the root cause remains on the ESS side - `run-ess-r' in
> Emacs 27, when called non-interactively, may set line splitting to
> something strange. I am unsure if it is something that should be fixed
> on ESS side. I am also unsure if the problem does not happen in other
> Emacs versions.
>
> Fixed, on main.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=83e446ea8
>
> I also added a test for :results value when the output is long. Just in
> case.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=3c449cc43
>
> --
> Ihor Radchenko // yantar92,
> Org mode contributor,
> Learn more about Org mode at <https://orgmode.org/>.
> Support Org development at <https://liberapay.com/org-mode>,
> or support my work at <https://liberapay.com/yantar92>
>