Summary: | Python checks for sys.platform have to be relaxed and/or extended for Linux >=3.0-rc1 support | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Torsten <gentoo> | ||||||||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, eric, tony, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
Torsten
2011-06-15 08:23:24 PDT
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. |