Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1890053004:
Stop gyp_chromium prior to mass DLL copies on error (Closed)
DescriptionStop gyp_chromium prior to mass DLL copies on error
Also fix last-modified-time comparison code to handle minor float
differences and thus correctly not copy identical files (for some
reason there appears to be a ~1e-07 diff between last-modified-time
of copied files...?).
BUG=603603
TEST=No more copy output from gyp_chromium when copied DLLs are already
there or gyp_rc != 0 :-)
Committed: https://crrev.com/381d9f17b002b97ea4cf38a859d91fafd051c587
Cr-Commit-Position: refs/heads/master@{#387911}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove unecessary copies #
Total comments: 3
Messages
Total messages: 21 (10 generated)
gab@chromium.org changed reviewers: + scottmg@chromium.org
PTAL, thanks https://codereview.chromium.org/1890053004/diff/1/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/1890053004/diff/1/build/vs_toolchain.py#newco... build/vs_toolchain.py:161: os.stat(target).st_mtime != os.stat(source).st_mtime)): Seems to already be supposed to skip if files haven't changed but it doesn't..?
Thanks, lgtm https://codereview.chromium.org/1890053004/diff/1/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/1890053004/diff/1/build/vs_toolchain.py#newco... build/vs_toolchain.py:188: print 'Copying %s to %s...' % (ucrt_src_dir, target_dir) Looks like it was duplicated here. I would be happy if you delete that, it's too noisy now.
Description was changed from ========== Stop gyp_chromium prior to mass DLL copies on error BUG=603603 ========== to ========== Stop gyp_chromium prior to mass DLL copies on error Also fix last-modified-time comparison code to handle minor float differences and thus correctly not copy identical files (for some reason there appears to be a ~1e-07 diff between last-modified-time of copied files...?). BUG=603603 TEST=No more output from gyp_chromium when copied DLLs are already there or the script fails :-) ==========
Description was changed from ========== Stop gyp_chromium prior to mass DLL copies on error Also fix last-modified-time comparison code to handle minor float differences and thus correctly not copy identical files (for some reason there appears to be a ~1e-07 diff between last-modified-time of copied files...?). BUG=603603 TEST=No more output from gyp_chromium when copied DLLs are already there or the script fails :-) ========== to ========== Stop gyp_chromium prior to mass DLL copies on error Also fix last-modified-time comparison code to handle minor float differences and thus correctly not copy identical files (for some reason there appears to be a ~1e-07 diff between last-modified-time of copied files...?). BUG=603603 TEST=No more output from gyp_chromium when copied DLLs are already there or the gyp_rc != 0 :-) ==========
Description was changed from ========== Stop gyp_chromium prior to mass DLL copies on error Also fix last-modified-time comparison code to handle minor float differences and thus correctly not copy identical files (for some reason there appears to be a ~1e-07 diff between last-modified-time of copied files...?). BUG=603603 TEST=No more output from gyp_chromium when copied DLLs are already there or the gyp_rc != 0 :-) ========== to ========== Stop gyp_chromium prior to mass DLL copies on error Also fix last-modified-time comparison code to handle minor float differences and thus correctly not copy identical files (for some reason there appears to be a ~1e-07 diff between last-modified-time of copied files...?). BUG=603603 TEST=No more copy output from gyp_chromium when copied DLLs are already there or the gyp_rc != 0 :-) ==========
Description was changed from ========== Stop gyp_chromium prior to mass DLL copies on error Also fix last-modified-time comparison code to handle minor float differences and thus correctly not copy identical files (for some reason there appears to be a ~1e-07 diff between last-modified-time of copied files...?). BUG=603603 TEST=No more copy output from gyp_chromium when copied DLLs are already there or the gyp_rc != 0 :-) ========== to ========== Stop gyp_chromium prior to mass DLL copies on error Also fix last-modified-time comparison code to handle minor float differences and thus correctly not copy identical files (for some reason there appears to be a ~1e-07 diff between last-modified-time of copied files...?). BUG=603603 TEST=No more copy output from gyp_chromium when copied DLLs are already there or gyp_rc != 0 :-) ==========
Dug a little deeper and figured out why the last-modified-time comparison failed (float exact compare FTL). The script no longer generates bogus copies, woot :-)
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1890053004/#ps20001 (title: "Remove unecessary copies")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1890053004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1890053004/20001
Message was sent while issue was closed.
Description was changed from ========== Stop gyp_chromium prior to mass DLL copies on error Also fix last-modified-time comparison code to handle minor float differences and thus correctly not copy identical files (for some reason there appears to be a ~1e-07 diff between last-modified-time of copied files...?). BUG=603603 TEST=No more copy output from gyp_chromium when copied DLLs are already there or gyp_rc != 0 :-) ========== to ========== Stop gyp_chromium prior to mass DLL copies on error Also fix last-modified-time comparison code to handle minor float differences and thus correctly not copy identical files (for some reason there appears to be a ~1e-07 diff between last-modified-time of copied files...?). BUG=603603 TEST=No more copy output from gyp_chromium when copied DLLs are already there or gyp_rc != 0 :-) ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Stop gyp_chromium prior to mass DLL copies on error Also fix last-modified-time comparison code to handle minor float differences and thus correctly not copy identical files (for some reason there appears to be a ~1e-07 diff between last-modified-time of copied files...?). BUG=603603 TEST=No more copy output from gyp_chromium when copied DLLs are already there or gyp_rc != 0 :-) ========== to ========== Stop gyp_chromium prior to mass DLL copies on error Also fix last-modified-time comparison code to handle minor float differences and thus correctly not copy identical files (for some reason there appears to be a ~1e-07 diff between last-modified-time of copied files...?). BUG=603603 TEST=No more copy output from gyp_chromium when copied DLLs are already there or gyp_rc != 0 :-) Committed: https://crrev.com/381d9f17b002b97ea4cf38a859d91fafd051c587 Cr-Commit-Position: refs/heads/master@{#387911} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/381d9f17b002b97ea4cf38a859d91fafd051c587 Cr-Commit-Position: refs/heads/master@{#387911}
Message was sent while issue was closed.
https://codereview.chromium.org/1890053004/diff/20001/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/1890053004/diff/20001/build/vs_toolchain.py#n... build/vs_toolchain.py:163: abs(os.stat(target).st_mtime - os.stat(source).st_mtime) >= 0.01)): Gah! Why the heck is mtime a float anyway :/
Message was sent while issue was closed.
https://codereview.chromium.org/1890053004/diff/20001/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/1890053004/diff/20001/build/vs_toolchain.py#n... build/vs_toolchain.py:163: abs(os.stat(target).st_mtime - os.stat(source).st_mtime) >= 0.01)): On 2016/04/18 16:48:39, scottmg wrote: > Gah! Why the heck is mtime a float anyway :/ I know, right..! Casting to int() and doing strict equal also works if you prefer but I wasn't sure if the float resolution could ever be somewhat useful...
Message was sent while issue was closed.
On 2016/04/18 16:58:51, gab wrote: > https://codereview.chromium.org/1890053004/diff/20001/build/vs_toolchain.py > File build/vs_toolchain.py (right): > > https://codereview.chromium.org/1890053004/diff/20001/build/vs_toolchain.py#n... > build/vs_toolchain.py:163: abs(os.stat(target).st_mtime - > os.stat(source).st_mtime) >= 0.01)): > On 2016/04/18 16:48:39, scottmg wrote: > > Gah! Why the heck is mtime a float anyway :/ > > I know, right..! Casting to int() and doing strict equal also works if you > prefer but I wasn't sure if the float resolution could ever be somewhat > useful... Meh, seems fine either way. Thanks for digging into it.
Message was sent while issue was closed.
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1890053004/diff/20001/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/1890053004/diff/20001/build/vs_toolchain.py#n... build/vs_toolchain.py:163: abs(os.stat(target).st_mtime - os.stat(source).st_mtime) >= 0.01)): WTF? There's something weird going on. When I run: os.stat("PRESUBMIT.PY") it says st_mtime=1460399946L, but when I run: os.stat("PRESUBMIT.PY").st_mtime it says "1460399946.7608104" That inconsistency is disturbing - I don't understand it. Using the C stat() function gives the answer 1460399946, but using GetFileTime gives sub-second precision that roughly matches Python. So... It is right and proper for Python to use float (which is actually double) for the file times. But I don't understand why when I print the entire stat() result it says it is returning a long integer. And I don't understand why st_mtime varies when the file is copied. BTW, 1e7 is the resolution of FILETIME in Windows, so the error is one unit in the last place. Also, I don't know why my FILETIME results don't perfectly match Python's, although that may be because I didn't add timezone conversion code.
Message was sent while issue was closed.
On 2016/04/18 18:25:17, brucedawson wrote: > https://codereview.chromium.org/1890053004/diff/20001/build/vs_toolchain.py > File build/vs_toolchain.py (right): > > https://codereview.chromium.org/1890053004/diff/20001/build/vs_toolchain.py#n... > build/vs_toolchain.py:163: abs(os.stat(target).st_mtime - > os.stat(source).st_mtime) >= 0.01)): > WTF? There's something weird going on. When I run: > > os.stat("PRESUBMIT.PY") > > it says st_mtime=1460399946L, but when I run: > > os.stat("PRESUBMIT.PY").st_mtime > > it says "1460399946.7608104" > > That inconsistency is disturbing - I don't understand it. > > Using the C stat() function gives the answer 1460399946, but using GetFileTime > gives sub-second precision that roughly matches Python. > > So... > > It is right and proper for Python to use float (which is actually double) for > the file times. But I don't understand why when I print the entire stat() result > it says it is returning a long integer. And I don't understand why st_mtime > varies when the file is copied. > > BTW, 1e7 is the resolution of FILETIME in Windows, so the error is one unit in > the last place. > > Also, I don't know why my FILETIME results don't perfectly match Python's, > although that may be because I didn't add timezone conversion code. Hooboy: 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 os >>> x=os.stat('host.cc') >>> x.st_mtime 1461008092.2359645 >>> x.__str__() 'nt.stat_result(st_mode=33206, st_ino=0L, st_dev=0, st_nlink=0, st_uid=0, st_gid=0, st_size=3209L, st_atime=1461008092L, st_mtime=1461008092L, st_ctime=1461008092L)' >>> x.__repr__() 'nt.stat_result(st_mode=33206, st_ino=0L, st_dev=0, st_nlink=0, st_uid=0, st_gid=0, st_size=3209L, st_atime=1461008092L, st_mtime=1461008092L, st_ctime=1461008092L)' >>> getattr(x, 'st_mtime') 1461008092.2359645 >>> import stat >>> x[stat.ST_MTIME] 1461008092L >>> Should probably check out _stat.c or stat.c but I probably don't really want to know. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
