Created attachment 362706 [details] [review]
core-3.28: gvfs now uses meson

Although autotools is still present, it will be removed on the future. This patch changes the default build system to meson.

One of the changes of meson is that auto options have been changed to boolean options, so unsupported features must be explicitly disabled. In order to test all the available features in gvfs, this patch also adds support for new core dependencies and their repositories.

Despite the fact that `libplist` and `libimobiledevice` core dependencies have been added, they are not used at the moment due to a bug in `libimobiledevice`[0] in their last release.

Finally, any suggestion would be very appreciated.

[0] https://github.com/libimobiledevice/libimobiledevice/issues/254

Review of attachment 362706 [details] [review]:

Good, but it would be even better if the new dependencies would go into sysdeps instead of core-deps. If the dependency is new enough in both Ubuntu 17.10 and Fedora 27 (which should be out very soon), then it's a good candidate for sysdeps. Only if we really need a newer version than is available in distros should it go into core-deps. That way, we minimize the number of modules that we're responsible for building.

Review of attachment 362706 [details] [review]:

Yes, sysdeps should be new enough and should fix the problem with libimobiledevice... 

Also, we should enable the tests by default: -Denable-installed-tests=true

And maybe: -Dwith-dbus-service-dir=${prefix}/share/dbus-1/services 

Michael?

Created attachment 362723 [details] [review]
core-3.28: gvfs now uses meson

(In reply to Michael Catanzaro from comment #2)
> Review of attachment 362706 [details] [review] [review]:
> 
> Good, but it would be even better if the new dependencies would go into
> sysdeps instead of core-deps. If the dependency is new enough in both Ubuntu
> 17.10 and Fedora 27 (which should be out very soon), then it's a good
> candidate for sysdeps. Only if we really need a newer version than is
> available in distros should it go into core-deps. That way, we minimize the
> number of modules that we're responsible for building.

Thank you for your review. I had several doubts regarding how to do this :)

As you pointed out, I have moved the dependencies to sysdep. I have been checking Ubuntu and Fedora packages list and both have recent enough dependecies. This has also allowed to enable AFC support. I hope the patch is in better shape now.

I would like to ask you another question. There is a wip patch for gvfs[0] which adds `elogind` support. I didn't know it, but `elogind` is a trimmed version of `systemd-login` used by `gentoo` and `openrc`. The patch checks if `systemd` is present or if it's not, it checks for `elogind` support. This support can be disabled by using a build option.

My approach would be to also add `elogind` as sysdeps, so in case `systemd` is not set, jhbuild can check for it. However, this would work only for `gentoo` and/or `openrc` systems. Would this be the correct approach or should it simply be disabled?

[0] https://bugzilla.gnome.org/show_bug.cgi?id=788707

Created attachment 362732 [details] [review]
core-3.28: gvfs now uses meson

Two minor changes from the previous patch:

- `libudev` dependency is not necessary anymore as gvfs now depends only on `gudev`.[0]
- `elogind` support has been merged and the `enable-libsystemd-login` option has been renamed to `enable-logind`.[1]

[0] https://git.gnome.org/browse/gvfs/commit/?id=c2d8564
[1] https://git.gnome.org/browse/gvfs/commit/?id=1e9d338

(In reply to Iñigo Martínez from comment #4)
> My approach would be to also add `elogind` as sysdeps, so in case `systemd`
> is not set, jhbuild can check for it. However, this would work only for
> `gentoo` and/or `openrc` systems. Would this be the correct approach or
> should it simply be disabled?

Forget about it. You could add an elogind JHBuild condition, but it's really not worth the effort, especially as you've already used the systemd condition to ensure gvfs builds just fine without it.

Review of attachment 362732 [details] [review]:

This looks good.

Comment on attachment 362732 [details] [review]
core-3.28: gvfs now uses meson

Pushed as ed10b16e - core-3.28: gvfs now uses meson

Thank you Michael for your kind support.

We should probably add also:
-Dwith-dbus-service-dir=${prefix}/share/dbus-1/services

In order to prevent the following error:
W: Files remaining in buildroot u'/opt/gnome/_jhbuild/root-gvfs-broken'; module may have installed files outside of prefix.

I've thought initially that dbus is also built as a dependency, but it is system dependency currently, so the dbus service dir points in /usr/share/dbus-1/services/...

Not sure if really somebody uses the whole jhbuild session... but systemd user units are also needed currenty in order to avoid errors like:
Error creating proxy: Error calling StartServiceByName for org.gtk.vfs.UDisks2VolumeMonitor: GDBus.Error:org.freedesktop.systemd1.NoSuchUnit: Unit gvfs-udisks2-volume-monitor.service not found. (g-io-error-quark, 36)
...

However, we should rather remove SystemdService= fields from dbus services if it is built without systemd user units...

Created attachment 363073 [details] [review]
core-3.28: Add D-Bus service dir option in gvfs

(In reply to Ondrej Holy from comment #9)
> We should probably add also:
> -Dwith-dbus-service-dir=${prefix}/share/dbus-1/services
> 
> In order to prevent the following error:
> W: Files remaining in buildroot u'/opt/gnome/_jhbuild/root-gvfs-broken';
> module may have installed files outside of prefix.
> 
> I've thought initially that dbus is also built as a dependency, but it is
> system dependency currently, so the dbus service dir points in
> /usr/share/dbus-1/services/...

This patch fixes this by adding the option above to the meson arguments.

I'd like to pose a question: is there a way to use ${datadir} instead of ${prefix}?

I think that the proper option value should be '${datadir}/dbus-1/services', since `datadir` could theoretically vary inside `prefix`. (Please correct me if i'm wrong).

I tried it in my computer (just with meson, not with jhbuild) without any luck.

(In reply to Ondrej Holy from comment #10)
> Not sure if really somebody uses the whole jhbuild session... but systemd
> user units are also needed currenty in order to avoid errors like:
> Error creating proxy: Error calling StartServiceByName for
> org.gtk.vfs.UDisks2VolumeMonitor:
> GDBus.Error:org.freedesktop.systemd1.NoSuchUnit: Unit
> gvfs-udisks2-volume-monitor.service not found. (g-io-error-quark, 36)
> ...
> 
> However, we should rather remove SystemdService= fields from dbus services
> if it is built without systemd user units...

Fix posted at: https://bugzilla.gnome.org/show_bug.cgi?id=786149#c71

commit ed10b16ea6a58e1b378230040c5f1e389cab5399 breaks the build.

Notably it adds:

+      <dep package="avahi-client"/>
+      <dep package="avahi-glib"/>

These dont exist in 3.28 modulesets.

(In reply to Tristan Van Berkom from comment #12)
> commit ed10b16ea6a58e1b378230040c5f1e389cab5399 breaks the build.
> 
> Notably it adds:
> 
> +      <dep package="avahi-client"/>
> +      <dep package="avahi-glib"/>
> 
> These dont exist in 3.28 modulesets.

Fixed at least the above breakage with the following commit, still need to see if it actually builds.


commit 74dab83ca2554b6b87b9372c288d84b7e43c6420
Author: Tristan Van Berkom <tristan.vanberkom@codethink.co.uk>
Date:   Wed Nov 8 18:50:51 2017 +0900

    core-3.28: Fixing breakage from gvfs changes
    
    The gvfs meson migration added a bunch of dependencies, not all
    of which are actually real.
    
      o avahi-client
      o avahi-glib
      o gudev
    
    Replaced these with existing dependencies:
    
      o avahi
      o libgudev

Looks correct, thanks!

Created attachment 363195 [details] [review]
sysdeps-3.28: Add avahi-client and avahi-glib

(In reply to Tristan Van Berkom from comment #12)
> commit ed10b16ea6a58e1b378230040c5f1e389cab5399 breaks the build.
> 
> Notably it adds:
> 
> +      <dep package="avahi-client"/>
> +      <dep package="avahi-glib"/>
> 
> These dont exist in 3.28 modulesets.

I thought I'd checked all the dependencies and the missing ones were already added...

This patch should fix the issue by adding these dependencies as system modules.

Please, let me know if it's ok for you, so I can merge it as soon as possible.

Thank you Tristan for pointing out the problem.

Oh, I arrived late. Please Tristan, let me know if the problem is fixed so I can mark the patch as obsolete.

Review of attachment 363195 [details] [review]:

I am convinced that sysdeps for avahi-client and avahi-glib are not needed because: 

gnome-sysdeps-3.28.modules
  <systemmodule id="avahi">
    <pkg-config>avahi-gobject.pc</pkg-config>
    <branch repo="system"
            version="0.6.31"/>
  </systemmodule>

sudo dnf repoquery --whatprovides */avahi-gobject.pc
avahi-gobject-devel-0:0.7-6.fc28.x86_64
...

$ sudo dnf repoquery --requires avahi-gobject-devel
pkgconfig(avahi-client)
pkgconfig(avahi-glib)
...

So it should be ok with commit 74dab83 :-)

Created attachment 363342 [details] [review]
core-3.28: Rename build options in gvfs

Following the new meson porting guidelines[0], gvfs build options have been renamed[1]. This patch modifies accordingly the build options used for gvfs.

[0] https://wiki.gnome.org/Initiatives/GnomeGoals/MesonPorting
[1] 0] https://wiki.gnome.org/Initiatives/GnomeGoals/MesonPorting

Comment on attachment 363073 [details] [review]
core-3.28: Add D-Bus service dir option in gvfs

Similar fix has been already pushed:
commit 96f5360b4f7bb1de13c3e210df84de2c1e3e918a

Comment on attachment 363342 [details] [review]
core-3.28: Rename build options in gvfs

Similar fix has been already pushed:
commit 55003c6f999da35055ef24664b2d068debf773aa

Let's close the bug. It seems GVfs is being built correctly with jhbuild...