Possible fix for :includes header argument in org-babel C source blocks

Possible fix for :includes header argument in org-babel C source blocks

[Brandon Guttersohn]

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

Hey all, I think I may have a small fix for executing C source blocks in 
org-babel. Or, possibly just a bad case of user error.

The issue (in emacs 27 with -q) is that it doesn't seem possible to 
specify non-system header files with the :includes header argument.

For example; while the following line:

     #+BEGIN_SRC C :includes <example.h>

...produces reasonable code...

     #include <example.h>

...all of the following lines...

     #+BEGIN_SRC C :includes example.h

     #+BEGIN_SRC C :includes "example.h"

     #+BEGIN_SRC C :includes "\"example.h\""

...just result in this...

     #include example.h

...rather than this...

     #include "example.h"

The attached patch will wrap filenames in quotes if they do not begin 
with "<", and works for me.

Is this a reasonable fix, or maybe I'm misusing the feature?


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

[-- Attachment #2: 0001-Fix-support-for-non-system-C-include-directives-in-o.patch --]
[-- Type: text/x-patch, Size: 1164 bytes --]

From e16887fd17c24397b3f9709fcb81e522adab13ee Mon Sep 17 00:00:00 2001
From: Brandon Guttersohn <brandon@guttersohn.org>
Date: Tue, 28 Apr 2020 19:16:41 -0500
Subject: [PATCH] Fix support for non-system C include directives in org-babel

C source blocks in org-babel support an ":includes" header argument,
which can generate #include directives prior to execution. Without
this patch, only system header files (enclosed in < and >) seem to
work, as double quotes are neither added nor preserved.

With this patch, any include file name which does not begin with a <
character will be wrapped in double quotes.
---
 lisp/org/ob-C.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/org/ob-C.el b/lisp/org/ob-C.el
index 3a26bc014b..00711cad12 100644
--- a/lisp/org/ob-C.el
+++ b/lisp/org/ob-C.el
@@ -232,7 +232,10 @@ org-babel-C-expand-C
 	       (list
 		;; includes
 		(mapconcat
-		 (lambda (inc) (format "#include %s" inc))
+		 (lambda (inc)
+           (if (string-prefix-p "<" inc)
+               (format "#include %s" inc)
+             (format "#include \"%s\"" inc)))
 		 includes "\n")
 		;; defines
 		(mapconcat
-- 
2.24.1

Re: Possible fix for :includes header argument in org-babel C source blocks

[Bastien]

Hi Brandon,

Brandon Guttersohn <brandon@guttersohn.org> writes:

> Hey all, I think I may have a small fix for executing C source blocks
> in org-babel. Or, possibly just a bad case of user error.
>
> The issue (in emacs 27 with -q) is that it doesn't seem possible to
> specify non-system header files with the :includes header argument.
>
> [...]
>
> The attached patch will wrap filenames in quotes if they do not begin
> with "<", and works for me.

Thanks for reporting this and for suggesting this patch, I think it is
good enough.  I have applied it to the master branch of Org:

https://code.orgmode.org/bzg/org-mode/commit/44cb98fdb6

Best,

-- 
 Bastien

Re: Possible fix for :includes header argument in org-babel C source blocks

[Kévin Le Gouguec]

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

Hi,

Bastien <bzg@gnu.org> writes:

> Brandon Guttersohn <brandon@guttersohn.org> writes:
>
>> Hey all, I think I may have a small fix for executing C source blocks
>> in org-babel. Or, possibly just a bad case of user error.
>>
>> The issue (in emacs 27 with -q) is that it doesn't seem possible to
>> specify non-system header files with the :includes header argument.
>>
>> [...]
>>
>> The attached patch will wrap filenames in quotes if they do not begin
>> with "<", and works for me.
>
> Thanks for reporting this and for suggesting this patch, I think it is
> good enough.  I have applied it to the master branch of Org:
>
> https://code.orgmode.org/bzg/org-mode/commit/44cb98fdb6
>
> Best,

I think this commit might have broken test ob-C/string-var: running
"make test" on master (516c038e5) right now I get:

> Test ob-C/string-var condition:
>     (wrong-type-argument sequencep <iostream>)
>    FAILED    8/834  ob-C/string-var (0.004651 sec)

The following patch fixes the test:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ob-C.patch --]
[-- Type: text/x-patch, Size: 373 bytes --]

diff --git a/lisp/ob-C.el b/lisp/ob-C.el
index c3e72c680..ae7b2ed1c 100644
--- a/lisp/ob-C.el
+++ b/lisp/ob-C.el
@@ -233,6 +233,7 @@ its header arguments."
 		;; includes
 		(mapconcat
 		 (lambda (inc)
+		   (when (symbolp inc) (setq inc (symbol-name inc)))
 		   (if (string-prefix-p "<" inc)
 		       (format "#include %s" inc)
 		     (format "#include \"%s\"" inc)))

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


I don't know if it's the best way forward; another way to make the test
pass is double-quoting "<iostream>" and "<cstring>" in ob-C-test.org:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: ob-C-test.patch --]
[-- Type: text/x-patch, Size: 472 bytes --]

diff --git a/testing/examples/ob-C-test.org b/testing/examples/ob-C-test.org
index 0faf630b9..efae02a19 100644
--- a/testing/examples/ob-C-test.org
+++ b/testing/examples/ob-C-test.org
@@ -38,7 +38,7 @@
 #+end_src
 
 #+source: string_var
-#+begin_src cpp :var q="word" :includes '(<iostream> <cstring>) :results silent
+#+begin_src cpp :var q="word" :includes '("<iostream>" "<cstring>") :results silent
   std::cout << q << ' ' << std::strlen(q);
   return 0;
 #+end_src

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


I guess there might be some people out there who will expect things to
keep working without double-quotes?  I have never used Babel, so I have
no idea…

I hope this has not already been brought up; apologies if so.

Re: Possible fix for :includes header argument in org-babel C source blocks

[Kyle Meyer]

Kévin Le Gouguec writes:

> Bastien <bzg@gnu.org> writes:
>
>> Brandon Guttersohn <brandon@guttersohn.org> writes:
>>
>>> Hey all, I think I may have a small fix for executing C source blocks
>>> in org-babel. Or, possibly just a bad case of user error.
>>>
>>> The issue (in emacs 27 with -q) is that it doesn't seem possible to
>>> specify non-system header files with the :includes header argument.
>>>
>>> [...]
>>>
>>> The attached patch will wrap filenames in quotes if they do not begin
>>> with "<", and works for me.
>>
>> Thanks for reporting this and for suggesting this patch, I think it is
>> good enough.  I have applied it to the master branch of Org:
>>
>> https://code.orgmode.org/bzg/org-mode/commit/44cb98fdb6

I think this discussion was on emacs-devel only, so here are some links
for others who might go looking for more context:

  https://lists.gnu.org/archive/html/emacs-devel/2020-04/msg01880.html
  https://lists.gnu.org/archive/html/emacs-devel/2020-05/msg03051.html

> I think this commit might have broken test ob-C/string-var: running
> "make test" on master (516c038e5) right now I get:
>
>> Test ob-C/string-var condition:
>>     (wrong-type-argument sequencep <iostream>)
>>    FAILED    8/834  ob-C/string-var (0.004651 sec)

Thanks.  I started seeing that as well but hadn't looked into it yet.

> The following patch fixes the test:
>
> diff --git a/lisp/ob-C.el b/lisp/ob-C.el
> index c3e72c680..ae7b2ed1c 100644
> --- a/lisp/ob-C.el
> +++ b/lisp/ob-C.el
> @@ -233,6 +233,7 @@ its header arguments."
>  		;; includes
>  		(mapconcat
>  		 (lambda (inc)
> +		   (when (symbolp inc) (setq inc (symbol-name inc)))
>  		   (if (string-prefix-p "<" inc)
>  		       (format "#include %s" inc)
>  		     (format "#include \"%s\"" inc)))
>
> I don't know if it's the best way forward; another way to make the test
> pass is double-quoting "<iostream>" and "<cstring>" in ob-C-test.org:

The above looks reasonable to me...

> diff --git a/testing/examples/ob-C-test.org b/testing/examples/ob-C-test.org
> index 0faf630b9..efae02a19 100644
> --- a/testing/examples/ob-C-test.org
> +++ b/testing/examples/ob-C-test.org
> @@ -38,7 +38,7 @@
>  #+end_src
>  
>  #+source: string_var
> -#+begin_src cpp :var q="word" :includes '(<iostream> <cstring>) :results silent
> +#+begin_src cpp :var q="word" :includes '("<iostream>" "<cstring>") :results silent
>    std::cout << q << ' ' << std::strlen(q);
>    return 0;
>  #+end_src

...given this test seems to be explicitly checking that case.

> I guess there might be some people out there who will expect things to
> keep working without double-quotes?  I have never used Babel, so I have
> no idea…

I don't know either, but the test does make me think so.  Hopefully
Brandon, Bastien, or someone else will chime in if that's not the case.

Could you send the first patch with a commit message tacked on?

Thanks again!

Re: Possible fix for :includes header argument in org-babel C source blocks

[Kévin Le Gouguec]

Kyle Meyer <kyle@kyleam.com> writes:

> I think this discussion was on emacs-devel only, so here are some links
> for others who might go looking for more context:
>
>   https://lists.gnu.org/archive/html/emacs-devel/2020-04/msg01880.html
>   https://lists.gnu.org/archive/html/emacs-devel/2020-05/msg03051.html

(Thanks, I should have thought to add some context before forwarding.)

>> I guess there might be some people out there who will expect things to
>> keep working without double-quotes?  I have never used Babel, so I have
>> no idea…
>
> I don't know either, but the test does make me think so.  Hopefully
> Brandon, Bastien, or someone else will chime in if that's not the case.

My opinion should only carry so much weight since I don't use Babel, but
from a quick reading of the sources, I couldn't find other examples of
this (symbol → string) coercion.  The only other instances of
lists-of-strings I could find in testing/examples were

> :var a='("abc" "def")

That leads me to believe that the coercion was an unintended side-effect
of (format …).

Of course, backward compatibility alone would mandate keeping the
coercion.

> Could you send the first patch with a commit message tacked on?

Will do ASAP.

BTW, does the change from 44cb98fdb deserve an ORG-NEWS entry?

Re: Possible fix for :includes header argument in org-babel C source blocks

[Kévin Le Gouguec]

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

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> That leads me to believe that the coercion was an unintended side-effect
> of (format …).

Never mind, the ORG-NEWS entry for 9.1 shows an example of unquoted
header, so I guess it is intentional.

Here is a patch to fix the regression:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Coerce-symbols-found-in-includes-header-arguments-to.patch --]
[-- Type: text/x-diff, Size: 1062 bytes --]

From b68122821a26578470506938c3a358f52f5d7a46 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Thu, 28 May 2020 11:09:18 +0200
Subject: [PATCH 1/2] Coerce symbols found in :includes header arguments to
 strings

Fix regression from 2020-05-24T16:23:26Z!brandon@guttersohn.org
(commit 44cb98fdb), which broke test ob-C/string-var.

* lisp/ob-C.el (org-babel-C-expand-C): Make sure items in :includes
arguments are strings before performing string operations on them.
---
 lisp/ob-C.el | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lisp/ob-C.el b/lisp/ob-C.el
index c3e72c680..42c60c296 100644
--- a/lisp/ob-C.el
+++ b/lisp/ob-C.el
@@ -233,6 +233,9 @@ its header arguments."
 		;; includes
 		(mapconcat
 		 (lambda (inc)
+		   ;; :includes '(<foo> <bar>) gives us a list of
+		   ;; symbols; convert those to strings.
+		   (when (symbolp inc) (setq inc (symbol-name inc)))
 		   (if (string-prefix-p "<" inc)
 		       (format "#include %s" inc)
 		     (format "#include \"%s\"" inc)))
-- 
2.17.1


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


And here is a patch to add a test for the unquoted-single-header case,
since otherwise it's hard to tell whether this behaviour is intentional:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-Add-test-case-for-symbol-coercion-in-C-Babel-blocks.patch --]
[-- Type: text/x-diff, Size: 4531 bytes --]

From cf1bb27215a46a521bb2f50d16b7dbc7441d81ec Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Thu, 28 May 2020 11:47:25 +0200
Subject: [PATCH 2/2] Add test case for symbol coercion in C Babel blocks

The ORG-NEWS entry for version 9.1 suggests that this coercion was
always intended, though AFAICT there was no test case for it.

* testing/lisp/test-ob-C.el (ob-C/symbol-include): Check explicitly
that :includes <iostream> (with no double-quotes around <iostream>)
will be parsed correctly.
(ob-D/simple-program, ob-C/integer-var, ob-D/integer-var,
ob-C/two-integer-var, ob-D/two-integer-var, ob-C/string-var,
ob-D/string-var, ob-C/preprocessor): Adjust block indices.

* testing/examples/ob-C-test.org (Simple tests): Add input for the new
test.
---
 testing/examples/ob-C-test.org |  6 ++++++
 testing/lisp/test-ob-C.el      | 23 +++++++++++++++--------
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/testing/examples/ob-C-test.org b/testing/examples/ob-C-test.org
index 0faf630b9..347607cae 100644
--- a/testing/examples/ob-C-test.org
+++ b/testing/examples/ob-C-test.org
@@ -10,6 +10,12 @@
   return 0;
 #+end_src
 
+#+source: simple
+#+begin_src cpp :includes <iostream> :results silent
+  std::cout << 42;
+  return 0;
+#+end_src
+
 #+source: simple
 #+begin_src D :results silent
   writefln ("%s", 42);
diff --git a/testing/lisp/test-ob-C.el b/testing/lisp/test-ob-C.el
index 3e4a63f88..6b6b728a2 100644
--- a/testing/lisp/test-ob-C.el
+++ b/testing/lisp/test-ob-C.el
@@ -32,60 +32,67 @@
 		      (org-babel-next-src-block 1)
 		      (should (= 42 (org-babel-execute-src-block))))))
 
+(ert-deftest ob-C/symbol-include ()
+  "Hello world program with unquoted :includes."
+  (if (executable-find org-babel-C++-compiler)
+      (org-test-at-id "fa6db330-e960-4ea2-ac67-94bb845b8577"
+		      (org-babel-next-src-block 2)
+		      (should (= 42 (org-babel-execute-src-block))))))
+
 (ert-deftest ob-D/simple-program ()
   "Hello world program."
   (if (executable-find org-babel-D-compiler)
       (org-test-at-id "fa6db330-e960-4ea2-ac67-94bb845b8577"
-		      (org-babel-next-src-block 2)
+		      (org-babel-next-src-block 3)
 		      (should (= 42 (org-babel-execute-src-block))))))
 
 (ert-deftest ob-C/integer-var ()
   "Test of an integer variable."
   (if (executable-find org-babel-C++-compiler)
       (org-test-at-id "fa6db330-e960-4ea2-ac67-94bb845b8577"
-		      (org-babel-next-src-block 3)
+		      (org-babel-next-src-block 4)
 		      (should (= 12 (org-babel-execute-src-block))))))
 
 (ert-deftest ob-D/integer-var ()
   "Test of an integer variable."
   (if (executable-find org-babel-D-compiler)
       (org-test-at-id "fa6db330-e960-4ea2-ac67-94bb845b8577"
-		      (org-babel-next-src-block 4)
+		      (org-babel-next-src-block 5)
 		      (should (= 12 (org-babel-execute-src-block))))))
 
 (ert-deftest ob-C/two-integer-var ()
   "Test of two input variables"
   (if (executable-find org-babel-C++-compiler)
       (org-test-at-id "fa6db330-e960-4ea2-ac67-94bb845b8577"
-		      (org-babel-next-src-block 5)
+		      (org-babel-next-src-block 6)
 		      (should (= 22 (org-babel-execute-src-block))))))
 
 (ert-deftest ob-D/two-integer-var ()
   "Test of two input variables"
   (if (executable-find org-babel-D-compiler)
       (org-test-at-id "fa6db330-e960-4ea2-ac67-94bb845b8577"
-		      (org-babel-next-src-block 6)
+		      (org-babel-next-src-block 7)
 		      (should (= 22 (org-babel-execute-src-block))))))
 
 (ert-deftest ob-C/string-var ()
   "Test of a string input variable"
   (if (executable-find org-babel-C++-compiler)
       (org-test-at-id "fa6db330-e960-4ea2-ac67-94bb845b8577"
-		      (org-babel-next-src-block 7)
+		      (org-babel-next-src-block 8)
 		      (should (equal "word 4" (org-babel-execute-src-block))))))
 
 (ert-deftest ob-D/string-var ()
   "Test of a string input variable"
   (if (executable-find org-babel-D-compiler)
       (org-test-at-id "fa6db330-e960-4ea2-ac67-94bb845b8577"
-		      (org-babel-next-src-block 8)
+		      (org-babel-next-src-block 9)
 		      (should (equal "word 4" (org-babel-execute-src-block))))))
 
 (ert-deftest ob-C/preprocessor ()
   "Test of a string variable"
   (if (executable-find org-babel-C++-compiler)
       (org-test-at-id "fa6db330-e960-4ea2-ac67-94bb845b8577"
-		      (org-babel-next-src-block 9)
+		      (org-babel-next-src-block 10)
 		      (should (= 42 (org-babel-execute-src-block))))))
 
 (ert-deftest ob-C/table ()
-- 
2.17.1


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


(Is the Org source for the C/C++/D Babel syntax[1] committed somewhere,
BTW?  I could not find it in the Org repo.)


I'd like to say that all tests pass now, but I'm getting failures on
master (even without my changes) for two other tests:

>   FAILED  ob-tangle/jump-to-org
>   FAILED  test-org-attach/dir


[1] https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-C.html

Re: Possible fix for :includes header argument in org-babel C source blocks

[Kyle Meyer]

Kévin Le Gouguec writes:

> Here is a patch to fix the regression:

Thanks, applied (6506ea1).

> And here is a patch to add a test for the unquoted-single-header case,
> since otherwise it's hard to tell whether this behaviour is intentional:

Looks good to me.  Thanks for taking the time to add that.  Applied
(e9163591a).

> (Is the Org source for the C/C++/D Babel syntax[1] committed somewhere,
> BTW?  I could not find it in the Org repo.)
> <shuffled quote> 
> [1] https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-C.html

The source for that page is in the worg repo:
https://code.orgmode.org/bzg/worg/src/master/org-contrib/babel/languages/ob-doc-C.org

> I'd like to say that all tests pass now, but I'm getting failures on
> master (even without my changes) for two other tests:
>
>>   FAILED  ob-tangle/jump-to-org
>>   FAILED  test-org-attach/dir

:(

After your first patch, all tests now pass on my end.  Would you mind
posting the details of those failures in a new thread?

Failing tests (was: Possible fix for :includes header argument in org-babel C source blocks)

[Kévin Le Gouguec]

Kyle Meyer <kyle@kyleam.com> writes:

> The source for that page is in the worg repo:
> https://code.orgmode.org/bzg/worg/src/master/org-contrib/babel/languages/ob-doc-C.org

Thanks for the pointer, and for applying the patches!

>>>   FAILED  ob-tangle/jump-to-org
>>>   FAILED  test-org-attach/dir
>
> :(
>
> After your first patch, all tests now pass on my end.  Would you mind
> posting the details of those failures in a new thread?

Mmm, on further inspection, those tests fail on one of my setups but
pass on another.

I think I've narrowed this down to org-open-file running "less
examples/att1/fileA" instead of visiting this file.

The following snippet returns "less '%s'" on the failing setup, and nil
on the passing one:

#+begin_src elisp
(mailcap-mime-info (mailcap-extension-to-mime ""))
#+end_src

IIUC this comes from /etc/mailcap; the failing setup (Xubuntu 18.04) has
an entry saying "less '%s'" for "text/plain"; the passing setup
(OpenSUSE Tumbleweed) simply has no /etc/mailcap.  mailcap-mime-info
falls back to "text/plain" when mailcap-extension-to-mime returns nil.

Let-binding org-file-apps to '(("." . emacs)) makes the tests pass, but
I don't know if that's the way we want to solve this.

Re: Failing tests (was: Possible fix for :includes header argument in org-babel C source blocks)

[Kyle Meyer]

Kévin Le Gouguec writes:

> Kyle Meyer <kyle@kyleam.com> writes:
>
>> The source for that page is in the worg repo:
>> https://code.orgmode.org/bzg/worg/src/master/org-contrib/babel/languages/ob-doc-C.org
>
> Thanks for the pointer, and for applying the patches!
>
>>>>   FAILED  ob-tangle/jump-to-org
>>>>   FAILED  test-org-attach/dir
>>
>> :(
>>
>> After your first patch, all tests now pass on my end.  Would you mind
>> posting the details of those failures in a new thread?
>
> Mmm, on further inspection, those tests fail on one of my setups but
> pass on another.
>
> I think I've narrowed this down to org-open-file running "less
> examples/att1/fileA" instead of visiting this file.
[...]
> Let-binding org-file-apps to '(("." . emacs)) makes the tests pass, but
> I don't know if that's the way we want to solve this.

Thanks for looking into the failures.  Let-binding org-file-apps sounds
like a good approach to me.  Rather than the catch-all regular
expression, I believe the value could be ((t . emacs)).

Re: Failing tests

[Kévin Le Gouguec]

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

Kyle Meyer <kyle@kyleam.com> writes:

>> I think I've narrowed this down to org-open-file running "less
>> examples/att1/fileA" instead of visiting this file.
> [...]
>> Let-binding org-file-apps to '(("." . emacs)) makes the tests pass, but
>> I don't know if that's the way we want to solve this.
>
> Thanks for looking into the failures.  Let-binding org-file-apps sounds
> like a good approach to me.  Rather than the catch-all regular
> expression, I believe the value could be ((t . emacs)).

Absolutely.  I've attached a patch to that effect.

I wonder though, shouldn't org-open-file always visit text/plain files?
Why would we ever want to send those to an external viewer?

I think this would need special-casing inside org-open-file, since I
don't see a way to catch all text/plain files with org-file-apps.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-tests-robust-with-respect-to-mailcap-entries.patch --]
[-- Type: text/x-diff, Size: 13127 bytes --]

From 05a71740c662fcde3fcfad7c07975052781ec589 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Mon, 1 Jun 2020 16:07:44 +0200
Subject: [PATCH] Make tests robust with respect to mailcap entries

When /etc/mailcap specifies a program for text/plain
files (e.g. less(1)), org-open-file will run this program instead of
visiting the file.  This throws off some tests which expect
extension-less temporary files to be visited.

* testing/lisp/test-ob-tangle.el (ob-tangle/jump-to-org):
* testing/lisp/test-org-attach.el (test-org-attach/dir): Rig
org-file-apps so that temporary files are visited inside Emacs.
---
 testing/lisp/test-ob-tangle.el  | 121 +++++++++++++-------------
 testing/lisp/test-org-attach.el | 147 ++++++++++++++++----------------
 2 files changed, 135 insertions(+), 133 deletions(-)

diff --git a/testing/lisp/test-ob-tangle.el b/testing/lisp/test-ob-tangle.el
index 2283037fc..35490f538 100644
--- a/testing/lisp/test-ob-tangle.el
+++ b/testing/lisp/test-ob-tangle.el
@@ -125,23 +125,24 @@ echo 1
 (ert-deftest ob-tangle/jump-to-org ()
   "Test `org-babel-tangle-jump-to-org' specifications."
   ;; Standard test.
-  (should
-   (equal
-    "* H\n#+begin_src emacs-lisp\n1\n#+end_src"
-    (org-test-with-temp-text-in-file
-        "* H\n#+begin_src emacs-lisp\n1\n#+end_src"
-      (let ((file (buffer-file-name)))
-        (org-test-with-temp-text
-            (format ";; [[file:%s][H:1]]\n<point>1\n;; H:1 ends here\n"
-                    (file-name-nondirectory file))
-          (org-babel-tangle-jump-to-org)
-          (buffer-string))))))
-  ;; Multiple blocks in the same section.
-  (should
-   (equal
-    "2"
-    (org-test-with-temp-text-in-file
-        "* H
+  (let ((org-file-apps '((t . emacs))))
+    (should
+     (equal
+      "* H\n#+begin_src emacs-lisp\n1\n#+end_src"
+      (org-test-with-temp-text-in-file
+          "* H\n#+begin_src emacs-lisp\n1\n#+end_src"
+	(let ((file (buffer-file-name)))
+          (org-test-with-temp-text
+              (format ";; [[file:%s][H:1]]\n<point>1\n;; H:1 ends here\n"
+                      (file-name-nondirectory file))
+            (org-babel-tangle-jump-to-org)
+            (buffer-string))))))
+    ;; Multiple blocks in the same section.
+    (should
+     (equal
+      "2"
+      (org-test-with-temp-text-in-file
+          "* H
 
 first block
 
@@ -155,49 +156,49 @@ another block
 2
 #+end_src
 "
-      (let ((file (buffer-file-name)))
-        (org-test-with-temp-text
-            (format ";; [[file:%s][H:2]]\n<point>2\n;; H:2 ends here\n"
-                    (file-name-nondirectory file))
-          (org-babel-tangle-jump-to-org)
-          (buffer-substring (line-beginning-position)
-                            (line-end-position)))))))
-  ;; Preserve position within the source code.
-  (should
-   (equal
-    "1)"
-    (org-test-with-temp-text-in-file
-        "* H\n#+begin_src emacs-lisp\n(+ 1 1)\n#+end_src"
-      (let ((file (buffer-file-name)))
-        (org-test-with-temp-text
-            (format ";; [[file:%s][H:1]]\n(+ 1 <point>1)\n;; H:1 ends here\n"
-                    (file-name-nondirectory file))
-          (org-babel-tangle-jump-to-org)
-          (buffer-substring-no-properties (point) (line-end-position)))))))
-  ;; Blocks before first heading.
-  (should
-   (equal
-    "Buffer start\n#+begin_src emacs-lisp\n1\n#+end_src\n* H"
-    (org-test-with-temp-text-in-file
-        "Buffer start\n#+begin_src emacs-lisp\n1\n#+end_src\n* H"
-      (let ((file (buffer-file-name)))
-        (org-test-with-temp-text
-            (format ";; [[file:%s][H:1]]\n<point>1\n;; H:1 ends here\n"
-                    (file-name-nondirectory file))
-          (org-babel-tangle-jump-to-org)
-          (buffer-string))))))
-  ;; Special case: buffer starts with a source block.
-  (should
-   (equal
-    "#+begin_src emacs-lisp\n1\n#+end_src\n* H"
-    (org-test-with-temp-text-in-file
-        "#+begin_src emacs-lisp\n1\n#+end_src\n* H"
-      (let ((file (buffer-file-name)))
-        (org-test-with-temp-text
-            (format ";; [[file:%s][H:1]]\n<point>1\n;; H:1 ends here\n"
-                    (file-name-nondirectory file))
-          (org-babel-tangle-jump-to-org)
-          (buffer-string)))))))
+	(let ((file (buffer-file-name)))
+          (org-test-with-temp-text
+              (format ";; [[file:%s][H:2]]\n<point>2\n;; H:2 ends here\n"
+                      (file-name-nondirectory file))
+            (org-babel-tangle-jump-to-org)
+            (buffer-substring (line-beginning-position)
+                              (line-end-position)))))))
+    ;; Preserve position within the source code.
+    (should
+     (equal
+      "1)"
+      (org-test-with-temp-text-in-file
+          "* H\n#+begin_src emacs-lisp\n(+ 1 1)\n#+end_src"
+	(let ((file (buffer-file-name)))
+          (org-test-with-temp-text
+              (format ";; [[file:%s][H:1]]\n(+ 1 <point>1)\n;; H:1 ends here\n"
+                      (file-name-nondirectory file))
+            (org-babel-tangle-jump-to-org)
+            (buffer-substring-no-properties (point) (line-end-position)))))))
+    ;; Blocks before first heading.
+    (should
+     (equal
+      "Buffer start\n#+begin_src emacs-lisp\n1\n#+end_src\n* H"
+      (org-test-with-temp-text-in-file
+          "Buffer start\n#+begin_src emacs-lisp\n1\n#+end_src\n* H"
+	(let ((file (buffer-file-name)))
+          (org-test-with-temp-text
+              (format ";; [[file:%s][H:1]]\n<point>1\n;; H:1 ends here\n"
+                      (file-name-nondirectory file))
+            (org-babel-tangle-jump-to-org)
+            (buffer-string))))))
+    ;; Special case: buffer starts with a source block.
+    (should
+     (equal
+      "#+begin_src emacs-lisp\n1\n#+end_src\n* H"
+      (org-test-with-temp-text-in-file
+          "#+begin_src emacs-lisp\n1\n#+end_src\n* H"
+	(let ((file (buffer-file-name)))
+          (org-test-with-temp-text
+              (format ";; [[file:%s][H:1]]\n<point>1\n;; H:1 ends here\n"
+                      (file-name-nondirectory file))
+            (org-babel-tangle-jump-to-org)
+            (buffer-string))))))))
 
 (ert-deftest ob-tangle/nested-block ()
   "Test tangling of org file with nested block."
diff --git a/testing/lisp/test-org-attach.el b/testing/lisp/test-org-attach.el
index f910526c2..168e5d56f 100644
--- a/testing/lisp/test-org-attach.el
+++ b/testing/lisp/test-org-attach.el
@@ -30,80 +30,81 @@
 
 (ert-deftest test-org-attach/dir ()
   "Test `org-attach-get' specifications."
-  (should (equal "Text in fileA\n"
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 157) ;; First attachment link
-		   (org-open-at-point)
-		   (buffer-string))))
-  (should-not (equal "Text in fileB\n"
-		     (org-test-in-example-file org-test-attachments-file
-		       (goto-char 219) ;; Second attachment link
-		       (let ((org-attach-use-inheritance nil))
-			 (org-open-at-point)
-			 (buffer-string)))))
-  (should (equal "Text in fileB\n"
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 219) ;; Second attachment link
-		   (let ((org-attach-use-inheritance t))
+  (let ((org-file-apps '((t . emacs))))
+    (should (equal "Text in fileA\n"
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 157) ;; First attachment link
 		     (org-open-at-point)
-		     (buffer-string)))))
-  (should-not (equal "att1"
-		     (org-test-in-example-file org-test-attachments-file
-		       (goto-char 179) ;; H1.1
-		       (let ((org-attach-use-inheritance nil))
-			 (org-attach-dir)))))
-  (should (equal "att1"
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 179) ;; H1.1
-		   (let ((org-attach-use-inheritance t))
-		     (org-attach-dir)))))
-  (should (equal '("fileC" "fileD")
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 239) ;; H1.2
-		   (org-attach-file-list (org-attach-dir)))))
-  (should (equal '("fileC" "fileD")
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 239) ;; H1.2
-		   (org-attach-file-list (org-attach-dir)))))
-  (should (equal '("fileE")
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 289) ;; H2
-		   (let ((org-attach-id-dir "data/"))
-		     (org-attach-file-list (org-attach-dir))))))
-  (should (equal "peek-a-boo\n"
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 289) ;; H2
-		   (let ((org-attach-id-dir "data/"))
-		     (org-attach-open-in-emacs)
-		     (buffer-string)))))
-  (should (equal  '("fileA" "fileB")
-		  (org-test-in-example-file org-test-attachments-file
-		    (goto-char 336) ;; H3
-		    (org-attach-file-list (org-attach-dir)))))
-  ;; Test for folder not initialized in the filesystem
-  (should-not (org-test-in-example-file org-test-attachments-file
-		(goto-char 401) ;; H3.1
-		(let ((org-attach-use-inheritance nil)
-		      (org-attach-id-dir "data/"))
-		  (org-attach-dir))))
-  ;; Not yet initialized folder should be found if no-fs-check is
-  ;; non-nil
-  (should (equal "data/ab/cd12345"
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 401) ;; H3.1
-		   (let ((org-attach-use-inheritance nil)
-			 (org-attach-id-dir "data/"))
-		     (file-relative-name (org-attach-dir nil t))))))
-  (should (equal '("fileA" "fileB")
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 401) ;; H3.1
-		   (let ((org-attach-use-inheritance t))
-		     ;; This is where it gets a bit sketchy...! DIR always has
-		     ;; priority over ID, even if ID is declared "higher up" in the
-		     ;; tree.  This can potentially be revised.  But it is also
-		     ;; pretty clean.  DIR is always higher in priority than ID right
-		     ;; now, no matter the depth in the tree.
-		     (org-attach-file-list (org-attach-dir)))))))
+		     (buffer-string))))
+    (should-not (equal "Text in fileB\n"
+		       (org-test-in-example-file org-test-attachments-file
+			 (goto-char 219) ;; Second attachment link
+			 (let ((org-attach-use-inheritance nil))
+			   (org-open-at-point)
+			   (buffer-string)))))
+    (should (equal "Text in fileB\n"
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 219) ;; Second attachment link
+		     (let ((org-attach-use-inheritance t))
+		       (org-open-at-point)
+		       (buffer-string)))))
+    (should-not (equal "att1"
+		       (org-test-in-example-file org-test-attachments-file
+			 (goto-char 179) ;; H1.1
+			 (let ((org-attach-use-inheritance nil))
+			   (org-attach-dir)))))
+    (should (equal "att1"
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 179) ;; H1.1
+		     (let ((org-attach-use-inheritance t))
+		       (org-attach-dir)))))
+    (should (equal '("fileC" "fileD")
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 239) ;; H1.2
+		     (org-attach-file-list (org-attach-dir)))))
+    (should (equal '("fileC" "fileD")
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 239) ;; H1.2
+		     (org-attach-file-list (org-attach-dir)))))
+    (should (equal '("fileE")
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 289) ;; H2
+		     (let ((org-attach-id-dir "data/"))
+		       (org-attach-file-list (org-attach-dir))))))
+    (should (equal "peek-a-boo\n"
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 289) ;; H2
+		     (let ((org-attach-id-dir "data/"))
+		       (org-attach-open-in-emacs)
+		       (buffer-string)))))
+    (should (equal  '("fileA" "fileB")
+		    (org-test-in-example-file org-test-attachments-file
+		      (goto-char 336) ;; H3
+		      (org-attach-file-list (org-attach-dir)))))
+    ;; Test for folder not initialized in the filesystem
+    (should-not (org-test-in-example-file org-test-attachments-file
+		  (goto-char 401) ;; H3.1
+		  (let ((org-attach-use-inheritance nil)
+			(org-attach-id-dir "data/"))
+		    (org-attach-dir))))
+    ;; Not yet initialized folder should be found if no-fs-check is
+    ;; non-nil
+    (should (equal "data/ab/cd12345"
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 401) ;; H3.1
+		     (let ((org-attach-use-inheritance nil)
+			   (org-attach-id-dir "data/"))
+		       (file-relative-name (org-attach-dir nil t))))))
+    (should (equal '("fileA" "fileB")
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 401) ;; H3.1
+		     (let ((org-attach-use-inheritance t))
+		       ;; This is where it gets a bit sketchy...! DIR always has
+		       ;; priority over ID, even if ID is declared "higher up" in the
+		       ;; tree.  This can potentially be revised.  But it is also
+		       ;; pretty clean.  DIR is always higher in priority than ID right
+		       ;; now, no matter the depth in the tree.
+		       (org-attach-file-list (org-attach-dir))))))))
 
 (ert-deftest test-org-attach/dired-attach-to-next-best-subtree/1 ()
   "Attach file at point in dired to subtree."
-- 
2.17.1


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


The diff looks big, but most of it is just re-indentation.  Here is the
signal beneath the whitespace noise:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: clean.diff --]
[-- Type: text/x-diff, Size: 1687 bytes --]

diff --git a/testing/lisp/test-ob-tangle.el b/testing/lisp/test-ob-tangle.el
index 2283037fc..35490f538 100644
--- a/testing/lisp/test-ob-tangle.el
+++ b/testing/lisp/test-ob-tangle.el
@@ -125,6 +125,7 @@
 (ert-deftest ob-tangle/jump-to-org ()
   "Test `org-babel-tangle-jump-to-org' specifications."
   ;; Standard test.
+  (let ((org-file-apps '((t . emacs))))
   (should
    (equal
     "* H\n#+begin_src emacs-lisp\n1\n#+end_src"
@@ -197,7 +197,7 @@
             (format ";; [[file:%s][H:1]]\n<point>1\n;; H:1 ends here\n"
                     (file-name-nondirectory file))
           (org-babel-tangle-jump-to-org)
-          (buffer-string)))))))
+            (buffer-string))))))))
 
 (ert-deftest ob-tangle/nested-block ()
   "Test tangling of org file with nested block."
diff --git a/testing/lisp/test-org-attach.el b/testing/lisp/test-org-attach.el
index f910526c2..168e5d56f 100644
--- a/testing/lisp/test-org-attach.el
+++ b/testing/lisp/test-org-attach.el
@@ -30,6 +30,7 @@
 
 (ert-deftest test-org-attach/dir ()
   "Test `org-attach-get' specifications."
+  (let ((org-file-apps '((t . emacs))))
   (should (equal "Text in fileA\n"
 		 (org-test-in-example-file org-test-attachments-file
 		   (goto-char 157) ;; First attachment link
@@ -103,7 +104,7 @@
 		     ;; tree.  This can potentially be revised.  But it is also
 		     ;; pretty clean.  DIR is always higher in priority than ID right
 		     ;; now, no matter the depth in the tree.
-		     (org-attach-file-list (org-attach-dir)))))))
+		       (org-attach-file-list (org-attach-dir))))))))
 
 (ert-deftest test-org-attach/dired-attach-to-next-best-subtree/1 ()
   "Attach file at point in dired to subtree."

Re: Failing tests

[Kévin Le Gouguec]

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

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> Absolutely.  I've attached a patch to that effect.

I just realized that these let-bindings probably deserved explanatory
comments.  Here is an updated patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-tests-robust-with-respect-to-mailcap-entries.patch --]
[-- Type: text/x-diff, Size: 13274 bytes --]

From f996ec3a10a845abae2fa463ab0ea7a761af1707 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Mon, 1 Jun 2020 16:07:44 +0200
Subject: [PATCH] Make tests robust with respect to mailcap entries

When /etc/mailcap specifies a program for text/plain
files (e.g. less(1)), org-open-file will run this program instead of
visiting the file.  This throws off some tests which expect
extension-less temporary files to be visited.

* testing/lisp/test-ob-tangle.el (ob-tangle/jump-to-org):
* testing/lisp/test-org-attach.el (test-org-attach/dir): Rig
org-file-apps so that temporary files are visited inside Emacs.
---
 testing/lisp/test-ob-tangle.el  | 124 +++++++++++++-------------
 testing/lisp/test-org-attach.el | 148 ++++++++++++++++----------------
 2 files changed, 138 insertions(+), 134 deletions(-)

diff --git a/testing/lisp/test-ob-tangle.el b/testing/lisp/test-ob-tangle.el
index 2283037fc..7b1f617ed 100644
--- a/testing/lisp/test-ob-tangle.el
+++ b/testing/lisp/test-ob-tangle.el
@@ -124,24 +124,26 @@ echo 1
 
 (ert-deftest ob-tangle/jump-to-org ()
   "Test `org-babel-tangle-jump-to-org' specifications."
-  ;; Standard test.
-  (should
-   (equal
-    "* H\n#+begin_src emacs-lisp\n1\n#+end_src"
-    (org-test-with-temp-text-in-file
-        "* H\n#+begin_src emacs-lisp\n1\n#+end_src"
-      (let ((file (buffer-file-name)))
-        (org-test-with-temp-text
-            (format ";; [[file:%s][H:1]]\n<point>1\n;; H:1 ends here\n"
-                    (file-name-nondirectory file))
-          (org-babel-tangle-jump-to-org)
-          (buffer-string))))))
-  ;; Multiple blocks in the same section.
-  (should
-   (equal
-    "2"
-    (org-test-with-temp-text-in-file
-        "* H
+  ;; Make sure temporary files will be visited inside Emacs.
+  (let ((org-file-apps '((t . emacs))))
+    ;; Standard test.
+    (should
+     (equal
+      "* H\n#+begin_src emacs-lisp\n1\n#+end_src"
+      (org-test-with-temp-text-in-file
+          "* H\n#+begin_src emacs-lisp\n1\n#+end_src"
+	(let ((file (buffer-file-name)))
+          (org-test-with-temp-text
+              (format ";; [[file:%s][H:1]]\n<point>1\n;; H:1 ends here\n"
+                      (file-name-nondirectory file))
+            (org-babel-tangle-jump-to-org)
+            (buffer-string))))))
+    ;; Multiple blocks in the same section.
+    (should
+     (equal
+      "2"
+      (org-test-with-temp-text-in-file
+          "* H
 
 first block
 
@@ -155,49 +157,49 @@ another block
 2
 #+end_src
 "
-      (let ((file (buffer-file-name)))
-        (org-test-with-temp-text
-            (format ";; [[file:%s][H:2]]\n<point>2\n;; H:2 ends here\n"
-                    (file-name-nondirectory file))
-          (org-babel-tangle-jump-to-org)
-          (buffer-substring (line-beginning-position)
-                            (line-end-position)))))))
-  ;; Preserve position within the source code.
-  (should
-   (equal
-    "1)"
-    (org-test-with-temp-text-in-file
-        "* H\n#+begin_src emacs-lisp\n(+ 1 1)\n#+end_src"
-      (let ((file (buffer-file-name)))
-        (org-test-with-temp-text
-            (format ";; [[file:%s][H:1]]\n(+ 1 <point>1)\n;; H:1 ends here\n"
-                    (file-name-nondirectory file))
-          (org-babel-tangle-jump-to-org)
-          (buffer-substring-no-properties (point) (line-end-position)))))))
-  ;; Blocks before first heading.
-  (should
-   (equal
-    "Buffer start\n#+begin_src emacs-lisp\n1\n#+end_src\n* H"
-    (org-test-with-temp-text-in-file
-        "Buffer start\n#+begin_src emacs-lisp\n1\n#+end_src\n* H"
-      (let ((file (buffer-file-name)))
-        (org-test-with-temp-text
-            (format ";; [[file:%s][H:1]]\n<point>1\n;; H:1 ends here\n"
-                    (file-name-nondirectory file))
-          (org-babel-tangle-jump-to-org)
-          (buffer-string))))))
-  ;; Special case: buffer starts with a source block.
-  (should
-   (equal
-    "#+begin_src emacs-lisp\n1\n#+end_src\n* H"
-    (org-test-with-temp-text-in-file
-        "#+begin_src emacs-lisp\n1\n#+end_src\n* H"
-      (let ((file (buffer-file-name)))
-        (org-test-with-temp-text
-            (format ";; [[file:%s][H:1]]\n<point>1\n;; H:1 ends here\n"
-                    (file-name-nondirectory file))
-          (org-babel-tangle-jump-to-org)
-          (buffer-string)))))))
+	(let ((file (buffer-file-name)))
+          (org-test-with-temp-text
+              (format ";; [[file:%s][H:2]]\n<point>2\n;; H:2 ends here\n"
+                      (file-name-nondirectory file))
+            (org-babel-tangle-jump-to-org)
+            (buffer-substring (line-beginning-position)
+                              (line-end-position)))))))
+    ;; Preserve position within the source code.
+    (should
+     (equal
+      "1)"
+      (org-test-with-temp-text-in-file
+          "* H\n#+begin_src emacs-lisp\n(+ 1 1)\n#+end_src"
+	(let ((file (buffer-file-name)))
+          (org-test-with-temp-text
+              (format ";; [[file:%s][H:1]]\n(+ 1 <point>1)\n;; H:1 ends here\n"
+                      (file-name-nondirectory file))
+            (org-babel-tangle-jump-to-org)
+            (buffer-substring-no-properties (point) (line-end-position)))))))
+    ;; Blocks before first heading.
+    (should
+     (equal
+      "Buffer start\n#+begin_src emacs-lisp\n1\n#+end_src\n* H"
+      (org-test-with-temp-text-in-file
+          "Buffer start\n#+begin_src emacs-lisp\n1\n#+end_src\n* H"
+	(let ((file (buffer-file-name)))
+          (org-test-with-temp-text
+              (format ";; [[file:%s][H:1]]\n<point>1\n;; H:1 ends here\n"
+                      (file-name-nondirectory file))
+            (org-babel-tangle-jump-to-org)
+            (buffer-string))))))
+    ;; Special case: buffer starts with a source block.
+    (should
+     (equal
+      "#+begin_src emacs-lisp\n1\n#+end_src\n* H"
+      (org-test-with-temp-text-in-file
+          "#+begin_src emacs-lisp\n1\n#+end_src\n* H"
+	(let ((file (buffer-file-name)))
+          (org-test-with-temp-text
+              (format ";; [[file:%s][H:1]]\n<point>1\n;; H:1 ends here\n"
+                      (file-name-nondirectory file))
+            (org-babel-tangle-jump-to-org)
+            (buffer-string))))))))
 
 (ert-deftest ob-tangle/nested-block ()
   "Test tangling of org file with nested block."
diff --git a/testing/lisp/test-org-attach.el b/testing/lisp/test-org-attach.el
index f910526c2..cdd1afb3d 100644
--- a/testing/lisp/test-org-attach.el
+++ b/testing/lisp/test-org-attach.el
@@ -30,80 +30,82 @@
 
 (ert-deftest test-org-attach/dir ()
   "Test `org-attach-get' specifications."
-  (should (equal "Text in fileA\n"
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 157) ;; First attachment link
-		   (org-open-at-point)
-		   (buffer-string))))
-  (should-not (equal "Text in fileB\n"
-		     (org-test-in-example-file org-test-attachments-file
-		       (goto-char 219) ;; Second attachment link
-		       (let ((org-attach-use-inheritance nil))
-			 (org-open-at-point)
-			 (buffer-string)))))
-  (should (equal "Text in fileB\n"
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 219) ;; Second attachment link
-		   (let ((org-attach-use-inheritance t))
+  ;; Make sure example files will be visited inside Emacs.
+  (let ((org-file-apps '((t . emacs))))
+    (should (equal "Text in fileA\n"
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 157) ;; First attachment link
 		     (org-open-at-point)
-		     (buffer-string)))))
-  (should-not (equal "att1"
-		     (org-test-in-example-file org-test-attachments-file
-		       (goto-char 179) ;; H1.1
-		       (let ((org-attach-use-inheritance nil))
-			 (org-attach-dir)))))
-  (should (equal "att1"
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 179) ;; H1.1
-		   (let ((org-attach-use-inheritance t))
-		     (org-attach-dir)))))
-  (should (equal '("fileC" "fileD")
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 239) ;; H1.2
-		   (org-attach-file-list (org-attach-dir)))))
-  (should (equal '("fileC" "fileD")
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 239) ;; H1.2
-		   (org-attach-file-list (org-attach-dir)))))
-  (should (equal '("fileE")
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 289) ;; H2
-		   (let ((org-attach-id-dir "data/"))
-		     (org-attach-file-list (org-attach-dir))))))
-  (should (equal "peek-a-boo\n"
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 289) ;; H2
-		   (let ((org-attach-id-dir "data/"))
-		     (org-attach-open-in-emacs)
-		     (buffer-string)))))
-  (should (equal  '("fileA" "fileB")
-		  (org-test-in-example-file org-test-attachments-file
-		    (goto-char 336) ;; H3
-		    (org-attach-file-list (org-attach-dir)))))
-  ;; Test for folder not initialized in the filesystem
-  (should-not (org-test-in-example-file org-test-attachments-file
-		(goto-char 401) ;; H3.1
-		(let ((org-attach-use-inheritance nil)
-		      (org-attach-id-dir "data/"))
-		  (org-attach-dir))))
-  ;; Not yet initialized folder should be found if no-fs-check is
-  ;; non-nil
-  (should (equal "data/ab/cd12345"
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 401) ;; H3.1
-		   (let ((org-attach-use-inheritance nil)
-			 (org-attach-id-dir "data/"))
-		     (file-relative-name (org-attach-dir nil t))))))
-  (should (equal '("fileA" "fileB")
-		 (org-test-in-example-file org-test-attachments-file
-		   (goto-char 401) ;; H3.1
-		   (let ((org-attach-use-inheritance t))
-		     ;; This is where it gets a bit sketchy...! DIR always has
-		     ;; priority over ID, even if ID is declared "higher up" in the
-		     ;; tree.  This can potentially be revised.  But it is also
-		     ;; pretty clean.  DIR is always higher in priority than ID right
-		     ;; now, no matter the depth in the tree.
-		     (org-attach-file-list (org-attach-dir)))))))
+		     (buffer-string))))
+    (should-not (equal "Text in fileB\n"
+		       (org-test-in-example-file org-test-attachments-file
+			 (goto-char 219) ;; Second attachment link
+			 (let ((org-attach-use-inheritance nil))
+			   (org-open-at-point)
+			   (buffer-string)))))
+    (should (equal "Text in fileB\n"
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 219) ;; Second attachment link
+		     (let ((org-attach-use-inheritance t))
+		       (org-open-at-point)
+		       (buffer-string)))))
+    (should-not (equal "att1"
+		       (org-test-in-example-file org-test-attachments-file
+			 (goto-char 179) ;; H1.1
+			 (let ((org-attach-use-inheritance nil))
+			   (org-attach-dir)))))
+    (should (equal "att1"
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 179) ;; H1.1
+		     (let ((org-attach-use-inheritance t))
+		       (org-attach-dir)))))
+    (should (equal '("fileC" "fileD")
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 239) ;; H1.2
+		     (org-attach-file-list (org-attach-dir)))))
+    (should (equal '("fileC" "fileD")
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 239) ;; H1.2
+		     (org-attach-file-list (org-attach-dir)))))
+    (should (equal '("fileE")
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 289) ;; H2
+		     (let ((org-attach-id-dir "data/"))
+		       (org-attach-file-list (org-attach-dir))))))
+    (should (equal "peek-a-boo\n"
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 289) ;; H2
+		     (let ((org-attach-id-dir "data/"))
+		       (org-attach-open-in-emacs)
+		       (buffer-string)))))
+    (should (equal  '("fileA" "fileB")
+		    (org-test-in-example-file org-test-attachments-file
+		      (goto-char 336) ;; H3
+		      (org-attach-file-list (org-attach-dir)))))
+    ;; Test for folder not initialized in the filesystem
+    (should-not (org-test-in-example-file org-test-attachments-file
+		  (goto-char 401) ;; H3.1
+		  (let ((org-attach-use-inheritance nil)
+			(org-attach-id-dir "data/"))
+		    (org-attach-dir))))
+    ;; Not yet initialized folder should be found if no-fs-check is
+    ;; non-nil
+    (should (equal "data/ab/cd12345"
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 401) ;; H3.1
+		     (let ((org-attach-use-inheritance nil)
+			   (org-attach-id-dir "data/"))
+		       (file-relative-name (org-attach-dir nil t))))))
+    (should (equal '("fileA" "fileB")
+		   (org-test-in-example-file org-test-attachments-file
+		     (goto-char 401) ;; H3.1
+		     (let ((org-attach-use-inheritance t))
+		       ;; This is where it gets a bit sketchy...! DIR always has
+		       ;; priority over ID, even if ID is declared "higher up" in the
+		       ;; tree.  This can potentially be revised.  But it is also
+		       ;; pretty clean.  DIR is always higher in priority than ID right
+		       ;; now, no matter the depth in the tree.
+		       (org-attach-file-list (org-attach-dir))))))))
 
 (ert-deftest test-org-attach/dired-attach-to-next-best-subtree/1 ()
   "Attach file at point in dired to subtree."
-- 
2.17.1

Re: Failing tests

[Kyle Meyer]

Kévin Le Gouguec writes:

> Kyle Meyer <kyle@kyleam.com> writes:
>
>>> I think I've narrowed this down to org-open-file running "less
>>> examples/att1/fileA" instead of visiting this file.
>> [...]
>>> Let-binding org-file-apps to '(("." . emacs)) makes the tests pass, but
>>> I don't know if that's the way we want to solve this.
>>
>> Thanks for looking into the failures.  Let-binding org-file-apps sounds
>> like a good approach to me.  Rather than the catch-all regular
>> expression, I believe the value could be ((t . emacs)).
>
> Absolutely.  I've attached a patch to that effect.

Thanks.  Applied (c8f7e89d7).

> I wonder though, shouldn't org-open-file always visit text/plain files?
> Why would we ever want to send those to an external viewer?
>
> I think this would need special-casing inside org-open-file, since I
> don't see a way to catch all text/plain files with org-file-apps.

Good question.  I'm not sure, though offhand I can't think of cases
where I'd want to send text/plain files to an external viewer.  I'd
guess this doesn't matter much in practice given the default position of

  (auto-mode . emacs)

in org-file-apps, which will catch a good number of text/plain files.

Re: Failing tests

[Bastien]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> Thanks for the pointer, and for applying the patches!
>
>>>>   FAILED  ob-tangle/jump-to-org
>>>>   FAILED  test-org-attach/dir

I have had both tests failing for a while without understanding why,
if this gets fixed as a side-effect of the incomplete fix I made for 
ob-C.el, I'd be very happy!

-- 
 Bastien

Re: Possible fix for :includes header argument in org-babel C source blocks

[Brandon Guttersohn]

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

Hey Kévin,

Apologies for the regression, and thank you for fixing it. I neglected to run the tests before suggesting that fix -- I'll try not to do that again..

I can at least confirm that the patch wasn't intended to change how C-header-files are specified in the org-babel-block-header. The goal was only to change how the headers are formatted in the generated C-language file during execution, and only for headers which were not wrapped in <>'s. Your fix looks right to me.

On 5/28/20 5:09 AM, Kévin Le Gouguec wrote:
> Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:
>
>> That leads me to believe that the coercion was an unintended side-effect
>> of (format …).
> Never mind, the ORG-NEWS entry for 9.1 shows an example of unquoted
> header, so I guess it is intentional.
>
> Here is a patch to fix the regression:
>
>
> And here is a patch to add a test for the unquoted-single-header case,
> since otherwise it's hard to tell whether this behaviour is intentional:
>
>
> (Is the Org source for the C/C++/D Babel syntax[1] committed somewhere,
> BTW?  I could not find it in the Org repo.)
>
>
> I'd like to say that all tests pass now, but I'm getting failures on
> master (even without my changes) for two other tests:
>
>>    FAILED  ob-tangle/jump-to-org
>>    FAILED  test-org-attach/dir
>
> [1] https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-C.html

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

Re: Possible fix for :includes header argument in org-babel C source blocks

[Kévin Le Gouguec]

Brandon Guttersohn <brandon@guttersohn.org> writes:

> Apologies for the regression, and thank you for fixing it. I neglected
> to run the tests before suggesting that fix -- I'll try not to do that
> again..

No biggie, that got me to finally try out Babel ;)

<rant>
I don't know if it's been mentioned in the "issue tracker?" thread, but
if I could pick just *one* feature off web-based forges, it'd be
automated testing with CI…

I find the "make-test-before-commit" discipline easy enough to adhere to
at $DAYJOB; it's not as straightforward when contributing to free
software, when I'm frequently pressed for time, running on battery on a
low-end laptop…

Running a few unit tests is not a big deal, but it's not trivial to
anticipate which ones to run; test-foo.el is rarely enough to catch
regressions introduced by tweaking foo.el.  

Having something (e.g. emba.gnu.org) pick up patches sent to the mailing
list and report new test failures would be very helpful, for
contributors if not for maintainers.
 </rant>

> I can at least confirm that the patch wasn't intended to change how
> C-header-files are specified in the org-babel-block-header. The goal
> was only to change how the headers are formatted in the generated
> C-language file during execution, and only for headers which were not
> wrapped in <>'s.

OK; IIUC, before the patch it was not possible to generate double-quoted
includes short of backslash-escaping the double quotes; that's why I
assumed that the goal of the patch was to make it easier to use
double-quoted includes, which I thought worth advertising in ORG-NEWS.

Re: Possible fix for :includes header argument in org-babel C source blocks

[Brandon Guttersohn]

> <rant>
> I don't know if it's been mentioned in the "issue tracker?" thread, but
> if I could pick just *one* feature off web-based forges, it'd be
> automated testing with CI…
> [...]

Automated testing has been a massive time-saver everywhere I've seen it 
used, though I'm not sure I've ever seen it combined with the 
mailing-list-workflow that GNU and (off the top of my head) Gnome use. I 
guess a bot could just individually cherry-pick patches onto master, and 
it would usually be the correct thing to do?

> OK; IIUC, before the patch it was not possible to generate double-quoted
> includes short of backslash-escaping the double quotes; that's why I
> assumed that the goal of the patch was to make it easier to use
> double-quoted includes, which I thought worth advertising in ORG-NEWS.

Yeah, I believe you understand correctly. Ori found that you could get 
it to work if you escape the double quotes, /and/ place that string 
literal inside a quoted form, but that was more of a happy accident than 
a design choice as far as I can tell. So this patch is sort of a new 
feature, but a trivial one.

Re: Possible fix for :includes header argument in org-babel C source blocks

[Bastien]

Brandon Guttersohn <brandon@guttersohn.org> writes:

> So this patch is sort of a
> new feature, but a trivial one.

Agreed.  Could you or Kevin propose a sentence to advertise this small
enhancement in etc/ORG-NEWS?

-- 
 Bastien

Re: Possible fix for :includes header argument in org-babel C source blocks

[Kévin Le Gouguec]

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

Bastien <bzg@gnu.org> writes:

> Brandon Guttersohn <brandon@guttersohn.org> writes:
>
>> So this patch is sort of a
>> new feature, but a trivial one.
>
> Agreed.  Could you or Kevin propose a sentence to advertise this small
> enhancement in etc/ORG-NEWS?

Here goes nothing.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-etc-ORG-NEWS-Announce-a-recent-fix-in-ob-C.el.patch --]
[-- Type: text/x-patch, Size: 961 bytes --]

From b18f6dc66ea4a05c95a4ee6825723da4beaa1c83 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Mon, 1 Jun 2020 21:33:01 +0200
Subject: [PATCH] * etc/ORG-NEWS: Announce a recent fix in ob-C.el.

---
 etc/ORG-NEWS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index c0df785d4..d3f2bb1ca 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -202,6 +202,12 @@ Org provides a new tool ~org-link-open-as-file~, useful when defining
 new link types similar to "file"-type links.  See docstring for
 details.
 
+*** =ob-C.el= allows you to include non-system header files
+
+In C and C++ blocks, ~:includes~ arguments that do not start with a
+~<~ character will now be formatted as double-quoted ~#include~
+statements.
+
 *** =ob-clojure.el= supports inf-clojure.el and ClojureScript evaluation
 
 You can now set ~(setq org-babel-clojure-backend 'inf-clojure)~ and
-- 
2.26.2


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


Note that IIUC, for non-system includes to work, either

- the filenames must be absolute, or
- the compiler must be given -I arguments through org-babel-C-compiler.

This variable can be set (e.g. to "gcc -I .") with file or
directory-local variables.  Should we promote this method in NEWS?  A
downside is that the user will be warned about the variable's value
being potentially unsafe, and we can't really avoid that unless we throw
a blanket :safe #'stringp on this defcustom.

Re: Possible fix for :includes header argument in org-babel C source blocks

[Brandon Guttersohn]

> Note that IIUC, for non-system includes to work, either
> 
> - the filenames must be absolute, or
> - the compiler must be given -I arguments through org-babel-C-compiler.
> 
> This variable can be set (e.g. to "gcc -I .") with file or
> directory-local variables.  Should we promote this method in NEWS?  A
> downside is that the user will be warned about the variable's value
> being potentially unsafe, and we can't really avoid that unless we throw
> a blanket :safe #'stringp on this defcustom.
> 

Yeah, when I used it, I just used an absolute path. It's not entirely 
intuitive.

Would it be reasonable to automatically add the value of 
(file-name-directory buffer-file-name) to GCC's search path when (1) 
non-system imports are used and (2) buffer-file-name is non-nil? If we 
do, then any header in the same directory as the *.org file should "just 
work".

Seems like it would be safe, and I'm happy to try putting that together 
if there's interest.

Re: Possible fix for :includes header argument in org-babel C source blocks

[Kyle Meyer]

Kévin Le Gouguec writes:

> BTW, does the change from 44cb98fdb deserve an ORG-NEWS entry?

Hmm, my impression from the thread/patch was that it was a plain bug
fix, in which case I think the lack of an entry is fine.

Possible fix for :includes header argument in org-babel C source blocks

[Ori]

Hi Brandon,

Some other interesting test results, the first of which makes me think that string un-escaping could be going too far and perhaps the problem lies elsewhere:

    :includes "example.h\""
results in:
    #includes example.h"

    :includes '("\"/some/path/to/example.h\"" "<example_2.h>")
results in:
    #include "/some/path/to/example.h"
    #include <example_2.h>

That works with just one sting in the list as well:
    :includes '("\"/some/path/to/example.h\"")
results in:
    #include "/some/path/to/example.h"


This is on Emacs 26 with org 9.3.4

Ori

Re: Possible fix for :includes header argument in org-babel C source blocks

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

How does a system do un-escaping?  With RECAPTCHA?

;-}

-- 
Dr Richard Stallman
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)