[PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Ken Mankoff]

* ob-screen.el: (org-babel-execute:screen): Parse header params
for `:var', then inject into screen session.
(org-babel-variable-assignments:screen): Copied from
ob-shell.el org-babel-variable-assignments:shell

* etc/ORG-NEWS: Document as New Feature

This change only supports single-value variables
(e.g., ~:var x=42~), not tables or arrays as is
currently supported by some other languages.

---
 etc/ORG-NEWS      | 13 +++++++++++++
 lisp/ob-screen.el | 21 +++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 87ecd77cd..d4b454d13 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -106,6 +106,19 @@ selection.
 TODO state, priority, tags, statistics cookies, and COMMENT keywords
 are allowed in the tree structure.
 
+*** ob-screen now supports :var header arguments
+
+The ~:var~ header arg now supports simple single-value variables
+(arrays and tables are not supported).
+
+#+BEGIN_src org
+,#+BEGIN_SRC screen :var x=42
+,echo $x
+,#+END_SRC
+#+END_src
+
+
+
 * Version 9.6
 
 ** Important announcements and breaking changes
diff --git a/lisp/ob-screen.el b/lisp/ob-screen.el
index 269538e79..d8f361e50 100644
--- a/lisp/ob-screen.el
+++ b/lisp/ob-screen.el
@@ -55,11 +55,32 @@ In case you want to use a different screen than one selected by your $PATH")
   (message "Sending source code block to interactive terminal session...")
   (save-window-excursion
     (let* ((session (cdr (assq :session params)))
+           (var-lines (org-babel-variable-assignments:screen params))
            (socket (org-babel-screen-session-socketname session)))
       (unless socket (org-babel-prep-session:screen session params))
+      (mapcar (lambda (var)
+                (org-babel-screen-session-execute-string session var))
+              var-lines)
       (org-babel-screen-session-execute-string
        session (org-babel-expand-body:generic body params)))))
 
+(defun org-babel-variable-assignments:screen (params)
+  "Return list of shell statements assigning the block's variables."
+  ;; From ob-shell but change function name from ":shell" to ":screen"
+  (let ((sep (cdr (assq :separator params)))
+       (hline (when (string= "yes" (cdr (assq :hlines params)))
+                (or (cdr (assq :hline-string params))
+                    "hline"))))
+    (mapcar
+     (lambda (pair)
+       (if (string-suffix-p "bash" shell-file-name)
+          (org-babel--variable-assignments:bash
+            (car pair) (cdr pair) sep hline)
+         (org-babel--variable-assignments:sh-generic
+         (car pair) (cdr pair) sep hline)))
+     (org-babel--get-vars params))))
+
+
 (defun org-babel-prep-session:screen (_session params)
   "Prepare SESSION according to the header arguments specified in PARAMS."
   (let* ((session (cdr (assq :session params)))

Re: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Ken Mankoff]

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


I realize the patch should be an attachment, not the body of the email. Sorry. R-sending as attachment.

  -k.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-ob-screen.el-Support-var-header-args-for-babel-.patch --]
[-- Type: text/x-diff, Size: 3024 bytes --]

From 5a4707cc16fb5f10cd394762f41d50d8830db240 Mon Sep 17 00:00:00 2001
From: "Kenneth D. Mankoff" <mankoff@gmail.com>
Date: Mon, 20 Feb 2023 21:40:39 -0800
Subject: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel
 blocks

* ob-screen.el: (org-babel-execute:screen): Parse header params
for `:var', then inject into screen session.
(org-babel-variable-assignments:screen): Copied from
ob-shell.el org-babel-variable-assignments:shell

* etc/ORG-NEWS: Document as New Feature

This change only supports single-value variables
(e.g., ~:var x=42~), not tables or arrays as is
currently supported by some other languages.
---
 etc/ORG-NEWS      | 13 +++++++++++++
 lisp/ob-screen.el | 21 +++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 87ecd77cd..d4b454d13 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -106,6 +106,19 @@ selection.
 TODO state, priority, tags, statistics cookies, and COMMENT keywords
 are allowed in the tree structure.
 
+*** ob-screen now supports :var header arguments
+
+The ~:var~ header arg now supports simple single-value variables
+(arrays and tables are not supported).
+
+#+BEGIN_src org
+,#+BEGIN_SRC screen :var x=42
+,echo $x
+,#+END_SRC
+#+END_src
+
+
+
 * Version 9.6
 
 ** Important announcements and breaking changes
diff --git a/lisp/ob-screen.el b/lisp/ob-screen.el
index 269538e79..d8f361e50 100644
--- a/lisp/ob-screen.el
+++ b/lisp/ob-screen.el
@@ -55,11 +55,32 @@ In case you want to use a different screen than one selected by your $PATH")
   (message "Sending source code block to interactive terminal session...")
   (save-window-excursion
     (let* ((session (cdr (assq :session params)))
+           (var-lines (org-babel-variable-assignments:screen params))
            (socket (org-babel-screen-session-socketname session)))
       (unless socket (org-babel-prep-session:screen session params))
+      (mapcar (lambda (var)
+                (org-babel-screen-session-execute-string session var))
+              var-lines)
       (org-babel-screen-session-execute-string
        session (org-babel-expand-body:generic body params)))))
 
+(defun org-babel-variable-assignments:screen (params)
+  "Return list of shell statements assigning the block's variables."
+  ;; From ob-shell but change function name from ":shell" to ":screen"
+  (let ((sep (cdr (assq :separator params)))
+	(hline (when (string= "yes" (cdr (assq :hlines params)))
+		 (or (cdr (assq :hline-string params))
+		     "hline"))))
+    (mapcar
+     (lambda (pair)
+       (if (string-suffix-p "bash" shell-file-name)
+	   (org-babel--variable-assignments:bash
+            (car pair) (cdr pair) sep hline)
+         (org-babel--variable-assignments:sh-generic
+	  (car pair) (cdr pair) sep hline)))
+     (org-babel--get-vars params))))
+
+
 (defun org-babel-prep-session:screen (_session params)
   "Prepare SESSION according to the header arguments specified in PARAMS."
   (let* ((session (cdr (assq :session params)))
-- 
2.34.1

Re: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Max Nikulin]

On 25/02/2023 01:33, Ken Mankoff wrote:
> Subject: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel
>  blocks

Thank you for the patch. I suppose, suggestion to send patches as 
attachments is added mostly to prevent patches inside HTML message body. 
If a message could be feed to "git am" then it should be OK.

I am unsure if Org markup is suitable for commit messages (~:var~) since 
it may appear in Emacs commit logs.

> +++ b/lisp/ob-screen.el
...
> +(defun org-babel-variable-assignments:screen (params)
> +  "Return list of shell statements assigning the block's variables."
> +  ;; From ob-shell but change function name from ":shell" to ":screen"
...
> +       (if (string-suffix-p "bash" shell-file-name)
> +	   (org-babel--variable-assignments:bash

The code still depends on on ob-shell. Is there a reason why calling 
`org-babel-variable-assignments:shell' is a worse variant than copy of 
the whole function body?

Re: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Ken Mankoff]

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

Hi Max,

On 2023-02-24 at 19:51 -08, Max Nikulin <manikulin@gmail.com> wrote:
> On 25/02/2023 01:33, Ken Mankoff wrote:
> I am unsure if Org markup is suitable for commit messages (~:var~)
> since it may appear in Emacs commit logs.

Fixed.

> The code still depends on on ob-shell. Is there a reason why calling
> `org-babel-variable-assignments:shell' is a worse variant than copy of
> the whole function body?

I didn't realize that was a dependency. I now use that function. This also means that arrays/tables/etc. are all supported by :var, although not in the default screen ":cmd sh" because sh doesn't support "declare", but it works with ":cmd bash".

Improved patch attached.

Thanks,

  -k.




[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-ob-screen.el-Support-var-header-args-for-babel-.patch --]
[-- Type: text/x-diff, Size: 1834 bytes --]

From 580499925644cdb9c2dd8c783ec03c095bec7a86 Mon Sep 17 00:00:00 2001
From: "Kenneth D. Mankoff" <mankoff@gmail.com>
Date: Mon, 20 Feb 2023 21:40:39 -0800
Subject: [PATCH] lisp/ob-screen.el: Support var header args for babel blocks

* ob-screen.el: (org-babel-execute:screen): Parse header params
for `:var', then inject into screen session.

* etc/ORG-NEWS: Document as New Feature
---
 etc/ORG-NEWS      | 12 ++++++++++++
 lisp/ob-screen.el |  4 ++++
 2 files changed, 16 insertions(+)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 87ecd77cd..0da686354 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -106,6 +106,18 @@ selection.
 TODO state, priority, tags, statistics cookies, and COMMENT keywords
 are allowed in the tree structure.
 
+*** ob-screen now supports :var header arguments
+
+The ~:var~ header arg is now supported.
+
+#+BEGIN_src org
+,#+BEGIN_SRC screen :var x=42
+,echo $x
+,#+END_SRC
+#+END_src
+
+
+
 * Version 9.6
 
 ** Important announcements and breaking changes
diff --git a/lisp/ob-screen.el b/lisp/ob-screen.el
index 269538e79..bd8650f6b 100644
--- a/lisp/ob-screen.el
+++ b/lisp/ob-screen.el
@@ -55,8 +55,12 @@ In case you want to use a different screen than one selected by your $PATH")
   (message "Sending source code block to interactive terminal session...")
   (save-window-excursion
     (let* ((session (cdr (assq :session params)))
+           (var-lines (org-babel-variable-assignments:shell params))
            (socket (org-babel-screen-session-socketname session)))
       (unless socket (org-babel-prep-session:screen session params))
+      (mapcar (lambda (var)
+                (org-babel-screen-session-execute-string session var))
+              var-lines)
       (org-babel-screen-session-execute-string
        session (org-babel-expand-body:generic body params)))))
 
-- 
2.34.1

Re: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Ken Mankoff]

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

I note that `org-babel-screen-test` fails, but it appears to fail before
this change too...

  -k.

On Sat, Feb 25, 2023 at 7:14 AM Ken Mankoff <mankoff@gmail.com> wrote:

> Hi Max,
>
> On 2023-02-24 at 19:51 -08, Max Nikulin <manikulin@gmail.com> wrote:
> > On 25/02/2023 01:33, Ken Mankoff wrote:
> > I am unsure if Org markup is suitable for commit messages (~:var~)
> > since it may appear in Emacs commit logs.
>
> Fixed.
>
> > The code still depends on on ob-shell. Is there a reason why calling
> > `org-babel-variable-assignments:shell' is a worse variant than copy of
> > the whole function body?
>
> I didn't realize that was a dependency. I now use that function. This also
> means that arrays/tables/etc. are all supported by :var, although not in
> the default screen ":cmd sh" because sh doesn't support "declare", but it
> works with ":cmd bash".
>
> Improved patch attached.
>
> Thanks,
>
>   -k.
>
>
>
>

[-- Attachment #2: Type: text/html, Size: 1433 bytes --]

Re: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Max Nikulin]

On 25/02/2023 22:14, Ken Mankoff wrote:
> On 2023-02-24 at 19:51 -08, Max Nikulin wrote:
>> The code still depends on on ob-shell. Is there a reason why calling
>> `org-babel-variable-assignments:shell' is a worse variant than copy of
>> the whole function body?
> 
> I didn't realize that was a dependency. I now use that function. This
> also means that arrays/tables/etc. are all supported by :var, although
> not in the default screen ":cmd sh" because sh doesn't support
> "declare", but it works with ":cmd bash".

> +           (var-lines (org-babel-variable-assignments:shell params))
>             (socket (org-babel-screen-session-socketname session)))

I believe, it is safer to define `org-babel-variable-assignments:screen' 
some way: alias, substitution, function that calls 
`org-babel-variable-assignments:shell'. I am unsure which variant is 
better. I see a couple of callers for specific language in ob-core.el.

I can not suggest anything specific concerning sh vs. bash. I have never 
tried to pass tables to sh src blocks.

Re: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Ken Mankoff]

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

Hi Max,

On 2023-02-25 at 08:05 -08, Max Nikulin <manikulin@gmail.com> wrote:
> I believe, it is safer to define
> `org-babel-variable-assignments:screen' some way: alias, substitution,
> function that calls `org-babel-variable-assignments:shell'. I am
> unsure which variant is better. I see a couple of callers for specific
> language in ob-core.el.

How's this with a defalias?

  -k.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-ob-screen.el-Support-var-header-args-for-babel-.patch --]
[-- Type: text/x-diff, Size: 2183 bytes --]

From f7ca0258d72b3da5743b1134718b1d1f0d74491d Mon Sep 17 00:00:00 2001
From: "Kenneth D. Mankoff" <mankoff@gmail.com>
Date: Mon, 20 Feb 2023 21:40:39 -0800
Subject: [PATCH] lisp/ob-screen.el: Support var header args for babel blocks

* ob-screen.el: (org-babel-execute:screen): Parse header params
for `:var', then inject into screen session.

* etc/ORG-NEWS: Document as New Feature
---
 etc/ORG-NEWS      | 12 ++++++++++++
 lisp/ob-screen.el |  8 ++++++++
 2 files changed, 20 insertions(+)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 87ecd77cd..0da686354 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -106,6 +106,18 @@ selection.
 TODO state, priority, tags, statistics cookies, and COMMENT keywords
 are allowed in the tree structure.
 
+*** ob-screen now supports :var header arguments
+
+The ~:var~ header arg is now supported.
+
+#+BEGIN_src org
+,#+BEGIN_SRC screen :var x=42
+,echo $x
+,#+END_SRC
+#+END_src
+
+
+
 * Version 9.6
 
 ** Important announcements and breaking changes
diff --git a/lisp/ob-screen.el b/lisp/ob-screen.el
index 269538e79..6e6a31ea6 100644
--- a/lisp/ob-screen.el
+++ b/lisp/ob-screen.el
@@ -40,6 +40,10 @@
 
 (require 'ob)
 
+;; Reuse the variable assignment code from ob-shell
+(defalias 'org-babel-variable-assignments:screen
+  'org-babel-variable-assignments:shell)
+
 (defvar org-babel-screen-location "screen"
   "The command location for screen.
 In case you want to use a different screen than one selected by your $PATH")
@@ -55,8 +59,12 @@ In case you want to use a different screen than one selected by your $PATH")
   (message "Sending source code block to interactive terminal session...")
   (save-window-excursion
     (let* ((session (cdr (assq :session params)))
+           (var-lines (org-babel-variable-assignments:screen params))
            (socket (org-babel-screen-session-socketname session)))
       (unless socket (org-babel-prep-session:screen session params))
+      (mapcar (lambda (var)
+                (org-babel-screen-session-execute-string session var))
+              var-lines)
       (org-babel-screen-session-execute-string
        session (org-babel-expand-body:generic body params)))))
 
-- 
2.34.1

Re: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Max Nikulin]

On 25/02/2023 23:47, Ken Mankoff wrote:
> On 2023-02-25 at 08:05 -08, Max Nikulin wrote:
>> I believe, it is safer to define
>> `org-babel-variable-assignments:screen' some way: alias, substitution,
>> function that calls `org-babel-variable-assignments:shell'. I am
>> unsure which variant is better. I see a couple of callers for specific
>> language in ob-core.el.
> 
> How's this with a defalias?

I do not mind since ob-shell.el uses it for particular shell names.

Concerning tables as variable values, it seems sh receives them as TAB 
separated text.

As to `org-babel-screen-test' perhaps the issue is additional newline 
added after random number. I have not tried stepping through the 
function in debugger though.

> diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
...
> +#+END_src
> +
> +
> +
>  * Version 9.6

It seems top level headings were separated by single empty line.

> diff --git a/lisp/ob-screen.el b/lisp/ob-screen.el
...
;; Reuse the variable assignment code from ob-shell
> +(defalias 'org-babel-variable-assignments:screen
> +  'org-babel-variable-assignments:shell)

Have you tried to compile using make? I expect a warning due to missed 
(require 'ob-shell).

I have no objections besides this couple of minor issues. I am not a 
maintainer though.

P.S. My expectations based on the package name were that ob-screen is 
intended for running commands on remote hosts. Reading the code I 
noticed a couple of issues that may be fixed when you will decide to 
touch this file next time:
- `org-babel-screen-session-socketname' does not respect 
`org-babel-screen-location'
- `string-match' is used where it may be replaced by `string-match-p' 
since match data is not used.

I noticed that ob-core besides org-babel-default-header-args:LANG uses 
org-babel-header-args:LANG. The latter is not defined in ob-screen, 
however I am unsure concerning its effect (completion?).

Re: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Ken Mankoff]

Hi Max,

On 2023-02-26 at 02:11 -08, Max Nikulin <manikulin@gmail.com> wrote:
> As to `org-babel-screen-test' perhaps the issue is additional newline
> added after random number. I have not tried stepping through the
> function in debugger though.

I haven't either. I'll see if I can figure out the issue with the help of the debugger.

> It seems top level headings were separated by single empty line.

Fixed.

> ;; Reuse the variable assignment code from ob-shell
>> +(defalias 'org-babel-variable-assignments:screen
>> +  'org-babel-variable-assignments:shell)
>
> Have you tried to compile using make? I expect a warning due to missed
> (require 'ob-shell).

Make did not complain about this, but did suggest mapcar -> mapc.

> P.S. My expectations based on the package name were that ob-screen is
> intended for running commands on remote hosts.

That too. But I also find it useful for running long commands on localhost without worrying about :async in emacs.

Another nice thing here is that this is, by default, session-based, and can be accessed outside of Org, while Bash blocks don't always play nice with sessions. I can, for example and all in Org Babel, spawn a screen session, ssh to a remote host, and then inject :var into the remote session. I find this quite powerful, and I haven't figured out how to do it with =BEGIN_SRC bash= or =BEGIN_SRC shell= blocks.

> Reading the code I noticed a couple of issues that may be fixed when
> you will decide to touch this file next time: -
> `org-babel-screen-session-socketname' does not respect
> `org-babel-screen-location' - `string-match' is used where it may be
> replaced by `string-match-p' since match data is not used.

Both fixed. Should these be separate patches (3 total), or two - one for the new feature, and one 'cleanup'?

> I noticed that ob-core besides org-babel-default-header-args:LANG uses
> org-babel-header-args:LANG. The latter is not defined in ob-screen,
> however I am unsure concerning its effect (completion?).

I don't understand this either.

  -k.

Re: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Ihor Radchenko]

Ken Mankoff <mankoff@gmail.com> writes:

>> I noticed that ob-core besides org-babel-default-header-args:LANG uses
>> org-babel-header-args:LANG. The latter is not defined in ob-screen,
>> however I am unsure concerning its effect (completion?).
>
> I don't understand this either.

It defines the expected values of header arguments.
Used in completion and by org-lint.

It is a good idea to define it, but mostly for completeness.
If the header args can be anything anyway, it will not affect anything
other than the code style.

-- 
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: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Ken Mankoff]

Hi Ihor and Max,

Just a follow-up note that I am unlikely to be able to complete this patch anytime soon. Re-alignment of priorities because my need for :var header support in Org Babel is mitigated by a different method of injecting variables into Org Babel sections: Use noweb.

I find this more powerful than =:var=. The examples below show (1) setting a bash environment variable in screen, or (2) printing from a Python prompt after sshing to a remote computer. It is language agnostic. Because it uses PROPERTIES and not :var, it also lets me work in Org Column View mode.

* Header
:PROPERTIES:
:foo: 42
:END:

#+NAME: ex1-screen-bash
#+BEGIN_SRC screen
export foo="<<get_property("foo")>>"
#+END_SRC

#+NAME: ex2-ssh-python
#+BEGIN_SRC bash
ssh somewhere
python
print("<<get_property("foo")>>")
#+END_SRC

#+CALL: ex2-ssh-python()

#+RESULTS:
: foo


The relevant section from my library-of-babel is:

* Properties into header args
:PROPERTIES:
:hellomessage: hello
:END:

https://emacs.stackexchange.com/questions/41922/

#+NAME: get_property
#+BEGIN_SRC emacs-lisp :var prop_name="" :results silent
(org-with-point-at org-babel-current-src-block-location
  (org-entry-get nil prop_name t))
#+END_SRC

** Example Usage

*** Header arg

#+HEADER: :var prop_message=(org-entry-get nil "hellomessage" t)
#+BEGIN_SRC emacs-lisp
  (message prop_message)
#+END_SRC

#+RESULTS:
: hello

*** Noweb
#+BEGIN_SRC emacs-lisp :noweb yes
  (message "<<get_property("hellomessage")>>")
#+END_SRC

#+RESULTS:
: hello

#+BEGIN_SRC bash :noweb yes :results verbatim
echo "<<get_property("hellomessage")>>"
#+END_SRC

#+RESULTS:
: hello

If hope this helps someone if they need it.

  -k.

Re: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Ihor Radchenko]

Ken Mankoff <mankoff@gmail.com> writes:

> Just a follow-up note that I am unlikely to be able to complete this patch anytime soon. Re-alignment of priorities because my need for :var header support in Org Babel is mitigated by a different method of injecting variables into Org Babel sections: Use noweb.

Ok.
Canceled.


-- 
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>

[PATCH v4] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Max Nikulin]

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

On 16/03/2023 17:22, Ihor Radchenko wrote:
> Canceled.

Anyway it does not work.

I have tried to "fix" `org-babel-variable-assignments:screen' by an 
extra patch. Perhaps executable name aka :cmd and babel language should 
not be rigidly coupled and it may be subject for further improvement.

Ken, you wrote that you have other patches for ob-screen.el. Do you plan 
to sent them?

[-- Attachment #2: v4-0001-ob-screen.el-Support-var-for-screen-source-blocks.patch --]
[-- Type: text/x-patch, Size: 2190 bytes --]

From a654876c33a13dafac865fa9ea88176302d810c2 Mon Sep 17 00:00:00 2001
From: "Kenneth D. Mankoff" <mankoff@gmail.com>
Date: Mon, 20 Feb 2023 21:40:39 -0800
Subject: [PATCH v4 1/2] ob-screen.el: Support var for screen source blocks

* lisp/ob-screen.el (org-babel-execute:screen): Parse header params
for `:var', then inject into screen session.
* etc/ORG-NEWS: Document as New Feature
---
 etc/ORG-NEWS      | 10 ++++++++++
 lisp/ob-screen.el |  8 ++++++++
 2 files changed, 18 insertions(+)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 4ca13af17..3f70dc99a 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -131,6 +131,16 @@ selection.
 TODO state, priority, tags, statistics cookies, and COMMENT keywords
 are allowed in the tree structure.
 
+*** ob-screen now supports :var header arguments
+
+The ~:var~ header arg is now supported.
+
+#+BEGIN_SRC org
+,#+BEGIN_SRC screen :var x=42
+,echo $x
+,#+END_SRC
+#+END_SRC
+
 ** Miscellaneous
 *** Remove undocumented ~:target~ header parameter in ~ob-clojure~
 
diff --git a/lisp/ob-screen.el b/lisp/ob-screen.el
index 269538e79..6e6a31ea6 100644
--- a/lisp/ob-screen.el
+++ b/lisp/ob-screen.el
@@ -40,6 +40,10 @@ (org-assert-version)
 
 (require 'ob)
 
+;; Reuse the variable assignment code from ob-shell
+(defalias 'org-babel-variable-assignments:screen
+  'org-babel-variable-assignments:shell)
+
 (defvar org-babel-screen-location "screen"
   "The command location for screen.
 In case you want to use a different screen than one selected by your $PATH")
@@ -55,8 +59,12 @@ (defun org-babel-execute:screen (body params)
   (message "Sending source code block to interactive terminal session...")
   (save-window-excursion
     (let* ((session (cdr (assq :session params)))
+           (var-lines (org-babel-variable-assignments:screen params))
            (socket (org-babel-screen-session-socketname session)))
       (unless socket (org-babel-prep-session:screen session params))
+      (mapcar (lambda (var)
+                (org-babel-screen-session-execute-string session var))
+              var-lines)
       (org-babel-screen-session-execute-string
        session (org-babel-expand-body:generic body params)))))
 
-- 
2.25.1


[-- Attachment #3: v4-0002-ob-screen.el-Do-not-rely-on-org-babel-variable-as.patch --]
[-- Type: text/x-patch, Size: 2128 bytes --]

From 1781c8986346a7ea894269f827cce80bfd5cfa70 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Thu, 16 Mar 2023 21:50:24 +0700
Subject: [PATCH v4 2/2] ob-screen.el: Do not rely on
 `org-babel-variable-assignments:shell'

* lisp/ob-screen.el (org-babel-variable-assignments:screen): Use `defun'
instead of `defalias' to call variable assignment function specific
to language obtained from the `:cmd' header argument.

`org-babel-variable-assignments:shell' might be considered as
implementation detail, see

Ihor Radchenko to emacs-orgmode. Re: [PATCH] lisp/ob-screen.el: Support
~:var~ header args for babel blocks. Sun, 26 Feb 2023 12:18:14 +0000.
<https://list.orgmode.org/87lekkzkl5.fsf@localhost>

The implemented approach is not optimal since it does not allow
to decouple interpreter executable (for example python3) and babel
language (for example python).
---
 lisp/ob-screen.el | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/lisp/ob-screen.el b/lisp/ob-screen.el
index 6e6a31ea6..5d09fe585 100644
--- a/lisp/ob-screen.el
+++ b/lisp/ob-screen.el
@@ -39,10 +39,23 @@ (require 'org-macs)
 (org-assert-version)
 
 (require 'ob)
-
-;; Reuse the variable assignment code from ob-shell
-(defalias 'org-babel-variable-assignments:screen
-  'org-babel-variable-assignments:shell)
+(require 'ob-shell)
+
+(defun org-babel-variable-assignments:screen (params)
+  "Return list of statements to assign block variables.
+
+This is an experimental feature, `:cmd' property from PARAMS
+treated as language while actually it is command (executable) name.
+It works at least for shell interpreters."
+  (and
+   (alist-get :var params)
+   (let* ((cmd (alist-get :cmd params))
+          (lang (and cmd (file-name-base cmd)))
+          (func (and lang (intern
+                           (concat "org-babel-variable-assignments:" lang)))))
+     (if (not (fboundp func))
+         (error "Babel language support for %s is not loaded" lang)
+       (funcall func params)))))
 
 (defvar org-babel-screen-location "screen"
   "The command location for screen.
-- 
2.25.1

Re: [PATCH v4] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Max Nikulin]

On 16/03/2023 22:09, Max Nikulin wrote:
> +(defun org-babel-variable-assignments:screen (params)
...>  (defvar org-babel-screen-location "screen"

Perhaps `defun' should be moved below `defvar's.

Re: [PATCH v4] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> Ken, you wrote that you have other patches for ob-screen.el. Do you plan 
> to sent them?

For context, Ken is the maintainer of ob-screen.
It is up to him is this feature is useful for ob-screen, with your
additions.

-- 
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: [PATCH v4] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Ken Mankoff]

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

Hi Max,

On 2023-03-16 at 11:09 -04, Max Nikulin <manikulin@gmail.com> wrote...
> Ken, you wrote that you have other patches for ob-screen.el. Do you
> plan to sent them?

Here is the other patch. Respect custom screen command.

  -k.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-ob-screen.el-respect-custom-screen-location.patch --]
[-- Type: text/x-diff, Size: 877 bytes --]

From ff919de09e020fa053f5727f3b36026a3a773f88 Mon Sep 17 00:00:00 2001
From: "Kenneth D. Mankoff" <mankoff@gmail.com>
Date: Sun, 26 Feb 2023 17:38:54 -0800
Subject: [PATCH 2/2] ob-screen.el: respect custom screen location

---
 lisp/ob-screen.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/ob-screen.el b/lisp/ob-screen.el
index 6356d84dd..2013cf12d 100644
--- a/lisp/ob-screen.el
+++ b/lisp/ob-screen.el
@@ -98,7 +98,7 @@ In case you want to use a different screen than one selected by your $PATH")
 
 (defun org-babel-screen-session-socketname (session)
   "Check if SESSION exists by parsing output of \"screen -ls\"."
-  (let* ((screen-ls (shell-command-to-string "screen -ls"))
+  (let* ((screen-ls (shell-command-to-string (concat org-babel-screen-location " -ls")))
          (sockets (delq
 		   nil
                    (mapcar
-- 
2.34.1

Re: [PATCH v4] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Max Nikulin]

On 19/03/2023 21:42, Ken Mankoff wrote:
> Here is the other patch. Respect custom screen command.

Thanks, Ken.

> +++ b/lisp/ob-screen.el
> @@ -98,7 +98,7 @@ In case you want to use a different screen than one selected by your $PATH")
>   
>   (defun org-babel-screen-session-socketname (session)
>     "Check if SESSION exists by parsing output of \"screen -ls\"."
> -  (let* ((screen-ls (shell-command-to-string "screen -ls"))
> +  (let* ((screen-ls (shell-command-to-string (concat org-babel-screen-location " -ls")))

Currently `org-babel-screen-location' is used as an argument of 
`start-process', so if the value contains spaces or other shell special 
characters they are not interpreted by shell. Since there is no 
convenience function like process-to-string that accepts command as list 
instead of string with a shell command, it is better to pass screen 
location through `shell-quote-argument'.

You may try to create a directory with a space in its name, create a 
symlink from this directory to /usr/bin/srceen, and set 
`org-babel-screen-location' to the full path (with space).

Re: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Max Nikulin]

On 27/02/2023 08:59, Ken Mankoff wrote:
>> Have you tried to compile using make? I expect a warning due to missed
>> (require 'ob-shell).
> 
> Make did not complain about this, but did suggest mapcar -> mapc.

I did not expect it. However I still think, it is better to add such 
require. It should not be an error if a user is going to use ob-screen 
without explicitly loading ob-shell.

> Both fixed. Should these be separate patches (3 total), or two - one for
> the new feature, and one 'cleanup'?

I do not think it really matters if 2 minor fixes will be committed 
separately or as a single patch.

>> I noticed that ob-core besides org-babel-default-header-args:LANG uses
>> org-babel-header-args:LANG. The latter is not defined in ob-screen,
>> however I am unsure concerning its effect (completion?).
> 
> I don't understand this either.

Anyway it is unrelated to the patch we are currently discussing. It is 
an option for the future.

As to calling org-babel-variable-assignments: for specific shell, as 
Ihor suggests, it should be something like (untested)

(funcall (intern (concat "org-babel-variable-assignments:"
                  (plist-get params :cmd))
          params)

Re: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Ihor Radchenko]

Ken Mankoff <mankoff@gmail.com> writes:

> From f7ca0258d72b3da5743b1134718b1d1f0d74491d Mon Sep 17 00:00:00 2001
> From: "Kenneth D. Mankoff" <mankoff@gmail.com>
> Date: Mon, 20 Feb 2023 21:40:39 -0800
> Subject: [PATCH] lisp/ob-screen.el: Support var header args for babel blocks

Thanks for the patch!

> +;; Reuse the variable assignment code from ob-shell
> +(defalias 'org-babel-variable-assignments:screen
> +  'org-babel-variable-assignments:shell)

This will work, but you are relying on implementation detail of
ob-shell.el. A more safe approach would be calling
org-babel-variable-assignments:LANG depending on the :cmd header arg.
For :cmd bash - org-babel-variable-assignments:bash, for :cmd fish -
org-babel-variable-assignments:fish.

To archive this, you can define a full
org-babel-variable-assignments:screen function that does what I described.

-- 
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: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Ken Mankoff]

Hi Ihor,

On 2023-02-26 at 04:18 -08, Ihor Radchenko <yantar92@posteo.net> wrote:
>> +;; Reuse the variable assignment code from ob-shell
>> +(defalias 'org-babel-variable-assignments:screen
>> +  'org-babel-variable-assignments:shell)
>
> This will work, but you are relying on implementation detail of
> ob-shell.el. A more safe approach would be calling
> org-babel-variable-assignments:LANG depending on the :cmd header arg.
> For :cmd bash - org-babel-variable-assignments:bash, for :cmd fish -
> org-babel-variable-assignments:fish.
>
> To archive this, you can define a full
> org-babel-variable-assignments:screen function that does what I
> described.

You're pushing my lisp skills here. Which is fine, but I might need some help.

In ob-shell I see

(defcustom org-babel-shell-names
  '("sh" "bash" "zsh" "fish" "csh" "ash" "dash" "ksh" "mksh" "posh")

Should I create a function that, using case statement or something similar, checks for each of these and calls that flavor? I think I could do that.


But if I also see org-babel-shell-initialize in ob-shell that looks like it creates defaliases for each of these to (org-babel-execute:shell) and #'org-babel-variable-assignments:shell.

I'm not sure how that is very different from what I've done. I'm not sure what to do here, nor how to do what you suggest (yet - I've only read it so far, not spent a lot of time experimenting and searching, so I may be able to implement what I think is a solution to what you wrote, but I doubt it'll be what you expect).

  -k.

Re: [PATCH] lisp/ob-screen.el: Support ~:var~ header args for babel blocks

[Ihor Radchenko]

Ken Mankoff <mankoff@gmail.com> writes:

>> To archive this, you can define a full
>> org-babel-variable-assignments:screen function that does what I
>> described.
>
> You're pushing my lisp skills here. Which is fine, but I might need some help.
>
> In ob-shell I see
>
> (defcustom org-babel-shell-names
>   '("sh" "bash" "zsh" "fish" "csh" "ash" "dash" "ksh" "mksh" "posh")
>
> Should I create a function that, using case statement or something similar, checks for each of these and calls that flavor? I think I could do that.

Not each of these. AFAIU, :cmd header property in ob-screen defines
which shell to use. Knowing shell name, you can deduce the function name
to be used for variable assignments. See how it is done in
`org-babel-expand-src-block'.

> But if I also see org-babel-shell-initialize in ob-shell that looks like it creates defaliases for each of these to (org-babel-execute:shell) and #'org-babel-variable-assignments:shell.

The convention is for every possible #+begin_src lang to have
org-babel-variable-assignments:LANG.
ob-shell just tries to avoid unnecessary code duplication and instead of
writing identical org-babel-variable-assignments:sh
org-babel-variable-assignments:bash, org-babel-variables-assignents:zsh,
..., it generates them using a macro.

> I'm not sure how that is very different from what I've done. I'm not sure what to do here, nor how to do what you suggest (yet - I've only read it so far, not spent a lot of time experimenting and searching, so I may be able to implement what I think is a solution to what you wrote, but I doubt it'll be what you expect).

AFAIU, you don't need to juggle with macros as ob-shell does. Something
similar to `org-babel-expand-src-block' will do.

-- 
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>