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

[cmake] always target Windows SDK v10.x when using Visual Studio generators (fixes #6448) #6451

Merged
merged 3 commits into from
May 30, 2024

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented May 9, 2024

Fixes #6448

Related to #5884.

Helps with #6213.

Proposes always targeting v10.x of the Windows SDK in CMake builds using the Visual Studio generators.

Notes for Reviewers

How I tested this

Building the R package, using Visual Studio 17 2022, on my Windows laptop running Windows 10 Home Edition.

Rscript build_r.R --no-build-vignettes

With rtools44 (https://cran.r-project.org/bin/windows/Rtools/rtools44/rtools.html) on PATH.

Without this change, CMake targets via 8.1 of the Windows SDK and detects the wrong Windows version.

Trying 'Visual Studio 17 2022'
-- Selecting Windows SDK version 8.1 to target Windows 6.2.9200.
-- The C compiler identification is unknown
-- The CXX compiler identification is unknown
CMake Error at CMakeLists.txt:28 (project):
-- Configuring incomplete, errors occurred!
  No CMAKE_C_COMPILER could be found.

That causes all of the Visual Studio and then the MSBuild solutions to fail, and therefore installation of the R package to fail.

With this change, it correctly targets v10 of the Windows SDK.

installing via 'install.libs.R' to C:/Users/James/Documents/R/win-library/4.4/00LOCK-lightgbm/00new/lightgbm
Trying 'Visual Studio 17 2022'
-- Selecting Windows SDK version 10.0.19041.0 to target Windows 10.0.
-- The C compiler identification is MSVC 19.33.31630.0
-- The CXX compiler identification is MSVC 19.33.31630.0
...

References

@jameslamb jameslamb changed the title WIP: [cmake] always target Windows SDK v10.x when using Visual Studio generators [cmake] always target Windows SDK v10.x when using Visual Studio generators May 9, 2024
@jameslamb jameslamb changed the title [cmake] always target Windows SDK v10.x when using Visual Studio generators [cmake] always target Windows SDK v10.x when using Visual Studio generators (fixes #6448) May 9, 2024
@jameslamb jameslamb marked this pull request as ready for review May 9, 2024 15:33
@david-cortes
Copy link
Contributor

@jameslamb Thanks for looking into this.

Some questions:

  • What happens if the platform is windows 11 or windows server?
  • Does lightgbm make any usage of the windows OS API that would make it require linkage to the windows SDK? As far as I am aware, all the functionalities for things like GPU, distributed training, etc. are handled by linking to external dependencies.

@jameslamb
Copy link
Collaborator Author

What happens if the platform is windows 11 or windows server?

Good question!

My understanding is that v10 of the SDK supports Windows 11 and most versions of Windows Server.

See "Supported Operating Systems" here: https://developer.microsoft.com/en-us/windows/downloads/windows-sdk/.

For example, the R-package MSVC tests here are targeting Windows Server 2019 and Windows 2022.

https://github.com/actions/runner-images?tab=readme-ov-file#available-images

# Visual Studio 2019
- os: windows-2019
task: r-package
compiler: MSVC
toolchain: MSVC
r_version: 3.6
build_type: cmake
container: null
# Visual Studio 2022
- os: windows-2022
task: r-package
compiler: MSVC
toolchain: MSVC
r_version: 4.3
build_type: cmake
container: null

Does lightgbm make any usage of the windows OS API that would make it require linkage to the windows SDK?

Not 100% sure (Windows-specific development is not something I have a lot of experience in), but I think that winsock2, for example, comes through the Windows SDK.

#include <winsock2.h>
#include <ws2tcpip.h>
#include <iphlpapi.h>

https://learn.microsoft.com/en-us/windows/win32/winsock/creating-a-basic-winsock-application

If you see some other alternative to simplify LightGBM's Windows builds here I'd welcome it.

@david-cortes
Copy link
Contributor

It compiles correctly with msvc after this PR, but with some caveats: if compiling for a second time, it hangs indefinitely without issuing any message (just stuck in installing the package to build vignettes). It looks like it requires killing the whole console application and then launching a new one in order to successfully install for a second time. Not sure if it has anything to do with the changes here though.

@jameslamb
Copy link
Collaborator Author

It looks like it requires killing the whole console application and then launching a new one in order to successfully install for a second time.

Oh interesting, I haven't seen that before. I'm not sure what the cause of that might be.

You can skip building the vignettes (and therefore compiling the library twice) by passing --no-build-vignettes:

Rscript build_r.R --no-build-vignettes

We have vignette-building on by default so that the package tarball that's produced is suitable to pass to R CMD check --as-cran. But for work like #6213 where you're primarily interested in just compiling the library and using it or running the unit tests, the vignettes are unnecessary.

@jameslamb
Copy link
Collaborator Author

@guolinke or @shiyu1994 could you help with a review of this?

Copy link
Collaborator

@guolinke guolinke left a comment

Choose a reason for hiding this comment

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

LGTM

@jameslamb jameslamb merged commit c07694b into master May 30, 2024
40 checks passed
@jameslamb jameslamb deleted the fix/windows-sdk-version branch May 30, 2024 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CMake __BUILD_FOR_R fails to find R source files
3 participants