tools addition to automate setting up windows toolchain

tools addition to automate setting up windows toolchain

Attempting to make it easier and more reliable to set up a windows build toolchain. No need to acquire/install IDEs, service packs, patch things, etc. This tool downloads and unpacks sdks and compiler from various freely available sources.

On a clean machine, after getting depot_tools, and running "gclient" (which installs python), and "git clone ..." or "svn co ..." to get the main chromium repo:

c:\chrome>python src\tools\win\toolchain\toolchain.py
...wait 2-10 minutes depending on download speed...
c:\chrome>win_toolchain\env.bat
c:\chrome>cd src
c:\chrome\src>gclient sync
c:\chrome\src>ninja -C out\Debug chrome

This script does not run any "installation" steps, it only extracts tools, and so avoids changing any global machine state. This way, it (intends to) avoid mucking up other compiler installations, and being mucked up by them.

Requires gyp patch at https://codereview.chromium.org/11637025 to avoid needing to add fake registry keys.

BUG=windows compilers is hard, let's go shopping
TEST=on clean vmware win7 x64, and many rollbacks to 'fresh install' snapshot. http://crbug.com/166949 is in progress for a bot that does this.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175004

[scottmg, 2013-01-02 17:32:10]

rogerm: Could you do the main review? It's a bit long, but relatively simple
code.

alexeypa, maruel, cpu: FYI, but happy to have suggestions/fixes if you are
interested in reviewing.

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
File tools/win/toolchain/toolchain.py (right):

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:222: if len(sys.argv) == 2 and sys.argv[1] ==
'local':
will remove this after reviewed

[Roger McFarlane (Chromium), 2013-01-02 18:46:23]

Nice!

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
File tools/win/toolchain/toolchain.py (right):

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:41: sys.stdout.write('Downloading %s...' % url)
You're got print and sys.stdout.write() used in various spots. Maybe pick one
and use it consistently.

If you were trying to avoid the newline from print: you can add a trailing comma
to suppress the newline.

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:50: print
an errant "print"?

It would be nice to track the total bytes read, compare it with the
Content-Length (if given in the response), and raise/retry on mismatch.

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:97: .msis to a target location. Unfortunately,
this tool requires elevation for
.MSIs or .msi files

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:127: raise SystemExit("Couldn't extract %s" %
iso_path)
maybe infer the path to 7z; otherwise, you're assuming that the user is running
the script from the same directory.

  os.path.join(os.path.dirname(__file__), '7z.exe'))

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:217: def main():
nit: Can this easiliy be broken down further? maybe with a structure or
dictionary tying it all together? If not, add some more running commentary.

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:230: # happens before the user wanders off to
get coffee.
Ha! Nice comment.

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:342: # Apply patch from
put this in its own function?

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:348: asyncinfo_h = open(asyncinfo_h_path,
'rb').read()
Do the read inside a with clause?

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:351: open(asyncinfo_h_path,
'wb').write(patched)
Do the write inside a with clause?

[alexeypa (please no reviews), 2013-01-02 19:33:57]

Nice script! I like that it does not modify the global environment. It makes it
very easy to re-create a clean environment.

Just a couple of nits below.

https://codereview.chromium.org/11633012/diff/18002/tools/win/toolchain/toolc...
File tools/win/toolchain/toolchain.py (right):

https://codereview.chromium.org/11633012/diff/18002/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:78: def DownloadWDKUpdate():
nit: This is not WDK update. This is SDK 7.1 update.

https://codereview.chromium.org/11633012/diff/18002/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:147: os.unlink(os.path.join(target_path,
os.path.basename(msi_path)))
Is this needed? The script deletes all temporary files it created at the end.

https://codereview.chromium.org/11633012/diff/18002/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:311: PullFrom(from_vcupdate_x64,
extracted_update_x64, target_dir)
nit: One small concern I have is that PullFrom() does not fully recreates what
the installer does, so the environment created by this script can be different
from the "stock" environment in subtle ways.

For instance VC-Compiler-KB2519277.exe deletes ammintrin.h header breaking the
build. While removing this header is a bug in the updater, there could be other
headers/libraries that should actually be deleted.

I suspect this is not a real problem but it might be worth comparing the list of
files we get via the traditional install and this script.

[scottmg, 2013-01-02 21:00:47]

Thanks both.

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
File tools/win/toolchain/toolchain.py (right):

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:41: sys.stdout.write('Downloading %s...' % url)
On 2013/01/02 18:46:23, Roger McFarlane (Chromium) wrote:
> You're got print and sys.stdout.write() used in various spots. Maybe pick one
> and use it consistently.
> 
> If you were trying to avoid the newline from print: you can add a trailing
comma
> to suppress the newline.

Yeah, I had that in ps #17, but it also needs sys.stdout.softspace = 0 to avoid
the magic space behaviour of print. I'll switch to sys.stdout.write everywhere I
guess.

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:50: print
On 2013/01/02 18:46:23, Roger McFarlane (Chromium) wrote:
> an errant "print"?

was for \n.

> 
> It would be nice to track the total bytes read, compare it with the
> Content-Length (if given in the response), and raise/retry on mismatch. 

Done.

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:97: .msis to a target location. Unfortunately,
this tool requires elevation for
On 2013/01/02 18:46:23, Roger McFarlane (Chromium) wrote:
> .MSIs or .msi files

Done.

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:127: raise SystemExit("Couldn't extract %s" %
iso_path)
On 2013/01/02 18:46:23, Roger McFarlane (Chromium) wrote:
> maybe infer the path to 7z; otherwise, you're assuming that the user is
running
> the script from the same directory.
> 
>   os.path.join(os.path.dirname(__file__), '7z.exe'))

Done.

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:217: def main():
On 2013/01/02 18:46:23, Roger McFarlane (Chromium) wrote:
> nit: Can this easiliy be broken down further? maybe with a structure or
> dictionary tying it all together? If not, add some more running commentary.

Done.

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:342: # Apply patch from
On 2013/01/02 18:46:23, Roger McFarlane (Chromium) wrote:
> put this in its own function?

Done.

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:348: asyncinfo_h = open(asyncinfo_h_path,
'rb').read()
On 2013/01/02 18:46:23, Roger McFarlane (Chromium) wrote:
> Do the read inside a with clause?

Sure. (fwiw, in "regular" CPython, it's guaranteed to be closed at the end of
the statement, which is why the write succeeds without a sharing violation.)

https://codereview.chromium.org/11633012/diff/17001/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:351: open(asyncinfo_h_path,
'wb').write(patched)
On 2013/01/02 18:46:23, Roger McFarlane (Chromium) wrote:
> Do the write inside a with clause?

Done.

https://codereview.chromium.org/11633012/diff/18002/tools/win/toolchain/toolc...
File tools/win/toolchain/toolchain.py (right):

https://codereview.chromium.org/11633012/diff/18002/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:78: def DownloadWDKUpdate():
On 2013/01/02 19:33:57, alexeypa wrote:
> nit: This is not WDK update. This is SDK 7.1 update.

Oops, thanks. Done.

https://codereview.chromium.org/11633012/diff/18002/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:147: os.unlink(os.path.join(target_path,
os.path.basename(msi_path)))
On 2013/01/02 19:33:57, alexeypa wrote:
> Is this needed? The script deletes all temporary files it created at the end.

Done.

https://codereview.chromium.org/11633012/diff/18002/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:311: PullFrom(from_vcupdate_x64,
extracted_update_x64, target_dir)
On 2013/01/02 19:33:57, alexeypa wrote:
> nit: One small concern I have is that PullFrom() does not fully recreates what
> the installer does, so the environment created by this script can be different
> from the "stock" environment in subtle ways.
> 
> For instance VC-Compiler-KB2519277.exe deletes ammintrin.h header breaking the
> build. While removing this header is a bug in the updater, there could be
other
> headers/libraries that should actually be deleted.
> 
> I suspect this is not a real problem but it might be worth comparing the list
of
> files we get via the traditional install and this script.

It took me a while to figure out what was going on with ammintrin.h! I am
concerned about subtle differences too. My handwave-y theory was that if it's
able to build Chromium and run some set of tests on a bot, it's probably close
enough. Having said that, I'm sure we'll need to improve the "install emulation"
over time.

[Roger McFarlane (Chromium), 2013-01-02 22:41:31]

lgtm with a nit

https://codereview.chromium.org/11633012/diff/21002/tools/win/toolchain/toolc...
File tools/win/toolchain/toolchain.py (right):

https://codereview.chromium.org/11633012/diff/21002/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:165: '%s x "%s" -y "-o%s" > nul' % (sz_path,
iso_path, target_path)) != 0:
quote the path to 7z

https://codereview.chromium.org/11633012/diff/21002/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:420: target_dir =
os.path.abspath('win_toolchain')
optional: command line param? maybe keeping current behaviour as default?

[scottmg, 2013-01-02 23:31:09]

https://codereview.chromium.org/11633012/diff/21002/tools/win/toolchain/toolc...
File tools/win/toolchain/toolchain.py (right):

https://codereview.chromium.org/11633012/diff/21002/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:165: '%s x "%s" -y "-o%s" > nul' % (sz_path,
iso_path, target_path)) != 0:
On 2013/01/02 22:41:31, Roger McFarlane (Chromium) wrote:
> quote the path to 7z

That seemed like a great idea, but wow! In doing that I ran into some strange
behaviour:
- 7z can't find its dll if argv[0] is quoted (wat!). So, instead, at startup I
chdir to 7z's directory and run unqualified. I should just remove the dependency
on 7z entirely, but it's a bit of work.
- When TMP/TEMP have spaces, the msi extract was missing quotes, so fixed that.
- Centralize the running and error handling into RunOrDie, except for robocopy
because it uses unusual exit codes.

https://codereview.chromium.org/11633012/diff/21002/tools/win/toolchain/toolc...
tools/win/toolchain/toolchain.py:420: target_dir =
os.path.abspath('win_toolchain')
On 2013/01/02 22:41:31, Roger McFarlane (Chromium) wrote:
> optional: command line param? maybe keeping current behaviour as default?

Done.

[commit-bot: I haz the power, 2013-01-02 23:32:56]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[scottmg, 2013-01-02 23:40:35]

On 2013/01/02 23:32:56, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a lowly provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Curses. If someone else could also review/stamp it would be appreciated.

[Roger McFarlane (Chromium), 2013-01-03 16:45:28]

On 2013/01/02 23:32:56, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a lowly provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Should I feel insulted by the commit-bot? Kinda harsh, no?

:)

[cpu_(ooo_6.6-7.5), 2013-01-03 20:37:26]

lgtm

[commit-bot: I haz the power, 2013-01-03 20:38:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/11633012/27001

[commit-bot: I haz the power, 2013-01-03 20:38:35]

Presubmit check for 11633012-27001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  tools/win/toolchain/toolchain.py, line 318, 132 chars

[scottmg, 2013-01-03 20:39:31]

M-A, I'm going to land this now. Once you're back in-office and have time to
look, let me know if you have any comments/concerns and I'll take care of them.