Re: [lmi] Problems with the Cygwin tests

[Vadim Zeitlin]

On Wed, 8 May 2019 18:11:09 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2019-05-07 15:05, Vadim Zeitlin wrote:
GC> > On Tue, 7 May 2019 12:00:49 +0000 Greg Chicares <address@hidden> wrote:
GC> > 
GC> > GC> I'll let you know when I've finished all of that, and I
GC> 
GC> I've finished all of that.
GC> 
GC> > GC> hope that at that time you'll be able to retest everything in
GC> > GC> 'INSTALL', along with
GC> > GC>   ./nychthemeral_test
GC> > GC>   ./gui_test
GC> > GC> in cygwin so we'll know cygwin is fully supported.
GC> 
GC> Except that you might like first to look through the dozen or so commits
GC> that I just pushed, because I could have missed something important, or
GC> in the last commit I may have followed shellcheck's recommendations too
GC> zealously. But everything builds and tests out perfectly here.

 It looks good for me too, thanks!

GC> >  Here are the testing results:
GC> > 
GC> >  For the gui_test:
GC> >  
GC> > 1. LMI_COMPILER=gcc LMI_TRIPLET=i686-w64-mingw32 need to be explicitly set
GC> >    in the environment. I don't know if this is expected, i.e. I thought 
the
GC> >    script would autodetect their values, but I could well have been wrong.
GC> 
GC> For now, it's a good idea to set both:
GC>   export LMI_COMPILER=gcc
GC>   export LMI_TRIPLET=i686-w64-mingw32

 OK.

GC> For the long term...I'm struggling with this issue. It's the only major
GC> difference between trunk and foo/multiarch (or it should be, once I merge
GC> trunk into foo/multiarch again). I'm not sure which one is better:
GC> 
GC>  - foo/multiarch consolidates all of {$PATH, $WINEPATH, $PERFORM, etc.}
GC>    into one script, and calls that script wherever it seems sensible.
GC>    To switch architectures, you need to do something like:
GC>      $export LMI_COMPILER=gcc ; export LMI_TRIPLET=x86_64-w64-mingw32 ; . 
./set_arch.sh
GC>    but the reward for that inconvenience is that the script is "perfect"
GC>    AFAICT, so all of the settings it controls are always consistent.
GC> 
GC>  - trunk uses no such script. Required settings are stored in makefiles
GC>    and scripts, exactly where they're required (except of course for the
GC>    imperfection you point out). There's a certain righteousness in setting
GC>    $WINEPATH only in 'msw_generic.make', because it's unnecessary for
GC>    cygwin. And if the makefiles and scripts are perfected, then perhaps
GC>    we don't ever need to think specify anything like LMI_TRIPLET on the
GC>    command line: we'd specify it iff we are deliberately switching to a
GC>    different architecture, and simply executing
GC>      LMI_TRIPLET=x86_64-w64-mingw32
GC>    should make it all just work. Or that's the dream.
GC> 
GC> If one of those two alternatives seems obviously right to you, please
GC> let me know, because I'm still torn between them. I'm starting to think
GC> that 'set_arch.sh' is preferable.

 I do think it's preferable, although I'd still rather call it
set_lmi_env.sh or something like this, because it doesn't (or at least
won't) set only the architecture (but also the compiler) and because it's
specific to lmi and not generic.

 My main problem with the latter approach is that it fails out of the box.
I could live with it if it at least gave an error saying that LMI_XXX must
be defined (and maybe giving an example of how they could be defined), but
right now it really seems suboptimal.

GC> >  With these caveats, the GUI test passes:
GC> > 
GC> >   time=51305ms (for all tests)
GC> >   SUCCESS: 21 tests successfully completed.
GC> >   NOTE: 4 tests were skipped
GC> 
GC> Excellent. I don't suppose we can make this faster except by making lmi
GC> itself faster.

 BTW, the full installation takes ~40 minutes on Ilya's VM.

GC> > 6. "system_test" doesn't work, but this is expected.
GC> 
GC> Yes. Probably I should put some nonproprietary testdecks in the public
GC> git repository, so that this would just work. But not today.

 There is obviously no hurry, but it would be nice, yes.

GC> >  Beyond that, there are some errors in the log, but I don't know if 
they're
GC> > expected or not: as I had already complained about the unit_tests target,
GC> > there is too much output and no summary at the end allowing to see at a
GC> > glance whether the execution was successful or not.
GC> 
GC> The idea is that all unit tests succeed all the time, so filtering out
GC> everything that worked correctly leaves nothing: "no news is good news".

 Sorry to insist, but I really don't think it's the best approach. IMO
normal output should be clearly separated from the error output.

GC> >  Another one:
GC> > 
GC> >   # test all valid emission types
GC> > 
GC> >   Unable to parse xml file '/tmp/lmi/tmp/sample.ill': File does not exist.
GC> >   [xml_lmi.cpp : 69]
GC> > 
GC> >   Unable to parse xml file '/tmp/lmi/tmp/sample.cns': File does not exist.
GC> >   [xml_lmi.cpp : 69]
GC> > 
GC> > This is confusing because the file does exist. However running the command
GC> > from the test manually:
GC> > 
GC> >   /opt/lmi/bin/lmi_cli_shared --file=/tmp/lmi/tmp/sample.ill --accept 
--ash_nazg --data_path=/opt/lmi/data 
--emit=emit_test_data,emit_spreadsheet,emit_text_stream,emit_custom_0,emit_custom_1
GC> > 
GC> > results in the same error message while copying the file to /opt/lmi/data
GC> > and using --file=sample.ill works fine, so something seems to be fishy
GC> > here, but I haven't had yet time to investigate.
GC> 
GC> I don't suppose the cause could be a quoting error that I've now fixed, 
viz.:
GC> 
GC> -cp --preserve $srcdir/sample.cns $srcdir/sample.ill .
GC> +cp --preserve "$srcdir"/sample.cns "$srcdir"/sample.ill .

 No. In fact, looking at it again the error looks rather obvious:
lmi_cli_shared.exe is a native, and not Cygwin, application, so for it /tmp
is the same as c:\tmp and this directory plainly doesn't exist. Under
Cygwin /tmp is c:\cygwin64-lmi\tmp, of course, which does exist.

 The simplest fix here is to just stop using the absolute paths and use
--file=sample.ill instead, as this command is being run from /tmp/lmi/tmp
already. But I might be missing something here because I don't see why
would you have added the absolute paths in the first place if they were so
unnecessary. Would you remember why was this done?

 Anyhow, if you don't see anything wrong with just removing the path from
the value of the --file option, it would be the simplest fix. If you'd like
to keep using the absolute paths, we need to either use DOS/mixed-style
paths here or actually create c:\tmp (which doesn't seem desirable).

 Please let me know what do you think and we'll rerun the tests with the
latest master in the meanwhile.

 Thanks,
VZ