build with MSVS will only need VS console environment set up

build with MSVS will only need VS console environment set up

Remove hard-coded paths, and all sort of voodoo in favor of
environment variables set by MSVS dev console
(e.g. `VSINSTALLDIR`, `VisualStudioVersion`, `WindowsSdkDir`, and `Path`)

BUG=683729

[refack, 2017-03-25 20:07:10]

refack@gmail.com changed reviewers:
+ bratell@opera.cmo, brucedawson@chromium.org, dpranke@google.com,
thestig@chromium.org

[refack, 2017-03-25 20:07:11]

Based on my idea from https://codereview.chromium.org/2754263002/.
Remove code that uses assumptions and let the developer decide which MSVS to use
(by setting up a VS dev console)

[brucedawson, 2017-03-27 23:23:52]

On 2017/03/25 20:07:11, refack wrote:
> Based on my idea from https://codereview.chromium.org/2754263002/.
> Remove code that uses assumptions and let the developer decide which MSVS to
use
> (by setting up a VS dev console)

Off the top of my head I see two concerns with this technique. One is just the
risk of change - Chrome developers have grown used to a particular way of
working, of running builds from a normal command prompt, with the only
environment setup being set DEPOT_TOOLS_WIN_TOOLCHAIN=0. This would change that.
Some of the disruption can be reduced by pointing out (perhaps in the
description) these alternate ways of setting up for a build after this change
lands:

"%VS140COMNTOOLS%..\..\VC\vcvarsall.bat" amd64_x86
"%VS140COMNTOOLS%..\..\VC\vcvarsall.bat" amd64
"C:\Program Files (x86)\Microsoft Visual
Studio\2017\Professional\VC\Auxiliary\Build\vcvarsall.bat" amd64_x86
"C:\Program Files (x86)\Microsoft Visual
Studio\2017\Professional\VC\Auxiliary\Build\vcvarsall.bat" amd64

That is, developers shouldn't have to run from a VS command prompt - they can
add the environment to a 'normal' command prompt. The CL description should be
updated to reflect that.


But I have one other, more serious, concern. Currently I can do x64 and x86
builds of Chrome from the same command prompt, just by adjusting ninja
variables. With this change I will not be able to do that. Worse yet, developers
will have to carefully match up the type of dev console that they open with the
type of build they plan to do. I fear that this complexity justifies retaining
the previous system.

https://codereview.chromium.org/2770193005/diff/20001/build/vs_toolchain.py
File build/vs_toolchain.py (right):

https://codereview.chromium.org/2770193005/diff/20001/build/vs_toolchain.py#n...
build/vs_toolchain.py:170: def _SdkFileSuffix():
This needs a larger comment to explain what this number means. In some contexts
2017 increases 140 to 141, and in other contexts it leaves it at 140. This
particular context is, I believe, not SDK files but CRT files.

So, I think this function needs renaming.

https://codereview.chromium.org/2770193005/diff/20001/build/vs_toolchain.py#n...
build/vs_toolchain.py:246: _CopyRuntime2013(target_dir, source_dir, 'msvc' +
dll_pattern)
You'll need to rebase as the 2013 support was just removed. Sorry :-(

[refack, 2017-03-27 23:39:21]

On Mon, Mar 27, 2017 at 7:23 PM, <brucedawson@chromium.org> wrote:

> Some of the disruption can be reduced by pointing out (perhaps in the
> description) these alternate ways of setting up for a build after this
> change
> lands:
>
> "%VS140COMNTOOLS%..\..\VC\vcvarsall.bat" amd64_x86
> "%VS140COMNTOOLS%..\..\VC\vcvarsall.bat" amd64
> "C:\Program Files (x86)\Microsoft Visual
> Studio\2017\Professional\VC\Auxiliary\Build\vcvarsall.bat" amd64_x86
> "C:\Program Files (x86)\Microsoft Visual
> Studio\2017\Professional\VC\Auxiliary\Build\vcvarsall.bat" amd64
>
> That is, developers shouldn't have to run from a VS command prompt - they
> can
> add the environment to a 'normal' command prompt. The CL description
> should be
> updated to reflect that.
>

​Yes, they are one and the same (a "VS dev console", is just a shortcut
with params to the .bat file ;)​
​I could be more clear​ in the description, docs, and error messages.

But I have one other, more serious, concern. Currently I can do x64 and x86
> builds of Chrome from the same command prompt, just by adjusting ninja
> variables. With this change I will not be able to do that. Worse yet,
> developers
> will have to carefully match up the type of dev console that they open
> with the
> type of build they plan to do. I fear that this complexity justifies
> retaining
> the previous system.
>

​I'll think of a way to allow simple retargeting with args. I believe you
can "rebase" the environment by calling the batch with new host/target args.
What we should be aware of is that now MS is allowing multiple tenancy; you
could have VS2013, VS2105 and 5 separate instances on VS2017, the dev
should be able to explicitly choose.
P.S. how do you call ninja to target different archs? (Sorry new to the
chromium way).
​

> https://codereview.chromium.org/2770193005/diff/20001/
> build/vs_toolchain.py#newcode246
> build/vs_toolchain.py:246: _CopyRuntime2013(target_dir, source_dir,
> 'msvc' + dll_pattern)
> You'll need to rebase as the 2013 support was just removed. Sorry :-(
>

​Yay, death to 2013 !!!!!!!!!​

-- 
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.

[brucedawson, 2017-03-27 23:42:16]

> P.S. how do you call ninja to target different archs? (Sorry new to the
> chromium way).

In args.gn (i.e.; when the editor comes up when you run "gn args out\foo") paste
in one of these:
target_cpu="x86"
target_cpu="x64"

[refack, 2017-03-27 23:43:37]

On 2017/03/27 23:42:16, brucedawson wrote:
> In args.gn (i.e.; when the editor comes up when you run "gn args out\foo")
paste
> in one of these:
> target_cpu="x86"
> target_cpu="x64"

👍

[Dirk Pranke, 2017-04-01 01:28:21]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2017-04-01 01:28:21]

I do not want users to have to have a particular environment set up if we can
handle things for them in (the common) case(s). 

I would be fine w/ taking advantage of the environment if it *was* set up, but
not requiring it.

So, I doubt this approach will fly with me, but I'm certainly open to continuing
the conversation :).

[refack, 2017-04-01 01:35:01]

On 2017/04/01 01:28:21, Dirk Pranke wrote:
> I do not want users to have to have a particular environment set up if we can
> handle things for them in (the common) case(s). 
> 
> I would be fine w/ taking advantage of the environment if it *was* set up, but
> not requiring it.
> 
> So, I doubt this approach will fly with me, but I'm certainly open to
continuing
> the conversation :).

This CL was indeed intended to be conversation piece in continuation of
https://codereview.chromium.org/2754263002/.

I'm mulling things over, and learning of all sort of solutions... Maybe we'll
find some optimal solution...