Fix KDevelop's detection for Visual Studio 2017 compiler and tools.
AbandonedPublic

Authored by kfunk on Oct 5 2018, 11:06 PM.

Details

Reviewers
Petross404
Group Reviewers
KDevelop
Summary

Visual Studio 2017 defines the VS150COMNTOOLS variable which kdevelop-msvc.bat ignored till now.

Also VS17 has a new folder layout, so vcvarsall.bat is under a different location. An if-else finds the script accordingly.

Last but not least clicking kdevelop-msvc.bat doesn't initialize our MSVC enviroment; one has to find and run the script from Developer Command Prompt!

Diff Detail

Repository
R32 KDevelop
Lint
Lint Skipped
Unit
Unit Tests Skipped
Petross404 requested review of this revision.Oct 5 2018, 11:06 PM
Petross404 created this revision.
arrowd added a subscriber: arrowd.Oct 6 2018, 8:22 AM
arrowd added inline comments.
app/windows/kdevelop-msvc.bat
22

Not sure about x64 part. What if user wants x32 project? Or that is arch of the toolchain itself?

Petross404 added a comment.EditedOct 6 2018, 11:39 AM

Not sure about x64 part. What if user wants x32 project? Or that is arch of the toolchain itself?

@arrowd Yes, I forgot to mention the most important problem. vcvarsall.bat now requires this kind of arguments. For example without the x64 argument I get this:

[ERROR:vcvarsall.bat] Error in script usage. The correct usage is:
Syntax:
    vcvarsall.bat [arch] [platform_type] [winsdk_version] [-vcvars_ver=vc_version]
where :
    [arch]: x86 | amd64 | x86_amd64 | x86_arm | x86_arm64 | amd64_x86 | amd64_arm | amd64_arm64
    [platform_type]: {empty} | store | uwp
    [winsdk_version] : full Windows 10 SDK number (e.g. 10.0.10240.0) or "8.1" to use the Windows 8.1 SDK.
    [vc_version] : {none} for default VS 2017 VC++ compiler toolset |
                   "14.0" for VC++ 2015 Compiler Toolset |
                   "14.1x" for the latest 14.1x.yyyyy toolset installed (e.g. "14.11") |
                   "14.1x.yyyyy" for a specific full version number (e.g. 14.11.25503)

The store parameter sets environment variables to support Universal Windows Plat
form application
development and is an alias for 'uwp'.

For example:
    vcvarsall.bat x86_amd64
    vcvarsall.bat x86_amd64 10.0.10240.0
    vcvarsall.bat x86_arm uwp 10.0.10240.0
    vcvarsall.bat x86_arm onecore 10.0.10240.0 -vcvars_ver=14.0
    vcvarsall.bat x64 8.1
    vcvarsall.bat x64 store 8.1

Please make sure either Visual Studio or C++ Build SKU is installed.
ERROR: The system was unable to find the specified registry key or value.

C:\Program Files (x86)\Microsoft Visual Studio\2017\Community>

I can't think of an interactive way to let the user choose the environment. Please enlighten me.

kfunk accepted this revision.Oct 8 2018, 1:58 PM
kfunk added a subscriber: kfunk.

Looks good to me, please push to 5.3 branch.

Unfortunately we have no possibility to switch between 64- and 32-bit right now; the .bat script is some hacky solution anyway, for the lack of compiler kits/etc. in KDevelop...

This revision is now accepted and ready to land.Oct 8 2018, 1:58 PM

@kfunk As I already have mentioned, clicking the script (finding it in Start menu) doesn't initialize correctly the environment. It has to be run from within Developer Command Prompt or the complete path of MSCV binaries (for the wanted target) has to be in %PATH%.

Is this by design? Perhaps we could fire up Developer Command Prompt with a small script and pass kdevelop-msvc.bat as an argument to the former.

kfunk requested changes to this revision.Oct 8 2018, 6:49 PM

Ah, right, I missed that part. The problem is that VS2017 by default doesn't set VS150COMNTOOLS as system-wide env variable.

@Petross404 Maybe, for the time being, something like this here could help:
https://github.com/dotnet/roslyn-analyzers/commit/f9ee28e16f253a7da9eee6f45e32251a41adb538 (just the if-cascade at the bottom, and without the if "%VS150COMNTOOLS%" EQU "" obviously)

Would you want to give this a try?

This revision now requires changes to proceed.Oct 8 2018, 6:49 PM
Petross404 updated this revision to Diff 43178.Oct 9 2018, 12:20 AM
  1. The script (sadly) uses hardcoded paths for vcvarsall.bat, instead of needing VS150COMNTOOLS. This way it can be found and executed, without the need of Developer's Command Prompt.
  1. The script asks the user which architecture KDevelop should be initialized for.

People with Visual Studio other than Community Edition, please consider trying out these changes. If a typo or a bug exists, it should be found early.

Thanks.

kfunk requested changes to this revision.Oct 9 2018, 8:29 AM
kfunk added inline comments.
app/windows/kdevelop-msvc.bat
22

I think this can be simplified quite a bit: I would move this whole block before line 3 (but do not call vcvarsall.bat yet, so we can reuse most of the logic from line 20ff.

I'll try to work on it later the day, probably easier if I just show the patch I have in mind.

This revision now requires changes to proceed.Oct 9 2018, 8:29 AM
Petross404 added a comment.EditedOct 9 2018, 5:17 PM

Can a windows user with VS installed, give me the content of HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\Microsoft\VisualStudio\SxS\VS7?

Petross404 updated this revision to Diff 43248.Oct 9 2018, 7:55 PM

Our script now initializes the environment correctly (at least for VS2017) even if the user doesn't run it from Developers Command Prompt. It reads Registry to find the path under which VS2017 is installed (I fear that I read the wrong variable from the Registry).

The script asks for the architecture when it's ran by clicking but it doesn't when it's ran from DCP. I didn't want to add more lines.

When kdevelop-msvc.bat is dealing with VS2017 and VS150COMNTOOLS defined or older VS versions, it makes use of the script variable, while when dealing with VS2017 without VS150COMNTOOLS defined (i.e. kdevelop-msvc.bat was clicked) it reads the Registry and asks for architecture.

kfunk requested changes to this revision.Oct 11 2018, 6:44 AM

Hey @Petross404 -- the other day I reworked that script a bit, here's the result: https://phabricator.kde.org/D16123 -- I refactored it this way before seeing the new revision of yours. I do think my version is a little bit easier to grasp, since it does not have that many branches compared to yours.

Mind reviewing my patch and probably choose that one?

This revision now requires changes to proceed.Oct 11 2018, 6:44 AM
kfunk added a comment.Oct 23 2018, 6:39 PM

Superseded by https://phabricator.kde.org/D16123.

@Petross404 If you want to reapply the registry-reading on top of that, feel free to do so. Sorry, but I dont think I'll have time to test that. :(

kfunk commandeered this revision.Oct 23 2018, 6:39 PM
kfunk abandoned this revision.
kfunk edited reviewers, added: Petross404; removed: kfunk.

@Petross404 If you want to reapply the registry-reading on top of that, feel free to do so. Sorry, but I dont think I'll have time to test that. :(

@kfunk I will give it a shot but I am working daily for many hours + I haven't Windows installed now. If it's ok for this to take some time, I am more than happy to help.

brauch added a subscriber: brauch.Oct 25 2018, 2:57 PM

Any help on the Windows version is greatly appreciated, whenever you find the time for it. :)