Add module dir to sys.path in FindPythonDependencies

Add module dir to sys.path in FindPythonDependencies

while loading modules using imp.load_source()

find_dependencies script was broken
after modifications done in tools/perf/run_benchmark
CL https://codereview.chromium.org/1280903003

BUG=527836
Committed: https://crrev.com/0c3d43f5dcef4c7334c044d3ab363eb8bf883ee2
Cr-Commit-Position: refs/heads/master@{#348653}

[alexanderk, 2015-09-04 15:17:03]

alexanderk@opera.com changed reviewers:
+ aiolos@chromium.org, eakuefner@chromium.org, nednguyen@google.com

[nednguyen, 2015-09-04 15:30:54]

https://codereview.chromium.org/1306953007/diff/1/tools/telemetry/telemetry/i...
File tools/telemetry/telemetry/internal/util/find_dependencies.py (right):

https://codereview.chromium.org/1306953007/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/util/find_dependencies.py:40:
sys.path.insert(0, os.path.abspath(os.path.dirname(module_path)))
Can you unload the sys.path at the end of the function in a finally block?
We do have the case of module name conflicts & want to isolate the modules as
much as possible.

[alexanderk, 2015-09-04 16:07:21]

On 2015/09/04 15:30:54, nednguyen wrote:
>
https://codereview.chromium.org/1306953007/diff/1/tools/telemetry/telemetry/i...
> File tools/telemetry/telemetry/internal/util/find_dependencies.py (right):
> 
>
https://codereview.chromium.org/1306953007/diff/1/tools/telemetry/telemetry/i...
> tools/telemetry/telemetry/internal/util/find_dependencies.py:40:
> sys.path.insert(0, os.path.abspath(os.path.dirname(module_path)))
> Can you unload the sys.path at the end of the function in a finally block?
> We do have the case of module name conflicts & want to isolate the modules as
> much as possible.

BTW, do we really need to call imp.load_source() here?

[nednguyen, 2015-09-04 17:12:22]

nednguyen@google.com changed reviewers:
+ dtu@chromium.org

[nednguyen, 2015-09-04 17:12:22]

+Dave: do we need impl.loadsource line?

[dtu, 2015-09-04 17:27:48]

On 2015/09/04 17:12:22, nednguyen wrote:
> +Dave: do we need impl.loadsource line?

Hm, not sure how modulegraph differs from modulefinder here, but - I think we
only need this if arbitrary modules modify sys.path. So the completion of
Ethan's third_party work / removing util.AddDirToPythonPath would remove the
need for it.

[eakuefner, 2015-09-04 17:55:06]

On 2015/09/04 at 17:27:48, dtu wrote:
> On 2015/09/04 17:12:22, nednguyen wrote:
> > +Dave: do we need impl.loadsource line?
> 
> Hm, not sure how modulegraph differs from modulefinder here, but - I think we
only need this if arbitrary modules modify sys.path. So the completion of
Ethan's third_party work / removing util.AddDirToPythonPath would remove the
need for it.

Let's keep it for now since we're not done adding things to Telemetry's
__init__.py and I don't want that to block landing this fix.

lgtm but please wait for stamp from aiolos, dtu, or nednguyen.

[aiolos (Not reviewing), 2015-09-08 09:06:06]

On 2015/09/04 17:55:06, eakuefner wrote:
> On 2015/09/04 at 17:27:48, dtu wrote:
> > On 2015/09/04 17:12:22, nednguyen wrote:
> > > +Dave: do we need impl.loadsource line?
> > 
> > Hm, not sure how modulegraph differs from modulefinder here, but - I think
we
> only need this if arbitrary modules modify sys.path. So the completion of
> Ethan's third_party work / removing util.AddDirToPythonPath would remove the
> need for it.
> 
> Let's keep it for now since we're not done adding things to Telemetry's
> __init__.py and I don't want that to block landing this fix.
> 
> lgtm but please wait for stamp from aiolos, dtu, or nednguyen.

lgtm. Can you write up a bug for removing the impl.loadsource call once the
__init__.py file changes are done?

[nednguyen, 2015-09-08 14:57:54]

lgtm

[nednguyen, 2015-09-08 14:58:35]

On 2015/09/08 14:57:54, nednguyen wrote:
> lgtm

Actually, can you also add a test in find_dependencies_unittest that catches
this breakage?

[alexanderk, 2015-09-11 14:02:08]

> > > Hm, not sure how modulegraph differs from modulefinder here, but - I think
> we
> > only need this if arbitrary modules modify sys.path. So the completion of
> > Ethan's third_party work / removing util.AddDirToPythonPath would remove the
> > need for it.
> > 
> > Let's keep it for now since we're not done adding things to Telemetry's
> > __init__.py and I don't want that to block landing this fix.
> > 
> > lgtm but please wait for stamp from aiolos, dtu, or nednguyen.
> 
> lgtm. Can you write up a bug for removing the impl.loadsource call once the
> __init__.py file changes are done?

https://crbug.com/530608

[aiolos (Not reviewing), 2015-09-11 14:03:26]

On 2015/09/11 14:02:08, alexanderk wrote:
> > > > Hm, not sure how modulegraph differs from modulefinder here, but - I
think
> > we
> > > only need this if arbitrary modules modify sys.path. So the completion of
> > > Ethan's third_party work / removing util.AddDirToPythonPath would remove
the
> > > need for it.
> > > 
> > > Let's keep it for now since we're not done adding things to Telemetry's
> > > __init__.py and I don't want that to block landing this fix.
> > > 
> > > lgtm but please wait for stamp from aiolos, dtu, or nednguyen.
> > 
> > lgtm. Can you write up a bug for removing the impl.loadsource call once the
> > __init__.py file changes are done?
> 
> https://crbug.com/530608

Thank you!

[eakuefner, 2015-09-14 16:16:55]

The CQ bit was checked by eakuefner@chromium.org

[eakuefner, 2015-09-14 16:16:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org, aiolos@chromium.org, nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/1306953007/#ps40001
(title: "Update to current master")

[commit-bot: I haz the power, 2015-09-14 16:17:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306953007/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306953007/40001

[eakuefner, 2015-09-14 16:21:25]

The CQ bit was unchecked by eakuefner@chromium.org

[eakuefner, 2015-09-14 16:23:40]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2015-09-14 16:24:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306953007/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306953007/40001

[commit-bot: I haz the power, 2015-09-14 17:56:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-14 17:57:55]

Patchset 3 (id:??) landed as
https://crrev.com/0c3d43f5dcef4c7334c044d3ab363eb8bf883ee2
Cr-Commit-Position: refs/heads/master@{#348653}

[alexanderk, 2015-09-15 08:17:59]

On 2015/09/08 14:58:35, nednguyen wrote:
> On 2015/09/08 14:57:54, nednguyen wrote:
> > lgtm
> 
> Actually, can you also add a test in find_dependencies_unittest that catches
> this breakage?

How do we run find_dependencies_unittest? Seems it is failing.
I run it like this: python -m unittest
telemetry.internal.util.find_dependencies_unittest

[nednguyen, 2015-09-15 14:59:11]

On 2015/09/15 08:17:59, alexanderk wrote:
> On 2015/09/08 14:58:35, nednguyen wrote:
> > On 2015/09/08 14:57:54, nednguyen wrote:
> > > lgtm
> > 
> > Actually, can you also add a test in find_dependencies_unittest that catches
> > this breakage?
> 
> How do we run find_dependencies_unittest? Seems it is failing.
> I run it like this: python -m unittest
> telemetry.internal.util.find_dependencies_unittest

You can run it using "/tools/telemetry/run_tests find_dependencies_unittest"

[nednguyen, 2015-09-15 15:01:10]

On 2015/09/15 14:59:11, nednguyen wrote:
> On 2015/09/15 08:17:59, alexanderk wrote:
> > On 2015/09/08 14:58:35, nednguyen wrote:
> > > On 2015/09/08 14:57:54, nednguyen wrote:
> > > > lgtm
> > > 
> > > Actually, can you also add a test in find_dependencies_unittest that
catches
> > > this breakage?
> > 
> > How do we run find_dependencies_unittest? Seems it is failing.
> > I run it like this: python -m unittest
> > telemetry.internal.util.find_dependencies_unittest
> 
> You can run it using "/tools/telemetry/run_tests find_dependencies_unittest"

Thanks for start working on adding unittest! Can you assign yourself as owner in
https://code.google.com/p/chromium/issues/detail?id=531562 ?

[alexanderk, 2015-09-15 16:29:32]

On 2015/09/15 14:59:11, nednguyen wrote:
> On 2015/09/15 08:17:59, alexanderk wrote:
> > On 2015/09/08 14:58:35, nednguyen wrote:
> > > On 2015/09/08 14:57:54, nednguyen wrote:
> > > > lgtm
> > > 
> > > Actually, can you also add a test in find_dependencies_unittest that
catches
> > > this breakage?
> > 
> > How do we run find_dependencies_unittest? Seems it is failing.
> > I run it like this: python -m unittest
> > telemetry.internal.util.find_dependencies_unittest
> 
> You can run it using "/tools/telemetry/run_tests find_dependencies_unittest"

Oh, thanks!

[alexanderk, 2015-09-15 16:30:51]

On 2015/09/15 15:01:10, nednguyen wrote:
> On 2015/09/15 14:59:11, nednguyen wrote:
> > On 2015/09/15 08:17:59, alexanderk wrote:
> > > On 2015/09/08 14:58:35, nednguyen wrote:
> > > > On 2015/09/08 14:57:54, nednguyen wrote:
> > > > > lgtm
> > > > 
> > > > Actually, can you also add a test in find_dependencies_unittest that
> catches
> > > > this breakage?
> > > 
> > > How do we run find_dependencies_unittest? Seems it is failing.
> > > I run it like this: python -m unittest
> > > telemetry.internal.util.find_dependencies_unittest
> > 
> > You can run it using "/tools/telemetry/run_tests find_dependencies_unittest"
> 
> Thanks for start working on adding unittest! Can you assign yourself as owner
in
> https://code.google.com/p/chromium/issues/detail?id=531562 ?

Actually I cannot - I have no permissions

[eakuefner, 2015-09-15 17:11:51]

On 2015/09/15 at 16:30:51, alexanderk wrote:
> On 2015/09/15 15:01:10, nednguyen wrote:
> > On 2015/09/15 14:59:11, nednguyen wrote:
> > > On 2015/09/15 08:17:59, alexanderk wrote:
> > > > On 2015/09/08 14:58:35, nednguyen wrote:
> > > > > On 2015/09/08 14:57:54, nednguyen wrote:
> > > > > > lgtm
> > > > > 
> > > > > Actually, can you also add a test in find_dependencies_unittest that
> > catches
> > > > > this breakage?
> > > > 
> > > > How do we run find_dependencies_unittest? Seems it is failing.
> > > > I run it like this: python -m unittest
> > > > telemetry.internal.util.find_dependencies_unittest
> > > 
> > > You can run it using "/tools/telemetry/run_tests
find_dependencies_unittest"
> > 
> > Thanks for start working on adding unittest! Can you assign yourself as
owner in
> > https://code.google.com/p/chromium/issues/detail?id=531562 ?
> 
> Actually I cannot - I have no permissions

I'll take ownership for the time being, and if you'd like to continue working on
the test I'm happy to review your CL (in which you can set BUG=531562).

[commit-bot: I haz the power, 2015-09-23 12:34:07]

Patchset 3 (id:??) landed as
https://crrev.com/0c3d43f5dcef4c7334c044d3ab363eb8bf883ee2
Cr-Commit-Position: refs/heads/master@{#348653}