Change the way of CONFFILES handling

This patchset has been tested by building core-image-sato and core-image-sato-sdk against the three package backends in OE (rpm, deb, ipk).

Changes in V2:
1. Extract the common code in FILES handling and CONFFILES handling
2. Fix error for ipk package backend
3. Fix errors for deb package backend

The following changes since commit 8d0e56a850579f9a6d501266deeef9b257ce4780:

serf: readded md5sum (2014-12-11 11:34:22 +0000)

are available in the git repository at:

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

Chen Qi (4):
packaging: allow globs in CONFFILES
package_manager.py: user 'purge' instead of 'remove' in case of deb update-rc.d: use '-f' option in updatercd_postrm useradd.bbclass: avoid do_rootfs error for debian package backend

 meta/classes/package.bbclass     | 79 +++++++++++++++++++++++++++++++---------
 meta/classes/package_deb.bbclass |  2 +-
 meta/classes/package_ipk.bbclass |  2 +-
 meta/classes/package_rpm.bbclass |  2 +-
 meta/classes/update-rc.d.bbclass |  4 +-
 meta/classes/useradd.bbclass     | 10 +++++
 meta/conf/bitbake.conf           |  2 +
 meta/lib/oe/package_manager.py   |  4 +-
 8 files changed, 80 insertions(+), 25 deletions(-)

1.9.1

Allow globs in CONFFILES and default CONFFILES to '${sysconfdir}'.

Previously, the CONFFILES variable is a list of config files to be packaged. However, as these files may come directly from the source instead of our WORKDIR, it's a very common mistake that we forget to set this variable in our recipes. This will lead to unexpected behavior when doing an on-target upgrade. For example, we modify the /etc/login.defs locally and then do an upgrade. The file will be replaced, which is obviously not correct.

This patch changes the way of CONFFILES handling. After this change, the CONFFILES can take the same form as FILES. That means, we don't have to list a bunch of files for CONFFILES. It will just be expanded like the FILES variable.

As almost all files under /etc are basically configuration files, we provide a default value for CONFFILES.

    CONNFFILES = "${sysconfdir}"

In this way, we don't need to modify every recipe to set the CONFFILES variable. Of course, setting CONFFILES in recipes take precedence over the CONFFILES. For example, if the recipe author decides that package A should only treat files under ${sysconfdir}/default/ as config files, he/she can write like this.

    CONFFILES_A = "${sysconfdir}/default"

The above situation should not be common. As according to FHS, the /etc directory is a place for all system related configuration files.

[YOCTO #5200]

---
 meta/classes/package.bbclass     | 79 +++++++++++++++++++++++++++++++---------
 meta/classes/package_deb.bbclass |  2 +-
 meta/classes/package_ipk.bbclass |  2 +-
 meta/classes/package_rpm.bbclass |  2 +-
 meta/conf/bitbake.conf           |  2 +
 5 files changed, 66 insertions(+), 21 deletions(-)

index 89cce40..f42e8fe 100644
@@ -237,6 +237,66 @@ python () {
         d.appendVarFlag('do_package', 'deptask', " do_packagedata")
 }
 
+# Get a list of files from file vars by searching files under current working directory
+# The list contains symlinks, directories and normal files.
+def files_from_filevars(filevars):
+    import os,glob
+    cpath = oe.cachedpath.CachedPath()
+    files = []
+    for f in filevars:
+        if os.path.isabs(f):
+            f = '.' + f
+        if not f.startswith("./"):
+            f = './' + f
+        globbed = glob.glob(f)
+        if globbed:
+            if [ f ] != globbed:
+                files += globbed
+                continue
+        files.append(f)
+
+    for f in files:
+        if not cpath.islink(f):
+            if cpath.isdir(f):
+                newfiles = [ os.path.join(f,x) for x in os.listdir(f) ]
+                if newfiles:
+                    files += newfiles
+
+    return files
+
+# Called in package_<rpm,ipk,deb>.bbclass to get the correct list of configuration files
+def get_conffiles(pkg, d):
+    pkgdest = d.getVar('PKGDEST', True)
+    root = os.path.join(pkgdest, pkg)
+    cwd = os.getcwd()
+    os.chdir(root)
+
+    conffiles = d.getVar('CONFFILES_%s' % pkg, True);
+    if conffiles == None:
+        conffiles = d.getVar('CONFFILES', True)
+    if conffiles == None:
+        conffiles = ""
+    conffiles = conffiles.split()
+    conf_orig_list = files_from_filevars(conffiles)
+
+    # Remove links and directories from conf_orig_list to get conf_list which only contains normal files
+    conf_list = []
+    for f in conf_orig_list:
+        if os.path.isdir(f):
+            continue
+        if os.path.islink(f):
+            continue
+        if not os.path.exists(f):
+            continue
+        conf_list.append(f)
+
+    # Remove the leading './'
+    for i in range(0, len(conf_list)):
+        conf_list[i] = conf_list[i][1:]
+
+    os.chdir(cwd)
+    return conf_list
+
 def splitdebuginfo(file, debugfile, debugsrcdir, sourcefile, d):
     # Function to split a single file into two components, one is the stripped
     # target system binary, the other contains any debugging information. The

@@ -1006,26 +1066,9 @@ python populate_packages () {
             filesvar.replace("//", "/")
 
         origfiles = filesvar.split()
-        files = []
-        for file in origfiles:
-            if os.path.isabs(file):
-                file = '.' + file
-            if not file.startswith("./"):
-                file = './' + file
-            globbed = glob.glob(file)
-            if globbed:
-                if [ file ] != globbed:
-                    files += globbed
-                    continue
-            files.append(file)
+        files = files_from_filevars(origfiles)
 
         for file in files:
-            if not cpath.islink(file):
-                if cpath.isdir(file):
-                    newfiles =  [ os.path.join(file,x) for x in os.listdir(file) ]
-                    if newfiles:
-                        files += newfiles
-                        continue
             if (not cpath.islink(file)) and (not cpath.exists(file)):
                 continue
             if file in seen:

index 5b5f7e2..9d7c59b 100644
@@ -262,7 +262,7 @@ python do_package_deb () {
             scriptfile.close()
             os.chmod(os.path.join(controldir, script), 0755)
 
-        conffiles_str = localdata.getVar("CONFFILES", True)
+        conffiles_str = ' '.join(get_conffiles(pkg, d))
         if conffiles_str:
             try:
                 conffiles = open(os.path.join(controldir, 'conffiles'), 'w')

index 44fd3eb..dba6804 100644
@@ -226,7 +226,7 @@ python do_package_ipk () {
             scriptfile.close()
             os.chmod(os.path.join(controldir, script), 0755)
 
-        conffiles_str = localdata.getVar("CONFFILES", True)
+        conffiles_str = ' '.join(get_conffiles(pkg, d))
         if conffiles_str:
             try:
                 conffiles = open(os.path.join(controldir, 'conffiles'), 'w')

index 92ddf7a..b87e634 100644
@@ -324,7 +324,7 @@ python write_specfile () {
 
         bb.data.update_data(localdata)
 
-        conffiles = (localdata.getVar('CONFFILES', True) or "").split()
+        conffiles = get_conffiles(pkg, d)
         dirfiles = localdata.getVar('DIRFILES', True)
         if dirfiles is not None:
             dirfiles = dirfiles.split()

index 7902bd7..3648508 100644
@@ -265,6 +265,8 @@ PACKAGES = "${PN}-dbg ${PN}-staticdev ${PN}-dev ${PN}-doc ${PN}-locale ${PACKAGE
 PACKAGES_DYNAMIC = "^${PN}-locale-.*"
 FILES = ""
 
+CONFFILES = "${sysconfdir}"
+
 FILES_${PN} = "${bindir}/* ${sbindir}/* ${libexecdir}/* ${libdir}/lib*${SOLIBS} \
             ${sysconfdir} ${sharedstatedir} ${localstatedir} \
             ${base_bindir}/* ${base_sbindir}/* \

1.9.1

This is going to hurt a lot of recipes. Please do not set this.

The default has always been that CONFFILES is empty. I see no reason whatsoever to suddenly change this.

FHS may have written that somewhere, but if you just take a quick look at what is in /etc/, you'll find that actual configuration files are quite rare, but there are tons of scripts and distro related files there. Whether or not a file there is supposed to be edited by the end-user is something of a distro decision.

Also, "configuration file" does not necessarily mean that the USER is in control. A configuration file may also simply distinguish machine capabilities or software features.

We'll end up having to patch hundreds of recipes with a CONFFILES="" or similar line.

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Systems Eindhovenseweg 32-C, NL-5683 KH Best Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax: (+31) (0) 499 33 69 70
E-mail: [email protected]
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/

Hi Mike,

Sorry for the late reply. I was involved in other work these days. Please see my explanation below.

Most files under /etc are configuration files. If we don't default CONFFILES to ${sysconfdir}, then we have to patch a lot of recipes with CONFFILES_xxx = "xxxx" lines.

The fact is that when people write recipes, they often forget about CONFFILES. The typical example is the shadow recipe. And I can easily give you more examples.

And it's not uncommon that people forget to check CONFFILES when they update the recipe. When the recipe is updated to a new version, it's possible that the CONFFILES should change. The typical example is the systemd recipe.

*The bad effect of the above? On-target package upgrades override user changes. That's a really annoying problem, isn't it?* Users of OE based projects have complained about this. There were patches on OE which fixed several recipes. But the solution doesn't scale.

The patchset is an effort to change this situation.

Best Regards,
Chen Qi

Definitely true!

No, similar lines will be required for quite a few recipes...

Well, there are a lot of those configuration files, that only the distribution will care about. Especially if we're talking embedded devices.

This patch won't help this, it just shifts everything 180 degrees around. Now we have to explicitly remove a number of files from CONFFILES in a number of packages; which obviously is something that people will forget...

I tend to agree with Mike here. It's better to explicitly add files (or directories) in each recipe, as that will make it more obvious which files won't get overwritten during a package upgrade. Sure, short-term this might mean more patches (I'm nt really sure of the status regarding this).

But more importantly, it will keep the determinism and explicit marking of CONFFILES. Again, remember that we're primarily talking about embedded devices here, where the distibution/distributor quite likely need some more control, compared to a regular desktop distribution.

I don't think we'll gain that much from this patch; rather we'll loose...

Cheers,
Anders

Anders Darander
ChargeStorm AB / eStorm AB

Hi Anders,

I read your comments carefully and thought for a while. Sorry but I still insist my opinion. Please see reasons below.

First of all, I don't think this issue has anything to do with the 'embedded devices' or 'desktop distribution' thing. The two concepts are too large and should not become the reason. Some desktop distributions support image based upgrades. Many small embedded devices should only do image based upgrades. Most desktop distributions support package based upgrades, some embedded devices should also support it. What we are talking here is on-target package upgrades. If the distributor wants to have full control, then its users/customers should be suggested to only do image-based upgrades.
But now, let's just focus on package based upgrades.

We have to be aware that package managers are designed to handle config files specially. Why they are designed to do so? Because users should not be surprised. Take 'rpm' as an example.

File marked as Changed in update RPM? On-disk file untouched On-disk file edited
[default] No File from update File from update
Yes File from update File from update
%config No File from update Edited file
Yes File from update File from update, edited file in .rpmsave
%config(noreplace) No File from update Edited file
Yes File from update Edited file, file from the update in .rpmnew

You can see that it's pretty reasonable.
The problem we have now is that if the user edit a configuration file (e.g. /etc/login.defs), the file will still be overridden by upgrades even if the file doesn't change in the new package.

Now the focus is whether we should take ${sysconfdir} as the default value for CONFFILES.
I strongly think we should. Why? We can easily make use of the power of package managers for dealing with configuration files.
If we don't do so, very likely the user changes would be overridden silently without informing the user.
This should not happen, especially when the files are under /etc. Otherwise we will get angry users or customers. How are we gonna explain to them? I don't 'embedded device' could be a satisfactory reason. Please see the number below. Hope this will reveal the situation of CONFFILES in OE.
chenqi@pek-hostel-deb01:~/poky [0] $ find meta meta-openembedded/ -name "*.bb" | wc -l
1897
chenqi@pek-hostel-deb01:~/poky [0] $ find meta meta-openembedded/ -name "*.bb" -o -name "*.inc" | xargs grep CONFFILES | wc -l
86

Best Regards,
Chen Qi

We need to use 'purge' instead of 'remove' for debian package backend when removing packages at rootfs time. This is because that 'remove' command for 'dpkg' and 'apt-get' does not remove configuration files.

---
 meta/lib/oe/package_manager.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 69100f1..c8aa61c 100644
@@ -1628,10 +1628,10 @@ class DpkgPM(PackageManager):
     def remove(self, pkgs, with_dependencies=True):
         if with_dependencies:
             os.environ['APT_CONFIG'] = self.apt_conf_file
-            cmd = "%s remove %s" % (self.apt_get_cmd, ' '.join(pkgs))
+            cmd = "%s purge %s" % (self.apt_get_cmd, ' '.join(pkgs))
         else:
             cmd = "%s --admindir=%s/var/lib/dpkg --instdir=%s" \
-                  " -r --force-depends %s" % \
+                  " -P --force-depends %s" % \
                   (bb.utils.which(os.getenv('PATH'), "dpkg"),
                    self.target_rootfs, self.target_rootfs, ' '.join(pkgs))
 

1.9.1

Use '-f' ('--force') option so that while removing packages using deb, we don't fail because of the following error:

update-rc.d: $initd/$bn exists during rc.d purge (use -f to force)

Using '-f' option would make this a warning but continue to execute.

update-rc.d: $initd/$bn exists during rc.d purge (continuing)

We need this option now because things under /etc/init.d/ are configuration files by default. As dpkg doesn't remove configuration files when using 'remove', errors appear.

---
 meta/classes/update-rc.d.bbclass | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index bc1aa7d..a9c0323 100644
@@ -48,9 +48,9 @@ fi
 updatercd_postrm() {
 if type update-rc.d >/dev/null 2>/dev/null; then
 	if [ -n "$D" ]; then
-		OPT="-r $D"
+		OPT="-f -r $D"
 	else
-		OPT=""
+		OPT="-f"
 	fi
 	update-rc.d $OPT ${INITSCRIPT_NAME} remove
 fi

1.9.1

If /etc/login.defs is treated as a configuration file, then we would meet errors at do_rootfs time telling us that useradd/groupadd cannot execute correctly.

This is because the dpkg handles config file specially, the login.defs is temporarily renamed as login.defs.dpkg-new.

How ubuntu deals the user adding problem? The do it at postinst of the package. And, in postinsts the packages would possibly do chown of its file or directories.

The above strategy is not suitable for OE. Because we do chown in do_install and add user/group in preinst scripts of the packages.

That's why we need this patch so that do_rootfs don't fail.

---
 meta/classes/useradd.bbclass | 10 ++++++++++
 1 file changed, 10 insertions(+)

index 0b9a843..e443f84 100644
@@ -24,6 +24,16 @@ if test "x$D" != "x"; then
 	# Installing into a sysroot
 	SYSROOT="$D"
 	OPT="--root $D"
+
+	# Make sure login.defs is there, this is to make debian package backend work
+	# correctly while doing rootfs.
+	# The problem here is that if /etc/login.defs is treated as a config file for
+	# shadow package, then while performing preinsts for packages that depend on
+	# shadow, there might only be /etc/login.def.dpkg-new there in root filesystem.
+	if [ ! -e $D${sysconfdir}/login.defs -a -e $D${sysconfdir}/login.defs.dpkg-new ]; then
+	    cp $D${sysconfdir}/login.defs.dpkg-new $D${sysconfdir}/login.defs
+	fi
+
 	# user/group lookups should match useradd/groupadd --root
 	export PSEUDO_PASSWD="$SYSROOT:${STAGING_DIR_NATIVE}"
 fi

1.9.1