Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2489393002:
Use the proper sys.path in build/print_python_deps.py (Closed)
DescriptionUse the proper sys.path in build/print_python_deps.py
We need to ensure the main module's directory is the first entry in
sys.path.
BUG=662258
Committed: https://crrev.com/69a180f7e6da80b60b1b2bca0eee0acab322f9aa
Cr-Commit-Position: refs/heads/master@{#431327}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 15 (8 generated)
Description was changed from ========== Use the proper sys.path in build/print_python_deps.py We need to ensure the main module's directory is the first entry in sys.path. BUG=662258 ========== to ========== Use the proper sys.path in build/print_python_deps.py We need to ensure the main module's directory is the first entry in sys.path. BUG=662258 ==========
agrieve@chromium.org changed reviewers: + jbudorick@chromium.org
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2489393002/diff/1/build/print_python_deps.py File build/print_python_deps.py (right): https://codereview.chromium.org/2489393002/diff/1/build/print_python_deps.py#... build/print_python_deps.py:87: sys.path[0] = os.path.dirname(options.module) Do we want anything else in sys.path at all? I'm concerned that this won't work in some cases if the modules don't resolve to something in the options.module path.
https://codereview.chromium.org/2489393002/diff/1/build/print_python_deps.py File build/print_python_deps.py (right): https://codereview.chromium.org/2489393002/diff/1/build/print_python_deps.py#... build/print_python_deps.py:87: sys.path[0] = os.path.dirname(options.module) On 2016/11/10 19:41:02, jbudorick wrote: > Do we want anything else in sys.path at all? I'm concerned that this won't work > in some cases if the modules don't resolve to something in the options.module > path. If you clear out sys.path, then normal system import don't work (e.g. import os). I did a local test with a file that just prints sys.path: PYTHONPATH=a python hi.py ['/usr/local/google/home/agrieve/ssd/git/clankium1/src', '/usr/local/google/home/agrieve/ssd/git/clankium1/src/a', '/usr/lib/python2.7', '/usr/lib/python2.7/plat-x86_64-linux-gnu', '/usr/lib/python2.7/lib-tk', '/usr/lib/python2.7/lib-old', '/usr/lib/python2.7/lib-dynload', '/usr/local/lib/python2.7/dist-packages', '/usr/lib/python2.7/dist-packages', '/usr/lib/python2.7/dist-packages/PILcompat', '/usr/lib/python2.7/dist-packages/gtk-2.0', '/usr/lib/pymodules/python2.7', '/usr/lib/python2.7/dist-packages/ubuntu-sso-client'] 1st entry is module dir, 2nd is PYTHONPATH, remaining is system paths
lgtm https://codereview.chromium.org/2489393002/diff/1/build/print_python_deps.py File build/print_python_deps.py (right): https://codereview.chromium.org/2489393002/diff/1/build/print_python_deps.py#... build/print_python_deps.py:87: sys.path[0] = os.path.dirname(options.module) On 2016/11/10 19:45:35, agrieve wrote: > On 2016/11/10 19:41:02, jbudorick wrote: > > Do we want anything else in sys.path at all? I'm concerned that this won't > work > > in some cases if the modules don't resolve to something in the options.module > > path. > > If you clear out sys.path, then normal system import don't work (e.g. import > os). I did a local test with a file that just prints sys.path: > > PYTHONPATH=a python hi.py > ['/usr/local/google/home/agrieve/ssd/git/clankium1/src', > '/usr/local/google/home/agrieve/ssd/git/clankium1/src/a', '/usr/lib/python2.7', > '/usr/lib/python2.7/plat-x86_64-linux-gnu', '/usr/lib/python2.7/lib-tk', > '/usr/lib/python2.7/lib-old', '/usr/lib/python2.7/lib-dynload', > '/usr/local/lib/python2.7/dist-packages', '/usr/lib/python2.7/dist-packages', > '/usr/lib/python2.7/dist-packages/PILcompat', > '/usr/lib/python2.7/dist-packages/gtk-2.0', '/usr/lib/pymodules/python2.7', > '/usr/lib/python2.7/dist-packages/ubuntu-sso-client'] > > 1st entry is module dir, 2nd is PYTHONPATH, remaining is system paths I guess I'm concerned about the case where a module appends to sys.path then imports something from that, but it conflicts with something on PYTHONPATH. Maybe that's too much of a corner case for this script to worry about, though -- perhaps the responsibility for not passing the infra PYTHONPATH contents falls on the infra side of the fence.
On 2016/11/10 19:49:12, jbudorick wrote: > lgtm > > https://codereview.chromium.org/2489393002/diff/1/build/print_python_deps.py > File build/print_python_deps.py (right): > > https://codereview.chromium.org/2489393002/diff/1/build/print_python_deps.py#... > build/print_python_deps.py:87: sys.path[0] = os.path.dirname(options.module) > On 2016/11/10 19:45:35, agrieve wrote: > > On 2016/11/10 19:41:02, jbudorick wrote: > > > Do we want anything else in sys.path at all? I'm concerned that this won't > > work > > > in some cases if the modules don't resolve to something in the > options.module > > > path. > > > > If you clear out sys.path, then normal system import don't work (e.g. import > > os). I did a local test with a file that just prints sys.path: > > > > PYTHONPATH=a python hi.py > > ['/usr/local/google/home/agrieve/ssd/git/clankium1/src', > > '/usr/local/google/home/agrieve/ssd/git/clankium1/src/a', > '/usr/lib/python2.7', > > '/usr/lib/python2.7/plat-x86_64-linux-gnu', '/usr/lib/python2.7/lib-tk', > > '/usr/lib/python2.7/lib-old', '/usr/lib/python2.7/lib-dynload', > > '/usr/local/lib/python2.7/dist-packages', '/usr/lib/python2.7/dist-packages', > > '/usr/lib/python2.7/dist-packages/PILcompat', > > '/usr/lib/python2.7/dist-packages/gtk-2.0', '/usr/lib/pymodules/python2.7', > > '/usr/lib/python2.7/dist-packages/ubuntu-sso-client'] > > > > 1st entry is module dir, 2nd is PYTHONPATH, remaining is system paths > > I guess I'm concerned about the case where a module appends to sys.path then > imports something from that, but it conflicts with something on PYTHONPATH. > Maybe that's too much of a corner case for this script to worry about, though -- > perhaps the responsibility for not passing the infra PYTHONPATH contents falls > on the infra side of the fence. Yeah, that setup would result in the same wrong module being used when running the script. This fix should be enough to at least make running vs finding deps consistent.
The CQ bit was unchecked by agrieve@chromium.org
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use the proper sys.path in build/print_python_deps.py We need to ensure the main module's directory is the first entry in sys.path. BUG=662258 ========== to ========== Use the proper sys.path in build/print_python_deps.py We need to ensure the main module's directory is the first entry in sys.path. BUG=662258 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Use the proper sys.path in build/print_python_deps.py We need to ensure the main module's directory is the first entry in sys.path. BUG=662258 ========== to ========== Use the proper sys.path in build/print_python_deps.py We need to ensure the main module's directory is the first entry in sys.path. BUG=662258 Committed: https://crrev.com/69a180f7e6da80b60b1b2bca0eee0acab322f9aa Cr-Commit-Position: refs/heads/master@{#431327} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/69a180f7e6da80b60b1b2bca0eee0acab322f9aa Cr-Commit-Position: refs/heads/master@{#431327} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
