webkitpy: Don't fail if importing a port fails.

webkitpy: Don't fail if importing a port fails.

This makes it easier to run the layout tests on swarming.

Currently if running on Linux or Mac you still need all the extra
Android dependencies installed otherwise the whole script fails. This
patch allows importing the Android "port" to fail when it isn't in use.

BUG=524758

[mithro, 2016-11-03 11:23:52]

tansell@chromium.org changed reviewers:
+ dpranke@chromium.org, qyearsley@chromium.org

[mithro, 2016-11-03 11:23:52]

Hi!

This is a small tweak for helping with running layout tests on swarming. Without
this, all the android test code has to be shipped to the Linux / Mac / Windows
bots despite it never being used (as it bits are imported).

As far as I can tell, port names always start with the same value as the import.
Could we only call __import__ on ports which start with the name instead?

Tim 'mithro' Ansell

[mithro, 2016-11-03 11:24:14]

The CQ bit was checked by tansell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 11:24:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 12:46:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 12:46:50]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2016-11-03 18:59:38]

dpranke@chromium.org changed reviewers:
+ jbudorick@chromium.org

[Dirk Pranke, 2016-11-03 18:59:39]

+jbudorick

I think the goal is good, but I'm unsure about the implementation.

Specifically, I'm concerned about how changing the default logic might affect
some of the code coverage, particularly for test-webkitpy (webkit_python_tests).

I also wonder whether this would break the mock-android port, which would be
bad.

I'm fine with changing the import logic if neither of the above things are
affected. If they are, I'd like to find a different approach (maybe not
importing devil et al. until we know it's really needed)?

As to the implementation, I don't know that I would use the warnings module. I
haven't actually used that myself, but it looks like it's mostly used for
warning about things that are deprecated in Python (that might be an issue for
Python3, etc.)?

Generally speaking, I don't like warnings, because they are ignored or missed
and lead to confusion. In most cases (and I think this is one of them), either
they shouldn't produce a message at all because it's not a real problem, or it
should produce a real, user-friendly error and fail hard.

[jbudorick, 2016-11-03 19:07:43]

On 2016/11/03 18:59:39, Dirk Pranke wrote:
> +jbudorick
> 
> I think the goal is good, but I'm unsure about the implementation.
> 
> Specifically, I'm concerned about how changing the default logic might affect
> some of the code coverage, particularly for test-webkitpy
(webkit_python_tests).
> 
> I also wonder whether this would break the mock-android port, which would be
> bad.
> 
> I'm fine with changing the import logic if neither of the above things are
> affected. If they are, I'd like to find a different approach (maybe not
> importing devil et al. until we know it's really needed)?
> 
> As to the implementation, I don't know that I would use the warnings
module. I
> haven't actually used that myself, but it looks like it's mostly used for
> warning about things that are deprecated in Python (that might be an issue for
> Python3, etc.)?
> 
> Generally speaking, I don't like warnings, because they are ignored or missed
> and lead to confusion. In most cases (and I think this is one of them), either
> they shouldn't produce a message at all because it's not a real problem, or it
> should produce a real, user-friendly error and fail hard.

I agree with all of this. I don't mind doing delayed imports of the devil code
in port/android.py (and I already had to do it for the forwarder:
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts...).

[mithro, 2016-11-04 02:23:18]

On 2016/11/03 19:07:43, jbudorick wrote:
> On 2016/11/03 18:59:39, Dirk Pranke wrote:
> > +jbudorick
> > 
> > I think the goal is good, but I'm unsure about the implementation.

By the implementation, you mean the try/except around the import or only
importing the targets as needed?

The big problem with the try/except is that if you are working on the android
code and do something accidently to break the import (IE import an unavaliable
module or spell the module name wrong) it might take a long time to debug.

> > Specifically, I'm concerned about how changing the default logic might
affect
> > some of the code coverage, particularly for test-webkitpy
> (webkit_python_tests).
> > 
> > I also wonder whether this would break the mock-android port, which would be
> > bad.
> > 
> > I'm fine with changing the import logic if neither of the above things are
> > affected. If they are, I'd like to find a different approach (maybe not
> > importing devil et al. until we know it's really needed)?

I'm 100% sure we can refactor this code to not need to import all the modules.
However, I don't really understand this code and it seems *very* unusual. 

How can I test things like the mock-android port or the code coverage stuff?

> > As to the implementation, I don't know that I would use the warnings
> module. I
> > haven't actually used that myself, but it looks like it's mostly used for
> > warning about things that are deprecated in Python (that might be an issue
for
> > Python3, etc.)?
> > 
> > Generally speaking, I don't like warnings, because they are ignored or
missed
> > and lead to confusion. In most cases (and I think this is one of them),
either
> > they shouldn't produce a message at all because it's not a real problem, or
it
> > should produce a real, user-friendly error and fail hard.

I generally agree with this idea.

FYI: I tried log.warning instead of the warning module. I went with warning
because it just prints the first time rather than every time. The log.warning
method *also* has all the same problems you listed, so it isn't really an
alternative.

> I agree with all of this. I don't mind doing delayed imports of the devil code
> in port/android.py (and I already had to do it for the forwarder:
>
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts...).

I worry slightly about delayed imports, they tend to cause a lot of subtle bugs.
Both the Python style guide and our style guide recommends against using them
for this reason.

jbudorick@ are you working on the Android layout tests? At some point we will
probably want to figure out how to isolate the Android port too, so maybe we
just bite the bullet and do that now?

Tim 'mithro' Ansell

[Dirk Pranke, 2016-11-04 17:21:12]

On 2016/11/04 02:23:18, mithro wrote:
> On 2016/11/03 19:07:43, jbudorick wrote:
> > On 2016/11/03 18:59:39, Dirk Pranke wrote:
> > > +jbudorick
> > > 
> > > I think the goal is good, but I'm unsure about the implementation.
> 
> By the implementation, you mean the try/except around the import or only
> importing the targets as needed?

I mean this particular implementation of doing a try/except in factory.py that
would gloss over *any* import failure.

> 
> The big problem with the try/except is that if you are working on the android
> code and do something accidently to break the import (IE import an unavaliable
> module or spell the module name wrong) it might take a long time to debug.

Right.

> 
> > > Specifically, I'm concerned about how changing the default logic might
> affect
> > > some of the code coverage, particularly for test-webkitpy
> > (webkit_python_tests).
> > > 
> > > I also wonder whether this would break the mock-android port, which would
be
> > > bad.
> > > 
> > > I'm fine with changing the import logic if neither of the above things are
> > > affected. If they are, I'd like to find a different approach (maybe not
> > > importing devil et al. until we know it's really needed)?
> 
> I'm 100% sure we can refactor this code to not need to import all the modules.
> However, I don't really understand this code and it seems *very* unusual. 
> 
> How can I test things like the mock-android port or the code coverage stuff?

Running `run-webkit-tests --platform mock-android` by hand should just work
all the time on any platform (the mock-* ports aren't supposed to require
anything built, and are used to show what run-webkit-tests would normally
do and expect to happen assuming every test passed).

To do coverage testing, install the `coverage` python module locally and run
`test-webkitpy --coverage`.

> FYI: I tried log.warning instead of the warning module. I went with warning
> because it just prints the first time rather than every time. The log.warning
> method *also* has all the same problems you listed, so it isn't really an
> alternative.

Right, log.warning is no better.

> I worry slightly about delayed imports, they tend to cause a lot of subtle
bugs.
> Both the Python style guide and our style guide recommends against using them
> for this reason.

Yup, we shouldn't generally use delayed imports, but I think they are justified
in this case.

[jbudorick, 2016-11-04 21:15:29]

On 2016/11/04 02:23:18, mithro wrote:
> On 2016/11/03 19:07:43, jbudorick wrote:
> > On 2016/11/03 18:59:39, Dirk Pranke wrote:
> > > +jbudorick
> > > 
> > > I think the goal is good, but I'm unsure about the implementation.
> 
> By the implementation, you mean the try/except around the import or only
> importing the targets as needed?
> 
> The big problem with the try/except is that if you are working on the android
> code and do something accidently to break the import (IE import an unavaliable
> module or spell the module name wrong) it might take a long time to debug.
> 
> > > Specifically, I'm concerned about how changing the default logic might
> affect
> > > some of the code coverage, particularly for test-webkitpy
> > (webkit_python_tests).
> > > 
> > > I also wonder whether this would break the mock-android port, which would
be
> > > bad.
> > > 
> > > I'm fine with changing the import logic if neither of the above things are
> > > affected. If they are, I'd like to find a different approach (maybe not
> > > importing devil et al. until we know it's really needed)?
> 
> I'm 100% sure we can refactor this code to not need to import all the modules.
> However, I don't really understand this code and it seems *very* unusual. 
> 
> How can I test things like the mock-android port or the code coverage stuff?
> 
> > > As to the implementation, I don't know that I would use the warnings
> > module. I
> > > haven't actually used that myself, but it looks like it's mostly used for
> > > warning about things that are deprecated in Python (that might be an issue
> for
> > > Python3, etc.)?
> > > 
> > > Generally speaking, I don't like warnings, because they are ignored or
> missed
> > > and lead to confusion. In most cases (and I think this is one of them),
> either
> > > they shouldn't produce a message at all because it's not a real problem,
or
> it
> > > should produce a real, user-friendly error and fail hard.
> 
> I generally agree with this idea.
> 
> FYI: I tried log.warning instead of the warning module. I went with warning
> because it just prints the first time rather than every time. The log.warning
> method *also* has all the same problems you listed, so it isn't really an
> alternative.
> 
> > I agree with all of this. I don't mind doing delayed imports of the devil
code
> > in port/android.py (and I already had to do it for the forwarder:
> >
>
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts...).
> 
> I worry slightly about delayed imports, they tend to cause a lot of subtle
bugs.
> Both the Python style guide and our style guide recommends against using them
> for this reason.
> 
> jbudorick@ are you working on the Android layout tests? At some point we will
> probably want to figure out how to isolate the Android port too, so maybe we
> just bite the bullet and do that now?

I am, yes, with the intention of making them work on newer versions w/ SELinux
on.

We definitely will want to isolate the Android port at some point, but I don't
have time to spend on doing so now.

> 
> Tim 'mithro' Ansell