Bug 66228

This is at least partly due to the leaks files being named differently in ORWT and NRWT. For comparison: ORWT: http://build.webkit.org/old-results/SnowLeopard%20Intel%20Leaks/r92628%20(18480)/ NRWT: http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r93040%20(18578)/

I'm happy to name them however you'd like, but I can't (because we use multiple processes) have a single increasing counter for the file numbers. http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/mac.py#L103 Is where the name is generated. That code is per-worker-instance. (Well, it exists in both the worker and the master process), so we don't have any way to coordinate names except through the file system. We could look at what files exist on disk, but then we'd have to look between the workers or we could end up with a race condition.

I've reverted back to ORWT for the Leaks bot for now: http://trac.webkit.org/changeset/93046

We don't necessarily have to change the names. We could teach Leaks Viewer how to interpret the NRWT-style names. One advantage of ORWT's serial test running + monotonically increasing leaks file numbers is that it's possible to figure out which leaks came from which tests. With NRWT it seems like that will be considerably harder. That's probably worth a separate bug.

How do we best move forward with this? Seems the simplest thing is to make the new LeaksViewer understand the new filename pattern. It's impossible (and meaningless) in a parallel environment for us to name the leaks files with increasing numbers in order of creation.

(In reply to comment #5) > Seems the simplest thing is to make the new LeaksViewer understand the new filename pattern. That seems like a fine solution to me.

Created attachment 105512 [details] Patch

Dirk: If you have suggestions on how to fix the integration tests, I'm all ears. But it appears that for most tests the skipped lists are not applied (or userscripts aren't skipped for the Mac port?), thus now that our glob() implementation works, we correctly end up trying to run this "userscripts" test, which then causes badness.

Comment on attachment 105512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105512&action=review > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:146 > + wildcard_index = glob_string.find('*') Maybe we should rewrite this to the equivalent regexp, i.e., ([^\/]*), and just do a regexp match? Does that fix the test?

(In reply to comment #9) > (From update of attachment 105512 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105512&action=review > > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:146 > > + wildcard_index = glob_string.find('*') > > Maybe we should rewrite this to the equivalent regexp, i.e., ([^\/]*), and just do a regexp match? Does that fix the test? I can try. I suspect that that would simply break the test in just the same way.

(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 105512 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=105512&action=review > > > > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:146 > > > + wildcard_index = glob_string.find('*') > > > > Maybe we should rewrite this to the equivalent regexp, i.e., ([^\/]*), and just do a regexp match? Does that fix the test? > > I can try. I suspect that that would simply break the test in just the same way. I think the test should now work, because the problem before with the "broken" implementation was that it wasn't recognizing directory separators as delimiters, and hence it was doing something a real glob wouldn't do. We should be bringing this closer to the real glob's semantics this way. If not, I'm guessing some variation on my suggestion should work, at least.

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > (From update of attachment 105512 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=105512&action=review > > > > > > > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:146 > > > > + wildcard_index = glob_string.find('*') > > > > > > Maybe we should rewrite this to the equivalent regexp, i.e., ([^\/]*), and just do a regexp match? Does that fix the test? > > > > I can try. I suspect that that would simply break the test in just the same way. > > I think the test should now work, because the problem before with the "broken" implementation was that it wasn't recognizing directory separators as delimiters, and hence it was doing something a real glob wouldn't do. We should be bringing this closer to the real glob's semantics this way. If not, I'm guessing some variation on my suggestion should work, at least. I'm not sure I follow what you're saying? I haven't tried the regexp approach. I was looking at http://docs.python.org/library/fnmatch.html, but I worry it will do the wrong thing on windows. Could you explain more?

(In reply to comment #12) > I'm not sure I follow what you're saying? I haven't tried the regexp approach. I was looking at http://docs.python.org/library/fnmatch.html, but I worry it will do the wrong thing on windows. > > Could you explain more? Since we're actually implementing glob, you don't actually want fnmatch semantics, you want glob semantics (http://docs.python.org/library/glob.html#module-glob). This means that 'f*' should match 'foo.html' and 'foobar/' but not 'foobar/baz.html'.

Comment on attachment 105512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105512&action=review > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:145 > + # FIXME: This only handles the simpliest of wildcard matches. Typo: simplest

The unittest failure was caused by me fixing the typo, and had nothing to do with the glob changes. Made that commit in http://trac.webkit.org/changeset/94316

Created attachment 105995 [details] Fix typo from Darin's review

Comment on attachment 105995 [details] Fix typo from Darin's review View in context: https://bugs.webkit.org/attachment.cgi?id=105995&action=review > Tools/ChangeLog:19 > + * Scripts/webkitpy/layout_tests/port/test.py: Nit. Doesn't look like you're actually changing port/test.py? > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:154 > + # We could use fnmatch.fnmatch, but that might not do the right thing on windows. Can you try my re.match suggestion instead ? If that passes the tests, I'd prefer we do that, as the code is likely much cleaner than this (and will handle more cases to boot).

Comment on attachment 105995 [details] Fix typo from Darin's review View in context: https://bugs.webkit.org/attachment.cgi?id=105995&action=review >> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:154 >> + # We could use fnmatch.fnmatch, but that might not do the right thing on windows. > > Can you try my re.match suggestion instead ? If that passes the tests, I'd prefer we do that, as the code is likely much cleaner than this (and will handle more cases to boot). Specifically you're suggesting that I do this: pattern = re.compile(glob_string.replace('*', '\w+')) Is that correct?

Comment on attachment 105995 [details] Fix typo from Darin's review View in context: https://bugs.webkit.org/attachment.cgi?id=105995&action=review >>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:154 >>> + # We could use fnmatch.fnmatch, but that might not do the right thing on windows. >> >> Can you try my re.match suggestion instead ? If that passes the tests, I'd prefer we do that, as the code is likely much cleaner than this (and will handle more cases to boot). > > Specifically you're suggesting that I do this: > > pattern = re.compile(glob_string.replace('*', '\w+')) > > Is that correct? I've tried both that, and re.compile(re.escape(glob_string).replace('\*', '\w+')) and failed to get it to pass the tests. I'd like to stick with this simpler solution unless you have a better suggestion.

Created attachment 106009 [details] Removed extra entry from ChangeLog and removed spurious import glob statement

(In reply to comment #19) > (From update of attachment 105995 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105995&action=review > > >>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:154 > >>> + # We could use fnmatch.fnmatch, but that might not do the right thing on windows. > >> > >> Can you try my re.match suggestion instead ? If that passes the tests, I'd prefer we do that, as the code is likely much cleaner than this (and will handle more cases to boot). > > > > Specifically you're suggesting that I do this: > > > > pattern = re.compile(glob_string.replace('*', '\w+')) > > > > Is that correct? > > I've tried both that, and re.compile(re.escape(glob_string).replace('\*', '\w+')) and failed to get it to pass the tests. > > I'd like to stick with this simpler solution unless you have a better suggestion. No, sorry, I meant pattern = re.compile(glob_string.replace('*', '[^\/]*')). It would need to be '*', not '+', and \w wouldn't handle files w/ spaces in them (and probably a bunch of other legal filename characters). At any rate, thanks for trying. Up to you if you want to try my suggestion again (I'll try it out later while I'm waiting for a compile or something). R+ otherwise.

Comment on attachment 106009 [details] Removed extra entry from ChangeLog and removed spurious import glob statement Thank you dirk. I think we'll still have trouble escaping other possible regexp characters in the filename. We can revisit this later if needed.

(In reply to comment #22) > (From update of attachment 106009 [details]) > Thank you dirk. I think we'll still have trouble escaping other possible regexp characters in the filename. We can revisit this later if needed. Ah, right, yeah '.' would probably screw things up and hence need to be escaped ...

http://stackoverflow.com/questions/445910/create-regex-from-glob-expression probably gets us close, although one commenter actually suggests fnmatch().

fnmatch is what glob uses under the covers. :)

The problem with fnmatch is that it's platform-specific (according to the docs). Which we don't want for a Mock.

Comment on attachment 106009 [details] Removed extra entry from ChangeLog and removed spurious import glob statement Clearing flags on attachment: 106009 Committed r94345: <http://trac.webkit.org/changeset/94345>

All reviewed patches have been landed. Closing bug.

Okay, fixing glob turned out to be slightly more complicated, but it revealed that we actually had (at least) two different bugs. See bug 67462.