[manifestdestiny] improve manifestparser.convert

RESOLVED FIXED

Status

Testing
Mozbase
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Jeff Hammel, Assigned: Jeff Hammel)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
https://github.com/mozilla/mozbase/blob/master/manifestdestiny/manifestparser/manifestparser.py#L785

The convert function was written in haste for the initial rollout of
manifestdestiny as a POC of transformation of the (recursive) creation
of manifests from a directory structure.  However, there are some, um,
stnanks (http://www.hrwiki.org/wiki/Stnanks) if not outright bugs that
should be addressed:

  def convert(directories, pattern=None, ignore=(), write=None):

^ Should also have a parameter, overwrite, that determines whether
manifests of the name given by write should be overwritten

"""
            # filter out directory names
            dirnames = [ i for i in dirnames if i not in ignore ]
            dirnames.sort()

            # reference only the subdirectory
            _dirpath = dirpath
            dirpath = dirpath.split(directory,
            1)[-1].strip(os.path.sep)

            if dirpath.split(os.path.sep)[0] in ignore:
                continue
"""

This is a misuse of dirnames which can be filtered in place.

More substantially, there is a subtle flaw: how do you know that the
subdirectory has any directory/files?  AIUI, right now things
work...we just write empty manifests.  We can continue to support
this, but IMHO it is better to not write manifests if all descendents
are empty (the chain is broken).

Convert should support more than one pattern.

Convert should have more tests.

- should be method of ManifestParser:
  @classmethod
  def directory_to_manifest(cls):
    "returns ManifestParser object"

- should be able to choose relative v absolute paths;
  if `write` is relative path and directories are descendents,
  paths should be relative;
  if `write` is an absolute path or directories are not
  descendents, path should be absolute.
  While this puts higher complexity on `write`, it is not
  ambiguous:  pass (e.g.) './manifest.ini' v 'manifest.ini'
  if relative paths to the current directory is desired,
  or (e.g.) `os.path.abspath('manifest.ini')` if absolute
  paths are desired.
  An override could be introduced, but this mostly does the right
  thing.

- write could take a file-like object; in this case,
  paths will be absolute
(Assignee)

Comment 1

4 years ago
Created attachment 787108 [details] [diff] [review]
bug-902610.diff

WIP; but not a bad start
(Assignee)

Comment 2

4 years ago
Created attachment 789647 [details] [diff] [review]
bug-902610.diff
Attachment #787108 - Attachment is obsolete: true
Attachment #789647 - Flags: review?(cmanchester)
(Assignee)

Comment 3

4 years ago
(In reply to Jeff Hammel [:jhammel] from comment #2)
> Created attachment 789647 [details] [diff] [review]
> bug-902610.diff

I rolled my own cached directory contents for os.walk; I could have also used https://pypi.python.org/pypi/pathfinder but I'm not sure what are attitude is re 3rd party packages ; there's also the widely used path.py but it seems less applicable for this particular issue.
(Assignee)

Updated

4 years ago
Assignee: nobody → jhammel
I get the following test failures when running locally:

http://pastebin.mozilla.org/2884112
Comment on attachment 789647 [details] [diff] [review]
bug-902610.diff

Review of attachment 789647 [details] [diff] [review]:
-----------------------------------------------------------------

This patch seems to have some code cleanup changes that don't seem directly related to this bug. I don't know what the convention is in these cases, but I'm having a little trouble parsing cleanup vs. changes directly related to this bug - Jeff, would you mind splitting these separate patch or follow-up before I review this?
Attachment #789647 - Flags: review?(cmanchester)
(Assignee)

Comment 6

4 years ago
(In reply to Chris Manchester [:chmanchester] from comment #5)
> Comment on attachment 789647 [details] [diff] [review]
> bug-902610.diff
> 
> Review of attachment 789647 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch seems to have some code cleanup changes that don't seem directly
> related to this bug. I don't know what the convention is in these cases, but
> I'm having a little trouble parsing cleanup vs. changes directly related to
> this bug - Jeff, would you mind splitting these separate patch or follow-up
> before I review this?

There's really not much general cleanup; the '###' comments up top and a few lines in read_ini are all that's really general cleanup.  While there are several improvements made here, like having a file-like object manifest in ManifestParser.read, they were made either directly for support of the from_directory functionality or for testing of that functionality.

I can't reproduce the failures in the pastebin .  Maybe OSX vs linux difference? :(
(Assignee)

Comment 7

4 years ago
(In reply to Chris Manchester [:chmanchester] from comment #4)
> I get the following test failures when running locally:
> 
> http://pastebin.mozilla.org/2884112

======================================================================
FAIL: test_relpath (test_convert_directory.TestDirectoryConversion)
test convert `relative_to` functionality
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mozilla/mozbase/manifestdestiny/tests/test_convert_directory.py", line 130, in test_relpath
    files)
AssertionError: Lists differ: ['../../../../../../../../var/... != ['../bar', '../fleem', '../foo...

First differing element 0:
../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmpGUOPxX/bar
../bar

+ ['../bar', '../fleem', '../foo', 'subfile']
- ['../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmpGUOPxX/bar',
-  '../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmpGUOPxX/fleem',
-  '../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmpGUOPxX/foo',
-  '../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmpGUOPxX/subdir/subfile']

======================================================================
FAIL: test_relpath_implicit (test_convert_directory.TestDirectoryConversion)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mozilla/mozbase/manifestdestiny/tests/test_convert_directory.py", line 147, in test_relpath_implicit
    files)
AssertionError: Lists differ: ['../../../../../../../../var/... != ['../bar', '../fleem', '../foo...

First differing element 0:
../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmp3XtDGS/bar
../bar

First list contains 1 additional elements.
First extra element 4:
../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmp3XtDGS/subdir/subfile

+ ['../bar', '../fleem', '../foo', 'subfile']
- ['../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmp3XtDGS/bar',
-  '../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmp3XtDGS/fleem',
-  '../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmp3XtDGS/foo',
-  '../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmp3XtDGS/subdir/manifest.ini',
-  '../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmp3XtDGS/subdir/subfile']
Comment on attachment 789647 [details] [diff] [review]
bug-902610.diff

Review of attachment 789647 [details] [diff] [review]:
-----------------------------------------------------------------

Here are some initial comments. I didn't dig into the main logic of from_directories - I'd like to see what you think of the idea of designing away from the "write" parameter having such subtle meanings in various cases. Let me see if I've understood this ok - when "write" is a relative or absolute path, everything is written to a single manifest file specified by that path, while if it is just a name, a manifest with that name is written in each directory traversed?

::: manifestdestiny/manifestparser/manifestparser.py
@@ +12,4 @@
>             'ManifestParser', 'TestManifest', 'convert', # manifest handling
>             'parse', 'ParseError', 'ExpressionParser'] # conditional expression parser
>  
> +import fnmatch

This gets changed from a relative to an absolute import, but the existing call site of fnmatch is unchanged. I think this reveals missing test coverage here: https://github.com/mozilla/mozbase/blob/master/manifestdestiny/manifestparser/manifestparser.py#L546

@@ +378,5 @@
>  
> +        # get directory of this file if not file-like object
> +        here = None
> +        if isinstance(filename, string):
> +            here = os.path.dirname(os.path.abspath(filename))

If I were reading this to try to debug something in the manifest parser, I would really expect a variable named filename to always be a string. What is the benefit we derive from letting this be one or the other rather than adding a new parameter? On the other hand, this is an internal method, so maybe not a huge deal.

@@ +403,2 @@
>                      else:
> +                        sys.stderr.write("%s\n" % message)

Removing the continue here is a pretty significant change in behavior, isn't it? If we recurse with a file that doesn't exist I guess it will just conk out in the next call to read_ini, is that what you had in mind here?

@@ +411,5 @@
>              test['name'] = section
> +            if isinstance(filename, string):
> +                test['manifest'] = os.path.abspath(filename)
> +            else:
> +                test['manifest'] = None # file pointer

Maybe you can explain a bit about the benefits of dynamically checking this vs. having explicit parameters for fp's vs. names. It seems to be a pattern in this file.

@@ +765,5 @@
> +        or (e.g.) `os.path.abspath('manifest.ini')` if absolute
> +        paths are desired.
> +
> +        Write can also take a file-like object; in this case,
> +        paths will be governed by `relative_to`.

I didn't get into the details of this function, but I wanted to raise the question of whether such an overloaded parameter is worth the complexity to test and maintain (I don't think I saw a test for the case of an absolute path or directory that isn't a descendant).

::: manifestdestiny/tests/test_convert_directory.py
@@ +151,5 @@
> +#                                         if line.strip()]),
> +# """[../bar]
> +# [../fleem]
> +# [../foo]
> +# [subfile]""")

Let's get this working. Maybe parse the resulting manifest file and compare based on the generated manifest object? This test is about testing the functionality of convert, not the basic parsing, so I don't see the need to compare the raw string.

@@ +174,5 @@
> +        newtempdir = tempfile.mkdtemp()
> +        manifest_file = os.path.join(newtempdir, 'manifest.ini')
> +        manifest_contents = str(convert([tempdir], relative_to=tempdir))
> +        with file(manifest_file, 'w') as f:
> +                f.write(manifest_contents)

Nit: indentation.
Attachment #789647 - Flags: feedback+
Something I suggested on IRC:
What about two parameters to the convert method to do what "write" does now: one representing the local name of any generated manifests, that is always a string and not a path, and another that is always a path to a master manifest that will contain all the data of the subdirectories.
(Assignee)

Comment 10

4 years ago
(In reply to Chris Manchester [:chmanchester] from comment #8)
> Comment on attachment 789647 [details] [diff] [review]
> bug-902610.diff
> 
> Review of attachment 789647 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Here are some initial comments. I didn't dig into the main logic of
> from_directories - I'd like to see what you think of the idea of designing
> away from the "write" parameter having such subtle meanings in various
> cases. Let me see if I've understood this ok - when "write" is a relative or
> absolute path, everything is written to a single manifest file specified by
> that path, while if it is just a name, a manifest with that name is written
> in each directory traversed?

Yes, that is correct.  I'll fully agree that the parameter is overloaded, a bit too implicit, and a bit too complicated.  I did it this way, however, so that there is a definitive mapping of (parameter)->(outcome).  Regarding comment 9:

> Something I suggested on IRC: What about two parameters to the
> convert method to do what "write" does now: one representing the
> local name of any generated manifests, that is always a string and
> not a path, and another that is always a path to a master manifest
> that will contain all the data of the subdirectories.

Do you mean e.g.

def from_directories(..., manifest=None, include=None, relative_to=None, ...):
    """
    ...
    - manifest : path to master manifest to write
    - include : filename of manifest to write to each subdirectory, if given, to be [include:]ed in the master manifest;  otherwise all items will be noted in the master manifest
    - relative_to : if include is None, manifest items will be written relative to this path; if None, paths will be absolute
    """

Alternatively, if relative_to is None, paths could be relative to `manifest` or absolute if `manifest` is an absolute path (my preference in this instance).

So....I am fine with this, if this is what is generally desired.  I generally dislike parameters that are only applicable depending on other parameters, which is why I overloaded `write`.

I'm more inclined to split this into two (or more) functions: one to write a single manifest with either relative or absolute paths, and one to write manifests that [include:]s subdirectories. (I might split into more support functions as well.)

What do people think of this?
 
> ::: manifestdestiny/manifestparser/manifestparser.py
> @@ +12,4 @@
> >             'ManifestParser', 'TestManifest', 'convert', # manifest handling
> >             'parse', 'ParseError', 'ExpressionParser'] # conditional expression parser
> >  
> > +import fnmatch
> 
> This gets changed from a relative to an absolute import, but the existing
> call site of fnmatch is unchanged. I think this reveals missing test
> coverage here:
> https://github.com/mozilla/mozbase/blob/master/manifestdestiny/
> manifestparser/manifestparser.py#L546

I'm not sure what you mean here. Can you elaborate?

> @@ +378,5 @@
> >  
> > +        # get directory of this file if not file-like object
> > +        here = None
> > +        if isinstance(filename, string):
> > +            here = os.path.dirname(os.path.abspath(filename))
> 
> If I were reading this to try to debug something in the manifest parser, I
> would really expect a variable named filename to always be a string. 

I kept the name to avoid an API change; I'd suggest changing in a follow-up for atomicity and to avoid breakages.  I can note this in a comment.

> What is
> the benefit we derive from letting this be one or the other rather than
> adding a new parameter? 

I'm not sure what you're suggesting here.  Can you elaborate?

> On the other hand, this is an internal method, so
> maybe not a huge deal.
> 
> @@ +403,2 @@
> >                      else:
> > +                        sys.stderr.write("%s\n" % message)
> 
> Removing the continue here is a pretty significant change in behavior, isn't
> it? If we recurse with a file that doesn't exist I guess it will just conk
> out in the next call to read_ini, is that what you had in mind here?

You are correct.  I did not mean to remove the continue, just add the message.  Will restore.
 
> @@ +411,5 @@
> >              test['name'] = section
> > +            if isinstance(filename, string):
> > +                test['manifest'] = os.path.abspath(filename)
> > +            else:
> > +                test['manifest'] = None # file pointer
> 
> Maybe you can explain a bit about the benefits of dynamically checking this
> vs. having explicit parameters for fp's vs. names. It seems to be a pattern
> in this file.

The general idea is that you can pass in a file-like object or the name of a file to be opened, but not both.  IMHO, there's no reason to allow both since they cannot both work together.  The pattern isn't just in this file, but is common in python code; however, far from consistently.

This could also be done, in this file and elsewhere, as a decorator (implementation available on request).
 
> @@ +765,5 @@
> > +        or (e.g.) `os.path.abspath('manifest.ini')` if absolute
> > +        paths are desired.
> > +
> > +        Write can also take a file-like object; in this case,
> > +        paths will be governed by `relative_to`.
> 
> I didn't get into the details of this function, but I wanted to raise the
> question of whether such an overloaded parameter is worth the complexity to
> test and maintain (I don't think I saw a test for the case of an absolute
> path or directory that isn't a descendant).

No, testing is incomplete.  OTOH, there's many untested code paths in manifestparser and elsewhere and my priority is getting this landed.  As far as worth, see above.

> ::: manifestdestiny/tests/test_convert_directory.py
> @@ +151,5 @@
> > +#                                         if line.strip()]),
> > +# """[../bar]
> > +# [../fleem]
> > +# [../foo]
> > +# [subfile]""")
> 
> Let's get this working. Maybe parse the resulting manifest file and compare
> based on the generated manifest object? This test is about testing the
> functionality of convert, not the basic parsing, so I don't see the need to
> compare the raw string.

Fair 'nuff

> @@ +174,5 @@
> > +        newtempdir = tempfile.mkdtemp()
> > +        manifest_file = os.path.join(newtempdir, 'manifest.ini')
> > +        manifest_contents = str(convert([tempdir], relative_to=tempdir))
> > +        with file(manifest_file, 'w') as f:
> > +                f.write(manifest_contents)
> 
> Nit: indentation.

Thanks!
(In reply to Jeff Hammel [:jhammel] from comment #10)
> (In reply to Chris Manchester [:chmanchester] from comment #8)
> > Comment on attachment 789647 [details] [diff] [review]
> > bug-902610.diff
> > 
> > Review of attachment 789647 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Here are some initial comments. I didn't dig into the main logic of
> > from_directories - I'd like to see what you think of the idea of designing
> > away from the "write" parameter having such subtle meanings in various
> > cases. Let me see if I've understood this ok - when "write" is a relative or
> > absolute path, everything is written to a single manifest file specified by
> > that path, while if it is just a name, a manifest with that name is written
> > in each directory traversed?
> 
> Yes, that is correct.  I'll fully agree that the parameter is overloaded, a
> bit too implicit, and a bit too complicated.  I did it this way, however, so
> that there is a definitive mapping of (parameter)->(outcome).  Regarding
> comment 9:
> 
> > Something I suggested on IRC: What about two parameters to the
> > convert method to do what "write" does now: one representing the
> > local name of any generated manifests, that is always a string and
> > not a path, and another that is always a path to a master manifest
> > that will contain all the data of the subdirectories.
> 
> Do you mean e.g.
> 
> def from_directories(..., manifest=None, include=None, relative_to=None,
> ...):
>     """
>     ...
>     - manifest : path to master manifest to write
>     - include : filename of manifest to write to each subdirectory, if
> given, to be [include:]ed in the master manifest;  otherwise all items will
> be noted in the master manifest
>     - relative_to : if include is None, manifest items will be written
> relative to this path; if None, paths will be absolute
>     """
> 
> Alternatively, if relative_to is None, paths could be relative to `manifest`
> or absolute if `manifest` is an absolute path (my preference in this
> instance).
> 
> So....I am fine with this, if this is what is generally desired.  I
> generally dislike parameters that are only applicable depending on other
> parameters, which is why I overloaded `write`.
> 
> I'm more inclined to split this into two (or more) functions: one to write a
> single manifest with either relative or absolute paths, and one to write
> manifests that [include:]s subdirectories. (I might split into more support
> functions as well.)
> 
> What do people think of this?
>  

I can't speak for others, but I'm in favor of splitting this functionality into two functions to ease cognitive overhead for users.

> > ::: manifestdestiny/manifestparser/manifestparser.py
> > @@ +12,4 @@
> > >             'ManifestParser', 'TestManifest', 'convert', # manifest handling
> > >             'parse', 'ParseError', 'ExpressionParser'] # conditional expression parser
> > >  
> > > +import fnmatch
> > 
> > This gets changed from a relative to an absolute import, but the existing
> > call site of fnmatch is unchanged. I think this reveals missing test
> > coverage here:
> > https://github.com/mozilla/mozbase/blob/master/manifestdestiny/
> > manifestparser/manifestparser.py#L546
> 
> I'm not sure what you mean here. Can you elaborate?

The line I linked calls fnmatch as a function, so with the switch to 'import fnmatch' from 'from fnmatch import fnmatch', I get this:

>>> from manifestparser import ManifestParser
>>> mp = ManifestParser(['/Users/mozilla/m-c/testing/xpcshell/example/unit/xpcshell.ini'])
>>> mp.verifyDirectory(['/Users/mozilla/m-c/testing/xpcshell/example/unit'], '*.js')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mozilla/mozbase/manifestdestiny/manifestparser/manifestparser.py", line 568, in verifyDirectory
    if fnmatch(filename, pattern)]
TypeError: 'module' object is not callable

> 
> > @@ +378,5 @@
> > >  
> > > +        # get directory of this file if not file-like object
> > > +        here = None
> > > +        if isinstance(filename, string):
> > > +            here = os.path.dirname(os.path.abspath(filename))
> > 
> > If I were reading this to try to debug something in the manifest parser, I
> > would really expect a variable named filename to always be a string. 
> 
> I kept the name to avoid an API change; I'd suggest changing in a follow-up
> for atomicity and to avoid breakages.  I can note this in a comment.

I see. This is the basically the same issue as below about filename vs. file object.

> 
> > What is
> > the benefit we derive from letting this be one or the other rather than
> > adding a new parameter?
> 
> I'm not sure what you're suggesting here.  Can you elaborate?

You've answered below.

> 
> > On the other hand, this is an internal method, so
> > maybe not a huge deal.
> > 
> > @@ +403,2 @@
> > >                      else:
> > > +                        sys.stderr.write("%s\n" % message)
> > 
> > Removing the continue here is a pretty significant change in behavior, isn't
> > it? If we recurse with a file that doesn't exist I guess it will just conk
> > out in the next call to read_ini, is that what you had in mind here?
> 
> You are correct.  I did not mean to remove the continue, just add the
> message.  Will restore.
>  
> > @@ +411,5 @@
> > >              test['name'] = section
> > > +            if isinstance(filename, string):
> > > +                test['manifest'] = os.path.abspath(filename)
> > > +            else:
> > > +                test['manifest'] = None # file pointer
> > 
> > Maybe you can explain a bit about the benefits of dynamically checking this
> > vs. having explicit parameters for fp's vs. names. It seems to be a pattern
> > in this file.
> 
> The general idea is that you can pass in a file-like object or the name of a
> file to be opened, but not both.  IMHO, there's no reason to allow both
> since they cannot both work together.  The pattern isn't just in this file,
> but is common in python code; however, far from consistently.
> 
> This could also be done, in this file and elsewhere, as a decorator
> (implementation available on request).

I'm curious about what the decorator might look like, but in general if this is a pattern in Python, it's probably something I'll have to come to accept.

>  
> > @@ +765,5 @@
> > > +        or (e.g.) `os.path.abspath('manifest.ini')` if absolute
> > > +        paths are desired.
> > > +
> > > +        Write can also take a file-like object; in this case,
> > > +        paths will be governed by `relative_to`.
> > 
> > I didn't get into the details of this function, but I wanted to raise the
> > question of whether such an overloaded parameter is worth the complexity to
> > test and maintain (I don't think I saw a test for the case of an absolute
> > path or directory that isn't a descendant).
> 
> No, testing is incomplete.  OTOH, there's many untested code paths in
> manifestparser and elsewhere and my priority is getting this landed.  As far
> as worth, see above.

I think I'm missing the big picture here - what does this block?
(Assignee)

Comment 12

4 years ago
(In reply to Chris Manchester [:chmanchester] from comment #11)
> (In reply to Jeff Hammel [:jhammel] from comment #10)
<snip/>
> > def from_directories(..., manifest=None, include=None, relative_to=None,
> > ...):
> >     """
> >     ...
> >     - manifest : path to master manifest to write
> >     - include : filename of manifest to write to each subdirectory, if
> > given, to be [include:]ed in the master manifest;  otherwise all items will
> > be noted in the master manifest
> >     - relative_to : if include is None, manifest items will be written
> > relative to this path; if None, paths will be absolute
> >     """
> > 
> > Alternatively, if relative_to is None, paths could be relative to `manifest`
> > or absolute if `manifest` is an absolute path (my preference in this
> > instance).
> > 
> > So....I am fine with this, if this is what is generally desired.  I
> > generally dislike parameters that are only applicable depending on other
> > parameters, which is why I overloaded `write`.
> > 
> > I'm more inclined to split this into two (or more) functions: one to write a
> > single manifest with either relative or absolute paths, and one to write
> > manifests that [include:]s subdirectories. (I might split into more support
> > functions as well.)
> > 
> > What do people think of this?
> >  
> 
> I can't speak for others, but I'm in favor of splitting this functionality
> into two functions to ease cognitive overhead for users.

Cool.  I'll put up a proposed API soon.

> > > ::: manifestdestiny/manifestparser/manifestparser.py
> > > @@ +12,4 @@
> > > >             'ManifestParser', 'TestManifest', 'convert', # manifest handling
> > > >             'parse', 'ParseError', 'ExpressionParser'] # conditional expression parser
> > > >  
> > > > +import fnmatch
> > > 
> > > This gets changed from a relative to an absolute import, but the existing
> > > call site of fnmatch is unchanged. I think this reveals missing test
> > > coverage here:
> > > https://github.com/mozilla/mozbase/blob/master/manifestdestiny/
> > > manifestparser/manifestparser.py#L546
> > 
> > I'm not sure what you mean here. Can you elaborate?
> 
> The line I linked calls fnmatch as a function, so with the switch to 'import
> fnmatch' from 'from fnmatch import fnmatch', I get this:

Ah, thanks. Missed that.

> > > @@ +378,5 @@
> > > >  
> > > > +        # get directory of this file if not file-like object
> > > > +        here = None
> > > > +        if isinstance(filename, string):
> > > > +            here = os.path.dirname(os.path.abspath(filename))
> > > 
> > > If I were reading this to try to debug something in the manifest parser, I
> > > would really expect a variable named filename to always be a string. 
> > 
> > I kept the name to avoid an API change; I'd suggest changing in a follow-up
> > for atomicity and to avoid breakages.  I can note this in a comment.
> 
> I see. This is the basically the same issue as below about filename vs. file
> object.

Yep.


> > > @@ +411,5 @@
> > > >              test['name'] = section
> > > > +            if isinstance(filename, string):
> > > > +                test['manifest'] = os.path.abspath(filename)
> > > > +            else:
> > > > +                test['manifest'] = None # file pointer
> > > 
> > > Maybe you can explain a bit about the benefits of dynamically checking this
> > > vs. having explicit parameters for fp's vs. names. It seems to be a pattern
> > > in this file.
> > 
> > The general idea is that you can pass in a file-like object or the name of a
> > file to be opened, but not both.  IMHO, there's no reason to allow both
> > since they cannot both work together.  The pattern isn't just in this file,
> > but is common in python code; however, far from consistently.
> > 
> > This could also be done, in this file and elsewhere, as a decorator
> > (implementation available on request).
> 
> I'm curious about what the decorator might look like, but in general if this
> is a pattern in Python, it's probably something I'll have to come to accept.

It appears a few places in the stdlib,but I'd love opinions from Will.  Two (not great) example decorators:  http://k0s.org/hg/config/file/95ba5770d2f0/python/fileobj.py ; http://stackoverflow.com/a/6783492

> > > @@ +765,5 @@
> > > > +        or (e.g.) `os.path.abspath('manifest.ini')` if absolute
> > > > +        paths are desired.
> > > > +
> > > > +        Write can also take a file-like object; in this case,
> > > > +        paths will be governed by `relative_to`.
> > > 
> > > I didn't get into the details of this function, but I wanted to raise the
> > > question of whether such an overloaded parameter is worth the complexity to
> > > test and maintain (I don't think I saw a test for the case of an absolute
> > > path or directory that isn't a descendant).
> > 
> > No, testing is incomplete.  OTOH, there's many untested code paths in
> > manifestparser and elsewhere and my priority is getting this landed.  As far
> > as worth, see above.
> 
> I think I'm missing the big picture here - what does this block?

Sorry, I suppose that was never stated.  The goal is to enable absolute paths and in general make directory -> manifest conversion.  AIUI, it may/may not be wanted for mochitests on manifests follow-up

I'll put up a new patch and/or API proposal soon, though ideas welcome
(Assignee)

Comment 13

4 years ago
Created attachment 795209 [details] [diff] [review]
bug-902610.diff

with corrections
Attachment #789647 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
Created attachment 795279 [details] [diff] [review]
bug-902610.diff

I think/hope this addresses the preliminary comments.  While IMHO this patch is ready to check in, if approved, there are areas for improvement:

- FilteredDirectoryContents could go at top-level, in its own file, or
  even e.g. in mozfile.  It could also be extended such that
  _walk_directories classmethod could be obseleted.

- CLI entry points for `from_directories` and `populate_directories
  could be implemented

- `root` is not used in ManifestParser._read

- manifestparser.py should be split into separate files:
  - expressionparser
  - read_ini could go in its own file, or it could just get replaced
    by ConfigParser
  - ManifestParser, TestManifest (either in the same file, or
    separately)
  - each command line program to its own file
  - probably a utils.py as much as I dislike the pattern

- documentation

- more tests


I was also wondering, thinking specifically of Will, if the filename/fp overloading is something we want for mozbase ... and if not, what pattern do we want?  (If there is a general answer to any of these questions).
Attachment #795209 - Attachment is obsolete: true
Attachment #795279 - Flags: review?(jmaher)
Attachment #795279 - Flags: feedback?(chmanchester)
Flags: needinfo?(wlachance)
(Assignee)

Comment 15

4 years ago
chmanchester: I gave this to jmaher to review as I figured you'd be back to school.  If you do have the time and inclination to review, I would appreciate it.
By filename/fp overloading, you mean the capability of either passing in a file or a filename into a function? That sounds like a nice pattern, I wouldn't be opposed to using it elsewhere inside mozbase.
Flags: needinfo?(wlachance)
Comment on attachment 795279 [details] [diff] [review]
bug-902610.diff

Review of attachment 795279 [details] [diff] [review]:
-----------------------------------------------------------------

this patch appears to do well and provide us great functionality for making manifest parser a great solution.
Attachment #795279 - Flags: review?(jmaher) → review+
(Assignee)

Comment 18

4 years ago
(In reply to Joel Maher (:jmaher) from comment #17)
> Comment on attachment 795279 [details] [diff] [review]
> bug-902610.diff
> 
> Review of attachment 795279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this patch appears to do well and provide us great functionality for making
> manifest parser a great solution.

Thanks, Joel! pushed: https://github.com/mozilla/mozbase/commit/af120878d417aa630800726007522fad7db7b65e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

4 years ago
(though feedback welcome, though not at any inconvenience, from chmanchester.)
Comment on attachment 795279 [details] [diff] [review]
bug-902610.diff

Review of attachment 795279 [details] [diff] [review]:
-----------------------------------------------------------------

I think having these as separate functions will make things more intuitive to users of the API - thanks!
Attachment #795279 - Flags: feedback?(chmanchester) → feedback+
There are test failures locally on my mac similar to comment 4. This is the output:

======================================================================
FAIL: test_relpath (test_convert_directory.TestDirectoryConversion)
test convert `relative_to` functionality
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mozilla/mozbase/manifestdestiny/tests/test_convert_directory.py", line 130, in test_relpath
    files)
AssertionError: Lists differ: ['../../../../../../../../var/... != ['../bar', '../fleem', '../foo...

First differing element 0:
../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmpfxdmfx/bar
../bar

+ ['../bar', '../fleem', '../foo', 'subfile']
- ['../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmpfxdmfx/bar',
-  '../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmpfxdmfx/fleem',
-  '../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmpfxdmfx/foo',
-  '../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/tmpfxdmfx/subdir/subfile']

----------------------------------------------------------------------
(Assignee)

Comment 22

4 years ago
(In reply to Chris Manchester [:chmanchester] from comment #21)
> There are test failures locally on my mac similar to comment 4. This is the
> output:
> 
> ======================================================================
> FAIL: test_relpath (test_convert_directory.TestDirectoryConversion)
> test convert `relative_to` functionality
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File
> "/Users/mozilla/mozbase/manifestdestiny/tests/test_convert_directory.py",
> line 130, in test_relpath
>     files)
> AssertionError: Lists differ: ['../../../../../../../../var/... !=
> ['../bar', '../fleem', '../foo...
> 
> First differing element 0:
> ../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/
> tmpfxdmfx/bar
> ../bar
> 
> + ['../bar', '../fleem', '../foo', 'subfile']
> -
> ['../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/
> tmpfxdmfx/bar',
> - 
> '../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/
> tmpfxdmfx/fleem',
> - 
> '../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/
> tmpfxdmfx/foo',
> - 
> '../../../../../../../../var/folders/r1/snrv12wj2139v3hqqyj62ns00000gp/T/
> tmpfxdmfx/subdir/subfile']
> 
> ----------------------------------------------------------------------

Does anyone else get this sort of error?  Perhaps we should disable this test on mac?

Thoughts, jgriffin, wlach?
Flags: needinfo?(wlachance)
I get a similar error on mac.

The problem seems to be https://github.com/mozilla/mozbase/blob/master/manifestdestiny/tests/test_convert_directory.py#L128.

On Mac, using '.' as relative_to doesn't seem to work correctly with os.path.relpath.  If '.' is changed to subdir, all is well.   Is there a specific reason we wanted to use '.' here?
(Assignee)

Comment 24

4 years ago
(In reply to Jonathan Griffin (:jgriffin) from comment #23)
> I get a similar error on mac.
> 
> The problem seems to be
> https://github.com/mozilla/mozbase/blob/master/manifestdestiny/tests/
> test_convert_directory.py#L128.
> 
> On Mac, using '.' as relative_to doesn't seem to work correctly with
> os.path.relpath.  If '.' is changed to subdir, all is well.   Is there a
> specific reason we wanted to use '.' here?

Not as such; it is surprising that that doesn't work though.  I would be fine either specifying os.getcwd() or having convert fix up `relative_to='./*')`.

Jonathan, would you be willing to test a patch if I posted?
(Assignee)

Updated

4 years ago
Flags: needinfo?(wlachance)
Neither of those work.  The problem seems to be that the stub dir is created at something like:

/var/folders/d_/dg59sn2d63d3tf7dck6q5vz40000gn/T/tmpGg2ACx

but, if I os.chdir() to that directory, os.getcwd() returns:

/private/var/folders/d_/dg59sn2d63d3tf7dck6q5vz40000gn/T/tmpGg2ACx

Passing the subdir variable to relative_dir seems to bypass these problems.
(Assignee)

Comment 26

4 years ago
:ted indicates that this is a symlink issue.  This *should* be work-aroundable, hopefully without causing too many issues.
(Assignee)

Comment 27

4 years ago
Created attachment 810101 [details] [diff] [review]
failing-test.diff

a failing test
(Assignee)

Comment 28

4 years ago
(In reply to Jeff Hammel [:jhammel] from comment #27)
> Created attachment 810101 [details] [diff] [review]
> failing-test.diff
> 
> a failing test

Output:

FAIL: test_relpath_symlink (__main__.TestDirectoryConversion)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "manifestdestiny/tests/test_convert_directory.py", line 164, in test_relpath_symlink
    files)
AssertionError: Lists differ: ['../../link/bar', '../../link... != ['../bar', '../fleem', '../foo...

First differing element 0:
../../link/bar
../bar

+ ['../bar', '../fleem', '../foo', 'subfile']
- ['../../link/bar',
-  '../../link/fleem',
-  '../../link/foo',
-  '../../link/subdir/subfile']

----------------------------------------------------------------------
Ran 7 tests in 0.008s

FAILED (failures=1)
(Assignee)

Comment 29

4 years ago
Created attachment 810193 [details] [diff] [review]
pretend fix
(Assignee)

Comment 30

4 years ago
Created attachment 810197 [details] [diff] [review]
now break it

You'd think that attachment 810193 [details] [diff] [review] would fix things, but...only for some cases.  This breaks them.
(Assignee)

Comment 31

4 years ago
Created attachment 810202 [details] [diff] [review]
lots_of_debug.diff

The test from attachment 810197 [details] [diff] [review] and a lot more code illustrating, um, ultimately, why this problem is hard/impossible/a policy decision.
(Assignee)

Comment 32

4 years ago
Since symlinks can be cyclic and break out of the dep structure, you can imagine why this  would be ... hard.

I propose moving the broken tests to a new file and skipping them for now with a bug filed to that end.
(Assignee)

Comment 33

4 years ago
(In reply to Jeff Hammel [:jhammel] from comment #32)
> Since symlinks can be cyclic and break out of the dep structure, you can
> imagine why this  would be ... hard.
> 
> I propose moving the broken tests to a new file and skipping them for now
> with a bug filed to that end.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=920938
See Also: → bug 920938
(Assignee)

Comment 34

4 years ago
(In reply to Jeff Hammel [:jhammel] from comment #31)
> Created attachment 810202 [details] [diff] [review]
> lots_of_debug.diff
> 
> The test from attachment 810197 [details] [diff] [review] and a lot more
> code illustrating, um, ultimately, why this problem is hard/impossible/a
> policy decision.

Sample output:

(mozbase)│python test_convert_directory.py TestDirectoryConversion.test_relpath_symlink
====================

directory: /tmp/tmp4PHESA/directory
seen: []
cached?: False : {}
dirnames: ('linky', 'subdir')
empty: is /tmp/tmp4PHESA/directory/linky -> /tmp/tmp4PHESA/directory/subdir in ['/tmp/tmp4PHESA/directory']?
  NOPE!

directory: /tmp/tmp4PHESA/directory/subdir
seen: ['/tmp/tmp4PHESA/directory']
cached?: False : {}
dirnames: ('sub',)
empty: is /tmp/tmp4PHESA/directory/subdir/sub -> /tmp/tmp4PHESA/directory/subdir/sub in ['/tmp/tmp4PHESA/directory', '/tmp/tmp4PHESA/directory/subdir']?
  NOPE!

directory: /tmp/tmp4PHESA/directory/subdir/sub
seen: ['/tmp/tmp4PHESA/directory', '/tmp/tmp4PHESA/directory/subdir']
cached?: False : {}
dirnames: ('linky',)
empty: is /tmp/tmp4PHESA/directory/subdir/sub/linky -> /tmp/tmp4PHESA/directory/subdir in ['/tmp/tmp4PHESA/directory', '/tmp/tmp4PHESA/directory/subdir', '/tmp/tmp4PHESA/directory/subdir/sub']?
  YEP!
  Avoiding: /tmp/tmp4PHESA/directory/subdir/sub/linky -> /tmp/tmp4PHESA/directory/subdir
/tmp/tmp4PHESA/directory/subdir/sub: ((), ())
/tmp/tmp4PHESA/directory/subdir: ((), ('subfile',))
empty: is /tmp/tmp4PHESA/directory/subdir -> /tmp/tmp4PHESA/directory/subdir in ['/tmp/tmp4PHESA/directory', '/tmp/tmp4PHESA/directory/subdir', '/tmp/tmp4PHESA/directory/subdir/sub']?
  YEP!
  Avoiding: /tmp/tmp4PHESA/directory/subdir -> /tmp/tmp4PHESA/directory/subdir
/tmp/tmp4PHESA/directory: (('linky',), ('bar', 'fleem', 'foo'))
F
======================================================================
FAIL: test_relpath_symlink (__main__.TestDirectoryConversion)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_convert_directory.py", line 171, in test_relpath_symlink
    files)
AssertionError: Lists differ: ['../bar', '../fleem', '../foo... != ['../bar', '../fleem', '../foo...

Second list contains 1 additional elements.
First extra element 3:
subfile

- ['../bar', '../fleem', '../foo']
+ ['../bar', '../fleem', '../foo', 'subfile']
(Assignee)

Comment 35

4 years ago
Created attachment 811262 [details] [diff] [review]
bug-902610.diff

disables the to_relative test and moves the problem to bug 920938
Attachment #811262 - Flags: review?(ahalberstadt)
Comment on attachment 811262 [details] [diff] [review]
bug-902610.diff

Review of attachment 811262 [details] [diff] [review]:
-----------------------------------------------------------------

Lgtm.
Attachment #811262 - Flags: review?(ahalberstadt) → review+
(Assignee)

Comment 37

4 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #36)
> Comment on attachment 811262 [details] [diff] [review]
> bug-902610.diff
> 
> Review of attachment 811262 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Lgtm.

pushed: https://github.com/mozilla/mozbase/commit/1325277b16e6a0c33213d7144fdb07dd9621f473

pardon the bad r=jhammel comment.  Of course, this should be r=ahal
You need to log in before you can comment on or make changes to this bug.