-
-
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
Enable building rabit on Windows #6105
Conversation
Let’s remove the Rabit Makefile, since XGBoost doesn’t use Makefile. |
@hcho3 We have R package .. |
Ah so Rabit is not part of the amalgamation and it needs a Makefile. Got it. |
It seems autotools is picking mingw over msvc. |
3e87aa6
to
353bced
Compare
Turns out I can remove the makefile. |
ff24aaf
to
e6c88c8
Compare
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 me see if I understand. This PR is removing the dummy library that used to be linked into the windows Xgboost. Macros are being used instead for features that don't work on windows. Does rabit actually work in any way on windows or is it still disabled?
#endif // defined(_WIN32) | ||
|
||
#define IS_MINGW() defined(__MINGW32__) |
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.
Ifdef'ing everything seems quite a messy solution. Any alternatives?
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.
I don't have any other options.
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.
I can think of anything better, as you said at least it's constrained to one file.
No test is performed. Just trying to get rid of the
rabit_empty
target.