[-- Attachment #1: Type: text/plain, Size: 1891 bytes --] icalendar is capable of exporting arbitrary diary sexp entries, but it looks like there's a bug in the code. Each conversion function `icalendar--convert-*-to-ical' can return either a dotted pair or a list of such pairs, but the code fails to differentiate the two cases properly. To reproduce: (require 'icalendar) (defun test-diary-sexp (sexp) (message "Testing %S\n" sexp) (let ((file (make-temp-file "export.ics"))) (with-temp-buffer (insert sexp) (icalendar-export-region (point-min) (point-max) file)) (with-current-buffer (get-buffer "*icalendar-errors*") (message "export: %S\n" (with-temp-buffer (insert-file-contents file) (buffer-string))) (message "errors: %S" (buffer-string)))) (terpri)) (defun my-float (&rest args) (apply #'diary-float args)) (let ((icalendar-export-sexp-enumeration-days 366)) (test-diary-sexp "%%(diary-float 7 0 1) First Sunday in July 1") (test-diary-sexp "%%(my-float 7 0 1) First Sunday in July 2")) Exporting the my-float sexp will fail with: "Error in line 0 -- (wrong-type-argument listp First Sunday in July 2): ‘%%(my-float 7 0 1) First Sunday in July 2" because the returned list is confused for a dotted pair. `icalendar-export-sexp-enumeration-days' is set to 366 to guarantee that the sexp event occurs at least once. It looks like there's a different bug (?) where, even if an entry is recognized as an arbitrary diary sexp, if it doesn't produce any events, the converter will go ahead with trying to interpret it in a different way and eventually fail. E.g., lowering the enumeration days to 0 gives: "Error in line 0 -- (error Could not parse date): ‘%%(my-float 7 0 1) First Sunday in July 2’" after exhausting all of the known entry types. Should I file this as a separate bug? Regards, hokomo [-- Attachment #2: 0001-Fix-detecting-dotted-pairs.patch --] [-- Type: text/x-patch, Size: 1083 bytes --] From 691f34a301359893bcffff8e4e0c8794a1fd5e68 Mon Sep 17 00:00:00 2001 From: hokomo <hokomo@airmail.cc> Date: Sat, 25 Jun 2022 09:44:54 +0200 Subject: [PATCH] Fix detecting dotted pairs * lisp/calendar/icalendar.el (icalendar-export-region): Fix detecting dotted pairs. --- lisp/calendar/icalendar.el | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lisp/calendar/icalendar.el b/lisp/calendar/icalendar.el index 1a5a071e20..cf54293989 100644 --- a/lisp/calendar/icalendar.el +++ b/lisp/calendar/icalendar.el @@ -1144,7 +1144,8 @@ icalendar-export-region (cdr contents-n-summary)))) (setq result (concat result header contents alarm "\nEND:VEVENT"))) - (if (consp cns-cons-or-list) + (if (and (consp cns-cons-or-list) + (not (listp (cdr cns-cons-or-list)))) (list cns-cons-or-list) cns-cons-or-list))) ;; handle errors -- 2.35.3
hokomo <hokomo@airmail.cc> writes: > Exporting the my-float sexp will fail with: > > "Error in line 0 -- (wrong-type-argument listp First Sunday in July > 2): ‘%%(my-float 7 0 1) First Sunday in July 2" > > because the returned list is confused for a dotted pair. Thanks; patch pushed to Emacs 29. > `icalendar-export-sexp-enumeration-days' is set to 366 to guarantee > that the sexp event occurs at least once. It looks like there's a > different bug (?) where, even if an entry is recognized as an > arbitrary diary sexp, if it doesn't produce any events, the converter > will go ahead with trying to interpret it in a different way and > eventually fail. E.g., lowering the enumeration days to 0 gives: > > "Error in line 0 -- (error Could not parse date): ‘%%(my-float 7 0 1) > First Sunday in July 2’" > > after exhausting all of the known entry types. Should I file this as a > separate bug? No, we can work on this problem here in this bug report. Do you have a recipe to reproduce the problem, starting from "emacs -Q"? -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
[-- Attachment #1: Type: text/plain, Size: 3207 bytes --] >> `icalendar-export-sexp-enumeration-days' is set to 366 to guarantee >> that the sexp event occurs at least once. It looks like there's a >> different bug (?) where, even if an entry is recognized as an >> arbitrary diary sexp, if it doesn't produce any events, the converter >> will go ahead with trying to interpret it in a different way and >> eventually fail. E.g., lowering the enumeration days to 0 gives: >> >> "Error in line 0 -- (error Could not parse date): ‘%%(my-float 7 0 1) >> First Sunday in July 2’" >> >> after exhausting all of the known entry types. Should I file this as a >> separate bug? > > No, we can work on this problem here in this bug report. > > Do you have a recipe to reproduce the problem, starting from "emacs -Q"? Yep, you can use the same testing script and just replace the last expression with: (let ((icalendar-export-sexp-enumeration-days 0)) (test-diary-sexp "%%(diary-float 7 0 1) First Sunday in July 1") (test-diary-sexp "%%(my-float 7 0 1) First Sunday in July 2")) which should give you the error I mentioned. I don't have a patch for this on hand but it would probably require restructuring the surrounding code a little bit. There's another bug that concerns sexps that contain more than a single closing parenthesis. Seems like the icalendar code uses a bunch of regexes to parse the sexp (see `icalendar--convert-sexp-to-ical'), rather than something like `read' or `read-from-string' (which is what diary does, as well as org-agenda). To reproduce (with the same testing code as before): (let ((icalendar-export-sexp-enumeration-days 366)) (test-diary-sexp "%%(= (calendar-day-of-week date) 0) Sunday 1") (test-diary-sexp "%%(= 0 (calendar-day-of-week date)) Sunday 2")) No matter whether the closing parentheses are bunched together or separated, parsing fails either way. I've attached a draft of a patch that modifies as little as possible and makes the above two cases work, but that's not enough as there's special handling of `and' forms. Furthermore, sexps that span multiple lines fail for the same reason, even though diary handles them just fine of course. Ideally the whole function should be rewritten to just use `read-from-string'. I don't understand why `and' forms are handled specially though, so I'm not sure how to proceed here. A last thing to note -- even though org-agenda properly handles multiline sexps and sexps with an arbitrary number of closing parentheses (because it does the parsing manually and uses `forward-sexp'), Org's parser itself does not properly handle multiline sexps (but does handle multiple closing parentheses). To confirm, use `org-element-at-point' at the beginning of these two diary sexps in an org-mode buffer: %%(let ((a 123) (b 456)) (+ a b)) %%(let ((a 123) (b 456)) (+ a b)) All of these bugs also propagate to ox-icalendar (which is how I got into exploring this whole thing) since it relies on the Org parse tree to pull out 'diary-sexp elements and on icalendar to export them into iCalendar format. Let me know if you want me to report any of these as separate bugs and what would be the best thing to do here. Regards, hokomo [-- Attachment #2: 0002-Draft-fix-nested-diary-sexps.patch --] [-- Type: text/x-patch, Size: 1197 bytes --] From e35ea1f3a9c62d9c07d10d9ec89bf879058775c2 Mon Sep 17 00:00:00 2001 From: hokomo <hokomo@airmail.cc> Date: Sat, 25 Jun 2022 13:43:28 +0200 Subject: [PATCH] Fix nested diary sexps --- lisp/calendar/icalendar.el | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lisp/calendar/icalendar.el b/lisp/calendar/icalendar.el index 46bcf4566f..2b6af79812 100644 --- a/lisp/calendar/icalendar.el +++ b/lisp/calendar/icalendar.el @@ -1641,9 +1641,11 @@ icalendar--convert-sexp-to-ical entry-main) ;; regular sexp entry (icalendar--dmsg "diary-sexp %s" entry-main) - (let ((p1 (substring entry-main (match-beginning 1) (match-end 1))) - (p2 (substring entry-main (match-beginning 2) (match-end 2))) - (now (or start (current-time)))) + (let* ((entry-main (substring entry-main 2)) + (res (read-from-string entry-main)) + (p1 (prin1-to-string (car res))) + (p2 (substring entry-main (cdr res))) + (now (or start (current-time)))) (delete nil (mapcar (lambda (offset) -- 2.35.3
hokomo <hokomo@airmail.cc> writes: > Yep, you can use the same testing script and just replace the last > expression with: > > (let ((icalendar-export-sexp-enumeration-days 0)) > (test-diary-sexp "%%(diary-float 7 0 1) First Sunday in July 1") > (test-diary-sexp "%%(my-float 7 0 1) First Sunday in July 2")) > > which should give you the error I mentioned. Right; I can reproduce that problem. I've added Ulf to the CCs; perhaps he has some comments. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
> Right; I can reproduce that problem. I've added Ulf to the CCs; perhaps
> he has some comments.
It's been a week since then. Any ideas on how to move forward or should
we give it more time? :-)
To summarize, the problems are:
(1) A diary sexp not generating any events for the configured number of
enumeration days is treated effectively as a parsing failure, meaning
that the next type of diary entry is tried instead of just not producing
any events.
(2) Improper parsing of diary sexps (using regexes instead of e.g.
`read') leads to diary sexps not being able to span multiple lines or
have more than a single closing parenthesis (i.e. any sort of nested
expression).
(3) Not strictly in the scope of icalendar itself, but still related the
handling of diary sexps: Org's parser seems to assume that each diary
sexp is on a single line. This and the two limitations above end up
propagating to ox-icalendar.
hokomo
hokomo <hokomo@airmail.cc> writes: > It's been a week since then. Any ideas on how to move forward or > should we give it more time? :-) If you have a possible patch for the issues, that'd be appreciated. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
Am 28.06.2022 um 13:21 (+0200) schrieb Lars Ingebrigtsen:
> hokomo <hokomo@airmail.cc> writes:
>
>> Yep, you can use the same testing script and just replace the last
>> expression with:
>>
>> (let ((icalendar-export-sexp-enumeration-days 0))
>> (test-diary-sexp "%%(diary-float 7 0 1) First Sunday in July 1")
>> (test-diary-sexp "%%(my-float 7 0 1) First Sunday in July 2"))
>>
>> which should give you the error I mentioned.
>
> Right; I can reproduce that problem. I've added Ulf to the CCs; perhaps
> he has some comments.
Sorry for the late reply. Thanks for the bug report, the patch and for
analysing the problems. I'll try to have a look at the icalendar code
the next days (or so).
Ulf Jasper <ulf.jasper@web.de> writes:
> Am 28.06.2022 um 13:21 (+0200) schrieb Lars Ingebrigtsen:
>> hokomo <hokomo@airmail.cc> writes:
>>
>>> Yep, you can use the same testing script and just replace the last
>>> expression with:
>>>
>>> (let ((icalendar-export-sexp-enumeration-days 0))
>>> (test-diary-sexp "%%(diary-float 7 0 1) First Sunday in July 1")
>>> (test-diary-sexp "%%(my-float 7 0 1) First Sunday in July 2"))
>>>
>>> which should give you the error I mentioned.
>>
>> Right; I can reproduce that problem. I've added Ulf to the CCs; perhaps
>> he has some comments.
>
> Sorry for the late reply. Thanks for the bug report, the patch and for
> analysing the problems. I'll try to have a look at the icalendar code
> the next days (or so).
That was 15 weeks ago, so here's a friendly ping.
I added a test to icalendar-tests.el that verifies that patch 0001-Fix-detecting-dotted-pairs.patch works as expected. I also prepared two additional tests for the other examples where icalendar-export fails. For the time being they are commented out as there is no fix available yet. Patch 0002-Draft-fix-nested-diary-sexps.patch seems to fix one of the examples but apparently breaks test 'icalendar-real-world'. So a look at 'icalendar--convert-sexp-to-ical' seems to be necessary. Best, Ulf
Am 23.11.2022 um 21:24 (+0100) schrieb Ulf Jasper:
> I added a test to icalendar-tests.el that verifies that patch
> 0001-Fix-detecting-dotted-pairs.patch works as expected.
>
> I also prepared two additional tests for the other examples where
> icalendar-export fails. For the time being they are commented out as
> there is no fix available yet. Patch
> 0002-Draft-fix-nested-diary-sexps.patch seems to fix one of the examples
> but apparently breaks test 'icalendar-real-world'. So a look at
> 'icalendar--convert-sexp-to-ical' seems to be necessary.
There was a testcase in 'icalendar-real-world' that relied on
icalendar-export to fail on a particular sexp. With patch 0002... that
testcase behaves differently (i.e. export works correctly now). In
other words: breaking that test was correct.
Applied patch 0002... and updated icalendar-tests.
Best,
Ulf
[-- Attachment #1: Type: text/plain, Size: 1784 bytes --] On Wed, Nov 23, 2022 at 09:24:45PM +0100, Ulf Jasper wrote: >I added a test to icalendar-tests.el that verifies that patch >0001-Fix-detecting-dotted-pairs.patch works as expected. Note that this test is failing on both the current 'master' and 'emacs-29' branch (excerpt, full log attached): "Error in line 0 -- (error Could not parse date): `%%(icalendar-tests--diary-float 7 0 1) First Sunday in July 2'\n" FAILED 25/42 icalendar-export-bug-56241-dotted-pair (0.003671 sec) at lisp/calendar/icalendar-tests.el:1000 From what I can tell, the problem is that while these calls look like they should be identical: (diary-float 7 0 1) (icalendar-tests--diary-float 7 0 1) ;; calls diary-float in practice, they are handled by different code paths: icalendar--convert-float-to-ical icalendar--convert-sexp-to-ical resp. The key difference being that the first one sets 'date' and 'entry' with calendar-dlet when calling diary-float*, and the latter does not. diary-float is documented as requiring those to be set.† I'm not sure of the fix here: if the expectation is that some arbitrary sexp should be able to indirectly call some of the diary-*‡ functions, then it may be that icalendar--convert-sexp-to-ical may need to similarly set those values. Alternately, if that is not supported, then maybe disable/remove the test. --bod * https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/calendar/icalendar.el?id=b42cd524b46a4f29ef13e9d03be9d3df917f9aa3#n1787 † https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/calendar/diary-lib.el?id=b42cd524b46a4f29ef13e9d03be9d3df917f9aa3#n1891 ‡ diary-date, diary-block, diary-float, diary-anniversary, diary-cyclic, and diary-offset (there may be others, those were the ones with an explicit comment) [-- Attachment #2: icalendar-tests.log.gz --] [-- Type: application/gzip, Size: 1770 bytes --]
Thank you for looking into these failures. The test started failing around the first Sunday of July, which is unlikely to be a coincidence. Calendar tests had better not depend on the time or date when the test is run. If necessary, these should be mocked up by the tests.
The `icalendar-export-bug-56241-dotted-pair` test has now been marked :unstable, which means that it is not run automatically by `make check`. The problem still remains, and it would be good to have a working regression test for the bug, assuming it has indeed been fixed.
[-- Attachment #1: Type: text/plain, Size: 496 bytes --] Not sure if this helps but I was wondering why exporting '%%(org-anniversary 2020 02 02) blah %d blah' worked but '%%(org-anniversary 2020 03 02) blah %d blah' did not. Long story short, icalendar-export-sexp-enumeration-days was set to 14 by default. I have changed this to 366 and the export works again. If relevant, I noticed that one of the tests mentioned above does not set the value explicitly while the others do. I'm not a programmer so take my observations with due caution! Jeremy [-- Attachment #2: Type: text/html, Size: 1343 bytes --]