[pacman-dev] [PATCHv2] pmpkg: add missing directories to test packages

[Andrew Gregory]

On 03/04/13 at 05:51pm, Allan McRae wrote:
> On 04/03/13 09:54, Andrew Gregory wrote:
> > Several tests require complete file lists in order to provide accurate
> > results.  These can be non-obvious.  Adding missing parent directories
> > helps insure the integrity of tests against human error.  Filling in
> > parent directories also allows us to check that file lists are actually
> > valid.
> > 
> > There didn't seem to be a good place to do this that was always
> > guaranteed to be run, so this adds a finalize() function to packages
> > that will always be run before the package is actually used to allow for
> > this type of tidying.
> > 
> > Fixes FS#30723
> > 
> > Signed-off-by: Andrew Gregory <andrew.gregory.8 at gmail.com>
> > ---
> > 
> > Not quite as simple as I had hoped...
> > 
> >  test/pacman/pmdb.py   |  2 +-
> >  test/pacman/pmpkg.py  | 49 ++++++++++++++++++++++++++++---------------------
> >  test/pacman/pmtest.py |  3 +++
> >  3 files changed, 32 insertions(+), 22 deletions(-)
> > 
> > diff --git a/test/pacman/pmdb.py b/test/pacman/pmdb.py
> > index b694dff..3e9d305 100644
> > --- a/test/pacman/pmdb.py
> > +++ b/test/pacman/pmdb.py
> > @@ -219,7 +219,7 @@ def db_write(self, pkg):
> >          # files and install
> >          if self.is_local:
> >              data = []
> > -            make_section(data, "FILES", pkg.full_filelist())
> > +            make_section(data, "FILES", pkg.filelist())
> >              make_section(data, "BACKUP", pkg.local_backup_entries())
> >              entry["files"] = "\n".join(data)
> >  
> > diff --git a/test/pacman/pmpkg.py b/test/pacman/pmpkg.py
> > index c0c9f13..1e019c8 100644
> > --- a/test/pacman/pmpkg.py
> > +++ b/test/pacman/pmpkg.py
> > @@ -69,6 +69,7 @@ def __init__(self, name, version = "1.0-1"):
> >              "post_upgrade": "",
> >          }
> >          self.path = None
> > +        self.finalized = False
> >  
> >      def __str__(self):
> >          s = ["%s" % self.fullname()]
> > @@ -182,27 +183,33 @@ def install_package(self, root):
> >              if os.path.isfile(path):
> >                  os.utime(path, (355, 355))
> >  
> > -    def full_filelist(self):
> > -        """Generate a list of package files.
> > -
> > -        Each path is decomposed to generate the list of all directories leading
> > -        to the file.
> > -
> > -        Example with 'usr/local/bin/dummy':
> > -        The resulting list will be:
> > -            usr/
> > -            usr/local/
> > -            usr/local/bin/
> > -            usr/local/bin/dummy
> > -        """
> > -        file_set = set()
> > -        for name in self.files:
> > -            name = self.parse_filename(name)
> > -            file_set.add(name)
> > -            while "/" in name:
> > -                name, tmp = name.rsplit("/", 1)
> > -                file_set.add(name + "/")
> > -        return sorted(file_set)
> > +    def filelist(self):
> > +        """Generate a list of package files."""
> > +        return sorted([self.parse_filename(f) for f in self.files])
> > +
> > +    def finalize(self):
> > +        """Perform any necessary operations to ready the package for use."""
> > +        if self.finalized:
> > +            return
> > +
> > +        # add missing parent dirs to file list
> > +        # use bare file names so trailing ' -> ', '*', etc don't throw off the
> > +        # checks for existing files
> > +        file_names = self.filelist()
> > +        for name in file_names:
> > +            name = os.path.dirname(name.rstrip("/"))
> > +
> > +            while name and name != "/":
> > +                if name in file_names:
> > +                    # path exists as both a file and a directory
> > +                    raise ValueError("Path '%s' duplicated in filelist." % name)
> > +                elif name + "/" not in file_names:
> > +                    file_names.append(name + "/")
> > +                    self.files.append(name + "/")
> > +                name = os.path.dirname(name)
> > +        self.files.sort()
> > +
> 
> Is there are reason not to use the old full_filelist function here?  I
> said ti was the better way in my last email...  I see the error for
> conflicting files/directories has been added - so that is good.  But in
> pactests can not start with a "/", so you should use "while "/" in name"
> 

full_filelist couldn't be used because it discarded file type, mode,
etc. information.  At this stage in the test that rule against leading
"/" in paths has not been enforced, so an absolute path would cause an
endless loop.  I could add an explicit error for that and reduce the
loop to 'while name' but 'while "/" in name" won't work without
rearranging the loop because the top directory won't have "/" in it.
Aside from that, the only real difference I see is that full_filelist
used direct string manipulation rather than os.path, is there
a preference for that?

apg