Enable build with Visual Studio 2017 by wjk · Pull Request #2390 · dotnet/corert

wjk · 2016-12-19T23:06:25Z

This PR enables building (the native parts of) CoreRT using Visual Studio 2017 RC.

Closes #2383.

This version is the first version that supports generating a project using the Visual Studio 2017 toolchain.

wjk · 2016-12-19T23:08:26Z

@jkotas I had to update the minimum CMake version to 3.7.0 to enable compilation with VS2017, as earlier versions don't support that version of VS. However, I presume that the CI machines don't have this version of VS installed, as all 6 checks failed as I was writing this. However, this version is only required when building for VS2017; earlier versions (or non-VS build systems) will still work with the older version previously used. Unfortunately, I don't know how to express this in CMake. Could you give me a few pointers? Thanks!

wjk · 2016-12-19T23:10:16Z

In addition, I cannot yet successfully build CoreRT using VS2017, because when the build script sources vcvarsall.bat, it places a copy of CMake on the front of the PATH that is too old (version 3.6.20160606-g836e2-dirty-MSVC, to be precise). This prevents the command prompt from using the 3.7.0 version of CMake that I previously installed separately. Could you help me with this as well?

jkotas · 2016-12-19T23:32:28Z

Try something like:

if(NOT(CMAKE_VERSION VERSION_LESS 3.7.0))
    message(FATAL_ERROR "CMake version 3.7.0 or higher is required to compile with VS2017")
endif()

jkotas · 2016-12-19T23:38:39Z

when the build script sources vcvarsall.bat, it places a copy of CMake on the front of the PATH that is too old (version 3.6.20160606-g836e2-dirty-MSVC, to be precise

Can we just use this version of CMake that comes with VS2017? Or is it not possible for some reason?

wjk · 2016-12-20T01:13:44Z

@jkotas According to the CMake website, CMake 3.7.0 is the minimum required version for VS2017 support. I will test on the version of CMake that is bundled, however. I have researched how to check the generator being used, and will push a commit later tonight that only requires a higher CMake version when targeting VS2017.

Unfortunately, the generated solution then does not build correctly. I am still looking into this.

The code previously assumed that the probe-win.ps1 script is located in the current working directory. During my testing, I found that this is not robust enough for all use-cases. Using %~dp0 to identify the directory containing the batch file is more robust against this issue.

wjk · 2016-12-20T01:45:54Z

@jkotas Note: The Ubuntu builds are currently failing with the following message:

There was an error processing the xunit test results: [xUnit] [ERROR] - No test reports found for the metric 'xUnit.Net' with the resolved pattern '**/testResults.xml'. Configuration error?.

I don't know what this means, but it seems unrelated to these changes.

wjk · 2016-12-20T01:51:13Z

I have fixed the build to function correctly using the CMake version included with VS2017. I have adjusted the version check to 3.6.0 accordingly.

However, I am running into a very strange issue: When the Visual Studio files generated by CMake are built by the automated build-native.cmd file, the build fails immediately complaining that The build tools for v141 (PlatformToolset = 'v141') cannot be found. Yet, when I build the exact same MSBuild files from the command line (using msbuild /nologo INSTALL.vcxproj), the build does not encounter this problem, and in fact completes successfully with no issues! Can you please help me resolve this?

Only the copy of MSBuild shipped with VS2017 can successfully build VS2017 vcxproj files. Using any other copy will cause strange errors.

wjk · 2016-12-20T02:01:16Z

My latest commit fixes the problem mentioned in my comment above regarding the PlatformToolset; please disregard that.

wjk · 2016-12-20T02:08:14Z

And the CoreRT build cycle (native and managed) completes successfully on my computer with no issues! I think this one may be ready to merge. Note that the CI system won't test these changes, as this is an optional feature that must be explicitly enabled by the user.

jkotas · 2016-12-20T02:22:00Z

There was an error processing the xunit test results

This is cascading error. The primary error is:

18:00:23 CMake Error at functions.cmake:11 (string):
18:00:23   string end index: 21 is out of range -1 - 14
18:00:23 Call Stack (most recent call first):
18:00:23   CMakeLists.txt:7 (string_has_prefix)

jkotas · 2016-12-20T02:35:13Z

Also, if I just fetch your branch and run "build.cmd", it fails with:

'_msbuildexe' is not recognized as an internal or external command, operable program or batch file.

Third time's the charm, right?

@jkotas

This should fix the problem @jkotas is reporting, but I cannot be certain.

wjk · 2016-12-20T03:11:34Z

@jkotas Try it now. I have fixed the Ubuntu build issue, and I should have fixed the issue you are having. (I have confirmed that it no longer complains of the issue you reported, but I cannot make that specific configuration build on my PC. This is because the only version of CMake I have now is the one in VS2017; I uninstalled the other because I didn't think I would need it anymore.) Sorry about that!

jkotas · 2016-12-20T04:33:30Z

This batch file variable is still misbehaving if I do "build.cmd vs2017". Here is output with set _echo=on:

 if not exist !_msbuildexe! (set _msbuildexe="C:\Program Files\MSBuild\14.0\Bin\MSBuild.exe" )
 if not exist !_msbuildexe! (echo Error: Could not find MSBuild.exe.  Please see https://github.com/dotnet/corert/blob/master/Documentation/prerequisites-for-building.md for build instructions.   && exit /b 1 )
)
The system cannot find the drive specified.
The system cannot find the drive specified.
The system cannot find the drive specified.

jkotas · 2016-12-20T04:34:03Z

@janvorli Could you please take a look at the CMake changes?

jkotas · 2016-12-20T04:43:55Z

buildscripts/buildvars-setup.cmd


 set __VSProductVersion=
 if /i "%__VSVersion%" == "vs2015" set __VSProductVersion=140
+if /i "%__VSVersion%" == "vs2017" set __VSProductVersion=150


It would be nice to mention vs2017 in the help text at the end of the file.

And update https://github.com/dotnet/corert/blob/master/Documentation/prerequisites-for-building.md too

wjk · 2016-12-20T04:51:25Z

@jkotas Regarding those drive-not-found errors: I've seen those, and have no idea what is causing them. Since they seem to have no effect on the build, I was ignoring them, but I understand how they should be corrected before this PR is merged. I will get on the documentation sometime tomorrow.

janvorli · 2016-12-20T09:43:01Z

src/Native/CMakeLists.txt

 # Include cmake functions
 include(functions.cmake)

+string_has_prefix(${CMAKE_GENERATOR} "Visual Studio 15 2017" GENERATOR_IS_VS2017)


You don't need to introduce a new function. string(FIND ${CMAKE_GENERATOR} "Visual Studio 15 2017" VS2017_POS) and comparing VS2017_POS to 0 would work for this purpose.

@janvorli Thank you! For some reason I couldn't find that in the documentation. I will get that fixed later today.

jkotas · 2016-12-21T05:14:20Z

buildscripts/build-native.cmd

+
+:: VS2017 changed the location of vcvarsall.bat.
+if /i "%__VSVersion%" == "vs2017" (
+	call "!VS%__VSProductVersion%COMNTOOLS!\..\..\VC\Auxiliary\Build\vcvarsall.bat" %__VCBuildArch%


Nit: tabs -> spaces

Just to be precise, how many spaces?

4 spaces per tab

jkotas · 2016-12-22T20:01:41Z

drive-not-found errors: I've seen those, and have no idea what is causing them.

They are caused by the :: tagged comment inside the if block that you have added. You can fix them by using the old fashioned rem instead of :: for the comment.

The Windows batch file interpreter treats these two symbols differently. When "::" is used within an if block, it can create spurious drive-not-found errors. Using "rem" avoids this problem.

jkotas · 2016-12-23T02:35:56Z

Thank you!

This is a followup to dotnet#2390 that fixes a few things: * vs2017 no longer needs to be passed to build.cmd (it will be autodetected) * Building and running tests now works * Documentation updates to point out running from Developer Command prompt is needed. Fixes dotnet#3394.

This is a followup to #2390 that fixes a few things: * vs2017 no longer needs to be passed to build.cmd (it will be autodetected) * Building and running tests now works * Documentation updates to point out running from Developer Command prompt is needed. Fixes #3394.

William Kent added 4 commits December 19, 2016 17:48

Add support for VS2017 in buildvars-setup.cmd

9029ad4

Add support for VS2017 in gen-buildsys-win.cmd

944c8ac

VS2017 rearranges some files, update build-native.cmd to match

634aa29

Update minimum CMake version to 3.7.0

117fabd

This version is the first version that supports generating a project using the Visual Studio 2017 toolchain.

dnfclas added the cla-already-signed label Dec 19, 2016

William Kent added 4 commits December 19, 2016 20:25

Only require CMake 3.7.0 when targeting VS 2017

657392f

Fix CMake generator syntax

b0740bc

Fix CMake script when using VS2017 bundled CMake

03d9aaf

Unfortunately, the generated solution then does not build correctly. I am still looking into this.

Use the correct copy of MSBuild to build the C++ projects

1e5390f

Only the copy of MSBuild shipped with VS2017 can successfully build VS2017 vcxproj files. Using any other copy will cause strange errors.

William Kent added 4 commits December 19, 2016 21:54

Fix build on non-Windows targets

2925136

*Really* fix build on non-Windows targets

1d58f2b

Fix non-Windows build yet again

984c60e

Third time's the charm, right?

Fix batch-file variable expansion

6fb8ee0

This should fix the problem @jkotas is reporting, but I cannot be certain.

jkotas reviewed Dec 20, 2016

View reviewed changes

janvorli suggested changes Dec 20, 2016

View reviewed changes

Use string(FIND) instead of a custom CMake function

61a4fe2

jkotas reviewed Dec 21, 2016

View reviewed changes

janvorli approved these changes Dec 21, 2016

View reviewed changes

William Kent added 2 commits December 21, 2016 18:37

Tabs to spaces

ca6fd43

Update documentation

00b7569

wjk mentioned this pull request Dec 22, 2016

Virtual method use optimization doesn't work for calls that are not through the declaring slot #2392

Closed

Use "rem" for comments instead of "::"

1ab1829

The Windows batch file interpreter treats these two symbols differently. When "::" is used within an if block, it can create spurious drive-not-found errors. Using "rem" avoids this problem.

jkotas merged commit 13f9167 into dotnet:master Dec 23, 2016

wjk deleted the vs2017-build branch December 23, 2016 03:29

MichalStrehovsky mentioned this pull request Apr 26, 2017

Add full support for building with VS 2017 #3436

Merged

Conversation

wjk commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjk commented Dec 19, 2016

Uh oh!

wjk commented Dec 19, 2016

Uh oh!

jkotas commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Dec 19, 2016

Uh oh!

wjk commented Dec 20, 2016

Uh oh!

wjk commented Dec 20, 2016

Uh oh!

wjk commented Dec 20, 2016

Uh oh!

wjk commented Dec 20, 2016

Uh oh!

wjk commented Dec 20, 2016

Uh oh!

jkotas commented Dec 20, 2016

Uh oh!

jkotas commented Dec 20, 2016

Uh oh!

wjk commented Dec 20, 2016

Uh oh!

jkotas commented Dec 20, 2016

Uh oh!

jkotas commented Dec 20, 2016

Uh oh!

jkotas Dec 20, 2016

Choose a reason for hiding this comment

Uh oh!

jkotas Dec 20, 2016

Choose a reason for hiding this comment

Uh oh!

wjk commented Dec 20, 2016

Uh oh!

janvorli Dec 20, 2016

Choose a reason for hiding this comment

Uh oh!

wjk Dec 20, 2016

Choose a reason for hiding this comment

Uh oh!

jkotas Dec 21, 2016

Choose a reason for hiding this comment

Uh oh!

wjk Dec 21, 2016

Choose a reason for hiding this comment

Uh oh!

janvorli Dec 21, 2016

Choose a reason for hiding this comment

Uh oh!

jkotas commented Dec 22, 2016

Uh oh!

jkotas commented Dec 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wjk commented Dec 19, 2016 •

edited

Loading

jkotas commented Dec 19, 2016 •

edited

Loading