I'm against (3) because it adds more moving parts to the process, namely "delayed" installation of packages by 'individual users'. It happens at unpredictable time, in a process that is not as monitored as puppet runs. It is also more complex, since there are possibilities of races now (if multiple processes attempt to use cold venv at once).

Doing this kind of stuff on the scale of "all our bots" is very scary, IMHO. And I don't think it buys us anything at all in return: in the end state (2) and (3) functionality are identical.

(4) use go

This is not realistic short-term. And people stubbornly continue to adding more python stuff there anyway :-/ The latest offender seem to be Dan himself :) https://chromium.googlesource.com/infra/infra/+/f3dce438f3a43fbdfbf84ba49bb785279a090b8e

As long as this stuff needs to run on non-developer machines, we need some way to package it (because using "gclient sync" in non-developer environment is madness and dark ages we shouldn't be returning to...)

I've just realized that if infra_python breaks, then infra_internal/cq package also breaks, since it uses similar mechanism (https://chrome-internal.googlesource.com/infra/infra_internal/+/master/build/packages/cq.yaml)

In particular, beware that CQ package built from HEAD is used as canary. And canary CQ handles infra repos, so there's a chance we may break infra CQ.

And that reminded me that infra_python for Windows is probably already in borked state, since it includes this wonderful hack: https://chromium.googlesource.com/infra/infra/+/master/bootstrap/bootstrap.py#226

So, breaking infra CQ through auto-built canary is WAI (aka Broken by design) :)
I don't fully follow: shouldn't a newly built *bad* CIPD CQ package fail in immediately run tests?

It won't, since it will hardcode paths to local python, which are valid for the bot where it was built (and where tests run), but may not be valid elsewhere.

(I'll try to confirm this to remove "(probably)" from the bug title...)

RE #3, "dockerbuild" is just a collection of user scripts, which is the sort of thing I think Python's actually good for. The concern here is mostly Python system scripts.

I think (3) ("vpython") should be left on the table. It offers a consistent way of managing VirtualEnvs, and can be used by both systems and users.

> because it adds more moving parts to the process, namely "delayed" installation of packages by 'individual users'.

If we know the users in advance, we could easily have Puppet seed the VirtualEnv for the user by running the "install" subcommand as that user during setup. "vpython" is also safe for concurrent use, so while worst-case there would be a few-second delay on initial VirtualEnv setup, it won't get worse than that.

The offline issue, as mentioned above, can be addressed by seeding a CIPD cache as part of the installation.

> It is also more complex, since there are possibilities of races now (if multiple processes attempt to use cold venv at once).

"vpython" uses locking to prevent races, so if this happens, one will create the VirtualEnv and the rest will immediately jump into it once it's finished.

If "vpython" is not our solution to Python / VirtualEnv, we should probably figure that out sooner rather than later. ATM I think it's up to the task here, and will make bundling and deployment safer and more ubiquitous (e.g., no need to split by distribution anymore). If there's a specific weakness, I'm also very open to modifying the tool to address it.

If you are willing to drive it to the completion (including puppet parts), I don't mind using vpython (as long as _all_ installation operations happen within Puppet agent run, and not at some arbitrary random process later).

Note that switching to vpython also implies converting infra/ENV to use vpython too, including converting all infra's own python code to be properly packaged wheels.

This is epic yak shave.

I believe vendoring python interpreter into existing infra_python crap-package will require much less work. 

(Also if we start doing that, we can also remove per-OS-flavor variants too. They exist only because our OSes have different version of python interpreters installed system-wide).

Yeah understood that it's a fairly big task. We can stop short of using wheels for *everything* b/c "run.py" and "test.py" can still make post-VirtualEnv path modifications.

Also agree that having Puppet install a bundled Python interpreter on systems and building ENV from there is a lower-risk and simpler short-term solution. I just hate the infra bootstrap ... a lot ... :(

Yep, it's definitely broken.

I've checked OSX one. All stdlib files are symlinks to /b/cipd_path_tools

$ readlink ./.cipd/pkgs/0/_current/ENV/lib/python2.7/re.py
/b/cipd_path_tools/lib/python2.7/re.py

(It used to by symlinks to system-level python libraries).

And orig_prefix points to it too:

$ cat ./.cipd/pkgs/0/_current/ENV/lib/python2.7/orig-prefix.txt
/b/cipd_path_tools

---

We should either fix it soon or revert the change :( It blocks deployment of infra.git python code.

We should definitely do something before rolling out hermetic python to internal.infra builders, it will break CQ canary.

Chatted in person. Plan is:

Short-term, update "build.py" to create a separate "infra" checkout, setup PATH to use system Python, and build against that.

Medium-term use "vpython", root-owned. Will need to implement "read-only mode" so non-root users can use it without needing/expecting to lock, and solve the problem of pruning old environments.

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/infra/puppet/+/6a059765eadcb696521a33c51243a6368fe4dc94

commit 6a059765eadcb696521a33c51243a6368fe4dc94
Author: Dan Jacques <dnj@chromium.org>
Date: Fri Jul 21 19:26:33 2017

Is this still blocking new infra-python deployments? I'd like to roll it forward sometime soon.

No, the bundled Python CL was reverted for this.

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/e725824e178839115a44c19f8a2b89f5c5218a06

commit e725824e178839115a44c19f8a2b89f5c5218a06
Author: Dan Jacques <dnj@google.com>
Date: Tue Aug 01 18:49:38 2017

[infra_python] Use bundled system Python.

Update the "infra_python" build instructions to use the system-bundled
Python interpreter. This is deployed by Puppet on all build systems.

Currently, "infra_python" CIPD package bakes in the current "infra" ENV.
This includes the prefix of the Python interpreter that was used to
create the environment. This can be problematic when the Python
interpreter that builds the bundle is not present on the systems on
which the bundle is deployed.

Currently, this is mitigated by appending the system's distribution to
the end of the CIPD package name with the expectation that a package
built on a specific distribution will, in turn, have its Python paths
align with the Python on that distribution. This falls apart when the
package is built with non-distribution (e.g., bundled) Python.

To mitigate this, we have deployed a "system Python" CIPD bundle to all
hosts at a fixed path. The "infra_python" bundle will now create a
separate infra VirtualEnv, built against the fixed-path system CIPD bundle,
and include *that* instead of the currently-running Infra VirtualEnv.

Since the system Python bundle is deployed on both builders and
"infra_python" hosts, which will share the same system CIPD bundle
pathing, this is expected to align.

As a plus, because we're using non-native bundles, we can drop the operating
system distribution (and its detection logic) from the "infra_python"
package name!

BUG= chromium:746995 
TEST=local

Change-Id: I053e94254c77e84c749bc53164dd7cb7519f39f1
Reviewed-on: https://chromium-review.googlesource.com/582627
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Commit-Queue: Daniel Jacques <dnj@chromium.org>

[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/build/README.md
[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipes/infra_continuous.expected/infra.json
[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipes/infra_continuous.expected/infra-go-deps-bundle.json
[add] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipe_modules/infra_system/examples/full.expected/mac.json
[add] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipe_modules/infra_system/examples/full.expected/linux.json
[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipes/infra_continuous.expected/infra_swarming.json
[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/build/build.py
[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/build/packages/infra_python.yaml
[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipes/infra_repo_trybot.expected/only_cipd_build.json
[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipes/infra_repo_trybot.expected/only_DEPS.json
[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipes/infra_repo_trybot.py
[add] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipe_modules/infra_system/examples/full.py
[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/README.recipes.md
[add] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipe_modules/infra_system/__init__.py
[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipes/infra_continuous.expected/infra_internal.json
[add] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipe_modules/infra_system/api.py
[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipes/infra_continuous.py
[add] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipe_modules/infra_system/examples/full.expected/win.json
[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipes/infra_continuous.expected/infra-cross-compile.json
[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipes/infra_continuous.expected/infra_win.json
[modify] https://crrev.com/e725824e178839115a44c19f8a2b89f5c5218a06/recipes/recipes/infra_continuous.expected/infra-64.json

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/infra/infra_internal/+/8b11dcb3e989c01a6a316379d43e775f44fc08ac

commit 8b11dcb3e989c01a6a316379d43e775f44fc08ac
Author: Dan Jacques <dnj@google.com>
Date: Tue Aug 01 20:04:26 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/infra/puppet/+/aa244e8e20b6f9cd0d0fb4f65952dd9c2d6db0c7

commit aa244e8e20b6f9cd0d0fb4f65952dd9c2d6db0c7
Author: Dan Jacques <dnj@google.com>
Date: Wed Aug 02 18:22:15 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/infra/puppet/+/5654ecde5220b74577445e40f262492270391a40

commit 5654ecde5220b74577445e40f262492270391a40
Author: Dan Jacques <dnj@google.com>
Date: Mon Aug 07 22:39:38 2017