Review of attachment 361245 [details] [review]:

::: c/meson.build
@@ +1544,3 @@
+# HACK
+glib_conf.set('G_THREAD_LIBS', '-pthread')
+glib_conf.set('G_THREAD_CFLAGS', '-pthread')

We really need something from Meson's support of threading libraries, here. Meson is hiding all threading support under a "magic" dependency object, but we don't know what kind of compiler and linker flags we should be using.

*** Bug 789850 has been marked as a duplicate of this bug. ***

(In reply to Emmanuele Bassi (:ebassi) from comment #1)
> We really need something from Meson's support of threading libraries, here.
> Meson is hiding all threading support under a "magic" dependency object, but
> we don't know what kind of compiler and linker flags we should be using.

Could you open an issue on meson then, please?

This is particularly critical because some projects assume that GLib will automatically give them a pthread dependency, and will start failing — see bug 791052

(In reply to Xavier Claessens from comment #3)
> (In reply to Emmanuele Bassi (:ebassi) from comment #1)
> > We really need something from Meson's support of threading libraries, here.
> > Meson is hiding all threading support under a "magic" dependency object, but
> > we don't know what kind of compiler and linker flags we should be using.
> 
> Could you open an issue on meson then, please?

Done: https://github.com/mesonbuild/meson/issues/2725

I think moving to the Meson pkg-config generator will fix this. If it doesn't, we should fix that. That's a better route than adding support for fetching cflags/libs from dependencies because that's easy to misuse, and is unlikely to get approval upstream because of that.

Created attachment 365334 [details] [review]
Meson: Use pkgconfig.generate() instead of .pc.in files

With that patch and the one attached in the meson issue, it generates this pc file:

prefix=/opt/gnome
libdir=${prefix}/lib/x86_64-linux-gnu
includedir=${prefix}/include

glib_genmarshal=glib-genmarshal
gobject_query=gobject-query
glib_mkenums=glib-mkenums

Name: GLib
Description: C Utility Library
Version: 2.55.0
Requires.private: libpcre
Libs: -L${libdir} -lglib-2.0
Libs.private: -pthread
Cflags: -I${includedir}/glib-2.0 -I${libdir}/glib-2.0/include


One difference is it does not add -lpcre in libs.private but is it needed since it's in requires.private already? Also adding "-I${libdir}/glib-2.0/include" in extra_cflags is ugly, maybe meson should support having include from libdir.

(In reply to Xavier Claessens from comment #8)

> One difference is it does not add -lpcre in libs.private but is it needed
> since it's in requires.private already?

It's not needed if it's in the requires field.

> Also adding
> "-I${libdir}/glib-2.0/include" in extra_cflags is ugly, maybe meson should
> support having include from libdir.

I think this is pretty much an exception, so I can live with it being an extra Cflag.

Review of attachment 365334 [details] [review]:

Thanks, I was going to work on this, but this patch looks like a good start already.

Care to also move all the other pkg-config files to pkgconfig.generate()? I'm mostly worried about gmodule.

There's also gthread-2.0, which is a backward compatibility pkg-config, but it also requires the @G_THREAD_CFLAGS@ and @GT_THREAD_LIBS@ expansions.

Created attachment 365387 [details] [review]
Meson: Use pkgconfig module to generate all pc files

Created attachment 365388 [details]
Generated pc files

Here is the generated pc files on my system. Probably a few things to tweak still.

Marking this bug as depending on bug #790837 to have the extra dependencies (xargs, selinux).

Created attachment 365390 [details] [review]
Meson: Use pkgconfig module to generate all pc files

Created attachment 365391 [details]
Generated pc files

This is getting closer. The differences I noticed:

 - Old pc files often have both "Requires" and the corresponding -lfoo flag in "Libs". With the generated pc files if the dependency comes from a pc file then it only add it in Requires or Requeres.private. I think it's actually the right thing to do, right?

- Some of the old pc files have `Cflags: -pthread`. That seems bogus, it should be only in Libs, no? Generated pc files doesn't have that.

- There are missing substitutions: @G_MODULE_LDFLAGS@, @G_MODULE_LIBS@, @G_LIBS_EXTRA@, @GLIB_EXTRA_CFLAGS@, 

- I don't know what win32/glibpc.py does. It seems to generate pc files? Can it be dropped?

Created attachment 365394 [details] [review]
Meson: Use pkgconfig module to generate all pc files

(In reply to Xavier Claessens from comment #15)
> - There are missing substitutions: @G_MODULE_LDFLAGS@, @G_MODULE_LIBS@,
> @G_LIBS_EXTRA@, @GLIB_EXTRA_CFLAGS@, 

Actually @G_MODULE_LIBS@ was not missing.

Created attachment 365548 [details] [review]
Meson: Use pkgconfig module to generate all pc files

Updated the patch, the proposed changes in meson is now smart enough to take all dependencies from the library object you pass so you don't have to pass them manually anymore. That's much less error prone.

For example, gio-2.0.pc now has:
Libs.private: -L${libdir} -lxdgmime -linotify -lmount -ldl -lresolv
Compared to Ubuntu's:
Libs.private: -lz -lresolv  -lselinux

Note that -lxdgmime and -linotify internal libraries were missing, I think that's a bug in the autotools generated pc file. Unless I'm mistaken.

Created attachment 366144 [details] [review]
Meson: Use pkgconfig module to generate all pc files

This requires improved pc file generator from meson 0.45.

Created attachment 366145 [details]
Generated .pc files

My patches landed in Meson master, to be released in 0.45.0.

Review of attachment 366144 [details] [review]:

::: gio/meson.build
@@ +755,3 @@
+)
+
+pkg.generate(requires : ['gobject-2.0', 'gio-2.0'],

This should only be generated when building gio on Unix-like platforms, not unconditionally.

@@ +764,3 @@
+)
+
+pkg.generate(requires : ['gobject-2.0', 'gmodule-no-export-2.0', 'gio-2.0'],

Same as above, but for Windows.

::: glib/meson.build
@@ +238,3 @@
+  subdirs : ['glib-2.0'],
+  extra_cflags : ['-I${libdir}/glib-2.0/include'],
+  variables : ['glib_genmarshal=glib-genmarshal',

I know that we currently use unprefixed variables here, and rely on $PATH being set; but I honestly think this should be changed to use `${bindir}`. This allows the pkg-config file to point to the right binary without having to deal with PATH.

Created attachment 366156 [details] [review]
Meson: Use pkgconfig module to generate all pc files

This requires improved pc file generator from meson 0.45.

(In reply to Emmanuele Bassi (:ebassi) from comment #22)
> ::: glib/meson.build
> @@ +238,3 @@
> +  subdirs : ['glib-2.0'],
> +  extra_cflags : ['-I${libdir}/glib-2.0/include'],
> +  variables : ['glib_genmarshal=glib-genmarshal',
> 
> I know that we currently use unprefixed variables here, and rely on $PATH
> being set; but I honestly think this should be changed to use `${bindir}`.
> This allows the pkg-config file to point to the right binary without having
> to deal with PATH.

I agree they should be prefixed with ${bindir}. Unfortunately meson's pkg-config generator doesn't define ${bindir} so it will need to be fixed there first.

Speaking about this, I've noticed that Ubuntu patched gio-2.0.pc to have them prefixed:

glib_compile_schemas=${prefix}/lib/x86_64-linux-gnu/glib-2.0/glib-compile-schemas
glib_compile_resources=${prefix}/lib/x86_64-linux-gnu/glib-2.0/glib-compile-resources

(In reply to Xavier Claessens from comment #24)
> (In reply to Emmanuele Bassi (:ebassi) from comment #22)
> > ::: glib/meson.build
> > @@ +238,3 @@
> > +  subdirs : ['glib-2.0'],
> > +  extra_cflags : ['-I${libdir}/glib-2.0/include'],
> > +  variables : ['glib_genmarshal=glib-genmarshal',
> > 
> > I know that we currently use unprefixed variables here, and rely on $PATH
> > being set; but I honestly think this should be changed to use `${bindir}`.
> > This allows the pkg-config file to point to the right binary without having
> > to deal with PATH.
> 
> I agree they should be prefixed with ${bindir}. Unfortunately meson's
> pkg-config generator doesn't define ${bindir} so it will need to be fixed
> there first.

That would be good.

> Speaking about this, I've noticed that Ubuntu patched gio-2.0.pc to have
> them prefixed:
> 
> glib_compile_schemas=${prefix}/lib/x86_64-linux-gnu/glib-2.0/glib-compile-
> schemas
> glib_compile_resources=${prefix}/lib/x86_64-linux-gnu/glib-2.0/glib-compile-
> resources

That's a pretty weird patch, but I guess Debian packaging rules say that binaries installed by a library ought to go into libexec. Except, now that I think about it, libexec is /usr/bin on Debian. It's not like we wouldn't namespace the binary, if we ever broke the API.

I have no idea why they do that, because they have a symlink:
/usr/bin/glib-compile-schemas -> ../lib/x86_64-linux-gnu/glib-2.0/glib-compile-schemas

Anyway, let's ignore that.

Created attachment 366157 [details] [review]
Meson: Use pkgconfig module to generate all pc files

This requires improved pc file generator from meson 0.45.

Actually don't really need ${bindir}, we can just write the full path, right?

${bindir} is better because pkg-config barfs on spaces in paths unless they're quoted in the precise way it expects them to be, and only the meson pkg-config generator knows how to do that.

All variables in a pkg-config file that might contain paths must be expressed in terms of ${variable} expansions.

Also, all path variables must be derived from ${prefix} otherwise your pkg-config file is not relocatable.

Review of attachment 366157 [details] [review]:

We should not be using absolute paths in the tools paths; we either have to wait until Meson's pkg-config generator sets `${bindir}` as `${prefix}/bin`, or we need to add it as a custom variable ourselves.

Ideally, we should be able to use `get_option('bindir')`, but of course builders will pass an absolute path, which means the `bindir` variable won't be relative to the `prefix` one — unless, of course, Meson's pkgconfig module checked for the presence of the contents of the `prefix` option inside an absolute `bindir` path, and if it found them, it would replace them with the `${prefix}` string.

(In reply to Emmanuele Bassi (:ebassi) from comment #31)
> Ideally, we should be able to use `get_option('bindir')`, but of course
> builders will pass an absolute path, which means the `bindir` variable won't
> be relative to the `prefix` one — unless, of course, Meson's pkgconfig
> module checked for the presence of the contents of the `prefix` option
> inside an absolute `bindir` path, and if it found them, it would replace
> them with the `${prefix}` string.

You should use `get_option('bindir')`. That will always return a value relative to prefix, even if the builders pass an absolute path to --bindir. meson does not allow bindir/libdir/etc to be outside of prefix, and it chops the prefix off when setting the value.

I just realized that this is not explicitly documented anywhere. I will update the reference manual.

Created attachment 366205 [details] [review]
Meson: Use pkgconfig module to generate all pc files

This requires improved pc file generator from meson 0.45.

Created attachment 366249 [details] [review]
Meson: Add export-dynamic flag

Created attachment 366328 [details] [review]
Meson: Add carbon and cocoa flags into glib and gio pc files

Created attachment 369351 [details] [review]
Meson: Use pkgconfig module to generate all pc files

This requires improved pc file generator from meson 0.45.

Created attachment 369352 [details] [review]
Meson: Add export-dynamic flag

Created attachment 369353 [details] [review]
Meson: Add carbon and cocoa flags into glib and gio pc files

*** Bug 794123 has been marked as a duplicate of this bug. ***

Review of attachment 369351 [details] [review]:

Looks good.

Review of attachment 369352 [details] [review]:

::: meson.build
@@ +1681,3 @@
+
+export_dynamic_flag = ''
+if host_system != 'windows'

I'm not really sure this'll work on any C compiler on non-Windows platforms.

This should probably flipped to:

```
if host_system != 'windows' and (cc.get_id() == 'gcc' or cc.get_id() == 'clang')
  export_dynamic_flags = ...
endif
```

Review of attachment 369353 [details] [review]:

Okay

Review of attachment 369353 [details] [review]:

Not a blocker, but reading this again made me think that those flags should be pulled automatically by the pc generator. So I opened a feature request for it:
https://github.com/mesonbuild/meson/issues/3334

(In reply to Emmanuele Bassi (:ebassi) from comment #41)
> Review of attachment 369352 [details] [review] [review]:
> This should probably flipped to:
> 
> ```
> if host_system != 'windows' and (cc.get_id() == 'gcc' or cc.get_id() ==
> 'clang')
>   export_dynamic_flags = ...
> endif
> ```

I have no idea where it's supported tbh. Maybe we should actually test if the compiler supports that flag, that would be another case for cc.has_link_argument()?

Meson has internally this func:
    def gen_export_dynamic_link_args(self, env):
        if for_windows(env.is_cross_build(), env) or for_cygwin(env.is_cross_build(), env):
            return ['-Wl,--export-all-symbols']
        elif for_darwin(env.is_cross_build(), env):
            return []
        else:
            return ['-Wl,-export-dynamic']

I opened a PR to expose that in meson's API:
https://github.com/mesonbuild/meson/pull/3460/commits/c35cc2650df5d0d51eb6e712dc1a7d08d9315856

Review of attachment 369352 [details] [review]:

::: gmodule/meson.build
@@ +104,3 @@
   dependencies : [libdl_dep, libglib_dep],
+  c_args : ['-DG_LOG_DOMAIN="GModule"', '-DG_DISABLE_DEPRECATED'] + glib_hidden_visibility_args,
+  link_args : [export_dynamic_flag])

Actually I have a doubt here: Why does libgmodule have this flag? Makefile.am does it too, but it makes no sense for a library, does it?

Created attachment 372127 [details] [review]
Meson: Add export-dynamic flag

--

* I installed cygwin and checked their gmodule-2.0.pc, it has 
'-Wl,--export-all-symbols'.

* I checked the .pc on macos and it has no flag, that's consistent with 
Meson's implementation.

* For Windows the flag got removed in 2002 in commit a488638747 with no 
real explanation other that is supposedly create issues. Let's copy that 
behaviour for now and if we think it's wrong we should fix both 
autotools and meson in a separate bug.

* I did not add this flag to build libgmodule, unlike Autotools because 
AFAIK that's useless. I'll open a separate bug to remove it from the 
Makefile.am, unless someone knows the reason?

(In reply to Xavier Claessens from comment #45)
> I opened a PR to expose that in meson's API:
> https://github.com/mesonbuild/meson/pull/3460/commits/
> c35cc2650df5d0d51eb6e712dc1a7d08d9315856

Sadly Jussi rejected that idea, I think he's wrong, but what can I do...

(In reply to Emmanuele Bassi (:ebassi) from comment #41)
> Review of attachment 369352 [details] [review] [review]:
> I'm not really sure this'll work on any C compiler on non-Windows platforms.

Currently Meson use that flag for all its compilers. Could be a bug in Meson, who knows... That's exactly the reason why I strongly think that flag must come from Meson.

Review of attachment 372127 [details] [review]:

The commit log could do with being a bit more explicit, but looks good to me.

Attachment 372127 [details] pushed as 487b1fd - Meson: Add export-dynamic flag

(In reply to Emmanuele Bassi (:ebassi) from comment #49)
> Review of attachment 372127 [details] [review] [review]:
> 
> The commit log could do with being a bit more explicit, but looks good to me.

Xavier, for future reference, I’d take that to mean “please make the commit message more explicit and then push”. Making changes to the build system like this without explanations is going to make `git log` archaeology in future just as hard as you’re finding it with autotools now.

(In reply to Xavier Claessens from comment #47)
> * I did not add this flag to build libgmodule, unlike Autotools because 
> AFAIK that's useless. I'll open a separate bug to remove it from the 
> Makefile.am, unless someone knows the reason?

You mean the use of $(G_MODULE_LDFLAGS) in libgmodule_2_0_la_LDFLAGS? I have no idea why that’s there. Please do open a separate bug about it.

Something I noticed while looking at this: gmodule-2.0.pc and gmodule-export-2.0.pc are identical, but that isn’t immediately obvious from the gmodule/meson.build file. Perhaps we could change it so that gmodule-2.0.pc is generated just by copying the generated gmodule-export-2.0.pc file? They’ve always been the same (since commit 117ae23f5d09ed22a7c0202e9760260b46d9afd5, when gmodule-export-2.0.pc was introduced). Could you file another bug about that please?

(In reply to Philip Withnall from comment #51)
> (In reply to Emmanuele Bassi (:ebassi) from comment #49)
> > Review of attachment 372127 [details] [review] [review] [review]:
> > 
> > The commit log could do with being a bit more explicit, but looks good to me.
> 
> Xavier, for future reference, I’d take that to mean “please make the commit
> message more explicit and then push”. Making changes to the build system
> like this without explanations is going to make `git log` archaeology in
> future just as hard as you’re finding it with autotools now.

Argh, pushed too fast, indeed forgot to edit the commit msg. otoh, it has the link to this bug with all the context, just a bit more work to dig it. Sorry.

> (In reply to Xavier Claessens from comment #47)
> > * I did not add this flag to build libgmodule, unlike Autotools because 
> > AFAIK that's useless. I'll open a separate bug to remove it from the 
> > Makefile.am, unless someone knows the reason?
> 
> You mean the use of $(G_MODULE_LDFLAGS) in libgmodule_2_0_la_LDFLAGS? I have
> no idea why that’s there. Please do open a separate bug about it.

bug #796238.

> Something I noticed while looking at this: gmodule-2.0.pc and
> gmodule-export-2.0.pc are identical, but that isn’t immediately obvious from
> the gmodule/meson.build file. Perhaps we could change it so that
> gmodule-2.0.pc is generated just by copying the generated
> gmodule-export-2.0.pc file? They’ve always been the same (since commit
> 117ae23f5d09ed22a7c0202e9760260b46d9afd5, when gmodule-export-2.0.pc was
> introduced). Could you file another bug about that please?

bug #796239.