Viper tests failure?

Viper tests failure?

[Fabrice Popineau]

Hi,

With emacs-25 commit e5e886d12f3eff3102916c82a5aa82e0db714579
Emacs compiled using Mingw64 I get a strange error
running `make check':

$ src/emacs --batch -Q -l ../emacs/test/automated/viper-tests.el --eval '(ert-
run-tests-batch t)'
Running 6 tests (2016-06-21 07:01:58+0200)
Loading viper...
Test viper-test-fix backtrace:
  byte-code(\204'\306\307\310!CB\301    CB\302\nCB\303
                                                      CB\311\307\311!
  load("viper")
  load-library("viper")
  viper-mode()
  (progn (viper-mode) (switch-to-buffer (get-buffer-create "*viper-tes
  (unwind-protect (progn (viper-mode) (switch-to-buffer (get-buffer-cr
  (let ((noninteractive nil) (viper-inhibit-startup-message (quote t))
  viper-test-undo-kmacro([])
  apply(viper-test-undo-kmacro [])
  (setq value-4 (apply fn-2 args-3))
  (unwind-protect (setq value-4 (apply fn-2 args-3)) (setq form-descri
  (if (unwind-protect (setq value-4 (apply fn-2 args-3)) (setq form-de
  (let (form-description-6) (if (unwind-protect (setq value-4 (apply f
  (let ((value-4 (quote ert-form-evaluation-aborted-5))) (let (form-de
  (let ((fn-2 (function viper-test-undo-kmacro)) (args-3 (list []))) (
  (lambda nil (let ((fn-2 (function viper-test-undo-kmacro)) (args-3 (
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test viper-test-fix "Test that the viper
  ert-run-or-rerun-test([cl-struct-ert--stats t [[cl-struct-ert-test v
  ert-run-tests(t #[385 "\306\307\"\203G\211\211G\310U\203\211@\20
  ert-run-tests-batch(t)
  eval((ert-run-tests-batch t))
  command-line-1(("-l" "../emacs/test/automated/viper-tests.el" "--eva
  command-line()
  normal-top-level()
Test viper-test-fix condition:
    (file-error "Opening stdio stream" "No such file or directory" 
"c:/Users/Fabrice/AppData/Local/Temp/viper-tests21052GpM")
   FAILED  1/6  viper-test-fix

I'm not sure what is happening there.
I checked the temp file is created and does exist, so why is there 
a failure to open it?

Regards,

Fabrice

Re: Viper tests failure?

[Phillip Lord]

Fabrice Popineau <fabrice.popineau@gmail.com> writes:

> Hi,
>
> With emacs-25 commit e5e886d12f3eff3102916c82a5aa82e0db714579
> Emacs compiled using Mingw64 I get a strange error
> running `make check':
>
> $ src/emacs --batch -Q -l ../emacs/test/automated/viper-tests.el --eval '(ert-
> run-tests-batch t)'
> Running 6 tests (2016-06-21 07:01:58+0200)
> Loading viper...
> Test viper-test-fix backtrace:
>   byte-code(\204'\306\307\310!CB\301    CB\302\nCB\303
>                                                       CB\311\307\311!
>   load("viper")
>   load-library("viper")
>   viper-mode()
>   (progn (viper-mode) (switch-to-buffer (get-buffer-create "*viper-tes
>   (unwind-protect (progn (viper-mode) (switch-to-buffer (get-buffer-cr
>   (let ((noninteractive nil) (viper-inhibit-startup-message (quote t))
>   viper-test-undo-kmacro([])
>   apply(viper-test-undo-kmacro [])
>   (setq value-4 (apply fn-2 args-3))
>   (unwind-protect (setq value-4 (apply fn-2 args-3)) (setq form-descri
>   (if (unwind-protect (setq value-4 (apply fn-2 args-3)) (setq form-de
>   (let (form-description-6) (if (unwind-protect (setq value-4 (apply f
>   (let ((value-4 (quote ert-form-evaluation-aborted-5))) (let (form-de
>   (let ((fn-2 (function viper-test-undo-kmacro)) (args-3 (list []))) (
>   (lambda nil (let ((fn-2 (function viper-test-undo-kmacro)) (args-3 (
>   ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
>   ert-run-test([cl-struct-ert-test viper-test-fix "Test that the viper
>   ert-run-or-rerun-test([cl-struct-ert--stats t [[cl-struct-ert-test v
>   ert-run-tests(t #[385 "\306\307\"\203G\211\211G\310U\203\211@\20
>   ert-run-tests-batch(t)
>   eval((ert-run-tests-batch t))
>   command-line-1(("-l" "../emacs/test/automated/viper-tests.el" "--eva
>   command-line()
>   normal-top-level()
> Test viper-test-fix condition:
>     (file-error "Opening stdio stream" "No such file or directory" 
> "c:/Users/Fabrice/AppData/Local/Temp/viper-tests21052GpM")
>    FAILED  1/6  viper-test-fix
>
> I'm not sure what is happening there.
> I checked the temp file is created and does exist, so why is there 
> a failure to open it?
>
> Regards,
>
> Fabrice


The file is only created to be empty (viper loads the custom-file
whatever I do, so making an empty one seemed the only way to disable it.

I've checked and the error comes from the viper-load-custom-file
function which is calle during load. The file exists according to emacs,
but it cannot be loaded.

Not sure what to test next.

Phil

Re: Viper tests failure?

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Date: Tue, 21 Jun 2016 15:27:28 +0100
> Cc: emacs-devel@gnu.org
> 
> > With emacs-25 commit e5e886d12f3eff3102916c82a5aa82e0db714579
> > Emacs compiled using Mingw64 I get a strange error
> > running `make check':
> >
> > $ src/emacs --batch -Q -l ../emacs/test/automated/viper-tests.el --eval '(ert-
> > run-tests-batch t)'
> > Running 6 tests (2016-06-21 07:01:58+0200)
> > Loading viper...
> > Test viper-test-fix backtrace:
> >   byte-code(\204'\306\307\310!CB\301    CB\302\nCB\303
> >                                                       CB\311\307\311!
> >   load("viper")
> >   load-library("viper")
> >   viper-mode()
> >   (progn (viper-mode) (switch-to-buffer (get-buffer-create "*viper-tes
> >   (unwind-protect (progn (viper-mode) (switch-to-buffer (get-buffer-cr
> >   (let ((noninteractive nil) (viper-inhibit-startup-message (quote t))
> >   viper-test-undo-kmacro([])
> >   apply(viper-test-undo-kmacro [])
> >   (setq value-4 (apply fn-2 args-3))
> >   (unwind-protect (setq value-4 (apply fn-2 args-3)) (setq form-descri
> >   (if (unwind-protect (setq value-4 (apply fn-2 args-3)) (setq form-de
> >   (let (form-description-6) (if (unwind-protect (setq value-4 (apply f
> >   (let ((value-4 (quote ert-form-evaluation-aborted-5))) (let (form-de
> >   (let ((fn-2 (function viper-test-undo-kmacro)) (args-3 (list []))) (
> >   (lambda nil (let ((fn-2 (function viper-test-undo-kmacro)) (args-3 (
> >   ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
> >   ert-run-test([cl-struct-ert-test viper-test-fix "Test that the viper
> >   ert-run-or-rerun-test([cl-struct-ert--stats t [[cl-struct-ert-test v
> >   ert-run-tests(t #[385 "\306\307\"\203G\211\211G\310U\203\211@\20
> >   ert-run-tests-batch(t)
> >   eval((ert-run-tests-batch t))
> >   command-line-1(("-l" "../emacs/test/automated/viper-tests.el" "--eva
> >   command-line()
> >   normal-top-level()
> > Test viper-test-fix condition:
> >     (file-error "Opening stdio stream" "No such file or directory" 
> > "c:/Users/Fabrice/AppData/Local/Temp/viper-tests21052GpM")
> >    FAILED  1/6  viper-test-fix
> >
> > I'm not sure what is happening there.
> > I checked the temp file is created and does exist, so why is there 
> > a failure to open it?
> >
> > Regards,
> >
> > Fabrice
> 
> 
> The file is only created to be empty (viper loads the custom-file
> whatever I do, so making an empty one seemed the only way to disable it.
> 
> I've checked and the error comes from the viper-load-custom-file
> function which is calle during load. The file exists according to emacs,
> but it cannot be loaded.
> 
> Not sure what to test next.

The problem is real.  There's a bug in Fload: it blindly overwrites
the last character of the file name, without making sure it ends in
".elc".  That's why the simple change below (which is really a
workaround) fixes the problem for me.

The bug is here:

          /* openp already checked for newness, no point doing it again.
             FIXME would be nice to get a message when openp
             ignores suffix order due to load_prefer_newer.  */
          if (!load_prefer_newer)
            {
              result = stat (SSDATA (efound), &s1);
              if (result == 0)
                {
                  SSET (efound, SBYTES (efound) - 1, 0);
                  result = stat (SSDATA (efound), &s2);
                  SSET (efound, SBYTES (efound) - 1, 'c');
                }

This removes the last character of the file, then replaces it with
'c', but we can end up in this fragment when the file name does not
end in a ".elc", because the condition which guards this says:

  if (suffix_p (found, ".elc") || (fd >= 0 && (version = safe_to_load_version (fd)) > 0))

and the second alternative does not guarantee that the file name ends
in ".elc".

The error message we see on Windows is misleading, because it uses the
correct file name, not the one Emacs tries to open.  The error only
happens on Windows, because we don't use fdopen there, but instead
close and reopen the file -- using the wrong name.

And here's the simple workaround I mentioned:

diff --git a/test/lisp/emulation/viper-tests.el b/test/lisp/emulation/viper-tests.el
index 074dd63..0d6095b 100644
--- a/test/lisp/emulation/viper-tests.el
+++ b/test/lisp/emulation/viper-tests.el
@@ -38,7 +38,7 @@ viper-test-undo-kmacro
         ;; Select an expert-level for the same reason.
         (viper-expert-level 5)
         ;; viper loads this even with -q so make sure it's empty!
-        (viper-custom-file-name (make-temp-file "viper-tests"))
+        (viper-custom-file-name (make-temp-file "viper-tests" nil ".elc"))
         (before-buffer (current-buffer)))
     (unwind-protect
         (progn

Re: Viper tests failure?

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:
>> The file is only created to be empty (viper loads the custom-file
>> whatever I do, so making an empty one seemed the only way to disable it.
>> 
>> I've checked and the error comes from the viper-load-custom-file
>> function which is calle during load. The file exists according to emacs,
>> but it cannot be loaded.
>> 
>> Not sure what to test next.
>
> The problem is real.  There's a bug in Fload: it blindly overwrites
> the last character of the file name, without making sure it ends in
> ".elc".  That's why the simple change below (which is really a
> workaround) fixes the problem for me.
>
> The bug is here:
>
>           /* openp already checked for newness, no point doing it again.
>              FIXME would be nice to get a message when openp
>              ignores suffix order due to load_prefer_newer.  */
>           if (!load_prefer_newer)
>             {
>               result = stat (SSDATA (efound), &s1);
>               if (result == 0)
>                 {
>                   SSET (efound, SBYTES (efound) - 1, 0);
>                   result = stat (SSDATA (efound), &s2);
>                   SSET (efound, SBYTES (efound) - 1, 'c');
>                 }
>
> This removes the last character of the file, then replaces it with
> 'c', but we can end up in this fragment when the file name does not
> end in a ".elc", because the condition which guards this says:
>
>   if (suffix_p (found, ".elc") || (fd >= 0 && (version = safe_to_load_version (fd)) > 0))
>
> and the second alternative does not guarantee that the file name ends
> in ".elc".
>
> The error message we see on Windows is misleading, because it uses the
> correct file name, not the one Emacs tries to open.  The error only
> happens on Windows, because we don't use fdopen there, but instead
> close and reopen the file -- using the wrong name.
>
> And here's the simple workaround I mentioned:
>
> diff --git a/test/lisp/emulation/viper-tests.el b/test/lisp/emulation/viper-tests.el
> index 074dd63..0d6095b 100644
> --- a/test/lisp/emulation/viper-tests.el
> +++ b/test/lisp/emulation/viper-tests.el
> @@ -38,7 +38,7 @@ viper-test-undo-kmacro
>          ;; Select an expert-level for the same reason.
>          (viper-expert-level 5)
>          ;; viper loads this even with -q so make sure it's empty!
> -        (viper-custom-file-name (make-temp-file "viper-tests"))
> +        (viper-custom-file-name (make-temp-file "viper-tests" nil ".elc"))
>          (before-buffer (current-buffer)))
>      (unwind-protect
>          (progn


Okay, I was wondering whether it was a problem with the suffix.

The viper-test is fix obviously fine by me -- should have done it that
way in the first place, although uncovering the Fload but is a nice side
effect. Can you add it to emacs-25 (I'm away from my windows machine,
and I'd rather not commit something I can't test).


Phil

Re: Viper tests failure?

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: fabrice.popineau@gmail.com,  emacs-devel@gnu.org
> Date: Tue, 21 Jun 2016 21:26:58 +0100
> 
> > --- a/test/lisp/emulation/viper-tests.el
> > +++ b/test/lisp/emulation/viper-tests.el
> > @@ -38,7 +38,7 @@ viper-test-undo-kmacro
> >          ;; Select an expert-level for the same reason.
> >          (viper-expert-level 5)
> >          ;; viper loads this even with -q so make sure it's empty!
> > -        (viper-custom-file-name (make-temp-file "viper-tests"))
> > +        (viper-custom-file-name (make-temp-file "viper-tests" nil ".elc"))
> >          (before-buffer (current-buffer)))
> >      (unwind-protect
> >          (progn
> 
> 
> Okay, I was wondering whether it was a problem with the suffix.

The final piece of the puzzle is that the code changes the _encoded_
file name, evidently on the assumption that the original file name is
unaffected.  But that assumption is false: when the file name is
all-ASCII, then some file-name encodings (notably, UTF-8) will return
the original Lisp string unaltered, so modifying it will also modify
the original.

> The viper-test is fix obviously fine by me -- should have done it that
> way in the first place, although uncovering the Fload but is a nice side
> effect.

I have now fixed this properly in Fload on master.  No change in
viper-tests is necessary.

> Can you add it to emacs-25

Doesn't sound like a good idea: the changes are not really trivial,
and this problem has been with us for so long that one more release is
not an issue.

Re: Viper tests failure?

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Cc: fabrice.popineau@gmail.com,  emacs-devel@gnu.org
>> Date: Tue, 21 Jun 2016 21:26:58 +0100
>> 
>> > --- a/test/lisp/emulation/viper-tests.el
>> > +++ b/test/lisp/emulation/viper-tests.el
>> > @@ -38,7 +38,7 @@ viper-test-undo-kmacro
>> >          ;; Select an expert-level for the same reason.
>> >          (viper-expert-level 5)
>> >          ;; viper loads this even with -q so make sure it's empty!
>> > -        (viper-custom-file-name (make-temp-file "viper-tests"))
>> > +        (viper-custom-file-name (make-temp-file "viper-tests" nil ".elc"))
>> >          (before-buffer (current-buffer)))
>> >      (unwind-protect
>> >          (progn
>> The viper-test is fix obviously fine by me -- should have done it that
>> way in the first place, although uncovering the Fload but is a nice side
>> effect.
>
> I have now fixed this properly in Fload on master.  No change in
> viper-tests is necessary.

I'd probably change it anyway, to make it clearer in the test that this
is an (empty) load file.


>> Can you add it to emacs-25
>
> Doesn't sound like a good idea: the changes are not really trivial,
> and this problem has been with us for so long that one more release is
> not an issue.

Apologies, I mean fix the test. I guess there is no problem with making
a change to test on emacs-25. It's also necessary to stop the test error
as the Fload fix is on master.

Phil

Re: Viper tests failure?

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: fabrice.popineau@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 22 Jun 2016 18:23:09 +0100
> 
> >> Can you add it to emacs-25
> >
> > Doesn't sound like a good idea: the changes are not really trivial,
> > and this problem has been with us for so long that one more release is
> > not an issue.
> 
> Apologies, I mean fix the test.

Feel free to do that.