Summary: | test-webkitpy: Display a warning if testing with a version higher than 2.5 | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Jerdonek <cjerdonek> | ||||||
Component: | Tools / Tests | Assignee: | Chris Jerdonek <cjerdonek> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, cjerdonek, dpranke, eric, hamaji, levin | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 31533 | ||||||||
Attachments: |
|
Description
Chris Jerdonek
2010-03-05 07:20:31 PST
Created attachment 50097 [details]
Proposed patch
Note that this patch does not say anything about the minimum version that WebKit should support (we are still discussing that). It just says what webkitpy is currently intended to support (2.5).
Comment on attachment 50097 [details]
Proposed patch
Why would we want to display a warning if using something higher?
In that case, why wouldn't we just use #!/usr/bin/python2.5 and fail instead of warn?
I think a warning/error is only needed if python is < 2.5
Why not just "import webkitpy" here?
34 from webkitpy import MINIMUM_SUPPORTED_PYTHON_VERSION as _min_version
If version-checking is something the module should support, why not put the code outside of test-webkitpy?
And why not use logging.warn?
I think parts of this change are great! (like splitting out unttests.py). But since I expect python to be forward compatible (or mostly?) I am suprised we'd bother showing a warning to 2.6 users (which is all Mac OS X 10.6 users!)
Comment on attachment 50097 [details]
Proposed patch
r- for my above nits.
(In reply to comment #2) Thanks a lot, Eric! > (From update of attachment 50097 [details]) > Why would we want to display a warning if using something higher? It reminds developers using test-webkitpy that they should really be testing with Python 2.5 if they want to be sure of their change -- since that is what other contributors use. > In that case, why wouldn't we just use #!/usr/bin/python2.5 and fail instead of > warn? Not every developer has 2.5. This point was also made here: https://bugs.webkit.org/show_bug.cgi?id=31533#c2 I don't think we should prevent developers using 2.5 from developing fixes -- just warn them. For most small fixes, it's not critical for the individual developer to be testing with 2.5 as they develop, but it's good that they at least be aware. Also, I think test-webkitpy should succeed for versions higher than 2.5 on the basis that 2.5 is the minimum supported version. It wasn't meant to be the only supported version. > I think a warning/error is only needed if python is < 2.5 That sounds like a good idea. In this patch I was more concerned about developers using 2.6 rather than 2.4 since test-webkitpy already gives a message for 2.4 -- a failure message. :) But telling them why it fails would be a nice addition. > Why not just "import webkitpy" here? > 34 from webkitpy import MINIMUM_SUPPORTED_PYTHON_VERSION as _min_version I've gotten into the practice of importing particular identifiers because I find that it's easier this way to see what outside identifiers each module is using (i.e. what the dependencies are in a module). > If version-checking is something the module should support, why not put the > code outside of test-webkitpy? That's a good idea. I thought about doing that, and there are a few reasons I didn't do it in this patch. For starters, I wanted the warning to display prior to test-webkitpy erroring out. If we put the version-checking functionality in webkitpy itself, then test-webkitpy can error out before getting to the version check. This is because webkit/__init__.py executes code (autoinstall stuff). It's on my to-do list to move the autoinstall initialization code out of webkit/__init__.py into a different place (probably webkit/thirdparty/autoinstalled/__init__.py). That will allow people to import from webkitpy without immediately triggering errors. This also one of the reasons I put the call to Init() before importing the unit tests. (I made a code comment about this.) The other reasons have to do with only needing this for test-webkitpy for now and not having a good place for it. test-webkitpy isn't a wrapper for a corresponding module in webkitpy. And I didn't want to put the code in a shared area until being more sure of what the API should be. > And why not use logging.warn? In the end I think we should. I wanted to postpone logging changes to a later patch because there are other logging considerations that should probably be taken into account, and I didn't want to get into them here. They are mostly around distinguishing between log messages you do want to show and messages you don't want to show (because they are side effects of testing code that has logging). > I think parts of this change are great! (like splitting out unttests.py). But > since I expect python to be forward compatible (or mostly?) I am suprised we'd > bother showing a warning to 2.6 users (which is all Mac OS X 10.6 users!) This was actually one of the driving reasons for this patch -- because it can be a surprise to 2.6 users that not everyone else is using 2.6, and that this matters (it was to me until I broke stuff). This patch helps inform 2.6 users that they should take 2.5 compatibility into consideration when developing Python changes. (In reply to comment #4) > I don't think we should prevent developers using 2.5 from developing fixes -- > just warn them. ^not using (In reply to comment #4) > > If version-checking is something the module should support, why not put the > > code outside of test-webkitpy? > > That's a good idea. I thought about doing that, and there are a few reasons I > didn't do it in this patch. For starters, I wanted the warning to display > prior to test-webkitpy erroring out. If we put the version-checking > functionality in webkitpy itself, then test-webkitpy can error out before > getting to the version check. This is because webkit/__init__.py executes code > (autoinstall stuff). > > It's on my to-do list to move the autoinstall initialization code out of > webkit/__init__.py into a different place (probably > webkit/thirdparty/autoinstalled/__init__.py). That will allow people to import > from webkitpy without immediately triggering errors. I made a report for this to-do here: https://bugs.webkit.org/show_bug.cgi?id=35828 Created attachment 50156 [details]
Proposed patch 2
(In reply to comment #2) In this latest patch-- > I think a warning/error is only needed if python is < 2.5 I added a warning for < 2.5. This code is in the versioning.check_version() method. > Why not just "import webkitpy" here? > 34 from webkitpy import MINIMUM_SUPPORTED_PYTHON_VERSION as _min_version There is no longer a need to import this value. It is in webkitpy/init/versioning.py. > If version-checking is something the module should support, why not put the > code outside of test-webkitpy? Done. > And why not use logging.warn? Done. I am addressing the issues I raised in my comment 4 in separate patches so that I could address your suggestions in this patch. These additional issues are-- (1) Move the autoinstall code out of webkitpy/__init__.py. https://bugs.webkit.org/show_bug.cgi?id=35828 (2) Adjust the unit tests as necessary so that "side-effect" log messages do not render to the screen while running unit tests. This mainly involves adjusting the code in webkit/init/logtesting.py. (No bug report yet.) (In reply to comment #8) > (2) Adjust the unit tests as necessary so that "side-effect" log messages do > not render to the screen while running unit tests. This mainly involves > adjusting the code in webkit/init/logtesting.py. (No bug report yet.) Created a report for this here: https://bugs.webkit.org/show_bug.cgi?id=35835 Comment on attachment 50156 [details]
Proposed patch 2
I was hoping you were going to fix my --all hack too. :)
(In reply to comment #10) > (From update of attachment 50156 [details]) > I was hoping you were going to fix my --all hack too. :) One thing at a time... :) Thanks again for reviewing all these patches, Adam. I really appreciate it! Well, I appreciate your patches. Our Python is become much better structured thanks to you. Manually committed (via git svn dcommit): http://trac.webkit.org/changeset/55970 |