Bug 50901

Created attachment 76349 [details] Patch Add DirAsZip and RemoteZip for providing a unified interface for directories and remote zip files.

Comment on attachment 76349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76349&action=review So I feel like you're trying to make a directory act like a zip, when you really want a zip to act like a directory. I'm confused as to the overarching design here. You want some abstraction to treat both local results as well as remote results the same, right? As well as zipped results vs. non-zipped, right? Why not just always download and unzip and then they're all teh same? I'm confused. > WebKitTools/ChangeLog:7 > + The ChangeLog should talk more about the "why" of these changes. Why are these two classes needed? > WebKitTools/ChangeLog:9 > + * Scripts/webkitpy/common/diraszip.py: Added. > + * Scripts/webkitpy/common/net/remotezip.py: Added. Every change should have a test. In python it's super easy. Just name a file with _unittest.py in the webkitpy directory and test-webkitpy will find it and run all the tests in it. > WebKitTools/Scripts/webkitpy/common/diraszip.py:36 > + """Provides a zipfile-like interface to a local directory""" So I'm confused why this is needed? > WebKitTools/Scripts/webkitpy/common/diraszip.py:39 > + self._filesystem = filesystem if filesystem != None else FileSystem() Using "None" to mean "default value" is often written as: if not filesystem: filesystem = FileSystem() and then later: self._filesystem = filesystem But your solution works too. I wouldn't have bothered checking explicit != None. I probably would have "cheated" with or and just written: self._filesystem = filesystem or FileSystem() > WebKitTools/Scripts/webkitpy/common/diraszip.py:41 > + if not self._path.endswith(os.path.sep): > + self._path += os.path.sep Unclear why this is necessary? > WebKitTools/Scripts/webkitpy/common/diraszip.py:58 > + return ospath.relpath(os.path.join(dir, filename), dir) is not None bool(EXPRESSION) or "not not EXPRESSION" are common ways of converting from a value to a bool (which are slightly less verbose than "is not None"

(In reply to comment #2) > (From update of attachment 76349 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76349&action=review > > So I feel like you're trying to make a directory act like a zip, when you really want a zip to act like a directory. I'm confused as to the overarching design here. > > You want some abstraction to treat both local results as well as remote results the same, right? As well as zipped results vs. non-zipped, right? Yes, you're right - and calling this abstraction 'zip' is definitely confusing. The point of these classes it to provide a simple interface for talking about collections of files (they just happen to crib the Python zipfile.ZipFile interface). What do you think about renaming them to ZipFileBundle and DirectoryFileBundle? (Or some noun better than FileBundle :-) ) > > Why not just always download and unzip and then they're all teh same? I'm confused. It's true that ZipFileBundle could be implemented by downloading and extracting a zip and then delegating to a DirectoryFileBundle, but I think that the current implementation is good because it is more efficient (it will only read files that it needs to, which can be significant on chromium builders which store passing and failing test results) and it is quite a short class anyway (<30 lines). > > > WebKitTools/ChangeLog:7 > > + > > The ChangeLog should talk more about the "why" of these changes. Why are these two classes needed? > > > WebKitTools/ChangeLog:9 > > + * Scripts/webkitpy/common/diraszip.py: Added. > > + * Scripts/webkitpy/common/net/remotezip.py: Added. > > Every change should have a test. In python it's super easy. Just name a file with _unittest.py in the webkitpy directory and test-webkitpy will find it and run all the tests in it. > > > WebKitTools/Scripts/webkitpy/common/diraszip.py:36 > > + """Provides a zipfile-like interface to a local directory""" > > So I'm confused why this is needed? I'll try and add something more concise to the ChangeLog, but briefly the reason why I want these interfaces is because they are much easier to deal with than raw strings representing paths and filenames. It's not that there's any use specifically in having a directory behave like a zip file (as discussed above), this is more just a way to define a common interface for dealing with collections of files in a simple and easy to test way. For instance, FakeZip is a trivial class to implement and leads to test code that looks like: local_results = FakeZip() local_results.insert('platform/mac/some-test-expected.txt', 'old baseline') remote_results = FakeZip() remote_results.insert('platform/mac/some-test-actual.txt', 'new baseline') > > > WebKitTools/Scripts/webkitpy/common/diraszip.py:39 > > + self._filesystem = filesystem if filesystem != None else FileSystem() > > Using "None" to mean "default value" is often written as: > if not filesystem: > filesystem = FileSystem() > > and then later: > self._filesystem = filesystem > > But your solution works too. I wouldn't have bothered checking explicit != None. I probably would have "cheated" with or and just written: self._filesystem = filesystem or FileSystem() > > > WebKitTools/Scripts/webkitpy/common/diraszip.py:41 > > + if not self._path.endswith(os.path.sep): > > + self._path += os.path.sep > > Unclear why this is necessary? If the user provides us with a path like "a/b", the results we get from an os.walk() will all have prefixes of "a/b/". We normalise our path at the beginning so we know how much to drop off the resulting os.walk() filenames. I'll add a comment to the code to make this more clear. > > > WebKitTools/Scripts/webkitpy/common/diraszip.py:58 > > + return ospath.relpath(os.path.join(dir, filename), dir) is not None > > bool(EXPRESSION) or "not not EXPRESSION" are common ways of converting from a value to a bool (which are slightly less verbose than "is not None"

(In reply to comment #2) > (From update of attachment 76349 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76349&action=review > > So I feel like you're trying to make a directory act like a zip, when you really want a zip to act like a directory. I'm confused as to the overarching design here. > > You want some abstraction to treat both local results as well as remote results the same, right? As well as zipped results vs. non-zipped, right? > > Why not just always download and unzip and then they're all teh same? I'm confused. > > > WebKitTools/ChangeLog:7 > > + > > The ChangeLog should talk more about the "why" of these changes. Why are these two classes needed? Done. > > > WebKitTools/ChangeLog:9 > > + * Scripts/webkitpy/common/diraszip.py: Added. > > + * Scripts/webkitpy/common/net/remotezip.py: Added. > > Every change should have a test. In python it's super easy. Just name a file with _unittest.py in the webkitpy directory and test-webkitpy will find it and run all the tests in it. Done. > > > WebKitTools/Scripts/webkitpy/common/diraszip.py:36 > > + """Provides a zipfile-like interface to a local directory""" > > So I'm confused why this is needed? > > > WebKitTools/Scripts/webkitpy/common/diraszip.py:39 > > + self._filesystem = filesystem if filesystem != None else FileSystem() > > Using "None" to mean "default value" is often written as: > if not filesystem: > filesystem = FileSystem() > > and then later: > self._filesystem = filesystem > > But your solution works too. I wouldn't have bothered checking explicit != None. I probably would have "cheated" with or and just written: self._filesystem = filesystem or FileSystem() Actually, I like your way the best, so I've changed it to that :-) > > > WebKitTools/Scripts/webkitpy/common/diraszip.py:41 > > + if not self._path.endswith(os.path.sep): > > + self._path += os.path.sep > > Unclear why this is necessary? > > > WebKitTools/Scripts/webkitpy/common/diraszip.py:58 > > + return ospath.relpath(os.path.join(dir, filename), dir) is not None > > bool(EXPRESSION) or "not not EXPRESSION" are common ways of converting from a value to a bool (which are slightly less verbose than "is not None" Done.

Created attachment 77865 [details] Patch

Comment on attachment 77865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77865&action=review > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:116 > + def files_under(self, path): > + results = [] > + for file in self.files: > + if file.startswith(path): > + results.append(file) > + return results This is just [file for file in self.files if file.startswith(path)]

Comment on attachment 77865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77865&action=review > Tools/Scripts/webkitpy/common/directoryfileset_unittest.py:51 > + two blank lines i believe. > Tools/Scripts/webkitpy/common/directoryfileset_unittest.py:53 > +if __name__ == '__main__': > + unittest.main() I generally omit these since the way we do imports makes it impossible to run these by hand anywa. > Tools/Scripts/webkitpy/common/net/zipfileset.py:1 > +#!/usr/bin/env python Please omit. > Tools/Scripts/webkitpy/common/net/zipfileset.py:80 > + if filename is None: > + self._fileset.extract(self._filename, path) I would have early returned here.

Comment on attachment 77865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77865&action=review You could just make this a new module common.fileset. YOu could also just slap this all into one file common/fileset.py. I'm not sure which is cleaner. Making it a module is better long-term if we plan to expand on this. > Tools/Scripts/webkitpy/common/directoryfileset_unittest.py:33 > +class DirectoryFileSetTest(unittest.TestCase): Seems we should test the whole api here, no? > Tools/Scripts/webkitpy/common/net/zipfileset.py:63 > +class FileSetFileHandle(object): Since this is used by more than one file seems it should be in its own file.

Comment on attachment 77865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77865&action=review > Tools/Scripts/webkitpy/common/directoryfileset.py:36 > + """Provides a zipfile-like interface to a local directory""" I'm still confused why we're making directories look like zips instead of zips look like directories. That seems backwards. :)

We would use something like this in common.net.layouttestresults if it existed, I think. It's still unclear to me why we treat directories like zips. But I do think it makes sense to have a concept of some sort of file set, so I think we want this change. I think my major source of confusion is just in the positioning of this class. Its basically an abstraction around a filesystem tree, which could be backed by a remote file system or a local file system or a zip or a directory. I'm not sure what we call that. FileSet is fine, I guess.

Comment on attachment 77865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77865&action=review > Tools/Scripts/webkitpy/common/directoryfileset.py:30 > +from webkitpy.common.net.zipfileset import FileSetFileHandle Having DirectoryFileSet depend on zipfileset seems wrong. Can there be a (Base)FileSet class/file for code that is used in both? > Tools/Scripts/webkitpy/common/directoryfileset.py:39 > + self._filesystem = filesystem if filesystem != None else FileSystem() This can just be self._filesystem = filesystem or Filesystem() > Tools/Scripts/webkitpy/common/directoryfileset.py:72 > + # directory heirarchy exists at the output path. Typo ("heirarchy") > Tools/Scripts/webkitpy/common/directoryfileset_unittest.py:35 > + self._files = {} Seems like _files doesn't need to be a property of the class (and you don't need an addFile method), given that you're using a MockFileSystem. Instead you could just say: def setUp(self): files = { '/test/some-file': 'contents', ... } self._fileset = DirectoryFileSet('/test', MockFileSystem(files)) >> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:116 > > This is just [file for file in self.files if file.startswith(path)] Though this should also handle directory names that are substrings of one another. I.e. if files is: /foo/bar /foo2/bar Then files_under('/foo') should not include ('foo2/bar). (see how listdir is implemented to avoid this)

Oops, mid-air review collision. (In reply to comment #10) > We would use something like this in common.net.layouttestresults if it existed, I think. It's still unclear to me why we treat directories like zips. But I do think it makes sense to have a concept of some sort of file set, so I think we want this change. Agreed, I think the goal shouldn't be "zip file-like implementation for local directories" but instead "file set abstraction that could be backed by local directories or zip files."

(In reply to comment #6) > (From update of attachment 77865 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77865&action=review > > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:116 > > + def files_under(self, path): > > + results = [] > > + for file in self.files: > > + if file.startswith(path): > > + results.append(file) > > + return results > > This is just [file for file in self.files if file.startswith(path)] Done.

Comment on attachment 77865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77865&action=review >> Tools/Scripts/webkitpy/common/directoryfileset.py:30 > > Having DirectoryFileSet depend on zipfileset seems wrong. Can there be a (Base)FileSet class/file for code that is used in both? Yep - I'll add a fileset.py file to hold the FileSetHandle class. I guess such a file would be where a general FileSet (ie: one not backed by filesystem or zip file) class could be defined if one is ever needed. >> Tools/Scripts/webkitpy/common/directoryfileset.py:36 > > I'm still confused why we're making directories look like zips instead of zips look like directories. That seems backwards. :) I think this comment is out of date. It should read "The set of files under a local directory." That this interface is like a zip file is incidental :-) The reason this interface looks like the one for zip files is because like a zip file it doesn't require the caller to remember the path to the directory that they are interested in, for example: d = DirectoryFileSet('/some/path/prefix') d.namelist() d.read('some-file') d.open('some-other-file') would be fs = FileSystem() fs.all_files_under('/some/path/prefix') fs.read('/some/path/prefix/some-file') fs.open('/some/path/prefix/some-other-file') >> Tools/Scripts/webkitpy/common/directoryfileset.py:39 > > This can just be self._filesystem = filesystem or Filesystem() Done. >> Tools/Scripts/webkitpy/common/directoryfileset.py:72 > > Typo ("heirarchy") Done. >> Tools/Scripts/webkitpy/common/directoryfileset_unittest.py:33 > > Seems we should test the whole api here, no? Yes, we should :-) In doing so I realised that FileSystemMock lacks a remove() function (though it exists in FileSystem). Thanks. I also added a test for ZipFileSet (it uses temp directories / files if that's okay). >> Tools/Scripts/webkitpy/common/directoryfileset_unittest.py:35 > > Seems like _files doesn't need to be a property of the class (and you don't need an addFile method), given that you're using a MockFileSystem. Instead you could just say: > def setUp(self): > files = { > '/test/some-file': 'contents', > ... > } > self._fileset = DirectoryFileSet('/test', MockFileSystem(files)) Ah yes, true :-) Done. >> Tools/Scripts/webkitpy/common/directoryfileset_unittest.py:51 > > two blank lines i believe. Done. >> Tools/Scripts/webkitpy/common/directoryfileset_unittest.py:53 > > I generally omit these since the way we do imports makes it impossible to run these by hand anywa. I've got a command that adds the webkitpy directory to python's path for running tests like these, which is handy because running all the unit tests with test-webkitpy can take a while. >> Tools/Scripts/webkitpy/common/net/zipfileset.py:1 > > Please omit. Done. >> Tools/Scripts/webkitpy/common/net/zipfileset.py:63 > > Since this is used by more than one file seems it should be in its own file. I put it in fileset.py to mean 'common code for file sets'. Would it be better to put it in filesetfilehandle.py? >> Tools/Scripts/webkitpy/common/net/zipfileset.py:80 > > I would have early returned here. Done.

(In reply to comment #11) > >> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:116 > > > > > This is just [file for file in self.files if file.startswith(path)] > > Though this should also handle directory names that are substrings of one another. I.e. if files is: > /foo/bar > /foo2/bar > > Then files_under('/foo') should not include ('foo2/bar). > > (see how listdir is implemented to avoid this) Done.

Created attachment 78188 [details] Patch

Comment on attachment 78188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78188&action=review > Tools/ChangeLog:12 > + * Scripts/webkitpy/common/directoryfileset.py: Added. Should be updated to include fileset.py > Tools/Scripts/webkitpy/common/directoryfileset.py:30 > +from webkitpy.common.system.filesystem import FileSystem Given that filesystem lives in webkitpy.common.system, perhaps the fileset classes should be in there too. > Tools/Scripts/webkitpy/common/directoryfileset.py:44 > + return os.path.join(self._path, filename) Should use filesystem.join instead of os.path.join. > Tools/Scripts/webkitpy/common/directoryfileset.py:69 > + dest = os.path.join(path, filename) Same here. > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:117 > + del(self.files[path]) del is actually a statement, this should be del self.files[path]

Created attachment 78201 [details] Patch

Comment on attachment 78188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78188&action=review >> Tools/ChangeLog:12 > > Should be updated to include fileset.py Done. >> Tools/Scripts/webkitpy/common/directoryfileset.py:30 > > Given that filesystem lives in webkitpy.common.system, perhaps the fileset classes should be in there too. Done. >> Tools/Scripts/webkitpy/common/directoryfileset.py:44 > > Should use filesystem.join instead of os.path.join. Done. >> Tools/Scripts/webkitpy/common/directoryfileset.py:69 > > Same here. Done. >> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:117 > > del is actually a statement, this should be del self.files[path] Done.

@ojan: Another example of a broken context from a comment reply in the review tool ^^^^^^^^^^

(In reply to comment #20) > @ojan: Another example of a broken context from a comment reply in the review tool ^^^^^^^^^^ Whoops. I mis-split my original patch up. Fixed http://trac.webkit.org/changeset/75222.

Comment on attachment 78201 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78201&action=review > Tools/Scripts/webkitpy/common/system/filesystem.py:162 > + def extract(self, zip, filename, path): As discussed in #webkit, extract doesn't really belong in FileSystem. For the sake of testability, passing in the ZipFile (or a lazy-loading wrapper) to the ZipFileSet constructor is cleaner. > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:60 > + return os.path.join(*comps) The mock filesystem hardcodes / as the separator, so it should keep using '/'.join(comps) as its join() implementation.

(In reply to comment #22) > (From update of attachment 78201 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78201&action=review > > > Tools/Scripts/webkitpy/common/system/filesystem.py:162 > > + def extract(self, zip, filename, path): > > As discussed in #webkit, extract doesn't really belong in FileSystem. For the sake of testability, passing in the ZipFile (or a lazy-loading wrapper) to the ZipFileSet constructor is cleaner. Yep. I used your suggestion of passing in the constructor. > > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:60 > > + return os.path.join(*comps) > > The mock filesystem hardcodes / as the separator, so it should keep using '/'.join(comps) as its join() implementation. I've changed it to use os.path.join() but then to substitute back to using '/' because os.path.join() collapses slashes, which '/'.join() doesn't.

Created attachment 78206 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 78206 [details]: http/tests/xmlhttprequest/logout.html bug 52047 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 78206 [details] Patch Clearing flags on attachment: 78206 Committed r75233: <http://trac.webkit.org/changeset/75233>

All reviewed patches have been landed. Closing bug.

Comment on attachment 78206 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78206&action=review LayoutTestResults/FlakyTestReporter/CommitQueueTask may want something like this for saving off a copy of the LayoutTestsDirectory (either in zip form for upload, or just for later use after a second layout-test run. > Tools/Scripts/webkitpy/common/system/directoryfileset.py:64 > + return self._filesystem.read_text_file(self._full_path(filename)) This won't work for binary files, obviously.

It broke Qt and GTK bots, reopen to fix. I think the problem caused by different python versions. test-webkitpy: INFO Suppressing most webkitpy logging while running unit tests. test-webkitpy: WARNING Skipping tests in ./Tools/QueueStatusServer due to failure (No module named dev_appserver). This module is optional. The failure is likely due to a missing Google AppEngine install. (http://code.google.com/appengine/downloads.html) webkitpy.test.main: INFO Excluding: webkitpy.common.checkout.scm_unittest (use --all to include) /home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/common/system/fileset.py:49: Warning: 'with' will become a reserved keyword in Python 2.6 Traceback (most recent call last): File "./Tools/Scripts/test-webkitpy", line 266, in <module> Tester().run_tests(sys.argv, external_package_paths) File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/test/main.py", line 140, in run_tests unittest.main(argv=sys_argv, module=None) File "/usr/lib/python2.5/unittest.py", line 767, in __init__ self.parseArgs(argv) File "/usr/lib/python2.5/unittest.py", line 794, in parseArgs self.createTests() File "/usr/lib/python2.5/unittest.py", line 800, in createTests self.module) File "/usr/lib/python2.5/unittest.py", line 565, in loadTestsFromNames suites = [self.loadTestsFromName(name, module) for name in names] File "/usr/lib/python2.5/unittest.py", line 533, in loadTestsFromName module = __import__('.'.join(parts_copy)) File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/common/system/directoryfileset_unittest.py", line 28, in <module> from webkitpy.common.system.directoryfileset import DirectoryFileSet File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/common/system/directoryfileset.py", line 29, in <module> from webkitpy.common.system.fileset import FileSetFileHandle File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/common/system/fileset.py", line 49 with self._filesystem.mkdtemp() as temp_dir: ^ SyntaxError: invalid syntax program finished with exit code 1

Oh, the file just needs a: from __future__ import with_statement at the top in order to work on python 2.5. :)

http://trac.webkit.org/changeset/75233 might have broken Leopard Intel Release (Tests) and Leopard Intel Debug (Tests)

(In reply to comment #30) > Oh, the file just needs a: > > from __future__ import with_statement > > at the top in order to work on python 2.5. :) Fixed in http://trac.webkit.org/changeset/75239 . Thx.

(In reply to comment #30) > Oh, the file just needs a: > > from __future__ import with_statement > > at the top in order to work on python 2.5. :) This happens periodically. Any chance the cq machines can be downgraded to 2.5 to catch this?

I'll see about making the commit-queue use python 2.5 to run the python tests if available.

filed bug 52066.