GNOME Bugzilla – Bug 795324
meson: Minor improvements for tests
Last modified: 2018-04-18 06:24:14 UTC
SSIA
Created attachment 371020 [details] [review] meson: Move one of the have_apache check to get_apache_module_dirs script Fail already in get_apache_module_dirs.py when it failed to found Apache module directory or Apache mod_ssl directory.
Created attachment 371021 [details] [review] meson: Use already set local variable To avoid looking for its value in configuration data.
Created attachment 371022 [details] [review] meson: Avoid creating a local variable for mod_ssl directory It's later used only for setting it to configuration data. Also reorganize the code a little bit.
Created attachment 371023 [details] [review] meson: Only compile and install test the have all the prerequisites This is to avoid situations when you don't have Apache, but the test that require Apache will be compiled and installed, the same applies for xmlrpc.
Review of attachment 371023 [details] [review]: ::: tests/meson.build @@ +51,3 @@ + ['xmlrpc-old', false, have_php_xmlrpc], + ['xmlrpc-server', true, have_php_xmlrpc], + ['xmlrpc', false, have_php_xmlrpc] Instead of using a compilation condition, why not split the array in groups? Something like: tests = [ ['cache', true] ['chunk', true] ... ] if have_apache tests += [ ['auth', false] ['connection', false] ... ] endif if have_php_xmlrpc tests += [ [xmlrpc-old-server', true] ... ] endif @@ +85,3 @@ + output : 'xmlrpc-server.php', + configuration : configuration_data()) +endif If the list is split in groups, this can go in its group. @@ +109,3 @@ + output : 'test-key.pem', + configuration : configuration_data()) +endif This can also go in its own group.
Review of attachment 371020 [details] [review]: I don't know enough about apache, and seems that dealing with apache is not an easy task. Isn't there anything that can help you with it? AFAIK, `apachectl`, which I don't know if its present in every distro, might help you getting some information from it. However, it might not be enough. Otherwise, these patches looks good to me. ::: meson.build @@ +134,3 @@ cdata.set('IF_HAVE_MOD_UNIXD', apache_mod_unixd.returncode() == 0 ? '' : '#') + else + have_apache = false What about the following? have_apache = (apache_module_dirs.returncode() == 0) if have_apache ... endif This way `have_apache` will have the value of the condition it depends on, so there is no need to set it as false later.
(In reply to Iñigo Martínez from comment #5) > Review of attachment 371023 [details] [review] [review]: > > ::: tests/meson.build > @@ +51,3 @@ > + ['xmlrpc-old', false, have_php_xmlrpc], > + ['xmlrpc-server', true, have_php_xmlrpc], > + ['xmlrpc', false, have_php_xmlrpc] > > Instead of using a compilation condition, why not split the array in groups? That's really a better option :) (In reply to Iñigo Martínez from comment #6) > Review of attachment 371020 [details] [review] [review]: > > I don't know enough about apache, and seems that dealing with apache is not > an easy task. Isn't there anything that can help you with it? > > AFAIK, `apachectl`, which I don't know if its present in every distro, might > help you getting some information from it. However, it might not be enough. I would avoid this for Apache it's only about checking whether some files are on the disk.
Created attachment 371031 [details] [review] meson: Move one of the have_apache check to get_apache_module_dirs script Fail already in get_apache_module_dirs.py when it failed to found Apache module directory or Apache mod_ssl directory.
Created attachment 371032 [details] [review] meson: Avoid creating a local variable for mod_ssl directory It's later used only for setting it to configuration data. Also reorganize the code a little bit.
Created attachment 371033 [details] [review] meson: Only compile and install test the have all the prerequisites This is to avoid situations when you don't have Apache, but the test that require Apache will be compiled and installed, the same applies for xmlrpc.
Created attachment 371034 [details] [review] meson: Only compile and install test the have all the prerequisites This is to avoid situations when you don't have Apache, but the test that require Apache will be compiled and installed, the same applies for xmlrpc.
Review of attachment 371031 [details] [review]: Sure!
Review of attachment 371034 [details] [review]: Nice!
Created attachment 371048 [details] [review] meson: Better use the output of get_apache_module_dirs.py * Use join_paths() for appending to path * Avoid creating a local variables for directories by saving the script output that is split by the separator Suggested by Iñigo Martínez.
Review of attachment 371048 [details] [review]: LGTM
Attachment 371031 [details] pushed as 1e1b4c1 - meson: Move one of the have_apache check to get_apache_module_dirs script Attachment 371034 [details] pushed as 964e130 - meson: Only compile and install test the have all the prerequisites Attachment 371048 [details] pushed as 8bcab0a - meson: Better use the output of get_apache_module_dirs.py