Bug 64086

Summary: [Qt] NRWT should pick up the right httpd config file
Product: WebKit Reporter: Andras Becsi <abecsi>
Component: Tools / TestsAssignee: Kristóf Kosztyó <kkristof>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dihan.wickremasuriya, dpranke, eric, galpeter, kkristof, laszlo.gombos, ossy, rgabor, webkit.review.bot
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 64441    
Bug Blocks: 34984    
Attachments:
Description Flags
proposed fix
eric: review-
Patch
none
Patch none

Description Andras Becsi 2011-07-07 06:57:33 PDT
NRWT currently uses apache2-debian-httpd.conf for running the httpd server, thus only runs on debian based distros, this is a huge regression compared to ORWT.

NRWT should at least check for the /etc/debian_version file, and if not present it should use the generic apache2-httpd.conf file.
Comment 1 Andras Becsi 2011-07-07 06:59:42 PDT
Created attachment 99977 [details]
proposed fix
Comment 2 Eric Seidel (no email) 2011-07-07 10:10:37 PDT
Comment on attachment 99977 [details]
proposed fix

OK.  There is a similar "is_redhat_based()" function in the Gtk port.

Please add a test for this in qt_unittest.py.  Should be very easy to do with MockFileSystem({'/etc/debian_version', ''})

Something like this:

def test_is_debian_based(self)
    port = self.make_port() # This is available in PortTestCase which QtPortTest isn't yet, I don't think, but could be made to
    self.assertEqual(port.is_debian_based(), False)
    port._filesystem = MockFileSystem({'/etc/debian_version', ''})
    self.assertEqual(port.is_debian_based(), True)

One could also test that overriding _is_debian_based() with lamba: False or lambda: True affected port._path_to_appache_config_file().basename() if you wanted.  In any case, we just need some sort of testing.
Comment 3 Eric Seidel (no email) 2011-07-07 10:11:39 PDT
Comment on attachment 99977 [details]
proposed fix

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

> Tools/Scripts/webkitpy/layout_tests/port/qt.py:32
> +import os

Don't need this once you use FileSystem.

> Tools/Scripts/webkitpy/layout_tests/port/qt.py:69
> +        if os.path.isfile('/etc/debian_version'):

Oh, this is wrong too.  This should use self._filesystem.exists() or isfile if that method exists on FileSystem.  We don't ever talk to os directly if we can help it.  (Makes mockng hard.)
Comment 4 Csaba Osztrogonác 2011-07-11 04:23:35 PDT
Same or similar bug on Qt Mac bot:

2011-07-11 04:15:11,000 86746 apache_http_server.py:182 DEBUG Starting httpd server, cmd="/usr/sbin/httpd -f "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/layout-test-results/httpd.conf" -C 'DocumentRoot "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/LayoutTests/http/tests"' -c 'Alias /js-test-resources "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/LayoutTests/fast/js/resources"' -c 'Alias /media-resources "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/LayoutTests/media"' -C 'Listen 127.0.0.1:8000' -C 'Listen 127.0.0.1:8081' -c 'TypesConfig "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/LayoutTests/http/conf/mime.types"' -c 'CustomLog "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/layout-test-results/access_log.txt" common' -c 'ErrorLog "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/layout-test-results/error_log.txt"' -C 'User "buildbot"' -c 'PidFile /tmp/WebKit/httpd.pid' -k start -c 'SSLCertificateFile /buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/LayoutTests/http/conf/webkit-httpd.pem'"
Traceback (most recent call last):
  File "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 438, in <module>
Ignoring unsupported option: --use-remote-links-to-tests
    sys.exit(main())
  File "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 433, in main
    return run(port_obj, options, args)
  File "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 112, in run
    num_unexpected_results = manager.run(result_summary)
  File "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 874, in run
    interrupted, keyboard_interrupted, thread_timings, test_timings, individual_test_timings = self._run_tests(self._test_files_list, result_summary)
  File "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 723, in _run_tests
    self.start_servers_with_lock()
  File "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 936, in start_servers_with_lock
    self._port.start_http_server()
  File "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/Tools/Scripts/webkitpy/layout_tests/port/base.py", line 669, in start_http_server
    server.start()
  File "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py", line 86, in start
    self._pid = self._spawn_process()
  File "/buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py", line 185, in _spawn_process
    raise http_server_base.ServerError('Failed to start %s: %s' % (self._name, err))
webkitpy.layout_tests.servers.http_server_base.ServerError: Failed to start httpd: httpd: Syntax error on line 197 of /buildbot/snowleopard-qt-release/snowleopard-qt-intel-release/build/layout-test-results/httpd.conf: Cannot load /usr/lib/apache2/modules/mod_mime.so into server: dlopen(/usr/lib/apache2/modules/mod_mime.so, 10): image not found
Comment 5 Andras Becsi 2011-07-11 04:26:59 PDT
Assigning this bug to Kristóf, since he has more knowledge of python than I have.
Comment 6 Andras Becsi 2011-07-11 04:28:03 PDT
Comment on attachment 99977 [details]
proposed fix

Marking my patch obsolete.
Comment 7 Csaba Osztrogonác 2011-07-11 08:40:59 PDT
I tried to disable NRWT on qt-mac platform:
http://trac.webkit.org/changeset/90746

isDarwin() checks $^O, and it should be "darwin" on the Mac bot.
Dihan, could you check what is $^O on the bot? (print $^O; in perl)
Comment 8 Dihan Wickremasuriya 2011-07-11 08:59:23 PDT
(In reply to comment #7)
> I tried to disable NRWT on qt-mac platform:
> http://trac.webkit.org/changeset/90746
> 
> isDarwin() checks $^O, and it should be "darwin" on the Mac bot.
> Dihan, could you check what is $^O on the bot? (print $^O; in perl)

It is indeed "darwin".
Comment 9 Csaba Osztrogonác 2011-07-11 09:00:43 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > I tried to disable NRWT on qt-mac platform:
> > http://trac.webkit.org/changeset/90746
> > 
> > isDarwin() checks $^O, and it should be "darwin" on the Mac bot.
> > Dihan, could you check what is $^O on the bot? (print $^O; in perl)
> 
> It is indeed "darwin".

The problem was that isSnowLeopard() is true for qt-mac too.
Fix landed in http://trac.webkit.org/changeset/90752.
Comment 10 Eric Seidel (no email) 2011-07-11 10:50:48 PDT
This is super-easy to fix.

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/qt.py#L66

Is the FIXME in question.  This is a 3 line python change.  Tell me where your httpd file is on all platforms you care about and I can do so.  (Or I can just copy the logic from ORWT).

Is this the only thing blocking NRWT no Qt?
Comment 11 Eric Seidel (no email) 2011-07-11 12:42:56 PDT
I have a patch to fix this.  I will post it this afternoon.
Comment 12 Eric Seidel (no email) 2011-07-11 16:28:05 PDT
Created attachment 100378 [details]
Patch
Comment 13 Andras Becsi 2011-07-11 17:06:38 PDT
Comment on attachment 100378 [details]
Patch

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

Thanks for fixing this, Eric.

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:396
> +        if sys_platform.startswith('linux'):
> +            if self._is_redhat_based():
> +                return 'fedora-httpd.conf'
> +            # FIXME: Seems wrong to assume that all non-redhat linux is debian.
> +            return 'apache2-debian-httpd.conf'
> +        if using_apache2:
> +            return "apache2-httpd.conf"

This seems wrong to me, too. ORWT checks for the /etc/debian_version file which all debian based distros have, and apache2-httpd.conf should be used if nor Fedora nor Debian was true and the version string contained "Apache/2" since for example on Slackware and Arch the apache2 binary is called httpd. All others should fallback to httpd.conf. This would mimic ORWT behaviour.
Comment 14 Dirk Pranke 2011-07-11 17:17:07 PDT
the patch looked fine to me apart from the apache2 on mac issue ...
Comment 15 Eric Seidel (no email) 2011-07-11 17:59:38 PDT
Created attachment 100395 [details]
Patch
Comment 16 Adam Barth 2011-07-12 00:27:01 PDT
Comment on attachment 100395 [details]
Patch

I'm not an expert on this code, but this looks fine.
Comment 17 WebKit Review Bot 2011-07-12 01:08:44 PDT
Comment on attachment 100395 [details]
Patch

Clearing flags on attachment: 100395

Committed r90810: <http://trac.webkit.org/changeset/90810>
Comment 18 WebKit Review Bot 2011-07-12 01:08:50 PDT
All reviewed patches have been landed.  Closing bug.