Re: [OE-Core][PATCH v2 1/3] rootfs: Add debugfs package db file copy and cleanup

[Alex Kiernan <alex.kiernan@gmail.com>]

On Mon, Aug 7, 2023 at 9:21 PM Peter Kjellerstedt
<peter.kjellerstedt@axis.com> wrote:
>
> > -----Original Message-----
> > From: openembedded-core@lists.openembedded.org <openembedded-core@lists.openembedded.org> On Behalf Of Alex Kiernan
> > Sent: den 20 juli 2023 12:20
> > To: openembedded-core@lists.openembedded.org
> > Cc: Alex Kiernan <alex.kiernan@gmail.com>
> > Subject: [OE-Core][PATCH v2 1/3] rootfs: Add debugfs package db file copy and cleanup
> >
> > When copying the package database files for the debugfs, add individual
> > file copy as well as tree copying. After the debug rootfs has been
> > created, cleanup the package files.
> >
> > This then allows us to avoid a problem where (for rpm at least)
> > extraneous files in the debug rootfs would cause failures during
> > oe-selftest because some files existed in both regular and debugfs
> > images.
> >
> > Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com>
> > ---
> >
> > Changes in v2:
> > - New in v2
> >
> >  meta/lib/oe/rootfs.py | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/meta/lib/oe/rootfs.py b/meta/lib/oe/rootfs.py
> > index 890ba5f03984..1a48ed10b3f6 100644
> > --- a/meta/lib/oe/rootfs.py
> > +++ b/meta/lib/oe/rootfs.py
> > @@ -106,7 +106,7 @@ class Rootfs(object, metaclass=ABCMeta):
> >      def _cleanup(self):
> >          pass
> >
> > -    def _setup_dbg_rootfs(self, dirs):
> > +    def _setup_dbg_rootfs(self, package_paths):
> >          gen_debugfs = self.d.getVar('IMAGE_GEN_DEBUGFS') or '0'
> >          if gen_debugfs != '1':
> >             return
> > @@ -122,11 +122,12 @@ class Rootfs(object, metaclass=ABCMeta):
> >          bb.utils.mkdirhier(self.image_rootfs)
> >
> >          bb.note("  Copying back package database...")
> > -        for dir in dirs:
> > -            if not os.path.isdir(self.image_rootfs + '-orig' + dir):
> > -                continue
> > -            bb.utils.mkdirhier(self.image_rootfs + os.path.dirname(dir))
> > -            shutil.copytree(self.image_rootfs + '-orig' + dir, self.image_rootfs + dir, symlinks=True)
> > +        for path in package_paths:
> > +            bb.utils.mkdirhier(self.image_rootfs + os.path.dirname(path))
> > +            if os.path.isdir(self.image_rootfs + '-orig' + path):
> > +                shutil.copytree(self.image_rootfs + '-orig' + path, self.image_rootfs + path, symlinks=True)
> > +            elif os.path.isfile(self.image_rootfs + '-orig' + path):
> > +                shutil.copyfile(self.image_rootfs + '-orig' + path, self.image_rootfs + path)
> >
> >          # Copy files located in /usr/lib/debug or /usr/src/debug
> >          for dir in ["/usr/lib/debug", "/usr/src/debug"]:
> > @@ -162,6 +163,13 @@ class Rootfs(object, metaclass=ABCMeta):
> >              bb.note("  Install extra debug packages...")
> >              self.pm.install(extra_debug_pkgs.split(), True)
> >
> > +        bb.note("  Removing package database...")
> > +        for path in package_paths:
> > +            if os.path.isdir(self.image_rootfs + path):
> > +                shutil.rmtree(self.image_rootfs + path)
> > +            elif os.path.isfile(self.image_rootfs + path):
> > +                os.remove(self.image_rootfs + path)
>
> What's the reason to do it like this rather than calling
> self.pm.remove_packaging_data()?
>

Just looking now, I suspect only that I didn't notice that it existed!
Though looking across the other packaging implementations, it looks
like you'd get different things removed than were passed initially.
Certainly looks like a refactor would be in order.

> I also just noticed that we apparently have a local change that does the
> above, but where we also do:
>
>      # Remove /etc as it may clash with rootfs.  Also saves some space.
>      bb.utils.remove(oe.path.join(self.image_rootfs, 'etc'), True)
>
> I do not know if that would be appropriate to do here as well?
>

The clash was exactly what I was fixing here (gshadow), but prior to
these two changes you got the whole of /etc copied, so a clash was
exactly what you were in danger of running into - anything that
clashes now, I think wants investigating as to why, rather than just
wiping out `/etc` entirely.

-- 
Alex Kiernan