Bug 62730 – Python checks for sys.platform have to be relaxed and/or extended for Linux >=3.0-rc1 support

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Torsten]

Hi,

while trying to build the chromium web browser on a system with Linux kernel 3.0-rc2 installed, I realized that python reports 'linux3' in sys.platform for Linux kernel version >=3.0-rc1.

That breaks several lookup dicts and conditional expressions in chromium's own code as well as in WebKit.

I will attach a patch shortly, which incorporates the following change to the relevant parts of WebKit.

  Add support for Linux kernels >=3.0-rc1 in Python related build files by:
    - replacing occurences of sys.platform == 'linux2' with
       sys.platform.startswith('linux')
    - congregating occurences of sys.platform in ('linux[X]', 'linuxY', ...)
       to sys.platform.startswith('linux')
    - adding the key 'linux3' to all relevant lookup dicts

Unfortunately I can't check, whether my changes would enable Linux-3-compatibility for other WebKit-based browsers, too. So, perhaps, someone with more insight to WebKit could check the rest of the code for occurrences of, e.g., "sys.platform == 'linux2'" and see, if it breaks something, if they are changed to "sys.platform.startswith('linux')".

If there's something wrong with the formatting of my patch, or otherwise, please let me know. It's my first contribution to WebKit.

For reference:
http://code.google.com/p/chromium/issues/detail?id=85845
http://bugs.gentoo.org/show_bug.cgi?id=371245

Best regards,
Torsten

[Torsten]

Created attachment 97304 [details]
WebKit-linux3-support.patch

[WebKit Review Bot]

Attachment 97304 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ChangeLog', u'Source/Thi..." exit_code: 1

Tools/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Tools/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Tools/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Tools/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Tools/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:153:  missing whitespace around operator  [pep8/E225] [5]
Total errors found: 6 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Torsten]

Created attachment 97313 [details]
WebKit-linux3-support.patch

[WebKit Review Bot]

Attachment 97313 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ChangeLog', u'Source/Thi..." exit_code: 1

Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:153:  missing whitespace around operator  [pep8/E225] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Torsten]

Created attachment 97317 [details]
WebKit-linux3-support.patch

[Tony Chang]

Comment on attachment 97317 [details]
WebKit-linux3-support.patch

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

Other than the gyp changes, the rest looks fine.

> Source/ThirdParty/ChangeLog:12
> +        * gyp/pylib/gyp/__init__.py:

We fixed gyp separately right?  I think we shouldn't change this ThirdParty code and just update to a newer version of gyp that includes the fix.

[Torsten]

Created attachment 97322 [details]
WebKit-linux3-support.patch

Tony,

I removed the parts related to gyp. Thanks for reviewing!

/Torsten

[Adam Barth]

Comment on attachment 97322 [details]
WebKit-linux3-support.patch

Looks good to me, but I'll let Tony do the real review.

[Eric Seidel (no email)]

Maybe we should check something off of platform instead of sys.platform?

[Tony Chang]

(In reply to comment #9)
> Maybe we should check something off of platform instead of sys.platform?

Maybe platform.system()?  I see Linux (Ubuntu Maverick), Windows (Windows 7), and Darwin (OSX 10.5) on my machines.

[Eric Seidel (no email)]

Comment on attachment 97322 [details]
WebKit-linux3-support.patch

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

Overall I think this change is fine.  I think it could be made better by just removign a bunch of these unneeded sys.platform checks instead, as I describe below.

> Tools/Scripts/webkitpy/common/system/executive.py:278
> +        if sys.platform.startswith('linux') or sys.platform in ('darwin', 'cygwin'):

This can just be removed and made into a ! win32 branch.  We don't need ot fall-through to an assert, we can just assume win32 is the only crazy one.

> Tools/Scripts/webkitpy/common/system/file_lock.py:46
> +        if sys.platform.startswith('linux') or sys.platform in ('darwin', 'cygwin'):

Again we just want a !win32 branch.

> Tools/Scripts/webkitpy/common/system/file_lock.py:54
> +        if sys.platform.startswith('linux') or sys.platform in ('darwin', 'cygwin'):

!win32.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:153
> +        options.use_apache = sys.platform.startswith('linux') or sys.platform == 'darwin'

This seems reasonable.  Perhaps we should be moving to a platform.system() check, but this is also OK as is.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:507
> +        while ((directory != 'http' or sys.platform == 'darwin' or sys.platform.startswith('linux'))

No, we should just check options.use_apache somewhere.  We shouldn't be copying the use_apache check here.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py:44
> +        elif platform.startswith('linux'):

Seems reasonable.

[Torsten]

(In reply to comment #11)
> Overall I think this change is fine.  I think it could be made better by just removign a bunch of these unneeded sys.platform checks instead, as I describe below.
> 
> > Tools/Scripts/webkitpy/common/system/executive.py:278
> > +        if sys.platform.startswith('linux') or sys.platform in ('darwin', 'cygwin'):
> 
> This can just be removed and made into a ! win32 branch.  We don't need ot fall-through to an assert, we can just assume win32 is the only crazy one.

Hmm, so what about derivates, like mingw32? I'd like to be as general as possible, too, but can we really be sure, there aren't any oddities, except for win32?

> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:507
> > +        while ((directory != 'http' or sys.platform == 'darwin' or sys.platform.startswith('linux'))
> 
> No, we should just check options.use_apache somewhere.  We shouldn't be copying the use_apache check here.

Can you explain this in more detail, please? I didn't see any part of WebKit's code until the day before yesterday, so I'm still a bit lost therein. ;-)

/Torsten

[Eric Seidel (no email)]

Comment on attachment 97322 [details]
WebKit-linux3-support.patch

I think I'm asking you to do too much on your first patch.  This patch is definitely a win, and is fine as-is.

[Torsten]

(In reply to comment #13)
> (From update of attachment 97322 [details])
> I think I'm asking you to do too much on your first patch.  This patch is definitely a win, and is fine as-is.

Thanks for reviewing and commiting my patch!

[WebKit Review Bot]

Comment on attachment 97322 [details]
WebKit-linux3-support.patch

Clearing flags on attachment: 97322

Committed r89031: <http://trac.webkit.org/changeset/89031>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.