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
Added LightGBM JAVA SWIG wrapper support for windows #1599
Conversation
Hi @imatiach-msft ! Is it possible to make the following minor fix for Linux in this PR too?
And what about MinGW on Windows? Should it work potentially? UPD:
I believe that someone with macOS from #1326 can help to test. |
@StrikerRUS sorry I'm not sure how to fix the aliasing errors - all of the SWIG code is autogenerated. What is the preferred way to fix them, for example for this error:
my guess is that I should change the generated code to instead look like:
|
mingw should work but I haven't tested it yet |
@StrikerRUS I ran it with mingw on windows but I got this error, for some reason the swig dll was not generated (I do see the lightgbm dll however):
|
@StrikerRUS hmm, actually it seems that it is deleting the lib_lightgbm_swig.dll, it looks like when compiling in mingw there is no "Release" directory generated and this fails: |
@imatiach-msft Yeah, MinGW places generated files right into the root folder of the project.
(https://lightgbm.readthedocs.io/en/latest/Installation-Guide.html#mingw-w64) As for warnings, lets leave them for another PR.
|
@StrikerRUS I was getting errors even when I removed the Release dir last night, maybe it has to do with how I installed mingw, I specified threads=posix (the default) whereas I should have specified win32 (?) |
We run tests with POSIX threads too...
Which one? |
even if I do this it seems to complain:
that's just copying everything to com folder... it seems it doesn't recognize com folder, weird |
I tried reinstalling mingw with win32 and running this but it didn't make a difference, seems that option doesn't have anything to do with paths (just with the threading library) |
Can you please try this fix? |
is the fix to remove git\usr\bin from path? |
also, how do I set:
I tried the following: |
Yep! Are you running tests at Appveyor, not locally? |
even when I removed from path and rebuilt I still got the error, here is my path:
this is the error:
|
I'm not running any tests I'm just trying to build lightgbm locally. I'm just running everything through powershell on my local machine. |
Maybe this one item in your PATH interrupts:
?
Got it! That fix affects only our test builds at Appveyor. It's like a tests script for CI service, and it's not connected with the LightGBM building process itself. |
still failing, this is the new path:
same error:
|
My assumption is that it cannot find Try to set |
here's the new path:
same error:
|
I think it's the same issue here https://stackoverflow.com/questions/33674973/makefile-error-make-e-2-the-system-cannot-find-the-file-specified Can you try to specify the full path of |
How about to avoid |
FYI this is the error for the add_library call:
|
Latest CMake https://cmake.org/cmake/help/v3.12/module/UseSWIG.html
|
Stupid proposal 😄 , but maybe just |
In addition to #1599 (comment) here is another way to copy file: https://stackoverflow.com/questions/24311402/copy-all-files-with-given-extension-to-output-directory-using-cmake |
I got this error when changing _lightgbm_swig -> _lightgbm:
with the following code:
|
the configure_file command doesn't seem to work because the cp needs to be done as part of the POST_BUILD task, hmmm |
I wonder if the fact that I have bash.exe in C:\Windows\System32 is messing mingw up (bash on windows) |
@StrikerRUS how did you build them? I had the lib_ in front:
did you have this fix included:
|
Yep. But it's for Visual Studio case and we're speaking about MinGW now. BTW, I have no idea why it works here for WinGW without specifying SWIG target
At Travis CI service this command didn't build SWIG part, only ordinal LightGBM dll. After changing to |
I don't know, maybe something wrong with my environment, but I think it's better to prevent build failures for other users who may have the same problems: diff --git a/CMakeLists.txt b/CMakeLists.txt
index da249c2..cab1a2a 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -177,15 +177,13 @@ if(USE_SWIG)
set_property(SOURCE swig/lightgbmlib.i PROPERTY SWIG_FLAGS "${swig_options}")
swig_add_module(_lightgbm_swig java swig/lightgbmlib.i)
swig_link_libraries(_lightgbm_swig _lightgbm)
- if(MSVC)
- set_target_properties(_lightgbm_swig PROPERTIES OUTPUT_NAME "lib_lightgbm_swig")
- endif(MSVC)
if(WIN32)
+ set_target_properties(_lightgbm_swig PROPERTIES OUTPUT_NAME "lib_lightgbm_swig")
if(MINGW OR CYGWIN) What do you think? |
I've found the reason of the
Now this change seems to be reasonable (and even required). Due to the latest JAVA environment I had to install latest CMake (3.12). That's why we have different behavior with you. |
you're change seems reasonable, I'll update the PR, good find! |
7d76a58
to
8a8fe38
Compare
"At Travis CI service this command didn't build SWIG part, only ordinal LightGBM dll. After changing to make _lightgbm_swig -j4 everything was OK." |
You are right.
didn't work, but
worked fine. But it was a month ago at Travis. I didn't re-checked this and thought that the same situation is with MinGW too. However, yesterday I discovered that
works OK. Let me do tests again and I'll back to you with results. PS. My intuition is that it's CMake version issue again (at Travis it's extremely old). UPD: UseSWIG: Do not set PREFIX property for SHARED and STATIC lua libraries |
CMakeLists.txt
Outdated
COMMAND "${Java_JAVAC_EXECUTABLE}" -d . java/*.java | ||
COMMAND cp "${PROJECT_SOURCE_DIR}/*.so" com/microsoft/ml/lightgbm/linux/x86_64 | ||
COMMAND "${Java_JAR_EXECUTABLE}" -cf lightgbmlib.jar com) | ||
set_target_properties(_lightgbm_swig PROPERTIES OUTPUT_NAME "lib_lightgbm_swig") |
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.
Can you please write a comment here, because someone might want to "optimize" this line in the future.
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.
done!
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.
did my comment make sense? what were you specifically looking for in the comment?
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.
Your comment is pretty good! Many thanks!
I'm so sorry, @imatiach-msft ! I've found that a month ago I ran tests with I've re-tested at Travis and everything is OK with default targets. Sorry again. |
Unfortunately, I've found a new bug. 😢 CMake 3.9.2 on Ubuntu produces
and need explicitly disable prefix somehow... |
maybe only execute that line on windows? |
hmm, I updated the PR, is there a particular corner case that I am missing? |
I'm afraid that it'll produce |
What do you think about the following "hack", which will hardcode the name and supposed to work for all CMake versions and platforms?
(Not sure about the order of these commands) |
"old CMake version and MinGW" |
Also, it'll protect us from any future changes in prefix policy. 💪 |
bd8695e
to
9636810
Compare
this works great! I verified this on: |
9636810
to
b41df0f
Compare
Let me enhance the list:
|
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.
Thank you very much, @imatiach-msft !
ping @guolinke
Thank you very much ! |
Thank you for this one. I could help here to test/validate on Mac. |
Added LightGBM JAVA SWIG wrapper support for windows OS
TODO: Add to Mac OS (need to find a Mac to validate this on)
To validate on windows (note, you must have SWIG and java sdk installed and JAVA_HOME environment variable must be set):
mkdir build
cd build
cmake -DCMAKE_GENERATOR_PLATFORM=x64 -DUSE_SWIG=ON ..
cmake --build . --target ALL_BUILD --config Release