Bug 53326

Created attachment 80486 [details] Patch

Comment on attachment 80486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80486&action=review > Tools/Scripts/webkitpy/common/system/fileset.py:40 > + def close(self): Why? > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:144 > + # FIXME: might want tests for this and/or a better comment about how > + # it works. go go punchcards. :) > Tools/Scripts/webkitpy/common/system/ospath.py:35 > +def relpath(path, start_path, os_path_abspath=None, sep=None): We should probably just move this onto filesystem.py now that we have it. > Tools/Scripts/webkitpy/common/system/zipfileset.py:43 > + return (temp_file, zipfile.ZipFile(temp_file)) Confused wehre this is used. > Tools/Scripts/webkitpy/common/system/zipfileset_unittest.py:67 > + return (None, result) Confused where thsi is used.

Comment on attachment 80486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80486&action=review I think this needs a bit more testing, but in general looks good! r- cause I'd like to see at least one tst of the .sep support. > Tools/Scripts/webkitpy/common/system/filesystem.py:206 > + """Returns the relative path from the starting point.""" This seems unhelpful. > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:56 > + idx = path.rfind(self.sep) Seems we should test this new .sep support somewhere. > Tools/Scripts/webkitpy/common/system/zipfileset.py:53 > + def close(self): Now that we have close, seems we might want to have __being__ __end__ support (or whatever it takes to have "with" work).

(In reply to comment #2) > (From update of attachment 80486 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80486&action=review > > > Tools/Scripts/webkitpy/common/system/fileset.py:40 > > + def close(self): > > Why? > The use is clearer in the patch in 53040. The zipfileset downloads the URL into a temp file, but provides no way for the tempfile to be subsequently deleted, which means that the archives get leaked into the filesystem over time. In theory they may get cleaned up when the machine reboots, but that tends to be dependent on the machine configuration (/tmp and friends are not guaranteed to be cleaned up), and if feels wrong to me to rely on that. > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:144 > > + # FIXME: might want tests for this and/or a better comment about how > > + # it works. > > go go punchcards. :) > > > Tools/Scripts/webkitpy/common/system/ospath.py:35 > > +def relpath(path, start_path, os_path_abspath=None, sep=None): > > We should probably just move this onto filesystem.py now that we have it. > Yeah, will do in a separate patch. > > Tools/Scripts/webkitpy/common/system/zipfileset.py:43 > > + return (temp_file, zipfile.ZipFile(temp_file)) > > Confused wehre this is used. > As per above. > > Tools/Scripts/webkitpy/common/system/zipfileset_unittest.py:67 > > + return (None, result) > > Confused where thsi is used.

(In reply to comment #3) > (From update of attachment 80486 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80486&action=review > > I think this needs a bit more testing, but in general looks good! r- cause I'd like to see at least one tst of the .sep support. > Okay, I can add some unit tests. > > Tools/Scripts/webkitpy/common/system/filesystem.py:206 > > + """Returns the relative path from the starting point.""" > > This seems unhelpful. > Will delete. > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:56 > > + idx = path.rfind(self.sep) > > Seems we should test this new .sep support somewhere. > Will do. > > Tools/Scripts/webkitpy/common/system/zipfileset.py:53 > > + def close(self): > > Now that we have close, seems we might want to have __being__ __end__ support (or whatever it takes to have "with" work). It's not clear to me that the lifetime of the zipfile will fit cleanly into a with block, but it's not much work to add it. I can do that in a separate patch and I'll add a FIXME to track it for now.

(In reply to comment #5) > (In reply to comment #3) > > Now that we have close, seems we might want to have __being__ __end__ support (or whatever it takes to have "with" work). > > It's not clear to me that the lifetime of the zipfile will fit cleanly into a with block, but it's not much work to add it. I can do that in a separate patch and I'll add a FIXME to track it for now. Yeah, like many of my review comments, I was just musing to the void about what the perfect platonic zipfileset might look like. My appologies that my musings are often interpretable as requests for action. Certainly not needed in this patch. :)

(In reply to comment #5) > (In reply to comment #3) > > (From update of attachment 80486 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=80486&action=review > > > > I think this needs a bit more testing, but in general looks good! r- cause I'd like to see at least one tst of the .sep support. > > > > Okay, I can add some unit tests. > Before I go off and write stuff .. is there something specific you're concerned about testing, or is this just a general concern for coverage? The code paths are well-covered under the layout_tests unit tests, but I have no problem adding unit tests here as well, I just wanted to make sure I was addressing what your concern really was.

Comment on attachment 80486 [details] Patch I just see you adding a .sep feature, and I don't see it used anywhere, that's all. No specific need. Just would like to see a test which uses a different .sep

(In reply to comment #8) > (From update of attachment 80486 [details]) > I just see you adding a .sep feature, and I don't see it used anywhere, that's all. No specific need. Just would like to see a test which uses a different .sep Ah, I didn't add it, it was already there. All I did was replace places where we still had hard-coded '/' strings with self.sep. The property is meant to be read-only (I suppose I could code it as such) and public just so people can refer to it if they need to rather than using os.sep and potentially introducing bugs in unit tests on windows. At some point it might be interesting to actually change the '/' to something else and see what happens.

Created attachment 80499 [details] add tests for fs.sep, make it readonly

(In reply to comment #9) > At some point it might be interesting to actually change the '/' to something else and see what happens. Every once in a while I think this and then I realize that you can't do this without breaking the constructors, because the files dict relies on the keys having '/' as the separator. I.e., we could change self.sep, but every unit test that relied on a pre-existing filename with a path separator in it would break.

Comment on attachment 80499 [details] add tests for fs.sep, make it readonly View in context: https://bugs.webkit.org/attachment.cgi?id=80499&action=review Still looks OK. See nits below. > Tools/Scripts/webkitpy/common/system/filesystem.py:55 > + def _get_sep(self): > + return self._sep > + > + sep = property(_get_sep, doc="pathname separator") I'm not sure making it read-only is really worth the trouble, but it's not horrible either. > Tools/Scripts/webkitpy/common/system/filesystem.py:211 > + """Returns the relative path from the starting point.""" Doesn't add anything, really. > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:55 > + def _get_sep(self): > + return self._sep > + > + sep = property(_get_sep, doc="pathname separator") Sigh, more boilerplate. There really isn't a way to do this with less code? > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:62 > def _split(self, path): > - idx = path.rfind('/') > + idx = path.rfind(self.sep) > return (path[0:idx], path[idx + 1:]) Isn't this just path.rsplit(self.sep, max=1)? > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:66 > + if path.endswith(self.sep): > + return path[:-1] I think you just want return path.rstrip('/'), no? > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:149 > + # FIXME: might want tests for this and/or a better comment about how > + # it works. I think the patch ordering might have gotten mixed locally. > Tools/Scripts/webkitpy/common/system/filesystem_unittest.py:175 > + self.assertEquals(fs.sep, os.sep) > + self.assertEquals(fs.join("foo", "bar"), > + os.path.join("foo", "bar")) I guess I was more intersted in seeing a FileSystem instantiated with a custom sep and seen that it was used. FileSystem(sep="#"), fs.join('foo', 'bar') == 'foo#bar', etc. > Tools/Scripts/webkitpy/common/system/filesystem_unittest.py:184 > + try: > + fs.sep = ' ' > + self.assertTrue(False, msg="fs.sep isn't readonly") > + except AttributeError, e: > + pass there is an assertRaises which seems more appropriate here (assuming you keep the read-only stuff, which I'm not sure is necessary, but is up to you.

(In reply to comment #12) > (From update of attachment 80499 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80499&action=review > > Still looks OK. See nits below. > > > Tools/Scripts/webkitpy/common/system/filesystem.py:55 > > + def _get_sep(self): > > + return self._sep > > + > > + sep = property(_get_sep, doc="pathname separator") > > I'm not sure making it read-only is really worth the trouble, but it's not horrible either. > I think it probably makes things a little more obvious. > > Tools/Scripts/webkitpy/common/system/filesystem.py:211 > > + """Returns the relative path from the starting point.""" > > Doesn't add anything, really. > Whoops, meant to delete that. > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:55 > > + def _get_sep(self): > > + return self._sep > > + > > + sep = property(_get_sep, doc="pathname separator") > > Sigh, more boilerplate. There really isn't a way to do this with less code? > Unfortunately, not as far as I know :( Still think it's probably worth it. > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:62 > > def _split(self, path): > > - idx = path.rfind('/') > > + idx = path.rfind(self.sep) > > return (path[0:idx], path[idx + 1:]) > > Isn't this just path.rsplit(self.sep, max=1)? > Yeah, I think that works. > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:66 > > + if path.endswith(self.sep): > > + return path[:-1] > > I think you just want return path.rstrip('/'), no? > rstrip will strip all trailing '/' rather than just one. That may or may not be the right thing to do in the mock system, so I'll probably just leave things the way they are. > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:149 > > + # FIXME: might want tests for this and/or a better comment about how > > + # it works. > > I think the patch ordering might have gotten mixed locally. > Maybe. > > Tools/Scripts/webkitpy/common/system/filesystem_unittest.py:175 > > + self.assertEquals(fs.sep, os.sep) > > + self.assertEquals(fs.join("foo", "bar"), > > + os.path.join("foo", "bar")) > > I guess I was more intersted in seeing a FileSystem instantiated with a custom sep and seen that it was used. FileSystem(sep="#"), fs.join('foo', 'bar') == 'foo#bar', etc. > Yeah, understood. As per comment #10 and #11, that isn't the intended usage and probably would be bad. I suppose I could change the constructor to substitute self.sep for '/' in the keys. > > Tools/Scripts/webkitpy/common/system/filesystem_unittest.py:184 > > + try: > > + fs.sep = ' ' > > + self.assertTrue(False, msg="fs.sep isn't readonly") > > + except AttributeError, e: > > + pass > To use assertRaises I need to stuff the assignment into a local function, but I can do that.

Created attachment 80509 [details] update w/ more review feedback from eseidel

Comment on attachment 80509 [details] update w/ more review feedback from eseidel OK.

Comment on attachment 80509 [details] update w/ more review feedback from eseidel Clearing flags on attachment: 80509 Committed r77093: <http://trac.webkit.org/changeset/77093>

All reviewed patches have been landed. Closing bug.

Committed r77096: <http://trac.webkit.org/changeset/77096>