Bug 35788 – test-webkitpy: Display a warning if testing with a version higher than 2.5

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Jerdonek]

webkitpy was developed to work with Python 2.5 or higher.  We should display a warning to the user if they are testing it with something higher than that.  Otherwise, the user can be misled into thinking their changes are fine when they are really not (this has happened to me a couple times now):

https://bugs.webkit.org/show_bug.cgi?id=33365
https://bugs.webkit.org/show_bug.cgi?id=35163#c21

Note that we are still in discussion about whether to reduce 2.5 to something lower:

https://lists.webkit.org/pipermail/webkit-dev/2010-March/011810.html
https://bugs.webkit.org/show_bug.cgi?id=35584

That discussion will not change the substance of this patch (we would just need to update the configuration variable).

[Chris Jerdonek]

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).

[Eric Seidel (no email)]

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!)

[Eric Seidel (no email)]

Comment on attachment 50097 [details]
Proposed patch

r- for my above nits.

[Chris Jerdonek]

(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.

[Chris Jerdonek]

(In reply to comment #4)
> I don't think we should prevent developers using 2.5 from developing fixes --
> just warn them.

^not using

[Chris Jerdonek]

(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

[Chris Jerdonek]

Created attachment 50156 [details]
Proposed patch 2

[Chris Jerdonek]

(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.)

[Chris Jerdonek]

(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

[Adam Barth]

Comment on attachment 50156 [details]
Proposed patch 2

I was hoping you were going to fix my --all hack too.  :)

[Chris Jerdonek]

(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!

[Adam Barth]

Well, I appreciate your patches.  Our Python is become much better structured thanks to you.

[Chris Jerdonek]

Manually committed (via git svn dcommit):

http://trac.webkit.org/changeset/55970