In this case, I think the concern was that if PYTHONPATH is passed on to scripts, then that may conceal the fact that the script is depending on some library in another place, when instead it should depend on a library in a third_party directory in the same repo.

In general, it looks like the recipe step runner takes the os.environ that the recipe is run under, and values can be overridden for particular recipes or steps (https://cs.chromium.org/chromium/infra/recipes-py/recipe_engine/step_runner.py?l=187).


Robbie, do you think it would make sense to "sanitize" the environment used in steps in general, e.g. copy a particular limited set of values from os.environ?

Or, if not, perhaps for python steps in particular (https://cs.chromium.org/chromium/infra/recipes-py/recipe_modules/python/api.py), it would make sense to clear or set PYTHONPATH?

Just talked to Dan, and it sounds like using vpython might be a good idea here -- in this case, if we run ScriptTest steps through vpython, then the PYTHONPATH in environment when running the recipe wouldn't matter; the set of python packages available would instead depend on the virtual environment configured somewhere else (e.g. in individual scripts, or some default "vanilla environment").

Example possible CL to try this out: https://chromium-review.googlesource.com/c/508124/

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

commit d937b4db084352c22aafd859149b4e72c015477f
Author: Quinten Yearsley <qyearsley@google.com>
Date: Thu Jun 01 22:34:22 2017

In chromium_tests steps.ScriptTest, add venv=True.

Bug: 670990
Change-Id: I96ca32651efb3af713b02160129df9e85510b099
Reviewed-on: https://chromium-review.googlesource.com/508124
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>

[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipes/android/perf.expected/src_side_script_fails.json
[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipe_modules/chromium_tests/tests/api/trybot_steps.expected/blink_linux.json
[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipes/chromium.expected/dynamic_script_test_with_args.json
[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipe_modules/chromium_tests/tests/steps/script_test.expected/basic.json
[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipes/chromium_trybot.expected/add_swarming_layout_tests_via_manual_diff_inspection_that_fails.json
[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipes/findit/chromium/test.expected/compile_skipped.json
[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipe_modules/chromium_tests/tests/steps/generate_script.expected/basic.json
[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipes/chromium_trybot.expected/add_swarming_layout_tests_via_manual_diff_inspection.json
[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipes/findit/chromium/test.expected/test_without_targets_not_skipped.json
[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipes/chromium_trybot.expected/add_layout_tests_via_manual_diff_inspection.json
[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipes/chromium.expected/dynamic_script_test_failure.json
[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipe_modules/chromium_tests/steps.py
[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipes/chromium_trybot.expected/use_v8_patch_on_chromium_trybot.json
[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipe_modules/chromium_tests/tests/api/trybot_steps.expected/blink_mac.json
[modify] https://crrev.com/d937b4db084352c22aafd859149b4e72c015477f/scripts/slave/recipes/chromium_trybot.expected/use_skia_patch_on_chromium_trybot.json

In theory the above change resolves this issue for scripts invoked in chromium_tests steps.ScriptTest -- but not for all scripts invoked in any recipe.

In general I think the solution is probably to incrementally start using vpython in more places.

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

commit b0712e5e20f84c1aecb5f352b09b030659ac31d4
Author: Peter Kasting <pkasting@chromium.org>
Date: Fri Jun 02 00:02:45 2017

Revert "In chromium_tests steps.ScriptTest, add venv=True."

This reverts commit d937b4db084352c22aafd859149b4e72c015477f.

Reason for revert: Possible cause of failures on Win 7 bots.  Sample failing output: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.win%2FWin7__32__Tests%2F20706%2F%2B%2Frecipes%2Fsteps%2Fnacl_integration%2F0%2Fstdout

From dnj via IM: "[Quinten] landed a change to bootstrap tools through an isolated VirtualEnv (see "vpython" at top instead of "python").
Failure to import would suggest that the VirtualEnv doesn't have the right package to me.  I think that's the likely cuplrit."

Original change's description:
> In chromium_tests steps.ScriptTest, add venv=True.
> 
> Bug: 670990
> Change-Id: I96ca32651efb3af713b02160129df9e85510b099
> Reviewed-on: https://chromium-review.googlesource.com/508124
> Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
> Reviewed-by: Dirk Pranke <dpranke@chromium.org>
> 

TBR=qyearsley@chromium.org,dpranke@chromium.org
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 670990

Change-Id: Ice4fc8aed7dac7c6f79cc02fba94d66535ef78c3
Reviewed-on: https://chromium-review.googlesource.com/521909
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>

[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipes/android/perf.expected/src_side_script_fails.json
[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipe_modules/chromium_tests/tests/api/trybot_steps.expected/blink_linux.json
[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipes/chromium.expected/dynamic_script_test_with_args.json
[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipe_modules/chromium_tests/tests/steps/script_test.expected/basic.json
[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipes/chromium_trybot.expected/add_swarming_layout_tests_via_manual_diff_inspection_that_fails.json
[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipes/findit/chromium/test.expected/compile_skipped.json
[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipe_modules/chromium_tests/tests/steps/generate_script.expected/basic.json
[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipes/chromium_trybot.expected/add_swarming_layout_tests_via_manual_diff_inspection.json
[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipes/findit/chromium/test.expected/test_without_targets_not_skipped.json
[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipes/chromium_trybot.expected/add_layout_tests_via_manual_diff_inspection.json
[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipes/chromium.expected/dynamic_script_test_failure.json
[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipe_modules/chromium_tests/steps.py
[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipes/chromium_trybot.expected/use_v8_patch_on_chromium_trybot.json
[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipe_modules/chromium_tests/tests/api/trybot_steps.expected/blink_mac.json
[modify] https://crrev.com/b0712e5e20f84c1aecb5f352b09b030659ac31d4/scripts/slave/recipes/chromium_trybot.expected/use_skia_patch_on_chromium_trybot.json

nacl_integration failed after landing that change; so before trying to reland that, it will be necessary to make sure that that script works with a vanilla vpython environment.

The failure was to import "win32api". Unfortunately, a VirtualEnv will not have that, since it is not part of vanilla Python. Chromium's Python has it b/c we hack it in there, which is ... /sad.

Anyway, solution might be to just create a common "default" VirtualEnv instead of vanilla one that we were using, and have it include the win32api wheel. I can work with you to get this implemented tomorrow!

Other things that were likely fallout of this were webkit_lint and webkit_python_tests failing due to failing to import "win32pipe".

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

commit 0054986e1fe971357c320ed5bf25b14236eebec3
Author: Quinten Yearsley <qyearsley@google.com>
Date: Thu Jun 15 21:33:31 2017

Add an initial vpython environment config file.

Bug: 670990
Change-Id: I89f618d559b19db5b60f383dd0a7f8170931a858
Reviewed-on: https://chromium-review.googlesource.com/527661
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Daniel Jacques <dnj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#479842}
[add] https://crrev.com/0054986e1fe971357c320ed5bf25b14236eebec3/third_party/WebKit/Tools/Scripts/common.vpython

run-webkit-tests doesn't work probably due to c#10. 
The following is the error when I tried to run a test.

==
shimazu@shimazu> ./third_party/WebKit/Tools/Scripts/run-webkit-tests   -t $(branch)_release  'external/wpt/service-workers/service-worker/fetch-request-resources.https.html' --no-retry-failures  --iterations=1
[E2017-06-19T15:53:47.946061+09:00 101233 0 annotate.go:343] original error: package "infra/python/wheels/psutil/linux-amd64_cp27_cp27m" is not registered
[E2017-06-19T15:53:47.946115+09:00 101233 0 annotate.go:343] 
[E2017-06-19T15:53:47.946126+09:00 101233 0 annotate.go:343] goroutine 1:
[E2017-06-19T15:53:47.946135+09:00 101233 0 annotate.go:343] #0 github.com/luci/luci-go/vpython/cipd/cipd.go:132 - cipd.(*PackageLoader).resolveWithOpts.func1()
[E2017-06-19T15:53:47.946145+09:00 101233 0 annotate.go:343]   reason: "failed to resolve package \"infra/python/wheels/psutil/linux-amd64_cp27_cp27m\" at version \"version:5.2.2\""
[E2017-06-19T15:53:47.946156+09:00 101233 0 annotate.go:343]   "package" = "infra/python/wheels/psutil/linux-amd64_cp27_cp27m"
[E2017-06-19T15:53:47.946167+09:00 101233 0 annotate.go:343]   "version" = "version:5.2.2"
[E2017-06-19T15:53:47.946177+09:00 101233 0 annotate.go:343] 
[E2017-06-19T15:53:47.946186+09:00 101233 0 annotate.go:343] #1 github.com/luci/luci-go/cipd/client/cipd/ensure/package_def.go:57 - ensure.(*PackageDef).Resolve()
[E2017-06-19T15:53:47.946195+09:00 101233 0 annotate.go:343]   reason: "failed to resolve package version (line 0)"
[E2017-06-19T15:53:47.946203+09:00 101233 0 annotate.go:343]   "line" = 0
[E2017-06-19T15:53:47.946211+09:00 101233 0 annotate.go:343] 
[E2017-06-19T15:53:47.946222+09:00 101233 0 annotate.go:343] #2 github.com/luci/luci-go/cipd/client/cipd/ensure/file.go:162 - ensure.(*File).ResolveWith()
[E2017-06-19T15:53:47.946231+09:00 101233 0 annotate.go:343]   reason: "resolving package"
[E2017-06-19T15:53:47.946240+09:00 101233 0 annotate.go:343] 
[E2017-06-19T15:53:47.946247+09:00 101233 0 annotate.go:343] #3 github.com/luci/luci-go/vpython/cipd/cipd.go:140 - cipd.(*PackageLoader).resolveWithOpts()
[E2017-06-19T15:53:47.946257+09:00 101233 0 annotate.go:343] #4 github.com/luci/luci-go/vpython/cipd/cipd.go:94 - cipd.(*PackageLoader).Resolve.func2()
[E2017-06-19T15:53:47.946267+09:00 101233 0 annotate.go:343] #5 github.com/luci/luci-go/common/system/filesystem/tempdir.go:45 - filesystem.(*TempDir).With()
[E2017-06-19T15:53:47.946278+09:00 101233 0 annotate.go:343] #6 github.com/luci/luci-go/vpython/cipd/cipd.go:95 - cipd.(*PackageLoader).Resolve()
[E2017-06-19T15:53:47.946287+09:00 101233 0 annotate.go:343] #7 github.com/luci/luci-go/vpython/venv/config.go:175 - venv.(*Config).makeEnv()
[E2017-06-19T15:53:47.946296+09:00 101233 0 annotate.go:343]   reason: "failed to resolve packages"
[E2017-06-19T15:53:47.946304+09:00 101233 0 annotate.go:343] 
[E2017-06-19T15:53:47.946314+09:00 101233 0 annotate.go:343] #8 github.com/luci/luci-go/vpython/venv/venv.go:127 - venv.With()
[E2017-06-19T15:53:47.946326+09:00 101233 0 annotate.go:343] #9 github.com/luci/luci-go/vpython/run.go:105 - vpython.Run()
[E2017-06-19T15:53:47.946340+09:00 101233 0 annotate.go:343] #10 github.com/luci/luci-go/vpython/application/application.go:242 - application.(*application).mainImpl()
[E2017-06-19T15:53:47.946351+09:00 101233 0 annotate.go:343] #11 github.com/luci/luci-go/vpython/application/application.go:319 - application.(*Config).Main.func1()
[E2017-06-19T15:53:47.946361+09:00 101233 0 annotate.go:343] #12 github.com/luci/luci-go/vpython/application/support.go:26 - application.run()
[E2017-06-19T15:53:47.946371+09:00 101233 0 annotate.go:343] #13 github.com/luci/luci-go/vpython/application/application.go:320 - application.(*Config).Main()
[E2017-06-19T15:53:47.946381+09:00 101233 0 annotate.go:343] #14 vpython/main.go:71 - main.mainImpl()
[E2017-06-19T15:53:47.946390+09:00 101233 0 annotate.go:343] #15 vpython/main.go:77 - main.main()
[E2017-06-19T15:53:47.946399+09:00 101233 0 annotate.go:343] #16 runtime/proc.go:185 - runtime.main()
[E2017-06-19T15:53:47.946407+09:00 101233 0 annotate.go:343] #17 runtime/asm_amd64.s:2197 - runtime.goexit()

That's very interesting. It looks like you have a Python compiled with "cp27m", while standard Linux is "cp27mu". Can you provide any details about the Python version that you're using, and the OS that you're running it on? Is it custom-built?

Can you run the following in your Python interpreter and paste the output:

$ python -c 'import pip.pep425tags; print pip.pep425tags.get_supported()'

We tailored the current wheel set to Ubuntu's stock Python, which (observationally) seems to be "cp27mu" for 64-bit Linux.

Thanks for your help!

I didn't remember why, but I'm using my custom-build Python.
OS is ubuntu (customized for Google), and the version is Python 2.7.13. 

The following is the result of that.
==
[('cp27', 'cp27m', 'manylinux1_x86_64'), ('cp27', 'cp27m', 'linux_x86_64'), ('cp27', 'none', 'manylinux1_x86_64'), ('cp27', 'none', 'linux_x86_64'), ('py2', 'none', 'manylinux1_x86_64'), ('py2', 'none', 'linux_x86_64'), ('cp27', 'none', 'any'), ('cp2', 'none', 'any'), ('py27', 'none', 'any'), ('py2', 'none', 'any'), ('py26', 'none', 'any'), ('py25', 'none', 'any'), ('py24', 'none', 'any'), ('py23', 'none', 'any'), ('py22', 'none', 'any'), ('py21', 'none', 'any'), ('py20', 'none', 'any')]
==

I also found the build command which I used several months ago from the history. It was very simple.
==
$ ./configure --prefix=$HOME/local --enable-ipv6
$ make && make install

Ah, that makes sense. This is a pretty difficult situation to handle on our part, since we want to support standard Chromium development, but there are countless opportunities for people to build one-off versions of Python and other core tools, and it's impractical for us to try and support all of them.

Standard Linux distros all use the "--enable-unicode=ucs4" configure flag, so that's what we planned on. While default ("--enable-unicode=ucs2") is not invalid, it was not standard enough for us to try and support.

There are two paths here:
1) Add support for ALL plausible developer platforms.
2) Bundle a Chromium version of Python and require developers to use it.

We're actually working on (2), but it's not trivial and will require some testing and integration before we're comfortable mandating it. On the other hand, (1) is impractical for obvious reasons.

In the short term, would you mind either using a standard Python version or rebuilding yours with the "ucs4" flag mentioned above? I'm going to evaluate the feasibility of supporting "ucs2" Linux Pythons - probably not too hard - but reverting / this change for a one-off Python environment seems like the wrong thing to do.

If you're OK with this, please re-revert the patch :)

Got it, I'll re-revert the patch.
Thanks!

Thanks! Don't hesitate to contact me or re-re-revert if you experience another problem. This is one of the first widespread deployments of the "vpython" tool, so I'd love to whack down any problems that may pop up.

I think the next step here is to make sure that vpython is used on the bots for any python scripts.

This is tricky, but agreed. iannucci@ and I are working towards this independently, and I suspect we'll all meet in the middle.

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

dnj@ iannucci@ what is the status of this? Do we have vpython on all the bots now? (Both "PYTHONPATH" and "vpython" appears in the source, so current status isn't transparent to me.)