emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
@ 2023-01-16 16:27 Osher Jacob
  2023-01-16 21:40 ` Matt
  0 siblings, 1 reply; 19+ messages in thread
From: Osher Jacob @ 2023-01-16 16:27 UTC (permalink / raw)
  To: emacs-orgmode


[-- Attachment #1.1: Type: text/plain, Size: 11129 bytes --]

*Expected behaviour:*On Windows, all lines of the babel shell block should
be evaluated, with full output printed.


*Actual behaviour:*All lines but the last are evaluated, the output ends
with 'More?'
instead of the last command's output.


*Steps to reproduce:*In Windows, start a clean Emacs instance (I used
"runemacs.exe -Q"), and evaluate the following blocks:

1) Enable shell language support
#+begin_src elisp
(org-babel-do-load-languages
   'org-babel-load-languages
   '(
     (shell . t)
     )
   )
#+end_src

#+RESULTS:


2) Evaluate this shell code block
#+begin_src shell
  echo 1
  echo 2


#+end_src

#+RESULTS:
| Microsoft             | Windows   | [Version     | 10.0.19044.2364] |
   |           |
| (c)                   | Microsoft | Corporation. | All              |
rights | reserved. |
|                       |           |              |                  |
   |           |
| c:\Users\osherj>echo  | 1         |              |                  |
   |           |
| 1                     |           |              |                  |
   |           |
|                       |           |              |                  |
   |           |
| c:\Users\osherj>More? |           |              |                  |
   |           |



*Additional info:*After some debugging, it seems to be caused by a missing
newline character at the
end of the code block content.
I believe this is caused by the org-trim function which is called upon the
body of the block.
At the function:
org/ob-shell.el -> (defun org-babel-sh-evaluate ...)
The line:
(org-babel-eval shell-file-name (org-trim body))



*Suggested fix:*The hacky way I solved it was to change this line:
(t (org-babel-eval shell-file-name (org-trim body))))))
to this:
(t (org-babel-eval shell-file-name (concat (org-trim body) "\n"))))))
Also attached as a patch file


Let me know if there's any other information you need, or if I can help in
any other way.

Thanks!
Osher


Emacs  : GNU Emacs 28.2 (build 2, x86_64-w64-mingw32)
 of 2022-09-13
Package: Org mode version 9.6.1 ( @
c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)

current state:
==============
(setq
 org-link-elisp-confirm-function 'yes-or-no-p
 org-format-latex-header
"\\documentclass{article}\n\\usepackage[usenames]{color}\n[PACKAGES]\n[DEFAULT-PACKAGES]\n\\pagestyle{empty}
            % do not remove\n% The settings below are copied from
fullpage.sty\n\\setlength{\\textwidth}{\\paperwidth}\n\\addtolength{\\textwidth}{-3cm}\n\\setlength{\\oddsidemargin}{1.5cm}\n\\addtolength{\\oddsidemargin}{-2.54cm}\n\\setlength{\\evensidemargin}{\\oddsidemargin}\n\\setlength{\\textheight}{\\paperheight}\n\\addtolength{\\textheight}{-\\headheight}\n\\addtolength{\\textheight}{-\\headsep}\n\\addtolength{\\textheight}{-\\footskip}\n\\addtolength{\\textheight}{-3cm}\n\\setlength{\\topmargin}{1.5cm}\n\\addtolength{\\topmargin}{-2.54cm}"
 org-bibtex-headline-format-function '(closure (org-id-locations
org-agenda-search-view-always-boolean org-agenda-overriding-header t)
(entry)
      (cdr (assq :title entry)))
 org-persist-after-read-hook '(org-element--cache-persist-after-read)
 org-time-stamp-custom-formats '("<%m/%d/%y %a>" . "<%m/%d/%y %a %H:%M>")
 org-export-before-parsing-hook '(org-attach-expand-links)
 org-cycle-tab-first-hook '(org-babel-hide-result-toggle-maybe
org-babel-header-arg-expand)
 org-archive-hook '(org-attach-archive-delete-maybe)
 org-odt-format-inlinetask-function
'org-odt-format-inlinetask-default-function
 org-ascii-format-drawer-function '(closure (t) (_name contents _width)
contents)
 org-cycle-hook '(org-cycle-hide-archived-subtrees org-cycle-hide-drawers
org-cycle-show-empty-lines org-optimize-window-after-visibility-change)
 org-persist-before-read-hook '(org-element--cache-persist-before-read)
 org-mode-hook '(#[0 "\300\301\302\303\304$\207" [add-hook
change-major-mode-hook org-fold-show-all append local] 5]
(closure
 (org--rds reftex-docstruct-symbol org--single-lines-list-is-paragraph
visual-fill-column-width org-clock-history
  org-agenda-current-date org-with-time org-defdecode org-def
org-read-date-inactive org-ans2 org-ans1 org-columns-current-fmt-compiled
  org-clock-current-task org-clock-effort org-agenda-skip-function
org-agenda-skip-comment-trees org-agenda-archives-mode
  org-end-time-was-given org-time-was-given org-blocked-by-checkboxes
org-state org-agenda-headline-snapshot-before-repeat
  org-agenda-buffer-name org-agenda-start-on-weekday
org-agenda-buffer-tmp-name buffer-face-mode-face org-struct-menu
org-last-state
  org-clock-start-time texmathp-why remember-data-file
org-agenda-tags-todo-honor-ignore-options calc-embedded-open-mode
  calc-embedded-open-formula calc-embedded-close-formula
align-mode-rules-list org-export-registered-backends crm-separator
  org-id-overriding-file-name org-indent-indentation-per-level
org-element--timestamp-regexp org-element-cache-map-continue-from
  org-element-paragraph-separate org-agenda-buffer-name
org-inlinetask-min-level t)
 nil (add-hook 'change-major-mode-hook 'org-fold-show-all 'append 'local))
(closure (*this* org-babel-confirm-evaluate-answer-no t) nil
 (add-hook 'change-major-mode-hook #'org-babel-show-result-all 'append
'local))
#[0 "\300\301\302\303\304$\207" [add-hook change-major-mode-hook
org-show-all append local] 5]
#[0 "\300\301\302\303\304$\207" [add-hook change-major-mode-hook
org-babel-show-result-all append local] 5] org-babel-result-hide-spec
org-babel-hide-all-hashes)
 org-babel-load-languages '((shell . t))
 org-latex-format-drawer-function '(closure (engrave-faces-latex-mathescape
engrave-faces-current-preset-style engrave-faces-latex-output-style t)
   (_ contents) contents)
 org-latex-format-headline-function
'org-latex-format-headline-default-function
 org-confirm-shell-link-function 'yes-or-no-p
 org-html-format-drawer-function '(closure
  (htmlize-buffer-places org-html-format-table-no-css
htmlize-css-name-prefix htmlize-output-type htmlize-output-type
   htmlize-css-name-prefix t)
  (_name contents) contents)
 outline-isearch-open-invisible-function 'outline-isearch-open-invisible
 org-fold-catch-invisible-edits nil
 org-odt-format-headline-function 'org-odt-format-headline-default-function
 org-agenda-before-write-hook '(org-agenda-add-entry-text)
 org-src-mode-hook '(org-src-babel-configure-edit-buffer
org-src-mode-configure-edit-buffer)
 org-confirm-elisp-link-function 'yes-or-no-p
 org-preview-latex-process-alist '((dvipng :programs ("latex" "dvipng")
:description "dvi > png" :message
   "you need to install the programs: latex and dvipng." :image-input-type
"dvi" :image-output-type "png"
   :image-size-adjust (1.0 . 1.0) :latex-compiler ("latex -interaction
nonstopmode -output-directory %o %f")
   :image-converter ("dvipng -D %D -T tight -bg Transparent -o %O %f"))
  (dvisvgm :programs ("latex" "dvisvgm") :description "dvi > svg" :message
   "you need to install the programs: latex and dvisvgm." :image-input-type
"dvi" :image-output-type "svg"
   :image-size-adjust (1.7 . 1.5) :latex-compiler ("latex -interaction
nonstopmode -output-directory %o %f")
   :image-converter ("dvisvgm %f -n -b min -c %S -o %O"))
  (imagemagick :programs ("latex" "convert") :description "pdf > png"
:message
   "you need to install the programs: latex and imagemagick."
:image-input-type "pdf" :image-output-type "png"
   :image-size-adjust (1.0 . 1.0) :latex-compiler ("pdflatex -interaction
nonstopmode -output-directory %o %f")
   :image-converter ("convert -density %D -trim -antialias %f -quality 100
%O"))
  )
 org-speed-command-hook '(org-speed-command-activate
org-babel-speed-command-activate)
 org-html-format-inlinetask-function
'org-html-format-inlinetask-default-function
 org-ascii-format-inlinetask-function 'org-ascii-format-inlinetask-default
 org-odt-format-drawer-function '(closure
 (hfy-user-sheet-assoc hfy-html-quote-regex hfy-html-quote-map
hfy-face-to-css hfy-begin-span-handler
  hfy-end-span-handler nxml-auto-insert-xml-declaration-flag t)
 (_name contents) contents)
 org-persist-directory
"c:/Users/Osher/AppData/Local/Temp/org-persist-bvcjPn"
 org-babel-exp-code-template "#+BEGIN_SRC
%lang%switches%flags\n%body\n#+END_SRC"
 org-fold-core-isearch-open-function 'org-fold--isearch-reveal
 org-latex-format-inlinetask-function
'org-latex-format-inlinetask-default-function
 org-persist-before-write-hook '(org-element--cache-persist-before-write)
 org-tab-first-hook '(org-babel-hide-result-toggle-maybe
org-babel-header-arg-expand)
 org-link-shell-confirm-function 'yes-or-no-p
 org-babel-pre-tangle-hook '(save-buffer)
 org-agenda-loop-over-headlines-in-active-region nil
 org-occur-hook '(org-first-headline-recenter)
 org-metadown-hook '(org-babel-pop-to-session-maybe)
 org-link-parameters '(("attachment" :follow org-attach-follow :complete
org-attach-complete-link) ("id" :follow org-id-open)
      ("eww" :follow org-eww-open :store org-eww-store-link) ("rmail"
:follow org-rmail-open :store org-rmail-store-link)
      ("mhe" :follow org-mhe-open :store org-mhe-store-link)
      ("irc" :follow org-irc-visit :store org-irc-store-link :export
org-irc-export)
      ("info" :follow org-info-open :export org-info-export :store
org-info-store-link :insert-description
org-info-description-as-command)
      ("gnus" :follow org-gnus-open :store org-gnus-store-link)
      ("docview" :follow org-docview-open :export org-docview-export :store
org-docview-store-link)
      ("bibtex" :follow org-bibtex-open :store org-bibtex-store-link)
      ("bbdb" :follow org-bbdb-open :export org-bbdb-export :complete
org-bbdb-complete-link :store org-bbdb-store-link)
      ("w3m" :store org-w3m-store-link) ("doi" :follow org-link-doi-open
:export org-link-doi-export) ("file+sys") ("file+emacs")
      ("shell" :follow org-link--open-shell)
      ("news" :follow #[514 "\301\300\302 Q \"\207" ["news" browse-url ":"]
6 "\n\n(fn URL ARG)"])
      ("mailto" :follow #[514 "\301\300\302 Q \"\207" ["mailto" browse-url
":"] 6 "\n\n(fn URL ARG)"])
      ("https" :follow #[514 "\301\300\302 Q \"\207" ["https" browse-url
":"] 6 "\n\n(fn URL ARG)"])
      ("http" :follow #[514 "\301\300\302 Q \"\207" ["http" browse-url ":"]
6 "\n\n(fn URL ARG)"])
      ("ftp" :follow #[514 "\301\300\302 Q \"\207" ["ftp" browse-url ":"] 6
"\n\n(fn URL ARG)"])
      ("help" :follow org-link--open-help :store org-link--store-help)
("file" :complete org-link-complete-file)
      ("elisp" :follow org-link--open-elisp))
 org-html-format-headline-function
'org-html-format-headline-default-function
 org-metaup-hook '(org-babel-load-in-session-maybe)
 org-src-lang-modes '(("C" . c) ("C++" . c++) ("asymptote" . asy) ("bash" .
sh) ("beamer" . latex) ("calc" . fundamental) ("cpp" . c++)
     ("ditaa" . artist) ("dot" . fundamental) ("elisp" . emacs-lisp)
("ocaml" . tuareg) ("screen" . shell-script) ("shell" . sh)
     ("sqlite" . sql))
 )

[-- Attachment #1.2: Type: text/html, Size: 13284 bytes --]

[-- Attachment #2: ob-shell-eval-with-newline --]
[-- Type: application/octet-stream, Size: 584 bytes --]

diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 2c30a2605..95ea4f635 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -324,7 +324,7 @@ return the value of the last statement in BODY."
 		(insert body))
 	      (set-file-modes script-file #o755)
 	      (org-babel-eval script-file "")))
-	   (t (org-babel-eval shell-file-name (org-trim body))))))
+	   (t (org-babel-eval shell-file-name (concat (org-trim body) "\n"))))))
     (when (and results value-is-exit-status)
       (setq results (car (reverse (split-string results "\n" t)))))
     (when results

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-16 16:27 [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)] Osher Jacob
@ 2023-01-16 21:40 ` Matt
  2023-01-17  1:07   ` Matt
  0 siblings, 1 reply; 19+ messages in thread
From: Matt @ 2023-01-16 21:40 UTC (permalink / raw)
  To: Osher Jacob; +Cc: emacs-orgmode

Thank you for your report, Osher!

Windows shells aren't currently supported by ob-shell, AFAIK.  I'm open to including them.  Unfortunately, I don't have a Windows machine to test against.  

 ---- On Mon, 16 Jan 2023 11:27:52 -0500  Osher Jacob  wrote --- 
 > Expected behaviour:
 > On Windows, all lines of the babel shell block should be evaluated, with full output printed.

What's does `C-h v shell-file-name' say?  That should tell us what shell is being used.

 > #+begin_src shell
 >  echo 1
 >  echo 2
 >
 >
 > #+end_src
 >
 > #+RESULTS:
 > | Microsoft             | Windows   | [Version     | 10.0.19044.2364] |        |           |
 > | (c)                   | Microsoft | Corporation. | All              | rights | reserved. |
 > |                       |           |              |                  |        |           |
 > | c:\Users\osherj>echo  | 1         |              |                  |        |           |
 > | 1                     |           |              |                  |        |           |
 > |                       |           |              |                  |        |           |
 > | c:\Users\osherj>More? |           |              |                  |        |           |
 > 
 > The hacky way I solved it was to change this line:
 > (t (org-babel-eval shell-file-name (org-trim body))))))
 > to this:
 > (t (org-babel-eval shell-file-name (concat (org-trim body) "\n"))))))

I don't think org-trim is the issue.

Running the block (eventually) calls the default shell command, `org-babel--shell-command-on-region'.  This calls `org-babel--get-shell-file-name' on a temp file containing the block source using the "-c" flag.

I assume the shell used is cmdproxy.exe (https://git.savannah.gnu.org/cgit/emacs.git/tree/nt/cmdproxy.c).   It looks like this converts a "-c" to a "/c", among other things.

MSDN says for cmd.exe,

Parameter 	Description
/c 	Carries out the command specified by string and then stops.
/k 	Carries out the command specified by string and continues.

So, the way I reason it, ob-shell tries calling cmdproxy.exe using /c which basically calls cmd.exe /c, the block temp file is executed, and the shell stops.

I see that `org-babel--shell-command-on-region' calls `process-file' using the `shell-command-switch'.  It doesn't appear to be set anywhere else in `ob-eval' (see ob-eval:112).

> Let me know if there's any other information you need, or if I can help in any other way.

I wonder if changing `shell-command-switch' to "/k" would make a difference?  

That is, go through the steps to reproduce and before executing the block, run `M-: (setq shell-command-switch "/k")'.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-16 21:40 ` Matt
@ 2023-01-17  1:07   ` Matt
  2023-01-17 19:53     ` Osher Jacob
  0 siblings, 1 reply; 19+ messages in thread
From: Matt @ 2023-01-17  1:07 UTC (permalink / raw)
  To: osher jacob; +Cc: emacs-orgmode


 ---- On Mon, 16 Jan 2023 16:40:27 -0500  Matt  wrote --- 
 > That is, go through the steps to reproduce and before executing the block, run `M-: (setq shell-command-switch "/k")'.

Whoa, just re-read this after stepping away and it sounds super demanding!  Please excuse me, I've been trying to use active voice while writing documentation.

Let me try that again:

What happens if you go through the steps to reproduce and run `M-: (setq shell-command-switch "/k") before executing the block?


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-17  1:07   ` Matt
@ 2023-01-17 19:53     ` Osher Jacob
  2023-01-18  5:09       ` Matt
  0 siblings, 1 reply; 19+ messages in thread
From: Osher Jacob @ 2023-01-17 19:53 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode


[-- Attachment #1.1: Type: text/plain, Size: 4124 bytes --]

Hey, thanks for the input!
My shell-file-name is indeed pointing to cmdproxy.exe, and after changing
shell-command-switch to "/k" or "-k", I get a similar output:

#+begin_src shell
  echo 1
  echo 2
  echo 3

#+end_src

#+RESULTS:
| Microsoft    | Windows | [Version  | 10.0.14393]  |     |        |
    |
| (c)          |    2016 | Microsoft | Corporation. | All | rights |
reserved. |
|              |         |           |              |     |        |
    |
| c:\tmp>echo  |       1 |           |              |     |        |
    |
| 1            |         |           |              |     |        |
    |
|              |         |           |              |     |        |
    |
| c:\tmp>echo  |       2 |           |              |     |        |
    |
| 2            |         |           |              |     |        |
    |
|              |         |           |              |     |        |
    |
| c:\tmp>More? |         |           |              |     |        |
    |

Where only the last line is missing.

Another observation that might be relevant, is that the block execution
works well when I set shell-file-name to "powershell.exe"

#+begin_src shell
  echo 1
  echo 2
  echo 3

#+end_src

#+RESULTS:
| Windows   | PowerShell |      |           |              |     |        |
          |
| Copyright | (C)        | 2016 | Microsoft | Corporation. | All | rights |
reserved. |
|           |            |      |           |              |     |        |
          |
| PS        | C:\tmp>    | echo |         1 |              |     |        |
          |
| 1         |            |      |           |              |     |        |
          |
| PS        | C:\tmp>    | echo |         2 |              |     |        |
          |
| 2         |            |      |           |              |     |        |
          |
| PS        | C:\tmp>    | echo |        33 |              |     |        |
          |
| PS        | C:\tmp>    |      |           |              |     |        |
          |

And here, it might be that the missing newline in the last command is
visible, hence the double '3' character (one is a part of the "echo 3"
input, the second is the output "3" echoed back).

You also mentioned the source code block is being passed through the "-c"
flag as a command-line argument.
I might be misunderstanding something here, but it seems like it is being
passed through the stdin of the shell process when the calls process-file
-> call-process are being made.

Just to confirm, I've made a simple utility that prints the contents of
argc and argv as hex strings, and got the following output when setting it
in place of the the shell-file-name:

#+begin_src shell
  echo 1
  echo 2
  echo 3


#+end_src

#+RESULTS:
| argc:                                                              | 3 |
| 633a5c6d73797336345c686f6d655c4f736865725c6563686f5f6865782e657865 |   |
| 2d63                                                               |   |
| 633a2f6d73797336342f686f6d652f4f736865722f6563686f5f6865782e657865 |   |

Which translates to:
c:\msys64\home\Osher\echo_hex.exe -c c:/msys64/home/Osher/echo_hex.exe

Exposing that there are two nested shell-file-name running.

And now I was curious to see if babel executes cmdproxy.exe -c
cmdproxy.exe, so I fired up Procmon, and indeed that was the case (see
attached screenshot).

[image: image.png]

Not sure if this nesting is really relevant to the problem at hand, but
thought it could be useful information.

Any ideas on how to proceed from here?


On Tue, Jan 17, 2023 at 3:07 AM Matt <matt@excalamus.com> wrote:

>
>  ---- On Mon, 16 Jan 2023 16:40:27 -0500  Matt  wrote ---
>  > That is, go through the steps to reproduce and before executing the
> block, run `M-: (setq shell-command-switch "/k")'.
>
> Whoa, just re-read this after stepping away and it sounds super
> demanding!  Please excuse me, I've been trying to use active voice while
> writing documentation.
>
> Let me try that again:
>
> What happens if you go through the steps to reproduce and run `M-: (setq
> shell-command-switch "/k") before executing the block?
>

[-- Attachment #1.2: Type: text/html, Size: 5897 bytes --]

[-- Attachment #2: image.png --]
[-- Type: image/png, Size: 13297 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-17 19:53     ` Osher Jacob
@ 2023-01-18  5:09       ` Matt
  2023-01-18  9:05         ` Ihor Radchenko
  0 siblings, 1 reply; 19+ messages in thread
From: Matt @ 2023-01-18  5:09 UTC (permalink / raw)
  To: Osher Jacob; +Cc: emacs-orgmode


 ---- On Tue, 17 Jan 2023 14:53:39 -0500  Osher Jacob  wrote --- 
 > changing shell-command-switch to "/k" or "-k", I get a similar output:

Thanks for checking that.

 > You also mentioned the source code block is being passed through the "-c" flag as a command-line argument.I might be misunderstanding something here, but it seems like it is being passed through the stdin of the shell process when the calls process-file -> call-process are being made.

That the block source is passed through "-c" is an educated guess.  I keep finding myself in C land where it's less clear to me what precisely happens.  You're correct that `call-process' acts on the block source.  It's not clear to me whether that's done through stdin or whether being passed through stdin would even help us.  Here's what I'm seeing.

Evaluating the block goes through 4 lisp commands and then into C:

1. Calls `(org-babel-eval shell-file-name (org-trim body))'

This evaluates as...

(org-babel-eval
  "cmdproxy.exe" ; command, whatever `shell-file-name' is
  (org-trim body) ; query, the block body
  )

The query parameter (the block body) gets inserted into a temp buffer on which `org-babel--shell-command-on-region' is called.

2. Calls `(org-babel--shell-command-on-region command error-buffer)' on a temp buffer containing block body.

This evaluates as...

(org-babel--shell-command-on-region
  command       ; "cmdproxy.exe"
  error-buffer  ;  #<buffer  *Org-Babel Error*>
  )

This in turn calls `process-file' where INPUT-FILE is a temp file containing the contents of the buffer `org-babel--shell-command-on-region' was called on (that is, the temp file contains the block body).

3. Calls `(process-file shell-file-name input-file (if error-file (list t error-file) t) nil shell-command-switch command)'

This evaluates as...

(process-file
  "cmdproxy.exe"                                               ; shell-file-name
  "/path/to/temp/containing/block/source"  ; input-file
  (t "/tmp/babel jTCHe/ob-error-yHOivA")    ; output destination (like call-process's DESTINATION)
  nil                                                                     ; display
 "-c"                                                                    ; args
  "cmdproxy.exe"                                               ; args
)

Of course, I imagine the paths would be Windows paths when run on Windows.

According to the documentation, the args are passed to the process (cmdproxy.exe) "verbatim without filename handling, as `call-process' does."  The `call-process' documentation says, "remaining arguments ARGS are strings passed as command arguments to PROGRAM."

To me, that sounds like cmdproxy.exe gets "passed into" cmdproxy.exe.  This would explain the nested shell calls.  If all this were written out, I imagine it would look something like:

cmdproxy.exe -c cmdproxy.exe

What's not clear to me is how the INPUT-FILE is handled.  

The `process-file' documentation says "normally".  I assume it's how `call-process' handles INPUT-FILE, but the documentation doesn't really address it.

Finally, `process-file' finally calls `call-process'.

4. Calls `(apply 'call-process program (or lc infile) (if stderr-file (list (car buffer) stderr-file) buffer) display args)'

This evaluates as...

(apply 'call-process 
           program                                                                  ; cmdproxy
          (or lc infile)                                                              ; local-copy of the INFILE,  "/path/to/temp/containing/block/source" 
          (if stderr-file (list (car buffer) stderr-file) buffer)  ; (t "/tmp/babel jTCHe/ob-error-yHOivA"  )
          display                                                                     ; nil
          args)                                                                         ; ("-c"  "cmdproxy.exe")

How this actually works is non-trivial (https://git.savannah.gnu.org/cgit/emacs.git/tree/src/callproc.c#n250) and not something I understand at the moment.  We can see here that indeed a call like `cmdproxy.exe -c cmdproxy.exe' is being made.  Still, I'm not sure how the INPUT-FILE gets processed.  For example, is it passed in the second cmdproxy call?  Or, maybe it gets called first and then the second call to cmdproxy happens and hangs?  I don't know.

 > Any ideas on how to proceed from here?

I have two ideas.

1. Another naive work around attempt.  Again, I'm going from memory, documentation, and what I have previously written.  

I have in my init a command to open a terminal when working on Windows that looks like:

    (start-process "cmd" nil "cmd.exe" "/C" "start" "cmd.exe" "/K" "cd" dir)

This starts a cmd process with /c which terminates the prompt after the command that follows is run.  The command  That follows starts another cmd process with /k which changes the directory and leaves the terminal open.

I have another command to open QGIS that does the same thing:

    (start-process "cmd" nil "cmd.exe" "/C" "start" "\"qgis\"" "cmd.exe" "/K" "C:\\Program Files\\QGIS 3.22.3\\bin\\qgis.bat")

It's not clear to me why the extra call to a second cmd with /k is needed.  Maybe it's copy-pasta or Windows being Windows.

Anyway, I mention these because I wonder if we might be able to do something similar.

Try changing `shell-file-name' to "cmd.exe" (ideally, the full path) and `shell-command-switch' to "/k" (or maybe "-k").  My hope would be that the final call would look something like,

cmd.exe /k cmd.exe /k <executes-input-file-as-stdin-or-whatever>

Given my analysis, I'm not sure if there's a way we could trick the call into being `cmd.exe /c cmd.exe /k input-file'.

2. We could write some ob-shell code to explicitly handle Windows cmd and powershell.  For example, call `process-file' similar to the stdin/cmdline branch of `org-babel-sh-evaluate'.

I'm open to this, but, like I've said, I don't have a Windows machine to work on right now.  I'd not be able to verify it.  I, and I'm sure others, would be happy to guide you if that's something you'd like to help implement.

Thoughts?



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-18  5:09       ` Matt
@ 2023-01-18  9:05         ` Ihor Radchenko
  2023-01-19 16:28           ` Osher Jacob
  0 siblings, 1 reply; 19+ messages in thread
From: Ihor Radchenko @ 2023-01-18  9:05 UTC (permalink / raw)
  To: Matt; +Cc: Osher Jacob, emacs-orgmode

Matt <matt@excalamus.com> writes:

> (process-file
>   "cmdproxy.exe"                                               ; shell-file-name
>   "/path/to/temp/containing/block/source"  ; input-file
>   (t "/tmp/babel jTCHe/ob-error-yHOivA")    ; output destination (like call-process's DESTINATION)
>   nil                                                                     ; display
>  "-c"                                                                    ; args
>   "cmdproxy.exe"                                               ; args

> Thoughts?

If running cmdproxy.exe /c cmdproxy.exe /path/to/input is not wrong, the
problem is on Emacs side.

Osher, could you try putting your example script into a file and running
the command line directly? What will it output?

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-18  9:05         ` Ihor Radchenko
@ 2023-01-19 16:28           ` Osher Jacob
  2023-01-20  4:29             ` Matt
  2023-01-20  9:24             ` Ihor Radchenko
  0 siblings, 2 replies; 19+ messages in thread
From: Osher Jacob @ 2023-01-19 16:28 UTC (permalink / raw)
  To: Ihor Radchenko, Matt; +Cc: emacs-orgmode

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

Thanks for the suggestions!

On Wed, Jan 18, 2023 at 7:09 AM Matt <matt@excalamus.com> wrote:

>
> 1. Another naive work around attempt.  Again, I'm going from memory,
> documentation, and what I have previously written.
>
> I have in my init a command to open a terminal when working on Windows
> that looks like:
>
>     (start-process "cmd" nil "cmd.exe" "/C" "start" "cmd.exe" "/K" "cd"
> dir)
>
> This starts a cmd process with /c which terminates the prompt after the
> command that follows is run.  The command  That follows starts another cmd
> process with /k which changes the directory and leaves the terminal open.
>
> I have another command to open QGIS that does the same thing:
>
>     (start-process "cmd" nil "cmd.exe" "/C" "start" "\"qgis\"" "cmd.exe"
> "/K" "C:\\Program Files\\QGIS 3.22.3\\bin\\qgis.bat")
>
> It's not clear to me why the extra call to a second cmd with /k is
> needed.  Maybe it's copy-pasta or Windows being Windows.
>
> Anyway, I mention these because I wonder if we might be able to do
> something similar.
>
> Try changing `shell-file-name' to "cmd.exe" (ideally, the full path) and
> `shell-command-switch' to "/k" (or maybe "-k").  My hope would be that the
> final call would look something like,
>
> cmd.exe /k cmd.exe /k <executes-input-file-as-stdin-or-whatever>
>
> Given my analysis, I'm not sure if there's a way we could trick the call
> into being `cmd.exe /c cmd.exe /k input-file'.
>


On Wed, Jan 18, 2023 at 11:05 AM Ihor Radchenko <yantar92@posteo.net> wrote:

> If running cmdproxy.exe /c cmdproxy.exe /path/to/input is not wrong, the
> problem is on Emacs side.


> Osher, could you try putting your example script into a file and running
> the command line directly? What will it output?
>

Unfortunately, it seems like cmdproxy.exe and cmd.exe cannot accept an
input file as a command-line argument and execute it.

In the case of running :
cmdproxy.exe /c cmdproxy.exe /c C:/tmp/inp

We get:
cmdproxy.exe /c cmdproxy.exe /c C:/tmp/inp
'C:/tmp/inp' is not recognized as an internal or external command,
operable program or batch file.


If we're insistent on passing the input through the command line arguments,
I can think of two ways to go about this, but both seem undesirable:
- Dumping the content of the input buffer into a ".bat" file and then
passing it as an argument. That way we end up executing "cmdproxy.exe /c
cmdproxy.exe input.bat", but then batch files produce a slightly different
behaviour than running in the context of an interactive shell.
or
- Concating the lines together with the "&" in between them. This would
look like 'cmdproxy.exe /c cmdproxy.exe /c "echo 1 & echo 2 & echo 3",
which is also a pretty nasty solution in my opinion.

This leads me to your next point, which sounds like the most natural way to
go about it.

2. We could write some ob-shell code to explicitly handle Windows cmd and
> powershell.  For example, call `process-file' similar to the stdin/cmdline
> branch of `org-babel-sh-evaluate'.
>
> I'm open to this, but, like I've said, I don't have a Windows machine to
> work on right now.  I'd not be able to verify it.  I, and I'm sure others,
> would be happy to guide you if that's something you'd like to help
> implement.
>
>
I think it could be enough to make sure the input ends with a newline, in
which case it could be done the way I proposed in the first message, that
is changing:
(t (org-babel-eval shell-file-name (org-trim body))))))
to
(t (org-babel-eval shell-file-name (concat (org-trim body) "\n"))))))

I think as long as this change doesn't break the code in non-Windows
operating systems (How would we go about verifying this?), it would be
preferable due to its simplicity and minimality.

If for any other reason you would rather not change
the org-babel-sh-evaluate, and would like to implement a separate function
for Windows, that's also a viable option.
I am willing to try and go down that path, although I'm not a very
experienced lisper.

Do any of these options sound like they could work well?

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-19 16:28           ` Osher Jacob
@ 2023-01-20  4:29             ` Matt
  2023-01-20  9:27               ` Ihor Radchenko
  2023-01-20  9:24             ` Ihor Radchenko
  1 sibling, 1 reply; 19+ messages in thread
From: Matt @ 2023-01-20  4:29 UTC (permalink / raw)
  To: Osher Jacob; +Cc: ihor radchenko, emacs-orgmode


 ---- On Thu, 19 Jan 2023 11:28:09 -0500  Osher Jacob  wrote --- 
 
 > If we're insistent on passing the input through the command line arguments, I can think of two ways to go about this, but both seem undesirable

They're good ideas and, I agree, aren't ideal.  

 >  I think it could be enough to make sure the input ends with a newline, in which case it could be done the way I proposed in the first message, that is changing:(t (org-babel-eval shell-file-name (org-trim body))))))
 > to(t (org-babel-eval shell-file-name (concat (org-trim body) "\n"))))))

Your original proposal was reasonable and looks even more so now. 
 
 > I think as long as this change doesn't break the code in non-Windows operating systems (How would we go about verifying this?), it would be preferable due to its simplicity and minimality.

Agreed, I like its simplicity.  Tests are a first step and they all pass with your suggested change.

 > If for any other reason you would rather not change the org-babel-sh-evaluate, and would like to implement a separate function for Windows, that's also a viable option.I am willing to try and go down that path, although I'm not a very experienced lisper.
 > Do any of these options sound like they could work well?

I agree, putting in the newline is simple and it would be easy enough to create a separate path for Windows/cmd if it were a problem.

I wonder if it might make sense to move where the trimming happens so that the body passed into `org-babel-sh-evaluate' is ready to go and we could handle the cmd case separately and earlier (by inserting the newline there)?

I appreciate you taking the time to explore and for being open to what feels like me over-thinking it :)

I'll be away from the keyboard for a few days and I look forward to getting this wrapped up for you.  Meanwhile, I'm curious if Ihor or others have advice.






^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-19 16:28           ` Osher Jacob
  2023-01-20  4:29             ` Matt
@ 2023-01-20  9:24             ` Ihor Radchenko
  1 sibling, 0 replies; 19+ messages in thread
From: Ihor Radchenko @ 2023-01-20  9:24 UTC (permalink / raw)
  To: Osher Jacob; +Cc: Matt, emacs-orgmode

Osher Jacob <osherz5@gmail.com> writes:

> On Wed, Jan 18, 2023 at 11:05 AM Ihor Radchenko <yantar92@posteo.net> wrote:
>
>> If running cmdproxy.exe /c cmdproxy.exe /path/to/input is not wrong, the
>> problem is on Emacs side.
>
>
>> Osher, could you try putting your example script into a file and running
>> the command line directly? What will it output?
>>
>
> Unfortunately, it seems like cmdproxy.exe and cmd.exe cannot accept an
> input file as a command-line argument and execute it.
>
> In the case of running :
> cmdproxy.exe /c cmdproxy.exe /c C:/tmp/inp
>
> We get:
> cmdproxy.exe /c cmdproxy.exe /c C:/tmp/inp
> 'C:/tmp/inp' is not recognized as an internal or external command,
> operable program or batch file.

... which means that I did not dig deep enough.
Apparently, `process-file' is not passing the file to cmdproxy.exe.  It
calls `call-process', which reads file and passes its contents as input.

So, we don't need to do awkward manoeuvring with "&" and making the src
block contents into a one-liner. It's about understanding how
cmdproxy.exe handles stdin.

> I think it could be enough to make sure the input ends with a newline, in
> which case it could be done the way I proposed in the first message, that
> is changing:
> (t (org-babel-eval shell-file-name (org-trim body))))))
> to
> (t (org-babel-eval shell-file-name (concat (org-trim body) "\n"))))))
>
> I think as long as this change doesn't break the code in non-Windows
> operating systems (How would we go about verifying this?), it would be
> preferable due to its simplicity and minimality.

This looks like the way to go. For non-Windows systems we have some test
coverage.

I am wondering if we should add tests for powershell and cmdproxy.
Though only users with Windows will actually be able to run those.

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-20  4:29             ` Matt
@ 2023-01-20  9:27               ` Ihor Radchenko
  2023-01-23  3:12                 ` Matt
  0 siblings, 1 reply; 19+ messages in thread
From: Ihor Radchenko @ 2023-01-20  9:27 UTC (permalink / raw)
  To: Matt; +Cc: Osher Jacob, emacs-orgmode

Matt <matt@excalamus.com> writes:

> I agree, putting in the newline is simple and it would be easy enough to create a separate path for Windows/cmd if it were a problem.
>
> I wonder if it might make sense to move where the trimming happens so that the body passed into `org-babel-sh-evaluate' is ready to go and we could handle the cmd case separately and earlier (by inserting the newline there)?

I think `org-babel--shell-command-on-region' will be more appropriate.
Because similar issues might appear when attempting to evaluate other
code blocks on Windows, where `shell-file-name' is set to cmdproxy.exe.

It will also be useful to leave a comment in the code explaining why we
need this newline.

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-20  9:27               ` Ihor Radchenko
@ 2023-01-23  3:12                 ` Matt
  2023-01-23 11:42                   ` Ihor Radchenko
  2023-01-26  4:04                   ` Matt
  0 siblings, 2 replies; 19+ messages in thread
From: Matt @ 2023-01-23  3:12 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: osher jacob, emacs-orgmode


 ---- On Fri, 20 Jan 2023 04:27:18 -0500  Ihor Radchenko  wrote --- 
 > I think `org-babel--shell-command-on-region' will be more appropriate.
 > Because similar issues might appear when attempting to evaluate other
 > code blocks on Windows, where `shell-file-name' is set to cmdproxy.exe.
 > 
 > It will also be useful to leave a comment in the code explaining why we
 > need this newline.

Works for me.  

How should I update bugfix to match main?  Since we're considering this a bug, I figured I'd do this on bugfix.  I notice, however, that bugfix doesn't have the latest updates to test-ob-shell.  When I rebase main onto bugfix I get a ton of conflicts.  If I instead merge main into bugfix, that shows many merge commits with bugfix.  Since I don't see any "Merge with 'bugfix'" commits on bugfix currently, I assume that's not the way to do it.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-23  3:12                 ` Matt
@ 2023-01-23 11:42                   ` Ihor Radchenko
  2023-01-26  4:04                   ` Matt
  1 sibling, 0 replies; 19+ messages in thread
From: Ihor Radchenko @ 2023-01-23 11:42 UTC (permalink / raw)
  To: Matt; +Cc: osher jacob, emacs-orgmode

Matt <matt@excalamus.com> writes:

>  ---- On Fri, 20 Jan 2023 04:27:18 -0500  Ihor Radchenko  wrote --- 
>  > I think `org-babel--shell-command-on-region' will be more appropriate.
>  > Because similar issues might appear when attempting to evaluate other
>  > code blocks on Windows, where `shell-file-name' is set to cmdproxy.exe.
>  > 
>  > It will also be useful to leave a comment in the code explaining why we
>  > need this newline.
>
> Works for me.  
>
> How should I update bugfix to match main?  Since we're considering
> this a bug, I figured I'd do this on bugfix.

bugfix is only for trivial and critical bugs.
Non-trivial yet non-critical should not be on bugfix.
They are for the next minor release.
See https://orgmode.org/worg/org-maintenance.html#branches

> I notice, however, that bugfix doesn't have the latest updates to test-ob-shell.  When I rebase main onto bugfix I get a ton of conflicts.  If I instead merge main into bugfix, that shows many merge commits with bugfix.  Since I don't see any "Merge with 'bugfix'" commits on bugfix currently, I assume that's not the way to do it.

You must not merge main onto bugfix.
Such merge is called release :)

Can you fix the bug on bugfix? If yes, do it on bugfix and then merge
bugfix onto main. If no, just fix it on main - this appears to be an old
bug that existed for a long time, and we may as well postpone it to the
next minor release and not risk breaking bugfix branch with non-trivial
changes.

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-23  3:12                 ` Matt
  2023-01-23 11:42                   ` Ihor Radchenko
@ 2023-01-26  4:04                   ` Matt
  2023-01-26  9:51                     ` Ihor Radchenko
  1 sibling, 1 reply; 19+ messages in thread
From: Matt @ 2023-01-26  4:04 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: osher jacob, emacs-orgmode

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


 >  ---- On Fri, 20 Jan 2023 04:27:18 -0500  Ihor Radchenko  wrote --- 
 > I think `org-babel--shell-command-on-region' will be more appropriate.
 > Because similar issues might appear when attempting to evaluate other
 > code blocks on Windows, where `shell-file-name' is set to cmdproxy.exe.

Is something like this what you're thinking?  Or, do we want to check explicitly for "cmdproxy.exe"?

Also, this bug isn't currently tracked in Woof!.  I'm not able to confirm it, but since we're putting in a fix, should be add it anyway?


[-- Attachment #2: ob-eval-handle-windows-shell.patch --]
[-- Type: application/octet-stream, Size: 718 bytes --]

diff --git a/lisp/ob-eval.el b/lisp/ob-eval.el
index b9d1f7f23..9d79527a0 100644
--- a/lisp/ob-eval.el
+++ b/lisp/ob-eval.el
@@ -118,7 +118,13 @@ returned."
 			(if error-file
 			    (list t error-file)
 			  t)
-			nil shell-command-switch command))
+			nil shell-command-switch
+                        ;; Pass newline to satisfy Windows
+                        ;; cmdproxy.exe prompt for More.
+                        ;;
+                        ;; See:
+                        ;; `https://list.orgmode.org/orgmode/87bkmttv2h.fsf@localhost/'
+                        (concat command "\n")))
 
     (when (and input-file (file-exists-p input-file)
 	       ;; bind org-babel--debug-input around the call to keep

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-26  4:04                   ` Matt
@ 2023-01-26  9:51                     ` Ihor Radchenko
  2023-01-30  6:00                       ` Matt
  0 siblings, 1 reply; 19+ messages in thread
From: Ihor Radchenko @ 2023-01-26  9:51 UTC (permalink / raw)
  To: Matt, Bastien; +Cc: osher jacob, emacs-orgmode

Matt <matt@excalamus.com> writes:

>  >  ---- On Fri, 20 Jan 2023 04:27:18 -0500  Ihor Radchenko  wrote --- 
>  > I think `org-babel--shell-command-on-region' will be more appropriate.
>  > Because similar issues might appear when attempting to evaluate other
>  > code blocks on Windows, where `shell-file-name' is set to cmdproxy.exe.
>
> Is something like this what you're thinking?  Or, do we want to check explicitly for "cmdproxy.exe"?

AFAIK, extra newline should be safe in all other script languages.

> Also, this bug isn't currently tracked in Woof!.  I'm not able to confirm it, but since we're putting in a fix, should be add it anyway?

Your message with a patch is supposed to be auto-detected... (It is not,
which is a bug)
Bastien, heads up.

Do not worry about Woof! just yet. The new version is not yet officially
used. Bastien is still polishing the code.

> -			nil shell-command-switch command))
> +			nil shell-command-switch
> +                        ;; Pass newline to satisfy Windows
> +                        ;; cmdproxy.exe prompt for More.
> +                        ;;
> +                        ;; See:
> +                        ;; `https://list.orgmode.org/orgmode/87bkmttv2h.fsf@localhost/'
> +                        (concat command "\n")))

This will add a newline to "cmdproxy.exe" command -> "cmdproxy.exe\n".
You should instead look into `org-babel--write-temp-buffer-input-file'.

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-26  9:51                     ` Ihor Radchenko
@ 2023-01-30  6:00                       ` Matt
  2023-01-30 14:00                         ` Ihor Radchenko
  0 siblings, 1 reply; 19+ messages in thread
From: Matt @ 2023-01-30  6:00 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: osher jacob, emacs-orgmode


 ---- On Thu, 26 Jan 2023 04:51:19 -0500  Ihor Radchenko  wrote --- 

 > This will add a newline to "cmdproxy.exe" command -> "cmdproxy.exe\n".
 > You should instead look into `org-babel--write-temp-buffer-input-file'.

I made the change in `org-babel--shell-command-on-region' based on your recommendation in https://list.orgmode.org/orgmode/87bkmttv2h.fsf@localhost/

I'd be happy to put it in `org-babel--write-temp-buffer-input-file'.  Unfortunately, I don't understand why you suggest the change be made there.  Wouldn't it make more sense to insert the newline into the temp buffer that `org-babel--write-temp-buffer-input-file' writes from? 

I'm confused because I don't see a good way write a newline from `org-babel--write-temp-buffer-input-file'.  All the file writing functions[fn:1], with the exception of `with-temp-file', require a region's start and end and `with-temp-file' appears to overwrite the file.

;; 99.999% sure this isn't what you meant
 (defun org-babel--write-temp-buffer-input-file (input-file)
   "Write the contents of the current temp buffer into INPUT-FILE."
   (let ((start (point-min))
         (end (point-max)))
     (goto-char start)
     (push-mark (point) 'nomsg)
     (write-region start end input-file)
+    (with-temp-buffer
+      (insert "\n")
+      (write-region (point-min) (point-max) input-file))
     (delete-region start end)
     (exchange-point-and-mark)))

AFAICT, `org-babel--write-temp-buffer-input-file' is only called by `org-babel--shell-command-on-region' and that's only ever called by `org-babel-eval'.  That's where the query (the block source) is inserted into a temp buffer.  Maybe something like this?

modified   lisp/ob-eval.el
@@ -64,7 +64,7 @@ Writes QUERY into a temp-buffer that is processed with
   (let ((error-buffer (get-buffer-create " *Org-Babel Error*")) exit-code)
     (with-current-buffer error-buffer (erase-buffer))
     (with-temp-buffer
-      (insert query "\n")
+      (insert (concat query "\n"))
       (setq exit-code
             (org-babel--shell-command-on-region
              command error-buffer))

 [fn:1] https://www.gnu.org/software/emacs/manual/html_node/elisp/Writing-to-Files.html#Writing-to-Files





^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-30  6:00                       ` Matt
@ 2023-01-30 14:00                         ` Ihor Radchenko
  2023-01-30 17:08                           ` Matt
  0 siblings, 1 reply; 19+ messages in thread
From: Ihor Radchenko @ 2023-01-30 14:00 UTC (permalink / raw)
  To: Matt; +Cc: osher jacob, emacs-orgmode

Matt <matt@excalamus.com> writes:

>  ---- On Thu, 26 Jan 2023 04:51:19 -0500  Ihor Radchenko  wrote --- 
>
>  > This will add a newline to "cmdproxy.exe" command -> "cmdproxy.exe\n".
>  > You should instead look into `org-babel--write-temp-buffer-input-file'.
>
> I made the change in `org-babel--shell-command-on-region' based on your recommendation in https://list.orgmode.org/orgmode/87bkmttv2h.fsf@localhost/
>
> I'd be happy to put it in `org-babel--write-temp-buffer-input-file'.  Unfortunately, I don't understand why you suggest the change be made there.  Wouldn't it make more sense to insert the newline into the temp buffer that `org-babel--write-temp-buffer-input-file' writes from? 

I just thought that it is the most logical place to add trailing
newline. `org-babel--write-temp-buffer-input-file' deletes the buffer
contents anyway, so modification will not cause any undesired side
effects.

> ;; 99.999% sure this isn't what you meant
>  (defun org-babel--write-temp-buffer-input-file (input-file)
>    "Write the contents of the current temp buffer into INPUT-FILE."
>    (let ((start (point-min))
>          (end (point-max)))
>      (goto-char start)
>      (push-mark (point) 'nomsg)
>      (write-region start end input-file)
> +    (with-temp-buffer
> +      (insert "\n")
> +      (write-region (point-min) (point-max) input-file))
>      (delete-region start end)
>      (exchange-point-and-mark)))

I meant inserting newline in current buffer at the beginning.

> AFAICT, `org-babel--write-temp-buffer-input-file' is only called by `org-babel--shell-command-on-region' and that's only ever called by `org-babel-eval'.  That's where the query (the block source) is inserted into a temp buffer.  Maybe something like this?
>
> modified   lisp/ob-eval.el
> @@ -64,7 +64,7 @@ Writes QUERY into a temp-buffer that is processed with
>    (let ((error-buffer (get-buffer-create " *Org-Babel Error*")) exit-code)
>      (with-current-buffer error-buffer (erase-buffer))
>      (with-temp-buffer
> -      (insert query "\n")
> +      (insert (concat query "\n"))

The original is just (insert query).
(insert query "\n") is also ok.

I don't have strong preference here.

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-30 14:00                         ` Ihor Radchenko
@ 2023-01-30 17:08                           ` Matt
  2023-02-01 12:05                             ` Ihor Radchenko
  0 siblings, 1 reply; 19+ messages in thread
From: Matt @ 2023-01-30 17:08 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: osher jacob, emacs-orgmode

Pushed the change to `org-babel-eval'.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-01-30 17:08                           ` Matt
@ 2023-02-01 12:05                             ` Ihor Radchenko
  2023-02-01 20:21                               ` Matt
  0 siblings, 1 reply; 19+ messages in thread
From: Ihor Radchenko @ 2023-02-01 12:05 UTC (permalink / raw)
  To: Matt; +Cc: osher jacob, emacs-orgmode

Matt <matt@excalamus.com> writes:

> Pushed the change to `org-babel-eval'.

Thanks!

This and another commit are rather trivial - they are eligible for
bugfix. See https://orgmode.org/worg/org-maintenance.html#branches
I cherry-picked the commits for bugfix.

I also added a comment clarifying the purpose of the newline.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=b4a1133a1

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)]
  2023-02-01 12:05                             ` Ihor Radchenko
@ 2023-02-01 20:21                               ` Matt
  0 siblings, 0 replies; 19+ messages in thread
From: Matt @ 2023-02-01 20:21 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: osher jacob, emacs-orgmode


 ---- On Wed, 01 Feb 2023 07:05:45 -0500  Ihor Radchenko  wrote --- 

 > This and another commit are rather trivial - they are eligible for
 > bugfix. See https://orgmode.org/worg/org-maintenance.html#branches
 > I cherry-picked the commits for bugfix.

Noted for the future.

 > I also added a comment clarifying the purpose of the newline.
 > https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=b4a1133a1

My apology for not carrying over the note.  Thank you for correcting it.


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2023-02-01 20:22 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-16 16:27 [BUG] ob-shell doesn't evaluate last line on Windows (cmd/cmdproxy) [9.6.1 ( @ c:/Users/Osher/AppData/Roaming/.emacs.d/elpa/org-9.6.1/)] Osher Jacob
2023-01-16 21:40 ` Matt
2023-01-17  1:07   ` Matt
2023-01-17 19:53     ` Osher Jacob
2023-01-18  5:09       ` Matt
2023-01-18  9:05         ` Ihor Radchenko
2023-01-19 16:28           ` Osher Jacob
2023-01-20  4:29             ` Matt
2023-01-20  9:27               ` Ihor Radchenko
2023-01-23  3:12                 ` Matt
2023-01-23 11:42                   ` Ihor Radchenko
2023-01-26  4:04                   ` Matt
2023-01-26  9:51                     ` Ihor Radchenko
2023-01-30  6:00                       ` Matt
2023-01-30 14:00                         ` Ihor Radchenko
2023-01-30 17:08                           ` Matt
2023-02-01 12:05                             ` Ihor Radchenko
2023-02-01 20:21                               ` Matt
2023-01-20  9:24             ` Ihor Radchenko

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).