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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 63337 in the body.
You can then email your comments to 63337 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] package-vc--build-documentation: Fix relative @include
 statements
Date: Sat, 06 May 2023 13:54:49 -0700

[Message part 1 (text/plain, inline)]

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

[0001-Fix-package-vc-build-documentation-Relative-include-.patch (text/x-diff, attachment)]

Message #8 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63337 <at> debbugs.gnu.org
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Sun, 07 May 2023 09:58:19 +0000

Joseph Turner <joseph <at> 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 <at> 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

Message #11 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63337 <at> debbugs.gnu.org, joseph <at> breatheoutbreathe.in
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix relative
 @include statements
Date: Sun, 07 May 2023 13:56:10 +0300

> Cc: 63337 <at> debbugs.gnu.org
> From: Philip Kaludercic <philipk <at> posteo.net>
> Date: Sun, 07 May 2023 09:58:19 +0000
> 
> Joseph Turner <joseph <at> 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.

Message #14 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63337 <at> debbugs.gnu.org
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Sun, 07 May 2023 11:40:46 -0700

[Message part 1 (text/plain, inline)]

Philip Kaludercic <philipk <at> posteo.net> writes:

> Joseph Turner <joseph <at> 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 <at> 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

[0001-Fix-package-vc-build-documentation-Relative-include-.patch (text/x-diff, attachment)]

Message #17 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63337 <at> debbugs.gnu.org, philipk <at> posteo.net
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix relative
 @include statements
Date: Sun, 07 May 2023 22:11:10 +0300

> Cc: 63337 <at> 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 <at> 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.

Message #20 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 63337 <at> debbugs.gnu.org, Joseph Turner <joseph <at> breatheoutbreathe.in>
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Sun, 07 May 2023 19:19:43 +0000

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: 63337 <at> 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 <at> 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.

Message #23 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63337 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Sun, 07 May 2023 13:29:53 -0700

[Message part 1 (text/plain, inline)]

Philip Kaludercic <philipk <at> posteo.net> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> Cc: 63337 <at> 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 <at> 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

[0001-Fix-package-vc-build-documentation-Relative-include-.patch (text/x-diff, attachment)]

Message #26 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63337 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Mon, 08 May 2023 13:51:23 +0000

Joseph Turner <joseph <at> breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>>>> Cc: 63337 <at> 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 <at> 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 <at> 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))))

Message #29 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63337 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Mon, 08 May 2023 12:05:51 -0700

[Message part 1 (text/plain, inline)]

Philip Kaludercic <philipk <at> 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

[0001-Fix-package-vc-build-documentation-Relative-include-.patch (text/x-diff, attachment)]

Message #32 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ruijie Yu <ruijie <at> netyu.xyz>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63337 <at> debbugs.gnu.org, Philip Kaludercic <philipk <at> posteo.net>,
 bug-gnu-emacs <at> gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Tue, 09 May 2023 09:34:18 +0800

Joseph Turner via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs <at> 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

Message #38 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Ruijie Yu <ruijie <at> netyu.xyz>
Cc: 63337 <at> debbugs.gnu.org, Philip Kaludercic <philipk <at> posteo.net>,
 bug-gnu-emacs <at> gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Mon, 08 May 2023 19:48:30 -0700

Ruijie Yu <ruijie <at> netyu.xyz> writes:

> Joseph Turner via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs <at> 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

Message #44 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63337 <at> debbugs.gnu.org, philipk <at> posteo.net
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Tue, 09 May 2023 07:36:42 +0300

> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 63337 <at> 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.

Message #47 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 63337 <at> debbugs.gnu.org, philipk <at> posteo.net
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Tue, 09 May 2023 16:49:24 -0700

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
>> Cc: Eli Zaretskii <eliz <at> gnu.org>, 63337 <at> 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

[0001-Fix-package-vc-build-documentation-Relative-include-.patch (text/x-diff, attachment)]

Message #50 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63337 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Wed, 10 May 2023 06:51:53 +0000

Joseph Turner <joseph <at> breatheoutbreathe.in> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
>>> Cc: Eli Zaretskii <eliz <at> gnu.org>, 63337 <at> 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?

Message #53 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63337 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Wed, 10 May 2023 19:04:02 -0700

Philip Kaludercic <philipk <at> 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

Message #56 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63337 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Fri, 12 May 2023 06:51:19 +0000

[Message part 1 (text/plain, inline)]

Joseph Turner <joseph <at> breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk <at> 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:

[Message part 2 (text/plain, inline)]

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.

[Message part 3 (text/plain, inline)]

> Joseph

Message #59 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63337 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Fri, 12 May 2023 06:56:36 +0000

[Message part 1 (text/plain, inline)]

Joseph Turner <joseph <at> breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk <at> 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:

[Message part 2 (text/plain, inline)]

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

[Message part 3 (text/plain, inline)]

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

Message #62 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63337 <at> debbugs.gnu.org, joseph <at> breatheoutbreathe.in
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Fri, 12 May 2023 10:14:31 +0300

> From: Philip Kaludercic <philipk <at> posteo.net>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  63337 <at> 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?

Message #65 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 63337 <at> debbugs.gnu.org, joseph <at> breatheoutbreathe.in
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Fri, 12 May 2023 07:35:16 +0000

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Philip Kaludercic <philipk <at> posteo.net>
>> Cc: Eli Zaretskii <eliz <at> gnu.org>,  63337 <at> 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).

Message #68 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63337 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Fri, 12 May 2023 22:47:11 -0700

Philip Kaludercic <philipk <at> 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.

Message #71 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63337 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Fri, 12 May 2023 22:54:00 -0700

Philip Kaludercic <philipk <at> posteo.net> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> From: Philip Kaludercic <philipk <at> posteo.net>
>>> Cc: Eli Zaretskii <eliz <at> gnu.org>,  63337 <at> 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?

Message #74 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63337 <at> debbugs.gnu.org
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Sat, 13 May 2023 08:41:19 +0000

Joseph Turner <joseph <at> breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> Joseph Turner <joseph <at> 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 <at> 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

Message #77 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63337 <at> debbugs.gnu.org
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Sat, 13 May 2023 09:38:11 -0700

[Message part 1 (text/plain, inline)]

Philip Kaludercic <philipk <at> posteo.net> writes:

> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>
>>> Joseph Turner <joseph <at> 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.

[0001-Fix-package-vc-build-documentation-Relative-include-.patch (text/x-diff, attachment)]

Message #80 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63337 <at> debbugs.gnu.org
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Sat, 13 May 2023 17:14:13 +0000

Joseph Turner <joseph <at> breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>>
>>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>>
>>>> Joseph Turner <joseph <at> 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 <at> 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

Message #85 received at 63337 <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 63337 <at> debbugs.gnu.org
Subject: Re: bug#63337: [PATCH] package-vc--build-documentation: Fix
 relative @include statements
Date: Sat, 13 May 2023 11:31:08 -0700

Philip Kaludercic <philipk <at> posteo.net> writes:

> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>
>>> Joseph Turner <joseph <at> breatheoutbreathe.in> writes:
>>>
>>>> Philip Kaludercic <philipk <at> posteo.net> writes:
>>>>
>>>>> Joseph Turner <joseph <at> 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 <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.