After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 795324 - meson: Minor improvements for tests
meson: Minor improvements for tests
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: Misc
unspecified
Other All
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks:
 
 
Reported: 2018-04-17 09:40 UTC by Tomas Popela
Modified: 2018-04-18 06:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
meson: Move one of the have_apache check to get_apache_module_dirs script (1.92 KB, patch)
2018-04-17 09:45 UTC, Tomas Popela
none Details | Review
meson: Use already set local variable (1.00 KB, patch)
2018-04-17 09:45 UTC, Tomas Popela
none Details | Review
meson: Avoid creating a local variable for mod_ssl directory (1.46 KB, patch)
2018-04-17 09:45 UTC, Tomas Popela
none Details | Review
meson: Only compile and install test the have all the prerequisites (5.56 KB, patch)
2018-04-17 09:45 UTC, Tomas Popela
none Details | Review
meson: Move one of the have_apache check to get_apache_module_dirs script (2.14 KB, patch)
2018-04-17 11:22 UTC, Tomas Popela
committed Details | Review
meson: Avoid creating a local variable for mod_ssl directory (1.44 KB, patch)
2018-04-17 11:22 UTC, Tomas Popela
none Details | Review
meson: Only compile and install test the have all the prerequisites (4.01 KB, patch)
2018-04-17 11:23 UTC, Tomas Popela
none Details | Review
meson: Only compile and install test the have all the prerequisites (4.07 KB, patch)
2018-04-17 11:45 UTC, Tomas Popela
committed Details | Review
meson: Better use the output of get_apache_module_dirs.py (2.23 KB, patch)
2018-04-17 13:37 UTC, Tomas Popela
committed Details | Review

Description Tomas Popela 2018-04-17 09:40:50 UTC
SSIA
Comment 1 Tomas Popela 2018-04-17 09:45:26 UTC
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.
Comment 2 Tomas Popela 2018-04-17 09:45:32 UTC
Created attachment 371021 [details] [review]
meson: Use already set local variable

To avoid looking for its value in configuration data.
Comment 3 Tomas Popela 2018-04-17 09:45:38 UTC
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.
Comment 4 Tomas Popela 2018-04-17 09:45:44 UTC
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.
Comment 5 Iñigo Martínez 2018-04-17 10:30:26 UTC
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.
Comment 6 Iñigo Martínez 2018-04-17 10:52:51 UTC
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.
Comment 7 Tomas Popela 2018-04-17 11:07:28 UTC
(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.
Comment 8 Tomas Popela 2018-04-17 11:22:25 UTC
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.
Comment 9 Tomas Popela 2018-04-17 11:22:59 UTC
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.
Comment 10 Tomas Popela 2018-04-17 11:23:10 UTC
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.
Comment 11 Tomas Popela 2018-04-17 11:45:47 UTC
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.
Comment 12 Iñigo Martínez 2018-04-17 13:06:08 UTC
Review of attachment 371031 [details] [review]:

Sure!
Comment 13 Iñigo Martínez 2018-04-17 13:07:34 UTC
Review of attachment 371034 [details] [review]:

Nice!
Comment 14 Tomas Popela 2018-04-17 13:37:56 UTC
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.
Comment 15 Iñigo Martínez 2018-04-17 14:42:36 UTC
Review of attachment 371048 [details] [review]:

LGTM
Comment 16 Tomas Popela 2018-04-18 06:24:00 UTC
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