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

[python][R][docs][ci] better compatibility with Visual Studio 2019 #2083

Merged
merged 13 commits into from Apr 29, 2019

Conversation

StrikerRUS
Copy link
Collaborator

@StrikerRUS StrikerRUS commented Apr 7, 2019

This PR is related to #1956.

VS 2019 has been released recently and the latest CMake 3.14.1 supports it. Therefore we can reorder generators (and Platform Toolsets) to try to build LightGBM with VS 2019 firstly.

Due to that CMake dropped the possibility to specify architecture inside generator name (Win64), we can set it via -A option, which was presented in CMake since 3.1 and we require >=3.8 for Windows (it's shorter and better than setting CMAKE_GENERATOR_PLATFORM directly and allows to handle all generators in the same way).

I'm doubt about only one thing: whether we need to specify -T host= option or not. Its default behavior differs in VS 2019 compared to all other versions:
2019:

By default this generator uses the 64-bit variant on x64 hosts and the 32-bit variant otherwise.

2017:

By default this generator uses the 32-bit variant even on a 64-bit host.

This option controls the following:

-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Enterprise/VC/Tools/MSVC/14.16.27023/bin/Hostx86/x64/cl.exe -- works
                                                                                                                               |------|
                                                                                                                                  ^
                                                                                                                                  |
                                                                                                                                  v
                                                                                                                               |------|
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Enterprise/VC/Tools/MSVC/14.16.27023/bin/Hostx64/x64/cl.exe -- works

Refer to https://gitlab.kitware.com/cmake/cmake/issues/17451#note_341418

Remaining things:

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure the other maintainers know more about CMake than me, but did my best to read the diff and the provided links, and all makes sense to me! Thank you.

@StrikerRUS
Copy link
Collaborator Author

Here is a couple of links and quotes for anyone who sees -T host= option for the first time, just like me:

  • If I open VS2015 x64 Native Tools Command Prompt then where cl.exe produces C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\amd64\cl.exe.
  • If I open VS2015 x86 x64 Cross Tools Command Prompt then where cl.exe produces C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\x86_amd64\cl.exe.

Milestone
3.8.0

means that we may safely use this option.

The 32-bit and 64-bit tools generate identical code, but the 64-bit tools support more memory for precompiled header symbols and the Whole Program Optimization (/GL and /LTCG) options.

@henry0312
Copy link
Contributor

I don't know much about Windows.

@henry0312 henry0312 removed their request for review April 10, 2019 14:21
@StrikerRUS
Copy link
Collaborator Author

@Laurae2 Link to MSVC Redistributable 2019 has been added.

Copy link
Contributor

@Laurae2 Laurae2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems OK on Windows with VS 2019 here.

@StrikerRUS
Copy link
Collaborator Author

@guolinke @Laurae2 Thank you for review!

What're your opinions about -T host=? Do we need to suggest this flag for users or/and use it at CI tests?

@guolinke
Copy link
Collaborator

@StrikerRUS
It was the hostx86/x64/cl.exe before this PR:
image

It seems there are no differences in the generated codes, from this https://docs.microsoft.com/en-us/cpp/build/how-to-enable-a-64-bit-visual-cpp-toolset-on-the-command-line?view=vs-2019.
So I think the hostx86 is enough for now.

@StrikerRUS
Copy link
Collaborator Author

@guolinke Yeah, you're absolutely right! It's all about compiler performance (speed, ability to compile huge projects, etc.). The code itself should be identical.

I agree, let's leave 32bit compilers for now.

@StrikerRUS
Copy link
Collaborator Author

OK, I think it's ready to merge.

@StrikerRUS StrikerRUS merged commit 8d2ec69 into master Apr 29, 2019
@StrikerRUS StrikerRUS deleted the vs branch April 29, 2019 10:39
@lock lock bot locked as resolved and limited conversation to collaborators Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants