Add a virtualenv-based python bootstrapping service to infra.

Add a virtualenv-based python bootstrapping service to infra.

R=agable@chromium.org, dnj@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/faf73634436999e7238e21d3d0b108b05d0af954

[iannucci, 2014-07-10 11:28:30]

On 2014/07/10 11:26:47, iannucci wrote:

Some things need to change (basically: repos need to be created), but other than
that, this all WAI. It should all work as-is on linux, some environmental stuff
remains to be done to support winders, but then it should be smooth sailing.

[dnj, 2014-07-10 16:06:18]

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py
File bootstrap/bootstrap.py (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:16: logging.basicConfig()
I like to put the 'basicConfig' and initial 'setLevel' in the "if __name__ ==
'__main__'" section. This allows others to import your module and use its
classes and functions without you force-configuring their logging setup.

Leaving the 'LOGGER =' would be fine in this case as long as you don't _use_ the
until the "basicConfig" is called (which is the case).

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:50: def print_deps(deps, indent=1, with_implicit=True):
If you anticipate any reuse, consider allowing a stream to be passed in and
defaulting to 'stdout'.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:60: with open(path, 'rb') as f:
Maybe pass a 'local' boolean that optionally toggles the 'b' flag. For seeded
'deps' you would use binary, but for local 'deps' (manifest) you would use local
encoding to make it easier to read/debug on that platform.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:102: tf = tempfile.mktemp('.html')
Consider using 'tempfile.NamedTemporaryFile', which can autodelete on 'close()',
giving you a more file-like API.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:107: print >> f, '<a href="%s">wat</a>' % link
Is "wat" going in the final version? :)

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:132: if isinstance(deps, basestring):
Is there a difference between 'basestring' and 'types.StringTypes'? I've always
used the latter for this sort of check.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:134: assert deps is not None
This seems useful to attach an error message to, like:
assert deps is not None, "No 'deps' found at %s" % (deps_path,)

(Of course, you'd have to also define/persist 'deps_path').

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:136: if hasattr(sys, 'real_prefix'):
Do this check before loading 'deps'? Since you short-circuit any usage of 'deps'
that seems reasonable.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:141: cur_deps = read_deps(manifest_path)
If you use the local 'read_deps' idea, this would need "local=True" added.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:144: shutil.rmtree(env, ignore_errors=True)
You may want to do a check to ensure that none of the doomed modules are
otherwise loaded in 'sys.modules' just in case someone imports those modules
explicitly/elsewhere only to have you explode them here.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:163: with open(manifest_path, 'wb') as f:
If you use the local 'read_deps' flag idea, this would become 'w'.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:164: f.write(repr(deps) + '\n')
Any love for 'pprint'? Also could emit the 'vim' line you use in 'deps.pst'.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:182: sys.exit(main(sys.argv[1:]))
Logger-friendly version would catch uncaught Exceptions and log them
(LOGGER.exception). Not much of difference, but...

Also consider an explicit return value in the case of an Exception so it's clear
what your script will return if something goes wrong.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap_wheel_wh...
File bootstrap/bootstrap_wheel_wheel.sh (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap_wheel_wh...
bootstrap/bootstrap_wheel_wheel.sh:6: 
Consider doing a script path discovery and constructing paths from that. This
would allow someone to run the script in another directory and still have it
work.

Alternatively you could assert "$0 == ./bootstrap_wheel_wheel.sh".

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap_wheel_wh...
bootstrap/bootstrap_wheel_wheel.sh:11: read -r -d '' PROG <<EOF
Consider exporting this to a "bootstrap_wheel_wheel_pre.py" script and just
running it. This would be pylintable and (IMO) cleaner.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap_wheel_wh...
bootstrap/bootstrap_wheel_wheel.sh:15: SHA=$(echo "$PROG" | python -)
Either set "-e" (errexit) or explicitly check the return value.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap_wheel_wh...
bootstrap/bootstrap_wheel_wheel.sh:19: rm -rf wheelhouse
Surround this w/ a directory check and maybe print a notice or something that
you're deleting user files.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap_wheel_wh...
bootstrap/bootstrap_wheel_wheel.sh:22: python - <<EOF
Consider exporting this to a "bootstrap_wheel_wheel_post.py" script and just
running it. This would be pylintable and (IMO) cleaner.

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py
File bootstrap/build_deps.py (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:76: wheeld = tempfile.mkdtemp()
This could fail after 'tmpd'. Since you have dual-cleanup, consider "tmpd =
wheeld = None", running 'mkdtemp' inside the 'try', and preconditioning the
'rmtree' on "if *blah*:".

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:79: m = getattr(__import__('custom_builds.%s' % name),
name)
'custom_builds.%s' % (name,)

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:109: tmp = tempfile.mktemp(ext)
Consider 'with tempfile.NamedTemporaryFile(mode='wb', suffix=ext) as tmp'; then
you can avoid the try/finally cleanup. You can also use "gsutil cat" w/
stdout=tmp to eliminate a race condition and also be more efficient.

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:131: os.chdir(os.path.dirname(__file__))
I'm generally avoid 'chdir' when possible. I understand this is a focused
application w/ known internal imports and stuffs so it's not so so bad, but for
the record it would be generically safer to construct relative/absolute paths
and pass "cwd" into subprocesses when needed.

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:147: if to_build and name not in to_build:
It would be worth doing an "argparse" w/ "to_build" as a nargs='*' argument IMO.

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:174: sys.exit(main(sys.argv[1:]))
If you are doing top-level logging initialization (see bootstrap.py), you would
need to 'basicConfig' here, too.

https://codereview.chromium.org/381043002/diff/1/bootstrap/ingest_source.py
File bootstrap/ingest_source.py (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/ingest_source.py#n...
bootstrap/ingest_source.py:29: if bit in ('tar', 'gz', 'tgz', 'zip', 'bz2'):
Poor 'tbz2' :(

https://codereview.chromium.org/381043002/diff/1/bootstrap/ingest_source.py#n...
bootstrap/ingest_source.py:36: tempname = tempfile.mktemp(suffix=ext)
Ugh Windows is stupid. With a streaming transfer, you can eliminate any temp
file grossness:
proc = subprocess.Popen(['gsutil', 'cp', '-', BUCKET + new_fname],
stdin=subprocess.PIPE)
proc.communicate(data)
if proc.returncode != 0:
    raise subprocess.CalledProcessError("wat")

[agable, 2014-07-10 17:12:57]

https://codereview.chromium.org/381043002/diff/1/DEPS
File DEPS (right):

https://codereview.chromium.org/381043002/diff/1/DEPS#newcode19
DEPS:19: "python", "./infra/bootstrap/bootstrap.py", "--deps_file",
Despite being pre-setup (obviously), this could still be invoked as "python -u
infra.bootstrap" couldn't it?

https://codereview.chromium.org/381043002/diff/1/DEPS#newcode27
DEPS:27: "infra.tools.bootstrap.get_appengine", "--dest=..",
I dislike the fact that there's now both "infra/bootstrap" and
"infra/tools/bootstrap". This should move into infra/bootstrap, probably.

https://codereview.chromium.org/381043002/diff/1/README.md
File README.md (right):

https://codereview.chromium.org/381043002/diff/1/README.md#newcode37
README.md:37: `./bootstrap/bootstrap.py --deps_file bootstrap/deps.pst`
Same comment as above -- could be python -m.

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md
File bootstrap/README.md (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode3
bootstrap/README.md:3: The infra/infra repo uses python [wheel files][1],
[virtualenv][2] and [pip][3]
FWIW, I prefer putting [asdf][1]-style footnotes at the bottom of the paragraph
or section, not the bottom of the whole document.

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode9
bootstrap/README.md:9: Run `./bootstrap/bootstrap.py --deps_file
bootstrap/deps.pst`. This will create
Same comment re: python -m.

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode11
bootstrap/README.md:11: `bootstrap/deps.pst`.  You must be online, or must
already have the wheels for
nit: normalize number of spaces after periods to always be 1.

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode15
bootstrap/README.md:15: see if it matches `deps.pst` (e.g. the diff is zero). If
it's not, then ENV
I think you mean i.e., not e.g.

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode18
bootstrap/README.md:18: Running `{repo_root}/run.py` in any capacity will
implicitly call `bootstrap.py`
I don't see it doing this...

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode19
bootstrap/README.md:19: unless `--no_bootstrap` is passed (which will make
run.py use whatever ENV is
run.py doesn't implement --no_bootstrap.

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode23
bootstrap/README.md:23: ## `deps.pst`
Is ".pst" a standard extension for PyAST files, or did you just make that up?

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode41
bootstrap/README.md:41:
'git+https://chromium.googlesource.com/infra/third_party/{package_name}'
What happens when spread this system to additional repositories. Will their
dependencies live in chromium/tools/third_party? What about infra_internal,
which may rely on both third_party repos from chromium.googlesource and
chrome-internal.googlesource?

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode52
bootstrap/README.md:52: 
nit: two newlines

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode61
bootstrap/README.md:61: ## `build_deps.py` / rolling deps
This file doesn't actually do a good job of describing the relationship between
ingest_source and build_deps and other tools. While you describe each tool well,
you don't describe workflows. AFAIK, the following workflows need to be covered:
* Adding a pure-python dependency
* Adding a pure-python dependency that isn't a well-formed package
* Adding a compiled dependency
* Rolling forward all of the above
* Forking + patching all of the above

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode106
bootstrap/README.md:106: To build and upload the wheel for 'wheel' pinned at the
version in deps.pst.
nit: uncapitalize To

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py
File bootstrap/bootstrap.py (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:16: logging.basicConfig()
On 2014/07/10 16:06:17, dnj wrote:
> I like to put the 'basicConfig' and initial 'setLevel' in the "if __name__ ==
> '__main__'" section. This allows others to import your module and use its
> classes and functions without you force-configuring their logging setup.
> 
> Leaving the 'LOGGER =' would be fine in this case as long as you don't _use_
the
> until the "basicConfig" is called (which is the case).

+1. This also lets you only set the loglevel once you actually know what it
should be, rather than setting a default level here and then overwriting it
after parsing command line arguments.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:20: ROOT =
os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
Comment on the expected path of root (i.e. /path/to/infra).

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:30: 'No matching wheel found for (%s==%s-%s_%s)' %
I think this version-build_sha format (and why is the last one an underscore?)
should be a GLOBAL.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:50: def print_deps(deps, indent=1, with_implicit=True):
On 2014/07/10 16:06:17, dnj wrote:
> If you anticipate any reuse, consider allowing a stream to be passed in and
> defaulting to 'stdout'.

If you do this, this might be a good opportunity for "from __future__ import
print_function" too.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:74: 
nit: no newline

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:88: LOGGER.error('Found more than one matching wheel?
%r', entry)
... matching wheel for %r.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:132: if isinstance(deps, basestring):
On 2014/07/10 16:06:17, dnj wrote:
> Is there a difference between 'basestring' and 'types.StringTypes'? I've
always
> used the latter for this sort of check.

Official python docs disagree, sorry :)

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py
File bootstrap/build_deps.py (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:21: 'coverage':
'git+https://github.com/nedbat/coveragepy/',
Why aren't we mirroring this yet? We should mirror this.

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:23:
'git+https://chromium.googlesource.com/infra/third_party/dateutil/'
Is this only 'missing' because its name is dateutil instead of python_dateutil?
That's kinda silly. We can rename the repository. Or rename the dependency?

https://codereview.chromium.org/381043002/diff/1/bootstrap/ingest_source.py
File bootstrap/ingest_source.py (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/ingest_source.py#n...
bootstrap/ingest_source.py:14: 
nit: two newlines

https://codereview.chromium.org/381043002/diff/1/run.py
File run.py (right):

https://codereview.chromium.org/381043002/diff/1/run.py#newcode1
run.py:1: #!ENV/bin/python
wat

[Vadim Sh., 2014-07-10 18:18:24]

Have you tested this on Win and Mac?

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md
File bootstrap/README.md (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode80
bootstrap/README.md:80: Once a wheel is sucessfully built, it is uploaded to
Who has write access to the bucket?

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode86
bootstrap/README.md:86: `build_deps.py` assumes that it can find `gsutil` on
PATH, so go ahead and
Does it have minimum version requirement for gsutil?

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode105
bootstrap/README.md:105: `bootstrap_wheel_wheel.sh`
funny name :)

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py
File bootstrap/bootstrap.py (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:65: import pip.wheel
nit: from pip import wheel

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:70: version, source_sha = entry['version'] ,
sha_for(entry)
nit: remove space before ','

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:115: py = os.path.join(sys.prefix, 'bin', 'python')
Does this work on windows? Also why not sys.executable?

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:131: print 'Activating environment: %r' % env
nit: use logging for this? Even logging.debug(...).

https://codereview.chromium.org/381043002/diff/1/run.py
File run.py (right):

https://codereview.chromium.org/381043002/diff/1/run.py#newcode1
run.py:1: #!ENV/bin/python
On 2014/07/10 17:12:57, agable wrote:
> wat

That's evil :) Also, how does it work on Windows?

Afaik, you can active virtualenv for the process inside the process, even if it
was launched with default python. See ENV/bin/active_this.py

[iannucci, 2014-07-11 02:14:46]

PTALP

https://codereview.chromium.org/381043002/diff/1/DEPS
File DEPS (right):

https://codereview.chromium.org/381043002/diff/1/DEPS#newcode19
DEPS:19: "python", "./infra/bootstrap/bootstrap.py", "--deps_file",
On 2014/07/10 17:12:55, agable wrote:
> Despite being pre-setup (obviously), this could still be invoked as "python -u
> infra.bootstrap" couldn't it?

I added the '-u' back, but the path.to.module syntax only works when using
run.py, which, in turn, assumes that ENV is already set up (and gclient is
running this from the infra/.. directory, which would mean that `python -m
infra.infra.bootstrap.bootstrap` would require an __init__.py in infra :p).

https://codereview.chromium.org/381043002/diff/1/DEPS#newcode27
DEPS:27: "infra.tools.bootstrap.get_appengine", "--dest=..",
On 2014/07/10 17:12:55, agable wrote:
> I dislike the fact that there's now both "infra/bootstrap" and
> "infra/tools/bootstrap". This should move into infra/bootstrap, probably.

Done.

https://codereview.chromium.org/381043002/diff/1/README.md
File README.md (right):

https://codereview.chromium.org/381043002/diff/1/README.md#newcode37
README.md:37: `./bootstrap/bootstrap.py --deps_file bootstrap/deps.pst`
On 2014/07/10 17:12:56, agable wrote:
> Same comment as above -- could be python -m.

Nack!

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md
File bootstrap/README.md (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode3
bootstrap/README.md:3: The infra/infra repo uses python [wheel files][1],
[virtualenv][2] and [pip][3]
On 2014/07/10 17:12:56, agable wrote:
> FWIW, I prefer putting [asdf][1]-style footnotes at the bottom of the
paragraph
> or section, not the bottom of the whole document.

Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode9
bootstrap/README.md:9: Run `./bootstrap/bootstrap.py --deps_file
bootstrap/deps.pst`. This will create
On 2014/07/10 17:12:56, agable wrote:
> Same comment re: python -m.

Nack!

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode11
bootstrap/README.md:11: `bootstrap/deps.pst`.  You must be online, or must
already have the wheels for
On 2014/07/10 17:12:56, agable wrote:
> nit: normalize number of spaces after periods to always be 1.

Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode15
bootstrap/README.md:15: see if it matches `deps.pst` (e.g. the diff is zero). If
it's not, then ENV
On 2014/07/10 17:12:56, agable wrote:
> I think you mean i.e., not e.g.

So I do... And I was being so careful....

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode18
bootstrap/README.md:18: Running `{repo_root}/run.py` in any capacity will
implicitly call `bootstrap.py`
On 2014/07/10 17:12:56, agable wrote:
> I don't see it doing this...

Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode19
bootstrap/README.md:19: unless `--no_bootstrap` is passed (which will make
run.py use whatever ENV is
On 2014/07/10 17:12:56, agable wrote:
> run.py doesn't implement --no_bootstrap.

Acknowledged.

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode23
bootstrap/README.md:23: ## `deps.pst`
On 2014/07/10 17:12:56, agable wrote:
> Is ".pst" a standard extension for PyAST files, or did you just make that up?

Changing to .pyl

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode41
bootstrap/README.md:41:
'git+https://chromium.googlesource.com/infra/third_party/{package_name}'
On 2014/07/10 17:12:56, agable wrote:
> What happens when spread this system to additional repositories. Will their
> dependencies live in chromium/tools/third_party? What about infra_internal,
> which may rely on both third_party repos from chromium.googlesource and
> chrome-internal.googlesource?

Chatted in person, but essentially, the only time this would happen is if we had
an internal library which we wanted to package as a wheel. This won't happen for
a while, and we can extend the deps.pyl format at that point.

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode52
bootstrap/README.md:52: 
On 2014/07/10 17:12:56, agable wrote:
> nit: two newlines

Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode61
bootstrap/README.md:61: ## `build_deps.py` / rolling deps
On 2014/07/10 17:12:56, agable wrote:
> This file doesn't actually do a good job of describing the relationship
between
> ingest_source and build_deps and other tools. While you describe each tool
well,
> you don't describe workflows. AFAIK, the following workflows need to be
covered:
> * Adding a pure-python dependency
> * Adding a pure-python dependency that isn't a well-formed package
> * Adding a compiled dependency
> * Rolling forward all of the above
> * Forking + patching all of the above

Done. Sort of. :D

https://codereview.chromium.org/381043002/diff/1/bootstrap/README.md#newcode106
bootstrap/README.md:106: To build and upload the wheel for 'wheel' pinned at the
version in deps.pst.
On 2014/07/10 17:12:56, agable wrote:
> nit: uncapitalize To

Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py
File bootstrap/bootstrap.py (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:16: logging.basicConfig()
On 2014/07/10 17:12:57, agable wrote:
> On 2014/07/10 16:06:17, dnj wrote:
> > I like to put the 'basicConfig' and initial 'setLevel' in the "if __name__
==
> > '__main__'" section. This allows others to import your module and use its
> > classes and functions without you force-configuring their logging setup.
> > 
> > Leaving the 'LOGGER =' would be fine in this case as long as you don't _use_
> the
> > until the "basicConfig" is called (which is the case).
> 
> +1. This also lets you only set the loglevel once you actually know what it
> should be, rather than setting a default level here and then overwriting it
> after parsing command line arguments.

Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:20: ROOT =
os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
On 2014/07/10 17:12:57, agable wrote:
> Comment on the expected path of root (i.e. /path/to/infra).

Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:30: 'No matching wheel found for (%s==%s-%s_%s)' %
On 2014/07/10 17:12:56, agable wrote:
> I think this version-build_sha format (and why is the last one an underscore?)
> should be a GLOBAL.

Unfortunately I don't think I can... they're constructed differently (on
purpose) in all places.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:50: def print_deps(deps, indent=1, with_implicit=True):
On 2014/07/10 17:12:56, agable wrote:
> On 2014/07/10 16:06:17, dnj wrote:
> > If you anticipate any reuse, consider allowing a stream to be passed in and
> > defaulting to 'stdout'.
> 
> If you do this, this might be a good opportunity for "from __future__ import
> print_function" too.

¯\_(ツ)_/¯

I'll refactor it if something needs it.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:60: with open(path, 'rb') as f:
On 2014/07/10 16:06:17, dnj wrote:
> Maybe pass a 'local' boolean that optionally toggles the 'b' flag. For seeded
> 'deps' you would use binary, but for local 'deps' (manifest) you would use
local
> encoding to make it easier to read/debug on that platform.

nah, it's just for newlines. It's all single-line text anyway.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:71: prefix = 'wheels/{}-{}-{}_{}'.format(name, version,
entry['build'],
from vadim: add a platform tag for linux (ubuntu)

e.g. u10.10

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:74: 
On 2014/07/10 17:12:56, agable wrote:
> nit: no newline

Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:88: LOGGER.error('Found more than one matching wheel?
%r', entry)
On 2014/07/10 17:12:56, agable wrote:
> ... matching wheel for %r.

Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:102: tf = tempfile.mktemp('.html')
On 2014/07/10 16:06:17, dnj wrote:
> Consider using 'tempfile.NamedTemporaryFile', which can autodelete on
'close()',
> giving you a more file-like API.

Yep, doesn't work on winders b/c the file can't be held open by more than one
process. wah-wah.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:107: print >> f, '<a href="%s">wat</a>' % link
On 2014/07/10 16:06:17, dnj wrote:
> Is "wat" going in the final version? :)

Yep, link text is ignored. It's dumb, dumb format. :p

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:132: if isinstance(deps, basestring):
On 2014/07/10 17:12:57, agable wrote:
> On 2014/07/10 16:06:17, dnj wrote:
> > Is there a difference between 'basestring' and 'types.StringTypes'? I've
> always
> > used the latter for this sort of check.
> 
> Official python docs disagree, sorry :)

Acknowledged.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:134: assert deps is not None
On 2014/07/10 16:06:17, dnj wrote:
> This seems useful to attach an error message to, like:
> assert deps is not None, "No 'deps' found at %s" % (deps_path,)
> 
> (Of course, you'd have to also define/persist 'deps_path').

Icing for later, this needs to land.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:136: if hasattr(sys, 'real_prefix'):
On 2014/07/10 16:06:17, dnj wrote:
> Do this check before loading 'deps'? Since you short-circuit any usage of
'deps'
> that seems reasonable.

Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:141: cur_deps = read_deps(manifest_path)
On 2014/07/10 16:06:17, dnj wrote:
> If you use the local 'read_deps' idea, this would need "local=True" added.

Acknowledged.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:144: shutil.rmtree(env, ignore_errors=True)
On 2014/07/10 16:06:17, dnj wrote:
> You may want to do a check to ensure that none of the doomed modules are
> otherwise loaded in 'sys.modules' just in case someone imports those modules
> explicitly/elsewhere only to have you explode them here.

I already check at the top that we're not in a virtualenv already ('real_prefix'
is the clue), so they couldn't be, unless someone is mucking with sys.path, in
which case they're asking for it.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:163: with open(manifest_path, 'wb') as f:
On 2014/07/10 16:06:17, dnj wrote:
> If you use the local 'read_deps' flag idea, this would become 'w'.

Acknowledged.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:164: f.write(repr(deps) + '\n')
On 2014/07/10 16:06:17, dnj wrote:
> Any love for 'pprint'? Also could emit the 'vim' line you use in 'deps.pst'.

mer. no one is going to poke at the manifest. can add later.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap_wheel_wh...
File bootstrap/bootstrap_wheel_wheel.sh (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap_wheel_wh...
bootstrap/bootstrap_wheel_wheel.sh:6: 
On 2014/07/10 16:06:18, dnj wrote:
> Consider doing a script path discovery and constructing paths from that. This
> would allow someone to run the script in another directory and still have it
> work.
> 
> Alternatively you could assert "$0 == ./bootstrap_wheel_wheel.sh".

Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap_wheel_wh...
bootstrap/bootstrap_wheel_wheel.sh:11: read -r -d '' PROG <<EOF
On 2014/07/10 16:06:18, dnj wrote:
> Consider exporting this to a "bootstrap_wheel_wheel_pre.py" script and just
> running it. This would be pylintable and (IMO) cleaner.

meh. there's already too many files here. Having this script self-contained is
more important, imo.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap_wheel_wh...
bootstrap/bootstrap_wheel_wheel.sh:15: SHA=$(echo "$PROG" | python -)
On 2014/07/10 16:06:18, dnj wrote:
> Either set "-e" (errexit) or explicitly check the return value.

Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap_wheel_wh...
bootstrap/bootstrap_wheel_wheel.sh:19: rm -rf wheelhouse
On 2014/07/10 16:06:17, dnj wrote:
> Surround this w/ a directory check and maybe print a notice or something that
> you're deleting user files.

wheelhouse is only used by these scripts for building. If the user puts stuff in
there... um... ??

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap_wheel_wh...
bootstrap/bootstrap_wheel_wheel.sh:22: python - <<EOF
On 2014/07/10 16:06:18, dnj wrote:
> Consider exporting this to a "bootstrap_wheel_wheel_post.py" script and just
> running it. This would be pylintable and (IMO) cleaner.

Acknowledged.

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py
File bootstrap/build_deps.py (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:21: 'coverage':
'git+https://github.com/nedbat/coveragepy/',
On 2014/07/10 17:12:57, agable wrote:
> Why aren't we mirroring this yet? We should mirror this.

Filed a bug: crbug.com/393015

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:23:
'git+https://chromium.googlesource.com/infra/third_party/dateutil/'
On 2014/07/10 17:12:57, agable wrote:
> Is this only 'missing' because its name is dateutil instead of
python_dateutil?
> That's kinda silly. We can rename the repository. Or rename the dependency?

Rename the repo. The name should match what's in setup.py for that package (e.g.
the wheel name). I mention this in crbug.com/393015 :)

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:76: wheeld = tempfile.mkdtemp()
On 2014/07/10 16:06:18, dnj wrote:
> This could fail after 'tmpd'. Since you have dual-cleanup, consider "tmpd =
> wheeld = None", running 'mkdtemp' inside the 'try', and preconditioning the
> 'rmtree' on "if *blah*:".

Did it moar betterz

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:79: m = getattr(__import__('custom_builds.%s' % name),
name)
On 2014/07/10 16:06:18, dnj wrote:
> 'custom_builds.%s' % (name,)

Right. Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:109: tmp = tempfile.mktemp(ext)
On 2014/07/10 16:06:18, dnj wrote:
> Consider 'with tempfile.NamedTemporaryFile(mode='wb', suffix=ext) as tmp';
then
> you can avoid the try/finally cleanup. You can also use "gsutil cat" w/
> stdout=tmp to eliminate a race condition and also be more efficient.

Nop. Winders. Made more better tho. 

Don't want to cp this into python memory. There's no point.

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:131: os.chdir(os.path.dirname(__file__))
On 2014/07/10 16:06:18, dnj wrote:
> I'm generally avoid 'chdir' when possible. I understand this is a focused
> application w/ known internal imports and stuffs so it's not so so bad, but
for
> the record it would be generically safer to construct relative/absolute paths
> and pass "cwd" into subprocesses when needed.

In general, I agree.

So I fixed it. :P

https://codereview.chromium.org/381043002/diff/1/bootstrap/build_deps.py#newc...
bootstrap/build_deps.py:147: if to_build and name not in to_build:
On 2014/07/10 16:06:18, dnj wrote:
> It would be worth doing an "argparse" w/ "to_build" as a nargs='*' argument
IMO.

Fiiiiiine.... Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/ingest_source.py
File bootstrap/ingest_source.py (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/ingest_source.py#n...
bootstrap/ingest_source.py:14: 
On 2014/07/10 17:12:57, agable wrote:
> nit: two newlines

Done.

https://codereview.chromium.org/381043002/diff/1/bootstrap/ingest_source.py#n...
bootstrap/ingest_source.py:29: if bit in ('tar', 'gz', 'tgz', 'zip', 'bz2'):
On 2014/07/10 16:06:18, dnj wrote:
> Poor 'tbz2' :(

Acknowledged.

https://codereview.chromium.org/381043002/diff/1/bootstrap/ingest_source.py#n...
bootstrap/ingest_source.py:36: tempname = tempfile.mktemp(suffix=ext)
On 2014/07/10 16:06:18, dnj wrote:
> Ugh Windows is stupid. With a streaming transfer, you can eliminate any temp
> file grossness:
> proc = subprocess.Popen(['gsutil', 'cp', '-', BUCKET + new_fname],
> stdin=subprocess.PIPE)
> proc.communicate(data)
> if proc.returncode != 0:
>     raise subprocess.CalledProcessError("wat")

Derp. Done.

https://codereview.chromium.org/381043002/diff/1/run.py
File run.py (right):

https://codereview.chromium.org/381043002/diff/1/run.py#newcode1
run.py:1: #!ENV/bin/python
On 2014/07/10 17:12:57, agable wrote:
> wat

yap!

[agable, 2014-07-11 18:38:16]

LGTM; make sure you read vadim's comments on patchset 1.

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py
File bootstrap/bootstrap.py (right):

https://codereview.chromium.org/381043002/diff/1/bootstrap/bootstrap.py#newco...
bootstrap/bootstrap.py:107: print >> f, '<a href="%s">wat</a>' % link
On 2014/07/11 02:14:45, iannucci wrote:
> On 2014/07/10 16:06:17, dnj wrote:
> > Is "wat" going in the final version? :)
> 
> Yep, link text is ignored. It's dumb, dumb format. :p

Maybe include a comment to that effect.

https://codereview.chromium.org/381043002/diff/20001/bootstrap/README.md
File bootstrap/README.md (right):

https://codereview.chromium.org/381043002/diff/20001/bootstrap/README.md#newc...
bootstrap/README.md:29: # File a ticke to have it mirrored (no matter what VCS!)
to
ticket

https://codereview.chromium.org/381043002/diff/20001/bootstrap/README.md#newc...
bootstrap/README.md:30: #   
`chromium.googlesource.com/infra/third_party/my_pkg`
Note that "my_pkg" is the self-identified name of the package, not necessarily
the name of the source vcs.

https://codereview.chromium.org/381043002/diff/20001/bootstrap/README.md#newc...
bootstrap/README.md:42: 'gs':  'deadbeefdeadbeefdeadbeefdeadbeef.tar.gz',  # if
tarball
if tarball, OR

https://codereview.chromium.org/381043002/diff/20001/bootstrap/README.md#newc...
bootstrap/README.md:52: # builds and uploads my_pkg-1.2.3-0_deadbeef...-....whl
to google storage
Maybe <thing> instead of ...

[Vadim Sh., 2014-07-11 18:39:44]

lgtm too
my comments are mostly nits :) I've discussed non-nits with Robbie offline.

[iannucci, 2014-07-22 18:30:50]

TALP!! Works on mac and linux. 

Wheelhouse is here:
https://commondatastorage.googleapis.com/chrome-python-wheelhouse/index.html?...

I'd like to land this and then work on windows compatibility today.

[Vadim Sh., 2014-07-22 18:43:25]

lgtm with nits

https://codereview.chromium.org/381043002/diff/80001/bootstrap/deps.pyl
File bootstrap/deps.pyl (right):

https://codereview.chromium.org/381043002/diff/80001/bootstrap/deps.pyl#newco...
bootstrap/deps.pyl:38: 'build_options': ['--without-libyaml'],
hm? Is it trying to use system-wide *.so by default or something like that?

https://codereview.chromium.org/381043002/diff/80001/bootstrap/util.py
File bootstrap/util.py (right):

https://codereview.chromium.org/381043002/diff/80001/bootstrap/util.py#newcode16
bootstrap/util.py:16: print ROOT
remove

https://codereview.chromium.org/381043002/diff/80001/bootstrap/util.py#newcode28
bootstrap/util.py:28: import platform
why lazy?

https://codereview.chromium.org/381043002/diff/80001/bootstrap/util.py#newcode30
bootstrap/util.py:30: if sys.platform.startswith('linux'):
if sys.....:
  return ...
return ''

[iannucci, 2014-07-22 18:51:03]

On 2014/07/22 18:43:25, Vadim Sh. wrote:
> lgtm with nits
> 
> https://codereview.chromium.org/381043002/diff/80001/bootstrap/deps.pyl
> File bootstrap/deps.pyl (right):
> 
>
https://codereview.chromium.org/381043002/diff/80001/bootstrap/deps.pyl#newco...
> bootstrap/deps.pyl:38: 'build_options': ['--without-libyaml'],
> hm? Is it trying to use system-wide *.so by default or something like that?

Exactly right. I had installed libyaml globally on my mac a long time ago and
forgot about it, so I thought that it was actually fully embedded in the
tarball.

Technically, this means we could use a mirror of the PyYAML repo instead of a
tarball now, too.

> 
> https://codereview.chromium.org/381043002/diff/80001/bootstrap/util.py
> File bootstrap/util.py (right):
> 
>
https://codereview.chromium.org/381043002/diff/80001/bootstrap/util.py#newcode16
> bootstrap/util.py:16: print ROOT
> remove

Oops, good catch.

> 
>
https://codereview.chromium.org/381043002/diff/80001/bootstrap/util.py#newcode28
> bootstrap/util.py:28: import platform
> why lazy?

Wanted to make sure it's importing under the virtualenv context. It's not
necessary though since it's a
system lib.

> 
>
https://codereview.chromium.org/381043002/diff/80001/bootstrap/util.py#newcode30
> bootstrap/util.py:30: if sys.platform.startswith('linux'):
> if sys.....:
>   return ...
> return ''

Done

[iannucci, 2014-07-22 20:38:14]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2014-07-22 20:39:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/iannucci@chromium.org/381043002/100001

[commit-bot: I haz the power, 2014-07-22 20:39:30]

Change committed as faf73634436999e7238e21d3d0b108b05d0af954

[Michael Achenbach, 2014-07-23 14:55:13]

Is there anything that needs to happen so that it works on master1? The v8 lkgr
is stale since this landed. Running locally on master1 doesn't work anymore.

/home/chrome-bot/infra/infra/run.py infra.services.lkgr_finder --project v8
-bash: /home/chrome-bot/infra/infra/run.py: ENV/bin/python: bad interpreter: No
such file or directory

[Michael Achenbach, 2014-07-23 14:58:10]

OK - a 'gclient sync' helps locally. Will make one on master1 as well. Looks
like the files were synced on master1 without a gclient sync.

[Michael Achenbach, 2014-07-23 15:03:59]

Done - this works again. If a 'gclient sync' is required elsewhere where these
scripts are running, please make sure to make one...

[Michael Achenbach, 2014-07-23 15:22:27]

Too happy too early. With "this works again" I meant a dry run I made on
master1. But the lkgrs are still stale. The dry runs would update them.

[Michael Achenbach, 2014-07-23 15:58:11]

FYI: Works again after phajdan's quickfix.

[jam, 2014-08-29 02:23:46]

On 2014/07/22 18:30:50, iannucci wrote:
> TALP!! Works on mac and linux. 
> 
> Wheelhouse is here:
>
https://commondatastorage.googleapis.com/chrome-python-wheelhouse/index.html?...
> 
> I'd like to land this and then work on windows compatibility today.

ping. 'fetch infra' is broken on Windows.

[iannucci, 2014-08-29 06:48:13]

On 2014/08/29 02:23:46, jam wrote:
> On 2014/07/22 18:30:50, iannucci wrote:
> > TALP!! Works on mac and linux. 
> > 
> > Wheelhouse is here:
> >
>
https://commondatastorage.googleapis.com/chrome-python-wheelhouse/index.html?...
> > 
> > I'd like to land this and then work on windows compatibility today.
> 
> ping. 'fetch infra' is broken on Windows.

Yeah, it is. git took over.