Created attachment 338635 [details] [review]
Stop providing our own marshaller and use the generic one

Created attachment 338636 [details] [review]
meson: Add support for building with the meson build system

Created attachment 338642 [details] [review]
meson: Add support for building with the meson build system

(In reply to Thibault Saunier from comment #3)
> Created attachment 338642 [details] [review] [review]
> meson: Add support for building with the meson build system

Just added support for building as a subproject.

Generating GIR is entirely possible and trivial to add.

The documentation part is, on the other hand, currently broken with Meson because of this issue: https://github.com/mesonbuild/meson/issues/550

Created attachment 338680 [details] [review]
meson: Add support for building with the meson build system

(In reply to Emmanuele Bassi (:ebassi) from comment #5)
> Generating GIR is entirely possible and trivial to add.

Added support in that last patch version.

Review of attachment 338680 [details] [review]:

Thanks; it's improved, but still needs some work.

::: config.h.meson
@@ +23,3 @@
+#mesondefine PACKAGE_TARNAME
+#mesondefine PACKAGE_URL
+#mesondefine PACKAGE_VERSION

All the `PACKAGE` defines are unused, as far as I know, and are there only because autoconf puts them there.

::: json-glib/meson.build
@@ +1,3 @@
+install_header_dir ='json-glib-1.0/json-glib'
+
+source_h = ['json-builder.h', 'json-generator.h',

Please, one line per header.

@@ +12,3 @@
+
+json_glib_enums = gnome.mkenums('json-enum-types',
+  sources : headers_paths,

This seems like a bug in gnome.mkenums(); I'd expect all sources to come from current_source_dir().

@@ +13,3 @@
+json_glib_enums = gnome.mkenums('json-enum-types',
+  sources : headers_paths,
+  h_template : 'json-enum-types.h.in',

I prefer the colon to be flush with the positional argument, e.g.: `h_template: foo`.

@@ +16,3 @@
+  c_template : 'json-enum-types.c.in',
+  install_header : true,
+  install_dir : get_option('includedir') + '/' + install_header_dir)

This sounds like something that should have its own variable.

@@ +20,3 @@
+
+source_c = [
+  'json-array.c', 'json-builder.c', 'json-debug.c',

One file per line.

::: json-glib/tests/meson.build
@@ +18,3 @@
+python3 = find_program('python3')
+
+cp = run_command(python3, '-c',

Ugh, this is pretty terrible.

::: meson.build
@@ +15,3 @@
+  json_version_nano = version_arr[3]
+else
+  json_version_nano = 0

We never use a nano version.

@@ +23,3 @@
+# maintaining compatibility with the previous libtool versioning
+# current = minor * 100 + micro
+libversion = '@0@.@1@.0'.format(soversion, json_version_minor.to_int() * 100 + json_version_micro.to_int())

This is not how we do libtool versioning.

We use the same scheme as GTK+:

```
version_arr = meson.project_version().split('.')
json_version_major = version_arr[0].to_int()
json_version_minor = version_arr[1].to_int()
json_version_micro = version_arr[2].to_int()

if json_version_micro.is_odd()
  json_interface_age = 0
else
  json_interface_age = json_version_micro
endif

# maintaining compatibility with the previous libtool versioning
# current = minor * 100 + micro - interface
# revision = interface
soversion = 0
current = json_version_minor * 100 + json_version_micro - json_interface_age
revision = json_interface_age
libversion = '@0@.@1@.@2@'.format(soversion, current, revision)
```

@@ +35,3 @@
+
+check_headers = [
+  ['dlfcn.h','HAVE_DLFCN_H'],

Unused.

@@ +37,3 @@
+  ['dlfcn.h','HAVE_DLFCN_H'],
+  ['unistd.h', 'HAVE_UNISTD_H'],
+  ['sys/types.h', 'HAVE_SYS_TYPES_H'],

Unused.

@@ +38,3 @@
+  ['unistd.h', 'HAVE_UNISTD_H'],
+  ['sys/types.h', 'HAVE_SYS_TYPES_H'],
+  ['sys/stat.h', 'HAVE_SYS_STAT_H'],

Unused.

@@ +40,3 @@
+  ['sys/stat.h', 'HAVE_SYS_STAT_H'],
+  ['string.h', 'HAVE_STRING_H'],
+  ['strings.h', 'HAVE_STRINGS_H'],

Unused.

@@ +41,3 @@
+  ['string.h', 'HAVE_STRING_H'],
+  ['strings.h', 'HAVE_STRINGS_H'],
+  ['stdint.h', 'HAVE_STDINT_H'],

Unused.

@@ +42,3 @@
+  ['strings.h', 'HAVE_STRINGS_H'],
+  ['stdint.h', 'HAVE_STDINT_H'],
+  ['memory.h', 'HAVE_MEMORY_H'],

Unused.

@@ +43,3 @@
+  ['stdint.h', 'HAVE_STDINT_H'],
+  ['memory.h', 'HAVE_MEMORY_H'],
+  ['inttypes.h', 'HAVE_INTTYPES_H'],

Unused.

@@ +44,3 @@
+  ['memory.h', 'HAVE_MEMORY_H'],
+  ['inttypes.h', 'HAVE_INTTYPES_H'],
+  ['dlfcn.h','HAVE_DLFCN_H'],

Duplicate, and unused.

@@ +54,3 @@
+
+check_functions = [
+  ['HAVE_DCGETTEXT', 'dcgettext', '#include<libintl.h>'],

Unused.

@@ +67,3 @@
+cdata.set_quoted('PACKAGE_STRING', 'json-glib ' + json_version)
+cdata.set_quoted('PACKAGE_URL', 'http://live.gnome.org/JsonGlib')
+cdata.set_quoted('PACKAGE_VERSION', json_version)

All the `PACKAGE_*` defines can go away. We're only interested in `GETTEXT_PACKAGE`.

@@ +69,3 @@
+cdata.set_quoted('PACKAGE_VERSION', json_version)
+# FIXME Check how to do it with mingw
+cdata.set('_JSON_EXTERN', '__attribute__((visibility("default"))) extern')

The FIXME can be fixed with:

if get_option('default_library') != 'static'
  if host_system == 'windows'
    conf.set('DLL_EXPORT', true)
    if cc.get_id() == 'msvc'
      conf.set('_JSON_EXTERN', '__declspec(dllexport) extern')
    else
      conf.set('_JSON_EXTERN', '__attribute__((visibility("default"))) __declspec(dllexport) extern')
      extra_args += ['-fvisibility=hidden']
    endif
  else
    conf.set('_JSON_EXTERN', '__attribute__((visibility("default")))')
    extra_args += ['-fvisibility=hidden']
  endif
endif

> ::: config.h.meson
> @@ +23,3 @@
> +#mesondefine PACKAGE_TARNAME
> +#mesondefine PACKAGE_URL
> +#mesondefine PACKAGE_VERSION
> 
> All the `PACKAGE` defines are unused, as far as I know, and are there only
> because autoconf puts them there.

For what it's worth the config.h.meson template is no longer needed, recent versions just do the right thing if you leave out the input file (and git master has support for a description: kwarg for cdata.set() etc. if you want descriptions in the config.h).

Created attachment 338827 [details] [review]
Update patch

Thanks for the review!

(In reply to Emmanuele Bassi (:ebassi) from comment #8)
> Review of attachment 338680 [details] [review] [review]:
> 
> Thanks; it's improved, but still needs some work.
> 
> ::: config.h.meson
> @@ +23,3 @@
> +#mesondefine PACKAGE_TARNAME
> +#mesondefine PACKAGE_URL
> +#mesondefine PACKAGE_VERSION
> 
> All the `PACKAGE` defines are unused, as far as I know, and are there only
> because autoconf puts them there.

Removed.

> ::: json-glib/meson.build
> @@ +1,3 @@
> +install_header_dir ='json-glib-1.0/json-glib'
> +
> +source_h = ['json-builder.h', 'json-generator.h',
> 
> Please, one line per header.

DONE

> @@ +12,3 @@
> +
> +json_glib_enums = gnome.mkenums('json-enum-types',
> +  sources : headers_paths,
> 
> This seems like a bug in gnome.mkenums(); I'd expect all sources to come
> from current_source_dir().

Yes, this is a bug when using subprojects, I will fix it in meson
(https://github.com/mesonbuild/meson/issues/973)
but meanwhile I would prefer to have this workaround.

Added well due FIXME.

> 
> @@ +13,3 @@
> +json_glib_enums = gnome.mkenums('json-enum-types',
> +  sources : headers_paths,
> +  h_template : 'json-enum-types.h.in',
> 
> I prefer the colon to be flush with the positional argument, e.g.:
> `h_template: foo`.

OK, fixed everywhere.

> @@ +16,3 @@
> +  c_template : 'json-enum-types.c.in',
> +  install_header : true,
> +  install_dir : get_option('includedir') + '/' + install_header_dir)
> 
> This sounds like something that should have its own variable.
DONE

> @@ +20,3 @@
> +
> +source_c = [
> +  'json-array.c', 'json-builder.c', 'json-debug.c',
> 
> One file per line.
DONE

> ::: json-glib/tests/meson.build
> @@ +18,3 @@
> +python3 = find_program('python3')
> +
> +cp = run_command(python3, '-c',
> 
> Ugh, this is pretty terrible.

Well, it is the simple solution to get the tests working without
modifying the current way it works.

> ::: meson.build
> @@ +15,3 @@
> +  json_version_nano = version_arr[3]
> +else
> +  json_version_nano = 0
> 
> We never use a nano version.
REMOVED

> @@ +23,3 @@
> +# maintaining compatibility with the previous libtool versioning
> +# current = minor * 100 + micro
> +libversion = '@0@.@1@.0'.format(soversion, json_version_minor.to_int() *
> 100 + json_version_micro.to_int())
> 
> This is not how we do libtool versioning.
> 
> We use the same scheme as GTK+:
> 
> ```
> version_arr = meson.project_version().split('.')
> json_version_major = version_arr[0].to_int()
> json_version_minor = version_arr[1].to_int()
> json_version_micro = version_arr[2].to_int()
> 
> if json_version_micro.is_odd()
>   json_interface_age = 0
> else
>   json_interface_age = json_version_micro
> endif
> 
> # maintaining compatibility with the previous libtool versioning
> # current = minor * 100 + micro - interface
> # revision = interface
> soversion = 0
> current = json_version_minor * 100 + json_version_micro - json_interface_age
> revision = json_interface_age
> libversion = '@0@.@1@.@2@'.format(soversion, current, revision)
> ```

DONE, almost reused your snipnet as is.

> @@ +35,3 @@
> +
> +check_headers = [
> +  ['dlfcn.h','HAVE_DLFCN_H'],
> 
> Unused.
DONE

> 
> @@ +37,3 @@
> +  ['dlfcn.h','HAVE_DLFCN_H'],
> +  ['unistd.h', 'HAVE_UNISTD_H'],
> +  ['sys/types.h', 'HAVE_SYS_TYPES_H'],
> 
> Unused.

+  ['unistd.h', 'HAVE_UNISTD_H'],

This one is used, rest is DONE.

> 
> @@ +38,3 @@
> +  ['unistd.h', 'HAVE_UNISTD_H'],
> +  ['sys/types.h', 'HAVE_SYS_TYPES_H'],
> +  ['sys/stat.h', 'HAVE_SYS_STAT_H'],
> 
> Unused.
DONE

> 
> @@ +40,3 @@
> +  ['sys/stat.h', 'HAVE_SYS_STAT_H'],
> +  ['string.h', 'HAVE_STRING_H'],
> +  ['strings.h', 'HAVE_STRINGS_H'],
> 
> Unused.
DONE

> 
> @@ +41,3 @@
> +  ['string.h', 'HAVE_STRING_H'],
> +  ['strings.h', 'HAVE_STRINGS_H'],
> +  ['stdint.h', 'HAVE_STDINT_H'],
> 
> Unused.
DONE

> 
> @@ +42,3 @@
> +  ['strings.h', 'HAVE_STRINGS_H'],
> +  ['stdint.h', 'HAVE_STDINT_H'],
> +  ['memory.h', 'HAVE_MEMORY_H'],
> 
> Unused.
DONE

> 
> @@ +43,3 @@
> +  ['stdint.h', 'HAVE_STDINT_H'],
> +  ['memory.h', 'HAVE_MEMORY_H'],
> +  ['inttypes.h', 'HAVE_INTTYPES_H'],
> 
> Unused.
DONE

> 
> @@ +44,3 @@
> +  ['memory.h', 'HAVE_MEMORY_H'],
> +  ['inttypes.h', 'HAVE_INTTYPES_H'],
> +  ['dlfcn.h','HAVE_DLFCN_H'],
> 
> Duplicate, and unused.
DONE

> 
> @@ +54,3 @@
> +
> +check_functions = [
> +  ['HAVE_DCGETTEXT', 'dcgettext', '#include<libintl.h>'],
> 
> Unused.
DONE

> 
> @@ +67,3 @@
> +cdata.set_quoted('PACKAGE_STRING', 'json-glib ' + json_version)
> +cdata.set_quoted('PACKAGE_URL', 'http://live.gnome.org/JsonGlib')
> +cdata.set_quoted('PACKAGE_VERSION', json_version)
> 
> All the `PACKAGE_*` defines can go away. We're only interested in
> `GETTEXT_PACKAGE`.
> 
> @@ +69,3 @@
> +cdata.set_quoted('PACKAGE_VERSION', json_version)
> +# FIXME Check how to do it with mingw
> +cdata.set('_JSON_EXTERN', '__attribute__((visibility("default"))) extern')
> 
> The FIXME can be fixed with:
> 
> if get_option('default_library') != 'static'
>   if host_system == 'windows'
>     conf.set('DLL_EXPORT', true)
>     if cc.get_id() == 'msvc'
>       conf.set('_JSON_EXTERN', '__declspec(dllexport) extern')
>     else
>       conf.set('_JSON_EXTERN', '__attribute__((visibility("default")))
> __declspec(dllexport) extern')
>       extra_args += ['-fvisibility=hidden']
>     endif
>   else
>     conf.set('_JSON_EXTERN', '__attribute__((visibility("default")))')
>     extra_args += ['-fvisibility=hidden']
>   endif
> endif

DONE, reusing your snipnet almost as is.


(In reply to Tim-Philipp Müller from comment #9)
> > ::: config.h.meson
> > @@ +23,3 @@
> > +#mesondefine PACKAGE_TARNAME
> > +#mesondefine PACKAGE_URL
> > +#mesondefine PACKAGE_VERSION
> > 
> > All the `PACKAGE` defines are unused, as far as I know, and are there only
> > because autoconf puts them there.
> 
> For what it's worth the config.h.meson template is no longer needed, recent
> versions just do the right thing if you leave out the input file (and git
> master has support for a description: kwarg for cdata.set() etc. if you want
> descriptions in the config.h).

I removed the config.h.meson file, as it is useless now indeed.

Anything missing here?

Comment on attachment 338635 [details] [review]
Stop providing our own marshaller and use the generic one

Note that for most of these there are also g_cclosure_marshal_* in GLib already and we could just use those here directly in case performance is a concern, then the generic marshaller would only be used for the VOID__BOXED_XYZ ones.

Actually nevermind the last comment, g_signal_new*() will figure out the right built-in marshaller for us already when we pass NULL, sorry for the noise.

Another gentle ping :)

Review of attachment 338635 [details] [review]:

I don't particularly like using the generic marshaller because the libffi → GValue → libffi transformation is slower, but hopefully *nobody* is using signals in performance critical paths.

Review of attachment 338827 [details] [review]:

Getting there.

::: json-glib/json-enum-types.c.in
@@ +2,3 @@
+#ifndef JSON_COMPILATION
+#define JSON_COMPILATION
+#endif

No, this is not right.

This will define JSON_COMPILATION when including json-enum-types.h from any dependent project, which is *definitely* not what we want.

::: json-glib/meson.build
@@ +1,2 @@
+install_header_subdir = 'json-glib-1.0/json-glib'
+install_header_dir = get_option('includedir') + '/' + install_header_subdir

This should be:

  includedir = join_paths(get_option('prefix'), get_option('includedir'))

@@ +15,3 @@
+]
+
+# FIXME remove this workaround https://github.com/mesonbuild/meson/issues/973

I guess this FIXME can be fixed, now.

@@ +60,3 @@
+install_headers(source_h, subdir: install_header_subdir)
+
+localedir = get_option('localedir')

This should really be:

  localedir = join_paths(get_option('prefix'), get_option('localedir'))

@@ +111,3 @@
+    dependencies: json_glib_dep)
+
+json_glib_validate = executable('json-glib-format',

I know it's not necessary, but you're re-using the same variable name.

::: meson.build
@@ +1,3 @@
+project('json-glib', 'c',
+  version: '1.2.3',
+  meson_version: '>= 0.35.0',

I'd bump it up to 0.37.1, since it's the version in Fedora, Debian, and the next Ubuntu release.

@@ +16,3 @@
+soversion = 0
+
+if json_version_micro.to_int() % 2 != 0

meson ≥ 0.36 has is_odd() for this case.

Review of attachment 338827 [details] [review]:

::: json-glib/json-enum-types.c.in
@@ +2,3 @@
+#ifndef JSON_COMPILATION
+#define JSON_COMPILATION
+#endif

This is the .c file so it is obviously being built... It fixes corner cases when building a subproject of in a subproject

::: json-glib/meson.build
@@ +1,2 @@
+install_header_subdir = 'json-glib-1.0/json-glib'
+install_header_dir = get_option('includedir') + '/' + install_header_subdir

No need to specify prefix no, using join_paths now though.

@@ +60,3 @@
+install_headers(source_h, subdir: install_header_subdir)
+
+localedir = get_option('localedir')

Done.

@@ +111,3 @@
+    dependencies: json_glib_dep)
+
+json_glib_validate = executable('json-glib-format',

Fixed.

::: meson.build
@@ +1,3 @@
+project('json-glib', 'c',
+  version: '1.2.3',
+  meson_version: '>= 0.35.0',

Done

@@ +16,3 @@
+soversion = 0
+
+if json_version_micro.to_int() % 2 != 0

Done.

Created attachment 345210 [details] [review]
meson: Add support for building with the meson build system

Review of attachment 345210 [details] [review]:

Looks generally good for an initial merge; still a couple of issues.

::: json-glib/json-gvariant.c
@@ +762,3 @@
   GList *children = NULL;
   gboolean roll_back = FALSE;
+  const gchar *orig_signature = NULL;

This is an extraneous chunk, and should be in a separate commit.

::: meson.build
@@ +61,3 @@
+    endif
+  else
+    cdata.set('_JSON_EXTERN', '__attribute__((visibility("default")))')

This is missing an `extern` after the `__attribute__`.

@@ +66,3 @@
+endif
+
+configure_file(output: 'config.h', configuration: cdata)

I'd rather have this in the `json-glib` sub-directory.

Comment on attachment 338635 [details] [review]
Stop providing our own marshaller and use the generic one

Attachment 338635 [details] pushed as d77a1c8 - Stop providing our own marshaller and use the generic one

Attachment 345210 [details] pushed as 234ae81 - meson: Add support for building with the meson build system

I've also pushed a series of follow up commits that make the Meson build equivalent to the autotools one:

  - API reference
  - man pages
  - localization

Thanks again for starting this!