Subject: bug#79177: 31.0.50; [PATCH] [v4] Add tests to ispell.el

From: Lockywolf <[email-masked]>
Date: Sat, 09 Aug 2025 19:49:19 +0800

Eli Zaretskii <[email-masked]> writes:

>> From: Lockywolf <[email-masked]>
>> Cc: Lockywolf <[email-masked]>,  [email-masked]
>> 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?

This is intended to succeed, because "fake_aspell.bash", irrespective of
the options, behaves in the way real aspell behaves when being called
with "-vv".

I thought that I might eventually mock the important bits of aspell
without actually implementing the dictionary-lookup algorithm, so that
it would be possible to run at least some backend-involving tests
without having _any_ backend installed on the device.  It would have
been nice to implement this mock in Emacs, but, as I said, emacs --batch
processing of command-line arguments does not allow that at present.

I do think that having bash is more common than having ispell, aspell,
or hunspell.  If you think otherwise, I can replace this call with a
call to real {ispell, aspell, hunspell, enchant-2}, like the other tests
do.

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

I can try to use an :override advice to `call-process' instead.  It
actually worked on my machine, even though I remember that advising
primitive forms is expected to be glitchy.

>> >> >> >> +    (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.

No it does not. But even if it did... (see the next paragraph)...

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

If you look at the test carefully once again, you will see that it does
the following
#+begin_src emacs-lisp
(insert
       "hello\n\n\n"
       "<!-- " ispell-dictionary-keyword test-dict " -->"
       "<!-- " ispell-pdict-keyword test-pdict " -->")
#+end_src

This test runs the test on a buffer, which is such as if it is written
by a user.  Surely we can expect files having
"Local IspellPersDict: /tmp/nonexistent.dict" in the footer,
being _written_ by SOME users, by _opened_on_Windows_ by OTHER users,
don't you think?  Don't you think that such files are expected to be
processed gracefully by Emacs, regardless of whether such paths are
absolute, relative, nonexistent, or even UNC?  Moreover, a user might
even put something like:
"Local IspellPersDict: (my-best-personal-dictionary)",
or
"Local IspellPersDict: https://cgit.git.savannah.gnu.org/cgit/miscfiles.git/tree/web2"
(Such structures are not supported, and should be ignored, or warned
about by ~ispell.el~.)
This is "user I/O", and tests, at least usually, are not expected to
receive graceful input, they are expected to make sure that the code
works reasonably even on garbage input.

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

Fair enough, but overall, the convention of Windows (or any systemn) is
to not fail on text files having some UNIX paths in them.  I can add a
test having "C:\TMP\nonexistent.pdict", to make sure that all platforms
are covered, if you'd like.

-- 
Your sincerely,
Vladimir Nikishkin (MiEr, lockywolf)
(Laptop)