New issue
Advanced search Search tips

Issue 825290 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 826871



Sign in to add a comment

Clean up PYTHONPATH on buildbot

Project Member Reported by iannucci@chromium.org, Mar 23 2018

Issue description

For a long time we've (incorrectly) exported all of build.git's third_party libraries into PYTHONPATH. We no longer do this at all on LUCI, and most things seem to mostly work, so we should stop doing the bad thing and clean this up once and for all :)

As a side-effect, we currently set the envvar $VPYTHON_CLEAR_PYTHONPATH on buildbot to give vpython invocations a clean environment.

This has the unfortunate side effect for e.g. presubmit_support when it needs to adjust PYTHONPATH. It correctly tweaks PYTHONPATH, and then invokes the sub-process with vpython... which then blows away PYTHONPATH :)

For now I'm going to put a band-aid in presubmit_support to clear the VPYTHON_CLEAR_PYTHONPATH when it adjusts PYTHONPATH, but the real fix is to stop exporting this stuff in run_slave.py.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/23bd73535c4ca820910393e088ba62f868ad5fec

commit 23bd73535c4ca820910393e088ba62f868ad5fec
Author: Robert Iannucci <iannucci@chromium.org>
Date: Fri Mar 23 19:05:27 2018

Clear $VPYTHON_CLEAR_PYTHONPATH when setting $PYTHONPATH.

On buildbot (only), we don't trust PYTHONPATH, so we have set
VPYTHON_CLEAR_PYTHONPATH to make the first invocation of vpython in a
given chain of python invocations clear PYTHONPATH.

This has the unfortunate side-effect that python scripts that set
PYTHONPATH and then call into vpython (like presubmit) end up
having their PYTHONPATH modifications removed.

As a workaround (until I clean up PYTHONPATH in buildbot for real), fix
presubmit_canned_checks so that it clears VPYTHON_CLEAR_PYTHONPATH when it
adjusts PYTHONPATH.

R=charlie@chromium.org, nodir@chromium.org, vadimsh@chromium.org

Bug:  825290 , 825174 
Change-Id: Ib5f73add1726fdf3c335d26fc0af76cfe3b747b2
Reviewed-on: https://chromium-review.googlesource.com/978632
Reviewed-by: Charlie Andrews <charliea@chromium.org>
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>

[modify] https://crrev.com/23bd73535c4ca820910393e088ba62f868ad5fec/presubmit_canned_checks.py
[modify] https://crrev.com/23bd73535c4ca820910393e088ba62f868ad5fec/recipes/trigger_recipe_roller.txt

Project Member

Comment 2 by bugdroid1@chromium.org, Mar 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/325503ce9c14a4685fb36be0c7df9053e7b5ab9b

commit 325503ce9c14a4685fb36be0c7df9053e7b5ab9b
Author: depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Mar 23 23:25:39 2018

Roll src/third_party/depot_tools/ 728244f70..0d9ecc925 (3 commits)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/728244f70a8b..0d9ecc925d38

$ git log 728244f70..0d9ecc925 --date=short --no-merges --format='%ad %ae %s'
2018-03-23 ehmaldonado Reland "gclient eval: Expand vars while parsing DEPS files"
2018-03-23 sergiyb Correct documentation for the --no-referenced-issues argument
2018-03-23 iannucci Clear $VPYTHON_CLEAR_PYTHONPATH when setting $PYTHONPATH.

Created with:
  roll-dep src/third_party/depot_tools
BUG= chromium:821199 , chromium:825290 , chromium:825174 


The AutoRoll server is located here: https://depot-tools-chromium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=agable@chromium.org

Change-Id: I58cbb0ca469176bb99a5a86d7504c986fa9b162b
Reviewed-on: https://chromium-review.googlesource.com/978865
Commit-Queue: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Reviewed-by: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#545622}
[modify] https://crrev.com/325503ce9c14a4685fb36be0c7df9053e7b5ab9b/DEPS

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/82a6480a3a0d67c1d8ad99809eafcbf87468fb52

commit 82a6480a3a0d67c1d8ad99809eafcbf87468fb52
Author: Robert Iannucci <iannucci@chromium.org>
Date: Fri Mar 23 23:45:49 2018

[presubmit_canned_checks] Stop exporting sys.path into PYTHONPATH.

Again, sys.path shouldn't ever be exported into PYTHONPATH :)

R=eakuefner@chromium.org, nodir@chromium.org, vadimsh@chromium.org

Bug:  825290 , 825174 
Change-Id: Ia0594da3ff25972a08fdf74ae2aef5be79cbf3af
Reviewed-on: https://chromium-review.googlesource.com/978594
Reviewed-by: Ethan Kuefner <eakuefner@chromium.org>
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>

[modify] https://crrev.com/82a6480a3a0d67c1d8ad99809eafcbf87468fb52/presubmit_canned_checks.py
[modify] https://crrev.com/82a6480a3a0d67c1d8ad99809eafcbf87468fb52/recipes/trigger_recipe_roller.txt

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/b4f1d4498c1ceed46868f003bbf6dae2097be5a6

commit b4f1d4498c1ceed46868f003bbf6dae2097be5a6
Author: Robert Iannucci <iannucci@chromium.org>
Date: Fri Mar 23 23:53:59 2018

Fix actual pylint errors.

TBR=eakuefner@chromium.org, nodir@chromium.org, vadimsh@chromium.org

Bug:  825290 , 825174 
Change-Id: Ide4f03b0f9100a2110bad9d510921b46b2d4e43a
Reviewed-on: https://chromium-review.googlesource.com/979112
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>

[modify] https://crrev.com/b4f1d4498c1ceed46868f003bbf6dae2097be5a6/tests/gclient_eval_unittest.py

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/dbbf350a34ab7991e635f91a70bd2d211eb921be

commit dbbf350a34ab7991e635f91a70bd2d211eb921be
Author: Robert Iannucci <iannucci@chromium.org>
Date: Sat Mar 24 00:01:34 2018

Fix trailing whitespace test.

And write the test better to avoid overzealous editor plugins :)

TBR=eakuefner@chromium.org, nodir@chromium.org, vadimsh@chromium.org

Bug:  825290 , 825174 
Change-Id: I0541d4d70bcb256c24cb659bae256fa0aacb6806
Reviewed-on: https://chromium-review.googlesource.com/979116
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>

[modify] https://crrev.com/dbbf350a34ab7991e635f91a70bd2d211eb921be/tests/gclient_eval_unittest.py

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e6be243fdc31f9bda03caf3c206a8982b45e5a84

commit e6be243fdc31f9bda03caf3c206a8982b45e5a84
Author: depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Sat Mar 24 01:38:36 2018

Roll src/third_party/depot_tools/ 0d9ecc925..82a6480a3 (1 commit)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/0d9ecc925d38..82a6480a3a0d

$ git log 0d9ecc925..82a6480a3 --date=short --no-merges --format='%ad %ae %s'
2018-03-23 iannucci [presubmit_canned_checks] Stop exporting sys.path into PYTHONPATH.

Created with:
  roll-dep src/third_party/depot_tools
BUG= chromium:825290 , chromium:825174 


The AutoRoll server is located here: https://depot-tools-chromium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=agable@chromium.org

Change-Id: Ic28a910f52c64051a17e2a9f94a3886f7bb237f2
Reviewed-on: https://chromium-review.googlesource.com/979114
Reviewed-by: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#545655}
[modify] https://crrev.com/e6be243fdc31f9bda03caf3c206a8982b45e5a84/DEPS

Project Member

Comment 7 by bugdroid1@chromium.org, Mar 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/27393a0dc7d9d6c63529b1e71847b477b53903bf

commit 27393a0dc7d9d6c63529b1e71847b477b53903bf
Author: Robert Iannucci <iannucci@chromium.org>
Date: Sat Mar 24 03:57:35 2018

[build/android/PRESUBMIT.py] Add explicit PYTHONPATH dep on depot_tools.

R=jbudorick@chromium.org

Bug:  825290 
Change-Id: Ic87937f62f390c567c7d0a5ed48795096574afff
Reviewed-on: https://chromium-review.googlesource.com/979257
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545668}
[modify] https://crrev.com/27393a0dc7d9d6c63529b1e71847b477b53903bf/build/android/PRESUBMIT.py

Project Member

Comment 8 by bugdroid1@chromium.org, Mar 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd97a8c02119790f363670851c2417fbb9236112

commit cd97a8c02119790f363670851c2417fbb9236112
Author: depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Sat Mar 24 04:59:02 2018

Roll src/third_party/depot_tools/ 82a6480a3..dbbf350a3 (2 commits)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/82a6480a3a0d..dbbf350a34ab

$ git log 82a6480a3..dbbf350a3 --date=short --no-merges --format='%ad %ae %s'
2018-03-23 iannucci Fix trailing whitespace test.
2018-03-23 iannucci Fix actual pylint errors.

Created with:
  roll-dep src/third_party/depot_tools
BUG= chromium:825290 , chromium:825174 , chromium:825290 , chromium:825174 


The AutoRoll server is located here: https://depot-tools-chromium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=agable@chromium.org

Change-Id: If3ca536bc4f21f1f692b3c6ebdffae05203db93b
Reviewed-on: https://chromium-review.googlesource.com/979293
Reviewed-by: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#545672}
[modify] https://crrev.com/cd97a8c02119790f363670851c2417fbb9236112/DEPS

Blockedon: 826871
Project Member

Comment 10 by bugdroid1@chromium.org, May 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/c55d8676ce4193462972ae3f0d17826ea7b0978f

commit c55d8676ce4193462972ae3f0d17826ea7b0978f
Author: Robert Iannucci <iannucci@chromium.org>
Date: Tue May 01 08:06:38 2018

[common/env.py] Reland: Remove GetSysPythonPath and GetEnvPythonPath.

GetSysPythonPath is interfering with using vpython for sub-invocations
(and I believe it was actually never necessary and was added due to a
misunderstanding of how PYTHONPATH works).

PYTHONPATH is prefixed to the internal calculation of sys.path of
whatever python interpreter starts up. The interpreter takes PYTHONPATH
from the env, then adds in any libraries which are installed with that
interpreter.

When you're using system python for both runit and also the subprocess,
exporting sys.path to PYTHONPATH is redundant, but not harmful. e.g.
runit would export:

  PYTHONPATH=/build.git:/sys_python

And then when the subpython starts up, it would calculate sys.path to be:

  sys.path=/build.git:/sys_python:/sys_python

However, when you're using vpython to invoke runit and the subprocess
with two different virtualenvs, you get:

  PYTHONPATH=/build.git:/vpython/deadbeef
  sys.path=/build.git:/vpython/deadbeef:/vpython/feedface

Where 'feedface' is the environment that your script actually wants!
By removing GetSysPythonPath from the equation, we'll now do:

  PYTHONPATH=/build.git
  sys.path=/build.git:/vpython/feedface

Which should actually work correctly. I believe that GetSysPythonPath
was added under the assumption that python ONLY consults PYTHONPATH for
libraries when PYTHONPATH is set in the environment, and so we needed
to preserve the current sys.path entries for correct behavior.

GetEnvPythonPath seems to be entirely unused.

Original CL: https://chromium-review.googlesource.com/974759
Fix for original revert: https://chromium-review.googlesource.com/976876

R=dnj@chromium.org, nodir@chromium.org, vadimsh@chromium.org, yyanagisawa@chromium.org

Bug: 776430,814607, 822103 ,822042, 825290 
Change-Id: I5eb0d32fd94ae4282f6877c894e6d00f01e810ec
Reviewed-on: https://chromium-review.googlesource.com/976878
Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/c55d8676ce4193462972ae3f0d17826ea7b0978f/scripts/common/env.py
[modify] https://crrev.com/c55d8676ce4193462972ae3f0d17826ea7b0978f/scripts/common/unittests/env_test.py

Project Member

Comment 11 by bugdroid1@chromium.org, May 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/da82356fbf21e140a6b1bc749f85b52adee3aeec

commit da82356fbf21e140a6b1bc749f85b52adee3aeec
Author: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Date: Tue May 01 08:57:19 2018

Revert "[common/env.py] Reland: Remove GetSysPythonPath and GetEnvPythonPath."

This reverts commit c55d8676ce4193462972ae3f0d17826ea7b0978f.

Reason for revert: 
This cl might have broken following builder.
https://ci.chromium.org/buildbot/chromium/Mac/41039

Original change's description:
> [common/env.py] Reland: Remove GetSysPythonPath and GetEnvPythonPath.
> 
> GetSysPythonPath is interfering with using vpython for sub-invocations
> (and I believe it was actually never necessary and was added due to a
> misunderstanding of how PYTHONPATH works).
> 
> PYTHONPATH is prefixed to the internal calculation of sys.path of
> whatever python interpreter starts up. The interpreter takes PYTHONPATH
> from the env, then adds in any libraries which are installed with that
> interpreter.
> 
> When you're using system python for both runit and also the subprocess,
> exporting sys.path to PYTHONPATH is redundant, but not harmful. e.g.
> runit would export:
> 
>   PYTHONPATH=/build.git:/sys_python
> 
> And then when the subpython starts up, it would calculate sys.path to be:
> 
>   sys.path=/build.git:/sys_python:/sys_python
> 
> However, when you're using vpython to invoke runit and the subprocess
> with two different virtualenvs, you get:
> 
>   PYTHONPATH=/build.git:/vpython/deadbeef
>   sys.path=/build.git:/vpython/deadbeef:/vpython/feedface
> 
> Where 'feedface' is the environment that your script actually wants!
> By removing GetSysPythonPath from the equation, we'll now do:
> 
>   PYTHONPATH=/build.git
>   sys.path=/build.git:/vpython/feedface
> 
> Which should actually work correctly. I believe that GetSysPythonPath
> was added under the assumption that python ONLY consults PYTHONPATH for
> libraries when PYTHONPATH is set in the environment, and so we needed
> to preserve the current sys.path entries for correct behavior.
> 
> GetEnvPythonPath seems to be entirely unused.
> 
> Original CL: https://chromium-review.googlesource.com/974759
> Fix for original revert: https://chromium-review.googlesource.com/976876
> 
> R=​dnj@chromium.org, nodir@chromium.org, vadimsh@chromium.org, yyanagisawa@chromium.org
> 
> Bug: 776430,814607, 822103 ,822042, 825290 
> Change-Id: I5eb0d32fd94ae4282f6877c894e6d00f01e810ec
> Reviewed-on: https://chromium-review.googlesource.com/976878
> Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
> Reviewed-by: Nodir Turakulov <nodir@chromium.org>

TBR=iannucci@chromium.org,vadimsh@chromium.org,yyanagisawa@chromium.org,dnj@chromium.org,nodir@chromium.org

Change-Id: I8e786ec50a5f4871391fa2bb658b2331816255ca
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 776430, 814607,  822103 , 822042,  825290 
Reviewed-on: https://chromium-review.googlesource.com/1037103
Reviewed-by: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>

[modify] https://crrev.com/da82356fbf21e140a6b1bc749f85b52adee3aeec/scripts/common/env.py
[modify] https://crrev.com/da82356fbf21e140a6b1bc749f85b52adee3aeec/scripts/common/unittests/env_test.py

Cc: d...@chromium.org
Landing code specified in #10 seems to cause https://ci.chromium.org/buildbot/chromium/Mac/41039.
Running ['/usr/bin/python', u'/b/rr/tmp2vnS88/rw/checkout/scripts/tools/runit.py', '--show-path', '/usr/bin/python', u'/b/rr/tmp2vnS88/rw/checkout/scripts/slave/runtest.py', '--target', 'Release', '--xvfb', '--builder-name', u'Mac', '--slave-name', u'vm682-m1', '--build-number', '41039', '--build-properties', '{"buildername": "Mac", "mastername": "chromium", "buildnumber": 41039, "slavename": "vm682-m1", "target_platform": "mac", "bot_id": "vm682-m1"}', '--test-type', 'sizes', '--run-python-script', '/b/c/b/mac/src/infra/scripts/legacy/scripts/slave/chromium/sizes.py', '--json', '/var/folders/2j/22s2gz0s7hn48k32d47clxf80000gm/T/tmpuiFCei'] in None (env: {'PYTHONIOENCODING': 'UTF-8', 'LOGDOG_STREAM_SERVER_PATH': 'unix:/b/build/rr/tmpkglRUQ/butler.sock', 'VERSIONER_PYTHON_PREFER_32_BIT': 'no', 'BUILDBOT_BUILDNUMBER': '41039', 'BUILDBOT_BUILDERNAME': 'Mac', 'LOGDOG_COORDINATOR_HOST': 'logs.chromium.org', 'LOGNAME': 'chrome-bot', 'USER': 'chrome-bot', 'PATH': '/b/cipd_client:/b/cipd_path_tools:/b/cipd_path_tools/bin:/b/cipd_path_tools:/b/cipd_path_tools/bin:/b/cipd_client:/Users/chrome-bot/slavebin:/b/depot_tools:/usr/local/git/bin:/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin', 'BOTO_CONFIG': '/b/build/site_config/.boto', 'HOME': '/Users/chrome-bot', 'PYTHONUNBUFFERED': '1', 'BUILDBOT_BUILDBOTURL': 'http://build.chromium.org/p/chromium/', 'BUILDBOT_BLAMELIST': "[u'sangwoo108@chromium.org']", 'SHELL': '/bin/bash', 'VERSIONER_PYTHON_VERSION': '2.7', 'BUILDBOT_MASTERNAME': 'chromium', 'LOGDOG_STREAM_PREFIX': 'bb/chromium/Mac/41039', 'CIPD_CACHE_DIR': '/b/c/cipd', 'BUILDBOT_GOT_REVISION': 'None', 'PYTHONPATH': '/b/build/site_config:/b/build/scripts:/b/build/scripts/release:/b/build/third_party:/b/build/third_party/google_api_python_client:/b/build/third_party/httplib2/python2:/b/build/third_party/infra_libs:/b/build/third_party/oauth2client:/b/build/third_party/pyasn1:/b/build/third_party/pyasn1-modules:/b/build/third_party/python-rsa:/b/build/third_party/requests_2_10_0:/b/build/third_party/setuptools-0.6c11:/b/build/third_party/site-packages:/b/build/third_party/uritemplate:/b/build_internal/site_config:/b/build_internal/symsrc:/b/build/slave:/b/build/third_party/buildbot_slave_8_4:/b/build/third_party/twisted_10_2:', 'SSH_AUTH_SOCK': '/private/tmp/com.apple.launchd.LHMW5WdNys/Listeners', 'VPYTHON_VIRTUALENV_ROOT': '/b/c/vpython', 'BUILDBOT_SLAVENAME': 'vm682-m1', 'INFRA_BUILDBOT_SLAVE_ACTIVE_SUBDIR': '', 'BUILDBOT_REVISION': '9d8d6c4a0c9c386bcbe6e03d958d91f6c45ad009', 'AWS_CREDENTIAL_FILE': '/b/build/site_config/.boto', 'FORCE_MAC_TOOLCHAIN': '1', 'CHROME_HEADLESS': '1', 'INFRA_BUILDBOT_MASTER_CLASS_NAME': 'Chromium', 'BUILDBOT_BRANCH': 'master', 'TMPDIR': '/var/folders/2j/22s2gz0s7hn48k32d47clxf80000gm/T/', 'GIT_USER_AGENT': 'git/2.7.4 darwin vm682-m1.golo.chromium.org', 'INFRA_BUILDBOT_SLAVE_NAME': 'vm682-m1', 'VPYTHON_CLEAR_PYTHONPATH': '1', '__CF_USER_TEXT_ENCODING': '0x1F4:0x0:0x0', 'PWD': '/b/build/slave/Mac/build', 'BUILDBOT_SCHEDULER': 'chromium', 'LOGDOG_STREAM_PROJECT': 'chromium', 'BUILDBOT_CLOBBER': '', 'PAGER': 'cat'})
Set PYTHONPATH: /b/rr/tmp2vnS88/rw/checkout/scripts:/b/rr/tmp2vnS88/rw/checkout/site_config
Traceback (most recent call last):
  File "/b/rr/tmp2vnS88/rw/checkout/scripts/slave/runtest.py", line 51, in <module>
    from slave import performance_log_processor
  File "/b/rr/tmp2vnS88/rw/checkout/scripts/slave/performance_log_processor.py", line 27, in <module>
    import config
  File "/b/rr/tmp2vnS88/rw/checkout/site_config/config.py", line 9, in <module>
    from twisted.spread import banana
ImportError: No module named twisted.spread

If I understand the code literary, it is natural for https://chromium-review.googlesource.com/c/chromium/tools/build/+/976878 to cause this.
Also, my workaround https://chromium-review.googlesource.com/c/chromium/tools/build/+/1039223 should not fix this because runit.py is not executed with --with-third-party flag.

I cannot understand how sizes.py execute the command but I guess it something similar to:
https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave/recipe_modules/chromium_tests/api.py#827

command to invoke runit is silently stored to JSON file to be executed by third party script.  Since runit does not have --with-third-party, #10 cl broke mac builder.
As far as I understand, LUCI uses vpython but since LUCI builders won't execute the steps I cannot say vpython fixes this or not.  I guess no because I could not find .vpython to load twisted library.
https://luci-milo.appspot.com/p/chromium/builders/luci.chromium.ci/Mac%20Builder/86115
https://luci-milo.appspot.com/p/chromium/builders/luci.chromium.ci/Linux%20Builder/102203

Also, like Mac builder Linux x64 also fails like this:
https://luci-milo.appspot.com/buildbot/chromium/Linux%20x64/63461

I guess possible workaround is adding --with-third-party to runit execution in testing.
Since slave/performance_log_processor.py depends on config in site_config.
https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave/performance_log_processor.py#27
If we use https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave/runtest.py, we need to set --with-third-party-lib to execute runit.py.

Comment 16 by d...@chromium.org, May 10 2018

> I could not find .vpython to load twisted library

I don't understand what you're saying here. If you need vpython to load "twistd", you can add it to the vpython spec file.

> I guess possible workaround is adding --with-third-party to runit execution in testing.

It looks like you actually added "--with-third-party-libs"? https://chromium.googlesource.com/chromium/tools/build/+/b72a9929093ff82c0f0f4961567ec123e544899e

I was reading up on crbug.com/807193, which inspired that. As far as I understand, "runit.py" is supplementing vpython's environment using PYTHONPATH, which includes (a) the local "tools/build" config files, and (b) the crap in "third_party". Our stance seems to be that (b) is bad, and that vpython ought to be the thing supplying that.

However, (a) is necessary and cannot be supplied by "vpython", since it's a local configuration. This is why you use "runit.py".

The piece that is missing is "twistd", which you previously got from (b). So I think the solution here is to use "vpython" and add the "twistd" wheel (https://pypi.org/project/Twisted/#history) to the infra "vpython" spec (https://chromium.googlesource.com/chromium/tools/build/+/master/.vpython).

Does this seem reasonable to you?
Project Member

Comment 17 by bugdroid1@chromium.org, May 11 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b4fce98dc7e5340f02009b9d171726a6bcb6440

commit 7b4fce98dc7e5340f02009b9d171726a6bcb6440
Author: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Date: Fri May 11 04:18:43 2018

Import build/third_party when executing runtest.py.

Since runtest.py depends on modules listed in build/third_party,
we need to set --with-third-party-lib when we execute runtest.py
via runit.py.

Bug:  825290 
Change-Id: Ic780388a9b9086364c1e4dcbd4a70a2cd2ed2425
Reviewed-on: https://chromium-review.googlesource.com/1053674
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557794}
[modify] https://crrev.com/7b4fce98dc7e5340f02009b9d171726a6bcb6440/testing/scripts/common.py

Comment 18 by d...@chromium.org, May 11 2018

Owner: yyanagisawa@chromium.org
Since #17 has landed, could you please un-revert: https://chromium.googlesource.com/chromium/tools/build/+/da82356fbf21e140a6b1bc749f85b52adee3aeec

The reason this is important is because that CL acts as a regression safeguard, ensuring that other changes and new scripts don't regress by manipulating their dependencies in a non-forward-supported manner.
Re: #18
FYI, I have already sent you cl.
https://chromium-review.googlesource.com/c/chromium/tools/build/+/1054909
Project Member

Comment 20 by bugdroid1@chromium.org, May 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/677b29788aedb625fdbfd5b75cd09cbbc406fb72

commit 677b29788aedb625fdbfd5b75cd09cbbc406fb72
Author: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Date: Mon May 14 02:33:23 2018

[common/env.py] Reland: Remove GetSysPythonPath and GetEnvPythonPath."

This reverts commit da82356fbf21e140a6b1bc749f85b52adee3aeec.

Reason for reland:
The fix has been landed:
https://chromium-review.googlesource.com/c/chromium/src/+/1053674

Original change's description:
> Revert "[common/env.py] Reland: Remove GetSysPythonPath and GetEnvPythonPath."
> 
> This reverts commit c55d8676ce4193462972ae3f0d17826ea7b0978f.
> 
> Reason for revert: 
> This cl might have broken following builder.
> https://ci.chromium.org/buildbot/chromium/Mac/41039
> 
> Original change's description:
> > [common/env.py] Reland: Remove GetSysPythonPath and GetEnvPythonPath.
> > 
> > GetSysPythonPath is interfering with using vpython for sub-invocations
> > (and I believe it was actually never necessary and was added due to a
> > misunderstanding of how PYTHONPATH works).
> > 
> > PYTHONPATH is prefixed to the internal calculation of sys.path of
> > whatever python interpreter starts up. The interpreter takes PYTHONPATH
> > from the env, then adds in any libraries which are installed with that
> > interpreter.
> > 
> > When you're using system python for both runit and also the subprocess,
> > exporting sys.path to PYTHONPATH is redundant, but not harmful. e.g.
> > runit would export:
> > 
> >   PYTHONPATH=/build.git:/sys_python
> > 
> > And then when the subpython starts up, it would calculate sys.path to be:
> > 
> >   sys.path=/build.git:/sys_python:/sys_python
> > 
> > However, when you're using vpython to invoke runit and the subprocess
> > with two different virtualenvs, you get:
> > 
> >   PYTHONPATH=/build.git:/vpython/deadbeef
> >   sys.path=/build.git:/vpython/deadbeef:/vpython/feedface
> > 
> > Where 'feedface' is the environment that your script actually wants!
> > By removing GetSysPythonPath from the equation, we'll now do:
> > 
> >   PYTHONPATH=/build.git
> >   sys.path=/build.git:/vpython/feedface
> > 
> > Which should actually work correctly. I believe that GetSysPythonPath
> > was added under the assumption that python ONLY consults PYTHONPATH for
> > libraries when PYTHONPATH is set in the environment, and so we needed
> > to preserve the current sys.path entries for correct behavior.
> > 
> > GetEnvPythonPath seems to be entirely unused.
> > 
> > Original CL: https://chromium-review.googlesource.com/974759
> > Fix for original revert: https://chromium-review.googlesource.com/976876
> > 
> > R=​dnj@chromium.org, nodir@chromium.org, vadimsh@chromium.org, yyanagisawa@chromium.org
> > 
> > Bug: 776430,814607, 822103 ,822042, 825290 
> > Change-Id: I5eb0d32fd94ae4282f6877c894e6d00f01e810ec
> > Reviewed-on: https://chromium-review.googlesource.com/976878
> > Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
> > Reviewed-by: Nodir Turakulov <nodir@chromium.org>
> 
> TBR=iannucci@chromium.org,vadimsh@chromium.org,yyanagisawa@chromium.org,dnj@chromium.org,nodir@chromium.org
> 
> Change-Id: I8e786ec50a5f4871391fa2bb658b2331816255ca
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 776430, 814607,  822103 , 822042,  825290 
> Reviewed-on: https://chromium-review.googlesource.com/1037103
> Reviewed-by: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
> Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>

TBR=iannucci@chromium.org,vadimsh@chromium.org,yyanagisawa@chromium.org,dnj@chromium.org,nodir@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 776430, 814607,  822103 , 822042,  825290 
Change-Id: I2035a01125452fbb63fbb93f8fe01a9415ac52e0
Reviewed-on: https://chromium-review.googlesource.com/1054909
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>

[modify] https://crrev.com/677b29788aedb625fdbfd5b75cd09cbbc406fb72/scripts/common/env.py
[modify] https://crrev.com/677b29788aedb625fdbfd5b75cd09cbbc406fb72/scripts/common/unittests/env_test.py

The builder that caused #11 seems to work now:
https://ci.chromium.org/buildbot/chromium/Mac/
Project Member

Comment 22 by bugdroid1@chromium.org, May 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/c802f8211a5018cfd2a6913e1023c51c76c870dd

commit c802f8211a5018cfd2a6913e1023c51c76c870dd
Author: Michael Moss <mmoss@chromium.org>
Date: Tue May 15 22:50:08 2018

Revert "[common/env.py] Reland: Remove GetSysPythonPath and GetEnvPythonPath.""

This reverts commit 677b29788aedb625fdbfd5b75cd09cbbc406fb72.

Reason for revert: Breaking official release builds. See discussion in  crbug.com/843012.

Original change's description:
> [common/env.py] Reland: Remove GetSysPythonPath and GetEnvPythonPath."
> 
> This reverts commit da82356fbf21e140a6b1bc749f85b52adee3aeec.
> 
> Reason for reland:
> The fix has been landed:
> https://chromium-review.googlesource.com/c/chromium/src/+/1053674
> 
> Original change's description:
> > Revert "[common/env.py] Reland: Remove GetSysPythonPath and GetEnvPythonPath."
> > 
> > This reverts commit c55d8676ce4193462972ae3f0d17826ea7b0978f.
> > 
> > Reason for revert: 
> > This cl might have broken following builder.
> > https://ci.chromium.org/buildbot/chromium/Mac/41039
> > 
> > Original change's description:
> > > [common/env.py] Reland: Remove GetSysPythonPath and GetEnvPythonPath.
> > > 
> > > GetSysPythonPath is interfering with using vpython for sub-invocations
> > > (and I believe it was actually never necessary and was added due to a
> > > misunderstanding of how PYTHONPATH works).
> > > 
> > > PYTHONPATH is prefixed to the internal calculation of sys.path of
> > > whatever python interpreter starts up. The interpreter takes PYTHONPATH
> > > from the env, then adds in any libraries which are installed with that
> > > interpreter.
> > > 
> > > When you're using system python for both runit and also the subprocess,
> > > exporting sys.path to PYTHONPATH is redundant, but not harmful. e.g.
> > > runit would export:
> > > 
> > >   PYTHONPATH=/build.git:/sys_python
> > > 
> > > And then when the subpython starts up, it would calculate sys.path to be:
> > > 
> > >   sys.path=/build.git:/sys_python:/sys_python
> > > 
> > > However, when you're using vpython to invoke runit and the subprocess
> > > with two different virtualenvs, you get:
> > > 
> > >   PYTHONPATH=/build.git:/vpython/deadbeef
> > >   sys.path=/build.git:/vpython/deadbeef:/vpython/feedface
> > > 
> > > Where 'feedface' is the environment that your script actually wants!
> > > By removing GetSysPythonPath from the equation, we'll now do:
> > > 
> > >   PYTHONPATH=/build.git
> > >   sys.path=/build.git:/vpython/feedface
> > > 
> > > Which should actually work correctly. I believe that GetSysPythonPath
> > > was added under the assumption that python ONLY consults PYTHONPATH for
> > > libraries when PYTHONPATH is set in the environment, and so we needed
> > > to preserve the current sys.path entries for correct behavior.
> > > 
> > > GetEnvPythonPath seems to be entirely unused.
> > > 
> > > Original CL: https://chromium-review.googlesource.com/974759
> > > Fix for original revert: https://chromium-review.googlesource.com/976876
> > > 
> > > R=​dnj@chromium.org, nodir@chromium.org, vadimsh@chromium.org, yyanagisawa@chromium.org
> > > 
> > > Bug: 776430,814607, 822103 ,822042, 825290 
> > > Change-Id: I5eb0d32fd94ae4282f6877c894e6d00f01e810ec
> > > Reviewed-on: https://chromium-review.googlesource.com/976878
> > > Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
> > > Reviewed-by: Nodir Turakulov <nodir@chromium.org>
> > 
> > TBR=iannucci@chromium.org,vadimsh@chromium.org,yyanagisawa@chromium.org,dnj@chromium.org,nodir@chromium.org
> > 
> > Change-Id: I8e786ec50a5f4871391fa2bb658b2331816255ca
> > No-Presubmit: true
> > No-Tree-Checks: true
> > No-Try: true
> > Bug: 776430, 814607,  822103 , 822042,  825290 
> > Reviewed-on: https://chromium-review.googlesource.com/1037103
> > Reviewed-by: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
> > Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
> 
> TBR=iannucci@chromium.org,vadimsh@chromium.org,yyanagisawa@chromium.org,dnj@chromium.org,nodir@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug: 776430, 814607,  822103 , 822042,  825290 
> Change-Id: I2035a01125452fbb63fbb93f8fe01a9415ac52e0
> Reviewed-on: https://chromium-review.googlesource.com/1054909
> Reviewed-by: Nodir Turakulov <nodir@chromium.org>
> Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>

TBR=iannucci@chromium.org,vadimsh@chromium.org,yyanagisawa@chromium.org,dnj@chromium.org,nodir@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 776430, 814607,  822103 , 822042,  825290 , 843012
Change-Id: I4ccffdc3f1fe9fa878ef78a0212c8b39d91c0189
Reviewed-on: https://chromium-review.googlesource.com/1060394
Reviewed-by: Michael Moss <mmoss@chromium.org>
Commit-Queue: Michael Moss <mmoss@chromium.org>

[modify] https://crrev.com/c802f8211a5018cfd2a6913e1023c51c76c870dd/scripts/common/env.py
[modify] https://crrev.com/c802f8211a5018cfd2a6913e1023c51c76c870dd/scripts/common/unittests/env_test.py

Project Member

Comment 23 by bugdroid1@chromium.org, May 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/ab63c10029dc010d0178057775b4817b7dbf01b3

commit ab63c10029dc010d0178057775b4817b7dbf01b3
Author: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Date: Tue May 22 01:07:26 2018

[common/env.py] Remove GetSysPythonPath.

This reverts commit c802f8211a5018cfd2a6913e1023c51c76c870dd.
Since PYTHONPATH need to be loaded for official builder,
let me remove GetSysPythonPath by default but allow to add
PYTHONPATH if with_third_party_lib is set.

Bug: 776430, 814607,  822103 , 822042,  825290 , 843012, 843012
Change-Id: Idc7306ce33e083c235f6e3c7f3a7d2e1d5d618b3
Reviewed-on: https://chromium-review.googlesource.com/1067224
Reviewed-by: Michael Moss <mmoss@chromium.org>
Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>

[modify] https://crrev.com/ab63c10029dc010d0178057775b4817b7dbf01b3/scripts/common/env.py
[modify] https://crrev.com/ab63c10029dc010d0178057775b4817b7dbf01b3/scripts/common/unittests/env_test.py

I will mark this fixed when it is not reverted this week.
Status: Fixed (was: Started)

Sign in to add a comment