All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] systemd: add support to manage user units
@ 2016-09-07  9:22 Chen Qi
  2016-09-07  9:22 ` [PATCH 1/3] systemd-systemctl: add option to manage user services Chen Qi
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Chen Qi @ 2016-09-07  9:22 UTC (permalink / raw)
  To: openembedded-core

The following changes since commit 55bb6816aca39bfa25d4f7e2158a57a5f0ac1cca:

  oeqa.buildperf: correct globalres time format (2016-09-06 10:24:00 +0100)

are available in the git repository at:

  git://git.openembedded.org/openembedded-core-contrib ChenQi/systemd-user-units
  http://cgit.openembedded.org/cgit.cgi/openembedded-core-contrib/log/?h=ChenQi/systemd-user-units

Chen Qi (3):
  systemd-systemctl: add option to manage user services
  systemd.bbclass: add support to manage user services
  pulseaudio: fix to manage user services corretly

 meta/classes/systemd.bbclass                       | 17 ++++++--
 .../systemd/systemd-systemctl/systemctl            | 45 ++++++++++++++--------
 meta/recipes-multimedia/pulseaudio/pulseaudio.inc  |  4 +-
 3 files changed, 43 insertions(+), 23 deletions(-)

-- 
1.9.1



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] systemd-systemctl: add option to manage user services
  2016-09-07  9:22 [PATCH 0/3] systemd: add support to manage user units Chen Qi
@ 2016-09-07  9:22 ` Chen Qi
  2016-09-07  9:22 ` [PATCH 2/3] systemd.bbclass: add support " Chen Qi
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Chen Qi @ 2016-09-07  9:22 UTC (permalink / raw)
  To: openembedded-core

Add '--global' option to our own systemctl script to manage user services.

[YOCTO #7800]

Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
---
 .../systemd/systemd-systemctl/systemctl            | 45 ++++++++++++++--------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/meta/recipes-core/systemd/systemd-systemctl/systemctl b/meta/recipes-core/systemd/systemd-systemctl/systemctl
index efad14c..17a7277 100755
--- a/meta/recipes-core/systemd/systemd-systemctl/systemctl
+++ b/meta/recipes-core/systemd/systemd-systemctl/systemctl
@@ -2,7 +2,8 @@
 echo "Started $0 $*"
 
 ROOT=
-
+USER_SERVICE=no
+location=system
 # parse command line params
 action=
 while [ $# != 0 ]; do
@@ -46,6 +47,11 @@ while [ $# != 0 ]; do
 			cmd_args="0"
 			shift
 			;;
+		--global)
+			USER_SERVICE=yes
+			cmd_args="0"
+			shift
+			;;
 		*)
 			if [ "$cmd_args" = "1" ]; then
 				services="$services $opt" 
@@ -57,8 +63,13 @@ while [ $# != 0 ]; do
 			;;
 	esac
 done
+
+if [ "$USER_SERVICE" = "yes" ]; then
+	location=user
+fi
+
 if [ "$action" = "preset" -a "$service_file" = "" ]; then
-	services=$(for f in `find $ROOT/etc/systemd/system $ROOT/lib/systemd/system $ROOT/usr/lib/systemd/system -type f 2>1`; do basename $f; done)
+	services=$(for f in `find $ROOT/etc/systemd/$location $ROOT/lib/systemd/$location $ROOT/usr/lib/systemd/$location -type f 2>1`; do basename $f; done)
 	services="$services $opt"
 	presetall=1
 fi
@@ -68,10 +79,10 @@ for service in $services; do
 		action="preset"
 	fi
 	if [ "$action" = "mask" ]; then
-		if [ ! -d $ROOT/etc/systemd/system/ ]; then
-			mkdir -p $ROOT/etc/systemd/system/
+		if [ ! -d $ROOT/etc/systemd/$location/ ]; then
+			mkdir -p $ROOT/etc/systemd/$location/
 		fi
-		cmd="ln -s /dev/null $ROOT/etc/systemd/system/$service"
+		cmd="ln -s /dev/null $ROOT/etc/systemd/$location/$service"
 		echo "$cmd"
 		$cmd
 		exit 0
@@ -92,9 +103,9 @@ for service in $services; do
 	fi
 
 	# find service file
-	for p in $ROOT/etc/systemd/system \
-		 $ROOT/lib/systemd/system \
-		 $ROOT/usr/lib/systemd/system; do
+	for p in $ROOT/etc/systemd/$location \
+		 $ROOT/lib/systemd/$location \
+		 $ROOT/usr/lib/systemd/$location; do
 		if [ -e $p/$service_base_file ]; then
 			service_file=$p/$service_base_file
 			service_file=${service_file##$ROOT}
@@ -151,18 +162,18 @@ for service in $services; do
 						enable_service=$(echo $service | sed "s/@/@$(echo $default_instance | sed 's/\\/\\\\/g')/")
 					fi
 				fi
-				mkdir -p $ROOT/etc/systemd/system/$r.$suffix
-				ln -s $service_file $ROOT/etc/systemd/system/$r.$suffix/$enable_service
+				mkdir -p $ROOT/etc/systemd/$location/$r.$suffix
+				ln -s $service_file $ROOT/etc/systemd/$location/$r.$suffix/$enable_service
 				echo "Enabled $enable_service for $r."
 			else
 				if [ "$service_template" = true -a "$instance_specified" = false ]; then
-					disable_service="$ROOT/etc/systemd/system/$r.$suffix/`echo $service | sed 's/@/@*/'`"
+					disable_service="$ROOT/etc/systemd/$location/$r.$suffix/`echo $service | sed 's/@/@*/'`"
 				else
-					disable_service="$ROOT/etc/systemd/system/$r.$suffix/$service"
+					disable_service="$ROOT/etc/systemd/$location/$r.$suffix/$service"
 				fi
 				rm -f $disable_service
-				[ -d $ROOT/etc/systemd/system/$r.$suffix ] && rmdir --ignore-fail-on-non-empty -p $ROOT/etc/systemd/system/$r.$suffix
-				echo "Disabled ${disable_service##$ROOT/etc/systemd/system/$r.$suffix/} for $r."
+				[ -d $ROOT/etc/systemd/$location/$r.$suffix ] && rmdir --ignore-fail-on-non-empty -p $ROOT/etc/systemd/$location/$r.$suffix
+				echo "Disabled ${disable_service##$ROOT/etc/systemd/$location/$r.$suffix/} for $r."
 			fi
 		done
 	done
@@ -174,11 +185,11 @@ for service in $services; do
 
 	for r in $alias; do
 		if [ "$action" = "enable" ]; then
-			mkdir -p $ROOT/etc/systemd/system
-			ln -s $service_file $ROOT/etc/systemd/system/$r
+			mkdir -p $ROOT/etc/systemd/$location
+			ln -s $service_file $ROOT/etc/systemd/$location/$r
 			echo "Enabled $service for $alias."
 		else
-			rm -f $ROOT/etc/systemd/system/$r
+			rm -f $ROOT/etc/systemd/$location/$r
 			echo "Disabled $service for $alias."
 		fi
 	done
-- 
1.9.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] systemd.bbclass: add support to manage user services
  2016-09-07  9:22 [PATCH 0/3] systemd: add support to manage user units Chen Qi
  2016-09-07  9:22 ` [PATCH 1/3] systemd-systemctl: add option to manage user services Chen Qi
@ 2016-09-07  9:22 ` Chen Qi
  2016-09-07 12:14   ` Pau Espin Pedrol
  2016-09-07  9:22 ` [PATCH 3/3] pulseaudio: fix to manage user services corretly Chen Qi
  2016-09-07  9:43 ` [PATCH 0/3] systemd: add support to manage user units Jérémy Rosen
  3 siblings, 1 reply; 13+ messages in thread
From: Chen Qi @ 2016-09-07  9:22 UTC (permalink / raw)
  To: openembedded-core

Add new variable SYSTEMD_USER_SERVICE and SYSTEM_USER_AUTO_ENABLE
to manage user services. Their usage is like SYSTEMD_SERVICE and
SYSTEMD_AUTO_ENABLE.

[YOCTO #7800]

Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
---
 meta/classes/systemd.bbclass | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/meta/classes/systemd.bbclass b/meta/classes/systemd.bbclass
index db7873f..78cec97 100644
--- a/meta/classes/systemd.bbclass
+++ b/meta/classes/systemd.bbclass
@@ -7,6 +7,7 @@ SYSTEMD_PACKAGES_class-nativesdk ?= ""
 
 # Whether to enable or disable the services on installation.
 SYSTEMD_AUTO_ENABLE ??= "enable"
+SYSTEMD_USER_AUTO_ENABLE ??= "enable"
 
 # This class will be included in any recipe that supports systemd init scripts,
 # even if systemd is not in DISTRO_FEATURES.  As such don't make any changes
@@ -30,10 +31,14 @@ fi
 
 if type systemctl >/dev/null 2>/dev/null; then
 	systemctl $OPTS ${SYSTEMD_AUTO_ENABLE} ${SYSTEMD_SERVICE}
+	systemctl $OPTS --global ${SYSTEMD_USER_AUTO_ENABLE} ${SYSTEMD_USER_SERVICE}
 
 	if [ -z "$D" -a "${SYSTEMD_AUTO_ENABLE}" = "enable" ]; then
 		systemctl restart ${SYSTEMD_SERVICE}
 	fi
+	if [ -z "$D" -a "${SYSTEMD_USER_AUTO_ENABLE}" = "enable" ]; then
+		systemctl --global restart ${SYSTEMD_USER_SERVICE}
+	fi
 fi
 }
 
@@ -47,9 +52,11 @@ fi
 if type systemctl >/dev/null 2>/dev/null; then
 	if [ -z "$D" ]; then
 		systemctl stop ${SYSTEMD_SERVICE}
+		systemctl --global stop ${SYSTEMD_USER_SERVICE}
 	fi
 
 	systemctl $OPTS disable ${SYSTEMD_SERVICE}
+	systemctl $OPTS --global disable ${SYSTEMD_USER_SERVICE}
 fi
 }
 
@@ -139,12 +146,14 @@ python systemd_populate_packages() {
     def systemd_check_services():
         searchpaths = [oe.path.join(d.getVar("sysconfdir", True), "systemd", "system"),]
         searchpaths.append(d.getVar("systemd_system_unitdir", True))
+        searchpaths.append(oe.path.join(d.getVar("sysconfdir", True), "systemd", "user"))
+        searchpaths.append(d.getVar("systemd_user_unitdir", True))
         systemd_packages = d.getVar('SYSTEMD_PACKAGES', True)
 
         keys = 'Also'
         # scan for all in SYSTEMD_SERVICE[]
         for pkg_systemd in systemd_packages.split():
-            for service in get_package_var(d, 'SYSTEMD_SERVICE', pkg_systemd).split():
+            for service in (get_package_var(d, 'SYSTEMD_SERVICE', pkg_systemd) + get_package_var(d, 'SYSTEMD_USER_SERVICE', pkg_systemd)).split():
                 path_found = ''
 
                 # Deal with adding, for example, 'ifplugd@eth0.service' from
@@ -165,14 +174,14 @@ python systemd_populate_packages() {
                 if path_found != '':
                     systemd_add_files_and_parse(pkg_systemd, path_found, service, keys)
                 else:
-                    raise bb.build.FuncFailed("SYSTEMD_SERVICE_%s value %s does not exist" % \
-                        (pkg_systemd, service))
+                    raise bb.build.FuncFailed("SYSTEMD_SERVICE_%s or SYSTEMD_USER_SERVICE_%s value %s does not exist" % \
+                        (pkg_systemd, pkg_systemd, service))
 
     # Run all modifications once when creating package
     if os.path.exists(d.getVar("D", True)):
         for pkg in d.getVar('SYSTEMD_PACKAGES', True).split():
             systemd_check_package(pkg)
-            if d.getVar('SYSTEMD_SERVICE_' + pkg, True):
+            if d.getVar('SYSTEMD_SERVICE_' + pkg, True) or d.getVar('SYSTEMD_USER_SERVICE_' + pkg, True):
                 systemd_generate_package_scripts(pkg)
         systemd_check_services()
 }
-- 
1.9.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] pulseaudio: fix to manage user services corretly
  2016-09-07  9:22 [PATCH 0/3] systemd: add support to manage user units Chen Qi
  2016-09-07  9:22 ` [PATCH 1/3] systemd-systemctl: add option to manage user services Chen Qi
  2016-09-07  9:22 ` [PATCH 2/3] systemd.bbclass: add support " Chen Qi
@ 2016-09-07  9:22 ` Chen Qi
  2016-09-07 10:29   ` Pau Espin Pedrol
  2016-09-07  9:43 ` [PATCH 0/3] systemd: add support to manage user units Jérémy Rosen
  3 siblings, 1 reply; 13+ messages in thread
From: Chen Qi @ 2016-09-07  9:22 UTC (permalink / raw)
  To: openembedded-core

Make use of the new SYSTEMD_USER_SERVICE variable added in systemd.bbclass
to manage user services in pulseaudio-server package.

Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
---
 meta/recipes-multimedia/pulseaudio/pulseaudio.inc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/meta/recipes-multimedia/pulseaudio/pulseaudio.inc b/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
index 6ed79ef..f3754d7 100644
--- a/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
+++ b/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
@@ -124,8 +124,8 @@ FILES_${PN}-conf = "${sysconfdir}"
 FILES_${PN}-bin += "${sysconfdir}/default/volatiles/volatiles.04_pulse"
 FILES_${PN}-server = "${bindir}/pulseaudio ${bindir}/start-* ${sysconfdir} ${bindir}/pactl */udev/rules.d/*.rules */*/udev/rules.d/*.rules ${systemd_user_unitdir}/*"
 
-#SYSTEMD_PACKAGES = "${PN}-server"
-SYSTEMD_SERVICE_${PN}-server = "pulseaudio.service"
+SYSTEMD_PACKAGES = "${PN}-server"
+SYSTEMD_USER_SERVICE_${PN}-server = "pulseaudio.service pulseaudio.socket"
 
 FILES_${PN}-misc = "${bindir}/* ${libdir}/pulseaudio/libpulsedsp.so"
 
-- 
1.9.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/3] systemd: add support to manage user units
  2016-09-07  9:22 [PATCH 0/3] systemd: add support to manage user units Chen Qi
                   ` (2 preceding siblings ...)
  2016-09-07  9:22 ` [PATCH 3/3] pulseaudio: fix to manage user services corretly Chen Qi
@ 2016-09-07  9:43 ` Jérémy Rosen
  3 siblings, 0 replies; 13+ messages in thread
From: Jérémy Rosen @ 2016-09-07  9:43 UTC (permalink / raw)
  To: openembedded-core

It's probably worth updating the documentation too... this is a usefull 
feature and it deserves the exposure


Regards

Jeremy Rosen


On 07/09/2016 11:22, Chen Qi wrote:
> The following changes since commit 55bb6816aca39bfa25d4f7e2158a57a5f0ac1cca:
>
>    oeqa.buildperf: correct globalres time format (2016-09-06 10:24:00 +0100)
>
> are available in the git repository at:
>
>    git://git.openembedded.org/openembedded-core-contrib ChenQi/systemd-user-units
>    http://cgit.openembedded.org/cgit.cgi/openembedded-core-contrib/log/?h=ChenQi/systemd-user-units
>
> Chen Qi (3):
>    systemd-systemctl: add option to manage user services
>    systemd.bbclass: add support to manage user services
>    pulseaudio: fix to manage user services corretly
>
>   meta/classes/systemd.bbclass                       | 17 ++++++--
>   .../systemd/systemd-systemctl/systemctl            | 45 ++++++++++++++--------
>   meta/recipes-multimedia/pulseaudio/pulseaudio.inc  |  4 +-
>   3 files changed, 43 insertions(+), 23 deletions(-)
>



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] pulseaudio: fix to manage user services corretly
  2016-09-07  9:22 ` [PATCH 3/3] pulseaudio: fix to manage user services corretly Chen Qi
@ 2016-09-07 10:29   ` Pau Espin Pedrol
  2016-09-08  6:34     ` ChenQi
  0 siblings, 1 reply; 13+ messages in thread
From: Pau Espin Pedrol @ 2016-09-07 10:29 UTC (permalink / raw)
  To: Chen Qi; +Cc: OE-core

[-- Attachment #1: Type: text/plain, Size: 1659 bytes --]

Pau Espin Pedrol

2016-09-07 11:22 GMT+02:00 Chen Qi <Qi.Chen@windriver.com>:

> Make use of the new SYSTEMD_USER_SERVICE variable added in systemd.bbclass
> to manage user services in pulseaudio-server package.
>
> Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
> ---
>  meta/recipes-multimedia/pulseaudio/pulseaudio.inc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
> b/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
> index 6ed79ef..f3754d7 100644
> --- a/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
> +++ b/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
> @@ -124,8 +124,8 @@ FILES_${PN}-conf = "${sysconfdir}"
>  FILES_${PN}-bin += "${sysconfdir}/default/volatiles/volatiles.04_pulse"
>  FILES_${PN}-server = "${bindir}/pulseaudio ${bindir}/start-*
> ${sysconfdir} ${bindir}/pactl */udev/rules.d/*.rules
> */*/udev/rules.d/*.rules ${systemd_user_unitdir}/*"
>
> -#SYSTEMD_PACKAGES = "${PN}-server"
> -SYSTEMD_SERVICE_${PN}-server = "pulseaudio.service"
> +SYSTEMD_PACKAGES = "${PN}-server"
> +SYSTEMD_USER_SERVICE_${PN}-server = "pulseaudio.service
> pulseaudio.socket"
>
> I think specifying "pulseaudio.socket" for SYSTEMD_USER_SERVICE_${PN}-server
should be enough, systemd.bbclass is going to add the .service file afair.


>  FILES_${PN}-misc = "${bindir}/* ${libdir}/pulseaudio/libpulsedsp.so"
>
> --
> 1.9.1
>
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>

[-- Attachment #2: Type: text/html, Size: 2726 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] systemd.bbclass: add support to manage user services
  2016-09-07  9:22 ` [PATCH 2/3] systemd.bbclass: add support " Chen Qi
@ 2016-09-07 12:14   ` Pau Espin Pedrol
  2016-09-08  2:33     ` ChenQi
  0 siblings, 1 reply; 13+ messages in thread
From: Pau Espin Pedrol @ 2016-09-07 12:14 UTC (permalink / raw)
  To: Chen Qi; +Cc: OE-core

[-- Attachment #1: Type: text/plain, Size: 5157 bytes --]

2016-09-07 11:22 GMT+02:00 Chen Qi <Qi.Chen@windriver.com>:

> Add new variable SYSTEMD_USER_SERVICE and SYSTEM_USER_AUTO_ENABLE
> to manage user services. Their usage is like SYSTEMD_SERVICE and
> SYSTEMD_AUTO_ENABLE.
>
> [YOCTO #7800]
>
> Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
> ---
>  meta/classes/systemd.bbclass | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/meta/classes/systemd.bbclass b/meta/classes/systemd.bbclass
> index db7873f..78cec97 100644
> --- a/meta/classes/systemd.bbclass
> +++ b/meta/classes/systemd.bbclass
> @@ -7,6 +7,7 @@ SYSTEMD_PACKAGES_class-nativesdk ?= ""
>
>  # Whether to enable or disable the services on installation.
>  SYSTEMD_AUTO_ENABLE ??= "enable"
> +SYSTEMD_USER_AUTO_ENABLE ??= "enable"
>
>  # This class will be included in any recipe that supports systemd init
> scripts,
>  # even if systemd is not in DISTRO_FEATURES.  As such don't make any
> changes
> @@ -30,10 +31,14 @@ fi
>
>  if type systemctl >/dev/null 2>/dev/null; then
>         systemctl $OPTS ${SYSTEMD_AUTO_ENABLE} ${SYSTEMD_SERVICE}
> +       systemctl $OPTS --global ${SYSTEMD_USER_AUTO_ENABLE}
> ${SYSTEMD_USER_SERVICE}
>

I'm not sure having these 2 systemctl being executed together everytime is
a good idea. What if a recipe has a user service and no system service? We
are calling the first one with an empty system service? Or we are may be
enabling a systemd system service which should not be enabled according to
the recipe? I have the feeling this kind of cases are not being catch in
here and other pkg scripts in this commit.

It is far from perfect, but in case you didn't, you may want to have a look
at my initial/previous commit to try to fix the issue:
https://www.mail-archive.com/openembedded-devel@lists.openembedded.org/msg42187.html


>
>         if [ -z "$D" -a "${SYSTEMD_AUTO_ENABLE}" = "enable" ]; then
>                 systemctl restart ${SYSTEMD_SERVICE}
>         fi
> +       if [ -z "$D" -a "${SYSTEMD_USER_AUTO_ENABLE}" = "enable" ]; then
> +               systemctl --global restart ${SYSTEMD_USER_SERVICE}
> +       fi
>  fi
>  }
>
> @@ -47,9 +52,11 @@ fi
>  if type systemctl >/dev/null 2>/dev/null; then
>         if [ -z "$D" ]; then
>                 systemctl stop ${SYSTEMD_SERVICE}
> +               systemctl --global stop ${SYSTEMD_USER_SERVICE}
>

I think this is not gonna work, you cannot call --global with stop afair.
I'm not sure which is the good solution for this though, but you should
ideally go through all systemd user sessions and call "systemctl --user
stop". No idea if that's actually easily feasible.


>         fi
>
>         systemctl $OPTS disable ${SYSTEMD_SERVICE}
> +       systemctl $OPTS --global disable ${SYSTEMD_USER_SERVICE}
>  fi
>  }
>
> @@ -139,12 +146,14 @@ python systemd_populate_packages() {
>      def systemd_check_services():
>          searchpaths = [oe.path.join(d.getVar("sysconfdir", True),
> "systemd", "system"),]
>          searchpaths.append(d.getVar("systemd_system_unitdir", True))
> +        searchpaths.append(oe.path.join(d.getVar("sysconfdir", True),
> "systemd", "user"))
> +        searchpaths.append(d.getVar("systemd_user_unitdir", True))
>          systemd_packages = d.getVar('SYSTEMD_PACKAGES', True)
>
>          keys = 'Also'
>          # scan for all in SYSTEMD_SERVICE[]
>          for pkg_systemd in systemd_packages.split():
> -            for service in get_package_var(d, 'SYSTEMD_SERVICE',
> pkg_systemd).split():
> +            for service in (get_package_var(d, 'SYSTEMD_SERVICE',
> pkg_systemd) + get_package_var(d, 'SYSTEMD_USER_SERVICE',
> pkg_systemd)).split():
>                  path_found = ''
>
>                  # Deal with adding, for example, 'ifplugd@eth0.service'
> from
> @@ -165,14 +174,14 @@ python systemd_populate_packages() {
>                  if path_found != '':
>                      systemd_add_files_and_parse(pkg_systemd, path_found,
> service, keys)
>                  else:
> -                    raise bb.build.FuncFailed("SYSTEMD_SERVICE_%s value
> %s does not exist" % \
> -                        (pkg_systemd, service))
> +                    raise bb.build.FuncFailed("SYSTEMD_SERVICE_%s or
> SYSTEMD_USER_SERVICE_%s value %s does not exist" % \
> +                        (pkg_systemd, pkg_systemd, service))
>
>      # Run all modifications once when creating package
>      if os.path.exists(d.getVar("D", True)):
>          for pkg in d.getVar('SYSTEMD_PACKAGES', True).split():
>              systemd_check_package(pkg)
> -            if d.getVar('SYSTEMD_SERVICE_' + pkg, True):
> +            if d.getVar('SYSTEMD_SERVICE_' + pkg, True) or
> d.getVar('SYSTEMD_USER_SERVICE_' + pkg, True):
>                  systemd_generate_package_scripts(pkg)
>          systemd_check_services()
>  }
> --
> 1.9.1
>
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>

[-- Attachment #2: Type: text/html, Size: 7044 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] systemd.bbclass: add support to manage user services
  2016-09-07 12:14   ` Pau Espin Pedrol
@ 2016-09-08  2:33     ` ChenQi
  2016-09-09 17:48       ` Pau Espin Pedrol
  0 siblings, 1 reply; 13+ messages in thread
From: ChenQi @ 2016-09-08  2:33 UTC (permalink / raw)
  To: Pau Espin Pedrol; +Cc: OE-core

[-- Attachment #1: Type: text/plain, Size: 6513 bytes --]

On 09/07/2016 08:14 PM, Pau Espin Pedrol wrote:
>
> 2016-09-07 11:22 GMT+02:00 Chen Qi <Qi.Chen@windriver.com 
> <mailto:Qi.Chen@windriver.com>>:
>
>     Add new variable SYSTEMD_USER_SERVICE and SYSTEM_USER_AUTO_ENABLE
>     to manage user services. Their usage is like SYSTEMD_SERVICE and
>     SYSTEMD_AUTO_ENABLE.
>
>     [YOCTO #7800]
>
>     Signed-off-by: Chen Qi <Qi.Chen@windriver.com
>     <mailto:Qi.Chen@windriver.com>>
>     ---
>      meta/classes/systemd.bbclass | 17 +++++++++++++----
>      1 file changed, 13 insertions(+), 4 deletions(-)
>
>     diff --git a/meta/classes/systemd.bbclass
>     b/meta/classes/systemd.bbclass
>     index db7873f..78cec97 100644
>     --- a/meta/classes/systemd.bbclass
>     +++ b/meta/classes/systemd.bbclass
>     @@ -7,6 +7,7 @@ SYSTEMD_PACKAGES_class-nativesdk ?= ""
>
>      # Whether to enable or disable the services on installation.
>      SYSTEMD_AUTO_ENABLE ??= "enable"
>     +SYSTEMD_USER_AUTO_ENABLE ??= "enable"
>
>      # This class will be included in any recipe that supports systemd
>     init scripts,
>      # even if systemd is not in DISTRO_FEATURES.  As such don't make
>     any changes
>     @@ -30,10 +31,14 @@ fi
>
>      if type systemctl >/dev/null 2>/dev/null; then
>             systemctl $OPTS ${SYSTEMD_AUTO_ENABLE} ${SYSTEMD_SERVICE}
>     +       systemctl $OPTS --global ${SYSTEMD_USER_AUTO_ENABLE}
>     ${SYSTEMD_USER_SERVICE}
>
>
> I'm not sure having these 2 systemctl being executed together 
> everytime is a good idea. What if a recipe has a user service and no 
> system service?

Hi Pau Espin Pedrol,

Thanks for your review.

The postinstall script runs successfully with expected result at rootfs 
time. But your question reminds me of the situation of the on-target 
install/remove situation. I think I'll need to make a new patch to make 
sure things work in both situations.

The key point here is that 'systemctl' at rootfs time is a shell script 
written by ourselves and 'systemctl' on target is that provided by systemd.

> We are calling the first one with an empty system service? Or we are 
> may be enabling a systemd system service which should not be enabled 
> according to the recipe? I have the feeling this kind of cases are not 
> being catch in here and other pkg scripts in this commit.
>
> It is far from perfect, but in case you didn't, you may want to have a 
> look at my initial/previous commit to try to fix the issue: 
> https://www.mail-archive.com/openembedded-devel@lists.openembedded.org/msg42187.html
>
>
>             if [ -z "$D" -a "${SYSTEMD_AUTO_ENABLE}" = "enable" ]; then
>                     systemctl restart ${SYSTEMD_SERVICE}
>             fi
>     +       if [ -z "$D" -a "${SYSTEMD_USER_AUTO_ENABLE}" = "enable"
>     ]; then
>     +               systemctl --global restart ${SYSTEMD_USER_SERVICE}
>     +       fi
>      fi
>      }
>
>     @@ -47,9 +52,11 @@ fi
>      if type systemctl >/dev/null 2>/dev/null; then
>             if [ -z "$D" ]; then
>                     systemctl stop ${SYSTEMD_SERVICE}
>     +               systemctl --global stop ${SYSTEMD_USER_SERVICE}
>
>
> I think this is not gonna work, you cannot call --global with stop 
> afair. I'm not sure which is the good solution for this though, but 
> you should ideally go through all systemd user sessions and call 
> "systemctl --user stop". No idea if that's actually easily feasible.

The service is enabled and started with '--global' option, and stopping 
it with '--global' option seems reasonable to me. If there's some 
special case, let's look into it then.

Best Regards,
Chen Qi

>             fi
>
>             systemctl $OPTS disable ${SYSTEMD_SERVICE}
>     +       systemctl $OPTS --global disable ${SYSTEMD_USER_SERVICE}
>      fi
>      }
>
>     @@ -139,12 +146,14 @@ python systemd_populate_packages() {
>          def systemd_check_services():
>              searchpaths = [oe.path.join(d.getVar("sysconfdir", True),
>     "systemd", "system"),]
>              searchpaths.append(d.getVar("systemd_system_unitdir", True))
>     +        searchpaths.append(oe.path.join(d.getVar("sysconfdir",
>     True), "systemd", "user"))
>     +        searchpaths.append(d.getVar("systemd_user_unitdir", True))
>              systemd_packages = d.getVar('SYSTEMD_PACKAGES', True)
>
>              keys = 'Also'
>              # scan for all in SYSTEMD_SERVICE[]
>              for pkg_systemd in systemd_packages.split():
>     -            for service in get_package_var(d, 'SYSTEMD_SERVICE',
>     pkg_systemd).split():
>     +            for service in (get_package_var(d, 'SYSTEMD_SERVICE',
>     pkg_systemd) + get_package_var(d, 'SYSTEMD_USER_SERVICE',
>     pkg_systemd)).split():
>                      path_found = ''
>
>                      # Deal with adding, for example,
>     'ifplugd@eth0.service' from
>     @@ -165,14 +174,14 @@ python systemd_populate_packages() {
>                      if path_found != '':
>                          systemd_add_files_and_parse(pkg_systemd,
>     path_found, service, keys)
>                      else:
>     -                    raise bb.build.FuncFailed("SYSTEMD_SERVICE_%s
>     value %s does not exist" % \
>     -                        (pkg_systemd, service))
>     +                    raise bb.build.FuncFailed("SYSTEMD_SERVICE_%s
>     or SYSTEMD_USER_SERVICE_%s value %s does not exist" % \
>     +                        (pkg_systemd, pkg_systemd, service))
>
>          # Run all modifications once when creating package
>          if os.path.exists(d.getVar("D", True)):
>              for pkg in d.getVar('SYSTEMD_PACKAGES', True).split():
>                  systemd_check_package(pkg)
>     -            if d.getVar('SYSTEMD_SERVICE_' + pkg, True):
>     +            if d.getVar('SYSTEMD_SERVICE_' + pkg, True) or
>     d.getVar('SYSTEMD_USER_SERVICE_' + pkg, True):
>                      systemd_generate_package_scripts(pkg)
>              systemd_check_services()
>      }
>     --
>     1.9.1
>
>     --
>     _______________________________________________
>     Openembedded-core mailing list
>     Openembedded-core@lists.openembedded.org
>     <mailto:Openembedded-core@lists.openembedded.org>
>     http://lists.openembedded.org/mailman/listinfo/openembedded-core
>     <http://lists.openembedded.org/mailman/listinfo/openembedded-core>
>
>


[-- Attachment #2: Type: text/html, Size: 11249 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] pulseaudio: fix to manage user services corretly
  2016-09-07 10:29   ` Pau Espin Pedrol
@ 2016-09-08  6:34     ` ChenQi
  2016-09-08 11:22       ` Tanu Kaskinen
  0 siblings, 1 reply; 13+ messages in thread
From: ChenQi @ 2016-09-08  6:34 UTC (permalink / raw)
  To: Pau Espin Pedrol; +Cc: OE-core

[-- Attachment #1: Type: text/plain, Size: 2588 bytes --]

On 09/07/2016 06:29 PM, Pau Espin Pedrol wrote:
>
>
> Pau Espin Pedrol
>
> 2016-09-07 11:22 GMT+02:00 Chen Qi <Qi.Chen@windriver.com 
> <mailto:Qi.Chen@windriver.com>>:
>
>     Make use of the new SYSTEMD_USER_SERVICE variable added in
>     systemd.bbclass
>     to manage user services in pulseaudio-server package.
>
>     Signed-off-by: Chen Qi <Qi.Chen@windriver.com
>     <mailto:Qi.Chen@windriver.com>>
>     ---
>      meta/recipes-multimedia/pulseaudio/pulseaudio.inc | 4 ++--
>      1 file changed, 2 insertions(+), 2 deletions(-)
>
>     diff --git a/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
>     b/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
>     index 6ed79ef..f3754d7 100644
>     --- a/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
>     +++ b/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
>     @@ -124,8 +124,8 @@ FILES_${PN}-conf = "${sysconfdir}"
>      FILES_${PN}-bin +=
>     "${sysconfdir}/default/volatiles/volatiles.04_pulse"
>      FILES_${PN}-server = "${bindir}/pulseaudio ${bindir}/start-*
>     ${sysconfdir} ${bindir}/pactl */udev/rules.d/*.rules
>     */*/udev/rules.d/*.rules ${systemd_user_unitdir}/*"
>
>     -#SYSTEMD_PACKAGES = "${PN}-server"
>     -SYSTEMD_SERVICE_${PN}-server = "pulseaudio.service"
>     +SYSTEMD_PACKAGES = "${PN}-server"
>     +SYSTEMD_USER_SERVICE_${PN}-server = "pulseaudio.service
>     pulseaudio.socket"
>
> I think specifying "pulseaudio.socket" for 
> SYSTEMD_USER_SERVICE_${PN}-server should be enough, systemd.bbclass is 
> going to add the .service file afair.

Add both:
chenqi@pek-hostel-deb01:~/poky/build-systemd [1] $ ls 
tmp/work/qemux86_64-poky-linux/core-image-minimal/1.0-r0/rootfs/etc/systemd/user
default.target.wants  sockets.target.wants

Add pulseaudio.socket:
chenqi@pek-hostel-deb01:~/poky/build-systemd [1] $ ls 
tmp/work/qemux86_64-poky-linux/core-image-minimal/1.0-r0/rootfs/etc/systemd/user
sockets.target.wants

They have different results.
I don't know a lot about pulseaudio. Can you confirm that 
pulseaudio.socket is enough?

Regards,
Chen Qi

>      FILES_${PN}-misc = "${bindir}/* ${libdir}/pulseaudio/libpulsedsp.so"
>
>     --
>     1.9.1
>
>     --
>     _______________________________________________
>     Openembedded-core mailing list
>     Openembedded-core@lists.openembedded.org
>     <mailto:Openembedded-core@lists.openembedded.org>
>     http://lists.openembedded.org/mailman/listinfo/openembedded-core
>     <http://lists.openembedded.org/mailman/listinfo/openembedded-core>
>
>


[-- Attachment #2: Type: text/html, Size: 5203 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] pulseaudio: fix to manage user services corretly
  2016-09-08  6:34     ` ChenQi
@ 2016-09-08 11:22       ` Tanu Kaskinen
  2016-09-09 18:10         ` Pau Espin Pedrol
  0 siblings, 1 reply; 13+ messages in thread
From: Tanu Kaskinen @ 2016-09-08 11:22 UTC (permalink / raw)
  To: openembedded-core

On Thu, 2016-09-08 at 14:34 +0800, ChenQi wrote:
> On 09/07/2016 06:29 PM, Pau Espin Pedrol wrote:
> > 
> > 
> > 
> > Pau Espin Pedrol
> > 
> > 2016-09-07 11:22 GMT+02:00 Chen Qi <Qi.Chen@windriver.com 
> > <mailto:Qi.Chen@windriver.com>>:
> > 
> >     Make use of the new SYSTEMD_USER_SERVICE variable added in
> >     systemd.bbclass
> >     to manage user services in pulseaudio-server package.
> > 
> >     Signed-off-by: Chen Qi <Qi.Chen@windriver.com
> >     <mailto:Qi.Chen@windriver.com>>
> >     ---
> >      meta/recipes-multimedia/pulseaudio/pulseaudio.inc | 4 ++--
> >      1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> >     diff --git a/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
> >     b/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
> >     index 6ed79ef..f3754d7 100644
> >     --- a/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
> >     +++ b/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
> >     @@ -124,8 +124,8 @@ FILES_${PN}-conf = "${sysconfdir}"
> >      FILES_${PN}-bin +=
> >     "${sysconfdir}/default/volatiles/volatiles.04_pulse"
> >      FILES_${PN}-server = "${bindir}/pulseaudio ${bindir}/start-*
> >     ${sysconfdir} ${bindir}/pactl */udev/rules.d/*.rules
> >     */*/udev/rules.d/*.rules ${systemd_user_unitdir}/*"
> > 
> >     -#SYSTEMD_PACKAGES = "${PN}-server"
> >     -SYSTEMD_SERVICE_${PN}-server = "pulseaudio.service"
> >     +SYSTEMD_PACKAGES = "${PN}-server"
> >     +SYSTEMD_USER_SERVICE_${PN}-server = "pulseaudio.service
> >     pulseaudio.socket"
> > 
> > I think specifying "pulseaudio.socket" for 
> > SYSTEMD_USER_SERVICE_${PN}-server should be enough, systemd.bbclass is 
> > going to add the .service file afair.
> 
> Add both:
> chenqi@pek-hostel-deb01:~/poky/build-systemd [1] $ ls 
> tmp/work/qemux86_64-poky-linux/core-image-minimal/1.0-r0/rootfs/etc/systemd/user
> default.target.wants  sockets.target.wants
> 
> Add pulseaudio.socket:
> chenqi@pek-hostel-deb01:~/poky/build-systemd [1] $ ls 
> tmp/work/qemux86_64-poky-linux/core-image-minimal/1.0-r0/rootfs/etc/systemd/user
> sockets.target.wants
> 
> They have different results.
> I don't know a lot about pulseaudio. Can you confirm that 
> pulseaudio.socket is enough?

If I understood correctly, Pau's argument was that there would be no
difference, which doesn't seem to be the case. However, if
pulseaudio.service isn't included in default.target.wants, that
shouldn't be a big problem, because socket activation will anyway start
the service when needed. On the other hand, I don't
think SYSTEMD_USER_SERVICE is intended to be used for controlling which
units to enable - it looks more like a variable that should contain all
units contained in the package. If my understanding is correct, you
should list both units in the variable.

The distributions that I've seen to use systemd to manage pulseaudio
only enable pulseaudio.socket, so that socket activation is used rather
than starting pulseaudio automatically as part of the user session. I
don't know if OE supports that - it looks like there's only a toggle to
enable all units or none. Other distributions also patch client.conf to
disable pulseaudio's own autospawning mechanism, since it's redundant
when using socket activation. Leaving autospawning enabled shouldn't
break anything in normal use, but it can cause some confusing behaviour
if the user tries to manually disable automatic pulseaudio starting by
disabling the systemd units.

-- 
Tanu


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] systemd.bbclass: add support to manage user services
  2016-09-08  2:33     ` ChenQi
@ 2016-09-09 17:48       ` Pau Espin Pedrol
  0 siblings, 0 replies; 13+ messages in thread
From: Pau Espin Pedrol @ 2016-09-09 17:48 UTC (permalink / raw)
  To: ChenQi; +Cc: OE-core

[-- Attachment #1: Type: text/plain, Size: 6712 bytes --]

Hi,

Pau Espin Pedrol

2016-09-08 4:33 GMT+02:00 ChenQi <Qi.Chen@windriver.com>:

> On 09/07/2016 08:14 PM, Pau Espin Pedrol wrote:
>
>
> 2016-09-07 11:22 GMT+02:00 Chen Qi <Qi.Chen@windriver.com>:
>
>> Add new variable SYSTEMD_USER_SERVICE and SYSTEM_USER_AUTO_ENABLE
>> to manage user services. Their usage is like SYSTEMD_SERVICE and
>> SYSTEMD_AUTO_ENABLE.
>>
>> [YOCTO #7800]
>>
>> Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
>> ---
>>  meta/classes/systemd.bbclass | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/meta/classes/systemd.bbclass b/meta/classes/systemd.bbclass
>> index db7873f..78cec97 100644
>> --- a/meta/classes/systemd.bbclass
>> +++ b/meta/classes/systemd.bbclass
>> @@ -7,6 +7,7 @@ SYSTEMD_PACKAGES_class-nativesdk ?= ""
>>
>>  # Whether to enable or disable the services on installation.
>>  SYSTEMD_AUTO_ENABLE ??= "enable"
>> +SYSTEMD_USER_AUTO_ENABLE ??= "enable"
>>
>>  # This class will be included in any recipe that supports systemd init
>> scripts,
>>  # even if systemd is not in DISTRO_FEATURES.  As such don't make any
>> changes
>> @@ -30,10 +31,14 @@ fi
>>
>>  if type systemctl >/dev/null 2>/dev/null; then
>>         systemctl $OPTS ${SYSTEMD_AUTO_ENABLE} ${SYSTEMD_SERVICE}
>> +       systemctl $OPTS --global ${SYSTEMD_USER_AUTO_ENABLE}
>> ${SYSTEMD_USER_SERVICE}
>>
>
> I'm not sure having these 2 systemctl being executed together everytime is
> a good idea. What if a recipe has a user service and no system service?
>
>
> Hi Pau Espin Pedrol,
>
> Thanks for your review.
>
> The postinstall script runs successfully with expected result at rootfs
> time. But your question reminds me of the situation of the on-target
> install/remove situation. I think I'll need to make a new patch to make
> sure things work in both situations.
>
> The key point here is that 'systemctl' at rootfs time is a shell script
> written by ourselves and 'systemctl' on target is that provided by systemd.
>

Yes, I was unaware of the existance of that OE script when I worked on
this. Why are we using that script at build time instead of using systemd's
systemctl? Any good reason for that? git log only shows it was imported
from another repo a while ago.


>
> We are calling the first one with an empty system service? Or we are may
> be enabling a systemd system service which should not be enabled according
> to the recipe? I have the feeling this kind of cases are not being catch in
> here and other pkg scripts in this commit.
>
> It is far from perfect, but in case you didn't, you may want to have a
> look at my initial/previous commit to try to fix the issue:
> https://www.mail-archive.com/openembedded-devel@lists.
> openembedded.org/msg42187.html
>
>
>>
>>         if [ -z "$D" -a "${SYSTEMD_AUTO_ENABLE}" = "enable" ]; then
>>                 systemctl restart ${SYSTEMD_SERVICE}
>>         fi
>> +       if [ -z "$D" -a "${SYSTEMD_USER_AUTO_ENABLE}" = "enable" ]; then
>> +               systemctl --global restart ${SYSTEMD_USER_SERVICE}
>> +       fi
>>  fi
>>  }
>>
>> @@ -47,9 +52,11 @@ fi
>>  if type systemctl >/dev/null 2>/dev/null; then
>>         if [ -z "$D" ]; then
>>                 systemctl stop ${SYSTEMD_SERVICE}
>> +               systemctl --global stop ${SYSTEMD_USER_SERVICE}
>>
>
> I think this is not gonna work, you cannot call --global with stop afair.
> I'm not sure which is the good solution for this though, but you should
> ideally go through all systemd user sessions and call "systemctl --user
> stop". No idea if that's actually easily feasible.
>
>
> The service is enabled and started with '--global' option, and stopping it
> with '--global' option seems reasonable to me. If there's some special
> case, let's look into it then.
>
>

Did you try running it in a running image? As in taking the ipkg, and
reinstalling it live in the image with systemd's systemctl. I bet you will
get an error from systemctl for both "--gobal restart" and "--global stop".
At least it used to be like that afair.



> Best Regards,
> Chen Qi
>
>
>
>
>>         fi
>>
>>         systemctl $OPTS disable ${SYSTEMD_SERVICE}
>> +       systemctl $OPTS --global disable ${SYSTEMD_USER_SERVICE}
>>  fi
>>  }
>>
>> @@ -139,12 +146,14 @@ python systemd_populate_packages() {
>>      def systemd_check_services():
>>          searchpaths = [oe.path.join(d.getVar("sysconfdir", True),
>> "systemd", "system"),]
>>          searchpaths.append(d.getVar("systemd_system_unitdir", True))
>> +        searchpaths.append(oe.path.join(d.getVar("sysconfdir", True),
>> "systemd", "user"))
>> +        searchpaths.append(d.getVar("systemd_user_unitdir", True))
>>          systemd_packages = d.getVar('SYSTEMD_PACKAGES', True)
>>
>>          keys = 'Also'
>>          # scan for all in SYSTEMD_SERVICE[]
>>          for pkg_systemd in systemd_packages.split():
>> -            for service in get_package_var(d, 'SYSTEMD_SERVICE',
>> pkg_systemd).split():
>> +            for service in (get_package_var(d, 'SYSTEMD_SERVICE',
>> pkg_systemd) + get_package_var(d, 'SYSTEMD_USER_SERVICE',
>> pkg_systemd)).split():
>>                  path_found = ''
>>
>>                  # Deal with adding, for example, 'ifplugd@eth0.service'
>> from
>> @@ -165,14 +174,14 @@ python systemd_populate_packages() {
>>                  if path_found != '':
>>                      systemd_add_files_and_parse(pkg_systemd,
>> path_found, service, keys)
>>                  else:
>> -                    raise bb.build.FuncFailed("SYSTEMD_SERVICE_%s value
>> %s does not exist" % \
>> -                        (pkg_systemd, service))
>> +                    raise bb.build.FuncFailed("SYSTEMD_SERVICE_%s or
>> SYSTEMD_USER_SERVICE_%s value %s does not exist" % \
>> +                        (pkg_systemd, pkg_systemd, service))
>>
>>      # Run all modifications once when creating package
>>      if os.path.exists(d.getVar("D", True)):
>>          for pkg in d.getVar('SYSTEMD_PACKAGES', True).split():
>>              systemd_check_package(pkg)
>> -            if d.getVar('SYSTEMD_SERVICE_' + pkg, True):
>> +            if d.getVar('SYSTEMD_SERVICE_' + pkg, True) or
>> d.getVar('SYSTEMD_USER_SERVICE_' + pkg, True):
>>                  systemd_generate_package_scripts(pkg)
>>          systemd_check_services()
>>  }
>> --
>> 1.9.1
>>
>> --
>> _______________________________________________
>> Openembedded-core mailing list
>> Openembedded-core@lists.openembedded.org
>> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>>
>
>
>

[-- Attachment #2: Type: text/html, Size: 12459 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] pulseaudio: fix to manage user services corretly
  2016-09-08 11:22       ` Tanu Kaskinen
@ 2016-09-09 18:10         ` Pau Espin Pedrol
  2016-09-09 18:34           ` Tanu Kaskinen
  0 siblings, 1 reply; 13+ messages in thread
From: Pau Espin Pedrol @ 2016-09-09 18:10 UTC (permalink / raw)
  To: Tanu Kaskinen, Chen Qi; +Cc: OE-core

Hi,

I think I didn't express myself correctly. Please note I did the
longer investigations quite a while ago and I'm mainly talking from
memory, so I may be wrong in some of the assumptions.

From systemd.bbclass, you can see this:
            if service.find('.socket') != -1:
                # for *.socket add *.service and *@.service
                service_base = service.replace('.socket', '')
                systemd_add_files_and_parse(pkg_systemd, path,
service_base + '.service', keys)
                systemd_add_files_and_parse(pkg_systemd, path,
service_base + '@.service', keys)


So, for installation purposes, no .service is required in
SYSTEMD_USER_SERVICE. Setting it .socket should also install (but not
enable) the .service file together with the .socket one into the
image, as actually the .socket one usually depends on the .service
file at runtime.

That being said, of course it's not the same behavior if you add only
one of them, the other or both:
1- Adding only .service -> It will install + enable the .service
2- Adding only the .socket -> It will install both. It will enable .socket
3- Adding .service + .socket -> It will install both. It will enable both.

Usually, you want either only the .service to be enabled (1, which
will start it automatically at startup of user session), or only the
.socket enabled (2, which will start only the socket at startup of
user session, and only start pulseaudio process when required by some
pulseaudio client). The third case, that is, installing + enabling
both is usually not a good idea, at least it makes no sense to me.

So, we should consider only "1" or "2". I would personally go for 2nd
option, and probably disable pulseaudio own autospawn system as
explained by Tanu. This way we don't start pulseaudio unless when it's
actually needed.

That is:
+SYSTEMD_PACKAGES = "${PN}-server"
+SYSTEMD_USER_SERVICE_${PN}-server = "pulseaudio.socket"

And by the way, you  should be able to remove
"${systemd_user_unitdir}/*" from FILES_${PN}-server as it should be
handled automatically by systemd.bbclass. If that's not the case, then
probably there's some error in systemd.bbclass you should look into.

Hope I explained myself better now. Regards!


2016-09-08 13:22 GMT+02:00 Tanu Kaskinen <tanuk@iki.fi>:
> On Thu, 2016-09-08 at 14:34 +0800, ChenQi wrote:
>> On 09/07/2016 06:29 PM, Pau Espin Pedrol wrote:
>> >
>> >
>> >
>> > Pau Espin Pedrol
>> >
>> > 2016-09-07 11:22 GMT+02:00 Chen Qi <Qi.Chen@windriver.com
>> > <mailto:Qi.Chen@windriver.com>>:
>> >
>> >     Make use of the new SYSTEMD_USER_SERVICE variable added in
>> >     systemd.bbclass
>> >     to manage user services in pulseaudio-server package.
>> >
>> >     Signed-off-by: Chen Qi <Qi.Chen@windriver.com
>> >     <mailto:Qi.Chen@windriver.com>>
>> >     ---
>> >      meta/recipes-multimedia/pulseaudio/pulseaudio.inc | 4 ++--
>> >      1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> >     diff --git a/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
>> >     b/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
>> >     index 6ed79ef..f3754d7 100644
>> >     --- a/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
>> >     +++ b/meta/recipes-multimedia/pulseaudio/pulseaudio.inc
>> >     @@ -124,8 +124,8 @@ FILES_${PN}-conf = "${sysconfdir}"
>> >      FILES_${PN}-bin +=
>> >     "${sysconfdir}/default/volatiles/volatiles.04_pulse"
>> >      FILES_${PN}-server = "${bindir}/pulseaudio ${bindir}/start-*
>> >     ${sysconfdir} ${bindir}/pactl */udev/rules.d/*.rules
>> >     */*/udev/rules.d/*.rules ${systemd_user_unitdir}/*"
>> >
>> >     -#SYSTEMD_PACKAGES = "${PN}-server"
>> >     -SYSTEMD_SERVICE_${PN}-server = "pulseaudio.service"
>> >     +SYSTEMD_PACKAGES = "${PN}-server"
>> >     +SYSTEMD_USER_SERVICE_${PN}-server = "pulseaudio.service
>> >     pulseaudio.socket"
>> >
>> > I think specifying "pulseaudio.socket" for
>> > SYSTEMD_USER_SERVICE_${PN}-server should be enough, systemd.bbclass is
>> > going to add the .service file afair.
>>
>> Add both:
>> chenqi@pek-hostel-deb01:~/poky/build-systemd [1] $ ls
>> tmp/work/qemux86_64-poky-linux/core-image-minimal/1.0-r0/rootfs/etc/systemd/user
>> default.target.wants  sockets.target.wants
>>
>> Add pulseaudio.socket:
>> chenqi@pek-hostel-deb01:~/poky/build-systemd [1] $ ls
>> tmp/work/qemux86_64-poky-linux/core-image-minimal/1.0-r0/rootfs/etc/systemd/user
>> sockets.target.wants
>>
>> They have different results.
>> I don't know a lot about pulseaudio. Can you confirm that
>> pulseaudio.socket is enough?
>
> If I understood correctly, Pau's argument was that there would be no
> difference, which doesn't seem to be the case. However, if
> pulseaudio.service isn't included in default.target.wants, that
> shouldn't be a big problem, because socket activation will anyway start
> the service when needed. On the other hand, I don't
> think SYSTEMD_USER_SERVICE is intended to be used for controlling which
> units to enable - it looks more like a variable that should contain all
> units contained in the package. If my understanding is correct, you
> should list both units in the variable.
>
> The distributions that I've seen to use systemd to manage pulseaudio
> only enable pulseaudio.socket, so that socket activation is used rather
> than starting pulseaudio automatically as part of the user session. I
> don't know if OE supports that - it looks like there's only a toggle to
> enable all units or none. Other distributions also patch client.conf to
> disable pulseaudio's own autospawning mechanism, since it's redundant
> when using socket activation. Leaving autospawning enabled shouldn't
> break anything in normal use, but it can cause some confusing behaviour
> if the user tries to manually disable automatic pulseaudio starting by
> disabling the systemd units.
>
> --
> Tanu
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] pulseaudio: fix to manage user services corretly
  2016-09-09 18:10         ` Pau Espin Pedrol
@ 2016-09-09 18:34           ` Tanu Kaskinen
  0 siblings, 0 replies; 13+ messages in thread
From: Tanu Kaskinen @ 2016-09-09 18:34 UTC (permalink / raw)
  To: openembedded-core

On Fri, 2016-09-09 at 20:10 +0200, Pau Espin Pedrol wrote:
> Hi,
> 
> I think I didn't express myself correctly. Please note I did the
> longer investigations quite a while ago and I'm mainly talking from
> memory, so I may be wrong in some of the assumptions.
> 
> From systemd.bbclass, you can see this:
>             if service.find('.socket') != -1:
>                 # for *.socket add *.service and *@.service
>                 service_base = service.replace('.socket', '')
>                 systemd_add_files_and_parse(pkg_systemd, path,
> service_base + '.service', keys)
>                 systemd_add_files_and_parse(pkg_systemd, path,
> service_base + '@.service', keys)
> 
> 
> So, for installation purposes, no .service is required in
> SYSTEMD_USER_SERVICE. Setting it .socket should also install (but not
> enable) the .service file together with the .socket one into the
> image, as actually the .socket one usually depends on the .service
> file at runtime.
> 
> That being said, of course it's not the same behavior if you add only
> one of them, the other or both:
> 1- Adding only .service -> It will install + enable the .service
> 2- Adding only the .socket -> It will install both. It will enable .socket
> 3- Adding .service + .socket -> It will install both. It will enable both.
> 
> Usually, you want either only the .service to be enabled (1, which
> will start it automatically at startup of user session), or only the
> .socket enabled (2, which will start only the socket at startup of
> user session, and only start pulseaudio process when required by some
> pulseaudio client). The third case, that is, installing + enabling
> both is usually not a good idea, at least it makes no sense to me.
> 
> So, we should consider only "1" or "2". I would personally go for 2nd
> option, and probably disable pulseaudio own autospawn system as
> explained by Tanu. This way we don't start pulseaudio unless when it's
> actually needed.
> 
> That is:
> +SYSTEMD_PACKAGES = "${PN}-server"
> +SYSTEMD_USER_SERVICE_${PN}-server = "pulseaudio.socket"
> 
> And by the way, you  should be able to remove
> "${systemd_user_unitdir}/*" from FILES_${PN}-server as it should be
> handled automatically by systemd.bbclass. If that's not the case, then
> probably there's some error in systemd.bbclass you should look into.
> 
> Hope I explained myself better now. Regards!

Thanks for the explanation. Part of my confusion was that I incorrectly
assumed that systemd.bbclass couldn't guess that the pulseaudio-server
package contains systemd units (due to the -server suffix), and
therefore it seemed to me that SYSTEMD_USER_SERVICE is expected to
contain all units, but that problem is fixed by the SYSTEMD_PACKAGES
variable.

Still, I think SYSTEMD_USER_SERVICE is a misleading variable name, if
the actual purpose of that variable is only to control which units to
enable during installation. Something like SYSTEMD_USER_ENABLE_UNITS
would be better. However, this problem is older than these patches (due
to SYSTEMD_SERVICE for system units), so I don't suggest that the
patches should be changed because of this. It indeed seems like the
best course of action would be to only add the pulseaudio.socket unit
to SYSTEMD_USER_SERVICE. Patching client.conf to disable autospawning
in case systemd is enabled would be a good addition, although not
strictly necessary.

-- 
Tanu


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-09-09 18:34 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-07  9:22 [PATCH 0/3] systemd: add support to manage user units Chen Qi
2016-09-07  9:22 ` [PATCH 1/3] systemd-systemctl: add option to manage user services Chen Qi
2016-09-07  9:22 ` [PATCH 2/3] systemd.bbclass: add support " Chen Qi
2016-09-07 12:14   ` Pau Espin Pedrol
2016-09-08  2:33     ` ChenQi
2016-09-09 17:48       ` Pau Espin Pedrol
2016-09-07  9:22 ` [PATCH 3/3] pulseaudio: fix to manage user services corretly Chen Qi
2016-09-07 10:29   ` Pau Espin Pedrol
2016-09-08  6:34     ` ChenQi
2016-09-08 11:22       ` Tanu Kaskinen
2016-09-09 18:10         ` Pau Espin Pedrol
2016-09-09 18:34           ` Tanu Kaskinen
2016-09-07  9:43 ` [PATCH 0/3] systemd: add support to manage user units Jérémy Rosen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.