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 784212 - Initial version of a meson based build system
Initial version of a meson based build system
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: Misc
unspecified
Other All
: Normal enhancement
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks: 782980
 
 
Reported: 2017-06-26 11:59 UTC by Sebastian Dröge (slomo)
Modified: 2018-04-17 07:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Initial version of a meson based build system (9.56 KB, patch)
2017-06-26 11:59 UTC, Sebastian Dröge (slomo)
none Details | Review
Initial version of a meson based build system (9.87 KB, patch)
2017-06-26 14:26 UTC, Sebastian Dröge (slomo)
none Details | Review
Initial version of a meson based build system (9.88 KB, patch)
2017-06-26 14:33 UTC, Sebastian Dröge (slomo)
none Details | Review
Initial version of a meson based build system (10.55 KB, patch)
2017-06-26 14:46 UTC, Sebastian Dröge (slomo)
none Details | Review
Initial version of a meson based build system (10.64 KB, patch)
2017-06-26 14:54 UTC, Sebastian Dröge (slomo)
needs-work Details | Review
Support for GSSAPI and NTLM (2.76 KB, patch)
2017-10-03 08:53 UTC, Tomas Popela
needs-work Details | Review

Description Sebastian Dröge (slomo) 2017-06-26 11:59:23 UTC
See summary
Comment 1 Sebastian Dröge (slomo) 2017-06-26 11:59:26 UTC
Created attachment 354501 [details] [review]
Initial version of a meson based build system

Does not build libsoup-gnome and exclude gssapi and ntlm_auth, and also
misses other features of the autotools build system. But generates a
useable library.
Comment 2 Sebastian Dröge (slomo) 2017-06-26 14:26:30 UTC
Created attachment 354505 [details] [review]
Initial version of a meson based build system

Does not build libsoup-gnome and exclude gssapi and ntlm_auth, and also
misses other features of the autotools build system. But generates a
useable library.
Comment 3 Sebastian Dröge (slomo) 2017-06-26 14:33:47 UTC
Created attachment 354510 [details] [review]
Initial version of a meson based build system

Does not build libsoup-gnome and exclude gssapi and ntlm_auth, and also
misses other features of the autotools build system. But generates a
useable library.
Comment 4 Sebastian Dröge (slomo) 2017-06-26 14:46:20 UTC
Created attachment 354511 [details] [review]
Initial version of a meson based build system

Does not build libsoup-gnome and exclude gssapi and ntlm_auth, and also
misses other features of the autotools build system. But generates a
useable library.
Comment 5 Sebastian Dröge (slomo) 2017-06-26 14:54:42 UTC
Created attachment 354512 [details] [review]
Initial version of a meson based build system

Does not build libsoup-gnome and exclude gssapi and ntlm_auth, and also
misses other features of the autotools build system. But generates a
useable library.
Comment 6 Tomas Popela 2017-10-03 08:53:26 UTC
Created attachment 360818 [details] [review]
Support for GSSAPI and NTLM
Comment 7 Sebastian Dröge (slomo) 2017-10-03 09:02:06 UTC
Thanks! libsoup-gnome and the unit tests at least are also missing, and I think after that we should double-check against the latest configure.ac :)
Comment 8 Sebastian Dröge (slomo) 2017-10-03 09:03:59 UTC
Comment on attachment 360818 [details] [review]
Support for GSSAPI and NTLM

Your patch is for *.orig files instead of the real ones :)

Can you make them a "git format-patch" formatted patch on top of mine? Otherwise the changes look good to me
Comment 9 Tomas Popela 2017-10-03 09:16:26 UTC
(In reply to Sebastian Dröge (slomo) from comment #8)
> Comment on attachment 360818 [details] [review] [review]
> Support for GSSAPI and NTLM
> 
> Your patch is for *.orig files instead of the real ones :)
> 
> Can you make them a "git format-patch" formatted patch on top of mine?
> Otherwise the changes look good to me

Yeah, it was generated with gendiff.. But I think that we should move the stuff to some upstream branch.. hence:

https://git.gnome.org/browse/libsoup/log/?h=wip/meson
Comment 10 Tomas Popela 2017-10-03 09:47:06 UTC
(In reply to Sebastian Dröge (slomo) from comment #7)
> Thanks! libsoup-gnome and the unit tests at least are also missing, and I
> think after that we should double-check against the latest configure.ac :)

We were missing the check for glib-networking - now it's implemented in the wip/meson branch.
Comment 11 Tomas Popela 2017-11-24 08:20:43 UTC
Hi Fan,

would you mind adding support for meson build for win32? I think that you are the most familiar with libsoup and win32. Feel free to commit to wip/meson branch.
Comment 12 Fan, Chun-wei 2017-12-04 09:31:02 UTC
Hello Tomas,

The wip/meson branch should now contain items that are needed to build libsoup using Visual Studio now, including support for MIT Kerberos (GSSAPI).

Just wondering, since soup-gnome essentially requires the GNOME proxy GIO module (which means gsettings-desktop-schemas is required, which is usable on Windows), do you think we want to try to enable that on Windows builds?

I need to look again into the NTLM items in libsoup to see whether it really does not work on Windows (since NTLM itself *is* a Windows item).

With blessings, and cheers!
Comment 13 Tomas Popela 2017-12-04 09:39:51 UTC
(In reply to Fan, Chun-wei from comment #12)
> The wip/meson branch should now contain items that are needed to build
> libsoup using Visual Studio now, including support for MIT Kerberos (GSSAPI).

Thank you Fan! Nice fixes in other parts of the Meson port!
 
> Just wondering, since soup-gnome essentially requires the GNOME proxy GIO
> module (which means gsettings-desktop-schemas is required, which is usable
> on Windows), do you think we want to try to enable that on Windows builds?

I think that Dan should answer this question.
Comment 14 Tomas Popela 2017-12-04 09:41:09 UTC
Is there something that we still miss? Or does it make sense to commit it to the master and have both - Autotools and Meson ports in place and once we are confident we will remove the Autotools port..
Comment 15 Tomas Popela 2018-01-29 17:12:53 UTC
Claudio, Dan what you think about commit this to master? Or should we wait after 3.28 is out?
Comment 16 Ignacio Casal Quinteiro (nacho) 2018-02-14 10:30:03 UTC
Hey, I don't think this patch is correct:
https://git.gnome.org/browse/libsoup/commit/?h=wip/meson&id=0514976cdc8081619f5c503aae7acd06b76c5ec5

For msvc I would not expect those flags. You should probably do something like this: https://git.gnome.org/browse/libgit2-glib/tree/meson.build#n48

Apart from that, I would say to merge this. Just ensure that the new files are part of the of EXTRA_DIST of autotools so we get them in the next release since anyway the autotools stuff will not be removed for now.
Comment 17 Ignacio Casal Quinteiro (nacho) 2018-02-14 10:34:29 UTC
Iñigo can you please give also a round of review to the meson stuff? Thanks
Comment 18 Tomas Popela 2018-02-14 10:52:40 UTC
(In reply to Ignacio Casal Quinteiro (nacho) from comment #16)
> Hey, I don't think this patch is correct:
> https://git.gnome.org/browse/libsoup/commit/?h=wip/
> meson&id=0514976cdc8081619f5c503aae7acd06b76c5ec5

Thanks for noticing! Should be fixed with https://git.gnome.org/browse/libsoup/commit/?h=wip/meson&id=24e4e44bb4eaa7805947bb5395edc997c20dda8e

> Apart from that, I would say to merge this. Just ensure that the new files
> are part of the of EXTRA_DIST of autotools so we get them in the next
> release since anyway the autotools stuff will not be removed for now.

Yes, the meson files are already part of it.
Comment 19 Ignacio Casal Quinteiro (nacho) 2018-02-14 14:47:38 UTC
If you do not get any ack today I would say go ahead and push it. It is not like you are changing the default build system anyway yet and like this the meson one can get some testing.
Comment 20 Piotr Drąg 2018-02-14 15:01:48 UTC
Review of attachment 354512 [details] [review]:

It’s missing i18n support. :(
Comment 21 Michael Catanzaro 2018-02-14 15:04:31 UTC
FWIW I think adding a new build system without removing the old one is a bad idea; it makes it much harder to maintain and you inevitably wind up with broken releases (e.g. meson files missing from EXTRA_DIST). And since removing the existing build system this late in the release cycle would be pretty unfriendly, what I would do if I was the maintainer would be to delete Autotools and add meson at the same time, right after creating the next stable branch.
Comment 22 Michael Catanzaro 2018-02-14 15:05:57 UTC
Review of attachment 354512 [details] [review]:

Also missing i18n support is bad. :P Good catch Piotr!
Comment 23 Piotr Drąg 2018-02-14 15:07:44 UTC
(In reply to Michael Catanzaro from comment #22)
> Review of attachment 354512 [details] [review] [review]:
> 
> Also missing i18n support is bad. :P Good catch Piotr!

It’s been pointed out to me that the attached patch is outdated, and the Meson port in a branch has i18n support.
Comment 24 Tomas Popela 2018-02-14 15:08:35 UTC
(In reply to Michael Catanzaro from comment #21)
> FWIW I think adding a new build system without removing the old one is a bad
> idea; it makes it much harder to maintain and you inevitably wind up with
> broken releases (e.g. meson files missing from EXTRA_DIST).

I tested this and it should be fine - see https://git.gnome.org/browse/libsoup/commit/?h=wip/meson&id=2d32b6f4ba6197ad27e984c5d3a362eac9baaed7

> And since
> removing the existing build system this late in the release cycle would be
> pretty unfriendly, what I would do if I was the maintainer would be to
> delete Autotools and add meson at the same time, right after creating the
> next stable branch.

Could be. But that's something what I would like to hear from others as well. Or at least know Claudio's opinion. I got "ack" from Dan (he's fine if Claduio doesn't object)..
Comment 25 Ignacio Casal Quinteiro (nacho) 2018-02-14 15:12:37 UTC
I would definitely like to see this in as I could start to use it on Windows.
Comment 26 Claudio Saavedra 2018-02-14 15:28:36 UTC
Without having reviewed the patch I want to say that I'm OK with the move; but it will have to wait until we branch. Now it's too late in the cycle to get this in.
Comment 27 Tomas Popela 2018-02-14 15:39:11 UTC
(In reply to Claudio Saavedra from comment #26)
> Without having reviewed the patch I want to say that I'm OK with the move;
> but it will have to wait until we branch. Now it's too late in the cycle to
> get this in.

Sounds good. And do you prefer to remove the autotools support at the same time?
Comment 28 Tomas Popela 2018-02-14 15:40:34 UTC
Comment on attachment 354512 [details] [review]
Initial version of a meson based build system

All the work is in https://git.gnome.org/browse/libsoup/log/?h=wip/meson
Comment 29 Tim-Philipp Müller 2018-02-14 15:54:05 UTC
libsoup isn't really so fast-moving that one really has to keep up with lots of build system churn, is it?

Not sure why there is a need to remove autotools build at the same time.

I'd say just merge the Meson build now, say it's experimental, and then remove the autotools when everyone's happy with the Meson build. The sooner it's merged, the sooner people are likely to give it a spin and fix up any remaining issues.
Comment 30 Ignacio Casal Quinteiro (nacho) 2018-02-14 16:20:42 UTC
FWIW other modules like gdk-pixbuf, glib or glib-openssl have done the same, starting to integrate meson and then the removal can come once you are confident it is ready
Comment 31 Iñigo Martínez 2018-02-15 08:36:36 UTC
I have done a very quick review (I'm a bit busy this week) and I usually make mistakes and I might have overlooked/missed things. These all are suggestions and everything can be discussed.

I hope it helps.

meson.build file

> license : 'LGPL',

I always make mistakes with licenses, but shouldn't this be LGPL2?

> default_options : ['warning_level=1', 'c_std=c89'])

`warning_level` is already `1` by default, so there is no need to add it here.

> foreach arg : [
>   ]
>   if cc.has_argument(arg)

Since meson 0.43 you have the `get_supported_arguments` helper function which is quite useful here, you probably will need an auxiliary variable though.

> glib_dep = [dependency('glib-2.0', version : '>=2.38'),
>             dependency('gobject-2.0', version : '>=2.38'),
>             dependency('gio-2.0', version : '>=2.38')]

This is fine. However, all the version requirements are the same. Why don't move these to a variable that would help the maintainer/developers change the version number easier?

> if not cc.compiles(check_gio_tls_src, name : 'GIO has real TLS support', dependencies : glib_dep)
>  error('libsoup requires glib-networking or glib-openssl for TLS support')
> endif

Although this is fine, you can also use `assert` here (be careful with the length of the lines, though).

assert(cc.compiles(check_gio_tls_src, name : 'GIO has real TLS support', dependencies : glib_dep),
       'libsoup requires glib-networking or glib-openssl for TLS support')

> cdata.set('APACHE_HTTPD', '"@0@"'.format(apache_httpd2.path())

You can add quoted strings to the config data file.

cdata.set_quoted('APACHE_HTTPD', apache_httpd2.path())

> add_project_arguments('-DHAVE_APACHE=1', language : 'c')
> add_project_arguments('-DAPACHE_HTTPD=' + cdata.get('APACHE_HTTPD'), language : 'c')

> add_project_arguments('-DHAVE_PHP_XMLRPC=1', language : 'c')

> add_project_arguments('-DHAVE_CURL=1', language : 'c')

It would be nice to move these to the `config.h` data, which is included by `tests-utils.h`, which is also included by all tests. In the case of `APACHE_HTTPD` you already have added it. It would be nice to unify it.

> gssapi_header = cc.has_header('gssapi/gssapi.h', required: false)
> if gssapi_header
>   gssapi_lib = cc.find_library('gssapi@0@'.format(gssapi_lib_type), required: false)
> endif
> if gssapi_header and gssapi_lib.found()
>   add_project_link_arguments('gssapi@0@.lib'.format(gssapi_lib_type), language : 'c')
> else
>   error('GSSAPI support requested, but the MIT Keberos 5 headers and libraries are not found')
> endif

You can change this code for the following, which is a bit shorter:

if cc.has_header('gssapi/gssapi.h', required: false)
  gssapi_lib = cc.find_library('gssapi' + gssapi_lib_type), required: false)
  assert(gssapi_lib.found(), 'GSSAPI support requested, but the MIT Keberos 5 headers and libraries are not found')
  add_project_link_arguments('gssapi@0@.lib'.format(gssapi_lib_type), language : 'c')
endif

I have also changed the first format to just use concatenation.

> if krb5_config.found()
>   krb5_config_output = run_command (krb5_config, '--libs', 'gssapi')
>   if krb5_config_output.returncode() == 0
>     add_project_link_arguments(krb5_config_output.stdout().split(), language : 'c')
>   else
>     error('Failed to obtain cflags for GSSAPI from krb5-config')
>     enable_gssapi = false
>   endif
>
>   krb5_config_output = run_command (krb5_config, '--cflags', 'gssapi')
>   if krb5_config_output.returncode() == 0
>     add_project_arguments(krb5_config_output.stdout().split(), language : 'c')
>   else
>     error('Failed to obtain cflags for GSSAPI from krb5-config')
>     enable_gssapi = false
>   endif
> else
>   error('GSSAPI support requested, but krb5-config not found. Please specify its path with -Dkrb5-config=PATH')
>   enable_gssapi = false
> endif

You can change this code for the following, which is a bit shorter:

assert(krb5_config.found(), 'GSSAPI support requested, but krb5-config not found. Please specify its path with -Dkrb5-config=PATH')

krb5_config_output = run_command (krb5_config, '--libs', 'gssapi')
assert(krb5_config_output.returncode() == 0, 'Failed to obtain cflags for GSSAPI from krb5-config')
add_project_link_arguments(krb5_config_output.stdout().split(), language : 'c')

krb5_config_output = run_command (krb5_config, '--cflags', 'gssapi')
assert(krb5_config_output.returncode() == 0, 'Failed to obtain cflags for GSSAPI from krb5-config')
add_project_arguments(krb5_config_output.stdout().split(), language : 'c')

> add_project_arguments('-DLIBSOUP_HAVE_GSSAPI=1', language : 'c')

This seems to be used only on libsoup. Why don't you move this to target specific compiler args?

> ntlm_auth_option = get_option('ntlm_auth')
> ntlm_auth_path = ntlm_auth_option != '' ? ntlm_auth_option : 'ntlm_auth'
> ntlm_auth = find_program(ntlm_auth_path, required : false)

If the default value of `ntlm_auth` option is `ntlm_auth` value, change it in the meson_options.txt file and you could replace these lines just for this:

ntlm_auth = find_program(get_option('ntlm_auth'), required: false)

It will then get `ntlm_auth` as default or the user entered value otherwise.

> add_project_arguments('-DUSE_NTLM_AUTH=1', language : 'c')
> add_project_arguments('-DNTLM_AUTH=' + ntlm_auth.path(), language : 'c')

Use on both tests and libsoup directories. Why don't you move this to target specific compiler args?

> cdata.set('BUILD_LIBSOUP_GNOME', enable_gnome)

Is this define actually used?

> cdata.set('PACKAGE_VERSION', '"@0@"'.format(soup_version))
> cdata.set('LOCALEDIR', '"@0@/@1@"'.format(prefix, get_option('localedir')))
> cdata.set('GETTEXT_PACKAGE', '"libsoup-2.4"')

You can also use `set_quoted` here, without any double quotes or string formatting.

In the case of locale directory, why don't you use `join_paths` without any formatting?

In the case of the gettext domain, why not format a string with the project name and the api version name, so it's easier to change it?

> pkgconf = configuration_data()
>
> pkgconf.set('prefix', get_option('prefix'))
> pkgconf.set('exec_prefix', '${prefix}')
> pkgconf.set('libdir', '${prefix}/@0@'.format(get_option('libdir')))
> pkgconf.set('includedir', '${prefix}/@0@'.format(get_option('includedir')))
> pkgconf.set('VERSION', soup_version)
>
> pkg_install_dir = '@0@/pkgconfig'.format(get_option('libdir'))
>
> configure_file(output : 'libsoup-2.4.pc',
>  input : 'libsoup-2.4.pc.in',
>  configuration : pkgconf,
>  install_dir : pkg_install_dir)
>
> if enable_gnome
>   configure_file(output : 'libsoup-gnome-2.4.pc',
>     input : 'libsoup-gnome-2.4.pc.in',
>     configuration : pkgconf,
>     install_dir : pkg_install_dir)
> endif

Although it's not always applicable, it seems to be in this case. Why don't you use meson's `pkg` module here?

libsoup/meson.build file

> includedir = 'libsoup-@0@/libsoup'.format(apiversion)

Why not use something like:

includedir = join_paths('@0@-@1@'.format(meson.project_name(), apiversion), meson.project_name())

Anyway I usually create a variable called `api_name` which is a combination of the project name and the api version. Something like:

api_name = '@0@-@1@'.format(meson.project_name(), apiversion)

It's usually highly useful.

> soup_enums = gnome.mkenums('soup-enum-types',
>   sources : soup_headers,
>   h_template : 'soup-enum-types.h.template',
>   c_template : 'soup-enum-types.c.template',
>   install_header : true,
>   install_dir : join_paths(get_option('includedir'), includedir))

Why not use a variable for the strings? It would help on modifications. Something like this:

enum_types = 'soup-enum-types'

soup_enums = gnome.mkenums(enum_types,
   sources : soup_headers,
   h_template : enum_types + '.h.template',
   c_template : 'enum_types + '.c.template',
   install_header : true,
   install_dir : join_paths(get_option('includedir'), includedir))

This would help meson detect any mispelled variable, but won't if they are strings.

> libsoup_c_args  = ['-DHAVE_CONFIG_H']
> libsoup_c_args += ['-DG_LOG_DOMAIN="libsoup"']
> libsoup_c_args += ['-DLIBSOUP_COMPILATION']

Why don't create a full array once instead of applying concatenations?. Something like:

libsoup_c_args = [
  '-DHAVE_CONFIG_H',
  '-DG_LOG_DOMAIN="@0@"'.format(meson.project_name()),
  '-DLIBSOUP_COMPILATION'
]

> libsoup = library('soup-@0@'.format(apiversion), soup_sources,
> libsoup_gnome = library('soup-gnome-@0@'.format(apiversion), soup_gnome_sources,

Be careful about `library` because it can be set as static.

> include_directories : [configinc],

It's not necessary to include square brackets if there is only one value.

> libsoup_dep = declare_dependency(link_with : libsoup,
>   include_directories : configinc,
>   sources : soup_enums)

You should only include the enums header: sources: soupenum_h.

> includedir = 'libsoup-gnome-@0@/libsoup'.format(apiversion)

What about this?

includedir = '@0@-gnome-@1@/@0@'.format(meson.project_name(), apiversion)

> include_directories : [configinc],

Square brackets are not necessary.

> dependencies : [deps, libsoup_dep])

Although this is fine, it's a bit odd. You are creating a new array, with a previous array (`deps` is already an array).

> soup_introspection_sources = [

Why don't you reuse `soup_sources` and `soup_headers` here somehow (they might need some tweaking), so the files are not listed twice?

> '--c-include=libsoup/soup.h'

Since meson 0.43 you already have the `header` parameter in `generate_gir`.

> soup_gir_gen_sources = [gnome.generate_gir(libsoup,

There is no need to create an array from `generate_gir's` return, it's already an array (formed by two elements [gir_target, typelib_target]).

Be also careful with `libsoup`, because if it's a static library, this won't work.

> nsversion : '2.4',

`nsversion's` value can be replace by `apiversion`.

> export_packages : 'libsoup-2.4',

Why don't use `apiname` for `export_packages's` value? ;)

> extra_args : gir_args,
> dependencies : [deps, libsoup],

I haven't figured out how `dependencies` works here, but I don't think `deps` should be included here. `libsoup` is already the first parameter, so there is no need to duplicate it.

> includes : ['Gio-2.0'],

Square brackets are not necessary.

> namespace : 'Soup',
> symbol_prefix : 'soup_',
> identifier_prefix : 'Soup',

Why don't you create a variable for `Soup` and use it here?

soup_ns = 'Soup'
 
namespace: soup_ns,
symbol_prefix: soup_ns.to_lower + '_',
identifier_prefix: soup_ns,
  
What do you think?

> include_directories : [configinc],

Square brackets are not necessary here.

> gnome.generate_vapi('libsoup-2.4',

Use apiname for `libsoup-2.4`?

> sources : [soup_gir_gen_sources[0][0]],

This should be:

sources : soup_gir_gen_sources[0]

> packages : ['gio-2.0'],

Square brackets are not necessary.

> '--identifier-prefix=Soup',

You are already using `identifier_prefix` parameter so it's not necessary.

> '--symbol-prefix=soup',

You are already ysing the `symbol_prefix` parameter so it's not necessary.

> '--c-include=libsoup/soup-gnome.h',

You can use the `header` parameter from meson 0.43.

> '--include-uninstalled=@0@/Soup-2.4.gir'.format(meson.current_build_dir())

You can replace this for the following:

'--include-uninstalled=' + soup_gir_gen_sources[0].full_path()

> gnome.generate_gir(libsoup_gnome,

Be careful because `libsoup_gnome` can be a static library.

> nsversion : '2.4'

Why don't you replace this for `apiversion`?

> dependencies : [deps, libsoup, libsoup_gnome, soup_gir_gen_dep],

Look above.

> includes : ['Gio-2.0'],

Square brackets are not necessary.

In this case you can also use something like a variable `soup_ns` like above and replace it here.

tests/meson.build

> test_utils = static_library('test-utils', 'test-utils.c', 'test-utils.h',

`test-utils.h` is not necessary here`. You can also use a variable to avoid `test-utils` string duplication as you have done below on the `foreach` loop.

> install : false,

It's already false by default so it's not necessary.

> source_dir : '.')

This is not necessary.

example/meson.build

> get_example = executable('get', 'get.c',
>     dependencies : [glib_dep, libsoup_dep])
> 
> simple_httpd_example = executable('simple_httpd', 'simple-httpd.c',
>     dependencies : [glib_dep, libsoup_dep])
> 
> simple_proxy_example = executable('simple_proxy', 'simple-proxy.c',
>     dependencies : [glib_dep, libsoup_dep])

What about something like this?

deps = [
  glib_dep,
  libsoup_dep
]

examples = [
  'get',
  'simple-httpd',
  'simple-proxy'
]

foreach example: examples
  executable(
    example
    example + '.c',
    dependencies: deps
  )
endforeach

If the probram must use an underscore, you can also use `underscorify`.
Comment 32 Tim-Philipp Müller 2018-02-15 10:08:13 UTC
Minor thing, but I think assert() should only be used in unit tests.

error() is the right thing to use in normal build scripts. It's makes the meson.build more readable and the user-facing error message will also be less confusing.
Comment 33 Iñigo Martínez 2018-02-15 10:57:34 UTC
(In reply to Tim-Philipp Müller from comment #32)
> Minor thing, but I think assert() should only be used in unit tests.
>
> error() is the right thing to use in normal build scripts. It's makes the
> meson.build more readable and the user-facing error message will also be
> less confusing.

Actually, the major issue with `assert` is that it's not documented.

I believe that is easier to read because it's shorter and it goes straight to the point, so I make heavy use of it. meson has also moved from `if-error-endif` to assert[0][1], though it's true that this only happens in its unit tests.

However, this is totally subjective and personal, so I can go back to `if-error-endif` if that is the right thing.

[0] https://github.com/mesonbuild/meson/commit/f3e20b2570a1779028f22772ebe295c2c21c3a94
[1] https://github.com/mesonbuild/meson/commit/b337145ec6b83aa763a4d21eb0e4b2bf35ec880a
Comment 34 Tomas Popela 2018-02-15 13:57:27 UTC
First of all thank you Iñigo for the review! It's nice to see some feedback from someone who's experienced with Meson! I didn't about set_quoted() or join_paths() before..

(In reply to Iñigo Martínez from comment #31)
> gssapi_lib = cc.find_library('gssapi' + gssapi_lib_type), required: false)

there was an extra closing bracket..

> > add_project_arguments('-DLIBSOUP_HAVE_GSSAPI=1', language : 'c')
> 
> This seems to be used only on libsoup. Why don't you move this to target
> specific compiler args?

It's also used in examples, so it should stay as a project argument.

> 
> > add_project_arguments('-DUSE_NTLM_AUTH=1', language : 'c')
> > add_project_arguments('-DNTLM_AUTH=' + ntlm_auth.path(), language : 'c')
> 
> Use on both tests and libsoup directories. Why don't you move this to target
> specific compiler args?

As above.. I would avoid duplicating the lines per targets..

> 
> > cdata.set('BUILD_LIBSOUP_GNOME', enable_gnome)
> 
> Is this define actually used?

Probably a leftover - now enable_gnome should be used everywhere..

> > libsoup = library('soup-@0@'.format(apiversion), soup_sources,
> > libsoup_gnome = library('soup-gnome-@0@'.format(apiversion), soup_gnome_sources,
> 
> Be careful about `library` because it can be set as static.

I thought that I will change it to shared_library(), but I'm not sure, if it's the right thing. Or we could skip introspection and vapi if it libsoup is going to be compiled as static. I probably need a help here to decide it..

> 
> > soup_introspection_sources = [
> 
> Why don't you reuse `soup_sources` and `soup_headers` here somehow (they
> might need some tweaking), so the files are not listed twice?

How obvious :) - actually the only difference actually is missing soup-proxy-resolver.h and soup.h for introspection.

> > namespace : 'Soup',
> > symbol_prefix : 'soup_',
> > identifier_prefix : 'Soup',
> 
> Why don't you create a variable for `Soup` and use it here?
> 
> soup_ns = 'Soup'
>  
> namespace: soup_ns,
> symbol_prefix: soup_ns.to_lower + '_',
> identifier_prefix: soup_ns,
>   
> What do you think?

Nice one! :)
Comment 35 Iñigo Martínez 2018-02-15 14:45:45 UTC
(In reply to Tomas Popela from comment #34)
> First of all thank you Iñigo for the review! It's nice to see some feedback
> from someone who's experienced with Meson! I didn't about set_quoted() or
> join_paths() before..

You are welcome. Don't worry about those, I'm still discovering new things and meson is also changing (and getting better) with new versions :)

> (In reply to Iñigo Martínez from comment #31)
> > gssapi_lib = cc.find_library('gssapi' + gssapi_lib_type), required: false)
> 
> there was an extra closing bracket..

Sorry for the mistake. Probably I should have tell you my policy when concatenating strings. If there is only one concatenation and the string is going to be added at the beginning or at the end, I just use the `+` operator, otherwise to avoid surrounding a variable with `+` operators something just in the middle I use format. For example:

x = str_var + 'foobar'
x = 'foobar' + str_var
x = 'foo@0@bar'.format(str_var)
x = '@0@foobar@1@'.format(str_var1, str_var2)

Just in case it helps...

> I thought that I will change it to shared_library(), but I'm not sure, if
> it's the right thing. Or we could skip introspection and vapi if it libsoup
> is going to be compiled as static. I probably need a help here to decide it..

I was in doubt at the time (iirc when porting totem). In this case, `shared_library` seems the best option here.

In the future it might be possible to create both types of libraries at the same time: https://github.com/mesonbuild/meson/pull/2711
Comment 36 Tomas Popela 2018-02-15 15:09:10 UTC
(In reply to Iñigo Martínez from comment #35)
> I was in doubt at the time (iirc when porting totem). In this case,
> `shared_library` seems the best option here.

Ok, I will change it to shared_library(). It can be changed later of course :)..
 
> In the future it might be possible to create both types of libraries at the
> same time: https://github.com/mesonbuild/meson/pull/2711

Saw this one as well, while googling for how identify the static build (the default_library option).
Comment 37 Iñigo Martínez 2018-02-16 11:58:25 UTC
Tim-Philipp Müller: I've created a new issue[0] in meson in order to clarify if the `assert` function should be used or not. An official statement would be nice.

I hope it helps :)

[0] https://github.com/mesonbuild/meson/issues/3076
Comment 38 Ignacio Casal Quinteiro (nacho) 2018-03-16 11:43:22 UTC
now that the release is out, can we get this sorted out?
Comment 39 Tomas Popela 2018-03-22 09:40:26 UTC
Claudio are you ok with merging the wip/meson to master?
Comment 40 Claudio Saavedra 2018-03-23 13:50:52 UTC
After we branch, you can squeeze it and merge it. Please expect to maintain this for a while if there are any issues. I have no intention to start fixing meson related issues for a while :)
Comment 41 Tomas Popela 2018-03-23 13:53:15 UTC
(In reply to Claudio Saavedra from comment #40)
> After we branch, you can squeeze it and merge it.

Thanks!

> Please expect to maintain
> this for a while if there are any issues. I have no intention to start
> fixing meson related issues for a while :)

I expect that ;)
Comment 42 Claudio Saavedra 2018-04-06 13:21:05 UTC
We have branched. Please notice that the libpsl patch from bug #769650 was merged already, so please make sure that works too.
Comment 43 Ignacio Casal Quinteiro (nacho) 2018-04-06 13:24:54 UTC
This kind of means we will have to port libpsl to meson... since it does not have a build system for msvc...
Comment 44 Ignacio Casal Quinteiro (nacho) 2018-04-06 13:31:58 UTC
Issue upstream: https://github.com/rockdaboot/libpsl/issues/95
Comment 45 Fan, Chun-wei 2018-04-07 09:24:01 UTC
Hi,

I took a quick look at it last night, it needs a little bit of header porting for it to build on Windows without Cygwin, but it seems to work alright.  I think for Meson it would be more extensive work involved.

The easiest way to get IDN support for MSVC is to use ICU.

My take at this.

With blessings and cheers!
Comment 46 Tomas Popela 2018-04-13 06:56:01 UTC
So I decided that I will merge the wip/meson to master. At this point we will have two build systems - Meson and Autotools. Once the MSVC situation will be resolved, then we can remove the Autotools support and only leave the Meson one.
Comment 47 Nirbheek Chauhan 2018-04-13 07:12:44 UTC
(In reply to Tomas Popela from comment #46)
> So I decided that I will merge the wip/meson to master. At this point we
> will have two build systems - Meson and Autotools. Once the MSVC situation
> will be resolved, then we can remove the Autotools support and only leave
> the Meson one.

I am not sure I understand how the two are related. You don't *need* to port libpsl to Meson, it's just a nice-to-have.

You can build libpsl with Autotools+MinGW and link to it with libsoup while building with Meson+MSVC. The workflow would be identical to how libsoup is built right now with Autotools+MinGW on Windows.

As an aside, you can also build libsoup with Meson+MinGW on Windows (or cross-compile it). Porting to Meson has the benefit of being able to build with MSVC and no other drawbacks.
Comment 48 Ignacio Casal Quinteiro (nacho) 2018-04-13 13:27:59 UTC
I do not agree with that mixing runtimes is always leading to weird crashes. Anyway I guess at some point I'll have to Port that lib to meson otherwise I will end up with the current version of libsoup forever. Just sad though that having everything ready for the current release this did not get in having the fact that both build systems could be there without issues.
Comment 49 Nirbheek Chauhan 2018-04-13 14:03:42 UTC
(In reply to Ignacio Casal Quinteiro (nacho) from comment #48)
> I do not agree with that mixing runtimes is always leading to weird crashes.

To add to this, there is no mixing of runtimes when you use MSVC to link to a library that was built with MinGW. MinGW literally links to msvcrt.dll. It's the same (release) runtime, hence there's no crashes as long as you stay within that.

You do have issues when you try to mix C++ code from MSVC and GCC, because of various reasons and none of them apply here.

In any case, removing Autotools does not result in any toolchain regressions since you can always build with MinGW on Windows using Meson.
Comment 50 Fan, Chun-wei 2018-04-14 02:46:20 UTC
Hi,

I think for libps1 www still need to do the aforementioned header porting (change of the included system headers) for it to compile on MinGW, as there are *NIX networking headers that are included, which can be replaced with the winsock2 headers, for example.

Sorry, as I am working with other projects, I didn't have the opportunity to open a PR there for this, but I hope to do so asap, with a set of NMake Makefiles at least.

Work blessings, and cheers!
Comment 51 Fan, Chun-wei 2018-04-14 02:47:25 UTC
(sorry for the noise, www should read we)
Comment 52 Claudio Saavedra 2018-04-16 12:51:03 UTC
Just for the sake of testing this, I tried to build/make a tarball with meson. I got a failure as soon as I started, not sure if I'm doing something wrong or if there are things that need fixing. In any case, I'll keep using autotools for tarballs for the time being. Here's the error:

claudio@patanjali:~/git/gnome/libsoup$ meson --prefix=/home/claudio/gnome3  build 
The Meson build system
Version: 0.45.1
Source dir: /home/claudio/git/gnome/libsoup
Build dir: /home/claudio/git/gnome/libsoup/build
Build type: native build
Project name: libsoup
Native C compiler: ccache cc (gcc 6.4.0 "cc (Debian 6.4.0-1) 6.4.0 20170704")
Appending CFLAGS from environment: '-Wall -g -O0'
Appending LDFLAGS from environment: '-L/home/claudio/gnome3/lib '
Build machine cpu family: x86_64
Build machine cpu: x86_64
Compiler for C supports arguments -Wall -Wmissing-include-dirs -Wpointer-arith -Winit-self -Wdeclaration-after-statement -Werror=missing-prototypes -Werror=implicit-function-declaration -Werror=aggregate-return -Werror=format=2 -Wstrict-prototypes -Wno-format-zero-length: YES
Found pkg-config: /usr/bin/pkg-config (0.29)
Native dependency glib-2.0 found: YES 2.57.0
Native dependency gobject-2.0 found: YES 2.57.0
Native dependency gio-2.0 found: YES 2.57.0
Native dependency sqlite3 found: YES 3.23.1
Native dependency libxml-2.0 found: YES 2.9.4
Native dependency libpsl found: YES 0.20.0
Checking if "GIO has real TLS support" compiles: YES
Program httpd2 found: NO
Program httpd found: NO
Program apache2 found: NO
Program apache found: NO
Program curl found: YES (/usr/bin/curl)
Program krb5-config found: NO

meson.build:176:4: ERROR: Assert failed: GSSAPI support requested, but krb5-config not found. Please specify its path with -Dkrb5-config=PATH
Comment 53 Tomas Popela 2018-04-16 13:16:50 UTC
GSSAPI is enabled by default so you need a krb5 package that provide /usr/bin/krb5-config. Or we should turn that option off by default.
Comment 54 Claudio Saavedra 2018-04-16 14:53:37 UTC
I think we need to preserve the defaults from autotools. GSSAPI's default value in configure.ac is "auto".
Comment 55 Tomas Popela 2018-04-16 14:58:27 UTC
(In reply to Claudio Saavedra from comment #54)
> I think we need to preserve the defaults from autotools. GSSAPI's default
> value in configure.ac is "auto".

The thing is that I really hate auto values. I was bitten by this not so long ago in Fedora. One package got removed from the build root and gstreamer was missing some functionality. But in this case I will fix it so we are consistent..
Comment 56 Ignacio Casal Quinteiro (nacho) 2018-04-16 15:02:49 UTC
With meson the auto value are discouraged
Comment 57 Nirbheek Chauhan 2018-04-16 15:34:07 UTC
(In reply to Ignacio Casal Quinteiro (nacho) from comment #56)
> With meson the auto value are discouraged

This is not true. "auto" values for options are an anti-pattern only if done incorrectly. Automatic detection and skipping of non-essential dependencies is important for users to be able to build software without going through dependency hell. 

However, "auto" options are terrible if there's no corresponding mechanism for forcing it to be on (failing the build if the dependency is not found) or off (not search for it at all).

In fact, we've been working on infrastructure that nudges people towards such a mechanism: all options would have three values: "auto", "on", or "off":

https://github.com/mesonbuild/meson/pull/3376
Comment 58 Claudio Saavedra 2018-04-16 15:35:41 UTC
You have a point. But if we're going to change default dependency needs then we should document it properly and not just make it a side-effect of changing the build system. Are there other dependencies whose default values have been changed similarly?
Comment 59 Ignacio Casal Quinteiro (nacho) 2018-04-16 15:39:34 UTC
See the gnome port page:

https://wiki.gnome.org/Initiatives/GnomeGoals/MesonPorting

Try to avoid automatic feature detection; this makes it harder for distributors and continuous integration systems to identify the dependencies needed to build your project
Comment 60 Nirbheek Chauhan 2018-04-16 15:43:50 UTC
(In reply to Ignacio Casal Quinteiro (nacho) from comment #59)
> See the gnome port page:

I would read that as "don't do automatic feature detection as the only way to detect features". Otherwise, it's quite user unfriendly, as we just saw.

... we should also not use assert() to signal errors, please. That's for internal use (in tests) and gives a particularly unfriendly error message.
Comment 61 Ignacio Casal Quinteiro (nacho) 2018-04-16 15:46:26 UTC
(In reply to Nirbheek Chauhan from comment #60)
> (In reply to Ignacio Casal Quinteiro (nacho) from comment #59)
> > See the gnome port page:
> 
> I would read that as "don't do automatic feature detection as the only way
> to detect features". Otherwise, it's quite user unfriendly, as we just saw.
> 
> ... we should also not use assert() to signal errors, please. That's for
> internal use (in tests) and gives a particularly unfriendly error message.

Since you are probably the best person to give suggestions on meson, can you please modify that wiki page so it does not happen to other projects as well?
Comment 62 Nirbheek Chauhan 2018-04-16 16:05:31 UTC
(In reply to Ignacio Casal Quinteiro (nacho) from comment #61)
> Since you are probably the best person to give suggestions on meson, can you
> please modify that wiki page so it does not happen to other projects as well?

After discussion with ebassi and mcatanzaro:

<mcatanzaro> We're just using the port to meson as a convenient time to drop auto dependencies....

So it's a GNOME Goal to drop automatic detection of dependencies. Nothing to do with Meson.
Comment 63 Iñigo Martínez 2018-04-16 16:12:30 UTC
(In reply to Nirbheek Chauhan from comment #60)
> ... we should also not use assert() to signal errors, please. That's for
> internal use (in tests) and gives a particularly unfriendly error message.

I agree with you, and something like `error_if` sounds also better for me[0], but there is still no such feature in meson and meanwhile, assert does the job[1].

It is something that should be easily replaceable in the future.

[0] https://github.com/mesonbuild/meson/issues/3076#issuecomment-366652495
[1] https://github.com/jpakkane/mesonarduino/blob/master/meson.build#L7
Comment 64 Tomas Popela 2018-04-17 05:03:27 UTC
(In reply to Claudio Saavedra from comment #58)
> You have a point. But if we're going to change default dependency needs then
> we should document it properly

What we could do is that once we will do the Meson only release, then we will make a note about the changed default in NEWS. I really don't see any other option than this..

> Are there other dependencies whose default values
> have been changed similarly?

Vala support in Autotools is set on auto, in Meson it's disabled
GSSAPI support in Autotools is set on auto, in Meson it's enabled
NTLM support in Autotools is basically set on auto, in Meson it's disabled
  -  Kerberos developers prefer to disable this by default
Comment 65 Claudio Saavedra 2018-04-17 07:33:35 UTC
(In reply to Tomas Popela from comment #64)
> (In reply to Claudio Saavedra from comment #58)
> > You have a point. But if we're going to change default dependency needs then
> > we should document it properly
> 
> What we could do is that once we will do the Meson only release, then we
> will make a note about the changed default in NEWS. I really don't see any
> other option than this..

Sounds good to me.
 
> > Are there other dependencies whose default values
> > have been changed similarly?
> 
> Vala support in Autotools is set on auto, in Meson it's disabled
> GSSAPI support in Autotools is set on auto, in Meson it's enabled
> NTLM support in Autotools is basically set on auto, in Meson it's disabled
>   -  Kerberos developers prefer to disable this by default

No objections on my side about those values.