Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2754263002:
Retrieve Visual Studio 2017 installation location from registry (Closed)
DescriptionRetrieve Visual Studio 2017 installation location from registry
Remove the hard-coded path that was needed for VS 15 Preview. Since VS
2017 RC Microsoft provides a way to read the installation location from
the registry like it was possible with older VS versions.
Luckily, the same code path can be used for all VS versions. Below
HKLM\Software\Wow6432Node\Microsoft\VisualStudio\SxS\VS7 are keys per VS
version that contain the installation path. That path does not have the
"\Common7\IDE" suffix, thus the code that removed it can go.
BUG=
Patch Set 1 #
Messages
Total messages: 28 (3 generated)
Description was changed from ========== Retrieve Visual Studio 2017 installation location from registry Remove the hard-coded path that was needed for VS 15 Preview. Since VS 2017 RC Microsoft provides a way to read the installation location from the registry like it was possible with older VS versions. Luckily, the same code path can be used for all VS versions. Below HKLM\Software\Wow6432Node\Microsoft\VisualStudio\SxS\VS7 are keys per VS version that contain the installation path. That path does not have the "\Common7\IDE" suffix, thus the code that removed it can go. BUG= ========== to ========== Retrieve Visual Studio 2017 installation location from registry Remove the hard-coded path that was needed for VS 15 Preview. Since VS 2017 RC Microsoft provides a way to read the installation location from the registry like it was possible with older VS versions. Luckily, the same code path can be used for all VS versions. Below HKLM\Software\Wow6432Node\Microsoft\VisualStudio\SxS\VS7 are keys per VS version that contain the installation path. That path does not have the "\Common7\IDE" suffix, thus the code that removed it can go. BUG= ==========
joerg.bornemann@gmail.com changed reviewers: + brucedawson@chromium.org
lgtm with two CL description changes: - Add this bug number: 683729. You should never have an empty BUG= line - either delete the line or (in this case) add the relevant bug number(s). - Shorten the title slightly. The target should be 50 characters, although perfect adherence is not crucial. But changing Visual Studio to VS will help. - Are you sure that this registry key existed for VS 2017 RC? I couldn't find it. This only affects the CL description, but I just thought I'd check.
refack@gmail.com changed reviewers: + refack@gmail.com
I'm sure the issue that the registry "HKLM\Software\Microsoft\VisualStudio\SxS\VS7" is not supported by Microsoft (https://blogs.msdn.microsoft.com/heaths/2016/09/15/changes-to-visual-studio-1...) This actually fails for the "Visual C++ build tools" SKU (which is my favorite). Also ironically the VS 2017 is an Electron app that gets frequent updates and might stop setting the reg key at any point...
Andrew, what's MS's take?
On 2017/03/20 23:41:08, refack wrote:
> Andrew, what's MS's take?
One option would be to leave in the override:
path = os.environ.get('vs2017_install', path)
That lets us keep the code simple while having a way to continue if the registry
key start working. Adding complexity before it is needed seems like a bad idea.
Refack/Bruce, thanks for looping me! This is a Heath issue. Adding him. From: brucedawson@chromium.org [mailto:brucedawson@chromium.org] Sent: Monday, March 20, 2017 4:45 PM To: joerg.bornemann@gmail.com; refack@gmail.com Cc: chromium-reviews@chromium.org; Andrew Pardoe <Andrew.Pardoe@microsoft.com> Subject: Re: Retrieve Visual Studio 2017 installation location from registry (issue 2754263002 by joerg.bornemann@gmail.com) On 2017/03/20 23:41:08, refack wrote: > Andrew, what's MS's take? One option would be to leave in the override: path = os.environ.get('vs2017_install', path) That lets us keep the code simple while having a way to continue if the registry key start working. Adding complexity before it is needed seems like a bad idea. https://codereview.chromium.org/2754263002/ -- 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.
On 2017/03/20 23:48:26, chromium-reviews wrote:
> Refack/Bruce, thanks for looping me!
>
> This is a Heath issue. Adding him.
>
> From: mailto:brucedawson@chromium.org [mailto:brucedawson@chromium.org]
> Sent: Monday, March 20, 2017 4:45 PM
> To: mailto:joerg.bornemann@gmail.com; mailto:refack@gmail.com
> Cc: mailto:chromium-reviews@chromium.org; Andrew Pardoe
<mailto:Andrew.Pardoe@microsoft.com>
> Subject: Re: Retrieve Visual Studio 2017 installation location from registry
> (issue 2754263002 by mailto:joerg.bornemann@gmail.com)
>
> On 2017/03/20 23:41:08, refack wrote:
> > Andrew, what's MS's take?
>
> One option would be to leave in the override:
>
> path = os.environ.get('vs2017_install', path)
>
> That lets us keep the code simple while having a way to continue if the
> registry
> key start working. Adding complexity before it is needed seems like a bad
idea.
>
>
> https://codereview.chromium.org/2754263002/
>
> --
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > One option would be to leave in the override:
> >
> > path = os.environ.get('vs2017_install', path)
> >
> > That lets us keep the code simple while having a way to continue if the
> > registry
> > key start working. Adding complexity before it is needed seems like a bad
> idea.
Rietvald ate my comment. :(
My tool has a very small footprint inside the code.
```
cmdPath = os.path.join(os.path.dirname(__file__), '..', '..', 'tools',
'vc141helper', 'get_key_helper.cmd')
try:
import subprocess
args = [cmdPath, 'IsVcCompatible', 'InstallationPath']
instPath2017 = subprocess.check_output(args).strip()
except Exception as e:
pass
continue
```
I'll make a PR.
Right, Visual C++ Build Tools can be supported by reading yet another registry key like it is done here: http://code.qt.io/cgit/qt-labs/qbs.git/commit/?id=72463bbd87044148fcafcdc2c11... IIUC the official way is to redistribute the PowerShell module from https://github.com/Microsoft/vssetup.powershell/releases and cross fingers that Powershell's execution policy allows user code to run Get-VSSetupInstance.
On 2017/03/21 10:29:27, jobor wrote: > Right, Visual C++ Build Tools can be supported by reading yet another registry > key like it is done here: > http://code.qt.io/cgit/qt-labs/qbs.git/commit/?id=72463bbd87044148fcafcdc2c11... > > IIUC the official way is to redistribute the PowerShell module from > https://github.com/Microsoft/vssetup.powershell/releases and cross fingers that > Powershell's execution policy allows user code to run Get-VSSetupInstance. My tools (https://github.com/node4good/msvs-com-helper) follow the officially supported method. The guy that's in charge of this in Microsoft knows about it (he doesn't like it) but acknowledge they work. P.S. Powershell's execution policy is only a user convenience, and is easily overridden ;) P.P.S. there is an Official MS exe (https://github.com/Microsoft/vswhere) to interrogate the COM, but I don't like it's API
On 2017/03/21 16:15:42, refack wrote: > P.P.S. there is an Official MS exe (https://github.com/Microsoft/vswhere) to > interrogate the COM, but I don't like it's API The COM API is the officially supported way to get at the install location. The registry isn't supported, or guaranteed to exist. Can you elaborate as to why you don't like it? Also >> The guy that's in charge of this in Microsoft knows about it (he doesn't like it) but acknowledge they work. Which guy? If you don't want to name him publicly, feel free to email me.
Allright, the official, non-registry-reading way is apparently the more future proof way to go. And it looks like we have plenty of tools to choose from.
On 2017/03/21 16:53:58, andrew.pardoe wrote: > On 2017/03/21 16:15:42, refack wrote: > > > P.P.S. there is an Official MS exe (https://github.com/Microsoft/vswhere) to > > interrogate the COM, but I don't like it's API > > The COM API is the officially supported way to get at the install location. The > registry isn't supported, or guaranteed to exist. Can you elaborate as to why > you don't like it? Adding a dependency on vswhere (~100 files) or using powershell/C# (31 lines and 326 lines respectively in one proposed solution) seems like massive engineering overkill for the problem. Our current solution in Chromium is "Assume the default install location and have an environment-variable override as an escape hatch". It's not clear to me what advantages the other solutions have over the one-liner. If it was entirely up to me I would stick with the current solution and move to something fancier if the one-liner solution became insufficient.
On 2017/03/21 23:55:21, brucedawson wrote: > Our current solution in Chromium is "Assume the default install location and > have an environment-variable override as an escape hatch". I like this proposal. A lot. Anyone smart enough to build Chromium should be smart enough to find their compiler.
From the people who brought you: gclient, depot-tools, GYP, gn... Yes a ~300 line tool is over engineering 🤣. Just make it clear in the docs, and in the error message. The "escape hatch" that is. Even simpler, just say, "run this from a VS dev console" and remove all the registry/default location voodoo. On Mar 21, 2017 20:06, <andrew.pardoe@gmail.com> wrote: On 2017/03/21 23:55:21, brucedawson wrote: > Our current solution in Chromium is "Assume the default install location and > have an environment-variable override as an escape hatch". I like this proposal. A lot. Anyone smart enough to build Chromium should be smart enough to find their compiler. https://codereview.chromium.org/2754263002/ -- 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.
On 2017/03/22 00:37:33, refack wrote: > From the people who brought you: gclient, depot-tools, GYP, gn... Yes a > ~300 line tool is over engineering 🤣. > > Just make it clear in the docs, and in the error message. The "escape > hatch" that is. > Even simpler, just say, "run this from a VS dev console" and remove all the > registry/default location voodoo. > > > On Mar 21, 2017 20:06, <mailto:andrew.pardoe@gmail.com> wrote: > > On 2017/03/21 23:55:21, brucedawson wrote: > > > Our current solution in Chromium is "Assume the default install location > and > > have an environment-variable override as an escape hatch". > > I like this proposal. A lot. Anyone smart enough to build Chromium should be > smart enough to find their compiler. > > > https://codereview.chromium.org/2754263002/ > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org. Andrew, why don't you add a cache mechanism to `VsDevCmd.bat` make it super fast, then it'll be a no brainer.
On 2017/03/22 00:42:57, refack wrote: > On 2017/03/22 00:37:33, refack wrote: > > From the people who brought you: gclient, depot-tools, GYP, gn... Yes a > > ~300 line tool is over engineering 🤣. Touché. I *think* that those tools are solving problems that are deserving of complex solutions, but, fair point.
Back to VS2017, way don't you just set `%VS150COMNTOOLS%` (for the multiple instance use-case you could set is to a `;` delimited list, like `%Path%`) so we could just find `VsDevCmd.bat`, you could even drop `vswhere` in there, and all would be solved happyly. ENV access is something all platforms have (COM isn't), and like I said before the installer is an Electron app, so I think it's you responsibility to assist us in getting Electron to easily compile on VS2017 😉
On 2017/03/24 16:32:31, refack wrote: > Back to VS2017, way don't you just set `%VS150COMNTOOLS%` (for the multiple > instance use-case you could set is to a `;` delimited list, like `%Path%`) so we > could just find `VsDevCmd.bat`, you could even drop `vswhere` in there, and all > would be solved happyly. > ENV access is something all platforms have (COM isn't), and like I said before > the installer is an Electron app, so I think it's you responsibility to assist > us in getting Electron to easily compile on VS2017 😉 That was to Andrew
On 2017/03/24 16:33:02, refack wrote: > On 2017/03/24 16:32:31, refack wrote: > > Back to VS2017, way don't you just set `%VS150COMNTOOLS%` (for the multiple > > instance use-case you could set is to a `;` delimited list, like `%Path%`) so > That was to Andrew I've made the arguments I can make to the VS team. Feel free to send feedback to them directly. The "Report a Problem" link in the VS IDE is actually a good way to send feedback to them.
​Will do. I urge everyone else to up vote this at https://developercommunity.visualstudio.com/content/problem/35325/missing-vs1... > I've made the arguments I can make to the VS team. Feel free to send > feedback to > them directly. The "Report a Problem" link in the VS IDE is actually a > good way > to send feedback to them. > -- 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.
Message was sent while issue was closed.
Closed this change. We will just set the vs2017_install environment variable in QtWebEngine's build system. I will add my two cents to the VS issue refack opened. This COM nonsense needs to end.
Message was sent while issue was closed.
On 2017/03/22 00:57:42, brucedawson wrote: > On 2017/03/22 00:42:57, refack wrote: > > On 2017/03/22 00:37:33, refack wrote: > > > From the people who brought you: gclient, depot-tools, GYP, gn... Yes a > > > ~300 line tool is over engineering 🤣. > > Touché. > > I *think* that those tools are solving problems that are deserving of complex > solutions, but, fair point. Just discovered `mb` https://chromium.googlesource.com/chromium/src/+/master/tools/mb 🤯 SMH Please comment on https://codereview.chromium.org/2770193005 - Remove path voodoo from code in favor of manually running `vcvarsall.bat`/`VsDevCmd.bat`
Message was sent while issue was closed.
On 2017/03/27 10:54:56, jobor wrote: > Closed this change. We will just set the vs2017_install environment variable in > QtWebEngine's build system. > > I will add my two cents to the VS issue refack opened. This COM nonsense needs > to end. @jobor like we used to say; GG!
Message was sent while issue was closed.
On 2017/03/27 12:02:34, refack wrote: > On 2017/03/27 10:54:56, jobor wrote: > > Closed this change. We will just set the vs2017_install environment variable > in > > QtWebEngine's build system. > > > > I will add my two cents to the VS issue refack opened. This COM nonsense needs > > to end. > > @jobor like we used to say; GG! The officially supported vswhere tool is now available as part of the install starting today with Visual Studio 15.2 preview 2: https://blogs.msdn.microsoft.com/heaths/2017/04/21/vswhere-is-now-installed-w.... This doesn't fix all the existing installs of VS 2017 but going forward you can now detect the VS install locations and whether C++ tools are available from a scripted environment. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
