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 786149 - Port to meson build system
Port to meson build system
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: general
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gvfs-maint
gvfs-maint
: 794549 795053 (view as bug list)
Depends on: 789736 789877
Blocks: 782980
 
 
Reported: 2017-08-11 12:17 UTC by Iñigo Martínez
Modified: 2018-04-10 12:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Port to meson build system (61.71 KB, patch)
2017-08-11 12:17 UTC, Iñigo Martínez
none Details | Review
Remove autotools (91.10 KB, patch)
2017-08-11 12:19 UTC, Iñigo Martínez
none Details | Review
Port to meson build system (61.93 KB, patch)
2017-08-25 18:02 UTC, Iñigo Martínez
none Details | Review
Remove autotools (91.29 KB, patch)
2017-08-25 18:04 UTC, Iñigo Martínez
none Details | Review
Port to meson build system (67.59 KB, patch)
2017-09-14 14:51 UTC, Iñigo Martínez
none Details | Review
Remove autotools (91.88 KB, patch)
2017-09-14 14:53 UTC, Iñigo Martínez
none Details | Review
afp: Prevent usage of uninitialized variable (1.49 KB, patch)
2017-09-25 11:32 UTC, Ondrej Holy
committed Details | Review
dav: Prevent usage of uninitialized variable (1.13 KB, patch)
2017-09-25 11:32 UTC, Ondrej Holy
committed Details | Review
dav: Prevent usage of uninitialized variable (1.44 KB, patch)
2017-09-25 11:32 UTC, Ondrej Holy
committed Details | Review
metadata: Use static keyword for private function (1.04 KB, patch)
2017-09-25 11:32 UTC, Ondrej Holy
committed Details | Review
Port to meson build system (70.76 KB, patch)
2017-09-27 11:07 UTC, Iñigo Martínez
none Details | Review
Port to meson build system (70.89 KB, patch)
2017-09-29 10:08 UTC, Iñigo Martínez
none Details | Review
Port to meson build system (71.22 KB, patch)
2017-10-09 09:37 UTC, Iñigo Martínez
none Details | Review
Make use of shutil's which (3.78 KB, patch)
2017-10-10 09:48 UTC, Iñigo Martínez
committed Details | Review
Make sshd test optional (946 bytes, patch)
2017-10-10 09:49 UTC, Iñigo Martínez
committed Details | Review
Port to meson build system (72.73 KB, patch)
2017-10-10 09:55 UTC, Iñigo Martínez
none Details | Review
build: Fix ninja check (1.75 KB, patch)
2017-10-11 16:19 UTC, Ondrej Holy
none Details | Review
Port to meson build system (70.95 KB, patch)
2017-10-15 17:57 UTC, Iñigo Martínez
none Details | Review
Remove autotools (91.87 KB, patch)
2017-10-15 17:58 UTC, Iñigo Martínez
none Details | Review
Port to meson build system (71.19 KB, patch)
2017-10-21 16:23 UTC, Iñigo Martínez
none Details | Review
Remove autotools (100.03 KB, patch)
2017-10-23 05:42 UTC, Iñigo Martínez
none Details | Review
Port to meson build system (67.58 KB, patch)
2017-10-25 11:18 UTC, Iñigo Martínez
committed Details | Review
Remove autotools (100.03 KB, patch)
2017-10-25 11:30 UTC, Iñigo Martínez
none Details | Review
build: Remove configuration summary (3.29 KB, patch)
2017-10-26 10:37 UTC, Ondrej Holy
none Details | Review
build: Reorder meson options and update description (8.95 KB, patch)
2017-10-26 10:37 UTC, Ondrej Holy
none Details | Review
build: Do not generate the deprecated programs by default (1.54 KB, patch)
2017-10-26 10:52 UTC, Ondrej Holy
committed Details | Review
build: Reorder meson options and update description (8.94 KB, patch)
2017-10-26 11:02 UTC, Ondrej Holy
committed Details | Review
build: Do not build gdu volume monitor by default (1.46 KB, patch)
2017-10-26 11:03 UTC, Ondrej Holy
committed Details | Review
mtp: Remove redundant ifdefs (895 bytes, patch)
2017-10-26 11:16 UTC, Ondrej Holy
committed Details | Review
metadata: Remove udev dependency in favor of gudev (3.60 KB, patch)
2017-10-26 11:16 UTC, Ondrej Holy
none Details | Review
Remove unnecessary guards (2.79 KB, patch)
2017-10-28 09:22 UTC, Iñigo Martínez
committed Details | Review
Fix missing GDU monitor service file (919 bytes, patch)
2017-10-31 12:35 UTC, Iñigo Martínez
committed Details | Review
Improve installation on system paths (11.62 KB, patch)
2017-10-31 12:38 UTC, Iñigo Martínez
none Details | Review
build: Remove configuration summary (3.35 KB, patch)
2017-10-31 16:59 UTC, Ondrej Holy
committed Details | Review
build: Improve installation on system paths (11.62 KB, patch)
2017-10-31 17:02 UTC, Ondrej Holy
committed Details | Review
metadata: Remove udev dependency in favor of gudev (5.10 KB, patch)
2017-11-01 07:51 UTC, Ondrej Holy
none Details | Review
metadata: Remove udev dependency in favor of gudev (5.97 KB, patch)
2017-11-01 10:02 UTC, Ondrej Holy
committed Details | Review
Fix GIO_MODULE_DIR in gdu volume monitor (934 bytes, patch)
2017-11-06 18:57 UTC, Iñigo Martínez
none Details | Review
build: Make SystemdService entry conditional (12.15 KB, patch)
2017-11-06 18:59 UTC, Iñigo Martínez
none Details | Review
build: Add an option for gcrypt (3.45 KB, patch)
2017-11-06 19:00 UTC, Iñigo Martínez
committed Details | Review
build: Fix GIO_MODULE_DIR in GDU volume monitor (938 bytes, patch)
2017-11-06 19:03 UTC, Iñigo Martínez
committed Details | Review
build: Make SystemdService entry conditional (12.80 KB, patch)
2017-11-07 09:33 UTC, Iñigo Martínez
committed Details | Review
build: Rename build options (22.30 KB, patch)
2017-11-10 12:29 UTC, Iñigo Martínez
none Details | Review
build: Rename build options (14.66 KB, patch)
2017-11-10 15:37 UTC, Iñigo Martínez
committed Details | Review
build: Remove default warning level (887 bytes, patch)
2017-11-10 15:43 UTC, Iñigo Martínez
committed Details | Review
build: Use ITS rules for polkit in meson (1.30 KB, patch)
2017-11-11 10:59 UTC, Iñigo Martínez
committed Details | Review
build: Add dbus.service.in to distributable files (769 bytes, patch)
2017-11-13 12:33 UTC, Iñigo Martínez
committed Details | Review
build: Define HAVE_GCR by meson (811 bytes, patch)
2017-11-13 13:15 UTC, Ondrej Holy
committed Details | Review
build: Fix statfs check on FreeBSD (2.31 KB, patch)
2017-12-24 13:43 UTC, Ting-Wei Lan
none Details | Review
build: Fix statfs check on FreeBSD (2.03 KB, patch)
2017-12-24 18:08 UTC, Ting-Wei Lan
committed Details | Review
build: Fix build when gudev is disabled (992 bytes, patch)
2018-01-23 18:59 UTC, Ting-Wei Lan
none Details | Review
build: Fix UDisks2 build when gudev is disabled (873 bytes, patch)
2018-01-23 20:10 UTC, Iñigo Martínez
committed Details | Review
build: Fix build when gudev is disabled (1.10 KB, patch)
2018-01-24 10:43 UTC, Ting-Wei Lan
none Details | Review
build: Fix gvfsd-metadata build when gudev is disabled (1.12 KB, patch)
2018-01-24 10:53 UTC, Ting-Wei Lan
none Details | Review
build: Fix gdu monitor build when gudev is disabled (839 bytes, patch)
2018-01-24 10:56 UTC, Ting-Wei Lan
committed Details | Review
build: Fix gvfsd-metadata build when gudev is disabled (1.38 KB, patch)
2018-01-24 11:14 UTC, Ting-Wei Lan
committed Details | Review
build: Make SFTP backend optional (3.53 KB, patch)
2018-04-03 18:15 UTC, Iñigo Martínez
committed Details | Review
build: Remove autotools (99.83 KB, patch)
2018-04-05 19:53 UTC, Iñigo Martínez
accepted-commit_now Details | Review

Description Iñigo Martínez 2017-08-11 12:17:46 UTC
Created attachment 357405 [details] [review]
Port to meson build system

Complete port to meson build system.

Despite the fact I have been testing it, some more testing will be very appreciated. I have tested it on my computer which has only Linux installed.

Although I've also ported the "in tree" testing, I've ended up removing it as it didn't make any sense to me as meson builds it in a different directory. 

Any suggestion is also welcome,
Comment 1 Iñigo Martínez 2017-08-11 12:19:09 UTC
Created attachment 357406 [details] [review]
Remove autotools

Some GNOME applications and core libraries are removing autotools to avoid the burden of maintaining two build systems.

In case it is considered, this patch removes autotools.
Comment 2 Iñigo Martínez 2017-08-11 12:21:06 UTC
I have created a wip branch[0], which I think may help on testing the meson port.

[0] https://git.gnome.org/browse/gvfs/log/?h=wip/inigomartinez/meson
Comment 3 Ondrej Holy 2017-08-14 09:21:45 UTC
Thanks for the port. Let's wait with this to the next devel cycle. I will try to test and review it, although I don't have many experiences with meson... so feedback from others is welcome.
Comment 4 Iñigo Martínez 2017-08-25 12:55:18 UTC
Although I tried my best, there could be things to improve or features missing in the process, so please don't hesitate to tell me any suggestions or doubts you may have.
Comment 5 Ondrej Holy 2017-08-25 16:20:29 UTC
Review of attachment 357405 [details] [review]:

I've tried the patches quickly...

I've not seen the following warning with Autotools before, but that is probably some detail...
[25/270] Compiling C object 'metadata/metadata@sta/metabuilder.c.o'.
../../gvfs/metadata/metabuilder.c:348:1: warning: no previous prototype for ‘metafile_key_lookup_iter’ [-Wmissing-prototypes]
 metafile_key_lookup_iter (MetaFile *file,

The worse is that it fails on the following:
[156/270] Compiling C object 'daemon/gvfsd-sftp@exe/pty_open.c.o'.
FAILED: daemon/gvfsd-sftp@exe/pty_open.c.o 
cc  -Idaemon/gvfsd-sftp@exe -Idaemon -I../../gvfs/daemon -I. -I../../gvfs/ -Icommon -I../../gvfs/common -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/gio-unix-2.0/ -I/usr/include/gcr-3 -I/usr/include/gck-1 -I/usr/include/p11-kit-1 -I/usr/include/libsecret-1 -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -O2 -g -DHAVE_CONFIG_H -Wcast-align -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-sign-compare -Wno-strict-aliasing -DSECRET_WITH_UNSTABLE -pthread '-DLIBEXEC_DIR="/usr/local/libexec"' '-DMOUNTABLE_DIR="/usr/local/share/gvfs/mounts"' '-DGVFS_LOCALEDIR="/usr/local/share/locale"' -DMAX_JOB_THREADS=1 '-DSSH_PROGRAM="/usr/bin/ssh"' -DBACKEND_HEADER=gvfsbackendsftp.h -DDEFAULT_BACKEND_TYPE=sftp '-DBACKEND_TYPES="sftp", G_VFS_TYPE_BACKEND_SFTP,' -MMD -MQ 'daemon/gvfsd-sftp@exe/pty_open.c.o' -MF 'daemon/gvfsd-sftp@exe/pty_open.c.o.d' -o 'daemon/gvfsd-sftp@exe/pty_open.c.o' -c ../../gvfs/daemon/pty_open.c
../../gvfs/daemon/pty_open.c:61:10: fatal error: stropts.h: No such file or directory
 #include <stropts.h>
          ^~~~~~~~~~~
compilation terminated.
[161/270] Compiling C object 'daemon/gvfsd-sftp@exe/gvfsbackendsftp.c.o'.
ninja: build stopped: subcommand failed.

I see the following line, which is correct:
Has header "stropts.h": NO

But the code contains the following check, which always succeeds...:
#ifdef HAVE_STROPTS_H

...because the value is always set:
config_h.set10(header[0], cc.has_header(header[1]))

However if I change it from "set10" to "set", then it fails on another place, where just "#if" is used. So not sure if there is one recommended way, but I would suggest unifying the codes to use one of those approaches. This will affect Autotools, however I don't want to maintain two build systems anyways, so let's do not care about it. What do you think?
Comment 6 Iñigo Martínez 2017-08-25 18:02:04 UTC
Created attachment 358437 [details] [review]
Port to meson build system

The issue was related to the type of guard used (based on '#ifdef' or '#if').

I mixed both trying to get a more readable meson.build file, in which 'defines' and the 'headers' are defined in an array over which meson iterates. It worked for me as I had 'stropts.h' on my system.

It's fixed now and I have checked the locations where the 'guards' are used in order to set the proper define.

I've also updated the wip branch.
Comment 7 Iñigo Martínez 2017-08-25 18:04:46 UTC
Created attachment 358439 [details] [review]
Remove autotools

I forgot to say that I've rebased the patch with the latest changes in the code (~15) which also includes some modifications on the configure.ac file: version bump and a new guard for libgdata (HAVE_LIBGDATA_0_17_9).

The remove autotools patch has also been modified so here goes an update.
Comment 8 Iñigo Martínez 2017-09-14 14:51:45 UTC
Created attachment 359789 [details] [review]
Port to meson build system

A follow up update now that gnome 3.26 is released.

The main changes in this update are version bump to 1.34.1 version and and autotools build files listing also meson build files as distributable files.

Commit messages has also been updated with a more descriptive information.
Comment 9 Iñigo Martínez 2017-09-14 14:53:17 UTC
Created attachment 359791 [details] [review]
Remove autotools

A follow up update also for the remove autotools patch.

The wip branch[0] has also been updated to ease it's testing.

[0] https://git.gnome.org/browse/gvfs/log/?h=wip/inigomartinez/meson
Comment 10 Ondrej Holy 2017-09-25 11:31:10 UTC
Review of attachment 359789 [details] [review]:

I've made another round of testing. 

I see the following problem:
$ /opt/gnome/libexec/gvfsd
/opt/gnome/libexec/gvfsd: error while loading shared libraries: libgvfsdaemon.so: cannot open shared object file: No such file or directory

$ ldd /opt/gnome/libexec/gvfsd
	libgvfsdaemon.so => not found
	libgvfscommon.so => not found
...

Have you any idea why?

Found just some nitpicks:

::: daemon/meson.build
@@ +192,3 @@
+  ['network', 'network', [], 'NETWORK', 'network', [], [], network_flags, true, ['network']],
+  ['burn', 'burn', [], 'BURN', 'burn', [], [], burn_flags, true, ['burn']]
+]

The automatization sounds like a good idea, but the number of arguments makes it pretty unreadable... but not sure how to make it better.

@@ +221,3 @@
+  programs += [
+    ['smb', 'smb', [], 'SMB', 'smb-share', [], smb_deps, smb_flags, true, ['smb']],
+    ['smb-browse', 'smbbrowse', smb_browse_sources, 'SMB_BROWSE', 'smb-network', ['smb-server'],  smb_deps, smb_flags, true, ['smb-browse']]

double spaces

::: man/meson.build
@@ +18,3 @@
+  ['gvfs-cat', 'gio cat', 1],
+  ['gvfs-copy', 'gio copy', 1],
+	['gvfs-info', 'gio info', 1],

wrong alignment

::: test/meson.build
@@ +82,3 @@
+# run tests against build tree
+check: $(CONFIG_FILES) gvfs-test
+	$(srcdir)/run-in-tree.sh $(srcdir)/gvfs-test $(TEST_NAMES)

possibly wrong alignment

@@ +87,3 @@
+# when running as root, use gvfs-testbed to enable all tests
+installcheck-local: gvfs-test
+	if [ `id -u` = 0 ]; then \

possibly wrong alignment
Comment 11 Ondrej Holy 2017-09-25 11:32:35 UTC
Created attachment 360341 [details] [review]
afp: Prevent usage of uninitialized variable

More warnings are printed when building thanks to meson port. The code
contains goto command which skips initialization of server_name variable.
Let's move the initialization to another place in order to prevent usage
of uninitialized variable and the following warning:
warning: ‘server_name’ may be used uninitialized in this function
Comment 12 Ondrej Holy 2017-09-25 11:32:41 UTC
Created attachment 360342 [details] [review]
dav: Prevent usage of uninitialized variable

More warnings are printed when building thanks to meson	port. The bytes_avail
variable is guarded by have_bytes_avail, so it should not be used unitialized,
but let's initialize it for sure in order to prevent the following warning:
warning: ‘bytes_avail’ may be used uninitialized in this function
Comment 13 Ondrej Holy 2017-09-25 11:32:46 UTC
Created attachment 360343 [details] [review]
dav: Prevent usage of uninitialized variable

More warnings are printed when building thanks to meson port. The mounting
loop may be breaked before is_webdav variable is defined. Let's move the
initialization to another place in order to prevent usage of uninitialized
variable and the following warning:
warning: ‘is_webdav’ may be used uninitialized in this function
Comment 14 Ondrej Holy 2017-09-25 11:32:52 UTC
Created attachment 360344 [details] [review]
metadata: Use static keyword for private function

More warnings are printed when building thanks to meson port. Let's add
static keyword for private metafile_key_lookup_iter function in order
to prevent the following warning:
warning: no previous prototype for ‘metafile_key_lookup_iter’
Comment 15 Ondrej Holy 2017-09-25 11:34:07 UTC
I've attached fixes for the warnings which start appearing after meson port...
Comment 16 Iñigo Martínez 2017-09-25 13:37:27 UTC
Thank you for testing it, your comments are very appreciated.

Some answers below:

(In reply to Ondrej Holy from comment #10)
> Review of attachment 359789 [details] [review] [review]:
> 
> I've made another round of testing. 
> 
> I see the following problem:
> $ /opt/gnome/libexec/gvfsd
> /opt/gnome/libexec/gvfsd: error while loading shared libraries:
> libgvfsdaemon.so: cannot open shared object file: No such file or directory
> 
> $ ldd /opt/gnome/libexec/gvfsd
> 	libgvfsdaemon.so => not found
> 	libgvfscommon.so => not found
> ...
> 
> Have you any idea why?

Although since meson's 0.42 version there is an option to add a `rpath`, this only applies to the build tree. Once installed it is removed, so the dynamic loader is not able to find the shared libraries. I usually set `LD_LIBRARY_PATH` to the `libdir` value to solve this issue.
 
> Found just some nitpicks:
> 
> ::: daemon/meson.build
> @@ +192,3 @@
> +  ['network', 'network', [], 'NETWORK', 'network', [], [], network_flags,
> true, ['network']],
> +  ['burn', 'burn', [], 'BURN', 'burn', [], [], burn_flags, true, ['burn']]
> +]
> 
> The automatization sounds like a good idea, but the number of arguments
> makes it pretty unreadable... but not sure how to make it better.

I've been abusing meson arrays in different meson ports. They usually help group similar cases together, making the code more readable, but I agree that, in this case, it doesn't help.

Probably, the best option would be to expand each daemon's parameters. I'll fix it asap.

> @@ +221,3 @@
> +  programs += [
> +    ['smb', 'smb', [], 'SMB', 'smb-share', [], smb_deps, smb_flags, true,
> ['smb']],
> +    ['smb-browse', 'smbbrowse', smb_browse_sources, 'SMB_BROWSE',
> 'smb-network', ['smb-server'],  smb_deps, smb_flags, true, ['smb-browse']]
> 
> double spaces
>
> ::: man/meson.build
> @@ +18,3 @@
> +  ['gvfs-cat', 'gio cat', 1],
> +  ['gvfs-copy', 'gio copy', 1],
> +	['gvfs-info', 'gio info', 1],
> 
> wrong alignment
> 
> ::: test/meson.build
> @@ +82,3 @@
> +# run tests against build tree
> +check: $(CONFIG_FILES) gvfs-test
> +	$(srcdir)/run-in-tree.sh $(srcdir)/gvfs-test $(TEST_NAMES)
> 
> possibly wrong alignment
> 
> @@ +87,3 @@
> +# when running as root, use gvfs-testbed to enable all tests
> +installcheck-local: gvfs-test
> +	if [ `id -u` = 0 ]; then \
> 
> possibly wrong alignment

I missed these all. They are fixed now.

[0] http://mesonbuild.com/Release-notes-for-0-42-0.html#added-build_rpath-keyword-argument
Comment 17 Iñigo Martínez 2017-09-27 11:07:52 UTC
Created attachment 360522 [details] [review]
Port to meson build system

I've reviewed the patch one more time, and in addition to the issue with the array I have done some more changes.

To solve the issue with the array, instead of completely removing it, I have opted for an intermediate solution by cutting the parameters to only 4 (program name, sources, libraries and cflags). Although there are some visible patterns regarding the used cflags, I've expanded them. I hope it's in a better shape now.

The array at man files has also been slightly modified because `gvfs` was the only man page using the section 7.

I've also fixed some options which involved `socketpair`, `libgdata`, `libgphoto2` and `libmtp`.

Finally I've update the wip branch[0], which also includes your patches.

https://git.gnome.org/browse/gvfs/log/?h=wip/inigomartinez/meson
Comment 18 Ondrej Holy 2017-09-29 08:53:06 UTC
(In reply to Iñigo Martínez from comment #16)
> Thank you for testing it, your comments are very appreciated.
> 
> Some answers below:
> 
> (In reply to Ondrej Holy from comment #10)
> > Review of attachment 359789 [details] [review] [review] [review]:
> > 
> > I've made another round of testing. 
> > 
> > I see the following problem:
> > $ /opt/gnome/libexec/gvfsd
> > /opt/gnome/libexec/gvfsd: error while loading shared libraries:
> > libgvfsdaemon.so: cannot open shared object file: No such file or directory
> > 
> > $ ldd /opt/gnome/libexec/gvfsd
> > 	libgvfsdaemon.so => not found
> > 	libgvfscommon.so => not found
> > ...
> > 
> > Have you any idea why?
> 
> Although since meson's 0.42 version there is an option to add a `rpath`,
> this only applies to the build tree. Once installed it is removed, so the
> dynamic loader is not able to find the shared libraries. I usually set
> `LD_LIBRARY_PATH` to the `libdir` value to solve this issue.

But jhbuild already set LD_LIBRARY_PATH=/opt/gnome/lib which doesn't work, shouldn't this be enough? It works with LD_LIBRARY_PATH=/opt/gnome/lib/gvfs though, don't you forget to specify that subfolder somewhere?
Comment 19 Iñigo Martínez 2017-09-29 10:08:21 UTC
Created attachment 360652 [details] [review]
Port to meson build system

(In reply to Ondrej Holy from comment #18)
> (In reply to Iñigo Martínez from comment #16)
> > Thank you for testing it, your comments are very appreciated.
> > 
> > Some answers below:
> > 
> > (In reply to Ondrej Holy from comment #10)
> > > Review of attachment 359789 [details] [review] [review] [review] [review]:
> > > 
> > > I've made another round of testing. 
> > > 
> > > I see the following problem:
> > > $ /opt/gnome/libexec/gvfsd
> > > /opt/gnome/libexec/gvfsd: error while loading shared libraries:
> > > libgvfsdaemon.so: cannot open shared object file: No such file or directory
> > > 
> > > $ ldd /opt/gnome/libexec/gvfsd
> > > 	libgvfsdaemon.so => not found
> > > 	libgvfscommon.so => not found
> > > ...
> > > 
> > > Have you any idea why?
> > 
> > Although since meson's 0.42 version there is an option to add a `rpath`,
> > this only applies to the build tree. Once installed it is removed, so the
> > dynamic loader is not able to find the shared libraries. I usually set
> > `LD_LIBRARY_PATH` to the `libdir` value to solve this issue.
> 
> But jhbuild already set LD_LIBRARY_PATH=/opt/gnome/lib which doesn't work,
> shouldn't this be enough? It works with LD_LIBRARY_PATH=/opt/gnome/lib/gvfs
> though, don't you forget to specify that subfolder somewhere?

You were totally right! I was just doing a workaround and actually there is a way to do it properly.

There is an argument called `install_rpath`[0] to set the `rpath` at install time, which in fact is needed for `gvfs` binaries to find the shared libraries. I'm sorry I didn't fix it sooner.

This updated patch adds the `RUNPATH` header to those executables which use those shared libraries, so the dynamic loader is now able to find them.

[0] http://mesonbuild.com/Reference-manual.html#executable
Comment 20 Ondrej Holy 2017-09-29 11:27:15 UTC
Let's push the fixes before gnome-3-26 bracnh...

Attachment 360341 [details] pushed as 7c0f22c - afp: Prevent usage of uninitialized variable
Attachment 360342 [details] pushed as bf6bd1d - dav: Prevent usage of uninitialized variable
Attachment 360343 [details] pushed as bf6bd1d - dav: Prevent usage of uninitialized variable
Attachment 360344 [details] pushed as e417487 - metadata: Use static keyword for private function
Comment 21 Ondrej Holy 2017-10-06 11:43:16 UTC
Review of attachment 360652 [details] [review]:

Thanks, looks much better now, so I did another test iteration and gvfsd works somehow:
$ ldd /opt/gnome/libexec/gvfsd | grep gvfs
	libgvfsdaemon.so => /opt/gnome/lib/gvfs/libgvfsdaemon.so (0x00007f00d4c84000)
	libgvfscommon.so => /opt/gnome/lib/gvfs/libgvfscommon.so (0x00007f00d4a48000)
but shouldn't be /usr/lib64/gvfs/*.so used if /opt/gnome/lib is not in LD_LIBRARY_PATH?

Also e.g. "jhbuild run gio mount localtest:///"  still doesn't work with meson (without LD_LIBRARY_PATH=/opt/gnome/lib/gvfs):
libgvfscommon.so: cannot open shared object file: No such file or directory
Failed to load module: /opt/gnome/lib/gio/modules/libgvfsdbus.so
libgvfscommon.so: cannot open shared object file: No such file or directory
Failed to load module: /opt/gnome/lib/gio/modules/libgioremote-volume-monitor.so
gio: localtest:///: volume doesn’t implement mount

So you should probably use rpath also for those...

::: meson_options.txt
@@ +24,3 @@
+option('enable-libmtp', type: 'combo', choices: ['yes', 'no', 'auto'], value: 'auto', description: 'build with libmtp support')
+option('enable-afp', type: 'boolean', value: true, description: 'build with AFP support')
+option('enable-man', type: 'boolean', value: false, description: 'generate man pages')

It is probably good idea to make man pages and tests false by default, but it would be nice to print this in "gvfs configuration summary" to be obvious it is disabled...

::: test/meson.build
@@ +83,3 @@
+# run tests against build tree
+check: $(CONFIG_FILES) gvfs-test
+  $(srcdir)/run-in-tree.sh $(srcdir)/gvfs-test $(TEST_NAMES)

How does this work?

$ ninja check
ninja: error: unknown target 'check', did you mean 'clean'?
Comment 22 Iñigo Martínez 2017-10-09 09:37:18 UTC
Created attachment 361167 [details] [review]
Port to meson build system

(In reply to Ondrej Holy from comment #21)
> Review of attachment 360652 [details] [review] [review]:
> 
> Thanks, looks much better now, so I did another test iteration and gvfsd
> works somehow:
> $ ldd /opt/gnome/libexec/gvfsd | grep gvfs
> 	libgvfsdaemon.so => /opt/gnome/lib/gvfs/libgvfsdaemon.so
> (0x00007f00d4c84000)
> 	libgvfscommon.so => /opt/gnome/lib/gvfs/libgvfscommon.so
> (0x00007f00d4a48000)
> but shouldn't be /usr/lib64/gvfs/*.so used if /opt/gnome/lib is not in
> LD_LIBRARY_PATH?

TL;DR I've never knew how `ld.so.conf` worked in depth and this was a good opportunity to investigate it, however despite the fact that I've been looking at it to be able to answer you properly, I don't have a definitive answer.

I would say that `LD_LIBRARY_PATH` is not necessary as binaries already have `DT_RUNPATH` attribute set with the absolute path to the libraries, and it won't look at `/usr/lib64/gvfs/*` because `DT_RUNPATH` has an absolute path and it's not in the default path.

Looking at `ld.conf.so`'s manual[0], it uses the following order: 

- It looks at DT_RPATH attribute, in case it exists and DT_UNPATH doesn't exists.
- It uses the LD_LIBRARY_PATH environment variable (unless run in secure-execution mode).
- It looks at the DT_RUNPATH attribute.
- It looks at the `ld.so.cache` cache file.
- Finally it looks at default path which is `/lib` and then `/usr/lib` (or `/lib64` and `/usr/lib64` on some 64-bit archs).

In your case, `libgvfsdaemon.so` and `libgvfscommon.so` are not installed in the system's default paths. And even if they were, they'd be installed in a subdirectory, `gvfs`.

My first solution used the absolute path where they were installed. Although it works, and `LD_LIBRARY_PATH` is not needed, it won't look for them in the `default path`, as they are inside a subdirectory, `gvfs`.

`ld.conf.so` also allows the use of token strings. There are three token strings:

- $ORIGIN: which expands to the directory containing the program/shared object.
- $LIB: which expands to `lib` or `lib64` depending on the architecture.
- $PLATFORM: which expands to the processor type (e.g. `x86_64`).

It may have sense to use something like `$LIB/../gvfs`, AFAIK it won't help in the final distro directory tree, which in my system would be as follows:

- `/usr/lib/gvfs/` for `libexec` executables.
- `/usr/lib/x86_64-linux-gnu/gvfs/` for shared libraries.

I haven't been able to either make these tokes to work with meson.
 
Finally, I've looking at my distro's installation and the executables at `libexec` use the `DT_RUNPATH` with an absolute path.

> Also e.g. "jhbuild run gio mount localtest:///"  still doesn't work with
> meson (without LD_LIBRARY_PATH=/opt/gnome/lib/gvfs):
> libgvfscommon.so: cannot open shared object file: No such file or directory
> Failed to load module: /opt/gnome/lib/gio/modules/libgvfsdbus.so
> libgvfscommon.so: cannot open shared object file: No such file or directory
> Failed to load module:
> /opt/gnome/lib/gio/modules/libgioremote-volume-monitor.so
> gio: localtest:///: volume doesn’t implement mount
> 
> So you should probably use rpath also for those...

This is totally my fault. I only looked at executables at the `daemon` directory and I missed to look at executables which are built in other directories but also depend on those shared libraries. It's fixed now.
 
> ::: meson_options.txt
> @@ +24,3 @@
> +option('enable-libmtp', type: 'combo', choices: ['yes', 'no', 'auto'],
> value: 'auto', description: 'build with libmtp support')
> +option('enable-afp', type: 'boolean', value: true, description: 'build with
> AFP support')
> +option('enable-man', type: 'boolean', value: false, description: 'generate
> man pages')
> 
> It is probably good idea to make man pages and tests false by default, but
> it would be nice to print this in "gvfs configuration summary" to be obvious
> it is disabled...

I have changed the default option value now. I've also expanded meson's output after configuration step so it also shows this option.

> ::: test/meson.build
> @@ +83,3 @@
> +# run tests against build tree
> +check: $(CONFIG_FILES) gvfs-test
> +  $(srcdir)/run-in-tree.sh $(srcdir)/gvfs-test $(TEST_NAMES)
> 
> How does this work?

This is one of the last things that remained to be solved. There are two targets in `tests/Makefile.am` related to test `gvfs`: `check` and `installcheck-local`.

I wasn't sure if these were actually in use. I've came across targets in other meson ports which are not in use anymore and I wasn't sure if this was the case here, so I let them commented in the meson port in case something should be made with them. I haven't investigated in depth them what they do. I may try to also port them if they are necessary, though looking at `check` comment and with also a quick look at `run-in-tree.sh`, seems to use binaries that are located in the built tree, which in autotools is shared with the source tree, and libraries created with `libtool`, which isn't used in meson. 

> $ ninja check
> ninja: error: unknown target 'check', did you mean 'clean'?

Once targets are in place, the command would be like that, or something like this if you also use a custom build dir: ninja -C _build check

[0] http://man7.org/linux/man-pages/man8/ld.so.8.html
Comment 23 Ondrej Holy 2017-10-09 11:37:45 UTC
Sorry, the answer (on "shouldn't be /usr/lib64/gvfs/*.so used if /opt/gnome/lib is not in LD_LIBRARY_PATH?") is probably that it shouldn't behave the way I asked, I've tried the autotools build and it behaves the same... So it is ok as it is...

Thanks for update, but still...

$ jhbuild run gio mount -l
libgvfscommon.so: cannot open shared object file: No such file or directory
Failed to load module: /opt/gnome/lib/gio/modules/libgvfsdbus.so

I am used to use "make check" from build dir... I have never used installcheck-local, or the utils e.g. benchmark*... so it would be nice to have "ninja check" supported as well if possible... (I don't understand why "ninja check" doesn't work currently, when "check" is defined in meson.build).
Comment 24 Iñigo Martínez 2017-10-10 09:48:06 UTC
Created attachment 361222 [details] [review]
Make use of shutil's which

In order to get proper `check` support, I've been looking at the `gvfs-test` script and I ended doing a minor update. Instead of creating a subprocess which calls to `which`, since Python 3.3, `shutil` supports `which` function which returns the binary path and also helps checking if the binary exists.
Comment 25 Iñigo Martínez 2017-10-10 09:49:09 UTC
Created attachment 361223 [details] [review]
Make sshd test optional

I don't have an `SSH` server installed in my computer, which makes the `gvfs-test` to fail. This patch makes this optiona in case it misses the `sshd` binary.
Comment 26 Iñigo Martínez 2017-10-10 09:55:42 UTC
Created attachment 361231 [details] [review]
Port to meson build system

An update to the previous meson port patch which fixes the problem with `libgvfsdbus.so` not able to load `libgvfscommon.so` and also creates a new `check` target which executes the `gvfs-test` test command.

I haven't been able to find a proper way to share the same script program `run-in-tree.sh` between autotools and meson, due to some variables using tree paths, so a new script has been created in a similar manner.

The command to be executed is as follows:

  ninja -C builddir check

The target depends on the executables tested on the test command.
Comment 27 Ondrej Holy 2017-10-10 15:45:27 UTC
Review of attachment 361222 [details] [review]:

Thanks, why not...
Comment 28 Ondrej Holy 2017-10-10 15:45:38 UTC
Review of attachment 361223 [details] [review]:

Thanks, looks ok.
Comment 29 Iñigo Martínez 2017-10-10 20:13:47 UTC
Comment on attachment 361222 [details] [review]
Make use of shutil's which

Pushed as b460e265 - test: Make use of shutil's which
Comment 30 Iñigo Martínez 2017-10-10 20:14:58 UTC
Comment on attachment 361223 [details] [review]
Make sshd test optional

Pushed as cffb8e8b - test: Make sshd test optional
Comment 31 Ondrej Holy 2017-10-11 16:19:38 UTC
Created attachment 361338 [details] [review]
build: Fix ninja check

Some tests fail with ninja check and are slow, because gvfs-test doesn't
see session.conf. It is because gvfs-test is in srcdir and session.conf
in buildir, so it starts dbus with default options, which causes some
troubles due to keyring...

Remove also redundant dbus-daemon call from check.sh.in.
Comment 32 Iñigo Martínez 2017-10-11 17:06:53 UTC
(In reply to Ondrej Holy from comment #31)
> Created attachment 361338 [details] [review] [review]
> build: Fix ninja check
> 
> Some tests fail with ninja check and are slow, because gvfs-test doesn't
> see session.conf. It is because gvfs-test is in srcdir and session.conf
> in buildir, so it starts dbus with default options, which causes some
> troubles due to keyring...
> 
> Remove also redundant dbus-daemon call from check.sh.in.

Thanks. It looks fine for me.

I'm sorry for not providing a good enough solution. I wasn't aware of what a successful test would look like.

Just as a side note, `test`[0] function also has a `workdir` parameter, that would help executing the command in the `builddir/test` directory which holds the `session.conf` file, avoiding the need to modify the `gvfs-test` script.

[0] http://mesonbuild.com/Reference-manual.html#test
Comment 33 Ondrej Holy 2017-10-12 09:49:33 UTC
Review of attachment 361231 [details] [review]:

Unfortunately, I have found some other issues (mostly just nitpicks)...

Please be aware of Bug 788707.

Is empty po/meson.build needed?

::: client/meson.build
@@ +9,3 @@
+)
+
+## Dynamic client lib

Please unify the comments, I think that # enough on all places probably...

::: client/symbol.map
@@ +6,3 @@
+  g_io_module_query;
+local:
+	*;

tabs vs spaces

::: common/meson.build
@@ +22,3 @@
+)
+
+cflags = common_cflags + ['-DREMOTE_VOLUME_MONITORS_DIR="@0@"'.format(gvfs_datadir, 'gvfs', 'remote-volume-monitors')]

join_paths? gvfs_remote_volume_monitors_dir?

::: daemon/meson.build
@@ +256,3 @@
+mounts += ['burn']
+
+if have_openpty

openpty should not be hard requirement for sftp

@@ +532,3 @@
+endif
+
+if have_afp

have_afp is not properly defined in /meson.build

::: meson.build
@@ +572,3 @@
+if have_afp
+  msg_afp = (have_gcrypt ? 'true' : 'partial (crypt support missing! Only anonymous logins)')
+endif

set have_afp properly

@@ +641,3 @@
+output += '        archive support:              ' + have_archive.to_string() + '\n'
+output += '        AFC support:                  ' + have_afc.to_string() + '\n'
+output += '        AFP support:                  ' + msg_afp + '\n'

have_afp

::: metadata/meson.build
@@ +60,3 @@
+  'meta-get',
+  'meta-set',
+  'meta-get-tree'

Just a note that we have various utils in the tree which are not installed... would be nice to have s way to not build them by default, but this is not relevant to this bug.

::: monitor/goa/meson.build
@@ +1,2 @@
+install_data(
+  'goa.monitor',

goa_monitor = files('goa.monitor')?

::: monitor/mtp/meson.build
@@ +4,3 @@
+)
+
+service = 'org.gtk.vfs.MTPVolumeMonitor.service'

mtp_service = gvfs_namespace + '.MTPVolumeMonitor.service' (applies on other places)

::: monitor/proxy/meson.build
@@ +19,3 @@
+
+cflags = [
+  '-DG_LOG_DOMAIN="GVFS-RemoveVolumeMonitor"',

GVFS-RemoteVolumeMonitor

I've also realized that glib doesn't see our monitors, which is hopefully just a consequence of this.

::: monitor/proxy/symbol.map
@@ +5,3 @@
+  g_io_module_query;
+local:
+	*;

tabs vs spaces

::: monitor/udisks2/meson.build
@@ +55,3 @@
+cflags = [
+  '-DG_LOG_DOMAIN="GVFS-UDisks2"',
+  '-DG_DISABLE_DEPRECATED',

Hmm, this is probably unwanted... but is also in Makefile.am.

::: test/check.sh.in
@@ +9,3 @@
+
+PIDFILE=`mktemp`
+export DBUS_SESSION_BUS_ADDRESS=`dbus-daemon --config-file=@dbus_conf@ --fork --print-address=1 --print-pid=3 3>${PIDFILE}`

This is not wanted since the test suite will run its own daemon, but let's remove this completely...

::: test/meson.build
@@ +61,3 @@
+)
+
+test_units = [

These are not united test, so maybe better just "tests = ["...

@@ +78,3 @@
+
+  # FIXME: tests need input parameters
+  #test(unit, exe)

...and remove this

@@ +98,3 @@
+
+run_target(
+  'check',

Please remove the custom check command, since it is fragile and proper test integration would require a lot of work... let's rely on gnome-desktop-testing-runner only.
Comment 34 Iñigo Martínez 2017-10-15 17:57:01 UTC
Created attachment 361623 [details] [review]
Port to meson build system

Thank you very much for checking the patch thoroughly. I have applied almost all the changes.

I have included some comments below.

(In reply to Ondrej Holy from comment #33)
> Review of attachment 361231 [details] [review] [review]:
> 
> @@ +532,3 @@
> +endif
> +
> +if have_afp
> 
> have_afp is not properly defined in /meson.build

`enable-afp` is the only option without a hard dependency againts any library. Although `AFP` support is preferrably built, it won't block the build in case its only dependency `gcrypt` isn't meet. I consider an `auto` option, a way to unlock the build in case of a "preferred" option not being able to compile if a dependency isn't meet. That's why I defined it as a boolean option. Still, I changed it for the sake of consistency.

> ::: meson.build
> @@ +572,3 @@
> +if have_afp
> +  msg_afp = (have_gcrypt ? 'true' : 'partial (crypt support missing! Only
> anonymous logins)')
> +endif
> 
> set have_afp properly
> 
> @@ +641,3 @@
> +output += '        archive support:              ' +
> have_archive.to_string() + '\n'
> +output += '        AFC support:                  ' + have_afc.to_string() +
> '\n'
> +output += '        AFP support:                  ' + msg_afp + '\n'
> 
> have_afp

The summary message is not based on a boolean option, it must also show if `gcrypt` is available. That's why 'msg_afp' string is used instead of 'have_afp' boolean.

> ::: metadata/meson.build
> @@ +60,3 @@
> +  'meta-get',
> +  'meta-set',
> +  'meta-get-tree'
> 
> Just a note that we have various utils in the tree which are not
> installed... would be nice to have s way to not build them by default, but
> this is not relevant to this bug.

This sounds very sensible for me. I have added a new option called `enable-utils` to disable their build.

> ::: monitor/goa/meson.build
> @@ +1,2 @@
> +install_data(
> +  'goa.monitor',
> 
> goa_monitor = files('goa.monitor')?

This would be nice if `goa.monitor` were to be used on multiple places, so it's not necessary. For example this is done for `gphoto2.monitor` because it's also used in `test/meson.build` file.

> ::: monitor/mtp/meson.build
> @@ +4,3 @@
> +)
> +
> +service = 'org.gtk.vfs.MTPVolumeMonitor.service'
> 
> mtp_service = gvfs_namespace + '.MTPVolumeMonitor.service' (applies on other
> places)

That's a nice suggestion. Sometimes you have to find a compromise between tokenizing too much, however this fits perfectly here.

> ...and remove this
> 
> @@ +98,3 @@
> +
> +run_target(
> +  'check',
> 
> Please remove the custom check command, since it is fragile and proper test
> integration would require a lot of work... let's rely on
> gnome-desktop-testing-runner only.

I have also included this, which removes the test integration. This means that `build: Fix ninja check` is not necessary anymore.

I have been reviewing the patch several times, But I hope I didn't miss anything. I have updated the wip-branch with this patches.

[0] https://bugzilla.gnome.org/attachment.cgi?id=361338
Comment 35 Iñigo Martínez 2017-10-15 17:58:10 UTC
Created attachment 361624 [details] [review]
Remove autotools

A folow up update.
Comment 36 Iñigo Martínez 2017-10-17 14:19:30 UTC
Regarding "Remove autotools" patch[0], if this option is considered, there are steps before/after to be done.

One of the steps is related to `gnome-continuous`[1]. Actually the `configure` script is not strictly necessary, as `gnome-continuous` may patch it on the fly[2]. It's up to the maintainer to include it.

Another step is related to `jhbuild`. It must be updated to set the build system to meson. This also implies a release with meson build system merged.

There is more information in the GNOME wiki[3].

I know how to proceed with these changes, so I can take responsibility for making the appropriate changes, but I have to know when the patch is going to be merged.

[0] https://bugzilla.gnome.org/attachment.cgi?id=361624
[1] https://mail.gnome.org/archives/desktop-devel-list/2017-April/msg00080.html
[2] https://git.gnome.org/browse/gnome-continuous/tree/patches
[3] [2] https://wiki.gnome.org/Initiatives/GnomeGoals/MesonPorting
Comment 37 Ondrej Holy 2017-10-20 10:05:29 UTC
Review of attachment 361623 [details] [review]:

Sorry that I've not found time for review earlier...

::: client/meson.build
@@ +61,3 @@
+)
+
+test_name = 'test-uri-utils'

The source codes contain more utils which are not installed and shouldn't be built by default...
if enable_utils

::: daemon/meson.build
@@ +140,3 @@
+)
+
+sources = daemon_main_sources + files('gvfsbackendtest.c')

if enable_utils

::: meson.build
@@ +607,3 @@
+# installed tests
+enable_installed_tests = get_option('enable-installed-tests')
+if enable_installed_tests

I would remove this if, see my comments for test/meson.build.

::: meson_options.txt
@@ +25,3 @@
+option('enable-afp', type: 'combo', choices: ['yes', 'no', 'auto'], value: 'auto', description: 'build with AFP support')
+option('enable-man', type: 'boolean', value: false, description: 'generate man pages')
+option('enable-utils', type: 'boolean', value: false, description: 'generate utility programs')

It should be obvious that this is nothing for regular people. Something like enable-devel-utils, or enable-test-utils? And update the description accordingly...

::: metadata/meson.build
@@ +81,3 @@
+if have_libxml
+  executable(
+    'convert-nautilus-metadata',

if enable_utils

::: test/meson.build
@@ +18,3 @@
+all_tests_conf.set('installed_testdir', installed_testdir)
+
+all_tests = 'gvfs-all-tests'

if enable_installed_tests

@@ +61,3 @@
+)
+
+tests = [

if enable_utils
Comment 38 Iñigo Martínez 2017-10-21 16:23:53 UTC
Created attachment 362019 [details] [review]
Port to meson build system

Ok, changes done. I've also included a very minor improvement.

Thank you for your support :)
Comment 39 Iñigo Martínez 2017-10-23 05:42:08 UTC
Created attachment 362067 [details] [review]
Remove autotools

An update where it also removes almost all the .gitignore files because they were holding autotools generated files. There are not necessary anymore if autootols is removed, so they have also been removed.

The only .gitignore that still remains is the one in the `po` directory because meson generates some files, i.e. `pot` file, there.
Comment 40 Iñigo Martínez 2017-10-23 13:32:07 UTC
Recently, when working in `glib-networking` meson build port, which also used `auto` options, Michael ended removing them[0].

I had also written about them some months ago[1], when I was porting `totem`.

In short, I am not in sympathy with those options.  I'm raising the question here because now might be a good time to reconsider and change them.

[0] https://bugzilla.gnome.org/show_bug.cgi?id=786639#c25
[1] https://bugzilla.gnome.org/show_bug.cgi?id=783205#c19
Comment 41 Ondrej Holy 2017-10-24 12:38:40 UTC
Thanks for updates, looks good and agree with the removal of auto options, so let's remove them together with meson port and will see what happens :-D

However, the removal of "auto" options cause that some of the options will be probably a bit in conflict, see my comment in Bug 788707.
Comment 42 Iñigo Martínez 2017-10-25 11:18:06 UTC
Created attachment 362249 [details] [review]
Port to meson build system

I have proceed to remove the `auto` options, so the code should look cleaner now, and some assumptions can be made without checking if an option is `required` or not.

I have also removed some guards as in the beginning I replicated all the guards present in `autotools` but some were actually not necessary.

Finally I have rebased master and changed version number, and I've also fixed the license (yaww...).

The code in the wip branch is also updated.

Please, let me know if everything is in place and working as it should.
Comment 43 Iñigo Martínez 2017-10-25 11:30:25 UTC
Created attachment 362251 [details] [review]
Remove autotools

A follow up update.
Comment 44 Iñigo Martínez 2017-10-25 11:43:05 UTC
One last comment regarding guards, there are a lot of defines in the `config.h` file. Most of them are created by autotools but are not actually needed. `PACKAGE` defines, std headers defines, functions defines.

I haven't removed them because I'm not sure if they are necessary in non-linux systems (bsd?), or even on older linux systems. They are at least not necessary on my system.

It would be nice to remove those that aren't necessary, though this can be done later.
Comment 45 Ondrej Holy 2017-10-26 10:04:12 UTC
Yes, it would be nice to remove as much as possible, however, I don't know also whether they are needed, or not (e.g. for BSD systems). What happened with this in other projects when porting to meson (e.g. glib)? I am afraid of that only way how to find the answer is remove this stuff and wait for bug reports... so maybe now is the right time, but would be good to do it in a separate patch...
Comment 46 Ondrej Holy 2017-10-26 10:36:52 UTC
Review of attachment 362249 [details] [review]:

Thanks, look ok, just...

::: meson_options.txt
@@ +8,3 @@
+option('enable-gudev', type: 'boolean', value: true, description: 'build with gudev support')
+option('enable-fuse', type: 'boolean', value: true, description: 'build with FUSE support')
+option('enable-gdu', type: 'boolean', value: true, description: 'build with GDU volume monitor')

This needs to be definitely false by default because the monitor is superseded by udisks2 volume monitor and I am about removing it soon.
Comment 47 Ondrej Holy 2017-10-26 10:37:36 UTC
Created attachment 362326 [details] [review]
build: Remove configuration summary

Meson port introduced boolean type for options and eliminated auto
options, so the final configuration is currently deterministically
based on given options and there is no need for the summary.
Comment 48 Ondrej Holy 2017-10-26 10:37:43 UTC
Created attachment 362327 [details] [review]
build: Reorder meson options and update description

meson_options.txt is hard to read. Let's create sorted groups of options
and unify the description. Also change some option names to match
correspoding backend names.
Comment 49 Ondrej Holy 2017-10-26 10:52:15 UTC
Created attachment 362328 [details] [review]
build: Do not generate the deprecated programs by default

Commandline utils were superseded by gio tool and shell scripts with
warning are generated instead. Let's do not generate them by default
in order to speed up the build.
Comment 50 Ondrej Holy 2017-10-26 11:02:38 UTC
Created attachment 362330 [details] [review]
build: Reorder meson options and update description
Comment 51 Ondrej Holy 2017-10-26 11:03:26 UTC
Created attachment 362331 [details] [review]
build: Do not build gdu volume monitor by default

The gdu volume monitor shouldn't be build by default. It is superseded
by udisks2 volume monitor and will be removed soon.
Comment 52 Ondrej Holy 2017-10-26 11:05:22 UTC
Review of attachment 362249 [details] [review]:

I am tenatively ok to push this with the follow-up patches and patches from Bug 788707...
Comment 53 Ondrej Holy 2017-10-26 11:07:22 UTC
Review of attachment 362251 [details] [review]:

This is also ok to push, but I would wait until tasks from Comment 36 are finished...
Comment 54 Ondrej Holy 2017-10-26 11:16:27 UTC
Created attachment 362334 [details] [review]
mtp: Remove redundant ifdefs

HAVE_GUDEV guards are redundant, because gudev is hard requirement.
Comment 55 Ondrej Holy 2017-10-26 11:16:34 UTC
Created attachment 362335 [details] [review]
metadata: Remove udev dependency in favor of gudev

GVfs uses udev and also gudev. Let's use just gudev and remove corresponding
--enable-udev option.
Comment 56 Ondrej Holy 2017-10-26 11:39:52 UTC
Not sure about Comment 45, otherwise I think we are ready to go. Inigo?
Comment 57 Iñigo Martínez 2017-10-28 09:22:27 UTC
Created attachment 362442 [details] [review]
Remove unnecessary guards

(In reply to Ondrej Holy from comment #45)
> Yes, it would be nice to remove as much as possible, however, I don't know
> also whether they are needed, or not (e.g. for BSD systems). What happened
> with this in other projects when porting to meson (e.g. glib)? I am afraid
> of that only way how to find the answer is remove this stuff and wait for
> bug reports... so maybe now is the right time, but would be good to do it in
> a separate patch...

This is the list of defines that are not used in the source code, and therefore they can be removed:

PACKAGE
PACKAGE_BUGREPORT
PACKAGE_NAME
PACKAGE_TARNAME
PACKAGE_URL
PACKAGE_VERSION

HAVE_DLFCN_H
HAVE_INTTYPES_H
HAVE_MEMORY_H
HAVE_STDINT_H
HAVE_STDLIB_H
HAVE_STRINGS_H
HAVE_STRING_H
HAVE_SYS_STAT_H
HAVE_SYS_TYPES_H
HAVE_UNISTD_H

HAVE_SYS_UIO_H

HAVE_DCGETTEXT
HAVE_GETTEXT
HAVE_ICONV

HAVE_CFLOCALECOPYCURRENT
HAVE_CFPREFERENCESCOPYAPPVALUE

HAVE_STRUCT_STATFS_F_FSTYPENAME

This patch removes all of them except the `PACKAGE`, `PACKAGE_BUGREPORT`, `PACKAGE_NAME` as they might be used a some point. For example `PACKAGE_STRING` is already used.

I haven't seen any problem yet with any package that has removed them. Your approach looks very sensible to me, remove them and restore them on demand.
Comment 58 Iñigo Martínez 2017-10-28 09:27:15 UTC
(In reply to Ondrej Holy from comment #56)
> Not sure about Comment 45, otherwise I think we are ready to go. Inigo?

I'm sorry as I've been very busy lately. All the patches look good to me, just a couple of comments.

(In reply to Ondrej Holy from comment #47)
> Created attachment 362326 [details] [review] [review]
> build: Remove configuration summary
> 
> Meson port introduced boolean type for options and eliminated auto
> options, so the final configuration is currently deterministically
> based on given options and there is no need for the summary.

Although I understand this, I've got my doubts. gvfs has several options and the summary would allow a developer quickly check what is enabled or not. In the other hand, this can also be checked with meson's configure command...

(In reply to Ondrej Holy from comment #52)
> Review of attachment 362249 [details] [review] [review]:
> 
> I am tenatively ok to push this with the follow-up patches and patches from
> Bug 788707...

Once the original elogind patch is merged, a new patch can be prepared as at the moment the elogind's meson patch is just theoretical.

(In reply to Ondrej Holy from comment #53)
> Review of attachment 362251 [details] [review] [review]:
> 
> This is also ok to push, but I would wait until tasks from Comment 36 are
> finished...

My proposal, just merge meson port and try it for a while. We can fix any issue that may arise during that time. During this time, changes can be made to jhbuild and possibly gnome-continuous (I should check this). Once it's stable enough, let's remove autotools.

What do you think?
Comment 59 Iñigo Martínez 2017-10-31 12:35:10 UTC
Created attachment 362621 [details] [review]
Fix missing GDU monitor service file

This patch fixes the missing service variable definition when building GDU monitor.
Comment 60 Iñigo Martínez 2017-10-31 12:38:37 UTC
Created attachment 362623 [details] [review]
Improve installation on system paths

This patch provides an improved support of installation on system paths. All the paths used by D-Bus, systemd and GIO modules dir, can be checked by using the information in their pkg-config files.

This patch uses the information on pkg-config files by default, and also allows the user to provide this information to avoid overwriting system files.

By using custom directories it would also support `opentmpfiles`, that might be necessary if `elogind` support is merged.
Comment 61 Ondrej Holy 2017-10-31 16:59:56 UTC
Created attachment 362653 [details] [review]
build: Remove configuration summary

Add mention about "meson configuration" in commit description...
Comment 62 Ondrej Holy 2017-10-31 17:02:13 UTC
Created attachment 362654 [details] [review]
build: Improve installation on system paths

Fixed typo which breaks build and wrong assert message...
Comment 63 Ondrej Holy 2017-10-31 17:52:28 UTC
Inigo, thanks for all your work! I've just pushed all the patches except autotools removal and attachment 362335 [details] [review], which has to be pushed after the removal, or updated for autotools.

Attachment 362328 [details] pushed as 80bebd7 - build: Do not generate the deprecated programs by default
Attachment 362330 [details] pushed as 4555036 - build: Reorder meson options and update description
Attachment 362331 [details] pushed as ac30500 - build: Do not build gdu volume monitor by default
Attachment 362334 [details] pushed as b411443 - mtp: Remove redundant ifdefs
Attachment 362653 [details] pushed as 4a04220 - build: Remove configuration summary
Attachment 362654 [details] pushed as 5e2dff6 - build: Improve installation on system paths
Comment 64 Iñigo Martínez 2017-10-31 21:06:13 UTC
Thank you for your kind words but actually this has been teamwork :)

I have finished the jhbuild patch and opened a bug for it[0]. I hope someone could review it.

[0] https://bugzilla.gnome.org/show_bug.cgi?id=789736
Comment 65 Ondrej Holy 2017-11-01 07:51:46 UTC
Created attachment 362722 [details] [review]
metadata: Remove udev dependency in favor of gudev

Updated for autotools, so we can push it and avoid adding libudev dep in jhbuild... Inigo?
Comment 66 Iñigo Martínez 2017-11-01 09:50:10 UTC
Ít sounds good, however `enable-udev` removal from `meson_options.txt` is missing.

BTW, although is not totally related, I've been looking to the code at `metadata/meta-daemon.c` as I've noticed the use of `makedev`. It uses these guards:

 38 #if MAJOR_IN_MKDEV
 39 #include <sys/mkdev.h>
 40 #elif MAJOR_IN_SYSMACROS
 41 #include <sys/sysmacros.h>
 42 #endif

What about if neither `MAJOR_IN_MKDEV` nor `MAJOR_IN_SYSMACROS` are defined? This is, if `makedev` is not present. Should meson block the building of gvfs?

I'm sorry if this is not the place to ask for this.
Comment 67 Ondrej Holy 2017-11-01 10:00:15 UTC
Dammit, will remove the enable-udev also...

Hmm, we should add assert if none of them is defined, otherwise build fails anyway later...
Comment 68 Ondrej Holy 2017-11-01 10:02:56 UTC
Created attachment 362729 [details] [review]
metadata: Remove udev dependency in favor of gudev

Fixed.
Comment 69 Ondrej Holy 2017-11-01 10:03:54 UTC
Comment on attachment 362729 [details] [review]
metadata: Remove udev dependency in favor of gudev

And pushed, so please reflect this in the jhbuild patch...

Attachment 362729 [details] pushed as c2d8564 - metadata: Remove udev dependency in favor of gudev
Comment 70 Iñigo Martínez 2017-11-06 18:57:32 UTC
Created attachment 363068 [details] [review]
Fix GIO_MODULE_DIR in gdu volume monitor

The `GIO_MODULE_DIR` macro is using an invalid `gio_module_dir` variable. This patch fixes the variable name.
Comment 71 Iñigo Martínez 2017-11-06 18:59:01 UTC
Created attachment 363069 [details] [review]
build: Make SystemdService entry conditional

When systemd user units installation is disabled, SystemdService entry should also not be present on D-Bus service files.

This patch adds a common D-Bus service file template allowing, an homogeneus D-Bus service file generation and also a SystemService entry conditioned to the systemd user units installation option.
Comment 72 Iñigo Martínez 2017-11-06 19:00:24 UTC
Created attachment 363070 [details] [review]
build: Add an option for gcrypt

This patch dds an option to make gcrypt conditional allowing to disable it if desired. The patch also makes the existence of `libgcrypt-config` program mandatory if the gcrypt option is enabled.
Comment 73 Iñigo Martínez 2017-11-06 19:03:09 UTC
Created attachment 363071 [details] [review]
build: Fix GIO_MODULE_DIR in GDU volume monitor

I have fixed the commit title and description.
Comment 74 Ondrej Holy 2017-11-07 07:47:05 UTC
Review of attachment 363071 [details] [review]:

Sure.
Comment 75 Ondrej Holy 2017-11-07 07:47:08 UTC
Review of attachment 363070 [details] [review]:

Thanks, looks ok.
Comment 76 Ondrej Holy 2017-11-07 07:57:51 UTC
Review of attachment 363069 [details] [review]:

File dbus.service.in does not exist.

::: daemon/meson.build
@@ +3,3 @@
 # D-Bus and systemd service files
+dbus_service = gvfs_namespace + '.Daemon'
+dbus_exec = 'gvfsd'

Why do not define the whole exec path here:
dbus_exec = join_paths(gvfs_libexecdir, 'gvfsd')?

@@ +22,3 @@
 
+dbus_service_conf.set('service', dbus_service)
+dbus_service_conf.set('libexecdir', gvfs_libexecdir)

...so you can drop this.
Comment 77 Iñigo Martínez 2017-11-07 09:20:19 UTC
Comment on attachment 363071 [details] [review]
build: Fix GIO_MODULE_DIR in GDU volume monitor

Pushed as d274add5 - build: Fix GIO_MODULE_DIR in GDU volume monitor
Comment 78 Iñigo Martínez 2017-11-07 09:21:36 UTC
Comment on attachment 363070 [details] [review]
build: Add an option for gcrypt

Pushed as d1189c1b - build: Add an option for gcrypt
Comment 79 Iñigo Martínez 2017-11-07 09:33:29 UTC
Created attachment 363132 [details] [review]
build: Make SystemdService entry conditional

(In reply to Ondrej Holy from comment #76)
> Review of attachment 363069 [details] [review] [review]:
> 
> File dbus.service.in does not exist.

I've forgotten to add the template file. It's added now.

> ::: daemon/meson.build
> @@ +3,3 @@
>  # D-Bus and systemd service files
> +dbus_service = gvfs_namespace + '.Daemon'
> +dbus_exec = 'gvfsd'
> 
> Why do not define the whole exec path here:
> dbus_exec = join_paths(gvfs_libexecdir, 'gvfsd')?
> 
> @@ +22,3 @@
>  
> +dbus_service_conf.set('service', dbus_service)
> +dbus_service_conf.set('libexecdir', gvfs_libexecdir)
> 
> ...so you can drop this.

`dbus_exec` it's used also as the name of the systemd's service unit file. However, there is no need to set two variables, one for the directory and another for the executable. I've merged both so the `exec` variable is used now for the full path of the executable.

I'm also setting systemd user unit as empty string as default so meson doesn't warn about an unset variable when the systemd user unit option is disabled. This has also helped reorganizing the `conf` object, that is filled after its creation (and also sets the variable by the order of appearance in the template file).
Comment 80 Ondrej Holy 2017-11-07 12:00:27 UTC
Review of attachment 363132 [details] [review]:

Looks ok, thanks!
Comment 81 Iñigo Martínez 2017-11-07 12:20:54 UTC
Comment on attachment 363132 [details] [review]
build: Make SystemdService entry conditional

Pushed as 327f1eee - build: Make SystemdService entry conditional
Comment 82 Iñigo Martínez 2017-11-10 12:29:58 UTC
Created attachment 363340 [details] [review]
build: Rename build options

Following the new meson porting guidelines[0], this patch renames the build options. The list of changes is as follows:

- Remove the `enable` prefix from boolean options.
- Remove the `with` prefix from string options.
- The character separator from multi-word options has been changed
  to underscore (`_`).

[0] https://wiki.gnome.org/Initiatives/GnomeGoals/MesonPorting
Comment 83 Ondrej Holy 2017-11-10 14:31:54 UTC
Review of attachment 363340 [details] [review]:

Thanks, why not, guidelines are guidelines, so it is probably best to do it as soon as possible...

Maybe also the following needs to be reworded a bit:
man/meson.build:assert(xsltproc.found(), 'xsltproc is required for enable-man')

Just maybe we may let the internal meson variables with enable_ prefix for better readability, but I don't insist on it, just idea...
Comment 84 Iñigo Martínez 2017-11-10 15:37:52 UTC
Created attachment 363352 [details] [review]
build: Rename build options

I have redone the patch by modifying only the name of the options. Here goes the patch.

(In reply to Ondrej Holy from comment #83)
> Maybe also the following needs to be reworded a bit:
> man/meson.build:assert(xsltproc.found(), 'xsltproc is required for
> enable-man')

Thanks! I missed that one. I've changed to `xsltproc is required for man pages generation`. I hope it helps.

> Just maybe we may let the internal meson variables with enable_ prefix for
> better readability, but I don't insist on it, just idea...

I do agree. That is the reason why I renamed `programs` to `deprecated_programs`, `programs` collided with the `programs` array used in the `daemon` directory.

Anyway, `deprecated_programs` sounds good too, so I've left the new name. (jhbuild and continuous patches have been also sent with this modification, though, they can still be marked as obsolete if you don't like it).
Comment 85 Iñigo Martínez 2017-11-10 15:43:53 UTC
Created attachment 363353 [details] [review]
build: Remove default warning level

gvfs uses `1` as the project's default warning level. However, meson already uses this level as the default warning level, so it's actually unnecessary.

This patch removes the warning level from project's default options.
Comment 86 Ondrej Holy 2017-11-10 16:41:47 UTC
Review of attachment 363353 [details] [review]:

Makes sense!
Comment 87 Ondrej Holy 2017-11-10 16:41:54 UTC
Review of attachment 363352 [details] [review]:

Looks ok, thanks!
Comment 88 Iñigo Martínez 2017-11-10 20:27:48 UTC
Comment on attachment 363352 [details] [review]
build: Rename build options

Pushed as cdc33bf5 - build: Rename build options
Comment 89 Iñigo Martínez 2017-11-10 20:29:39 UTC
Comment on attachment 363353 [details] [review]
build: Remove default warning level

Pushed as b33a5de7 - build: Remove default warning level
Comment 90 Tristan Van Berkom 2017-11-11 10:03:11 UTC
I'm not sure if this is the right place to report the regression, but I'll comment here since the general bug report seems to still be open.

With autotools, there is a nice little work around to the famous polkit problem:

https://git.gnome.org/browse/gvfs/commit/?id=c4da270a189001393c2eb2c5ee2ddfb3f429a944

This is not present with meson builds, so gvfs fails to build with meson where it passes with autotools.
Comment 91 Iñigo Martínez 2017-11-11 10:59:33 UTC
Created attachment 363391 [details] [review]
build: Use ITS rules for polkit in meson

(In reply to Tristan Van Berkom from comment #90)
> I'm not sure if this is the right place to report the regression, but I'll
> comment here since the general bug report seems to still be open.
> 
> With autotools, there is a nice little work around to the famous polkit
> problem:
> 
> https://git.gnome.org/browse/gvfs/commit/
> ?id=c4da270a189001393c2eb2c5ee2ddfb3f429a944
> 
> This is not present with meson builds, so gvfs fails to build with meson
> where it passes with autotools.

Thank you for pointing this out Tristan. This patch should fix the issue.
Comment 92 Iñigo Martínez 2017-11-11 11:20:26 UTC
BTW, I've been trying to figure out how I missed this, because the files are there and it's obvious.

I remembered this bug[0], where I created new ITS rules (`its|loc` files) for AppStream files. Finally, this weren't necessary due to `appstream` package shipping with necessary rules.

I've noticed that `policykit` also ships with these rules (at least on my system), which makes me doubt if the patch would actually be necessary.

What do you think?

[0] https://bugzilla.gnome.org/show_bug.cgi?id=787105
Comment 93 Piotr Drąg 2017-11-11 17:34:24 UTC
Various distributions patch polkit or ship a git snapshot, to work around a lack of proper release with the ITS files.
Comment 94 Ondrej Holy 2017-11-13 07:59:06 UTC
Review of attachment 363391 [details] [review]:

Looks ok, thanks!
Comment 95 Iñigo Martínez 2017-11-13 08:09:39 UTC
Comment on attachment 363391 [details] [review]
build: Use ITS rules for polkit in meson

Pushed as 5694afe2 - build: Use ITS rules for polkit in meson
Comment 96 Iñigo Martínez 2017-11-13 12:33:45 UTC
Created attachment 363499 [details] [review]
build: Add dbus.service.in to distributable files

I've also missed to add the dbus.service.in template file to the autotools distributable file list[0].

I've built gvfs, create a distributable file and use the meson build system from that file a couple of times, so I hope everything is in place now.

[0] https://bugzilla.gnome.org/show_bug.cgi?id=786149#c76
Comment 97 Ondrej Holy 2017-11-13 12:36:47 UTC
Review of attachment 363499 [details] [review]:

Sure
Comment 98 Iñigo Martínez 2017-11-13 12:47:10 UTC
Comment on attachment 363499 [details] [review]
build: Add dbus.service.in to distributable files

Pushed as 192a38c3 - build: Add dbus.service.in to distributable files
Comment 99 Ondrej Holy 2017-11-13 13:15:13 UTC
Created attachment 363505 [details] [review]
build: Define HAVE_GCR by meson

HAVE_GCR is not	defined	by meson currently, so GVfs is built without
certificate support, which causes davs tests to	fail. Let's define it
properly.
Comment 100 Ondrej Holy 2017-11-13 13:16:33 UTC
Comment on attachment 363505 [details] [review]
build: Define HAVE_GCR by meson

Attachment 363505 [details] pushed as cdfd443 - build: Define HAVE_GCR by meson
Comment 101 Ondrej Holy 2017-12-19 08:45:25 UTC
Adding blockers for autotools removal...
Comment 102 Ting-Wei Lan 2017-12-24 13:36:38 UTC
The check for statfs doesn't work on FreeBSD:

Meson encountered an error in file meson.build, line 107, column 2:
Error encountered: unable to determine number of arguments to statfs()

The reason is that FreeBSD has neither sys/statfs.h nor sys/vfs.h. We have to check whether a header exists before including, just like what the autotools build does. I will upload a patch to fix this problem.
Comment 103 Ting-Wei Lan 2017-12-24 13:43:50 UTC
Created attachment 365933 [details] [review]
build: Fix statfs check on FreeBSD

To include a header without the possibility of causing 'no such file or
directory' error, we have to check for it before including it.
Comment 104 Iñigo Martínez 2017-12-24 14:07:36 UTC
(In reply to Ting-Wei Lan from comment #102)
> The check for statfs doesn't work on FreeBSD:
> 
> Meson encountered an error in file meson.build, line 107, column 2:
> Error encountered: unable to determine number of arguments to statfs()
> 
> The reason is that FreeBSD has neither sys/statfs.h nor sys/vfs.h. We have
> to check whether a header exists before including, just like what the
> autotools build does. I will upload a patch to fix this problem.

I'm sorry about this. I don't have access to a *BSD instance so I wasn't aware of this problem.

(In reply to Ting-Wei Lan from comment #103)
> Created attachment 365933 [details] [review] [review]
> build: Fix statfs check on FreeBSD
> 
> To include a header without the possibility of causing 'no such file or
> directory' error, we have to check for it before including it.

Thank you for the patch. I was thinking on another approach. Why not just include the existing headers instead of defining or undefining their guards?

if has_header
  includes += '#include "@0@"\n'.format(header[1])
endif

statfs_code = includes + '''
  int main() {
    struct statfs st;
    @0@;
 };
'''

What do you think?
Comment 105 Ting-Wei Lan 2017-12-24 14:17:29 UTC
(In reply to Iñigo Martínez from comment #104)
> (In reply to Ting-Wei Lan from comment #103)
> > Created attachment 365933 [details] [review] [review] [review]
> > build: Fix statfs check on FreeBSD
> > 
> > To include a header without the possibility of causing 'no such file or
> > directory' error, we have to check for it before including it.
> 
> Thank you for the patch. I was thinking on another approach. Why not just
> include the existing headers instead of defining or undefining their guards?
> 
> if has_header
>   includes += '#include "@0@"\n'.format(header[1])
> endif
> 
> statfs_code = includes + '''
>   int main() {
>     struct statfs st;
>     @0@;
>  };
> '''
> 
> What do you think?

Yes, it is simpler to directly add '#include' to code. The reason of using '#define' and '#unref' is to avoid including non-needed headers when the check_headers list is updated to contain many headers unrelated to statfs.
Comment 106 Iñigo Martínez 2017-12-24 17:43:28 UTC
(In reply to Ting-Wei Lan from comment #105)
> Yes, it is simpler to directly add '#include' to code. The reason of using
> '#define' and '#unref' is to avoid including non-needed headers when the
> check_headers list is updated to contain many headers unrelated to statfs.

A second specialized loop could be used if that happens.
Comment 107 Ting-Wei Lan 2017-12-24 18:08:15 UTC
Created attachment 365947 [details] [review]
build: Fix statfs check on FreeBSD

To include a header without the possibility of causing 'no such file or
directory' error, we have to check for it before including it.
Comment 108 Iñigo Martínez 2017-12-24 19:28:31 UTC
lgtm, thanks :)
Comment 109 Ondrej Holy 2018-01-02 10:10:08 UTC
Comment on attachment 365947 [details] [review]
build: Fix statfs check on FreeBSD

Attachment 365947 [details] pushed as 623ccee - build: Fix statfs check on FreeBSD

Thanks!
Comment 110 Ting-Wei Lan 2018-01-23 18:54:41 UTC
Meson fails to configure when gudev is disabled.

Meson encountered an error in file metadata/meson.build, line 67, column 0:
Unknown variable "gudev_dep".
Comment 111 Ting-Wei Lan 2018-01-23 18:59:13 UTC
Created attachment 367330 [details] [review]
build: Fix build when gudev is disabled

The reordering of libmetadata_dep and libgvfscommon_dep has a side effect of
fixing undefined reference error when using the default linker of FreeBSD.
Comment 112 Iñigo Martínez 2018-01-23 20:01:11 UTC
LGTM. It's also happening with `udisks2` monitor, but I don't know if `gudev` is optional or mandatory.
Comment 113 Iñigo Martínez 2018-01-23 20:10:10 UTC
Created attachment 367335 [details] [review]
build: Fix UDisks2 build when gudev is disabled

Here goes a fix for comment 112. Feel free to squash it with the previous commit as it would make sense to me.
Comment 114 Ting-Wei Lan 2018-01-24 08:11:00 UTC
udisks hard-depends on gudev and it is (at least currently) Linux-only, so I think it is expected for gvfs to require gudev when udisks is enabled.
Comment 115 Iñigo Martínez 2018-01-24 08:37:03 UTC
(In reply to Ting-Wei Lan from comment #114)
> udisks hard-depends on gudev and it is (at least currently) Linux-only, so I
> think it is expected for gvfs to require gudev when udisks is enabled.

Ok, thanks! It makes sense to me so the patch should be correct then :)
Comment 116 Ondrej Holy 2018-01-24 09:51:04 UTC
Review of attachment 367330 [details] [review]:

Looks, good to me, thanks.

Can you please add some commit description?

::: metadata/meson.build
@@ +72,3 @@
+if enable_gudev
+  metadata_deps += gudev_dep
+endif

I suppose that the same fix is needed also for monitor/gdu/meson.build, can you please prepare a patch for it also? Although I will probably remove the gdu volume monitor soon, because I have no evidence that it is used somewhere these days...
Comment 117 Ondrej Holy 2018-01-24 09:51:06 UTC
Review of attachment 367335 [details] [review]:

Thanks, looks ok!
Comment 118 Iñigo Martínez 2018-01-24 09:57:43 UTC
Comment on attachment 367335 [details] [review]
build: Fix UDisks2 build when gudev is disabled

Pushed as 8cd3e024 - build: Fix UDisks2 build when gudev is disabled
Comment 119 Ting-Wei Lan 2018-01-24 10:43:48 UTC
Created attachment 367366 [details] [review]
build: Fix build when gudev is disabled

The order of dependencies of gvfsd-metadata is reversed to workaround
undefined reference error on FreeBSD when -Wl,--as-needed is used.
Comment 120 Iñigo Martínez 2018-01-24 10:49:20 UTC
(In reply to Ting-Wei Lan from comment #119)
> Created attachment 367366 [details] [review] [review]
> build: Fix build when gudev is disabled
> 
> The order of dependencies of gvfsd-metadata is reversed to workaround
> undefined reference error on FreeBSD when -Wl,--as-needed is used.

Have you noticed that a bit below[0] the same set of dependendencies are used? Just in case more tweaking is needed.

[0] https://git.gnome.org/browse/gvfs/tree/metadata/meson.build#n95
Comment 121 Ting-Wei Lan 2018-01-24 10:53:05 UTC
Created attachment 367367 [details] [review]
build: Fix gvfsd-metadata build when gudev is disabled

The order of dependencies of gvfsd-metadata is reversed to workaround
undefined reference error on FreeBSD when -Wl,--as-needed is used.
Comment 122 Ting-Wei Lan 2018-01-24 10:56:02 UTC
Created attachment 367368 [details] [review]
build: Fix gdu monitor build when gudev is disabled

It seems that gdu.pc was removed from gnome-disk-utility more than 6 years ago.
I don't have any system which still runs GNOME from 2011, so it is untested.
Comment 123 Iñigo Martínez 2018-01-24 11:01:02 UTC
attachment 367366 [details] [review] and attachment 367367 [details] [review] seem identical. Is it possible that you have mixed the patches?
Comment 124 Ting-Wei Lan 2018-01-24 11:12:33 UTC
(In reply to Iñigo Martínez from comment #120)
> (In reply to Ting-Wei Lan from comment #119)
> > Created attachment 367366 [details] [review] [review] [review]
> > build: Fix build when gudev is disabled
> > 
> > The order of dependencies of gvfsd-metadata is reversed to workaround
> > undefined reference error on FreeBSD when -Wl,--as-needed is used.
> 
> Have you noticed that a bit below[0] the same set of dependendencies are
> used? Just in case more tweaking is needed.
> 
> [0] https://git.gnome.org/browse/gvfs/tree/metadata/meson.build#n95

Thanks for your reminding. I didn't see the problem because devel_utils is disabled by default. I really don't like the old linker from binutils 2.17 released in 2007 provided by FreeBSD ...

(In reply to Iñigo Martínez from comment #123)
> attachment 367366 [details] [review] [review] and attachment 367367 [details] [review]
> [review] seem identical. Is it possible that you have mixed the patches?

It is just a simple update to the commit message because I didn't notice your comment before uploading the new patch. Yes, I used git-bz to submit those two patches in a row.
Comment 125 Ting-Wei Lan 2018-01-24 11:14:14 UTC
Created attachment 367369 [details] [review]
build: Fix gvfsd-metadata build when gudev is disabled

The order of dependencies of gvfsd-metadata is reversed to workaround
undefined reference error on FreeBSD when -Wl,--as-needed is used.
Comment 126 Ondrej Holy 2018-01-24 11:19:22 UTC
Review of attachment 367368 [details] [review]:

Looks ok, thanks!
Comment 127 Ondrej Holy 2018-01-24 11:19:24 UTC
Review of attachment 367369 [details] [review]:

Looks ok, thanks!
Comment 128 Ting-Wei Lan 2018-01-24 11:36:16 UTC
Attachment 367369 [details] pushed as d7262a6 - build: Fix gvfsd-metadata build when gudev is disabled
Attachment 367368 [details] pushed as 75b5097 - build: Fix gdu monitor build when gudev is disabled
Comment 129 Ondrej Holy 2018-01-25 13:03:07 UTC
(In reply to Ting-Wei Lan from comment #122)
> Created attachment 367368 [details] [review] [review]
> build: Fix gdu monitor build when gudev is disabled
> 
> It seems that gdu.pc was removed from gnome-disk-utility more than 6 years
> ago.

Good to know that, then it can be safely removed...
Comment 130 Iñigo Martínez 2018-02-05 18:55:24 UTC
One of the major issues that still remains to be solved is the generation of the D-Bus files with `gdbus-codegen`. The issue involved both glib and meson. The glib side is already fixed[0], but meson still is not.

There is a MR[1] that might be correct but it's still pending (Probably due to the LCA and/or FOSDEM).

Recently, `gnome-settings-daemon` has also been ported and it suffered the same issue. I have applied an (ugly) workaround[2] that involves a new script that simulates both files generation, but it does the trick.

Ondrej, do you think it's worth applying the same workaround to gvfs?

[0] https://bugzilla.gnome.org/show_bug.cgi?id=791015
[1] https://github.com/mesonbuild/meson/pull/2930
[2] https://bugzilla.gnome.org/show_bug.cgi?id=793087#c41
Comment 131 Ondrej Holy 2018-02-06 12:00:35 UTC
Thanks for the update, but I am afraid of that it is too late now for autotools removal, so I don't think that this workaround is really necessary...
Comment 132 Iñigo Martínez 2018-02-06 12:04:28 UTC
Ok. I'm sorry for being late. I was expecting this issue to be solved some time ago.

Let's wait then for the next cycle to improve the missing bits and remove autotools properly.
Comment 133 Ondrej Holy 2018-03-19 09:46:23 UTC
It is still not yet resolved upstream :-( and new devel phase is here. Inigo, can you please provide that suggested workaround, so we can fully switch to meson and remove the autotools support finally?
Comment 134 Ondrej Holy 2018-03-21 08:19:38 UTC
*** Bug 794549 has been marked as a duplicate of this bug. ***
Comment 135 Brendan L 2018-04-03 04:32:51 UTC
The newest gvfs-1.36 with meson can't be configured without ssh, whereas it could be with gvfs-1.34 and autotools.  I never had to have ssh installed to configure and build gvfs.  Thought this bug would be a good place to mention it.
Comment 136 Iñigo Martínez 2018-04-03 05:48:44 UTC
(In reply to Brendan L from comment #135)
> The newest gvfs-1.36 with meson can't be configured without ssh, whereas it
> could be with gvfs-1.34 and autotools.  I never had to have ssh installed to
> configure and build gvfs.  Thought this bug would be a good place to mention
> it.

Yes, this is the place for this. Thank you for testing this and writing the comment :)

Actually, looking at autotools files[0][1][2], SSH seems to be mandatory. However, `AC_PATH_PROG` sets `SSH_PROGRAM` to `ssh` (although it's not mentioned what is `AC_PATH_PROG` behaviour when the program is not found and `value-if-not-found` is not given[3])

It can be fixed rather easily, but what should the correct behaviour be in this case? use a default value as autotools does even if `ssh` is not present? Throw an error as meson does now so we can be sure that `ssh` is present?

[0] https://git.gnome.org/browse/gvfs/tree/configure.ac#n104
[1] https://git.gnome.org/browse/gvfs/tree/daemon/Makefile.am#n323
[2] https://git.gnome.org/browse/gvfs/tree/daemon/gvfsbackendsftp.c#n244
[3] https://www.gnu.org/software/autoconf/manual/autoconf-2.68/html_node/Generic-Programs.html
Comment 137 Ondrej Holy 2018-04-03 13:26:01 UTC
I would suggest adding "sftp" configuration option, enable by default and simply fail if find_program fails...
Comment 138 Iñigo Martínez 2018-04-03 18:15:48 UTC
Created attachment 370497 [details] [review]
build: Make SFTP backend optional

One of the requirements for SFTP is the existence of a ssh client.

A new option has been added to make SFTP backend optional, so a ssh client is not a hard requirement anymore.
Comment 139 Ondrej Holy 2018-04-04 06:24:28 UTC
Review of attachment 370497 [details] [review]:

Thanks, looks ok, just...

::: meson.build
@@ +458,3 @@
+if enable_sftp
+  ssh = find_program('ssh', required: false)
+  assert(ssh.found(), 'SFTP backend requested but a ssh client is required')

Not sure about this assert since we can use "required: true" as in other cases, but it is definitely more understandable...
Comment 140 Iñigo Martínez 2018-04-04 14:29:00 UTC
Comment on attachment 370497 [details] [review]
build: Make SFTP backend optional

Pushed as 44d45dca - build: Make SFTP backend optional

(In reply to Ondrej Holy from comment #139)
> Not sure about this assert since we can use "required: true" as in other
> cases, but it is definitely more understandable...

You are right, technically is unnecessary. However, imho the message falls a little short:

meson.build:459:2: ERROR: Program(s) ['ssh'] not found or not executable

There is no obvious reason why a ssh client is necessary.

It would be nice to add a parameter to dependencies/programs, as an alternate message when they aren't found. I would change this if support for custom messages is added to meson.
Comment 141 Bastien Nocera 2018-04-04 14:54:54 UTC
Why not default to "look for ssh in PATH when instantiating the backend", and allow an option to override the ssh program's path?

You could make the backend disable-able, if it was never going to be used.
Comment 142 Iñigo Martínez 2018-04-05 19:53:22 UTC
Created attachment 370559 [details] [review]
build: Remove autotools

gnome-continuous is finally able to build gvfs with meson, so I have rebased the patch to remove autotools.
Comment 143 Ondrej Holy 2018-04-06 09:12:15 UTC
(In reply to Bastien Nocera from comment #141)
> Why not default to "look for ssh in PATH when instantiating the backend",
> and allow an option to override the ssh program's path?

Simply because nobody requested option for custom ssh path yet...

Why would be better to check this when instantiating?

> You could make the backend disable-able, if it was never going to be used.

That's exactly what we did by attachment 370497 [details] [review], no?
Comment 144 Ondrej Holy 2018-04-06 09:12:33 UTC
Review of attachment 370559 [details] [review]:

Looks ok, thanks, so let's remove it finally...
Comment 145 Iñigo Martínez 2018-04-06 15:29:13 UTC
attachment 370559 [details] [review] pushed as 5aaf80fd - build: Remove autotools

imho, meson support is actually quite stable :)

We are only missing linker features checking, which is handled in bug 794365, so let's close this issue.
Comment 146 Ondrej Holy 2018-04-06 15:35:51 UTC
Thanks for all of this work!
Comment 147 Ondrej Holy 2018-04-09 08:45:01 UTC
*** Bug 795053 has been marked as a duplicate of this bug. ***
Comment 148 Bastien Nocera 2018-04-09 16:28:12 UTC
(In reply to Ondrej Holy from comment #143)
> (In reply to Bastien Nocera from comment #141)
> > Why not default to "look for ssh in PATH when instantiating the backend",
> > and allow an option to override the ssh program's path?
> 
> Simply because nobody requested option for custom ssh path yet...

I'm saying that you could replace the check for a local binary (at compile-time), with a check for a binary in the path (at run-time).

> Why would be better to check this when instantiating?

Because you wouldn't need to install ssh on the build machine, and it could be installed in a different place on the build machine, and at run-time.

> > You could make the backend disable-able, if it was never going to be used.
> 
> That's exactly what we did by attachment 370497 [details] [review] [review], no?

Right, my idea was to always enable it, because you'd either:
- consider the "ssh" binary path to be a compile-time configurable path, for example, check whether "ssh" is in the path, if not, use "/usr/bin/ssh" or a path provided by the vendor. At this point, you can choose to have an enable/disable option as well, but is there any case where we'd want not to build the ssh backend?
- consider the "ssh" binary path to be a run-time check, and check for "ssh" in the path when instantiating the backend. Again, the backend could be made disable-able at compile-time, but is there a case where that's useful?
Comment 149 Ondrej Holy 2018-04-10 12:47:03 UTC
Hmm, gvfs.spec for Fedora contains "BuildRequires: /usr/bin/ssh" anyway, so should not be a problem there... but it is true that Comment 135 doesn't explicitly ask for the possibility to disable sftp at all, however, Bug 795053 does. We can change this later if it will cause some issues somewhere...