Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating the build scripts to support VS2017. #9956

Merged
merged 3 commits into from Mar 9, 2017

Conversation

@tannergooding
Copy link
Member

commented Mar 4, 2017

This updates the build scripts to support VS 2017, provided you are running from a developer command prompt.

This does not support locating VS 2017 dynamically. There are two parts to this:

  1. The installation environment variables we used previously no longer exist globally
  2. In order to locate an instance of VS 2017, you need to use the Managed Query APIs (http://www.nuget.org/packages/Microsoft.VisualStudio.Setup.Configuration.Interop/1.7.13-rc). Adding support for this would take quite a bit more work, and supporting running from an existing DevCmd Prompt instance is at least significantly better than what we have today.

This also drops support for VS 2013 from the RunTest script. Given that we don't support building from VS 2013, it doesn't make sense to have this support here (let me know if there is some reason we do actually still need this).

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2017

Build gets a good ways through and then fails due to the DiaSDK not being registered:

System.Runtime.InteropServices.COMException (0x80040154): Retrieving the COM class factory for component
   with CLSID {3BFCEA48-620F-4B6B-81F7-B9AF75454C7D} failed due to the following error: 80040154 Class not registere
  d (Exception from HRESULT: 0x80040154 (REGDB_E_CLASSNOTREG)).
     at Dia.Util.DiaFile..ctor(String pdbFile, String dllFile)
     at PdbSymbolProvider..ctor(String symbolFilename, String dllFilename)
     at Shell.DoMain(String[] args)
     at Shell.Main(String[] args)

I imagine this has something to do with supporting SxS installs of a single version of VS in VS2017. I am going to investigate a bit further to see if I can get around this.

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2017

Equivalent CoreFX PR is here: dotnet/corefx#16710

This should resolve: #8441

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2017

Ok, looks like {3BFCEA48-620F-4B6B-81F7-B9AF75454C7D} is for msdia120.dll, which shipped with the 'DIA SDK' in VS2013.

The 'DIA SDK' in VS2015 and VS2017 ship with msdia140.dll which is {91904831-49CA-4766-B95C-25397E2DD6DC}.

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2017

And the failure is because Microsoft.DotNet.BuildTools.Coreclr\1.0.4-prerelease\DacTableGen.exe requires msdia120.dll

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2017

Looks like the appropriate course of action here is to rebuild the Microsoft.DotNet.BuildTools.Coreclr binaries on a machine that only has VS2017 installed.

A workaround is to install an older version of the DIA SDK that contains the appropriate binaries.

Edit: Looks like VS2017 also has a copy of msdia120.dll under Common7\IDE, and running regsvr32.exe on the library is a functioning workaround as well (NOTE: this requires admin privileges).

@tannergooding tannergooding force-pushed the tannergooding:vs2017 branch from 694bae5 to 62b71f2 Mar 4, 2017

@tannergooding tannergooding force-pushed the tannergooding:vs2017 branch from 62b71f2 to 01f21bf Mar 4, 2017

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2017

Awesome. With this PR + one of the DiaSDK workarounds I listed above, I am able to successfully build and run tests for CoreCLR on a machine with only VS2017 installed (which at least means I no longer have to maintain a VS 2015 VM just for CoreCLR work).

Not sure who is the best person to talk to about updating the Microsoft.DotNet.BuildTools.Coreclr package though. It looks like the sources for several of the tools it includes can be found under src\ToolBox after restoring, but it is not immediately obvious how these are meant to be built.

@gkhanna79

This comment has been minimized.

Copy link
Member

commented Mar 4, 2017

Not sure who is the best person to talk to about updating the Microsoft.DotNet.BuildTools.Coreclr package though

@chcosta Do you know?

@gkhanna79

This comment has been minimized.

Copy link
Member

commented Mar 4, 2017

@tannergooding Can you confirm this works when both VS 2015 and VS 2017 are installed?

@gkhanna79

This comment has been minimized.

Copy link
Member

commented Mar 4, 2017

@wtgodbe PTAL

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2017

Yes. It resolves to vs2017 if running from the vs2017 developer command prompt. Otherwise it falls back to vs2015.

@chcosta

This comment has been minimized.

Copy link
Member

commented Mar 4, 2017

It looks like Microsoft.DotNet.BuildTools.CoreClr is not a package we actively produce anymore (we haven't published it in about a year). I'm not certain where this was published from. Perhaps @weshaggard has some historical context for this package.

set __VSVersion=vs2017
) else (
set "__VSToolsRoot=%VS140COMNTOOLS%"
set "__VCToolsRoot=%VS150COMNTOOLS%\..\..\VC"

This comment has been minimized.

Copy link
@janvorli

janvorli Mar 6, 2017

Member

It looks like this should be VS140COMNTOOLS, not VS150COMNTOOLS

This comment has been minimized.

Copy link
@tannergooding

tannergooding Mar 6, 2017

Author Member

Fixed.

build.cmd Outdated
set __VSVersion=vs2017
) else (
set "__VSToolsRoot=%VS140COMNTOOLS%"
set "__VCToolsRoot=%VS150COMNTOOLS%\..\..\VC"

This comment has been minimized.

Copy link
@janvorli

janvorli Mar 6, 2017

Member

Here again: It looks like this should be VS140COMNTOOLS, not VS150COMNTOOLS

This comment has been minimized.

Copy link
@tannergooding

tannergooding Mar 6, 2017

Author Member

Fixed. I am revalidating that everything builds (both standalone and with VS2015/VS2017 SxS) and will repush once complete.

@tannergooding tannergooding force-pushed the tannergooding:vs2017 branch from 01f21bf to 029c38f Mar 6, 2017

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2017

@janvorli, I've resolved the issue and validated that everything continues to build (both standalone and SxS).

@gkhanna79

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Are you able to build all the Priority 1 tests in VS 2017?

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2017

I hadn't tested that yet. They are building now and I will update once that completes.

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2017

@gkhanna79, Priority 1 (and Priority 2) tests are building in VS2015/VS2017 standalone and in VS2015/VS2017 SxS configurations.

@gkhanna79

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

@tannergooding We only have Priority 0 (which build by default as part of the Windows build) and Priority 1 tests (the complete set that is > 11K right now). Can you confirm the count?

What is priority 2?

@weshaggard

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

It looks like Microsoft.DotNet.BuildTools.CoreClr is not a package we actively produce anymore (we haven't published it in about a year). I'm not certain where this was published from. Perhaps @weshaggard has some historical context for this package.

That package was built in our TFS branches and if we need to update it we would need to rebuild it from there (or port it to the open as I'm not sure what all it contains).

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2017

@gkhanna79, I'm not sure if we actually have any priority 2 tests, but Priority 2 is given as an example in our test-configuration documentation: https://github.com/dotnet/coreclr/blob/32f0f9721afb584b4a14d69135bea7ddc129f755/Documentation/building/test-configuration.md#priority, so I built it as well to be safe.

What is the best way to get the test count? I don't see anything obvious printed out by the command line or in the build logs...

@gkhanna79

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

@tannergooding If you run the tests, post build, you should see the count.

@wtgodbe

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Are we definitely fine with deprecating support for VS2013 users? If no one objects (certainly fine by me), then LGTM

@gkhanna79

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

We do not support VS 2013 already. However, we are continuing to support VS 2015 and add support for VS 2017 - the support for former should not be removed.

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2017

@gkhanna79, there are 11176 tests for both standalone and SxS runs.

@wtgodbe, I dropped it since we didn't support building on VS2013 anyways (the build script had no support for this). We only had support for running tests under VS2013 (which isn't very useful if you can't build them).

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2017

@gkhanna79, are there any other checks you would like me to do here?

@dpodder

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2017

@tannergooding as of a few days ago there is now also a tool that can find VS2017 dynamically without having to call the managed query APIs directly: Microsoft/vswhere. I'm thinking that can make a follow-up PR to support a plain CMD environment again much easier.

set __VSToolsRoot=%VS140COMNTOOLS%

:: Default to highest Visual Studio version available
if defined VS150COMNTOOLS (

This comment has been minimized.

Copy link
@gkhanna79

gkhanna79 Mar 7, 2017

Member

I think we should not build the policy to pickup the latest version by default but rather have the user specify it. I think we should introduce a command line argument to specify picking up VS2017 so that there is no guesswork around which VS got used for building the repo.

The default is always VS 2015 and VS 2017 should be opt-in.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Mar 7, 2017

Author Member

This is how it is today. VS150COMNTOOLS is only defined if either:
A) The user has manually defined it or
B) The user is running from a VS2017 Developer Command Prompt

In either case, I would argue the user has explicitly opted into building using the VS2017 toolset.

This comment has been minimized.

Copy link
@gkhanna79

gkhanna79 Mar 7, 2017

Member

CoreCLR build should be done from a barebones command prompt - this is why VCVarsall.bat is invoked so that it sets the right environment. We cannot (and should not) assume a specific command prompt to be set.

As a result, from a clean command prompt, there is no VS 2017 variables defined and thus, no opted in. The only opt-in works when we explicitly specify one.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Mar 7, 2017

Author Member

Running from a regular command prompt depends on the VS***COMNTOOLS environment variable to be defined.

For VS2015 and prior, this is a global environment variable set by the vs installer. For VS2017 and later, there is no global environment variable and no registry keys. In order to get VS150COMNTOOLS you have to run developer command prompt.

Launching vcvarsall.bat itself is dependent on us being in developer command prompt as well (so we can locate the script).

The logic I have here is: are you in the vs2017 command prompt?

If yes, assume vs 2017 tools.

Otherwise, use vs2015 tools.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Mar 7, 2017

Author Member

Additionally, even if we have manual detection for the user passing in the tool version they wish to use, we should fail if the user is already in a Developer Command Prompt and they have specified a different version. This is because the environment variables and path settings can overlap and lead to weird behaviors.

This comment has been minimized.

Copy link
@gkhanna79

gkhanna79 Mar 7, 2017

Member

Thanks for the explanation @tannergooding. In that case, please add a similar comment here to explain the same to the read as well.

This comment has been minimized.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Mar 8, 2017

Author Member

@gkhanna79, updated both.

@@ -3,7 +3,18 @@ setlocal EnableDelayedExpansion EnableExtensions

echo Starting Build at %TIME%
set __ThisScriptFull="%~f0"
set __VSToolsRoot=%VS140COMNTOOLS%

:: Default to highest Visual Studio version available

This comment has been minimized.

Copy link
@gkhanna79

gkhanna79 Mar 7, 2017

Member

Same as above.

run.cmd Outdated
@@ -2,7 +2,13 @@
setlocal

if not defined VisualStudioVersion (
if defined VS140COMNTOOLS (
if defined VS150COMNTOOLS (

This comment has been minimized.

Copy link
@gkhanna79

gkhanna79 Mar 7, 2017

Member

As above.

@@ -8,10 +8,10 @@ set __BuildOS=Windows_NT
set __MSBuildBuildArch=x64

:: Default to highest Visual Studio version available
set __VSVersion=vs2015
set __VSVersion=vs2017

This comment has been minimized.

Copy link
@gkhanna79

gkhanna79 Mar 7, 2017

Member

See previous comments that will affect this.

* Make sure that you install the ".NET Core Cross-Platform Development" workload.
* Make sure that you install the "Desktop Development with C++" workload.
* Make sure that the "VC++ 2017 v141 toolset (x86,x64)" component is selected in list of Optional components
* To build for Arm32, Make sure that the "Windows 10 SDK (10.0.14393.0)" component is selected in list of Optional components for the "Desktop Development with C++" workload.

This comment has been minimized.

Copy link
@gkhanna79

gkhanna79 Mar 8, 2017

Member

Why do we need to specify a specific SDK version as opposed to having a general guideline like https://github.com/dotnet/coreclr/pull/9956/files#diff-4b7d5829731d454157a0e13d9d46da42R20?

This comment has been minimized.

Copy link
@tannergooding

tannergooding Mar 8, 2017

Author Member

A specific version isn't required, but it is:

  • Selected by default in the Desktop Development with C++ workload
  • Is easily configured using the VS2017 installer
  • Has the most bug fixes and functionality of any of the SDK versions
  • Is currently the default version exposed on the downloads page linked in the VS2015 section (you have to go through several more links to download an older version)

This comment has been minimized.

Copy link
@tannergooding

tannergooding Mar 8, 2017

Author Member

I could change this to:

  • To build for Arm32, Make sure that you have the Windows SDK for Windows 10 installed. You can either ensure "Windows 10 SDK (10.0.14393.0)" component is selected in list of Optional components for the "Desktop Development with C++" workload or download the SDK separately here: Windows SDK for Windows 10

If you think that is an improvement

This comment has been minimized.

Copy link
@gkhanna79

gkhanna79 Mar 8, 2017

Member

The proposed change sounds good. I would suggest modifying it a bit to be something like "...for Windows 10 installed (or selected to be installed as part of VS installation). To explicitly install Windows SDK, download it from here: ...."

This comment has been minimized.

Copy link
@tannergooding

tannergooding Mar 8, 2017

Author Member

Updated.

@gkhanna79
Copy link
Member

left a comment

LGTM modulo the comment around Windows SDK.

@tannergooding tannergooding force-pushed the tannergooding:vs2017 branch from 2e9edbd to a87a662 Mar 8, 2017

For Visual Studio 2015:
* To debug managed code, ensure you have installed at least [Visual Studio 2015 Update 3](https://www.visualstudio.com/en-us/news/releasenotes/vs2015-update3-vs).
* Make sure that you install "VC++ Tools". By default, they will not be installed.
* To build for Arm32, you need to have [Windows SDK for Windows 10](https://developer.microsoft.com/en-us/windows/downloads) installed.

This comment has been minimized.

Copy link
@gkhanna79

gkhanna79 Mar 8, 2017

Member

Please unify the two set of instructions for Windows SDK.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Mar 8, 2017

Author Member

Fixed.

Updating the building/windows-instructions.md to indicate the support…
… for and requirements of using VS2017.

@tannergooding tannergooding force-pushed the tannergooding:vs2017 branch from a87a662 to af15826 Mar 8, 2017

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2017

@janvorli, do you have any other feedback here or am I good to merge?

@janvorli
Copy link
Member

left a comment

LGTM

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2017

Thanks, will merge once OSX passes.

@tannergooding tannergooding merged commit 767f6e0 into dotnet:master Mar 9, 2017

13 checks passed

CentOS7.1 x64 Debug Build and Test Build finished.
Details
FreeBSD x64 Checked Build Build finished.
Details
Linux ARM Emulator Cross Debug Build Build finished.
Details
Linux ARM Emulator Cross Release Build Build finished.
Details
OSX x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Windows_NT arm Cross Debug Build Build finished.
Details
Windows_NT arm Cross Release Build Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x86 Checked Build and Test Build finished.
Details

@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.