bug#64242: [PATCH] Fix VC package build when there is no docs dir

bug#64242: [PATCH] Fix VC package build when there is no docs dir

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

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

Tags: patch

This fixes a small mistake introduced recently in
'package-vc--build-documentation' which causes it to error out when its
'file' argument isn't a directory.



In GNU Emacs 30.0.50 (build 4, x86_64-pc-linux-gnu, GTK+ Version
 3.24.38, cairo version 1.16.0) of 2023-06-21 built on coldharbour
Repository revision: 6085ee8139cc3d815a5028babb4daf438df9d06b
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
System Description: Void Linux

Configured using:
 'configure -C --prefix=/opt/Emacs --with-x --with-x-toolkit=gtk3
 --without-gsettings --without-dbus --with-xinput2 --with-small-ja-dic
 --with-native-compilation'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-VC-package-build-when-there-is-no-docs-dir.patch --]
[-- Type: text/patch, Size: 1184 bytes --]

From b21139b7d900d6c3bd38b2790758d2c1b631ada3 Mon Sep 17 00:00:00 2001
From: Daniel Semyonov <daniel@dsemy.com>
Date: Fri, 23 Jun 2023 08:40:57 +0300
Subject: [PATCH] Fix VC package build when there is no docs dir

* lisp/emacs-lisp/package-vc.el (package-vc--build-documentation): Set
'docs-directory' to 'default-directory' if 'file' isn't a directory.
---
 lisp/emacs-lisp/package-vc.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index f34cfb3120b..b87bcbc738e 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -386,7 +386,8 @@ package-vc--build-documentation
 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)))
+         (docs-directory (if (file-name-directory file)
+                             (expand-file-name file) default-directory))
          (output (expand-file-name (format "%s.info" pkg-name)))
          clean-up)
     (when (string-match-p "\\.org\\'" file)
-- 
2.41.0

bug#64242: [PATCH] Fix VC package build when there is no docs dir

[Eli Zaretskii]

> Date: Fri, 23 Jun 2023 08:45:03 +0300
> From:  Daniel Semyonov via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> This fixes a small mistake introduced recently in
> 'package-vc--build-documentation' which causes it to error out when its
> 'file' argument isn't a directory.

What do you mean by "isn't a directory"?  FILE is supposed to be a
documentation file, not a directory, see the doc string of this
function.  Do you mean that FILE is a relative file name so that
file-name-directory returns nil for it?  If so, I think the correct
fix would be to reverse the order:

  (docs-directory (file-name-directory (expand-file-name file)))

Can you show a recipe to reproduce this problem, preferably starting
from "emacs -Q"?

And finally, this issue exists on the emacs-29 release branch as well,
doesn't it?

Thanks.

bug#64242: [PATCH] Fix VC package build when there is no docs dir

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

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

>>>>> Eli Zaretskii writes:

    > What do you mean by "isn't a directory"?  FILE is supposed to be a
    > documentation file, not a directory, see the doc string of this
    > function.  Do you mean that FILE is a relative file name so that
    > file-name-directory returns nil for it?  If so, I think the
    > correct fix would be to reverse the order:

    >   (docs-directory (file-name-directory (expand-file-name file)))

Yeah, you're completely right, this was a misunderstanding on my part;
my fix just happened to also work so I made some incorrect assumptions.

After some further testing, this seems to happen for packages for which
the doc file is in the base directory of the package (which causes
'file-name-directory' to return nil as the doc file name is relative to
the package dir).

I attached an updated patch.

    > Can you show a recipe to reproduce this problem, preferably
    > starting from "emacs -Q"?

1. $ emacs -Q
2. M-x package-vc-install RET eat RET
3. A few seconds later,
   "package-vc--unpack-1: Wrong type argument: stringp, nil"

    > And finally, this issue exists on the emacs-29 release branch as
    > well, doesn't it?

I haven't tested it, but I don't see a reason it wont.


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

From 0160171b1663bf349af7ffe6de0b2ea6b4b326a7 Mon Sep 17 00:00:00 2001
From: Daniel Semyonov <daniel@dsemy.com>
Date: Fri, 23 Jun 2023 08:40:57 +0300
Subject: [PATCH] Fix VC package build when doc file isn't in a subdir

* lisp/emacs-lisp/package-vc.el (package-vc--build-documentation):
Expand 'file' before attempting to get its directory.
---
 lisp/emacs-lisp/package-vc.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index f34cfb3120b..db8b41aee6a 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -386,7 +386,7 @@ package-vc--build-documentation
 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)))
+         (docs-directory (file-name-directory (expand-file-name file)))
          (output (expand-file-name (format "%s.info" pkg-name)))
          clean-up)
     (when (string-match-p "\\.org\\'" file)
-- 
2.41.0

bug#64242: [PATCH] Fix VC package build when there is no docs dir

[Eli Zaretskii]

> From: Daniel Semyonov <daniel@dsemy.com>
> Cc: 64242@debbugs.gnu.org
> Date: Fri, 23 Jun 2023 09:32:01 +0300
> 
> >>>>> Eli Zaretskii writes:
> 
>     > What do you mean by "isn't a directory"?  FILE is supposed to be a
>     > documentation file, not a directory, see the doc string of this
>     > function.  Do you mean that FILE is a relative file name so that
>     > file-name-directory returns nil for it?  If so, I think the
>     > correct fix would be to reverse the order:
> 
>     >   (docs-directory (file-name-directory (expand-file-name file)))
> 
> Yeah, you're completely right, this was a misunderstanding on my part;
> my fix just happened to also work so I made some incorrect assumptions.
> 
> After some further testing, this seems to happen for packages for which
> the doc file is in the base directory of the package (which causes
> 'file-name-directory' to return nil as the doc file name is relative to
> the package dir).
> 
> I attached an updated patch.
> 
>     > Can you show a recipe to reproduce this problem, preferably
>     > starting from "emacs -Q"?
> 
> 1. $ emacs -Q
> 2. M-x package-vc-install RET eat RET
> 3. A few seconds later,
>    "package-vc--unpack-1: Wrong type argument: stringp, nil"
> 
>     > And finally, this issue exists on the emacs-29 release branch as
>     > well, doesn't it?
> 
> I haven't tested it, but I don't see a reason it wont.

Philip, any comments or objections to installing this on the emacs-29
branch?

Thanks.

bug#64242: [PATCH] Fix VC package build when there is no docs dir

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Daniel Semyonov <daniel@dsemy.com>
>> Cc: 64242@debbugs.gnu.org
>> Date: Fri, 23 Jun 2023 09:32:01 +0300
>> 
>> >>>>> Eli Zaretskii writes:
>> 
>>     > What do you mean by "isn't a directory"?  FILE is supposed to be a
>>     > documentation file, not a directory, see the doc string of this
>>     > function.  Do you mean that FILE is a relative file name so that
>>     > file-name-directory returns nil for it?  If so, I think the
>>     > correct fix would be to reverse the order:
>> 
>>     >   (docs-directory (file-name-directory (expand-file-name file)))
>> 
>> Yeah, you're completely right, this was a misunderstanding on my part;
>> my fix just happened to also work so I made some incorrect assumptions.
>> 
>> After some further testing, this seems to happen for packages for which
>> the doc file is in the base directory of the package (which causes
>> 'file-name-directory' to return nil as the doc file name is relative to
>> the package dir).
>> 
>> I attached an updated patch.
>> 
>>     > Can you show a recipe to reproduce this problem, preferably
>>     > starting from "emacs -Q"?
>> 
>> 1. $ emacs -Q
>> 2. M-x package-vc-install RET eat RET
>> 3. A few seconds later,
>>    "package-vc--unpack-1: Wrong type argument: stringp, nil"
>> 
>>     > And finally, this issue exists on the emacs-29 release branch as
>>     > well, doesn't it?
>> 
>> I haven't tested it, but I don't see a reason it wont.
>
> Philip, any comments or objections to installing this on the emacs-29
> branch?

I have none.  There were just come complains last time, because we made
the change too soon, so I believe the consensus was to wait for a bit
longer and then it was just forgotten.

> Thanks.

bug#64242: [PATCH] Fix VC package build when there is no docs dir

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Daniel Semyonov <daniel@dsemy.com>
>>> Cc: 64242@debbugs.gnu.org
>>> Date: Fri, 23 Jun 2023 09:32:01 +0300
>>> 
>>> >>>>> Eli Zaretskii writes:
>>> 
>>>     > What do you mean by "isn't a directory"?  FILE is supposed to be a
>>>     > documentation file, not a directory, see the doc string of this
>>>     > function.  Do you mean that FILE is a relative file name so that
>>>     > file-name-directory returns nil for it?  If so, I think the
>>>     > correct fix would be to reverse the order:
>>> 
>>>     >   (docs-directory (file-name-directory (expand-file-name file)))
>>> 
>>> Yeah, you're completely right, this was a misunderstanding on my part;
>>> my fix just happened to also work so I made some incorrect assumptions.
>>> 
>>> After some further testing, this seems to happen for packages for which
>>> the doc file is in the base directory of the package (which causes
>>> 'file-name-directory' to return nil as the doc file name is relative to
>>> the package dir).
>>> 
>>> I attached an updated patch.
>>> 
>>>     > Can you show a recipe to reproduce this problem, preferably
>>>     > starting from "emacs -Q"?
>>> 
>>> 1. $ emacs -Q
>>> 2. M-x package-vc-install RET eat RET
>>> 3. A few seconds later,
>>>    "package-vc--unpack-1: Wrong type argument: stringp, nil"
>>> 
>>>     > And finally, this issue exists on the emacs-29 release branch as
>>>     > well, doesn't it?
>>> 
>>> I haven't tested it, but I don't see a reason it wont.
>>
>> Philip, any comments or objections to installing this on the emacs-29
>> branch?
>
> I have none.  There were just come complains last time, because we made
> the change too soon, so I believe the consensus was to wait for a bit
> longer and then it was just forgotten.

Oops, I misread this thread and was under the assumption you were
responding to a different message.  Sorry about that!  Shouldn't write
messages when I am in a hurry.

Regarding Daniel's patch, I don't believe we need `file-name-directory'
at all, the variable `docs-directory' is just passed to makeinfo -I, and to
my knowledge it should make a difference if the argument ends with a
directory delimiter or not.

-- 
Philip Kaludercic

bug#64242: [PATCH] Fix VC package build when there is no docs dir

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: 64242@debbugs.gnu.org,  Daniel Semyonov <daniel@dsemy.com>
> Date: Sat, 24 Jun 2023 10:24:47 +0000
> 
> Regarding Daniel's patch, I don't believe we need `file-name-directory'
> at all, the variable `docs-directory' is just passed to makeinfo -I, and to
> my knowledge it should make a difference if the argument ends with a
> directory delimiter or not.

Are you thinking about directory-file-name instead of
file-name-directory?  The latter returns the parent directory of its
argument, it doesn't affect the trailing slash.  The FILE argument
passed to package-vc--build-documentation is a file, not a directory,
so the file-name-directory call seems justified if we want to pass the
directory to makeinfo.

Or what am I missing?

bug#64242: [PATCH] Fix VC package build when there is no docs dir

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: 64242@debbugs.gnu.org,  Daniel Semyonov <daniel@dsemy.com>
>> Date: Sat, 24 Jun 2023 10:24:47 +0000
>> 
>> Regarding Daniel's patch, I don't believe we need `file-name-directory'
>> at all, the variable `docs-directory' is just passed to makeinfo -I, and to
>> my knowledge it should make a difference if the argument ends with a
>> directory delimiter or not.
>
> Are you thinking about directory-file-name instead of
> file-name-directory?  

Eh, yes of course I didn't try the patch and expected that to fail.

>                       The latter returns the parent directory of its
> argument, it doesn't affect the trailing slash.  The FILE argument
> passed to package-vc--build-documentation is a file, not a directory,
> so the file-name-directory call seems justified if we want to pass the
> directory to makeinfo.

Right, my bad.

> Or what am I missing?

No, it is all my fault.  I'll try the patch our and if it all works I'll
push it to emacs-29.  Ok?

-- 
Philip Kaludercic

bug#64242: [PATCH] Fix VC package build when there is no docs dir

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: 64242@debbugs.gnu.org,  daniel@dsemy.com
> Date: Sat, 24 Jun 2023 15:34:40 +0000
> 
> I'll try the patch our and if it all works I'll push it to emacs-29.
> Ok?

Yes, please.

bug#64242: [PATCH] Fix VC package build when there is no docs dir

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: 64242@debbugs.gnu.org,  daniel@dsemy.com
>> Date: Sat, 24 Jun 2023 15:34:40 +0000
>> 
>> I'll try the patch our and if it all works I'll push it to emacs-29.
>> Ok?
>
> Yes, please.

OK, I have tested it and pushed the patch.  Thanks Daniel!