Re: Possible fix for :includes header argument in org-babel C source blocks
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
Bastien writes: > Brandon Guttersohn 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. >From b18f6dc66ea4a05c95a4ee6825723da4beaa1c83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= 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 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 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
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
Brandon Guttersohn 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 ;) 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. > 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
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 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
Re: Possible fix for :includes header argument in org-babel C source blocks
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.) > > [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?
Re: Possible fix for :includes header argument in org-babel C source blocks
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.
Re: Possible fix for :includes header argument in org-babel C source blocks
Kévin Le Gouguec 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:
>From b68122821a26578470506938c3a358f52f5d7a46 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?=
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!bran...@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 '( ) 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
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:
>From cf1bb27215a46a521bb2f50d16b7dbc7441d81ec Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?=
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 (with no double-quotes around )
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 :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
Re: Possible fix for :includes header argument in org-babel C source blocks
Kyle Meyer 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 writes:
> Bastien writes:
>
>> Brandon Guttersohn 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 )
>>FAILED8/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 "" and "" 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 '( ) :results
> silent
> +#+begin_src cpp :var q="word" :includes '("" "") :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
Hi,
Bastien writes:
> Brandon Guttersohn 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 )
>FAILED8/834 ob-C/string-var (0.004651 sec)
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 "" and "" in ob-C-test.org:
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 '( ) :results silent
+#+begin_src cpp :var q="word" :includes '("" "") :results silent
std::cout << q << ' ' << std::strlen(q);
return 0;
#+end_src
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.
