Add the possibility to keep several version of the VS toolchain.

Add the possibility to keep several version of the VS toolchain.

BUG=

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

[Sébastien Marchand, 2016-01-25 22:48:39]

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

[Sébastien Marchand, 2016-01-25 22:48:39]

What do you think of this approach ? This CL is really not finished (I still
need to implement the logic to limit the number of versions etc) but with this
approach the new structure directory is:
depot_tools/win_toolchain/vs(2013)_files/{hash}/Blah

Does it sound like a good approach ?

[scottmg, 2016-01-25 22:56:54]

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

[scottmg, 2016-01-25 22:56:54]

Yeah, that seems reasonable with a cap on how many get kept around.

Does the .timestamps file end up in a right (different) place?

[brucedawson, 2016-01-25 23:04:27]

On 2016/01/25 22:56:54, scottmg wrote:
> Yeah, that seems reasonable with a cap on how many get kept around.
> 
> Does the .timestamps file end up in a right (different) place?

I should have implemented something like this when experimenting with VS 2013
and VS 2015 packages - it would have made switching around faster.

The extra directory depth isn't great, but it still seems worthwhile. It makes
the current-hash more obvious.

Limiting the number of versions could be messy and may not be necessary. The
growth rate isn't particularly high. But if you do want to limit it then keep
updating a file with the last-used times in it, I guess?

[scottmg, 2016-01-25 23:07:31]

On 2016/01/25 23:04:27, brucedawson wrote:
> On 2016/01/25 22:56:54, scottmg wrote:
> > Yeah, that seems reasonable with a cap on how many get kept around.
> > 
> > Does the .timestamps file end up in a right (different) place?
> 
> I should have implemented something like this when experimenting with VS 2013
> and VS 2015 packages - it would have made switching around faster.
> 
> The extra directory depth isn't great, but it still seems worthwhile. It makes
> the current-hash more obvious.
> 
> Limiting the number of versions could be messy and may not be necessary. The
> growth rate isn't particularly high. But if you do want to limit it then keep
> updating a file with the last-used times in it, I guess?

Fair enough, we could punt on that part. It was mostly for bots that had disk
constraints, that may not be an issue any more.

Maybe touch just .timestamps for now so that we could clean out later if we
needed to?

I guess we could also just add some simple code to delete some
old-known-hashes-dirs too that we add to once they're totally irrelevant.

[Sébastien Marchand, 2016-01-25 23:09:42]

Thanks, I haven't looked at the .timestamp file yet so it probably end up
at the wrong place :)

I mostly wanted to make sure that the extra directory depth isn't a
blocker, I'll see what I can do to limit the number of versions (I don't
want to see a "why do I have 10 versions of the toolchain in depot tools"
thread on chromium-dev in 2 years)
On Jan 25, 2016 6:04 PM, <brucedawson@chromium.org> wrote:

> On 2016/01/25 22:56:54, scottmg wrote:
>
>> Yeah, that seems reasonable with a cap on how many get kept around.
>>
>
> Does the .timestamps file end up in a right (different) place?
>>
>
> I should have implemented something like this when experimenting with VS
> 2013
> and VS 2015 packages - it would have made switching around faster.
>
> The extra directory depth isn't great, but it still seems worthwhile. It
> makes
> the current-hash more obvious.
>
> Limiting the number of versions could be messy and may not be necessary.
> The
> growth rate isn't particularly high. But if you do want to limit it then
> keep
> updating a file with the last-used times in it, I guess?
>
>
> https://codereview.chromium.org/1634923002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scottmg, 2016-01-25 23:12:53]

This looks fine to me from what I can tell, but honestly it's very fiddly and
there's too much caching and state (well.. that's all it really is) so it'll
need manual testing to see if it does what you want.

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

https://codereview.chromium.org/1634923002/diff/1/win_toolchain/get_toolchain...
win_toolchain/get_toolchain_if_necessary.py:344: if len(desired_hash) == 0:
I don't think you'll get an empty args[0] here?

[Sébastien Marchand, 2016-01-27 21:35:20]

I've moved the timestamp to win_toolchain/{sha1}.timestamps (instead of just
win_toolchain/.timestamps), another option would be to store several toolchain
entries in the .timestamp file.

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

https://codereview.chromium.org/1634923002/diff/1/win_toolchain/get_toolchain...
win_toolchain/get_toolchain_if_necessary.py:344: if len(desired_hash) == 0:
On 2016/01/25 23:12:53, scottmg wrote:
> I don't think you'll get an empty args[0] here?

Moved this check earlier in the script.

[scottmg, 2016-01-27 21:46:12]

https://codereview.chromium.org/1634923002/diff/20001/win_toolchain/get_toolc...
File win_toolchain/get_toolchain_if_necessary.py (right):

https://codereview.chromium.org/1634923002/diff/20001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:85: def CalculateHash(root,
expected_hash=''):
Can we get rid of the default value for expected_hash here?

https://codereview.chromium.org/1634923002/diff/20001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:89: # Check whether we previously
saved timestamps in $root/../.timestamps. If
../{sha1}.timestamps

https://codereview.chromium.org/1634923002/diff/20001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:388:
DelayBeforeRemoving(toolchain_target_dir)
Can we can delete from line 388-400 now? It seems like we can never delete a
toolchain here now.

[Sébastien Marchand, 2016-01-28 21:59:34]

Patchset #3 (id:40001) has been deleted

[Sébastien Marchand, 2016-01-28 22:01:15]

I've added some logic to remove the versions of the toolchain that haven't been
used in the past 30 days and it seems to be working pretty well, I'll do more
test tomorrow. What do you think of this approach ?

https://codereview.chromium.org/1634923002/diff/20001/win_toolchain/get_toolc...
File win_toolchain/get_toolchain_if_necessary.py (right):

https://codereview.chromium.org/1634923002/diff/20001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:85: def CalculateHash(root,
expected_hash=''):
On 2016/01/27 21:46:12, scottmg wrote:
> Can we get rid of the default value for expected_hash here?

Done, it was here because it's used by vs_toolchain.py

https://codereview.chromium.org/1634923002/diff/20001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:89: # Check whether we previously
saved timestamps in $root/../.timestamps. If
On 2016/01/27 21:46:12, scottmg wrote:
> ../{sha1}.timestamps

Done.

https://codereview.chromium.org/1634923002/diff/20001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:388:
DelayBeforeRemoving(toolchain_target_dir)
On 2016/01/27 21:46:12, scottmg wrote:
> Can we can delete from line 388-400 now? It seems like we can never delete a
> toolchain here now.

Move to the RemoveUnusedToolchains function.

[scottmg, 2016-01-28 23:04:12]

https://codereview.chromium.org/1634923002/diff/60001/win_toolchain/get_toolc...
File win_toolchain/get_toolchain_if_necessary.py (right):

https://codereview.chromium.org/1634923002/diff/60001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:82: return os.path.join(root,
os.pardir, '%s.timestamps' % sha1 if sha1 else '')
Get rid of "if sha1 else ''".

https://codereview.chromium.org/1634923002/diff/60001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:123: str(path).replace('/',
'\\').replace(expected_hash, '').replace(
Can you trim expected_hash only from where you expect it to be at the beginning?

https://codereview.chromium.org/1634923002/diff/60001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:124: '\\\\', '\\'))
Where are the double \ coming from?

https://codereview.chromium.org/1634923002/diff/60001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:296:
valid_toolchains.append((os.stat(full_path).st_atime, d))
Hm, won't st_atime update for all of them every time you
CalculateToolchainHashes()?

https://codereview.chromium.org/1634923002/diff/60001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:309: toolchain_expiration_time = 60
* 60 * 24 * 30
I'm not sure about this heuristic now. What if a mostly-Mac dev hasn't built on
Windows in a while, will all toolchains be deleted? Maybe we could sort by date
and keep at least the most recent one?

[Sébastien Marchand, 2016-02-01 19:29:29]

I've switched to using the VS_VERSION file instead of the timestamp one (see my
comment about st_atime not being updated on my machine). This file seems to be
present for all the recent version of the toolchain ? (There's a comment about
it being not present in the previous version of the toolchain, are we still
supporting them ?)

https://codereview.chromium.org/1634923002/diff/60001/win_toolchain/get_toolc...
File win_toolchain/get_toolchain_if_necessary.py (right):

https://codereview.chromium.org/1634923002/diff/60001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:82: return os.path.join(root,
os.pardir, '%s.timestamps' % sha1 if sha1 else '')
On 2016/01/28 23:04:11, scottmg wrote:
> Get rid of "if sha1 else ''".

Done.

https://codereview.chromium.org/1634923002/diff/60001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:123: str(path).replace('/',
'\\').replace(expected_hash, '').replace(
On 2016/01/28 23:04:11, scottmg wrote:
> Can you trim expected_hash only from where you expect it to be at the
beginning?

Done.

https://codereview.chromium.org/1634923002/diff/60001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:124: '\\\\', '\\'))
On 2016/01/28 23:04:11, scottmg wrote:
> Where are the double \ coming from?

It was because I was just removing {hash} from {root}\{hash}\foo, so the
resulting path was {root}\\foo, I've fixed this.

https://codereview.chromium.org/1634923002/diff/60001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:296:
valid_toolchains.append((os.stat(full_path).st_atime, d))
On 2016/01/28 23:04:11, scottmg wrote:
> Hm, won't st_atime update for all of them every time you
> CalculateToolchainHashes()?

No, reading a file (via read()) doesn't seem to update it's timestamp, here's an
example:

-----------------------------------------------------------
import os
import time

filename = 'delete.me'
f = open(filename, 'w').close()
creation_time = os.path.getatime(filename)
print 'Creation time: %f' % creation_time
print 'Sleeping for one second.'
print 'Reading the file.'
time.sleep(1)
f = open(filename, 'r')
f.read()
f.close()
last_access_time = os.path.getatime(filename)
print 'Access time: %f (delta: %f)' % (last_access_time, last_access_time -
creation_time)
print 'Touching file.'
os.utime(filename, None)
last_access_time = os.path.getatime(filename)
print 'Access time (after touching): %f (delta: %f)' % (last_access_time,
last_access_time - creation_time)
os.remove(filename)
-----------------------------------------------------------

On Windows it gives me the following output:
Creation time: 1454351806.138276
Sleeping for one second.
Reading the file.
Access time: 1454351806.138276 (delta: 0.000000)
Touching file.
Access time (after touching): 1454351807.143000 (delta: 1.004724)

On Linux reading the file change its timestamp:
sebmarchand@sebmarchand0-l:~/src/tmp$ python py_access_time.py 
Creation time: 1454351758.208726
Sleeping for one second.
Reading the file.
Access time: 1454351759.208748 (delta: 1.000022)
Touching file.
Access time (after touching): 1454351759.208748 (delta: 1.000022)

It looks like the NtfsDisableLastAccessUpdate is set on my machine
(https://msdn.microsoft.com/en-us/library/ff794679(v=winembedded.60).aspx), I'm
not sure why so I'll try to find a better approach.

https://codereview.chromium.org/1634923002/diff/60001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:309: toolchain_expiration_time = 60
* 60 * 24 * 30
On 2016/01/28 23:04:11, scottmg wrote:
> I'm not sure about this heuristic now. What if a mostly-Mac dev hasn't built
on
> Windows in a while, will all toolchains be deleted? Maybe we could sort by
date
> and keep at least the most recent one?

No, I'm touching the timestamp file of the toolchain that has been asked for
when calling this script (the user provided sha1), so there should always be at
least one toolchain installed.

[brucedawson, 2016-02-01 22:50:53]

> It looks like the NtfsDisableLastAccessUpdate is set on my machine
> (https://msdn.microsoft.com/en-us/library/ff794679(v=winembedded.60).aspx),
I'm
> not sure why so I'll try to find a better approach.

Updating metadata every time you read from a file is pretty expensive so I'm not
surprised that it's disabled.

[scottmg, 2016-02-02 17:34:07]

lgtm

https://codereview.chromium.org/1634923002/diff/80001/win_toolchain/get_toolc...
File win_toolchain/get_toolchain_if_necessary.py (right):

https://codereview.chromium.org/1634923002/diff/80001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:299:
valid_toolchains.append((os.path.getatime(version_file), d))
Let's use mtime here because it's the one more commonly used for dependency
checks (and matches what we ignore above).

https://codereview.chromium.org/1634923002/diff/80001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:466: # Touch the version file, has
reading it isn't always enough to update its
has -> as

[Sébastien Marchand, 2016-02-02 18:37:26]

Thanks, should I take any particular precaution before committing this ? (I've
done a bunch of tests locally).

My plan is to commit it and then do a dummy CL in Chrome and make sure that the
TS stays green...

https://codereview.chromium.org/1634923002/diff/80001/win_toolchain/get_toolc...
File win_toolchain/get_toolchain_if_necessary.py (right):

https://codereview.chromium.org/1634923002/diff/80001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:299:
valid_toolchains.append((os.path.getatime(version_file), d))
On 2016/02/02 17:34:07, scottmg wrote:
> Let's use mtime here because it's the one more commonly used for dependency
> checks (and matches what we ignore above).

Done.

https://codereview.chromium.org/1634923002/diff/80001/win_toolchain/get_toolc...
win_toolchain/get_toolchain_if_necessary.py:466: # Touch the version file, has
reading it isn't always enough to update its
On 2016/02/02 17:34:07, scottmg wrote:
> has -> as

Oops.

[scottmg, 2016-02-02 19:14:42]

On 2016/02/02 18:37:26, Sébastien Marchand wrote:
> Thanks, should I take any particular precaution before committing this ? (I've
> done a bunch of tests locally).
> 
> My plan is to commit it and then do a dummy CL in Chrome and make sure that
the
> TS stays green...

Nah, should be fine. Maybe do it in the morning or sometime you'll be around for
a while I guess. It's going to cause a re-download when it lands, right? Might
be a bit confusing with the switch to 2015, but no big deal I think.

> 
>
https://codereview.chromium.org/1634923002/diff/80001/win_toolchain/get_toolc...
> File win_toolchain/get_toolchain_if_necessary.py (right):
> 
>
https://codereview.chromium.org/1634923002/diff/80001/win_toolchain/get_toolc...
> win_toolchain/get_toolchain_if_necessary.py:299:
> valid_toolchains.append((os.path.getatime(version_file), d))
> On 2016/02/02 17:34:07, scottmg wrote:
> > Let's use mtime here because it's the one more commonly used for dependency
> > checks (and matches what we ignore above).
> 
> Done.
> 
>
https://codereview.chromium.org/1634923002/diff/80001/win_toolchain/get_toolc...
> win_toolchain/get_toolchain_if_necessary.py:466: # Touch the version file, has
> reading it isn't always enough to update its
> On 2016/02/02 17:34:07, scottmg wrote:
> > has -> as
> 
> Oops.

[Sébastien Marchand, 2016-02-02 19:18:28]

K, I'll do it tomorrow morning then (around 10AM EST, so 7AM PST), it'll
re-download of the toolchain but it shouldn't be a big deal.

[Sébastien Marchand, 2016-02-02 19:38:25]

Dang ! I just realized that VS_VERSION isn't present in
ee7d718ec60c2dc5d255bbe325909c2021a7efef and it's the version I need in Syzygy
(the reason why I'm doing this change). I could use the mtime of the VC
directory instead, any objection ?

[scottmg, 2016-02-02 19:44:37]

On 2016/02/02 19:38:25, Sébastien Marchand wrote:
> Dang ! I just realized that VS_VERSION isn't present in
> ee7d718ec60c2dc5d255bbe325909c2021a7efef and it's the version I need in Syzygy
> (the reason why I'm doing this change). I could use the mtime of the VC
> directory instead, any objection ?

I'm not sure. I've always been leery of directory mtimes on Windows, but that
might be a holdover from FAT.

Looking at https://support.microsoft.com/en-us/kb/299648 it looks like it should
be OK on NTFS though.

Does os.utime work ok on dirs?

[Sébastien Marchand, 2016-02-02 19:46:54]

It works on my machine at least:
>>> import os
>>> os.path.getmtime('VC')
1454442210.566179
>>> os.path.getmtime('VC\\bin')
1454442202.1473372
>>> os.utime('VC', None)
>>> os.path.getmtime('VC')
1454442353.394
>>> os.path.getmtime('VC\\bin')
1454442202.1473372

[scottmg, 2016-02-02 19:52:22]

On 2016/02/02 19:46:54, Sébastien Marchand wrote:
> It works on my machine at least:
> >>> import os
> >>> os.path.getmtime('VC')
> 1454442210.566179
> >>> os.path.getmtime('VC\\bin')
> 1454442202.1473372
> >>> os.utime('VC', None)
> >>> os.path.getmtime('VC')
> 1454442353.394
> >>> os.path.getmtime('VC\\bin')
> 1454442202.1473372

OK, we could probably do that then. Bruce is about to roll a new 2013 toolchain
here https://codereview.chromium.org/1616553002/ . Can you just use the code as
is, and have Syzygy use that one instead? Or it has to be ee7d7...?

[Sébastien Marchand, 2016-02-02 19:53:45]

Well, Syzygy hasn't been switched to the Win10 SDK, and I think that it's better
if we don't break support for the old versions of the toolchain no ?

[scottmg, 2016-02-02 20:01:02]

On 2016/02/02 19:53:45, Sébastien Marchand wrote:
> Well, Syzygy hasn't been switched to the Win10 SDK, and I think that it's
better
> if we don't break support for the old versions of the toolchain no ?

OK, VC dir SG.

[Sébastien Marchand, 2016-02-02 20:52:43]

Done.

[scottmg, 2016-02-02 21:09:38]

ps8 lgtm

[Sébastien Marchand, 2016-02-03 14:57:01]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2016-02-03 14:57:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634923002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634923002/160001

[commit-bot: I haz the power, 2016-02-03 14:59:19]

Description was changed from

==========
Add the possibility to keep several version of the VS toolchain.

BUG=
==========

to

==========
Add the possibility to keep several version of the VS toolchain.

BUG=

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

[commit-bot: I haz the power, 2016-02-03 14:59:21]

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

[brucedawson, 2016-02-04 23:47:58]

A revert of this CL (patchset #8 id:160001) has been created in
https://codereview.chromium.org/1669993002/ by brucedawson@chromium.org.

The reason for reverting is: Suspected of causing goma errors on some machines
when using VS 2015. The suspicion is that the longer paths are causing
problems..

[scottmg, 2016-02-04 23:50:19]

Darn, and I just redownloaded twice. That sucks. We can't fix it on the goma
side?

[brucedawson, 2017-04-04 21:03:32]

On 2016/02/04 23:50:19, scottmg wrote:
> Darn, and I just redownloaded twice. That sucks. We can't fix it on the goma
> side?

Note that this CL was *not* reverted. The revert was created but not landed.

Also note that this CL turns out to be very handy. It means then when we roll
the toolchain the compile and link command lines change, which makes ninja
automatically think those steps are dirty. In conjunction with
crrev.com/2780023002 (which gets gn analyze to build all targets when
vs_toolchain.py changes) this should make toolchain rolls clean and automatic.