Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
135933002:
Automatic Windows toolchain (Closed)
|
Created:
6 years, 11 months ago by scottmg Modified:
6 years, 11 months ago CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, ilevy-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Visibility:
Public. |
DescriptionAutomatic Windows toolchain
Per comments in linked bug, this is simply moved from chromium. The
only new thing is hooking it into update_depot_tools.bat, and
updating a few paths in the scripts.
This is currently opt-in via "set DEPOT_TOOLS_WIN_TOOLCHAIN=1" but
that will be removed once it's ready for deployment.
R=iannucci@chromium.org, maruel@chromium.org
BUG=323300
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244983
Patch Set 1 #Patch Set 2 : opt-in for now #
Total comments: 47
Patch Set 3 : fixes #Patch Set 4 : extract whole iso #
Total comments: 18
Patch Set 5 : fixes 2 #Patch Set 6 : +x #
Total comments: 6
Patch Set 7 : fixes 3 #
Total comments: 21
Patch Set 8 : fixes 4 #
Messages
Total messages: 21 (0 generated)
I'm not super warm to the idea but I understand the rationale. I'll request the rationale to be written down somewhere, likely in the python script docstring. There's 3 things that needs to be taken in account here: - future proof-ness - backward compatibility in the future - maintainability That's why I'd request the python code to be of very high standard. Also it needs to take in account what's the upgrade flow to VS2015 (or whatever name the next version has), it's future proofness. On the other hand, what happens when we decide to bump to an hypothetical VS2015 and someones wants to build an old version that doesn't compile yet on this toolset? That's backward compatibility. These needs to be called for explicitly in the python script, so that if you leave the project, someone else can take on from there and do it on your behalf. Hopefully you won't leave the project but still. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/get_toolch... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/135933002/diff/50001/win_toolchain/get_toolch... win_toolchain/get_toolchain_if_necessary.py:4: docstring, to explain what it does overall. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:5: # Extracts a Windows VS2013 toolchain from various downloadable pieces. comment -> docstring https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:37: """Generate a temporary directory (for downloading or extracting to) and keep Generates https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:46: """Remove all temporary directories created by |TempDir()|.""" Removes Imperative everywhere https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:57: """Get the .iso path.""" \n\nIf pro is False, downloads the express edition. It's not a path, it's an url. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:73: bytes_read = 0 0L otherwise it'll overflow on python 32 bits on 2gb file. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:102: ExtractExe = ExtractIso Why? https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:130: for (package, skippable) in packages: I'd prefer 'required' to 'skippable'. Also a docstring to quickly explain the 'package' format. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:140: extracted_iso = ExtractIso(image) Could you just extract the files needed? It'd save a lot of temporary space. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:141: results = ExtractMsiList(extracted_iso, [ Create the list as a named variable, then call the function. It'll be more readable. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:169: 'Program Files\\Microsoft Visual Studio 12.0\\': '.\\', Sort keys https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:182: partial_path = full_path[len(prefix) + 1:] # +1 for trailing \ comment on line before. Note that the trailing \ may have bad side effects, use '\\' just in case even if it is in a comment. (At worst it only confuses the text editor or the code reviewer). https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:183: #print 'partial_path', partial_path Remove these, or use logging.info() A --verbose flag would be useful. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:199: environment. This is normally generated by a full install of the SDK, but we empty line between 1 liner summary and description. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:202: target_dir, r'win8sdk\bin\SetEnv.cmd'), 'w') as file: nit: Personally, I'd use a single call per functional block. And I'd use 'f' instead of 'file', which is a python intrinsic. f.write( '@echo off\n' ':: Generated by win_toolchain\\toolchain2013.py.\n' '\n' 'set PATH=%~dp0..\\..\\Common7\\IDE;%PATH%\n' 'set INCLUDE=%~dp0..\\..\\win8sdk\\Include\\um;' '%~dp0..\\..\\win8sdk\\Include\\shared;' '%~dp0..\\..\\VC\\include;' '%~dp0..\\..\\VC\\atlmfc\\include\n' 'if "%1"=="/x64" goto x64\n') https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:213: # x86. If we're Pro, then use the amd64_x86 cross (we don't support x86 Would it be possible to write the file unconditionally except for one flag so that it is always the same file that is written? It'd be more maintainable IMHO. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:226: file.write('goto done\n') FYI, you can use 'goto :EOF\n' https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:249: parser = OptionParser() parser = OptionParser(description=sys.moduels[__name__].__doc__) https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:252: default=os.path.abspath('win_toolchain_2013')) you probably want os.path.join(FILE_PATH, 'win_toolcain_2013') and not rely on cwd. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:258: parser.add_option('--express', metavar='EXPRESS', do not use metavar on a store_true flag, it's confusing. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:264: sys.stderr.write('%s already exists. Please [re]move it or use ' parser.error() would work fine. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:268: pro = not options.express Why use options.clean but then use pro as a standalone variable? Use one style or the other. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:284: main() sys.exit(main()) otherwise you are ditching the return code.
Thanks! On 2014/01/12 16:39:13, M-A Ruel wrote: > I'm not super warm to the idea but I understand the rationale. I'll request the > rationale to be written down somewhere, likely in the python script docstring. > > There's 3 things that needs to be taken in account here: > - future proof-ness > - backward compatibility in the future > - maintainability > > That's why I'd request the python code to be of very high standard. Also it > needs to take in account what's the upgrade flow to VS2015 (or whatever name the > next version has), it's future proofness. On the other hand, what happens when > we decide to bump to an hypothetical VS2015 and someones wants to build an old > version that doesn't compile yet on this toolset? That's backward compatibility. My thinking on this is that we maintain the ability to build the last version of a particular major revision. So, once Update 1 for VS2013 is out, we update to that, and all future updates. When VS2015 comes out though, that's not over top of 2013. Instead, we leave toolchain2013.py there, and for a time automatically pull _both_ 2013 and 2015 (with a hypothetical new toolchain2015.py). These scripts would both just be putting the bits in a known location. The details of how to build are left to the project code. To that end, I renamed the target directory from "files" to "vs2013_files" so that they can be side-by-side in the future when the next major revision is released. > These needs to be called for explicitly in the python script, I added something to this effect to the docstring in get_toolchain_if_necessary.py. > so that if you > leave the project, someone else can take on from there and do it on your behalf. > Hopefully you won't leave the project but still. Geez, try to make a change to depot_tools and all of a sudden they're trying to get you off the team. ;)
https://codereview.chromium.org/135933002/diff/50001/win_toolchain/get_toolch... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/135933002/diff/50001/win_toolchain/get_toolch... win_toolchain/get_toolchain_if_necessary.py:4: On 2014/01/12 16:39:13, M-A Ruel wrote: > docstring, to explain what it does overall. Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:5: # Extracts a Windows VS2013 toolchain from various downloadable pieces. On 2014/01/12 16:39:13, M-A Ruel wrote: > comment -> docstring Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:37: """Generate a temporary directory (for downloading or extracting to) and keep On 2014/01/12 16:39:13, M-A Ruel wrote: > Generates Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:46: """Remove all temporary directories created by |TempDir()|.""" On 2014/01/12 16:39:13, M-A Ruel wrote: > Removes > > Imperative everywhere Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:57: """Get the .iso path.""" On 2014/01/12 16:39:13, M-A Ruel wrote: > \n\nIf pro is False, downloads the express edition. > > It's not a path, it's an url. Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:73: bytes_read = 0 On 2014/01/12 16:39:13, M-A Ruel wrote: > 0L > otherwise it'll overflow on python 32 bits on 2gb file. Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:102: ExtractExe = ExtractIso On 2014/01/12 16:39:13, M-A Ruel wrote: > Why? Oops, removed. It was necessary in the 2010/2012 version of the script. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:130: for (package, skippable) in packages: On 2014/01/12 16:39:13, M-A Ruel wrote: > I'd prefer 'required' to 'skippable'. Also a docstring to quickly explain the > 'package' format. Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:140: extracted_iso = ExtractIso(image) On 2014/01/12 16:39:13, M-A Ruel wrote: > Could you just extract the files needed? It'd save a lot of temporary space. I tried this thinking it sounded like a good idea (see ps3), but the .msi aren't self-contained, they refer to other cabs, etc. so I don't know the full list. So, unfortunately no, not in any sensible way. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:141: results = ExtractMsiList(extracted_iso, [ On 2014/01/12 16:39:13, M-A Ruel wrote: > Create the list as a named variable, then call the function. It'll be more > readable. Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:169: 'Program Files\\Microsoft Visual Studio 12.0\\': '.\\', On 2014/01/12 16:39:13, M-A Ruel wrote: > Sort keys Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:182: partial_path = full_path[len(prefix) + 1:] # +1 for trailing \ On 2014/01/12 16:39:13, M-A Ruel wrote: > comment on line before. Note that the trailing \ may have bad side effects, use > '\\' just in case even if it is in a comment. (At worst it only confuses the > text editor or the code reviewer). How about a trailing '.'? Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:183: #print 'partial_path', partial_path On 2014/01/12 16:39:13, M-A Ruel wrote: > Remove these, or use logging.info() > A --verbose flag would be useful. Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:199: environment. This is normally generated by a full install of the SDK, but we On 2014/01/12 16:39:13, M-A Ruel wrote: > empty line between 1 liner summary and description. Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:202: target_dir, r'win8sdk\bin\SetEnv.cmd'), 'w') as file: On 2014/01/12 16:39:13, M-A Ruel wrote: > nit: Personally, I'd use a single call per functional block. And I'd use 'f' > instead of 'file', which is a python intrinsic. > > f.write( > '@echo off\n' > ':: Generated by win_toolchain\\toolchain2013.py.\n' > '\n' > 'set PATH=%~dp0..\\..\\Common7\\IDE;%PATH%\n' > 'set INCLUDE=%~dp0..\\..\\win8sdk\\Include\\um;' > '%~dp0..\\..\\win8sdk\\Include\\shared;' > '%~dp0..\\..\\VC\\include;' > '%~dp0..\\..\\VC\\atlmfc\\include\n' > 'if "%1"=="/x64" goto x64\n') Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:213: # x86. If we're Pro, then use the amd64_x86 cross (we don't support x86 On 2014/01/12 16:39:13, M-A Ruel wrote: > Would it be possible to write the file unconditionally except for one flag so > that it is always the same file that is written? It'd be more maintainable IMHO. Do you mean having a flag passed to the batch file instead of a flag at generation time? No, it's run by other tools that won't know which flag to pass. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:226: file.write('goto done\n') On 2014/01/12 16:39:13, M-A Ruel wrote: > FYI, you can use 'goto :EOF\n' Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:249: parser = OptionParser() On 2014/01/12 16:39:13, M-A Ruel wrote: > parser = OptionParser(description=sys.moduels[__name__].__doc__) Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:252: default=os.path.abspath('win_toolchain_2013')) On 2014/01/12 16:39:13, M-A Ruel wrote: > you probably want os.path.join(FILE_PATH, 'win_toolcain_2013') and not rely on > cwd. Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:258: parser.add_option('--express', metavar='EXPRESS', On 2014/01/12 16:39:13, M-A Ruel wrote: > do not use metavar on a store_true flag, it's confusing. Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:264: sys.stderr.write('%s already exists. Please [re]move it or use ' On 2014/01/12 16:39:13, M-A Ruel wrote: > parser.error() would work fine. Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:268: pro = not options.express On 2014/01/12 16:39:13, M-A Ruel wrote: > Why use options.clean but then use pro as a standalone variable? Use one style > or the other. Done. https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:284: main() On 2014/01/12 16:39:13, M-A Ruel wrote: > sys.exit(main()) > otherwise you are ditching the return code. Done.
https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/135933002/diff/50001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:213: # x86. If we're Pro, then use the amd64_x86 cross (we don't support x86 On 2014/01/13 18:21:25, scottmg wrote: > On 2014/01/12 16:39:13, M-A Ruel wrote: > > Would it be possible to write the file unconditionally except for one flag so > > that it is always the same file that is written? It'd be more maintainable > IMHO. > > Do you mean having a flag passed to the batch file instead of a flag at > generation time? No, it's run by other tools that won't know which flag to pass. No I meant always write the same file except for a set PRO=1 or 0 at the top of the file. But I don't really mind. https://codereview.chromium.org/135933002/diff/190001/win_toolchain/get_toolc... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/135933002/diff/190001/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. shebang + +x too. https://codereview.chromium.org/135933002/diff/190001/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:117: if sys.platform not in ('win32', 'cygwin'): Sort values. This was tested in cygwin? https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... File win_toolchain/toolchain.sha1 (right): https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... win_toolchain/toolchain.sha1:1: f31aca625a2ebb7f05439641e1c2ce5e23ed1bf9 Shouldn't this file be named toolchain_vs2013.sha1? Isn't the sha1 different when a user gets the Pro or the Express version? https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. add shebang (even if it is windows only) for consistency. https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:2: # Use of this source code is governed by a BSD-style license that can be Set the file +x. (Not sure how to do this on git on Windows) https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:60: If |pro| is False, downloads the Express edition.""" If |pro| is False, downloads the Express edition. """ (everywhere when it doesn't fit a single line) https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:138: sys.stdout.write('Pro-only %s skipped.\n' % package) Is this useful information for the user? https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:172: sys.stdout.write('Copying to final location...\n') I prefer the more standard: print('Copying to final location...')
On 2014/01/13 18:12:13, scottmg wrote: > > so that if you > > leave the project, someone else can take on from there and do it on your > behalf. > > Hopefully you won't leave the project but still. > > Geez, try to make a change to depot_tools and all of a sudden they're trying to > get you off the team. ;) Usually I use the wording "in case you are hit by a bus", which ever comes first.
(Reitveld is being difficult) https://codereview.chromium.org/135933002/diff/190001/win_toolchain/get_toolc... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/135933002/diff/190001/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. On 2014/01/13 18:29:05, M-A Ruel wrote: > shebang + +x too. Done. https://codereview.chromium.org/135933002/diff/190001/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:117: if sys.platform not in ('win32', 'cygwin'): On 2014/01/13 18:29:05, M-A Ruel wrote: > Sort values. Done. > > This was tested in cygwin? Not even a little. https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... File win_toolchain/toolchain.sha1 (right): https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... win_toolchain/toolchain.sha1:1: f31aca625a2ebb7f05439641e1c2ce5e23ed1bf9 On 2014/01/13 18:29:05, M-A Ruel wrote: > Shouldn't this file be named toolchain_vs2013.sha1? Done. > > Isn't the sha1 different when a user gets the Pro or the Express version? Yes, I haven't tackled that problem yet. I guess it will need to be a pair (or even set) of versions. Currently get_toolchain_if_necessary.py always gets Pro. https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. On 2014/01/13 18:29:05, M-A Ruel wrote: > add shebang (even if it is windows only) for consistency. Done. https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:2: # Use of this source code is governed by a BSD-style license that can be On 2014/01/13 18:29:05, M-A Ruel wrote: > Set the file +x. > (Not sure how to do this on git on Windows) Done. https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:60: If |pro| is False, downloads the Express edition.""" On 2014/01/13 18:29:05, M-A Ruel wrote: > If |pro| is False, downloads the Express edition. > """ > > > (everywhere when it doesn't fit a single line) Done. https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:138: sys.stdout.write('Pro-only %s skipped.\n' % package) On 2014/01/13 18:29:05, M-A Ruel wrote: > Is this useful information for the user? Removed. https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:172: sys.stdout.write('Copying to final location...\n') On 2014/01/13 18:29:05, M-A Ruel wrote: > I prefer the more standard: > print('Copying to final location...') Mostly because there's \r and no-\n in |Download()| so I didn't use print for consistency. Would you prefer this function uses print though?
Mostly fine at this point. I'll let Robbie do the stamping. https://codereview.chromium.org/135933002/diff/190001/win_toolchain/get_toolc... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/135933002/diff/190001/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:117: if sys.platform not in ('win32', 'cygwin'): On 2014/01/13 18:46:18, scottmg wrote: > > This was tested in cygwin? > > Not even a little. Then it doesn't work. You'll have to wait for Peter to complain I guess. https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/135933002/diff/190001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:172: sys.stdout.write('Copying to final location...\n') On 2014/01/13 18:46:18, scottmg wrote: > On 2014/01/13 18:29:05, M-A Ruel wrote: > > I prefer the more standard: > > print('Copying to final location...') > > Mostly because there's \r and no-\n in |Download()| so I didn't use print for > consistency. Would you prefer this function uses print though? No, that's fine. https://codereview.chromium.org/135933002/diff/30003/win_toolchain/get_toolch... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/135933002/diff/30003/win_toolchain/get_toolch... win_toolchain/get_toolchain_if_necessary.py:2: no empty line https://codereview.chromium.org/135933002/diff/30003/win_toolchain/toolchain2... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/135933002/diff/30003/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:2: no empty line https://codereview.chromium.org/135933002/diff/30003/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:11: from optparse import OptionParser import optparse it's only used once anyway so it's not like it's saving a lots of bytes.
https://codereview.chromium.org/135933002/diff/30003/win_toolchain/get_toolch... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/135933002/diff/30003/win_toolchain/get_toolch... win_toolchain/get_toolchain_if_necessary.py:2: On 2014/01/13 18:50:32, M-A Ruel wrote: > no empty line Done. https://codereview.chromium.org/135933002/diff/30003/win_toolchain/toolchain2... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/135933002/diff/30003/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:2: On 2014/01/13 18:50:32, M-A Ruel wrote: > no empty line Done. https://codereview.chromium.org/135933002/diff/30003/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:11: from optparse import OptionParser On 2014/01/13 18:50:32, M-A Ruel wrote: > import optparse > > it's only used once anyway so it's not like it's saving a lots of bytes. Done.
ping: iannucci per https://codereview.chromium.org/135933002/#msg8
looks like code to me :P I'm mostly curious about your thoughts on the download_from_google_storage comment. https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:51: assert p != 0xffffffff Is this the same as (p != -1)? https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:70: def CalculateHash(root): We may want to (eventually) integrate this with depot_tools/download_from_google_storage.py, which does a very similar thing. In fact, the format is pretty close to identical (i.e. filename.sha1 whose contents is the sha1 to find+download+place at filename). Actually it's close enough that I think it may actually cause problems. I think some of the bots run download_from_google_storage over depot_tools, and they may see your sha1 file. https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:118: if sys.platform not in ('cygwin', 'win32'): this should probably be `not sys.platform.startswith(('cygwin', 'win'))`, but it also probably doesn't matter. https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:145: subprocess.check_call('rmdir /s/q "%s"' % target_dir, shell=True) [Ew... I know this is win-only and all, but can't we at least use shutil where it works?] https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... win_toolchain/toolchain2013.py:28: raise SystemExit('Long form of path longer than 260 chars: %s' % path) fyi, this can also be said like: sys.exit('Long form of path longer than 260 chars: %s' % path) https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... win_toolchain/toolchain2013.py:35: raise SystemExit('%s failed.' % command) Isn't this just subprocess.check_call? https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... win_toolchain/toolchain2013.py:44: g_temp_dirs.append(temp) since you're not actually assigning g_temp_dirs, you don't need to global it (yeah, I know, python is silly, non-constable object references are evil, etc. etc.) https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... win_toolchain/toolchain2013.py:56: g_temp_dirs = [] you could also get rid of the global line here by saying g_temp_dirs.clear() instead of assigning an empty list.
https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:51: assert p != 0xffffffff On 2014/01/14 10:14:11, iannucci wrote: > Is this the same as (p != -1)? I would assume not in python? https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:70: def CalculateHash(root): On 2014/01/14 10:14:11, iannucci wrote: > We may want to (eventually) integrate this with > depot_tools/download_from_google_storage.py, which does a very similar thing. In > fact, the format is pretty close to identical (i.e. filename.sha1 whose contents > is the sha1 to find+download+place at filename). Actually it's close enough that > I think it may actually cause problems. I think some of the bots run > download_from_google_storage over depot_tools, and they may see your sha1 file. OK. I don't think we can mirror VS for non-Googlers, so I can't just download it unfortunately. I guess I'll rename this one to .hash. https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:118: if sys.platform not in ('cygwin', 'win32'): On 2014/01/14 10:14:11, iannucci wrote: > this should probably be `not sys.platform.startswith(('cygwin', 'win'))`, but it > also probably doesn't matter. Done. https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:145: subprocess.check_call('rmdir /s/q "%s"' % target_dir, shell=True) On 2014/01/14 10:14:11, iannucci wrote: > [Ew... I know this is win-only and all, but can't we at least use shutil where > it works?] shutil.rmtree sucks; it aborts if there's any files marked read only. https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... win_toolchain/toolchain2013.py:28: raise SystemExit('Long form of path longer than 260 chars: %s' % path) On 2014/01/14 10:14:11, iannucci wrote: > fyi, this can also be said like: > > sys.exit('Long form of path longer than 260 chars: %s' % path) Done. https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... win_toolchain/toolchain2013.py:35: raise SystemExit('%s failed.' % command) On 2014/01/14 10:14:11, iannucci wrote: > Isn't this just subprocess.check_call? Done. https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... win_toolchain/toolchain2013.py:44: g_temp_dirs.append(temp) On 2014/01/14 10:14:11, iannucci wrote: > since you're not actually assigning g_temp_dirs, you don't need to global it > (yeah, I know, python is silly, non-constable object references are evil, etc. > etc.) Yeah, it feels a bit nicer to make it obvious that it's not local to me. But I can change it if you object. https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... win_toolchain/toolchain2013.py:56: g_temp_dirs = [] On 2014/01/14 10:14:11, iannucci wrote: > you could also get rid of the global line here by saying g_temp_dirs.clear() > instead of assigning an empty list. Same.
ping
lgtm now tell jschuh to leave me alone :P https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:51: assert p != 0xffffffff On 2014/01/14 17:09:35, scottmg wrote: > On 2014/01/14 10:14:11, iannucci wrote: > > Is this the same as (p != -1)? > > I would assume not in python? Well... it depends if GetFileAttributes is considered to return signed or unsigned data. In particular, I think it's actually going to return a signed int in this context (http://docs.python.org/2/library/ctypes.html#return-types) https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:70: def CalculateHash(root): On 2014/01/14 17:09:35, scottmg wrote: > On 2014/01/14 10:14:11, iannucci wrote: > > We may want to (eventually) integrate this with > > depot_tools/download_from_google_storage.py, which does a very similar thing. > In > > fact, the format is pretty close to identical (i.e. filename.sha1 whose > contents > > is the sha1 to find+download+place at filename). Actually it's close enough > that > > I think it may actually cause problems. I think some of the bots run > > download_from_google_storage over depot_tools, and they may see your sha1 > file. > > OK. > > I don't think we can mirror VS for non-Googlers, so I can't just download it > unfortunately. I guess I'll rename this one to .hash. Hm that /is/ unfortunate... We could maybe put it in a different bucket which needs moar better creds or something. Anyway, this logic should be merged at some point. You've been warned :) https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:145: subprocess.check_call('rmdir /s/q "%s"' % target_dir, shell=True) On 2014/01/14 17:09:36, scottmg wrote: > On 2014/01/14 10:14:11, iannucci wrote: > > [Ew... I know this is win-only and all, but can't we at least use shutil where > > it works?] > > shutil.rmtree sucks; it aborts if there's any files marked read only. *sigh* why is everything terrible? https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... win_toolchain/toolchain2013.py:35: raise SystemExit('%s failed.' % command) On 2014/01/14 17:09:36, scottmg wrote: > On 2014/01/14 10:14:11, iannucci wrote: > > Isn't this just subprocess.check_call? > > Done. I was more meaning... inline RunOrDie? https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... win_toolchain/toolchain2013.py:44: g_temp_dirs.append(temp) On 2014/01/14 17:09:36, scottmg wrote: > On 2014/01/14 10:14:11, iannucci wrote: > > since you're not actually assigning g_temp_dirs, you don't need to global it > > (yeah, I know, python is silly, non-constable object references are evil, etc. > > etc.) > > Yeah, it feels a bit nicer to make it obvious that it's not local to me. But I > can change it if you object. 'sfine
On 2014/01/15 21:55:26, iannucci wrote: > lgtm now tell jschuh to leave me alone :P Sorry, I know you're busy with many things. But I will get him to send the dog next time. > > https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... > File win_toolchain/get_toolchain_if_necessary.py (right): > > https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... > win_toolchain/get_toolchain_if_necessary.py:51: assert p != 0xffffffff > On 2014/01/14 17:09:35, scottmg wrote: > > On 2014/01/14 10:14:11, iannucci wrote: > > > Is this the same as (p != -1)? > > > > I would assume not in python? > > Well... it depends if GetFileAttributes is considered to return signed or > unsigned data. In particular, I think it's actually going to return a signed int > in this context (http://docs.python.org/2/library/ctypes.html#return-types) [auto-win-toolchain]d:\src\depot_tools>python Python 2.7.6 (default, Nov 10 2013, 19:24:18) [MSC v.1500 32 bit (Intel)] on win32 Type "help", "copyright", "credits" or "license" for more information. >>> import ctypes.wintypes >>> GetFileAttributes = ctypes.windll.kernel32.GetFileAttributesW >>> GetFileAttributes.argtypes = (ctypes.wintypes.LPWSTR,) >>> GetFileAttributes.restype = ctypes.wintypes.DWORD >>> FILE_ATTRIBUTE_HIDDEN = 0x2 >>> FILE_ATTRIBUTE_SYSTEM = 0x4 >>> >>> >>> def IsHidden(file_path): ... """Returns whether the given |file_path| has the 'system' or 'hidden' ... attribute set.""" ... p = GetFileAttributes(file_path) ... assert p != 0xffffffff ... return bool(p & (FILE_ATTRIBUTE_HIDDEN | FILE_ATTRIBUTE_SYSTEM)) ... >>> IsHidden('does_not_exist') Traceback (most recent call last): File "<stdin>", line 1, in <module> File "<stdin>", line 5, in IsHidden AssertionError >>> def IsHidden(file_path): ... """Returns whether the given |file_path| has the 'system' or 'hidden' ... attribute set.""" ... p = GetFileAttributes(file_path) ... assert p != -1 ... return bool(p & (FILE_ATTRIBUTE_HIDDEN | FILE_ATTRIBUTE_SYSTEM)) ... >>> IsHidden('does_not_exist') True >>> Magic, magic, everywhere! > > https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... > win_toolchain/get_toolchain_if_necessary.py:70: def CalculateHash(root): > On 2014/01/14 17:09:35, scottmg wrote: > > On 2014/01/14 10:14:11, iannucci wrote: > > > We may want to (eventually) integrate this with > > > depot_tools/download_from_google_storage.py, which does a very similar > thing. > > In > > > fact, the format is pretty close to identical (i.e. filename.sha1 whose > > contents > > > is the sha1 to find+download+place at filename). Actually it's close enough > > that > > > I think it may actually cause problems. I think some of the bots run > > > download_from_google_storage over depot_tools, and they may see your sha1 > > file. > > > > OK. > > > > I don't think we can mirror VS for non-Googlers, so I can't just download it > > unfortunately. I guess I'll rename this one to .hash. > > Hm that /is/ unfortunate... We could maybe put it in a different bucket which > needs moar better creds or something. Anyway, this logic should be merged at > some point. You've been warned :) Got it. It would be super-fantastic if I could just build a pristine image and then rsync it to everyone rather than having to all this. clang, I guess. > > https://codereview.chromium.org/135933002/diff/340009/win_toolchain/get_toolc... > win_toolchain/get_toolchain_if_necessary.py:145: subprocess.check_call('rmdir > /s/q "%s"' % target_dir, shell=True) > On 2014/01/14 17:09:36, scottmg wrote: > > On 2014/01/14 10:14:11, iannucci wrote: > > > [Ew... I know this is win-only and all, but can't we at least use shutil > where > > > it works?] > > > > shutil.rmtree sucks; it aborts if there's any files marked read only. > > *sigh* why is everything terrible? > > https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... > File win_toolchain/toolchain2013.py (right): > > https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... > win_toolchain/toolchain2013.py:35: raise SystemExit('%s failed.' % command) > On 2014/01/14 17:09:36, scottmg wrote: > > On 2014/01/14 10:14:11, iannucci wrote: > > > Isn't this just subprocess.check_call? > > > > Done. > > I was more meaning... inline RunOrDie? I figured. :P I'd prefer to keep it so that I can insert a print(command) more easily when things are going pear-shaped. > > https://codereview.chromium.org/135933002/diff/340009/win_toolchain/toolchain... > win_toolchain/toolchain2013.py:44: g_temp_dirs.append(temp) > On 2014/01/14 17:09:36, scottmg wrote: > > On 2014/01/14 10:14:11, iannucci wrote: > > > since you're not actually assigning g_temp_dirs, you don't need to global it > > > (yeah, I know, python is silly, non-constable object references are evil, > etc. > > > etc.) > > > > Yeah, it feels a bit nicer to make it obvious that it's not local to me. But I > > can change it if you object. > > 'sfine
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/135933002/540001
Can't process patch for file win_toolchain/7z/7z.dll. Binary file is empty. Maybe the file wasn't uploaded in the first place?
On 2014/01/15 22:03:10, I haz the power (commit-bot) wrote: > Can't process patch for file win_toolchain/7z/7z.dll. > Binary file is empty. Maybe the file wasn't uploaded in the first place? (oh rietveld, you so silly)
Message was sent while issue was closed.
Committed patchset #8 manually as r244983.
Message was sent while issue was closed.
On 2014/01/15 22:03:48, iannucci wrote: > On 2014/01/15 22:03:10, I haz the power (commit-bot) wrote: > > Can't process patch for file win_toolchain/7z/7z.dll. > > Binary file is empty. Maybe the file wasn't uploaded in the first place? > > (oh rietveld, you so silly) Nothing that git cl dcommit --bypass-hooks can't fix. (Should I even attempt to fix the presubmit tests on Windows or are they totally-Linux/Mac?)
Message was sent while issue was closed.
On 2014/01/15 22:06:10, scottmg wrote: > On 2014/01/15 22:03:48, iannucci wrote: > > On 2014/01/15 22:03:10, I haz the power (commit-bot) wrote: > > > Can't process patch for file win_toolchain/7z/7z.dll. > > > Binary file is empty. Maybe the file wasn't uploaded in the first place? > > > > (oh rietveld, you so silly) > > Nothing that git cl dcommit --bypass-hooks can't fix. > > (Should I even attempt to fix the presubmit tests on Windows or are they > totally-Linux/Mac?) You didn't really touch anything which needed presubmit, so it's probably fine................. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
