Review of attachment 318439 [details] [review]:

Thanks for your patch.

Please add some description in a commit messages and link for a bug report.

The patch is in some wrong format, please try to regenerate using "git format-patch":
$ git am gvfs-no-systemd.patch
Patch format detection failed.

Also it seems there are still SystemdService entries in services files, which is probably wrong.

::: daemon/Makefile.am
@@ +41,3 @@
 
+if HAVE_SYSTEMD
+systemd_userdir = @systemd_user_units_dir@

Please do this change ${prefix}/lib/systemd/user -> @systemd_user_units_dir@ in separate commit.

@@ +153,3 @@
+EXTRA_DIST += \
+       $(systemd_user_in_files)        \
+        $(NULL)

Please fix whitespaces to match rest of the file...
(it applies on all other files)

::: monitor/afc/Makefile.am
@@ +62,1 @@
    $(systemd_user_in_files) \

It seems this line should be removed also...

::: monitor/gdu/Makefile.am
@@ +70,1 @@
    $(systemd_user_in_files) \

It seems this line should be removed also...

::: monitor/hal/Makefile.am
@@ +71,3 @@
 
+endif
+

No need to add another line...

::: configure.ac
@@ +266,3 @@
+
+sustemd_user_dir=${prefix}/lib/systemd/user
+AC_ARG_WITH([systemd-userunits], 

I would check for systemd modules instead and it has to be enabled by default. It might be merged with libsystemd-login check...

Review of attachment 318439 [details] [review]:

::: configure.ac
@@ +266,3 @@
+
+sustemd_user_dir=${prefix}/lib/systemd/user
+AC_ARG_WITH([systemd-userunits], 

It seems it is common to use "--with-systemduserunitdir", so taking back, but "--with-systemd-userunits" needs to changed to "--with-systemduserunitdir".

@@ +279,3 @@
+msg_systemd_user_units=${systemd_user_units}
+
+AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$systemd_user_units" != "xno"])

Also would be nice to use something like HAVE_SYSTEMD_USER_UNITS, or so...

@@ +971,3 @@
+        Bash-completion support:          $msg_bash_completion
+        Directory for systemd user units: $msg_systemd_user_units
+        Installed tests:                  $msg_installed_tests

Please do not change whitespace for lines, which you don't edit.

Created attachment 318857 [details] [review]
A reviewed version of previous patch

(In reply to Ondrej Holy from comment #1)
> Review of attachment 318439 [details] [review] [review]:
> 
> Also it seems there are still SystemdService entries in services files,
> which is probably wrong.
> 
No, see this link for details: https://github.com/blueman-project/blueman/pull/361#issuecomment-144802023

Created attachment 318858 [details] [review]
Use substitution string from configure

Comment on attachment 318858 [details] [review]
Use substitution string from configure

>commit c81c1f2a643076318daa8b5e40617468474cd0e5
>Author: Yaroslav Shmelev <yars068@yandex.ru>
>Date:   Tue Jan 12 16:00:24 2016 +0300
>
>    Use @systemd_user_units_dir@ to substitute the placement from configure
>    For more details, see https://bugzilla.gnome.org/show_bug.cgi?id=760293
>
>diff --git a/configure.ac b/configure.ac
>index c1fd28c..89c7fbd 100644
>--- a/configure.ac
>+++ b/configure.ac
>@@ -273,7 +273,7 @@ AC_ARG_WITH([systemd-userunitdir],
> 
> if test "x$systemd_user_units" != "xno"; then
>     PKG_CHECK_MODULES([SYSTEMD], [systemd >= 219],
>-        [])
>+        AC_SUBST(systemd_user_units_dir, [$systemd_user_dir]))
> fi
> 
> msg_systemd_userunits=${systemd_user_units}
>diff --git a/metadata/Makefile.am b/metadata/Makefile.am
>index ca6105c..e9605af 100644
>--- a/metadata/Makefile.am
>+++ b/metadata/Makefile.am
>@@ -73,7 +73,7 @@ service_in_files = org.gtk.vfs.Metadata.service.in
> service_DATA = org.gtk.vfs.Metadata.service
> 
> if HAVE_SYSTEMD_USER_UNITS_DIR
>-systemd_userdir = ${prefix}/lib/systemd/user
>+systemd_userdir = @systemd_user_units_dir@
> systemd_user_in_files = gvfs-metadata.service.in
> systemd_user_DATA = gvfs-metadata.service
> endif
>diff --git a/monitor/afc/Makefile.am b/monitor/afc/Makefile.am
>index 0de8315..4364852 100644
>--- a/monitor/afc/Makefile.am
>+++ b/monitor/afc/Makefile.am
>@@ -36,7 +36,7 @@ service_in_files = org.gtk.vfs.AfcVolumeMonitor.service.in
> service_DATA = $(service_in_files:.service.in=.service)
> 
> if HAVE_SYSTEMD_USER_UNITS_DIR
>-systemd_userdir = ${prefix}/lib/systemd/user
>+systemd_userdir = @systemd_user_units_dir@
> systemd_user_in_files = gvfs-afc-volume-monitor.service.in
> systemd_user_DATA = $(systemd_user_in_files:.service.in=.service)
> endif
>diff --git a/monitor/gdu/Makefile.am b/monitor/gdu/Makefile.am
>index c361f39..c012d0f 100644
>--- a/monitor/gdu/Makefile.am
>+++ b/monitor/gdu/Makefile.am
>@@ -44,7 +44,7 @@ service_in_files = org.gtk.vfs.GduVolumeMonitor.service.in
> service_DATA     = $(service_in_files:.service.in=.service)
> 
> if HAVE_SYSTEMD_USER_UNITS_DIR
>-systemd_userdir = ${prefix}/lib/systemd/user
>+systemd_userdir = @systemd_user_units_dir@
> systemd_user_in_files = gvfs-gdu-volume-monitor.service.in
> systemd_user_DATA = $(systemd_user_in_files:.service.in=.service)
> endif
>diff --git a/monitor/goa/Makefile.am b/monitor/goa/Makefile.am
>index 4421dec..b6f7a83 100644
>--- a/monitor/goa/Makefile.am
>+++ b/monitor/goa/Makefile.am
>@@ -36,7 +36,7 @@ service_in_files = org.gtk.vfs.GoaVolumeMonitor.service.in
> service_DATA = $(service_in_files:.service.in=.service)
> 
> if HAVE_SYSTEMD_USER_UNIT_DIR
>-systemd_userdir = ${prefix}/lib/systemd/user
>+systemd_userdir = @systemd_user_inits_dir@
> systemd_user_in_files = gvfs-goa-volume-monitor.service.in
> systemd_user_DATA = $(systemd_user_in_files:.service.in=.service)
> endif
>diff --git a/monitor/gphoto2/Makefile.am b/monitor/gphoto2/Makefile.am
>index 055db95..6215d22 100644
>--- a/monitor/gphoto2/Makefile.am
>+++ b/monitor/gphoto2/Makefile.am
>@@ -78,7 +78,7 @@ service_in_files = org.gtk.vfs.GPhoto2VolumeMonitor.service.in
> service_DATA     = $(service_in_files:.service.in=.service)
> 
> if HAVE_SYSTEMD_USER_UNITS_DIR
>-systemd_userdir = ${prefix}/lib/systemd/user
>+systemd_userdir = @systemd_user_units_dir@
> systemd_user_in_files = gvfs-gphoto2-volume-monitor.service.in
> systemd_user_DATA = $(systemd_user_in_files:.service.in=.service)
> endif
>diff --git a/monitor/hal/Makefile.am b/monitor/hal/Makefile.am
>index ef5c7ec..dbc7928 100644
>--- a/monitor/hal/Makefile.am
>+++ b/monitor/hal/Makefile.am
>@@ -57,7 +57,7 @@ service_in_files = org.gtk.vfs.HalVolumeMonitor.service.in
> service_DATA     = $(service_in_files:.service.in=.service)
> 
> if HAVE_SYSTEMD_USER_UNITS_DIR
>-systemd_userdir = ${prefix}/lib/systemd/user
>+systemd_userdir = @systemd_user_units_dir@
> systemd_user_in_files = gvfs-hal-volume-monitor.service.in
> systemd_user_DATA = $(systemd_user_in_files:.service.in=.service)
> endif
>diff --git a/monitor/mtp/Makefile.am b/monitor/mtp/Makefile.am
>index 35f927d..997b591 100644
>--- a/monitor/mtp/Makefile.am
>+++ b/monitor/mtp/Makefile.am
>@@ -41,7 +41,7 @@ service_in_files = org.gtk.vfs.MTPVolumeMonitor.service.in
> service_DATA     = $(service_in_files:.service.in=.service)
> 
> if HAVE_SYSTEMD_USER_UNITS_DIR
>-systemd_userdir = ${prefix}/lib/systemd/user
>+systemd_userdir = @systemd_user_units_dir@
> systemd_user_in_files = gvfs-mtp-volume-monitor.service.in
> systemd_user_DATA = $(systemd_user_in_files:.service.in=.service)
> endif
>diff --git a/monitor/udisks2/Makefile.am b/monitor/udisks2/Makefile.am
>index 6aa02b2..83acd21 100644
>--- a/monitor/udisks2/Makefile.am
>+++ b/monitor/udisks2/Makefile.am
>@@ -49,7 +49,7 @@ service_in_files = org.gtk.vfs.UDisks2VolumeMonitor.service.in
> service_DATA     = $(service_in_files:.service.in=.service)
> 
> if HAVE_SYSTEMD_USER_UNITS_DIR
>-systemd_userdir = ${prefix}/lib/systemd/user
>+systemd_userdir = @systemd_user_units_dir@
> systemd_user_in_files = gvfs-udisks2-volume-monitor.service.in
> systemd_user_DATA = $(systemd_user_in_files:.service.in=.service)
> endif

Created attachment 318897 [details] [review]
Fix typo in previous patch

Review of attachment 318857 [details] [review]:

Thanks, looks much better, but still there are some issues...

I realized that would be good to treat CLEANFILES similar as EXTRA_DIST also (put this in a same if-statement with EXTRA_DIST).

::: configure.ac
@@ +266,3 @@
+
+sustemd_user_dir=${prefix}/lib/systemd/user
+AC_ARG_WITH([systemd-userunitdir],

systemd-userunitdir -> systemduserunitdir

@@ +267,3 @@
+sustemd_user_dir=${prefix}/lib/systemd/user
+AC_ARG_WITH([systemd-userunitdir],
+        [AS_HELP_STRING([--with-systemd-userunitdir=PATH],

--with-systemd-userunitdir -> --with-systemduserunitdir

@@ +270,3 @@
+            [enable or disable systemd user units support, defaults to no])], 
+        [systemd_user_units=$withval],
+        [msg_systemd_user_units=no])

should be enabled by default

@@ +273,3 @@
+
+if test "x$systemd_user_units" != "xno"; then
+    PKG_CHECK_MODULES([SYSTEMD], [systemd >= 219],

Why you use version 219? I suppose that user units were introduced much earlier. Maybe I would remove this check at all since it is not critical to install those file anyway, or do not check version, or find appropriate version.

::: daemon/Makefile.am
@@ +44,3 @@
 systemd_user_in_files = gvfs-daemon.service.in
 systemd_user_DATA = gvfs-daemon.service
+endif

EXTRA_DIST needs to be treated also...

@@ +124,2 @@
 mount_in_files += afc.mount.in
+/if USE_AFC

unwanted modification

::: monitor/goa/Makefile.am
@@ +36,3 @@
 service_DATA = $(service_in_files:.service.in=.service)
 
+if HAVE_SYSTEMD_USER_UNIT_DIR

HAVE_SYSTEMD_USER_UNIT_DIR -> HAVE_SYSTEMD_USER_UNITS_DIR

@@ +45,3 @@
 	$(AM_V_GEN) $(SED) -e "s|\@libexecdir\@|$(libexecdir)|" $< > $@
 
+if HAVE_SYSTEMD_USER_UNIT_DIR

dtto

@@ +63,3 @@
 	$(NULL)
+
+if HAVE_SYSTEMD_USER_UNIT_DIR

dtto

::: monitor/gphoto2/Makefile.am
@@ +105,1 @@
 	$(systemd_user_in_files) \

you should remove this line

::: monitor/udisks2/Makefile.am
@@ +57,3 @@
 $(service_DATA): $(service_in_files) Makefile
 	$(AM_V_GEN) $(SED) -e "s|\@libexecdir\@|$(libexecdir)|" $< > $@
+if HAVE_SYSTEMD_USER_UNITS_DIR

add empty line before

Review of attachment 318897 [details] [review]:

I meant to have this patch as predecessor of the original one. I realized later that --with-systemduserunitdir allows specifying custom dir (would be good to mention it in the commit message). In that case you may merge the patches, sorry for inconveniences.

::: monitor/goa/Makefile.am
@@ +38,2 @@
 if HAVE_SYSTEMD_USER_UNIT_DIR
+systemd_userdir = @systemd_user_inits_dir@

@systemd_user_inits_dir@ -> @systemd_user_units_dir@

Thanks for criticism. 
> Why you use version 219?
Hmm, I think it was introduced in that version. A searching in the web shows that this was before version 188, but I think that is safely to use version 206, which have changed behavior, according to arch-wiki: https://wiki.archlinux.org/index.php/Systemd/User

Created attachment 319153 [details] [review]
Possibility to not install systemd stuff on non-systemd distros

Created attachment 319188 [details] [review]
Switched to enabled by default systemd-userunitdir

Sorry, I forgot to change to enabled by default systemd-userunitdir option in configure, so I uploaded fixed version of the patch...

Created attachment 319189 [details] [review]
Fixed also commit message.

Ah, also I'll edited commit message. Sorry again.

Review of attachment 319189 [details] [review]:

Thanks for updated version though there are still some issues, have you ever tried your patch?

Please take a look, how to write commit messages:
https://wiki.gnome.org/Git/CommitMessages

::: configure.ac
@@ +265,3 @@
+dnl ************************************
+
+sustemd_user_dir=${prefix}/lib/systemd/user

sustemd?

Apart from that there are used following variables, which is pretty confusing and needless:
systemd_user_dir
systemd_user_units
systemd_user_units_dir

Apart from that the following code doesn't work for all possible cases, so please take a look on another projects and try to do it similarly, e.g. 
https://git.gnome.org/browse/evolution-data-server/patch/?id=51a37f76bec193847f0ec2c5df4aa879947a47c2

From the mentioned commit I also realized that we should include those files in EXTRA_DIST, otherwise tarball is corrupted if it is made on system without systemd support. Sorry that I didn't realized this earlier.

(I realized also that we can do it much simpler. We can set systemd_userdir in configure.ac and also CLEANFILES doesn't have to be in the if-statements even though I told you something else last time, I am really sorry for that.)

Could you please fix it again and test all possible cases before attaching? I hope this is last iteration...

::: monitor/udisks2/Makefile.am
@@ +74,2 @@
 	$(systemd_user_DATA) \
 	$(NULL)

endif

Created attachment 319523 [details] [review]
Add configure option --with-systemdusserunitdir

> ...have you ever tried your patch?
Thanks a lot again. I'll reworked the patch now, and yes, I was tested it (except installation with systemd because it is not on my system -- I use Slackware) for and seems all works fine. But if you want, I can download any systemd distro and try that patch under a VM.

Review of attachment 319523 [details] [review]:

Thanks, though there are still some issues, but I am going to fix them myself...

git am returns "Patch format detection failed." Please make patches using "git format-patch" next time:
https://wiki.gnome.org/Git/Developers#Contributing_patches

::: configure.ac
@@ +277,3 @@
+fi
+
+AM_CONDITIONAL([USE_SYSTEMD_USER_UNITS], [test "x$systemd_user_dir" != "xno"])

This may succeed also if systemd was not found...

::: daemon/Makefile.am
@@ +44,1 @@
 systemd_user_in_files = gvfs-daemon.service.in

This line have to be outside of the if-statement to be sure that those files will be placed in the tarball.

Created attachment 319556 [details] [review]
build: Add --with-systemduserunitdir option

commit 5dcf92a112a010e67b6fc58dbd0b39cec8e823d6

Thanks you for fix my mistakes. I will do patches more accurate next time.