-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
[CI] Add Windows GPU to Jenkins CI pipeline #4463
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit strange having to call it more than once. Maybe these flags get reset in places. Works on my local machine. This will do for now though so we can get 0.9 out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's stay with this hack for now. Looks good to me except for whether is MSVC getting the right c++ version.
@RAMitchell @trivialfis I'm asking you to review again, now that I added a CI pipeline for Windows GPU target. See example at https://xgboost-ci.net/blue/organizations/jenkins/xgboost-win64/detail/PR-4463/10/pipeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good thanks Philip! Do you plan to add tests, or should we do this later?
@RAMitchell I'll add tests in a follow-up pull request. |
I keep seeing the error
(https://xgboost-ci.net/blue/organizations/jenkins/xgboost-win64/detail/PR-4463/17/pipeline) And the funny thing is that the error goes away when I run |
This is interesting: apache/mxnet#12563. NMake JOM is supposedly faster than MSBuild. Let me try it. |
Actually I'm seeing the same issue with JOM. And I'm not seeing any significant build speedup with JOM. For now, I'm resorting to running msbuild 3 times to stamp out spurious errors like LNK1104. |
Looks like running MSBuild 3 times fixed the issue for now. I'll merge this after all tests pass. |
You can fix this issue by adding a dependency between targets that are conflicting: https://cmake.org/cmake/help/v3.8/command/add_dependencies.html This forces certain operations to finish before others. |
@RAMitchell Nice. Thanks for the pointer. I spent quite some time to try to fix the issue. Indeed, the old CMakeLists.txt used to have Line 302 in 3078b59
I'll go ahead and add it back. |
@RAMitchell Yes! The LNK1104 error is now fixed for good. I can now sleep better tonight |
Merged. Thanks all |
1. Add Windows GPU to Jenkins CI pipeline (see #4234). All pull requests will now be tested to ensure compatibility with Windows platform.
2. Fix #4462: Use
/MT
flag consistently so that MSVC builds don't failBackground: Microsoft Visual C++ (MSVC) offers multiple options for the C++ runtime:
/MT
(static runtime) and/MD
(shared DLL runtime). XGBoost has traditionally used/MT
to compile all sources. The utility functionmsvc_use_static_runtime()
is defined incmake/Utils.cmake
to replace all occurrences of/MD
with/MT
.Diagnosis of #4462: Recently, #4323 revised CMakeLists.txt extensively so as to incorporate CUDA as a first-class language. The same pull request also split off
objxgboost
target as a separate CMake module. For some unknown reasons,objxgboost
was using/MD
, whereas the rest of XGBoost was using/MT
.Fix (more of a hack actually): Call
msvc_use_static_runtime()
multiple times to completely remove/MD
.Fixes #4462.