Package: emacs;
Reported by: Lockywolf <for_emacs_1 <at> lockywolf.net>
Date: Tue, 5 Aug 2025 14:10:03 UTC
Severity: normal
Tags: patch
Found in version 31.0.50
Message #29 received at 79177 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Lockywolf <for_emacs_1 <at> lockywolf.net> Cc: 79177 <at> debbugs.gnu.org Subject: Re: bug#79177: 31.0.50; [PATCH] [v4] Add tests to ispell.el Date: Sat, 09 Aug 2025 13:57:49 +0300
> From: Lockywolf <for_emacs_1 <at> lockywolf.net>
> Cc: Lockywolf <for_emacs_1 <at> lockywolf.net>, 79177 <at> debbugs.gnu.org
> Date: Fri, 08 Aug 2025 10:21:37 +0800
>
> >> >> Aspell needs to parse the "-vv" command-line argument, and Emacs
> >> >> launched in batch mode cannot do that. Or I did not manage to make it.
> >> >
> >> > Sorry, I don't understand. The Bash script just outputs a fixed
> >> > string, and Emacs is capable of doing that. What did you try, and if
> >> > it didn''t work, how it failed to work?
> >>
> >> No, it does not /just/ output a fixed string. It also _ignores_ the
> >> command-line argument "-vv", which, if passed to emacs --batch, produces
> >> and error Error: error ("Unknown option ‘-vv’"), and ispell.el does it
> >> when initialising the backend.
> >
> > I feel there's some misunderstanding here. What I suggested is that
> > instead of doing this:
> >
> > (let ((fake-aspell-path (expand-file-name
> > "./fake-aspell.bash"
> > tests-ispell-data-directory)))
> > (chmod fake-aspell-path 504)
> > (call-process fake-aspell-path nil nil nil)
> >
> > where fake-aspell.bash does
> >
> > printf '%s\n' "@(#) International Ispell Version 3.1.20 (but really Aspell 0.59.800)"
> >
> > the code does this (not tested):
> >
> > (call-process "emacs" nil nil nil "-Q" "--batch" "--eval" "(princ \\"@(#) International Ispell Version 3.1.20 (but really Aspell 0.59.800)\\")")
> >
> > For best results, use
> >
> > (expand invocation-name invocation-directory)
> >
> > instead of a literal "emacs". This makes sure you invoke the same
> > Emacs as the one running the test, not some other version.
>
> I am not calling "fake_aspell.bash" with "(call-process fake-aspell-path
> nil nil nil)" in the name of printing a string. I am calling it in
> order to make sure that calling it works on the target system. Read the
> test further on, the important lines of the test are
> ```
> (setopt ispell-program-name fake-aspell-path)
> (ispell-check-version t)
> ```
> where ispell.el will call "fake_aspell.bash" in the way it likes calling
> backend executables.
So this is intended to fail the version check because fake_aspell
script ignores the -vv switch? If so, why calling Emacs would not do,
since Emacs also doesn't support -vv?
> Maybe I should wrap it in (skip-unless), but I remember it behaving
> strangely within a "let".
I'd prefer not to skip tests if we can avoid that. Relying on shell
scripts will require us to do something special on platforms without
Bash, and I'd like to avoid that if possible.
> >> >> >> + (let ((test-dict "nonexistent")
> >> >> >> + (test-pdict "/tmp/lnonexistent.txt"))
> >> >> > ^^^^^^^^^^^^^^^^^^^^^^^
> >> >> > Likewise here: please use
> >> >> >
> >> >> > (expand-file-name "lnonexistent.txt" temporary-file-directory)
> >> >> >
> >> >> > instead. (There are several other places in the patch with the same
> >> >> > problem.)
> >> >>
> >> >> This is not a file path. This is just a string which looks like a file
> >> >> path. If something tries to access this as if it is a path during the
> >> >> execution of the test, this is an error.
> >> >
> >> > OK, but the file name you use is not an absolute file name on Windows,
> >> > so it could cause problems. Is there a problem with using my
> >> > suggestion, even though you don't need a real file name?
> >>
> >> ispell's man page says the following:
> >>
> >> ```
> >> The -p option is used to specify an alternate personal dictionary file. If the file name does not begin with "/", $HOME is prefixed.
> >> ```
> >>
> >> I do not want ispell to do any mangling of the "HOME" directory if this
> >> "string which looks like a path" is by chance (if a test malfunctions)
> >> passed to ispell binary and it creates a bogus file in the home
> >> directory or overwrites something.
> >> On windows "temporary-file-directory" probably does not begin with "/".
> >
> > Yes, but a Windows port of ispell will know that, and detect absolute
> > file names as appropriate: by looking at the drive letter, as in
> > "d:/foo". On the contrary, using file names that begin with a slash
> > on Windows is likely to fail the test mentioned by the man page.
>
> No, it will not fail. Citing tree.c from ispell's source:
> ```c
> /*
> ** Figure out if p is an absolute path name. Note that beginning
> ** with "./" and "../" is considered an absolute path, since this
> ** still means we can't prepend HOME.
> */
> abspath = IS_SLASH (*p) || strncmp (p, "./", 2) == 0
> || strncmp (p, "../", 3) == 0;
> #ifdef MSDOS
> /*
> ** DTRT with drive-letter braindamage and with backslashes.
> */
> abspath |= strncmp (p, ".\\", 2) == 0
> || strncmp (p, "..\\", 3) == 0
> || (p[0] != '\0' && p[1] == ':');
> #endif
> ```
>
> That is it, all of it.
> There is no #ifdef WINDOWS block, and even the MSDOS block only "adds"
> to the absolute path check, so things starting with "/" are always
> considered absolute.
But it also recognizes the absolute file names that start with a drive
letter, which is the full form of an absolute name on Windows. So
using expand-file-name will not fail the test, would it? Moreover, we
cannot expect all Windows ports of the various spellers to do what
Ispell does, and it is IMO wrong to rely on specific details of a
specific port in the test suite. What if in some not-too-distant
future this test will be also used for Aspell or Hunspell, which might
not consider "/foo/bar" and absolute file name on Windows?
> In fact, quoting the README (line 258) from the ispell root:
>
> ```
> Although the ispell maintainer does not support MS-DOS and
> Windows, a generous contributor, Eli Zaretskii, has added MS-DOS
> support. You can build ispell for MS-DOS with either EMX/GCC or
> DJGPP. See the file pc/README for compilation instructions.
> ```
>
> I am surprised that you are asking me this question.
You always remember every line of code you wrote 20+ years ago?
And anyway, the important point here is not to rely on such fine
details of the implementation, but instead to go with the general
conventions of the target platform. Because if the application
doesn't behave according to those conventions, it's a bug in the
application that needs to be fixed there.
Thanks.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.