RESOLVED FIXED 58272
new-run-webkit-tests: --results-directory is relative to builddir, not $PWD
https://bugs.webkit.org/show_bug.cgi?id=58272
Summary new-run-webkit-tests: --results-directory is relative to builddir, not $PWD
Dirk Pranke
Reported 2011-04-11 14:39:20 PDT
For reasons that are probably lost in the mists of time, new-run-webkit-tests appears to treat --results-directory as relative to the build directory, not the current working directory. This seems kinda odd to me, but more importantly, is inconsistent with the way old-run-webkit-tests works. It looks like the chromium bots end up actually using the same location the webkit.org bots use, by using "../../layout-test-results" instead of "layout-test-results". I think it makes sense to change NRWT to match ORWT, but the patches to do so need to be a little hacky to work in coordination with changing the chromium bots.
Attachments
Patch (11.86 KB, patch)
2011-04-11 18:17 PDT, Dirk Pranke
no flags
rebase, use port.get_option() instead of getattr (11.75 KB, patch)
2011-04-12 12:40 PDT, Dirk Pranke
tony: review+
Dirk Pranke
Comment 1 2011-04-11 14:40:23 PDT
Marc-Antoine, Nicholas - Can you think of any reason to keep NRWT's current logic? Currently the chromium bots use a wrapper around NRWT anyway, so they can always pass in whatever logic they need to.
Dirk Pranke
Comment 2 2011-04-11 18:17:42 PDT
Tony Chang
Comment 3 2011-04-12 10:02:04 PDT
Comment on attachment 89137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89137&action=review > Tools/ChangeLog:14 > + This patch includes a hack to treat a value of '..' as special > + to keep the Chromium bots from breaking; the hack can be removed > + once the bots have been updated. Is it possible to just have the bots use an absolute dir? > Tools/Scripts/webkitpy/layout_tests/port/base.py:575 > + option_val = getattr(self._options, 'results_directory') self._options.results_directory seems a bit clearer. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:172 > - return self._build_path(self.get_option('results_directory')) > + return self._build_path('layout-test-results') Is this the default for ORWT? I thought it just put results in /tmp/layout-test-results.
Eric Seidel (no email)
Comment 4 2011-04-12 10:08:21 PDT
I really like moving the results directory away from /tmp/layout-test-results and making it per-checkout. But that doesn't have to be as part of the ORWT->NRWT conversion.
Dirk Pranke
Comment 5 2011-04-12 10:15:51 PDT
(In reply to comment #3) > (From update of attachment 89137 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89137&action=review > > > Tools/ChangeLog:14 > > + This patch includes a hack to treat a value of '..' as special > > + to keep the Chromium bots from breaking; the hack can be removed > > + once the bots have been updated. > > Is it possible to just have the bots use an absolute dir? > The currently running NRWT code does not support absolute dirs, so even if we changed the bots it wouldn't work right. More importantly, I think we try to avoid absolute dirs on the bots since the paths have things like the bot name embedded in them. > > Tools/Scripts/webkitpy/layout_tests/port/base.py:575 > > + option_val = getattr(self._options, 'results_directory') > > self._options.results_directory seems a bit clearer. > It would be, but it won't work if options.results_directory doesn't exist (which it doesn't in some of the unit tests). > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:172 > > - return self._build_path(self.get_option('results_directory')) > > + return self._build_path('layout-test-results') > > Is this the default for ORWT? I thought it just put results in /tmp/layout-test-results. ORWT does put things in /tmp. As Eric says, that seems like a misfeature.
Marc-Antoine Ruel
Comment 6 2011-04-12 10:16:01 PDT
Comment on attachment 89137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89137&action=review >> Tools/ChangeLog:14 >> + once the bots have been updated. > > Is it possible to just have the bots use an absolute dir? Maybe it would be the simplest solution? >> Tools/Scripts/webkitpy/layout_tests/port/base.py:575 >> + option_val = getattr(self._options, 'results_directory') > > self._options.results_directory seems a bit clearer. results_directory = self._options.results_directory or self.default_results_directory() is a one liner. :) > Tools/Scripts/webkitpy/layout_tests/port/test.py:333 > + return '/tmp/layout-test-results' Pardon my ignorance, what blocks from using tempfile.mkdtemp()? I agree with Eric it's probably out of scope of this change.
Eric Seidel (no email)
Comment 7 2011-04-12 10:18:26 PDT
ORWT has written layout tests to /tmp/layout-test-results for as long as I can remember... which is at least 6 years now. :)
Dirk Pranke
Comment 8 2011-04-12 10:19:52 PDT
(In reply to comment #6) > > > Tools/Scripts/webkitpy/layout_tests/port/test.py:333 > > + return '/tmp/layout-test-results' > > Pardon my ignorance, what blocks from using tempfile.mkdtemp()? I agree with Eric it's probably out of scope of this change. When run locally on a developer workstation, it's useful to leave the results lying around in some easily findable place. At least on the mac, tempfile.mkdtemp() puts in things in a weirdly named directory under /var/tmp, so I don't think this would be a usability improvement.
Tony Chang
Comment 9 2011-04-12 11:03:28 PDT
Comment on attachment 89137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89137&action=review >>>> Tools/ChangeLog:14 >>>> + once the bots have been updated. >>> >>> Is it possible to just have the bots use an absolute dir? >> >> The currently running NRWT code does not support absolute dirs, so even if we changed the bots it wouldn't work right. More importantly, I think we try to avoid absolute dirs on the bots since the paths have things like the bot name embedded in them. > > Maybe it would be the simplest solution? It's fine for the code to generate the absolute dir to be in the layout_test_wrapper.py, the script that launches NRWT. We don't have to hard code paths into config files. >>>> Tools/Scripts/webkitpy/layout_tests/port/base.py:575 >>>> + option_val = getattr(self._options, 'results_directory') >>> >>> self._options.results_directory seems a bit clearer. >> >> It would be, but it won't work if options.results_directory doesn't exist (which it doesn't in some of the unit tests). > > results_directory = self._options.results_directory or self.default_results_directory() > > is a one liner. :) If it doesn't exist, won't getattr also throw an AttributeError? Did you mean getattr(self._options, 'results_directory', None)?
Dirk Pranke
Comment 10 2011-04-12 11:32:27 PDT
(In reply to comment #9) > (From update of attachment 89137 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89137&action=review > > >>>> Tools/ChangeLog:14 > >>>> + once the bots have been updated. > >>> > >>> Is it possible to just have the bots use an absolute dir? > >> > >> The currently running NRWT code does not support absolute dirs, so even if we changed the bots it wouldn't work right. More importantly, I think we try to avoid absolute dirs on the bots since the paths have things like the bot name embedded in them. > > > > Maybe it would be the simplest solution? > > It's fine for the code to generate the absolute dir to be in the layout_test_wrapper.py, the script that launches NRWT. We don't have to hard code paths into config files. Sure, we could do that. Regardless, it won't work in the current NRWT code, so I'm not sure that I see much of an improvement? I suppose we could modify NRWT to accept absolute paths, then modify the chromium bot code to change the location, and then either modify NRWT again to change how it interpreted relative paths (or change the webkit.org bot code to use absolute paths). It seems easier to me to make the changes as describe in the patch, though. > > >>>> Tools/Scripts/webkitpy/layout_tests/port/base.py:575 > >>>> + option_val = getattr(self._options, 'results_directory') > >>> > >>> self._options.results_directory seems a bit clearer. > >> > >> It would be, but it won't work if options.results_directory doesn't exist (which it doesn't in some of the unit tests). > > > > results_directory = self._options.results_directory or self.default_results_directory() > > > > is a one liner. :) > > If it doesn't exist, won't getattr also throw an AttributeError? Did you mean getattr(self._options, 'results_directory', None)? Ah, you're right. It needs a third parameter of None. That's lame. I figured it defaulted to None like get() does on dicts. Will fix once we decide if we want to change anything else, so I only have to post one new patch.
Tony Chang
Comment 11 2011-04-12 11:56:01 PDT
(In reply to comment #10) > Sure, we could do that. Regardless, it won't work in the current NRWT code, so I'm not sure that I see much of an improvement? I suppose we could modify NRWT to accept absolute paths, then modify the chromium bot code to change the location, and then either modify NRWT again to change how it interpreted relative paths (or change the webkit.org bot code to use absolute paths). The benefit to doing it in this order is we don't have to check in the relative path hack. This has a couple benefits: - something unexpected coming up (pulled off to work on a critical bug, hit by a bus, etc) and the third step gets delayed for some reason - someone happens to be reading the code before the third step gets checked in and is confused
Dirk Pranke
Comment 12 2011-04-12 12:08:35 PDT
(In reply to comment #11) > (In reply to comment #10) > > Sure, we could do that. Regardless, it won't work in the current NRWT code, so I'm not sure that I see much of an improvement? I suppose we could modify NRWT to accept absolute paths, then modify the chromium bot code to change the location, and then either modify NRWT again to change how it interpreted relative paths (or change the webkit.org bot code to use absolute paths). > > The benefit to doing it in this order is we don't have to check in the relative path hack. This has a couple benefits: > > - something unexpected coming up (pulled off to work on a critical bug, hit by a bus, etc) and the third step gets delayed for some reason > - someone happens to be reading the code before the third step gets checked in and is confused Okay. I think this adds a layer of unnecessary complication (by modifying the buildbot code to use absolute paths), but I'll do it your way.
Dirk Pranke
Comment 13 2011-04-12 12:09:26 PDT
(In reply to comment #12) > Okay. I think this adds a layer of unnecessary complication (by modifying the buildbot code to use absolute paths), but I'll do it your way. [ Sigh. I wish you could edit change comments in bugzilla ]. I meant to add that I don't feel that strongly about this, which is why I'll do it your way since you do seem to have a preference.
Dirk Pranke
Comment 14 2011-04-12 12:40:59 PDT
Created attachment 89246 [details] rebase, use port.get_option() instead of getattr
Tony Chang
Comment 15 2011-04-12 14:35:04 PDT
(In reply to comment #14) > Created an attachment (id=89246) [details] > rebase, use port.get_option() instead of getattr It's not clear to me if you want this reviewed or not. In comment #13, you said you'd rewrite without the relative path hack, but this patch has r? and the relative path hack.
Tony Chang
Comment 16 2011-04-12 14:50:49 PDT
Comment on attachment 89246 [details] rebase, use port.get_option() instead of getattr View in context: https://bugs.webkit.org/attachment.cgi?id=89246&action=review Sorry, I was just confused. I was thinking you would just fix absolute paths, then fix the bots, then land this patch. This is fine too. > Tools/ChangeLog:14 > + This patch includes a hack to treat a value of '..' as special > + to keep the Chromium bots from breaking; the hack can be removed > + once the bots have been updated. Remove this comment since it's no longer accurate.
Dirk Pranke
Comment 17 2011-04-12 15:02:25 PDT
Tony Chang
Comment 18 2011-04-13 10:23:19 PDT
Note You need to log in before you can comment on or make changes to this bug.