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 794365 - Different meson improvements
Different meson improvements
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: build
git master
Other Linux
: Normal normal
: ---
Assigned To: gvfs-maint
gvfs-maint
Depends on:
Blocks:
 
 
Reported: 2018-03-15 16:54 UTC by Ondrej Holy
Modified: 2018-07-23 08:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
build: Avoid version script linker flag check (1.95 KB, patch)
2018-03-15 17:37 UTC, Iñigo Martínez
none Details | Review
build: Fix version script linker flag check (989 bytes, patch)
2018-03-19 21:27 UTC, Iñigo Martínez
none Details | Review
build: Remove no undefined linker flag option (873 bytes, patch)
2018-03-19 21:34 UTC, Iñigo Martínez
committed Details | Review
build: Properly define giomoduledir and session_bus_services_dir (2.97 KB, patch)
2018-03-19 21:36 UTC, Iñigo Martínez
none Details | Review
build: Reuse schemas and GIO modules paths variables (1.76 KB, patch)
2018-03-19 21:37 UTC, Iñigo Martínez
committed Details | Review
build: Fix version script linker flag check (2.34 KB, patch)
2018-03-19 22:12 UTC, Iñigo Martínez
needs-work Details | Review
build: Remove missing unnecessary defines (951 bytes, patch)
2018-03-20 07:53 UTC, Iñigo Martínez
committed Details | Review
build: Improve header and function checking (7.22 KB, patch)
2018-03-20 07:53 UTC, Iñigo Martínez
committed Details | Review
build: Properly define giomoduledir and session_bus_services_dir (3.35 KB, patch)
2018-03-21 19:57 UTC, Iñigo Martínez
committed Details | Review
build: Apply a workaround for D-Bus code generation (4.81 KB, patch)
2018-03-23 20:21 UTC, Iñigo Martínez
committed Details | Review
build: Revise dependencies (13.51 KB, patch)
2018-03-26 20:51 UTC, Iñigo Martínez
committed Details | Review
Remove redundant gmodule.h includes (3.41 KB, patch)
2018-03-27 12:25 UTC, Ondrej Holy
committed Details | Review
build: Use find_program to find codegen.py (1.10 KB, patch)
2018-04-05 08:30 UTC, Iñigo Martínez
committed Details | Review
build: Fix type when searching for util.h (991 bytes, patch)
2018-04-11 14:57 UTC, Iñigo Martínez
committed Details | Review
build: Simplify openpty check (1.04 KB, patch)
2018-04-12 06:31 UTC, Ondrej Holy
committed Details | Review

Description Ondrej Holy 2018-03-15 16:54:19 UTC
I see the following with meson:
WARNING: -Wl,--version-script looks like a linker argument, but has_argument
and other similar methods only support checking compiler arguments.
Using them to check linker arguments are never supported, and results
are likely to be wrong regardless of the compiler you are using.

The warning was added by:
https://github.com/mesonbuild/meson/pull/2971

We can't do much with it right now probably:
https://github.com/mesonbuild/meson/issues/3047
Comment 1 Iñigo Martínez 2018-03-15 17:37:43 UTC
Created attachment 369751 [details] [review]
build: Avoid version script linker flag check

gvfs is checking compiler's support for version script linker flag, which is wrong.

This has been changed to adding this linker flag when the host system is running linux, assuming that the used compiler has support for this, because there is no support at the moment to check linker flags support.
Comment 2 Ondrej Holy 2018-03-16 07:51:56 UTC
Review of attachment 369751 [details] [review]:

Thanks! It seems that the system names are standardized already, so it should do the job... but do all Linux linkers support version scripts (though not sure that somebody really uses something else than GNU linker)?
Comment 3 Iñigo Martínez 2018-03-16 13:58:04 UTC
Every linker I know can handle it (GNU ld, GNU gold, LLVM lld). My biggest concern at the moment is BSD, because this patch would limit "version-script" only to linux.

I'm also adding Ting-Wei Lan to this bug who has helped before with BSD related issues.

Ting-Wei, how is the support for "version-script" in BSD?
Comment 4 Ting-Wei Lan 2018-03-16 15:54:31 UTC
(In reply to Iñigo Martínez from comment #3)
> Every linker I know can handle it (GNU ld, GNU gold, LLVM lld). 

Yes, all major four BSDs (FreeBSD, OpenBSD, NetBSD, DragonflyBSD) can handle it. They all use one of the above linkers as system linkers.

FreeBSD uses GNU ld 2.17.50 and there is a plan to switch to LLVM lld.
OpenBSD uses GNU ld 2.17.
NetBSD uses GNU ld 2.23.2.
DragonflyBSD uses GNU gold 1.12 (GNU Binutils 2.27)

> My biggest
> concern at the moment is BSD, because this patch would limit
> "version-script" only to linux.
>
> I'm also adding Ting-Wei Lan to this bug who has helped before with BSD
> related issues.
> 
> Ting-Wei, how is the support for "version-script" in BSD?

Yes, it is not good to check for Linux when a feature is not specific to Linux kernel. Most desktop libraries and services are the same on *BSD and Linux.

(In reply to Iñigo Martínez from comment #1)
> Created attachment 369751 [details] [review] [review]
> build: Avoid version script linker flag check
> 
> gvfs is checking compiler's support for version script linker flag, which is
> wrong.
> 
> This has been changed to adding this linker flag when the host system is
> running linux, assuming that the used compiler has support for this, because
> there is no support at the moment to check linker flags support.

It is possible to check for linker flags with 'cc.links'. Please see the gnome-builder bug I filed and the commit that fixes it:

https://gitlab.gnome.org/GNOME/gnome-builder/issues/369
https://gitlab.gnome.org/GNOME/gnome-builder/commit/428cc6b8544a

However, '--version-script' is only meaningful for shared libraries. You can pass it when linking an executable on GNU/Linux, OpenBSD, NetBSD, DragonflyBSD, but doing so causes error on FreeBSD. You will probably have to pass it along with '-shared' in order to test it.
Comment 5 Iñigo Martínez 2018-03-19 21:27:36 UTC
Created attachment 369877 [details] [review]
build: Fix version script linker flag check

(In reply to Ting-Wei Lan from comment #4)
> It is possible to check for linker flags with 'cc.links'. Please see the
> gnome-builder bug I filed and the commit that fixes it:
> 
> https://gitlab.gnome.org/GNOME/gnome-builder/issues/369
> https://gitlab.gnome.org/GNOME/gnome-builder/commit/428cc6b8544a
> 
> However, '--version-script' is only meaningful for shared libraries. You can
> pass it when linking an executable on GNU/Linux, OpenBSD, NetBSD,
> DragonflyBSD, but doing so causes error on FreeBSD. You will probably have
> to pass it along with '-shared' in order to test it.

Thank you very much! It looks a good solution for this missing feature in meson.

I have applied the same approach. In this case it only applies to shared libraries, so there is no need to worry.
Comment 6 Iñigo Martínez 2018-03-19 21:34:28 UTC
Created attachment 369878 [details] [review]
build: Remove no undefined linker flag option

There is no need to set the option to enable `no-undefined` linker flag in the default options as it is already enabled by default.
Comment 7 Iñigo Martínez 2018-03-19 21:36:23 UTC
Created attachment 369879 [details] [review]
build: Properly define giomoduledir and session_bus_services_dir

Following the recommendations[0], the `giomoduledir` and `session_bus_services_dir` use the information from the `pc` files where they are defined, gio-2.0.pc and dbus-1.pc. However, these variables are relative to some other variable which is set to be a directory under prefix, where the user should be able to write.

On the other hand, systemd related paths, `systemduserunitdir` and `tmpfilesdir` can not use this approach because they use absolute paths, so there is no way to set those paths to directories under prefix.

The `gio_module_dir` and `dbus_services_dir` have also been removed because they are not necessary anymore.

[0] https://www.bassi.io/articles/2018/03/15/pkg-config-and-paths/
Comment 8 Iñigo Martínez 2018-03-19 21:37:48 UTC
Created attachment 369880 [details] [review]
build: Reuse schemas and GIO modules paths variables

meson's post install script, compiles glib schemas and creates a cache from the GIO modules. To do this, an assumption is made regarding the directories where the files are installed under prefix.

However, the final directories are available as meson variables and these can be passed to the post install script, so there is no need to make any assumption.
Comment 9 Iñigo Martínez 2018-03-19 22:12:17 UTC
Created attachment 369881 [details] [review]
build: Fix version script linker flag check

An update to the patch where the symbol map file is used when checking the flag support.
Comment 10 Iñigo Martínez 2018-03-20 07:53:06 UTC
Created attachment 369894 [details] [review]
build: Remove missing unnecessary defines

There are some PACKAGE_* defines that are not used anywhere. They were left for their future use, but they can be readded at any moment.
Comment 11 Iñigo Martínez 2018-03-20 07:53:48 UTC
Created attachment 369895 [details] [review]
build: Improve header and function checking

When checking for a header or a function, usually the define name is also specified. However, most of the time, the define name is a pattern based on the header name, the function name or the version to be checked.

This has improved so the define name is formed from the original information.
Comment 12 Ondrej Holy 2018-03-20 11:21:41 UTC
Review of attachment 369878 [details] [review]:

Sure, thanks!
Comment 13 Ondrej Holy 2018-03-20 11:21:55 UTC
Review of attachment 369880 [details] [review]:

Looks ok, thanks!
Comment 14 Ondrej Holy 2018-03-20 11:22:57 UTC
Review of attachment 369881 [details] [review]:

Seems ok, thanks.
Comment 15 Ondrej Holy 2018-03-20 11:23:05 UTC
Review of attachment 369894 [details] [review]:

Sure, thanks.
Comment 16 Ondrej Holy 2018-03-20 13:10:40 UTC
Review of attachment 369895 [details] [review]:

Hmm, looks ok, though not sure that this isn't a bit counterproductive...  with this patch is a bit harder to find out where is each macro defined. Is this common in other projects?
Comment 17 Iñigo Martínez 2018-03-20 14:01:51 UTC
I don't have a strong opinion about it. I also though that it might be harder to understand. However, this is exactly what autotools does and it might be less error prone.

There is no consensus about this, but glib[0][1] that can be seen as reference also does it[0][1].

[0] https://gitlab.gnome.org/GNOME/glib/blob/master/meson.build#L203
[1] https://gitlab.gnome.org/GNOME/glib/blob/master/meson.build#L257
Comment 18 Ondrej Holy 2018-03-20 14:13:02 UTC
Review of attachment 369879 [details] [review]:

Hmm, the `gio_module_dir` and `dbus_services_dir` options are probably not more needed with this change (though jhbuild modulesets should be updated accordingly). So we can also probably remove them from the configuration summary, or print just gvfs_prefix instead.

The `systemduserunitdir` and `tmpfilesdir` are still needed anyway due to some alternatives...
Comment 19 Ting-Wei Lan 2018-03-20 15:12:24 UTC
Review of attachment 369881 [details] [review]:

::: meson.build
@@ +239,2 @@
 ldflag = '-Wl,--version-script'
+have_version_script = cc.links('int main () { return 0; }', name: ldflag, args: ','.join([ldflag, client_symbol_map]))

Sorry, this still doesn't work on FreeBSD. As what I said in comment #4, 'cc.links' works for many linker flags but not '--version-script' on FreeBSD. It can be used when linking a shared library, but 'cc.links' links an executable.

Messages in meson-log.txt on FreeBSD:

Command line:  clang -L/home/lantw44/gnome/devinstall/lib -L/usr/local/lib /tmp/tmprpkp3hk_/testfile.c -march=corei7 -B/home/lantw44/.local/bin -pipe -g3 -O0 -O0 -Wl,--version-script,/home/lantw44/gnome/source/gvfs/client/symbol.map -D_FILE_OFFSET_BITS=64 -o /tmp/tmprpkp3hk_/output.exe 

Code:
 int main () { return 0; }
Compiler stdout:
 
Compiler stderr:
 /home/lantw44/.local/bin/ld: /tmp/tmprpkp3hk_/output.exe: local symbol `environ' in /usr/lib/crt1.o is referenced by DSO
/home/lantw44/.local/bin/ld: final link failed: Bad value
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Checking if "-Wl,--version-script" links: NO

I know this means '-Wl,--version-script' is usually not used on FreeBSD because this kind of broken check can also be found in many autotools-based projects. I am not sure what is the best solution. The only solution I think of is to pass '-shared' to 'cc.links' to make it link a shared library.

You probably wonder why my linker is located under my home directory. /home/lantw44/.local/bin/ld is just a symlink to the real linker (GNU ld 2.30 here). The reason of doing so is to allow me easily changing the linker or replacing it with a shell wrapper during testing and debugging. It doesn't change the behaviour of the linker.
Comment 20 Ting-Wei Lan 2018-03-20 15:21:23 UTC
I did the '-Wl,--version-script' check on FreeBSD again. Yes, it cannot be used when linking an executable with GNU ld, but it can be used when linking with GNU gold and LLVM lld.

$ echo 'int main () { return 0; }' > conftest.c

$ cc -fuse-ld=ld -Wl,--version-script=symbol.map main.c
/usr/bin/ld: a.out: local symbol `__progname' in /usr/lib/crt1.o is referenced by DSO
/usr/bin/ld: final link failed: Nonrepresentable section on output
cc: error: linker command failed with exit code 1 (use -v to see invocation)

$ cc -fuse-ld=gold -Wl,--version-script=symbol.map main.c
(no error)

$ cc -fuse-ld=lld -Wl,--version-script=symbol.map main.c
(no error)

The default value is '-fuse-ld=ld'.
Comment 21 Iñigo Martínez 2018-03-21 18:27:50 UTC
attachment 369878 [details] [review] pushed as 4a4d1f8c - build: Remove no undefined linker flag option
attachment 369880 [details] [review] pushed as ed563a1b - build: Reuse schemas and GIO modules paths variables
attachment 369894 [details] [review] pushed as 285c513e - build: Remove missing unnecessary defines
Comment 22 Iñigo Martínez 2018-03-21 18:49:37 UTC
(In reply to Ting-Wei Lan from comment #20)
> I did the '-Wl,--version-script' check on FreeBSD again. Yes, it cannot be
> used when linking an executable with GNU ld, but it can be used when linking
> with GNU gold and LLVM lld.
> 
> $ echo 'int main () { return 0; }' > conftest.c
> 
> $ cc -fuse-ld=ld -Wl,--version-script=symbol.map main.c
> /usr/bin/ld: a.out: local symbol `__progname' in /usr/lib/crt1.o is
> referenced by DSO
> /usr/bin/ld: final link failed: Nonrepresentable section on output
> cc: error: linker command failed with exit code 1 (use -v to see invocation)
> 
> $ cc -fuse-ld=gold -Wl,--version-script=symbol.map main.c
> (no error)
> 
> $ cc -fuse-ld=lld -Wl,--version-script=symbol.map main.c
> (no error)
> 
> The default value is '-fuse-ld=ld'.

There doesn't seem to be a good solution to this. I have added a new comment[0] in which I state that this would be a desired feature.

I have left the patch without pushing it to master. Let's see how this evolves.

[0] https://github.com/mesonbuild/meson/issues/3047#issuecomment-375054872
Comment 23 Iñigo Martínez 2018-03-21 19:57:28 UTC
Created attachment 369977 [details] [review]
build: Properly define giomoduledir and session_bus_services_dir

(In reply to Ondrej Holy from comment #18)
> Review of attachment 369879 [details] [review] [review]:
> 
> Hmm, the `gio_module_dir` and `dbus_services_dir` options are probably not
> more needed with this change (though jhbuild modulesets should be updated
> accordingly). So we can also probably remove them from the configuration
> summary, or print just gvfs_prefix instead.
> 
> The `systemduserunitdir` and `tmpfilesdir` are still needed anyway due to
> some alternatives...

I have updated the patch, which also removes `gio_module_dir` and `dbus_services_dir` form the configuration summary.

Regarding jhbuild, the only option that affects it is `dbus_services_dir`[0]. In fact, this change does what it tries to do.

gnome-continuous doesn't need any change at the moment, because it forces autotools to avoid the `gdbus-codegen` issue.

[0] https://git.gnome.org/browse/jhbuild/tree/modulesets/gnome-suites-core-3.28.modules#n454
Comment 24 Ondrej Holy 2018-03-22 07:43:14 UTC
Comment on attachment 369881 [details] [review]
build: Fix version script linker flag check

Changing status as per Comment 22.
Comment 25 Ondrej Holy 2018-03-22 07:43:33 UTC
Comment on attachment 369895 [details] [review]
build: Improve header and function checking

(In reply to Iñigo Martínez from comment #17)
> ...
> There is no consensus about this, but glib[0][1] that can be seen as
> reference also does it[0][1].

Ok, let's go with this also...
Comment 26 Ondrej Holy 2018-03-22 07:46:23 UTC
Review of attachment 369977 [details] [review]:

Thanks, feel free to push with the following change:

::: meson.build
@@ +269,3 @@
 # *** Check for dbus service dir ***
+dbus_dep = dependency('dbus-1', required: false)
+assert(dbus_dep.found(), 'dbus-1 required but not found, please provide a valid D-Bus service dir')

I suppose that "dbus_dep = dependency('dbus-1')" should be here and the assert can be removed consequently.
Comment 27 Iñigo Martínez 2018-03-23 19:40:10 UTC
attachment 369895 [details] [review] pushed as f2163d2d - build: Improve header and function checking
attachment 369977 [details] [review] pushed as f3bc7a6e - build: Properly define giomoduledir and session_bus_services_dir
Comment 28 Iñigo Martínez 2018-03-23 20:21:58 UTC
Created attachment 370067 [details] [review]
build: Apply a workaround for D-Bus code generation

meson uses `gdbus-codegen` for D-Bus code generation. However, both files are generated implicitly, so meson is not able to know how many files are generated, so it does generate only one opaque target that represents the two files.

A new script has been created only to call `gdbus-codegen` and simulate the generation of the source code and header as different targets.

Please see:
  https://bugzilla.gnome.org/show_bug.cgi?id=791015
  https://github.com/mesonbuild/meson/pull/2930
Comment 29 Ondrej Holy 2018-03-26 07:55:29 UTC
Review of attachment 370067 [details] [review]:

It should be ok, given the fact that this is more or less the same what has been used in gnome-settings-daemon, thanks. Can you consequently revise Bug 789877, please?
Comment 30 Iñigo Martínez 2018-03-26 20:49:32 UTC
Comment on attachment 370067 [details] [review]
build: Apply a workaround for D-Bus code generation

Pushed as 06c28a63 - build: Apply a workaround for D-Bus code generation

(In reply to Ondrej Holy from comment #29)
> Review of attachment 370067 [details] [review] [review]:
> 
> It should be ok, given the fact that this is more or less the same what has
> been used in gnome-settings-daemon, thanks. Can you consequently revise Bug
> 789877, please?

Sure! I've been trying it in my personal computer and the issues seem to be fixed. I'll also try to contact ebassi to check if it builds properly in gnome-continuous.
Comment 31 Iñigo Martínez 2018-03-26 20:51:10 UTC
Created attachment 370164 [details] [review]
build: Revise dependencies

gvfs is using a set of glib libraries (gio-2.0, gio-unix-2.0, glib, gobject-2.0) in almost all the created objects. However, these dependencies are not always necessary.

gvfs' meson port also uses some internal dependencies formed by built libraries. This internal dependencies depend on other dependendecies as well.

These both issues have been fixed by reviewing all the internal dependencies and built objects.
Comment 32 Ondrej Holy 2018-03-27 12:24:54 UTC
Review of attachment 370164 [details] [review]:

I see that not all objects need all the dependencies which are used for them currently. But to be honest, I don't understand the following changes... Can you give me some insight how you created this patch and why those changes are ok?

::: common/meson.build
@@ +39,3 @@
   sources: sources + [dbus_sources],
   include_directories: top_inc,
+  dependencies: deps + [gio_unix_dep],

Is really gio_unix_dep needed here? "grep -r unix common/" is silent. Similarly with some other gio_unix_dep usages...

@@ +48,3 @@
   sources: dbus_sources[1],
+  include_directories: common_inc,
+  dependencies: deps,

I don't understand why some dependencies (especially the different set that the shared_library has) are needed here and similarly for other declare_dependency occurrences...

::: daemon/meson.build
@@ -422,2 @@
   deps = [
-    gudev_dep,

But gudev is directly used in gphoto2/mtp backends, why this is not needed? Also on other places...
Comment 33 Ondrej Holy 2018-03-27 12:25:42 UTC
Created attachment 370186 [details] [review]
Remove redundant gmodule.h includes

Our codes don't use GModule APIs directly, so there is no need for
gmodule.h includes. GModule has been replaced by GIOModule in our codes
long time ago. gmodule.h is included indirectly over gio.h anyway.
Comment 34 Iñigo Martínez 2018-03-27 17:31:58 UTC
(In reply to Ondrej Holy from comment #32)
> Review of attachment 370164 [details] [review] [review]:
> 
> I see that not all objects need all the dependencies which are used for them
> currently. But to be honest, I don't understand the following changes... Can
> you give me some insight how you created this patch and why those changes
> are ok?

Sure! I also understand your concern, because these changes are due to a change of approach. I hope you find the following reasoning helpful.

Until now, I was mostly using internal dependencies as holders for built libraries to be linked and the directories to be included holding their headers. However, these libraries also include other headers from other dependencies, so those must also be indirect dependencies.

For example, take a look at `libgvfscommon_dep` internal dependency, which is used as a dependency by different other targets. It provides a dependency over `gvfsdbus.h`, it also links with `libgvfscommon` and finally it includes the `common` directory as a directory to search for headers.

One of such headers is `gmountspec.h`[1] which is included by different files in the source code. This file exposes the `glib.h`[2] and the `gio.h`[3] headers, so any file using these headers should also depend indirectly over `glib_dep` and `gio_dep`.

This patch provides these indirect dependencies after a detailed look at the dependencies/files.

> ::: common/meson.build
> @@ +39,3 @@
>    sources: sources + [dbus_sources],
>    include_directories: top_inc,
> +  dependencies: deps + [gio_unix_dep],
> 
> Is really gio_unix_dep needed here? "grep -r unix common/" is silent.
> Similarly with some other gio_unix_dep usages...

Yes, you are right. You haven't found it because this dependency comes from generated dbus files. The following code, comes from `gvfsdbus.c`.

  #ifdef G_OS_UNIX
  #  include <gio/gunixfdlist.h>
  #endif

Deu to this snippet of source code, the `gio_unix_dep` dependency is necessary.

> @@ +48,3 @@
>    sources: dbus_sources[1],
> +  include_directories: common_inc,
> +  dependencies: deps,
> 
> I don't understand why some dependencies (especially the different set that
> the shared_library has) are needed here and similarly for other
> declare_dependency occurrences...

The above explanation may explain this as well. `libgvfscommon` needs both `gvfsdbus.c` and `gvfsdbus.h`. `gvfsdbus.c` needs `gio_unix_dep` so this dependency is necessary for building `libgvfscommon`. However, the only exposed header in `gvfsdbus.h` is `gio.h`, so `gio_unix_dep` is not an indirect dependency needed by any target which depends on `libgvfscommon_dep`.

> ::: daemon/meson.build
> @@ -422,2 @@
>    deps = [
> -    gudev_dep,
> 
> But gudev is directly used in gphoto2/mtp backends, why this is not needed?
> Also on other places...

Following previous explanations, both `gvfsd-mtp` and `gvfsd-photo2` depend on `libgvfscommon_gphoto2_dep`, which provides an indirect dependency over `gudev_dep` because it exposes `gudev.h` header in `gvfsgphoto2utils.h`[4], so it is not necessary to include the dependency again.

[0] https://git.gnome.org/browse/gvfs/tree/common/meson.build#n41
[1] https://git.gnome.org/browse/gvfs/tree/common/gmountspec.h
[2] https://git.gnome.org/browse/gvfs/tree/common/gmountspec.h#n26
[3] https://git.gnome.org/browse/gvfs/tree/common/gmountspec.h#n27
[4] https://git.gnome.org/browse/gvfs/tree/common/gvfsgphoto2utils.h#n24
Comment 35 Iñigo Martínez 2018-03-27 17:34:58 UTC
(In reply to Ondrej Holy from comment #33)
> Created attachment 370186 [details] [review] [review]
> Remove redundant gmodule.h includes
> 
> Our codes don't use GModule APIs directly, so there is no need for
> gmodule.h includes. GModule has been replaced by GIOModule in our codes
> long time ago. gmodule.h is included indirectly over gio.h anyway.

LGTM :)

I have also tested it here and it works perfectly.
Comment 36 Ondrej Holy 2018-04-03 15:56:54 UTC
Review of attachment 370164 [details] [review]:

Thanks for the explanation. I haven't checked all changes in detail, but it works correctly for me, so why not...

(In reply to Iñigo Martínez from comment #34)
> ... 
> Following previous explanations, both `gvfsd-mtp` and `gvfsd-photo2` depend
> on `libgvfscommon_gphoto2_dep`, which provides an indirect dependency over
> `gudev_dep` because it exposes `gudev.h` header in `gvfsgphoto2utils.h`[4],
> so it is not necessary to include the dependency again.

Just to be curious, are there any drawbacks if some dependency is included multiple times?
Comment 37 Ondrej Holy 2018-04-03 15:57:55 UTC
Attachment 370164 [details] pushed as f456052 - build: Revise dependencies
Attachment 370186 [details] pushed as 9d845f7 - Remove redundant gmodule.h includes
Comment 38 Iñigo Martínez 2018-04-03 16:54:32 UTC
(In reply to Ondrej Holy from comment #36)
> Review of attachment 370164 [details] [review] [review]:
> 
> Thanks for the explanation. I haven't checked all changes in detail, but it
> works correctly for me, so why not...

Thank you. Although it also works properly here, let me know if anything is not working as it should.

> Just to be curious, are there any drawbacks if some dependency is included
> multiple times?


I don't think so.
Comment 39 Iñigo Martínez 2018-04-05 08:30:19 UTC
Created attachment 370548 [details] [review]
build: Use find_program to find codegen.py

Following Emmanuelle's wise advice, I have changed the way `codegen.py`, which was introduced in attachment 370067 [details] [review], is found to be more meson idiomatic.
Comment 40 Ondrej Holy 2018-04-06 07:59:34 UTC
Review of attachment 370548 [details] [review]:

Nice!
Comment 41 Iñigo Martínez 2018-04-06 15:23:20 UTC
Comment on attachment 370548 [details] [review]
build: Use find_program to find codegen.py

Pushed as b607fdd4 - build: Use find_program to find codegen.py
Comment 42 Iñigo Martínez 2018-04-11 14:57:53 UTC
Created attachment 370807 [details] [review]
build: Fix type when searching for util.h

There is a type in the `dependencies` parameter when searching for the `util.h` header.
Comment 43 Ondrej Holy 2018-04-12 06:30:47 UTC
Review of attachment 370807 [details] [review]:

Ops, sure...

> There is a type in the `dependencies` parameter when searching for
a typo?
Comment 44 Ondrej Holy 2018-04-12 06:31:24 UTC
Created attachment 370831 [details] [review]
build: Simplify openpty check

This prevents two messages for openpty in meson output and should
be equal.
Comment 45 Iñigo Martínez 2018-04-12 06:40:06 UTC
Comment on attachment 370807 [details] [review]
build: Fix type when searching for util.h

Pushed as 76ddb132 - build: Fix typo when searching for util.h

(In reply to Ondrej Holy from comment #43)
> Review of attachment 370807 [details] [review] [review]:
> > There is a type in the `dependencies` parameter when searching for
> a typo?

Thanks! I have pushed a fixed version.
Comment 46 Iñigo Martínez 2018-04-12 06:52:56 UTC
(In reply to Ondrej Holy from comment #44)
> Created attachment 370831 [details] [review] [review]
> build: Simplify openpty check
> 
> This prevents two messages for openpty in meson output and should
> be equal.

Probably this is correct. However, my lack of knowledge of non linux *nix systems prevents me from making a correct assessment.

Previously it tried to search for the `openpty` function in the standard library, and search in the `util` library if it was not present in the standard library.
Comment 47 Ondrej Holy 2018-04-12 07:29:59 UTC
My understanding of has_function is that it should work in all cases... if it is in standard library regardless we specify some dependencies, or not, or they are not simply found... but maybe I am wrong :-)
Comment 48 Iñigo Martínez 2018-04-12 07:39:52 UTC
Your change should be correct[0]:

* This function is missing on some platforms: AIX 5.1, HP-UX 11, IRIX 6.5, Solaris 11 2011-11.
* One some systems (at least including Cygwin, Interix, OSF/1 4 and 5, and Mac OS X) linking with -lutil is not required.
* On glibc, OpenBSD, NetBSD and FreeBSD linking with -lutil is required.
* The function is declared in pty.h on Cygwin, Interix, OSF/1 4 and 5, and glibc. It is declared in util.h on Mac OS X, OpenBSD and NetBSD. It is declared in libutil.h on FreeBSD.

It should also work on newer versions of Solaris[1]. It will find it even if `util` is not present.

[0] https://www.gnu.org/software/gnulib/manual/html_node/openpty.html
[1] https://docs.oracle.com/cd/E88353_01/html/E37843/openpty-3c.html
Comment 49 Ondrej Holy 2018-04-12 13:22:51 UTC
I wanted to say that in the following snippet...: 

foo_dep = cc.find_library('foo', required: false)
have_bar = cc.has_function('bar', dependencies: foo_dep)

...have_bar will be true when:

foo is found and provides bar
foo is found and bar is provided by the standard library
foo is not found and bar is provided by the standard library

...and thus the code should be fully equal to:

foo_dep = cc.find_library('foo', required: false)
have_bar = cc.has_function('bar')
if not have_bar
  have_bar = foo_dep.found() and cc.has_function('bar', dependencies: foo_dep)
endif

...and then it is irrelevant to talk about non-Linux systems, or openpty concretely, or am I wrong?
Comment 50 Iñigo Martínez 2018-04-12 14:08:18 UTC
You are right.

Consider a situation where the function `bar` is provided by both the standard library and the `foo` library, but you want to priorize one implementation over the other for different reasons: performance, features, security, etc.. 

I assumed that meson would prioritize the provided library over the standard library. However, I've checked it now and this is not the case[0], meson will *always* (though, i don't know if this is good) prioritize the standard library over any provided libraries, so yes, the following:

> foo_dep = cc.find_library('foo', required: false)
> have_bar = cc.has_function('bar')
> if not have_bar
>   have_bar = foo_dep.found() and cc.has_function('bar', dependencies: foo_dep)
> endif

is equivalent to:

> foo_dep = cc.find_library('foo', required: false)
> have_bar = cc.has_function('bar', dependencies: foo_dep)

It is even better because the same is expressed in less lines :)

[0] https://github.com/mesonbuild/meson/blob/master/mesonbuild/compilers/c.py#L618
Comment 51 Iñigo Martínez 2018-04-12 14:51:20 UTC
Nevermind.

Actually, it doesn't have any sense to prioritize one over the other because that is done in the code. It's enough to find if it's available in any the possible libraries.
Comment 52 Ondrej Holy 2018-04-13 08:12:09 UTC
Comment on attachment 370831 [details] [review]
build: Simplify openpty check

Attachment 370831 [details] pushed as d47e70e - build: Simplify openpty check

Thanks for the investigation :-)
Comment 53 Ting-Wei Lan 2018-04-15 07:13:17 UTC
(In reply to Iñigo Martínez from comment #51)
> Nevermind.
> 
> Actually, it doesn't have any sense to prioritize one over the other because
> that is done in the code. It's enough to find if it's available in any the
> possible libraries.

Linking two libraries having the same function is almost always broken. It is the runtime linker that chooses the implementation for you. You don't know which version is picked in the code unless you use dlopen or setup things like LD_PRELOAD, LD_LIBRARY_PATH, DT_RUNPATH before running your program.
Comment 54 Ting-Wei Lan 2018-05-24 14:34:14 UTC
Meson 0.46 now has a new method called 'has_link_argument' which can be used to check linker arguments. However, it still doesn't work for '-Wl,--version-script'.
Comment 55 Iñigo Martínez 2018-05-25 15:00:41 UTC
Yes, and imho it should be the way to go. I still haven't tried it, but any issues should be fixed in meson.
Comment 56 Ondrej Holy 2018-07-17 14:02:43 UTC
The following MR should fix the linker checks:
https://gitlab.gnome.org/GNOME/gvfs/merge_requests/5
Comment 57 Ondrej Holy 2018-07-23 08:32:40 UTC
Comment on attachment 369881 [details] [review]
build: Fix version script linker flag check

This is obsolete since https://gitlab.gnome.org/GNOME/gvfs/merge_requests/5 was merged.