Bug 58272 – new-run-webkit-tests: --results-directory is relative to builddir, not $PWD

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dirk Pranke]

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.

[Dirk Pranke]

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]

Created attachment 89137 [details]
Patch

[Tony Chang]

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)]

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]

(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 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)]

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]

(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 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]

(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]

(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]

(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]

(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]

Created attachment 89246 [details]
rebase, use port.get_option() instead of getattr

[Tony Chang]

(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 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]

Committed r83646: <http://trac.webkit.org/changeset/83646>

[Tony Chang]

(In reply to comment #17)
> Committed r83646: <http://trac.webkit.org/changeset/83646>

Fixed in http://trac.webkit.org/changeset/83749