GNOME Bugzilla – Bug 786149
Port to meson build system
Last modified: 2018-04-10 12:47:03 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,
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.
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
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.
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.
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?
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.
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.
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.
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
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
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
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
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
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’
I've attached fixes for the warnings which start appearing after meson port...
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
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
(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?
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
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
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'?
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
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).
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.
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.
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.
Review of attachment 361222 [details] [review]: Thanks, why not...
Review of attachment 361223 [details] [review]: Thanks, looks ok.
Comment on attachment 361222 [details] [review] Make use of shutil's which Pushed as b460e265 - test: Make use of shutil's which
Comment on attachment 361223 [details] [review] Make sshd test optional Pushed as cffb8e8b - test: Make sshd test optional
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.
(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
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.
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
Created attachment 361624 [details] [review] Remove autotools A folow up update.
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
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
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 :)
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.
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
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.
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.
Created attachment 362251 [details] [review] Remove autotools A follow up update.
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.
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...
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.
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.
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.
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.
Created attachment 362330 [details] [review] build: Reorder meson options and update description
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.
Review of attachment 362249 [details] [review]: I am tenatively ok to push this with the follow-up patches and patches from Bug 788707...
Review of attachment 362251 [details] [review]: This is also ok to push, but I would wait until tasks from Comment 36 are finished...
Created attachment 362334 [details] [review] mtp: Remove redundant ifdefs HAVE_GUDEV guards are redundant, because gudev is hard requirement.
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.
Not sure about Comment 45, otherwise I think we are ready to go. Inigo?
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.
(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?
Created attachment 362621 [details] [review] Fix missing GDU monitor service file This patch fixes the missing service variable definition when building GDU monitor.
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.
Created attachment 362653 [details] [review] build: Remove configuration summary Add mention about "meson configuration" in commit description...
Created attachment 362654 [details] [review] build: Improve installation on system paths Fixed typo which breaks build and wrong assert message...
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
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
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?
Í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.
Dammit, will remove the enable-udev also... Hmm, we should add assert if none of them is defined, otherwise build fails anyway later...
Created attachment 362729 [details] [review] metadata: Remove udev dependency in favor of gudev Fixed.
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
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.
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.
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.
Created attachment 363071 [details] [review] build: Fix GIO_MODULE_DIR in GDU volume monitor I have fixed the commit title and description.
Review of attachment 363071 [details] [review]: Sure.
Review of attachment 363070 [details] [review]: Thanks, looks ok.
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 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 on attachment 363070 [details] [review] build: Add an option for gcrypt Pushed as d1189c1b - build: Add an option for gcrypt
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).
Review of attachment 363132 [details] [review]: Looks ok, thanks!
Comment on attachment 363132 [details] [review] build: Make SystemdService entry conditional Pushed as 327f1eee - build: Make SystemdService entry conditional
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
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...
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).
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.
Review of attachment 363353 [details] [review]: Makes sense!
Review of attachment 363352 [details] [review]: Looks ok, thanks!
Comment on attachment 363352 [details] [review] build: Rename build options Pushed as cdc33bf5 - build: Rename build options
Comment on attachment 363353 [details] [review] build: Remove default warning level Pushed as b33a5de7 - build: Remove default warning level
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.
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.
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
Various distributions patch polkit or ship a git snapshot, to work around a lack of proper release with the ITS files.
Review of attachment 363391 [details] [review]: Looks ok, thanks!
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
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
Review of attachment 363499 [details] [review]: Sure
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
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 on attachment 363505 [details] [review] build: Define HAVE_GCR by meson Attachment 363505 [details] pushed as cdfd443 - build: Define HAVE_GCR by meson
Adding blockers for autotools removal...
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.
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.
(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?
(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.
(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.
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.
lgtm, thanks :)
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!
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".
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.
LGTM. It's also happening with `udisks2` monitor, but I don't know if `gudev` is optional or mandatory.
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.
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.
(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 :)
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...
Review of attachment 367335 [details] [review]: Thanks, looks ok!
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
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.
(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
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.
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.
attachment 367366 [details] [review] and attachment 367367 [details] [review] seem identical. Is it possible that you have mixed the patches?
(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.
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.
Review of attachment 367368 [details] [review]: Looks ok, thanks!
Review of attachment 367369 [details] [review]: Looks ok, thanks!
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
(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...
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
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...
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.
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?
*** Bug 794549 has been marked as a duplicate of this bug. ***
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.
(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
I would suggest adding "sftp" configuration option, enable by default and simply fail if find_program fails...
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.
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 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.
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.
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.
(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?
Review of attachment 370559 [details] [review]: Looks ok, thanks, so let's remove it finally...
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.
Thanks for all of this work!
*** Bug 795053 has been marked as a duplicate of this bug. ***
(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?
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...