[PATCH] systemd.bbclass: Introduce do_install_append and use systemd unitdir

[PATCH] systemd.bbclass: Introduce do_install_append and use systemd unitdir

[Khem Raj]

systemd always uses /lib and /usr/lib to store unit files
so using libdir and base_libdir is incorrect. It will work
where libdir is usr/lib and base_libdir is /lib but wont work
when say its /lib64

Additionally introduce the install append from meta-oe
lot of recipe appends in meta-systemd depend on that

Signed-off-by: Khem Raj <raj.khem@gmail.com>
---
 meta/classes/systemd.bbclass |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/meta/classes/systemd.bbclass b/meta/classes/systemd.bbclass
index e0ea65c..32cc5c2 100644
--- a/meta/classes/systemd.bbclass
+++ b/meta/classes/systemd.bbclass
@@ -115,11 +115,9 @@ def systemd_populate_packages(d):
 
     # Check service-files and call systemd_add_files_and_parse for each entry
     def systemd_check_services():
-        base_libdir = d.getVar('base_libdir', True)
-        searchpaths = [oe.path.join(d.getVar("sysconfdir", True), "systemd", "system"),]
-        searchpaths.append(oe.path.join(d.getVar("base_libdir", True), "systemd", "system"))
-        searchpaths.append(oe.path.join(d.getVar("libdir", True), "systemd", "system"))
-        searchpaths.append(oe.path.join(d.getVar("libdir", True), "systemd", "user"))
+        searchpaths = '/etc/systemd/system/' + ' '
+        searchpaths += '/lib/systemd/system/' + ' '
+        searchpaths += '/usr/lib/systemd/system/' + ' '
         systemd_packages = d.getVar('SYSTEMD_PACKAGES', True)
         has_exactly_one_service = len(systemd_packages.split()) == 1
         if has_exactly_one_service:
@@ -133,7 +131,7 @@ def systemd_populate_packages(d):
         for pkg_systemd in systemd_packages.split():
             for service in get_package_var(d, 'SYSTEMD_SERVICE', pkg_systemd).split():
                 path_found = ''
-                for path in searchpaths:
+                for path in searchpaths.split():
                     if os.path.exists(oe.path.join(d.getVar("D", True), path, service)):
                         path_found = path
                         break
@@ -156,3 +154,14 @@ python populate_packages_prepend () {
     if oe.utils.contains ('DISTRO_FEATURES', 'systemd', True, False, d):
         systemd_populate_packages (d)
 }
+# automatically install all *.service and *.socket supplied in recipe's SRC_URI
+do_install_append() {
+	for service in `find ${WORKDIR} -maxdepth 1 -name '*.service' -o -name '*.socket'` ; do
+	# ensure installing systemd-files only (e.g not avahi *.service)
+		if grep -q '\[Unit\]' $service ; then
+			install -d ${D}${systemd_unitdir}/system
+			install -m 644 $service ${D}${systemd_unitdir}/system
+		fi
+	done
+}
+
-- 
1.7.9.5

Re: [PATCH] systemd.bbclass: Introduce do_install_append and use systemd unitdir

[Burton, Ross]

On 12 February 2013 17:42, Khem Raj <raj.khem@gmail.com> wrote:
> systemd always uses /lib and /usr/lib to store unit files
> so using libdir and base_libdir is incorrect. It will work
> where libdir is usr/lib and base_libdir is /lib but wont work
> when say its /lib64

That's interesting, wasn't aware of that.  Why doesn't systemd respect
the configure options?

> Additionally introduce the install append from meta-oe
> lot of recipe appends in meta-systemd depend on that

This was removed on purpose because it should never be used.  It was
used in meta-systemd because the service files in SRC_URI had
hard-coded /usr/bin, /usr/sbin, etc.  Hard-coding these paths is
wrong, you'll need e.g. run a service.in through a
s/@sbindir@/${sbindir} and if you're doing this you can just do that
in do_install_append() and drop it directly into ${D}.

Radu's work at merging the rest of the changes in meta-systemd to
oe-core does this, for example:

http://git.yoctoproject.org/cgit/cgit.cgi/poky-contrib/commit/?h=rmoisan/systemd-ross&id=08d9095b31955eca862d6702dddeaf155ac8c987

Ross

Re: [PATCH] systemd.bbclass: Introduce do_install_append and use systemd unitdir

[Richard Purdie]

On Tue, 2013-02-12 at 09:42 -0800, Khem Raj wrote:
> systemd always uses /lib and /usr/lib to store unit files
> so using libdir and base_libdir is incorrect. It will work
> where libdir is usr/lib and base_libdir is /lib but wont work
> when say its /lib64
> 
> Additionally introduce the install append from meta-oe
> lot of recipe appends in meta-systemd depend on that
> 
> Signed-off-by: Khem Raj <raj.khem@gmail.com>
> ---
>  meta/classes/systemd.bbclass |   21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/meta/classes/systemd.bbclass b/meta/classes/systemd.bbclass
> index e0ea65c..32cc5c2 100644
> --- a/meta/classes/systemd.bbclass
> +++ b/meta/classes/systemd.bbclass
> @@ -115,11 +115,9 @@ def systemd_populate_packages(d):
>  
>      # Check service-files and call systemd_add_files_and_parse for each entry
>      def systemd_check_services():
> -        base_libdir = d.getVar('base_libdir', True)
> -        searchpaths = [oe.path.join(d.getVar("sysconfdir", True), "systemd", "system"),]
> -        searchpaths.append(oe.path.join(d.getVar("base_libdir", True), "systemd", "system"))
> -        searchpaths.append(oe.path.join(d.getVar("libdir", True), "systemd", "system"))
> -        searchpaths.append(oe.path.join(d.getVar("libdir", True), "systemd", "user"))
> +        searchpaths = '/etc/systemd/system/' + ' '

Why remove sysconfdir? 

Also, lets standardise on one variable please. We have
${nonarch_base_libdir} and ${systemd_unitdir} so do we need to
hardcode /lib and /usr/lib?

I'd suggest we add ${nonarch_libdir}, standardise on those and drop
systemd_unitdir. We use the nonarch for other non-systemd multilib work.

> +        searchpaths += '/lib/systemd/system/' + ' '
> +        searchpaths += '/usr/lib/systemd/system/' + ' '
>          systemd_packages = d.getVar('SYSTEMD_PACKAGES', True)
>          has_exactly_one_service = len(systemd_packages.split()) == 1
>          if has_exactly_one_service:
> @@ -133,7 +131,7 @@ def systemd_populate_packages(d):
>          for pkg_systemd in systemd_packages.split():
>              for service in get_package_var(d, 'SYSTEMD_SERVICE', pkg_systemd).split():
>                  path_found = ''
> -                for path in searchpaths:
> +                for path in searchpaths.split():
>                      if os.path.exists(oe.path.join(d.getVar("D", True), path, service)):
>                          path_found = path
>                          break
> @@ -156,3 +154,14 @@ python populate_packages_prepend () {
>      if oe.utils.contains ('DISTRO_FEATURES', 'systemd', True, False, d):
>          systemd_populate_packages (d)
>  }
> +# automatically install all *.service and *.socket supplied in recipe's SRC_URI
> +do_install_append() {
> +	for service in `find ${WORKDIR} -maxdepth 1 -name '*.service' -o -name '*.socket'` ; do
> +	# ensure installing systemd-files only (e.g not avahi *.service)
> +		if grep -q '\[Unit\]' $service ; then
> +			install -d ${D}${systemd_unitdir}/system
> +			install -m 644 $service ${D}${systemd_unitdir}/system
> +		fi
> +	done
> +}
> +

Please no. We have this kind of mess from the binconfig class and its
horrible to maintain. I'm looking forward to ripping that class out
someday.

Lets just write proper do_install functions in the recipes and make them
conditional on systemd being enabled.

Cheers,

Richard

Re: [PATCH] systemd.bbclass: Introduce do_install_append and use systemd unitdir

[Andreas Müller]

On Tue, Feb 12, 2013 at 10:24 PM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> On Tue, 2013-02-12 at 09:42 -0800, Khem Raj wrote:
>> systemd always uses /lib and /usr/lib to store unit files
>> so using libdir and base_libdir is incorrect. It will work
>> where libdir is usr/lib and base_libdir is /lib but wont work
>> when say its /lib64
>>
>> Additionally introduce the install append from meta-oe
>> lot of recipe appends in meta-systemd depend on that
>>
>> Signed-off-by: Khem Raj <raj.khem@gmail.com>
>> ---
>>  meta/classes/systemd.bbclass |   21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/meta/classes/systemd.bbclass b/meta/classes/systemd.bbclass
>> index e0ea65c..32cc5c2 100644
>> --- a/meta/classes/systemd.bbclass
>> +++ b/meta/classes/systemd.bbclass
>> @@ -115,11 +115,9 @@ def systemd_populate_packages(d):
>>
>>      # Check service-files and call systemd_add_files_and_parse for each entry
>>      def systemd_check_services():
>> -        base_libdir = d.getVar('base_libdir', True)
>> -        searchpaths = [oe.path.join(d.getVar("sysconfdir", True), "systemd", "system"),]
>> -        searchpaths.append(oe.path.join(d.getVar("base_libdir", True), "systemd", "system"))
>> -        searchpaths.append(oe.path.join(d.getVar("libdir", True), "systemd", "system"))
>> -        searchpaths.append(oe.path.join(d.getVar("libdir", True), "systemd", "user"))
>> +        searchpaths = '/etc/systemd/system/' + ' '
>
> Why remove sysconfdir?
>
> Also, lets standardise on one variable please. We have
> ${nonarch_base_libdir} and ${systemd_unitdir} so do we need to
> hardcode /lib and /usr/lib?
>
> I'd suggest we add ${nonarch_libdir}, standardise on those and drop
> systemd_unitdir. We use the nonarch for other non-systemd multilib work.
>
>> +        searchpaths += '/lib/systemd/system/' + ' '
>> +        searchpaths += '/usr/lib/systemd/system/' + ' '
>>          systemd_packages = d.getVar('SYSTEMD_PACKAGES', True)
>>          has_exactly_one_service = len(systemd_packages.split()) == 1
>>          if has_exactly_one_service:
>> @@ -133,7 +131,7 @@ def systemd_populate_packages(d):
>>          for pkg_systemd in systemd_packages.split():
>>              for service in get_package_var(d, 'SYSTEMD_SERVICE', pkg_systemd).split():
>>                  path_found = ''
>> -                for path in searchpaths:
>> +                for path in searchpaths.split():
>>                      if os.path.exists(oe.path.join(d.getVar("D", True), path, service)):
>>                          path_found = path
>>                          break
>> @@ -156,3 +154,14 @@ python populate_packages_prepend () {
>>      if oe.utils.contains ('DISTRO_FEATURES', 'systemd', True, False, d):
>>          systemd_populate_packages (d)
>>  }
>> +# automatically install all *.service and *.socket supplied in recipe's SRC_URI
>> +do_install_append() {
>> +     for service in `find ${WORKDIR} -maxdepth 1 -name '*.service' -o -name '*.socket'` ; do
>> +     # ensure installing systemd-files only (e.g not avahi *.service)
>> +             if grep -q '\[Unit\]' $service ; then
>> +                     install -d ${D}${systemd_unitdir}/system
>> +                     install -m 644 $service ${D}${systemd_unitdir}/system
>> +             fi
>> +     done
>> +}
>> +
>
> Please no. We have this kind of mess from the binconfig class and its
> horrible to maintain. I'm looking forward to ripping that class out
> someday.
>
> Lets just write proper do_install functions in the recipes and make them
> conditional on systemd being enabled.
>
Copying similar code in in tons of recipes is easier to maintain? We
had this before meta-systemd started and it was a mess. How about a
variable SYSTEMD_AUTO_INSTALL?="enable" for this - since it fits in
many cases.

Andreas

Re: [PATCH] systemd.bbclass: Introduce do_install_append and use systemd unitdir

[Richard Purdie]

On Wed, 2013-02-13 at 00:55 +0100, Andreas Müller wrote:
> On Tue, Feb 12, 2013 at 10:24 PM, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > On Tue, 2013-02-12 at 09:42 -0800, Khem Raj wrote:
> >> systemd always uses /lib and /usr/lib to store unit files
> >> so using libdir and base_libdir is incorrect. It will work
> >> where libdir is usr/lib and base_libdir is /lib but wont work
> >> when say its /lib64
> >>
> >> Additionally introduce the install append from meta-oe
> >> lot of recipe appends in meta-systemd depend on that
> >>
> >> Signed-off-by: Khem Raj <raj.khem@gmail.com>
> >> ---
> >>  meta/classes/systemd.bbclass |   21 +++++++++++++++------
> >>  1 file changed, 15 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/meta/classes/systemd.bbclass b/meta/classes/systemd.bbclass
> >> index e0ea65c..32cc5c2 100644
> >> --- a/meta/classes/systemd.bbclass
> >> +++ b/meta/classes/systemd.bbclass
> >> @@ -115,11 +115,9 @@ def systemd_populate_packages(d):
> >>
> >>      # Check service-files and call systemd_add_files_and_parse for each entry
> >>      def systemd_check_services():
> >> -        base_libdir = d.getVar('base_libdir', True)
> >> -        searchpaths = [oe.path.join(d.getVar("sysconfdir", True), "systemd", "system"),]
> >> -        searchpaths.append(oe.path.join(d.getVar("base_libdir", True), "systemd", "system"))
> >> -        searchpaths.append(oe.path.join(d.getVar("libdir", True), "systemd", "system"))
> >> -        searchpaths.append(oe.path.join(d.getVar("libdir", True), "systemd", "user"))
> >> +        searchpaths = '/etc/systemd/system/' + ' '
> >
> > Why remove sysconfdir?
> >
> > Also, lets standardise on one variable please. We have
> > ${nonarch_base_libdir} and ${systemd_unitdir} so do we need to
> > hardcode /lib and /usr/lib?
> >
> > I'd suggest we add ${nonarch_libdir}, standardise on those and drop
> > systemd_unitdir. We use the nonarch for other non-systemd multilib work.
> >
> >> +        searchpaths += '/lib/systemd/system/' + ' '
> >> +        searchpaths += '/usr/lib/systemd/system/' + ' '
> >>          systemd_packages = d.getVar('SYSTEMD_PACKAGES', True)
> >>          has_exactly_one_service = len(systemd_packages.split()) == 1
> >>          if has_exactly_one_service:
> >> @@ -133,7 +131,7 @@ def systemd_populate_packages(d):
> >>          for pkg_systemd in systemd_packages.split():
> >>              for service in get_package_var(d, 'SYSTEMD_SERVICE', pkg_systemd).split():
> >>                  path_found = ''
> >> -                for path in searchpaths:
> >> +                for path in searchpaths.split():
> >>                      if os.path.exists(oe.path.join(d.getVar("D", True), path, service)):
> >>                          path_found = path
> >>                          break
> >> @@ -156,3 +154,14 @@ python populate_packages_prepend () {
> >>      if oe.utils.contains ('DISTRO_FEATURES', 'systemd', True, False, d):
> >>          systemd_populate_packages (d)
> >>  }
> >> +# automatically install all *.service and *.socket supplied in recipe's SRC_URI
> >> +do_install_append() {
> >> +     for service in `find ${WORKDIR} -maxdepth 1 -name '*.service' -o -name '*.socket'` ; do
> >> +     # ensure installing systemd-files only (e.g not avahi *.service)
> >> +             if grep -q '\[Unit\]' $service ; then
> >> +                     install -d ${D}${systemd_unitdir}/system
> >> +                     install -m 644 $service ${D}${systemd_unitdir}/system
> >> +             fi
> >> +     done
> >> +}
> >> +
> >
> > Please no. We have this kind of mess from the binconfig class and its
> > horrible to maintain. I'm looking forward to ripping that class out
> > someday.
> >
> > Lets just write proper do_install functions in the recipes and make them
> > conditional on systemd being enabled.
> >
> Copying similar code in in tons of recipes is easier to maintain?

Its dangerous and risky. This code sits silently in the class file and
copies files. Imagine some project starts installing its own service
files yet someone doesn't notice when they upgrade the recipe. All of a
sudden we start overwriting files and nobody notices. This does happen
as things get adopted by upstreams.

This is the big problem with the binconfig class, it overwrites anything
do_install does. We have so much history there we don't dare touch it
either. How did we get there? Originally packages didn't provide -config
files. Personally, I can't wait to delete the thing.

> We had this before meta-systemd started and it was a mess. How about a
> variable SYSTEMD_AUTO_INSTALL?="enable" for this - since it fits in
> many cases.

At least spell out which files to install:

SYSTEMD_EXTRAINSTALL = "${WORKDIR}/xxxx.service"

then the user stands a better change of spotting a conflict. Yes, I know
the files are listed in SRC_URI but I don't think its enough. We can
also throw an error if the files already exist.

There is a time and a place for magic things behind the scenes but IME
find calls in functions like this don't end well. The "ignore avahi
service files" issue in the code already should serve as a suitable
warning too.

Cheers,

Richard

Re: [PATCH] systemd.bbclass: Introduce do_install_append and use systemd unitdir

[Phil Blundell]

On Wed, 2013-02-13 at 00:55 +0100, Andreas Müller wrote:
> Copying similar code in in tons of recipes is easier to maintain?

If the code that needs to go in the "tons" of recipes is just:

do_install_append() {
	install -m 644 ${WORKDIR}/my.service ${D}${systemd_unitdir}/system
}

then yes, this is easier to maintain than a mechanism that magically
fishes out files from the workdir and installs them for itself.  As
Richard mentioned, we have previous experience with this in the form of
binconfig (and there have been others in the past) and this sort of
thing has universally turned out to be a maintenance headache after a
while.

There's even a comment in the code you quoted:

>> +     # ensure installing systemd-files only (e.g not avahi *.service)

which suggests that some poor soul has previously found that it was
installing things it oughtn't to have done.

p.

Re: [PATCH] systemd.bbclass: Introduce do_install_append and use systemd unitdir

[Khem Raj]

On Wed, Feb 13, 2013 at 9:52 AM, Phil Blundell <pb@pbcl.net> wrote:
> On Wed, 2013-02-13 at 00:55 +0100, Andreas Müller wrote:
>> Copying similar code in in tons of recipes is easier to maintain?
>
> If the code that needs to go in the "tons" of recipes is just:
>
> do_install_append() {
>         install -m 644 ${WORKDIR}/my.service ${D}${systemd_unitdir}/system
> }
>
> then yes, this is easier to maintain than a mechanism that magically
> fishes out files from the workdir and installs them for itself.  As
> Richard mentioned, we have previous experience with this in the form of
> binconfig (and there have been others in the past) and this sort of
> thing has universally turned out to be a maintenance headache after a
> while.
>
> There's even a comment in the code you quoted:
>
>>> +     # ensure installing systemd-files only (e.g not avahi *.service)
>
> which suggests that some poor soul has previously found that it was
> installing things it oughtn't to have done.
>

I have dropped the idea of having append and taken the pain to add the append to
bbappends I think as systemd gets adopted widely I think the packages themselves
will start providing systemd support and then we have to start
dropping them one by one
so if they are per recipe then its easier.

> p.
>
>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core