Re: [lmi] Tests PR 174 (was: Can't get GUI test to run for pc-linux-gnu)

[Vadim Zeitlin]

On Wed, 24 Mar 2021 00:45:07 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 3/23/21 10:00 PM, Vadim Zeitlin wrote:
[...]
GC> > I'd like to ask you once again if you could please merge the PR fixing the
GC> > tests with autotools and adding them to the CI builds from
GC> > https://github.com/let-me-illustrate/lmi/pull/174
GC> Maybe I can look at it tomorrow. At a quick glance, it seems
GC> that the changes span several categories.

 Yes, and most of them shouldn't need much, or maybe even any, review at
all, as they only touch GitHub CI and autotools files. I'm not sure if it's
worth duplicating what I've written in

https://lists.nongnu.org/archive/html/lmi/2021-03/msg00062.html
https://lists.nongnu.org/archive/html/lmi/2021-03/msg00067.html

but, to summarize, the only changes you should need to review are those to
alert_cli.cpp and, just because I know you're very apprehensive of it, the
commit updating wx submodule.

GC> Some have pervasive effect, but are simple ('alert_cli.cpp').

 This is the most complicated change here outside of GitHub/autotools ones.

GC> I don't know whether
GC> I even have the tools to test this change in 'boost_regex.hpp':
GC>   -#   if 9 <= __GNUC__
GC>   +#   if 10 <= __GNUC__

 The fact that the CI build, using gcc 9, passes, should give some credit
to the idea that this change is not totally wrong.

GC> but I haven't read the commit messages yet (and it looks like
GC> there are twenty-eight of them).

 Only the following 7 commits touch any files but GitHub/autotools ones:

6c4024313 Avoid -Wsometimes-uninitialized clang warning in a test
fc1fcd230 Ignore more warnings in Boost.Regex headers with clang
26d8c16f9 Update wx submodule to avoid clang warning in wx/hashmap.h
817032ffd Show config.log if running configure for XML libraries failed
03cfe334f Suppress -Wdeprecated-copy in Boost headers for gcc9 too
199b5c92f Use gcc_version value from the environment, if any, in scripts
d9204c724 Force linking CLI alert initialization functions into the tests

GC> At least one change affects only a unit test ('numeric_io_test.cpp'),

 This is 6c4024313 and it is indeed trivial. So is fc1fcd230 which is quite
similar. And both of them only affect clang anyhow. 03cfe334f is just as
simple, but does affect gcc.

GC> OTOH, three scripts that rebuild libraries have changed;

 They've changed in extremely trivial and minor ways, as you can see from
looking at 199b5c92f and, by default, there is no change at all as the
added condition does nothing if gcc_version environment variable is not
defined (which is not the case normally). FWIW I'm not completely happy
about this change, but it is the simplest one possible. A better change
might be to fix show_gcc_version makefile target to work for clang too, but
I didn't want to open this can of worms: e.g. should we even still call it
gcc_version in this case, or should it be renamed to compiler_version (as I
think) and so on. By doing just what 199b5c92f does, I can predefine
gcc_version in the GitHub CI script and avoid any other changes to lmi.

 Oh, and there is 817032ffd but it just adds some diagnostics in case of
configure failure and it is not, of course, supposed to fail. It was useful
to me while debugging GitHub CI workflow and I thought it could be useful
again in the future, but if it can make your life simpler to not apply it,
please feel free to just drop it.

GC> to test them, I'll have to do a complete reinstallation,
GC> and I'll have to do it on the corporate server as well as my own
GC> machine.

 Sure, it's probably wiser to do this rather than just rely on the fact
that these changes are too trivial to affect anything, but, again, the fact
the CI build which does do complete reinstallation in a pristine
environment passes should inspire some confidence that it should work.

GC> And it looks like you've updated the wx version. To us, that's
GC> a large manual testing effort.

 I really hope you could be convinced that it is not needed for this
particular change. I am going to end up by repeating what I wrote in a
previous message, but the change here is completely trivial. I didn't
update to the latest (pre-3.1.5) wx version, but just added a single
trivial commit affecting clang only to the existing version.

GC> Is that commit separable from the rest? Looking in the commit
GC> messages...is the only purpose to avoid a clang warning?

 Yes. And before you say that you prefer to do at GitHub CI or configure
level: this could be done, but IMO it would be a pity to suppress this
warning in other places, even if only inside wx. It can indicate real
problems and so I'd like to keep it enabled.

 Also, one of the advantages of switching to submodules was to make it
simple to make arbitrarily small changes to them, instead of being
restricted to the upstream commits. I'd like to use the extra flexibility
that this gives us with this simplest possible update, just to check that
you're everybody is comfortable with it. Of course, this passes by actually
convincing you to be comfortable and, judging from my recent track record,
this is not a done deal. But I'd like to at least try, so let me say once
again that this update of wx is truly trivial and can't possibly change any
run-time, or even compile-time, for gcc, behaviour of anything. Do you
still object to it even so?

 In any case, thanks for looking at this!
VZ

pgp_HZkvwhHoz.pgp
Description: PGP signature