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
Created attachment 97304 [details] WebKit-linux3-support.patch
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.
Created attachment 97313 [details] WebKit-linux3-support.patch
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.
Created attachment 97317 [details] WebKit-linux3-support.patch
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.
Created attachment 97322 [details] WebKit-linux3-support.patch Tony, I removed the parts related to gyp. Thanks for reviewing! /Torsten
Comment on attachment 97322 [details] WebKit-linux3-support.patch Looks good to me, but I'll let Tony do the real review.
Maybe we should check something off of platform instead of sys.platform?
(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.
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.
(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
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.
(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!
Comment on attachment 97322 [details] WebKit-linux3-support.patch Clearing flags on attachment: 97322 Committed r89031: <http://trac.webkit.org/changeset/89031>
All reviewed patches have been landed. Closing bug.