Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
70493006:
Auto-updating script for Windows toolchain (Closed)
|
Created:
7 years, 1 month ago by scottmg Modified:
7 years ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAuto-updating script for Windows toolchain
Updates third_party/win_toolchain to pull down a toolchain
based on SHA1. The intention is that this will be a DEPS hook
once it's ready for more broad usage and the rest of
infrastructure is ready.
R=maruel@chromium.org
TBR=cpu@chromium.org
BUG=323300
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237369
Patch Set 1 : . #Patch Set 2 : more noise in download to keep runhooks happy #Patch Set 3 : nicer output #
Total comments: 15
Patch Set 4 : fixes #Patch Set 5 : remove --local debugging #
Total comments: 8
Patch Set 6 : fixes #
Total comments: 10
Patch Set 7 : . #
Total comments: 6
Patch Set 8 : . #Patch Set 9 : remove DEPS #
Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... File tools/win/toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:12: rc = subprocess.call(command, shell=True) that's called subprocess.check_call() https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:23: for name in files: I'm not sure the files are listed in deterministic orders or if the directories are traversed in deterministic order. You can fix this with: dirs.sort() for name in sorted(files): ... Then there's issue with path casing. You could .lower() everything. And you are including 'root' in sha-1 calculation. You should base it at the relative directory instead. Finally, the code will generate different hash if the input is given '/' or '\\'. https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:27: digest.update(f.read()) Make this a function that reads chunks. It is necessary to support large files anyway, then you can reuse the function for line 42. https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:34: if len(sys.argv) != 1: print >> sys.stderr, 'Unsupported arguments' return 1 https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:36: toolchain_dir = 'src/third_party/win_toolchain' os.path.join() https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:55: sys.executable, This shouldn't be run under shell=True. https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:56: 'src/tools/win/toolchain/toolchain2013.py', os.path.join() also, you assume about the current working directory. https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:61: raise SystemExit( print >> sys.stderr, 'Got ...' return 1
https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... File tools/win/toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:12: rc = subprocess.call(command, shell=True) On 2013/11/26 02:07:43, M-A Ruel wrote: > that's called subprocess.check_call() Done. https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:23: for name in files: On 2013/11/26 02:07:43, M-A Ruel wrote: Good points, thanks. > I'm not sure the files are listed in deterministic orders or if the directories > are traversed in deterministic order. > > You can fix this with: > dirs.sort() > for name in sorted(files): > ... Done. > Then there's issue with path casing. You could .lower() everything. Done, and before sorted() too, in case the file case is inconsistent on disk. > > And you are including 'root' in sha-1 calculation. You should base it at the > relative directory instead. The root is the path that's passed to os.walk, so 'src/third_party/win_toolchain/...' I think that's right in case we want to move it. > > Finally, the code will generate different hash if the input is given '/' or > '\\'. Done. https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:27: digest.update(f.read()) On 2013/11/26 02:07:43, M-A Ruel wrote: > Make this a function that reads chunks. It is necessary to support large files > anyway, then you can reuse the function for line 42. The largest file is 42M, so it seems unnecessary. https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:34: On 2013/11/26 02:07:43, M-A Ruel wrote: > if len(sys.argv) != 1: > print >> sys.stderr, 'Unsupported arguments' > return 1 Done. https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:36: toolchain_dir = 'src/third_party/win_toolchain' On 2013/11/26 02:07:43, M-A Ruel wrote: > os.path.join() I'm not sure what you mean. This script is a DEPS helper, so it only makes sense to run with cwd as the location .gclient. https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:55: sys.executable, On 2013/11/26 02:07:43, M-A Ruel wrote: > This shouldn't be run under shell=True. Done. https://codereview.chromium.org/70493006/diff/70001/tools/win/toolchain/get_t... tools/win/toolchain/get_toolchain_if_necessary.py:61: raise SystemExit( On 2013/11/26 02:07:43, M-A Ruel wrote: > print >> sys.stderr, 'Got ...' > return 1 Done.
https://codereview.chromium.org/70493006/diff/110001/DEPS File DEPS (right): https://codereview.chromium.org/70493006/diff/110001/DEPS#newcode647 DEPS:647: "pattern": "\\.sha1$", third_party/win_toolchain/toolchain\\.sha1 https://codereview.chromium.org/70493006/diff/110001/tools/win/toolchain/get_... File tools/win/toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/70493006/diff/110001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:19: for name in sorted(files_lower): for name in sorted(f.lower() for f in files): https://codereview.chromium.org/70493006/diff/110001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:35: # cwd is assumed to be where .gclient is. s/<comment>/os.chdir(<path>)/ The first thing people will do is to try to call this script manually. https://codereview.chromium.org/70493006/diff/110001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:57: '--targetdir=' + target_dir]) '--targetdir', target_dir]) this increase the likelihood of working with path that requires escaping.
Thanks https://codereview.chromium.org/70493006/diff/110001/DEPS File DEPS (right): https://codereview.chromium.org/70493006/diff/110001/DEPS#newcode647 DEPS:647: "pattern": "\\.sha1$", On 2013/11/26 16:09:50, M-A Ruel wrote: > third_party/win_toolchain/toolchain\\.sha1 Done. https://codereview.chromium.org/70493006/diff/110001/tools/win/toolchain/get_... File tools/win/toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/70493006/diff/110001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:19: for name in sorted(files_lower): On 2013/11/26 16:09:50, M-A Ruel wrote: > for name in sorted(f.lower() for f in files): Done. https://codereview.chromium.org/70493006/diff/110001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:35: # cwd is assumed to be where .gclient is. On 2013/11/26 16:09:50, M-A Ruel wrote: > s/<comment>/os.chdir(<path>)/ > The first thing people will do is to try to call this script manually. Done. https://codereview.chromium.org/70493006/diff/110001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:57: '--targetdir=' + target_dir]) On 2013/11/26 16:09:50, M-A Ruel wrote: > '--targetdir', target_dir]) > > this increase the likelihood of working with path that requires escaping. Done.
https://codereview.chromium.org/70493006/diff/130001/third_party/win_toolchai... File third_party/win_toolchain/.gitignore (right): https://codereview.chromium.org/70493006/diff/130001/third_party/win_toolchai... third_party/win_toolchain/.gitignore:1: files/ It should be in the .gitignore two directories above. https://codereview.chromium.org/70493006/diff/130001/tools/win/toolchain/get_... File tools/win/toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/70493006/diff/130001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:11: BASEDIR = os.path.abspath(os.path.dirname(__file__)) BASEDIR = os.path.dirname(os.path.abspath(__file__)) order is important https://codereview.chromium.org/70493006/diff/130001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:24: digest.update(path.lower()) I'd prefer this to be outside of the with statement. https://codereview.chromium.org/70493006/diff/130001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:52: current_hash = CalculateHash(target_dir) How long does it take in practice on a spinning drive? If it is significant, it could be worth keeping a cache of the files timestamps.
https://codereview.chromium.org/70493006/diff/130001/third_party/win_toolchai... File third_party/win_toolchain/.gitignore (right): https://codereview.chromium.org/70493006/diff/130001/third_party/win_toolchai... third_party/win_toolchain/.gitignore:1: files/ On 2013/11/26 17:48:18, M-A Ruel wrote: > It should be in the .gitignore two directories above. Done. https://codereview.chromium.org/70493006/diff/130001/tools/win/toolchain/get_... File tools/win/toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/70493006/diff/130001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:11: BASEDIR = os.path.abspath(os.path.dirname(__file__)) On 2013/11/26 17:48:18, M-A Ruel wrote: > BASEDIR = os.path.dirname(os.path.abspath(__file__)) > > order is important Done. When does it matter? os.path.abspath('') == os.getcwd() https://codereview.chromium.org/70493006/diff/130001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:24: digest.update(path.lower()) On 2013/11/26 17:48:18, M-A Ruel wrote: > I'd prefer this to be outside of the with statement. Done. https://codereview.chromium.org/70493006/diff/130001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:52: current_hash = CalculateHash(target_dir) On 2013/11/26 17:48:18, M-A Ruel wrote: > How long does it take in practice on a spinning drive? If it is significant, it > could be worth keeping a cache of the files timestamps. Spinning disk, pssh. Can you even build chrome on one of those? It's about 8s on SSD. I guess worthwhile to cache.
https://codereview.chromium.org/70493006/diff/130001/tools/win/toolchain/get_... File tools/win/toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/70493006/diff/130001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:11: BASEDIR = os.path.abspath(os.path.dirname(__file__)) On 2013/11/26 17:57:54, scottmg wrote: > On 2013/11/26 17:48:18, M-A Ruel wrote: > > BASEDIR = os.path.dirname(os.path.abspath(__file__)) > > > > order is important > > Done. > > When does it matter? os.path.abspath('') == os.getcwd() but os.path.dirname(os.getcwd()) != os.path.dirname('') https://codereview.chromium.org/70493006/diff/130001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:52: current_hash = CalculateHash(target_dir) On 2013/11/26 17:57:54, scottmg wrote: > On 2013/11/26 17:48:18, M-A Ruel wrote: > > How long does it take in practice on a spinning drive? If it is significant, > it > > could be worth keeping a cache of the files timestamps. > > Spinning disk, pssh. Can you even build chrome on one of those? That's 90% of build slaves. > It's about 8s on SSD. I guess worthwhile to cache. If you could do a test on a cold spinning disk, it'd be appreciated. https://codereview.chromium.org/70493006/diff/150001/tools/win/toolchain/get_... File tools/win/toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/70493006/diff/150001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:16: contents of those files, and returns as a hex string.""" you may want to: assert not os.path.isabs(root) assert os.path.normpath(root) == root https://codereview.chromium.org/70493006/diff/150001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:22: path = os.path.normpath(os.path.join(root, name)) technically, the normpath() call here will always be a no-op as long as root is well formed. https://codereview.chromium.org/70493006/diff/150001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:40: target_dir = os.path.join(toolchain_dir, 'files') Note that target_dir will be 'src/third_party/win_toolchain\\files' I'm not super happy about that.
Running a timing on HDD now. https://codereview.chromium.org/70493006/diff/150001/tools/win/toolchain/get_... File tools/win/toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/70493006/diff/150001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:16: contents of those files, and returns as a hex string.""" On 2013/11/26 18:03:40, M-A Ruel wrote: > you may want to: > assert not os.path.isabs(root) > assert os.path.normpath(root) == root Done. https://codereview.chromium.org/70493006/diff/150001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:22: path = os.path.normpath(os.path.join(root, name)) On 2013/11/26 18:03:40, M-A Ruel wrote: > technically, the normpath() call here will always be a no-op as long as root is > well formed. Yes, I guess with the assert it's ok to remove. https://codereview.chromium.org/70493006/diff/150001/tools/win/toolchain/get_... tools/win/toolchain/get_toolchain_if_necessary.py:40: target_dir = os.path.join(toolchain_dir, 'files') On 2013/11/26 18:03:40, M-A Ruel wrote: > Note that target_dir will be > 'src/third_party/win_toolchain\\files' > > I'm not super happy about that. Normalized all paths to Windows style.
Code now formally lgtm, e.g. I expect it to be idempotent. I'm still concerned about performance but that can be fixed in a follow up, depending how bad it is or it is not.
On 2013/11/26 18:23:47, M-A Ruel wrote: > Code now formally lgtm, e.g. I expect it to be idempotent. > > I'm still concerned about performance but that can be fixed in a follow up, > depending how bad it is or it is not. After killing cache, HDD time is 46s. I'll take a look at doing a cache of timestamps to see how much it helps. I do like that this way is currently very simple (and hopefully not buggy). Hot cache on HDD or SSD is ~5s, but it doesn't seem likely we'll often get a very hot cache.
On 2013/11/26 18:30:58, scottmg wrote: > On 2013/11/26 18:23:47, M-A Ruel wrote: > > Code now formally lgtm, e.g. I expect it to be idempotent. > > > > I'm still concerned about performance but that can be fixed in a follow up, > > depending how bad it is or it is not. > > After killing cache, HDD time is 46s. I'll take a look at doing a cache of > timestamps to see how much it helps. I do like that this way is currently very > simple (and hopefully not buggy). > > Hot cache on HDD or SSD is ~5s, but it doesn't seem likely we'll often get a > very hot cache. Ok, so it's worth doing a cache of the directories entries, but I recommend to do this in a follow up CL.
On 2013/11/26 18:32:51, M-A Ruel wrote: > On 2013/11/26 18:30:58, scottmg wrote: > > On 2013/11/26 18:23:47, M-A Ruel wrote: > > > Code now formally lgtm, e.g. I expect it to be idempotent. > > > > > > I'm still concerned about performance but that can be fixed in a follow up, > > > depending how bad it is or it is not. > > > > After killing cache, HDD time is 46s. I'll take a look at doing a cache of > > timestamps to see how much it helps. I do like that this way is currently very > > simple (and hopefully not buggy). > > > > Hot cache on HDD or SSD is ~5s, but it doesn't seem likely we'll often get a > > very hot cache. > > Ok, so it's worth doing a cache of the directories entries, but I recommend to > do this in a follow up CL. OK, I'll land without DEPS for now, and follow up other required changes in future CLs. I'll note the next steps on the linked bug.
TBR cpu for .sha1 file added in third_party/win_toolchain.
Message was sent while issue was closed.
Committed patchset #9 manually as r237369 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
