make `python build/vs_toolchain.py update` work a bit better on non-windows

make `python build/vs_toolchain.py update` work a bit better on non-windows

It still early-exits on non-Windows, so no visible change yet.

BUG=495204
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=295807

[Nico, 2015-06-12 05:29:04]

thakis@chromium.org changed reviewers:
+ scottmg@chromium.org

[Nico, 2015-06-12 05:29:05]

This is a request for advice, not a review request. I'm trying to download the
toolchain on a non-windows host. The toolchain hash includes some "IsHidden"
state which can't be computed on non-Windows from what I can tell. Is it
important that this is part of the hash? Could hidden files just be hashed as
well? Other ideas?

https://codereview.chromium.org/1181943003/diff/1/win_toolchain/get_toolchain...
File win_toolchain/get_toolchain_if_necessary.py (right):

https://codereview.chromium.org/1181943003/diff/1/win_toolchain/get_toolchain...
win_toolchain/get_toolchain_if_necessary.py:55: GetFileAttributes.restype =
ctypes.wintypes.DWORD
This won't work on non-win…

https://codereview.chromium.org/1181943003/diff/1/win_toolchain/get_toolchain...
win_toolchain/get_toolchain_if_necessary.py:63: return False
…so I have to do this, which means the hashes will never match and I have to
download 500 MB each time I run `python build/vs_toolchain.py update`

[scottmg, 2015-06-12 16:59:13]

Good stuff, would be nice for this to work.

https://codereview.chromium.org/1181943003/diff/1/win_toolchain/get_toolchain...
File win_toolchain/get_toolchain_if_necessary.py (right):

https://codereview.chromium.org/1181943003/diff/1/win_toolchain/get_toolchain...
win_toolchain/get_toolchain_if_necessary.py:63: return False
On 2015/06/12 05:29:05, Nico (afk vacation Fri Jun 12) wrote:
> …so I have to do this, which means the hashes will never match and I have to
> download 500 MB each time I run `python build/vs_toolchain.py update`

I had forgotten why. The long ago rationale was here:

https://codereview.chromium.org/95983002#msg5

It doesn't seem that important, but maybe occasionally confusing for a Windows
user that pokes around in that directory and then it gets nuked.

But otoh, 

d:\src\depot_tools\win_toolchain\vs_files>attrib /s | grep -v "A            "

returned nothing. So, I'm not sure. Maybe I'm unzipping wrong as of recently (I
changed from 7z to python zipfile) or maybe there's none anyway? You're sure
this is why the hash doesn't match?

https://codereview.chromium.org/1181943003/diff/1/win_toolchain/get_toolchain...
win_toolchain/get_toolchain_if_necessary.py:72: assert os.path.normpath(root) ==
root
It seems like posix.normpath vs. nt.normpath might cause problems too?

https://codereview.chromium.org/1181943003/diff/1/win_toolchain/get_toolchain...
win_toolchain/get_toolchain_if_necessary.py:220: shutil.rmtree(temp_dir,
ignore_errors=True)
This function is terrible, and fails for files marked readonly on Windows, which
there are (were?) in the toolchain. It's possible only with the
old-Express-from-bits path, not the google extraction path though.

[Nico, 2015-06-12 17:04:29]

Thanks for the suggestions! I'll ping this again once I've looked at this some
more.

https://codereview.chromium.org/1181943003/diff/1/win_toolchain/get_toolchain...
File win_toolchain/get_toolchain_if_necessary.py (right):

https://codereview.chromium.org/1181943003/diff/1/win_toolchain/get_toolchain...
win_toolchain/get_toolchain_if_necessary.py:63: return False
On 2015/06/12 16:59:12, scottmg wrote:
> On 2015/06/12 05:29:05, Nico (afk vacation Fri Jun 12) wrote:
> > …so I have to do this, which means the hashes will never match and I have to
> > download 500 MB each time I run `python build/vs_toolchain.py update`
> 
> I had forgotten why. The long ago rationale was here:
> 
> https://codereview.chromium.org/95983002#msg5
> 
> It doesn't seem that important, but maybe occasionally confusing for a Windows
> user that pokes around in that directory and then it gets nuked.
> 
> But otoh, 
> 
> d:\src\depot_tools\win_toolchain\vs_files>attrib /s | grep -v "A            "
> 
> returned nothing. So, I'm not sure. Maybe I'm unzipping wrong as of recently
(I
> changed from 7z to python zipfile) or maybe there's none anyway? You're sure
> this is why the hash doesn't match?

No, I just saw that the hash didn't match and figured it's probably this. I'll
check if running the update command on my Windows box tries to redownload if I
always return False here when I'm back near a Windows box (monday)

https://codereview.chromium.org/1181943003/diff/1/win_toolchain/get_toolchain...
win_toolchain/get_toolchain_if_necessary.py:220: shutil.rmtree(temp_dir,
ignore_errors=True)
On 2015/06/12 16:59:12, scottmg wrote:
> This function is terrible, and fails for files marked readonly on Windows,
which
> there are (were?) in the toolchain. It's possible only with the
> old-Express-from-bits path, not the google extraction path though.

TIL! Could the onerror handler in the first reply in
http://stackoverflow.com/questions/2656322/python-shutil-rmtree-fails-on-wind...
work?

[scottmg, 2015-06-12 17:13:56]

https://codereview.chromium.org/1181943003/diff/1/win_toolchain/get_toolchain...
File win_toolchain/get_toolchain_if_necessary.py (right):

https://codereview.chromium.org/1181943003/diff/1/win_toolchain/get_toolchain...
win_toolchain/get_toolchain_if_necessary.py:220: shutil.rmtree(temp_dir,
ignore_errors=True)
On 2015/06/12 17:04:29, Nico (afk vacation Fri Jun 12) wrote:
> On 2015/06/12 16:59:12, scottmg wrote:
> > This function is terrible, and fails for files marked readonly on Windows,
> which
> > there are (were?) in the toolchain. It's possible only with the
> > old-Express-from-bits path, not the google extraction path though.
> 
> TIL! Could the onerror handler in the first reply in
>
http://stackoverflow.com/questions/2656322/python-shutil-rmtree-fails-on-wind...
> work?

Looks like it, but it seems longer than an if win: rmdir else: rmtree function.
/shrug

[Dirk Pranke, 2015-06-12 20:14:42]

Yeah, I seem to recall rmtree is supposed to be dodgy on windows as well, but
looking at
the code in webkitpy.common.system.filesystem we use it anyway, so it can't be
that dodgy.

[scottmg, 2015-06-12 20:26:42]

https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/gcli...

https://code.google.com/p/chromium/codesearch#chromium/src/build/get_syzygy_b...

https://code.google.com/p/chromium/codesearch#chromium/src/tools/deps2git/dep...

https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...


I think it's more like Blink and Infra tools get away with mostly ignoring
the annoyance of having to run in the wild woolly world of Windows.

https://code.google.com/p/chromium/codesearch#chromium/infra/go/bootstrap.py&...

https://code.google.com/p/chromium/codesearch#chromium/infra/glyco/glucose/ut...

https://code.google.com/p/chromium/codesearch#chromium/infra/node/node.py&l=38


Also, code reuse is hard. :/


On Fri, Jun 12, 2015 at 1:14 PM, <dpranke@chromium.org> wrote:

> Yeah, I seem to recall rmtree is supposed to be dodgy on windows as well,
> but
> looking at
> the code in webkitpy.common.system.filesystem we use it anyway, so it
> can't be
> that dodgy.
>
>
> https://codereview.chromium.org/1181943003/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2015-06-23 20:34:53]

ptal

I haven't figured out why the hash is different yet.

[scottmg, 2015-06-23 20:37:46]

lgtm

https://codereview.chromium.org/1181943003/diff/120001/win_toolchain/get_tool...
File win_toolchain/get_toolchain_if_necessary.py (right):

https://codereview.chromium.org/1181943003/diff/120001/win_toolchain/get_tool...
win_toolchain/get_toolchain_if_necessary.py:276: # This stays resident and will
make the rmdir below fail.
maybe move this comment inside the if win32.

[Nico, 2015-06-23 21:01:17]

Thanks!

https://codereview.chromium.org/1181943003/diff/120001/win_toolchain/get_tool...
File win_toolchain/get_toolchain_if_necessary.py (right):

https://codereview.chromium.org/1181943003/diff/120001/win_toolchain/get_tool...
win_toolchain/get_toolchain_if_necessary.py:276: # This stays resident and will
make the rmdir below fail.
On 2015/06/23 20:37:46, scottmg wrote:
> maybe move this comment inside the if win32.

Done.

[Nico, 2015-06-23 21:01:30]

The CQ bit was checked by thakis@chromium.org

[Nico, 2015-06-23 21:01:30]

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/1181943003/#ps140001
(title: ".")

[commit-bot: I haz the power, 2015-06-23 21:01:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181943003/140001

[commit-bot: I haz the power, 2015-06-23 21:04:04]

Committed patchset #8 (id:140001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=295807