GNOME Bugzilla – Bug 701241
standardise --enable-installed-tests
Last modified: 2018-05-24 15:22:01 UTC
I have a branch that aims to standardise the handling of installed tests across all modules by way of introducing a new m4 macro and expanding Makefile.decl. This new functionality facilitates a substantial cleanup of glib's Makefiles. The m4 macro isn't installed yet. I'd rather keep this on a copy-paste basis for now (as is already the case with Makefile.decl). Please see https://git.gnome.org/browse/glib/log/?h=wip/test-cleanup
As per #701262 this is broken when cross-compiling: AS_IF([ test $cross_compiling = yes && test x$enable_modular_tests = xyes], [ $enable_modular_tests isn't defined anymore (it's also used in the dbus check).
(In reply to comment #0) > I have a branch that aims to standardise the handling of installed tests across > all modules by way of introducing a new m4 macro and expanding Makefile.decl. see also bug 654395, which creates an installed Makefile.glib, to standardize non-test-related rules, which gets pulled in with the help of an m4 macro. I had pondered pulling bits of Makefile.decl into that as well though...
I'm a bit confused about exactly what is desired for cross-compilation: (a) in case we have --enable-installed-tests it seems pretty clear to me that we should install the tests built for the target machine (b) in case we don't have that, if someone doesn't type 'make check' then tests will not be built or run at all (under the new work in the branch). (c) in case you _do_ type 'make check' when cross-building, you will compile and attempt to run the tests for the target architecture on your build machine. In many cases this will work properly (like qemu transparent emulation or when doing cross-builds for Windows with wine installed). If this would not work properly for you then you should not type 'make check'. Are you looking for an option that will allow you to type 'make check' and have it do nothing?
All of (a) (b) and (c) are correct. In my environment we're currently hacking something like installed-tests using the old modular tests and we'll swap that for installed tests shortly. Running make check in a cross environment generally fails, if you've got qemu emulation you're not really cross-compiling. My point was that there is a bug in the configure script, it's using variables that don't exist.
(In reply to comment #4) > My point was that there is a bug in the configure script, it's using variables > that don't exist. So since this branch deletes the code with the bug, no complaints?
$ git checkout origin/wip/test-cleanup HEAD is now at 491ee49... Rework the build system for a new tests approach $ git grep modular_tests configure.ac:AS_IF([ test $cross_compiling = yes && test x$enable_modular_tests configure.ac:AS_IF([ test x$enable_modular_tests = xyes], [ Nothing is setting $enable_modular_tests in the branch as AC_ARG_ENABLE(modular_tests) was deleted, but there are two checks for it's value.
ah! Now I understand. Thanks :)
So the real issue that remains, then, is if people want a way to configure glib for cross-compiling on a system that does not already have glib installed (ie: no system version of genmarshal and schema/resource compilers). So there are a few options here: 1) as now, fail the configure unless you --disable-modular-tests (which would be incompatible with --enable-installed-tests) 2) fail the configure only in the case that you --enable-installed-tests and fail 'make check' later, if you type it. 3) disable the tests that depend on the unavailable tools Out of these options, I think I prefer 2.
I'm happy with the current situation of needing them to be provided as glib is sufficiently low in the stack that it's generally going be needed for the host anyway (unlike gtk+, where I contributed a patch to make it build gtk-update-icon-cache for the host and target in the cross-compilation case). We really do want a way of building the installable tests in a cross-compilation environment as the traditional "make check" tests obviously don't work.
Right. That's the fourth option and perhaps best of all... You're going to be able to build glib, but you won't get far after that, so we may as well just require it.
There's still the option of disabling the tests and not needing existing tools right? If we're not using the tests it's nice to not need a host glib.
So if we're planning to export glib's Makefile.decl for the whole world to use, can someone explain what the difference between "make test" and "make check" is supposed to be? > By default, tests are no longer compiled as part of 'make'. And so the main argument I remember for having $(top_srcdir)/tests/ goes away, and we should start migrating all those tests into {glib,gobject,gio}/tests/. > The long-running GObject performance tests in this directory have been > removed from 'make check' because they take too long. We should eventually port all of these to gtester and run them unconditionally, but have them check g_test_perf() and exit(77) if it's not set. (And gtester should have a #define for 77...) > There is one regression in this patch: the appinfo tests can no longer > be installed appinfo-test is kind of an abomination anyway... >#GTESTER = gtester # for non-GLIB packages >GTESTER = $(top_builddir)/glib/gtester # for the GLIB package >GTESTER_REPORT = $(top_builddir)/glib/gtester-report # for the GLIB package GLIB_TEST should handle this. (And it needs to correctly set both GTESTER and GTESTER_REPORT.) >+noinst_PROGRAMS = ... So... Makefile.decl is already initializing EXTRA_DIST, and now these other things... Should we just bite the bullet and initialize BUILT_SOURCES, CLEANFILES, and DISTCLEANFILES too? (Any others?) >+%.test: %$(EXEEXT) Makefile >+ $(AM_V_GEN) (echo '[Test]' > $@.tmp; \ >+ echo 'Type=session' >> $@.tmp; \ ... It probably doesn't actually matter in this rule, but it's good practice to use "&&" rather than ";" >+dist_test_data = \ >+ $(wildcard cert-tests/*) \ >+ $(wildcard schema-tests/*) \ that will result in emacs backup files and core dumps and stuff getting disted. (Likewise in glib/tests/Makefile.am.) >+noinst_PROGRAMS += \ >+ resolver \ ... >+ gdbus-example-export \ I'm not sure to what extent the gdbus and gapplication example programs are useful as non-automated tests, but if they're really just examples, maybe we should move them out into $(top_srcdir)/examples/ or something. Also, if you're completely rewriting all the test program lists in all the Makefiles, please alphabetize the tests, so that in the future, when adding new tests, there's only a 1/26(-ish) chance that you'll hit a conflict with another new test when rebasing, rather than a 100% chance like we have now with everyone always adding to the bottom of the list. >+test_data += \ >+ appinfo-test.desktop \ >... >+test_extra_programs += \ > appinfo-test \ Since appinfo is uninstalled, these should be as well, right? Including "file.c" in test_data is kinda random. It would be clearer if we just installed a small text file for gdbus-peer to open. >+ AM_CONDITIONAL([ENABLE_INSTALLED_TESTS], test x$ENABLE_INSTALLED_TESTS == x1) "=", not "==" (likewise in several other places). ("=" is standard sh, "==" is a bash-ism) Also, I'm more a fan of test "$ENABLE_INSTALLED_TESTS" = "1" than test x$ENABLE_INSTALLED_TESTS = x1 >+# These don't appear to work installed and we don't want to run them under gtester either... >+dist_uninstalled_test_extra_scripts = \ >+ run-collate-tests.sh \ >+ run-assert-msg-test.sh \ There's no reason they couldn't work... maybe add "currently" ("These don't currently appear to work") to clearly imply that someone should fix them. >+# Run the 'installed' tests manually in-tree. >+# This will cause them to be built even if installed tests are disabled. >+check_PROGRAMS += $(installed_test_programs) $(installed_test_extra_programs) >+check_SCRIPTS += $(installed_test_scripts) why isn't this handled by Makefile.decl? And actually, Makefile.decl needs to clarify which parts of it can only be used with gtestutils-based tests. >+if ENABLE_TIMELOOP >+installed_test_programs += timeloop >+endif This appears to be a program that Owen wrote to make sure that GMainContext was not slower than the old glib 1.x main loop. It's probably safe to get rid of (along with timeloop-basic.c). If not, it should just be treated like all the other performance tests.
(In reply to comment #12) > So if we're planning to export glib's Makefile.decl for the whole world to use, > can someone explain what the difference between "make test" and "make check" is > supposed to be? I don't think we need more than one target to run tests. make check is just fine for me.
(In reply to comment #12) > >+ gdbus-example-export \ > > I'm not sure to what extent the gdbus and gapplication example programs are > useful as non-automated tests, but if they're really just examples, maybe we > should move them out into $(top_srcdir)/examples/ or something. > > Also, if you're completely rewriting all the test program lists in all the > Makefiles, please alphabetize the tests, so that in the future, when adding new > tests, there's only a 1/26(-ish) chance that you'll hit a conflict with another > new test when rebasing, rather than a 100% chance like we have now with > everyone always adding to the bottom of the list. Both of these are good suggestions. > > >+# These don't appear to work installed and we don't want to run them under gtester either... > >+dist_uninstalled_test_extra_scripts = \ > >+ run-collate-tests.sh \ > >+ run-assert-msg-test.sh \ > > There's no reason they couldn't work... maybe add "currently" ("These don't > currently appear to work") to clearly imply that someone should fix them. I'm pretty sure I had them working installed at some point.
(In reply to comment #12) > So if we're planning to export glib's Makefile.decl for the whole world to use, > can someone explain what the difference between "make test" and "make check" is > supposed to be? 'make check' is the automake rule for make check. 'make test' is gtester's rule for "run the gtester tests". > > By default, tests are no longer compiled as part of 'make'. > > And so the main argument I remember for having $(top_srcdir)/tests/ goes away, > and we should start migrating all those tests into {glib,gobject,gio}/tests/. I agree with this. Note: I want to make sure we have at least one small program in each subdir that does a link against each library (just to catch unresolved symbols). In some dirs, our various utilities will already do this for us. > > The long-running GObject performance tests in this directory have been > > removed from 'make check' because they take too long. > > We should eventually port all of these to gtester and run them unconditionally, > but have them check g_test_perf() and exit(77) if it's not set. (And gtester > should have a #define for 77...) What is 77? > > There is one regression in this patch: the appinfo tests can no longer > > be installed > > appinfo-test is kind of an abomination anyway... I think this will be easy to fix, actually... can just use chdir()... > GLIB_TEST should handle this. (And it needs to correctly set both GTESTER and > GTESTER_REPORT.) Ah! Great point! It's a pretty glib-specific feature, though... and it's also tied up in cross-compiling... we'll probably want to use the host's version of gtester even if we're running tests under some sort of emulation layer. > >+noinst_PROGRAMS = > ... > > So... Makefile.decl is already initializing EXTRA_DIST, and now these other > things... Should we just bite the bullet and initialize BUILT_SOURCES, > CLEANFILES, and DISTCLEANFILES too? (Any others?) After some further work, I consider BUILT_SOURCES to be an anti-pattern in almost all cases. I've only found one case where it's useful, which is pretty weird. In gio/tests/object-manager-example we want to make sure we always generate the .c/.h/.xml output in the case that ENABLE_GTK_DOC because these files will be pulled into the doc build... In every other case, I think it's better to add an explicit depend. > >+%.test: %$(EXEEXT) Makefile > >+ $(AM_V_GEN) (echo '[Test]' > $@.tmp; \ > >+ echo 'Type=session' >> $@.tmp; \ > ... > > It probably doesn't actually matter in this rule, but it's good practice to use > "&&" rather than ";" Fair. This was just copy-paste. > >+dist_test_data = \ > >+ $(wildcard cert-tests/*) \ > >+ $(wildcard schema-tests/*) \ > > that will result in emacs backup files and core dumps and stuff getting disted. > (Likewise in glib/tests/Makefile.am.) I guess maybe we could match on a more specific pattern. I'd rather avoid having those huge lists of files, though... > >+noinst_PROGRAMS += \ > >+ resolver \ > ... > >+ gdbus-example-export \ > > I'm not sure to what extent the gdbus and gapplication example programs are > useful as non-automated tests, but if they're really just examples, maybe we > should move them out into $(top_srcdir)/examples/ or something. Ya. I'm starting to agree with this more and more. The problem is that the line between "uninstalled manual test" and "example program" is pretty blurry... > Also, if you're completely rewriting all the test program lists in all the > Makefiles, please alphabetize the tests, so that in the future, when adding new > tests, there's only a 1/26(-ish) chance that you'll hit a conflict with another > new test when rebasing, rather than a 100% chance like we have now with > everyone always adding to the bottom of the list. OK. Note that I'm developing a bit of a new policy for this, which I think is sane. Let's say there are two kinds of tests: 'simple' and 'not simple'. Simple tests are ones with a single .c file and no need for extra options. These ones can just get added to the main test_programs list. Non-simple tests are any test that requires more than a single line in the Makefile. These never go in the main list, but instead go somewhere below, and appear like so: test_programs += foo foo_CFLAGS = ... foo_SOURCES = ... CLEANFILES += file-generated-by-foo-test So that every line in the Makefile that is there on account of a specific test is very clearly related to said test. > >+test_data += \ > >+ appinfo-test.desktop \ > >... > >+test_extra_programs += \ > > appinfo-test \ > > Since appinfo is uninstalled, these should be as well, right? See another reason why the above is useful... > Including "file.c" in test_data is kinda random. It would be clearer if we just > installed a small text file for gdbus-peer to open. And another... > >+ AM_CONDITIONAL([ENABLE_INSTALLED_TESTS], test x$ENABLE_INSTALLED_TESTS == x1) > > "=", not "==" (likewise in several other places). ("=" is standard sh, "==" is > a bash-ism) OK > Also, I'm more a fan of > test "$ENABLE_INSTALLED_TESTS" = "1" > than > test x$ENABLE_INSTALLED_TESTS = x1 agree. again, copy-paste job. > >+# These don't appear to work installed and we don't want to run them under gtester either... > >+dist_uninstalled_test_extra_scripts = \ > >+ run-collate-tests.sh \ > >+ run-assert-msg-test.sh \ > > There's no reason they couldn't work... maybe add "currently" ("These don't > currently appear to work") to clearly imply that someone should fix them. Ya.. I start to think that the next step is to go through all of the tests with the aim of moving everything to test_programs, either by 'fixing' or by gtesterifying or removing unix-onlyisms, etc. I bet we have a lot of testcases that are not run in scenarios where they easily could be. > >+# Run the 'installed' tests manually in-tree. > >+# This will cause them to be built even if installed tests are disabled. > >+check_PROGRAMS += $(installed_test_programs) $(installed_test_extra_programs) > >+check_SCRIPTS += $(installed_test_scripts) > > why isn't this handled by Makefile.decl? Makefile.decl has no concept of in-tree tests that are not run under gtester. The correct solution to this problem is to port those tests to gtester. > And actually, Makefile.decl needs to clarify which parts of it can only be used > with gtestutils-based tests. See above. Anything in test_programs/scripts or uninstalled_test_programs/scripts (and variants) is assumed to be good for gtester use. This is specifically why I use only installed_* in the toplevel tests/ directory for now. > >+if ENABLE_TIMELOOP > >+installed_test_programs += timeloop > >+endif > > This appears to be a program that Owen wrote to make sure that GMainContext was > not slower than the old glib 1.x main loop. It's probably safe to get rid of > (along with timeloop-basic.c). If not, it should just be treated like all the > other performance tests. K.
(In reply to comment #15) > 'make check' is the automake rule for make check. 'make test' is gtester's > rule for "run the gtester tests". but Makefile.decl causes "make check" to run "make test". So... I guess "make check" means "test everything" and "make test" means "only run gtester tests"? I guess if "test" is intended to be an internal-only rule that makes sense, but if it is, it should have a less generic name. ("run-gtester"?) > What is 77? automake-speak for "skipped". So that you can explicitly see in the test output that there are test programs that aren't being run in the current configuration. > > appinfo-test is kind of an abomination anyway... > > I think this will be easy to fix, actually... can just use chdir()... I was complaining about how it pops up windows at you. > Ah! Great point! It's a pretty glib-specific feature, though... gtester-report? There's no reason it needs to be glib-specific though. If it's useful for glib, it should be useful for other packages too. > After some further work, I consider BUILT_SOURCES to be an anti-pattern in > almost all cases. So what would you do for foo-enum-types.h? > I guess maybe we could match on a more specific pattern. I'd rather avoid > having those huge lists of files, though... Even if you match on a specific pattern, you still risk picking up files you shouldn't. Eg, if I add a new file on a branch, forget to "git add" it before committing, then switch back to master. No one ever says "libfoo_la_SOURCES = $(wildcard *.c)". This isn't any different. > Non-simple tests are any test that requires more than a single line in the > Makefile. These never go in the main list, but instead go somewhere below in alphabetical order? :-) (the policy sounds like a good idea)
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/708.