bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hello!

Because package-vc--build-documentation exports the texinfo manual to a
temp file inside /tmp/ , any @include statements with relative paths
break the makeinfo call.

I noticed this issue when attempting to use package-vc to install
org-transclusion, whose manual contains the line

#+texinfo: @include fdl.texi

See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org

The attached patch solves this problem by passing the -I flag to
makeinfo. From makeinfo --help:

-I DIR                        append DIR to the @include search path.

Best,

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-package-vc-build-documentation-Relative-include-.patch --]
[-- Type: text/x-diff, Size: 1478 bytes --]

From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 14:49:43 -0700
Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
 statements

---
 lisp/emacs-lisp/package-vc.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 489610e2a1e..63c10285ca7 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
 otherwise it's assumed to be an Info file."
   (let* ((pkg-name (package-desc-name pkg-desc))
          (default-directory (package-desc-dir pkg-desc))
+         (docs-directory (expand-file-name (file-name-directory file)))
          (output (expand-file-name (format "%s.info" pkg-name)))
          clean-up)
     (when (string-match-p "\\.org\\'" file)
@@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
       (erase-buffer)
       (cond
        ((/= 0 (call-process "makeinfo" nil t nil
-                            "--no-split" file "-o" output))
+                            "-I" docs-directory
+                            "--no-split" file
+                            "-o" output))
         (message "Failed to build manual %s, see buffer %S"
                  file (buffer-name)))
        ((/= 0 (call-process "install-info" nil t nil
-- 
2.39.2

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Hello!
>
> Because package-vc--build-documentation exports the texinfo manual to a
> temp file inside /tmp/ , any @include statements with relative paths
> break the makeinfo call.
>
> I noticed this issue when attempting to use package-vc to install
> org-transclusion, whose manual contains the line
>
> #+texinfo: @include fdl.texi
>
> See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
>
> The attached patch solves this problem by passing the -I flag to
> makeinfo. From makeinfo --help:
>
> -I DIR                        append DIR to the @include search path.

Good catch, this should be applied to emacs-29.

> Best,
>
> Joseph
>
> From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Sat, 6 May 2023 14:49:43 -0700
> Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
>  statements
>
> ---
>  lisp/emacs-lisp/package-vc.el | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index 489610e2a1e..63c10285ca7 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
>  otherwise it's assumed to be an Info file."
>    (let* ((pkg-name (package-desc-name pkg-desc))
>           (default-directory (package-desc-dir pkg-desc))
> +         (docs-directory (expand-file-name (file-name-directory file)))
>           (output (expand-file-name (format "%s.info" pkg-name)))
>           clean-up)
>      (when (string-match-p "\\.org\\'" file)
> @@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
>        (erase-buffer)
>        (cond
>         ((/= 0 (call-process "makeinfo" nil t nil
> -                            "--no-split" file "-o" output))
> +                            "-I" docs-directory

According to the docs, makeinfo has -I to append the search path, and -P
to prepend.  I don't know how well either of the two are supported, but
assuming they are, shouldn't -P be preferred?  Or wouldn't it have any
effect?

> +                            "--no-split" file
> +                            "-o" output))
>          (message "Failed to build manual %s, see buffer %S"
>                   file (buffer-name)))
>         ((/= 0 (call-process "install-info" nil t nil

-- 
Philip Kaludercic

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Eli Zaretskii]

> Cc: 63337@debbugs.gnu.org
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sun, 07 May 2023 09:58:19 +0000
> 
> Joseph Turner <joseph@breatheoutbreathe.in> writes:
> 
> > Hello!
> >
> > Because package-vc--build-documentation exports the texinfo manual to a
> > temp file inside /tmp/ , any @include statements with relative paths
> > break the makeinfo call.
> >
> > I noticed this issue when attempting to use package-vc to install
> > org-transclusion, whose manual contains the line
> >
> > #+texinfo: @include fdl.texi
> >
> > See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
> >
> > The attached patch solves this problem by passing the -I flag to
> > makeinfo. From makeinfo --help:
> >
> > -I DIR                        append DIR to the @include search path.
> 
> Good catch, this should be applied to emacs-29.

Fine by me.

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


Philip Kaludercic <philipk@posteo.net> writes:

> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Hello!
>>
>> Because package-vc--build-documentation exports the texinfo manual to a
>> temp file inside /tmp/ , any @include statements with relative paths
>> break the makeinfo call.
>>
>> I noticed this issue when attempting to use package-vc to install
>> org-transclusion, whose manual contains the line
>>
>> #+texinfo: @include fdl.texi
>>
>> See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
>>
>> The attached patch solves this problem by passing the -I flag to
>> makeinfo. From makeinfo --help:
>>
>> -I DIR                        append DIR to the @include search path.
>
> Good catch, this should be applied to emacs-29.
>
>> Best,
>>
>> Joseph
>>
>> From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Sat, 6 May 2023 14:49:43 -0700
>> Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
>>  statements
>>
>> ---
>>  lisp/emacs-lisp/package-vc.el | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index 489610e2a1e..63c10285ca7 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
>>  otherwise it's assumed to be an Info file."
>>    (let* ((pkg-name (package-desc-name pkg-desc))
>>           (default-directory (package-desc-dir pkg-desc))
>> +         (docs-directory (expand-file-name (file-name-directory file)))
>>           (output (expand-file-name (format "%s.info" pkg-name)))
>>           clean-up)
>>      (when (string-match-p "\\.org\\'" file)
>> @@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
>>        (erase-buffer)
>>        (cond
>>         ((/= 0 (call-process "makeinfo" nil t nil
>> -                            "--no-split" file "-o" output))
>> +                            "-I" docs-directory
>
> According to the docs, makeinfo has -I to append the search path, and -P
> to prepend.  I don't know how well either of the two are supported, but
> assuming they are, shouldn't -P be preferred?  Or wouldn't it have any
> effect?

I am not sure what difference it would make. I don't know if the default
@include search path includes anything besides the working directory.

In the attached diff, I have changed -I to -P.

>> +                            "--no-split" file
>> +                            "-o" output))
>>          (message "Failed to build manual %s, see buffer %S"
>>                   file (buffer-name)))
>>         ((/= 0 (call-process "install-info" nil t nil


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-package-vc-build-documentation-Relative-include-.patch --]
[-- Type: text/x-diff, Size: 1478 bytes --]

From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 14:49:43 -0700
Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
 statements

---
 lisp/emacs-lisp/package-vc.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 489610e2a1e..63c10285ca7 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
 otherwise it's assumed to be an Info file."
   (let* ((pkg-name (package-desc-name pkg-desc))
          (default-directory (package-desc-dir pkg-desc))
+         (docs-directory (expand-file-name (file-name-directory file)))
          (output (expand-file-name (format "%s.info" pkg-name)))
          clean-up)
     (when (string-match-p "\\.org\\'" file)
@@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
       (erase-buffer)
       (cond
        ((/= 0 (call-process "makeinfo" nil t nil
-                            "--no-split" file "-o" output))
+                            "-P" docs-directory
+                            "--no-split" file
+                            "-o" output))
         (message "Failed to build manual %s, see buffer %S"
                  file (buffer-name)))
        ((/= 0 (call-process "install-info" nil t nil
-- 
2.39.2

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Eli Zaretskii]

> Cc: 63337@debbugs.gnu.org
> Date: Sun, 07 May 2023 11:40:46 -0700
> From:  Joseph Turner via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > According to the docs, makeinfo has -I to append the search path, and -P
> > to prepend.  I don't know how well either of the two are supported, but
> > assuming they are, shouldn't -P be preferred?  Or wouldn't it have any
> > effect?
> 
> I am not sure what difference it would make. I don't know if the default
> @include search path includes anything besides the working directory.

It doesn't, according to the Texinfo manual.  Only the current
directory is searched.

> In the attached diff, I have changed -I to -P.

I think it's a mistake: the current directory should searched first.
So -I is better.

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 63337@debbugs.gnu.org
>> Date: Sun, 07 May 2023 11:40:46 -0700
>> From:  Joseph Turner via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> > According to the docs, makeinfo has -I to append the search path, and -P
>> > to prepend.  I don't know how well either of the two are supported, but
>> > assuming they are, shouldn't -P be preferred?  Or wouldn't it have any
>> > effect?
>> 
>> I am not sure what difference it would make. I don't know if the default
>> @include search path includes anything besides the working directory.

I don't know that either, and I can imagine that certain versions of
makeinfo might be patched or this could change in the future.

> It doesn't, according to the Texinfo manual.  Only the current
> directory is searched.
>
>> In the attached diff, I have changed -I to -P.
>
> I think it's a mistake: the current directory should searched first.
> So -I is better.

What do we mean by the current directory?  When building the manual from
an org-file, we switch to a temporary directory (where the .org -> .texi
conversion is stored), so the "actual" directory is not the same as the
default-directory.

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


Philip Kaludercic <philipk@posteo.net> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Cc: 63337@debbugs.gnu.org
>>> Date: Sun, 07 May 2023 11:40:46 -0700
>>> From:  Joseph Turner via "Bug reports for GNU Emacs,
>>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>>
>>> > According to the docs, makeinfo has -I to append the search path, and -P
>>> > to prepend.  I don't know how well either of the two are supported, but
>>> > assuming they are, shouldn't -P be preferred?  Or wouldn't it have any
>>> > effect?
>>>
>>> I am not sure what difference it would make. I don't know if the default
>>> @include search path includes anything besides the working directory.
>
> I don't know that either, and I can imagine that certain versions of
> makeinfo might be patched or this could change in the future.
>
>> It doesn't, according to the Texinfo manual.  Only the current
>> directory is searched.
>>
>>> In the attached diff, I have changed -I to -P.
>>
>> I think it's a mistake: the current directory should searched first.
>> So -I is better.
>
> What do we mean by the current directory?  When building the manual from
> an org-file, we switch to a temporary directory (where the .org -> .texi
> conversion is stored), so the "actual" directory is not the same as the
> default-directory.

AFAICT, makeinfo searches the default-directory. See attached patch,
where we let-bind default-directory to the docs-directory. In this case,
neither -I nor -P is necessary.

It's a bit strange to let-bind default-directory twice in the same
function, but we can't bind it at the top of the function, the
insert-file-contents expects default-directory to be package-desc-dir.

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-package-vc-build-documentation-Relative-include-.patch --]
[-- Type: text/x-diff, Size: 2344 bytes --]

From 7ddfd7ab08820eef159b21047194aaaf4c8841f7 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 14:49:43 -0700
Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
 statements

---
 lisp/emacs-lisp/package-vc.el | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 476c38916a8..c25a96ed942 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -377,6 +377,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
 otherwise it's assumed to be an Info file."
   (let* ((pkg-name (package-desc-name pkg-desc))
          (default-directory (package-desc-dir pkg-desc))
+         (docs-directory (expand-file-name (file-name-directory file)))
          (output (expand-file-name (format "%s.info" pkg-name)))
          clean-up)
     (when (string-match-p "\\.org\\'" file)
@@ -389,16 +390,18 @@ otherwise it's assumed to be an Info file."
         (setq clean-up t)))
     (with-current-buffer (get-buffer-create " *package-vc doc*")
       (erase-buffer)
-      (cond
-       ((/= 0 (call-process "makeinfo" nil t nil
-                            "--no-split" file "-o" output))
-        (message "Failed to build manual %s, see buffer %S"
-                 file (buffer-name)))
-       ((/= 0 (call-process "install-info" nil t nil
-                            output (expand-file-name "dir")))
-        (message "Failed to install manual %s, see buffer %S"
-                 output (buffer-name)))
-       ((kill-buffer))))
+      (let ((default-directory docs-directory))
+        ;; `let'-bind `default-directory' so that makeinfo resolves
+        ;; relative @include statements in the docs directory
+        (cond
+         ((/= 0 (call-process "makeinfo" nil t nil "--no-split" file "-o" output))
+          (message "Failed to build manual %s, see buffer %S"
+                   file (buffer-name)))
+         ((/= 0 (call-process "install-info" nil t nil
+                              output (expand-file-name "dir")))
+          (message "Failed to install manual %s, see buffer %S"
+                   output (buffer-name)))
+         ((kill-buffer)))))
     (when clean-up
       (delete-file file))))
 
-- 
2.39.2

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> Cc: 63337@debbugs.gnu.org
>>>> Date: Sun, 07 May 2023 11:40:46 -0700
>>>> From:  Joseph Turner via "Bug reports for GNU Emacs,
>>>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>>>
>>>> > According to the docs, makeinfo has -I to append the search path, and -P
>>>> > to prepend.  I don't know how well either of the two are supported, but
>>>> > assuming they are, shouldn't -P be preferred?  Or wouldn't it have any
>>>> > effect?
>>>>
>>>> I am not sure what difference it would make. I don't know if the default
>>>> @include search path includes anything besides the working directory.
>>
>> I don't know that either, and I can imagine that certain versions of
>> makeinfo might be patched or this could change in the future.
>>
>>> It doesn't, according to the Texinfo manual.  Only the current
>>> directory is searched.
>>>
>>>> In the attached diff, I have changed -I to -P.
>>>
>>> I think it's a mistake: the current directory should searched first.
>>> So -I is better.
>>
>> What do we mean by the current directory?  When building the manual from
>> an org-file, we switch to a temporary directory (where the .org -> .texi
>> conversion is stored), so the "actual" directory is not the same as the
>> default-directory.
>
> AFAICT, makeinfo searches the default-directory. See attached patch,
> where we let-bind default-directory to the docs-directory. In this case,
> neither -I nor -P is necessary.
>
> It's a bit strange to let-bind default-directory twice in the same
> function, but we can't bind it at the top of the function, the
> insert-file-contents expects default-directory to be package-desc-dir.

It might be, but I'll have to look into that in more detail, that the
first default-directory binding is not necessary if we pass
(package-desc-dir pkg-desc) as the second argument to `expand-file-name'
when binding `output'.  Then this would all be simplified, and we could
avoid the confusion you mention.

> Joseph
>
> From 7ddfd7ab08820eef159b21047194aaaf4c8841f7 Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Sat, 6 May 2023 14:49:43 -0700
> Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
>  statements
>
> ---
>  lisp/emacs-lisp/package-vc.el | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index 476c38916a8..c25a96ed942 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -377,6 +377,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
>  otherwise it's assumed to be an Info file."
>    (let* ((pkg-name (package-desc-name pkg-desc))
>           (default-directory (package-desc-dir pkg-desc))
> +         (docs-directory (expand-file-name (file-name-directory file)))
>           (output (expand-file-name (format "%s.info" pkg-name)))
>           clean-up)
>      (when (string-match-p "\\.org\\'" file)
> @@ -389,16 +390,18 @@ otherwise it's assumed to be an Info file."
>          (setq clean-up t)))
>      (with-current-buffer (get-buffer-create " *package-vc doc*")
>        (erase-buffer)
> -      (cond
> -       ((/= 0 (call-process "makeinfo" nil t nil
> -                            "--no-split" file "-o" output))
> -        (message "Failed to build manual %s, see buffer %S"
> -                 file (buffer-name)))
> -       ((/= 0 (call-process "install-info" nil t nil
> -                            output (expand-file-name "dir")))
> -        (message "Failed to install manual %s, see buffer %S"
> -                 output (buffer-name)))
> -       ((kill-buffer))))
> +      (let ((default-directory docs-directory))
> +        ;; `let'-bind `default-directory' so that makeinfo resolves
> +        ;; relative @include statements in the docs directory
> +        (cond
> +         ((/= 0 (call-process "makeinfo" nil t nil "--no-split" file "-o" output))
> +          (message "Failed to build manual %s, see buffer %S"
> +                   file (buffer-name)))
> +         ((/= 0 (call-process "install-info" nil t nil
> +                              output (expand-file-name "dir")))
> +          (message "Failed to install manual %s, see buffer %S"
> +                   output (buffer-name)))
> +         ((kill-buffer)))))
>      (when clean-up
>        (delete-file file))))

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


Philip Kaludercic <philipk@posteo.net> writes:

> It might be, but I'll have to look into that in more detail, that the
> first default-directory binding is not necessary if we pass
> (package-desc-dir pkg-desc) as the second argument to `expand-file-name'
> when binding `output'.  Then this would all be simplified, and we could
> avoid the confusion you mention.

While this would mean binding default-directory only once, it still
requires two `let'-bindings.

The solution in the attached patch requires only one let-binding while
changing the behavior of package-vc--build-documentation slightly. Now
the output .info file is put inside the same directory as FILE, instead
of inside the directory returned by (package-desc-dir pkg-desc).

Note about the following two lines:

+ (file-path (expand-file-name file (package-desc-dir pkg-desc)))
+ (default-directory (expand-file-name (file-name-directory file-path)))

(package-desc-dir pkg-desc) may return a relative path with or without a
directory, e.g. "doc/manual.org" or "manual.org". In the latter case,
(file-name-directory "manual.org") would return `nil' and
(expand-file-name nil) would signal an error.

Therefore, in the `file-path' `let'-binding, we first expand the return
value of (package-desc-dir pkg-desc) to ensure that it contains a directory.

Best,

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-package-vc-build-documentation-Relative-include-.patch --]
[-- Type: text/x-diff, Size: 1588 bytes --]

From 2cf2d522818c75ff5626324251bb74cdc3c36dc7 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 14:49:43 -0700
Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
 statements

---
 lisp/emacs-lisp/package-vc.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 476c38916a8..65767cf043a 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -376,14 +376,17 @@ Package specs are loaded from trusted package archives."
 FILE can be an Org file, indicated by its \".org\" extension,
 otherwise it's assumed to be an Info file."
   (let* ((pkg-name (package-desc-name pkg-desc))
-         (default-directory (package-desc-dir pkg-desc))
+         (file-path (expand-file-name file (package-desc-dir pkg-desc)))
+         ;; `let'-bind `default-directory' to the directory containing the .org or .info FILE
+         ;; so that makeinfo can resolve relative @include statements in the docs directory.
+         (default-directory (expand-file-name (file-name-directory file-path)))
          (output (expand-file-name (format "%s.info" pkg-name)))
          clean-up)
     (when (string-match-p "\\.org\\'" file)
       (require 'ox)
       (require 'ox-texinfo)
       (with-temp-buffer
-        (insert-file-contents file)
+        (insert-file-contents file-path)
         (setq file (make-temp-file "ox-texinfo-"))
         (org-export-to-file 'texinfo file)
         (setq clean-up t)))
-- 
2.39.2

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Joseph Turner via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> writes:

> + (file-path (expand-file-name file (package-desc-dir pkg-desc)))
> + (default-directory (expand-file-name (file-name-directory file-path)))
>
> (package-desc-dir pkg-desc) may return a relative path with or without a
> directory, e.g. "doc/manual.org" or "manual.org". In the latter case,
> (file-name-directory "manual.org") would return `nil' and
> (expand-file-name nil) would signal an error.

In this case, can't you do this instead:

    (expand-file-name (or (file-name-directory ...) "."))

?

-- 
Best,


RY

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Ruijie Yu <ruijie@netyu.xyz> writes:

> Joseph Turner via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> writes:
>
>> + (file-path (expand-file-name file (package-desc-dir pkg-desc)))
>> + (default-directory (expand-file-name (file-name-directory file-path)))
>>
>> (package-desc-dir pkg-desc) may return a relative path with or without a
>> directory, e.g. "doc/manual.org" or "manual.org". In the latter case,
>> (file-name-directory "manual.org") would return `nil' and
>> (expand-file-name nil) would signal an error.
>
> In this case, can't you do this instead:
>
>     (expand-file-name (or (file-name-directory ...) "."))

Yes, we could do this, but we make use of FILE-PATH anyway. We can't use
FILE after DEFAULT-DIRECTORY has been set the file containing FILE.

The choice is between

(file-name-directory file-path)

and

(or (file-name-directory file) ".")

I think the intent comes across more clearly in the former.

Joseph

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: Eli Zaretskii <eliz@gnu.org>, 63337@debbugs.gnu.org
> Date: Mon, 08 May 2023 12:05:51 -0700
> 
> Note about the following two lines:
> 
> + (file-path (expand-file-name file (package-desc-dir pkg-desc)))
> + (default-directory (expand-file-name (file-name-directory file-path)))
> 
> (package-desc-dir pkg-desc) may return a relative path with or without a
> directory, e.g. "doc/manual.org" or "manual.org". In the latter case,
> (file-name-directory "manual.org") would return `nil' and
> (expand-file-name nil) would signal an error.
> 
> Therefore, in the `file-path' `let'-binding, we first expand the return
> value of (package-desc-dir pkg-desc) to ensure that it contains a directory.

Please don't use "path" for anything that is not a PATH-style list of
directory: the GNU Coding Standards frown on such usage.  We use
file-name instead.  For the same reasons, please don't give your
variables names that include "path" unless they are lists of
directories.

> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -376,14 +376,17 @@ Package specs are loaded from trusted package archives."
>  FILE can be an Org file, indicated by its \".org\" extension,
>  otherwise it's assumed to be an Info file."
>    (let* ((pkg-name (package-desc-name pkg-desc))
> -         (default-directory (package-desc-dir pkg-desc))
> +         (file-path (expand-file-name file (package-desc-dir pkg-desc)))
> +         ;; `let'-bind `default-directory' to the directory containing the .org or .info FILE
> +         ;; so that makeinfo can resolve relative @include statements in the docs directory.
> +         (default-directory (expand-file-name (file-name-directory file-path)))

There should be no reason to call expand-file-name in the last line,
since the argument of file-name-directory is already expanded.

Also, please make the comment lines shorter, preferably less than 75
columns.

Thanks.

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


Eli Zaretskii <eliz@gnu.org> writes:

>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Cc: Eli Zaretskii <eliz@gnu.org>, 63337@debbugs.gnu.org
>> Date: Mon, 08 May 2023 12:05:51 -0700
>>
>> Note about the following two lines:
>>
>> + (file-path (expand-file-name file (package-desc-dir pkg-desc)))
>> + (default-directory (expand-file-name (file-name-directory file-path)))
>>
>> (package-desc-dir pkg-desc) may return a relative path with or without a
>> directory, e.g. "doc/manual.org" or "manual.org". In the latter case,
>> (file-name-directory "manual.org") would return `nil' and
>> (expand-file-name nil) would signal an error.
>>
>> Therefore, in the `file-path' `let'-binding, we first expand the return
>> value of (package-desc-dir pkg-desc) to ensure that it contains a directory.
>
> Please don't use "path" for anything that is not a PATH-style list of
> directory: the GNU Coding Standards frown on such usage.  We use
> file-name instead.  For the same reasons, please don't give your
> variables names that include "path" unless they are lists of
> directories.

Good to know, thank you! I changed `file-path' to `file-name'.

>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -376,14 +376,17 @@ Package specs are loaded from trusted package archives."
>>  FILE can be an Org file, indicated by its \".org\" extension,
>>  otherwise it's assumed to be an Info file."
>>    (let* ((pkg-name (package-desc-name pkg-desc))
>> -         (default-directory (package-desc-dir pkg-desc))
>> +         (file-path (expand-file-name file (package-desc-dir pkg-desc)))
>> +         ;; `let'-bind `default-directory' to the directory containing the .org or .info FILE
>> +         ;; so that makeinfo can resolve relative @include statements in the docs directory.
>> +         (default-directory (expand-file-name (file-name-directory file-path)))
>
> There should be no reason to call expand-file-name in the last line,
> since the argument of file-name-directory is already expanded.

Good catch! Fixed.

> Also, please make the comment lines shorter, preferably less than 75
> columns.

Done.

Thank you!!

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-package-vc-build-documentation-Relative-include-.patch --]
[-- Type: text/x-diff, Size: 1583 bytes --]

From c670f47ef55b41265c064a2d4ab1e56c46e57272 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 14:49:43 -0700
Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
 statements

---
 lisp/emacs-lisp/package-vc.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 95e12fc829a..efcfd635e98 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -377,14 +377,18 @@ Package specs are loaded from trusted package archives."
 FILE can be an Org file, indicated by its \".org\" extension,
 otherwise it's assumed to be an Info file."
   (let* ((pkg-name (package-desc-name pkg-desc))
-         (default-directory (package-desc-dir pkg-desc))
+         (file-name (expand-file-name file (package-desc-dir pkg-desc)))
+         ;; `let'-bind `default-directory' to the directory containing
+         ;; the .org or .info FILE so that makeinfo can resolve
+         ;; relative @include statements in the docs directory.
+         (default-directory (file-name-directory file-name))
          (output (expand-file-name (format "%s.info" pkg-name)))
          clean-up)
     (when (string-match-p "\\.org\\'" file)
       (require 'ox)
       (require 'ox-texinfo)
       (with-temp-buffer
-        (insert-file-contents file)
+        (insert-file-contents file-name)
         (setq file (make-temp-file "ox-texinfo-"))
         (org-export-to-file 'texinfo file)
         (setq clean-up t)))
-- 
2.39.2

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>> Cc: Eli Zaretskii <eliz@gnu.org>, 63337@debbugs.gnu.org
>>> Date: Mon, 08 May 2023 12:05:51 -0700
>>>
>>> Note about the following two lines:
>>>
>>> + (file-path (expand-file-name file (package-desc-dir pkg-desc)))
>>> + (default-directory (expand-file-name (file-name-directory file-path)))
>>>
>>> (package-desc-dir pkg-desc) may return a relative path with or without a
>>> directory, e.g. "doc/manual.org" or "manual.org". In the latter case,
>>> (file-name-directory "manual.org") would return `nil' and
>>> (expand-file-name nil) would signal an error.
>>>
>>> Therefore, in the `file-path' `let'-binding, we first expand the return
>>> value of (package-desc-dir pkg-desc) to ensure that it contains a directory.
>>
>> Please don't use "path" for anything that is not a PATH-style list of
>> directory: the GNU Coding Standards frown on such usage.  We use
>> file-name instead.  For the same reasons, please don't give your
>> variables names that include "path" unless they are lists of
>> directories.
>
> Good to know, thank you! I changed `file-path' to `file-name'.
>
>>> --- a/lisp/emacs-lisp/package-vc.el
>>> +++ b/lisp/emacs-lisp/package-vc.el
>>> @@ -376,14 +376,17 @@ Package specs are loaded from trusted package archives."
>>>  FILE can be an Org file, indicated by its \".org\" extension,
>>>  otherwise it's assumed to be an Info file."
>>>    (let* ((pkg-name (package-desc-name pkg-desc))
>>> -         (default-directory (package-desc-dir pkg-desc))
>>> +         (file-path (expand-file-name file (package-desc-dir pkg-desc)))
>>> +         ;; `let'-bind `default-directory' to the directory containing the .org or .info FILE
>>> +         ;; so that makeinfo can resolve relative @include statements in the docs directory.
>>> +         (default-directory (expand-file-name (file-name-directory file-path)))
>>
>> There should be no reason to call expand-file-name in the last line,
>> since the argument of file-name-directory is already expanded.
>
> Good catch! Fixed.
>
>> Also, please make the comment lines shorter, preferably less than 75
>> columns.
>
> Done.
>
> Thank you!!
>
> Joseph

Ok, do you have a few example repositories that we can use to test edge-cases?

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> Ok, do you have a few example repositories that we can use to test edge-cases?

I first noticed this issue when attempting to build the docs for
org-transclusion. Besides that, we could select a few package specs
containing :make at random from
https://git.savannah.gnu.org/cgit/emacs/elpa.git/tree/elpa-packages ?

Joseph

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Philip Kaludercic]

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

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Ok, do you have a few example repositories that we can use to test edge-cases?
>
> I first noticed this issue when attempting to build the docs for
> org-transclusion. Besides that, we could select a few package specs
> containing :make at random from
> https://git.savannah.gnu.org/cgit/emacs/elpa.git/tree/elpa-packages ?

One issue I still notice in this case is that if the file doesn't exist,
the issue is raised by `insert-file-contents' which aborts the entire
installation.  Does checking the existence of the file, and continuing
on if this is not the case make sense:


[-- Attachment #2: Type: text/plain, Size: 2453 bytes --]

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index e9794eac783..1c1492d87b2 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -353,28 +353,30 @@ package-vc--build-documentation
          (default-directory (file-name-directory file-name))
          (output (expand-file-name (format "%s.info" pkg-name)))
          clean-up)
-    (when (string-match-p "\\.org\\'" file)
-      (require 'ox)
-      (require 'ox-texinfo)
-      (with-temp-buffer
-        (insert-file-contents file-name)
-        (setq file (make-temp-file "ox-texinfo-"))
-        (org-export-to-file 'texinfo file)
-        (setq clean-up t)))
-    (with-current-buffer (get-buffer-create " *package-vc doc*")
-      (erase-buffer)
-      (cond
-       ((/= 0 (call-process "makeinfo" nil t nil
-                            "--no-split" file "-o" output))
-        (message "Failed to build manual %s, see buffer %S"
-                 file (buffer-name)))
-       ((/= 0 (call-process "install-info" nil t nil
-                            output (expand-file-name "dir")))
-        (message "Failed to install manual %s, see buffer %S"
-                 output (buffer-name)))
-       ((kill-buffer))))
-    (when clean-up
-      (delete-file file))))
+    (if (not (file-exists-p file-name))
+        (message "Documentation file %S for %s not found" file pkg-name)
+      (when (string-match-p "\\.org\\'" file)
+        (require 'ox)
+        (require 'ox-texinfo)
+        (with-temp-buffer
+          (insert-file-contents file-name)
+          (setq file (make-temp-file "ox-texinfo-"))
+          (org-export-to-file 'texinfo file)
+          (setq clean-up t)))
+      (with-current-buffer (get-buffer-create " *package-vc doc*")
+        (erase-buffer)
+        (cond
+         ((/= 0 (call-process "makeinfo" nil t nil
+                              "--no-split" file "-o" output))
+          (message "Failed to build manual %s, see buffer %S"
+                   file (buffer-name)))
+         ((/= 0 (call-process "install-info" nil t nil
+                              output (expand-file-name "dir")))
+          (message "Failed to install manual %s, see buffer %S"
+                   output (buffer-name)))
+         ((kill-buffer))))
+      (when clean-up
+        (delete-file file)))))
 
 (defun package-vc-install-dependencies (requirements)
   "Install missing dependencies, and return missing ones.

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



> Joseph

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  63337@debbugs.gnu.org
> Date: Fri, 12 May 2023 06:51:19 +0000
> 
> One issue I still notice in this case is that if the file doesn't exist,
> the issue is raised by `insert-file-contents' which aborts the entire
> installation.  Does checking the existence of the file, and continuing
> on if this is not the case make sense:

If the file's (non)existence is the problem, then checking that up
front is an okay solution, IMO.  Are there any downsides to doing that?

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  63337@debbugs.gnu.org
>> Date: Fri, 12 May 2023 06:51:19 +0000
>> 
>> One issue I still notice in this case is that if the file doesn't exist,
>> the issue is raised by `insert-file-contents' which aborts the entire
>> installation.  Does checking the existence of the file, and continuing
>> on if this is not the case make sense:
>
> If the file's (non)existence is the problem, then checking that up
> front is an okay solution, IMO.  Are there any downsides to doing that?

One might not notice that there was an issue with the package
specification, since a number of messages are usually generated when
installing a package.  But I agree that this sounds better than not
being able to install the package at all (as is currently the case with
org-transclusion).

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Philip Kaludercic <philipk@posteo.net>
>>> Cc: Eli Zaretskii <eliz@gnu.org>,  63337@debbugs.gnu.org
>>> Date: Fri, 12 May 2023 06:51:19 +0000
>>>
>>> One issue I still notice in this case is that if the file doesn't exist,
>>> the issue is raised by `insert-file-contents' which aborts the entire
>>> installation.  Does checking the existence of the file, and continuing
>>> on if this is not the case make sense:
>>
>> If the file's (non)existence is the problem, then checking that up
>> front is an okay solution, IMO.  Are there any downsides to doing that?
>
> One might not notice that there was an issue with the package
> specification, since a number of messages are usually generated when
> installing a package.  But I agree that this sounds better than not
> being able to install the package at all (as is currently the case with
> org-transclusion).

This change makes sense to me also. Would it be appropriate to use warn
instead of message? I'm not sure the convention here.

This change belongs in a separate commit, right?

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Philip Kaludercic]

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

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Ok, do you have a few example repositories that we can use to test edge-cases?
>
> I first noticed this issue when attempting to build the docs for
> org-transclusion. Besides that, we could select a few package specs
> containing :make at random from
> https://git.savannah.gnu.org/cgit/emacs/elpa.git/tree/elpa-packages ?

Oh, and a big problem is that the "dir" file is written into the wrong
directory.  It has to be located in the root directory of the package,
not in docs/ (in the case of org-transclusion).  Sadly adjusting the
second argument doesn't fix the issue:


[-- Attachment #2: Type: text/plain, Size: 704 bytes --]

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index e9794eac783..9876705e57f 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -369,7 +369,8 @@ package-vc--build-documentation
         (message "Failed to build manual %s, see buffer %S"
                  file (buffer-name)))
        ((/= 0 (call-process "install-info" nil t nil
-                            output (expand-file-name "dir")))
+                            output
+                            (expand-file-name "dir" (package-desc-dir pkg-desc))))
         (message "Failed to install manual %s, see buffer %S"
                  output (buffer-name)))
        ((kill-buffer))))

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


While the entry does appear in (dir) Top, the file cannot be opened:

  Info-find-file: Info file ‘org-transclusion’ does not exist; consider installing it

> Joseph

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> Oh, and a big problem is that the "dir" file is written into the wrong
> directory.  It has to be located in the root directory of the package,
> not in docs/ (in the case of org-transclusion).  Sadly adjusting the
> second argument doesn't fix the issue:
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index e9794eac783..9876705e57f 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -369,7 +369,8 @@ package-vc--build-documentation
>          (message "Failed to build manual %s, see buffer %S"
>                   file (buffer-name)))
>         ((/= 0 (call-process "install-info" nil t nil
> -                            output (expand-file-name "dir")))
> +                            output
> +                            (expand-file-name "dir" (package-desc-dir pkg-desc))))
>          (message "Failed to install manual %s, see buffer %S"
>                   output (buffer-name)))
>         ((kill-buffer))))
>
>
> While the entry does appear in (dir) Top, the file cannot be opened:
>
>   Info-find-file: Info file ‘org-transclusion’ does not exist; consider installing it

IIUC, you're saying that default-directory needs to be (package-desc-dir pkg-desc) when
install-info runs, right? Perhaps we should revisit the first patch I sent:

- leave default-directory as-is
- pass "-I docs-directory" to makeinfo

If makeinfo starts including more directory besides the working
directory in the future, I think it will still make sense to append the
docs-directory to the search path.

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>> Hello!
>>>
>>> Because package-vc--build-documentation exports the texinfo manual to a
>>> temp file inside /tmp/ , any @include statements with relative paths
>>> break the makeinfo call.
>>>
>>> I noticed this issue when attempting to use package-vc to install
>>> org-transclusion, whose manual contains the line
>>>
>>> #+texinfo: @include fdl.texi
>>>
>>> See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
>>>
>>> The attached patch solves this problem by passing the -I flag to
>>> makeinfo. From makeinfo --help:
>>>
>>> -I DIR                        append DIR to the @include search path.
>>
>> Good catch, this should be applied to emacs-29.
>>
>>> Best,
>>>
>>> Joseph
>>>
>>> From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
>>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>>> Date: Sat, 6 May 2023 14:49:43 -0700
>>> Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
>>>  statements
>>>
>>> ---
>>>  lisp/emacs-lisp/package-vc.el | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>>> index 489610e2a1e..63c10285ca7 100644
>>> --- a/lisp/emacs-lisp/package-vc.el
>>> +++ b/lisp/emacs-lisp/package-vc.el
>>> @@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
>>>  otherwise it's assumed to be an Info file."
>>>    (let* ((pkg-name (package-desc-name pkg-desc))
>>>           (default-directory (package-desc-dir pkg-desc))
>>> +         (docs-directory (expand-file-name (file-name-directory file)))
>>>           (output (expand-file-name (format "%s.info" pkg-name)))
>>>           clean-up)
>>>      (when (string-match-p "\\.org\\'" file)
>>> @@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
>>>        (erase-buffer)
>>>        (cond
>>>         ((/= 0 (call-process "makeinfo" nil t nil
>>> -                            "--no-split" file "-o" output))
>>> +                            "-I" docs-directory
>>
>> According to the docs, makeinfo has -I to append the search path, and -P
>> to prepend.  I don't know how well either of the two are supported, but
>> assuming they are, shouldn't -P be preferred?  Or wouldn't it have any
>> effect?
>
> I am not sure what difference it would make. I don't know if the default
> @include search path includes anything besides the working directory.
>
> In the attached diff, I have changed -I to -P.

I can confirm that this patch does the right thing, and I think we
should apply it.

>>> +                            "--no-split" file
>>> +                            "-o" output))
>>>          (message "Failed to build manual %s, see buffer %S"
>>>                   file (buffer-name)))
>>>         ((/= 0 (call-process "install-info" nil t nil

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


Philip Kaludercic <philipk@posteo.net> writes:

> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>
>>>> Hello!
>>>>
>>>> Because package-vc--build-documentation exports the texinfo manual to a
>>>> temp file inside /tmp/ , any @include statements with relative paths
>>>> break the makeinfo call.
>>>>
>>>> I noticed this issue when attempting to use package-vc to install
>>>> org-transclusion, whose manual contains the line
>>>>
>>>> #+texinfo: @include fdl.texi
>>>>
>>>> See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
>>>>
>>>> The attached patch solves this problem by passing the -I flag to
>>>> makeinfo. From makeinfo --help:
>>>>
>>>> -I DIR                        append DIR to the @include search path.

>>> According to the docs, makeinfo has -I to append the search path, and -P
>>> to prepend.  I don't know how well either of the two are supported, but
>>> assuming they are, shouldn't -P be preferred?  Or wouldn't it have any
>>> effect?
>>
>> I am not sure what difference it would make. I don't know if the default
>> @include search path includes anything besides the working directory.
>>
>> In the attached diff, I have changed -I to -P.
>
> I can confirm that this patch does the right thing, and I think we
> should apply it.

I think Eli suggested we prepend (-I) instead of append (-P), as in the
very first patch I sent, also attached here.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-package-vc-build-documentation-Relative-include-.patch --]
[-- Type: text/x-diff, Size: 1478 bytes --]

From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sat, 6 May 2023 14:49:43 -0700
Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
 statements

---
 lisp/emacs-lisp/package-vc.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 489610e2a1e..63c10285ca7 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
 otherwise it's assumed to be an Info file."
   (let* ((pkg-name (package-desc-name pkg-desc))
          (default-directory (package-desc-dir pkg-desc))
+         (docs-directory (expand-file-name (file-name-directory file)))
          (output (expand-file-name (format "%s.info" pkg-name)))
          clean-up)
     (when (string-match-p "\\.org\\'" file)
@@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
       (erase-buffer)
       (cond
        ((/= 0 (call-process "makeinfo" nil t nil
-                            "--no-split" file "-o" output))
+                            "-I" docs-directory
+                            "--no-split" file
+                            "-o" output))
         (message "Failed to build manual %s, see buffer %S"
                  file (buffer-name)))
        ((/= 0 (call-process "install-info" nil t nil
-- 
2.39.2

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>
>>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>>
>>>>> Hello!
>>>>>
>>>>> Because package-vc--build-documentation exports the texinfo manual to a
>>>>> temp file inside /tmp/ , any @include statements with relative paths
>>>>> break the makeinfo call.
>>>>>
>>>>> I noticed this issue when attempting to use package-vc to install
>>>>> org-transclusion, whose manual contains the line
>>>>>
>>>>> #+texinfo: @include fdl.texi
>>>>>
>>>>> See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
>>>>>
>>>>> The attached patch solves this problem by passing the -I flag to
>>>>> makeinfo. From makeinfo --help:
>>>>>
>>>>> -I DIR                        append DIR to the @include search path.
>
>>>> According to the docs, makeinfo has -I to append the search path, and -P
>>>> to prepend.  I don't know how well either of the two are supported, but
>>>> assuming they are, shouldn't -P be preferred?  Or wouldn't it have any
>>>> effect?
>>>
>>> I am not sure what difference it would make. I don't know if the default
>>> @include search path includes anything besides the working directory.
>>>
>>> In the attached diff, I have changed -I to -P.
>>
>> I can confirm that this patch does the right thing, and I think we
>> should apply it.
>
> I think Eli suggested we prepend (-I) instead of append (-P), as in the
> very first patch I sent, also attached here.

I did not understand the argument, but it probably does not matter that
much.  As this patch has Eli's blessing, I will apply it.

> From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Sat, 6 May 2023 14:49:43 -0700
> Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
>  statements
>
> ---
>  lisp/emacs-lisp/package-vc.el | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index 489610e2a1e..63c10285ca7 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
>  otherwise it's assumed to be an Info file."
>    (let* ((pkg-name (package-desc-name pkg-desc))
>           (default-directory (package-desc-dir pkg-desc))
> +         (docs-directory (expand-file-name (file-name-directory file)))
>           (output (expand-file-name (format "%s.info" pkg-name)))
>           clean-up)
>      (when (string-match-p "\\.org\\'" file)
> @@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
>        (erase-buffer)
>        (cond
>         ((/= 0 (call-process "makeinfo" nil t nil
> -                            "--no-split" file "-o" output))
> +                            "-I" docs-directory
> +                            "--no-split" file
> +                            "-o" output))
>          (message "Failed to build manual %s, see buffer %S"
>                   file (buffer-name)))
>         ((/= 0 (call-process "install-info" nil t nil

bug#63337: [PATCH] package-vc--build-documentation: Fix relative @include statements

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>
>>>> Philip Kaludercic <philipk@posteo.net> writes:
>>>>
>>>>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>>>>
>>>>>> Hello!
>>>>>>
>>>>>> Because package-vc--build-documentation exports the texinfo manual to a
>>>>>> temp file inside /tmp/ , any @include statements with relative paths
>>>>>> break the makeinfo call.
>>>>>>
>>>>>> I noticed this issue when attempting to use package-vc to install
>>>>>> org-transclusion, whose manual contains the line
>>>>>>
>>>>>> #+texinfo: @include fdl.texi
>>>>>>
>>>>>> See: https://raw.githubusercontent.com/nobiot/org-transclusion/main/docs/org-transclusion-manual.org
>>>>>>
>>>>>> The attached patch solves this problem by passing the -I flag to
>>>>>> makeinfo. From makeinfo --help:
>>>>>>
>>>>>> -I DIR                        append DIR to the @include search path.
>>
>>>>> According to the docs, makeinfo has -I to append the search path, and -P
>>>>> to prepend.  I don't know how well either of the two are supported, but
>>>>> assuming they are, shouldn't -P be preferred?  Or wouldn't it have any
>>>>> effect?
>>>>
>>>> I am not sure what difference it would make. I don't know if the default
>>>> @include search path includes anything besides the working directory.
>>>>
>>>> In the attached diff, I have changed -I to -P.
>>>
>>> I can confirm that this patch does the right thing, and I think we
>>> should apply it.
>>
>> I think Eli suggested we prepend (-I) instead of append (-P), as in the
>> very first patch I sent, also attached here.
>
> I did not understand the argument, but it probably does not matter that
> much.  As this patch has Eli's blessing, I will apply it.

Great! Thank you!!

>
>> From a41abce88ed3b833c5531208945474c9cd16284b Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Sat, 6 May 2023 14:49:43 -0700
>> Subject: [PATCH] Fix: (package-vc--build-documentation) Relative @include
>>  statements
>>
>> ---
>>  lisp/emacs-lisp/package-vc.el | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index 489610e2a1e..63c10285ca7 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -381,6 +381,7 @@ FILE can be an Org file, indicated by its \".org\" extension,
>>  otherwise it's assumed to be an Info file."
>>    (let* ((pkg-name (package-desc-name pkg-desc))
>>           (default-directory (package-desc-dir pkg-desc))
>> +         (docs-directory (expand-file-name (file-name-directory file)))
>>           (output (expand-file-name (format "%s.info" pkg-name)))
>>           clean-up)
>>      (when (string-match-p "\\.org\\'" file)
>> @@ -395,7 +396,9 @@ otherwise it's assumed to be an Info file."
>>        (erase-buffer)
>>        (cond
>>         ((/= 0 (call-process "makeinfo" nil t nil
>> -                            "--no-split" file "-o" output))
>> +                            "-I" docs-directory
>> +                            "--no-split" file
>> +                            "-o" output))
>>          (message "Failed to build manual %s, see buffer %S"
>>                   file (buffer-name)))
>>         ((/= 0 (call-process "install-info" nil t nil