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

[Allan McRae]

On 04/03/13 23:32, Andrew Gregory wrote:
> 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?
> 

We should have an error if the path starts with "/" so it would be great
if you could add that.  The testsuite does wired things if you do use
patch starting with / anyway.

The rest of your explanation makes sense, so no need to change that.