Bug 98071 – webkitpy should accept a different httpd.conf specified by the user

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

[Raphael Kubo da Costa (:rakuco)]

webkitpy should accept a different httpd.conf specified by the user

[Raphael Kubo da Costa (:rakuco)]

Created attachment 166545 [details]
Patch

[Dirk Pranke]

Comment on attachment 166545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166545&action=review

> Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:603
> +        os.environ = saved_environ.copy()

You should wrap these in a try-finally block to ensure that os.environ gets restored no matter what happens.

Longer term we might want to consider enhancing the environment wrapper in webkitpy.common.system to make this more mockable.

> Tools/Scripts/webkitpy/layout_tests/servers/http_server.py:103
> +        base_conf_file = self._path_to_config_file()

I'm not a fan of the duplication between this and the code in base.py. I'm also not sure that this is even worth doing since AFAIK only chromium uses this routine and we're not likely to set this. I appreciate your diligence, but how about we just add a FIXME to consider supporting the variable and otherwise leave the files alone?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #2)
> (From update of attachment 166545 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166545&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:603
> > +        os.environ = saved_environ.copy()
> 
> You should wrap these in a try-finally block to ensure that os.environ gets restored no matter what happens.

Alright.

> > Tools/Scripts/webkitpy/layout_tests/servers/http_server.py:103
> > +        base_conf_file = self._path_to_config_file()
> 
> I'm not a fan of the duplication between this and the code in base.py. I'm also not sure that this is even worth doing since AFAIK only chromium uses this routine and we're not likely to set this. I appreciate your diligence, but how about we just add a FIXME to consider supporting the variable and otherwise leave the files alone?

I'm OK with that, but it might make my life more difficult if I want to run http tests in chromium ;)

[Raphael Kubo da Costa (:rakuco)]

Created attachment 166556 [details]
Patch for landing

[Raphael Kubo da Costa (:rakuco)]

Committed r130083: <http://trac.webkit.org/changeset/130083>

[Ojan Vafai]

Comment on attachment 166556 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=166556&action=review

> Tools/ChangeLog:21
> +        In the long term, we should try to generate our configuration file
> +        and stop requiring all the different httpd.conf files we have as
> +        well as this hack.

Yes please!

[Dirk Pranke]

(In reply to comment #3)
> I'm OK with that, but it might make my life more difficult if I want to run http tests in chromium ;)

Only on Windows, I'm assuming that's not a big concern for you. Chromium uses the Apache config on the other platforms