Automatically copy the DIA DLL in the Syzygy binaries directory.

Automatically copy the DIA DLL in the Syzygy binaries directory.

Committed: https://crrev.com/f34c072397b99c313e79cd10a77f5ca397154399
Committed: https://crrev.com/af7cc2f1de47235b720dafa5536f16666fd282df
Cr-Original-Commit-Position: refs/heads/master@{#422655}
Cr-Commit-Position: refs/heads/master@{#422861}

[Sébastien Marchand, 2016-09-23 14:48:30]

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

[Sébastien Marchand, 2016-09-23 14:48:31]

PTAL.

[Sébastien Marchand, 2016-09-23 15:47:26]

The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 15:47:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sébastien Marchand, 2016-09-23 15:53:01]

The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 15:53:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 17:52:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 17:52:06]

Dry run: This issue passed the CQ dry run.

[Sébastien Marchand, 2016-09-23 18:10:03]

Description was changed from

==========
Automatically copy the DIA DLL in the Syzygy binaries directory.
==========

to

==========
Automatically copy the DIA DLL in the Syzygy binaries directory.

This doesn't ensure that the version of msdia that we're copying is the one that
we need, it just copy the Chromium one so it only works if Syzygy is using the
same version of VS as Chromium.

VS2015 would probably be the last toolchain that we support anyway, so it should
be fine?
==========

[Sébastien Marchand, 2016-09-23 18:10:03]

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

[Sébastien Marchand, 2016-09-23 18:10:31]

+siggi@

[chrisha, 2016-09-26 17:44:24]

You could make this accept a toolchain version on the command-line, and have it
fail if it can't find the right version?

[Sébastien Marchand, 2016-09-26 17:47:48]

I'm not sure about this... The toolchains in depot_tools get deleted after 30
days if they're not used, this mean that 'runhooks' will start failing after 30
days if the toolchain gets upgraded.

[Sébastien Marchand, 2016-09-29 15:03:49]

The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 15:04:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sébastien Marchand, 2016-09-29 15:04:19]

PTAnL

[chrisha, 2016-09-29 15:06:38]

Actually, I guess we should simply hardcode the msdiaXXX name we expect? This is
implicitly tied to a generation of the toolchain, and is the actual thing we
care about.

[Sébastien Marchand, 2016-09-29 15:18:13]

The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 15:18:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sébastien Marchand, 2016-09-29 15:18:33]

Done, PTAnL.

[chrisha, 2016-09-29 15:26:59]

https://codereview.chromium.org/2365893002/diff/60001/build/get_syzygy_binari...
File build/get_syzygy_binaries.py (right):

https://codereview.chromium.org/2365893002/diff/60001/build/get_syzygy_binari...
build/get_syzygy_binaries.py:307: if binary != 'msdia140.dll':
Lift this to a definition at the top of the file?

You could probably safely skip the whole os.listdir and fnmatch logic, and
simply test for existence and copy, failing if its not found.

[Sébastien Marchand, 2016-09-29 15:29:57]

Yeah, I just kept this so get a clean error message (e.g. instead of telling you
that you're missing msdia140.dll it also tells you which version you have).

[commit-bot: I haz the power, 2016-09-29 16:27:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 16:27:45]

Dry run: This issue passed the CQ dry run.

[chrisha, 2016-09-30 03:00:42]

I'm not sure there's much value in knowing which version is there, but I don't
care too much.

lgtm

[Sébastien Marchand, 2016-09-30 14:09:15]

Yeah, I'm not sure if it's super helpful but it doesn't hurt and provide some
extra debug info for 'almost' free.

[Sébastien Marchand, 2016-09-30 14:09:20]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2016-09-30 14:09:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 14:17:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 14:17:54]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Sébastien Marchand, 2016-10-03 19:54:48]

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

[Sébastien Marchand, 2016-10-03 19:54:49]

+scottmg for owner approval.

[scottmg, 2016-10-03 20:20:56]

https://codereview.chromium.org/2365893002/diff/60001/build/get_syzygy_binari...
File build/get_syzygy_binaries.py (right):

https://codereview.chromium.org/2365893002/diff/60001/build/get_syzygy_binari...
build/get_syzygy_binaries.py:305: for binary in
os.listdir(dia_sdk_binaries_dir):
If you're only copying msdia140.dll, why not just copy it instead of all this?

[Sébastien Marchand, 2016-10-03 20:23:42]

https://codereview.chromium.org/2365893002/diff/60001/build/get_syzygy_binari...
File build/get_syzygy_binaries.py (right):

https://codereview.chromium.org/2365893002/diff/60001/build/get_syzygy_binari...
build/get_syzygy_binaries.py:305: for binary in
os.listdir(dia_sdk_binaries_dir):
On 2016/10/03 20:20:55, scottmg wrote:
> If you're only copying msdia140.dll, why not just copy it instead of all this?

Chris had the same question :), I just wanted this to break with a meaningful
error message if the invalid version of msdia is missing, e.g. "Invalid version,
expecting msdia140.dll but got msdia150.dll". I can remove all of this and just
copy mdsia if you also think that this detailed log is missing (and that
"msdia140.dll is missing" is enough as an error message).

[scottmg, 2016-10-03 20:26:17]

On 2016/10/03 20:23:42, Sébastien Marchand wrote:
>
https://codereview.chromium.org/2365893002/diff/60001/build/get_syzygy_binari...
> File build/get_syzygy_binaries.py (right):
> 
>
https://codereview.chromium.org/2365893002/diff/60001/build/get_syzygy_binari...
> build/get_syzygy_binaries.py:305: for binary in
> os.listdir(dia_sdk_binaries_dir):
> On 2016/10/03 20:20:55, scottmg wrote:
> > If you're only copying msdia140.dll, why not just copy it instead of all
this?
> 
> Chris had the same question :), I just wanted this to break with a meaningful
> error message if the invalid version of msdia is missing, e.g. "Invalid
version,
> expecting msdia140.dll but got msdia150.dll". I can remove all of this and
just
> copy mdsia if you also think that this detailed log is missing (and that
> "msdia140.dll is missing" is enough as an error message).

I see. I think "msdia140 not found" is sufficient. But if you think this might
help somehow, then lgtm.

[scottmg, 2016-10-03 20:27:10]

On 2016/10/03 20:26:17, scottmg wrote:
> On 2016/10/03 20:23:42, Sébastien Marchand wrote:
> >
>
https://codereview.chromium.org/2365893002/diff/60001/build/get_syzygy_binari...
> > File build/get_syzygy_binaries.py (right):
> > 
> >
>
https://codereview.chromium.org/2365893002/diff/60001/build/get_syzygy_binari...
> > build/get_syzygy_binaries.py:305: for binary in
> > os.listdir(dia_sdk_binaries_dir):
> > On 2016/10/03 20:20:55, scottmg wrote:
> > > If you're only copying msdia140.dll, why not just copy it instead of all
> this?
> > 
> > Chris had the same question :), I just wanted this to break with a
meaningful
> > error message if the invalid version of msdia is missing, e.g. "Invalid
> version,
> > expecting msdia140.dll but got msdia150.dll". I can remove all of this and
> just
> > copy mdsia if you also think that this detailed log is missing (and that
> > "msdia140.dll is missing" is enough as an error message).
> 
> I see. I think "msdia140 not found" is sufficient. But if you think this might
> help somehow, then lgtm.

(But I guess if you're going to keep it, then you should explain that in a
comment before the loop since apparently everyone who reads it has the same
question. :)

[Sébastien Marchand, 2016-10-03 20:34:39]

The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 20:35:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sébastien Marchand, 2016-10-03 20:36:16]

Bah, I've removed this extra complexity, saying that the DLL is missing is
probably enough (and I expect this to be gone long before the switch to VC
15.0+)

[scottmg, 2016-10-03 20:49:28]

lgtm

https://codereview.chromium.org/2365893002/diff/80001/build/get_syzygy_binari...
File build/get_syzygy_binaries.py (right):

https://codereview.chromium.org/2365893002/diff/80001/build/get_syzygy_binari...
build/get_syzygy_binaries.py:312: dia_dll_dest =
os.path.join(options.output_dir, 'exe', binary)
binary -> _DIA_DLL_NAME

https://codereview.chromium.org/2365893002/diff/80001/build/get_syzygy_binari...
build/get_syzygy_binaries.py:318: return
Remove 'return'

[commit-bot: I haz the power, 2016-10-03 20:52:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 20:52:16]

Dry run: Exceeded global retry quota

[Sébastien Marchand, 2016-10-03 21:01:11]

The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 21:02:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2016-10-03 21:47:04]

https://codereview.chromium.org/2365893002/diff/120001/build/get_syzygy_binar...
File build/get_syzygy_binaries.py (right):

https://codereview.chromium.org/2365893002/diff/120001/build/get_syzygy_binar...
build/get_syzygy_binaries.py:290: def _MaybeCopyDIABinaries(options, contents):
I guess it shouldn't be called "Maybe..." any more either as it either succeeds
at copying or raises.

(If you do that, I'd probably make the _LOGGER.debug with early return be raise
too if that doesn't break something.)

[Sébastien Marchand, 2016-10-03 21:54:04]

Patchset #8 (id:140001) has been deleted

[Sébastien Marchand, 2016-10-03 21:54:33]

Description was changed from

==========
Automatically copy the DIA DLL in the Syzygy binaries directory.

This doesn't ensure that the version of msdia that we're copying is the one that
we need, it just copy the Chromium one so it only works if Syzygy is using the
same version of VS as Chromium.

VS2015 would probably be the last toolchain that we support anyway, so it should
be fine?
==========

to

==========
Automatically copy the DIA DLL in the Syzygy binaries directory.
==========

[Sébastien Marchand, 2016-10-03 21:56:52]

Patchset #8 (id:160001) has been deleted

[Sébastien Marchand, 2016-10-03 21:57:08]

https://codereview.chromium.org/2365893002/diff/120001/build/get_syzygy_binar...
File build/get_syzygy_binaries.py (right):

https://codereview.chromium.org/2365893002/diff/120001/build/get_syzygy_binar...
build/get_syzygy_binaries.py:290: def _MaybeCopyDIABinaries(options, contents):
On 2016/10/03 21:47:03, scottmg wrote:
> I guess it shouldn't be called "Maybe..." any more either as it either
succeeds
> at copying or raises.
> 
> (If you do that, I'd probably make the _LOGGER.debug with early return be
raise
> too if that doesn't break something.)

Ha good point, initially this function was only a 'best effort' thing (i.e. copy
the dll if it's there, skip it otherwise), but I can't think of any reason why
it might be missing.

[Sébastien Marchand, 2016-10-03 23:39:48]

The CQ bit was checked by sebmarchand@chromium.org

[Sébastien Marchand, 2016-10-03 23:39:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrisha@chromium.org, scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/2365893002/#ps240001
(title: ".")

[commit-bot: I haz the power, 2016-10-03 23:40:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 01:25:23]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2016-10-04 01:28:13]

Description was changed from

==========
Automatically copy the DIA DLL in the Syzygy binaries directory.
==========

to

==========
Automatically copy the DIA DLL in the Syzygy binaries directory.

Committed: https://crrev.com/f34c072397b99c313e79cd10a77f5ca397154399
Cr-Commit-Position: refs/heads/master@{#422655}
==========

[commit-bot: I haz the power, 2016-10-04 01:28:14]

Patchset 11 (id:??) landed as
https://crrev.com/f34c072397b99c313e79cd10a77f5ca397154399
Cr-Commit-Position: refs/heads/master@{#422655}

[claudiomdsjr, 2016-10-04 12:16:52]

On 2016/10/04 01:28:14, commit-bot: I haz the power wrote:
> Patchset 11 (id:??) landed as
> https://crrev.com/f34c072397b99c313e79cd10a77f5ca397154399
> Cr-Commit-Position: refs/heads/master@{#422655}

I'm afraid this addition has broken gclient sync when you use it with
DEPOT_TOOLS_WIN_TOOLCHAIN=0. I have not been able to sync dependencies since
then...

Claudio.

[Sébastien Marchand, 2016-10-04 12:42:06]

A revert of this CL (patchset #11 id:240001) has been created in
https://codereview.chromium.org/2390993002/ by sebmarchand@chromium.org.

The reason for reverting is: Break gclient for the open contributors..

[Sébastien Marchand, 2016-10-04 14:54:55]

Description was changed from

==========
Automatically copy the DIA DLL in the Syzygy binaries directory.

Committed: https://crrev.com/f34c072397b99c313e79cd10a77f5ca397154399
Cr-Commit-Position: refs/heads/master@{#422655}
==========

to

==========
Automatically copy the DIA DLL in the Syzygy binaries directory.

Committed: https://crrev.com/f34c072397b99c313e79cd10a77f5ca397154399
Cr-Commit-Position: refs/heads/master@{#422655}
==========

[Sébastien Marchand, 2016-10-04 14:58:32]

I've made some changes to not fail hard. The non-Google contributors don't have
the automated toolchain and so it doesn't work for them. This isn't a big deal
as this dll is mostly for our builders (the local devs can always manually
register msdia if they need it). PTAnL

[chrisha, 2016-10-04 15:14:36]

lgtm with a nit

https://codereview.chromium.org/2365893002/diff/260001/build/get_syzygy_binar...
File build/get_syzygy_binaries.py (right):

https://codereview.chromium.org/2365893002/diff/260001/build/get_syzygy_binar...
build/get_syzygy_binaries.py:405: 'binaries directory if it\'s available.')
update this comment to reflect that it will *try*.

[Sébastien Marchand, 2016-10-04 15:18:50]

https://codereview.chromium.org/2365893002/diff/260001/build/get_syzygy_binar...
File build/get_syzygy_binaries.py (right):

https://codereview.chromium.org/2365893002/diff/260001/build/get_syzygy_binar...
build/get_syzygy_binaries.py:405: 'binaries directory if it\'s available.')
On 2016/10/04 15:14:36, chrisha (slow) wrote:
> update this comment to reflect that it will *try*.

It already mention "If it's available", isn't it enough?

[chrisha, 2016-10-04 17:15:54]

Ah, missed that. lgtm!

[Sébastien Marchand, 2016-10-04 17:16:44]

The CQ bit was checked by sebmarchand@chromium.org

[Sébastien Marchand, 2016-10-04 17:16:44]

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/2365893002/#ps260001
(title: "Don't fail hard if DIA is missing.")

[commit-bot: I haz the power, 2016-10-04 17:17:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 18:23:01]

Description was changed from

==========
Automatically copy the DIA DLL in the Syzygy binaries directory.

Committed: https://crrev.com/f34c072397b99c313e79cd10a77f5ca397154399
Cr-Commit-Position: refs/heads/master@{#422655}
==========

to

==========
Automatically copy the DIA DLL in the Syzygy binaries directory.

Committed: https://crrev.com/f34c072397b99c313e79cd10a77f5ca397154399
Cr-Commit-Position: refs/heads/master@{#422655}
==========

[commit-bot: I haz the power, 2016-10-04 18:23:02]

Committed patchset #12 (id:260001)

[commit-bot: I haz the power, 2016-10-04 18:25:27]

Description was changed from

==========
Automatically copy the DIA DLL in the Syzygy binaries directory.

Committed: https://crrev.com/f34c072397b99c313e79cd10a77f5ca397154399
Cr-Commit-Position: refs/heads/master@{#422655}
==========

to

==========
Automatically copy the DIA DLL in the Syzygy binaries directory.

Committed: https://crrev.com/f34c072397b99c313e79cd10a77f5ca397154399
Committed: https://crrev.com/af7cc2f1de47235b720dafa5536f16666fd282df
Cr-Original-Commit-Position: refs/heads/master@{#422655}
Cr-Commit-Position: refs/heads/master@{#422861}
==========

[commit-bot: I haz the power, 2016-10-04 18:25:28]

Patchset 12 (id:??) landed as
https://crrev.com/af7cc2f1de47235b720dafa5536f16666fd282df
Cr-Commit-Position: refs/heads/master@{#422861}