Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2039563002:
Make "vs_toolchain.py update" work on case-sensitive file systems (Closed)
|
Created:
4 years, 6 months ago by hans Modified:
4 years, 6 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org, Sébastien Marchand, brucedawson Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionMake "vs_toolchain.py update" work on case-sensitive file systems
Filenames must have their case preserved so the files can be opened,
but they need to be lowercased when hashed and sorted, to match the
expected hash.
In other words, this patch moves the lower-casing from GetFileList
(except to sort the list) to where filenames are fed into the hash.
BUG=495204
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300696
Patch Set 1 #
Total comments: 3
Patch Set 2 : Ignore ignored_directories on non-Win #
Total comments: 2
Patch Set 3 : Windows hosts still need p to be lowered #
Total comments: 2
Messages
Total messages: 28 (8 generated)
hans@chromium.org changed reviewers: + thakis@chromium.org
I tried to play with https://codereview.chromium.org/1183633003/ on Linux, and wanted to see how far I could get without creating a case-insensitive file system. This let's me at least get the toolchain. I'm curious how much further we can get.
hans@chromium.org changed reviewers: + scottmg@chromium.org
+scottmg too
nice, lgtm https://codereview.chromium.org/2039563002/diff/1/win_toolchain/get_toolchain... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/2039563002/diff/1/win_toolchain/get_toolchain... win_toolchain/get_toolchain_if_necessary.py:78: # to store the symbol files. You could add a "# Note: These files are only created on a Windows host, so the ignored_directories list isn't relevant on non-Windows hosts" and remove the /->\ transform below, I think.
https://codereview.chromium.org/2039563002/diff/1/win_toolchain/get_toolchain... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/2039563002/diff/1/win_toolchain/get_toolchain... win_toolchain/get_toolchain_if_necessary.py:78: # to store the symbol files. On 2016/06/03 18:44:46, Nico wrote: > You could add a "# Note: These files are only created on a Windows host, so the > ignored_directories list isn't relevant on non-Windows hosts" and remove the > /->\ transform below, I think. I figured this is why you hadn't done this on Mac, but couldn't some of these files potentially get created if some VS tool is run under wine or something?
https://codereview.chromium.org/2039563002/diff/1/win_toolchain/get_toolchain... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/2039563002/diff/1/win_toolchain/get_toolchain... win_toolchain/get_toolchain_if_necessary.py:78: # to store the symbol files. On 2016/06/03 18:47:32, hans wrote: > On 2016/06/03 18:44:46, Nico wrote: > > You could add a "# Note: These files are only created on a Windows host, so > the > > ignored_directories list isn't relevant on non-Windows hosts" and remove the > > /->\ transform below, I think. > > I figured this is why you hadn't done this on Mac, but couldn't some of these > files potentially get created if some VS tool is run under wine or something? The comment says cl (which we don't run in cross-builds) and windbg. I think it's fairly unlikely that either is run under wine, but what you have is fine too if you prefer to leave it as is
On 2016/06/03 18:56:00, Nico wrote: > https://codereview.chromium.org/2039563002/diff/1/win_toolchain/get_toolchain... > File win_toolchain/get_toolchain_if_necessary.py (right): > > https://codereview.chromium.org/2039563002/diff/1/win_toolchain/get_toolchain... > win_toolchain/get_toolchain_if_necessary.py:78: # to store the symbol files. > On 2016/06/03 18:47:32, hans wrote: > > On 2016/06/03 18:44:46, Nico wrote: > > > You could add a "# Note: These files are only created on a Windows host, so > > the > > > ignored_directories list isn't relevant on non-Windows hosts" and remove the > > > /->\ transform below, I think. > > > > I figured this is why you hadn't done this on Mac, but couldn't some of these > > files potentially get created if some VS tool is run under wine or something? > > The comment says cl (which we don't run in cross-builds) and windbg. I think > it's fairly unlikely that either is run under wine, but what you have is fine > too if you prefer to leave it as is I was worried I could get my win_toolchain dir into a bad state if I happened to somehow generate one of those files, but now I realize that would just cause vs_toolchain.py to nuke and re-download, so that's fine. Removing my change and adding a note. scottmg: please take an owner's look when you're around.
https://codereview.chromium.org/2039563002/diff/20001/win_toolchain/get_toolc... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/2039563002/diff/20001/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:89: if any(ignored_dir in p for ignored_dir in ignored_directories): does this need a p.lower()?
https://codereview.chromium.org/2039563002/diff/20001/win_toolchain/get_toolc... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/2039563002/diff/20001/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:89: if any(ignored_dir in p for ignored_dir in ignored_directories): On 2016/06/03 19:57:41, Nico wrote: > does this need a p.lower()? Oh, good catch. Done.
scottmg@chromium.org changed reviewers: + brucedawson@chromium.org
Isn't this going to modify the hash of already downloaded toolchains?
On 2016/06/06 15:50:40, scottmg wrote: > Isn't this going to modify the hash of already downloaded toolchains? It shouldn't (unless you're seeming something that I'm missing?) The hash is currently based on the lowercase filenames, and it is after my change too. This change just makes us not lowercase the filename used for actually opening the file, as that doesn't work on Linux.
https://codereview.chromium.org/2039563002/diff/40001/win_toolchain/get_toolc... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/2039563002/diff/40001/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:165: digest.update(path_without_hash.lower()) I don't really remember what the data looks like now, but you're saying this lower() is unnecessary then? This looks like it's only used to calculate the digest, so best case it does nothing, worst case it changes the hashes, right?
lgtm https://codereview.chromium.org/2039563002/diff/40001/win_toolchain/get_toolc... File win_toolchain/get_toolchain_if_necessary.py (right): https://codereview.chromium.org/2039563002/diff/40001/win_toolchain/get_toolc... win_toolchain/get_toolchain_if_necessary.py:165: digest.update(path_without_hash.lower()) On 2016/06/06 16:39:22, scottmg wrote: > I don't really remember what the data looks like now, but you're saying this > lower() is unnecessary then? This looks like it's only used to calculate the > digest, so best case it does nothing, worst case it changes the hashes, right? Oh, I missed the old lower(). I'm probably not the right reviewer for this any more :(
Description was changed from ========== Make "vs_toolchain.py update" work on case-sensitive file systems Filenames must have their case preserved so the files can be opened, but they need to be lowercased when hashed and sorted, to match the expected hash. BUG=495204 ========== to ========== Make "vs_toolchain.py update" work on case-sensitive file systems Filenames must have their case preserved so the files can be opened, but they need to be lowercased when hashed and sorted, to match the expected hash. In other words, this patch moves the lower-casing from GetFileList (except to sort the list) to where filenames are fed into the hash. BUG=495204 ==========
On 2016/06/06 16:40:56, scottmg wrote: > lgtm > > https://codereview.chromium.org/2039563002/diff/40001/win_toolchain/get_toolc... > File win_toolchain/get_toolchain_if_necessary.py (right): > > https://codereview.chromium.org/2039563002/diff/40001/win_toolchain/get_toolc... > win_toolchain/get_toolchain_if_necessary.py:165: > digest.update(path_without_hash.lower()) > On 2016/06/06 16:39:22, scottmg wrote: > > I don't really remember what the data looks like now, but you're saying this > > lower() is unnecessary then? This looks like it's only used to calculate the > > digest, so best case it does nothing, worst case it changes the hashes, right? > > Oh, I missed the old lower(). I'm probably not the right reviewer for this any > more :( No worries, I should have pointed to it more clearly. I've updated the CL comment to hopefully make it more clear for anyone else who might stumble across it later.
The CQ bit was checked by hans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2039563002/#ps40001 (title: "Windows hosts still need p to be lowered")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2039563002/40001
Message was sent while issue was closed.
Description was changed from ========== Make "vs_toolchain.py update" work on case-sensitive file systems Filenames must have their case preserved so the files can be opened, but they need to be lowercased when hashed and sorted, to match the expected hash. In other words, this patch moves the lower-casing from GetFileList (except to sort the list) to where filenames are fed into the hash. BUG=495204 ========== to ========== Make "vs_toolchain.py update" work on case-sensitive file systems Filenames must have their case preserved so the files can be opened, but they need to be lowercased when hashed and sorted, to match the expected hash. In other words, this patch moves the lower-casing from GetFileList (except to sort the list) to where filenames are fed into the hash. BUG=495204 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300696 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300696
Message was sent while issue was closed.
sebmarchand@chromium.org changed reviewers: + sebmarchand@chromium.org
Message was sent while issue was closed.
The 'vs_toolchain_wrapper.py update' hook now takes between 20 and 50 secs each time I call gclient runhooks, I've tried locally reverting this CL and it fixed the issue. I didn't had time to look at this in detail yet but I was just wondering if you've also observed this ?
Message was sent while issue was closed.
On 2016/06/08 16:13:32, Sébastien Marchand wrote: > The 'vs_toolchain_wrapper.py update' hook now takes between 20 and 50 secs each > time I call gclient runhooks, I've tried locally reverting this CL and it fixed > the issue. I didn't had time to look at this in detail yet but I was just > wondering if you've also observed this ? I didn't notice that, but my checkout might not have been in a vanilla state since i was working on this. Does it print the "Calculating hash of toolchain" message each time? The hash is supposed to get cached, but maybe my patch broke that somehow.
Message was sent while issue was closed.
Yep, it says 'Calculating hash of toolchain' each time. I'll try to debug this locally (I've touched most of this code recently so I'm familiar with it).
Message was sent while issue was closed.
On 2016/06/08 16:25:37, Sébastien Marchand wrote: > Yep, it says 'Calculating hash of toolchain' each time. I'll try to debug this > locally (I've touched most of this code recently so I'm familiar with it). Thanks! Maybe the filenames get lowercased when written to the file, but are then compared against the non-lowercased names? Or maybe the new file isn't getting written or something? Anyway, if you don't find anything we can just revert this and I'll look into it later.
Message was sent while issue was closed.
https://codereview.chromium.org/2052533002 should fix this. |
